[BT-128] Migrate forwardTo Routes - 2FA Flow#89608
Conversation
ced339c to
edca12f
Compare
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
| }, | ||
| TWO_FACTOR_AUTH_VERIFY_ACCOUNT: { | ||
| path: 'two-factor-auth/verify-account', | ||
| entryScreens: ['*'], |
There was a problem hiding this comment.
Curious, why do you use entryScreens: ['*'] here while the top one has a list ?
There was a problem hiding this comment.
We have a RequireTwoFactorAuthenticationOverlay modal component that can appear on top of any screen in the app. It triggers when the backend signals that the user is required to have 2FA (for example, if the user is made an admin of a workspace that already has Xero set up). Since the entire 2FA flow can overlay any screen, we have to specify all screens as entryScreens here
| [SCREENS.SETTINGS.DYNAMIC_VERIFY_ACCOUNT]: undefined; | ||
| [SCREENS.SETTINGS.DYNAMIC_VERIFY_ACCOUNT]: { | ||
| // eslint-disable-next-line no-restricted-syntax -- `backTo` usages in this file are legacy. Do not add new `backTo` params to screens. See contributingGuides/NAVIGATION.md | ||
| backTo?: Routes; |
There was a problem hiding this comment.
Do we have to add backTo here ?
There was a problem hiding this comment.
Previously, verify-account was used in flows that didn't have a backTo parameter. In this batch, I added a case where the basePath for verify-account can actually have a backTo, so we are propagating backTo into verify-account. This ensures that the previous flow path isn't lost, even on a dynamic screen. Once all routes with backTo that served as a basePath for this case are migrated, this won't be needed anymore and can be removed 🙂
|
|
||
| let forwardPath = backPath; | ||
| if (backPath === ROUTES.SETTINGS_WALLET) { | ||
| if (forwardPath === ROUTES.SETTINGS_NEW_CONTACT_METHOD_CONFIRM_MAGIC_CODE.route) { |
There was a problem hiding this comment.
now that we have useDynamicForwardPath would be nice to move all the logic for calculating forwardPath there
There was a problem hiding this comment.
I love the idea of consolidating, but it might be quite tricky to fit everything into useDynamicForwardPath. It was built more as a test for dynamic screens with forwardTo: if the screen follows the standard forward route, the function returns nothing; if it takes a non-standard route, it returns a specific path. We have some weird edge cases that would be hard to fit inside useDynamicForwardPath (like adding a backTo parameter to a route, or navigating to Xero after a goBack)
| * - entryScreen: The screen that the dynamic suffix is appended to (e.g. 'Settings_Wallet'). | ||
| * - forwardRoute: The destination route to navigate forward to from the dynamic route page. | ||
| */ | ||
| const FORWARD_TO_MAPPINGS: Record<string, Record<string, Route>> = { |
There was a problem hiding this comment.
I could see this in ROUTES same as entryScreens. wdyt ?
There was a problem hiding this comment.
Since forwardTo hasn't been heavily used so far (just in about 5 routes, plus a couple we'll add after the verify-account refactoring), adding it to ROUTES.ts might be a bit much right now. It feels a bit more like a local utility rather than a global one. I could instead extract it into a separate file to keep things tidy, but I'd love to avoid cluttering ROUTES.ts 🙂
There was a problem hiding this comment.
Ok, let's maybe revisit this after couple routes w/ forward to are migrated
|
@huult I noticed a few bugs while checking these two flows, but don't worry, I'll have them fixed by tomorrow. |
|
@collectioneur I checked the other flows and they are working well. |
|
Hey @huult, I’ve fixed all the bugs. I added one more test for the USD flow. For the non-USD flow, I couldn’t get through it manually (since, as far as I know, we don’t have a mock bank account for non-USD flows). So, I just navigated directly to the finish page link and tested it that way. Everything works perfectly 👍 Screen.Recording.2026-05-20.at.12.45.11.mov |
Screen.Recording.2026-05-21.at.15.46.26.mov@collectioneur Before the migration, after completing the 2FA setup, the app would return to the bank account verification flow. After your migration, once 2FA setup is completed, it no longer returns to the bank account verification screen. Could you check this? You can compare the behavior with staging to see what changed. |
Screen.Recording.2026-05-21.at.15.58.10.movOn the BANK_ACCOUNT_NON_USD_SETUP_FINISH page, when I go back, it redirects to the Security page, which is incorrect. In staging, it correctly goes back to the BANK_ACCOUNT_NON_USD_SETUP_FINISH flow instead. Could you check this issue? |
It happens due to #89710, where fullscreen screens aren't updating when navigating to certain modals. As soon as that issue is resolved, this bug will also be fixed because we'll route to a static |
Screen.Recording.2026-05-21.at.11.33.17.movResolved, thanks! |
|
Too quick, haha. I’ll re-check this PR in the next 4 hours |
|
🚧 @mjasikowski 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/mjasikowski in version: 9.3.80-0 🚀
Bundle Size Analysis (Sentry): |
|
No help site changes are required for this PR. This PR is a purely internal code refactoring — it migrates 2FA routes from static The existing help article at |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.81-2 🚀
|
| return setError(translate('twoFactorAuth.errorStepCodes')); | ||
| } | ||
| Navigation.navigate(ROUTES.SETTINGS_2FA_VERIFY.getRoute(route.params?.backTo, route.params?.forwardTo)); | ||
| Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.TWO_FACTOR_AUTH_VERIFY.path, backPath), {forceReplace: true}); |
There was a problem hiding this comment.
Why are we using forceReplace here @collectioneur @huult ?
There was a problem hiding this comment.
@parasharrajat What issue does this cause? We need forceReplace to fix an issue where users can't navigate back
There was a problem hiding this comment.
Do you have steps for that issue or any comments? We are solving this #92544
Explanation of Change
Migrate 2FA routes (
SETTINGS_2FA_ROOT,SETTINGS_2FA_VERIFY,SETTINGS_2FA_SUCCESS,SETTINGS_2FA_VERIFY_ACCOUNT) and the contact-method verify-account route from staticbackTo/forwardToquery params to the dynamic route system. IntroducesuseTwoFactorAuthRoutehook for resolving the correct 2FA entry point anduseDynamicForwardPathhook for forward navigation after dynamic pages. Splits the oldTwoFactorAuthPageinto dedicated dynamic pages, extractsSuccessPageBase, and updates all consumers.Fixed Issues
$ #83360
Tests
Test 1
/verify-accountpage, and after validating you proceed to the/two-factor-authpage/two-factor-authpage loads, copy the recovery codes, proceed to the verify step, enter a valid code, and verify the success page appearsTest 2
/verify-accountpage (URL uses the dynamic route pattern, e.g.settings/profile/contact-methods/verify-account)Test 3
Test 4
Offline tests
N/A
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Test 1:
1_test.mov
Test 2:
2_test.mov
Test 3:
3_test.mov
Test 4:
Screen.Recording.2026-05-21.at.11.33.17.mov