feat(desktop): add minimal i18n foundation for desktop UI strings#7380
feat(desktop): add minimal i18n foundation for desktop UI strings#7380bavadim wants to merge 3 commits intoblock:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48b63f36f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ui/desktop/src/i18n/index.ts
Outdated
| export function currentLocaleTag(): string { | ||
| const raw = getConfiguredLocaleRaw(); | ||
| if (raw) { | ||
| return normalizeLocaleTag(raw); |
There was a problem hiding this comment.
When GOOSE_LOCALE is set to a common POSIX-style value like en_US.UTF-8, currentLocaleTag() returns en-US.UTF-8, which is not a valid BCP-47 locale tag; formatMessageTimestamp() then passes this value to toLocaleTimeString/toLocaleDateString, which throws RangeError and can break session/chat rendering wherever timestamps are shown. This only appears when users or distros provide a locale with encoding suffixes, but that is a realistic configuration path given the new env-based locale support.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in fix(desktop): validate locale tag before timestamp formatting.
Changes:
- Strip POSIX locale suffixes (like
.UTF-8and@modifier) before using locale tags - Validate/canonicalize locale tags via
Intl.getCanonicalLocales - Fallback to
en-USwhen locale is invalid - Added tests for
en_US.UTF-8normalization and invalid tag fallback
This prevents RangeError in toLocaleTimeString/toLocaleDateString paths.
bavadim
force-pushed
the
feat/desktop-i18n-support
branch
from
d6e8edf to
aead6aa
Compare
- add lightweight i18n runtime and English catalog\n- wire key desktop UI strings via translation keys with fallbacks\n- validate/canonicalize locale tag for timestamp formatting\n- document desktop localization path for custom distros Signed-off-by: Vadim Polulyakh <[email protected]>
Signed-off-by: Vadim Polulyakh <[email protected]>
Signed-off-by: Vadim Polulyakh <[email protected]>
bavadim
force-pushed
the
feat/desktop-i18n-support
branch
from
4301fed to
5cd3820
Compare
| @@ -81,7 +93,10 @@ export function GroupedExtensionLoadingToast({ | |||
| <div className="font-medium text-base">{getSummaryText()}</div> | |||
| {errorCount > 0 && ( | |||
| <div className="text-sm opacity-90"> | |||
| {errorCount} extension{errorCount !== 1 ? 's' : ''} failed to load | |||
| {t('extensions.failed_count', '{count} extension{suffix} failed to load', { | |||
There was a problem hiding this comment.
It's not clear to me how this framework will support pluralisation in most languages, where a simple suffix is not enough. I might have missed something though. To give an example, in French this string would have one of these forms:
Singular: 1 extension n'a pas pu être chargée
Plural: 10 extensions n'ont pas pu être chargées
How would a hypothetical i18n/fr.json implement this translation?
There was a problem hiding this comment.
Due to this issue I decided to take a stab at a different approach using ICU MessageFormat for the translation strings (which is flexible enough to support the wide variety of different plural/gender/etc complexities that different languages have, and also has the advantage of being a standard with a lot of tooling support)
That PR is in #8105 -- I'll close this one. Would welcome any review/discussion/help over there
| @@ -37,7 +38,7 @@ export const SearchBar: React.FC<SearchBarProps> = ({ | |||
| searchResults, | |||
| inputRef: externalInputRef, | |||
| initialSearchTerm = '', | |||
| placeholder = 'Search conversation...', | |||
| placeholder = t('common.search_conversation', 'Search conversation...'), | |||
There was a problem hiding this comment.
This gets called at module load time; are we sure locale config is always already available then? It might be safer to move the t() call into the component body
| {getSearchShortcutText()} to search. | ||
| <p className="text-sm text-text-muted mb-2"> | ||
| {t( | ||
| 'extensions.description', |
There was a problem hiding this comment.
This key doesn't exist in i18n/en.json -- could we add a lint that all keys used in t() calls in code actually exist in the base translation file? If we have that, maybe we don't need to duplicate the default text in both the t() call and the translation file.
2 participants
Summary
This PR adds a minimal, low-maintenance i18n foundation for Goose Desktop without introducing new maintained locales in upstream.
What is included:
ui/desktop/src/i18n/index.tsui/desktop/src/i18n/en.jsonas the canonical defaultt('key', 'fallback')across key desktop surfacesGOOSE_LOCALE(currentLocaleTag())ui/desktop/src/i18n/i18n.test.tsCUSTOM_DISTROS.mdfor localization in custom distrosWhat is intentionally NOT included:
Why this should be accepted now:
Type of Change
AI Assistance
Testing
cd ui/desktop && npm run test:run -- src/i18n/i18n.test.tscd ui/desktop && npm run typecheckRelated Issues
Relates to #2376
Discussion: #2376
Discussion: #4230
Screenshots/Demos (for UX changes)
Before:
UI strings were hardcoded in many desktop components.
After:
UI strings are routed through i18n keys with English fallback, and locale tag is respected for date/time formatting.