fix(desktop): register with macOS Notification Center at startup#1492
fix(desktop): register with macOS Notification Center at startup#1492Kitenite merged 9 commits intosuperset-sh:mainfrom
Conversation
On macOS 26.x, the app was not appearing in System Settings > Notifications because it never explicitly registered with Notification Center. Electron's Notification API doesn't have a requestPermission() method. Registration happens implicitly when notification.show() is called. This fix shows a silent notification at startup to force registration, then immediately closes it. The app will now appear in System Settings and notifications will be delivered properly. Fixes #1461
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change registers the Electron desktop app with macOS's notification center during startup and gates deep link processing until app initialization completes. This ensures the application appears in System Settings > Notifications on macOS. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/solutions/integration-issues/macos-notification-center-registration.md`:
- Around line 109-115: The Debug Mode docs use the wrong env var and command;
update the section to use SUPERSET_DEBUG=1 and the desktop run command (replace
ELECTRON_DEBUG_NOTIFICATIONS=1 bun run dev with SUPERSET_DEBUG=1 bun run
desktop) so it matches the debug implementation (see the SUPERSET_DEBUG usage in
apps/desktop/src/shared/debug.ts).
🧹 Nitpick comments (1)
apps/desktop/src/main/windows/main.ts (1)
183-209: Wrap the registration block in try-catch for resilience.This registration is best-effort and runs during
MainWindow()startup. Ifnew Notification()or.show()throws unexpectedly (e.g., on a CI/headless environment, or a future Electron version change), it would propagate and break window initialization. A defensive try-catch keeps the critical startup path safe.Proposed fix
// Register with macOS Notification Center on startup // This ensures the app appears in System Settings > Notifications // Fixes https://github.com/superset-sh/superset/issues/1461 if (PLATFORM.IS_MAC && Notification.isSupported()) { + try { const registrationNotification = new Notification({ title: productName, body: " ", silent: true, }); let handled = false; const cleanup = () => { if (handled) return; handled = true; registrationNotification.close(); }; registrationNotification.on("show", () => { cleanup(); console.log("[notifications] Registered with Notification Center"); }); // Fallback timeout in case macOS doesn't fire events setTimeout(cleanup, 1000); registrationNotification.show(); + } catch (error) { + console.warn("[notifications] Failed to register with Notification Center:", error); + } }
Update debug command from ELECTRON_DEBUG_NOTIFICATIONS=1 to SUPERSET_DEBUG=1 to match the actual debug implementation in apps/desktop/src/shared/debug.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/solutions/integration-issues/macos-notification-center-registration.md`:
- Around line 126-136: Update the Electron Notification Events Reference table's
"close" row to clarify platform reliability: change the Description for the
`close` event from "Notification closed" to something like "Notification closed
(may not be emitted on all platforms — reliability varies, see platform notes)"
or add a brief caveat immediately after the table explaining that the `close`
event is not guaranteed in all scenarios (notably some Windows edge cases);
ensure the change references the `close` event label in the "Electron
Notification Events Reference" table so readers can find the note.
On macOS 26.x, the app was not appearing in System Settings > Notifications because it never explicitly registered with Notification Center. Electron's Notification API doesn't have a requestPermission() method. Registration happens implicitly when notification.show() is called. This fix shows a silent notification at startup to force registration, then immediately closes it. The app will now appear in System Settings and notifications will be delivered properly. Fixes #1461
Update debug command from ELECTRON_DEBUG_NOTIFICATIONS=1 to SUPERSET_DEBUG=1 to match the actual debug implementation in apps/desktop/src/shared/debug.ts
zoumo
force-pushed
the
fix-notification
branch
from
f448a3f to
638d068
Compare
Note that the close event may not fire on all platforms.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/solutions/integration-issues/macos-notification-center-registration.md`:
- Around line 32-33: Change the definitive phrasing "macOS 26 changes — New
'Liquid Glass' notification architecture may require more explicit registration"
to a neutral observation; for example, replace "New 'Liquid Glass' notification
architecture" with "an observed behavior change in macOS 26.x (sometimes
referred to as 'Liquid Glass')" and use wording like "may require" or "appears
to require" instead of asserting the root cause; if you have an authoritative
source, add a citation after the sentence, otherwise keep the softer language
and update the heading text "macOS 26 changes" to "Observed macOS 26 behavior"
or similar.
| 4. **macOS 26 changes** - New "Liquid Glass" notification architecture may require more explicit registration | ||
|
|
There was a problem hiding this comment.
Rephrase speculative “Liquid Glass” root cause as an observation.
The current wording implies a confirmed architectural change. Consider softening to “observed behavior change in macOS 26.x” unless you can cite an authoritative source.
🤖 Prompt for AI Agents
In `@docs/solutions/integration-issues/macos-notification-center-registration.md`
around lines 32 - 33, Change the definitive phrasing "macOS 26 changes — New
'Liquid Glass' notification architecture may require more explicit registration"
to a neutral observation; for example, replace "New 'Liquid Glass' notification
architecture" with "an observed behavior change in macOS 26.x (sometimes
referred to as 'Liquid Glass')" and use wording like "may require" or "appears
to require" instead of asserting the root cause; if you have an authoritative
source, add a citation after the sentence, otherwise keep the softer language
and update the heading text "macOS 26 changes" to "Observed macOS 26 behavior"
or similar.
|
Thanks @zoumo, I removed the doc and moved the logic to start instead of per window but otherwise looks great! |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/index.ts (1)
299-304:⚠️ Potential issue | 🟡 MinorMinor race: a deep link arriving during
processDeepLinkon Line 300 can be lost.If an
open-urlevent fires whileawait processDeepLink(pendingDeepLinkUrl)yields (Line 300), the handler (Line 134) overwritespendingDeepLinkUrlwith the new URL. Then Line 301 nulls it out, and the new URL is never processed.This is an unlikely edge case, but if you'd like to close it:
Possible fix
- if (pendingDeepLinkUrl) { - await processDeepLink(pendingDeepLinkUrl); - pendingDeepLinkUrl = null; - } - - appReady = true; + appReady = true; + if (pendingDeepLinkUrl) { + const url = pendingDeepLinkUrl; + pendingDeepLinkUrl = null; + await processDeepLink(url); + }By setting
appReady = truefirst, any subsequentopen-urlevents will be processed directly viaprocessDeepLink(Line 132) instead of being queued and potentially overwritten.
🧹 Nitpick comments (1)
apps/desktop/src/main/index.ts (1)
97-122: Consider clearing the fallback timeout on successful cleanup.When the
"show"event fires promptly, the 1-secondsetTimeoutstill lingers. Storing the timer ID and clearing it insidecleanupis a small improvement.Also,
body: " "(a single space) means users may briefly see a notification toast before it's closed. If macOS allows it, an empty string or\u200B(zero-width space) might be less noticeable—though if an empty body prevents registration, the current approach is the right trade-off.Suggested tweak
function registerWithMacOSNotificationCenter() { if (!PLATFORM.IS_MAC || !Notification.isSupported()) return; const registrationNotification = new Notification({ title: app.name, - body: " ", + body: "\u200B", silent: true, }); let handled = false; + let fallbackTimer: ReturnType<typeof setTimeout> | null = null; const cleanup = () => { if (handled) return; handled = true; + if (fallbackTimer) clearTimeout(fallbackTimer); registrationNotification.close(); }; registrationNotification.on("show", () => { cleanup(); console.log("[notifications] Registered with Notification Center"); }); // Fallback timeout in case macOS doesn't fire events - setTimeout(cleanup, 1000); + fallbackTimer = setTimeout(cleanup, 1000); registrationNotification.show(); }
2 participants
Summary
requestPermission()- registration happens implicitly vianotification.show()Problem
On macOS 26.x (Tahoe), the Superset desktop app was not appearing in System Settings > Notifications, causing:
Solution
Added code to show a silent notification at app startup, which triggers registration with macOS Notification Center:
Testing
Files Changed
apps/desktop/src/main/windows/main.ts- Added registration code (~28 lines)docs/solutions/integration-issues/macos-notification-center-registration.md- Solution documentationFixes #1461
Summary by CodeRabbit
New Features
Bug Fixes