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.
This commit is contained in:
@@ -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<Task>? ReconcileTickTestBarrier { get; set; }
|
||||
|
||||
public ObservableCollection<MergeHelperTaskRowViewModel> 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<string, bool>(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)
|
||||
|
||||
@@ -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<string, (bool Checked, BatchMergeOutcome Outcome, bool Selected)>(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)
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user