diff --git a/backend/src/app/migrations/sql/0157-add-file-branch-change-table.sql b/backend/src/app/migrations/sql/0157-add-file-branch-change-table.sql index 5fecae9319..ed44a3d7a4 100644 --- a/backend/src/app/migrations/sql/0157-add-file-branch-change-table.sql +++ b/backend/src/app/migrations/sql/0157-add-file-branch-change-table.sql @@ -1,14 +1,19 @@ -- The durable op log of a branch: one row per branch save (the change --- vector the editor produced, blob-encoded) plus the ops a --- update-branch-from-main integration produced. The branch file's --- :data is DERIVED by replaying these rows over the merge-base snapshot --- (`file_branch.base_snapshot_id`), so the branch file itself stores no --- data. +-- vector the editor produced, blob-encoded). An update-branch-from-main +-- replaces the whole log with one row holding the branch's squashed net +-- changes against the repositioned merge base (no row when that net is +-- empty); the integration vector itself goes to `file_change`. The +-- branch file's :data is DERIVED by replaying these rows over the +-- merge-base snapshot (`file_branch.base_snapshot_id`), so the branch +-- file itself stores no data. -- -- `file_change` rows remain the transient source for the lagged-changes -- machinery; this table is what keeps the branch reconstructible after --- those rows are GC'd (team deletion delay). Rows live until the branch --- is deleted or the op log is squashed (update-branch-from-main). +-- those rows are GC'd (each is GC-eligible one hour after its insert). +-- Rows live until the op log is squashed (update-branch-from-main) or +-- dropped (materialize-file-branch), or until the objects GC hard-deletes +-- the branch file row and the foreign keys cascade; a logical branch +-- deletion leaves them in place. CREATE TABLE IF NOT EXISTS file_branch_change ( id uuid PRIMARY KEY, diff --git a/backend/src/app/rpc/commands/files_branch.clj b/backend/src/app/rpc/commands/files_branch.clj index b1f7d34cd4..a92d4fa361 100644 --- a/backend/src/app/rpc/commands/files_branch.clj +++ b/backend/src/app/rpc/commands/files_branch.clj @@ -5,10 +5,12 @@ ;; Copyright (c) KALEIDOS INC Sucursal en España SL (ns app.rpc.commands.files-branch - "RPC commands for file branching: create an isolated copy of a file - (a \"branch\") linked to its source file (\"main\"), list branches, - diff a branch against main, merge a branch into main (with conflict - resolutions), and update a branch from main. See + "RPC commands for file branching: create a branch of a file linked to + its source file (\"main\"), list branches, diff a branch against main, + merge a branch into main (with conflict resolutions), and update a + branch from main. A branch is a file row with no data of its own: its + state is derived from a pinned base snapshot of main plus the branch's + op log (`app.binfile.common/branch-file-data`). See `app.rpc.commands.files-snapshot` for the patterns this namespace mirrors." (:require @@ -152,10 +154,11 @@ ;; operation attaches it to its own event. (defn- audited - "Attach the `:duration-ms` and `:outcome` props to the audit event of a - branch operation, and print the matching log line. `result` is returned - unchanged to the caller, and `extra` lets an operation add numbers of - its own to the same record. + "Attach the `:branch-operation`, `:branch-outcome` and + `:branch-duration-ms` props to the audit event of a branch operation, + and print the matching log line. `result` is returned unchanged to the + caller, and `extra` lets an operation add numbers of its own to the + same record. A listing returns a vector rather than a map, and a vector carries metadata exactly like the maps the other commands return, so the props @@ -178,9 +181,12 @@ ;; --- Helpers: media pairing ;; -;; `duplicate-file` (branch creation) gives the branch's file_media_object -;; rows FRESH ids and relinks the branch `:data` to them, so the same image -;; carries a different id on each side. Before diffing or merging, the +;; A branch copies no media at creation: the content it inherits from its +;; base references main's file_media_object rows. Rows cross between the +;; two files only as copies under FRESH ids (`copy-media-rows!`): an update +;; from main copies the media main added into the branch, and a merge +;; copies the media the branch added into main. So the same image can +;; carry a different id on each side. Before diffing or merging, the ;; branch-side media ids must be normalized back to the target's ids ;; (together with the file id, via `bm/remap-refs`); otherwise every image ;; looks changed, and merged shapes would reference media rows owned by the @@ -195,9 +201,10 @@ (defn- media-pairs "{from-media-id -> to-media-id} for the file_media_object rows present on both files with the same content identity (storage object + name + - dimensions + mtype) — i.e. the rows `duplicate-file` copied at branch - creation. Rows with several identical copies are paired positionally - (they are interchangeable). Media added on one side stays unpaired." + dimensions + mtype), such as the rows one file received as a copy of + the other's (`copy-media-rows!`, run by an update from main or a merge). + Rows with several identical copies are paired positionally (they are + interchangeable). Media added on one side stays unpaired." [cfg from-file-id to-file-id] (let [ident (juxt :media-id :name :width :height :mtype) from-gs (group-by ident (db/exec! cfg [sql:file-media-identity from-file-id])) @@ -470,9 +477,10 @@ ;; 6. Persist the branch metadata. `base-revn` ;; tracks MAIN's revision counter and - ;; `base-branch-revn` the BRANCH file's one (they - ;; coincide at creation but drift apart: they are - ;; independent counters). + ;; `base-branch-revn` the BRANCH file's one. Each + ;; starts at its own side's revn (main's current + ;; revn, the new branch file's 0) and is compared + ;; against that side alone (see `revn-deltas`). (db/insert! conn :file-branch {:id meta-id :branch-file-id branch-id @@ -759,7 +767,9 @@ (sv/defmethod ::get-branch-diff "Read-only three-way diff between a branch and its source (main), - using the merge base captured at branch creation. Returns the summary + using the branch's current merge base: the snapshot taken at branch + creation, or the one the latest update from main repositioned it to + (`reposition-base!` in `update-branch-from-main`). Returns the summary produced by `branch-merge/compute-merge` (stats, changes, conflicts). NOTE (Phase 2): base/main/branch are assumed to share the same file @@ -830,13 +840,14 @@ (sv/defmethod ::merge-file-branch "Merge a branch into its source file (main). - Phase 3 scope: clean merges only. If the three-way diff has conflicts - the command returns `{:status :conflicts}` (resolution UI lands in a - later phase); if it contains change kinds not yet translatable - (components, pages, tokens) it returns `{:status :unsupported}` so no - change is silently dropped. Otherwise it applies the merge to main - through the production change pipeline, takes a safety snapshot, marks - the branch merged and notifies open clients via msgbus." + Conflicts that `resolutions` leaves unresolved make the command return + `{:status :conflicts}`; a resolved conflict merges the side its + resolution picks, per entity or per attr. Change kinds the engine + cannot translate (`bm/unsupported-kinds`) make it return + `{:status :unsupported}` so no change is silently dropped. Otherwise it + takes a safety snapshot of main, applies the merge to main through the + production change pipeline, marks the branch merged and notifies open + clients via msgbus." {::doc/added "2.16" ::webhooks/event? true ::sm/params schema:merge-file-branch @@ -1411,9 +1422,13 @@ AND fb.deleted_at IS NULL") (sv/defmethod ::get-file-branch-info - "If the given file is a branch, return its branch metadata (with cheap - revn-based ahead/behind); otherwise nil. Returns nil when branching is - disabled, so it is safe to call on every file open." + "If the given file is a branch, return its branch metadata with + entity-level `ahead`/`behind`/`conflicts` counts read through the + summary cache (`cached-diff-counts`); otherwise nil. A cache miss on an + open branch whose revn deltas are not both zero runs the three-way diff + (`branch-diff-counts!`), which loads main, the branch and its base. + Returns nil when branching is disabled, so it is safe to call on every + file open." {::doc/added "2.16" ::sm/params schema:get-file-branch-info} [cfg {:keys [::rpc/profile-id file-id]}] diff --git a/backend/src/app/rpc/commands/files_pull_request.clj b/backend/src/app/rpc/commands/files_pull_request.clj index b041f0570b..e4cf59d2d8 100644 --- a/backend/src/app/rpc/commands/files_pull_request.clj +++ b/backend/src/app/rpc/commands/files_pull_request.clj @@ -7,11 +7,15 @@ (ns app.rpc.commands.files-pull-request "RPC commands for pull requests on top of file branching: a review request over a pinned snapshot of a branch. A pull request does NOT - create a new file; the review sandbox is a virtual read-only view of - the branch served from the pinned snapshot (`get-pull-request-bundle`), - so it disappears when the pull request is closed and the snapshot pin - is released. Merging stays on `app.rpc.commands.files-branch`; approving - a pull request is informative and never a gate for the merge." + create a new file. Reviewers open the live branch file, which stays + editable, so their edits change the branch and mark the pull request + `outdated` (the branch revn moved past the snapshot's). The pinned + snapshot serves only the read-only views: the interactions viewer + (`viewer/get-view-only-bundle` with `:pr-id`) and + `get-pull-request-bundle`. Both refuse once the pull request is no + longer open, and closing it releases the snapshot pin. Merging stays on + `app.rpc.commands.files-branch`; approving a pull request is + informative and never a gate for the merge." (:require [app.binfile.common :as bfc] [app.common.data :as d] diff --git a/common/src/app/common/files/branch_merge.cljc b/common/src/app/common/files/branch_merge.cljc index f2c11fbe2b..e602add9df 100644 --- a/common/src/app/common/files/branch_merge.cljc +++ b/common/src/app/common/files/branch_merge.cljc @@ -7,12 +7,14 @@ (ns app.common.files.branch-merge "Three-way, entity-level merge/diff engine for file branching. - Phase 2 scope: read-only diff. `compute-merge` takes the file `:data` - of the merge base, main and branch and returns a serializable summary - of what the branch changes relative to main, plus the set of - conflicting entities. It never mutates the blob; translating the - result into `changes` (for an actual merge) and token diffing arrive - in later phases. + `compute-merge` takes the file `:data` of the merge base, main and + branch and returns a serializable summary of what the branch changes + relative to main, plus the set of conflicting entities. It never + mutates the blob. `compute-changes` translates the same comparison + into `changes` for an actual merge or update, resolved conflicts + included. It covers every kind `compute-merge` reports, token kinds + included, except the residual page attrs (`:page-attrs`), which + `unsupported-kinds` reports so the caller refuses them. Direction `:branch->main` (merge/compare) treats main as `theirs` and branch as `ours`; `:main->branch` (update from main) swaps them." @@ -127,16 +129,18 @@ "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." + The three sides reach the comparison by different routes: the base is a + stored snapshot, main is its own saved data, and the branch is that base + with its op log replayed over it. They do not always spell an unset + attribute the same way: one side can carry a key like + `:fill-color-ref-file` or `:typography-ref-file` with `nil` where another + omits it, and 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) @@ -428,12 +432,12 @@ ;; --- Tokens --- ;; -;; Token *values* (tokens within an existing set) are mergeable (kind -;; :token -> :set-token). Structural token changes (adding/renaming sets, -;; themes, active-theme/active-set toggles) are surfaced with -;; non-mergeable kinds (:token-set, :token-theme, :token-active-themes) -;; so the merge refuses them rather than dropping them silently — a full -;; structural token merge is a later step. +;; Every token change is mergeable. Token *values* (tokens within an +;; existing set) carry kind :token -> :set-token. Structural changes +;; carry their own kinds, each translated by `compute-changes`: set +;; add/delete :token-set, rename/description :token-set-rename, set order +;; :token-set-order, themes :token-theme, active themes +;; :token-active-themes and active-set toggles :token-active-sets. (defn- lib-set-ids [lib] @@ -560,7 +564,7 @@ {:kind :token-set}) (presence-only) (update :conflicts #(slim-conflict-sides slim-set %))) - ;; set rename/description on common sets (NOT yet mergeable) + ;; set rename/description on common sets (mergeable: :token-set-rename) rename (three-way-entities (select-keys (lib-set-meta bl) common) (select-keys (lib-set-meta tl) common) (select-keys (lib-set-meta ol) common) @@ -578,7 +582,7 @@ {:active-themes (lib-active-paths tl)} {:active-themes (lib-active-paths ol)} {:kind :token-active-themes}) - ;; active-set toggles (hidden theme) — NOT yet mergeable + ;; active-set toggles (hidden theme) (mergeable: :token-active-sets) active-sets (three-way-entities {:active-sets (lib-hidden-sets bl)} {:active-sets (lib-hidden-sets tl)} {:active-sets (lib-hidden-sets ol)} @@ -602,21 +606,24 @@ (defn remap-refs "Rewrite cross-file references in `data` through `id-map` - ({old-id -> new-id}): the file refs relinked by `duplicate-file` + ({old-id -> new-id}): the file refs `cfh/relink-refs` rewrites (`:component-file`, `:fill-color-ref-file`, `:stroke-color-ref-file`, `: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) — the exact same - surface `binfile.common/process-file` remaps when the branch copy is - created. + collection keys and library-color `:image` ids). It is the same surface + `binfile.common/process-file` remaps when it copies a file. - A branch is a file copy whose LOCAL references were re-pointed to its - own ids by the duplication pipeline. Before diffing or merging against - another file those references must be normalized to the target's ids; - otherwise (a) every affected entity looks modified, inflating the - diff/conflicts, and (b) merged references cannot be resolved in the - target file — repair would detach components and images would break. - Ids not present in `id-map` (external libraries) are left untouched." + 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 + (`binfile.common/branch-file-data`). What it inherited from the base + keeps the source's ids, but the references the branch writes itself + name the branch file and the media rows it owns. Before diffing or + merging against another file those references must be normalized to + the target's ids; otherwise (a) every affected entity looks modified, + inflating the diff/conflicts, and (b) merged references cannot be + resolved in the target file — repair would detach components and + images would break. Ids not present in `id-map` (external libraries) + are left untouched." [data id-map] (if (empty? id-map) data @@ -760,19 +767,19 @@ NOT sound for the direction that reports main's changes, and passing it there would hide them. - NOTE: token-lib diffing is not yet implemented (handled in a later - phase); `:tokens-lib` changes are not reported here." + The `:tokens-lib` is diffed as well (`diff-tokens`), so token changes + are reported here with their own kinds." ([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))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; -;; MERGE -> CHANGES (Phase 3, no-conflict path) +;; MERGE -> CHANGES (clean changes plus resolved conflicts) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; -;; Kinds `compute-changes` can translate into change ops. Components, -;; pages (add/remove/rename) and tokens are not yet supported and cause -;; the merge to refuse rather than silently drop changes. +;; Kinds `compute-changes` can translate into change ops: every kind +;; `compute-merge` reports except the residual page attrs (`:page-attrs`), +;; which make the merge refuse rather than silently drop changes. (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 @@ -1041,8 +1048,8 @@ no conflict remains unresolved before applying). Returns `{:changes [..] :unsupported #{kinds..}}`. When `:unsupported` - is non-empty the caller must refuse the merge (translation for those - kinds — components, pages, tokens — is not implemented yet). + is non-empty the caller must refuse the merge (the only kind it cannot + translate is `:page-attrs`, the residual page attrs no pass handles). The 5-arity accepts the `compute-merge` summary the caller usually already computed (for the conflict gate), so the full three-way diff is diff --git a/frontend/src/app/main/data/workspace/notifications.cljs b/frontend/src/app/main/data/workspace/notifications.cljs index 117bd6bbac..bb8f1a7072 100644 --- a/frontend/src/app/main/data/workspace/notifications.cljs +++ b/frontend/src/app/main/data/workspace/notifications.cljs @@ -329,8 +329,11 @@ (rx/of (ptk/event ::dw/reload-current-file))))))) (defn handle-file-merged - "A branch was merged into this file (main). Reload the current file so - the merged changes appear for everyone with main open." + "Another session integrated changes into this file: a merge of a branch + into main (sent on main's topic) or an update of a branch from main + (sent on the branch file's topic). Reload the current file so the + integrated changes appear for everyone with that file open. The + websocket drops the message for the session that made the change." [{:keys [file-id] :as _msg}] (ptk/reify ::handle-file-merged ptk/WatchEvent diff --git a/frontend/src/app/main/data/workspace/pull_requests.cljs b/frontend/src/app/main/data/workspace/pull_requests.cljs index d46ed6ba2f..17a0db7d64 100644 --- a/frontend/src/app/main/data/workspace/pull_requests.cljs +++ b/frontend/src/app/main/data/workspace/pull_requests.cljs @@ -6,10 +6,12 @@ (ns app.main.data.workspace.pull-requests "Data layer for pull requests over file branches. A pull request points - to a pinned review snapshot of its branch; the review sandbox is that - snapshot loaded read-only into the workspace (same mechanics as the - version-history preview in `app.main.data.workspace.versions`), so it - is never an editable copy of anything. Mirrors the patterns of + to a pinned review snapshot of its branch. Reviewing opens the live + branch file as a normal, editable workspace with a review context + attached (`:workspace-pr-preview`, which drives the review banner and + header), so a reviewer's edits change the branch and mark the pull + request outdated. The pinned snapshot backs the interactions viewer and + the outdated flag, not the canvas. Mirrors the patterns of `app.main.data.workspace.branches`." (:require [app.common.data :as d] @@ -253,13 +255,14 @@ (rx/of (ntf/error (tr "workspace.pull-requests.lifecycle.error")))))))))) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; -;; REVIEW SANDBOX (read-only preview of the pinned snapshot) +;; REVIEW CONTEXT (the live, editable branch file plus the review banner) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (defn open-pull-request - "Navigate into the review sandbox of `pr`: the branch file's workspace - with the `pr-id` query param, which loads the pinned snapshot - read-only once the file is initialized." + "Navigate into the review of `pr`: the branch file's workspace with the + `pr-id` query param, which attaches the review context once the file is + initialized (`initialize-pull-request-preview`). The canvas is the live + branch, editable as always." [pr] (assert (uuid? (:id pr)) "expected a pull request row with a valid `:id`") (ptk/reify ::open-pull-request diff --git a/frontend/src/app/main/ui/workspace.cljs b/frontend/src/app/main/ui/workspace.cljs index 9995b84f0d..5355b3cf48 100644 --- a/frontend/src/app/main/ui/workspace.cljs +++ b/frontend/src/app/main/ui/workspace.cljs @@ -94,9 +94,9 @@ pr-preview (mf/deref refs/pull-request-preview)] [:* (when (not ^boolean hide-ui?) - ;; inside a review sandbox the pull request banner replaces the - ;; branch banner: the user is reviewing a pinned snapshot, not - ;; editing the branch + ;; during a pull request review the review banner replaces the + ;; branch banner. The canvas is still the live, editable branch: + ;; edits change it and mark the pull request outdated (if (some? pr-preview) [:> pr-review-banner* {}] [:> branch-context-banner* {:file-id (get file :id)}])) diff --git a/frontend/src/app/main/ui/workspace/left_header.cljs b/frontend/src/app/main/ui/workspace/left_header.cljs index 6f56819f77..74b6be4a4b 100644 --- a/frontend/src/app/main/ui/workspace/left_header.cljs +++ b/frontend/src/app/main/ui/workspace/left_header.cljs @@ -41,9 +41,9 @@ branch-ctx (mf/deref refs/branch-context) branch? (and (some? branch-ctx) (some? (:source-name branch-ctx))) - ;; When a pull request review sandbox is active the header gets a - ;; distinct badge and title so the snapshot is never mistaken for - ;; the editable branch. + ;; When a pull request review is active the header gets a + ;; distinct badge and the pull request title, so the user knows a + ;; review is open. The file is still the live, editable branch. pr-preview (mf/deref refs/pull-request-preview) review? (some? pr-preview) pr-title (get-in pr-preview [:info :title] "") diff --git a/frontend/src/app/main/ui/workspace/left_header.scss b/frontend/src/app/main/ui/workspace/left_header.scss index 3738ae7f78..aa37332394 100644 --- a/frontend/src/app/main/ui/workspace/left_header.scss +++ b/frontend/src/app/main/ui/workspace/left_header.scss @@ -94,8 +94,8 @@ color: var(--button-secondary-foreground-color-rest); } -// pull request review sandbox: accent-colored so the read-only snapshot -// is never mistaken for the editable branch +// pull request review: accent-colored so an active review stands out from +// the plain branch badge (the file is still the live, editable branch) .review-badge { @include deprecated.flex-center; diff --git a/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs b/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs index be35c6367d..4ab66444ad 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/pull_requests.cljs @@ -484,9 +484,9 @@ team (mf/deref refs/team) author? (= (:created-by pr) (:id profile)) reviewer? (some #(= (:profile-id %) (:id profile)) (:reviews pr)) - ;; merging keeps requiring edition permissions on main: the - ;; sandbox file permissions are stripped, but the team role tells - ;; whether this user could integrate the branch + ;; merging requires edition permissions on main, not on the open + ;; branch file, so the team role tells whether this user could + ;; integrate the branch can-merge? (boolean (get-in team [:permissions :can-edit])) [approvals total] (current-approvals pr) diff --git a/frontend/src/app/main/ui/workspace/sidebar/pull_requests.scss b/frontend/src/app/main/ui/workspace/sidebar/pull_requests.scss index cfc762468f..d51f906f7a 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/pull_requests.scss +++ b/frontend/src/app/main/ui/workspace/sidebar/pull_requests.scss @@ -588,8 +588,9 @@ max-inline-size: 90vw; border-radius: deprecated.$s-8; - // the review sandbox must never be confused with the editable branch: - // accent-colored border instead of the branch banner's neutral one + // the review banner must stand out from the branch banner it replaces + // (the canvas is still the live branch): accent-colored border instead + // of the branch banner's neutral one border: deprecated.$s-2 solid var(--color-accent-primary); background-color: var(--color-background-primary);