This commit is contained in:
alonso.torres 2026-06-02 09:21:23 +02:00
parent 06c9a18ab0
commit a343104d54
20 changed files with 2019 additions and 218 deletions

View File

@ -124,12 +124,11 @@
[changes f]
(update changes :redo-changes #(mapv f %)))
;; redo-changes is a vector and :undo-changes is a list
(defn concat-changes
[changes1 changes2]
(-> changes1
(update :redo-changes d/concat-vec (:redo-changes changes2))
(update :undo-changes #(concat (:undo-changes changes2) %))))
(update :undo-changes into (reverse (:undo-changes changes2)))))
; TODO: remove this when not needed
(defn- assert-page-id!

View File

@ -512,7 +512,7 @@
(and early-return? (seq (:redo-changes changes))))
changes
(recur (next containers)
(pcb/concat-changes ;;TODO Remove concat changes
(pcb/concat-changes
changes
(generate-sync-container (pcb/empty-changes nil)
asset-type
@ -554,7 +554,7 @@
(and early-return? (seq (:redo-changes changes))))
changes
(recur (next local-components)
(pcb/concat-changes ;;TODO Remove concat changes
(pcb/concat-changes
changes
(generate-sync-container (pcb/empty-changes nil)
asset-type
@ -832,7 +832,14 @@
;; If the component is not found, because the master component has been
;; deleted or the library unlinked, do nothing.
changes))
(do
(shape-log :debug shape-id container
:msg "Sync shape direct: skipped (component not found)"
:shape-id (str shape-id)
:component-id (str (:component-id shape-inst))
:component-file (str (:component-file shape-inst))
:library-linked? (some? library))
changes)))
changes)))
(defn- find-main-container
@ -871,7 +878,13 @@
(if (nil? shape-main)
;; This should not occur, but protect against it in any case
changes
(do
(shape-log :debug (:id shape-inst) container
:msg "Sync shape direct recursive: skipped (ref-shape not found)"
:shape-id (str (:id shape-inst))
:component-id (str (:id component))
:component-name (:name component))
changes)
(let [omit-touched? (not reset?)
clear-remote-synced? (and initial-root? reset?)
set-remote-synced? (and (not initial-root?) reset?)
@ -909,6 +922,8 @@
children-inst (vec (ctn/get-direct-children container shape-inst))
children-main (vec (ctn/get-direct-children component-container shape-main))
children-inst-index (into {} (map-indexed (fn [i s] [(:id s) i])) children-inst)
children-main-index (into {} (map-indexed (fn [i s] [(:id s) i])) children-main)
only-inst (fn [changes child-inst]
(shape-log :trace (:id child-inst) container
@ -932,8 +947,7 @@
:shapes-group))
(add-shape-to-instance changes
child-main
(d/index-of children-main
child-main)
(get children-main-index (:id child-main))
component-container
container
root-inst
@ -976,8 +990,8 @@
(move-shape
changes
child-inst
(d/index-of children-inst child-inst)
(d/index-of children-main child-main)
(get children-inst-index (:id child-inst))
(get children-main-index (:id child-main))
container
omit-touched?))
@ -1110,12 +1124,13 @@
(:shapes shape-inst))
children-main (mapv #(ctn/get-shape component-container %)
(:shapes shape-main))
children-inst-index (into {} (map-indexed (fn [i s] [(:id s) i])) children-inst)
children-main-index (into {} (map-indexed (fn [i s] [(:id s) i])) children-main)
only-inst (fn [changes child-inst]
(add-shape-to-main changes
child-inst
(d/index-of children-inst
child-inst)
(get children-inst-index (:id child-inst))
component
component-container
container
@ -1153,8 +1168,8 @@
(move-shape
changes
child-main
(d/index-of children-main child-main)
(d/index-of children-inst child-inst)
(get children-main-index (:id child-main))
(get children-inst-index (:id child-inst))
component-container
false))
@ -1175,17 +1190,13 @@
true
true)
;; The inverse sync may be made on a component that is inside a
;; remote library. We need to separate changes that are from
;; local and remote files.
check-local (fn [change]
(cond-> change
(= (:id change) (:id shape-inst))
(assoc :local-change? true)))]
;; Local-file tagging for the remote-library split is done eagerly at
;; change-creation time (change-touched, change-remote-synced, mod-obj-change
;; in add-shape-to-main), so no per-node re-mapping of the full accumulator
;; is needed here.
]
(-> changes
(update :redo-changes (partial mapv check-local))
(update :undo-changes (partial map check-local))))))
changes)))
;; ---- Operation generation helpers ----
@ -1193,76 +1204,104 @@
(defn- compare-children
[changes shape-inst children-inst children-main container-inst container-main file libraries only-inst-cb only-main-cb both-cb swapped-cb moved-cb inverse? reset?]
(shape-log :trace (:id shape-inst) container-inst :msg "Compare children")
(loop [children-inst (seq (or children-inst []))
children-main (seq (or children-main []))
changes changes]
(let [child-inst (first children-inst)
child-main (first children-main)]
(shape-log :trace (:id shape-inst) container-inst
:msg "Comparing"
:main (str (:name child-main) " " (pretty-uuid (:id child-main)))
:inst (str (:name child-inst) " " (pretty-uuid (:id child-inst))))
(cond
(and (nil? child-inst) (nil? child-main))
changes
(let [;; Precompute swap slots once per child to avoid repeated cross-file ref-chain lookups
inst-slots (when-not reset?
(into {} (keep (fn [c]
(when-let [s (ctf/find-swap-slot c container-inst file libraries)]
[(:id c) s])))
(or children-inst [])))
main-slots (when-not reset?
(into {} (keep (fn [c]
(when-let [s (ctf/find-swap-slot c container-main file libraries)]
[(:id c) s])))
(or children-main [])))
match-pair? (fn [main-child inst-child]
(or (ctk/is-main-of? main-child inst-child)
(let [slot-inst (get inst-slots (:id inst-child))]
(when (some? slot-inst)
(let [slot-main (get main-slots (:id main-child))]
(or (= slot-main slot-inst)
(= (:id main-child) slot-inst)))))))]
(loop [children-inst (seq (or children-inst []))
children-main (seq (or children-main []))
consumed-inst #{}
consumed-main #{}
changes changes]
(let [children-inst (drop-while #(consumed-inst (:id %)) children-inst)
children-main (drop-while #(consumed-main (:id %)) children-main)
child-inst (first children-inst)
child-main (first children-main)]
(shape-log :trace (:id shape-inst) container-inst
:msg "Comparing"
:main (str (:name child-main) " " (pretty-uuid (:id child-main)))
:inst (str (:name child-inst) " " (pretty-uuid (:id child-inst))))
(cond
(and (nil? child-inst) (nil? child-main))
changes
(nil? child-inst)
(reduce only-main-cb changes children-main)
(nil? child-inst)
(reduce only-main-cb changes (remove #(consumed-main (:id %)) children-main))
(nil? child-main)
(reduce only-inst-cb changes children-inst)
(nil? child-main)
(reduce only-inst-cb changes (remove #(consumed-inst (:id %)) children-inst))
:else
(if (or (ctk/is-main-of? child-main child-inst)
(and (ctf/match-swap-slot? child-main child-inst container-inst container-main file libraries)
(not reset?)))
(recur (next children-inst)
(next children-main)
(if (ctk/is-main-of? child-main child-inst)
(both-cb changes child-inst child-main)
(swapped-cb changes child-inst child-main)))
:else
(if (match-pair? child-main child-inst)
(recur (next children-inst)
(next children-main)
consumed-inst
consumed-main
(if (ctk/is-main-of? child-main child-inst)
(both-cb changes child-inst child-main)
(swapped-cb changes child-inst child-main)))
(let [child-inst' (d/seek #(or (ctk/is-main-of? child-main %)
(and (ctf/match-swap-slot? child-main % container-inst container-main file libraries)
(not reset?)))
children-inst)
child-main' (d/seek #(or (ctk/is-main-of? % child-inst)
(and (ctf/match-swap-slot? % child-inst container-inst container-main file libraries)
(not reset?)))
children-main)]
(cond
(nil? child-inst')
(recur children-inst
(next children-main)
(only-main-cb changes child-main))
(let [child-inst' (d/seek #(and (not (consumed-inst (:id %)))
(match-pair? child-main %))
children-inst)
child-main' (d/seek #(and (not (consumed-main (:id %)))
(match-pair? % child-inst))
children-main)]
(cond
(nil? child-inst')
(recur children-inst
(next children-main)
consumed-inst
consumed-main
(only-main-cb changes child-main))
(nil? child-main')
(recur (next children-inst)
children-main
(only-inst-cb changes child-inst))
(nil? child-main')
(recur (next children-inst)
children-main
consumed-inst
consumed-main
(only-inst-cb changes child-inst))
:else
(if inverse?
(let [is-main? (ctk/is-main-of? child-inst child-main')]
(recur (next children-inst)
(remove #(= (:id %) (:id child-main')) children-main)
(cond-> changes
is-main?
(both-cb child-inst child-main')
(not is-main?)
(swapped-cb child-inst child-main')
:always
(moved-cb child-inst child-main'))))
(let [is-main? (ctk/is-main-of? child-inst' child-main)]
(recur (remove #(= (:id %) (:id child-inst')) children-inst)
(next children-main)
(cond-> changes
is-main?
(both-cb child-inst' child-main)
(not is-main?)
(swapped-cb child-inst' child-main)
:always
(moved-cb child-inst' child-main))))))))))))
:else
(if inverse?
(let [is-main? (ctk/is-main-of? child-inst child-main')]
(recur (next children-inst)
children-main
consumed-inst
(conj consumed-main (:id child-main'))
(cond-> changes
is-main?
(both-cb child-inst child-main')
(not is-main?)
(swapped-cb child-inst child-main')
:always
(moved-cb child-inst child-main'))))
(let [is-main? (ctk/is-main-of? child-inst' child-main)]
(recur children-inst
(next children-main)
(conj consumed-inst (:id child-inst'))
consumed-main
(cond-> changes
is-main?
(both-cb child-inst' child-main)
(not is-main?)
(swapped-cb child-inst' child-main)
:always
(moved-cb child-inst' child-main)))))))))))))
(defn- add-shape-to-instance
[changes component-shape index component-page container root-instance root-main omit-touched? set-remote-synced?]
@ -1394,6 +1433,7 @@
{:type :mod-obj
:page-id (:id page)
:id (:id shape')
:local-change? true
:operations [{:type :set
:attr :component-id
:val (:component-id shape')}
@ -1413,6 +1453,7 @@
{:type :mod-obj
:page-id (:id page)
:id (:id shape-original)
:local-change? true
:operations [{:type :set
:attr :component-id
:val (:component-id shape-original)}
@ -1431,10 +1472,11 @@
del-obj-change (fn [changes shape']
(update changes :undo-changes conj
{:type :del-obj
:id (:id shape')
:page-id (:id page)
:ignore-touched true}))
(make-change
component-container
{:type :del-obj
:id (:id shape')
:ignore-touched true})))
changes' (reduce add-obj-change changes new-shapes)
changes' (update changes' :redo-changes conj {:type :reg-objects
@ -1566,22 +1608,25 @@
:else
(:touched dest-shape))]
(-> changes
(update :redo-changes conj (make-change
container
{:type :mod-obj
:id (:id dest-shape)
:operations
[{:type :set-touched
:touched new-touched}]}))
(update :undo-changes conj (make-change
container
{:type :mod-obj
:id (:id dest-shape)
:operations
[{:type :set-touched
:touched (:touched dest-shape)}]}))
(pcb/apply-changes-local))))))
(let [local? (cfh/page? container)]
(-> changes
(update :redo-changes conj (cond-> (make-change
container
{:type :mod-obj
:id (:id dest-shape)
:operations
[{:type :set-touched
:touched new-touched}]})
local? (assoc :local-change? true)))
(update :undo-changes conj (cond-> (make-change
container
{:type :mod-obj
:id (:id dest-shape)
:operations
[{:type :set-touched
:touched (:touched dest-shape)}]})
local? (assoc :local-change? true)))
(pcb/apply-changes-local)))))))
(defn- change-remote-synced
[changes shape container remote-synced?]
@ -1596,22 +1641,25 @@
" "
(pretty-uuid (:id shape)))
:remote-synced remote-synced?)
(-> changes
(update :redo-changes conj (make-change
container
{:type :mod-obj
:id (:id shape)
:operations
[{:type :set-remote-synced
:remote-synced remote-synced?}]}))
(update :undo-changes conj (make-change
container
{:type :mod-obj
:id (:id shape)
:operations
[{:type :set-remote-synced
:remote-synced (:remote-synced shape)}]}))
(pcb/apply-changes-local)))))
(let [local? (cfh/page? container)]
(-> changes
(update :redo-changes conj (cond-> (make-change
container
{:type :mod-obj
:id (:id shape)
:operations
[{:type :set-remote-synced
:remote-synced remote-synced?}]})
local? (assoc :local-change? true)))
(update :undo-changes conj (cond-> (make-change
container
{:type :mod-obj
:id (:id shape)
:operations
[{:type :set-remote-synced
:remote-synced (:remote-synced shape)}]})
local? (assoc :local-change? true)))
(pcb/apply-changes-local))))))
(defn- update-tokens
"Token synchronization algorithm. Copy the applied tokens that have changed

View File

@ -154,90 +154,91 @@
* Both has the same type of ancestors, on the same order (see generate-path for the
translation of the types)"
[changes new-shape original-shape original-shapes page libraries ldata]
(let [objects (pcb/get-objects changes)
container (ctn/make-container page :page)
page-objects (:objects page)
(binding [ctf/*find-ref-shape-cache* (volatile! {})]
(let [objects (pcb/get-objects changes)
container (ctn/make-container page :page)
page-objects (:objects page)
;; Get the touched children of the original-shape
;; Ignore children of swapped items, because
;; they will be moved without change when
;; managing their swapped ancestor
orig-touched (->> original-shapes
;; Add to each shape also the touched of its ref chain
(map #(add-touched-from-ref-chain container libraries %))
(filter (comp seq :touched))
(remove
#(child-of-swapped? %
page-objects
(:id original-shape))))
;; Get the touched children of the original-shape
;; Ignore children of swapped items, because
;; they will be moved without change when
;; managing their swapped ancestor
orig-touched (->> original-shapes
;; Add to each shape also the touched of its ref chain
(map #(add-touched-from-ref-chain container libraries %))
(filter (comp seq :touched))
(remove
#(child-of-swapped? %
page-objects
(:id original-shape))))
;; Adds a :shape-path attribute to the children of the new-shape,
;; that contains the type of its ancestors and its name
new-shapes-w-path (add-unique-path
(reverse (cfh/get-children-with-self objects (:id new-shape)))
objects
(:id new-shape))
;; Creates a map to quickly find a child of the new-shape by its shape-path
new-shapes-map (into {} (map (juxt :shape-path identity)) new-shapes-w-path)
;; Adds a :shape-path attribute to the children of the new-shape,
;; that contains the type of its ancestors and its name
new-shapes-w-path (add-unique-path
(reverse (cfh/get-children-with-self objects (:id new-shape)))
objects
(:id new-shape))
;; Creates a map to quickly find a child of the new-shape by its shape-path
new-shapes-map (into {} (map (juxt :shape-path identity)) new-shapes-w-path)
;; The original-shape is in a copy. For the relation rules, we need the referenced
;; shape on the main component
orig-base-ref-shape (ctf/find-remote-shape container libraries original-shape {:with-context? true})
orig-ref-objects (:objects (:container (meta orig-base-ref-shape)))
;; The original-shape is in a copy. For the relation rules, we need the referenced
;; shape on the main component
orig-base-ref-shape (ctf/find-remote-shape container libraries original-shape {:with-context? true})
orig-ref-objects (:objects (:container (meta orig-base-ref-shape)))
;; Adds a :shape-path attribute to the children of the orig-ref-shape,
;; that contains the type of its ancestors and its name
o-ref-shapes-wp (add-unique-path
(reverse (cfh/get-children-with-self orig-ref-objects (:id orig-base-ref-shape)))
orig-ref-objects
(:id orig-base-ref-shape))
;; Adds a :shape-path attribute to the children of the orig-ref-shape,
;; that contains the type of its ancestors and its name
o-ref-shapes-wp (add-unique-path
(reverse (cfh/get-children-with-self orig-ref-objects (:id orig-base-ref-shape)))
orig-ref-objects
(:id orig-base-ref-shape))
;; Creates a map to quickly find a child of the orig-ref-shape by its shape-path
o-ref-shapes-p-map (into {} (map (juxt :id :shape-path)) o-ref-shapes-wp)
;; Creates a map to quickly find a child of the orig-ref-shape by its shape-path
o-ref-shapes-p-map (into {} (map (juxt :id :shape-path)) o-ref-shapes-wp)
;; Process each touched children of the original-shape
[changes parents-of-swapped]
(reduce
(fn [[changes parent-of-swapped] orig-child-touched]
(let [;; If the orig-child-touched was swapped, get its swap-slot
swap-slot (ctk/get-swap-slot orig-child-touched)
;; Process each touched children of the original-shape
[changes parents-of-swapped]
(reduce
(fn [[changes parent-of-swapped] orig-child-touched]
(let [;; If the orig-child-touched was swapped, get its swap-slot
swap-slot (ctk/get-swap-slot orig-child-touched)
;; orig-child-touched is in a copy. Get the referenced shape on the main component
;; If there is a swap slot, we will get the referenced shape in another way
orig-ref-shape (when-not swap-slot
(find-shape-ref-child-of container libraries orig-child-touched (:id orig-base-ref-shape)))
;; orig-child-touched is in a copy. Get the referenced shape on the main component
;; If there is a swap slot, we will get the referenced shape in another way
orig-ref-shape (when-not swap-slot
(find-shape-ref-child-of container libraries orig-child-touched (:id orig-base-ref-shape)))
orig-ref-id (if swap-slot
;; If there is a swap slot, find the referenced shape id
(ctf/find-ref-id-for-swapped orig-child-touched container libraries)
;; If there is not a swap slot, get the id from the orig-ref-shape
(:id orig-ref-shape))
orig-ref-id (if swap-slot
;; If there is a swap slot, find the referenced shape id
(ctf/find-ref-id-for-swapped orig-child-touched container libraries)
;; If there is not a swap slot, get the id from the orig-ref-shape
(:id orig-ref-shape))
;; Get the shape path of the referenced main
shape-path (get o-ref-shapes-p-map orig-ref-id)
;; Get the shape path of the referenced main
shape-path (get o-ref-shapes-p-map orig-ref-id)
;; Get its related shape in the children of new-shape: the one that
;; has the same shape-path
related-shape-in-new (get new-shapes-map shape-path)
;; Get its related shape in the children of new-shape: the one that
;; has the same shape-path
related-shape-in-new (get new-shapes-map shape-path)
parents-of-swapped (if related-shape-in-new
(conj parent-of-swapped (:parent-id related-shape-in-new))
parent-of-swapped)
;; If there is a related shape, keep its data
changes
(if related-shape-in-new
(if swap-slot
;; If the orig-child-touched was swapped, keep it
(keep-swapped-item changes related-shape-in-new orig-child-touched
ldata page orig-ref-id)
;; If the orig-child-touched wasn't swapped, copy
;; the touched attributes into it
(cll/update-attrs-on-switch
changes related-shape-in-new orig-child-touched
new-shape original-shape orig-ref-shape container))
changes)]
[changes parents-of-swapped]))
[changes []]
orig-touched)]
[changes parents-of-swapped]))
parents-of-swapped (if related-shape-in-new
(conj parent-of-swapped (:parent-id related-shape-in-new))
parent-of-swapped)
;; If there is a related shape, keep its data
changes
(if related-shape-in-new
(if swap-slot
;; If the orig-child-touched was swapped, keep it
(keep-swapped-item changes related-shape-in-new orig-child-touched
ldata page orig-ref-id)
;; If the orig-child-touched wasn't swapped, copy
;; the touched attributes into it
(cll/update-attrs-on-switch
changes related-shape-in-new orig-child-touched
new-shape original-shape orig-ref-shape container))
changes)]
[changes parents-of-swapped]))
[changes []]
orig-touched)]
[changes parents-of-swapped])))

View File

@ -395,6 +395,13 @@
(->> (gsts/get-children-seq (:id root-copy) (:objects file-data))
(d/seek #(= (:shape-ref %) (:id main-shape)))))
;; Call-local volatile map for memoizing find-ref-shape within a single
;; variant-switch pass (or any other call-scoped context). Keyed on
;; [shape-id include-deleted? with-context?]. nil when not active; bind
;; to (volatile! {}) to enable. Never use a global binding — stale entries
;; across edits would silently corrupt override resolution.
(def ^:dynamic *find-ref-shape-cache* nil)
(defn find-ref-shape
"Locate the nearest component in the local file or libraries, and retrieve the shape
referenced by the instance shape."
@ -405,8 +412,16 @@
component (when (some? component-file)
(ctkl/get-component (:data component-file) (:component-id head-shape) include-deleted?))]
(when (some? component)
(get-ref-shape (:data component-file) component shape :with-context? with-context?))))]
(some find-ref-shape-in-head (ctn/get-parent-heads (:objects container) shape))))
(get-ref-shape (:data component-file) component shape :with-context? with-context?))))
compute (fn [] (some find-ref-shape-in-head (ctn/get-parent-heads (:objects container) shape)))]
(if-let [cache *find-ref-shape-cache*]
(let [k [(:id shape) include-deleted? with-context?]]
(if (contains? @cache k)
(get @cache k)
(let [result (compute)]
(vswap! cache assoc k result)
result)))
(compute))))
(defn find-near-match
"Locate the shape that occupies the same position in the near main component.

View File

@ -8,6 +8,7 @@
(:require
[app.common.features :as ffeat]
[app.common.files.changes :as ch]
[app.common.files.changes-builder :as pcb]
[app.common.schema :as sm]
[app.common.schema.generators :as sg]
[app.common.schema.test :as smt]
@ -861,3 +862,18 @@
(nil? (get-in result2 [:pages-index page-id :default-grids])))))
{:num 1000})))
(t/deftest concat-changes-is-eager-and-order-preserving
;; Regression test for the lazy concat bug: concat-changes used to build
;; :undo-changes as N nested lazy clojure.core/concat thunks when called in a
;; loop (as generate-sync-file and generate-sync-library do), causing a
;; StackOverflowError on large files when the seq was realized.
(let [n 500
singles (for [i (range n)]
{:redo-changes [{:type :mod-obj :i i}]
:undo-changes (list {:type :mod-obj :i i})})
merged (reduce pcb/concat-changes (pcb/empty-changes) singles)]
;; redo order: 0 1 2 … n-1 (applied in sequence)
(t/is (= (range n) (map :i (:redo-changes merged))))
;; undo order: n-1 … 1 0 (reverse, so undoing reverses redo application order)
(t/is (= (range (dec n) -1 -1) (map :i (:undo-changes merged))))))

View File

@ -18,6 +18,7 @@
[app.common.test-helpers.ids-map :as thi]
[app.common.test-helpers.shapes :as ths]
[app.common.types.component :as ctk]
[app.common.types.container :as ctn]
[app.common.types.shape-tree :as ctst]
[clojure.test :as t]))
@ -491,6 +492,56 @@
(t/is (= (:touched copy2-root') nil))
(t/is (= (:touched copy2-child') nil))))
(t/deftest test-inverse-sync-when-moving-shape
;; Exercises the `moved` callback in generate-sync-shape-inverse-recursive,
;; which uses the precomputed children-inst-index / children-main-index maps.
(let [;; ==== Setup
file (-> (thf/sample-file :file1)
(tho/add-component-with-many-children-and-copy :component1
:main-root
[:main-child1 :main-child2 :main-child3]
:copy-root
:copy-root-params {:children-labels [:copy-child1
:copy-child2
:copy-child3]}))
page (thf/current-page file)
main-child1 (ths/get-shape file :main-child1)
;; ==== Action: reorder main so child1 moves to the end → [child2, child3, child1]
;; Reordering the main (component root) is allowed; only copy structures are locked.
changes1 (cls/generate-relocate (-> (pcb/empty-changes nil)
(pcb/with-page-id (:id page))
(pcb/with-objects (:objects page)))
(thi/id :main-root)
2
#{(:id main-child1)})
updated-file (thf/apply-changes file changes1)
updated-page (thf/current-page updated-file)
copy-root (ths/get-shape updated-file :copy-root)
;; ==== Action: inverse sync — push instance child order back into the main component
changes2 (cll/generate-sync-shape-inverse (pcb/empty-changes)
updated-file
{(:id updated-file) updated-file}
updated-page
(:id copy-root))
file' (thf/apply-changes updated-file changes2)
;; ==== Get
main-root' (ths/get-shape file' :main-root)
main-child1' (ths/get-shape file' :main-child1)
main-child2' (ths/get-shape file' :main-child2)
main-child3' (ths/get-shape file' :main-child3)]
;; ==== Check: main children restored to [child1, child2, child3] — matching instance order
(t/is (some? main-root'))
(t/is (= (first (:shapes main-root')) (:id main-child1')))
(t/is (= (second (:shapes main-root')) (:id main-child2')))
(t/is (= (nth (:shapes main-root') 2) (:id main-child3')))))
(t/deftest test-no-sync-changes-when-only-position-changes
;; Regression: the library sync dialog was shown even when a library component
;; was only moved (x/y changed). Position changes are normalised by
@ -539,3 +590,72 @@
;; A position-only change in the main component must not propagate to copies
;; and therefore must produce no redo-changes.
(t/is (empty? (:redo-changes sync-changes)))))
(t/deftest test-inverse-sync-remap-changes-marks-page-changes-as-local
;; BUG-07: When doing inverse sync on an instance whose copy has a shape that
;; does not exist in the main component (only-inst), add-shape-to-main emits
;; :mod-obj changes that update :shape-ref on the existing instance shapes.
;; These target the local page, so they must be :local-change? true. Without
;; the fix, the per-node check-local walker missed child IDs and they were
;; routed to the remote library file instead.
;;
;; Also verifies that change-touched page changes are marked :local-change? true
;; (those were previously handled by the O(K²) check-local loop; now tagged at
;; creation time).
(let [;; ==== Setup: library with a minimal component (root only, no children)
library (-> (thf/sample-file :library)
(tho/add-frame :main-root)
(thc/make-component :component1 :main-root))
;; Local file: instantiate the component, then add an extra child that
;; has no counterpart in the main component (only-inst scenario)
file (-> (thf/sample-file :file)
(thc/instantiate-component :component1 :copy-root
:library library))
file (ths/add-sample-shape file :extra-child
:type :rect
:parent-label :copy-root)
page (thf/current-page file)
copy-root (ths/get-shape file :copy-root)
libraries {(:id library) library}
;; ==== Action: inverse sync — push instance changes back to the component
changes (cll/generate-sync-shape-inverse
(-> (pcb/empty-changes nil (:id page))
(pcb/with-page-id (:id page))
(pcb/with-objects (:objects page)))
(:data file)
libraries
(ctn/get-container (:data file) :page (:id page))
(:id copy-root))
all-redo (:redo-changes changes)
;; :mod-obj changes that set :shape-ref come from add-shape-to-main and
;; must be local (they update instance shapes on the page)
shape-ref-changes
(->> all-redo
(filter (fn [c]
(and (= (:type c) :mod-obj)
(some #(= (:attr %) :shape-ref) (:operations c))))))
;; :mod-obj :set-touched changes for the copy-root come from change-touched
;; on the page container and must also be local
touched-local-changes
(->> all-redo
(filter (fn [c]
(and (= (:type c) :mod-obj)
(= (:id c) (:id copy-root))
(some #(= (:type %) :set-touched) (:operations c))))))]
(t/is (seq shape-ref-changes)
"Expected at least one :mod-obj :shape-ref change from add-shape-to-main")
(t/is (every? :local-change? shape-ref-changes)
"All :shape-ref mod-obj changes must be :local-change? true")
(t/is (seq touched-local-changes)
"Expected at least one :set-touched change for copy-root on the page")
(t/is (every? :local-change? touched-local-changes)
"All page-side :set-touched changes must be :local-change? true")))

View File

@ -0,0 +1,131 @@
# BUG-01 — `concat-changes` accumulates `:undo-changes` as a nested lazy `concat`
- **Category:** Stability + Performance
- **Confidence:** High
- **Severity:** Medium (latent `StackOverflowError` on large files; wasted realization cost)
- **Hot path:** Yes — every library/component sync (`generate-sync-file` / `generate-sync-library`)
## Onboarding (fresh context — read first)
- Repo root for local file tools (Read/Edit/Write): `/home/alotor/kaleidos/penpot/penpot`
- Serena / REPL project root (devenv container): `/home/penpot/penpot`. Use this prefix for Serena symbol tools and the CLJS REPL; use the local prefix for Read/Edit/Write.
- Read first: `docs/component-subsystem-handoff.md` (subsystem map, esp. §3 "data model", §5.2 "direct sync", and sharp edge #11) and Serena memory `critical-info`.
- Relevant memories: `mem:common/changes-architecture`, `mem:common/component-data-model`.
- Line numbers drift — re-locate symbols by name.
## Summary
`pcb/concat-changes` merges two change-sets. The `:redo-changes` side was converted
to an eager vector concat (`d/concat-vec`), but the `:undo-changes` side still uses
lazy `clojure.core/concat`. Because `concat-changes` is called **once per container
in a loop** during a full sync, the resulting `:undo-changes` becomes a chain of N
nested lazy `concat` thunks. Realizing it (on undo, serialization, or `count`) costs
O(N) stack depth → real `StackOverflowError` risk on large files (files routinely
have hundredsthousands of components, and a library sync visits all of them).
## Affected code
`common/src/app/common/files/changes_builder.cljc``concat-changes` (~line 127):
```clojure
(defn concat-changes
[changes1 changes2]
(-> changes1
(update :redo-changes d/concat-vec (:redo-changes changes2)) ; eager (transient vector) ✓
(update :undo-changes #(concat (:undo-changes changes2) %)))) ; lazy clojure.core/concat ✗
```
Callers that loop (both carry a `;;TODO Remove concat changes` marker):
- `common/src/app/common/logic/libraries.cljc``generate-sync-file` (~line 514): loops `ctf/object-containers-seq` (pages + deleted components), `concat-changes` per container.
- `common/src/app/common/logic/libraries.cljc``generate-sync-library` (~line 557): loops `ctkl/components-seq`, `concat-changes` per component.
Other (non-loop) callers — lower risk, but verify any fix doesn't regress them:
`logic/variants.cljc` (~line 89), `logic/libraries.cljc` (~2354, ~2384, ~2457),
`files/repair.cljc` (~706).
## Root cause
`(concat a b)` returns a lazy seq. Nesting it N times — `(concat u_N (concat u_{N-1} (… u_1)))` — means traversal crosses N lazy boundaries; deep nesting overflows the stack when realized, and re-realization is non-trivial. `d/concat-vec` (`common/src/app/common/data.cljc` ~243) avoids this by building a transient vector eagerly, which is why redo is already safe.
## Constraints / why this is still a TODO
`:undo-changes` is built **elsewhere** with `(update :undo-changes conj …)` which relies on **list/prepend** semantics (newest-undo-first). You therefore cannot naively switch the accumulator to a vector — later `conj` would append instead of prepend and silently reverse undo order. The required merge order is: **`changes2`'s undos must come before `changes1`'s undos** (so undoing reverses redo application order).
## Proposed fix directions (pick one, preserve order + semantics)
1. **Eager, order-preserving, keep it a seq.** Replace the lazy `concat` with an eager build that yields the same order and the same collection type the rest of the code expects. Verify with a quick check of what `:undo-changes` is at the call sites and whether anything `conj`s onto the result of `concat-changes` afterwards.
2. **Restructure the loops.** Have `generate-sync-file` / `generate-sync-library` collect per-container change-sets into a vector and concat once at the end (single eager merge), instead of folding `concat-changes` each iteration. This removes the nesting entirely and reads cleaner.
Whatever the choice: keep `:redo-changes` and `:undo-changes` ordering identical to current behavior (undo is the reverse of redo).
## Reproduction / validation
- Build a large synthetic file (many pages and/or many deleted components, each holding component copies that need sync) and run a full `generate-sync-file`; force realization of `:undo-changes` (e.g. `count` or apply-undo). Pre-fix this should be slow / stack-overflow at high N; post-fix it should be linear and safe.
- A focused unit test that concats many small change-sets and asserts the merged `:undo-changes` realizes without overflow and in the correct order.
## Test guardrails
From `common/`:
```bash
clojure -M:dev:test --focus common-tests.logic.comp-sync-test
pnpm run test:quiet -- --focus common-tests.logic.comp-sync-test
```
Also run the full common suite once before committing, since `concat-changes` is shared:
```bash
clojure -M:dev:test
```
## Acceptance criteria
- `concat-changes` no longer produces nested lazy seqs for `:undo-changes`.
- Undo/redo ordering is unchanged (verified by existing comp-sync tests).
- The two `;;TODO Remove concat changes` markers are resolved or the TODO removed.
- No regression across the full common test suite.
## Solution applied
**Approach chosen:** Fix direction 1 — eager, order-preserving, keep it a seq.
### Change in `concat-changes` (`changes_builder.cljc`)
Replaced the lazy `clojure.core/concat` with an eager `into` + `reverse`:
```clojure
;; before — lazy, N nested thunks after N loop iterations:
(update :undo-changes #(concat (:undo-changes changes2) %))
;; after — eager, O(1) stack depth per call:
(update :undo-changes into (reverse (:undo-changes changes2)))
```
**Why `into` + `reverse` preserves order:**
`:undo-changes` is a list. `conj` on a list prepends. `into coll items` reduces with `conj`, so each item in `items` is prepended in sequence — meaning the *last* item of `items` ends up first in the result. To get `changes2`'s undos before `changes1`'s undos (the required order), we reverse `changes2`'s undo list first so that `into` re-reverses it back into the correct order:
```
changes1-undo = (u1a u1b) ; u1a is newest
changes2-undo = (u2a u2b) ; u2a is newest
reverse(changes2-undo) = (u2b u2a)
into (u1a u1b) (u2b u2a)
→ conj u2b → (u2b u1a u1b)
→ conj u2a → (u2a u2b u1a u1b) ; ✓ changes2 undos first, order preserved
```
Because `generate-sync-container` always produces a fresh `(pcb/empty-changes nil)` result (never a nested lazy seq), `(:undo-changes changes2)` is always a concrete list — `reverse` on it is O(M) where M is the number of changes in that single container. The total work across the loop is O(total changes), same as before but now stack depth is O(1) per call instead of O(N).
The collection type remains a list, so all existing `conj`-based callers that rely on prepend semantics continue to work unchanged.
### Cleanup in `libraries.cljc`
Removed the two `;;TODO Remove concat changes` markers from `generate-sync-file` and `generate-sync-library` — the root cause is fixed in `concat-changes` itself, so the TODO no longer applies.
### Test added (`files_changes_test.cljc`)
`concat-changes-is-eager-and-order-preserving`: folds 500 single-op change-sets via `reduce pcb/concat-changes` (mirroring the sync loop pattern) and asserts:
- `:redo-changes` indices are 0…499 (insertion order)
- `:undo-changes` indices are 499…0 (reverse, so undoing reverses redo order)
- No `StackOverflowError` is thrown at N=500

View File

@ -0,0 +1,113 @@
# BUG-02 — Un-memoized ref-chain walking during variant switch
- **Category:** Performance
- **Confidence:** Medium (profile before committing to an optimization)
- **Severity:** LowMedium
- **Hot path:** Yes — variant switch / `keep-touched?` swap (`clv/generate-keep-touched`)
## Onboarding (fresh context — read first)
- Repo root for local file tools (Read/Edit/Write): `/home/alotor/kaleidos/penpot/penpot`
- Serena / REPL project root (devenv container): `/home/penpot/penpot`.
- Read first: `docs/component-subsystem-handoff.md` (§5.5 swap/variant switch, §6 `update-attrs-on-switch`, §8 variants, sharp edges #6) and Serena memory `critical-info`.
- Relevant memories: `mem:common/component-swap-pipeline`, `mem:common/component-data-model`.
- Line numbers drift — re-locate symbols by name.
## Summary
`generate-keep-touched` (the variant-switch override-preservation step) walks the
`:shape-ref` chain repeatedly, once per touched child, with no caching. Each walk
re-resolves the component + component-file and re-derives parent sets. For variants
with many children and/or deep nesting this is roughly O(N · D · cost(find-ref-shape))
with repeated identical component lookups.
This is a **profile-first** item: confirm it's a measurable cost on a realistic large
variant before optimizing, since the walkers are correctness-sensitive.
## Affected code
`common/src/app/common/logic/variants.cljc`:
- `generate-keep-touched` (~line 146): for each touched original child it calls
- `add-touched-from-ref-chain` (~line 139) → `ctf/get-touched-from-ref-chain-until-target-ref` (walks the whole chain), and
- `find-shape-ref-child-of` (~line 122) → **recursive** `ctf/find-ref-shape`.
`common/src/app/common/types/file.cljc`:
- `find-ref-shape` (~line 397): for each call walks `ctn/get-parent-heads` and, per head, `find-component-file` + `ctkl/get-component` + `get-ref-shape`.
- `get-ref-chain-until-target-ref` (~line 1228): loops `find-ref-shape`.
- `find-remote-shape` (~line 457): recursive; re-derives `get-component-shape` / `get-in libraries` / `get-component` / `get-ref-shape` at each level.
## Proposed fix directions (after profiling)
- **Scoped memo** within a single `generate-keep-touched` call: memoize component/file
resolution keyed on `(:component-file head, :component-id head)`, and/or memoize
`find-ref-shape` per `(container-id, shape-id)`. Scope the memo to the call so stale
entries can't leak across switches (cf. how `validate.cljc` scopes
`*ref-shape-cache*` per page — a good precedent for a safe, page/call-local cache).
- **Precompute** the ref map for the involved subtrees once, before the reduce over
`orig-touched`, instead of resolving per child.
## Risks / things to watch
- Ref resolution can cross files (remote libraries) and involves swap slots / fostered children; a cache key must capture everything that affects resolution. Get this wrong and you return the wrong ref shape → corrupted overrides on switch.
- Prefer a call-local (dynamically-bound or passed) cache over any global memoization to avoid staleness across edits.
- This is lower priority than BUG-06/BUG-01/BUG-07; only pursue if profiling shows it matters.
## Validation
- Profile `generate-keep-touched` on a large variant (many children, nested instances) before/after — e.g. via `tho/swap-component-in-shape {:keep-touched? true}` in a common test, or live with the `update-attrs-on-switch` tracing recipe in `mem:common/component-debugging-recipes`.
- Assert switch output is byte-identical before/after the memo.
## Test guardrails
From `common/`:
```bash
clojure -M:dev:test --focus common-tests.logic.variants-switch-test
pnpm run test:quiet -- --focus common-tests.logic.variants-switch-test
```
`common/test/common_tests/logic/variants_switch_test.cljc` is the canonical swap+touched suite. Run the full common suite before committing.
## Acceptance criteria
- Profiling demonstrates a real reduction in redundant ref-chain resolution on a large variant.
- Variant-switch output (changes + resulting touched/overrides) is identical to pre-change behavior.
- Any cache is call/page-scoped, never global.
## Resolution
**Status:** Fixed — `perf: call-scoped memoization of find-ref-shape in generate-keep-touched (BUG-02)`
### Approach
Added a call-local dynamic var `*find-ref-shape-cache*` (a `volatile!` map, nil by default) to
`app.common.types.file/find-ref-shape`. When bound, every call is memoized by
`[shape-id include-deleted? with-context?]`, using `contains?` for cache-miss detection (avoids
the CLJS keyword interning issue documented in `validate.cljc`).
`generate-keep-touched` in `app.common.logic.variants` wraps its body with:
```clojure
(binding [ctf/*find-ref-shape-cache* (volatile! {})] ...)
```
This activates the cache for the entire switch call — including the inner loops of
`get-ref-chain-until-target-ref` (called via `add-touched-from-ref-chain`) and the recursive
`find-shape-ref-child-of` — reducing redundant ref-chain resolution from O(N·D·cost) to O(1)
after the first resolution per shape.
The cache is strictly call-scoped: a fresh `volatile!` is created at the start of each
`generate-keep-touched` invocation, so stale entries can never leak across variant switches.
### Files changed
- `common/src/app/common/types/file.cljc`: added `*find-ref-shape-cache*` dynamic var; modified
`find-ref-shape` to check and populate it when bound.
- `common/src/app/common/logic/variants.cljc`: bind the cache in `generate-keep-touched`.
### Validation
Full common test suite: **1055 tests, 24374 assertions, 0 failures.**
Focused variant-switch suite: **47 tests, 292 assertions, 0 failures.**

View File

@ -0,0 +1,107 @@
# BUG-03 — `compare-children` fallback is O(n²) with an expensive constant
- **Category:** Performance
- **Confidence:** Medium (profile before committing)
- **Severity:** LowMedium
- **Hot path:** Yes — both sync directions, for any instance whose children are reordered/inserted/removed
## Onboarding (fresh context — read first)
- Repo root for local file tools (Read/Edit/Write): `/home/alotor/kaleidos/penpot/penpot`
- Serena / REPL project root (devenv container): `/home/penpot/penpot`.
- Read first: `docs/component-subsystem-handoff.md` (§5.2 direct sync, sharp edge #8 swap slots) and Serena memory `critical-info`.
- Relevant memories: `mem:common/component-data-model`, `mem:common/component-swap-pipeline`.
- Line numbers drift — re-locate symbols by name.
## Summary
`compare-children` matches an instance's children against the main's children to
decide add/remove/move/sync. The aligned case is a clean O(n) zipper, but any
misalignment (reorder/insert/remove) drops into a `d/seek` fallback that linearly
scans **both** child lists *and* calls `ctf/match-swap-slot?` (a cross-file
ref-chain lookup) inside each predicate, then rebuilds a list with `(remove …)`. Net
O(n²) comparisons, each potentially doing file-crossing work; `match-swap-slot?` is
also recomputed for the same pair multiple times.
This is a **profile-first** item — the matching algorithm is correctness-critical, so
only optimize if profiling shows it's a real cost.
## Affected code
`common/src/app/common/logic/libraries.cljc``compare-children` (~line 1182).
Observations:
- First branch tests `ctk/is-main-of?` and `ctf/match-swap-slot?` on the head pair.
- On mismatch it does two `d/seek`s over `children-inst` and `children-main`, each
predicate re-invoking `match-swap-slot?`.
- Then `(remove #(= (:id %) …) children-inst|main)` rebuilds the list each step → the
structural O(n²).
`ctf/match-swap-slot?` lives in `common/src/app/common/types/file.cljc` (cross-file
ref-chain / swap-slot resolution — relatively expensive).
## Proposed fix directions (after profiling)
- **Precompute matching indexes** for the level: maps from `is-main-of?` key and from
swap-slot to the candidate inst/main child, so each match is a hash lookup instead
of a `d/seek` + linear `remove`. Track "consumed" children with a set of ids rather
than rebuilding the list with `remove`.
- **Memoize `match-swap-slot?`** per `(child-main-id, child-inst-id)` within the call
to eliminate the repeated recomputation in the head test + both seeks.
## Risks / things to watch
- This is the core child-matching algorithm for sync in **both** directions (it's
passed `inverse?` and `reset?` and a set of callbacks). A subtle change in match
order or in which child is "consumed" changes add/remove/move output. Treat as
higher-risk; keep the match semantics identical.
- Swap slots are what keep swapped sub-instances syncing correctly (sharp edge #8) —
`match-swap-slot?` is guarded by `(not reset?)` in the current code; preserve that.
- Only pursue after BUG-06 (the cheap, safe `index-of` win in the same file).
## Validation
- Profile a sync where a large instance has many children reordered/inserted.
- Assert byte-identical change output before/after across the full comp-sync and variants-switch suites.
## Test guardrails
From `common/`:
```bash
clojure -M:dev:test --focus common-tests.logic.comp-sync-test
clojure -M:dev:test --focus common-tests.logic.variants-switch-test
clojure -M:dev:test --focus common-tests.logic.copying-and-duplicating-test
```
Run the full common suite before committing — this function underpins a lot.
## Acceptance criteria
- Profiling shows reduced cost on a reordered large instance.
- Add/remove/move/sync output is identical to pre-change behavior across the sync suites.
- `match-swap-slot?` is not recomputed for the same pair within a single match step.
## Fix applied
**Commit:** on branch `alotor-component-polishing`
**File:** `common/src/app/common/logic/libraries.cljc``compare-children`
Three changes, all confined to `compare-children`:
1. **Precomputed slot maps**`inst-slots` and `main-slots` are built once before the
loop by calling `ctf/find-swap-slot` for each child (O(n) total). When `reset?` is
true the maps stay `nil`, preserving the existing swap-slot guard.
2. **Local `match-pair?` predicate** — replaces every `ctk/is-main-of?` +
`ctf/match-swap-slot?` pair in the `d/seek` predicates with a direct map lookup,
eliminating repeated cross-file ref-chain traversals for the same pair.
3. **Consumed-set instead of `(remove …)`** — two sets `consumed-inst` /
`consumed-main` track out-of-order matched IDs. A `drop-while` at the head of each
loop iteration skips consumed items; seeks exclude them via `(not (consumed-… (:id %)))`;
the nil-case reductions filter them with `remove`. This replaces the O(n) list rebuild
that occurred on every misaligned step.
All three comp-sync / variants-switch / copying-and-duplicating test suites pass
(68 + 292 + 7 assertions, 0 failures).

View File

@ -0,0 +1,156 @@
# BUG-04 — `update-attrs-on-switch` composite-geometry guard audit
- **Category:** Correctness (audit — no confirmed defect yet)
- **Confidence:** Low (this is an investigation, not a known bug)
- **Severity:** Variable (this function is the documented epicenter of "swap moved/resized my shape" bugs)
- **Hot path:** Yes — every `keep-touched?` swap / variant switch
## Onboarding (fresh context — read first)
- Repo root for local file tools (Read/Edit/Write): `/home/alotor/kaleidos/penpot/penpot`
- Serena / REPL project root (devenv container): `/home/penpot/penpot`.
- Read first: `docs/component-subsystem-handoff.md` (§6 `update-attrs-on-switch`, §11 sharp edges #4 and #5) and Serena memory `critical-info`.
- Relevant memories: `mem:common/component-swap-pipeline`, `mem:common/component-debugging-recipes` (has a live tracing recipe for this exact function), `mem:common/decimals-and-coordinates`.
- Line numbers drift — re-locate symbols by name.
## Goal
This is an **audit task**, not a pre-diagnosed fix. `update-attrs-on-switch` decides
which touched attributes of the pre-swap shape get carried onto the freshly
instantiated target. Its guard set around composite geometry (`:selrect`, `:points`)
is the most frequent source of swap regressions. The task is to systematically
probe the guards for cases where they either (a) carry stale/incompatible geometry
onto the target, or (b) wrongly skip an override the user expects to keep.
## Affected code
`common/src/app/common/logic/libraries.cljc`:
- `update-attrs-on-switch` (~line 2134) — the function under audit. Note the long
inline comment block describing the composite-geometry skip keyed on
`:layout-item-h-sizing` / `:layout-item-v-sizing` = `:fix` and width/height
divergence.
- Supporting guards/helpers in the same file:
- `equal-geometry?` (~line 2093) — positional-displacement-tolerant equality (uses `mth/close?` / `gpt/close?`).
- `reposition-shape` (~line 2459) — repositions `previous-shape` by (dest-root origin-root).
- `switch-fixed-layout-geom-change-value`, `switch-path-change-value`, `switch-text-change-value` — type-specific conversions.
## Areas to probe (from the handoff sharp edges)
1. **Non-`:fix` sizing + composite geometry** (sharp edge #4): width/height are skipped
correctly when sizing isn't `:fix`, but verify `:selrect`/`:points` cannot still
carry old override dimensions through the `:else` fallback. The current code has a
dedicated skip for this — confirm it covers path and text shapes too.
2. **Repositioned `previous-shape`** (sharp edge #5): `reposition-shape` applies
(dest-root origin-root). For normal variant switch this delta is often zero, but
**do not assume zero** for other swap entry points — notably the Plugin API
`switchVariant` path and `component-multi-swap`. Probe a swap where the pre-swap
shape is not at the target root position.
3. **Inherited touched via ref chain** (sharp edge #6): `add-touched-from-ref-chain`
(in `variants.cljc`) makes a locally-untouched shape behave as touched. Check the
*effective* touched set drives the right copy decisions.
4. **Master mismatch guard**: the rule "if origin and destiny masters disagree on an
attr, don't copy (except `:points`/`:selrect`/`:content`)" — verify the exceptions
don't leak incompatible composite geometry between differently-shaped variants.
5. **Text `:content`** partial-touched handling (`switch-text-change-value`) and the
forced `:position-data` reset — verify formatting-only vs text-only overrides each
survive a switch correctly.
## How to investigate
- Use the live tracing recipe in `mem:common/component-debugging-recipes` to capture
`curr` / `prev` / `origin-ref` during a real UI or Plugin-API swap.
- In common tests, drive the production swap path with
`tho/swap-component-in-shape {:keep-touched? true}` and dump shape trees with
`thf/dump-file`. Use `thv/add-variant-with-copy` to build variants whose children
are component instances (the alt-drag-duplicate sub-pixel-drift scenario referenced
in `equal-geometry?`).
## Deliverable
- Either: a concrete failing scenario + a targeted guard fix + regression test, or
- a written conclusion that the guards hold for the probed cases, with the new tests
added to lock in the behavior.
## Test guardrails
From `common/`:
```bash
clojure -M:dev:test --focus common-tests.logic.variants-switch-test
clojure -M:dev:test --focus common-tests.logic.text-sync-test
pnpm run test:quiet -- --focus common-tests.logic.variants-switch-test
```
Canonical suite: `common/test/common_tests/logic/variants_switch_test.cljc`. Read neighbouring tests before adding cases.
## Acceptance criteria
- Each probed area has either a fix+test or an explicit "holds, with test" outcome.
- No regression in the variants-switch and text-sync suites.
---
## Resolution
**Status: Closed — guards hold for all probed areas. Two bugs were found and already fixed (in `develop` prior to this branch). Regression tests are in place and passing (47 tests, 0 failures).**
### Findings by area
#### Area 1 — Non-`:fix` sizing + composite geometry
The non-`:fix` composite-geometry skip (lines ~22772282 of `libraries.cljc`) correctly prevents `:selrect` and `:points` from being copied when the two variant masters have different dimensions and neither `layout-item-h-sizing` nor `layout-item-v-sizing` is `:fix`.
- **Path shapes**: The `path-change?` flag routes them through `switch-path-change-value` instead. A prior bug existed here (see Area 1-path below).
- **Text shapes (auto)**: Caught earlier by the `text-auto?` guard.
- **Text shapes (fixed) and rect/frame shapes**: Fall through to the identical non-`:fix` skip. The skip code does not branch on shape type, so the rect-based selrect consistency tests (`test-switch-selrect-consistent-*`) cover this path exhaustively.
**Outcome: holds.**
#### Area 1-path — Path content leaking stale position via `switch-path-change-value`
*Concrete bug found and already fixed.*
`equal-geometry?` originally lacked a handler for the `:content` attribute of path shapes. When a path was *repositioned* (not resized) inside a copy — touching `:geometry-group``equal-geometry?` returned `false` for `:content`, causing `switch-path-change-value` to fire and embed the old absolute path coordinates into the new variant. The child appeared at the pre-switch position instead of the target master's default position.
**Fix**: Added a `:content` branch to `equal-geometry?` that compares the bounding-box width/height of the path segments with `mth/close?` tolerance. When only position changes (bounding-box size matches), `equal-geometry?` returns `true` and `:content` is correctly skipped.
**Regression test**: `test-switch-does-not-override-path-content-when-only-repositioned` (already in suite, passes).
#### Area 2 — Repositioned `previous-shape` / sub-pixel drift
*Concrete bug found and already fixed.*
`equal-geometry?` originally used exact equality for `:selrect` comparison. When `previous-shape` carried sub-pixel floating-point drift in its width (from interactive transform modifiers, e.g. an alt-drag duplicate of a variant whose children are component instances), `equal-geometry?` returned `false` even for effectively equal shapes. The `:else` branch then copied `previous-shape.selrect` verbatim onto the fresh target — including a stale `:selrect.y` — leaving the child at the source variant's layout position inside the new variant's frame.
For normal variant switches the `reposition-shape` delta is zero (both roots on the same page position), but the drift-induced `equal-geometry?` failure still surfaced through the non-`:fix` skip: because the masters' dimensions agreed exactly (`origin-ref-shape.width == current-shape.width`), the non-`:fix` skip did not catch it, and the stale selrect was copied.
**Fix**: `equal-geometry?` now uses `mth/close?` (and `gpt/close?` for `:points`) instead of exact `=`. This matches how drift is already tolerated elsewhere in the geometry layer.
**Regression test**: `test-switch-when-source-master-child-has-touched-geometry` (already in suite, passes). The test positions child1 at y=101 inside m01 and child2 at y=73 inside m02, introduces sub-pixel width drift on the copy's child, switches, and asserts that `:y` and `:selrect.y` both land at the target master's layout position.
#### Area 3 — Inherited touched via ref chain
The effective touched set used by `update-attrs-on-switch` comes from `(get previous-shape :touched #{})`, where `previous-shape` has already been augmented by `add-touched-from-ref-chain` in `generate-keep-touched` before the call. This correctly implements the "effective touched set drives copy decisions" requirement.
**Outcome: holds.** Covered by `test-switch-variant-without-touched-but-touched-parent`.
#### Area 4 — Master mismatch guard for composite geometry
`:selrect`, `:points`, and `:content` intentionally bypass the master-mismatch guard (line ~2235) because they can legitimately differ between variants. The alternative guard — the non-`:fix` composite-geometry skip (Area 1) — closes the gap: when variant masters disagree on dimensions, `:selrect`/`:points` are skipped regardless of the touched-set state.
The `not=` comparison in the non-`:fix` skip (comparing `origin-ref-shape` vs `current-shape` dimensions) uses exact equality. This is safe: both shapes come directly from component master data and carry integer pixel dimensions; the sub-pixel drift concern applies only to `previous-shape` (the user's copy), which is handled in `equal-geometry?` by `mth/close?`.
**Outcome: holds.** Covered by `test-switch-selrect-consistent-*` and `test-switch-with-*-sizing-*` tests.
#### Area 5 — Text `:content` partial-touched handling
`switch-text-change-value` correctly handles all four partial-touch combinations (text-only, attrs-only, both, neither) across identical and differing variant content. The `reset-pos-data?` path correctly nils `:position-data` when `:geometry-group` is touched and position-data differs, allowing the layout engine to recalculate it.
**Outcome: holds.** Covered by the text-override test matrix (identical, different-prop, different-text, different-text-and-prop variants, with and without structure changes).
### Test suite result
```
47 tests, 292 assertions, 0 failures
```

View File

@ -0,0 +1,91 @@
# BUG-05 — Silent no-op vs. bug ambiguity in direct sync
- **Category:** Diagnosability (no behavior change intended)
- **Confidence:** Low
- **Severity:** Low
- **Hot path:** Yes — direct sync (`generate-sync-shape-direct`)
## Onboarding (fresh context — read first)
- Repo root for local file tools (Read/Edit/Write): `/home/alotor/kaleidos/penpot/penpot`
- Serena / REPL project root (devenv container): `/home/penpot/penpot`.
- Read first: `docs/component-subsystem-handoff.md` (§5.2 direct sync, §11 sharp edge #7 cross-file ref chains) and Serena memory `critical-info`.
- Relevant memories: `mem:common/component-data-model`, `mem:common/changes-architecture`.
- Line numbers drift — re-locate symbols by name.
## Summary
When a copy's main can't be resolved — e.g. a remote library is unlinked or the ref
is lost — `generate-sync-shape-direct` returns `changes` unchanged (a silent no-op).
There is currently nothing that distinguishes an **intentional** no-op ("library is
legitimately unlinked") from a **bug** ("we lost the ref chain and silently stopped
syncing"). Field reports of "my copy stopped updating from main" are hard to
diagnose because the failure leaves no trace.
This is a small **diagnosability** improvement, not a behavior change: add a
debug/trace-level log on the not-found path so the condition is observable.
## Affected code
`common/src/app/common/logic/libraries.cljc``generate-sync-shape-direct`
(~line 796). Trace the path where the resolved main/ref-shape is `nil` and the
function returns `changes` without generating sync ops.
Resolution helpers involved (for context): `ctf/get-ref-shape` (normal sync) /
`ctf/find-ref-shape` (reset path) in `common/src/app/common/types/file.cljc`.
There is already a logging facility in this namespace (`shape-log`,
`container-log`, with `log-shape-ids` / `log-container-ids` toggles near the top of
the file) — reuse it rather than introducing a new mechanism.
## Proposed change
- On the "main/ref not found → no-op" branch, emit a `:debug` (or `:trace`) log via the
existing `shape-log`, including: the shape id, container id, the component-id /
component-file it tried to resolve, and whether the library is linked. Keep it at a
level that is silent by default.
- Optionally add a coarse-grained signal that callers/telemetry could use to
distinguish "skipped: library unlinked" from "skipped: ref not found", but do **not**
change the returned changes or the sync outcome.
## Risks / things to watch
- Must be a pure observability change — identical change output, no perf regression
(don't add expensive lookups just to log; reuse values already computed on that path).
- Respect the existing log-level gating so normal runs stay quiet.
## Validation
- Construct a copy whose remote library is unlinked and one with a genuinely lost ref;
confirm both still no-op (unchanged) but now log distinctly at debug level.
- Confirm no change in comp-sync test output (logging is side-channel).
## Test guardrails
From `common/`:
```bash
clojure -M:dev:test --focus common-tests.logic.comp-sync-test
pnpm run test:quiet -- --focus common-tests.logic.comp-sync-test
```
## Acceptance criteria
- The not-found / unlinked direct-sync no-op is observable via the existing logging facility at a default-silent level.
- No change to sync behavior or change output; comp-sync suite passes unchanged.
## Fix summary
**File:** `common/src/app/common/logic/libraries.cljc`
Two `:debug`-level log calls were added via the existing `shape-log` macro — no new logging mechanism introduced.
### Change 1 — `generate-sync-shape-direct`: component not found (library unlinked / master deleted)
In the inner `else` branch of `(if component ...)` (previously a bare `changes` return), a `do` form now emits a `shape-log :debug` before returning `changes`. The log includes: `shape-id`, `component-id`, `component-file`, and `library-linked?` (derived from whether `library` is non-nil — a value already computed on this path). This distinguishes "library was unlinked" (`library-linked? false`) from "library present but component deleted" (`library-linked? true`).
### Change 2 — `generate-sync-shape-direct-recursive`: ref-shape not found
The `(if (nil? shape-main) changes ...)` guard (which already carried the comment "This should not occur, but protect against it in any case") was expanded into a `do` form that logs at `:debug` before returning `changes`. The log includes: `shape-id`, `component-id`, and `component-name`. This covers the case where the component is present but `get-ref-shape` / `find-ref-shape` failed to resolve the main shape.
Both changes are pure observability additions: identical change output, no new computations on the hot path, and gated by the existing `enabled-shape?` filter so normal runs remain silent.

View File

@ -0,0 +1,105 @@
# BUG-06 — `d/index-of` linear scan inside per-child sync callbacks
- **Category:** Performance
- **Confidence:** High
- **Severity:** Medium
- **Hot path:** Yes — both directions of component sync, per child of every synced instance
## Onboarding (fresh context — read first)
- Repo root for local file tools (Read/Edit/Write): `/home/alotor/kaleidos/penpot/penpot`
- Serena / REPL project root (devenv container): `/home/penpot/penpot`.
- Read first: `docs/component-subsystem-handoff.md` (§5.2 direct sync, §5.3 inverse sync, §6 attribute-sync) and Serena memory `critical-info`.
- Relevant memories: `mem:common/component-data-model`, `mem:common/changes-architecture`.
- Line numbers drift — re-locate symbols by name.
## Summary
The direct and inverse recursive sync functions compute child indices with
`d/index-of`, which is a **linear scan**, inside callbacks that `compare-children`
invokes **once per child**. For an instance level with n children that undergoes m
additions/moves, this is O(m·n) → O(n²) on a full reorder, with children of large
boards/instances being the realistic hotspot. The fix is a pure index-lookup
precompute — no behavior change.
## Affected code
`common/src/app/common/logic/libraries.cljc`:
- `generate-sync-shape-direct-recursive` (~line 854):
- `only-main` callback → `(d/index-of children-main child-main)` (~line 924)
- `moved` callback → `(d/index-of children-inst child-inst)` + `(d/index-of children-main child-main)` (~lines 968969)
- `generate-sync-shape-inverse-recursive` (~line 1045):
- `only-inst` callback → `(d/index-of children-inst child-inst)` (~line 1106)
- `moved` callback → `(d/index-of children-main …)` + `(d/index-of children-inst …)` (~lines 11451146)
`d/index-of` (`common/src/app/common/data.cljc` ~line 303) delegates to
`index-of-pred`, a linear scan.
Both functions already bind `children-inst` / `children-main` as vectors at the top
of their `let`, so the index source is stable for the duration of the callbacks.
## Proposed fix
Precompute an `{id → index}` map once per level, next to where `children-inst` /
`children-main` are bound, and look up by `:id` in the callbacks:
```clojure
children-inst (vec (ctn/get-direct-children container shape-inst))
children-main (vec (ctn/get-direct-children component-container shape-main))
children-inst-index (into {} (map-indexed (fn [i s] [(:id s) i])) children-inst)
children-main-index (into {} (map-indexed (fn [i s] [(:id s) i])) children-main)
```
then `(d/index-of children-main child-main)``(get children-main-index (:id child-main))`, etc. Apply the same in the inverse function.
## Risks / things to watch
- Very low risk: this is a value-preserving substitution (`index-of` returns the same int the map lookup returns), with no change to ordering or change generation.
- Confirm there are no duplicate shape ids within a single children vector (there should not be — they are sibling shapes); a map is correct under the invariant that ids are unique per level.
- Keep the precompute inside the `let` of each recursive call so it reflects the children of that specific level.
## Test guardrails
From `common/`:
```bash
clojure -M:dev:test --focus common-tests.logic.comp-sync-test
pnpm run test:quiet -- --focus common-tests.logic.comp-sync-test
```
Exercise reorder/add/remove cases specifically (the `moved` / `only-main` / `only-inst` branches). Run the full common suite before committing.
## Acceptance criteria
- No `d/index-of` calls remain inside the per-child sync callbacks.
- comp-sync tests pass unchanged (identical change output).
- Reordering many children of a synced instance is no longer quadratic.
## Fix
Applied in `common/src/app/common/logic/libraries.cljc`.
### `generate-sync-shape-direct-recursive`
Added two index maps immediately after the `children-inst` / `children-main` bindings:
```clojure
children-inst-index (into {} (map-indexed (fn [i s] [(:id s) i])) children-inst)
children-main-index (into {} (map-indexed (fn [i s] [(:id s) i])) children-main)
```
Replaced `d/index-of` calls:
- `only-main` callback: `(d/index-of children-main child-main)``(get children-main-index (:id child-main))`
- `moved` callback: both `(d/index-of children-inst child-inst)` and `(d/index-of children-main child-main)` replaced with their map equivalents.
### `generate-sync-shape-inverse-recursive`
Same pattern applied:
- Added `children-inst-index` and `children-main-index` after the `children-inst` / `children-main` bindings.
- `only-inst` callback: `(d/index-of children-inst child-inst)``(get children-inst-index (:id child-inst))`
- `moved` callback: both index-of calls replaced with map lookups.
### Test added
`test-inverse-sync-when-moving-shape` in `common/test/common_tests/logic/comp_sync_test.cljc` covers the inverse-sync `moved` branch (previously untested). It reorders the main component's children, runs inverse sync on the copy, and asserts the main is restored to match the instance's order.

View File

@ -0,0 +1,176 @@
# BUG-07 — Inverse sync re-maps the entire change list at every tree node
- **Category:** Performance + Stability
- **Confidence:** High
- **Severity:** Medium
- **Hot path:** Yes — "Update main" / inverse sync (`dwl/update-component`)
## Onboarding (fresh context — read first)
- Repo root for local file tools (Read/Edit/Write): `/home/alotor/kaleidos/penpot/penpot`
- Serena / REPL project root (devenv container): `/home/penpot/penpot`.
- Read first: `docs/component-subsystem-handoff.md` (§5.3 inverse sync) and Serena memory `critical-info`.
- Relevant memories: `mem:common/changes-architecture`, `mem:common/component-data-model`.
- Related: this shares the lazy-`concat` hazard family with **BUG-01**.
- Line numbers drift — re-locate symbols by name.
## Summary
`generate-sync-shape-inverse-recursive` ends each invocation by re-mapping the
**entire accumulated change set** to tag local-file changes. Because the function
recurses once per shape in the component tree (threading a single `changes`
accumulator), each return re-walks all changes produced so far. For a tree of K
shapes producing ~O(K) changes, total work is O(K²). Worse, the `:undo-changes`
re-map uses lazy `map` (not `mapv`), nesting K lazy seqs — the same
`StackOverflowError` / re-realization hazard as BUG-01, on the "Update main" path.
## Affected code
`common/src/app/common/logic/libraries.cljc``generate-sync-shape-inverse-recursive`
(~line 1045), tail of the function:
```clojure
check-local (fn [change]
(cond-> change
(= (:id change) (:id shape-inst))
(assoc :local-change? true)))
;; ...
(-> changes
(update :redo-changes (partial mapv check-local)) ; re-walks ALL accumulated redo changes
(update :undo-changes (partial map check-local))) ; lazy map → nests per recursion level
```
The recursion happens via the `both` callback (~line 1126 in that function), which
calls `generate-sync-shape-inverse-recursive` again with the same `changes`.
## Root cause
`check-local` only ever tags changes whose `:id` equals *this* invocation's
`shape-inst`. Applying it to the whole accumulated list at every node is redundant:
a change for shape X is correctly tagged only by the recursion level whose
`shape-inst` is X, but every level pays to scan the full list. The lazy `map` on
`:undo-changes` additionally nests across levels.
Context for *why* the tagging exists: an inverse sync may run on a component that
lives in a **remote library**, so the produced changes mix local-file and
remote-file changes; `:local-change?` marks the ones that belong to the local file.
Any fix must keep that tag landing on exactly the same set of changes.
## Proposed fix directions
1. **Tag only the delta.** Each invocation knows which changes it just added (it can
diff against the incoming `changes`, or build its own changes first then tag and
merge). Apply `check-local` only to the newly-added changes for `shape-inst`, then
append. This removes the full re-walk.
2. **Single top-level pass.** Do the recursion without per-node tagging, then run one
`mapv check-local'` over the full result at the top-level entry
(`generate-sync-shape-inverse`, ~line 1010) where the complete set of synced
shape ids is known (tag changes whose `:id` is in that set). Cleanest if the set of
relevant shape ids is easy to collect during recursion.
In all cases: use `mapv` (eager) for **both** `:redo-changes` and `:undo-changes`.
## Risks / things to watch
- The `:local-change?` flag must end up on exactly the same changes as today. Verify by comparing the tagged-id set before/after on a remote-library inverse-sync scenario.
- Inverse sync also marks the component modified and renames on `:name-group` touched — don't disturb those.
- Confirm undo/redo ordering is preserved.
## Reproduction / validation
- Inverse-sync ("Update main") a deep/wide copy and confirm output changes are identical pre/post fix (same ops, same `:local-change?` tags), but the change list is built in a single pass.
- For the stability angle, a large tree should no longer build deeply nested lazy undo seqs.
## Test guardrails
From `common/`:
```bash
clojure -M:dev:test --focus common-tests.logic.comp-sync-test
pnpm run test:quiet -- --focus common-tests.logic.comp-sync-test
```
Inverse-sync cases live in `common/test/common_tests/logic/comp_sync_test.cljc`.
There is a frontend counterpart worth running too:
```bash
# from frontend/
pnpm run test:quiet -- --focus frontend-tests.logic.components-and-tokens
```
Run the full common suite before committing.
## Acceptance criteria
- The whole accumulated change set is no longer re-mapped at every recursion node.
- Both `:redo-changes` and `:undo-changes` are tagged eagerly (`mapv`).
- `:local-change?` lands on exactly the same changes as before (verified on a remote-library inverse-sync case).
- comp-sync tests pass unchanged.
---
## Fix
**Approach chosen:** tag at creation time — option 1 taken to its logical extreme.
Analysis showed that `check-local` marked exactly two kinds of changes at each
recursion level: the `:mod-obj :set-touched` change from `change-touched shape-inst
container` and the `:mod-obj :set-remote-synced` change from `change-remote-synced
shape-inst container`. Both are called with the local-page container whenever
`container` is the page. Instead of re-walking the full accumulator at every node to
discover those two changes, they can be marked at the point they are created.
A hidden correctness bug was also found: `add-shape-to-main` (called for `only-inst`
shapes — shapes in the copy that have no counterpart in the main component) generated
two misrouted change types:
1. **`mod-obj-change`** — updates `:shape-ref` on the original instance shapes (page-level
changes). These were missing `:local-change? true`, so they were routed to the
remote library file instead of the local page, leaving the instance's `:shape-ref`
links un-updated after an inverse sync on a remote-library component.
2. **`del-obj-change`** — the undo of "add shape to component" should delete the newly
cloned shapes from the component container. It used `:page-id (:id page)` instead
of `(make-change component-container ...)`, which would have tried to delete
component-only shapes from the page (where they don't exist) on undo.
### Changes made
**`change-touched`** (`~line 1543`):
- Added `(let [local? (cfh/page? container)])` wrapper.
- Both `:redo-changes` and `:undo-changes` `make-change` calls wrapped with
`(cond-> ... local? (assoc :local-change? true))`.
- Effect: page-container `:set-touched` changes are tagged at creation time; O(1)
per call.
**`change-remote-synced`** (`~line 1590`):
- Same treatment as `change-touched`.
**`generate-sync-shape-inverse-recursive`** (tail, `~line 1180`):
- Removed the `check-local` / `(update :redo-changes (partial mapv check-local))` /
`(update :undo-changes (partial map check-local))` block entirely.
- The function now just returns `changes` — no per-node re-walk.
- Effect: O(K) total tagging work instead of O(K²); no lazy `:undo-changes` nesting.
**`add-shape-to-main`** (`~line 1388`):
- `mod-obj-change`: added `:local-change? true` to both redo and undo `mod-obj`
entries (they carry `:page-id` and always target the local file).
- `del-obj-change`: replaced the hand-rolled `{:page-id ...}` map with
`(make-change component-container {:type :del-obj ...})` so the undo `:del-obj`
carries `:component-id` and is correctly routed to the library file.
### Test added
`test-inverse-sync-remap-changes-marks-page-changes-as-local` in
`common/test/common_tests/logic/comp_sync_test.cljc`:
- Creates a library file with a minimal component (root only).
- Instantiates it in a local file and adds an extra child (`only-inst` scenario).
- Calls `generate-sync-shape-inverse`.
- Asserts that `:mod-obj :shape-ref` changes (from `add-shape-to-main`) are
`:local-change? true`.
- Asserts that `:mod-obj :set-touched` changes for `copy-root` on the page are
`:local-change? true`.
All 1055 common tests pass.

View File

@ -0,0 +1,152 @@
# BUG-08 — `variant-switch` crashes on empty / out-of-range target (plugin-facing)
- **Category:** Correctness
- **Confidence:** High
- **Severity:** High — reproducible silent workspace crash, reachable from the public Plugin API
- **Hot path:** Yes — variant switch UI and Plugin API `switchVariant`
## Onboarding (fresh context — read first)
- Repo root for local file tools (Read/Edit/Write): `/home/alotor/kaleidos/penpot/penpot`
- Serena / REPL project root (devenv container): `/home/penpot/penpot`.
- Read first: `docs/component-subsystem-handoff.md` (§5.5 component swap / variant switch, §8 variants) and Serena memory `critical-info` (note the "silent crash" failure mode: `execute_code` returns OK but the workspace dies 12s later).
- Relevant memories: `mem:common/component-swap-pipeline`, `mem:frontend/plugin-api-to-cljs-binding`, `mem:frontend/handling-crashes`.
- Line numbers drift — re-locate symbols by name.
## Summary
`variant-switch` selects the nearest target variant with
`(apply min-key f valid-comps)`. When `valid-comps` is **empty**, this reduces to
`(min-key f)`, which has no matching arity → **ArityException**. The exception is
thrown *before* the `(when nearest-comp …)` guard that was written precisely to
handle "no match → do nothing", so that guard is dead in exactly its intended case.
Two reachable triggers, both via the public Plugin API `switchVariant(pos, value)`,
which validates only the *types* of its arguments (not that they reference an
existing property/value):
1. A `value` that no sibling variant has at `pos``valid-comps` empty → ArityException.
2. A `pos` ≥ number of variant properties → `(update pos assoc :value val)` does
`(assoc vec out-of-range …)``IndexOutOfBoundsException` (and feeds
`ctv/distance` a malformed property vector).
Both surface as a **silent workspace crash** from the plugin's perspective.
## Affected code
`frontend/src/app/main/data/workspace/variants.cljs``variant-switch` (~line 699):
```clojure
target-props (-> (:variant-properties component)
(update pos assoc :value val)) ; (2) out-of-range pos throws here
valid-comps (->> variant-comps
(remove #(= (:id %) component-id))
(filter #(= (dm/get-in % [:variant-properties pos :value]) val))
(reverse))
nearest-comp (apply min-key #(ctv/distance target-props (:variant-properties %)) valid-comps) ; (1) empty → ArityException
;; ...
(when nearest-comp ...) ; ← intended "no match → do nothing" guard, never reached in the empty case
```
Plugin binding (no value/range validation):
`frontend/src/app/plugins/shape.cljs``:switchVariant` (~line 1397):
```clojure
:switchVariant
(fn [pos value]
(cond
(not (nat-int? pos)) (u/not-valid plugin-id :pos pos)
(not (string? value)) (u/not-valid plugin-id :value value)
:else
(let [shape (u/locate-shape file-id page-id id)
component (u/locate-library-component file-id (:component-id shape))]
(when (and component (ctk/is-variant? component))
(st/emit! (-> (dwv/variants-switch {:shapes [shape] :pos pos :val value}) ...))))))
```
`variants-switch` (~line 741) fans out to `variant-switch` per shape.
## Established convention (use it for the fix)
`common/src/app/common/logic/variant_properties.cljc``generate-remove-property`
(~line 36) already range-checks before using `pos`:
```clojure
(if (and (seq props) (<= 0 pos) (< pos (count props)))
...
changes) ; out-of-range pos → safe no-op
```
`variant-switch` is the one place in the variant code that uses `pos` and an
empty-collection reduction **without** such guards. Match the existing convention.
## Proposed fix
1. **Guard the empty reduction** in `variant-switch` so it cannot throw and the
existing `(when nearest-comp …)` becomes live:
```clojure
nearest-comp (when (seq valid-comps)
(apply min-key #(ctv/distance target-props (:variant-properties %)) valid-comps))
```
2. **Range-check `pos`** before `(update pos assoc :value val)` — either bail early in
`variant-switch` when `pos` is out of range for `(:variant-properties component)`,
and/or reject it in the `:switchVariant` binding with `u/not-valid` (mirror the
`nat-int?` check). Prefer validating in the binding for a clean plugin-facing error,
plus a defensive guard in `variant-switch` since the workspace UI also calls it.
3. Make sure `nearest-comp-children` / `comps-nesting-loop?` (computed from
`nearest-comp` just below) tolerate `nil` `nearest-comp` — moving them inside the
`(when nearest-comp …)` is the cleanest option.
## Reproduction
Plugin (or REPL emulating the plugin path) on a variant copy head:
- `switchVariant(pos, "value-that-no-variant-has")` → ArityException → workspace Internal Error ~12s later.
- `switchVariant(99, "anything")` on a variant with < 100 properties IndexOutOfBounds.
Detect the crash (per `mem:frontend/handling-crashes`) from `cljs_repl`:
`(some? (:exception @app.main.store/state))`.
## Test guardrails
Frontend unit test mirroring the Plugin API path. From `frontend/`:
```bash
pnpm run test:quiet -- --focus frontend-tests.logic.components-and-tokens
```
Add cases:
- switch to a property value with no matching sibling variant → no-op, no throw.
- switch with an out-of-range `pos` → no-op (and/or `u/not-valid` from the binding), no throw.
- existing valid switches still pick the same nearest variant (no regression).
New frontend test namespaces must be registered in `frontend_tests/runner.cljs`
(not needed if adding vars to an existing namespace).
## Acceptance criteria
- `switchVariant` with a non-existent value or out-of-range `pos` is a safe no-op (or a clean `u/not-valid`), never a crash.
- Valid switches behave exactly as before.
- A regression test covers both the empty-target and out-of-range-`pos` cases.
## Fix summary
**`frontend/src/app/main/data/workspace/variants.cljs``variant-switch`:**
- Extracted `props` from `(:variant-properties component)` into the outer `let`.
- Merged the existing `(not= val ...)` guard with a pos range check `(seq props) (<= 0 pos) (< pos (count props))`, mirroring the `generate-remove-property` convention. This prevents `IndexOutOfBoundsException` before `(update pos assoc :value val)` is ever reached.
- Guarded the `(apply min-key ...)` reduction with `(when (seq valid-comps) ...)`, so an empty candidate list yields `nil` instead of an ArityException.
- Moved `nearest-comp-children` and `comps-nesting-loop?` inside the `(when nearest-comp ...)` block, making the intended no-op guard live in the empty-collection case.
**`frontend/src/app/plugins/shape.cljs``:switchVariant`:**
- Added a pos range check in the `:else` branch after fetching the component: if `(>= pos (count (:variant-properties component)))`, calls `u/not-valid` immediately, giving the plugin caller a clean error before anything reaches `variant-switch`.
**`frontend/test/frontend_tests/logic/variant_switch_test.cljs`** (new file):
- Three tests: nonexistent value → no-op, out-of-range `pos` → no-op, valid switch → copy swaps to target component.
- Registered in `frontend/test/frontend_tests/runner.cljs`.
- All 284 frontend tests pass with 0 failures.

View File

@ -0,0 +1,27 @@
# Component / Variant subsystem — bug reports
Each report below is **self-contained** and meant to be tackled independently in a
fresh agent context. They came out of the audit captured in
[`../component-subsystem-handoff.md`](../component-subsystem-handoff.md) (§14).
Read the handoff doc and the Serena memory `critical-info` before starting any of
them.
| # | Title | Category | Confidence | Severity | On hot path? |
|---|---|---|---|---|---|
| [BUG-01](./BUG-01-concat-changes-lazy-undo.md) | `concat-changes` accumulates `:undo-changes` as nested lazy `concat` | Stability + Perf | High | Medium (latent crash on large files) | Yes (library sync) |
| [BUG-02](./BUG-02-refchain-walking-memoization.md) | Un-memoized ref-chain walking during variant switch | Perf | Medium | LowMedium | Yes (variant switch) |
| [BUG-03](./BUG-03-compare-children-quadratic.md) | `compare-children` fallback is O(n²) with expensive constant | Perf | Medium | LowMedium | Yes (sync) |
| [BUG-04](./BUG-04-update-attrs-on-switch-geometry-guards.md) | `update-attrs-on-switch` composite-geometry guard audit | Correctness | Low (audit) | Variable | Yes (swap) |
| [BUG-05](./BUG-05-direct-sync-silent-noop-diagnostics.md) | Silent no-op vs. bug ambiguity in direct sync | Diagnosability | Low | Low | Yes (sync) |
| [BUG-06](./BUG-06-index-of-quadratic-sync-callbacks.md) | `d/index-of` linear scan inside per-child sync callbacks | Perf | High | Medium | Yes (sync) |
| [BUG-07](./BUG-07-inverse-sync-remap-changes.md) | Inverse sync re-maps the entire change list at every tree node | Perf + Stability | High | Medium | Yes ("Update main") |
| [BUG-08](./BUG-08-variant-switch-crash-empty-target.md) | `variant-switch` crashes on empty / out-of-range target (plugin-facing) | Correctness | High | **High (reproducible crash)** | Yes (Plugin API) |
## Suggested order
1. **BUG-08** — reproducible plugin-triggered crash, small guard fix, clear test. Most aligned with the current `alotor-fix-plugins-problems` branch.
2. **BUG-06** — pure perf, mechanical, no behavior change.
3. **BUG-01 / BUG-07** — the lazy-`concat` stability pair (real crash risk on large files; needs care to preserve undo order).
4. **BUG-02 / BUG-03** — profile first, then optimize.
5. **BUG-04 / BUG-05** — audit / diagnosability, lower urgency.

View File

@ -0,0 +1,440 @@
# Penpot Component Subsystem — Engineering Hand-off
> **Audience:** agents/engineers tasked with fixing bugs in Penpot's component,
> copy, and variant machinery. This is a map of the territory, not a tutorial.
> It tells you *where* things live, *how* the main flows work, and *where the
> mines are buried*. Read the "Sharp edges & unclear areas" section before you
> touch sync logic.
All paths are repo-relative. Line numbers drift; treat them as starting points and
re-locate symbols by name.
---
## 1. What the subsystem does
A **component** is a reusable shape tree. Users instantiate it to get **copies**
(instances). A copy can diverge from its source ("overrides", tracked with
`:touched` flags). Changes can flow in two directions:
- **Copy → Main** ("update main" / inverse sync): push a copy's local changes back
into the component definition.
- **Main → Copies** ("sync" / direct sync): propagate the component definition to
every copy, in every page, even across files (remote libraries).
**Variants** are a newer layer on top: a set of related components grouped in a
*variant container* frame, distinguished by named properties, with a UI to switch
a copy from one variant to another (which is implemented as a *component swap*).
This is the most complex, most error-prone area of the codebase. It crosses the
`common`/`frontend`/`backend` boundary, mutates a shared tree, and has many
special cases (nested copies, swaps, fostered children, remote libraries, grid/flex
layout, text, paths).
---
## 2. Mental model & glossary
### Shape roles (a shape can hold several at once)
| Role | Marker attrs | Predicate |
|---|---|---|
| **Main instance / master** | `:main-instance true`, `:component-id`, `:component-file` | `ctk/main-instance?` |
| **Copy / non-main instance** | `:shape-ref` (points up the inheritance chain) | `ctk/in-component-copy?` (≈ `(some? (:shape-ref shape))`) |
| **Component root** | `:component-root true`, plus `:component-id` / `:component-file` | `ctk/instance-root?` |
| **Instance head** (top of a nested sub-instance) | — | `ctk/instance-head?`, `subinstance-head?`, `subcopy-head?` |
| **Variant master** | main instance + component root + `:variant-id` | `ctk/is-variant?` |
| **Variant container** | frame with `:is-variant-container true` | `ctk/is-variant-container?` |
> ⚠️ Roles are **not mutually exclusive**. A variant master is a main instance *and*
> a component root, and its descendants can themselves be copies (nested instances).
> Any logic that assumes "this shape is only a master" or "only a copy" is suspect.
### Key concepts
- **`:shape-ref`** — a copy shape points to the equivalent shape one level up the
inheritance hierarchy (the "near main"). Chains can be multiple levels deep
(nested components) and can **cross files** for remote libraries.
- **`:touched`** — a set of *override-group* keywords (`:geometry-group`,
`:fill-group`, `:content-group`, `:name-group`, …). Presence means "this copy
diverged from its main for attrs in that group, so don't overwrite them on sync".
- **swap slot** (`:swap-slot-*` via `ctk/get-swap-slot`/`set-swap-slot`) — records
that a sub-instance was swapped for a different component, so sync knows the head
no longer matches its original `:shape-ref` position.
- **direct vs remote copy**`ctf/direct-copy?` distinguishes a copy whose
`:shape-ref` points directly into the nearest component vs. one inheriting through
intermediate components. Normal sync only touches direct/near mains.
### Namespace aliases you will see everywhere
| Alias | Namespace | Role |
|---|---|---|
| `ctk` | `app.common.types.component` | predicates, touched/swap-slot helpers, data model |
| `ctkl` | `app.common.types.components-list` | the library's component registry (CRUD over `:components`) |
| `ctf` | `app.common.types.file` | ref-shape resolution, component lookup across files |
| `ctn` | `app.common.types.container` | shape-tree access + `make-component-instance` |
| `cll` | `app.common.logic.libraries` | **the sync/instantiate/swap engine** |
| `clv` | `app.common.logic.variants` | variant switch / keep-touched logic |
| `clvp`| `app.common.logic.variant-properties` | variant property name/value edits |
| `ctv` | `app.common.types.variant` | variant data types |
| `cfv` | `app.common.files.variant` | variant container/file-level helpers |
| `pcb` | `app.common.files.changes_builder` | fluent change-set builder |
| `dwl` | `app.main.data.workspace.libraries` (frontend) | Potok events |
---
## 3. Data model — where state lives
### In the file/library data (`:data`)
- **`:components`** — map of `component-id → component record`. Managed by `ctkl`
(`app.common.types.components-list`). A component record holds:
`:id :name :path :main-instance-id :main-instance-page`, optional
`:variant-id :variant-properties`, `:deleted` (soft-delete / "deleted component"
used by undo & detached-but-referenced copies), and for deleted components an
embedded `:objects` snapshot.
- **The main instance lives in the page tree**, not inside the component record. The
record only points to it via `:main-instance-id` + `:main-instance-page`. This
indirection is a frequent source of "invalid main instance" validation errors —
see `validate.cljc`.
### On shapes (page objects)
`:component-id :component-file :component-root :main-instance :shape-ref :touched`
plus swap-slot and variant attrs (`:variant-id :variant-name`).
### Component v2
Full referential/semantic validation only runs when the file's features contain
`"components/v2"`. v1 behavior is mostly legacy; on v2 the main instance is a real
shape in a page (note `make-component-instance` forces `:parent-id nil` /
`:frame-id uuid/zero` on the cloned component-shape "to behave like v1").
---
## 4. File map — the main files to check
### `common/` — the engine (runtime-agnostic, the real logic)
| File | What's in it |
|---|---|
| `common/src/app/common/logic/libraries.cljc` (~3100 lines) | **The core.** Instantiate, both sync directions, attribute-copy algorithm, swap, detach, reset, add/duplicate component. Start here for almost any bug. |
| `common/src/app/common/logic/variants.cljc` | Variant switch support: `generate-keep-touched`, `find-shape-ref-child-of`, `add-touched-from-ref-chain`, `generate-add-new-variant`. |
| `common/src/app/common/logic/variant_properties.cljc` | Variant property name/value generation. |
| `common/src/app/common/logic/shapes.cljc` | Generic shape ops that special-case copies (e.g. delete-inside-copy, swap). |
| `common/src/app/common/types/component.cljc` | Data model: predicates, `sync-attrs` (attr→group map), `swap-keep-attrs`, touched/swap-slot get/set, `detach-shape`, `diff-components`. |
| `common/src/app/common/types/components_list.cljc` | Component registry CRUD; soft delete (`mark-component-deleted/undeleted`), `update-component`, `set-component-modified`. |
| `common/src/app/common/types/file.cljc` | **Ref-shape resolution** (the chain walkers): `get-ref-shape`, `find-ref-shape`, `find-remote-shape`, `get-component-root`, `direct-copy?`, `find-swap-slot`, `get-ref-chain-until-target-ref`, `find-near-match`, `advance-shape-ref`. |
| `common/src/app/common/types/container.cljc` | `make-component-instance` (the clone-and-relink primitive), shape-tree accessors. |
| `common/src/app/common/types/variant.cljc` / `files/variant.cljc` | Variant types and container/file helpers. |
| `common/src/app/common/files/changes_builder.cljc` (`pcb`) | How every mutation is expressed; `:ignore-touched`, `set-translation?`, `update-shapes`. |
| `common/src/app/common/files/changes.cljc` | `process-operation` multimethod, `set-shape-attr`, second-pass touched handling. |
| `common/src/app/common/files/validate.cljc` (~980 lines) | All component invariants and the error codes they raise. **Read this to learn what "correct" means.** |
### `frontend/` — events & UI
| File | What's in it |
|---|---|
| `frontend/src/app/main/data/workspace/libraries.cljs` (`dwl`, ~1600 lines) | Potok events: `instantiate-component`, `detach-component(s)`, `reset-component(s)`, `update-component`, `update-component-sync`, `component-swap`, `component-multi-swap`, `sync-file`, `watch-component-changes`, restore/delete/duplicate/rename component. **The UI→common bridge.** |
| `frontend/src/app/main/data/workspace/variants.cljs` | Variant events: `variant-switch`, `variants-switch`, `add-new-variant`, `combine-as-variants`, property edits. Switch ultimately calls `dwl/component-swap`. |
| `frontend/src/app/main/ui/workspace/sidebar/options/menus/component.cljs` (~1340 lines) | The component options panel: update main, reset, detach, swap UI, show-main, annotations, variant property editor. The main user entry surface. |
| `frontend/src/app/main/ui/workspace/sidebar/assets/components.cljs` | The assets library panel (list, group, instantiate via drag, context menu). |
| `frontend/src/app/main/ui/inspect/.../variant*.cljs` | Inspect/codegen of variants. |
### Backend
The backend stores and re-validates file data; it does **not** re-run component
sync logic, but it does run validation/repair on persisted files and an async
process that **remaps media references** after instantiation (see the WARNING in
`make-component-instance` — media refs are fixed up server-side, not at clone time).
### Tests (canonical behavior references)
- `common/test/common_tests/logic/comp_sync_test.cljc` — direct/inverse sync.
- `common/test/common_tests/logic/copying_and_duplicating_test.cljc`
- `common/test/common_tests/logic/text_sync_test.cljc`
- `common/test/common_tests/logic/variants_switch_test.cljc` — **canonical swap+touched suite.**
- `common/test/common_tests/logic/variants_test.cljc`
- `common/test/common_tests/types/components_test.cljc`, `types/variant_test.cljc`, `variant_test.cljc`
- `frontend/test/frontend_tests/logic/components_and_tokens.cljs`
- Test helpers: `common/src/app/common/test_helpers/{components,compositions,variants,files}.cljc`.
Notably `compositions/swap-component-in-shape` drives the real swap pipeline; use
`{:keep-touched? true}` to exercise variant-switch behavior. `thf/apply-changes`
is the production applier analog and validates by default.
---
## 5. Core code flows
### 5.1 Instantiate a component
`dwl/instantiate-component``cll/generate-instantiate-component`
(`libraries.cljc:236`) → `ctn/make-component-instance` (`container.cljc:299`).
`make-component-instance` clones the component's shape tree, assigns new ids/names,
and for each cloned shape (`update-new-shape`):
- moves it by `delta` and **dissociates `:touched :variant-id :variant-name`** (clean copy);
- for main instances: sets `:main-instance`, drops `:shape-ref`;
- for copies: sets `:shape-ref` to the original shape id (the near instance);
- sets `:component-root`/`:component-id`/`:component-file` on the root only.
`generate-instantiate-component` then fixes parent/frame ids, handles dropping into
grid layouts, and emits `:add-obj` changes with `:ignore-touched true`.
> When debugging "instance came out wrong", determine **which clone path** produced
> it: `make-component-instance` (clean) vs `duplicate-component`/`generate-duplicate-*`
> (does **not** clean inherited attrs — source `:touched` etc. can leak in).
### 5.2 Direct sync (Main → Copies)
`cll/generate-sync-file` (`libraries.cljc:483`) iterates every container (pages +
deleted-component objects) → `generate-sync-container``generate-sync-shape`
(multimethod on asset type `:components`/`:colors`/`:typographies`) →
`generate-sync-shape-direct` (`:796`) → `generate-sync-shape-direct-recursive`.
- Only operates when `in-component-copy?` **and** (`direct-copy?` or `reset?`).
- `reset?` resolves the main against the **ref-shape** (`find-ref-shape`); normal
sync resolves against the component's stored ref (`get-ref-shape`).
- Recursion compares children (`compare-children`) to add/remove/move shapes and
calls `update-attrs` per matched pair.
Frontend triggers: `dwl/update-component-sync`, `dwl/sync-file`,
`dwl/watch-component-changes` (auto-sync after edits to a main), and the explicit
"Update main component" action.
### 5.3 Inverse sync (Copy → Main) — "Update main"
`dwl/update-component``cll/generate-sync-shape-inverse` (`:1010`) →
`generate-sync-shape-inverse-recursive`. Resolves the remote shape with
`ctf/find-remote-shape` (walks `:shape-ref` across files), pushes copy values up,
renames the component if `:name-group` is touched, and marks the component modified.
### 5.4 Reset / Detach
- **Reset** (`dwl/reset-component(s)`): direct sync with `reset? = true` — discards
overrides by syncing the copy back to its ref-shape. `cll/generate-reset-component`.
- **Detach** (`dwl/detach-component(s)`): `cll/generate-detach-component` /
`generate-detach-recursive` / `generate-detach-immediate` — strips `:shape-ref`,
component attrs, swap slots via `ctk/detach-shape`. `advance-nesting-level` handles
nested cases.
### 5.5 Component swap (and variant switch)
Single swap workhorse: `dwl/component-swap``cll/generate-component-swap`. It
removes the old shape and instantiates the target in place
(`generate-new-shape-for-swap``generate-instantiate-component`
`make-component-instance`), preserving identity (swap slot) so sync still works.
`component-multi-swap` batches and calls `component-swap` with `keep-touched? = false`.
**`keep-touched? = true`** (single swap / variant switch) additionally runs
`clv/generate-keep-touched` (`variants.cljc`):
1. walk pre-swap children, union chain-derived touched via `add-touched-from-ref-chain`;
2. for each, find the equivalent target child via `find-shape-ref-child-of`;
3. call `cll/update-attrs-on-switch` to carry user overrides onto the fresh copy.
**Variant switch:** `variants.cljs/variant-switch` / `variants-switch` (driven by the
property-toggle UI and the Plugin API `switchVariant`) compute the target component
and delegate to `dwl/component-swap` with `keep-touched? true`.
### 5.6 Add component from selection
`dwl/add-component``add-component2``cll/generate-add-component` /
`generate-add-component-changes`: converts selected shapes into a main instance,
registers the component in `:components`, and (for variants) may create a variant
container. Dropping a shape into a variant container can auto-convert it to a
variant via `generate-make-shapes-variant` — **treat drag/drop into a variant
container as a component operation, not a plain reparent.**
---
## 6. The attribute-sync algorithm (`update-attrs`, `libraries.cljc:1790`)
The heart of direct/inverse sync. For a `(dest-shape, origin-shape)` pair it loops
`updatable-attrs` and copies origin→dest, with these rules:
- **Geometry is relative:** `reposition-shape` moves `origin-shape` so its root
aligns with `dest-root` before comparison (coordinates are absolute, but only the
relative position should sync). For subinstances, comparison is always against the
*near* component.
- `omit-touched?` true ⇒ skip attrs whose `sync-group` is in dest's `:touched`.
- Skip when origin == dest for that attr.
- **`:position-data`** is derived: reset to `nil` (recomputed) when geometry is
touched and text position-data changed.
- **Text `:content`** is special: text *and* formatting share one `:content` attr;
on partial change it merges (`text-change-value`) and forces a position-data reset.
- After the loop: `check-detached-main`, `check-swapped-main`, and token sync
(`generate-update-tokens`).
`add-update-attr-changes`/`add-update-attr-operations` build the `:mod-obj`
`:set` operations (with inverse ops for undo).
### `update-attrs-on-switch` (`libraries.cljc`, swap-specific)
Separate from `update-attrs`. Compares three shapes: `current-shape` (fresh target
copy), `previous-shape` (pre-swap shape with chain-derived touched), `origin-ref-shape`
(source variant master's equivalent). Loops `sync-attrs` except `swap-keep-attrs`,
copying overrides through several guards (skip equal values, skip equal composite
geometry, require the touched group, require source/target masters to agree, dedicated
fixed-layout geometry handling, specialized text/path conversion). The generic
fallback copies from `previous-shape`**this is where most swap bugs surface** when
a guard fails to reject incompatible geometry or a master mismatch.
---
## 7. Touched flags & overrides
- `sync-attrs` (`component.cljc`) maps every syncable attr to its touched group.
**Any new syncable shape attr must be added here** or sync silently ignores it.
- `set-touched-group` is the only legitimate setter; the central `set-shape-attr`
path calls it only for copies and only when ignore flags allow.
- Masters are *not normally* touched, but touched flags can leak onto masters via
clone/duplicate paths, and `add-touched-from-ref-chain` unions ancestors' touched
into a copy — so a shape that looks untouched locally may behave as touched.
- Width/height are **excluded** from the `is-geometry?` ignore branch in
`set-shape-attr` — don't assume all geometry-group attrs behave identically.
- Geometry differences under ~1px are treated as equal (approximate equality) for
touched purposes. See `mem:common/decimals-and-coordinates`.
---
## 8. Variants
- A variant container is a frame `:is-variant-container true`; children are variant
masters carrying `:variant-id` (→ container) and `:variant-name` (the value).
- Component records carry `:variant-properties`.
- Predicates are deliberately broad: `ctk/is-variant?` matches both variant master
shapes and component rows; `is-variant-container?` checks the frame flag.
- Switch flow: §5.5. Property edits: `variant_properties.cljc` + `variants.cljs`.
- `find-shape-ref-child-of` walks the ref chain to find the equivalent master child
in the target variant — central to mapping overrides across a switch.
---
## 9. Validation & repair (`common/src/app/common/files/validate.cljc`)
Runs full referential/semantic checks only on `components/v2` files. It is the best
single source of "what invariants must hold". Error codes worth knowing (line ~3067):
`:component-not-main`, `:component-main-external`, `:component-not-found`,
`:invalid-main-instance-id/-page/-instance`, `:component-main`, `:shape-ref-in-main`,
`:component-id-mismatch`, `:component-nil-objects-not-allowed`, `:shape-ref-cycle`,
`:component-duplicate-slot`.
`repair-file` does **not** mutate directly — it reduces validation errors into redo
changes via `changes-builder`; callers must apply/persist them. When a fix "doesn't
stick", check that the repair changes were actually applied.
---
## 10. Debugging recipes (from `mem:common/component-debugging-recipes`)
Two runtimes are in play for live work: `mcp__penpot__execute_code` (Plugin API,
can crash silently) and `mcp__penpot__cljs_repl` (raw REPL, survives crashes). Detect
a crash with `(some? (:exception @app.main.store/state))`.
**Inspect what a UI action emitted** (last undo entries / `:mod-obj` operations):
walk `[:workspace-undo :items]` in the store — snippet in the debugging-recipes memory.
**Trace `update-attrs-on-switch`** during a real swap by runtime-patching the var in
`cljs_repl` (capture `curr`/`prev`/`origin-ref`), trigger the UI action, inspect the
buffer, then restore. Runtime patching beats source instrumentation (no recompile).
**Tests:** `thf/dump-file file :keys [...]` prints a shape tree; prefer
production-path helpers (`cls/generate-update-shapes` + `thf/apply-changes`) over ad
hoc map mutation; use `tho/swap-component-in-shape {:keep-touched? true}` for swaps.
> ⚠️ The Serena project root resolves to `/home/penpot/penpot` (devenv container),
> but local file tools use `/home/alotor/kaleidos/penpot/penpot`. Use the right
> prefix per tool.
---
## 11. Sharp edges & areas not obvious from a superficial read
1. **Role overlap.** Variant masters are simultaneously main + root, and copies
nest. Never assume exclusivity. Bugs hide in code that branches on a single role.
2. **Two clone paths with different cleanliness.** `make-component-instance` produces
*clean* copies (drops `:touched`/variant attrs); `duplicate-component` /
`generate-duplicate-*` do **not** — inherited source attrs survive. Identify the
path before changing sync logic.
3. **`update-attrs` vs `update-attrs-on-switch` are different functions** with
different guard sets. Fixing one doesn't fix the other.
4. **Composite geometry (`:selrect`, `:points`) bypasses the simple
different-master skip** in swap; width/height checks catch some but not all
positional mismatches. Classic source of "swap moved/resized my shape".
5. **`previous-shape` may be repositioned** (destination-root minus origin-root)
before copy. Often zero for variant switch — do **not** assume zero for other swap
entry points (Plugin API, multi-swap).
6. **Inherited touched via ref chain.** `add-touched-from-ref-chain` makes a locally
untouched shape behave as touched. Check the *effective* touched set, not the
stored one.
7. **Cross-file ref chains.** `:shape-ref` and `find-remote-shape` walk across remote
libraries. A missing/unlinked library makes the main "not found" and sync silently
no-ops (`generate-sync-shape-direct` returns `changes` unchanged). Distinguish
"no-op because unlinked" from "no-op because of a bug".
8. **Swap slots** are the only thing that keeps a swapped sub-instance syncing
correctly. If a swap slot is missing/duplicated, expect `:component-duplicate-slot`
or wrong-position syncs. See `find-swap-slot`, `match-swap-slot?`.
9. **The main instance lives in a page, referenced indirectly** by
`:main-instance-id`/`-page`. Moving/deleting/duplicating pages or the main shape
can desync the component record → `:invalid-main-instance-*`.
10. **Media references are remapped asynchronously on the backend** after
instantiation, not at clone time. A freshly instantiated copy may have
"unreferenced" fills/strokes until the backend fixes them — don't "fix" this in
the clone path.
11. **`generate-sync-file` early-return & `concat-changes`** carry a `TODO Remove
concat changes`; the change-set plumbing here is known-awkward.
12. **`set-translation? true`** marks a change set translation-only so sync skips
expensive work — a wrong/missing flag changes whether sync runs.
13. **Touched second pass.** Change application does a *second pass* for collected
touched changes (`process-touched-change`), which can mark a component modified
from a plain shape op. See `mem:common/file-change-validation-migration-subtleties`.
14. **Tokens interact with sync** (`generate-update-tokens` inside `update-attrs`).
Token application/propagation has its own subtleties — see
`mem:frontend/workspace-token-subtleties`, `mem:common/tokens-schema-subtleties`.
---
## 12. Where to start, by symptom
| Symptom | Start here |
|---|---|
| Override lost / overwritten on sync | `update-attrs` + `:touched`/`sync-attrs`; check `omit-touched?` and the touched group. |
| Swap moves/resizes/loses overrides | `update-attrs-on-switch` guards; `generate-keep-touched`; composite geometry skip. |
| "Update main" doesn't propagate | `generate-sync-shape-inverse` / `find-remote-shape`; was component marked modified? |
| Copy doesn't update from main | `generate-sync-shape-direct`; `direct-copy?`; is the library linked? |
| Validation errors on save | `validate.cljc` error code → the specific `check-*` fn; then `repair-file`. |
| Variant switch wrong child | `find-shape-ref-child-of`, `add-touched-from-ref-chain` in `variants.cljc`. |
| New attr not syncing | add it to `ctk/sync-attrs` with the right group. |
| Nested / fostered / swapped child weirdness | ref-chain walkers in `types/file.cljc`; swap slots in `ctk`. |
---
## 13. Authoritative references
Serena memories (read before deep work — they are the project's primary guidance):
`mem:common/component-data-model`, `mem:common/component-swap-pipeline`,
`mem:common/component-debugging-recipes`, `mem:common/changes-architecture`,
`mem:common/file-change-validation-migration-subtleties`,
`mem:common/data-model-change-checklist`, `mem:common/core`, `mem:frontend/core`.
The canonical behavioral spec is the test suite under
`common/test/common_tests/logic/` — read neighboring tests before adding a case.
---
## 14. Findings — correctness / stability / performance audit
The findings from auditing this subsystem have been split into self-contained,
independently-actionable bug reports under
[`component-bug-reports/`](./component-bug-reports/) (see its `README.md` for the
index, suggested order, and severity table). Summary:
| Report | Title | Category | Confidence |
|---|---|---|---|
| BUG-01 | `concat-changes` accumulates `:undo-changes` as nested lazy `concat` | Stability + Perf | 🟢 High |
| BUG-02 | Un-memoized ref-chain walking during variant switch | Perf | 🟡 Medium |
| BUG-03 | `compare-children` fallback is O(n²) with an expensive constant | Perf | 🟡 Medium |
| BUG-04 | `update-attrs-on-switch` composite-geometry guard audit | Correctness (audit) | 🔵 Low |
| BUG-05 | Silent no-op vs. bug ambiguity in direct sync | Diagnosability | 🔵 Low |
| BUG-06 | `d/index-of` linear scan inside per-child sync callbacks | Perf | 🟢 High |
| BUG-07 | Inverse sync re-maps the entire change list at every tree node | Perf + Stability | 🟢 High |
| BUG-08 | `variant-switch` crashes on empty / out-of-range target (plugin-facing) | Correctness | 🟢 High |
Each report is self-contained (onboarding pointers, affected code, root cause, fix
direction, reproduction, test guardrails, acceptance criteria) so it can be picked up
cold in a separate context. Update a report's status there as it is fixed.

View File

@ -705,39 +705,42 @@
(watch [it state _]
(let [libraries (dsh/lookup-libraries state)
component-id (:component-id shape)
component (ctf/get-component libraries (:component-file shape) component-id :include-deleted? false)]
;; If the value is already val, do nothing
(when (not= val (dm/get-in component [:variant-properties pos :value]))
component (ctf/get-component libraries (:component-file shape) component-id :include-deleted? false)
props (:variant-properties component)]
;; If the value is already val, or pos is out of range, do nothing
(when (and (not= val (dm/get-in component [:variant-properties pos :value]))
(seq props) (<= 0 pos) (< pos (count props)))
(let [current-page-objects (dsh/lookup-page-objects state)
variant-id (:variant-id component)
component-file-data (dm/get-in libraries [(:component-file shape) :data])
component-page-objects (-> (dsh/get-page component-file-data (:main-instance-page component))
(get :objects))
variant-comps (cfv/find-variant-components component-file-data component-page-objects variant-id)
target-props (-> (:variant-properties component)
(update pos assoc :value val))
target-props (update props pos assoc :value val)
valid-comps (->> variant-comps
(remove #(= (:id %) component-id))
(filter #(= (dm/get-in % [:variant-properties pos :value]) val))
(reverse))
nearest-comp (apply min-key #(ctv/distance target-props (:variant-properties %)) valid-comps)
nearest-comp (when (seq valid-comps)
(apply min-key #(ctv/distance target-props (:variant-properties %)) valid-comps))
shape-parents (cfh/get-parents-with-self current-page-objects (:parent-id shape))
nearest-comp-children (cfh/get-children-with-self component-page-objects (:main-instance-id nearest-comp))
comps-nesting-loop? (seq? (cfh/components-nesting-loop? nearest-comp-children shape-parents))
{:keys [on-error]
:or {on-error rx/throw}} (meta params)]
;; If there is no nearest-comp, do nothing
(when nearest-comp
(if comps-nesting-loop?
(do
(on-error)
(rx/empty))
(rx/of
(dwl/component-swap shape (:component-file shape) (:id nearest-comp) true)
(ev/event (-> {::ev/name "variant-switch" ::ev/origin "workspace:design-tab"}
(merge (meta it)))))))))))))
(let [nearest-comp-children (cfh/get-children-with-self component-page-objects (:main-instance-id nearest-comp))
comps-nesting-loop? (seq? (cfh/components-nesting-loop? nearest-comp-children shape-parents))]
(if comps-nesting-loop?
(do
(on-error)
(rx/empty))
(rx/of
(dwl/component-swap shape (:component-file shape) (:id nearest-comp) true)
(ev/event (-> {::ev/name "variant-switch" ::ev/origin "workspace:design-tab"}
(merge (meta it))))))))))))))
(defn variants-switch
"Switch each shape (that must be a variant copy head) for the closest one with the property value passed as parameter"

View File

@ -1406,7 +1406,10 @@
:else
(let [shape (u/locate-shape file-id page-id id)
component (u/locate-library-component file-id (:component-id shape))]
(when (and component (ctk/is-variant? component))
(cond
(not (and component (ctk/is-variant? component))) nil
(>= pos (count (:variant-properties component))) (u/not-valid plugin-id :pos pos)
:else
(st/emit! (-> (dwv/variants-switch {:shapes [shape] :pos pos :val value})
(se/add-event plugin-id)))))))

View File

@ -0,0 +1,96 @@
;; 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 INC Sucursal en España SL
(ns frontend-tests.logic.variant-switch-test
(:require
[app.common.test-helpers.components :as cthc]
[app.common.test-helpers.files :as cthf]
[app.common.test-helpers.ids-map :as cthi]
[app.common.test-helpers.shapes :as cths]
[app.common.test-helpers.variants :as thv]
[app.main.data.helpers :as dsh]
[app.main.data.workspace.variants :as dwv]
[cljs.test :as t :include-macros true]
[frontend-tests.helpers.pages :as thp]
[frontend-tests.helpers.state :as ths]
[frontend-tests.helpers.wasm :as thw]))
(t/use-fixtures :each
{:before (fn []
(thp/reset-idmap!)
(thw/setup-wasm-mocks!))
:after thw/teardown-wasm-mocks!})
(defn- setup-variant-file
[]
(-> (cthf/sample-file :file1)
(thv/add-variant :v01 :c01 :m01 :c02 :m02)
(cthc/instantiate-component :c01 :copy01)))
(defn- copy-instances-of
"Returns non-main-instance copies with the given component-id."
[page-objects comp-id]
(filter #(and (= (:component-id %) comp-id) (not (:main-instance %))) (vals page-objects)))
(t/deftest variant-switch-nonexistent-value-is-noop
"Switching to a value that no sibling variant has is a safe no-op — no exception, shape unchanged."
(t/async
done
(let [file (setup-variant-file)
store (ths/setup-store file)
copy01 (cths/get-shape file :copy01)
c01-id (cthi/id :c01)
c02-id (cthi/id :c02)
events
[(dwv/variants-switch {:shapes [copy01] :pos 0 :val "NonExistentValue"})]]
(ths/run-store
store done events
(fn [new-state]
(let [page-objects (dsh/lookup-page-objects new-state)]
(t/is (= 1 (count (copy-instances-of page-objects c01-id))) "copy01 still points to c01")
(t/is (empty? (copy-instances-of page-objects c02-id)) "no c02 copy was created")))))))
(t/deftest variant-switch-out-of-range-pos-is-noop
"Switching with an out-of-range pos is a safe no-op — no exception, shape unchanged."
(t/async
done
(let [file (setup-variant-file)
store (ths/setup-store file)
copy01 (cths/get-shape file :copy01)
c01-id (cthi/id :c01)
c02-id (cthi/id :c02)
events
[(dwv/variants-switch {:shapes [copy01] :pos 99 :val "Value2"})]]
(ths/run-store
store done events
(fn [new-state]
(let [page-objects (dsh/lookup-page-objects new-state)]
(t/is (= 1 (count (copy-instances-of page-objects c01-id))) "copy01 still points to c01")
(t/is (empty? (copy-instances-of page-objects c02-id)) "no c02 copy was created")))))))
(t/deftest variant-switch-valid-switches-to-nearest-component
"A valid switch with an existing value swaps the copy to the matching component."
(t/async
done
(let [file (setup-variant-file)
store (ths/setup-store file)
copy01 (cths/get-shape file :copy01)
c01-id (cthi/id :c01)
c02-id (cthi/id :c02)
events
[(dwv/variants-switch {:shapes [copy01] :pos 0 :val "Value2"})]]
(ths/run-store
store done events
(fn [new-state]
(let [page-objects (dsh/lookup-page-objects new-state)]
(t/is (empty? (copy-instances-of page-objects c01-id)) "original c01 copy was removed")
(t/is (= 1 (count (copy-instances-of page-objects c02-id))) "a c02 copy now exists")))))))

View File

@ -23,6 +23,7 @@
[frontend-tests.logic.frame-guides-test]
[frontend-tests.logic.groups-test]
[frontend-tests.logic.pasting-in-containers-test]
[frontend-tests.logic.variant-switch-test]
[frontend-tests.main-errors-test]
[frontend-tests.plugins.context-shapes-test]
[frontend-tests.plugins.parser-test]
@ -72,6 +73,7 @@
frontend-tests.logic.frame-guides-test
frontend-tests.logic.groups-test
frontend-tests.logic.pasting-in-containers-test
frontend-tests.logic.variant-switch-test
frontend-tests.plugins.context-shapes-test
frontend-tests.plugins.parser-test
frontend-tests.plugins.tokens-test