Merged
Conversation
Use new Spree 6-ready resources/naming
WalkthroughDependency bumps to Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant UIComp as DeliveryMethodSection
participant Page as Checkout Page Handler
participant DataLayer as src/lib/data/checkout.selectDeliveryRate
participant SDK as `@spree/sdk.selectDeliveryRate`
participant API as Server/API
rect rgba(200,200,255,0.5)
User ->> UIComp: choose delivery rate (rateId)
UIComp ->> Page: onDeliveryRateSelect(fulfillmentId, rateId)
Page ->> DataLayer: selectDeliveryRate(cartId, fulfillmentId, rateId)
DataLayer ->> SDK: selectDeliveryRateSdk(cartId, fulfillmentId, rateId)
SDK ->> API: POST /cart/:id/fulfillments/:id/select_rate
API -->> SDK: 200 OK
SDK -->> DataLayer: success response
DataLayer -->> Page: updated cart/order
Page -->> UIComp: refresh state / show processing result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
📝 Coding Plan
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 (2)
src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx (1)
32-40: Initializer fallback is redundant after Line 33 guardAfter the early return at Line 33,
return product.default_variant || nullis unreachable as a “default variant” path and effectively just returnsnull. Consider simplifying to reduce ambiguity.Cleanup diff
- // Initialize with default variant or first available variant + // Initialize with default variant or first available purchasable variant const [selectedVariant, setSelectedVariant] = useState<Variant | null>(() => { if (product.default_variant) { return product.default_variant; } if (hasVariants) { return variants.find((v) => v.purchasable) || variants[0]; } - // For products without variants, use default variant - return product.default_variant || null; + return null; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx around lines 32 - 40, The initializer for selectedVariant contains a redundant final fallback ("return product.default_variant || null") that can never add value because product.default_variant is already handled earlier; update the initializer in the selectedVariant useState (the function passed to useState) to simply: if (product.default_variant) return product.default_variant; if (hasVariants) return variants.find(v => v.purchasable) || variants[0]; return null; — i.e., remove the redundant product.default_variant || null and return plain null for the no-variant case so the logic is unambiguous.src/lib/data/checkout.ts (1)
64-75: Mark intentionally unusedcartIdparameters to avoid ambiguous API intent.Line 64 and Line 72 accept
cartIdbut do not use it. If this is intentional for signature compatibility, prefix with_to make that explicit and keep lint/readability clean.As per coding guidelines: "Remove unused variables and imports. If intentionally unused, prefix parameters with underscore."♻️ Proposed fix
-export async function getFulfillments(cartId: string) { +export async function getFulfillments(_cartId: string) { @@ export async function selectDeliveryRate( - cartId: string, + _cartId: string, fulfillmentId: string, deliveryRateId: string, ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/data/checkout.ts` around lines 64 - 75, The functions getFulfillments and selectDeliveryRate declare a cartId parameter that is not used; to make the intent explicit and satisfy linting, rename those parameters to _cartId (i.e., change the signature of getFulfillments(cartId: string) and selectDeliveryRate(cartId: string, fulfillmentId: string, deliveryRateId: string) to use _cartId) so callers/signatures remain compatible while marking the parameter as intentionally unused; update any internal references (none expected) and keep the rest of the function body unchanged.
🤖 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]/(storefront)/account/orders/[id]/page.tsx:
- Around line 332-338: The FulfillmentBlock is receiving the full order.items
array for every fulfillment, causing duplicate line items; update the
FulfillmentBlock instantiation in the order.fulfillments.map to pass only items
belonging to that fulfillment (e.g., derive itemsForFulfillment by filtering
order.items against the fulfillment's associated item ids or fulfillment.items
field) and pass that filtered array as the lineItems prop instead of
order.items; locate the map over order.fulfillments and replace
lineItems={order.items || []} with lineItems={/* filtered items for this
fulfillment */} referencing fulfillment.id or
fulfillment.line_item_ids/fulfillment.items as appropriate.
In `@src/app/`[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx:
- Around line 23-26: The current useMemo for "variants" uses filter(Boolean)
which lets master variants through and causes broken matching/purchasability in
VariantPicker; change the filter to explicitly exclude master variants (e.g.,
filter(v => v && !v.isMaster && !v.master)) so only real sellable variants from
product.variants are returned, then pass that filtered "variants" into
VariantPicker as before; keep the same dependency array ([product.variants]).
---
Nitpick comments:
In `@src/app/`[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx:
- Around line 32-40: The initializer for selectedVariant contains a redundant
final fallback ("return product.default_variant || null") that can never add
value because product.default_variant is already handled earlier; update the
initializer in the selectedVariant useState (the function passed to useState) to
simply: if (product.default_variant) return product.default_variant; if
(hasVariants) return variants.find(v => v.purchasable) || variants[0]; return
null; — i.e., remove the redundant product.default_variant || null and return
plain null for the no-variant case so the logic is unambiguous.
In `@src/lib/data/checkout.ts`:
- Around line 64-75: The functions getFulfillments and selectDeliveryRate
declare a cartId parameter that is not used; to make the intent explicit and
satisfy linting, rename those parameters to _cartId (i.e., change the signature
of getFulfillments(cartId: string) and selectDeliveryRate(cartId: string,
fulfillmentId: string, deliveryRateId: string) to use _cartId) so
callers/signatures remain compatible while marking the parameter as
intentionally unused; update any internal references (none expected) and keep
the rest of the function body unchanged.
🪄 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
Run ID: bbb7f8b9-d2c5-43dd-9742-789676af3f1a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
package.jsonsrc/app/[country]/[locale]/(checkout)/checkout/[id]/page.tsxsrc/app/[country]/[locale]/(checkout)/order-placed/[id]/page.tsxsrc/app/[country]/[locale]/(storefront)/account/gift-cards/page.tsxsrc/app/[country]/[locale]/(storefront)/account/orders/[id]/page.tsxsrc/app/[country]/[locale]/(storefront)/account/orders/page.tsxsrc/app/[country]/[locale]/(storefront)/account/page.tsxsrc/app/[country]/[locale]/(storefront)/cart/page.tsxsrc/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsxsrc/components/checkout/DeliveryMethodSection.tsxsrc/components/checkout/Summary.tsxsrc/components/checkout/index.tssrc/lib/analytics/gtm.tssrc/lib/data/__tests__/checkout.test.tssrc/lib/data/checkout.ts
src/app/[country]/[locale]/(storefront)/account/orders/[id]/page.tsx
Outdated
Show resolved
Hide resolved
src/app/[country]/[locale]/(storefront)/products/[slug]/ProductDetails.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/[country]/[locale]/(storefront)/account/orders/[id]/page.tsx (1)
205-238:⚠️ Potential issue | 🟡 MinorAlign tracking-related field checks for consistency.
Line 205 checks
fulfillment.tracking_urlto enable the Track button, while line 234 checks!fulfillment.trackingto show the "No tracking information" message. While these are separate fields in the Fulfillment type (trackingis the number/code andtracking_urlis the derived tracking link), using them interchangeably for related logic creates a maintainability risk.Consider checking the same field for both conditions (preferably
tracking_urlsince it's the actionable link), or add a comment explaining the intentional semantic difference. This prevents edge cases where the fields could drift out of sync without visibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[country]/[locale]/(storefront)/account/orders/[id]/page.tsx around lines 205 - 238, The tracking logic mixes fulfillment.tracking_url (used to enable the "Track Items" link) and fulfillment.tracking (used to decide whether to show "No tracking information"), which risks inconsistent behavior; update the checks to use the same field (preferably fulfillment.tracking_url since it's the actionable link) in both places—i.e., change the conditional that currently tests !fulfillment.tracking to test !fulfillment.tracking_url—or, if there is an intentional semantic difference, add a brief inline comment near the two conditionals explaining why tracking and tracking_url are treated differently so future maintainers won’t conflate them (affecting the conditionals that reference fulfillment.status, fulfillment.tracking_url, and fulfillment.tracking).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/app/`[country]/[locale]/(storefront)/account/orders/[id]/page.tsx:
- Around line 205-238: The tracking logic mixes fulfillment.tracking_url (used
to enable the "Track Items" link) and fulfillment.tracking (used to decide
whether to show "No tracking information"), which risks inconsistent behavior;
update the checks to use the same field (preferably fulfillment.tracking_url
since it's the actionable link) in both places—i.e., change the conditional that
currently tests !fulfillment.tracking to test !fulfillment.tracking_url—or, if
there is an intentional semantic difference, add a brief inline comment near the
two conditionals explaining why tracking and tracking_url are treated
differently so future maintainers won’t conflate them (affecting the
conditionals that reference fulfillment.status, fulfillment.tracking_url, and
fulfillment.tracking).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43d7433a-4ee1-4f9a-99da-375c093b5517
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/app/[country]/[locale]/(storefront)/account/orders/[id]/page.tsx
This was referenced Mar 17, 2026
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.
Use new Spree 6-ready resources/naming
Summary by CodeRabbit