🐛 Keep multi-select variant switches refreshing every instance (#12857)

* 🐛 Keep multi-select variant switches refreshing every instance

Multi variants-switch ended each swap with select-shape, which replaced
the selection, interrupted layout/WASM work, and navigated once per copy
so only one instance looked updated under flex. Skip per-swap select and
layout in the batch, re-read live geometry, coalesce layout ids via a
companion DataEvent, then restore selection once. The same path covers
Assets component-multi-swap. Frontend tests lock select-shape count,
layout coalesce, and mixed no-op selections.

Closes #12829

AI-assisted-by: Composer
This commit is contained in:
Pablo Alba 2026-11-05 16:52:52 +01:00 committed by GitHub
parent d82b5a4b1e
commit f4a0d42363
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 427 additions and 99 deletions

View File

@ -1057,67 +1057,123 @@
(cfh/get-children-with-self (:objects page) (:id root))
(cfh/get-parents-with-self objects (:parent-id shape))))))
(def component-swap-layout-ids-type
"Companion DataEvent type emitted by `component-swap` when `:skip-layout?`
is set. Batch callers filter this exact keyword (libraries ns) — never a
bare `::component-swap-layout-ids` in another namespace."
::component-swap-layout-ids)
(def ^:private component-swap-batch-done-type
::component-swap-batch-done)
(defn coalesce-skipped-layout
"While `swap-source` runs, collect companion layout-id DataEvents from
`stream`, then emit one `:layout/update` with the union.
`swap-source` must emit WatchEvents already configured with
`:skip-layout? true`. Uses a done marker (not `(rx/take (count …))`) so
no-op swaps cannot hang the fold. Reads the buffer inside `rx/defer` so
the id set is not frozen empty when `watch` returns."
[stream swap-source]
(let [layout-acc (volatile! [])]
(rx/concat
(rx/merge
(->> stream
(rx/take-until (rx/filter (ptk/type? component-swap-batch-done-type) stream))
(rx/filter (ptk/type? component-swap-layout-ids-type))
(rx/tap (fn [ev]
(vswap! layout-acc into (:ids (deref ev)))))
(rx/ignore))
(rx/concat
swap-source
(rx/of (ptk/data-event component-swap-batch-done-type {}))))
(rx/defer
#(rx/of (ptk/data-event :layout/update {:ids @layout-acc}))))))
(defn component-swap
"Swaps a component with another one"
[shape file-id id-new-component keep-touched?]
(dm/assert! (uuid? id-new-component))
(dm/assert! (uuid? file-id))
(ptk/reify ::component-swap
ptk/WatchEvent
(watch [it state _]
;; First delete shapes so we have space in the layout otherwise we can have problems
;; in the grid creating new rows/columns to make space
(let [libraries (dsh/lookup-libraries state)
page (dsh/lookup-page state)
objects (:objects page)
parent (get objects (:parent-id shape))
"Swaps a component with another one.
ldata (dsh/lookup-file-data state file-id)
orig-shapes (when keep-touched? (cfh/get-children-with-self objects (:id shape)))
Optional `opts`:
- `:skip-select?` — do not emit `select-shape` after the swap (batch callers
restore selection once).
- `:skip-layout?` — do not emit `:layout/update`; emit a companion
`component-swap-layout-ids-type` DataEvent instead so the batch can
coalesce one layout update."
([shape file-id id-new-component keep-touched?]
(component-swap shape file-id id-new-component keep-touched? nil))
([shape file-id id-new-component keep-touched?
{:keys [skip-select? skip-layout?] :or {skip-select? false skip-layout? false}}]
(dm/assert! (uuid? id-new-component))
(dm/assert! (uuid? file-id))
(ptk/reify ::component-swap
ptk/WatchEvent
(watch [it state _]
;; First delete shapes so we have space in the layout otherwise we can have problems
;; in the grid creating new rows/columns to make space
(let [libraries (dsh/lookup-libraries state)
page (dsh/lookup-page state)
objects (:objects page)]
;; Re-read from live state: sibling swaps / layout can move this head.
;; Gone from objects → no-op (do not swap a closed-over stale shape).
(if-let [shape (get objects (:id shape))]
(let [parent (get objects (:parent-id shape))
;; If the target parent is a grid layout we need to pass the target cell
target-cell (when (ctsl/grid-layout? parent)
(ctsl/get-cell-by-shape-id parent (:id shape)))
ldata (dsh/lookup-file-data state file-id)
orig-shapes (when keep-touched? (cfh/get-children-with-self objects (:id shape)))
index (find-shape-index objects (:parent-id shape) (:id shape))
;; If the target parent is a grid layout we need to pass the target cell
target-cell (when (ctsl/grid-layout? parent)
(ctsl/get-cell-by-shape-id parent (:id shape)))
;; Store the properties that need to be maintained when the component is swapped
keep-props-values (select-keys shape ctk/swap-keep-attrs)
index (find-shape-index objects (:parent-id shape) (:id shape))
undo-id (js/Symbol)
undo-group (uuid/next)
;; Store the properties that need to be maintained when the component is swapped
keep-props-values (select-keys shape ctk/swap-keep-attrs)
[new-shape all-parents changes]
(-> (pcb/empty-changes it (:id page))
(pcb/set-undo-group undo-group)
(cll/generate-component-swap objects shape ldata page libraries id-new-component
index target-cell keep-props-values keep-touched?))
undo-id (js/Symbol)
undo-group (uuid/next)
updated-objects (pcb/get-objects changes)
new-children-ids (cfh/get-children-ids-with-self updated-objects (:id new-shape))
new-text-ids (->> new-children-ids
(keep (fn [id]
(when-let [child (get updated-objects id)]
(when (and (cfh/text-shape? child)
(not= :fixed (:grow-type child)))
id))))
(vec))
[new-shape all-parents changes]
(-> (pcb/empty-changes it (:id page))
(pcb/set-undo-group undo-group)
(cll/generate-component-swap objects shape ldata page libraries id-new-component
index target-cell keep-props-values keep-touched?))
[changes parents-of-swapped]
(if keep-touched?
(clv/generate-keep-touched changes new-shape shape orig-shapes page libraries ldata)
[changes []])
update-layout-ids (concat all-parents parents-of-swapped new-children-ids)]
(rx/of
(dwu/start-undo-transaction undo-id)
(dch/commit-changes changes)
(dwtp/propagate-workspace-tokens) ;; Make the new instance get the token values from the current file, not from the component's library
(when (and (features/active-feature? state "render-wasm/v1")
(seq new-text-ids))
(dwwt/resize-wasm-text-all new-text-ids {:skip-component-sync? true}))
(ptk/data-event :layout/update {:ids update-layout-ids :undo-group undo-group})
(dwu/commit-undo-transaction undo-id)
(dws/select-shape (:id new-shape) false))))))
updated-objects (pcb/get-objects changes)
new-children-ids (cfh/get-children-ids-with-self updated-objects (:id new-shape))
new-text-ids (->> new-children-ids
(keep (fn [id]
(when-let [child (get updated-objects id)]
(when (and (cfh/text-shape? child)
(not= :fixed (:grow-type child)))
id))))
(vec))
[changes parents-of-swapped]
(if keep-touched?
(clv/generate-keep-touched changes new-shape shape orig-shapes page libraries ldata)
[changes []])
update-layout-ids (concat all-parents parents-of-swapped new-children-ids)]
(cond-> (rx/of
(dwu/start-undo-transaction undo-id)
(dch/commit-changes changes)
;; Make the new instance get the token values from the current file,
;; not from the component's library
(dwtp/propagate-workspace-tokens)
(when (and (features/active-feature? state "render-wasm/v1")
(seq new-text-ids))
(dwwt/resize-wasm-text-all new-text-ids {:skip-component-sync? true})))
(not skip-layout?)
(rx/concat (rx/of (ptk/data-event :layout/update
{:ids update-layout-ids :undo-group undo-group})))
skip-layout?
(rx/concat (rx/of (ptk/data-event component-swap-layout-ids-type
{:ids update-layout-ids})))
true
(rx/concat (rx/of (dwu/commit-undo-transaction undo-id)))
(not skip-select?)
(rx/concat (rx/of (dws/select-shape (:id new-shape) false)))))
(rx/empty)))))))
(defn component-multi-swap
"Swaps several components with another one"
@ -1131,17 +1187,21 @@
{::ev/name "component-swap"})
ptk/WatchEvent
(watch [_ state _]
(let [undo-id (js/Symbol)]
(watch [_ state stream]
(let [undo-id (js/Symbol)
swap-opts {:skip-select? true :skip-layout? true}]
(log/info :msg "COMPONENT-SWAP"
:file (debug-pretty-file file-id state)
:id-new-component id-new-component
:undo-id undo-id)
(rx/concat
(rx/of (dwu/start-undo-transaction undo-id))
(rx/map #(component-swap % file-id id-new-component false) (rx/from shapes))
(rx/of (dwu/commit-undo-transaction undo-id))
(rx/of (dwsp/open-specialized-panel :component-swap)))))))
(coalesce-skipped-layout
stream
(->> (rx/from shapes)
(rx/map #(component-swap % file-id id-new-component false swap-opts))))
(rx/of (dwu/commit-undo-transaction undo-id)
(dwsp/open-specialized-panel :component-swap)))))))
(def valid-asset-types
#{:colors :components :typographies})

View File

@ -780,60 +780,72 @@
(rx/of (combine-as-variants selected options))))))
(defn- variant-switch
"Switch the shape (that must be a variant copy head) for the closest one with the property value passed as parameter"
[shape {:keys [pos val] :as params}]
"Switch the shape (that must be a variant copy head) for the closest one with the property value passed as parameter.
`params` may include `:swap-opts` forwarded to `component-swap` (e.g.
`{:skip-select? true :skip-layout? true}` for a multi batch)."
[shape {:keys [pos val swap-opts] :as params}]
(ptk/reify ::variant-switch
ptk/WatchEvent
(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]))
(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))
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)
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))
(let [objects (dsh/lookup-page-objects state)]
;; Sibling swaps / layout can move this head before our turn.
;; Gone from objects → no-op (do not switch a closed-over stale shape).
(if-let [shape (get objects (:id shape))]
(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
(if (not= val (dm/get-in component [:variant-properties pos :value]))
(let [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))
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)
shape-parents (cfh/get-parents-with-self 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)]
{: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)))))))))))))
;; If there is no nearest-comp, do nothing
(if nearest-comp
(if comps-nesting-loop?
(do
(on-error)
(rx/empty))
(rx/of
(dwl/component-swap shape (:component-file shape) (:id nearest-comp) true swap-opts)
(ev/event (-> {::ev/name "variant-switch" ::ev/origin "workspace:design-tab"}
(merge (meta it))))))
(rx/empty)))
(rx/empty)))
(rx/empty))))))
(defn variants-switch
"Switch each shape (that must be a variant copy head) for the closest one with the property value passed as parameter"
[{:keys [shapes] :as params}]
(ptk/reify ::variants-switch
ptk/WatchEvent
(watch [it _ _]
(let [ids (into (d/ordered-set) d/xf:map-id shapes)
undo-id (js/Symbol)]
(watch [it _ stream]
(let [ids (into (d/ordered-set) d/xf:map-id shapes)
undo-id (js/Symbol)
swap-opts {:skip-select? true :skip-layout? true}]
(rx/concat
(rx/of (dwu/start-undo-transaction undo-id))
(->> (rx/from shapes)
(rx/map (fn [data]
(-> (variant-switch data params)
(with-meta (meta it))))))
(dwl/coalesce-skipped-layout
stream
(->> (rx/from shapes)
(rx/map (fn [shape]
(-> (variant-switch shape (assoc params :swap-opts swap-opts))
(with-meta (meta it)))))))
(rx/of (dwu/commit-undo-transaction undo-id)
(dws/select-shapes ids)))))))

View File

@ -0,0 +1,254 @@
;; This Source Code Form is subject to the terms of the Mozilla Public
;; License, v. 2.0. If a copy of the MPL was not distributed with this
;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
;;
;; Copyright (c) KALEIDOS SUBSIDIARY SL
(ns frontend-tests.logic.multi-variant-switch-test
"Regression tests for #12829: multi-select variants-switch /
component-multi-swap must not emit one select-shape per copy, and must
coalesce skipped per-swap layout into one :layout/update."
(:require
[app.common.data :as d]
[app.common.test-helpers.components :as cthc]
[app.common.test-helpers.compositions :as ctho]
[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 cthv]
[app.common.uuid :as uuid]
[app.main.data.workspace.libraries :as dwl]
[app.main.data.workspace.selection :as-alias dws]
[app.main.data.workspace.variants :as dwv]
[beicon.v2.core :as rx]
[cljs.test :as t :include-macros true]
[frontend-tests.helpers.pages :as thp]
[frontend-tests.helpers.state :as ths]
[potok.v2.core :as ptk]))
(t/use-fixtures :each
{:before thp/reset-idmap!})
(def ^:private fill-value1 "#2152e5")
(def ^:private fill-value2 "#e52152")
(defn- count-events
"Atom counting how many events of `type` land on the store input-stream
(nested WatchEvents only appear there — not on a bare ptk/watch of
variants-switch)."
[store type]
(let [counter (atom 0)]
(->> (ptk/input-stream store)
(rx/filter (ptk/type? type))
(rx/tap (fn [_] (swap! counter inc)))
(rx/subs! (fn [_] nil)))
counter))
(defn- collect-layout-updates
"Atom accumulating deref'd `:layout/update` payloads from the store
input-stream (same subscribe-before-emit pattern as `count-events`)."
[store]
(let [acc (atom [])]
(->> (ptk/input-stream store)
(rx/filter (ptk/type? :layout/update))
(rx/tap #(swap! acc conj (deref %)))
(rx/subs! (fn [_] nil)))
acc))
(defn- assert-coalesced-layout
[layout-updates flex-board-id]
(t/is (= 1 (count @layout-updates))
(str "expected one coalesced :layout/update; got "
(count @layout-updates)))
(let [ids (set (:ids (first @layout-updates)))]
(t/is (seq ids)
"coalesced :layout/update must carry non-empty :ids")
(t/is (contains? ids flex-board-id)
(str "coalesced :ids must include flex board; got " ids))))
(defn- fill-color
[shape]
(-> shape :fills first :fill-color))
(defn- setup-flex-variant-file
"Two variant members with distinct fills and sizes."
[]
(-> (cthf/sample-file :file1)
(cthv/add-variant :v01 :c01 :m01 :c02 :m02
:variant1-params {:fills (cths/sample-fills-color
:fill-color fill-value1)
:width 40
:height 40}
:variant2-params {:fills (cths/sample-fills-color
:fill-color fill-value2)
:width 50
:height 50})
(ctho/add-frame :flex-board
:name "Toggle Group"
:layout :flex
:layout-flex-dir :row
:width 400
:height 80)))
(defn- setup-flex-multi-copies
"Three Value1 copies as children of a flex board."
[]
(-> (setup-flex-variant-file)
(cthc/instantiate-component :c01 :copy1 :parent-label :flex-board)
(cthc/instantiate-component :c01 :copy2 :parent-label :flex-board)
(cthc/instantiate-component :c01 :copy3 :parent-label :flex-board)))
(defn- setup-flex-mixed-copies
"Two Value1 copies and one already on Value2 (no-op on switch to Value2)."
[]
(-> (setup-flex-variant-file)
(cthc/instantiate-component :c01 :copy1 :parent-label :flex-board)
(cthc/instantiate-component :c01 :copy2 :parent-label :flex-board)
(cthc/instantiate-component :c02 :copy3 :parent-label :flex-board)))
(t/deftest multi-variants-switch-emits-no-intermediate-select-shape
(t/async
done
(let [file (setup-flex-multi-copies)
store (ths/setup-store file)
copy1 (cths/get-shape file :copy1)
copy2 (cths/get-shape file :copy2)
copy3 (cths/get-shape file :copy3)
flex-board-id (:id (cths/get-shape file :flex-board))
target-comp-id (cthi/id :c02)
copy-ids (d/ordered-set (:id copy1) (:id copy2) (:id copy3))
select-shape-n (count-events store ::dws/select-shape)
layout-updates (collect-layout-updates store)
events
[(dwv/variants-switch {:shapes [copy1 copy2 copy3]
:pos 0
:val "Value2"})]]
(ths/run-store
store done events
(fn [new-state]
(let [file' (ths/get-file-from-state new-state)
copy1' (cths/get-shape file' :copy1)
copy2' (cths/get-shape file' :copy2)
copy3' (cths/get-shape file' :copy3)
selected (get-in new-state [:workspace-local :selected])]
(t/is (zero? @select-shape-n)
(str "multi variants-switch must not emit intermediate "
"select-shape; got " @select-shape-n))
(assert-coalesced-layout layout-updates flex-board-id)
(t/is (= target-comp-id (:component-id copy1')))
(t/is (= target-comp-id (:component-id copy2')))
(t/is (= target-comp-id (:component-id copy3')))
(t/is (= fill-value2 (fill-color copy1')))
(t/is (= fill-value2 (fill-color copy2')))
(t/is (= fill-value2 (fill-color copy3')))
(t/is (= (set copy-ids) selected))))))))
;; Done-marker collection must not hang when some selected copies already
;; have the target value (no companion from those no-ops).
(t/deftest multi-variants-switch-coalesces-layout-when-some-noop
(t/async
done
(let [file (setup-flex-mixed-copies)
store (ths/setup-store file)
copy1 (cths/get-shape file :copy1)
copy2 (cths/get-shape file :copy2)
copy3 (cths/get-shape file :copy3)
flex-board-id (:id (cths/get-shape file :flex-board))
target-comp-id (cthi/id :c02)
copy-ids (d/ordered-set (:id copy1) (:id copy2) (:id copy3))
select-shape-n (count-events store ::dws/select-shape)
layout-updates (collect-layout-updates store)
events
[(dwv/variants-switch {:shapes [copy1 copy2 copy3]
:pos 0
:val "Value2"})]]
(ths/run-store
store done events
(fn [new-state]
(let [file' (ths/get-file-from-state new-state)
copy1' (cths/get-shape file' :copy1)
copy2' (cths/get-shape file' :copy2)
copy3' (cths/get-shape file' :copy3)
selected (get-in new-state [:workspace-local :selected])]
(t/is (zero? @select-shape-n))
(assert-coalesced-layout layout-updates flex-board-id)
(t/is (= target-comp-id (:component-id copy1')))
(t/is (= target-comp-id (:component-id copy2')))
(t/is (= target-comp-id (:component-id copy3')))
(t/is (= fill-value2 (fill-color copy1')))
(t/is (= fill-value2 (fill-color copy2')))
(t/is (= fill-value2 (fill-color copy3')))
(t/is (= (set copy-ids) selected))))))))
(t/deftest component-multi-swap-skips-select-and-coalesces-layout
(t/async
done
(let [file (setup-flex-multi-copies)
store (ths/setup-store file)
copy1 (cths/get-shape file :copy1)
copy2 (cths/get-shape file :copy2)
copy3 (cths/get-shape file :copy3)
flex-board-id (:id (cths/get-shape file :flex-board))
target-comp-id (cthi/id :c02)
select-shape-n (count-events store ::dws/select-shape)
layout-updates (collect-layout-updates store)
events
[(dwl/component-multi-swap [copy1 copy2 copy3]
(:id file)
target-comp-id)]]
(ths/run-store
store done events
(fn [new-state]
(let [file' (ths/get-file-from-state new-state)
copy1' (cths/get-shape file' :copy1)
copy2' (cths/get-shape file' :copy2)
copy3' (cths/get-shape file' :copy3)]
(t/is (zero? @select-shape-n)
(str "component-multi-swap must not emit intermediate "
"select-shape; got " @select-shape-n))
(assert-coalesced-layout layout-updates flex-board-id)
(t/is (= target-comp-id (:component-id copy1')))
(t/is (= target-comp-id (:component-id copy2')))
(t/is (= target-comp-id (:component-id copy3')))
(t/is (= fill-value2 (fill-color copy1')))
(t/is (= fill-value2 (fill-color copy2')))
(t/is (= fill-value2 (fill-color copy3')))))))))
(t/deftest component-swap-noop-when-shape-missing
(t/async
done
(let [file (setup-flex-multi-copies)
store (ths/setup-store file)
copy1 (cths/get-shape file :copy1)
ghost (assoc copy1 :id (uuid/next))
before (ths/get-file-from-state @store)]
(ths/run-store
store done
[(dwl/component-swap ghost (:id file) (cthi/id :c02) true)]
(fn [new-state]
(t/is (= before (ths/get-file-from-state new-state))
"component-swap must no-op when shape id is absent from objects"))))))
(t/deftest variant-switch-noop-when-shape-missing
(t/async
done
(let [file (setup-flex-multi-copies)
store (ths/setup-store file)
copy1 (cths/get-shape file :copy1)
ghost (assoc copy1 :id (uuid/next))
before (ths/get-file-from-state @store)]
(ths/run-store
store done
[(#'dwv/variant-switch ghost {:pos 0 :val "Value2"})]
(fn [new-state]
(t/is (= before (ths/get-file-from-state new-state))
"variant-switch must no-op when shape id is absent from objects"))))))

View File

@ -52,6 +52,7 @@
[frontend-tests.logic.copying-and-duplicating-test]
[frontend-tests.logic.frame-guides-test]
[frontend-tests.logic.groups-test]
[frontend-tests.logic.multi-variant-switch-test]
[frontend-tests.logic.nudge-selected-shapes-test]
[frontend-tests.logic.pasting-in-containers-test]
[frontend-tests.logic.path-actions-test]
@ -204,6 +205,7 @@
'frontend-tests.logic.copying-and-duplicating-test
'frontend-tests.logic.frame-guides-test
'frontend-tests.logic.groups-test
'frontend-tests.logic.multi-variant-switch-test
'frontend-tests.logic.nudge-selected-shapes-test
'frontend-tests.logic.path-actions-test
'frontend-tests.logic.path-clipboard-test