Fix folder-move UI: tree state, stale list, optimistic feedback

BitwardenSharp.slnx

@@ -12,6 +12,7 @@
     <Project Path="tests/Domain.Tests/BitwardenSharp.Domain.Tests.csproj" />
     <Project Path="tests/Application.Tests/BitwardenSharp.Application.Tests.csproj" />
     <Project Path="tests/Infrastructure.Tests/BitwardenSharp.Infrastructure.Tests.csproj" />
+    <Project Path="tests/Desktop.Tests/BitwardenSharp.Desktop.Tests.csproj" />
     <Project Path="tests/Architecture.Tests/BitwardenSharp.Architecture.Tests.csproj" />
   </Folder>
 </Solution>

src/Presentation/Desktop/ViewModels/ItemViewModel.cs

@@ -104,6 +104,16 @@ public async Task LoadIconAsync(IconLoader loader, CancellationToken cancellatio
         Icon = await loader.GetAsync(IconDomain, cancellationToken);
     }
 
+    /// <summary>
+    /// True while a write affecting this item is in flight. The row dims and stops responding, so
+    /// the user sees their action acknowledged without the list reordering under them.
+    /// </summary>
+    [ObservableProperty]
+    [NotifyPropertyChangedFor(nameof(RowOpacity))]
+    private bool _isPending;
+
+    public double RowOpacity => IsPending ? 0.45 : 1.0;
+
     // ── reveal ───────────────────────────────────────────────────────────────────────────────
 
     /// <summary>

src/Presentation/Desktop/ViewModels/VaultViewModel.cs

@@ -52,13 +52,22 @@ public sealed partial class VaultViewModel(
 
     // ── loading ──────────────────────────────────────────────────────────────────────────────
 
-    public async Task LoadAsync()
+    /// <summary>
+    /// Reads folders and items and rebuilds the view.
+    /// </summary>
+    /// <param name="sync">
+    /// Whether to pull from the server first. Pass false when reloading straight after one of our
+    /// own writes: a sync exists to pick up remote changes, and immediately after a local write it
+    /// can only re-fetch state the server has not applied yet, which shows the change as having
+    /// been lost.
+    /// </param>
+    public async Task LoadAsync(bool sync = true)
     {
         IsBusy = true;
         Error = null;
         try
         {
-            await vault.SyncAsync();
+            if (sync) await vault.SyncAsync();
             _allFolders = await vault.GetFoldersAsync();
             _allItems = await vault.GetItemsAsync();
 
@@ -79,10 +88,10 @@ public async Task LoadAsync()
     }
 
     /// <summary>Reloads folders and items while keeping the selected folder path selected.</summary>
-    private async Task ReloadPreservingSelectionAsync()
+    private async Task ReloadPreservingSelectionAsync(bool sync = true)
     {
         var selectedPath = SelectedFolder?.Path;
-        await LoadAsync();
+        await LoadAsync(sync);
 
         if (selectedPath is null) return;
         SelectedFolder = Folders
@@ -97,6 +106,15 @@ private async Task ReloadPreservingSelectionAsync()
     /// </summary>
     private void RebuildFolderTree()
     {
+        // The rebuild discards the FolderNode objects that hold the expansion state, so capture it
+        // by path first. Collapsed rather than expanded, because expanded is the default and a
+        // newly appeared folder should come up open.
+        var collapsed = Folders
+            .SelectMany(f => f.SelfAndDescendants())
+            .Where(n => !n.IsExpanded)
+            .Select(n => n.Path)
+            .ToHashSet(StringComparer.Ordinal);
+
         Folders.Clear();
 
         var counts = _allItems
@@ -143,6 +161,9 @@ private void RebuildFolderTree()
             });
 
         foreach (var root in roots) Folders.Add(root);
+
+        foreach (var node in Folders.SelectMany(f => f.SelfAndDescendants()))
+            if (collapsed.Contains(node.Path)) node.IsExpanded = false;
     }
 
     private void ApplyFilter()
@@ -258,7 +279,22 @@ private async Task DeleteFolderAsync()
         await RunFolderOperationAsync(() => folders.DeleteAsync(node.FolderId!));
     }
 
-    /// <summary>Moves items into a folder. Called by the view when a drag is dropped on the tree.</summary>
+    /// <summary>
+    /// Moves items into a folder. Called by the view when a drag is dropped on the tree.
+    /// </summary>
+    /// <remarks>
+    /// Optimistic: the affected rows are dimmed the moment the drop happens, before the write is
+    /// attempted, so the gesture is acknowledged immediately rather than after a round-trip. On
+    /// success the reload drops them from the list naturally; on failure the dimming is undone and
+    /// the error is shown.
+    ///
+    /// Dimming rather than removing on purpose. Removing and then restoring a failed move would
+    /// make the row vanish and reappear at a different position in a sorted list, which reads as a
+    /// glitch; dimming shows the in-flight state honestly and rollback is just un-dimming.
+    ///
+    /// Optimism stops here. A move is one reversible field on one item — it is not the merge path,
+    /// which writes, verifies and only then deletes, and must never be short-circuited.
+    /// </remarks>
     public async Task MoveItemsToFolderAsync(IReadOnlyList<string> itemIds, FolderNode target)
     {
         if (itemIds.Count == 0) return;
@@ -270,8 +306,14 @@ public async Task MoveItemsToFolderAsync(IReadOnlyList<string> itemIds, FolderNo
             return;
         }
 
+        var moving = Items.Where(i => itemIds.Contains(i.Id)).ToList();
+        foreach (var item in moving) item.IsPending = true;
+
         var folderId = target.IsUnfiled ? null : target.FolderId;
-        await RunFolderOperationAsync(() => folders.MoveItemsAsync(itemIds, folderId));
+        var succeeded = await RunFolderOperationAsync(() => folders.MoveItemsAsync(itemIds, folderId));
+
+        if (!succeeded)
+            foreach (var item in moving) item.IsPending = false;
     }
 
     /// <summary>Moves a folder under another. Called by the view on a folder-to-folder drop.</summary>
@@ -284,7 +326,8 @@ public async Task MoveFolderAsync(FolderNode source, FolderNode? target)
         await RunFolderOperationAsync(() => folders.MoveAsync(source.FolderId!, target?.Path));
     }
 
-    private async Task RunFolderOperationAsync(Func<Task<FolderOperationResult>> operation)
+    /// <summary>Runs a folder operation and reloads on success. Returns whether it succeeded.</summary>
+    private async Task<bool> RunFolderOperationAsync(Func<Task<FolderOperationResult>> operation)
     {
         IsBusy = true;
         Error = null;
@@ -294,13 +337,18 @@ private async Task RunFolderOperationAsync(Func<Task<FolderOperationResult>> ope
             if (!result.Succeeded)
             {
                 Error = result.Error;
-                return;
+                return false;
             }
-            await ReloadPreservingSelectionAsync();
+
+            // No sync: we just wrote this ourselves, and pulling from the server here can return
+            // the pre-write state. See LoadAsync.
+            await ReloadPreservingSelectionAsync(sync: false);
+            return true;
         }
         catch (Exception ex)
         {
             Error = ex.Message;
+            return false;
         }
         finally
         {

src/Presentation/Desktop/Views/VaultView.axaml

@@ -124,8 +124,11 @@
         <ListBox ItemsSource="{Binding Items}" SelectedItem="{Binding SelectedItem}">
           <ListBox.ItemTemplate>
             <DataTemplate x:DataType="vm:ItemViewModel">
+              <!-- Dimmed and inert while its move is in flight; see MoveItemsToFolderAsync. -->
               <Grid ColumnDefinitions="Auto,*,Auto" Margin="0,3"
                     Background="Transparent"
+                    Opacity="{Binding RowOpacity}"
+                    IsHitTestVisible="{Binding !IsPending}"
                     PointerPressed="OnDragSourcePressed"
                     PointerMoved="OnDragSourceMoved"
                     PointerReleased="OnDragSourceReleased">

tests/Desktop.Tests/BitwardenSharp.Desktop.Tests.csproj

@@ -0,0 +1,15 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <IsPackable>false</IsPackable>
+  </PropertyGroup>
+  <ItemGroup>
+    <ProjectReference Include="..\..\src\Presentation\Desktop\BitwardenSharp.Desktop.csproj" />
+  </ItemGroup>
+  <ItemGroup>
+    <PackageReference Include="Microsoft.NET.Test.Sdk" />
+    <PackageReference Include="xunit.v3" />
+    <PackageReference Include="xunit.runner.visualstudio" />
+    <PackageReference Include="Shouldly" />
+    <PackageReference Include="NSubstitute" />
+  </ItemGroup>
+</Project>

tests/Desktop.Tests/FakeVault.cs

@@ -0,0 +1,114 @@
+using BitwardenSharp.Application.Abstractions;
+using BitwardenSharp.Domain.Vault;
+
+namespace BitwardenSharp.Desktop.Tests;
+
+/// <summary>
+/// An in-memory vault. Writes take effect immediately, so a test that still observes stale data
+/// is observing a view-model bug rather than a transport delay.
+/// </summary>
+internal sealed class FakeVault : IVaultClient, IVaultSession
+{
+    private readonly Dictionary<string, VaultItem> _items = [];
+    private readonly Dictionary<string, VaultFolder> _folders = [];
+    private int _sequence;
+
+    public int SyncCount { get; private set; }
+
+    /// <summary>Set to make the next write fail, for testing rollback.</summary>
+    public bool FailNextWrite { get; set; }
+
+    public VaultItem AddItem(string name, string? folderId = null, string? uri = null)
+    {
+        var item = new VaultItem
+        {
+            Id = $"item-{++_sequence:D3}",
+            Type = ItemType.Login,
+            Name = name,
+            FolderId = folderId,
+            Login = new LoginDetails
+            {
+                Username = "user@example.com",
+                Password = "hunter2",
+                Uris = uri is null ? [] : [new LoginUri { Uri = uri }],
+            },
+        };
+        _items[item.Id] = item;
+        return item;
+    }
+
+    public VaultFolder AddFolder(string name)
+    {
+        var folder = new VaultFolder { Id = $"folder-{++_sequence:D3}", Name = name };
+        _folders[folder.Id] = folder;
+        return folder;
+    }
+
+    public Task SyncAsync(CancellationToken cancellationToken = default)
+    {
+        SyncCount++;
+        return Task.CompletedTask;
+    }
+
+    public Task<VaultStatus> GetStatusAsync(CancellationToken cancellationToken = default) =>
+        Task.FromResult(new VaultStatus { Status = "unlocked", UserEmail = "t@example.com" });
+
+    public Task<IReadOnlyList<VaultItem>> GetItemsAsync(CancellationToken cancellationToken = default) =>
+        Task.FromResult<IReadOnlyList<VaultItem>>(_items.Values.ToList());
+
+    public Task<IReadOnlyList<VaultFolder>> GetFoldersAsync(CancellationToken cancellationToken = default) =>
+        Task.FromResult<IReadOnlyList<VaultFolder>>(_folders.Values.ToList());
+
+    public Task<VaultItem> GetItemAsync(string id, CancellationToken cancellationToken = default) =>
+        Task.FromResult(_items[id]);
+
+    public Task<VaultItem> UpdateItemAsync(VaultItem item, CancellationToken cancellationToken = default)
+    {
+        if (FailNextWrite)
+        {
+            FailNextWrite = false;
+            throw new InvalidOperationException("the vault rejected the write");
+        }
+        _items[item.Id] = item;
+        return Task.FromResult(item);
+    }
+
+    public Task<VaultItem> CreateItemAsync(VaultItem item, CancellationToken cancellationToken = default)
+    {
+        var created = item with { Id = $"item-{++_sequence:D3}" };
+        _items[created.Id] = created;
+        return Task.FromResult(created);
+    }
+
+    public Task DeleteItemAsync(string id, bool permanent = false, CancellationToken cancellationToken = default)
+    {
+        _items.Remove(id);
+        return Task.CompletedTask;
+    }
+
+    public Task<VaultFolder> CreateFolderAsync(string name, CancellationToken cancellationToken = default)
+    {
+        var folder = AddFolder(name);
+        return Task.FromResult(folder);
+    }
+
+    public Task<VaultFolder> RenameFolderAsync(string id, string name, CancellationToken cancellationToken = default)
+    {
+        var renamed = _folders[id] with { Name = name };
+        _folders[id] = renamed;
+        return Task.FromResult(renamed);
+    }
+
+    public Task DeleteFolderAsync(string id, CancellationToken cancellationToken = default)
+    {
+        _folders.Remove(id);
+        foreach (var (key, item) in _items.Where(kv => kv.Value.FolderId == id).ToList())
+            _items[key] = item with { FolderId = null };
+        return Task.CompletedTask;
+    }
+
+    public Task<UnlockResult> UnlockAsync(string masterPassword, CancellationToken cancellationToken = default) =>
+        Task.FromResult(UnlockResult.Success());
+
+    public Task LockAsync(CancellationToken cancellationToken = default) => Task.CompletedTask;
+}