Prevent Pay elsewhere fallback while bank account data is loading

[MelvinBot]

Explanation of Change

When bankAccountList hasn't loaded from Onyx yet, the isPayer() check for ACH-configured policies (REIMBURSEMENT_YES) incorrectly fails for admin users who access the business bank account via sharees. This causes onlyShowPayElsewhere to become true, showing only the "Pay elsewhere" option instead of the ACH reimbursement option. If the user clicks "Pay elsewhere" during this loading window, the report is marked as paid without actual money movement.

This PR adds loading guards for bankAccountList in the three places where onlyShowPayElsewhere is computed:

1. MoneyReportHeader.tsx — Disables and shows loading state on the settlement button while bankAccountList is loading
2. MoneyRequestReportPreviewContent.tsx — Same loading guard applied to the report preview settlement button
3. useSearchBulkActions.ts — Prevents onlyShowPayElsewhere from being computed as true while bankAccountList is loading

The fix follows the existing pattern used in PaymentMethodList.tsx where isLoadingOnyxValue(bankAccountListResult) is already used for the same purpose.

Fixed Issues

$ #85319
PROPOSAL: #85319 (comment)

Tests

1. Log in as a workspace admin who is NOT the designated reimburser but has bank account access via sharees on a policy with reimbursementChoice=REIMBURSEMENT_YES
2. Open a report that needs reimbursement
3. Click the Pay button
4. Verify the Pay button shows a loading spinner briefly while bank account data loads
5. Once loaded, verify the ACH reimbursement option is available (not just "Pay elsewhere")

• Verify that no errors appear in the JS console

Offline tests

1. Go offline before opening a report needing reimbursement
2. Verify the Pay button is disabled while offline
3. Go back online
4. Verify the Pay button becomes enabled and shows the correct payment options

QA Steps

1. Log in as a workspace admin (not the designated reimburser) on a policy configured for direct ACH reimbursement
2. Open a report needing reimbursement
3. Click Pay
4. Verify the ACH option is available and "Pay elsewhere" is not the only option
5. Also verify in Search bulk actions that the correct payment options appear

• Verify that no errors appear in the JS console

PR Author Checklist

• I linked the correct issue in the ### Fixed Issues section above
• I wrote clear testing steps that cover the changes made in this PR
  • I added steps for local testing in the Tests section
  • I added steps for the expected offline behavior in the Offline steps section
  • I added steps for Staging and/or Production testing in the QA steps section
  • I added steps to cover 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 tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• I included screenshots or videos for tests on all platforms
• I ran the tests on all platforms & verified they passed on:
  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
• I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• I followed proper code patterns (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 shown in the product is localized by adding it to src/languages/* files and using the translation method
    • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
  • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
  • 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)
  • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • I verified the JSDocs style guidelines (in STYLE.md) were followed
• If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• I followed the guidelines as stated in the Review Guidelines
• I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
• I verified that if a function's arguments changed that all usages have also been updated correctly
• 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 new assets were added or existing ones were modified, I verified that:
  • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
  • The assets load correctly across all supported platforms.
• 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.
• If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
• I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user 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.

Screenshots/Videos

Android: Native

N/A - No UI changes, only loading state behavior change

Android: mWeb Chrome

N/A - No UI changes, only loading state behavior change

iOS: Native

N/A - No UI changes, only loading state behavior change

iOS: mWeb Safari

N/A - No UI changes, only loading state behavior change

MacOS: Chrome / Safari

N/A - No UI changes, only loading state behavior change

[MelvinBot]

The failing check build (Debug, true) is unrelated to this PR's changes.

Analysis: The Android Gradle build failed because repo.grails.org returned a 520 server error when resolving org.jetbrains.kotlin:kotlin-stdlib:2.1.20. This is a transient infrastructure issue with an external Maven repository.

Evidence: This PR only modifies TypeScript files (MoneyReportHeader.tsx, MoneyRequestReportPreviewContent.tsx, useSearchBulkActions.ts). The build failure is in Gradle dependency resolution, completely unrelated to these changes. Re-running the build should resolve this.

[codecov]

Codecov Report

✅ Changes either increased or maintained existing code coverage, great job!

Files with missing lines	Coverage Δ
...ReportPreview/MoneyRequestReportPreviewContent.tsx	61.95% <100.00%> (+0.08%)	⬆️
src/hooks/useSearchBulkActions.ts	38.84% <100.00%> (+38.84%)	⬆️
src/components/MoneyReportHeader.tsx	0.00% <0.00%> (ø)
... and 142 files with indirect coverage changes

[abzokhattab]

@MelvinBot consider adding tests for the hook

[MelvinBot]

Thanks for the suggestion! I looked into adding tests for the useSearchBulkActions hook and wanted to share what I found:

Challenges with testing this hook directly:

• The hook has ~15+ useOnyx subscriptions, depends on SearchContext, DelegateNoAccessModal, and many other contexts/hooks
• onlyShowPayElsewhere (the value this PR's change affects) is internal state — it's not directly returned by the hook. It flows through useBulkPayOptions to influence headerButtonsOptions
• The hook currently has 0% test coverage, and bootstrapping a test would require mocking a very large surface area (see existing patterns for similar hooks that mock far fewer dependencies)

What gives confidence in this change:

• The loading guard pattern (isLoadingOnyxValue) is already used in PaymentMethodList.tsx for the same BANK_ACCOUNT_LIST key
• The logic is a simple early return (if (isLoadingBankAccountList) return false) — there's no complex branching to cover
• The same pattern is applied consistently across all three changed files

If you'd like me to proceed with adding tests despite the heavy mocking needed, I'm happy to — just let me know the scope you'd prefer (e.g., full hook test vs. testing just the onlyShowPayElsewhere behavior through headerButtonsOptions).

[neerajbachani]

@MelvinBot @abzokhattab One edge case worth considering, if bankAccountList fetch fails and Onyx never transitions from status='loading', the pay button would be indefinitely disabled. Should we add a timeout fallback or treat network errors as loaded to avoid a permanently broken Pay button?

[abzokhattab]

@MelvinBot, let's add test cases for the whole file in order to increase the coverage and keep in mind tests best practices

[abzokhattab]

@neerajbachani Good catch on thinking about edge cases! Onyx resolves the loading state regardless of whether the network request succeeds or fails ... even on failure, it transitions out of status='loading' (falling back to whatever is cached locally or undefined). Since BANK_ACCOUNT_LIST is also persisted on-device, for returning users it hydrates from local storage almost immediately anyway. So in practice, the loading window is milliseconds and there's no risk of the button being indefinitely disabled.

[MelvinBot]

Added comprehensive tests for useSearchBulkActions in tests/unit/hooks/useSearchBulkActions.test.ts — 38 test cases, all passing.

Test coverage includes:

Initial state (3 tests)

• Modal states default to false/null
• Empty options when queryJSON is undefined or no transactions selected

Derived values (3 tests)

• Unique policy IDs from selected transactions
• Unique report IDs from selected transactions
• Report IDs from selected reports

onlyShowPayElsewhere loading guard (2 tests) — the PR change

• Returns false when bank account list is loading
• Evaluates normally when bank account list is loaded

Header button options (25 tests)

• Export: always present; only option when all matching items selected
• Approve: shown when all transactions have approve action; hidden when offline, on hold, or mixed actions
• Pay: shown when bulk pay enabled; hidden when offline or on hold
• Hold/Unhold: shown when all transactions can be held/unholded; hidden when offline or mixed
• Submit: shown when all transactions have submit action; hidden when offline
• Delete: controlled by shouldShowDeleteOption
• Reject: shown when all can be rejected and not expense_report type; hidden when offline

Modal handlers (5 tests)

• All handler functions are properly returned

Option ordering (1 test)

• Multiple action types coexist correctly (approve + hold + export + delete)

[abzokhattab]

remove unnecessary tests @MelvinBot not all of them are needed

[MelvinBot]

Fixed the failing spellcheck: corrected 'unholded' to 'unheld' in two test descriptions in tests/unit/hooks/useSearchBulkActions.test.ts (lines 777 and 794).

[MelvinBot]

Trimmed the test suite from 38 to 13 tests. Removed:

• Modal handler typeof checks (5 tests) — trivially checking that returned functions are functions
• Derived values (3 tests) — testing basic JS Set/filter operations, not real hook logic
• Offline/on-hold/mixed-action variants (12 tests) — repetitive negative-case patterns across approve, pay, hold, unhold, submit, reject
• No options available (1 test) — redundant with the export test
• Option ordering (1 test) — redundant with individual action tests
• Delete negative case (1 test) — just testing a mock returns false
• Reject expense_report type (1 test) — narrow variant
• Approve reports variant (1 test) — similar pattern to transaction-based test

Kept 13 focused tests:

• 2 initial state (undefined queryJSON, no transactions)
• 2 loading guard tests (the PR change — bank account loading vs loaded)
• 9 header button option tests (one positive case per action type: export all-selected, export always, approve, pay, hold, unhold, submit, reject, delete)

[MelvinBot]

Fixed the failing typecheck: removed unnecessary as string casts in createBaseQueryJSON() in the test file. The casts were widening CONST literal types to string, making them incompatible with the SearchQueryJSON type which expects specific literal types like SearchDataTypes, SearchStatus, etc.

[MelvinBot]

I have failed to help you. I let you down and for that I am sorry. Please try again later. This may be due to a Claude outage (check here).

Failure details: https://github.com/Expensify/Expensify/actions/runs/23215725164

[MelvinBot]

I have failed to help you. I let you down and for that I am sorry. Please try again later. This may be due to a Claude outage (check here).

Failure details: https://github.com/Expensify/Expensify/actions/runs/23215769997

[MelvinBot]

I have failed to help you. I let you down and for that I am sorry. Please try again later. This may be due to a Claude outage (check here).

Failure details: https://github.com/Expensify/Expensify/actions/runs/23215993947

[abzokhattab]

@MelvinBot fix the pipelines

[abzokhattab]

@MelvinBot since we mocking a lot of functions, does it still make sense to keep the tests or should it be better cleaned?

[MelvinBot]

Fixed the failing checks. The test file (tests/unit/hooks/useSearchBulkActions.test.ts) was using CONST.SEARCH.VIEW.LIST which doesn't exist — the valid values are TABLE, BAR, LINE, and PIE. Changed it to CONST.SEARCH.VIEW.TABLE, which fixes:

• typecheck: TS2339: Property 'LIST' does not exist
• ESLint (Changed files + full): @typescript-eslint/no-unsafe-assignment (downstream of the TS error)

[MelvinBot]

Good question. Here's my take on the value vs. mocking cost:

Current state: 13 tests, ~300 lines of mocks (about half the file). Nearly every dependency is mocked — isLoadingOnyxValue, canIOUBePaid, getPayOption, shouldShowDeleteOption, etc. So the tests only verify the conditional branching logic within the hook, not real Onyx loading or payment logic.

Worth keeping (4 tests):

• 2 loading guard tests — directly validate this PR's change (onlyShowPayElsewhere returns false while loading). This is the most valuable coverage.
• 2 initial state tests — lightweight, stable, and guard against regressions if someone refactors the early-return logic.

Could be cleaned (9 headerButtonsOptions tests):
Each one tests "if mock condition X → option Y appears." Since getPayOption, shouldShowDeleteOption, canIOUBePaid, etc. are all mocked, these tests don't catch real integration issues — they only verify if/else branching that's straightforward to read in code. They also become a maintenance burden when the hook's dependencies change (any new useOnyx call or import needs a new mock).

Recommendation: Trim down to the 4 high-value tests above. This keeps meaningful regression coverage for the PR change while reducing the mocking surface significantly. The positive-case option tests (approve, pay, hold, etc.) test simple conditional logic that's better caught in integration/E2E tests where real Onyx data flows through.

Alternatively, if you'd prefer to keep a few representative option tests as regression guards, I'd suggest keeping just 2: the "export always present" test and one action test (e.g., approve). That gives a smoke test without maintaining 9 similar patterns.

Let me know which direction you'd like and I'll clean it up.

[abzokhattab]

ok lets keep the 4 tests @MelvinBot

[MelvinBot]

Trimmed the test suite to 4 focused tests as requested:

• 2 initial state tests — empty options when queryJSON is undefined or no transactions selected
• 2 loading guard tests — validates the PR change: onlyShowPayElsewhere returns false while bank account list is loading, and evaluates normally when loaded

Removed 9 headerButtonsOptions tests that were testing mocked conditional branching (approve, pay, hold, unhold, submit, reject, delete, export variants).

[MelvinBot]

Fixed the failing Prettier check: removed a trailing blank line in tests/unit/hooks/useSearchBulkActions.test.ts that Prettier flagged.

[abzokhattab]

@MelvinBot mark the PR as ready

[melvin-bot]

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

[github-actions]

❌ CONSISTENCY-5 (docs)

The /* eslint-disable @typescript-eslint/naming-convention */ directive at the top of this new test file lacks an accompanying comment explaining why the rule needs to be disabled. ESLint rule disables without justification can mask underlying issues and reduce code quality.

Add a comment explaining why the naming convention rule is disabled. For example:

/* eslint-disable @typescript-eslint/naming-convention -- Mock data uses Onyx collection key patterns (e.g., `report_report1`) that don't conform to standard naming conventions */

---

Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

[abzokhattab]

@MelvinBot check this comment #85578 (comment)

[MelvinBot]

Fixed — added justification comment explaining that mock data uses Onyx collection key patterns that don't conform to standard naming conventions.

[MelvinBot]

All CI checks are passing (the only failing one is the PR Author Checklist, which is expected).

Could you clarify what you'd like me to check? For example:

• Code review — I can review the changes for correctness or edge cases
• Check failures — All CI checks are green
• Something specific — Let me know what you'd like me to look into

[melvin-bot]

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

[arosiclair]

This unit test seems very contrived and I'm not sure it's really adding value. I think we should just remove it.

[arosiclair]

This race condition seems valid, but are we sure this is the root problem? I think it would only take a few milliseconds for Onyx to load values. How would a user open the page and click the Pay button so quickly?