feat: add FiraCode Nerd Font and custom font (TTF) support#57
Open
MADE-ADI wants to merge 1 commit intognoviawan:devfrom
Open
feat: add FiraCode Nerd Font and custom font (TTF) support#57MADE-ADI wants to merge 1 commit intognoviawan:devfrom
MADE-ADI wants to merge 1 commit intognoviawan:devfrom
Conversation
- Bundle FiraCode Nerd Font (Regular + Bold) in public/fonts/ with @font-face - Add 'FiraCode Nerd Font' as built-in terminal font option - New custom font feature: upload TTF files via AppPreferences UI - Custom fonts registered dynamically via FontFace API - Custom fonts appear in font family dropdown alongside built-in options - Persist custom fonts in app settings (base64 encoded) - Auto-load all custom fonts on app startup - Support up to 5 custom fonts (max 5MB each) - New files: font-loader.ts, custom-font-store.ts, use-custom-fonts.ts Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive custom font system to the Tauri renderer application. It adds type definitions, a Zustand store for state management, font loading utilities, React hooks for font lifecycle operations (load, add, remove), CSS font declarations, app initialization logic, and a complete UI for managing custom fonts in the preferences panel. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as AppPreferences UI
participant Hook as useAddCustomFont
participant Loader as font-loader
participant Store as custom-font-store
participant API as persistenceApi
User->>UI: Select TTF file
UI->>Hook: Call addCustomFont(file)
Hook->>Hook: Validate .ttf extension
Hook->>Hook: Validate file size ≤ 5MB
Hook->>Hook: Validate max 5 fonts
Hook->>Loader: readFontFile(file)
Loader-->>Hook: base64 data
Hook->>Hook: Extract fontFamily name
Hook->>Hook: Check for duplicates
Hook->>Loader: registerFont(fontFamily, base64)
Loader->>Loader: Convert to ArrayBuffer
Loader->>Loader: Create FontFace & load
Loader-->>Hook: true/false
Hook->>Store: addFont(CustomFont)
Store-->>Hook: updated state
Hook->>API: writeDebounced(CUSTOM_FONTS_KEY, fonts)
API-->>Hook: persisted
Hook-->>UI: {success: true, font}
UI->>UI: Display success message
sequenceDiagram
participant App
participant Hook as useCustomFontsLoader
participant API as persistenceApi
participant Store as custom-font-store
participant Loader as font-loader
App->>Hook: Invoke on mount
Hook->>API: Read CUSTOM_FONTS_KEY
API-->>Hook: CustomFont[]
Hook->>Store: setFonts(fonts)
Store-->>Hook: isLoaded = true
loop For each font
Hook->>Loader: registerFont(fontFamily, base64)
Loader-->>Hook: registration status
end
Hook-->>App: Fonts loaded & registered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/renderer/pages/AppPreferences.tsx (1)
110-121: Minor: Consider a more robust font selection check.The check
fontFamily.includes(font.fontFamily)on Line 116 works correctly for the current format but could be fragile if the font family string format changes. Consider an exact match against the constructed value.♻️ Proposed improvement
const handleRemoveCustomFont = async (id: string) => { setFontError(null) setFontSuccess(null) // If the removed font is currently selected, switch to default const font = customFonts.find((f) => f.id === id) - if (font && fontFamily.includes(font.fontFamily)) { + const selectedValue = font ? `"${font.fontFamily}", monospace` : '' + if (font && fontFamily === selectedValue) { handleFontFamilyChange('Menlo, Monaco, "Courier New", monospace') } await removeCustomFont(id) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/pages/AppPreferences.tsx` around lines 110 - 121, The removal handler handleRemoveCustomFont uses a fragile substring check (fontFamily.includes(font.fontFamily)); change it to a precise comparison: compute the exact font-family string that the app sets for custom fonts (the constructed value used elsewhere for selections) or normalize both sides (trim quotes/whitespace and compare the primary family token) and then check equality before calling handleFontFamilyChange; reference customFonts, font.fontFamily, fontFamily, and handleFontFamilyChange to locate and update the comparison logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/renderer/hooks/use-custom-fonts.ts`:
- Around line 101-117: The callback returned by useRemoveCustomFont captures the
stale fonts variable; update the callback to read the latest fonts from
useCustomFontStore.getState().fonts inside the async function (instead of the
captured fonts) so unregisterFont and persistenceApi.writeDebounced use the
current list, and adjust the useCallback dependency array to only include
removeFont; reference functions/values: useRemoveCustomFont,
useCustomFontStore.getState().fonts, removeFont, unregisterFont,
persistenceApi.writeDebounced, CUSTOM_FONTS_KEY, and useCallback.
- Around line 37-95: The callback returned by useAddCustomFont captures the
stale fonts variable; instead, inside the async callback read the live state
from the store (e.g., const currentFonts = useCustomFontStore.getState().fonts)
and replace all uses of fonts with currentFonts (checks for length >=
MAX_CUSTOM_FONTS, duplicate fontFamily, and composing allFonts for persistence).
Keep addFont from useCustomFontStore for updating, and update the useCallback
dependency array to omit fonts (e.g., [addFont]) so the closure no longer relies
on a stale array; ensure persistenceApi.writeDebounced(CUSTOM_FONTS_KEY,
allFonts) uses the computed allFonts based on currentFonts.
---
Nitpick comments:
In `@src/renderer/pages/AppPreferences.tsx`:
- Around line 110-121: The removal handler handleRemoveCustomFont uses a fragile
substring check (fontFamily.includes(font.fontFamily)); change it to a precise
comparison: compute the exact font-family string that the app sets for custom
fonts (the constructed value used elsewhere for selections) or normalize both
sides (trim quotes/whitespace and compare the primary family token) and then
check equality before calling handleFontFamilyChange; reference customFonts,
font.fontFamily, fontFamily, and handleFontFamilyChange to locate and update the
comparison logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93a3a08d-e25c-4451-b39c-97de168ec24d
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/fonts/FiraCodeNerdFont-Bold.ttfis excluded by!**/*.ttfpublic/fonts/FiraCodeNerdFont-Regular.ttfis excluded by!**/*.ttf
📒 Files selected for processing (8)
src/renderer/App.tsxsrc/renderer/TauriApp.tsxsrc/renderer/hooks/use-custom-fonts.tssrc/renderer/index.csssrc/renderer/lib/font-loader.tssrc/renderer/pages/AppPreferences.tsxsrc/renderer/stores/custom-font-store.tssrc/renderer/types/settings.ts
Comment on lines
+37
to
+95
| export function useAddCustomFont(): (file: File) => Promise<{ success: true; font: CustomFont } | { success: false; error: string }> { | ||
| const addFont = useCustomFontStore((state) => state.addFont) | ||
| const fonts = useCustomFontStore((state) => state.fonts) | ||
|
|
||
| return useCallback( | ||
| async (file: File) => { | ||
| // Validate file type | ||
| if (!file.name.toLowerCase().endsWith('.ttf')) { | ||
| return { success: false, error: 'Only TTF font files are supported.' } | ||
| } | ||
|
|
||
| // Validate file size | ||
| if (file.size > MAX_FONT_FILE_SIZE) { | ||
| const sizeMB = (MAX_FONT_FILE_SIZE / 1024 / 1024).toFixed(0) | ||
| return { success: false, error: `Font file is too large. Maximum size is ${sizeMB}MB.` } | ||
| } | ||
|
|
||
| // Check limit | ||
| if (fonts.length >= MAX_CUSTOM_FONTS) { | ||
| return { success: false, error: `Maximum of ${MAX_CUSTOM_FONTS} custom fonts reached. Remove one first.` } | ||
| } | ||
|
|
||
| try { | ||
| // Read file as base64 | ||
| const base64Data = await readFontFile(file) | ||
| const name = extractFontName(file.name) | ||
| const fontFamily = `Custom-${name.replace(/\s+/g, '-')}` | ||
|
|
||
| // Check for duplicate name | ||
| if (fonts.some((f) => f.fontFamily === fontFamily)) { | ||
| return { success: false, error: `A custom font with the name "${name}" already exists.` } | ||
| } | ||
|
|
||
| // Register with browser | ||
| const registered = await registerFont(fontFamily, base64Data) | ||
| if (!registered) { | ||
| return { success: false, error: 'Failed to load font. The file may be corrupted.' } | ||
| } | ||
|
|
||
| const customFont: CustomFont = { | ||
| id: crypto.randomUUID(), | ||
| name, | ||
| fontFamily, | ||
| data: base64Data, | ||
| addedAt: Date.now() | ||
| } | ||
|
|
||
| // Update store and persist | ||
| addFont(customFont) | ||
| const allFonts = [...fonts, customFont] | ||
| await persistenceApi.writeDebounced(CUSTOM_FONTS_KEY, allFonts) | ||
|
|
||
| return { success: true, font: customFont } | ||
| } catch (err) { | ||
| return { success: false, error: `Failed to add font: ${String(err)}` } | ||
| } | ||
| }, | ||
| [addFont, fonts] | ||
| ) |
There was a problem hiding this comment.
Stale closure issue with fonts reference.
The fonts array is captured in the useCallback closure, but it can become stale between renders. If a user adds fonts quickly in succession, the callback may operate on an outdated fonts array, potentially allowing duplicate fonts or exceeding the limit.
Consider reading the current state directly from the store within the callback.
🐛 Proposed fix
export function useAddCustomFont(): (file: File) => Promise<{ success: true; font: CustomFont } | { success: false; error: string }> {
const addFont = useCustomFontStore((state) => state.addFont)
- const fonts = useCustomFontStore((state) => state.fonts)
return useCallback(
async (file: File) => {
+ // Read current fonts from store to avoid stale closure
+ const fonts = useCustomFontStore.getState().fonts
+
// Validate file type
if (!file.name.toLowerCase().endsWith('.ttf')) {
return { success: false, error: 'Only TTF font files are supported.' }
}
// ... rest of validation using fresh fonts ...
// Update store and persist
addFont(customFont)
- const allFonts = [...fonts, customFont]
+ const allFonts = useCustomFontStore.getState().fonts
await persistenceApi.writeDebounced(CUSTOM_FONTS_KEY, allFonts)
return { success: true, font: customFont }
},
- [addFont, fonts]
+ [addFont]
)
}📝 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
| export function useAddCustomFont(): (file: File) => Promise<{ success: true; font: CustomFont } | { success: false; error: string }> { | |
| const addFont = useCustomFontStore((state) => state.addFont) | |
| const fonts = useCustomFontStore((state) => state.fonts) | |
| return useCallback( | |
| async (file: File) => { | |
| // Validate file type | |
| if (!file.name.toLowerCase().endsWith('.ttf')) { | |
| return { success: false, error: 'Only TTF font files are supported.' } | |
| } | |
| // Validate file size | |
| if (file.size > MAX_FONT_FILE_SIZE) { | |
| const sizeMB = (MAX_FONT_FILE_SIZE / 1024 / 1024).toFixed(0) | |
| return { success: false, error: `Font file is too large. Maximum size is ${sizeMB}MB.` } | |
| } | |
| // Check limit | |
| if (fonts.length >= MAX_CUSTOM_FONTS) { | |
| return { success: false, error: `Maximum of ${MAX_CUSTOM_FONTS} custom fonts reached. Remove one first.` } | |
| } | |
| try { | |
| // Read file as base64 | |
| const base64Data = await readFontFile(file) | |
| const name = extractFontName(file.name) | |
| const fontFamily = `Custom-${name.replace(/\s+/g, '-')}` | |
| // Check for duplicate name | |
| if (fonts.some((f) => f.fontFamily === fontFamily)) { | |
| return { success: false, error: `A custom font with the name "${name}" already exists.` } | |
| } | |
| // Register with browser | |
| const registered = await registerFont(fontFamily, base64Data) | |
| if (!registered) { | |
| return { success: false, error: 'Failed to load font. The file may be corrupted.' } | |
| } | |
| const customFont: CustomFont = { | |
| id: crypto.randomUUID(), | |
| name, | |
| fontFamily, | |
| data: base64Data, | |
| addedAt: Date.now() | |
| } | |
| // Update store and persist | |
| addFont(customFont) | |
| const allFonts = [...fonts, customFont] | |
| await persistenceApi.writeDebounced(CUSTOM_FONTS_KEY, allFonts) | |
| return { success: true, font: customFont } | |
| } catch (err) { | |
| return { success: false, error: `Failed to add font: ${String(err)}` } | |
| } | |
| }, | |
| [addFont, fonts] | |
| ) | |
| export function useAddCustomFont(): (file: File) => Promise<{ success: true; font: CustomFont } | { success: false; error: string }> { | |
| const addFont = useCustomFontStore((state) => state.addFont) | |
| return useCallback( | |
| async (file: File) => { | |
| // Read current fonts from store to avoid stale closure | |
| const fonts = useCustomFontStore.getState().fonts | |
| // Validate file type | |
| if (!file.name.toLowerCase().endsWith('.ttf')) { | |
| return { success: false, error: 'Only TTF font files are supported.' } | |
| } | |
| // Validate file size | |
| if (file.size > MAX_FONT_FILE_SIZE) { | |
| const sizeMB = (MAX_FONT_FILE_SIZE / 1024 / 1024).toFixed(0) | |
| return { success: false, error: `Font file is too large. Maximum size is ${sizeMB}MB.` } | |
| } | |
| // Check limit | |
| if (fonts.length >= MAX_CUSTOM_FONTS) { | |
| return { success: false, error: `Maximum of ${MAX_CUSTOM_FONTS} custom fonts reached. Remove one first.` } | |
| } | |
| try { | |
| // Read file as base64 | |
| const base64Data = await readFontFile(file) | |
| const name = extractFontName(file.name) | |
| const fontFamily = `Custom-${name.replace(/\s+/g, '-')}` | |
| // Check for duplicate name | |
| if (fonts.some((f) => f.fontFamily === fontFamily)) { | |
| return { success: false, error: `A custom font with the name "${name}" already exists.` } | |
| } | |
| // Register with browser | |
| const registered = await registerFont(fontFamily, base64Data) | |
| if (!registered) { | |
| return { success: false, error: 'Failed to load font. The file may be corrupted.' } | |
| } | |
| const customFont: CustomFont = { | |
| id: crypto.randomUUID(), | |
| name, | |
| fontFamily, | |
| data: base64Data, | |
| addedAt: Date.now() | |
| } | |
| // Update store and persist | |
| addFont(customFont) | |
| const allFonts = useCustomFontStore.getState().fonts | |
| await persistenceApi.writeDebounced(CUSTOM_FONTS_KEY, allFonts) | |
| return { success: true, font: customFont } | |
| } catch (err) { | |
| return { success: false, error: `Failed to add font: ${String(err)}` } | |
| } | |
| }, | |
| [addFont] | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/use-custom-fonts.ts` around lines 37 - 95, The callback
returned by useAddCustomFont captures the stale fonts variable; instead, inside
the async callback read the live state from the store (e.g., const currentFonts
= useCustomFontStore.getState().fonts) and replace all uses of fonts with
currentFonts (checks for length >= MAX_CUSTOM_FONTS, duplicate fontFamily, and
composing allFonts for persistence). Keep addFont from useCustomFontStore for
updating, and update the useCallback dependency array to omit fonts (e.g.,
[addFont]) so the closure no longer relies on a stale array; ensure
persistenceApi.writeDebounced(CUSTOM_FONTS_KEY, allFonts) uses the computed
allFonts based on currentFonts.
Comment on lines
+101
to
+117
| export function useRemoveCustomFont(): (id: string) => Promise<void> { | ||
| const removeFont = useCustomFontStore((state) => state.removeFont) | ||
| const fonts = useCustomFontStore((state) => state.fonts) | ||
|
|
||
| return useCallback( | ||
| async (id: string) => { | ||
| const font = fonts.find((f) => f.id === id) | ||
| if (font) { | ||
| unregisterFont(font.fontFamily) | ||
| } | ||
|
|
||
| removeFont(id) | ||
| const remaining = fonts.filter((f) => f.id !== id) | ||
| await persistenceApi.writeDebounced(CUSTOM_FONTS_KEY, remaining) | ||
| }, | ||
| [removeFont, fonts] | ||
| ) |
There was a problem hiding this comment.
Same stale closure issue in useRemoveCustomFont.
The fonts reference captured in the closure can become stale. Use useCustomFontStore.getState().fonts within the callback for consistency.
🐛 Proposed fix
export function useRemoveCustomFont(): (id: string) => Promise<void> {
const removeFont = useCustomFontStore((state) => state.removeFont)
- const fonts = useCustomFontStore((state) => state.fonts)
return useCallback(
async (id: string) => {
+ const fonts = useCustomFontStore.getState().fonts
const font = fonts.find((f) => f.id === id)
if (font) {
unregisterFont(font.fontFamily)
}
removeFont(id)
- const remaining = fonts.filter((f) => f.id !== id)
+ const remaining = useCustomFontStore.getState().fonts
await persistenceApi.writeDebounced(CUSTOM_FONTS_KEY, remaining)
},
- [removeFont, fonts]
+ [removeFont]
)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/renderer/hooks/use-custom-fonts.ts` around lines 101 - 117, The callback
returned by useRemoveCustomFont captures the stale fonts variable; update the
callback to read the latest fonts from useCustomFontStore.getState().fonts
inside the async function (instead of the captured fonts) so unregisterFont and
persistenceApi.writeDebounced use the current list, and adjust the useCallback
dependency array to only include removeFont; reference functions/values:
useRemoveCustomFont, useCustomFontStore.getState().fonts, removeFont,
unregisterFont, persistenceApi.writeDebounced, CUSTOM_FONTS_KEY, and
useCallback.
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 by CodeRabbit