fix: Refactor token refresh logic in GraphQL API handler - SFS-3104#3263
fix: Refactor token refresh logic in GraphQL API handler - SFS-3104#3263eduardoformiga wants to merge 2 commits intodevfrom
Conversation
- Simplified conditions for refreshing tokens by consolidating checks for expired tokens and the presence of refresh tokens. - Removed redundant checks to streamline the authentication flow.
eduardoformiga
requested review from
gabpaladino and
lariciamota
and removed request for
a team
WalkthroughConsolidated token-expiration/refresh condition in the GraphQL API and changed SDK session error handling to perform a logout and clear session state when refresh fails, replacing the previous behavior that postponed refresh attempts. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/pages/api/graphql.ts (1)
182-188: Remove the redundant refresh predicate.Line 183 never changes the outcome:
tokenExpiredalready impliesjwtexists, sotokenExpired && !refreshAfterExistis fully covered by Line 180's!!jwt && !refreshAfterExist. Also, perpackages/core/src/utils/getCookie.ts:26-38, a cleared auth cookie becomesjwt = null, so the "after browser clears the cookie" note is not accurate for this branch.♻️ Proposed simplification
- // token expired with no refreshAfter: covers jwt expired before first refresh and after browser clears the cookie - const tokenExpiredWithNoRefreshAfter = tokenExpired && !refreshAfterExist - const shouldRefreshToken = tokenExistAndIsFirstRefreshTokenRequest || - tokenExpiredWithNoRefreshAfter || refreshAfterExpiredIf you apply this,
tokenExpiredabove becomes unused and can be removed too.As per coding guidelines, "Prefer flat conditionals over nested ifs: combine conditions with && or use early returns to avoid deep nesting and improve readability."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/pages/api/graphql.ts` around lines 182 - 188, Remove the redundant tokenExpiredWithNoRefreshAfter predicate and any now-unused tokenExpired variable; simplify the shouldRefreshToken logic to rely on the existing jwt/refresAfter checks (use the existing tokenExistAndIsFirstRefreshTokenRequest or the !!jwt && !refreshAfterExist branch and refreshAfterExpired) so the condition no longer duplicates jwt presence via tokenExpired, and update usage of shouldRefreshToken accordingly (referencing tokenExpired, refreshAfterExist, tokenExistAndIsFirstRefreshTokenRequest, refreshAfterExpired, and jwt to locate and update the expressions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/core/src/pages/api/graphql.ts`:
- Around line 182-188: Remove the redundant tokenExpiredWithNoRefreshAfter
predicate and any now-unused tokenExpired variable; simplify the
shouldRefreshToken logic to rely on the existing jwt/refresAfter checks (use the
existing tokenExistAndIsFirstRefreshTokenRequest or the !!jwt &&
!refreshAfterExist branch and refreshAfterExpired) so the condition no longer
duplicates jwt presence via tokenExpired, and update usage of shouldRefreshToken
accordingly (referencing tokenExpired, refreshAfterExist,
tokenExistAndIsFirstRefreshTokenRequest, refreshAfterExpired, and jwt to locate
and update the expressions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 94c47872-d3ca-4d2f-a552-5c992cbece3e
📒 Files selected for processing (1)
packages/core/src/pages/api/graphql.ts
- Added logic to clear server-side cookies and reset the session to an anonymous state when refresh token fails. - Removed unnecessary session properties to prevent retry loops.
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 `@packages/core/src/sdk/session/index.ts`:
- Around line 138-147: The logout fetch call (fetch('/api/fs/logout')) inside
the refresh-token catch block should be wrapped in its own try-catch so network
errors don't propagate and prevent cleanup; call fetch('/api/fs/logout', {
method: 'POST' }) inside a try and swallow or log any error (e.g., using
console.error or processLogger) in the catch, then always call
sessionStore.set({ ...session, person: null, b2b: null, refreshAfter: null })
after the try-catch to guarantee the session is reset even if the logout request
fails.
- Around line 138-147: When refresh token handling in
packages/core/src/sdk/session/index.ts fails, don’t immediately clear the
session via the fetch('/api/fs/logout') + sessionStore.set({...person: null,
b2b: null}) path; instead align with useRefreshToken.ts retry behavior by
deferring logout — set session.refreshAfter to Date.now() + 3600_000 (or reuse
the same retry helper) and leave person and b2b intact so the hourly retry
window is honored; alternatively, if you prefer immediate logout, replace the
deferred retry in useRefreshToken.ts with the same logout path to keep behavior
consistent across both flows.
🪄 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: 75357123-0a25-4afa-8f4d-2b435b0df65b
📒 Files selected for processing (1)
packages/core/src/sdk/session/index.ts
| // Refresh token failed: clear server-side cookies (JWT + vid_rt) via logout | ||
| // and reset session to anonymous state to break the hourly retry loop | ||
| await fetch('/api/fs/logout', { method: 'POST' }) | ||
|
|
||
| sessionStore.set({ | ||
| ...session, | ||
| refreshAfter: String(Math.floor(Date.now() / 1000) + 1 * 60 * 60), // now + 1 hour | ||
| person: null, | ||
| b2b: null, | ||
| refreshAfter: null, | ||
| }) |
There was a problem hiding this comment.
Wrap the logout fetch in try-catch to prevent silent failures.
If the fetch throws (network error, etc.), the exception propagates and the session state won't be cleared. Since we're already inside a catch block, this needs its own error handling to ensure the session reset always executes.
🛡️ Proposed fix
} else {
// Refresh token failed: clear server-side cookies (JWT + vid_rt) via logout
// and reset session to anonymous state to break the hourly retry loop
- await fetch('/api/fs/logout', { method: 'POST' })
+ try {
+ await fetch('/api/fs/logout', { method: 'POST' })
+ } catch (logoutError) {
+ console.error('Failed to call logout endpoint:', logoutError)
+ }
sessionStore.set({
...session,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/sdk/session/index.ts` around lines 138 - 147, The logout
fetch call (fetch('/api/fs/logout')) inside the refresh-token catch block should
be wrapped in its own try-catch so network errors don't propagate and prevent
cleanup; call fetch('/api/fs/logout', { method: 'POST' }) inside a try and
swallow or log any error (e.g., using console.error or processLogger) in the
catch, then always call sessionStore.set({ ...session, person: null, b2b: null,
refreshAfter: null }) after the try-catch to guarantee the session is reset even
if the logout request fails.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if useRefreshToken.ts still contains the 1-hour deferral pattern
rg -n "refreshAfter.*1 \* 60 \* 60" --type tsRepository: vtex/faststore
Length of output: 349
🏁 Script executed:
# Read context around lines 138-147 in session/index.ts
cat -n packages/core/src/sdk/session/index.ts | sed -n '130,155p'Repository: vtex/faststore
Length of output: 949
🏁 Script executed:
# Read context around lines 40-54 in useRefreshToken.ts
cat -n packages/core/src/sdk/account/useRefreshToken.ts | sed -n '30,65p'Repository: vtex/faststore
Length of output: 1598
🏁 Script executed:
# Also check the beginning of both files to understand their purpose and exports
head -50 packages/core/src/sdk/session/index.tsRepository: vtex/faststore
Length of output: 1347
🏁 Script executed:
# Check useRefreshToken hook structure
head -50 packages/core/src/sdk/account/useRefreshToken.tsRepository: vtex/faststore
Length of output: 1865
🏁 Script executed:
# Get more context around the session validation to understand the flow
cat -n packages/core/src/sdk/session/index.ts | sed -n '100,150p'Repository: vtex/faststore
Length of output: 1849
🏁 Script executed:
# Check if useRefreshToken is called from session/index.ts
rg "useRefreshToken" packages/core/src/sdk/session/ --type tsRepository: vtex/faststore
Length of output: 40
🏁 Script executed:
# Check if there's any connection between these modules
rg "useRefreshToken|useRefreshToken" packages/core/src/sdk/ --type ts -lRepository: vtex/faststore
Length of output: 106
Behavioral inconsistency between session validation and client-side refresh handling.
This code clears the session immediately on refresh failure (lines 140-147), but useRefreshToken.ts (lines 43, 54) defers retries with a 1-hour window instead. Depending on which error path is triggered first (401 during validation vs. 403 on page access), users experience different session states. Align both approaches—either both should clear and log out, or both should defer—to avoid confusing behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/sdk/session/index.ts` around lines 138 - 147, When refresh
token handling in packages/core/src/sdk/session/index.ts fails, don’t
immediately clear the session via the fetch('/api/fs/logout') +
sessionStore.set({...person: null, b2b: null}) path; instead align with
useRefreshToken.ts retry behavior by deferring logout — set session.refreshAfter
to Date.now() + 3600_000 (or reuse the same retry helper) and leave person and
b2b intact so the hourly retry window is honored; alternatively, if you prefer
immediate logout, replace the deferred retry in useRefreshToken.ts with the same
logout path to keep behavior consistent across both flows.
1 participant
What's the purpose of this pull request?
How it works?
How to test it?
Starters Deploy Preview
References
Checklist
You may erase this after checking them all 😉
PR Title and Commit Messages
feat,fix,chore,docs,style,refactor,ciandtestPR Description
breaking change,bug,contributing,performance,documentation..Dependencies
pnpm-lock.yamlfile when there were changes to the packagesDocumentation
@Mariana-Caetanoto review and update (Or submit a doc request)Summary by CodeRabbit