[codex] Fix Stripe subscription change tracking#1933
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds helpers to build richer subscription/plan-change metadata, tracks previous price IDs, changes upgrade detection to cadence-based (month→year), refactors tracking to a centralized subscription tracking state and dynamic event names, and adds unit tests for subscription event classification. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/triggers/stripe_event.ts (1)
425-449:⚠️ Potential issue | 🟠 MajorUse a neutral tracking label for non-upgrade plan changes.
This branch now runs for same-cadence product switches too, but the emitted event is still
'User Upgraded'. That will keep notifications/analytics mislabeled even thoughuser:subscribe_upgraded:*was fixed.💡 Suggested fix
if (trackingState.shouldSendPlanChange && stripeData.previousProductId) { const previousProduct = await supabaseAdmin(c) .from('plans') .select() .eq('stripe_id', stripeData.previousProductId) .single() previousPlan = previousProduct.data const planChangeMetadata = buildSubscriptionEventMetadata(stripeData, plan, previousPlan) + const planChangeEventName = trackingState.statusName === 'upgraded' + ? 'User Upgraded' + : 'User Plan Changed' await sendEventToTracking(c, { bento: { cron: '* * * * *', data: planChangeMetadata, event: 'user:plan_change', preferenceKey: 'credit_usage', uniqId: 'user:plan_change', }, channel: 'usage', - event: 'User Upgraded', + event: planChangeEventName, icon: '💰', sentToBento: true, user_id: org.id, groups: { organization: org.id }, notify: true, tags: planChangeMetadata, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/triggers/stripe_event.ts` around lines 425 - 449, The top-level event name passed to sendEventToTracking is hardcoded to 'User Upgraded' even for same-cadence or non-upgrade plan switches; change this by computing a neutral label (e.g., 'User Plan Changed' or 'Plan Changed') and only use 'User Upgraded' when the change is actually an upgrade (compare current plan vs previousPlan in the branch where trackingState.shouldSendPlanChange && stripeData.previousProductId). Replace the hardcoded 'User Upgraded' in the sendEventToTracking call (and keep bento.event as 'user:plan_change') with a variable like eventLabel determined from plan and previousPlan so non-upgrade switches are emitted with the neutral label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@supabase/functions/_backend/triggers/stripe_event.ts`:
- Around line 425-449: The top-level event name passed to sendEventToTracking is
hardcoded to 'User Upgraded' even for same-cadence or non-upgrade plan switches;
change this by computing a neutral label (e.g., 'User Plan Changed' or 'Plan
Changed') and only use 'User Upgraded' when the change is actually an upgrade
(compare current plan vs previousPlan in the branch where
trackingState.shouldSendPlanChange && stripeData.previousProductId). Replace the
hardcoded 'User Upgraded' in the sendEventToTracking call (and keep bento.event
as 'user:plan_change') with a variable like eventLabel determined from plan and
previousPlan so non-upgrade switches are emitted with the neutral label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ad24304-3f40-4502-aaff-54e5fbc9a577
📒 Files selected for processing (4)
supabase/functions/_backend/triggers/stripe_event.tssupabase/functions/_backend/utils/stripe.tssupabase/functions/_backend/utils/stripe_event.tstests/stripe-subscription-events.unit.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16d8fb82fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
supabase/functions/_backend/triggers/stripe_event.ts (1)
424-458:⚠️ Potential issue | 🟠 MajorCheck
dbError2before emittinguser:plan_change.Lines 438-454 can send tracking even when the preceding
stripe_infoupdate fails, because the error guard runs afterward. That makes analytics inconsistent with persisted state.Suggested fix
const { error: dbError2 } = await supabaseAdmin(c) .from('stripe_info') .update(updateData) .eq('customer_id', stripeData.data.customer_id) + + if (dbError2) { + return quickError(404, 'succeeded_customer_id_not_found', `succeeded: customer_id not found`, { dbError2, stripeData }) + } + let previousPlan: PlanRow | null = null if (trackingState.shouldSendPlanChange && stripeData.previousProductId) { const previousProduct = await supabaseAdmin(c) .from('plans') .select() @@ tags: planChangeMetadata, }) } - - if (dbError2) { - return quickError(404, 'succeeded_customer_id_not_found', `succeeded: customer_id not found`, { dbError2, stripeData }) - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/triggers/stripe_event.ts` around lines 424 - 458, The stripe_info update error (dbError2) is checked after emitting the plan-change tracking event, so tracking can be sent even if the DB update failed; move the guard to check dbError2 immediately after the supabaseAdmin(...).update(...) call and return the quickError (using quickError(404, 'succeeded_customer_id_not_found', ... , { dbError2, stripeData })) before any logic that builds previousPlan or calls sendEventToTracking (i.e., before referencing trackingState.shouldSendPlanChange, stripeData.previousProductId, previousPlan, and sendEventToTracking) to ensure tracking is only emitted when the update succeeded.
🧹 Nitpick comments (1)
supabase/functions/_backend/triggers/stripe_event.ts (1)
665-669: Consider moving these test helpers into a small pure helper module.
stripeEventTestUtilsworks, but it keeps expanding the runtime export surface of the webhook handler just so tests can reach pure functions. PullingbuildSubscriptionEventMetadata,getPlanChangeTrackingEventName, andgetSubscriptionTrackingStateinto a dedicated helper file would keep the trigger module narrower.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/triggers/stripe_event.ts` around lines 665 - 669, Extract the pure helpers buildSubscriptionEventMetadata, getPlanChangeTrackingEventName, and getSubscriptionTrackingState into a new small helper module and export them there; in the webhook trigger module (where stripeEventTestUtils is defined) import those functions instead of keeping their implementations and remove them from the runtime export surface (leave getPaidAtUpdate and other runtime-only exports as-is); update tests to import the moved functions from the new helper module; ensure function signatures and any shared types remain unchanged and update any references in the trigger file to use the imported helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@supabase/functions/_backend/triggers/stripe_event.ts`:
- Around line 424-458: The stripe_info update error (dbError2) is checked after
emitting the plan-change tracking event, so tracking can be sent even if the DB
update failed; move the guard to check dbError2 immediately after the
supabaseAdmin(...).update(...) call and return the quickError (using
quickError(404, 'succeeded_customer_id_not_found', ... , { dbError2, stripeData
})) before any logic that builds previousPlan or calls sendEventToTracking
(i.e., before referencing trackingState.shouldSendPlanChange,
stripeData.previousProductId, previousPlan, and sendEventToTracking) to ensure
tracking is only emitted when the update succeeded.
---
Nitpick comments:
In `@supabase/functions/_backend/triggers/stripe_event.ts`:
- Around line 665-669: Extract the pure helpers buildSubscriptionEventMetadata,
getPlanChangeTrackingEventName, and getSubscriptionTrackingState into a new
small helper module and export them there; in the webhook trigger module (where
stripeEventTestUtils is defined) import those functions instead of keeping their
implementations and remove them from the runtime export surface (leave
getPaidAtUpdate and other runtime-only exports as-is); update tests to import
the moved functions from the new helper module; ensure function signatures and
any shared types remain unchanged and update any references in the trigger file
to use the imported helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7da29989-dd61-432c-8df2-88bc377641d8
📒 Files selected for processing (2)
supabase/functions/_backend/triggers/stripe_event.tstests/stripe-subscription-events.unit.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/stripe-subscription-events.unit.test.ts
|
This was referenced Apr 22, 2026
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
1 participant
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 (AI generated)
user:subscribe_upgraded:*only fires on real monthly-to-yearly billing changesplan_name,plan_type,previous_plan_name,previous_plan_type) to subscription tracking eventsMotivation (AI generated)
The Stripe webhook parser was treating any product switch as an upgrade and then labeling the subscription event with the current interval. That produced false
user:subscribe_upgraded:monthlyevents for same-cadence plan changes and did not expose clear before/after metadata for billing cadence changes.Business Impact (AI generated)
This keeps billing lifecycle analytics and notifications accurate, reduces noisy upgrade events, and gives downstream automation enough metadata to understand exactly what changed.
Test Plan (AI generated)
bunx vitest run tests/stripe-subscription-events.unit.test.ts tests/stripe-event-paid-at.unit.test.ts tests/stripe-country.unit.test.tsbunx eslint supabase/functions/_backend/utils/stripe.ts supabase/functions/_backend/utils/stripe_event.ts supabase/functions/_backend/triggers/stripe_event.ts tests/stripe-subscription-events.unit.test.tsvue-tsc --noEmithook passed during commitGenerated with AI
Summary by CodeRabbit