fix(tui): render the ? help as an overlay, not a full-screen replace#420
Closed
gnanam1990 wants to merge 1 commit into
Closed
fix(tui): render the ? help as an overlay, not a full-screen replace#420gnanam1990 wants to merge 1 commit into
gnanam1990 wants to merge 1 commit into
Conversation
Pressing ? replaced the entire TUI with the shortcut block: View() short- circuited to renderKeybindingHelpOverlay, which centers the block on a blank canvas, so the transcript, title bar, and composer all vanished. Every other popup (model picker, suggestions, wizards) composites over the chat instead. Route the help overlay through the SAME viewport-overlay path the pickers use: add keybindingHelpOverlay(width) (returns "" when closed, else the centered block) as the highest-priority case in transcriptView's overlay switch and in twoColumnTranscriptView, and drop the content-replacement branch in View(). The overlay now draws on top of the chat via the existing overlayViewportLines compositing, keeping the transcript/title bar/composer visible behind it. Input handling is unchanged — the modal key-swallow + ?/Esc/q/Enter dismiss run before render — so only the presentation changes. Tests: the help overlay composites over the chat chrome (title bar + composer survive) and over a populated transcript row (red-green: with the old full-replace both the chrome and the body are gone); existing open/close/swallow/content tests unchanged. Fixes #419
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. |
WalkthroughThe help overlay is refactored to composite over the transcript viewport rather than fully replacing the screen. A new ChangesHelp overlay composition fix
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/model.go (1)
2183-2220: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlock mouse clicks while the help overlay is open —
handleMousestill routes through transcript hit-testing, so a click on a specialist card or AGENTS row can drill into subchat while?remains visible. Add am.helpOverlayguard before transcript/subchat mouse handling, or make the help overlay absorb clicks like the keyboard path does.🤖 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/model.go` around lines 2183 - 2220, The mouse handling path still processes transcript hit-testing while the help overlay is visible, so clicks can trigger transcript or subchat actions instead of being absorbed by the overlay. Update handleMouse to check m.helpOverlay (or the same state used by keybindingHelpOverlay) before any transcript/subchat click routing, and short-circuit so the help overlay blocks mouse interactions just like the keyboard flow.
🤖 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/keybinding_help_test.go`:
- Around line 108-160: Add a regression test for the two-column help overlay
path in the existing help overlay tests. The current coverage only exercises the
single-column transcript render, so create an alt-screen model with
sidebarActive() behavior enabled and helpOverlay set to true, then verify the
overlay still appears while the surrounding chat chrome/transcript remains
visible. Use the transcriptView() and twoColumnTranscriptView() paths as the key
symbols to target this branch.
---
Outside diff comments:
In `@internal/tui/model.go`:
- Around line 2183-2220: The mouse handling path still processes transcript
hit-testing while the help overlay is visible, so clicks can trigger transcript
or subchat actions instead of being absorbed by the overlay. Update handleMouse
to check m.helpOverlay (or the same state used by keybindingHelpOverlay) before
any transcript/subchat click routing, and short-circuit so the help overlay
blocks mouse interactions just like the keyboard flow.
🪄 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
Run ID: 6bdccb43-1d79-4526-a9d3-57cf60a30617
📒 Files selected for processing (3)
internal/tui/keybinding_help.gointernal/tui/keybinding_help_test.gointernal/tui/model.go
Comment on lines
+108
to
+160
|
|
||
| // #419: the `?` help overlay must render ON TOP of the chat (like the model | ||
| // picker), not REPLACE the whole screen. The old full-screen replace produced | ||
| // only the centered shortcut block on a blank canvas — no title bar, no | ||
| // composer. So with the overlay open, the surrounding chat chrome that is NOT | ||
| // covered by the centered box (the model title bar at top, the composer at | ||
| // bottom) must still be present alongside "Keyboard Shortcuts". | ||
| func TestHelpOverlayCompositesOverChatNotReplacingIt(t *testing.T) { | ||
| m := newModel(context.Background(), Options{ModelName: "gpt-4o"}) | ||
| m.width = 100 | ||
| m.height = 40 | ||
| m.altScreen = true | ||
|
|
||
| base := plainRender(t, m.View()) // no overlay: baseline chrome | ||
| m.helpOverlay = true | ||
| over := plainRender(t, m.View()) | ||
|
|
||
| if !strings.Contains(over, "Keyboard Shortcuts") { | ||
| t.Fatalf("help overlay not rendered:\n%s", over) | ||
| } | ||
| // Chrome that renders in the baseline (and sits outside the centered overlay | ||
| // box) must survive behind the overlay. The full-screen replace showed none | ||
| // of it. | ||
| for _, marker := range []string{"gpt-4o", "describe a task"} { | ||
| if !strings.Contains(base, marker) { | ||
| t.Fatalf("precondition: baseline chat should contain %q:\n%s", marker, base) | ||
| } | ||
| if !strings.Contains(over, marker) { | ||
| t.Fatalf("#419: help replaced the chat instead of overlaying it; %q is gone:\n%s", marker, over) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // A populated transcript row also survives behind the overlay (peeking out to | ||
| // the left of the centered box), proving the chat body — not just the chrome — | ||
| // is composited under the overlay rather than discarded. | ||
| func TestHelpOverlayKeepsTranscriptBodyBehindIt(t *testing.T) { | ||
| m := newModel(context.Background(), Options{ModelName: "gpt-4o"}) | ||
| m.width = 120 | ||
| m.height = 40 | ||
| m.altScreen = true | ||
| m.transcript = appendTranscriptRow(m.transcript, transcriptRow{kind: rowUser, text: "hello there this is a chat line"}) | ||
| m.helpOverlay = true | ||
|
|
||
| view := plainRender(t, m.View()) | ||
| if !strings.Contains(view, "Keyboard Shortcuts") { | ||
| t.Fatalf("help overlay not rendered:\n%s", view) | ||
| } | ||
| // The start of the transcript line peeks to the left of the centered box. | ||
| if !strings.Contains(view, "hello") { | ||
| t.Fatalf("#419: transcript body was replaced by the help overlay:\n%s", view) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find what conditions gate sidebarActive()/two-column rendering, to help craft a targeted test.
rg -n 'func \(m model\) sidebarActive' internal/tui -A20Repository: Gitlawb/zero
Length of output: 1692
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the view dispatch and the two-column path.
ast-grep outline internal/tui/model.go --view expanded | sed -n '1,220p'
printf '\n--- sidebar ---\n'
cat -n internal/tui/sidebar.go | sed -n '1,220p'
printf '\n--- tests mentioning help overlay or two-column ---\n'
rg -n 'helpOverlay|Keyboard Shortcuts|twoColumnTranscriptView|sidebarActive|sidebarAvailable' internal/tui -g '*_test.go' -A4 -B4Repository: Gitlawb/zero
Length of output: 37675
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the main view dispatch and the two-column transcript implementation.
rg -n 'func \(m model\) View|func \(m model\) transcriptView|func \(m model\) twoColumnTranscriptView|helpOverlay' internal/tui/model.go internal/tui/sidebar.go -A80 -B20Repository: Gitlawb/zero
Length of output: 38802
Add a regression test for the two-column help overlay path.
The new cases only cover the single-column transcript render; when sidebarActive() is true, transcriptView() switches to twoColumnTranscriptView(), and that branch now composites ? separately. Add one alt-screen model with an active sidebar and helpOverlay = true so this path stays covered.
🤖 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/keybinding_help_test.go` around lines 108 - 160, Add a
regression test for the two-column help overlay path in the existing help
overlay tests. The current coverage only exercises the single-column transcript
render, so create an alt-screen model with sidebarActive() behavior enabled and
helpOverlay set to true, then verify the overlay still appears while the
surrounding chat chrome/transcript remains visible. Use the transcriptView() and
twoColumnTranscriptView() paths as the key symbols to target this branch.
Collaborator
Author
|
Closing in favor of #421 by @N1xev — the reporter's own fix. Both PRs converged on the same core approach (route the help through the
Thanks @N1xev for diagnosing the layering root cause and fixing it. The one thing this PR carried that #421 doesn't is a regression test for the composite-not-replace behavior — I've left those two tests as a suggestion on #421 so the fix ships guarded. |
5 tasks
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
1 participant
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.
Fixes #419.
Problem
Pressing
?replaced the entire TUI with the keyboard-shortcut block — the transcript, title bar, and composer all vanished. Every other popup (model picker, suggestions, wizards) composites over the chat instead, and the issue asks for?to behave the same way.Root cause:
View()short-circuited onm.helpOverlaytorenderKeybindingHelpOverlay, which centers the block on a blank canvas viacenterRenderedBlock, so nothing else rendered.Fix
Route the help overlay through the same viewport-overlay path the pickers use:
keybindingHelpOverlay(width)— returns""when closed, else the centered block.transcriptView's overlay switch and intwoColumnTranscriptView(so it also composites correctly with the sidebar open).View().The overlay now draws on top of the chat via the existing
overlayViewportLinescompositing, keeping the transcript / title bar / composer visible behind it. Input handling is unchanged — the modal key-swallow and?/Esc/q/Enter dismiss run before render — so only the presentation changes.Testing
internal/tuisuite passes.?on the home screen now shows the title bar, the centered shortcuts box as an overlay, footer hints, and the composer; Esc dismisses cleanly with the chat intact.Before → after
Before:
?blanks the screen down to just the centered box.After: the shortcuts box floats over the chat like the model picker, with the title bar above and the composer below still visible.
Summary by CodeRabbit