From 47768074d212e58ce8982685bbbb0ccb85d03fcd Mon Sep 17 00:00:00 2001 From: Anilcan Cakir Date: Sat, 15 Aug 2026 00:57:24 +0300 Subject: [PATCH 1/2] fix(routing): name every page so navigator observers can tell screens 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. --- CHANGELOG.md | 4 + doc/basics/routing.md | 15 +++ lib/src/routing/magic_router.dart | 24 ++++- test/routing/page_route_name_test.dart | 122 +++++++++++++++++++++++++ 4 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 test/routing/page_route_name_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index 13d6695..d35f712 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Fixed + +- **Every page this router built was anonymous, which silently disabled every screen-aware observer.** `GoRoute.name` names the ROUTE and never reaches `RouteSettings`, so a `NavigatorObserver` reading `route.settings.name` got `null` on every push and could not tell one screen from another. Analytics, breadcrumb trails and Sentry's Flutter Web release health all key on exactly that value, and the last one fails in the worst possible way: the transport keeps working, events keep arriving, and the session count sits at zero forever with nothing in any log to explain it, because `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 exactly 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. (`lib/src/routing/magic_router.dart`, `test/routing/page_route_name_test.dart`) + ### Improvements - **The FileStore expiration test no longer races the clock, so master stops going red at random.** `it handles expiration` wrote a value with a 100ms TTL and immediately asserted it was readable. That window had to survive a file write plus the scheduler, and on a loaded CI runner it did not: the entry expired before the read and the assertion failed with `Expected: 'value' Actual: ` while the store was behaving correctly. It failed twice today, once on a PR and once on master after merge. The readable case now uses a 5-minute TTL, and the expiry case passes an already-elapsed TTL so `expire_at` lands in the past by construction, which removes the wall-clock delay entirely (a delay can only ever be too short, never too long). (`test/cache/drivers/file_store_test.dart`) diff --git a/doc/basics/routing.md b/doc/basics/routing.md index 305f23d..dae0db2 100644 --- a/doc/basics/routing.md +++ b/doc/basics/routing.md @@ -481,6 +481,21 @@ class RouteServiceProvider extends ServiceProvider { } ``` +Every page carries a name in its `RouteSettings`, which is what an observer +reads to tell one screen from another. It is the route's `.name()` when you set +one, and the route path otherwise: + +```dart +MagicRoute.page('/orders', () => OrdersPage()).name('orders'); // -> 'orders' +MagicRoute.page('/settings', () => SettingsPage()); // -> '/settings' +``` + +> [!NOTE] +> `GoRoute.name` and `RouteSettings.name` are different things, and only the +> second one reaches an observer. Anything that identifies screens depends on +> it: analytics, breadcrumb trails, and Sentry's Flutter Web release health, +> which starts a session only when it sees this value change. + Observers are passed directly to GoRouter and receive all navigation events (`didPush`, `didPop`, `didReplace`, `didRemove`). > [!NOTE] diff --git a/lib/src/routing/magic_router.dart b/lib/src/routing/magic_router.dart index 233cdd8..a666383 100644 --- a/lib/src/routing/magic_router.dart +++ b/lib/src/routing/magic_router.dart @@ -342,10 +342,25 @@ class MagicRouter { // Wrap child with opaque background to prevent overlap during transitions final opaqueChild = Material(type: MaterialType.canvas, child: child); + // The name a NavigatorObserver will read off this page. + // + // `GoRoute.name` names the ROUTE and never reaches `RouteSettings`, so + // without this every observer sees null and cannot tell one screen from + // another. That silently disables anything screen-aware: analytics, + // breadcrumb trails, and Sentry's web release health, which starts a + // session only when it sees this value change and otherwise reports zero + // sessions forever with nothing in any log to explain it. + // + // Falls back to the path because `.name()` is optional and most routes + // skip it, so keying only on `routeName` would leave the common case as + // broken as before. The path is always present and already unique. + final pageName = route.routeName ?? route.fullPath; + switch (route.transitionType) { case RouteTransition.fade: return CustomTransitionPage( key: state.pageKey, + name: pageName, child: opaqueChild, transitionsBuilder: (context, animation, secondaryAnimation, child) { return FadeTransition(opacity: animation, child: child); @@ -355,6 +370,7 @@ class MagicRouter { case RouteTransition.slideRight: return CustomTransitionPage( key: state.pageKey, + name: pageName, child: opaqueChild, transitionsBuilder: (context, animation, secondaryAnimation, child) { // Incoming page slides from right @@ -391,6 +407,7 @@ class MagicRouter { case RouteTransition.slideUp: return CustomTransitionPage( key: state.pageKey, + name: pageName, child: opaqueChild, transitionsBuilder: (context, animation, secondaryAnimation, child) { return SlideTransition( @@ -412,6 +429,7 @@ class MagicRouter { case RouteTransition.scale: return CustomTransitionPage( key: state.pageKey, + name: pageName, child: opaqueChild, transitionsBuilder: (context, animation, secondaryAnimation, child) { return ScaleTransition( @@ -423,7 +441,11 @@ class MagicRouter { case RouteTransition.none: // No animation - instant page switch - return NoTransitionPage(key: state.pageKey, child: opaqueChild); + return NoTransitionPage( + key: state.pageKey, + name: pageName, + child: opaqueChild, + ); } } diff --git a/test/routing/page_route_name_test.dart b/test/routing/page_route_name_test.dart new file mode 100644 index 0000000..f0a9e4f --- /dev/null +++ b/test/routing/page_route_name_test.dart @@ -0,0 +1,122 @@ +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:magic/magic.dart'; + +/// Tests that every page this router builds carries a name in its +/// [RouteSettings]. +/// +/// ## Why this matters beyond tidiness +/// +/// A `NavigatorObserver` sees pages, not routes. `GoRoute.name` names the ROUTE +/// and never reaches `RouteSettings`, so an observer reading +/// `route.settings.name` gets null for every push unless the `Page` itself was +/// given one. Every observer that identifies screens is therefore blind: +/// analytics, breadcrumbs, and Sentry's web release health, which starts a +/// session only when it sees the name change +/// (`WebSessionHandler.startSession`). That last one fails silently and +/// completely: the transport works, events arrive, and the session count stays +/// at zero forever with nothing in any log to explain it. +/// +/// ## Why the fallback is the path +/// +/// `RouteDefinition.name()` is optional and most routes never call it, so +/// naming pages only when `routeName` exists would leave the common case +/// exactly as broken as before. The full path is always present, is already +/// unique per route, and is what a reader recognises in a breadcrumb trail. +/// It also happens to satisfy Sentry's first-session rule, which starts a +/// session on the very first navigation only when the name is `/`. +void main() { + setUpAll(() { + TestWidgetsFlutterBinding.ensureInitialized(); + }); + + setUp(() { + MagicApp.reset(); + Magic.flush(); + TitleManager.reset(); + MagicRouter.reset(); + }); + + group('page names', () { + testWidgets('a named route names its page with the route name', ( + tester, + ) async { + MagicRoute.page('/', () => const SizedBox()).name('home'); + + final observer = _NameRecordingObserver(); + MagicRouter.instance.addObserver(observer); + + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + expect(observer.names, contains('home')); + }); + + testWidgets('an unnamed route falls back to its path', (tester) async { + // The common case: `.name()` is optional and most routes skip it, so a + // fix that only handled named routes would leave the majority broken. + // + // The path this asserts is also the one Sentry's web session tracking + // treats specially: it starts a session on the very first navigation + // only when the name is exactly `/`, which the fallback produces for + // free on the root route. + MagicRoute.page('/', () => const SizedBox()); + + final observer = _NameRecordingObserver(); + MagicRouter.instance.addObserver(observer); + + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + expect(observer.names, contains('/')); + }); + + testWidgets('a page name never comes back null after navigation', ( + tester, + ) async { + MagicRoute.page('/', () => const SizedBox()).name('home'); + MagicRoute.page('/profile', () => const SizedBox()).name('profile'); + + final observer = _NameRecordingObserver(); + MagicRouter.instance.addObserver(observer); + + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + MagicRoute.to('/profile'); + await tester.pumpAndSettle(); + + expect(observer.names, contains('profile')); + expect( + observer.names.contains(null), + isFalse, + reason: + 'A null name is what silently disables every screen-aware observer.', + ); + }); + }); +} + +/// Records the name of every route pushed or replaced, exactly as a +/// screen-aware observer reads it. +class _NameRecordingObserver extends NavigatorObserver { + final List names = []; + + @override + void didPush(Route route, Route? previousRoute) { + names.add(route.settings.name); + } + + @override + void didReplace({Route? newRoute, Route? oldRoute}) { + if (newRoute != null) { + names.add(newRoute.settings.name); + } + } +} From e78d319c4e3f70fe9e9bae48d90a6f597add6d6e Mon Sep 17 00:00:00 2001 From: Anilcan Cakir Date: Sat, 15 Aug 2026 01:09:03 +0300 Subject: [PATCH 2/2] fix(pick): await the gallery fallbacks so their failures are reported 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. --- CHANGELOG.md | 1 + doc/basics/routing.md | 5 ++++ lib/src/facades/pick.dart | 8 +++++-- test/routing/page_route_name_test.dart | 33 ++++++++++++++++++++++++++ 4 files changed, 45 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d35f712..a1c0a35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to this project will be documented in this file. ### Fixed +- **`Pick`'s two gallery fallbacks escaped their own error handling, and CI could not build until it was fixed.** `pickFromCamera` and `pickVideoFromCamera` returned the fallback future without awaiting it, so the future left the `try` block before completing: a failure inside the fallback never reached the `catch`, and the `onError` callback the caller supplied never fired. Flutter 3.47 added `unawaited_return_in_try_block`, which turned the latent bug into two analyzer warnings and a red `Lint & Test` job on every branch, including ones that never touch this file. Both are awaited now, which fixes the reporting and the build together. (`lib/src/facades/pick.dart`) - **Every page this router built was anonymous, which silently disabled every screen-aware observer.** `GoRoute.name` names the ROUTE and never reaches `RouteSettings`, so a `NavigatorObserver` reading `route.settings.name` got `null` on every push and could not tell one screen from another. Analytics, breadcrumb trails and Sentry's Flutter Web release health all key on exactly that value, and the last one fails in the worst possible way: the transport keeps working, events keep arriving, and the session count sits at zero forever with nothing in any log to explain it, because `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 exactly 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. (`lib/src/routing/magic_router.dart`, `test/routing/page_route_name_test.dart`) ### Improvements diff --git a/doc/basics/routing.md b/doc/basics/routing.md index dae0db2..7cf9649 100644 --- a/doc/basics/routing.md +++ b/doc/basics/routing.md @@ -496,6 +496,11 @@ MagicRoute.page('/settings', () => SettingsPage()); // -> '/settings > it: analytics, breadcrumb trails, and Sentry's Flutter Web release health, > which starts a session only when it sees this value change. +Pages inside a layout are named the same way and reach the same observers. +Layouts compile to `ShellRoute`, and because the shell does not take its own +`navigatorKey`, its children push onto the root navigator that your observers +are already watching. + Observers are passed directly to GoRouter and receive all navigation events (`didPush`, `didPop`, `didReplace`, `didRemove`). > [!NOTE] diff --git a/lib/src/facades/pick.dart b/lib/src/facades/pick.dart index 27ec2b6..bdcfc63 100644 --- a/lib/src/facades/pick.dart +++ b/lib/src/facades/pick.dart @@ -149,7 +149,10 @@ class Pick { // User cancelled camera - check if we should fallback if (xFile == null && fallbackToGallery) { - return image( + // Awaited, not just returned: an un-awaited future escapes this try + // block, so a failure inside the gallery fallback would never reach the + // `catch` below and `onError` would never fire. + return await image( maxWidth: maxWidth, maxHeight: maxHeight, imageQuality: imageQuality, @@ -257,7 +260,8 @@ class Pick { // User cancelled - check if we should fallback if (xFile == null && fallbackToGallery) { - return video(maxDuration: maxDuration); + // Awaited for the same reason as the image fallback above. + return await video(maxDuration: maxDuration); } return xFile != null ? _xFileToMagicFile(xFile) : null; diff --git a/test/routing/page_route_name_test.dart b/test/routing/page_route_name_test.dart index f0a9e4f..03a0785 100644 --- a/test/routing/page_route_name_test.dart +++ b/test/routing/page_route_name_test.dart @@ -100,6 +100,39 @@ void main() { 'A null name is what silently disables every screen-aware observer.', ); }); + + testWidgets('a page inside a layout is named too', (tester) async { + // Layouts become ShellRoutes, which introduce a nested Navigator. An + // observer registered on the root router does not automatically see + // pushes inside that shell, so this asserts the case an app with a + // sidebar or tab bar actually runs in. + MagicRoute.group( + layoutId: 'app', + layout: (child) => Column( + children: [ + const Text('shell'), + Expanded(child: child), + ], + ), + routes: () { + MagicRoute.page('/', () => const SizedBox()).name('home'); + }, + ); + + final observer = _NameRecordingObserver(); + MagicRouter.instance.addObserver(observer); + + await tester.pumpWidget( + MaterialApp.router(routerConfig: MagicRouter.instance.routerConfig), + ); + await tester.pumpAndSettle(); + + expect( + observer.names, + contains('home'), + reason: 'A shell must not hide its pages from a screen-aware observer.', + ); + }); }); }