Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 49 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughIntegrates Supabase Auth into the Next.js website: adds SSR/browser Supabase clients, env validation, login UI and OAuth callback, auth-aware proxy/middleware, safe redirect utility, account menu, and tests for redirect sanitization. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant NextServer as Next.js Server
participant SupaAuth as Supabase Auth
rect rgba(100, 150, 200, 0.5)
note over Browser, SupaAuth: OAuth Sign-In Flow
Browser->>Browser: Click OAuth (Facebook) in LoginForm
Browser->>SupaAuth: signInWithOAuth requested (browser)
SupaAuth-->>Browser: Authorization URL
Browser-->>Browser: Redirect to provider, user authenticates
Browser->>NextServer: GET /auth/callback?code=...
NextServer->>SupaAuth: exchangeCodeForSession(code) (server)
SupaAuth-->>NextServer: Session + Set-Cookie
NextServer-->>Browser: Redirect to safe next path (cookies set)
end
sequenceDiagram
participant Browser
participant NextServer as Next.js Server
participant SupaAuth as Supabase Auth
rect rgba(150, 200, 100, 0.5)
note over Browser, NextServer: Protected Route Access via Proxy
Browser->>NextServer: GET /protected-route (with cookies)
NextServer->>NextServer: proxy: createServerClient(cookies) -> getUser()
alt Authenticated
NextServer-->>Browser: Serve page
else Unauthenticated
NextServer-->>Browser: Redirect to /login?next=/protected-route
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
apps/website/src/app/lib/supabase/config.ts (1)
3-11: Runtime check is redundant givenz.url()in the schema.
NEXT_PUBLIC_SUPABASE_URLis validated as a required URL inwebsiteEnvSchema, soenv.NEXT_PUBLIC_SUPABASE_URLcannot be empty here (except whenSKIP_ENV_VALIDATION=true). The extra guard is harmless; optional cleanup only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/website/src/app/lib/supabase/config.ts` around lines 3 - 11, The runtime null-check in getSupabaseUrl duplicates validation already enforced by websiteEnvSchema; remove the redundant guard and simply return env.NEXT_PUBLIC_SUPABASE_URL (or use a non-null assertion) so the function becomes a thin accessor; keep a short comment referencing websiteEnvSchema or SKIP_ENV_VALIDATION if you want to document the exceptional bypass.apps/website/src/app/lib/supabase/client.ts (1)
6-14: Module-scoped singleton is fine; consider aDatabasegeneric.For end-to-end typing of
supabase.from(...), parameterizecreateBrowserClient<Database>()once types are generated. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/website/src/app/lib/supabase/client.ts` around lines 6 - 14, The module-scoped singleton should instantiate the Supabase client with the generated Database types: update the browserClient initialization in createClient to call createBrowserClient with the Database generic (e.g., createBrowserClient<Database>(...)) and type browserClient as ReturnType<typeof createBrowserClient<Database>> | undefined so all supabase.from(...) calls are fully typed; ensure you import or reference the generated Database type and adjust the browserClient declaration and createClient function accordingly.apps/website/src/app/login/components/LoginForm.tsx (1)
27-50: Nit: reusegetCallbackUrl()in the sign-up flow.
getCallbackUrl()already builds${origin}/auth/callback?next=...but sign-up duplicates the expression inline. Using the helper keeps the two callback URLs in lockstep.♻️ Proposed fix
: await supabase.auth.signUp({ email: trimmedEmail, password, options: { - emailRedirectTo: `${window.location.origin}/auth/callback?next=${encodeURIComponent(safeNextPath)}`, + emailRedirectTo: getCallbackUrl(), }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/website/src/app/login/components/LoginForm.tsx` around lines 27 - 50, The sign-up branch in handleSubmit duplicates the callback URL expression instead of reusing getCallbackUrl(); update the options.emailRedirectTo passed to supabase.auth.signUp to call getCallbackUrl() (same as the sign-in path uses getCallbackUrl) so both flows use the single helper; modify the code in handleSubmit where supabase.auth.signUp is called to replace the inline `${window.location.origin}/auth/callback?next=...` with getCallbackUrl().apps/website/src/app/components/AuthMenu.tsx (1)
19-50: Optional: handleloadUser()rejection so the UI doesn't get stuck on "Loading...".
loadUser()is invoked fire-and-forget. Ifsupabase.auth.getUser()rejects (network error, etc.),isLoadingis never cleared and the UI sticks on "Loading...". The subsequentonAuthStateChangefires on sign-in/out but not for the initial failed fetch, so the stuck state can persist until the user acts.♻️ Proposed fix
async function loadUser() { - const { - data: { user: currentUser }, - } = await supabase.auth.getUser(); - - if (!isMounted) { - return; - } - - setUser(currentUser); - setIsLoading(false); + try { + const { + data: { user: currentUser }, + } = await supabase.auth.getUser(); + if (!isMounted) return; + setUser(currentUser); + } finally { + if (isMounted) setIsLoading(false); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/website/src/app/components/AuthMenu.tsx` around lines 19 - 50, The loadUser() async call inside the useEffect can throw and leave the component stuck in loading; update loadUser to catch/reject errors (wrap the supabase.auth.getUser() call in try/catch) and ensure setIsLoading(false) runs in both success and failure paths (respecting the isMounted guard) and optionally log the error; also ensure the cleanup still unsubscribes the subscription in the return. Target the loadUser function, the useEffect cleanup block, and the calls to supabase.auth.getUser(), setUser, and setIsLoading..agents/skills/nextjs-supabase-auth/SKILL.md (1)
278-291: Nit: specify a language on the workflow fenced code blocks (MD040).markdownlint flags these as missing a language. Use
```text(or similar) for the plain-text workflow listings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/nextjs-supabase-auth/SKILL.md around lines 278 - 291, The fenced code blocks in SKILL.md that list the numbered workflow and the Workflow: plain-text section lack a language specifier; update each opening triple-backtick to include a language such as text (e.g., change ``` to ```text) for the blocks containing "1. Database setup (supabase-backend)..." and the Workflow block so markdownlint MD040 is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/website/src/app/lib/redirect.ts`:
- Around line 1-9: getSafeNextPath allows values like "/\\evil.com" because it
only checks for '//' and not backslashes or control characters; update the
function signature to export function getSafeNextPath(nextPath: string |
string[] | undefined): string and add stricter validation: normalize to the
first string, trim it, reject if empty, then ensure it starts with a single
forward slash (value[0] === '/') but not a double slash (value.startsWith('//'))
and also reject if it starts with "/\" or contains any backslash characters
(value.includes('\\')) or any control/whitespace characters (use a regex to
detect [\u0000-\u001F\s]); return '/' for any failing case and return the
original value otherwise, referencing the getSafeNextPath function and the local
variable value in the diff.
In `@apps/website/src/app/lib/supabase/server.ts`:
- Around line 1-20: The setAll implementation in createClient can throw when
called from a Server Component because cookieStore.set may throw in that
environment; wrap the body of setAll in a try-catch and swallow or log the error
(do not rethrow) so server-side renders won't crash; specifically update
createClient's cookies.setAll (the function that iterates cookiesToSet and calls
cookieStore.set) to catch any exceptions and no-op (or log) them, preserving the
existing behavior for getAll and relying on your middleware (proxy.ts) to
refresh sessions.
In `@apps/website/src/app/login/page.tsx`:
- Around line 7-24: LoginPage currently treats searchParams as a plain object
but in Next.js 16 it is a Promise; update the LoginPageProps to type
searchParams?: Promise<{ next?: string; error?: string }> and inside the async
LoginPage function await it (e.g. const resolvedSearchParams = await
searchParams) then use resolvedSearchParams when calling getSafeNextPath and
when checking/displaying the error; ensure all references to searchParams
(including the redirect call using getSafeNextPath and the later error display)
are switched to the awaited resolvedSearchParams variable.
In `@apps/website/src/proxy.ts`:
- Around line 20-60: The early-return branches create new NextResponse objects
(the API error via NextResponse.json, the unauthenticated redirect, and the
post-login redirect) and thus drop any refreshed cookies written into the local
response by the supabase client (setAll callback after supabase.auth.getUser());
fix by copying all cookies from the original response to the new response before
returning, e.g., implement a small helper like withRefreshedCookies(newResp,
response) that iterates response.cookies and sets them onto newResp, and call it
in the branches that return NextResponse.json or NextResponse.redirect (the
branches using isApiPath(...), the unauthenticated redirect that builds
loginUrl, and the redirect when user && pathname === '/login').
---
Nitpick comments:
In @.agents/skills/nextjs-supabase-auth/SKILL.md:
- Around line 278-291: The fenced code blocks in SKILL.md that list the numbered
workflow and the Workflow: plain-text section lack a language specifier; update
each opening triple-backtick to include a language such as text (e.g., change
``` to ```text) for the blocks containing "1. Database setup
(supabase-backend)..." and the Workflow block so markdownlint MD040 is
satisfied.
In `@apps/website/src/app/components/AuthMenu.tsx`:
- Around line 19-50: The loadUser() async call inside the useEffect can throw
and leave the component stuck in loading; update loadUser to catch/reject errors
(wrap the supabase.auth.getUser() call in try/catch) and ensure
setIsLoading(false) runs in both success and failure paths (respecting the
isMounted guard) and optionally log the error; also ensure the cleanup still
unsubscribes the subscription in the return. Target the loadUser function, the
useEffect cleanup block, and the calls to supabase.auth.getUser(), setUser, and
setIsLoading.
In `@apps/website/src/app/lib/supabase/client.ts`:
- Around line 6-14: The module-scoped singleton should instantiate the Supabase
client with the generated Database types: update the browserClient
initialization in createClient to call createBrowserClient with the Database
generic (e.g., createBrowserClient<Database>(...)) and type browserClient as
ReturnType<typeof createBrowserClient<Database>> | undefined so all
supabase.from(...) calls are fully typed; ensure you import or reference the
generated Database type and adjust the browserClient declaration and
createClient function accordingly.
In `@apps/website/src/app/lib/supabase/config.ts`:
- Around line 3-11: The runtime null-check in getSupabaseUrl duplicates
validation already enforced by websiteEnvSchema; remove the redundant guard and
simply return env.NEXT_PUBLIC_SUPABASE_URL (or use a non-null assertion) so the
function becomes a thin accessor; keep a short comment referencing
websiteEnvSchema or SKIP_ENV_VALIDATION if you want to document the exceptional
bypass.
In `@apps/website/src/app/login/components/LoginForm.tsx`:
- Around line 27-50: The sign-up branch in handleSubmit duplicates the callback
URL expression instead of reusing getCallbackUrl(); update the
options.emailRedirectTo passed to supabase.auth.signUp to call getCallbackUrl()
(same as the sign-in path uses getCallbackUrl) so both flows use the single
helper; modify the code in handleSubmit where supabase.auth.signUp is called to
replace the inline `${window.location.origin}/auth/callback?next=...` with
getCallbackUrl().
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c04b9d38-fc57-44cf-905c-507767ba081e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.agents/skills/nextjs-supabase-auth/SKILL.mdapps/website/package.jsonapps/website/src/app/applications/page.tsxapps/website/src/app/auth/callback/route.tsapps/website/src/app/components/AuthMenu.tsxapps/website/src/app/lib/redirect.tsapps/website/src/app/lib/supabase/client.tsapps/website/src/app/lib/supabase/config.tsapps/website/src/app/lib/supabase/server.tsapps/website/src/app/login/components/LoginForm.tsxapps/website/src/app/login/page.tsxapps/website/src/app/page.tsxapps/website/src/app/utils/env.tsapps/website/src/proxy.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/website/src/app/lib/redirect.spec.ts`:
- Around line 8-15: The test block in redirect.spec.ts misplaces the array
assertion: move the expect(getSafeNextPath([' /jobs ',
'/ignored'])).toBe('/jobs') out of the "returns root for unsafe paths" it-block
and into the "returns safe value" test (or create a new it-case named e.g.
"normalizes array input"), so each test description matches its assertions;
while there, add extra assertions covering other guarded cases
(expect(getSafeNextPath(undefined)).toBe('/'),
expect(getSafeNextPath('jobs')).toBe('/'),
expect(getSafeNextPath(['//evil.com'])).toBe('/'), and
expect(getSafeNextPath('/path\twithtab')).toBe('/')) to fully lock down
getSafeNextPath's contract.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b51faad-0abb-4621-8b3c-28c166a4e70c
📒 Files selected for processing (6)
apps/website/src/app/lib/redirect.spec.tsapps/website/src/app/lib/redirect.tsapps/website/src/app/lib/supabase/config.tsapps/website/src/app/lib/supabase/server.tsapps/website/src/app/login/page.tsxapps/website/src/proxy.ts
✅ Files skipped from review due to trivial changes (1)
- apps/website/src/app/lib/supabase/config.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/website/src/app/lib/redirect.ts
- apps/website/src/app/lib/supabase/server.ts
- apps/website/src/app/login/page.tsx
- apps/website/src/proxy.ts
lojzatran
force-pushed
the
add-auth
branch
3 times, most recently
from
5e93d08 to
45035bf
Compare
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.
Summary by CodeRabbit