[Performance] Pre-mount new expense destinations behind confirmation RHP - #97874
[Performance] Pre-mount new expense destinations behind confirmation RHP#97874JakubKorytko wants to merge 28 commits into
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 946460a7b6
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@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". |
d77efce to
c5ccc51
Compare
17fd0cb to
ac63040
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac630400e6
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71d480f11f
ℹ️ 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".
|
Addressed @MelvinBot concerns ✅ |
|
❗ For the QA testers There is a visible "blink" of an RHP after swiping back from the QAB expense creation, this is pre-existing and unrelated to the pre-mounting. There is a pending PR to fix this - #97020 |
I can't swipe back from the QAB create expense flow. It keeps scrolling. This is a bug on prod already. So please update this test step. Screen.Recording.2026-08-12.at.23.34.02.mov |
|
BUG: I can't swipe back from the confirmation step. I keeps re-navigating to the confirmation step. Screen.Recording.2026-08-12.at.23.39.52.mov |
|
Some bugs that my Claude found. Might just be false positives. Promotion marker cleared before real write can bail
Consequence chain: marker gone, ref gone, no optimistic write ran. Unmount cleanup skips ( Same root cause, second shape: marker-null Non-reveal dismiss paths get cancel semantics with buffer live
If buffer was inserted (RHP inner index 0 at pre-insert), successful submit through these paths dismisses RHP → state listener fires → destination stripped → user lands at origin, expense created but chat never shown. Reachability hinges on whether PAY/per-diem can sit at inner index 0 — possibly not today, but invariant "every submit dismiss must go through reveal()" is unenforced and undocumented. At minimum |
gijoe0295
left a comment
There was a problem hiding this comment.
Minor comment cleanups
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-08-13.at.10.12.42.PM.movAndroid: mWeb ChromeRecording_20260813_221428.mp4iOS: HybridAppScreen.Recording.2026-08-13.at.10.45.47.PM.moviOS: mWeb SafariScreen.Recording.2026-08-13.at.9.27.38.PM.movMacOS: Chrome / SafariScreen.Recording.2026-08-13.at.10.18.09.PM.mov |
|
@MelvinBot Review |
ReviewSolid, carefully-reasoned PR. The behavior is well-motivated, the comments explain why (not just what) throughout, and the new pure helpers + navigation handlers have good unit coverage. I verified the primary web-testable flow end-to-end on a narrow layout and it works as described. Below are the higher-value things to double-check before merge — none are hard blockers. Verified behavior (narrow web)On a 375×812 viewport, creating an expense for a brand-new recipient (no prior chat) opens that recipient's 1:1 chat showing the created expense, with only a brief loading state — no flash of the previous page and no stuck loading / not-found. Screenshots below. EvidenceConfirm page → Create → transient loading (tab bar + bare Back only, not a re-render of the prior Inbox/Concierge screen) → recipient chat with the $25.00 outstanding expense card. What the PR does
Points worth a second look1. Native swipe-dismiss guard is untested by CI — needs manual native QA. Nearly all of the buffer machinery is gated behind 2. Startup cleanup can delete the report row — confirm the invariant holds. 3. 4. Module-level Test coverageGood coverage on the mechanical pieces: Overall this looks well-engineered and the web happy-path is solid. My main ask is that the native-only swipe/back paths get real on-device verification, since CI can't reach them. |
…ffer stripping destination on same-report dismiss
You need to do it fast enough, the best to start swiping just before/after opening animation ends. Also, on the video it seems you are starting the swipe on the frame 😅
This is due to the simulator itself, it's hard to do it - needs to be almost pixel perfect, keep trying. Not related to code at all. Also addressed the @gijoe0295 Claude & Melvin comments. |
marufsharifi
left a comment
There was a problem hiding this comment.
Code changes looks good, i am still testing.
|
@JakubKorytko, could you please merge the main. thanks. |
Resolve import conflict in IOUUtilsTest.ts by keeping both ReportAction (from main) and Participant (from branch).
marufsharifi
left a comment
There was a problem hiding this comment.
Code changes looks good, but some minor suggestions.
|
@JakubKorytko, could you please merge the main. thanks. |
|
@JakubKorytko, could you please check this. thanks. RF.mp4 |
|
Good catch, may you re-test now? |


Explanation of Change
Improves confirmation RHP destination pre-mounting for new 1:1 chats and employer workspaces. It reuses stable optimistic report IDs, safely promotes draft reports for rendering, suppresses premature report fetching, cleans interrupted promotions, and inserts a neutral buffer route under the RHP so a native swipe-dismiss reveals it instead of flashing the pre-mounted destination.
Fixed Issues
$ #98139
PROPOSAL: N/A
Tests
+, selectCreate expense, enter an amount, and choose that user as the recipient.Create expense.SUBMIT_2026beta enabled and no existing workspace, open a tracked expense in the self chat, and clickSubmit to my employer.Create expenseon the confirmation page.Create expenseentry point for 1:1 expense - this lands directly on a single-screen RHP (no participants/amount steps stacked underneath), so a swipe from the screen edge dismisses the whole RHP instead of popping one inner step back.+flow and swipe on the confirmation screen instead: verify it just pops back to the amount step (RHP stays open).Offline tests
Create expense.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.mov
Android: mWeb Chrome
iOS: Native
iOS.mov
iOS: mWeb Safari
MacOS: Chrome / Safari