[codex] add retention metrics backfill#1938
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Bun/TypeScript CLI to backfill daily Stripe retention metrics, centralizes revenue-metrics logic into a new shared utils module, updates Supabase triggers to use that module, and adds unit tests and a package script to run the backfill. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Backfill CLI
participant Utils as Revenue Utils
participant DB as Database
participant Stripe as Stripe API
CLI->>DB: Load plans & stripe_info baselines
DB-->>CLI: RevenuePlanRow[], StripeInfo rows
alt events from file
CLI->>CLI: Read events from --events-file
else fetch from Stripe
CLI->>Stripe: Fetch subscription events (range/customer)
Stripe-->>CLI: Stripe.Event[]
end
CLI->>Utils: buildRevenueMovementEvents(events, plans, baselines)
Utils-->>CLI: BackfillRevenueMovementEvent[]
CLI->>Utils: aggregateRevenueMovementEvents(movements)
Utils-->>CLI: DailyRevenueMetricInsert[]
alt --reset
CLI->>DB: Delete processed_stripe_events & daily_revenue_metrics (range)
DB-->>CLI: Deleted
end
CLI->>DB: Upsert daily_revenue_metrics & insert processed_stripe_events
DB-->>CLI: Upserted/Inserted
CLI->>DB: Recompute/Update global_stats (churn, NRR) for dates
DB-->>CLI: Updated/Skipped (if missing)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![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: b80e0c6d4f
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/backfill_retention_metrics.ts (1)
615-633: Verify insert safety without ON CONFLICT.
insertProcessedEventsuses plain INSERT without conflict handling. This is safe because:
- Non-reset mode: Line 862 filters out already-processed event IDs
- Reset mode: Line 878 deletes existing rows first
However, if there's a partial failure and retry, this could cause duplicate key errors. Consider adding
ON CONFLICT DO NOTHINGfor idempotency.♻️ Add conflict handling for retry safety
async function insertProcessedEvents(supabase: SupabaseClient, movements: BackfillRevenueMovementEvent[]) { const rows: ProcessedStripeEventInsert[] = movements.map(movement => ({ event_id: movement.event_id, customer_id: movement.customer_id, date_id: movement.date_id, })) for (const chunk of chunkArray(rows, DB_CHUNK_SIZE)) { if (chunk.length === 0) continue const { error } = await supabase .from('processed_stripe_events') - .insert(chunk) + .upsert(chunk, { onConflict: 'event_id', ignoreDuplicates: true }) if (error) throw error } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/backfill_retention_metrics.ts` around lines 615 - 633, Change insertProcessedEvents to make inserts idempotent: instead of a plain insert into the processed_stripe_events table, perform an INSERT ... ON CONFLICT DO NOTHING (or use Supabase's upsert API with the conflict key) so retries or partial failures don't raise duplicate-key errors; locate the insert call in insertProcessedEvents (uses chunkArray and DB_CHUNK_SIZE) and apply conflict handling targeting the unique event key (event_id).supabase/functions/_backend/utils/revenue_metrics.ts (1)
74-79: Optional chaining could simplify the condition (optional).SonarCloud suggests using optional chaining. The current explicit null check is clear and correct, but optional chaining is more concise.
♻️ Use optional chaining (optional)
export function getSubscriptionMrr(plans: RevenuePlanRow[], stripeInfo: StripeInfoRevenueState) { - if (!stripeInfo || stripeInfo.status !== 'succeeded' || stripeInfo.is_good_plan === false) + if (stripeInfo?.status !== 'succeeded' || stripeInfo?.is_good_plan === false) return 0 return getPlanMrr(getPlanByProductId(plans, stripeInfo.product_id), stripeInfo.price_id) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/utils/revenue_metrics.ts` around lines 74 - 79, Replace the explicit null check in getSubscriptionMrr with optional chaining to simplify the condition: instead of checking "if (!stripeInfo || stripeInfo.status !== 'succeeded' || stripeInfo.is_good_plan === false) return 0", use optional chaining on stripeInfo fields (refer to getSubscriptionMrr, stripeInfo.status, stripeInfo.is_good_plan) so the guard reads concisely and still returns 0 when stripeInfo is missing or its status/is_good_plan indicate failure, then return getPlanMrr(getPlanByProductId(plans, stripeInfo.product_id), stripeInfo.price_id) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/backfill_retention_metrics.ts`:
- Around line 615-633: Change insertProcessedEvents to make inserts idempotent:
instead of a plain insert into the processed_stripe_events table, perform an
INSERT ... ON CONFLICT DO NOTHING (or use Supabase's upsert API with the
conflict key) so retries or partial failures don't raise duplicate-key errors;
locate the insert call in insertProcessedEvents (uses chunkArray and
DB_CHUNK_SIZE) and apply conflict handling targeting the unique event key
(event_id).
In `@supabase/functions/_backend/utils/revenue_metrics.ts`:
- Around line 74-79: Replace the explicit null check in getSubscriptionMrr with
optional chaining to simplify the condition: instead of checking "if
(!stripeInfo || stripeInfo.status !== 'succeeded' || stripeInfo.is_good_plan ===
false) return 0", use optional chaining on stripeInfo fields (refer to
getSubscriptionMrr, stripeInfo.status, stripeInfo.is_good_plan) so the guard
reads concisely and still returns 0 when stripeInfo is missing or its
status/is_good_plan indicate failure, then return
getPlanMrr(getPlanByProductId(plans, stripeInfo.product_id),
stripeInfo.price_id) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 81a5e836-4617-4b41-9300-e088f85c8ef1
📒 Files selected for processing (6)
package.jsonscripts/backfill_retention_metrics.tssupabase/functions/_backend/triggers/logsnag_insights.tssupabase/functions/_backend/triggers/stripe_event.tssupabase/functions/_backend/utils/revenue_metrics.tstests/backfill-retention-metrics.unit.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/backfill_retention_metrics.ts (1)
884-897: Consider the non-atomic nature of reset mode.When
--resetis used, the delete operation (line 885) completes before the insert/upsert (lines 896-897). If the subsequent operations fail, the deleted data is lost until the script is re-run successfully.This is acceptable for a backfill script since the source of truth is Stripe and the operation can be repeated, but it may be worth documenting this behavior in the header comments or adding a warning log when
--resetis used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/backfill_retention_metrics.ts` around lines 884 - 897, The reset flow is non-atomic: calling resetBackfillRange(...) before inserts/upserts can lead to data loss if subsequent insertProcessedEvents(...) or upsertDailyRevenueMetrics(...) fail; update the script to make this explicit by logging a clear warning when the reset flag is provided (include the reset variable name), and add/update header comments to document that reset will delete existing rows immediately and the operation must be re-run on failure; reference resetBackfillRange, insertProcessedEvents, upsertDailyRevenueMetrics, and the reset flag so reviewers can locate and verify the added warning and documentation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/backfill_retention_metrics.ts`:
- Around line 497-511: The loadEventsFile function currently casts parsed JSON
to Stripe.Event[] without element validation; add minimal runtime checks in
loadEventsFile to verify each event object has the required properties
(id:string, type:string, created:number or parsable date, and
data?.object?.customer present) before calling sortStripeEvents and returning,
and throw a clear Error if any event is malformed so buildRevenueMovementEvents
never receives invalid input; reference the loadEventsFile function and the
subsequent sortStripeEvents/buildRevenueMovementEvents flow when implementing
the checks.
---
Nitpick comments:
In `@scripts/backfill_retention_metrics.ts`:
- Around line 884-897: The reset flow is non-atomic: calling
resetBackfillRange(...) before inserts/upserts can lead to data loss if
subsequent insertProcessedEvents(...) or upsertDailyRevenueMetrics(...) fail;
update the script to make this explicit by logging a clear warning when the
reset flag is provided (include the reset variable name), and add/update header
comments to document that reset will delete existing rows immediately and the
operation must be re-run on failure; reference resetBackfillRange,
insertProcessedEvents, upsertDailyRevenueMetrics, and the reset flag so
reviewers can locate and verify the added warning and documentation.
🪄 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: 3106b729-45b7-40b8-9b49-02f9c2a469e1
📒 Files selected for processing (2)
scripts/backfill_retention_metrics.tstests/backfill-retention-metrics.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/backfill-retention-metrics.unit.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4126c13f11
ℹ️ 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".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/backfill_retention_metrics.ts`:
- Around line 449-470: The code forces previous state to look billable by
falling back to currentPriceId/currentProductId and status:'succeeded' when
previousItem is missing, causing updates that are actually "new paid activation"
to be classified as no movement; change the previous-state derivation so
previousPriceId/previousProductId do NOT default to
currentPriceId/currentProductId (use trackedState?.price_id or null) and compute
previousStatus from trackedState?.status or a non-billable sentinel (e.g.,
'none'/'cancelled'/'unpaid') instead of hardcoding 'succeeded'; pass that
derived previousStatus to getSubscriptionMrr when computing previousMrr, and
ensure buildTrackedState calls (for the customer.subscription.updated branch)
use activePaidAt but keep previous price/product as null when unknown so
classifyRevenueMovement will see a true non-billable→paid transition.
- Around line 947-953: The processed-event marking must be made atomic with the
metric writes: replace the separate calls to insertProcessedEvents,
upsertDailyRevenueMetrics and refreshGlobalRetentionMetrics with a single
transactional operation (e.g., an RPC/SQL function like
upsert_metrics_and_mark_processed) that accepts movementsToApply,
mergedMetricRows and retentionDates and within one DB transaction performs the
upsert into daily_revenue_metrics, the refresh/update of global_stats (or calls
refreshGlobalRetentionMetrics logic), and only then inserts into
processed_stripe_events; alternatively implement a resumable two-phase state
(mark events as "pending" first, run metric upserts and refresh, then atomically
flip to "processed") so that insertProcessedEvents, upsertDailyRevenueMetrics
and refreshGlobalRetentionMetrics are not independently committed and cannot
leave processed markers without corresponding metric/refresh writes.
🪄 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: 5071a90a-a8db-40fb-93ba-96a698626235
📒 Files selected for processing (2)
scripts/backfill_retention_metrics.tssupabase/functions/_backend/utils/revenue_metrics.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6476d53f89
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af720342dd
ℹ️ 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".
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/backfill_retention_metrics.ts (2)
1010-1012: Redundant null check fordatabaseUrl.Line 952 already throws if
apply && !databaseUrl, sodatabaseUrlis guaranteed non-null here. The extra check and local variable are unnecessary.♻️ Remove redundant check
- const applyDatabaseUrl = databaseUrl - if (!applyDatabaseUrl) - throw new Error('Missing direct database URL for apply') - const existingMetrics = reset ? [] : await fetchExistingDailyRevenueMetrics(supabase, fromDateId, toDateId, customerId) @@ ... @@ try { const refreshResult = await applyBackfillTransaction({ customerId, - databaseUrl: applyDatabaseUrl, + databaseUrl: databaseUrl!, fromDateId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/backfill_retention_metrics.ts` around lines 1010 - 1012, Remove the redundant local variable and null-check: since earlier code already throws when apply && !databaseUrl, you can eliminate the applyDatabaseUrl assignment and the subsequent if (!applyDatabaseUrl) throw; simply use the existing databaseUrl directly (or assume it's non-null after the earlier guard) and delete the lines referencing applyDatabaseUrl and its check to avoid duplication.
341-343: Fallback to first subscription item may select metered pricing.When no
licensedusage type item is found, the function falls back toitems?.[0]. If a subscription has multiple items with a metered item first, this could select incorrect pricing for MRR calculations.Consider logging a warning when falling back to ensure visibility during backfill runs.
🔧 Optional: Add fallback warning
function getLicensedSubscriptionItem(items: Stripe.SubscriptionItem[] | undefined) { - return items?.find(item => item.plan?.usage_type === 'licensed') ?? items?.[0] ?? null + const licensed = items?.find(item => item.plan?.usage_type === 'licensed') + if (licensed) + return licensed + if (items?.[0]) + console.warn(`No licensed item found, falling back to first item: ${items[0].id}`) + return items?.[0] ?? null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/backfill_retention_metrics.ts` around lines 341 - 343, The function getLicensedSubscriptionItem currently falls back to items?.[0], which can pick a metered item; change it to return null when no item with plan?.usage_type === 'licensed' is found, and add a warning log when items exist but no licensed item is present so backfill runs surface this case; update the function getLicensedSubscriptionItem to detect the fallback condition (items && !licensedItem) and call the project logger or console.warn with context (subscription items count and the first item's plan?.usage_type) instead of silently returning the first item.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/backfill_retention_metrics.ts`:
- Around line 1010-1012: Remove the redundant local variable and null-check:
since earlier code already throws when apply && !databaseUrl, you can eliminate
the applyDatabaseUrl assignment and the subsequent if (!applyDatabaseUrl) throw;
simply use the existing databaseUrl directly (or assume it's non-null after the
earlier guard) and delete the lines referencing applyDatabaseUrl and its check
to avoid duplication.
- Around line 341-343: The function getLicensedSubscriptionItem currently falls
back to items?.[0], which can pick a metered item; change it to return null when
no item with plan?.usage_type === 'licensed' is found, and add a warning log
when items exist but no licensed item is present so backfill runs surface this
case; update the function getLicensedSubscriptionItem to detect the fallback
condition (items && !licensedItem) and call the project logger or console.warn
with context (subscription items count and the first item's plan?.usage_type)
instead of silently returning the first item.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20028b26-ed1f-43ee-8ec2-ad3ba61ee94f
📒 Files selected for processing (2)
scripts/backfill_retention_metrics.tstests/backfill-retention-metrics.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/backfill-retention-metrics.unit.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/backfill_retention_metrics.ts (2)
186-193: SSL certificate validation disabled for non-local connections.
rejectUnauthorized: falsedisables SSL certificate verification. While this is sometimes necessary for managed database services (e.g., Supabase connection poolers with self-signed certs), it exposes the connection to potential MITM attacks. Consider adding a comment explaining why this is needed or making it configurable via environment variable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/backfill_retention_metrics.ts` around lines 186 - 193, The createPgClient function currently sets ssl: { rejectUnauthorized: false } for non-local hosts which disables SSL certificate verification; change this to make rejection configurable and add a clarifying comment: keep rejectUnauthorized defaulting to true (validating certs) and only set it to false when an explicit environment variable (e.g., PG_ALLOW_SELF_SIGNED_CERT=true or PG_SSL_REJECT_UNAUTHORIZED=0) is present, and document in a comment why/when disabling verification is allowed (e.g., for specific managed pools with self-signed certs); update the ssl construction in createPgClient to read that env var and include the explanatory comment next to the ssl option.
399-410: Type safety: Returning falsystatedoesn't match declared return type.The function returns
statewhen falsy (line 401), but the return type isStripeInfoRevenueState, notStripeInfoRevenueState | null | undefined. This creates a type inconsistency.Consider either:
- Updating the return type to
StripeInfoRevenueState | null- Throwing an error for invalid input
- Removing the check if callers guarantee non-null input
♻️ Proposed fix
-function toRevenueState(state: TrackedSubscriptionState | StripeInfoRevenueState): StripeInfoRevenueState { - if (!state) - return state +function toRevenueState(state: TrackedSubscriptionState | StripeInfoRevenueState | null): StripeInfoRevenueState | null { + if (!state) + return null return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/backfill_retention_metrics.ts` around lines 399 - 410, toRevenueState currently returns a falsy state but is declared to return StripeInfoRevenueState, causing a type mismatch; change the function signature to return StripeInfoRevenueState | null (or | undefined) and return null instead of state when input is falsy, and update callers to handle the nullable return; alternatively, if callers guarantee non-null input, remove the falsy check and keep the signature—refer to toRevenueState, TrackedSubscriptionState, and StripeInfoRevenueState when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/backfill_retention_metrics.ts`:
- Around line 186-193: The createPgClient function currently sets ssl: {
rejectUnauthorized: false } for non-local hosts which disables SSL certificate
verification; change this to make rejection configurable and add a clarifying
comment: keep rejectUnauthorized defaulting to true (validating certs) and only
set it to false when an explicit environment variable (e.g.,
PG_ALLOW_SELF_SIGNED_CERT=true or PG_SSL_REJECT_UNAUTHORIZED=0) is present, and
document in a comment why/when disabling verification is allowed (e.g., for
specific managed pools with self-signed certs); update the ssl construction in
createPgClient to read that env var and include the explanatory comment next to
the ssl option.
- Around line 399-410: toRevenueState currently returns a falsy state but is
declared to return StripeInfoRevenueState, causing a type mismatch; change the
function signature to return StripeInfoRevenueState | null (or | undefined) and
return null instead of state when input is falsy, and update callers to handle
the nullable return; alternatively, if callers guarantee non-null input, remove
the falsy check and keep the signature—refer to toRevenueState,
TrackedSubscriptionState, and StripeInfoRevenueState when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbdde97e-4dd9-47c2-b3d1-7167d9103af1
📒 Files selected for processing (2)
scripts/backfill_retention_metrics.tstests/backfill-retention-metrics.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/backfill-retention-metrics.unit.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 392f6be34f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33fcbc4afe
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bba9a9310c
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ea39557c8
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7169c3240e
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfe95c5211
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64c15bdf21
ℹ️ 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".
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91bc95e4e1
ℹ️ 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".
| if (apply && reset && reachedEventFetchLimit) { | ||
| throw new Error('--apply --reset cannot use a truncated Stripe event snapshot. Increase or remove --limit, or provide --events-file.') | ||
| } |
There was a problem hiding this comment.
--apply currently proceeds when fetchStripeEvents hit --limit unless --reset is also set, which means a partial event snapshot can still be written. In that path, processed_stripe_events markers are committed for only the truncated subset, and later reruns cannot safely recompute those already-claimed events with full historical context, so retention deltas can remain permanently skewed unless a reset is performed. This should fail fast (or force dry-run) whenever reachedEventFetchLimit is true and writes are requested.
Useful? React with 👍 / 👎.
1 participant
Summary (AI generated)
stripe:backfill-retention-metricsto backfill daily churn revenue and NRR source data from Stripe subscription events.Motivation (AI generated)
The admin MRR chart now has NRR and churn revenue metrics, but existing historical rows need a controlled operational backfill path. The script supports dry-run review first, append-only processing for new events, exact range rebuilds with
--reset, customer scoping, and--events-filefor archived Stripe events beyond live event-list availability.Business Impact (AI generated)
This lets the admin revenue dashboard show meaningful retention and lost-revenue history instead of starting empty after deploy. It improves visibility into churn, expansion, contraction, and retained revenue trends without manual database edits.
Test Plan (AI generated)
bun lintbun lint:backendbun typecheckbun run supabase:with-env -- bunx vitest run tests/stripe-revenue-movement.unit.test.ts tests/logsnag-insights-revenue.unit.test.ts tests/backfill-retention-metrics.unit.test.tsbun test:backendbun run supabase:with-env -- bunx vitest run tests/cli.test.tsGenerated with AI
Summary by CodeRabbit
New Features
Refactor
Tests