fix(desktop): recover terminal from non-monospace font crash (#3513)#3554
Merged
fix(desktop): recover terminal from non-monospace font crash (#3513)#3554
Conversation
Setting the terminal font to a proportional family like "Inter" blanked the app on next launch — the bad value persisted in SQLite and xterm couldn't lay out cells on reload, leaving no way back into settings. - Sanitize the stored family on read: if the primary family isn't monospace (per canvas measurement), fall back to the default terminal font so a poisoned DB value can never blank the renderer. - Hide the "Other" group and custom free-form entry in the terminal font picker so new selections are restricted to monospace candidates.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a sanitization utility, Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Settings UI
participant Sanitizer as sanitizeTerminalFontFamily
participant Doc as document / Canvas
participant Cache as MonospaceCache
participant Terminal as Terminal Renderer
UI->>Sanitizer: provide font-family string
Sanitizer->>Cache: check cached result (by lowercased name)
alt cache hit
Cache-->>Sanitizer: cached verdict
else cache miss
Sanitizer->>Doc: create canvas & set font
Doc-->>Sanitizer: measureText widths / or throw
Sanitizer->>Cache: store detection result
end
Sanitizer-->>UI: return normalized font-family (with monospace fallback if needed)
UI->>Terminal: apply sanitized fontFamily
Terminal-->>UI: render/update terminal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 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 |
Greptile SummaryThis PR fixes a startup crash (#3513) where persisting a proportional font (e.g. Two complementary defences are introduced:
Key observations:
Confidence Score: 4/5Safe to merge — the crash recovery path is solid and well-tested; two minor gaps (generic proportional bypass, Nerd Fonts in editor) are worth addressing but don't affect the primary bug fix. The core fix — sanitizing persisted terminal font families and restricting the picker — is correct, complete, and covered by targeted unit tests. The two P2 issues (all-generic proportional bypass and potential Nerd Fonts regression in the editor) are edge cases that don't impact the primary crash scenario reported in #3513. Both could be addressed in a follow-up without blocking the merge.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/lib/terminal/appearance/index.ts | Core sanitization logic: adds sanitizeTerminalFontFamily with canvas-based monospace detection and caching — but all-generic proportional families (cursive, fantasy) bypass the guard because parsePrimaryFontFamily returns null for generic-only lists. |
| apps/desktop/src/renderer/lib/terminal/appearance/appearance.test.ts | Well-structured unit tests covering null/empty, generic monospace pass-through, monospace font, proportional font fallback (quoted + bare), and canvas-unavailable path; uses unique font name to avoid module-level cache poisoning. |
| apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalAppearance/useTerminalAppearance.ts | v2 appearance hook correctly wraps font family lookup with sanitizeTerminalFontFamily before passing to xterm. |
| apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontFamilyCombobox/FontFamilyCombobox.tsx | Adds variant prop to gate off the "Other" font group and free-form custom entry for terminal pickers; Nerd Fonts are also gated to terminal-only which may unintentionally hide them from the editor picker. |
| apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx | v1 terminal correctly applies sanitizeTerminalFontFamily before passing font family to v1TerminalCache.updateAppearance. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
DB[(SQLite DB\nterminal_font_family)]
DB --> |fontSettings query| SAN
SAN["sanitizeTerminalFontFamily(cssValue)"]
SAN --> NULL{null / empty?}
NULL -- yes --> DEF[Return DEFAULT_TERMINAL_FONT_FAMILY]
NULL -- no --> PARSE["parsePrimaryFontFamily(cssValue)"]
PARSE --> PRIMQ{primary found?}
PRIMQ -- no, all-generic --> RAW["Return cssValue as-is\n⚠️ cursive/fantasy not caught"]
PRIMQ -- yes --> MONO["isFontFamilyMonospace(primary)"]
MONO --> CACHE{In cache?}
CACHE -- yes --> HIT[Return cached result]
CACHE -- no --> CANVAS[Canvas measureText\niiiiii vs MMMMMM]
CANVAS --> MEASURE{width diff < 1px?}
MEASURE -- yes --> PASS[Return cssValue ✓]
MEASURE -- no --> WARN[console.warn + Return DEFAULT]
CANVAS --> EX{Exception or no document?}
EX -- yes --> TRUST[Return true / pass through]
PASS --> V1[Terminal.tsx v1]
DEF --> V1
WARN --> V1
PASS --> V2[useTerminalAppearance v2]
DEF --> V2
WARN --> V2
Prompt To Fix All With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/appearance/index.ts
Line: 143-145
Comment:
**Generic proportional families bypass the sanitizer**
When the stored CSS value consists entirely of generic families (e.g. `"cursive"`, `"fantasy"`, or `"cursive, monospace"`), `parsePrimaryFontFamily` returns `null` (it skips generics when finding the "primary" family). The sanitizer then returns the raw value as-is, even though `cursive` and `fantasy` are proportional and would cause the same xterm crash the PR is fixing.
```ts
const primary = parsePrimaryFontFamily(cssValue);
if (!primary) return cssValue; // passes through "cursive", "fantasy", etc.
```
A straightforward fix is to check whether the raw value is a known safe generic before passing it through:
```
const SAFE_GENERIC_MONOSPACE = new Set(["monospace", "ui-monospace"]);
const primary = parsePrimaryFontFamily(cssValue);
if (primary === null) {
// All-generic list — only safe if every resolved family is monospace-generic
const allSafe = cssValue
.split(",")
.map((s) => s.trim().toLowerCase())
.every((f) => SAFE_GENERIC_MONOSPACE.has(f));
return allSafe ? cssValue : DEFAULT_TERMINAL_FONT_FAMILY;
}
```
While a user storing `"cursive"` by hand is unlikely once the picker is restricted, the sanitizer's stated goal is to recover any poisoned DB value — proportional generic families are the same class of problem.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontFamilyCombobox/FontFamilyCombobox.tsx
Line: 152-154
Comment:
**Nerd Fonts hidden from the editor picker — intentional?**
The conditional gates Nerd Fonts exclusively on `variant === "terminal"`:
```tsx
{variant === "terminal" && renderGroup("Nerd Fonts", nerdFonts)}
{renderGroup("Monospace", monoFonts)}
{variant !== "terminal" && renderGroup("Other", otherFonts)}
```
This means the editor font picker only shows Monospace + Other — Nerd Fonts never appear there. Nerd Fonts are monospace and widely used in editors for powerline/git-blame symbols. Was it a deliberate decision to exclude them from the editor, or were they meant to be available in both pickers (and only `"Other"` should be restricted to terminal)?
The PR description mentions hiding `"Other"` for terminal, but is silent on hiding Nerd Fonts from the editor.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(desktop): recover terminal from non-..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+143
to
+145
| const primary = parsePrimaryFontFamily(cssValue); | ||
| if (!primary) return cssValue; | ||
| if (isFontFamilyMonospace(primary)) return cssValue; |
There was a problem hiding this comment.
When the stored CSS value consists entirely of generic families (e.g. "cursive", "fantasy", or "cursive, monospace"), parsePrimaryFontFamily returns null (it skips generics when finding the "primary" family). The sanitizer then returns the raw value as-is, even though cursive and fantasy are proportional and would cause the same xterm crash the PR is fixing.
const primary = parsePrimaryFontFamily(cssValue);
if (!primary) return cssValue; // passes through "cursive", "fantasy", etc.A straightforward fix is to check whether the raw value is a known safe generic before passing it through:
const SAFE_GENERIC_MONOSPACE = new Set(["monospace", "ui-monospace"]);
const primary = parsePrimaryFontFamily(cssValue);
if (primary === null) {
// All-generic list — only safe if every resolved family is monospace-generic
const allSafe = cssValue
.split(",")
.map((s) => s.trim().toLowerCase())
.every((f) => SAFE_GENERIC_MONOSPACE.has(f));
return allSafe ? cssValue : DEFAULT_TERMINAL_FONT_FAMILY;
}
While a user storing "cursive" by hand is unlikely once the picker is restricted, the sanitizer's stated goal is to recover any poisoned DB value — proportional generic families are the same class of problem.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/lib/terminal/appearance/index.ts
Line: 143-145
Comment:
**Generic proportional families bypass the sanitizer**
When the stored CSS value consists entirely of generic families (e.g. `"cursive"`, `"fantasy"`, or `"cursive, monospace"`), `parsePrimaryFontFamily` returns `null` (it skips generics when finding the "primary" family). The sanitizer then returns the raw value as-is, even though `cursive` and `fantasy` are proportional and would cause the same xterm crash the PR is fixing.
```ts
const primary = parsePrimaryFontFamily(cssValue);
if (!primary) return cssValue; // passes through "cursive", "fantasy", etc.
```
A straightforward fix is to check whether the raw value is a known safe generic before passing it through:
```
const SAFE_GENERIC_MONOSPACE = new Set(["monospace", "ui-monospace"]);
const primary = parsePrimaryFontFamily(cssValue);
if (primary === null) {
// All-generic list — only safe if every resolved family is monospace-generic
const allSafe = cssValue
.split(",")
.map((s) => s.trim().toLowerCase())
.every((f) => SAFE_GENERIC_MONOSPACE.has(f));
return allSafe ? cssValue : DEFAULT_TERMINAL_FONT_FAMILY;
}
```
While a user storing `"cursive"` by hand is unlikely once the picker is restricted, the sanitizer's stated goal is to recover any poisoned DB value — proportional generic families are the same class of problem.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+152
to
+154
| {variant === "terminal" && renderGroup("Nerd Fonts", nerdFonts)} | ||
| {renderGroup("Monospace", monoFonts)} | ||
| {renderGroup("Other", otherFonts)} | ||
| {variant !== "terminal" && renderGroup("Other", otherFonts)} |
There was a problem hiding this comment.
The conditional gates Nerd Fonts exclusively on variant === "terminal":
{variant === "terminal" && renderGroup("Nerd Fonts", nerdFonts)}
{renderGroup("Monospace", monoFonts)}
{variant !== "terminal" && renderGroup("Other", otherFonts)}This means the editor font picker only shows Monospace + Other — Nerd Fonts never appear there. Nerd Fonts are monospace and widely used in editors for powerline/git-blame symbols. Was it a deliberate decision to exclude them from the editor, or were they meant to be available in both pickers (and only "Other" should be restricted to terminal)?
The PR description mentions hiding "Other" for terminal, but is silent on hiding Nerd Fonts from the editor. Was it intentional to hide Nerd Fonts from the editor font picker? They are monospace and commonly used in code editors too — the PR description only mentions restricting the terminal picker's "Other" group.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontFamilyCombobox/FontFamilyCombobox.tsx
Line: 152-154
Comment:
**Nerd Fonts hidden from the editor picker — intentional?**
The conditional gates Nerd Fonts exclusively on `variant === "terminal"`:
```tsx
{variant === "terminal" && renderGroup("Nerd Fonts", nerdFonts)}
{renderGroup("Monospace", monoFonts)}
{variant !== "terminal" && renderGroup("Other", otherFonts)}
```
This means the editor font picker only shows Monospace + Other — Nerd Fonts never appear there. Nerd Fonts are monospace and widely used in editors for powerline/git-blame symbols. Was it a deliberate decision to exclude them from the editor, or were they meant to be available in both pickers (and only `"Other"` should be restricted to terminal)?
The PR description mentions hiding `"Other"` for terminal, but is silent on hiding Nerd Fonts from the editor. Was it intentional to hide Nerd Fonts from the editor font picker? They are monospace and commonly used in code editors too — the PR description only mentions restricting the terminal picker's "Other" group.
How can I resolve this? If you propose a fix, please make it concise.
Collaborator
Author
There was a problem hiding this comment.
Good catch, but out of scope for this PR — that gate was pre-existing, not introduced here. git blame on this line predates this branch; this PR only adds the variant !== "terminal" gate on the Other group below it.
Two reasons to defer: (1) editor users who want a specific Nerd Font can still type it in the search box and hit 'Use …' — the custom-entry path is intentionally left open for the editor variant. (2) The Superset editor font only applies to diff views and file editors, where Nerd Font icon glyphs aren't actually rendered, so the UX upside is modest. Happy to revisit in a follow-up.
![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 the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/lib/terminal/appearance/appearance.test.ts`:
- Around line 70-72: Add a regression test to ensure proportional generic
families are not trusted: in appearance.test.ts add a test (e.g. "falls back for
proportional generic families") that calls
sanitizeTerminalFontFamily("sans-serif") and sanitizeTerminalFontFamily("serif")
and asserts both results equal the terminal fallback value by comparing them to
sanitizeTerminalFontFamily("") (or whatever the existing function returns for
the default/fallback). Place the test alongside the existing "trusts all-generic
monospace values without canvas" test and reference the
sanitizeTerminalFontFamily function to locate the code under test.
In `@apps/desktop/src/renderer/lib/terminal/appearance/index.ts`:
- Around line 90-92: parsePrimaryFontFamily currently lets an all-generic stack
slip through via the caller fallback; instead explicitly detect and reject
stacks composed only of generic families: in parsePrimaryFontFamily (the
function using GENERIC_FONT_FAMILIES and families.find(...)), add a check like
if (families.length && families.every(f =>
GENERIC_FONT_FAMILIES.has(f.toLowerCase()))) return null so pure-generic inputs
(e.g., "sans-serif") are treated as invalid and won't be returned/used by the
caller.
🪄 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: aa888184-bd39-4b90-93ba-5f4578beeab2
📒 Files selected for processing (5)
apps/desktop/src/renderer/lib/terminal/appearance/appearance.test.tsapps/desktop/src/renderer/lib/terminal/appearance/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalAppearance/useTerminalAppearance.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontFamilyCombobox/FontFamilyCombobox.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue 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="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalAppearance/useTerminalAppearance.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalAppearance/useTerminalAppearance.ts:24">
P2: Sanitization still lets all-generic non-monospace font stacks through, so the terminal can still be launched with a proportional font.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const theme = terminalTheme ?? fallbackTheme; | ||
| const fontFamily = | ||
| fontSettings?.terminalFontFamily || DEFAULT_TERMINAL_FONT_FAMILY; | ||
| const fontFamily = sanitizeTerminalFontFamily( |
Contributor
There was a problem hiding this comment.
P2: Sanitization still lets all-generic non-monospace font stacks through, so the terminal can still be launched with a proportional font.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/TerminalPane/hooks/useTerminalAppearance/useTerminalAppearance.ts, line 24:
<comment>Sanitization still lets all-generic non-monospace font stacks through, so the terminal can still be launched with a proportional font.</comment>
<file context>
@@ -21,8 +21,9 @@ export function useTerminalAppearance(): TerminalAppearance {
const theme = terminalTheme ?? fallbackTheme;
- const fontFamily =
- fontSettings?.terminalFontFamily || DEFAULT_TERMINAL_FONT_FAMILY;
+ const fontFamily = sanitizeTerminalFontFamily(
+ fontSettings?.terminalFontFamily,
+ );
</file context>
Follow-up on #3554 review. sanitizeTerminalFontFamily previously passed any all-generic CSS value through untouched (e.g. "cursive", "sans-serif", "monospace, sans-serif") because parsePrimaryFontFamily returns null when no concrete family is present — same blank-window crash class as the "Inter" report. Refactor the sanitizer to inspect the full family list: when no concrete primary exists, only trust the value if every entry is a monospace generic; otherwise fall back to the default. Add regression tests.
- Always append "monospace" to the sanitized terminal font stack when it doesn't already end with one. Mirrors VS Code's behavior in src/vs/workbench/contrib/terminal/browser/terminalConfigurationService.ts so that if the configured primary isn't installed on this machine, the browser falls back to the OS monospace generic instead of a proportional default. - Swap the terminal font preview from a box-drawing layout (which rendered as broken in proportional fonts and used tofu glyphs) to a shell session that demonstrates column alignment naturally. - Drop a couple of narrating comments flagged in simplify review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/renderer/lib/terminal/appearance/index.ts`:
- Around line 141-171: The current code picks the first non-generic entry as
`primary`, which lets a persisted value like `sans-serif, "JetBrains Mono"` slip
through; instead validate the actual CSS primary (the first family token in
`families`, i.e. `families[0]`) — if the first is a generic, apply the existing
"all-mono" check using `MONOSPACE_GENERIC_FAMILIES` and fall back to
`DEFAULT_TERMINAL_FONT_FAMILY` or return `cssValue` accordingly; if the first is
non-generic, call `isFontFamilyMonospace` on that actual primary and fall back
to `DEFAULT_TERMINAL_FONT_FAMILY` on failure; finally preserve the existing
logic that appends a monospace generic tail (`MONOSPACE_GENERIC_FAMILIES`) to
`cssValue` when missing.
🪄 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: db92d0b7-05d2-4a7c-bd0d-170aa87e56b4
📒 Files selected for processing (3)
apps/desktop/src/renderer/lib/terminal/appearance/appearance.test.tsapps/desktop/src/renderer/lib/terminal/appearance/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/FontPreview.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/FontPreview.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/lib/terminal/appearance/appearance.test.ts
Comment on lines
+141
to
+171
| const primary = families.find( | ||
| (f) => !GENERIC_FONT_FAMILIES.has(f.toLowerCase()), | ||
| ); | ||
| if (!primary) { | ||
| // All-generic stack (e.g. "monospace", "cursive, fantasy"). Only trust it | ||
| // when every entry is a monospace generic — otherwise a value like | ||
| // "sans-serif" would still blank the terminal. | ||
| const allMono = families.every((f) => | ||
| MONOSPACE_GENERIC_FAMILIES.has(f.toLowerCase()), | ||
| ); | ||
| if (allMono) return cssValue; | ||
| console.warn( | ||
| `[terminal] Font stack "${cssValue}" has no monospace family; falling back to default terminal font.`, | ||
| ); | ||
| return DEFAULT_TERMINAL_FONT_FAMILY; | ||
| } | ||
|
|
||
| if (!isFontFamilyMonospace(primary)) { | ||
| console.warn( | ||
| `[terminal] Font "${primary}" is not monospace; falling back to default terminal font.`, | ||
| ); | ||
| return DEFAULT_TERMINAL_FONT_FAMILY; | ||
| } | ||
| // Ensure a generic monospace tail — if the configured primary isn't | ||
| // installed on this machine, the browser falls back to the OS monospace | ||
| // generic instead of a proportional default (mirrors VS Code's behavior | ||
| // in src/vs/workbench/contrib/terminal/browser/terminalConfigurationService.ts). | ||
| const hasMonoTail = families.some((f) => | ||
| MONOSPACE_GENERIC_FAMILIES.has(f.toLowerCase()), | ||
| ); | ||
| return hasMonoTail ? cssValue : `${cssValue}, monospace`; |
Contributor
There was a problem hiding this comment.
Validate the actual CSS primary family, not the first non-generic entry.
Line 141 skips leading generics, so a persisted value like sans-serif, "JetBrains Mono" passes validation because "JetBrains Mono" is monospace. CSS will still render sans-serif first, leaving the proportional primary in place and potentially preserving the crash path.
🐛 Proposed fix
- const primary = families.find(
- (f) => !GENERIC_FONT_FAMILIES.has(f.toLowerCase()),
- );
- if (!primary) {
- // All-generic stack (e.g. "monospace", "cursive, fantasy"). Only trust it
- // when every entry is a monospace generic — otherwise a value like
- // "sans-serif" would still blank the terminal.
- const allMono = families.every((f) =>
- MONOSPACE_GENERIC_FAMILIES.has(f.toLowerCase()),
- );
- if (allMono) return cssValue;
+ const primary = families[0];
+ const primaryKey = primary.toLowerCase();
+
+ if (GENERIC_FONT_FAMILIES.has(primaryKey)) {
+ if (MONOSPACE_GENERIC_FAMILIES.has(primaryKey)) return cssValue;
console.warn(
`[terminal] Font stack "${cssValue}" has no monospace family; falling back to default terminal font.`,
);
return DEFAULT_TERMINAL_FONT_FAMILY;
}
if (!isFontFamilyMonospace(primary)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const primary = families.find( | |
| (f) => !GENERIC_FONT_FAMILIES.has(f.toLowerCase()), | |
| ); | |
| if (!primary) { | |
| // All-generic stack (e.g. "monospace", "cursive, fantasy"). Only trust it | |
| // when every entry is a monospace generic — otherwise a value like | |
| // "sans-serif" would still blank the terminal. | |
| const allMono = families.every((f) => | |
| MONOSPACE_GENERIC_FAMILIES.has(f.toLowerCase()), | |
| ); | |
| if (allMono) return cssValue; | |
| console.warn( | |
| `[terminal] Font stack "${cssValue}" has no monospace family; falling back to default terminal font.`, | |
| ); | |
| return DEFAULT_TERMINAL_FONT_FAMILY; | |
| } | |
| if (!isFontFamilyMonospace(primary)) { | |
| console.warn( | |
| `[terminal] Font "${primary}" is not monospace; falling back to default terminal font.`, | |
| ); | |
| return DEFAULT_TERMINAL_FONT_FAMILY; | |
| } | |
| // Ensure a generic monospace tail — if the configured primary isn't | |
| // installed on this machine, the browser falls back to the OS monospace | |
| // generic instead of a proportional default (mirrors VS Code's behavior | |
| // in src/vs/workbench/contrib/terminal/browser/terminalConfigurationService.ts). | |
| const hasMonoTail = families.some((f) => | |
| MONOSPACE_GENERIC_FAMILIES.has(f.toLowerCase()), | |
| ); | |
| return hasMonoTail ? cssValue : `${cssValue}, monospace`; | |
| const primary = families[0]; | |
| const primaryKey = primary.toLowerCase(); | |
| if (GENERIC_FONT_FAMILIES.has(primaryKey)) { | |
| if (MONOSPACE_GENERIC_FAMILIES.has(primaryKey)) return cssValue; | |
| console.warn( | |
| `[terminal] Font stack "${cssValue}" has no monospace family; falling back to default terminal font.`, | |
| ); | |
| return DEFAULT_TERMINAL_FONT_FAMILY; | |
| } | |
| if (!isFontFamilyMonospace(primary)) { | |
| console.warn( | |
| `[terminal] Font "${primary}" is not monospace; falling back to default terminal font.`, | |
| ); | |
| return DEFAULT_TERMINAL_FONT_FAMILY; | |
| } | |
| // Ensure a generic monospace tail — if the configured primary isn't | |
| // installed on this machine, the browser falls back to the OS monospace | |
| // generic instead of a proportional default (mirrors VS Code's behavior | |
| // in src/vs/workbench/contrib/terminal/browser/terminalConfigurationService.ts). | |
| const hasMonoTail = families.some((f) => | |
| MONOSPACE_GENERIC_FAMILIES.has(f.toLowerCase()), | |
| ); | |
| return hasMonoTail ? cssValue : `${cssValue}, monospace`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/renderer/lib/terminal/appearance/index.ts` around lines 141
- 171, The current code picks the first non-generic entry as `primary`, which
lets a persisted value like `sans-serif, "JetBrains Mono"` slip through; instead
validate the actual CSS primary (the first family token in `families`, i.e.
`families[0]`) — if the first is a generic, apply the existing "all-mono" check
using `MONOSPACE_GENERIC_FAMILIES` and fall back to
`DEFAULT_TERMINAL_FONT_FAMILY` or return `cssValue` accordingly; if the first is
non-generic, call `isFontFamilyMonospace` on that actual primary and fall back
to `DEFAULT_TERMINAL_FONT_FAMILY` on failure; finally preserve the existing
logic that appends a monospace generic tail (`MONOSPACE_GENERIC_FAMILIES`) to
`cssValue` when missing.
Nerd Fonts are monospace — the terminal-only gate was pre-existing special-casing and reviewers pointed out it hides a widely-used class of fonts from users picking an editor font. Drop the gate.
…crete entry Addresses the coderabbit review on b2e6a04. The sanitizer previously skipped leading generics when picking the primary to measure, so a value like `sans-serif, "JetBrains Mono"` passed validation because the later concrete entry was monospace — but CSS resolves the first generic (sans-serif) and the terminal still renders proportional. Switch to validating families[0] (the actual CSS primary): if it's a monospace generic, trust the stack; if it's a proportional generic, fall back; if it's concrete, canvas-measure it. Add regression tests.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
7 tasks
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
Fixes #3513. Setting the terminal font to a proportional family like "Inter" blanked the main window on next launch — xterm.js can't lay out cells against a non-monospace font, and because the bad value is persisted to SQLite the crash repeated on every relaunch with no way back into settings (reporter had to hand-edit
~/.superset/local.dbto recover).Two changes:
sanitizeTerminalFontFamilyinapps/desktop/src/renderer/lib/terminal/appearance/index.ts). When the primary family of the stored CSS value isn't monospace (decided by canvas measurement ofiiiiiivsMMMMMM), fall back toDEFAULT_TERMINAL_FONT_FAMILY. Wired into both the v1 (Terminal.tsx) and v2 (useTerminalAppearance) apply paths. This is the recovery path for users already stuck.FontFamilyCombobox.tsx). Forvariant === "terminal", hide the "Other" (proportional) group, theCommandEmpty"Use ''" escape hatch, and the free-form "Custom" group — so new selections can only come from the Nerd Fonts / Monospace groups.Test plan
bun test src/renderer/lib/terminal/appearance/appearance.test.ts— 6 new unit tests cover null/empty, generic monospace pass-through, monospace font, proportional font fallback (quoted + bare), and canvas-unavailable path.bun run typecheckclean.bun run lintclean.terminal_font_family = 'Inter'in~/.superset/local.dbbefore launch — app should start normally and use the default terminal font.Summary by cubic
Recover from desktop launch crash caused by non‑monospace terminal fonts and prevent it from happening again. Fixes #3513.
Bug Fixes
DEFAULT_TERMINAL_FONT_FAMILY. Append ", monospace" when missing so the OS monospace is used if the chosen font isn’t installed. Applied in both v1 and v2 terminal paths.Refactors
Written for commit 8404892. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
User Interface
Tests