diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs index dfc74d31..712b2351 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs @@ -570,20 +570,21 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable if (_suppressDescSave || Task is null) return; _descSaveCts?.Cancel(); _descSaveCts = new CancellationTokenSource(); - _ = SaveDescriptionAsync(_descSaveCts.Token); + // Capture the row so a task switch mid-debounce cannot write A's text into B's description. + _ = SaveDescriptionAsync(Task, value, _descSaveCts.Token); } - private async System.Threading.Tasks.Task SaveDescriptionAsync(CancellationToken ct) + private async System.Threading.Tasks.Task SaveDescriptionAsync( + TaskRowViewModel row, string value, CancellationToken ct) { try { await System.Threading.Tasks.Task.Delay(400, ct); - if (Task is null) return; await using var ctx = _dbFactory.CreateDbContext(); var repo = new TaskRepository(ctx); - var entity = await repo.GetByIdAsync(Task.Id); + var entity = await repo.GetByIdAsync(row.Id); if (entity is null) return; - entity.Description = string.IsNullOrWhiteSpace(EditableDescription) ? null : EditableDescription; + entity.Description = string.IsNullOrWhiteSpace(value) ? null : value; await repo.UpdateAsync(entity); } catch (OperationCanceledException) { } @@ -617,6 +618,11 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable _loadCts = new CancellationTokenSource(); var ct = _loadCts.Token; + // A pending debounced title/description save targets the row being unbound — + // let it be, don't have it land on whichever row replaces it. + _titleSaveCts?.Cancel(); + _descSaveCts?.Cancel(); + Task = row; OnPropertyChanged(nameof(TaskIdBadge)); Monitor.Reset(); diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs index 37dcb5ee..414e7bfd 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs @@ -212,7 +212,7 @@ public sealed partial class ListsIslandViewModel : ViewModelBase, IDisposable _worker.TaskFinishedEvent += (_slot, _id, _status, _at) => _ = RefreshCountsAsync(); _worker.TaskUpdatedEvent += _id => _ = RefreshCountsAsync(); _worker.WorktreeUpdatedEvent += _id => _ = RefreshCountsAsync(); - _worker.ConnectionRestoredEvent += () => _ = RefreshCountsAsync(); + _worker.ConnectionRestoredEvent += () => _ = ReloadAfterReconnectAsync(); } _langChangedHandler = (_, _) => RefreshLocalizedLabels(); @@ -295,6 +295,19 @@ public sealed partial class ListsIslandViewModel : ViewModelBase, IDisposable SelectedList = Items.FirstOrDefault(); } + /// + /// A reconnect can have missed offline list creates/renames/deletes (SignalR was down) — unlike + /// the task/worktree events, which only need a count refresh — so this re-reads the full list + /// set instead, restoring the prior selection if it still exists. + /// + private async Task ReloadAfterReconnectAsync() + { + var selectedId = SelectedList?.Id; + await LoadAsync(); + if (selectedId is not null && Items.FirstOrDefault(i => i.Id == selectedId) is { } restored) + SelectedList = restored; + } + public async Task RefreshCountsAsync(CancellationToken ct = default) { var sw = System.Diagnostics.Stopwatch.StartNew(); @@ -438,15 +451,24 @@ public sealed partial class ListsIslandViewModel : ViewModelBase, IDisposable { try { + // The worker broadcasts the raw list id (ListMcpTools/ConfigMcpTools), never the + // "user:" prefixed row id used in this VM's own collections — match on the raw id + // either way so an MCP-driven create/rename is recognized against the loaded row. var rawId = rowId.StartsWith("user:") ? rowId["user:".Length..] : rowId; - var row = UserLists.FirstOrDefault(r => r.Id == rowId); - if (row is null) return; + var row = UserLists.FirstOrDefault(r => + (r.Id.StartsWith("user:") ? r.Id["user:".Length..] : r.Id) == rawId); await using var ctx = await _dbFactory.CreateDbContextAsync(); var lists = new ListRepository(ctx); var entity = await lists.GetByIdAsync(rawId); if (entity is null) return; + if (row is null) + { + await AddRowAsync(lists, entity); + return; + } + row.Name = entity.Name; row.WorkingDir = entity.WorkingDir; row.DefaultCommitType = entity.DefaultCommitType; @@ -455,4 +477,33 @@ public sealed partial class ListsIslandViewModel : ViewModelBase, IDisposable } catch { /* best-effort refresh */ } } + + /// + /// A `create_list` from a running MCP session has no local row yet — build one from the DB + /// entity and insert it at its DB sort position, instead of waiting for the next full reload. + /// + private async Task AddRowAsync(ListRepository lists, ListEntity entity) + { + var dotColors = new[] { "Moss", "Peat", "Sage" }; + var item = new ListNavItemViewModel + { + Id = $"user:{entity.Id}", + Name = entity.Name, + Kind = ListKind.User, + IconKey = "Folder", + DotColorKey = dotColors[UserLists.Count % dotColors.Length], + WorkingDir = entity.WorkingDir, + DefaultCommitType = entity.DefaultCommitType, + IsManual = entity.IsManual, + FindingsTracked = entity.FindingsTracked, + }; + + var seedNames = new HashSet(new[] { "My Day", "Important", "Planned" }); + var ordered = (await lists.GetAllAsync()).Where(l => !seedNames.Contains(l.Name)).ToList(); + var dbIndex = ordered.FindIndex(l => l.Id == entity.Id); + var insertAt = dbIndex < 0 ? UserLists.Count : Math.Min(dbIndex, UserLists.Count); + + UserLists.Insert(insertAt, item); + Items.Insert(SmartLists.Count + insertAt, item); + } } diff --git a/src/ClaudeDo.Ui/ViewModels/Modals/UsageMonitorModalViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Modals/UsageMonitorModalViewModel.cs index 388d2da9..339534a8 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/UsageMonitorModalViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/UsageMonitorModalViewModel.cs @@ -13,6 +13,7 @@ public sealed partial class UsageMonitorModalViewModel : ViewModelBase { private readonly IWorkerClient _worker; private bool _applyingRange; + private bool _isClosed; public UsageMonitorModalViewModel(IWorkerClient worker) => _worker = worker; @@ -78,6 +79,7 @@ public sealed partial class UsageMonitorModalViewModel : ViewModelBase [RelayCommand] private void Close() { + _isClosed = true; _worker.UsageUpdatedEvent -= OnUsageUpdated; CloseAction?.Invoke(); } @@ -95,7 +97,11 @@ public sealed partial class UsageMonitorModalViewModel : ViewModelBase IsBusy = true; try { - Snapshot = await _worker.GetUsageSnapshotAsync(); + var snapshot = await _worker.GetUsageSnapshotAsync(); + // The modal can be closed while the first-load scan is still running (see remarks + // above) — subscribing after that would leak this VM onto the long-lived WorkerClient. + if (_isClosed) return; + Snapshot = snapshot; _worker.UsageUpdatedEvent -= OnUsageUpdated; _worker.UsageUpdatedEvent += OnUsageUpdated; ApplyPresetRange(SelectedPresetDays); diff --git a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs index 0a556936..b2f9a8d5 100644 --- a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs +++ b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs @@ -52,6 +52,7 @@ public abstract class StubWorkerClient : IWorkerClient public void RaiseTaskMessage(string taskId, string line) => TaskMessageEvent?.Invoke(taskId, line); public void RaiseTaskUpdated(string taskId) => TaskUpdatedEvent?.Invoke(taskId); public void RaiseConnectionRestored() => ConnectionRestoredEvent?.Invoke(); + public void RaiseListUpdated(string listId) => ListUpdatedEvent?.Invoke(listId); public void RaiseTaskQuestionAsked(string taskId, string questionId, string question) => TaskQuestionAskedEvent?.Invoke(taskId, questionId, question); public void RaiseTaskQuestionResolved(string taskId, string questionId) => TaskQuestionResolvedEvent?.Invoke(taskId, questionId); public void RaiseHandoffRequested(string taskId, IReadOnlyList survivingTaskIds, string nextPhase = "wait") => HandoffRequestedEvent?.Invoke(taskId, survivingTaskIds, nextPhase); diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandDescriptionSaveRaceTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandDescriptionSaveRaceTests.cs new file mode 100644 index 00000000..da7861e9 --- /dev/null +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandDescriptionSaveRaceTests.cs @@ -0,0 +1,140 @@ +using ClaudeDo.Data; +using ClaudeDo.Data.Models; +using ClaudeDo.Ui.Services; +using ClaudeDo.Ui.ViewModels.Islands; +using Microsoft.EntityFrameworkCore; +using TaskStatus = ClaudeDo.Data.Models.TaskStatus; + +namespace ClaudeDo.Ui.Tests.ViewModels; + +// Polish-Audit 2026-08-20 #1: typing in task A, then binding to task B inside the 400 ms debounce +// window used to write A's text into B's row, because SaveDescriptionAsync/SaveTitleAsync read +// `Task`/`EditableDescription` after the delay instead of capturing them at the call site. +public class DetailsIslandDescriptionSaveRaceTests : IDisposable +{ + private readonly string _dbPath; + + public DetailsIslandDescriptionSaveRaceTests() + { + _dbPath = Path.Combine(Path.GetTempPath(), $"claudedo_desc_race_test_{Guid.NewGuid():N}.db"); + using var ctx = NewContext(); + ctx.Database.EnsureCreated(); + } + + public void Dispose() + { + try { File.Delete(_dbPath); } catch { } + try { File.Delete(_dbPath + "-wal"); } catch { } + try { File.Delete(_dbPath + "-shm"); } catch { } + } + + private ClaudeDoDbContext NewContext() + { + var opts = new DbContextOptionsBuilder() + .UseSqlite($"Data Source={_dbPath}") + .Options; + return new ClaudeDoDbContext(opts); + } + + private sealed class TestDbFactory : IDbContextFactory + { + private readonly Func _create; + public TestDbFactory(Func create) => _create = create; + public ClaudeDoDbContext CreateDbContext() => _create(); + } + + private sealed class NullServiceProvider : IServiceProvider + { + public object? GetService(Type serviceType) => null; + } + + private sealed class StubNotesApi : ClaudeDo.Ui.Services.Interfaces.INotesApi + { + public Task> ListAsync(DateOnly day) => + Task.FromResult(new List()); + public Task AddAsync(DateOnly day, string text) => + Task.FromResult(null); + public Task UpdateAsync(string id, string text) => Task.CompletedTask; + public Task DeleteAsync(string id) => Task.CompletedTask; + } + + private sealed class FakeWorker : StubWorkerClient + { + public override bool IsConnected => true; + } + + private async Task SeedTaskAsync(string id, string title, string? description) + { + await using var db = NewContext(); + if (!await db.Lists.AnyAsync(l => l.Id == "L1")) + db.Lists.Add(new ListEntity { Id = "L1", Name = "Work", CreatedAt = DateTime.UtcNow }); + db.Tasks.Add(new TaskEntity + { + Number = TestTaskNumbers.Next(), + Id = id, ListId = "L1", Title = title, Description = description, + Status = TaskStatus.Idle, CreatedAt = DateTime.UtcNow, + }); + await db.SaveChangesAsync(); + } + + private DetailsIslandViewModel BuildVm() => + new(new TestDbFactory(NewContext), new FakeWorker(), new NullServiceProvider(), new StubNotesApi(), + new ClaudeDo.Ui.Services.MergeCoordinator()); + + private async Task ReadTaskAsync(string id) + { + await using var db = NewContext(); + return await db.Tasks.FirstAsync(t => t.Id == id); + } + + [Fact] + public async Task TypingInA_ThenTaskSwappedUnderneath_StillWritesOntoA_NeverB() + { + // Reproduces the exact split the audit called out: `Bind()` assigns `Task` synchronously, + // while `EditableDescription` is only reset once `BindAsync`'s DB round trip completes — + // so for a window, `Task` already points at B while `EditableDescription` still holds A's + // freshly typed text. Setting `Task` directly (bypassing `Bind()`'s own DB round trip and + // its now-added save cancellation) isolates that exact window deterministically, instead of + // racing the real 400 ms debounce against however long a real bind happens to take. + await SeedTaskAsync("A", "Task A", "original A"); + await SeedTaskAsync("B", "Task B", "original B"); + + var vm = BuildVm(); + vm.Bind(new TaskRowViewModel { Id = "A", Title = "Task A" }); + await Task.Delay(50); + + vm.EditableDescription = "typed into A"; + // Simulate the mid-debounce moment where Task already points at B but EditableDescription + // has not been reset yet — without going through Bind() (which would cancel the pending + // save and dispose this exact race). + vm.Task = new TaskRowViewModel { Id = "B", Title = "Task B" }; + + await Task.Delay(600); // past the debounce window + + var a = await ReadTaskAsync("A"); + var b = await ReadTaskAsync("B"); + Assert.Equal("typed into A", a.Description); + Assert.Equal("original B", b.Description); + } + + [Fact] + public async Task Bind_CancelsPendingTitleAndDescriptionSaves_ForThePreviousTask() + { + await SeedTaskAsync("A", "Task A", "original A"); + await SeedTaskAsync("B", "Task B", "original B"); + + var vm = BuildVm(); + vm.Bind(new TaskRowViewModel { Id = "A", Title = "Task A" }); + await Task.Delay(50); + + vm.EditableTitle = "typed title A"; + vm.EditableDescription = "typed desc A"; + vm.Bind(new TaskRowViewModel { Id = "B", Title = "Task B" }); + + await Task.Delay(600); // past the debounce window + + var a = await ReadTaskAsync("A"); + Assert.Equal("Task A", a.Title); + Assert.Equal("original A", a.Description); + } +} diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/ListsIslandListUpdatedTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/ListsIslandListUpdatedTests.cs new file mode 100644 index 00000000..b3bdef31 --- /dev/null +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/ListsIslandListUpdatedTests.cs @@ -0,0 +1,125 @@ +using ClaudeDo.Data; +using ClaudeDo.Data.Models; +using ClaudeDo.Localization; +using ClaudeDo.Ui.Localization; +using ClaudeDo.Ui.Services; +using ClaudeDo.Ui.ViewModels.Islands; +using Microsoft.EntityFrameworkCore; + +namespace ClaudeDo.Ui.Tests.ViewModels; + +// Polish-Audit 2026-08-20 #3: a list created by a running MCP session broadcasts ListUpdated with +// the raw list id, which RefreshRowAsync had no add-branch for — the list stayed invisible until +// the next app restart. And a reconnect only refreshed counts, missing offline list changes. +public class ListsIslandListUpdatedTests : IDisposable +{ + private readonly string _dbPath; + + public ListsIslandListUpdatedTests() + { + _dbPath = Path.Combine(Path.GetTempPath(), $"claudedo_lists_updated_test_{Guid.NewGuid():N}.db"); + using var ctx = NewContext(); + ctx.Database.EnsureCreated(); + + var dir = AppContext.BaseDirectory; + while (dir is not null && !Directory.Exists(Path.Combine(dir, "src", "ClaudeDo.Localization", "locales"))) + dir = Path.GetDirectoryName(dir); + Loc.Current = new Localizer( + LocaleStore.Load(Path.Combine(dir!, "src", "ClaudeDo.Localization", "locales")), "en"); + } + + public void Dispose() + { + try { File.Delete(_dbPath); } catch { } + try { File.Delete(_dbPath + "-wal"); } catch { } + try { File.Delete(_dbPath + "-shm"); } catch { } + } + + private ClaudeDoDbContext NewContext() + { + var opts = new DbContextOptionsBuilder() + .UseSqlite($"Data Source={_dbPath}") + .Options; + return new ClaudeDoDbContext(opts); + } + + private sealed class TestDbFactory : IDbContextFactory + { + private readonly Func _create; + public TestDbFactory(Func create) => _create = create; + public ClaudeDoDbContext CreateDbContext() => _create(); + } + + private sealed class FakeWorker : StubWorkerClient + { + public override bool IsConnected => true; + } + + private async Task SeedListAsync(string id, string name, int sortOrder) + { + await using var db = NewContext(); + db.Lists.Add(new ListEntity { Id = id, Name = name, SortOrder = sortOrder, CreatedAt = DateTime.UtcNow }); + await db.SaveChangesAsync(); + } + + [Fact] + public async Task ListUpdated_ForAnIdNotYetLoaded_AddsItToUserLists() + { + await SeedListAsync("existing", "Existing", sortOrder: 0); + + var worker = new FakeWorker(); + var vm = new ListsIslandViewModel(new TestDbFactory(NewContext), worker: worker); + await vm.LoadAsync(); + Assert.Single(vm.UserLists); + + // A running MCP session creates the list after our initial load — the worker broadcasts + // the raw db id, never the "user:" prefixed row id. + await SeedListAsync("new-from-mcp", "Created via MCP", sortOrder: 1); + worker.RaiseListUpdated("new-from-mcp"); + await Task.Delay(100); + + Assert.Equal(2, vm.UserLists.Count); + Assert.Contains(vm.UserLists, r => r.Id == "user:new-from-mcp" && r.Name == "Created via MCP"); + // Inserted at its DB sort position (after "existing"), not just appended — same relative + // order as if it had been present since LoadAsync. + Assert.Equal("user:existing", vm.UserLists[0].Id); + Assert.Equal("user:new-from-mcp", vm.UserLists[1].Id); + Assert.Contains(vm.Items, r => r.Id == "user:new-from-mcp"); + } + + [Fact] + public async Task ListUpdated_ForAnIdMissingFromTheDbToo_DoesNothingAndDoesNotThrow() + { + var worker = new FakeWorker(); + var vm = new ListsIslandViewModel(new TestDbFactory(NewContext), worker: worker); + await vm.LoadAsync(); + var countBefore = vm.UserLists.Count; + + worker.RaiseListUpdated("ghost-id"); + await Task.Delay(100); + + Assert.Equal(countBefore, vm.UserLists.Count); + } + + [Fact] + public async Task ConnectionRestored_FullyReloadsLists_AndKeepsSelectionIfStillPresent() + { + await SeedListAsync("keep-me", "Keep Me", sortOrder: 0); + + var worker = new FakeWorker(); + var vm = new ListsIslandViewModel(new TestDbFactory(NewContext), worker: worker); + await vm.LoadAsync(); + var toSelect = vm.UserLists.Single(r => r.Id == "user:keep-me"); + vm.SelectedList = toSelect; + + // A list created while offline — the reconnect must pick it up, unlike a bare count refresh. + await SeedListAsync("came-in-offline", "Came In Offline", sortOrder: 1); + + worker.RaiseConnectionRestored(); + await Task.Delay(150); + + Assert.Contains(vm.UserLists, r => r.Id == "user:came-in-offline"); + Assert.NotNull(vm.SelectedList); + Assert.Equal("user:keep-me", vm.SelectedList!.Id); + } +} diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/UsageMonitorModalViewModelTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/UsageMonitorModalViewModelTests.cs index 7d16b339..699c075a 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/UsageMonitorModalViewModelTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/UsageMonitorModalViewModelTests.cs @@ -155,6 +155,24 @@ public class UsageMonitorModalViewModelTests Assert.False(vm.IsBusy); } + // ── Leak on early close (Polish-Audit 2026-08-20 #2) ──────────────────── + + [Fact] + public async Task LoadAsync_ClosedBeforeSnapshotArrives_NeverSubscribesToUsageUpdated() + { + var worker = new FakeWorker { SnapshotGate = new TaskCompletionSource() }; + var vm = new UsageMonitorModalViewModel(worker); + + var load = vm.LoadAsync(); + vm.CloseCommand.Execute(null); + worker.SnapshotGate.SetResult(Snapshot(new[] { Limit("session") })); + await load; + + worker.RaiseUsageUpdated(Snapshot(new[] { Limit("session"), Limit("weekly_all") })); + + Assert.Null(vm.Snapshot); + } + // ── Manual refresh ────────────────────────────────────────────────────── [Fact]