Skip to content

Fix issues 191 188 180 - #196

Closed
laaichiu wants to merge 3 commits into
spree:mainfrom
laaichiu:fix-issues-191-188-180
Closed

Fix issues 191 188 180#196
laaichiu wants to merge 3 commits into
spree:mainfrom
laaichiu:fix-issues-191-188-180

Conversation

@laaichiu

@laaichiu laaichiu commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes

    • Product details now display the master SKU when no custom variant SKU is available.
    • Improved policy page metadata with localized descriptions, canonical URLs, and more accurate social sharing links.
    • Reduced root-page hydration warnings caused by browser extensions.
  • Tests

    • Added coverage for policy metadata, product SKU display, default variant data, and root layout behavior.

@vercel

vercel Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

@laaichiu is attempting to deploy a commit to the Spree Commerce Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Policy metadata now builds localized descriptions and canonical URLs. Product details fall back to the default variant SKU and request its data. The root layout suppresses hydration warnings, with tests covering each change.

Changes

Policy Metadata

Layer / File(s) Summary
Policy metadata generation and coverage
src/app/[country]/[locale]/(storefront)/policies/[slug]/page.tsx, src/app/[country]/[locale]/(storefront)/policies/[slug]/page.test.tsx
Policy metadata now combines the policy and store names, sets canonical and Open Graph URLs when available, and tests localized metadata output.

Product SKU Fallback

Layer / File(s) Summary
Default variant SKU display
src/lib/data/cached.ts, src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx, src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.test.tsx
Product data expands default_variant, and ProductDetails displays its SKU when no selected variant SKU exists.

Root Layout Hydration

Layer / File(s) Summary
Root HTML hydration attribute
src/app/layout.tsx, src/app/layout.test.tsx
The root <html> element sets suppressHydrationWarning, with isolated tests verifying the attribute.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • spree/storefront#184: Both changes use selected or default variant data in ProductDetails.
  • spree/storefront#190: The wholesale portal also relies on PRODUCT_PAGE_EXPAND and shared ProductDetails SKU data.

Suggested reviewers: damianlegawiec

Poem

I’m a rabbit with metadata bright,
Canonical paths now hop just right.
A default SKU joins the display,
Hydration warnings burrow away.
Tests nibble every change with care.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title references issue numbers but does not describe the actual changes in the pull request. Use a concise, specific title that summarizes the main change, such as fixing product SKU display and metadata/canonical URL behavior.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.test.tsx (3)

5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the @/ alias for the component import.

This test uses a relative import even though repository guidelines require absolute imports. Replace ./ProductDetails with the corresponding @/... path.

As per coding guidelines, TypeScript files should use absolute @/ imports instead of relative imports.

Proposed change
-import { ProductDetails } from "./ProductDetails";
+import { ProductDetails } from "`@/app/`[country]/[locale]/(storefront)/products/[slug]/ProductDetails";
🤖 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 test’s ProductDetails import to use the
repository’s `@/` absolute alias instead of the relative ./ProductDetails path,
preserving the same module target.

Source: Coding guidelines


36-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep the test fixture type-checked.

as unknown as Product bypasses the SDK contract, so required or renamed fields can drift without this test catching it. Define the object with satisfies Product and add any required fields, or construct a fully typed Product.

As per coding guidelines, use strict TypeScript checking and satisfies for object literals instead of bypassing the type contract with a double assertion.

🤖 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, and add or correct all
fields required by the Product contract so TypeScript validates the object while
preserving the existing test data.

Source: Coding guidelines


77-86: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Exercise the new SKU fallback branch.

Because ProductDetails initially selects product.default_variant, this fixture makes selectedVariant?.sku equal "MASTER-SKU-001". The test would still pass if the new fallback were removed. Select a variant without a SKU while keeping the default variant SKU populated, then assert the fallback value.

🤖 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 77 - 86, Update the ProductDetails test to select a variant whose
sku is absent while leaving product.default_variant with "MASTER-SKU-001", then
assert the rendered SKU is the intended fallback value. Ensure the test
exercises the selectedVariant?.sku fallback branch rather than only verifying
the default variant SKU.
🤖 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]/(storefront)/products/[slug]/ProductDetails.test.tsx:
- Line 5: Update the ProductDetails test’s ProductDetails import to use the
repository’s `@/` absolute alias instead of the relative ./ProductDetails path,
preserving the same module target.
- Around line 36-70: Update the productWithoutCustomVariants fixture to use
satisfies Product instead of as unknown as Product, and add or correct all
fields required by the Product contract so TypeScript validates the object while
preserving the existing test data.
- Around line 77-86: Update the ProductDetails test to select a variant whose
sku is absent while leaving product.default_variant with "MASTER-SKU-001", then
assert the rendered SKU is the intended fallback value. Ensure the test
exercises the selectedVariant?.sku fallback branch rather than only verifying
the default variant SKU.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ce19e33c-ebc7-4af5-9211-1683a8a7038a

📥 Commits

Reviewing files that changed from the base of the PR and between 437ae66 and 4ce8c65.

📒 Files selected for processing (7)
  • src/app/[country]/[locale]/(storefront)/policies/[slug]/page.test.tsx
  • src/app/[country]/[locale]/(storefront)/policies/[slug]/page.tsx
  • src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.test.tsx
  • src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx
  • src/app/layout.test.tsx
  • src/app/layout.tsx
  • src/lib/data/cached.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant