What's wrong
UndoRedoService.UndoAsync (UndoRedo/Services/UndoRedoService.cs ~173) does three things before it navigates:
- applies
command.Undo()
- moves the position
- raises
CommandUndone
Only after that does it await _navigationProvider.NavigateToAsync(...). The only exception it catches around that call is OperationCanceledException. Any other exception from the navigation provider reaches the caller even though the undo has already succeeded. An example is an InvalidOperationException because the target editor or document was closed.
RedoAsync (~214) and UndoToSaveBoundaryAsync use the same pattern.
Why it matters
Since #75, the contract is that a throwing Undo leaves the position unchanged. A caller who sees an exception from UndoAsync will reasonably assume the undo did not happen and retry. The retry undoes a second command, and the user loses an edit they did not ask to undo. The existing catch comment already calls navigation "not critical", so a navigation failure should not look like an undo failure.
Repro (reproduced)
sealed class ThrowingNav : INavigationProvider {
public Task<bool> NavigateToAsync(string c, CancellationToken t = default) => throw new InvalidOperationException("editor closed");
public bool IsValidContext(string c) => true;
}
UndoRedoService s = new(new StackManager(), new SaveBoundaryManager(), new CommandMerger(), null, new ThrowingNav());
int v = 0;
s.Execute(new DelegateCommand("inc", () => v++, () => v--, navigationContext: "x"));
await s.UndoAsync(); // throws InvalidOperationException
// Observed: exception thrown, but v == 0 and CurrentPosition == -1 (the undo was applied).
// Expected: UndoAsync returns true; the navigation failure is swallowed or reported separately.Suggested fix
Replace the three navigation call sites with one helper, NavigateSafelyAsync(context, ct). The helper should catch non-fatal exceptions from NavigateToAsync, the same way cancellation and timeouts are already handled, and either swallow them or report them through an event or result. It must not throw after the state has changed.
Acceptance: with a throwing navigation provider, UndoAsync, RedoAsync and UndoToSaveBoundaryAsync return normally and the stack state is correct. Regression tests cover all three.
What's wrong
UndoRedoService.UndoAsync(UndoRedo/Services/UndoRedoService.cs~173) does three things before it navigates:command.Undo()CommandUndoneOnly after that does it
await _navigationProvider.NavigateToAsync(...). The only exception it catches around that call isOperationCanceledException. Any other exception from the navigation provider reaches the caller even though the undo has already succeeded. An example is anInvalidOperationExceptionbecause the target editor or document was closed.RedoAsync(~214) andUndoToSaveBoundaryAsyncuse the same pattern.Why it matters
Since #75, the contract is that a throwing
Undoleaves the position unchanged. A caller who sees an exception fromUndoAsyncwill reasonably assume the undo did not happen and retry. The retry undoes a second command, and the user loses an edit they did not ask to undo. The existing catch comment already calls navigation "not critical", so a navigation failure should not look like an undo failure.Repro (reproduced)
Suggested fix
Replace the three navigation call sites with one helper,
NavigateSafelyAsync(context, ct). The helper should catch non-fatal exceptions fromNavigateToAsync, the same way cancellation and timeouts are already handled, and either swallow them or report them through an event or result. It must not throw after the state has changed.Acceptance: with a throwing navigation provider,
UndoAsync,RedoAsyncandUndoToSaveBoundaryAsyncreturn normally and the stack state is correct. Regression tests cover all three.