Fix gift card amount not deducted from Stripe payment (V-3386)#135
Open
Fix gift card amount not deducted from Stripe payment (V-3386)#135
Conversation
When a gift card was applied on the payment step, the Spree backend registered a store-credit payment but the storefront kept the original Stripe PaymentIntent (sized to `cart.total`). Customer ended up charged the full order total while the gift card was also processed, resulting in overcharge / "Credit Owed" orders. PaymentSection now watches `cart.amount_due ?? cart.total` and uses `paymentSessions.update` (with explicit `amount`) to resize the same Stripe PaymentIntent on cart changes, instead of creating a new session every time and leaving stale `Incomplete` PaymentIntents behind.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR updates Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/data/payment.ts (1)
33-57:cartIdparameter is ignored —requireCartId()is used instead.Same pattern as
createCheckoutPaymentSessionabove, so this is a pre-existing convention rather than a new defect. Worth noting that if a caller ever passes a cart id that differs from the cookie-resolved one, the call will silently target the cookie cart. Not blocking given the existing pattern; consider either dropping the parameter or honoring it in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/data/payment.ts` around lines 33 - 57, The exported function updateCheckoutPaymentSession currently ignores the cartId parameter and always calls requireCartId(); change it to honor the passed cartId by using the provided cartId when non-empty and falling back to requireCartId() otherwise (e.g., const id = cartId || await requireCartId()); update references inside updateCheckoutPaymentSession (notably the call to getClient().carts.paymentSessions.update) to use that id; alternatively, if you prefer the existing cookie-only behavior, remove the cartId parameter from updateCheckoutPaymentSession and all callers—choose one approach and make the signature, implementation, and callers consistent.
🤖 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/components/checkout/PaymentSection.tsx`:
- Around line 150-160: The PATCH payload built when switching cards omits
external_data if cardId is null, which can leave a prior
stripe_payment_method_id attached; modify the updateCheckoutPaymentSession call
site (the branch using existingSessionId) so that when cardId is null you
include external_data: { stripe_payment_method_id: null } in the body (i.e.,
always send external_data, setting stripe_payment_method_id to cardId or to
null), referencing the existingSessionId, cardId, updateCheckoutPaymentSession
and createCheckoutPaymentSession usage to locate the code to change.
---
Nitpick comments:
In `@src/lib/data/payment.ts`:
- Around line 33-57: The exported function updateCheckoutPaymentSession
currently ignores the cartId parameter and always calls requireCartId(); change
it to honor the passed cartId by using the provided cartId when non-empty and
falling back to requireCartId() otherwise (e.g., const id = cartId || await
requireCartId()); update references inside updateCheckoutPaymentSession (notably
the call to getClient().carts.paymentSessions.update) to use that id;
alternatively, if you prefer the existing cookie-only behavior, remove the
cartId parameter from updateCheckoutPaymentSession and all callers—choose one
approach and make the signature, implementation, and callers consistent.
🪄 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: c8e1de4a-1a5a-4609-9711-a86683b98e0c
📒 Files selected for processing (3)
src/components/checkout/PaymentSection.tsxsrc/lib/data/__tests__/payment.test.tssrc/lib/data/payment.ts
Previously, switching from a saved card back to "add new payment method" left the prior stripe_payment_method_id attached to the Spree payment session. Stripe Elements overrides it at confirm time so no mischarge occurred, but session metadata went stale. Now updateCheckoutPaymentSession accepts `stripePaymentMethodId: string | null`, and PaymentSection always syncs the current cardId (including null) on PATCH, explicitly clearing the attached payment method when needed.
…cted-from-payment-when-applied Resolved conflicts in src/lib/data/payment.ts and src/components/checkout/PaymentSection.tsx by taking main's payment-provider abstraction (Stripe/Adyen/PayPal/manual) as the base and re-applying the gift-card fix on top: - updateCheckoutPaymentSession adapted to main's generic externalData shape - PaymentSection watches `cart.amount_due ?? cart.total` instead of `cart.total`, so the session is re-synced when a gift card reduces the amount due (fixes the bug for all three gateways, not just Stripe) - Stripe-only PATCH path inside createSession: when a session already exists, PATCH amount + stripe_payment_method_id on the same Spree session — keeps the same Stripe PaymentIntent and client_secret, so the form does not remount on every gift-card / shipping change - Adyen/PayPal still recreate the session on amount change (their drop-in / PayPal order is bound to the initial amount) - handleMethodSelect now also clears paymentSessionIdRef when switching to a direct/manual method
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/components/checkout/PaymentSection.tsx`:
- Around line 160-162: The current canPatch logic can PATCH any existing session
id regardless of which gateway created it; change it to only allow PATCH when
the existing session was created for the same payment method/gateway as the
currently selected one: store the session's originating payment method/gateway
in a ref alongside paymentSessionIdRef (e.g., paymentSessionMethodRef or
paymentGatewayRef) when createSession receives a new session, update all
canPatch checks to verify both paymentSessionIdRef and that
paymentSessionMethodRef === currentPaymentMethodId/currentGateway, and only call
updateCheckoutPaymentSession when that equality holds; otherwise treat it as no
valid session and call createSession to make a new session.
- Around line 208-214: The PATCH failure branch currently leaves the old Stripe
session live; immediately clear the stale session when canPatch is false by
calling setSessionExternalData(null) and setPaymentSessionId(null) and nulling
paymentSessionIdRef.current and gatewayHandleRef.current so the previous
client_secret cannot be re-confirmed; apply this same clearing logic in both
places where the PATCH path is handled (the block using canPatch around
setSessionExternalData/setPaymentSessionId and the other analogous block around
lines 277-283) to ensure no outdated payment session or gateway handle remains
after a failed resize.
🪄 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: a6ec706c-f09c-43c0-a9ec-f8adc8f7f26c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/components/checkout/PaymentSection.tsxsrc/lib/data/__tests__/payment.test.tssrc/lib/data/payment.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/data/payment.ts
- src/lib/data/tests/payment.test.ts
Drops the Stripe-only PATCH path (and the helper updateCheckoutPaymentSession) introduced earlier in this branch. PaymentSection now always calls createCheckoutPaymentSession on every relevant change — gift card / shipping / saved-card switch / payment-method switch — so the gateway charges exactly `cart.amount_due ?? cart.total`. Reason: per project guideline, mutating an existing PaymentSession in place risks the user being charged the wrong amount (and gateway-specific shortcuts drift from each other). Uniform recreate is safer than the pi_… reuse it replaces. Kept: `paymentTarget = cart.amount_due ?? cart.total` watcher — the actual gift-card fix, applied uniformly to Stripe / Adyen / PayPal.
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/components/checkout/PaymentSection.tsx (1)
372-377:⚠️ Potential issue | 🟠 MajorPrevent duplicate session creation on method switch by syncing
lastPaymentTargetRefin the non-init path.When switching back to a session-based method (Line 377),
lastPaymentTargetRefcan still hold an old value. Then the effect at Line 305 may immediately fire and callcreateSessionagain for the same target, causing redundant backend session creation.🔧 Minimal fix
} else { + lastPaymentTargetRef.current = paymentTarget; createSession(selectedCardRef.current, newMethod); }Based on learnings: "Ensure components in checkout flows guard auto-save or backend calls with a processing state ... to prevent redundant or erroneous backend calls ...".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/checkout/PaymentSection.tsx` around lines 372 - 377, The else branch that calls createSession(selectedCardRef.current, newMethod) should first sync lastPaymentTargetRef to avoid triggering the effect that re-creates the same session; update the non-init path to set lastPaymentTargetRef.current = selectedCardRef.current (or the equivalent payment target) before calling createSession, referencing lastPaymentTargetRef, selectedCardRef, and createSession so the immediate effect check at Line ~305 sees the current target and does not create a duplicate session.
🤖 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/components/checkout/PaymentSection.tsx`:
- Around line 372-377: The else branch that calls
createSession(selectedCardRef.current, newMethod) should first sync
lastPaymentTargetRef to avoid triggering the effect that re-creates the same
session; update the non-init path to set lastPaymentTargetRef.current =
selectedCardRef.current (or the equivalent payment target) before calling
createSession, referencing lastPaymentTargetRef, selectedCardRef, and
createSession so the immediate effect check at Line ~305 sees the current target
and does not create a duplicate session.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aade4613-2858-4d64-85bd-046a96e0a74d
📒 Files selected for processing (1)
src/components/checkout/PaymentSection.tsx
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
2 participants
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.
Summary
cart.amount_due ?? cart.total(gift cards reduceamount_due, nottotal).updateCheckoutPaymentSessionserver action wrappingcarts.paymentSessions.update. Instead ofPOSTing a new payment session on every cart change and leaving staleIncompleteStripe PaymentIntents behind, we create once andPATCHthe existing session with an explicitamount.payment.test.ts(4 new cases: empty params, amount-only, amount + stripe payment method id, error path).Why
Customers were financially overcharged: the Stripe PaymentIntent was created/locked at the pre-gift-card total and not resized when the gift card was applied. Spree recorded both the full Stripe charge and the gift-card store-credit payment → Credit Owed.
Test plan
npx tsc --noEmitnpm run check(Biome)npx vitest run src/lib/data/__tests__/payment.test.ts(23/23 passing)app.vendo.dev: add $53.99 item → complete address → payment step → apply gift card (e.g.TEST9, $5). In DevTools Network: confirmPATCH /carts/:id/payment_sessions/:idfires with{ "amount": "48.99" }in the request body. In Stripe dashboard: the PaymentIntentamountshould reflect48.99after the PATCH.amount_duebecomes "0.00"). If backend 422s on zero-amount PATCH, surface as follow-up ticket — this PR does not change the $0 flow.Summary by CodeRabbit