Feature/font customization#730
Draft
rm1dev wants to merge 3 commits into
Draft
Conversation
Add dir="auto" to chat message content and composer textarea so Persian and Arabic text automatically renders right-to-left while English and other LTR text remains unaffected.
- Add font selection and custom font input to code editor settings - Add appearance font settings with font family, custom font, and font size options - Create fontSettings utility module for font management - Update Markdown component to respect code editor font preferences - Add event listener for real-time font changes in code blocks - Extend AppearanceSettingsTab with new font customization controls - Update all locale files (en, de, it, ja, ko, ru, tr, zh-CN) with font setting labels - Persist font preferences to localStorage and sync across application - Enable users to select from default fonts or specify custom font families
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (5)
📝 WalkthroughWalkthroughAdds appearance and code-editor font settings (types, defaults, controller persistence and events), applies bidirectional ChangesAppearance & rendering changes
Suggested reviewers
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 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 |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/settings/hooks/useSettingsController.ts (1)
88-102:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse non-empty fallback semantics when hydrating font settings from storage.
Lines 94-95 and 99-101 use
??, so''is accepted and can put invalid empty values into settings state. Prefer||(or a small normalizer) for these fields.Proposed fix
- font: localStorage.getItem('codeEditorFont') ?? DEFAULT_CODE_EDITOR_SETTINGS.font, - customFont: localStorage.getItem('codeEditorCustomFont') ?? DEFAULT_CODE_EDITOR_SETTINGS.customFont, + font: localStorage.getItem('codeEditorFont') || DEFAULT_CODE_EDITOR_SETTINGS.font, + customFont: localStorage.getItem('codeEditorCustomFont') || DEFAULT_CODE_EDITOR_SETTINGS.customFont, @@ - font: localStorage.getItem('appearanceFont') ?? DEFAULT_APPEARANCE_FONT_SETTINGS.font, - customFont: localStorage.getItem('appearanceCustomFont') ?? DEFAULT_APPEARANCE_FONT_SETTINGS.customFont, - fontSize: localStorage.getItem('appearanceFontSize') ?? DEFAULT_APPEARANCE_FONT_SETTINGS.fontSize, + font: localStorage.getItem('appearanceFont') || DEFAULT_APPEARANCE_FONT_SETTINGS.font, + customFont: localStorage.getItem('appearanceCustomFont') || DEFAULT_APPEARANCE_FONT_SETTINGS.customFont, + fontSize: localStorage.getItem('appearanceFontSize') || DEFAULT_APPEARANCE_FONT_SETTINGS.fontSize,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/settings/hooks/useSettingsController.ts` around lines 88 - 102, readCodeEditorSettings and readAppearanceFontSettings currently use the nullish coalescing operator (??) for font-related fields, which allows empty strings from localStorage to be accepted into state; change those font and customFont and fontSize fallbacks to use logical OR (||) or a small normalizer (e.g., value => value?.trim() || DEFAULT_...) so empty strings become the default values; update occurrences in readCodeEditorSettings (font, customFont, fontSize) and readAppearanceFontSettings (font, customFont, fontSize) to use this non-empty fallback logic.
🧹 Nitpick comments (1)
src/utils/fontSettings.ts (1)
35-42: ⚡ Quick winMake
initializeFontSettingsteardown-safe to avoid duplicate global listeners.If this initializer is called more than once (tests/HMR/re-init paths), listeners accumulate and handlers run repeatedly.
Proposed refactor
export const initializeFontSettings = () => { applyAppearanceFontSettings(); applyCodeEditorFontSettings(); // Listen for settings changes window.addEventListener('appearanceFontSettingsChanged', applyAppearanceFontSettings); window.addEventListener('codeEditorSettingsChanged', applyCodeEditorFontSettings); + + return () => { + window.removeEventListener('appearanceFontSettingsChanged', applyAppearanceFontSettings); + window.removeEventListener('codeEditorSettingsChanged', applyCodeEditorFontSettings); + }; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/fontSettings.ts` around lines 35 - 42, The initializer currently adds global listeners each call causing duplicates; make initializeFontSettings idempotent by first removing existing listeners for 'appearanceFontSettingsChanged' and 'codeEditorSettingsChanged' (using window.removeEventListener with the same handler functions applyAppearanceFontSettings and applyCodeEditorFontSettings) before calling addEventListener, and optionally return or export a teardown function that calls removeEventListener for those same handlers so tests/HMR can clean up; reference applyAppearanceFontSettings, applyCodeEditorFontSettings, and the event names when implementing.
🤖 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/utils/fontSettings.ts`:
- Around line 4-15: The code reads localStorage.getItem('appearanceFontSize')
into fontSize and injects it directly into body.style.fontSize causing malformed
values to create invalid CSS; update the logic where fontSize is derived (the
const fontSize assignment) to parse the stored value as an integer (use parseInt
or Number), validate it, and clamp it to a safe range (e.g., min 12, max 24)
falling back to the default 16 when parsing fails or value is out of bounds;
then apply the clamped numeric value when setting body.style.fontSize (the usage
around body.style.fontSize = `${fontSize}px`) so only normalized, safe pixel
sizes are written.
---
Outside diff comments:
In `@src/components/settings/hooks/useSettingsController.ts`:
- Around line 88-102: readCodeEditorSettings and readAppearanceFontSettings
currently use the nullish coalescing operator (??) for font-related fields,
which allows empty strings from localStorage to be accepted into state; change
those font and customFont and fontSize fallbacks to use logical OR (||) or a
small normalizer (e.g., value => value?.trim() || DEFAULT_...) so empty strings
become the default values; update occurrences in readCodeEditorSettings (font,
customFont, fontSize) and readAppearanceFontSettings (font, customFont,
fontSize) to use this non-empty fallback logic.
---
Nitpick comments:
In `@src/utils/fontSettings.ts`:
- Around line 35-42: The initializer currently adds global listeners each call
causing duplicates; make initializeFontSettings idempotent by first removing
existing listeners for 'appearanceFontSettingsChanged' and
'codeEditorSettingsChanged' (using window.removeEventListener with the same
handler functions applyAppearanceFontSettings and applyCodeEditorFontSettings)
before calling addEventListener, and optionally return or export a teardown
function that calls removeEventListener for those same handlers so tests/HMR can
clean up; reference applyAppearanceFontSettings, applyCodeEditorFontSettings,
and the event names when implementing.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e0dedc8d-5958-4b01-a538-9981d5862dd3
📒 Files selected for processing (18)
src/components/chat/view/subcomponents/ChatComposer.tsxsrc/components/chat/view/subcomponents/Markdown.tsxsrc/components/chat/view/subcomponents/MessageComponent.tsxsrc/components/settings/constants/constants.tssrc/components/settings/hooks/useSettingsController.tssrc/components/settings/types/types.tssrc/components/settings/view/Settings.tsxsrc/components/settings/view/tabs/AppearanceSettingsTab.tsxsrc/i18n/locales/de/settings.jsonsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/it/settings.jsonsrc/i18n/locales/ja/settings.jsonsrc/i18n/locales/ko/settings.jsonsrc/i18n/locales/ru/settings.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/main.jsxsrc/utils/fontSettings.ts
Comment on lines
+4
to
+15
| const fontSize = localStorage.getItem('appearanceFontSize') || '16'; | ||
|
|
||
| const body = document.body; | ||
|
|
||
| if (font === 'custom' && customFont.trim()) { | ||
| body.style.fontFamily = customFont; | ||
| } else { | ||
| body.style.fontFamily = '-apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Helvetica Neue", Arial, sans-serif'; | ||
| } | ||
|
|
||
| body.style.fontSize = `${fontSize}px`; | ||
| }; |
Contributor
There was a problem hiding this comment.
Normalize and clamp appearanceFontSize before applying styles.
Line 14 currently trusts raw localStorage text; malformed values become invalid CSS and silently fail.
Proposed fix
- const fontSize = localStorage.getItem('appearanceFontSize') || '16';
+ const rawFontSize = localStorage.getItem('appearanceFontSize');
+ const parsedFontSize = Number.parseInt(rawFontSize ?? '', 10);
+ const fontSize = Number.isFinite(parsedFontSize) && parsedFontSize >= 10 && parsedFontSize <= 32
+ ? parsedFontSize
+ : 16;
@@
- body.style.fontSize = `${fontSize}px`;
+ body.style.fontSize = `${fontSize}px`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/fontSettings.ts` around lines 4 - 15, The code reads
localStorage.getItem('appearanceFontSize') into fontSize and injects it directly
into body.style.fontSize causing malformed values to create invalid CSS; update
the logic where fontSize is derived (the const fontSize assignment) to parse the
stored value as an integer (use parseInt or Number), validate it, and clamp it
to a safe range (e.g., min 12, max 24) falling back to the default 16 when
parsing fails or value is out of bounds; then apply the clamped numeric value
when setting body.style.fontSize (the usage around body.style.fontSize =
`${fontSize}px`) so only normalized, safe pixel sizes are written.
blackmammoth
reviewed
Jun 2, 2026
Collaborator
There was a problem hiding this comment.
Hey @rm1dev, thanks for the PR. However, here are some concerns:
- It's not clear how to change fonts. I think it would be better if the custom fonts are from a dropdown list instead.
- The font size changer doesn't work. I changed it from 12 to 20px and it's still the same.
After this changes have been addressed, I would love to review it again. So, I'm making this a draft for now.
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 by CodeRabbit