From 56902b0469b89ef194e0bb70ca491b0e77bbab6f Mon Sep 17 00:00:00 2001 From: David Miguel Date: Tue, 24 Feb 2026 17:15:18 +0100 Subject: [PATCH 1/3] [go_router] Fix chained top-level redirects not being fully resolved Fixes https://github.com/flutter/flutter/issues/178984 --- packages/go_router/lib/src/configuration.dart | 32 +- .../fix_chained_redirect_regression.yaml | 4 + packages/go_router/test/on_enter_test.dart | 171 +++++ .../go_router/test/redirect_chain_test.dart | 630 ++++++++++++++++++ 4 files changed, 832 insertions(+), 5 deletions(-) create mode 100644 packages/go_router/pending_changelogs/fix_chained_redirect_regression.yaml create mode 100644 packages/go_router/test/redirect_chain_test.dart diff --git a/packages/go_router/lib/src/configuration.dart b/packages/go_router/lib/src/configuration.dart index ef408b442708..f59abd498aec 100644 --- a/packages/go_router/lib/src/configuration.dart +++ b/packages/go_router/lib/src/configuration.dart @@ -427,7 +427,20 @@ class RouteConfiguration { if (newMatch.isError) { return newMatch; } - return redirect(context, newMatch, redirectHistory: redirectHistory); + // Re-evaluate top-level redirect on the new location before + // recursing into route-level redirects. This ensures that + // route-level redirects landing on a new path also trigger + // top-level redirect evaluation for that path. + final FutureOr afterTopLevel = applyTopLegacyRedirect( + context, + newMatch, + redirectHistory: redirectHistory, + ); + return redirect( + context, + afterTopLevel, + redirectHistory: redirectHistory, + ); } return prevMatchList; } @@ -484,9 +497,10 @@ class RouteConfiguration { /// /// Shares [redirectHistory] with later route-level redirects for proper loop detection. /// - /// Note: Legacy top-level redirect is executed at most once per navigation, - /// before route-level redirects. It does not re-evaluate if it redirects to - /// a location that would itself trigger another top-level redirect. + /// Recursively re-evaluates the top-level redirect when it produces a new + /// location, so that top-level redirect chains (e.g. `/` → `/a` → `/b`) are + /// fully resolved. Loop detection and redirect limit are enforced via the + /// shared [redirectHistory]. FutureOr applyTopLegacyRedirect( BuildContext context, RouteMatchList prevMatchList, { @@ -500,7 +514,15 @@ class RouteConfiguration { prevMatchList.uri, redirectHistory, ); - return newMatch; + if (newMatch.isError) { + return newMatch; + } + // Recursively re-evaluate the top-level redirect on the new location. + return applyTopLegacyRedirect( + context, + newMatch, + redirectHistory: redirectHistory, + ); } return prevMatchList; } diff --git a/packages/go_router/pending_changelogs/fix_chained_redirect_regression.yaml b/packages/go_router/pending_changelogs/fix_chained_redirect_regression.yaml new file mode 100644 index 000000000000..4804674f05eb --- /dev/null +++ b/packages/go_router/pending_changelogs/fix_chained_redirect_regression.yaml @@ -0,0 +1,4 @@ +changelog: | + - Fixes chained top-level redirects not being fully resolved (e.g. `/ → /a → /b` stopping at `/a`). + - Fixes route-level redirects not triggering top-level redirect re-evaluation on the new location. +version: patch diff --git a/packages/go_router/test/on_enter_test.dart b/packages/go_router/test/on_enter_test.dart index c9408ea4183d..2fb09cc051f3 100644 --- a/packages/go_router/test/on_enter_test.dart +++ b/packages/go_router/test/on_enter_test.dart @@ -1848,5 +1848,176 @@ void main() { }, ); }); + + // Tests for onEnter interaction with chained redirects. + // These validate that onEnter works correctly when top-level and + // route-level redirects produce chains. + group('onEnter with chained redirects', () { + testWidgets('onEnter called once when top-level redirect chains', ( + WidgetTester tester, + ) async { + // Regression test for https://github.com/flutter/flutter/issues/178984 + // + // Top-level redirect: / -> /a -> /b + // onEnter should allow navigation and be called exactly once + // for the final resolved location. + var onEnterCallCount = 0; + var redirectCallCount = 0; + + router = GoRouter( + initialLocation: '/', + onEnter: + ( + BuildContext context, + GoRouterState current, + GoRouterState next, + GoRouter goRouter, + ) async { + onEnterCallCount++; + return const Allow(); + }, + redirect: (BuildContext context, GoRouterState state) { + redirectCallCount++; + if (state.matchedLocation == '/') { + return '/a'; + } + if (state.matchedLocation == '/a') { + return '/b'; + } + return null; + }, + routes: [ + GoRoute(path: '/', builder: (_, __) => const Text('Home')), + GoRoute(path: '/a', builder: (_, __) => const Text('A')), + GoRoute(path: '/b', builder: (_, __) => const Text('B')), + ], + ); + + await tester.pumpWidget(MaterialApp.router(routerConfig: router)); + await tester.pumpAndSettle(); + + // Chain should resolve to /b. + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/b'); + expect(find.text('B'), findsOneWidget); + // onEnter should be called exactly once for the initial navigation. + expect(onEnterCallCount, 1); + // Redirect should be called for /, /a, and /b. + expect(redirectCallCount, 3); + }); + + testWidgets( + 'onEnter called once when route-level triggers top-level redirect', + (WidgetTester tester) async { + // Route-level on /src: /src -> /dst + // Top-level: /dst -> /final + // onEnter should be called exactly once. + var onEnterCallCount = 0; + + router = GoRouter( + initialLocation: '/src', + onEnter: + ( + BuildContext context, + GoRouterState current, + GoRouterState next, + GoRouter goRouter, + ) async { + onEnterCallCount++; + return const Allow(); + }, + redirect: (BuildContext context, GoRouterState state) { + if (state.matchedLocation == '/dst') { + return '/final'; + } + return null; + }, + routes: [ + GoRoute( + path: '/', + builder: (_, __) => const Text('Home'), + routes: [ + GoRoute( + path: 'src', + builder: (_, __) => const Text('Src'), + redirect: (BuildContext context, GoRouterState state) => + '/dst', + ), + ], + ), + GoRoute(path: '/dst', builder: (_, __) => const Text('Dst')), + GoRoute(path: '/final', builder: (_, __) => const Text('Final')), + ], + ); + + await tester.pumpWidget(MaterialApp.router(routerConfig: router)); + await tester.pumpAndSettle(); + + // Chain should resolve to /final. + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/final', + ); + expect(find.text('Final'), findsOneWidget); + // onEnter should be called exactly once for the initial navigation. + expect(onEnterCallCount, 1); + }, + ); + + testWidgets('onEnter block prevents redirect chain evaluation', ( + WidgetTester tester, + ) async { + // onEnter blocks navigation to /a. + // Top-level redirect: /a -> /b (should never be evaluated). + var redirectCallCount = 0; + + router = GoRouter( + initialLocation: '/', + onEnter: + ( + BuildContext context, + GoRouterState current, + GoRouterState next, + GoRouter goRouter, + ) async { + // Block all navigation except to /. + if (next.uri.path == '/') { + return const Allow(); + } + return const Block.stop(); + }, + redirect: (BuildContext context, GoRouterState state) { + redirectCallCount++; + if (state.matchedLocation == '/a') { + return '/b'; + } + return null; + }, + routes: [ + GoRoute(path: '/', builder: (_, __) => const Text('Home')), + GoRoute(path: '/a', builder: (_, __) => const Text('A')), + GoRoute(path: '/b', builder: (_, __) => const Text('B')), + ], + ); + + await tester.pumpWidget(MaterialApp.router(routerConfig: router)); + await tester.pumpAndSettle(); + + // Initial navigation to / is allowed. + expect(find.text('Home'), findsOneWidget); + final redirectCountAfterInit = redirectCallCount; + + // Navigate to /a — should be blocked by onEnter. + router.go('/a'); + await tester.pumpAndSettle(); + + // Navigation was blocked; still on Home. + expect(find.text('Home'), findsOneWidget); + expect(find.text('A'), findsNothing); + expect(find.text('B'), findsNothing); + // Redirect function should NOT have been called for /a since + // onEnter blocked before redirects were evaluated. + expect(redirectCallCount, redirectCountAfterInit); + }); + }); }); } diff --git a/packages/go_router/test/redirect_chain_test.dart b/packages/go_router/test/redirect_chain_test.dart new file mode 100644 index 000000000000..ce8caa8c7f74 --- /dev/null +++ b/packages/go_router/test/redirect_chain_test.dart @@ -0,0 +1,630 @@ +// Copyright 2013 The Flutter Authors +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:flutter/material.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:go_router/go_router.dart'; + +import 'test_helpers.dart'; + +// Tests for chained redirect behavior. +// +// These tests validate that top-level redirects are fully re-evaluated +// when they produce a new location, and that route-level redirects +// trigger top-level re-evaluation on the new location. +void main() { + group('chained redirects', () { + testWidgets('top-level redirect chain', (WidgetTester tester) async { + // Regression test for https://github.com/flutter/flutter/issues/178984 + // + // Top-level redirect: / -> /a -> /b + // Expected: navigating to / should end up at /b + final redirectLog = []; + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const Page1Screen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const Page2Screen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + redirectLog.add(state.matchedLocation); + if (state.matchedLocation == '/') { + return '/a'; + } + if (state.matchedLocation == '/a') { + return '/b'; + } + return null; + }, + ); + + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/b'); + expect(find.byType(Page2Screen), findsOneWidget); + // Redirect evaluated for /, /a, and /b. + expect(redirectLog, ['/', '/a', '/b']); + }); + + testWidgets('top-level redirect chain with three hops', ( + WidgetTester tester, + ) async { + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/step1', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/step2', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/step3', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + return switch (state.matchedLocation) { + '/' => '/step1', + '/step1' => '/step2', + '/step2' => '/step3', + _ => null, + }; + }, + ); + + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/step3', + ); + expect(find.byType(LoginScreen), findsOneWidget); + }); + + testWidgets('async top-level redirect chain', (WidgetTester tester) async { + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) async { + // Simulate an async operation (e.g., checking auth status). + await Future.delayed(Duration.zero); + if (state.matchedLocation == '/') { + return '/a'; + } + if (state.matchedLocation == '/a') { + return '/b'; + } + return null; + }, + ); + + await tester.pumpAndSettle(); + + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/b'); + expect(find.byType(LoginScreen), findsOneWidget); + }); + + testWidgets('top-level redirect chain loop detection', ( + WidgetTester tester, + ) async { + // Redirect loop: / -> /a -> /b -> /a (loop) + await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + return switch (state.matchedLocation) { + '/' => '/a', + '/a' => '/b', + '/b' => '/a', // Loop: /a -> /b -> /a + _ => null, + }; + }, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + expect(find.byType(TestErrorScreen), findsOneWidget); + final TestErrorScreen screen = tester.widget( + find.byType(TestErrorScreen), + ); + expect( + (screen.ex as GoException).message, + startsWith('redirect loop detected'), + ); + }); + + testWidgets('top-level redirect chain loop to initial location', ( + WidgetTester tester, + ) async { + // Redirect loop returning to initial location: / -> /a -> / + await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + return switch (state.matchedLocation) { + '/' => '/a', + '/a' => '/', // Loop back to initial. + _ => null, + }; + }, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + expect(find.byType(TestErrorScreen), findsOneWidget); + final TestErrorScreen screen = tester.widget( + find.byType(TestErrorScreen), + ); + expect( + (screen.ex as GoException).message, + startsWith('redirect loop detected'), + ); + }); + + testWidgets('top-level redirect chain into route-level redirect', ( + WidgetTester tester, + ) async { + // Top-level: / -> /intermediate + // Route-level on /intermediate: /intermediate -> /final + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/intermediate', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + redirect: (BuildContext context, GoRouterState state) => '/final', + ), + GoRoute( + path: '/final', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + if (state.matchedLocation == '/') { + return '/intermediate'; + } + return null; + }, + ); + + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/final', + ); + expect(find.byType(LoginScreen), findsOneWidget); + }); + + testWidgets('route-level redirect triggers top-level redirect', ( + WidgetTester tester, + ) async { + // Route-level on /src: /src -> /dst + // Top-level: /dst -> /final + final topRedirectLog = []; + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + routes: [ + GoRoute( + path: 'src', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + redirect: (BuildContext context, GoRouterState state) => '/dst', + ), + ], + ), + GoRoute( + path: '/dst', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/final', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + initialLocation: '/src', + redirect: (BuildContext context, GoRouterState state) { + topRedirectLog.add(state.matchedLocation); + if (state.matchedLocation == '/dst') { + return '/final'; + } + return null; + }, + ); + + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/final', + ); + expect(find.byType(LoginScreen), findsOneWidget); + // Top-level redirect was evaluated on /dst (after route-level redirect). + expect(topRedirectLog, contains('/dst')); + }); + + testWidgets('top-level redirect returns null after first redirect', ( + WidgetTester tester, + ) async { + // The most common pattern: single redirect, then null on re-evaluation. + var callCount = 0; + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/login', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + callCount++; + if (state.matchedLocation == '/') { + return '/login'; + } + return null; + }, + ); + + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/login', + ); + expect(find.byType(LoginScreen), findsOneWidget); + // Redirect is called twice: once for /, once for /login. + expect(callCount, 2); + }); + + testWidgets('top-level redirect chain respects redirect limit', ( + WidgetTester tester, + ) async { + // Endless chain: /0 -> /1 -> /2 -> ... with a low limit. + await createRouter( + [ + for (int i = 0; i <= 20; i++) + GoRoute( + path: '/$i', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + ], + tester, + initialLocation: '/0', + redirect: (BuildContext context, GoRouterState state) { + final RegExpMatch? match = RegExp( + r'^/(\d+)$', + ).firstMatch(state.matchedLocation); + if (match != null) { + final int current = int.parse(match.group(1)!); + return '/${current + 1}'; + } + return null; + }, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + expect(find.byType(TestErrorScreen), findsOneWidget); + }); + + testWidgets('top-level redirect chain succeeds at exact redirect limit', ( + WidgetTester tester, + ) async { + // Chain of 2 redirects: / -> /a -> /b. With limit=2, this is exactly + // at the boundary (2 entries added to redirectHistory). + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + return switch (state.matchedLocation) { + '/' => '/a', + '/a' => '/b', + _ => null, + }; + }, + redirectLimit: 2, + ); + + // Exactly 2 redirects with limit=2 should succeed. + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/b'); + expect(find.byType(LoginScreen), findsOneWidget); + }); + + testWidgets( + 'top-level redirect chain fails when exceeding redirect limit', + (WidgetTester tester) async { + // Chain of 2 redirects: / -> /a -> /b. With limit=1, the second + // redirect exceeds the limit and should error. + await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + return switch (state.matchedLocation) { + '/' => '/a', + '/a' => '/b', + _ => null, + }; + }, + redirectLimit: 1, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + // 2 redirects with limit=1 should error. + expect(find.byType(TestErrorScreen), findsOneWidget); + final TestErrorScreen screen = tester.widget( + find.byType(TestErrorScreen), + ); + expect( + (screen.ex as GoException).message, + startsWith('too many redirects'), + ); + }, + ); + + testWidgets('top-level and route-level redirects share redirect limit', ( + WidgetTester tester, + ) async { + // Top-level: / -> /a (1 redirect) + // Route-level on /a: /a -> /b (1 redirect) + // Route-level on /b: /b -> /c (1 redirect) + // Total: 3 redirects, limit=2 → should error. + await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + redirect: (BuildContext context, GoRouterState state) => '/b', + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + redirect: (BuildContext context, GoRouterState state) => '/c', + ), + GoRoute( + path: '/c', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + if (state.matchedLocation == '/') { + return '/a'; + } + return null; + }, + redirectLimit: 2, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + // Combined chain of 3 redirects exceeds the shared limit of 2. + expect(find.byType(TestErrorScreen), findsOneWidget); + final TestErrorScreen screen = tester.widget( + find.byType(TestErrorScreen), + ); + expect( + (screen.ex as GoException).message, + startsWith('too many redirects'), + ); + }); + + testWidgets('async top-level redirect chain loop detection', ( + WidgetTester tester, + ) async { + // Async redirect loop: / -> /a -> /b -> /a (loop) + await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) async { + await Future.delayed(Duration.zero); + return switch (state.matchedLocation) { + '/' => '/a', + '/a' => '/b', + '/b' => '/a', // Loop + _ => null, + }; + }, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + + await tester.pumpAndSettle(); + + expect(find.byType(TestErrorScreen), findsOneWidget); + final TestErrorScreen screen = tester.widget( + find.byType(TestErrorScreen), + ); + expect( + (screen.ex as GoException).message, + startsWith('redirect loop detected'), + ); + }); + + testWidgets('top-level redirect chain works with router.go()', ( + WidgetTester tester, + ) async { + // Start at /, no redirect from /. Navigate to /a which chains to /c. + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/a', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/b', + builder: (BuildContext context, GoRouterState state) => + const Page1Screen(), + ), + GoRoute( + path: '/c', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) { + if (state.matchedLocation == '/a') { + return '/b'; + } + if (state.matchedLocation == '/b') { + return '/c'; + } + return null; + }, + ); + + // Initial location is / (no redirect for /). + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/'); + expect(find.byType(HomeScreen), findsOneWidget); + + // Navigate to /a — should chain /a -> /b -> /c. + router.go('/a'); + await tester.pumpAndSettle(); + + expect(router.routerDelegate.currentConfiguration.uri.toString(), '/c'); + expect(find.byType(LoginScreen), findsOneWidget); + }); + }); +} From 3a021e06d987dccf5ddd5afa17adca3276d665bb Mon Sep 17 00:00:00 2001 From: David Miguel Date: Sat, 4 Apr 2026 00:43:42 +0200 Subject: [PATCH 2/3] Refactor: unify top-level and route-level redirects into redirect() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address reviewer feedback by moving applyTopLegacyRedirect() inside redirect() so it's the single entry point for all redirect processing. - redirect() now calls applyTopLegacyRedirect() first, then route-level redirects, recursing naturally when either changes the location - Simplified parseRouteInformationWithDependencies — no longer calls applyTopLegacyRedirect separately - Removed preSharedHistory parameter from _navigate - Added context.mounted guard in _navigate result handler to protect the relocated async boundary - Added async cross-type redirect tests and context disposal tests --- packages/go_router/lib/src/configuration.dart | 166 +++++++----- packages/go_router/lib/src/parser.dart | 74 ++--- .../go_router/test/redirect_chain_test.dart | 252 ++++++++++++++++++ 3 files changed, 372 insertions(+), 120 deletions(-) diff --git a/packages/go_router/lib/src/configuration.dart b/packages/go_router/lib/src/configuration.dart index f59abd498aec..8f955bb4989d 100644 --- a/packages/go_router/lib/src/configuration.dart +++ b/packages/go_router/lib/src/configuration.dart @@ -249,7 +249,7 @@ class RouteConfiguration { /// Legacy top level page redirect. /// - /// This is handled via [applyTopLegacyRedirect] and runs at most once per navigation. + /// This is handled via [applyTopLegacyRedirect] inside [redirect]. GoRouterRedirect get topRedirect => _routingConfig.value.redirect; /// Top level page on enter. @@ -401,93 +401,121 @@ class RouteConfiguration { return const []; } - /// Processes route-level redirects by returning a new [RouteMatchList] representing the new location. + /// Processes all redirects (top-level and route-level) and returns the + /// final [RouteMatchList]. /// - /// This method now handles ONLY route-level redirects. - /// Top-level redirects are handled by applyTopLegacyRedirect. + /// Top-level redirects are evaluated first via [applyTopLegacyRedirect], + /// then route-level redirects run on the resulting match list. If a + /// route-level redirect changes the location, this method recurses — + /// re-evaluating top-level redirects on the new location naturally. FutureOr redirect( BuildContext context, FutureOr prevMatchListFuture, { required List redirectHistory, }) { FutureOr processRedirect(RouteMatchList prevMatchList) { - final prevLocation = prevMatchList.uri.toString(); - - FutureOr processRouteLevelRedirect( - String? routeRedirectLocation, - ) { - if (routeRedirectLocation != null && - routeRedirectLocation != prevLocation) { - final RouteMatchList newMatch = _getNewMatches( - routeRedirectLocation, - prevMatchList.uri, - redirectHistory, - ); + // Step 1: Apply top-level redirect first (self-recursive for chains). + final FutureOr afterTopLevel = applyTopLegacyRedirect( + context, + prevMatchList, + redirectHistory: redirectHistory, + ); - if (newMatch.isError) { - return newMatch; - } - // Re-evaluate top-level redirect on the new location before - // recursing into route-level redirects. This ensures that - // route-level redirects landing on a new path also trigger - // top-level redirect evaluation for that path. - final FutureOr afterTopLevel = applyTopLegacyRedirect( - context, - newMatch, - redirectHistory: redirectHistory, - ); - return redirect( - context, - afterTopLevel, - redirectHistory: redirectHistory, - ); - } - return prevMatchList; + // Step 2: Then apply route-level redirects on the post-top-level result. + if (afterTopLevel is RouteMatchList) { + return _processRouteLevelRedirects( + context, + afterTopLevel, + redirectHistory, + ); } - - final routeMatches = []; - prevMatchList.visitRouteMatches((RouteMatchBase match) { - if (match.route.redirect != null) { - routeMatches.add(match); + return afterTopLevel.then((RouteMatchList ml) { + if (!context.mounted) { + return ml; } - return true; + return _processRouteLevelRedirects(context, ml, redirectHistory); }); + } - try { - final FutureOr routeLevelRedirectResult = - _getRouteLevelRedirect(context, prevMatchList, routeMatches, 0); + if (prevMatchListFuture is RouteMatchList) { + return processRedirect(prevMatchListFuture); + } + return prevMatchListFuture.then(processRedirect); + } + + /// Processes route-level redirects on [matchList]. + /// + /// If a route-level redirect changes the location, recurses back into + /// [redirect] which will re-evaluate top-level redirects on the new path. + FutureOr _processRouteLevelRedirects( + BuildContext context, + RouteMatchList matchList, + List redirectHistory, + ) { + final prevLocation = matchList.uri.toString(); + + FutureOr processRouteLevelRedirect( + String? routeRedirectLocation, + ) { + if (routeRedirectLocation != null && + routeRedirectLocation != prevLocation) { + final RouteMatchList newMatch = _getNewMatches( + routeRedirectLocation, + matchList.uri, + redirectHistory, + ); - if (routeLevelRedirectResult is String?) { - return processRouteLevelRedirect(routeLevelRedirectResult); + if (newMatch.isError) { + return newMatch; } - return routeLevelRedirectResult - .then(processRouteLevelRedirect) - .catchError((Object error) { - final GoException goException = error is GoException - ? error - : GoException('Exception during route redirect: $error'); - return _errorRouteMatchList( - prevMatchList.uri, - goException, - extra: prevMatchList.extra, - ); - }); - } catch (exception) { - final GoException goException = exception is GoException - ? exception - : GoException('Exception during route redirect: $exception'); - return _errorRouteMatchList( - prevMatchList.uri, - goException, - extra: prevMatchList.extra, + // Recurse into redirect — top-level will be re-evaluated at the + // top of the next cycle. + return redirect( + context, + newMatch, + redirectHistory: redirectHistory, ); } + return matchList; } - if (prevMatchListFuture is RouteMatchList) { - return processRedirect(prevMatchListFuture); + final routeMatches = []; + matchList.visitRouteMatches((RouteMatchBase match) { + if (match.route.redirect != null) { + routeMatches.add(match); + } + return true; + }); + + try { + final FutureOr routeLevelRedirectResult = + _getRouteLevelRedirect(context, matchList, routeMatches, 0); + + if (routeLevelRedirectResult is String?) { + return processRouteLevelRedirect(routeLevelRedirectResult); + } + return routeLevelRedirectResult + .then(processRouteLevelRedirect) + .catchError((Object error) { + final GoException goException = error is GoException + ? error + : GoException('Exception during route redirect: $error'); + return _errorRouteMatchList( + matchList.uri, + goException, + extra: matchList.extra, + ); + }); + } catch (exception) { + final GoException goException = exception is GoException + ? exception + : GoException('Exception during route redirect: $exception'); + return _errorRouteMatchList( + matchList.uri, + goException, + extra: matchList.extra, + ); } - return prevMatchListFuture.then(processRedirect); } /// Applies the legacy top-level redirect to [prevMatchList] and returns the diff --git a/packages/go_router/lib/src/parser.dart b/packages/go_router/lib/src/parser.dart index c900b02d8570..5b91b0e81ff9 100644 --- a/packages/go_router/lib/src/parser.dart +++ b/packages/go_router/lib/src/parser.dart @@ -111,54 +111,22 @@ class GoRouteInformationParser extends RouteInformationParser { ); // ALL navigation types now go through onEnter, and if allowed, - // legacy top-level redirect runs, then route-level redirects. + // redirect() handles both top-level and route-level redirects. return _onEnterHandler.handleTopOnEnter( context: context, routeInformation: effectiveRoute, infoState: infoState, onCanEnter: () { - // Compose legacy top-level redirect here (one shared cycle/history). final RouteMatchList initialMatches = configuration.findMatch( effectiveRoute.uri, extra: infoState.extra, ); - final redirectHistory = []; - - final FutureOr afterLegacy = configuration - .applyTopLegacyRedirect( - context, - initialMatches, - redirectHistory: redirectHistory, - ); - - if (afterLegacy is RouteMatchList) { - return _navigate( - effectiveRoute, - context, - infoState, - startingMatches: afterLegacy, - preSharedHistory: redirectHistory, - ); - } - return afterLegacy.then((RouteMatchList ml) { - if (!context.mounted) { - return _lastMatchList ?? - _OnEnterHandler._errorRouteMatchList( - effectiveRoute.uri, - GoException( - 'Navigation aborted because the router context was disposed.', - ), - extra: infoState.extra, - ); - } - return _navigate( - effectiveRoute, - context, - infoState, - startingMatches: ml, - preSharedHistory: redirectHistory, - ); - }); + return _navigate( + effectiveRoute, + context, + infoState, + startingMatches: initialMatches, + ); }, onCanNotEnter: () { // If blocked, stay on the current route by restoring the last known good configuration. @@ -198,7 +166,6 @@ class GoRouteInformationParser extends RouteInformationParser { BuildContext context, RouteInfoState infoState, { FutureOr? startingMatches, - List? preSharedHistory, }) { // If we weren't given matches, compute them here. The URI has already been // normalized at the parser entry point. @@ -206,11 +173,10 @@ class GoRouteInformationParser extends RouteInformationParser { startingMatches ?? configuration.findMatch(routeInformation.uri, extra: infoState.extra); - // History may be shared with the legacy step done in onEnter. - final List redirectHistory = - preSharedHistory ?? []; + final redirectHistory = []; - FutureOr afterRouteLevel(FutureOr base) { + // redirect() handles both top-level and route-level redirects. + FutureOr applyRedirects(FutureOr base) { if (base is RouteMatchList) { return configuration.redirect( context, @@ -222,27 +188,33 @@ class GoRouteInformationParser extends RouteInformationParser { if (!context.mounted) { return ml; } - final FutureOr step = configuration.redirect( + return configuration.redirect( context, ml, redirectHistory: redirectHistory, ); - return step; }); } - // Only route-level redirects from here on out. - final FutureOr redirected = afterRouteLevel(baseMatches); + final FutureOr redirected = applyRedirects(baseMatches); return debugParserFuture = (redirected is RouteMatchList ? SynchronousFuture(redirected) : redirected) .then((RouteMatchList matchList) { + // Guard against context disposal during async redirects. + if (!context.mounted) { + return _lastMatchList ?? + _OnEnterHandler._errorRouteMatchList( + routeInformation.uri, + GoException( + 'Navigation aborted because the router context was disposed.', + ), + extra: infoState.extra, + ); + } if (matchList.isError && onParserException != null) { - if (!context.mounted) { - return matchList; - } return onParserException!(context, matchList); } diff --git a/packages/go_router/test/redirect_chain_test.dart b/packages/go_router/test/redirect_chain_test.dart index ce8caa8c7f74..7eee12809189 100644 --- a/packages/go_router/test/redirect_chain_test.dart +++ b/packages/go_router/test/redirect_chain_test.dart @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:async'; + import 'package:flutter/material.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:go_router/go_router.dart'; @@ -576,6 +578,256 @@ void main() { ); }); + testWidgets('async top-level redirect into route-level redirect', ( + WidgetTester tester, + ) async { + // Async top-level: / -> /mid (async) + // Route-level on /mid: /mid -> /final (sync) + // Exercises the async boundary between top-level and route-level + // processing, and verifies route-level uses the post-top-level match. + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/mid', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + redirect: (BuildContext context, GoRouterState state) => '/final', + ), + GoRoute( + path: '/final', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + redirect: (BuildContext context, GoRouterState state) async { + await Future.delayed(Duration.zero); + if (state.matchedLocation == '/') { + return '/mid'; + } + return null; + }, + ); + + await tester.pumpAndSettle(); + + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/final', + ); + expect(find.byType(LoginScreen), findsOneWidget); + }); + + testWidgets('async route-level redirect into sync top-level chain', ( + WidgetTester tester, + ) async { + // Route-level on /src: /src -> /dst (async) + // Top-level: /dst -> /final (sync) + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + routes: [ + GoRoute( + path: 'src', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + redirect: (BuildContext context, GoRouterState state) async { + await Future.delayed(Duration.zero); + return '/dst'; + }, + ), + ], + ), + GoRoute( + path: '/dst', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/final', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + initialLocation: '/src', + redirect: (BuildContext context, GoRouterState state) { + if (state.matchedLocation == '/dst') { + return '/final'; + } + return null; + }, + ); + + await tester.pumpAndSettle(); + + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/final', + ); + expect(find.byType(LoginScreen), findsOneWidget); + }); + + testWidgets('sync route-level redirect into async top-level chain', ( + WidgetTester tester, + ) async { + // Route-level on /src: /src -> /dst (sync) + // Top-level: /dst -> /final (async) + final GoRouter router = await createRouter( + [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + routes: [ + GoRoute( + path: 'src', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + redirect: (BuildContext context, GoRouterState state) => '/dst', + ), + ], + ), + GoRoute( + path: '/dst', + builder: (BuildContext context, GoRouterState state) => + const DummyScreen(), + ), + GoRoute( + path: '/final', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + tester, + initialLocation: '/src', + redirect: (BuildContext context, GoRouterState state) async { + await Future.delayed(Duration.zero); + if (state.matchedLocation == '/dst') { + return '/final'; + } + return null; + }, + ); + + await tester.pumpAndSettle(); + + expect( + router.routerDelegate.currentConfiguration.uri.toString(), + '/final', + ); + expect(find.byType(LoginScreen), findsOneWidget); + }); + + testWidgets( + 'context disposal during async top-level redirect does not crash', + (WidgetTester tester) async { + // Simulate context disposal while an async top-level redirect is + // in flight. The router should handle this gracefully — not navigate + // to /target after the context is unmounted. + final redirectStarted = Completer(); + final proceedRedirect = Completer(); + + final router = GoRouter( + routes: [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + ), + GoRoute( + path: '/target', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + redirect: (BuildContext context, GoRouterState state) async { + if (state.matchedLocation == '/') { + redirectStarted.complete(); + await proceedRedirect.future; + return '/target'; + } + return null; + }, + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + addTearDown(router.dispose); + + // Mount the router. + await tester.pumpWidget(MaterialApp.router(routerConfig: router)); + await tester.pump(); + + // Wait for the redirect to start. + await redirectStarted.future; + + // Unmount the MaterialApp.router, which disposes the router context. + await tester.pumpWidget(const SizedBox.shrink()); + + // Let the redirect complete after context is unmounted. + proceedRedirect.complete(); + await tester.pumpAndSettle(); + + // The router should NOT have navigated to /target. + expect(find.byType(LoginScreen), findsNothing); + }, + ); + + testWidgets( + 'context disposal during async route-level redirect does not crash', + (WidgetTester tester) async { + // Same as above but for route-level async redirects. + final redirectStarted = Completer(); + final proceedRedirect = Completer(); + + final router = GoRouter( + routes: [ + GoRoute( + path: '/', + builder: (BuildContext context, GoRouterState state) => + const HomeScreen(), + redirect: (BuildContext context, GoRouterState state) async { + redirectStarted.complete(); + await proceedRedirect.future; + return '/target'; + }, + ), + GoRoute( + path: '/target', + builder: (BuildContext context, GoRouterState state) => + const LoginScreen(), + ), + ], + errorBuilder: (BuildContext context, GoRouterState state) => + TestErrorScreen(state.error!), + ); + addTearDown(router.dispose); + + await tester.pumpWidget(MaterialApp.router(routerConfig: router)); + await tester.pump(); + + await redirectStarted.future; + + // Unmount the MaterialApp.router. + await tester.pumpWidget(const SizedBox.shrink()); + + // Let the redirect complete after context is unmounted. + proceedRedirect.complete(); + await tester.pumpAndSettle(); + + // The router should NOT have navigated to /target. + expect(find.byType(LoginScreen), findsNothing); + }, + ); + testWidgets('top-level redirect chain works with router.go()', ( WidgetTester tester, ) async { From c543730eada5ea6512481e5e316a754586a97a79 Mon Sep 17 00:00:00 2001 From: David Miguel Date: Thu, 9 Apr 2026 20:07:20 +0200 Subject: [PATCH 3/3] Fix formatting issue --- packages/go_router/lib/src/configuration.dart | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/packages/go_router/lib/src/configuration.dart b/packages/go_router/lib/src/configuration.dart index 8f955bb4989d..e78011f6aa16 100644 --- a/packages/go_router/lib/src/configuration.dart +++ b/packages/go_router/lib/src/configuration.dart @@ -470,11 +470,7 @@ class RouteConfiguration { } // Recurse into redirect — top-level will be re-evaluated at the // top of the next cycle. - return redirect( - context, - newMatch, - redirectHistory: redirectHistory, - ); + return redirect(context, newMatch, redirectHistory: redirectHistory); } return matchList; } @@ -488,8 +484,12 @@ class RouteConfiguration { }); try { - final FutureOr routeLevelRedirectResult = - _getRouteLevelRedirect(context, matchList, routeMatches, 0); + final FutureOr routeLevelRedirectResult = _getRouteLevelRedirect( + context, + matchList, + routeMatches, + 0, + ); if (routeLevelRedirectResult is String?) { return processRouteLevelRedirect(routeLevelRedirectResult);