docs(design): git tab merge & review rework — shared foundation + 3 layers
Design for simplifying single-task review/merge (Layer A), multi-worktree batch merge cockpit (Layer B), and inline conflict resolver (Layer C), with frozen shared contracts so B/C build in parallel worktrees. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
mika kuns
@@ -0,0 +1,197 @@
|
|||||||
|
# Git Tab / Merge & Review Rework — Design
|
||||||
|
|
||||||
|
Date: 2026-06-05
|
||||||
|
Status: Approved
|
||||||
|
|
||||||
|
## Goal
|
||||||
|
|
||||||
|
Make handling merges and reviews as simple as possible in the Terminal component's
|
||||||
|
Git tab, and rework the diff viewers and worktree modals along the way. The work is
|
||||||
|
split into three layers built across separate sessions, with a shared foundation that
|
||||||
|
is built and pushed first so the parallel sessions branch from frozen contracts.
|
||||||
|
|
||||||
|
The user mostly trusts task output but wants the diff one click away for important
|
||||||
|
work, and wants to land several independently-queued worktrees without per-task
|
||||||
|
hopping or hand-resolving conflicts in an external editor.
|
||||||
|
|
||||||
|
## Layers
|
||||||
|
|
||||||
|
- **Layer A — Review/merge cockpit** (this session). Single-task review + merge UX in
|
||||||
|
the Git tab; consolidate the four diff renderers into one `DiffView`.
|
||||||
|
- **Layer B — Multi-worktree merge cockpit** (parallel session). Batch-merge N
|
||||||
|
worktrees into one target, skip-and-continue, conflicts collected for resolution.
|
||||||
|
- **Layer C — Inline conflict resolver** (parallel session). VSCode-style inline hunk
|
||||||
|
resolver plus the worker-side conflict plumbing it needs.
|
||||||
|
|
||||||
|
They stack: A defines the single-task flow, B reuses it for many tasks, both funnel
|
||||||
|
conflicts into C.
|
||||||
|
|
||||||
|
## Shared foundation (built & pushed this session, before B/C branch)
|
||||||
|
|
||||||
|
Everything B and C depend on lands first on `main`. B and C branch from that commit.
|
||||||
|
|
||||||
|
### 1. One diff model + one `DiffView` control
|
||||||
|
|
||||||
|
Today there are four diff renderers and two parallel diff models:
|
||||||
|
|
||||||
|
- `DiffLinesView.axaml` (used by `DiffModalView`)
|
||||||
|
- the inline diff `ItemsControl` in `WorktreeModalView.axaml`
|
||||||
|
- `PlanningDiffView.axaml`
|
||||||
|
- their backing models: `DiffFileViewModel`/`DiffLineViewModel` (+ `UnifiedDiffParser`)
|
||||||
|
vs `WorktreeNodeViewModel`/`WorktreeDiffLineViewModel`
|
||||||
|
|
||||||
|
Collapse into a single canonical diff model + parser + a `DiffView` UserControl. All
|
||||||
|
diff rendering across the app goes through `DiffView`.
|
||||||
|
|
||||||
|
- Model: `DiffFileViewModel { Path, AddCount, DelCount, Lines }`,
|
||||||
|
`DiffLineViewModel { OldNo, NewNo, Kind (Add|Del|Ctx|File|Hunk), Text }`.
|
||||||
|
- Parser: one static `UnifiedDiffParser.Parse(rawUnifiedDiff)` returning the model.
|
||||||
|
- `DiffView` exposes a `Files` styled property (file list + selected-file lines), or a
|
||||||
|
simpler `Lines` property for single-file use — Layer A decides the exact surface
|
||||||
|
while building it, but the type names above are frozen so B and C can bind to them.
|
||||||
|
|
||||||
|
### 2. Frozen worker conflict contract
|
||||||
|
|
||||||
|
Added to `IWorkerClient` (and `WorkerClient` with stub bodies that throw
|
||||||
|
`NotSupportedException`) plus new DTOs, so A and B compile against the interface while
|
||||||
|
C provides the real worker-side implementation.
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
// IWorkerClient additions (signatures frozen this session)
|
||||||
|
Task<MergeResultDto> StartConflictMergeAsync(string taskId, string targetBranch);
|
||||||
|
Task<MergeConflictsDto> GetMergeConflictsAsync(string taskId);
|
||||||
|
Task WriteConflictResolutionAsync(string taskId, string path, string resolvedContent);
|
||||||
|
Task<MergeResultDto> ContinueMergeAsync(string taskId);
|
||||||
|
Task AbortMergeAsync(string taskId);
|
||||||
|
```
|
||||||
|
|
||||||
|
- `StartConflictMergeAsync` performs the merge with `leaveConflictsInTree: true` (the
|
||||||
|
worker already supports this flag — used today by the planning orchestrator) and
|
||||||
|
returns `MergeResultDto` with `Status="conflict"` and the conflict file list, leaving
|
||||||
|
`.git/MERGE_HEAD` in place in the list's `WorkingDir`.
|
||||||
|
- `GetMergeConflictsAsync` returns each conflicted file with ours/theirs/base content,
|
||||||
|
read via `git show :2:<path>` (ours), `:3:<path>` (theirs), `:1:<path>` (base).
|
||||||
|
- `WriteConflictResolutionAsync` writes resolved content to the file in `WorkingDir`
|
||||||
|
and `git add`s it.
|
||||||
|
- `ContinueMergeAsync` wraps the existing `TaskMergeService.ContinueMergeAsync`
|
||||||
|
(`git add -A` → re-check `git diff --name-only --diff-filter=U` → `git commit`).
|
||||||
|
- `AbortMergeAsync` wraps the existing `TaskMergeService.AbortMergeAsync`
|
||||||
|
(`git merge --abort`).
|
||||||
|
|
||||||
|
New DTOs (defined in the worker hub DTO file, mirrored client-side):
|
||||||
|
|
||||||
|
```csharp
|
||||||
|
public record MergeConflictsDto(string TaskId, IReadOnlyList<ConflictFileDto> Files);
|
||||||
|
public record ConflictFileDto(string Path, IReadOnlyList<ConflictHunkDto> Hunks);
|
||||||
|
public record ConflictHunkDto(string Ours, string Theirs, string? Base);
|
||||||
|
```
|
||||||
|
|
||||||
|
Existing DTOs reused unchanged: `MergeResultDto(Status, ConflictFiles, ErrorMessage)`,
|
||||||
|
`MergePreviewDto`, `MergeTargetsDto`.
|
||||||
|
|
||||||
|
### 3. Conflict data model (UI)
|
||||||
|
|
||||||
|
`ConflictFile { Path, Hunks[] }`, `ConflictHunk { Ours, Theirs, Base, Resolution }`.
|
||||||
|
Shaped so a future 3-way merge pane needs no model change (Layer C is the inline
|
||||||
|
resolver now; the model leaves room for 3-way later).
|
||||||
|
|
||||||
|
### 4. Integration seams (delegates, wired by the integrator at merge)
|
||||||
|
|
||||||
|
A's and B's cockpits hold a `RequestConflictResolution(string taskId)` callback (an
|
||||||
|
`Action<string>` or `Func<string, Task>`). They never reference Layer C's resolver
|
||||||
|
types. The integrator connects these callbacks to C's `ConflictResolverViewModel`
|
||||||
|
factory when merging the three branches together.
|
||||||
|
|
||||||
|
## Parallel boundaries (verified disjoint)
|
||||||
|
|
||||||
|
| Area | A (this session) | B (parallel) | C (parallel) |
|
||||||
|
|---|---|---|---|
|
||||||
|
| `DiffView` + diff model/parser | builds | reuses | reuses |
|
||||||
|
| `WorkConsole.axaml` / `DetailsIslandViewModel` | owns | — | — |
|
||||||
|
| `DiffModalView` + `PlanningDiffView` | migrates to `DiffView` | — | — |
|
||||||
|
| `WorktreesOverviewModalView/VM` + `WorktreeModalView` | — | owns | — |
|
||||||
|
| `WorkerHub` / `TaskMergeService` / `GitService` | — | — | owns |
|
||||||
|
| New `ConflictResolverView/VM` + conflict UI model | — | — | owns |
|
||||||
|
| `IWorkerClient` / `WorkerClient` | adds frozen stubs + DTOs | reuses `MergeTaskAsync` | fills stub bodies |
|
||||||
|
| Test fakes (`IWorkerClient`) in both test projects | adds new no-op methods | — | makes them functional if needed |
|
||||||
|
|
||||||
|
The only file C and A both touch is `WorkerClient.cs` (C replaces the stub bodies A
|
||||||
|
wrote). Contained; reconciled at integration. Everything else is disjoint.
|
||||||
|
|
||||||
|
## Layer A — review/merge cockpit (this session)
|
||||||
|
|
||||||
|
- The Git tab becomes the single Approve + merge surface. `Approve` and the merge
|
||||||
|
target / preview / diff flow together as one block (no separate REVIEW vs
|
||||||
|
MERGE & WORKTREE sections).
|
||||||
|
- `Continue` (reject → requeue with feedback) and `Reset` (reject → idle) **stay** in
|
||||||
|
the Output tab footer — unchanged.
|
||||||
|
- The diff is shown via the unified `DiffView` opened as a modal from the cockpit. No
|
||||||
|
inline diff recap in the tab (the island is too small).
|
||||||
|
- On a single-task **Approve that conflicts**: instead of today's auto-abort, call
|
||||||
|
`StartConflictMergeAsync` and fire `RequestConflictResolution(taskId)`. This leaves
|
||||||
|
the main checkout mid-merge until the user resolves or aborts (behavior change,
|
||||||
|
intended). The callback is inert until Layer C is merged; the integrator wires it.
|
||||||
|
- Migrate `DiffModalView` and `PlanningDiffView` onto the new `DiffView`.
|
||||||
|
|
||||||
|
### Behavior change accepted
|
||||||
|
|
||||||
|
Today `MergeTask`/`ApproveReview` use `leaveConflictsInTree: false` (auto-abort on
|
||||||
|
conflict). Under this design, an Approve that conflicts leaves the merge in progress
|
||||||
|
and opens the resolver. The mid-merge guard (`IsMidMergeAsync`) still prevents a second
|
||||||
|
concurrent merge.
|
||||||
|
|
||||||
|
## Layer B — multi-worktree merge cockpit (parallel)
|
||||||
|
|
||||||
|
- Rework `WorktreesOverviewModalView`/`WorktreesOverviewModalViewModel` into a
|
||||||
|
batch-merge cockpit: list mergeable worktrees, select N, choose one target branch
|
||||||
|
(single target — 99% of the time everything goes to the same branch), "Merge all".
|
||||||
|
- **Skip-and-continue**: client-side loop calling the existing
|
||||||
|
`MergeTaskAsync(taskId, target, removeWorktree, msg)` per selected task. Clean merges
|
||||||
|
apply; conflicting ones are collected (existing `MergeTaskAsync` auto-aborts on
|
||||||
|
conflict, leaving the tree clean) into a "needs resolution" list with live progress.
|
||||||
|
- Each conflict row exposes a **Resolve** action → `RequestConflictResolution(taskId)`
|
||||||
|
(wired to Layer C at integration).
|
||||||
|
- Per-task diff via the shared `DiffView`; migrate `WorktreeModalView`'s inline diff
|
||||||
|
onto it.
|
||||||
|
- B touches no worker files — keeps it parallel-safe.
|
||||||
|
|
||||||
|
## Layer C — inline conflict resolver (parallel)
|
||||||
|
|
||||||
|
### Worker side
|
||||||
|
|
||||||
|
Implement the five frozen contract methods:
|
||||||
|
|
||||||
|
- Add hub methods `StartConflictMerge`, `GetMergeConflicts`, `WriteConflictResolution`,
|
||||||
|
`ContinueMerge`, `AbortMerge` in `WorkerHub`.
|
||||||
|
- `StartConflictMerge` calls the existing `TaskMergeService.MergeAsync` overload with
|
||||||
|
`leaveConflictsInTree: true`.
|
||||||
|
- `ContinueMerge` / `AbortMerge` wrap the existing `TaskMergeService.ContinueMergeAsync`
|
||||||
|
/ `AbortMergeAsync` (currently service-level only, not hub-exposed).
|
||||||
|
- `GetMergeConflicts` reads ours/theirs/base per conflicted file via
|
||||||
|
`git show :2:/:3:/:1:`; add the `GitService` helpers needed.
|
||||||
|
- `WriteConflictResolution` writes the resolved content to `WorkingDir` and stages it.
|
||||||
|
- Fill the `WorkerClient` stub bodies (real SignalR `InvokeAsync` calls).
|
||||||
|
- Update the hand-rolled `IWorkerClient` fakes in both test projects.
|
||||||
|
|
||||||
|
### UI
|
||||||
|
|
||||||
|
- New `ConflictResolverView` + `ConflictResolverViewModel`. Per conflict hunk, show
|
||||||
|
ours vs theirs stacked, with buttons **Accept Current / Accept Incoming / Accept Both
|
||||||
|
/ Edit manually** plus a free-text box for the merged result of that hunk.
|
||||||
|
- When every file's hunks are resolved → `ContinueMergeAsync(taskId)` → `MergeResultDto`
|
||||||
|
(`merged` closes the resolver; `conflict` means not fully resolved, stay open).
|
||||||
|
- `AbortMergeAsync(taskId)` cancels and aborts the merge.
|
||||||
|
- Expose a factory (`Func<string, ConflictResolverViewModel>`) the integrator wires to
|
||||||
|
A's and B's `RequestConflictResolution` callbacks.
|
||||||
|
|
||||||
|
## Build / test
|
||||||
|
|
||||||
|
`.slnx` needs .NET 9; on .NET 8 build individual csproj with `-c Release` (a running
|
||||||
|
Worker locks `Debug`). Run the relevant test projects. No tests that spawn the real
|
||||||
|
`claude` CLI. Keep `en.json`/`de.json` localization keys in parity.
|
||||||
|
|
||||||
|
## Out of scope
|
||||||
|
|
||||||
|
- Full 3-way synchronized merge editor (model leaves room; not built now).
|
||||||
|
- Per-task differing merge targets in the batch (single target only).
|
||||||
|
- Any CI/PR tooling (direct push-to-main workflow).
|
||||||
Reference in New Issue
Block a user