chore: Make Stripe Great Again #4479
Conversation
This makes stripe more robust. It checks for actual changes before firing webhooks, and also checks if we should auto apply the updates to quotas based upon a DB entry.
perkinsjr
requested review from
MichaelUnkey,
chronark,
mcstepp and
ogzhanolguncu
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughDetects automated Stripe subscription renewals and early-exits the webhook handler for those events, skipping database updates and notifications; non-automated subscription updates continue with explicit quota and workspace tier updates inside a transaction and a dedicated notification step. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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)
apps/dashboard/app/api/webhooks/stripe/route.ts (1)
154-157: Add logging for observability when skipping automated renewals.When silently returning early, debugging becomes difficult if something goes wrong. Consider adding a debug log to track when renewals are skipped:
// Skip database updates and notifications for automated billing renewals if (isAutomatedBillingRenewal(sub, previousAttributes)) { + console.log(`Skipping automated billing renewal for subscription: ${sub.id}`); return new Response("OK"); }This will help with debugging and monitoring the behavior in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/api/webhooks/stripe/route.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-08T15:33:04.290Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 2120
File: apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx:19-19
Timestamp: 2024-10-08T15:33:04.290Z
Learning: In the `StripeSuccess` function, TypeScript's type-checking of the `new_plan` parameter ensures that only "free", "pro", or undefined values are accepted, so additional runtime validation is not necessary.
Applied to files:
apps/dashboard/app/api/webhooks/stripe/route.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/dashboard/app/api/webhooks/stripe/route.ts (3)
14-34: Interface definition looks reasonable.The interface correctly models the expected fields from Stripe's
previous_attributespayload. Note that Stripe's actual type is a partial of the subscription object, so this custom interface provides clearer typing for the specific fields being checked.
204-204: Clarifying comment is helpful.
286-286: Clarifying comment is helpful.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/dashboard/app/api/webhooks/stripe/route.ts (1)
36-63: Consider a more resilient detection approach for automated renewals.As noted in the previous review, the current exact-match logic is brittle. Stripe's
subscription.updatedwebhook can include additional fields beyond justcurrent_period_startandcurrent_period_endduring legitimate automated renewals (e.g.,billing_cycle_anchor, nesteditemschanges). This will cause the function to returnfalsefor actual automated renewals, leading to unnecessary database updates and notifications.Consider inverting the logic to check for the absence of meaningful change keys (such as
items,plan,quantity,discount,cancel_at_period_end,collection_method) rather than requiring an exact match of expected keys. This approach is more forward-compatible and resilient to Stripe's evolving webhook payloads.
🧹 Nitpick comments (1)
apps/dashboard/app/api/webhooks/stripe/route.ts (1)
154-157: Add logging and use standard status code for observability.The early exit lacks logging, making it difficult to monitor how often automated renewals are being skipped. Additionally, returning a 201 status with "Skip" is non-standard for webhook handlers.
Apply this diff to improve observability and use standard conventions:
// Skip database updates and notifications for automated billing renewals if (isAutomatedBillingRenewal(sub, previousAttributes)) { + console.log(`Skipping automated renewal for subscription: ${sub.id}, workspace: ${ws.id}`); - return new Response("Skip", { status: 201 }); + return new Response("OK", { status: 200 }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/app/api/webhooks/stripe/route.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-08T15:33:04.290Z
Learnt from: mcstepp
Repo: unkeyed/unkey PR: 2120
File: apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx:19-19
Timestamp: 2024-10-08T15:33:04.290Z
Learning: In the `StripeSuccess` function, TypeScript's type-checking of the `new_plan` parameter ensures that only "free", "pro", or undefined values are accepted, so additional runtime validation is not necessary.
Applied to files:
apps/dashboard/app/api/webhooks/stripe/route.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test Dashboard / Test Dashboard
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/dashboard/app/api/webhooks/stripe/route.ts (2)
14-34: LGTM: Interface structure is adequate.The
PreviousAttributesinterface appropriately models the Stripe subscription fields that can appear inprevious_attributes. The comments help distinguish between automated renewal fields and manual change fields.
204-204: LGTM: Clarifying comments improve readability.The added comments clearly describe the distinct steps in the subscription update workflow, making the code easier to follow.
Also applies to: 286-286
* fix: Make stripe webhooks more robust
What does this PR do?
This updates the Stripe webhook to check if anything changed and if not just early exit. This will stop it from firing on 1st of the month but still fire for everything else.
Fixes # (issue)
#4441
Type of change
How should this be tested?
Join stripe-test via invite
Subscriptions on first of the month:
Put card in.
Setup a subscription
Use fast forward to go to the 1st of January
Make sure that the payment for that month goes through
No notifications
Checklist
Required
pnpm buildpnpm fmtmake fmton/godirectoryconsole.logsgit pull origin mainAppreciated