fix: resolve session expiry by implementing automatic token refresh (#14)#88
fix: resolve session expiry by implementing automatic token refresh (#14)#88Arjita-2B-18 wants to merge 3 commits into
Conversation
|
@Arjita-2B-18 is attempting to deploy a commit to the karan3431's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
🎉 Thank you for your Pull Request! We're thrilled to have your contribution to FreshScan AI. Before we review, please make sure you have:
A maintainer will review your code as soon as possible! |
|
Caution Review failedPull request was closed or merged during review Warning
|
| Layer / File(s) | Summary |
|---|---|
Backend refresh endpoint and wiring backend/auth.py, backend/main.py |
New POST /api/v1/auth/refresh endpoint accepts a refresh_token and calls Supabase's auth.refresh_session() to obtain new tokens; returns tokens on success or raises HTTPException(401, ...) on invalid/expired tokens or other errors. Router is imported and registered with the FastAPI app after initialization. |
Frontend refresh token capture and storage src/pages/AuthPage.tsx |
OAuth callback handler extracts refresh_token from URL query parameters and persists it to localStorage under key fs_refresh_token before proceeding with the existing redirect flow. |
Frontend token refresh and request retry src/lib/api.ts |
Adds REFRESH_KEY, extends clearToken() to also remove the refresh token, implements tryRefreshToken() to POST the stored refresh token to the backend and persist returned tokens, centralizes handleResponse (including 5xx handling), and updates safeFetch() to attempt a one-time refresh on 401 and retry the original request; api.submitScan no longer accepts meta. |
Dependency / Merge Conflict
| Layer / File(s) | Summary |
|---|---|
package.json devDependencies and conflict markers package.json |
Bumps @types/node, updates @vitejs/plugin-react and vite versions and leaves unresolved merge-conflict markers around the vite/PWA dependency block (conflicting presence of vite-plugin-pwa/workbox-window). |
Sequence Diagram(s)
sequenceDiagram
participant Browser
participant Frontend
participant BackendRefreshEndpoint
participant SupabaseAuth
Browser->>Frontend: API request with access_token
Frontend->>BackendRefreshEndpoint: API request (expired) -> 401
Frontend->>BackendRefreshEndpoint: POST /api/v1/auth/refresh (fs_refresh_token)
BackendRefreshEndpoint->>SupabaseAuth: auth.refresh_session(refresh_token)
SupabaseAuth->>BackendRefreshEndpoint: new access_token + refresh_token
BackendRefreshEndpoint->>Frontend: {access_token, refresh_token}
Frontend->>Frontend: persist tokens, retry original request with new access_token
Frontend->>BackendRefreshEndpoint: retried API request (new access_token)
BackendRefreshEndpoint->>Frontend: success response
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested labels
Hard, size/level: hard
Suggested reviewers
- jpdevhub
Poem
🐰 I found a token, worn and thin,
I hop and swap to let you in.
One POST, one store, a retry spun,
A fresh new key — the session's won.
Hooray, stay logged in — hop on and run!
🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check | The PR contains unrelated changes to package.json with merge conflict markers and dependency version updates unrelated to token refresh functionality. | Resolve the merge conflict in package.json (<<<<<<< HEAD / ======= / >>>>>>>) and remove unrelated dependency updates, or justify these changes in the PR description. | |
| Docstring Coverage | Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly summarizes the main change: implementing automatic token refresh to fix session expiry. |
| Linked Issues check | ✅ Passed | All core requirements from issue #14 are addressed: backend refresh endpoint, frontend token persistence, 401 interception with refresh, and token cleanup on failure. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Warning
There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
🔧 Biome (2.4.16)
package.json
File contains syntax errors that prevent linting: Line 50: unexpected character <; Line 50: unexpected character <; Line 50: unexpected character <; Line 50: unexpected character <; Line 50: unexpected character <; Line 50: unexpected character <; Line 50: unexpected character <; Line 50: String values must be double quoted.; Line 51: End of file expected; Line 51: End of file expected; Line 51: End of file expected; Line 51: End of file expected; Line 52: End of file expected; Line 52: End of file expected; Line 52: End of file expected; Line 52: End of file expected; Line 53: End of file expected; Line 53: End of file expected; Line 53: End of file expected; Line 54: unexpected character =; Line 54: unexpected character =; Line 54: unexpected character =; Line 54: unexpected character =; Line 54: unexpected character =; Line 54: unexpected character =; Line 54: unexpected character =; Line 55: End of file expected; Line 55: End of file expected; Line 55: End of file expected; Line 56: unexpected character >; Line 56: unexpected character >; Line 56: unexpected character >; Line 56: unexpected character >; Line 56: unexpected character >; Line 56: unexpected character >; Line 56: unexpected character >; Line 56: End of file expected; Line 56: String values must be double quoted.; Line 56: unexpected character (; Line 56: String values must be double quoted.; Line 56: End of file expected; Line 56: unexpected character .; Line 56: unexpected character )
src/lib/api.ts
File contains syntax errors that prevent linting: Line 62: Expected a statement but instead found '<<<<<<< HEAD'.; Line 80: Expected a statement but instead found '=======
8f270f1 (fix: resolve all TypeScript errors in api.ts)'.; Line 81: numbers cannot be followed by identifiers directly after; Line 132: Expected a statement but instead found '<<<<<<< HEAD'.; Line 135: Illegal use of an export declaration not at the top level; Line 136: Expected a statement but instead found '======='.; Line 137: Illegal use of an export declaration not at the top level; Line 138: Expected a statement but instead found '>>>>>>> 8f270f1 (fix: resolve all TypeScript errors in api.ts)'.; Line 138: numbers cannot be followed by identifiers directly after; Line 139: Illegal use of an export declaration not at the top level; Line 140: Illegal use of an export declaration not at the top level; Line 141: Illegal use of an export declaration not at the top level; Line 143: Expected a statement but inst
... [truncated 1289 characters] ...
; Line 214: Expected a statement but instead found ',
getLatestScan: (): Promise =>
apiFetch('/ap; Line 223: expected : but instead found {; Line 223: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 228: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 230: expected : but instead found {; Line 230: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 235: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 235: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 235: Invalid regex flag; Line 235: unterminated template literal; Line 255: expected } but instead the file ends
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 help to get the list of available commands and usage tips.
![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
🧹 Nitpick comments (2)
backend/auth.py (1)
25-28: ⚡ Quick winReuse the existing Supabase client singleton instead of creating a new client per request.
The file already defines a
get_auth_client()singleton (lines 77-81) that should be reused here. Creating a new client on every refresh request adds unnecessary overhead.♻️ Proposed fix
+from auth import get_auth_client + `@router.post`("/refresh") async def refresh_session(body: RefreshRequest): """ Exchange a Supabase refresh_token for a new access_token + refresh_token. Called automatically by the frontend when a 401 Unauthorized error occurs. """ try: - # Create a Supabase client using environment variables - supabase = create_client( - os.environ["SUPABASE_URL"], - os.environ["SUPABASE_KEY"] - ) + # Use the existing singleton client + supabase = get_auth_client()Note: Since
get_auth_client()is defined later in the same file, you may need to reorganize the file to place the router code after the helper functions, or extract the client singleton to a shared module.🤖 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 `@backend/auth.py` around lines 25 - 28, Replace the per-request Supabase client creation in the refresh logic with the existing singleton accessor get_auth_client(): stop calling create_client(...) inside the request handler (the variable currently named supabase) and instead call get_auth_client() to obtain the shared client; if get_auth_client() is defined after the router, either move the router below the helper or move the singleton into a common module so the handler can import and reuse it. Ensure all references that used the local supabase variable now use the returned client from get_auth_client().src/pages/AuthPage.tsx (1)
34-36: 💤 Low valueConsider importing and using the shared constant for the refresh token key.
The key
'fs_refresh_token'is hardcoded here but defined asREFRESH_KEYinapi.ts. Using the shared constant would prevent key mismatches if renamed later.♻️ Proposed fix
In
api.ts, export the constant:export const REFRESH_KEY = 'fs_refresh_token';Then in
AuthPage.tsx:-import { api, setToken, isAuthenticated } from '../lib/api'; +import { api, setToken, isAuthenticated, REFRESH_KEY } from '../lib/api'; if (refreshToken) { - localStorage.setItem('fs_refresh_token', refreshToken); + localStorage.setItem(REFRESH_KEY, refreshToken); }🤖 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 `@src/pages/AuthPage.tsx` around lines 34 - 36, Replace the hardcoded 'fs_refresh_token' string in the AuthPage logic with the exported shared constant REFRESH_KEY from api.ts: export REFRESH_KEY from api.ts (if not already exported) and import REFRESH_KEY into AuthPage.tsx, then use REFRESH_KEY in the localStorage.setItem call inside the AuthPage component (the block that currently checks refreshToken and calls localStorage.setItem).
🤖 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 `@backend/auth.py`:
- Around line 46-48: The except block that currently does "except Exception as
e: raise HTTPException(status_code=401, detail=f'Token refresh failed:
{str(e)}')" should be changed to avoid leaking internal error text: replace the
HTTPException detail with a generic message like "Token refresh failed" or
"Unauthorized", and log the original exception internally using an application
logger (e.g., logger.exception or logger.error with the exception) before
raising HTTPException; keep the status_code=401 and continue to raise
HTTPException but do not include str(e) in the response.
In `@src/lib/api.ts`:
- Around line 69-82: The 401 branch in handleResponse is redundant and broken:
remove the entire "if (res.status === 401 && !retried) { ... }" block from
handleResponse so it no longer tries to refresh tokens or return the original
failed Response; instead let safeFetch continue to own 401 handling and retries
(keep tryRefreshToken/clearToken usage only where safeFetch implements them).
Ensure handleResponse only processes non-OK responses generically and that
callers still call safeFetch which handles the retry flow.
---
Nitpick comments:
In `@backend/auth.py`:
- Around line 25-28: Replace the per-request Supabase client creation in the
refresh logic with the existing singleton accessor get_auth_client(): stop
calling create_client(...) inside the request handler (the variable currently
named supabase) and instead call get_auth_client() to obtain the shared client;
if get_auth_client() is defined after the router, either move the router below
the helper or move the singleton into a common module so the handler can import
and reuse it. Ensure all references that used the local supabase variable now
use the returned client from get_auth_client().
In `@src/pages/AuthPage.tsx`:
- Around line 34-36: Replace the hardcoded 'fs_refresh_token' string in the
AuthPage logic with the exported shared constant REFRESH_KEY from api.ts: export
REFRESH_KEY from api.ts (if not already exported) and import REFRESH_KEY into
AuthPage.tsx, then use REFRESH_KEY in the localStorage.setItem call inside the
AuthPage component (the block that currently checks refreshToken and calls
localStorage.setItem).
🪄 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: 206f103c-4313-4cb1-bc34-ad2ddbe884f9
📒 Files selected for processing (4)
backend/auth.pybackend/main.pysrc/lib/api.tssrc/pages/AuthPage.tsx
| except Exception as e: | ||
| # Any other error means the refresh failed | ||
| raise HTTPException(status_code=401, detail=f"Token refresh failed: {str(e)}") |
There was a problem hiding this comment.
Avoid exposing internal error details in the response.
Including str(e) in the HTTP response could leak sensitive information about the backend (e.g., database connection strings, internal paths, or Supabase error internals). Return a generic message instead.
🛡️ Proposed fix
except Exception as e:
# Any other error means the refresh failed
- raise HTTPException(status_code=401, detail=f"Token refresh failed: {str(e)}")
+ print(f"Token refresh failed: {e}") # Log internally for debugging
+ raise HTTPException(status_code=401, detail="Token refresh failed")🤖 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 `@backend/auth.py` around lines 46 - 48, The except block that currently does
"except Exception as e: raise HTTPException(status_code=401, detail=f'Token
refresh failed: {str(e)}')" should be changed to avoid leaking internal error
text: replace the HTTPException detail with a generic message like "Token
refresh failed" or "Unauthorized", and log the original exception internally
using an application logger (e.g., logger.exception or logger.error with the
exception) before raising HTTPException; keep the status_code=401 and continue
to raise HTTPException but do not include str(e) in the response.
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)
src/lib/api.ts (1)
69-97:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Duplicate function definitions break the file.
The code contains two
handleResponsefunction declarations where the first one (line 69) is never closed before the second one (line 85) begins. This is a parse error that will prevent the module from loading.Additionally:
- Line 122 calls
handleResponse(retryRes, true)with two arguments, but the second definition (line 85) only accepts one parameter- The static analysis errors ("Illegal use of an export declaration not at the top level" at lines 149+) confirm that subsequent code is nested inside the unclosed first function
This appears to be an incomplete merge or edit where the old
handleResponse(lines 69-82) should have been removed. The past review flagged the 401 logic inhandleResponseas unreachable sincesafeFetchalready handles it.🐛 Proposed fix — remove the stale function fragment
// ── Shared Error Handling Logic ────────────────────────────────────────────── -// ── Shared Error Handling Logic ────────────────────────────────────────────── - -async function handleResponse(res: Response, retried = false): Promise<Response> { - if (res.ok) return res; - - // Handle 401 — try to refresh token and retry once - if (res.status === 401 && !retried) { - const refreshed = await tryRefreshToken(); - if (!refreshed) { - clearToken(); - window.location.href = '/auth'; - throw new Error('Session expired. Redirecting to login.'); - } - // Return a special signal to retry - return res; - } - - // Handle 5xx errors (Server Side) async function handleResponse(res: Response): Promise<Response> { if (res.ok) return res; + // 401 is handled by safeFetch before reaching here; this is a fallback + if (res.status === 401) { + clearToken(); + window.location.href = '/auth'; + throw new Error('Session expired. Redirecting to login.'); + } + if (res.status >= 500) { const msg = "Server error. Please try again later."; toast.error(msg);Also update the call site in
safeFetch:- return await handleResponse(retryRes, true); + return await handleResponse(retryRes);🤖 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 `@src/lib/api.ts` around lines 69 - 97, Remove the stale/duplicate handleResponse fragment that begins with "async function handleResponse(res: Response, retried = false)" so there is only one handleResponse implementation; ensure the remaining handleResponse signature matches its call sites (safeFetch should call handleResponse(retryRes) or update calls to pass the retried flag consistently), and remove any nested code that resulted from the unclosed function (verify tryRefreshToken, clearToken logic is handled in safeFetch and not duplicated here). Locate occurrences of handleResponse, safeFetch, tryRefreshToken, and clearToken in the file to remove the old fragment and reconcile the single canonical handleResponse implementation with existing callers.
🧹 Nitpick comments (1)
src/lib/api.ts (1)
196-217: 💤 Low value
scanOnlinebypasses 401 refresh logic.This function uses raw
fetch()instead ofsafeFetch(), so if the access token expires mid-session, users will receive an error instead of automatic token refresh and retry.If the intent is to keep this lightweight for the fallback path, consider at least handling 401 explicitly:
if (!res.ok) { + // Let caller handle auth expiry like other endpoints + if (res.status === 401) { + throw new Error('Unauthorized'); + } const err = await res.json().catch(() => ({ detail: res.statusText })); throw new Error((err as { detail?: string }).detail || `HTTP ${res.status}`); }Alternatively, if silent refresh is desired here too, switch to
safeFetch()and catch the redirect behavior.🤖 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 `@src/lib/api.ts` around lines 196 - 217, The scanOnline function currently calls fetch directly which bypasses the app's token-refresh logic; update scanOnline to use the existing safeFetch wrapper (or implement explicit 401 handling) so expired access tokens are refreshed and the request is retried: specifically, replace the direct fetch call inside scanOnline with a call to safeFetch(`${API_BASE}/api/v1/scan-auto`, { method: 'POST', headers: authHeaders(), body: form }) or, if you prefer minimal change, catch a 401 response from the fetch, invoke the token refresh routine and retry the same request once; ensure the error handling behavior (returning null on network TypeError and propagating non-401 server errors like NOT_A_FISH) is preserved.
🤖 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 `@src/lib/api.ts`:
- Around line 69-97: Remove the stale/duplicate handleResponse fragment that
begins with "async function handleResponse(res: Response, retried = false)" so
there is only one handleResponse implementation; ensure the remaining
handleResponse signature matches its call sites (safeFetch should call
handleResponse(retryRes) or update calls to pass the retried flag consistently),
and remove any nested code that resulted from the unclosed function (verify
tryRefreshToken, clearToken logic is handled in safeFetch and not duplicated
here). Locate occurrences of handleResponse, safeFetch, tryRefreshToken, and
clearToken in the file to remove the old fragment and reconcile the single
canonical handleResponse implementation with existing callers.
---
Nitpick comments:
In `@src/lib/api.ts`:
- Around line 196-217: The scanOnline function currently calls fetch directly
which bypasses the app's token-refresh logic; update scanOnline to use the
existing safeFetch wrapper (or implement explicit 401 handling) so expired
access tokens are refreshed and the request is retried: specifically, replace
the direct fetch call inside scanOnline with a call to
safeFetch(`${API_BASE}/api/v1/scan-auto`, { method: 'POST', headers:
authHeaders(), body: form }) or, if you prefer minimal change, catch a 401
response from the fetch, invoke the token refresh routine and retry the same
request once; ensure the error handling behavior (returning null on network
TypeError and propagating non-401 server errors like NOT_A_FISH) is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e46c221-4c17-4eda-b8ca-6c8f274849a7
📒 Files selected for processing (3)
backend/auth.pybackend/main.pysrc/lib/api.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/main.py
- backend/auth.py
jpdevhub
left a comment
jpdevhub
left a comment
There was a problem hiding this comment.
Their are some major issues in the code fix this with some suggestions.
- src/lib/api.ts - Duplicate Function Declaration (Syntax Error)
- backend/auth.py - Unsafe Environment Variable Access
Fix: They should change os.environ["SUPABASE_URL"] to os.environ.get("SUPABASE_URL") (and the same for the key), and explicitly raise an HTTPException(status_code=500) if they are None. - src/lib/api.ts - Conflicting Retry Logic
Fix: They should centralize the token refresh logic. Either safeFetch catches the 401 and retries the request, OR handleResponse throws a specific error that safeFetch catches to trigger the retry. Having it in both places will cause race conditions and duplicate API calls.
2 participants
Summary
Fixes #14 — users were being logged out after 1 hour due to missing refresh token logic.
Changes
backend/auth.py— addedPOST /api/v1/auth/refreshendpointbackend/main.py— registered auth routersrc/pages/AuthPage.tsx— now savesrefresh_tokento localStorage on loginsrc/lib/api.ts—safeFetchnow intercepts 401, refreshes token silently, retries request. On failure clears tokens and redirects to/authBehavior
Users stay logged in across long sessions. Token refresh is fully silent with no UI interruption.
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores