From b3a93655cf679bb0f7989000871c6db1c60be5a0 Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Thu, 15 Oct 2026 15:10:05 +0200 Subject: [PATCH] :bug: Fix the container helpers for copies and components Fixes in the helpers that walk, instantiate, compare and absorb components (Phase 3c). - List each shape once in get-children-in-instance. The recursion handed each child the vector built so far, ancestors included, and the caller added what came back to its own copy of that vector, so every ancestor came back once per child below it (F59). It now threads one result through a reduce. Both callers, the repair of :shape-ref-cycle and migration legacy-65, make a set of the result, so they see the same shapes. container-test counts the shapes of an instance. - Walk every ancestor in has-any-copy-parent?. It recursed with the parent's id where it expects a shape, so it only ever looked at the direct parent (F63). It now walks up to the root, which is its own parent. Users never saw it: every descendant of a copy is a copy, so the direct parent gave the right answer. container-test checks a shape two levels below the copy. - Start a new copy without the remote-synced mark of its main. Instantiating a component cleared the touched groups of the shapes it copies from the main, but not :remote-synced, so a nested copy in the main that had been synced from a remote main passed the mark on to every new copy of the component, whose shapes refer to their near main (F18). make-component-instance now clears it too. container-test: a new copy of a component whose nested copy is touched and remote-synced has neither mark. - Compare the stored shapes of components by value. mod-component marks a component modified when the change alters any of its keys, which ctk/diff-components reports. For :objects, the shapes a deleted component stores, it compared the ids found by walking :children, a shape format older than the objects map, so it read nil on both sides and never saw a change there (F96). It now compares every key, :objects included, by value. container-test: a change to a stored shape of a deleted component is a difference; the same component is none. - Keep nested copies and annotations of absorbed components. When a file absorbs a library's components (the library is deleted, or a binfile export merges libraries), each used component gets a main in the file, a copy of the library main with the same ids (F89): - every shape of that main lost its :shape-ref, so the copies nested in it stopped being copies of anything; - every head in it was pointed at the local file, even one whose component lives in a third library; - the component lost its annotation. The main now keeps the refs of its nested copies, which with the same ids point at the absorbed mains of the components they nest (a file that shows a nested copy uses its component, so that one is absorbed too); only heads of the absorbed library are pointed at the local file; the annotation is copied. A variant still becomes a plain component. absorb-assets-test: the nested copy of an absorbed main keeps its ref and is a copy of the local component; the annotation stays. - Say what the component placement rule checks. invalid-structure-for-component? said it kept a shape out of its own subtree, but its check compares components only: a plain shape moved into its own child passes, and the move itself refuses it (F58). The comments now say what it checks; the behavior stays, as the layers panel relies on it to do nothing for such a drop (decision (s)). Benchmark (lower quantile, ms; baseline / now / ratio): deepest-edit-through-three-levels 35.2 / 17.0 / 0.48 edit-through-a-chain-of-dependents 68.9 / 33.5 / 0.49 library-update-into-copies 83.1 / 40.1 / 0.48 main-edit-among-many-shapes 4.8 / 2.1 / 0.43 main-edit-into-10-copies 1.8 / 0.8 / 0.47 main-edit-into-100-copies 15.4 / 8.3 / 0.54 main-edit-into-1000-copies 153.1 / 82.3 / 0.54 variant-switch-into-copies 32.1 / 18.6 / 0.58 AI-assisted-by: claude-opus-5-5 --- common/src/app/common/types/component.cljc | 18 ++------- common/src/app/common/types/container.cljc | 37 +++++++++---------- common/src/app/common/types/file.cljc | 6 ++- .../types/absorb_assets_test.cljc | 21 +++++++++++ .../common_tests/types/container_test.cljc | 29 ++++++++++++--- 5 files changed, 70 insertions(+), 41 deletions(-) diff --git a/common/src/app/common/types/component.cljc b/common/src/app/common/types/component.cljc index ff07b66e2c..0e82eff410 100644 --- a/common/src/app/common/types/component.cljc +++ b/common/src/app/common/types/component.cljc @@ -313,24 +313,12 @@ :component-file component-file :component-id component-id)) -(defn- extract-ids [shape] - (if (map? shape) - (let [current-id (:id shape) - child-ids (mapcat extract-ids (:children shape))] - (cons current-id child-ids)) - [])) - (defn diff-components "Compare two components, and return a set of the keys with different values" [comp1 comp2] - (let [eq (fn [key val1 val2] - (if (= key :objects) - (= (extract-ids val1) (extract-ids val2)) - (= val1 val2)))] - (->> (concat (keys comp1) (keys comp2)) - (distinct) - (filter #(not (eq % (get comp1 %) (get comp2 %)))) - set))) + (into #{} + (filter #(not= (get comp1 %) (get comp2 %))) + (concat (keys comp1) (keys comp2)))) (defn allow-duplicate? [objects shape] diff --git a/common/src/app/common/types/container.cljc b/common/src/app/common/types/container.cljc index 9b569e04a7..3757b6c692 100644 --- a/common/src/app/common/types/container.cljc +++ b/common/src/app/common/types/container.cljc @@ -102,14 +102,14 @@ "Get the shape and their children recursively, but stopping when a component nested instance is found." [objects id] - (letfn [(get-children-rec [children id] + (letfn [(get-children-rec [result id top?] (let [shape (get objects id)] - (if (and (ctk/instance-head? shape) (seq children)) - children - (let [children' (conj children shape)] - (into children' - (mapcat #(get-children-rec children' %) (:shapes shape)))))))] - (get-children-rec [] id))) + (if (and (ctk/instance-head? shape) (not top?)) + result + (reduce #(get-children-rec %1 %2 false) + (conj result shape) + (:shapes shape)))))] + (get-children-rec [] id true))) (defn get-component-shape "Get the parent top shape linked to a component main for this shape, if any" @@ -347,7 +347,7 @@ (cond-> new-shape :always (-> (gsh/move delta) - (dissoc :touched :variant-id :variant-name)) + (dissoc :touched :remote-synced :variant-id :variant-name)) (and main-instance? root?) (assoc :main-instance true) @@ -355,9 +355,6 @@ (not main-instance?) (dissoc :main-instance) - main-instance? - (dissoc :shape-ref) - (not main-instance?) (assoc :shape-ref (:id original-shape)) ; shape-ref points to the near instance @@ -408,11 +405,10 @@ "Check if the shape has any parent that is a copy of a component." [objects shape] (let [parent (get objects (:parent-id shape))] - (if (nil? parent) - false - (if (ctk/in-component-copy? parent) - true - (has-any-copy-parent? objects (:parent-id shape)))))) + (cond + (or (nil? parent) (cfh/root? shape)) false + (ctk/in-component-copy? parent) true + :else (has-any-copy-parent? objects parent)))) (defn detachable? "Whether the shape can be detached from its component: a copy shape that @@ -451,7 +447,9 @@ (get-in libraries [(:component-file shape) :data :components (:component-id shape)])) (defn invalid-structure-for-component? - "Check if the structure generated nesting children in parent is invalid in terms of nested components" + "Check if the structure generated nesting children in parent is invalid in + terms of nested components: a change of a copy's structure, a main inside + a component, or a component nesting loop." [objects parent children pasting? libraries] (let [; If the original shapes had been cutted, and we are pasting them now, they aren't ; in objects. We can add them to locate later @@ -480,8 +478,9 @@ (has-any-copy-parent? objects parent) ;; If we are moving something containing a main instance the container can't be part of a component (neither main nor copy) (and selected-main-instance? parent-in-component?) - ;; Avoid placing a shape as a direct or indirect child of itself, - ;; or inside its main component if it's in a copy. + ;; Avoid a component nesting loop: a main inside itself, or a copy + ;; inside its own main. A plain shape moved into its own subtree passes; + ;; the move itself (`relocate-shapes`) refuses it. comps-nesting-loop?))) (defn parent-validation-cache diff --git a/common/src/app/common/types/file.cljc b/common/src/app/common/types/file.cljc index ae5eaa0332..549ac4a3c3 100644 --- a/common/src/app/common/types/file.cljc +++ b/common/src/app/common/types/file.cljc @@ -775,9 +775,12 @@ {:main-instance? true :keep-ids? true}) + ;; the heads of the library's components now point to the + ;; local ones; with the same ids, the nested copies keep + ;; their refs main-instance-shapes (map #(cond-> % - (some? (:component-file %)) + (= (:component-file %) (:id library-data)) (assoc :component-file (:id file-data))) main-instance-shapes) @@ -802,6 +805,7 @@ {:id (:id component) :name (:name component) :path (:path component) + :annotation (:annotation component) :main-instance-id (:id main-instance-shape) :main-instance-page page-id})) diff --git a/common/test/common_tests/types/absorb_assets_test.cljc b/common/test/common_tests/types/absorb_assets_test.cljc index 6d2e46e0db..d053fa6735 100644 --- a/common/test/common_tests/types/absorb_assets_test.cljc +++ b/common/test/common_tests/types/absorb_assets_test.cljc @@ -103,6 +103,27 @@ (t/is (some? (:objects card')) "the deleted component keeps its shapes") (t/is (empty? library-refs') "no shape points to the library") (t/is (nil? errors')))) +(t/deftest absorb-nested-components + ;; component2's main nests a copy of component1, and has an annotation; + ;; the file has a copy of component2 + (let [library (-> (thf/sample-file :library :is-shared true) + (tho/add-nested-component :component1 :main1-root :main1-child + :component2 :main2-root :nested-head) + (thc/update-component :component2 :annotation "Note")) + file (-> (thf/sample-file :file) + (thc/instantiate-component :component2 :copy2-root :library library + :children-labels [:copy-nested-head])) + file' (ctf/update-file-data file #(ctf/absorb-assets % (:data library))) + data' (ctf/file-data file') + page' (second (ctpl/pages-seq data')) + nested' (get-in page' [:objects (thi/id :nested-head)])] + (thf/validate-file! file') + (t/is (= 2 (count (ctkl/components-seq data')))) + (t/testing "the nested copy in the absorbed main keeps its link" + (t/is (= (thi/id :main1-root) (:shape-ref nested'))) + (t/is (= (:id file') (:component-file nested')))) + (t/testing "the component keeps its annotation" + (t/is (= "Note" (:annotation (ctkl/get-component data' (thi/id :component2)))))))) (t/deftest absorb-colors (let [;; Setup diff --git a/common/test/common_tests/types/container_test.cljc b/common/test/common_tests/types/container_test.cljc index be40472843..6857a0c0fc 100644 --- a/common/test/common_tests/types/container_test.cljc +++ b/common/test/common_tests/types/container_test.cljc @@ -8,7 +8,6 @@ (:require [app.common.geom.point :as gpt] [app.common.test-helpers.components :as thc] - [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] @@ -332,8 +331,7 @@ labels #(set (map (comp label :id) (ctc/get-children-in-instance objects (thi/id %))))] (t/is (= #{:main1 :main1-child} (labels :main1))) (t/is (= #{:main2} (labels :main2))) - ;; each ancestor comes back once per child below it - (tm/known-failure "F59" 2 3 (count (ctc/get-children-in-instance objects (thi/id :main1)))))) + (t/is (= 2 (count (ctc/get-children-in-instance objects (thi/id :main1))))))) (t/deftest copy-parent-above-a-plain-parent ;; a rect in a plain group inside a copy: the copy is two levels up @@ -346,9 +344,7 @@ rect (make-shape (uuid/next) :parent-id group-id) objects (objects-map copy group rect)] (t/is (ctc/has-any-copy-parent? objects group)) - ;; the walk passes on the parent's id where it expects a shape, so it - ;; stops after the direct parent - (tm/known-failure "F63" true false (ctc/has-any-copy-parent? objects rect)))) + (t/is (ctc/has-any-copy-parent? objects rect)))) (t/deftest nesting-level-delta-of-a-move-between-copies ;; :copy2-root #--> [Component :component2] @@ -406,3 +402,24 @@ (t/is (= 2 (count shapes))) (t/testing "each shape references its main" (t/is (= #{(thi/id :main1) (thi/id :main1-child)} (set (map :shape-ref shapes))))))) + +(t/deftest a-new-copy-starts-clean + ;; the main of component2 nests a copy of component1, synced from a remote + ;; main and touched; a new copy of component2 has neither mark + (let [file (-> (thf/sample-file :file) + (tho/add-nested-component :c1 :main1 :main1-child + :c2 :main2 :nested-head) + (ths/update-shape :nested-head :remote-synced true) + (ths/update-shape :nested-head :touched #{:fill-group}) + (thc/instantiate-component :c2 :copy2 :children-labels [:copy-nested-head])) + shape (ths/get-shape file :copy-nested-head)] + (t/is (nil? (:touched shape))) + (t/is (nil? (:remote-synced shape))))) + +(t/deftest a-change-of-the-shapes-of-a-deleted-component-marks-it-modified + (let [component {:id (uuid/next) :name "C" :deleted true :modified-at 1 + :objects {uuid/zero {:id uuid/zero :name "Old"}}} + changed (assoc-in component [:objects uuid/zero :name] "New")] + (t/is (= #{:objects} (ctk/diff-components component changed))) + (t/is (= #{} (ctk/diff-components component component))))) +