Improved markets support - #41
Merged
Merged
Conversation
Also update order currency and locale when switching markets
Contributor
WalkthroughMarket-scoped country/market handling added across the app: new markets data module; StoreContext now exposes enriched CountryWithMarket objects; checkout and country-switch flows resolve markets, update order market (currency/locale), and fetch market-specific countries. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CountrySwitcher as CountrySwitcher
participant Cart as Cart Context
participant CheckoutAPI as Checkout Data (updateOrderMarket)
participant Server as Spree Server
User->>CountrySwitcher: Select country
CountrySwitcher->>Cart: Read current order (orderId, currency, locale)
alt Currency or locale differ
CountrySwitcher->>CheckoutAPI: updateOrderMarket(orderId, {currency, locale})
CheckoutAPI->>Server: _updateOrder (market/currency/locale)
Server-->>CheckoutAPI: Updated order
CheckoutAPI-->>CountrySwitcher: Updated order
CountrySwitcher->>Cart: refreshCart()
Cart->>Server: Fetch latest cart
Server-->>Cart: Cart data
end
CountrySwitcher->>CountrySwitcher: Set cookies/state and navigate
sequenceDiagram
participant CheckoutPage as Checkout Page
participant StoreCtx as StoreContext
participant MarketsAPI as Markets Module
participant CountriesAPI as Markets Module (getMarketCountries)
participant Server as Spree Server
CheckoutPage->>MarketsAPI: resolveMarket(urlCountry)
MarketsAPI->>Server: _resolveMarket(urlCountry)
Server-->>MarketsAPI: Market object
MarketsAPI-->>CheckoutPage: Market or null
alt Market found
CheckoutPage->>CountriesAPI: getMarketCountries(market.id)
CountriesAPI->>Server: _listMarketCountries(marketId)
Server-->>CountriesAPI: Market-scoped countries
CountriesAPI-->>CheckoutPage: Countries list
else Market not found
CheckoutPage->>CheckoutPage: Use empty countries list
end
CheckoutPage->>StoreCtx: Load order with market context
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/contexts/StoreContext.tsx (1)
186-188: Keeplocalesynchronized whensetCountryis used.
setCountrycurrently updatescountryandcurrencyonly. SyncinglocalefromcountryObj.default_localeavoids temporary mixed market state for consumers reading context before navigation completes.🔧 Suggested adjustment
if (countryObj) { setCurrency(countryObj.currency); + 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 186 - 188, When updating country in setCountry, also synchronize locale so consumers don't observe mismatched market state: in the block where you check countryObj (the same place you call setCurrency(countryObj.currency)), also call setLocale(countryObj.default_locale) (or the equivalent state setter used in this context) so country, currency and locale are updated together; ensure you reference setCountry, setCurrency, setLocale and countryObj.default_locale in the change.src/lib/data/__tests__/checkout.test.ts (1)
125-137: Add the non-Errorfallback case forupdateOrderMarket.The new suite covers success and
Errorrejection, but not the fallback branch for non-Errorthrows (actionResultbehavior).Based on learnings: Applies to src/lib/data/**/*.{test,spec}.{ts,tsx} : Test Server Actions independently with proper mocking of Spree SDK and cookie dependencies.✅ Suggested test addition
describe("updateOrderMarket", () => { @@ it("returns error on failure", async () => { mockUpdateOrder.mockRejectedValue(new Error("Currency not supported")); @@ expect(result).toEqual({ success: false, error: "Currency not supported", }); }); + + it("returns fallback message for non-Error throws", async () => { + mockUpdateOrder.mockRejectedValue("unexpected"); + + const result = await updateOrderMarket("order-1", { + currency: "EUR", + locale: "de", + }); + + expect(result).toEqual({ + success: false, + error: "Failed to update order market", + }); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/data/__tests__/checkout.test.ts` around lines 125 - 137, Add a test that covers the non-Error rejection branch of updateOrderMarket by making mockUpdateOrder reject with a non-Error value (e.g., mockUpdateOrder.mockRejectedValue("NonErrorFailure") or an object) and assert that updateOrderMarket("order-1", {currency: "XYZ", locale: "en"}) returns { success: false, error: String(rejection) } (i.e., the actionResult fallback handling of non-Error throws). Reference updateOrderMarket and mockUpdateOrder in the new test so the fallback branch is exercised.
🤖 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/app/`[country]/[locale]/(checkout)/checkout/[id]/page.tsx:
- Around line 193-197: The checkout flow currently treats a rejection from
getMarketCountries as a fatal error; wrap the await
getMarketCountries(market.id) call in its own try/catch (or use .catch) so
failures only affect countriesData and don’t propagate to the outer catch that
marks the entire checkout failed; on error set countriesData = { data: [] as
StoreCountry[] } and optionally log the error (referencing getMarketCountries
and the countriesData variable in page.tsx) so orderData can still be used when
market-country lookup fails.
In `@src/components/layout/CountrySwitcher.tsx`:
- Around line 55-61: The current flow ignores failures from updateOrderMarket
and proceeds to refreshCart and update local state, risking inconsistent
cart/market data; modify the handler that calls updateOrderMarket to await its
result and check its return (e.g., const result = await
updateOrderMarket(cart.id, { currency: newCurrency, locale: newLocale })), and
if result.success is falsy (or result.error exists) then stop the local market
switch: surface/log the error (or show user feedback), do not call refreshCart
or update cookies/navigation, and ensure exceptions from updateOrderMarket are
caught with try/catch so failures prevent the UI/state transition for
functions/variables referenced here (updateOrderMarket, refreshCart, cart,
newCurrency, newLocale).
---
Nitpick comments:
In `@src/contexts/StoreContext.tsx`:
- Around line 186-188: When updating country in setCountry, also synchronize
locale so consumers don't observe mismatched market state: in the block where
you check countryObj (the same place you call setCurrency(countryObj.currency)),
also call setLocale(countryObj.default_locale) (or the equivalent state setter
used in this context) so country, currency and locale are updated together;
ensure you reference setCountry, setCurrency, setLocale and
countryObj.default_locale in the change.
In `@src/lib/data/__tests__/checkout.test.ts`:
- Around line 125-137: Add a test that covers the non-Error rejection branch of
updateOrderMarket by making mockUpdateOrder reject with a non-Error value (e.g.,
mockUpdateOrder.mockRejectedValue("NonErrorFailure") or an object) and assert
that updateOrderMarket("order-1", {currency: "XYZ", locale: "en"}) returns {
success: false, error: String(rejection) } (i.e., the actionResult fallback
handling of non-Error throws). Reference updateOrderMarket and mockUpdateOrder
in the new test so the fallback branch is exercised.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.jsonsrc/app/[country]/[locale]/(checkout)/checkout/[id]/page.tsxsrc/components/layout/CountrySwitcher.tsxsrc/contexts/StoreContext.tsxsrc/lib/data/__tests__/checkout.test.tssrc/lib/data/checkout.tssrc/lib/data/markets.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/contexts/StoreContext.tsx (1)
135-180: Consider moving store/market initialization to a Server Component to avoid useEffect-based initialization.This
useEffectblock at lines 136–180 fetches and initializes critical app state (store, markets, country/locale resolution) on the client. Per guidelines, initialization should avoiduseEffectand leverage server-driven patterns instead.While the Server Actions (
getStoreAction(),getMarketsAction()) are correctly used, the overall flow could be optimized by:
- Converting the
StoreProviderto a Server Component that fetches and initializes this data server-side- Passing pre-resolved store/market data and country/locale state as props to the provider
- Using React 19's
use()function to unwrap promises alongside Suspense boundaries for loading statesThis would eliminate the client-side initialization race condition, improve performance, and align with the Next.js Server Component-first architecture used elsewhere in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/StoreContext.tsx` around lines 135 - 180, The client-side initialization inside useEffect (fetchData) uses getStoreAction, getMarketsAction and resolveCountryAndCurrency to set store, countries, locale, currency and cookies—move this logic to the server: convert the StoreProvider into a Server Component that calls getStoreAction and getMarketsAction (or uses React 19 use() to unwrap their promises within a Suspense boundary), perform buildCountriesFromMarkets and resolveCountryAndCurrency server-side, and pass the resolved store, countries, countryState, localeState and currency as props into the provider so the client no longer runs the fetchData useEffect or sets cookies/redirects there (keep router.replace/cookie-setting handled by server-rendered redirect or minimal client effect only if unavoidable).src/lib/data/__tests__/checkout.test.ts (1)
39-39: Usevi.mocked()for type-safe mock casting.
vi.mocked(updateOrder)is the idiomatic vitest pattern that preserves mock typing withoutanycasts, aligning with strict TypeScript checking. The same pattern should be applied to all other mocks on lines 38–45.♻️ Proposed fixes
-// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -- test fixtures are intentionally partial -const mockGetCheckout = getCheckout as any; -const mockUpdateOrder = updateOrder as any; -const mockAdvance = advance as any; -const mockGetShipments = getShipmentsSdk as any; -const mockSelectShippingRate = selectShippingRateSdk as any; -const mockApplyCoupon = applyCoupon as any; -const mockRemoveCoupon = removeCoupon as any; -const mockComplete = complete as any; +const mockGetCheckout = vi.mocked(getCheckout); +const mockUpdateOrder = vi.mocked(updateOrder); +const mockAdvance = vi.mocked(advance); +const mockGetShipments = vi.mocked(getShipmentsSdk); +const mockSelectShippingRate = vi.mocked(selectShippingRateSdk); +const mockApplyCoupon = vi.mocked(applyCoupon); +const mockRemoveCoupon = vi.mocked(removeCoupon); +const mockComplete = vi.mocked(complete);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/data/__tests__/checkout.test.ts` at line 39, The tests cast mocks with unsafe any (e.g., const mockUpdateOrder = updateOrder as any); replace these with Vitest's type-safe helper by using vi.mocked(updateOrder) and similarly use vi.mocked(...) for all other mock variables in the same block (the mocks defined around updateOrder) so mock typing is preserved under strict TypeScript; update references to mockUpdateOrder accordingly to rely on the typed mock API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/contexts/StoreContext.tsx`:
- Around line 135-180: The client-side initialization inside useEffect
(fetchData) uses getStoreAction, getMarketsAction and resolveCountryAndCurrency
to set store, countries, locale, currency and cookies—move this logic to the
server: convert the StoreProvider into a Server Component that calls
getStoreAction and getMarketsAction (or uses React 19 use() to unwrap their
promises within a Suspense boundary), perform buildCountriesFromMarkets and
resolveCountryAndCurrency server-side, and pass the resolved store, countries,
countryState, localeState and currency as props into the provider so the client
no longer runs the fetchData useEffect or sets cookies/redirects there (keep
router.replace/cookie-setting handled by server-rendered redirect or minimal
client effect only if unavoidable).
In `@src/lib/data/__tests__/checkout.test.ts`:
- Line 39: The tests cast mocks with unsafe any (e.g., const mockUpdateOrder =
updateOrder as any); replace these with Vitest's type-safe helper by using
vi.mocked(updateOrder) and similarly use vi.mocked(...) for all other mock
variables in the same block (the mocks defined around updateOrder) so mock
typing is preserved under strict TypeScript; update references to
mockUpdateOrder accordingly to rely on the typed mock API.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/[country]/[locale]/(checkout)/checkout/[id]/page.tsxsrc/components/layout/CountrySwitcher.tsxsrc/contexts/StoreContext.tsxsrc/lib/data/__tests__/checkout.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Also update order currency and locale when switching markets
Summary by CodeRabbit
New Features
Tests
Chores