From 560208ce744648c7d0cce00380c5117dd489c11a Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Sat, 24 Oct 2026 22:50:03 +0200 Subject: [PATCH] :bug: Refuse duplicates that expand component nesting loops A finite repeated component tree can cut recursion with a swapped child. Duplicating another child in its main opens an unblocked branch that sync keeps expanding. Check component nesting against the actual destination before generating duplicate changes, and keep safe shapes in mixed batches. ALT duplication at the page root remains allowed. Use the same eligibility in workspace duplication, pure nodes and generated candidates. The portable regression fails three assertions before the fix and passes all five afterward. The original library seed 712 case 1601 and its reduced switch/nest/duplicate program pass. Focused regression and golden checks pass 123 tests, 2492 assertions; goldens are unchanged. Live WASM verifies refusal, ALT duplication, undo/redo and later stable sync. Component benchmark ratios: mains 0.47/0.55/0.55; levels 0.51; chain 0.53; library 0.58; many shapes 0.52; variant switch 0.74. All worlds pass the unchanged baseline gate. AI-assisted-by: GPT-6 --- .../memories/common/changes-architecture.md | 8 ++++ common/src/app/common/logic/duplicate.cljc | 18 ++++++--- .../test_helpers/composable/comp/nodes.cljc | 4 +- .../common/test_helpers/composable/gen.cljc | 4 +- .../logic/copying_and_duplicating_test.cljc | 37 +++++++++++++++++++ .../app/main/data/workspace/selection.cljs | 4 +- 6 files changed, 66 insertions(+), 9 deletions(-) diff --git a/.serena/memories/common/changes-architecture.md b/.serena/memories/common/changes-architecture.md index a329bf80e4..b7ba2e1974 100644 --- a/.serena/memories/common/changes-architecture.md +++ b/.serena/memories/common/changes-architecture.md @@ -56,3 +56,11 @@ To inspect what a UI action emitted, use `mem:frontend/cljs-repl` with the snipp 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. + +Normal duplication checks the component nesting guard too (F181). A finite +repeated component tree can use a swapped child to cut recursion; adding +another child in its main can open an expanding branch. cld/allow-duplicate? +combines copy-structure and nesting guards for normal frontend/pure nodes. +The duplicate builder checks the actual destination after ALT placement; +a duplicate at the page root remains allowed. Undo still restores existing +nesting through its explicit inverse flag, not through a forward bypass. diff --git a/common/src/app/common/logic/duplicate.cljc b/common/src/app/common/logic/duplicate.cljc index 206290b240..62057ae908 100644 --- a/common/src/app/common/logic/duplicate.cljc +++ b/common/src/app/common/logic/duplicate.cljc @@ -494,6 +494,13 @@ objects (mapcat #(ctn/get-child-heads objects %) selected)))) +(defn allow-duplicate? + "Whether a duplicate at the same position preserves component structure." + [objects shape] + (and (some? shape) + (ctk/allow-duplicate? objects shape) + (not (cfh/components-nesting-loop? objects (:id shape) (:parent-id shape))))) + (defn generate-duplicate-changes "Prepare objects to duplicate: generate new ids, move them to the desired position, and recalculate parents and frames as needed." @@ -511,11 +518,12 @@ ;; This is because inside a variant-container can't be a copy ;; For other shape this way the layout won't be changed when duplicated ;; and if you move outside the layout will not change - shapes (map (fn [shape] - (cond-> shape - alt-duplication? - (assoc :parent-id uuid/zero :frame-id uuid/zero))) - shapes) + shapes (->> shapes + (map (fn [shape] + (cond-> shape + alt-duplication? + (assoc :parent-id uuid/zero :frame-id uuid/zero)))) + (remove #(cfh/components-nesting-loop? all-objects (:id %) (:parent-id %)))) changes (-> changes diff --git a/common/src/app/common/test_helpers/composable/comp/nodes.cljc b/common/src/app/common/test_helpers/composable/comp/nodes.cljc index f888032fce..b9e04c08ac 100644 --- a/common/src/app/common/test_helpers/composable/comp/nodes.cljc +++ b/common/src/app/common/test_helpers/composable/comp/nodes.cljc @@ -988,9 +988,9 @@ shape-id (tm/target-shape-id situation target) page (thf/current-page the-file) objects (:objects page) - _ (when-not (ctk/allow-duplicate? objects (get objects shape-id)) + _ (when-not (cld/allow-duplicate? objects (get objects shape-id)) ;; the frontend's `duplicate-shapes` drops it and does nothing - (throw (ex-info "The shape cannot be duplicated (inside a copy)" + (throw (ex-info "The shape cannot be duplicated in its current parent" {:type ::duplicate-refused :target target}))) changes (-> (pcb/empty-changes nil) (cld/generate-duplicate-changes objects diff --git a/common/src/app/common/test_helpers/composable/gen.cljc b/common/src/app/common/test_helpers/composable/gen.cljc index ddf03b4c18..5f805289a9 100644 --- a/common/src/app/common/test_helpers/composable/gen.cljc +++ b/common/src/app/common/test_helpers/composable/gen.cljc @@ -11,6 +11,7 @@ (:refer-clojure :exclude [flush]) (:require [app.common.files.helpers :as cfh] + [app.common.logic.duplicate :as cld] [app.common.logic.shapes :as cls] [app.common.test-helpers.composable.comp.files :as cf] [app.common.test-helpers.composable.comp.nodes :as n] @@ -120,7 +121,8 @@ :add-child (ctk/main-instance? shape) :remove-child (and (= :rect (:type shape)) (nil? (:shape-ref shape)) (some ctk/main-instance? (cfh/get-parents-with-self objects (:id shape)))) - :duplicate (and (top-copy shape) (< (count objects) 120)) + :duplicate (and (top-copy shape) (< (count objects) 120) + (cld/allow-duplicate? objects shape)) :nest (and (top-copy shape) (not (overridden (:id shape)))) :delete (ctk/main-instance? shape) false)) diff --git a/common/test/common_tests/logic/copying_and_duplicating_test.cljc b/common/test/common_tests/logic/copying_and_duplicating_test.cljc index 2c8f1161ee..404ab7a991 100644 --- a/common/test/common_tests/logic/copying_and_duplicating_test.cljc +++ b/common/test/common_tests/logic/copying_and_duplicating_test.cljc @@ -8,10 +8,15 @@ (:require [app.common.files.changes :as ch] [app.common.files.changes-builder :as pcb] + [app.common.geom.point :as gpt] + [app.common.logic.duplicate :as cld] [app.common.logic.libraries :as cll] [app.common.logic.shapes :as cls] [app.common.pprint :as pp] [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] @@ -24,6 +29,38 @@ (t/use-fixtures :each thi/test-fixture) +(t/deftest duplicating-existing-component-nesting-does-not-open-an-expanding-branch + (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) + (thc/instantiate-component :b :copy-b :children-labels [:copy-a]) + (tho/add-rect :safe)) + nested (-> (tm/make-situation file) + (r/step (n/swap-shape :copy-a :other)) r/settle + (r/step (n/nest-copy :copy-b :main-a)) r/settle) + file (tm/file nested) + page (thf/current-page file) + objects (:objects page) + id (thi/id :nested-a) + generate (fn [ids & options] + (apply cld/generate-duplicate-changes (pcb/empty-changes) + objects page ids (gpt/point 0 0) {(:id file) file} + (:data file) (:id file) options)) + refused (generate #{id}) + mixed (generate #{id (thi/id :safe)}) + added (filter #(= :add-obj (:type %)) (:redo-changes mixed)) + outside (generate #{id} :alt-duplication? true) + root (first (filter #(and (= :add-obj (:type %)) (= id (:old-id %))) + (:redo-changes outside)))] + (t/is (empty? (:redo-changes refused))) + (t/is (= (:data file) (:data (thf/apply-changes file refused)))) + (t/is (not-any? #(= id (:old-id %)) added)) + (t/is (some #(= (thi/id :safe) (:old-id %)) added)) + (t/is (= uuid/zero (get-in root [:obj :parent-id]))))) + (defn- setup [] (-> (thf/sample-file :file1) (tho/add-simple-component :simple-1 :frame-simple-1 :rect-simple-1 diff --git a/frontend/src/app/main/data/workspace/selection.cljs b/frontend/src/app/main/data/workspace/selection.cljs index 063e85cb04..d7c42ae959 100644 --- a/frontend/src/app/main/data/workspace/selection.cljs +++ b/frontend/src/app/main/data/workspace/selection.cljs @@ -473,7 +473,9 @@ objects (:objects page) ids (into #{} (comp (map (d/getf objects)) - (filter #(ctk/allow-duplicate? objects %)) + (filter #(if alt-duplication? + (ctk/allow-duplicate? objects %) + (cld/allow-duplicate? objects %))) (map :id)) ids)] (when (seq ids)