From 05e03e2673c71b64f4b68580208c2b362851856e Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 13 Oct 2026 12:53:07 +0000 Subject: [PATCH] :zap: Stop resending deleted component objects on every touch `pcb/update-component` attached the whole `:objects` map of a deleted component to every `mod-component` change, so a single component swap re-sent thousands of shapes that the same batch already carried as shape changes. In one measured swap the 49 maps with objects were 94% of the request (6.3 MB), and only 27% of the 4.275 shapes they carried differed from what the server had. Now `mod-component` carries component metadata only, and shapes inside a component travel as `:mod-obj` changes scoped with `:component-id`, which is what the component sync already emits. An absent `:objects` key means "leave the stored shapes alone", only an explicit `nil` drops them, so metadata-only changes cannot wipe a deleted component. The two repairs that fix shapes inside a component (shape-ref cycle and duplicated swap slot) now emit one `:mod-obj` per shape; the two that move or clear the `:objects` key keep the whole-map write behind the new `:objects? true` option. Same swap on the measured file: 6.328.307 to 382.571 bytes. AI-assisted-by: space-bunny --- .../memories/common/changes-architecture.md | 3 +- .../src/app/common/files/changes_builder.cljc | 19 +++- common/src/app/common/files/repair.cljc | 81 ++++++++--------- .../src/app/common/types/components_list.cljc | 10 ++- .../test/common_tests/files/repair_test.cljc | 52 +++++++++++ .../test/common_tests/files_changes_test.cljc | 87 +++++++++++++++++++ 6 files changed, 204 insertions(+), 48 deletions(-) diff --git a/.serena/memories/common/changes-architecture.md b/.serena/memories/common/changes-architecture.md index 82da7bb71e..32b811fd62 100644 --- a/.serena/memories/common/changes-architecture.md +++ b/.serena/memories/common/changes-architecture.md @@ -8,6 +8,7 @@ Each change is a map such as `{:type ... :id ... :page-id ...}`. Common families - `:add-obj`, `:mod-obj`, `:del-obj`: shape lifecycle. `:mod-obj` contains `:operations`, commonly `{:type :set :attr ... :val ... :ignore-geometry ... :ignore-touched ...}` or `{:type :set-touched ...}`. - `:add-component`, `:mod-component`, `:del-component`: component/library metadata. +- Shapes stored inside the component record (deleted components keep them in `:objects`) are never written as a whole map: `mod-component` carries only component metadata by default, and shapes travel as `:mod-obj`/`:add-obj`/`:del-obj` changes scoped with `:component-id`. `pcb/update-component` has an `:objects? true` option for the two repairs that move or clear that key, and `ctkl/mod-component` reads an absent `:objects` as "leave the stored shapes alone" (only an explicit `nil` drops them). Before this, one component swap resent the full object map of every deleted component it touched (~6 MB in the request); do not reintroduce it. - `:add-children`, `:remove-children`, `:reg-objects`: tree and object-map edits. - `:set-option`, `:add-page`, `:mov-page`, and related file/page metadata changes. @@ -21,7 +22,7 @@ High-value builder operations: - `pcb/with-page-id`, `pcb/with-objects`, `pcb/with-library-data`: set context for following operations. - `pcb/update-shapes ids update-fn`: emits `:mod-obj` with diff-derived `:set` ops. Options include `{:with-objects? true}`, `{:ignore-touched true}`, and `{:attrs #{...}}`. - `pcb/add-objects`, `pcb/change-parent`, `pcb/remove-objects`, `pcb/resize-parents`: shape/tree edits. -- `pcb/add-component`, `pcb/update-component`, `pcb/mod-component`: component/library edits. +- `pcb/add-component`, `pcb/update-component`, `pcb/mod-component`: component/library edits. Repairs that fix shapes inside a component use `pcb/with-container` + `pcb/with-objects` + `pcb/update-shapes` so the changes come out scoped with `:component-id`. - `pcb/set-translation? true`: marks the whole change set as translation-only, which lets component sync skip expensive work. ## Applying changes in tests diff --git a/common/src/app/common/files/changes_builder.cljc b/common/src/app/common/files/changes_builder.cljc index 1e71d05b4d..31eeacefc5 100644 --- a/common/src/app/common/files/changes_builder.cljc +++ b/common/src/app/common/files/changes_builder.cljc @@ -924,7 +924,14 @@ (apply-changes-local {:apply-to-library? apply-changes-local-library?}))))) (defn update-component - [changes id update-fn & {:keys [apply-changes-local-library?]}] + "Update a component record. + + By default the change carries only component metadata. Shapes stored inside + the component (deleted components keep them in `:objects`) travel as + `:mod-obj` changes scoped with `:component-id`, which is what the component + sync emits. Pass `:objects? true` to rewrite the whole `:objects` map, only + needed by the repairs that move or clear that key." + [changes id update-fn & {:keys [apply-changes-local-library? objects?]}] (assert-library! changes) (let [library-data (::library-data (meta changes)) prev-component (get-in library-data [:components id]) @@ -938,8 +945,10 @@ :main-instance-id (:main-instance-id new-component) :main-instance-page (:main-instance-page new-component) :annotation (:annotation new-component) - :objects (:objects new-component) ;; for deleted components :modified-at (:modified-at new-component)} + + objects? (assoc :objects (:objects new-component)) + (some? (:variant-id new-component)) (assoc :variant-id (:variant-id new-component)) (nil? (:variant-id new-component)) @@ -954,8 +963,10 @@ :path (:path prev-component) :main-instance-id (:main-instance-id prev-component) :main-instance-page (:main-instance-page prev-component) - :annotation (:annotation prev-component) - :objects (:objects prev-component)} + :annotation (:annotation prev-component)} + + objects? (assoc :objects (:objects prev-component)) + (some? (:variant-id prev-component)) (assoc :variant-id (:variant-id prev-component)) (nil? (:variant-id prev-component)) diff --git a/common/src/app/common/files/repair.cljc b/common/src/app/common/files/repair.cljc index a8d98a2d87..6debc034ba 100644 --- a/common/src/app/common/files/repair.cljc +++ b/common/src/app/common/files/repair.cljc @@ -26,6 +26,20 @@ (defmulti repair-error (fn [code _error _file-data _libraries] code)) +(defn- update-component-shapes + "Update shapes stored inside a component `:objects` map. + + Emits one `:mod-obj` change per shape, scoped with `:component-id`, instead + of rewriting the whole map through a `:mod-component` change. `:ignore-touched` + keeps the value the caller computed: no touched group is merged on top." + [component objects ids update-fn] + (-> (pcb/empty-changes nil) + ;; A made container, so `update-shapes` scopes the changes to the + ;; component instead of to a page. + (pcb/with-container {:id (:id component) :type :component}) + (pcb/with-objects objects) + (pcb/update-shapes ids update-fn {:ignore-touched true}))) + (defmethod repair-error :invalid-geometry [_ {:keys [shape page-id] :as error} file-data _] (let [repair-shape @@ -372,33 +386,26 @@ (pcb/update-shapes [(:id shape)] repair-shape)))) (defmethod repair-error :shape-ref-cycle - [_ {:keys [shape args] :as error} file-data _] - (let [repair-component - (fn [component] - (let [objects (:objects component) - ;; we only have encounter this on deleted components, - ;; so the relevant objects are inside the component - to-detach (->> (:cycles-ids args) - (map #(get objects %)) - (map #(ctn/get-head-shape objects %)) - (map :id) - distinct - (mapcat #(ctn/get-children-in-instance objects %)) - (map :id) - set)] + [_ {:keys [shape args] :as error} _file-data _] + (let [objects (:objects shape) + ;; we only have encounter this on deleted components, + ;; so the relevant objects are inside the component + to-detach (->> (:cycles-ids args) + (map #(get objects %)) + (map #(ctn/get-head-shape objects %)) + (map :id) + distinct + (mapcat #(ctn/get-children-in-instance objects %)) + (map :id) + set) + + detach-shape + (fn [shape] + (log/debug :hint " -> detach shape" :shape-id (:id shape)) + (ctk/detach-shape shape))] - (update component :objects - (fn [objects] - (reduce-kv (fn [acc k v] - (if (contains? to-detach k) - (assoc acc k (ctk/detach-shape v)) - (assoc acc k v))) - {} - objects)))))] (log/debug :hint "repairing component :shape-ref-cycle" :id (:id shape) :name (:name shape)) - (-> (pcb/empty-changes nil nil) - (pcb/with-library-data file-data) - (pcb/update-component (:id shape) repair-component)))) + (update-component-shapes shape objects to-detach detach-shape))) (defmethod repair-error :shape-ref-in-main [_ {:keys [shape page-id] :as error} file-data _] @@ -540,7 +547,7 @@ (log/debug :hint "repairing component :component-nil-objects-not-allowed" :id (:id component) :name (:name component)) (-> (pcb/empty-changes nil) (pcb/with-library-data file-data) - (pcb/update-component (:id component) repair-component)))) + (pcb/update-component (:id component) repair-component {:objects? true})))) (defmethod repair-error :non-deleted-component-cannot-have-objects [_ {component :shape} file-data _] ; in this error the :shape argument is the component @@ -556,7 +563,7 @@ (log/debug :hint "repairing component :non-deleted-component-cannot-have-objects" :id (:id component) :name (:name component)) (-> (pcb/empty-changes nil) (pcb/with-library-data file-data) - (pcb/update-component (:id component) repair-component)))) + (pcb/update-component (:id component) repair-component {:objects? true})))) (defmethod repair-error :invalid-text-touched [_ {:keys [shape page-id] :as error} file-data _] @@ -608,7 +615,7 @@ (pcb/update-shapes (map :id child-with-duplicate) repair-shape)))) (defmethod repair-error :component-duplicate-slot - [_ {:keys [shape] :as error} file-data _] + [_ {:keys [shape] :as error} _file-data _] (let [main-shape (get-in shape [:objects (:main-instance-id shape)]) childs (map #(get (:objects shape) %) (:shapes main-shape)) childs-with-duplicate (let [result (reduce (fn [[seen duplicates] item] @@ -620,20 +627,14 @@ childs)] (second result)) duplicated-ids (set (mapv :id childs-with-duplicate)) - repair-component - (fn [component] - (let [objects (reduce-kv (fn [acc k v] - (if (contains? duplicated-ids k) - (assoc acc k (ctk/remove-swap-slot v)) - (assoc acc k v))) - {} - (:objects component))] - (assoc component :objects objects)))] + repair-shape + (fn [shape] + ;; Remove the swap slot + (log/debug :hint " -> remove swap-slot" :child-id (:id shape)) + (ctk/remove-swap-slot shape))] (log/debug :hint "repairing component :component-duplicated-slot" :id (:id shape) :name (:name shape)) - (-> (pcb/empty-changes nil) - (pcb/with-library-data file-data) - (pcb/update-component (:id shape) repair-component)))) + (update-component-shapes shape (:objects shape) duplicated-ids repair-shape))) (defmethod repair-error :missing-slot [_ {:keys [shape page-id args] :as error} file-data _] diff --git a/common/src/app/common/types/components_list.cljc b/common/src/app/common/types/components_list.cljc index be92b16999..9935f85e2a 100644 --- a/common/src/app/common/types/components_list.cljc +++ b/common/src/app/common/types/components_list.cljc @@ -41,7 +41,7 @@ variant-properties (update-in [:components id] assoc :variant-properties variant-properties)))) (defn mod-component - [file-data {:keys [id name path main-instance-id main-instance-page objects annotation variant-id variant-properties modified-at]}] + [file-data {:keys [id name path main-instance-id main-instance-page objects annotation variant-id variant-properties modified-at] :as change}] (d/update-in-when file-data [:components id] (fn [component] (let [new-comp (cond-> component @@ -57,10 +57,14 @@ (some? main-instance-page) (assoc :main-instance-page main-instance-page) - (some? objects) + ;; An absent `:objects` key means "leave the stored + ;; shapes alone", only an explicit `nil` drops them. + ;; Without this distinction a metadata-only change + ;; would wipe the shapes of a deleted component. + (and (contains? change :objects) (some? objects)) (assoc :objects objects) - (nil? objects) + (and (contains? change :objects) (nil? objects)) (dissoc :objects) (some? modified-at) diff --git a/common/test/common_tests/files/repair_test.cljc b/common/test/common_tests/files/repair_test.cljc index d5aba47c98..45fb9e23c4 100644 --- a/common/test/common_tests/files/repair_test.cljc +++ b/common/test/common_tests/files/repair_test.cljc @@ -12,6 +12,7 @@ generate accurate errors, and that the repair functions return the file to a stable state." (:require + [app.common.files.changes :as ch] [app.common.files.repair :as cfr] [app.common.files.validate :as cfv] [app.common.test-helpers.components :as thc] @@ -19,6 +20,8 @@ [app.common.test-helpers.ids-map :as thi] [app.common.test-helpers.shapes :as ths] [app.common.test-helpers.variants :as thv] + [app.common.types.component :as ctk] + [app.common.types.shape :as cts] [app.common.uuid :as uuid] [clojure.test :as t])) @@ -229,6 +232,55 @@ (t/is (= "Group / Subgroup" (:path comp1'))) (t/is (= "Component" (:name comp1')))))) +(t/deftest repair-component-duplicate-slot + (t/testing "repair a deleted component whose children share a swap slot, with changes scoped to the component" + (let [component-id (uuid/next) + main-id (uuid/next) + child1 (uuid/next) + child2 (uuid/next) + slot (uuid/next) + child (fn [id] + (ctk/set-swap-slot + (cts/setup-shape + {:id id :type :rect :name "Child" + :frame-id main-id :parent-id main-id + :x 0 :y 0 :width 10 :height 10 + :touched #{:geometry-group}}) + slot)) + component {:id component-id + :name "Component" :path "" + :main-instance-id main-id + :deleted true + :objects {main-id (cts/setup-shape + {:id main-id :type :frame :name "Main" + :frame-id uuid/zero :parent-id uuid/zero + :shapes [child1 child2]}) + child1 (child child1) + child2 (child child2)}} + changes (cfr/repair-error :component-duplicate-slot + {:code :component-duplicate-slot :shape component} + {:components {component-id component}} + {}) + change (first (:redo-changes changes)) + data' (ch/process-changes {:components {component-id component}} (:redo-changes changes)) + objects' (:objects (get-in data' [:components component-id]))] + + (t/is (= 1 (count (:redo-changes changes)))) + (t/is (= :mod-obj (:type change)) + "the repair updates one shape instead of rewriting the whole objects map") + (t/is (= component-id (:component-id change))) + (t/is (= child2 (:id change)) + "the second child is the one carrying the duplicated slot") + (t/is (= [{:type :set :attr :touched :val #{:geometry-group} + :ignore-geometry false :ignore-touched true}] + (:operations change))) + + (t/testing "applying the repair drops the slot and leaves the rest alone" + (t/is (= 3 (count objects'))) + (t/is (nil? (ctk/get-swap-slot (get objects' child2)))) + (t/is (= #{:geometry-group} (:touched (get objects' child2)))) + (t/is (= slot (ctk/get-swap-slot (get objects' child1)))))))) + (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) diff --git a/common/test/common_tests/files_changes_test.cljc b/common/test/common_tests/files_changes_test.cljc index 935ff092fb..9e7423cb79 100644 --- a/common/test/common_tests/files_changes_test.cljc +++ b/common/test/common_tests/files_changes_test.cljc @@ -119,6 +119,93 @@ (t/testing label (assert-idempotent data [change]))))) +(defn- make-deleted-component + "File data with a deleted component, so it keeps the shapes of its main + instance inside its own `:objects` map. Returns the file data and the ids + needed to address those shapes." + [] + (let [file-id (uuid/custom 20 1) + page-id (uuid/custom 20 2) + main-id (uuid/custom 20 3) + child-id (uuid/custom 20 4) + component-id (uuid/custom 20 5) + data (-> (make-file-data file-id page-id) + (ch/process-changes + [{:type :add-obj + :id main-id :page-id page-id :frame-id uuid/zero + :obj (cts/setup-shape + {:id main-id :type :frame :name "Main" + :frame-id uuid/zero :parent-id uuid/zero + :component-id component-id :component-file file-id + :main-instance true :component-root true})} + {:type :add-obj + :id child-id :page-id page-id :frame-id main-id :parent-id main-id + :obj (cts/setup-shape + {:id child-id :type :rect :name "Child" + :frame-id main-id :parent-id main-id + :x 0 :y 0 :width 10 :height 10})} + {:type :add-component + :id component-id :name "Component" :path "" + :main-instance-id main-id :main-instance-page page-id} + {:type :del-component :id component-id}]))] + {:data data + :page-id page-id + :main-id main-id + :child-id child-id + :component-id component-id})) + +(t/deftest update-component-does-not-carry-the-objects-map + (let [{:keys [data component-id]} (make-deleted-component) + changes (pcb/update-component (pcb/with-library-data (pcb/empty-changes nil) data) + component-id + #(assoc % :name "Renamed")) + change (first (:redo-changes changes))] + + (t/testing "the change carries component metadata only" + (t/is (= :mod-component (:type change))) + (t/is (= "Renamed" (:name change))) + (t/is (not (contains? change :objects)) + "resending the whole objects map on every touch is what bloated the changes") + (t/is (not (contains? (first (:undo-changes changes)) :objects)))) + + (t/testing "applying the change keeps the component shapes" + (let [data' (ch/process-changes data (:redo-changes changes))] + (t/is (= 2 (count (get-in data' [:components component-id :objects])))))))) + +(t/deftest mod-component-touches-objects-only-when-it-carries-them + (let [{:keys [data component-id]} (make-deleted-component) + objects (:objects (get-in data [:components component-id]))] + + (t/testing "a change without :objects leaves the stored shapes alone" + (let [data' (ch/process-changes data [{:type :mod-component + :id component-id :name "Renamed"}])] + (t/is (= objects (get-in data' [:components component-id :objects]))))) + + (t/testing "an explicit nil :objects drops them" + (let [data' (ch/process-changes data [{:type :mod-component + :id component-id :objects nil}])] + (t/is (not (contains? (get-in data' [:components component-id]) :objects))))) + + (t/testing "a change with :objects replaces them" + (let [data' (ch/process-changes data [{:type :mod-component + :id component-id :objects {}}])] + (t/is (= {} (get-in data' [:components component-id :objects]))))))) + +(t/deftest shape-change-scoped-to-a-component-matches-the-whole-map-change + (let [{:keys [data child-id component-id]} (make-deleted-component) + objects (:objects (get-in data [:components component-id])) + scoped-change {:type :mod-obj + :component-id component-id :id child-id + :operations [{:type :set :attr :name :val "Renamed"}]} + ;; what `update-component` used to emit: the whole objects map rewritten + whole-map-change {:type :mod-component + :id component-id + :objects (assoc objects child-id + (assoc (get objects child-id) :name "Renamed"))}] + + (t/is (= (without-modified-at (ch/process-changes data [whole-map-change])) + (without-modified-at (ch/process-changes data [scoped-change])))))) + (t/deftest token-changes-are-idempotent (let [set-id (uuid/custom 20 1) other-id (uuid/custom 20 2)