Skip to content

Commit c21e66e

Browse files
gantunesrclaude
gantunesr
and
claude
authored
perf: derive accounts/evmAccounts with useMemo instead of state+effect (#32721)
## **Description** `useAccounts` was deriving `accounts` and `evmAccounts` via `useState` + a `useEffect` that called `getAccounts()`. This caused an extra commit/render on every input change (account switch, account added, etc.) because React must flush the state update separately from the selector change. This PR replaces that pattern with `useMemo`: - `accounts` is now derived synchronously — no extra render - `evmAccounts` is a second `useMemo` over `accounts` - `ensByAccountAddress` stays as `useState` (populated asynchronously by ENS lookups) - ENS fetching moves to a standalone `useEffect` that depends on the memoized `accounts` The returned `accounts` reference is now stable across re-renders when `internalAccounts` and the selected address are unchanged, which prevents downstream consumers from invalidating their own memoization unnecessarily. ## **Changelog** CHANGELOG entry: null ## **Related issues** Fixes: #31373 ## **Manual testing steps** N/A — internal performance change; behaviour and output are identical. Covered by the existing unit tests plus a new referential-stability assertion (`yarn jest app/components/hooks/useAccounts/useAccounts.test.ts` — 9 tests, all passing). ## **Screenshots/Recordings** N/A — no UI change. ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I've included tests if applicable - [x] I've documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I've applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. #### Performance checks (if applicable) - [ ] I've tested on Android - Ideally on a mid-range device; emulator is acceptable - [ ] I've tested with a power user scenario - Use these [power-user SRPs](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/edit-v2/401401446401?draftShareId=9d77e1e1-4bdc-4be1-9ebb-ccd916988d93) to import wallets with many accounts and tokens - [ ] I've instrumented key operations with Sentry traces for production performance metrics - See [`trace()`](/app/util/trace.ts) for usage and [`addToken`](/app/components/Views/AddAsset/components/AddCustomToken/AddCustomToken.tsx#L274) for an example For performance guidelines and tooling, see the [Performance Guide](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/400085549067/Performance+Guide+for+Engineers). ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > `useAccounts` is used broadly across wallet UI; the refactor should be behavior-equivalent but changes timing of ENS fetches and loading empty-list handling, so regressions in account lists or ENS would surface in many flows. > > **Overview** > Refactors **`useAccounts`** so `accounts` and `evmAccounts` are **synchronously derived** with `useMemo` instead of `useState` plus a `useEffect` that called `getAccounts()`. That removes an extra render/commit when Redux account inputs change and keeps the **`accounts` array reference stable** when `internalAccounts` and the selected address are unchanged—helping consumers that memoize on `accounts` (e.g. bridge, account selector, browser). > > `ensByAccountAddress` stays async `useState`; ENS reverse lookups now run from a **dedicated `useEffect`** keyed on the memoized `accounts` (and `fetchENS`), rather than being triggered inside account derivation. While loading, **`isLoading` yields an empty `accounts` list** via the memo instead of skipping updates in the effect. > > Adds a unit test asserting **`accounts` keeps the same reference** across benign re-renders when inputs are unchanged. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit fab91a8. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 056a3fd commit c21e66e

2 files changed

Lines changed: 48 additions & 55 deletions

File tree

‎app/components/hooks/useAccounts/useAccounts.test.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,13 @@ describe('useAccounts', () => {
135135
expect(result.current.accounts[0].scopes).toStrictEqual([EthScope.Eoa]);
136136
});
137137

138+
it('returns a stable accounts reference when inputs are unchanged', () => {
139+
const { result, rerender } = renderHook(() => useAccounts());
140+
const firstRef = result.current.accounts;
141+
rerender({});
142+
expect(result.current.accounts).toBe(firstRef);
143+
});
144+
138145
describe('fetchENS parameter', () => {
139146
it('fetches ENS names when fetchENS is true (default)', async () => {
140147
const expectedENSNames = {

‎app/components/hooks/useAccounts/useAccounts.ts‎

Lines changed: 41 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
// Third party dependencies.
2-
import { useCallback, useEffect, useRef, useState } from 'react';
2+
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
33
import { useSelector } from 'react-redux';
44
import { KeyringTypes } from '@metamask/keyring-controller';
55

@@ -34,8 +34,6 @@ const useAccounts = ({
3434
fetchENS = true,
3535
}: UseAccountsParams = {}): UseAccounts => {
3636
const isMountedRef = useRef(false);
37-
const [accounts, setAccounts] = useState<Account[]>([]);
38-
const [evmAccounts, setEVMAccounts] = useState<Account[]>([]);
3937
const [ensByAccountAddress, setENSByAccountAddress] =
4038
useState<EnsByAccountAddress>({});
4139
const currentChainId = useSelector(selectChainId);
@@ -101,67 +99,55 @@ const useAccounts = ({
10199
[currentChainId],
102100
);
103101

104-
const getAccounts = useCallback(() => {
105-
if (!isMountedRef.current) return;
106-
// Keep track of the Y position of account item. Used for scrolling purposes.
102+
const accounts = useMemo((): Account[] => {
103+
if (isLoading) return [];
107104
let yOffset = 0;
108-
let selectedIndex = 0;
109-
const flattenedAccounts: Account[] = internalAccounts.map(
110-
(internalAccount: InternalAccount, index: number) => {
111-
const formattedAddress =
112-
getFormattedAddressFromInternalAccount(internalAccount);
113-
const isSelected =
114-
selectedInternalAccount?.address === internalAccount.address;
115-
if (isSelected) {
116-
selectedIndex = index;
117-
}
105+
return internalAccounts.map((internalAccount: InternalAccount) => {
106+
const formattedAddress =
107+
getFormattedAddressFromInternalAccount(internalAccount);
108+
const isSelected =
109+
selectedInternalAccount?.address === internalAccount.address;
118110

119-
const mappedAccount: Account = {
120-
id: internalAccount.id,
121-
name: internalAccount.metadata.name,
122-
address: formattedAddress,
123-
type: internalAccount.metadata.keyring.type as KeyringTypes,
124-
yOffset,
125-
isSelected,
126-
// This only works for EOAs
127-
caipAccountId: `${internalAccount.scopes[0]}:${internalAccount.address}`,
128-
scopes: internalAccount.scopes,
129-
snapId: internalAccount.metadata.snap?.id,
130-
isLoadingAccount: false,
131-
};
132-
// Calculate height of the account item.
133-
yOffset += 78;
134-
if (internalAccount.metadata.keyring.type !== KeyringTypes.hd) {
135-
yOffset += 24;
136-
}
137-
return mappedAccount;
138-
},
139-
);
111+
const mappedAccount: Account = {
112+
id: internalAccount.id,
113+
name: internalAccount.metadata.name,
114+
address: formattedAddress,
115+
type: internalAccount.metadata.keyring.type as KeyringTypes,
116+
yOffset,
117+
isSelected,
118+
// This only works for EOAs
119+
caipAccountId: `${internalAccount.scopes[0]}:${internalAccount.address}`,
120+
scopes: internalAccount.scopes,
121+
snapId: internalAccount.metadata.snap?.id,
122+
isLoadingAccount: false,
123+
};
124+
// Calculate height of the account item.
125+
yOffset += 78;
126+
if (internalAccount.metadata.keyring.type !== KeyringTypes.hd) {
127+
yOffset += 24;
128+
}
129+
return mappedAccount;
130+
});
131+
}, [isLoading, internalAccounts, selectedInternalAccount?.address]);
140132

141-
setAccounts(flattenedAccounts);
142-
setEVMAccounts(
143-
flattenedAccounts.filter((account) => !isNonEvmAddress(account.address)),
144-
);
145-
if (fetchENS) {
146-
fetchENSNames({ flattenedAccounts, startingIndex: selectedIndex });
147-
}
148-
}, [
149-
internalAccounts,
150-
fetchENS,
151-
fetchENSNames,
152-
selectedInternalAccount?.address,
153-
]);
133+
const evmAccounts = useMemo(
134+
() => accounts.filter((account) => !isNonEvmAddress(account.address)),
135+
[accounts],
136+
);
154137

155138
useEffect(() => {
156-
if (!isMountedRef.current) {
157-
isMountedRef.current = true;
139+
isMountedRef.current = true;
140+
if (fetchENS && accounts.length > 0) {
141+
const selectedIndex = accounts.findIndex((a) => a.isSelected);
142+
fetchENSNames({
143+
flattenedAccounts: accounts,
144+
startingIndex: selectedIndex >= 0 ? selectedIndex : 0,
145+
});
158146
}
159-
if (isLoading) return;
160-
getAccounts();
161147
return () => {
162148
isMountedRef.current = false;
163149
};
164-
}, [getAccounts, isLoading]);
150+
}, [fetchENS, fetchENSNames, accounts]);
165151

166152
return {
167153
accounts,

0 commit comments

Comments
 (0)