Hold a cold-start click until something listens, and cut 0.3.0 - #27
Conversation
…s version The reference an agent adopting this package reads lives in the magic repo, not here: .pubignore keeps CLAUDE.md and .claude/ out of the published archive, so pub.dev ships doc/ and README.md and nothing else an agent is pointed at. That reference is versioned by magic's releases rather than by this package's, and it drifted far enough to document a contract that no longer compiles. A test compares its first-line stamp against this pubspec's version. It cannot run in CI, which clones no siblings, so it skips there instead of failing; releases are cut locally and that is where it fires. release.md carries the same requirement in prose for anyone running the suite elsewhere.
`onPushClicked` is a broadcast stream, and a broadcast stream drops what it publishes to nobody. On a cold start there is a window where nobody is there yet: `onesignal_flutter` buffers the tap that launched the app and drains it in a microtask scheduled from `addClickListener`, which this manager calls inside `driver.initialize()`, which its own provider awaits in `boot()`. A consumer whose provider list puts notifications BEFORE the package that bridges clicks into deep links subscribes only afterwards, so the launch tap is published into an empty stream. No exception, no log, and the app finishes booting onto its own initial route. Which order a consumer ends up with is decided by the order the two packages happened to be installed in, because artisan's installer appends each provider to the END of the list. uptizm has the safe order by accident, which is why a device measurement passed while the defect was live. The manager now holds clicks until the first listener and replays them once, mirroring `onesignal_flutter`'s own shape: filled only until the first-ever listener, drained once, never refilled, and bounded so a build that never listens does not grow a list for the life of the process. Refilling for a late subscriber would re-navigate an app somebody has since moved through, which is worse than the failure this closes. `forgetDrivers` clears both fields so a buffered click cannot replay into the next test.
OneSignal's iOS setup needs an App Group and a Notification Service Extension, and this package never mentioned either: no check, no doc line, nothing in the install manifest. Push works without them, which is the reason it matters. A build with no extension delivers notifications normally and quietly reports no confirmed deliveries, no rich media and no badge counts, so the absence reads as the product working rather than as an install left half done. Neither can be automated, because both add or change an Xcode target and a pub package cannot. So the doctor warns and the installation doc carries the manual steps. Warnings rather than failures: an app that never wants rich notifications is a legitimate build, and a doctor that fails one stops being read. The two halves are checked separately because they fail independently. An extension with no shared App Group gives rich media and still no confirmed delivery, since the container is how the extension hands what it saw back to the app. Splitting getWarnings into configWarnings plus the platform ones came with it: rendering every warning under "Config Validation" filed an Xcode target's absence as a config finding and printed it twice, and the closing line claimed push "cannot send yet" over something that does not stop a single notification. The doc also gains the cold-start caveat OneSignal documents and that cost real time here: on iOS in Debug a force-closed app opened from a notification never registers the click listener, so the cold path can only be tested from a profile or release build.
Minor rather than patch, following this package's own cadence (0.0.3 -> 0.1.0 -> 0.2.0, minor for a feature release). The unreleased section carries the cold-start click buffer, which is the fix, and the bulk preferences row, which is a feature that needs two translation keys an adopter has to supply: a raw key renders where they do not, so a patch would slide a soft break into every consumer on `^0.2.0` without a bump. That choice costs two edits elsewhere, and forgetting the first is how a release train breaks: `magic_starter` pins `magic_notifications: ^0.2.0` and uptizm pins the same, so both have to follow before either resolves. Version lives in four places besides the manifest and the package gates two of them itself: `install_command_test.dart` compares `magicNotificationsVersion` against the pubspec, and `skill_reference_stamp_test.dart` compares the sibling magic repo's reference stamp. The second is red locally until magic's `feature/plugin-skill-references` lands, and skips in CI, which clones no siblings. Not published. The publish is the owner's to run.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. The cold-start buffer is the right shape, but its "has anybody heard this?" flag tracks stream ACCESS rather than subscription, which reopens the same drop in two reachable cases; the doctor and docs work looks correct. Major
Minor
TestsThe two new manager tests cover buffer-then-replay and no-replay-for-a-late-listener, and the four doctor tests cover both iOS halves plus the no- Checks I ran
|
This reverts commit b69bf13.
|
Dropped the version bump from this PR. It carried A publish is not on the critical path for anything, so a bump has no business in a feature PR: it made a sibling's The version bump is its own later act, whenever the release is actually cut. |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental re-review: the only new commit is Since my last review: the version bump came out. All five files move My earlier Major findings on the cold-start buffer ( TestsNo behaviour changed, so no new coverage is expected. Checks I ran
|
This reverts commit dbb4e7a.
…tter read
Two holes in the cold-start buffer this branch added, both found by review and
both confirmed against the source before acting. The buffer asked "has anybody
read this stream", and the question that matters is "is anybody subscribed".
**Reading a stream and listening to it are different moments, and only the
second can receive anything.** `onPushClicked` set `_pushClickedHeard` on the
getter and scheduled the drain there, so a consumer that captures the stream,
awaits its router and subscribes afterwards flipped the flag, had the microtask
drain into a broadcast controller with no subscriber, and lost the launch tap in
the exact way this buffer exists to prevent.
**`forgetDrivers()` could not close the second hole.** It reset the flag but
cannot cancel a subscription somebody else holds, and it is public through
`Notify.forgetDrivers()`. An app calling it on sign-out kept its boot-time
subscription, so the manager buffered again for a listener that was already
there and never drained, because nothing reads the getter twice. Every tap after
a sign-out was swallowed in silence.
Both close on the same change: the buffer fills when
`_pushClickedController.hasListener` is false and drains from that controller's
own `onListen`, which fires on each 0 -> 1 transition. The drain refuses an
empty controller, so a subscriber that cancels inside the turn it subscribed
leaves the buffer for the next one instead of having it added to nobody. The
window therefore reopens on any later gap between subscribers rather than
closing for the life of the process, which is the honest shape: a tap that
arrives while nothing is listening is a tap nobody received.
The replay is also re-judged rather than replayed blind. A tap buffered while
the device was subscribed as one person and drained after `want()` moved it to
another handed the deep-link bridge the previous identity's payload, which is
precisely what the receive-side guard exists to stop; `forgetDrivers` clears the
buffer, `want` does not.
Three tests, each verified to fail against the previous implementation and pass
against this one: read-then-subscribe-later, a live subscriber surviving
`forgetDrivers`, and a buffered tap dropped after the identity moves.
Also from the same review:
- `notifications:doctor` identified a Notification Service Extension by a
`.appex` suffix, which every app extension has. A project shipping a widget
and no NSE reported it present and only nagged about App Groups: a false
green on the one check whose justification is that its subject looks exactly
like the product working. It now requires the `.appex` AND an `Info.plist`
under `ios/` declaring `com.apple.usernotifications.service`, since the
extension point lives in the target's own plist and the target can be named
anything. Runner's own plist is excluded. Covered by a test that fails
against the old check.
- `skill_reference_stamp_test.dart` called `.first` on the reference's lines,
which throws `Bad state: No element` on an empty file and replaces the
carefully worded reason with a StateError naming nothing.
Verified: `flutter analyze` clean, `dart format` clean, 623 tests pass. The one
failure is the stamp gate, which reads the sibling magic checkout and is red
until fluttersdk/magic#152 lands; it skips in CI.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental re-review: the one new commit Since my last review: the version bump is back. All five version sites move My earlier Major findings on the cold-start buffer ( TestsNo behaviour changed, so no new coverage is expected. Checks I ran
|
|
@kodizm re-review request:
Both Majors, on one change. Minor 1. The replay re-runs Minor 2. Minor 3. Three manager tests and one doctor test, each verified to fail against the previous commit and pass against this one. |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental re-review of Since my last comment: Major
Verified against a synthetic tree: Minor
TestsThe three new manager tests cover read-then-subscribe-later, a live subscriber surviving Checks I ran
Only the six files in the |
The extension-point check I added last commit was right about WHAT identifies a Notification Service Extension and wrong about where to look for it. Reviewed, reproduced, and confirmed by hand before fixing. Walking all of `ios/` recursively and reading each `Info.plist` as UTF-8 crashes on any real project. `ios/Pods` is full of vendored frameworks whose plist is a BINARY plist; `readAsStringSync` throws a FileSystemException on one, nothing here caught it, and `getWarnings()` is called unguarded from `handle()`, so `notifications:doctor` and the `notifications_doctor` MCP tool both blew up instead of printing a warning. It only bit a project whose pbxproj already names a `.appex`, which is exactly this check's audience, and OneSignal's own iOS SDK arrives as an XCFramework through CocoaPods. Verified against a real binary plist written with plistlib: `readAsStringSync` throws, `utf8.decode(bytes, allowMalformed: true)` does not. The same walk followed links, so it descended `ios/.symlinks/plugins/*` into the pub cache and all of Pods. A dependency shipping an NSE template plist then read as THIS app's extension, which is the false green the extension-point check was added to remove. It also paid a full Pods walk on every run. Both close together: the walk starts at the immediate children of `ios/`, skips the directories that are never an app target (`Runner`, `Pods`, `.symlinks`, `build`, `Flutter`), never follows a link, and decodes tolerantly. Excluding Runner by NAME rather than by a `'/Runner/'` substring also fixes the third finding: that test did nothing on Windows, where the separator is a backslash. Two tests, both verified to fail against the previous commit: a binary plist under `ios/Pods` (asserting `returnsNormally` as well as the warning, since the regression was a crash) and a dependency's NSE template that must not count. Verified: analyze clean, format clean, 625 pass. The one failure is the stamp gate against the sibling magic checkout, which sits on another branch here; fluttersdk/magic#152 is merged, so master carries v0.3.0.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental re-review of Since my last comment: A top-level TestsThe two new cases are the coverage I asked for and they match the paths precisely: Checks I ran
Only the three files in the |
1 participant
What
Five commits, and the first is the one that matters: a push tapped while the app is CLOSED no longer depends on which order the consumer's service providers happen to be in. Then the doctor learns to report the iOS pieces no pub package can install, the agent-facing reference gains a stamp gate, and the release is cut as 0.3.0.
Why the cold-start fix
onPushClickedis a broadcast stream, and a broadcast stream drops what it publishes to nobody. On a cold start there is a window where nobody is there yet:onesignal_flutterbuffers the tap that launched the app and drains it in a microtask scheduled fromaddClickListener, which this manager calls insidedriver.initialize(), whichNotificationServiceProvider.boot()awaits.So a consumer whose provider list put notifications BEFORE
magic_deeplinkpublished the launch tap into an empty stream. The app finished booting onto its own initial route, with no exception and no log. Which order a consumer ends up with is decided by install order, because artisan's installer appends each provider to the end of the list, so this was a coin flip rather than a misconfiguration.The manager now holds clicks until the first listener and replays them once: filled only until the first-ever listener, drained once, never refilled, bounded at 20. A second listener arriving later is deliberately NOT handed the same tap again, because re-navigating an app somebody has since moved through is a worse failure than the one this closes.
Measured on a physical iPhone against a real server-sent notification, cold and warm.
Why 0.3.0 and not 0.2.1
This package's own cadence is minor for a feature release (0.0.3 -> 0.1.0 -> 0.2.0). The release carries the fix above plus the bulk preferences row, and that row needs two translation keys an adopter has to add, so a raw key renders where they do not.
^0.2.0is>=0.2.0 <0.3.0, so a patch would have slid that soft break into every consumer without a bump.The cost is two edits elsewhere, and forgetting the first is how a release train breaks:
magic_starterpinsmagic_notifications: ^0.2.0and uptizm pins the same.Order
Depends on fluttersdk/magic#152:
skill_reference_stamp_test.dartcompares this package's version against the reference's stamp in the sibling checkout, so it is red locally until that lands. It skips in CI, which clones no siblings.magic_starter(pin follows) alpha.27magic_deeplink0.1.0Testing
flutter analyzeclean, 619 tests pass. The one local failure is the stamp gate above.