Skip to content

feat(tui): replace /diff command with persistent Diff tab#804

Merged
kelsonpw merged 1 commit into
mainfrom
feat/diff-tab
May 22, 2026
Merged

feat(tui): replace /diff command with persistent Diff tab#804
kelsonpw merged 1 commit into
mainfrom
feat/diff-tab

Conversation

@kelsonpw
Copy link
Copy Markdown
Member

@kelsonpw kelsonpw commented May 16, 2026

What landed

  • New src/ui/tui/components/DiffTab.tsx — persistent tab body in RunScreen. File list (newest-first, deduped by path) + colored unified diff for the selected file below. ↑↓ / j/k move selection; PgUp/PgDn scroll the diff body; ←→ still switch tabs.
  • src/ui/tui/screens/RunScreen.tsx — wired the Diff tab between Events and Logs.

What was removed

  • /diff slash command registration in src/ui/tui/console-commands.ts (and the parseDiffSlashInput helper).
  • /diff dispatch case in src/ui/tui/components/ConsoleView.tsx (~80 lines of summary + detail branches).
  • (kept) DiffViewer primitive — now consumed by the new tab and the existing outro "what changed" section.

Diff tab behavior

The tab subscribes narrowly to fileWritesTotal (a monotonic counter that climbs on every agent file write) and re-walks the canonical FileChangeLedger only when that counter advances. The DiffViewer primitive already windows the patch body to maxLines (default 30) around scrollOffset, so large diffs render at a constant ~30-line cost per frame. Outer Box uses overflow="hidden" (defense against the #779 overdraw bug). Empty state copy: no file changes yet — the agent hasn't edited anything. Discoverability hints in FileWritesPanel and DiffViewer summary mode now point to the Diff tab instead of the removed slash command.

Tests

  • New: src/ui/tui/screens/__tests__/RunScreen.diffTab.test.tsx (6 tests + 2 snapshots at 80 / 60 cols). Covers tab visibility, tab ordering (Progress → Events → Diff → Logs), empty state, and content rendering with seeded ledger entries.
  • Updated: console-commands.test.ts — replaced the old /diff registry assertions with a "removal" describe block that asserts /diff is absent and isKnownCommand('/diff') is false (the registry-doesn't-contain-/diff acceptance check).
  • Updated: DiffViewer.test.tsx and FileWritesPanel.test.tsx for the new hint copy.

Test plan

  • pnpm tsc --noEmit && pnpm lint && pnpm test — all green (4470 tests)
  • src/utils/wizard-abort.ts untouched
  • tab switching still works
  • /diff is gone from the slash palette (registry + isKnownCommand assertions)
  • Snapshot tests at 80 cols + 60 cols

Deferred

The end-of-run "all changes summary" overlay that PR #599 also added is intentionally untouched — this PR only addresses the slash command → tab migration. No docs / skill prompts referenced /diff.

🤖 Generated with Claude Code


Note

Medium Risk
Medium risk because it changes a key navigation/workflow surface (removing /diff and adding a new RunScreen tab) and introduces new keyboard/scroll state that could regress TUI behavior, though it is isolated to UI and covered by new tests.

Overview
Adds a persistent RunScreen Diff tab that lists changed files (deduped, newest-first) and renders a colored unified diff for the selected file with keyboard navigation (↑↓/j/k) and paging scroll.

Removes the /diff slash command end-to-end (command registry, parsing/tests, and ConsoleView dispatch) and updates related UI copy/hints (e.g. DiffViewer summary footer, FileWritesPanel) to point users to the new tab. Adds an end-to-end RunScreen.diffTab test suite with snapshots to lock tab ordering, empty state, and narrow-width rendering.

Reviewed by Cursor Bugbot for commit a3fd2f7. Bugbot is set up for automated code reviews on this repo. Configure here.

What landed
-----------
- New `src/ui/tui/components/DiffTab.tsx` — persistent tab body in
  RunScreen. File list (newest-first, deduped by path) with a colored
  unified diff for the selected file rendered below. `↑↓` / `j/k` move
  selection; `PgUp/PgDn` scroll the diff body; `←→` still switch tabs.
- `src/ui/tui/screens/RunScreen.tsx` — wired the Diff tab between
  Events and Logs.

What was removed
----------------
- `/diff` slash command registration in `src/ui/tui/console-commands.ts`
  (and the `parseDiffSlashInput` helper).
- `/diff` dispatch case + its summary/detail branches in
  `src/ui/tui/components/ConsoleView.tsx`.
- (kept) `DiffViewer` primitive — now consumed by the new tab and the
  existing outro "what changed" section.

Diff tab behavior
-----------------
The tab subscribes narrowly to `fileWritesTotal` (a monotonic counter
that climbs on every agent file write) and re-walks the canonical
`FileChangeLedger` only when that counter advances. The DiffViewer
primitive already windows the patch body to `maxLines` (default 30)
around `scrollOffset`, so large diffs render at a constant ~30-line
cost per frame. Outer Box uses `overflow="hidden"` (defense against
the #779 overdraw bug). Empty state copy: "no file changes yet — the
agent hasn't edited anything." Discoverability hints in
`FileWritesPanel` and `DiffViewer` summary mode now point to the
Diff tab instead of the removed slash command.

Tests
-----
New: `src/ui/tui/screens/__tests__/RunScreen.diffTab.test.tsx`
(6 tests + 2 snapshots at 80/60 cols) covers tab visibility, tab
ordering (Progress → Events → Diff → Logs), empty state, and
content rendering with seeded ledger entries.
Updated: `console-commands.test.ts` — replaced the old `/diff` registry
assertions with a "removal" describe block that asserts `/diff` is
absent and `isKnownCommand('/diff')` is false (the registry-doesn't-
contain-/diff acceptance check). `DiffViewer.test.tsx` and
`FileWritesPanel.test.tsx` updated for the new hint copy.

Verification
------------
- `pnpm tsc --noEmit && pnpm lint && pnpm test` — all green (4470 tests)
- `src/utils/wizard-abort.ts` untouched
- ← → tab switching still works
- /diff is gone from the slash palette (registry + isKnownCommand
  assertions)

Deferred
--------
The end-of-run "all changes summary" overlay that PR #599 also added
is intentionally untouched — this PR only addresses the slash command
→ tab migration. No docs/skill prompts referenced `/diff`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@kelsonpw kelsonpw requested a review from a team as a code owner May 16, 2026 00:27
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

Bugbot Autofix prepared fixes for both issues found in the latest run.

  • ✅ Fixed: Stale scroll hint misguides DiffTab users
    • Updated the DiffViewer detail-mode hint from "press j/k or arrow keys to scroll" to "press PageUp/PageDown to scroll" to match DiffTab's actual key bindings.
  • ✅ Fixed: useEffect clamp leaves render-time undefined access window
    • Added an inline clamp (Math.min(selectedIndex, files.length - 1)) at the render-time access site so files[clampedIndex] is always in bounds, closing the one-render gap before the useEffect fires.

Create PR

Or push these changes by commenting:

@cursor push 813ab281f7
Preview (813ab281f7)
diff --git a/src/ui/tui/components/DiffTab.tsx b/src/ui/tui/components/DiffTab.tsx
--- a/src/ui/tui/components/DiffTab.tsx
+++ b/src/ui/tui/components/DiffTab.tsx
@@ -238,7 +238,8 @@
     return <EmptyState />;
   }
 
-  const selected = files[selectedIndex];
+  const clampedIndex = Math.min(selectedIndex, files.length - 1);
+  const selected = files[clampedIndex];
 
   return (
     // Outer Box uses `overflow="hidden"` — same defense the rest of the
@@ -247,7 +248,7 @@
     <Box flexDirection="column" flexGrow={1} overflow="hidden">
       <FileList
         files={files}
-        selectedIndex={selectedIndex}
+        selectedIndex={clampedIndex}
         installDir={store.session.installDir}
       />
       <Box marginTop={1} paddingX={1} flexDirection="column">

diff --git a/src/ui/tui/components/DiffViewer.tsx b/src/ui/tui/components/DiffViewer.tsx
--- a/src/ui/tui/components/DiffViewer.tsx
+++ b/src/ui/tui/components/DiffViewer.tsx
@@ -18,9 +18,9 @@
  *   - hunk header → `Colors.accent`
  *   - context    → `Colors.muted`
  *
- * Keyboard nav (detail mode): j/down to scroll forward, k/up to scroll
- * back, q/esc to exit. Owned by the parent surface — DiffViewer just
- * renders the slice given by `scrollOffset`.
+ * Keyboard nav (detail mode): PageDown to scroll forward, PageUp to scroll
+ * back. Owned by the parent surface — DiffViewer just renders the slice
+ * given by `scrollOffset`.
  */
 
 import { Box, Text } from 'ink';
@@ -159,7 +159,7 @@
             <Text color={Colors.muted}>
               {patchLines.length - scrollOffset - maxLines} more line
               {patchLines.length - scrollOffset - maxLines === 1 ? '' : 's'}{' '}
-              {Icons.dot} press j/k or arrow keys to scroll
+              {Icons.dot} press PageUp/PageDown to scroll
             </Text>
           </Box>
         )}

You can send follow-ups to the cloud agent here.

Reviewed by Cursor Bugbot for commit a3fd2f7. Configure here.

installDir={store.session.installDir}
scrollOffset={scrollOffset}
maxLines={maxLines}
/>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale scroll hint misguides DiffTab users

Medium Severity

DiffViewer detail mode displays the hint "press j/k or arrow keys to scroll" (line 162), but DiffTab binds j/k and arrow keys to file selection, while PageUp/PageDown handle diff scrolling. Since this PR is the first production consumer of DiffViewer detail mode (OutroScreen only uses summary mode, and the old /diff command used plain-text feedback), the hint is now actively misleading for any diff longer than maxLines. Users following it will navigate the file list instead of scrolling.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a3fd2f7. Configure here.

if (selectedIndex >= files.length) {
setSelectedIndex(files.length - 1);
}
}, [files.length, selectedIndex]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useEffect clamp leaves render-time undefined access window

Low Severity

The selectedIndex clamp lives in a useEffect (post-render), but files[selectedIndex] is accessed during render on line 241 and dereferenced as selected.path on line 256. If files ever shrinks between renders, one render will access an out-of-bounds index before the effect corrects it, causing a TypeError. The comment explicitly anticipates FIFO eviction, so clamping inline (e.g. Math.min(selectedIndex, files.length - 1)) at the access site would close the gap.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a3fd2f7. Configure here.

@kelsonpw kelsonpw removed the request for review from a team May 18, 2026 18:04
@kelsonpw kelsonpw merged commit 9290389 into main May 22, 2026
12 checks passed
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