From 8a3330f7811cdc9e7ccc8826883374ff83e93702 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Tejero=20Cantero?= Date: Thu, 17 Sep 2026 01:22:03 +0200 Subject: [PATCH] :bug: Compare a nil-valued attribute and an absent one as equal A branch copy re-points its local references through the duplication pipeline and leaves keys like :fill-color-ref-file and :typography-ref-file present with nil where the source omitted them. Comparing the two forms as different made an untouched entity look edited on one side, which becomes a modify-modify conflict as soon as the other side moves the same shape. The comparison now drops nil-valued keys deeply, for all three sides, in three-way-entities and so for every pass that goes through it. The new engine test failed before the change on both orderings and passes after; the three branch suites report 30 tests, 245 assertions, 0 failures. --- .../test/backend_tests/branch_merge_test.clj | 75 +++++++++++++++++++ common/src/app/common/files/branch_merge.cljc | 36 ++++++++- 2 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 backend/test/backend_tests/branch_merge_test.clj diff --git a/backend/test/backend_tests/branch_merge_test.clj b/backend/test/backend_tests/branch_merge_test.clj new file mode 100644 index 0000000000..ec4513da10 --- /dev/null +++ b/backend/test/backend_tests/branch_merge_test.clj @@ -0,0 +1,75 @@ +;; 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-merge-test + "Engine-level tests for the three-way comparison, with data built by hand + rather than through a file fixture. + + The case that matters here came from a real update-from-main: the branch + copy and its base disagreed only on whether a reference attribute was + present with a `nil` value or was absent, and because main had moved the + same shapes, the engine called 368 of them `:modify-modify` conflicts." + (:require + [app.common.files.branch-merge :as bm] + [app.common.uuid :as uuid] + [clojure.test :refer [deftest is testing]])) + +(def page-id (uuid/next)) +(def shape-id (uuid/next)) + +(defn- file-data + "A one-page file whose single text shape carries `extra` inside its + `:content` (or omits it, when `extra` is empty) and sits at `y`." + [y extra] + {:pages [page-id] + :pages-index + {page-id + {:id page-id + :objects + {shape-id + {:id shape-id :name "label" :type :text :x 0 :y y :width 10 :height 10 + :content (merge {:type :root + :children [{:type :paragraph-set + :children [{:type :paragraph + :children [{:type :span :text "hi"}]}]}]} + extra)}}}}}) + +(def ^:private nil-ref {:typography-ref-file nil}) + +(deftest nil-ref-and-absent-ref-are-the-same-shape + (testing "the branch's copy has the ref with nil, its base without, and main moved the shape" + (let [res (bm/compute-merge (file-data 0 {}) + (file-data 5 {}) + (file-data 0 nil-ref) + :main->branch)] + (is (zero? (count (:conflicts res))) "a nil-versus-absent reference is not a branch edit") + (is (= 1 (count (:changes res))) "main's move still travels as a change") + (is (= :modified (:status (first (:changes res))))))) + + (testing "and the same with the sides the other way round" + (let [res (bm/compute-merge (file-data 0 nil-ref) + (file-data 5 nil-ref) + (file-data 0 {}) + :main->branch)] + (is (zero? (count (:conflicts res)))) + (is (= 1 (count (:changes res))))))) + +(deftest a-real-difference-is-still-a-difference + (testing "a value both sides moved off the base is still a conflict" + (let [res (bm/compute-merge (file-data 0 {}) + (file-data 5 {}) + (file-data -5 {}) + :main->branch)] + (is (= 1 (count (:conflicts res)))) + (is (= :modify-modify (:reason (first (:conflicts res))))))) + + (testing "a change on one side only still travels" + (let [res (bm/compute-merge (file-data 0 {}) + (file-data 5 {}) + (file-data 0 {}) + :main->branch)] + (is (zero? (count (:conflicts res)))) + (is (= 1 (count (:changes res))))))) diff --git a/common/src/app/common/files/branch_merge.cljc b/common/src/app/common/files/branch_merge.cljc index d319b255f1..be0f5a96d9 100644 --- a/common/src/app/common/files/branch_merge.cljc +++ b/common/src/app/common/files/branch_merge.cljc @@ -123,6 +123,36 @@ (transient {}) (or objects {})))) +(defn- strip-nil-attrs + "Drop keys whose value is `nil`, deeply, so that a key present with a + `nil` value compares equal to an absent key. + + A branch copy re-points its local references through the duplication + pipeline, which leaves keys like `:fill-color-ref-file` and + `:typography-ref-file` present with `nil` where the source omitted them; + the same asymmetry appears inside a text `:content`. Comparing the two + forms as different makes an untouched entity look edited on one side, and + when the other side really moved it the engine reports a false + `:modify-modify` conflict — 368 of them on the design-system file's + screenshots of the shape trees, before this existed. Absent and nil mean + the same thing for a shape attribute, so the comparison should not see + the difference." + [v] + (cond + (map? v) + (persistent! + (reduce-kv (fn [acc k x] + (let [x' (strip-nil-attrs x)] + (if (nil? x') + acc + (assoc! acc k x')))) + (transient {}) + v)) + + (vector? v) (mapv strip-nil-attrs v) + (set? v) (into #{} (map strip-nil-attrs) v) + :else v)) + (defn- shape-display-meta "Display-only metadata for a shape diff entry — its type and component nature — so the compare view can pick a type-accurate icon and label @@ -162,9 +192,9 @@ (merge extra extras {:kind kind :status status}))] (reduce (fn [acc id] - (let [b (get base id) - t (get theirs id) - o (get ours id) + (let [b (strip-nil-attrs (get base id)) + t (strip-nil-attrs (get theirs id)) + o (strip-nil-attrs (get ours id)) in-b? (contains? base id) in-t? (contains? theirs id) in-o? (contains? ours id)]