feat(i18n): finish internationalization setup #6526
Conversation
WalkthroughReplaces env-var driven Lingui flag with a feature-flag-driven internationalization flow: adds a hook to read Changes
Sequence DiagramsequenceDiagram
participant App as App Init
participant FF as Feature Flags Atom
participant Hook as useIsInternationalizationEnabled
participant Prov as LanguageProvider / Provider
participant i18n as dynamicActivate
App->>FF: Read feature flags state
FF-->>Hook: Return isInternationalizationEnabled
Hook-->>Prov: Provide flag value
Prov->>i18n: Call dynamicActivate(locale, isInternationalizationEnabled)
alt isInternationalizationEnabled = true
i18n->>i18n: Load locale-specific catalog
else isInternationalizationEnabled = false
i18n->>i18n: Load default locale catalog (no persistence)
end
i18n-->>Prov: Catalog loaded (or fallback)
Prov->>App: Render with/without localized resources
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (5)
apps/cowswap-frontend/src/lib/README-LOCALIZATION.md (1)
7-9: Add language identifier to code block.The fenced code block should specify a language identifier for better rendering and syntax highlighting.
As per static analysis hints.
Apply this diff:
-``` -isInternationalizationEnabled -``` +```typescript +isInternationalizationEnabled +```apps/cowswap-frontend/src/i18n.tsx (2)
18-24: Fix lang attribute and avoid wiping user preference when i18n is disabledWhen the feature flag is off, you activate DEFAULT_LOCALE but still set document.lang to the selected locale and overwrite the saved user locale. Set and persist the effective locale instead, and skip persisting when disabled.
const onActivate = useCallback( (locale: SupportedLocale) => { - document.documentElement.setAttribute('lang', locale) - setLocale(isInternationalizationEnabled ? locale : DEFAULT_LOCALE) // stores the selected locale to persist across sessions + const effectiveLocale = isInternationalizationEnabled ? locale : DEFAULT_LOCALE + document.documentElement.setAttribute('lang', effectiveLocale) + if (isInternationalizationEnabled) { + // Persist only when i18n is enabled to avoid overwriting the user's preference + setLocale(effectiveLocale) + } }, - [setLocale, isInternationalizationEnabled], + [setLocale, isInternationalizationEnabled], )
11-11: Reconsider eager activation to reduce flicker and redundant workThis runs at module load and immediately activates DEFAULT_LOCALE regardless of user preference. Either remove it and rely on Provider’s effect, or make intent explicit by activating DEFAULT_LOCALE directly.
Option A — remove:
-void dynamicActivate(initialLocale, false)Option B — make intent explicit (and drop the now-unused initialLocale import):
-void dynamicActivate(initialLocale, false) +void dynamicActivate(DEFAULT_LOCALE, false)apps/cowswap-frontend/src/lib/i18n.tsx (2)
10-30: Fallback to DEFAULT_LOCALE on load failureIf a locale fails to load, only logging degrades UX. Fallback to DEFAULT_LOCALE to ensure consistent rendering.
export async function dynamicActivate(locale: SupportedLocale, isInternationalizationEnabled?: boolean): Promise<void> { try { // Load default (en-US) catalog if internationalization is disabled if (!isInternationalizationEnabled) { const defaultCatalog = await import(`../locales/${DEFAULT_LOCALE}.po`) i18n.load(DEFAULT_LOCALE, defaultCatalog.messages || defaultCatalog.default.messages) i18n.activate(DEFAULT_LOCALE) return } const catalog = await import(`../locales/${locale}.po`) i18n.load(locale, catalog.messages || catalog.default.messages) i18n.activate(locale) } catch (error) { - // Do nothing - console.error('Could not load locale file: ' + locale, error) + console.error('Could not load locale file: ' + locale, error) + try { + const defaultCatalog = await import(`../locales/${DEFAULT_LOCALE}.po`) + i18n.load(DEFAULT_LOCALE, defaultCatalog.messages || defaultCatalog.default.messages) + i18n.activate(DEFAULT_LOCALE) + } catch (fallbackError) { + console.error('Could not load default locale catalog', fallbackError) + } } }
38-48: Pass effective locale to onActivateonActivate currently receives the requested locale even when DEFAULT_LOCALE is activated (flag off). Send the effective locale to keep document.lang and persistence in sync.
export function Provider({ locale, onActivate, children }: ProviderProps): ReactNode { const isInternationalizationEnabled = useIsInternationalizationEnabled() useEffect(() => { - dynamicActivate(locale, isInternationalizationEnabled) - .then(() => onActivate?.(locale)) + const effectiveLocale = isInternationalizationEnabled ? locale : DEFAULT_LOCALE + dynamicActivate(locale, isInternationalizationEnabled) + .then(() => onActivate?.(effectiveLocale)) .catch((error) => { console.error('Failed to activate locale', locale, error) }) - }, [locale, onActivate, isInternationalizationEnabled]) + }, [locale, onActivate, isInternationalizationEnabled])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/cowswap-frontend/.env(1 hunks)apps/cowswap-frontend/src/common/hooks/featureFlags/useIsInternationalizationEnabled.ts(1 hunks)apps/cowswap-frontend/src/i18n.tsx(1 hunks)apps/cowswap-frontend/src/legacy/components/Header/AccountElement/NotificationAlertPopover.tsx(2 hunks)apps/cowswap-frontend/src/lib/README-LOCALIZATION.md(2 hunks)apps/cowswap-frontend/src/lib/i18n.tsx(2 hunks)apps/cowswap-frontend/src/locales/en-US.po(9 hunks)apps/cowswap-frontend/src/modules/application/containers/AppMenu/index.tsx(3 hunks)apps/cowswap-frontend/src/modules/notifications/pure/NotificationSettingsPopover.tsx(3 hunks)crowdin.yml(1 hunks)libs/common-utils/src/i18n.ts(0 hunks)libs/ui/src/pure/MenuBar/index.tsx(8 hunks)
💤 Files with no reviewable changes (1)
- libs/common-utils/src/i18n.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-02-20T15:59:33.749Z
Learnt from: shoom3301
Repo: cowprotocol/cowswap PR: 5443
File: apps/cowswap-frontend/src/modules/swap/containers/ConfirmSwapModalSetup/index.tsx:71-71
Timestamp: 2025-02-20T15:59:33.749Z
Learning: The swap module in apps/cowswap-frontend/src/modules/swap/ is marked for deletion in PR #5444 as part of the swap widget unification effort.
Applied to files:
apps/cowswap-frontend/.env
📚 Learning: 2025-07-24T16:42:53.154Z
Learnt from: cowdan
Repo: cowprotocol/cowswap PR: 6009
File: apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/HighFeeWarningTooltipContent.tsx:23-33
Timestamp: 2025-07-24T16:42:53.154Z
Learning: In apps/cowswap-frontend/src/modules/tradeWidgetAddons/containers/HighFeeWarning/HighFeeWarningTooltipContent.tsx, the use of toFixed(2) for percentage formatting in tooltip content is intentional and differs from the banner message formatting that uses toSignificant(2, undefined, Rounding.ROUND_DOWN). This formatting difference serves different UX purposes and should not be flagged as inconsistent.
Applied to files:
apps/cowswap-frontend/src/locales/en-US.poapps/cowswap-frontend/src/legacy/components/Header/AccountElement/NotificationAlertPopover.tsxapps/cowswap-frontend/src/modules/notifications/pure/NotificationSettingsPopover.tsx
📚 Learning: 2025-10-10T20:28:16.565Z
Learnt from: fairlighteth
Repo: cowprotocol/cowswap PR: 6347
File: apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.tsx:49-49
Timestamp: 2025-10-10T20:28:16.565Z
Learning: In apps/cowswap-frontend/src/modules/trade, TradeConfirmation follows a two-layer architecture: TradeConfirmationView (pure/stateless) in pure/TradeConfirmation/index.tsx renders the UI, while TradeConfirmation (container) in containers/TradeConfirmation/index.tsx wraps it to freeze props during pending trades (via useStableTradeConfirmationProps), wire in signing state (useSigningStep), and inject trade confirmation state (useTradeConfirmState). Consuming modules should import the container TradeConfirmation from 'modules/trade' to preserve this stateful behavior.
<!-- [add_learning]
When reviewing component refactoring in apps/cowswap-frontend/src/modules/trade, recognize the pattern where a pure view component (e.g., TradeConfirmationView) is separated from a stateful container (e.g., TradeConfirmation) that wraps it. The container adds runtime state management (prop stabilization, signing state, etc.) while the view remains testable and composable. Do not flag usages that import th...
Applied to files:
apps/cowswap-frontend/src/legacy/components/Header/AccountElement/NotificationAlertPopover.tsxapps/cowswap-frontend/src/modules/notifications/pure/NotificationSettingsPopover.tsxapps/cowswap-frontend/src/modules/application/containers/AppMenu/index.tsx
🪛 markdownlint-cli2 (0.18.1)
apps/cowswap-frontend/src/lib/README-LOCALIZATION.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (2)
- GitHub Check: Cypress
- GitHub Check: Setup
🔇 Additional comments (27)
crowdin.yml (1)
1-1: LGTM! Crowdin project ID updated.The project_id change aligns with the new Crowdin project setup mentioned in the PR description.
apps/cowswap-frontend/.env (1)
143-145: LGTM! Environment configuration updated for Crowdin.The change correctly removes the Lingui-specific environment flag and adds the Crowdin token placeholder, aligning with the feature flag approach.
apps/cowswap-frontend/src/modules/notifications/pure/NotificationSettingsPopover.tsx (2)
6-7: LGTM! Lingui imports added correctly.The imports for translation macros are properly added.
129-149: LGTM! UI text properly internationalized.All user-facing text is correctly wrapped with translation helpers:
tmacro for the aria-labelTranscomponent for JSX contentapps/cowswap-frontend/src/common/hooks/featureFlags/useIsInternationalizationEnabled.ts (1)
1-9: LGTM! Feature flag hook correctly implemented.The hook properly reads the internationalization flag from the feature flags atom and returns a boolean value. The implementation is clean and follows React hooks best practices.
apps/cowswap-frontend/src/legacy/components/Header/AccountElement/NotificationAlertPopover.tsx (2)
7-8: LGTM! Lingui imports added correctly.Translation macros imported as expected.
153-167: LGTM! Alert popover text properly internationalized.All user-facing text in the notification alert popover is correctly wrapped with translation helpers, consistent with the internationalization approach used in other components.
apps/cowswap-frontend/src/lib/README-LOCALIZATION.md (1)
5-9: LGTM! Documentation updated to reflect feature flag approach.The documentation correctly describes that the feature flag controls both locale loading and the languages dropdown, replacing the previous environment variable approach.
libs/ui/src/pure/MenuBar/index.tsx (4)
20-20: LGTM! Removed obsolete import.The import of
isLinguiInternationalizationEnabledis correctly removed as the logic now uses a prop-based approach.
728-728: LGTM! Props correctly typed.The
isInternationalizationEnabledprop is properly added to bothGlobalSettingsDropdownPropsandMenuBarPropsas optional booleans, maintaining backward compatibility.Also applies to: 872-872
799-801: LGTM! Internationalization flag correctly applied.The conditional rendering of language items now correctly uses the
isInternationalizationEnabledprop instead of the previous function-based check, aligning with the feature flag approach.
1060-1060: LGTM! Props properly propagated.The
isInternationalizationEnabledprop is correctly passed toGlobalSettingsDropdownin both mobile and desktop rendering paths.Also applies to: 1075-1075
apps/cowswap-frontend/src/modules/application/containers/AppMenu/index.tsx (3)
18-18: LGTM! Feature flag hook imported.The import statement correctly references the new hook for reading the internationalization feature flag.
48-48: LGTM! Feature flag read correctly.The hook is called at the appropriate component level to obtain the internationalization flag state.
115-115: LGTM! Feature flag propagated to MenuBar.The
isInternationalizationEnabledvalue is correctly passed to the MenuBar component, completing the feature flag propagation chain throughout the UI hierarchy.apps/cowswap-frontend/src/i18n.tsx (2)
13-13: Explicit return type for LanguageProvider looks goodClearer typing and aligns with usage.
8-8: Hook import integration LGTMFeature flag is localized to the provider scope as intended.
apps/cowswap-frontend/src/locales/en-US.po (9)
911-914: LGTM: “Got it” entryString and mapping look correct.
1707-1709: LGTM: “Enable order alerts”Clear and consistent with context.
2137-2140: LGTM: “Enable trade alerts”Copy matches UI intent.
2649-2652: LGTM: “Not now”Standard dismiss action; fine.
2711-2714: LGTM: “Trade alerts enabled”Status message reads well.
3421-3423: LGTM: “Trade alert settings info”OK as a caption/help label.
3435-3437: LGTM: “Change trade alert settings here”Action hint is clear.
3560-3562: LGTM: “When orders fill or expire”Concise descriptor; fine.
5023-5026: LGTM: “Get trade alerts”CTA phrasing looks good.
apps/cowswap-frontend/src/lib/i18n.tsx (1)
8-8: Hook usage LGTMFlag sourcing inside core Provider is appropriate; dependencies updated accordingly.
|
I'll optimistically merge to test out the translation from Crowdin. |
|
Checked on Develop, works as expected! |
Summary
REACT_APP_LINGUI_INTERNATIONALIZATIONwith feature flagisInternationalizationEnabled.Added also 2 more translations which were missed, but it was great to test the translation flow.
They will be available in Crowdin once this PR is merged to
develop.To Test
isInternationalizationEnabledonSummary by CodeRabbit
New Features
Chores