feat(tui): add 'open' border mode to TerminalStyle#6368
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughThis PR implements Phase 0 of the TUI audit roadmap, introducing a copy-clean "open" border style, fixing render-state caching, preventing event-loop blocking via resource cleanup, and fixing bugs in text handling, selection, and fuzzy matching across the pi-tui and pi-coding-agent packages. ChangesTUI Audit Phase 0 Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 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 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 skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
Contributor
🟠 PR Risk Report — HIGH
Affected Systems
File Breakdown
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/pi-tui/src/__tests__/style.test.ts (1)
106-117: ⚡ Quick winAdd a long-title regression for open rule width clamping.
This test only exercises a width where both titles fit. Add one case with overlong left/right titles and assert
visibleWidth(plain[0])still equals the requested width to prevent overflow regressions.🤖 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 `@packages/pi-tui/src/__tests__/style.test.ts` around lines 106 - 117, Add a regression case to the existing test that verifies width clamping when titles are too long: in the "open border places left and right titles in the top rule" test, after the current assertions, call style().border("open").title(longLeftTitle).titleRight(longRightTitle).render(["output"], 40).map(stripAnsi) using deliberately overlong strings for longLeftTitle and longRightTitle, then assert visibleWidth(plain[0]) === 40 (and optionally that the rendered line still contains truncated/partial title fragments) to ensure the open-rule top border clamps to the requested width rather than overflowing.packages/pi-tui/src/tui.ts (1)
663-667: ⚡ Quick winTrack visible overlays (not stack length) in the fast-path gate.
The new gate is intended to model “was an overlay composited last frame”, but Line 665 / Line 671 use
this.overlayStack.length > 0. Hidden or non-visible overlays keep the optimization disabled unnecessarily.♻️ Proposed adjustment
// Render all components to get new lines let newLines = this.render(width); + const hasVisibleOverlays = this.hasOverlay(); if ( newLines === this._lastRenderedComponents && - this.overlayStack.length === 0 && + !hasVisibleOverlays && !this._lastFrameHadOverlays ) { return; } this._lastRenderedComponents = newLines; - this._lastFrameHadOverlays = this.overlayStack.length > 0; + this._lastFrameHadOverlays = hasVisibleOverlays; // Composite overlays into the rendered lines (before differential compare) - if (this.overlayStack.length > 0) { + if (hasVisibleOverlays) { newLines = compositeOverlays(newLines, this.overlayStack, width, height, this.maxLinesRendered); }Also applies to: 671-676
🤖 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 `@packages/pi-tui/src/tui.ts` around lines 663 - 667, The fast-path gate currently checks overlayStack.length to decide if an overlay was composited, which is wrong for hidden/non-visible overlays; replace those length checks with a computed boolean that tests whether any overlay in overlayStack is actually visible/composited (e.g., const currentFrameHasVisibleOverlays = overlayStack.some(o => o.visible /* or o.isVisible / o.shouldComposite as appropriate */)), use currentFrameHasVisibleOverlays in the two conditional expressions (where overlayStack.length > 0 is used), and set _lastFrameHadOverlays = currentFrameHasVisibleOverlays instead of using the stack length.
🤖 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 `@packages/pi-tui/src/style.ts`:
- Around line 227-253: The renderOpenTopRule function can produce output wider
than the requested width when titles are long because fill is forced to at least
1 and titles are never truncated; fix it by clamping/truncating title text to
available space before computing fill (use visibleWidth to measure and a
truncation helper to shorten left/right titles while preserving color via
color()), compute fill = Math.max(0, w - fixed) instead of min 1, and recalc
fixed after truncation so the concatenated pieces always equal the requested
width; update the branches in renderOpenTopRule to truncate left/right (or both)
as needed and only then build the final string.
---
Nitpick comments:
In `@packages/pi-tui/src/__tests__/style.test.ts`:
- Around line 106-117: Add a regression case to the existing test that verifies
width clamping when titles are too long: in the "open border places left and
right titles in the top rule" test, after the current assertions, call
style().border("open").title(longLeftTitle).titleRight(longRightTitle).render(["output"],
40).map(stripAnsi) using deliberately overlong strings for longLeftTitle and
longRightTitle, then assert visibleWidth(plain[0]) === 40 (and optionally that
the rendered line still contains truncated/partial title fragments) to ensure
the open-rule top border clamps to the requested width rather than overflowing.
In `@packages/pi-tui/src/tui.ts`:
- Around line 663-667: The fast-path gate currently checks overlayStack.length
to decide if an overlay was composited, which is wrong for hidden/non-visible
overlays; replace those length checks with a computed boolean that tests whether
any overlay in overlayStack is actually visible/composited (e.g., const
currentFrameHasVisibleOverlays = overlayStack.some(o => o.visible /* or
o.isVisible / o.shouldComposite as appropriate */)), use
currentFrameHasVisibleOverlays in the two conditional expressions (where
overlayStack.length > 0 is used), and set _lastFrameHadOverlays =
currentFrameHasVisibleOverlays instead of using the stack length.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fb93238-368d-49dc-b8fa-ff46d57c2d87
📒 Files selected for processing (16)
docs/adr/0001-unify-tui-style-system.mddocs/tui-audit.mdpackages/pi-coding-agent/src/modes/interactive/components/armin.tspackages/pi-coding-agent/src/modes/interactive/components/countdown-timer.tspackages/pi-coding-agent/src/modes/interactive/components/daxnuts.tspackages/pi-coding-agent/src/modes/interactive/components/dynamic-border.tspackages/pi-coding-agent/src/modes/interactive/components/model-selector.tspackages/pi-coding-agent/src/modes/interactive/components/session-selector.tspackages/pi-coding-agent/src/modes/interactive/interactive-mode.tspackages/pi-tui/src/__tests__/style.test.tspackages/pi-tui/src/components/input.tspackages/pi-tui/src/components/markdown.tspackages/pi-tui/src/components/select-list.tspackages/pi-tui/src/fuzzy.tspackages/pi-tui/src/style.tspackages/pi-tui/src/tui.ts
Comment on lines
+227
to
+253
| private renderOpenTopRule(width: number, borderColor: (text: string) => string): string { | ||
| const w = Math.max(1, width); | ||
| const left = this.spec.title ?? ""; | ||
| const right = this.spec.titleRight ?? ""; | ||
| if (!left && !right) return borderColor("─".repeat(w)); | ||
|
|
||
| const styledLeft = color(this.spec.titleColor, left); | ||
| const styledRight = color(this.spec.titleRightColor, right); | ||
|
|
||
| if (left && right) { | ||
| const fixed = 4 + visibleWidth(left) + 2 + visibleWidth(right) + 4; | ||
| const fill = Math.max(1, w - fixed); | ||
| return ( | ||
| borderColor("─── ") + | ||
| styledLeft + | ||
| borderColor(` ${"─".repeat(fill)} `) + | ||
| styledRight + | ||
| borderColor(" ───") | ||
| ); | ||
| } | ||
| if (left) { | ||
| const fill = Math.max(1, w - 5 - visibleWidth(left)); | ||
| return borderColor("─── ") + styledLeft + borderColor(` ${"─".repeat(fill)}`); | ||
| } | ||
| const fill = Math.max(1, w - 5 - visibleWidth(right)); | ||
| return borderColor(`${"─".repeat(fill)} `) + styledRight + borderColor(" ───"); | ||
| } |
Contributor
There was a problem hiding this comment.
Clamp open-rule titles to preserve exact line width.
Line 237 through Line 252 can generate lines wider than width when titles are long (titles are never truncated, and fill is forced to at least 1). That can wrap and break the surface contract.
💡 Proposed fix
private renderOpenTopRule(width: number, borderColor: (text: string) => string): string {
const w = Math.max(1, width);
- const left = this.spec.title ?? "";
- const right = this.spec.titleRight ?? "";
+ let left = this.spec.title ?? "";
+ let right = this.spec.titleRight ?? "";
if (!left && !right) return borderColor("─".repeat(w));
+ if (w < 8) return borderColor("─".repeat(w));
- const styledLeft = color(this.spec.titleColor, left);
- const styledRight = color(this.spec.titleRightColor, right);
+ if (left && right) {
+ const titleBudget = Math.max(1, w - 11); // decorations + at least 1 fill rune
+ right = truncateToWidth(right, Math.max(1, Math.floor(titleBudget / 2)), "");
+ left = truncateToWidth(left, Math.max(1, titleBudget - visibleWidth(right)), "");
+ } else {
+ const singleBudget = Math.max(1, w - 6);
+ left = truncateToWidth(left, singleBudget, "");
+ right = truncateToWidth(right, singleBudget, "");
+ }
+
+ const styledLeft = color(this.spec.titleColor, left);
+ const styledRight = color(this.spec.titleRightColor, right);🤖 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 `@packages/pi-tui/src/style.ts` around lines 227 - 253, The renderOpenTopRule
function can produce output wider than the requested width when titles are long
because fill is forced to at least 1 and titles are never truncated; fix it by
clamping/truncating title text to available space before computing fill (use
visibleWidth to measure and a truncation helper to shorten left/right titles
while preserving color via color()), compute fill = Math.max(0, w - fixed)
instead of min 1, and recalc fixed after truncation so the concatenated pieces
always equal the requested width; update the branches in renderOpenTopRule to
truncate left/right (or both) as needed and only then build the final string.
The doRender() short-circuit skipped all post-processing when component output was byte-identical to the previous frame. If the previous frame composited an overlay onto the screen, an identical next frame with no overlay would short-circuit and never erase it, leaving the overlay stuck on screen. Track _lastFrameHadOverlays and force a redraw when the previous frame drew an overlay, even if base component output is unchanged.
When a filter matched no items, selectUp set selectedIndex to filteredItems.length - 1 (i.e. -1), corrupting list state until the next setFilter() reset it. Skip navigation and selection entirely when the filtered list is empty; still honor cancel.
renderList() identified nested-list lines with the regex /^\s+\x1b\[36m[-\d]/ — assuming the list bullet is always ANSI cyan. Any theme with a different bullet color silently broke nested-list indentation. renderListItem() now returns ListItemLine[] with a 'nested' flag set on lines produced by recursive renderList() calls, so renderList() keys off structure instead of pattern-matching theme output.
yankPop() computed the deletion start as cursor - prevText.length with no floor. A negative index passed to String.slice() counts from the end of the string, so an underflow would silently corrupt the input value rather than delete nothing. Clamp the start index to 0.
fuzzyMatch() indexed strings with [i], which yields UTF-16 code units. Astral-plane characters (emoji, rare CJK) occupy two code units, so a single such character was compared as two lone surrogates and never matched. Iterate over Array.from() code-point arrays instead.
The daxnuts easter-egg image emitted 24-bit truecolor SGR sequences (\x1b[38;2;r;g;bm) unconditionally. On 256/16-color terminals (many containers, remote sessions, Apple Terminal) those render incorrectly. Add an rgbTo256() quantizer; rgb() now emits 256-color SGR when theme.getColorMode() reports the terminal is not truecolor.
The shutdown path called onThemeChange(() => {}), which registered a
new empty listener instead of removing the one added at startup,
leaking theme-change subscribers. Store the unsubscriber returned by
onThemeChange() and invoke it during cleanup.
Timer-hygiene pass over animated/timed TUI components: - DynamicBorder: add dispose() that stops the spinner. Without it, a spinner started via startSpinner() keeps firing its interval and calling ui.requestRender() after the border is detached. - SessionSelectorHeader: add dispose() that clears the status-message auto-hide timeout, so it cannot fire requestRender() post-removal. - CountdownTimer / Armin: unref() the interval so a cosmetic timer never pins the Node event loop on its own if dispose() is missed. terminal.ts:199 was also flagged but is correct as-is — the fallback setTimeout body is guarded by !_kittyProtocolActive, so it is a no-op when the protocol is confirmed before the 150ms deadline.
Provider-header and model rows were built with Text, which word-wraps. A model id longer than the overlay width wrapped onto extra lines, pushing rows out of the fixed 12-row selection window and leaving the selection cursor on a different line from the rest of the row. Use TruncatedText for header and model rows so an over-long id is truncated to one line. The multi-line 'No providers' help text keeps using Text intentionally — it should wrap.
jeremymcs
force-pushed
the
tui-p2/01-open-border-mode
branch
from
4fa36b7 to
8e38045
Compare
ADR-019. Two converging problems: (1) users cannot cleanly copy TUI output because every content line carries a border/rail prefix that the terminal copies with the text; (2) four independent border implementations and three message container styles. Decision: content surfaces (message turns, tool/bash output, summaries) render copy-clean with horizontal-rule framing and zero leading characters on body lines. One border primitive (TerminalStyle, with a new 'open' mode); one app vocabulary module; two surfaces by role — Open (content, copy-clean) and Panel (interactive, bordered). Rail and box-background styles eliminated. Seven-step stacked-PR migration plan. Status: Accepted.
First step of the ADR-019 style-system migration. Adds an 'open' border mode for copy-clean content surfaces: - A titled top rule (── bash · success ──── 1.2s ──) via the new renderOpenTopRule helper. - Body lines emitted verbatim with no border column and no prefix, so selecting a body line in the terminal copies only its content. - A plain closing rule. Purely additive — no existing consumer uses 'open' yet. Unit tests assert body lines never gain a leading border glyph.
jeremymcs
force-pushed
the
tui-p2/01-open-border-mode
branch
from
8e38045 to
21277ed
Compare
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.
TUI Phase 2 — migration step 1/7
Implements step 1 of the ADR-019 migration plan (#6367). Stacked on the ADR branch.
What
Adds an
openborder mode toTerminalStyle— the copy-clean content surface the ADR calls for:─── bash · success ─────────── 1.2s ───, via a newrenderOpenTopRulehelper. Supports left title, right title, or neither.Why additive-only
No existing component uses
openyet — this PR just lands the primitive. Steps 2–7 migrate consumers onto it.Tests
3 new tests in
style.test.ts, including an explicit assertion that no body line starts with a border glyph (│/┃/─) — the copy-cleanliness contract. Fullstylesuite: 9/9 pass.tscclean.Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Documentation