fix(tui): compose help overlay through the viewport overlay pipeline#421
Conversation
exactly like how other popups works.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe help overlay now composes over the transcript view instead of replacing the full TUI. Its renderer signature is simplified, the sidebar is hidden while the overlay is active, and tests verify chat and transcript content remain visible behind it. ChangesHelp overlay compositing
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Model
participant TranscriptView
participant Sidebar
Model->>TranscriptView: View() routes helpOverlay to transcriptView()
TranscriptView->>TranscriptView: select helpOverlayContent as viewportOverlay
Model->>Sidebar: sidebarAvailable() checks m.helpOverlay
Sidebar-->>Model: hide right-hand sidebar
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/model.go (1)
2198-2222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a regression test for the help+subchat interaction.
Since the subchat early-return silently drops the help overlay (see above), a small test asserting the expected behavior (either "help renders in subchat" or "help is deferred/no-op in subchat") would lock in intent and catch future regressions in this compositing switch.
🤖 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 2198 - 2222, The overlay compositing in m.render() currently has ambiguous behavior when subchat triggers an early return and helpOverlayContent is involved, so add a regression test that locks in the intended help+subchat interaction. Use the existing overlay selection logic around helpOverlayContent, suggestionOverlay, providerOverlay, mcpAddOverlay, mcpOverlay, and pickerOverlay to verify whether help should render in subchat or be deferred/no-op, and assert that behavior explicitly so future changes don’t silently drop the help overlay.
🤖 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.
Nitpick comments:
In `@internal/tui/model.go`:
- Around line 2198-2222: The overlay compositing in m.render() currently has
ambiguous behavior when subchat triggers an early return and helpOverlayContent
is involved, so add a regression test that locks in the intended help+subchat
interaction. Use the existing overlay selection logic around helpOverlayContent,
suggestionOverlay, providerOverlay, mcpAddOverlay, mcpOverlay, and pickerOverlay
to verify whether help should render in subchat or be deferred/no-op, and assert
that behavior explicitly so future changes don’t silently drop the help overlay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fbc1b4b3-b34c-4d4e-9f40-27981ef48ced
📒 Files selected for processing (3)
internal/tui/keybinding_help.gointernal/tui/model.gointernal/tui/sidebar.go
kevincodex1
left a comment
kevincodex1
left a comment
There was a problem hiding this comment.
nice one! thank you so much
|
kindly please check the failing test |
done! it was the formatting CI/CD step |
|
Thanks for this, @N1xev — and for the precise root-cause writeup in the issue. Routing the overlay through the same The one thing #420 carried that this doesn't is a regression test that pins the fix. The existing help tests cover close/swallow/content, but none of them assert that the chat survives behind the overlay — so a future refactor could silently bring back the full-screen blank without any test going red. Here are two tests that cover exactly that. I checked out this branch and ran them against your implementation — both pass ( Two regression tests (verified against this branch)// #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. With the overlay open, 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.
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)
}
}The first asserts the title bar + composer survive alongside "Keyboard Shortcuts"; the second that a transcript line peeks out beside the centered box. Both fail against the old full-screen replace and pass with your fix. Looks good to me once something like this is in. |
thanks! i've added the regression tests and pushed them. |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Approving — this is the right fix for #419. Routing the overlay through the same viewportOverlay path as the pickers/wizards, and suppressing the sidebar via sidebarAvailable() so it renders full-width, is exactly the "like the other popups" behavior the issue asked for.
The only ask before merge is the regression test I left in the comment above, so the composite-not-replace behavior stays guarded against a future refactor. Thanks for the clean root-cause diagnosis and the fix.
i added them and pushed, check them. |
3 participants
exactly like how other popups works.
before:
after:
Summary
Linked issue
Fixes #419
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Summary by CodeRabbit