Harden TUI layout and mouse routing#226
Merged
Merged
Conversation
Add a reusable transcript frame snapshot with terminal regions for header, body, footer, composer, and status lines. Mouse composer hit testing, overlay positioning, and transcript viewport mapping now consume the same frame geometry instead of recalculating pinned-header and clipped-footer offsets separately. Tested: env GOCACHE=/tmp/zero-go-cache go test ./internal/tui
Return overlay hit coordinates relative to the overlay block and derive transcript hit rows from the shared body rect. This keeps the current scroll behavior while making mouse handling consume component-local geometry from the frame layout. Tested: env GOCACHE=/tmp/zero-go-cache go test ./internal/tui
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughIntroduces ChangesRectangle-based TUI frame geometry with viewport scrolling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/tui/transcript_selection.go (1)
313-314:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock transcript selection while the MCP add wizard is open.
Clicks that miss a wizard item fall through from
handleMouseinto transcript selection, and this guard does not includem.mcpAddWizard. That lets users select/toggle transcript rows behind the modal.🐛 Proposed fix
- if !m.altScreen || m.height <= 0 || m.setup.visible || m.providerWizard != nil || m.mcpManager != nil || m.picker != nil || m.suggestionsActive() { + if !m.altScreen || m.height <= 0 || m.setup.visible || m.providerWizard != nil || m.mcpAddWizard != nil || m.mcpManager != nil || m.picker != nil || m.suggestionsActive() { return transcriptSelectableLine{}, false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/transcript_selection.go` around lines 313 - 314, The guard condition that blocks transcript selection is missing a check for the MCP add wizard state. In the condition that checks various modal/wizard states (m.altScreen, m.height, m.setup.visible, m.providerWizard, m.mcpManager, m.picker, m.suggestionsActive), add a check for m.mcpAddWizard != nil to prevent transcript selection when the MCP add wizard is open. This will prevent clicks that miss wizard items from falling through to transcript selection and allowing row selection behind the modal.internal/tui/mouse.go (1)
445-452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClamp overlay mouse hits to the visible body window.
When an overlay is taller than
frame.bodyRect.height,overlayViewportLinesrenders only the visible body rows, butoverlayMouseHitstill acceptstop+len(lines). Clicks in the footer/composer can select invisible overlay rows.🐛 Proposed fix
func (m model) overlayMouseHit(msg tea.MouseMsg, overlay string, width int) (mouseOverlayHit, bool) { lines := viewLines(overlay) if len(lines) == 0 { return mouseOverlayHit{}, false } left, lines, overlayWidth := normalizeOverlayBlock(lines, width) if overlayWidth <= 0 || len(lines) == 0 { return mouseOverlayHit{}, false } - top := m.overlayMouseTop(len(lines), width) - if mouseY(msg) < top || mouseY(msg) >= top+len(lines) { + rect := m.overlayMouseRect(len(lines), width) + if rect.height <= 0 || mouseY(msg) < rect.y || mouseY(msg) >= rect.y+rect.height { return mouseOverlayHit{}, false } if mouseX(msg) < left || mouseX(msg) >= left+overlayWidth { return mouseOverlayHit{}, false } - return mouseOverlayHit{x: mouseX(msg) - left, y: mouseY(msg) - top}, true + return mouseOverlayHit{x: mouseX(msg) - left, y: mouseY(msg) - rect.y}, true } func (m model) overlayMouseRect(overlayHeight int, width int) tuiRect { if overlayHeight <= 0 { return tuiRect{} } if m.altScreen && m.height > 0 { frame := m.scrollableTranscriptFrame(m.pinnedTitleBar(width), m.footerView(width)) + visibleHeight := minInt(overlayHeight, frame.bodyRect.height) + if visibleHeight <= 0 { + return tuiRect{} + } return tuiRect{ - y: frame.bodyRect.y + maxInt(0, (frame.bodyRect.height-overlayHeight)/2), + y: frame.bodyRect.y + maxInt(0, (frame.bodyRect.height-visibleHeight)/2), width: width, - height: overlayHeight, + height: visibleHeight, } }Also applies to: 459-469
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/mouse.go` around lines 445 - 452, The overlayMouseHit function currently checks if mouseY is within the entire overlay height using top+len(lines), but this doesn't account for the visible body window when the overlay is taller than the frame.bodyRect.height. The mouse Y boundary check needs to be clamped to the visible body window instead of the full overlay. Modify the condition that checks mouseY(msg) >= top+len(lines) to use the actual visible height (capped by frame.bodyRect.height) instead of the total len(lines). Apply this same fix to the similar mouse boundary check code that appears around lines 459-469.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/tui/mouse.go`:
- Around line 445-452: The overlayMouseHit function currently checks if mouseY
is within the entire overlay height using top+len(lines), but this doesn't
account for the visible body window when the overlay is taller than the
frame.bodyRect.height. The mouse Y boundary check needs to be clamped to the
visible body window instead of the full overlay. Modify the condition that
checks mouseY(msg) >= top+len(lines) to use the actual visible height (capped by
frame.bodyRect.height) instead of the total len(lines). Apply this same fix to
the similar mouse boundary check code that appears around lines 459-469.
In `@internal/tui/transcript_selection.go`:
- Around line 313-314: The guard condition that blocks transcript selection is
missing a check for the MCP add wizard state. In the condition that checks
various modal/wizard states (m.altScreen, m.height, m.setup.visible,
m.providerWizard, m.mcpManager, m.picker, m.suggestionsActive), add a check for
m.mcpAddWizard != nil to prevent transcript selection when the MCP add wizard is
open. This will prevent clicks that miss wizard items from falling through to
transcript selection and allowing row selection behind the modal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 463dbc26-54f4-41ee-a4da-3e27dde0ea82
📒 Files selected for processing (3)
internal/tui/model.gointernal/tui/mouse.gointernal/tui/transcript_selection.go
Add focused tests for the shared frame snapshot: pinned header/body/footer regions, tiny-terminal footer clipping, composer mouse hit regions, overlay centering, and transcript mouse row mapping through the body region. Tested: env GOCACHE=/tmp/zero-go-cache go test ./internal/tui
Introduce a line-based transcript viewport adapter that preserves Zero's existing bottom-anchored scroll offset. Render slicing, chat scroll metrics, scrollChat, and transcript selection viewport mapping now share the same clamping and visible-window calculation. Tested: env GOCACHE=/tmp/zero-go-cache go test ./internal/tui
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/tui/layout_test.go (1)
40-65: 💤 Low valueConsider edge case: zero or negative dimensions.
While the tiny terminal test covers height=3, consider adding tests for zero or negative dimensions to ensure the layout system handles degenerate cases gracefully without panics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/layout_test.go` around lines 40 - 65, The test TestTranscriptFrameLayoutClipsFooterInTinyTerminal only validates behavior for a tiny terminal (height=3), but does not cover edge cases with zero or negative dimensions. Add additional test cases to verify that the layout system methods (scrollableTranscriptFrame, pinnedTitleBar, footerView, and chatWidth) handle degenerate inputs gracefully without panics when width or height are zero or negative values. Ensure these edge case tests confirm the system either returns valid results or fails in a controlled manner.internal/tui/transcript_viewport_test.go (1)
5-46: 💤 Low valueConsider edge cases: zero and negative dimensions.
While the tests cover normal and inflated values, consider adding tests for edge cases:
- Zero total lines (
totalLines=0)- Zero viewport height (
height=0)- Negative values (if not validated elsewhere)
These would ensure the viewport primitives handle degenerate inputs gracefully.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/transcript_viewport_test.go` around lines 5 - 46, Add test functions to cover edge cases for the transcript viewport that are not currently tested in the existing test suite. Create new test functions following the existing pattern (TestTranscriptViewport*) to verify behavior when totalLines is zero, when height is zero, and potentially when negative values are passed to newTranscriptViewport. Each test should verify that the viewport and window calculations handle these degenerate inputs gracefully and produce sensible results, checking properties like offset, window.start, window.end, window.height, and window.maxOffset as appropriate for each edge case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/layout_test.go`:
- Around line 32-34: The containment check in the test is incomplete because it
only validates that the top-left corner of composerRect is inside footerRect
using the contains method, but does not verify that the entire rectangle
(including its right and bottom edges) stays within the footer bounds. Fix this
by enhancing the validation condition to additionally check that the
bottom-right corner of composerRect (at coordinates x + width and y + height) is
also contained within footerRect, ensuring the entire rectangle is fully
contained within the footer.
---
Nitpick comments:
In `@internal/tui/layout_test.go`:
- Around line 40-65: The test TestTranscriptFrameLayoutClipsFooterInTinyTerminal
only validates behavior for a tiny terminal (height=3), but does not cover edge
cases with zero or negative dimensions. Add additional test cases to verify that
the layout system methods (scrollableTranscriptFrame, pinnedTitleBar,
footerView, and chatWidth) handle degenerate inputs gracefully without panics
when width or height are zero or negative values. Ensure these edge case tests
confirm the system either returns valid results or fails in a controlled manner.
In `@internal/tui/transcript_viewport_test.go`:
- Around line 5-46: Add test functions to cover edge cases for the transcript
viewport that are not currently tested in the existing test suite. Create new
test functions following the existing pattern (TestTranscriptViewport*) to
verify behavior when totalLines is zero, when height is zero, and potentially
when negative values are passed to newTranscriptViewport. Each test should
verify that the viewport and window calculations handle these degenerate inputs
gracefully and produce sensible results, checking properties like offset,
window.start, window.end, window.height, and window.maxOffset as appropriate for
each edge case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e79b6124-1e19-4f2f-8c7f-94c8e4078cce
📒 Files selected for processing (5)
internal/tui/layout_test.gointernal/tui/model.gointernal/tui/transcript_selection.gointernal/tui/transcript_viewport.gointernal/tui/transcript_viewport_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/transcript_selection.go
Introduce a compatibility transcript body item layer for title, empty state, separators, transcript rows, pending prompts, streaming interim content, and spec review prompts. The body still renders to the same line/selectable output, but layout now records item spans and local selectable offsets for the next visible-range step. Tested: env GOCACHE=/tmp/zero-go-cache go test ./internal/tui Tested: /bin/bash -lc "GOCACHE=/tmp/zero-go-cache go test ./..."
Clamp overlay mouse hit regions to the visible transcript body and block transcript selection while the MCP add wizard is open. Strengthen layout and viewport edge-case tests, including full composer containment and degenerate dimensions. Tested: env GOCACHE=/tmp/zero-go-cache go test ./internal/tui Tested: /bin/bash -lc "GOCACHE=/tmp/zero-go-cache go test ./..."
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/mouse_test.go`:
- Around line 489-505: The test
TestMCPAddWizardBlocksTranscriptSelectionBehindOverlay uses x=0 in the
testMouseClick call, which may miss the transcript text hitboxes entirely
regardless of overlay state, causing the test to pass vacuously without actually
proving the overlay blocks selection. Replace the x=0 coordinate parameter in
the testMouseClick call with textStart, which represents a known selectable
transcript text column, to ensure the click targets transcript text that would
be selectable if not for the overlay blocking it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ba210d5a-a894-4bac-83ec-b86ee1ef4832
📒 Files selected for processing (5)
internal/tui/layout_test.gointernal/tui/mouse.gointernal/tui/mouse_test.gointernal/tui/transcript_selection.gointernal/tui/transcript_viewport_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui/transcript_selection.go
- internal/tui/mouse.go
gnanam1990
reviewed
Jun 16, 2026
gnanam1990
left a comment
gnanam1990
left a comment
Collaborator
There was a problem hiding this comment.
Strong, well-structured hardening pass. The tuiRect geometry + region rects (header / body / footer / composer / status) replace a lot of ad-hoc offset math with one model, and the coverage — degenerate 0/negative dims, tiny-terminal clipping, footer clip, overlay clamping, frame-body click mapping — is exactly what this kind of layout code needs.
Highlights:
mouseOverComposercollapsing toframe.composerRect.contains(x, y)is a big readability win over the old manual clip math.overlayMouseRectclamping the overlay to the visible body height (with the test that a click below the visible rows misses) fixes a real class of off-screen-overlay hit bugs.layoutTranscriptBodyItemswith typed spans (empty / row / separator / pending-interim) and selectable lines shifted by item start is a much cleaner transcript-body model than threadingbodyYthrough the render funcs.
A few things:
- The coordination footprint is large. This rewrites the exact geometry/mouse surface (
scrollableTranscriptFrame,overlayMouseTop→overlayMouseRect,transcriptViewportStart,mouseOverComposer,transcriptLineAtMouse) that several in-flight changes also touch — #224 and a couple of other TUI changes (a clickable/selectable permission popup, and a right-docked panel that reserves horizontal space from the chat area). Whatever lands first forces a real rebase on the rest, so it's worth sequencing this deliberately. One concrete hook: anything that reserves width from the chat (a side panel) will want the frame rects'widthto come from a shared "content width" helper rather thanchatWidth(m.width)directly — a natural follow-up on this geometry. - Since this is the draft foundation with follow-ups stacking onto the branch, keeping each stacked slice independently green (build + the touched tests) will make the eventual review of the stack much easier.
- Minor:
mouseOverlayHitnow returnsx— is a current caller using it, or is it staged for the follow-ups? If unused for now, a one-line note saves a future "why is this here?".
Solid base, and nicely tested. 👍
CodeRabbit caught that the MCP add wizard blocking test clicked at x=0, which could miss selectable transcript text and pass without proving the overlay guard. This updates the test helper to return both textStart and y, then clicks that known selectable point before asserting the wizard blocks transcript selection behind the modal. Tested: env GOCACHE=/tmp/zero-go-cache go test ./internal/tui Tested: git diff --check Tested: GOCACHE=/tmp/zero-go-cache go test ./...
Thread the structured transcriptBodyLayout through alt-screen rendering, scroll metrics, and transcript mouse lookup instead of repeatedly joining and splitting the rendered body string. This preserves the current line-based viewport behavior while making the body layout the source of truth for visible windows and selectable line lookup. This is an incremental step toward visible-item rendering and item-level caching; it does not change the transcript visual output or input flow. Tested: env GOCACHE=/tmp/zero-go-cache go test ./internal/tui Tested: git diff --check Tested: /bin/bash -lc "GOCACHE=/tmp/zero-go-cache go test ./..."
Selectable user and assistant rows need selectable metadata for mouse interactions, but when no transcript selection is active their rendered strings are identical to the normal row render path. Route those rendered strings through renderRow so stable rows reuse the existing row cache while selection-active frames still render highlight-aware output. This keeps transcript behavior unchanged and adds a regression test that verifies selectable assistant rows produce stable metadata while hitting the render cache on the second render. Tested: env GOCACHE=/tmp/zero-go-cache go test ./internal/tui Tested: git diff --check Tested: /bin/bash -lc "GOCACHE=/tmp/zero-go-cache go test ./..."
anandh8x
marked this pull request as ready for review
June 16, 2026 17:56
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Vasanthdev2004
approved these changes
Jun 16, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: Approve — clean hardening pass with addressed review findings
This is a strong refactor that replaces a lot of ad-hoc Y-offset math with a typed, region-based geometry model. The two open CodeRabbit findings are already fixed in the current 0f2307f tip; @gnanam1990's human review was positive with one operational note and one unused-field note. No blockers.
What's good
tuiRect+transcriptFrameLayout—headerRect,bodyRect,footerRect,composerRect,statusRectare explicit rectangles withcontains/localhelpers. The frame computation inscrollableTranscriptFrame(model.go:1345-1392) lays them all out and the assertions inlayout_test.golock the invariant (no gaps, footer bottom == terminal height, status is the last visible footer line).mouseOverComposercollapsed to one line — the old code did its own footer clip + top +mouseYmath; the new code isframe.composerRect.contains(mouseX(msg), mouseY(msg)). Big readability win.overlayMouseRectclamps to visible body —mouse.go:464-471clamps the overlay toframe.bodyRect.height, so a click below the visible rows correctly misses. Fixes a real off-screen-overlay hit bug class.transcriptBodyLayoutis the right shape —lines+selectable+ typedspans(title/empty/separator/row/pending-prompt/pending-interim/spec-review). The body is no longer a string threaded withbodyYoffsets; it's a structured layout the viewport can window over (transcript_viewport.go) and the mouse can hit-test against.transcriptViewportis a pure value type —transcript_viewport.gois 57 LoC, no model dependency, table-driven tests for zero/negative inputs and clamp behavior. Easy to reason about.- CodeRabbit #1 already fixed —
layout_test.go:34now contains the full composer-rect containment check (top-left AND bottom-right corners insidefooterRect). The "Addressed in commit f3dfbfb" stamp on the inline comment confirms it. - CodeRabbit #2 already fixed —
mouse_test.go:491now usestextX, textY := firstTranscriptTextMousePoint(t, m)(line 562), which returns the actualtextStartcolumn, notx=0. The new testTestMCPAddWizardBlocksTranscriptSelectionBehindOverlayclicks at the real selectable position. - Test coverage is strong —
layout_test.go(185 LoC, 4 tests),transcript_viewport_test.go(85 LoC, 4 tests including a table-driven edge case pass),transcript_body_test.go(86 LoC, 3 tests),render_cache_test.go(20 LoC, 1 new test). Each commit on the branch added 30-100 LoC of tests.
Non-blocking notes (skip if you want)
-
mouseOverlayHit.xis added but unused (mouse.go:5, set at line 452). The struct is constructed and returned, but every caller reads onlyhit.y(verified:selectSuggestionAtMouse,selectMCPManagerAtMouse,selectMCPAddWizardAtMouse,selectModelPickerAtMouse,selectProviderWizardAtMouse, etc.). @gnanam1990 flagged this; agreed. Two options:- Drop the field now (1-line change in the struct + 1-line change at the return site). Keeps the surface area tight.
- Leave it with a one-line
// Reserved for the clickable permission popup / right-docked panel follow-upscomment** so the next reader knows it's staged, not stale.
I lean toward option 2 since the field's name and return-site comment in the diff suggest it's deliberate staging. If you don't have a follow-up PR that uses it, drop it. Otherwise comment it.
-
PR coordination footgun (raised by @gnanam1990) — this PR rewrites the exact geometry/mouse surface that #224 (resume picker), a planned clickable permission popup, and a planned right-docked side panel all touch. The right-docked panel is the one that would change
chatWidth(m.width)— anything that reserves width from the chat will want the frame rects'widthto come from a shared "content width" helper, notchatWidth(m.width)directly. If the side-panel PR is in flight, this PR should land first so the side panel stacks on top of the rect-based model. If the panel PR lands first, the rebase will be mechanical but tedious. Worth a quick sync with whoever's working on the panel. -
Inlined
chatWidth(m.width)in the frame construction (model.go:1380) andoverlayMouseRect(mouse.go:471). A future side-panel refactor that takes a content-width helper will want to thread it through here. Not a blocker for this PR; just worth a// TODO: thread content width here when the right-docked panel landsfor the next reader.
CI
All 6 smoke checks green (ubuntu / macos / windows / performance / security / CodeRabbit). "Zero Review" re-ran and is IN_PROGRESS as of this review; expect it to clear. Local ./internal/tui/... is green except for the pre-existing env-fragile TestModelCommandPersistsSelectedModelToUserConfig from PR #220 (fails only in dev envs with ANTHROPIC_* set, not this PR's fault).
Recommended follow-ups (separate PRs, NOT this one)
- The clickable permission popup and the right-docked side panel that @gnanam1990 mentions. Both benefit from the rect-based model this PR introduces; both can land as independent stacks on this branch as the PR description suggests.
- A
TestTranscriptFrameLayoutHandlesTransitionstest (tiny terminal ↔ full terminal) to lock in the clip math. The current tests cover tiny and full separately; an explicit transition test would be one more tripwire.
Solid foundation for the TUI backend hardening sequence. ✅
This was referenced Jun 23, 2026
This was referenced Jul 1, 2026
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
3 participants
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.
Summary
Notes
This is the single draft PR for the TUI backend hardening work. Follow-up commits for the transcript viewport/cache work should stack onto this branch instead of opening separate PRs.
Tests
Summary by CodeRabbit