fix(routing): name every page so navigator observers can tell screens apart - #121
Merged
Merged
Conversation
… apart `GoRoute.name` names the ROUTE and never reaches `RouteSettings`, so a `NavigatorObserver` reading `route.settings.name` got null on every push. That silently disables anything screen-aware: analytics, breadcrumb trails, and Sentry's Flutter Web release health. The Sentry case is the one that hurts, because it fails without a symptom. Its transport keeps working and events keep arriving, while the session count stays at zero forever: `WebSessionHandler.startSession` only fires when the name changes, or on the first navigation when it is exactly `/`. Measured on a deployed app before this fix, a browser with no ad blocker made zero requests to Sentry's ingest across three route changes, while a forced captureMessage from the same page returned 200. All five pages the transition switch returns now carry `route.routeName ?? route.fullPath`. The fallback is the path rather than nothing, because `.name()` is optional and most routes never call it, so keying only on `routeName` would have left the common case as broken as before. The path is always present, already unique per route, and on the root route it produces the `/` that the first-session rule wants.
There was a problem hiding this comment.
Pull request overview
This PR fixes routing observability by ensuring every page built by MagicRouter is assigned a RouteSettings.name, allowing NavigatorObservers (analytics/Sentry/breadcrumbs) to reliably distinguish screens.
Changes:
- Set a
pageNameon all page types produced by the router transition switch (route.routeName ?? route.fullPath). - Add widget tests that assert page names are present for named routes, unnamed routes (path fallback), and after navigation.
- Update routing docs and add a changelog entry describing the behavior change.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/src/routing/magic_router.dart | Assigns a non-null name to pages returned by the router’s transition builder. |
| test/routing/page_route_name_test.dart | Adds widget coverage verifying observers can read non-null page names (route name or path fallback). |
| doc/basics/routing.md | Documents how page names are derived for observers (route name vs path). |
| CHANGELOG.md | Records the fix under Fixed for the unreleased version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Both camera pickers returned the fallback future without awaiting it, so the future escaped the try block before completing: a failure inside the fallback never reached the catch, and the onError callback the caller passed never fired. Flutter 3.47 added unawaited_return_in_try_block, which turned that latent bug into two analyzer warnings and a red Lint & Test job on every branch, including ones that never touch this file. Awaiting fixes the reporting and unblocks CI at the same time. Also pins the layout case for page naming: layouts compile to ShellRoute, and since the shell takes no navigatorKey its children push onto the root navigator, so a root-registered observer does see them. Raised in review as a possible gap; measured instead of assumed, and now covered by a test.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
anilcancakir
added a commit
to anilcancakir/uptizm
that referenced
this pull request
Aug 14, 2026
…shboard (#51) Both were measured on the deployed app after a question that started as 'why has nothing arrived from the client', and both are invisible from the code. AD BLOCKERS. Since v9 the SDK fetches the Sentry Browser JS bundle from browser.sentry-cdn.com, a URL hardcoded in sentry_js_bundle.dart and reachable through no option. Blocklists match it by domain, so a blocked visitor never fetches the script and the SDK never initialises; WebSdkIntegration catches the failure and does not rethrow. A same-origin tunnel does not help, because it would move only the event-sending leg, and sentry-dart has no tunnel option anyway (getsentry/sentry-dart#872, open since 2022). Client counts are a floor, not a measurement. Backend and worker are server to server and unaffected. SESSIONS NEED ROUTE NAMES. WebSessionHandler.startSession fires only when the route name changes, or on first navigation when it is exactly '/', and it reads RouteSettings.name rather than GoRoute.name. magic left pages unnamed, so this app reported zero sessions while its transport worked: a browser with no ad blocker made zero ingest requests across three route changes while a forced capture from the same page returned 200. Fixed upstream in fluttersdk/magic#121.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Every page this router builds was anonymous.
GoRoute.namenames the ROUTE andnever reaches
RouteSettings, so aNavigatorObserverreadingroute.settings.namegotnullon every push and could not distinguish onescreen from another.
That silently disables anything screen-aware.
doc/basics/routing.mdhas showna
SentryNavigatorObserverexample in this exact section for a while, so thedocumentation promised something the router did not deliver.
Why it is worth a fix rather than a note
The Sentry case fails without a symptom, which is the worst shape a bug can
have. Its transport keeps working, events keep arriving, and the session count
stays at zero forever, because
WebSessionHandler.startSessiononly fires whenthe name CHANGES (or on the first navigation when it is exactly
/).Measured on a deployed app before this fix: a real browser with no ad blocker
made zero requests to Sentry ingest across three route changes, while a
forced
captureMessage()from the same page returned 200. Transport fine,release health permanently empty, nothing in any log.
The fix
All five pages the transition switch returns now carry:
The fallback is the path rather than nothing, and that is the load-bearing half:
.name()is optional and most routes never call it, so keying only onrouteNamewould have left the common case exactly as broken. The path isalways present, already unique per route, and on the root route it produces the
/that the first-session rule happens to want.Testing
Three widget tests drive the real router with a recording
NavigatorObserver,which is precisely how a screen-aware observer reads this: a named route, an
unnamed one, and a navigation that asserts no name ever comes back null.
Full suite green (1293 tests),
dart analyzeclean,dart formatclean.CHANGELOG.mdanddoc/basics/routing.mdupdated per the post-change syncrule.