Skip to content

fix: cancel flash-message timers; make store init() idempotent#14

Open
Teamingzooper wants to merge 1 commit into
feat/command-palette-actionsfrom
fix/audit-cleanups
Open

fix: cancel flash-message timers; make store init() idempotent#14
Teamingzooper wants to merge 1 commit into
feat/command-palette-actionsfrom
fix/audit-cleanups

Conversation

@Teamingzooper

Copy link
Copy Markdown
Owner

Summary

Two latent bugs spotted during the audit pass at the end of the recent feature batch. Both are real but minor — no user-visible breakage today, but they cause React warnings or accumulating listeners in edge cases.

Branch base: `feat/command-palette-actions` — ninth and final in the recent stack.

Bug 1 — Flash-message timers leak on unmount

`SettingsPanel` and `ThemeEditor` both use the pattern:

```ts
setMsg('Saved'); setTimeout(() => setMsg(null), 4000);
```

without ever cancelling the timer. If the user closes the settings modal within those 4 seconds, React 18 logs "can't perform a React state update on an unmounted component" and the pending setState runs on a stale component instance.

Fix: new `useFlash` hook in `src/renderer/hooks/useFlash.ts` that:

  • Stores the timer id in a `useRef`.
  • Cancels the timer on unmount.
  • Cancels any previous timer when called again (so two flashes in quick succession don't fight).

Both components now use it; all 8 raw `setTimeout(() => setX(null), ...)` sites are gone.

Bug 2 — Store `init()` accumulates listeners across retries

`store.init()` subscribes to five `window.nexus.onX(...)` events (unread, instance-activated, view-crashed, instance-hibernated, instance-woken). The Retry button on the error screen calls `init()` again, which would have added a second set of listeners — every event would then fire its state-update closure twice (or N times after N retries).

Fix: module-scoped `initDisposers` array. `init()` calls `disposeInitSubscriptions()` at the top before creating new ones. The `onX` IPC helpers already return a remove-listener disposer, so we just collect and replay them.

Test plan

  • `npm run typecheck` — clean
  • `npm run test` — 178/178 passing
  • `npm run build` — renderer + main both clean
  • Manual smoke: open Settings → click "Send test notification" → immediately close the modal (within 4s). DevTools console should be quiet (no "setState on unmounted component" warning).
  • Manual smoke: trigger an init error (e.g. kill the main process briefly), see the error screen, click Retry. Each retry should leave exactly one listener per event, not N.

🤖 Generated with Claude Code

…tent

Two latent bugs I noticed during the audit pass at the end of the
recent feature batch. Both are real but minor — no user-visible
breakage today, but they cause noisy warnings or accumulating
listeners in edge cases.

### Flash-message timers

SettingsPanel and ThemeEditor both used the pattern

  setX('Saved'); setTimeout(() => setX(null), 4000);

without ever cancelling the timer. If the component unmounted (user
closed the settings modal) within those 4 seconds, React 18 logged a
"can't perform a React state update on an unmounted component" warning
and the pending setState ran on a stale component instance.

New `useFlash` hook in `src/renderer/hooks/useFlash.ts` wraps the
pattern: stores the timer id in a ref, cancels it on unmount, and
cancels any previous timer when called again so two flashes in quick
succession don't fight. Both components now use it; all eight raw
setTimeout sites are gone.

### Store init() accumulating listeners

`store.init()` subscribes to five `window.nexus.onX(...)` events
(unread, instance-activated, view-crashed, instance-hibernated,
instance-woken). The Retry button on the error screen calls init()
again, which would have added a second set of listeners on top of
the first — every event would then fire its state-update closure
twice (or N times after N retries).

Now there's a module-scoped `initDisposers` array. init() tears down
any pre-existing subscriptions at the top before creating new ones.

No tests added — both fixes are in code paths that are hard to
exercise without renderer-side test infrastructure. Verified
manually that the audit findings no longer reproduce, typecheck +
178/178 unit tests + build all clean.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants