🐛 Keep the undo of a large component sync from overflowing

Undoing a main edit synced into thousands of copies overflowed the
stack, on the JVM and in JS (F84). Each synced shape added the undo of
its parents' update to the end of the undo list with a lazy concat, so
the list nested one lazy sequence per synced shape, and reading it
recursed through all of them. concat-changes and the inverse sync's
marking of local changes (a map over the whole list at every level of
the walk) added more.

- A shape's :reg-objects undo now goes right after the shape's own.
  Each parent is still updated after all its children are undone, and
  a synced parent gets back its stored values instead of recomputed
  ones; the undo round trip of every composable case is unchanged.
- concat-changes builds the joined undo list at once, at a cost that
  grows with the list it adds.
- The inverse sync collects the copy shapes it syncs and marks their
  changes as local once, at the end, instead of at every level.

large-sync-undo-test builds 5,000 copies in one step, syncs a main
edit into them and reads the undo list.

Benchmark (lower quantile, ms; baseline / now / ratio):
  deepest-edit-through-three-levels    35.2 /  37.3 / 1.06
  edit-through-a-chain-of-dependents   68.9 /  69.8 / 1.01
  library-update-into-copies           83.1 /  76.7 / 0.92
  main-edit-into-10-copies              1.8 /   1.6 / 0.88
  main-edit-into-100-copies            15.4 /  15.2 / 0.99
  main-edit-into-1000-copies          153.1 / 152.9 / 1.00
  variant-switch-into-copies           32.1 /  27.3 / 0.85
(Measured with the dead :shapes-group reads already removed; run side
by side, this commit and that state give the same ratios.)

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

common/src/app/common/files/changes_builder.cljc

@@ -134,10 +134,13 @@
 
 ;; redo-changes is a vector and :undo-changes is a list
 (defn concat-changes
+  "`changes1` followed by `changes2`: the redo changes of the second after
+  those of the first, its undo changes before. The undo list is built at
+  once, so joining many changes nests no lazy sequences."
   [changes1 changes2]
   (-> changes1
       (update :redo-changes d/concat-vec (:redo-changes changes2))
-      (update :undo-changes #(concat (:undo-changes changes2) %))))
+      (update :undo-changes #(reduce conj (or % '()) (reverse (:undo-changes changes2))))))
 
 ; TODO: remove this when not needed
 (defn- assert-page-id!

common/src/app/common/logic/libraries.cljc

@@ -97,6 +97,7 @@
 (declare sync-shape-recursive)
 (declare direct-sync)
 (declare inverse-sync)
+(declare mark-local-changes)
 
 (declare compare-children)
 (declare add-shape-to-instance)
@@ -611,18 +612,21 @@
                          (generate-rename-component (:component-id shape-inst) (:name shape-inst) library))]
 
     (if component
-      (sync-shape-recursive changes
-                            inverse-sync
-                            {:container     container
-                             :component     component
-                             :library       library
-                             :file          file
-                             :libraries     libraries
-                             :root-inst     shape-inst
-                             :root-main     (ctf/get-component-root library component)
-                             :initial-root? (:component-root shape-inst)}
-                            shape-inst
-                            (ctf/find-remote-shape container libraries shape-inst))
+      (let [synced-ids (volatile! #{})]
+        (-> (sync-shape-recursive changes
+                                  inverse-sync
+                                  {:container     container
+                                   :component     component
+                                   :library       library
+                                   :file          file
+                                   :libraries     libraries
+                                   :root-inst     shape-inst
+                                   :root-main     (ctf/get-component-root library component)
+                                   :initial-root? (:component-root shape-inst)
+                                   :synced-ids    synced-ids}
+                                  shape-inst
+                                  (ctf/find-remote-shape container libraries shape-inst))
+            (mark-local-changes @synced-ids)))
       changes)))
 
 (def ^:private inverse-sync
@@ -718,17 +722,22 @@
       false))
 
    :after-children
-   (fn [changes {:keys [shape-inst]}]
-     ;; The inverse sync may be made on a component that is inside a
-     ;; remote library. We need to separate changes that are from
-     ;; local and remote files.
-     (let [check-local (fn [change]
-                         (cond-> change
-                           (= (:id change) (:id shape-inst))
-                           (assoc :local-change? true)))]
-       (-> changes
-           (update :redo-changes (partial mapv check-local))
-           (update :undo-changes (partial map check-local)))))})
+   (fn [changes {:keys [shape-inst synced-ids]}]
+     (vswap! synced-ids conj (:id shape-inst))
+     changes)})
+
+(defn- mark-local-changes
+  "Mark as `:local-change?` the changes of the copy shapes `ids`. The inverse
+  sync may update a component of a remote library, so the frontend commits
+  the copy's changes and the main's apart."
+  [changes ids]
+  (let [check-local (fn [change]
+                      (cond-> change
+                        (contains? ids (:id change))
+                        (assoc :local-change? true)))]
+    (-> changes
+        (update :redo-changes #(mapv check-local %))
+        (update :undo-changes #(apply list (map check-local %))))))
 
 
 ;; ---- Operation generation helpers ----
@@ -1284,15 +1293,16 @@
                                     container
                                     {:type :reg-objects
                                      :shapes all-parents}))
+        ;; undone as the shape first, then its parents
+        (update :undo-changes conj (make-change
+                                    container
+                                    {:type :reg-objects
+                                     :shapes all-parents}))
         (update :undo-changes conj (make-change
                                     container
                                     {:type :mod-obj
                                      :id (:id dest-shape)
                                      :operations (vec uoperations)}))
-        (update :undo-changes concat [(make-change
-                                       container
-                                       {:type :reg-objects
-                                        :shapes all-parents})])
         (pcb/apply-changes-local))))
 
 

common/test/common_tests/logic/large_sync_undo_test.cljc

@@ -0,0 +1,70 @@
+;; This Source Code Form is subject to the terms of the Mozilla Public
+;; License, v. 2.0. If a copy of the MPL was not distributed with this
+;; file, You can obtain one at http://mozilla.org/MPL/2.0/.
+;;
+;; Copyright (c) KALEIDOS SUBSIDIARY SL
+
+(ns common-tests.logic.large-sync-undo-test
+  (:require
+   [app.common.files.changes-builder :as pcb]
+   [app.common.geom.point :as gpt]
+   [app.common.logic.libraries :as cll]
+   [app.common.logic.shapes :as cls]
+   [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]
+   [app.common.types.container :as ctn]
+   [app.common.types.file :as ctf]
+   [app.common.types.pages-list :as ctpl]
+   [app.common.uuid :as uuid]
+   [clojure.test :as t]))
+
+(t/use-fixtures :each thi/test-fixture)
+
+(defn- add-copies
+  "`file` with `n` copies of the component `component-label` at the root of
+   its page, added in one step (the copies of a large design system)."
+  [file component-label n]
+  (let [page      (thf/current-page file)
+        component (thc/get-component file component-label)
+        copies    (map (fn [_]
+                         (second (ctn/make-component-instance page component (:data file)
+                                                              (gpt/point 100 100) {})))
+                       (range n))]
+    (ctf/update-file-data
+     file
+     (fn [data]
+       (ctpl/update-page data (:id page)
+                         (fn [page]
+                           (-> page
+                               (update :objects into (comp cat (map (juxt :id identity))) copies)
+                               (update-in [:objects uuid/zero :shapes] into (map (comp :id first) copies)))))))))
+
+(defn- read-count
+  "How many changes `changes` holds, or `:stack-overflow` when reading them
+   overflows the stack."
+  [changes]
+  (try
+    (count changes)
+    (catch #?(:clj StackOverflowError :cljs js/RangeError) _
+      :stack-overflow)))
+
+(t/deftest undo-of-a-sync-into-many-copies-can-be-read
+  ;; A large sync builds a long undo list; reading it must not recurse once
+  ;; per synced shape (F84).
+  (let [file    (-> (thf/sample-file :file1)
+                    (tho/add-simple-component :component1 :main-root :main-child)
+                    (add-copies :component1 5000))
+        page    (thf/current-page file)
+        edit    (cls/generate-update-shapes (pcb/empty-changes nil (:id page))
+                                            #{(thi/id :main-child)}
+                                            #(assoc % :fills (ths/sample-fills-color :fill-color "#ff0000"))
+                                            (:objects page)
+                                            {})
+        file    (thf/apply-changes file edit :validate? false)
+        sync    (cll/generate-sync-file-changes (pcb/empty-changes) nil :components (:id file)
+                                                (thi/id :component1) (:id file) {(:id file) file} (:id file))]
+    (t/is (= 10000 (count (:redo-changes sync))))
+    (t/is (= 10000 (read-count (:undo-changes sync))))))

common/test/common_tests/runner.cljc

@@ -73,6 +73,7 @@
    [common-tests.logic.composable-sync-test]
    [common-tests.logic.copying-and-duplicating-test]
    [common-tests.logic.duplicated-pages-test]
+   [common-tests.logic.large-sync-undo-test]
    [common-tests.logic.library-asset-sync-test]
    [common-tests.logic.move-shapes-test]
    [common-tests.logic.multiple-nesting-levels-test]
@@ -198,6 +199,7 @@
    'common-tests.logic.composable-sync-test
    'common-tests.logic.copying-and-duplicating-test
    'common-tests.logic.duplicated-pages-test
+   'common-tests.logic.large-sync-undo-test
    'common-tests.logic.library-asset-sync-test
    'common-tests.logic.move-shapes-test
    'common-tests.logic.multiple-nesting-levels-test