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 4 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! |
|
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 constant and extends clearToken() to also remove the refresh token; implements tryRefreshToken() to POST the stored refresh token to the backend and persist returned tokens; updates handleResponse() to toast on 5xx errors (unless silent) and parse JSON detail with HTTP status fallback; modifies safeFetch() to intercept 401 responses, attempt a one-time token refresh, and retry the original request with updated headers, or clear tokens and redirect to login on failure. apiFetch() is updated to call safeFetch() with combined URL and merged headers. Response envelope types and API endpoint method wiring are reformatted. |
Dependency / Merge Conflict
| Layer / File(s) | Summary |
|---|---|
package.json devDependencies and conflict markers package.json |
Bumps @types/node to 25.9.3, updates @vitejs/plugin-react to ^6.0.2, and changes vite from ^8.0.1 to ^8.0.16; unresolved merge-conflict markers remain around the vite/PWA dependency block (conflicting presence of vite-plugin-pwa/workbox-window). |
Sequence Diagram(s)
sequenceDiagram
participant Browser
participant Frontend as Frontend/safeFetch
participant RefreshEndpoint as Backend Refresh
participant SupabaseAuth
Browser->>Frontend: API request with access_token
Frontend->>RefreshEndpoint: API request (expired access_token) -> 401
Frontend->>RefreshEndpoint: POST /api/v1/auth/refresh (fs_refresh_token)
RefreshEndpoint->>SupabaseAuth: auth.refresh_session(refresh_token)
SupabaseAuth->>RefreshEndpoint: new access_token + refresh_token
RefreshEndpoint->>Frontend: {access_token, refresh_token}
Frontend->>Frontend: persist tokens to localStorage
Frontend->>RefreshEndpoint: retry original API request (new access_token)
RefreshEndpoint->>Frontend: success response
Frontend->>Browser: response to caller
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
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 | ✅ 4 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check | The pull request includes out-of-scope changes: package.json has unmerged merge conflict markers and dependency updates unrelated to the token refresh implementation. | Resolve the merge conflict in package.json (remove conflict markers and select appropriate branch version) and remove unrelated dependency version bumps, or explain their necessity for token refresh functionality. | |
| Docstring Coverage | Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately summarizes the main change: implementing automatic token refresh to resolve session expiry issues, matching the core objective of issue #14. |
| Linked Issues check | ✅ Passed | All requirements from issue #14 are met: refresh token captured/persisted [AuthPage.tsx], backend refresh endpoint implemented [auth.py/main.py], frontend intercepts 401 and retries [api.ts], and automatic silent refresh is enabled. |
| Description check | ✅ Passed | The PR description clearly explains the bug fix, implementation approach (backend and frontend changes), and expected behavior with automatic silent token refresh. |
✏️ 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)
src/lib/api.ts
File contains syntax errors that prevent linting: Line 38: expected , but instead found $; Line 38: expected , but instead found {; Line 38: expected : but instead found }; Line 38: Expected a statement but instead found ':'.; Line 39: Expected a statement but instead found '}
async'.; Line 45: expected , but instead found {; Line 45: await is only allowed within async functions and at the top levels of modules.; Line 51: await is only allowed within async functions and at the top levels of modules.; Line 79: expected , but instead found $; Line 79: expected , but instead found {; Line 79: expected , but instead found .; Line 121: expected , but instead found {; Line 121: expected , but instead found $; Line 121: expected , but instead found {; Line 171: Expected a semicolon or an implicit semicolon after a statement, but found none; Line 171: Invalid regex flag; Line 191: expected , but instead found {; Line 206: expected , but instead found {; Line 214: expected , but instead found $; Line 214: expected , but instead found {; Line 214: expected , but instead found .; Line 230: Invalid regex flag; Line 230: expected , but instead found {; Line 234: Invalid regex flag; Line 234: expected : but instead found {; Line 234: expected , but instead found {; Line 240: expected , but instead found {; Line 257: Invalid regex flag; Line 257: expected : but instead found {; Line 257: expected , but instead found {; Line 257: expected , but instead found {
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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/api.ts (1)
41-59: ⚡ Quick winConsider guarding against concurrent refresh attempts.
If multiple API requests fail with 401 simultaneously (e.g., on page load with parallel fetches), each will independently call
tryRefreshToken(). This can cause multiple refresh requests racing against each other, potentially leading to one succeeding while others fail (invalidating the first's tokens) or wasted network calls.A common pattern is to use a module-level promise lock so only the first 401 triggers a refresh, and subsequent callers await the same promise.
♻️ Proposed fix with refresh lock
+let refreshPromise: Promise<boolean> | null = null; + async function tryRefreshToken(): Promise<boolean> { + // If a refresh is already in progress, wait for it + if (refreshPromise) return refreshPromise; + const refreshToken = localStorage.getItem(REFRESH_KEY); if (!refreshToken) return false; + + refreshPromise = (async () => { try { const res = await fetch(`${API_BASE}/api/v1/auth/refresh`, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ refresh_token: refreshToken }), }); if (!res.ok) return false; const data = await res.json(); localStorage.setItem(TOKEN_KEY, data.access_token); localStorage.setItem(REFRESH_KEY, data.refresh_token); window.dispatchEvent(new Event("auth-change")); return true; } catch { return false; + } finally { + refreshPromise = null; } + })(); + + return refreshPromise; }🤖 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 41 - 59, The tryRefreshToken() function can be called multiple times concurrently when several requests fail with 401 simultaneously, causing race conditions and wasted network calls. Add a module-level promise variable (outside the function) to track an ongoing refresh attempt. Inside tryRefreshToken(), check if this promise exists; if it does, await and return its result instead of making a new refresh request. When starting a fresh token refresh, store the fetch promise in the module-level variable, clear it after completion (whether success or failure), and ensure all concurrent callers receive the same refresh outcome by waiting for the shared promise.
🤖 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.
Nitpick comments:
In `@src/lib/api.ts`:
- Around line 41-59: The tryRefreshToken() function can be called multiple times
concurrently when several requests fail with 401 simultaneously, causing race
conditions and wasted network calls. Add a module-level promise variable
(outside the function) to track an ongoing refresh attempt. Inside
tryRefreshToken(), check if this promise exists; if it does, await and return
its result instead of making a new refresh request. When starting a fresh token
refresh, store the fetch promise in the module-level variable, clear it after
completion (whether success or failure), and ensure all concurrent callers
receive the same refresh outcome by waiting for the shared promise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d57e61c3-00e5-476b-aa4f-1b652089500a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
backend/main.pysrc/lib/api.tssrc/pages/AuthPage.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/AuthPage.tsx
|
Both the test is failing fix this shortly
|
|
Are you doing the changes or fed up with the review and all ? i can fix it if you feel its over from your side! |
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