From b890b94d27e69b5129ca11a8990ee57dd4429819 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 28 Sep 2026 14:57:17 +0200 Subject: [PATCH] :bug: Add a migration dropping the obsolete stroke-per-side attr (#11946) Individual stroke widths shipped with a `:stroke-per-side` boolean on every stroke, telling the renderer and the CSS generator whether the four per-side widths were meaningful. Comparing the sides answers that on its own, so the attribute was later dropped from the closed stroke schema and the toggle became ephemeral editor state. Files written between those two changes still carry the attribute, and the closed schema rejects the unknown key, so loading such a file fails `check-file-data` with a `:malli.core/extra-key` error and surfaces as an internal error in the editor. Add migration `0030-remove-stroke-per-side-attr`, which drops the attribute from every stroke of every page and component shape. The per-side widths are the saved design data and are kept untouched, so a file whose boolean was `true` renders exactly as before. The migration is naturally idempotent and a no-op for files that never carried the attribute, since `dissoc` on a map without the key returns an equal map. Cover it with tests for the schema rejection, the per-side and global widths surviving, component shapes, idempotency, and the run through `migrate-file`. Closes #11943 AI-assisted-by: space-bunny-free --- ...-change-validation-migration-subtleties.md | 3 +- common/src/app/common/files/migrations.cljc | 22 ++- .../common_tests/files_migrations_test.cljc | 142 ++++++++++++++++++ 3 files changed, 165 insertions(+), 2 deletions(-) diff --git a/.serena/memories/common/file-change-validation-migration-subtleties.md b/.serena/memories/common/file-change-validation-migration-subtleties.md index 014a458a30..d013415730 100644 --- a/.serena/memories/common/file-change-validation-migration-subtleties.md +++ b/.serena/memories/common/file-change-validation-migration-subtleties.md @@ -28,4 +28,5 @@ - Prefer optional attrs/default behavior so old files continue working without migration. If absence cannot preserve old behavior, add a migration. - Migrations are an ordered set mixing legacy version-derived ids and newer named ids. Keep append order stable; `migrate` applies the set difference between available migrations and file migrations. - `migrate-file` synthesizes legacy migration ids from old numeric versions when `:migrations` is absent, migrates legacy features, and records feature flags created through `cfeat/*new*`. -- When a file had no previous `:migrations`, `migrate-file` marks all migrations as migrated in metadata so callers persist the complete migration set, not only transformations that changed data. \ No newline at end of file +- When a file had no previous `:migrations`, `migrate-file` marks all migrations as migrated in metadata so callers persist the complete migration set, not only transformations that changed data. +- Closed schemas (`schema:stroke-attrs` and others) reject a key that was dropped from the declaration, so removing a declared attr needs a migration in the same change, for data already written with it. \ No newline at end of file diff --git a/common/src/app/common/files/migrations.cljc b/common/src/app/common/files/migrations.cljc index 46f24f6add..649f980021 100644 --- a/common/src/app/common/files/migrations.cljc +++ b/common/src/app/common/files/migrations.cljc @@ -2132,6 +2132,25 @@ (update :pages-index d/update-vals repair-container) (d/update-when :components d/update-vals repair-container)))) +(defmethod migrate-data "0030-remove-stroke-per-side-attr" + ;; Individual stroke widths shipped with a `:stroke-per-side` boolean on every + ;; stroke, telling the renderer and the CSS generator whether the four side + ;; widths were meaningful. Comparing the sides answers that on its own, so + ;; the attribute was dropped from the closed stroke schema and the toggle + ;; became ephemeral editor state. Files written while it was still persisted + ;; fail validation with an `:malli.core/extra-key` error; drop the attribute + ;; and keep the side widths, which hold the real design data. + [data _] + (letfn [(repair-shape [shape] + (d/update-when shape :strokes d/update-vals #(dissoc % :stroke-per-side))) + + (repair-container [container] + (d/update-when container :objects d/update-vals repair-shape))] + + (-> data + (update :pages-index d/update-vals repair-container) + (d/update-when :components d/update-vals repair-container)))) + (def available-migrations (into (d/ordered-set) ["legacy-2" @@ -2218,4 +2237,5 @@ "0026-fix-svg-raw-shapes-uuids" "0027-separate-tokens-status" "0028-normalize-constrained-values" - "0029-move-background-blur-out-of-blur"])) + "0029-move-background-blur-out-of-blur" + "0030-remove-stroke-per-side-attr"])) diff --git a/common/test/common_tests/files_migrations_test.cljc b/common/test/common_tests/files_migrations_test.cljc index b87d1b4d4c..7e92deda66 100644 --- a/common/test/common_tests/files_migrations_test.cljc +++ b/common/test/common_tests/files_migrations_test.cljc @@ -317,3 +317,145 @@ "background blur moved before schema validation") (t/is (nil? (get-in file' [:data :pages-index page-id :objects shape-id :blur])) "mis-typed :blur removed"))) + +(t/deftest migration-0030-removes-stroke-per-side-attr + (let [migration-id "0030-remove-stroke-per-side-attr" + page-id (uuid/next) + shape-id (uuid/next) + stroke {:stroke-width 2 + :stroke-width-top 2 + :stroke-width-right 4 + :stroke-width-bottom 3 + :stroke-width-left 6 + :stroke-color "#000000" + :stroke-per-side true} + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + data {:pages-index {page-id {:objects {shape-id shape}}}} + data' (cfm/migrate-data data migration-id) + shape' (get-in data' [:pages-index page-id :objects shape-id]) + stroke' (first (:strokes shape'))] + + (t/is (nil? (:stroke-per-side stroke')) "obsolete :stroke-per-side removed") + (t/is (= (dissoc stroke :stroke-per-side) stroke') + "no other stroke attr changed") + (t/is (= [2 4 3 6] [(:stroke-width-top stroke') + (:stroke-width-right stroke') + (:stroke-width-bottom stroke') + (:stroke-width-left stroke')]) + "per-side widths preserved") + (t/is (= data' (cfm/migrate-data data' migration-id)) + "migration is idempotent"))) + +(t/deftest migration-0030-repairs-strokes-rejected-by-schema + (let [migration-id "0030-remove-stroke-per-side-attr" + file-id (uuid/next) + page-id (uuid/next) + shape-id (uuid/next) + stroke {:stroke-width 1 + :stroke-width-top 1 + :stroke-width-right 1 + :stroke-width-bottom 1 + :stroke-width-left 1 + :stroke-color "#000000" + :stroke-per-side true} + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + data (-> (ctf/make-file-data file-id page-id) + (assoc-in [:pages-index page-id :objects shape-id] shape))] + + (t/is (thrown? #?(:clj Exception :cljs js/Error) + (ctf/check-file-data data)) + "new schema rejects a stroke carrying :stroke-per-side") + + (let [data' (cfm/migrate-data data migration-id)] + (t/is (= data' (ctf/check-file-data data')) + "migrated data passes the schema")))) + +(t/deftest migration-0030-repairs-component-strokes + (let [migration-id "0030-remove-stroke-per-side-attr" + component-id (uuid/next) + shape-id (uuid/next) + stroke {:stroke-width 3 + :stroke-width-top 3 + :stroke-width-right 5 + :stroke-width-bottom 3 + :stroke-width-left 3 + :stroke-color "#000000" + :stroke-per-side true} + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + data {:components {component-id {:objects {shape-id shape}}}} + data' (cfm/migrate-data data migration-id) + stroke' (get-in data' [:components component-id :objects shape-id :strokes 0])] + + (t/is (nil? (:stroke-per-side stroke')) + "obsolete attr removed from a component stroke") + (t/is (= 5 (:stroke-width-right stroke')) + "component per-side width preserved"))) + +(t/deftest migration-0030-keeps-strokes-without-the-obsolete-attr + (let [migration-id "0030-remove-stroke-per-side-attr" + page-id (uuid/next) + shape-id (uuid/next) + stroke {:stroke-width 2 + :stroke-width-top 2 + :stroke-width-right 2 + :stroke-width-bottom 2 + :stroke-width-left 2} + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + data {:pages-index {page-id {:objects {shape-id shape}}}}] + + (t/is (= data (cfm/migrate-data data migration-id)) + "a stroke without the obsolete attr is left untouched"))) + +(t/deftest migration-0030-keeps-distinct-sides-when-the-attr-was-false + (let [migration-id "0030-remove-stroke-per-side-attr" + page-id (uuid/next) + shape-id (uuid/next) + ;; The old toggle set the flag to false without equalizing the sides, + ;; so the per-side values are the saved design intent and must survive. + stroke {:stroke-width 1 + :stroke-width-top 1 + :stroke-width-right 8 + :stroke-width-bottom 1 + :stroke-width-left 1 + :stroke-color "#000000" + :stroke-per-side false} + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + data {:pages-index {page-id {:objects {shape-id shape}}}} + stroke' (get-in (cfm/migrate-data data migration-id) + [:pages-index page-id :objects shape-id :strokes 0])] + + (t/is (nil? (:stroke-per-side stroke')) "obsolete attr removed") + (t/is (= 8 (:stroke-width-right stroke')) + "distinct side width kept instead of being equalized"))) + +(t/deftest migration-0030-runs-through-file-migration + (let [migration-id "0030-remove-stroke-per-side-attr" + shape-id (uuid/next) + stroke {:stroke-width 5 + :stroke-width-top 5 + :stroke-width-right 5 + :stroke-width-bottom 5 + :stroke-width-left 5 + :stroke-color "#000000" + :stroke-per-side true} + file (ctf/make-file {:name "Legacy per-side stroke"}) + page-id (first (get-in file [:data :pages])) + shape (-> (cts/setup-shape {:id shape-id :type :rect}) + (assoc :strokes [stroke])) + file (-> file + (assoc :migrations (disj cfm/available-migrations migration-id)) + (assoc-in [:data :pages-index page-id :objects shape-id] shape)) + file' (cfm/migrate-file file {}) + stroke' (get-in file' [:data :pages-index page-id :objects shape-id :strokes 0])] + + (t/is (cfm/need-migration? file) "new migration detected") + (t/is (not (cfm/need-migration? file')) "new migration recorded") + (t/is (contains? (:migrations file') migration-id) "migration id persisted") + (t/is (nil? (:stroke-per-side stroke')) + "obsolete attr removed before schema validation") + (t/is (= 5 (:stroke-width stroke')) "stroke width preserved")))