From b7638d2fc41bc54321de989eebd0296195c94a2d Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Thu, 15 Oct 2026 15:10:09 +0200 Subject: [PATCH] :bug: Fix component validation and repair Fixes in the validation of component refs and in the file repairs (Phase 3c). - Name the component in the :ref-shape-is-head error. A copy shape whose near main is a nested copy head must be a head of the same component. The check reported no component, so its repair reheaded the shape with nil ids, validation found the same error again and the file never converged (F48). The error now carries the near main's component file and id, as :ref-shape-is-not-head does, and the repair reheads the shape with them. repair-components-test repairs a nested copy head that lost its component. - Remove a stray component file in the :component-main repair. Validation reports :component-main on a shape inside a main that is not a head when it has :main-instance or :component-file. The repair only removed :main-instance, so a shape with :component-file kept the error after every pass and the file never converged (F47). For a shape that is not a head, the repair also removes :component-file. repair-components-test repairs a main child with a component file. - Check refs inside copies at every level where they are local. Validation of the shapes inside a copy had two faults: - it checked the ref of a nested copy head (it exists, and is a child of the near main of its parent) only for heads directly under a top copy root: the flag that allowed it reached one level down, so heads under a group or nested twice were never checked (F87); - for a library copy it reported a ref the library has deleted since the file last took its updates (F104). That state is normal and lasts until the user takes the update, which removes the copy shape; the repair detached the shape or took another position instead, so the update no longer removed it. The flag now says whether the shapes refer into this file, and passes through every level of the copy: the children of a top copy refer into its component's file, and those of a swapped head into its own component's. :ref-shape-not-found and :ref-not-in-near-main are checked on nested heads only where it holds. The call on copy shapes that are not heads read the library from the shape itself, which only heads have, so it never ran; it goes. repair-components-test: a dangling ref on a head nested twice and on one under a group is reported; a library main that deleted its nested copy leaves the consuming file valid. - Check the ref of every local copy shape that is not a head. Validation never reported a dangling :shape-ref on a copy shape that is not a head (F46). It now checks it wherever the shapes of the copy refer into this file; refs into a library may name a shape the library deleted since the file last took its updates (F104). The repair of :ref-shape-not-found looks for a matching main shape and, finding none, detaches the shape, which stays in the copy as a shape of its own. repair-components-test repairs a copy child with a dangling ref. - Keep copies linked while their component's main is repaired. When a component's :main-instance-id or :main-instance-page was wrong, validation also reported :ref-shape-not-found on every copy of it, as their near main could not be found. Both repairs ran in the same pass: the component got its pointer back, and the copies, which the repair could not match against the old pointer, were detached (F60). Every user file the backend repaired lost those links. While some error in a component's main instance pointer is repaired, the :ref-shape-not-found errors wait for the next pass, which finds the near main and reports nothing. repair-components-test: the copies keep their link after the repair of :invalid-main-instance-id, :invalid-main-instance-page and :invalid-main-instance. - Look up a library component's shapes in its library in repairs. The :ref-shape-not-found repair looks for a main shape the dangling ref can be matched to among the shapes of the copy's component. It found that component in the right file, local or library, but then read its shapes from the local file data, where a library component's main is not: nothing matched, and the whole copy was detached (F88). It now reads them from the component's own file. repair-components-test: a library copy that points at another copy of the same component is reassigned to the library main. - Keep file repairs from touching the copies they fix. The repairs of a file's shapes wrote through the changes builder like an edit, so a repair of a copy shape touched the groups it changed: a copy shape whose geometry was reset kept the reset size as an override, and sync never gave it back its main's (F7, part 1). A repair is not an override; the repairs of shapes now touch nothing, and those that fix the touched groups still set them. repair-test: a copy shape with invalid geometry is repaired and stays untouched. AI-assisted-by: claude-opus-5-5 --- common/src/app/common/files/repair.cljc | 30 ++++- common/src/app/common/files/validate.cljc | 70 ++++++------ .../files/repair_components_test.cljc | 106 +++++++++++++----- .../test/common_tests/files/repair_test.cljc | 19 ++++ 4 files changed, 160 insertions(+), 65 deletions(-) diff --git a/common/src/app/common/files/repair.cljc b/common/src/app/common/files/repair.cljc index a590530019..a224be7ef5 100644 --- a/common/src/app/common/files/repair.cljc +++ b/common/src/app/common/files/repair.cljc @@ -30,14 +30,15 @@ (defn- repair-shapes "The changes that apply `repair-fn` to the shapes `ids` of the page of - `error` (its shape, by default)." + `error` (its shape, by default). A repair is not an override, so it + touches nothing; a repair of the touched groups sets them itself." ([code error file-data repair-fn] (repair-shapes code error file-data [(:id (:shape error))] repair-fn)) ([code {:keys [shape page-id]} file-data ids repair-fn] (log/debug :hint (str "repairing shape " code) :id (:id shape) :name (:name shape) :page-id page-id) (-> (pcb/empty-changes nil page-id) (pcb/with-file-data file-data) - (pcb/update-shapes ids repair-fn)))) + (pcb/update-shapes ids repair-fn {:ignore-touched true})))) (defn- repair-component "The changes that apply `repair-fn` to the component `component-id`." @@ -166,7 +167,10 @@ (repair-shapes code error file-data (fn [shape] (log/debug :hint " -> unset :main-instance") - (dissoc shape :main-instance)))) + (cond-> (dissoc shape :main-instance) + ;; only a head may name a component file + (not (ctk/instance-head? shape)) + (dissoc :component-file))))) (defmethod repair-error :should-not-be-component-root [code error file-data _] @@ -184,7 +188,7 @@ (-> (get libraries (:component-file root-shape)) :data)) component (when component-file (ctkl/get-component component-file (:component-id root-shape) true)) - component-shapes (ctf/get-component-shapes file-data component)] + component-shapes (ctf/get-component-shapes component-file component)] ;; Check if the shape points to the remote main. If so, reassign to the near main. (if-let [near-shape-1 (d/seek #(= (:shape-ref %) (:shape-ref shape)) component-shapes)] @@ -201,7 +205,7 @@ (-> (get libraries (:component-file head-shape)) :data)) component (when component-file (ctkl/get-component component-file (:component-id head-shape) true)) - component-shapes (ctf/get-component-shapes file-data component)] + component-shapes (ctf/get-component-shapes component-file component)] (if-let [near-shape-3 (d/seek #(= (:id %) (:shape-ref random-shape)) component-shapes)] near-shape-3 nil))))))] @@ -527,6 +531,20 @@ (log/error :hint "Unknown error code, don't know how to repair" :code (:code error)) file) +(def ^:private main-instance-codes + "Errors in the pointer from a component to its main instance. Until + they are repaired, the copies of the component find no near main." + #{:invalid-main-instance-id :invalid-main-instance-page :invalid-main-instance}) + +(defn- repairable-now + "The errors to repair in this pass: a copy shape whose main is not found + waits for a later pass while some component's main instance is being + repaired, as the main may be found then." + [errors] + (if (some (comp main-instance-codes :code) errors) + (remove #(= :ref-shape-not-found (:code %)) errors) + errors)) + (defn repair-file [{:keys [data id] :as file} libraries errors] (log/debug :hint "repairing file" :id (str id) :errors (count errors)) @@ -538,5 +556,5 @@ data libraries))) (pcb/empty-changes nil) - errors)] + (repairable-now errors))] redo-changes)) diff --git a/common/src/app/common/files/validate.cljc b/common/src/app/common/files/validate.cljc index 7556c55546..10a31b3571 100644 --- a/common/src/app/common/files/validate.cljc +++ b/common/src/app/common/files/validate.cljc @@ -342,13 +342,10 @@ (defn- check-component-ref "Validate that the referenced shape exists in the near component." [shape file page libraries] - (let [library-exists (library-exists? file libraries shape) - ref-shape (when library-exists - (find-ref-shape* file page libraries shape))] - (when (and library-exists (nil? ref-shape)) - (report-error :ref-shape-not-found - (str/ffmt "Referenced shape % not found in near component" (:shape-ref shape)) - shape file page)))) + (when (nil? (find-ref-shape* file page libraries shape)) + (report-error :ref-shape-not-found + (str/ffmt "Referenced shape % not found in near component" (:shape-ref shape)) + shape file page))) (defn- check-component-not-ref "Validate that this shape does not reference other one." @@ -366,7 +363,9 @@ (ctk/instance-head? ref-shape)) (report-error :ref-shape-is-head (str/ffmt "Referenced shape % is a component, so the copy must also be" (:shape-ref shape)) - shape file page)))) + shape file page + :component-file (:component-file ref-shape) + :component-id (:component-id ref-shape))))) (defn- check-ref-is-head "Validate that the referenced shape is a nested copy root." @@ -495,36 +494,42 @@ - :component-root - :shape-ref" [shape file page libraries] - ;; We propagate have to propagate to nested shapes if library is valid or not - (let [library-exists (library-exists? file libraries shape)] - (check-component-not-main-head shape file page libraries) - (check-component-ref shape file page libraries) - (check-ref-is-head shape file page libraries) - (check-no-swap-slot shape file page) - (check-not-swapped shape file page) - (check-valid-touched shape file page) - (check-no-duplicate-ref shape file page) - (run! #(check-shape % file page libraries :context :copy-top :library-exists library-exists) (:shapes shape)))) + (check-component-not-main-head shape file page libraries) + (when (library-exists? file libraries shape) + (check-component-ref shape file page libraries)) + (check-ref-is-head shape file page libraries) + (check-no-swap-slot shape file page) + (check-not-swapped shape file page) + (check-valid-touched shape file page) + (check-no-duplicate-ref shape file page) + (run! #(check-shape % file page libraries :context :copy-top + :local-refs (= (:component-file shape) (:id file))) + (:shapes shape))) (defn- check-shape-copy-root-nested "Root shape of a nested copy instance - :component-id - :component-file - :shape-ref" - [shape file page libraries library-exists] + [shape file page libraries local-refs] (check-component-not-main-head shape file page libraries) (check-component-not-root shape file page) (check-valid-touched shape file page) (check-ref-component-id shape file page libraries) (check-no-swap-slot shape file page) - (check-ref-in-near-main shape file page libraries) - ;; We can have situations where the nested copy and the ancestor copy come from different libraries and some of them have been dettached - ;; so we only validate the shape-ref if the ancestor is from a valid library - (when library-exists - (check-component-ref shape file page libraries) - (check-ref-is-head shape file page libraries)) + ;; a ref into a library may name a shape the library has deleted since + ;; the file last took its updates, which then remove the copy shape + (when local-refs + (check-ref-in-near-main shape file page libraries) + (check-component-ref shape file page libraries)) + (check-ref-is-head shape file page libraries) (check-no-duplicate-ref shape file page) - (run! #(check-shape % file page libraries :context :copy-nested) (:shapes shape))) + ;; the children of a swapped head refer into its own component + (run! #(check-shape % file page libraries :context :copy-nested + :local-refs (if (ctk/swapped? shape) + (= (:component-file shape) (:id file)) + local-refs)) + (:shapes shape))) (defn- check-shape-main-not-root "Not-root shape of a main instance (not any attribute)" @@ -538,16 +543,17 @@ (defn- check-shape-copy-not-root "Not-root shape of a copy instance :shape-ref" - [shape file page libraries] + [shape file page libraries local-refs] (check-component-not-main-not-head shape file page) (check-component-not-root shape file page) - (check-component-ref shape file page libraries) + (when local-refs + (check-component-ref shape file page libraries)) (check-ref-is-not-head shape file page libraries) (check-no-swap-slot shape file page) (check-not-swapped shape file page) (check-valid-touched shape file page) (check-no-duplicate-ref shape file page) - (run! #(check-shape % file page libraries :context :copy-any) (:shapes shape))) + (run! #(check-shape % file page libraries :context :copy-any :local-refs local-refs) (:shapes shape))) (defn- check-shape-not-component "Shape is not in a component or is a fostered children (not any @@ -644,7 +650,7 @@ - :main-any - :copy-any " - [shape-id file page libraries & {:keys [context library-exists] :or {context :not-component library-exists false}}] + [shape-id file page libraries & {:keys [context local-refs] :or {context :not-component local-refs false}}] (let [shape (ctst/get-shape page shape-id)] (when (some? shape) (check-geometry shape file page) @@ -689,14 +695,14 @@ (report-error :nested-copy-not-allowed "Nested copy component only allowed inside other component" shape file page) - (check-shape-copy-root-nested shape file page libraries library-exists))))) + (check-shape-copy-root-nested shape file page libraries local-refs))))) (if (ctk/in-component-copy? shape) (if-not (#{:copy-top :copy-nested :copy-any} context) (report-error :not-head-copy-not-allowed "Non-root copy only allowed inside a copy" shape file page) - (check-shape-copy-not-root shape file page libraries)) + (check-shape-copy-not-root shape file page libraries local-refs)) (cond (#{:main-top :main-nested :main-any} context) diff --git a/common/test/common_tests/files/repair_components_test.cljc b/common/test/common_tests/files/repair_components_test.cljc index 68b9f4c4ee..677ef27558 100644 --- a/common/test/common_tests/files/repair_components_test.cljc +++ b/common/test/common_tests/files/repair_components_test.cljc @@ -16,7 +16,6 @@ [app.common.files.validate :as cfv] [app.common.logic.shapes :as cls] [app.common.test-helpers.components :as thc] - [app.common.test-helpers.composable.core :as tm] [app.common.test-helpers.compositions :as tho] [app.common.test-helpers.files :as thf] [app.common.test-helpers.ids-map :as thi] @@ -171,11 +170,8 @@ (t/is (= #{:invalid-main-instance-id :ref-shape-not-found} found)) (t/is (nil? errors)) (t/is (= (thi/id :main-root) (:main-instance-id (thc/get-component file' :component1)))) - ;; the :ref-shape-not-found repair, run in the same pass, detaches the copy - (tm/known-failure "F60" - [(thi/id :component1) (thi/id :main-root)] - [nil nil] - (copy-link file')))) + ;; the copy keeps its link + (t/is (= [(thi/id :component1) (thi/id :main-root)] (copy-link file'))))) (t/deftest invalid-main-instance-page (let [file (-> (simple) @@ -186,11 +182,8 @@ (t/is (= #{:invalid-main-instance-page :ref-shape-not-found} found)) (t/is (nil? errors)) (t/is (= (thi/id :page1) (:main-instance-page (thc/get-component file' :component1)))) - ;; the :ref-shape-not-found repair, run in the same pass, detaches the copy - (tm/known-failure "F60" - [(thi/id :component1) (thi/id :main-root)] - [nil nil] - (copy-link file')))) + ;; the copy keeps its link + (t/is (= [(thi/id :component1) (thi/id :main-root)] (copy-link file'))))) (t/deftest invalid-main-instance ;; the component names the copy as its main; the repair of @@ -199,11 +192,8 @@ (t/is (= #{:invalid-main-instance :invalid-main-instance-id :ref-shape-not-found} found)) (t/is (nil? errors)) (t/is (= (thi/id :main-root) (:main-instance-id (thc/get-component file' :component1)))) - ;; the :ref-shape-not-found repair, run in the same pass, detaches the copy - (tm/known-failure "F60" - [(thi/id :component1) (thi/id :main-root)] - [nil nil] - (copy-link file')))) + ;; the copy keeps its link + (t/is (= [(thi/id :component1) (thi/id :main-root)] (copy-link file'))))) (t/deftest shape-ref-in-main (let [[found file' errors] (repair (ths/update-shape (simple) :main-root :shape-ref (thi/id :copy-root)))] @@ -220,10 +210,10 @@ (t/is (nil? (:main-instance (ths/get-shape file' :main-child)))))) (t/deftest component-main-file-on-a-child - ;; the repair only removes :main-instance, so the error never goes away - (let [[found _ errors] (repair (ths/update-shape (simple) :main-child :component-file (thi/id :file1)))] + (let [[found file' errors] (repair (ths/update-shape (simple) :main-child :component-file (thi/id :file1)))] (t/is (= #{:component-main} found)) - (tm/known-failure "F47" nil #{:component-main} (codes errors)))) + (t/is (nil? errors)) + (t/is (nil? (:component-file (ths/get-shape file' :main-child)))))) (t/deftest should-not-be-component-root (let [[found file' errors] (repair (ths/update-shape (simple) :main-child :component-root true))] @@ -261,21 +251,83 @@ (t/is (= (thi/id :main2-nested-head) (:shape-ref (ths/get-shape file' :copy2-nested-head)))) (t/is (ctk/instance-head? (ths/get-shape file' :copy2-nested-head))))) +(t/deftest ref-shape-not-found-pointing-at-another-library-copy + ;; a library copy points at another copy of the same component: it is + ;; reassigned to the library main that one references + (let [library (-> (thf/sample-file :library :is-shared true :page-label :lib-page) + (tho/add-simple-component :component1 :lib-main :lib-main-child)) + file (-> (thf/sample-file :file1 :page-label :page1) + (thc/instantiate-component :component1 :copy1 :library library) + (thc/instantiate-component :component1 :copy2 :library library) + (ths/update-shape :copy2 :shape-ref (thi/id :copy1))) + [found file' errors] (repair file {(:id library) library})] + (t/is (= #{:ref-shape-not-found} found)) + (t/is (nil? errors)) + (t/is (= [(thi/id :component1) (thi/id :lib-main)] + ((juxt :component-id :shape-ref) (ths/get-shape file' :copy2)))))) + (t/deftest ref-shape-not-found-in-a-copy-child - ;; the reference of a non-head copy shape is never checked: the check reads - ;; the library from the shape itself, and only heads have one - (let [errors (cfv/validate-file (ths/update-shape (simple) :copy-child :shape-ref (uuid/next)) {})] - (tm/known-failure "F46" #{:ref-shape-not-found} #{} (codes errors)))) + (let [[found file' errors] (repair (ths/update-shape (simple) :copy-child :shape-ref (uuid/next)))] + (t/is (= #{:ref-shape-not-found} found)) + (t/is (nil? errors)) + ;; no matching shape: the child is detached, the copy keeps its link + (t/is (nil? (:shape-ref (ths/get-shape file' :copy-child)))) + (t/is (= [(thi/id :component1) (thi/id :main-root)] (copy-link file'))))) + +(t/deftest ref-shape-not-found-in-a-head-nested-twice + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (tho/add-two-levels-nested-component-with-copy + :component1 :main1-root :main1-child + :component2 :main2-root :nested-head1 + :component3 :main3-root :nested-head2 :nested-subhead2 + :copy-root + :copy2-root-params {:children-labels [:copy-head :copy-subhead :copy-rect]}) + (ths/update-shape :copy-subhead :shape-ref (uuid/next))) + errors (cfv/validate-file file {})] + (t/is (contains? (codes errors) :ref-shape-not-found)))) + +(t/deftest ref-shape-not-found-in-a-head-under-a-group + ;; component2 nests the copy of component1 inside a group + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (tho/add-simple-component :component1 :main1-root :main1-child) + (tho/add-frame :main2-root) + (tho/add-group :main2-group :parent-label :main2-root) + (thc/instantiate-component :component1 :main2-nested-head :parent-label :main2-group) + (thc/make-component :component2 :main2-root) + (thc/instantiate-component :component2 :copy-root + :children-labels [:copy-group :copy-nested-head :copy-child]) + (ths/update-shape :copy-nested-head :shape-ref (uuid/next))) + errors (cfv/validate-file file {})] + (t/is (contains? (codes errors) :ref-shape-not-found)))) + +(t/deftest library-main-child-deleted-before-the-update + ;; a library main deletes its nested copy; the copy in the consuming file + ;; still refers to it until the user takes the library update, which + ;; removes it + (let [library (-> (thf/sample-file :library :is-shared true :page-label :lib-page) + (tho/add-nested-component :component1 :main1-root :main1-child + :component2 :main2-root :main2-nested-head)) + file (-> (thf/sample-file :file1 :page-label :page1) + (thc/instantiate-component :component2 :copy2-root + :library library + :children-labels [:copy2-nested-head])) + page (thf/current-page library) + [_ del] (cls/generate-delete-shapes (pcb/empty-changes nil (:id page)) + (:data library) page (:objects page) + #{(thi/id :main2-nested-head)} {}) + library' (thf/apply-changes library del)] + (t/is (nil? (cfv/validate-file file {(:id library') library'}))))) (t/deftest ref-shape-is-head - ;; the check reports no component, so the repair cannot make the shape a - ;; head again + ;; a nested copy head that lost its component is made a head again (let [file (-> (nested) (ths/update-shape :copy2-nested-head :component-id nil) (ths/update-shape :copy2-nested-head :component-file nil)) - [found _ errors] (repair file)] + [found file' errors] (repair file)] (t/is (= #{:ref-shape-is-head} found)) - (tm/known-failure "F48" nil #{:ref-shape-is-head} (codes errors)))) + (t/is (nil? errors)) + (t/is (= [(thi/id :component1) (thi/id :file1)] + ((juxt :component-id :component-file) (ths/get-shape file' :copy2-nested-head)))))) (t/deftest ref-shape-is-not-head ;; a copy child made a head while the shape it references is not one diff --git a/common/test/common_tests/files/repair_test.cljc b/common/test/common_tests/files/repair_test.cljc index d5aba47c98..ebb77f6a77 100644 --- a/common/test/common_tests/files/repair_test.cljc +++ b/common/test/common_tests/files/repair_test.cljc @@ -15,6 +15,7 @@ [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] [app.common.test-helpers.shapes :as ths] @@ -275,3 +276,21 @@ (t/is (= uuid/zero (:parent-id shape1'))) (t/is (= uuid/zero (:parent-id shape2'))) (t/is (= #{(thi/id :shape1) (thi/id :shape2)} (set (:shapes root'))))))) + +(t/deftest repair-invalid-geometry-of-a-copy-shape + (t/testing "a repair is not an override: the copy shape it resets stays untouched, so sync gives it back its main's geometry" + (let [file (-> (thf/sample-file :file1 :page-label :page1) + (tho/add-simple-component :component1 :main-root :main-child) + (thc/instantiate-component :component1 :copy-root + :children-labels [:copy-child]) + (ths/update-shape :copy-child :width nil) + (ths/update-shape :copy-child :touched nil)) + + errors (cfv/validate-file file {}) + changes (cfr/repair-file file {} errors) + file' (thf/apply-changes file {:redo-changes changes} :validate? false) + child' (ths/get-shape file' :copy-child :page-label :page1)] + + (t/is (= [:invalid-geometry] (mapv :code errors))) + (t/is (nil? (cfv/validate-file file' {}))) + (t/is (nil? (:touched child'))))))