fix(sso): ensure org provisioning when public profile is missing#1898
fix(sso): ensure org provisioning when public profile is missing#1898
Conversation
📝 WalkthroughWalkthroughThis PR adds a new helper function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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 (1)
tests/sso.test.ts (1)
492-610: Good regression test coverage.The test correctly exercises the missing-profile scenario by:
- Creating an auth user with SSO metadata
- Explicitly deleting
public.users(line 568) to simulate the edge case- Verifying both the profile sync and org membership postconditions
One minor suggestion: since this test creates fully isolated resources (unique IDs for org, stripe_info, provider, and user), it could use
it.concurrent()to improve CI parallelism. Other tests in this file with similar isolation patterns also useit(), so this is a broader consistency consideration.♻️ Optional: enable parallel execution
- it('creates missing public.users profile before assigning org membership', async () => { + it.concurrent('creates missing public.users profile before assigning org membership', async () => {As per coding guidelines: "Use
it.concurrent()instead ofit()when possible to run tests in parallel within the same file, maximizing parallelism for faster CI/CD"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sso.test.ts` around lines 492 - 610, The test "creates missing public.users profile before assigning org membership" is isolated and can run in parallel; change the test declaration from it(...) to it.concurrent(...) to enable parallel execution, i.e., locate the test block with the description string "creates missing public.users profile before assigning org membership" and replace the top-level it call with it.concurrent so the test runner executes it concurrently like other isolated tests in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/sso.test.ts`:
- Around line 492-610: The test "creates missing public.users profile before
assigning org membership" is isolated and can run in parallel; change the test
declaration from it(...) to it.concurrent(...) to enable parallel execution,
i.e., locate the test block with the description string "creates missing
public.users profile before assigning org membership" and replace the top-level
it call with it.concurrent so the test runner executes it concurrently like
other isolated tests in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0985acdc-3c0c-471a-9867-dd803b7e4f6c
📒 Files selected for processing (2)
supabase/functions/_backend/private/sso/provision-user.tstests/sso.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/app.test.ts`:
- Around line 5-10: The helper is too permissive because
isDuplicateAppCreationError currently treats a generic 'cannot_create_app' as a
duplicate; change it so duplicates are only detected by explicit duplicate
signals: 'app_id_already_exists', supabaseError.code === '23505', or
moreInfo.constraint === 'apps_pkey'; if you keep 'cannot_create_app' include it
only when paired with one of those DB indicators (e.g., require
body?.cannot_create_app && (body?.supabaseError?.code === '23505' ||
body?.moreInfo?.constraint === 'apps_pkey' || body?.error ===
'app_id_already_exists')), and update any tests that relied on the previous
broad behavior.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9807b98b-4e25-4743-ab5e-13f9ef502d2d
📒 Files selected for processing (2)
tests/app.test.tstests/plan-usage-org-rpc-access.test.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/auto-imports.d.ts (1)
48-55: Align generated declaration imports with the~/alias convention.The new declarations on Line 48, Line 55, Line 101, Line 395, Line 402, and Line 447 use
./stores/organization. Please update the auto-import generator config so emitted.d.tsimports use~/stores/organizationto stay consistent with TS import rules in this repo.As per coding guidelines "
**/*.ts: Use path alias~/to map tosrc/for cleaner imports".Also applies to: 101-101, 395-402, 447-447
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/auto-imports.d.ts` around lines 48 - 55, The generated declarations import './stores/organization' for symbols like isAdminRole and isSuperAdminRole; update the auto-import generator configuration (e.g., your unplugin-auto-import or equivalent config used to emit src/auto-imports.d.ts) so that emitted imports use the repo path alias '~/stores/organization' instead of relative './stores/organization' — either change the import entries for those symbols to '~/stores/organization' or add an alias/mapping in the generator so all resolved imports under src/ map to the '~/...' form when writing the .d.ts file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/app.test.ts`:
- Line 365: The file ends with the closing test block token "})" and is missing
a trailing newline; add a single newline character after the final "})" in
tests/app.test.ts so the file ends with an EOF newline and satisfies the
style/eol-last lint rule.
---
Nitpick comments:
In `@src/auto-imports.d.ts`:
- Around line 48-55: The generated declarations import './stores/organization'
for symbols like isAdminRole and isSuperAdminRole; update the auto-import
generator configuration (e.g., your unplugin-auto-import or equivalent config
used to emit src/auto-imports.d.ts) so that emitted imports use the repo path
alias '~/stores/organization' instead of relative './stores/organization' —
either change the import entries for those symbols to '~/stores/organization' or
add an alias/mapping in the generator so all resolved imports under src/ map to
the '~/...' form when writing the .d.ts file.
🪄 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: a11884bc-dacf-4b2d-beed-ca98f044f4f1
📒 Files selected for processing (4)
src/auto-imports.d.tssrc/components.d.tssrc/route-map.d.tstests/app.test.ts
💤 Files with no reviewable changes (1)
- src/components.d.ts
| expect(responseData).toHaveProperty('app_id', APPNAME) | ||
| }) | ||
| }) | ||
| }) No newline at end of file |
There was a problem hiding this comment.
Fix EOF newline to satisfy lint.
Line 365 is missing a trailing newline, which currently fails style/eol-last.
🔧 Proposed fix
})
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }) | |
| }) | |
🧰 Tools
🪛 ESLint
[error] 365-365: Newline required at end of file but not found.
(style/eol-last)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/app.test.ts` at line 365, The file ends with the closing test block
token "})" and is missing a trailing newline; add a single newline character
after the final "})" in tests/app.test.ts so the file ends with an EOF newline
and satisfies the style/eol-last lint rule.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/app.test.ts (1)
374-374:⚠️ Potential issue | 🟡 MinorAdd the missing EOF newline to pass lint.
Line 374 currently leaves the file without a trailing newline, which fails
style/eol-last.🔧 Proposed fix
}) +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/app.test.ts` at line 374, The file ends with the closing test block "})" but is missing a trailing newline; add a single newline character after the final "})" (end of file) so the file terminates with an EOF newline to satisfy the style/eol-last lint rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/app.test.ts`:
- Line 374: The file ends with the closing test block "})" but is missing a
trailing newline; add a single newline character after the final "})" (end of
file) so the file terminates with an EOF newline to satisfy the style/eol-last
lint rule.
|
1 participant
Summary (AI generated)
/private/sso/provision-usercreates a missingpublic.usersprofile fromauth.usersbefore org assignment.org_usersmembership.public.usersis missing.Motivation (AI generated)
Some SSO users could authenticate successfully but still fail org association if their
public.usersrow was missing at provisioning time. This left them in a broken post-login state and could redirect them into onboarding despite an existing SSO org.Business Impact (AI generated)
This reduces failed enterprise SSO activations and prevents friction for first-time SSO logins, improving conversion and reducing support load around onboarding misroutes.
Test Plan (AI generated)
bun lintbun run lint:backendbun run supabase:with-env -- bunx vitest run tests/sso.test.tscreates missing public.users profile before assigning org membershipGenerated with AI
Summary by CodeRabbit
New Features
Tests
Chores