[go_router] Fixes crashes when popping navigators manually (#2952)

* [go_router] Fixes crashes when popping navigators manually

* rename

* add additional test case

packages/go_router/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 6.0.1
+
+- Fixes crashes when popping navigators manually.
+- Fixes trailing slashes after pops.
+
 ## 6.0.0
 
 - **BREAKING CHANGE**

packages/go_router/lib/src/builder.dart

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'package:collection/collection.dart';
 import 'package:flutter/widgets.dart';
 
 import 'configuration.dart';
@@ -50,6 +51,18 @@ class RouteBuilder {
 
   final GoRouterStateRegistry _registry = GoRouterStateRegistry();
 
+  final Map<Page<Object?>, RouteMatch> _routeMatchLookUp =
+      <Page<Object?>, RouteMatch>{};
+
+  /// Looks the the [RouteMatch] for a given [Page].
+  ///
+  /// The [Page] must be in the latest [Navigator.pages]; otherwise, this method
+  /// returns null.
+  RouteMatch? getRouteMatchForPage(Page<Object?> page) =>
+      _routeMatchLookUp[page];
+
+  // final Map<>
+
   /// Builds the top-level Navigator for the given [RouteMatchList].
   Widget build(
     BuildContext context,
@@ -57,6 +70,7 @@ class RouteBuilder {
     PopPageCallback onPopPage,
     bool routerNeglect,
   ) {
+    _routeMatchLookUp.clear();
     if (matchList.isEmpty) {
       // The build method can be called before async redirect finishes. Build a
       // empty box until then.
@@ -119,10 +133,15 @@ class RouteBuilder {
       GlobalKey<NavigatorState> navigatorKey,
       Map<Page<Object?>, GoRouterState> registry) {
     try {
+      assert(_routeMatchLookUp.isEmpty);
       final Map<GlobalKey<NavigatorState>, List<Page<Object?>>> keyToPage =
           <GlobalKey<NavigatorState>, List<Page<Object?>>>{};
       _buildRecursive(context, matchList, 0, onPopPage, routerNeglect,
           keyToPage, navigatorKey, registry);
+
+      // Every Page should have a corresponding RouteMatch.
+      assert(keyToPage.values.flattened
+          .every((Page<Object?> page) => _routeMatchLookUp.containsKey(page)));
       return keyToPage[navigatorKey]!;
     } on _RouteBuilderError catch (e) {
       return <Page<Object?>>[
@@ -271,12 +290,13 @@ class RouteBuilder {
       page = null;
     }
 
+    page ??= buildPage(context, state, Builder(builder: (BuildContext context) {
+      return _callRouteBuilder(context, state, match, childWidget: child);
+    }));
+    _routeMatchLookUp[page] = match;
+
     // Return the result of the route's builder() or pageBuilder()
-    return page ??
+    return page;
-        // Uses a Builder to make sure its rebuild scope is limited to the page.
-        buildPage(context, state, Builder(builder: (BuildContext context) {
-          return _callRouteBuilder(context, state, match, childWidget: child);
-        }));
   }
 
   /// Calls the user-provided route builder from the [RouteMatch]'s [RouteBase].
@@ -363,7 +383,7 @@ class RouteBuilder {
     _cacheAppType(context);
     return _pageBuilderForAppType!(
       key: state.pageKey,
-      name: state.name ?? state.fullpath,
+      name: state.name ?? state.path,
       arguments: <String, String>{...state.params, ...state.queryParams},
       restorationId: state.pageKey.value,
       child: child,

packages/go_router/lib/src/delegate.dart

@@ -16,7 +16,7 @@ import 'typedefs.dart';
 
 /// GoRouter implementation of [RouterDelegate].
 class GoRouterDelegate extends RouterDelegate<RouteMatchList>
-    with PopNavigatorRouterDelegateMixin<RouteMatchList>, ChangeNotifier {
+    with ChangeNotifier {
   /// Constructor for GoRouter's implementation of the RouterDelegate base
   /// class.
   GoRouterDelegate({
@@ -132,7 +132,12 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
     if (!route.didPop(result)) {
       return false;
     }
-    _matchList.pop();
+    final Page<Object?> page = route.settings as Page<Object?>;
+    final RouteMatch? match = builder.getRouteMatchForPage(page);
+    if (match == null) {
+      return true;
+    }
+    _matchList.remove(match);
     notifyListeners();
     assert(() {
       _debugAssertMatchListNotEmpty();
@@ -146,7 +151,7 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
   /// See also:
   /// * [push] which pushes the given location onto the page stack.
   void pushReplacement(RouteMatchList matches) {
-    _matchList.pop();
+    _matchList.remove(_matchList.last);
     push(matches); // [push] will notify the listeners.
   }
 
@@ -155,7 +160,6 @@ class GoRouterDelegate extends RouterDelegate<RouteMatchList>
   RouteMatchList get matches => _matchList;
 
   /// For use by the Router architecture as part of the RouterDelegate.
-  @override
   GlobalKey<NavigatorState> get navigatorKey => _configuration.navigatorKey;
 
   /// For use by the Router architecture as part of the RouterDelegate.

packages/go_router/lib/src/match.dart

@@ -73,6 +73,6 @@ class RouteMatch {
   /// An exception if there was an error during matching.
   final Exception? error;
 
-  /// Optional value key of type string, to hold a unique reference to a page.
+  /// Value key of type string, to hold a unique reference to a page.
   final ValueKey<String> pageKey;
 }

packages/go_router/lib/src/matching.dart

@@ -5,6 +5,7 @@
 import 'package:flutter/widgets.dart';
 
 import 'configuration.dart';
+import 'delegate.dart';
 import 'match.dart';
 import 'path_utils.dart';
 
@@ -56,7 +57,7 @@ class RouteMatchList {
   static RouteMatchList empty =
       RouteMatchList(<RouteMatch>[], Uri.parse(''), const <String, String>{});
 
-  static String _generateFullPath(List<RouteMatch> matches) {
+  static String _generateFullPath(Iterable<RouteMatch> matches) {
     final StringBuffer buffer = StringBuffer();
     bool addsSlash = false;
     for (final RouteMatch match in matches) {
@@ -96,17 +97,27 @@ class RouteMatchList {
     _matches.add(match);
   }
 
-  /// Removes the last match.
+  /// Removes the match from the list.
-  void pop() {
+  void remove(RouteMatch match) {
-    if (_matches.last.route is GoRoute) {
+    final int index = _matches.indexOf(match);
-      final GoRoute route = _matches.last.route as GoRoute;
+    assert(index != -1);
-      _uri = _uri.replace(path: removePatternFromPath(route.path, _uri.path));
+    _matches.removeRange(index, _matches.length);
-    }
+
-    _matches.removeLast();
     // Also pop ShellRoutes when there are no subsequent route matches
     while (_matches.isNotEmpty && _matches.last.route is ShellRoute) {
       _matches.removeLast();
     }
+
+    final String fullPath = _generateFullPath(
+        _matches.where((RouteMatch match) => match is! ImperativeRouteMatch));
+    // Need to remove path parameters that are no longer in the fullPath.
+    final List<String> newParameters = <String>[];
+    patternToRegExp(fullPath, newParameters);
+    final Set<String> validParameters = newParameters.toSet();
+    pathParameters.removeWhere(
+        (String key, String value) => !validParameters.contains(key));
+
+    _uri = _uri.replace(path: patternToPath(fullPath, pathParameters));
   }
 
   /// An optional object provided by the app during navigation.

packages/go_router/lib/src/path_utils.dart

@@ -47,44 +47,6 @@ RegExp patternToRegExp(String pattern, List<String> parameters) {
   return RegExp(buffer.toString(), caseSensitive: false);
 }
 
-/// Removes string from the end of the path that matches a `pattern`.
-///
-/// The path parameters can be specified by prefixing them with `:`. The
-/// `parameters` are used for storing path parameter names.
-///
-///
-/// For example:
-///
-///  `path` = `/user/123/book/345`
-///  `pattern` = `book/:id`
-///
-/// The return value = `/user/123`.
-String removePatternFromPath(String pattern, String path) {
-  final StringBuffer buffer = StringBuffer();
-  int start = 0;
-  for (final RegExpMatch match in _parameterRegExp.allMatches(pattern)) {
-    if (match.start > start) {
-      buffer.write(RegExp.escape(pattern.substring(start, match.start)));
-    }
-    final String? optionalPattern = match[2];
-    final String regex =
-        optionalPattern != null ? _escapeGroup(optionalPattern) : '[^/]+';
-    buffer.write(regex);
-    start = match.end;
-  }
-
-  if (start < pattern.length) {
-    buffer.write(RegExp.escape(pattern.substring(start)));
-  }
-
-  if (!pattern.endsWith('/')) {
-    buffer.write(r'(?=/|$)');
-  }
-  buffer.write(r'$');
-  final RegExp regexp = RegExp(buffer.toString(), caseSensitive: false);
-  return path.replaceFirst(regexp, '');
-}
-
 String _escapeGroup(String group, [String? name]) {
   final String escapedGroup = group.replaceFirstMapped(
       RegExp(r'[:=!]'), (Match match) => '\\${match[0]}');

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: 6.0.0
+version: 6.0.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

@@ -953,6 +953,41 @@ void main() {
       ]);
     });
 
+    testWidgets('on pop twice', (WidgetTester tester) async {
+      final List<GoRoute> routes = <GoRoute>[
+        GoRoute(
+            path: '/',
+            builder: (_, __) => const DummyScreen(),
+            routes: <RouteBase>[
+              GoRoute(
+                  path: 'settings',
+                  builder: (_, __) => const DummyScreen(),
+                  routes: <RouteBase>[
+                    GoRoute(
+                      path: 'profile',
+                      builder: (_, __) => const DummyScreen(),
+                    ),
+                  ]),
+            ]),
+      ];
+
+      final GoRouter router = await createRouter(routes, tester,
+          initialLocation: '/settings/profile');
+
+      log.clear();
+      router.pop();
+      router.pop();
+      await tester.pumpAndSettle();
+      expect(log, <Object>[
+        isMethodCall('selectMultiEntryHistory', arguments: null),
+        isMethodCall('routeInformationUpdated', arguments: <String, dynamic>{
+          'location': '/',
+          'state': null,
+          'replace': false
+        }),
+      ]);
+    });
+
     testWidgets('on pop with path parameters', (WidgetTester tester) async {
       final List<GoRoute> routes = <GoRoute>[
         GoRoute(
@@ -1012,6 +1047,80 @@ void main() {
       ]);
     });
 
+    testWidgets('Can manually pop root navigator and display correct url',
+        (WidgetTester tester) async {
+      final GlobalKey<NavigatorState> rootNavigatorKey =
+          GlobalKey<NavigatorState>();
+
+      final List<RouteBase> routes = <RouteBase>[
+        GoRoute(
+          path: '/',
+          builder: (BuildContext context, GoRouterState state) {
+            return const Scaffold(
+              body: Text('Home'),
+            );
+          },
+          routes: <RouteBase>[
+            ShellRoute(
+              builder:
+                  (BuildContext context, GoRouterState state, Widget child) {
+                return Scaffold(
+                  appBar: AppBar(),
+                  body: child,
+                );
+              },
+              routes: <RouteBase>[
+                GoRoute(
+                  path: 'b',
+                  builder: (BuildContext context, GoRouterState state) {
+                    return const Scaffold(
+                      body: Text('Screen B'),
+                    );
+                  },
+                  routes: <RouteBase>[
+                    GoRoute(
+                      path: 'c',
+                      builder: (BuildContext context, GoRouterState state) {
+                        return const Scaffold(
+                          body: Text('Screen C'),
+                        );
+                      },
+                    ),
+                  ],
+                ),
+              ],
+            ),
+          ],
+        ),
+      ];
+
+      await createRouter(routes, tester,
+          initialLocation: '/b/c', navigatorKey: rootNavigatorKey);
+      expect(find.text('Screen C'), findsOneWidget);
+      expect(log, <Object>[
+        isMethodCall('selectMultiEntryHistory', arguments: null),
+        isMethodCall('routeInformationUpdated', arguments: <String, dynamic>{
+          'location': '/b/c',
+          'state': null,
+          'replace': false
+        }),
+      ]);
+
+      log.clear();
+      rootNavigatorKey.currentState!.pop();
+      await tester.pumpAndSettle();
+
+      expect(find.text('Home'), findsOneWidget);
+      expect(log, <Object>[
+        isMethodCall('selectMultiEntryHistory', arguments: null),
+        isMethodCall('routeInformationUpdated', arguments: <String, dynamic>{
+          'location': '/',
+          'state': null,
+          'replace': false
+        }),
+      ]);
+    });
+
     testWidgets('works correctly with async redirect',
         (WidgetTester tester) async {
       final UniqueKey login = UniqueKey();