[No QA] Fix - Add bank account forward path ignores shouldSetUpUSBankAccount by jmusial · Pull Request #99729 · Expensify/App

jmusial · 2026-08-28T05:33:19Z

Explanation of Change

The add-bank-account verify page now mirrors the shouldSetUpUSBankAccount flag from openPersonalBankAccountSetupView via a query param and forwards to the US bank account flow after validation, so the post-verification path can no longer silently diverge from where a validated user is sent directly.

Fixed Issues

$ #98069
PROPOSAL:

Tests

Important

The flow is unreachable by user (as issue states). Following test steps can be forced on web.

Open app in web browser
Use unvalidated account or paste Onyx.merge('account', {validated: false}); in the console
Paste /add-bank-account/verify-account?shouldSkipPurposeSelection=true&shouldSetUpUSBankAccount=true url
Verify that the "Verify account" magic-code RHP is shown.
Validate with magic code or paste Onyx.merge('account', {validated: true); in the console
Verify that the app forwards to /settings/wallet/add-us-bank-account (US bank account flow)
Repeat step 2
Paste /add-bank-account/verify-account?shouldSkipPurposeSelection=true url (w/o shouldSetUpUSBankAccount )
Validate with magic code or paste Onyx.merge('account', {validated: true); in the console
Verify app forwards to /settings/wallet/add-bank-account/country

Offline tests

N/A

QA Steps

N/A

PR Author Checklist

Screenshots/Videos

Details

MacOS: Chrome / Safari

Screen.Recording.2026-08-28.at.13.33.29.mov

jmusial · 2026-08-28T05:42:34Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e1398fa677

ℹ️ 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".

jmusial · 2026-08-28T09:33:38Z

@codex review

chatgpt-codex-connector · 2026-08-28T09:37:26Z

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: ee399341b2

ℹ️ 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".

melvin-bot · 2026-08-28T11:39:29Z

@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]

aimane-chnaif · 2026-08-29T02:57:20Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified that the composer does not automatically focus or open the keyboard on mobile unless explicitly intended. This includes checking that returning the app from the background does not unexpectedly open the keyboard.
I verified tests pass on all platforms & I tested again on:
- Android: HybridApp
- Android: mWeb Chrome
- iOS: HybridApp
- iOS: mWeb Safari
- MacOS: Chrome / Safari
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
- I verified that all the inputs inside a form are aligned with each other.
- I added Design label and/or tagged @Expensify/design so the design team can review the changes.
For any bug fix or new feature in this PR, I verified that sufficient unit tests are included to prevent regressions in this flow.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: HybridApp

Android: mWeb Chrome

iOS: HybridApp

ios.mov

iOS: mWeb Safari

MacOS: Chrome / Safari

web.mov

web2.mov

chatgpt-codex-connector · 2026-08-30T17:01:58Z

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: ee399341b2

ℹ️ 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".

MelvinBot · 2026-08-30T17:07:53Z

Review: LGTM 👍

The change does exactly what issue #98069 asked for (option 1: mirror the flag in the forward path), and it's correctly scoped. I found no correctness bugs — only small nits below.

I also confirmed the flow is still unreachable in production: BaseKYCWall.tsx:164-167 is the only caller passing shouldSetUpUSBankAccount and it still doesn't pass isUserValidated, so [No QA] is right.

Nits (non-blocking)

Dangling comment. ROUTES.ts:159 now reads "Entry points that already know the user is adding a personal deposit account (e.g. a queued reimbursement)." with the predicate removed — it no longer says what those entry points do. Either drop it (the inline per-param comments already carry the meaning) or restore "…pass shouldSkipPurposeSelection."
The new comment over-promises. DynamicAddBankAccountVerifyAccountPage.tsx:25 says the forward path "must agree with the validated branch of openPersonalBankAccountSetupView", but the isAdmin → purpose-screen branch has no counterpart there — that action always passes shouldSkipPurposeSelection: true. That branch exists for PaymentMethodList.tsx:652, which navigates with no params. Worth a half-sentence so the next reader doesn't "fix" the apparent divergence.
Tautological assertion. In BankAccountsTest.ts:183 the expected value is built by calling the same getRoute(true, true) under test, so it can't catch a regression in getRoute. Line 184's stringContaining covers that gap, so this is cosmetic — a literal expected string would be stronger.
Consistency. PaymentMethodList.tsx:652 still uses DYNAMIC_ROUTES.ADD_BANK_ACCOUNT_VERIFY_ACCOUNT.path while the other caller uses getRoute(). Now that a getRoute exists, using it in both places would keep the route construction in one place.

What I verified

The boolean → 'true' type change is a real fix, not just churn. Query params arrive as strings — the linking config maps this screen to a bare path with no parse (config.ts:441), and getPathFromState.ts:60-64 only serializes string | number params back out. So the old boolean type was a lie, and !shouldSkipPurposeSelection only worked because any non-empty string is truthy — ?shouldSkipPurposeSelection=false would have skipped the purpose screen. The new === 'true' / !== 'true' checks fix that.

Precedence matches the action. shouldSetUpUSBankAccount wins over the purpose screen in both the page and BankAccounts.ts:161-168, and both navigate to SETTINGS_ADD_US_BANK_ACCOUNT.getRoute() with no args — so the two paths land identically.

queryParams registration is complete. Adding shouldSetUpUSBankAccount to queryParams means it's stripped correctly on back-navigation via getPathWithoutDynamicSuffix.

Url.ts change is behaviour-neutral. TBase | \${TBase}?${string}`and the old ``${TBase}${'' | ...}`` resolve to the same type; the overload just moves theascast behind an implementation signature. TheTParamsgeneric in the overload is now unused and could beparams: UrlParams`.

CI: all checks green except Check independent approval, which isn't a code signal.

Automated test run — could not execute the PR steps

I tried to run the documented test steps on web. They're not runnable in this harness: steps 1/3/5 need Onyx.merge('account', {validated: …}) in the browser console, and arbitrary page scripting is out of scope for the web driver.

Result: fail (harness limitation, not a PR signal).

Step	Status	Observation
1. Force `validated: false` via console	fail	No console/eval capability in the web driver
2. Open `…?shouldSkipPurposeSelection=true&shouldSetUpUSBankAccount=true`, expect Verify account RHP	fail	Cannot load an arbitrary URL in the CI-owned session; route is unreachable via normal UI
3. Set `validated: true`, expect `/settings/wallet/add-us-bank-account`	fail	Precondition never reached
4. Force unvalidated, open `…?shouldSkipPurposeSelection=true`, expect Verify account RHP	fail	Same as 1–2
5. Set `validated: true`, expect `/settings/wallet/add-bank-account/country`	fail	Precondition never reached

The unit tests in this PR cover the same five branches, so the behaviour is exercised — just not end-to-end.

Evidence (1)

Home screen reached after prestart; no PR step could be executed

view run

MelvinBot · 2026-08-30T17:23:46Z

No regression found

I checked all three changed surfaces. Every input reachable from app code produces the exact same route before and after.

Entry point	`shouldSkipPurposeSelection`	`isAdmin`	Before	After
`PaymentMethodList.tsx:652` (no params)	`undefined`	true	purpose	purpose
same	`undefined`	false	add-bank-account	add-bank-account
`BankAccounts.ts:162` (always passes `true`)	`'true'`	true	add-bank-account	add-bank-account
same	`'true'`	false	add-bank-account	add-bank-account

The !value → value !== 'true' switch is safe because the param is always the string 'true' or undefined — it's parsed by URLSearchParams in getStateForDynamicRoute.ts:32-42, the linking config declares no parse, and nothing pushes this screen with a params object. The old boolean type was the lie. src/libs/Url.ts is a pure type-level no-op — the body is unchanged apart from deleting an erased as cast.

One thing worth acting on: the new branch is dead code

BaseKYCWall.tsx:164-167 is the only caller that sets shouldSetUpUSBankAccount, and it still doesn't pass isUserValidated — so it defaults to true and the unvalidated branch never fires. Today an unvalidated user paying an IOU is sent straight to SETTINGS_ADD_US_BANK_ACCOUNT and never sees the verify RHP at all.

That's pre-existing, not caused by this PR, and it's what [No QA] reflects. But it means the new branch plus its two test rows exercise a path production can't reach. Is BaseKYCWall also meant to pass isUserValidated? If so that's the other half of this fix; if not, the dead branch is fine as a guard but worth saying so explicitly.

Two nits

Stale eslint-seatbelt count. config/eslint/eslint.seatbelt.tsv:687 still claims 3 @typescript-eslint/no-unsafe-type-assertion suppressions for src/libs/Url.ts, but removing the cast leaves only 2. Harmless — npm run lint sets SEATBELT_FROZEN=0 so decreases don't error, and OSBotify recommits the baseline on merge to main. Lint is green. Flagging only because it looks like a break if you hit it later.
?: 'true' is narrower than a URL can carry. A deeplink ?shouldSkipPurposeSelection=banana yields the string 'banana', which the declared type doesn't admit. The runtime !== 'true' handles it correctly, so this is cosmetic — but the sibling precedent types it as string: types.ts:96 with DynamicContactMethodDetailsPage.tsx:247 comparing === 'true'.

What else I ruled out

Navigation / stack behaviour. All four screens live in the same SettingsModalStackNavigator (ModalStackNavigators/index.tsx:399,467,468,474), so forceReplace: true behaves identically for all three forward branches — no cross-stack hop, no underlying-tab swap (shouldCheckFullScreenRouteMatching requires PUSH).

Missing backTo on the US branch. Not a bug: SETTINGS_ADD_US_BANK_ACCOUNT.getRoute has no backTo parameter at all, and AddPersonalBankAccountPage never reads one — the flow computes its own exit. The resulting nav state matches what a validated user gets from openPersonalBankAccountSetupView.

let navigateForwardTo; (evolving any). The if/else is exhaustive and all three assignments are Route members, so control-flow analysis resolves it to that union — never any. prefer-const doesn't fire (three write references), and let x; at component top level is an existing pattern in the repo.

React Compiler. No ignore/seatbelt entry for this file; the change adds only plain let + if/else with no hook calls, mutation, or new closures, so it can't regress compilation or introduce memoization divergence. The useMemo for isAdmin is pre-existing and untouched.

Url.ts type equivalence. `${TBase}${''}` and TBase normalize to the same type at every one of the 38 call sites (33 string literals, 3 pattern literals, 1 union, 1 widened string). Only 2 call sites feed the exported Route union, and both flow through getUrlWithBackToParam which re-normalizes anyway. The 6 toBe assertions in tests/unit/UrlTest.ts are runtime-only and still hold.

CI: all checks green except Check independent approval.

Automated web test — partial (blocked on magic code)

Result: partial — the reachable entry point works and correctly gates on validation, but the flow couldn't be driven past the magic code (no mail access to the test inbox, and no dev bypass code exists in the App source).

Step	Status	Observation
Settings > Wallet loads with "Add bank account" present	pass	Wallet page rendered a "Bank accounts" section with the button
Click "Add bank account" — identify which flow appears	pass	"Validate your account" magic-code RHP appeared (account is unvalidated); `POST /api/ResendValidateCode` fired at 200. Exact SPA path not capturable — the web driver only reports the origin
Complete validation, report forwarded URL	fail	Not reachable: no mail access, no dev bypass magic code. Treated as a physical blocker, not brute-forced
If bank-account flow opened directly, first screen renders	pass	Not applicable — the verify RHP appeared instead
No console errors during the flow	pass	Console capture is out of scope for web sessions; network dump across the whole drive showed zero non-200/304 responses and no error banners

Evidence (2)

Settings > Wallet with Add bank account present

Validate your account magic-code RHP after clicking Add bank account

view run

fix Add bank account forward path ignores shouldSetUpUSBankAccount

8f8d264

melvin-bot Bot assigned jmusial Aug 28, 2026

fix ts

e1398fa

chatgpt-codex-connector Bot reviewed Aug 28, 2026

View reviewed changes

Comment thread tests/unit/pages/settings/Wallet/DynamicAddBankAccountVerifyAccountPageTest.tsx Outdated

fix ts

ee39934

jmusial marked this pull request as ready for review August 28, 2026 11:39

jmusial requested review from a team as code owners August 28, 2026 11:39

melvin-bot Bot requested review from aimane-chnaif and removed request for a team August 28, 2026 11:39

melvin-bot Bot requested review from flaviadefaria and removed request for a team August 28, 2026 11:39

aimane-chnaif approved these changes Aug 30, 2026

View reviewed changes

melvin-bot Bot requested a review from mountiny August 30, 2026 17:00

Navigation Menu

Uh oh!

[No QA] Fix - Add bank account forward path ignores shouldSetUpUSBankAccount - #99729

[No QA] Fix - Add bank account forward path ignores shouldSetUpUSBankAccount#99729
jmusial wants to merge 3 commits into
Expensify:mainfrom
software-mansion-labs:chore/add-bank-account-ignores-should-setup-us-bank-account

jmusial commented Aug 28, 2026 •

edited

Loading

Uh oh!

jmusial commented Aug 28, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

jmusial commented Aug 28, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 28, 2026

Uh oh!

melvin-bot Bot commented Aug 28, 2026

Uh oh!

aimane-chnaif commented Aug 29, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented Aug 30, 2026

Uh oh!

MelvinBot commented Aug 30, 2026

Home screen reached after prestart; no PR step could be executed

Uh oh!

MelvinBot commented Aug 30, 2026

Settings > Wallet with Add bank account present

Validate your account magic-code RHP after clicking Add bank account

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

jmusial commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

jmusial commented Aug 28, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

jmusial commented Aug 28, 2026

Uh oh!

chatgpt-codex-connector Bot commented Aug 28, 2026

Uh oh!

melvin-bot Bot commented Aug 28, 2026

Uh oh!

aimane-chnaif commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

chatgpt-codex-connector Bot commented Aug 30, 2026

Uh oh!

MelvinBot commented Aug 30, 2026

Review: LGTM 👍

Nits (non-blocking)

Home screen reached after prestart; no PR step could be executed

Uh oh!

MelvinBot commented Aug 30, 2026

No regression found

One thing worth acting on: the new branch is dead code

Two nits

Settings > Wallet with Add bank account present

Validate your account magic-code RHP after clicking Add bank account

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmusial commented Aug 28, 2026 •

edited

Loading

aimane-chnaif commented Aug 29, 2026 •

edited

Loading