feat: add system fonts support and enhance font customization options#156
feat: add system fonts support and enhance font customization options#156MareDevi wants to merge 1 commit intodannysmith:mainfrom
Conversation
- Added `tauri-plugin-system-fonts` to manage system fonts. - Updated `Cargo.toml` and `Cargo.lock` to include new dependencies. - Modified default capabilities to allow system fonts access. - Integrated system fonts plugin in the Tauri application. - Enhanced CSS to utilize system fonts for better UI consistency. - Implemented font selection in preferences with a new `FontSelector` component. - Added hooks to fetch and manage system fonts dynamically. - Updated global settings to include customizable font options for interface and editor.
📝 WalkthroughWalkthroughThis pull request introduces system font selection functionality to the application. It adds the Tauri system-fonts plugin dependency, creates a FontSelector React component, integrates system font querying, extends global settings with font customization options, and updates CSS variables and editor theming to apply selected fonts across the UI. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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)
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 |
There was a problem hiding this comment.
Pull request overview
This PR adds system font support end-to-end (Tauri plugin + frontend selection UI) and wires user font preferences into CSS variables so both the app UI and editor can be restyled dynamically.
Changes:
- Integrates
tauri-plugin-system-fonts(Rust) andtauri-plugin-system-fonts-api(TS) and enables the corresponding capability. - Adds new global settings for font customization and applies them at runtime via CSS variables.
- Introduces a preferences UI (
FontSelector) and a query hook to load system fonts.
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/setup.ts | Adds a localStorage mock for tests. |
| src/lib/project-registry/types.ts | Extends GlobalSettings.appearance with fonts. |
| src/lib/project-registry/index.ts | Deep-merges appearance.fonts during updates. |
| src/lib/project-registry/defaults.ts | Defines default font preferences (and editor base font size). |
| src/lib/editor/syntax/highlightStyle.ts | Switches code font to a CSS variable. |
| src/lib/editor/extensions/theme.ts | Applies CSS-variable code font to codeblock lines. |
| src/hooks/queries/useSystemFontsQuery.ts | Adds a React Query hook to fetch system fonts. |
| src/components/preferences/panes/GeneralPane.tsx | Adds UI controls to set/reset interface/editor fonts. |
| src/components/preferences/FontSelector.tsx | New searchable combobox for selecting fonts (default + system). |
| src/components/layout/Layout.tsx | Applies selected fonts by setting CSS variables on :root. |
| src/components/editor/Editor.css | Adds/uses editor font CSS variables for body/italic/code. |
| src/App.css | Adds --font-interface and uses it globally for body. |
| src-tauri/src/lib.rs | Registers the system fonts Tauri plugin. |
| src-tauri/capabilities/default.json | Grants "system-fonts:default" capability. |
| src-tauri/Cargo.toml | Adds tauri-plugin-system-fonts dependency. |
| src-tauri/Cargo.lock | Locks new Rust dependencies. |
| pnpm-lock.yaml | Locks new JS dependency and platform libc metadata. |
| package.json | Adds tauri-plugin-system-fonts-api. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) { | ||
| const mockStorage: Record<string, string> = {} | ||
| const storageMock = { | ||
| getItem: vi.fn((key: string) => mockStorage[key] || null), |
There was a problem hiding this comment.
This getItem mock returns null when the stored value is an empty string because it uses mockStorage[key] || null. Real localStorage.getItem returns '' for an empty-string value. Use an explicit key presence check (e.g., Object.prototype.hasOwnProperty.call(mockStorage, key)) so falsy values are preserved.
| getItem: vi.fn((key: string) => mockStorage[key] || null), | |
| getItem: vi.fn((key: string) => | |
| Object.prototype.hasOwnProperty.call(mockStorage, key) | |
| ? mockStorage[key] | |
| : null | |
| ), |
| const handleResetFont = useCallback( | ||
| (type: 'interface' | 'editorBody' | 'editorCode') => { | ||
| const defaults = { | ||
| interface: 'system-ui, -apple-system, sans-serif', | ||
| editorBody: 'iA Writer Duo', | ||
| editorCode: 'iA Writer Mono', | ||
| } | ||
| handleFontChange(type, defaults[type]) |
There was a problem hiding this comment.
handleResetFont('interface') resets to a font stack string ("system-ui, -apple-system, sans-serif"), but FontSelector is built around selecting a single font name from defaultFonts/system fonts. This means the reset value likely won’t match any selectable item (no checkmark state, and users can’t re-select that exact stack). Consider either (1) storing a single interface font family (and optionally appending a standard fallback stack when applying it), or (2) adding an explicit "System default" option in FontSelector whose value is the full stack. Also, try to avoid duplicating the default strings here—reusing the values from DEFAULT_GLOBAL_SETTINGS.appearance.fonts would prevent drift with the defaults in defaults.ts/App.css.
| var(--editor-font-family-body), 'iA Writer Duo Variable', 'iA Writer Duo', | ||
| -apple-system, 'Segoe UI', 'Roboto', sans-serif; | ||
| --editor-font-family-italic: | ||
| 'iA Writer Duo Variable Italic', 'iA Writer Duo', -apple-system, 'Segoe UI', | ||
| 'Roboto', sans-serif; | ||
| var(--editor-font-family-body-italic), 'iA Writer Duo Variable Italic', | ||
| 'iA Writer Duo', -apple-system, 'Segoe UI', 'Roboto', sans-serif; | ||
| --editor-font-family-code: | ||
| var(--editor-font-family-mono), 'iA Writer Mono Variable', 'iA Writer Mono', | ||
| monospace; |
There was a problem hiding this comment.
The var(--editor-font-family-body) / var(--editor-font-family-body-italic) / var(--editor-font-family-mono) references have no fallback values. If those base variables aren’t set (e.g., on first paint before the Layout effect runs, or if a user never sets a code font), the entire custom property becomes invalid and the editor will lose the intended default font stack. Use var(--… , <fallback>) in these definitions (or define defaults for the base variables) so the fallback fonts always apply.
| var(--editor-font-family-code), 'iA Writer Mono Variable', 'iA Writer Mono', | ||
| monospace; |
There was a problem hiding this comment.
font-family: var(--editor-font-family-code), ... has the same issue as the root variables: if --editor-font-family-code is unset/invalid, the whole font-family declaration is dropped and the fallback list won’t apply. Add a fallback to the var() (or ensure --editor-font-family-code is always defined) so inline code reliably uses the mono stack.
| var(--editor-font-family-code), 'iA Writer Mono Variable', 'iA Writer Mono', | |
| monospace; | |
| var(--editor-font-family-code, 'iA Writer Mono Variable'), | |
| 'iA Writer Mono Variable', 'iA Writer Mono', monospace; |
| if ( | ||
| typeof localStorage === 'undefined' || | ||
| typeof localStorage.getItem !== 'function' | ||
| ) { |
There was a problem hiding this comment.
This check can still crash in jsdom opaque-origin scenarios because accessing localStorage may throw a SecurityError (it’s not always safe to reference it in a typeof localStorage guard). Wrap the detection in a try/catch and fall back to the mock when access fails.
| if ( | |
| typeof localStorage === 'undefined' || | |
| typeof localStorage.getItem !== 'function' | |
| ) { | |
| let shouldMockStorage = false | |
| try { | |
| if ( | |
| typeof globalThis.localStorage === 'undefined' || | |
| typeof globalThis.localStorage.getItem !== 'function' | |
| ) { | |
| shouldMockStorage = true | |
| } | |
| } catch { | |
| // Accessing localStorage can throw in some jsdom opaque-origin scenarios | |
| shouldMockStorage = true | |
| } | |
| if (shouldMockStorage) { |
| fonts: { | ||
| ...this.globalSettings.appearance.fonts, | ||
| ...settings.appearance?.fonts, | ||
| }, |
There was a problem hiding this comment.
updateGlobalSettings does a nested merge for appearance.fonts, but ...this.globalSettings.appearance.fonts will throw if fonts is undefined (object spread calls ToObject). This can happen right after a v1→v2 migration because migrateGlobalSettingsV1toV2 currently returns an appearance object without fonts, and loadGlobalSettings returns the migrated object directly (without merging defaults) in that code path. Use nullish coalescing when spreading (e.g., ...(this.globalSettings.appearance.fonts ?? {})) and ensure the migration or post-migration load populates the new defaults so the in-memory settings shape is consistent.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/hooks/queries/useSystemFontsQuery.ts (1)
12-13: Use the shared query-key factory here.Hardcoding
['system-fonts']makes this hook an exception to the rest of the query invalidation story. Please route it throughsrc/lib/query-keys.tsbefore this gets reused elsewhere.As per coding guidelines, "Define query keys using the factory pattern in
src/lib/query-keys.ts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/queries/useSystemFontsQuery.ts` around lines 12 - 13, The hook currently hardcodes queryKey: ['system-fonts']; replace this with the shared factory by using the exported query key factory (e.g., queryKeys.systemFonts() or systemFontsKey()) instead, import the factory from the query-keys module at the top of useSystemFontsQuery.ts, and pass its result to useQuery's queryKey so this hook participates in the centralized query-key/invalidation strategy (update the useQuery call in useSystemFontsQuery to use the factory symbol rather than the literal array).src/components/layout/Layout.tsx (1)
54-56: AdduseShallowfor thefontsobject subscription.The
fontsselector returns an object, which could cause unnecessary re-renders if the reference changes even when the content is the same. TheheadingColorselector (line 48-50) correctly usesuseShallowfor the same reason.♻️ Proposed fix
const fonts = useProjectStore( - state => state.globalSettings?.appearance?.fonts + useShallow(state => state.globalSettings?.appearance?.fonts) )As per coding guidelines: "Use
useShallowfor Zustand object/array subscriptions to prevent re-renders from reference changes"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/layout/Layout.tsx` around lines 54 - 56, The fonts selector uses useProjectStore to subscribe to an object (state.globalSettings?.appearance?.fonts) and should use shallow comparison to avoid unnecessary re-renders; update the subscription to pass useShallow so the fonts object is compared shallowly (similar to the existing headingColor subscription), i.e., find the useProjectStore call that assigns fonts and add useShallow as the second argument to the hook to prevent reference-change re-renders.src/components/preferences/panes/GeneralPane.tsx (1)
34-44: Hardcoded defaults duplicate values fromdefaults.ts.The default font values here (lines 37-39) duplicate those defined in
src/lib/project-registry/defaults.ts(lines 43-45). If the defaults change in one place, they could fall out of sync.Consider importing and referencing the defaults from the single source of truth:
♻️ Proposed refactor to use centralized defaults
+import { DEFAULT_GLOBAL_SETTINGS } from '@/lib/project-registry/defaults' + const handleResetFont = useCallback( (type: 'interface' | 'editorBody' | 'editorCode') => { - const defaults = { - interface: 'system-ui, -apple-system, sans-serif', - editorBody: 'iA Writer Duo', - editorCode: 'iA Writer Mono', - } + const defaults = DEFAULT_GLOBAL_SETTINGS.appearance.fonts handleFontChange(type, defaults[type]) }, [handleFontChange] )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/preferences/panes/GeneralPane.tsx` around lines 34 - 44, The reset handler duplicates font defaults; update handleResetFont to import and reference the single-source defaults from src/lib/project-registry/defaults.ts instead of hardcoding values. Replace the local defaults object used in handleResetFont with the imported defaults (use the exported names from defaults.ts) and call handleFontChange(type, importedDefaults[type]) so values stay in sync; ensure the import is added at the top of the file and the type union ('interface' | 'editorBody' | 'editorCode') maps to the corresponding keys in the imported defaults.
🤖 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/test/setup.ts`:
- Around line 23-31: getItem currently returns mockStorage[key] || null which
treats stored empty strings as missing; change it to check presence (e.g.
hasOwnProperty or key in mockStorage) and return the stored value when present,
otherwise null; do the same for the key(index) implementation so an existing
empty-string value is returned instead of null; replace the clear() forEach with
an explicit for loop that iterates Object.keys(mockStorage) and deletes each key
(avoiding relying on the callback return value); keep setItem, removeItem,
mockStorage, getItem, key, and clear identifiers as the locations to modify.
---
Nitpick comments:
In `@src/components/layout/Layout.tsx`:
- Around line 54-56: The fonts selector uses useProjectStore to subscribe to an
object (state.globalSettings?.appearance?.fonts) and should use shallow
comparison to avoid unnecessary re-renders; update the subscription to pass
useShallow so the fonts object is compared shallowly (similar to the existing
headingColor subscription), i.e., find the useProjectStore call that assigns
fonts and add useShallow as the second argument to the hook to prevent
reference-change re-renders.
In `@src/components/preferences/panes/GeneralPane.tsx`:
- Around line 34-44: The reset handler duplicates font defaults; update
handleResetFont to import and reference the single-source defaults from
src/lib/project-registry/defaults.ts instead of hardcoding values. Replace the
local defaults object used in handleResetFont with the imported defaults (use
the exported names from defaults.ts) and call handleFontChange(type,
importedDefaults[type]) so values stay in sync; ensure the import is added at
the top of the file and the type union ('interface' | 'editorBody' |
'editorCode') maps to the corresponding keys in the imported defaults.
In `@src/hooks/queries/useSystemFontsQuery.ts`:
- Around line 12-13: The hook currently hardcodes queryKey: ['system-fonts'];
replace this with the shared factory by using the exported query key factory
(e.g., queryKeys.systemFonts() or systemFontsKey()) instead, import the factory
from the query-keys module at the top of useSystemFontsQuery.ts, and pass its
result to useQuery's queryKey so this hook participates in the centralized
query-key/invalidation strategy (update the useQuery call in useSystemFontsQuery
to use the factory symbol rather than the literal array).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6e816d0e-1f42-4de1-ad84-7cbb2f3124f4
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yamlsrc-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
package.jsonsrc-tauri/Cargo.tomlsrc-tauri/capabilities/default.jsonsrc-tauri/src/lib.rssrc/App.csssrc/components/editor/Editor.csssrc/components/layout/Layout.tsxsrc/components/preferences/FontSelector.tsxsrc/components/preferences/panes/GeneralPane.tsxsrc/hooks/queries/useSystemFontsQuery.tssrc/lib/editor/extensions/theme.tssrc/lib/editor/syntax/highlightStyle.tssrc/lib/project-registry/defaults.tssrc/lib/project-registry/index.tssrc/lib/project-registry/types.tssrc/test/setup.ts
| getItem: vi.fn((key: string) => mockStorage[key] || null), | ||
| setItem: vi.fn((key: string, value: { toString(): string }) => { | ||
| mockStorage[key] = value.toString() | ||
| }), | ||
| removeItem: vi.fn((key: string) => { | ||
| delete mockStorage[key] | ||
| }), | ||
| clear: vi.fn(() => { | ||
| Object.keys(mockStorage).forEach(key => delete mockStorage[key]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "setup.ts" -path "*/test/*" | head -5Repository: dannysmith/astro-editor
Length of output: 86
🏁 Script executed:
cat -n src/test/setup.ts | head -50Repository: dannysmith/astro-editor
Length of output: 1908
🏁 Script executed:
# Check if there's a linting configuration in the project
fd -e "eslintrc" -e "biome.json" -e ".eslintignore" . | head -10Repository: dannysmith/astro-editor
Length of output: 49
🏁 Script executed:
# Check if there are any tests using localStorage with empty strings
rg "localStorage|mockStorage" src/test --type ts -A 2 -B 2 | head -40Repository: dannysmith/astro-editor
Length of output: 1524
🏁 Script executed:
# Also check the actual web Storage interface - verify the proposed fix is correct
# by checking if there are any other methods on storageMock that might have similar issues
cat -n src/test/setup.ts | sed -n '22,43p'Repository: dannysmith/astro-editor
Length of output: 897
🏁 Script executed:
# Check for any linting errors in the file
rg "field-sizing|forEach" src/test/setup.ts --type ts -A 1Repository: dannysmith/astro-editor
Length of output: 145
🏁 Script executed:
# Verify the Storage API behavior by checking if there are any uses of localStorage
# with empty strings in the codebase
rg "setItem.*['\"]" src --type ts -B 1 -A 1 | grep -E "(setItem|'')" | head -20Repository: dannysmith/astro-editor
Length of output: 163
🏁 Script executed:
# Check if there's a linting configuration and what rules are active
cat biome.json 2>/dev/null || cat eslint.config.mjs 2>/dev/null || echo "No config found"Repository: dannysmith/astro-editor
Length of output: 1841
🏁 Script executed:
# Check if there are any tests that use empty strings with localStorage
rg "setItem.*['\"].*['\"]" src --type ts -B 2 -A 2Repository: dannysmith/astro-editor
Length of output: 503
🏁 Script executed:
# Double-check the actual behavior: verify the Storage interface contract
# by looking at any test files that may exercise this mock
fd "\.test\.ts$" src/test | head -5 | xargs -I {} sh -c 'echo "=== {} ===" && head -30 {}'Repository: dannysmith/astro-editor
Length of output: 49
Fix the localStorage mock to handle empty string values correctly.
The getItem implementation using mockStorage[key] || null will incorrectly return null for intentionally stored empty string values ('') instead of returning the stored string. The Storage.getItem() API only returns null when the key is absent; otherwise it returns the stored value regardless of its contents. Additionally, the key() method on line 33 has the same issue.
The clear() method also uses a forEach callback that returns the result of the delete operator—use a for loop instead for cleaner code.
🩹 Proposed fix
const storageMock = {
- getItem: vi.fn((key: string) => mockStorage[key] || null),
+ getItem: vi.fn((key: string) =>
+ key in mockStorage ? mockStorage[key] : null
+ ),
setItem: vi.fn((key: string, value: { toString(): string }) => {
mockStorage[key] = value.toString()
}),
removeItem: vi.fn((key: string) => {
delete mockStorage[key]
}),
clear: vi.fn(() => {
- Object.keys(mockStorage).forEach(key => delete mockStorage[key])
+ for (const key of Object.keys(mockStorage)) {
+ delete mockStorage[key]
+ }
}),
- key: vi.fn((index: number) => Object.keys(mockStorage)[index] || null),
+ key: vi.fn((index: number) => {
+ const keys = Object.keys(mockStorage)
+ return index < keys.length ? keys[index] : null
+ }),📝 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.
| getItem: vi.fn((key: string) => mockStorage[key] || null), | |
| setItem: vi.fn((key: string, value: { toString(): string }) => { | |
| mockStorage[key] = value.toString() | |
| }), | |
| removeItem: vi.fn((key: string) => { | |
| delete mockStorage[key] | |
| }), | |
| clear: vi.fn(() => { | |
| Object.keys(mockStorage).forEach(key => delete mockStorage[key]) | |
| getItem: vi.fn((key: string) => | |
| key in mockStorage ? mockStorage[key] : null | |
| ), | |
| setItem: vi.fn((key: string, value: { toString(): string }) => { | |
| mockStorage[key] = value.toString() | |
| }), | |
| removeItem: vi.fn((key: string) => { | |
| delete mockStorage[key] | |
| }), | |
| clear: vi.fn(() => { | |
| for (const key of Object.keys(mockStorage)) { | |
| delete mockStorage[key] | |
| } | |
| }), | |
| key: vi.fn((index: number) => { | |
| const keys = Object.keys(mockStorage) | |
| return index < keys.length ? keys[index] : null | |
| }), |
🧰 Tools
🪛 Biome (2.4.6)
[error] 31-31: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/setup.ts` around lines 23 - 31, getItem currently returns
mockStorage[key] || null which treats stored empty strings as missing; change it
to check presence (e.g. hasOwnProperty or key in mockStorage) and return the
stored value when present, otherwise null; do the same for the key(index)
implementation so an existing empty-string value is returned instead of null;
replace the clear() forEach with an explicit for loop that iterates
Object.keys(mockStorage) and deletes each key (avoiding relying on the callback
return value); keep setItem, removeItem, mockStorage, getItem, key, and clear
identifiers as the locations to modify.
|
Hey @MareDevi, thanks for taking the time to put this together. I'm going to close this without merging, but I want to explain why properly so it's useful context for any future contributions. The main issue is: too many unrelated changes bundled together. This PR mixes font customisation with lockfile churn (libc annotations), test setup changes, and what look like sneaked-in defaults modifications. That makes it really hard to review with confidence. PRs should do one thing - if there are separate improvements, they should be separate PRs. On editor font customisation specifically: this won't be happening. The editor is deliberately built around the iA Writer font family. The typography system (font sizes, line heights, letter spacing, content measure breakpoints) is all tuned to these specific fonts across five responsive breakpoints. Swapping in arbitrary fonts would break that entire system. This is a core design decision: the editor fonts are a part of the product. That said, the interface/UI font handling on Linux is a real issue since Linux has no guaranteed system font. I'll open a separate issue to explore a UI font preference for Linux users specifically. |
This pull request introduces support for system font customization in the application, enabling users to select and apply system fonts for both the interface and editor. The changes span dependency updates, backend plugin integration, capability adjustments, and frontend logic for dynamic font application.
System font support and customization:
tauri-plugin-system-fonts-apidependency topackage.jsonandpnpm-lock.yaml, and integrated the corresponding Rust plugin (tauri-plugin-system-fonts) insrc-tauri/Cargo.tomlandsrc-tauri/src/lib.rsto enable system font access and management. [1] [2] [3] [4] [5]"system-fonts:default"insrc-tauri/capabilities/default.json.Frontend font application logic:
--font-interfaceinsrc/App.cssand refactored the global font-family declaration to use this variable, enabling dynamic interface font changes. [1] [2]src/components/editor/Editor.css, allowing these to be set dynamically. [1] [2]src/components/layout/Layout.tsxto dynamically apply user-selected fonts from preferences to the relevant CSS variables, updating the interface and editor fonts in real time. [1] [2]Dependency and platform support updates:
pnpm-lock.yamlto include platform-specificlibcrequirements for various dependencies, ensuring compatibility across Linux distributions and architectures. [1] [2] [3] [4] [5] [6] [7]snapshotssection ofpnpm-lock.yaml.- Addedtauri-plugin-system-fontsto manage system fonts.Cargo.tomlandCargo.lockto include new dependencies.FontSelectorcomponent.Summary by CodeRabbit
New Features
Chores