Fix international SEO, Market-aware routing, SKU fallback, and extension hydration - #198
Conversation
|
@laaichiu is attempting to deploy a commit to the Spree Commerce Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
Next review available in: 59 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe PR centralizes locale configuration and negotiation, adds market-aware routing and runtime message loading, introduces localized canonical and hreflang metadata, refactors sitemap generation around chunked catalog ranges, improves asynchronous storefront navigation, and fixes SKU rendering for products without custom variants. ChangesLocalized storefront routing
Localized metadata and policy retrieval
Chunked sitemap generation
Product detail SKU fallback
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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: 2
🧹 Nitpick comments (8)
src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.test.tsx (3)
36-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType-check the product fixture instead of bypassing it.
as unknown as Productsuppresses SDK shape validation. Usesatisfies Product(and fill required fields) or a typed fixture builder so SDK contract changes fail the test at compile time.As per coding guidelines, “use
satisfiesfor type checking object literals.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[country]/[locale]/(storefront)/products/[slug]/ProductDetails.test.tsx around lines 36 - 70, Update the productWithoutCustomVariants fixture to use satisfies Product instead of as unknown as Product, adding any required Product fields so the object conforms to the SDK contract and future shape changes fail compilation.Source: Coding guidelines
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the configured absolute import alias.
Replace
./ProductDetailswith its@/app/.../ProductDetailspath.As per coding guidelines, “Use absolute imports with
@/alias prefix instead of relative imports.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[country]/[locale]/(storefront)/products/[slug]/ProductDetails.test.tsx at line 5, Update the ProductDetails import in the test to use the configured `@/` absolute alias and the module’s full app path instead of the relative "./ProductDetails" path.Source: Coding guidelines
7-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd explicit return types to the new callbacks.
The mock factories/components and
describe/itcallbacks currently rely on inferred return types.As per coding guidelines, “Always define explicit return types for functions.”
Also applies to: 72-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[country]/[locale]/(storefront)/products/[slug]/ProductDetails.test.tsx around lines 7 - 34, Add explicit return type annotations to the callback functions introduced in the test, including the vi.mock factories, mocked components, and describe/it callbacks. Update the callbacks near the mocked modules and the additional callbacks around the referenced later section, using appropriate return types while preserving their existing behavior.Source: Coding guidelines
src/lib/data/sitemap.ts (1)
23-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit return type to
getSitemapMarkets.Unlike the other three exports, this function has no explicit return type and relies on inference of
.data.As per coding guidelines: "Always define explicit return types for functions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/data/sitemap.ts` around lines 23 - 28, Update the exported getSitemapMarkets function to declare an explicit return type matching the resolved type of the markets.list(options).data result, consistent with the other sitemap exports, while preserving its existing implementation.Source: Coding guidelines
src/app/[country]/[locale]/layout.tsx (1)
87-133: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize the independent
getMarkets/loadMessagescalls.
loadMessages(requestedLocale)only depends onrequestedLocale, which is known beforegetMarketsis even called, yet it's awaited later, after several branches. Per the project's data-fetching guideline, kick both off together and awaitloadMessageswhere needed.⚡ Sketch: start loadMessages alongside getMarkets
const requestedLocale = resolveSupportedLocale(locale); if (!requestedLocale) notFound(); - const markets = await getMarkets({ country, locale: requestedLocale }) - .then((res) => res.data) - .catch(() => null); + const messagesPromise = loadMessages(requestedLocale); + const markets = await getMarkets({ country, locale: requestedLocale }) + .then((res) => res.data) + .catch(() => null);Then replace the later
const messages = await loadMessages(requestedLocale);occurrences withawait messagesPromise.As per coding guidelines, "Use Promise.all() for parallel data fetching in server components instead of sequential awaits."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[country]/[locale]/layout.tsx around lines 87 - 133, Parallelize the independent market and message fetches in the layout’s data-loading flow: start getMarkets and loadMessages(requestedLocale) together using Promise.all, retain the existing markets error handling and validation branches, and replace every later loadMessages await with the shared messages result.Source: Coding guidelines
src/lib/spree/middleware.test.ts (1)
22-32: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse
buildLocalizedRedirectPathfor middleware locale redirects.
buildLocalizedRedirectPathexists insrc/i18n/routing.ts, but this middleware still reimplements prefix matching/slicing increateSpreeMiddleware. Reuse that helper for lines 108–132 to keep middleware and layout-level redirects aligned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/spree/middleware.test.ts` around lines 22 - 32, Update createSpreeMiddleware’s locale redirect logic to use the existing buildLocalizedRedirectPath helper from routing.ts instead of reimplementing locale prefix matching and path slicing. Preserve the current redirect behavior, including retaining the request path and applying the resolved country and locale values.src/lib/metadata/alternates.ts (1)
194-210: 🧹 Nitpick | 🔵 TrivialConsider logging swallowed
resolvePathfailures.
.catch(() => null)treats a genuine "no localized resource" 404 and a transient Store API 500 identically, silently dropping that locale from the hreflang cluster (this is called out as deliberate in the docstring at lines 137-146, so not raising as a bug). For SEO-facing output it may be worth at least logging/observing these failures so a temporary Store API blip isn't mistaken for permanently-missing translations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/metadata/alternates.ts` around lines 194 - 210, Add observability to the resolvePath failure handler in resolveLocalizedResource without changing its existing null-return behavior. Capture the rejection error and log or report it through the module’s established logging/monitoring mechanism, while continuing to cache and return null for failed resolutions.src/app/[country]/[locale]/(storefront)/policies/[slug]/page.test.tsx (1)
16-19: 📐 Maintainability & Code Quality | 🔵 TrivialConsider testing
generateMetadata'sresolvePathclosure directly.
buildLocalizedAlternatesis fully mocked here, so the actualresolvePathimplementation inpage.tsx(which callscachedGetPolicy(policy.id, target)and maps its result to{ path, fingerprint }orundefined) is untested by this file, and isn't covered elsewhere with the real closure either. Consider asserting the args passed tobuildLocalizedAlternates(viamock.calls) and/or invoking the capturedresolvePathcallback with a mockedcachedGetPolicyto cover the null/localized-slug mapping.Also applies to: 49-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[country]/[locale]/(storefront)/policies/[slug]/page.test.tsx around lines 16 - 19, Extend the generateMetadata tests to cover the real resolvePath closure in page.tsx instead of only mocking buildLocalizedAlternates. Inspect buildLocalizedAlternates mock.calls to capture and invoke resolvePath, mock cachedGetPolicy with matching and null results, and assert it returns { path, fingerprint } for localized policies and undefined when no policy is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/`[country]/[locale]/layout.tsx:
- Around line 109-122: Update the currentMarket fallback in the layout component
to avoid redirecting when fallbackCountry and fallbackLocale match the requested
country and locale; use the existing outage-safe render path for this
empty-markets case instead. Preserve the redirect behavior when the fallback
target differs, and add coverage for getMarkets resolving to an empty data array
on the default route.
In `@src/lib/spree/middleware.ts`:
- Around line 106-122: Update the locale resolution for the country/locale
prefix in the middleware’s visible locale-matching flow to use negotiateLocale()
instead of matchLocale(), allowing values such as en-US to resolve to the
configured en bundle. Preserve the requested country and let the existing
canonical-prefix redirect normalize the locale spelling; only use the
defaultCountry/defaultLocale fallback when negotiation returns no locale.
---
Nitpick comments:
In `@src/app/`[country]/[locale]/(storefront)/policies/[slug]/page.test.tsx:
- Around line 16-19: Extend the generateMetadata tests to cover the real
resolvePath closure in page.tsx instead of only mocking
buildLocalizedAlternates. Inspect buildLocalizedAlternates mock.calls to capture
and invoke resolvePath, mock cachedGetPolicy with matching and null results, and
assert it returns { path, fingerprint } for localized policies and undefined
when no policy is found.
In
`@src/app/`[country]/[locale]/(storefront)/products/[slug]/ProductDetails.test.tsx:
- Around line 36-70: Update the productWithoutCustomVariants fixture to use
satisfies Product instead of as unknown as Product, adding any required Product
fields so the object conforms to the SDK contract and future shape changes fail
compilation.
- Line 5: Update the ProductDetails import in the test to use the configured `@/`
absolute alias and the module’s full app path instead of the relative
"./ProductDetails" path.
- Around line 7-34: Add explicit return type annotations to the callback
functions introduced in the test, including the vi.mock factories, mocked
components, and describe/it callbacks. Update the callbacks near the mocked
modules and the additional callbacks around the referenced later section, using
appropriate return types while preserving their existing behavior.
In `@src/app/`[country]/[locale]/layout.tsx:
- Around line 87-133: Parallelize the independent market and message fetches in
the layout’s data-loading flow: start getMarkets and
loadMessages(requestedLocale) together using Promise.all, retain the existing
markets error handling and validation branches, and replace every later
loadMessages await with the shared messages result.
In `@src/lib/data/sitemap.ts`:
- Around line 23-28: Update the exported getSitemapMarkets function to declare
an explicit return type matching the resolved type of the
markets.list(options).data result, consistent with the other sitemap exports,
while preserving its existing implementation.
In `@src/lib/metadata/alternates.ts`:
- Around line 194-210: Add observability to the resolvePath failure handler in
resolveLocalizedResource without changing its existing null-return behavior.
Capture the rejection error and log or report it through the module’s
established logging/monitoring mechanism, while continuing to cache and return
null for failed resolutions.
In `@src/lib/spree/middleware.test.ts`:
- Around line 22-32: Update createSpreeMiddleware’s locale redirect logic to use
the existing buildLocalizedRedirectPath helper from routing.ts instead of
reimplementing locale prefix matching and path slicing. Preserve the current
redirect behavior, including retaining the request path and applying the
resolved country and locale values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 090b2ebd-9cda-45ef-bde5-b0fae1a6d0e7
📒 Files selected for processing (38)
src/app/[country]/[locale]/(storefront)/page.tsxsrc/app/[country]/[locale]/(storefront)/policies/[slug]/page.test.tsxsrc/app/[country]/[locale]/(storefront)/policies/[slug]/page.tsxsrc/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.test.tsxsrc/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsxsrc/app/[country]/[locale]/layout.test.tsxsrc/app/[country]/[locale]/layout.tsxsrc/app/dev/layout.tsxsrc/app/robots.tssrc/app/sitemap.test.tssrc/app/sitemap.tssrc/components/layout/DocumentShell.test.tsxsrc/components/layout/DocumentShell.tsxsrc/i18n/__tests__/locales.test.tssrc/i18n/__tests__/markets.test.tssrc/i18n/__tests__/routing.test.tssrc/i18n/locales.tssrc/i18n/markets.tssrc/i18n/normalize.tssrc/i18n/request.tssrc/i18n/routing.tssrc/lib/data/__tests__/policies.test.tssrc/lib/data/__tests__/sitemap.test.tssrc/lib/data/cached.tssrc/lib/data/categories.tssrc/lib/data/policies.tssrc/lib/data/sitemap.tssrc/lib/metadata/__tests__/alternates.test.tssrc/lib/metadata/alternates.tssrc/lib/metadata/categories.tssrc/lib/metadata/category.tssrc/lib/metadata/home.tssrc/lib/metadata/product.tssrc/lib/metadata/products.tssrc/lib/spree/middleware.test.tssrc/lib/spree/middleware.tssrc/proxy.tssrc/types/next-intl.d.ts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/app/[country]/[locale]/layout.test.tsx (1)
26-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType the new mock and test callbacks explicitly.
Add return annotations and use
satisfies(or an equivalent typed mock) so the@/lib/storemock cannot drift from its module contract; annotate the async test callback asasync (): Promise<void> =>.Also applies to: 111-111
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`[country]/[locale]/layout.test.tsx around lines 26 - 29, Type the `@/lib/store` mock explicitly by annotating getDefaultCountry and getDefaultLocale return values and constraining the mock with satisfies or an equivalent typed mock against the module contract. Update the async test callback referenced in the test to use the explicit async (): Promise<void> signature.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/app/`[country]/[locale]/layout.test.tsx:
- Around line 26-29: Type the `@/lib/store` mock explicitly by annotating
getDefaultCountry and getDefaultLocale return values and constraining the mock
with satisfies or an equivalent typed mock against the module contract. Update
the async test callback referenced in the test to use the explicit async ():
Promise<void> signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 23eaf9e0-3942-432b-8a96-48b2c8aa480a
📒 Files selected for processing (2)
src/app/[country]/[locale]/layout.test.tsxsrc/app/[country]/[locale]/layout.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/[country]/[locale]/layout.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/`[country]/[locale]/(storefront)/layout.tsx:
- Around line 14-44: Update the navigation Suspense boundaries that currently
use async Header and Footer fallbacks: move category-aware Header/Footer
rendering into the suspended content, and replace each fallback with a
lightweight synchronous skeleton or loading placeholder. Keep the existing
getRootCategories and Header/Footer category behavior unchanged once the
boundary resolves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c9c29262-94b7-4172-a0e7-420384c5e45f
📒 Files selected for processing (6)
src/app/[country]/[locale]/(storefront)/layout.tsxsrc/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsxsrc/app/[country]/[locale]/layout.test.tsxsrc/app/[country]/[locale]/layout.tsxsrc/lib/data/categories.tssrc/lib/spree/middleware.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx
- src/app/[country]/[locale]/layout.tsx
- src/lib/spree/middleware.ts
2 participants
Summary
This PR fixes several storefront issues related to product data, international SEO, locale routing, sitemap generation, and hydration behavior.
It also introduces a shared Market-aware locale foundation so routing, metadata, and sitemaps use the same set of storefront-supported locales.
Changes
Display the default variant SKU
Products without custom variants now display the SKU from
default_variant.default_varianton product detail requests.Add canonical metadata to policy pages
Policy pages now generate a localized canonical URL using the current country, locale, and resolved policy slug.
rel="canonical"metadata.404responses as missing policies instead of hiding unrelated API errors.Make routing and sitemaps Market-aware
Locale handling is now derived from a single registry of storefront message bundles and the Markets returned by the Store API.
Routing now:
Accept-Languageusing quality weights.langanddirattributes from the resolved route locale.Sitemaps now:
Tolerate browser-extension root mutations
Adds
suppressHydrationWarningonly to the root<html>element.This handles extensions that inject classes or inline styles before React hydrates the document, while still allowing hydration mismatches inside the application tree to be reported normally.
Add reciprocal Market-aware hreflang metadata
Localized metadata is now generated for:
The hreflang implementation:
en-USandde-DEwhen multiple countries are present.x-default.Adding a new storefront locale now requires only registering its message loader and enabling it in the relevant Market; middleware, metadata, and sitemap generation will use it automatically.
Verification
pnpm test— 24 test files, 174 tests passedpnpm check:localespnpm exec tsc --noEmitpnpm checkpnpm buildgit diff --checkAdditional runtime scenarios verified:
x-defaultremains stable across Markets and locales.Closes #191
Closes #188
Closes #180
Closes #187
Summary by CodeRabbit