github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-29 14:00:37 +00:00
Code Issues Packages Projects Releases Wiki Activity

🔧 Validate variant containers after property changes

Browse Source 
apply-changes-local replays the pending redo changes on a local copy of
the data, so that later builder calls can read up-to-date values. It
replayed :validate-shapes too, so the validator ran on that copy, which
has no real file id and no libraries. Any chain of two generators that
apply local changes then failed with false errors, such as
:component-not-found. Skip :validate-shapes when applying changes
locally.

Add the variant container to the shapes that
generate-update-property-name and generate-remove-property validate.
Validating only the main instances misses errors that only the container
check finds, such as siblings with different property names.

Register variant-properties-test in the runner so that it runs on JS,
where validation takes place. Add tests that corrupt a main instance or
the container and expect a validation error. Remove two debug prn calls
from the validator.

AI-assisted-by: claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Eva Marco 2026-09-29 14:26:28 +02:00
parent 5c80841143
commit b46bcc9207
5 changed files with 109 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
common/src/app/common/files/changes_builder.cljc
View File

@ -197,6 +197,8 @@
redo-changes (:redo-changes changes)
new-changes (if (< index (count redo-changes))
(->> (subvec (:redo-changes changes) index)
;; Validation only makes sense on the real file data
(remove #(= :validate-shapes (:type %)))
(map #(-> %
(assoc :page-id uuid/zero)
(dissoc :component-id))))

2
common/src/app/common/files/validate.cljc
View File

@ -270,8 +270,6 @@
"Shape expected to be main instance"
shape file page))
(when-not (= (:component-file shape) (:id file))
(prn "shape" (:id shape) (:name shape) (:component-file shape))
(prn "file" (:id file) (:name file))
(report-error :component-main-external
"Main instance should refer to a component in the same file"
shape file page))

6
common/src/app/common/logic/variant_properties.cljc
View File

@ -41,8 +41,8 @@
{:apply-changes-local-library? true}))
changes
related-components)
ids-to-validate (map :main-instance-id related-components)
ids-to-validate (conj (mapv :main-instance-id related-components) variant-id)
changes (pcb/validate-shapes changes
(pcb/get-page-id changes)
@ -74,7 +74,7 @@
changes
related-components)
ids-to-validate (map :main-instance-id related-components)
ids-to-validate (conj (mapv :main-instance-id related-components) variant-id)
changes (pcb/validate-shapes changes
(pcb/get-page-id changes)

103
common/test/common_tests/logic/variant_properties_test.cljc
View File

@ -15,10 +15,29 @@
[app.common.test-helpers.shapes :as ths]
[app.common.test-helpers.variants :as thv]
[app.common.uuid :as uuid]
[clojure.test :as t]))
[clojure.test :as t]
[cuerdas.core :as str]))
(t/use-fixtures :each thi/test-fixture)
#?(:cljs
(defn- validation-error
"Apply the changes and return the ex-data of the :validate-shapes
error they raise, or nil if they raise none."
[file changes]
(try
(thf/apply-changes file changes :validate? false)
nil
(catch :default e
(let [data (ex-data e)]
(when (= :referential-integrity (:code data))
data))))))
#?(:cljs
(defn- error-codes
[error]
(into #{} (map :code) (:details error))))
;; =============================================================================
;; generate-update-property-name tests
;; =============================================================================
@ -655,3 +674,85 @@
(t/is (= "Property 1" (-> c03' :variant-properties first :name)))
(t/is (= "Frame1" (-> c03' :variant-properties first :value)))
(t/is (= "Frame1" (-> m03' :variant-name)))))
;; =============================================================================
;; validate-shapes failure tests (only run on CLJS, where validation runs)
;; =============================================================================
#?(:cljs
(t/deftest test-update-property-name-validates-main
(let [file (-> (thf/sample-file :file1)
(thv/add-variant-two-properties :v01 :c01 :m01 :c02 :m02)
(ths/update-shape :m01 :variant-id (uuid/next)))
v-id (-> (ths/get-shape file :v01) :id)
page (thf/current-page file)
changes (-> (pcb/empty-changes nil)
(pcb/with-page-id (:id page))
(pcb/with-library-data (:data file))
(pcb/with-objects (:objects page))
(clvp/generate-update-property-name v-id 0 "NewName"))
error (validation-error file changes)]
(t/is (str/includes? (:hint error) "generate-update-property-name"))
(t/is (contains? (error-codes error) :variant-component-bad-id)))))
#?(:cljs
(t/deftest test-update-property-name-validates-container
(let [file (-> (thf/sample-file :file1)
(thv/add-variant-two-properties :v01 :c01 :m01 :c02 :m02)
(thc/update-component :c02 {:variant-properties [{:name "Other" :value "p1v2"}
{:name "Property 2" :value "p2v2"}]}))
v-id (-> (ths/get-shape file :v01) :id)
page (thf/current-page file)
changes (-> (pcb/empty-changes nil)
(pcb/with-page-id (:id page))
(pcb/with-library-data (:data file))
(pcb/with-objects (:objects page))
(clvp/generate-update-property-name v-id 1 "NewName"))
error (validation-error file changes)]
(t/is (str/includes? (:hint error) "generate-update-property-name"))
(t/is (contains? (error-codes error) :invalid-variant-properties)))))
#?(:cljs
(t/deftest test-remove-property-validates-main
(let [file (-> (thf/sample-file :file1)
(thv/add-variant-two-properties :v01 :c01 :m01 :c02 :m02)
(ths/update-shape :m01 :variant-id (uuid/next)))
v-id (-> (ths/get-shape file :v01) :id)
page (thf/current-page file)
changes (-> (pcb/empty-changes nil)
(pcb/with-page-id (:id page))
(pcb/with-library-data (:data file))
(pcb/with-objects (:objects page))
(clvp/generate-remove-property v-id 1))
error (validation-error file changes)]
(t/is (str/includes? (:hint error) "generate-remove-property"))
(t/is (contains? (error-codes error) :variant-component-bad-id)))))
#?(:cljs
(t/deftest test-remove-property-validates-container
(let [file (-> (thf/sample-file :file1)
(thv/add-variant-two-properties :v01 :c01 :m01 :c02 :m02)
(thc/update-component :c02 {:variant-properties [{:name "Other" :value "p1v2"}
{:name "Property 2" :value "p2v2"}]}))
v-id (-> (ths/get-shape file :v01) :id)
page (thf/current-page file)
changes (-> (pcb/empty-changes nil)
(pcb/with-page-id (:id page))
(pcb/with-library-data (:data file))
(pcb/with-objects (:objects page))
(clvp/generate-remove-property v-id 1))
error (validation-error file changes)]
(t/is (str/includes? (:hint error) "generate-remove-property"))
(t/is (contains? (error-codes error) :invalid-variant-properties)))))

2
common/test/common_tests/runner.cljc
View File

@ -63,6 +63,7 @@
[common-tests.logic.swap-and-reset-test]
[common-tests.logic.swap-as-override-test]
[common-tests.logic.token-test]
[common-tests.logic.variant-properties-test]
[common-tests.logic.variants-switch-test]
[common-tests.math-test]
[common-tests.media-test]
@ -146,6 +147,7 @@
'common-tests.logic.swap-and-reset-test
'common-tests.logic.swap-as-override-test
'common-tests.logic.token-test
'common-tests.logic.variant-properties-test
'common-tests.logic.variants-switch-test
'common-tests.math-test
'common-tests.types.variant-test