Repository navigation
feat(terminal): native theming and chrome-free pane rebuild - #1965
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces a shared CSS color resolution module and a new xterm theme hook consumed by the terminal component, refactors Monaco's fallback palette to reuse that module, and reworks terminal panes into a column-based split layout (splitRight/splitDown) with chrome-free UI styling, along with corresponding reducer, store, and test updates. ChangesTerminal and Monaco Theming
Chrome-free Terminal Panes and Column-based Splitting
Estimated code review effort: 4 (Complex) | ~55 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Round 1 of the Terminal Command Center UI epic: makes the Terminal feature feel native to PageSpace instead of xterm's raw green-tinted default look, and rebuilds pane chrome to match PurePoint's real chrome-free design. - Extract useMonacoTheme's CSS-var-to-hex resolution pipeline into a shared apps/web/src/lib/theme/css-color-resolution.ts util (no behavior change). - Add useXtermTheme, reactive to next-themes' resolvedTheme, with a curated muted ANSI palette per light/dark mode. - Wire theme + typography into XtermTerminal; live theme updates via terminal.options.theme without remounting the PTY connection. - Rebuild TerminalPanes chrome-free (no per-pane header, ever): top accent-bar focus indicator, hover-revealed Split Right/Split Down/Close controls, design tokens instead of hardcoded black/white and red/green states. - Add a real vertical split axis: workspace-reducer now models a horizontal row of columns, each an independent vertical stack of panes, via splitRight/splitDown. Rendered as nested ResizablePanelGroups over the existing resizable primitives. - Add a chrome-free ResizableHandle variant so the seam stays faintly visible at rest (chrome-free panes have no other cue), without touching the global default used elsewhere in the app. Tests: reducer/store tests rewritten for the columns structure (28 tests, given/should style). typecheck + build green across all 16 workspace packages.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0581006178
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| </span> | ||
| <div className="group/pane relative flex h-full flex-col" onClick={onSelect}> | ||
| <div className={`absolute inset-x-0 top-0 z-10 h-0.5 ${isActive ? 'bg-primary' : 'bg-transparent'}`} /> | ||
| <div className="absolute right-1.5 top-1.5 z-10 flex items-center gap-0.5 rounded-md border border-border bg-card/90 p-0.5 opacity-0 shadow-sm backdrop-blur-sm transition-opacity group-hover/pane:opacity-100"> |
There was a problem hiding this comment.
When a user navigates the terminal pane controls with the keyboard, this toolbar stays opacity-0 because it only becomes visible on group-hover; the split/close buttons are still focusable since opacity does not remove them from the tab order. That leaves keyboard users tabbing to invisible controls (and touch users can also hit invisible buttons before any hover state appears), so add a focus-within visibility state or otherwise keep the controls out of the interaction flow while hidden.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Good catch — fixed in the next commit: added group-focus-within/pane:opacity-100 alongside the existing group-hover/pane:opacity-100, matching the pattern already used elsewhere in this codebase (sidebar.tsx, MessageHoverToolbar.tsx, NotificationItem.tsx). Now the toolbar becomes visible as soon as keyboard focus lands on any control inside it, so Tab users never land on an invisible button.
2witstudios
force-pushed
the
pu/terminal-theming-pane-chrome
branch
from
July 9, 2026 19:03
0581006 to
5cd66e5
Compare
…geometry
Proactive high-effort self-review (no external reviewer available yet —
CodeRabbit hit its rate limit) surfaced 3 confirmed defects, all fixed:
- resizable.tsx: the handle's accent-line div was hardcoded to vertical
geometry (absolute + translateX centering) regardless of orientation.
This PR's splitDown is the first orientation="vertical" panel group in
the app, and the new chrome-free variant makes the line permanently
visible — so the bug, latent before, now renders a floating vertical
dash instead of a horizontal seam between stacked panes. Fixed by
sizing the line (h-full w-px vs h-px w-full via an
aria-orientation-aware variant) and letting the parent's existing flex
centering position it, instead of fighting it with absolute/translate.
- workspace-reducer.ts: splitRight silently appended an orphan column
when fromPaneId didn't resolve, unlike every sibling transition
(splitDown/closePane/selectPane), which no-op on an unresolved pane id.
Now consistent; added a matching no-op test.
- useXtermTheme.ts / useMonacoTheme.ts: the light/dark fallback swatches
(background/foreground/primary hex) were duplicated verbatim between
both hooks. Extracted to THEME_FALLBACK in css-color-resolution.ts so
Monaco and xterm can't silently drift apart on a future rebrand.
Also documented (not reverted) the header-removal tradeoff flagged by
the review: TerminalPanes drops the per-pane header per explicit,
PurePoint-verified spec ("never shown, in any state"), but the Navigator
sidebar doesn't yet show which open terminal maps to which visible pane
with 2+ panes open. That's Navigator/sidebar work, out of scope for this
theming-foundation round — added a comment recording it as a known,
intentional gap rather than an oversight.
typecheck/build green across all 16 workspace packages; 518 apps/web
tests passing (29 in terminal-workspace, +1 new no-op case).
Addresses Codex review feedback on PR #1965: the Split Right/Split Down/Close toolbar was opacity-0 until group-hover, but the buttons stayed in the tab order — keyboard users tabbing through would land on invisible controls with no visual focus indicator. Add group-focus-within/pane:opacity-100 alongside the existing group-hover/pane:opacity-100, matching the same pattern already used in sidebar.tsx, MessageHoverToolbar.tsx, and NotificationItem.tsx.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
…hole pane Second-pass self-review caught a regression in the previous commit's accessibility fix: group-focus-within/pane fires on ANY focus inside the pane, including xterm.js's own hidden helper textarea that captures keystrokes during ordinary terminal typing (a sibling of the controls div under the same group/pane wrapper). That pinned the split/close controls overlay visible for the entire time a user was typing, not just when a keyboard user tabs onto the buttons — defeating the chrome-free design intent and occluding terminal output. Fix: drop the group indirection for the focus case. Plain focus-within:opacity-100 directly on the controls div only matches focus on its own descendants (the 3 buttons), not the sibling terminal content. This exactly matches the existing pattern in MessageHoverToolbar.tsx (group-hover/msg:opacity-100 combined with plain focus-within:opacity-100) — should have used it the first time. group-hover/pane:opacity-100 is unchanged and correct: hovering anywhere in the pane revealing the controls is the intended UX.
![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: 2
🤖 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 `@apps/web/src/lib/theme/css-color-resolution.ts`:
- Around line 153-160: The canvas-based color parsing in css-color-resolution.ts
can return a transparent sentinel when CSS.supports accepts a value that canvas
rejects, so update the color resolution flow to detect that case in the same
branch that uses context.fillStyle and fall back to the computed-style path
instead of returning `#00000000`. Use the existing color resolution logic around
the canvas draw/read sequence and the computed-style fallback helper to ensure
invalid canvas parses do not bypass the fallback.
In `@apps/web/src/stores/terminal-workspace/useTerminalWorkspaceStore.ts`:
- Around line 87-98: The new splitRight and splitDown ID generation is using
crypto.randomUUID(), which should be replaced with the project’s CUID2-based ID
generator to keep TypeScript IDs in the expected format. Update the splitRight
and splitDown methods in useTerminalWorkspaceStore to call the existing
`@paralleldrive/cuid2` helper used elsewhere in the codebase instead of generating
UUIDs, and keep the newColumnId/newPaneId creation logic otherwise unchanged.
🪄 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
Run ID: c023c153-9a67-42ae-9382-9f4d716166fa
📒 Files selected for processing (10)
apps/web/src/components/layout/middle-content/page-views/terminal/XtermTerminal.tsxapps/web/src/components/layout/middle-content/page-views/terminal/workspace/TerminalPanes.tsxapps/web/src/components/ui/resizable.tsxapps/web/src/hooks/useMonacoTheme.tsapps/web/src/hooks/useXtermTheme.tsapps/web/src/lib/theme/css-color-resolution.tsapps/web/src/stores/terminal-workspace/__tests__/useTerminalWorkspaceStore.test.tsapps/web/src/stores/terminal-workspace/__tests__/workspace-reducer.test.tsapps/web/src/stores/terminal-workspace/useTerminalWorkspaceStore.tsapps/web/src/stores/terminal-workspace/workspace-reducer.ts
…ent black Addresses CodeRabbit review feedback on PR #1965: CSS.supports('color', input) can accept syntax the canvas 2D fillStyle parser still silently rejects. An invalid fillStyle assignment is ignored per spec, leaving fillStyle on whatever was set before it — the rgba(0, 0, 0, 0) sentinel this function sets to detect exactly that. Without a check, that read back as a successfully-resolved transparent-black color instead of a rejected parse, bypassing the computed-style fallback. Capture the sentinel's canonical string right after setting it; if fillStyle still reads back as that same string after assigning `input`, treat it as rejected and fall through to the DOM-probe fallback instead of returning #00000000. No observable behavior change for any color value this codebase actually resolves (oklch/hex/rgb from globals.css all parse identically in canvas and CSS.supports on evergreen browsers) — this only closes a theoretical gap for pathological inputs.
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.
Summary
Round 1 ("theming foundation") of the Terminal — Command Center UI epic. Fixes the Terminal feature looking broken/unstyled inside PageSpace — xterm.js was constructed with no
themeoption, falling back to its own green-tinted default palette, andTerminalPanes.tsxhardcodedbg-black/ring-emerald-500/raw red-green status colors instead of design tokens.Fully decoupled from the Machine-page routing rebuild epic (separate sibling node) — ships standalone against the current right-sidebar-hosted Terminal feature.
useMonacoTheme's CSS-var → hex pipeline (hex/rgb parse,CSS.supports+ canvas fallback, hidden-span computed-style fallback,withAlpha) intoapps/web/src/lib/theme/css-color-resolution.ts, plus a sharedTHEME_FALLBACKlight/dark swatch constant so Monaco and xterm's themes can't drift apart on a future rebrand.useMonacoThemenow imports from it — no behavior change, existing Monaco theming unaffected. The canvas-based parse path also now correctly falls through to the computed-style fallback instead of reporting transparent black whenCSS.supports()accepts a color the canvasfillStyleparser silently rejects.useXtermTheme: reactive tonext-themes'resolvedTheme, resolves--background/--foreground/--primaryvia the shared util into xterm'sIThemeshape, with a curated muted/low-contrast ANSI 16-color palette per light/dark mode (no ANSI CSS tokens exist in the codebase — these are new design surface).XtermTerminal.tsx: wirestheme,fontFamily(from--font-mono),fontSize,cursorStyle,letterSpacing,lineHeightinto theTerminalconstructor. Theme updates live viaterminal.options.theme = themein an effect keyed on the returned theme object — the[socket, sessionId]-keyed PTY connection effect is untouched.TerminalPanes.tsxchrome rebuild: dropped the per-pane header entirely (session identity is sidebar-only now, matching PurePoint's real chrome-free design, verified against its actual SwiftUI source) — kept a top 2px--primaryaccent bar for focus and hover-revealed Split Right / Split Down (new) / Close controls using--card/--border/--muted-foregroundtokens. The controls toolbar is visible ongroup-hover/pane(hovering anywhere in the pane) and, scoped tightly to just the controls themselves via plainfocus-within(not the whole pane — an earlier version of this fix usedgroup-focus-within/pane, which also fired on xterm's own hidden input during ordinary typing and pinned the overlay visible the whole time; caught and fixed via a second self-review pass), so keyboard users tabbing through never land on an invisible control. Connecting/error states use--destructive/--primary/muted-foreground instead of hardcodedred-400/green-400.workspace-reducer.tsnow models a horizontal row of columns, each an independent vertical stack of panes (columns: { id, panes }[]), viasplitRight/splitDown— not a full recursive split tree. Both no-op on an unresolved pane id (consistent withclosePane/selectPane). Rendered as nestedResizablePanelGroups (outer horizontal, inner vertical per column) over the existing resizable primitives. Pane/column ids usecrypto.randomUUID(), matching the established in-file/client-side convention for ephemeral, never-persisted UI-state ids (this store's pre-existingensureWorkspace,XtermTerminal.tsx's connection id) —@paralleldrive/cuid2is this codebase's convention for persisted DB-entity ids, a different concern.ResizableHandle's restingopacity-0as the only clue a seam exists — confirmed unusable via live testing. Added avariant="chrome-free"prop that keeps the center line atopacity-60at rest, full opacity +--primarywhile dragging, without touching the component's global default used elsewhere in the app. The accent line itself is now sized (not absolute-positioned) so the parent's existing flex-centering handles both orientations correctly — the prior absolute/translate implementation was hardcoded vertical and silently broke for the neworientation="vertical"split-down groups.Test plan
bun run typecheck— green across all 16 workspace packages (includingnext build)bunx eslinton all touched files — cleanworkspace-reducer.test.ts,useTerminalWorkspaceStore.test.ts), covering split-right creating a column, split-down stacking within a column, closing the last pane in a column removing it, closing the last column no-op'ing like today, and both split transitions no-op'ing on an unresolved pane idapps/webhook test suite (518 tests) still green after theuseMonacoThemerefactorfocus-within, matching the existing pattern inMessageHoverToolbar.tsxuseXtermThemereactivity and chrome-free pane behavior in a running browser (not run in this session — no dev server available in this pass)🤖 Generated with Claude Code
https://claude.ai/code/session_01BxroUuSJVV1wsXUpvhF8bb