fix: Avoid duplicate SKU selectors when changing locale#3264
fix: Avoid duplicate SKU selectors when changing locale#3264hellofanny wants to merge 1 commit intocanary-v2from
Conversation
WalkthroughThis change extracts the product data merging logic in a PDP page into a dedicated Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 docstrings
🧪 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 |
|
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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/pages/[slug]/p.tsx (1)
96-102: Simplify type safety by removing unnecessary casts inmergePdpData.The function accepts
client: Partial<ClientProductQueryQuery> | Record<string, unknown> | undefined, but casts it toServerProductQueryQueryfor deepmerge, then immediately re-casts it back to extractskuVariants. Instead, defineclientwith a narrower type (e.g.,Partial<ClientProductQueryQuery> | undefined) to eliminate bothasassertions and keep the path fully typed without type widening.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/pages/`[slug]/p.tsx around lines 96 - 102, The code widens `client` then uses unsafe casts in mergePdpData; change the `client` parameter type from `Partial<ClientProductQueryQuery> | Record<string, unknown> | undefined` to a narrower `Partial<ClientProductQueryQuery> | undefined`, remove the `as ServerProductQueryQuery` cast passed into `deepmerge` and the subsequent `as Partial<ClientProductQueryQuery>` cast when computing `clientSkuVariants`, and ensure `deepmerge(server, client ?? {}, { arrayMerge: overwriteMerge })` and the `client?.product?.isVariantOf?.skuVariants` access use the properly typed `client` so no type assertions are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/pages/`[slug]/p.tsx:
- Around line 96-102: The code widens `client` then uses unsafe casts in
mergePdpData; change the `client` parameter type from
`Partial<ClientProductQueryQuery> | Record<string, unknown> | undefined` to a
narrower `Partial<ClientProductQueryQuery> | undefined`, remove the `as
ServerProductQueryQuery` cast passed into `deepmerge` and the subsequent `as
Partial<ClientProductQueryQuery>` cast when computing `clientSkuVariants`, and
ensure `deepmerge(server, client ?? {}, { arrayMerge: overwriteMerge })` and the
`client?.product?.isVariantOf?.skuVariants` access use the properly typed
`client` so no type assertions are needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac8279a1-f1dd-4d18-b3a3-e8142e0c2509
📒 Files selected for processing (1)
packages/core/src/pages/[slug]/p.tsx
What's the purpose of this pull request?
This PR attempts to fix the duplicate SKU selectors when switching the locales.
How it works?
mergePdpDatauses deepmerge with overwriteMerge for arrays, then overridesproduct.isVariantOf.skuVariantswith the client payload when the client sends skuVariants. That way localized keys in availableVariations are not accumulated across server and client; the client’s skuVariants wins and the UI renders a single selector block.How to test it?
sku-selector-no-duplication.mov
Starters Deploy Preview
preview link
PR
References
https://vtex-dev.atlassian.net/browse/SFS-3094?atlOrigin=eyJpIjoiYjZmYWFkNTJhYWU2NDA5MzgxNTcyMzBjMzgwYmNlMDMiLCJwIjoiaiJ9
Summary by CodeRabbit