feat(notifications): focus-aware notification bus on quill toasts#2934
feat(notifications): focus-aware notification bus on quill toasts#2934adamleithp wants to merge 2 commits into
Conversation
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
77a5864 to
94658f5
Compare
|
Reviews (1): Last reviewed commit: "feat(notifications): focus-aware notific..." | Re-trigger Greptile |
Route every app notification through one bus that delivers by focus + what you're looking at: - viewing the exact target (task/canvas) → suppress - app focused but elsewhere → in-app toast (with a click-through) - app unfocused → native OS notification (click deep-links to the target) Core changes: - NotificationBus (renderer) replaces TaskNotificationService; pure, tested routeNotification() decides the tier from a generic NotificationTarget union (task | canvas) defined in @posthog/platform. - Native click routing generalized via a new OpenTargetLinkService + deep-link router onOpenTarget/getPendingOpenTarget; the renderer consumer drains pending on every (re)subscribe so a click during a reconnect/HMR gap still routes. - Task notifications (prompt-complete, permission) migrated through the bus; canvas-generation-done re-emits through it (detection kept). - Toast primitive rebuilt on @posthog/quill (provider-managed dismiss, stacking, auto-dismiss); all direct-sonner callers migrated and sonner dropped. - New Settings → Notifications section: defaults, OS-permission banner, reset-to-defaults, and a per-tier test harness. Canvas copy gated on the bluebird flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
94658f5 to
32bef75
Compare
adboio
left a comment
There was a problem hiding this comment.
not tested too extensively but seems legit!! left a few comments, nothing blocking but lmk what you think :)
| import type { NotificationTarget } from "@posthog/platform/notifications"; | ||
|
|
||
| // Whether two targets point at the same thing (same kind + ids). | ||
| export function targetsEqual( |
There was a problem hiding this comment.
[still reading code, maybe this is stupid, will update if so lol]
can/should we make this a bit more generic? like just give NotificationTarget some uniqueIdentifier property so we can just return a.kind === b.kind && a.uniqueId === b.uniqueId ? otherwise we'll have to update this if/when new target types are introduced
There was a problem hiding this comment.
edit: just looked at the NotificationTarget type, both task and canvas are effectively the same thing (seems like identifier + "sub-identifier" (? not familiar w/ canvas)
in that case, could we make it a bit more generic and maybe not even split out the kind on the target?
There was a problem hiding this comment.
Good call on the maintainability point — done in 64df565. Collapsed the per-kind equality chain into a single targetKey(target) identity function; targetsEqual is now just targetKey(a) === targetKey(b), so adding a kind touches one exhaustive switch (compile error if you forget) instead of growing an if-chain that silently returns false.
On dropping kind / a flat uniqueId: I kept kind because it's load-bearing for navigation, not just equality — deriveAction in notifications.ts switches on it to pick navigateToTaskDetail(taskId) vs navigateToChannelDashboard(channelId, dashboardId), which need different fields. And canvas identity is composite (channelId + dashboardId), so a single uniqueId would have to be a synthesized composite key anyway — which is exactly what targetKey now produces. So we get the generic comparison you wanted while keeping the typed per-kind fields navigation relies on.
| // The single channel every app notification flows through. Reads focus + the | ||
| // active route, decides suppress / toast / native (see routeNotification), and | ||
| // dispatches accordingly. Native delivery + dock effects are gated by the user's | ||
| // notification settings; the in-app toast always shows (it's non-intrusive and | ||
| // only appears while the app is focused). |
There was a problem hiding this comment.
[nit] not just here specifically, but lots of bgi comments throughout, thoughts on stripping them down a bit and shipping this with a markdown file that explains the notif system instead ? no strong opinion here just personal pref
There was a problem hiding this comment.
Fair — left them as concise inline comments for now rather than a separate markdown file. My reasoning: the routing decision (suppress/toast/native) is the one genuinely non-obvious bit and it's right next to the code it explains, where it's hardest to let drift out of sync. A standalone doc tends to rot once the code moves. Happy to pull it into a notifications/README.md if the comment density bugs you on a re-read though — no attachment.
Address review: collapse the per-kind equality chain into a single targetKey() identity function. New target kinds are now a compile error in one exhaustive switch instead of a silent false. Keeps `kind` (click navigation in deriveAction dispatches on it). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
A single notification bus that every app notification routes through, delivering by focus + what you're currently viewing:
How
NotificationBus(renderer) replacesTaskNotificationService. A pure, table-testedrouteNotification()decides the tier from a genericNotificationTargetunion (task | canvas), defined in@posthog/platform(so core/host-router/ui share it without an internal-import violation).OpenTargetLinkService+ deep-link routeronOpenTarget/getPendingOpenTarget. The renderer consumer drains pending every (re)subscribe, so a click landing during a reconnect/HMR gap still routes (was the "OS deep-link doesn't route" bug).@posthog/quill— provider-managed dismiss, stacking, and auto-dismiss (fixes the hand-rolled sonner-custom toast that couldn't be dismissed). All ~18 direct-sonner callers migrated;sonnerdropped entirely.Behavior change worth calling out
Task notifications that previously fired a native OS notification while the app was focused-but-elsewhere now show an in-app toast; native is reserved for when the app is backgrounded.
Testing
routeNotification/targetsEqualtable tests; zod↔platformNotificationTargetparity test;useOpenTargetDeepLinkconsumer test (warm task + canvas targets + pending-drain);NotificationBustier-routing tests; coreNotificationServicetarget-click tests.🤖 Generated with Claude Code