Skip to content

[codex] add NRR and churn revenue charts#1936

Merged
riderx merged 6 commits into
mainfrom
codex/admin-nrr-churn-charts
Apr 23, 2026
Merged

[codex] add NRR and churn revenue charts#1936
riderx merged 6 commits into
mainfrom
codex/admin-nrr-churn-charts

Conversation

@riderx
Copy link
Copy Markdown
Member

@riderx riderx commented Apr 22, 2026

Summary (AI generated)

  • add daily retention metrics backed by Stripe revenue movement rollups for admin reporting
  • expose NRR and churn revenue through global_stats_trend and render both charts on the admin revenue dashboard
  • add focused unit coverage for Stripe revenue movement classification and retention math

Motivation (AI generated)

The admin revenue dashboard only showed topline recurring revenue metrics. It was missing the retention view needed to understand whether MRR is being sustained by existing customers or eroded by churn and downgrades.

Business Impact (AI generated)

This gives Capgo a better operator view of subscription health by surfacing retained revenue and lost recurring revenue alongside MRR. That helps spot leakage earlier and makes revenue trends more actionable.

Test Plan (AI generated)

  • bun lint
  • bun lint:backend
  • bun typecheck
  • bun vitest run tests/stripe-revenue-movement.unit.test.ts tests/stripe-subscription-events.unit.test.ts tests/logsnag-insights-revenue.unit.test.ts tests/stripe-event-paid-at.unit.test.ts
  • bun run build

Generated with AI

Summary by CodeRabbit

  • New Features

    • Added NRR and Churn Revenue charts to the admin dashboard with percent and dollar formatting.
    • Chart components now support configurable Y-axis scaling (start-at-zero and suggested min/max).
  • Backend / Data

    • System collects per-customer daily revenue movement and persists NRR and churn revenue to global stats for dashboard use.
  • Tests

    • Added unit tests for revenue metric calculations and revenue-movement classification.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 22, 2026

Warning

Rate limit exceeded

@riderx has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 24 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 24 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 38c4c8ca-3aa5-4583-a30f-ccd657d54600

📥 Commits

Reviewing files that changed from the base of the PR and between a1a1528 and 251db29.

📒 Files selected for processing (6)
  • src/types/supabase.types.ts
  • supabase/functions/_backend/triggers/logsnag_insights.ts
  • supabase/functions/_backend/triggers/stripe_event.ts
  • supabase/functions/_backend/utils/supabase.types.ts
  • supabase/migrations/20260422203355_add_admin_retention_metrics.sql
  • tests/stripe-revenue-movement.unit.test.ts
📝 Walkthrough

Walkthrough

Adds per-customer daily MRR rollups, Stripe webhook classification and transactional persistence of MRR deltas, retention metric calculations (NRR and churn_revenue) persisted to global_stats, Supabase schema/types/migration updates, dashboard chart prop/axis changes to surface NRR/churn visuals, and unit tests.

Changes

Cohort / File(s) Summary
Database Schema & Migrations
supabase/migrations/20260422203355_add_admin_retention_metrics.sql
Creates daily_revenue_metrics table; adds nrr (default 100) and churn_revenue (default 0) to global_stats, backfills values, sets comments/indices and permissions.
Supabase Types
src/types/supabase.types.ts, supabase/functions/_backend/utils/supabase.types.ts
Adds daily_revenue_metrics table type and extends global_stats Row/Insert/Update with nrr and churn_revenue.
Stripe Webhook & Movement Classification
supabase/functions/_backend/triggers/stripe_event.ts
Computes subscription MRR, classifies deltas (new/expansion/contraction/churn), adds staleness checks, replaces direct updates with transactional persistence (persistStripeInfoAndRevenueMovement), and upserts per-customer daily deltas; exports test helpers.
Revenue Aggregation & Insights
supabase/functions/_backend/triggers/logsnag_insights.ts
Adds helpers getPreviousDateId, calculateNrr, calculateChurnRevenue, aggregates daily movements, computes retention metrics, and persists nrr/churn_revenue to global_stats; exports utilities for tests.
Query Utilities
supabase/functions/_backend/utils/pg.ts
Extends AdminGlobalStatsTrend and its query to include nrr and churn_revenue (casts/defaults applied) and maps results to numbers.
Admin Chart Component
src/components/admin/AdminMultiLineChart.vue
Adds props beginAtZero, suggestedMin, suggestedMax and applies them to Chart.js Y-axis config (replacing fixed beginAtZero).
Admin Dashboard Pages
src/pages/admin/dashboard/revenue.vue
Adds NRR and churn revenue series, percent-formatted NRR axis with auto-range padding and fallback, dollar-formatted revenue labels, and renders retention charts section wired to has-data.
Unit Tests
tests/logsnag-insights-revenue.unit.test.ts, tests/stripe-revenue-movement.unit.test.ts
Adds tests for NRR/churn calculations, stripe movement classification, staleness logic, and related helpers.
Utilities / Types in Functions
supabase/functions/_backend/utils/supabase.types.ts
Mirrors DB type additions inside functions codebase (adds daily_revenue_metrics types and global_stats fields).

Sequence Diagram

sequenceDiagram
    participant Stripe as Stripe Event
    participant Webhook as stripe_event.ts
    participant DB as Postgres (daily_revenue_metrics / global_stats)
    participant Insights as logsnag_insights.ts
    participant Admin as Admin Dashboard

    Stripe->>Webhook: deliver webhook (created/updated/canceled)
    Webhook->>Webhook: compute plan MRR & classifyRevenueMovement
    Webhook->>Webhook: check isStaleStripeEvent
    Webhook->>DB: begin transaction -> lock stripe_info row
    Webhook->>DB: update stripe_info (allowlisted) or return 'missing'
    Webhook->>DB: upsert additive deltas into daily_revenue_metrics (date_id, customer_id)
    Webhook->>DB: commit transaction
    Insights->>DB: query daily_revenue_metrics and prior global_stats.mrr
    Insights->>Insights: calculateChurnRevenue & calculateNrr
    Insights->>DB: update global_stats with nrr and churn_revenue
    Admin->>DB: query AdminGlobalStatsTrend (includes nrr, churn_revenue)
    Admin->>Admin: render NRR and churn charts (AdminMultiLineChart uses new axis props)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐇 I hopped through rows of MRR and cheer,

I counted churn and growth with whiskers near,
Daily deltas tucked in tidy rows,
Charts now bloom where retention flows,
A carrot-coded chorus — metrics, hop, hooray! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title '[codex] add NRR and churn revenue charts' clearly and concisely describes the main change: adding NRR and churn revenue visualization components to the admin dashboard.
Description check ✅ Passed The description includes a comprehensive summary, clear motivation, business impact, and a detailed test plan with completed checkmarks covering linting, type checking, unit tests, and build verification.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/admin-nrr-churn-charts

Comment @coderabbitai help to get the list of available commands and usage tips.

@codspeed-hq
Copy link
Copy Markdown
Contributor

codspeed-hq Bot commented Apr 22, 2026

Merging this PR will not alter performance

✅ 28 untouched benchmarks


Comparing codex/admin-nrr-churn-charts (251db29) with main (229ba5e)

Open in CodSpeed

@riderx riderx marked this pull request as ready for review April 22, 2026 20:49
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

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)

617-625: ⚠️ Potential issue | 🟠 Major

Make the subscription-state write and revenue-rollup write atomic.

stripe_info is committed before daily_revenue_metrics. If the second write fails or the worker exits between them, a retry will read the already-updated stripe_info row and classify a zero delta, so that revenue movement is lost permanently. Please execute both writes in one DB transaction, or persist an idempotent event ledger first and derive both updates from it.

Also applies to: 849-853

🤖 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 617 - 625,
The code currently updates stripe_info (the update using
supabaseAdmin(...).from('stripe_info').update(...)) and then calls
recordRevenueMovement(...) separately, causing lost revenue if the second write
fails; fix by making the subscription-state write and the revenue-rollup write
atomic: either wrap the stripe_info update and the operations performed by
recordRevenueMovement into a single DB transaction (execute both SQL statements
inside one BEGIN/COMMIT using the same supabaseAdmin(c) connection or a stored
procedure that performs both updates atomically) so that the update and revenue
rollup succeed or fail together, or change the flow to first persist an
idempotent event ledger row (insert into a stripe_events/event_ledger table) and
then derive both stripe_info and daily_revenue_metrics from that ledger in a
separate, idempotent worker run (insert ledger row before calling
recordRevenueMovement); update both places referenced (the stripe_info update +
the call to recordRevenueMovement at this location and the other occurrence
around lines 849-853) to use the chosen atomic approach and ensure error
handling returns failures when the transaction/ledger insert fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/pages/admin/dashboard/revenue.vue`:
- Around line 169-179: The nrrSeries computed currently falls back to 0 for
missing item.nrr values causing incorrect charting; update the mapping in the
nrrSeries computed (where it builds data from globalStatsTrendData.value) to use
a fallback of 100 for item.nrr (e.g., replace the use of item.nrr || 0 with a
fallback of 100) so missing NRR points render as 100% baseline instead of 0%.

In `@supabase/migrations/20260422203355_add_admin_retention_metrics.sql`:
- Around line 1-10: The rollup table daily_revenue_metrics lacks per-customer
opening/prior-day MRR so NRR cannot distinguish new-day upsells from true
retained expansion; add a column (e.g., opening_mrr or prior_day_mrr) of type
double precision NOT NULL DEFAULT 0 to the table and ensure any ETL/insert logic
that populates daily_revenue_metrics (the rows identified by PRIMARY KEY
daily_revenue_metrics_pkey on (date_id, customer_id)) sets this field to the
customer's MRR at the start of the day (or store a full per-customer daily
snapshot instead), so downstream NRR calculations can use opening_mrr +
expansion/contraction/churn to compute true retention metrics.

---

Outside diff comments:
In `@supabase/functions/_backend/triggers/stripe_event.ts`:
- Around line 617-625: The code currently updates stripe_info (the update using
supabaseAdmin(...).from('stripe_info').update(...)) and then calls
recordRevenueMovement(...) separately, causing lost revenue if the second write
fails; fix by making the subscription-state write and the revenue-rollup write
atomic: either wrap the stripe_info update and the operations performed by
recordRevenueMovement into a single DB transaction (execute both SQL statements
inside one BEGIN/COMMIT using the same supabaseAdmin(c) connection or a stored
procedure that performs both updates atomically) so that the update and revenue
rollup succeed or fail together, or change the flow to first persist an
idempotent event ledger row (insert into a stripe_events/event_ledger table) and
then derive both stripe_info and daily_revenue_metrics from that ledger in a
separate, idempotent worker run (insert ledger row before calling
recordRevenueMovement); update both places referenced (the stripe_info update +
the call to recordRevenueMovement at this location and the other occurrence
around lines 849-853) to use the chosen atomic approach and ensure error
handling returns failures when the transaction/ledger insert fails.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8d0da97c-ed3c-48c0-96c1-f001f913ad1a

📥 Commits

Reviewing files that changed from the base of the PR and between c299d70 and d1fe539.

📒 Files selected for processing (10)
  • src/components/admin/AdminMultiLineChart.vue
  • src/pages/admin/dashboard/revenue.vue
  • src/types/supabase.types.ts
  • supabase/functions/_backend/triggers/logsnag_insights.ts
  • supabase/functions/_backend/triggers/stripe_event.ts
  • supabase/functions/_backend/utils/pg.ts
  • supabase/functions/_backend/utils/supabase.types.ts
  • supabase/migrations/20260422203355_add_admin_retention_metrics.sql
  • tests/logsnag-insights-revenue.unit.test.ts
  • tests/stripe-revenue-movement.unit.test.ts

Comment thread src/pages/admin/dashboard/revenue.vue
Comment thread supabase/migrations/20260422203355_add_admin_retention_metrics.sql
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d1fe539637

ℹ️ 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".

Comment thread supabase/functions/_backend/triggers/stripe_event.ts Outdated
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@supabase/functions/_backend/triggers/logsnag_insights.ts`:
- Around line 535-540: In the catch block inside getRevenueRetentionMetrics
(where cloudlogErr is called), do not return the fake metrics { churnRevenue: 0,
nrr: 100 }; instead rethrow the error (or throw a new Error after logging) so
the failure bubbles up and the caller that upserts into public.global_stats can
detect the read failure and skip writing those fields; likewise remove the
analogous default-return behavior at the other catch (around lines referenced in
the review) so transient read failures don't produce permanent "healthy"
metrics.

In `@supabase/functions/_backend/triggers/stripe_event.ts`:
- Around line 342-360: The daily revenue rollup is not idempotent because the
INSERT into public.daily_revenue_metrics (in the shouldRecordMovement branch)
only keys on (date_id, customer_id) and will re-add the same delta if Stripe
re-sends an event; update the logic to record and check the Stripe event id
before applying deltas: add/store a unique event identifier (e.g., event_id or
processed_event_id) associated with the metric row or in a separate table, check
that the current Stripe event.id has not already been processed for that
date/customer, and only then apply the INSERT ... ON CONFLICT update of
daily_revenue_metrics (or skip) so retries are no-ops; refer to
shouldRecordMovement, the INSERT into public.daily_revenue_metrics, and the
Stripe event.id when making the change.
- Around line 683-692: The revenue classification uses the live
currentStripeInfo row and can be incorrect when events arrive out-of-order;
change the created/updated path (where classifyRevenueMovement is called before
persistStripeInfoAndRevenueMovement) to either (A) reject any eventOlderThanRow
by comparing eventOccurredAtIso to the persisted row timestamp on
currentStripeInfo and skip persisting when eventOccurredAtIso <=
currentStripeInfo.updated_at, or (B) compute revenue deltas from the event's
previous state instead of currentStripeInfo by extracting the prior values from
the webhook payload and passing that to classifyRevenueMovement; update the
logic around classifyRevenueMovement, the updateData assignment (upgraded_at),
and the call to persistStripeInfoAndRevenueMovement to enforce this timestamp or
previous-state gating so stale events cannot overwrite newer data.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c51aea36-cd77-48b5-ad46-01047b675139

📥 Commits

Reviewing files that changed from the base of the PR and between d1fe539 and 8fa9b5c.

📒 Files selected for processing (6)
  • src/pages/admin/dashboard/revenue.vue
  • src/types/supabase.types.ts
  • supabase/functions/_backend/triggers/logsnag_insights.ts
  • supabase/functions/_backend/triggers/stripe_event.ts
  • supabase/functions/_backend/utils/supabase.types.ts
  • supabase/migrations/20260422203355_add_admin_retention_metrics.sql
✅ Files skipped from review due to trivial changes (2)
  • supabase/functions/_backend/utils/supabase.types.ts
  • src/types/supabase.types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/pages/admin/dashboard/revenue.vue

Comment thread supabase/functions/_backend/triggers/logsnag_insights.ts Outdated
Comment thread supabase/functions/_backend/triggers/stripe_event.ts
Comment thread supabase/functions/_backend/triggers/stripe_event.ts
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

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/logsnag_insights.ts (1)

873-903: ⚠️ Potential issue | 🟠 Major

A retention read error now aborts the entire stats snapshot.

getRevenueRetentionMetrics() rethrows, but this Promise.all() still assumes the promise always resolves and newData always dereferences it. A transient issue in daily_revenue_metrics or the previous-day global_stats row will now skip the whole global_stats upsert instead of just leaving nrr / churn_revenue unset for that day. Catch this promise separately and only write those two fields when the read succeeds.

💡 Minimal fix
-    res.retention_metrics,
+    res.retention_metrics.catch((error) => {
+      cloudlogErr({ requestId: c.get('requestId'), message: 'retention metrics unavailable', error })
+      return null
+    }),

Then gate the newData assignment so nrr and churn_revenue are only included when retention_metrics is non-null.

Also applies to: 961-962

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@supabase/functions/_backend/triggers/logsnag_insights.ts` around lines 873 -
903, The Promise.all call that awaits res.retention_metrics (from
getRevenueRetentionMetrics()) must not let a rejection abort the whole snapshot:
change the call-site to fetch retention_metrics separately (call
getRevenueRetentionMetrics() or catch the individual promise) so failures are
handled independently, and modify the newData construction (the code that writes
nrr and churn_revenue) to only add those two fields when retention_metrics is
present/non-null; update the code paths that read/write retention_metrics (the
variable retention_metrics and the newData assignment that references
nrr/churn_revenue) so a transient read error only leaves those fields unset
instead of aborting the global_stats upsert (also apply the same separation
where retention_metrics is used again around the other referenced lines).
♻️ Duplicate comments (1)
supabase/functions/_backend/triggers/stripe_event.ts (1)

352-370: ⚠️ Potential issue | 🟠 Major

Webhook retries still double-count daily revenue deltas.

This UPSERT is keyed only by (date_id, customer_id) and increments the delta columns on every execution. If Stripe re-delivers the same event, the same new_business_mrr / expansion_mrr / contraction_mrr / churn_mrr gets added again, overstating the day's retention data. Persist a unique processed Stripe event id and make retries a no-op before applying the additive update.

🤖 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 352 - 370,
The UPSERT into public.daily_revenue_metrics (executed when shouldRecordMovement
is true via pgClient.query in the Stripe webhook handler) is additive and will
double-count on Stripe retries; make the operation idempotent by persisting and
checking a unique Stripe event id before applying the additive update: add or
use a processed_events (or processed_stripe_events) table with a unique
constraint on event_id, attempt to insert the current Stripe event id first and
only proceed to run the INSERT ... ON CONFLICT ... DO UPDATE that increments
new_business_mrr/expansion_mrr/contraction_mrr/churn_mrr if the insert of the
event id succeeded (or otherwise skip), or alternatively add a
processed_event_id column/unique constraint on public.daily_revenue_metrics and
gate the additive update with a WHERE NOT EXISTS check so repeated deliveries
become a no-op; update the logic surrounding shouldRecordMovement and the
pgClient.query to perform this idempotency check using the Stripe event id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@supabase/functions/_backend/triggers/stripe_event.ts`:
- Around line 707-715: The code calls persistStripeInfoAndRevenueMovement and
then calls quickError() but never returns its Response, so the route still
continues to cancelingOrFinished and ACKs the webhook; update the caller in
createdOrUpdated (the place awaiting persistStripeInfoAndRevenueMovement) to
either throw when persistStripeInfoAndRevenueMovement returns false or capture
and return the helper Response—e.g., have createdOrUpdated return the quickError
Response (or throw) and in the top-level route check the createdOrUpdated return
value and immediately return it (if present) before proceeding to
cancelingOrFinished; reference functions: persistStripeInfoAndRevenueMovement,
createdOrUpdated, cancelingOrFinished, and quickError.
- Around line 289-305: The current getRevenuePlans function swallows DB read
errors and returns [], causing classifyRevenueMovement to treat the failure as
“no revenue movement”; instead make the webhook fail on read errors so Stripe
will retry: after logging the error in getRevenuePlans (the
supabaseAdmin(...).from('plans').select(...) block), throw the error (or return
a rejected Promise) rather than returning an empty array so the caller
(classifyRevenueMovement / webhook handler) surfaces the failure and aborts the
webhook processing.

---

Outside diff comments:
In `@supabase/functions/_backend/triggers/logsnag_insights.ts`:
- Around line 873-903: The Promise.all call that awaits res.retention_metrics
(from getRevenueRetentionMetrics()) must not let a rejection abort the whole
snapshot: change the call-site to fetch retention_metrics separately (call
getRevenueRetentionMetrics() or catch the individual promise) so failures are
handled independently, and modify the newData construction (the code that writes
nrr and churn_revenue) to only add those two fields when retention_metrics is
present/non-null; update the code paths that read/write retention_metrics (the
variable retention_metrics and the newData assignment that references
nrr/churn_revenue) so a transient read error only leaves those fields unset
instead of aborting the global_stats upsert (also apply the same separation
where retention_metrics is used again around the other referenced lines).

---

Duplicate comments:
In `@supabase/functions/_backend/triggers/stripe_event.ts`:
- Around line 352-370: The UPSERT into public.daily_revenue_metrics (executed
when shouldRecordMovement is true via pgClient.query in the Stripe webhook
handler) is additive and will double-count on Stripe retries; make the operation
idempotent by persisting and checking a unique Stripe event id before applying
the additive update: add or use a processed_events (or processed_stripe_events)
table with a unique constraint on event_id, attempt to insert the current Stripe
event id first and only proceed to run the INSERT ... ON CONFLICT ... DO UPDATE
that increments new_business_mrr/expansion_mrr/contraction_mrr/churn_mrr if the
insert of the event id succeeded (or otherwise skip), or alternatively add a
processed_event_id column/unique constraint on public.daily_revenue_metrics and
gate the additive update with a WHERE NOT EXISTS check so repeated deliveries
become a no-op; update the logic surrounding shouldRecordMovement and the
pgClient.query to perform this idempotency check using the Stripe event id.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 96e712b9-4073-464a-9b99-1c5f0c91f11b

📥 Commits

Reviewing files that changed from the base of the PR and between 8fa9b5c and 7a18a1c.

📒 Files selected for processing (3)
  • supabase/functions/_backend/triggers/logsnag_insights.ts
  • supabase/functions/_backend/triggers/stripe_event.ts
  • tests/stripe-revenue-movement.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/stripe-revenue-movement.unit.test.ts

Comment thread supabase/functions/_backend/triggers/stripe_event.ts
Comment thread supabase/functions/_backend/triggers/stripe_event.ts
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
supabase/functions/_backend/triggers/stripe_event.ts (2)

723-725: ⚠️ Potential issue | 🟠 Major

Revenue movement still depends on webhook arrival order.

These call sites classify against the current stripe_info row, not the event’s actual prior Stripe state. If Stripe delivers a later same-day downgrade before an earlier upgrade, you can record the downgrade as the wrong bucket and then skip the real earlier event as stale. That distorts expansion_mrr / contraction_mrr / churn_mrr in public.daily_revenue_metrics, which then flows into the admin NRR and churn charts.

Please derive the “before” state from the webhook’s previous state (or another event-time-aware source), rather than from the last persisted row.

Also applies to: 978-979

🤖 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 723 - 725,
The revenue classification currently uses the persisted currentStripeInfo
instead of the webhook's prior state, causing incorrect buckets when events
arrive out of order; update classifyRevenueMovement and its call sites (where
getRevenuePlans, classifyRevenueMovement, currentStripeInfo, and updateData are
used) to accept and use the event-provided "previous" Stripe state (e.g.,
event.previous_stripe_info or previousStripeInfo extracted from the webhook
payload) as the "before" baseline for comparisons instead of the DB row, and
change all call sites (including the other occurrences referenced) to pass that
previous state so movement is computed against the event's prior state.

291-304: ⚠️ Potential issue | 🟠 Major

Do not downgrade plans read failures into “no revenue movement.”

Returning [] here makes classifyRevenueMovement() fall back to zero MRR while the transactional stripe_info update can still commit. That permanently under-reports daily_revenue_metrics, and logsnag_insights.ts builds NRR/churn directly from that table. Let the webhook fail so Stripe retries with complete accounting.

Suggested fix
   if (error) {
     cloudlog({
       requestId: c.get('requestId'),
       message: 'Failed to load revenue plans for Stripe revenue movement tracking',
       error,
     })
-    return []
+    throw simpleError('revenue_plans_unavailable', 'Failed to load revenue plans for revenue movement tracking', { error })
   }
🤖 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 291 - 304,
The current getRevenuePlans function swallows Supabase read failures by logging
then returning [], causing classifyRevenueMovement to treat failures as zero
MRR; instead, after logging the error in getRevenuePlans (use c.get('requestId')
and include error details), rethrow the original error or throw a new Error so
the webhook handler fails and Stripe will retry; update getRevenuePlans (and
callers like classifyRevenueMovement/stripe_event handler) to not treat a thrown
error as a normal empty-plan path so transactional updates (stripe_info) cannot
commit on incomplete accounting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@supabase/functions/_backend/triggers/stripe_event.ts`:
- Around line 281-289: The staleness check in isStaleStripeEvent (and similar
checks at the other noted locations) is comparing Stripe event timestamps
against the row write timestamp updated_at, which can be updated by unrelated
writes; change those checks to use a dedicated persisted timestamp field that
records the timestamp of the last applied Stripe event (e.g.,
last_stripe_event_at or stripe_event_processed_at) instead of updated_at. Update
the function isStaleStripeEvent to accept/read that dedicated field (replace
currentStripeInfo.updated_at usage), and update the other occurrences you
pointed out so the comparison uses the dedicated Stripe-last-processed timestamp
field whenever deciding whether an incoming eventOccurredAtIso is stale. Ensure
the dedicated field is only written when a Stripe event's state is applied so
ordering is correct.

---

Duplicate comments:
In `@supabase/functions/_backend/triggers/stripe_event.ts`:
- Around line 723-725: The revenue classification currently uses the persisted
currentStripeInfo instead of the webhook's prior state, causing incorrect
buckets when events arrive out of order; update classifyRevenueMovement and its
call sites (where getRevenuePlans, classifyRevenueMovement, currentStripeInfo,
and updateData are used) to accept and use the event-provided "previous" Stripe
state (e.g., event.previous_stripe_info or previousStripeInfo extracted from the
webhook payload) as the "before" baseline for comparisons instead of the DB row,
and change all call sites (including the other occurrences referenced) to pass
that previous state so movement is computed against the event's prior state.
- Around line 291-304: The current getRevenuePlans function swallows Supabase
read failures by logging then returning [], causing classifyRevenueMovement to
treat failures as zero MRR; instead, after logging the error in getRevenuePlans
(use c.get('requestId') and include error details), rethrow the original error
or throw a new Error so the webhook handler fails and Stripe will retry; update
getRevenuePlans (and callers like classifyRevenueMovement/stripe_event handler)
to not treat a thrown error as a normal empty-plan path so transactional updates
(stripe_info) cannot commit on incomplete accounting.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fad72906-41a5-4f73-bad3-9ba47150fb00

📥 Commits

Reviewing files that changed from the base of the PR and between 7a18a1c and a1a1528.

📒 Files selected for processing (1)
  • supabase/functions/_backend/triggers/stripe_event.ts

Comment thread supabase/functions/_backend/triggers/stripe_event.ts
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@sonarqubecloud
Copy link
Copy Markdown

@riderx riderx merged commit 956ae21 into main Apr 23, 2026
15 checks passed
@riderx riderx deleted the codex/admin-nrr-churn-charts branch April 23, 2026 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant