From 7f07f13940bd2487834b96e41ee9ce6847f23530 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Tejero=20Cantero?= Date: Wed, 28 Oct 2026 12:31:05 +0100 Subject: [PATCH] :bug: Merge the tokens source of a branch The merge engine never compared the file's tokens source, so a branch that switched its tokens to a library, or back to its own, lost that change on merge, and main's change was lost on an update from main. The source is now a mergeable kind, :tokens-source, compared raw (nil means the file itself, and a value naming the file reads as nil). A change on one side applies as a :set-tokens-source change, and a change on both sides is a conflict resolved like the base font size. remap-refs now also rewrites the document's own :id and its :tokens-source through the id map, and remap-changes rewrites the :library-id of a :set-tokens-source change, so a branch document moved into main's id space names main as itself. The compare panel labels the new kind. AI-assisted-by: claude-opus-5-5 --- common/src/app/common/files/branch_merge.cljc | 65 ++++++++-- .../common_tests/files_branch_merge_test.cljc | 117 ++++++++++++++++++ .../sidebar/branch_review/common.cljs | 8 +- frontend/translations/en.po | 4 + 4 files changed, 182 insertions(+), 12 deletions(-) diff --git a/common/src/app/common/files/branch_merge.cljc b/common/src/app/common/files/branch_merge.cljc index 234dda71d6..6d8ecd872c 100644 --- a/common/src/app/common/files/branch_merge.cljc +++ b/common/src/app/common/files/branch_merge.cljc @@ -428,6 +428,24 @@ (case option :base-font-size {:type :set-base-font-size :base-font-size value})) +(defn- tokens-source + "The file's tokens source as the merge compares it, keyed by + `:tokens-source`: the RAW `:tokens-source` value, nil meaning the file + itself. A value naming one of `self-ids` (the `:id`s of the compared + documents, all ids of the same logical file) also reads as nil, so a + side that names itself explicitly does not differ from one that leaves + the attribute out. The effective source is not compared: it falls back + to the document's own `:id`, and that differs between a branch and its + source." + [data self-ids] + (let [source (:tokens-source data)] + {:tokens-source (when-not (contains? self-ids source) source)})) + +(defn- self-ids + "The data `:id`s of the compared documents." + [& datas] + (into #{} (keep :id) datas)) + ;; --- Shape tree --- ;; ;; `three-way-entities` classifies shapes one by one, while containment @@ -1073,7 +1091,11 @@ `:typography-ref-file`, shadow/grid `:file-id`) plus the media refs (shape `:metadata`/`:fill-image`/`:stroke-image` ids, the `:media` collection keys and library-color `:image` ids). It is the same surface - `binfile.common/process-file` remaps when it copies a file. + `binfile.common/process-file` remaps when it copies a file. The + document's own `:id` and its `:tokens-source` are rewritten too, so a + document moved into another file's id space names that file as itself, + and a tokens source that named the old file names the new one, which + `set-tokens-source` and `get-effective-tokens-source` read as self. A branch stores no data of its own: its state is its source's base snapshot with the branch's op log replayed over it @@ -1091,6 +1113,8 @@ data (let [lookup #(get id-map % %)] (-> data + (d/update-when :id #(if (uuid? %) (lookup %) %)) + (d/update-when :tokens-source lookup) (d/update-when :pages-index #(cfh/relink-refs % lookup)) (d/update-when :components #(cfh/relink-refs % lookup)) (d/update-when :media @@ -1149,12 +1173,14 @@ complete on their own. The payloads it cannot see are the values that ARE the reference: a media object's `:id` (its `:media-id` is a storage key and must not be touched), a library color's `:image` id, a `:del-media`'s - top-level `:id`, and an operation value that is the reference itself. The - top-level `:id` of the other delete changes (`:del-color`, - `:del-typography`, `:del-component`, `:del-obj`, `:del-page`) is NOT such a - value: those name entities the two files share by id, never a media row, - so they are left alone — only a media id or the file id is ever a key of - `id-map`. Token changes are left alone: no id in `id-map` is a token id." + top-level `:id`, a `:set-tokens-source`'s `:library-id`, and an operation + value that is the reference itself. A `:set-tokens-source`'s `:file-id` is + a `:file-id` key, which `relink-refs` rewrites. The top-level `:id` of the + other delete changes (`:del-color`, `:del-typography`, `:del-component`, + `:del-obj`, `:del-page`) is NOT such a value: those name entities the two + files share by id, never a media row, so they are left alone — only a + media id or the file id is ever a key of `id-map`. The other token changes + are left alone: no id in `id-map` is a token id." [changes id-map] (if (empty? id-map) changes @@ -1168,6 +1194,10 @@ (uuid? (:id (:object change))) (update-in [:object :id] lookup) + (and (= :set-tokens-source (:type change)) + (uuid? (:library-id change))) + (update :library-id lookup) + (uuid? (get-in change [:color :image :id])) (update-in [:color :image :id] lookup) @@ -1294,6 +1324,11 @@ {:kind :file-plugin}) (three-way-entities (file-options base) (file-options theirs) (file-options ours) {:kind :file-option}) + ;; the tokens source: a kind of its own, compared raw + (let [ids (self-ids base theirs ours)] + (three-way-entities (tokens-source base ids) (tokens-source theirs ids) + (tokens-source ours ids) + {:kind :tokens-source})) (diff-pages base theirs ours only-pages dir) (diff-tokens base theirs ours)] {:keys [changes conflicts]} (merge-results results) @@ -1345,7 +1380,8 @@ there would hide them. The `:tokens-lib` is diffed as well (`diff-tokens`), so token changes - are reported here with their own kinds." + are reported here with their own kinds. The file's tokens source is + reported under kind `:tokens-source`, entity id `:tokens-source`." ([base main branch dir] (compute-merge* base main branch dir nil)) ([base main branch dir {:keys [only-pages]}] (compute-merge* base main branch dir only-pages))) @@ -1373,7 +1409,7 @@ #{:color :typography :media :shape :token :token-set :token-set-rename :token-set-order :token-theme :token-active-themes :token-active-sets :page :page-order :page-guide :page-flow :page-grid :page-plugin :component - :file-plugin :file-option}) + :file-plugin :file-option :tokens-source}) (defn unsupported-kinds "Set of change kinds present in `changes` that `compute-changes` @@ -1999,6 +2035,16 @@ (file-options base) (file-options main) (file-options branch) set-option-change set-option-change (fn [_] nil)) + ;; tokens source: always present on every side (nil = self), so + ;; never added or deleted. The change names the target document. + ids (self-ids base main branch) + set-source (fn [_ library-id] + {:type :set-tokens-source :file-id (:id main) :library-id library-id}) + source-changes (flat :tokens-source {} + (tokens-source base ids) (tokens-source main ids) + (tokens-source branch ids) + set-source set-source (fn [_] nil)) + ;; pages: add (full page incl. objects) / delete + rename (mod-page) bpi (:pages-index base) mpi (:pages-index main) opi (:pages-index branch) bpids (set (keys bpi)) mpids (set (keys mpi)) opids (set (keys opi)) @@ -2257,6 +2303,7 @@ :changes (vec (concat page-presence components shapes page-meta-changes page-guides page-flows page-grids page-plugins page-order-changes colors typos media file-plugins (filterv some? file-opts) + (filterv some? source-changes) set-presence set-rename-changes set-order-changes token-vals themes active-changes))}))) diff --git a/common/test/common_tests/files_branch_merge_test.cljc b/common/test/common_tests/files_branch_merge_test.cljc index d3bd965f1a..8db10566cc 100644 --- a/common/test/common_tests/files_branch_merge_test.cljc +++ b/common/test/common_tests/files_branch_merge_test.cljc @@ -324,6 +324,34 @@ (t/is (= {media-new {:id media-new :name "img"}} (:media data'))) (t/is (= media-new (get-in data' [:colors :c1 :image :id]))))) +(t/deftest remap-refs-remaps-document-id-and-tokens-source + (let [branch-id (uuid/next) + main-id (uuid/next) + ext-lib (uuid/next) + media-old (uuid/next)] + (t/testing "the file id map moves the document and its self-named source" + (let [data' (bm/remap-refs (assoc (mkdata {}) :id branch-id :tokens-source branch-id) + {branch-id main-id})] + (t/is (= main-id (:id data'))) + (t/is (= main-id (:tokens-source data'))) + (t/is (cfo/effective-tokens-source? data' main-id)))) + (t/testing "a library tokens source outside the map stays" + (let [data' (bm/remap-refs (assoc (mkdata {}) :id branch-id :tokens-source ext-lib) + {branch-id main-id})] + (t/is (= main-id (:id data'))) + (t/is (= ext-lib (:tokens-source data'))))) + (t/testing "a media-only map leaves the document id alone" + (let [data' (bm/remap-refs (assoc (mkdata {}) :id branch-id) {media-old (uuid/next)})] + (t/is (= branch-id (:id data'))) + (t/is (not (contains? data' :tokens-source))))) + (t/testing "remap-changes maps a set-tokens-source change" + (t/is (= [{:type :set-tokens-source :file-id branch-id :library-id branch-id}] + (bm/remap-changes [{:type :set-tokens-source :file-id main-id :library-id main-id}] + {main-id branch-id}))) + (t/is (= [{:type :set-tokens-source :file-id branch-id :library-id nil}] + (bm/remap-changes [{:type :set-tokens-source :file-id main-id :library-id nil}] + {main-id branch-id})))))) + (t/deftest merge-component-keeps-head-and-remaps-file ;; a component created on a branch must merge into main as a VALID head: ;; its main instance keeps component-id/main-instance and its @@ -796,6 +824,95 @@ (t/is (= "bar" (get-in data' [:plugin-data :my-plugin "foo"]))) (t/is (= "20px" (get-in data' [:options :base-font-size]))))) +;; --- tokens source + +(def ^:private main-file-id (uuid/custom 0 100)) +(def ^:private lib-a (uuid/custom 0 101)) +(def ^:private lib-b (uuid/custom 0 102)) + +(defn- source-data + "File-level data of main's file whose tokens source is `source` (nil = + the file itself)." + [source] + (cond-> (assoc (file-level-data) :id main-file-id) + (some? source) (assoc :tokens-source source))) + +(t/deftest compute-merge-tokens-source-one-side + (let [base (source-data nil) + branch (source-data lib-a) + r (bm/compute-merge base base branch :branch->main) + c (first (:changes r))] + (t/is (= {:added 0 :modified 1 :deleted 0 :conflicts 0} (:stats r))) + (t/is (= :tokens-source (:kind c))) + (t/is (= :tokens-source (:id c))) + (t/is (= "tokens-source//tokens-source" (:key c))))) + +(t/deftest compute-changes-tokens-source-one-side + (t/testing "the branch points the file at a library" + (let [base (source-data nil) + branch (source-data lib-a) + {:keys [changes unsupported]} (bm/compute-changes base base branch)] + (t/is (empty? unsupported)) + (t/is (= [{:type :set-tokens-source :file-id main-file-id :library-id lib-a}] changes)) + (t/is (= lib-a (:tokens-source (cfc/process-changes base changes)))))) + (t/testing "the branch points the file back at itself" + (let [base (source-data lib-a) + branch (source-data nil) + {:keys [changes]} (bm/compute-changes base base branch)] + (t/is (= [{:type :set-tokens-source :file-id main-file-id :library-id nil}] changes)) + (t/is (not (contains? (cfc/process-changes base changes) :tokens-source))))) + (t/testing "main's change stays when the branch leaves the source alone" + (let [base (source-data nil) + main (source-data lib-a)] + (t/is (empty? (:changes (bm/compute-merge base main base :branch->main)))) + (t/is (empty? (:changes (bm/compute-changes base main base))))))) + +(t/deftest compute-merge-tokens-source-self-id-reads-as-self + ;; a source naming the file itself is the same as no source + (let [base (source-data nil) + branch (source-data main-file-id)] + (t/is (empty? (:changes (bm/compute-merge base base branch :branch->main)))) + (t/is (empty? (:changes (bm/compute-changes base base branch)))))) + +(t/deftest tokens-source-merges-after-branch-remap + ;; a branch that named its own file id merges into main as "self" + (let [branch-id (uuid/custom 0 103) + base (source-data lib-a) + branch (-> (source-data nil) + (assoc :id branch-id :tokens-source branch-id) + (bm/remap-refs {branch-id main-file-id})) + {:keys [changes]} (bm/compute-changes base base branch)] + (t/is (= [{:type :set-tokens-source :file-id main-file-id :library-id nil}] changes)) + (t/is (not (contains? (cfc/process-changes base changes) :tokens-source))))) + +(t/deftest compute-merge-tokens-source-conflict + (let [base (source-data nil) + main (source-data lib-a) + branch (source-data lib-b) + r (bm/compute-merge base main branch :branch->main) + c (first (:conflicts r)) + apply* (fn [resolutions] + (:tokens-source + (cfc/process-changes main (:changes (bm/compute-changes base main branch resolutions)))))] + (t/is (= 1 (-> r :stats :conflicts))) + (t/is (= {:kind :tokens-source :id :tokens-source :reason :modify-modify + :base nil :main lib-a :branch lib-b} + (select-keys c [:kind :id :reason :base :main :branch]))) + (t/is (bm/conflict-resolved? c :branch)) + (t/testing "unresolved: main keeps its source" + (t/is (= lib-a (apply* {})))) + (t/testing "resolved to the branch: the branch's source lands" + (t/is (= lib-b (apply* {:tokens-source :branch})))))) + +(t/deftest compute-changes-tokens-source-update-from-main + (let [base (source-data nil) + main (source-data lib-a) + r (bm/compute-merge base main base :main->branch) + {:keys [changes unsupported]} (bm/compute-changes base base main {} r)] + (t/is (= [:tokens-source] (map :kind (:changes r)))) + (t/is (empty? unsupported)) + (t/is (= lib-a (:tokens-source (cfc/process-changes base changes)))))) + ;; --- components (t/deftest compute-changes-component-add diff --git a/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs b/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs index 4d4c5b979f..95f8fcda31 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branch_review/common.cljs @@ -69,7 +69,8 @@ :token-set-order i/tokens :token-theme i/tokens :token-active-themes i/tokens - :token-active-sets i/tokens}) + :token-active-sets i/tokens + :tokens-source i/tokens}) (def ^:private shape-type->icon {:frame "board" :rect "rectangle" :line "rectangle" :circle "ellipse" @@ -114,7 +115,8 @@ :token-set-order "workspace.branches.review.type.token-set-order" :token-theme "workspace.branches.review.type.theme" :token-active-themes "workspace.branches.review.type.active-themes" - :token-active-sets "workspace.branches.review.type.active-sets"}) + :token-active-sets "workspace.branches.review.type.active-sets" + :tokens-source "workspace.branches.review.type.tokens-source"}) (def ^:private shape-type->label {:frame "workspace.branches.compare.shape.board" @@ -308,7 +310,7 @@ (or (= :component kind) (:component? item) (:component-copy? item)) :components (contains? #{:token :token-set :token-set-rename :token-set-order :token-theme - :token-active-themes :token-active-sets} kind) + :token-active-themes :token-active-sets :tokens-source} kind) :tokens (contains? #{:color} kind) :colour (contains? #{:typography} kind) :copy diff --git a/frontend/translations/en.po b/frontend/translations/en.po index 3c90ffe581..8285352057 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -11623,6 +11623,10 @@ msgstr "Active themes" msgid "workspace.branches.review.type.active-sets" msgstr "Active token sets" +#: src/app/main/ui/workspace/sidebar/branch_review/common.cljs +msgid "workspace.branches.review.type.tokens-source" +msgstr "Tokens source" + #: src/app/main/ui/workspace/sidebar/branch_review/facts.cljs msgid "workspace.branches.review.facts.contents" msgstr "Contents"