Add multi-segment dynamic route suffix support and migrate add-bank-account/verify-account#83701
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
jmusial
left a comment
There was a problem hiding this comment.
Would be good to have an example of adding an account (full flow) on the video as well
| const backPath = `${backPathWithoutQuery}${query ? `?${query}` : ''}`; | ||
|
|
||
| return backPath as Route; | ||
| return removeDynamicSuffixFromPath(pathWithoutLeadingSlash, dynamicRouteSuffix) as Route; |
There was a problem hiding this comment.
Can't we just return Route from removeDynamicSuffixFromPath instead of mapping it here ?
| * @param path - The path to find the matching dynamic suffix for | ||
| * @returns The matching dynamic suffix, or undefined if no matching suffix is found | ||
| */ | ||
| function findMatchingDynamicSuffix(path: string | undefined): string | undefined { |
There was a problem hiding this comment.
| function findMatchingDynamicSuffix(path: string | undefined): string | undefined { | |
| function findMatchingDynamicSuffix(path = ''): string | undefined { |
| * @param dynamicSuffix - The dynamic suffix to strip (e.g., 'verify-account') | ||
| * @returns The path with the suffix removed and query params re-appended | ||
| */ | ||
| function removeDynamicSuffixFromPath(fullPath: string, dynamicSuffix: string): string { |
…ing tests for removeDynamicSuffixFromPath function
JmillsExpensify
left a comment
There was a problem hiding this comment.
All these test cases and expected results look right to me, so approving for product. cc @joekaufmanexpensify in case you want to take a look at this PR, since it's your area.
|
@collectioneur please resovle the conflict! |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-03-06.at.14.33.43.mp4Android: mWeb ChromeScreen.Recording.2026-03-06.at.14.38.02.mp4iOS: HybridAppScreen.Recording.2026-03-06.at.14.45.05.mp4iOS: mWeb SafariScreen.Recording.2026-03-06.at.14.48.06.mp4MacOS: Chrome / SafariScreen.Recording.2026-03-06.at.14.20.46.mp4Screen.Recording.2026-03-06.at.14.23.08.mp4 |
|
@collectioneur I think you should record a screenshot on mobile as well, at least on one version, to make sure it works correctly on mobile. |
|
|
||
| // Fallback to not found page so users can't land on dynamic suffix directly. | ||
| if (pathWithoutDynamicSuffix === '/' || pathWithoutDynamicSuffix === '') { | ||
| if ((pathWithoutDynamicSuffix as string) === '/' || (pathWithoutDynamicSuffix as string) === '') { |
There was a problem hiding this comment.
I noticed getPathWithoutDynamicSuffix is typed to return Route, but pathWithoutDynamicSuffix is cast to string.
Maybe we should update getPathWithoutDynamicSuffix to allow returning '/' instead of relying on a string cast.
|
🚧 @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.33-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.3.33-5 🚀
|
Explanation of Change
This PR adds multi-segment dynamic route suffix support and migrates the Add Bank Account Verify Account screen to use it.
The old
getLastSuffixFromPathonly matched single-segment suffixes. NewfindMatchingDynamicSuffixchecks sub-suffixes longest-first, soadd-bank-account/verify-accountmatches correctly without colliding withverify-account.Removed the static route/screen/params, renamed the page to
DynamicAddBankAccountVerifyAccountPage, updated callers to use createDynamicRoute.Fixed Issues
$ #83363
$ #83551
Tests
Preconditions
Account A: Active account with created workspace and linked bank account.
Account B: Fresh, unverified account with no bank account added.
Initial Setup:
Test Case 1: "Add Bank Account" flow via Expense Reports
Steps:
Expected Result 1: The page reloads successfully without errors, and the UI state remains consistent, URL:
/search/view/3204037071619365/add-bank-account/verify-account?...".Expected Result 2: The system successfully navigates the user to the "Add bank account" flow.
Test Case 2: "Add Bank Account" flow via Chats
Steps:
Expected Result 1: The page reloads successfully without errors, and the UI state remains consistent, URL:
/search/add-bank-account/verify-account?q=....Expected Result 2: The system successfully navigates the user to the "Add bank account" flow.
Test Case 3: "Add Bank Account" flow via Wallet Settings
Steps:
Expected Result 1: The "Verify Account" page is displayed, and the URL correctly updates to
/settings/wallet/add-bank-account/verify-account.Expected Result 2: The page reloads correctly, the user remains on the same URL, and the UI state is preserved without errors.
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
Android: Native
Android: mWeb Chrome
iOS: Native
Screen.Recording.2026-03-06.at.10.50.36.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-03-02.at.15.53.55.mov