Implement query parameters handling in dynamic routes#84158
Conversation
…ponents, and updating navigation dynamic routes logic.
…tioneur/dynamic-routes-parameters-handling
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
39a53ba to
a55b125
Compare
| /** | ||
| * Merges two query strings into one. If both contain the same key, | ||
| * the suffix value wins and a warning is logged. | ||
| */ |
There was a problem hiding this comment.
Could you add an example in the docs? What will be the result of merging two example values etc.
| import splitPathAndQuery from './splitPathAndQuery'; | ||
|
|
||
| function getQueryParamsToStrip(dynamicSuffix: string): readonly string[] | undefined { |
There was a problem hiding this comment.
Add a description to this function
|
I see we have a lot of functions related to dynamic routes which are kept directly in |
…ter organization and maintainability
…dynamic routes and dynamic routes with query parameters
|
No product review needed |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Screen.Recording.2026-03-11.at.15.03.59.movBUG:
This issue does not occur if the page is not reloaded. |
Screen.Recording.2026-03-11.at.15.15.41.movBUG:
|
|
@collectioneur Could you add the test steps and record the case when adding a bank account at the address step? This is because the CountrySelector is used there. Screen.Recording.2026-03-11.at.15.36.56.mov |
Screen.Recording.2026-03-11.at.15.42.42.movBUG:
|
@huult This behavior is specific to the page I just migrated. Here's what's happening: to pass the data about a selected or existing parameter back, it has to attach this parameter to the previous page and then navigate back. This allows the previous page to properly extract the parameter and clean it up from the URL. Screen.Recording.2026-03-11.at.16.36.37.movScreen.Recording.2026-03-11.at.16.37.20.mov |
Added test and recorded the case. Could you please give it one more review when you have a chance 🙂 ? Thank you! |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ 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". |
|
One more try |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70807267fa
ℹ️ 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".
| !isNavigatingToAttachmentScreen(focusedRouteFromPath?.name) && | ||
| !isNavigatingToReportWithSameReportID(currentFocusedRoute, focusedRouteFromPath) | ||
| !isNavigatingToReportWithSameReportID(currentFocusedRoute, focusedRouteFromPath) && | ||
| !findMatchingDynamicSuffix(normalizedPath) |
There was a problem hiding this comment.
Limit dynamic-route detection before skipping PUSH conversion
This new guard uses findMatchingDynamicSuffix(normalizedPath) to decide whether to keep NAVIGATE, but that suffix check is purely textual and also matches non-dynamic routes that end in the same segment (for example static .../verify-account routes such as SETTINGS_2FA_VERIFY_ACCOUNT and SETTINGS_WALLET_TRAVEL_CVV_VERIFY_ACCOUNT in src/ROUTES.ts). In those cases this branch now stops converting NAVIGATE to PUSH, which can reuse an existing stack entry and skip adding a history entry, regressing back/navigation behavior for normal static screens.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Given the context of the screens (RHPs) we are currently migrating, there is no scenario where they would have been in the navigation stack previously. Because of this, it is perfectly safe to leave the logic as it is. If any unforeseen regressions arise in the future, we can easily revisit and update this behavior
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @mjasikowski 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.3.37-0 🚀
|
|
@collectioneur @mjasikowski this likely caused #85191 (comment). Should we revert? Not sure, as I feel the bug (landing on the settings page after adding the PBA instead of staying on the expense details page) is not a huge deal, and could maybe just be a follow up |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.37-10 🚀
|
Explanation of Change
Add query parameter support to dynamic routes. Previously, dynamic suffixes couldn't include query params (e.g.,
country?country=US). This extendsDynamicRouteConfigwith an optionalgetRoutefunction, refactorscreateDynamicRouteto merge base and suffix query strings, and updatesgetPathWithoutDynamicSuffixto strip suffix-specific params. MigratesSETTINGS_ADDRESS_COUNTRYfrom a static route to the newADDRESS_COUNTRYdynamic route as a proof of concept.Fixed Issues
$ #84119
Tests
1 test
2 test
3 test
settings/profile/address/country?country=USTest 4
settings/wallet.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
Screen.Recording.2026-03-06.at.17.22.34.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-03-06.at.16.51.33.mov
add-us-bank-accounttest country page:https://github.com/user-attachments/assets/39923dfb-4636-4c20-a96d-878508f6f617