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

🐛 Restore existing component nesting when undoing a move

Browse Source 
Sync and swapped children can leave finite repeated component nesting.
Moving a copy out succeeds, but undo rejects its old position under the
normal component-loop guard. Record restore-component-nesting only on
inverses whose original position already repeats a component.

Normal forward moves retain that guard. The applier still rejects physical
object-tree cycles and changes to copy structure. The new optional change
field defaults to the current behavior and changes no stored shape data.

Portable regression: exact undo fails before the fix. Five assertions now
pass, including redo, normal loop refusal and object-cycle refusal using
the generated inverse. Focused JVM and golden checks: 195 tests, 2189
assertions. Both original shrunk programs pass. Live WASM undo/redo restores
complete snapshots. Common/frontend lint and formatting pass.

Component benchmark ratios: mains 0.57/0.62/0.67; levels 0.80;
chain 0.82; library 0.56; many shapes 0.55; variant switch 0.91.
All worlds pass the unchanged baseline gate.

AI-assisted-by: GPT-6
This commit is contained in:
alonso.torres 2026-10-24 15:20:03 +02:00
parent 60f063a775
commit 2ea74b7ad1
4 changed files with 58 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
.serena/memories/common/changes-architecture.md
View File

@ -49,3 +49,10 @@ To inspect what a UI action emitted, use `mem:frontend/cljs-repl` with the snipp
When the builder lacks a main's container (inverse sync), use that main
container's original index. Both rules preserve order after several
siblings disappear (F158).
- A move inverse can restore component nesting that existed before the
move, even when the normal component-loop check would reject creating
it now (F177). `change-parent` records `:restore-component-nesting` on
the inverse only when the original position already repeats a component.
The change applier still rejects object-tree cycles and copy-structure
edits. Normal forward moves keep the component-loop guard.

10
common/src/app/common/files/changes.cljc
View File

@ -238,6 +238,7 @@
[:shapes ::sm/any]
[:index {:optional true} [:maybe :int]]
[:after-shape {:optional true} ::sm/any]
[:restore-component-nesting {:optional true} :boolean]
[:allow-altering-copies {:optional true} :boolean]]]
[:reorder-children
@ -755,7 +756,8 @@
(defmethod process-change :mov-objects
;; FIXME: ignore-touched is no longer used, so we can consider it deprecated
[data {:keys [parent-id shapes index page-id component-id #_ignore-touched after-shape allow-altering-copies syncing]}]
[data {:keys [parent-id shapes index page-id component-id #_ignore-touched after-shape
allow-altering-copies restore-component-nesting syncing]}]
(letfn [(calculate-invalid-targets [objects shape-id]
(let [reduce-fn #(into %1 (calculate-invalid-targets objects %2))]
(->> (get-in objects [shape-id :shapes])
@ -769,7 +771,11 @@
shape (get objects shape-id)]
(and shape
(not (invalid-targets parent-id))
(not (cfh/components-nesting-loop? objects shape-id parent-id))
;; Undo may restore repeated component nesting that was
;; already present before the move. Object cycles above
;; and copy structure guards below still apply.
(or restore-component-nesting
(not (cfh/components-nesting-loop? objects shape-id parent-id)))
(or
;; In some cases (like a component
;; swap) it's allowed to change the

2
common/src/app/common/files/changes_builder.cljc
View File

@ -553,6 +553,8 @@
:shapes [(:id shape)]
:after-shape prev-sibling
:index 0} ; index is used in case there is no after-shape (moving bottom shapes)
(cfh/components-nesting-loop? objects (:id shape) (:parent-id shape))
(assoc :restore-component-nesting true)
(:allow-altering-copies options)
(assoc :allow-altering-copies true)))))

41
common/test/common_tests/logic/move_shapes_test.cljc
View File

@ -8,6 +8,10 @@
(:require
[app.common.files.changes-builder :as pcb]
[app.common.logic.shapes :as cls]
[app.common.test-helpers.components :as thc]
[app.common.test-helpers.composable.comp.nodes :as n]
[app.common.test-helpers.composable.comp.runner :as r]
[app.common.test-helpers.composable.core :as tm]
[app.common.test-helpers.compositions :as tho]
[app.common.test-helpers.files :as thf]
[app.common.test-helpers.ids-map :as thi]
@ -21,6 +25,43 @@
(t/use-fixtures :each thi/test-fixture)
(t/deftest moving-out-of-existing-component-nesting-undoes-exactly
(let [file (-> (thf/sample-file :file)
(tho/add-simple-component :a :main-a :rect-a)
(tho/add-simple-component :other :main-other :rect-other)
(tho/add-frame :main-b)
(thc/instantiate-component :a :nested-a :parent-label :main-b)
(thc/make-component :b :main-b)
(tho/add-frame :main-c)
(thc/instantiate-component :b :nested-b :parent-label :main-c)
(thc/make-component :c :main-c)
(thc/instantiate-component :c :copy-c :children-labels [:copy-b :copy-a])
(tho/add-simple-component :destination-component :destination :destination-child))
nested (-> (tm/make-situation file)
(r/step (n/swap-shape :copy-a :other)) r/settle
(r/step (n/nest-copy :copy-c :main-a)) r/settle)
moved (-> nested (r/step (n/nest-copy :nested-a :destination)) r/settle)
undone (r/step moved (n/undo))
page (thf/current-page (tm/file moved))
forward (cls/generate-relocate (-> (pcb/empty-changes nil (:id page))
(pcb/with-objects (:objects page))
(pcb/with-library-data (:data (tm/file moved))))
(thi/id :main-b) 0 #{(thi/id :nested-a)})
refused (thf/apply-changes (tm/file moved) forward)
old-page (thf/current-page (tm/file nested))
move-changes (-> (pcb/empty-changes nil (:id old-page))
(pcb/with-objects (:objects old-page))
(pcb/change-parent (thi/id :destination) [(tm/resolve-shape nested :nested-a)]))
inverse (first (filter #(= :mov-objects (:type %)) (:undo-changes move-changes)))
cyclic (assoc inverse :parent-id (first (:shapes (tm/resolve-shape moved :nested-a))))
cycle-refused (thf/apply-changes (tm/file moved) {:redo-changes [cyclic]})]
(t/is (= (thi/id :destination) (:parent-id (tm/resolve-shape moved :nested-a))))
(t/is (= (thi/id :destination) (:parent-id (ths/get-shape refused :nested-a))))
(t/is (= (thn/normalize-file (tm/file moved)) (thn/normalize-file cycle-refused)))
(t/is (= (thn/normalize-file (tm/file nested)) (thn/normalize-file (tm/file undone))))
(t/is (= (thn/normalize-file (tm/file moved))
(thn/normalize-file (tm/file (r/step undone (n/redo))))))))
(t/deftest moving-a-child-out-of-a-grid-clears-its-old-cell
(let [file (-> (thf/sample-file :file1)
(tho/add-grid-frame :grid :width 200 :height 100)