feat: Localization Selector logic #3175
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR introduces an internationalization selector feature with a new I18nSelector component for language and currency selection. It includes a new Changes
Sequence DiagramsequenceDiagram
participant User
participant I18nButton
participant I18nSelector
participant useBindingSelector
participant BindingResolver
participant Navigation
User->>I18nButton: Click i18n button
I18nButton->>I18nSelector: Open selector (Desktop: Popover / Mobile: SlideOver)
I18nSelector->>useBindingSelector: Read current locale/currency
useBindingSelector-->>I18nSelector: Return languages, currencies, current selections
I18nSelector->>User: Display language/currency options
User->>I18nSelector: Select new locale
I18nSelector->>useBindingSelector: setLocaleCode(newLocale)
useBindingSelector->>useBindingSelector: Filter currencies for new locale
useBindingSelector-->>I18nSelector: Update available currencies
User->>I18nSelector: Select currency
I18nSelector->>useBindingSelector: setCurrencyCode(newCurrency)
useBindingSelector->>useBindingSelector: Validate selection, compute canSave
useBindingSelector-->>I18nSelector: Enable save button
User->>I18nSelector: Click save
I18nSelector->>useBindingSelector: save()
useBindingSelector->>BindingResolver: resolveBinding(currencyCode)
BindingResolver->>BindingResolver: Validate URL from binding
BindingResolver-->>useBindingSelector: Binding with valid URL
useBindingSelector->>Navigation: Navigate to binding URL
Navigation-->>User: Page reloads with new locale/currency
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
lariciamota
force-pushed
the
feat/logic-i18n-selector
branch
from
631dd81 to
fd89b72
Compare
lariciamota
requested review from
gabpaladino,
hellofanny,
lemagnetic and
lucasfp13
and removed request for
a team,
gabpaladino and
lucasfp13
lariciamota
changed the title
lariciamota
force-pushed
the
feat/logic-i18n-selector
branch
from
8bf54f0 to
fd89b72
Compare
|
Renaming PR (from I18nSelector to LocalizationSelector) |
|
@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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/components/src/molecules/Popover/Popover.tsx (1)
151-151: Conditional hook call violates Rules of Hooks.
useRefis called conditionally based on the value ofref. React hooks must be called unconditionally and in the same order on every render. Ifrefchanges fromnull/undefinedto a valid ref (or vice versa) between renders, this will cause inconsistent hook ordering.Proposed fix
- // Use forwarded ref or internal ref for fallback - const popoverRef = ref || useRef<HTMLDivElement>(null) + // Internal ref for fallback when no forwarded ref provided + const internalRef = useRef<HTMLDivElement>(null) + const popoverRef = ref || internalRef
🤖 Fix all issues with AI agents
In `@packages/core/src/components/i18n/I18nSelector/I18nSelector.tsx`:
- Around line 196-199: The selector currently calls onClose() unconditionally in
handleSave which hides any save errors; change handleSave to only close on a
confirmed successful save: have onSave return a success indicator (e.g., boolean
or resolved promise) or propagate errors, then in handleSave await onSave(),
check the returned value or catch errors and set the component's error state
(the same error state used by the error display logic) and only call onClose()
when the save succeeded; if onSave throws, catch and set the error state and do
not call onClose.
🧹 Nitpick comments (7)
packages/core/src/sdk/i18n/bindingSelector.ts (1)
75-83: Consider validating URL scheme to prevent unsafe protocols.The
URLconstructor accepts URLs with potentially dangerous schemes likejavascript:. Since this is used for redirect validation, consider restricting to safe protocols.Optional: Restrict to http/https schemes
export function isValidUrl(url: string): boolean { if (!url || url.trim() === '') return false try { - new URL(url) - return true + const parsed = new URL(url) + return ['http:', 'https:'].includes(parsed.protocol) } catch { return false } }packages/core/test/sdk/i18n/bindingSelector.test.ts (1)
38-46: Edge case:makeLocaleassumes simplelanguage-regionformat.The split on
-may not work correctly for locale codes with scripts (e.g.,zh-Hans-CN) or extended formats. Since test data currently uses simple codes, this isn't blocking, but worth noting if test coverage expands.🔧 Suggested improvement for robustness
function makeLocale(code: string, languageName: string): Locale { - const [languageCode, regionCode] = code.split('-') + const parts = code.split('-') + const languageCode = parts[0] + const regionCode = parts[parts.length - 1] // Last segment is typically region return createLocale({ code, languageName, languageCode, regionCode, }) }packages/core/src/sdk/i18n/useBindingSelector.ts (2)
13-34: Consider extracting shared types totypes.ts.
Currency,Region, andLocalizationConfigare defined locally but relate to the broader i18n type system. If these types are used elsewhere or represent thediscovery.configschema, consider exporting them fromtypes.tsfor consistency.
131-162: Consider providing user feedback before redirect.The
save()function redirects immediately on success viawindow.location.href. If the redirect fails or is slow, users have no indication the save was processed. Consider returning a promise or adding a loading state for better UX feedback.packages/core/test/sdk/i18n/useBindingSelector.test.tsx (1)
1-9: File tests utilities, not the React hook - consider renaming or adding hook tests.The file is named
useBindingSelector.test.tsx(suggesting React component/hook tests) but only imports and tests utility functions frombindingSelector.ts. There are no React Testing Library imports orrenderHookcalls to test the actual hook behavior.Consider either:
- Renaming to
bindingSelectorIntegration.test.ts(since it's integration scenarios for utilities)- Adding actual hook tests using
@testing-library/reactto test state management, session integration, and side effects#!/bin/bash # Check if there are actual hook tests elsewhere or if renderHook is used in the codebase rg -n "renderHook|@testing-library/react" packages/core/test/packages/ui/src/components/organisms/I18nSelector/styles.scss (1)
33-36: Consider usingflex-endfor broader browser compatibility.While
justify-content: endis valid CSS, usingflex-endprovides better compatibility with older browsers if that's a concern for this project.🔧 Suggested change
[data-fs-i18n-selector-actions] { display: flex; - justify-content: end; + justify-content: flex-end; }packages/core/src/components/sections/Navbar/Navbar.tsx (1)
76-82: Makei18nSelectoroptional to align with schema/default usage.
sections.jsondoesn’t require this field and you already default to{}during destructuring, so keeping it optional avoids forcing callers to provide it.♻️ Suggested tweak
- i18nSelector: { + i18nSelector?: {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
packages/components/src/molecules/Popover/Popover.tsxpackages/components/src/organisms/SlideOver/SlideOver.tsxpackages/core/cms/faststore/sections.jsonpackages/core/src/components/i18n/I18nSelector/I18nSelector.tsxpackages/core/src/components/i18n/I18nSelector/index.tspackages/core/src/components/i18n/I18nSelector/section.module.scsspackages/core/src/components/i18n/index.tspackages/core/src/components/navigation/Navbar/Navbar.tsxpackages/core/src/components/navigation/NavbarSlider/NavbarSlider.tsxpackages/core/src/components/sections/Navbar/Navbar.tsxpackages/core/src/components/ui/I18nButton/I18nButton.tsxpackages/core/src/sdk/i18n/bindingSelector.tspackages/core/src/sdk/i18n/index.tspackages/core/src/sdk/i18n/types.tspackages/core/src/sdk/i18n/useBindingSelector.tspackages/core/test/sdk/i18n/bindingSelector.test.tspackages/core/test/sdk/i18n/useBindingSelector.test.tsxpackages/ui/src/components/organisms/I18nSelector/styles.scsspackages/ui/src/components/organisms/SlideOver/styles.scsspackages/ui/src/styles/components.scss
🧰 Additional context used
🧬 Code graph analysis (6)
packages/core/src/components/navigation/NavbarSlider/NavbarSlider.tsx (2)
packages/core/src/components/navigation/Navbar/Navbar.tsx (1)
NavbarProps(39-95)packages/core/src/components/sections/Navbar/Navbar.tsx (1)
NavbarProps(14-94)
packages/components/src/organisms/SlideOver/SlideOver.tsx (1)
packages/components/src/organisms/SlideOver/index.ts (1)
Direction(3-3)
packages/core/test/sdk/i18n/bindingSelector.test.ts (2)
packages/core/src/sdk/i18n/types.ts (2)
Locale(8-17)Binding(1-6)packages/core/src/sdk/i18n/bindingSelector.ts (4)
buildLanguageOptions(10-30)getCurrenciesForLocale(38-46)resolveBinding(56-67)isValidUrl(75-83)
packages/core/test/sdk/i18n/useBindingSelector.test.tsx (2)
packages/core/src/sdk/i18n/types.ts (1)
Locale(8-17)packages/core/src/sdk/i18n/bindingSelector.ts (4)
buildLanguageOptions(10-30)getCurrenciesForLocale(38-46)resolveBinding(56-67)isValidUrl(75-83)
packages/core/src/components/ui/I18nButton/I18nButton.tsx (2)
packages/core/src/sdk/i18n/index.ts (1)
useBindingSelector(2-2)packages/core/src/sdk/i18n/useBindingSelector.ts (1)
useBindingSelector(66-177)
packages/core/src/components/i18n/I18nSelector/I18nSelector.tsx (3)
packages/core/src/sdk/i18n/index.ts (1)
BindingSelectorError(1-1)packages/core/src/sdk/i18n/types.ts (1)
BindingSelectorError(19-22)packages/components/src/index.ts (1)
SlideOverHeader(385-385)
🔇 Additional comments (43)
packages/components/src/organisms/SlideOver/SlideOver.tsx (1)
7-7: LGTM!The
Directiontype expansion to include'bottomSide'is additive and backward compatible. The component correctly propagates this to thedata-fs-slide-over-directionattribute, and the corresponding CSS styles are added instyles.scssto handle the new direction.packages/core/src/sdk/i18n/types.ts (1)
1-22: LGTM!The type definitions are well-structured:
Bindingcaptures binding configuration cleanlyLocalehas comprehensive locale metadata with appropriatetextDirectionconstraintBindingSelectorErroruses a discriminated union pattern, making error handling type-safe and exhaustivepackages/ui/src/components/organisms/SlideOver/styles.scss (1)
92-111: LGTM!The
bottomSidevariant implementation is correct:
- Positioning anchors the panel to the bottom with full width
translateY(100%)/translateY(0)transitions provide the slide-up/down effect- Max-height constraints for
full(100vh) andpartial(80vh) sizes are appropriateNote: This block uses physical properties (
top,right,bottom,left) whileleftSide/rightSideuse logical properties (inset-inline-start/end). This is acceptable since vertical positioning is direction-agnostic in RTL layouts.packages/components/src/molecules/Popover/Popover.tsx (2)
96-127: LGTM!The position calculation correctly differentiates between portal and non-portal modes:
- Portal mode uses raw viewport coordinates (fixed positioning)
- Non-portal mode adds scroll offsets (absolute positioning)
233-240: LGTM!The conditional rendering approach is clean—portal mode wraps the element and renders via
createPortal, while inline mode returns the element directly.packages/core/src/sdk/i18n/bindingSelector.ts (2)
10-30: LGTM!The disambiguation logic correctly identifies duplicate language names and appends the region code only when necessary. This produces clean labels like "Portuguese (BR)" vs "Portuguese (PT)" while keeping unique languages simple.
56-67: LGTM!The binding resolution logic handles all cases correctly:
- No matches →
null- Single match → return it
- Multiple matches → prefer
isDefault, fallback to firstThe nullish coalescing (
?? matches[0]) ensures a deterministic result even if no binding hasisDefault: true.packages/core/test/sdk/i18n/bindingSelector.test.ts (5)
1-46: LGTM! Well-structured test helpers.The helper functions provide good reusability and sensible defaults for building test fixtures.
48-93: LGTM! Good coverage for language options disambiguation.Tests cover unique names, duplicated names requiring disambiguation, mixed scenarios, and empty input.
95-135: LGTM! Data-driven tests with good edge case coverage.Using
testCases.forEachpattern keeps tests DRY while covering deduplication, single bindings, empty arrays, and insertion order preservation.
137-220: LGTM! Thorough tests for binding resolution logic.Tests cover: no match, single match,
isDefaulttie-breaker, fallback to first match, empty array, and multiple currencies with defaults. The factory pattern for mock bindings is clean.
222-246: LGTM! URL validation tests cover key scenarios.Testing valid URLs (with paths, query params, localhost), empty/whitespace, invalid formats, and null-like values provides solid coverage.
packages/core/src/sdk/i18n/useBindingSelector.ts (5)
1-11: LGTM! Clean imports with appropriate type-only imports.
36-58: LGTM! Well-documented return type interface.JSDoc comments clearly describe each property and action. The interface provides a clean contract for consumers.
96-122: LGTM! Locale change handler with proper currency adjustment logic.The logic correctly:
- Clears errors on selection
- Checks if current currency is available in new locale
- Auto-selects if only one currency exists
- Clears selection otherwise
164-164: Potential staleerrorincanSavecomputation.
canSaveis computed inline but depends onerrorstate. IfsetLocaleCodeorsetCurrencyCodeis called and clears the error, the component will re-render with updatedcanSave. This is correct, but ensure the UI properly reflects the derived state on each render.
70-77: No syncing with session updates after mount—clarify if intentional.The
useStateinitializers capture session values only once at mount via lazy initialization. IfcurrentLocaleorcurrentCurrencychange in the session afterward, the hook state won't update. The code structure suggests this is intentional (user selections in the selector override session defaults), but there's nouseEffector documentation confirming this design. Verify whether:
- Session updates during the selector lifetime should propagate to UI state, or
- The current behavior (selections override and persist until save/redirect) is the intended pattern.
packages/core/test/sdk/i18n/useBindingSelector.test.tsx (6)
11-93: LGTM! Comprehensive mock data covering diverse scenarios.The
mockLocalesfixture includes:
- Languages with disambiguation needs (pt-BR, pt-PT)
- Single and multiple currency bindings
- Multiple bindings for same currency with
isDefaulttie-breaker (fr-FR)
95-119: LGTM! Language options tests validate disambiguation and output format.
121-139: LGTM! Currency filtering tests cover expected scenarios.
141-169: LGTM! Binding resolution tests verify tie-breaker logic.
171-180: LGTM! URL validation ensures all mock bindings are valid.
182-218: LGTM! Flow simulations demonstrate realistic user journeys.These end-to-end scenarios provide good documentation of expected behavior through tests.
packages/ui/src/styles/components.scss (1)
97-97: LGTM! Import follows existing conventions.The I18nSelector styles import is correctly placed in the Organisms section, maintaining alphabetical proximity with other components.
packages/core/src/sdk/i18n/index.ts (1)
1-4: LGTM! Clean barrel exports with proper type-only syntax.The utility functions (
buildLanguageOptions,getCurrenciesForLocale,resolveBinding,isValidUrl) are kept as internal implementation details inbindingSelector.tsand not exported from the public API. This is correct—they're used only internally by theuseBindingSelectorhook and should not be directly imported elsewhere.packages/core/src/components/i18n/index.ts (1)
1-1: LGTM!Clean barrel export that properly exposes the I18nSelector component through the i18n module's public API.
packages/core/src/components/i18n/I18nSelector/index.ts (1)
1-1: LGTM!Standard re-export pattern for the component folder.
packages/core/src/components/i18n/I18nSelector/section.module.scss (1)
1-16: LGTM!Well-organized style composition separating desktop, mobile, and common styles. The scoped
@importpattern provides clear context for which styles apply to each variant.packages/ui/src/components/organisms/I18nSelector/styles.scss (2)
1-37: Well-structured base styles.Good use of CSS custom properties for theming and data attributes for component styling. The hidden indicators/headers (lines 27-31) provide clean I18nSelector presentation inside overlays.
39-67: Mobile variant styles look good.Properly extends base styles with mobile-specific overrides for full-width elements and adjusted padding/typography.
packages/core/src/components/i18n/I18nSelector/I18nSelector.tsx (6)
17-45: Well-documented props interface.Clear JSDoc comments explaining each prop's purpose and expected format.
47-109: Comprehensive props interface for I18nSelector.Good documentation coverage and alignment with the useBindingSelector hook's API as described in the PR objectives.
111-156: Clean presentational component.The I18nSelectorContent properly renders the form fields and conditionally shows the save button. Good use of the
disabledprop on the currency select when no currencies are available.
158-172: Good error message helper.Covers all
BindingSelectorErrortypes with user-friendly messages. The default case provides defensive handling for any future error types.
217-251: Desktop Popover variant is well-implemented.Good use of
enablePortalfor proper stacking context and appropriate null-coalescing for locale/currency codes.
253-291: Mobile SlideOver variant looks good.Clean implementation with proper fade transitions and footer button placement. The
direction="bottomSide"creates a bottom sheet UX appropriate for mobile.packages/core/src/components/sections/Navbar/Navbar.tsx (1)
114-120: Clean prop threading intoNavbar.
The selector labels are passed through consistently without impacting existing behavior.Also applies to: 142-148
packages/core/src/components/navigation/NavbarSlider/NavbarSlider.tsx (1)
18-26: Props and render wiring for the selector look consistent.
Good pass-through of configuration to theI18nButton.Also applies to: 33-34, 100-108
packages/core/cms/faststore/sections.json (1)
454-484: Schema addition fori18nSelectoris well-formed.
Defaults and labels read clearly.packages/core/src/components/navigation/Navbar/Navbar.tsx (2)
73-82:i18nSelectorprop surface is clear and optional.
106-107: Selector props are passed through cleanly to desktop and slider flows.Also applies to: 214-222, 257-257
packages/core/src/components/ui/I18nButton/I18nButton.tsx (2)
1-38: Hook integration and prop surface look solid.
Nice consolidation arounduseBindingSelector.
40-90: Selector rendering and state wiring are clear.
Good use of the trigger ref and callback pass-throughs.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @renatomaurovtex. * #3175 (comment) The following files were modified: * `packages/core/src/components/i18n/I18nSelector/I18nSelector.tsx` * `packages/core/src/components/navigation/Navbar/Navbar.tsx` * `packages/core/src/components/navigation/NavbarSlider/NavbarSlider.tsx` * `packages/core/src/components/sections/Navbar/Navbar.tsx` * `packages/core/src/sdk/i18n/bindingSelector.ts` * `packages/core/src/sdk/i18n/useBindingSelector.ts`
packages/core/src/components/i18n/I18nSelector/I18nSelector.tsx
Outdated
Show resolved
Hide resolved
|
I found a little be strange to update/display the selected Language in the Localization Button even before really setting the language (save and redirect) Screen.Recording.2026-01-19.at.23.29.22.mov |
lariciamota
force-pushed
the
feat/logic-i18n-selector
branch
2 times, most recently
from
13ff950 to
bf4ba8f
Compare
Totally! I've confirmed with Vanessinha and we should display the session locale/currency indeed. Fixed it! Thanks! |
lariciamota
force-pushed
the
feat/logic-i18n-selector
branch
from
4d934ef to
fd60a5a
Compare
What's the purpose of this pull request?
This PR implements the logic for the localization binding selector. Merchants need to support multiple language/currency combinations (bindings) with different sales channels. This feature provides the logic layer that enables users to switch between these bindings dynamically.
How it works?
Core Components
1. Hook:
useBindingSelectorlanguagesandcurrenciesasRecord<string, string>format ready for the selector UI component2. Utility:
buildLanguageOptions3. Utility:
getCurrenciesForLocale4. Utility:
resolveBindingisDefaultas a tie-breaker for deterministic selection5. Utility:
isValidUrlLogic Flow
How to test it?
Check that the options in the selector match the bindings saved on discovery.config
Examples using the
brandlessaccount configuration:Screen.Recording.2026-01-15.at.08.16.53.mov
Starters Deploy Preview
References
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.