[Performance] Migrate submit-expense flow to usePreMountDestination - #96480
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
![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: 64de0aad5f
ℹ️ 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 |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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". |
70c8e6d to
f64c512
Compare
|
@codex review |
JakubKorytko
changed the title
usePreMountDestinationusePreMountDestination
|
Codex Review: Didn't find any major issues. Nice work! 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". |
There was a problem hiding this comment.
Pull request overview
This PR migrates the submit-expense (and related skip-confirmation) modal-to-destination navigation from bespoke pre-insert logic to the shared usePreMountDestination hook, extracting pure “route resolver” helpers and adding unit coverage to keep modal dismiss transitions gapless (no flash/gap).
Changes:
- Replaced inline pre-insert orchestration in
IOURequestStepConfirmationand the deprecateduseSkipConfirmationPreInserthook withusePreMountDestination. - Introduced two route-resolver utilities (
getSubmitExpensePreMountDestinationRoute,getSkipConfirmationPreMountDestinationRoute) plus sharedgetSubmitExpenseSearchType, with new unit tests. - Tightened enforcement by updating the ESLint local rule to only allow direct fullscreen pre-insert from
usePreMountDestination/Navigation.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/getSubmitExpensePreMountDestinationRouteTest.ts | Adds unit tests covering the submit-expense pre-mount destination decision logic. |
| tests/unit/getSkipConfirmationPreMountDestinationRouteTest.ts | Adds unit tests for skip-confirmation pre-mount destination decision logic. |
| src/pages/iou/request/step/IOURequestStepScan/components/ScanSkipConfirmation.tsx | Switches skip-confirmation pre-insert to usePreMountDestination via the new route resolver. |
| src/pages/iou/request/step/IOURequestStepDistance.tsx | Switches skip-confirmation pre-insert to usePreMountDestination via the new route resolver. |
| src/pages/iou/request/step/IOURequestStepAmount.tsx | Switches skip-confirmation pre-insert to usePreMountDestination via the new route resolver. |
| src/pages/iou/request/step/IOURequestStepConfirmation.tsx | Replaces inline pre-insert effect with memoized route resolution + usePreMountDestination, and wires reveal into the orchestrator. |
| src/pages/iou/request/step/confirmation/SubmitExpenseOrchestrator.tsx | Consumes the injected revealPreMountDestination instead of managing pre-insert flags directly. |
| src/pages/iou/request/step/confirmation/getSubmitExpenseSearchType.ts | Centralizes IOU-type → Search-tab mapping to keep handlers/resolvers consistent. |
| src/pages/iou/request/step/confirmation/getSubmitExpensePreMountDestinationRoute.ts | New submit-expense-specific pre-mount route resolver (narrow-only). |
| src/pages/iou/request/step/confirmation/getSkipConfirmationPreMountDestinationRoute.ts | New skip-confirmation pre-mount route resolver (delegates narrow gating to the hook). |
| src/hooks/useSkipConfirmationPreInsert.ts | Removes the old bespoke skip-confirmation pre-insert hook. |
| src/hooks/usePreMountDestination/index.ts | Adds a warning for unexpected “non-owned reveal with pre-insert flag set” scenarios and exports AfterTransition. |
| eslint-plugin-local-rules/no-direct-pre-insert-fullscreen-under-rhp.js | Updates allowlist to enforce using usePreMountDestination instead of ad-hoc pre-insert calls. |
| .claude/skills/coding-standards/rules/perf-18-use-pre-mount-destination.md | Updates the internal rule doc to remove deprecated exception references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ZhenjaHorbach 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-07-23.16.06.55.movAndroid: mWeb Chrome2026-07-23.16.00.52.moviOS: HybridApp2026-07-23.16.06.55.moviOS: mWeb Safari2026-07-23.16.00.52.movMacOS: Chrome / Safari2026-07-23.15.58.51.mov |
|
@JakubKorytko |
|
LGTM! |
|
🚧 roryabraham 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.45-0 🚀
|
|
🤖 I reviewed the changes in this PR against Expensify's help site files under No help site changes are required, so I did not create a draft PR. This PR is a purely internal performance/architecture refactor:
Per the linked issue Since the help site documents user-facing product functionality and none of that changes here, there is nothing to update in @JakubKorytko, since no user-facing behavior changed, no help site PR was created and there is nothing to review. If you believe a specific article is affected, let me know which one and I'll take another look. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.45-14 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Migrates the submit-expense flow off the bespoke pre-insert logic (
useSkipConfirmationPreInsert+ the inline effect inIOURequestStepConfirmation) to theusePreMountDestinationhook, extracting two tested pure route resolvers. Also fixes a self-invalidation loop where a route's own pre-insertion flipped its eligibility to false and tore it back down.Fixed Issues
$ #94673
PROPOSAL: N/A
Tests
The migrated behavior is the modal-to-destination transition: after submitting, the destination screen must already be there when the modal closes, with no flash of the screen you started on.
Setup: two accounts, at least one workspace you belong to, and one existing 1:1 chat.
"Wait until fully settled" = let the Confirm details screen finish animating before submitting; submitting sooner exercises the fallback path (covered by Narrow test 2).
Narrow layout
Wide layout
Offline tests
QA Steps
Same as the Tests section above.
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.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Web.mov