From c36eb12f0aacc62e533bd1db2e9f9b09fe46f02e Mon Sep 17 00:00:00 2001 From: "berlis.b" Date: Sat, 30 May 2026 08:29:11 +0100 Subject: [PATCH 1/2] [go_router] Fix duplicate page key when re-entering a shell route When a route inside a `ShellRoute` is imperatively pushed while a previous instance of that same shell route is still in the navigation stack (for example pushing into a shell after a `pushReplacement`), go_router cloned the shell route match while reusing the shell's page key and navigator key. The two shell pages and navigators then shared the same keys, tripping the framework's `!keyReservation.contains(key)` duplicate page key assertion (and, in turn, a duplicate `GlobalKey`). The cloned shell instance now receives distinct page and navigator keys derived from the imperative match's stable page key, so they remain consistent across reparses. Shell routes reached via `go` or appearing only once keep their natural route-derived keys, leaving `parentNavigatorKey` and explicit `navigatorKey` behavior unchanged. Fixes flutter/flutter#140586 --- packages/go_router/CHANGELOG.md | 4 + packages/go_router/lib/src/match.dart | 67 ++++++++++-- packages/go_router/pubspec.yaml | 2 +- .../go_router/test/imperative_api_test.dart | 100 ++++++++++++++++++ 4 files changed, 163 insertions(+), 10 deletions(-) diff --git a/packages/go_router/CHANGELOG.md b/packages/go_router/CHANGELOG.md index 9aad45c2da0c..33002acae060 100644 --- a/packages/go_router/CHANGELOG.md +++ b/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. diff --git a/packages/go_router/lib/src/match.dart b/packages/go_router/lib/src/match.dart index 87f588a3eeef..d58781c4411e 100644 --- a/packages/go_router/lib/src/match.dart +++ b/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? matches}) { + ShellRouteMatch copyWith({ + List? matches, + ValueKey? pageKey, + GlobalKey? navigatorKey, + }) { return ShellRouteMatch( matches: matches ?? this.matches, route: route, matchedLocation: matchedLocation, - pageKey: pageKey, - navigatorKey: navigatorKey, + pageKey: pageKey ?? this.pageKey, + navigatorKey: navigatorKey ?? this.navigatorKey, ); } @@ -654,22 +658,67 @@ class RouteMatchList with Diagnosticable { ); return newMatches; } + // Collect the page keys already in use so that re-entering a shell route + // that is still in the stack does not reuse its page and navigator keys. + final usedPageKeys = {}; + _collectPageKeys(newMatches, usedPageKeys); newMatches.add( - _cloneBranchAndInsertImperativeMatch(otherMatches.last, match), + _cloneBranchAndInsertImperativeMatch( + otherMatches.last, + match, + usedPageKeys, + ), ); return newMatches; } + /// Recursively collects the [RouteMatchBase.pageKey] values of `matches`. + static void _collectPageKeys( + List matches, + Set result, + ) { + for (final match in matches) { + result.add(match.pageKey.value); + if (match is ShellRouteMatch) { + _collectPageKeys(match.matches, result); + } + } + } + static RouteMatchBase _cloneBranchAndInsertImperativeMatch( RouteMatchBase branch, ImperativeRouteMatch match, + Set usedPageKeys, ) { if (branch is ShellRouteMatch) { - return branch.copyWith( - matches: [ - _cloneBranchAndInsertImperativeMatch(branch.matches.last, match), - ], - ); + final matches = [ + _cloneBranchAndInsertImperativeMatch( + branch.matches.last, + match, + usedPageKeys, + ), + ]; + // A [ShellRouteMatch] derives its page key and navigator key from its + // route, so re-entering the same shell route while a previous instance is + // still in the stack would produce two pages and two navigators sharing + // the same keys. That trips the framework's duplicate page key assertion + // (and, in turn, a duplicate [GlobalKey]). Give the new instance distinct + // keys derived from the imperative match's (stable) page key so they stay + // consistent across reparses. + // Regression test for https://github.com/flutter/flutter/issues/140586. + if (usedPageKeys.contains(branch.pageKey.value)) { + final pageKey = ValueKey( + '${match.pageKey.value}${branch.pageKey.value}', + ); + usedPageKeys.add(pageKey.value); + return branch.copyWith( + matches: matches, + pageKey: pageKey, + navigatorKey: GlobalObjectKey(match.pageKey), + ); + } + usedPageKeys.add(branch.pageKey.value); + return branch.copyWith(matches: matches); } // Add the input `match` instead of the incompatibleMatch since it contains // page key and push future. diff --git a/packages/go_router/pubspec.yaml b/packages/go_router/pubspec.yaml index d4da05afae9f..306fe24daf0e 100644 --- a/packages/go_router/pubspec.yaml +++ b/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 diff --git a/packages/go_router/test/imperative_api_test.dart b/packages/go_router/test/imperative_api_test.dart index b56d0eefa565..ed0c9de04926 100644 --- a/packages/go_router/test/imperative_api_test.dart +++ b/packages/go_router/test/imperative_api_test.dart @@ -316,4 +316,104 @@ 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(); + final a = UniqueKey(); + final b = UniqueKey(); + final c = UniqueKey(); + final routes = [ + GoRoute( + path: '/a', + builder: (_, __) => DummyScreen(key: a), + ), + ShellRoute( + navigatorKey: shellKey, + builder: (_, __, Widget child) => child, + routes: [ + 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 = [ + GoRoute( + path: '/a', + builder: (_, __) => DummyScreen(key: a), + routes: [ + ShellRoute( + builder: (_, __, Widget child) => child, + routes: [ + 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); + }); } From 03234cdc8a9e2609f5e76ba6015a0bae716c8f8b Mon Sep 17 00:00:00 2001 From: "Berlis B." Date: Tue, 2 Jun 2026 23:52:42 +0100 Subject: [PATCH 2/2] [go_router] Address review: stable unique keys for duplicated nested shells - Handle page key and navigator key collisions independently, since either can collide on its own. - Give a duplicated shell instance a navigator key that is both unique and stable across reparses. GlobalObjectKey compares by identity, so reusing the imperative match's page key collided across nested duplicated shells, while a freshly built key would change every reparse and drop navigator state. Synthetic navigator keys are now cached per imperative push (weak Expando keyed on the reparse-stable imperative page key) and kept distinct per shell. - The first/bottom-most instance keeps the route's original keys, so an explicitly provided navigatorKey still resolves to it. - Add a regression test for re-entering nested shell routes. --- packages/go_router/lib/src/match.dart | 88 ++++++++++++++----- .../go_router/test/imperative_api_test.dart | 59 +++++++++++++ 2 files changed, 124 insertions(+), 23 deletions(-) diff --git a/packages/go_router/lib/src/match.dart b/packages/go_router/lib/src/match.dart index d58781c4411e..f35557e57c35 100644 --- a/packages/go_router/lib/src/match.dart +++ b/packages/go_router/lib/src/match.dart @@ -658,37 +658,64 @@ class RouteMatchList with Diagnosticable { ); return newMatches; } - // Collect the page keys already in use so that re-entering a shell route - // that is still in the stack does not reuse its page and navigator keys. + // 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 = {}; - _collectPageKeys(newMatches, usedPageKeys); + final usedNavigatorKeys = >{}; + _collectUsedKeys(newMatches, usedPageKeys, usedNavigatorKeys); newMatches.add( _cloneBranchAndInsertImperativeMatch( otherMatches.last, match, usedPageKeys, + usedNavigatorKeys, ), ); return newMatches; } - /// Recursively collects the [RouteMatchBase.pageKey] values of `matches`. - static void _collectPageKeys( + /// Recursively collects the page keys and shell navigator keys already in use + /// by `matches`. + static void _collectUsedKeys( List matches, - Set result, + Set usedPageKeys, + Set> usedNavigatorKeys, ) { for (final match in matches) { - result.add(match.pageKey.value); + usedPageKeys.add(match.pageKey.value); if (match is ShellRouteMatch) { - _collectPageKeys(match.matches, result); + 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>> + _syntheticShellNavigatorKeys = + Expando>>(); + + static GlobalKey _syntheticShellNavigatorKey( + ValueKey imperativeKey, + String discriminator, + ) { + final Map> keys = + _syntheticShellNavigatorKeys[imperativeKey] ??= + >{}; + return keys.putIfAbsent(discriminator, () => GlobalKey()); + } + static RouteMatchBase _cloneBranchAndInsertImperativeMatch( RouteMatchBase branch, ImperativeRouteMatch match, Set usedPageKeys, + Set> usedNavigatorKeys, ) { if (branch is ShellRouteMatch) { final matches = [ @@ -696,29 +723,44 @@ class RouteMatchList with Diagnosticable { branch.matches.last, match, usedPageKeys, + usedNavigatorKeys, ), ]; - // A [ShellRouteMatch] derives its page key and navigator key from its - // route, so re-entering the same shell route while a previous instance is - // still in the stack would produce two pages and two navigators sharing - // the same keys. That trips the framework's duplicate page key assertion - // (and, in turn, a duplicate [GlobalKey]). Give the new instance distinct - // keys derived from the imperative match's (stable) page key so they stay - // consistent across reparses. + // 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. - if (usedPageKeys.contains(branch.pageKey.value)) { - final pageKey = ValueKey( + ValueKey pageKey = branch.pageKey; + if (!usedPageKeys.add(pageKey.value)) { + pageKey = ValueKey( '${match.pageKey.value}${branch.pageKey.value}', ); usedPageKeys.add(pageKey.value); - return branch.copyWith( - matches: matches, - pageKey: pageKey, - navigatorKey: GlobalObjectKey(match.pageKey), + } + GlobalKey navigatorKey = branch.navigatorKey; + if (!usedNavigatorKeys.add(navigatorKey)) { + navigatorKey = _syntheticShellNavigatorKey( + match.pageKey, + branch.pageKey.value, ); + usedNavigatorKeys.add(navigatorKey); } - usedPageKeys.add(branch.pageKey.value); - return branch.copyWith(matches: matches); + return branch.copyWith( + matches: matches, + pageKey: pageKey, + navigatorKey: navigatorKey, + ); } // Add the input `match` instead of the incompatibleMatch since it contains // page key and push future. diff --git a/packages/go_router/test/imperative_api_test.dart b/packages/go_router/test/imperative_api_test.dart index ed0c9de04926..fdc4e30ed70a 100644 --- a/packages/go_router/test/imperative_api_test.dart +++ b/packages/go_router/test/imperative_api_test.dart @@ -416,4 +416,63 @@ void main() { 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(); + final innerKey = GlobalKey(); + final a = UniqueKey(); + final b = UniqueKey(); + final c = UniqueKey(); + final routes = [ + GoRoute( + path: '/a', + builder: (_, __) => DummyScreen(key: a), + ), + ShellRoute( + navigatorKey: outerKey, + builder: (_, __, Widget child) => child, + routes: [ + ShellRoute( + navigatorKey: innerKey, + builder: (_, __, Widget child) => child, + routes: [ + 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); + }); }