Open validation RHP directly for pending USD bank accounts - #94707
Conversation
Co-authored-by: Monil Bhavsar <MonilBhavsar@users.noreply.github.com>
|
I created this PR from an upstream branch since you don't have push access to a fork. To take ownership of this branch so you can push updates, run: Then you can close this PR and open a new one from your fork. |
|
@MelvinBot looks like you added changes outside of your own proposal. Please only apply the changes from your proposal |
… RHP Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
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.
|
|
|
||
| // If USD bank account is in pending state, we should navigate straight to the validation step and skip Continue step | ||
| if (policyCurrency === CONST.CURRENCY.USD && achData?.state === CONST.BANK_ACCOUNT.STATE.PENDING) { | ||
| setUSDBankAccountStep(CONST.BANK_ACCOUNT.STEP.VALIDATION); |
There was a problem hiding this comment.
Done in 8f47310 — removed the setUSDBankAccountStep(CONST.BANK_ACCOUNT.STEP.VALIDATION) call. It was redundant: this branch navigates to the separate BANK_ACCOUNT_USD_SETUP (page VALIDATION) route and returns early, so updating this page's local step state had no effect before it navigated away. Lint and typecheck pass.
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
@MelvinBot add this as the QA step. Test and Offline steps are the same.
|
| setUSDBankAccountStep(CONST.BANK_ACCOUNT.STEP.VALIDATION); | ||
| goToWithdrawalAccountSetupStep(CONST.BANK_ACCOUNT.STEP.VALIDATION); | ||
| setShouldShowContinueSetupButton(shouldShowContinueSetupButtonValue); | ||
| Navigation.navigate(ROUTES.BANK_ACCOUNT_USD_SETUP.getRoute({policyID: policyIDParam, page: CONST.BANK_ACCOUNT.PAGE_NAMES.VALIDATION, backTo})); |
There was a problem hiding this comment.
@MelvinBot Remove this navigation code.
Navigating immediately in useEffect after mount somehow causes this page to be unmounted (on small screen), which resets the achData to the default state.
So, instead of navigating here, let's navigate to the validation page from navigateToBankAccountRoute. We will need to add policyCurrency and achDataState params. policyCurrency from policy.outputCurrency, while achDataState is from reimbursementAccount.achData.state.
function navigateToBankAccountRoute({
policyID = '',
policyCurrency,
bankAccountID,
achDataState,
backTo,
navigationOptions,
}: {
policyID?: string;
policyCurrency: string;
bankAccountID?: number;
achDataState: ACHData['state'];
backTo?: string;
navigationOptions?: LinkToOptions;
}) {
if (policyCurrency === CONST.CURRENCY.USD && achDataState === CONST.BANK_ACCOUNT.STATE.PENDING) {
Navigation.navigate(ROUTES.BANK_ACCOUNT_USD_SETUP.getRoute({policyID, page: CONST.BANK_ACCOUNT.PAGE_NAMES.VALIDATION, backTo}));
return;
}
Navigation.navigate(ROUTES.BANK_ACCOUNT_WITH_STEP_TO_OPEN.getRoute({policyID, bankAccountID, backTo}), navigationOptions);
}
There was a problem hiding this comment.
@MelvinBot nevermind, revert to the previous changes.
There was a problem hiding this comment.
@MelvinBot can you first investigate why navigating inside the useEffect causes the ReimbursementAccountPage to be unmounted
There was a problem hiding this comment.
@MelvinBot please investigate why navigating to validation page inside the useEffect causes the ReimbursementAccountPage to be unmounted
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
…ccountRoute" This reverts commit 9477df0.
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
1 similar comment
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot please investigate why navigating to validation page inside the useEffect causes the ReimbursementAccountPage to be unmounted |
1 similar comment
|
@MelvinBot please investigate why navigating to validation page inside the useEffect causes the ReimbursementAccountPage to be unmounted |
Why
|
|
@MelvinBot it's not expected. If I delay the navigation (for example by using setTimeout 0), then the ReimbursementAccountPage is not unmounted. |
|
You're right, and my earlier "this is expected RHP detachment" answer was wrong. If it were just inherent card-detachment, Root cause: The redirect fires synchronously in This is a known bug class in this codebase — navigation racing an Onyx update. The purpose-built helper for it is Two ways to fix it:
Why immediate vs. deferred actually differ (mechanism, traced)
Confidence: high that this is an Onyx-load-vs-navigation race fixed by the microtask helper (documented precedent + your Next Steps: Reply with |
|
@MelvinBot let's use settimeout with 0 delay. explain why with a comment |
… unmount race Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Done in 7b11994 — wrapped the validation redirect in The comment captures the mechanism from our investigation: navigating synchronously in this mount effect runs in the same frame as the navigation that just opened the page from Workflows, collapsing both into one commit while Onyx is still hydrating
|
| // page mounts from Workflows) collapses the two navigations into a single commit while Onyx is still | ||
| // hydrating achData. On small screens that detaches/unmounts this page mid-load and resets achData to its | ||
| // defaults. Letting the current frame finish so the first navigation commits and Onyx settles makes the | ||
| // redirect a clean push, keeping the page mounted. |
There was a problem hiding this comment.
@MelvinBot don't mention about onyx hydration. it's unrelated
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Updated in ffcf059 — removed the Onyx hydration wording. The comment now attributes the unmount solely to the navigation-timing race (two navigations landing in the same frame collapse into one commit, detaching/unmounting the page and resetting its |
|
@MelvinBot copy this to PR Author Checklis checklist
|
|
Done — replaced the PR Author Checklist section in the PR body with the full checklist you provided, all items checked. |
|
@MelvinBot check the |
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
@MonilBhavsar done |
|
ES lint is failing |
|
@codex review |
|
Were you able to test it? If so could you please attach screenshots in the description? |
|
@MelvinBot fix the lint |
|
There was no actual merge conflict to resolve — GitHub reports the branch as
The PR still shows "blocked", but that's branch protection waiting on required status checks (e.g. the failing ESLint you flagged separately) — not a merge conflict. Once those checks pass it'll be mergeable. |
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Fixed the lint in 947e669. The failing rule was
|
![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: ac1b782f24
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const bankAccountConnectedToWorkspace = policyID ? Object.values(bankAccountList ?? {}).find((bankAccount) => bankAccount?.accountData?.policyIDs?.includes(policyID)) : undefined; | ||
| const bankAccountState = bankAccountConnectedToWorkspace?.accountData?.state ?? ''; |
There was a problem hiding this comment.
When this new direct validation route is opened from Workflows/Wallet, the selected workspace bank account is identified by accountData.additionalData.policyID in the callers, but this screen only searches accountData.policyIDs. For a persisted pending VBA that doesn't have the optimistic policyIDs array, bankAccountConnectedToWorkspace stays undefined, bankAccountState is empty, and the validation form never renders. Please also match additionalData.policyID here (or pass the bankAccountID through the route).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This accountData.policyIDs and accountData.additionalData.policyID is kinda confusing. When I connect the BA to a 2nd workspace, the 2nd workspace will be appended to policyIDs. But after reopening the Workflows page, the policyIDs become a single array again that only contains the 2nd workspace, and accountData.additionalData.policyID is also pointing to the 2nd workspace. I checked the first workspace Workflows and yes, the BA is disconnected. Not sure if that's a bug or not, but I think we shouldn't worry with the comment above. I'd rather not touch it (the workflows and wallet page that uses accountData.additionalData.policyID).
| if (policyCurrency === CONST.CURRENCY.USD && bankAccountState === CONST.BANK_ACCOUNT.STATE.PENDING) { | ||
| Navigation.navigate(ROUTES.BANK_ACCOUNT_USD_SETUP.getRoute({policyID, page: CONST.BANK_ACCOUNT.PAGE_NAMES.VALIDATION, backTo}), navigationOptions); |
There was a problem hiding this comment.
Navigating straight to BANK_ACCOUNT_USD_SETUP skips the ReimbursementAccountPage path that calls openReimbursementAccountPage; after leaving the original setup flow that Onyx data can be cleared, while USDVerifiedBankAccountFlowPage still uses reimbursementAccount?.achData?.state to decide whether Back should return to the entry route. In that state, pressing Back from the validation RHP falls through to previous USD setup pages instead of returning to backTo (and max-attempt state is also unavailable). Please fetch/hydrate the reimbursement account before opening this route, or make the validation route self-hydrate from policyID/bank account data.
Useful? React with 👍 / 👎.
|
@MelvinBot we currently get the BA state from App/src/pages/ReimbursementAccount/USD/USDVerifiedBankAccountFlowPage.tsx Lines 120 to 126 in 7a94579 Let's update it too by getting it from the bankAccountList |
Co-authored-by: Bernhard Owen Josephus <bernhardoj@users.noreply.github.com>
|
Done in 137b793 — the pending-state check in const bankAccountConnectedToWorkspace = policyID ? Object.values(bankAccountList ?? {}).find((bankAccount) => bankAccount?.accountData?.policyIDs?.includes(policyID)) : undefined;
const bankAccountState = bankAccountConnectedToWorkspace?.accountData?.state ?? '';
|
|
@MonilBhavsar done, the recordings is on the checklist here |
|
Thanks! |
|
🚧 MonilBhavsar 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/MonilBhavsar in version: 9.4.34-0 🚀
|
Help site review — no changes requiredI reviewed the changes in this PR against the help site files under Why: This PR is a navigation bug fix. When a USD business bank account is in the The relevant article, I checked both help articles that touch bank-account validation:
Since no help-site content becomes inaccurate, I did not open a docs PR. If you'd like a doc change anyway (e.g., to explicitly document reaching validation from the Workflows page), let me know and I'll draft one. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
Bundle Size Analysis (Sentry): |
4 participants
Explanation of Change
When a USD business bank account is in the
PENDING(micro-deposit validation) state and the user taps it from Workspace Settings → Workflows,ReimbursementAccountPageis supposed to skip the entry screen and open the "Validate your bank account" RHP directly.The existing
PENDINGbranch only mutated local/Onyx step state — it never navigated. The actual validation form lives on a different route (BANK_ACCOUNT_USD_SETUP, pageVALIDATION), so the page fell through to its terminal render ofVerifiedBankAccountFlowEntryPoint, which showed the "Continue setup" / "Start over" menu instead of the validation form.This change makes the
PENDINGbranch actually perform the forward navigation the inline comment promises — it navigates toROUTES.BANK_ACCOUNT_USD_SETUPwithpage = VALIDATION(the same destinationcontinueUSDVBBASetupalready produces for aVALIDATIONstep), sending the user straight to the "Validate your bank account" RHP.Fixed Issues
$ #93564
PROPOSAL: #93564 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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