[NO QA] Create DynamicVerifyAccountPage Component (BATCH-4)#81392
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
…r/dynamic-routes-batch-4
…r/dynamic-routes-batch-4
melvin-bot
Bot
requested review from
JmillsExpensify and
huult
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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: bad2c69f79
ℹ️ 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".
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-02-11.at.17.12.38.mp4 |
Screen.Recording.2026-02-11.at.14.04.38.movI’m seeing “Uh-oh, something went wrong!” when accessing https://dev.new.expensify.com:8082/verify-account. Since this route has no valid context on its own, I think it should instead resolve to a Not Found page. |
|
@huult AFAIK there's no way to access that route other than manually typing the URL, so I think it's OK that it's empty for now (it will be fixed in this project anyway) |
|
@mjasikowski Sounds good to me if we already have a plan to address this as part of the project 👍 |
|
I noticed that the options parameter is no longer used anywhere. Because of that, the App/src/libs/Navigation/NavigationRoot.tsx Line 151 in b19f605 |
|
Line 241 in 1c4fd9a There are still two places using |
If we really can’t remove the options parameter, I think we should add a comment explaining why it needs to stay. That said, we should remove App/src/libs/Navigation/NavigationRoot.tsx Line 151 in b19f605 In both cases, we are passing the parameter but it is no longer used, so it has no effect. |
|
@collectioneur Could you please resolve the conflict? |
…tions from args when calling getAdaptedStateFromPath
|
@huult Addressed all comments✅ |
| // Check for backTo param. One screen with different backTo value may need different screens visible under the overlay. | ||
| if (isRouteWithBackToParam(route)) { | ||
| const stateForBackTo = getStateFromPath(route.params.backTo, config); | ||
| const stateForBackTo = getStateFromPath(route.params.backTo as RoutePath); |
There was a problem hiding this comment.
I noticed that you’re using type assertions like as Route or as RoutePath because we’re calling getStateFromPath with a value typed as Route. However, the getStateFromPath API from React Navigation actually accepts a plain string.
What’s your opinion on this approach? Do you think we should avoid these casts and instead handle the path as a string with proper validation before passing it in?
There was a problem hiding this comment.
I believe we should keep the Route type. It makes type assertions explicit and gives developers a hint about which routes are valid for state generation.
I kept it as Route to maintain consistency, since our custom getStateFromPath has accepted Route from the start. I still need to research whether switching to string is strictly better. If it turns out that Route isn't necessary, I suggest making that change in a follow-up PR.
|
|
||
| // Fallback to not found page so users can't land on dynamic suffix directly. | ||
| if (pathWithoutDynamicSuffix === '/' || pathWithoutDynamicSuffix === '') { | ||
| const state = {routes: [{name: 'not-found', path: normalizedPathAfterRedirection}]}; |
There was a problem hiding this comment.
| const state = {routes: [{name: 'not-found', path: normalizedPathAfterRedirection}]}; | |
| const state = {routes: [{name: SCREENS.NOT_FOUND, path: normalizedPathAfterRedirection}]}; |
Please update not-found to SCREENS.NOT_FOUND.
There was a problem hiding this comment.
Thanks for catching this, updated ✅
|
🚧 @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! 🧪🧪
|
|
✋ 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 If possible, could we hold off on releasing this PR to staging for now? It’s causing some bugs, and @collectioneur is currently working on fixing them (#81505). |
|
We can only revert it, let me know if you want me to do that |
|
@mjasikowski I think we should revert after @collectioneur is back from OOO so we can double-check everything. |
|
Reverted |
|
Working on v2! |
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.3.18-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.18-8 🚀
|
5 participants
Explanation of Change
I've created the first dynamic page based on dynamic routes. To support this, I needed to add a few custom functions:
getPathFromStateto correctly retrieve the current path on the page, and theuseDynamicBackPathhook, which returns the correct return path for each dynamic page.Fixed Issues
$ #80909
Tests
Offline tests
QA Steps
NO QA
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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.