Use new Spree Markets feature for proper multi region support - #31
Conversation
WalkthroughUpdated Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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. ✨ 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: 1
🧹 Nitpick comments (3)
src/contexts/StoreContext.tsx (3)
141-141: Dead fallback:resolved.localeis never empty.
resolveCountryAndCurrencyalways returns a non-emptylocalestring (falls back tostoreData.default_locale || "en"), so the|| initialLocalebranch is unreachable. This is harmless but misleading.♻️ Suggested cleanup
- setLocaleState(resolved.locale || initialLocale); + setLocaleState(resolved.locale);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/StoreContext.tsx` at line 141, The fallback is dead because resolveCountryAndCurrency always returns a non-empty locale; update the setter to rely directly on the resolved value instead of a redundant fallback: replace the expression using setLocaleState(resolved.locale || initialLocale) with a single-source assignment (setLocaleState(resolved.locale)) in the StoreContext where you call resolveCountryAndCurrency, removing the misleading initialLocale branch; if you prefer a defensive guard, use a nullish check (resolved.locale ?? initialLocale) instead of ||.
152-158:setCountryupdates currency but not locale — stale locale window betweenrouter.pushand effect re-run.When the user picks a new country in
CountrySwitcher,setCountryupdatescurrencybut leaveslocalestale untilinitialLocalechanges (i.e., after navigation completes and the layout re-renders). For most UIs this is invisible, but any component readinglocalefrom the context (e.g., for API calls on the current page before navigation) will see the old locale.Consider also updating
localeinsidesetCountry, deriving it from the matched country object:♻️ Proposed fix
const setCountry = (newCountry: string) => { setCountryState(newCountry); const countryObj = findCountry(countries, newCountry); if (countryObj?.currency) { setCurrency(countryObj.currency); } + if (countryObj?.default_locale) { + setLocaleState(countryObj.default_locale); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/StoreContext.tsx` around lines 152 - 158, The setCountry function updates currency but not locale, causing consumers of locale (e.g., API calls) to see a stale value; modify setCountry (the function using setCountryState, findCountry, countries, and setCurrency) to also setLocale immediately when a matching countryObj is found by deriving the locale from that object (e.g., countryObj.locale or equivalent field) so locale is updated synchronously alongside currency before navigation; keep existing behavior if no locale is available.
106-150:useEffectre-fetches store + countries on every navigation due toinitialLocalein deps.
initialLocaleandinitialCountryare derived from the URL, so they change whenever the user switches country/locale viarouter.push. BecauseStoreProviderlives in a shared layout, this means the fullPromise.all([getStoreAction(), getCountriesAction()])is re-executed on every country selection — fetching data that is already known.Consider using a
useRefguard to run the full fetch only once (or only when the component truly mounts), and update just the resolved state from the newinitialCountry/initialLocaleon subsequent renders without re-fetching:♻️ Suggested approach
+ const hasFetchedRef = useRef(false); + useEffect(() => { const fetchData = async () => { + if (hasFetchedRef.current) { + // Props changed due to navigation; re-resolve without re-fetching + if (store && countries.length > 0) { + const resolved = resolveCountryAndCurrency(countries, store, initialCountry); + setCountryState(resolved.country?.iso.toLowerCase() ?? initialCountry); + setCurrency(resolved.currency); + setLocaleState(resolved.locale || initialLocale); + } + return; + } + hasFetchedRef.current = true; try { ... } }; fetchData(); }, [initialCountry, initialLocale, router]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/StoreContext.tsx` around lines 106 - 150, The effect currently re-runs the full fetch (getStoreAction + getCountriesAction) whenever initialLocale/initialCountry change; add a useRef (e.g., fetchedRef) to ensure the heavy Promise.all fetchData runs only once on initial mount, and only set fetchedRef.current = true after successful fetch; on subsequent renders when initialLocale or initialCountry change, skip calling getStoreAction/getCountriesAction and instead call resolveCountryAndCurrency using the already-set store and countries state (or bail if not yet loaded) to update setCountryState, setLocaleState, setCurrency, setStoreCookies and router.replace as needed; keep the useEffect dependencies minimal (router and the refs/state you read) so navigation-driven locale changes update resolved state without re-fetching data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/data/countries.ts`:
- Around line 3-6: The named imports in the import statement are not
alphabetically ordered; reorder the specifiers so getCountry appears before
listCountries (keeping the existing local aliases _getCountry and
_listCountries) to satisfy Biome's alphabetical import rule.
---
Nitpick comments:
In `@src/contexts/StoreContext.tsx`:
- Line 141: The fallback is dead because resolveCountryAndCurrency always
returns a non-empty locale; update the setter to rely directly on the resolved
value instead of a redundant fallback: replace the expression using
setLocaleState(resolved.locale || initialLocale) with a single-source assignment
(setLocaleState(resolved.locale)) in the StoreContext where you call
resolveCountryAndCurrency, removing the misleading initialLocale branch; if you
prefer a defensive guard, use a nullish check (resolved.locale ?? initialLocale)
instead of ||.
- Around line 152-158: The setCountry function updates currency but not locale,
causing consumers of locale (e.g., API calls) to see a stale value; modify
setCountry (the function using setCountryState, findCountry, countries, and
setCurrency) to also setLocale immediately when a matching countryObj is found
by deriving the locale from that object (e.g., countryObj.locale or equivalent
field) so locale is updated synchronously alongside currency before navigation;
keep existing behavior if no locale is available.
- Around line 106-150: The effect currently re-runs the full fetch
(getStoreAction + getCountriesAction) whenever initialLocale/initialCountry
change; add a useRef (e.g., fetchedRef) to ensure the heavy Promise.all
fetchData runs only once on initial mount, and only set fetchedRef.current =
true after successful fetch; on subsequent renders when initialLocale or
initialCountry change, skip calling getStoreAction/getCountriesAction and
instead call resolveCountryAndCurrency using the already-set store and countries
state (or bail if not yet loaded) to update setCountryState, setLocaleState,
setCurrency, setStoreCookies and router.replace as needed; keep the useEffect
dependencies minimal (router and the refs/state you read) so navigation-driven
locale changes update resolved state without re-fetching data.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/contexts/StoreContext.tsx (3)
147-148:initialLocalein the dependency array triggers a full re-fetch on every locale navigation.When
router.pushfires after a country switch, Next.js re-rendersStoreProviderwith a newinitialLocale. BecauseinitialLocaleis in the dependency array, theuseEffectre-runs and firesPromise.all([getStoreAction(), getCountriesAction()])again — data that hasn't changed. Consider guarding with ahasFetchedRefor splitting the effect so locale changes don't re-trigger the full fetch.♻️ Proposed approach
+ const hasFetchedRef = useRef(false); useEffect(() => { + if (hasFetchedRef.current) return; + hasFetchedRef.current = true; const fetchData = async () => {Alternatively, remove
initialLocalefrom the dependency array if the resolved locale from store/country data is always preferred over the URL locale after the first fetch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/StoreContext.tsx` around lines 147 - 148, The useEffect inside StoreProvider currently depends on initialLocale which causes fetchData to rerun and call Promise.all([getStoreAction(), getCountriesAction()]) on every locale navigation; change this by preventing redundant refetches—either add a hasFetchedRef (e.g., a useRef flag checked/updated inside the effect) so fetchData runs only once on mount, or split the effect into two: one effect that runs once to call getStoreAction/getCountriesAction (no initialLocale dependency) and a separate effect that responds to locale changes for only locale-specific updates; update the dependency array accordingly and ensure fetchData, getStoreAction, and getCountriesAction are referenced consistently in your changed effects.
121-133:setLoading(false)on Line 131 is redundant —finallyalways runs it.The
finallyblock unconditionally callssetLoading(false)(Line 143), so the explicit call on Line 131 before the earlyreturnis never needed. Remove it to avoid setting the same state twice.♻️ Proposed fix
setCountryState(resolved.country.iso.toLowerCase()); setLocaleState(newLocale); setCurrency(resolved.currency); router.replace(newPath); - setLoading(false); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/StoreContext.tsx` around lines 121 - 133, The setLoading(false) call inside the resolved.needsRedirect branch is redundant because the outer finally block already calls setLoading(false); remove the explicit setLoading(false) before the early return in the block that handles resolved.needsRedirect (the branch using resolved.needsRedirect, resolved.country, setStoreCookies, setCountryState, setLocaleState, router.replace and return) so state is only set once in the finally.
139-139:|| initialLocalefallback is unreachable.
resolveCountryAndCurrencyalways returns a non-emptylocalestring (the fallback chain bottoms out at"en"), soresolved.localeis never falsy. The|| initialLocalebranch is dead code.♻️ Proposed fix
- setLocaleState(resolved.locale || initialLocale); + setLocaleState(resolved.locale);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/StoreContext.tsx` at line 139, The fallback "|| initialLocale" is dead because resolveCountryAndCurrency always returns a non-empty resolved.locale; update the assignment to use the resolved locale directly by replacing setLocaleState(resolved.locale || initialLocale) with setLocaleState(resolved.locale) (or, if you want a minimal defensive guard, use setLocaleState(resolved?.locale ?? initialLocale)); reference setLocaleState, resolved.locale and resolveCountryAndCurrency to locate the change.src/lib/data/countries.ts (1)
8-19: Missing explicit return types on exported async functions.
getCountriesandgetCountryhave no declared return types, which violates the project's strict TypeScript guidelines.As per coding guidelines, "use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/data/countries.ts` around lines 8 - 19, Add explicit TypeScript return types to the exported async functions to satisfy strict typing: annotate getCountries with the same Promise return type as _listCountries (e.g., Promise<Country[]> or the exact type exported by _listCountries) and annotate getCountry with the same Promise return type as _getCountry (e.g., Promise<Country> or Promise<CountryWithStates | null> depending on _getCountry’s signature); inspect the declared return types of _listCountries and _getCountry and mirror them in the function signatures for getCountries and getCountry (do not use any).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/layout/CountrySwitcher.tsx`:
- Line 113: The use of c.currency in CountrySwitcher.tsx relies on the
StoreCountry shape and duplicates the same field-name issue flagged in
StoreContext.tsx; update CountrySwitcher to access the canonical field defined
on StoreCountry (either revert to c.default_currency or use a type-safe access
like optional chaining with a fallback) and ensure its usage matches the
verification script referenced in StoreContext.tsx so the component will not
break if the StoreCountry type uses default_currency; confirm and align the
property name in CountrySwitcher, the StoreCountry type, and any mapping logic
that translates external data to StoreCountry.
---
Nitpick comments:
In `@src/contexts/StoreContext.tsx`:
- Around line 147-148: The useEffect inside StoreProvider currently depends on
initialLocale which causes fetchData to rerun and call
Promise.all([getStoreAction(), getCountriesAction()]) on every locale
navigation; change this by preventing redundant refetches—either add a
hasFetchedRef (e.g., a useRef flag checked/updated inside the effect) so
fetchData runs only once on mount, or split the effect into two: one effect that
runs once to call getStoreAction/getCountriesAction (no initialLocale
dependency) and a separate effect that responds to locale changes for only
locale-specific updates; update the dependency array accordingly and ensure
fetchData, getStoreAction, and getCountriesAction are referenced consistently in
your changed effects.
- Around line 121-133: The setLoading(false) call inside the
resolved.needsRedirect branch is redundant because the outer finally block
already calls setLoading(false); remove the explicit setLoading(false) before
the early return in the block that handles resolved.needsRedirect (the branch
using resolved.needsRedirect, resolved.country, setStoreCookies,
setCountryState, setLocaleState, router.replace and return) so state is only set
once in the finally.
- Line 139: The fallback "|| initialLocale" is dead because
resolveCountryAndCurrency always returns a non-empty resolved.locale; update the
assignment to use the resolved locale directly by replacing
setLocaleState(resolved.locale || initialLocale) with
setLocaleState(resolved.locale) (or, if you want a minimal defensive guard, use
setLocaleState(resolved?.locale ?? initialLocale)); reference setLocaleState,
resolved.locale and resolveCountryAndCurrency to locate the change.
In `@src/lib/data/countries.ts`:
- Around line 8-19: Add explicit TypeScript return types to the exported async
functions to satisfy strict typing: annotate getCountries with the same Promise
return type as _listCountries (e.g., Promise<Country[]> or the exact type
exported by _listCountries) and annotate getCountry with the same Promise return
type as _getCountry (e.g., Promise<Country> or Promise<CountryWithStates | null>
depending on _getCountry’s signature); inspect the declared return types of
_listCountries and _getCountry and mirror them in the function signatures for
getCountries and getCountry (do not use any).
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores