fix(notifications): the list could never delete a notification - #121
Conversation
`_mountNotificationViews()` built `const NotificationsListView()`, and that view
renders its per-row delete control only when `onDelete` is non-null, so no
delete affordance has ever appeared.
The null mattered more than it looks. This registration deliberately REPLACES
the default `magic_notifications` seeds, in order to wrap both screens in the
host page geometry, so it is the registration a magic_starter app actually gets
and its answer is the whole ecosystem's answer. `Notify.deleteNotification` and
the `DELETE /notifications/{id}` route behind it were working code that nothing
could reach: an endpoint with no surface.
Passing `Notify.deleteNotification` here rather than defaulting it inside the
view, because the nullable parameter is a real capability: a host that does not
want its people deleting notifications registers its own screen, and collapsing
the default into the widget would take that away.
A test asserts the mounted view carries the callback, and turns red when the
parameter is dropped again.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. One-line wiring fix that looks correct: the callback type matches the parameter exactly, the mount point is the right place for it, and the new test fails if the parameter is dropped. Verified against the resolved dependency ( Minor
Tests
Checks I ran
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The previous commit gave the delete affordance a surface, which made a destructive, irreversible action reachable one tap away in a list of rows a thumb scrolls past. The mount now routes it through `Magic.confirm` and only reaches the server once somebody says yes. Asked here rather than in `magic_notifications`. That package removed its own dialog widget in 0.1.0 precisely so a published package stops imposing one adopter's tone and layout on everybody, and putting the dialog back would undo that decision to fix a wiring problem. Here it also sits in the same package as every other destructive confirmation a starter app shows. `Magic.confirm` rather than this package's own `MSConfirmDialog`, even though it owns that component: the view registry hands a builder no `BuildContext` and neither does `onDelete`, so there is nothing to show a dialog against. The magic facade resolves its own overlay, which makes it the only confirmation API reachable from a mount point at all. A refusal returns without touching the server, and `deleteNotification`'s throw is left to propagate: the list row catches it and says so. Hosts must add `notifications.delete_confirm_title` and `notifications.delete_confirm_message`. The buttons reuse the existing `common.delete` and `common.cancel`. The new test drives the refusal path, which is what `Magic.confirm` answers with no overlay mounted, and asserts nothing reached the server. Mutation-checked: wiring `onDelete` straight back to `Notify.deleteNotification` fails it with "Expected no matching request but one was found."
The notification list's delete is being given a surface upstream (fluttersdk/magic_notifications#21, fluttersdk/magic_starter#121), and both halves speak through keys the HOST carries: neither package ships a catalogue. `trans()` renders a missing key as the key itself, so without these three a person would see `notifications.delete_confirm_title` in a dialog and `notifications.delete_failed` in a toast. Landed here rather than in a follow-up so there is no window where this app builds against the merged siblings and paints raw keys. The keys are inert until those merge: nothing in this repo reads them today. `delete_failed` is the message the list shows when the request fails, which it could not say before because the manager swallowed the error. The two `delete_confirm_*` keys are the dialog; its buttons reuse the `common.delete` and `common.cancel` this catalogue already carries.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. The new commit adds the confirmation my last review asked for, and it settles that half; but three of its four labels are keys this package ships no translation for, so the dialog it opens reads Reviewing only Major
Minor
Tests
Checks I ran
|
…theme Review found three real problems with the confirmation the last commit added, and all three were mine. The three labels it asks with are keys this package ships no translation for. `assets/stubs/install/en.stub` is the catalogue `starter:install` scaffolds into every consumer project, and `Translator.get` answers a missing key with the key itself, so a freshly installed app opened a dialog titled `notifications.delete_confirm_title` with a confirm button reading `common.delete`. Only `common.cancel` resolved. The CHANGELOG framed this as "hosts must add", which was wrong: the stub is this package's own file. All three are in it now. `Magic.confirm` was the wrong dialog, and the reason given for choosing it was also wrong. It styles from `view.confirm.*` with hardcoded light-mode fallbacks (`bg-white`, `bg-red-500`) and nothing in this package registers a confirm builder to replace them, so it would have shipped the one destructive dialog in the app that ignores `MagicStarter.manager.modalTheme` and the host's dark mode. The stated reason, that no `BuildContext` is reachable from a mount point, is untrue: `MagicRouter.instance.navigatorKey.currentContext` is exactly where `MagicFeedback` gets its own. It now shows `MSConfirmDialog` against that context, and a null context refuses rather than deleting, because nobody could have been asked. And the docblock claimed a failed delete surfaces to the person because the list row catches the throw. That is not true of the resolved dependency: `magic_notifications` 0.1.0 swallows. It becomes true with fluttersdk/magic_notifications#21, and the docblock now says which is which instead of describing an unreleased version as the present. Tests: the previous case only drove the refusal path, which a callback that returned without calling anything would also satisfy. There are two now, and the accept path needed a harness with the navigator key AND `WindTheme` ABOVE `MaterialApp`: a dialog is pushed onto the navigator, so with the theme inside `home` its own `WDiv` asserts "No WindTheme found in context". That is the order `MagicApplication` builds and the order the existing confirm-dialog test uses. Both mutation-checked: skipping the delete after a yes fails the accept case, and wiring `onDelete` straight through fails the refusal case. `doc/basics/notifications.md` documents the whole delete path, per the post-change checklist in CLAUDE.md.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting.
Reviewing only Verified each:
Minor
Tests
Checks I ran
|
…d five more (#136) * fix(push): a live row's wrong icon, a prompt blind to its own fix, and five more Seven defects a review of #134 found in the notification surface. None of them failed a test, and one of them had a test asserting it. The broadcast frame named the wrong type. `toBroadcast()` sent `static::class` while `NotificationResource` serves `data['type']`, and the client reads the top-level key, so one notification arrived as two different types depending on its transport: a row delivered over the socket missed uptizm's icon lookup and rendered the generic fallback, then silently changed to the right icon on the next fetch. The socket is the primary path, so the wrong icon is what an operator saw first, during an incident. Now derived from the payload the frame already carries, which makes the two structurally identical rather than merely equal today. Fixed in both notifications: `IncidentResolved` declares its own `toBroadcast()` rather than inheriting, so the same line was wrong twice. The prompt never noticed the permission it asked for. `_PushPromptHostState` read the platform in `initState` and nowhere else, while its twin in the shell subscribed to both driver streams and documented why. A grant almost always lands out of band, so the one screen that exists to turn push on was the only one that never saw push get turned on. The bell wore the package's palette. Both shells mounted `NotificationDropdown` with no overrides, so it rendered `bg-white` / `text-gray-500` / `bg-red-500` beside controls written in semantic aliases. The commit that introduced it had deleted the app's own tokens. Restored from shared constants, including the `active:` tone the widget computes and hands down as a Wind state: without it the bell looked identical whether its panel was open or shut, which on a touch device is the whole affordance. Four `listen()` calls carried no `onError`, on streams the driver deliberately pushes errors into, so a failed platform read escaped to the zone and reached Sentry as an app error instead of the log line every other guard here writes. The deep-link guard did not mean what its comment said. `startsWith('/')` admits `//host/path`, which parses to a URI with a foreign authority, and `MagicRoute` hands its argument straight to the router. Now a scheme and an authority are rejected explicitly. `notificationRouteFor` answered a literal `/settings/notifications` where the config helper composes the same path from a key that exists in order to be changed. And the config seam test could not fail for the reason it claimed: both cases injected the factory themselves, so deleting the registration from `main.dart` left the suite green while the app shipped with no push driver on any platform, silently. It reads the source now, the way this repo's other boot-seam tests do. Seven new tests, each mutation-checked where a mutant carries information: the grant, the failed read, the host-carrying deep link, the config-driven fallback and all three broadcast tokens go red when their fix is reverted. The three `monitor_*` keys in the icon map are documented as unreachable rather than deleted, since the mapping is the one a monitor-level notification would want. * i18n(notifications): the three keys the delete path needs The notification list's delete is being given a surface upstream (fluttersdk/magic_notifications#21, fluttersdk/magic_starter#121), and both halves speak through keys the HOST carries: neither package ships a catalogue. `trans()` renders a missing key as the key itself, so without these three a person would see `notifications.delete_confirm_title` in a dialog and `notifications.delete_failed` in a toast. Landed here rather than in a follow-up so there is no window where this app builds against the merged siblings and paints raw keys. The keys are inert until those merge: nothing in this repo reads them today. `delete_failed` is the message the list shows when the request fails, which it could not say before because the manager swallowed the error. The two `delete_confirm_*` keys are the dialog; its buttons reuse the `common.delete` and `common.cancel` this catalogue already carries.
* fix(views): the default list could never delete a notification
`Notify.view` seeds `notifications.list` with `const NotificationsListView()`,
and the list renders its per-row delete control only when `onDelete` is
non-null, so the affordance has never appeared for an app using the package
default. `deleteNotification()` and the `DELETE /notifications/{id}` route
behind it were working code with no surface.
The parameter stays nullable rather than being collapsed into the widget. A host
that does not want its people deleting notifications registers its own screen
over this default, which is exactly the seam `registerDefault` exists for;
defaulting inside the view would remove that choice in order to fix a wiring
mistake.
`magic_starter` replaces this registration to apply its host page geometry and
carried the same gap, fixed in fluttersdk/magic_starter#121. Neither repo can
express the dependency, so both needed the line.
* fix(notifications)!: a failed delete said nothing, and emptied the page it left
Three defects in the delete path, all of them newly REACHABLE because the
previous commit gave the affordance a surface. Reported by review on the PR that
wired it.
`deleteNotification` logged a failed request, rolled the row back and completed
NORMALLY. A caller holding that future could not tell a delete that worked from
one that did not, so the only thing a person saw was the row leaving the list and
returning, with nothing said. It rethrows now. The rollback is unchanged. This is
the breaking half: a caller that wants the old silence adds a `catch`.
`markAsRead` and `markAllAsRead` keep swallowing on purpose. Their failure is
recoverable by looking again and costs nothing, and changing all three would
widen the break far past the defect. The asymmetry is documented where it lives
rather than left to be discovered.
The list row now catches that throw and surfaces `notifications.delete_failed`
through `Magic.error`, then re-reads the page either way: after a success to
reconcile with the server, after a failure to re-read what the server still
holds rather than trusting the rolled-back local copy. Unhandled, the throw would
have escaped into the gesture callback and been reported as a framework error
while the person still learned nothing. Hosts have to carry the new key.
`refresh()` re-read the page the reader was on, so deleting the only row of
page 3 in a list that now ends at page 2 answered an empty page and showed
"nothing here yet" while the notifications sat one page back with no control
saying so. It now reads `last_page` when the paginator reports
`current_page > last_page`. Keyed on that rather than on an empty `data` list:
emptiness lies in both directions, since a failed read leaves the previous page
in place and a backend claiming more pages while answering none would send the
reader backwards. An existing test caught that imprecision in the first attempt.
And the delete icon's `hover:text-red-500` had no `dark:` peer, so dark mode
hovered to a red tuned for a white background. The surface tone beside it was
already paired.
Three tests, each mutation-checked. One existing case asserted the old
swallowing contract as the requirement ("completes without error") and now
asserts the throw.
* docs(manager): the delete contract page still described the swallow
The architecture page is where a host reads to learn these contracts, and it
still said read-mutate operations "revert on failure" full stop, then printed
the pre-change body of `deleteNotification` ending at the rollback with no
`rethrow`. The contract changed in a breaking way in the commit before this
one, so the page was actively wrong about the one operation a host now has to
handle.
The snippet is the current body, the surrounding sentence no longer implies all
three behave alike, and a note records why delete diverges from `markAsRead` and
`markAllAsRead`, what a caller wanting the old silence does, and that
`notifications.delete_failed` is a key the host has to supply because this
package ships no catalogue.
Also corrects `_safeLogError` in the snippet, which is not a member of this
class; the real call is `NotificationLog.error`.
CLAUDE.md's post-change checklist asks for `doc/` alongside `CHANGELOG.md`, and
that is what this is.
* docs(manager): the same stale log call, one snippet up
`_safeLogError` is not a member of this class and has not been for a while; the
previous commit corrected it in the `deleteNotification` snippet and left it in
the `markAsRead` snippet directly above, so the page showed two different
logging calls for code that makes one.
A correct fix applied to one of two identical sites, which is its own recurring
mistake and the reason this is a separate commit rather than a quiet amend.
`grep -rn "_safeLogError"` now has no hits in `doc/`, `lib/` or `test/`.
* chore(release): 0.0.1-alpha.26 (magic_notifications ^0.2.0) Cuts alpha.26, carrying the delete-affordance work merged in #121 and the callback answer merged in #122. Stays on the alpha rail: nothing here is a public API break in this package. _confirmThenDelete is private, so what changed for a consumer is the dependency floor, and that is what the parenthetical names. The dependency move is the point of this release. magic_notifications 0.2.0 changed NotificationsListView.onDelete to Future<bool>, and this package's default branch could not compile against it until #122 landed. Publishing this is what carries that fix to an adopter, since alpha.25 on pub.dev still pins ^0.1.0 and still returns Future<void>. Six version sites, swept by shape rather than by the old number: pubspec.yaml, CHANGELOG.md, CLAUDE.md, README.md, doc/getting-started/installation.md, and the magicStarterVersion constant behind the command banners. 1409 tests, analyze clean, format clean. There is no pubspec_overrides.yaml in this worktree, so the resolution is the one the publish workflow sees: pubspec.lock records magic_notifications 0.2.0 from pub.dev as a hosted dependency, which is the release that went out an hour ago. * chore(release): make the floor comment say what the floor says The comment above the constraint still narrated 0.1.0 as the floor while the line below it read ^0.2.0. It came in with #122 rather than with the bump, but this is the release that ships that file to pub.dev and the dependency floor is the stated point of it, so an adopter reading the published pubspec would have found the prose arguing against the constraint. Also says WHY 0.2.0 rather than 0.1.0, which the old text could not: against 0.1.0 the library still compiles, because Future<bool> satisfies a Future<void> parameter, and it is the routes test that does not, since it reads onDelete's return type. The floor is what keeps the package and its own suite resolvable together.
1 participant
_mountNotificationViews()builtconst NotificationsListView(), and that view renders its per-row delete control only whenonDeleteis non-null, so no delete affordance has ever appeared.The null mattered more than it looks. This registration deliberately REPLACES the default
magic_notificationsseeds, in order to wrap both screens in the host page geometry, so it is the registration a magic_starter app actually gets and its answer is the whole ecosystem's answer.Notify.deleteNotificationand theDELETE /notifications/{id}route behind it were working code that nothing could reach: an endpoint with no surface.Why here and not in the view
Passing the callback at the mount rather than defaulting it inside
NotificationsListView, because the nullable parameter is a real capability. Its docblock says "No delete affordance renders whennull", and a host that does not want its people deleting notifications registers its own screen. Collapsing the default into the widget would take that choice away to fix a wiring mistake.magic_notifications' own default registration (notify.dart) has the same gap and is worth the same one-line change, but it is not what a magic_starter app renders, so it is a separate PR in that repo rather than a reason to hold this one.Verification
flutter analyzeclean, full suite green,dart formatclean on both changed files.The new test asserts the mounted view carries the callback. Mutation-checked: reverting the mount to
const NotificationsListView()turns it red withExpected: not null / Actual: <null>.Found while reviewing uptizm's push work (anilcancakir/uptizm#136), where the missing affordance showed up as a working endpoint with no caller.