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.
This commit is contained in:
@@ -236,6 +236,10 @@ public sealed partial class AgentConfigEditorViewModel : ViewModelBase, IDisposa
|
|||||||
return names.Count == 0 ? null : names;
|
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)
|
public async System.Threading.Tasks.Task LoadForListAsync(string listId, CancellationToken ct = default)
|
||||||
{
|
{
|
||||||
_suppressSave = true;
|
_suppressSave = true;
|
||||||
@@ -246,6 +250,7 @@ public sealed partial class AgentConfigEditorViewModel : ViewModelBase, IDisposa
|
|||||||
await LoadGlobalDefaultsAsync();
|
await LoadGlobalDefaultsAsync();
|
||||||
|
|
||||||
var cfg = await _worker.GetListConfigAsync(listId);
|
var cfg = await _worker.GetListConfigAsync(listId);
|
||||||
|
LastLoadedListConfig = cfg;
|
||||||
ApplyConfig(cfg?.Model, cfg?.MaxTurns, cfg?.SystemPrompt, cfg?.AgentPath, cfg?.PermissionMode);
|
ApplyConfig(cfg?.Model, cfg?.MaxTurns, cfg?.SystemPrompt, cfg?.AgentPath, cfg?.PermissionMode);
|
||||||
await ReloadSessionSkillsAsync(cfg?.SessionSkills);
|
await ReloadSessionSkillsAsync(cfg?.SessionSkills);
|
||||||
|
|
||||||
|
|||||||
@@ -58,7 +58,7 @@ public sealed partial class ListsIslandViewModel : ViewModelBase, IDisposable
|
|||||||
if (row is null || Dialogs is null || _services is null) return;
|
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 rawId = row.Id.StartsWith("user:", StringComparison.Ordinal) ? row.Id["user:".Length..] : row.Id;
|
||||||
var vm = _services.GetRequiredService<ListSettingsModalViewModel>();
|
var vm = _services.GetRequiredService<ListSettingsModalViewModel>();
|
||||||
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);
|
await Dialogs.ShowListSettingsAsync(vm);
|
||||||
if (vm.Deleted) await LoadAsync();
|
if (vm.Deleted) await LoadAsync();
|
||||||
else await RefreshRowAsync(row.Id);
|
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)
|
if (Dialogs is not null && _services is not null)
|
||||||
{
|
{
|
||||||
var vm = _services.GetRequiredService<ListSettingsModalViewModel>();
|
var vm = _services.GetRequiredService<ListSettingsModalViewModel>();
|
||||||
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);
|
await Dialogs.ShowListSettingsAsync(vm);
|
||||||
if (!vm.Saved && !vm.Deleted)
|
if (!vm.Saved && !vm.Deleted)
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -56,6 +56,20 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase
|
|||||||
[ObservableProperty] private TicketProjectOption? _selectedTicketProject;
|
[ObservableProperty] private TicketProjectOption? _selectedTicketProject;
|
||||||
public ObservableCollection<TicketProjectOption> TicketProjects { get; } = new();
|
public ObservableCollection<TicketProjectOption> 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<string> CommitTypeOptions { get; } = new(CommitTypeRegistry.Types);
|
public ObservableCollection<string> CommitTypeOptions { get; } = new(CommitTypeRegistry.Types);
|
||||||
|
|
||||||
public const string DefaultIcon = "Folder";
|
public const string DefaultIcon = "Folder";
|
||||||
@@ -91,6 +105,19 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase
|
|||||||
Agent = new AgentConfigEditorViewModel(worker, AgentConfigScope.List);
|
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(
|
public async Task LoadAsync(
|
||||||
string listId,
|
string listId,
|
||||||
string name,
|
string name,
|
||||||
@@ -100,6 +127,10 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase
|
|||||||
bool findingsTracked = false,
|
bool findingsTracked = false,
|
||||||
string? icon = null,
|
string? icon = null,
|
||||||
CancellationToken ct = default)
|
CancellationToken ct = default)
|
||||||
|
{
|
||||||
|
ListConfigDto? cfg;
|
||||||
|
IsLoading = true;
|
||||||
|
try
|
||||||
{
|
{
|
||||||
ListId = listId;
|
ListId = listId;
|
||||||
Name = name;
|
Name = name;
|
||||||
@@ -112,32 +143,54 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase
|
|||||||
DefaultCommitType = string.IsNullOrWhiteSpace(defaultCommitType) ? CommitTypeRegistry.DefaultType : defaultCommitType;
|
DefaultCommitType = string.IsNullOrWhiteSpace(defaultCommitType) ? CommitTypeRegistry.DefaultType : defaultCommitType;
|
||||||
|
|
||||||
await Agent.LoadForListAsync(listId, ct);
|
await Agent.LoadForListAsync(listId, ct);
|
||||||
var cfg = await _worker.GetListConfigAsync(listId);
|
cfg = Agent.LastLoadedListConfig;
|
||||||
VerifyCommand = cfg?.VerifyCommand ?? "";
|
VerifyCommand = cfg?.VerifyCommand ?? "";
|
||||||
PowerUseAllowed = cfg?.PowerUseAllowed ?? false;
|
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
|
var ticketSettings = TicketSettingsTabViewModel.FeatureEnabled
|
||||||
? await _worker.GetTicketSettingsAsync()
|
? await _worker.GetTicketSettingsAsync()
|
||||||
: null;
|
: null;
|
||||||
TicketsAvailable = !string.IsNullOrWhiteSpace(ticketSettings?.ApiBaseUrl);
|
TicketsAvailable = !string.IsNullOrWhiteSpace(ticketSettings?.ApiBaseUrl);
|
||||||
TicketProjects.Clear();
|
if (!TicketsAvailable)
|
||||||
if (TicketsAvailable)
|
|
||||||
{
|
{
|
||||||
|
SelectedTicketProject = null;
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
TicketProjects.Add(new TicketProjectOption(0, Loc.T("vm.listSettings.ticketProjectNone")));
|
TicketProjects.Add(new TicketProjectOption(0, Loc.T("vm.listSettings.ticketProjectNone")));
|
||||||
foreach (var p in await _worker.GetTicketProjectsAsync())
|
foreach (var p in await _worker.GetTicketProjectsAsync())
|
||||||
TicketProjects.Add(new TicketProjectOption(p.Id, $"{p.DepartmentName} / {p.Title}"));
|
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.FirstOrDefault(t => t.Id == id) ?? TicketProjects[0]
|
||||||
: 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()
|
private async Task SaveAsync()
|
||||||
{
|
{
|
||||||
await _worker.UpdateListAsync(new UpdateListDto(
|
await _worker.UpdateListAsync(new UpdateListDto(
|
||||||
@@ -149,10 +202,11 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase
|
|||||||
FindingsTracked,
|
FindingsTracked,
|
||||||
Icon));
|
Icon));
|
||||||
|
|
||||||
// Tri-state: dropdown never shown (no base URL) -> null, leave the stored link alone.
|
// Tri-state: dropdown never shown (no base URL), or ticket projects failed/never loaded
|
||||||
// Dropdown shown -> always an explicit value, 0 for "— none —" to actively clear it.
|
// (e.g. an unreachable ticket system) -> null, leave the stored link alone. Dropdown shown
|
||||||
// Never null here, or an explicit unlink would silently fail to reach the worker.
|
// and loaded -> always an explicit value, 0 for "— none —" to actively clear it. Never
|
||||||
var ticketProjectId = TicketsAvailable ? (SelectedTicketProject?.Id ?? 0) : (int?)null;
|
// 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
|
// 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".
|
// the worker as a real false rather than a "leave it alone".
|
||||||
@@ -165,7 +219,7 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase
|
|||||||
CloseAction?.Invoke();
|
CloseAction?.Invoke();
|
||||||
}
|
}
|
||||||
|
|
||||||
[RelayCommand]
|
[RelayCommand(CanExecute = nameof(CanDelete))]
|
||||||
private async Task DeleteAsync()
|
private async Task DeleteAsync()
|
||||||
{
|
{
|
||||||
var displayName = string.IsNullOrWhiteSpace(Name) ? Loc.T("vm.listSettings.untitled") : Name;
|
var displayName = string.IsNullOrWhiteSpace(Name) ? Loc.T("vm.listSettings.untitled") : Name;
|
||||||
|
|||||||
@@ -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<ClaudeDoDbContext> NewFactory()
|
||||||
|
{
|
||||||
|
var dbPath = Path.Combine(Path.GetTempPath(), $"claudedo_listsettings_{Guid.NewGuid():N}.db");
|
||||||
|
ClaudeDoDbContext New() => new(new DbContextOptionsBuilder<ClaudeDoDbContext>()
|
||||||
|
.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<Task<List<TicketProjectDto>>>? TicketProjectsFactory;
|
||||||
|
public UpdateListConfigDto? SavedListConfig;
|
||||||
|
|
||||||
|
public override Task<ListConfigDto?> GetListConfigAsync(string listId)
|
||||||
|
{
|
||||||
|
GetListConfigCalls++;
|
||||||
|
return Task.FromResult(Cfg);
|
||||||
|
}
|
||||||
|
|
||||||
|
public override Task<TicketSettingsDto?> GetTicketSettingsAsync() => Task.FromResult(TicketSettings);
|
||||||
|
|
||||||
|
public override Task<List<TicketProjectDto>> GetTicketProjectsAsync() =>
|
||||||
|
TicketProjectsFactory?.Invoke() ?? Task.FromResult(new List<TicketProjectDto>());
|
||||||
|
|
||||||
|
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<List<TicketProjectDto>>();
|
||||||
|
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<TicketProjectDto>
|
||||||
|
{
|
||||||
|
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);
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user