From eed3a9ba601828b8c7cb31cc0d15a1d114381a7b Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Thu, 15 Oct 2026 15:11:30 +0200 Subject: [PATCH] :bug: Keep guides and reflows in the undo group of their step Two derived commits that left the copies' sync in an undo entry of its own (Phase 3c). - Move frame guides in the undo group of their transform. A transform of a board sends the move of its guides before the shapes' update. The guide move committed every guide of the board, moved or not, each in a commit with an undo group of its own, so when the board was a main with a guide, its transform and the copies' sync landed in different undo entries: one undo reverted only the copies (F6). Both transform paths now give the shapes' update an undo group up front and pass it to the guide move, which commits only the guides that move, all in one commit in that group. workspace-frame-guides-undo-test: one undo reverts a rotation of a main with a guide, its copy included, and a move of a main together with its guide. - Reflow layouts in the undo group of the step that asked for it. A layout reflow runs a moment after the commit that needs it: the :layout/update events are buffered for 100 ms and reflowed per page. The reflow lost the undo group of the step (F43): - the buffer passed no undo group to the reflow; - update-shapes asked for a reflow without the group of its commit; - the reflow's transform opened an undo transaction, so it made an entry of its own instead of stacking on the latest one, and the sync that a reflow of a main starts took that entry's fresh group. So one undo after a sync that reflows (a flex main's margin edit, a nested sync with minimal frames, a swap) reverted only the reflow, or only part of the step. update-shapes now gives a commit that needs a reflow an undo group up front and passes it on; the buffer reflows each page in the group an update of that buffer names (those that name none come from the same step, like update-layout-child's); the reflow stacks on the latest entry with no transaction of its own. Cases AJ and AN no longer pin F43: one undo after a nested sync reverts the remote change and the float noise of the reflow, and one undo after a flex main's margin edit reverts the margin, the sync and the reflow. The frontend golden master records that. AI-assisted-by: claude-opus-5-5 --- .../test_helpers/composable/comp/cases.cljc | 28 ++------ .../src/app/main/data/workspace/guides.cljs | 33 +++++---- .../app/main/data/workspace/modifiers.cljs | 17 ++++- .../app/main/data/workspace/shape_layout.cljs | 21 ++++-- .../src/app/main/data/workspace/shapes.cljs | 10 ++- .../composable_tests/golden_master.edn | 4 +- .../workspace_frame_guides_undo_test.cljs | 69 +++++++++++++++++++ frontend/test/frontend_tests/runner.cljs | 2 + 8 files changed, 139 insertions(+), 45 deletions(-) create mode 100644 frontend/test/frontend_tests/data/workspace_frame_guides_undo_test.cljs diff --git a/common/src/app/common/test_helpers/composable/comp/cases.cljc b/common/src/app/common/test_helpers/composable/comp/cases.cljc index 9001f4d6b5..8a289afc91 100644 --- a/common/src/app/common/test_helpers/composable/comp/cases.cljc +++ b/common/src/app/common/test_helpers/composable/comp/cases.cljc @@ -625,18 +625,16 @@ "Case AJ (frontend only). A component nested two levels deep, with the minimal frame size, has a copy; its remote main changes the rect, then one undo runs. The transform math rewrites the minimal nested frames with float - noise, so the sync of the change reflows them, and the reflow commits land - in undo entries of their own (F43). One undo should revert the change and - its sync; it reverts only the last reflow entry, so the remote rect keeps - the change. How many entries the reflow makes depends on when its 100 ms - buffer flushes, so the case checks the value one undo leaves, never the - undo stack." + noise, so the sync of the change reflows them. The reflow joins the undo + group of the change, so one undo reverts the change, its sync and the + reflow: the remote rect is back to red. How many entries the reflow makes + depends on when its 100 ms buffer flushes, so the case checks the value + one undo leaves, never the undo stack." [] (let [m "main" remote (n/remote-rect-of m)] {:setup setup/empty-situation - ;; the round trip's signature depends on when the reflow buffer flushes - :undo-check {:off "the case undoes its own step (F43)"} + :undo-check {:off "the case undoes its own step"} :operation (tm/in-sequence [(n/create-component m red) (n/make-nested-component m) @@ -645,8 +643,7 @@ (n/change-property remote :fills blue) (n/undo)]) :asserter (fn [s] - (tm/known-failure "F43" red blue - (fill-color (tm/shape-by-id s (remote s)))))})) + (t/is (= red (fill-color (tm/shape-by-id s (remote s))))))})) (defn text-override-merges-with-main-text-change "Case Y. A main text changes its characters or its font size; before that, @@ -1085,17 +1082,6 @@ update-main (n/update-main :copy-root) follow-up (tm/one-of [main-change reset update-main])] {:setup setup/flex-component-with-copy - ;; the reflow after the sync of the main's margin lands in undo entries - ;; of its own, so one undo leaves the margin and the reflow in place - :undo-check {:known-failures [{:bug "F43" - :fails {:undo #{[:object :main-child :layout-item-margin] - [:object :main-child :layout-item-margin-type] - [:object :main-child :x] - [:object :main-child :selrect] - [:object :main-child :points]} - :undo-index #{[:undo-index 2]}} - :op main-change - :runners #{:frontend}}]} :operation (tm/in-sequence [override follow-up]) :asserter (fn [s] (let [copy (tm/role-shape s :copy-instance) diff --git a/frontend/src/app/main/data/workspace/guides.cljs b/frontend/src/app/main/data/workspace/guides.cljs index 0fd638411e..57e20dfd8a 100644 --- a/frontend/src/app/main/data/workspace/guides.cljs +++ b/frontend/src/app/main/data/workspace/guides.cljs @@ -115,16 +115,17 @@ (every? uuid? (:ids args))) (ptk/reify ::move-frame-guides ptk/WatchEvent - (watch [_ state _] + (watch [it state _] (let [ids (:ids args) object-modifiers (:modifiers args) object-transforms (:transforms args) - objects (dsh/lookup-page-objects state) + page (dsh/lookup-page state) + objects (:objects page) is-frame? (fn [id] (= :frame (get-in objects [id :type]))) frame-ids? (into #{} (filter is-frame?) ids) - build-move-event + move-guide (fn [guide] (let [frame-id (:frame-id guide) frame (get objects frame-id) @@ -140,17 +141,25 @@ (gsh/apply-transform transform)) moved (gpt/to-vec (gpt/point (:x frame) (:y frame)) - (gpt/point (:x frame') (:y frame'))) + (gpt/point (:x frame') (:y frame')))] + (update guide :position + (get moved (:axis guide))))) - guide (update guide :position + (get moved (:axis guide)))] - (update-guides guide))) + ;; one commit, in the undo group of the transform, with the + ;; guides that move + changes + (->> (:guides page) + (vals) + (filter (comp frame-ids? :frame-id)) + (keep (fn [guide] + (let [guide' (move-guide guide)] + (when (not= guide guide') guide')))) + (reduce (fn [changes guide] (pcb/set-guide changes (:id guide) guide)) + (-> (pcb/empty-changes it) + (pcb/with-page page) + (pcb/set-undo-group (:undo-group args)))))] - guides (-> state dsh/lookup-page :guides vals)] - - (->> guides - (filter (comp frame-ids? :frame-id)) - (map build-move-event) - (rx/from)))))) + (when (seq (:redo-changes changes)) + (rx/of (dwc/commit-changes changes))))))) (defn update-guide-color [guide-id color] diff --git a/frontend/src/app/main/data/workspace/modifiers.cljs b/frontend/src/app/main/data/workspace/modifiers.cljs index 8c4aba9eca..a0b9ac564a 100644 --- a/frontend/src/app/main/data/workspace/modifiers.cljs +++ b/frontend/src/app/main/data/workspace/modifiers.cljs @@ -687,8 +687,14 @@ ignore-tree (clp/calculate-ignore-tree objects (keys transforms) transform-shape) + ;; the guides move in a commit of their own, in the undo + ;; group of the shapes, so the copies' sync joins them both + undo-group + (or (:undo-group params) (uuid/next)) + options (-> params + (assoc :undo-group undo-group) (assoc :reg-objects? true) (assoc :ignore-tree ignore-tree) (assoc :translation? translation?) @@ -728,7 +734,7 @@ (rx/empty)) (rx/of (clear-local-transform) - (ptk/event ::dwg/move-frame-guides {:ids ids :transforms transforms}) + (ptk/event ::dwg/move-frame-guides {:ids ids :transforms transforms :undo-group undo-group}) (ptk/event ::dwcm/move-frame-comment-threads transforms) (dwsh/update-shapes ids update-shape options)) @@ -839,8 +845,14 @@ (clp/calculate-ignore-tree objects (keys object-modifiers) (clp/modifiers-transform object-modifiers)) + ;; the guides move in a commit of their own, in the undo group of + ;; the shapes, so the copies' sync joins them both + undo-group + (or (:undo-group options) (uuid/next)) + options (-> options + (assoc :undo-group undo-group) (assoc :reg-objects? true) (assoc :ignore-tree ignore-tree) ;; Attributes that can change in the transform. This @@ -860,7 +872,8 @@ (cond-> (d/not-empty? pos-data) (assoc-position-data pos-data shape)))))] - (rx/of (ptk/event ::dwg/move-frame-guides {:ids ids-with-children :modifiers object-modifiers}) + (rx/of (ptk/event ::dwg/move-frame-guides {:ids ids-with-children :modifiers object-modifiers + :undo-group undo-group}) (ptk/event ::dwcm/move-frame-comment-threads ids-with-children) (dwsh/update-shapes ids update-shape options)))))) diff --git a/frontend/src/app/main/data/workspace/shape_layout.cljs b/frontend/src/app/main/data/workspace/shape_layout.cljs index 82b3886c5a..16d9bc027b 100644 --- a/frontend/src/app/main/data/workspace/shape_layout.cljs +++ b/frontend/src/app/main/data/workspace/shape_layout.cljs @@ -61,14 +61,18 @@ update-positions-stream (if (d/not-empty? ids) (let [modif-tree (dwm/create-modif-tree ids (ctm/reflow-modifiers))] + ;; the reflow stacks on the latest undo entry, which a + ;; transaction of its own would not (if (features/active-feature? state "render-wasm/v1") (rx/of (dwm/apply-wasm-modifiers modif-tree :stack-undo? true + :undo-transation? false :undo-group undo-group :ignore-touched true)) (rx/of (dwm/apply-modifiers {:page-id page-id :modifiers modif-tree :stack-undo? true + :undo-transation? false :ignore-touched true :undo-group undo-group})))) (rx/empty)) @@ -112,14 +116,19 @@ (rx/map #(assoc % ::reflow-task (wrf/start! :layout (:ids %)))) (rx/buffer-time 100) (rx/filter #(d/not-empty? %)) + ;; The updates of a page reflow together, in the undo group of + ;; the step that asked for them: the one an update names, as + ;; those with none come from the same step. (rx/mapcat (fn [updates] - (->> (group-by :page-id updates) - (map (fn [[page-id updates]] - (let [ids (into #{} (mapcat :ids) updates)] - (update-layout-positions {:page-id page-id - :ids ids - :reflow-tasks (mapv ::reflow-task updates)}))))))) + (let [undo-group (some :undo-group updates)] + (->> (group-by :page-id updates) + (map (fn [[page-id updates]] + (let [ids (into #{} (mapcat :ids) updates)] + (update-layout-positions {:page-id page-id + :ids ids + :undo-group undo-group + :reflow-tasks (mapv ::reflow-task updates)})))))))) (rx/take-until stopper) ;; On workspace teardown clear everything still pending. (rx/finalize wrf/reset-pending!)))))) diff --git a/frontend/src/app/main/data/workspace/shapes.cljs b/frontend/src/app/main/data/workspace/shapes.cljs index 347c997f6e..18702666df 100644 --- a/frontend/src/app/main/data/workspace/shapes.cljs +++ b/frontend/src/app/main/data/workspace/shapes.cljs @@ -17,6 +17,7 @@ [app.common.types.container :as ctn] [app.common.types.shape :as cts] [app.common.types.shape-tree :as ctst] + [app.common.uuid :as uuid] [app.main.data.changes :as dch] [app.main.data.comments :as dc] [app.main.data.event :as ev] @@ -263,8 +264,12 @@ {:keys [layout-ids text-ids]} (wrfs/reflow-ids state page-id objects changed-objects ids props) + ;; the reflow joins the undo group of this commit changes - (add-undo-group changes state)] + (let [changes (add-undo-group changes state)] + (cond-> changes + (and layout-ids (nil? (:undo-group changes))) + (pcb/set-undo-group (uuid/next))))] (rx/concat ;; Announces the texts still to be re-measured, so a reflow wait @@ -281,7 +286,8 @@ ;; Update layouts for properties marked (if layout-ids - (rx/of (ptk/data-event :layout/update {:ids layout-ids})) + (rx/of (ptk/data-event :layout/update {:ids layout-ids + :undo-group (:undo-group changes)})) (rx/empty))))))))) (defn add-shape diff --git a/frontend/test/frontend_tests/composable_tests/golden_master.edn b/frontend/test/frontend_tests/composable_tests/golden_master.edn index 7b4feb9103..26259fbe2d 100644 --- a/frontend/test/frontend_tests/composable_tests/golden_master.edn +++ b/frontend/test/frontend_tests/composable_tests/golden_master.edn @@ -30,7 +30,7 @@ {:transcript "one-of -> \n {:target :copy-root, :value 500}", :states ["df9a66ef7c8ee469" "c4f0c92f92575cda"]} {:transcript "one-of -> \n one-of -> ", :states ["94318a628db305b6"]}] :case-aj-reflow-after-nested-sync-is-its-own-undo-step - [{:transcript " {:component \"main\", :id sync-main-component}\n {:component \"main\", :level 0, :outer sync-main-outercomp-0}\n {:component \"main\", :level 1, :outer sync-main-outercomp-1}\n {:component \"main\", :copy sync-main-copyhead-0}\n {:target #fn, :property :fills, :value \"#0000ff\"}\n {:times 1}", :states ["43ca8b2de80a51fc" "43ca8b2de80a51fc" "9c3022331b3fe9ae"]}] + [{:transcript " {:component \"main\", :id sync-main-component}\n {:component \"main\", :level 0, :outer sync-main-outercomp-0}\n {:component \"main\", :level 1, :outer sync-main-outercomp-1}\n {:component \"main\", :copy sync-main-copyhead-0}\n {:target #fn, :property :fills, :value \"#0000ff\"}\n {:times 1}", :states ["43ca8b2de80a51fc" "32b45a5ac7536b08" "7104bfaccd42cd3b"]}] :case-ak-update-main-pushes-grid-cells [{:transcript " {:target :copy-child, :to :other, :keep-touched? false}\n {:board :copy-root, :child :copy-child, :row 1, :column 3}\n {:target :copy-root}", :states ["5a6af6c0b8f86ac3" "278e4ff6bf0df746" "322e03c88f4a9dce" "432cfef7e9b05164"]} {:transcript "one-of -> \n {:board :copy-root, :child :copy-child, :row 1, :column 3}\n {:target :copy-root}", :states ["9900a7f71dc0cb5f" "de66ab1c3d4cc157" "6b5ff9fd86e2aa87"]}] @@ -40,7 +40,7 @@ :case-am-update-main-pushes-a-swap-inside-a-group [{:transcript " {:target :copy-child, :to :other, :keep-touched? false}\n {:target :copy-root}", :states ["b4f808a12f723283" "9b695fc212f26b53" "32b94deb961ac763"]}] :case-an-flex-child-override-follows-sync-reset-and-update-main - [{:transcript " {:target :copy-instance, :property :margin, :value 8}\n {:target :main-instance, :property :margin, :value 20}", :states ["234a21e54bf21bd5" "03664b1e6eb74856" "9c3b12abbc6089ce"]} + [{:transcript " {:target :copy-instance, :property :margin, :value 8}\n {:target :main-instance, :property :margin, :value 20}", :states ["234a21e54bf21bd5" "03664b1e6eb74856" "820db97e4e4b43dc"]} {:transcript " {:target :copy-instance, :property :margin, :value 8}\n {:target :copy-root}", :states ["234a21e54bf21bd5" "1f92f9b4d2e50598" "a09954797e57930c"]} {:transcript " {:target :copy-instance, :property :margin, :value 8}\n {:target :copy-root}", :states ["234a21e54bf21bd5" "ace1e76c1da2c443" "8f3452e82544f1ad"]}] :case-ao-copy-dropped-into-a-grid-cell-reaches-copies diff --git a/frontend/test/frontend_tests/data/workspace_frame_guides_undo_test.cljs b/frontend/test/frontend_tests/data/workspace_frame_guides_undo_test.cljs new file mode 100644 index 0000000000..9bf68045ca --- /dev/null +++ b/frontend/test/frontend_tests/data/workspace_frame_guides_undo_test.cljs @@ -0,0 +1,69 @@ +;; 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-frame-guides-undo-test + "Undo of a transform of a main board that holds a guide: the transform + moves the guide in a commit of its own before the shapes, and the copies' + sync must still be undone with the transform." + (:require + [app.common.files.changes-builder :as pcb] + [app.common.test-helpers.composable.comp.setups :as setup] + [app.common.test-helpers.files :as thf] + [app.common.test-helpers.ids-map :as thi] + [app.common.test-helpers.shapes :as ths] + [app.common.uuid :as uuid] + [app.main.data.changes :as dch] + [app.main.data.workspace.transforms :as dwt] + [app.main.data.workspace.undo :as dwu] + [cljs.test :as t :include-macros true] + [frontend-tests.composable-tests.interpreter :as ftm])) + +(t/use-fixtures :each ftm/fixture) + +(defn- add-guide + "The commit, out of the undo stack, of a guide on the board `label`." + [label] + (let [page (thf/current-page (ftm/current-file)) + id (uuid/next)] + (dch/commit-changes + (-> (pcb/empty-changes nil (:id page)) + (pcb/with-page page) + (pcb/set-guide id {:id id + :axis :x + :position 10 + :frame-id (thi/id label)}) + (pcb/set-save-undo? false))))) + +(defn- rotation-of + [label] + (:rotation (ths/get-shape (ftm/current-file) label))) + +(defn- guide-positions + [] + (mapv :position (vals (:guides (thf/current-page (ftm/current-file)))))) + +(t/deftest ^:async one-undo-reverts-a-rotation-of-a-main-with-a-guide + ;; the guide does not move, and adds no commit of its own + (ftm/install! (setup/simple-component-with-labeled-copy)) + (await (ftm/await-step [(add-guide :main-root)])) + (await (ftm/await-step [(dwt/increase-rotation [(thi/id :main-root)] 45)])) + (t/is (= 45 (rotation-of :main-root))) + (t/is (= 45 (rotation-of :copy-root))) + + (t/is (= :settled (await (ftm/await-step [dwu/undo])))) + (t/is (= 0 (rotation-of :main-root))) + (t/is (= 0 (rotation-of :copy-root)))) + +(t/deftest ^:async one-undo-reverts-a-move-of-a-main-with-its-guide + (ftm/install! (setup/simple-component-with-labeled-copy)) + (await (ftm/await-step [(add-guide :main-root)])) + (let [x (:x (ths/get-shape (ftm/current-file) :main-root))] + (await (ftm/await-step [(dwt/update-position (thi/id :main-root) {:x (+ x 50)})])) + (t/is (= [60] (guide-positions))) + + (t/is (= :settled (await (ftm/await-step [dwu/undo])))) + (t/is (= x (:x (ths/get-shape (ftm/current-file) :main-root)))) + (t/is (= [10] (guide-positions))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 3c358fce29..6fcb10a101 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -34,6 +34,7 @@ [frontend-tests.data.workspace-context-menu-test] [frontend-tests.data.workspace-edit-watchers-test] [frontend-tests.data.workspace-font-variants-test] + [frontend-tests.data.workspace-frame-guides-undo-test] [frontend-tests.data.workspace-interactions-test] [frontend-tests.data.workspace-libraries-test] [frontend-tests.data.workspace-library-links-test] @@ -214,6 +215,7 @@ 'frontend-tests.data.workspace-edit-watchers-test 'frontend-tests.data.workspace-interactions-test 'frontend-tests.data.workspace-font-variants-test + 'frontend-tests.data.workspace-frame-guides-undo-test 'frontend-tests.data.workspace-libraries-test 'frontend-tests.data.workspace-library-links-test 'frontend-tests.data.workspace-library-updates-test