mirror of
https://github.com/penpot/penpot.git
synced 2026-10-29 05:50:35 +00:00
🐛 Merge file-level plugin data and base font size on branches (#12569)
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 <alvorithm@teje.ro>
This commit is contained in:
parent
6ddbfcf749
commit
d613972474
@ -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})
|
||||
|
||||
@ -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))})))
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user