fix: do not save queueFlushedData on failed responses#96041
Conversation
|
@cretadn22 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] |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Reviewer Checklist
Screenshots/VideosOpenApp return 200 Screen.Recording.2026-07-15.at.00.51.38.movOpenApp return 400 Screen.Recording.2026-07-15.at.00.50.21.mov |
|
@BartekObudzinski mind updating this branch with the latest from main? |
| // Only commit queueFlushedData (e.g. HAS_LOADED_APP: true) on success — HttpUtils resolves (not rejects) app-level | ||
| // failures, so committing on a failed-but-resolved OpenApp/ReconnectApp would wrongly mark the app as loaded and | ||
| // break self-healing on the next boot. | ||
| if (requestToProcess.queueFlushedData && response?.jsonCode === CONST.JSON_CODE.SUCCESS) { |
There was a problem hiding this comment.
response?.jsonCode has type number | string. While the backend usually returns a number, using strict equality (===) could fail if "200" is ever returned as a string. Should we normalize it with Number(response?.jsonCode) === CONST.JSON_CODE.SUCCESS, or confirm whether the backend API contract strictly guarantees a number here?
There was a problem hiding this comment.
Intentional, it matches the successData gate in OnyxUpdates.ts so both commit under the same condition. Normalizing one side only could make them diverge
…hed-data-on-jsoncode
|
@cretadn22 main sync ready to review |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.36-0 🚀
|
|
🤖 I reviewed the changes in this PR against Expensify's help site content under No help site changes are required. This is a purely internal networking fix: it gates the commit of Files reviewed
None of these correspond to any customer-facing help article. No draft PR was created since no documentation update is needed. @BartekObudzinski, since no help site changes are required, no draft PR was created. If you believe a customer-facing behavior did change and warrants a docs update, let me know and I'll create the PR. |
|
@BartekObudzinski @mountiny
|
|
Since this requires dev tools/proxy interception to verify, I think we should mark this PR as 'No QA' |
Explanation of Change
SequentialQueue'sprocess()function commitsqueueFlushedData(this is howHAS_LOADED_APPgets written to Onyx) whenever a queued request resolves, regardless of the response'sjsonCode.HttpUtils.xhrresolves application-level failures instead of rejecting them, so a failed-but-resolvedOpenApporReconnectApprequest still committedHAS_LOADED_APP: true. Once that happens, the next app boot runsReconnectApponly, and the app can no longer retry a fullOpenAppto recover from a stuck loading state.This change only commits
queueFlushedDatawhen the response'sjsonCodeisCONST.JSON_CODE.SUCCESS, so a failed request no longer marks the app as loaded.Fixed Issues
$ #96059
PROPOSAL:
Tests
jsonCode: 400while still returning HTTP 200. Inspect Onyx state and verifyhasLoadedAppis NOT set totrue, so the next boot retries a full OpenApp instead of being stuck in ReconnectApp-only mode.jsonCode: 200and verifyhasLoadedAppcorrectly becomestrue.Offline tests
N/A — the gate only runs when a response arrives; offline requests stay queued and produce no response.
QA Steps
Same as tests (step 2 requires dev tools or a proxy to intercept the OpenApp response)
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari