From a4b81da59715db0e906f0ce9b0f641dc576b318a Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Fri, 2 Oct 2026 13:16:55 +0200 Subject: [PATCH] Desktop: show the right vault's people, leave settings on lock, say when an import fell short, and stop host names being cut to four letters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Choosing a vault while the screen was busy went through a gate that did nothing, so the members panel kept showing the previous vault's people under the new name. The read now starts regardless; the selection generation discards answers for an earlier choice. - Locking or signing out from Settings left the window in settings mode under the lock card, with no title bar to drag, minimise or close, and unlock returned to Settings. Leaving the unlocked state leaves Settings and closes the members panel; Escape closes that panel before leaving Settings. - Both importers reported success when the keychain was busy and skipped the batch, or failed partway. ImportOutcome now carries refused hosts and why a batch stopped, and the screens say so. - Host cards: the sync badge took 70 of 214 pixels from the name ("bastion" drew as "bast…", two k8s workers both as "k8s…"); it now sits on the address line. Tag chips wrap and trim inside the card. Keychain names trim inside their column instead of printing over TYPE, and the Security page's caption wraps inside its card. --- .../Views/HostsScreen.axaml | 40 +++++-- .../Views/KeychainScreen.axaml | 26 +++-- .../Views/MainWindow.axaml.cs | 7 ++ .../Views/SettingsSecurityPage.axaml | 12 +- .../ViewModels/HetznerImportViewModel.cs | 2 +- .../ViewModels/ImportViewModel.cs | 23 +++- .../ViewModels/MainWindowViewModel.cs | 12 ++ .../ViewModels/VaultViewModel.cs | 105 ++++++++++++------ .../ViewModels/VaultsViewModel.cs | 31 +++++- .../ShellFlowTests.cs | 64 +++++++++++ .../VaultSharingTests.cs | 60 ++++++++++ 11 files changed, 314 insertions(+), 68 deletions(-) diff --git a/src/DodoSSH.Client.App/Views/HostsScreen.axaml b/src/DodoSSH.Client.App/Views/HostsScreen.axaml index d91eba9..97bdbb0 100644 --- a/src/DodoSSH.Client.App/Views/HostsScreen.axaml +++ b/src/DodoSSH.Client.App/Views/HostsScreen.axaml @@ -683,7 +683,7 @@ TextTrimming never fires and the name would run on past the card's own border instead of trimming. See the longer note this carried in v4. --> - + @@ -708,17 +708,27 @@ + - + + + + - - + - + @@ -767,12 +782,13 @@ + Foreground="{StaticResource TextFaint}" + TextTrimming="CharacterEllipsis" /> - + - @@ -290,17 +290,23 @@ Margin="8,0,8,10"> - - - + + + - - + VerticalAlignment="Center" Margin="0,0,10,0" /> + + + Foreground="{StaticResource TextGhost}" VerticalAlignment="Center" + TextTrimming="CharacterEllipsis" /> diff --git a/src/DodoSSH.Client.App/Views/MainWindow.axaml.cs b/src/DodoSSH.Client.App/Views/MainWindow.axaml.cs index e802611..cdc73c1 100644 --- a/src/DodoSSH.Client.App/Views/MainWindow.axaml.cs +++ b/src/DodoSSH.Client.App/Views/MainWindow.axaml.cs @@ -244,6 +244,13 @@ internal sealed partial class MainWindow : Window viewModel.CloseImportCommand.Execute(null); e.Handled = true; } + // The members dialog on Settings → Vaults, by the same rule: it is drawn over the page, so it goes + // first and settings stays. + else if (e.Key == Key.Escape && viewModel.IsSettingsMode && viewModel.Vaults.IsMembersPanelOpen) + { + viewModel.Vaults.CloseMembersPanelCommand.Execute(null); + e.Handled = true; + } else if (e.Key == Key.Escape && viewModel.IsSettingsMode) { viewModel.LeaveSettingsCommand.Execute(null); diff --git a/src/DodoSSH.Client.App/Views/SettingsSecurityPage.axaml b/src/DodoSSH.Client.App/Views/SettingsSecurityPage.axaml index 7550f3d..778b49b 100644 --- a/src/DodoSSH.Client.App/Views/SettingsSecurityPage.axaml +++ b/src/DodoSSH.Client.App/Views/SettingsSecurityPage.axaml @@ -46,19 +46,21 @@ - - + + + - + - - + diff --git a/src/DodoSSH.Client.Shell/ViewModels/HetznerImportViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/HetznerImportViewModel.cs index 51d08fb..0dd0a42 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/HetznerImportViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/HetznerImportViewModel.cs @@ -566,7 +566,7 @@ internal sealed partial class HetznerImportViewModel( // and visible rather than harmless and confusing. Refresh(resetSelection: true); - Status = WhatWasStored(outcome.Hosts); + Status = WhatWasStored(outcome.Hosts) + outcome.Caveats; Step = HetznerImportStep.Done; } diff --git a/src/DodoSSH.Client.Shell/ViewModels/ImportViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/ImportViewModel.cs index 75ffa46..691875e 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/ImportViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/ImportViewModel.cs @@ -22,10 +22,27 @@ namespace DodoSSH.Client.Shell.ViewModels; /// internal sealed record ImportedHostRequest(HostSecret Host, SshKeySecret? Key, string? KeyPath); -/// What an import stored. +/// What an import stored, and what it did not. /// Hosts written. /// Private keys written. Counts files read, not hosts bound to one. -internal sealed record ImportOutcome(int Hosts, int Keys); +/// Hosts skipped because they would not validate — no hostname, a bad port. +/// Why the batch stopped before the end, or null if it did not. +/// +/// The last two are what the importing screen needs to not claim a success it did not have. The vault's own +/// status line says these things too, but it is hidden in settings mode, which is where both importers live. +/// +internal sealed record ImportOutcome(int Hosts, int Keys, int Refused = 0, string? Failure = null) +{ + /// What to add after the screen's own sentence about what was stored. + internal string Caveats => + (Refused switch + { + 0 => string.Empty, + 1 => " 1 was skipped because it is not a host this keychain can connect to.", + _ => $" {Refused} were skipped because they are not hosts this keychain can connect to.", + }) + + (Failure is { } failure ? $" The import stopped early: {failure}" : string.Empty); +} /// One host an ssh_config offered, as a row somebody decides about. /// @@ -409,7 +426,7 @@ internal sealed partial class ImportViewModel( _ => $" {outcome.Keys} private keys came with them, and every host that named one is bound to it.", }; - return $"Imported {outcome.Hosts} host(s). They are on the Hosts screen.{material}"; + return $"Imported {outcome.Hosts} host(s). They are on the Hosts screen.{material}{outcome.Caveats}"; } /// diff --git a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs index a67d7ff..e84e703 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs @@ -1795,6 +1795,10 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp ActiveSettingsPage = null; IsImportOpen = false; IsHetznerImportOpen = false; + + // Drawn over Settings → Vaults. Left open, it was still up the next time Settings opened, over + // whichever page that was. + vaults.IsMembersPanelOpen = false; Screen = settingsReturnScreen; Surface = settingsReturnSurface; } @@ -5107,6 +5111,14 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp Screen = ShellScreen.Hosts; Surface = ShellSurface.Page; } + else + { + // Settings is a mode of the unlocked window, and it brings its own title bar while the main one + // is hidden. Locking or signing out from it — LOCK NOW is on Preferences, sign-out on Account — + // left the window in settings mode under the lock card: no title bar, so nothing to drag, minimise + // or close, and an unlock that came back to Settings rather than to Hosts. + LeaveSettings(); + } } /// diff --git a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs index 97f226e..b50889b 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs @@ -8838,53 +8838,94 @@ internal sealed partial class VaultViewModel( ArgumentNullException.ThrowIfNull(requests); var target = vaultId ?? session.ActiveVaultId; - var hosts = 0; - var keys = 0; - var refused = 0; + var tally = new ImportTally(); + var ran = false; + string? failure = null; await RunAsync( requests.Count == 1 ? "Importing 1 host…" : $"Importing {requests.Count} hosts…", async () => { - // Keyed on the path rather than on the material, and filled as the batch runs rather than up - // front: the second host naming a file has to land on the id the first one produced, and - // nothing is reloaded until the batch finishes. - var bound = new Dictionary(StringComparer.OrdinalIgnoreCase); + ran = true; - foreach (var request in requests) + try { - if (!request.Host.TryValidate(out _)) - { - refused++; - continue; - } - - var (keyId, created) = await BindImportedKeyAsync( - request, bound, target, cancellationToken) - .ConfigureAwait(true); - - if (created) - { - keys++; - } - - var host = keyId is { } id ? request.Host with { SshKeyId = id } : request.Host; - - await session.Hosts - .CreateAsync(target, host, cancellationToken) - .ConfigureAwait(true); - - hosts++; + await ImportEachAsync(requests, target, tally, cancellationToken).ConfigureAwait(true); + } + catch (Exception exception) when (exception is not OutOfMemoryException + and not OperationCanceledException) + { + // Reported, and the hosts written before it are still reloaded below: they are in the + // keychain whether or not the rest followed, and the screen has to say both. + failure = exception.Message; } await ReloadAsync(cancellationToken).ConfigureAwait(true); - Status = DescribeImport(hosts, keys, refused); + Status = DescribeImport(tally.Hosts, tally.Keys, tally.Refused) + + (failure is null ? string.Empty : $" The import stopped early: {failure}"); }).ConfigureAwait(true); + if (!ran) + { + // RunAsync returns at once while anything else is running — a sync pass, a save — and the + // screen asking would otherwise have said it imported nothing as though that were the result. + failure = "the keychain was busy with something else. Nothing was imported; try again."; + } + await AutoSyncAsync(cancellationToken).ConfigureAwait(true); - return new ImportOutcome(hosts, keys); + return new ImportOutcome(tally.Hosts, tally.Keys, tally.Refused, failure); + } + + /// Writes each host of an import, and any key it brought, counting as it goes. + /// + /// Counted into rather than returned, so that when a write fails partway the + /// caller still knows how many landed before it — which is what it has to tell the person. + /// + private async Task ImportEachAsync( + IReadOnlyList batch, + Guid into, + ImportTally tally, + CancellationToken token) + { + // Keyed on the path rather than on the material, and filled as the batch runs rather than up + // front: the second host naming a file has to land on the id the first one produced, and nothing + // is reloaded until the batch finishes. + var bound = new Dictionary(StringComparer.OrdinalIgnoreCase); + + foreach (var request in batch) + { + if (!request.Host.TryValidate(out _)) + { + tally.Refused++; + continue; + } + + var (keyId, created) = await BindImportedKeyAsync(request, bound, into, token) + .ConfigureAwait(true); + + if (created) + { + tally.Keys++; + } + + var host = keyId is { } id ? request.Host with { SshKeyId = id } : request.Host; + + await session.Hosts.CreateAsync(into, host, token).ConfigureAwait(true); + + tally.Hosts++; + } + } + + /// What one import has written so far. + private sealed class ImportTally + { + internal int Hosts { get; set; } + + internal int Keys { get; set; } + + internal int Refused { get; set; } } /// Finds or stores the key an imported host should be bound to. diff --git a/src/DodoSSH.Client.Shell/ViewModels/VaultsViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/VaultsViewModel.cs index 8e0184a..27017e7 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/VaultsViewModel.cs +++ b/src/DodoSSH.Client.Shell/ViewModels/VaultsViewModel.cs @@ -1639,11 +1639,32 @@ internal sealed partial class VaultsViewModel( } // Fire-and-forget on purpose, and the only place in this class that is: selection changes come - // from a list box, which has no cancellation token and no way to await. Wrapped here rather than - // inside the read, because the read is also awaited from inside a reload that already holds the - // busy gate — and a second RunAsync there would find the gate held by its own caller and skip the - // members list entirely. - _ = RunAsync(() => LoadSelectedAsync(CancellationToken.None)); + // from a list box, which has no cancellation token and no way to await. + // + // ◆ Not through RunAsync. Its busy gate returns at once while anything else runs — the first load, + // a share, a reload — so a vault chosen in that moment never had its people read, and the panel + // went on showing the previous vault's under the new one's name, with SHARE and REVOKE acting on + // that mixture. The read does not need the gate: LoadSelectedAsync bumps the selection generation, + // and every answer for an earlier selection is discarded by it. + _ = LoadSelectionAsync(); + } + + /// Reads the newly selected vault's people, turning a failure into a sentence. + private async Task LoadSelectionAsync() + { + try + { + await LoadSelectedAsync(CancellationToken.None).ConfigureAwait(true); + } + catch (DodoSshApiException exception) + { + Status = exception.Message; + } + catch (Exception exception) when (exception is not OutOfMemoryException + and not OperationCanceledException) + { + Status = exception.Message; + } } partial void OnPendingActionChanged(VaultActionRequest? value) diff --git a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs index b8538da..c5445f1 100644 --- a/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs +++ b/tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs @@ -1871,6 +1871,31 @@ public sealed partial class ShellFlowTests : IAsyncLifetime /// connect and restored afterwards — and the dial itself goes through the ordinary connect path, so the /// host key gate and the log entry are the ones every other connection gets. /// + /// + /// A sync pass reloads the vault, which empties its host list and fills it again — and the picker used to + /// re-find its selection from the empty moment, landing on the first host every minute. CONNECT then + /// dialled that host with the password typed for the one chosen. + /// + [Fact] + public async Task TheFilesHostPicker_KeepsTheChosenHostAndItsPasswordAcrossAVaultReload() + { + var vault = await ReadyToConnectAsync(); + await AddHostAsync(vault, "staging-db"); + + shell.Transfers.Attach(vault, knownHosts); + + var chosen = shell.Transfers.Hosts.Single(row => row.Host.Label == "staging-db"); + shell.Transfers.SelectedHost = chosen; + shell.Transfers.TypedPassword = "typed for staging"; + + shell.Transfers.Hosts[0].ShouldNotBe(chosen, "otherwise falling back to the first proves nothing"); + + await vault.LoadAsync(Token); + + shell.Transfers.SelectedHost.ShouldNotBeNull().EntityId.ShouldBe(chosen.EntityId); + shell.Transfers.TypedPassword.ShouldBe("typed for staging"); + } + [Fact] public async Task ReconnectingALostFileSession_DialsAgainAndGoesBackToTheDirectoryItWasShowing() { @@ -5705,6 +5730,16 @@ public sealed partial class ShellFlowTests : IAsyncLifetime .Single(row => string.Equals(row.Alias, "web-01", StringComparison.Ordinal)) .IsSelected.ShouldBeTrue(); + // A keychain busy with something else — a sync pass, a save — used to skip the whole batch and let this + // screen say "Imported 0 host(s)" as though that were the result. + vault.IsBusy = true; + + await import.ImportCommand.ExecuteAsync(null); + + vault.IsBusy = false; + vault.Hosts.Count.ShouldBe(1); + import.Status.ShouldContain("Nothing was imported"); + await import.ImportCommand.ExecuteAsync(null); var imported = vault.Hosts.Single(row => string.Equals(row.Label, "web-01", StringComparison.Ordinal)); @@ -10628,6 +10663,35 @@ public sealed partial class ShellFlowTests : IAsyncLifetime shell.Screen.ShouldBe(ShellScreen.Keychain); } + /// + /// LOCK NOW is on the Preferences page. Settings mode hides the main title bar and draws its own, which + /// the lock card then covers — so a lock that stayed in settings mode left a window with nothing to drag, + /// minimise or close, and an unlock that came back to Settings rather than to Hosts. + /// + [Fact] + public async Task LockingFromSettings_LeavesSettingsModeAndUnlockingLandsOnHosts() + { + await ReadyToConnectAsync(); + shell.ShowScreenCommand.Execute(ShellScreen.Keychain); + + shell.EnterSettingsCommand.Execute(SettingsPage.Vaults); + await shell.Vaults.LoadAsync(Token); + shell.Vaults.OpenMembersPanelCommand.Execute(shell.Vaults.Vaults[0]); + shell.Vaults.IsMembersPanelOpen.ShouldBeTrue("otherwise its closing proves nothing"); + + await shell.LockCommand.ExecuteAsync(null); + + shell.State.ShouldBe(ShellState.Locked); + shell.IsSettingsMode.ShouldBeFalse("the lock card needs the main title bar above it"); + shell.Vaults.IsMembersPanelOpen.ShouldBeFalse(); + + shell.Passphrase = Passphrase; + await shell.UnlockCommand.ExecuteAsync(null); + + shell.IsSettingsMode.ShouldBeFalse(); + shell.Screen.ShouldBe(ShellScreen.Hosts); + } + /// /// Settings mode collapses the terminal the same way any other page does — /// and are exclusive by construction — and "Back to application" has diff --git a/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs b/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs index 43edf05..9acd93a 100644 --- a/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs +++ b/tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs @@ -2177,10 +2177,70 @@ public sealed class VaultSharingTests : IAsyncLifetime vaults.Members.ShouldHaveSingleItem().Role.ShouldBe("OWNER"); } + /// + /// A vault chosen while anything else on the screen was running — a reload, a share — went through a busy + /// gate that answered by doing nothing, so its people were never read and the panel kept showing the + /// previous vault's under the new one's name. SHARE and REVOKE then acted on that mixture. + /// + [Fact] + public async Task AVaultChosenWhileAReloadIsRunning_ShowsItsOwnPeople() + { + await UnlockedAsync(); + + var vaults = shell.Vaults; + server.AddAccount("bob@example.com", "Bob Example"); + + await CreateVaultAsync(vaults, "Alone"); + var alone = vaults.SelectedVault!.VaultId; + + await CreateVaultAsync(vaults, "Shared with Bob"); + var shared = vaults.SelectedVault!.VaultId; + + vaults.NewMemberEmail = "bob@example.com"; + await vaults.AddMemberCommand.ExecuteAsync(null); + vaults.Members.Count.ShouldBe(2, vaults.Status); + + vaults.SelectedVault = vaults.Vaults.Single(row => row.VaultId == alone); + await EventuallyAsync(() => vaults.Members.Count == 1, "the vault on its own has only its owner"); + + var gate = new TaskCompletionSource(); + server.MemberReadGate = gate; + + var reload = vaults.LoadAsync(Token); + + vaults.IsBusy.ShouldBeTrue("the reload is what holds the screen busy while its read is held"); + + vaults.SelectedVault = vaults.Vaults.Single(row => row.VaultId == shared); + + gate.SetResult(); + server.MemberReadGate = null; + await reload; + + await EventuallyAsync(() => vaults.Members.Count == 2, "Bob is in the vault now showing"); + } + /// /// Through the form rather than straight at the command, because the name is what the form is for — /// and because the form is now the only way in: there is no separate "make a team" step behind it. /// + /// Waits for something a fire-and-forget read will make true, within a bound. + private static async Task EventuallyAsync(Func condition, string because) + { + var deadline = TimeProvider.System.GetUtcNow().AddSeconds(10); + + while (TimeProvider.System.GetUtcNow() < deadline) + { + if (condition()) + { + return; + } + + await Task.Delay(TimeSpan.FromMilliseconds(20), Token); + } + + throw new ShouldAssertException(because); + } + /// The armour a key is stored in, which this suite never parses and only round-trips. private static string PrivateKey(string body) => $"-----BEGIN OPENSSH PRIVATE KEY-----\n{body}\n-----END OPENSSH PRIVATE KEY-----\n";