chore(claude-do): merge [B1] UnifiedDiffParser.Parse vom UI-Thread nehmen (DiffViewe
ClaudeDo-Task: e2003fc8-f029-44d0-a66a-51b57a95583f
This commit is contained in:
@@ -698,6 +698,9 @@
|
|||||||
"merging": "Wird zusammengeführt…",
|
"merging": "Wird zusammengeführt…",
|
||||||
"verifying": "Verify-Kommando der Liste läuft… ({0})"
|
"verifying": "Verify-Kommando der Liste läuft… ({0})"
|
||||||
},
|
},
|
||||||
|
"diff": {
|
||||||
|
"parsing": "Diff wird geparst…"
|
||||||
|
},
|
||||||
"review": {
|
"review": {
|
||||||
"submitting": "Wird zur Prüfung eingereicht…",
|
"submitting": "Wird zur Prüfung eingereicht…",
|
||||||
"rejecting": "Wird abgelehnt…",
|
"rejecting": "Wird abgelehnt…",
|
||||||
|
|||||||
@@ -698,6 +698,9 @@
|
|||||||
"merging": "Merging…",
|
"merging": "Merging…",
|
||||||
"verifying": "Running the list's verify command… ({0})"
|
"verifying": "Running the list's verify command… ({0})"
|
||||||
},
|
},
|
||||||
|
"diff": {
|
||||||
|
"parsing": "Parsing diff…"
|
||||||
|
},
|
||||||
"review": {
|
"review": {
|
||||||
"submitting": "Submitting for review…",
|
"submitting": "Submitting for review…",
|
||||||
"rejecting": "Rejecting…",
|
"rejecting": "Rejecting…",
|
||||||
|
|||||||
@@ -98,6 +98,14 @@ public sealed partial class DiffViewerViewModel : ViewModelBase
|
|||||||
|
|
||||||
public Action? CloseAction { get; set; }
|
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)
|
public DiffViewerViewModel(GitService git, IWorkerClient worker, AppSettings settings)
|
||||||
{
|
{
|
||||||
_git = git;
|
_git = git;
|
||||||
@@ -145,6 +153,16 @@ public sealed partial class DiffViewerViewModel : ViewModelBase
|
|||||||
public Task LoadAsync(CancellationToken ct = default) =>
|
public Task LoadAsync(CancellationToken ct = default) =>
|
||||||
Mode == DiffViewerMode.Planning ? LoadPlanningAsync() : LoadFilesAsync(ct);
|
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<T> ParseOffUiThreadAsync<T>(Func<T> parse, CancellationToken ct = default)
|
||||||
|
{
|
||||||
|
using (ParseOp.Begin(Loc.T("ops.diff.parsing")))
|
||||||
|
return await Task.Run(parse, ct);
|
||||||
|
}
|
||||||
|
|
||||||
private async Task LoadFilesAsync(CancellationToken ct)
|
private async Task LoadFilesAsync(CancellationToken ct)
|
||||||
{
|
{
|
||||||
FileTree.Clear();
|
FileTree.Clear();
|
||||||
@@ -179,8 +197,13 @@ public sealed partial class DiffViewerViewModel : ViewModelBase
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
var files = UnifiedDiffParser.Parse(raw).ToList();
|
var (files, tree) = await ParseOffUiThreadAsync(() =>
|
||||||
foreach (var node in DiffTree.Build(files))
|
{
|
||||||
|
var parsed = UnifiedDiffParser.Parse(raw);
|
||||||
|
return (parsed, DiffTree.Build(parsed));
|
||||||
|
}, ct);
|
||||||
|
|
||||||
|
foreach (var node in tree)
|
||||||
FileTree.Add(node);
|
FileTree.Add(node);
|
||||||
|
|
||||||
SelectedNode = DiffTree.FirstLeaf(FileTree);
|
SelectedNode = DiffTree.FirstLeaf(FileTree);
|
||||||
@@ -254,10 +277,24 @@ public sealed partial class DiffViewerViewModel : ViewModelBase
|
|||||||
|
|
||||||
partial void OnIsCombinedModeChanged(bool value) => ToggleCombinedCommand.Execute(null);
|
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)
|
partial void OnDisplayedDiffChanged(string value)
|
||||||
{
|
{
|
||||||
|
var generation = ++_planningFilesGeneration;
|
||||||
|
_ = ApplyParsedPlanningFilesAsync(value, generation);
|
||||||
|
}
|
||||||
|
|
||||||
|
private async Task ApplyParsedPlanningFilesAsync(string diff, int generation)
|
||||||
|
{
|
||||||
|
var files = await ParseOffUiThreadAsync(() => UnifiedDiffParser.Parse(diff));
|
||||||
|
if (generation != _planningFilesGeneration) return;
|
||||||
|
|
||||||
PlanningFiles.Clear();
|
PlanningFiles.Clear();
|
||||||
foreach (var file in UnifiedDiffParser.Parse(value))
|
foreach (var file in files)
|
||||||
PlanningFiles.Add(file);
|
PlanningFiles.Add(file);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -48,6 +48,7 @@
|
|||||||
ToolTip.Tip="{loc:Tr modals.diff.wrapLines}">
|
ToolTip.Tip="{loc:Tr modals.diff.wrapLines}">
|
||||||
<PathIcon Data="{StaticResource Icon.Wrap}" Width="14" Height="14"/>
|
<PathIcon Data="{StaticResource Icon.Wrap}" Width="14" Height="14"/>
|
||||||
</ToggleButton>
|
</ToggleButton>
|
||||||
|
<ctl:OperationIndicator Status="{Binding ParseOp}"/>
|
||||||
</StackPanel>
|
</StackPanel>
|
||||||
|
|
||||||
<!-- Planning toolbar: combined-mode toggle + warning/loading -->
|
<!-- Planning toolbar: combined-mode toggle + warning/loading -->
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
using System.ComponentModel;
|
||||||
using System.IO;
|
using System.IO;
|
||||||
using ClaudeDo.Localization;
|
using ClaudeDo.Localization;
|
||||||
using ClaudeDo.Ui.Localization;
|
using ClaudeDo.Ui.Localization;
|
||||||
@@ -140,6 +141,115 @@ public class DiffViewerViewModelTests
|
|||||||
Assert.Equal("DIFF-B", vm.DisplayedDiff);
|
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);
|
||||||
|
}
|
||||||
|
|
||||||
|
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]
|
[Fact]
|
||||||
public async Task Planning_ToggleCombined_Success_DisplaysUnifiedDiff()
|
public async Task Planning_ToggleCombined_Success_DisplaysUnifiedDiff()
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user