[go_router] Fix duplicate page key when re-entering a shell route

packages/go_router/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 17.2.4
+
+- Fixes a duplicate page key assertion when imperatively re-entering a shell route that is still in the navigation stack (e.g. pushing into a shell after a `pushReplacement`).
+
 ## 17.2.3
 
 - Fixes an assertion failure when navigating to URLs with hash fragments missing a leading slash.

packages/go_router/lib/src/match.dart

@@ -424,13 +424,17 @@ class ShellRouteMatch extends RouteMatchBase {
   // TODO(loic-sharma): Remove meta library prefix.
   // https://github.com/flutter/flutter/issues/171410
   @meta.internal
-  ShellRouteMatch copyWith({required List<RouteMatchBase>? matches}) {
+  ShellRouteMatch copyWith({
+    List<RouteMatchBase>? matches,
+    ValueKey<String>? pageKey,
+    GlobalKey<NavigatorState>? navigatorKey,
+  }) {
     return ShellRouteMatch(
       matches: matches ?? this.matches,
       route: route,
       matchedLocation: matchedLocation,
-      pageKey: pageKey,
-      navigatorKey: navigatorKey,
+      pageKey: pageKey ?? this.pageKey,
+      navigatorKey: navigatorKey ?? this.navigatorKey,
     );
   }
 
@@ -654,21 +658,108 @@ class RouteMatchList with Diagnosticable {
       );
       return newMatches;
     }
+    // Collect the page keys and shell navigator keys already in use so that
+    // re-entering a shell route that is still in the stack does not reuse them.
+    final usedPageKeys = <String>{};
+    final usedNavigatorKeys = <GlobalKey<NavigatorState>>{};
+    _collectUsedKeys(newMatches, usedPageKeys, usedNavigatorKeys);
     newMatches.add(
-      _cloneBranchAndInsertImperativeMatch(otherMatches.last, match),
+      _cloneBranchAndInsertImperativeMatch(
+        otherMatches.last,
+        match,
+        usedPageKeys,
+        usedNavigatorKeys,
+      ),
     );
     return newMatches;
   }
 
+  /// Recursively collects the page keys and shell navigator keys already in use
+  /// by `matches`.
+  static void _collectUsedKeys(
+    List<RouteMatchBase> matches,
+    Set<String> usedPageKeys,
+    Set<GlobalKey<NavigatorState>> usedNavigatorKeys,
+  ) {
+    for (final match in matches) {
+      usedPageKeys.add(match.pageKey.value);
+      if (match is ShellRouteMatch) {
+        usedNavigatorKeys.add(match.navigatorKey);
+        _collectUsedKeys(match.matches, usedPageKeys, usedNavigatorKeys);
+      }
+    }
+  }
+
+  /// Weak, per-imperative-push cache of synthesized navigator keys for shell
+  /// routes that are re-entered while a previous instance is still in the stack.
+  ///
+  /// Keyed on the imperative match's page key, which is reused by reference
+  /// across reparses, so the same logical duplicate shell keeps a stable
+  /// navigator key (and therefore its navigator state). Entries disappear once
+  /// the imperative match is removed and its page key is garbage collected.
+  static final Expando<Map<String, GlobalKey<NavigatorState>>>
+  _syntheticShellNavigatorKeys =
+      Expando<Map<String, GlobalKey<NavigatorState>>>();
+
+  static GlobalKey<NavigatorState> _syntheticShellNavigatorKey(
+    ValueKey<String> imperativeKey,
+    String discriminator,
+  ) {
+    final Map<String, GlobalKey<NavigatorState>> keys =
+        _syntheticShellNavigatorKeys[imperativeKey] ??=
+            <String, GlobalKey<NavigatorState>>{};
+    return keys.putIfAbsent(discriminator, () => GlobalKey<NavigatorState>());
+  }
+
   static RouteMatchBase _cloneBranchAndInsertImperativeMatch(
     RouteMatchBase branch,
     ImperativeRouteMatch match,
+    Set<String> usedPageKeys,
+    Set<GlobalKey<NavigatorState>> usedNavigatorKeys,
   ) {
     if (branch is ShellRouteMatch) {
+      final matches = <RouteMatchBase>[
+        _cloneBranchAndInsertImperativeMatch(
+          branch.matches.last,
+          match,
+          usedPageKeys,
+          usedNavigatorKeys,
+        ),
+      ];
+      // A [ShellRouteMatch] derives both its page key and its navigator key from
+      // its route. Re-entering the same shell route while a previous instance is
+      // still in the stack would therefore produce two pages and two navigators
+      // sharing the same keys, tripping the framework's duplicate page key (and
+      // duplicate [GlobalKey]) assertions.
+      //
+      // The page key and the navigator key are handled independently since
+      // either can collide on its own. A page key only needs a distinct value.
+      // A navigator key is compared with [identical], so its replacement must
+      // also be stable across reparses; it is cached per imperative push (keyed
+      // on the reparse-stable imperative page key) and kept distinct per shell
+      // via the original page key value. The first instance keeps the route's
+      // original keys, so an explicitly provided navigatorKey still resolves to
+      // the bottom-most shell.
+      // Regression test for https://github.com/flutter/flutter/issues/140586.
+      ValueKey<String> pageKey = branch.pageKey;
+      if (!usedPageKeys.add(pageKey.value)) {
+        pageKey = ValueKey<String>(
+          '${match.pageKey.value}${branch.pageKey.value}',
+        );
+        usedPageKeys.add(pageKey.value);
+      }
+      GlobalKey<NavigatorState> navigatorKey = branch.navigatorKey;
+      if (!usedNavigatorKeys.add(navigatorKey)) {
+        navigatorKey = _syntheticShellNavigatorKey(
+          match.pageKey,
+          branch.pageKey.value,
+        );
+        usedNavigatorKeys.add(navigatorKey);
+      }
       return branch.copyWith(
-        matches: <RouteMatchBase>[
-          _cloneBranchAndInsertImperativeMatch(branch.matches.last, match),
-        ],
+        matches: matches,
+        pageKey: pageKey,
+        navigatorKey: navigatorKey,
       );
     }
     // Add the input `match` instead of the incompatibleMatch since it contains

packages/go_router/pubspec.yaml

@@ -1,7 +1,7 @@
 name: go_router
 description: A declarative router for Flutter based on Navigation 2 supporting
   deep linking, data-driven routes and more
-version: 17.2.3
+version: 17.2.4
 repository: https://github.com/flutter/packages/tree/main/packages/go_router
 issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+go_router%22
 

packages/go_router/test/imperative_api_test.dart

@@ -316,4 +316,163 @@ void main() {
     expect(find.text('shell'), findsNothing);
     expect(find.byKey(e), findsOneWidget);
   });
+
+  testWidgets('re-enter a shell route still in the stack', (
+    WidgetTester tester,
+  ) async {
+    // Regression test for https://github.com/flutter/flutter/issues/140586.
+    final shellKey = GlobalKey<NavigatorState>();
+    final a = UniqueKey();
+    final b = UniqueKey();
+    final c = UniqueKey();
+    final routes = <RouteBase>[
+      GoRoute(
+        path: '/a',
+        builder: (_, __) => DummyScreen(key: a),
+      ),
+      ShellRoute(
+        navigatorKey: shellKey,
+        builder: (_, __, Widget child) => child,
+        routes: <RouteBase>[
+          GoRoute(
+            path: '/b',
+            builder: (_, __) => DummyScreen(key: b),
+          ),
+          GoRoute(
+            path: '/c',
+            builder: (_, __) => DummyScreen(key: c),
+          ),
+        ],
+      ),
+    ];
+    final GoRouter router = await createRouter(
+      routes,
+      tester,
+      initialLocation: '/a',
+    );
+
+    router.push('/b');
+    await tester.pumpAndSettle();
+    router.push('/c');
+    await tester.pumpAndSettle();
+    router.pushReplacement('/a');
+    await tester.pumpAndSettle();
+
+    // Re-entering the shell route while a previous instance is still in the
+    // stack used to throw a duplicate page key assertion.
+    router.push('/b');
+    await tester.pumpAndSettle();
+    expect(tester.takeException(), isNull);
+    expect(find.byKey(b), findsOneWidget);
+
+    // Popping the re-entered shell restores the replaced top-level page.
+    router.pop();
+    await tester.pumpAndSettle();
+    expect(tester.takeException(), isNull);
+    expect(find.byKey(a), findsOneWidget);
+  });
+
+  testWidgets('re-enter a nested shell route still in the stack', (
+    WidgetTester tester,
+  ) async {
+    // Regression test for https://github.com/flutter/flutter/issues/140586.
+    // Variant with a nested shell and auto-generated navigator keys.
+    final a = UniqueKey();
+    final b = UniqueKey();
+    final c = UniqueKey();
+    final routes = <RouteBase>[
+      GoRoute(
+        path: '/a',
+        builder: (_, __) => DummyScreen(key: a),
+        routes: <RouteBase>[
+          ShellRoute(
+            builder: (_, __, Widget child) => child,
+            routes: <RouteBase>[
+              GoRoute(
+                path: 'b',
+                builder: (_, __) => DummyScreen(key: b),
+              ),
+            ],
+          ),
+        ],
+      ),
+      GoRoute(
+        path: '/c',
+        builder: (_, __) => DummyScreen(key: c),
+      ),
+    ];
+    final GoRouter router = await createRouter(
+      routes,
+      tester,
+      initialLocation: '/a',
+    );
+
+    router.push('/a/b');
+    await tester.pumpAndSettle();
+    router.push('/c');
+    await tester.pumpAndSettle();
+    router.push('/a/b');
+    await tester.pumpAndSettle();
+    expect(tester.takeException(), isNull);
+    expect(find.byKey(b), findsOneWidget);
+  });
+
+  testWidgets('re-enter nested shell routes still in the stack', (
+    WidgetTester tester,
+  ) async {
+    // Regression test for https://github.com/flutter/flutter/issues/140586.
+    // Two nested shell routes are both duplicated when re-entered; each cloned
+    // shell must get its own distinct navigator key.
+    final outerKey = GlobalKey<NavigatorState>();
+    final innerKey = GlobalKey<NavigatorState>();
+    final a = UniqueKey();
+    final b = UniqueKey();
+    final c = UniqueKey();
+    final routes = <RouteBase>[
+      GoRoute(
+        path: '/a',
+        builder: (_, __) => DummyScreen(key: a),
+      ),
+      ShellRoute(
+        navigatorKey: outerKey,
+        builder: (_, __, Widget child) => child,
+        routes: <RouteBase>[
+          ShellRoute(
+            navigatorKey: innerKey,
+            builder: (_, __, Widget child) => child,
+            routes: <RouteBase>[
+              GoRoute(
+                path: '/b',
+                builder: (_, __) => DummyScreen(key: b),
+              ),
+              GoRoute(
+                path: '/c',
+                builder: (_, __) => DummyScreen(key: c),
+              ),
+            ],
+          ),
+        ],
+      ),
+    ];
+    final GoRouter router = await createRouter(
+      routes,
+      tester,
+      initialLocation: '/a',
+    );
+
+    router.push('/b');
+    await tester.pumpAndSettle();
+    router.push('/c');
+    await tester.pumpAndSettle();
+    router.pushReplacement('/a');
+    await tester.pumpAndSettle();
+    router.push('/b');
+    await tester.pumpAndSettle();
+    expect(tester.takeException(), isNull);
+    expect(find.byKey(b), findsOneWidget);
+
+    // The original navigator keys still resolve to the bottom-most instances.
+    expect(outerKey.currentState, isNotNull);
+    expect(innerKey.currentState, isNotNull);
+  });
 }