[Internal QA] Migrate ReportLostCardPage to Navigation-Based Validation #76280
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.
|
|
@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] |
Code Review SummaryI've completed a comprehensive review of this PR. The refactoring from a modal-based validation flow to a navigation-based approach is well-implemented. The code follows React best practices and TypeScript patterns correctly. Key Observations:
Architecture Notes:The migration improves the user experience by using native navigation transitions instead of modals, which is more consistent with modern mobile app patterns. The forceReplace: true parameter added to the success navigation prevents users from navigating back to intermediate validation steps. No critical issues found. The implementation follows Expensify's established patterns and coding standards. |
|
No product considerations |
|
|
||
| function ReportCardLostConfirmMagicCodePage({ | ||
| route: { | ||
| params: {cardID = '', reason = 'damaged'}, |
There was a problem hiding this comment.
Where do these default params come from?
There was a problem hiding this comment.
cardID = '' is widely used in all other pages, reason = 'damaged' there are 2 of them, just picked one
| shouldShowButton | ||
| onButtonPress={() => { | ||
| Navigation.navigate(ROUTES.SETTINGS_WALLET_DOMAIN_CARD.getRoute(cardID)); | ||
| Navigation.navigate(ROUTES.SETTINGS_WALLET_DOMAIN_CARD.getRoute(cardID), {forceReplace: true}); |
There was a problem hiding this comment.
Won't this cause navigation animation direction issue on mobile?
There was a problem hiding this comment.
You can see the animation for example in IOS: Native clip. IMO looks ok
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb Chromemchrome.moviOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
2 bugs in this video: Screen.Recording.2025-12-04.at.12.35.37.pm.mov
|
@1 - fixed repro.mov |
This is still reproducible Screen.Recording.2025-12-05.at.1.33.07.pm.mov |
|
Ok, the reload should be fixed now. Found another issue, but it's on main as well. Not sure it's a real life scenario since I'm forcing the button for the card that has not been send. After reporting card stolen and entering magic code BE responds with but |
|
|
||
| const newErrors = physicalCard?.errors ?? {}; | ||
| // Only update if errors have actually changed to prevent additional rerender | ||
| if (deepEqual(newErrors, formData?.errors ?? {})) { |
There was a problem hiding this comment.
The reload didn't happen before without using deepEqual.
Why do you think this happened after migration and are forced to use deep compare?
There was a problem hiding this comment.
As far as I could reproduce it updating errors was causing the reload, so was hoping checking weather they actually changed will solve it. And I couldn't reproduce reload on my side after.
But looking @ your comment below it may be that I just get API response that code has beed issued fast enough that I cannot get into the state before.
|
There's still case where magic code input is reset but I think it's fine as it doesn't make sense to input before getting magic code web-refresh.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.2.75-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.2.75-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mjasikowski in version: 9.2.77-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.2.77-1 🚀
|
|
@truph01 Definitely wasn't planned so it's a regression. Do you want to fix it as a part of yours ? Otherwise I can take a look but realistically next week. |

Explanation of Change
Refactor
ReportLostCardPageto use navigation based confirm modal page instead of modal based.Fixed Issues
$ #75869
PROPOSAL:
Tests
Pre requisite:
Have an account with physical card issued.
Due to dev acc limitations w/ physical cards, the flow tested in 2 parts.
Dev pre requisites:
src/pages/settings/Wallet/ExpensifyCardPage/index.tsx#147src/pages/settings/Wallet/ReportCardLostConfirmMagicCodePage.tsx#34Steps:
Offline tests
QA Steps
IMPORTANT: Must have an actual issue physical card.
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.Screenshots/Videos
Android: Native
0077.android.native.mov
Android: mWeb Chrome
0077.android.chrome.mov
iOS: Native
0077.ios.native.mov
iOS: mWeb Safari
0077.ios.safari.mov
MacOS: Chrome / Safari
0077.desktop.chrome.mov