fix: bridge asset picker and input component performance cp-12.20.0 #33507
+381
−237
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✨ Files requiring CODEOWNER review ✨🔄 @MetaMask/swaps-engineers (8 files, +300 -187)
🖥️ @MetaMask/wallet-ux (5 files, +81 -50)
|
Builds ready [21a1279]
UI Startup Metrics (1231 ± 72 ms)
Benchmark value 1217 exceeds gate value 1190 for chrome browserify home p95 load Benchmark value 1210 exceeds gate value 1180 for chrome browserify home p95 domContentLoaded Benchmark value 1215 exceeds gate value 1180 for chrome browserify home p95 firstPaint Benchmark value 960 exceeds gate value 940 for chrome browserify home p95 loadScripts Benchmark value 53 exceeds gate value 32 for chrome webpack home mean setupStore Benchmark value 2561 exceeds gate value 2454 for chrome webpack home p95 uiStartup Benchmark value 312 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 40 exceeds gate value 38 for firefox webpack home mean firstReactRender Sum of mean exceeds: 24ms | Sum of p95 exceeds: 466ms Sum of all benchmark exceeds: 490ms Bundle size diffs
|
davidmurdoch
requested changes
Jun 6, 2025
ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
Show resolved
Hide resolved
Builds ready [9c7b31b]
UI Startup Metrics (1234 ± 64 ms)
Benchmark value 25 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Benchmark value 36 exceeds gate value 32 for chrome webpack home mean setupStore Benchmark value 2523 exceeds gate value 2454 for chrome webpack home p95 uiStartup Benchmark value 300 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 1410 exceeds gate value 1405 for firefox browserify home mean uiStartup Benchmark value 1244 exceeds gate value 1239 for firefox browserify home mean domContentLoaded Benchmark value 11 exceeds gate value 9 for firefox browserify home mean setupStore Benchmark value 1780 exceeds gate value 1660 for firefox browserify home p95 uiStartup Benchmark value 1598 exceeds gate value 1495 for firefox browserify home p95 load Benchmark value 1598 exceeds gate value 1495 for firefox browserify home p95 domContentLoaded Benchmark value 1582 exceeds gate value 1475 for firefox browserify home p95 loadScripts Benchmark value 34 exceeds gate value 27 for firefox browserify home p95 setupStore Benchmark value 43 exceeds gate value 38 for firefox webpack home mean firstReactRender Sum of mean exceeds: 22ms | Sum of p95 exceeds: 751ms Sum of all benchmark exceeds: 773ms Bundle size diffs
|
GustavoRSSilva
previously approved these changes
Jun 6, 2025
MajorLift
reviewed
Jun 6, 2025
ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
Show resolved
Hide resolved
MajorLift
reviewed
Jun 6, 2025
There was a problem hiding this comment.
Nit: The extra state variable and state update using useEffect seem unnecessary.
When something can be calculated from the existing props or state, don’t put it in state. Instead, calculate it during rendering. This makes your code faster (you avoid the extra “cascading” updates), simpler (you remove some code), and less error-prone (you avoid bugs caused by different state variables getting out of sync with each other).
-- https://react.dev/learn/you-might-not-need-an-effect#updating-state-based-on-props-or-state
diff --git a/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx b/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
index 7778e01d30..222e10f93f 100644
--- a/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
+++ b/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
@@ -157,28 +157,27 @@ export function AssetPickerModal({
const prevNeedsSolanaAccountRef = useRef(false);
const [searchQuery, setSearchQuery] = useState('');
- const [debouncedSearchQuery, setDebouncedSearchQuery] = useState(searchQuery);
const debouncedSetSearchQuery = useCallback(
- debounce((value) => {
- setDebouncedSearchQuery(value);
- }, 200),
+ debounce((value) => value, 200),
[],
);
+ const debouncedSearchQuery =
+ debouncedSetSearchQuery(searchQuery) ?? searchQuery;
const abortControllerRef = useRef<AbortController | null>(null);
- // Cleanup abort controller and debounce on unmount
+ // Cleanup abort controller on unmount
useEffect(() => {
return () => {
abortControllerRef.current?.abort();
abortControllerRef.current = null;
- debouncedSetSearchQuery.cancel();
};
}, []);
+ // clean up the debounced function on unmount or when `searchQuery` changes
useEffect(() => {
- debouncedSetSearchQuery(searchQuery);
- }, [searchQuery, debouncedSetSearchQuery]);
+ return () => debouncedSetSearchQuery.cancel();
+ }, [searchQuery]);
const swapsBlockedTokens = useSelector(getSwapsBlockedTokens);
const memoizedSwapsBlockedTokens = useMemo(() => {
There was a problem hiding this comment.
Good call - will try this out
MajorLift
reviewed
Jun 6, 2025
There was a problem hiding this comment.
Should we be passing the debounced value elsewhere in the file as well? There might be some performance gains to be had from reducing the frequency of child component props changes.
diff --git a/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx b/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
index 7778e01d30..f895896b99 100644
--- a/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
+++ b/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
@@ -516,7 +514,7 @@ export function AssetPickerModal({
// This fetches the metadata for the asset if it is not already in the filteredTokenList
const unlistedAssetMetadata = useAssetMetadata(
- searchQuery,
+ debouncedSearchQuery,
filteredTokenList.length === 0,
abortControllerRef,
selectedNetwork?.chainId,
@@ -666,7 +664,7 @@ export function AssetPickerModal({
<AssetPickerModalTabs {...tabProps}>
<React.Fragment key={TabName.TOKENS}>
<Search
- searchQuery={searchQuery}
+ searchQuery={debouncedSearchQuery}
onChange={(value) => {
// Cancel previous asset metadata fetch
abortControllerRef.current?.abort();
@@ -696,12 +694,12 @@ export function AssetPickerModal({
</React.Fragment>
<AssetPickerModalNftTab
key={TabName.NFTS}
- searchQuery={searchQuery}
+ searchQuery={debouncedSearchQuery}
onClose={onClose}
renderSearch={() => (
<Search
isNFTSearch
- searchQuery={searchQuery}
+ searchQuery={debouncedSearchQuery}
onChange={(value) => setSearchQuery(value)}
/>
)}
There was a problem hiding this comment.
We pass the searchQuery instead of the debounced value here so that the input fields render the typed value right away. If we used the debounced value, it would look like the ui takes some time after typing to update the input field
There was a problem hiding this comment.
Thanks for the explanation! Makes sense to keep the ui responsive, and it shouldn't affect memory usage either way, so lgtm to keep it as is.
|
@Gudahtt I attempted to optimize the expensive for loop in
Haven't benchmarked this yet, but it doesn't seem to break functionality at least. diff --git a/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx b/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
index 7778e01d30..5cad0ee345 100644
--- a/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
+++ b/ui/components/multichain/asset-picker-amount/asset-picker-modal/asset-picker-modal.tsx
@@ -157,28 +157,31 @@ export function AssetPickerModal({
const prevNeedsSolanaAccountRef = useRef(false);
const [searchQuery, setSearchQuery] = useState('');
- const [debouncedSearchQuery, setDebouncedSearchQuery] = useState(searchQuery);
const debouncedSetSearchQuery = useCallback(
- debounce((value) => {
- setDebouncedSearchQuery(value);
- }, 200),
+ debounce((value) => value, 200),
+ [],
+ );
+ const debouncedSearchQuery =
+ debouncedSetSearchQuery(searchQuery) ?? searchQuery;
+
+ const tokensWithBalanceDataCache = useMemo(
+ () => new Map<`${string}:${string}`, AssetWithDisplayData<ERC20Asset>>(),
[],
);
-
const abortControllerRef = useRef<AbortController | null>(null);
- // Cleanup abort controller and debounce on unmount
+ // Cleanup abort controller on unmount
useEffect(() => {
return () => {
abortControllerRef.current?.abort();
abortControllerRef.current = null;
- debouncedSetSearchQuery.cancel();
+ tokensWithBalanceDataCache.clear();
};
}, []);
+ // clean up the debounced function on unmount or when `searchQuery` changes
useEffect(() => {
- debouncedSetSearchQuery(searchQuery);
- }, [searchQuery, debouncedSetSearchQuery]);
+ return () => debouncedSetSearchQuery.cancel();
+ }, [searchQuery]);
const swapsBlockedTokens = useSelector(getSwapsBlockedTokens);
const memoizedSwapsBlockedTokens = useMemo(() => {
@@ -416,24 +419,23 @@ export function AssetPickerModal({
],
);
- const filteredTokenList = useMemo(() => {
- const filteredTokens: (
- | AssetWithDisplayData<ERC20Asset>
- | AssetWithDisplayData<NativeAsset>
- )[] = [];
- // List of token identifiers formatted like `chainId:address`
- const filteredTokensAddresses = new Set<string | undefined>();
- const getTokenKey = (address?: string | null, tokenChainId?: string) =>
+ // List of token identifiers formatted like `chainId:address`
+ const filteredTokensAddresses = useMemo(
+ () => new Set<string | undefined>(),
+ [],
+ );
+
+ const getTokenKey = useCallback(
+ (address?: string | null, tokenChainId?: string) =>
`${address?.toLowerCase() ?? zeroAddress()}:${
tokenChainId ?? currentChainId
- }`;
+ }` as `${string}:${string}`,
+ [currentChainId],
+ );
- // Default filter predicate for whether a token should be included in displayed list
- const shouldAddToken = (
- symbol: string,
- address?: string | null,
- tokenChainId?: string,
- ) => {
+ // Default filter predicate for whether a token should be included in displayed list
+ const defaultShowAddToken = useCallback(
+ (symbol: string, address?: string | null, tokenChainId?: string) => {
const trimmedSearchQuery = debouncedSearchQuery.trim().toLowerCase();
const isMatchedBySearchQuery = Boolean(
!trimmedSearchQuery ||
@@ -449,12 +451,27 @@ export function AssetPickerModal({
isMatchedBySearchQuery &&
!filteredTokensAddresses.has(getTokenKey(address, tokenChainId)),
);
- };
+ },
+ [
+ debouncedSearchQuery,
+ filteredTokensAddresses,
+ getTokenKey,
+ isMultiselectEnabled,
+ selectedChainIds,
+ selectedNetwork?.chainId,
+ ],
+ );
+
+ const filteredTokenList = useMemo(() => {
+ const filteredTokens: (
+ | AssetWithDisplayData<ERC20Asset>
+ | AssetWithDisplayData<NativeAsset>
+ )[] = [];
// If filteredTokensGenerator is passed in, use it to generate the filtered tokens
// Otherwise use the default tokenGenerator
const tokenGenerator = (customTokenListGenerator ?? tokenListGenerator)(
- shouldAddToken,
+ defaultShowAddToken,
);
for (const token of tokenGenerator) {
@@ -462,25 +479,33 @@ export function AssetPickerModal({
continue;
}
- filteredTokensAddresses.add(getTokenKey(token.address, token.chainId));
-
- const tokenWithBalanceData =
- !customTokenListGenerator && isStrictHexString(token.address)
- ? getRenderableTokenData(
- token.address
- ? ({
- ...token,
- ...evmTokenMetadataByAddress[token.address.toLowerCase()],
- type: AssetType.token,
- } as AssetWithDisplayData<ERC20Asset>)
- : token,
- tokenConversionRates,
- conversionRate,
- currentCurrency,
- token.chainId,
- evmTokenMetadataByAddress,
- )
- : (token as unknown as AssetWithDisplayData<ERC20Asset>);
+ const tokenKey = getTokenKey(token.address, token.chainId);
+ filteredTokensAddresses.add(tokenKey);
+
+ let tokenWithBalanceData;
+ if (tokensWithBalanceDataCache.has(tokenKey)) {
+ tokenWithBalanceData = tokensWithBalanceDataCache.get(tokenKey);
+ } else {
+ tokenWithBalanceData =
+ !customTokenListGenerator && isStrictHexString(token.address)
+ ? getRenderableTokenData(
+ token.address
+ ? ({
+ ...token,
+ ...evmTokenMetadataByAddress[token.address.toLowerCase()],
+ type: AssetType.token,
+ } as AssetWithDisplayData<ERC20Asset>)
+ : token,
+ tokenConversionRates,
+ conversionRate,
+ currentCurrency,
+ token.chainId,
+ evmTokenMetadataByAddress,
+ )
+ : (token as unknown as AssetWithDisplayData<ERC20Asset>);
+
+ tokensWithBalanceDataCache.set(tokenKey, tokenWithBalanceData);
+ }
// Add selected asset to the top of the list if it is the selected asset
if (
@@ -497,12 +522,14 @@ export function AssetPickerModal({
}
}
+ filteredTokensAddresses.clear();
+
return filteredTokens;
}, [
- currentChainId,
- debouncedSearchQuery,
- isMultiselectEnabled,
- selectedChainIds,
+ tokensWithBalanceDataCache,
+ filteredTokensAddresses,
+ getTokenKey,
+ defaultShowAddToken,
selectedNetwork?.chainId,
customTokenListGenerator,
tokenListGenerator,
|
|
Neat! Maybe we can try that in a subequent PR @MajorLift ? |
micaelae
commented
Jun 6, 2025
| } | ||
| // Only tokens on the active chain are processed here here | ||
| const sharedFields = { | ||
| ...token, |
There was a problem hiding this comment.
Moved this to outside the hook to remove the closure that was retaining references to the token data
Builds ready [59e374d]
UI Startup Metrics (1214 ± 75 ms)
Benchmark value 53 exceeds gate value 32 for chrome webpack home mean setupStore Benchmark value 2539 exceeds gate value 2454 for chrome webpack home p95 uiStartup Benchmark value 312 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 41 exceeds gate value 38 for firefox webpack home mean firstReactRender Sum of mean exceeds: 24ms | Sum of p95 exceeds: 333ms Sum of all benchmark exceeds: 357ms Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [d1d928c]
UI Startup Metrics (1220 ± 74 ms)
Benchmark value 1196 exceeds gate value 1190 for chrome browserify home p95 load Benchmark value 1186 exceeds gate value 1180 for chrome browserify home p95 domContentLoaded Benchmark value 22 exceeds gate value 18 for chrome browserify home p95 backgroundConnect Benchmark value 941 exceeds gate value 940 for chrome browserify home p95 loadScripts Benchmark value 43 exceeds gate value 32 for chrome webpack home mean setupStore Benchmark value 2483 exceeds gate value 2454 for chrome webpack home p95 uiStartup Benchmark value 306 exceeds gate value 65 for chrome webpack home p95 setupStore Benchmark value 12 exceeds gate value 11 for firefox browserify home mean getState Benchmark value 26 exceeds gate value 24 for firefox browserify home p95 getState Benchmark value 41 exceeds gate value 38 for firefox webpack home mean firstReactRender Sum of mean exceeds: 15ms | Sum of p95 exceeds: 323ms Sum of all benchmark exceeds: 338ms Bundle size diffs
|
micaelae
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Issues
Related issues
Fixes: #33466 (partially)
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist