feat(desktop): replace font text input with searchable dropdown, fix broken font loading#2458
Merged
Merged
Conversation
- Add searchable combobox for font family selection with categorized groups (Nerd Fonts, Monospace, Other) and per-item typeface preview - Detect installed system fonts via canvas measurement + queryLocalFonts API - Show syntax-highlighted code preview (editor) and multi-agent terminal preview - Add "not installed" banner when selected font isn't available - Load SF Mono at runtime from macOS system paths via superset-font:// protocol (no font files bundled — Apple licensing compliant) - Platform-gate SF Mono to macOS only where the protocol handler exists - Co-locate FontPreview under FontSettingSection per project structure rules
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds macOS system-font support and a new font-selection UI: registers a Changes
Sequence DiagramsequenceDiagram
participant User
participant Renderer as Renderer Process
participant Main as Main Process
participant System as OS/File System
participant Canvas as Canvas/DOM
User->>Renderer: Open Appearance Settings
Renderer->>Renderer: useSystemFonts initializes
Renderer->>Canvas: await document.fonts.ready & queryLocalFonts?()
Canvas-->>Renderer: font metadata / ready
Renderer->>Renderer: classify/group fonts (nerd/mono/other)
User->>Renderer: Search/select font in FontFamilyCombobox
Renderer->>Renderer: update selected font value
Renderer->>Canvas: check font availability (canvas measurement / document.fonts.load)
Canvas-->>Renderer: availability result
alt Font available
Renderer->>Renderer: render preview (terminal or syntax-highlighted) with font
else Font unavailable
Renderer->>Renderer: show FontNotFoundBanner and fallback preview
end
Renderer->>Main: request superset-font://... URL for font resource
Main->>System: read font file from macOS font directories
System-->>Main: font bytes
Main-->>Renderer: serve font bytes via protocol response
Renderer->>Renderer: `@font-face` loads and applies font
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 15 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/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/components/FontNotFoundBanner/FontNotFoundBanner.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/components/FontNotFoundBanner/FontNotFoundBanner.tsx:18">
P2: Avoid swallowing font availability check failures in `catch`; log a warning with context before returning `false`.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/font-utils.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/font-utils.ts:16">
P2: Do not split `font-family` on raw commas; it breaks quoted family names containing commas.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts:166">
P2: Don't use an empty `catch` here; log a warning with context so local-font discovery failures are diagnosable.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts:180">
P2: Handle the `loadFonts()` promise rejection when calling it from `useEffect` to prevent unhandled promise rejections.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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: 4
🤖 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/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontFamilyCombobox/FontFamilyCombobox.tsx`:
- Line 26: The search logic in FontFamilyCombobox uses items.slice(0,
MAX_VISIBLE) before filtering, so fonts beyond MAX_VISIBLE are never searchable;
update the component (referencing MAX_VISIBLE, the items array and the filtering
code around FontFamilyCombobox render/search logic) to apply slicing after the
search/filter step (or slice the filteredResults instead of the raw items) so
that filtering/matching happens against the full items list and only the visible
subset is limited afterward; also apply the same change to the other occurrence
around the code region currently at lines 71-92 where items.slice is used before
filtering.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/components/FontNotFoundBanner/FontNotFoundBanner.tsx`:
- Around line 31-44: The useEffect that probes primaryFont should reset the
available state immediately when primaryFont changes so a previously-false
result doesn't show a stale "not found" banner while the new check is pending;
inside the useEffect that references primaryFont and calls
checkFontAvailable(primaryFont), call setAvailable(true) (or otherwise
clear/optimistically hide the "not found" state) before launching the async
check, keep the existing cancelled flag and promise handling (the
checkFontAvailable(...).then(...) and return cancelling function) intact so the
new probe still updates available when it resolves.
- Around line 10-20: checkFontAvailable currently relies on document.fonts.check
which falsely returns true due to browser fallback; replace this logic by
reusing the same installed-font detection used by the combobox (or accept a
boolean prop from the parent) instead of calling document.fonts.check. Update
the FontNotFoundBanner to either import and call the shared utility (the same
function/class used by the combobox) to deterministically detect installed fonts
or add a new prop like isFontInstalled that the parent provides; retain the
GENERIC_FAMILIES short-circuit but remove the document.fonts.check fallback and
any try/catch around it so availability is driven by the shared detector or the
passed-in status (ensure family case normalization matches the combobox
detector).
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/FontPreview.tsx`:
- Around line 1-5: The file uses React types (React.CSSProperties) but doesn't
import the React namespace, causing TS errors; update the imports in FontPreview
to include the React namespace (e.g., import React from "react") or directly
import CSSProperties (e.g., import type { CSSProperties } from "react") so
references like React.CSSProperties or CSSProperties in the FontPreview
component type annotations compile correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7efb71ca-3ee4-4c7c-9917-4ba076b821b2
📒 Files selected for processing (15)
apps/desktop/src/main/index.tsapps/desktop/src/renderer/index.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontPreview/FontPreview.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/FontSettingSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontFamilyCombobox/FontFamilyCombobox.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontFamilyCombobox/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/FontPreview.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/components/FontNotFoundBanner/FontNotFoundBanner.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/components/FontNotFoundBanner/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/font-utils.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/index.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/types.d.tsapps/desktop/src/renderer/styles/bundled-fonts.css
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontPreview/FontPreview.tsx
CodeRabbit / Cubic review fixes: - Fix font-family parsing to handle quoted names with commas - Replace document.fonts.check() with canvas-based detection (check() always returns true due to browser fallback) - Reset "not found" state immediately on font change to prevent stale banner - Import CSSProperties properly instead of unscoped React.CSSProperties - Apply MAX_VISIBLE slice after search filtering so all fonts are searchable - Add console.warn in catch blocks for queryLocalFonts and font checks - Add .catch() to loadFonts() promise in useEffect Additional fixes: - Remove SF Mono from default editor and terminal font stacks (users can still select it from the dropdown) - Only show "not installed" banner for user-selected fonts, not the default fallback chain - Verify registered @font-face fonts (SF Mono) via canvas measurement before adding to dropdown - Replace unicode ellipsis (\u2026) with "..." in search placeholder - Platform-gate SF Mono to macOS only
Contributor
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
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/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/components/FontNotFoundBanner/FontNotFoundBanner.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/components/FontNotFoundBanner/FontNotFoundBanner.tsx:10">
P1: Duplicate font availability detection logic introduced in PR. `isFontInstalled` in FontNotFoundBanner.tsx duplicates the canvas-based measurement logic already implemented in `isFontAvailable` in useSystemFonts.ts. Both use the same test string, fallback fonts, canvas measurement at 72px, and 0.5px width threshold. Extract this into a shared utility in font-utils.ts to prevent divergence and maintenance burden.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/components/FontNotFoundBanner/FontNotFoundBanner.tsx:61">
P2: The availability check runs before `@font-face` fonts are guaranteed to load, so custom fonts can be incorrectly flagged as “not installed.”</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts:148">
P2: `document.fonts.ready` does not guarantee unused `@font-face` fonts are loaded, so this availability check can incorrectly hide registered fonts from the dropdown.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| * Measures text with the target font against a known fallback — if the widths | ||
| * differ, the font is installed. | ||
| */ | ||
| function isFontInstalled(family: string): boolean { |
Contributor
There was a problem hiding this comment.
P1: Duplicate font availability detection logic introduced in PR. isFontInstalled in FontNotFoundBanner.tsx duplicates the canvas-based measurement logic already implemented in isFontAvailable in useSystemFonts.ts. Both use the same test string, fallback fonts, canvas measurement at 72px, and 0.5px width threshold. Extract this into a shared utility in font-utils.ts to prevent divergence and maintenance burden.
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/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/components/FontNotFoundBanner/FontNotFoundBanner.tsx, line 10:
<comment>Duplicate font availability detection logic introduced in PR. `isFontInstalled` in FontNotFoundBanner.tsx duplicates the canvas-based measurement logic already implemented in `isFontAvailable` in useSystemFonts.ts. Both use the same test string, fallback fonts, canvas measurement at 72px, and 0.5px width threshold. Extract this into a shared utility in font-utils.ts to prevent divergence and maintenance burden.</comment>
<file context>
@@ -3,20 +3,39 @@ import { useEffect, useMemo, useState } from "react";
+ * differ, the font is installed.
*/
-async function checkFontAvailable(family: string): Promise<boolean> {
+function isFontInstalled(family: string): boolean {
if (GENERIC_FAMILIES.has(family.toLowerCase())) return true;
</file context>
Comment on lines
+148
to
+151
| if (isFontAvailable(font.family)) { | ||
| result.push(font); | ||
| seen.add(font.family); | ||
| } |
Contributor
There was a problem hiding this comment.
P2: document.fonts.ready does not guarantee unused @font-face fonts are loaded, so this availability check can incorrectly hide registered fonts from the dropdown.
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/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts, line 148:
<comment>`document.fonts.ready` does not guarantee unused `@font-face` fonts are loaded, so this availability check can incorrectly hide registered fonts from the dropdown.</comment>
<file context>
@@ -140,8 +140,16 @@ export function useSystemFonts() {
+
+ // Add registered @font-face fonts only if they loaded successfully
+ for (const font of REGISTERED_FONTS) {
+ if (isFontAvailable(font.family)) {
+ result.push(font);
+ seen.add(font.family);
</file context>
Suggested change
| if (isFontAvailable(font.family)) { | |
| result.push(font); | |
| seen.add(font.family); | |
| } | |
| result.push(font); | |
| seen.add(font.family); |
|
|
||
| // Use rAF to ensure @font-face fonts have a chance to load | ||
| const raf = requestAnimationFrame(() => { | ||
| setAvailable(isFontInstalled(primaryFont)); |
Contributor
There was a problem hiding this comment.
P2: The availability check runs before @font-face fonts are guaranteed to load, so custom fonts can be incorrectly flagged as “not installed.”
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/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/components/FontNotFoundBanner/FontNotFoundBanner.tsx, line 61:
<comment>The availability check runs before `@font-face` fonts are guaranteed to load, so custom fonts can be incorrectly flagged as “not installed.”</comment>
<file context>
@@ -34,13 +53,14 @@ export function FontNotFoundBanner({ fontFamily }: { fontFamily: string }) {
+
+ // Use rAF to ensure @font-face fonts have a chance to load
+ const raf = requestAnimationFrame(() => {
+ setAvailable(isFontInstalled(primaryFont));
});
- return () => {
</file context>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts (2)
15-17: Consider usingnavigator.userAgentData.platformwith fallback.
navigator.platformis deprecated. While it still works in Electron, consider future-proofing with a utility that triesuserAgentDatafirst:const isMacOS = navigator.userAgentData?.platform === "macOS" ?? navigator.platform.startsWith("Mac");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts` around lines 15 - 17, The code uses the deprecated navigator.platform when building REGISTERED_FONTS; change it to prefer navigator.userAgentData?.platform with a fallback to navigator.platform (e.g., compute an isMacOS boolean using navigator.userAgentData?.platform === "macOS" ?? navigator.platform.startsWith("Mac")) and then use that boolean when setting REGISTERED_FONTS so mac detection works in future userAgentData-enabled environments; update the REGISTERED_FONTS constant initialization (and any related platform checks in useSystemFonts) to use this new isMacOS check.
74-93: Consider extracting canvas-based font detection to a shared utility.The
isFontAvailablefunction here is nearly identical toisFontInstalledinFontNotFoundBanner.tsx. Extracting this to a shared module (e.g., infont-utils.ts) would reduce duplication and ensure consistent behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts` around lines 74 - 93, The isFontAvailable function duplicates logic from isFontInstalled in FontNotFoundBanner.tsx; extract the canvas-based detection into a shared utility (e.g., create font-utils.ts exporting a single function like isFontAvailableOrInstalled) and replace the local implementations with imports. Update the hook file (useSystemFonts/useSystemFonts.ts) to import the shared function instead of its local isFontAvailable, and update FontNotFoundBanner.tsx to import the same utility so both files use the single source of truth for canvas font detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.ts`:
- Around line 15-17: The code uses the deprecated navigator.platform when
building REGISTERED_FONTS; change it to prefer navigator.userAgentData?.platform
with a fallback to navigator.platform (e.g., compute an isMacOS boolean using
navigator.userAgentData?.platform === "macOS" ??
navigator.platform.startsWith("Mac")) and then use that boolean when setting
REGISTERED_FONTS so mac detection works in future userAgentData-enabled
environments; update the REGISTERED_FONTS constant initialization (and any
related platform checks in useSystemFonts) to use this new isMacOS check.
- Around line 74-93: The isFontAvailable function duplicates logic from
isFontInstalled in FontNotFoundBanner.tsx; extract the canvas-based detection
into a shared utility (e.g., create font-utils.ts exporting a single function
like isFontAvailableOrInstalled) and replace the local implementations with
imports. Update the hook file (useSystemFonts/useSystemFonts.ts) to import the
shared function instead of its local isFontAvailable, and update
FontNotFoundBanner.tsx to import the same utility so both files use the single
source of truth for canvas font detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 127f8492-a01b-4db0-a577-a1720fb4838d
📒 Files selected for processing (8)
apps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/FontSettingSection.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontFamilyCombobox/FontFamilyCombobox.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/FontPreview.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/components/FontPreview/components/FontNotFoundBanner/FontNotFoundBanner.tsxapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/font-utils.tsapps/desktop/src/renderer/routes/_authenticated/settings/appearance/components/AppearanceSettings/components/FontSettingSection/hooks/useSystemFonts/useSystemFonts.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/config.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/components/CodeEditor/constants.ts
💤 Files with no reviewable changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/config.ts
Roshvan
changed the title
Update editor and terminal preview examples to show real Mastra patterns (createTool with zod schema, agent workspace sessions) instead of generic tRPC router code.
Roshvan
force-pushed
the
feat/desktop-font-picker
branch
from
b275cc8 to
39d764a
Compare
Collaborator
|
Amazing @Roshvan you're a G! |
Contributor
Author
My pleasure |
6 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
2 participants
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
document.fonts.check()APITest plan
bun run lint:fix— no issuesbunx tsc --noEmit -p apps/desktop/tsconfig.json— no type errorsSummary by CodeRabbit