From d6139724748904bf84909f6edf8683e2ad3f97b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Tejero-Cantero?= <807608+alvorithm@users.noreply.github.com> Date: Thu, 22 Oct 2026 20:04:02 +0200 Subject: [PATCH] :bug: Merge file-level plugin data and base font size on branches (#12569) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A branch whose only change was file-level plugin data or the base font size merged with a success response, and the change was lost. The merge engine compared colours, typographies, components, media, pages, and tokens only, so such a branch produced an empty summary and the merge took the empty-changes shortcut. The engine now classifies two more kinds three-way: file plugin data, flattened to namespace and key as page plugin data already is, and the file options, where an unset base font size reads as the default. Both kinds count towards ahead, behind, and conflicts, appear in the compare dialog, and translate into set-plugin-data and set-base-font-size changes for merge and update from main. Both sides changing the same key to different values is a modify-modify conflict. AI-assisted-by: claude-opus-5-5 Signed-off-by: Álvaro Tejero Cantero --- .../backend_tests/rpc_file_branch_test.clj | 60 +++++++++++++ common/src/app/common/files/branch_merge.cljc | 42 ++++++++- .../common_tests/files_branch_merge_test.cljc | 90 +++++++++++++++++++ .../main/ui/workspace/sidebar/branches.cljs | 2 + 4 files changed, 192 insertions(+), 2 deletions(-) diff --git a/backend/test/backend_tests/rpc_file_branch_test.clj b/backend/test/backend_tests/rpc_file_branch_test.clj index a4d84a42dc..fccb45e45d 100644 --- a/backend/test/backend_tests/rpc_file_branch_test.clj +++ b/backend/test/backend_tests/rpc_file_branch_test.clj @@ -648,6 +648,66 @@ :features cfeat/supported-features :changes [change]}))) +(def ^:private file-plugin-change + {:type :set-plugin-data :object-type :file :namespace :my-plugin :key "foo" :value "bar"}) + +(def ^:private base-font-size-change + {:type :set-base-font-size :base-font-size "20px"}) + +(t/deftest merge-applies-file-level-changes + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [profile (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + main-data (fn [] + (-> (th/command! {::th/type :get-file + ::rpc/profile-id (:id profile) + :id (:id file)}) + :result :data))] + (doseq [[label change landed?] + [["file plugin data" file-plugin-change + #(= "bar" (get-in % [:plugin-data :my-plugin "foo"]))] + ["base font size" base-font-size-change + #(= "20px" (get-in % [:options :base-font-size]))]]] + (t/testing label + (let [create (:result (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id profile) + :file-id (:id file) + :name label}))] + (t/is (nil? (:error (apply-change* profile (:branch-file-id create) change)))) + (let [info (:result (th/command! {::th/type :get-file-branch-info + ::rpc/profile-id (:id profile) + :file-id (:branch-file-id create)}))] + (t/is (= [1 0 0] ((juxt :ahead :behind :conflicts) info)))) + (let [out (th/command! {::th/type :merge-file-branch + ::rpc/profile-id (:id profile) + :branch-id (:id create)})] + (t/is (nil? (:error out))) + (t/is (= :merged (-> out :result :status)))) + (t/is (landed? (main-data))))))))) + +(t/deftest merge-reports-file-level-conflicts + (with-redefs [cf/flags (conj cf/flags :branching)] + (let [profile (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + create (:result (th/command! {::th/type :create-file-branch + ::rpc/profile-id (:id profile) + :file-id (:id file) + :name "b"}))] + (apply-change* profile (:branch-file-id create) file-plugin-change) + (apply-change* profile (:branch-file-id create) base-font-size-change) + (apply-change* profile (:id file) (assoc file-plugin-change :value "main")) + (apply-change* profile (:id file) (assoc base-font-size-change :base-font-size "18px")) + (let [info (:result (th/command! {::th/type :get-file-branch-info + ::rpc/profile-id (:id profile) + :file-id (:branch-file-id create)}))] + (t/is (= 2 (:conflicts info)))) + (let [out (th/command! {::th/type :merge-file-branch + ::rpc/profile-id (:id profile) + :branch-id (:id create)})] + (t/is (= :conflicts (-> out :result :status))))))) + (t/deftest merge-deletes-branch-by-default-and-releases-base (with-redefs [cf/flags (conj cf/flags :branching)] (let [profile (th/create-profile* 1 {:is-active true}) diff --git a/common/src/app/common/files/branch_merge.cljc b/common/src/app/common/files/branch_merge.cljc index 02d48a243e..262d3785aa 100644 --- a/common/src/app/common/files/branch_merge.cljc +++ b/common/src/app/common/files/branch_merge.cljc @@ -23,6 +23,7 @@ [app.common.data :as d] [app.common.files.helpers :as cfh] [app.common.types.component :as ctk] + [app.common.types.file :as ctf] [app.common.types.pages-list :as ctpl] [app.common.types.tokens-lib :as ctob] [app.common.types.tokens-status :as ctos] @@ -414,6 +415,19 @@ (transient {}) (or pd {})))) +(defn- file-options + "File options the merge carries, keyed by option name. An unset base + font size reads as the default, so a side that never set it does not + differ from a side that set the default explicitly." + [data] + {:base-font-size (ctf/get-base-font-size data)}) + +(defn- set-option-change + "Change op that sets file option `option` to `value`." + [option value] + (case option + :base-font-size {:type :set-base-font-size :base-font-size value})) + ;; --- Shape tree --- ;; ;; `three-way-entities` classifies shapes one by one, while containment @@ -1248,6 +1262,14 @@ {:kind :component}) (three-way-entities (:media base) (:media theirs) (:media ours) {:kind :media}) + ;; file-level plugin data and options: kinds of their own, + ;; or a branch that changes only them merges as a no-op + (three-way-entities (flatten-plugin-data (:plugin-data base)) + (flatten-plugin-data (:plugin-data theirs)) + (flatten-plugin-data (:plugin-data ours)) + {:kind :file-plugin}) + (three-way-entities (file-options base) (file-options theirs) (file-options ours) + {:kind :file-option}) (diff-pages base theirs ours only-pages dir) (diff-tokens base theirs ours)] {:keys [changes conflicts]} (merge-results results) @@ -1326,7 +1348,8 @@ (def ^:private mergeable-kinds #{: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}) + :page :page-order :page-guide :page-flow :page-grid :page-plugin :component + :file-plugin :file-option}) (defn unsupported-kinds "Set of change kinds present in `changes` that `compute-changes` @@ -1937,6 +1960,21 @@ (fn [_ o] {:type :mod-media :object o}) (fn [id] {:type :del-media :id id})) + ;; file-level plugin-data (flattened to [ns key] -> value) + file-plugins (flat :file-plugin {} + (flatten-plugin-data (:plugin-data base)) + (flatten-plugin-data (:plugin-data main)) + (flatten-plugin-data (:plugin-data branch)) + (fn [[ns k] v] {:type :set-plugin-data :object-type :file :namespace ns :key k :value v}) + (fn [[ns k] v] {:type :set-plugin-data :object-type :file :namespace ns :key k :value v}) + (fn [[ns k]] {:type :set-plugin-data :object-type :file :namespace ns :key k :value nil})) + + ;; file options: every side carries every option (`file-options` + ;; fills the default), so no option is ever added or deleted + file-opts (flat :file-option {} + (file-options base) (file-options main) (file-options branch) + set-option-change set-option-change (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)) @@ -2194,7 +2232,7 @@ ;; objects (still on the page) before del-obj removes them :changes (vec (concat page-presence components shapes page-meta-changes page-guides page-flows page-grids page-plugins page-order-changes - colors typos media + colors typos media file-plugins (filterv some? file-opts) 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 90d0295f1f..633daaf2a5 100644 --- a/common/test/common_tests/files_branch_merge_test.cljc +++ b/common/test/common_tests/files_branch_merge_test.cljc @@ -670,6 +670,96 @@ (let [data' (cfc/process-changes base changes)] (t/is (= "bar" (get-in data' [:pages-index pid :plugin-data :my-plugin "foo"])))))) +;; --- file-level plugin data and options + +(defn- file-level-data + [& {:keys [plugin-data base-font-size]}] + (let [pid (uuid/custom 0 1)] + (cond-> (pages-data {pid {:id pid :name "P" :objects {}}} [pid]) + (some? plugin-data) (assoc :plugin-data plugin-data) + (some? base-font-size) (assoc-in [:options :base-font-size] base-font-size)))) + +(t/deftest compute-merge-file-plugin-data + (let [base (file-level-data) + branch (file-level-data :plugin-data {:my-plugin {"foo" "bar"}}) + r (bm/compute-merge base base branch :branch->main) + c (first (:changes r))] + (t/is (= {:added 1 :modified 0 :deleted 0 :conflicts 0} (:stats r))) + (t/is (= :file-plugin (:kind c))) + (t/is (= [:my-plugin "foo"] (:id c))))) + +(t/deftest compute-merge-file-base-font-size + (let [base (file-level-data :base-font-size "16px") + branch (file-level-data :base-font-size "20px") + 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 (= :file-option (:kind c))) + (t/is (= :base-font-size (:id c))))) + +(t/deftest compute-merge-file-base-font-size-absent-is-the-default + (let [base (file-level-data) + branch (file-level-data :base-font-size "16px")] + (t/is (empty? (:changes (bm/compute-merge base base branch :branch->main)))))) + +(t/deftest compute-merge-file-level-conflicts + (let [base (file-level-data :plugin-data {:my-plugin {"foo" "a"}} :base-font-size "16px") + main (file-level-data :plugin-data {:my-plugin {"foo" "b"}} :base-font-size "18px") + branch (file-level-data :plugin-data {:my-plugin {"foo" "c"}} :base-font-size "20px") + r (bm/compute-merge base main branch :branch->main)] + (t/is (= 2 (-> r :stats :conflicts))) + (t/is (= #{:file-plugin :file-option} (set (map :kind (:conflicts r))))) + (t/is (every? #(= :modify-modify (:reason %)) (:conflicts r))) + (t/is (= #{["b" "c"] ["18px" "20px"]} + (set (map (juxt :main :branch) (:conflicts r))))))) + +(t/deftest compute-changes-file-plugin-data + (let [base (file-level-data :plugin-data {:my-plugin {"gone" "x"}}) + branch (file-level-data :plugin-data {:my-plugin {"foo" "bar"}}) + {:keys [changes unsupported]} (bm/compute-changes base base branch) + sets (filter #(= :set-plugin-data (:type %)) changes)] + (t/is (empty? unsupported)) + (t/is (= #{{:type :set-plugin-data :object-type :file + :namespace :my-plugin :key "foo" :value "bar"} + {:type :set-plugin-data :object-type :file + :namespace :my-plugin :key "gone" :value nil}} + (set sets))) + (let [data' (cfc/process-changes base changes)] + (t/is (= {"foo" "bar"} (get-in data' [:plugin-data :my-plugin])))))) + +(t/deftest compute-changes-file-base-font-size + (let [base (file-level-data :base-font-size "16px") + branch (file-level-data :base-font-size "20px") + {:keys [changes unsupported]} (bm/compute-changes base base branch)] + (t/is (empty? unsupported)) + (t/is (= [{:type :set-base-font-size :base-font-size "20px"}] changes)) + (let [data' (cfc/process-changes base changes)] + (t/is (= "20px" (get-in data' [:options :base-font-size])))))) + +(t/deftest compute-changes-file-level-conflict-resolutions + (let [base (file-level-data :plugin-data {:my-plugin {"foo" "a"}} :base-font-size "16px") + main (file-level-data :plugin-data {:my-plugin {"foo" "b"}} :base-font-size "18px") + branch (file-level-data :plugin-data {:my-plugin {"foo" "c"}} :base-font-size "20px") + apply* (fn [resolutions] + (cfc/process-changes main (:changes (bm/compute-changes base main branch resolutions))))] + (t/testing "unresolved: main keeps its values" + (let [data' (apply* {})] + (t/is (= "b" (get-in data' [:plugin-data :my-plugin "foo"]))) + (t/is (= "18px" (get-in data' [:options :base-font-size]))))) + (t/testing "resolved to the branch: the branch values land" + (let [data' (apply* {[:my-plugin "foo"] :branch :base-font-size :branch})] + (t/is (= "c" (get-in data' [:plugin-data :my-plugin "foo"]))) + (t/is (= "20px" (get-in data' [:options :base-font-size]))))))) + +(t/deftest compute-changes-file-level-update-from-main + (let [base (file-level-data) + main (file-level-data :plugin-data {:my-plugin {"foo" "bar"}} :base-font-size "20px") + r (bm/compute-merge base main base :main->branch) + data' (cfc/process-changes base (:changes (bm/compute-changes base base main {} r)))] + (t/is (= 2 (count (:changes r)))) + (t/is (= "bar" (get-in data' [:plugin-data :my-plugin "foo"]))) + (t/is (= "20px" (get-in data' [:options :base-font-size]))))) + ;; --- components (t/deftest compute-changes-component-add diff --git a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs index 2f3d36809f..b3fede1741 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/branches.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/branches.cljs @@ -65,6 +65,8 @@ :page-flow i/document :page-grid i/document :page-plugin i/document + :file-plugin i/document + :file-option i/document :token i/tokens :token-set i/tokens :token-set-rename i/tokens