github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-11-10 03:50:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

⚡ Keep the translation flag on undo and redo of a move (#12870)

Browse Source 
* ⚡ 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 <niwi@niwi.nz>
This commit is contained in:
Luke Lau 2026-11-10 03:47:42 +08:00 committed by GitHub
parent bdf16bcfda
commit cd1ece4beb
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
8 changed files with 204 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
.serena/memories/frontend/workspace-state-persistence-subtleties.md
View File

@ -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

5
frontend/src/app/main/data/workspace.cljs
View File

@ -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))))))

26
frontend/src/app/main/data/workspace/undo.cljs
View File

@ -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

5
frontend/test/frontend_tests/composable_tests/interpreter.cljs
View File

@ -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))))))

3
frontend/test/frontend_tests/data/workspace_undo_test.cljs
View File

@ -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,

171
frontend/test/frontend_tests/data/workspace_undo_translation_test.cljs Normal file
View File

@ -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])))))))

2
frontend/test/frontend_tests/runner.cljs
View File

@ -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

5
frontend/test/frontend_tests/tokens/helpers/state.cljs
View File

@ -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))))))