fix(views): the default list could never delete a notification - #21
Merged
Conversation
`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.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. A one-line wiring fix that does what it says: the seeded Minor
Neither blocks the fix; both are about the surface the fix exposes rather than the fix itself. Tests
Checks I ran
|
…ge 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.
anilcancakir
added a commit
to anilcancakir/uptizm
that referenced
this pull request
Sep 2, 2026
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. Incremental re-review of the commits after Minor
Notes on the rest, no action wanted:
Tests
Checks I ran
|
anilcancakir
added a commit
to fluttersdk/magic_starter
that referenced
this pull request
Sep 2, 2026
…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.
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.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental re-review of the commits after Minor
Otherwise the snippet is faithful: the rollback order, the TestsNo code changed in these commits, so nothing new to cover. The test set from the previous round still stands. Checks I ran
|
`_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/`.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental re-review of the commits after TestsNo code changed in this commit, so nothing new to cover. The test set from the earlier rounds still stands. Checks I ran
|
anilcancakir
added a commit
to anilcancakir/uptizm
that referenced
this pull request
Sep 2, 2026
…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.
anilcancakir
added a commit
to fluttersdk/magic_starter
that referenced
this pull request
Sep 2, 2026
* fix(notifications): the list could never delete a notification
`_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.
* feat(notifications): ask before deleting a notification
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."
* fix(notifications): the confirm dialog read raw keys and ignored the 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.
Merged
anilcancakir
added a commit
that referenced
this pull request
Sep 2, 2026
Cuts 0.2.0, carrying the delete-affordance repair merged in #21 and the QA round merged in #22. Minor rather than patch: three breaking changes, listed in CHANGELOG.md. Two are API shape and one is behaviour that changes on upgrade with no code edit at all. - NotificationsListView.onDelete becomes Future<bool>. A host passing a Future<void> callback no longer compiles. The widget had no way to learn whether the row left, and the list is a separately paginated fetch, so it reloaded after every tap: a host that asks for confirmation spent a GET /notifications every time somebody declined. - deleteNotification rethrows a failed request instead of completing normally, so a caller can finally tell a delete that worked from one that did not. - notifications.database.polling_interval now takes effect. It was validated by the CLI, reported by notifications:doctor and shipped in every install stub while the runtime read nobody, so every install effectively polled every 30 seconds. Both directions move on upgrade: an app configured at 5 issues six times the requests, and one configured at 3600 is clamped to 600. Two host keys are newly required, notifications.channel_sms and notifications.delete; both render as the raw key without them and both are called out in the changelog. Six version sites, swept by SHAPE rather than by the old number, which is the same lesson 0.1.0 recorded: doc/getting-started/installation.md needed no change last time because it was already ahead, so a grep for the current version finds the wrong set. The provider constant is a sixth site the release command's own table does not list, and every previous release has bumped it. The ^0.0.1 in test/cli/commands/uninstall_getter_test.dart is deliberately untouched: it is fixture data for a temp app's pubspec, not a version site. 602 tests, analyze clean, format clean.
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
1 participant
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.
Notify.viewseedsnotifications.listwithconst NotificationsListView(), and the list renders its per-row delete control only whenonDeleteis non-null, so the affordance has never appeared for an app using the package default.deleteNotification()and theDELETE /notifications/{id}route behind it were working code with no surface.Why not default it inside the view
The parameter stays nullable. Its docblock says "No delete affordance renders when
null", and that is a real capability: a host that does not want its people deleting notifications registers its own screen over this default, which is exactly the seamregisterDefaultexists for. Collapsing the default into the widget would remove that choice in order to fix a wiring mistake.Two repos, one line each
magic_starterreplaces this registration in order to wrap both screens in its host page geometry, so a magic_starter app never renders this default and carried the identical gap. That half is fluttersdk/magic_starter#121. Neither repo can express the dependency (the starter's registration wins by running later, not by any pubspec constraint), so both needed the line.Verification
flutter analyzeclean, full suite green,dart formatclean on both changed files.The new test resolves the default through
Notify.view.make('notifications.list')and asserts the built view carries the callback. Mutation-checked: reverting the default toconst NotificationsListView()turns it red withExpected: not null / Actual: <null>.Found while reviewing uptizm's push work (anilcancakir/uptizm#136).