Add configurable terminal font settings - #3946
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
github-actions
Bot
added
vouch:unvouched
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
ApprovabilityVerdict: Needs human review 2 blocking correctness issues found. This PR introduces new terminal font settings (a new feature) and refactors terminal exit handling logic. Multiple unresolved review comments identify potential bugs including race conditions during surface creation and spurious 'Terminal closed' messages, warranting human review. You can customize Macroscope's approvability policy. Learn more. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
5160f78 to
4e2232e
Compare
There was a problem hiding this comment.
🟡 Medium
If terminal font settings change while GhosttyTerminalSurface.create is still awaiting font/WASM loading, the surface is created with the stale font and never updates. The setFont effect runs while terminalRef.current is still null (creation hasn't completed), so it no-ops, and the effect isn't retriggered once terminalRef.current is set. The surface then stays on the old font until the next unrelated settings change. After create resolves, apply the latest font from requestedTerminalFontRef.current via setFont.
| terminal.setTheme(terminalThemeFromApp(mount)); | |
| terminal.setFont(requestedTerminalFontRef.current); |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ThreadTerminalDrawer.tsx around line 442:
If terminal font settings change while `GhosttyTerminalSurface.create` is still awaiting font/WASM loading, the surface is created with the stale font and never updates. The `setFont` effect runs while `terminalRef.current` is still `null` (creation hasn't completed), so it no-ops, and the effect isn't retriggered once `terminalRef.current` is set. The surface then stays on the old font until the next unrelated settings change. After `create` resolves, apply the latest font from `requestedTerminalFontRef.current` via `setFont`.
4e2232e to
f32a442
Compare
f32a442 to
5094a0b
Compare
| if (latestSession.status === "closed" || latestSession.status === "exited") { | ||
| writeSystemMessage( | ||
| terminal, | ||
| latestSession.status === "closed" ? "Terminal closed" : "Process exited", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 Medium components/ThreadTerminalDrawer.tsx:455
The setup path prints Terminal closed for a live terminal when the surface finishes creating before the attach query hydrates. At that point latestSession is still the synthetic EMPTY_TERMINAL_SESSION_STATE (status "closed", version 0), so the check on line 455 treats it as a finished session and writes the exit notice. When the real running snapshot arrives later, the buffer sync effect writes the output but never removes the stale notice, leaving a false Terminal closed message permanently in the terminal. Consider guarding the initial notice so it only fires for an ended session that has real buffer content (version > 0) or skipping it when previousVersion === 0 and the status is "closed", letting the latest classification in the buffer effect handle only genuinely initial ended sessions.
| if (latestSession.status === "closed" || latestSession.status === "exited") { | |
| writeSystemMessage( | |
| terminal, | |
| latestSession.status === "closed" ? "Terminal closed" : "Process exited", | |
| ); | |
| } | |
| if ( | |
| latestSession.version > 0 && | |
| (latestSession.status === "closed" || latestSession.status === "exited") | |
| ) { | |
| writeSystemMessage( | |
| terminal, | |
| latestSession.status === "closed" ? "Terminal closed" : "Process exited", | |
| ); | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ThreadTerminalDrawer.tsx around lines 455-460:
The setup path prints `Terminal closed` for a live terminal when the surface finishes creating before the attach query hydrates. At that point `latestSession` is still the synthetic `EMPTY_TERMINAL_SESSION_STATE` (status `"closed"`, version `0`), so the check on line 455 treats it as a finished session and writes the exit notice. When the real `running` snapshot arrives later, the buffer sync effect writes the output but never removes the stale notice, leaving a false `Terminal closed` message permanently in the terminal. Consider guarding the initial notice so it only fires for an ended session that has real buffer content (version > 0) or skipping it when `previousVersion === 0` and the status is `"closed"`, letting the `latest` classification in the buffer effect handle only genuinely initial ended sessions.
54126e9 to
8b2aed7
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8b2aed7. Configure here.
Add `terminalFontFamily` and `terminalFontSize` to client settings, wire them through the settings panel, and apply them to the Ghostty terminal surface via its `font` option and `setFont()`. The contracts size range (8..32) sits inside the renderer's own 6..32 clamp so no accepted value is silently changed on the way to the canvas. Family names are quoted before they reach the surface, which appends its Nerd Font glyph fallbacks after whatever it is handed and does not quote. Also split the terminal exit handling in two: the "[terminal] ..." notice now fires when attaching to a session that had already ended, while closing the drawer still requires an observed running -> ended transition. Previously, opening a drawer on a finished terminal either dismissed it immediately or explained nothing.
8b2aed7 to
c63b5ac
Compare
1 participant
What Changed
Why
The integrated terminal used a hardcoded font stack beginning with SF Mono. Powerlevel10k prompts rely on private-use Powerline glyphs that SF Mono does not provide, which caused malformed segment caps and dangling artifacts.
Terminal fonts are a client rendering concern rather than a shell setting. Keeping these preferences client-local lets local and remote-backed terminals use fonts installed on the machine rendering T3 Code. Automatic mode provides a compatible fallback, while explicit family and size controls allow an exact match with another terminal.
UI Changes
A new Terminal section in General settings provides:
Changes apply live to existing terminals while preserving the shell session and scroll position. Powerlevel10k rendering and live updates were manually verified in the desktop app with JetBrainsMono Nerd Font.
Validation
Checklist
Note
Medium Risk
Touches integrated terminal lifecycle and client settings persistence; behavior changes on session exit could surprise users, but scope is UI/rendering with solid unit coverage.
Overview
Adds client-local terminal appearance via
terminalFontFamilyandterminalFontSizeonClientSettingsSchema, with schema defaults for legacy configs and patch validation (integer size 8–32).Settings → Appearance gains a Terminal section (family override with blank = auto Nerd Font stack, pixel size with reset/restore-defaults). Search catalog and restore-defaults flows include the new keys.
terminalAppearancenormalizes inputs and quotes font family names for safe CSS/Ghostty font lists.TerminalViewportreads those settings, passes font intoGhosttyTerminalSurface.create, and callssetFontwhen preferences change without recreating the surface.Separately refactors terminal exit behavior:
classifyTerminalExitTransitionandshouldHandleLiveTerminalExitsplit exit notices from auto-closing the drawer, so reopening or hydrating an already-ended session shows a message but stays visible; only a liverunning→exited/closedtransition dismisses the drawer. Status-driven close runs in its own effect so exits without new buffer output still close.Reviewed by Cursor Bugbot for commit c63b5ac. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add configurable terminal font family and size settings
terminalFontFamilyandterminalFontSizefields toClientSettingsSchemaandClientSettingsPatchin settings.ts, with boundsMIN=8,MAX=32,DEFAULT=12.TerminalViewportreads the new settings viauseClientSettings, builds aGhosttyTerminalFont, passes it toGhosttyTerminalSurface.create, and callsterminal.setFontwhen settings change.classifyTerminalExitTransitionandshouldHandleLiveTerminalExithelpers to distinguish initial hydrated exits from live exits, fixing a bug where the drawer closed on already-ended sessions.Macroscope summarized c63b5ac.