fix: use MAIN_SUPABASE_DB_URL for retention backfill#1953
Conversation
|
Warning Rate limit exceeded
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 5 minutes and 45 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR exports two database URL helper functions from the backfill retention metrics script and adjusts environment variable lookup order to prioritize Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
![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)
215-229: Avoid duplicating the DB env-key list across resolver and error message.The key list is currently duplicated in logic and text. Centralizing it prevents drift and keeps future changes safer.
Refactor sketch
+const DATABASE_URL_ENV_KEYS = [ + 'MAIN_SUPABASE_DB_URL', + 'DATABASE_URL', + 'POSTGRES_URL', + 'SUPABASE_DB_URL', + 'SUPABASE_DB_DIRECT_URL', + 'DIRECT_URL', +] as const + export function getRequiredDatabaseUrl(env: Record<string, string | undefined>) { const value = getDatabaseUrl(env) if (!value) - throw new Error('--apply requires MAIN_SUPABASE_DB_URL, DATABASE_URL, POSTGRES_URL, SUPABASE_DB_URL, SUPABASE_DB_DIRECT_URL, or DIRECT_URL so metric writes and processed-event markers are committed atomically') + throw new Error(`--apply requires ${DATABASE_URL_ENV_KEYS.join(', ')} so metric writes and processed-event markers are committed atomically`) return value } export function getDatabaseUrl(env: Record<string, string | undefined>) { - return env.MAIN_SUPABASE_DB_URL?.trim() - || env.DATABASE_URL?.trim() - || env.POSTGRES_URL?.trim() - || env.SUPABASE_DB_URL?.trim() - || env.SUPABASE_DB_DIRECT_URL?.trim() - || env.DIRECT_URL?.trim() - || null + for (const key of DATABASE_URL_ENV_KEYS) { + const value = env[key]?.trim() + if (value) + return value + } + return 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 215 - 229, Declare a single constant array (e.g., DATABASE_ENV_KEYS) containing the env key strings now duplicated, then update getDatabaseUrl to iterate that array (return first non-empty trimmed value or null) and update getRequiredDatabaseUrl to build its error message using the same array (e.g., join(', ') or format with human-readable separators) so the list of env keys is defined once and reused by getDatabaseUrl and getRequiredDatabaseUrl; reference the existing functions getDatabaseUrl and getRequiredDatabaseUrl when making these changes.tests/backfill-retention-metrics.unit.test.ts (1)
375-386: Add one fallback-order regression test whenMAIN_SUPABASE_DB_URLis absent.Current assertions validate the main writer path, but not the non-main fallback order. A small extra test will guard against accidental resolver reordering.
Proposed test addition
it.concurrent('accepts MAIN_SUPABASE_DB_URL as the required apply database url', () => { expect(getRequiredDatabaseUrl({ MAIN_SUPABASE_DB_URL: 'postgres://main-writer', })).toBe('postgres://main-writer') }) + + it.concurrent('falls back to DATABASE_URL before direct-url env names', () => { + expect(getDatabaseUrl({ + DATABASE_URL: 'postgres://database-url', + SUPABASE_DB_DIRECT_URL: 'postgres://direct', + DIRECT_URL: 'postgres://direct-legacy', + })).toBe('postgres://database-url') + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/backfill-retention-metrics.unit.test.ts` around lines 375 - 386, Add a regression test that covers the non-MAIN fallback ordering: when MAIN_SUPABASE_DB_URL is absent but SUPABASE_DB_URL and SUPABASE_DB_READER are present, assert that getDatabaseUrl(...) returns SUPABASE_DB_URL (and not SUPABASE_DB_READER); add a similar assertion for getRequiredDatabaseUrl(...) if that function should also prefer SUPABASE_DB_URL as the required URL. Reference the existing getDatabaseUrl and getRequiredDatabaseUrl test cases and add the new test alongside them to ensure resolver ordering isn't accidentally changed.
🤖 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 215-229: Declare a single constant array (e.g., DATABASE_ENV_KEYS)
containing the env key strings now duplicated, then update getDatabaseUrl to
iterate that array (return first non-empty trimmed value or null) and update
getRequiredDatabaseUrl to build its error message using the same array (e.g.,
join(', ') or format with human-readable separators) so the list of env keys is
defined once and reused by getDatabaseUrl and getRequiredDatabaseUrl; reference
the existing functions getDatabaseUrl and getRequiredDatabaseUrl when making
these changes.
In `@tests/backfill-retention-metrics.unit.test.ts`:
- Around line 375-386: Add a regression test that covers the non-MAIN fallback
ordering: when MAIN_SUPABASE_DB_URL is absent but SUPABASE_DB_URL and
SUPABASE_DB_READER are present, assert that getDatabaseUrl(...) returns
SUPABASE_DB_URL (and not SUPABASE_DB_READER); add a similar assertion for
getRequiredDatabaseUrl(...) if that function should also prefer SUPABASE_DB_URL
as the required URL. Reference the existing getDatabaseUrl and
getRequiredDatabaseUrl test cases and add the new test alongside them to ensure
resolver ordering isn't accidentally changed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a23e875-32bc-4928-8faf-f3c6f6b207dc
📒 Files selected for processing (2)
scripts/backfill_retention_metrics.tstests/backfill-retention-metrics.unit.test.ts
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
1 participant
Summary (AI generated)
Motivation (AI generated)
The production env file used to run this script exposes the write connection as MAIN_SUPABASE_DB_URL. The backfill script ignored that variable, so --apply aborted before any rows were written even though the required database URL was already configured.
Business Impact (AI generated)
This restores the backfill path for retention metrics in production, which unblocks NRR and churn revenue chart data repair without requiring manual env rewrites or one-off commands.
Test Plan (AI generated)
Generated with AI
Summary by CodeRabbit
Release Notes