From 9fc8ed391eff1c670f8845d88b5215fc84609a7c Mon Sep 17 00:00:00 2001 From: Mika Kuns Date: Wed, 12 Aug 2026 13:19:20 +0200 Subject: [PATCH 1/2] fix(ui): offload UnifiedDiffParser.Parse off the UI thread in DiffViewerViewModel Both call sites (LoadFilesAsync, OnDisplayedDiffChanged) ran the parser synchronously on the dispatcher, freezing the window on a large diff. The Files-mode parse+tree-build now runs via Task.Run behind a ParseOp OperationStatus wired to the toolbar; the planning-mode parse is fire-and-forget with a generation counter so a fast second DisplayedDiff change can't write stale PlanningFiles. --- src/ClaudeDo.Localization/locales/de.json | 3 ++ src/ClaudeDo.Localization/locales/en.json | 3 ++ .../ViewModels/Modals/DiffViewerViewModel.cs | 38 +++++++++++++++++-- .../Views/Modals/DiffViewerView.axaml | 1 + .../ViewModels/DiffViewerViewModelTests.cs | 27 +++++++++++++ 5 files changed, 69 insertions(+), 3 deletions(-) diff --git a/src/ClaudeDo.Localization/locales/de.json b/src/ClaudeDo.Localization/locales/de.json index 8901dbed..9d6a4eb6 100644 --- a/src/ClaudeDo.Localization/locales/de.json +++ b/src/ClaudeDo.Localization/locales/de.json @@ -698,6 +698,9 @@ "merging": "Wird zusammengeführt…", "verifying": "Verify-Kommando der Liste läuft… ({0})" }, + "diff": { + "parsing": "Diff wird geparst…" + }, "review": { "submitting": "Wird zur Prüfung eingereicht…", "rejecting": "Wird abgelehnt…", diff --git a/src/ClaudeDo.Localization/locales/en.json b/src/ClaudeDo.Localization/locales/en.json index 1a7d6493..35ce84f3 100644 --- a/src/ClaudeDo.Localization/locales/en.json +++ b/src/ClaudeDo.Localization/locales/en.json @@ -698,6 +698,9 @@ "merging": "Merging…", "verifying": "Running the list's verify command… ({0})" }, + "diff": { + "parsing": "Parsing diff…" + }, "review": { "submitting": "Submitting for review…", "rejecting": "Rejecting…", diff --git a/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs index f8003bc6..dfd0e7e9 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs @@ -98,6 +98,14 @@ public sealed partial class DiffViewerViewModel : ViewModelBase public Action? CloseAction { get; set; } + // Parsing a large diff (UnifiedDiffParser.Parse + DiffTree.Build, both pure CPU work) is + // offloaded to a threadpool thread so it never blocks the UI dispatcher. + public OperationStatus ParseOp { get; } = new(); + + // Guards the fire-and-forget planning parse below against a stale write: a second + // DisplayedDiff change while the first parse is still running must not clobber PlanningFiles. + private int _planningFilesGeneration; + public DiffViewerViewModel(GitService git, IWorkerClient worker, AppSettings settings) { _git = git; @@ -179,8 +187,18 @@ public sealed partial class DiffViewerViewModel : ViewModelBase return; } - var files = UnifiedDiffParser.Parse(raw).ToList(); - foreach (var node in DiffTree.Build(files)) + List files; + List tree; + using (ParseOp.Begin(Loc.T("ops.diff.parsing"))) + { + (files, tree) = await Task.Run(() => + { + var parsed = UnifiedDiffParser.Parse(raw); + return (parsed, DiffTree.Build(parsed)); + }, ct); + } + + foreach (var node in tree) FileTree.Add(node); SelectedNode = DiffTree.FirstLeaf(FileTree); @@ -254,10 +272,24 @@ public sealed partial class DiffViewerViewModel : ViewModelBase partial void OnIsCombinedModeChanged(bool value) => ToggleCombinedCommand.Execute(null); + // DisplayedDiff is set from four places (OnSelectedSubtaskChanged plus three branches of + // ToggleCombinedAsync), and OnSelectedSubtaskChanged is itself a synchronous, non-awaitable + // callback — funneling the offload through this single change handler covers all of them + // without touching every call site. Fire-and-forget is safe because the generation counter + // below discards a parse whose result is no longer the current DisplayedDiff. partial void OnDisplayedDiffChanged(string value) { + var generation = ++_planningFilesGeneration; + _ = ApplyParsedPlanningFilesAsync(value, generation); + } + + private async Task ApplyParsedPlanningFilesAsync(string diff, int generation) + { + var files = await Task.Run(() => UnifiedDiffParser.Parse(diff)); + if (generation != _planningFilesGeneration) return; + PlanningFiles.Clear(); - foreach (var file in UnifiedDiffParser.Parse(value)) + foreach (var file in files) PlanningFiles.Add(file); } diff --git a/src/ClaudeDo.Ui/Views/Modals/DiffViewerView.axaml b/src/ClaudeDo.Ui/Views/Modals/DiffViewerView.axaml index 2788b78a..a6e7e137 100644 --- a/src/ClaudeDo.Ui/Views/Modals/DiffViewerView.axaml +++ b/src/ClaudeDo.Ui/Views/Modals/DiffViewerView.axaml @@ -48,6 +48,7 @@ ToolTip.Tip="{loc:Tr modals.diff.wrapLines}"> + diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DiffViewerViewModelTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DiffViewerViewModelTests.cs index 5c27c6b5..98169c56 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/DiffViewerViewModelTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DiffViewerViewModelTests.cs @@ -140,6 +140,33 @@ public class DiffViewerViewModelTests Assert.Equal("DIFF-B", vm.DisplayedDiff); } + [Fact] + public async Task Planning_SelectSubtask_PopulatesPlanningFilesAsync() + { + const string diff = "diff --git a/foo.cs b/foo.cs\n" + + "--- a/foo.cs\n" + + "+++ b/foo.cs\n" + + "@@ -1,1 +1,1 @@\n" + + "-old\n" + + "+new\n"; + var fake = new FakePlanningWorker + { + AggregateResult = new[] { new SubtaskDiffDto("s1", "First", "b1", "base1", "head1", null, diff) } + }; + var vm = new DiffViewerViewModel(null!, fake, TestSettings()); + vm.ConfigurePlanning("plan-1", "main"); + + await vm.LoadAsync(); + + // OnDisplayedDiffChanged offloads the parse via Task.Run and writes PlanningFiles back + // fire-and-forget, so it may not have landed yet when LoadAsync returns. + var deadline = DateTime.UtcNow.AddSeconds(5); + while (DateTime.UtcNow < deadline && vm.PlanningFiles.Count == 0) await Task.Delay(10); + + var file = Assert.Single(vm.PlanningFiles); + Assert.Equal("foo.cs", file.Path); + } + [Fact] public async Task Planning_ToggleCombined_Success_DisplaysUnifiedDiff() { From f6994f32d5fcb85810d4928d16aed4475b4e4d35 Mon Sep 17 00:00:00 2001 From: Mika Kuns Date: Wed, 12 Aug 2026 13:41:05 +0200 Subject: [PATCH 2/2] fix(ui): show ParseOp indicator on the planning-mode diff parse too ParseOp only covered LoadFilesAsync; OnDisplayedDiffChanged's parse (driven by ToggleCombinedAsync and OnSelectedSubtaskChanged) had no visible feedback even though the offload was already correct. Both call sites now go through one ParseOffUiThreadAsync helper (Begin on the UI thread before Task.Run, End via `using` regardless of a stale generation), so the already-visible toolbar indicator covers the planning path without duplicating the begin/offload/end ceremony. --- .../ViewModels/Modals/DiffViewerViewModel.cs | 25 +++--- .../ViewModels/DiffViewerViewModelTests.cs | 83 +++++++++++++++++++ 2 files changed, 98 insertions(+), 10 deletions(-) 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() {