diff --git a/BitwardenSharp.slnx b/BitwardenSharp.slnx index df6d3ad..f966acb 100644 --- a/BitwardenSharp.slnx +++ b/BitwardenSharp.slnx @@ -12,6 +12,7 @@ + diff --git a/src/Presentation/Desktop/ViewModels/ItemViewModel.cs b/src/Presentation/Desktop/ViewModels/ItemViewModel.cs index ed94422..ac479ea 100644 --- a/src/Presentation/Desktop/ViewModels/ItemViewModel.cs +++ b/src/Presentation/Desktop/ViewModels/ItemViewModel.cs @@ -104,6 +104,16 @@ public async Task LoadIconAsync(IconLoader loader, CancellationToken cancellatio Icon = await loader.GetAsync(IconDomain, cancellationToken); } + /// + /// 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. + /// + [ObservableProperty] + [NotifyPropertyChangedFor(nameof(RowOpacity))] + private bool _isPending; + + public double RowOpacity => IsPending ? 0.45 : 1.0; + // ── reveal ─────────────────────────────────────────────────────────────────────────────── /// diff --git a/src/Presentation/Desktop/ViewModels/VaultViewModel.cs b/src/Presentation/Desktop/ViewModels/VaultViewModel.cs index 5bb6d2d..a870792 100644 --- a/src/Presentation/Desktop/ViewModels/VaultViewModel.cs +++ b/src/Presentation/Desktop/ViewModels/VaultViewModel.cs @@ -52,13 +52,22 @@ public sealed partial class VaultViewModel( // ── loading ────────────────────────────────────────────────────────────────────────────── - public async Task LoadAsync() + /// + /// Reads folders and items and rebuilds the view. + /// + /// + /// 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. + /// + 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() } /// Reloads folders and items while keeping the selected folder path selected. - 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() /// 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!)); } - /// Moves items into a folder. Called by the view when a drag is dropped on the tree. + /// + /// Moves items into a folder. Called by the view when a drag is dropped on the tree. + /// + /// + /// 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. + /// public async Task MoveItemsToFolderAsync(IReadOnlyList itemIds, FolderNode target) { if (itemIds.Count == 0) return; @@ -270,8 +306,14 @@ public async Task MoveItemsToFolderAsync(IReadOnlyList 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; } /// Moves a folder under another. Called by the view on a folder-to-folder drop. @@ -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> operation) + /// Runs a folder operation and reloads on success. Returns whether it succeeded. + private async Task RunFolderOperationAsync(Func> operation) { IsBusy = true; Error = null; @@ -294,13 +337,18 @@ private async Task RunFolderOperationAsync(Func> 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 { diff --git a/src/Presentation/Desktop/Views/VaultView.axaml b/src/Presentation/Desktop/Views/VaultView.axaml index f117043..85fe102 100644 --- a/src/Presentation/Desktop/Views/VaultView.axaml +++ b/src/Presentation/Desktop/Views/VaultView.axaml @@ -124,8 +124,11 @@ + diff --git a/tests/Desktop.Tests/BitwardenSharp.Desktop.Tests.csproj b/tests/Desktop.Tests/BitwardenSharp.Desktop.Tests.csproj new file mode 100644 index 0000000..1f3aae2 --- /dev/null +++ b/tests/Desktop.Tests/BitwardenSharp.Desktop.Tests.csproj @@ -0,0 +1,15 @@ + + + false + + + + + + + + + + + + diff --git a/tests/Desktop.Tests/FakeVault.cs b/tests/Desktop.Tests/FakeVault.cs new file mode 100644 index 0000000..a44c5c5 --- /dev/null +++ b/tests/Desktop.Tests/FakeVault.cs @@ -0,0 +1,114 @@ +using BitwardenSharp.Application.Abstractions; +using BitwardenSharp.Domain.Vault; + +namespace BitwardenSharp.Desktop.Tests; + +/// +/// 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. +/// +internal sealed class FakeVault : IVaultClient, IVaultSession +{ + private readonly Dictionary _items = []; + private readonly Dictionary _folders = []; + private int _sequence; + + public int SyncCount { get; private set; } + + /// Set to make the next write fail, for testing rollback. + 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 GetStatusAsync(CancellationToken cancellationToken = default) => + Task.FromResult(new VaultStatus { Status = "unlocked", UserEmail = "t@example.com" }); + + public Task> GetItemsAsync(CancellationToken cancellationToken = default) => + Task.FromResult>(_items.Values.ToList()); + + public Task> GetFoldersAsync(CancellationToken cancellationToken = default) => + Task.FromResult>(_folders.Values.ToList()); + + public Task GetItemAsync(string id, CancellationToken cancellationToken = default) => + Task.FromResult(_items[id]); + + public Task 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 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 CreateFolderAsync(string name, CancellationToken cancellationToken = default) + { + var folder = AddFolder(name); + return Task.FromResult(folder); + } + + public Task 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 UnlockAsync(string masterPassword, CancellationToken cancellationToken = default) => + Task.FromResult(UnlockResult.Success()); + + public Task LockAsync(CancellationToken cancellationToken = default) => Task.CompletedTask; +} diff --git a/tests/Desktop.Tests/VaultViewModelSpecs.cs b/tests/Desktop.Tests/VaultViewModelSpecs.cs new file mode 100644 index 0000000..baff911 --- /dev/null +++ b/tests/Desktop.Tests/VaultViewModelSpecs.cs @@ -0,0 +1,213 @@ +using BitwardenSharp.Application.Abstractions; +using BitwardenSharp.Application.Folders; +using BitwardenSharp.Desktop.Services; +using BitwardenSharp.Desktop.ViewModels; +using NSubstitute; +using Shouldly; +using Xunit; + +namespace BitwardenSharp.Desktop.Tests; + +public class VaultViewModelSpecs +{ + private static (VaultViewModel Vm, FakeVault Vault) Build() + { + var vault = new FakeVault(); + var icons = Substitute.For(); + icons.IsEnabled.Returns(false); + var vm = new VaultViewModel(vault, vault, new FolderService(vault), new IconLoader(icons)); + return (vm, vault); + } + + /// Two folders and an item in each, plus one unfiled. + private static async Task<(VaultViewModel Vm, FakeVault Vault)> LoadedAsync() + { + var (vm, vault) = Build(); + var home = vault.AddFolder("Homelab"); + vault.AddFolder("Homelab/Proxmox"); + var finance = vault.AddFolder("Finance"); + + vault.AddItem("NUC", home.Id, "https://10.0.0.11/"); + vault.AddItem("Bank", finance.Id, "https://bank.example/"); + vault.AddItem("Loose", null, "https://loose.example/"); + + await vm.LoadAsync(); + return (vm, vault); + } + + [Fact] + public async Task The_tree_reflects_the_slash_separated_folder_names() + { + var (vm, _) = await LoadedAsync(); + + vm.Folders.Select(f => f.Name).ShouldBe(["Finance", "Homelab", "No folder"], ignoreOrder: true); + vm.Folders.Single(f => f.Name == "Homelab").Children.Single().Name.ShouldBe("Proxmox"); + } + + // ── #2: expand/collapse state must survive an operation ────────────────────────────────── + + [Fact] + public async Task Collapsing_a_node_survives_a_reload() + { + var (vm, _) = await LoadedAsync(); + var homelab = vm.Folders.Single(f => f.Name == "Homelab"); + homelab.IsExpanded = false; + + await vm.RefreshCommand.ExecuteAsync(null); + + vm.Folders.Single(f => f.Name == "Homelab").IsExpanded + .ShouldBeFalse("a reload changed what is in the tree, not how the user is viewing it"); + } + + [Fact] + public async Task Collapsing_a_node_survives_a_move() + { + var (vm, _) = await LoadedAsync(); + vm.Folders.Single(f => f.Name == "Homelab").IsExpanded = false; + + var finance = vm.Folders.Single(f => f.Name == "Finance"); + var loose = vm.Items.Single(i => i.Name == "Loose"); + await vm.MoveItemsToFolderAsync([loose.Id], finance); + + vm.Folders.Single(f => f.Name == "Homelab").IsExpanded.ShouldBeFalse(); + } + + // ── #3: the item list must not keep showing what was moved away ────────────────────────── + + [Fact] + public async Task An_item_moved_out_of_the_viewed_folder_leaves_the_list() + { + var (vm, _) = await LoadedAsync(); + + vm.SelectedFolder = vm.Folders.Single(f => f.Name == "Finance"); + vm.Items.Select(i => i.Name).ShouldBe(["Bank"]); + + var bank = vm.Items.Single(); + var homelab = vm.Folders.Single(f => f.Name == "Homelab"); + await vm.MoveItemsToFolderAsync([bank.Id], homelab); + + vm.Items.Select(i => i.Name).ShouldBeEmpty("Bank is no longer in Finance"); + } + + [Fact] + public async Task The_selected_folder_stays_selected_across_a_move() + { + var (vm, _) = await LoadedAsync(); + vm.SelectedFolder = vm.Folders.Single(f => f.Name == "Finance"); + + var bank = vm.Items.Single(); + await vm.MoveItemsToFolderAsync([bank.Id], vm.Folders.Single(f => f.Name == "Homelab")); + + vm.SelectedFolder.ShouldNotBeNull().Path.ShouldBe("Finance"); + } + + [Fact] + public async Task An_item_moved_into_the_viewed_folder_appears_in_the_list() + { + var (vm, _) = await LoadedAsync(); + var finance = vm.Folders.Single(f => f.Name == "Finance"); + vm.SelectedFolder = finance; + + // The loose item is not visible while Finance is selected, so move it by id. + await vm.LoadAsync(); + vm.SelectedFolder = null; + var loose = vm.Items.Single(i => i.Name == "Loose"); + vm.SelectedFolder = vm.Folders.Single(f => f.Name == "Finance"); + + await vm.MoveItemsToFolderAsync([loose.Id], vm.Folders.Single(f => f.Name == "Finance")); + + vm.Items.Select(i => i.Name).ShouldBe(["Bank", "Loose"], ignoreOrder: true); + } + + [Fact] + public async Task Folder_counts_update_after_a_move() + { + var (vm, _) = await LoadedAsync(); + var finance = vm.Folders.Single(f => f.Name == "Finance"); + finance.TotalCount.ShouldBe(1); + + var loose = vm.Items.Single(i => i.Name == "Loose"); + await vm.MoveItemsToFolderAsync([loose.Id], finance); + + vm.Folders.Single(f => f.Name == "Finance").TotalCount.ShouldBe(2); + } + + // ── #3's actual cause: syncing after our own write can pull back the pre-write state ────── + + /// + /// Regression. The view-model logic was never wrong — with an instant-write vault the list + /// updated correctly all along. The staleness came from LoadAsync starting with a server sync, + /// which immediately after our own write can return state the server has not applied yet. + /// + [Fact] + public async Task Reloading_after_a_move_does_not_sync() + { + var (vm, vault) = await LoadedAsync(); + var before = vault.SyncCount; + + var loose = vm.Items.Single(i => i.Name == "Loose"); + await vm.MoveItemsToFolderAsync([loose.Id], vm.Folders.Single(f => f.Name == "Finance")); + + vault.SyncCount.ShouldBe(before, "a sync here can only re-fetch pre-write state"); + } + + [Fact] + public async Task An_explicit_refresh_still_syncs() + { + var (vm, vault) = await LoadedAsync(); + var before = vault.SyncCount; + + await vm.RefreshCommand.ExecuteAsync(null); + + vault.SyncCount.ShouldBe(before + 1, "Refresh exists to pick up changes made elsewhere"); + } + + // ── #4: optimistic feedback, and rollback when the write fails ──────────────────────────── + + [Fact] + public async Task A_failed_move_leaves_the_item_in_place_and_undimmed() + { + var (vm, vault) = await LoadedAsync(); + vault.FailNextWrite = true; + + var loose = vm.Items.Single(i => i.Name == "Loose"); + await vm.MoveItemsToFolderAsync([loose.Id], vm.Folders.Single(f => f.Name == "Finance")); + + vm.Error.ShouldNotBeNull(); + vm.Items.ShouldContain(i => i.Name == "Loose"); + vm.Items.Single(i => i.Name == "Loose").IsPending + .ShouldBeFalse("the dimming is rolled back when the write fails"); + } + + [Fact] + public async Task Dropping_onto_an_implied_folder_segment_is_refused_without_writing() + { + var (vm, vault) = await ImpliedSegmentAsync(); + var before = vault.SyncCount; + + // "Homelab" exists here only as a path segment of "Homelab/Proxmox" — there is no folder + // to move into, so this must be refused rather than silently doing nothing. + var implied = vm.Folders.Single(f => f.Name == "Homelab"); + implied.IsRealFolder.ShouldBeFalse(); + + await vm.MoveItemsToFolderAsync([vm.Items.First().Id], implied); + + vm.Error.ShouldNotBeNull().ShouldContain("isn't a real folder"); + vault.SyncCount.ShouldBe(before); + } + + private static async Task<(VaultViewModel Vm, FakeVault Vault)> ImpliedSegmentAsync() + { + var vault = new FakeVault(); + var icons = Substitute.For(); + icons.IsEnabled.Returns(false); + var vm = new VaultViewModel(vault, vault, new FolderService(vault), new IconLoader(icons)); + + // Only the child folder exists; "Homelab" is implied by the name. + var proxmox = vault.AddFolder("Homelab/Proxmox"); + vault.AddItem("NUC", proxmox.Id, "https://10.0.0.11/"); + + await vm.LoadAsync(); + return (vm, vault); + } +}