Skip to content

Commit 933cd09

Browse files
feat: implement safe state management for notification hooks (MetaMask#40401)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** Root cause: whenever any team performs async local state updates and the component unmounts, you will receive this react error (this does eventually get cleaned up and isn't a breaking error). E.g. 1. Component Mounts 2. Component calls an async function which once finished will update its local state (useState) 3. Component unmounts 4. Async function finished, and tries calling setState (but component was unmounted!) -- react error is logged (not breaking and eventually cleaned). This update introduces a new `useSafeState` hook to manage state updates safely while components are mounted, preventing potential memory leaks and reduced the opportunity for `React: State updates on unmounted components` errors. The `useListNotifications`, `useCreateNotifications`, `useEnableNotifications`, and `useDisableNotifications` hooks have been refactored to utilize `useSafeState` for loading, error, and notifications data states. This change enhances the stability and reliability of notifications handling in the application. If this hook is useful, we can move this for other teams to use. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/40401?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: feat: implement safe state management for notification hooks ## **Related issues** Fixes: https://consensys.slack.com/archives/CTQAGKY5V/p1771942084169109 https://github.com/MetaMask/metamask-extension/actions/runs/22353733972/job/64687641620?pr=40327 ## **Manual testing steps** ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Small, localized React hook change that only affects how async notification actions update local state; minimal behavioral impact beyond preventing post-unmount state updates. > > **Overview** > Adds an internal `useSafeState` hook in `useNotifications.ts` that gates `setState` calls behind a mounted check to avoid React warnings/memory leaks when async work completes after unmount. > > Refactors `useListNotifications`, `useCreateNotifications`, `useEnableNotifications`, and `useDisableNotifications` to use `useSafeState` for `loading`/`error`/`notificationsData`, and updates `useCallback` dependency arrays accordingly. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 2b153c2. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 4c2c0cc commit 933cd09

File tree

1 file changed

+46
-12
lines changed

1 file changed

+46
-12
lines changed

ui/hooks/metamask-notifications/useNotifications.ts

Lines changed: 46 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,11 @@
1-
import { useState, useCallback } from 'react';
1+
import {
2+
useState,
3+
useRef,
4+
useEffect,
5+
useCallback,
6+
type Dispatch,
7+
type SetStateAction,
8+
} from 'react';
29
import { useDispatch } from 'react-redux';
310
import type { InternalAccount } from '@metamask/keyring-internal-api';
411
import log from 'loglevel';
@@ -18,6 +25,34 @@ import {
1825
updateNotificationSubscriptionExpiration,
1926
} from '../../contexts/metamask-notifications/notification-storage-keys';
2027

28+
/**
29+
* Internal: useState that only applies updates while mounted. Prevents
30+
* "state updates on unmounted components" when async work completes after unmount.
31+
*
32+
* @param initialValue - The initial value of the state.
33+
*/
34+
function useSafeState<TValue>(
35+
initialValue: TValue,
36+
): [TValue, Dispatch<SetStateAction<TValue>>] {
37+
const [state, setState] = useState<TValue>(initialValue);
38+
const isMountedRef = useRef(true);
39+
40+
useEffect(() => {
41+
isMountedRef.current = true;
42+
return () => {
43+
isMountedRef.current = false;
44+
};
45+
}, []);
46+
47+
const setSafeState = useCallback((value: SetStateAction<TValue>) => {
48+
if (isMountedRef.current) {
49+
setState(value);
50+
}
51+
}, []);
52+
53+
return [state, setSafeState];
54+
}
55+
2156
// Define KeyringType interface
2257
type KeyringType = {
2358
type: string;
@@ -44,9 +79,9 @@ export function useListNotifications(): {
4479
} {
4580
const dispatch = useDispatch();
4681

47-
const [loading, setLoading] = useState<boolean>(false);
48-
const [error, setError] = useState<unknown>(null);
49-
const [notificationsData, setNotificationsData] = useState<
82+
const [loading, setLoading] = useSafeState<boolean>(false);
83+
const [error, setError] = useSafeState<unknown>(null);
84+
const [notificationsData, setNotificationsData] = useSafeState<
5085
INotification[] | undefined
5186
>(undefined);
5287

@@ -72,7 +107,7 @@ export function useListNotifications(): {
72107
} finally {
73108
setLoading(false);
74109
}
75-
}, [dispatch]);
110+
}, [dispatch, setLoading, setError, setNotificationsData]);
76111

77112
return {
78113
listNotifications,
@@ -93,7 +128,7 @@ export function useCreateNotifications(): {
93128
error: string | null;
94129
} {
95130
const dispatch = useDispatch();
96-
const [error, setError] = useState<string | null>(null);
131+
const [error, setError] = useSafeState<string | null>(null);
97132

98133
const createNotifications = useCallback(async () => {
99134
setError(null);
@@ -106,7 +141,7 @@ export function useCreateNotifications(): {
106141
log.error(e);
107142
throw e;
108143
}
109-
}, [dispatch]);
144+
}, [dispatch, setError]);
110145

111146
return {
112147
createNotifications,
@@ -130,7 +165,7 @@ export function useEnableNotifications(): {
130165
} {
131166
const dispatch = useDispatch();
132167

133-
const [error, setError] = useState<string | null>(null);
168+
const [error, setError] = useSafeState<string | null>(null);
134169

135170
const enableNotifications = useCallback(async () => {
136171
setError(null);
@@ -143,7 +178,7 @@ export function useEnableNotifications(): {
143178
log.error(e);
144179
throw e;
145180
}
146-
}, [dispatch]);
181+
}, [dispatch, setError]);
147182

148183
return {
149184
enableNotifications,
@@ -162,8 +197,7 @@ export function useDisableNotifications(): {
162197
error: string | null;
163198
} {
164199
const dispatch = useDispatch();
165-
166-
const [error, setError] = useState<string | null>(null);
200+
const [error, setError] = useSafeState<string | null>(null);
167201

168202
const disableNotifications = useCallback(async () => {
169203
setError(null);
@@ -176,7 +210,7 @@ export function useDisableNotifications(): {
176210
log.error(e);
177211
throw e;
178212
}
179-
}, [dispatch]);
213+
}, [dispatch, setError]);
180214

181215
return {
182216
disableNotifications,

0 commit comments

Comments
 (0)