From a1b20d9f277ef9c982611fc07725c0b271c4806c Mon Sep 17 00:00:00 2001 From: mika kuns Date: Wed, 2 Sep 2026 22:33:48 +0200 Subject: [PATCH] fix(ui): report and gate ListSettingsModal load failures instead of swallowing them BeginLoad's fire-and-forget LoadAsync now catches instead of leaving an unobserved task exception, reports it via the new ErrorReported event (wired into FlashFooterError like the other modal VMs), and keeps Save locked so the failure can't clobber list_config with blank fields. LoadTicketProjectsAsync's empty catch now reports too; behavior for the dropdown itself is unchanged. --- src/ClaudeDo.Localization/locales/de.json | 2 +- src/ClaudeDo.Localization/locales/en.json | 2 +- .../Islands/ListsIslandViewModel.cs | 2 + .../Modals/ListSettingsModalViewModel.cs | 26 +++++++++-- .../ListSettingsModalViewModelTests.cs | 44 +++++++++++++++++++ 5 files changed, 71 insertions(+), 5 deletions(-) diff --git a/src/ClaudeDo.Localization/locales/de.json b/src/ClaudeDo.Localization/locales/de.json index 83462c7d..1246018e 100644 --- a/src/ClaudeDo.Localization/locales/de.json +++ b/src/ClaudeDo.Localization/locales/de.json @@ -772,7 +772,7 @@ "sessionSkillsTab": { "installed": "Installiert: {0}", "installFailed": "Installation fehlgeschlagen: {0}", "updated": "Aktualisiert.", "updateFailed": "Aktualisierung fehlgeschlagen: {0}", "removed": "Entfernt.", "removeFailed": "Entfernen fehlgeschlagen: {0}" }, "worktreesTab": { "workerOffline": "Worker offline.", "removed": "{0} Worktree(s) entfernt.", "blocked": "Zwangsentfernung nicht möglich: {0} Aufgabe(n) laufen noch. Brich sie zuerst ab.", "removedFrom": "{0} Worktree(s) von {1} Aufgabe(n) entfernt.", "cleanupFailed": "Aufräumen fehlgeschlagen: {0}", "resetFailed": "Zurücksetzen fehlgeschlagen: {0}" }, "worktreesOverview": { "titleAll": "Worktrees", "titleList": "Worktrees — {0}", "listFallback": "Liste", "cleanupFailed": "Aufräumen fehlgeschlagen.", "cleanupFailedDetailed": "Aufräumen fehlgeschlagen: {0}", "removed": "{0} Worktree(s) entfernt.", "discardConfirm": "Worktree für \"{0}\" verwerfen? Nicht committete Arbeit geht verloren.", "discardFailed": "Worktree konnte nicht verworfen werden.", "keepFailed": "Worktree konnte nicht behalten werden.", "cannotForceRunning": "Eine laufende Aufgabe kann nicht zwangsweise entfernt werden.", "forceRemoveFailed": "Zwangsentfernung fehlgeschlagen.", "forceRemoveFailedDetailed": "Zwangsentfernung fehlgeschlagen: {0}", "batchDone": "{0} gemergt, {1} zu lösen.", "openInExplorerFailed": "Pfad konnte nicht geöffnet werden: {0}" }, - "listSettings": { "untitled": "Unbenannt", "ticketProjectNone": "— keins —" }, + "listSettings": { "untitled": "Unbenannt", "ticketProjectNone": "— keins —", "loadFailed": "Listeneinstellungen konnten nicht geladen werden: {0}", "ticketProjectsLoadFailed": "Ticket-Projekte konnten nicht geladen werden: {0}" }, "detailsIsland": { "verifyFailed": "Merge ist erfolgt, aber das Verifikationskommando der Liste ist fehlgeschlagen — die Aufgabe wurde nicht auf 'Erledigt' gesetzt.", "untrackedCollision": "Merge abgelehnt — er würde eine unversionierte Datei im Ziel-Arbeitsverzeichnis überschreiben.", "stopOffline": "Worker offline — Task kann nicht gestoppt werden.", "stopFailed": "Stoppen fehlgeschlagen: {0}", "enqueueFailed": "In die Warteschlange stellen fehlgeschlagen: {0}", "dequeueFailed": "Aus der Warteschlange entfernen fehlgeschlagen: {0}", "resetAndRetryFailed": "Zurücksetzen & erneut versuchen fehlgeschlagen: {0}", "toggleDoneFailed": "Erledigt-Status konnte nicht aktualisiert werden: {0}" }, "mergeSection": { "openWorktreeFailed": "Worktree konnte nicht geöffnet werden: {0}" }, "about": { "openPathFailed": "Pfad konnte nicht geöffnet werden: {0}" }, diff --git a/src/ClaudeDo.Localization/locales/en.json b/src/ClaudeDo.Localization/locales/en.json index 866828da..bbd8f170 100644 --- a/src/ClaudeDo.Localization/locales/en.json +++ b/src/ClaudeDo.Localization/locales/en.json @@ -772,7 +772,7 @@ "sessionSkillsTab": { "installed": "Installed: {0}", "installFailed": "Install failed: {0}", "updated": "Updated.", "updateFailed": "Update failed: {0}", "removed": "Removed.", "removeFailed": "Remove failed: {0}" }, "worktreesTab": { "workerOffline": "Worker offline.", "removed": "Removed {0} worktree(s).", "blocked": "Cannot force-remove: {0} task(s) still running. Cancel them first.", "removedFrom": "Removed {0} worktree(s) from {1} task(s).", "cleanupFailed": "Cleanup failed: {0}", "resetFailed": "Reset failed: {0}" }, "worktreesOverview": { "titleAll": "Worktrees", "titleList": "Worktrees — {0}", "listFallback": "list", "cleanupFailed": "Cleanup failed.", "cleanupFailedDetailed": "Cleanup failed: {0}", "removed": "Removed {0} worktree(s).", "discardConfirm": "Discard the worktree for \"{0}\"? Uncommitted work will be lost.", "discardFailed": "Failed to discard worktree.", "keepFailed": "Failed to keep worktree.", "cannotForceRunning": "Cannot force-remove a running task.", "forceRemoveFailed": "Force remove failed.", "forceRemoveFailedDetailed": "Force remove failed: {0}", "batchDone": "Merged {0}, {1} need resolution.", "openInExplorerFailed": "Couldn't open path: {0}" }, - "listSettings": { "untitled": "Untitled", "ticketProjectNone": "— none —" }, + "listSettings": { "untitled": "Untitled", "ticketProjectNone": "— none —", "loadFailed": "Couldn't load list settings: {0}", "ticketProjectsLoadFailed": "Couldn't load ticket projects: {0}" }, "detailsIsland": { "verifyFailed": "Merge landed, but the list's verify command failed — the task was kept out of Done.", "untrackedCollision": "Merge refused — it would overwrite an untracked file in the target working directory.", "stopOffline": "Worker offline — can't stop the task.", "stopFailed": "Stop failed: {0}", "enqueueFailed": "Queue failed: {0}", "dequeueFailed": "Remove from queue failed: {0}", "resetAndRetryFailed": "Reset & retry failed: {0}", "toggleDoneFailed": "Couldn't update done state: {0}" }, "mergeSection": { "openWorktreeFailed": "Couldn't open worktree: {0}" }, "about": { "openPathFailed": "Couldn't open path: {0}" }, diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs index 2a89e84d..8a7ea8b7 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/ListsIslandViewModel.cs @@ -58,6 +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(); + vm.ErrorReported += msg => ErrorReported?.Invoke(msg); vm.BeginLoad(rawId, row.Name, row.WorkingDir, row.DefaultCommitType, row.IsManual, row.FindingsTracked, row.IconKey); await Dialogs.ShowListSettingsAsync(vm); if (vm.Deleted) await LoadAsync(); @@ -408,6 +409,7 @@ public sealed partial class ListsIslandViewModel : ViewModelBase, IDisposable if (Dialogs is not null && _services is not null) { var vm = _services.GetRequiredService(); + vm.ErrorReported += msg => ErrorReported?.Invoke(msg); 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 c0065005..0406759c 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/ListSettingsModalViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/ListSettingsModalViewModel.cs @@ -34,6 +34,9 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase public Func>? ConfirmAsync { get; set; } public Func? ShowErrorAsync { get; set; } + // Mirrors UsageMonitorModalViewModel.ErrorReported — the caller wires this into FlashFooterError. + public event Action? ErrorReported; + [ObservableProperty] private string _name = ""; [ObservableProperty] private string _workingDir = ""; [ObservableProperty] private string _defaultCommitType = CommitTypeRegistry.DefaultType; @@ -65,6 +68,11 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase [NotifyCanExecuteChangedFor(nameof(DeleteCommand))] private bool _isLoading; + // Set when the core load (name/agent/verify command) threw — keeps Save gated even after + // IsLoading resets to false in LoadAsync's finally, so a failed load can't clobber the + // stored config with blank fields. + private bool _loadFailed; + // 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". @@ -128,8 +136,9 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase string? icon = null, CancellationToken ct = default) { - ListConfigDto? cfg; + ListConfigDto? cfg = null; IsLoading = true; + _loadFailed = false; try { ListId = listId; @@ -147,8 +156,18 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase VerifyCommand = cfg?.VerifyCommand ?? ""; PowerUseAllowed = cfg?.PowerUseAllowed ?? false; } + catch (Exception ex) + { + // BeginLoad fires this without awaiting it — catching here (instead of letting it + // propagate) turns what would be an unobserved task exception, with the modal stuck + // open and every field blank, into a reported error and a locked Save button. + _loadFailed = true; + ErrorReported?.Invoke(Loc.T("vm.listSettings.loadFailed", ex.Message)); + } finally { IsLoading = false; } + if (_loadFailed) return; + // 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); @@ -179,15 +198,16 @@ public sealed partial class ListSettingsModalViewModel : ViewModelBase : TicketProjects[0]; _ticketProjectsLoaded = true; } - catch (Exception) + catch (Exception ex) { // 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". + ErrorReported?.Invoke(Loc.T("vm.listSettings.ticketProjectsLoadFailed", ex.Message)); } } - private bool CanSave() => !IsLoading; + private bool CanSave() => !IsLoading && !_loadFailed; private bool CanDelete() => !IsLoading; [RelayCommand(CanExecute = nameof(CanSave))] diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/ListSettingsModalViewModelTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/ListSettingsModalViewModelTests.cs index 0ecd052f..2178dbcb 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/ListSettingsModalViewModelTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/ListSettingsModalViewModelTests.cs @@ -38,9 +38,12 @@ public class ListSettingsModalViewModelTests public Func>>? TicketProjectsFactory; public UpdateListConfigDto? SavedListConfig; + public Exception? GetListConfigThrows; + public override Task GetListConfigAsync(string listId) { GetListConfigCalls++; + if (GetListConfigThrows is not null) throw GetListConfigThrows; return Task.FromResult(Cfg); } @@ -125,4 +128,45 @@ public class ListSettingsModalViewModelTests Assert.Equal(7, worker.SavedListConfig!.TicketProjectId); } + + [Fact] + public async Task LoadAsync_CoreLoadThrows_BlocksSaveAndDoesNotOverwriteConfig() + { + var worker = new CountingWorker + { + Cfg = new ListConfigDto("opus", null, null), + GetListConfigThrows = new InvalidOperationException("worker offline"), + }; + var vm = new ListSettingsModalViewModel(worker, NewFactory()); + string? reported = null; + vm.ErrorReported += m => reported = m; + + await vm.LoadAsync("l1", "List One", "/repo", "feat"); + + Assert.False(vm.SaveCommand.CanExecute(null)); + Assert.NotNull(reported); + Assert.Contains("worker offline", reported); + Assert.Null(worker.SavedListConfig); + } + + [Fact] + public async Task LoadAsync_TicketProjectsThrow_ReportsErrorButRestOfLoadStillWorks() + { + var worker = new CountingWorker + { + Cfg = new ListConfigDto("opus", null, null, null, null, "make test", null, 7, true), + TicketProjectsFactory = () => throw new InvalidOperationException("ticket system down"), + }; + var vm = new ListSettingsModalViewModel(worker, NewFactory()); + string? reported = null; + vm.ErrorReported += m => reported = m; + + await vm.LoadAsync("l1", "List One", "/repo", "feat"); + + Assert.Equal("make test", vm.VerifyCommand); + Assert.True(vm.PowerUseAllowed); + Assert.True(vm.SaveCommand.CanExecute(null)); + Assert.NotNull(reported); + Assert.Contains("ticket system down", reported); + } }