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

🐛 Fix component validation and repair

Browse Source 
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
This commit is contained in:
alonso.torres 2026-10-15 15:10:09 +02:00
parent b3a93655cf
commit 44c1cb89c1
4 changed files with 158 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -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`."
@ -180,7 +181,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 _]
@ -198,7 +202,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)]
@ -215,7 +219,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))))))]
@ -580,6 +584,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))
@ -591,7 +609,7 @@
data
libraries)))
(pcb/empty-changes nil)
errors)]
(repairable-now errors))]
redo-changes))
(defn- error-fingerprint

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

@ -351,13 +351,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."
@ -375,7 +372,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."
@ -504,36 +503,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)"
@ -547,16 +552,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
@ -671,7 +677,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)
@ -716,14 +722,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)

106
common/test/common_tests/files/repair_components_test.cljc
View File

@ -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

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

@ -546,3 +546,20 @@
;; First iteration used the seed; errors-fn runs only after the repair.
(t/is (= 1 @calls)))))
(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'))))))