Survive the login bounce with an intended url - #126
Conversation
Wave 1 of the deep-link plan. EnsureAuthenticated bounced an unauthenticated visitor to login and recorded nothing, so a cold start at a deep link lost its destination. It now records the location through MagicRouter.setIntendedUrl, which existed with zero callers, and a new NavigatesRoutes.navigateHome reads it back once the user authenticates. Nothing is recorded for the login route or any other guest-only auth route, since a visitor bounced off one of those cannot use it as a destination either. All five post-auth navigations now go through navigateHome, which makes it the single seam. Known limit, recorded in the docblock: redirectTarget only ever sees state.matchedLocation, which carries no query string.
Carries the intended-URL replay: a deep link that lands on a signed-out device returns to its destination after login.
The two starter:* banners read magicStarterVersion, and its own test compares it against pubspec.yaml for exactly this reason: a literal nothing compares against drifts silently, and this one was already twenty-four releases stale once before.
//host/path starts with a slash and is a different origin, so the leading-slash test alone admitted it. Nothing reachable can store one today, since only a router location is ever recorded, which is why the extra condition is cheap enough to keep.
…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 still document the notification UI that had moved out of this package entirely. 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.
0.3.0 is where a push tapped on a cold start stops depending on which order the consumer's provider list happens to be in, which was decided by install order rather than by anything a consumer chose. `^0.2.0` is `>=0.2.0 <0.3.0`, so it excludes that release: leaving this line alone would hold every adopter of this starter on the version where a cold tap silently opens the wrong screen. This is the step in a release train that gets skipped, because nothing fails until an adopter resolves the published graph and finds the fix absent.
|
The 0.3.0 is not on pub.dev yet; it is fluttersdk/magic_notifications#27, which has to be published before this merges. That is intrinsic to a release train rather than something to work around: this package's alpha.27 has to carry It goes green on a re-run once 0.3.0 is published. Do not downgrade the constraint to make it pass, which is what pub's own suggestion in that log proposes. The other two checks are unaffected: |
|
Correcting my previous comment:
Worth noting because it changes where this test can fire: So both red checks on this PR clear in the same order, and neither needs a code change:
Nothing in this branch needs editing for either. |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. The intended-url logic itself is sound and tested, but two things in this branch are red right now: the package no longer resolves at all, and the new stamp test fires in the blocking CI job rather than skipping there. Critical
Minor
Tests
Checks I ran
|
This reverts commit ecf6ffe.
|
Dropped the Raising it before 0.3.0 was published is what made That leaves one red check, |
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of the one commit since Only MinorPR title - still reads "and follow Still open from my earlier reviewThe three Minors on the intended-url logic itself are unaddressed and unanswered: intent recorded on a logout-triggered bounce ( The TestsUnchanged by this commit. Checks I ran
|
Raised in review. Flipping the auth state makes go_router re-run its redirects while the app is still sitting on the protected route, so `EnsureAuthenticated` writes that route down as somewhere to return to, in the gap between `Auth.logout()` and the navigation to login. It belongs to the session that just ended. Sign out on /teams/settings, hand the device over, and the next person to sign in lands there rather than on home. The destination refetches under the new token so this is not an exposure, but it is somebody else's page and nobody asked for it. The account-deletion path had the sharper version of the same thing: it sent the next sign-in to a deleted account's settings screen. Read-and-discard through `pullIntendedUrl`, which is the one-time read; `MagicRouter` exposes no separate clear. Also from the same review: `doc/basics/authentication.md` still said login "navigates to `MagicStarterConfig.homeRoute()`", which has been only the fallback since this branch landed `navigateHome()`. Test verified to fail without the clear.
anilcancakir
changed the title
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of the one commit since Scope: Major
Minor
Still open from my earlier review
Tests
Checks I ran
|
…logout call site Second review pass, and the reviewer was right that my first fix was the wrong shape. Clearing after each `Auth.logout()` covered the two logouts a user asks for and missed the one that matters most: magic's `AuthInterceptor` calls `Auth.logout()` itself when a token refresh fails (`auth_interceptor.dart:77`) and `AuthServiceProvider` installs that interceptor unconditionally, so a session that simply EXPIRES on a protected route still recorded it. No call site can reach that one, and it is a regression this branch introduced: before it, the value was never recorded at all. `SessionScopeSync` already listens to `Auth.stateNotifier`, which is the single funnel all three pass through, and already has the branch for the identity dropping to null. The clear goes there and the two call-site clears come out. Deferred by a microtask, which is load-bearing rather than caution. The notifier fans out in registration order: this listener attaches during the starter provider's boot, go_router's attaches when magic builds the router AFTER boot, so a synchronous clear runs BEFORE the redirect that records the value and clears nothing. Named limit, carried into the changelog: a host route with an async `redirect` records after the microtask and is not covered. Three tests. Two fail against the previous commit (a session ending, and a logout no call site performed). The third pins the opposite direction, that an intent survives the login it was recorded for, and it takes TWO guards failing together to break, so it survives removing either alone; verified against a mutant that drops both. Its comment says so rather than claiming more than it measures. The changelog gained the rule itself, which the previous commit synced into `doc/` and not there.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of the one commit since Scope: Major
Minor
Settled / still openThe microtask deferral is sound and does not depend on listener order as the comment implies: the whole record path ( Tests
Checks I ran
|
Third review pass on the same defect, and the reviewer was right twice running. The clear moved onto the auth notifier, which was the right funnel, but it went into `SessionScopeSync` and that class is OPT-IN: `attach()` is the host's call (`doc/basics/session-scope.md:111`) and nothing in `lib/src/providers/`, `install.yaml` or `assets/stubs/` calls or scaffolds it. Verified by grep. So an app that never adopted `SessionScopedController` lost the clear entirely, and the previous commit had deleted the two unconditional call-site clears on the way past. Strictly worse than either shape before it. It now hangs off `MagicStarterServiceProvider.boot()`, which every starter app runs. Same notifier, same microtask deferral, same `Auth.check()` re-check, and the docblock says why it is not in `SessionScopeSync` so the next person does not move it back. The tests moved with it, and this is the point rather than bookkeeping: they lived in the session-scope suite, which calls `attach()` itself, so they proved the clear worked in exactly the configuration an adopter might not have. They now boot the real provider, which is what an adopter has. Verified to fail against a provider without the listener. The duplicate the reviewer flagged is gone too. "A logout no call site performed" drove the same `Auth.logout()` as the test above it, so it was a second copy rather than coverage of the interceptor path; one test on the notifier covers every route into it, which is the whole argument for putting the clear there. `doc/basics/authentication.md` and the changelog both said `SessionScopeSync` clears it and read as unconditional guarantees. Both now name the provider and say no host wiring is needed.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of the one commit since Scope: Major
Minor
Still open from my earlier reviews
SettledThe move off Tests
Checks I ran
|
Third finding on the same clear, and the reviewer proved it by running the suite rather than reading it: `flutter test test/middleware/ensure_authenticated_test.dart --test-randomize-ordering-seed=1` failed, and declaration order passed. I reproduced both. `if (_authState != null) return;` was a one-way latch over a static nothing clears, while `Auth.stateNotifier` resolves through the container and hands back a DIFFERENT notifier every time the guard is re-bound. So the first boot in a process subscribed and every later one returned early, leaving the listener on a notifier nobody bumps: no clear at all. `SessionScopeSync` solves exactly this with `detach()` and says why in its own docblock; I copied the field and not the escape. It now compares the held notifier against the current one by identity and MOVES the subscription. That needs the listener to be a named static rather than a closure, since `removeListener` matches by identity and a fresh closure never equals the one that was added. The knock-on the reviewer also caught: with the latch, the second test booted a provider that early-returned, so its listener sat on the previous test's notifier and the assertion held vacuously. Its comment claimed a mutation verification that the ordering had quietly disabled. Both are real again, and verified: five seeds (1, 2, 3, 12345, 777) all pass, and a mutant with both `Auth.check()` guards removed fails the survives-login test under seed 1. Dropped the "a hot restart cannot double-subscribe" line while I was there. A hot restart re-runs main with statics reset, so the field is null again anyway; the case it has to survive is the re-bind, which is the one it was not surviving.
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. Incremental review of the one commit since Scope: The knock-on Minor is fixed with it: Still open from my earlier reviews
Tests
Checks I ran
|
Raised in three consecutive reviews and never answered, which is my omission rather than the reviewer's persistence. The branch IS unreachable through this package's own routes: `auth_routes.dart:19` registers the whole auth group under `middleware: ['guest']`, so `EnsureAuthenticated` never sees `/auth/register` or its siblings. Verified. It stays because this middleware is public API and a host applying `auth` globally over a shell route is a supported configuration, not a hypothetical. Without the branch such an app bounces a visitor off `/auth/register`, records it, and sends them back to a guest-only route after they sign in. Dead for us, live for an adopter, and the two tests over it call `redirectTarget` directly so nothing shows the router reaching it. All of that now sits in the docblock instead of in a review thread.
|
Note Kodizm (AI-generated). May contain mistakes; verify before acting. Incremental review of the one commit since Scope: Settled
TestsNo test change, correctly - nothing executable changed. Coverage is as I described last review: Checks I ran
|
1 participant
What
A deep link that lands on a signed-out device now survives the login bounce, and the
magic_notificationspin follows that package to^0.3.0.Why the intended url
A link into a protected screen on a device nobody is signed into used to bounce to login and then land on the home route, dropping the destination.
EnsureAuthenticated.redirectTargetnow records the requested location viaMagicRouter.setIntendedUrlbefore bouncing, andNavigatesRoutes.navigateHomereads it back withpullIntendedUrlonce they authenticate, falling back toMagicStarterConfig.homeRoute().Nothing is recorded for the login route itself or for any other guest-only auth route (register, forgot-password, reset-password, two-factor-challenge, otp), since a bounced visitor cannot use one of those as a destination either. All five post-auth navigations now go through
navigateHome().Known limit, stated rather than discovered later:
redirectTargetonly ever seesstate.matchedLocation, which carries no query string, so a recorded intent loses any?token=...the original link carried.Why the pin has to move
magic_notifications0.3.0 is where a push tapped on a COLD START stops depending on provider order, which was decided by install order rather than by anything a consumer chose.^0.2.0is>=0.2.0 <0.3.0, so it excludes that release: leaving the line alone would hold every adopter of this starter on the version where a cold tap silently opens the wrong screen.This is the step in a release train that gets skipped, because nothing fails until an adopter resolves the published graph and finds the fix absent.
Order
Depends on fluttersdk/magic#152 only, and not for the code:
skill_reference_stamp_test.dartreadsplugin-starter.mdfrom magic's DEFAULT branch, which this repo's CI clones withgit clone --depth 1and no--branch(ci.yml:37-40). That stamp is stillalpha.23; #152 carriesalpha.27.Sibling working treesgoes green on a re-run once it merges.The
magic_notificationspin bump that used to sit here is gone, so nothing in this PR waits on a publish.Testing
flutter analyzeclean.