fix(notifications): four defects a live QA walk turned up - #22
Conversation
Driven end to end against a real Chrome on a consumer app, then each one pinned by a test that fails on the pre-fix code. BREAKING: NotificationsListView.onDelete returns Future<bool>. 1. A declined delete cost a full page refetch. The list is a separately paginated fetch, so a real delete has to be followed by a reload; with no result to read, the row reloaded after every tap and a host that asks for confirmation spent a GET every time somebody said no. true reloads, false does not, a throw still reloads because the server's own state is then unknown. 2. notifications.database.polling_interval was never read. The poller was constructed with no argument on both routes onto it, so its 30-second default always won while the CLI validated the key, doctor reported it and every install stub shipped it. Non-positive values are refused: Timer.periodic accepts zero and then fires continuously. 3. The sms channel rendered as "Sms", untranslated, in every locale. magic-starter-laravel offers sms out of the box, so the machine-name fallback was reachable on a default install. 4. The delete control had no accessible name, so a screen reader announced a bare "button" on every row. Hosts must add notifications.channel_sms and notifications.delete. 595 tests pass, analyze clean, format clean.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. All four fixes look correct and each is pinned by a test; I found nothing that blocks merge, only four minor points. I read all 8 changed files (nothing was dropped for size), traced the only in-package caller of the new Minor
TestsWell covered: the declined/completed reload split, the semantic label, config-driven interval, the non-positive fallback, the Checks I ran
|
All four were real. 1. The runtime now enforces the 5 to 600 second polling range that notifications:doctor and the configuration docs have always published. While the key was ignored it never had to agree with them, and reading it without the range removed an accidental safety net: a stub saying 1 used to be capped at 30 by the bug and would now genuinely fire every second. Out of range clamps to the nearest bound rather than falling back to the default, so 1 becomes 5 instead of jumping to 30. 2. A value that is present but unusable is logged instead of silently substituted. Config.get<int> type-checks rather than casting, so '30' or 30.0 reads as absent, and quietly using a different number is the same shape of defect this change set is fixing. 3. The interval test no longer sleeps. It was three reads inside 2400 ms of a real one-second timer, so about 400 ms of slack on a loaded runner and 3.6 s added to the suite. Replaced by six exact cases on the new public pollingInterval getter plus one fakeAsync case that elapses the clock, which is what pins the WIRING: reverting the poller to ignore the getter turns only that case red, so the unit cases alone would have missed it. 4. doc/architecture/notification-manager.md carried the old delete story and neither of the two new host keys; doc/getting-started/ configuration.md said the runtime used a 30-second default. Both updated, with the true/false/throw contract as a table. 600 tests pass, analyze clean, format clean.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental re-review of Since my last review: the range is now enforced at runtime by clamping to 5-600 ( I verified the pieces the new code leans on rather than assuming them: Minor
TestsWell covered, and better than before: absent, in-range, below-floor, zero/negative, above-ceiling and wrong-type each get their own assertion on Checks I ran
|
The review was right that "on every startPolling()" overstated it. `_poller ??= ...` only re-reads when the poller is null, and only stopPolling() and the realtime handoff null it. pausePolling() keeps the instance, so a Config.set between a pause and a resume changes nothing, which the doc now says outright along with what to call instead.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental re-review of The only file changed since I checked that against the code rather than taking the wording on trust: TestsNo test change, and none needed: this commit touches only prose. The interval behaviour it describes is already covered by the six unit cases and the Checks I ran
|
…elog with the reason Three points from an independent review, all real. 1. pollingInterval is PUBLIC and logged on read, so a consumer surfacing it in a build() would write a line per frame, and the realtime handoff rebuilds the poller on every socket drop, so a flapping connection repeated the same warning for as long as it flapped and buried the incident it was flapping over. The getter is now pure; a private resolver returns the interval and, separately, whatever is wrong with it, and the construction path issues that at most once per manager. 2. The polling-interval change is a behaviour change on upgrade and was filed only under Fixed, which is not where somebody planning an upgrade looks. It now leads Breaking Changes, naming both directions: an app configured at 5 issues six times the requests it did yesterday, and an app configured at 3600 is clamped to 600, so its bell can be ten minutes stale where it used to be thirty seconds. 3. The onDelete entry led with the avoided HTTP request, which is a consequence rather than the reason and invites the obvious objection. It now leads with the reason: onDelete is an opaque host callback, so a host may delete by any route and never touch deleteNotification, and the return value is the only channel that can answer whether the row left. 601 tests, analyze clean, format clean. A getter that logs again, and dropping the once-only flag, each turn the new case red.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental re-review of Since Minor
TestsThe new case is the right shape: ten getter reads assert Checks I ran
|
The review caught that one boolean was suppressing two distinct warnings: once either had fired the other never would for the life of the manager. Concretely, a host starting with a wrong-typed '30' (which warns), then setting the key to 1 and restarting polling, took the five-second clamp silently. De-duplicated on the warning TEXT instead. The repeat this guard exists to stop is the SAME line on every socket flap, and comparing the text stops exactly that while leaving a different reason free to speak. 602 tests. Reverting to the single-flag behaviour turns the new case red, and nothing else.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental re-review of Since The only nuance left is that an A → B → A config sequence would re-emit A, since only the last warning is remembered. That is the correct answer for a real config change, so I am not filing it. Tests
Checks I ran
|
|
Held deliberately, not stalled. Review is clean and the gate is CLEAN; this waits on a release step rather than on anything in the diff. Merging this alone puts a compile error on
Sequence when the release happens, each step verified before the next:
One thing NOT to do in the meantime: relax #122's pin back to |
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.
1 participant
Found by driving the notification system end to end against a real Chrome on a consumer app (uptizm), at desktop and mobile width both. Each fix is pinned by a test that goes red on the pre-fix code; every mutation check is quoted below.
1. BREAKING: a declined delete cost a full page refetch
NotificationsListView.onDeleteis nowFuture<bool> Function(String id)?.The list is a separately paginated fetch, so a real delete has to be followed by a reload: 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, so a host that asks for confirmation (which
magic_starterdoes) spent a fullGET /notificationsevery time somebody declined.truemeans the row is gone: reload.falsemeans the host chose not to go ahead: nothing is re-read.false: the manager removes the row optimistically and puts it back on failure, so what the server still holds is unknown and the list reloads.Migration is one line (
return true) for a callback that always deletes. The package's own seeded default is already updated.2.
notifications.database.polling_intervalwas never readValidated by the CLI, reported by
notifications:doctor, shipped in every install stub, ignored by the runtime:NotificationPoller(this)was constructed with no argument on both routes onto it (the explicit start and the realtime-drop fallback), so its own 30-second default always won. A consumer who set 10 got 30 with nothing to say why.A missing, non-numeric or non-positive value falls back to 30 rather than throwing: this is a timer a consumer wired to its auth state, and a mistyped config value must not take notification delivery down. Zero and negatives are refused specifically because
Timer.periodicaccepts them and then fires on every event-loop turn.3. The
smschannel rendered as "Sms", in every locale_channelLabelnamedmail,databaseandpushand let everything else fall to a helper that raises the machine name's first letter.magic-starter-laravelofferssmsin the matrix out of the box, so that fallback was reachable on a default install: three properly localised rows with an untranslated machine name beside them. Seen live on a Turkish screen next to "E-posta", "Uygulama İçi" and "Anlık Bildirim".The fallback stays: a host can register a channel of its own, and the machine name is the only thing available for it. There is a test for that path too.
4. The delete control had no accessible name
A bare glyph in a
WAnchorwith no label, so a screen reader announced "button" on every row and an E2E driver had no handle to resolve it by.Hosts must add two keys
notifications.channel_smsandnotifications.delete. Both render as the raw key without them, and both are noted in the CHANGELOG.Verification
flutter test: 595 passed (588 before, 7 new)dart analyze: no issuesdart format --output=none --set-exit-if-changed lib test: 0 changeda declined delete costs no reloadfailssemanticLabel→the delete control has an accessible namefailsNotificationPoller(this)with no interval →comes from config, not from the poller defaultfailsfalls back to 30 seconds when the configured value cannot firefails'sms'arm →every offered channel renders a translated labelfails, while the unknown-channel fallback test stays greenOne note on the accessibility test: it reads the annotation off the widget tree rather than through
find.bySemanticsLabel. That finder resolves againstrenderObject.debugSemantics, which is only populated once the semantics pipeline has run, and it answers "none found" rather than complaining when it has not, so it reported zero whether or not the label was there. That is a green test in reverse, and it is why the assertion is shaped the way it is.Release note
The
onDeletesignature is a breaking change on a 0.1.0 package, so this wants 0.2.0, not 0.1.1.magic_starterpins^0.1.0and will need its pin moved plus a one-line change in_confirmThenDeleteto return the bool (a companion PR follows).Author: Anılcan Çakır anilcan.cakir@gmail.com