Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@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] |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dc81dff6b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (isLoadingApp) { | ||
| const reasonAttributes: SkeletonSpanReasonAttributes = {context: 'WalletPage', isLoadingApp: !!isLoadingApp}; | ||
| if (isAppLoadPending) { |
There was a problem hiding this comment.
When another write follows OpenApp in the sequential queue, this becomes false before the app data is available: SequentialQueue.process() removes the completed request at src/libs/Network/SequentialQueue.ts:199-203, while write response/finally updates are deliberately held until the entire queue is empty and flushed at lines 424-427. This occurs, for example, when clearOnyxAndResetApp() enqueues OpenApp and then restores previously queued writes; a slow or retrying restored write can therefore make Wallet and the other migrated screens render cleared/stale Onyx data for an extended period, whereas IS_LOADING_APP remained true until that flush applied its finallyData.
Useful? React with 👍 / 👎.
|
No product review needed. |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
This PR adds a new |
|
let me validate or prepare other approach |
Co-authored-by: Cursor <cursoragent@cursor.com>
|
I removed it |
|
|
||
| // HAS_LOADED_APP is committed only after deferred OpenApp updates flush. Combining both existing | ||
| // markers covers that gap without trusting an IS_LOADING_APP value stranded after a completed load. | ||
| return hasPendingOpenApp || (isLoadingApp === true && hasLoadedApp !== true); |
There was a problem hiding this comment.
@BartekObudzinski I understand this gap. My question is whether we actually need to handle it at all. Could we drop the IS_LOADING_APP/HAS_LOADED_APP fallback and rely on hasPendingOpenApp only? Do you see any problem with that?
There was a problem hiding this comment.
Honestly, I'd like to remove isLoadingApp completely
There was a problem hiding this comment.
We can't drop it fully yet, but the reliance is minimal and self limiting.
hasPendingOpenApp goes false the moment OpenApp leaves the queue, which is before its finallyData flushes (the queue holds those writes until it drains). In that window the app data and HAS_LOADED_APP are not committed yet, so a pure hasPendingOpenApp gate would flash cleared or stale data on the migrated screens. That is the P1 Codex flagged.
The isLoadingApp === true && hasLoadedApp !== true term exists only to cover that first load flush window, and it is self healing for the stranded flag case: HAS_LOADED_APP is a permanent latch set only after the flush, so once the app has loaded it stays true and the term is false regardless of a stranded IS_LOADING_APP. A later reload runs ReconnectApp, not OpenApp, so nothing is left pending to strand.
Removing isLoadingApp completely is exactly the goal of the dedicated cleanup PR that deletes the flag and its writers. It can't come out here because these screens still need the flush window cover and other consumers still read it. If you would rather not lean on the markers, we can pair the removal with a different flush signal in that PR.
There was a problem hiding this comment.
Correction to my note above: the marker was not fully self healing. An account switch preserves HAS_LOADED_APP true while firing a real OpenApp, and its data plus the IS_LOADING_APP clear are deferred to the queue drain, so gating on HAS_LOADED_APP dropped the skeleton mid flush (the P2 Codex flagged). I replaced the marker with a process session latch keyed on an observed OpenApp instead, which covers that window and still ignores a stranded flag on reload, since a reload runs ReconnectApp not OpenApp so the latch is never set.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-22.at.15.12.58.movAndroid: mWeb ChromeScreen.Recording.2026-07-22.at.15.10.48.moviOS: HybridAppScreen.Recording.2026-07-22.at.15.12.35.moviOS: mWeb SafariScreen.Recording.2026-07-22.at.15.09.57.movMacOS: Chrome / SafariScreen.Recording.2026-07-22.at.15.07.19.mov |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60ef1cc863
ℹ️ 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".
|
|
||
| // HAS_LOADED_APP is committed only after deferred OpenApp updates flush. Combining both existing | ||
| // markers covers that gap without trusting an IS_LOADING_APP value stranded after a completed load. | ||
| return hasPendingOpenApp || (isLoadingApp === true && hasLoadedApp !== true); |
There was a problem hiding this comment.
When HAS_LOADED_APP is already true (for example, delegate transitions preserve it while seeding IS_LOADING_APP=true before calling openApp()), this expression stops reporting pending as soon as the OpenApp leaves PERSISTED_ONGOING_REQUESTS. The sequential queue clears the ongoing request before it flushes the queued OpenApp Onyx updates and finallyData, so the newly migrated screens can render against cleared or stale account data instead of the loading skeleton during that gap; keep the post-request IS_LOADING_APP gate for OpenApp instances that were actually observed rather than keying it only on HAS_LOADED_APP.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think that's @BartekObudzinski's intention now as explained above
|
Let me check the latest update again |
|
thanks there i put and update #96394 (comment) |
|
@BartekObudzinski The thing I'd actually want to know before endorsing all of this: how visible is that flash, really? If it's only 1–2 frames and only on cold start, a simpler solution might be acceptable, and this cleverness would be buying us very little |
|
The current implementation does ensure the app works well, but it feels a bit over-engineered |
|
@DylanDylann Thanks, I verified both cases directly. During normal account switches, I did not observe stale data appearing. I also tested a delayed restored write with OpenApp followed by AddComment. After OpenApp was removed, the current latch correctly stayed true while AddComment remained. Using only hasPendingOpenApp returned false too early. The simpler approach is not equivalent and can remove the skeleton before deferred OpenApp data is flushed. I think we should keep the current implementation. |
|
I'll be waiting for the cleanup PR to simplify it if possible 😄 |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Are there no docs that need to be updated?
|
🚧 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.43-0 🚀
|
|
🤖 I reviewed the changes in this PR against the help site content under No help site changes are required, so I did not create a draft PR. Why: This PR is a purely internal refactor. It migrates several loader consumers from the stored Expensify's help articles document user-facing product features and workflows. Loading skeletons/spinners and their internal trigger mechanism are not documented there, so nothing needs to be updated. @BartekObudzinski — no linked help site PR was created since no docs changes are needed. If you believe a specific article should be updated as a result of this change, let me know which one and I'll take another look. |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.4.43-1 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
Several full-page and skeleton loaders decide whether the app is still doing its initial load by reading the stored
IS_LOADING_APPOnyx flag. That flag is a shadow of the request queue: it is set optimistically when the initialOpenApprequest is enqueued and cleared when the request leaves the queue, but a reload or dropped response in between can strand it, leaving the loader up forever with no request running.This change moves those consumers onto the queue-derived
useIsAppLoadPending()hook, which reads directly whether anOpenApprequest is currently in the SequentialQueue. The flag was already set and cleared atomically with that request, so healthy-path behavior is identical — the only difference is that a stranded flag can no longer pin a loader.Scope is the consumers that read the flag as a plain loading gate. Consumers with different semantics (a
true/falsedefault that treats "unknown" as loading, completion checks, latches, or non-loader logic) are intentionally left on the flag. The flag, its writers, and the recovery watchdog are untouched — their removal is a separate change. TelemetryreasonAttributeskeep their existingisLoadingAppkey name (now sourced from the hook) for continuity in Sentry.Migrated: WorkspaceSelector, DomainsListPage, DynamicNewReportWorkspaceSelectionPage, DynamicReportChangeWorkspacePage, SetDefaultWorkspacePage, ReportActions, MoneyRequestReportView, useReportActionsListModel, ProfilePage, WalletPage.
Fixed Issues
$ #96487
PROPOSAL:
Tests
OpenAppcompletes (no stuck loader).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