Repository navigation
Conversation
…ation middleware When reauthentication returns false (redirecting to sign-in), the middleware was returning undefined without clearing failureData/finallyData from the request, causing SaveResponseInOnyx to still apply them. This made callers incorrectly see shouldFailAllRequests or shouldForceOffline set to true after a sign-in redirect. Also handle HTTP errors on the Authenticate command (e.g. 403 Forbidden) by resolving the request with the actual HTTP status code and message rather than the generic unableToRetry code, allowing callers to display the correct error to the user. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@Krishna2323 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c534586940
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (request.command === 'Authenticate' && error instanceof HttpsError) { | ||
| if (request.resolve) { | ||
| request.resolve({jsonCode: Number(error.status), message: error.message, title: error.title}); |
There was a problem hiding this comment.
Preserve retries for transient Authenticate HTTP errors
When the Authenticate request hits a transient HTTP HttpsError such as 500/502/504/520, 429, or the forced-offline Failed to fetch path with an empty status, this branch now resolves with that status instead of UNABLE_TO_RETRY. The reauthentication caller only retries when it sees UNABLE_TO_RETRY; any other non-200 response is treated as an authentication failure and redirects the user to sign-in, so a temporary Auth/site outage during token refresh can incorrectly log users out rather than using the existing retry throttle.
Useful? React with 👍 / 👎.
|
Closing: the identical fix was already merged to main via PR #91619 (fix: sign out user when session expires). Local main branch was stale. |
Explanation of Change
When
reauthenticate()returnsfalse(the user is being redirected to sign-in), the middleware was returningundefinedwithout clearingfailureData/finallyDatafrom the request. BecauseSaveResponseInOnyxonly skips Onyx writes when all ofsuccessData,failureData, andfinallyDataare absent, those values were still being applied — causing callers to incorrectly seeshouldFailAllRequestsorshouldForceOfflineset totrueafter a sign-in redirect.Two fixes:
request.failureDataandrequest.finallyDatathen return the original response data so callers get the correctjsonCodeandSaveResponseInOnyxdoes not apply stale failure values.Authenticatecommand (e.g. 403 Forbidden) now resolve with the actual status code and message instead of the genericunableToRetry, so the caller can surface the correct error.Fixed Issues
$ #94292
PROPOSAL:
Tests
Unit tests added in
tests/unit/ReauthenticationMiddlewareTest.tscovering all four scenarios:failureDataandfinallyDataand returns original responseAuthenticateHTTP errors resolve with the HTTP status/messageOffline tests
N/A — middleware logic is network-state aware; offline 407 path is already tested.
QA Steps
No visible UI change. Unit tests cover the regression path.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)Screenshots/Videos
N/A — middleware-only change with no UI
Made with Cursor