From 5be4b5c5fba0f0998e026cfb70fb0b548cf6b115 Mon Sep 17 00:00:00 2001 From: Mika Kuns Date: Mon, 22 Jun 2026 17:18:57 +0200 Subject: [PATCH] refactor(merge): single IMergeCoordinator replaces the 5 conflict seams The RequestConflictResolution Func was declared on 5 VMs and hand-threaded shell->details->merge-section->diff->merge-modal. Replaced with a DI-singleton IMergeCoordinator (MergeCoordinator holder; shell wires its Handler at composition, breaking the shell<->island cycle). Invokers (MergeModal, DetailsIsland, WorktreesOverview) depend on the interface; the two pass-through VMs (DiffModal, MergeSection) drop the seam entirely. No behavior change; conflict-seam + batch tests rewired to assert via the coordinator. --- src/ClaudeDo.App/Program.cs | 8 ++++- src/ClaudeDo.Ui/Services/IMergeCoordinator.cs | 29 +++++++++++++++++++ .../Islands/DetailsIslandViewModel.cs | 27 ++++------------- .../Islands/MergeSectionViewModel.cs | 2 -- .../ViewModels/IslandsShellViewModel.cs | 1 - .../ViewModels/Modals/DiffModalViewModel.cs | 2 -- .../ViewModels/Modals/MergeModalViewModel.cs | 28 +++++------------- .../Modals/WorktreesOverviewModalViewModel.cs | 9 +++--- src/ClaudeDo.Ui/Views/WindowDialogService.cs | 2 -- .../DetailsIslandConflictSeamTests.cs | 23 ++++++++------- .../ViewModels/DetailsIslandPlanningTests.cs | 2 +- .../ViewModels/DetailsIslandPrepModeTests.cs | 2 +- .../DetailsIslandReviewActionsTests.cs | 2 +- .../ViewModels/DetailsIslandTabsTests.cs | 2 +- .../WorktreesOverviewBatchMergeTests.cs | 15 ++++++---- 15 files changed, 79 insertions(+), 75 deletions(-) create mode 100644 src/ClaudeDo.Ui/Services/IMergeCoordinator.cs diff --git a/src/ClaudeDo.App/Program.cs b/src/ClaudeDo.App/Program.cs index 992f8e1..c4a4e61 100644 --- a/src/ClaudeDo.App/Program.cs +++ b/src/ClaudeDo.App/Program.cs @@ -116,6 +116,10 @@ sealed class Program return new UpdateCheckService(releases, version); }); + // Conflict-merge coordinator: single seam the shell wires to its resolver entry. + sc.AddSingleton(); + sc.AddSingleton(sp => sp.GetRequiredService()); + // ViewModels sc.AddTransient(); sc.AddTransient>(sp => () => sp.GetRequiredService()); @@ -152,12 +156,14 @@ sealed class Program sp.GetRequiredService>(), sp.GetRequiredService(), sp, - sp.GetRequiredService())); + sp.GetRequiredService(), + sp.GetRequiredService())); sc.AddSingleton(sp => { var shell = ActivatorUtilities.CreateInstance(sp); shell.ConflictResolverFactory = sp.GetRequiredService>(); + sp.GetRequiredService().Handler = shell.RequestConflictResolutionAsync; return shell; }); diff --git a/src/ClaudeDo.Ui/Services/IMergeCoordinator.cs b/src/ClaudeDo.Ui/Services/IMergeCoordinator.cs new file mode 100644 index 0000000..0349697 --- /dev/null +++ b/src/ClaudeDo.Ui/Services/IMergeCoordinator.cs @@ -0,0 +1,29 @@ +using System; +using System.Threading.Tasks; + +namespace ClaudeDo.Ui.Services; + +/// +/// Single entry point for handing a conflicting merge to the in-app 3-pane resolver. +/// Replaces the per-VM RequestConflictResolution Func seams that used to be +/// hand-threaded shell → details → merge-section → diff → merge-modal. The shell wires +/// once at composition; invokers depend only on +/// this interface (injected via DI). +/// +public interface IMergeCoordinator +{ + Task ResolveConflictAsync(string taskId, string targetBranch); +} + +/// +/// DI singleton holding the resolver entry. The holder breaks the shell↔island construction +/// cycle: islands depend on the interface, the shell sets after it is built. +/// +public sealed class MergeCoordinator : IMergeCoordinator +{ + /// Set once at composition to the shell's resolver entry. Null (headless/tests) ⇒ no-op. + public Func? Handler { get; set; } + + public Task ResolveConflictAsync(string taskId, string targetBranch) => + Handler?.Invoke(taskId, targetBranch) ?? Task.CompletedTask; +} diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs index d600687..3fa64be 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs @@ -52,6 +52,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable private readonly IWorkerClient _worker; private readonly IServiceProvider _services; private readonly INotesApi _notesApi; + private readonly IMergeCoordinator _merge; // ── Section view models ─────────────────────────────────────────────────── public AgentSettingsSectionViewModel AgentSettings { get; } @@ -343,13 +344,6 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable public bool HasReviewFeedback => !string.IsNullOrWhiteSpace(ReviewFeedback); - // Kept for backwards-compat surface — delegates to Merge.RequestConflictResolution - public Func? RequestConflictResolution - { - get => Merge.RequestConflictResolution; - set => Merge.RequestConflictResolution = value; - } - private static string StatusToStateKey(ClaudeDo.Data.Models.TaskStatus status) => status switch { ClaudeDo.Data.Models.TaskStatus.Queued => "queued", @@ -404,12 +398,14 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable IDbContextFactory dbFactory, IWorkerClient worker, IServiceProvider services, - INotesApi notesApi) + INotesApi notesApi, + IMergeCoordinator merge) { _dbFactory = dbFactory; _worker = worker; _services = services; _notesApi = notesApi; + _merge = merge; AgentSettings = new AgentSettingsSectionViewModel(worker); Merge = new MergeSectionViewModel(worker, services); @@ -1151,20 +1147,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable var hasChildren = Subtasks.Count > 0 || ChildOutcomes.Count > 0; var result = await _worker.ApproveReviewAsync(Task.Id, Merge.SelectedMergeTarget ?? ""); if (!hasChildren && result?.Status == "conflict") - { - if (Merge.RequestConflictResolution is not null) - { - await Merge.RequestConflictResolution(Task.Id, Merge.SelectedMergeTarget ?? ""); - } - else - { - var (text, _, _) = MergePreviewPresenter.Describe( - new MergePreviewDto("conflict", result.ConflictFiles, 0)); - Merge.MergePreviewText = text; - Merge.MergeIsClean = false; - Merge.MergeIsConflict = true; - } - } + await _merge.ResolveConflictAsync(Task.Id, Merge.SelectedMergeTarget ?? ""); } catch (Exception ex) { diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs index 568f871..3895f5a 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs @@ -48,7 +48,6 @@ public sealed partial class MergeSectionViewModel : ViewModelBase public Func? ShowDiffModal { get; set; } public Func? ShowMergeModal { get; set; } public Func? ShowPlanningDiffModal { get; set; } - public Func? RequestConflictResolution { get; set; } public MergeSectionViewModel(IWorkerClient worker, IServiceProvider services) { @@ -156,7 +155,6 @@ public sealed partial class MergeSectionViewModel : ViewModelBase TaskTitle = TaskTitle ?? "", ShowMergeModal = ShowMergeModal, ResolveMergeVm = () => _services.GetRequiredService(), - RequestConflictResolution = RequestConflictResolution, }; } else if (CanDiffMergedRange) diff --git a/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs b/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs index 29576c9..a11a6b6 100644 --- a/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs @@ -194,7 +194,6 @@ public sealed partial class IslandsShellViewModel : ViewModelBase, IDisposable _ = Lists.RefreshCountsAsync(); return System.Threading.Tasks.Task.CompletedTask; }; - Details.RequestConflictResolution = RequestConflictResolutionAsync; Worker.PropertyChanged += (_, e) => { if (e.PropertyName is nameof(IWorkerClient.IsConnected) or nameof(IWorkerClient.IsReconnecting)) diff --git a/src/ClaudeDo.Ui/ViewModels/Modals/DiffModalViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Modals/DiffModalViewModel.cs index b7a3f9d..2ffe596 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/DiffModalViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/DiffModalViewModel.cs @@ -72,7 +72,6 @@ public sealed partial class DiffModalViewModel : ViewModelBase public string TaskTitle { get; init; } = ""; public Func? ShowMergeModal { get; set; } public Func? ResolveMergeVm { get; set; } - public Func? RequestConflictResolution { get; set; } public ObservableCollection Files { get; } = new(); @@ -100,7 +99,6 @@ public sealed partial class DiffModalViewModel : ViewModelBase { if (TaskId is null || ShowMergeModal is null || ResolveMergeVm is null) return; var vm = ResolveMergeVm(); - vm.RequestConflictResolution = RequestConflictResolution; await vm.InitializeAsync(TaskId, TaskTitle); await ShowMergeModal(vm); // The diff is stale once the worktree merged away or a conflict opened the editor. diff --git a/src/ClaudeDo.Ui/ViewModels/Modals/MergeModalViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Modals/MergeModalViewModel.cs index c9aa681..351499a 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/MergeModalViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/MergeModalViewModel.cs @@ -9,6 +9,7 @@ namespace ClaudeDo.Ui.ViewModels.Modals; public sealed partial class MergeModalViewModel : ViewModelBase { private readonly IWorkerClient _worker; + private readonly IMergeCoordinator _merge; public string TaskId { get; set; } = ""; public string TaskTitle { get; set; } = ""; @@ -28,10 +29,6 @@ public sealed partial class MergeModalViewModel : ViewModelBase public Action? CloseAction { get; set; } - /// Set by the caller to hand a conflicting merge off to the in-app 3-pane editor - /// instead of dead-ending on the conflict message. - public Func? RequestConflictResolution { get; set; } - /// True once a merge has succeeded — lets the caller (e.g. the diff window) /// close itself after this modal closes. public bool Merged { get; private set; } @@ -39,9 +36,10 @@ public sealed partial class MergeModalViewModel : ViewModelBase /// True once a conflict has been handed off to the resolver — also a cue to close the diff window. public bool RoutedToResolver { get; private set; } - public MergeModalViewModel(IWorkerClient worker) + public MergeModalViewModel(IWorkerClient worker, IMergeCoordinator merge) { _worker = worker; + _merge = merge; } public async Task InitializeAsync(string taskId, string taskTitle) @@ -103,21 +101,11 @@ public sealed partial class MergeModalViewModel : ViewModelBase }); break; case "conflict": - // Hand off to the in-app 3-pane merge editor when wired (MergeTask aborted - // cleanly, so the resolver re-starts the merge leaving conflicts in the tree). - if (RequestConflictResolution is not null) - { - var branch = SelectedBranch!; - RoutedToResolver = true; - CloseAction?.Invoke(); - await RequestConflictResolution(TaskId, branch); - } - else - { - HasConflict = true; - ConflictFiles = result.ConflictFiles; - ErrorMessage = Loc.T("vm.merge.conflict"); - } + // MergeTask aborted cleanly; hand the conflict to the in-app 3-pane editor, + // which re-starts the merge leaving conflicts in the tree. + RoutedToResolver = true; + CloseAction?.Invoke(); + await _merge.ResolveConflictAsync(TaskId, SelectedBranch!); break; case "blocked": ErrorMessage = Loc.T("vm.merge.blocked", result.ErrorMessage ?? ""); diff --git a/src/ClaudeDo.Ui/ViewModels/Modals/WorktreesOverviewModalViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Modals/WorktreesOverviewModalViewModel.cs index ffa90df..ca3d110 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/WorktreesOverviewModalViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/WorktreesOverviewModalViewModel.cs @@ -62,6 +62,7 @@ public sealed partial class WorktreesOverviewModalViewModel : ViewModelBase { private readonly IWorkerClient _worker; private readonly Func _diffVmFactory; + private readonly IMergeCoordinator _merge; [ObservableProperty] private string? _listIdFilter; [ObservableProperty] private string _title = "Worktrees"; @@ -79,9 +80,6 @@ public sealed partial class WorktreesOverviewModalViewModel : ViewModelBase public ObservableCollection MergeTargets { get; } = new(); public ObservableCollection ConflictRows { get; } = new(); - /// Inert seam wired by the integrator to Layer C's resolver at merge time. (taskId, targetBranch) - public Func? RequestConflictResolution { get; set; } - public Action? CloseAction { get; set; } public Action? ShowDiffAction { get; set; } public Action? JumpToTaskAction { get; set; } @@ -89,10 +87,11 @@ public sealed partial class WorktreesOverviewModalViewModel : ViewModelBase public Func? ResolveMergeVm { get; set; } public Func? ShowMergeAction { get; set; } - public WorktreesOverviewModalViewModel(IWorkerClient worker, Func diffVmFactory) + public WorktreesOverviewModalViewModel(IWorkerClient worker, Func diffVmFactory, IMergeCoordinator merge) { _worker = worker; _diffVmFactory = diffVmFactory; + _merge = merge; } public void SelectRow(WorktreeOverviewRowViewModel row) @@ -328,7 +327,7 @@ public sealed partial class WorktreesOverviewModalViewModel : ViewModelBase private void ResolveConflict(WorktreeOverviewRowViewModel? row) { if (row is null) return; - RequestConflictResolution?.Invoke(row.TaskId, SelectedTarget ?? ""); + _ = _merge.ResolveConflictAsync(row.TaskId, SelectedTarget ?? ""); } [RelayCommand] diff --git a/src/ClaudeDo.Ui/Views/WindowDialogService.cs b/src/ClaudeDo.Ui/Views/WindowDialogService.cs index 8c4f70d..cf8a5a9 100644 --- a/src/ClaudeDo.Ui/Views/WindowDialogService.cs +++ b/src/ClaudeDo.Ui/Views/WindowDialogService.cs @@ -87,8 +87,6 @@ public sealed class WindowDialogService : IDialogService var mergeDlg = new MergeModalView { DataContext = mergeVm }; await mergeDlg.ShowDialog(_owner); }; - vm.RequestConflictResolution = (taskId, target) => - shell.RequestConflictResolutionAsync(taskId, target); } await dlg.ShowDialog(_owner); } diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandConflictSeamTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandConflictSeamTests.cs index ec494f3..97d50e6 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandConflictSeamTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandConflictSeamTests.cs @@ -64,10 +64,10 @@ public class DetailsIslandConflictSeamTests : IDisposable Task.FromResult(new MergeResultDto("conflict", new[] { "a.cs" }, null)); } - private DetailsIslandViewModel BuildVm(StubWorkerClient worker) + private DetailsIslandViewModel BuildVm(StubWorkerClient worker, IMergeCoordinator merge) { var factory = new TestDbFactory(NewContext); - return new DetailsIslandViewModel(factory, worker, new NullServiceProvider(), new StubNotesApi()); + return new DetailsIslandViewModel(factory, worker, new NullServiceProvider(), new StubNotesApi(), merge); } [Fact] @@ -75,19 +75,22 @@ public class DetailsIslandConflictSeamTests : IDisposable { const string taskId = "task-conflict-1"; - var vm = BuildVm(new ConflictApproveWorkerClient()); - vm.Bind(new TaskRowViewModel { Id = taskId, Status = TaskStatus.WaitingForReview }); - vm.Merge.SelectedMergeTarget = "main"; - string? capturedTaskId = null; string? capturedTarget = null; - vm.Merge.RequestConflictResolution = (tid, target) => + var coordinator = new MergeCoordinator { - capturedTaskId = tid; - capturedTarget = target; - return Task.CompletedTask; + Handler = (tid, target) => + { + capturedTaskId = tid; + capturedTarget = target; + return Task.CompletedTask; + }, }; + var vm = BuildVm(new ConflictApproveWorkerClient(), coordinator); + vm.Bind(new TaskRowViewModel { Id = taskId, Status = TaskStatus.WaitingForReview }); + vm.Merge.SelectedMergeTarget = "main"; + await vm.ApproveReviewCommand.ExecuteAsync(null); Assert.Equal(taskId, capturedTaskId); diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandPlanningTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandPlanningTests.cs index 78efc1c..a5893ad 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandPlanningTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandPlanningTests.cs @@ -66,7 +66,7 @@ public class DetailsIslandPlanningTests : IDisposable private DetailsIslandViewModel BuildVm(StubWorkerClient worker) { var factory = new TestDbFactory(NewContext); - return new DetailsIslandViewModel(factory, worker, new NullServiceProvider(), new StubNotesApi()); + return new DetailsIslandViewModel(factory, worker, new NullServiceProvider(), new StubNotesApi(), new ClaudeDo.Ui.Services.MergeCoordinator()); } // Connected worker whose review calls fail the way the hub does when the task diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandPrepModeTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandPrepModeTests.cs index 388da5b..cc4ea70 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandPrepModeTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandPrepModeTests.cs @@ -51,7 +51,7 @@ public class DetailsIslandPrepModeTests : IDisposable private DetailsIslandViewModel NewDetailsVm(StubWorkerClient stub) { var factory = new TestDbFactory(NewContext); - return new DetailsIslandViewModel(factory, stub, new NullServiceProvider(), new StubNotesApi()); + return new DetailsIslandViewModel(factory, stub, new NullServiceProvider(), new StubNotesApi(), new ClaudeDo.Ui.Services.MergeCoordinator()); } private sealed class NullServiceProvider : IServiceProvider diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs index b224224..ef04cb2 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs @@ -69,7 +69,7 @@ public class DetailsIslandReviewActionsTests : IDisposable private DetailsIslandViewModel BuildVm(StubWorkerClient worker) { var factory = new TestDbFactory(NewContext); - return new DetailsIslandViewModel(factory, worker, new NullServiceProvider(), new StubNotesApi()); + return new DetailsIslandViewModel(factory, worker, new NullServiceProvider(), new StubNotesApi(), new ClaudeDo.Ui.Services.MergeCoordinator()); } [Fact] diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandTabsTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandTabsTests.cs index e6918e8..6648b54 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandTabsTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandTabsTests.cs @@ -57,7 +57,7 @@ public class DetailsIslandTabsTests : IDisposable private DetailsIslandViewModel NewVm() { var factory = new TestDbFactory(NewContext); - return new DetailsIslandViewModel(factory, new DefaultStub(), new NullServiceProvider(), new StubNotesApi()); + return new DetailsIslandViewModel(factory, new DefaultStub(), new NullServiceProvider(), new StubNotesApi(), new ClaudeDo.Ui.Services.MergeCoordinator()); } [Fact] diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/WorktreesOverviewBatchMergeTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/WorktreesOverviewBatchMergeTests.cs index eab3fb6..60eba1f 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/WorktreesOverviewBatchMergeTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/WorktreesOverviewBatchMergeTests.cs @@ -30,8 +30,9 @@ public class WorktreesOverviewBatchMergeTests Assert.False(row.IsConflict); } - private static WorktreesOverviewModalViewModel NewVm() => - new(new ClaudeDo.Ui.Services.WorkerClient("http://127.0.0.1:1/hub"), () => null!); + private static WorktreesOverviewModalViewModel NewVm(IMergeCoordinator? merge = null) => + new(new ClaudeDo.Ui.Services.WorkerClient("http://127.0.0.1:1/hub"), () => null!, + merge ?? new ClaudeDo.Ui.Services.MergeCoordinator()); private static MergeResultDto Merged() => new("merged", System.Array.Empty(), null); private static MergeResultDto Conflict() => new("conflict", new[] { "f.cs" }, null); @@ -137,13 +138,15 @@ public class WorktreesOverviewBatchMergeTests [Fact] public void ResolveConflict_invokes_seam_with_task_and_target() { - var vm = NewVm(); + (string Task, string Target)? captured = null; + var coordinator = new ClaudeDo.Ui.Services.MergeCoordinator + { + Handler = (taskId, target) => { captured = (taskId, target); return System.Threading.Tasks.Task.CompletedTask; }, + }; + var vm = NewVm(coordinator); vm.SelectedTarget = "release"; var row = ActiveRow("x"); row.MergeOutcome = BatchMergeOutcome.Conflict; - (string Task, string Target)? captured = null; - vm.RequestConflictResolution = (taskId, target) => { captured = (taskId, target); return System.Threading.Tasks.Task.CompletedTask; }; - vm.ResolveConflictCommand.Execute(row); Assert.Equal(("x", "release"), captured);