Skip to content

fix: surface notification failure to renderer on macOS#185

Merged
johannesjo merged 2 commits into
johannesjo:mainfrom
xbrxr03:fix/macos-notification-surface-error
Jun 24, 2026
Merged

fix: surface notification failure to renderer on macOS#185
johannesjo merged 2 commits into
johannesjo:mainfrom
xbrxr03:fix/macos-notification-surface-error

Conversation

@xbrxr03

@xbrxr03 xbrxr03 commented Jun 22, 2026

Copy link
Copy Markdown

Summary

Surface notification failed events to the renderer so the UI can inform the user when a notification did not deliver.

On macOS, unsigned development builds silently drop notifications because UNNotification requires code-signing. The failed event currently fires only on Windows (Electron 40). Starting with Electron 42, it will fire on macOS too — this PR lays the groundwork by wiring the IPC channel now.

Refs #122

Changes

  • Add IPC.NotificationFailed channel + preload allowlist entry
  • In register.ts, forward the failed event to the renderer via NotificationFailed (includes error string + platform)
  • In desktopNotifications.ts, add a listener that logs a macOS-specific console.warn explaining unsigned-build requirements
  • Update the watcher-replacement test to cover both NotificationClicked and NotificationFailed listener cleanup

Reviewer notes

  • taskIds removed from the NotificationFailed payload — the renderer listener does not use them
  • Comments updated to note the Electron 42+ scope for macOS
  • The console.warn in the renderer is a DevTools-visible diagnostic. A proper toast/banner can follow when the settings-level alerts design lands

@johannesjo

Copy link
Copy Markdown
Owner

Thanks for digging into this — the diagnosis is sharp. You correctly spotted that the requestNotificationPermission() fix proposed in #122 is obsolete and that code-signing is the real constraint on modern Electron. The diff is clean, the win.isDestroyed() guard and String(error) coercion are right, and the test update properly models the new two-listeners-per-watcher ordering.

One blocking concern before this can land as a fix for #122:

The failed-on-macOS behavior only exists in Electron 42+, but this repo is on Electron 40

The whole mechanism here depends on notification.on('failed') firing for unsigned macOS builds. That behavior — and the code-signing requirement — was introduced by the NSUserNotificationUNUserNotificationCenter migration that shipped in Electron 42.0:

  • Breaking change "macOS notifications now use UNNotification API (42.0)": "Electron has migrated from the deprecated NSUserNotification API to the UNNotification API on macOS. The new API requires that an application be code-signed in order for notifications to be displayed." (breaking-changes, electron#47817)
  • In the v40.8.5 docs the failed event is annotated _Windows_ only — it does not fire on macOS, and there's no code-signing note. (Compare with main, where it's now _macOS_ _Windows_.)

This repo pins electron: "^40.8.5" (resolved install: 40.8.5), and ^40 can't float to 42. So on the currently shipped build the new console.warn will essentially never fire on macOS — issue #122 users get the same silence as before. The PR description itself says "Electron 42+", which is the tell: the change is correct for a version we're not on yet.

Knock-on points

  • Closes #122 overstates it. Desktop Notifications Never Appear on macOS #122 asks for notifications to actually work (prompt → appear in System Settings → deliver). This is a diagnostic only, and an inert one on Electron 40. Suggest Refs #122 and leaving the issue open. Worth noting a collaborator reported notifications working on a npm run dev branch but not the released build — the inverse of what code-signing alone predicts, which hints the released-app signing/entitlement is the real gap to chase.
  • The user is never actually informed. Both code comments say "so the UI can inform the user", but the renderer only does console.warn, which lives in DevTools and is invisible to someone who just toggled a setting. Even on Electron 42 this wouldn't meet the stated intent without a real surface (settings banner / toast) driven off the event.
  • Minor: taskIds is forwarded on failure but unused by the renderer listener.

Suggested path

Any of: (a) hold this until the Electron 42 upgrade and bundle it there; (b) keep it but pair it with a user-facing surface instead of a DevTools log; or (c) merge as deliberately forward-looking groundwork — with Closes #122 downgraded and a comment noting it activates on Electron 42+.

Sources: Electron 42 release notes · electron/electron#47817 · Notification API docs · Breaking Changes

AJH763 added 2 commits June 23, 2026 15:03
On macOS, Electron uses UNNotification which requires code-signing.
Unsigned dev builds silently drop every notification and the user
has no idea why — they toggle the setting on and nothing happens.

Now the main process forwards the notification 'failed' event to the
renderer via a new NotificationFailed IPC channel. The renderer logs
a warning explaining that macOS dev builds need ad-hoc signing for
notifications to work (codesign --force --deep -s - <app>).

Closes johannesjo#122
…2+ scope

- Remove taskIds from NotificationFailed payload (unused by renderer)
- Add comment noting  event is macOS-inert on Electron 40
- Update renderer comment: this activates after Electron 42 upgrade
- Change PR scope: Refs johannesjo#122 (forward-looking groundwork, not full fix)
@xbrxr03 xbrxr03 force-pushed the fix/macos-notification-surface-error branch from dee2281 to 4d63b5f Compare June 23, 2026 19:04
@xbrxr03

xbrxr03 commented Jun 23, 2026

Copy link
Copy Markdown
Author

Thanks for the thorough review! I've pushed an update addressing all three points:

  1. Closes #122Refs #122 — Updated the PR description and scope. This is forward-looking groundwork that activates after an Electron 42 upgrade, not a full fix for the current version.

  2. Removed taskIds from the payload — The renderer listener doesn't use them, so they're gone. The NotificationFailed event now sends only error and platform.

  3. Updated all comments to explicitly note the Electron 42+ scope for macOS — both in register.ts and desktopNotifications.ts. The renderer handler logs a DevTools-visible console.warn for macOS. A proper toast/banner can land separately when the settings-level alerts design is ready.

All 1420 tests pass, typecheck + lint clean.

@johannesjo

Copy link
Copy Markdown
Owner

Thank you very much ! <3

@johannesjo johannesjo merged commit d1561ab into johannesjo:main Jun 24, 2026
2 checks passed
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.

3 participants