fix(ci): fix all failing tests (types, mocks, RLS, rate limits)#1698
Merged
fix(ci): fix all failing tests (types, mocks, RLS, rate limits)#1698
Conversation
…email_otp_verified - Add missing delete_group_with_bindings(group_id: uuid) -> void - Update record_email_otp_verified signature from () to (user_id: uuid) after migration 20260224100001 changed the function signature - Applied to both src/types and backend copy
Contributor
|
Warning Rate limit exceeded
⌛ 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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request updates RPC function signatures in Supabase type definitions, renames the parameter in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…al_metrics - record_email_otp_verified: rename param user_id -> p_user_id to fix ambiguous column reference (plpgsql_check 42702) - get_total_metrics(): read current_setting into text var first, then cast to uuid, avoiding invalid uuid comparison with empty string (22P02) - Update TypeScript types and calling code for renamed parameter
- record_email_otp_verified: rename param user_id -> p_user_id to fix ambiguous column reference (plpgsql_check 42702) - get_total_metrics(): read current_setting into text var first, then cast to uuid, avoiding invalid uuid comparison with empty string (22P02) - Remove unnecessary new migration — edited originals in place since neither has been applied to production
- Use importOriginal for hono/adapter mock to preserve getRuntimeKey - Mock stripe npm package constructor instead of vi.spyOn on getStripe - Use mockImplementation with regular function for new Stripe() calls - Use dynamic imports to get properly mocked module exports
Diagnostic: log actual error body when PUT /organization returns non-200 to understand why checkPermission is failing in CI.
The orgs UPDATE RLS policy called has_2fa_enabled(auth.uid()::uuid), but the uuid overload is restricted to postgres/service_role only. The authenticated role (used by API key and JWT auth) gets "permission denied for function has_2fa_enabled". Switch to the no-arg has_2fa_enabled() which is granted to authenticated and uses auth.uid() internally. Also removes temporary diagnostic logging from apikeys tests.
All CF tests share the same API keys, so the 2000 req/minute per-key rate limit is hit after ~60s of concurrent test execution, causing cascading 429 failures across 18+ test files. Set rate limits to 999999 in the test env file to prevent this.
WcaleNieWolny
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes multiple CI test failures across type checking, unit tests, integration tests, and SQL migrations. The changes address stale type definitions, broken test mocks, RLS permission errors, and rate limiting issues that were blocking the test suite.
Changes:
- Updated Supabase RPC type signatures for
delete_group_with_bindingsandrecord_email_otp_verifiedto match actual SQL function signatures - Fixed Stripe unit test mocking to use proper ESM vi.mock() pattern with dynamic imports instead of broken vi.spyOn
- Fixed RLS permission error in orgs UPDATE policy by switching from
has_2fa_enabled(uuid)(service_role only) tohas_2fa_enabled()(authenticated) - Fixed
get_total_metrics()function to properly handle JWT claim parsing with intermediate text variable before UUID casting - Fixed
record_email_otp_verifiedparameter name collision by renaminguser_idtop_user_id - Disabled rate limits in Cloudflare Workers test environment to prevent cascading 429 failures across parallel test runs
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/stripe-redirects.unit.test.ts | Rewrites Stripe mocking using vi.mock() with constructor pattern and dynamic imports to fix broken unit tests |
| supabase/migrations/20260226000100_fix_org_rls_2fa_function_permissions.sql | Fixes RLS permission error by using no-arg has_2fa_enabled() instead of uuid overload in UPDATE policy |
| supabase/migrations/20260224100001_revoke_record_email_otp_verified_auth_role.sql | Renames parameter from user_id to p_user_id to avoid SQL naming collision |
| supabase/migrations/20260224093000_fix_get_total_metrics_auth.sql | Adds intermediate text variable for JWT claim parsing before UUID casting to prevent type errors |
| supabase/functions/_backend/utils/supabase.types.ts | Regenerates types to add missing delete_group_with_bindings RPC and update record_email_otp_verified signature |
| supabase/functions/_backend/private/verify_email_otp.ts | Updates RPC call to use new p_user_id parameter name |
| src/types/supabase.types.ts | Mirrors backend type updates for frontend consistency |
| scripts/start-cloudflare-workers.sh | Sets extremely high rate limits (999999) for test environment to prevent false 429 failures |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/stripe-redirects.unit.test.ts (1)
46-46:⚠️ Potential issue | 🟡 MinorUse
it.concurrent()for all test cases in this file.This file currently uses
it(...), which violates the test parallelization rule for*.test.tsfiles.✅ Suggested change
- it('allows same-origin return URLs for billing portal', async () => { + it.concurrent('allows same-origin return URLs for billing portal', async () => { @@ - it('rejects external return URLs for billing portal', async () => { + it.concurrent('rejects external return URLs for billing portal', async () => { @@ - it('allows same-origin success and cancel URLs for checkout', async () => { + it.concurrent('allows same-origin success and cancel URLs for checkout', async () => { @@ - it('rejects external success URLs for checkout', async () => { + it.concurrent('rejects external success URLs for checkout', async () => { @@ - it('rejects external cancel URLs for one-time checkout', async () => { + it.concurrent('rejects external cancel URLs for one-time checkout', async () => {As per coding guidelines, "ALL test files run in parallel; use
it.concurrent()instead ofit()to run tests within the same file in parallel for faster CI/CD".Also applies to: 68-68, 89-89, 121-121, 151-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/stripe-redirects.unit.test.ts` at line 46, Replace all synchronous Jest test declarations in tests/stripe-redirects.unit.test.ts that use it(...) with parallelizable it.concurrent(...). Specifically update the test that begins with "allows same-origin return URLs for billing portal" (and the other it(...) occurrences in this file) to call it.concurrent('...') and keep the async test bodies unchanged so tests run in parallel per the project guideline.supabase/migrations/20260224093000_fix_get_total_metrics_auth.sql (1)
226-230:⚠️ Potential issue | 🟡 MinorNo
anongrant — API key callers cannot invoke this overload.The no-arg
get_total_metrics()is granted only toauthenticated, but the coding guidelines require bothauthenticatedandanonroles in access grants to enable API key authentication (API keys connect asanon). If this function needs to be callable via API keys,anonmust also be granted. If restricting to JWT-only callers is intentional, please add a comment documenting this decision.As per coding guidelines, "Always use both
authenticatedandanonroles in RLS policies to enable API key authentication." While this is a function grant rather than an RLS policy, the same principle applies if API key access is expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260224093000_fix_get_total_metrics_auth.sql` around lines 226 - 230, The migration revokes and grants privileges for public.get_total_metrics() only to the authenticated role, which prevents API-key callers (who connect as anon) from invoking the no-arg overload; update the migration so API-key access works by adding a matching grant for anon (e.g., GRANT ALL ON FUNCTION public.get_total_metrics() TO anon) alongside the existing authenticated grant, or if the intent is to restrict this function to JWT-only callers, add a clear SQL comment above the REVOKE/GRANT lines documenting that the omission of anon is intentional and why.
🧹 Nitpick comments (1)
scripts/start-cloudflare-workers.sh (1)
80-82: Make test rate-limit values overridable (keep high defaults).Line 80–82 hardcode
999999, which is fine for CI, but prevents local throttling-focused runs from overriding behavior. Prefer env-default expansion.Proposed change
-RATE_LIMIT_API_KEY=999999 -RATE_LIMIT_FAILED_AUTH=999999 -RATE_LIMIT_CHANNEL_SELF_IP=999999 +RATE_LIMIT_API_KEY=${RATE_LIMIT_API_KEY:-999999} +RATE_LIMIT_FAILED_AUTH=${RATE_LIMIT_FAILED_AUTH:-999999} +RATE_LIMIT_CHANNEL_SELF_IP=${RATE_LIMIT_CHANNEL_SELF_IP:-999999}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/start-cloudflare-workers.sh` around lines 80 - 82, Replace the hardcoded test rate-limit numeric assignments with shell environment-default expansions so local environment variables can override while keeping the high defaults; specifically update the RATE_LIMIT_API_KEY, RATE_LIMIT_FAILED_AUTH, and RATE_LIMIT_CHANNEL_SELF_IP assignments to use parameter expansion (default to 999999 when the env var is unset) rather than literal 999999 so CI keeps the high defaults but local overrides are respected.
🤖 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/migrations/20260224093000_fix_get_total_metrics_auth.sql`:
- Around line 188-192: The current unguarded cast of v_org_id_text into
v_request_org_id (v_request_org_id := v_org_id_text::uuid) can raise an
exception for malformed JWT org_id values; wrap the cast in a defensive block
either by validating the format first (e.g. test v_org_id_text against a UUID
regex) before casting, or perform the cast inside a BEGIN … EXCEPTION WHEN
invalid_text_representation THEN ... END block to set v_request_org_id to NULL
or handle the error safely; update the code around
current_setting('request.jwt.claim.org_id') / v_org_id_text and v_request_org_id
to use one of these approaches so malformed strings no longer abort the
function.
In `@tests/stripe-redirects.unit.test.ts`:
- Line 56: Replace the verbose function expressions used in the Stripe mock
implementations with concise arrow expression bodies to satisfy lint rules;
change instances like vi.mocked(Stripe).mockImplementation(function () { return
stripeClient } as any) to use an expression-bodied arrow (e.g., () =>
stripeClient as any) and apply the same change for the other occurrences at the
mockImplementation calls around the tests (the calls that reference Stripe and
stripeClient on lines similar to 56, 78, 102, 134, 164).
- Around line 1-2: The import order is wrong for linting: move the
default/namespace import Stripe so it appears before the named imports from
vitest; specifically, reorder the import statements so "import Stripe from
'stripe'" comes before "import { afterEach, describe, expect, it, vi } from
'vitest'"; update the top-of-file imports in tests/stripe-redirects.unit.test.ts
accordingly so symbols Stripe and the vitest names (afterEach, describe, expect,
it, vi) remain unchanged.
---
Outside diff comments:
In `@supabase/migrations/20260224093000_fix_get_total_metrics_auth.sql`:
- Around line 226-230: The migration revokes and grants privileges for
public.get_total_metrics() only to the authenticated role, which prevents
API-key callers (who connect as anon) from invoking the no-arg overload; update
the migration so API-key access works by adding a matching grant for anon (e.g.,
GRANT ALL ON FUNCTION public.get_total_metrics() TO anon) alongside the existing
authenticated grant, or if the intent is to restrict this function to JWT-only
callers, add a clear SQL comment above the REVOKE/GRANT lines documenting that
the omission of anon is intentional and why.
In `@tests/stripe-redirects.unit.test.ts`:
- Line 46: Replace all synchronous Jest test declarations in
tests/stripe-redirects.unit.test.ts that use it(...) with parallelizable
it.concurrent(...). Specifically update the test that begins with "allows
same-origin return URLs for billing portal" (and the other it(...) occurrences
in this file) to call it.concurrent('...') and keep the async test bodies
unchanged so tests run in parallel per the project guideline.
---
Nitpick comments:
In `@scripts/start-cloudflare-workers.sh`:
- Around line 80-82: Replace the hardcoded test rate-limit numeric assignments
with shell environment-default expansions so local environment variables can
override while keeping the high defaults; specifically update the
RATE_LIMIT_API_KEY, RATE_LIMIT_FAILED_AUTH, and RATE_LIMIT_CHANNEL_SELF_IP
assignments to use parameter expansion (default to 999999 when the env var is
unset) rather than literal 999999 so CI keeps the high defaults but local
overrides are respected.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
scripts/start-cloudflare-workers.shsrc/types/supabase.types.tssupabase/functions/_backend/private/verify_email_otp.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260224093000_fix_get_total_metrics_auth.sqlsupabase/migrations/20260224100001_revoke_record_email_otp_verified_auth_role.sqlsupabase/migrations/20260226000100_fix_org_rls_2fa_function_permissions.sqltests/stripe-redirects.unit.test.ts
Comment on lines
188
to
192
| SELECT current_setting('request.jwt.claim.org_id', true) INTO v_org_id_text; | ||
|
|
||
| IF v_request_org_id IS NULL OR v_request_org_id = '' THEN | ||
| IF v_org_id_text IS NOT NULL AND v_org_id_text <> '' THEN | ||
| v_request_org_id := v_org_id_text::uuid; | ||
| END IF; |
Contributor
There was a problem hiding this comment.
Uncaught exception if JWT org_id claim contains a malformed (non-UUID) string.
current_setting returns a raw text value. If it's non-null and non-empty but not a valid UUID, the ::uuid cast on line 191 will throw a runtime exception, aborting the function. Consider wrapping in a BEGIN … EXCEPTION WHEN invalid_text_representation block or validating the format before casting.
🛡️ Proposed defensive cast
SELECT current_setting('request.jwt.claim.org_id', true) INTO v_org_id_text;
IF v_org_id_text IS NOT NULL AND v_org_id_text <> '' THEN
- v_request_org_id := v_org_id_text::uuid;
+ BEGIN
+ v_request_org_id := v_org_id_text::uuid;
+ EXCEPTION WHEN invalid_text_representation THEN
+ v_request_org_id := NULL;
+ END;
END IF;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@supabase/migrations/20260224093000_fix_get_total_metrics_auth.sql` around
lines 188 - 192, The current unguarded cast of v_org_id_text into
v_request_org_id (v_request_org_id := v_org_id_text::uuid) can raise an
exception for malformed JWT org_id values; wrap the cast in a defensive block
either by validating the format first (e.g. test v_org_id_text against a UUID
regex) before casting, or perform the cast inside a BEGIN … EXCEPTION WHEN
invalid_text_representation THEN ... END block to set v_request_org_id to NULL
or handle the error safely; update the code around
current_setting('request.jwt.claim.org_id') / v_org_id_text and v_request_org_id
to use one of these approaches so malformed strings no longer abort the
function.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tests/stripe-redirects.unit.test.ts (2)
56-56: SimplifymockImplementationto arrow function to fix lint.Already flagged in a prior review round. Also applies to lines 78, 102, 134, 164.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/stripe-redirects.unit.test.ts` at line 56, Replace the non-arrow mockImplementation usage with an arrow function to satisfy linting: change vi.mocked(Stripe).mockImplementation(function () { return stripeClient } as any) to vi.mocked(Stripe).mockImplementation(() => stripeClient as any); apply the same change for the other Stripe mocks in this file that use function() { return stripeClient } (instances referencing vi.mocked(Stripe).mockImplementation and stripeClient).
1-2: Import order:stripeshould precedevitest.Already flagged in a prior review round.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/stripe-redirects.unit.test.ts` around lines 1 - 2, The import order is wrong—move the default Stripe import before the named vitest imports so Stripe is imported first; update the top of the file so the Stripe import line ("import Stripe from 'stripe'") appears before the vitest line ("import { afterEach, describe, expect, it, vi } from 'vitest'") to satisfy the required import ordering rule.supabase/migrations/20260224093000_fix_get_total_metrics_auth.sql (1)
188-192:⚠️ Potential issue | 🟡 MinorGuard the JWT
org_idcast before converting touuid.Line 191 can throw
invalid_text_representationfor malformed claim values, which aborts the RPC instead of falling back toorg_users.Proposed fix
SELECT current_setting('request.jwt.claim.org_id', true) INTO v_org_id_text; IF v_org_id_text IS NOT NULL AND v_org_id_text <> '' THEN - v_request_org_id := v_org_id_text::uuid; + BEGIN + v_request_org_id := v_org_id_text::uuid; + EXCEPTION WHEN invalid_text_representation THEN + v_request_org_id := NULL; + END; END IF;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260224093000_fix_get_total_metrics_auth.sql` around lines 188 - 192, The code casts v_org_id_text to uuid directly (v_request_org_id := v_org_id_text::uuid) which can raise invalid_text_representation for malformed JWT claim values; wrap that cast in a small PL/pgSQL BEGIN...EXCEPTION block catching invalid_text_representation and leaving v_request_org_id NULL (so the flow falls back to org_users) — locate the current_setting call and the assignment to v_request_org_id and replace the direct cast with a guarded block that only assigns when the cast succeeds and silently ignores/sets NULL on parse errors.
🧹 Nitpick comments (1)
tests/stripe-redirects.unit.test.ts (1)
46-46: Tests useit()instead ofit.concurrent()per coding guidelines.The coding guidelines require
it.concurrent()for parallel test execution. However, all five tests here mutate shared mock state viavi.mocked(Stripe).mockImplementation(...), which would race under concurrent execution. To safely adoptit.concurrent(), you'd need to isolate each test's Stripe mock — e.g., by moving thevi.mock/mockImplementationinto per-test module isolation (vi.hoisted+vi.doMock) or restructuring so each test gets its own module scope.Worth addressing in a follow-up since the current sequential execution is correct. As per coding guidelines, "ALL test files run in parallel; use
it.concurrent()instead ofit()to run tests within the same file in parallel for faster CI/CD".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/stripe-redirects.unit.test.ts` at line 46, Tests in tests/stripe-redirects.unit.test.ts use sequential it() which violates the guideline to use it.concurrent(); fix by isolating the Stripe mock per test so tests can safely run in parallel: replace shared vi.mocked(Stripe).mockImplementation(...) usage with per-test module isolation (e.g., move mock setup into each test using vi.doMock/vi.hoisted or recreate the mock inside each it.concurrent block) and then convert each it(...) to it.concurrent(...), ensuring functions/classes referenced (Stripe, vi.mocked, mockImplementation, vi.doMock, vi.hoisted, and the test blocks) are updated so no shared mutable mock state is reused across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@supabase/migrations/20260224093000_fix_get_total_metrics_auth.sql`:
- Around line 188-192: The code casts v_org_id_text to uuid directly
(v_request_org_id := v_org_id_text::uuid) which can raise
invalid_text_representation for malformed JWT claim values; wrap that cast in a
small PL/pgSQL BEGIN...EXCEPTION block catching invalid_text_representation and
leaving v_request_org_id NULL (so the flow falls back to org_users) — locate the
current_setting call and the assignment to v_request_org_id and replace the
direct cast with a guarded block that only assigns when the cast succeeds and
silently ignores/sets NULL on parse errors.
In `@tests/stripe-redirects.unit.test.ts`:
- Line 56: Replace the non-arrow mockImplementation usage with an arrow function
to satisfy linting: change vi.mocked(Stripe).mockImplementation(function () {
return stripeClient } as any) to vi.mocked(Stripe).mockImplementation(() =>
stripeClient as any); apply the same change for the other Stripe mocks in this
file that use function() { return stripeClient } (instances referencing
vi.mocked(Stripe).mockImplementation and stripeClient).
- Around line 1-2: The import order is wrong—move the default Stripe import
before the named vitest imports so Stripe is imported first; update the top of
the file so the Stripe import line ("import Stripe from 'stripe'") appears
before the vitest line ("import { afterEach, describe, expect, it, vi } from
'vitest'") to satisfy the required import ordering rule.
---
Nitpick comments:
In `@tests/stripe-redirects.unit.test.ts`:
- Line 46: Tests in tests/stripe-redirects.unit.test.ts use sequential it()
which violates the guideline to use it.concurrent(); fix by isolating the Stripe
mock per test so tests can safely run in parallel: replace shared
vi.mocked(Stripe).mockImplementation(...) usage with per-test module isolation
(e.g., move mock setup into each test using vi.doMock/vi.hoisted or recreate the
mock inside each it.concurrent block) and then convert each it(...) to
it.concurrent(...), ensuring functions/classes referenced (Stripe, vi.mocked,
mockImplementation, vi.doMock, vi.hoisted, and the test blocks) are updated so
no shared mutable mock state is reused across tests.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
scripts/start-cloudflare-workers.shsrc/types/supabase.types.tssupabase/functions/_backend/private/verify_email_otp.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260224093000_fix_get_total_metrics_auth.sqlsupabase/migrations/20260224100001_revoke_record_email_otp_verified_auth_role.sqlsupabase/migrations/20260226000100_fix_org_rls_2fa_function_permissions.sqltests/stripe-redirects.unit.test.ts
- Reorder imports: stripe before vitest (alphabetical) - Replace verbose function expressions with arrow expressions in mockImplementation calls
…rg_id Wrap v_org_id_text::uuid in BEGIN/EXCEPTION block so a malformed request.jwt.claim.org_id gracefully falls through to the org_users lookup instead of aborting the function. Note: migration edited in-place — it has not been applied to production.
Contributor
Author
|
The following migrations were edited in-place (not via new corrective migrations):
This is safe because neither migration has been applied to production yet — they were introduced in this same PR cycle. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
supabase/migrations/20260224093000_fix_get_total_metrics_auth.sql (1)
188-197: Defensive UUID cast looks good.The
BEGIN … EXCEPTION WHEN invalid_text_representationblock correctly handles malformed JWTorg_idclaims by falling through to theorg_userslookup. This addresses the previously flagged concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260224093000_fix_get_total_metrics_auth.sql` around lines 188 - 197, The previous concern about casting the JWT org_id to UUID is resolved by the defensive block: capture current_setting('request.jwt.claim.org_id', true) into v_org_id_text and, inside the BEGIN ... EXCEPTION WHEN invalid_text_representation block, attempt the cast to v_request_org_id := v_org_id_text::uuid and on failure set v_request_org_id := NULL so the code falls back to the org_users lookup; leave the v_org_id_text / v_request_org_id logic as-is (no further changes required).
🧹 Nitpick comments (1)
tests/stripe-redirects.unit.test.ts (1)
46-66: Consider usingit.concurrent()for parallel test execution — but note the mock-state caveat.The coding guideline requires
it.concurrent()for all tests. However, these tests mutate shared mock state (vi.mocked(Stripe).mockImplementation(...)) before each invocation. Running them concurrently would race on the Stripe mock, causing flaky failures.If you want to comply with the guideline, you'd need to restructure each test to use a fully isolated mock (e.g.,
vi.doMockper test or move mock setup into a factory). Given the tradeoff, keeping sequentialit()here is pragmatically safer.As per coding guidelines: "ALL test files run in parallel; use
it.concurrent()instead ofit()to run tests within the same file in parallel for faster CI/CD."Also applies to: 68-87, 89-119, 121-149, 151-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/stripe-redirects.unit.test.ts` around lines 46 - 66, Tests are using a shared mocked Stripe implementation which prevents safe parallelization; to switch to it.concurrent() you must isolate the mock per test by resetting module state and mocking before importing the code under test. For each test (including the one referencing createPortal, Stripe, and createSession) call vi.resetModules(), use vi.doMock(...) or vi.mock(...) inside the test to provide a fresh Stripe implementation (or factory that returns the stripeClient) and only then import createPortal via await import('../supabase/functions/_backend/utils/stripe.ts'); after the test ensure you restore/reset mocks so each concurrent test gets an independent Stripe mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@supabase/migrations/20260224093000_fix_get_total_metrics_auth.sql`:
- Around line 188-197: The previous concern about casting the JWT org_id to UUID
is resolved by the defensive block: capture
current_setting('request.jwt.claim.org_id', true) into v_org_id_text and, inside
the BEGIN ... EXCEPTION WHEN invalid_text_representation block, attempt the cast
to v_request_org_id := v_org_id_text::uuid and on failure set v_request_org_id
:= NULL so the code falls back to the org_users lookup; leave the v_org_id_text
/ v_request_org_id logic as-is (no further changes required).
---
Nitpick comments:
In `@tests/stripe-redirects.unit.test.ts`:
- Around line 46-66: Tests are using a shared mocked Stripe implementation which
prevents safe parallelization; to switch to it.concurrent() you must isolate the
mock per test by resetting module state and mocking before importing the code
under test. For each test (including the one referencing createPortal, Stripe,
and createSession) call vi.resetModules(), use vi.doMock(...) or vi.mock(...)
inside the test to provide a fresh Stripe implementation (or factory that
returns the stripeClient) and only then import createPortal via await
import('../supabase/functions/_backend/utils/stripe.ts'); after the test ensure
you restore/reset mocks so each concurrent test gets an independent Stripe mock.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
scripts/start-cloudflare-workers.shsrc/types/supabase.types.tssupabase/functions/_backend/private/verify_email_otp.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260224093000_fix_get_total_metrics_auth.sqlsupabase/migrations/20260224100001_revoke_record_email_otp_verified_auth_role.sqlsupabase/migrations/20260226000100_fix_org_rls_2fa_function_permissions.sqltests/stripe-redirects.unit.test.ts
…or mock Arrow functions cannot be used as constructors. Since Stripe is instantiated with `new Stripe(...)`, mockImplementation must use regular function() syntax.
WcaleNieWolny
force-pushed
the
fix/update-supabase-types
branch
from
0c91db2 to
ac153ea
Compare
|
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
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
delete_group_with_bindings,record_email_otp_verified) — fixes typecheck failures blocking CIget_total_metricsRPC access — new migration drops old overloads, adds admin-only UUID overloads (service_role only) and an authenticated no-arg overload that resolves the org from caller contextrecord_email_otp_verified— rewrites as a(uuid)function granted only toservice_role/postgres, revokesauthenticated/publicaccess, and drops the legacy zero-arg overloadvi.mock('stripe')constructor pattern instead of brokenvi.spyOnon internal module callsorgsUPDATE policy fromhas_2fa_enabled(uuid)(restricted toservice_role) tohas_2fa_enabled()(granted toauthenticated)Test plan
Summary by CodeRabbit
Security
Improvements