(SP: 3) [Backend] Harden Stripe webhook processing and stabilize tests (dedupe/idempotency, raw-body signature path, cleanup + small janitor/admin nitpicks)#123
Conversation
…nup, mock hoisting, PI id validation) and apply small admin/janitor nitpicks
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. |
📝 WalkthroughWalkthroughEnhanced payment validation and refund handling across admin products, API routes, and Stripe webhook processing. Added input validation for Stripe operations, hardened error type guards, introduced conditional restocking logic, and improved test infrastructure with dynamic imports and comprehensive webhook scenario coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Webhook as Stripe Webhook
participant Handler as Webhook Handler
participant DB as Database
participant Restock as Restock Service
participant PSP as PSP/Stripe
Webhook->>Handler: Receive payment_intent.succeeded
Handler->>DB: Fetch order (with stockRestored, inventoryStatus)
DB-->>Handler: Return order
Handler->>Handler: Check shouldRestockFromWebhook(order)
alt Should Restock (stockRestored=false, inventoryStatus≠'released')
Handler->>Restock: Trigger restock
Restock-->>Handler: Success
Handler->>DB: Update order (stockRestored=true)
else Should Not Restock
Handler->>Handler: Skip restock (gate prevents call)
end
Handler-->>Webhook: Complete
Webhook->>Handler: Receive charge.refund.updated
Handler->>DB: Fetch order
DB-->>Handler: Return order
Handler->>PSP: retrieveCharge(chargeId) - get refunds.data
PSP-->>Handler: Charge with refund details
Handler->>Handler: Calculate cumulativeRefunded or fullness
alt Full Refund Detected
Handler->>Handler: Check shouldRestockFromWebhook(order)
alt Should Restock
Handler->>Restock: Trigger restock
Restock-->>Handler: Success
end
Handler->>DB: Update order (full refund state, stockRestored=true)
else Partial Refund
Handler->>DB: Mark PARTIAL_REFUND_IGNORED, keep state
end
Handler-->>Webhook: Complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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
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
🤖 Fix all issues with AI agents
In @frontend/lib/tests/stripe-webhook-paid-status-repair.test.ts:
- Around line 113-123: The afterEach currently only calls cleanupByIds when both
lastOrderId and lastEventId are truthy, leaving orphaned records if only one was
set; update the afterEach to always attempt cleanup by calling cleanupByIds if
either lastOrderId or lastEventId is truthy (or invoke separate cleanup calls
for order and event when each ID is present), ensuring you reference and clear
lastOrderId and lastEventId after awaiting cleanup so no test-state leaks
remain; modify the afterEach closure that references lastOrderId, lastEventId,
and cleanupByIds accordingly.
🧹 Nitpick comments (1)
frontend/app/api/shop/webhooks/stripe/route.ts (1)
662-664: ThrowingREFUND_FULLNESS_UNDETERMINEDreturns 500 and triggers retry — verify this is intentional.When
amtorcumulativeRefundedcannot be determined, the code throwsREFUND_FULLNESS_UNDETERMINED, which bubbles up to the catch block at line 802-804 and returns a 500 error. This triggers Stripe to retry the webhook.This is a deliberate design choice (fail-safe over fail-silent), which is good for data integrity. However, if Stripe consistently sends malformed events, this could cause repeated 500s and eventual webhook disabling by Stripe.
Consider logging a specific warning before throwing to aid debugging:
💡 Optional: Add structured logging before throwing
if (amt == null || cumulativeRefunded == null) { + console.warn('stripe_webhook_refund_fullness_undetermined', { + eventId: event.id, + chargeAmount: amt, + cumulativeRefunded, + eventType, + }); throw new Error('REFUND_FULLNESS_UNDETERMINED'); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/app/[locale]/shop/admin/products/page.tsxfrontend/app/api/shop/admin/products/[id]/route.tsfrontend/app/api/shop/admin/products/route.tsfrontend/app/api/shop/webhooks/stripe/route.tsfrontend/lib/psp/stripe.tsfrontend/lib/tests/checkout-concurrency-stock1.test.tsfrontend/lib/tests/order-items-snapshot-immutable.test.tsfrontend/lib/tests/stripe-webhook-paid-status-repair.test.tsfrontend/lib/tests/stripe-webhook-refund-full.test.tsfrontend/scripts/shop-janitor-restock-stale.mjs
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/app/[locale]/shop/admin/products/page.tsx (1)
frontend/lib/shop/money.ts (1)
fromDbMoney(51-56)
frontend/app/api/shop/webhooks/stripe/route.ts (2)
frontend/db/schema/shop.ts (1)
orders(107-198)frontend/lib/services/orders.ts (2)
restockOrder(1039-1197)restockOrder(1640-1640)
🔇 Additional comments (18)
frontend/app/api/shop/admin/products/route.ts (1)
26-31: LGTM: Improved type guard with explicit validation.The explicit null and type checks are safer than the previous truthy coercion (
!!value). This guards against edge cases wherevaluemight be a primitive (string, number, boolean) that would pass a truthy check but fail when accessing.code.frontend/app/api/shop/admin/products/[id]/route.ts (1)
32-37: LGTM: Consistent type guard improvement.Matches the safer validation pattern from the sibling route file. Good consistency across the codebase.
frontend/scripts/shop-janitor-restock-stale.mjs (1)
22-46: LGTM: Robust timeout validation.The strict digit-only regex and safe integer validation appropriately harden the input handling. The distinct warning messages for different failure modes improve debuggability.
frontend/lib/tests/stripe-webhook-paid-status-repair.test.ts (1)
60-62: LGTM: Better test isolation with stubEnv.Using
vi.stubEnvinstead of directly mutatingprocess.envis the correct approach for test environment variable handling. The correspondingvi.unstubAllEnvs()in afterEach ensures proper cleanup.frontend/app/[locale]/shop/admin/products/page.tsx (1)
15-33: LGTM: Enhanced observability with context-aware logging.The additional context (productId, currency, valueType) in the warning message provides excellent debugging information when money conversion fails. The early return for null values is cleaner. All call sites have been properly updated—there is one call site at line 123 that correctly passes the new context parameter.
frontend/lib/tests/checkout-concurrency-stock1.test.ts (1)
2-2: LGTM! Dynamic import pattern correctly ensures mock hoisting.The dynamic import of the checkout route after mock installation is the correct approach for vitest. This ensures
vi.mockcalls are hoisted and in effect before the module under test is loaded, preventing flaky behavior from static imports racing with mock setup.Also applies to: 16-17, 124-126
frontend/lib/psp/stripe.ts (2)
82-84: LGTM! Defensive input validation prevents invalid Stripe API calls.The early validation for empty
paymentIntentIdis a good safeguard that fails fast with a clear error message rather than making an API call that would fail anyway.
104-106: LGTM! Consistent validation pattern for charge retrieval.Same defensive validation applied to
retrieveCharge, maintaining consistency across Stripe utility functions.frontend/lib/tests/order-items-snapshot-immutable.test.ts (2)
123-125: LGTM! Dynamic import ensures mocks are in place.Consistent with the pattern used in
checkout-concurrency-stock1.test.ts.
140-141: LGTM! Improved error handling ensures cleanup failures are surfaced.The
primaryError/cleanupErrorpattern ensures:
- Primary test failures are propagated immediately (line 216)
- Cleanup always runs in
finally- Silent cleanup failures are surfaced when the primary test passes (lines 225-227)
This prevents flaky test state from accumulating due to hidden cleanup issues.
Also applies to: 214-227
frontend/app/api/shop/webhooks/stripe/route.ts (3)
115-124: LGTM! Webhook-level restock gate prevents redundant calls.The
shouldRestockFromWebhookhelper correctly implements an early-exit check that mirrors the guards inrestockOrder(per the relevant code snippet fromfrontend/lib/services/orders.ts). This avoids unnecessary work and potential race conditions from multiple restock attempts.
324-325: LGTM! Required fields for restock gating are now fetched.The order select now includes
stockRestoredandinventoryStatus, which are required for theshouldRestockFromWebhookdecision.
541-543: LGTM! Restock calls are now consistently gated.All restock invocations (
payment_failed,payment_canceled,charge.refunded/charge.refund.updated) now use theshouldRestockFromWebhookguard, ensuring idempotent behavior and avoiding redundant restocking when inventory has already been released.Also applies to: 592-594, 781-783
frontend/lib/tests/stripe-webhook-refund-full.test.ts (5)
9-19: LGTM! Mock setup correctly exposes both Stripe functions.The mock now properly exposes both
verifyWebhookSignatureandretrieveCharge, enabling comprehensive testing of webhook flows that require charge retrieval.
86-118: LGTM!makeChargehelper provides flexible test data construction.The helper cleanly constructs
Stripe.Chargeobjects with configurable refunds, enabling tests to cover various refund scenarios (full, partial, multiple refunds). Theas unknown as Stripe.Chargecast is acceptable for test data.
312-378: Excellent coverage for retry-after-failure scenario.This test validates critical behavior: when
restockOrderfails (500 returned), the event'sprocessedAtremains NULL, allowing Stripe's retry to reprocess successfully. This ensures restock is not lost due to transient failures.
445-494: LGTM! Edge case coverage for missingamount_refunded.This test validates the fallback logic in the webhook route (lines 640-660) where
amount_refundedis missing and the cumulative must be computed from the refunds list. The explicitdelete (charge as any).amount_refundedsimulates the edge case effectively.
44-60: LGTM! Typed row object improves type safety.Using
typeof orders.$inferInsertfor the row definition ensures the test data matches the schema, reducing the risk of test failures due to schema drift.
| // Fallback: sum refunds list if amount_refunded is missing | ||
| if ( | ||
| cumulativeRefunded == null && | ||
| Array.isArray((effectiveCharge as any)?.refunds?.data) | ||
| ) { | ||
| const list = (effectiveCharge as any).refunds.data as any[]; | ||
| const sumFromList = list.reduce((sum, r) => { | ||
| const a = typeof r?.amount === 'number' ? r.amount : 0; | ||
| return sum + a; | ||
| }, 0); | ||
|
|
||
| const currentAmt = | ||
| typeof (refund as any)?.amount === 'number' | ||
| ? (refund as any).amount | ||
| : 0; | ||
|
|
||
| const hasCurrent = | ||
| refund?.id && list.some(r => r?.id && r.id === refund.id); | ||
|
|
||
| cumulativeRefunded = sumFromList + (hasCurrent ? 0 : currentAmt); | ||
| } | ||
|
|
||
| if (amt == null || cumulativeRefunded == null) { | ||
| throw new Error('REFUND_FULLNESS_UNDETERMINED'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files frontend/app/api/shop/webhooks/stripe/ | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 117
🏁 Script executed:
wc -l frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 121
🏁 Script executed:
# Search for the charge.refunded event handler around line 609
sed -n '600,620p' frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 682
🏁 Script executed:
# Also search for the full refund determination logic
sed -n '600,680p' frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 2961
🏁 Script executed:
# Search for refundObject assignment/definition
sed -n '1,610p' frontend/app/api/shop/webhooks/stripe/route.ts | grep -n "refundObject" | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 454
🏁 Script executed:
# Get more context around where refundObject is used
sed -n '570,610p' frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1114
🏁 Script executed:
# Get context around refundObject initialization
sed -n '160,190p' frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 963
🏁 Script executed:
# Check the event type and object type logic
sed -n '140,180p' frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1425
🏁 Script executed:
# Check if there's any refund amount data available in charge.refunded event
sed -n '609,675p' frontend/app/api/shop/webhooks/stripe/route.ts | head -80Repository: DevLoversTeam/devlovers.net
Length of output: 2663
🏁 Script executed:
# Search for any comments or documentation about Stripe charge.refunded event structure
grep -n "charge.refunded" frontend/app/api/shop/webhooks/stripe/route.ts | head -20Repository: DevLoversTeam/devlovers.net
Length of output: 223
🏁 Script executed:
# Check if there's any information about refund amount in the charge object itself during charge.refunded
sed -n '600,650p' frontend/app/api/shop/webhooks/stripe/route.ts | grep -E "(amount|refund)" -A 2 -B 2Repository: DevLoversTeam/devlovers.net
Length of output: 1712
🏁 Script executed:
# Look for how charge.refunded is documented or any comments about Stripe API structure
grep -n "amount_refunded\|refunds.data" frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 687
🏁 Script executed:
# Check Stripe API documentation comments or any related notes in the codebase
grep -n "Stripe\|refund\|amount_refunded" frontend/app/api/shop/webhooks/stripe/route.ts | grep -i "comment\|note\|todo\|bug\|fixme" -A 2 -B 2Repository: DevLoversTeam/devlovers.net
Length of output: 53
🏁 Script executed:
# Look at the entire charge.refunded block to see if there's any other way amount_refunded or refund data could be obtained
sed -n '627,675p' frontend/app/api/shop/webhooks/stripe/route.tsRepository: DevLoversTeam/devlovers.net
Length of output: 1994
Clarify handling when refunds list is unavailable in charge.refunded events.
The fallback logic at lines 640-664 correctly sums charge.refunds.data when amount_refunded is missing. However, the edge case the reviewer identified is valid: if charge.refunds.data is empty AND refund is null (because charge?.refunds?.data?.[0] returns undefined), then cumulativeRefunded becomes 0, not null. Since the error check at line 663 only throws when cumulativeRefunded == null, it proceeds with isFullRefund = (0 === amt), which will be false even if a full refund just occurred.
This can happen due to API timing: the charge.refunded webhook event fires before the charge object's refunds.data array is populated. The current logic treats this as "no refunds recorded" (cumulativeRefunded = 0) rather than "refund status undetermined."
Consider either:
- Throwing
REFUND_FULLNESS_UNDETERMINEDwhen the refunds list is empty and refund data cannot be extracted from the event, or - Adding a check that validates whether
currentAmtcould actually be determined from the event before relying on the fallback calculation.
| afterEach(async () => { | ||
| if (!lastOrderId || !lastEventId) return; | ||
| await cleanupByIds({ orderId: lastOrderId, eventId: lastEventId }); | ||
| // restore env stubs to avoid cross-test coupling | ||
| vi.unstubAllEnvs(); | ||
|
|
||
| if (lastOrderId && lastEventId) { | ||
| await cleanupByIds({ orderId: lastOrderId, eventId: lastEventId }); | ||
| } | ||
|
|
||
| lastOrderId = null; | ||
| lastEventId = null; | ||
| }); |
There was a problem hiding this comment.
Potential test cleanup gap with conditional logic.
The cleanup now requires both lastOrderId AND lastEventId to be truthy (line 117). If a test fails after setting only one ID, cleanup won't execute, potentially leaving orphaned records in the database.
🧹 Proposed fix to ensure cleanup always runs
afterEach(async () => {
// restore env stubs to avoid cross-test coupling
vi.unstubAllEnvs();
- if (lastOrderId && lastEventId) {
+ if (lastOrderId || lastEventId) {
await cleanupByIds({ orderId: lastOrderId, eventId: lastEventId });
}
lastOrderId = null;
lastEventId = null;
});Alternatively, clean up each resource independently:
afterEach(async () => {
vi.unstubAllEnvs();
- if (lastOrderId && lastEventId) {
- await cleanupByIds({ orderId: lastOrderId, eventId: lastEventId });
- }
+ if (lastEventId) {
+ try {
+ await db.delete(stripeEvents).where(eq(stripeEvents.eventId, lastEventId));
+ } catch (e) {
+ console.error('[test cleanup]', e);
+ }
+ }
+ if (lastOrderId) {
+ try {
+ await db.delete(orders).where(eq(orders.id, lastOrderId));
+ } catch (e) {
+ console.error('[test cleanup]', e);
+ }
+ }
lastOrderId = null;
lastEventId = null;
});🤖 Prompt for AI Agents
In @frontend/lib/tests/stripe-webhook-paid-status-repair.test.ts around lines
113 - 123, The afterEach currently only calls cleanupByIds when both lastOrderId
and lastEventId are truthy, leaving orphaned records if only one was set; update
the afterEach to always attempt cleanup by calling cleanupByIds if either
lastOrderId or lastEventId is truthy (or invoke separate cleanup calls for order
and event when each ID is present), ensuring you reference and clear lastOrderId
and lastEventId after awaiting cleanup so no test-state leaks remain; modify the
afterEach closure that references lastOrderId, lastEventId, and cleanupByIds
accordingly.
2 participants
Description
This PR hardens Stripe webhook processing and stabilizes the associated test suite after recent shop payment/ledger changes. It focuses on correctness and idempotency in webhook handling (paid/refund flows), plus small follow-up fixes in admin products and the janitor runner to reduce operational risk.
Related Issue
Issue: #<issue_number>
Changes
Database Changes (if applicable)
How Has This Been Tested?
Commands
npx vitest run(23 files / 56 tests passed)npm run build(Next.js build succeeded)Screenshots (if applicable)
N/A (UI changes are minor / non-visual or not meaningful to capture)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.