Repository navigation
Refactor useSelectionMode report actions to use dynamic verify account - #93715
Conversation
…ions-to-use-dynamic-verify-account
|
@aimane-chnaif 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] |
| @@ -292,7 +295,7 @@ function useSelectionModeReportActions({ | |||
| return true; | |||
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This replaces the shared handleUnvalidatedAccount(report) utility with an inline Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.VERIFY_ACCOUNT.path)) call. That exact line already exists as the fallback branch inside handleUnvalidatedAccount (src/libs/PaymentUtils.ts:210), so the navigation logic is now duplicated in two places. The utility also adds route-context-aware behavior (navigating to SEARCH_MONEY_REQUEST_REPORT_VERIFY_ACCOUNT, SEARCH_REPORT_VERIFY_ACCOUNT, EXPENSE_REPORT_VERIFY_ACCOUNT, or REPORT_VERIFY_ACCOUNT based on the active route) that is lost by inlining only the generic case.
Prefer reusing the existing utility so the verify-account navigation stays in one place and keeps its context-aware routing:
if (!isUserValidated && paymentMethodType !== CONST.IOU.PAYMENT_TYPE.ELSEWHERE) {
handleUnvalidatedAccount(report);
return true;
}If the intent is specifically to always navigate to the generic verify-account page here, consider exposing that variant from PaymentUtils (e.g. a small helper) rather than re-implementing the Navigation.navigate(createDynamicRoute(...)) call inline.
Reviewed at: c8497c3 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Same as previously : This is 2. out of 4 PRs to get rid of handleUnvalidatedAccount. Cannot get rid of helper logic branches yet as they are reused by other call places. This will happen in following PRs
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
| } | ||
| if (!isUserValidated && paymentMethodType !== CONST.IOU.PAYMENT_TYPE.ELSEWHERE) { | ||
| handleUnvalidatedAccount(report); | ||
| Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.VERIFY_ACCOUNT.path)); |
There was a problem hiding this comment.
ok so as reported before, app freezes on iOS.
ios.mov
Production also has bug but not freezes.
ios-production.mov
There was a problem hiding this comment.
@aimane-chnaif can you provide details @ ios version ? I wen't though the same scenario and no freeze on Phone 17 Pro (iOS 26.5) ?
There was a problem hiding this comment.
Phone 17 Pro (iOS 26.3)
Is it possible to fix navigation issue happening on production?
There was a problem hiding this comment.
Will look into this today
There was a problem hiding this comment.
I tested on real iPhone device. App freezes on magic code input page.
Confirmed no happening on main.
There was a problem hiding this comment.
Aslo the prod bug only appears if you get there from Inbox. The spend path is ok
There was a problem hiding this comment.
@mountiny could we get ios ad hoc build here please ?
There was a problem hiding this comment.
I see you tested on Spend tab. Can you try on Inbox tab?
There was a problem hiding this comment.
Screen.Recording.2026-06-23.at.15.44.09.mov
I see the "back" bug (looking into it now) but no crash
There was a problem hiding this comment.
ok @aimane-chnaif I think I found the back bug culprit. Could you test w/ last commit ?
|
No product review needed |
|
@mountiny Can we ask QA team to test adhoc build on iPhone? Tests steps:
|
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| // else if (!forwardPath) { | ||
| // forwardPath = backPath; | ||
| // } |
There was a problem hiding this comment.
This seems to fix both issues (freeze and back button loop).
We should understand why this fallback was added in the first place and make sure removing it doesn't cause any regressions.
Originally added in #89608.
@collectioneur was this added on purpose?
There was a problem hiding this comment.
We can remove this part, since it defaults to backPath anyway. Navigation.navigate(forwardPath) with forceReplace: true used to behave differently before we introduced TabNavigator and my changes to the dynamic routing logic, but it looks like it's just dead code that introduces bugs now, so it’s safe to remove 🙂
There was a problem hiding this comment.
Thanks. @jmusial let's remove it and fix conflict
…ions-to-use-dynamic-verify-account
|
🚧 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.19-1 🚀
Bundle Size Analysis (Sentry): |
|
🤖 No help site changes required. This PR is an internal code refactor with no user-facing impact, so no updates are needed to the help site files under WhyThe diff touches two files and makes no behavioral, label, or copy changes:
There are no new features, no changed UI labels/tabs/settings, and no Since no documentation changes are needed, I have not created a draft PR. If you believe a help site update is warranted, let me know what behavior changed and I'll create one. @jmusial, please confirm you agree no help site changes are required here. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.4.19-5 🚀
|
Explanation of Change
Second out fo 4 PR that are originating from #90171. Depends on #93701
This PR migrates MoneyReportHeaderSelectionDropdown to use dynamic verify path.
Fixed Issues
$ #72980
PROPOSAL:
Tests
Repro step:
Offline tests
QA Steps
Same as tests
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
Only affects narrow screens
Android: Native
Screen.Recording.2026-06-16.at.16.28.34.mov
Android: mWeb Chrome
Screen.Recording.2026-06-16.at.16.31.22.mov
iOS: Native
Screen.Recording.2026-06-16.at.16.17.33.mov
iOS: mWeb Safari
Screen.Recording.2026-06-16.at.16.20.18.mov