Feature: Improve browser locale detection and unify language selection behavior #371
Conversation
WalkthroughThis PR changes language preference handling: UserProfile.vue introduces a LanguagePreference type (Locale | 'browser'), replaces the Listbox v-model Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Files to pay extra attention to:
Possibly related PRs
Suggested Reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
frontend/src/i18n/index.ts (2)
105-133: Locale normalization logic looks solid for supported languages
mapToLocalecleanly handles empty input, normalizes underscore vs. dash, accepts exact enum values, and falls back to a base‑language mapping, so short browser values like"de"or"fr"will reliably resolve to one of the definedLocaleentries. I don’t see correctness issues here for the current set of supported locales.If you ever need to extend this, a small optional improvement would be to precompute a
Record<string, Locale>map (including lower‑cased variants) to avoid repeatedObject.valuesscans and make additions less error‑prone than growing theswitch, but that’s purely a maintainability tweak.
135-143: detectBrowserLocale() is SSR‑safe and prefers user language orderThe guard around
navigatorplus thenavigator.languages[0] || navigator.language || EN_USfallback chain is a good, SSR‑safe way to derive the browser locale and respects the user’s preferred languages array when available.If you want to slightly simplify readability, you could factor the
typeof navigator !== 'undefined'check once and then use optional chaining (navigator?.languages?.[0] ?? navigator?.language ?? Locale.EN_US), but that’s optional and doesn’t change behavior.frontend/src/components/UserProfile.vue (2)
79-91: Minor typing/structure improvement for browserLocale
detectBrowserLocale()already guarantees a validLocale, sobrowserLocaledoesn’t need to be aref<string>—it can be a simpleLocalevalue. That tightens typing (no accidental non‑Locale strings) and removes an unnecessary ref:-import { Locale, detectBrowserLocale } from '../i18n'; +import { Locale, detectBrowserLocale } from '../i18n'; -type LanguagePreference = Locale | 'browser'; - -const languagePreference = ref<LanguagePreference>('browser'); +type LanguagePreference = Locale | 'browser'; + +const languagePreference = ref<LanguagePreference>('browser');And for
browserLocaleand its usage:-const browserLocale = ref<string>(detectBrowserLocale()); +const browserLocale: Locale = detectBrowserLocale(); ... - locale.value = selectedLocale ?? browserLocale.value; + locale.value = selectedLocale ?? browserLocale;Functionally this is equivalent but gives you stronger type safety and simpler code.
96-107: Consider also applying stored user language to the active i18n localeIn
onMounted, whenme.value?.languageis present you setlanguagePreferencebut leavelocale.valueunchanged. If nothing else in the app overwriteslocale.valuefrom the stored user language, the effective UI language may remain at the browser‑detected locale despite a saved preference, until the user manually changes it in this listbox.If this component is intended to enforce the saved preference, you could (optionally) also sync the global locale here, and optionally normalize legacy values via
mapToLocale:-import { Locale, detectBrowserLocale } from '../i18n'; +import { Locale, detectBrowserLocale, mapToLocale } from '../i18n'; ... -const browserLocale = ref<string>(detectBrowserLocale()); +const browserLocale: Locale = detectBrowserLocale(); ... onMounted(async () => { const cfg = config.get(); keycloakUserAccountURL.value = `${cfg.keycloakUrl}/realms/${cfg.keycloakRealm}/account`; await fetchData(); - if (me.value?.language) { - languagePreference.value = me.value.language as Locale; - } else { - languagePreference.value = 'browser'; - } + if (me.value?.language) { + const persistedLocale = mapToLocale(me.value.language); + languagePreference.value = persistedLocale; + locale.value = persistedLocale; + } else { + languagePreference.value = 'browser'; + locale.value = browserLocale; + } });This would ensure the stored preference is both reflected in the dropdown and actually applied to the app’s language. If some other global bootstrap code already handles this, then the current code is fine, but it’s worth double‑checking to avoid subtle preference inconsistencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/UserProfile.vue(4 hunks)frontend/src/i18n/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 282
File: frontend/src/common/userdata.ts:82-88
Timestamp: 2024-07-27T13:14:40.963Z
Learning: The error handling suggestion for `createBrowserKeys` in `frontend/src/common/userdata.ts` was discussed and resolved in PR #282, and should not be repeated in future reviews.
🔇 Additional comments (3)
frontend/src/i18n/index.ts (1)
145-154: Using detectBrowserLocale() for initial i18n locale is consistent with new mappingInitializing
createI18nwithlocale: detectBrowserLocale()nicely centralizes locale detection and ensures the same normalization logic is reused everywhere. The additional options (globalInjection,missingWarn: false,fallbackWarn: false,legacy: false) are compatible with composition‑API usage viauseI18n({ useScope: 'global' })elsewhere.Just make sure any code that restores a user‑saved language (e.g., from
userdata.me) still overrides this initial browser‑derived locale as intended, so existing preferences aren’t silently ignored.frontend/src/components/UserProfile.vue (2)
25-47: LanguagePreference binding and explicit 'browser' sentinel are coherentSwitching the listbox
v-modeltolanguagePreferenceand using a stablevalue="browser"for the browser‑language option makes the model much clearer than encoding the browser locale string itself. The click handlers (saveLanguage(undefined)for browser,saveLanguage(availableLocale)for concrete locales) line up with this model and correctly distinguish between “follow browser” and “force specific locale”.This all looks consistent with the new
LanguagePreferencetype and should yield predictable behavior in the UI.
120-128: saveLanguage flow correctly distinguishes stored vs. browser‑derived languageThe
saveLanguageimplementation cleanly applies the chosen locale tolocale.valueand persists only explicit selections to the backend (clearingupdatedUser.languagewhen the “browser” option is chosen viaselectedLocalebeingundefined). Given the top‑levelv-if="me == null"guard in the template, users can’t trigger this beforemeis loaded, so theupdatedUser !== undefinedcheck is an adequate safety net.No issues from a correctness perspective; this aligns well with the new
LanguagePreferencemodel.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/i18n/index.ts (1)
105-105: Minor typo:local→localeThe parameter name appears to be a typo.
-export const mapToLocale = (local: string): Locale => { +export const mapToLocale = (locale: string): Locale => {Then update references inside the function accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/i18n/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: overheadhunter
Repo: cryptomator/hub PR: 282
File: frontend/src/common/userdata.ts:82-88
Timestamp: 2024-07-27T13:14:40.963Z
Learning: The error handling suggestion for `createBrowserKeys` in `frontend/src/common/userdata.ts` was discussed and resolved in PR #282, and should not be repeated in future reviews.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (3)
frontend/src/i18n/index.ts (3)
135-139: LGTM!Clean helper that centralizes browser locale detection. The separation between fetching the raw browser value and mapping it to a supported Locale is well-structured.
141-151: LGTM!Robust fallback chain for browser locale detection. Correctly handles SSR environments where
navigatoris undefined, prefers the more reliablenavigator.languages[0], and provides sensible defaults.
153-163: LGTM!Good integration of
detectBrowserLocale()for initial locale. The added options are appropriate for vue-i18n v9+ with Vue 3 Composition API.Note that
missingWarn: falseandfallbackWarn: falsewill suppress console warnings for missing translations. This reduces noise but could hide issues during development. If debugging translation coverage becomes necessary, consider enabling these in development builds only.
mindmonk
changed the title
This PR improves how the application detects and handles browser languages. The locale normalization logic in i18n/index.ts has been expanded so that short or inconsistent browser locale values (like de from Firefox) are now mapped reliably to full locale codes such as de-DE. A new detectBrowserLocale() helper ensures that the initial i18n locale is always a valid enum value.
In UserProfile.vue, the language selection has been cleaned up by introducing a dedicated languagePreference model and by using the centralized locale detection instead of raw browser values. The browser-language option is now handled explicitly via "browser", and the correct preference is restored when user data is loaded.
Overall, this fixes incorrect language behavior across different browsers and makes the language selection more consistent and predictable.