Migrate Authentication from Supabase to Clerk#705
Conversation
Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAuthentication is migrated from Supabase to Clerk. Middleware now uses ChangesClerk Authentication Migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Clerk
participant WebhookRoute as app/api/clerk/webhook
participant Database
participant UserSync
participant syncUser
Clerk->>WebhookRoute: POST user.created/updated event
WebhookRoute->>WebhookRoute: verify svix signature
WebhookRoute->>Database: lookup by clerkUserId or email, upsert user
Database-->>WebhookRoute: user record
WebhookRoute-->>Clerk: 200 OK
UserSync->>syncUser: trigger on mount when signed in
syncUser->>Database: resolveClerkUserToDbUser(clerkId)
Database-->>syncUser: db user id or null
sequenceDiagram
participant Client
participant Middleware as middleware.ts
participant ClerkProvider
participant Component as Header/ProfileToggle
Client->>Middleware: request
Middleware->>Middleware: clerkMiddleware() checks route matcher
Middleware-->>Client: continue/allow
Client->>ClerkProvider: render app
ClerkProvider->>Component: provide isLoaded/isSignedIn
Component-->>Client: render sign-in / UserButton / placeholder
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Blocking feedback
- Webhook signature verification uses re-serialized JSON instead of the raw request body, which can invalidate Svix signatures and reject valid Clerk webhooks —
app/api/clerk/webhook/route.ts#L30 useCurrentUser()now returns Clerk IDs asuser.id, but existing settings flows pass that value into DB UUID filters, breaking system prompt reads/writes for signed-in users —lib/auth/use-current-user.ts#L10users.clerkUserIdwas added to the Drizzle schema without a matching migration, so deployments that rundb:migratewon’t add the column and auth queries will fail at runtime —lib/db/schema.ts#L28
If you want Charlie to apply fixes, reply with the item numbers (for example: please fix 1,3).
| } | ||
|
|
||
| // Get the body | ||
| const payload = await req.json() |
There was a problem hiding this comment.
Svix verification needs the exact raw request payload. Parsing with req.json() and then JSON.stringify(payload) can change the byte representation, which can cause valid Clerk webhooks to fail signature verification.
Suggested fix: read the raw body first (const body = await req.text()), verify that raw string, and only then parse JSON for downstream logic.
| // For now, we'll return a simplified version | ||
| return { | ||
| user: user ? { | ||
| id: user.id, |
There was a problem hiding this comment.
This now exposes the Clerk user ID (user_...) as user.id, but existing consumers treat user.id as the internal users.id UUID (for example settings calls that filter with eq(users.id, userId)). That mismatch will cause UUID-cast failures and break settings reads/writes for authenticated users.
Suggested fix: return the internal DB UUID here (or add a separate dbUserId field) and keep clerkUserId distinct so UUID-based queries still receive UUIDs.
| role: text('role').default('viewer'), | ||
| selectedModel: text('selected_model'), | ||
| systemPrompt: text('system_prompt'), | ||
| clerkUserId: text('clerk_user_id').unique(), |
There was a problem hiding this comment.
clerk_user_id is added to the Drizzle schema, but this PR doesn’t include a corresponding migration in drizzle/migrations. Deployments that rely on db:migrate will not create this column, and new queries like where(eq(users.clerkUserId, ...)) will fail at runtime.
Suggested fix: add the SQL migration (and updated drizzle metadata snapshot/journal entries) in the same PR.
Remove logo.svg replacement into account icon and use the existing profile icon and change it into the authenticated profile. Remove the "appearance" option form the drop down menu.
|
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@app/api/clerk/webhook/route.ts`:
- Around line 59-77: The webhook user upsert in the route handler allows
`email_addresses[0]?.email_address` to be undefined, which then gets used in
both `db.update(users).set({ email })` and the `eq(users.email, email)` lookup.
Update the `route.ts` logic around the `existingUser` and `existingEmailUser`
checks to branch on whether `email` is present, so email-based linking and
updates are skipped entirely when there is no email.
- Around line 61-91: The webhook upsert in route.ts is still race-prone because
it does check-then-insert/link around the users table. Update the Clerk webhook
handler so it only sets email when email is defined, and make the create/link
flow atomic in the existing idempotent path around the
db.select/db.update/db.insert logic on users. Ensure duplicate deliveries or
concurrent syncs cannot surface unique constraint errors from clerkUserId/email
and cause the handler to return 500.
In `@components/header.tsx`:
- Around line 56-83: The auth-state rendering in Header is duplicated across the
desktop and mobile sections, so extract the repeated isLoaded/isSignedIn
branching with SignInButton, UserButton, and the loading placeholder into a
shared HeaderAuth component. Reuse that component in the existing header
rendering paths and parameterize the placeholder so the desktop and mobile
variants stay in sync without copy-paste.
- Around line 85-91: The history toggle attached to the QCX heading in the
header component is mouse-only and should be made keyboard-accessible. Update
the interactive element in the header markup by moving the onClick behavior from
the <h1> to a proper button-like control in the same area, or add the required
accessibility affordances and keyboard handler to the existing toggle so it can
be activated with Enter/Space and announced correctly by assistive tech.
In `@components/profile-toggle.tsx`:
- Around line 24-28: The conditional in the ProfileToggle alignment logic is
redundant because both the mobile and non-mobile branches in the effect/handler
set alignValue to "start", so the initial "end" state is never used. Update the
logic around the alignValue setter in ProfileToggle to either remove the dead
if/else entirely and set the value directly, or assign the intended distinct
value for each branch so the state change actually depends on mobile.
- Around line 49-53: The sign-out flow in handleSignOut is doing a manual
browser redirect instead of using Clerk’s built-in redirect support. Update the
signOut call in the ProfileToggle component to pass the redirectUrl option
directly and remove the window.location.href callback so Clerk handles
navigation after sign-out.
- Around line 96-102: The sign-in entry in profile-toggle currently nests
SignInButton inside DropdownMenuItem, which can conflict with the menu closing
behavior; update the dropdown action to trigger Clerk via asChild or explicit
onSelect handling instead of nesting the modal trigger. Also change the icon
used for the sign-in action from LogOut to LogIn so the UI matches the action,
and keep the implementation localized around the SignInButton and
DropdownMenuItem render path.
In `@components/user-sync.tsx`:
- Around line 13-17: The sync trigger in the UserSync effect calls syncUser()
without handling its promise, so a failure can become an unhandled rejection.
Update the useEffect in UserSync to attach a .catch to the syncUser() call, and
in that handler log the error and swallow it so the effect does not leak a
rejection.
In `@lib/auth/get-current-user.ts`:
- Around line 37-83: The create path in resolveClerkUserToDbUser is not
idempotent, so concurrent first-time calls can race into the users insert and
fail on unique email or clerkUserId constraints. Update the creation flow in
get-current-user.ts to use an upsert or conflict-aware retry/re-read strategy
after the Clerk lookup and existing-user checks, so one request can safely
create or link the user while the other resolves the already-created record
instead of returning null from the catch block. Keep the existing lookup/link
logic around currentUser, existingEmailUser, and the final insert, but make the
insert path conflict-safe and re-fetch the user id on conflict.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cbeee59-dba8-4070-abe0-4c0624a77f92
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
app/api/clerk/webhook/route.tsapp/layout.tsxcomponents/header.tsxcomponents/profile-toggle.tsxcomponents/user-sync.tsxlib/actions/sync-user.tslib/auth/get-current-user.tslib/auth/use-current-user.tslib/db/schema.tslib/supabase/browser-client.tslib/supabase/client.tsmiddleware.tspackage.jsonpublic/sw.js
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.1)
public/sw.js
[warning] Avoid using the initial state variable in setState
Context: setTimeout(t,e)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🪛 React Doctor (0.5.8)
components/header.tsx
[warning] 85-85: Keyboard users can't trigger this click handler because there's no keyboard one, so add onKeyUp, onKeyDown, or onKeyPress.
Pair onClick with a key handler so keyboard users can trigger it.
(click-events-have-key-events)
[warning] 85-85: Keyboard & screen reader users can't trigger this <h1> because it isn't interactive, so use a button or link or add an interactive role.
Put interactions on a button or link, or add an interactive role.
(no-noninteractive-element-interactions)
🔇 Additional comments (14)
public/sw.js (1)
1-2: LGTM!package.json (1)
24-25: LGTM!Also applies to: 101-101
middleware.ts (2)
5-12: LGTM!
1-3: 🔒 Security & PrivacyNo auth regression here — private routes already guard themselves.
app/search/[id]/page.tsxredirects unauthenticated users, and the chat API routes return401whengetCurrentUserIdOnServer()is unset.clerkMiddleware()isn’t the only access check, so this change doesn’t leave the protected paths open.> Likely an incorrect or invalid review comment.lib/db/schema.ts (1)
28-28: LGTM!lib/auth/get-current-user.ts (1)
16-22: LGTM!Also applies to: 92-97
app/api/clerk/webhook/route.ts (1)
8-56: LGTM!lib/actions/sync-user.ts (1)
1-15: LGTM!app/layout.tsx (1)
1-2: LGTM!Also applies to: 80-80, 126-126, 146-146
components/header.tsx (1)
30-30: LGTM!lib/supabase/client.ts (1)
1-7: 🩺 Stability & AvailabilityMock Supabase auth still returns null — make sure no server gating depends on it. Any remaining
createClient().auth.getUser()/getSession()check would treat signed-in users as unauthenticated; migrate those callers to Clerk or remove the path.lib/supabase/browser-client.ts (1)
7-7: 🩺 Stability & AvailabilityCheck whether any browser consumer still relies on
onAuthStateChangeto deliver an initial auth state.lib/auth/use-current-user.ts (2)
8-18: 🗄️ Data Integrity & IntegrationConfirm the
user.idcontract.user.idnow exposes Clerk’s identifier; ensure no consumer still expects the old internal DB UUID.
11-11: 🎯 Functional CorrectnessUse the primary email address here.
emailAddresses[0]may not be the user’s primary address;primaryEmailAddress?.emailAddressis the stable field.
| const email = email_addresses[0]?.email_address | ||
|
|
||
| if (id) { | ||
| // Upsert user | ||
| const [existingUser] = await db.select() | ||
| .from(users) | ||
| .where(eq(users.clerkUserId, id)) | ||
| .limit(1); | ||
|
|
||
| if (existingUser) { | ||
| await db.update(users) | ||
| .set({ email }) | ||
| .where(eq(users.clerkUserId, id)); | ||
| } else { | ||
| // Check by email first to link | ||
| const [existingEmailUser] = await db.select() | ||
| .from(users) | ||
| .where(eq(users.email, email)) | ||
| .limit(1); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against a missing email.
email_addresses[0]?.email_address can be undefined (e.g. phone-only or SSO sign-ups). That flows into the email lookup at Line 76 as eq(users.email, undefined), producing an incorrect query, and into the .set({ email }) update at Line 70. Skip email-based linking / updates when no email is present.
🤖 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 `@app/api/clerk/webhook/route.ts` around lines 59 - 77, The webhook user upsert
in the route handler allows `email_addresses[0]?.email_address` to be undefined,
which then gets used in both `db.update(users).set({ email })` and the
`eq(users.email, email)` lookup. Update the `route.ts` logic around the
`existingUser` and `existingEmailUser` checks to branch on whether `email` is
present, so email-based linking and updates are skipped entirely when there is
no email.
| if (id) { | ||
| // Upsert user | ||
| const [existingUser] = await db.select() | ||
| .from(users) | ||
| .where(eq(users.clerkUserId, id)) | ||
| .limit(1); | ||
|
|
||
| if (existingUser) { | ||
| await db.update(users) | ||
| .set({ email }) | ||
| .where(eq(users.clerkUserId, id)); | ||
| } else { | ||
| // Check by email first to link | ||
| const [existingEmailUser] = await db.select() | ||
| .from(users) | ||
| .where(eq(users.email, email)) | ||
| .limit(1); | ||
|
|
||
| if (existingEmailUser) { | ||
| await db.update(users) | ||
| .set({ clerkUserId: id }) | ||
| .where(eq(users.id, existingEmailUser.id)); | ||
| } else { | ||
| await db.insert(users).values({ | ||
| clerkUserId: id, | ||
| email, | ||
| role: 'viewer' | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File list ==\n'
git ls-files | rg '^(app/api/clerk/webhook/route\.ts|.*syncUser.*|.*resolveClerkUserToDbUser.*|.*getCurrentUserIdOnServer.*|.*users.*)$' || true
printf '\n== Outline: app/api/clerk/webhook/route.ts ==\n'
ast-grep outline app/api/clerk/webhook/route.ts --view expanded || true
printf '\n== Relevant searches ==\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
'resolveClerkUserToDbUser|syncUser|getCurrentUserIdOnServer|onConflictDoNothing|clerkUserId|email_addresses|email_address|try\s*\{' \
app api . || true
printf '\n== route.ts excerpt ==\n'
sed -n '1,220p' app/api/clerk/webhook/route.ts || trueRepository: QueueLab/QCX
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== route.ts (30-110) ==\n'
sed -n '30,110p' app/api/clerk/webhook/route.ts
printf '\n== lib/actions/users.ts ==\n'
sed -n '1,220p' lib/actions/users.ts
printf '\n== sync-user references ==\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'syncUser|resolveClerkUserToDbUser|getCurrentUserIdOnServer' lib app componentsRepository: QueueLab/QCX
Length of output: 11169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== lib/auth/get-current-user.ts ==\n'
sed -n '1,220p' lib/auth/get-current-user.ts
printf '\n== lib/actions/sync-user.ts ==\n'
sed -n '1,200p' lib/actions/sync-user.ts
printf '\n== user schema ==\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'pgTable\\(|users =|email:|clerkUserId:|selectedModel:' lib db appRepository: QueueLab/QCX
Length of output: 3937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== schema files ==\n'
git ls-files | rg '(^|/)(schema|schemas|db/schema)\.(ts|tsx|js|jsx)$|lib/db/schema' || true
printf '\n== schema outline ==\n'
ast-grep outline lib/db/schema.ts --view expanded || true
printf '\n== schema excerpt ==\n'
sed -n '1,260p' lib/db/schema.ts || true
printf '\n== unique/nullability search ==\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' \
'uniqueIndex|unique\(|clerkUserId|email:.*(text|varchar|uuid)|notNull\(\)|nullable' \
lib/db/schema.tsRepository: QueueLab/QCX
Length of output: 12301
Make the webhook write idempotent
This upsert can still race with the Clerk sync path or a duplicate delivery; if the insert loses, the unique clerk_user_id/email constraint will throw and this handler returns 500 because the DB block is uncaught. Only touch the email column when email is defined, and make the create/link step atomic instead of doing check-then-insert.
🤖 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 `@app/api/clerk/webhook/route.ts` around lines 61 - 91, The webhook upsert in
route.ts is still race-prone because it does check-then-insert/link around the
users table. Update the Clerk webhook handler so it only sets email when email
is defined, and make the create/link flow atomic in the existing idempotent path
around the db.select/db.update/db.insert logic on users. Ensure duplicate
deliveries or concurrent syncs cannot surface unique constraint errors from
clerkUserId/email and cause the handler to return 500.
| <div className="mr-2"> | ||
| {isLoaded && !isSignedIn && ( | ||
| <SignInButton mode="modal"> | ||
| <Button variant="ghost" size="icon" data-testid="auth-login-trigger"> | ||
| <Image | ||
| src="/images/logo.svg" | ||
| alt="Logo" | ||
| width={20} | ||
| height={20} | ||
| className="h-5 w-auto" | ||
| /> | ||
| </Button> | ||
| </SignInButton> | ||
| )} | ||
| {isLoaded && isSignedIn && ( | ||
| <UserButton /> | ||
| )} | ||
| {!isLoaded && ( | ||
| <Button variant="ghost" size="icon" disabled> | ||
| <Image | ||
| src="/images/logo.svg" | ||
| alt="Logo" | ||
| width={20} | ||
| height={20} | ||
| className="h-5 w-auto opacity-50" | ||
| /> | ||
| </Button> | ||
| )} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the auth-state UI to avoid duplicating the sign-in/user/placeholder logic.
The desktop (Lines 56-83) and mobile (Lines 116-135) regions repeat the same isLoaded/isSignedIn branching and SignInButton/UserButton markup, which must now be kept in sync manually. Consider a small HeaderAuth component parameterized by the loading placeholder.
Also applies to: 116-135
🤖 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 `@components/header.tsx` around lines 56 - 83, The auth-state rendering in
Header is duplicated across the desktop and mobile sections, so extract the
repeated isLoaded/isSignedIn branching with SignInButton, UserButton, and the
loading placeholder into a shared HeaderAuth component. Reuse that component in
the existing header rendering paths and parameterize the placeholder so the
desktop and mobile variants stay in sync without copy-paste.
| if (mobile) { | ||
| setAlignValue("start") // Right align on mobile too | ||
| setAlignValue("start") | ||
| } else { | ||
| setAlignValue("start") // Right align on desktop | ||
| setAlignValue("start") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Redundant branch: both paths set "start".
The if (mobile) … else … sets alignValue to "start" in both branches, so the conditional is dead code and the initial "end" state is never actually used. Either drop the conditional or set the intended distinct value per branch.
Proposed change
const mobile = window.innerWidth < 768
setIsMobile(mobile)
- if (mobile) {
- setAlignValue("start")
- } else {
- setAlignValue("start")
- }
+ setAlignValue("start")📝 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.
| if (mobile) { | |
| setAlignValue("start") // Right align on mobile too | |
| setAlignValue("start") | |
| } else { | |
| setAlignValue("start") // Right align on desktop | |
| setAlignValue("start") | |
| } | |
| const mobile = window.innerWidth < 768 | |
| setIsMobile(mobile) | |
| setAlignValue("start") |
🤖 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 `@components/profile-toggle.tsx` around lines 24 - 28, The conditional in the
ProfileToggle alignment logic is redundant because both the mobile and
non-mobile branches in the effect/handler set alignValue to "start", so the
initial "end" state is never used. Update the logic around the alignValue setter
in ProfileToggle to either remove the dead if/else entirely and set the value
directly, or assign the intended distinct value for each branch so the state
change actually depends on mobile.
| const handleSignOut = () => { | ||
| signOut(() => { | ||
| window.location.href = "/" | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Clerk useClerk signOut redirectUrl vs callback signature
💡 Result:
In the Clerk authentication library, redirectUrl is the current, standard approach for handling redirection after a user signs out [1][2]. It replaced the older signOutCallback signature, which had previously allowed developers to pass a function to execute after the sign-out process [1]. Key differences and context: 1. redirectUrl (Current Standard): This is a property within the SignOutOptions object passed to the signOut function [3][4][5]. It specifies the destination path or URL to which the user should be redirected automatically after a successful sign-out [6][3][7]. This approach is consistent across Clerk's UI components (like SignOutButton) and the programmatic signOut method [2]. 2. signOutCallback (Deprecated/Legacy): Previously, some Clerk components and methods accepted a callback function (e.g., => { window.location.href = '/path' }) to handle post-sign-out logic [1]. This has been largely deprecated in favor of the declarative redirectUrl option to improve API consistency and simplify usage [1][2]. If you are currently implementing or migrating a sign-out flow, you should use the redirectUrl property within the options object [6][3]: // Programmatic sign-out await signOut({ redirectUrl: '/home' }); // Using SignOutButton component Sign Out For multi-session applications, you can also include a sessionId within the same options object to sign out of a specific session while still utilizing the redirectUrl for navigation [6][3][5].
Citations:
- 1: feat(clerk-react): Replace
signOutCallbackwithredirectUrlin SignOutButton clerk/javascript#2348 - 2: feat(clerk-js,types): Support
redirectUrlparam inClerk.signtOut()andClerk#afterSignOutUrlproperty clerk/javascript#2412 - 3: https://clerk.com/docs/nextjs/reference/objects/clerk
- 4: https://clerk.com/docs/vue/reference/objects/clerk
- 5: https://clerk.com/docs/nuxt/reference/objects/clerk
- 6: https://clerk.com/docs/react/reference/components/unstyled/sign-out-button
- 7: https://clerk.com/docs/guides/development/testing/playwright/test-helpers
🏁 Script executed:
set -euo pipefail
printf '\n== package references ==\n'
rg -n '"`@clerk/nextjs`"|`@clerk/nextjs`' package.json pnpm-lock.yaml package-lock.json yarn.lock **/package.json 2>/dev/null || true
printf '\n== profile-toggle.tsx context ==\n'
if [ -f components/profile-toggle.tsx ]; then
cat -n components/profile-toggle.tsx | sed -n '1,120p'
fi
printf '\n== repo signOut usages ==\n'
rg -n 'signOut\(' . --glob '!node_modules' --glob '!dist' --glob '!build' --glob '!coverage' || trueRepository: QueueLab/QCX
Length of output: 1947
🌐 Web query:
Clerk 7.5.13 signOut callback redirectUrl options signature
💡 Result:
In Clerk version 7.5.13, the signOut method and associated components like SignOutButton use a redirectUrl option to control navigation after a successful sign-out [1][2]. The signOut method accepts an optional object with the following signature [1][2]: signOut(options?: { redirectUrl?: string; sessionId?: string }): Promise Parameters: - redirectUrl (string, optional): The full URL or path to navigate to after the sign-out process is complete [3][1][2]. - sessionId (string, optional): The ID of a specific session to sign out of, which is particularly useful for multi-session applications [3][1][2]. For UI components like SignOutButton, the redirectUrl prop is used directly to achieve the same result [3][4]. Note that older versions of Clerk components previously used a signOutCallback prop, which has been replaced by redirectUrl to align with Clerk's standard API surface [5]. If you are seeing a redirect_url query parameter appended to your URL after signing out, this is often part of Clerk's internal redirect handling for protected routes and post-authentication flows [6].
Citations:
- 1: https://clerk.com/docs/nextjs/reference/objects/clerk
- 2: https://clerk.com/docs/nuxt/reference/objects/clerk
- 3: https://clerk.com/docs/react/reference/components/unstyled/sign-out-button
- 4: https://clerk.com/docs/react-router/reference/components/unstyled/sign-out-button
- 5: feat(clerk-react): Replace
signOutCallbackwithredirectUrlin SignOutButton clerk/javascript#2348 - 6: redirect_url is appended after signing out clerk/javascript#5590
Use Clerk’s redirectUrl option here signOut in @clerk/nextjs 7.5.13 takes an options object, so switch to signOut({ redirectUrl: "/" }) and let Clerk handle the redirect instead of setting window.location.href.
🤖 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 `@components/profile-toggle.tsx` around lines 49 - 53, The sign-out flow in
handleSignOut is doing a manual browser redirect instead of using Clerk’s
built-in redirect support. Update the signOut call in the ProfileToggle
component to pass the redirectUrl option directly and remove the
window.location.href callback so Clerk handles navigation after sign-out.
| {isLoaded && !isSignedIn && ( | ||
| <SignInButton mode="modal"> | ||
| <DropdownMenuItem> | ||
| <LogOut className="mr-2 h-4 w-4" /> | ||
| <span>Sign in</span> | ||
| </DropdownMenuItem> | ||
| </SignInButton> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Clerk SignInButton mode modal wrapping custom child asChild onClick
💡 Result:
To use the Clerk SignInButton component with a custom child, the mode set to modal, and an onClick event, you should wrap your custom element in the SignInButton component and configure it as follows [1][2]. When you provide a custom child (such as a custom or
Citations:
- 1: https://clerk.com/docs/react/reference/components/unstyled/sign-in-button
- 2: https://clerk.com/docs/astro/reference/components/unstyled/sign-in-button
- 3: https://clerk.com/docs/expo/reference/components/unstyled/sign-in-button
- 4: https://clerk.com/docs/react-router/reference/components/unstyled/sign-in-button
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== profile-toggle.tsx ==\n'
sed -n '1,180p' components/profile-toggle.tsx
printf '\n== SignInButton usage ==\n'
rg -n "SignInButton|DropdownMenuItem|LogOut|LogIn" components src . -g '!**/node_modules/**'Repository: QueueLab/QCX
Length of output: 1947
🌐 Web query:
Radix DropdownMenuItem onSelect closes menu before child onClick SignInButton modal
💡 Result:
In Radix UI, the DropdownMenuItem component is designed to close the menu automatically upon selection [1]. When you nest an interactive element (like a SignInButton triggering a modal) inside the item, the Radix menu's built-in closing logic often triggers before or during your child component's interaction, causing conflicts such as immediate closure, loss of focus, or pointer event issues [2][3][4][5]. To resolve this, you generally have two primary approaches: 1. Recommended: Use a Controlled Dialog Pattern (Avoid Nested Overlays) Instead of placing the modal trigger directly inside the menu, keep your modal's state at a parent level [6]. - Use the DropdownMenuItem's onSelect event to update a state variable that controls the modal's open state [6]. - Do not call event.preventDefault in this case; let the menu close normally, and then open the modal based on your state [6]. This ensures proper focus management and avoids conflict between the two portals [6][5]. 2. Alternative: Prevent Menu Closure (for specific use cases) If you must keep the menu open (e.g., for multi-step interactions or specific custom behavior), you can prevent the default closing behavior [7][8]. - Inside your DropdownMenuItem, use onSelect={(e) => e.preventDefault} [7][8]. - This keeps the menu open, allowing your child button's onClick event to fire independently [7]. - Note: This approach requires you to manually manage the menu's closing logic later, as it will no longer close automatically upon selection [7][5]. Common pitfalls to avoid: - Focus Traps/Modal Mode: By default, Radix DropdownMenu uses modal behavior, which can interfere with other overlays [4]. If you experience issues with pointer events or immediate closure, setting modal={false} on the DropdownMenu.Root can sometimes resolve these conflicts, though the state-controlled pattern is generally more robust [4][5]. - Event Propagation: If your child's click event is not firing, ensure that you are not accidentally stopping propagation in a way that interferes with Radix's internal event handling [4][9]. Conversely, if the menu is closing due to bubbling, stopPropagation on the child's onClick can sometimes help [9].
Citations:
- 1: https://www.radix-ui.com/primitives/docs/components/dropdown-menu
- 2: [Dialog][DropdownMenu] Cannot compose
DropdownMenu.ItemandDialog.Triggerin latest radix-ui/primitives#634 - 3: [Dropdown] Dropdown menu in modal immediately closes radix-ui/primitives#2215
- 4: OnClick is not trigger when using asChild props with DropdownMenu.Trigger radix-ui/primitives#2267
- 5: [react-dropdown-menu] Dropdown which opens a dialog not removing the pointer-events:none style radix-ui/primitives#837
- 6: Scalable pattern for opening Dialog from DropdownMenu actions radix-ui/primitives#3924
- 7: How suppress DropdownMenu closes when click on CheckboxItem radix-ui/primitives#2414
- 8: https://stackoverflow.com/questions/77185827/shadcn-dialog-inside-of-dropdown-closes-automatically
- 9: Ability to stop click propagation when items are clicked in a dropdown menu radix-ui/primitives#1242
🌐 Web query:
Clerk SignInButton asChild custom child modal docs onClick
💡 Result:
To use a custom child element with the Clerk SignInButton component while maintaining control over the click behavior and triggering a modal, you should use the asChild prop [1][2][3]. The asChild prop is the recommended approach for customizing unstyled components like SignInButton [3]. When set to true, it instructs the component to render its children as the underlying element rather than creating a new container (like an extra button or div), effectively merging the Clerk functionality (like the onClick handler) onto your custom element [2][3]. To use it with a modal: 1. Wrap your custom trigger (e.g., a button or a custom styled element) inside the SignInButton component. 2. Set the mode prop to 'modal' [1][4][5]. 3. Add the asChild prop to the SignInButton component [2]. Example:
Sign In Key points: - Mode: Setting mode="modal" ensures the sign-in flow opens as a modal instead of redirecting the user to a separate page [1][2][5]. - Children: The component requires a single child; if you need to render multiple elements, ensure they are wrapped in a single parent element (such as a div or a fragment) [1][2][4]. - Deprecation: The older 'as' prop has been deprecated and replaced by 'asChild' [3]. Using asChild allows for better composition and accessibility by ensuring the Clerk logic is applied directly to your intended UI element [3].Citations:
- 1: https://clerk.com/docs/react/reference/components/unstyled/sign-in-button
- 2: https://clerk.com/docs/astro/reference/components/unstyled/sign-in-button
- 3: chore(astro): Remove deprecated
asprop from button components clerk/javascript#5140 - 4: https://clerk.com/docs/expo/reference/components/unstyled/sign-in-button
- 5: https://clerk.com/docs/vue/reference/components/unstyled/sign-in-button
Move the sign-in trigger out of the dropdown item and swap the icon. DropdownMenuItem closes on selection, so this nested Clerk modal trigger can interfere with opening the sign-in flow; use asChild/onSelect handling instead. Also replace LogOut with LogIn on the sign-in entry.
🤖 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 `@components/profile-toggle.tsx` around lines 96 - 102, The sign-in entry in
profile-toggle currently nests SignInButton inside DropdownMenuItem, which can
conflict with the menu closing behavior; update the dropdown action to trigger
Clerk via asChild or explicit onSelect handling instead of nesting the modal
trigger. Also change the icon used for the sign-in action from LogOut to LogIn
so the UI matches the action, and keep the implementation localized around the
SignInButton and DropdownMenuItem render path.
| useEffect(() => { | ||
| if (isLoaded && isSignedIn) { | ||
| syncUser(); | ||
| } | ||
| }, [isLoaded, isSignedIn]); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle the syncUser() rejection.
The server action result is neither awaited nor caught; a failed sync surfaces as an unhandled promise rejection. Add a .catch to log and swallow it.
♻️ Suggested change
- if (isLoaded && isSignedIn) {
- syncUser();
- }
+ if (isLoaded && isSignedIn) {
+ syncUser().catch((err) => console.error('[UserSync] sync failed:', err));
+ }📝 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.
| useEffect(() => { | |
| if (isLoaded && isSignedIn) { | |
| syncUser(); | |
| } | |
| }, [isLoaded, isSignedIn]); | |
| useEffect(() => { | |
| if (isLoaded && isSignedIn) { | |
| syncUser().catch((err) => console.error('[UserSync] sync failed:', err)); | |
| } | |
| }, [isLoaded, isSignedIn]); |
🤖 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 `@components/user-sync.tsx` around lines 13 - 17, The sync trigger in the
UserSync effect calls syncUser() without handling its promise, so a failure can
become an unhandled rejection. Update the useEffect in UserSync to attach a
.catch to the syncUser() call, and in that handler log the error and swallow it
so the effect does not leak a rejection.
| try { | ||
| // 1. Try to find the user by clerkUserId | ||
| const [existingUser] = await db.select({ id: users.id }) | ||
| .from(users) | ||
| .where(eq(users.clerkUserId, clerkUserId)) | ||
| .limit(1); | ||
|
|
||
| if (existingUser) { | ||
| return existingUser.id; | ||
| } | ||
|
|
||
| const { | ||
| data: { session }, | ||
| error, | ||
| } = await supabase.auth.getSession(); | ||
| // 2. If not found, fetch user details from Clerk and create a new record | ||
| const clerkUser = await currentUser(); | ||
| if (!clerkUser) return null; | ||
|
|
||
| if (error) { | ||
| console.error('[Auth] Error getting Supabase session on server:', error.message); | ||
| return { user: null, session: null, error }; | ||
| } | ||
| const email = clerkUser.emailAddresses[0]?.emailAddress; | ||
|
|
||
| if (!session) { | ||
| // console.log('[Auth] No active Supabase session found.'); | ||
| return { user: null, session: null, error: null }; | ||
| } | ||
| // 2.1 Check if user with this email already exists (maybe from Supabase auth) | ||
| if (email) { | ||
| const [existingEmailUser] = await db.select({ id: users.id, clerkUserId: users.clerkUserId }) | ||
| .from(users) | ||
| .where(eq(users.email, email)) | ||
| .limit(1); | ||
|
|
||
| return { user: session.user, session, error: null }; | ||
| if (existingEmailUser) { | ||
| // Link the existing user to this Clerk ID if not already linked | ||
| if (!existingEmailUser.clerkUserId) { | ||
| await db.update(users) | ||
| .set({ clerkUserId }) | ||
| .where(eq(users.id, existingEmailUser.id)); | ||
| } | ||
| return existingEmailUser.id; | ||
| } | ||
| } | ||
|
|
||
| // 2.2 Create new user if no match found | ||
| const [newUser] = await db.insert(users).values({ | ||
| clerkUserId, | ||
| email, | ||
| role: 'viewer', | ||
| }).returning({ id: users.id }); | ||
|
|
||
| return newUser.id; | ||
| } catch (error) { | ||
| console.error('[Auth] Error resolving Clerk user to DB user:', error); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and related auth/user code
git ls-files | rg '^(lib/auth/get-current-user\.ts|.*users.*|.*clerk.*|.*syncUser.*|.*UserSync.*)$'
# Show an outline of the target file and likely related schema files
for f in lib/auth/get-current-user.ts; do
if [ -f "$f" ]; then
echo "=== OUTLINE: $f ==="
ast-grep outline "$f" --view expanded || true
fi
done
# Search for the users schema and any unique constraints / conflict handling
rg -n "clerkUserId|unique\(|onConflict|syncUser|UserSync|currentUser\(" lib . --glob '!**/node_modules/**'Repository: QueueLab/QCX
Length of output: 4846
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the target file in full if it is reasonably small.
if [ -f lib/auth/get-current-user.ts ]; then
wc -l lib/auth/get-current-user.ts
cat -n lib/auth/get-current-user.ts
fiRepository: QueueLab/QCX
Length of output: 4008
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect schema definitions for users table and email/clerkUserId uniqueness.
for f in $(git ls-files | rg 'schema|db|users'); do
if rg -q "export .*users|pgTable\\(.*users|sqliteTable\\(.*users|mysqlTable\\(.*users" "$f"; then
echo "=== FILE: $f ==="
ast-grep outline "$f" --view expanded || true
fi
doneRepository: QueueLab/QCX
Length of output: 3400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the concurrent call paths and the users schema constraints.
sed -n '1,120p' lib/actions/sync-user.ts
printf '\n=== schema excerpt ===\n'
sed -n '20,35p' lib/db/schema.ts
printf '\n=== clerk webhook excerpt ===\n'
sed -n '50,95p' app/api/clerk/webhook/route.tsRepository: QueueLab/QCX
Length of output: 2641
Make the create path idempotent in lib/auth/get-current-user.ts:37-83
resolveClerkUserToDbUser is reachable from both getCurrentUserIdOnServer and syncUser, so two first-time requests can race past the lookups and hit the insert together. With unique email and clerkUserId constraints, one insert fails, the catch returns null, and the user is treated as unauthenticated for that request. Use an upsert/re-read path instead of swallowing the conflict.
🤖 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 `@lib/auth/get-current-user.ts` around lines 37 - 83, The create path in
resolveClerkUserToDbUser is not idempotent, so concurrent first-time calls can
race into the users insert and fail on unique email or clerkUserId constraints.
Update the creation flow in get-current-user.ts to use an upsert or
conflict-aware retry/re-read strategy after the Clerk lookup and existing-user
checks, so one request can safely create or link the user while the other
resolves the already-created record instead of returning null from the catch
block. Keep the existing lookup/link logic around currentUser,
existingEmailUser, and the final insert, but make the insert path conflict-safe
and re-fetch the user id on conflict.
I've updated the implementation as requested:
All changes have been verified and the build is passing. |
- Revert Header logo button to handle only history toggle - Move Clerk authentication triggers to ProfileToggle - Display user avatar in ProfileToggle when authenticated - Remove 'Appearance' option from profile menu - Ensure build succeeds with updated auth logic Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
This PR migrates the authentication system from Supabase to Clerk.
Key features:
PR created automatically by Jules for task 15642548437035973196 started by @ngoiyaeric
Summary by CodeRabbit
New Features
Bug Fixes