fix(payments): handle payment.failed webhook event#63
Merged
Conversation
Two business-layer spec gaps found in the implementation audit, both about CheckoutSession lifecycle completeness: payment.failed handling. The Source-based flow this codebase uses fires `payment.failed`, which fell through to the webhook route's default case and was dropped: the session stayed PENDING and the buyer was never told. Add handlePaymentFailed (marks the session FAILED, refuses to downgrade an already-PAID session) and a sendPaymentFailedEmail retry notice, wired into the webhook route. Closes the missing "Payment failed" email trigger. Checkout-expiry sweep. Abandoned sessions had no path out of PENDING/AWAITING_PAYMENT, violating the spec guarantee that every session ends in PAID, EXPIRED, FAILED, or ERROR. Add sweepExpiredCheckouts plus an hourly Vercel cron at /api/cron/expire-checkouts, guarded by CRON_SECRET (fails closed when unset). Covered by unit tests for both handlers and the cron auth guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012nKVRGvpD3ens22gb1p8CW
📝 WalkthroughWalkthroughAdds PayMongo payment-failure handling with checkout updates and email notifications, plus a scheduled, authenticated endpoint that expires stale checkout sessions hourly. ChangesCheckout lifecycle handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (7)
.env.example (1)
18-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove quotes around the placeholder value.
dotenv-linter flags quote characters on this line.
🔧 Proposed fix
-CRON_SECRET="generate-with-openssl-rand-base64-32" +CRON_SECRET=generate-with-openssl-rand-base64-32🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 18 - 21, Update the CRON_SECRET entry in the environment example to remove the surrounding quote characters while preserving the placeholder value and accompanying comments.Source: Linters/SAST tools
src/lib/enrollment.ts (2)
672-702: 🧹 Nitpick | 🔵 TrivialLGTM! One operational note:
sweepExpiredCheckoutsscans bystatusandexpiresAthourly; ifCheckoutSessiongrows large, an index on(status, expiresAt)would keep this query cheap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/enrollment.ts` around lines 672 - 702, Add a database index covering status and expiresAt for CheckoutSession to support the filtering performed by sweepExpiredCheckouts. Update the schema or migration definition for CheckoutSession, preserving the existing query behavior and soft-delete filtering.
595-601: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFire-and-forget email may never complete on serverless.
sendPaymentFailedEmail(...).catch(...)isn't awaited, and the caller (webhook/route.tsLine 138) awaits onlyhandlePaymentFailed, which returns as soon as the transaction commits. Once the webhook route sends its response, Vercel can terminate the invocation before this pending promise resolves.Once the function returns the response payload, it stops processing including any pending background tasks. Consider scheduling this with Next's
after()(stable since Next 15.1) orwaitUntil()so the send is guaranteed to run to completion instead of being silently dropped.♻️ Proposed fix using `after()`
+import { after } from 'next/server'; ... if (notify) { - // Best-effort — a failed email must not fail the webhook. - sendPaymentFailedEmail({ - to: notify.email, - studentName: notify.email.split('@')[0] ?? 'there', - tierName: notify.tierName, - }).catch((err: Error) => logger.error({ err }, 'payment.failed: retry email send failed')); + // Best-effort — scheduled via after() so it survives past the response. + after(() => + sendPaymentFailedEmail({ + to: notify.email, + studentName: notify.email.split('@')[0] ?? 'there', + tierName: notify.tierName, + }).catch((err: Error) => logger.error({ err }, 'payment.failed: retry email send failed')), + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/enrollment.ts` around lines 595 - 601, Update the payment-failed notification flow around sendPaymentFailedEmail and its caller handlePaymentFailed so the email work is registered with Next.js after() or waitUntil() instead of being left as an unawaited promise. Preserve the best-effort behavior and existing error logging while ensuring the scheduled task can complete after the webhook response is returned.src/lib/email.tsx (2)
717-724: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOuter wrapper uses Inter instead of the design system fonts.
Every other element in this template (brand label, heading, CTA) uses
Space Grotesk, but the outerdivfalls back toInter, system-ui, sans-serif. As per coding guidelines, "Use Space Grotesk and JetBrains Mono only in product UI. Do not use Inter or system fonts."🎨 Proposed fix
style={{ - fontFamily: 'Inter, system-ui, sans-serif', + fontFamily: 'Space Grotesk, sans-serif', backgroundColor: '`#F5F5F0`',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/email.tsx` around lines 717 - 724, Update the outer wrapper’s fontFamily in the email template to use the design-system font stack, matching the existing Space Grotesk usage in the brand label, heading, and CTA; remove Inter and system-font fallbacks while leaving the other wrapper styles unchanged.Source: Coding guidelines
696-696: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEm-dashes in user-facing email copy.
The subject line ("Your payment didn't go through — ${tierName}") and body text ("...try again below — if a payment method was declined...") both use em-dashes. As per coding guidelines, "do not use emojis in code or commit messages, and do not use em-dashes. Use periods, commas, or parentheses instead."
✏️ Proposed fix
- subject: `Your payment didn't go through — ${tierName}`, + subject: `Your payment didn't go through (${tierName})`,- Hi {studentName}, we couldn't complete your payment for {tierName}. - No charge was made. You can try again below — if a payment method was - declined, use a different one. + Hi {studentName}, we couldn't complete your payment for {tierName}. + No charge was made. You can try again below. If a payment method was + declined, use a different one.Also applies to: 753-753
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/email.tsx` at line 696, Replace the em-dashes in the user-facing payment email copy with permitted punctuation such as periods, commas, or parentheses. Update both the subject template near the payment failure copy and the body text near the declined payment guidance, preserving the existing wording and interpolation.Source: Coding guidelines
src/lib/__tests__/enrollment.test.ts (1)
29-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM! One coverage gap worth adding: no test exercises the
paymongoPaymentIdfallback lookup (whensourceIddoesn't match), only thepaymongoSourceIdpath is covered.Also applies to: 132-156, 609-696
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/__tests__/enrollment.test.ts` around lines 29 - 83, Add coverage in the enrollment handler tests for the fallback lookup using paymongoPaymentId when the provided sourceId does not match, alongside the existing paymongoSourceId scenarios. Update the relevant handleSourceChargeable or payment-event test setup and assertions to verify the fallback record is found and processed correctly.src/app/api/cron/expire-checkouts/route.ts (1)
24-38: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLGTM on the auth flow. Consider wrapping
sweepExpiredCheckouts()in a try/catch withlogger.error(mirroring the paymongo webhook route's error handling) so a DB failure during the hourly sweep is observable rather than surfacing only as a generic 500.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/api/cron/expire-checkouts/route.ts` around lines 24 - 38, Wrap the sweepExpiredCheckouts call in GET with try/catch, log caught database or sweep errors via logger.error using the cron-expire-checkouts component context, and return an appropriate JSON 500 response instead of allowing the failure to surface as an unhandled generic error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/enrollment.ts`:
- Around line 559-593: Replace the unconditional update in the payment.failed
flow with an atomic conditional updateMany, matching the pattern used by
sweepExpiredCheckouts: target checkout.id and require status not equal to
CheckoutStatus.PAID. Preserve the existing early PAID check and return the
enrollment details only when the conditional mutation succeeds; handle a
zero-row update without downgrading or revoking the paid session.
---
Nitpick comments:
In @.env.example:
- Around line 18-21: Update the CRON_SECRET entry in the environment example to
remove the surrounding quote characters while preserving the placeholder value
and accompanying comments.
In `@src/app/api/cron/expire-checkouts/route.ts`:
- Around line 24-38: Wrap the sweepExpiredCheckouts call in GET with try/catch,
log caught database or sweep errors via logger.error using the
cron-expire-checkouts component context, and return an appropriate JSON 500
response instead of allowing the failure to surface as an unhandled generic
error.
In `@src/lib/__tests__/enrollment.test.ts`:
- Around line 29-83: Add coverage in the enrollment handler tests for the
fallback lookup using paymongoPaymentId when the provided sourceId does not
match, alongside the existing paymongoSourceId scenarios. Update the relevant
handleSourceChargeable or payment-event test setup and assertions to verify the
fallback record is found and processed correctly.
In `@src/lib/email.tsx`:
- Around line 717-724: Update the outer wrapper’s fontFamily in the email
template to use the design-system font stack, matching the existing Space
Grotesk usage in the brand label, heading, and CTA; remove Inter and system-font
fallbacks while leaving the other wrapper styles unchanged.
- Line 696: Replace the em-dashes in the user-facing payment email copy with
permitted punctuation such as periods, commas, or parentheses. Update both the
subject template near the payment failure copy and the body text near the
declined payment guidance, preserving the existing wording and interpolation.
In `@src/lib/enrollment.ts`:
- Around line 672-702: Add a database index covering status and expiresAt for
CheckoutSession to support the filtering performed by sweepExpiredCheckouts.
Update the schema or migration definition for CheckoutSession, preserving the
existing query behavior and soft-delete filtering.
- Around line 595-601: Update the payment-failed notification flow around
sendPaymentFailedEmail and its caller handlePaymentFailed so the email work is
registered with Next.js after() or waitUntil() instead of being left as an
unawaited promise. Preserve the best-effort behavior and existing error logging
while ensuring the scheduled task can complete after the webhook response is
returned.
🪄 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 Plus
Run ID: 88e59330-7933-4c6b-98dc-09dc09b41cc6
📒 Files selected for processing (8)
.env.examplesrc/app/api/cron/expire-checkouts/route.test.tssrc/app/api/cron/expire-checkouts/route.tssrc/app/api/paymongo/webhook/route.tssrc/lib/__tests__/enrollment.test.tssrc/lib/email.tsxsrc/lib/enrollment.tsvercel.json
Comment on lines
+559
to
+593
| const checkout = | ||
| (sourceId | ||
| ? await tx.checkoutSession.findFirst({ | ||
| where: { paymongoSourceId: sourceId, deletedAt: null }, | ||
| select: { id: true, email: true, status: true, pricingTier: { select: { name: true } } }, | ||
| }) | ||
| : null) ?? | ||
| (await tx.checkoutSession.findFirst({ | ||
| where: { paymongoPaymentId: paymentIdPm, deletedAt: null }, | ||
| select: { id: true, email: true, status: true, pricingTier: { select: { name: true } } }, | ||
| })); | ||
|
|
||
| if (!checkout) { | ||
| logger.warn({ paymentId: paymentIdPm, sourceId }, 'payment.failed: no checkout session found'); | ||
| return null; | ||
| } | ||
|
|
||
| // Never downgrade a session that already succeeded — a late/duplicate | ||
| // failure event must not revoke a paid enrollment. | ||
| if (checkout.status === CheckoutStatus.PAID) { | ||
| logger.warn({ checkoutId: checkout.id }, 'payment.failed: session already PAID, ignoring'); | ||
| return null; | ||
| } | ||
|
|
||
| await tx.checkoutSession.update({ | ||
| where: { id: checkout.id }, | ||
| data: { | ||
| status: CheckoutStatus.FAILED, | ||
| failedAt: new Date(), | ||
| failureReason: reason, | ||
| }, | ||
| }); | ||
|
|
||
| return { email: checkout.email, tierName: checkout.pricingTier.name }; | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
TOCTOU race can let a payment.failed event overwrite a concurrently-committed PAID session.
checkout.status === PAID is checked, then update is issued unconditionally on where: { id: checkout.id }. Under Postgres's default Read Committed isolation, a concurrent payment.paid transaction can commit PAID in between this findFirst and update, so this transaction's snapshot still shows the pre-paid status and proceeds to overwrite it back to FAILED — exactly the invariant the code comments say must never happen ("Never downgrade a session that already succeeded").
Guard the mutation atomically instead of doing check-then-act, the same pattern already used correctly in sweepExpiredCheckouts (Line 689-696) via a conditional updateMany.
🔒 Proposed fix: atomic conditional update
- // Never downgrade a session that already succeeded — a late/duplicate
- // failure event must not revoke a paid enrollment.
- if (checkout.status === CheckoutStatus.PAID) {
- logger.warn({ checkoutId: checkout.id }, 'payment.failed: session already PAID, ignoring');
- return null;
- }
-
- await tx.checkoutSession.update({
- where: { id: checkout.id },
- data: {
- status: CheckoutStatus.FAILED,
- failedAt: new Date(),
- failureReason: reason,
- },
- });
+ // Never downgrade a session that already succeeded — the WHERE clause
+ // guards this atomically against a concurrent payment.paid commit.
+ const { count } = await tx.checkoutSession.updateMany({
+ where: { id: checkout.id, status: { not: CheckoutStatus.PAID } },
+ data: {
+ status: CheckoutStatus.FAILED,
+ failedAt: new Date(),
+ failureReason: reason,
+ },
+ });
+ if (count === 0) {
+ logger.warn({ checkoutId: checkout.id }, 'payment.failed: session already PAID, ignoring');
+ return null;
+ }📝 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.
Suggested change
| const checkout = | |
| (sourceId | |
| ? await tx.checkoutSession.findFirst({ | |
| where: { paymongoSourceId: sourceId, deletedAt: null }, | |
| select: { id: true, email: true, status: true, pricingTier: { select: { name: true } } }, | |
| }) | |
| : null) ?? | |
| (await tx.checkoutSession.findFirst({ | |
| where: { paymongoPaymentId: paymentIdPm, deletedAt: null }, | |
| select: { id: true, email: true, status: true, pricingTier: { select: { name: true } } }, | |
| })); | |
| if (!checkout) { | |
| logger.warn({ paymentId: paymentIdPm, sourceId }, 'payment.failed: no checkout session found'); | |
| return null; | |
| } | |
| // Never downgrade a session that already succeeded — a late/duplicate | |
| // failure event must not revoke a paid enrollment. | |
| if (checkout.status === CheckoutStatus.PAID) { | |
| logger.warn({ checkoutId: checkout.id }, 'payment.failed: session already PAID, ignoring'); | |
| return null; | |
| } | |
| await tx.checkoutSession.update({ | |
| where: { id: checkout.id }, | |
| data: { | |
| status: CheckoutStatus.FAILED, | |
| failedAt: new Date(), | |
| failureReason: reason, | |
| }, | |
| }); | |
| return { email: checkout.email, tierName: checkout.pricingTier.name }; | |
| }); | |
| const checkout = | |
| (sourceId | |
| ? await tx.checkoutSession.findFirst({ | |
| where: { paymongoSourceId: sourceId, deletedAt: null }, | |
| select: { id: true, email: true, status: true, pricingTier: { select: { name: true } } }, | |
| }) | |
| : null) ?? | |
| (await tx.checkoutSession.findFirst({ | |
| where: { paymongoPaymentId: paymentIdPm, deletedAt: null }, | |
| select: { id: true, email: true, status: true, pricingTier: { select: { name: true } } }, | |
| })); | |
| if (!checkout) { | |
| logger.warn({ paymentId: paymentIdPm, sourceId }, 'payment.failed: no checkout session found'); | |
| return null; | |
| } | |
| // Never downgrade a session that already succeeded — the WHERE clause | |
| // guards this atomically against a concurrent payment.paid commit. | |
| const { count } = await tx.checkoutSession.updateMany({ | |
| where: { id: checkout.id, status: { not: CheckoutStatus.PAID } }, | |
| data: { | |
| status: CheckoutStatus.FAILED, | |
| failedAt: new Date(), | |
| failureReason: reason, | |
| }, | |
| }); | |
| if (count === 0) { | |
| logger.warn({ checkoutId: checkout.id }, 'payment.failed: session already PAID, ignoring'); | |
| return null; | |
| } | |
| return { email: checkout.email, tierName: checkout.pricingTier.name }; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/enrollment.ts` around lines 559 - 593, Replace the unconditional
update in the payment.failed flow with an atomic conditional updateMany,
matching the pattern used by sweepExpiredCheckouts: target checkout.id and
require status not equal to CheckoutStatus.PAID. Preserve the existing early
PAID check and return the enrollment details only when the conditional mutation
succeeds; handle a zero-row update without downgrading or revoking the paid
session.
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
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
payment.failedevent (fired for the Source-based checkout flow this app actually uses) was previously dropped silently by the webhook handler'sdefaultcase — a buyer whose card was declined got no notice and the CheckoutSession stayed PENDING forever.handlePaymentFailed()insrc/lib/enrollment.tsto mark the session FAILED and best-effort email the buyer a retry link.sendPaymentFailedEmail()template insrc/lib/email.tsx.Test plan
pnpm typecheckcleanpnpm testpassing (includes new/extendedenrollment.test.tscoverage)Summary by CodeRabbit
New Features
CRON_SECRET.Bug Fixes
Tests