feat: a rework of the signup flow#199
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAuth flows now route users through onboarding-aware login, callback, confirmation, logout, and profile-completion steps. Supabase configuration, migration, and setup scripts were also updated for new OAuth providers and profile bootstrapping. ChangesAuth onboarding flow
Supabase config and bootstrap
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Login as login action
participant Callback as auth callback
participant Confirm as auth confirm
participant Guard as auth guard
participant Complete as complete-profile action
Browser->>Login: submit credentials
Login->>Login: validate and sign in
Login->>Complete: redirect when profile is missing or not onboarded
Browser->>Callback: OAuth return
Callback->>Callback: exchange code and inspect profile onboarding
Browser->>Confirm: email verification return
Confirm->>Confirm: verify session and profile state
Guard->>Guard: load profile onboarded flag
Guard->>Complete: redirect unauthenticated or incomplete users
Browser->>Complete: submit display_name and username
Complete->>Complete: update profile and mark onboarded
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/`(auth)/auth/logout/+server.ts:
- Around line 6-17: The logout handler currently lets an empty scope string
bypass validation because the check in the auth logout route only runs when
scope is truthy. Update the validation in the logout server handler to reject
any provided scope value that is not one of the allowed SIGN_OUT_SCOPES,
including empty strings, by checking for scope !== null and validating the
actual string before calling supabase.auth.signOut. Keep the fix centered in the
logout route logic around the scope parsing and logError path.
🪄 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 Plus
Run ID: 10cad96f-6c86-4261-860f-d5918465c5e2
📒 Files selected for processing (1)
src/routes/(auth)/auth/logout/+server.ts
… login error handling - Added Google OAuth login functionality in `src/routes/(auth)/auth/google/+server.ts`. - Introduced WCA OAuth login functionality in `src/routes/(auth)/auth/wca/+server.ts`. - Improved error handling for Discord login in `src/routes/(auth)/auth/discord/+server.ts`. - Updated signup process to streamline account creation and profile completion in `src/routes/(auth)/auth/signup/+page.server.ts` and `src/routes/(auth)/auth/signup/+page.svelte`. - Refactored profile creation SQL migration to enforce constraints and add triggers for automatic profile creation upon user registration. - Adjusted Supabase configuration for external authentication providers.
…mprove error handling
… and user feedback
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks.server.ts (1)
128-149: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTreat a missing profile as recoverable onboarding, not a hard failure.
Issue
#198is specifically “signed-in user without profile”. Using.single()here turns that state into an error before the user can be redirected into/auth/complete-profile, and Line 143 assumesprofilealways exists.maybeSingle()plus!profile || !profile.onboardedwould let those users recover instead of hitting another 500.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks.server.ts` around lines 128 - 149, Treat the signed-in-but-missing-profile case as recoverable in the profile lookup flow inside hooks.server.ts. Update the Supabase query in the profile fetch logic to avoid using .single() for this path, and handle a null/absent profile explicitly in the conditional that checks onboarded so users without a profile can still be redirected to /auth/complete-profile instead of triggering logError and a 500. Reference the profile fetch block in the hook and the onboarded guard that currently assumes profile always exists.src/routes/(auth)/auth/callback/+server.ts (1)
9-38: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAbort after fatal auth errors instead of falling through.
Lines 10-31 now only log fatal conditions, but Line 37 still dereferences
user.id. A missingcode, failed code exchange, or missinguserwill now cascade into a second crash instead of surfacing the intended auth failure. Return or throw from each fatal branch before continuing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/`(auth)/auth/callback/+server.ts around lines 9 - 38, The auth callback handler in +server.ts is logging fatal conditions but still continues execution, which lets later code dereference user.id after a missing code, failed exchange, or absent user. Update the callback flow to stop immediately in each fatal branch inside the auth callback logic (after the code check, exchangeCodeForSession error, and missing user case) by returning or throwing before reaching the profiles query, so the handler never falls through after a fatal auth error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/components/helper_functions/addToEmailList.ts`:
- Around line 9-27: The Brevo call in addToEmailList is not protected against
network failures or hanging requests, so it can throw outside the { success,
error } contract or stall the auth flow. Update the fetch logic in
addToEmailList to use a timeout and wrap the request in error handling so any
fetch rejection or abort is converted into a { success: false, error } result,
while preserving the existing success path and response-text handling.
In `@src/lib/components/validation/auth.ts`:
- Line 5: The email validator in auth.ts is using z.email(...).trim(), but
z.email() does not support trim chaining. Update the email schema in the
validation logic to start from a string schema and apply trim before email
validation, or preprocess whitespace before validating, while keeping the
existing "Please enter a valid email address" message.
In `@src/routes/`(auth)/auth/complete-profile/+page.server.ts:
- Around line 61-63: The uniqueness validation in the profile completion handler
is attached to the wrong field: in the branch handling profileUpdateError.code
=== "23505" inside the complete profile page server logic, update the setError
call so the "username" field receives the error instead of "display_name". Keep
the existing message text, but make sure the error is bound to the username
input so the UI highlights the correct field.
- Around line 52-59: The profile save logic in the complete-profile server
action only updates an existing row, so users with no `profiles` record remain
profile-less after finishing onboarding. Update the `supabase.from("profiles")`
write in `+page.server.ts` to use an insert-on-miss approach, such as `upsert`,
keyed by `user_id`, while still setting `username`, `display_name`, and
`onboarded` in the same flow. Keep the existing error handling around
`profileUpdateError` and ensure the `profiles` row is created when absent.
In `@src/routes/`(auth)/auth/confirm/+server.ts:
- Around line 8-58: The confirm handler in the auth callback continues execution
after each logError call, which can lead to null dereferences and invalid
session handling. In the +server.ts route handler, add immediate returns after
the missing code, exchangeCodeForSession, profile lookup, and profile existence
error branches so execution stops before using code, data.user, or
existingProfile. Make sure the control flow in the confirm endpoint only reaches
the onboarding and user redirects when authErr, profileErr, and existingProfile
checks have all passed.
In `@src/routes/`(auth)/auth/discord/+server.ts:
- Around line 16-20: The Discord OAuth start flow in the handler should not
always call redirect after signInWithOAuth returns. Update the logic around the
error check so that redirect(307, data.url) only executes on the success path
when data.url is present, and on the error path return or stop execution after
logError to avoid a second failure.
In `@src/routes/`(auth)/auth/login/+page.server.ts:
- Around line 54-73: The login handler in auth/login/+page.server.ts still
returns a 500 from the profiles lookup when no `profiles` row exists; update the
`profileErr` handling in the login flow so `profiles` missing/incomplete data is
recovered instead of failing. In the logic around
`supabase.from("profiles").select("username").eq("user_id", user.id).single()`
and the subsequent redirect, detect the missing-profile case and either
create/bootstrap the profile or send the user to profile completion before the
final `redirect(303, ...)`, while keeping real lookup errors on the failure
path.
- Around line 63-70: The redirect_to validation in the login page server allows
backslash-based network-path redirects, so tighten the check in +page.server.ts
before calling redirect(). Reject any value containing backslashes and
normalize/validate the target using URL so only same-site relative paths are
allowed; keep the existing redirect_to guard logic and adjust the redirect call
to use the validated normalized path.
In `@src/routes/`(auth)/auth/login/+page.svelte:
- Around line 89-94: The login form’s password input is not bound, so taint
tracking never marks password-only typing and the submit button stays disabled.
Update the password field in the login page component to use a bound value on
the relevant input so changes are tracked by the form state and
isTainted($tainted) becomes true when the user types. Make the same binding
change for the other affected password field mentioned in the component so both
inputs participate in taint tracking.
In `@src/routes/`(auth)/auth/signup/+page.server.ts:
- Around line 66-72: The 401 branch in the signup server action is returning the
wrong form key, so the active survey form is not repopulated and the error
message is lost. Update the fail(401, ...) response in the signup handler to
return surveyForm instead of profileForm, and keep the existing form data plus
the missing-user message so the UI can render it correctly.
In `@src/routes/`(auth)/auth/signup/+page.svelte:
- Around line 12-18: The signup account form in the superForm setup is missing a
submitting/in-flight state, so duplicate submissions can happen before the first
request completes. Update the signup page’s account form wiring to track the
form’s submitting status and use it to disable the submit action/button while
the request is pending, using the existing superForm bindings such as account,
enhanceAccount, accountMessage, and accountConstraints to locate the form state
and prevent double-click resubmits.
In `@src/routes/`(auth)/auth/wca/+server.ts:
- Around line 16-20: The OAuth startup path in the WCA login handler needs a
guard before redirecting, because when signInWithOAuth() fails data.url may be
undefined and redirect(307, data.url) will throw. In the +server route handler,
keep the existing error check around logError(500, "Failed to initiate WCA
login", log, error) and return or otherwise exit early on failure so redirect is
only reached when data.url is present; use the signInWithOAuth result handling
in this route to locate the fix.
In `@supabase/config.toml`:
- Around line 120-122: The Supabase auth redirect allowlist only includes the
alternate origin root, but `redirectTo` and `emailRedirectTo` require exact
callback URLs including paths. Update the `additional_redirect_urls` entry in
the auth config to include the full localhost 5174 callback and confirmation
URLs, using the same `/auth/callback` and `/auth/confirm` paths that the client
sends from the auth flow.
In `@supabase/migrations/20260629085141_profile-creation.sql`:
- Around line 5-7: The default UUID on brands.added_by_id and
cube_models.verified_by_id is hard-coded to a profile that may not exist, so
update the migration to avoid referencing an unbootstrapped public.profiles row.
In the migration that alters these defaults, replace the static UUID with a safe
seeded profile reference, a dynamically created bootstrap profile, or remove the
FK default entirely if it cannot be guaranteed. Make the same fix for the
related statements mentioned in the review so all affected FK defaults use the
same safe source.
In `@supabase/scripts/setup-wca-provider.js`:
- Around line 8-10: The Supabase client setup only rejects the config when both
env vars are missing, so a single missing value can still reach createClient and
fail later. Update the validation in setup-wca-provider.js to require both
supabaseUrl and supabaseSecretKey before calling createClient, and keep the
error handling close to that check so the failure is immediate and explicit.
- Around line 32-39: The setup flow in registerWCAProvider currently logs
Supabase creation errors but still exits successfully, which can let CI report
success incorrectly. Update the error path in setup-wca-provider.js so that when
the provider creation call returns an error, the script fails with a non-zero
exit (for example after the console.error in registerWCAProvider), while keeping
the success log unchanged.
---
Outside diff comments:
In `@src/hooks.server.ts`:
- Around line 128-149: Treat the signed-in-but-missing-profile case as
recoverable in the profile lookup flow inside hooks.server.ts. Update the
Supabase query in the profile fetch logic to avoid using .single() for this
path, and handle a null/absent profile explicitly in the conditional that checks
onboarded so users without a profile can still be redirected to
/auth/complete-profile instead of triggering logError and a 500. Reference the
profile fetch block in the hook and the onboarded guard that currently assumes
profile always exists.
In `@src/routes/`(auth)/auth/callback/+server.ts:
- Around line 9-38: The auth callback handler in +server.ts is logging fatal
conditions but still continues execution, which lets later code dereference
user.id after a missing code, failed exchange, or absent user. Update the
callback flow to stop immediately in each fatal branch inside the auth callback
logic (after the code check, exchangeCodeForSession error, and missing user
case) by returning or throwing before reaching the profiles query, so the
handler never falls through after a fatal auth error.
🪄 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 Plus
Run ID: 1317845a-0eca-46b8-ba62-871c30190cb4
📒 Files selected for processing (25)
.env.examplesrc/hooks.server.tssrc/lib/components/helper_functions/addToEmailList.tssrc/lib/components/layout/ExternalAuthProviders.sveltesrc/lib/components/validation/auth.tssrc/routes/(auth)/+layout.server.tssrc/routes/(auth)/auth/callback/+server.tssrc/routes/(auth)/auth/complete-profile/+page.server.tssrc/routes/(auth)/auth/complete-profile/+page.sveltesrc/routes/(auth)/auth/confirm/+server.tssrc/routes/(auth)/auth/discord/+server.tssrc/routes/(auth)/auth/google/+server.tssrc/routes/(auth)/auth/login/+page.server.tssrc/routes/(auth)/auth/login/+page.sveltesrc/routes/(auth)/auth/logout/+server.tssrc/routes/(auth)/auth/resend/+server.tssrc/routes/(auth)/auth/reset/+page.server.tssrc/routes/(auth)/auth/reset/+page.sveltesrc/routes/(auth)/auth/reset/+page.tssrc/routes/(auth)/auth/signup/+page.server.tssrc/routes/(auth)/auth/signup/+page.sveltesrc/routes/(auth)/auth/wca/+server.tssupabase/config.tomlsupabase/migrations/20260629085141_profile-creation.sqlsupabase/scripts/setup-wca-provider.js
💤 Files with no reviewable changes (2)
- src/routes/(auth)/+layout.server.ts
- src/routes/(auth)/auth/reset/+page.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/components/layout/ExternalAuthProviders.svelte
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/(auth)/auth/logout/+server.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/`(auth)/auth/login/+page.server.ts:
- Around line 69-70: The redirect handling in the login page server can still
throw when `new URL(redirect_to, url.origin)` receives malformed absolute input.
Update the `+page.server.ts` redirect flow to validate `redirect_to` before
constructing the `URL`, or wrap the `URL` parsing in a try/catch inside the
login handler so invalid values are rejected safely instead of causing a 500.
Use the existing `redirect_to` check and the `target` construction in the login
action as the place to apply the fix.
🪄 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 Plus
Run ID: fb586eba-ddc5-4473-b128-4bcdae34db70
📒 Files selected for processing (8)
src/lib/components/helper_functions/addToEmailList.tssrc/lib/components/validation/auth.tssrc/routes/(auth)/auth/complete-profile/+page.server.tssrc/routes/(auth)/auth/login/+page.server.tssrc/routes/(auth)/auth/login/+page.sveltesrc/routes/(auth)/auth/signup/+page.server.tssrc/routes/(auth)/auth/signup/+page.sveltesupabase/scripts/setup-wca-provider.js
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/components/helper_functions/addToEmailList.ts
- supabase/scripts/setup-wca-provider.js
- src/lib/components/validation/auth.ts
- src/routes/(auth)/auth/signup/+page.server.ts
- src/routes/(auth)/auth/complete-profile/+page.server.ts
- src/routes/(auth)/auth/login/+page.svelte
- src/routes/(auth)/auth/signup/+page.svelte
✅ Deploy Preview for cubeindex ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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
1 participant
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.
Why:
Already explained in the related issue: #198
Closes #198
Type of change
What's being changed (if available, include any code snippets, screenshots, or gifs):
?scope=to the/auth/signoutrouteChecklist:
Summary by CodeRabbit