🐛 Detect and repair children missing from their parent's list (#12216)

* 🐛 Detect and repair children missing from their parent's list

A shape could point to an existing parent that did not list it in its
:shapes. The validator missed it, since it only walks the tree from
the root plus the shapes whose parent does not exist. Operations that
walk :shapes missed it too: deleting a variant also deleted its
container, which looked empty, and left the other variants without a
parent, so the backend rejected the change and the file stopped
saving.

- get-orphan-shapes also returns shapes not listed by their parent,
  so :child-not-in-parent is reported.
- The :child-not-in-parent repair adds every unlisted child of the
  parent. All repairs share the same file data and each one sets the
  whole :shapes, so with several children only the last one was kept.
- New migration 0032-fix-children-not-in-parent repairs the affected
  files when they are opened.

Closes #11758

AI-assisted-by: claude-opus-5-5

* 🐛 List shapes moved to root by the parent-not-found repair

The :parent-not-found repair set the shape's parent to the root but
did not add it to the root's :shapes, so it left the shape in the
child-not-in-parent state that the validator now reports. It now
adds every shape without parent to the root at once, since all
repairs are built from the same file data and each one sets the
whole :shapes.

Also cover the missing parent case in the repair tests.

AI-assisted-by: claude-opus-5-5

* ♻️ Simplify unlisted children validation and repair

Address review comments on the validation and repair of shapes that
their parent does not list.

- get-orphan-shapes no longer rebuilds *children-sets*: both callers
  bind it. The lookup moves to a listed-in-parent? helper shared with
  check-parent-children, which scans :shapes when the var is unbound.
- The :child-not-in-parent and :parent-not-found repairs no longer
  walk every shape of the page. Each one moves only its own shape with
  pcb/change-parent. The :mov-objects change adds the shape to the
  parent's :shapes when applied, so several repairs on one parent no
  longer overwrite each other.
- The :child-not-in-parent repair test uses plain shapes instead of
  variants.

AI-assisted-by: claude-opus-5-5

common/src/app/common/files/migrations.cljc

@@ -2179,6 +2179,44 @@
         (update :pages-index d/update-vals repair-container)
         (d/update-when :components d/update-vals repair-container))))
 
+;; A shape can point to an existing parent that does not list it in its
+;; `:shapes`. Such shapes are unreachable from the root, and operations
+;; that walk the tree through `:shapes` miss them; e.g. deleting a variant
+;; also deleted its container, which looked empty, and left the other
+;; variants pointing to a deleted parent. Add them back to their parent.
+(defmethod migrate-data "0032-fix-children-not-in-parent"
+  [data _]
+  (letfn [(get-unlisted-children [objects]
+            ;; Returns a {parent-id -> [child-id]} map
+            (let [listed (reduce-kv (fn [result id shape]
+                                      (if-let [shapes (not-empty (:shapes shape))]
+                                        (assoc result id (set shapes))
+                                        result))
+                                    {}
+                                    objects)]
+              (reduce-kv (fn [result id shape]
+                           (let [parent-id (:parent-id shape)]
+                             (if (and (not (cfh/root? shape))
+                                      (contains? objects parent-id)
+                                      (not (contains? (get listed parent-id) id)))
+                               (update result parent-id (fnil conj []) id)
+                               result)))
+                         {}
+                         objects)))
+
+          (fix-objects [objects]
+            (reduce-kv (fn [objects parent-id child-ids]
+                         (update-in objects [parent-id :shapes] (fnil into []) child-ids))
+                       objects
+                       (get-unlisted-children objects)))
+
+          (update-container [container]
+            (d/update-when container :objects fix-objects))]
+
+    (-> data
+        (update :pages-index d/update-vals update-container)
+        (d/update-when :components d/update-vals update-container))))
+
 (def available-migrations
   (into (d/ordered-set)
         ["legacy-2"
@@ -2267,4 +2305,5 @@
          "0028-normalize-constrained-values"
          "0029-move-background-blur-out-of-blur"
          "0030-remove-stroke-per-side-attr"
-         "0031-migrate-stroke-width-token-attr"]))
+         "0031-migrate-stroke-width-token-attr"
+         "0032-fix-children-not-in-parent"]))

common/src/app/common/files/repair.cljc

@@ -45,29 +45,25 @@
 
 (defmethod repair-error :parent-not-found
   [_ {:keys [shape page-id] :as error} file-data _]
-  (let [repair-shape
-        (fn [shape]
-          ;; Set parent to root frame.
-          (log/debug :hint "  -> set to " :parent-id uuid/zero)
-          (assoc shape :parent-id uuid/zero))]
-
-    (log/debug :hint "repairing shape :parent-not-found" :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 [(:id shape)] repair-shape))))
+  ;; Move the shape to the root frame. `:mov-objects` adds it to the
+  ;; root's children list when applied, so several repairs on the same
+  ;; root do not overwrite each other
+  (log/debug :hint "repairing shape :parent-not-found" :id (:id shape) :name (:name shape) :page-id page-id)
+  (log/debug :hint "  -> set to " :parent-id uuid/zero)
+  (-> (pcb/empty-changes nil page-id)
+      (pcb/with-file-data file-data)
+      (pcb/change-parent uuid/zero [shape] nil {:allow-altering-copies true})))
 
 (defmethod repair-error :child-not-in-parent
   [_ {:keys [shape page-id] :as error} file-data _]
-  (let [repair-shape
-        (fn [parent-shape]
-          ;; Add shape to parent's children list
-          (log/debug :hint "  -> add children to" :parent-id (:id parent-shape))
-          (update parent-shape :shapes conj (:id shape)))]
-
-    (log/debug :hint "repairing shape :child-not-in-parent" :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 [(:parent-id shape)] repair-shape))))
+  ;; Add the shape to its parent's children list. `:mov-objects` adds it
+  ;; when applied, so several repairs on the same parent do not
+  ;; overwrite each other
+  (log/debug :hint "repairing shape :child-not-in-parent" :id (:id shape) :name (:name shape) :page-id page-id)
+  (log/debug :hint "  -> add children to" :parent-id (:parent-id shape))
+  (-> (pcb/empty-changes nil page-id)
+      (pcb/with-file-data file-data)
+      (pcb/change-parent (:parent-id shape) [shape] nil {:allow-altering-copies true})))
 
 (defmethod repair-error :duplicated-children
   [_ {:keys [shape page-id] :as error} file-data _]

common/src/app/common/files/validate.cljc

@@ -118,6 +118,19 @@
              {}
              objects))
 
+(defn- listed-in-parent?
+  "Check if `parent` lists `shape-id` in its `:shapes`. A `nil` parent
+  lists nothing.
+
+  Fast path: O(1) set lookup when `*children-sets*` is pre-computed for
+  the current page. Falls back to an O(k) linear scan via `some` when
+  the var is unbound (e.g. in isolated single-shape validation calls
+  outside a full page pass)."
+  [parent shape-id]
+  (if-let [cs *children-sets*]
+    (contains? (get cs (:id parent)) shape-id)
+    (some #(= shape-id %) (:shapes parent))))
+
 (defn- find-ref-shape*
   "Cached wrapper around `ctf/find-ref-shape` with `:include-deleted? true`.
   When `*ref-shape-cache*` is bound, each shape-id is resolved at most once
@@ -195,13 +208,7 @@
                     shape file page)
       (do
         (when-not (cfh/root? shape)
-          ;; Fast path: O(1) set lookup when `*children-sets*` is pre-computed
-          ;; for the current page.  Falls back to an O(k) linear scan via
-          ;; `some` when the var is unbound (e.g. in isolated single-shape
-          ;; validation calls outside a full page pass).
-          (when-not (if-let [cs *children-sets*]
-                      (contains? (get cs (:id parent)) shape-id)
-                      (some #(= shape-id %) (:shapes parent)))
+          (when-not (listed-in-parent? parent shape-id)
             (report-error :child-not-in-parent
                           (str/ffmt "Shape % not in parent's children list" shape-id)
                           shape file page)))
@@ -797,9 +804,10 @@
     (check-variant-component component file)))
 
 (defn- get-orphan-shapes
-  "Return the ids of shapes whose parent does not exist in the objects
-  map (i.e. shapes unreachable from the root traversal). The root
-  shape itself is excluded since it is always validated separately.
+  "Return the ids of shapes unreachable from the root traversal: shapes
+  whose parent does not exist in the objects map, or whose parent exists
+  but does not list them in its `:shapes`. The root shape itself is
+  excluded since it is always validated separately.
 
   Implemented with `reduce-kv` rather than a `map`/`filter` pipeline.
   The previous implementation mapped over `objects` and returned a
@@ -810,8 +818,10 @@
   [{:keys [objects] :as _page}]
   (persistent!
    (reduce-kv (fn [result id shape]
+                ;; A missing parent is `nil`, which lists nothing, so this
+                ;; also covers shapes whose parent does not exist
                 (if (and (not (cfh/root? shape))
-                         (not (contains? objects (:parent-id shape))))
+                         (not (listed-in-parent? (get objects (:parent-id shape)) id)))
                   (conj! result id)
                   result))
               (transient [])

common/test/common_tests/files/repair_test.cljc

@@ -228,3 +228,50 @@
       (t/is (nil? errors'))
       (t/is (= "Group / Subgroup" (:path comp1')))
       (t/is (= "Component" (:name comp1'))))))
+
+(t/deftest repair-child-not-in-parent
+  (t/testing "detect and repair shapes whose parent exists but does not list them"
+    (let [file    (-> (thf/sample-file :file1 :page-label :page1)
+                      (ths/add-sample-shape :frame1 :type :frame)
+                      (ths/add-sample-shape :shape1 :parent-label :frame1)
+                      (ths/add-sample-shape :shape2 :parent-label :frame1)
+                      (ths/update-shape :frame1 :shapes []))
+
+          errors  (cfv/validate-file file {})
+          changes (cfr/repair-file file {} errors)
+          file'   (thf/apply-changes file {:redo-changes changes} :validate? false)
+          errors' (cfv/validate-file file' {})
+
+          frame1' (ths/get-shape file' :frame1 :page-label :page1)]
+
+      (t/is (= #{:child-not-in-parent} (into #{} (map :code) errors)))
+      (t/is (= #{(thi/id :shape1) (thi/id :shape2)} (into #{} (map :shape-id) errors)))
+
+      (t/is (nil? errors'))
+      (t/is (= #{(thi/id :shape1) (thi/id :shape2)} (set (:shapes frame1')))))))
+
+(t/deftest repair-parent-not-found
+  (t/testing "detect and repair shapes whose parent does not exist"
+    (let [file    (-> (thf/sample-file :file1 :page-label :page1)
+                      (ths/add-sample-shape :shape1)
+                      (ths/add-sample-shape :shape2)
+                      (ths/update-shape :shape1 :parent-id (uuid/next))
+                      (ths/update-shape :shape2 :parent-id (uuid/next))
+                      (ths/update-shape-by-id uuid/zero :shapes []))
+
+          errors  (cfv/validate-file file {})
+          changes (cfr/repair-file file {} errors)
+          file'   (thf/apply-changes file {:redo-changes changes} :validate? false)
+          errors' (cfv/validate-file file' {})
+
+          shape1' (ths/get-shape file' :shape1 :page-label :page1)
+          shape2' (ths/get-shape file' :shape2 :page-label :page1)
+          root'   (ths/get-shape-by-id file' uuid/zero :page-label :page1)]
+
+      (t/is (= #{:parent-not-found} (into #{} (map :code) errors)))
+      (t/is (= #{(thi/id :shape1) (thi/id :shape2)} (into #{} (map :shape-id) errors)))
+
+      (t/is (nil? errors'))
+      (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')))))))

common/test/common_tests/files_migrations_test.cljc

@@ -554,3 +554,54 @@
     (t/is (contains? (:migrations file') migration-id) "migration id persisted")
     (t/is (= "30" (:stroke-width-top applied'))
           "legacy token applied to the sides")))
+
+(t/deftest migration-0032-adds-unlisted-children-to-their-parent
+  (let [migration-id "0032-fix-children-not-in-parent"
+        page-id      (uuid/next)
+        parent-id    (uuid/next)
+        listed-id    (uuid/next)
+        unlisted1-id (uuid/next)
+        unlisted2-id (uuid/next)
+        orphan-id    (uuid/next)
+        root         (cts/setup-shape {:id uuid/zero :type :frame :shapes [parent-id orphan-id]})
+        parent       (cts/setup-shape {:id parent-id :type :frame :parent-id uuid/zero
+                                       :shapes [listed-id]})
+        child        (fn [id parent-id]
+                       (cts/setup-shape {:id id :type :rect :parent-id parent-id}))
+        data         {:pages-index
+                      {page-id
+                       {:objects {uuid/zero    root
+                                  parent-id    parent
+                                  listed-id    (child listed-id parent-id)
+                                  unlisted1-id (child unlisted1-id parent-id)
+                                  unlisted2-id (child unlisted2-id parent-id)
+                                  orphan-id    (child orphan-id (uuid/next))}}}}
+        data'        (cfm/migrate-data data migration-id)
+        objects'     (get-in data' [:pages-index page-id :objects])
+        shapes'      (get-in objects' [parent-id :shapes])]
+
+    (t/is (= listed-id (first shapes')) "already listed children keep their place")
+    (t/is (= #{listed-id unlisted1-id unlisted2-id} (set shapes'))
+          "unlisted children added to the parent")
+    (t/is (= 3 (count shapes')) "no duplicated children")
+    (t/is (= [parent-id orphan-id] (get-in objects' [uuid/zero :shapes]))
+          "root unchanged")
+    (t/is (= (get-in data [:pages-index page-id :objects orphan-id])
+             (get objects' orphan-id))
+          "shapes whose parent does not exist are left untouched")
+    (t/is (= data' (cfm/migrate-data data' migration-id))
+          "migration is idempotent")))
+
+(t/deftest migration-0032-adds-unlisted-children-in-components
+  (let [migration-id "0032-fix-children-not-in-parent"
+        component-id (uuid/next)
+        main-id      (uuid/next)
+        child-id     (uuid/next)
+        data         {:components
+                      {component-id
+                       {:objects {main-id  (cts/setup-shape {:id main-id :type :frame :shapes []})
+                                  child-id (cts/setup-shape {:id child-id :type :rect
+                                                             :parent-id main-id})}}}}
+        data'        (cfm/migrate-data data migration-id)]
+
+    (t/is (= [child-id] (get-in data' [:components component-id :objects main-id :shapes])))))