From 8e306b8e24ee1b75fe9f8aa2eebcc669016eab29 Mon Sep 17 00:00:00 2001 From: Eva Marco Date: Tue, 29 Sep 2026 16:13:40 +0200 Subject: [PATCH] :wrench: Validate shapes after removing them from a variant generate-make-shapes-no-variant now validates the shapes and their old variant containers, and accepts a :skip-validation? option. generate-relocate calls it before moving the shapes, while they are still inside the container. Validating the container at that point reported :main-instance-not-a-variant on valid data. Relocate now passes :skip-validation? and validates at the end: the old and new variant containers, and the moved variants when they end up outside any component. Inside a component they are no longer roots, and the validator can't check them on their own. Add tests for the function and for moving a variant out of its container, with valid and corrupted data. AI-assisted-by: claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- common/src/app/common/logic/shapes.cljc | 24 ++++++- .../app/common/logic/variant_properties.cljc | 20 ++++-- .../logic/variant_properties_test.cljc | 64 ++++++++++++++++++- 3 files changed, 100 insertions(+), 8 deletions(-) diff --git a/common/src/app/common/logic/shapes.cljc b/common/src/app/common/logic/shapes.cljc index 020f92d0a8..79123a12a2 100644 --- a/common/src/app/common/logic/shapes.cljc +++ b/common/src/app/common/logic/shapes.cljc @@ -585,7 +585,18 @@ (conj to-delete (:id parent)) to-delete))) #{} - (remove #(= % parent-id) all-parents))] + (remove #(= % parent-id) all-parents)) + + ;; The old and new variant containers, and the moved variants when they + ;; end up outside any component (inside one, they are not roots anymore + ;; and can't be validated on their own) + variant-ids-to-validate + (cond-> (into #{} (keep :variant-id) variant-shapes) + (ctk/is-variant-container? parent) + (conj parent-id) + + (nil? (ctn/get-instance-root objects parent)) + (into (map :id) variant-shapes))] (-> changes ;; Remove layout-item properties and tokens when moving a shape outside a layout @@ -614,8 +625,9 @@ (pcb/update-shapes child-heads-ids #(assoc % :component-root true))) ;; Remove variant info and rename when moving outside a variant-container + ;; The shapes are still inside the container, so validate at the end (cond-> (not (ctk/is-variant-container? parent)) - (clvp/generate-make-shapes-no-variant variant-shapes)) + (clvp/generate-make-shapes-no-variant variant-shapes :skip-validation? true)) ;; Add variant info and rename when moving into a different variant-container (cond-> (ctk/is-variant-container? parent) @@ -698,7 +710,13 @@ ;; Remove parents when are a variant-container that becomes empty (cond-> (seq empty-variant-cont) - (#(second (generate-delete-shapes % empty-variant-cont {}))))))) + (#(second (generate-delete-shapes % empty-variant-cont {})))) + + ;; Validate the variants once the shapes are in their new parent + (cond-> (seq variant-ids-to-validate) + (pcb/validate-shapes (pcb/get-page-id changes) + variant-ids-to-validate + (str "generate-relocate: " (vec ids) " to parent " parent-id)))))) (defn change-show-in-viewer [shape hide?] diff --git a/common/src/app/common/logic/variant_properties.cljc b/common/src/app/common/logic/variant_properties.cljc index eca535955e..afc8a3334f 100644 --- a/common/src/app/common/logic/variant_properties.cljc +++ b/common/src/app/common/logic/variant_properties.cljc @@ -242,10 +242,22 @@ (assoc :name new-name)))))) (defn generate-make-shapes-no-variant - "Extract some components from a variant, removing the variant-id and variant-name from the - main instances and the variant-id and variant-properties from the components." - [changes shapes] - (reduce generate-make-shape-no-variant changes shapes)) + "Extract some components from a variant, removing the variant-id and variant-name from the + main instances and the variant-id and variant-properties from the components. + + The skip-validation? flag, if set, avoids validating the shapes. Use it when the shapes + are still inside the variant container, and validate after moving them." + [changes shapes & {:keys [skip-validation?]}] + (let [changes' (reduce generate-make-shape-no-variant changes shapes)] + (if (or skip-validation? (empty? shapes)) + changes' + (let [ids-to-validate (into (mapv :id shapes) + (comp (keep :variant-id) (distinct)) + shapes)] + (pcb/validate-shapes changes' + (pcb/get-page-id changes') + ids-to-validate + (str "generate-make-shapes-no-variant: " (mapv :id shapes))))))) (defn- create-new-properties-from-variant [shape min-props data container-name base-properties] diff --git a/common/test/common_tests/logic/variant_properties_test.cljc b/common/test/common_tests/logic/variant_properties_test.cljc index 5e2eccf19c..ccbc28a0de 100644 --- a/common/test/common_tests/logic/variant_properties_test.cljc +++ b/common/test/common_tests/logic/variant_properties_test.cljc @@ -7,6 +7,7 @@ (ns common-tests.logic.variant-properties-test (:require [app.common.files.changes-builder :as pcb] + [app.common.logic.shapes :as cls] [app.common.logic.variant-properties :as clvp] [app.common.test-helpers.components :as thc] [app.common.test-helpers.compositions :as tho] @@ -607,7 +608,9 @@ (pcb/with-page-id (:id page)) (pcb/with-library-data (:data file)) (pcb/with-objects (:objects page)) - (clvp/generate-make-shapes-no-variant [main01])) + ;; main01 is still inside the variant container, as happens in + ;; generate-relocate before moving it, so this state is not valid + (clvp/generate-make-shapes-no-variant [main01] :skip-validation? true)) file' (thf/apply-changes file changes :validate? false) @@ -675,6 +678,27 @@ (t/is (= "Frame1" (-> c03' :variant-properties first :value))) (t/is (= "Frame1" (-> m03' :variant-name))))) +(t/deftest test-relocate-variant-outside-container + (let [file (-> (thf/sample-file :file1) + (thv/add-variant :v01 :c01 :m01 :c02 :m02)) + page (thf/current-page file) + main01 (ths/get-shape file :m01) + + changes (cls/generate-relocate (-> (pcb/empty-changes nil) + (pcb/with-page-id (:id page)) + (pcb/with-library-data (:data file)) + (pcb/with-objects (:objects page))) + uuid/zero 0 #{(:id main01)}) + + file' (thf/apply-changes file changes) + + comp01' (thc/get-component file' :c01) + main01' (ths/get-shape file' :m01)] + + (t/is (= uuid/zero (:parent-id main01'))) + (t/is (nil? (:variant-id comp01'))) + (t/is (nil? (:variant-id main01'))))) + ;; This makes generate-make-shapes-variant call generate-add-new-property ;; several times before the new shape joins the variant (t/deftest test-make-shapes-variant-convert-adding-properties @@ -958,3 +982,41 @@ (clvp/generate-add-new-property v-id :skip-validation? true))] (t/is (nil? (validation-error file changes)))))) + +#?(:cljs + (t/deftest test-make-shapes-no-variant-validates-shape + (let [file (-> (thf/sample-file :file1) + (thv/add-variant :v01 :c01 :m01 :c02 :m02) + (ths/update-shape :m01 :component-file (uuid/next))) + page (thf/current-page file) + main01 (ths/get-shape file :m01) + + 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-no-variant [main01])) + + error (validation-error file changes)] + + (t/is (str/includes? (:hint error) "generate-make-shapes-no-variant")) + (t/is (contains? (error-codes error) :component-main-external))))) + +#?(:cljs + (t/deftest test-relocate-variant-outside-container-validates-container + (let [file (-> (thf/sample-file :file1) + (thv/add-variant :v01 :c01 :m01 :c02 :m02) + (ths/update-shape :m02 :variant-id (uuid/next))) + page (thf/current-page file) + main01 (ths/get-shape file :m01) + + changes (cls/generate-relocate (-> (pcb/empty-changes nil) + (pcb/with-page-id (:id page)) + (pcb/with-library-data (:data file)) + (pcb/with-objects (:objects page))) + uuid/zero 0 #{(:id main01)}) + + error (validation-error file changes)] + + (t/is (str/includes? (:hint error) "generate-relocate")) + (t/is (contains? (error-codes error) :main-instance-invalid-variant-id)))))