fix(billing): a user can add a payment method while upgrading their plan #2117
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughWalkthroughThe changes introduce a series of updates to the billing and payment handling functionality within the application. A new function, Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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! 🙏 |
| @@ -47,6 +48,18 @@ export const ChangePlanButton: React.FC<Props> = ({ workspace, newPlan, label }) | |||
| }, | |||
| }); | |||
|
|
|||
| const handleClick = () => { | |||
| const hasPaymentMethod = !!workspace.stripeCustomerId; | |||
There was a problem hiding this comment.
Only checking if there's a customer associated because I can't configure the stripe env within the client side component, and I want to avoid passing stripe info as a prop from the server page.
this is where the edge case can occur: if they have a stripe customer id, but they've deleted their payment method from stripe, it won't error out until the trpc procedure attempts to change the plan, so it's just a UX inconvenience. the likelihood that the user will delete their payment method is very slim.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (1)
apps/dashboard/lib/posthog.ts (1)
1-28: Consider adding type annotations to improve code readability and maintainability.While the code is functional, adding type annotations to function parameters, return values, and variables can enhance code readability and catch potential type-related issues during development. For example:
private static instance: PostHog | null = null; public static getInstance(): PostHog { // ... }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (5)
- apps/dashboard/app/(app)/settings/billing/plans/button.tsx (3 hunks)
- apps/dashboard/app/(app)/settings/billing/stripe/page.tsx (2 hunks)
- apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx (3 hunks)
- apps/dashboard/lib/posthog.ts (1 hunks)
- apps/dashboard/package.json (1 hunks)
Additional context used
Biome
apps/dashboard/app/(app)/settings/billing/stripe/page.tsx
[error] 65-65: Use !== instead of !=.
!= is only allowed when comparing againstnull!= is only allowed when comparing against null
Using != may be unsafe if you are relying on type coercion
Unsafe fix: Use !==(lint/suspicious/noDoubleEquals)
apps/dashboard/app/(app)/settings/billing/stripe/success/page.tsx
[error] 75-75: Use !== instead of !=.
!= is only allowed when comparing againstnull!= is only allowed when comparing against null
Using != may be unsafe if you are relying on type coercion
Unsafe fix: Use !==(lint/suspicious/noDoubleEquals)
Additional comments not posted (8)
apps/dashboard/lib/posthog.ts (1)
1-28: Great use of the singleton pattern for the PostHog client!The implementation of the
PostHogClientWrapperclass ensures that only one instance of thePostHogclient is created and shared throughout the application. This approach promotes efficient resource utilization and prevents multiple instances from being created unnecessarily.apps/dashboard/app/(app)/settings/billing/stripe/page.tsx (3)
10-14: LGTM!The new
Propstype definition is correct and matches the updated function signature. Thenew_planproperty accurately represents the expected plan types.
15-16: LGTM!The function signature update and the destructuring of
new_planfromprops.searchParamsare implemented correctly.
62-63: LGTM!The
successUrlconstruction is implemented correctly, and the comment provides valuable context about avoidingnew URL(...).searchParamsto prevent escaping curly braces.apps/dashboard/app/(app)/settings/billing/plans/button.tsx (3)
51-61: Great job addressing the upgrade flow issue! 👍The new
handleClickfunction correctly handles the case when a user attempts to switch to the "pro" plan without a payment method by redirecting them to the Stripe payment setup page. This addresses the issue described in the linked issue #2080, where users were encountering errors without any guidance on how to resolve them.The function also handles the case when the user has a payment method or is switching to the "free" plan by calling the
changePlan.mutateAsyncfunction, ensuring a smooth plan change process.Overall, the changes in this code segment are well-structured and follow the expected logic based on the PR objectives and summary.
111-111: Nice refactoring! 🎉Updating the button's
onClickevent handler to call the newhandleClickfunction instead of directly invokingchangePlan.mutateAsyncis a great refactoring decision.This change improves the control flow by centralizing the logic for handling the plan change within the
handleClickfunction. It also enhances the maintainability and readability of the code by separating the plan change logic from the button's event handler.Good job on this refactoring!
Line range hint
1-120: Excellent work on this PR! 🚀The overall changes in this file align perfectly with the PR objectives of fixing the upgrade account flow issue and addressing the problem described in the linked issue #2080.
The introduction of the
handleClickfunction and the refactoring of the button'sonClickevent handler significantly improve the control flow and maintainability of the code. The changes handle the case when a user attempts to upgrade without a payment method by redirecting them to the Stripe payment setup page, addressing the confusion and errors encountered by users as described in the linked issue.Moreover, the changes ensure a smooth plan change process for users with a payment method or those switching to the "free" plan, enhancing the overall user experience.
The refactoring also improves the maintainability and readability of the code by centralizing the plan change logic within the
handleClickfunction, making it easier for future developers to understand and modify the code if needed.Great job on this PR! The changes effectively address the identified issues and improve the codebase. 👏
apps/dashboard/package.json (1)
92-92: Dependency addition looks good!The
posthog-nodedependency has been added correctly with an appropriate version constraint. This aligns with the objective of capturing plan change events from the server side.Please ensure that the captured events respect user privacy and consent. Consider providing an opt-out mechanism if necessary.
| if (!process.env.NEXT_PUBLIC_POSTHOG_KEY) { | ||
| // Return a mock client when the key is not present |
There was a problem hiding this comment.
Add error handling for missing or invalid environment variables.
To improve the robustness of the code, consider adding error handling for cases where the required environment variables (NEXT_PUBLIC_POSTHOG_KEY, NEXT_PUBLIC_POSTHOG_HOST) are missing or have invalid values. You can throw an error or log a warning to alert developers about the misconfiguration.
Here's an example of how you can handle missing or invalid environment variables:
if (!process.env.NEXT_PUBLIC_POSTHOG_KEY) {
console.warn('PostHog key is missing. Analytics data will not be sent.');
// Return a mock client when the key is not present
// ...
} else {
const host = process.env.NEXT_PUBLIC_POSTHOG_HOST;
if (!host) {
console.warn('PostHog host is missing. Using the default host.');
}
PostHogClientWrapper.instance = new PostHog(process.env.NEXT_PUBLIC_POSTHOG_KEY, {
host: host || 'https://app.posthog.com',
// ...
});
}Also applies to: 17-21
| // if they're coming from the change plan flow, pass along the new plan param | ||
| if (new_plan && new_plan != ws.plan) { | ||
| successUrl += `&new_plan=${new_plan}`; | ||
| } |
There was a problem hiding this comment.
Use !== instead of != for the comparison.
To avoid potential type coercion issues, it's safer to use !== instead of != when comparing new_plan and ws.plan.
Apply this diff to fix the comparison:
-if (new_plan && new_plan != ws.plan) {
+if (new_plan && new_plan !== ws.plan) {
successUrl += `&new_plan=${new_plan}`;
}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.
| // if they're coming from the change plan flow, pass along the new plan param | |
| if (new_plan && new_plan != ws.plan) { | |
| successUrl += `&new_plan=${new_plan}`; | |
| } | |
| // if they're coming from the change plan flow, pass along the new plan param | |
| if (new_plan && new_plan !== ws.plan) { | |
| successUrl += `&new_plan=${new_plan}`; | |
| } |
Tools
Biome
[error] 65-65: Use !== instead of !=.
!= is only allowed when comparing againstnull!= is only allowed when comparing against null
Using != may be unsafe if you are relying on type coercion
Unsafe fix: Use !==(lint/suspicious/noDoubleEquals)
| properties: { | ||
| properties: { plan: new_plan, workspace: ws.id } | ||
| } |
There was a problem hiding this comment.
Flatten the properties object in PostHogClient.capture.
The properties key is nested within another properties object, which may lead to incorrect event data being sent. Flatten the object to ensure the properties are correctly captured.
Apply this diff to fix the issue:
properties: {
- properties: { plan: new_plan, workspace: ws.id }
+ plan: new_plan,
+ workspace: ws.id
}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.
| properties: { | |
| properties: { plan: new_plan, workspace: ws.id } | |
| } | |
| properties: { | |
| plan: new_plan, | |
| workspace: ws.id | |
| } |
| @@ -69,14 +72,27 @@ export default async function StripeSuccess(props: Props) { | |||
| ); | |||
| } | |||
|
|
|||
| const isChangingPlan = new_plan && new_plan != ws.plan; | |||
There was a problem hiding this comment.
Use strict inequality operator !==.
When comparing strings, it's safer to use the strict inequality operator !== instead of != to avoid unintended type coercion issues.
Apply this diff to fix the issue:
- const isChangingPlan = new_plan && new_plan != ws.plan;
+ const isChangingPlan = new_plan && new_plan !== ws.plan;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.
| const isChangingPlan = new_plan && new_plan != ws.plan; | |
| const isChangingPlan = new_plan && new_plan !== ws.plan; |
Tools
Biome
[error] 75-75: Use !== instead of !=.
!= is only allowed when comparing againstnull!= is only allowed when comparing against null
Using != may be unsafe if you are relying on type coercion
Unsafe fix: Use !==(lint/suspicious/noDoubleEquals)
|
From Meg Stepp ‣ I'm cleaning up the suggestions |
|
feel free to ignore them if they don’t make sense |
|
From Meg Stepp ‣ it caught something wrong with the posthog node analytics, and im just putting out a new branch to fix this merge conflict hell im in with the pnpm-lock.yaml |
|
I've moved this to a new branch for merge conflict reasons |
What does this PR do?
Previously, when a free user without a payment method was attempting to upgrade their plan, it would error out and notify the user that they must add add a payment method before upgrading.
This change will now catch a missing payment method (stripe association) when the user is upgrading their plan and redirect them Stripe to configure a payment method, and then continue the processing the plan change upon stripe success.
Edge case: When a user has already their stripe associated with Unkey, but has deleted their payment method from stripe, the user will get an error PRECONDITION FAILED when attempting to process their plan change. This is due to technical limitation due to where the stripe check lives (client-side), but it's still checked and guarded on the trpc route. The likelihood of a user configuring stripe and then deleting a payment is very slim.
New Package: PostHog-node to capture the plan change event from the server-side.
Fixes #2080
If there is not an issue for this, please create one first. This is used to tracking purposes and also helps use understand why this PR exists
Type of change
How should this be tested?
Manual testing
Upgrade flow when user has added a payment method first is unchanged.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Bug Fixes
Chores