diff --git a/docs/superpowers/plans/2026-06-19-feature-unification-plan.md b/docs/superpowers/plans/2026-06-19-feature-unification-plan.md new file mode 100644 index 0000000..7fd0848 --- /dev/null +++ b/docs/superpowers/plans/2026-06-19-feature-unification-plan.md @@ -0,0 +1,104 @@ +# Feature unification — phased plan + +Date: 2026-06-19 +Design: `docs/superpowers/specs/2026-06-19-feature-unification-design.md` + +Six slices, sequenced cheapest/lowest-risk first. Each ends green +(`dotnet build -c Release` + the touched test project) and is independently +committable. Phases 0–1 are detailed here; 2–5 are scoped, and each gets its own +`docs/superpowers/plans/2026-06-19-unify-.md` when picked up (per the +2026-06-05 layer-A/B/C convention). Build per-csproj (`-c Release`) — `.slnx` needs +.NET 9 and a running Worker locks `Debug`. + +--- + +## Phase 0 — Groundwork (Bucket C). No UX change. + +**0a. Delete the dead hunks conflict API (C1).** +- Remove `TaskMergeService.GetConflictsAsync` + the `MergeConflicts`/`ConflictFileContent` records it returns (`src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs:250`) if unused elsewhere. +- Remove `WorkerHub.GetMergeConflicts` (`src/ClaudeDo.Worker/Hub/WorkerHub.cs:378`) + `MergeConflictsDto`/`ConflictFileDto`/`ConflictHunkDto` if unused. +- Remove `WorkerClient`'s `"GetMergeConflicts"` invoke (`src/ClaudeDo.Ui/Services/WorkerClient.cs:276`) + the `IWorkerClient` member + every fake override (`tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs`, `TasksIslandViewModelPlanningTests.cs`, others — grep `GetMergeConflicts`). +- Delete `TaskMergeServiceTests.cs:672` `GetConflictsAsync_AfterConflictMerge_ReturnsOursAndTheirs`. +- Verify with grep first: `GetConflictsAsync` and `GetMergeConflicts` have **no** callers outside this chain + tests. +- Acceptance: Worker + Ui build; Worker.Tests + Ui.Tests green; `GetMergeConflictDocuments` path untouched. + +**0b. Single task-creation path (C2).** +- Identify the path MCP `ExternalMcpService.AddTask` uses; expose a thin creation method (repository or a small `TaskCreationService`) that applies the same defaults (ListId, SortOrder, CreatedAt). +- Re-point `TasksIslandViewModel.AddAsync` at it instead of `db.Tasks.Add` direct EF. +- Acceptance: quick-add still works; one creation path; Ui.Tests + Worker.Tests green. + +**0c. Prune stale worktrees (C3).** +- `git worktree list`; remove the orphaned `.claude/worktrees/*` entries (confirm each is unwanted with Mika before `git worktree remove`). +- Acceptance: only intended worktrees remain; no tracked files change. + +> C4 (naming alignment) intentionally NOT in this phase — see design. + +--- + +## Phase 1 — DialogService (B3–B5). Low–medium. + +**Goal:** one `IDialogService` replaces the scattered `Show*` Func seams and the +duplicate open-commands. + +- New `IDialogService` (Ui/Services) with typed methods: `OpenListSettings(ListNavItemViewModel)`, `OpenRepoImport()`, `OpenWorktreesOverview(string? listId)`, `OpenWeeklyReport()`, `OpenAbout()`, `OpenWorkerConnectionHelp()`. Implementation owns the factories + `ModalShell`/TCS wiring currently in `MainWindow.axaml.cs` + `IslandsShellViewModel.cs:59-71`. +- Inject it into `ListsIslandViewModel`, `TasksIslandViewModel`, `IslandsShellViewModel`. Collapse the three List-Settings doors (Lists context menu, Tasks header, shell bridge `IslandsShellViewModel.cs:190-194`) to one `dialogs.OpenListSettings(row)` call; same for Repo Import (2→1) and Worktrees Overview (2→1, keep the `listId?` param for global-vs-per-list). +- Keep `ModalShell`/TCS dialog pattern; this only centralizes *opening*. +- Update fakes/ctors per the IWorkerClient-fakes hazard (ctor changes ripple to Ui.Tests). +- Acceptance: every dialog opens via one method; no duplicate open-commands; Ui.Tests green; visual gap flagged (open each dialog from each former door). + +--- + +## Phase 2 — MergeCoordinator (B1). Medium. + +**Goal:** delete the five `RequestConflictResolution` seams; one coordinator. + +- New `IMergeCoordinator` (Ui) `MergeAsync(taskId, targetBranch)` = the body of `IslandsShellViewModel.RequestConflictResolutionAsync` (`:49`) plus the "open MergeModal → on conflict open resolver" flow currently split across `MergeModalViewModel:108` and `DiffModalViewModel:103`. +- Remove the `Func? RequestConflictResolution` from `WorktreesOverviewModalViewModel:83`, `DiffModalViewModel:75`, `MergeModalViewModel:33`, `MergeSectionViewModel:51`, and the `DetailsIslandViewModel:347` delegate; inject the coordinator instead. +- Re-point doors: review Approve, Diff Merge button, WorktreesOverview single + batch (`:331`), Details merge section. +- Update seam tests (`WorktreesOverviewBatchMergeTests.cs:145`, `DetailsIslandConflictSeamTests.cs:84`) to assert via the coordinator. +- Acceptance: one merge entry API; resolver still opens for single-task AND planning conflict; Ui.Tests green; visual gap flagged (force a conflict from Approve and from the Diff Merge button). + +--- + +## Phase 3 — WorktreeActions (A3). Medium. + +**Goal:** one per-task worktree-actions VM reused by overview rows + Details. + +- New `WorktreeActionsViewModel(taskId)` with Merge/Diff/Discard/Keep/ForceRemove over `IWorkerClient` (uses the Phase-2 coordinator for Merge, the Phase-5 viewer for Diff — until then, current calls). +- `WorktreesOverviewModalViewModel` rows compose one each; `MergeSectionViewModel` hosts one for the active task. Remove the duplicated commands. +- Acceptance: both surfaces drive the same VM; Ui.Tests green; visual gap flagged. + +--- + +## Phase 4 — AgentConfigEditor (A2). Medium. + +**Goal:** one config editor for Global | List | Task scope. + +- New `AgentConfigEditorViewModel(scope)` over `InheritanceResolver` exposing Model/SystemPrompt/AgentPath/MaxTurns + reset commands + `InheritedBadge` state; persists via the scope's hub method (`UpdateListConfig` / `UpdateTaskAgentSettings` / app settings). +- Embed in `SettingsModalViewModel`, `ListSettingsModalViewModel`, and the Details `AgentSettingsSectionViewModel` host; delete the duplicated field/reset logic. +- Acceptance: identical editor in all three scopes; Localization parity; Ui.Tests green; visual gap flagged. + +--- + +## Phase 5 — DiffViewer (A1 + B2). High; last. + +**Goal:** one diff component replaces DiffModal + WorktreeModal + PlanningDiff. + +- New `DiffViewerViewModel` with `DiffSource` enum/abstraction (`DirtyWorktree | BranchVsBase | CommitRange | PlanningAggregate | IntegrationBranch`) and an optional file-tree pane (port `WorktreeModal`'s tree + Avalonia-12 selection workaround); reuse `UnifiedDiffParser` + `DiffLinesView`; keep PlanningDiff's combined-mode toggle as a source switch. +- Re-point all B2 doors to open it with the right source. Remove the three old VMs/views. +- Update `DiffModalViewModelTests`, `PlanningDiffViewModelTests`. +- Acceptance: every diff door opens the one viewer; whole-unified AND file-tree layouts work; Ui.Tests green; visual gap flagged (worktree-dirty, post-merge commit-range, planning per-subtask + integration). + +--- + +## Sequencing rationale + +0 (delete/no-UX) → 1 (isolated, unblocks nothing but cheap) → 2 (coordinator; 3 & 5 +lean on it for Merge/Diff) → 3 → 4 (independent) → 5 (biggest, most UX-sensitive, +benefits from 2's coordinator). Stop after any phase and the app is shippable. + +## Per-phase commits + +Conventional Commits, one per phase (or per sub-step in Phase 0): e.g. +`refactor(merge): single MergeCoordinator replaces 5 conflict seams`. Stage by path +(never `git add -A` — concurrent sessions). Commit the spec + this plan first. diff --git a/docs/superpowers/specs/2026-06-19-feature-unification-design.md b/docs/superpowers/specs/2026-06-19-feature-unification-design.md new file mode 100644 index 0000000..11bcce9 --- /dev/null +++ b/docs/superpowers/specs/2026-06-19-feature-unification-design.md @@ -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? 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-.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.