fix(onesignal): the deeplink handler has never been wired, and now is - #14
Conversation
This package advertises opening a deeplink from a push notification. It has
never worked, on any release.
`DeeplinkServiceProvider` reached for the notification driver's
`onNotificationClicked` and cast it to `Stream<Map<String, dynamic>>`. The
declared type is `Stream<PushNotificationEvent>`, so the cast threw. The comment
above it read "Assume driver has onNotificationClicked stream", and the
assumption was the defect.
It could not have worked even with the right type. A consumer registers
`DeeplinkServiceProvider` before `NotificationServiceProvider`, so at boot there
is no driver to read a stream off, and the getter throws first.
Neither throw was ever seen, because both landed in a `catch (e)` whose body was
two comment lines. A feature can be inert across releases when nothing is
allowed to say so.
The fix is not the cast. `setup` now takes the notification MANAGER and
subscribes to its `onPushClicked`, which the manager owns from construction and
republishes onto when a driver attaches later. Provider order stops mattering
because there is nothing to be too early for, which is a better property than
ordering advice a consumer has to remember. It is also the subject-guarded
stream, so a push addressed to an identity this device no longer carries cannot
drive a navigation.
No dependency on `magic_notifications` was added and none will be. The coupling
stays optional and structural, resolved through `app.bound('notifications')` and
read without naming a type, because an app using deeplinks with no push at all
is a normal app and must not be made to carry a notifications package.
That has one consequence worth stating plainly, since no resolver can: this
release needs `magic_notifications` 0.1.0 or newer at RUNTIME, because
`onPushClicked` does not exist before it. Paired with an older one, the handler
reports at error level rather than routing. Loud is the point; the empty catch
is what made the last two years of this quiet.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. The diagnosis is right and the manager-stream subscription genuinely fixes the ordering problem, but dropping the try-catch moves a failure that used to be swallowed onto the app-boot path, and the payload type check repeats the same "assume the shape" pattern the PR is fixing. Major
Minor
TestsGood coverage of what changed: routing, notifications-registered-after-deeplinks ordering, notifications absent, manager without the stream, unreadable payload, and no bound logger. Not covered: a payload whose runtime map type is not Checks I ran
|
Three findings from review on #14, all confirmed in source before fixing. `app.make('notifications')` runs the binding factory and `onPushClicked` is a getter, so either can throw. The handler answers `NoSuchMethodError` by name, because that one means "this build of magic_notifications is too old", but a `StateError` out of an uninitialised manager is a different thing and escaped. magic's `Application.boot` awaits providers in a bare loop with no error handling (`foundation/application.dart:375`), so that escape did not degrade the deep-link feature: it stopped the app booting and took every provider registered after this one with it, over a plugin that is optional by design. The resolution is now guarded and reports at error level through the same seam every other failure here uses. This is not the empty catch this branch removed: that one had two comment lines for a body and is why the feature stayed inert across two years of releases. This one names what failed, then lets boot continue. `.claude/rules/flutter.md:16` asks for exactly this shape. The handler was also constructed inline and its reference discarded, so the `dispose` that `doc/basics/handlers.md` tells consumers to call from provider teardown could never reach it. The provider now holds what it wired and exposes `dispose()`. And `setup`'s early return for an unresolvable stream sat before the cancel, so re-wiring against a manager this handler cannot follow left it routing taps through the previous one. The cancel moves ahead of the return. The review's fourth finding, that `extractData` should accept any `Map` rather than `Map<String, dynamic>`, is not taken. The reviewer said it could not verify the declared type because `magic_notifications` is deliberately not a dependency; it is `Map<String, dynamic>` (`push_driver.dart:7`), non-nullable, and both drivers narrow to it before publishing (mobile passes `additionalData ?? {}`, web's `_payloadOf` returns an already-narrowed map or a context-typed `const {}`). A `Map<String, String>` satisfies the check anyway under Dart's covariance, and a `Map<Object?, Object?>` cannot reach the field without a cast that throws at the driver. Loosening it would defend against a state the type system prevents. Four tests, each verified to go red against a mutant of the line it covers: a throwing factory and a throwing getter both leave `app.isBooted` true and log, `dispose()` stops routing, and a second `setup` against an unfollowable manager drops the first subscription.
|
Thanks, three of the four are real and are fixed in 0e93850. I verified each against the source before acting. Major 1, unguarded resolution aborting boot: confirmed and fixed. Minor 1, unreachable Minor 2, cancel after the early return: confirmed and fixed. The cancel moves ahead of the return. Major 2, the payload type check: not taken, and I think it is refuted rather than deferred. You flagged your own uncertainty here and said On the coverage gaps you named, both are now covered, and each test was verified to go red against a mutant of the exact line it covers: a throwing factory and a throwing getter each leave 83 tests, |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of Since my last review: the notifications resolution in Minor
TestsBoth gaps I named are now covered and both assert the property that matters ( Checks I ran
|
…vider Two findings from the second review pass on #14, both real. `dispose()` tore down only the push-click handler while the driver's own link subscription and the driver itself survived it. The doc comment scoped that claim honestly, so nothing was wrong today, but `doc/basics/handlers.md` tells a consumer to call teardown from their service provider, and a provider-level `dispose()` that leaves the provider running is a name arguing against its own behaviour. It now drops all three and is idempotent, because a consumer calling it does not know which parts a given deployment wired. `doc/architecture/service-provider.md` was not synced with the previous commit, which this repo's post-change checklist asks for. The OneSignal snippet still showed the unguarded call, the "two layers of defence" list was one short, and `dispose()` appeared nowhere on the page. All three fixed, plus a short teardown section the handlers doc can point at. The teardown test asserts what is actually observable, that dispose completes on a driver-wired provider and repeats safely, and a comment says plainly what it does NOT cover: `AppLinksDriver.onLink` is the `app_links` package stream with no injection seam, so a unit test cannot emit on it and the cancellation itself is read from the source rather than exercised. 84 tests, analyze clean, format clean.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of Minor
Both are the same shape: the doc and CHANGELOG claim total teardown, and the manager-side driver plus the pending timer sit outside it. Either narrow the claim or add TestsThe new test pins that Checks I ran
|
Both findings from the third review pass on #14, and both are the same shape: the doc I wrote last commit claimed total teardown while two things sat outside it. The claim is the better half to keep, so the code moved to meet it. `boot` calls `manager.setDriver(driver)` on the `DeeplinkManager` singleton, and `dispose()` cleared only the provider's own field, so afterwards `manager.driver` still answered with a driver this provider believed it had torn down and `getInitialLink()` still called through it. `forgetDriver()` has existed on the manager the whole time with no caller; it has one now. Benign today only because `AppLinksDriver.dispose()` is an empty method, which is not a property to build on. The initial-link read is scheduled with `Future.delayed(Duration.zero)` and there is no handle to cancel that with, so a teardown in the same turn as boot left the callback to run and route a deep link afterwards. It checks a disposed flag on both sides of its await now: before the read for the same-turn case, after it for a teardown that lands while the read is in flight. The manager-driver fix has a test, mutation-verified. The disposed flag does NOT, and a comment in the test file says so rather than implying otherwise. One was written and deleted: it stayed green against a build with both checks removed, because `forgetDriver()` makes the callback's `getInitialLink()` raise into a fire-and-forget future that swallows it, so the assertion held either way. Isolating it needs a `getInitialLink()` that answers a URI, which needs a seam `AppLinksDriver` does not have. The checks stay because relying on that throw is the swallowed-error shape this release exists to remove; an early return is the deliberate version of the same outcome. Writing the first test also corrected an assumption: `DeeplinkManager.driver` raises `DeeplinkException` when nothing is configured rather than answering null, so the assertion is that reaching for it throws. 85 tests, analyze clean, format clean.
|
Both fixed in e31be35, and you framed the choice correctly: the claim was the better half to keep, so the code moved to meet it rather than the doc narrowing.
The scheduled initial-link read: confirmed and fixed, with a disposed flag checked on both sides of the await, since a teardown can also land while On covering the second one, a report rather than a claim. I wrote the test you would expect, it passed, and then it also passed against a build with both checks removed. The reason is the first fix: Writing the first test also corrected an assumption of mine worth noting for the file: 85 tests, |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of The two Minor
TestsThe new test ( Checks I ran
|
…subscription Reported on #14. `_driver` is assigned, then `await driver.initialize(...)` suspends; a teardown in that window cleared `_driver` and forgot the manager's, and then boot resumed and assigned `_links` anyway, creating a subscription AFTER the teardown meant to have caught it, which nothing would ever cancel. Same class as the scheduled read below it, and the flag that already existed answers it. It returns out of `boot` rather than skipping the one block, deliberately: attaching the push-click handler further down to a provider somebody has torn down is the same defect one block later. The driver is disposed on the way out, because `dispose()` ran before that assignment and never saw it. Not covered by a test, for the reason the deleted one taught: `AppLinksDriver` offers no seam to suspend `initialize()` from a test, so any assertion would pass against the unguarded build too. 85 tests, analyze clean, format clean.
|
Fixed in d49106d. Confirmed exactly as you described: It returns out of Not covered by a test, and I am saying that rather than shipping one: Thank you for checking the placement of the two 85 tests, analyze clean, format clean. |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of Minor
Nothing else in the diff. The TestsNot covered, and your reply says so plainly and correctly: Checks I ran
|
Reported on #14, and the finding is against my own reasoning rather than the outcome. The comment claimed the driver had to be disposed there because `dispose()` ran before the assignment and never saw it. That path does not exist: `_driver = driver` sits before the only suspension in the block, so the only way to reach the guard with the flag set is a teardown inside that await, and by then `dispose()` has already seen `_driver`, disposed it and forgotten the manager's. So the call was always a second dispose, benign only while `AppLinksDriver.dispose()` has an empty body and stopping the moment it releases anything. Dropped, and the comment now says which path actually reaches the line instead of inventing one. 85 tests, analyze clean, format clean.
|
Fixed in 6739c27, and this one is a finding against my reasoning rather than my outcome, which is the harder kind to catch. You are right that the path the comment described does not exist. Dropped rather than made idempotent: there is no first-dispose path here to protect, so a second dispose is not a case to support. The comment now names the path that actually reaches the line instead of inventing one. 85 tests, analyze clean, format clean. |
…#15) Cuts 0.0.3, carrying the handler repair merged in #14. Five version sites, three of which were still on 0.0.1 because the 0.0.2 release moved only the pubspec and the changelog. A sweep for the current version would have missed the same three again, which is the argument for grepping the shape. One changelog claim expired while the branch was open: it said onPushClicked arrives in a magic_notifications 0.1.0 that was not yet released. That went out earlier today, so the entry states the floor as a fact now. 85 tests, analyze clean, format clean.
1 participant
This package advertises opening a deeplink from a push notification. It has never worked, on any release.
DeeplinkServiceProviderreached for the notification driver'sonNotificationClickedand cast it toStream<Map<String, dynamic>>. The declared type isStream<PushNotificationEvent>, so the cast threw. The comment above it read "Assume driver has onNotificationClicked stream", and the assumption was the defect.It could not have worked even with the right type. A consumer registers
DeeplinkServiceProviderbeforeNotificationServiceProvider, so at boot there is no driver to read a stream off, and the getter throws first.Neither throw was ever seen, because both landed in a
catch (e)whose body was two comment lines. A feature can be inert across releases when nothing is allowed to say so.The fix
Not the cast.
setupnow takes the notification MANAGER and subscribes to itsonPushClicked, which the manager owns from construction and republishes onto when a driver attaches later. Provider order stops mattering because there is nothing to be too early for, which is a better property than ordering advice a consumer has to remember. It is also the subject-guarded stream, so a push addressed to an identity this device no longer carries cannot drive a navigation.The empty catch is gone. A failure now reports at error level.
The coupling stays optional
No dependency on
magic_notificationswas added and none will be. The coupling stays optional and structural, resolved throughapp.bound('notifications')and read without naming a type, because an app using deeplinks with no push at all is a normal app and must not be made to carry a notifications package.That has one consequence worth stating plainly, since no resolver can express it: this release needs
magic_notifications0.1.0 or newer at RUNTIME, becauseonPushClickeddoes not exist before it. Paired with an older one, the handler reports at error level rather than routing. Loud is the point.Verification
79 tests,
dart analyzeclean,dart formatclean.The version bump ships separately as
chore(release): 0.0.3.