Skip to content

Add the /undo and /redo picker, with the cascade warning (part 2 of #944) - #1344

Open
KazenDev wants to merge 2 commits into
CodebuffAI:mainfrom
KazenDev:feat/undo-history-ui
Open

Add the /undo and /redo picker, with the cascade warning (part 2 of #944)#1344
KazenDev wants to merge 2 commits into
CodebuffAI:mainfrom
KazenDev:feat/undo-history-ui

Conversation

@KazenDev

@KazenDev KazenDev commented Sep 13, 2026

Copy link
Copy Markdown

Second half of #944 (the /undo and /redo feature): the picker, the commands, and the two review points that only exist on this side of the split — the race in the finally, and the warning the picker owes the user.

Stacked on #1343. The four engine files (undo-snapshot.ts, undo-store.ts and their two test files) also appear in this diff and are unchanged here — they belong to that PR, which the review asked for first. Read #1343 first; the diff that belongs to this PR is the other 17 files. When #1343 lands, this rebases onto main and those four files drop out of the diff on their own.

Not a rebase of #944: the history rewrite destroyed its base commit, so the UI half was re-applied on current main by a three-way merge against the original commit.

What this is

17 files, +1302/-16. Of the three files added here that are not new logic (undo-history-screen.tsx, command-result-block.tsx, undo-history-store.ts), 569 lines are the original UI, carried over.

File +/- How it got here
cli/src/components/undo-history-screen.tsx +414 378 ported, 36 from this PR
cli/src/components/blocks/command-result-block.tsx +159 ported as-is
cli/src/state/undo-history-store.ts +32 ported as-is
cli/src/app.tsx +112/-1 three-way merge
cli/src/commands/command-registry.ts +103/-1 three-way merge
cli/src/hooks/use-send-message.ts +75/-1 three-way merge, 26 from this PR
cli/src/utils/settings.ts +20 three-way merge
cli/src/data/slash-commands.ts +10/-10 three-way merge
cli/src/chat.tsx +9 three-way merge
cli/src/types/chat.ts +3 three-way merge
cli/src/components/message-with-agents.tsx +12/-1 clean apply
cli/src/utils/message-history.ts +10/-1 clean apply
cli/src/components/message-block.tsx +7/-1 clean apply
cli/src/utils/active-run.ts +4 clean apply
cli/src/state/undo-guards.ts +90 new in this PR
cli/src/state/__tests__/undo-guards.test.ts +104 new in this PR
cli/src/components/__tests__/undo-history-screen.test.tsx +138 new in this PR

Of the seven files main had moved, three merged clean and four conflicted; all seven conflicts were the same shape — both sides added something at the same point — and were resolved by keeping both. The one that needed a decision was use-send-message.ts: the pre-turn snapshot has to be captured before client.run(), so the two sides could not just be concatenated.

1. The picker now says how far back the undo reaches

The review: "jumping back via /undo reverts everything newer … users need to be warned about clearly in the picker (right now it's only in a code comment, not obviously surfaced in the UI copy for multi-turn jumps)."

It was only in a code comment. Now the files panel carries the notice for the focused row, before Enter:

  ┌──────────────────────────────────────────────┐
  │ Files (4)                                    │
  │   • src/three.ts                             │
  │   • src/shared.ts                            │
  │   • src/two.ts                               │
  │   • src/one.ts                               │
  │   ⚠ Also reverts 2 newer changes             │
  └──────────────────────────────────────────────┘

Frame rendered by the component test, trimmed to the panel.

Two things had to be true for that line to be honest, and neither was:

  • The count. The list is newest-first, so the position of the focused row is the number of turns that go with it. newerTurnCount returns it; the same call gives the redo picker its counterpart ("⚠ Also discards 2 newer redos"), because a redo jump invalidates the newer redo entries the same way.
  • The files. The panel listed only the focused entry's files while the action reverts the focused turn and every newer one — a warning about blast radius, above an understated file list. The panel now shows the union undoToRecord actually reverts (filesTakenBack), and for a redo the turns it brings back (filesRestored). shared.ts, touched by three turns, is listed once.

This is not a nicety in this feature. Claude Code's /rewind is the same shape — restore the picked checkpoint, reverting everything after it — and its blast radius is the single most-reported complaint about it: #64615 (open) asks for exactly "a blast-radius preview (file count + list …) before applying a code revert", after #50897 and #27387 were closed as not planned; one user patched the binary to move the destructive option off the default. The same tracker has #34368, "Rewind offers to undo file deletions it cannot actually restore" — the hole fix 3 of #1343 closes here.

2. A turn that was replaced no longer records

The review: "recordUndoEntry in the finally block runs regardless of whether the run was superseded by a newer one; interleaved concurrent turns could record entries against the wrong snapshot state."

Reproduced path:

  1. Turn A captures its snapshot before client.run().
  2. The user interrupts. On Esc the chain lock is released before A's finally finishes — the code says so in the block below this one.
  3. Turn B starts and edits files.
  4. A's finally calls patchSnapshot, which diffs the worktree as it is now: B's files are in that diff.
  5. The journal records B's work as A's. /undo on that entry reverts it, and reports it as the change the user selected.

The fix is a stamp, not a lock: the turn stamps itself when it captures its snapshot (beginUndoTurn), and records only while its stamp is still the newest for that chat (isLatestUndoTurn) — checked before the diff, and again after it, which is the await where a newer turn can appear. Past that point the turn is dropped rather than recorded wrong, which matches the best-effort rule the rest of the feature already follows: a failure disables undo for that turn instead of breaking the chat.

Stamps are per chat, so switching chats mid-run cannot invalidate a turn, and a turn that started in another chat still records into its own. In memory only — undo.json does not change shape.

Where each point of the review is answered

Review point Answer
Split it into a smaller first PR #1343 is the engine half; this is the UI half
Does the snapshot store grow without bound? #1343: anchor, release on drop, and a sweep per project per process
Race: recordUndoEntry runs even if the turn was replaced This PR, section 2 — the stamp, with a test that fails without it
The picker should say the undo cascades This PR, section 1 — the notice, and the file list it was understating

Not in this PR, on purpose

  • Reverting the conversation too — and making that the default — is a follow-up. As it stands, this PR reverts files while the model keeps its memory of the turn, which is what makes the agent write back a file the undo just removed (observed live: • Create new.py/undonew.py deleted → the next turn "the earlier write may not have completed — let me create it again"). Restoring the conversation, staging that cut so /redo can lift it, and offering code only vs code and conversation the way Claude Code and OpenCode do is the next PR, stacked on this one — and that is also where the removed prompt goes back into the input for editing, so the default here is expected to change.
  • No confirmation gate. The notice is shown before Enter, but Enter still runs it. Claude Code's report asks for both a preview and a confirmation ("continue? (y/N)"); gating a destructive action is a product decision with its own UX, so it is a follow-up rather than a silent addition here.
  • The panel does not yet mark which file gets restored and which gets removed. The union is listed; the / 🗑 distinction appears only in the confirmation, after the fact. That is a redesign of the row, and it belongs with the confirmation gate above.
  • The skipped count Add the /undo snapshot engine, anchored so it survives cleanup (part 1 of #944) #1343 deferred to this half is still not reported. That note says the counter has to come out of revertFiles, which is an engine file this PR claims not to touch. Adding it here would make that claim false; it stays tracked, and the summary keeps being silent rather than wrong.
  • The engine's own copy is untouched, including Undid 3 change(s) back to: … — the pluralisation fix is an edit to undo-store.ts, same reason.
  • Nothing about the finally's ordering with releaseRunOwnership changed. The stamp answers the race the review named; whether an interrupted turn should record at all is a separate question.

Verification

Public CI does not run this suite. Both columns are the same command, on the same machine, against the worktree with the steps above removed and re-added — not against memory:

without this PR's changes with
CLI suite (bun test cli/src) 3053 pass, 74 fail, 201 files 3069 pass, 74 fail, 203 files
Failures identical set (diff after normalising timings is empty)
This PR's tests 16 pass, 0 fail
CLI typecheck 10 errors 10 errors, none in undo

+16 pass and +2 files is exactly the 16 tests added here. The 74 failures are pre-existing and unrelated (release wrapper, model picker, prompts).

The UI assertions fail without the change: with the picker reverted to the ported version, 3 of its 5 tests go red

(fail) lists the files of every turn the undo takes with it
(fail) says how many newer turns an undo also reverts
(fail) warns that a redo discards the newer redo actions

The two that stay green either way are the absence assertions ("says nothing when there is nothing newer"), which is expected.

How to try it

bun test cli/src/components/__tests__/undo-history-screen.test.tsx \
         cli/src/state/__tests__/undo-guards.test.ts
bun run --cwd cli typecheck

Manual walkthrough, from #944's validation comment:

  1. Start the CLI on a scratch project that is a git repository. /undo needs a git root (the snapshot repo borrows its objects), and the picker only opens once the journal has an entry — and a turn that changed no files leaves none.
  2. Ask the agent to modify, create, and delete files in a single turn. Do it twice more so the journal holds three turns.
  3. Run /undo — the picker opens. The newest entry shows nothing extra; move down with the arrows and the notice appears ("⚠ Also reverts 2 newer changes") while the file panel grows to the union of the turns that go with it.
  4. Enter reverts the selected turn and every newer one, and the confirmation shows the diff stat.
  5. Run /redo — its picker warns "⚠ Also discards N newer redos" for the same reason.
  6. Quit and reopen the CLI on the same project; /undo still lists the history.

Snapshot capture, restore, and the per-chat journal. No UI yet: the
picker, the command registry entry, and the send-message hook come in
the follow-up.

Three data-loss paths closed, each with a test that fails without it:
- a failed `git add` no longer hands back the tree of a stale index
- every entry is anchored (`refs/freebuff/undo/<chat>/<hash>`) so the
  cleanup job's prune cannot collect a snapshot the journal still lists
- reverting checks the content, not just the tree: `git checkout`
  deletes a file whose blob it cannot read, and `ls-tree` still lists it

The journal is the root set: refs are released when the entry is
dropped, and `sweepAnchors` collects what a deleted chat left behind.

Verified: 20 tests pass, the CLI typecheck has no new errors, and the
full suite matches its baseline (+20 pass, same 61 pre-existing failures).
…odebuffAI#944)

The UI half: the picker, the commands, and the send-path hook. The
snapshot engine (part 1) is CodebuffAI#1343 and is not touched here.

Two review points from CodebuffAI#944, each with a test that fails without its fix:

- The picker warned about the cascade only in a code comment. The files
  panel now says how many newer turns go with the selected one, and lists
  the union of the files they touch rather than the selected turn's alone.
- recordUndoEntry ran in the finally even when a newer turn had started
  (an interrupt releases the chain lock first), so its diff picked up that
  turn's files. A turn now stamps itself when it starts and records only
  while its stamp is still the newest for that chat.

Verified: 16 new tests, the CLI suite at 3069 pass with the same 74
pre-existing failures as the port alone, and a typecheck with no undo
errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant