From 2fc6e33f17111d6995baf39f014e685efca770a2 Mon Sep 17 00:00:00 2001 From: Eva Marco Date: Tue, 29 Sep 2026 16:50:24 +0200 Subject: [PATCH] :wrench: Validate the enclosing component when relocating a variant When generate-relocate moves a variant inside a component, the variant is no longer a root, so relocate left it out and validated nothing in its place. Validate the root of the enclosing component instead. The workspace does not allow this move, but relocate can be called directly. Document that the default call of generate-make-shapes-no-variant requires the shapes to be outside their variant container. Add tests for moving a variant into another container, and into a component with a corrupted root. AI-assisted-by: claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 --- common/src/app/common/logic/shapes.cljc | 16 +++-- .../app/common/logic/variant_properties.cljc | 5 +- .../common_tests/logic/variants_test.cljc | 58 +++++++++++++++++++ 3 files changed, 72 insertions(+), 7 deletions(-) diff --git a/common/src/app/common/logic/shapes.cljc b/common/src/app/common/logic/shapes.cljc index c40e2290cd..a7567ea649 100644 --- a/common/src/app/common/logic/shapes.cljc +++ b/common/src/app/common/logic/shapes.cljc @@ -587,16 +587,22 @@ #{} (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) + ;; The old and new variant containers, and the moved variants. Inside a + ;; component they are not roots anymore and can't be validated on their + ;; own, so validate the root of that component instead + parent-root + (ctn/get-instance-root objects parent) + 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))] + (and (seq variant-shapes) (nil? parent-root)) + (into (map :id) variant-shapes) + + (and (seq variant-shapes) (some? parent-root)) + (conj (:id parent-root)))] (-> changes ;; Remove layout-item properties and tokens when moving a shape outside a layout diff --git a/common/src/app/common/logic/variant_properties.cljc b/common/src/app/common/logic/variant_properties.cljc index f2f056f660..35e2a9232e 100644 --- a/common/src/app/common/logic/variant_properties.cljc +++ b/common/src/app/common/logic/variant_properties.cljc @@ -249,8 +249,9 @@ "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." + By default it validates the shapes and their old variant containers, so the shapes + must already be outside the container. If they are still inside it, set the + skip-validation? flag 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)) diff --git a/common/test/common_tests/logic/variants_test.cljc b/common/test/common_tests/logic/variants_test.cljc index 858d623c21..e3a0abef77 100644 --- a/common/test/common_tests/logic/variants_test.cljc +++ b/common/test/common_tests/logic/variants_test.cljc @@ -11,6 +11,7 @@ [app.common.logic.libraries :as cll] [app.common.logic.shapes :as cls] [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] @@ -21,6 +22,11 @@ (t/use-fixtures :each thi/test-fixture) +#?(:cljs + (defn- error-codes + [error] + (into #{} (map :code) (:details error)))) + #?(:cljs (defn- validation-error "Apply the changes and return the ex-data of the :validate-shapes @@ -213,3 +219,55 @@ ;; Ctrl+D inside the same container also updates the last property value (t/is (str/includes? (:hint error) "generate-update-property-value"))))) + +(defn- relocate-changes + [file shape-label parent-label] + (let [page (thf/current-page file)] + (cls/generate-relocate (-> (pcb/empty-changes nil) + (pcb/with-page-id (:id page)) + (pcb/with-library-data (:data file)) + (pcb/with-objects (:objects page))) + (thi/id parent-label) 0 #{(thi/id shape-label)}))) + +(t/deftest test-relocate-variant-into-another-container + (let [;; ==== Setup + file (-> (thf/sample-file :file1) + add-two-variants) + + ;; ==== Action + changes (relocate-changes file :m01 :v02) + file' (thf/apply-changes file changes) + + ;; ==== Get + m01' (ths/get-shape file' :m01) + c01' (thc/get-component file' :c01)] + + ;; ==== Check + (t/is (= (thi/id :v02) (:parent-id m01'))) + (t/is (= (thi/id :v02) (:variant-id m01'))) + (t/is (= (thi/id :v02) (:variant-id c01'))))) + +#?(:cljs + (t/deftest test-relocate-variant-into-another-container-validates + (let [file (-> (thf/sample-file :file1) + add-two-variants) + changes (relocate-changes file :m01 :v02)] + + (t/is (nil? (validation-error file changes)))))) + +;; The workspace does not allow moving a main instance inside a component +;; (see ctn/invalid-structure-for-component?), but relocate may be called +;; directly. The moved variant is not a root anymore, so relocate validates +;; the root of the component that holds it. +#?(:cljs + (t/deftest test-relocate-variant-into-component-validates-root + (let [file (-> (thf/sample-file :file1) + (thv/add-variant :v01 :c01 :m01 :c02 :m02) + (tho/add-simple-component :c03 :m03 :b03 + :child-params {:type :frame}) + (ths/update-shape :m03 :component-file (uuid/next))) + changes (relocate-changes file :m01 :b03) + error (validation-error file changes)] + + (t/is (str/includes? (:hint error) "generate-relocate")) + (t/is (contains? (error-codes error) :component-main-external)))))