From 4fb0ea9eeeeb09bfb8edc728398eb1125d4da74f Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Tue, 10 Nov 2026 09:16:08 +0100 Subject: [PATCH] :bug: Give unique values to same-name components combined as variants (#12881) When components whose name matches the variant container are moved into it, removing the container name leaves an empty value. Combining several components named the same produced "Value 1" for the first variant and empty values for the rest, so variants shared the same combination. Assign the first unused "Value N" to the last property when all computed values are blank, tracking values given within the same batch. Combine moves the remaining mains into the container one at a time, in the combine order, so the generated values follow that order instead of the layer order. Closes #12878 --- .../app/common/logic/variant_properties.cljc | 63 +++++++++----- .../common_tests/logic/variants_test.cljc | 36 ++++++++ .../src/app/main/data/workspace/variants.cljs | 11 ++- .../data/workspace_variants_test.cljs | 53 +++++++++++ .../src/tests/variants.test.ts | 87 +++++++++++++++++++ 5 files changed, 225 insertions(+), 25 deletions(-) diff --git a/common/src/app/common/logic/variant_properties.cljc b/common/src/app/common/logic/variant_properties.cljc index 3e950a81b8..02490a3c81 100644 --- a/common/src/app/common/logic/variant_properties.cljc +++ b/common/src/app/common/logic/variant_properties.cljc @@ -196,6 +196,19 @@ shape-name (ctv/remove-prefix (:name shape) container-name)] (ctv/path-to-properties shape-name base-properties min-props))) +(defn- fill-blank-properties + "When every value in `props` is blank, set the last one to the first + `Value N` not contained in `used-values`, so the variant does not end up + with an empty combination." + [props used-values] + (if (and (seq props) (every? (comp str/blank? :value) props)) + (let [value (->> (iterate inc 1) + (map #(str ctv/value-prefix %)) + (remove used-values) + first)] + (assoc-in (vec props) [(dec (count props)) :value] value)) + props)) + (defn generate-make-shapes-variant [changes shapes variant-container] @@ -244,24 +257,32 @@ changes (pcb/update-shapes changes (map :id shapes) #(assoc % :variant-id variant-id - :name (:name variant-container)))] - (reduce - (fn [changes shape] - (let [component (ctcl/get-component data (:component-id shape) true)] - (if (or (zero? num-shapes) ;; do nothing if there are no shapes - (and (= variant-id (:variant-id shape)) ;; or we are only moving the shape inside its parent (it is - (not (:deleted component)))) ;; the same parent and the component isn't deleted) - changes - (let [props (create-new-properties shape total-props) - variant-name (ctv/properties-to-name props)] - (-> (pcb/update-component changes - (:component-id shape) - #(assoc % :variant-id variant-id - :variant-properties props - :name cname - :path cpath) - {:apply-changes-local-library? true}) - (pcb/update-shapes [(:id shape)] - #(assoc % :variant-name variant-name))))))) - changes - shapes))) + :name (:name variant-container))) + + ;; Property values already present in the container's variants + used-values (->> (cfv/extract-properties-values data objects variant-id) + (mapcat :value) + set)] + (first + (reduce + (fn [[changes used-values] shape] + (let [component (ctcl/get-component data (:component-id shape) true)] + (if (or (zero? num-shapes) ;; do nothing if there are no shapes + (and (= variant-id (:variant-id shape)) ;; or we are only moving the shape inside its parent (it is + (not (:deleted component)))) ;; the same parent and the component isn't deleted) + [changes used-values] + (let [props (-> (create-new-properties shape total-props) + (fill-blank-properties used-values)) + variant-name (ctv/properties-to-name props)] + [(-> (pcb/update-component changes + (:component-id shape) + #(assoc % :variant-id variant-id + :variant-properties props + :name cname + :path cpath) + {:apply-changes-local-library? true}) + (pcb/update-shapes [(:id shape)] + #(assoc % :variant-name variant-name))) + (into used-values (map :value) props)])))) + [changes used-values] + shapes)))) diff --git a/common/test/common_tests/logic/variants_test.cljc b/common/test/common_tests/logic/variants_test.cljc index 0323df3afb..8162cf5598 100644 --- a/common/test/common_tests/logic/variants_test.cljc +++ b/common/test/common_tests/logic/variants_test.cljc @@ -412,6 +412,42 @@ (t/is (contains? values "Bigred")))) +(t/deftest test-make-shapes-variant-same-name-components-get-unique-values + ;; Mirrors combine-as-variants with components that share the container + ;; name: the first main defines the container, the rest are moved into it. + (let [file (-> (thf/sample-file :file1) + (tho/add-frame :v01 {:name "Tag"}) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:parent-label :v01 :name "Tag"}) + (tho/add-simple-component :c02 :m02 :r02 + :root-params {:name "Tag"}) + (tho/add-simple-component :c03 :m03 :r03 + :root-params {:name "Tag"})) + page (thf/current-page file) + define (-> (pcb/empty-changes nil (:id page)) + (pcb/with-objects (:objects page)) + (pcb/with-library-data (:data file)) + (clv/generate-define-variant-container + (thi/id :v01) (thi/id :m01) (thi/id :c01) "Tag")) + file (thf/apply-changes file define) + + page (thf/current-page file) + container (ths/get-shape file :v01) + shapes [(ths/get-shape file :m02) (ths/get-shape file :m03)] + changes (-> (pcb/empty-changes nil) + (pcb/with-page-id (:id page)) + (pcb/with-library-data (:data file)) + (pcb/with-objects (:objects page)) + (clvp/generate-make-shapes-variant shapes container) + (pcb/change-parent (:id container) shapes 0)) + file' (thf/apply-changes file changes) + values (mapv #(-> (thc/get-component file' %) :variant-properties first :value) + [:c01 :c02 :c03])] + + (t/is (= ["Value 1" "Value 2" "Value 3"] values)) + (t/is (= "Value 2" (:variant-name (ths/get-shape file' :m02)))) + (t/is (= "Value 3" (:variant-name (ths/get-shape file' :m03)))))) + (t/deftest test-duplicate-variant-container (let [;; ==== Setup diff --git a/frontend/src/app/main/data/workspace/variants.cljs b/frontend/src/app/main/data/workspace/variants.cljs index 46730ee112..49eb435f34 100644 --- a/frontend/src/app/main/data/workspace/variants.cljs +++ b/frontend/src/app/main/data/workspace/variants.cljs @@ -697,10 +697,13 @@ (transform-in-variant-stream state (first ids) variant-id delta prefix add-wrapper? false false current-page) - (rx/of (dwsh/relocate-shapes (into #{} (rest ids)) variant-id 0) - ;; relocate-shapes inserts the shapes in layer-tree - ;; order, so restore the intended order afterwards - (order-variant-children variant-id ids) + ;; One relocation per main, in `ids` order, so the + ;; generated property values follow the combine order + (rx/from (map #(dwsh/relocate-shapes #{%} variant-id 0) (rest ids))) + + ;; Each relocation inserts at index 0, so restore the + ;; intended order afterwards + (rx/of (order-variant-children variant-id ids) (dwsh/update-shapes ids #(-> % (assoc :constraints-h :left) (assoc :constraints-v :top) diff --git a/frontend/test/frontend_tests/data/workspace_variants_test.cljs b/frontend/test/frontend_tests/data/workspace_variants_test.cljs index 20c2fa7acf..d596add6c2 100644 --- a/frontend/test/frontend_tests/data/workspace_variants_test.cljs +++ b/frontend/test/frontend_tests/data/workspace_variants_test.cljs @@ -190,6 +190,16 @@ (tho/add-simple-component :c02 :m02 :r02 :root-params {:name "UI / Beta" :x 150 :y 0 :width 100 :height 80}))) +(defn- three-same-name-mains-file + [] + (-> (cthf/sample-file :file1) + (tho/add-simple-component :c01 :m01 :r01 + :root-params {:name "Tag" :x 0 :y 0 :width 100 :height 80}) + (tho/add-simple-component :c02 :m02 :r02 + :root-params {:name "Tag" :x 150 :y 0 :width 100 :height 80}) + (tho/add-simple-component :c03 :m03 :r03 + :root-params {:name "Tag" :x 300 :y 0 :width 100 :height 80}))) + (defn- path-named-main-file [] (-> (cthf/sample-file :file1) @@ -524,6 +534,49 @@ (t/is (= variant-id (:parent-id m01') (:parent-id m02'))))) after-commits-idle)))) +(t/deftest combine-as-variants-same-name-gives-unique-values + (t/async + done + (let [file (three-same-name-mains-file) + ids [(cthi/id :m01) (cthi/id :m02) (cthi/id :m03)] + variant-id (uuid/next) + store (ths/setup-store file)] + (ths/run-store + store + done + [(dwv/combine-as-variants ids {:variant-id variant-id :trigger "test"})] + (fn [state] + (let [file' (ths/get-file-from-state state) + values (mapv #(-> (cthc/get-component file' %) :variant-properties first :value) + [:c01 :c02 :c03])] + (cthf/validate-file! file') + (t/is (= ["Value 1" "Value 2" "Value 3"] values)))) + after-commits-idle)))) + +(t/deftest combine-as-variants-same-name-values-follow-combine-order + ;; The combine order differs from the layer order of the mains + (t/async + done + (let [file (three-same-name-mains-file) + ids [(cthi/id :m03) (cthi/id :m01) (cthi/id :m02)] + variant-id (uuid/next) + store (ths/setup-store file)] + (ths/run-store + store + done + [(dwv/combine-as-variants ids {:variant-id variant-id :trigger "test"})] + (fn [state] + (let [file' (ths/get-file-from-state state) + objects (page-objects file') + values (mapv #(-> (cthc/get-component file' %) :variant-properties first :value) + [:c03 :c01 :c02]) + ordered (->> (cfv/find-variant-components (:data file') objects variant-id) + (mapv #(-> % :variant-properties first :value)))] + (cthf/validate-file! file') + (t/is (= ["Value 1" "Value 2" "Value 3"] values)) + (t/is (= ["Value 1" "Value 2" "Value 3"] ordered)))) + after-commits-idle)))) + (t/deftest combine-as-variants-from-another-page-validates-after-every-commit (t/async done diff --git a/plugins/apps/plugin-api-test-suite/src/tests/variants.test.ts b/plugins/apps/plugin-api-test-suite/src/tests/variants.test.ts index 451e75f91b..ace7b44d32 100644 --- a/plugins/apps/plugin-api-test-suite/src/tests/variants.test.ts +++ b/plugins/apps/plugin-api-test-suite/src/tests/variants.test.ts @@ -41,6 +41,27 @@ function sameIds(a: string[], b: string[]): boolean { return a.length === b.length && a.every((id, i) => id === b[i]); } +// Value of the first property for each variant component, in variant order. +function firstPropertyValues(container: VariantContainer): string[] { + const variants = container.variants; + if (!variants) return []; + const property = variants.properties[0]; + return variants + .variantComponents() + .map((c) => (c as LibraryVariantComponent).variantProps[property]); +} + +function variantValueOf( + container: VariantContainer, + componentId: string, +): string | undefined { + const variants = container.variants; + const component = variants + ?.variantComponents() + .find((c) => c.id === componentId) as LibraryVariantComponent | undefined; + return component?.variantProps[variants!.properties[0]]; +} + // Polls until the condition holds. On timeout it returns normally so the // assertion that follows reports the actual mismatch. async function waitFor( @@ -134,6 +155,72 @@ describe('Variants', () => { expect(variantComponentIds(container)).toEqual(expected); }); + // The components built here all share the name "Rectangle". Combining + // components with the same name must give every variant a unique, + // non-empty value, numbered in the combine order (#12878). + test('createVariantFromComponents gives same-name components unique values in input order', async (ctx) => { + const a = componentWithMain(ctx); + const b = componentWithMain(ctx); + const c = componentWithMain(ctx); + const container = ctx.penpot.createVariantFromComponents([ + c.main, + a.main, + b.main, + ]); + const expected = ['Value 1', 'Value 2', 'Value 3']; + await waitFor(() => sameIds(firstPropertyValues(container), expected)); + expect(firstPropertyValues(container)).toEqual(expected); + expect(variantComponentIds(container)).toEqual([ + c.comp.id, + a.comp.id, + b.comp.id, + ]); + }); + + test('combineAsVariants gives same-name components unique values in combine order', async (ctx) => { + const a = componentWithMain(ctx); + const b = componentWithMain(ctx); + const c = componentWithMain(ctx); + const container = a.main.combineAsVariants([b.main.id, c.main.id]); + const expected = ['Value 1', 'Value 2', 'Value 3']; + await waitFor(() => sameIds(firstPropertyValues(container), expected)); + expect(firstPropertyValues(container)).toEqual(expected); + expect(variantComponentIds(container)).toEqual([ + a.comp.id, + b.comp.id, + c.comp.id, + ]); + }); + + test('a same-name main moved into a variant container takes the first free value', async (ctx) => { + const a = componentWithMain(ctx); + const b = componentWithMain(ctx); + const c = componentWithMain(ctx); + const container = ctx.penpot.createVariantFromComponents([ + a.main, + b.main, + c.main, + ]); + await waitFor(() => firstPropertyValues(container).includes('Value 2')); + + // Free "Value 2" so the moved main has a gap to fill. + const withValue2 = container + .variants!.variantComponents() + .find( + (comp) => variantValueOf(container, comp.id) === 'Value 2', + ) as LibraryVariantComponent; + withValue2.setVariantProperty(0, 'Renamed'); + await waitFor(() => !firstPropertyValues(container).includes('Value 2')); + + const d = componentWithMain(ctx); + container.appendChild(d.main); + await waitFor(() => variantValueOf(container, d.comp.id) !== undefined); + expect(variantValueOf(container, d.comp.id)).toBe('Value 2'); + + const values = firstPropertyValues(container); + expect(new Set(values).size).toBe(values.length); + }); + test('variant component exposes variant props and Variants', (ctx) => { const rect = ctx.penpot.createRectangle(); ctx.board.appendChild(rect);