From f24654dfb84487c9786da016ac922b33bbdc57e0 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Wed, 2 Sep 2026 22:18:39 +0200 Subject: [PATCH] perf(ui): show list settings modal before its seven-round-trip load finishes OpenListSettingsAsync/CreateListAsync used to await LoadAsync (name/agent config/list_config x2/ticket settings/ticket projects, all serial) before showing the window -- an unreachable ticket system alone could add a 10s HTTP timeout to every open. LoadAsync now runs via BeginLoad (fire-and-forget, mirrors UsageMonitorModalViewModel), the modal shows immediately and fields fill in behind it. GetListConfigAsync no longer runs twice per open -- AgentConfigEditorViewModel exposes the row it already fetched (LastLoadedListConfig) instead of the caller re-fetching it. Ticket-project loading is split into its own fire-and-forget step so a slow/unreachable ticket system can't block the rest of the fields, and its tri-state Save logic now also treats "never finished loading" like "not available" -- leaving the stored TicketProjectId link untouched instead of overwriting it with 0. Save/Delete are gated (CanExecute) on the core load being done, so a save can't land before the fields it writes are populated. Not measured with OperationTiming against a live worker in this session (no running app instance available) -- verified instead via 4 new unit tests that assert GetListConfigAsync fires exactly once and that a hung/failing ticket-projects call neither blocks LoadAsync's other fields nor corrupts the saved TicketProjectId. Before/after OperationTiming numbers and a visual pass are still open follow-ups. --- .../Agent/AgentConfigEditorViewModel.cs | 5 + .../Islands/ListsIslandViewModel.cs | 4 +- .../Modals/ListSettingsModalViewModel.cs | 114 ++++++++++++---- .../ListSettingsModalViewModelTests.cs | 128 ++++++++++++++++++ 4 files changed, 219 insertions(+), 32 deletions(-) create mode 100644 tests/ClaudeDo.Ui.Tests/ViewModels/ListSettingsModalViewModelTests.cs diff --git a/src/ClaudeDo.Ui/ViewModels/Agent/AgentConfigEditorViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Agent/AgentConfigEditorViewModel.cs index b8c6bb20..952ee7c0 100644 --- a/src/ClaudeDo.Ui/ViewModels/Agent/AgentConfigEditorViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Agent/AgentConfigEditorViewModel.cs @@ -236,6 +236,10 @@ public sealed partial class AgentConfigEditorViewModel : ViewModelBase, IDisposa return names.Count == 0 ? null : names; } + // The list_config row fetched by the most recent LoadForListAsync — exposed so a caller that + // needs the same row (ListSettingsModalViewModel) can reuse it instead of fetching it again. + public ListConfigDto? LastLoadedListConfig { get; private set; } + public async System.Threading.Tasks.Task LoadForListAsync(string listId, CancellationToken ct = default) { _suppressSave = true; @@ -246,6 +250,7 @@ public sealed partial class AgentConfigEditorViewModel : ViewModelBase, IDisposa await LoadGlobalDefaultsAsync(); var cfg = await _worker.GetListConfigAsync(listId); + LastLoadedListConfig = cfg; ApplyConfig(cfg?.Model, cfg?.MaxTurns, cfg?.SystemPrompt, cfg?.AgentPath, cfg?.PermissionMode); await ReloadSessionSkillsAsync(cfg?.SessionSkills); diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs index e6aea290..2a89e84d 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs @@ -58,7 +58,7 @@ public sealed partial class ListsIslandViewModel : ViewModelBase, IDisposable if (row is null || Dialogs is null || _services is null) return; var rawId = row.Id.StartsWith("user:", StringComparison.Ordinal) ? row.Id["user:".Length..] : row.Id; var vm = _services.GetRequiredService(); - await vm.LoadAsync(rawId, row.Name, row.WorkingDir, row.DefaultCommitType, row.IsManual, row.FindingsTracked, row.IconKey); + vm.BeginLoad(rawId, row.Name, row.WorkingDir, row.DefaultCommitType, row.IsManual, row.FindingsTracked, row.IconKey); await Dialogs.ShowListSettingsAsync(vm); if (vm.Deleted) await LoadAsync(); else await RefreshRowAsync(row.Id); @@ -408,7 +408,7 @@ public sealed partial class ListsIslandViewModel : ViewModelBase, IDisposable if (Dialogs is not null && _services is not null) { var vm = _services.GetRequiredService(); - await vm.LoadAsync(entity.Id, entity.Name, entity.WorkingDir, entity.DefaultCommitType, entity.IsManual, entity.FindingsTracked, entity.Icon); + vm.BeginLoad(entity.Id, entity.Name, entity.WorkingDir, entity.DefaultCommitType, entity.IsManual, entity.FindingsTracked, entity.Icon); await Dialogs.ShowListSettingsAsync(vm); if (!vm.Saved && !vm.Deleted) { diff --git a/src/ClaudeDo.Ui/ViewModels/Modals/ListSettingsModalViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Modals/ListSettingsModalViewModel.cs index 29824cf6..c0065005 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/ListSettingsModalViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/ListSettingsModalViewModel.cs @@ -56,6 +56,20 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase [ObservableProperty] private TicketProjectOption? _selectedTicketProject; public ObservableCollection TicketProjects { get; } = new(); + // Gates Save/Delete while the core fields (name/agent/verify command) are still loading, so a + // save can't land before they're populated and clobber the list_config row with blanks. Ticket + // projects load separately (see LoadTicketProjectsAsync) and are NOT covered by this — Save's + // own tri-state handles that case instead. + [ObservableProperty] + [NotifyCanExecuteChangedFor(nameof(SaveCommand))] + [NotifyCanExecuteChangedFor(nameof(DeleteCommand))] + private bool _isLoading; + + // True once GetTicketProjectsAsync has completed successfully for the current load — Save + // only writes an explicit TicketProjectId when this is true, so a slow/unreachable ticket + // system leaves the stored link untouched instead of wiping it to "none". + private bool _ticketProjectsLoaded; + public ObservableCollection CommitTypeOptions { get; } = new(CommitTypeRegistry.Types); public const string DefaultIcon = "Folder"; @@ -91,6 +105,19 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase Agent = new AgentConfigEditorViewModel(worker, AgentConfigScope.List); } + // Fires the load without blocking the caller, so the modal window can be shown right away + // instead of appearing only after seven serial hub round-trips (one of them a 10s-timeout + // ticket-system HTTP call) have finished — same pattern as UsageMonitorModalViewModel.BeginLoad. + public void BeginLoad( + string listId, + string name, + string? workingDir, + string defaultCommitType, + bool isManual = false, + bool findingsTracked = false, + string? icon = null) + => _ = LoadAsync(listId, name, workingDir, defaultCommitType, isManual, findingsTracked, icon); + public async Task LoadAsync( string listId, string name, @@ -101,43 +128,69 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase string? icon = null, CancellationToken ct = default) { - ListId = listId; - Name = name; - IsManual = isManual; - FindingsTracked = findingsTracked; - // An unknown/legacy key would leave the picker with no selection and silently reset the - // icon on the next save — fall back to the default instead. - Icon = IconOptions.Contains(icon) ? icon! : DefaultIcon; - WorkingDir = workingDir ?? ""; - DefaultCommitType = string.IsNullOrWhiteSpace(defaultCommitType) ? CommitTypeRegistry.DefaultType : defaultCommitType; - - await Agent.LoadForListAsync(listId, ct); - var cfg = await _worker.GetListConfigAsync(listId); - VerifyCommand = cfg?.VerifyCommand ?? ""; - PowerUseAllowed = cfg?.PowerUseAllowed ?? false; - - var ticketSettings = TicketSettingsTabViewModel.FeatureEnabled - ? await _worker.GetTicketSettingsAsync() - : null; - TicketsAvailable = !string.IsNullOrWhiteSpace(ticketSettings?.ApiBaseUrl); - TicketProjects.Clear(); - if (TicketsAvailable) + ListConfigDto? cfg; + IsLoading = true; + try { + ListId = listId; + Name = name; + IsManual = isManual; + FindingsTracked = findingsTracked; + // An unknown/legacy key would leave the picker with no selection and silently reset the + // icon on the next save — fall back to the default instead. + Icon = IconOptions.Contains(icon) ? icon! : DefaultIcon; + WorkingDir = workingDir ?? ""; + DefaultCommitType = string.IsNullOrWhiteSpace(defaultCommitType) ? CommitTypeRegistry.DefaultType : defaultCommitType; + + await Agent.LoadForListAsync(listId, ct); + cfg = Agent.LastLoadedListConfig; + VerifyCommand = cfg?.VerifyCommand ?? ""; + PowerUseAllowed = cfg?.PowerUseAllowed ?? false; + } + finally { IsLoading = false; } + + // Deliberately NOT awaited: an unreachable ticket system (10s HTTP timeout) must not hold + // up the rest of the modal. The dropdown just stays empty until this finishes. + _ = LoadTicketProjectsAsync(cfg?.TicketProjectId); + } + + private async Task LoadTicketProjectsAsync(int? existingTicketProjectId) + { + _ticketProjectsLoaded = false; + TicketProjects.Clear(); + try + { + var ticketSettings = TicketSettingsTabViewModel.FeatureEnabled + ? await _worker.GetTicketSettingsAsync() + : null; + TicketsAvailable = !string.IsNullOrWhiteSpace(ticketSettings?.ApiBaseUrl); + if (!TicketsAvailable) + { + SelectedTicketProject = null; + return; + } + TicketProjects.Add(new TicketProjectOption(0, Loc.T("vm.listSettings.ticketProjectNone"))); foreach (var p in await _worker.GetTicketProjectsAsync()) TicketProjects.Add(new TicketProjectOption(p.Id, $"{p.DepartmentName} / {p.Title}")); - SelectedTicketProject = cfg?.TicketProjectId is int id + SelectedTicketProject = existingTicketProjectId is int id ? TicketProjects.FirstOrDefault(t => t.Id == id) ?? TicketProjects[0] : TicketProjects[0]; + _ticketProjectsLoaded = true; } - else + catch (Exception) { - SelectedTicketProject = null; + // Ticket system unreachable/slow — leave the dropdown as-is. Save's tri-state (below) + // skips writing TicketProjectId when _ticketProjectsLoaded is false, so the stored + // link survives instead of being wiped to "none". } } - [RelayCommand] + private bool CanSave() => !IsLoading; + private bool CanDelete() => !IsLoading; + + [RelayCommand(CanExecute = nameof(CanSave))] private async Task SaveAsync() { await _worker.UpdateListAsync(new UpdateListDto( @@ -149,10 +202,11 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase FindingsTracked, Icon)); - // Tri-state: dropdown never shown (no base URL) -> null, leave the stored link alone. - // Dropdown shown -> always an explicit value, 0 for "— none —" to actively clear it. - // Never null here, or an explicit unlink would silently fail to reach the worker. - var ticketProjectId = TicketsAvailable ? (SelectedTicketProject?.Id ?? 0) : (int?)null; + // Tri-state: dropdown never shown (no base URL), or ticket projects failed/never loaded + // (e.g. an unreachable ticket system) -> null, leave the stored link alone. Dropdown shown + // and loaded -> always an explicit value, 0 for "— none —" to actively clear it. Never + // null in that case, or an explicit unlink would silently fail to reach the worker. + var ticketProjectId = TicketsAvailable && _ticketProjectsLoaded ? (SelectedTicketProject?.Id ?? 0) : (int?)null; // Always explicit (never null) — this modal owns the checkbox, so unchecking it has to reach // the worker as a real false rather than a "leave it alone". @@ -165,7 +219,7 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase CloseAction?.Invoke(); } - [RelayCommand] + [RelayCommand(CanExecute = nameof(CanDelete))] private async Task DeleteAsync() { var displayName = string.IsNullOrWhiteSpace(Name) ? Loc.T("vm.listSettings.untitled") : Name; diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/ListSettingsModalViewModelTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/ListSettingsModalViewModelTests.cs new file mode 100644 index 00000000..0ecd052f --- /dev/null +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/ListSettingsModalViewModelTests.cs @@ -0,0 +1,128 @@ +using ClaudeDo.Data; +using ClaudeDo.Localization; +using ClaudeDo.Ui.Localization; +using ClaudeDo.Ui.ViewModels.Modals; +using Microsoft.EntityFrameworkCore; + +namespace ClaudeDo.Ui.Tests.ViewModels; + +// Perf fix: the modal used to await seven serial hub round-trips (including a 10s-timeout +// ticket-system HTTP call) before showing itself. LoadAsync must fetch the list config exactly +// once and must never let a slow/failing ticket-projects call block or corrupt the rest of the +// load, or the save that follows it. +public class ListSettingsModalViewModelTests +{ + static ListSettingsModalViewModelTests() + { + 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"); + } + + private static IDbContextFactory NewFactory() + { + var dbPath = Path.Combine(Path.GetTempPath(), $"claudedo_listsettings_{Guid.NewGuid():N}.db"); + ClaudeDoDbContext New() => new(new DbContextOptionsBuilder() + .UseSqlite($"Data Source={dbPath}").Options); + using (var ctx = New()) ctx.Database.EnsureCreated(); + return new TestDbFactory(New); + } + + private sealed class CountingWorker : StubWorkerClient + { + public int GetListConfigCalls; + public ListConfigDto? Cfg; + public TicketSettingsDto? TicketSettings = new("http://ticket.example", true); + public Func>>? TicketProjectsFactory; + public UpdateListConfigDto? SavedListConfig; + + public override Task GetListConfigAsync(string listId) + { + GetListConfigCalls++; + return Task.FromResult(Cfg); + } + + public override Task GetTicketSettingsAsync() => Task.FromResult(TicketSettings); + + public override Task> GetTicketProjectsAsync() => + TicketProjectsFactory?.Invoke() ?? Task.FromResult(new List()); + + public override Task UpdateListConfigAsync(UpdateListConfigDto dto) + { + SavedListConfig = dto; + return Task.CompletedTask; + } + } + + [Fact] + public async Task LoadAsync_CallsGetListConfig_ExactlyOnce() + { + var worker = new CountingWorker { Cfg = new ListConfigDto("opus", null, null) }; + var vm = new ListSettingsModalViewModel(worker, NewFactory()); + + await vm.LoadAsync("l1", "List One", "/repo", "feat"); + + Assert.Equal(1, worker.GetListConfigCalls); + } + + [Fact] + public async Task LoadAsync_TicketProjectsNeverResolving_DoesNotBlockRestOfLoad() + { + var pending = new TaskCompletionSource>(); + var worker = new CountingWorker + { + Cfg = new ListConfigDto("opus", null, null, 42, null, "make test", "acceptEdits", 7, true), + TicketProjectsFactory = () => pending.Task, + }; + var vm = new ListSettingsModalViewModel(worker, NewFactory()); + + // LoadAsync must complete on its own even though the ticket-projects call it kicked off + // never resolves -- ticket loading is fire-and-forget precisely so a slow/unreachable + // ticket system can't hold up the rest of the modal. + var loadTask = vm.LoadAsync("l1", "List One", "/repo", "feat"); + var completed = await Task.WhenAny(loadTask, Task.Delay(TimeSpan.FromSeconds(2))); + + Assert.Same(loadTask, completed); + Assert.Equal("make test", vm.VerifyCommand); + Assert.True(vm.PowerUseAllowed); + Assert.False(vm.IsLoading); + } + + [Fact] + public async Task Save_WhileTicketProjectsUnreachable_PreservesStoredTicketLink() + { + var worker = new CountingWorker + { + Cfg = new ListConfigDto("opus", null, null, null, null, null, null, 7, false), + TicketProjectsFactory = () => throw new InvalidOperationException("ticket system unreachable"), + }; + var vm = new ListSettingsModalViewModel(worker, NewFactory()); + + await vm.LoadAsync("l1", "List One", "/repo", "feat"); + await vm.SaveCommand.ExecuteAsync(null); + + Assert.NotNull(worker.SavedListConfig); + Assert.Null(worker.SavedListConfig!.TicketProjectId); + } + + [Fact] + public async Task Save_AfterSuccessfulTicketLoad_WritesSelectedTicketProject() + { + var worker = new CountingWorker + { + Cfg = new ListConfigDto("opus", null, null, null, null, null, null, 7, false), + TicketProjectsFactory = () => Task.FromResult(new List + { + new(7, "Fix the thing", "Dev"), + }), + }; + var vm = new ListSettingsModalViewModel(worker, NewFactory()); + + await vm.LoadAsync("l1", "List One", "/repo", "feat"); + await vm.SaveCommand.ExecuteAsync(null); + + Assert.Equal(7, worker.SavedListConfig!.TicketProjectId); + } +}