[go_router] Fix key collisions when sibling ShellRoutes coexist during push

packages/go_router/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 17.1.1
+
+- Fixes `ShellRoute` key collisions when `push()` keeps multiple shell branches in
+  the widget tree at the same time.
+
 ## 17.1.0
 
 - Adds `TypedQueryParameter` annotation to override parameter names in `TypedGoRoute` constructors.

packages/go_router/lib/src/builder.dart

@@ -108,7 +108,7 @@ class RouteBuilder {
       context,
       _CustomNavigator(
         // The state needs to persist across rebuild.
-        key: GlobalObjectKey(configuration.navigatorKey.hashCode),
+        key: GlobalObjectKey(configuration.navigatorKey),
         navigatorKey: configuration.navigatorKey,
         observers: observers,
         navigatorRestorationId: restorationScopeId,
@@ -300,7 +300,7 @@ class _CustomNavigatorState extends State<_CustomNavigator> {
               canPop: match.matches.length == 1,
               child: _CustomNavigator(
                 // The state needs to persist across rebuild.
-                key: GlobalObjectKey(navigatorKey.hashCode),
+                key: GlobalObjectKey(navigatorKey),
                 navigatorRestorationId: restorationScopeId,
                 navigatorKey: navigatorKey,
                 matches: match.matches,

packages/go_router/lib/src/match.dart

@@ -175,14 +175,22 @@ abstract class RouteMatchBase with Diagnosticable {
     if (subRouteMatches?.isEmpty ?? true) {
       return _empty;
     }
+    final matchNavigatorKey = route is ShellRoute
+        ? GlobalKey<NavigatorState>(debugLabel: navigatorKeyUsed.toString())
+        : navigatorKeyUsed;
+    final pageKey = route is ShellRoute
+        ? ValueKey<String>(
+            '${route.hashCode}-${identityHashCode(matchNavigatorKey)}',
+          )
+        : ValueKey<String>(route.hashCode.toString());
     final RouteMatchBase result = ShellRouteMatch(
       route: route,
       // The RouteConfiguration should have asserted the subRouteMatches must
       // have at least one match for this ShellRouteBase.
       matches: subRouteMatches!.remove(null)!,
       matchedLocation: remainingLocation,
-      pageKey: ValueKey<String>(route.hashCode.toString()),
-      navigatorKey: navigatorKeyUsed,
+      pageKey: pageKey,
+      navigatorKey: matchNavigatorKey,
     );
     subRouteMatches
         .putIfAbsent(parentKey, () => <RouteMatchBase>[])

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.1.0
+version: 17.1.1
 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/go_router_test.dart

@@ -5579,7 +5579,7 @@ void main() {
                     return Scaffold(
                       body: TextButton(
                         onPressed: () async {
-                          shellNavigatorKey.currentState!.push(
+                          Navigator.of(context).push(
                             MaterialPageRoute<void>(
                               builder: (BuildContext context) {
                                 return const Scaffold(

packages/go_router/test/imperative_api_test.dart

@@ -192,6 +192,73 @@ void main() {
     expect(find.byKey(b), findsOneWidget);
   });
 
+  testWidgets(
+    'push to a sibling shell route under the same parent shell route',
+    (WidgetTester tester) async {
+      const firstNavigatorKey = _CollidingNavigatorKey('first');
+      const secondNavigatorKey = _CollidingNavigatorKey('second');
+      final routes = <RouteBase>[
+        ShellRoute(
+          builder: (_, __, Widget child) =>
+              _ShellScaffold(label: 'project-shell', child: child),
+          routes: <RouteBase>[
+            ShellRoute(
+              navigatorKey: firstNavigatorKey,
+              builder: (_, __, Widget child) =>
+                  _ShellScaffold(label: 'first-shell', child: child),
+              routes: <RouteBase>[
+                GoRoute(
+                  path: '/first',
+                  builder: (_, __) => const _CounterPage(label: 'first count'),
+                ),
+              ],
+            ),
+            ShellRoute(
+              navigatorKey: secondNavigatorKey,
+              builder: (_, __, Widget child) =>
+                  _ShellScaffold(label: 'second-shell', child: child),
+              routes: <RouteBase>[
+                GoRoute(
+                  path: '/second',
+                  builder: (_, __) => const Text('second page'),
+                ),
+              ],
+            ),
+          ],
+        ),
+      ];
+      final GoRouter router = await createRouter(
+        routes,
+        tester,
+        initialLocation: '/first',
+      );
+
+      expect(find.text('project-shell'), findsOneWidget);
+      expect(find.text('first-shell'), findsOneWidget);
+      expect(find.text('first count: 0'), findsOneWidget);
+
+      await tester.tap(find.text('first count: 0'));
+      await tester.pump();
+      expect(find.text('first count: 1'), findsOneWidget);
+
+      router.push('/second');
+      await tester.pumpAndSettle();
+      expect(tester.takeException(), isNull);
+      expect(find.text('project-shell'), findsOneWidget);
+      expect(find.text('first-shell'), findsNothing);
+      expect(find.text('second-shell'), findsOneWidget);
+      expect(find.text('second page'), findsOneWidget);
+
+      router.pop();
+      await tester.pumpAndSettle();
+      expect(tester.takeException(), isNull);
+      expect(find.text('project-shell'), findsOneWidget);
+      expect(find.text('second-shell'), findsNothing);
+      expect(find.text('first-shell'), findsOneWidget);
+      expect(find.text('first count: 1'), findsOneWidget);
+    },
+  );
+
   testWidgets('push inside or outside shell route', (
     WidgetTester tester,
   ) async {
@@ -317,3 +384,62 @@ void main() {
     expect(find.byKey(e), findsOneWidget);
   });
 }
+
+class _ShellScaffold extends StatelessWidget {
+  const _ShellScaffold({required this.label, required this.child});
+
+  final String label;
+  final Widget child;
+
+  @override
+  Widget build(BuildContext context) {
+    return Material(
+      child: Column(
+        children: <Widget>[
+          Text(label),
+          Expanded(child: child),
+        ],
+      ),
+    );
+  }
+}
+
+class _CounterPage extends StatefulWidget {
+  const _CounterPage({required this.label});
+
+  final String label;
+
+  @override
+  State<_CounterPage> createState() => _CounterPageState();
+}
+
+class _CounterPageState extends State<_CounterPage> {
+  int _count = 0;
+
+  @override
+  Widget build(BuildContext context) {
+    return Center(
+      child: TextButton(
+        onPressed: () {
+          setState(() {
+            _count++;
+          });
+        },
+        child: Text('${widget.label}: $_count'),
+      ),
+    );
+  }
+}
+
+class _CollidingNavigatorKey extends GlobalKey<NavigatorState> {
+  const _CollidingNavigatorKey(this._label) : super.constructor();
+
+  final String _label;
+
+  @override
+  int get hashCode => 0;
+
+  @override
+  bool operator ==(Object other) =>
+      other is _CollidingNavigatorKey && other._label == _label;
+}

packages/go_router/test/match_test.dart

@@ -43,27 +43,35 @@ void main() {
       expect(matches.first.pageKey, isNotNull);
     });
 
-    test('ShellRoute Match has stable unique key', () {
+    test('ShellRoute Match has unique keys and preserves them on copy', () {
       final route = ShellRoute(
         builder: _shellBuilder,
         routes: <GoRoute>[GoRoute(path: '/users/:userId', builder: _builder)],
       );
-      final pathParameters = <String, String>{};
       final List<RouteMatchBase> matches1 = RouteMatchBase.match(
         route: route,
-        pathParameters: pathParameters,
+        pathParameters: <String, String>{},
         uri: Uri.parse('/users/123'),
         rootNavigatorKey: GlobalKey<NavigatorState>(),
       );
       final List<RouteMatchBase> matches2 = RouteMatchBase.match(
         route: route,
-        pathParameters: pathParameters,
+        pathParameters: <String, String>{},
         uri: Uri.parse('/users/1234'),
         rootNavigatorKey: GlobalKey<NavigatorState>(),
       );
       expect(matches1.length, 1);
       expect(matches2.length, 1);
-      expect(matches1.first.pageKey, matches2.first.pageKey);
+      final match1 = matches1.first as ShellRouteMatch;
+      final match2 = matches2.first as ShellRouteMatch;
+      expect(match1.pageKey, isNot(equals(match2.pageKey)));
+      expect(match1.navigatorKey, isNot(same(match2.navigatorKey)));
+
+      final ShellRouteMatch copiedMatch = match1.copyWith(
+        matches: match1.matches,
+      );
+      expect(copiedMatch.pageKey, match1.pageKey);
+      expect(copiedMatch.navigatorKey, same(match1.navigatorKey));
     });
 
     test('GoRoute Match has stable unique key', () {