From 87d61cbdb401a4966ec5d878aa993b178e39a221 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Tejero=20Cantero?= Date: Wed, 28 Oct 2026 12:31:12 +0100 Subject: [PATCH] :bug: Give a branch file its own id in the document it derives A branch derives its document from main's base snapshot, and the derive never re-pointed main's ids: :data :id stayed main's, and so did every inherited reference to the file itself (component instances, library colour and typography refs). The branch workspace then took its own components for an external library it never loads and read its own tokens through main: validation reported :component-main-external and :component-not-found on a fresh branch, the local component sync skipped inherited copies, an update from main refused once the branch held its own copy of a base component, and materialize detached the main instance of every inherited component. The derive now sets the branch's id on the document before the replay, so a change or migration comparing a reference with the document's id sees the branch, and re-routes main's id to the branch's after it, so op log rows written before this fix are covered too. The base keeps main's ids because it shares main's fragments; comparisons already map the branch's id back to main's. relink-refs rebuilds a collection only when something inside it changed (median 803 ms instead of 1506 ms on a 90,612-shape file), and it now relinks the :ref-file of shadow and grid colours, the key migration 0015 gave them, which it had missed since. AI-assisted-by: claude-opus-5-5 --- backend/src/app/binfile/common.clj | 21 +- .../branch_file_identity_test.clj | 242 ++++++++++++++++++ common/src/app/common/files/helpers.cljc | 75 ++++-- 3 files changed, 317 insertions(+), 21 deletions(-) create mode 100644 backend/test/backend_tests/branch_file_identity_test.clj diff --git a/backend/src/app/binfile/common.clj b/backend/src/app/binfile/common.clj index 67b04e90db..347f66eada 100644 --- a/backend/src/app/binfile/common.clj +++ b/backend/src/app/binfile/common.clj @@ -11,6 +11,7 @@ [app.common.data :as d] [app.common.exceptions :as ex] [app.common.features :as cfeat] + [app.common.files.branch-merge :as bm] [app.common.files.changes :as cpc] [app.common.files.helpers :as cfh] [app.common.files.migrations :as fmg] @@ -401,6 +402,15 @@ `get-file*` migrates the rest of the way in memory, and a log with no upgrade since the base (the steady state) never migrates at all. + The derived document names the branch file: its `:data :id` and every + reference to the source file (component instances, library colour and + typography refs, the tokens source) carry the branch's id, so the + branch's own components and tokens resolve in the branch itself. The + base keeps the source's ids, because it shares the source's fragments, + and the re-route runs after the replay so it also covers op log rows + that named the source. A comparison maps the branch's id back to the + source's (`files_branch.clj::branch-id-map`). + Returns the file with `:data` decoded (plain maps); callers that asked for `:decode? false` get it re-encoded as bytes so the update-file pipeline (which always blob-decodes) is unaffected. @@ -471,9 +481,11 @@ ;; the document the replay works on: the merge base at the ;; base's data version, under the branch file's identity, so a - ;; migration on the way resolves the branch's libraries + ;; migration on the way resolves the branch's libraries, and a + ;; change or migration that compares a reference with the + ;; document's own `:id` sees the branch doc (-> file - (assoc :data (:data base)) + (assoc :data (assoc (:data base) :id id)) (assoc :version (or (:version base) (:version file))) (assoc :migrations (:migrations base))) @@ -497,7 +509,10 @@ [doc 0] rows) - data (cond-> (:data doc) + ;; every reference to the source file becomes a reference to the + ;; branch: the base names the source, and so do op log rows + ;; written while the derive still handed out the source's ids + data (cond-> (bm/remap-refs (:data doc) {(:source-file-id branch) id}) (not decode?) (blob/encode)) ;; the comparison's `base` is the merge base before the replay, diff --git a/backend/test/backend_tests/branch_file_identity_test.clj b/backend/test/backend_tests/branch_file_identity_test.clj new file mode 100644 index 0000000000..a0e646d132 --- /dev/null +++ b/backend/test/backend_tests/branch_file_identity_test.clj @@ -0,0 +1,242 @@ +;; 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 INC Sucursal en España SL + +(ns backend-tests.branch-file-identity-test + "A branch file is its own file: the document a branch derives from + main's base snapshot names the branch, in `:data :id` and in every + reference to itself (component instances, library colours, the tokens + source). The tests build main with a local component and a token set, + branch it, and check each branch verb against that identity." + (:require + [app.binfile.common :as bfc] + [app.common.features :as cfeat] + [app.common.files.tokens :as cfo] + [app.common.files.validate :as cfv] + [app.common.types.file :as ctf] + [app.common.types.shape :as cts] + [app.common.uuid :as uuid] + [app.config :as cf] + [app.db :as db] + [app.rpc :as-alias rpc] + [backend-tests.helpers :as th] + [clojure.test :as t])) + +(t/use-fixtures :once th/state-init) +(t/use-fixtures :each th/database-reset) + +(defn- save + "Apply `changes` to `file-id` through the RPC, as the workspace does." + [profile file-id changes] + (let [file (th/db-get :file {:id file-id}) + out (th/command! {::th/type :update-file + ::rpc/profile-id (:id profile) + :id file-id + :session-id (uuid/random) + :revn (:revn file) + :vern (:vern file) + :features cfeat/supported-features + :changes changes})] + (t/is (nil? (:error out))))) + +(defn- get-file + [profile file-id] + (:result (th/command! {::th/type :get-file + ::rpc/profile-id (:id profile) + :id file-id}))) + +(defn- component-frame + [id attrs] + (merge (cts/setup-shape {:id id :name "Button" :type :frame + :x 0 :y 0 :width 100 :height 100 + :parent-id uuid/zero :frame-id uuid/zero}) + attrs)) + +(defn- add-root + [page-id id obj] + {:type :add-obj :page-id page-id :id id + :parent-id uuid/zero :frame-id uuid/zero :obj obj}) + +(defn- set-fill + [page-id id color] + {:type :mod-obj :page-id page-id :id id + :operations [{:type :set :attr :fills + :val [{:fill-color color :fill-opacity 1}]}]}) + +(defn- copy-of + "A copy of the component `component-id` whose main is `main-id`, linked + to the component through `component-file`." + [page-id id component-id main-id component-file] + (add-root page-id id (component-frame id {:component-root true + :component-id component-id + :component-file component-file + :shape-ref main-id}))) + +(defn- painted + "A rect painted with the library colour `color-id` of `file-id`, as a + fill and as a shadow." + [page-id id color-id file-id] + (add-root page-id id + (-> (cts/setup-shape {:id id :name "Swatch" :type :rect + :x 0 :y 0 :width 10 :height 10 + :parent-id uuid/zero :frame-id uuid/zero}) + (assoc :fills [{:fill-color "#ff0000" :fill-opacity 1 + :fill-color-ref-id color-id + :fill-color-ref-file file-id}]) + (assoc :shadow [{:id (uuid/random) :style :drop-shadow + :offset-x 4 :offset-y 4 :blur 4 :spread 0 + :hidden false + :color {:color "#ff0000" :opacity 1 + :ref-id color-id :ref-file file-id}}])))) + +(defn- setup + "Main holding a local component (main `:mi-id`, copy `:copy-id`), a + library colour painted on `:swatch-id`, and a token set, branched + once." + [] + (let [profile (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :is-shared false}) + main-id (:id file) + page-id (-> (get-file profile main-id) :data :pages first) + comp-id (uuid/random) + mi-id (uuid/random) + copy-id (uuid/random) + color-id (uuid/random) + swatch-id (uuid/random) + set-id (uuid/random) + tok-id (uuid/random)] + (save profile main-id + [(add-root page-id mi-id (component-frame mi-id {:component-root true + :main-instance true + :component-id comp-id + :component-file main-id})) + {:type :add-component :id comp-id :name "Button" :path "" + :main-instance-id mi-id :main-instance-page page-id} + (copy-of page-id copy-id comp-id mi-id main-id) + {:type :add-color + :color {:id color-id :name "red" :color "#ff0000" :opacity 1}} + (painted page-id swatch-id color-id main-id) + {:type :set-token-set :id set-id :attrs {:id set-id :name "core"}} + {:type :set-token :set-id set-id :token-id tok-id + :attrs {:id tok-id :name "brand" :type :color :value "#ff0000"}}]) + (let [branch (:result (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id profile) + :file-id main-id + :name "identity"}))] + {:profile profile + :main-id main-id + :page-id page-id + :comp-id comp-id + :mi-id mi-id + :copy-id copy-id + :color-id color-id + :swatch-id swatch-id + :branch-id (:id branch) + :branch-file-id (:branch-file-id branch)}))) + +(defn- shape + [file page-id id] + (get-in file [:data :pages-index page-id :objects id])) + +(defn- colour-refs + [file page-id id] + (let [s (shape file page-id id)] + [(-> s :fills first :fill-color-ref-file) + (-> s :shadow first :color :ref-file)])) + +(defn- validation-codes + [file] + (let [libs (db/run! th/*system* #(bfc/get-resolved-file-libraries % file))] + (into #{} (map :code) (cfv/validate-file file libs)))) + +(t/deftest an-opened-branch-names-itself + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [{:keys [profile page-id mi-id copy-id swatch-id branch-file-id]} (setup) + branch (get-file profile branch-file-id)] + + (t/testing "the document carries the branch file's id" + (t/is (= branch-file-id (-> branch :data :id)))) + + (t/testing "inherited instances resolve their component locally" + (t/is (= branch-file-id (:component-file (shape branch page-id mi-id)))) + (t/is (= branch-file-id (:component-file (shape branch page-id copy-id)))) + (t/is (= branch-file-id + (:id (ctf/find-component-file branch {} + (:component-file (shape branch page-id copy-id))))))) + + (t/testing "inherited fills and shadows name the branch's own colour" + (t/is (= [branch-file-id branch-file-id] (colour-refs branch page-id swatch-id)))) + + (t/testing "the branch is its own tokens source" + (t/is (= branch-file-id (cfo/get-effective-tokens-source (:data branch))))) + + (t/testing "the branch validates clean" + (t/is (empty? (validation-codes branch))))))) + +(t/deftest update-from-main-keeps-a-branch-copy-of-a-base-component + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [{:keys [profile main-id page-id comp-id mi-id copy-id branch-id branch-file-id]} (setup) + own-copy (uuid/random)] + ;; the workspace instantiates a local component with the current + ;; file, the branch, as its library + (save profile branch-file-id [(copy-of page-id own-copy comp-id mi-id branch-file-id)]) + (save profile main-id [(set-fill page-id mi-id "#0000ff") + (set-fill page-id copy-id "#0000ff")]) + + (let [out (th/command! {::th/type :update-branch-from-main + ::rpc/profile-id (:id profile) + :branch-id branch-id})] + (t/is (nil? (:error out))) + (t/is (= :updated (-> out :result :status)))) + + (let [branch (get-file profile branch-file-id)] + (t/is (= "#0000ff" (-> (shape branch page-id mi-id) :fills first :fill-color))) + (t/is (= branch-file-id (:component-file (shape branch page-id own-copy)))) + (t/is (empty? (validation-codes branch))))))) + +(t/deftest merge-names-main-in-what-it-applies + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [{:keys [profile main-id page-id comp-id mi-id color-id branch-id branch-file-id]} (setup) + own-copy (uuid/random) + own-swatch (uuid/random)] + ;; the workspace paints with a local colour naming the current + ;; file, the branch + (save profile branch-file-id [(copy-of page-id own-copy comp-id mi-id branch-file-id) + (painted page-id own-swatch color-id branch-file-id) + (set-fill page-id mi-id "#00ff00")]) + + (let [out (th/command! {::th/type :merge-file-branch + ::rpc/profile-id (:id profile) + :branch-id branch-id})] + (t/is (nil? (:error out))) + (t/is (= :merged (-> out :result :status)))) + + (let [main (get-file profile main-id)] + (t/is (= main-id (-> main :data :id))) + (t/is (= main-id (:component-file (shape main page-id own-copy)))) + (t/is (= main-id (:component-file (shape main page-id mi-id)))) + (t/is (= "#00ff00" (-> (shape main page-id mi-id) :fills first :fill-color))) + (t/is (= [main-id main-id] (colour-refs main page-id own-swatch))) + (t/is (empty? (validation-codes main))))))) + +(t/deftest materialize-keeps-the-base-components + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [{:keys [profile page-id comp-id mi-id copy-id branch-file-id]} (setup) + out (th/command! {::th/type :materialize-file-branch + ::rpc/profile-id (:id profile) + :branch-file-id branch-file-id})] + (t/is (nil? (:error out))) + + (let [file (get-file profile branch-file-id) + main (shape file page-id mi-id)] + (t/is (= branch-file-id (-> file :data :id))) + (t/is (true? (:main-instance main))) + (t/is (= comp-id (:component-id main))) + (t/is (= branch-file-id (:component-file main))) + (t/is (= branch-file-id (:component-file (shape file page-id copy-id)))) + (t/is (empty? (validation-codes file))))))) + diff --git a/common/src/app/common/files/helpers.cljc b/common/src/app/common/files/helpers.cljc index 2b83fa75c5..0c28f759ad 100644 --- a/common/src/app/common/files/helpers.cljc +++ b/common/src/app/common/files/helpers.cljc @@ -11,7 +11,6 @@ [app.common.geom.shapes.common :as gco] [app.common.uuid :as uuid] [clojure.set :as set] - [clojure.walk :as walk] [cuerdas.core :as str])) #?(:clj (set! *warn-on-reflection* true)) @@ -659,49 +658,89 @@ (defn relink-refs "A function responsible to analyze the file data or shape for references - and apply lookup-index on it." + and apply lookup-index on it. + + It visits every map in `data` but rebuilds a collection only when + something inside it changed: a branch derive runs it over the whole + document on every read, and a copying walk spent seconds on a file of + 90,000 shapes where most shapes hold no reference at all." [data lookup-index] - (letfn [(process-map-form [form] + (letfn [(relink-key [form k] + (let [v (get form k) + v' (lookup-index v)] + (if (= v v') form (assoc form k v')))) + + (relink-id-in [form k] + (let [inner (get form k) + inner' (relink-key inner :id)] + (if (identical? inner inner') form (assoc form k inner')))) + + (process-map-form [form] (cond-> form ;; Relink image shapes (and (map? (:metadata form)) (= :image (:type form))) - (update-in [:metadata :id] lookup-index) + (relink-id-in :metadata) ;; Relink paths with fill image (map? (:fill-image form)) - (update-in [:fill-image :id] lookup-index) + (relink-id-in :fill-image) (map? (:stroke-image form)) - (update-in [:stroke-image :id] lookup-index) + (relink-id-in :stroke-image) ;; This covers old shapes and the new :fills. (uuid? (:fill-color-ref-file form)) - (update :fill-color-ref-file lookup-index) + (relink-key :fill-color-ref-file) ;; This covers the old shapes and the new :strokes (uuid? (:stroke-color-ref-file form)) - (update :stroke-color-ref-file lookup-index) + (relink-key :stroke-color-ref-file) ;; This covers all text shapes that have typography referenced (uuid? (:typography-ref-file form)) - (update :typography-ref-file lookup-index) + (relink-key :typography-ref-file) ;; This covers the component instance links (uuid? (:component-file form)) - (update :component-file lookup-index) + (relink-key :component-file) + + ;; This covers the shadows and grids: their colour names the + ;; library in `:ref-file` (migration 0015 renamed the legacy + ;; `:file-id`, still relinked for unmigrated data) + (uuid? (:ref-file form)) + (relink-key :ref-file) - ;; This covers the shadows and grids (they have directly - ;; the :file-id prop) (uuid? (:file-id form)) - (update :file-id lookup-index))) + (relink-key :file-id))) - (process-form [form] - (if (map? form) - (process-map-form form) - form))] + (walk-children [form] + (cond + (or (map? form) (vector? form)) + (reduce-kv (fn [acc k v] + (let [v' (walk v)] + (if (identical? v v') acc (assoc acc k v')))) + form + form) - (walk/postwalk process-form data))) + (coll? form) + (let [items (map walk form)] + (cond + (every? true? (map identical? form items)) form + (list? form) (apply list items) + (seq? form) (doall items) + :else (into (empty form) items))) + + :else + form)) + + (walk [form] + (let [form (walk-children form)] + (if (map? form) + (process-map-form form) + form)))] + + (walk data))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; SHAPES ORGANIZATION