github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-11-06 01:50:39 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Keep a copy linked when the repair finds its component's main

Browse Source 
A component whose :main-instance-id points to a missing shape fails
validation on its copies with :ref-shape-not-found. A repair scoped to
the copy's page never sees the main's own error, so it detached the
copy, even when the main was still on the component's page.

The repair of :ref-shape-not-found now looks for the component's main
on its page first and points :main-instance-id back to it, which keeps
the copy linked. A copy is still detached when no main is found.
This commit is contained in:
alonso.torres 2026-11-04 10:00:51 +01:00
parent bd79f07665
commit ca72f98238
4 changed files with 129 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
.serena/memories/common/file-change-validation-migration-subtleties.md
View File

@ -23,6 +23,7 @@
- `update-file` validates per save with the change-scoped variants: `validate-file-affected!` (referential, whole touched pages/components) and `validate-file-schema-affected!` (file schema with `:pages-index` reduced to the touched pages; non-page data checked whole). The schema variant maps change types to pages itself (`touched-page-ids`): a new change type that writes a page must be added there, or to `file-level-change-types` if it touches no page; an unknown type makes it check the whole file. The whole-file schema check loads and decodes every page, ~75–80% of a save on multi-page files.
- `repair-file` does not mutate data directly; it reduces validation errors into redo changes using `changes-builder`. Callers must apply or persist those changes.
- `repair-file-until-valid` stops on: no errors; unchanged `:data` after apply (backend page/`PointerMap` `=` is pointer identity); stable error fingerprint `(frequencies (map #(dissoc % :shape) errors))` vs previous pass (check fingerprint before `repair-file`/`process-changes`); or `max-iterations`. Each `repair-error` builds from the original `file-data`, so N `:set`-based errors on one shape take N passes.
- `:ref-shape-not-found` repair: when the copy root's local component has a `:main-instance-id` that resolves to no shape but a `:main-instance` shape of that component sits on its `:main-instance-page`, it emits `mod-component` pointing the id back to that main (keeps the copy linked; a repair scoped to the copy's page never sees the main's `:invalid-main-instance-id`). Otherwise near-main reassign, else detach the copy subtree.
- `:missing-slot` fires only for a REAL swap: a copy sub-head whose `shape-ref` is no longer a child of the near main parent. A pure positional mismatch (ref still a sibling elsewhere) is a reorder — valid, realigned by the async component sync; do not "repair" it by assigning swap slots (a slot freezes the child out of normal sync). `fix-missing-swap-slots` (migration 0019) follows the same membership rule.
## Migrations

26
backend/test/backend_tests/rpc_file_update_repair_test.clj
View File

@ -10,6 +10,7 @@
[app.common.features :as cfeat]
[app.common.files.repair :as cfr]
[app.common.files.validate :as cfv]
[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]
@ -257,6 +258,31 @@
(t/is (nil? (cfv/validate-file stored {})))
(t/is (nil? (get-in stored [:data :components (thi/id :component1) :objects])))))))
(t/deftest repair-on-copy-page-keeps-copy-of-lost-main
(thi/reset-idmap!)
(with-repair-flags
(fn []
(let [file (-> (thf/sample-file :file1 :page-label :page1)
(assoc :revn 0)
(tho/add-simple-component :component1 :root1 :child1)
(thf/add-sample-page :page2)
(thc/instantiate-component :component1 :copy1)
(thc/update-component :component1 :main-instance-id (uuid/next)))
page-id (thi/id :page2)
changes [(set-attr page-id (thi/id :copy1) :name "renamed")]
cfg {::files-update/validation-timings (atom {})}
stored (binding [pmap/*tracked* (pmap/create-tracked)]
(with-redefs [bfc/get-resolved-file-libraries (fn [_ _] {})]
(@#'files-update/process-changes-and-validate cfg file changes nil nil)))
copy1 (get-in stored [:data :pages-index page-id :objects (thi/id :copy1)])]
(t/is (some? (::files-update/repair stored)))
(t/is (nil? (cfv/validate-file stored {})))
(t/is (= (thi/id :root1)
(get-in stored [:data :components (thi/id :component1) :main-instance-id])))
(t/is (= "renamed" (:name copy1)))
(t/is (= (thi/id :root1) (:shape-ref copy1)))
(t/is (= (thi/id :component1) (:component-id copy1)))))))
(t/deftest repair-records-timings-with-schema-validation
(thi/reset-idmap!)
(with-full-repair-flags

34
common/src/app/common/files/repair.cljc
View File

@ -281,9 +281,26 @@
(pcb/with-file-data file-data)
(pcb/update-shapes [(:id shape)] repair-shape))))
(defn- find-lost-main
"Return the main of the local copy root `root-shape` when its component's
`:main-instance-id` points to a missing shape and the main is still on
the component's page; nil otherwise."
[file-data root-shape]
(when (= (:component-file root-shape) (:id file-data))
(let [component (ctkl/get-component file-data (:component-id root-shape))]
(when (and (some? component)
(nil? (ctf/get-component-root file-data component)))
(d/seek #(and (ctk/main-instance? %)
(= (:component-id %) (:id component))
(= (:component-file %) (:id file-data)))
(ctn/shapes-seq (ctf/get-component-page file-data component)))))))
(defmethod repair-error :ref-shape-not-found
[_ {:keys [shape page-id] :as error} file-data libraries]
(let [matching-shape (let [page (ctpl/get-page file-data page-id)
(let [lost-main (let [page (ctpl/get-page file-data page-id)]
(find-lost-main file-data (ctn/get-component-shape (:objects page) shape)))
matching-shape (let [page (ctpl/get-page file-data page-id)
root-shape (ctn/get-component-shape (:objects page) shape)
component-file (if (= (:component-file root-shape) (:id file-data))
file-data
@ -321,13 +338,24 @@
(log/debug :hint " -> detach shape" :shape-id (:id shape))
(ctk/detach-shape shape))]
;; If the shape still refers to the remote component, try to find the corresponding near one
;; If the copy's component has lost track of its main, point it back to the main. Else, if
;; the shape still refers to the remote component, try to find the corresponding near one
;; and link to it. If not, detach the shape.
(log/debug :hint "repairing shape :ref-shape-not-found" :id (:id shape) :name (:name shape) :page-id page-id)
(if (some? matching-shape)
(cond
(some? lost-main)
(do
(log/debug :hint " -> assign main-instance-id" :main-instance-id (:id lost-main))
(-> (pcb/empty-changes nil page-id)
(pcb/with-library-data file-data)
(pcb/update-component (:component-id lost-main) #(assoc % :main-instance-id (:id lost-main)))))
(some? matching-shape)
(-> (pcb/empty-changes nil page-id)
(pcb/with-file-data file-data)
(pcb/update-shapes [(:id shape)] reassign-shape))
:else
(let [page (ctpl/get-page file-data page-id)
shape-ids (cfh/get-children-ids-with-self (:objects page) (:id shape))]
(-> (pcb/empty-changes nil page-id)

71
common/test/common_tests/files/repair_test.cljc
View File

@ -546,3 +546,74 @@
;; First iteration used the seed; errors-fn runs only after the repair.
(t/is (= 1 @calls)))))
(defn- lost-main-file
"A component whose main is on page1 and its copy on page2, with the
component's `:main-instance-id` pointing to a missing shape."
[]
(-> (thf/sample-file :file1 :page-label :page1)
(tho/add-simple-component :component1 :root1 :child1)
(thf/add-sample-page :page2)
(thc/instantiate-component :component1 :copy1)
(thc/update-component :component1 :main-instance-id (uuid/next))))
(defn- edit-copy-changes
[]
[{:type :mod-obj
:page-id (thi/id :page2)
:id (thi/id :copy1)
:operations [{:type :set :attr :name :val "renamed"}]}])
(defn- repair-scoped
[file changes]
(cfr/repair-file-until-valid
file {}
(fn [file applied]
(cfv/validate-file-affected file {} (into applied changes)))))
(t/deftest repair-ref-shape-not-found-fixes-lost-main-instance-id
(t/testing "a repair scoped to the copy's page points the component back
to its main and keeps the copy linked"
(let [file (lost-main-file)
changes (edit-copy-changes)
errors (cfv/validate-file-affected file {} changes)
result (repair-scoped file changes)
file' (:file result)
copy1' (ths/get-shape file' :copy1 :page-label :page2)
component' (thc/get-component file' :component1)]
(t/is (= #{:ref-shape-not-found} (into #{} (map :code) errors)))
(t/is (nil? (:errors result)))
(t/is (nil? (cfv/validate-file file' {})))
(t/is (= (thi/id :root1) (:main-instance-id component')))
(t/is (= (thi/id :component1) (:component-id copy1')))
(t/is (= (thi/id :file1) (:component-file copy1')))
(t/is (= (thi/id :root1) (:shape-ref copy1')))
(t/is (true? (:component-root copy1'))))))
(t/deftest repair-whole-file-keeps-copy-of-lost-main
(t/testing "a whole-file repair fixes the component and keeps the copy linked"
(let [file (lost-main-file)
result (cfr/repair-file-until-valid
file {}
(fn [file _] (cfv/validate-file file {})))
file' (:file result)
copy1' (ths/get-shape file' :copy1 :page-label :page2)]
(t/is (nil? (:errors result)))
(t/is (nil? (cfv/validate-file file' {})))
(t/is (= (thi/id :root1) (:main-instance-id (thc/get-component file' :component1))))
(t/is (= (thi/id :root1) (:shape-ref copy1')))
(t/is (= (thi/id :component1) (:component-id copy1'))))))
(t/deftest repair-ref-shape-not-found-detaches-copy-of-missing-main
(t/testing "a copy is detached when no shape on the component's page is its main"
(let [file (-> (lost-main-file)
(ths/update-shape :root1 :main-instance nil :page-label :page1))
changes (edit-copy-changes)
result (repair-scoped file changes)
copy1' (ths/get-shape (:file result) :copy1 :page-label :page2)]
(t/is (nil? (:errors result)))
(t/is (nil? (:component-id copy1')))
(t/is (nil? (:shape-ref copy1'))))))