Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/components/Search/FilterComponents/BankAccountSelector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import SelectionListWithSections from '@components/SelectionList/SelectionListWi
import type {TextInputOptions} from '@components/SelectionList/types';

import useDebouncedState from '@hooks/useDebouncedState';
import useInitialValue from '@hooks/useInitialValue';
import useLocalize from '@hooks/useLocalize';
import useOnyx from '@hooks/useOnyx';
import useResponsiveLayout from '@hooks/useResponsiveLayout';
Expand Down Expand Up @@ -97,12 +98,21 @@ function BankAccountSelector({value = [], selectionListTextInputStyle, selection

const shouldShowSearchInput = openItems.length + closedItems.length >= CONST.STANDARD_LIST_ITEM_LIMIT;

// Snapshot the accounts selected when the filter first opened so they stay floated in the top section on first render
// without repinning rows that are toggled afterwards. Section membership keys on this snapshot while each row's
// checkbox still reflects the live selection, so selecting/deselecting an account no longer makes it jump between sections.
// Only float the initial selection when the list is long enough to warrant it (>= STANDARD_LIST_ITEM_LIMIT), mirroring
// the shared moveInitialSelectionToTop gate; for short lists items stay in their natural order so nothing is pinned.
const initialSelectedValues = useInitialValue(() => value);
const wasInitiallySelected = (item: BankAccountFilterItem) => initialSelectedValues.includes(item.value);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CONSISTENCY-3 (docs)

shouldPinInitialSelection computes the exact same expression (openItems.length + closedItems.length >= CONST.STANDARD_LIST_ITEM_LIMIT) that shouldShowSearchInput already computes a few lines above. This duplicates the same list-length gate in two named constants, so any future change to the threshold logic has to be made in both places.

Reuse the already-computed value instead of recalculating it:

const shouldShowSearchInput = openItems.length + closedItems.length >= CONST.STANDARD_LIST_ITEM_LIMIT;
// ...
const shouldPinInitialSelection = shouldShowSearchInput;

or, if the two concepts should stay distinct, hoist the shared comparison into one named boolean (e.g. isLongList) that both derive from.


Reviewed at: 7e11554 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 7924ff1shouldPinInitialSelection now just aliases the already-computed shouldShowSearchInput instead of recomputing openItems.length + closedItems.length >= CONST.STANDARD_LIST_ITEM_LIMIT, so the list-length gate lives in one place. Kept the distinct name since it documents the pinning intent at its use sites.

const shouldPinInitialSelection = shouldShowSearchInput;

const searchFunction = (item: BankAccountFilterItem) =>
item.text.toLocaleLowerCase().includes(debouncedSearchTerm.toLocaleLowerCase()) || item.lastFour.toLocaleLowerCase().includes(debouncedSearchTerm.toLocaleLowerCase());

const selectedData = [...openItems, ...closedItems].filter((item) => item.isSelected && searchFunction(item));
const unselectedOpenData = openItems.filter((item) => !item.isSelected && searchFunction(item));
const unselectedClosedData = closedItems.filter((item) => !item.isSelected && searchFunction(item));
const selectedData = shouldPinInitialSelection ? [...openItems, ...closedItems].filter((item) => wasInitiallySelected(item) && searchFunction(item)) : [];
const unselectedOpenData = openItems.filter((item) => (!shouldPinInitialSelection || !wasInitiallySelected(item)) && searchFunction(item));
const unselectedClosedData = closedItems.filter((item) => (!shouldPinInitialSelection || !wasInitiallySelected(item)) && searchFunction(item));
Comment on lines +114 to +115

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Disable auto-scroll when new bank selections stay in place

When selecting an unselected bank account, this new grouping now leaves the row in unselectedOpenData/unselectedClosedData, but SelectionListWithSections still auto-scrolls multi-section multi-select lists to index 0 before calling onSelectRow (BaseSelectionListWithSections.tsx:132-135) unless shouldPreventAutoScrollOnSelect is set. In bank-account lists, choosing an account below the fold will therefore jump the viewport to the top even though the row no longer moves there, so the no-jump behavior is not actually achieved.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by setting shouldPreventAutoScrollOnSelect = true


const itemCount = selectedData.length + unselectedOpenData.length + unselectedClosedData.length;
const sectionHeaderCount = unselectedClosedData.length > 0 ? 1 : 0;
Expand Down Expand Up @@ -176,6 +186,9 @@ function BankAccountSelector({value = [], selectionListTextInputStyle, selection
textInputOptions={textInputOptions}
shouldStopPropagation
canSelectMultiple
shouldClearInputOnSelect={false}
shouldUpdateFocusedIndex
shouldPreventAutoScrollOnSelect
style={selectionListStyle}
footerContent={footer}
/>
Expand Down
21 changes: 17 additions & 4 deletions src/components/Search/FilterComponents/CardSelector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type {TextInputOptions} from '@components/SelectionList/types';

import {useCompanyCardFeedIcons} from '@hooks/useCompanyCardIcons';
import useDebouncedState from '@hooks/useDebouncedState';
import useInitialValue from '@hooks/useInitialValue';
import useLocalize from '@hooks/useLocalize';
import useNetwork from '@hooks/useNetwork';
import useOnyx from '@hooks/useOnyx';
Expand Down Expand Up @@ -67,17 +68,26 @@ function CardSelector({value = [], selectionListTextInputStyle, selectionListSty

const closedCardsSectionData = buildCardsData(workspaceCardFeeds ?? {}, userCardList ?? {}, personalDetails ?? {}, value, illustrations, companyCardFeedIcons, true, customCardNames);

const shouldShowSearchInput = individualCardsSectionData.length >= CONST.STANDARD_LIST_ITEM_LIMIT;
const shouldShowSearchInput = individualCardsSectionData.length + closedCardsSectionData.length >= CONST.STANDARD_LIST_ITEM_LIMIT;

// Snapshot the cards selected when the filter first opened so they stay floated in the top section on first render
// without repinning rows that are toggled afterwards. Section membership keys on this snapshot while each row's
// checkbox still reflects the live selection, so selecting/deselecting a card no longer makes it jump between sections.
// Only float the initial selection when the list is long enough to warrant it (>= STANDARD_LIST_ITEM_LIMIT), mirroring
// the shared moveInitialSelectionToTop gate; for short lists items stay in their natural order so nothing is pinned.
const initialSelectedValues = useInitialValue(() => value);
const wasInitiallySelected = (item: CardFilterItem) => !!item.keyForList && initialSelectedValues.includes(item.keyForList);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ CONSISTENCY-3 (docs)

shouldPinInitialSelection computes the exact same expression (individualCardsSectionData.length + closedCardsSectionData.length >= CONST.STANDARD_LIST_ITEM_LIMIT) that shouldShowSearchInput already computes on the line above. This duplicates the same list-length gate in two named constants, so the threshold logic must be kept in sync in two places.

Reuse the already-computed value instead of recalculating it:

const shouldShowSearchInput = individualCardsSectionData.length + closedCardsSectionData.length >= CONST.STANDARD_LIST_ITEM_LIMIT;
// ...
const shouldPinInitialSelection = shouldShowSearchInput;

or, if the two concepts should stay distinct, hoist the shared comparison into one named boolean (e.g. isLongList) that both derive from.


Reviewed at: 7e11554 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 7924ff1shouldPinInitialSelection now just aliases the already-computed shouldShowSearchInput instead of recomputing individualCardsSectionData.length + closedCardsSectionData.length >= CONST.STANDARD_LIST_ITEM_LIMIT, so the list-length gate lives in one place. Kept the distinct name since it documents the pinning intent at its use sites.

const shouldPinInitialSelection = shouldShowSearchInput;

const searchFunction = (item: CardFilterItem) =>
!!item.text?.toLocaleLowerCase().includes(debouncedSearchTerm.toLocaleLowerCase()) ||
!!item.lastFourPAN?.toLocaleLowerCase().includes(debouncedSearchTerm.toLocaleLowerCase()) ||
!!item.cardName?.toLocaleLowerCase().includes(debouncedSearchTerm.toLocaleLowerCase()) ||
(item.isVirtual && translate('workspace.expensifyCard.virtual').toLocaleLowerCase().includes(debouncedSearchTerm.toLocaleLowerCase()));

const selectedData = [...individualCardsSectionData, ...closedCardsSectionData].filter((item) => item.isSelected && searchFunction(item));
const unselectedIndividualCardsData = individualCardsSectionData.filter((item) => !item.isSelected && searchFunction(item));
const unselectedClosedCardsData = closedCardsSectionData.filter((item) => !item.isSelected && searchFunction(item));
const selectedData = shouldPinInitialSelection ? [...individualCardsSectionData, ...closedCardsSectionData].filter((item) => wasInitiallySelected(item) && searchFunction(item)) : [];
const unselectedIndividualCardsData = individualCardsSectionData.filter((item) => (!shouldPinInitialSelection || !wasInitiallySelected(item)) && searchFunction(item));
const unselectedClosedCardsData = closedCardsSectionData.filter((item) => (!shouldPinInitialSelection || !wasInitiallySelected(item)) && searchFunction(item));
Comment on lines +89 to +90

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Disable auto-scroll when new card selections stay in place

When selecting an unselected card, this new grouping now leaves the row in unselectedIndividualCardsData, but SelectionListWithSections still auto-scrolls multi-section multi-select lists to index 0 before calling onSelectRow (BaseSelectionListWithSections.tsx:132-135) unless shouldPreventAutoScrollOnSelect is set. In long card lists, choosing a card below the fold will therefore jump the viewport to the top even though the row no longer moves there, which preserves the original disruptive jump in a different form.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by setting shouldPreventAutoScrollOnSelect = true


const itemCount = selectedData.length + unselectedIndividualCardsData.length + unselectedClosedCardsData.length;
const sectionHeaderCount = unselectedClosedCardsData.length > 0 ? 1 : 0;
Expand Down Expand Up @@ -155,6 +165,9 @@ function CardSelector({value = [], selectionListTextInputStyle, selectionListSty
textInputOptions={textInputOptions}
shouldStopPropagation
canSelectMultiple
shouldClearInputOnSelect={false}
shouldUpdateFocusedIndex
shouldPreventAutoScrollOnSelect
style={selectionListStyle}
footerContent={footer}
/>
Expand Down
164 changes: 164 additions & 0 deletions tests/ui/BankAccountSelectorTest.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
import {render} from '@testing-library/react-native';

import BankAccountSelector from '@components/Search/FilterComponents/BankAccountSelector';
import SelectionListWithSections from '@components/SelectionList/SelectionListWithSections';

import CONST from '@src/CONST';

import React from 'react';

// The bank accounts returned by the (mocked) Onyx `BANK_ACCOUNT_LIST` key. Tests mutate this to switch between a
// long list (pinning enabled) and a short list (pinning disabled). Each account's `text` drives the search filter.
let mockBankAccountList: Record<string, {accountData: {bankAccountID: number; accountNumber: string; additionalData: {bankName: string}; state: string}; text: string}> = {};
let mockSearchTerm = '';

jest.mock('@components/SelectionList/SelectionListWithSections', () => jest.fn(() => null));
jest.mock('@components/SelectionList/ListItem/MultiSelectListItem', () => jest.fn(() => null));
jest.mock('@components/Icon', () => jest.fn(() => null));
jest.mock('@components/Icon/BankIcons', () => jest.fn(() => ({icon: jest.fn(() => null), iconSize: 0, iconStyles: []})));
jest.mock('@components/Search/FilterComponents/ListFilterViewWrapper', () => jest.fn(({children}: {children: React.ReactNode}) => children));

jest.mock('@hooks/useOnyx', () => jest.fn(() => [mockBankAccountList]));
jest.mock('@hooks/useDebouncedState', () => jest.fn(() => [mockSearchTerm, mockSearchTerm, jest.fn()]));
jest.mock('@hooks/useResponsiveLayout', () => jest.fn(() => ({isLargeScreenWidth: true})));
jest.mock('@hooks/useTheme', () => jest.fn(() => ({})));
jest.mock('@hooks/useThemeStyles', () => jest.fn(() => ({})));
jest.mock('@hooks/useLocalize', () =>
jest.fn(() => ({
translate: (key: string) => key,
})),
);
jest.mock('@src/types/utils/isLoadingOnyxValue', () => jest.fn(() => false));
jest.mock('@libs/BankAccountUtils', () => ({
// The mocked account carries its display label directly on `text`.
getBankAccountSearchLabel: (bankAccount: {text: string}) => bankAccount.text,
isFilterableBankAccount: () => true,
}));

describe('BankAccountSelector', () => {
const mockedSelectionList = jest.mocked(SelectionListWithSections);

// Build `count` open bank accounts keyed "1".."count" (IDs must be truthy, so they start at 1).
// `matcher` optionally decides which accounts' labels contain the word "match" so the search test can target them.
const buildBankAccounts = (count: number, matcher?: (key: string) => boolean) => {
const list: typeof mockBankAccountList = {};
for (let index = 1; index <= count; index++) {
const key = index.toString();
list[key] = {
accountData: {
bankAccountID: index,
accountNumber: `0000${index}`,
additionalData: {bankName: 'Chase'},
state: CONST.BANK_ACCOUNT.STATE.OPEN,
},
text: matcher?.(key) ? `match ${key}` : `Bank ${key}`,
};
}
return list;
};

const getSections = () => mockedSelectionList.mock.lastCall?.[0].sections ?? [];

beforeEach(() => {
mockedSelectionList.mockClear();
mockSearchTerm = '';
// A long list so the "move selected to top" behavior is enabled.
mockBankAccountList = buildBankAccounts(CONST.STANDARD_LIST_ITEM_LIMIT + 2);
});

it('floats the initially-selected bank account to the top of a long list', () => {
render(
<BankAccountSelector
value={['11']}
onChange={jest.fn()}
/>,
);

const sections = getSections();
// Top section holds only the pre-selected account.
expect(sections.at(0)?.data.map((item) => item.keyForList)).toEqual(['11']);
// Main section keeps every other account, with the pinned one removed.
expect(sections.at(1)?.data.map((item) => item.keyForList)).not.toContain('11');
expect(sections.at(1)?.data).toHaveLength(CONST.STANDARD_LIST_ITEM_LIMIT + 1);
});

it('keeps a bank account in place when it is toggled after first render (does not jump to the top)', () => {
const {rerender} = render(
<BankAccountSelector
value={['11']}
onChange={jest.fn()}
/>,
);

// Simulate the user toggling another account on: the parent re-renders the filter with the updated value.
rerender(
<BankAccountSelector
value={['11', '4']}
onChange={jest.fn()}
/>,
);

const sections = getSections();
// Only the originally pre-selected account stays pinned at the top.
expect(sections.at(0)?.data.map((item) => item.keyForList)).toEqual(['11']);

// The newly toggled account is marked selected but stays in its natural position (keys "1".."14" minus "11",
// so "4" is at index 3) instead of jumping up.
const mainSection = sections.at(1)?.data ?? [];
expect(mainSection.findIndex((item) => item.keyForList === '4')).toBe(3);
expect(mainSection.find((item) => item.keyForList === '4')?.isSelected).toBe(true);
});

it('keeps the initially-selected account pinned at the top while searching, and drops non-matching pinned accounts', () => {
// Only accounts "3", "11", and "12" have labels containing the search term. "11" is pinned and matches -> it
// stays at the top; "5" is pinned but does not match -> it is dropped.
mockSearchTerm = 'match';
mockBankAccountList = buildBankAccounts(CONST.STANDARD_LIST_ITEM_LIMIT + 2, (key) => key === '3' || key === '11' || key === '12');

render(
<BankAccountSelector
value={['11', '5']}
onChange={jest.fn()}
/>,
);

const sections = getSections();
// The pinned account that still matches the search stays at the top; the non-matching pinned account is dropped.
expect(sections.at(0)?.data.map((item) => item.keyForList)).toEqual(['11']);
// The main section shows the remaining matches with the pinned one removed (no duplicate).
expect(sections.at(1)?.data.map((item) => item.keyForList)).toEqual(['3', '12']);
});

it('does not pin selected accounts to the top of a short list', () => {
mockBankAccountList = buildBankAccounts(5);

render(
<BankAccountSelector
value={['2']}
onChange={jest.fn()}
/>,
);

const sections = getSections();
// No pinned section for a short list.
expect(sections.at(0)?.data).toHaveLength(0);
// Every account stays in place; the selected one is simply marked in position.
const mainSection = sections.at(1)?.data ?? [];
expect(mainSection.map((item) => item.keyForList)).toEqual(['1', '2', '3', '4', '5']);
expect(mainSection.find((item) => item.keyForList === '2')?.isSelected).toBe(true);
});

it('passes the expected selection behavior props to the list so a selected row does not clear the search, lose focus, or auto-scroll', () => {
render(
<BankAccountSelector
value={[]}
onChange={jest.fn()}
/>,
);

const props = mockedSelectionList.mock.lastCall?.[0];
expect(props?.shouldClearInputOnSelect).toBe(false);
expect(props?.shouldUpdateFocusedIndex).toBe(true);
expect(props?.shouldPreventAutoScrollOnSelect).toBe(true);
});
});
Loading
Loading