feat(notifications): take notification state from the socket, not a 30s timer - #12
Merged
Merged
Conversation
…0s timer The bell polled `GET /notifications` every 30 seconds while an authenticated broadcast socket was already open in the same app, and there was not one line of Echo code in this package. So a notification the server had already published sat invisible for up to a polling interval, and the request that eventually found it asked for a list the socket could have handed over for free. `Notify.startRealtime(channel: ...)` subscribes to the notifiable's private channel and applies each `notification.created` frame straight to the stream. The frame carries the whole row in the same shape `GET /notifications` returns, so it is applied rather than used as a signal to fetch: asking the API for a row that just arrived in full is the round trip this removes. `startPolling()` becomes a no-op while realtime is live, so a consumer keeps wiring it to auth state and never has to branch on whether a socket happens to be up. Degradation runs in both directions, which is most of the code: - No broadcast driver configured returns false and changes nothing. The driver is read from config rather than probed by subscribing, because the null driver ACCEPTS a subscription and silently delivers nothing: an attempt-based check would report success on the one configuration that cannot work, silence the poller, and leave the bell permanently empty. - A dropped connection arms the poller as a stand-in and keeps the subscription, because realtime is still the intent. Without it, a socket that never comes back is a bell that never updates again. - A reconnect drops the fallback and refetches once, since Reverb has no replay. Only `connectionState` is watched, not `onReconnect` as well: a reconnect necessarily transitions the state to `connected`, so both would fetch twice for one event. - A redelivered id replaces the held row instead of appending a duplicate the bell would count twice, and a frame the decoder cannot read is logged and dropped rather than thrown into the driver's listener or allowed to clear the list. `magic` moves to `^0.0.6` for `Echo.connection`. Without a public accessor there is no way to tell an open connection from a closed one, and magic's Reverb driver opens a SECOND WebSocket on a redundant `connect()` rather than refusing it. A `0.0.z` caret pins the patch digit, so `^0.0.5` resolved exactly 0.0.5, which has no such accessor. The channel name is the caller's to supply: this package has no user model and cannot know whose notifications it is receiving. Verified against PUBLISHED magic 0.0.6 with the local path override removed, which is what CI resolves: 294 tests green, analyze clean, format clean. Four guards were mutation-checked and each reddens exactly one test, no two absorbing each other: the polling no-op, the null-driver refusal, the id dedupe, and the offline fallback. The tests drive frames through a channel double implementing the public `BroadcastChannel` contract, because the fake shipped in 0.0.6 discards both the event name and the callback.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in realtime delivery path to Magic Notifications by consuming notification state directly from an authenticated broadcast socket (via Echo) instead of relying solely on the 30s HTTP poller, while preserving polling as a fallback when sockets aren’t available or the connection drops.
Changes:
- Introduces
NotificationManager.startRealtime/stopRealtime,isRealtime/isPolling, and arealtimeEventcontract; makesstartPolling()a no-op while realtime is active. - Bumps
magicto^0.0.6to useEcho.connectionfor safe connection idempotence. - Adds a comprehensive realtime test suite plus updates README/docs/changelog for the new delivery mode and backend requirements.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/notification_realtime_test.dart | Adds end-to-end style unit tests covering subscription, frame application, dedupe, fallback polling, reconnect refetch, and stop/idempotence behavior. |
| lib/src/notification_manager.dart | Implements realtime subscription lifecycle, polling suppression/fallback, new public state getters/constants. |
| lib/src/facades/notify.dart | Exposes new realtime and polling state APIs via the Notify facade. |
| pubspec.yaml | Bumps magic dependency to ^0.0.6 for Echo.connection. |
| README.md | Documents socket delivery usage and recommended login/logout wiring alongside polling fallback. |
| doc/basics/laravel-backend-setup.md | Documents required Laravel broadcast setup: event name, payload shape compatible with DatabaseNotification.fromMap, and channel authorization. |
| doc/architecture/notification-manager.md | Adds architecture-level explanation of realtime delivery, degradation paths, and lifecycle hooks. |
| CHANGELOG.md | Records the new realtime feature, related API additions, and the dependency bump under [Unreleased]. |
Suppressed comments (1)
lib/src/notification_manager.dart:562
- On reconnect,
_watchRealtimeConnection()callsfetchNotifications()which replaces_notifications. Frames may start arriving again immediately after reconnect; if one is applied while this fetch is running, the fetch completion can overwrite it (lost notification). To avoid races, consider serializing the reconnect refetch with frame application (buffer/merge) or performing the merge by id when realtime is enabled.
// The socket is back. Reverb has no replay, so anything published while
// it was down is gone from the stream and only a fetch recovers it.
_poller?.stop();
_poller = null;
fetchNotifications();
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…tion All four came from the Copilot review on #12 and all four are real. **A frame that landed mid-fetch was clobbered.** `startRealtime()` subscribes and THEN fetches the existing list, so there is a window where a frame prepends to the cached list and the completing read assigns the server's list over the top. The notification was gone until something fetched again. That window is not theoretical: it is the exact moment a backlog of pending notifications is most likely to be publishing. Frames received during a read are now merged back on top, keyed by id, in reverse arrival order so the newest stays at the head. The buffer is cleared in a `finally`, including on a failed read, because the frame was already applied to the cached list as it arrived and must not be re-merged into the next fetch. **The idempotence key ignored the event name.** Keyed on the channel alone, a caller that passed a different `event` for the same channel hit the early return, so the manager silently kept handling the OLD event name: no subscription change, no error, no notifications. The event is part of the key now. **A dartdoc symbol link that is not a symbol.** `[BROADCAST_CONNECTION]` is link markup, not code font, so generated docs carry a broken link. Backticks. **A test-owned broadcast StreamController was never closed**, leaking a handle into every following test. Closed in `tearDown`. Three new tests, each mutation-checked: dropping the mid-fetch merge reddens the two that name it, and reverting the idempotence key reddens only the event one. The mid-fetch test drives the race deterministically rather than by timing, because `fetchNotifications()` sets its in-flight flag synchronously and then suspends on the HTTP await, so emitting between the call and the await lands the frame inside the window every run. 297 tests green, analyze clean, format clean, and re-verified with the local path override removed so the run resolves the PUBLISHED magic 0.0.6 that CI resolves.
anilcancakir
added a commit
to anilcancakir/uptizm
that referenced
this pull request
Aug 19, 2026
…n beside it (#63) The in-app bell fetched `GET /notifications` on a 30-second timer, in an app that already holds an authenticated Reverb connection for monitoring. Two costs: a notification the server had already written stayed invisible for up to a polling interval, and the request that eventually found it asked for a list the socket could have handed over for free. This is the uptizm half. The client half is a `magic_notifications` change (fluttersdk/magic_notifications#12) that subscribes to a private channel and applies each `notification.created` frame to the notification stream, with `startPolling()` becoming a no-op while the socket is live. ## The three pieces here **A per-user channel.** `App.Models.User.{id}`, authorised in `routes/channels.php` with `['guards' => ['sanctum']]`, because the channel-auth request from the Flutter client carries a bearer token rather than a session cookie and the default `web` guard would deny every subscription. Compared as strings, since the key is a UUID. Deliberately NOT the existing `teams.{teamId}` channel: every teammate is subscribed to that one, and a notification is personal. **The name is written down, not derived.** `User::receivesBroadcastNotificationsOn()` returns what Laravel would derive from the class name anyway, so it changes no behaviour today. It exists because that string is a contract in three places at once (this file, `routes/channels.php`, and the Dart caller), and a derived name would silently change if the model ever moved namespace. The failure mode would be a bell that quietly stopped updating rather than anything that goes red. **Broadcast follows database.** `via()` appends the driver only when `database` survived the preference filter, so a notifiable that turned the in-app channel off gets neither. Broadcast alone would push a frame for a notification no row exists for, and the bell would show an entry that vanished on the next fetch. `GateNotificationChannels` cannot enforce this: it maps a driver channel back to a logical one and ALLOWS anything it cannot map, so a `broadcast` driver sails through it fail-open. `IncidentEscalated` inherits all of it from `IncidentOpened`. The registry is untouched on purpose. Broadcast is delivery of the in-app row, not a preference of its own; giving it a toggle would let someone disable live delivery while keeping the row, which is a setting nobody asked for. ## The payload shape is the whole game Laravel's DEFAULT broadcast payload flattens the notification data to the top level and appends `id` and `type`, while the client's `DatabaseNotification.fromMap` reads `data.title`, `data.body` and `data.action_url` from a NESTED `data` key. The default decodes to nothing usable and fails silently: a frame arrives, the decoder throws, the row is dropped. `toBroadcast()` builds the frame from `toArray()`, so ONE serializer sits behind both the API row and the socket frame and the two cannot drift. ## What the gate caught, and I would not have `receivesBroadcastNotificationsOn()` took a REQUIRED argument first, matching Laravel's own call. Filament's notification component calls the same method with no arguments at all, guarded only by `method_exists` (`filament/notifications/src/Livewire/Notifications.php:103`), so an `ArgumentCountError` came out of a Blade render and turned every admin panel page into a 500. Three admin tests went red and named it. The parameter is optional now and a test pins the zero-argument call, since nothing in this feature's own surface touches that caller. ## Verification `bin/check` all seven green (2425 backend tests). The `database`-follows guard was mutation-checked and reddens exactly the one test that names it; the optional parameter likewise. The client wiring in `AppServiceProvider` has no unit test, because the seam needs a booted app: it is covered live instead, driving a real notification to a real browser.
anilcancakir
added a commit
that referenced
this pull request
Aug 19, 2026
Releases the socket-driven notification state added in #12: `Notify.startRealtime()` takes the bell off its 30-second timer and onto the app's broadcast connection, with polling kept as the fallback in both directions (no driver configured, or a socket that dropped). The `magic` floor moves to `^0.0.6` with it, for `Echo.connection`. That accessor is what lets the realtime path tell an open connection from a closed one, and without it magic's Reverb driver opens a SECOND WebSocket on a redundant `connect()` rather than refusing. A `0.0.z` caret pins the patch digit, so `^0.0.5` resolved exactly 0.0.5, which has no such accessor. **This release does not break the existing consumers, and does not reach them either.** `magic_starter` and `magic_example` both require `magic_notifications: ^0.0.2`, which under the same `0.0.z` caret rule means `<0.0.3`: they keep resolving 0.0.2 and are untouched. Adopting the feature there is a bump in each of those repositories, which also drags their own release train, so it is deliberately not part of this commit. `uptizm` needs nothing: it consumes this package by path, both locally and in CI. Also corrects two version pins that were already stale before this release: the README install snippet (0.0.2) and `CLAUDE.md`'s header, which still said 0.0.1. Verified with exactly the gate `publish.yml` runs (pub get, analyze, format, 297 tests) plus a `pub publish --dry-run`, whose only warning was these uncommitted files. The suite was also re-run with the local path override removed so it resolved the PUBLISHED magic 0.0.6 that a real consumer gets.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Notify.startRealtime(channel: ...)subscribes to the notifiable's private broadcast channel and applies eachnotification.createdframe straight to the notification stream.startPolling()becomes a no-op while it is live.New public API on
Notify/NotificationManager:startRealtime,stopRealtime,isRealtime,isPolling, and therealtimeEventconstant.Why
The bell polled
GET /notificationsevery 30 seconds while an authenticated broadcast socket was already open in the same app, and there was not one line of Echo code in this package. Two costs: a notification the server had already published stayed invisible for up to a polling interval, and the request that eventually found it asked for a list the socket could have handed over for free.The frame carries the whole row in the same shape
GET /notificationsreturns, so it is applied rather than used as a signal to fetch. Asking the API for a row that just arrived in full is the round trip this removes.The degradation is most of the code
Realtime is opt-in and falls back in both directions:
false, changes nothing. The driver is read frombroadcasting.defaultrather than probed by subscribing, because the null driver ACCEPTS a subscription and silently delivers nothing. An attempt-based check would report success on the one configuration that cannot work, silence the poller, and leave the bell permanently empty.connectionStateis watched, notonReconnectas well: a reconnect necessarily transitions the state toconnected, so both would fetch twice for one event.Dependency bump
magic: ^0.0.5->^0.0.6, forEcho.connection. Without a public accessor there is no way to tell an open connection from a closed one, and magic's Reverb driver opens a SECOND WebSocket on a redundantconnect()rather than refusing it. A0.0.zcaret pins the patch digit, so^0.0.5resolved exactly 0.0.5, which has no such accessor.The channel name is the caller's
This package has no user model and cannot know whose notifications it is receiving, so the channel is a required argument (
App.Models.User.{id}by Laravel's default). The server half is documented indoc/basics/laravel-backend-setup.md: thebroadcastchannel on the notification,broadcastAs(), atoBroadcast()built fromtoArray()so one serializer sits behind both the API and the socket, and theroutes/channels.phpauthorisation with['guards' => ['sanctum']].Worth calling out from that doc, because it is a trap: Laravel's DEFAULT broadcast payload flattens the notification data to the top level, while
DatabaseNotification.fromMapreadsdata.title/data.body/data.action_urlfrom a nesteddatakey. The default decodes to nothing useful, which is why the doc shows an explicittoBroadcast().Testing
Verified against published magic 0.0.6 with the local path override removed, which is what CI resolves: 294 tests green,
flutter analyze --no-fatal-infosclean,dart format --set-exit-if-changedclean. Confirming this against the published release rather than the local sibling checkout is the point; a local override hides an unreleased API and turns into a red CI.13 new tests in
test/notification_realtime_test.dart. They drive frames through a channel double implementing the publicBroadcastChannelcontract, becauseFakeBroadcastDriver's channel in 0.0.6 discards both the event name and the callback, so a test against it can prove a channel was opened and nothing about what happens to a frame on it.Four guards were mutation-checked, each reddening exactly one test with no two absorbing each other's mutation:
startPolling()no longer respects realtime'null'driver treated as usableDocs
CHANGELOG.mdunder[Unreleased],README.md(feature row plus login/logout/usage),doc/architecture/notification-manager.md(a Realtime Delivery section),doc/basics/laravel-backend-setup.md(the server half).