fix(routing): name every page so navigator observers can tell screens apart

CHANGELOG.md

@@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file.
 
 ## [Unreleased]
 
+### 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
 
 - **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: <null>` 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`)

doc/basics/routing.md

@@ -481,6 +481,26 @@ 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.
+
+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]

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;

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,
+        );
     }
   }
 

test/routing/page_route_name_test.dart

@@ -0,0 +1,155 @@
+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.',
+      );
+    });
+
+    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.',
+      );
+    });
+  });
+}
+
+/// Records the name of every route pushed or replaced, exactly as a
+/// screen-aware observer reads it.
+class _NameRecordingObserver extends NavigatorObserver {
+  final List<String?> names = <String?>[];
+
+  @override
+  void didPush(Route<dynamic> route, Route<dynamic>? previousRoute) {
+    names.add(route.settings.name);
+  }
+
+  @override
+  void didReplace({Route<dynamic>? newRoute, Route<dynamic>? oldRoute}) {
+    if (newRoute != null) {
+      names.add(newRoute.settings.name);
+    }
+  }
+}