From 9fab07612b6634907c06ec82ef6f3e45edb09aa6 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 15 Apr 2026 03:06:31 +0000 Subject: [PATCH 1/2] Fix: don't redirect to /signin on callback session failure When the session API returned a non-ok response, the callback page was sending users to /signin which briefly showed a confusing "Redirecting to sign in..." screen even after a successful OAuth. Now failures surface an inline error with a "Try again" link on the callback page itself, so the /signin page is never shown post-authentication. https://claude.ai/code/session_01U3mwGcNbKKXSb9P6cBEaqa --- frontend/src/app/signin/callback/page.tsx | 42 +++++++++++++++++++++-- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/frontend/src/app/signin/callback/page.tsx b/frontend/src/app/signin/callback/page.tsx index 7d83290..b0fe14e 100644 --- a/frontend/src/app/signin/callback/page.tsx +++ b/frontend/src/app/signin/callback/page.tsx @@ -1,6 +1,6 @@ 'use client'; -import { useEffect, Suspense } from 'react'; +import { useEffect, useState, Suspense } from 'react'; import { useSearchParams, useRouter } from 'next/navigation'; import { useUser } from '@/context/UserContext'; @@ -8,6 +8,7 @@ function CallbackInner() { const searchParams = useSearchParams(); const router = useRouter(); const { setActiveUser, confirmApproved } = useUser(); + const [errorMsg, setErrorMsg] = useState(null); useEffect(() => { const userId = searchParams.get('user_id'); @@ -31,15 +32,50 @@ function CallbackInner() { if (res.ok) { confirmApproved(); router.replace('/dashboard'); + } else if (res.status === 403) { + router.replace('/pending'); } else { - router.replace('/signin'); + setErrorMsg('Unable to complete sign-in. Please try again.'); } + }).catch(() => { + setErrorMsg('Unable to reach the server. Please try again.'); }); } else { - router.replace('/signin'); + setErrorMsg('Sign-in failed. Please try again.'); } }, []); // eslint-disable-line react-hooks/exhaustive-deps + if (errorMsg) { + return ( +
+

{errorMsg}

+ + Try again + +
+ ); + } + return (
Date: Wed, 15 Apr 2026 03:24:40 +0000 Subject: [PATCH 2/2] Address CodeRabbit feedback on PR #59 - Distinguish missing is_approved param from explicit denial. Previously a missing param was silently coerced to "not approved" and bounced the user to /pending, hiding callback failures. Now /pending only fires for an explicit "false" or error=not_approved; a missing param shows the inline error state. - Add tests covering all new callback branches: dashboard redirect on success, /pending on 403, inline error on other failures, inline error on network rejection, inline error on missing user_id/name, inline error on missing is_approved, /pending on is_approved=false, /pending on error=not_approved. https://claude.ai/code/session_01U3mwGcNbKKXSb9P6cBEaqa --- .../src/__tests__/signinCallback.test.tsx | 123 ++++++++++++++++++ frontend/src/app/signin/callback/page.tsx | 9 +- 2 files changed, 130 insertions(+), 2 deletions(-) create mode 100644 frontend/src/__tests__/signinCallback.test.tsx diff --git a/frontend/src/__tests__/signinCallback.test.tsx b/frontend/src/__tests__/signinCallback.test.tsx new file mode 100644 index 0000000..c42da91 --- /dev/null +++ b/frontend/src/__tests__/signinCallback.test.tsx @@ -0,0 +1,123 @@ +/** + * Tests for app/signin/callback/page.tsx + * + * Covers the new error branches added to the OAuth callback handler: + * - Successful session → redirect to /dashboard + * - 403 from session API → redirect to /pending + * - Other failures from session API → inline error + * - Network errors → inline error + * - Missing user_id/name params → inline error + * - Missing is_approved param → inline error (not a /pending redirect) + * - Explicit is_approved=false → /pending redirect + */ +import React from 'react'; +import { render, screen, waitFor } from '@testing-library/react'; + +const replace = jest.fn(); +const setActiveUser = jest.fn(); +const confirmApproved = jest.fn(); +let searchParams = new URLSearchParams(); + +jest.mock('next/navigation', () => ({ + useRouter: () => ({ replace }), + useSearchParams: () => searchParams, +})); + +jest.mock('@/context/UserContext', () => ({ + useUser: () => ({ setActiveUser, confirmApproved }), +})); + +import CallbackPage from '@/app/signin/callback/page'; + +beforeEach(() => { + jest.clearAllMocks(); + searchParams = new URLSearchParams(); + global.fetch = jest.fn(); +}); + +afterEach(() => { + // @ts-expect-error allow cleanup + delete global.fetch; +}); + +function setParams(params: Record) { + searchParams = new URLSearchParams(params); +} + +describe('signin/callback page', () => { + it('redirects to /dashboard on a successful session', async () => { + setParams({ user_id: 'u1', name: 'Ada', is_approved: 'true' }); + (global.fetch as jest.Mock).mockResolvedValue({ ok: true, status: 200 }); + + render(); + + await waitFor(() => expect(replace).toHaveBeenCalledWith('/dashboard')); + expect(setActiveUser).toHaveBeenCalledWith('u1', 'Ada', ''); + expect(confirmApproved).toHaveBeenCalled(); + }); + + it('redirects to /pending when session API returns 403', async () => { + setParams({ user_id: 'u1', name: 'Ada', is_approved: 'true' }); + (global.fetch as jest.Mock).mockResolvedValue({ ok: false, status: 403 }); + + render(); + + await waitFor(() => expect(replace).toHaveBeenCalledWith('/pending')); + expect(confirmApproved).not.toHaveBeenCalled(); + }); + + it('shows an inline error when session API returns a non-403 failure', async () => { + setParams({ user_id: 'u1', name: 'Ada', is_approved: 'true' }); + (global.fetch as jest.Mock).mockResolvedValue({ ok: false, status: 500 }); + + render(); + + expect(await screen.findByText(/unable to complete sign-in/i)).toBeInTheDocument(); + expect(replace).not.toHaveBeenCalled(); + }); + + it('shows an inline error when the session fetch rejects', async () => { + setParams({ user_id: 'u1', name: 'Ada', is_approved: 'true' }); + (global.fetch as jest.Mock).mockRejectedValue(new Error('network down')); + + render(); + + expect(await screen.findByText(/unable to reach the server/i)).toBeInTheDocument(); + expect(replace).not.toHaveBeenCalled(); + }); + + it('shows an inline error when user_id is missing', async () => { + setParams({ name: 'Ada', is_approved: 'true' }); + + render(); + + expect(await screen.findByText(/sign-in failed/i)).toBeInTheDocument(); + expect(global.fetch).not.toHaveBeenCalled(); + expect(replace).not.toHaveBeenCalled(); + }); + + it('shows an inline error when is_approved is missing entirely', async () => { + setParams({ user_id: 'u1', name: 'Ada' }); + + render(); + + expect(await screen.findByText(/sign-in failed/i)).toBeInTheDocument(); + expect(replace).not.toHaveBeenCalled(); + }); + + it('redirects to /pending when is_approved is explicitly "false"', async () => { + setParams({ user_id: 'u1', name: 'Ada', is_approved: 'false' }); + + render(); + + await waitFor(() => expect(replace).toHaveBeenCalledWith('/pending')); + }); + + it('redirects to /pending when error=not_approved is set', async () => { + setParams({ error: 'not_approved' }); + + render(); + + await waitFor(() => expect(replace).toHaveBeenCalledWith('/pending')); + }); +}); diff --git a/frontend/src/app/signin/callback/page.tsx b/frontend/src/app/signin/callback/page.tsx index b0fe14e..70f80be 100644 --- a/frontend/src/app/signin/callback/page.tsx +++ b/frontend/src/app/signin/callback/page.tsx @@ -14,14 +14,19 @@ function CallbackInner() { const userId = searchParams.get('user_id'); const name = searchParams.get('name'); const avatar = searchParams.get('avatar'); - const isApproved = searchParams.get('is_approved') === 'true'; + const approvedParam = searchParams.get('is_approved'); const error = searchParams.get('error'); - if (error === 'not_approved' || !isApproved) { + if (error === 'not_approved' || approvedParam === 'false') { router.replace('/pending'); return; } + if (approvedParam !== 'true') { + setErrorMsg('Sign-in failed. Please try again.'); + return; + } + if (userId && name) { setActiveUser(userId, name, avatar || ''); fetch('/api/auth/session', {