feat(review): improve drill-in scrolling and post-run access#1184
Open
peyton-alt wants to merge 12 commits into
Open
feat(review): improve drill-in scrolling and post-run access#1184peyton-alt wants to merge 12 commits into
peyton-alt wants to merge 12 commits into
Conversation
The drill-in alt-screen (Ctrl+O) previously rendered each agent event as a single terminal-width-truncated line, with manual scroll-position math driving which window of events was visible. Long multi-paragraph AssistantText findings were unreadable. This change reworks the drill-in around bubbles/v2/viewport so: - A new wrapDisplayWidth helper in tui_text.go honors embedded '\n' as paragraph boundaries and wraps each paragraph independently with ansi.Wrap, preserving multi-paragraph review findings instead of collapsing newlines to spaces. - eventLine is replaced by eventLines, returning []string (wrapped). All existing event types (Started/AssistantText/ToolCall/Tokens/Finished/ RunError) still render; AssistantText is the only one that fans out to multiple lines on embedded newlines. - detailView is replaced by detailFrame, a pure-function chrome (header + body + footer) that wraps an already-rendered viewport body and pads to exactly termHeight lines so the alt-screen frame diff doesn't leave ghost rows. - reviewTUIModel gets a viewport.Model field initialized with sane defaults (80x22) so Ctrl+O before any WindowSizeMsg still renders. WindowSizeMsg, Ctrl+O entry, and ←/→ agent-cycling all resize and re-fill the viewport via refreshDetailContent. - Unhandled keys in detail mode are delegated to viewport.Update so PgUp/PgDn/Home/End/↑/↓/mouse wheel reach its internal keymap. - Auto-tail behavior is preserved: refreshDetailContent samples AtBottom() before SetContentLines and re-issues GotoBottom() if the user was tailing, so a scrolled-up user is not yanked back to the bottom by incoming events. Removed (now dead): - detailScroll field on reviewTUIModel - maxDetailScroll() and clampScroll() helpers - buildBodyLines (replaced by the viewport's own visible-line math via SetContentLines) - Old eventLine and detailView (renamed to eventLines and detailFrame) The footer hint shifts from "↑/↓ scroll" to "PgUp/PgDn scroll" to match what reaches the viewport via key delegation. Test changes: - Existing detailView_* tests are rewritten as detailFrame_* tests that drive rendering through the model (newReviewTUIModel + WindowSizeMsg + agentEventMsg + Ctrl+O + View()), since the viewport is now the source of truth for body lines. - Auto-follow tests now assert on viewport.YOffset() / AtBottom() instead of detailScroll. - New TestDetailFrame_WrapsLongAssistantText (load-bearing) pins that a ≥200-char AssistantText on a 40-wide terminal wraps to multiple visible body lines, each fitting termWidth. - New TestDetailFrame_PreservesAssistantTextNewlines pins multi- paragraph text survives intact. - New TestTUIModel_InitializesDetailViewport pins non-zero default viewport dimensions. - New TestTUIModel_DelegatesScrollKeysToViewport pins that PgDn in detail mode advances viewport YOffset (key delegation works). Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Entire-Checkpoint: 4e31dbdd1b05
Two user-visible fixes to the multi-agent review TUI: 1. Post-finish drill-in is still reachable. Before: any key on the post-finish dashboard quit the TUI, so Ctrl+O on a completed run was swallowed and the user couldn't inspect the final per-agent buffer. Now: only q, Esc, Enter, and Ctrl+C dismiss the post-finish dashboard; every other key (including Ctrl+O) falls through to normal handling. The footer changes from "Press any key to exit." to "Ctrl+O: drill in · q/Esc/Enter: exit" so the available actions are explicit. Detail-mode behavior is unchanged — Esc still returns to the dashboard and viewport keys still scroll. 2. Cancellation is visible while agents drain. Before: pressing Ctrl+C once cancelled both agents via the shared CancelFunc, but the dashboard kept showing the running spinner until the agents fully drained. With no visual feedback, users pressed Ctrl+C a second time to dismiss, which short-circuited the dump. Now: a `cancelling` flag flips to true on the first Ctrl+C alongside the existing `cancelOnce.Do(cancel)`. The dashboard footer becomes "Cancelling agents... · Ctrl+C again: force quit", and any agent still in the running branch of renderRow renders as the literal "cancelling" — distinct from the terminal "— cancel" used for AgentStatusCancelled, and 10 chars so it fits the existing statusWidth column without resizing the table. The first Ctrl+C no longer emits tea.Quit; the TUI stays up so the user can see the indicator while the program awaits the natural runFinishedMsg. A second Ctrl+C while cancelling=true force-quits immediately via tea.Quit. The shared CancelFunc still fires at most once thanks to cancelOnce, so the second press doesn't double-cancel. The single shared CancelFunc mechanism is unchanged — only the visibility/feedback layer was added. Per-agent context cancellation, mouse-wheel scroll, and viewport rendering are out of scope. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Entire-Checkpoint: e3562dc89402
Commit 1 of this branch swapped the manual drill-in scroll path for bubbles/v2/viewport. The viewport widget handles mouse-wheel events natively, but only if the Bubble Tea program is configured to deliver mouse messages — otherwise wheel input is consumed by the terminal as a plain scroll-region scroll, which the alt-screen ignores. Result: the viewport sat inert under the wheel even though every other interaction worked. Bubble Tea v2 reshaped mouse config from a Program option (`tea.WithMouseCellMotion()` in v1) to a per-view field (`View.MouseMode`). Set `MouseMode = tea.MouseModeCellMotion` on the detail-mode View next to the existing AltScreen toggle. Dashboard mode keeps the default `MouseModeNone` so the summary table doesn't capture mouse selection — wheel scrolling only kicks in once the user enters the drill-in via Ctrl+O. Extended the existing AltScreen-toggle tests to also assert the MouseMode flip; the wheel-handling itself lives in viewport and is covered upstream. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Entire-Checkpoint: 007dbb86f787
The RunFinished doc on TUISink still described the pre-Commit-2
behavior ("press any key to dismiss"). Update to reflect the new
explicit exit-key handling (q/Esc/Enter/Ctrl+C) with Ctrl+O still
available for post-mortem drill-in. Comment-only change, no code
behavior touched.
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Entire-Checkpoint: a6c21d0fe3b7
Commit 998ddfd correctly set View.MouseMode = MouseModeCellMotion on entry to detail mode so the bubbletea Program would receive mouse events from the terminal. But the Update switch only routed tea.KeyPressMsg to handleKey and tea.WindowSizeMsg to a resize handler — mouse events fell through to the catch-all `return m, nil` at the bottom and never reached the viewport. bubbles/v2/viewport handles tea.MouseWheelMsg natively when its MouseWheelEnabled flag is set (true by default), so the fix is just to add a case in the Update switch that forwards mouse messages to m.detail.Update when in detail mode. Includes a regression test (TestTUIModel_DelegatesMouseWheelToViewport) that sends a synthetic tea.MouseWheelMsg{Button: tea.MouseWheelDown} into the model in detail mode and asserts the viewport's YOffset advances. Mirrors the existing keyboard-delegation test pattern. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Entire-Checkpoint: 39b4e9bacbf6
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the entire review TUI drill-in (Ctrl+O) to behave like a real pager: long events wrap to multiple lines, scrolling is handled by bubbles/v2/viewport with standard keys and mouse wheel support, and drill-in remains accessible after the run finishes. It also changes cancellation UX so the first Ctrl+C shows an in-flight “cancelling” state (with a second Ctrl+C force-quitting).
Changes:
- Replaced manual drill-in scrolling/truncation with a
viewport.Model-driven pager and multi-line wrapping. - Updated post-finish behavior to require explicit exit keys and keep Ctrl+O drill-in usable after completion.
- Added cancellation-in-progress UI feedback and mouse wheel event delegation to the viewport, with regression tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/review/tui_text.go | Adds display-width wrapping helper for multi-line drill-in rendering. |
| cmd/entire/cli/review/tui_sink.go | Updates RunFinished behavior documentation to reflect explicit exit keys + post-finish drill-in. |
| cmd/entire/cli/review/tui_sink_test.go | Adjusts dismissal behavior in tests to use explicit exit key (q). |
| cmd/entire/cli/review/tui_model.go | Integrates viewport.Model, adds cancellation state/UX, enables mouse mode in detail, delegates keys/mouse to viewport. |
| cmd/entire/cli/review/tui_model_test.go | Adds/updates tests for viewport init, key/mouse delegation, post-finish access, and cancellation behavior. |
| cmd/entire/cli/review/tui_detail.go | Refactors detail renderer into chrome frame + viewport body; wraps events into multiple lines. |
| cmd/entire/cli/review/tui_detail_test.go | Updates drill-in rendering tests to exercise model+viewport behavior and wrapping/newline preservation. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Entire-Checkpoint: 21824f232bbf
peyton-alt
changed the title
Contributor
Author
|
@BugBot review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit cc90173. Configure here.
- tui_sink.go: TUISink doc still described the pre-PR "single Ctrl+C cancels and quits" semantic; update to reflect the two-press flow (first press fires CancelFunc + shows indicator, second press force-quits). - tui_detail.go: drill-in footer listed only PgUp/PgDn even though the viewport's default keymap binds ↑/↓/Home/End as well; expand so users see what actually works. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Entire-Checkpoint: 091517a61c13
Two related fixes to the TUI's cancellation and post-finish dismissal: 1. Ctrl+C with all agents already terminal short-circuits to tea.Quit instead of setting cancelling=true and firing the CancelFunc. The race window between the last agent's Finished event and the orchestrator's runFinishedMsg was flashing "Cancelling agents..." for a user who effectively had nothing left to cancel. New allAgentsTerminal() helper gates the cancelling path on whether any row is still AgentStatusUnknown. 2. q / Enter / Ctrl+C in detail mode AFTER the run finishes now quit directly. Previously they fell through to the viewport (which has no quit binding) and were silently inert — the user had to Esc to the dashboard first, then press an exit key. Esc itself keeps its "back to dashboard" meaning so the post-finish dashboard is still reachable from detail mode. Removes the now-unused isPostFinishExitKey helper in favor of an inlined switch that handles both dashboard and detail-mode post-finish cases together. Three new tests pin the changed behaviors: - TestTUIModel_CtrlCAfterAllRowsTerminalQuitsImmediately - TestTUIModel_PostFinishInDetailMode_ExitKeysQuit (q/Enter/Ctrl+C) - TestTUIModel_PostFinishInDetailMode_EscReturnsToDashboard Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Entire-Checkpoint: 216d1bdc0348
… tests Two regression-guard tests covering behaviors that are correct in the current implementation but were unpinned: - TestTUIModel_DelegatesNonWheelMouseEventsToViewport asserts that the Update case-arm routing mouse events to the viewport covers Click, Release, and Motion in addition to Wheel. A future refactor narrowing the case-arm to wheel-only would silently break click-drag selection inside the drill-in while still passing the existing wheel test. The viewport does not expose selection state publicly, so the test pins the weaker invariant that the events do not panic and detailMode survives — enough to catch the case-arm-removal regression. - TestTUIModel_CancellingIndicatorOnlyAffectsRunningRows asserts that renderRow only swaps "running" for "cancelling" — rows already in a terminal status keep their own indicator (e.g. "✓ done"). Without this pin a future refactor could surface "cancelling" on Succeeded rows, misleading the user about which agents actually got cancelled. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> Entire-Checkpoint: 787efb0b4aa5
6 tasks
Four small follow-ups from review of this PR:
1. Reorder the Ctrl+C handler so cancelling beats detailMode. The
dashboard footer hint "Ctrl+C again: force quit" was unreachable
from inside the drill-in view: detailMode was checked first and
silently swallowed the keypress. A user who hits Ctrl+C, drills in
to watch a hanging agent, and then wants out had to Esc back to the
dashboard first — the exact failure mode the force-quit escape
hatch exists for, blocked in the one context where it matters most.
The idle "Esc first" guard still applies when cancelling=false.
2. Hoist defaultTermWidth/defaultTermHeight to package level. They
were local to newReviewTUIModel and the dashboardWidth() /
detailViewportWidth() fallbacks duplicated the 80 literal — except
detailViewportWidth() actually fell back to 1, which would collapse
the viewport to a single column. Now both functions share the same
package-level constant.
3. Resolve the tea.KeyEscape || tea.KeyEsc redundancy. Per
ultraviolet/key.go:224, KeyEsc = KeyEscape (literal alias) — the
disjunction was dead code. Kept KeyEscape (canonical name).
4. SetContent("") → SetContentLines(nil) on the empty-buffer path.
refreshDetailContentWithAutoTail mixed the two viewport APIs in a
single function; nil-lines is the symmetric twin to the populated
SetContentLines call below it.
New test pins the cancelling-beats-detailMode contract:
TestTUIModel_SecondCtrlCForceQuits_FromDetailMode
Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
…ewport # Conflicts: # cmd/entire/cli/review/tui_model.go # cmd/entire/cli/review/tui_model_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://entire.io/gh/entireio/cli/trails/359
Summary
Fixes bug #3 from the broader review observability sweep: the Ctrl+O drill-in view was unhelpful because each event was truncated to one terminal-wide line, navigation was limited to ↑/↓, and the whole view became unavailable after both agents finished (any key dismissed the TUI). This PR replaces the manual scroll math with
bubbles/v2/viewport, wraps long events to multiple lines, adds standard pager keys + mouse wheel, keeps Ctrl+O usable post-finish, and adds visible cancellation feedback so users see "agents are stopping" instead of staring at a still-spinning dashboard.What lands
Five commits, each independently sensible:
a9210fb9f— Viewport widget + multi-line wrap + key delegationdetailScroll intfield withbubbles/v2/viewportwidget. Viewport owns scroll state, key bindings, and rendering.wrapDisplayWidth(s, width) []stringhelper intui_text.gomirroringtruncateDisplayWidth. Preserves embedded newlines as paragraph boundaries (load-bearing for multi-paragraph AssistantText).eventLine→eventLinesreturning[]string. Each event renders as one or more wrapped lines instead of one truncated line.detailView→detailFrame(header + viewport body + footer, padded totermHeight).viewport.Update— PgUp/PgDn/Home/End/g/G/u/d/j/k all work via viewport's internal keymap.~130 linesof manual windowing logic (buildBodyLines,maxDetailScroll,clampScroll).27fd1baca— Post-finish access + visible cancellation feedbackq/Esc/Enter/Ctrl+C. Other keys includingCtrl+Ofall through, so users can drill into completed agent buffers for post-mortem inspection.Ctrl+O: drill in · q/Esc/Enter: exit(replaces the misleading "Press any key to exit.")cancelling boolflag onreviewTUIModel. First Ctrl+C sets it and fires the shared CancelFunc once viacancelOnce. Second Ctrl+C during drain emitstea.Quitimmediately for force-quit.cancelling, dashboard rows for still-running agents showcancellingstatus text (10 chars, fits the existing column).Cancelling agents... · Ctrl+C again: force quit.998ddfd1e— Enable mouse mode in detail-mode Viewv.MouseMode = tea.MouseModeCellMotion), not a Program option. Only enabled while in detail mode so dashboard preserves normal terminal mouse-select for copy/paste.6fb988b43— Doc comment fixuptui_sink.godoc onRunFinishedstill described the pre-Commit-2 "press any key to dismiss" behavior. Updated to reflect explicit-exit-keys + Ctrl+O drill-in availability. Comment-only change.c279308b2— Route mouse events to viewport (the actual wheel-scroll wire-up)View.MouseModemade the bubbletea Program receive mouse events from the terminal, but theUpdateswitch only routedtea.KeyPressMsgtohandleKeyandtea.WindowSizeMsgto the resize handler. Mouse events fell through to the catch-allreturn m, niland never reached the viewport.tea.MouseWheelMsg,tea.MouseClickMsg,tea.MouseReleaseMsg,tea.MouseMotionMsgtom.detail.Updatewhen in detail mode. Viewport'sMouseWheelEnabledis true by default so wheel scroll just works.TestTUIModel_DelegatesMouseWheelToViewportregression test.Smoke evidence
End-to-end verified on this branch with a real
entire reviewrun:…truncation mid-content)TestDetailFrame_*,TestTUIModel_DelegatesScrollKeysToViewport,TestTUIModel_DelegatesMouseWheelToViewport)What this PR does NOT do (explicitly deferred)
proc.Wait()errors will surface in the drill-in event log AND the runFinishedMsg handler will propagate Failed status correctly. Independent of this PR.-pand codexexecbuffer their stdout until the response is complete, so during the run the drill-in shows mostly[started]until the buffer flushes at the end. Improving live streaming requires per-agent investigation (different output modes for each agent CLI) — that's its own brainstorm.tea.WithMouseCellMotion()as a Program option (v1 idiom); v2 moved this toView.MouseMode. Adapted accordingly with regression test.Test plan
mise run fmt && mise run lint— 0 issuesgo test ./cmd/entire/cli/review/ -count=1— all green (includes 3 new tests for wrap, viewport init, scroll-key delegation, mouse-wheel delegation, post-finish access, cancellation feedback)Two-stage subagent review per task
Both Commits 1 and 2 went through spec-compliance + code-quality review:
6fb988b43doc fixup)c279308b2with regression test🤖 Generated with Claude Code
Note
Medium Risk
Moderate risk: refactors drill-in rendering/scroll handling to a
viewport.Modeland changes Ctrl+C and post-finish key handling, which could affect TUI interaction flow and exit/cancellation behavior.Overview
Revamps the Ctrl+O drill-in view by replacing the manual one-line-per-event renderer/scroll math with a
bubbles/v2/viewport, so longAssistantTextwraps to multiple lines (preserving embedded newlines), supports standard pager keys and mouse-wheel scrolling, and keeps auto-tail behavior without yanking users when they’ve scrolled up.Adjusts run-completion and cancellation UX: post-finish no longer exits on any key (explicit exit keys only; Ctrl+O still works for post-mortem), and Ctrl+C now enters a visible “cancelling” state on first press (showing cancelling status + footer hint) with a second Ctrl+C force-quitting.
Adds/updates tests to exercise viewport-driven detail rendering (wrapping, ANSI stripping, width/height padding), mouse/key delegation, auto-follow semantics, and the new post-finish/cancellation behaviors.
Reviewed by Cursor Bugbot for commit cc90173. Configure here.