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

🐛 Fix the container helpers for copies and components

Browse Source 
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
This commit is contained in:
alonso.torres 2026-10-15 15:10:05 +02:00
parent bef4b2f387
commit b3a93655cf
5 changed files with 70 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
common/src/app/common/types/component.cljc
View File

@ -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]

37
common/src/app/common/types/container.cljc
View File

@ -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

6
common/src/app/common/types/file.cljc
View File

@ -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}))

21
common/test/common_tests/types/absorb_assets_test.cljc
View File

@ -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

29
common/test/common_tests/types/container_test.cljc
View File

@ -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)))))