feat(onboarding): require org setup before app onboarding#1835
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 routed multi-step organization onboarding flow, a website-preview backend, org logo upload support, auth guard redirects for users without selectable organizations, removes automatic per-user org creation, simplifies the dashboard org dropdown, and updates tests and migrations accordingly. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant Browser as Browser/UI
participant Router as Router
participant AuthGuard as Auth Guard
participant OrgStore as Organization Store
participant Onboarding as Onboarding Page
participant Functions as Website Preview (Edge)
participant Supabase as Supabase DB/Storage
User->>Browser: submit registration
Browser->>Supabase: create user (no auto-org trigger)
Supabase-->>Browser: user created
Browser->>Router: navigate /onboarding/organization
Router->>AuthGuard: beforeEach -> check auth & orgs
AuthGuard->>OrgStore: fetchOrganizations / dedupFetchOrganizations
OrgStore->>Supabase: query orgs
Supabase-->>OrgStore: orgs list (maybe empty)
OrgStore-->>AuthGuard: hasOrganizations? false
AuthGuard->>Router: ensure /onboarding/organization
Router->>Onboarding: render onboarding
User->>Onboarding: provide website or details
Onboarding->>Functions: POST /website_preview (optional)
Functions-->>Onboarding: preview {name, icon, hostname}
Onboarding->>Supabase: create org row
Supabase-->>Onboarding: org created
Onboarding->>OrgStore: refresh & set currentOrganization
User->>Onboarding: upload/import logo
Onboarding->>Supabase: uploadOrgLogoFile -> storage & update orgs.logo
Supabase-->>Onboarding: logo stored, org updated
User->>Onboarding: invite teammates & finish
Onboarding->>Router: navigate to `/apps` or safe target
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 8cdd40b16b
ℹ️ 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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/sso.test.ts (1)
343-418: 🛠️ Refactor suggestion | 🟠 MajorAdd the missing end-to-end coverage for the no-org onboarding path.
This assertion covers the trigger change, but the PR also rewires signup, the auth guard, and the org switcher into
/onboarding/organization. Please add a user-flow test that proves a no-org user is redirected there and cannot reach apps until the create step succeeds.As per coding guidelines, "tests/**/*.test.ts: Always cover database changes with Postgres-level tests and complement them with end-to-end tests for affected user flows".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/sso.test.ts` around lines 343 - 418, The test adds Postgres-level coverage but misses an end-to-end user-flow for the no-org onboarding path: add an E2E test that signs up (or logs in) the email-auth user you create in this test and verifies they are redirected to /onboarding/organization (covering the rewritten signup, auth guard, and org switcher) and cannot access app routes until they complete the organization create step; simulate the user clicking through the onboarding create flow and assert successful access to app routes only after the create completes. Locate the new test alongside tests/sso.test.ts (or your E2E suite) and reference the created user id/email from this test setup, the /onboarding/organization route, and the orgs creation endpoint to complete the flow. Ensure the E2E asserts both redirect and blocked access before creation and successful access after creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/auth.ts`:
- Around line 159-162: The guard currently awaits
organizationStore.fetchOrganizations() without error handling so RPC failures
can abort navigation; wrap the fetchOrganizations() calls in a try/catch (both
the block using organizationStore.fetchOrganizations() and the similar code at
lines 223-226) and only perform the redirect based on
organizationStore.hasOrganizations when the fetch succeeded; on catch, log or
handle the error (do not rethrow) and continue to call next() / hideLoader() so
the router guard does not get blocked.
In `@src/pages/onboarding/organization.vue`:
- Around line 48-52: The onboarding flow must not fall back to the user's
existing currentOrganization when the URL requests onboarding steps like "logo"
or "invite" unless there is a validated onboarding org; update
hydrateOnboardingFromQuery to validate the incoming org/step and only set
createdOrgId when the org was actually created/validated, and change the
computed activeOrgId and activeOrgName logic (the computed using createdOrgId,
currentOrganization, importedOrgName, orgNameInput) so that for onboarding-only
steps (logo/invite) activeOrgId returns an empty string unless createdOrgId is
present and validated; ensure activeOrgName only reads currentOrganization.name
when currentOrganization.gid === activeOrgId to avoid targeting a previously
selected org.
- Around line 143-150: Add an auth guard to prevent creating orgs with empty
user fields by (1) adding a component-level check in the onboarding component's
onMounted handler to verify main.auth exists and disable/redirect the page if
not authenticated before allowing the form or submit button to be used, and (2)
declaring meta.middleware on the page's <route> block (matching the pattern used
in /settings pages) so the global middleware enforces authentication; ensure the
submit path that calls the supabase .from('orgs').insert(...) (where created_by:
main.auth?.id ?? '' and management_email: main.auth?.email ?? '') is only
reachable when main.auth is present so those fallbacks are never used.
In `@src/services/photos.ts`:
- Around line 157-173: The org logo flow in uploadOrgLogoFile uploads to
supabase.storage (storagePath) before updating the org row but does not verify
the update affected a row, causing orphaned files; change the update call in
uploadOrgLogoFile to include .select().single() (matching
uploadUserProfilePhoto()/uploadGroupLogo()) and check the returned data/error to
detect a missing row, and if the update did not return the updated row (or
returns null/404), call supabase.storage.from('images').remove([storagePath]) to
delete the uploaded file and then throw an appropriate error so callers know the
update failed for safeOrgId.
In `@src/stores/organization.ts`:
- Line 146: hasOrganizations currently checks organizations.value.length but
fetchOrganizations filters out invite-only orgs when setting
currentOrganization, causing invite-only users to appear onboarded; update
hasOrganizations to count only selectable organizations (i.e., filter out invite
roles the same way fetchOrganizations does) or derive it from the same helper
used to compute selectable orgs so it matches currentOrganization selection
logic (apply the same change to the other occurrence that mirrors this check
around the selectable-org logic).
---
Outside diff comments:
In `@tests/sso.test.ts`:
- Around line 343-418: The test adds Postgres-level coverage but misses an
end-to-end user-flow for the no-org onboarding path: add an E2E test that signs
up (or logs in) the email-auth user you create in this test and verifies they
are redirected to /onboarding/organization (covering the rewritten signup, auth
guard, and org switcher) and cannot access app routes until they complete the
organization create step; simulate the user clicking through the onboarding
create flow and assert successful access to app routes only after the create
completes. Locate the new test alongside tests/sso.test.ts (or your E2E suite)
and reference the created user id/email from this test setup, the
/onboarding/organization route, and the orgs creation endpoint to complete the
flow. Ensure the E2E asserts both redirect and blocked access before creation
and successful access after creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c61ce06-e83d-4ef7-816b-c1bb67ba7e69
📒 Files selected for processing (8)
src/components/dashboard/DropdownOrganization.vuesrc/modules/auth.tssrc/pages/onboarding/organization.vuesrc/pages/register.vuesrc/services/photos.tssrc/stores/organization.tssupabase/migrations/20260319235626_disable_auto_org_on_user_create.sqltests/sso.test.ts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?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: df145379bf
ℹ️ 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: 3
♻️ Duplicate comments (1)
src/pages/onboarding/organization.vue (1)
302-307:⚠️ Potential issue | 🟡 MinorRevalidate query-driven step changes after mount.
hydrateOnboardingFromQuery()guardslogoandinvite, but this watcher later accepts those steps from the URL without checkingcreatedOrgId. Editing the query can still land the page on a later step with no onboarding org and leave the flow in a dead end.Suggested fix
-watch(() => route.query.step, (nextValue) => { - if (typeof nextValue !== 'string') - return - if (nextValue === 'details' || nextValue === 'logo' || nextValue === 'invite') - step.value = nextValue -}) +watch(() => [route.query.step, route.query.org], async () => { + const nextStep = typeof route.query.step === 'string' ? route.query.step as OnboardingStep : 'details' + if ((nextStep === 'logo' || nextStep === 'invite') && !createdOrgId.value) { + await syncRouteQuery('details', '') + return + } + step.value = nextStep +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/onboarding/organization.vue` around lines 302 - 307, The watcher on route.query.step allows 'logo' and 'invite' unconditionally, which can advance the flow without a createdOrgId and break onboarding; update the watcher in src/pages/onboarding/organization.vue to revalidate before setting step.value by either calling hydrateOnboardingFromQuery() or checking the same condition used there (ensure createdOrgId exists) — only set step.value to 'logo' or 'invite' when createdOrgId is present, otherwise restrict to 'details' or fallback to the safe step; reference the existing hydrateOnboardingFromQuery(), route.query.step, createdOrgId, and step.value to locate and fix the logic.
🧹 Nitpick comments (1)
playwright/e2e/register.spec.ts (1)
8-33: Add coverage for the org-switcher entrypoint too.This scenario only exercises the post-signup redirect. The PR also reroutes the dashboard selector’s “new org” action into onboarding, and that customer-facing path is not exercised here.
As per coding guidelines, "Cover customer-facing flows with Playwright MCP test suite; add scenarios under playwright/e2e and run with
bun run test:frontbefore shipping UI changes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playwright/e2e/register.spec.ts` around lines 8 - 33, The test only validates the post-signup redirect to onboarding; add a second Playwright scenario in register.spec.ts that covers the dashboard/org-switcher entrypoint by signing in (or reusing the created user), navigating to '/apps', triggering the dashboard/org-switcher "new org" action (the selector tied to the PR's change — e.g. the org switcher button and the "new org" item used in UI), asserting it routes into the onboarding flow (/onboarding/organization), then complete the same onboarding steps (fill org name, skip logo, finish) and assert final /apps redirect; implement this as a separate test (e.g., 'should route new org from org-switcher into onboarding') using the same uniqueSuffix pattern and the existing data-test selectors used in the file to locate elements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/onboarding/organization.vue`:
- Around line 208-213: The post-create refresh
(organizationStore.fetchOrganizations() and syncRouteQuery('logo', data.id)) can
throw and currently will reject the whole handler after the org was created;
wrap these calls in their own try/catch blocks so failures don’t abort a
successful create: ensure createdOrgId.value = data.id,
organizationStore.setCurrentOrganization(data.id), step.value = 'logo' and
toast.success(...) always run, then call await
organizationStore.fetchOrganizations() and await syncRouteQuery('logo', data.id)
inside separate try/catch handlers (log or toast the errors) so the flow remains
anchored on createdOrgId even if refresh fails.
In `@src/stores/organization.ts`:
- Around line 379-390: The early-return branch after computing
selectableOrganizations leaves invite-only entries in _organizations, which lets
the watcher rebuild _organizationsByAppId from those rows; update this branch to
also remove invite-only rows from _organizations and then clear/rebuild the app
map: filter _organizations (the reactive store holding mappedData rows) to
exclude invite-only entries using the same isSelectableOrganization(org.role)
predicate, set _organizationsByAppId.value to an empty Map, and resolve
_initialLoadPromise.value as before so no watcher can resurrect invite-only orgs
for app lookups; use the existing
selectableOrganizations/mappedData/_organizationsByAppId/_organizations symbols
to locate and change the code.
In `@supabase/functions/_backend/private/website_preview.ts`:
- Around line 109-113: The code must validate that body.website is a string
before passing it to normalizeWebsiteUrl to avoid runtime errors for non-string
JSON values; update the app.post handler (where parseBody and
normalizeWebsiteUrl are used) to read const raw = body.website, check if typeof
raw !== 'string' or raw.trim() === '' and return quickError(400,
'invalid_website', 'Website must be a valid URL') for invalid input, otherwise
call normalizeWebsiteUrl(raw.trim()) and continue as before.
---
Duplicate comments:
In `@src/pages/onboarding/organization.vue`:
- Around line 302-307: The watcher on route.query.step allows 'logo' and
'invite' unconditionally, which can advance the flow without a createdOrgId and
break onboarding; update the watcher in src/pages/onboarding/organization.vue to
revalidate before setting step.value by either calling
hydrateOnboardingFromQuery() or checking the same condition used there (ensure
createdOrgId exists) — only set step.value to 'logo' or 'invite' when
createdOrgId is present, otherwise restrict to 'details' or fallback to the safe
step; reference the existing hydrateOnboardingFromQuery(), route.query.step,
createdOrgId, and step.value to locate and fix the logic.
---
Nitpick comments:
In `@playwright/e2e/register.spec.ts`:
- Around line 8-33: The test only validates the post-signup redirect to
onboarding; add a second Playwright scenario in register.spec.ts that covers the
dashboard/org-switcher entrypoint by signing in (or reusing the created user),
navigating to '/apps', triggering the dashboard/org-switcher "new org" action
(the selector tied to the PR's change — e.g. the org switcher button and the
"new org" item used in UI), asserting it routes into the onboarding flow
(/onboarding/organization), then complete the same onboarding steps (fill org
name, skip logo, finish) and assert final /apps redirect; implement this as a
separate test (e.g., 'should route new org from org-switcher into onboarding')
using the same uniqueSuffix pattern and the existing data-test selectors used in
the file to locate elements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e739da54-baa0-44fb-970e-14eb51eeca0d
📒 Files selected for processing (8)
playwright/e2e/register.spec.tssrc/components/dashboard/DropdownOrganization.vuesrc/modules/auth.tssrc/pages/onboarding/organization.vuesrc/services/photos.tssrc/stores/organization.tssupabase/functions/_backend/private/website_preview.tssupabase/functions/private/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/dashboard/DropdownOrganization.vue
- src/services/photos.ts
|
Progress on PR #1835: I fixed the onboarding flow regressions and resolved the linked review threads for admin-route onboarding, deep-link preservation, post-create refresh safety, invite-only org leakage, and website-preview input validation.\n\nCI still fails in |
|
CI is still failing on Run tests in with at lines 89 and 162 ( / ). I wasn’t able to recover a concrete SDK error from the job logs, so I can’t safely patch this branch without a local repro. The onboarding review comments that were still open are addressed in the current branch state, but the remaining failure looks like a shared CLI/dependency issue rather than the onboarding change itself. |
|
CI is still failing on Run tests in tests/cli-min-version.test.ts with AssertionError: expected false to be true at lines 89 and 162 (result0.success / result2.success). I could not recover a concrete SDK error from the job logs, so I cannot safely patch this branch without a local repro. The onboarding review comments that were still open are addressed in the current branch state, but the remaining failure looks like a shared CLI/dependency issue rather than the onboarding change itself. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 212197cfd7
ℹ️ 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
🧹 Nitpick comments (4)
supabase/functions/_backend/private/website_preview.ts (1)
46-61: Static analysis ReDoS warning - low risk given hardcoded keys.The static analysis flags regex construction from the
keyparameter as a ReDoS risk. However,findMetaContentis only called with hardcoded string literals ('og:site_name','application-name'), so the risk is minimal in practice.If you want to eliminate the warning entirely, you could use a fixed lookup instead of dynamic regex:
Optional: static pattern approach
const META_PATTERNS = { 'og:site_name': [ /<meta[^>]+property=["']og:site_name["'][^>]+content=["']([^"']+)["'][^>]*>/i, /<meta[^>]+content=["']([^"']+)["'][^>]+property=["']og:site_name["'][^>]*>/i, ], 'application-name': [ /<meta[^>]+name=["']application-name["'][^>]+content=["']([^"']+)["'][^>]*>/i, /<meta[^>]+content=["']([^"']+)["'][^>]+name=["']application-name["'][^>]*>/i, ], } as const function findMetaContent(html: string, key: keyof typeof META_PATTERNS) { for (const pattern of META_PATTERNS[key]) { const match = pattern.exec(html) if (match?.[1]) return decodeHtmlEntities(match[1]) } return '' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/_backend/private/website_preview.ts` around lines 46 - 61, Replace the dynamic RegExp construction in findMetaContent with a static map of precompiled RegExp patterns (e.g., META_PATTERNS) keyed by the allowed meta keys and update findMetaContent to accept only those keys (keyof typeof META_PATTERNS); iterate the precompiled patterns for the given key, exec each, and return decodeHtmlEntities(match[1]) when found, otherwise return ''. This removes runtime RegExp construction and the ReDoS warning while keeping decodeHtmlEntities and the existing match logic intact.src/modules/auth.ts (2)
99-100: Consider simplifyingredirectToOrgOnboardingto a constant.
redirectToOrgOnboardingis defined as a function buttodoesn't change during the guard execution. A simple boolean would be clearer:- const redirectToOrgOnboarding = () => !to.path.startsWith('/onboarding/organization') + const shouldRedirectToOrgOnboarding = !to.path.startsWith('/onboarding/organization') const isAdminRoute = to.path.startsWith('/admin')Then use
shouldRedirectToOrgOnboardingdirectly instead of callingredirectToOrgOnboarding().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/auth.ts` around lines 99 - 100, The guard defines redirectToOrgOnboarding as a function but it never depends on changing state — replace it with a boolean (e.g. shouldRedirectToOrgOnboarding = !to.path.startsWith('/onboarding/organization')) and update all usages of redirectToOrgOnboarding() to use the boolean directly; keep the existing isAdminRoute = to.path.startsWith('/admin') as-is and ensure any references (redirectToOrgOnboarding, shouldRedirectToOrgOnboarding) in the surrounding auth guard logic are updated to the new identifier and removed parentheses.
172-192: DuplicateisPlatformAdmin()call for admin users navigating to onboarding.When an admin user without orgs navigates to a non-admin route,
isPlatformAdmin()is called at line 176, then called again at line 200. The second call is unconditional and will overwritemain.isAdmin. Consider restructuring to avoid the duplicate RPC:- if (organizationsLoaded && !organizationStore.hasOrganizations && redirectToOrgOnboarding()) { - if (isAdminRoute) { - try { - main.isAdmin = main.isAdmin ?? await isPlatformAdmin() - } - catch (error) { - console.error('Failed to resolve platform admin status:', error) - main.isAdmin = false - } - } - - if (!isAdminRoute || !main.isAdmin) { - return next({ + // Resolve admin status once for all checks + try { + main.isAdmin = await isPlatformAdmin() + } + catch (error) { + console.error('Failed to resolve platform admin status:', error) + main.isAdmin = false + } + + if (organizationsLoaded && !organizationStore.hasOrganizations && shouldRedirectToOrgOnboarding) { + if (!isAdminRoute || !main.isAdmin) { + return next({This moves the admin check earlier and removes lines 198-205.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/auth.ts` around lines 172 - 192, The code makes duplicate isPlatformAdmin() RPCs for admin users during onboarding flow; ensure you call isPlatformAdmin() once and reuse the result by populating main.isAdmin before the later conditional instead of re-calling it: move the admin check into the block after organizationsLoaded (where tryLoadOrganizations/organizationStore.fetchOrganizations runs) and set main.isAdmin = main.isAdmin ?? await isPlatformAdmin() (with the existing try/catch) and remove the later unconditional isPlatformAdmin() invocation so subsequent logic (the redirectToOrgOnboarding/isAdminRoute checks and the next({ path: '/onboarding/organization' ... })) uses the cached main.isAdmin value.src/stores/organization.ts (1)
379-392: Redundant reassignment when no selectable orgs exist.Line 385 reassigns
_organizations.valueto a Map built fromselectableOrganizations, but at this pointselectableOrganizationsis empty (the!organizationcheck passed). This overwrites the full org list with an empty map, which may be intentional to prevent UI from showing invite-only orgs, but the intent isn't clear.If the goal is to clear the map when no selectable orgs exist, consider making that explicit:
if (!organization) { - _organizations.value = new Map(selectableOrganizations.map(item => [item.gid, item as Organization])) + // No selectable orgs - clear the store to prevent invite-only orgs from appearing + _organizations.value = new Map()Otherwise, if invite-only orgs should remain visible (but not selectable), remove line 385 entirely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/organization.ts` around lines 379 - 392, The code currently assigns _organizations.value = new Map(selectableOrganizations.map(...)) after determining there is no selectable organization (organization is undefined), which overwrites the full org list with an empty map; either make the intent explicit by clearing the map and adding a comment (e.g., "clear visible organizations because none are selectable") or remove that assignment so invite-only organizations remain in _organizations.value while still leaving currentOrganization, currentRole, currentOrganizationFailed, _organizationsByAppId, and _initialLoadPromise unchanged; update the block around selectableOrganizations, organization, and _organizations.value accordingly to reflect the chosen behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/onboarding/organization.vue`:
- Around line 257-277: In useImportedLogo, avoid calling fetch on data: URLs
(importedLogoUrl) — detect if importedLogoUrl.value startsWith('data:') and, in
that case, convert the data URL to a Blob client-side (parse the MIME/type and
base64 payload, decode to a Uint8Array and create a Blob) and then call
uploadLogoBlob(blob, `${websiteHostname.value || 'website-logo'}.png`);
otherwise keep the existing fetch path and response/content-type checks and the
same error handling in the catch block.
In `@supabase/functions/_backend/private/website_preview.ts`:
- Around line 27-35: The decodeHtmlEntities function currently uses DOMParser
(which is unavailable in Cloudflare Workers) so replace its implementation in
decodeHtmlEntities with a CF-Workers-compatible decoder: remove DOMParser usage
and implement a lightweight entity decoder (either add the small regex-based
replacer for common entities like &, <, >, ", ', numeric
entities, etc., or import a worker-compatible package such as "he"); ensure the
function returns the decoded and trimmed string (preserve the existing function
name decodeHtmlEntities and its single string parameter) and keep error-free
behavior in the Workers runtime.
---
Nitpick comments:
In `@src/modules/auth.ts`:
- Around line 99-100: The guard defines redirectToOrgOnboarding as a function
but it never depends on changing state — replace it with a boolean (e.g.
shouldRedirectToOrgOnboarding = !to.path.startsWith('/onboarding/organization'))
and update all usages of redirectToOrgOnboarding() to use the boolean directly;
keep the existing isAdminRoute = to.path.startsWith('/admin') as-is and ensure
any references (redirectToOrgOnboarding, shouldRedirectToOrgOnboarding) in the
surrounding auth guard logic are updated to the new identifier and removed
parentheses.
- Around line 172-192: The code makes duplicate isPlatformAdmin() RPCs for admin
users during onboarding flow; ensure you call isPlatformAdmin() once and reuse
the result by populating main.isAdmin before the later conditional instead of
re-calling it: move the admin check into the block after organizationsLoaded
(where tryLoadOrganizations/organizationStore.fetchOrganizations runs) and set
main.isAdmin = main.isAdmin ?? await isPlatformAdmin() (with the existing
try/catch) and remove the later unconditional isPlatformAdmin() invocation so
subsequent logic (the redirectToOrgOnboarding/isAdminRoute checks and the next({
path: '/onboarding/organization' ... })) uses the cached main.isAdmin value.
In `@src/stores/organization.ts`:
- Around line 379-392: The code currently assigns _organizations.value = new
Map(selectableOrganizations.map(...)) after determining there is no selectable
organization (organization is undefined), which overwrites the full org list
with an empty map; either make the intent explicit by clearing the map and
adding a comment (e.g., "clear visible organizations because none are
selectable") or remove that assignment so invite-only organizations remain in
_organizations.value while still leaving currentOrganization, currentRole,
currentOrganizationFailed, _organizationsByAppId, and _initialLoadPromise
unchanged; update the block around selectableOrganizations, organization, and
_organizations.value accordingly to reflect the chosen behavior.
In `@supabase/functions/_backend/private/website_preview.ts`:
- Around line 46-61: Replace the dynamic RegExp construction in findMetaContent
with a static map of precompiled RegExp patterns (e.g., META_PATTERNS) keyed by
the allowed meta keys and update findMetaContent to accept only those keys
(keyof typeof META_PATTERNS); iterate the precompiled patterns for the given
key, exec each, and return decodeHtmlEntities(match[1]) when found, otherwise
return ''. This removes runtime RegExp construction and the ReDoS warning while
keeping decodeHtmlEntities and the existing match logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a6f5c61-b32e-4d7f-b0be-2d5345a33bd0
📒 Files selected for processing (4)
src/modules/auth.tssrc/pages/onboarding/organization.vuesrc/stores/organization.tssupabase/functions/_backend/private/website_preview.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1284e39205
ℹ️ 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: 3
♻️ Duplicate comments (2)
src/stores/organization.ts (1)
377-391:⚠️ Potential issue | 🟠 MajorPublish only selectable orgs into
_organizations.Line 377 stores every row before the no-selectable branch runs. The watcher at Lines 229-268 can capture that first map and rebuild
_organizationsByAppIdfrom invite-only rows after the reset, which reopens the invite-only org leakage this change is trying to close. Compute the selectable set first, then assign_organizationsonce with only publishable orgs.Possible fix
- _organizations.value = new Map(mappedData.map(item => [item.gid, item as Organization])) - const selectableOrganizations = mappedData .filter(org => isSelectableOrganization(org.role)) .sort((a, b) => b.app_count - a.app_count) const organization = selectableOrganizations[0] if (!organization) { // Clear visible organizations because none are selectable. _organizations.value = new Map() currentOrganization.value = undefined currentRole.value = null currentOrganizationFailed.value = false _organizationsByAppId.value = new Map() _initialLoadPromise.value.resolve(true) return } + + _organizations.value = new Map( + selectableOrganizations.map(item => [item.gid, item as Organization]), + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/stores/organization.ts` around lines 377 - 391, Compute the selectable organizations before mutating _organizations: filter mappedData with isSelectableOrganization to produce selectableOrganizations, sort it, then assign _organizations.value = new Map(selectableOrganizations.map(item => [item.gid, item as Organization])) instead of assigning all mappedData first; after that proceed with the existing logic that reads the first selectable organization and updates _organizationsByAppId, currentOrganization, currentRole, currentOrganizationFailed, and _initialLoadPromise so invite-only rows never get published into _organizations or trigger the watcher.src/pages/onboarding/organization.vue (1)
327-335:⚠️ Potential issue | 🟠 MajorMake the final org refresh best-effort too.
If
organizationStore.fetchOrganizations()throws here, onboarding is already complete but the user never reachesnextPath. Catch the refresh failure and still navigate, the same way the create step now treats its post-create refresh.Possible fix
async function finishOnboarding() { - await organizationStore.fetchOrganizations() - if (activeOrgId.value) - organizationStore.setCurrentOrganization(activeOrgId.value) + try { + await organizationStore.fetchOrganizations() + if (activeOrgId.value) + organizationStore.setCurrentOrganization(activeOrgId.value) + } + catch (error) { + console.error('Failed to refresh organizations after onboarding finish', error) + } const nextPath = typeof route.query.to === 'string' && route.query.to && !route.query.to.startsWith('/onboarding/') ? route.query.to : '/apps' await router.push(nextPath) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/onboarding/organization.vue` around lines 327 - 335, The final org refresh in finishOnboarding can throw and block navigation; wrap the call to organizationStore.fetchOrganizations() in a try/catch (inside the finishOnboarding function) so failures are best-effort: if fetchOrganizations throws, catch the error (optionally log it) and proceed to setCurrentOrganization(activeOrgId.value) and compute/await router.push(nextPath) as before so navigation always occurs even when the refresh fails.
🤖 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/functions/_backend/private/website_preview.ts`:
- Around line 119-133: The code currently calls getWebhookUrlValidationError(c,
currentUrl) which only blocks localhost/literal IPs; add a DNS resolution +
IP-range check before each outbound fetch and immediately after following a
redirect: resolve the hostname from currentUrl to one or more IPs (e.g., via a
DNS lookup helper), verify none are in RFC1918/link-local/metadata/mapped
ranges, and return the same error shape if any IP is disallowed; apply this
check right before the initial fetch call and again after computing location/new
URL in the redirect branch (referencing currentUrl, response, location, and
fetch) so no redirected hostnames bypass the validation.
- Around line 123-126: Wrap the fetch(currentUrl, { ...init, redirect: 'manual'
}) call in a try-catch and convert thrown network/DNS/TLS errors into the
function's expected error contract: on error return the same shape used
elsewhere (e.g., { error: 'invalid_website', response: null } or the equivalent
error object your caller expects) so the page-fetch path returns a 400-style
invalid_website error instead of throwing and the icon-fetch path returns null
rather than crashing; update the block around the fetch call and any early
returns that consume its result (referencing the fetch call, currentUrl, init
and the local response variable) to use the caught error to produce the proper
return value.
- Around line 1-12: The file instantiates Hono directly (export const app = new
Hono<MiddlewareKeyVariables>()) instead of using the shared factory that injects
request context middleware; replace that instantiation with createHono('',
version) and import createHono from '../utils/hono.ts' and version from
'../utils/version.ts' so the route inherits requestId/cf-ray, logging,
worker-source header and optional Sentry integration; ensure existing
app.use('/', useCors) remains unchanged and remove the direct Hono import if
unused.
---
Duplicate comments:
In `@src/pages/onboarding/organization.vue`:
- Around line 327-335: The final org refresh in finishOnboarding can throw and
block navigation; wrap the call to organizationStore.fetchOrganizations() in a
try/catch (inside the finishOnboarding function) so failures are best-effort: if
fetchOrganizations throws, catch the error (optionally log it) and proceed to
setCurrentOrganization(activeOrgId.value) and compute/await
router.push(nextPath) as before so navigation always occurs even when the
refresh fails.
In `@src/stores/organization.ts`:
- Around line 377-391: Compute the selectable organizations before mutating
_organizations: filter mappedData with isSelectableOrganization to produce
selectableOrganizations, sort it, then assign _organizations.value = new
Map(selectableOrganizations.map(item => [item.gid, item as Organization]))
instead of assigning all mappedData first; after that proceed with the existing
logic that reads the first selectable organization and updates
_organizationsByAppId, currentOrganization, currentRole,
currentOrganizationFailed, and _initialLoadPromise so invite-only rows never get
published into _organizations or trigger the watcher.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2c180c7-8ad6-4667-a8c8-9aeb30d36ce4
📒 Files selected for processing (4)
src/modules/auth.tssrc/pages/onboarding/organization.vuesrc/stores/organization.tssupabase/functions/_backend/private/website_preview.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/auth.ts
…instead-of-auto-create
…instead-of-auto-create
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09d86d9696
ℹ️ 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/onboarding/organization.vue`:
- Around line 518-520: The preview logic currently uses currentOrganization.logo
even when currentOrganization is still the previous org; update the v-if and
:src checks to only use currentOrganization.logo when activeOrgName (the same
gate used for currentOrganization.name) is truthy—i.e., change the conditions
around currentOrganization?.logo to also require activeOrgName, so the component
prefers selectedLogoPreview or importedLogoUrl until the new org is loaded;
apply the same change to the second occurrence around lines with the same
bindings (the block using
selectedLogoPreview/importedLogoUrl/currentOrganization?.logo).
- Around line 329-337: In finishOnboarding, don't let a failing
organizationStore.fetchOrganizations abort navigation: wrap the await
organizationStore.fetchOrganizations() call in a try/catch (similar to
createOrganization's error handling), log or ignore the error as appropriate,
and continue to setCurrentOrganization and always await router.push(nextPath) so
the final navigation to nextPath happens even if the refresh fails.
In `@src/services/photos.ts`:
- Around line 184-185: The call to organizationStore.fetchOrganizations() should
be made best-effort so a failure there doesn't surface as a hard onboarding
error after the logo is already saved; wrap
organizationStore.fetchOrganizations() in a try/catch, log or warn the error
(but do not rethrow), and ensure
organizationStore.setCurrentOrganization(safeOrgId) still runs regardless;
reference the existing organizationStore.fetchOrganizations and
organizationStore.setCurrentOrganization calls and handle errors locally so the
flow continues even if fetchOrganizations fails.
In `@tests/app-error-cases.test.ts`:
- Around line 20-41: The test is inserting directly into the 'apikeys' table via
getSupabaseClient() which does not produce a valid credential the backend
accepts; replace this direct insert with the same helper or RPC the rest of the
suite uses (the project's API-key seeding RPC/helper) so the key is registered
the backend recognizes; update the setup to call that RPC/helper instead of
inserting into 'apikeys', then set testApiKeyId and testHeaders from the
RPC/helper response (keeping the 'Authorization' header = testApiKeyValue) so
the requests hit the /app handlers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f7182e9-0ff5-42b6-9640-8ec924af74b2
📒 Files selected for processing (8)
src/main.tssrc/pages/onboarding/organization.vuesrc/route-map.d.tssrc/services/photos.tssrc/stores/organization.tssupabase/migrations/20260319235626_disable_auto_org_on_user_create.sqlsupabase/seed.sqltests/app-error-cases.test.ts
💤 Files with no reviewable changes (1)
- supabase/seed.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- src/stores/organization.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0294bc8ec2
ℹ️ 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: f477e165fa
ℹ️ 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: 38ccac11e3
ℹ️ 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 (normalized.startsWith('fe80:') || normalized.startsWith('fc') || normalized.startsWith('fd')) | ||
| return true |
There was a problem hiding this comment.
isPrivateIpv6() only blocks the literal fe80: prefix, but link-local addresses cover the entire fe80::/10 range (fe8* through feb*). In IPv6-enabled deployments, a hostname that resolves to something like fe90::1 will pass validation and still be fetched by /private/website_preview, so this hardening still leaves an SSRF path to non-public hosts.
Useful? React with 👍 / 👎.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a7c452e70
ℹ️ 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 (!organization) { | ||
| // Clear visible organizations because none are selectable. | ||
| _organizations.value = new Map() | ||
| currentOrganization.value = undefined | ||
| currentRole.value = null |
There was a problem hiding this comment.
Fresh evidence in this revision: hasOrganizations now only counts selectable orgs, so src/modules/auth.ts redirects accounts whose only memberships are pending invites into /onboarding/organization. This branch then wipes _organizations, and repo search shows DropdownOrganization.vue is the only UI that calls accept_invitation_to_org, so an existing user with only invite rows loses their only way to accept the invite and is effectively stuck on onboarding/create-org instead of joining the org they were invited to.
Useful? React with 👍 / 👎.
| const ips = [ | ||
| ...await resolveHostnameIps(url.hostname, 'A'), | ||
| ...await resolveHostnameIps(url.hostname, 'AAAA'), | ||
| ] |
There was a problem hiding this comment.
getPublicHostnameValidationError() resolves the hostname through Cloudflare DoH, but fetchValidatedUrl() later still calls fetch(currentUrl) with the original hostname. In split-horizon or DNS-rebinding scenarios, an attacker-controlled domain can return a public IP to the DoH check and a private/internal IP to the runtime resolver, so /private/website_preview remains an SSRF primitive on deployments that can reach internal hosts. The actual fetch needs to be bound to the validated resolution (or use the same resolver for both steps) for the private-host check to be trustworthy.
Useful? React with 👍 / 👎.
2 participants
Summary (AI generated)
generate_org_on_user_createtrigger behaviorMotivation (AI generated)
Capgo should not create an organization implicitly when a user registers. The first organization should be created explicitly through onboarding so the setup flow is intentional and consistent for both first-time users and anyone creating a new organization later from the selector.
Business Impact (AI generated)
This makes first-run setup clearer, reduces accidental organization creation, and aligns the product with a more guided onboarding path that can improve team activation and org configuration quality before users begin creating apps.
Test Plan (AI generated)
bun lintbun typecheckbun test tests/sso.test.tswith local Supabase running/onboarding/organizationand no org exists before the onboarding create stepGenerated with AI
Summary by CodeRabbit