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] |
![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: f54be55f46
ℹ️ 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".
|
PR doesn’t need product input as a perf refactor PR. Unassigning and unsubscribing myself. |
Co-authored-by: Cursor <cursoragent@cursor.com>
…-migrate-consumers # Conflicts: # src/components/MoneyRequestReportView/MoneyRequestReportView.tsx # src/hooks/useInFlightRequests.ts
|
@DylanDylann Are you able to complete review today? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-23.at.16.10.43.movAndroid: mWeb ChromeScreen.Recording.2026-07-23.at.16.07.54.moviOS: HybridAppiOS: mWeb SafariScreen.Recording.2026-07-23.at.16.07.07.movMacOS: Chrome / SafariScreen.Recording.2026-07-23.at.16.08.52.mov |
| // The matching OpenReport request starts this lifecycle, and its terminal loading update ends it after | ||
| // deferred response data is applied. `reportLoadingState` is still read for the transaction-wait check below. |
There was a problem hiding this comment.
Let's move this comment into the hook to avoid duplication
| // The queue arms this lifecycle, so a loading flag stranded by a previous process cannot gate by itself. | ||
| // Once armed, the terminal flag keeps consumers gated until deferred response updates are flushed. | ||
| const [trackedReportID, setTrackedReportID] = useState(reportID); | ||
| const [hasObservedPendingRequest, setHasObservedPendingRequest] = useState(false); |
There was a problem hiding this comment.
It would be better if we ensured that the value returned from this hook is consistent across all consumers with the same reportID
There was a problem hiding this comment.
Let's consider the case where two components mount at different times
Step 1: Request enters the queue. ReportFooter is on screen
hasPendingRequest is true and hasObservedPendingRequest is true -> useIsReportLoadPending returns true ✅
Step 2. Request leaves the queue. Data not written yet. isLoadingInitialReportActions is still true
Step 3. NOW the Header component appears on screen for the first time
hasPendingRequest = false (request already gone in Step 2)
-> setHasObservedPendingRequest(true) is not triggered
Both components compute their return value:
ReportFooter:
false || (true && true && true) = true → shows skeleton ✅
Header:
false || (true && false && true) = false → shows real content ❌ (data not ready)
There was a problem hiding this comment.
Good point. I moved the latch to shared process session state keyed by reportID, so all consumers return the same value even when one mounts after OpenReport leaves the queue. I also added a regression test covering this exact sequence. Both consumers remain pending until the terminal loading update clears the flag.
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
@BartekObudzinski Looks good, but lets "humanize" the comments first a bit please
|
✋ 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.45-0 🚀
|
|
🤖 I reviewed the changes in this PR against the help site articles under Why: This is a purely internal engineering refactor. It changes how the report-load loading skeleton is gated — moving three screen-level consumers ( Since there is no product-behavior or terminology change for users, there is nothing to add or update in the customer-facing docs, so I did not open a draft docs PR. @BartekObudzinski, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR (Note: no help site PR was created because no documentation changes are warranted for this internal refactor. If you believe a user-facing behavior did change and should be documented, reply with the specifics and I'll draft the docs update.) |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.45-14 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
The initial report-load skeleton on a few report surfaces reads the stored
isLoadingInitialReportActionsflag. Like other stored loading flags, it is a shadow of the request queue and can be stranded by a reload or dropped response, leaving the skeleton up with no request running.This change moves the clean, screen-level consumers onto the queue-derived, per-report
useIsReportLoadPending(reportID)hook, which reads whether anOpenReportrequest for that report is currently in the SequentialQueue. The flag is set and cleared atomically with that request, so healthy-path behavior is identical.The hook now also accepts
reportID: string | undefined(an undefined scope key never matches a realOpenReport, so it reads as "not loading") to avoid defaulting the id to a sentinel value where a caller's reportID is optional.Scope is deliberately narrow: only consumers that read the flag as a plain, screen-level skeleton gate for a single report. Consumers with completion semantics (
=== false), default-trueguards, per-row render paths, transition trackers, or pure utilities that take the state as a parameter are intentionally left on the flag. The flag and its writers are untouched — removal is a separate change.Migrated: MoneyReportHeaderNextStep, MoneyRequestReportView, ReportFooter.
Fixed Issues
$ #96486
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