Skip to content

fix: Initial scroll to selected account for MultichainAccountSelectorList #19880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 17, 2025
+65 −92
Merged

fix: Initial scroll to selected account for MultichainAccountSelectorList #19880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
104 changes: 45 additions & 59 deletions ...p/MultichainAccounts/MultichainAccountSelectorList/MultichainAccountSelectorList.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import React from 'react';
import { fireEvent, waitFor } from '@testing-library/react-native';
import { fireEvent, waitFor, within } from '@testing-library/react-native';
import '@shopify/flash-list/jestSetup';
import {
AccountGroupObject,
AccountWalletObject,
} from '@metamask/account-tree-controller';
import { InternalAccount } from '@metamask/keyring-internal-api';
import MultichainAccountSelectorList from './MultichainAccountSelectorList';
import { FlashListRef } from '@shopify/flash-list';
import { FlattenedMultichainAccountListItem } from './MultichainAccountSelectorList.types';
import renderWithProvider from '../../../../util/test/renderWithProvider';
import {
MULTICHAIN_ACCOUNT_SELECTOR_SEARCH_INPUT_TESTID,
Expand All @@ -20,6 +19,7 @@ import {
createMockInternalAccountsFromGroups,
createMockInternalAccountsWithAddresses,
} from '../test-utils';
import { ReactTestInstance } from 'react-test-renderer';

jest.mock('../../../../core/Engine', () => ({
context: {
Expand Down Expand Up @@ -117,7 +117,7 @@ describe('MultichainAccountSelectorList', () => {
const { getByText } = renderComponentWithMockState(
[wallet1, wallet2],
internalAccounts,
[account1],
[],
);

expect(getByText('Wallet 1')).toBeTruthy();
Expand Down Expand Up @@ -145,7 +145,7 @@ describe('MultichainAccountSelectorList', () => {
const { getByText } = renderComponentWithMockState(
[srpWallet, snapWallet],
internalAccounts,
[srpAccount],
[],
);

expect(getByText('Wallet 1')).toBeTruthy();
Expand Down Expand Up @@ -173,7 +173,7 @@ describe('MultichainAccountSelectorList', () => {
const { getByText } = renderComponentWithMockState(
[srpWallet, ledgerWallet],
internalAccounts,
[srpAccount],
[],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to remove this since having the selected account here meant that the other list items were not in the initial view and therefore not searchable. Nothing functionally changed though.

);

expect(getByText('Wallet 1')).toBeTruthy();
Expand Down Expand Up @@ -201,11 +201,15 @@ describe('MultichainAccountSelectorList', () => {
const { getAllByTestId } = renderComponentWithMockState(
[wallet1],
internalAccounts,
[account2],
[],
);

const accountCells = getAllByTestId('multichain-account-cell-container');
fireEvent.press(accountCells[0]);
const account1Cell = accountCells.find((cell) =>
within(cell).queryByText('Account 1'),
);
expect(account1Cell).toBeTruthy();
fireEvent.press(account1Cell as ReactTestInstance);

expect(mockOnSelectAccount).toHaveBeenCalledWith(account1);
});
Expand Down Expand Up @@ -437,7 +441,7 @@ describe('MultichainAccountSelectorList', () => {
const { getByTestId, queryByText } = renderComponentWithMockState(
[wallet1, wallet2],
internalAccounts,
[account1],
[],
);

// Initially all accounts should be visible
Expand Down Expand Up @@ -717,7 +721,7 @@ describe('MultichainAccountSelectorList', () => {
expect(getByText('Create account')).toBeTruthy();
});

it('scrolls to the first selected account', async () => {
it('positions the list so the first selected account is initially visible', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I verified that this test fails if we remove the auto scroll logic

const account1 = createMockAccountGroup(
'keyring:wallet1/group1',
'Account 1',
Expand All @@ -735,57 +739,39 @@ describe('MultichainAccountSelectorList', () => {
account1,
account2,
]);
const { queryByText } = renderWithProvider(
<MultichainAccountSelectorList
onSelectAccount={mockOnSelectAccount}
selectedAccountGroups={[account2]}
/>,
{ state: createMockState([wallet1], internalAccounts) },
);

type TestFlashListRef = FlashListRef<FlattenedMultichainAccountListItem>;
const listRef = React.createRef<TestFlashListRef>();

// Mock RAF to queue callbacks so we can attach our mock ref before flushing
const rafCallbacks: FrameRequestCallback[] = [];
const rafSpy = jest
.spyOn(global, 'requestAnimationFrame')
.mockImplementation((cb: FrameRequestCallback) => {
rafCallbacks.push(cb);
return rafCallbacks.length as unknown as number;
});
expect(queryByText('Account 2')).toBeTruthy();
});
it('renders a far selected account in the initial viewport when provided as initial selection', () => {
// Create many accounts so the selected one is far enough to require initialScrollIndex
const total = 60;
const accounts = Array.from({ length: total }, (_, i) =>
createMockAccountGroup(
`keyring:wallet1/group${i + 1}`,
`Account ${i + 1}`,
),
);
const selectedIdx = 36;
const selected = accounts[selectedIdx];
const wallet1 = createMockWallet('wallet1', 'Wallet 1', accounts);

const internalAccounts = createMockInternalAccountsFromGroups(accounts);

const { queryByText } = renderWithProvider(
<MultichainAccountSelectorList selectedAccountGroups={[selected]} />,
{ state: createMockState([wallet1], internalAccounts) },
);

try {
renderWithProvider(
<MultichainAccountSelectorList
onSelectAccount={mockOnSelectAccount}
selectedAccountGroups={[account2]}
listRef={listRef}
/>,
{ state: createMockState([wallet1], internalAccounts) },
);

// Attach mock imperative methods after render, before running RAF
interface MinimalFlashListRef {
scrollToIndex: jest.Mock;
scrollToOffset: jest.Mock;
scrollToEnd: jest.Mock;
}
const mockRefImpl: MinimalFlashListRef = {
scrollToIndex: jest.fn(),
scrollToOffset: jest.fn(),
scrollToEnd: jest.fn(),
};
const mockRef = mockRefImpl as unknown as TestFlashListRef;
(listRef as unknown as { current: TestFlashListRef | null }).current =
mockRef;

// Flush queued RAF callbacks to trigger the scroll effect
rafCallbacks.forEach((cb) => cb(0));

await waitFor(() => {
expect(mockRefImpl.scrollToIndex).toHaveBeenCalledWith({
index: 2, // header (0), Account 1 (1), Account 2 (2), footer (3)
animated: true,
viewPosition: 0.5,
});
});
} finally {
rafSpy.mockRestore();
}
// Without initialScrollIndex, this would not be visible initially
expect(queryByText(`Account ${selectedIdx + 1}`)).toBeTruthy();
expect(queryByText('Account 1')).toBeFalsy();
});
});
});
43 changes: 15 additions & 28 deletions ...s-temp/MultichainAccounts/MultichainAccountSelectorList/MultichainAccountSelectorList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import React, {
} from 'react';
import { View, ScrollViewProps } from 'react-native';
import { ScrollView } from 'react-native-gesture-handler';
import { FlashList, ListRenderItem } from '@shopify/flash-list';
import { FlashList, ListRenderItem, FlashListRef } from '@shopify/flash-list';
import { useSelector } from 'react-redux';
import { AccountGroupObject } from '@metamask/account-tree-controller';

Expand Down Expand Up @@ -54,7 +54,8 @@ const MultichainAccountSelectorList = ({
const [lastCreatedAccountId, setLastCreatedAccountId] = useState<
string | null
>(null);
const internalListRef = useRef(null);
const internalListRef =
useRef<FlashListRef<FlattenedMultichainAccountListItem>>(null);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just better typing on this file. Not strictly needed.

const listRefToUse = listRef || internalListRef;

const selectedIdSet = useMemo(
Expand Down Expand Up @@ -155,6 +156,16 @@ const MultichainAccountSelectorList = ({
return items;
}, [filteredWalletSections]);

// Compute first selected account index for initial positioning only
const initialSelectedIndex = useMemo(() => {
const targetId = selectedAccountGroups?.[0]?.id;
if (!targetId) return undefined;
const idx = flattenedData.findIndex(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

returns the first available index that matches the search query so this will not break for multi select.

(item) => item.type === 'cell' && item.data.id === targetId,
);
return idx > 0 ? idx : undefined;
Copy link

Choose a reason for hiding this comment

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

Bug: FlashList Scroll Issue at Index Zero

The initialSelectedIndex calculation uses idx > 0, which incorrectly returns undefined when the target account is found at index 0. This prevents the FlashList from scrolling to an item at the very beginning of the list, even though 0 is a valid index.

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this too but after some thought this makes more sense. The default behaviour of the FlashList is to render the list at the top (0 index). Therefore by returning undefined when the selected account is the 0 index, we are simply saying, "resort to your default behaviour". I did not verify this but my assumption was that there are some small performance benefits to not providing that prop in this default case. This is why I opted to return undefined here.

}, [flattenedData, selectedAccountGroups]);

// Reset scroll to top when search text changes
useEffect(() => {
if (listRefToUse.current) {
Expand Down Expand Up @@ -189,31 +200,6 @@ const MultichainAccountSelectorList = ({
}
}, [lastCreatedAccountId, flattenedData, listRefToUse]);

// Scroll to the first selected account whenever selection or data changes
useEffect(() => {
if (debouncedSearchText.trim()) return;
if (!listRefToUse.current) return;
if (!selectedAccountGroups?.length) return;

const targetId = selectedAccountGroups[0]?.id;
if (!targetId) return;

const index = flattenedData.findIndex(
(item) => item.type === 'cell' && item.data.id === targetId,
);

if (index !== -1) {
const raf = requestAnimationFrame(() => {
listRefToUse.current?.scrollToIndex({
index,
animated: true,
viewPosition: 0.5,
});
});
return () => cancelAnimationFrame(raf);
}
}, [selectedAccountGroups, flattenedData, debouncedSearchText, listRefToUse]);

// Handle account creation callback
const handleAccountCreated = useCallback((newAccountId: string) => {
setLastCreatedAccountId(newAccountId);
Expand All @@ -229,7 +215,7 @@ const MultichainAccountSelectorList = ({

const renderItem: ListRenderItem<FlattenedMultichainAccountListItem> =
useCallback(
({ item }) => {
({ item }: { item: FlattenedMultichainAccountListItem }) => {
switch (item.type) {
case 'header': {
return <AccountListHeader title={item.data.title} />;
Expand Down Expand Up @@ -332,6 +318,7 @@ const MultichainAccountSelectorList = ({
showsVerticalScrollIndicator={false}
getItemType={getItemType}
keyExtractor={keyExtractor}
initialScrollIndex={initialSelectedIndex}
renderScrollComponent={
ScrollView as React.ComponentType<ScrollViewProps>
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,7 @@
"@sentry/core": "~8.54.0",
"@sentry/react": "~8.54.0",
"@sentry/react-native": "~6.10.0",
"@shopify/flash-list": "2.0.1",
"@shopify/flash-list": "2.0.3",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Release info. Needed to fix the unit tests since the mock that was exported from flash-list was returning undefined.

"@solana/addresses": "2.0.0",
"@tradle/react-native-http": "2.0.1",
"@types/he": "^1.2.3",
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8404,10 +8404,10 @@
dependencies:
"@sentry/core" "8.54.0"

"@shopify/flash-list@2.0.1":
version "2.0.1"
resolved "https://registry.yarnpkg.com/@shopify/flash-list/-/flash-list-2.0.1.tgz#cde79aa41e9f677e1e5cdc542fe4f304a7338397"
integrity sha512-bLgwTcVOn1kzSL4q0qk6JxE+4je1xF2yUDVoIKmeI9miFPAvIxzI8cCoksUrGmRt/MXRj4mrOhAjvxQY1lSoVg==
"@shopify/flash-list@2.0.3":
version "2.0.3"
resolved "https://registry.yarnpkg.com/@shopify/flash-list/-/flash-list-2.0.3.tgz#222427d1e09bf5cdd8a219d0a5a80f6f1d20465d"
integrity sha512-jUlHuZFoPdqRCDvOqsb2YkTttRPyV8Tb/EjCx3gE2wjr4UTM+fE0Ltv9bwBg0K7yo/SxRNXaW7xu5utusRb0xA==
dependencies:
tslib "2.8.1"

Expand Down
Loading