From 7666d7d50015f4bf3cd0b28e0470dadf66d37566 Mon Sep 17 00:00:00 2001 From: Pablo Alba Date: Tue, 3 Nov 2026 10:42:59 +0100 Subject: [PATCH] :bug: Fix Creating or combining a variant set renames the component first --- common/src/app/common/logic/variants.cljc | 18 ++ .../common_tests/logic/variants_test.cljc | 44 +++++ .../src/app/main/data/workspace/variants.cljs | 26 +-- .../data/workspace_variants_test.cljs | 160 ++++++++++++++++-- 4 files changed, 214 insertions(+), 34 deletions(-) diff --git a/common/src/app/common/logic/variants.cljc b/common/src/app/common/logic/variants.cljc index b9c2037788..9a78d6cc20 100644 --- a/common/src/app/common/logic/variants.cljc +++ b/common/src/app/common/logic/variants.cljc @@ -38,6 +38,24 @@ (pcb/update-shapes changes shape-ids #(assoc % :name stored-name)) components))) +(defn generate-define-variant-container + "One change vector. `name` is the string stored on the board. + The component path and name are `split-group-name` of that string. + Library data is refreshed before the first property is added." + [changes variant-id main-id component-id name] + (let [[path item-name] (cpn/split-group-name name)] + (-> changes + (pcb/update-shapes [variant-id] + #(assoc % :is-variant-container true :name name)) + (pcb/update-shapes [main-id] + #(assoc % :name name :variant-id variant-id)) + (pcb/update-component component-id + #(assoc % :variant-id variant-id + :path path + :name item-name) + {:apply-changes-local-library? true}) + (clvp/generate-add-new-property variant-id {:fill-values? true})))) + (defn generate-add-new-variant [changes shape variant-id new-component-id new-shape-id prop-num] (let [data (pcb/get-library-data changes) diff --git a/common/test/common_tests/logic/variants_test.cljc b/common/test/common_tests/logic/variants_test.cljc index 75b292598f..0323df3afb 100644 --- a/common/test/common_tests/logic/variants_test.cljc +++ b/common/test/common_tests/logic/variants_test.cljc @@ -14,10 +14,12 @@ [app.common.logic.variants :as clv] [app.common.path-names :as cpn] [app.common.test-helpers.components :as thc] + [app.common.test-helpers.compositions :as tho] [app.common.test-helpers.files :as thf] [app.common.test-helpers.ids-map :as thi] [app.common.test-helpers.shapes :as ths] [app.common.test-helpers.variants :as thv] + [app.common.types.component :as ctk] [app.common.types.variant :as ctv] [clojure.test :as t] [cuerdas.core :as str])) @@ -566,6 +568,48 @@ (t/is (= (ctv/properties-to-name props') (:variant-name main'))) (t/is (= "active, crítica" (:variant-name main'))))) +(defn- post-artboard-alpha-file + "A normal frame holding one main named Alpha — before the container flag." + [] + (-> (thf/sample-file :file1) + (tho/add-frame :v01 {:name "Board"}) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:parent-label :v01 :name "Alpha"}))) + +(t/deftest test-define-variant-container-renames-component-in-same-vector + (let [file (post-artboard-alpha-file) + page (thf/current-page file) + variant-id (thi/id :v01) + main-id (thi/id :m01) + component-id (:id (thc/get-component file :c01)) + changes (-> (pcb/empty-changes nil (:id page)) + (pcb/with-objects (:objects page)) + (pcb/with-library-data (:data file)) + (clv/generate-define-variant-container + variant-id main-id component-id "Component")) + file' (thf/apply-changes file changes) + container' (ths/get-shape file' :v01) + main' (ths/get-shape file' :m01) + comp' (thc/get-component file' :c01) + restored (thf/apply-undo-changes file' changes) + container-rest (ths/get-shape restored :v01) + main-rest (ths/get-shape restored :m01) + comp-rest (thc/get-component restored :c01)] + + (t/is (ctk/is-variant-container? container')) + (t/is (= "Component" + (:name container') + (:name main') + (cpn/merge-path-item (:path comp') (:name comp')))) + (t/is (= variant-id (:variant-id main') (:variant-id comp'))) + (t/is (= (ctv/properties-to-name (:variant-properties comp')) + (:variant-name main'))) + (t/is (not (ctk/is-variant-container? container-rest))) + (t/is (= "Alpha" (:name main-rest))) + (t/is (= "Alpha" (cpn/merge-path-item (:path comp-rest) (:name comp-rest)))) + (t/is (nil? (:variant-id main-rest))) + (t/is (nil? (:variant-id comp-rest))))) + (defn- rename-variant-changes [file name] (let [page (thf/current-page file)] diff --git a/frontend/src/app/main/data/workspace/variants.cljs b/frontend/src/app/main/data/workspace/variants.cljs index 18ac285055..11aa37c85c 100644 --- a/frontend/src/app/main/data/workspace/variants.cljs +++ b/frontend/src/app/main/data/workspace/variants.cljs @@ -304,21 +304,6 @@ (dch/commit-changes changes) (dwu/commit-undo-transaction undo-id))))))))) -(defn generate-define-variant-container - "One commit that turns an existing frame into a valid variant set: - container flag and name, the main's variant-id, the component's - variant-id (refreshing library data), then the first property." - [changes variant-id main-id component-id name] - (-> changes - (pcb/update-shapes [variant-id] - #(assoc % :is-variant-container true :name name)) - (pcb/update-shapes [main-id] - #(assoc % :name name :variant-id variant-id)) - (pcb/update-component component-id - #(assoc % :variant-id variant-id) - {:apply-changes-local-library? true}) - (clvp/generate-add-new-property variant-id {:fill-values? true}))) - (defn- define-variant-container "Read page objects after the artboard exists, then commit the defining variant fields in one change." @@ -332,7 +317,8 @@ changes (-> (pcb/empty-changes it page-id) (pcb/with-objects objects) (pcb/with-library-data data) - (generate-define-variant-container variant-id main-id component-id name))] + (clv/generate-define-variant-container + variant-id main-id component-id name))] (rx/of (dch/commit-changes changes)))))) (defn- focus-property @@ -340,7 +326,10 @@ (ptk/reify ::focus-property ptk/EffectEvent (effect [_ _ _] - (dom/focus! (dom/get-element (str "variant-prop-" variant-id "-0")))))) + ;; Unit tests run without a DOM; skip rather than throw into the store. + (when (exists? js/document) + (some-> (dom/get-element (str "variant-prop-" variant-id "-0")) + (dom/focus!)))))) (defn- resposition-and-resize-variant "Resize the variant container, and move the shape (that is a variant) to the right" @@ -468,9 +457,6 @@ (rx/of (dwu/start-undo-transaction undo-id) - (when (not= name (:name main)) - (dwl/rename-component component-id name)) - ;; Create the frame while it is still a normal board (dwsh/create-artboard-from-shapes [main-instance-id] variant-id nil nil nil delta flex?) (cl/remove-all-fills variant-vec {:color clr/black :opacity 1}) diff --git a/frontend/test/frontend_tests/data/workspace_variants_test.cljs b/frontend/test/frontend_tests/data/workspace_variants_test.cljs index 44e68d6745..20c2fa7acf 100644 --- a/frontend/test/frontend_tests/data/workspace_variants_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_variants_test.cljs @@ -8,6 +8,7 @@ (:require [app.common.files.changes-builder :as pcb] [app.common.files.variant :as cfv] + [app.common.logic.variants :as clv] [app.common.path-names :as cpn] [app.common.test-helpers.components :as cthc] [app.common.test-helpers.compositions :as tho] @@ -181,6 +182,20 @@ (tho/add-simple-component :c02 :m02 :r02 :root-params {:name "Beta" :x 150 :y 0 :width 100 :height 80}))) +(defn- two-mains-shared-prefix-file + [] + (-> (cthf/sample-file :file1) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:name "UI / Alpha" :x 0 :y 0 :width 100 :height 80}) + (tho/add-simple-component :c02 :m02 :r02 + :root-params {:name "UI / Beta" :x 150 :y 0 :width 100 :height 80}))) + +(defn- path-named-main-file + [] + (-> (cthf/sample-file :file1) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:name "Group / Button"}))) + (defn- two-mains-on-page1-with-page2 "Two mains on :page1; the open page is :page2 (Assets combine while elsewhere)." [] @@ -201,6 +216,10 @@ (tho/add-simple-component :c01 :m01 :r01 :root-params {:parent-label :v01 :name "Set"}))) +(defn- component-full-name + [component] + (cpn/merge-path-item (:path component) (:name component))) + (defn- page-objects [file] (:objects (cthf/current-page file))) @@ -258,7 +277,7 @@ changes (-> (pcb/empty-changes nil (:id page)) (pcb/with-objects (:objects page)) (pcb/with-library-data (:data file)) - (dwv/generate-define-variant-container + (clv/generate-define-variant-container variant-id main-id component-id name)) file' (cthf/apply-changes file changes) container' (cths/get-shape file' :v01) @@ -271,7 +290,7 @@ (t/is (= variant-id (:variant-id comp'))) (t/is (some? (:variant-id comp'))) (t/is (= (ctv/properties-to-name (:variant-properties comp')) - (:variant-name main')))))) + (:variant-name main'))))) (t/deftest transform-in-variant-validates-after-every-commit (t/async @@ -320,6 +339,54 @@ (t/is (= variant-id (:variant-id comp'))))) after-commits-idle)))) +(t/deftest transform-in-variant-path-name-validates-after-every-commit + (t/async + done + (let [file (path-named-main-file) + main-id (cthi/id :m01) + variant-id (uuid/next) + snapshots (atom []) + store (ths/setup-store file)] + (watch-applied-commits store snapshots) + (ths/run-store + store + done + [(dwv/transform-in-variant main-id variant-id nil [] false true true)] + (fn [state] + (let [snaps @snapshots + flagged (first-flagged snaps variant-id) + file' (ths/get-file-from-state state) + objects' (page-objects file') + container' (get objects' variant-id) + main' (get objects' main-id) + comp' (cthc/get-component file' :c01) + before-flag (take-while + #(not (ctk/is-variant-container? + (get (page-objects (:file %)) variant-id))) + snaps)] + (assert-valid-snapshots snaps) + (t/is (some? flagged) "a commit must set :is-variant-container") + (doseq [snap before-flag] + (let [ffile (:file snap) + fmain (get (page-objects ffile) main-id) + fcomp (cthc/get-component ffile :c01)] + (t/is (= "Group / Button" (:name fmain))) + (t/is (= "Group / Button" (component-full-name fcomp))))) + (when flagged + (let [ffile (:file flagged) + fobjs (page-objects ffile) + fcont (get fobjs variant-id) + fmain (get fobjs main-id) + fcomp (cthc/get-component ffile :c01)] + (t/is (= "Group" (:name fcont) (:name fmain) (component-full-name fcomp))) + (t/is (= variant-id (:variant-id fmain) (:variant-id fcomp))) + (t/is (= (ctv/properties-to-name (:variant-properties fcomp)) + (:variant-name fmain))))) + (t/is (ctk/is-variant-container? container')) + (t/is (= "Group" (:name container') (:name main') (component-full-name comp'))) + (t/is (= variant-id (:variant-id main') (:variant-id comp'))))) + after-commits-idle)))) + (t/deftest add-new-variant-on-existing-set-still-validates (t/async done @@ -363,19 +430,37 @@ done [(dwv/combine-as-variants [id1 id2] {:variant-id variant-id :trigger "test"})] (fn [state] - (let [snaps @snapshots - file' (ths/get-file-from-state state) - objects (page-objects file') - container (get objects variant-id) - m01' (get objects id1) - m02' (get objects id2) - c01' (cthc/get-component file' :c01) - c02' (cthc/get-component file' :c02) - names1 (mapv :name (:variant-properties c01')) - names2 (mapv :name (:variant-properties c02')) - ordered (mapv :main-instance-id - (cfv/find-variant-components (:data file') objects variant-id))] + (let [snaps @snapshots + file' (ths/get-file-from-state state) + objects (page-objects file') + container (get objects variant-id) + m01' (get objects id1) + m02' (get objects id2) + c01' (cthc/get-component file' :c01) + c02' (cthc/get-component file' :c02) + names1 (mapv :name (:variant-properties c01')) + names2 (mapv :name (:variant-properties c02')) + ordered (mapv :main-instance-id + (cfv/find-variant-components (:data file') objects variant-id)) + before-flag (take-while + #(not (ctk/is-variant-container? + (get (page-objects (:file %)) variant-id))) + snaps) + flagged (first-flagged snaps variant-id)] (assert-valid-snapshots snaps) + (doseq [snap before-flag] + (let [ffile (:file snap) + fmain (get (page-objects ffile) id1) + fcomp (cthc/get-component ffile :c01)] + (t/is (= "Alpha" (:name fmain))) + (t/is (= "Alpha" (component-full-name fcomp))))) + (when flagged + (let [ffile (:file flagged) + fobjs (page-objects ffile) + fcont (get fobjs variant-id) + fmain (get fobjs id1) + fcomp (cthc/get-component ffile :c01)] + (t/is (= "Component" (:name fcont) (:name fmain) (component-full-name fcomp))))) (doseq [{:keys [file]} snaps] (let [objs (page-objects file) cont (get objs variant-id) @@ -392,6 +477,53 @@ (t/is (= variant-id (:parent-id m02'))))) after-commits-idle)))) +(t/deftest combine-as-variants-shared-prefix-validates-after-every-commit + (t/async + done + (let [file (two-mains-shared-prefix-file) + id1 (cthi/id :m01) + id2 (cthi/id :m02) + variant-id (uuid/next) + snapshots (atom []) + store (ths/setup-store file)] + (watch-applied-commits store snapshots) + (ths/run-store + store + done + [(dwv/combine-as-variants [id1 id2] {:variant-id variant-id :trigger "test"})] + (fn [state] + (let [snaps @snapshots + file' (ths/get-file-from-state state) + objects (page-objects file') + container (get objects variant-id) + m01' (get objects id1) + m02' (get objects id2) + c01' (cthc/get-component file' :c01) + c02' (cthc/get-component file' :c02) + flagged (first-flagged snaps variant-id)] + (assert-valid-snapshots snaps) + (doseq [{:keys [file]} snaps] + (let [objs (page-objects file) + cont (get objs variant-id) + kids (set (:shapes cont))] + (when (contains? kids id2) + (t/is (ctk/is-variant-container? cont) + "other mains must not enter the frame before it is a variant container")))) + (when flagged + (let [ffile (:file flagged) + fobjs (page-objects ffile) + fcont (get fobjs variant-id) + fmain (get fobjs id1) + fcomp (cthc/get-component ffile :c01)] + (t/is (= "UI" (:name fcont) (:name fmain) (component-full-name fcomp))))) + (t/is (ctk/is-variant-container? container)) + (t/is (= "UI" (:name container) (:name m01') (:name m02') + (component-full-name c01') (component-full-name c02'))) + (t/is (= variant-id (:variant-id m01') (:variant-id m02') + (:variant-id c01') (:variant-id c02'))) + (t/is (= variant-id (:parent-id m01') (:parent-id m02'))))) + after-commits-idle)))) + (t/deftest combine-as-variants-from-another-page-validates-after-every-commit (t/async done