Fix race condition on stripe subscription #9629
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR addresses a race condition in the Stripe subscription handling process by implementing better synchronization between frontend and backend states.
- Added new
RefreshSubscriptionStatusEffectcomponent in/modules/billing/components/RefreshSubscriptionStatusEffect.tsxthat polls subscription status up to 5 times with 1-second intervals - Modified
PaymentSuccesspage to includeRefreshSubscriptionStatusEffect, preventing premature navigation before subscription status is confirmed - Updated navigation logic in
usePageChangeEffectNavigateLocationto treatPlanRequiredSuccessas an open signed-in route - Adjusted test cases to reflect new navigation behavior, particularly for
PlanRequiredSuccessstates
4 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
packages/twenty-front/src/modules/billing/components/RefreshSubscriptionStatusEffect.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/billing/components/RefreshSubscriptionStatusEffect.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/billing/components/RefreshSubscriptionStatusEffect.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/hooks/usePageChangeEffectNavigateLocation.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/hooks/usePageChangeEffectNavigateLocation.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/billing/components/RefreshSubscriptionStatusEffect.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/billing/components/RefreshSubscriptionStatusEffect.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Left comments! I think we can merge the change on PaymentSuccess and on RefreshSubStatusEffect.
Questioning the change in usePageChangeEffect: I don't get how it works + will be overridden by @etiennejouan change
| if (!isDefined(subscriptionStatus)) { | ||
| const result = await getCurrentUser({ fetchPolicy: 'network-only' }); | ||
| const currentUser = result.data?.currentUser; | ||
| if (isDefined(currentUser)) { |
There was a problem hiding this comment.
let's make a deep equal here (make it a useRecoilCallback and check the currentUser in the snapshot, this will avoid unnecessary re-renders)
martmull
force-pushed
the
fix-stripe-webhook-race-condition
branch
from
0ad1d9e to
769723e
Compare
| return; | ||
| } | ||
|
|
||
| throw new Error( |
There was a problem hiding this comment.
shouldn't we use a Snackbar here?
There was a problem hiding this comment.
yes, but we also want the error to be send to sentry, so I throw an error (in add snackBar and send error to sentry)
| if (currentUser?.onboardingStatus === OnboardingStatus.Completed) { | ||
| const navigateWithSubscriptionCheck = async () => { | ||
| if (isDefined(subscriptionStatus)) { | ||
| navigate(AppPath.CreateWorkspace); |
There was a problem hiding this comment.
I don't get this, won't it always redirect here?
There was a problem hiding this comment.
no, if we receive the stripe subscription create event after the stripe checkout redirection, subscriptionStatus will be undefined.
In that case, we refresh subscriptionStatus, redirect if not undefined, and throw error if undefined
Fixes twentyhq/core-team-issues#191
Observation: Locally, I had to delay the stripe webhook subscription created endpoint by 7s to see race condition issue
Enregistrement.de.l.ecran.2025-01-15.a.15.56.49.mov