From d3f9c8eb103f1fa8649aaa5658ee933bb56e8698 Mon Sep 17 00:00:00 2001 From: Matthew Edmondson Date: Sun, 27 Sep 2026 06:24:25 +0000 Subject: [PATCH] fix: stop a navigation failure from surfacing after undo/redo is applied [patch] UndoAsync, RedoAsync and UndoToSaveBoundaryAsync applied the change, moved the position and raised the event before navigating, but only caught OperationCanceledException from the navigation provider. Any other provider exception reached the caller after the undo had succeeded, so a caller following the #75 contract would retry and undo a second command. The three call sites now share NavigateSafelyAsync, which treats every navigation failure as non-critical, as it already did for timeouts. Fixes ktsu-dev/UndoRedo#89 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01TX4nouJvSXNs7eSvDP13qU --- UndoRedo.Test/UndoRedoStackTests.cs | 68 +++++++++++++++++++++++++++ UndoRedo/Services/UndoRedoService.cs | 70 +++++++++++++--------------- 2 files changed, 101 insertions(+), 37 deletions(-) diff --git a/UndoRedo.Test/UndoRedoStackTests.cs b/UndoRedo.Test/UndoRedoStackTests.cs index ce23036..e70b1bc 100644 --- a/UndoRedo.Test/UndoRedoStackTests.cs +++ b/UndoRedo.Test/UndoRedoStackTests.cs @@ -538,6 +538,66 @@ public async Task NavigationProvider_CancellationToken_HandlesCorrectly() Assert.IsTrue(navigationProvider.WasCancelled, "Navigation should have been cancelled due to timeout"); } + [TestMethod] + public async Task UndoAsync_NavigationProviderThrows_ReturnsTrueWithUndoApplied() + { + // Arrange + UndoRedoService stack = CreateService(); + stack.SetNavigationProvider(new ThrowingNavigationProvider()); + int value = 0; + stack.Execute(new DelegateCommand("Increment", () => value++, () => value--, navigationContext: "editor")); + + // Act + bool result = await stack.UndoAsync().ConfigureAwait(false); + + // Assert + Assert.IsTrue(result, "UndoAsync should report the undo, which was applied before navigation failed"); + Assert.AreEqual(0, value); + Assert.AreEqual(-1, stack.CurrentPosition); + } + + [TestMethod] + public async Task RedoAsync_NavigationProviderThrows_ReturnsTrueWithRedoApplied() + { + // Arrange + UndoRedoService stack = CreateService(); + stack.SetNavigationProvider(new ThrowingNavigationProvider()); + int value = 0; + stack.Execute(new DelegateCommand("Increment", () => value++, () => value--, navigationContext: "editor")); + await stack.UndoAsync(navigateToChange: false).ConfigureAwait(false); + + // Act + bool result = await stack.RedoAsync().ConfigureAwait(false); + + // Assert + Assert.IsTrue(result, "RedoAsync should report the redo, which was applied before navigation failed"); + Assert.AreEqual(1, value); + Assert.AreEqual(0, stack.CurrentPosition); + } + + [TestMethod] + public async Task UndoToSaveBoundaryAsync_NavigationProviderThrows_ReturnsTrueAtBoundary() + { + // Arrange + UndoRedoService stack = CreateService(); + stack.SetNavigationProvider(new ThrowingNavigationProvider()); + int value = 0; + stack.Execute(new DelegateCommand("Increment", () => value++, () => value--, navigationContext: "editor")); + stack.MarkAsSaved(); + stack.Execute(new DelegateCommand("Increment", () => value++, () => value--, navigationContext: "editor")); + stack.Execute(new DelegateCommand("Increment", () => value++, () => value--, navigationContext: "editor")); + SaveBoundary boundary = stack.SaveBoundaries[0]; + + // Act + bool result = await stack.UndoToSaveBoundaryAsync(boundary).ConfigureAwait(false); + + // Assert + Assert.IsTrue(result, "UndoToSaveBoundaryAsync should report the undo, which was applied before navigation failed"); + Assert.AreEqual(1, value); + Assert.AreEqual(0, stack.CurrentPosition); + Assert.IsFalse(stack.HasUnsavedChanges, "The stack should be back at the save boundary"); + } + [TestMethod] public void Execute_CommandThrowsException_DoesNotCorruptStack() { @@ -1049,6 +1109,14 @@ public override ICommand MergeWith(ICommand other) } } + private sealed class ThrowingNavigationProvider : INavigationProvider + { + public Task NavigateToAsync(string context, CancellationToken cancellationToken = default) => + throw new InvalidOperationException("editor closed"); + + public bool IsValidContext(string context) => true; + } + private sealed class SlowNavigationProvider : INavigationProvider { public bool WasCancelled { get; private set; } diff --git a/UndoRedo/Services/UndoRedoService.cs b/UndoRedo/Services/UndoRedoService.cs index 99b0f7a..152b1c2 100644 --- a/UndoRedo/Services/UndoRedoService.cs +++ b/UndoRedo/Services/UndoRedoService.cs @@ -165,19 +165,9 @@ public async Task UndoAsync(bool navigateToChange = true, CancellationToke _stackManager.MovePrevious(); CommandUndone?.Invoke(this, new CommandUndoneEventArgs(command, _stackManager.CurrentPosition)); - if (navigateToChange && _options.EnableNavigation && _navigationProvider != null && !string.IsNullOrEmpty(command.NavigationContext)) + if (navigateToChange) { - using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - cts.CancelAfter(_options.EffectiveNavigationTimeout); - - try - { - await _navigationProvider.NavigateToAsync(command.NavigationContext!, cts.Token).ConfigureAwait(false); - } - catch (OperationCanceledException) - { - // Navigation timeout or cancellation - not critical - } + await NavigateSafelyAsync(command.NavigationContext, cancellationToken).ConfigureAwait(false); } return true; @@ -201,19 +191,9 @@ public async Task RedoAsync(bool navigateToChange = true, CancellationToke _stackManager.MoveNext(); CommandRedone?.Invoke(this, new CommandRedoneEventArgs(command, _stackManager.CurrentPosition)); - if (navigateToChange && _options.EnableNavigation && _navigationProvider != null && !string.IsNullOrEmpty(command.NavigationContext)) + if (navigateToChange) { - using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - cts.CancelAfter(_options.EffectiveNavigationTimeout); - - try - { - await _navigationProvider.NavigateToAsync(command.NavigationContext!, cts.Token).ConfigureAwait(false); - } - catch (OperationCanceledException) - { - // Navigation timeout or cancellation - not critical - } + await NavigateSafelyAsync(command.NavigationContext, cancellationToken).ConfigureAwait(false); } return true; @@ -266,25 +246,41 @@ public async Task UndoToSaveBoundaryAsync(SaveBoundary saveBoundary, bool CommandUndone?.Invoke(this, new CommandUndoneEventArgs(command, _stackManager.CurrentPosition)); } - if (navigateToLastChange && lastCommand != null && _options.EnableNavigation && _navigationProvider != null && - !string.IsNullOrEmpty(lastCommand.NavigationContext)) + if (navigateToLastChange && lastCommand != null) { - using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); - cts.CancelAfter(_options.EffectiveNavigationTimeout); - - try - { - await _navigationProvider.NavigateToAsync(lastCommand.NavigationContext!, cts.Token).ConfigureAwait(false); - } - catch (OperationCanceledException) - { - // Navigation timeout or cancellation - not critical - } + await NavigateSafelyAsync(lastCommand.NavigationContext, cancellationToken).ConfigureAwait(false); } return true; } + /// + /// Navigates to where a change was made, after the undo or redo has already been applied. + /// Navigation is best effort: any failure is swallowed, because an exception here would tell the + /// caller the undo or redo did not happen when it did, and a retry would then undo a second command. + /// + private async Task NavigateSafelyAsync(string? navigationContext, CancellationToken cancellationToken) + { + if (!_options.EnableNavigation || _navigationProvider == null || string.IsNullOrEmpty(navigationContext)) + { + return; + } + + using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + cts.CancelAfter(_options.EffectiveNavigationTimeout); + + try + { + await _navigationProvider.NavigateToAsync(navigationContext!, cts.Token).ConfigureAwait(false); + } +#pragma warning disable CA1031 // Do not catch general exception types + catch (Exception) + { + // Navigation timeout, cancellation or provider failure - not critical + } +#pragma warning restore CA1031 // Do not catch general exception types + } + /// public IEnumerable GetChangeVisualizations(int maxItems = 50) {