RestoreFromState wipes the whole history and returns true when the state is built from service.Commands / service.SaveBoundaries

[matt-edmondson]

What's wrong

UndoRedoService.RestoreFromState (UndoRedo/Services/UndoRedoService.cs, ~lines 386-430) validates the state, then clears the live stack and save boundaries, and only afterwards enumerates state.Commands and state.SaveBoundaries:

_stackManager.Clear();
_saveBoundaryManager.Clear();

foreach (ICommand command in state.Commands) { ... }          // already empty
...
foreach (SaveBoundary boundary in state.SaveBoundaries) { ... } // already empty
return true;

But the public IUndoRedoService.Commands and IUndoRedoService.SaveBoundaries are live views, not copies:

• StackManager.cs:15 — Commands => _commands.AsReadOnly()
• SaveBoundaryManager.cs:20 — SaveBoundaries => _saveBoundaries.AsReadOnly()

UndoRedoStackState is a plain record that stores whatever IReadOnlyList it is given, so a state built from those properties aliases the lists that Clear() empties.

Failure scenario

// 3 commands executed, then MarkAsSaved()
var state = new UndoRedoStackState(service.Commands, 0, service.SaveBoundaries, "1.0", DateTime.UtcNow);
bool ok = service.RestoreFromState(state);

• Actual: ok == true, CommandCount == 0, CurrentPosition == -1, no save boundaries. The entire history is silently destroyed while the call reports success.
• Expected: 3 commands, position 0, 1 save boundary.

Variant: copying the commands but passing the live service.SaveBoundaries keeps the commands but drops every save boundary, so HasUnsavedChanges reports true at the saved position.

GetCurrentState() copies its lists, so it is unaffected; the trap is only for callers who assemble a state from the public properties (e.g. "jump to position N, keep the history"), which nothing in the API warns against. Related to, but distinct from, #95 (lazy query results over the live stack).

Reproduced with a scratch MSTest probe on .NET 10: the live-collections case prints ok=True count=0 pos=-1 boundaries=0; the live-boundaries-only case prints ok=True count=3 pos=2 boundaries=0 unsaved=True.

Suggested fix

Snapshot the collections at the top of RestoreFromState, before validation and clearing, and use only the snapshots afterwards:

List<ICommand> commands = [.. state.Commands];
List<SaveBoundary> boundaries = [.. state.SaveBoundaries];

(Optionally also make UndoRedoStackState / the Commands and SaveBoundaries properties return copies, though the local snapshot alone fixes the bug.)

Acceptance criteria

• Restoring a state built from service.Commands and service.SaveBoundaries keeps all commands, lands at the requested position, and keeps the boundaries; HasUnsavedChanges is false at the saved position.
• Regression tests for both scenarios above; existing tests still pass.

[matt-edmondson]

Triage

• Category: Bug
• Priority: Medium. When it triggers, the whole history is silently destroyed while the call reports success. It only triggers for callers who build a state from the live Commands/SaveBoundaries properties rather than GetCurrentState().
• Area / suggested assignment: Service state restore (Services/UndoRedoService.cs RestoreFromState)
• Duplicates: none. The root cause is related to GetChangeVisualizations/GetCommandsToUndo return lazy queries over the live stack, so enumerating after a change reports the current command as not executed #95 (live views leaking out of the stack), but the issue is distinct.
• In progress: not directly. Open PR Return a snapshot from GetCommandsToUndo instead of a live query [patch] #115 fixes GetChangeVisualizations/GetCommandsToUndo return lazy queries over the live stack, so enumerating after a change reports the current command as not executed #95 by snapshotting GetCommandsToUndo, and PR Keep save-boundary timestamps through load and restore [minor] #117 touches save-boundary restore (Save-boundary timestamps are reset to "now" on load, restore and stack trim #91), so coordinate with Keep save-boundary timestamps through load and restore [minor] #117 to avoid conflicts in RestoreFromState.
• Related: Clear(), LoadStateAsync() and RestoreFromState() raise no event, so UIs synced through events show stale CanUndo/CanRedo/unsaved state #114 (RestoreFromState raises no event), which could be fixed in the same pass.

Notes: Snapshot state.Commands and state.SaveBoundaries at the top of RestoreFromState. Longer term, having Commands/SaveBoundaries return copies (or documenting them as live) would close off the whole class alongside #95.

---

Generated by Claude Code