Separate Auth Page with Discord Redirect and Google-only Sign-In#742
Conversation
…ct and linking routes - Created custom `/sign-in` page showing only Google SSO. - Implemented `/discord-auth`, `/sign-in/discord`, and `/auth/discord` routes that redirect directly to Discord SSO. - Added custom "Account" tab in Settings to link/unlink Discord account programmatically using Clerk user API. - Integrated redirects to `/sign-in` into `profile-toggle` and `chat-history-client`. - Added Playwright E2E testing safeguards for background loading screens. 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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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? |
|
ngoiyaeric seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
Warning Review limit reached
Next review available in: 27 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 (2)
WalkthroughAdds Clerk-based Google and Discord authentication routes, Discord account linking in Settings, custom Clerk environment configuration, auth-route animation suppression, resilient test setup, and refreshed service-worker precache metadata. ChangesClerk authentication flow
Test and build support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Visitor
participant SignInComponent
participant Clerk
participant SSOCallbackPage
Visitor->>SignInComponent: select Google or Discord
SignInComponent->>Clerk: start OAuth SSO
Clerk->>SSOCallbackPage: redirect to callback URL
SSOCallbackPage->>Clerk: authenticate callback
Clerk-->>Visitor: redirect to requested destination
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
| return ( | ||
| <div className="flex items-center gap-4 p-4 border rounded-xl bg-accent/20 border-primary/50"> | ||
| <div className="h-12 w-12 rounded-full overflow-hidden bg-muted flex items-center justify-center"> | ||
| {discordAccount.avatarUrl ? ( |
There was a problem hiding this comment.
[blocking] exposes and , not or . With this repository's strict TypeScript settings, this new Account tab should fail type-check/build before deployment. Use the supported Clerk fields (or otherwise update the type/API contract) here and on the ID line below.
| try { | ||
| const res = await clerkUser?.createExternalAccount({ | ||
| strategy: 'oauth_discord', | ||
| redirectUrl: window.location.href, |
There was a problem hiding this comment.
[blocking] expects to be an OAuth callback route that invokes /. Returning to sends Discord back to the Settings page, which does not process the callback, so the verification will not complete and the external account will not be linked. Redirect through and preserve the return location separately.
| const [isLoading, setIsLoading] = useState(false) | ||
|
|
||
| // Get redirect url from search params, default to '/' | ||
| const redirectUrl = searchParams?.get('redirect_url') || '/' |
There was a problem hiding this comment.
[blocking/security] is attacker-controlled and is passed directly to both and Clerk's . A link such as can send an already signed-in user immediately, or a newly authenticated user after OAuth, to an external site. Accept only same-origin relative paths (or an explicit allowlist) before using this value.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/auth/discord/page.tsx (1)
1-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicate route handlers.
The client-side Discord SSO logic is duplicated identically across multiple alias routes. To avoid maintaining identical code in multiple places and to reduce the client-side bundle load, use Next.js server-side redirects for the aliases to point to the primary route.
app/auth/discord/page.tsx#L1-L50: Replace this duplicated client component with a server-side redirect to the primary route (e.g.,import { redirect } from "next/navigation";andredirect('/sign-in/discord')).app/discord-auth/page.tsx#L1-L50: Replace this duplicated client component with a server-side redirect to the primary route.🤖 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/auth/discord/page.tsx` around lines 1 - 50, Replace the duplicated Discord SSO client logic in app/auth/discord/page.tsx (lines 1-50) with a server-side redirect to /sign-in/discord, removing the client component, hooks, and Suspense wrapper. Apply the same replacement in app/discord-auth/page.tsx (lines 1-50), leaving the primary route’s DiscordAuthRedirect implementation unchanged.
🤖 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/sign-in/sign-in-component.tsx`:
- Around line 19-26: Sanitize the `redirectUrl` value used by the `useEffect`
redirect before passing it to `router.push`: allow only same-origin relative
paths, and fall back to `/` for absolute or external URLs. Keep the existing
signed-in and loaded checks, and ensure the effect depends on the normalized
redirect value.
In `@components/conditional-lottie.tsx`:
- Line 14: Initialize the isPlaywright state in ConditionalLottie from the
build-time environment variable instead of false, so the component returns null
on the first render during Playwright tests and prevents the LottiePlayer flash.
Preserve the existing useEffect synchronization for subsequent environment
changes.
In `@components/profile-toggle.tsx`:
- Around line 80-83: Replace the full-page sign-in navigation with Next.js
client-side routing: in components/profile-toggle.tsx lines 80-83, import
useRouter from next/navigation, initialize router in the component, and use
router.push("/sign-in") in the DropdownMenuItem handler; in
components/sidebar/chat-history-client.tsx lines 117-119, use the already
initialized router.push("/sign-in") instead.
- Around line 80-83: Update the “Sign in” DropdownMenuItem in the profile toggle
to use the LogIn icon instead of LogOut, and add the corresponding lucide-react
import if needed. Leave the sign-in navigation and menu text unchanged.
In `@components/settings/components/settings.tsx`:
- Around line 265-321: Update the Connect and Disconnect button handlers in the
settings component to share or use appropriate in-flight state, disabling the
clicked button while destroy/reload or createExternalAccount is pending. Set the
state before each async request and clear it in a finally path so buttons
re-enable after both success and failure, preventing duplicate Clerk calls and
preserving existing toast behavior.
- Around line 299-307: Update the createExternalAccount call in the Discord
linking onClick handler to use the /sso-callback route as redirectUrl instead of
window.location.href. Preserve the existing OAuth strategy and
externalVerificationRedirectURL handling, matching the established Clerk OAuth
flow.
- Around line 249-264: In the Discord account rendering block, hoist the
external-account lookup into a single variable and reuse it for the conditional
and displayed account data. Update the avatar source in the Discord account
markup to use discordAccount.imageUrl so the connected account image renders
correctly.
In `@tests/header.spec.ts`:
- Around line 6-12: Replace the try/catch blocks around the “Later” button in
tests/header.spec.ts lines 6-12 and tests/map.spec.ts lines 6-12 with
page.addLocatorHandler, configuring it to dismiss the visible “Later” overlay
asynchronously without a 5-second waitFor timeout.
---
Outside diff comments:
In `@app/auth/discord/page.tsx`:
- Around line 1-50: Replace the duplicated Discord SSO client logic in
app/auth/discord/page.tsx (lines 1-50) with a server-side redirect to
/sign-in/discord, removing the client component, hooks, and Suspense wrapper.
Apply the same replacement in app/discord-auth/page.tsx (lines 1-50), leaving
the primary route’s DiscordAuthRedirect implementation unchanged.
🪄 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: 590bb703-8882-48ae-bb32-1bc34775da12
📒 Files selected for processing (15)
.env.env.local.example.gitignoreapp/auth/discord/page.tsxapp/discord-auth/page.tsxapp/sign-in/discord/page.tsxapp/sign-in/page.tsxapp/sign-in/sign-in-component.tsxapp/sso-callback/page.tsxcomponents/conditional-lottie.tsxcomponents/profile-toggle.tsxcomponents/settings/components/settings.tsxcomponents/sidebar/chat-history-client.tsxtests/header.spec.tstests/map.spec.ts
📜 Review details
🔇 Additional comments (10)
.env (1)
4-6: LGTM!.env.local.example (1)
35-38: LGTM!.gitignore (1)
62-64: LGTM!app/sign-in/page.tsx (1)
1-17: LGTM!app/sign-in/sign-in-component.tsx (2)
28-34: LGTM!
36-96: LGTM!components/settings/components/settings.tsx (2)
28-28: LGTM!Also applies to: 77-77
215-220: LGTM!app/sign-in/discord/page.tsx (1)
1-49: LGTM!app/sso-callback/page.tsx (1)
1-17: LGTM!
| // Get redirect url from search params, default to '/' | ||
| const redirectUrl = searchParams?.get('redirect_url') || '/' | ||
|
|
||
| useEffect(() => { | ||
| if (isUserLoaded && isSignedIn) { | ||
| router.push(redirectUrl) | ||
| } | ||
| }, [isUserLoaded, isSignedIn, router, redirectUrl]) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does @clerk/nextjs 7.x validate redirect_url against allowedRedirectOrigins by default when no ClerkProvider config is set?
💡 Result:
Yes, @clerk/nextjs 7.x validates redirect URLs against the allowedRedirectOrigins configuration [1][2]. When no specific allowedRedirectOrigins configuration is provided, the Clerk SDK applies default validation rules [3][4]. By default, the SDK allows the following types of URLs: - Relative URLs [3] - Absolute URLs with an 'https' protocol where the domain matches, or is a subdomain of, the eTLD+1 (the effective Top-Level Domain plus one) of your application [3]. If a redirect URL does not match these default allowed origins or any explicitly configured origins, the redirect is considered unsafe [1][5]. In such cases, the redirect is ignored, a warning is logged in the browser console, and the application falls back to the default redirect behavior [1][3][5].
Citations:
- 1: https://clerk.com/docs/nextjs/reference/components/clerk-provider
- 2: https://clerk.com/docs/nextjs/reference/objects/clerk
- 3: feat(clerk-js,types): Validate query param redirect urls with the allowedRedirectOrigins prop clerk/javascript#1096
- 4: feat(clerk-js): Add default Allowed redirect origins clerk/javascript#2128
- 5: https://clerk.com/docs/js-frontend/reference/objects/clerk
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== app/sign-in/sign-in-component.tsx ==\n'
sed -n '1,120p' app/sign-in/sign-in-component.tsx
printf '\n== ClerkProvider references ==\n'
rg -n "ClerkProvider|allowedRedirectOrigins|allowedRedirectProtocols|redirect_url|redirectUrlComplete" -S . --glob '!node_modules' --glob '!dist' --glob '!build'Repository: QueueLab/QCX
Length of output: 4295
🌐 Web query:
Next.js App Router router.push absolute external URL behavior
💡 Result:
In the Next.js App Router, the router.push function supports navigation to external absolute URLs [1][2]. When you provide a fully qualified URL (e.g., 'https://google.com') to router.push, Next.js handles it by navigating to that external destination [2]. Historically, older versions of Next.js or the Pages Router required using window.location for external navigation, as router.push would interpret absolute paths as internal routes, causing them to be appended to the current origin (e.g., localhost:3000/https://google.com) [3][4]. Modern versions of the App Router have improved this behavior to support absolute URLs natively [1][2]. Key considerations when using router.push for external URLs: 1. Security (Open Redirects): If you allow user-provided input to determine the URL passed to router.push, you may expose your application to open redirect vulnerabilities [5]. Always validate and sanitize external URLs to ensure they point to expected or trusted domains before redirecting [5]. 2. Prefetching: Next.js distinguishes between internal and external URLs [6]. Internal routes are often prefetched, whereas external URLs are identified as external (using origin checking) and are not subject to the same prefetching mechanisms [6]. 3. Alternative Methods: While router.push is supported, the native window.location.href approach remains a reliable and standard way to perform external redirects in client-side code [4]. Additionally, for server-side redirects (e.g., in Server Actions or Route Handlers), the redirect function from next/navigation also natively supports external absolute URLs [7].
Citations:
- 1: [Docs] Adds external redirects section for App Router vercel/next.js#61052
- 2: Add support for navigating to external urls vercel/next.js#45388
- 3: https://nextjs.org/docs/pages/api-reference/functions/use-router
- 4: https://stackoverflow.com/questions/73261368/next-js-redirec-to-external-url-on-click-of-button-and-not-on-loading-a-route
- 5: https://vibeappscanner.com/vulnerability-in/open-redirect-nextjs
- 6: https://github.com/vercel/next.js/blob/v15.5.15/packages/next/src/client/components/app-router.tsx
- 7: https://nextjs.org/docs/app/guides/redirecting
Sanitize redirect_url before calling router.push.
Clerk already protects redirectUrlComplete, but the effect still trusts the query param and can send signed-in users to an external URL. Normalize it to a same-origin relative path (or fall back to /) before redirecting.
🤖 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/sign-in/sign-in-component.tsx` around lines 19 - 26, Sanitize the
`redirectUrl` value used by the `useEffect` redirect before passing it to
`router.push`: allow only same-origin relative paths, and fall back to `/` for
absolute or external URLs. Keep the existing signed-in and loaded checks, and
ensure the effect depends on the normalized redirect value.
| const { isMapLoaded } = useMapLoading(); | ||
| const { activeView } = useProfileToggle(); // Added this line | ||
| const { isUsageOpen } = useUsageToggle(); | ||
| const [isPlaywright, setIsPlaywright] = useState(false); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Initialize state from the environment variable to prevent Lottie flashes.
By initializing the state to false, the LottiePlayer will still render during the initial client frame before the useEffect hides it. This flash can still cause flakiness or steal focus in visual E2E tests.
Initialize the state directly with the build-time environment variable so it renders null from the very first frame when testing.
💡 Proposed fix
- const [isPlaywright, setIsPlaywright] = useState(false);
+ const [isPlaywright, setIsPlaywright] = useState(
+ () => process.env.NEXT_PUBLIC_PLAYWRIGHT_TEST === 'true'
+ );📝 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.
| const [isPlaywright, setIsPlaywright] = useState(false); | |
| const [isPlaywright, setIsPlaywright] = useState( | |
| () => process.env.NEXT_PUBLIC_PLAYWRIGHT_TEST === 'true' | |
| ); |
🤖 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/conditional-lottie.tsx` at line 14, Initialize the isPlaywright
state in ConditionalLottie from the build-time environment variable instead of
false, so the component returns null on the first render during Playwright tests
and prevents the LottiePlayer flash. Preserve the existing useEffect
synchronization for subsequent environment changes.
| <DropdownMenuItem onClick={() => window.location.href = "/sign-in"}> | ||
| <LogOut className="mr-2 h-4 w-4" /> | ||
| <span>Sign in</span> | ||
| </DropdownMenuItem> |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Use client-side navigation instead of full page reloads.
Using window.location.href bypasses Next.js's router, forcing a full page reload which drops client-side state and degrades performance. Use Next.js client-side routing (router.push()) to navigate to the sign-in page smoothly.
components/profile-toggle.tsx#L80-L83: Update theonClickhandler to userouter.push("/sign-in"). (You will need to ensureconst router = useRouter()fromnext/navigationis added to the component).components/sidebar/chat-history-client.tsx#L117-L119: Update theonClickhandler to use the already initializedrouter.push("/sign-in").
📍 Affects 2 files
components/profile-toggle.tsx#L80-L83(this comment)components/sidebar/chat-history-client.tsx#L117-L119
🤖 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 80 - 83, Replace the full-page
sign-in navigation with Next.js client-side routing: in
components/profile-toggle.tsx lines 80-83, import useRouter from
next/navigation, initialize router in the component, and use
router.push("/sign-in") in the DropdownMenuItem handler; in
components/sidebar/chat-history-client.tsx lines 117-119, use the already
initialized router.push("/sign-in") instead.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use a LogIn icon for the "Sign in" action.
The "Sign in" menu item currently uses the LogOut icon, which is semantically misleading. Consider importing and using the LogIn icon from lucide-react instead for better visual clarity.
💡 Proposed change
- <DropdownMenuItem onClick={() => window.location.href = "/sign-in"}>
- <LogOut className="mr-2 h-4 w-4" />
- <span>Sign in</span>
- </DropdownMenuItem>
+ <DropdownMenuItem onClick={() => window.location.href = "/sign-in"}>
+ <LogIn className="mr-2 h-4 w-4" />
+ <span>Sign in</span>
+ </DropdownMenuItem>📝 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.
| <DropdownMenuItem onClick={() => window.location.href = "/sign-in"}> | |
| <LogOut className="mr-2 h-4 w-4" /> | |
| <span>Sign in</span> | |
| </DropdownMenuItem> | |
| <DropdownMenuItem onClick={() => window.location.href = "/sign-in"}> | |
| <LogIn className="mr-2 h-4 w-4" /> | |
| <span>Sign in</span> | |
| </DropdownMenuItem> |
🤖 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 80 - 83, Update the “Sign in”
DropdownMenuItem in the profile toggle to use the LogIn icon instead of LogOut,
and add the corresponding lucide-react import if needed. Leave the sign-in
navigation and menu text unchanged.
| {clerkUser?.externalAccounts.find(acc => acc.provider === 'discord') ? ( | ||
| (() => { | ||
| const discordAccount = clerkUser.externalAccounts.find(acc => acc.provider === 'discord')!; | ||
| return ( | ||
| <div className="flex items-center gap-4 p-4 border rounded-xl bg-accent/20 border-primary/50"> | ||
| <div className="h-12 w-12 rounded-full overflow-hidden bg-muted flex items-center justify-center"> | ||
| {discordAccount.avatarUrl ? ( | ||
| <img src={discordAccount.avatarUrl} alt="Discord Avatar" className="h-full w-full object-cover" /> | ||
| ) : ( | ||
| <span className="text-xl font-bold text-primary">D</span> | ||
| )} | ||
| </div> | ||
| <div className="flex-1"> | ||
| <p className="font-semibold text-foreground">Connected as {discordAccount.username || 'Discord User'}</p> | ||
| <p className="text-xs text-muted-foreground">ID: {discordAccount.externalId}</p> | ||
| </div> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
In @clerk/nextjs 7.5.13, what is the current property name on ExternalAccount for the provider avatar image?
💡 Result:
In @clerk/nextjs 7.5.13, the property name on the ExternalAccount object for the provider avatar image is imageUrl [1][2].
Citations:
- 1: https://clerk.com/docs/reference/backend/types/backend-external-account
- 2: https://clerk.com/docs/vue/reference/types/external-account
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section and any nearby handling of Discord account fields.
sed -n '230,290p' components/settings/components/settings.tsx
printf '\n--- searches ---\n'
rg -n "avatarUrl|imageUrl|externalAccounts\.find\(acc => acc\.provider === 'discord'\)" components/settings/components/settings.tsx . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: QueueLab/QCX
Length of output: 6074
Use imageUrl for the Discord avatar, and reuse the lookup
discordAccount.avatarUrl should be discordAccount.imageUrl; otherwise the avatar never renders and always falls back to the placeholder. Also hoist the discord account lookup into one variable instead of calling .find() twice.
🤖 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/settings/components/settings.tsx` around lines 249 - 264, In the
Discord account rendering block, hoist the external-account lookup into a single
variable and reuse it for the conditional and displayed account data. Update the
avatar source in the Discord account markup to use discordAccount.imageUrl so
the connected account image renders correctly.
| <Button | ||
| type="button" | ||
| variant="destructive" | ||
| size="sm" | ||
| onClick={async () => { | ||
| try { | ||
| await discordAccount.destroy(); | ||
| await clerkUser.reload(); | ||
| toast({ | ||
| title: "Discord Unlinked", | ||
| description: "Your Discord account has been disconnected.", | ||
| }); | ||
| } catch (err: any) { | ||
| toast({ | ||
| title: "Error", | ||
| description: err.message || "Failed to disconnect Discord account.", | ||
| variant: "destructive", | ||
| }); | ||
| } | ||
| }} | ||
| > | ||
| Disconnect | ||
| </Button> | ||
| </div> | ||
| ); | ||
| })() | ||
| ) : ( | ||
| <div className="flex flex-col items-center justify-center py-6 text-center space-y-4"> | ||
| <p className="text-sm text-muted-foreground"> | ||
| You haven't connected your Discord account yet. Link it now to connect your SaaS profile. | ||
| </p> | ||
| <Button | ||
| type="button" | ||
| className="bg-[#5865F2] hover:bg-[#4752C4] text-white flex items-center gap-2 px-6 py-5 text-base font-semibold" | ||
| onClick={async () => { | ||
| try { | ||
| const res = await clerkUser?.createExternalAccount({ | ||
| strategy: 'oauth_discord', | ||
| redirectUrl: window.location.href, | ||
| }); | ||
| if (res?.verification?.externalVerificationRedirectURL) { | ||
| window.location.href = res.verification.externalVerificationRedirectURL.href; | ||
| } | ||
| } catch (err: any) { | ||
| toast({ | ||
| title: "Connection Error", | ||
| description: err.message || "Failed to initiate Discord linking.", | ||
| variant: "destructive", | ||
| }); | ||
| } | ||
| }} | ||
| > | ||
| <svg className="h-5 w-5 fill-current" viewBox="0 0 127.14 96.36"> | ||
| <path d="M107.7,8.07A105.15,105.15,0,0,0,77.26,0a77.19,77.19,0,0,0-3.3,6.83A96.67,96.67,0,0,0,52.88,6.83,77.19,77.19,0,0,0,49.58,0,105.15,105.15,0,0,0,19.14,8.07C3,36.79-1.45,64.83.45,92.48a106.4,106.4,0,0,0,32.22,16.22,78,78,0,0,0,6.77-11,68.86,68.86,0,0,1-10.74-5.12c.91-.66,1.8-1.34,2.65-2a75.58,75.58,0,0,0,64,0c.86.69,1.75,1.37,2.65,2a68.86,68.86,0,0,1-10.74,5.12,78,78,0,0,0,6.77,11,106.4,106.4,0,0,0,32.22-16.22C129.21,64.83,124.2,36.79,107.7,8.07ZM42.45,65.69C36.18,65.69,31,60,31,53S36.18,40.36,42.45,40.36,53.83,46,53.83,53,48.72,65.69,42.45,65.69Zm42.24,0C78.41,65.69,73.24,60,73.24,53S78.41,40.36,84.69,40.36,96.07,46,96.07,53,91,65.69,84.69,65.69Z"/> | ||
| </svg> | ||
| <span>Connect Discord Account</span> | ||
| </Button> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Disable Connect/Disconnect while the request is in flight.
Neither button guards against double-clicks during its async call (destroy()+reload(), or createExternalAccount()). A rapid double click can fire the non-idempotent Clerk call twice, typically surfacing a confusing error toast on the second attempt.
🤖 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/settings/components/settings.tsx` around lines 265 - 321, Update
the Connect and Disconnect button handlers in the settings component to share or
use appropriate in-flight state, disabling the clicked button while
destroy/reload or createExternalAccount is pending. Set the state before each
async request and clear it in a finally path so buttons re-enable after both
success and failure, preventing duplicate Clerk calls and preserving existing
toast behavior.
| onClick={async () => { | ||
| try { | ||
| const res = await clerkUser?.createExternalAccount({ | ||
| strategy: 'oauth_discord', | ||
| redirectUrl: window.location.href, | ||
| }); | ||
| if (res?.verification?.externalVerificationRedirectURL) { | ||
| window.location.href = res.verification.externalVerificationRedirectURL.href; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Clerk createExternalAccount custom flow: is handleRedirectCallback required on the redirectUrl page to complete account linking, or does Clerk finalize it automatically?
💡 Result:
In a custom Clerk flow using createExternalAccount, it is required to call handleRedirectCallback (or render the AuthenticateWithRedirectCallback component) on the redirectUrl page to finalize the account linking process [1][2][3]. Clerk does not automatically finalize the account linking upon the user's return from the identity provider [1]. The redirectUrl route acts as a handler where you must explicitly trigger Clerk's callback logic to process the returned OAuth state, verify the connection, and complete the integration [1][4]. If you are using Clerk's SDKs (e.g., in Next.js), you can handle this by: 1. Creating a dedicated route (commonly /sso-callback) that corresponds to the redirectUrl provided to createExternalAccount [1][2]. 2. Rendering the component or calling the Clerk.handleRedirectCallback method on that route [1][4]. Failure to perform this step will result in the external account remaining in an unverified or incomplete state, as the callback is necessary for Clerk to finalize the association between the external identity provider and the user's account [1][3].
Citations:
- 1: https://clerk.com/docs/guides/development/custom-flows/authentication/legacy/oauth-connections
- 2: https://clerk.com/docs/nextjs/reference/types/external-account
- 3: docs: missing linking to external accounts (oauth) in custom flow clerk/clerk-docs#1680
- 4: https://clerk.com/docs/nextjs/reference/components/control/authenticate-with-redirect-callback
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Matching files ==\n'
git ls-files | rg '(^|/)(settings\.tsx|sso-callback|AuthenticateWithRedirectCallback|handleRedirectCallback|clerk|external account|externalAccount|oauth_discord|oauth_google)$' || true
printf '\n== Search for callback handling ==\n'
rg -n "handleRedirectCallback|AuthenticateWithRedirectCallback|createExternalAccount|oauth_discord|oauth_google|sso-callback" .
printf '\n== Relevant file snippets ==\n'
for f in \
components/settings/components/settings.tsx \
components/sign-in-component.tsx \
app/sso-callback/page.tsx \
pages/sso-callback.tsx \
src/sso-callback.tsx
do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
sed -n '1,220p' "$f"
fi
doneRepository: QueueLab/QCX
Length of output: 11666
Route Discord linking through /sso-callback.
createExternalAccount needs a callback route that renders AuthenticateWithRedirectCallback; using window.location.href here sends Discord back to the settings page without completing the OAuth handoff. Mirror the existing Clerk OAuth flow and point this to /sso-callback instead.
🤖 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/settings/components/settings.tsx` around lines 299 - 307, Update
the createExternalAccount call in the Discord linking onClick handler to use the
/sso-callback route as redirectUrl instead of window.location.href. Preserve the
existing OAuth strategy and externalVerificationRedirectURL handling, matching
the established Clerk OAuth flow.
| try { | ||
| await page.locator('text=Later').click({ timeout: 3000 }); | ||
| } catch (e) {} | ||
| const laterBtn = page.getByRole('button', { name: 'Later' }); | ||
| await laterBtn.waitFor({ state: 'visible', timeout: 5000 }); | ||
| await laterBtn.click({ force: true }); | ||
| } catch (e) { | ||
| console.warn('Could not click "Later" button:', e); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Use page.addLocatorHandler to prevent blocking tests for 5 seconds.
If the "Later" modal does not appear, the waitFor command will block test execution for the full 5 seconds before timing out and continuing, which adds significant dead time to the E2E suite on every test. Playwright provides page.addLocatorHandler specifically for dismissing unexpected overlays (like popups or banners) asynchronously without slowing down the test run.
tests/header.spec.ts#L6-L12: Replace thetry/catchblock withpage.addLocatorHandler.tests/map.spec.ts#L6-L12: Replace thetry/catchblock withpage.addLocatorHandler.
⚡ Proposed fix for both files
- try {
- const laterBtn = page.getByRole('button', { name: 'Later' });
- await laterBtn.waitFor({ state: 'visible', timeout: 5000 });
- await laterBtn.click({ force: true });
- } catch (e) {
- console.warn('Could not click "Later" button:', e);
- }
+ await page.addLocatorHandler(page.getByRole('button', { name: 'Later' }), async () => {
+ await page.getByRole('button', { name: 'Later' }).click({ force: true });
+ });📝 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.
| try { | |
| await page.locator('text=Later').click({ timeout: 3000 }); | |
| } catch (e) {} | |
| const laterBtn = page.getByRole('button', { name: 'Later' }); | |
| await laterBtn.waitFor({ state: 'visible', timeout: 5000 }); | |
| await laterBtn.click({ force: true }); | |
| } catch (e) { | |
| console.warn('Could not click "Later" button:', e); | |
| } | |
| await page.addLocatorHandler(page.getByRole('button', { name: 'Later' }), async () => { | |
| await page.getByRole('button', { name: 'Later' }).click({ force: true }); | |
| }); |
📍 Affects 2 files
tests/header.spec.ts#L6-L12(this comment)tests/map.spec.ts#L6-L12
🤖 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 `@tests/header.spec.ts` around lines 6 - 12, Replace the try/catch blocks
around the “Later” button in tests/header.spec.ts lines 6-12 and
tests/map.spec.ts lines 6-12 with page.addLocatorHandler, configuring it to
dismiss the visible “Later” overlay asynchronously without a 5-second waitFor
timeout.
…ct and linking routes - Created custom `/sign-in` page showing only Google SSO. - Implemented `/discord-auth`, `/sign-in/discord`, and `/auth/discord` routes that redirect directly to Discord SSO using modern Clerk v7 APIs. - Added custom "Account" tab in Settings to link/unlink Discord account programmatically using Clerk user API. - Integrated redirects to `/sign-in` into `profile-toggle` and `chat-history-client`. - Added Playwright E2E testing safeguards for background loading screens. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
…ct and linking routes - Created custom `/sign-in` page showing only Google SSO. - Implemented `/discord-auth`, `/sign-in/discord`, and `/auth/discord` routes that redirect directly to Discord SSO using modern Clerk v7 APIs. - Added custom "Account" tab in Settings to link/unlink Discord account programmatically using Clerk user API. - Integrated redirects to `/sign-in` into `profile-toggle` and `chat-history-client`. - Added Playwright E2E testing safeguards for background loading screens. - Completely removed the Lottie loader on all auth pages to ensure perfect accessibility and interactivity. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/auth/discord/page.tsx (1)
7-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate Discord authentication routes and handle SSO errors.
These three files are exact copies of one another, violating DRY principles. Additionally, they share a UX bug: if
signIn.sso()fails, the.catchblock logs the error but does not update any state, leaving the user permanently stuck on the "Redirecting to Discord SSO..." spinner.
app/auth/discord/page.tsx#L7-L50: Add an error state to display a fallback message or redirect the user back to the sign-in page ifsignIn.sso()fails.app/discord-auth/page.tsx#L7-L50: Delete this duplicate file and rely on Next.js redirects (e.g., innext.config.ts) if this route must be preserved for backward compatibility.app/sign-in/discord/page.tsx#L7-L50: Delete this duplicate file and rely on Next.js redirects.🤖 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/auth/discord/page.tsx` around lines 7 - 50, Update app/auth/discord/page.tsx lines 7-50 in DiscordAuthRedirect to track SSO failure and render a fallback message or redirect to sign-in when signIn.sso rejects, instead of leaving the spinner displayed. Delete the duplicate pages at app/discord-auth/page.tsx lines 7-50 and app/sign-in/discord/page.tsx lines 7-50; preserve either route only through Next.js redirects if backward compatibility is required.
🤖 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.
Outside diff comments:
In `@app/auth/discord/page.tsx`:
- Around line 7-50: Update app/auth/discord/page.tsx lines 7-50 in
DiscordAuthRedirect to track SSO failure and render a fallback message or
redirect to sign-in when signIn.sso rejects, instead of leaving the spinner
displayed. Delete the duplicate pages at app/discord-auth/page.tsx lines 7-50
and app/sign-in/discord/page.tsx lines 7-50; preserve either route only through
Next.js redirects if backward compatibility is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96631bd3-4a99-47a1-ad85-0a9332830267
📒 Files selected for processing (7)
app/auth/discord/page.tsxapp/discord-auth/page.tsxapp/sign-in/discord/page.tsxapp/sign-in/sign-in-component.tsxcomponents/conditional-lottie.tsxcomponents/settings/components/settings.tsxpublic/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)
🔇 Additional comments (5)
public/sw.js (1)
2-2: LGTM!components/conditional-lottie.tsx (1)
9-12: LGTM!Also applies to: 30-35
app/sign-in/sign-in-component.tsx (1)
21-27: Sanitizeredirect_urlbefore callingrouter.push.Clerk already protects
redirectUrlComplete, but the effect still trusts the query param and can send signed-in users to an external URL. Normalize it to a same-origin relative path (or fall back to/) before redirecting.🛡️ Proposed fix
- // Get redirect url from search params, default to '/' - const redirectUrl = searchParams?.get('redirect_url') || '/' + // Get redirect url from search params, default to '/' + const rawRedirectUrl = searchParams?.get('redirect_url') || '/' + const redirectUrl = rawRedirectUrl.startsWith('/') && !rawRedirectUrl.startsWith('//') ? rawRedirectUrl : '/'components/settings/components/settings.tsx (2)
265-321: Disable Connect/Disconnect while the request is in flight.Neither button guards against double-clicks during its async call (
destroy()+reload(), orcreateExternalAccount()). A rapid double-click can fire the non-idempotent Clerk call twice, typically surfacing a confusing error toast on the second attempt.Please introduce an
isLinkingstate and use it to disable these buttons while their respective operations are pending.
301-307: Route Discord linking through/sso-callback.
createExternalAccountneeds a callback route that rendersAuthenticateWithRedirectCallback(or callshandleRedirectCallback). Usingwindow.location.hrefhere sends Discord back to the Settings page without completing the OAuth handoff, leaving the account unlinked. Mirror the existing Clerk OAuth flow and point this to/sso-callbackinstead.🛠 Proposed fix
- const res = await clerkUser?.createExternalAccount({ - strategy: 'oauth_discord', - redirectUrl: window.location.href, - }); - if (res?.verification?.externalVerificationRedirectURL) { - window.location.href = res.verification.externalVerificationRedirectURL.href; - } + const res = await clerkUser?.createExternalAccount({ + strategy: 'oauth_discord', + redirectUrl: '/sso-callback', + }); + if (res?.verification?.externalVerificationRedirectURL) { + window.location.href = res.verification.externalVerificationRedirectURL.href; + }
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/sign-in/sign-in-component.tsx (1)
22-29: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSanitize
redirect_urlbefore callingrouter.push.As previously flagged,
redirectUrlis derived directly from query parameters without validation, which exposes the application to an open redirect vulnerability when passed torouter.push. Ensure the URL is validated as a same-origin relative path (or fall back to/) before redirecting.🔒 Proposed fix to prevent open redirects
- // Get redirect url from search params, default to '/' - const redirectUrl = searchParams?.get('redirect_url') || '/' + // Get redirect url from search params, default to '/' and ensure it's a relative path + const rawRedirectUrl = searchParams?.get('redirect_url') || '/' + const redirectUrl = rawRedirectUrl.startsWith('/') && !rawRedirectUrl.startsWith('//') ? rawRedirectUrl : '/' useEffect(() => {🤖 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/sign-in/sign-in-component.tsx` around lines 22 - 29, Sanitize the query-derived redirectUrl before it reaches router.push: accept only same-origin relative paths, and fall back to "/" for absolute, protocol-relative, or otherwise external URLs. Update the redirect initialization near the sign-in useEffect while preserving the existing redirect behavior for valid paths.
🤖 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/sign-in/sign-in-component.tsx`:
- Around line 105-114: Replace the Link wrapper around the Discord Button in the
sign-in component with an onClick navigation that includes the current
redirectUrl as the redirect_url query parameter, avoiding nested interactive
elements. Update the Discord sign-in page flow to read redirect_url from the
query parameters and pass it to signIn.sso() instead of hardcoding “/”, while
preserving “/” as the fallback.
---
Duplicate comments:
In `@app/sign-in/sign-in-component.tsx`:
- Around line 22-29: Sanitize the query-derived redirectUrl before it reaches
router.push: accept only same-origin relative paths, and fall back to "/" for
absolute, protocol-relative, or otherwise external URLs. Update the redirect
initialization near the sign-in useEffect while preserving the existing redirect
behavior for valid paths.
🪄 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: 6f8bb340-cbe0-4ed7-9ddb-7f60e2ce23ea
📒 Files selected for processing (1)
app/sign-in/sign-in-component.tsx
| <Link href="/sign-in/discord" className="w-full"> | ||
| <Button | ||
| variant="outline" | ||
| type="button" | ||
| className="w-full flex items-center justify-center gap-3 py-6 text-base font-semibold hover:bg-muted/80 border-border" | ||
| > | ||
| <FaDiscord className="h-5 w-5 text-[#5865F2]" /> | ||
| <span>Continue with Discord</span> | ||
| </Button> | ||
| </Link> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate redirect_url and avoid invalid HTML nesting.
The <Link> wrapper drops the redirectUrl state, meaning users who authenticate via Discord will lose their intended destination and always land on /. Additionally, nesting a <Button> (which renders a <button>) inside a <Link> (which renders an <a> tag) results in invalid HTML that can cause accessibility issues and hydration warnings.
Consider replacing the Link wrapper with an onClick handler on the button to propagate the redirect URL safely.
Note: To fully preserve the destination, you will also need to update the downstream app/sign-in/discord/page.tsx file to read the redirect_url query parameter and pass it into its signIn.sso() call, which currently hardcodes '/'.
🛠️ Proposed fix
- <Link href="/sign-in/discord" className="w-full">
- <Button
- variant="outline"
- type="button"
- className="w-full flex items-center justify-center gap-3 py-6 text-base font-semibold hover:bg-muted/80 border-border"
- >
- <FaDiscord className="h-5 w-5 text-[`#5865F2`]" />
- <span>Continue with Discord</span>
- </Button>
- </Link>
+ <Button
+ variant="outline"
+ type="button"
+ className="w-full flex items-center justify-center gap-3 py-6 text-base font-semibold hover:bg-muted/80 border-border"
+ onClick={() => router.push(`/sign-in/discord?redirect_url=${encodeURIComponent(redirectUrl)}`)}
+ >
+ <FaDiscord className="h-5 w-5 text-[`#5865F2`]" />
+ <span>Continue with Discord</span>
+ </Button>🤖 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/sign-in/sign-in-component.tsx` around lines 105 - 114, Replace the Link
wrapper around the Discord Button in the sign-in component with an onClick
navigation that includes the current redirectUrl as the redirect_url query
parameter, avoiding nested interactive elements. Update the Discord sign-in page
flow to read redirect_url from the query parameters and pass it to signIn.sso()
instead of hardcoding “/”, while preserving “/” as the fallback.
2 participants
Implemented the custom sign-in page that only shows Google SSO, with a separate custom redirect page for Discord SSO and a robust setting tab for linking existing user profiles with Discord. Tested and verified successfully.
PR created automatically by Jules for task 3043004821660012340 started by @ngoiyaeric
Summary by CodeRabbit