Migrate ExpensifyCardMissingDetailsPage to Navigation-Based Validation #76141
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.
|
jmusial
changed the title
melvin-bot
bot
requested review from
abdulrahuman5196
and removed request for
a team
|
@abdulrahuman5196 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] |
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| clearPersonalDetailsErrors(); | ||
| }, [validateCodeAction?.errorFields, validateLoginError]); | ||
|
|
||
| const values = useMemo(() => normalizeCountryCode(getSubstepValues(privatePersonalDetails, draftValues)) as PersonalDetailsForm, [privatePersonalDetails, draftValues]); |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Passing entire objects (privatePersonalDetails and draftValues) as dependencies causes this useMemo to re-execute whenever any property in these objects changes, even unrelated ones. This creates unnecessary recalculations.
Specify individual properties that getSubstepValues actually uses:
const values = useMemo(
() => normalizeCountryCode(getSubstepValues(privatePersonalDetails, draftValues)) as PersonalDetailsForm,
[
privatePersonalDetails?.legalFirstName,
privatePersonalDetails?.legalLastName,
privatePersonalDetails?.dob,
privatePersonalDetails?.phoneNumber,
privatePersonalDetails?.addresses,
draftValues?.legalFirstName,
draftValues?.legalLastName,
draftValues?.dob,
draftValues?.phoneNumber,
draftValues?.address,
]
);
There was a problem hiding this comment.
Please address this feedback
There was a problem hiding this comment.
Theoretically true, but this causes lint error.
| setIsCardDetailsLoading((prevState: Record<number, boolean>) => ({...prevState, [cardID]: false})); | ||
| }); | ||
| }, | ||
| [cardID, countryCode, setCardsDetails, setCardsDetailsErrors, setIsCardDetailsLoading, values], |
There was a problem hiding this comment.
❌ PERF-6 (docs)
Including the entire values object in the dependency array causes this useCallback to be recreated whenever any property in values changes, even if those properties aren't used by setPersonalDetailsAndRevealExpensifyCard.
Since setPersonalDetailsAndRevealExpensifyCard requires the complete PersonalDetailsForm object, and values is already memoized (line 48), consider whether this callback truly needs to be memoized at all, or specify the individual properties from values that actually matter:
const handleSubmitForm = useCallback(
(validateCode: string) => {
// ... implementation
},
[
cardID,
countryCode,
setCardsDetails,
setCardsDetailsErrors,
setIsCardDetailsLoading,
values.legalFirstName,
values.legalLastName,
values.dob,
values.phoneNumber,
values.address,
]
);Or remove useCallback if the callback isn't passed to a memoized child component.
mountiny
requested review from
aimane-chnaif
and removed request for
abdulrahuman5196
| const [privatePersonalDetails, privatePersonalDetailsMetadata] = useOnyx(ONYXKEYS.PRIVATE_PERSONAL_DETAILS, {canBeMissing: false}); | ||
| const [draftValues, draftValuesMetadata] = useOnyx(ONYXKEYS.FORMS.PERSONAL_DETAILS_FORM_DRAFT, {canBeMissing: false}); |
There was a problem hiding this comment.
any reason for setting canBeMissing to false? I see more occurrences of true than false in the app
| clearPersonalDetailsErrors(); | ||
| }, [validateCodeAction?.errorFields, validateLoginError]); | ||
|
|
||
| const values = useMemo(() => normalizeCountryCode(getSubstepValues(privatePersonalDetails, draftValues)) as PersonalDetailsForm, [privatePersonalDetails, draftValues]); |
There was a problem hiding this comment.
Please address this feedback
| setIsCardDetailsLoading((prevState: Record<number, boolean>) => ({...prevState, [cardID]: false})); | ||
| }); | ||
| }, | ||
| [cardID, countryCode, setCardsDetails, setCardsDetailsErrors, setIsCardDetailsLoading, values], |
|
Not a product change. Removing myself and unsubscribing |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.mov |
There was a problem hiding this comment.
Bug: after input incorrect magic code, modal dismisses.
Bug: browser back button goes to the same page
Screen.Recording.2025-12-01.at.4.43.13.pm.mov
Actually, this is regression from this old PR - #70736.
Let's fix this as follow-up!
|
Please check last part of this video. There's browser back button issue. It should go to Expensify Card page, not Reveal Details. web.mov |
on it |
…ation-based-validation
@1 @2 It's weird and I cannot repro. If you look @ your url, the card Id changes, the back button works as expected since those are 2 different urls. But I cannot get why the card ID changes ? |
Weird, cannot repro either Screen.Recording.2025-12-03.at.13.45.59.mov |
Maybe because I've hardcoded. I manually set cards like below: |
ok I can't reproduce with only one card set. |
|
✋ 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.73-0 🚀
|
|
Hi @jmusial When adding a bank account the system forces to update USD currency. So we can't test it. Let me know if we missed anything. Thank you in advance explorer_8R7IHM8Hqj.mp4 |
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.2.73-5 🚀
|
6 participants
Explanation of Change
Refactor
ExpensifyCardMissingDetailsPageto use navigation based confirm modal page instead of modal based.Fixed Issues
$ #75872
PROPOSAL:
Tests
If you have access to An EU/UK bank account
Pre requisite: An EU/UK bank account on EU/UK workspace so EU/UK virtual card can be issued
Otherwise:
change
src/pages/settings/Wallet/ExpensifyCardPage/index.tsx#263to
to make error appear permanently
Offline tests
n/a
QA Steps
Pre requisite: An EU/UK bank account on EU/UK workspace so EU/UK virtual card can be issued
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
0075.android.native.mov
Android: mWeb Chrome
0075.android.chrome.mov
iOS: Native
0075.ios.native.mov
iOS: mWeb Safari
0075.ios.safari.mov
MacOS: Chrome / Safari
0075.desktop.mov