fix: allow Supabase pooler TLS for retention backfill#1955
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 2 minutes and 13 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 changes centralize database URL environment variable resolution by exporting helper functions with prioritized env key handling, and update self-signed TLS certificate logic to be URL-aware. The script now decides certificate handling based on explicit env overrides or inferred from the database URL pattern (Supabase pooler detection). Unit tests validate the new precedence rules and TLS behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/backfill-retention-metrics.unit.test.ts (1)
402-414: Add explicitPG_ALLOW_SELF_SIGNED_CERTprecedence tests.Since override precedence is part of the behavior contract, add tests for
PG_ALLOW_SELF_SIGNED_CERT=true/false(including whenPG_SSL_REJECT_UNAUTHORIZEDis also set).🧪 Suggested test additions
+ it.concurrent('honors PG_ALLOW_SELF_SIGNED_CERT=true as highest-priority override', () => { + expect(shouldAllowSelfSignedPgCertificate( + { PG_ALLOW_SELF_SIGNED_CERT: 'true', PG_SSL_REJECT_UNAUTHORIZED: '1' }, + 'postgresql://postgres:secret@db.project-ref.supabase.co:6543/postgres', + )).toBe(true) + }) + + it.concurrent('honors PG_ALLOW_SELF_SIGNED_CERT=false as highest-priority override', () => { + expect(shouldAllowSelfSignedPgCertificate( + { PG_ALLOW_SELF_SIGNED_CERT: 'false', PG_SSL_REJECT_UNAUTHORIZED: '0' }, + 'postgresql://postgres:secret@db.project-ref.supabase.co:6543/postgres', + )).toBe(false) + })🤖 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 402 - 414, Add explicit unit tests for PG_ALLOW_SELF_SIGNED_CERT precedence: update tests/backfill-retention-metrics.unit.test.ts to include cases asserting shouldAllowSelfSignedPgCertificate returns true when PG_ALLOW_SELF_SIGNED_CERT='1' (even if PG_SSL_REJECT_UNAUTHORIZED is set), and returns false when PG_ALLOW_SELF_SIGNED_CERT='0' (including when PG_SSL_REJECT_UNAUTHORIZED is unset or set). Target the existing test block using the shouldAllowSelfSignedPgCertificate helper to add these scenarios so behavior contract for override precedence is covered.scripts/backfill_retention_metrics.ts (1)
223-227: Validate DB URL format at selection time for clearer failures.
getRequiredDatabaseUrlcurrently guarantees presence but not URL validity; malformed values fail later with less actionable errors. Consider failing early here.♻️ Proposed improvement
export function getRequiredDatabaseUrl(env: Record<string, string | undefined>) { const value = getDatabaseUrl(env) if (!value) throw new Error(`--apply requires ${DATABASE_URL_ENV_KEYS.join(', ')} so metric writes and processed-event markers are committed atomically`) + try { + new URL(value) + } + catch { + throw new Error(`Invalid database URL in ${DATABASE_URL_ENV_KEYS.join(', ')}`) + } return value }🤖 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 223 - 227, Update getRequiredDatabaseUrl to not only check presence but validate the URL format immediately: call getDatabaseUrl(env) as before, then attempt to parse/validate the returned value (e.g., using new URL(value) or a DB-URL-specific validation) and if parsing/validation fails throw a clear Error that includes the invalid value and DATABASE_URL_ENV_KEYS; reference getRequiredDatabaseUrl and getDatabaseUrl so reviewers can find the change and ensure downstream code can rely on a well-formed URL.
🤖 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 223-227: Update getRequiredDatabaseUrl to not only check presence
but validate the URL format immediately: call getDatabaseUrl(env) as before,
then attempt to parse/validate the returned value (e.g., using new URL(value) or
a DB-URL-specific validation) and if parsing/validation fails throw a clear
Error that includes the invalid value and DATABASE_URL_ENV_KEYS; reference
getRequiredDatabaseUrl and getDatabaseUrl so reviewers can find the change and
ensure downstream code can rely on a well-formed URL.
In `@tests/backfill-retention-metrics.unit.test.ts`:
- Around line 402-414: Add explicit unit tests for PG_ALLOW_SELF_SIGNED_CERT
precedence: update tests/backfill-retention-metrics.unit.test.ts to include
cases asserting shouldAllowSelfSignedPgCertificate returns true when
PG_ALLOW_SELF_SIGNED_CERT='1' (even if PG_SSL_REJECT_UNAUTHORIZED is set), and
returns false when PG_ALLOW_SELF_SIGNED_CERT='0' (including when
PG_SSL_REJECT_UNAUTHORIZED is unset or set). Target the existing test block
using the shouldAllowSelfSignedPgCertificate helper to add these scenarios so
behavior contract for override precedence is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8163e2f7-a17e-43ef-a0d4-f00cb3344c93
📒 Files selected for processing (2)
scripts/backfill_retention_metrics.tstests/backfill-retention-metrics.unit.test.ts
riderx
force-pushed
the
codex/fix-retention-backfill-pooler-tls
branch
from
643c99b to
6fe2ba1
Compare
|
1 participant
Summary (AI generated)
Motivation (AI generated)
The backfill script now finds
MAIN_SUPABASE_DB_URL, but production applies still fail whenpgconnects to the Supabase writer pooler atdb.<project>.supabase.co:6543. That pooler currently presents a TLS chain that local Node/Bunpgrejects, so--applyaborts before writing any rows.Business Impact (AI generated)
This unblocks the actual production backfill for NRR and churn revenue metrics. Without this fix, the script can fetch Stripe data but still cannot commit retention metrics into Supabase, leaving the admin charts empty or stale.
Test Plan (AI generated)
bunx vitest run tests/backfill-retention-metrics.unit.test.tsbun lintbun typecheckSELECT 1probe againstMAIN_SUPABASE_DB_URLfrominternal/cloudflare/.env.prodGenerated with AI
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests