From ae22eaa2960a36bc4d1b35810dfc97d46d7fc20b Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Wed, 14 Oct 2026 16:04:16 +0200 Subject: [PATCH] :fire: Remove dead code from the component helpers - generate-relocate set :component-root on the moved shapes twice. The second pass looked at the same copy heads again and wrote the values the first had written, and update-shapes had already applied them, so it emitted nothing. It goes. Delete and relocate found the masked groups whose mask leaves them with the same reduce; groups-losing-mask finds them for both. The comment "Remove the hide in viewer flag" sat above code that sets it. - generate-duplicate-shape-change took unames, update-unames! and ids only to pass them to its own recursive call, assigned each shape the name it already had, and had a 13-argument arity that nothing called. It takes the values that stay the same along the subtree in one map, and it is private. - make-component-instance collected the names used in the page and the new ids in two volatiles: the names were never read, and the ids only fed a second remap of the grid cells, which clone-shape already does. - ctk/in-component-copy-not-root? had no caller, and ctn/get-container-root and ctk/valid-touched-group? only their tests. - ctf/swapped-subhead? tells whether a copy head refers to a main shape that is not a child of the main shape its parent refers to; its docstring still spoke of swap slots. It is ref-outside-near-main?. - dump-shape-component-info built the name of a nested head's component in a branch whose condition ended in `true`, so it never ran. No behavior change: the golden masters and the suites are unchanged. Benchmark (lower quantile, ms; baseline / now / ratio): deepest-edit-through-three-levels 35.2 / 16.1 / 0.46 edit-through-a-chain-of-dependents 68.9 / 30.1 / 0.44 library-update-into-copies 83.1 / 39.4 / 0.47 main-edit-among-many-shapes 4.8 / 2.2 / 0.45 main-edit-into-10-copies 1.8 / 0.8 / 0.44 main-edit-into-100-copies 15.4 / 7.7 / 0.50 main-edit-into-1000-copies 153.1 / 78.1 / 0.51 variant-switch-into-copies 32.1 / 18.5 / 0.58 AI-assisted-by: claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/app/common/files/comp_processors.cljc | 5 +- common/src/app/common/files/validate.cljc | 4 +- common/src/app/common/logic/duplicate.cljc | 59 ++++++-------- common/src/app/common/logic/shapes.cljc | 79 ++++--------------- common/src/app/common/types/component.cljc | 21 ----- common/src/app/common/types/container.cljc | 35 ++------ common/src/app/common/types/file.cljc | 21 +---- common/test/common_tests/runner.cljc | 2 - .../common_tests/types/components_test.cljc | 21 ----- .../common_tests/types/container_test.cljc | 6 -- 10 files changed, 54 insertions(+), 199 deletions(-) delete mode 100644 common/test/common_tests/types/components_test.cljc diff --git a/common/src/app/common/files/comp_processors.cljc b/common/src/app/common/files/comp_processors.cljc index d5f9c92bfa..724daa307c 100644 --- a/common/src/app/common/files/comp_processors.cljc +++ b/common/src/app/common/files/comp_processors.cljc @@ -165,7 +165,8 @@ file-data))) (defn fix-missing-swap-slots - "Locate shapes that have been swapped (see `ctf/swapped-subhead?`) but don't have a swap slot. + "Locate shapes that have been swapped (their ref is outside the near main, see + `ctf/ref-outside-near-main?`) but don't have a swap slot. In this case, add one pointing to the near match." [file-data libraries] (try @@ -175,7 +176,7 @@ (if (ctk/subcopy-head? shape) (let [container (:container (meta shape)) file {:id (:id file-data) :data file-data} - swapped? (ctf/swapped-subhead? + swapped? (ctf/ref-outside-near-main? shape container #(ctf/find-ref-shape file container libraries % :include-deleted? true)) near-match (when swapped? diff --git a/common/src/app/common/files/validate.cljc b/common/src/app/common/files/validate.cljc index ada143cdfb..780c627f91 100644 --- a/common/src/app/common/files/validate.cljc +++ b/common/src/app/common/files/validate.cljc @@ -409,8 +409,8 @@ "Validate that a nested copy head refers to a child of the near main of its parent: the shape whose position it takes." [shape file page libraries] - (when (ctf/swapped-subhead? shape page #(ctf/find-content-ref-shape file page libraries % - :include-deleted? true)) + (when (ctf/ref-outside-near-main? shape page #(ctf/find-content-ref-shape file page libraries % + :include-deleted? true)) (report-error :ref-not-in-near-main (str/ffmt "Referenced shape % is not a child of the near main of the parent" (:shape-ref shape)) shape file page))) diff --git a/common/src/app/common/logic/duplicate.cljc b/common/src/app/common/logic/duplicate.cljc index 5494bef24b..206290b240 100644 --- a/common/src/app/common/logic/duplicate.cljc +++ b/common/src/app/common/logic/duplicate.cljc @@ -309,13 +309,17 @@ {}))] changes)) -(defn generate-duplicate-shape-change - ([changes objects page unames update-unames! ids ids-map obj delta level-delta libraries library-data file-id] - (generate-duplicate-shape-change changes objects page unames update-unames! ids ids-map obj delta level-delta libraries library-data file-id (:frame-id obj) (:parent-id obj) false false nil)) - ([changes objects page unames update-unames! ids ids-map obj delta level-delta libraries library-data file-id variant-props] - (generate-duplicate-shape-change changes objects page unames update-unames! ids ids-map obj delta level-delta libraries library-data file-id (:frame-id obj) (:parent-id obj) false false variant-props)) - - ([changes objects page unames update-unames! ids ids-map obj delta level-delta libraries library-data file-id frame-id parent-id duplicating-component? child? variant-props] +(defn- generate-duplicate-shape-change + "Add to `changes` a duplicate of `obj` and its subtree in `objects`, under + `parent-id` and `frame-id`, and `level-delta` nesting levels shallower + (computed when nil). `ctx` holds what stays the same along the subtree: + the `:page`, the new id of each shape (`:ids-map`), the `:delta` they + move, the `:libraries`, the `:library-data`, the `:file-id` and the + `:variant-props` of a duplicated variant." + ([changes ctx objects obj] + (generate-duplicate-shape-change changes ctx objects obj nil (:frame-id obj) (:parent-id obj) false false)) + ([changes {:keys [page ids-map delta libraries library-data file-id variant-props] :as ctx} + objects obj level-delta frame-id parent-id duplicating-component? child?] (cond (nil? obj) changes @@ -330,7 +334,6 @@ new-id (ids-map (:id obj)) parent-id (or parent-id frame-id) parent (get objects parent-id) - name (:name obj) is-component-root? (or (:saved-component-root obj) ;; Backward compatibility @@ -365,7 +368,6 @@ new-obj (-> obj (assoc :id new-id - :name name :parent-id parent-id :frame-id frame-id) @@ -431,24 +433,15 @@ (reduce (fn [changes child] (generate-duplicate-shape-change changes + ctx (:objects page') - page - unames - update-unames! - ids - ids-map child - delta ;; the children of a swapped head refer into its own component (if (ctk/swapped? obj) 0 level-delta) - libraries - library-data - file-id (if frame? new-id frame-id) new-id duplicating-component? - true - variant-props)) + true)) changes (map (d/getf objects) (:shapes obj))))))) @@ -502,12 +495,10 @@ (mapcat #(ctn/get-child-heads objects %) selected)))) (defn generate-duplicate-changes - "Prepare objects to duplicate: generate new id, give them unique names, - move to the desired position, and recalculate parents and frames as needed." + "Prepare objects to duplicate: generate new ids, move them to the desired + position, and recalculate parents and frames as needed." [changes all-objects page ids delta libraries library-data file-id & {:keys [variant-props alt-duplication?]}] (let [shapes (map (d/getf all-objects) ids) - unames (volatile! (cfh/get-used-names (:objects page))) - update-unames! (fn [new-name] (vswap! unames conj new-name)) all-ids (reduce #(into %1 (cons %2 (cfh/get-children-ids all-objects %2))) (d/ordered-set) ids) ;; We need ids-map for remapping the grid layout. But when duplicating the guides @@ -535,19 +526,15 @@ changes (->> shapes (reduce #(generate-duplicate-shape-change %1 + {:page page + :ids-map ids-map + :delta delta + :libraries libraries + :library-data library-data + :file-id file-id + :variant-props variant-props} all-objects - page - unames - update-unames! - ids - ids-map - %2 - delta - nil - libraries - library-data - file-id - variant-props) + %2) changes)) ;; We need to check the changes to get the ids-map diff --git a/common/src/app/common/logic/shapes.cljc b/common/src/app/common/logic/shapes.cljc index 14e8ce254c..7324acaae2 100644 --- a/common/src/app/common/logic/shapes.cljc +++ b/common/src/app/common/logic/shapes.cljc @@ -232,6 +232,17 @@ changes copy-deletions)) +(defn- groups-losing-mask + "The masked groups whose mask, their first child, is among `ids`." + [objects ids] + (into #{} + (keep (fn [id] + (let [parent (get objects (:parent-id (get objects id)))] + (when (and (:masked-group parent) + (= id (first (:shapes parent)))) + (:id parent))))) + ids)) + (defn generate-delete-shapes ([changes file page objects ids options] (generate-delete-shapes (-> changes @@ -288,21 +299,10 @@ lookup (d/getf objects) + ;; a group whose mask is deleted becomes a plain group groups-to-unmask (when-not ignore-mask - (reduce (fn [group-ids id] - ;; When the shape to delete is the mask of a masked group, - ;; the mask condition must be removed, and it must be - ;; converted to a normal group. - (let [obj (lookup id) - parent (lookup (:parent-id obj))] - (if (and (:masked-group parent) - (= id (first (:shapes parent)))) - (conj group-ids (:id parent)) - group-ids))) - #{} - ids-to-delete) - []) + (groups-losing-mask objects ids-to-delete)) id-to-delete? (set ids-to-delete) @@ -495,46 +495,9 @@ removed-id? result))))) + ;; a group whose mask moves out of it becomes a plain group groups-to-unmask - (reduce (fn [group-ids id] - ;; When a masked group loses its mask shape, because it's - ;; moved outside the group, the mask condition must be - ;; removed, and it must be converted to a normal group. - (let [obj (get objects id) - parent (get objects (:parent-id obj))] - (if (and (:masked-group parent) - (= id (first (:shapes parent))) - (not= (:id parent) parent-id)) - (conj group-ids (:id parent)) - group-ids))) - #{} - ids) - - ;; TODO: Probably implementing this using loop/recur will - ;; be more efficient than using reduce and continuous data - ;; desturcturing. - - ;; Sets the correct components metadata for the moved copy heads - ;; `shapes-to-deroot` Removes the root flag from a component instance moved inside another component - ;; `shapes-to-reroot` Adds a root flag when a nested component instance is moved outside - [shapes-to-deroot shapes-to-reroot] - (reduce (fn [[shapes-to-deroot shapes-to-reroot] id] - (let [shape (get objects id) - parent (get objects parent-id) - component-shape-parent (ctn/get-component-shape objects parent {:allow-main? true}) - root-parent (ctn/get-instance-root objects parent) - - deroot? (and (ctk/instance-root? shape) - root-parent) - reroot? (and (ctk/subinstance-head? shape) - (not component-shape-parent))] - - [(cond-> shapes-to-deroot deroot? (conj id)) - (cond-> shapes-to-reroot reroot? (conj id))])) - [[] []] - (->> ids - (mapcat #(ctn/get-child-heads objects %)) - (map :id))) + (disj (groups-losing-mask objects ids) parent-id) index-cell-data (when to-index (ctl/get-cell-by-index parent to-index)) cell (or cell (and index-cell-data [(:row index-cell-data) (:column index-cell-data)])) @@ -557,7 +520,7 @@ (-> (pcb/update-shapes ids ctl/remove-layout-item-data) (pcb/update-shapes ids cto/unapply-layout-item-tokens))) - ;; Remove the hide in viewer flag + ;; Hide in the viewer the boards moved into a board (cond-> (and (not= uuid/zero parent-id) (cfh/frame-shape? parent)) (pcb/update-shapes ids #(cond-> % (cfh/frame-shape? %) (assoc :hide-in-viewer true)))) @@ -590,16 +553,6 @@ (fn [shape] (assoc shape :masked-group false))) - ;; Make non root a component moved inside another one - (pcb/update-shapes shapes-to-deroot - (fn [shape] - (assoc shape :component-root nil))) - - ;; Make root a subcomponent moved outside its parent component - (pcb/update-shapes shapes-to-reroot - (fn [shape] - (assoc shape :component-root true))) - ;; Reset constraints depending on the new parent (pcb/update-shapes ids (fn [shape] diff --git a/common/src/app/common/types/component.cljc b/common/src/app/common/types/component.cljc index f49a52641c..90785a8ae3 100644 --- a/common/src/app/common/types/component.cljc +++ b/common/src/app/common/types/component.cljc @@ -6,7 +6,6 @@ (ns app.common.types.component (:require - [app.common.exceptions :as ex] [app.common.schema :as sm] [app.common.time :as-alias ct] [app.common.types.page :as ctp] @@ -220,13 +219,6 @@ (and (some? (:shape-ref shape)) (nil? (:component-id shape)))) -(defn in-component-copy-not-root? - "Check if the shape is inside a component non-main instance and - is not the root shape." - [shape] - (and (some? (:shape-ref shape)) - (nil? (:component-root shape)))) - (defn main-instance-of? "Check if this shape is the root of the main instance of the given component." [shape-id page-id component] @@ -332,16 +324,3 @@ (or (not (instance-head? shape)) (not (in-component-copy? parent)))))) -(def ^:private all-touched-groups - (reduce-kv (fn [acc _ v] - (if (map? v) - (into acc (vals v)) - (conj acc v))) - #{} - sync-attrs)) - -(defn valid-touched-group? - [group] - (ex/ignoring - (or (contains? all-touched-groups group) - (= group :swap-group)))) diff --git a/common/src/app/common/types/container.cljc b/common/src/app/common/types/container.cljc index 1a19f529d4..f61e08e783 100644 --- a/common/src/app/common/types/container.cljc +++ b/common/src/app/common/types/container.cljc @@ -94,10 +94,6 @@ [container shape-id f] (update-in container [:objects shape-id] f)) -(defn get-container-root - [container] - (d/seek #(or (nil? (:parent-id %)) (= (:parent-id %) uuid/zero)) (shapes-seq container))) - (defn get-direct-children [container shape] (map #(get-shape container %) (:shapes shape))) @@ -300,7 +296,7 @@ (defn make-component-instance "Generate a new instance of the component inside the given container. - Clone the shapes of the component, generating new names and ids, and + Clone the shapes of the component, generating new ids, and linking each new shape to the corresponding one of the component. Place the new instance coordinates in the given position. @@ -324,7 +320,6 @@ delta (gpt/subtract position orig-pos) objects (:objects page) - unames (volatile! (cfh/get-used-names objects)) component-children (d/index-by :id (cfh/get-children-with-self objects (:id component-shape))) @@ -346,18 +341,9 @@ frame (get-shape page frame-id) component-frame (get-component-shape objects frame {:allow-main? true}) - ids-map (volatile! {}) - update-new-shape (fn [new-shape original-shape] - (let [new-name (:name new-shape) - root? (ctk/instance-root? original-shape)] - - (when root? - (vswap! unames conj new-name)) - - (vswap! ids-map assoc (:id original-shape) (:id new-shape)) - + (let [root? (ctk/instance-root? original-shape)] (cond-> new-shape :always (-> (gsh/move delta) @@ -378,8 +364,7 @@ (nil? (:parent-id original-shape)) (assoc :component-id (:id component) :component-file (:id library-data) - :component-root true - :name new-name) + :component-root true) (or (some? (:parent-id original-shape)) ; On v2 we have removed the parent-id for component roots (some? component-frame)) @@ -395,17 +380,13 @@ :frame-id frame-id :dest-objects (:objects page)) - ;; Fix empty parent-id and remap all grid cells to the new ids. - remap-ids + ;; the root has no parent in the component; it goes to its frame + fix-parent-id (fn [shape] - (as-> shape $ - (update $ :parent-id #(or % (:frame-id $))) - (cond-> $ - (ctl/grid-layout? shape) - (ctl/remap-grid-cells @ids-map))))] + (update shape :parent-id #(or % (:frame-id shape))))] - [(remap-ids new-shape) - (map remap-ids new-shapes)]))) + [(fix-parent-id new-shape) + (map fix-parent-id new-shapes)]))) (defn get-first-valid-parent "Go through the parents until we find a shape that is not a copy of a component nor diff --git a/common/src/app/common/types/file.cljc b/common/src/app/common/types/file.cljc index e245e24034..f51b94fc37 100644 --- a/common/src/app/common/types/file.cljc +++ b/common/src/app/common/types/file.cljc @@ -496,7 +496,7 @@ (ctk/remove-swap-group))) (d/update-when shape :shape-ref #(get ref-map % %)))))) -(defn swapped-subhead? +(defn ref-outside-near-main? "Whether `shape` refers to a main shape that is not a child of the main shape its parent refers to (`find-parent-ref-shape`). Same-parent positional differences are synchronized as reorders." @@ -961,7 +961,7 @@ shape :include-deleted? true)] - (str/format " %s--> %s%s%s%s%s" + (str/format " %s--> %s%s%s%s" (cond (:component-root shape) "#" (:component-id shape) "@" :else "-") @@ -984,23 +984,6 @@ (when (and show-ids component-shape) (str/format " %s" (:id component-shape))) - (if (or (:component-root shape) - (nil? (:component-id shape)) - true) - "" - (let [component-id (:component-id shape) - component-file-id (:component-file shape) - component-file (when component-file-id (get libraries component-file-id nil)) - component (if component-file - (ctkl/get-component (:data component-file) component-id true) - (ctkl/get-component (:data file) component-id true))] - (str/format " (%s%s)" - (when component-file (str/format "<%s> " - (if (= (:id component-file) (:id file)) - "local" - (:name component-file)))) - (:name component)))) - (when (and show-ids (:component-id shape)) (str/format " [Component %s]" (:component-id shape))))))) diff --git a/common/test/common_tests/runner.cljc b/common/test/common_tests/runner.cljc index 9aa58a15d7..745fa4ed97 100644 --- a/common/test/common_tests/runner.cljc +++ b/common/test/common_tests/runner.cljc @@ -106,7 +106,6 @@ [common-tests.types-shape-tree-test] [common-tests.types.absorb-assets-test] [common-tests.types.color-test] - [common-tests.types.components-test] [common-tests.types.container-test] [common-tests.types.file-test] [common-tests.types.fill-test] @@ -227,7 +226,6 @@ 'common-tests.types-shape-tree-test 'common-tests.types.absorb-assets-test 'common-tests.types.color-test - 'common-tests.types.components-test 'common-tests.types.container-test 'common-tests.types.file-test 'common-tests.types.fill-test diff --git a/common/test/common_tests/types/components_test.cljc b/common/test/common_tests/types/components_test.cljc deleted file mode 100644 index 2f5eb545eb..0000000000 --- a/common/test/common_tests/types/components_test.cljc +++ /dev/null @@ -1,21 +0,0 @@ -;; 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 common-tests.types.components-test - (:require - [app.common.test-helpers.ids-map :as thi] - [app.common.types.component :as ctk] - [clojure.test :as t])) - -(t/use-fixtures :each thi/test-fixture) - -(t/deftest test-valid-touched-group - (t/is (ctk/valid-touched-group? :name-group)) - (t/is (ctk/valid-touched-group? :geometry-group)) - (t/is (ctk/valid-touched-group? :swap-group)) - (t/is (not (ctk/valid-touched-group? :this-is-not-a-group))) - (t/is (not (ctk/valid-touched-group? :swap-slot-9cc181fa-5eef-8084-8004-7bb2ab45fd1f))) - (t/is (not (ctk/valid-touched-group? nil)))) diff --git a/common/test/common_tests/types/container_test.cljc b/common/test/common_tests/types/container_test.cljc index 0fd1e8879d..be40472843 100644 --- a/common/test/common_tests/types/container_test.cljc +++ b/common/test/common_tests/types/container_test.cljc @@ -374,12 +374,6 @@ (t/is (not (ctc/page? container))) (t/is (= (thi/id :main1) (:main-instance-id container))))) -(t/deftest container-root - (let [root-id (uuid/next) - root (assoc (make-shape root-id) :parent-id nil) - child (make-shape (uuid/next) :parent-id root-id)] - (t/is (= root (ctc/get-container-root {:objects (objects-map root child)}))))) - (t/deftest swap-keep-attrs-keep-auto-sizing-of-a-layout (let [shape (make-shape (uuid/next) :type :frame