diff --git a/.serena/memories/common/file-change-validation-migration-subtleties.md b/.serena/memories/common/file-change-validation-migration-subtleties.md index fff0b03a23..bf6ddbfeca 100644 --- a/.serena/memories/common/file-change-validation-migration-subtleties.md +++ b/.serena/memories/common/file-change-validation-migration-subtleties.md @@ -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 diff --git a/backend/test/backend_tests/rpc_file_update_repair_test.clj b/backend/test/backend_tests/rpc_file_update_repair_test.clj index 81723d4a03..f8eba29f53 100644 --- a/backend/test/backend_tests/rpc_file_update_repair_test.clj +++ b/backend/test/backend_tests/rpc_file_update_repair_test.clj @@ -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 diff --git a/common/src/app/common/files/repair.cljc b/common/src/app/common/files/repair.cljc index f0a52b788c..f0a3e80f1b 100644 --- a/common/src/app/common/files/repair.cljc +++ b/common/src/app/common/files/repair.cljc @@ -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) diff --git a/common/test/common_tests/files/repair_test.cljc b/common/test/common_tests/files/repair_test.cljc index f6f24821ce..ab7c32bc6e 100644 --- a/common/test/common_tests/files/repair_test.cljc +++ b/common/test/common_tests/files/repair_test.cljc @@ -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'))))))