Repository navigation
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.
|
|
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". |
|
@shubham1206agra 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: 3ff0c25f69
ℹ️ 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".
| Promise.all(setParticipantsPromises).then(() => { | ||
| setMoneyRequestParticipantsFromReport(tid, targetReport, currentUserPersonalDetails.accountID); | ||
| } | ||
| deferNavigate(() => { |
There was a problem hiding this comment.
In the default-policy branch, setTransactionReport() and setMoneyRequestParticipantsFromReport() enqueue two Onyx merges against the same transaction key, so the participant merge may remain queued behind the report merge when this single microtask navigates. If confirmation mounts while participants is still empty, MoneyRequestInitializer treats the global-create draft as uninitialized and calls startMoneyRequest(), which clears/restarts the captured flow. The removed Promise.all prevented that race by waiting for the participant merges; navigation should remain gated on those merges or the fields should be written atomically.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Both writes Onyx.merge the same transactionsDraft_<tid> key synchronously, and Onyx batches those into one delta, so the confirmation never sees reportID set with participants still empty and MoneyRequestInitializer won't restart the flow.
Verified on device (self-DM, workspace, multi-scan, change-recipient): confirmation mounts with participants.length === 1 and startMoneyRequest never runs.
There was a problem hiding this comment.
@OlimpiaZurek Can you find the original issue where this logic was added?
There was a problem hiding this comment.
@shubham1206agra It was added in #61176 (PR #61574) as part of the multi-scan IOU refactor.
It looks like the Promise.all was there to let the participant writes across the draft transactions finish before navigating, not for any navigation timing reason. Removing it and deferring the navigation to a microtask instead seems safe: the writes still fire first, and it's more performant
|
No product review needed |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
@shubham1206agra Could you take a look and review it when you have a chance? Thanks |
|
Hi 👋 |
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-09-04.at.6.38.05.PM.mov |
| function deferNavigate(fn: () => void) { | ||
| Promise.resolve().then(fn); | ||
| } |
There was a problem hiding this comment.
@sosek108 NAB: Can we document this pattern somewhere so peeps starts preferring this over full Promise then chain?
There was a problem hiding this comment.
I added pulled deferNavigate into a shared util and added a short tip in NAVIGATION.md
|
🚧 Julesssss 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/Julesssss in version: 9.4.74-0 🚀
|
|
No help site changes are required for this PR, so I did not create a docs PR. This change is internal-only. It removes an Why no docs update is neededWhat changed:
Why it doesn't touch
I searched Since nothing in @OlimpiaZurek, no help site PR was created because no article content is affected by this change. If you believe a customer-facing behavior did change here that I've missed, reply with the detail and I'll draft the docs PR. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.74-2 🚀
|
Explanation of Change
PR removes the
Promise.allgate innavigateGlobalCreatethat awaited the participant Onyx merges before navigating to "Confirm details". The navigation does not depend on those merges, so awaiting them just held up the shutter-to-confirmation path (~12ms, measured locally).Dropping the await on its own regressed the self-DM path (confirmation did not open on the first shutter tap; the flow returned to the scan step). Root cause: the navigation then ran synchronously inside the shutter's touch handler, so on release the touch resolved to the scan page's back button (
IOURequestStartPage.navigateBack→closeRHPFlow()) instead of the shutter.Fix: keep the writes fire-and-forget (perf gain stands) and queue the navigation on the microtask so it runs after the touch dispatch unwinds and the participants have propagated. Verified on Android across self-DM, workspace, multi-scan, and change-recipient flows.
Fixed Issues
$ #99672
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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