Migrate TUI overlays onto the design language (Epic 2.1) - #136
Conversation
renderOverlayBox now draws a rounded, tinted (info/danger) modal composed from theme tokens, opaque by overwriting each cell, with ANSI-aware content padding. Overlay bodies tokenize titles, dim hints, and render key hints as keycaps; help and switcher route through the shared box; the notification panel gets a rounded two-pane border with styled list/detail cells. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
More reviews will be available in 1 hour, 22 minutes, and 2 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesTUI Overlay Theme Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
🧹 Nitpick comments (2)
src/tui/render/box.test.ts (1)
23-27: 💤 Low valueTest name implies width verification that isn't actually performed.
The test name mentions "without breaking width," but the assertion only confirms the visible content is present (
stripAnsi(out).toContain("bold plain")). To verify width behavior, consider adding an assertion that checks padding/truncation—e.g., verifying the output row length after stripping ANSI matches the expectedboxWidth.That said, the current assertion does validate ANSI preservation which is valuable. Consider either renaming to "preserves ANSI styling in body content" or adding a width-specific check.
🤖 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 `@src/tui/render/box.test.ts` around lines 23 - 27, The test "preserves ANSI styling in body content without breaking width" has a name that claims to verify width behavior, but the only assertion checks content presence via stripAnsi(out).toContain("bold plain"), not actual width. Either rename the test to "preserves ANSI styling in body content" to accurately reflect what it tests, or add an additional assertion that verifies width behavior (e.g., checking that stripAnsi(out) lines match the expected boxWidth after stripping ANSI codes). Choose the approach that best reflects the test's primary intent.src/tui/screens/overlay-renderers.ts (1)
199-201: 💤 Low valueWidth constraints can exceed available space on narrow terminals.
When
widthis small, the minimum bounds (leftWidth ≥ 28,rightWidth ≥ 20) forceboxWidthto at least 55 columns regardless of actual terminal width. For a 40-column terminal, this would cause overflow.Consider capping
boxWidthtowidthor adjusting the minimums relative to available space:const leftWidth = Math.max(28, Math.floor((width - 7) * 0.5)); const rightWidth = Math.max(20, width - 7 - leftWidth); - const boxWidth = leftWidth + rightWidth + 7; + const boxWidth = Math.min(width, leftWidth + rightWidth + 7);This is an edge case for unusually narrow terminals, so it may be acceptable to defer.
🤖 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 `@src/tui/screens/overlay-renderers.ts` around lines 199 - 201, The width calculation in the overlay renderer is causing boxWidth to potentially exceed the available terminal width on narrow displays. The minimum constraints applied to leftWidth (28) and rightWidth (20) force a minimum boxWidth of 55 columns, which can overflow terminals narrower than this. Fix this by either capping the final boxWidth calculation to not exceed the available width parameter, or by adjusting the minimum width values (28 and 20) to be relative to and respect the available space. Ensure that leftWidth, rightWidth, and the resulting boxWidth remain constrained within the actual terminal width.
🤖 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 `@src/tui/render/box.test.ts`:
- Around line 23-27: The test "preserves ANSI styling in body content without
breaking width" has a name that claims to verify width behavior, but the only
assertion checks content presence via stripAnsi(out).toContain("bold plain"),
not actual width. Either rename the test to "preserves ANSI styling in body
content" to accurately reflect what it tests, or add an additional assertion
that verifies width behavior (e.g., checking that stripAnsi(out) lines match the
expected boxWidth after stripping ANSI codes). Choose the approach that best
reflects the test's primary intent.
In `@src/tui/screens/overlay-renderers.ts`:
- Around line 199-201: The width calculation in the overlay renderer is causing
boxWidth to potentially exceed the available terminal width on narrow displays.
The minimum constraints applied to leftWidth (28) and rightWidth (20) force a
minimum boxWidth of 55 columns, which can overflow terminals narrower than this.
Fix this by either capping the final boxWidth calculation to not exceed the
available width parameter, or by adjusting the minimum width values (28 and 20)
to be relative to and respect the available space. Ensure that leftWidth,
rightWidth, and the resulting boxWidth remain constrained within the actual
terminal width.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5c540d7-9adf-49ec-9c54-09eb2db13f1e
📒 Files selected for processing (3)
src/tui/render/box.test.tssrc/tui/render/box.tssrc/tui/screens/overlay-renderers.ts
Address CodeRabbit nitpick: the box test now verifies every modal row is padded/truncated to the same visible width (and within cols), instead of only checking content presence. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed CodeRabbit nitpicks in 46d24b6:
|
1 participant
Summary
Epic 2, phase 1 of the TUI design-language rollout. Migrates the overlay surfaces off hand-written escape codes onto the shared theme primitives from
src/tui/render/theme.ts.renderOverlayBoxnow draws a rounded, tinted modal (info/dangertone) composed from theme tokens, opaque by overwriting each cell, with ANSI-aware content padding (padVisible/visibleWidth).strong), secondary text (muted), and render key hints askeycaps via a sharedhints()helper.renderOverlayBoxinstead of hand-rolling their own boxes.statusDotread/unread markers.Behavior preserved
Same triggering conditions, null-return guards, keybindings, and content — purely a presentation migration.
Verification
yarn typecheck,yarn lint,yarn buildclean; full suite 1256 tests pass (newbox.test.ts).🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Refactor