(SP: 3) [Checkout] Money no-float + DB-canonical Stripe init + durable payment_attempts (bounded retries)#151
Merged
Merged
Conversation
…erge canonical cart lines
…→minor parser + strict minor-unit invariants
…ipe PaymentIntent creation (remove in-memory totals)
… bounded retries, audit trail)
liudmylasovetovs
requested review from
AM1007 and
ViktorSvertoka
as code owners
✅ Deploy Preview for develop-devlovers ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
📝 WalkthroughWalkthroughIntroduces durable, auditable Stripe payment-attempt tracking (DB table, migration, service layer) and replaces direct PaymentIntent lifecycle with ensureStripePaymentIntentForOrder across checkout API and pages. Adds webhook finalization calls, locale-aware checkout routing and encoded orderId in frontend, variant sanitization/aggregation in cart rehydrate, and stricter money parsing/validation. Changes
Sequence DiagramsequenceDiagram
actor Client
participant StripeClient as StripePaymentClient
participant PaymentPage as Payment Page
participant CheckoutAPI as Checkout API
participant AttemptSvc as PaymentAttempt Service
participant StripeAPI as Stripe API
participant Webhook as Stripe Webhook Handler
participant DB as Database
Client->>StripeClient: start payment (locale, orderId)
StripeClient->>PaymentPage: navigate with encoded orderId & shopBase
PaymentPage->>CheckoutAPI: request payment intent (orderId)
CheckoutAPI->>AttemptSvc: ensureStripePaymentIntentForOrder(orderId)
AttemptSvc->>DB: getActiveAttempt(orderId,'stripe')
DB-->>AttemptSvc: attempt or null
alt existing active attempt
AttemptSvc->>StripeAPI: retrieve payment intent
StripeAPI-->>AttemptSvc: clientSecret, status, piId
else create/backfill attempt
AttemptSvc->>DB: createActiveAttempt(...)
DB-->>AttemptSvc: attempt record
AttemptSvc->>StripeAPI: create payment intent
StripeAPI-->>AttemptSvc: clientSecret, piId
AttemptSvc->>DB: link attempt -> piId
end
AttemptSvc-->>CheckoutAPI: {clientSecret, paymentIntentId, attemptId}
CheckoutAPI-->>PaymentPage: clientSecret
PaymentPage->>StripeClient: render form with clientSecret
Client->>StripeAPI: confirm payment
StripeAPI->>Webhook: emit payment event
Webhook->>AttemptSvc: markStripeAttemptFinal(piId, status, error?)
AttemptSvc->>DB: update attempt status & finalizedAt
DB-->>AttemptSvc: ok
AttemptSvc-->>Webhook: ack
Webhook-->>StripeAPI: 200
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 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
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/lib/validation/shop.ts (1)
260-305: Prices schema validation conflicts with PATCH semantics.The
productAdminUpdateSchemarequires USD when any prices are provided (.min(1).optional()with USD enforcement insuperRefine). However,updateProducthandles prices via UPSERT—it merges with existing prices rather than replacing them. This means partial updates (e.g., updating only EUR) will fail validation even though the handler would process them correctly.Either merge existing prices into the payload before validation, move the USD + non-empty check to the service layer, or clarify if prices must always be replace-all (breaking PATCH semantics).
🤖 Fix all issues with AI agents
In `@frontend/lib/services/orders/payment-attempts.ts`:
- Around line 108-159: The query in upsertBackfillAttemptForExistingPI currently
returns a payment attempt by provider+providerPaymentIntentId but may belong to
a different order; before returning found[0], verify found[0].orderId ===
orderId and only reuse it when it matches the passed orderId, otherwise ignore
the found row and proceed with computing next attempt/insert (and keep the
existing conflict handling via getActiveAttempt). Locate the check after the
db.select(...) result and add this equality guard referencing paymentAttempts
and upsertBackfillAttemptForExistingPI so you don't prematurely return a
cross-order attempt.
🧹 Nitpick comments (4)
frontend/lib/services/orders/payment-intent.ts (1)
93-129: Verify payable‑state gating and avoidanyerror mutation.
readStripePaymentIntentParamsonly checks provider + totalAmountMinor; please confirm callers block paid/failed orders (or add a status guard here). Also, the diagnostics can be set via theOrderStateInvalidErroroptions to avoid(err as any)mutation.♻️ Typed diagnostics without `any`
- const err = new OrderStateInvalidError( - 'Invalid order total for Stripe payment intent creation.' - ); - - // attach diagnostics for API handler (keeps existing errorResponse shape) - (err as any).orderId = orderId; - (err as any).field = 'totalAmountMinor'; - (err as any).rawValue = amountMinor; - (err as any).details = { - reason: 'Invalid order total for Stripe payment intent creation.', - }; + const err = new OrderStateInvalidError( + 'Invalid order total for Stripe payment intent creation.', + { + orderId, + field: 'totalAmountMinor', + rawValue: amountMinor, + details: { + reason: 'Invalid order total for Stripe payment intent creation.', + }, + } + );frontend/app/[locale]/shop/checkout/payment/[orderId]/page.tsx (1)
168-212: Remove commented-out dead code.This large block of commented code adds noise and reduces maintainability. Since the new
ensureStripePaymentIntentForOrderflow is implemented below, this old logic should be deleted rather than kept as comments.🧹 Remove dead code
- // if ( - // paymentsEnabled && - // publishableKey && - // (!clientSecret || !clientSecret.trim()) - // ) { - // const existingPi = order.paymentIntentId?.trim() ?? ''; - // let phase: - // | 'retrievePaymentIntent' - // | 'createPaymentIntent' - // | 'setOrderPaymentIntent' - // | 'unknown' = 'unknown'; - - // try { - // if (existingPi) { - // phase = 'retrievePaymentIntent'; - // const retrieved = await retrievePaymentIntent(existingPi); - // clientSecret = retrieved.clientSecret; - // } else { - // phase = 'createPaymentIntent'; - // const snapshot = await readStripePaymentIntentParams(order.id); - // const created = await createPaymentIntent({ - // amount: snapshot.amountMinor, - // currency: snapshot.currency, - // orderId: order.id, - // idempotencyKey: `pi:${order.id}`, - // }); - - // phase = 'setOrderPaymentIntent'; - // await setOrderPaymentIntent({ - // orderId: order.id, - // paymentIntentId: created.paymentIntentId, - // }); - - // clientSecret = created.clientSecret; - // } - // } catch (error) { - // logError('payment_page_failed', error, { - // orderId: order.id, - // existingPi, - // phase, - // }); - - // // Leave clientSecret empty -> UI shows "Payment cannot be initialized" - // } - // } -frontend/app/api/shop/checkout/route.ts (1)
295-303: Consider inlining the ensureStripePI helper.This helper is a thin wrapper that doesn't add meaningful abstraction—it just forwards parameters to
ensureStripePaymentIntentForOrder. Consider inlining it at the two call sites to reduce indirection.♻️ Inline the helper
- async function ensureStripePI( - orderId: string, - existingPaymentIntentId: string | null - ) { - return await ensureStripePaymentIntentForOrder({ - orderId, - existingPaymentIntentId, - }); - }Then replace calls like:
- const ensured = await ensureStripePI( - order.id, - order.paymentIntentId ?? null - ); + const ensured = await ensureStripePaymentIntentForOrder({ + orderId: order.id, + existingPaymentIntentId: order.paymentIntentId ?? null, + });frontend/drizzle/0001_add_payment_attempts.sql (1)
4-16: Add a provider CHECK/enum to prevent invalid values.
provideris free-text in the DB while TS restricts to'stripe'. A DB-level constraint avoids accidental bad data from manual SQL or future code paths.♻️ Suggested migration tweak
CREATE TABLE "payment_attempts" ( "id" uuid PRIMARY KEY DEFAULT gen_random_uuid() NOT NULL, "order_id" uuid NOT NULL, "provider" text NOT NULL, "status" text DEFAULT 'active' NOT NULL, "attempt_number" integer NOT NULL, "idempotency_key" text NOT NULL, "provider_payment_intent_id" text, "last_error_code" text, "last_error_message" text, "metadata" jsonb DEFAULT '{}'::jsonb NOT NULL, "created_at" timestamp with time zone DEFAULT now() NOT NULL, "updated_at" timestamp with time zone DEFAULT now() NOT NULL, "finalized_at" timestamp with time zone, + CONSTRAINT "payment_attempts_provider_check" CHECK ("payment_attempts"."provider" in ('stripe')), CONSTRAINT "payment_attempts_status_check" CHECK ("payment_attempts"."status" in ('active','succeeded','failed','canceled')), CONSTRAINT "payment_attempts_attempt_number_check" CHECK ("payment_attempts"."attempt_number" >= 1) );
…oss-order PI guard, payable-state gating; cleanup dead code; add provider CHECK; inline ensureStripePI
ViktorSvertoka
approved these changes
Jan 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
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.
Description
This PR hardens the checkout/payment critical path by eliminating float-based money conversions, ensuring Stripe PaymentIntent creation uses DB-canonical totals/currency, and introducing a durable
payment_attemptslayer with strict uniqueness and bounded retries. The goal is to remove rounding risk, prevent infinite payment init loops, and provide an auditable retry trail.Related Issue
Issue: #<issue_number>
Changes
Number(...) * 100/Math.round(...)with string-safe parsing in the critical path.totalAmountMinor+currencyfrom the persisted order row immediately before the PSP call (DB is the canonical source).payment_attemptstable + service logic to enforce unique constraints, one active attempt per (order, provider), bounded retries, and an auditable attempt history.Database Changes (if applicable)
How Has This Been Tested?
Commands / checks
npx vitest run(31/31 files, 78/78 tests passing)payment_attemptstable exists and indexes/unique constraints are present in Neon (including partial unique forstatus='active').Screenshots (if applicable)
N/A (no UI changes)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.