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..f35557e57c35 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,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 = {}; + final usedNavigatorKeys = >{}; + _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 matches, + Set usedPageKeys, + Set> 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>> + _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 = [ + _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 pageKey = branch.pageKey; + if (!usedPageKeys.add(pageKey.value)) { + pageKey = ValueKey( + '${match.pageKey.value}${branch.pageKey.value}', + ); + usedPageKeys.add(pageKey.value); + } + GlobalKey navigatorKey = branch.navigatorKey; + if (!usedNavigatorKeys.add(navigatorKey)) { + navigatorKey = _syntheticShellNavigatorKey( + match.pageKey, + branch.pageKey.value, + ); + usedNavigatorKeys.add(navigatorKey); + } return branch.copyWith( - matches: [ - _cloneBranchAndInsertImperativeMatch(branch.matches.last, match), - ], + matches: matches, + pageKey: pageKey, + navigatorKey: navigatorKey, ); } // Add the input `match` instead of the incompatibleMatch since it contains 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..fdc4e30ed70a 100644 --- a/packages/go_router/test/imperative_api_test.dart +++ b/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(); + 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); + }); + + 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); + }); }