From cd1ece4beb55391a428c632b1fe609dc4748cb4e Mon Sep 17 00:00:00 2001 From: Luke Lau Date: Tue, 10 Nov 2026 03:47:42 +0800 Subject: [PATCH] :zap: Keep the translation flag on undo and redo of a move (#12870) * :zap: Keep the translation flag on undo and redo of a move Previously undos and redos always forced a component sync + thumbnail rerender for every component touched even if the underlying commits were just translations. This propagates the translation flag to undos and redos and propagates it to commit-changes. An entry merged from several undos/redos is a translation only if all of its undos/redos are translations. This reduces a UI freeze when undoing a move of a board with 453 components from 2.42s -> 0.22s. AI-assisted-by: claude-sonnet-5-5 * :translation? -> :translation --------- Co-authored-by: Andrey Antukh --- .../workspace-state-persistence-subtleties.md | 1 + frontend/src/app/main/data/workspace.cljs | 5 +- .../src/app/main/data/workspace/undo.cljs | 26 ++- .../composable_tests/interpreter.cljs | 5 +- .../data/workspace_undo_test.cljs | 3 +- .../data/workspace_undo_translation_test.cljs | 171 ++++++++++++++++++ frontend/test/frontend_tests/runner.cljs | 2 + .../frontend_tests/tokens/helpers/state.cljs | 5 +- 8 files changed, 204 insertions(+), 14 deletions(-) create mode 100644 frontend/test/frontend_tests/data/workspace_undo_translation_test.cljs diff --git a/.serena/memories/frontend/workspace-state-persistence-subtleties.md b/.serena/memories/frontend/workspace-state-persistence-subtleties.md index 7e8196080f..6d2b111eea 100644 --- a/.serena/memories/frontend/workspace-state-persistence-subtleties.md +++ b/.serena/memories/frontend/workspace-state-persistence-subtleties.md @@ -46,6 +46,7 @@ - `start-undo-transaction` stamps the transaction with a fresh `:commit-group` (dropped from the history entry); `commit-changes` uses `(or undo-group (:undo-group tx) (:commit-group tx) (uuid/next))`, so all commits of one transaction are one rollback unit. Pre-setting `:undo-group` instead breaks entries whose group is set later (sync transactions). - Undo transactions can stay open only temporarily; timed-out pending transactions are force-committed after about 20s. Undo entries are capped at 50. - Undo/redo are ignored while a normal editor/drawing interaction is active, except grid-layout edition handles undo through this path. `undo`, `redo` and `undo-to-index` also do nothing while the save lock holds: `materialize-undo` would move the index while the commit is refused. +- Undo entries carry a required boolean `:translation` (`schema:undo-entry` rejects an entry without it; `initialize-workspace` sets it when it records a local commit) and `undo`, `redo` and `undo-to-index` pass it back to `commit-changes`, so replaying a pure move skips component sync and thumbnails like the original commit did. Merging ANDs the flags: `stack-undo-entry`, `accumulate-undo-entry` (`empty-tx` starts at true) and undo groups in `undo-to-index` (`every?`). Test harnesses that mirror the recorder (`tokens/helpers/state.cljs`, `composable_tests/interpreter.cljs`) must pass it too. - After local commits and when render-wasm is active, text shapes get derived `:position-data` recomputed in a separate commit tagged `#{:position-data}`; that tag is excluded from the position-data watcher to avoid loops. ## Refs diff --git a/frontend/src/app/main/data/workspace.cljs b/frontend/src/app/main/data/workspace.cljs index 7581866dc6..792169a9df 100644 --- a/frontend/src/app/main/data/workspace.cljs +++ b/frontend/src/app/main/data/workspace.cljs @@ -592,13 +592,14 @@ (rx/filter dch/commit?) (rx/map deref) (rx/mapcat - (fn [{:keys [save-undo? undo-changes redo-changes undo-group tags stack-undo? selected-before]}] + (fn [{:keys [save-undo? undo-changes redo-changes undo-group tags stack-undo? selected-before translation?]}] (if (and save-undo? (seq undo-changes)) (let [entry {:undo-changes undo-changes :redo-changes redo-changes :undo-group undo-group :tags tags - :selected-before selected-before}] + :selected-before selected-before + :translation (boolean translation?)}] (rx/of (dwu/append-undo entry stack-undo?))) (rx/empty)))))) diff --git a/frontend/src/app/main/data/workspace/undo.cljs b/frontend/src/app/main/data/workspace/undo.cljs index 769f90a929..ea1b0c6e57 100644 --- a/frontend/src/app/main/data/workspace/undo.cljs +++ b/frontend/src/app/main/data/workspace/undo.cljs @@ -11,7 +11,7 @@ The undo stack has a maximum size of `MAX-UNDO-SIZE` (50) entries. Undo entries: - - Consist of `:undo-changes`, `:redo-changes`, `:undo-group`, `:tags` + - Consist of `:undo-changes`, `:redo-changes`, `:undo-group`, `:tags`, `:translation` - Can be: - `added`: pushes a new entry on top of the undo stack - `stacked`: appends undo/redo changes to the last undo item under @@ -67,6 +67,7 @@ [:tags [:set :keyword]] [:selected-before {:optional true} [:maybe [:set ::sm/uuid]]] [:selected-after {:optional true} [:maybe [:set ::sm/uuid]]] + [:translation :boolean] ;; When the entry was pushed onto the stack; used by the actions ;; history panel to show a relative timestamp next to each entry. ;; Issue #7660. @@ -140,7 +141,7 @@ exists, or creates a new entry if it doesn't. When stacking onto an existing entry, the entry's original timestamp is preserved so the history panel keeps showing when the action originated. Issue #7660." - [state {:keys [undo-changes redo-changes undo-group selected-after] :as entry}] + [state {:keys [undo-changes redo-changes undo-group selected-after translation] :as entry}] (let [index (get-in state [:workspace-undo :index] -1)] (if (>= index 0) (update-in state [:workspace-undo :items index] @@ -149,15 +150,18 @@ (update :undo-changes #(into undo-changes %)) (update :redo-changes #(into % redo-changes)) (update :undo-groups (fnil conj #{}) undo-group) + (update :translation #(and % translation)) (assoc :selected-after selected-after)))) (add-undo-entry state entry)))) (defn- accumulate-undo-entry "Extends the current undo transaction with new changes." - [state {:keys [undo-changes redo-changes undo-group tags selected-before selected-after]}] + [state {:keys [undo-changes redo-changes undo-group tags selected-before selected-after translation]}] (-> state (update-in [:workspace-undo :transaction :undo-changes] #(into undo-changes %)) (update-in [:workspace-undo :transaction :redo-changes] #(into % redo-changes)) + (update-in [:workspace-undo :transaction :translation] + #(and % translation)) (cond-> (nil? (get-in state [:workspace-undo :transaction :undo-group])) (assoc-in [:workspace-undo :transaction :undo-group] undo-group)) (update-in [:workspace-undo :transaction :undo-groups] (fnil conj #{}) undo-group) @@ -198,7 +202,7 @@ (add-undo-entry state entry)))))) (def empty-tx - {:undo-changes [] :redo-changes []}) + {:undo-changes [] :redo-changes [] :translation true}) (declare check-open-transactions) @@ -355,7 +359,12 @@ (into [] (mapcat :undo-changes) (reverse entries))]) :else - [[] []])] + [[] []]) + + translation (->> (subvec items + (inc (min dest-index index)) + (inc (max dest-index index))) + (every? :translation))] (when (seq redo-changes) (rx/of (materialize-undo redo-changes dest-index) ;; Undoable by a rollback, which also puts the index back @@ -363,7 +372,8 @@ :undo-changes undo-changes :undo-index index :origin it - :save-undo? false}))))))))))) + :save-undo? false + :translation? translation}))))))))))) (declare ^:private assure-valid-current-page) @@ -409,6 +419,7 @@ :undo-changes (:redo-changes item) :undo-index index :save-undo? false + :translation? (:translation item) :origin it}) (restore-selection (:selected-before item)) (assure-valid-current-page))))))))))) @@ -449,7 +460,8 @@ :undo-changes (:undo-changes item) :undo-index index :origin it - :save-undo? false}) + :save-undo? false + :translation? (:translation item)}) (restore-selection (:selected-after item)))))))))))) (defn- assure-valid-current-page diff --git a/frontend/test/frontend_tests/composable_tests/interpreter.cljs b/frontend/test/frontend_tests/composable_tests/interpreter.cljs index 905604466c..80934441cb 100644 --- a/frontend/test/frontend_tests/composable_tests/interpreter.cljs +++ b/frontend/test/frontend_tests/composable_tests/interpreter.cljs @@ -399,14 +399,15 @@ (rx/map deref) (rx/filter #(= :local (:source %))) (rx/mapcat - (fn [{:keys [save-undo? undo-changes redo-changes undo-group tags stack-undo? selected-before]}] + (fn [{:keys [save-undo? undo-changes redo-changes undo-group tags stack-undo? selected-before translation?]}] (if (and save-undo? (seq undo-changes)) (rx/of (dwu/append-undo {:undo-changes undo-changes :redo-changes redo-changes :undo-group undo-group :tags tags - :selected-before selected-before} + :selected-before selected-before + :translation (boolean translation?)} stack-undo?)) (rx/empty)))) (rx/take-until stopper-s)))))) diff --git a/frontend/test/frontend_tests/data/workspace_undo_test.cljs b/frontend/test/frontend_tests/data/workspace_undo_test.cljs index 55a1ae0cca..1794b13a90 100644 --- a/frontend/test/frontend_tests/data/workspace_undo_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_undo_test.cljs @@ -129,7 +129,8 @@ {:undo-changes [{:type :mod-page :id page-id :name "Page 1"}] :redo-changes [{:type :mod-page :id page-id :name name}] :undo-group group - :tags #{}}) + :tags #{} + :translation false}) ;; Scenario: inside one transaction, a commit names another undo group (as a ;; component update does). Proves: the history entry records both groups, diff --git a/frontend/test/frontend_tests/data/workspace_undo_translation_test.cljs b/frontend/test/frontend_tests/data/workspace_undo_translation_test.cljs new file mode 100644 index 0000000000..6dc42a89d5 --- /dev/null +++ b/frontend/test/frontend_tests/data/workspace_undo_translation_test.cljs @@ -0,0 +1,171 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.data.workspace-undo-translation-test + "The `:translation` flag of undo entries. Undo, redo and undo-to-index + replay changes through `commit-changes`, and a replay of a commit that only + moved shapes must say so, so that nothing downstream reacts to it as it + would to an edit." + (:require + [app.common.test-helpers.compositions :as ctho] + [app.common.test-helpers.files :as cthf] + [app.common.test-helpers.ids-map :as cthi] + [app.common.uuid :as uuid] + [app.main.data.changes :as dch] + [app.main.data.workspace.undo :as dwu] + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.async :as async] + [frontend-tests.helpers.pages :as thp] + [frontend-tests.helpers.state :as ths] + [potok.v2.core :as ptk])) + +(t/use-fixtures :each + {:before thp/reset-idmap!}) + +(defn- setup + [] + (let [file (-> (cthf/sample-file :file1) + (ctho/add-frame :frame1)) + store (ths/setup-store file)] + {:store store + :page-id (cthf/current-page-id file) + :frame-id (cthi/id :frame1) + :commits (atom [])})) + +(defn- set-attr + [{:keys [page-id frame-id]} attr val] + {:type :mod-obj + :id frame-id + :page-id page-id + :operations [{:type :set :attr attr :val val}]}) + +(defn- move-entry + "An undo entry for a move, with the given `translation?` flag." + [ctx translation? & {:keys [undo-group]}] + {:undo-changes [(set-attr ctx :x 0)] + :redo-changes [(set-attr ctx :x 10)] + :undo-group (or undo-group (uuid/next)) + :tags #{} + :translation translation?}) + +(defn- edit-entry + "An undo entry for an edit that is not a move." + [ctx translation? & {:keys [undo-group]}] + (-> (move-entry ctx translation? :undo-group undo-group) + (assoc :undo-changes [(set-attr ctx :opacity 1)] + :redo-changes [(set-attr ctx :opacity 0.5)]))) + +(defn- observe-commits + "Starts collecting the data of every commit the store sees." + [{:keys [store commits]}] + (->> (ptk/input-stream store) + (rx/filter dch/commit?) + (rx/map deref) + (rx/subs! #(swap! commits conj %)))) + +(defn- ^:async emit-and-wait + "Emits `event` and resolves with the commit it caused." + [{:keys [store commits]} event] + (let [before (count @commits)] + (ptk/emit! store event) + (await (async/wait-for #(< before (count @commits)) "the commit of the event")) + (last @commits))) + +(defn- append-all + [{:keys [store]} entries] + (doseq [entry entries] + (ptk/emit! store (dwu/append-undo entry false)))) + +(t/deftest ^:async undo-and-redo-replay-with-the-flag-of-the-entry + (let [ctx (setup)] + (observe-commits ctx) + + (t/testing "a move" + (append-all ctx [(move-entry ctx true)]) + (t/is (true? (:translation? (await (emit-and-wait ctx dwu/undo))))) + (t/is (true? (:translation? (await (emit-and-wait ctx dwu/redo)))))) + + (t/testing "an edit that is not a move" + (append-all ctx [(edit-entry ctx false)]) + (t/is (false? (:translation? (await (emit-and-wait ctx dwu/undo))))) + (t/is (false? (:translation? (await (emit-and-wait ctx dwu/redo)))))))) + +(t/deftest entry-without-the-translation-flag-is-rejected + (let [ctx (setup) + entry (dissoc (move-entry ctx true) :translation)] + (t/is (thrown? js/Error (dwu/append-undo entry false))))) + +(t/deftest ^:async undo-to-index-replays-a-translation-only-if-all-entries-are + (let [ctx (setup)] + (observe-commits ctx) + (append-all ctx [(move-entry ctx true) + (move-entry ctx true) + (move-entry ctx true)]) + + (t/is (true? (:translation? (await (emit-and-wait ctx (dwu/undo-to-index -1))))) + "undoing three moves") + (t/is (true? (:translation? (await (emit-and-wait ctx (dwu/undo-to-index 2))))) + "redoing three moves") + + (append-all ctx [(move-entry ctx true) + (edit-entry ctx false) + (move-entry ctx true)]) + + (t/is (not (true? (:translation? (await (emit-and-wait ctx (dwu/undo-to-index 2)))))) + "undoing a move, an edit and a move") + (t/is (not (true? (:translation? (await (emit-and-wait ctx (dwu/undo-to-index 5)))))) + "redoing a move, an edit and a move") + + (t/is (true? (:translation? (await (emit-and-wait ctx (dwu/undo-to-index 4))))) + "undoing only the last move of those"))) + +(t/deftest ^:async undo-of-a-group-replays-a-translation-only-if-all-entries-are + (let [ctx (setup) + group-a (uuid/next) + group-b (uuid/next)] + (observe-commits ctx) + + (append-all ctx [(move-entry ctx true :undo-group group-a) + (move-entry ctx true :undo-group group-a)]) + (t/is (true? (:translation? (await (emit-and-wait ctx dwu/undo)))) + "a group of moves") + + (append-all ctx [(move-entry ctx true :undo-group group-b) + (edit-entry ctx false :undo-group group-b)]) + (t/is (not (true? (:translation? (await (emit-and-wait ctx dwu/undo))))) + "a group with an edit"))) + +(defn- merged-flag + "The flag of the only undo entry left after adding one entry per flag of + `flags`, either stacked on each other or accumulated in a transaction." + [mode flags] + (let [ctx (setup) + entries (map #(move-entry ctx %) flags) + events (case mode + :stacked + (cons (dwu/append-undo (first entries) false) + (map #(dwu/append-undo % true) (rest entries))) + + :transaction + (concat [(dwu/start-undo-transaction ::tx)] + (map #(dwu/append-undo % false) entries) + [(dwu/commit-undo-transaction ::tx)])) + state (reduce (fn [state event] (ptk/update event state)) + @(:store ctx) + events) + items (get-in state [:workspace-undo :items])] + (t/is (= 1 (count items)) (str mode " " (pr-str flags))) + (:translation (first items)))) + +(t/deftest merged-entries-are-a-translation-only-if-all-parts-are + (doseq [mode [:stacked :transaction]] + (t/testing (name mode) + (t/is (true? (merged-flag mode [true true]))) + (t/is (true? (merged-flag mode [true true true]))) + (t/is (not (true? (merged-flag mode [true false])))) + (t/is (not (true? (merged-flag mode [false true])))) + (t/is (not (true? (merged-flag mode [true false true]))))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 85ff90970d..0e6187caff 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -48,6 +48,7 @@ [frontend-tests.data.workspace-texts-test] [frontend-tests.data.workspace-thumbnails-test] [frontend-tests.data.workspace-undo-test] + [frontend-tests.data.workspace-undo-translation-test] [frontend-tests.data.workspace-variant-rename-test] [frontend-tests.data.workspace-variants-test] [frontend-tests.data.workspace-versions-test] @@ -248,6 +249,7 @@ 'frontend-tests.data.workspace-texts-test 'frontend-tests.data.workspace-thumbnails-test 'frontend-tests.data.workspace-undo-test + 'frontend-tests.data.workspace-undo-translation-test 'frontend-tests.data.workspace-variant-rename-test 'frontend-tests.data.workspace-variants-test 'frontend-tests.data.workspace-versions-test diff --git a/frontend/test/frontend_tests/tokens/helpers/state.cljs b/frontend/test/frontend_tests/tokens/helpers/state.cljs index e03b15b099..6ef0345e41 100644 --- a/frontend/test/frontend_tests/tokens/helpers/state.cljs +++ b/frontend/test/frontend_tests/tokens/helpers/state.cljs @@ -70,14 +70,15 @@ (rx/map deref) (rx/filter #(= :local (:source %))) (rx/mapcat - (fn [{:keys [save-undo? undo-changes redo-changes undo-group tags stack-undo? selected-before]}] + (fn [{:keys [save-undo? undo-changes redo-changes undo-group tags stack-undo? selected-before translation?]}] (if (and save-undo? (seq undo-changes)) (rx/of (dwu/append-undo {:undo-changes undo-changes :redo-changes redo-changes :undo-group undo-group :tags tags - :selected-before selected-before} + :selected-before selected-before + :translation (boolean translation?)} stack-undo?)) (rx/empty)))) (rx/take-until stopper-s))))))