Checkout fixes - #55
Conversation
+ non-session payment gateways (eg. check, bank transfer) + saved credit cards now are gateway agnostic
WalkthroughThis PR refactors the checkout payment system to support multiple payment methods with dynamic session handling, replacing the previous single-method (saved cards) approach. New gateway infrastructure enables method-agnostic payment processing, while address selection logic is improved to track saved vs. new address states independently. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PaymentStep
participant PaymentMethodSelector as Payment<br/>Method Selector
participant Gateway as Gateway<br/>Component
participant SessionAPI as Session API
participant PaymentAPI as Payment API
participant CheckoutPage
User->>PaymentStep: Select payment method
PaymentStep->>PaymentMethodSelector: Render method options
PaymentMethodSelector->>User: Display methods
User->>PaymentMethodSelector: Click method
PaymentMethodSelector->>PaymentStep: onSelect(methodId)
PaymentStep->>SessionAPI: createCheckoutPaymentSession(methodId)
SessionAPI-->>PaymentStep: PaymentSession | null
PaymentStep->>PaymentStep: resolveGatewayComponent(methodId)
PaymentStep->>Gateway: Render via Suspense
Gateway->>Gateway: Initialize (onReady)
Gateway-->>PaymentStep: Ready signal
User->>Gateway: Confirm payment
Gateway->>SessionAPI: Confirm session OR direct method
SessionAPI-->>Gateway: Result
Gateway->>PaymentStep: onPaymentComplete callback
PaymentStep->>CheckoutPage: Call onPaymentComplete(sessionId, methodId)
CheckoutPage->>PaymentAPI: createCheckoutPayment OR completeSession
PaymentAPI-->>CheckoutPage: Success
CheckoutPage->>User: Order confirmation
sequenceDiagram
participant PaymentStep as PaymentStep<br/>(SessionBased)
participant SimpleGateway as SimpleConfirmation<br/>Gateway
participant StripeGateway as StripeGateway
participant SessionAPI as Session API
Note over PaymentStep: Method: Non-Session
PaymentStep->>SimpleGateway: Render (no session needed)
SimpleGateway->>PaymentStep: onReady()
PaymentStep->>SimpleGateway: confirmPayment()
SimpleGateway-->>PaymentStep: {}<br/>(no error)
PaymentStep->>PaymentStep: onPaymentComplete(null, methodId)
Note over PaymentStep: Method: Stripe<br/>(Session-Based)
PaymentStep->>SessionAPI: createCheckoutPaymentSession(methodId)
SessionAPI-->>PaymentStep: PaymentSession{clientSecret}
PaymentStep->>StripeGateway: Render with session
StripeGateway->>StripeGateway: Initialize payment form
StripeGateway->>PaymentStep: onReady()
PaymentStep->>StripeGateway: confirmPayment(returnUrl)
StripeGateway->>SessionAPI: Confirm via Stripe
SessionAPI-->>StripeGateway: {error?}
StripeGateway-->>PaymentStep: Result
PaymentStep->>PaymentStep: onPaymentComplete(sessionId, methodId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/components/checkout/gateways/SimpleConfirmationGateway.tsx (1)
10-27: Prefer a named export for this gateway component.Everything else in
src/components/checkoutis using named component exports, so this default export makes the new gateway the odd one out.As per coding guidelines, "`src/components/**/*.{ts,tsx}: Prefer named exports for components instead of default exports`."♻️ Suggested update
-const SimpleConfirmationGateway = forwardRef< +export const SimpleConfirmationGateway = forwardRef< PaymentGatewayHandle, PaymentGatewayProps >(function SimpleConfirmationGateway({ onReady }, ref) { @@ -}); - -export default SimpleConfirmationGateway; +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/checkout/gateways/SimpleConfirmationGateway.tsx` around lines 10 - 27, The component currently uses a default export which violates the project's named-export guideline; change the export to a named export by exporting SimpleConfirmationGateway as a named symbol instead of default (e.g. export const SimpleConfirmationGateway = forwardRef<PaymentGatewayHandle, PaymentGatewayProps>(...) or keep the const and add export { SimpleConfirmationGateway };), update the final export statement to remove "export default SimpleConfirmationGateway", and ensure any imports across the codebase that consumed the default export are updated to the named import of SimpleConfirmationGateway.src/components/checkout/PaymentMethodSelector.tsx (1)
15-21: Add an explicit return type to this exported component.This new public component is relying on inference today. The repo TypeScript rule asks for an explicit function return type here.
As per coding guidelines, "
**/*.ts{,x}: Use strict TypeScript type checking; define explicit return types for functions and avoid 'any' type`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/checkout/PaymentMethodSelector.tsx` around lines 15 - 21, The exported component PaymentMethodSelector currently relies on inferred return type; add an explicit return type (e.g., : JSX.Element or React.ReactElement) to the function signature so the function is typed explicitly (keep using PaymentMethodSelectorProps for the params and avoid any). Update the declaration for PaymentMethodSelector to include the chosen return type and ensure any necessary React types are imported.package.json (1)
22-22: Move@testing-library/domtodevDependencies.This package is only used in tests. The rest of the Testing Library stack is already dev-only, so keeping this under
dependenciesunnecessarily adds a test package to production installs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 22, Remove "@testing-library/dom" from the dependencies section of package.json and add it under devDependencies with the same version string ("^10.4.1"); update package.json so "dependencies" no longer lists "@testing-library/dom" and "devDependencies" includes it, then reinstall (npm/yarn) to update lockfile.src/components/checkout/gateways/stripe/StripeGateway.tsx (1)
101-106: Potential duplicateonReadycalls for saved card selection.When selecting a saved card,
onReady()is called twice:
- In
switchSessionat line 86 whencardIdis truthy andsecretis set- In this
useEffectwhenselectedCardId && clientSecret && !loadingConsider removing the
onReady()call from this effect, asswitchSessionalready handles the saved card case, and the form flow useshandleFormReady.♻️ Proposed simplification
- // Signal ready if using initial session with no saved cards - useEffect(() => { - if (selectedCardId && clientSecret && !loading) { - onReady(); - } - }, [selectedCardId, clientSecret, loading, onReady]);The
switchSessioncallback (line 86) andhandleFormReadycallback (line 39) already cover all readiness signals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/checkout/gateways/stripe/StripeGateway.tsx` around lines 101 - 106, The useEffect that calls onReady() when selectedCardId && clientSecret && !loading causes duplicate ready signals for saved-card selection; remove the onReady() invocation from that useEffect and rely on switchSession (which already calls onReady when cardId/secret are set) and handleFormReady to signal readiness instead; keep the effect if it has other side-effects but ensure it no longer calls onReady(), and verify switchSession and handleFormReady cover initial-session-with-no-saved-cards and saved-card flows.
🤖 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/AddressStep.tsx`:
- Around line 55-69: The saved-address inference in the selectedSavedAddressId
state initializer can pick the wrong saved address because it only compares
address1, city, zipcode, and country_iso; update the initializer to first prefer
and return order.ship_address.id (or ship_address_id) when present, and only if
no explicit saved ID exists fall back to a stricter comparison using
addressToFormData(order.ship_address) against initialSavedAddresses that
includes address2, state/region, company, phone, and recipient names (or perform
a normalized deep-equality of the form data) before returning match?.id; ensure
this logic lives where selectedSavedAddressId is initialized so subsequent
submission (ship_address_id usage) uses the explicit saved ID when available.
In `@src/components/checkout/gateways/SavedCards.tsx`:
- Around line 33-63: The one-shot initRef prevents loadCards from running when
isAuthenticated changes; remove the initRef guard and change the effect to react
to isAuthenticated (e.g., useEffect(() => { if (isAuthenticated) { loadCards();
} else { setCards([]); setLoading(false); /* optionally call onSelect(null) */ }
}, [isAuthenticated, loadCards])). Ensure loadCards still uses isAuthenticated
in its guard or rely on the effect to call it only when authenticated, and clear
cards/state when auth becomes false so saved cards don't persist after logout.
- Around line 46-51: The auto-selection of a default card always calls onSelect
and overwrites the "add new card" flow used by callers that set selectedCardId
=== null (as in StripeGateway). Change the logic in SavedCards (the block using
gatewayCards and onSelect) to only auto-select when the parent has not
explicitly chosen the "new card" path — e.g., check the selectedCardId prop and
only call onSelect(defaultCard.gateway_payment_profile_id) if selectedCardId !==
null (allow undefined to auto-select, but skip when explicitly null), so callers
can preserve the null=new-card state.
In `@src/components/checkout/gateways/types.ts`:
- Around line 17-18: The type declares paymentSession as required
(paymentSession: PaymentSession) but some gateways (e.g., Check/Bank Transfer)
have no session and PaymentStep.tsx is using a non-null assertion when passing
paymentSession into SimpleConfirmationGateway; change the types to reflect
reality by making paymentSession optional/nullable (e.g., paymentSession?:
PaymentSession | null) or split into two interfaces (SessionGateway with
paymentSession: PaymentSession and NonSessionGateway without it) and update
usages in PaymentStep.tsx and any gateway components (SimpleConfirmationGateway)
to handle the optional/null case accordingly.
In `@src/components/checkout/PaymentStep.tsx`:
- Around line 383-400: The non-null assertion on paymentSession in the
non-session branch is wrong because method.session_required is false and
paymentSession is null; remove the assertion and make paymentSession optional in
the gateway props so SimpleConfirmationGateway (and other non-session gateways)
do not require it. Update the PaymentGatewayProps type in gateways/types.ts to
mark paymentSession as optional (e.g., paymentSession?: PaymentSession | null)
and then remove the `!` from paymentSession when rendering
SimpleConfirmationGateway (or pass undefined/null) so the prop matches the new
type.
---
Nitpick comments:
In `@package.json`:
- Line 22: Remove "@testing-library/dom" from the dependencies section of
package.json and add it under devDependencies with the same version string
("^10.4.1"); update package.json so "dependencies" no longer lists
"@testing-library/dom" and "devDependencies" includes it, then reinstall
(npm/yarn) to update lockfile.
In `@src/components/checkout/gateways/SimpleConfirmationGateway.tsx`:
- Around line 10-27: The component currently uses a default export which
violates the project's named-export guideline; change the export to a named
export by exporting SimpleConfirmationGateway as a named symbol instead of
default (e.g. export const SimpleConfirmationGateway =
forwardRef<PaymentGatewayHandle, PaymentGatewayProps>(...) or keep the const and
add export { SimpleConfirmationGateway };), update the final export statement to
remove "export default SimpleConfirmationGateway", and ensure any imports across
the codebase that consumed the default export are updated to the named import of
SimpleConfirmationGateway.
In `@src/components/checkout/gateways/stripe/StripeGateway.tsx`:
- Around line 101-106: The useEffect that calls onReady() when selectedCardId &&
clientSecret && !loading causes duplicate ready signals for saved-card
selection; remove the onReady() invocation from that useEffect and rely on
switchSession (which already calls onReady when cardId/secret are set) and
handleFormReady to signal readiness instead; keep the effect if it has other
side-effects but ensure it no longer calls onReady(), and verify switchSession
and handleFormReady cover initial-session-with-no-saved-cards and saved-card
flows.
In `@src/components/checkout/PaymentMethodSelector.tsx`:
- Around line 15-21: The exported component PaymentMethodSelector currently
relies on inferred return type; add an explicit return type (e.g., : JSX.Element
or React.ReactElement) to the function signature so the function is typed
explicitly (keep using PaymentMethodSelectorProps for the params and avoid any).
Update the declaration for PaymentMethodSelector to include the chosen return
type and ensure any necessary React types are imported.
🪄 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: 8050ec16-d6b1-4830-8e7a-9ba09144cc1d
⛔ 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/components/checkout/AddressSelector.tsxsrc/components/checkout/AddressStep.tsxsrc/components/checkout/DeliveryStep.tsxsrc/components/checkout/PaymentMethodSelector.tsxsrc/components/checkout/PaymentStep.tsxsrc/components/checkout/gateways/SavedCards.tsxsrc/components/checkout/gateways/SimpleConfirmationGateway.tsxsrc/components/checkout/gateways/registry.tssrc/components/checkout/gateways/stripe/StripeGateway.tsxsrc/components/checkout/gateways/stripe/StripePaymentForm.tsxsrc/components/checkout/gateways/types.tssrc/components/checkout/index.tssrc/lib/data/payment.ts
| const [selectedSavedAddressId, setSelectedSavedAddressId] = useState< | ||
| string | null | ||
| >(() => { | ||
| // Check if the current order address matches a saved address | ||
| if (initialSavedAddresses.length === 0 || !order.ship_address) return null; | ||
| const formData = addressToFormData(order.ship_address); | ||
| const match = initialSavedAddresses.find( | ||
| (addr) => | ||
| addr.address1 === formData.address1 && | ||
| addr.city === formData.city && | ||
| addr.zipcode === formData.zipcode && | ||
| addr.country_iso === formData.country_iso, | ||
| ); | ||
| return match?.id ?? null; | ||
| }); |
There was a problem hiding this comment.
This saved-address inference can pick the wrong address.
The initializer only matches address1, city, zipcode, and country_iso. Two saved addresses can share those fields but differ in address2, state, company, phone, or even recipient name, and Lines 100-103 will then submit the wrong ship_address_id. Prefer the saved-address ID when it exists, or fall back to a fully normalized address comparison.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/checkout/AddressStep.tsx` around lines 55 - 69, The
saved-address inference in the selectedSavedAddressId state initializer can pick
the wrong saved address because it only compares address1, city, zipcode, and
country_iso; update the initializer to first prefer and return
order.ship_address.id (or ship_address_id) when present, and only if no explicit
saved ID exists fall back to a stricter comparison using
addressToFormData(order.ship_address) against initialSavedAddresses that
includes address2, state/region, company, phone, and recipient names (or perform
a normalized deep-equality of the form data) before returning match?.id; ensure
this logic lives where selectedSavedAddressId is initialized so subsequent
submission (ship_address_id usage) uses the explicit saved ID when available.
| const initRef = useRef(false); | ||
|
|
||
| const loadCards = useCallback(async () => { | ||
| if (!isAuthenticated) return; | ||
|
|
||
| setLoading(true); | ||
| try { | ||
| const result = await getCreditCards(); | ||
| const gatewayCards = result.data.filter( | ||
| (card) => card.gateway_payment_profile_id, | ||
| ); | ||
| setCards(gatewayCards); | ||
|
|
||
| // Auto-select the default card on first load | ||
| if (gatewayCards.length > 0) { | ||
| const defaultCard = | ||
| gatewayCards.find((c) => c.default) || gatewayCards[0]; | ||
| onSelect(defaultCard.gateway_payment_profile_id); | ||
| } | ||
| } catch { | ||
| // Cards failed to load — proceed with new card flow | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [isAuthenticated, onSelect]); | ||
|
|
||
| useEffect(() => { | ||
| if (initRef.current) return; | ||
| initRef.current = true; | ||
| loadCards(); | ||
| }, [loadCards]); |
There was a problem hiding this comment.
The one-shot init guard makes saved cards drift from isAuthenticated.
If isAuthenticated is false on the first render and becomes true later, loadCards() never runs because initRef is already latched. The inverse transition is broken too: previously loaded cards stay in state and still render after auth becomes false. This makes card visibility depend on remounts instead of the actual auth state.
🔧 Suggested fix
- const initRef = useRef(false);
-
const loadCards = useCallback(async () => {
if (!isAuthenticated) return;
@@
- useEffect(() => {
- if (initRef.current) return;
- initRef.current = true;
- loadCards();
- }, [loadCards]);
+ useEffect(() => {
+ if (!isAuthenticated) {
+ setCards([]);
+ return;
+ }
+
+ void loadCards();
+ }, [isAuthenticated, loadCards]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/checkout/gateways/SavedCards.tsx` around lines 33 - 63, The
one-shot initRef prevents loadCards from running when isAuthenticated changes;
remove the initRef guard and change the effect to react to isAuthenticated
(e.g., useEffect(() => { if (isAuthenticated) { loadCards(); } else {
setCards([]); setLoading(false); /* optionally call onSelect(null) */ } },
[isAuthenticated, loadCards])). Ensure loadCards still uses isAuthenticated in
its guard or rely on the effect to call it only when authenticated, and clear
cards/state when auth becomes false so saved cards don't persist after logout.
| // Auto-select the default card on first load | ||
| if (gatewayCards.length > 0) { | ||
| const defaultCard = | ||
| gatewayCards.find((c) => c.default) || gatewayCards[0]; | ||
| onSelect(defaultCard.gateway_payment_profile_id); | ||
| } |
There was a problem hiding this comment.
Auto-selecting a saved card here breaks the controlled null = new card state.
src/components/checkout/gateways/stripe/StripeGateway.tsx uses selectedCardId === null as the explicit "add new payment method" path, but this block always calls onSelect(defaultCard.gateway_payment_profile_id) once cards load. That makes it impossible for a caller to keep the new-card flow selected and can recreate a session for the wrong saved card after remounts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/checkout/gateways/SavedCards.tsx` around lines 46 - 51, The
auto-selection of a default card always calls onSelect and overwrites the "add
new card" flow used by callers that set selectedCardId === null (as in
StripeGateway). Change the logic in SavedCards (the block using gatewayCards and
onSelect) to only auto-select when the parent has not explicitly chosen the "new
card" path — e.g., check the selectedCardId prop and only call
onSelect(defaultCard.gateway_payment_profile_id) if selectedCardId !== null
(allow undefined to auto-select, but skip when explicitly null), so callers can
preserve the null=new-card state.
| /** The created payment session (contains gateway-specific external_data) */ | ||
| paymentSession: PaymentSession; |
There was a problem hiding this comment.
Consider making paymentSession optional for non-session gateways.
The paymentSession property is required, but non-session payment methods (Check, Bank Transfer) don't have a session. In PaymentStep.tsx (line 391), SimpleConfirmationGateway receives paymentSession! (non-null assertion) when paymentSession is actually null.
This creates a type-runtime mismatch. Consider either:
- Making
paymentSessionoptional:paymentSession?: PaymentSession | null - Creating separate interfaces for session-based and non-session gateways
🛠️ Option 1: Make paymentSession optional
export interface PaymentGatewayProps {
/** The Spree payment method for this gateway */
paymentMethod: PaymentMethod;
/** The created payment session (contains gateway-specific external_data) */
- paymentSession: PaymentSession;
+ paymentSession?: PaymentSession | null;
/** The current order */
order: Order;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** The created payment session (contains gateway-specific external_data) */ | |
| paymentSession: PaymentSession; | |
| export interface PaymentGatewayProps { | |
| /** The Spree payment method for this gateway */ | |
| paymentMethod: PaymentMethod; | |
| /** The created payment session (contains gateway-specific external_data) */ | |
| paymentSession?: PaymentSession | null; | |
| /** The current order */ | |
| order: Order; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/checkout/gateways/types.ts` around lines 17 - 18, The type
declares paymentSession as required (paymentSession: PaymentSession) but some
gateways (e.g., Check/Bank Transfer) have no session and PaymentStep.tsx is
using a non-null assertion when passing paymentSession into
SimpleConfirmationGateway; change the types to reflect reality by making
paymentSession optional/nullable (e.g., paymentSession?: PaymentSession | null)
or split into two interfaces (SessionGateway with paymentSession: PaymentSession
and NonSessionGateway without it) and update usages in PaymentStep.tsx and any
gateway components (SimpleConfirmationGateway) to handle the optional/null case
accordingly.
| // Non-session gateway | ||
| if (!method.session_required) { | ||
| return ( | ||
| <> | ||
| {errorBanner} | ||
| <SimpleConfirmationGateway | ||
| ref={gatewayRef} | ||
| paymentMethod={method} | ||
| paymentSession={paymentSession!} | ||
| order={order} | ||
| isAuthenticated={isAuthenticated} | ||
| onReady={handleGatewayReady} | ||
| onError={handleGatewayError} | ||
| onCreateSession={handleCreateSession} | ||
| /> | ||
| </> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Non-null assertion on paymentSession for non-session methods is incorrect.
At line 391, paymentSession! asserts non-null, but for non-session methods (Check, Bank Transfer), paymentSession is always null (it's never set since method.session_required is false). This assertion masks a type error.
This relates to the type issue flagged in gateways/types.ts where paymentSession is required in PaymentGatewayProps. If SimpleConfirmationGateway doesn't use paymentSession, the cleanest fix is to update the types as suggested there.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/checkout/PaymentStep.tsx` around lines 383 - 400, The non-null
assertion on paymentSession in the non-session branch is wrong because
method.session_required is false and paymentSession is null; remove the
assertion and make paymentSession optional in the gateway props so
SimpleConfirmationGateway (and other non-session gateways) do not require it.
Update the PaymentGatewayProps type in gateways/types.ts to mark paymentSession
as optional (e.g., paymentSession?: PaymentSession | null) and then remove the
`!` from paymentSession when rendering SimpleConfirmationGateway (or pass
undefined/null) so the prop matches the new type.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Style
Chores