MetaMask / MetaMask/metamask-mobile

Add cancellation (`AbortController` / cancelled flag) to async effects that `setState` after resolve

Open
#31,495 1 comment 0 reactions 0 assignees View on GitHub
area-performance INVALID-ISSUE-TEMPLATE needs-engineer Sev2 Sev3 size-L ta-triaged team-accounts
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: 16 sweep hits app-wide

### What is this about?

Async effects that `fetch(...)`/`.then(...)` and set state on resolve, with no cancelled flag or `AbortController`, keep running after unmount (wasted work, "setState on unmounted component") and race when the driving input changes quickly (stale response overwrites fresh data). Sweep on `main` (`b3e7eb88`) found 16 candidate sites. Verified instances:

- [ ] `app/components/Views/ConnectQRHardware/index.tsx:129` — `KeyringController.getAccounts().then(...)`
- [ ] `app/components/Views/LedgerSelectAccount/index.tsx:135` and `:210`
- [ ] `app/components/Views/RevealPrivateCredential/RevealPrivateCredential.tsx:139`
- [ ] remaining 12 hits: re-run the detection recipe and triage (some may be guarded upstream)

Detection recipe:
`grep -rn -A6 "useEffect(" app --include="*.ts" --include="*.tsx" | grep -E "fetch\(|\.then\(" | grep -v "signal\|cancelled\|abort" | grep -v ".test."`

### Technical Details

Extract shared hooks ([`useIsMounted`](https://github.com/MetaMask/MetaMask-planning/issues/6544), [`useAbortableEffect`](https://github.com/MetaMask/MetaMask-planning/issues/6542)) so cleanup is default.

Fix per site: cancelled-flag cleanup (cheapest, works for any promise) or `AbortController` (also cancels the request; RN fetch supports `signal`).

### Acceptance Criteria

- All 16 hits triaged: fixed or waived with a stated reason (e.g. app-lifetime component).
- No "setState on unmounted component" warnings when rapidly entering/leaving the affected screens.

### References

- Source: `mms-performance` sweep recipes added in MetaMask/skills#49 (`mm-useeffect-antipatterns`)
- Status: **UNVALIDATED** per instance (static sweep)

Contributor guide

Open the contributing guide

Research direction

Start by rerunning the provided grep recipe on app/, then inspect the listed effects in app/components/Views/ConnectQRHardware/index.tsx, LedgerSelectAccount/index.tsx, and RevealPrivateCredential/RevealPrivateCredential.tsx. Triage all 16 hits, applying the stated cleanup approach or recording a waiver, then test rapid entry and exit of the affected screens for unmounted-state warnings.

Written by the indexing model from the issue text.

Assessment

Tech stack
react-native, typescript
Domain
frontend, mobile
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.