feat(notifications): route all notifications through the in-app toast (from #349) - #402
Conversation
Replace Electron's native Notification with an API-compatible custom `Notification` class that drives our own frameless BrowserWindow toast (the existing /notification renderer route), so every notification is visually consistent and theme-aware on macOS and Windows instead of an OS banner. All 8 existing call sites keep their title/body/click/action behavior unchanged (the class emits the same click/action/close events, so trackNotificationLifecycle and each site's handlers still work); each gets an appropriate iconType (alert/success/recording). The pre-meeting reminder migrates off the standalone createNotificationWindow onto the same class (marked premeeting:true to keep its Join/focus + own analytics), and the generic action/body-click bridge is wired via two new send channels (notification-action-clicked / notification-body-clicked). Includes the closure-bug fix: the ready-to-show/closed closures capture the window in a local `win` instead of reading the module-level notificationWindow, so a fast-following toast can't make an earlier one act on the wrong window. Keeps the 15s auto-close and passive-dismiss analytics. Single-toast (superseding) semantics preserved. Cross-platform: reuses the window config the pre-meeting toast already shipped on both platforms; the macOS-only visibleOnFullScreen option and 'screen-saver' level are harmlessly ignored on Windows. Co-authored-by: Vassista <vassista@icloud.com>
…n types Render title/body/actions/iconType in the toast. Generic notifications show action buttons (wired to main via notification.actionClicked) and a state icon (alert/success/recording via lucide, or the brand AppIcon for the default), and route body taps through notification.bodyClicked. The pre-meeting reminder keeps its bespoke Join & take notes / focus path and its renderer-side analytics, selected by the premeeting flag. Add a `color` prop to AppIcon so the toast can tint the brand mark, and a small vitest covering the iconType -> icon mapping. Co-authored-by: Vassista <vassista@icloud.com>
There was a problem hiding this comment.
All reported issues were addressed across 6 files
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…wn toast Read the pre-meeting 'close' handler's interaction flag from the notification's own window (notif._window) instead of the module-level notificationWindow, which a superseding toast reassigns (resetting _analyticsInteracted) before the earlier toast's 'close' fires — otherwise the dismissal is attributed to the successor's flag, dropping or duplicating notification_dismissed telemetry. Also assert the default-branch className in the notificationIconMeta test so the unknown-iconType fallback is covered symmetrically with the other cases. Co-authored-by: Vassista <vassista@icloud.com>
Optic00
left a comment
There was a problem hiding this comment.
Verdict: request-changes - the shadow-class approach is right and the closure work is careful (including the 13ba916 per-instance analytics fix), but the flagged behavior change is a real data-loss path for one notification, and the single-toast model drops concurrent notices while analytics reports them shown.
Summary
Replaces Electron's native Notification with an API-compatible EventEmitter class rendering into the existing frameless toast window, so all 8 call sites and trackNotificationLifecycle keep working unchanged. Two new IPC channels relay action/body taps back to main-side handlers. The pre-meeting toast moves onto the same class.
Findings
Blocker
- Sleep-paused "Recording paused" must not be a 15s toast. It fires exactly when the user is away: the machine slept, they come back minutes later, the toast (and its Resume button) is long gone - or a pre-meeting/note-ready toast superseded it - and the recording silently stays paused for the rest of the meeting. Previously this persisted in Notification Center. Suggestion that keeps the PR's shape: a
persistent: trueoption on the shadow class (skip the auto-close timer, survive supersession or re-present while still paused), set for sleep-paused and arguably note-ready (also typically fires after the user walked away).
Important
- Single-toast supersede drops concurrent notifications, while analytics still counts them as shown.
show()closes the current toast;notification_shownis tracked before the renderer even loads, and the superseded toast'sclosedhandler then records a dismiss the user never saw. Real sequence: silence auto-stop → note-ready lands moments later; only the last survives.notifications.t2keeps passing because each call returnsshown: true. Even a small FIFO (show next on close) plus emitting "shown" only after presentation would fix both the UX and the numbers. - Native notification semantics silently dropped: no sound (
silentis ignored), no OS Focus/Do-Not-Disturb respect - and withvisibleOnFullScreen+'screen-saver'level the toast overlays fullscreen presentations, where the OS would have suppressed a banner. Part of this is the acknowledged tradeoff, but DND-bypass during a fullscreen presentation is the kind that produces bug reports; worth an explicit decision in the PR text, not just the sound line. - Keyboard accessibility:
focusable: falsemeans Resume / Take Notes / Summarise can never receive keyboard focus - native notifications were reachable via Notification Center. Pre-existing for the pre-meeting toast, but this PR extends it to every actionable notification. - IPC hygiene on the new channels:
notification-action-clicked/notification-body-clicked(and the pre-existingclose-notification-window) act on the toast without verifyingevent.sender === notificationWindow.webContents, and an unknownactionIdstill emitsactionwith index-1. With #387 adding sender guards elsewhere, the one-line check +index < 0rejection belongs here too.
Nit
- Toast always opens on the primary display (
screen.getPrimaryDisplay()); a user presenting on an external monitor may never see it. Pre-existing pattern - followingmainWindow's display would fix it cheaply. - The
actionevent emits({}, index); current Electron exposesdetails.actionIndexand deprecates the positional arg. No caller reads it today, but it undercuts the API-compatible claim - cheap to pass{ actionIndex: index }, index. getEventColorgives generic toasts a hash-of-title random accent bar (green/purple/pink/…), which reads as meaningful but isn't and clashes with the no-chromatic-accent brand rule; theiconTypetint already carries the state. A neutral bar for non-premeeting toasts would fit the design system.- If
ready-to-shownever fires (load failure), the window leaks invisibly with no auto-close armed;loadFile()'s rejection is also unobserved. Adid-fail-load/.catch→close()caps it.
Praise
- The closure-bug fix is exactly right: capturing
winlocally plus registeringclosedimmediately closes the superseded-before-load hole the old code had. - The analytics split (renderer-tracked active dismiss vs main-side passive dismiss,
_analyticsInteractedscoped per window) is preserved faithfully through a gnarly refactor. - The API-compatible shadow class keeps the blast radius to one file section;
ipc-contractand the icon-mapping vitest confirm the seams.
Summary
Routes all app notifications through Steno's own frameless React toast (the one already used for pre-meeting reminders), instead of native OS notifications. Extracted from @Vassista's #349, adapted and made cross-platform-safe.
Approach (low-blast-radius)
Rather than rip out every call site, this replaces Electron's native
Notificationwith an API-compatible shadow class (extendsEventEmitter, samenew Notification({title,body,…})/.show()/.close()/.on('click'|'action'|'close')/static isSupported()). All 8 existing call sites andtrackNotificationLifecyclekeep working unchanged; two new send channels relay action/body taps back so each site's handlers + analytics still fire. The pre-meeting reminder moves onto the same class.All 8 sites preserved with equivalent actions + icon types: shortcut, sleep-paused (Resume), silence auto-stop, system-audio mic-only (→Settings), note-ready (→note), meeting-detected (Take Notes), meeting-ended (Summarise), recording-failed.
Cross-platform
Reuses the exact window config the pre-meeting toast already shipped on both OSes (transparent, alwaysOnTop, skipTaskbar, focusable:false, top-right via workArea DIPs). The mac-only bits (
visibleOnFullScreen,'screen-saver'level) are ignored on Windows; the mac-only shortcut-notification gate is untouched.Closure-bug fix
show()captures the window locally soclosed/auto-close closures never touch a module-level ref a superseding toast reassigned; theclosedhandler is registered immediately so a superseded-before-load toast still emitsclose. 15s auto-close kept; passive dismissals still tracked.Tests
NotificationToast.tsx+app-icon.tsxcolor prop; new vitest for the icon mapping (5/5).notifications.t2still passes (the enabled-toggleshown-signal gating intact).typecheck:renderer+ipc-contract(7/7) green.The 7 previously-native notifications no longer appear in the OS Notification Center, have no notification sound, and auto-dismiss after 15s. Most consequential for the sleep-paused "Recording paused" notice, which previously persisted until dismissed. This trade-off is inherent to the feature (in-app toast vs. native). Flagging for an explicit decision before merge — everything else is behavior-preserving.
Credit: @Vassista (#349).
Summary by cubic
Route all app notifications through our in‑app toast instead of native OS banners. Preserves existing handlers and makes notifications consistent on macOS and Windows.
New Features
Notificationreplacement renders a frameless toast; all 8 call sites continue to work.premeetingflag). Generic toasts relay body/actions vianotification-body-clickedandnotification-action-clicked; added state icons (alert,success,recording) and optional action buttons;AppIconsupports acolorprop.Bug Fixes
closealways applies to the right toast; maintains single‑toast supersede behavior and passive‑dismiss tracking.notif._window.Written for commit 41a4f07. Summary will update on new commits.