Merge branch 'claude/edit-screen-refresh-items-63a808'

docs/manual-checks.md

@@ -915,6 +915,25 @@ add a bucket — then come back.
 **Failure means:** the screen has gone back to copying the vault's lists once at unlock. Covered by
 `TheTransfersScreen_FollowsTheVaultsHostList`; this is the version of it with a real picker in front of it.
 
+### 7.12 A key made now can be bound in the editor that is already open
+
+The same check one screen over, and the ordinary way round: start editing a host, type a port into it, then go
+to KEYS and add a key — or a password, or a tag — and come back to HOSTS. Repeat it with a group's editor open
+instead of a host's.
+
+**Pass:** the editor is still open with the port still in it, and the new key is in the AUTHENTICATION picker
+straight away. Choosing it and saving binds the host. A new tag is a chip among the others, unworn.
+
+Then the other half, which matters more: with a host that already authenticates with a key, open its editor
+and add a *second* key. **Pass:** the picker still reads the first one, and saving leaves the host bound to it.
+
+**Failure means:** an editor that has to be cancelled and reopened before it can see what was just made — the
+pickers went back to being snapshots taken when the editor opened, which was defensible only while the two
+screens refused each other. See `VaultViewModel.RefreshOpenEditors`, and
+`AKeyAddedWithTheHostEditorOpen_AppearsInItsAuthenticationPicker` for the same thing asserted against view
+models. A picker that *moves* instead — landing on the key that just arrived, or back on "Password (ask each
+time)" — is the worse failure of the two: it rebinds a host as a side effect of somebody else's sync.
+
 ---
 
 ## Phase 8 — Adding and removing on the phone's host list

src/DodoSSH.Client.Shell/ViewModels/SnippetsViewModel.cs

@@ -72,6 +72,11 @@ internal sealed partial class SnippetsViewModel : ObservableObject
 
         vault.Snippets.CollectionChanged += OnSnippetsChanged;
 
+        // The editor's vault picker is a snapshot of this list — see BuildEditorVaultChoices — and a vault
+        // created on the Teams screen with a half-typed snippet open behind it would otherwise not be
+        // offered until the editor was closed and opened again.
+        vault.TargetVaults.CollectionChanged += OnTargetVaultsChanged;
+
         Rebuild();
     }
 
@@ -577,6 +582,21 @@ internal sealed partial class SnippetsViewModel : ObservableObject
 
     private void OnSnippetsChanged(object? sender, NotifyCollectionChangedEventArgs e) => Rebuild();
 
+    /// <summary>Refills the open editor's vault picker, landing back on the vault it was already writing to.</summary>
+    /// <remarks>
+    /// Only while the editor is open, because that is the only picker built from a copy of the list — the move
+    /// panel's is built when it opens and folds away with the selection it was opened about. Restored by the
+    /// latched <see cref="editorVaultId"/> rather than by what the control shows, so a vault arriving mid-edit
+    /// cannot move a half-typed snippet: the refill is invisible except for the entry it adds.
+    /// </remarks>
+    private void OnTargetVaultsChanged(object? sender, NotifyCollectionChangedEventArgs e)
+    {
+        if (IsEditing)
+        {
+            BuildEditorVaultChoices(editorVaultId);
+        }
+    }
+
     private void Rebuild()
     {
         // Captured and restored around the refill, for the reason the host sidebar's rebuild is written the

src/DodoSSH.Client.Shell/ViewModels/VaultViewModel.cs

@@ -2629,10 +2629,19 @@ internal sealed partial class VaultViewModel(
     /// What the authentication picker offers: a typed password, then every key, then every credential.
     /// </summary>
     /// <remarks>
-    /// Rebuilt when the editor opens rather than kept in step with the two lists. A background sync could pull
+    /// <para>
-    /// a new key while a host is being edited, and having the picker's contents change under the user mid-edit
+    /// Filled when the editor opens and kept in step with the two lists from then on, by
-    /// is worse than the list being a minute stale — only one editor may be open at a time, so the only way to
+    /// <see cref="RefreshOpenEditors"/>. It used to be the snapshot alone, on the grounds that only one editor
-    /// add a key or a credential is to close this one anyway.
+    /// could be open at a time and so the only way to add a key was to close this one — which stopped being
+    /// true when <see cref="AHostEditorIsInTheWay"/> was split from <see cref="AVaultEditorIsInTheWay"/>. A
+    /// host editor now sits open on the Hosts screen while a key is added on the Keychain screen, and a
+    /// picker that did not notice left the user cancelling an editor to see the key they had just made.
+    /// </para>
+    /// <para>
+    /// What the old note was protecting against is real, and is why the refill restores the selection by id
+    /// rather than rebuilding from the stored host: entries appear and disappear under the user, but what
+    /// they have chosen does not move.
+    /// </para>
     /// </remarks>
     internal ObservableCollection<AuthenticationChoice> EditorAuthenticationChoices { get; } = [];
 
@@ -3529,9 +3538,86 @@ internal sealed partial class VaultViewModel(
         // After all four lists, because the table is a projection of three of them.
         RebuildVaultItems();
 
+        // Last of the rebuilds, because every picker an open editor holds is drawn from one of the lists
+        // above and would otherwise be showing the vault as it was when that editor opened.
+        RefreshOpenEditors();
+
         await LoadConflictsAsync(cancellationToken).ConfigureAwait(true);
     }
 
+    /// <summary>
+    /// Refills the pickers of whichever editor is open, so a key or a vault that has just arrived shows in it.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// The pickers are snapshots — see <see cref="EditorAuthenticationChoices"/> — and taking one when the
+    /// editor opens was correct while nothing could change a list without closing it first. Nothing about
+    /// that holds any more: the host editor lives on the Hosts screen and the keychain's editors live on the
+    /// Vault screen, so a key, a credential, a tag, a group or a whole vault can be added with a host editor
+    /// standing open behind it. Every one of them then failed to appear in the picker that exists to offer
+    /// it, and the only way to see it was to abandon the edit and start again.
+    /// </para>
+    /// <para>
+    /// Called from the one reload rather than from each of the twenty-odd places that write to the vault,
+    /// which is what makes a sync count as well as a save: a key pulled from another machine reaches the
+    /// open editor by the same path a key typed here does.
+    /// </para>
+    /// <para>
+    /// <b>Selections are carried across by id, and every typed field is left alone.</b> The refill has to be
+    /// invisible to somebody halfway through a form — an editor that reset its own bindings because a
+    /// background sync landed would be a worse bug than the stale list it fixes. So each picker is rebuilt
+    /// and then put back onto what it was already showing, including the placeholder entries that stand for
+    /// a binding whose target has gone: it is the same restore-by-id the open path does, from the editor's
+    /// current selection rather than from the stored item, because for as long as the editor is open the two
+    /// deliberately differ.
+    /// </para>
+    /// </remarks>
+    private void RefreshOpenEditors()
+    {
+        if (IsEditing)
+        {
+            // Read before anything is cleared. Rebuilding a bound collection makes the control null its own
+            // selection and write that back, so by the time the last picker is refilled these properties no
+            // longer say what the user chose.
+            var authentication = EditorSelectedAuthentication;
+            var groupId = EditorSelectedGroup?.EntityId;
+
+            BuildTagChoices();
+
+            // The same order the two commands that open this editor use: the vault picker first because a
+            // group belongs to one vault, then the groups, then the bindings — which offer "inherit from
+            // group" only where the group picker has landed on something.
+            BuildEditorVaultChoices(editingHostVaultId);
+
+            // The selection as it stands, not filtered through GroupInEditingVault: a group that has gone
+            // keeps its placeholder here for the reason it does on the open path, so that a sync arriving
+            // mid-edit cannot unfile the host when the form is saved.
+            BuildGroupChoices(groupId);
+
+            BuildAuthenticationChoices(
+                authentication?.Kind == AuthenticationKind.SshKey ? authentication.EntityId : null,
+                authentication?.Kind == AuthenticationKind.Credential ? authentication.EntityId : null,
+                asksForPassword: authentication?.Kind == AuthenticationKind.Typed,
+                grouped: EditorSelectedGroup?.EntityId is not null);
+        }
+
+        if (IsEditingGroup)
+        {
+            var authentication = GroupEditorSelectedAuthentication;
+            var parentId = GroupEditorSelectedParent?.EntityId;
+
+            BuildGroupEditorVaultChoices(GroupEditorVaultId);
+
+            // Guid.Empty while creating, which is what EditingGroupId being null means and is the same
+            // stand-in ClearGroupEditor uses: a group that does not exist yet cannot be its own parent.
+            BuildGroupParentChoices(EditingGroupId ?? Guid.Empty, parentId);
+
+            BuildGroupAuthenticationChoices(
+                authentication?.Kind == AuthenticationKind.SshKey ? authentication.EntityId : null,
+                authentication?.Kind == AuthenticationKind.Credential ? authentication.EntityId : null);
+        }
+    }
+
     /// <summary>Redraws every list from the vault, without saying anything about it.</summary>
     /// <remarks>
     /// For the two things that change which vaults exist or which are drawn without going through this type

tests/DodoSSH.Client.App.Tests/ShellFlowTests.cs

@@ -2494,6 +2494,130 @@ public sealed class ShellFlowTests : IAsyncLifetime
         vault.Status.ShouldContain("host");
     }
 
+    // ---- Keeping an open editor's pickers in step with the vault ----
+
+    /// <remarks>
+    /// The other side of the split guard, and the bug it left behind. The host editor and the keychain's
+    /// editors are on different screens and refuse each other no longer, so a key is very often added
+    /// <em>because</em> the host in front of the user needs one — with that host's editor still standing on the
+    /// Hosts screen. The picker was a snapshot taken when the editor opened, so the key never appeared in it
+    /// and the only way to reach it was to abandon the edit and start again.
+    /// </remarks>
+    [Fact]
+    public async Task AKeyAddedWithTheHostEditorOpen_AppearsInItsAuthenticationPicker()
+    {
+        await UnlockedAsync();
+        var vault = shell.Vault!;
+
+        await AddHostAsync(vault, "prod-db");
+        vault.SelectedHost = vault.Hosts[0];
+        vault.EditSelectedHostCommand.Execute(null);
+        vault.EditorPort = 2244;
+
+        await AddKeyAsync(vault, "deploy");
+
+        vault.IsEditing.ShouldBeTrue("adding a key must not close the host editor");
+        vault.EditorPort.ShouldBe(2244, "nor discard what has been typed into it");
+
+        var offered = vault.EditorAuthenticationChoices.Single(
+            choice => string.Equals(choice.Label, "deploy", StringComparison.Ordinal));
+
+        offered.Kind.ShouldBe(AuthenticationKind.SshKey);
+        offered.EntityId.ShouldBe(vault.Keys[0].EntityId);
+
+        // A real entry rather than a label: choosing it and saving is what the user came here to do.
+        vault.EditorSelectedAuthentication = offered;
+        await vault.SaveHostCommand.ExecuteAsync(null);
+
+        vault.Hosts.ShouldHaveSingleItem().Host.SshKeyId.ShouldBe(vault.Keys[0].EntityId);
+        vault.Hosts[0].Host.Port.ShouldBe(2244);
+    }
+
+    /// <remarks>
+    /// The counterweight, and the reason the refill restores each picker by id rather than reloading the
+    /// stored host: a list that grows under somebody halfway through a form must not move what they had
+    /// already chosen in it, and the save that follows must not rebind the host to something nobody picked.
+    /// </remarks>
+    [Fact]
+    public async Task AnItemArrivingWhileTheHostEditorIsOpen_LeavesItsSelectionWhereItWas()
+    {
+        var vault = await ReadyToConnectAsync();
+        await AddKeyAsync(vault, "deploy");
+
+        var keyId = vault.Keys.ShouldHaveSingleItem().EntityId;
+
+        await BindKeyAsync(vault, vault.Hosts[0], keyId);
+
+        vault.SelectedHost = vault.Hosts[0];
+        vault.EditSelectedHostCommand.Execute(null);
+
+        await AddCredentialAsync(vault, "pg-primary");
+
+        vault.EditorAuthenticationChoices.ShouldContain(
+            choice => choice.Kind == AuthenticationKind.Credential);
+
+        vault.EditorSelectedAuthentication.ShouldNotBeNull().EntityId
+            .ShouldBe(keyId, "a credential appearing must not unbind the host from its key");
+
+        await vault.SaveHostCommand.ExecuteAsync(null);
+
+        vault.Hosts.ShouldHaveSingleItem().Host.SshKeyId.ShouldBe(keyId);
+        vault.Hosts[0].Host.CredentialId.ShouldBeNull();
+    }
+
+    /// <remarks>
+    /// Tags reach the same editor by a different route — the keychain screen rather than the box under the
+    /// chips — and a chip that only appeared on the next open would send the user round the same detour.
+    /// </remarks>
+    [Fact]
+    public async Task ATagAddedWithTheHostEditorOpen_AppearsAmongItsChips()
+    {
+        await UnlockedAsync();
+        var vault = shell.Vault!;
+
+        await AddHostAsync(vault, "prod-db");
+        vault.SelectedHost = vault.Hosts[0];
+        vault.EditSelectedHostCommand.Execute(null);
+
+        await AddTagAsync(vault, "production");
+
+        vault.HasTagChoices.ShouldBeTrue();
+
+        var chip = vault.EditorTagChoices.ShouldHaveSingleItem();
+        chip.Label.ShouldBe("production");
+        chip.IsWorn.ShouldBeFalse("appearing is not the same as being put on");
+    }
+
+    /// <remarks>
+    /// The group editor shares the drawer with the host editor and its own picker was the same snapshot, so
+    /// the same detour applied to the default binding a whole group of hosts inherits.
+    /// </remarks>
+    [Fact]
+    public async Task AKeyAddedWithTheGroupEditorOpen_AppearsInItsDefaultBindingPicker()
+    {
+        await UnlockedAsync();
+        var vault = shell.Vault!;
+
+        await AddGroupAsync(vault, "platform");
+
+        vault.SelectedGroup = vault.Groups.ShouldHaveSingleItem();
+        vault.EditGroupCommand.Execute(null);
+        vault.GroupEditorDefaultPort = 2222;
+
+        await AddKeyAsync(vault, "deploy");
+
+        vault.IsEditingGroup.ShouldBeTrue("adding a key must not close the group editor");
+        vault.GroupEditorDefaultPort.ShouldBe(2222, "nor discard what has been typed into it");
+
+        vault.GroupEditorSelectedAuthentication = vault.GroupEditorAuthenticationChoices.Single(
+            choice => string.Equals(choice.Label, "deploy", StringComparison.Ordinal));
+
+        await vault.SaveGroupCommand.ExecuteAsync(null);
+
+        vault.Groups.ShouldHaveSingleItem().Group.DefaultSshKeyId.ShouldBe(vault.Keys[0].EntityId);
+        vault.Groups[0].Group.DefaultPort.ShouldBe(2222);
+    }
+
     // ---- Binding a key to a host ----
 
     [Fact]

tests/DodoSSH.Client.App.Tests/VaultSharingTests.cs

@@ -319,6 +319,95 @@ public sealed class VaultSharingTests : IAsyncLifetime
         shell.Vault.HasVaultChoice.ShouldBeTrue();
     }
 
+    /// <remarks>
+    /// <para>
+    /// And it reaches a host editor that was already open, which is very nearly the only way a vault gets made:
+    /// somebody starts adding a host, realises it belongs to the team rather than to them, and goes to make
+    /// somewhere to put it. The editor's picker is its own — see <c>VaultViewModel.EditorVaultChoices</c> — and
+    /// it was filled when the editor opened, so the vault they had just made for this host was the one place
+    /// they could not file it without throwing the form away.
+    /// </para>
+    /// <para>
+    /// The second half is what stops the fix being worse than the bug: the refill adds the entry and leaves the
+    /// selection alone, so a vault appearing cannot move a half-typed host into it.
+    /// </para>
+    /// </remarks>
+    [Fact]
+    public async Task AVaultCreatedWithAHostEditorOpen_IsOfferedAsSomewhereToFileThatHost()
+    {
+        await UnlockedAsync();
+
+        var vault = shell.Vault!;
+
+        vault.NewHostCommand.Execute(null);
+        vault.EditorLabel = "prod-db";
+        vault.EditorHostname = "db.internal";
+
+        await CreateVaultAsync(shell.Vaults, "Platform secrets");
+
+        var vaultId = shell.Vaults.SelectedVault!.VaultId;
+
+        vault.IsEditing.ShouldBeTrue("making a vault must not close the host editor");
+        vault.EditorLabel.ShouldBe("prod-db", "nor discard what has been typed into it");
+
+        vault.ShowsEditorVaultChoice.ShouldBeTrue(
+            "a second writable vault is what makes the picker worth drawing at all");
+
+        vault.EditorVaultChoices.Select(choice => choice.VaultId).ShouldContain(vaultId);
+
+        vault.EditorSelectedVault.ShouldNotBeNull().IsPersonal
+            .ShouldBeTrue("the refill must not move the host into the vault that has just appeared");
+
+        // Choosing it, on the other hand, files the host there — which is what the picker was for.
+        vault.EditorSelectedVault = vault.EditorVaultChoices.Single(
+            choice => choice.VaultId == vaultId);
+
+        await vault.SaveHostCommand.ExecuteAsync(null);
+
+        vault.Hosts.ShouldHaveSingleItem().VaultId.ShouldBe(vaultId, vault.Status);
+    }
+
+    /// <remarks>
+    /// The snippet editor's picker is a copy of the same list — see
+    /// <c>SnippetsViewModel.BuildEditorVaultChoices</c> — and went stale in exactly the same way. It watches the
+    /// list rather than the reload, because this screen has always been a wrapper over the vault's collections
+    /// and has no reload of its own to hang off.
+    /// </remarks>
+    [Fact]
+    public async Task AVaultCreatedWithASnippetEditorOpen_IsOfferedAsSomewhereToFileThatSnippet()
+    {
+        await UnlockedAsync();
+
+        var vault = shell.Vault!;
+        var snippets = SnippetsOver(vault);
+
+        snippets.NewCommand.Execute(null);
+        snippets.EditorLabel = "restart the api";
+        snippets.EditorCommand = "sudo systemctl restart dodossh-api";
+
+        await CreateVaultAsync(shell.Vaults, "Platform secrets");
+
+        var vaultId = shell.Vaults.SelectedVault!.VaultId;
+
+        snippets.IsEditing.ShouldBeTrue("making a vault must not close the snippet editor");
+        snippets.EditorCommand.ShouldBe(
+            "sudo systemctl restart dodossh-api",
+            "nor discard what has been typed into it");
+
+        snippets.ShowsEditorVaultChoice.ShouldBeTrue("there are two vaults to choose between now");
+        snippets.EditorVaultChoices.Select(choice => choice.VaultId).ShouldContain(vaultId);
+
+        snippets.EditorSelectedVault.ShouldNotBeNull().IsPersonal
+            .ShouldBeTrue("the refill must not move the snippet into the vault that has just appeared");
+
+        snippets.EditorSelectedVault = snippets.EditorVaultChoices.Single(
+            choice => choice.VaultId == vaultId);
+
+        await snippets.SaveCommand.ExecuteAsync(null);
+
+        Snippet(snippets, "restart the api").VaultId.ShouldBe(vaultId, snippets.Status);
+    }
+
     /// <remarks>
     /// Making a vault makes exactly one membership list, and this is the assertion that the two-step create
     /// has not started leaking them: the screen no longer offers to make one on its own, so a second one