Fix: don't redirect to /signin on callback session failure#59
Merged
Jose-Gael-Cruz-Lopez merged 2 commits intoApr 15, 2026
Conversation
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
📝 WalkthroughWalkthroughSign-in callback now tracks local error state and refines control flow: checks Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant CallbackPage
participant UserContext
participant SessionAPI as /api/auth/session
participant Router
Browser->>CallbackPage: Load /signin/callback?user_id=...&is_approved=...
CallbackPage->>CallbackPage: validate query params & approvedParam
alt approvedParam == "true" and params present
CallbackPage->>SessionAPI: POST session (userId, name, token...)
alt SessionAPI returns 200 OK
SessionAPI-->>CallbackPage: 200 + session data
CallbackPage->>UserContext: setActiveUser(...) / confirmApproved(...)
CallbackPage->>Router: replace("/dashboard")
else SessionAPI returns 403
SessionAPI-->>CallbackPage: 403
CallbackPage->>Router: replace("/pending")
else other non-OK or network error
SessionAPI-->>CallbackPage: error
CallbackPage-->>Browser: render error UI (errorMsg + Try again link)
end
else approvedParam == "false" or error=not_approved
CallbackPage->>Router: replace("/pending")
else missing required params
CallbackPage-->>Browser: render error UI (errorMsg)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Deploying web with
|
| Latest commit: |
6e4a611
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dfc1b397.web-75h.pages.dev |
| Branch Preview URL: | https://claude-sapling-codebase-anal.web-75h.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
frontend/src/app/signin/callback/page.tsx (2)
17-23:⚠️ Potential issue | 🟠 MajorHandle missing
is_approvedseparately from explicit denialLine 20 currently treats a missing
is_approvedparam as “not approved” and redirects to/pending. That hides callback failures instead of showing the new inline error state.Suggested fix
- const isApproved = searchParams.get('is_approved') === 'true'; + const approvedParam = searchParams.get('is_approved'); - 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; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/signin/callback/page.tsx` around lines 17 - 23, The current check uses const isApproved = searchParams.get('is_approved') === 'true', which makes a missing is_approved param behave like an explicit denial; change to read the raw param (e.g., const isApprovedParam = searchParams.get('is_approved')) and only call router.replace('/pending') when error === 'not_approved' OR isApprovedParam === 'false' (explicit denial). For the case isApprovedParam === null, do not redirect—let the component render the new inline error state instead. Update references to isApproved in this file (page.tsx) to use the new raw param logic so missing, explicit false, and true are handled distinctly.
25-45:⚠️ Potential issue | 🟡 MinorAdd tests for new callback error branches before merge
The new behavior paths at lines 31-45 (fetch with 403 handling and error states) and lines 48-77 (error message UI display) lack corresponding test coverage. Per coding guidelines, significant behavioral changes require tests. Verify or add tests covering:
- 403 response handling (redirect to
/pending)- Network errors (fetch catch, error message display)
- Missing userId/name validation (error message display)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/signin/callback/page.tsx` around lines 25 - 45, Add unit/integration tests that exercise the new callback branches in the signin flow: test the fetch('/api/auth/session') success path triggers confirmApproved() and router.replace('/dashboard'); test a 403 response from fetch causes router.replace('/pending'); simulate a network error to assert setErrorMsg('Unable to reach the server. Please try again.') is shown; and test missing userId or name input results in setErrorMsg('Sign-in failed. Please try again.'). Locate the logic around setActiveUser, confirmApproved, router.replace and setErrorMsg in the callback handler to stub/mock fetch responses and router behavior and assert UI/state outcomes accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/app/signin/callback/page.tsx`:
- Around line 17-23: The current check uses const isApproved =
searchParams.get('is_approved') === 'true', which makes a missing is_approved
param behave like an explicit denial; change to read the raw param (e.g., const
isApprovedParam = searchParams.get('is_approved')) and only call
router.replace('/pending') when error === 'not_approved' OR isApprovedParam ===
'false' (explicit denial). For the case isApprovedParam === null, do not
redirect—let the component render the new inline error state instead. Update
references to isApproved in this file (page.tsx) to use the new raw param logic
so missing, explicit false, and true are handled distinctly.
- Around line 25-45: Add unit/integration tests that exercise the new callback
branches in the signin flow: test the fetch('/api/auth/session') success path
triggers confirmApproved() and router.replace('/dashboard'); test a 403 response
from fetch causes router.replace('/pending'); simulate a network error to assert
setErrorMsg('Unable to reach the server. Please try again.') is shown; and test
missing userId or name input results in setErrorMsg('Sign-in failed. Please try
again.'). Locate the logic around setActiveUser, confirmApproved, router.replace
and setErrorMsg in the callback handler to stub/mock fetch responses and router
behavior and assert UI/state outcomes accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f812a96-3321-489b-b39b-a11c0377c535
📒 Files selected for processing (1)
frontend/src/app/signin/callback/page.tsx
- 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
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/app/signin/callback/page.tsx (1)
30-47:⚠️ Potential issue | 🟠 MajorDefer client auth mutation until the session POST succeeds.
On Line 31,
setActiveUser(...)runs before/api/auth/sessionis confirmed. If the request hits the failure paths on Line 43 or Line 46, the page shows an error but keeps local authenticated state (isAuthenticated/sapling_user) set.Proposed fix
- if (userId && name) { - setActiveUser(userId, name, avatar || ''); - fetch('/api/auth/session', { + if (userId && name) { + fetch('/api/auth/session', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ userId }), }).then(res => { if (res.ok) { + setActiveUser(userId, name, avatar || ''); confirmApproved(); router.replace('/dashboard'); } else if (res.status === 403) { router.replace('/pending'); } else { setErrorMsg('Unable to complete sign-in. Please try again.'); } }).catch(() => { setErrorMsg('Unable to reach the server. Please try again.'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/app/signin/callback/page.tsx` around lines 30 - 47, The code calls setActiveUser(userId, name, avatar) before confirming the server-side session; move the setActiveUser invocation so it only runs after fetch('/api/auth/session') returns res.ok (i.e., inside the success branch where confirmApproved() and router.replace('/dashboard') are called), and ensure no local auth state is set on the 403 or error branches (or explicitly clear/revert it in the catch/else branches) so isAuthenticated/sapling_user are only set when the POST succeeds; update references: setActiveUser, fetch('/api/auth/session'), confirmApproved, router.replace.
🧹 Nitpick comments (1)
frontend/src/__tests__/signinCallback.test.tsx (1)
1-12: Remove the top-of-file banner comment.Lines 1-12 restate what the test names already express and add maintenance noise.
As per coding guidelines: "No docstrings or comments unless the logic is non-obvious".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/__tests__/signinCallback.test.tsx` around lines 1 - 12, Remove the top-of-file banner comment block in the signinCallback.test.tsx test file (the multi-line comment that summarizes test cases at the very top); delete those redundant lines so only the tests and their descriptive test names remain, keeping the file focused and free of high-level docstring-style comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/__tests__/signinCallback.test.tsx`:
- Around line 89-106: Add a new test case mirroring the existing missing-param
tests that asserts the page shows an inline error when the name query parameter
is absent: use setParams to provide user_id and is_approved (e.g., setParams({
user_id: 'u1', is_approved: 'true' })), render the CallbackPage component, then
assert await screen.findByText(/sign-in failed/i) is in the document and that
global.fetch and replace were not called; place this alongside the other tests
for missing user_id and missing is_approved so the suite covers the missing-name
regression.
---
Outside diff comments:
In `@frontend/src/app/signin/callback/page.tsx`:
- Around line 30-47: The code calls setActiveUser(userId, name, avatar) before
confirming the server-side session; move the setActiveUser invocation so it only
runs after fetch('/api/auth/session') returns res.ok (i.e., inside the success
branch where confirmApproved() and router.replace('/dashboard') are called), and
ensure no local auth state is set on the 403 or error branches (or explicitly
clear/revert it in the catch/else branches) so isAuthenticated/sapling_user are
only set when the POST succeeds; update references: setActiveUser,
fetch('/api/auth/session'), confirmApproved, router.replace.
---
Nitpick comments:
In `@frontend/src/__tests__/signinCallback.test.tsx`:
- Around line 1-12: Remove the top-of-file banner comment block in the
signinCallback.test.tsx test file (the multi-line comment that summarizes test
cases at the very top); delete those redundant lines so only the tests and their
descriptive test names remain, keeping the file focused and free of high-level
docstring-style comments.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8845e65c-db53-48b2-a8ec-9681784ba32b
📒 Files selected for processing (2)
frontend/src/__tests__/signinCallback.test.tsxfrontend/src/app/signin/callback/page.tsx
Comment on lines
+89
to
+106
| it('shows an inline error when user_id is missing', async () => { | ||
| setParams({ name: 'Ada', is_approved: 'true' }); | ||
|
|
||
| render(<CallbackPage />); | ||
|
|
||
| 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(<CallbackPage />); | ||
|
|
||
| expect(await screen.findByText(/sign-in failed/i)).toBeInTheDocument(); | ||
| expect(replace).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
Add a regression test for missing name query param.
The callback logic requires both user_id and name, but this suite currently only asserts the missing-user_id branch. Please add a missing-name case to lock the behavior.
Based on learnings: "If you change how a feature works (not just its interface), update the tests that cover it."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/__tests__/signinCallback.test.tsx` around lines 89 - 106, Add a
new test case mirroring the existing missing-param tests that asserts the page
shows an inline error when the name query parameter is absent: use setParams to
provide user_id and is_approved (e.g., setParams({ user_id: 'u1', is_approved:
'true' })), render the CallbackPage component, then assert await
screen.findByText(/sign-in failed/i) is in the document and that global.fetch
and replace were not called; place this alongside the other tests for missing
user_id and missing is_approved so the suite covers the missing-name regression.
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
2 participants
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.
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
Description
Brief summary of what this PR does and why.
Changes Made
Related Issues
Closes #
Testing
Screenshots (if applicable)
Notes for Reviewers
Summary by CodeRabbit
Bug Fixes
Tests