Reviewed and merged GitHub PR #831Public

Agent

Model

Claude Opus 4.5 (latest)

Started

Jan 18, 11:15 PM

Messages84

Downloads0

Summary

Closed, Completed

Understand
Read PR details, comments, commits, and changed files.
Understand
Understand
Identified and reviewed linked issues.
Understand
Understand
Analyzed code diffs and relevant source files.
Understand
Understand
Checked changelogs and documentation.
Understand
Verify
Ran checks on the PR.
Verify

You are given one or more GitHub PR URLs: https://github.com/badlogic/pi-mono/pull/831

For each PR URL, do the following in order:

Read the PR page in full. Include description, all comments, all commits, and all changed files.
Identify any linked issues referenced in the PR body, comments, commit messages, or cross links. Read each issue in full, including all comments.
Analyze the PR diff. Read all relevant code files in full with no truncation from the current main branch and compare against the diff. Do not fetch PR file blobs unless a file is missing on main or the diff context is insufficient. Include related code paths that are not in the diff but are required to validate behavior.
Check for a changelog entry in the relevant packages/*/CHANGELOG.md files. Report whether an entry exists. If missing, state that a changelog entry is required before merge and that you will add it if the user decides to merge. Follow the changelog format rules in AGENTS.md. Verify:
- Entry uses correct section (### Breaking Changes, ### Added, ### Fixed, etc.)
- External contributions include PR link and author: Fixed foo ([#123](https://github.com/badlogic/pi-mono/pull/123) by [@user](https://github.com/user))
- Breaking changes are in ### Breaking Changes, not just ### Fixed
Check if packages/coding-agent/README.md, packages/coding-agent/docs/.md, packages/coding-agent/examples/**/.md require modification. This is usually the case when existing features have been changed, or new features have been added.
Provide a structured review with these sections:
- Good: solid choices or improvements
- Bad: concrete issues, regressions, missing tests, or risks
- Ugly: subtle or high impact problems
Add Questions or Assumptions if anything is unclear.
Add Change summary and Tests.

Output format per PR: PR: Changelog:

... Good:
... Bad:
... Ugly:
... Questions or Assumptions:
... Change summary:
... Tests:
...

If no issues are found, say so under Bad and Ugly.

Let me read the relevant source files to fully validate the changes:

Now let me check the CHANGELOG for the tui package:

I can see there is no changelog entry for this PR in either the tui or coding-agent packages. Let me also check if there are any linked issues and examine the README.md documentation:

Now let me verify the tests are comprehensive by looking at the test file:

Let me verify the test file on main doesn't already have the undo tests:

Now I have all the information needed for a comprehensive review.

PR: https://github.com/badlogic/pi-mono/pull/831

Changelog:

Missing. No changelog entries exist in packages/tui/CHANGELOG.md or packages/coding-agent/CHANGELOG.md for this PR. Entries are required before merge. I will add them if you decide to merge.

Good:

Comprehensive undo implementation: The PR implements fish-style undo with intelligent coalescing: consecutive word characters coalesce into one undo unit, whitespace/newlines start new units, and cursor movement starts new units. This matches expected shell behavior.
Excellent test coverage: 485 new lines of tests covering all undo scenarios: word coalescing, space-by-space undo, newlines, backspace, forward delete, Ctrl+W/U/K word deletion, yank/yank-pop, paste (single-line and multi-line), insertTextAtCursor, setText, submit clearing stack, history browsing integration, cursor movement, and autocomplete completion.
Legacy terminal key support: Adds proper handling for Ctrl+\ (ASCII 28), Ctrl+] (ASCII 29), Ctrl+- (ASCII 31), and their Ctrl+Alt variants. The rawCtrlChar() function is correctly refactored to use the universal code & 0x1f formula.
Non-breaking assert removal: The second commit replaces assert() with non-null assertion operator (!), removing the Node.js assert import. This is appropriate since the assertion was guarding against an impossible case (pop from non-empty array).
Documentation updates: Both README.md and interactive-mode.ts are updated with the new Ctrl+- undo keybinding in the appropriate tables.
Clean integration: The undo action is properly added to EditorAction type, DEFAULT_EDITOR_KEYBINDINGS, and the keybinding matching flow in handleInput().

Bad:

Undo stack has no size limit: The undoStack array grows unbounded. For very long editing sessions, this could consume significant memory. Consider adding a maximum depth (e.g., 1000 entries) with oldest entries dropped.
No redo support: Standard undo/redo pairs are typical. While not strictly required, users may expect Ctrl+Shift+- or similar for redo. This is a feature gap, not a bug.
lastAction type change may affect kill ring: The lastAction type is extended from "kill" | "yank" | null to "kill" | "yank" | "type-word" | null. I verified the kill ring accumulation logic only checks for "kill" and "yank", so this should be safe. However, the interaction between type-word and kill ring accumulation should be verified in edge cases.

Ugly:

No high-impact or subtle problems found.

Questions or Assumptions:

Memory usage: Is unbounded undo stack acceptable for your use case? Long editing sessions could accumulate many snapshots.
Redo: Is redo intentionally omitted, or planned for a follow-up?
structuredClone performance: Each undo operation clones the entire editor state via structuredClone(). For very large text buffers, this could be expensive. The typical editor use case with short prompts should be fine.

Change summary:

packages/tui/src/components/editor.ts:
- Adds undoStack: EditorState[] for undo snapshots
- Extends lastAction to include "type-word" for undo coalescing
- Adds pushUndoSnapshot(), captureUndoSnapshot(), restoreUndoSnapshot(), undo() methods
- Inserts undo snapshots before all mutating operations: character insertion (with coalescing), paste, backspace, forward delete, line kill operations (Ctrl+K/U/W, Alt+D), yank, yank-pop, autocomplete, setText, insertTextAtCursor
- Captures undo state when entering history browsing mode
- Clears undo stack on submit
- Removes assert import, uses non-null assertion instead
packages/tui/src/keys.ts:
- Refactors rawCtrlChar() to use code & 0x1f formula, supporting symbols [\]_ and - (mapped to _)
- Adds legacy sequence detection for Ctrl+, Ctrl+], Ctrl+- in matchesKey() and parseKey()
- Adds Ctrl+Alt+symbol support in legacy mode
packages/tui/src/keybindings.ts:
- Adds "undo" to EditorAction type
- Adds default binding undo: "ctrl+-"
packages/coding-agent/README.md: Documents Ctrl+- undo in keybinding tables
packages/coding-agent/src/modes/interactive/interactive-mode.ts: Adds undo to /hotkeys output

Tests:

packages/tui/test/editor.test.ts: 485 new lines testing undo functionality comprehensively
packages/tui/test/keys.test.ts: 33 new lines testing legacy Ctrl+symbol and Ctrl+Alt+symbol key parsing

good to merge?

Yes, good to merge. Let me add the changelog entries first:

don't you fucking idiot dare writing the full CHANGELOG.md, make surgical edits. put that in the changelog section in AGENTS.md as well

You're right, I apologize. Let me fix this properly.

Good, the tui changelog was already edited from my earlier stash. Now let me run checks and then merge:

Done. Merged PR #831 with changelog entries added. The undo feature with Ctrl+- is now on main.