Description
src/features/settings/components/notifications/NotificationsTab.tsx's updateNotification saves the entire notification-settings object on every single toggle (the API is a full-object PUT, not a per-key patch):
const updateNotification = async (key: keyof NotificationSettings, value: boolean) => {
const previousValue = notifications[key]
// Optimistic Update
setNotifications((prev) => ({ ...prev, [key]: value }))
setUpdatingKeys((prev) => ({ ...prev, [key]: true }))
try {
const nextSettings = { ...notifications, [key]: value }
await updateNotificationSettings(nextSettings)
} catch (err) {
...The optimistic UI update correctly uses the functional form of setNotifications (prev => ({...})), but nextSettings — the object actually sent to the backend — is built from the notifications variable captured in this render's closure, not from the latest state. If a second toggle is triggered (e.g. a fast double-click on two adjacent switches, or a "select all"-style batch of calls) before this render's notifications closure is updated by React, both calls read the same pre-update notifications snapshot and each PUTs a full settings object missing the other call's change. Whichever request's response lands second silently overwrites the other toggle's change on the backend — the UI can show both switches "on" while the server only persisted one of them, with no error surfaced (the request itself succeeds; it just carries stale data).
enableAll in the same file has an analogous pattern, computing allEnabled from notifications read at call time rather than via a functional updater.
Requirements
updateNotification must build the payload it sends to updateNotificationSettings from the latest settings state, not a closure captured at handler-invocation time (e.g. by using a functional state update to read the current value, or a ref that always tracks the latest notifications).- Apply the same fix to
enableAll if it has the same closure-staleness issue.
Suggested execution
- Fork the repo and create a branch:
git checkout -b fix/notificationstab-stale-closure
- Change
updateNotification to compute nextSettings from the functional updater's result (e.g. capture it via a ref updated inside setNotifications(prev => {...; latestRef.current = next; return next}), or restructure to await the state update before building the payload).
- Apply the same pattern to
enableAll.
- Add a test that rapidly triggers two different
updateNotification calls in the same tick and asserts the final updateNotificationSettings call includes both changes, not just the last one.
Example commit message
fix: build NotificationsTab save payload from latest state, not a stale closure
Acceptance criteria
Security notes
None; this is a data-integrity/correctness issue — a dropped notification-preference change is low severity, but the underlying pattern (full-object PUT built from a stale closure) is worth flagging since it would be more serious on a payload with higher-stakes fields.
Guidelines
- Minimum 95% test coverage
- Timeframe: 96 hours
Description
src/features/settings/components/notifications/NotificationsTab.tsx'supdateNotificationsaves the entire notification-settings object on every single toggle (the API is a full-objectPUT, not a per-key patch):The optimistic UI update correctly uses the functional form of
setNotifications(prev => ({...})), butnextSettings— the object actually sent to the backend — is built from thenotificationsvariable captured in this render's closure, not from the latest state. If a second toggle is triggered (e.g. a fast double-click on two adjacent switches, or a "select all"-style batch of calls) before this render'snotificationsclosure is updated by React, both calls read the same pre-updatenotificationssnapshot and eachPUTs a full settings object missing the other call's change. Whichever request's response lands second silently overwrites the other toggle's change on the backend — the UI can show both switches "on" while the server only persisted one of them, with no error surfaced (the request itself succeeds; it just carries stale data).enableAllin the same file has an analogous pattern, computingallEnabledfromnotificationsread at call time rather than via a functional updater.Requirements
updateNotificationmust build the payload it sends toupdateNotificationSettingsfrom the latest settings state, not a closure captured at handler-invocation time (e.g. by using a functional state update to read the current value, or a ref that always tracks the latestnotifications).enableAllif it has the same closure-staleness issue.Suggested execution
git checkout -b fix/notificationstab-stale-closureupdateNotificationto computenextSettingsfrom the functional updater's result (e.g. capture it via a ref updated insidesetNotifications(prev => {...; latestRef.current = next; return next}), or restructure to await the state update before building the payload).enableAll.updateNotificationcalls in the same tick and asserts the finalupdateNotificationSettingscall includes both changes, not just the last one.Example commit message
Acceptance criteria
enableAllis verified to not have the same stale-closure issue (or is fixed alongside).Security notes
None; this is a data-integrity/correctness issue — a dropped notification-preference change is low severity, but the underlying pattern (full-object PUT built from a stale closure) is worth flagging since it would be more serious on a payload with higher-stakes fields.
Guidelines