Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions packages/go_router/lib/src/match.dart
Original file line number Diff line number Diff line change
Expand Up @@ -785,6 +785,56 @@ class RouteMatchList with Diagnosticable {
);
}

/// Returns a copy of this list where each [ImperativeRouteMatch] adopts the
/// completer of the live match with the same [pageKey] in [source].
///
/// Decoding an encoded configuration (state restoration, browser
/// back/forward, or a `refreshListenable` re-parse) mints a fresh completer
/// per imperative match, since the push future cannot be serialized. Carrying
/// over the live completer keeps a re-decoded configuration equal to the one
/// already on screen, so the no-op fast path holds — no route spuriously
/// exits and the in-flight push future is preserved. A [pageKey] is unique
/// per push, so it identifies the live match unambiguously.
// TODO(loic-sharma): Remove meta library prefix.
// https://github.com/flutter/flutter/issues/171410
@meta.internal
RouteMatchList reconcileImperativeCompleters(RouteMatchList source) {
final liveCompleters = <ValueKey<String>, Completer<Object?>>{};
source.visitRouteMatches((RouteMatchBase match) {
if (match is ImperativeRouteMatch) {
liveCompleters[match.pageKey] = match.completer;
}
return true;
});
if (liveCompleters.isEmpty) {
return this;
}
return copyWith(matches: _reconcileMatches(matches, liveCompleters));
}

static List<RouteMatchBase> _reconcileMatches(
List<RouteMatchBase> matches,
Map<ValueKey<String>, Completer<Object?>> liveCompleters,
) {
return matches.map<RouteMatchBase>((RouteMatchBase match) {
if (match is ImperativeRouteMatch) {
final Completer<Object?>? live = liveCompleters[match.pageKey];
if (live == null || live == match.completer) {
return match;
}
return ImperativeRouteMatch(
pageKey: match.pageKey,
matches: match.matches,
completer: live,
);
}
if (match is ShellRouteMatch) {
return match.copyWith(matches: _reconcileMatches(match.matches, liveCompleters));
}
return match;
}).toList();
}

@override
bool operator ==(Object other) {
if (other.runtimeType != runtimeType) {
Expand Down
7 changes: 6 additions & 1 deletion packages/go_router/lib/src/parser.dart
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,12 @@ class GoRouteInformationParser extends RouteInformationParser<RouteMatchList> {
incomingUri = routeInformation.uri;
} else if (raw is! RouteInformationState) {
// Restoration/back-forward: decode the stored match list and treat as restore.
final RouteMatchList decoded = _routeMatchListCodec.decode(raw as Map<Object?, Object?>);
// Decoding mints fresh completers for imperative matches; carry over the
// live ones so a re-parse of the current configuration stays a no-op
// instead of spuriously exiting a route or orphaning its push future.
final RouteMatchList decoded = _routeMatchListCodec
.decode(raw as Map<Object?, Object?>)
.reconcileImperativeCompleters(router.routerDelegate.currentConfiguration);
infoState = RouteInformationState.restore(base: decoded);
incomingUri = decoded.uri;
} else {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
changelog: |
- Fixes a spurious `onExit` call on a pushed route when `refreshListenable` fires (e.g. on tab refocus), by carrying over the live push completer when the current configuration is re-decoded.
version: patch
42 changes: 42 additions & 0 deletions packages/go_router/test/match_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,48 @@ void main() {
expect(match1 == match2, isFalse);
expect(match1.hashCode == match2.hashCode, isFalse);
});

test('reconcileImperativeCompleters adopts live completers by pageKey', () async {
// A decoded configuration carries fresh completers; reconciling against the
// live configuration must make the two lists equal again (completer
// included in identity), so the delegate's no-op fast path holds.
final live = RouteMatchList(
matches: <RouteMatchBase>[
matchList1.matches.first,
ImperativeRouteMatch(pageKey: key1, matches: matchList1, completer: completer1),
],
uri: Uri.parse('/'),
pathParameters: const <String, String>{},
);
final decoded = RouteMatchList(
matches: <RouteMatchBase>[
matchList1.matches.first,
ImperativeRouteMatch(pageKey: key1, matches: matchList1, completer: completer2),
],
uri: Uri.parse('/'),
pathParameters: const <String, String>{},
);

expect(decoded == live, isFalse);
final RouteMatchList reconciled = decoded.reconcileImperativeCompleters(live);
expect(reconciled == live, isTrue);
expect((reconciled.matches.last as ImperativeRouteMatch).completer, completer1);
});

test('reconcileImperativeCompleters keeps fresh completer when no live match', () async {
// Genuine state restoration (no live imperative match with that pageKey):
// the fresh completer is kept as-is.
final decoded = RouteMatchList(
matches: <RouteMatchBase>[
matchList1.matches.first,
ImperativeRouteMatch(pageKey: key1, matches: matchList1, completer: completer2),
],
uri: Uri.parse('/'),
pathParameters: const <String, String>{},
);
final RouteMatchList reconciled = decoded.reconcileImperativeCompleters(RouteMatchList.empty);
expect((reconciled.matches.last as ImperativeRouteMatch).completer, completer2);
});
});
}

Expand Down
139 changes: 139 additions & 0 deletions packages/go_router/test/on_exit_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -606,4 +606,143 @@ void main() {
await tester.pumpAndSettle();
expect(find.byKey(home), findsOneWidget);
});

testWidgets('onExit is not called when refreshListenable fires on a pushed route', (
WidgetTester tester,
) async {
var onExitCount = 0;
final home = UniqueKey();
final form = UniqueKey();
final notifier = ValueNotifier<int>(0);
addTearDown(notifier.dispose);
final router = GoRouter(
initialLocation: '/',
refreshListenable: notifier,
routes: <GoRoute>[
GoRoute(
path: '/',
builder: (_, _) => DummyScreen(key: home),
),
GoRoute(
path: '/form',
builder: (_, _) => DummyScreen(key: form),
onExit: (BuildContext context, GoRouterState state) {
onExitCount++;
return true;
},
),
],
);
addTearDown(router.dispose);
await tester.pumpWidget(MaterialApp.router(routerConfig: router));

unawaited(router.push('/form'));
await tester.pumpAndSettle();
expect(find.byKey(form), findsOneWidget);
expect(onExitCount, 0);

// A refreshListenable tick (e.g. a tab-focus claims refresh) re-parses the
// encoded current route. That round-trip must be a no-op: onExit fires only
// when a route actually leaves the stack, not on a refresh re-decode.
notifier.value++;
await tester.pumpAndSettle();

expect(onExitCount, 0);
expect(find.byKey(form), findsOneWidget);
});

testWidgets('live push completer is preserved across a refreshListenable tick', (
WidgetTester tester,
) async {
final home = UniqueKey();
final form = UniqueKey();
final notifier = ValueNotifier<int>(0);
addTearDown(notifier.dispose);
final router = GoRouter(
initialLocation: '/',
refreshListenable: notifier,
routes: <GoRoute>[
GoRoute(
path: '/',
builder: (_, _) => DummyScreen(key: home),
),
GoRoute(
path: '/form',
builder: (_, _) => DummyScreen(key: form),
onExit: (BuildContext context, GoRouterState state) => true,
),
],
);
addTearDown(router.dispose);
await tester.pumpWidget(MaterialApp.router(routerConfig: router));

// Capture completion via a flag rather than awaiting the future directly: if
// the refresh orphans the live completer (the bug), the future never completes
// and `completed` stays false — a synchronous, fail-fast assertion that cannot
// hang the suite.
var completed = false;
Object? completedWith;
unawaited(
router.push<String>('/form').then((Object? value) {
completed = true;
completedWith = value;
}),
);
await tester.pumpAndSettle();

notifier.value++;
await tester.pumpAndSettle();

router.pop<String>('result');
await tester.pumpAndSettle();

expect(completed, isTrue);
expect(completedWith, 'result');
});

testWidgets('replace() to the same location still completes its future', (
WidgetTester tester,
) async {
final home = UniqueKey();
final form = UniqueKey();
final router = GoRouter(
initialLocation: '/',
routes: <GoRoute>[
GoRoute(
path: '/',
builder: (_, _) => DummyScreen(key: home),
),
GoRoute(
path: '/form',
builder: (_, _) => DummyScreen(key: form),
),
],
);
addTearDown(router.dispose);
await tester.pumpWidget(MaterialApp.router(routerConfig: router));

unawaited(router.push<String>('/form'));
await tester.pumpAndSettle();
expect(find.byKey(form), findsOneWidget);

// replace() reuses the old match's pageKey and differs only by a new
// completer, so the completer must stay part of identity — otherwise the new
// configuration equals the current one, setNewRoutePath no-ops, and the
// returned future is orphaned.
var completed = false;
Object? completedWith;
unawaited(
router.replace<String>('/form').then((Object? value) {
completed = true;
completedWith = value;
}),
);
await tester.pumpAndSettle();

router.pop<String>('done');
await tester.pumpAndSettle();

expect(completed, isTrue);
expect(completedWith, 'done');
});
}