MetaMask / MetaMask/metamask-mobile
Add cleanup to timers and listeners registered in effects (batch sweep)
- Dominant language
- TypeScript
- Stars
- 3k
- Forks
- 1.7k
- Avg merge
- 1d 14h
- Merged PRs (30d)
- 669
Description
> **Performance audit finding** · Severity: **Medium (batch)** · Effort: Medium · Fix risk: Simple · Test safety net: Partial
> Owner: per-file CODEOWNERS
> Scope: timers and listeners registered in effects without cleanup, app-wide — GitHub code search sizes the triage surface at **39 files** co-locating `setInterval` with `useEffect` and **34 files** co-locating `addEventListener` with `useEffect` under `app/` (file-level co-occurrence; each hit needs a teardown check, many will be fine)
### What is this about?
Every `setInterval`, `setTimeout`, and `addEventListener` inside a `useEffect` must be torn down in the effect's cleanup function. A leaked interval keeps firing (and dispatching) after unmount; a leaked listener pins its closure — and everything the closure captured — out of garbage collection. Mount/unmount cycles stack additional copies. The extension's equivalent sweep (MetaMask-planning#6541) found 122 instances; mobile's known anchors:
- [ ] `app/core/SDKConnectV2/services/connection-registry.ts:487` — `AppState` listener without cleanup (catalogued in the `mms-performance` skill)
- [ ] Per-subscriber 1s connection-status interval in Predict live hooks (filed as #31363; pattern recurs elsewhere)
- [ ] Full sweep: run the detection recipe and triage every hit
Detection recipe (from the skill's `mm-useeffect-antipatterns` / audit playbook):
```bash
grep -rn "setInterval\|setTimeout\|addEventListener\|\.on(" app --include="*.ts" --include="*.tsx" | grep -v ".test." | grep -vE "clear|remove|off\("
```
For each hit, confirm a matching teardown (`return () => …` with `clearInterval`/`clearTimeout`/`removeEventListener`/`.off()`); fix or waive with a stated reason (app-lifetime singletons).
### Scenario
N/A — see Technical Details.
### Design
N/A — internal performance change; no UI/design impact.
### Technical Details
Fix per site: return the teardown from the effect. Once patterns repeat, prefer the shared-hook route already prescribed in #31495 (`useEventListener` with automatic cleanup, per the extension's MetaMask-planning#7166) so cleanup is the default rather than per-site diligence.
### Threat Modeling Framework
N/A — performance-only change; behavior is preserved, no new data flow / trust boundary / attack surface.
### Acceptance Criteria
- All sweep hits triaged: teardown added or waived with reason.
- Navigate-in/navigate-out N times on touched screens: timer/listener counts stay flat (heap snapshot or `js-memory-leaks` recipe); no post-unmount dispatches.
### References
- Sibling: #31495 (async-effect cancellation; shares the shared-hooks fix direction)
- Extension counterparts: MetaMask-planning#6541, #7166
- Source: `mms-performance` sweep recipes (MetaMask/skills#49, `mm-useeffect-antipatterns`)
- Status: **UNVALIDATED** per instance (static sweep)
Contributor guide
Research direction
Start with the detection recipe in the issue and inspect app/core/SDKConnectV2/services/connection-registry.ts:487, then triage each app/ hit for a matching teardown. Use the existing useEventListener direction from #31495 where applicable. Done means every hit has cleanup or a documented app-lifetime waiver, with navigation cycles showing flat timer/listener counts and no post-unmount dispatches.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- react-native, typescript
- Domain
- mobile, performance
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100