Desktop: swap the host lists in whole instead of announcing every row to listeners that redo all their work per row

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

@@ -0,0 +1,44 @@
+using System.Collections.ObjectModel;
+using System.Collections.Specialized;
+using System.ComponentModel;
+
+namespace DodoSSH.Client.Shell.ViewModels;
+
+/// <summary>
+/// An observable list whose whole contents can be swapped and announced once.
+/// </summary>
+/// <remarks>
+/// <para>
+/// For the lists rebuilt wholesale from the vault. They used to be emptied with <c>Clear</c> and refilled an
+/// <c>Add</c> at a time, which announced every row separately — and two other view models listen to the host
+/// list and redo their own work on every announcement: the transfers screen refilled its host picker, and the
+/// shell walked every host against every tab. A reload of n hosts was n of each, so roughly n² rows touched;
+/// three hundred hosts came to some ninety thousand notifications per reload.
+/// </para>
+/// <para>
+/// One <see cref="NotifyCollectionChangedAction.Reset"/> instead, which is also exactly what <c>Clear</c> has
+/// always said first, so a listener that coped with the old sequence copes with this. Nothing listening to
+/// these lists reads the items an announcement carries; they all re-read the list.
+/// </para>
+/// </remarks>
+internal sealed class ReplaceableCollection<T> : ObservableCollection<T>
+{
+    /// <summary>Makes <paramref name="items"/> the whole of the list, and says so once.</summary>
+    internal void ReplaceAll(IEnumerable<T> items)
+    {
+        ArgumentNullException.ThrowIfNull(items);
+
+        CheckReentrancy();
+
+        Items.Clear();
+
+        foreach (var item in items)
+        {
+            Items.Add(item);
+        }
+
+        OnPropertyChanged(new PropertyChangedEventArgs(nameof(Count)));
+        OnPropertyChanged(new PropertyChangedEventArgs("Item[]"));
+        OnCollectionChanged(new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));
+    }
+}

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

@@ -867,14 +867,14 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo
     /// not this object's to clear. The rows are shared: they carry the decrypted host, and copying that would
     /// be a second decrypted copy of a secret for no gain.
     /// </remarks>
-    internal ObservableCollection<HostRowViewModel> Hosts { get; } = [];
+    internal ReplaceableCollection<HostRowViewModel> Hosts { get; } = [];
 
     [ObservableProperty]
     private HostRowViewModel? selectedHost;
 
     /// <summary>The buckets that can be browsed, which is the vault's list.</summary>
     /// <inheritdoc cref="Hosts" path="/remarks" />
-    internal ObservableCollection<ObjectStoreRowViewModel> Buckets { get; } = [];
+    internal ReplaceableCollection<ObjectStoreRowViewModel> Buckets { get; } = [];
 
     [ObservableProperty]
     private ObjectStoreRowViewModel? selectedBucket;
@@ -3986,7 +3986,21 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo
         }
     }
 
-    private void OnVaultListChanged(object? sender, NotifyCollectionChangedEventArgs e) => RefreshHosts();
+    /// <remarks>
+    /// Only the picker the list feeds. Refilling both on either list's change made every reload refill the host
+    /// picker once for the hosts and again for the buckets.
+    /// </remarks>
+    private void OnVaultListChanged(object? sender, NotifyCollectionChangedEventArgs e)
+    {
+        if (vault is { } open && ReferenceEquals(sender, open.ObjectStores))
+        {
+            RefreshPickers(hosts: false, buckets: true);
+        }
+        else
+        {
+            RefreshPickers(hosts: true, buckets: false);
+        }
+    }
 
     /// <summary>
     /// Rebuilds both pickers from the vault's lists.
@@ -4004,34 +4018,34 @@ internal sealed partial class TransfersViewModel : ObservableObject, IAsyncDispo
     /// connected to.
     /// </para>
     /// </remarks>
-    private void RefreshHosts()
+    private void RefreshHosts() => RefreshPickers(hosts: true, buckets: true);
+
+    private void RefreshPickers(bool hosts, bool buckets)
     {
         refreshingPickers = true;
 
         try
         {
-            Hosts.Clear();
-
             if (vault is not { } open)
             {
+                Hosts.ReplaceAll([]);
                 return;
             }
 
-            foreach (var row in open.Hosts)
+            if (hosts)
             {
-                Hosts.Add(row);
+                Hosts.ReplaceAll(open.Hosts);
-            }
 
                 SelectedHost = Hosts.FirstOrDefault(row => row.EntityId == chosenHost) ?? Hosts.FirstOrDefault();
-
-            Buckets.Clear();
-
-            foreach (var row in open.ObjectStores)
-            {
-                Buckets.Add(row);
             }
 
-            SelectedBucket = Buckets.FirstOrDefault(row => row.EntityId == chosenBucket) ?? Buckets.FirstOrDefault();
+            if (buckets)
+            {
+                Buckets.ReplaceAll(open.ObjectStores);
+
+                SelectedBucket =
+                    Buckets.FirstOrDefault(row => row.EntityId == chosenBucket) ?? Buckets.FirstOrDefault();
+            }
         }
         finally
         {

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

@@ -1663,7 +1663,7 @@ internal sealed partial class VaultViewModel(
     /// host key list checks itself against, so a filter applied here would change what the application can
     /// do rather than what it shows. <see cref="VisibleHosts"/> is the filtered view.
     /// </remarks>
-    internal ObservableCollection<HostRowViewModel> Hosts { get; } = [];
+    internal ReplaceableCollection<HostRowViewModel> Hosts { get; } = [];
 
     /// <summary>The hosts the grid is showing: one level of the group tree, narrowed by the box.</summary>
     /// <remarks>
@@ -1677,7 +1677,7 @@ internal sealed partial class VaultViewModel(
     /// alone: the phone draws <see cref="SidebarRows"/>, which is the same hosts flattened under headings.
     /// </para>
     /// </remarks>
-    internal ObservableCollection<HostRowViewModel> VisibleHosts { get; } = [];
+    internal ReplaceableCollection<HostRowViewModel> VisibleHosts { get; } = [];
 
     /// <summary>Whether the grid has anything to draw.</summary>
     /// <remarks>
@@ -1891,7 +1891,7 @@ internal sealed partial class VaultViewModel(
     /// groupless keychain is one flat grid and nothing above it.
     /// </para>
     /// </remarks>
-    internal ObservableCollection<HostSectionViewModel> HostSections { get; } = [];
+    internal ReplaceableCollection<HostSectionViewModel> HostSections { get; } = [];
 
     /// <summary>Every host on the board, in the order its cards are drawn — every section, one after another.</summary>
     /// <remarks>
@@ -2003,7 +2003,7 @@ internal sealed partial class VaultViewModel(
     internal ObservableCollection<CredentialRowViewModel> Credentials { get; } = [];
 
     /// <summary>The buckets to show, unpushed local state included.</summary>
-    internal ObservableCollection<ObjectStoreRowViewModel> ObjectStores { get; } = [];
+    internal ReplaceableCollection<ObjectStoreRowViewModel> ObjectStores { get; } = [];
 
     /// <summary>The tags to show, with the number of hosts wearing each.</summary>
     /// <remarks>
@@ -4714,18 +4714,14 @@ internal sealed partial class VaultViewModel(
 
         var (rows, unreadable) = await FetchHostRowsAsync(cancellationToken).ConfigureAwait(true);
 
-        Hosts.Clear();
-
         // Grouped by vault, with the one new items go into first, then by name inside each. Two vaults can
         // hold a host with the same label and both are shown: which vault it is in is what tells them
-        // apart, which is why the row carries the name rather than the list deduplicating.
+        // apart, which is why the row carries the name rather than the list deduplicating. Swapped in whole,
-        foreach (var host in rows
+        // and announced once: see ReplaceableCollection for what a row at a time cost.
+        Hosts.ReplaceAll(rows
             .OrderByDescending(row => row.VaultId == session.ActiveVaultId)
             .ThenBy(row => row.VaultName, StringComparer.CurrentCulture)
-            .ThenBy(row => row.Label, StringComparer.CurrentCulture))
+            .ThenBy(row => row.Label, StringComparer.CurrentCulture));
-        {
-            Hosts.Add(host);
-        }
 
         SelectedHost = SelectionAfterReload(selectedId);
         ApplyTheChosenHosts();
@@ -4816,13 +4812,9 @@ internal sealed partial class VaultViewModel(
 
         var selectedId = SelectedObjectStore?.EntityId;
 
-        ObjectStores.Clear();
+        ObjectStores.ReplaceAll(listing.Items
-
+            .OrderBy(store => store.Secret.Label, StringComparer.CurrentCulture)
-        foreach (var store in listing.Items
+            .Select(store => new ObjectStoreRowViewModel(store)));
-            .OrderBy(store => store.Secret.Label, StringComparer.CurrentCulture))
-        {
-            ObjectStores.Add(new ObjectStoreRowViewModel(store));
-        }
 
         SelectedObjectStore = ObjectStores.FirstOrDefault(row => row.EntityId == selectedId);
 
@@ -5388,12 +5380,7 @@ internal sealed partial class VaultViewModel(
         // a walk up the group tree, and this is the map that walk is made against.
         var parents = EffectiveParents();
 
-        VisibleHosts.Clear();
+        VisibleHosts.ReplaceAll(Hosts.Where(host => Matches(host, parents)));
-
-        foreach (var host in Hosts.Where(host => Matches(host, parents)))
-        {
-            VisibleHosts.Add(host);
-        }
 
         RebuildSidebarRows();
 
@@ -5403,7 +5390,7 @@ internal sealed partial class VaultViewModel(
         // property directly, and a host that is not on screen is not one any of them should act on.
         SelectedHost = selected is not null && VisibleHosts.Contains(selected) ? selected : null;
 
-        // After the host selection, not before: this mirrors it, and the ListBox's own answer to the Clear()
+        // After the host selection, not before: this mirrors it, and the ListBox's own answer to the reset
         // above is a null that has to be overwritten rather than read.
         SelectedSidebarRow = SelectedHost;
 
@@ -5473,12 +5460,7 @@ internal sealed partial class VaultViewModel(
     /// </remarks>
     private void RebuildHostSections()
     {
-        HostSections.Clear();
+        HostSections.ReplaceAll(BuildHostSections());
-
-        foreach (var built in BuildHostSections())
-        {
-            HostSections.Add(built);
-        }
 
         // Computed rather than stored, and none of the four has a change notification of its own — the same
         // arrangement RebuildVisibleHosts holds for HasVisibleHosts and NoVisibleHostsMessage, and for the

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

@@ -669,6 +669,38 @@ public sealed partial class ShellFlowTests : IAsyncLifetime
         shell.StatusMessage.ShouldBe("Locked.");
     }
 
+    /// <remarks>
+    /// A reload used to empty the host list and refill it a row at a time, announcing each — and the transfers
+    /// screen refilled its own picker, announcing each of those, on every one of them. Three hundred hosts came
+    /// to some ninety thousand notifications. Counted here as announcements reaching the two lists, which is
+    /// what the transfers screen and the board both answer.
+    /// </remarks>
+    [Fact]
+    public async Task ReloadingTheHosts_AnnouncesTheListOnce_NotOncePerHost()
+    {
+        await UnlockedAsync();
+        var vault = shell.Vault!;
+
+        foreach (var label in new[] { "prod-db", "prod-web", "stage-web", "dev-box" })
+        {
+            await AddHostAsync(vault, label);
+        }
+
+        var vaultAnnouncements = 0;
+        var pickerAnnouncements = 0;
+
+        vault.Hosts.CollectionChanged += (_, _) => vaultAnnouncements++;
+        shell.Transfers.Hosts.CollectionChanged += (_, _) => pickerAnnouncements++;
+
+        await vault.LoadAsync(Token);
+
+        vault.Hosts.Count.ShouldBe(4);
+        shell.Transfers.Hosts.Count.ShouldBe(4);
+
+        vaultAnnouncements.ShouldBe(1);
+        pickerAnnouncements.ShouldBe(1);
+    }
+
     [Fact]
     public async Task AnAutomaticPass_SaysNothingWhenThereIsNothingToDo()
     {