refactor: migrate WorkspacesListPage loading gate to queue-derived truth#96300
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@DylanDylann 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: 857b29ea3b
ℹ️ 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".
…he full-page loader
|
The failing jobs are not mine, its already on main fyi @DylanDylann It requires margin main |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
| * the queue truth matches the old flag before any PR deletes the flag. Logs once per disagreement transition | ||
| * (not every render) so a persistent mismatch is a single line, not a flood. | ||
| */ | ||
| function useIsLoadingAppShadowLog() { |
There was a problem hiding this comment.
The old flag was cleared through finallyData. For write commands, that doesn't run until the entire sequential queue has flushed. The new queue-derived value flips to false as soon as OpenApp leaves the queue, it doesn't wait for anything queued behind it.
So when other writes are sitting behind OpenApp, the new code clears the workspaces loader a bit earlier than the old flag did.
This is the intended benefit of the migration, not a bug. But it looks like the shadow log can't distinguish this case and will still report it as a false positive
Screen.Recording.2026-07-16.at.23.57.41.mov
There was a problem hiding this comment.
Good point, the false positive is real: the queue truth clears when OpenApp leaves the queue while the old flag waits for the full flush, so a plain disagreement log can't tell that benign earlier-clear apart from an actual stranded flag. Rather than ship a noisy diagnostic, I removed the shadow log from this PR. We'll bring it back debounced (only report a disagreement that persists past a threshold, which a real strand does and the transient does not) at the point where it actually matters, the PR that deletes IS_LOADING_APP.
| // Temporary safety net for the migration off IS_LOADING_APP: logs when the stored flag and the queue truth disagree. | ||
| useIsLoadingAppShadowLog(); |
There was a problem hiding this comment.
I'd prefer to move this into useIsAppLoadPending
There was a problem hiding this comment.
Dropped the shadow log from this PR entirely, so this is moot for now. When it comes back at the flag-deletion step we'll decide placement then; I'd lean toward keeping it out of the generic useIsAppLoadPending so the permanent queue hook doesn't carry a temporary IS_LOADING_APP-specific diagnostic, but happy to discuss when we get there.
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-07-17.at.00.01.03.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Explanation of Change
A loading indicator driven by a stored boolean can get stuck on screen if the write that clears the flag is ever lost. This moves the Workspaces list page off the stored
IS_LOADING_APPflag and ontouseIsAppLoadPending, which derives the loading state from the in flight request queue instead. A request always leaves the queue once it resolves, so this state cannot get stuck the way a stored flag can. The hook matches OpenApp only, the same scope as the old flag, so background reconnects do not change this loader. Behavior is unchanged today: both the old flag and the new hook feed the same check.Fixed Issues
$ #96302
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests
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
Screen.Recording.2026-07-16.at.15.11.11.mov