fix(refresh-token): add retry mechanism with back off to emit new AT#199
Merged
fix(refresh-token): add retry mechanism with back off to emit new AT#199
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the OAuth2 refresh-token flow by distinguishing transient network/server failures from genuine auth failures, adding retry with exponential backoff to reduce forced logouts during temporary outages.
Changes:
- Add
AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERRORand update refresh-token logic to treat network/5xx errors as retryable. - Introduce exponential backoff retry wrapper for refresh-token requests and prevent logout in Dropzone uploads on transient refresh failures.
- Add Jest unit tests for
refreshAccessTokenand add GitHub Actions workflows foryarn testandyarn build.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/components/security/methods.js |
Adds retry-with-backoff for refresh token and new network/server error handling. |
src/components/security/constants.js |
Adds a new constant to classify refresh-token network errors. |
src/components/security/__tests__/methods.test.js |
Adds unit tests for refreshAccessToken error classification. |
src/components/inputs/upload-input-v2/dropzone.js |
Avoids logging out on transient refresh-token network errors during upload setup. |
src/components/extra-questions/__test__/extra-questions.test.js |
Adds a jsdom scrollIntoView mock to stabilize tests. |
.github/workflows/jest.yml |
Adds CI job to run yarn test. |
.github/workflows/build_test.yml |
Adds CI job to run yarn build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…k, and response.ok handling * Fix retryWithBackoff exhaustion guard (attempt === maxRetries was dead code after loop bound change to < maxRetries; errors were silently swallowed, function returned undefined instead of throwing). * Fix Dropzone accept callback to call done() on error path, preventing files from getting stuck in an indeterminate state. * Use response.ok instead of checking individual status codes, closing a gap where 4xx codes other than 400 fell through unhandled. * Add ok property to existing test mocks to match the new response.ok check. * Export retryWithBackoff and add 5 unit tests covering retry count, auth error short-circuit, transient recovery, and exponential backoff. * Add defensive check for missing access_token in refresh response.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds GitHub Actions workflows for build and tests, introduces exponential backoff retry logic and new AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR constant, updates upload error handling to avoid unconditional logout, and adds/tests security retry/backoff behavior plus a jsdom test workaround. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/App
participant Retry as retryWithBackoff
participant TokenRefresh as refreshAccessToken
participant AuthServer as Auth Server
participant SessionMgr as Session Manager
Client->>Retry: request token refresh (refresh_token)
rect rgba(100,150,255,0.5)
Note over Retry: Attempt 1
Retry->>TokenRefresh: call refreshAccessToken(refresh_token)
TokenRefresh->>AuthServer: POST /token
AuthServer-->>TokenRefresh: Network error or 5xx/429
TokenRefresh-->>Retry: throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR
Retry->>Retry: wait (exponential backoff)
end
rect rgba(100,150,255,0.5)
Note over Retry: Attempt 2
Retry->>TokenRefresh: call refreshAccessToken(refresh_token)
TokenRefresh->>AuthServer: POST /token
AuthServer-->>TokenRefresh: 200 OK + access_token
TokenRefresh-->>Retry: return tokens
Retry-->>Client: success (tokens returned)
end
alt Fatal 4xx or invalid response
TokenRefresh-->>SessionMgr: clear session
SessionMgr-->>Client: logout flow triggered
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/jest.yml (1)
8-13: Upgrade GitHub Actions to v4.The
actions/checkout@v3andactions/setup-node@v3are outdated. GitHub now recommends v4 for these actions.♻️ Proposed fix
- - uses: actions/checkout@v3 + - uses: actions/checkout@v4 with: fetch-depth: 0 - - uses: actions/setup-node@v3 + - uses: actions/setup-node@v4 with: node-version: 18🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/jest.yml around lines 8 - 13, Update the workflow to use the v4 releases of the GitHub Actions; specifically replace the usages of actions/checkout@v3 and actions/setup-node@v3 with actions/checkout@v4 and actions/setup-node@v4 (keep the existing with: keys such as fetch-depth and node-version, e.g. node-version: 18 or node-version: '18.x' if you prefer the semver form) so the workflow uses the recommended v4 action versions..github/workflows/build_test.yml (1)
8-13: Upgrade GitHub Actions to v4.Same as the jest workflow, update the action versions to v4.
♻️ Proposed fix
- - uses: actions/checkout@v3 + - uses: actions/checkout@v4 with: fetch-depth: 0 - - uses: actions/setup-node@v3 + - uses: actions/setup-node@v4 with: node-version: 18🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_test.yml around lines 8 - 13, Update the GitHub Actions steps using actions/checkout and actions/setup-node to their v4 releases: replace the uses values "actions/checkout@v3" and "actions/setup-node@v3" with their "@v4" counterparts and keep the existing with: fetch-depth: 0 and with: node-version: 18 settings unchanged; ensure the steps referencing actions/checkout and actions/setup-node are the only changes so the workflow syntax and node-version setting remain intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/security/methods.js`:
- Around line 270-288: The retryWithBackoff function has an off-by-one in the
for loop (attempt runs 0..maxRetries inclusive) which conflicts with the throw
condition (attempt === maxRetries - 1) and leaves the final iteration
unreachable; fix by making the loop bound consistent — change the loop in
retryWithBackoff to iterate attempt < maxRetries (or alternatively adjust the
throw condition to match the inclusive bound) so that exactly maxRetries
attempts are made and the error is thrown when retries are exhausted, preserving
the existing delay/backoff logic and log behavior.
---
Nitpick comments:
In @.github/workflows/build_test.yml:
- Around line 8-13: Update the GitHub Actions steps using actions/checkout and
actions/setup-node to their v4 releases: replace the uses values
"actions/checkout@v3" and "actions/setup-node@v3" with their "@v4" counterparts
and keep the existing with: fetch-depth: 0 and with: node-version: 18 settings
unchanged; ensure the steps referencing actions/checkout and actions/setup-node
are the only changes so the workflow syntax and node-version setting remain
intact.
In @.github/workflows/jest.yml:
- Around line 8-13: Update the workflow to use the v4 releases of the GitHub
Actions; specifically replace the usages of actions/checkout@v3 and
actions/setup-node@v3 with actions/checkout@v4 and actions/setup-node@v4 (keep
the existing with: keys such as fetch-depth and node-version, e.g. node-version:
18 or node-version: '18.x' if you prefer the semver form) so the workflow uses
the recommended v4 action versions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71654997-b9d3-4aeb-a184-f3e87ce1f2ee
📒 Files selected for processing (7)
.github/workflows/build_test.yml.github/workflows/jest.ymlsrc/components/extra-questions/__test__/extra-questions.test.jssrc/components/inputs/upload-input-v2/dropzone.jssrc/components/security/__tests__/methods.test.jssrc/components/security/constants.jssrc/components/security/methods.js
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* consistent with the exhaustion guard attempt === maxRetries - 1. With MAX_RETRIES = 5, the function makes exactly 5 attempts (0 through 4), no dead code.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/components/inputs/upload-input-v2/dropzone.js:76
- In
accept, when the max-files limit is hit you calldone('Max files reached.')but then still fall through todone()a few lines later. Dropzone expectsdoneto be called exactly once; add areturnafter the errordone(...)(or convert to anelse) to avoid double-callback behavior.
if (options.maxFiles && options.maxFiles < (this.state.files.length + this.props.uploadCount)) {
done('Max files reached.');
}
done();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 `@src/components/security/methods.js`:
- Around line 438-446: The refreshAccessToken error branch currently treats 408
and 429 as auth failures and calls setSessionClearingState(true); change the
logic in the refreshAccessToken response handling so that status codes 408
(Request Timeout) and 429 (Too Many Requests) are treated as transient/network
errors (same branch as >=500) — do not call setSessionClearingState(true) for
those codes, instead throw the AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR with the
status and statusText; only call setSessionClearingState(true) and throw
AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR for genuine non-retryable auth errors.
- Around line 342-347: The code uses navigator?.locks which throws if navigator
is undeclared in SSR; replace the optional chaining guard with a typeof check
(e.g. typeof navigator !== 'undefined' && navigator.locks) before calling
navigator.locks.request (the block around GET_TOKEN_SILENTLY_LOCK_KEY that calls
_getAccessToken). Make the same change for the other similar block in this file
that uses navigator?.locks (the second navigator locks request later in the
file) so both runtime environments without a navigator don't throw.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f266324-9847-45c4-b57b-6d4ff07af26b
📒 Files selected for processing (1)
src/components/security/methods.js
smarcet
force-pushed
the
hotfix/refresh-token-retry-on-network-errors
branch
from
78d9bc1 to
a192553
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/security/methods.js (1)
342-347:⚠️ Potential issue | 🟠 MajorUse
typeof navigatorbefore touching Web Locks.
navigator?.locksstill throws whennavigatoris undeclared, so SSR / Node consumers fail before the fallback lock path runs. This same guard is still present in both access-token entry points.Suggested fix
- if (navigator?.locks) { + if (typeof navigator !== 'undefined' && navigator.locks) {Also applies to: 386-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/security/methods.js` around lines 342 - 347, The code accesses navigator?.locks which still throws when navigator is undeclared; update both access token entry points to guard with typeof navigator !== "undefined" before touching navigator.locks: replace checks like navigator?.locks with (typeof navigator !== "undefined" && navigator.locks) and wrap the navigator.locks.request call (used with GET_TOKEN_SILENTLY_LOCK_KEY and invoking navigator.locks.request to run _getAccessToken) inside that guard, and apply the same change to the other access-token entry point (the similar block around lines 386-391) so SSR/Node consumers hit the fallback path safely.
🧹 Nitpick comments (1)
src/components/security/__tests__/methods.test.js (1)
78-258: Add a regression test for a200 OKrefresh response withoutaccess_token.
refreshAccessToken()now clears session state and throwsAUTH_ERROR_REFRESH_TOKEN_REQUEST_ERRORwhen the body is missing the token, but this suite never exercises that branch. A targeted test here would lock in the new safeguard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/security/__tests__/methods.test.js` around lines 78 - 258, Add a regression test in the refreshAccessToken describe block that simulates a 200 OK response whose JSON body lacks access_token (mockResponse.ok = true, status = 200, json resolves to an object without access_token) and assert that refreshAccessToken('<some-refresh>') rejects with AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR and that setSessionClearingState(true) was called; reference the existing test helpers/global.fetch mock, the refreshAccessToken function, AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR constant, and setSessionClearingState to locate where to add this new it(...) case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/security/methods.js`:
- Around line 270-285: The retryWithBackoff helper currently retries any error
except AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR; change it to only retry when the
thrown error is explicitly the retryable network error by checking err.message
startsWith AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR and otherwise rethrow
immediately; update the catch in retryWithBackoff so that if err.message
startsWith AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR or does not startWith
AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR it throws the error, and only when it
matches AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR proceed with the backoff, delay
calculation, logging, and retry logic.
- Around line 422-436: The refresh flow currently calls fetch(...) with no
timeout; wrap that call in an AbortController: create const controller = new
AbortController(), pass signal: controller.signal to fetch, set a timeout (e.g.
const TIMEOUT_MS = 10000) that calls controller.abort() after the timeout, and
clear the timer when fetch resolves; in the catch, detect AbortError and throw a
distinct timeout error (e.g. AUTH_ERROR_REFRESH_TOKEN_TIMEOUT) so
retryWithBackoff() receives a failure it can retry; update the fetch(...) call
and its surrounding try/catch in methods.js (the refreshAccessToken logic)
accordingly.
---
Duplicate comments:
In `@src/components/security/methods.js`:
- Around line 342-347: The code accesses navigator?.locks which still throws
when navigator is undeclared; update both access token entry points to guard
with typeof navigator !== "undefined" before touching navigator.locks: replace
checks like navigator?.locks with (typeof navigator !== "undefined" &&
navigator.locks) and wrap the navigator.locks.request call (used with
GET_TOKEN_SILENTLY_LOCK_KEY and invoking navigator.locks.request to run
_getAccessToken) inside that guard, and apply the same change to the other
access-token entry point (the similar block around lines 386-391) so SSR/Node
consumers hit the fallback path safely.
---
Nitpick comments:
In `@src/components/security/__tests__/methods.test.js`:
- Around line 78-258: Add a regression test in the refreshAccessToken describe
block that simulates a 200 OK response whose JSON body lacks access_token
(mockResponse.ok = true, status = 200, json resolves to an object without
access_token) and assert that refreshAccessToken('<some-refresh>') rejects with
AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR and that setSessionClearingState(true)
was called; reference the existing test helpers/global.fetch mock, the
refreshAccessToken function, AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR constant,
and setSessionClearingState to locate where to add this new it(...) case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18d87db1-632c-49a4-8b92-38c6a48e84b3
📒 Files selected for processing (2)
src/components/security/__tests__/methods.test.jssrc/components/security/methods.js
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/components/inputs/upload-input-v2/dropzone.js:76
- In the Dropzone
acceptcallback,done('Max files reached.')is called but the function then continues and callsdone()again. Dropzone expects the callback to be invoked exactly once; calling it twice can lead to inconsistent acceptance/rejection behavior. Add an early return after the max-files rejection (or wrap the successdone()in anelse).
if (options.maxFiles && options.maxFiles < (this.state.files.length + this.props.uploadCount)) {
done('Max files reached.');
}
done();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
smarcet
force-pushed
the
hotfix/refresh-token-retry-on-network-errors
branch
from
a192553 to
79828c3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/components/security/methods.js (3)
422-435:⚠️ Potential issue | 🟠 MajorA hung refresh request bypasses the new retry mechanism.
The catch block assumes
fetch()rejects on timeout, but this call has no abort signal. A stalled token request can hang indefinitely and never produce the retryable error this PR is adding.🔧 Suggested fix
export const refreshAccessToken = async (refresh_token) => { @@ - let response; + let response; + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), 10000); try { response = await fetch(`${baseUrl}/oauth2/token`, { method: 'POST', headers: { 'Accept': 'application/json', 'Content-Type': 'application/json' }, - body: JSON.stringify(payload) + body: JSON.stringify(payload), + signal: controller.signal, }); } catch (networkError) { - // fetch rejects on network failures (DNS, timeout, no connectivity) - console.log('refreshAccessToken network error:', networkError.message); - throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${networkError.message}`); + const message = networkError?.name === 'AbortError' + ? 'refresh request timed out' + : networkError.message; + console.log('refreshAccessToken network error:', message); + throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${message}`); + } finally { + clearTimeout(timeoutId); }Does the browser `fetch()` API have a built-in timeout, or must `AbortController` be used to abort a stalled request so retry logic can run?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/security/methods.js` around lines 422 - 435, The fetch in refreshAccessToken lacks an AbortController and timeout, so a stalled request never rejects and bypasses retry logic; update refreshAccessToken to create an AbortController, pass controller.signal into fetch(`${baseUrl}/oauth2/token`, ...), start a timeout (e.g., setTimeout) that calls controller.abort() after the desired timeout, and clear the timeout when fetch resolves; catch abort errors and map them to AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR (or a new specific error) so the existing retry mechanism sees a rejection; ensure the controller and timeout cleanup happens in all code paths.
270-285:⚠️ Potential issue | 🟠 MajorOnly retry explicitly retryable refresh failures.
This still backs off on any exception other than
AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR, so unexpected runtime bugs fromfnget retried as if they were transient network errors. Restrict the retry path toAUTH_ERROR_REFRESH_TOKEN_NETWORK_ERRORonly; the new tests atsrc/components/security/__tests__/methods.test.jsLines 287-346 will need to throw that prefixed error as well.🔧 Suggested fix
export const retryWithBackoff = async (fn, maxRetries = MAX_RETRIES, baseDelayMs = BACKOFF_BASE_MS) => { for (let attempt = 0; attempt < maxRetries; attempt++) { try { return await fn(); } catch (err) { - // auth errors (HTTP 400) are fatal — never retry - if (err.message && err.message.startsWith(AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR)) { + const isRetryable = err?.message?.startsWith(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); + if (!isRetryable) { throw err; } - // network/5xx errors are retryable — retry unless exhausted if (attempt === maxRetries - 1) { throw err; } const delay = baseDelayMs * Math.pow(2, attempt); console.log(`retryWithBackoff retry ${attempt + 1}/${maxRetries} in ${delay}ms`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/security/methods.js` around lines 270 - 285, The retryWithBackoff function currently retries on any error except AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR; change it so only errors whose message starts with AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR are considered retryable (keep the existing fatal check for AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR and rethrow all other unexpected errors immediately), and only when the error matches the NETWORK error prefix proceed with the exponential backoff/attempt logic and eventual rethrow when attempts are exhausted; ensure tests that throw the AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR-prefixed error will trigger the retry path.
342-347:⚠️ Potential issue | 🟠 MajorUse a
typeof navigatorguard before touching Web Locks.
navigator?.locksstill throws whennavigatoris undeclared, so SSR/non-browser callers never reach thebrowser-tabs-lockfallback.🔧 Suggested fix
export const getAccessToken = async () => { - if (navigator?.locks) { + if (typeof navigator !== 'undefined' && navigator.locks) { return await navigator.locks.request(GET_TOKEN_SILENTLY_LOCK_KEY, async lock => { console.log(`openstack-uicore-foundation::Security::methods::getAccessToken web lock api`, lock); return await _getAccessToken(); }); @@ export const clearAccessToken = async () => { // see https://developer.mozilla.org/en-US/docs/Web/API/Web_Locks_API - if (navigator?.locks) { + if (typeof navigator !== 'undefined' && navigator.locks) { await navigator.locks.request(GET_TOKEN_SILENTLY_LOCK_KEY, async lock => { console.log(`openstack-uicore-foundation::Security::methods::clearAccessToken web lock api`, lock); _clearAccessToken(); });Does JavaScript optional chaining on an undeclared global like `navigator?.locks` throw a `ReferenceError`, and is `typeof navigator !== 'undefined' && navigator.locks` the correct SSR-safe guard?Also applies to: 386-391
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/security/methods.js` around lines 342 - 347, Replace the unsafe navigator?.locks checks with an SSR-safe guard using typeof navigator !== 'undefined' before accessing navigator.locks: where you currently call navigator?.locks (in the block using GET_TOKEN_SILENTLY_LOCK_KEY and the similar block at 386-391), change the condition to check typeof navigator !== 'undefined' && navigator.locks (or typeof navigator !== 'undefined' && navigator?.locks) so non-browser/SSR code paths fall through to the browser-tabs-lock fallback; keep using navigator.locks.request and the existing _getAccessToken, GET_TOKEN_SILENTLY_LOCK_KEY, and the browser-tabs-lock fallback logic otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/components/security/methods.js`:
- Around line 422-435: The fetch in refreshAccessToken lacks an AbortController
and timeout, so a stalled request never rejects and bypasses retry logic; update
refreshAccessToken to create an AbortController, pass controller.signal into
fetch(`${baseUrl}/oauth2/token`, ...), start a timeout (e.g., setTimeout) that
calls controller.abort() after the desired timeout, and clear the timeout when
fetch resolves; catch abort errors and map them to
AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR (or a new specific error) so the existing
retry mechanism sees a rejection; ensure the controller and timeout cleanup
happens in all code paths.
- Around line 270-285: The retryWithBackoff function currently retries on any
error except AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR; change it so only errors
whose message starts with AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR are considered
retryable (keep the existing fatal check for
AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR and rethrow all other unexpected errors
immediately), and only when the error matches the NETWORK error prefix proceed
with the exponential backoff/attempt logic and eventual rethrow when attempts
are exhausted; ensure tests that throw the
AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR-prefixed error will trigger the retry
path.
- Around line 342-347: Replace the unsafe navigator?.locks checks with an
SSR-safe guard using typeof navigator !== 'undefined' before accessing
navigator.locks: where you currently call navigator?.locks (in the block using
GET_TOKEN_SILENTLY_LOCK_KEY and the similar block at 386-391), change the
condition to check typeof navigator !== 'undefined' && navigator.locks (or
typeof navigator !== 'undefined' && navigator?.locks) so non-browser/SSR code
paths fall through to the browser-tabs-lock fallback; keep using
navigator.locks.request and the existing _getAccessToken,
GET_TOKEN_SILENTLY_LOCK_KEY, and the browser-tabs-lock fallback logic otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a4e1ecd-9567-4eca-9053-37a5a2eda488
📒 Files selected for processing (2)
src/components/security/__tests__/methods.test.jssrc/components/security/methods.js
smarcet
force-pushed
the
hotfix/refresh-token-retry-on-network-errors
branch
from
79828c3 to
dbdb8e0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+275
to
+282
| const abortError = new DOMException('The operation was aborted.', 'AbortError'); | ||
| global.fetch = jest.fn().mockRejectedValue(abortError); | ||
|
|
||
| await expect(refreshAccessToken('valid-token')) | ||
| .rejects | ||
| .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); | ||
|
|
||
| expect(setSessionClearingState).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
This test name says it verifies the request is aborted by the timeout, but fetch is mocked to immediately reject with an AbortError. That doesn't exercise the timeout path (the setTimeout(...controller.abort...)), so it can pass even if the timeout logic is broken. Consider either (a) renaming the test to reflect what it actually asserts (mapping an AbortError to AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR), or (b) actually testing the timeout by using fake timers + a fetch mock that stays pending until the signal is aborted and then advancing timers past REFRESH_TOKEN_FETCH_TIMEOUT_MS.
Suggested change
| const abortError = new DOMException('The operation was aborted.', 'AbortError'); | |
| global.fetch = jest.fn().mockRejectedValue(abortError); | |
| await expect(refreshAccessToken('valid-token')) | |
| .rejects | |
| .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); | |
| expect(setSessionClearingState).not.toHaveBeenCalled(); | |
| jest.useFakeTimers(); | |
| const abortError = new DOMException('The operation was aborted.', 'AbortError'); | |
| global.fetch = jest.fn().mockImplementation((_, options = {}) => { | |
| return new Promise((_, reject) => { | |
| const signal = options.signal; | |
| if (signal) { | |
| signal.addEventListener('abort', () => { | |
| reject(abortError); | |
| }); | |
| } | |
| }); | |
| }); | |
| const refreshPromise = refreshAccessToken('valid-token'); | |
| jest.runAllTimers(); | |
| await expect(refreshPromise) | |
| .rejects | |
| .toThrow(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR); | |
| expect(setSessionClearingState).not.toHaveBeenCalled(); | |
| jest.useRealTimers(); |
Comment on lines
+347
to
+368
| it('should apply exponential backoff delays', async () => { | ||
| jest.useRealTimers(); | ||
| const setTimeoutSpy = jest.spyOn(global, 'setTimeout'); | ||
|
|
||
| const fn = jest.fn() | ||
| .mockRejectedValueOnce(new Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: fail 1`)) | ||
| .mockRejectedValueOnce(new Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: fail 2`)) | ||
| .mockResolvedValue('ok'); | ||
|
|
||
| const baseDelay = 100; | ||
| const result = await retryWithBackoff(fn, 5, baseDelay); | ||
|
|
||
| expect(result).toBe('ok'); | ||
| expect(fn).toHaveBeenCalledTimes(3); | ||
|
|
||
| // Extract the delay arguments from setTimeout calls made by retryWithBackoff | ||
| const retryDelays = setTimeoutSpy.mock.calls | ||
| .map(call => call[1]) | ||
| .filter(delay => delay >= baseDelay); | ||
|
|
||
| // 100 * 2^0 = 100ms, 100 * 2^1 = 200ms | ||
| expect(retryDelays).toEqual([100, 200]); |
There was a problem hiding this comment.
These retryWithBackoff tests switch to real timers and end up sleeping for real backoff delays (eg 100ms + 200ms here). That makes the Jest suite slower and potentially flaky under load. Prefer using fake timers and advancing time (advanceTimersByTime) while awaiting the retry promise, instead of real waits.
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: 18 | ||
| - run: yarn install |
There was a problem hiding this comment.
CI currently runs yarn install without enforcing the checked-in yarn.lock. This can lead to non-reproducible installs (and unexpected lockfile changes) depending on the Yarn version and registry state. Consider using yarn install --frozen-lockfile (Yarn v1) so CI fails when the lockfile is out of sync.
Suggested change
| - run: yarn install | |
| - run: yarn install --frozen-lockfile |
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: 18 | ||
| - run: yarn install |
There was a problem hiding this comment.
CI currently runs yarn install without enforcing the checked-in yarn.lock. This can lead to non-reproducible installs (and unexpected lockfile changes) depending on the Yarn version and registry state. Consider using yarn install --frozen-lockfile (Yarn v1) so CI fails when the lockfile is out of sync.
Suggested change
| - run: yarn install | |
| - run: yarn install --frozen-lockfile |
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
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.
ref: https://app.clickup.com/t/86b8ub5fy
Summary by CodeRabbit
New Features
Bug Fixes
Tests