Skip to content

fix(notifications): answer whether the confirmed delete went ahead - #122

Merged
anilcancakir merged 1 commit into
mainfrom
fix/notification-delete-returns-bool
Sep 2, 2026
Merged

fix(notifications): answer whether the confirmed delete went ahead#122
anilcancakir merged 1 commit into
mainfrom
fix/notification-delete-returns-bool

Conversation

@anilcancakir

Copy link
Copy Markdown
Contributor

Draft, and it stays a draft until magic_notifications 0.2.0 is on pub. CI resolves that package from pub rather than from the sibling checkout, so pub get cannot satisfy the ^0.2.0 pin this PR needs and every job will fail until the release lands. Companion to fluttersdk/magic_notifications#22.

What

NotificationsListView.onDelete becomes Future<bool> in magic_notifications 0.2.0, so _confirmThenDelete now reports its decision:

  • false when it refuses, for either reason: no navigator to put the question in, or somebody said no.
  • true after a delete the server accepted.

Why the signature changed at all

The list reloads its page after a real delete, and it has to: a row leaving page one pulls one up from page two, and only the server knows which. With no result to read, the row reloaded after every tap.

Which means the confirmation dialog this package added in the same Unreleased block was costing a full GET /notifications every single time somebody declined it, on a list that had not changed. That is the defect this closes, and it is one this package introduced.

Nothing about the dialog itself changes.

Verification

  • flutter test: 1409 passed
  • dart analyze: no issues
  • dart format --output=none --set-exit-if-changed lib test: 0 of 329 changed
  • New case a declined delete answers false and sends nothing taps Cancel and asserts both the false and that no DELETE was sent.
  • The two existing delete cases gained the assertion they were missing: the null-context branch has to answer false (answering true there would reload for a delete that never happened), and the confirmed branch has to answer true.
  • Mutation check: making a decline answer true turns exactly the new case red.

Local resolution used a redirected pubspec_overrides.yaml pointing at the magic_notifications branch; that file is gitignored and is not part of this PR.

Merge order

  1. magic_notifications#22 merges.
  2. magic_notifications 0.2.0 is published.
  3. This PR is marked ready, CI goes green, merge.
  4. magic_starter is released, and consumers move their own pin.

Author: Anılcan Çakır anilcan.cakir@gmail.com

magic_notifications 0.2.0 changes NotificationsListView.onDelete to
Future<bool>, so _confirmThenDelete now reports its decision: false when
it refuses (no navigator to ask in, or somebody said no), true after a
delete the server accepted.

That answer is why the signature changed. The list reloads its page
after a real delete, because a row leaving page one pulls one up from
page two and only the server knows which. With nothing to read it had to
reload after every tap, so the confirmation dialog this package added in
the same unreleased block was costing a full GET /notifications every
time somebody declined it.

The pin moves to ^0.2.0 in the same commit, because the old pin admits a
version whose onDelete has the other signature and would not compile.

1409 tests pass. A decline answering true turns the new case red.
@kodizm

kodizm Bot commented Sep 2, 2026

Copy link
Copy Markdown

Note

Kodizm (AI-generated). May contain mistakes; verify before acting.

The bool contract is right on all three branches and matches the published 0.2.0 consumer exactly; nothing blocking, two stale references to tidy - and the draft blocker is gone, because magic_notifications 0.2.0 is on pub and this PR resolves against it.

The signature change lines up with what the consumer actually does: notifications_list_view.dart:361-398 in the resolved 0.2.0 seeds reload = true, overwrites it with the callback's answer, and forces true again in its own catch. So false on refuse, true after a delete the server accepted, and a rethrown failure reloading anyway are each the correct answer at this call site, and leaving deleteNotification unguarded stays correct for the same reason.

Minor

doc/basics/notifications.md:85 — the "Deleting a notification" section still tells a host to "register your own screen for 'notifications.list' before the routes are mapped and pass whatever onDelete you want", with no mention that the callback is now Future<bool> and that answering true on a refusal costs a GET /notifications. A host following this page writes a Future<void> and gets a compile error, and the doc gives them nothing to explain it. Line 15 of the same file also still reads "magic_notifications >= 0.1.0", which this PR makes false. CLAUDE.md's post-change checklist asks for doc/ to be synced with the change. (maintainability)

lib/src/cli/commands/magic_starter_install_command.dart:1159_setupNotifications writes magic_notifications: ^0.0.1-alpha.1 into the consumer's pubspec, which under pub's 0.0.z caret rule is >=0.0.1-alpha.1 <0.0.2 and cannot co-resolve with this package's own >=0.2.0 <0.3.0: starter:install --features notifications leaves the host with a pubspec that will not solve. Pre-existing, and already broken against ^0.1.0, so not introduced here - but this PR is the version bump, and the fix is the same one line.

Tests

Covered, and the new case is the one that was missing. a declined delete answers false and sends nothing taps Cancel, asserts false and asserts no DELETE went out; the null-context and confirmed cases each gained the return assertion they lacked. All three branches of the new return value now have a test.

Checks I ran

  • flutter pub get — resolved, 162 dependencies, magic_notifications 0.2.0 from pub.dev with no override present (no pubspec_overrides.yaml in the checkout). The description's "CI cannot satisfy the ^0.2.0 pin" is stale.
  • grep on the resolved package — notifications_list_view.dart:39: final Future<bool> Function(String id)? onDelete;. Signature matches.
  • flutter test test/routes/notification_routes_test.dart — 12 passed, including the new declined-delete case.
  • flutter testAll tests passed!, 1409 cases, matching the description's count.
  • flutter analyzeNo issues found!
  • dart format --output=none --set-exit-if-changed lib test — 329 files, 0 changed.

@anilcancakir
anilcancakir merged commit 0f88991 into main Sep 2, 2026
4 of 6 checks passed
@anilcancakir
anilcancakir deleted the fix/notification-delete-returns-bool branch September 2, 2026 21:50
@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

anilcancakir added a commit that referenced this pull request Sep 2, 2026
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.
anilcancakir added a commit that referenced this pull request Sep 2, 2026
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant