mirror of
https://github.com/penpot/penpot.git
synced 2026-10-31 06:50:33 +00:00
🐛 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
This commit is contained in:
parent
58e23f3f2c
commit
560208ce74
@ -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.
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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))
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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)
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user