MetaMask / MetaMask/metamask-mobile
Stabilize useTokenLogo default Set params and memoize returned handlers
- Dominant language
- TypeScript
- Stars
- 3k
- Forks
- 1.7k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 669
Description
> **Performance audit finding** · Severity: **Medium** · Effort: Easy · Fix risk: Simple · Test safety net: Covered (app/components/hooks/useTokenLogo/useTokenLogo.test.ts)
> Owner: `@MetaMask/metamask-assets (suggested)`
> File: `app/components/hooks/useTokenLogo/useTokenLogo.ts:38`
### What is this about?
`useTokenLogo` has two interacting issues:
1. **Default params create fresh `Set`s every render**, busting downstream memoization:
```ts
export const useTokenLogo = ({
symbol,
size = 44,
assetsRequiringLightBg = new Set(), // line 38 — new Set each render when omitted
assetsRequiringDarkBg = new Set(), // line 39 — new Set each render when omitted
}: UseTokenLogoConfig): UseTokenLogoReturn => {
```
These two `Set`s are dependencies of the `needsLightBg/needsDarkBg` `useMemo` (line 58), so when a caller omits them the memo recomputes on every render — and that result feeds the `containerStyle` `useMemo` (line 80), so the container style object is rebuilt every render too.
2. **The three event handlers are recreated every render** and the return is a fresh object literal:
```ts
const handleLoadStart = () => { setIsLoading(true); setHasError(false); }; // line 111
const handleLoadEnd = () => { setIsLoading(false); }; // line 116
const handleError = () => { setIsLoading(false); setHasError(true); }; // line 120
return { isLoading, hasError, containerStyle, ..., handleLoadStart, handleLoadEnd, handleError }; // line 125
```
All the styles are carefully `useMemo`'d, but the handlers (passed straight to ``) get a new identity each render, so any memoized child `Image` re-renders anyway.
**Why it matters**
`useTokenLogo` backs `TrendingTokenLogo` (`app/components/UI/Trending/components/TrendingTokenLogo/TrendingTokenLogo.tsx`), which renders one logo per row in trending-token lists — a hot, virtualized list path. The fresh-`Set` default defeats the style memoization the hook author intentionally added, and the unstable handlers defeat `Image` prop stability, so each row recomputes styles and re-binds image callbacks on every render of the list.
### Scenario
N/A — see Technical Details.
### Design
N/A — internal performance change; no UI/design impact.
### Technical Details
**Evidence**
`app/components/hooks/useTokenLogo/useTokenLogo.ts:38`
```ts
assetsRequiringLightBg = new Set(),
assetsRequiringDarkBg = new Set(),
```
`app/components/hooks/useTokenLogo/useTokenLogo.ts:52`
```ts
const { needsLightBg, needsDarkBg } = useMemo(() => { ... },
[symbol, assetsRequiringLightBg, assetsRequiringDarkBg]); // fresh Sets bust this
```
`app/components/hooks/useTokenLogo/useTokenLogo.ts:111` handlers are inline, returned at line 125.
**Fix**
1. Hoist the empty-`Set` defaults to module-level constants so the default identity is stable:
```ts
const EMPTY_SET = new Set();
... assetsRequiringLightBg = EMPTY_SET, assetsRequiringDarkBg = EMPTY_SET ...
```
2. Wrap `handleLoadStart`/`handleLoadEnd`/`handleError` in `useCallback` with empty deps (they only call stable setters).
### Threat Modeling Framework
N/A — performance-only change; behavior is preserved, no new data flow / trust boundary / attack surface.
### Acceptance Criteria
- - Run `yarn jest app/components/hooks/useTokenLogo/useTokenLogo.test.ts`.
- Add a test that renders the hook twice (omitting the `Set` params) and asserts `containerStyle`, `handleLoadStart`, etc. are `===` across renders.
### References
- File: `app/components/hooks/useTokenLogo/useTokenLogo.ts:38`
- Source: MetaMask Mobile performance audit — finding `unstablehook-usetokenlogo-default-set-params-bust-memo`
- Owner (CODEOWNERS / best-effort): @MetaMask/metamask-assets (suggested)
- Status: **UNVALIDATED**
Contributor guide
Research direction
Start with app/components/hooks/useTokenLogo/useTokenLogo.ts, focusing on the defaults, memoized styles, and returned handlers. Run yarn jest app/components/hooks/useTokenLogo/useTokenLogo.test.ts and add the described repeated-render coverage; done means omitted Set parameters and returned style and handler values retain identity without changing behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- react-native, typescript
- Domain
- frontend
- Issue type
- Refactor
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 76/100