[Fix] Hoist IPC call out of notification setState updater#492
Conversation
GeneralSection.handleNotificationToggle was performing window.clawwork.updateSettings(...) inside a setState updater. React treats updaters as pure functions and double-invokes them in Strict Mode (dev) to detect impurity, so every notification toggle was firing the IPC settings persist twice. Issue clawwork-ai#400 has the full reproducer. Compute the next state once, dispatch the state update, then dispatch the IPC call as a separate effect. notifyState moves into the useCallback deps so the closure stays in sync with the stored value across re-renders. Closes clawwork-ai#400. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Hi @mvanhorn, DetailsInstructions for interacting with me using comments are available here. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a side-effect issue within the notification settings toggle logic. By moving the IPC communication outside of the state updater function, it ensures that settings persistence is only triggered once per interaction, preventing redundant calls caused by React's Strict Mode behavior. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the handleNotificationToggle function to move the IPC call outside of the state updater. While this prevents double-invocation in React Strict Mode, it introduces a potential race condition where rapid toggles could result in stale state overwriting updates. Feedback suggests using useEffect or a ref to safely synchronize state with the backend. Additionally, it is recommended to add error handling and success notifications to align with the component's existing patterns.
| const next = { ...notifyState, [key]: value }; | ||
| setNotifyState(next); | ||
| window.clawwork.updateSettings({ notifications: next }); | ||
| }, | ||
| [], | ||
| [notifyState], |
There was a problem hiding this comment.
Hoisting the IPC call out of the setNotifyState updater successfully prevents double-invocation in React Strict Mode. However, this implementation introduces a potential race condition because it relies on notifyState from the closure. If a user toggles multiple settings quickly, notifyState might be stale during the second click (before the first update's re-render), causing the first update to be overwritten in the final state.
To maintain state consistency while keeping the side effect out of the updater, consider using a useEffect hook to sync state to the backend, or use a ref to track the latest state value for the IPC call.
| }); | ||
| const next = { ...notifyState, [key]: value }; | ||
| setNotifyState(next); | ||
| window.clawwork.updateSettings({ notifications: next }); |
There was a problem hiding this comment.
For consistency with other setting handlers in this component (e.g., handleThemeToggle at line 38 or handleDensityChange at line 46), consider adding a success toast after the settings are updated, and handle potential IPC errors to inform the user if the persistence fails.
1 participant
Summary
Closes #400.
GeneralSection.handleNotificationTogglewas performingwindow.clawwork.updateSettings(...)inside asetStateupdater. Reacttreats updaters as pure functions and double-invokes them in Strict Mode
(dev) to detect impurity, so every notification toggle was firing the IPC
settings persist twice -- the issue body has the full diagnosis.
Implementation
Match the fix recipe in the issue:
const handleNotificationToggle = useCallback( (key: 'taskComplete' | 'approvalRequest' | 'gatewayDisconnect', value: boolean) => { - setNotifyState((prev) => { - const next = { ...prev, [key]: value }; - window.clawwork.updateSettings({ notifications: next }); - return next; - }); + const next = { ...notifyState, [key]: value }; + setNotifyState(next); + window.clawwork.updateSettings({ notifications: next }); }, - [], + [notifyState], );Compute the next state once, dispatch the state update, then dispatch
the IPC call as a separate side effect.
notifyStatemoves into theuseCallbackdeps so the closure stays in sync with the stored valueacross re-renders.
Verification
pnpm checkwas not run because corepack downloaded pnpm 11.0.9 in thisenvironment, which exits non-zero on every workspace-level invocation
(unrelated to this change). The individual steps that
pnpm checkchains run cleanly when invoked directly via
node/npx tsc/npx eslint/npx prettier. Pinning pnpm or running on the project'sexpected toolchain should produce the same green result.
Notes
changes.
the user-facing acceptance from the issue's "Verification" section --
flagging that I did not run the desktop app locally; happy to do that
if you'd like before merge.