fix(notifications): fix macOS desktop notifications

[schulzf]

Main process:

• import Notification statically instead of using require at runtime
• add Notification.isSupported() guard before creating notifications
• skip custom icon on macOS (app icon is used automatically)
• register click handler before calling show() to avoid race condition

Notifications hook:

• add notifyWhenFocused preference to control notifications when app is focused
• apply focus check to notifyAgentComplete, notifyAgentNeedsInput, and notifyPlanReady

Active chat:

• move notifyAgentComplete call outside the sound-enabled block so notifications fire independently of sound preference
• skip notification when agent was manually aborted

Settings UI:

• add "Notify When Focused" toggle to agents preferences tab, disabled when desktop notifications are off

Atoms:

• add notifyWhenFocusedAtom persisted to localStorage (default: false)

[Danverr]

@1code review this pr

[1code-async]

Task completed.

Here's my review of PR #163:

---

PR Review: fix(notifications): fix macOS desktop notifications

Summary

This PR fixes macOS desktop notifications and adds a "Notify When Focused" preference. The changes are well-scoped and address real issues.

Positive Changes

1. Static import of Notification (main/windows/main.ts): Replacing require("electron") at runtime with a static import is the correct approach. Runtime require inside an IPC handler is unnecessary when the module is already imported at the top of the file.

2. Notification.isSupported() guard: Good defensive check before attempting to create a notification.

3. Skip custom icon on macOS: Correct — macOS uses the app icon automatically, and setting a custom .ico icon on macOS can cause issues. The per-platform extension handling (icon.ico for Windows, icon.png for Linux) is also an improvement.

4. Register click handler before show(): This fixes a real race condition. Registering the click handler after show() means a fast click could be missed. Moving show() after the handler registration is the right fix.

5. Skip notification on manual abort (active-chat.tsx): Sensible — users don't want a "completed" notification when they cancelled the operation themselves.

6. Decouple notification from sound preference (active-chat.tsx): Moving notifyAgentComplete outside the soundEnabled block is correct — these are independent preferences.

Issues / Suggestions

1. Icon path is likely wrong (bug)

const iconPath = join(__dirname, "../../build", ext)

The original path was join(__dirname, "../../../build/icon.ico"). The new path changes from ../../../build to ../../build. Was this intentional? In a packaged Electron app, __dirname for the main process points to the compiled output directory. Changing the relative path depth could break icon resolution on Windows/Linux. This should be verified against the actual build output structure.

2. Duplicated code block in active-chat.tsx

The same notification logic appears twice (around lines 6105 and 6304 in the diff). This is pre-existing duplication, not introduced by this PR, but worth noting — the two blocks are identical and could diverge over time.

3. notifyWhenFocused naming could be clearer

The name notifyWhenFocused could be read as "notify only when focused" rather than "also notify when focused." Something like showNotificationsWhileFocused or notifyEvenWhenFocused would be less ambiguous. Minor point.

4. document.hasFocus() granularity

The focus check uses document.hasFocus(), which returns true if the Electron window is focused — even if the user is in a completely different chat tab. The settings description says "e.g. when working in a different chat," implying per-chat awareness, but the implementation doesn't distinguish which chat is focused. This is a UX gap rather than a bug, and probably fine for a v1.

5. Spread conditional for icon

...(icon && { icon }),

This works but icon will be undefined when skipped on macOS, and passing icon: undefined to the Notification constructor would also be harmless. The conditional spread is fine but not strictly necessary.

Verdict

The core fixes (static import, isSupported() guard, click handler race condition, decoupling notification from sound) are all solid. The icon path change (../../build vs ../../../build) is the main thing I'd want verified before merging — if that path is wrong, notifications on Windows/Linux will silently lose their icon. Everything else looks good.

---

View full conversation

Powered by 1Code