fix(auth): register Google/GitHub OAuth via fetch + redirect#240
Conversation
The OAuth API returns JSON with the provider URL; document navigation showed raw JSON. Align register with login using loginWithOAuth. Add optional Playwright OAuth entrypoint tests (RUN_OAUTH_E2E=1), including register page. Made-with: Cursor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
size/M
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdd ARIA/loading semantics to OAuth buttons in the registration form and introduce Playwright E2E tests that verify Google/GitHub OAuth redirect behavior for login and register pages. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
seanhanca
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/web-next/src/app/`(auth)/register/register-form.tsx:
- Around line 70-82: The oauth loading flag isn't reset on success in
handleOAuthRegister; ensure setOauthLoading(false) is called on all control
paths by moving it into a finally-equivalent place (e.g., use a try { await
loginWithOAuth(provider); } finally { setOauthLoading(false); }) or by adding a
subsequent setOauthLoading(false) after the awaited loginWithOAuth call; update
the handleOAuthRegister callback (which references loginWithOAuth and setError)
so oauthLoading is cleared whether loginWithOAuth succeeds or throws.
In `@apps/web-next/tests/oauth.spec.ts`:
- Around line 12-16: The test gating treats any non-empty RUN_OAUTH_E2E value as
enabled; change the condition in the test.beforeEach to explicitly check for the
string '1' (e.g., process.env.RUN_OAUTH_E2E !== '1') so only RUN_OAUTH_E2E='1'
runs the OAuth E2E tests; update the test.skip invocation in oauth.spec.ts
accordingly to use this strict equality check.
🪄 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: e4b6aca1-6265-4c05-b3df-bb9a67beb1d4
📒 Files selected for processing (2)
apps/web-next/src/app/(auth)/register/register-form.tsxapps/web-next/tests/oauth.spec.ts
…='1' Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/web-next/src/app/(auth)/register/register-form.tsx (1)
211-230: Minor UX note: both OAuth buttons show identical spinners during loading.When
oauthLoadingis true, both buttons display only a spinner without the provider label (e.g., "Google" or "GitHub"). This makes it unclear which provider the user clicked. Since the redirect happens quickly, this may be acceptable, but consider tracking which provider is loading to show a spinner only on the clicked button.Optional: Track which provider is loading
- const [oauthLoading, setOauthLoading] = useState(false); + const [oauthLoading, setOauthLoading] = useState<'google' | 'github' | null>(null); const handleOAuthRegister = useCallback( async (provider: 'google' | 'github') => { setError(null); - setOauthLoading(true); + setOauthLoading(provider); try { await loginWithOAuth(provider); } catch (err) { setError(err instanceof Error ? err.message : 'OAuth sign up failed'); } finally { - setOauthLoading(false); + setOauthLoading(null); } }, [loginWithOAuth], );Then update button disabled/spinner logic:
- disabled={isLoading || oauthLoading} + disabled={isLoading || oauthLoading !== null} ... - {oauthLoading ? ( + {oauthLoading === 'google' ? (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/`(auth)/register/register-form.tsx around lines 211 - 230, The two OAuth buttons use the shared oauthLoading boolean so both show a spinner; introduce a piece of state (e.g., oauthLoadingProvider or loadingProvider) and set it inside handleOAuthRegister(provider) before starting the flow, then change each button to show the spinner and disabled state only when oauthLoading && loadingProvider === 'google' (or 'github' respectively) while keeping the global oauthLoading behavior for overall flow; update handleOAuthRegister and the button render logic (referencing handleOAuthRegister and oauthLoading in the register form) to use this provider-specific loading flag.apps/web-next/tests/oauth.spec.ts (2)
41-57: Register tests lackgoBackassertions present in login tests.The login OAuth tests include
goBackand button visibility assertions (lines 26-27, 37-38), but the register tests omit them. Consider adding for consistency, especially since the register form's loading state handling was the focus of this fix.Optional: Add goBack assertions for register tests
test('Register: Google button navigates toward Google OAuth', async ({ page }) => { await page.goto('/register'); const btn = page.getByRole('button', { name: 'Google' }); await expect(btn).toBeEnabled(); const nav = page.waitForURL(/google\.com|accounts\.google\./i, { timeout: 20_000 }); await Promise.all([nav, btn.click()]); expect(page.url()).toMatch(/google/i); + await page.goBack({ waitUntil: 'domcontentloaded' }); + await expect(btn).toBeVisible({ timeout: 15_000 }); }); test('Register: GitHub button navigates toward GitHub OAuth', async ({ page }) => { await page.goto('/register'); const btn = page.getByRole('button', { name: 'GitHub' }); await expect(btn).toBeEnabled(); const nav = page.waitForURL(/github\.com/i, { timeout: 20_000 }); await Promise.all([nav, btn.click()]); expect(page.url()).toMatch(/github/i); + await page.goBack({ waitUntil: 'domcontentloaded' }); + await expect(btn).toBeVisible({ timeout: 15_000 }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/tests/oauth.spec.ts` around lines 41 - 57, Add the same post-navigation assertions used in the login tests to the register OAuth tests: after initiating navigation in the tests "Register: Google button navigates toward Google OAuth" and "Register: GitHub button navigates toward GitHub OAuth", call page.goBack() and then assert the OAuth button is visible/enabled again (mirror the existing goBack and visibility checks from the login tests) so the register flow validates returning from OAuth and the button state is correct.
12-17: Remove unusedtestInfoparameter.The
testInfoparameter is declared but never used. The empty destructuring({ }, testInfo)can be simplified.Suggested fix
- test.beforeEach(({ }, testInfo) => { + test.beforeEach(() => { test.skip( process.env.RUN_OAUTH_E2E !== '1', 'Set RUN_OAUTH_E2E=1 to run OAuth redirect checks (see tests/E2E-OAUTH-MANUAL.md)', ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/tests/oauth.spec.ts` around lines 12 - 17, The before-each hook declares an unused parameter list; update the test.beforeEach declaration to remove the empty destructuring and unused testInfo parameter so the callback takes no parameters (i.e., replace "({ }, testInfo) =>" with "() =>"). This keeps the test.skip logic intact while eliminating the unused parameter warning in the test.beforeEach hook.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/web-next/src/app/`(auth)/register/register-form.tsx:
- Around line 211-230: The two OAuth buttons use the shared oauthLoading boolean
so both show a spinner; introduce a piece of state (e.g., oauthLoadingProvider
or loadingProvider) and set it inside handleOAuthRegister(provider) before
starting the flow, then change each button to show the spinner and disabled
state only when oauthLoading && loadingProvider === 'google' (or 'github'
respectively) while keeping the global oauthLoading behavior for overall flow;
update handleOAuthRegister and the button render logic (referencing
handleOAuthRegister and oauthLoading in the register form) to use this
provider-specific loading flag.
In `@apps/web-next/tests/oauth.spec.ts`:
- Around line 41-57: Add the same post-navigation assertions used in the login
tests to the register OAuth tests: after initiating navigation in the tests
"Register: Google button navigates toward Google OAuth" and "Register: GitHub
button navigates toward GitHub OAuth", call page.goBack() and then assert the
OAuth button is visible/enabled again (mirror the existing goBack and visibility
checks from the login tests) so the register flow validates returning from OAuth
and the button state is correct.
- Around line 12-17: The before-each hook declares an unused parameter list;
update the test.beforeEach declaration to remove the empty destructuring and
unused testInfo parameter so the callback takes no parameters (i.e., replace "({
}, testInfo) =>" with "() =>"). This keeps the test.skip logic intact while
eliminating the unused parameter warning in the test.beforeEach hook.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90fc47b4-9aff-4f95-8277-1b933369fa33
📒 Files selected for processing (2)
apps/web-next/src/app/(auth)/register/register-form.tsxapps/web-next/tests/oauth.spec.ts
eliteprox
left a comment
eliteprox
left a comment
There was a problem hiding this comment.
Core fix is sound and matches the API’s JSON contract. Tighten test comments and the manual-doc reference (or add the doc), and optionally improve loading-state accessibility on the OAuth buttons.
| * OAuth provider redirects are flaky in CI (Google/GitHub bot detection). | ||
| * Run explicitly: `RUN_OAUTH_E2E=1 npx playwright test oauth.spec.ts` | ||
| * Default `npx playwright test` skips these unless RUN_OAUTH_E2E is exactly `1`. | ||
| * In CI, tests matching @oauth are also excluded via playwright.config.ts grepInvert. |
There was a problem hiding this comment.
Inaccurate comment in oauth.spec.ts
The file says tests matching oauth are excluded in CI via playwright.config.ts grepInvert. PR #240 only changes two files, and playwright.config.ts on main has no such grepInvert. Skipping still happens in beforeEach, but the grepInvert sentence should be removed or the config updated so the comment stays true.
| test.beforeEach(() => { | ||
| test.skip( | ||
| process.env.RUN_OAUTH_E2E !== '1', | ||
| 'Set RUN_OAUTH_E2E=1 to run OAuth redirect checks (see tests/E2E-OAUTH-MANUAL.md)', |
There was a problem hiding this comment.
The skip message points at tests/E2E-OAUTH-MANUAL.md. repo search shows no such file (at least on current main).
Either add a short doc or change the message to something that exists (e.g. .env.local.example already mentions RUN_OAUTH_E2E).
| {oauthLoadingProvider === 'google' ? ( | ||
| <Loader2 className="h-4 w-4 animate-spin" /> | ||
| ) : ( |
There was a problem hiding this comment.
Accessibility while loading
When oauthLoadingProvider === 'google'|'github', the button content is only <Loader2 />, so the visible “Google” / “GitHub” label disappears. For consistency and a11y, consider aria-label (e.g. Continue with Google) and/or aria-busy="true" on the button while loading.
| const handleOAuthRegister = useCallback( | ||
| async (provider: 'google' | 'github') => { | ||
| setError(null); | ||
| setOauthLoadingProvider(provider); | ||
| try { | ||
| await loginWithOAuth(provider); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : 'OAuth sign up failed'); | ||
| } finally { | ||
| setOauthLoadingProvider(null); | ||
| } | ||
| }, | ||
| [loginWithOAuth], | ||
| ); |
There was a problem hiding this comment.
finally after successful redirect
On success, loginWithOAuth assigns window.location.href, then your finally clears oauthLoadingProvider. That usually runs before unload; if it ever races with unmount you could see a benign “setState on unmounted component” in dev.
Low priority; login doesn’t use finally for this path.
I would recommend avoiding JS to catch the call-back for the authorization_code. I would instead capture the code in the redirect_url and set a Location header to redirect the browser on successful authorization.
See below recommendation:
Redirect with Location (302/303)
If the user hits your OAuth start URL with a full navigation (, window.location, or a form GET), the server can respond with 302/303 + Set-Cookie (oauth state) + Location: https://accounts.google.com/.... The browser follows the redirect; users never see a JSON body.
Pros
No client-side “parse JSON then assign location.href” for the happy path.
Works without JavaScript (plain links).
Matches how many OAuth guides describe “redirect to authorize URL.”
Cons / constraints
SPAs that only fetch() this URL won’t get a useful automatic redirect to Google: fetch won’t replace the page with the provider the way a top-level navigation does (and following cross-origin redirects inside fetch is the wrong tool). So you still either use top-level navigation to your start URL or keep the current fetch + window.location.href pattern.
You must still set state in a cookie on that first response; doing it on the redirect response is standard and works.
Errors (provider not configured, etc.) are awkward as redirects — you usually 302 to an error page or keep a JSON endpoint for programmatic clients and a separate “start OAuth” route that only redirects.
Compared to finally / loading state
Using a top-level navigation to a redirecting start URL means the page is about to unload, so client finally / spinners matter less on success (the browser leaves your app). You’d still use try/catch only when you use fetch for initiation or for reporting errors without navigating.
Practical recommendation
Two surfaces are often clean: a GET route that 302s to the IdP for buttons/links, and optionally the existing JSON route for API-style callers.
For your app, either keep fetch + window.location.href (what loginWithOAuth does now) or change buttons to window.location.href = '/api/.../oauth/google' (no fetch) if that route returns 302 + Location. Both avoid showing JSON; the second drops the extra client step and makes finally mostly irrelevant on success.
Made-with: Cursor
Made-with: Cursor
github-actions
Bot
added
size/XS
2 participants
Summary
Register page Google/GitHub buttons used
window.locationto/api/v1/auth/oauth/:provider, which returns JSON. The browser showed raw JSON instead of redirecting to the provider.This change uses
useAuth().loginWithOAuth(same as login):fetchwith credentials, thenwindow.location.hreftodata.url.Tests
apps/web-next/tests/oauth.spec.ts— optional Playwright checks for login + register OAuth entrypoints; run withRUN_OAUTH_E2E=1(skipped by default in CI).Verification
npm run typecheckinapps/web-nextMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests