diff --git a/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs index dfd0e7e9..03e3599c 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs @@ -153,6 +153,16 @@ public sealed partial class DiffViewerViewModel : ViewModelBase public Task LoadAsync(CancellationToken ct = default) => Mode == DiffViewerMode.Planning ? LoadPlanningAsync() : LoadFilesAsync(ct); + // Shared by both parse call sites (LoadFilesAsync, ApplyParsedPlanningFilesAsync) so the + // ParseOp begin/offload/end ceremony isn't duplicated. Begin runs synchronously on the UI + // thread (the caller hasn't awaited anything yet), and the `using` guarantees End runs when + // the offloaded parse finishes — even if the caller's own result turns out to be stale. + private async Task ParseOffUiThreadAsync(Func parse, CancellationToken ct = default) + { + using (ParseOp.Begin(Loc.T("ops.diff.parsing"))) + return await Task.Run(parse, ct); + } + private async Task LoadFilesAsync(CancellationToken ct) { FileTree.Clear(); @@ -187,16 +197,11 @@ public sealed partial class DiffViewerViewModel : ViewModelBase return; } - List files; - List tree; - using (ParseOp.Begin(Loc.T("ops.diff.parsing"))) + var (files, tree) = await ParseOffUiThreadAsync(() => { - (files, tree) = await Task.Run(() => - { - var parsed = UnifiedDiffParser.Parse(raw); - return (parsed, DiffTree.Build(parsed)); - }, ct); - } + var parsed = UnifiedDiffParser.Parse(raw); + return (parsed, DiffTree.Build(parsed)); + }, ct); foreach (var node in tree) FileTree.Add(node); @@ -285,7 +290,7 @@ public sealed partial class DiffViewerViewModel : ViewModelBase private async Task ApplyParsedPlanningFilesAsync(string diff, int generation) { - var files = await Task.Run(() => UnifiedDiffParser.Parse(diff)); + var files = await ParseOffUiThreadAsync(() => UnifiedDiffParser.Parse(diff)); if (generation != _planningFilesGeneration) return; PlanningFiles.Clear(); diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DiffViewerViewModelTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DiffViewerViewModelTests.cs index 98169c56..105b9400 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/DiffViewerViewModelTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DiffViewerViewModelTests.cs @@ -1,3 +1,4 @@ +using System.ComponentModel; using System.IO; using ClaudeDo.Localization; using ClaudeDo.Ui.Localization; @@ -167,6 +168,88 @@ public class DiffViewerViewModelTests Assert.Equal("foo.cs", file.Path); } + private static string DiffFor(string path) => + $"diff --git a/{path} b/{path}\n--- a/{path}\n+++ b/{path}\n@@ -1,1 +1,1 @@\n-old\n+new\n"; + + [Fact] + public async Task Planning_SelectSubtask_RunsAndEndsParseOp() + { + var fake = new FakePlanningWorker + { + AggregateResult = new[] + { + new SubtaskDiffDto("s1", "First", "b1", "base1", "head1", null, DiffFor("a.cs")), + new SubtaskDiffDto("s2", "Second", "b2", "base2", "head2", null, DiffFor("b.cs")), + } + }; + var vm = new DiffViewerViewModel(null!, fake, TestSettings()); + vm.ConfigurePlanning("plan-1", "main"); + await vm.LoadAsync(); + + // Let the auto-selected first subtask's parse settle before starting the assertion. + var settleDeadline = DateTime.UtcNow.AddSeconds(5); + while (DateTime.UtcNow < settleDeadline && vm.ParseOp.IsRunning) await Task.Delay(10); + + // ParseOp.Begin runs synchronously (before the offloaded parse's first await point can + // return), so this handler is guaranteed to observe IsRunning=true regardless of how + // fast the background parse itself completes — no race with the poll loop below. + var sawRunning = false; + vm.ParseOp.PropertyChanged += (_, e) => + { + if (e.PropertyName == nameof(OperationStatus.IsRunning) && vm.ParseOp.IsRunning) + sawRunning = true; + }; + + vm.SelectedSubtask = vm.Subtasks[1]; + + var deadline = DateTime.UtcNow.AddSeconds(5); + while (DateTime.UtcNow < deadline && vm.ParseOp.IsRunning) await Task.Delay(10); + + Assert.True(sawRunning); + Assert.False(vm.ParseOp.IsRunning); + var file = Assert.Single(vm.PlanningFiles); + Assert.Equal("b.cs", file.Path); + } + + [Fact] + public async Task Planning_RapidSubtaskSwitches_ParseOpEndsAndOnlyLatestWins() + { + var fake = new FakePlanningWorker + { + AggregateResult = new[] + { + new SubtaskDiffDto("s1", "First", "b1", "base1", "head1", null, DiffFor("a.cs")), + new SubtaskDiffDto("s2", "Second", "b2", "base2", "head2", null, DiffFor("b.cs")), + } + }; + var vm = new DiffViewerViewModel(null!, fake, TestSettings()); + vm.ConfigurePlanning("plan-1", "main"); + await vm.LoadAsync(); + + var settleDeadline = DateTime.UtcNow.AddSeconds(5); + while (DateTime.UtcNow < settleDeadline && vm.ParseOp.IsRunning) await Task.Delay(10); + + var sawRunning = false; + vm.ParseOp.PropertyChanged += (_, e) => + { + if (e.PropertyName == nameof(OperationStatus.IsRunning) && vm.ParseOp.IsRunning) + sawRunning = true; + }; + + // Two changes fired back-to-back without awaiting in between: the first parse is + // overtaken by the second before it can write PlanningFiles. + vm.SelectedSubtask = vm.Subtasks[1]; + vm.SelectedSubtask = vm.Subtasks[0]; + + var deadline = DateTime.UtcNow.AddSeconds(5); + while (DateTime.UtcNow < deadline && vm.ParseOp.IsRunning) await Task.Delay(10); + + Assert.True(sawRunning); + Assert.False(vm.ParseOp.IsRunning); + var file = Assert.Single(vm.PlanningFiles); + Assert.Equal("a.cs", file.Path); + } + [Fact] public async Task Planning_ToggleCombined_Success_DisplaysUnifiedDiff() {