From 79b35801ae4be0596334c8a55977ebc180ce62de Mon Sep 17 00:00:00 2001 From: mika kuns Date: Tue, 11 Aug 2026 16:39:20 +0200 Subject: [PATCH] fix(ui): stop the reconcile ticks from destroying user state The 4s reconcile tick was added to three long-lived surfaces. On two of them it reloads by rebuilding every row instance, which silently threw away whatever the user had built up since the overlay opened; on the third it healed a row's data but left it in the wrong section. - WorktreesOverview: LoadAsync clears Rows, SelectedCount, ConflictRows and BatchProgress, so batch-merge ticks, the highlighted row and a finished batch's outcome badges were wiped every four seconds -- assembling a multi-worktree selection was impossible. Carry that state across the reload, keyed by task id, and re-point SelectedRow at the fresh instance (or clear it when the worktree is gone). - MergeHelperSelection: the remember/restore had no re-entrancy guard, so a second tick entering between the first one's reload and its restore snapshotted rows already back at IsTickedByDefault and wrote that default back, re-ticking what the user had unticked. One tick at a time, and hold the row instances instead of a value snapshot so a tick landed during the await survives. - TasksIsland: the tick deliberately never called Regroup because Phase 2b owned Rows in parallel. 2b has landed, so a healed task that went Done stayed in the Open section under a stale count, and a healed depends-on link never pulled its dependent under the chain head. Regroup when a patch moved a grouping input, gated on a cheap key so an idle tick stays free. --- .../Islands/TasksIslandViewModel.cs | 28 +++- .../MergeHelperSelectionModalViewModel.cs | 54 ++++++-- .../Modals/WorktreesOverviewModalViewModel.cs | 38 +++++- ...MergeHelperSelectionModalViewModelTests.cs | 50 ++++++++ .../TasksIslandReconcileTickTests.cs | 66 +++++++++- .../WorktreesOverviewReconcileTickTests.cs | 120 ++++++++++++++++++ 6 files changed, 335 insertions(+), 21 deletions(-) create mode 100644 tests/ClaudeDo.Ui.Tests/ViewModels/WorktreesOverviewReconcileTickTests.cs diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs index cec83175..bd590100 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs @@ -1420,10 +1420,11 @@ public sealed partial class TasksIslandViewModel : ViewModelBase, IDisposable // ── Phase 3: reconcile tick ────────────────────────────────────────────── // Self-healing safety net for a lost broadcast: every few seconds, diff the flat `Items` - // master collection against SQLite and patch properties in place. Never rebuilds a row, - // never calls Regroup() (Phase 2b owns OverdueItems/OpenItems/CompletedItems/Regroup and is - // rewriting them in parallel), and never falls back to LoadForList — the delta path already - // owns that escalation. + // master collection against SQLite and patch properties in place. Never rebuilds a row and + // never falls back to LoadForList — the delta path already owns that escalation. It DOES + // call Regroup(), but only when a patch actually moved a row's grouping inputs: a healed + // row can change section, order or rail label, and none of that shows until Rows is + // re-emitted. // Above this many rows, only the first N (in Items order) are reconciled per tick, so the // query cost stays bounded instead of growing with an ever-larger list. @@ -1487,6 +1488,7 @@ public sealed partial class TasksIslandViewModel : ViewModelBase, IDisposable // scan per id is 250k comparisons every few seconds, for nothing. var rowById = new Dictionary(Items.Count); foreach (var r in Items) rowById[r.Id] = r; + var groupingChanged = false; foreach (var id in ids) { // Superseded by a fresher delta refresh or a later tick that landed while this one @@ -1494,7 +1496,23 @@ public sealed partial class TasksIslandViewModel : ViewModelBase, IDisposable if (!_deltaSeq.TryGetValue(id, out var current) || current != seqByTaskId[id]) continue; if (!byId.TryGetValue(id, out var entity)) continue; // deleted; the delta path removes rows, not the tick - if (rowById.TryGetValue(id, out var row)) row.UpdateFromEntity(entity); + if (!rowById.TryGetValue(id, out var row)) continue; + var before = GroupingKey(row); + row.UpdateFromEntity(entity); + if (!before.Equals(GroupingKey(row))) groupingChanged = true; } + + // A patched row can have left its section (Done), moved inside it (a new/removed + // depends-on link re-orders the chain), or changed what its rail label reads. Rows only + // reflects any of that after a Regroup — without this the tick would heal the row's data + // while leaving a completed task sitting in the Open section under a stale count. Gated + // on a real change so an idle tick stays free. + if (groupingChanged) Regroup(); } + + // The slice of a row that Regroup reads: which section it lands in, where it sits inside it, + // and what its chain rail/after-chip says. Only fields UpdateFromEntity actually writes — + // IsExpanded and HasPlanningChildren are owned elsewhere and would produce false positives. + private static (bool, DateTime?, string?, string?, PlanningPhase, int, string) GroupingKey(TaskRowViewModel r) => + (r.Done, r.ScheduledFor, r.ParentTaskId, r.DependsOnTaskId, r.PlanningPhase, r.Number, r.Title); } diff --git a/src/ClaudeDo.Ui/ViewModels/Modals/MergeHelperSelectionModalViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Modals/MergeHelperSelectionModalViewModel.cs index 95a06766..15cd1fb6 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/MergeHelperSelectionModalViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/MergeHelperSelectionModalViewModel.cs @@ -45,6 +45,16 @@ public sealed partial class MergeHelperSelectionModalViewModel : ViewModelBase // doesn't cascade over its children's own remembered state. private bool _suppressCascade; + // Only one tick may be in flight. A second one entering while the first awaits LoadAsync + // would snapshot an already-cleared Tasks collection, restore that emptiness, and let + // IsTickedByDefault re-tick every row — silently undoing the user's unticks, i.e. exactly + // what the remember/restore below exists to prevent. + private bool _tickInFlight; + + // Test seam: awaited inside ReconcileTickAsync after the reload and before the restore, so a + // test can hold one tick open in exactly the window a second one would corrupt. Null in prod. + internal Func? ReconcileTickTestBarrier { get; set; } + public ObservableCollection Tasks { get; } = new(); [ObservableProperty] private string _scopeLabel = ""; @@ -81,25 +91,43 @@ public sealed partial class MergeHelperSelectionModalViewModel : ViewModelBase // already existed across the reload; only genuinely new rows get the default. internal async Task ReconcileTickAsync() { - if (string.IsNullOrEmpty(_listId)) return; - var previous = Tasks.ToDictionary(t => t.Id, t => t.IsSelected); - await LoadAsync(); - - // Restore without the parent→children cascade: it would overwrite a child's own remembered - // state with its parent's, undoing exactly the per-child choice being restored. - _suppressCascade = true; + if (string.IsNullOrEmpty(_listId) || _tickInFlight) return; + _tickInFlight = true; try { - foreach (var t in Tasks) - if (previous.TryGetValue(t.Id, out var wasSelected)) - t.IsSelected = wasSelected; + // Hold the row INSTANCES, not a value snapshot, and read their state back only after + // the reload: a tick the user lands during the await happens before LoadAsync clears + // the collection, so the live instance still carries it. Snapshotting up front would + // drop that click. + var previousRows = Tasks.ToList(); + await LoadAsync(); + + if (ReconcileTickTestBarrier is { } barrier) await barrier(); + + var previous = new Dictionary(previousRows.Count); + foreach (var t in previousRows) previous[t.Id] = t.IsSelected; + + // Restore without the parent→children cascade: it would overwrite a child's own + // remembered state with its parent's, undoing exactly the per-child choice being + // restored. + _suppressCascade = true; + try + { + foreach (var t in Tasks) + if (previous.TryGetValue(t.Id, out var wasSelected)) + t.IsSelected = wasSelected; + } + finally + { + _suppressCascade = false; + } + + OnPropertyChanged(nameof(CanConfirm)); } finally { - _suppressCascade = false; + _tickInFlight = false; } - - OnPropertyChanged(nameof(CanConfirm)); } public async Task LoadAsync(CancellationToken ct = default) diff --git a/src/ClaudeDo.Ui/ViewModels/Modals/WorktreesOverviewModalViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Modals/WorktreesOverviewModalViewModel.cs index 1c839acb..672a6edf 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/WorktreesOverviewModalViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/WorktreesOverviewModalViewModel.cs @@ -112,7 +112,43 @@ public sealed partial class WorktreesOverviewModalViewModel : ViewModelBase private readonly System.Timers.Timer _reconcileTimer = new(4_000); - internal Task ReconcileTickAsync() => IsBusy || IsMerging ? Task.CompletedTask : LoadAsync(); + // LoadAsync rebuilds every row instance from scratch and resets SelectedCount/ConflictRows/ + // BatchProgress, so a bare reload on a 4s timer would wipe the user's batch-merge ticks, the + // highlighted row and a finished batch's outcome badges out from under them — assembling a + // multi-worktree selection would be impossible. Carry the state that is the user's (or a + // finished batch's) across the reload, keyed by task id; genuinely new rows come up unticked. + internal async Task ReconcileTickAsync() + { + if (IsBusy || IsMerging) return; + + // Hold the row INSTANCES, not a value snapshot, and read them back only after the reload: + // a tick the user lands during the await happens before LoadAsync clears the collection, + // so the live instance still carries it. + var previousRows = AllRows.ToList(); + var previousProgress = BatchProgress; + + await LoadAsync(); + + var previous = new Dictionary(previousRows.Count); + foreach (var r in previousRows) previous[r.TaskId] = (r.IsChecked, r.MergeOutcome, r.IsSelected); + + WorktreeOverviewRowViewModel? reselect = null; + foreach (var row in AllRows) + { + if (!previous.TryGetValue(row.TaskId, out var state)) continue; + row.IsChecked = state.Checked; + row.MergeOutcome = state.Outcome; + if (state.Outcome == BatchMergeOutcome.Conflict) ConflictRows.Add(row); + if (state.Selected) reselect = row; + } + + // SelectedRow still points at a detached pre-reload instance either way — re-point it at + // the fresh row, or clear it when that worktree is gone rather than leave it dangling. + if (reselect is not null) SelectRow(reselect); + else SelectedRow = null; + + BatchProgress = previousProgress; + } public void SelectRow(WorktreeOverviewRowViewModel row) { diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/MergeHelperSelectionModalViewModelTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/MergeHelperSelectionModalViewModelTests.cs index a8d0e0b5..488b3d83 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/MergeHelperSelectionModalViewModelTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/MergeHelperSelectionModalViewModelTests.cs @@ -350,4 +350,54 @@ public class MergeHelperSelectionModalViewModelTests : IDisposable Assert.False(vm.HasTasks); Assert.False(vm.CanConfirm); } + + // The tick reloads by rebuilding every row, so it remembers and replays the user's ticks. + // A second tick entering while the first is between its reload and its restore would snapshot + // rows that are back at IsTickedByDefault and write that default back — silently re-ticking + // whatever the user had deliberately unticked. Only one tick may be in flight. + [Fact] + public async Task ReconcileTick_ignores_a_second_tick_while_one_is_still_in_flight() + { + await SeedAllStatusesAsync(); + + var factory = new CountingDbFactory(NewContext); + var vm = new MergeHelperSelectionModalViewModel(factory); + vm.Configure("L1", "Work"); + await vm.LoadAsync(); + + var unticked = vm.Tasks.Single(t => t.Id == "t-idle"); + unticked.IsSelected = false; + + var reached = new TaskCompletionSource(); + var proceed = new TaskCompletionSource(); + vm.ReconcileTickTestBarrier = async () => + { + reached.TrySetResult(); + await proceed.Task; + }; + + var first = vm.ReconcileTickAsync(); + await reached.Task; + + var loadsBefore = factory.CreateCalls; + await vm.ReconcileTickAsync(); + Assert.Equal(loadsBefore, factory.CreateCalls); // the overlapping tick did not reload + + proceed.TrySetResult(); + await first; + + Assert.False(vm.Tasks.Single(t => t.Id == "t-idle").IsSelected); + } + + private sealed class CountingDbFactory : IDbContextFactory + { + private readonly Func _create; + public int CreateCalls { get; private set; } + public CountingDbFactory(Func create) => _create = create; + public ClaudeDoDbContext CreateDbContext() + { + CreateCalls++; + return _create(); + } + } } diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandReconcileTickTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandReconcileTickTests.cs index 6b6a8610..5a19aac7 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandReconcileTickTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandReconcileTickTests.cs @@ -7,9 +7,11 @@ using TaskStatus = ClaudeDo.Data.Models.TaskStatus; namespace ClaudeDo.Ui.Tests.ViewModels; // Phase 3 reconcile tick: a periodic, awaitable-for-tests diff of the flat `Items` master -// collection against SQLite that patches properties in place. It must never rebuild a row, -// never call Regroup(), and never fall back to LoadForList — see +// collection against SQLite that patches properties in place. It must never rebuild a row and +// never fall back to LoadForList — see // docs/superpowers/specs/2026-08-07-ui-reaktivitaet-und-listen-performance-design.md. +// It DOES Regroup, but only when a patch changed a grouping input: healing a row's data while +// leaving it in the wrong section would fix nothing the user can see. public class TasksIslandReconcileTickTests : IDisposable { private readonly string _dbPath; @@ -216,4 +218,64 @@ public class TasksIslandReconcileTickTests : IDisposable var lastRow = vm.Items.Single(r => r.Id == $"T{total - 1}"); Assert.Equal(TaskStatus.Queued, lastRow.Status); } + + [Fact] + public async Task Tick_moves_a_healed_done_task_into_the_completed_section() + { + await SeedListAsync(); + await SeedTaskAsync("T1", "Task one", TaskStatus.Idle, 0); + await SeedTaskAsync("T2", "Task two", TaskStatus.Idle, 1); + + var vm = new TasksIslandViewModel(new CountingDbFactory(NewContext), worker: null); + await LoadAndWaitAsync(vm, UserList("L1", "Work"), expectedCount: 2); + vm.IsShowingCompleted = true; + Assert.False(vm.HasCompleted); + + // The TaskUpdated broadcast goes missing: SQLite says Done, the row still reads open. + await using (var db = NewContext()) + { + var t = await db.Tasks.FirstAsync(x => x.Id == "T1"); + t.Status = TaskStatus.Done; + await db.SaveChangesAsync(); + } + + await vm.ReconcileTickAsync(); + + Assert.True(vm.HasCompleted); + + // Rows is the rendered order, so the healed row has to sit *under* the completed header + // (the only one carrying an action command), not still be interleaved with the open rows. + var rows = vm.Rows.ToList(); + var headerIndex = rows.FindIndex(r => r is HeaderRow { HasAction: true }); + var healedIndex = rows.FindIndex(r => r is TaskRowViewModel { Id: "T1" }); + Assert.True(headerIndex >= 0, "completed header was never emitted"); + Assert.True(healedIndex > headerIndex, $"healed row at {healedIndex}, completed header at {headerIndex}"); + } + + [Fact] + public async Task Tick_reorders_the_chain_when_a_depends_on_link_is_healed() + { + await SeedListAsync(); + await SeedTaskAsync("HEAD", "Chain head", TaskStatus.Idle, 0); + await SeedTaskAsync("MID", "Unrelated", TaskStatus.Idle, 1); + await SeedTaskAsync("DEP", "Dependent", TaskStatus.Idle, 2); + + var vm = new TasksIslandViewModel(new CountingDbFactory(NewContext), worker: null); + await LoadAndWaitAsync(vm, UserList("L1", "Work"), expectedCount: 3); + Assert.Equal(new[] { "HEAD", "MID", "DEP" }, vm.Rows.OfType().Select(r => r.Id)); + + await using (var db = NewContext()) + { + var t = await db.Tasks.FirstAsync(x => x.Id == "DEP"); + t.DependsOnTaskId = "HEAD"; + await db.SaveChangesAsync(); + } + + await vm.ReconcileTickAsync(); + + // The dependent is pulled directly under its head and gets the rail, not just the data. + Assert.Equal(new[] { "HEAD", "DEP", "MID" }, vm.Rows.OfType().Select(r => r.Id)); + Assert.True(vm.Items.Single(r => r.Id == "DEP").ShowAsChainMember); + Assert.Equal(1, vm.Items.Single(r => r.Id == "DEP").ChainStep); + } } diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/WorktreesOverviewReconcileTickTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/WorktreesOverviewReconcileTickTests.cs new file mode 100644 index 00000000..9a94a789 --- /dev/null +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/WorktreesOverviewReconcileTickTests.cs @@ -0,0 +1,120 @@ +using ClaudeDo.Data.Models; +using ClaudeDo.Ui.Services; +using ClaudeDo.Ui.ViewModels.Modals; +using Xunit; +using TaskStatus = ClaudeDo.Data.Models.TaskStatus; + +namespace ClaudeDo.Ui.Tests.ViewModels; + +// The overview's 4s reconcile tick goes through LoadAsync, which rebuilds every row instance and +// resets SelectedCount/ConflictRows/BatchProgress. Left bare, that wipes the user's batch-merge +// ticks and the highlighted row every four seconds — assembling a multi-worktree selection would +// be impossible. The tick has to carry that state across the reload. +public class WorktreesOverviewReconcileTickTests +{ + private sealed class FakeWorker : StubWorkerClient + { + public List Worktrees { get; set; } = new(); + public override Task> GetWorktreesOverviewAsync(string? listId) + => Task.FromResult(Worktrees.ToList()); + public override Task GetMergeTargetsAsync(string taskId) + => Task.FromResult(new MergeTargetsDto("main", new[] { "main" })); + } + + private sealed class NoopMergeCoordinator : IMergeCoordinator + { + public Task ResolveConflictAsync(string taskId, string targetBranch) => Task.CompletedTask; + } + + private static WorktreeOverviewDto Wt(string taskId, WorktreeState state = WorktreeState.Active) => + new(taskId, $"Task {taskId}", TaskStatus.WaitingForReview, "L1", "Work", + $@"C:\wt\{taskId}", $"claudedo/{taskId}", "base0", state, "+1 -0", + DateTime.UtcNow, PathExistsOnDisk: true); + + private static async Task<(WorktreesOverviewModalViewModel Vm, FakeWorker Worker)> BuildLoadedAsync() + { + var worker = new FakeWorker { Worktrees = { Wt("a"), Wt("b"), Wt("c") } }; + var vm = new WorktreesOverviewModalViewModel( + worker, () => throw new InvalidOperationException("no diff vm in this test"), new NoopMergeCoordinator()); + vm.Configure("L1", "Work"); + await vm.LoadAsync(); + return (vm, worker); + } + + [Fact] + public async Task Tick_keeps_the_batch_merge_ticks() + { + var (vm, _) = await BuildLoadedAsync(); + + vm.Rows.Single(r => r.TaskId == "a").IsChecked = true; + vm.Rows.Single(r => r.TaskId == "c").IsChecked = true; + Assert.Equal(2, vm.SelectedCount); + + await vm.ReconcileTickAsync(); + + Assert.True(vm.Rows.Single(r => r.TaskId == "a").IsChecked); + Assert.False(vm.Rows.Single(r => r.TaskId == "b").IsChecked); + Assert.True(vm.Rows.Single(r => r.TaskId == "c").IsChecked); + Assert.Equal(2, vm.SelectedCount); + } + + [Fact] + public async Task Tick_re_points_the_selected_row_at_the_fresh_instance() + { + var (vm, _) = await BuildLoadedAsync(); + + vm.SelectRow(vm.Rows.Single(r => r.TaskId == "b")); + + await vm.ReconcileTickAsync(); + + Assert.NotNull(vm.SelectedRow); + Assert.Equal("b", vm.SelectedRow!.TaskId); + // Not the stale pre-reload instance: it must be the row actually in the collection. + Assert.Contains(vm.Rows, r => ReferenceEquals(r, vm.SelectedRow)); + Assert.True(vm.SelectedRow.IsSelected); + } + + [Fact] + public async Task Tick_clears_the_selection_when_that_worktree_is_gone() + { + var (vm, worker) = await BuildLoadedAsync(); + + vm.SelectRow(vm.Rows.Single(r => r.TaskId == "b")); + worker.Worktrees.RemoveAll(w => w.TaskId == "b"); + + await vm.ReconcileTickAsync(); + + Assert.Null(vm.SelectedRow); + } + + [Fact] + public async Task Tick_keeps_a_finished_batch_outcome_and_its_conflict_row() + { + var (vm, _) = await BuildLoadedAsync(); + + vm.Rows.Single(r => r.TaskId == "a").MergeOutcome = BatchMergeOutcome.Merged; + vm.Rows.Single(r => r.TaskId == "b").MergeOutcome = BatchMergeOutcome.Conflict; + + await vm.ReconcileTickAsync(); + + Assert.Equal(BatchMergeOutcome.Merged, vm.Rows.Single(r => r.TaskId == "a").MergeOutcome); + Assert.Equal(BatchMergeOutcome.Conflict, vm.Rows.Single(r => r.TaskId == "b").MergeOutcome); + Assert.Single(vm.ConflictRows); + Assert.Equal("b", vm.ConflictRows[0].TaskId); + } + + [Fact] + public async Task Tick_leaves_a_newly_appeared_worktree_unticked() + { + var (vm, worker) = await BuildLoadedAsync(); + + vm.Rows.Single(r => r.TaskId == "a").IsChecked = true; + worker.Worktrees.Add(Wt("d")); + + await vm.ReconcileTickAsync(); + + Assert.True(vm.Rows.Single(r => r.TaskId == "a").IsChecked); + Assert.False(vm.Rows.Single(r => r.TaskId == "d").IsChecked); + Assert.Equal(1, vm.SelectedCount); + } +}