fix(tui): per-route vertical reserve + required panel props (#1069 round-2 follow-up) - #1071
fix(tui): per-route vertical reserve + required panel props (#1069 round-2 follow-up)#1071saravmajestic wants to merge 1 commit into
Conversation
…nd-2 review) Follow-up to the merged #1069, addressing the round-2 review. - Split the vertical reserve per route: HOME_VERTICAL_RESERVE (10) and SESSION_VERTICAL_RESERVE (7) accurately reflect each route's chrome (home has a top spacer + footer; session doesn't), instead of one constant that fit only session. Thresholds re-expressed in usable terms (MEDIUM_MIN_HEIGHT 13, FULL_MIN_HEIGHT 34) so `full` still engages at a ~44-row terminal on home and the classic 80x24 clears the medium floor with margin. - Make WelcomePanel's availableWidth/availableHeight props REQUIRED (drop the raw-terminal fallback + the component's own useTerminalDimensions), so a call site that forgets to scale is a compile error, not a silent regression. - Height now gets the same available-space treatment as width (terminal minus the route's prompt/footer chrome), so a short window shrinks the panel instead of crowding the prompt. - Centralize PANEL_HORIZONTAL_PADDING (4) — was a bare `-4` in the home call site and test helper. Tests: add the sidebar-toggle transition (session 150x50 full -> medium when the sidebar opens — the exact #1067 case), the classic 80x24 on both routes, and keep the isolated-gate / boundary / degenerate coverage. tsgo clean; oxlint 0/0. Reviewed by a 2-model consensus panel (both SOUND); the PANEL_HORIZONTAL_PADDING and sidebar-toggle test came out of that review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughWelcome panel sizing now uses caller-provided available dimensions. Home and session routes apply separate vertical reserves and shared horizontal padding. Responsive height thresholds and route-specific boundary tests were updated. ChangesWelcome Panel Sizing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Home
participant SessionRoute
participant WelcomePanel
participant welcomePanelVariant
Home->>WelcomePanel: provide available width and height
SessionRoute->>WelcomePanel: provide available width and height
WelcomePanel->>welcomePanelVariant: calculate responsive variant
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Notes:
Reviewed by glm-5.2 · Input: 59.7K · Output: 9.9K · Cached: 379.6K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/tui/test/component/welcome-panel-utils.test.ts">
<violation number="1" location="packages/tui/test/component/welcome-panel-utils.test.ts:21">
P3: The session test helper duplicates the exact width formula (including the magic sidebar width 42) that lives inline in routes/session/index.tsx:275. If the sidebar width or content-column math changes in the component, this mirror silently diverges and the #1067 pin no longer guards the real path. Consider exporting the sidebar width/formula from welcome-panel-utils (as was done for PANEL_HORIZONTAL_PADDING) and importing it in both the component and this test so they stay in sync.</violation>
</file>
<file name="packages/tui/src/component/welcome-panel-utils.ts">
<violation number="1" location="packages/tui/src/component/welcome-panel-utils.ts:37">
P3: The padding centralization is incomplete: PANEL_HORIZONTAL_PADDING is used at the home call site, but the session route's contentWidth still hardcodes the bare `- 4` rather than referencing the constant (the new doc comment even acknowledges session "applies the same 4 as part of its own content-column math"). If this constant ever changes, the session width math won't follow and drifts out of sync; consider having session/index.tsx import PANEL_HORIZONTAL_PADDING so both routes share one source of truth.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| // Map a terminal size to the AVAILABLE size each route feeds the pure function. | ||
| const home = (w: number, h: number) => welcomePanelVariant(w - PANEL_HORIZONTAL_PADDING, h - HOME_VERTICAL_RESERVE) | ||
| const session = (w: number, h: number, sidebar: boolean) => |
There was a problem hiding this comment.
P3: The session test helper duplicates the exact width formula (including the magic sidebar width 42) that lives inline in routes/session/index.tsx:275. If the sidebar width or content-column math changes in the component, this mirror silently diverges and the #1067 pin no longer guards the real path. Consider exporting the sidebar width/formula from welcome-panel-utils (as was done for PANEL_HORIZONTAL_PADDING) and importing it in both the component and this test so they stay in sync.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/test/component/welcome-panel-utils.test.ts, line 21:
<comment>The session test helper duplicates the exact width formula (including the magic sidebar width 42) that lives inline in routes/session/index.tsx:275. If the sidebar width or content-column math changes in the component, this mirror silently diverges and the #1067 pin no longer guards the real path. Consider exporting the sidebar width/formula from welcome-panel-utils (as was done for PANEL_HORIZONTAL_PADDING) and importing it in both the component and this test so they stay in sync.</comment>
<file context>
@@ -2,18 +2,25 @@ import { expect, test } from "bun:test"
+// Map a terminal size to the AVAILABLE size each route feeds the pure function.
+const home = (w: number, h: number) => welcomePanelVariant(w - PANEL_HORIZONTAL_PADDING, h - HOME_VERTICAL_RESERVE)
+const session = (w: number, h: number, sidebar: boolean) =>
+ welcomePanelVariant(w - (sidebar ? 42 : 0) - PANEL_HORIZONTAL_PADDING, h - SESSION_VERTICAL_RESERVE)
+
</file context>
| // Columns the home slot spends on its own left/right padding (2 + 2); the caller | ||
| // subtracts this to get the panel's usable width. (Session's contentWidth applies | ||
| // the same 4 as part of its own content-column math.) | ||
| export const PANEL_HORIZONTAL_PADDING = 4 |
There was a problem hiding this comment.
P3: The padding centralization is incomplete: PANEL_HORIZONTAL_PADDING is used at the home call site, but the session route's contentWidth still hardcodes the bare - 4 rather than referencing the constant (the new doc comment even acknowledges session "applies the same 4 as part of its own content-column math"). If this constant ever changes, the session width math won't follow and drifts out of sync; consider having session/index.tsx import PANEL_HORIZONTAL_PADDING so both routes share one source of truth.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/tui/src/component/welcome-panel-utils.ts, line 37:
<comment>The padding centralization is incomplete: PANEL_HORIZONTAL_PADDING is used at the home call site, but the session route's contentWidth still hardcodes the bare `- 4` rather than referencing the constant (the new doc comment even acknowledges session "applies the same 4 as part of its own content-column math"). If this constant ever changes, the session width math won't follow and drifts out of sync; consider having session/index.tsx import PANEL_HORIZONTAL_PADDING so both routes share one source of truth.</comment>
<file context>
@@ -7,36 +7,46 @@
+// Columns the home slot spends on its own left/right padding (2 + 2); the caller
+// subtracts this to get the panel's usable width. (Session's contentWidth applies
+// the same 4 as part of its own content-column math.)
+export const PANEL_HORIZONTAL_PADDING = 4
-/** Minimum usable size for the medium panel; below either → compact (one line). */
</file context>
1 participant
Follow-up to the merged #1069, addressing @sahrizvi's round-2 review. Also passed a 2-model consensus panel (Claude + Gemini) — both SOUND.
What changed (from the round-2 comments)
MINOR — reserve fit only session, under-reserved home. Split into per-route constants that match each route's actual chrome:
HOME_VERTICAL_RESERVE = 10(top spacer 2 + prompt wrapper 1 + prompt ~4 + footer ~3)SESSION_VERTICAL_RESERVE = 7(gaps 2 + paddingBottom 1 + prompt ~4; no top spacer/footer in that column)Thresholds re-expressed in usable terms (
MEDIUM_MIN_HEIGHT 13,FULL_MIN_HEIGHT 34) sofullstill engages at a ~44-row terminal on home, and the classic 80×24 clears the medium floor with margin (the cliff the review flagged now sits at 23, off the common size).MINOR — optional props silently defaulted to raw terminal. Made
availableWidth/availableHeightrequired and dropped the component's ownuseTerminalDimensions+ fallback. A call site that forgets to scale is now a compile error, not a silent regression — this is the type-level guard for the "no render test" finding.NITs
fullengages ~3 rows sooner on session (≥41) than home (≥44) — intentional (session has less chrome, so more usable height).From the consensus panel (both models endorsed)
PANEL_HORIZONTAL_PADDING = 4— was a bare-4in the home call site + test helper (vertical reserves were already named constants).session(150,50,false) → fullvssession(150,50,true) → medium— pins the exact home_logo slot always visible and cannot be dismissed in TUI #1067 case directly.Effective terminal → variant (verified)
Verification
tsgoclean · 12 unit + 11 render/adjacent tests pass · oxlint 0/0 · prettier. Gate-isolation tests still kill mutants (deleting or<→<=-flipping any gate fails a test).Deferred (noted by both reviewers): a session-with-sidebar render test as the true call-site regression guard — happy to add as a follow-up.
🤖 Generated with Claude Code
Summary by cubic
Fixes welcome panel sizing (AI-1067) so variants are chosen by available space, not raw terminal size. Prevents “full” from appearing with the session sidebar or on short terminals.
Bug Fixes
HOME_VERTICAL_RESERVE = 10,SESSION_VERTICAL_RESERVE = 7.MEDIUM_MIN_HEIGHT = 13,FULL_MIN_HEIGHT = 34.PANEL_HORIZONTAL_PADDING = 4.Migration
WelcomePanelnow requiresavailableWidthandavailableHeight.HOME_VERTICAL_RESERVEorSESSION_VERTICAL_RESERVE.Written for commit 2eac798. Summary will update on new commits.
Summary by CodeRabbit