From 60c7e82f48cf1bbc9a031bef0a1ac8dd9f79a6da Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Nov 2026 11:23:51 +0000 Subject: [PATCH 1/2] Roll back an undo or redo whose NavigationChanged handler throws [patch] RestoreState swapped in the restored items and index, then raised NavigationChanged. When a handler threw, SimpleUndoRedoProvider put the action back as though it had never run, but the stack had already moved, so the stack and its history disagreed: after a failed undo the page was undone while Redo stayed disabled and the next undo did nothing visible. RestoreState now puts the stack back to its state before the restore when a handler throws, then rethrows, which keeps the provider's assumption that an action that throws changed nothing. Fixes ktsu-dev/Navigation#75 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW5pmswBjLMVcM1HgMb --- .gitignore | 18 ++++++++ Navigation.Test/NavigationStackTests.cs | 60 +++++++++++++++++++++++++ Navigation/Services/NavigationStack.cs | 18 ++++++++ 3 files changed, 96 insertions(+) diff --git a/.gitignore b/.gitignore index dc0470a..e043c9f 100644 --- a/.gitignore +++ b/.gitignore @@ -203,6 +203,11 @@ PublishScripts/ **/[Pp]ackages/* # except build/, which is used as an MSBuild target. !**/[Pp]ackages/build/ +# and except a Unity project's Packages/, which is source: Unity's package manifest and its +# resolved lock file are both meant to be committed, and a NuGet restore folder never contains +# a file by either name. +!**/[Pp]ackages/manifest.json +!**/[Pp]ackages/packages-lock.json # Uncomment if necessary however generally it will be regenerated when needed #!**/[Pp]ackages/repositories.config # NuGet v3's project.json files produces more ignorable files @@ -651,3 +656,16 @@ Temporary Items # ImGui.ini files imgui.ini + +# Game engine projects +# +# Godot: the import cache, and the mono/temp bin+obj a C# build writes. +.godot/ + +# Unity: .meta files are source, not the Visual Studio C++ build artifact that the `*.meta` rule +# further up targets. Unity generates one per asset and it carries the GUID that scenes, prefabs +# and serialized references point at, so ignoring them gives every clone fresh GUIDs and silently +# breaks those references - including for a plug-in whose .dll is itself a build output. This +# negation has to come after that rule to win, and is scoped to the asset tree so the Visual +# Studio artifact stays ignored everywhere else. +!**/[Aa]ssets/**/*.meta diff --git a/Navigation.Test/NavigationStackTests.cs b/Navigation.Test/NavigationStackTests.cs index 8dd3a65..0cc3cc6 100644 --- a/Navigation.Test/NavigationStackTests.cs +++ b/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(() => _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(() => _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() { diff --git a/Navigation/Services/NavigationStack.cs b/Navigation/Services/NavigationStack.cs index 9725090..8a77b87 100644 --- a/Navigation/Services/NavigationStack.cs +++ b/Navigation/Services/NavigationStack.cs @@ -314,10 +314,17 @@ private static bool IsValidState(List items, int currentIndex) => /// /// The items to restore /// The current index to restore + /// + /// If a 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. + /// internal void RestoreState(IEnumerable items, int currentIndex) { T? previousItem; T? currentItem; + List beforeItems; + int beforeIndex; lock (_sync) { List restored = [.. items]; @@ -332,6 +339,8 @@ internal void RestoreState(IEnumerable items, int currentIndex) } previousItem = CurrentUnlocked; + beforeItems = [.. _items]; + beforeIndex = _currentIndex; ReplaceItems(restored, currentIndex); currentItem = CurrentUnlocked; } @@ -348,6 +357,15 @@ internal void RestoreState(IEnumerable items, int currentIndex) { OnNavigationChanged(NavigationType.NavigateTo, previousItem, currentItem); } + catch + { + lock (_sync) + { + ReplaceItems(beforeItems, beforeIndex); + } + + throw; + } finally { lock (_sync) From 8075cefffefd3257b26f4327e048bf84ec9e81da Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Nov 2026 11:24:13 +0000 Subject: [PATCH 2/2] Leave .gitignore as it was on main Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_017vfJW5pmswBjLMVcM1HgMb --- .gitignore | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/.gitignore b/.gitignore index e043c9f..dc0470a 100644 --- a/.gitignore +++ b/.gitignore @@ -203,11 +203,6 @@ PublishScripts/ **/[Pp]ackages/* # except build/, which is used as an MSBuild target. !**/[Pp]ackages/build/ -# and except a Unity project's Packages/, which is source: Unity's package manifest and its -# resolved lock file are both meant to be committed, and a NuGet restore folder never contains -# a file by either name. -!**/[Pp]ackages/manifest.json -!**/[Pp]ackages/packages-lock.json # Uncomment if necessary however generally it will be regenerated when needed #!**/[Pp]ackages/repositories.config # NuGet v3's project.json files produces more ignorable files @@ -656,16 +651,3 @@ Temporary Items # ImGui.ini files imgui.ini - -# Game engine projects -# -# Godot: the import cache, and the mono/temp bin+obj a C# build writes. -.godot/ - -# Unity: .meta files are source, not the Visual Studio C++ build artifact that the `*.meta` rule -# further up targets. Unity generates one per asset and it carries the GUID that scenes, prefabs -# and serialized references point at, so ignoring them gives every clone fresh GUIDs and silently -# breaks those references - including for a plug-in whose .dll is itself a build output. This -# negation has to come after that rule to win, and is scoped to the asset tree so the Visual -# Studio artifact stays ignored everywhere else. -!**/[Aa]ssets/**/*.meta