[WIP] Add language setting #4112
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged No assets were unchanged |
jfdoming
force-pushed
the
jfdoming/language-picker
branch
from
574a871 to
7015060
Compare
|
/update-vrt |
WalkthroughThe pull request introduces comprehensive language settings functionality across multiple files in the desktop client and core application. A new Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (6)
packages/desktop-client/src/i18n.ts (2)
8-10: Consider using a more robust path parsing approachThe current path parsing logic assumes a specific directory structure. Consider using a more robust approach to handle potential path variations.
-export const availableLanguages = Object.keys(languages).map( - path => path.split('/')[2].split('.')[0], -); +export const availableLanguages = Object.keys(languages).map(path => { + const match = path.match(/\/locale\/([^.]+)\.json$/); + return match ? match[1] : ''; +}).filter(Boolean);
43-66: Enhance error handling and logging in language setting logicThe language setting implementation could benefit from more detailed error logging and user feedback.
export const setI18NextLanguage = (language: string) => { + const logPrefix = '[Language Setting]'; if (language === 'en' && !isLanguageAvailable(language)) { - // English is always ~available since we use natural-language keys. + console.debug(`${logPrefix} Using natural-language keys for English`); return; } if (!language) { - // System default + console.debug(`${logPrefix} No language specified, using system default`); setI18NextLanguage(navigator.language || 'en'); return; } language = language.toLowerCase(); if (!availableLanguages.includes(language)) { if (language.includes('-')) { + console.debug(`${logPrefix} Falling back to primary language code: ${language.split('-')[0]}`); setI18NextLanguage(language.split('-')[0]); return; } - console.error(`Unknown locale ${language}`); + console.error(`${logPrefix} Unsupported language: ${language}`); throw new Error(`Unknown locale ${language}`); } + console.debug(`${logPrefix} Setting language to: ${language}`); i18n.changeLanguage(language || 'en'); };packages/loot-core/src/types/prefs.d.ts (1)
80-80: Add JSDoc comment for the language preferenceConsider adding documentation for the language property to improve code maintainability.
+ /** The user's preferred display language code (e.g., 'en', 'es', 'fr') */ language: string;packages/desktop-client/src/components/settings/LanguageSettings.tsx (2)
13-23: Cache language display names for better performanceThe current implementation creates a new
Intl.DisplayNamesinstance for each language. Consider caching the display names.+const getLanguageDisplayNames = (languages: string[]) => { + const cache = new Map<string, string>(); + return languages.map(lang => { + if (!cache.has(lang)) { + cache.set( + lang, + new Intl.DisplayNames([lang], { type: 'language' }).of(lang) || lang + ); + } + return [lang, cache.get(lang)]; + }); +}; const languageOptions: SelectOption[] = [ ['', 'System default'] as [string, string], Menu.line as typeof Menu.line, -].concat( - availableLanguages.map(lang => [ - lang, - new Intl.DisplayNames([lang], { - type: 'language', - }).of(lang) || lang, - ]), -); +].concat(getLanguageDisplayNames(availableLanguages));
46-78: Consider adding loading state for language optionsThe component should handle the loading state while language options are being initialized.
+ const [isLoading, setIsLoading] = React.useState(true); + + React.useEffect(() => { + // Simulate checking language availability + setIsLoading(false); + }, []); return ( <Setting primaryAction={ <Select options={languageOptions} value={isEnabled ? (language ?? '') : 'not-available'} defaultLabel={ - isEnabled ? 'Select language' : 'No languages available' + isLoading + ? 'Loading languages...' + : isEnabled + ? 'Select language' + : 'No languages available' } onChange={value => { setLanguage(value); setI18NextLanguage(value); }} - disabled={!isEnabled} + disabled={!isEnabled || isLoading} /> } >packages/loot-core/src/server/main.ts (1)
1376-1376: Consider adding type validation for the language value.The language value is loaded and returned without any validation. Consider adding type checking or sanitization to ensure the value is a valid language code.
return { floatingSidebar: floatingSidebar === 'true' ? true : false, maxMonths: stringToInteger(maxMonths || ''), documentDir: documentDir || getDefaultDocumentDir(), keyId: encryptKey && JSON.parse(encryptKey).id, - language, + language: typeof language === 'string' && language.match(/^[a-z]{2}(-[A-Z]{2})?$/) ? language : 'en', theme: theme === 'light' || theme === 'dark' ||Also applies to: 1385-1385, 1395-1395
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
packages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-4-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-5-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.mobile.test.js-snapshots/Mobile-Settings-checks-that-settings-page-can-be-opened-6-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.js-snapshots/Settings-checks-the-page-visuals-1-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.js-snapshots/Settings-checks-the-page-visuals-2-chromium-linux.pngis excluded by!**/*.pngpackages/desktop-client/e2e/settings.test.js-snapshots/Settings-checks-the-page-visuals-3-chromium-linux.pngis excluded by!**/*.pngupcoming-release-notes/4112.mdis excluded by!**/*.md
📒 Files selected for processing (6)
packages/desktop-client/src/components/settings/LanguageSettings.tsx(1 hunks)packages/desktop-client/src/components/settings/index.tsx(2 hunks)packages/desktop-client/src/i18n.ts(2 hunks)packages/loot-core/src/client/actions/prefs.ts(2 hunks)packages/loot-core/src/server/main.ts(4 hunks)packages/loot-core/src/types/prefs.d.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Functional
- GitHub Check: Visual regression
🔇 Additional comments (2)
packages/desktop-client/src/components/settings/index.tsx (1)
35-35: LGTM! The language settings integration looks good.The
LanguageSettingscomponent is properly imported and placed in a logical position within the settings interface, between format and authentication settings.Also applies to: 199-199
packages/loot-core/src/server/main.ts (1)
1349-1351: LGTM! Language preference persistence is properly implemented.The language preference is correctly saved to async storage when present in the prefs object.
| // We need to load translations before the app renders | ||
| setI18NextLanguage(prefs.language); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling potential errors in language initialization
The language initialization should handle potential errors to prevent app crashes.
- // We need to load translations before the app renders
- setI18NextLanguage(prefs.language);
+ try {
+ // We need to load translations before the app renders
+ setI18NextLanguage(prefs.language);
+ } catch (error) {
+ console.error('[Preferences] Failed to set language:', error);
+ // Fallback to English
+ setI18NextLanguage('en');
+ }📝 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.
| // We need to load translations before the app renders | |
| setI18NextLanguage(prefs.language); | |
| try { | |
| // We need to load translations before the app renders | |
| setI18NextLanguage(prefs.language); | |
| } catch (error) { | |
| console.error('[Preferences] Failed to set language:', error); | |
| // Fallback to English | |
| setI18NextLanguage('en'); | |
| } |
| <Select | ||
| options={languageOptions} | ||
| value={isEnabled ? (language ?? '') : 'not-available'} | ||
| defaultLabel={ | ||
| isEnabled ? 'Select language' : 'No languages available' | ||
| } | ||
| onChange={value => { | ||
| setLanguage(value); | ||
| setI18NextLanguage(value); | ||
| }} | ||
| disabled={!isEnabled} | ||
| /> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA labels for better accessibility
The language selector should have appropriate ARIA labels for better accessibility.
<Select
options={languageOptions}
value={isEnabled ? (language ?? '') : 'not-available'}
+ aria-label="Select display language"
defaultLabel={
isEnabled ? 'Select language' : 'No languages available'
}
onChange={value => {
setLanguage(value);
setI18NextLanguage(value);
}}
disabled={!isEnabled}
/>📝 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.
| <Select | |
| options={languageOptions} | |
| value={isEnabled ? (language ?? '') : 'not-available'} | |
| defaultLabel={ | |
| isEnabled ? 'Select language' : 'No languages available' | |
| } | |
| onChange={value => { | |
| setLanguage(value); | |
| setI18NextLanguage(value); | |
| }} | |
| disabled={!isEnabled} | |
| /> | |
| } | |
| <Select | |
| options={languageOptions} | |
| value={isEnabled ? (language ?? '') : 'not-available'} | |
| aria-label="Select display language" | |
| defaultLabel={ | |
| isEnabled ? 'Select language' : 'No languages available' | |
| } | |
| onChange={value => { | |
| setLanguage(value); | |
| setI18NextLanguage(value); | |
| }} | |
| disabled={!isEnabled} | |
| /> | |
| } |
Allows choosing a non-English language for the UI