Roll back an undo or redo whose NavigationChanged handler throws [patch]

Navigation.Test/NavigationStackTests.cs

@@ -479,6 +479,66 @@ public void Redo_WithAHandlerThatRedirects_KeepsTheRestOfTheRedoHistory()
 		Assert.IsFalse(_undoRedoProvider.CanRedo);
 	}
 
+	[TestMethod]
+	public void Undo_WhenAHandlerThrows_LeavesTheStackAndTheHistoryInAgreement()
+	{
+		// Arrange: a handler that throws once, on the first navigation after the history is built
+		NavigateToABC();
+		bool throwOnce = true;
+		_navigation!.NavigationChanged += (_, _) =>
+		{
+			if (throwOnce)
+			{
+				throwOnce = false;
+				throw new InvalidOperationException("handler failed");
+			}
+		};
+
+		// Act
+		Assert.ThrowsExactly<InvalidOperationException>(() => _undoRedoProvider!.Undo());
+
+		// Assert: the undo did not happen, so the stack is still at C with C's navigation still to undo
+		AssertStack("A,B,C", "C");
+		Assert.HasCount(3, _undoRedoProvider!.GetUndoStack());
+		Assert.IsFalse(_undoRedoProvider.CanRedo);
+
+		// And the next undo goes back to B, as the first one should have
+		Assert.IsTrue(_undoRedoProvider.Undo());
+		AssertStack("A,B", "B");
+		Assert.IsTrue(_undoRedoProvider.CanRedo);
+	}
+
+	[TestMethod]
+	public void Redo_WhenAHandlerThrows_LeavesTheStackAndTheHistoryInAgreement()
+	{
+		// Arrange: undo to B, then a handler that throws once on the redo
+		NavigateToABC();
+		Assert.IsTrue(_undoRedoProvider!.Undo());
+		AssertStack("A,B", "B");
+		bool throwOnce = true;
+		_navigation!.NavigationChanged += (_, _) =>
+		{
+			if (throwOnce)
+			{
+				throwOnce = false;
+				throw new InvalidOperationException("handler failed");
+			}
+		};
+
+		// Act
+		Assert.ThrowsExactly<InvalidOperationException>(() => _undoRedoProvider.Redo());
+
+		// Assert: the redo did not happen, so the stack is still at B with C still to redo
+		AssertStack("A,B", "B");
+		Assert.HasCount(2, _undoRedoProvider.GetUndoStack());
+		Assert.IsTrue(_undoRedoProvider.CanRedo);
+
+		// And the next redo goes forward to C, as the first one should have
+		Assert.IsTrue(_undoRedoProvider.Redo());
+		AssertStack("A,B,C", "C");
+		Assert.IsFalse(_undoRedoProvider.CanRedo);
+	}
+
 	[TestMethod]
 	public void NavigateTo_FromAHandlerOutsideUndo_IsStillRecorded()
 	{

Navigation/Services/NavigationStack.cs

@@ -314,10 +314,17 @@ private static bool IsValidState(List<T> items, int currentIndex) =>
 	/// </summary>
 	/// <param name="items">The items to restore</param>
 	/// <param name="currentIndex">The current index to restore</param>
+	/// <remarks>
+	/// If a <see cref="NavigationChanged"/> handler throws, the stack goes back to the state it had before the
+	/// restore and the exception propagates. The undo/redo provider treats an action that throws as one that
+	/// changed nothing, so leaving the restored state in place would put the stack and its history out of step.
+	/// </remarks>
 	internal void RestoreState(IEnumerable<T> items, int currentIndex)
 	{
 		T? previousItem;
 		T? currentItem;
+		List<T> beforeItems;
+		int beforeIndex;
 		lock (_sync)
 		{
 			List<T> restored = [.. items];
@@ -332,6 +339,8 @@ internal void RestoreState(IEnumerable<T> items, int currentIndex)
 			}
 
 			previousItem = CurrentUnlocked;
+			beforeItems = [.. _items];
+			beforeIndex = _currentIndex;
 			ReplaceItems(restored, currentIndex);
 			currentItem = CurrentUnlocked;
 		}
@@ -348,6 +357,15 @@ internal void RestoreState(IEnumerable<T> items, int currentIndex)
 		{
 			OnNavigationChanged(NavigationType.NavigateTo, previousItem, currentItem);
 		}
+		catch
+		{
+			lock (_sync)
+			{
+				ReplaceItems(beforeItems, beforeIndex);
+			}
+
+			throw;
+		}
 		finally
 		{
 			lock (_sync)