refactor(TUI): improve command menu scroll and resize mechanics - #2467
Conversation
agentcore-devx-automation
Bot
added
agentcore-harness-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
![agentcore-devx-automation[bot]](https://avatars.githubusercontent.com/u/2232217?s=60&v=4){kind=small}
There was a problem hiding this comment.
AgentCore Harness Review
Verdict: Looks good
Nice refactor. The createResizeGate design is clean — coalescing shrink events until the gesture settles addresses the wrap-and-repaint race that the old clearScreenOnNarrowing could only mitigate, and routing Ink's resize listeners through an in-process EventEmitter while proxying dimensions keeps Ink from seeing intermediate shrink sizes. The traces I walked (shrink, shrink→grow, grow→shrink, no-op cycle) all publish exactly once at the final dimensions, and the ERASE happens before the emit so Ink's listener repaints on a cleared screen.
A few non-blocking observations for your consideration:
useScrollWindowis now unused (src/components/scrollWindow.tsx:86-95). SinceCommandMenuBodynow deriveswindowStartfromhighlighton every render, the hook that persistedstartacross renders is dead code. Worth removing (or leaving with a comment) so future readers don't wonder which is canonical.- Intentional UX change in scroll behavior? The new
windowStart = max(0, highlight - floor(menuHeight/2))(RouterScreen.tsx:260) recenters on the highlight every render, whereasuseScrollWindowpreviously kept the window stable and scrolled only enough to keep the highlight visible. On arrow-key navigation this means every keystroke past the midpoint shifts every row, versus the older "scroll only when leaving the viewport." If that's the intended UX given the commit message, fine — flagging in case it wasn't. BANNER_ROWS = 4inRouterScreen.tsx:19duplicates structural knowledge ofBrandBanner(3 logo rows + divider). Since you already exportMIN_BANNER_COLUMNS/ROWSfromBrandBanner.tsx, consider exporting the banner height there too so it can't drift.
Resize test (tui.test.tsx) exercises the settle+coalesce behavior well; the new RouterScreen test for banner hide/restore is a good addition.
agentcore-devx-automation
Bot
removed
the
agentcore-harness-reviewing
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #2467 +/- ##
============================================
- Coverage 97.26% 97.24% -0.03%
============================================
Files 615 615
Lines 43665 43804 +139
============================================
+ Hits 42471 42596 +125
- Misses 1194 1208 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| const { columns, rows } = useWindowSize(); | ||
| const bannerVisible = | ||
| Boolean(banner) && !hideBanner && columns >= bannerMinColumns && rows >= bannerMinRows; | ||
| const contentRows = Math.max(0, rows - FRAME_ROWS - (bannerVisible ? bannerHeight : 0)); |
There was a problem hiding this comment.
[P2] Could we account for wrapped breadcrumbs and descriptions when calculating contentRows? At 40×15, scrolling the root menu to update hides the selected command because FRAME_ROWS underestimates the header height.
There was a problem hiding this comment.
Updated this PR. We now are measuring the breadcrumb headers actual rendered height, including wrapped breadcrumbs and descriptions.This value is subtracted when calculating contentRows. Added a regression test to cover this case as well
|
|
||
| const publish = (clear: boolean) => { | ||
| const changed = columns !== pendingColumns || rows !== pendingRows; | ||
| if (!changed) return; |
There was a problem hiding this comment.
[P2] Could we force a full repaint after a shrink, even if the final size matches the cached one? Resizing 100×40 → 100×15 → 100×40 with 25ms between changes skips all output here, leaving the header and selection missing until another state change.
There was a problem hiding this comment.
Good callout. Updated PR so we force Ink’s cache to be invalidated when a shrink gesture lands at its original dimensions. We then follow with a repaint to the actual final size. Added a regression test covering this exact scenario
| addListener("prependOnceListener", event, listener); | ||
| } | ||
| if (property === "off" || property === "removeListener") { | ||
| return removeListener; |
There was a problem hiding this comment.
It looks like this is only used in tests. If so, can it be put in a test util file? That would make the intent clearer.
There was a problem hiding this comment.
Oh wait, it's used in the root.
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
agentcore-devx-automation
Bot
added
the
claude-security-reviewing
|
Claude Security Review: no high-confidence findings. (run) |
agentcore-devx-automation
Bot
removed
the
claude-security-reviewing
4 participants
Description
PR has before and after videos of behavior, make sure to scroll to bottom of description
Improves TUI behavior with optimizations to scroll and resizing mechanics.
Problem:
The command menu had some rough edges with scroll/resize behavior:
AgentCore CLIbanner took up majority of available space on small terminalsBefore this PR:
Screen.Recording.2026-09-29.at.8.23.01.PM.mov
Changes:
After this PR:
Screen.Recording.2026-09-29.at.8.28.45.PM.mov
Type of Change
Testing
How have you tested the change?
bun run test(x pass, 0 fail)npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.