docs(unification): spec + phased plan for one-component-per-feature
Maps duplication into three buckets (parallel impls, entry-point sprawl, dead/leftover) and defines six phased unification slices: groundwork, DialogService, MergeCoordinator, WorktreeActions, AgentConfigeditor, unified DiffViewer.
This commit is contained in:
Mika Kuns
@@ -0,0 +1,91 @@
|
||||
# Feature unification — one component per feature
|
||||
|
||||
Date: 2026-06-19
|
||||
|
||||
## Goal
|
||||
|
||||
ClaudeDo grew organically; several features now exist as parallel implementations
|
||||
or are reachable through many hand-wired entry points. This design maps the
|
||||
duplication and defines a target where **each feature is one component**, reached
|
||||
through one path, with dead code removed.
|
||||
|
||||
## Method
|
||||
|
||||
Mapped via five parallel exploration agents (merge/conflict, review→merge,
|
||||
diff+worktree, task create/edit, UI entry-point inventory), then verified the
|
||||
load-bearing claims by grep/read before writing this. Every file:line below was
|
||||
confirmed against the working tree on 2026-06-19.
|
||||
|
||||
## Key finding: it is NOT three merge engines
|
||||
|
||||
There is **one** merge engine (`TaskMergeService`), wrapped **once** for multi-child
|
||||
units (`PlanningMergeOrchestrator`), with **one** conflict resolver (the Rider
|
||||
3-pane). `Worker/CLAUDE.md` already records "there is no separate 'Merge all' entry —
|
||||
approve is the single review+merge action." What *looks* like 2–3 merge features is
|
||||
**entry-point sprawl** in the UI plus **one dead hunks-API** left over from the
|
||||
Layer-C rework. So unification is mostly UI plumbing + deletion, not re-architecting
|
||||
the engine.
|
||||
|
||||
## Findings — three buckets
|
||||
|
||||
### Bucket A — genuine duplication (parallel implementations of one job)
|
||||
|
||||
| # | Feature | Duplicated components | Shared already |
|
||||
|---|---|---|---|
|
||||
| A1 | Diff viewing | `DiffModalViewModel` (worktree + commit-range), `WorktreeModalViewModel` (file-tree + per-file), `PlanningDiffViewModel` (per-subtask + integration) | `UnifiedDiffParser`, `DiffLinesView` (good) |
|
||||
| A2 | Agent-config editing | `ListSettingsModalViewModel` (list scope), `AgentSettingsSectionViewModel` (task scope); global lives in `SettingsModalViewModel` | `InheritanceResolver`, `InheritedBadge` (good) |
|
||||
| A3 | Worktree actions | `WorktreesOverviewModalViewModel` per-row cmds (Merge/Discard/Keep/ForceRemove/ShowDiff/Jump) vs `MergeSectionViewModel` (Merge/OpenDiff) | same `IWorkerClient` calls |
|
||||
| A4 | Merge display | `AgentStripView` re-displays `MergeSectionViewModel` state | — |
|
||||
|
||||
### Bucket B — entry-point sprawl (one backend, many hand-wired doors)
|
||||
|
||||
| # | Feature | Doors | Evidence |
|
||||
|---|---|---|---|
|
||||
| B1 | Conflict-resolution seam | 5 copies of `Func<string,string,Task>? RequestConflictResolution` | `WorktreesOverviewModalViewModel.cs:83`, `DiffModalViewModel.cs:75`, `MergeModalViewModel.cs:33`, `MergeSectionViewModel.cs:51`, `DetailsIslandViewModel.cs:347` (delegates). Threaded through `MainWindow.axaml.cs:81`, `IslandsShellViewModel.cs:49/202`, `DiffModalViewModel.cs:103`, `MergeSectionViewModel.cs:159` |
|
||||
| B2 | Diff (open) | 3–4 | MergeSection "Open Diff", TaskHeaderBar "Review Merged Diff", WorktreesOverview "Show Diff", Planning "Review Combined" |
|
||||
| B3 | List Settings dialog | 3 | Lists context menu, Tasks header button, shell bridge `IslandsShellViewModel.cs:190-194` |
|
||||
| B4 | Worktrees Overview | 2–3 | Repos menu (global), Lists context menu (per-list) |
|
||||
| B5 | Repo Import | 2 | Repos menu, Lists footer button |
|
||||
|
||||
The conflict-resolution *target* is already single-point (`IslandsShellViewModel.RequestConflictResolutionAsync`, line 49). What is duplicated is the **seam plumbing**: five VMs each own the Func and it is threaded by hand.
|
||||
|
||||
### Bucket C — dead / leftover
|
||||
|
||||
| # | Item | Evidence |
|
||||
|---|---|---|
|
||||
| C1 | Dead hunks conflict API | `TaskMergeService.GetConflictsAsync` (`Lifecycle/TaskMergeService.cs:250`) ← `WorkerHub.GetMergeConflicts` (`Hub/WorkerHub.cs:378`) ← `WorkerClient` `"GetMergeConflicts"` (`Services/WorkerClient.cs:276`) ← `IWorkerClient`. Live resolver uses `GetMergeConflictDocuments` (`WorkerHub.cs:389`). Only `TaskMergeServiceTests.cs:672` still references the old one. |
|
||||
| C2 | Two task-creation paths | UI quick-add `TasksIslandViewModel.AddAsync` writes EF directly (`db.Tasks.Add`); MCP `ExternalMcpService.AddTask` is the service path. They can drift. |
|
||||
| C3 | Stale worktrees | `.claude/worktrees/feat+planning-sessions-ui/…` carries old copies of `DiffModalViewModel`/`ListSettingsModalViewModel`/`WorktreeModalViewModel`; layer-c resolver leftovers. Worktree hygiene, not main code. |
|
||||
| C4 | Naming drift (deferred) | Hub `StartConflictMerge`/`ContinueConflictMerge`/`AbortConflictMerge` (`WorkerHub.cs:367/405/414`) vs service `MergeAsync`/`ContinueMergeAsync`/`AbortMergeAsync`. **Documented as intentional** at `Worker/CLAUDE.md:153`. |
|
||||
|
||||
## Targets — one component per feature
|
||||
|
||||
1. **MergeCoordinator (B1).** Replace the five `RequestConflictResolution` Func seams with one injected coordinator exposing `MergeAsync(taskId, targetBranch)` that owns the "merge → on-conflict open resolver" sequence. Every door (review Approve, Diff Merge button, WorktreesOverview single + batch, Details merge section) calls it. The single resolution point (`IslandsShellViewModel.RequestConflictResolutionAsync`) becomes the coordinator's body.
|
||||
2. **DiffViewer (A1 + B2).** One `DiffViewerViewModel` + view with a `DiffSource` abstraction (`DirtyWorktree | BranchVsBase | CommitRange | PlanningAggregate | IntegrationBranch`) and an optional file-tree pane. Replaces `DiffModal` + `WorktreeModal` + `PlanningDiff` shells; keeps `UnifiedDiffParser`/`DiffLinesView`. All B2 doors open it with a different source.
|
||||
3. **WorktreeActions (A3).** One `WorktreeActionsViewModel` for a single task's worktree (merge/diff/discard/keep/force-remove), reused by both the overview rows and the Details merge section instead of each owning copies.
|
||||
4. **AgentConfigEditor (A2).** One editor component parameterized by scope (`Global | List | Task`) over `InheritanceResolver`, embedded in Settings, List Settings, and the Details panel. Collapses the duplicated property set + reset commands + badges.
|
||||
5. **DialogService (B3–B5).** Consolidate the per-modal `Show*` Func seams (`IslandsShellViewModel.cs:59-71`) into one `IDialogService` with typed open methods (`OpenListSettings(list)`, `OpenRepoImport()`, `OpenWorktreesOverview(listId?)`…). Menu, context menu, and footer all call the same method; duplicate command definitions across `ListsIsland`/shell collapse to one.
|
||||
6. **Single task-creation path (C2).** Route UI quick-add through the same creation path MCP `AddTask` uses (repository/service), so both honor the same invariants.
|
||||
|
||||
Plus **C1** (delete dead hunks API + its test) and **C3** (prune stale worktrees) as groundwork. **C4** naming alignment is **deferred** — it is documented-intentional and would churn the hub + `WorkerClient` + every `IWorkerClient` fake (see the "fakes to sync" hazard) for cosmetic gain.
|
||||
|
||||
## Decisions
|
||||
|
||||
- **Phased, each phase ships green.** Six independently buildable/committable slices; cheapest and lowest-risk first (see the plan). No big-bang.
|
||||
- **One plan file per slice.** Matching the 2026-06-05 layer-A/B/C convention, each slice gets its own `docs/superpowers/plans/2026-06-19-unify-<slice>.md` authored when it is picked up. This umbrella plan sequences them and details Phase 0–1.
|
||||
- **DiffViewer (A1) is last.** Highest effort and most UX-sensitive (file-tree vs whole-unified are different layouts); deferring it lets the cheaper wins land first and de-risks the big one.
|
||||
- **Keep the merge engine and the resolver seam contract.** `TaskMergeService`, `PlanningMergeOrchestrator`, `ConflictResolverViewModel` ctor/`OpenAsync`/`OpenForPlanningAsync`/`CloseRequested` are unchanged — unification is above them.
|
||||
- **Naming alignment deferred, not done** (rationale above).
|
||||
|
||||
## Out of scope / deferred
|
||||
|
||||
- Hub/service merge-method renaming (C4).
|
||||
- Subtask deletion in the UI (a missing feature surfaced during mapping, not a duplicate).
|
||||
- Any DB migration, worker engine change, or push.
|
||||
|
||||
## Acceptance (per phase)
|
||||
|
||||
Each phase: `dotnet build -c Release` clean for touched projects; the relevant test
|
||||
project green; locales in parity (Localization.Tests) where keys change; the feature
|
||||
reachable through its single new path with the old doors removed or delegating. UI
|
||||
phases (2–5) flag a visual-verification gap for Mika to confirm in the running app.
|
||||
Reference in New Issue
Block a user