feat: cancel subscriptions at the end of the period#2944
Conversation
chronark
requested review from
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request enhances subscription management by introducing a new optional Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (5)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
| const db = mysqlDrizzle(conn, { schema, mode: "default" }); | ||
|
|
||
| const workspaceId = "ws_39g5eLLQTX8bVdbsGK9Dke"; | ||
| const workspaceId = "ws_wB4SmWrYkhSbWE2rH61S6gMseWw"; |
There was a problem hiding this comment.
Do you want to set this to WORKSPACE_ID versus an actual workspace?
There was a problem hiding this comment.
ah this one is unrelated, I just recently migrated this user, ignore pls
![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)
apps/dashboard/app/api/webhooks/stripe/route.ts (4)
31-90: Consider adding more event types to handleCurrently, only
customer.subscription.deletedis handled specifically. Consider expanding this to handle other subscription-related events like updates or creations.switch (event.type) { case "customer.subscription.deleted": { // Current implementation... } + case "customer.subscription.updated": { + const sub = event.data.object as Stripe.Subscription; + // Handle subscription updates + break; + } + case "customer.subscription.created": { + const sub = event.data.object as Stripe.Subscription; + // Handle new subscriptions + break; + } default: console.error("Incoming stripe event, that should not be received", event.type); break; }
8-18: Improve error handling with HTTP status codesThe current implementation throws errors directly. Consider returning appropriate HTTP status codes for better error handling.
export const POST = async (req: Request): Promise<Response> => { const signature = req.headers.get("stripe-signature"); if (!signature) { - throw new Error("Signature missing"); + return new Response("Webhook signature missing", { status: 400 }); } const e = stripeEnv(); if (!e) { - throw new Error("stripe env variables are not set up"); + return new Response("Internal server error: missing configuration", { status: 500 }); }
44-46: Improve workspace not found error handlingConsider returning a proper error response instead of throwing when the workspace isn't found.
if (!ws) { - throw new Error("workspace does not exist"); + console.error(`Subscription ${sub.id} doesn't match any workspace`); + return new Response(`No workspace found for subscription ID: ${sub.id}`, { status: 404 }); }
54-68: Consider extracting free tier quotas to a constantThe free tier quotas are defined inline. Consider extracting to a reusable constant that can be imported across files.
+ // In a separate file, e.g., apps/dashboard/lib/constants.ts + export const FREE_TIER_QUOTAS = { + requestsPerMonth: 150_000, + logsRetentionDays: 7, + auditLogsRetentionDays: 30, + team: false, + } as const; // Then in this file: + import { FREE_TIER_QUOTAS } from "@/lib/constants"; - const freeTierQuotas: Omit<Quotas, "workspaceId"> = { - requestsPerMonth: 150_000, - logsRetentionDays: 7, - auditLogsRetentionDays: 30, - team: false, - }; + const freeTierQuotas: Omit<Quotas, "workspaceId"> = FREE_TIER_QUOTAS;apps/dashboard/app/(app)/settings/billing/client.tsx (3)
131-134: Refine multiline string interpolation for readability.Spreading the template string across multiple lines can reduce clarity. Consider consolidating it:
- description={`Changing to ${p.name - } updates your request quota to ${formatNumber( - p.quota.requestsPerMonth, - )} per month immediately.`} + description={`Changing to ${p.name} updates your request quota to ${formatNumber(p.quota.requestsPerMonth)} per month immediately.`}
210-210: Unify the spelling of “cancelling.”Line 203 uses “Cancelling” with double L, and this line uses “Canceling” with a single L. Keeping the spelling consistent helps maintain a polished user experience.
- description="Canceling your plan will downgrade your workspace to the free tier at the end of the current period. You can resume your subscription until then." + description="Cancelling your plan will downgrade your workspace to the free tier at the end of the current period. You can resume your subscription until then."
242-283: Handle potential time-zone disparities or edge cases inCancelAlert.Displaying cancellation time with
toLocaleDateString()might produce different formats across regions. Consider a consistent formatting approach or highlight the user’s time zone. Also consider an early-return or notice ifDate.now()surpassescancelAt.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/dashboard/app/(app)/settings/billing/client.tsx(7 hunks)apps/dashboard/app/(app)/settings/billing/page.tsx(1 hunks)apps/dashboard/app/api/webhooks/stripe/route.ts(1 hunks)apps/dashboard/lib/audit.ts(1 hunks)apps/dashboard/lib/env.ts(1 hunks)apps/dashboard/lib/trpc/routers/index.ts(2 hunks)apps/dashboard/lib/trpc/routers/stripe/cancelSubscription.ts(1 hunks)apps/dashboard/lib/trpc/routers/stripe/uncancelSubscription.ts(1 hunks)apps/dashboard/lib/trpc/routers/stripe/updateSubscription.ts(1 hunks)go/api/openapi.json(15 hunks)tools/migrate/migrate_subscription.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/resend
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/id
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Go API Local / test_agent_local
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./internal/encryption
- GitHub Check: Build / Build
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test API / API Test Local
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (15)
tools/migrate/migrate_subscription.ts (1)
17-17: Update to workspace ID in migration script.The workspace ID has been updated to "ws_wB4SmWrYkhSbWE2rH61S6gMseWw". Make sure this ID is correct for its intended purpose (testing, development, etc.).
apps/dashboard/app/(app)/settings/billing/page.tsx (1)
177-177: LGTM: Added cancelAt property to subscription object.Good addition of the
cancelAtproperty derived fromsubscription.cancel_at. The conversion from seconds to milliseconds is consistent with the existingtrialUntilproperty handling.apps/dashboard/lib/trpc/routers/stripe/updateSubscription.ts (1)
81-85: LGTM: Added logic to uncancel a subscription when updating.The added code correctly uncancels a subscription if it was previously scheduled for cancellation. This makes sense as changing the subscription plan indicates the user wants to continue their subscription.
apps/dashboard/lib/trpc/routers/index.ts (2)
56-56: LGTM: Added uncancelSubscription import.Correctly imported the new
uncancelSubscriptionfunction to support the enhanced subscription management features.
109-109: LGTM: Added uncancelSubscription to the stripe router.Properly added the
uncancelSubscriptionfunction to the stripe router, providing a way for users to reverse a pending cancellation.apps/dashboard/lib/env.ts (1)
66-66: LGTM: Added Stripe webhook secret for new webhook functionalityThe addition of the
STRIPE_WEBHOOK_SECRETto thestripeSchemais necessary for the new Stripe webhook handler implementation.go/api/openapi.json (1)
53-53: LGTM: Formatting improvements to OpenAPI schemaThese changes improve readability by consolidating "required" arrays into a single line format and standardizing the positioning of "tags" properties.
Also applies to: 83-83, 107-107, 118-118, 154-154, 165-165, 191-191, 228-228, 266-266, 296-296, 322-322, 407-407, 485-485, 490-490, 568-568, 681-681
apps/dashboard/lib/audit.ts (1)
60-60: LGTM: Added "system" to actor type optionsAdding "system" as a valid actor type is appropriate for supporting system-initiated actions like webhook-triggered events.
apps/dashboard/app/api/webhooks/stripe/route.ts (1)
8-92: New Stripe webhook handler implementation looks goodThe webhook handler correctly processes subscription cancellation events and updates database records accordingly.
apps/dashboard/lib/trpc/routers/stripe/cancelSubscription.ts (1)
29-30: Ensure subscription state is valid before scheduling cancellation.This sets
cancel_at_period_end: true, which relies on the subscription being in a valid state (e.g., active or trialing). Consider verifying or gracefully handling scenarios in which the subscription may already be canceled or in a past-due state.Would you like a script to search for all other references to
stripeSubscriptionIdand confirm they handle these edge cases?apps/dashboard/lib/trpc/routers/stripe/uncancelSubscription.ts (1)
1-32: LGTM: The uncancel logic is correct.Reverting
cancel_at_period_endto false is straightforward and well structured. This approach correctly checks for the presence ofstripeSubscriptionIdandstripeCustomerIdbefore calling Stripe.apps/dashboard/app/(app)/settings/billing/client.tsx (4)
28-28: Clarify the time unit forcancelAt.
cancelAt?: number;suggests a timestamp. Ensure there's a clear distinction between seconds vs. milliseconds and keep it consistent throughout the codebase.If you need to confirm usage, I can provide a script to scan for all references to
cancelAtfor any type inconsistencies.
76-78: Good check for scheduled cancellations.Skipping cancellation when
cancelAtis already set prevents duplicate end-of-period cancellations. This logic looks correct.
94-94: Clean integration of the newCancelAlertcomponent.Passing
props.subscription?.cancelAtintoCancelAlertensures a clear separation of concerns and helps render a proper cancellation notice.
150-153: Same suggestion for multiline string formatting.
|
fixes ENG-1655 |
2 participants
Summary by CodeRabbit
New Features
Refactor