Fix empty "Upload Additional Documents" page in NonUSD owner flow#91289
Conversation
Derive the Documents skip predicate from getNeededDocumentsStatusForBeneficialOwner so "page is skipped" can no longer drift from "page is empty", and read the bank country via getCurrencyForNonUSDBankAccount (achData-aware) instead of the draft only, so the predicate also fires when country lives only in achData. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
melvin-bot
Bot
requested review from
abzokhattab and
heyjennahay
and removed request for
a team
|
@abzokhattab 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
a team and
youssef-lr
and removed request for
a team
|
@youssef-lr 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| function BeneficialOwnerDetailsFormPages({stepNames, policyID, onFinished, backTo}: BeneficialOwnerDetailsFormPagesProps) { | ||
| const {translate} = useLocalize(); | ||
| const [reimbursementAccountDraft] = useOnyx(ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM_DRAFT); | ||
| const [reimbursementAccount] = useOnyx(ONYXKEYS.REIMBURSEMENT_ACCOUNT); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
useOnyx(ONYXKEYS.REIMBURSEMENT_ACCOUNT) subscribes to the entire reimbursement account object, but only achData.country is used (via getCurrencyForNonUSDBankAccount). This causes unnecessary re-renders when any unrelated field on the reimbursement account changes.
Use a selector to extract only the needed field:
const [reimbursementAccountCountry] = useOnyx(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {
selector: (ra) => ra?.achData?.additionalData?.country,
});Then pass the extracted value directly instead of the full object, or restructure getCurrencyForNonUSDBankAccount to accept the primitive values it actually needs.
Reviewed at: 16e329e | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| const {translate} = useLocalize(); | ||
| const [reimbursementAccountDraft] = useOnyx(ONYXKEYS.FORMS.REIMBURSEMENT_ACCOUNT_FORM_DRAFT); | ||
| const [reimbursementAccount] = useOnyx(ONYXKEYS.REIMBURSEMENT_ACCOUNT); | ||
| const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`); |
There was a problem hiding this comment.
❌ PERF-11 (docs)
useOnyx(ONYXKEYS.COLLECTION.POLICY + policyID) subscribes to the entire policy object, but only outputCurrency is used (via getCurrencyForNonUSDBankAccount). Policy objects are large and change frequently, causing unnecessary re-renders.
Use a selector to extract only the needed field:
const [outputCurrency] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {
selector: (p) => p?.outputCurrency,
});Then pass the extracted primitive value directly instead of the full policy object.
Reviewed at: 16e329e | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
Valid principle (PERF-11), but the suggested path is wrong — country is at achData.country |
1 participant
Explanation of Change
In the NonUSD (Corpay) bank account flow, the beneficial owner "Upload Additional Documents" sub-step could appear with no required upload fields, no requirements, and the user could skip it without entering anything.
The Documents page renders only four upload sections, each gated by
getNeededDocumentsStatusForBeneficialOwner(currency, country, nationality). For workspace currencyGBP, bank countryGB, owner nationalityGB, all four flags collapse tofalse, so the page shows only a heading and two paragraphs — an empty page.The parent (
BeneficialOwnerDetailsFormPages.tsx) was supposed to skip that page, but its skip rule was a narrow hardcodedcountry === GB && nationality === GBcheck that (1) did not match the actual render predicate, so the two could drift apart, and (2) read the bank country from the form draft only. When the country lives only inachData(e.g. after server hydration of the reimbursement account, whereCountryFullStepdoes not re-write the draft because the value already equals the default), the draft value is'', theGB && GBclause isfalse, the page is not skipped, and the empty page renders.This change derives the skip predicate from the same
getNeededDocumentsStatusForBeneficialOwnerfunction that drives the page's render, and reads country/currency through the samegetCurrencyForNonUSDBankAccounthelper already used byDocuments.tsxandConfirmation.tsx(which falls back toachData.country). The Documents sub-step is now skipped exactly when all four document flags arefalse— i.e. exactly when the page would be empty — so "page is skipped" and "page is empty" can no longer diverge. This resolves the reported GBP/UK/UK case and is future-proof for any other combination where no documents are required.Fixed Issues
$ #90505
PROPOSAL: #90505 (comment)
Tests
Offline tests
This change adds no API calls. The skip decision is computed purely from locally available Onyx data (
reimbursementAccountDraft,reimbursementAccount.achData, andpolicy.outputCurrency), so it behaves identically online and offline.QA Steps
Same as the Tests section above (run on staging).
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
iOS: mWeb Safari
MacOS: Chrome / Safari
After Fix:
Before Fix:
Comparison:
