MetaMask / MetaMask/metamask-mobile

Add cleanup to timers and listeners registered in effects (batch sweep)

Open
#31,509 1 comment 0 reactions 0 assignees View on GitHub
area-performance needs-engineer Sev2 Sev3 size-L ta-triaged team-mobile-platform
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.