Cleans up navigator pop and remove logic #175612

chunhtai · 2025-09-18T18:03:10Z

The original issue is that when a page gets stuck in the adding stage due to top-level animation is still ongoing, removing that route causes it to be in a inconsistent state. While fixing this issue, I also do some clean up on the onDidRemovePage logic because the existing logic can't handle this kind of operation

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

gemini-code-assist

Code Review

This pull request refactors the logic for navigator's pop and remove operations. It introduces a new flag, imperativeRemoval, to distinguish between imperative and declarative route removals. The onDidRemovePage callback is now consistently triggered only for imperative removals of page-based routes. This change also fixes a bug where removing a route that is still in the process of being added could lead to an inconsistent state. The logic now correctly handles this case by transitioning such routes directly to a disposable state. New tests have been added to verify the corrected behavior for onDidRemovePage and to ensure that removing routes during transitions no longer causes issues.

justinmc

Some questions to make sure I understand this correctly before I approve, but otherwise looks good.

This has the potential to break users, but I'm encouraged to see that the customer tests and Google tests are passing.

justinmc · 2025-09-23T15:13:46Z

packages/flutter/lib/src/widgets/navigator.dart

+    if (route._navigator == navigator) {
+      currentState = _RouteLifecycle.removing;
+    } else {
+      // This route is still waiting to be added while a top-most push or pop


push or pop

Since this is handleRemoval, does that mean this is always a pop? Or am I misundestanding?

We have a mechanism that when you suddenly add 2 or more route on top using page api, the top-most route will be pushed with the animation, and the routes in between are added without animation. However, to avoid suddenly jump of animation, the these routes in between will remain in adding state and wait for the top-most route finish page transition.

this line is referring the top-most route has page transition ongoing, in this case any page below that are waiting to be added will stuck in adding state and not installed yet (thus route._navigator == null). In this case if another page update triggered to remove these route, we can just dispose them directly

Got it, thank you for explaining!

packages/flutter/lib/src/widgets/navigator.dart

justinmc · 2025-09-23T15:18:56Z

packages/flutter/lib/src/widgets/navigator.dart

            previousPresent: _getRouteBefore(index, _RouteEntry.willBePresentPredicate)?.route,
          );
-          assert(entry.currentState == _RouteLifecycle.removing);
+          assert(entry.currentState.index >= _RouteLifecycle.removing.index);


I worry about someone naively reordering the entries in _RouteLifecycle and breaking this. Would it be practical to assert the specific values that it can be? Or maybe just add a comment to _RouteLifecycle about the order if there isn't one already?

only page that are <= idle can be moved around using page api

flutter/packages/flutter/lib/src/widgets/navigator.dart

Line 3182 in 6cd85a9

if (!willBePresent) {

either way this assert is to make sure handleRemoval doesn't somehow bring the route back to idle

Ok I see this kind of order comparison is done in many other places, sounds good.

justinmc · 2025-09-23T15:22:52Z

packages/flutter/lib/src/widgets/navigator.dart

+  // Whether this route is removed through Navigator.pop or
+  // Navigator.pushReplacement.
+  bool imperativeRemoval = false;


It looks like false means pop and true means pushReplacement, but that's not super clear from the name of the variable and the comment here. Maybe just explicitly state that in the comment?

In both cases, this will be true.

imperative API in the context of navigator means any history manipulations that do not go through updating Navigator.pages (the declarative API)

imperative API: Navigator.push, Navigator.pop, Navigator.popUntil, Navigator.pushReplacement....

declarative API: Navigator.pages.

The problem is that when people use declarative API to populate the history, but uses imperative API to remove them. You may have inconsistent state that the route has already be removed but the Navigator.pages still contains the page for that route.

Therefore, we provide a onDidRemovePage callback to let developer to update the state when that happen.

I added this boolean to make it clear whether we need to call onDidRemovePage when the _RouteEntry is removed

I updated the code comment

Looks good now, thank you.

justinmc · 2025-09-23T15:31:43Z

packages/flutter/lib/src/widgets/navigator.dart

      assert(!popResult);
    }
-    pop<dynamic>(result);
+    pop<dynamic>(result, imperativeRemoval: false);


So onDidRemovePage will not be called when it is triggered by a pop?

Do you think it would make sense to explain in the docs of onDidRemovePage all of the cases where it will be called?

markForPop is only called when Navigator.pages api update

Ah I see, I read this as a method declaration when I was reviewing the diff.

packages/flutter/test/widgets/navigator_on_did_remove_page_test.dart

justinmc · 2025-09-23T15:35:17Z

packages/flutter/test/widgets/navigator_on_did_remove_page_test.dart

+    expect(removedPage, isEmpty);
+  });
+
+  testWidgets('pop calls onDidRemovePage', (WidgetTester tester) async {


I see above that pop sets imperativeRemoval to false, so how does it end up calling onDidRemovePage?

see the reply to the imperativeRemoval comment

justinmc · 2025-09-23T15:35:53Z

packages/flutter/test/widgets/navigator_on_did_remove_page_test.dart

+    await tester.pumpAndSettle();
+    expect(find.text('new page'), findsOneWidget);
+    expect(removedPage, <Page<void>>[page1]);
+  });


Should any of the cases where onDidRemovePage is not called that are worth testing? Or maybe they're already covered elsewhere...

they only case it shouldn't be called is page api. For testing onDidRemovePage called, I really only need to test pop and replace since all the other variants in terms call these two methods

chunhtai · 2025-09-23T17:03:23Z

I either replied or addressed the comment. Let me know if you have more question, and happy to chat in VC.

justinmc

LGTM 👍 . Thanks for answering all my questions here.

justinmc · 2025-09-23T20:33:30Z

packages/flutter/lib/src/widgets/navigator.dart

+    if (route._navigator == navigator) {
+      currentState = _RouteLifecycle.removing;
+    } else {
+      // This route is still waiting to be added while a top-most push or pop


Got it, thank you for explaining!

justinmc · 2025-09-23T20:36:05Z

packages/flutter/lib/src/widgets/navigator.dart

            previousPresent: _getRouteBefore(index, _RouteEntry.willBePresentPredicate)?.route,
          );
-          assert(entry.currentState == _RouteLifecycle.removing);
+          assert(entry.currentState.index >= _RouteLifecycle.removing.index);


Ok I see this kind of order comparison is done in many other places, sounds good.

justinmc · 2025-09-23T20:37:27Z

packages/flutter/lib/src/widgets/navigator.dart

      assert(!popResult);
    }
-    pop<dynamic>(result);
+    pop<dynamic>(result, imperativeRemoval: false);


Ah I see, I read this as a method declaration when I was reviewing the diff.

justinmc · 2025-09-23T20:38:30Z

packages/flutter/lib/src/widgets/navigator.dart

+  // Whether this route is removed through Navigator.pop or
+  // Navigator.pushReplacement.
+  bool imperativeRemoval = false;


Looks good now, thank you.

auto-submit · 2025-09-24T21:07:06Z

autosubmit label was removed for flutter/flutter/175612, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

…75612)

Roll Flutter from 4a042046a0f1 to b1a28bc065b0 (44 revisions) flutter/flutter@4a04204...b1a28bc 2025-09-25 48625061+muradhossin@users.noreply.github.com web_ui: avoid crash for showPerformanceOverlay; log 'not supported' once (flutter/flutter#173518) 2025-09-25 paulberry@google.com Ignore upcoming `experimental_member_use` warnings. (flutter/flutter#175969) 2025-09-25 engine-flutter-autoroll@skia.org Roll Skia from 753ce2221ce7 to 55436d87e414 (16 revisions) (flutter/flutter#176004) 2025-09-25 tirth@nevercode.io Add google_fonts to team-framework triage guidelines (flutter/flutter#175675) 2025-09-25 bruno.leroux@gmail.com Add tests for InputDecoration borders (M3 and theme normalization) (flutter/flutter#175838) 2025-09-24 737941+loic-sharma@users.noreply.github.com Update Flutter's templates to use dot shorthands (flutter/flutter#175891) 2025-09-24 matt.boetger@gmail.com In Gradle Flutter task, correctly replace '\ ' with ' '. (flutter/flutter#175815) 2025-09-24 47866232+chunhtai@users.noreply.github.com Cleans up navigator pop and remove logic (flutter/flutter#175612) 2025-09-24 rmolivares@renzo-olivares.dev Fix docs in `EditableText` (flutter/flutter#175787) 2025-09-24 47866232+chunhtai@users.noreply.github.com Fixes SemanticsFlags.isLink mis-translated in dart ui ffi (flutter/flutter#175812) 2025-09-24 1063596+reidbaker@users.noreply.github.com Update AGP/Java/Gradle comparison when using analyze --suggestions (flutter/flutter#175808) 2025-09-24 108678139+manu-sncf@users.noreply.github.com Fix SliverMainAxisGroup SliverEnsureSemantics support (flutter/flutter#175671) 2025-09-24 32538273+ValentinVignal@users.noreply.github.com Migrate to `WidgetStateColor` (flutter/flutter#175573) 2025-09-24 ahmedsameha1@gmail.com Make sure that a FlexibleSpaceBar doesn't crash in 0x0 environment (flutter/flutter#175228) 2025-09-24 engine-flutter-autoroll@skia.org Roll Fuchsia Test Scripts from BWj3yYC74ud58QhN0... to APSBP-sS-3FX69Ihf... (flutter/flutter#175944) 2025-09-24 ahmedsameha1@gmail.com Make sure that a MaterialApp doesn't crash in 0x0 environment (flutter/flutter#173090) 2025-09-24 36043466+koukibadr@users.noreply.github.com feat(cupertino): Add selectableDayPredicate parameter to CupertinoDatePicker for selectable day control #171332 (flutter/flutter#171334) 2025-09-24 mohellebiabdessalem@gmail.com Refactor `FlutterInjectorTest` to use lambdas/method reference (flutter/flutter#175777) 2025-09-24 mohellebiabdessalem@gmail.com Replace curly braces with lambdas in `KeyEventChannelTest` (flutter/flutter#175729) 2025-09-24 bkonyi@google.com [ Widget Preview ] Fix filter by file on Windows (flutter/flutter#175783) 2025-09-24 mohellebiabdessalem@gmail.com use lambda expressions /method reference to fix linter issue in `DartMessengerTest.java` (flutter/flutter#175733) 2025-09-24 engine-flutter-autoroll@skia.org Roll Packages from 3413b65 to 117bf63 (9 revisions) (flutter/flutter#175935) 2025-09-24 mohellebiabdessalem@gmail.com refactor code to use method reference and lambdas in `DartMessengerTest.java` (flutter/flutter#175731) 2025-09-24 mohellebiabdessalem@gmail.com Simplify/fix ordering of asserts in `TextInputPluginTest` (flutter/flutter#175784) 2025-09-24 mohellebiabdessalem@gmail.com Introduce a getter for `Project` to get `gradle-wrapper.properties` directly (flutter/flutter#175485) 2025-09-24 mohellebiabdessalem@gmail.com Change the arguments order in `assertEquals` to fix linter issues (flutter/flutter#175719) 2025-09-24 42980667+srivats22@users.noreply.github.com Broken link in NavigationRail documentation (flutter/flutter#175852) 2025-09-24 mdebbar@google.com Updates to flutter web triage links (flutter/flutter#175791) 2025-09-24 jason-simmons@users.noreply.github.com Do not present textures in FlutterMetalLayer if the drawable size changed and the texture's size does not match the new drawable size (flutter/flutter#175450) 2025-09-24 bkonyi@google.com Remove comment about trailing commas from templates (flutter/flutter#175864) 2025-09-24 engine-flutter-autoroll@skia.org Roll Skia from 1c1b19f2ffc3 to 753ce2221ce7 (4 revisions) (flutter/flutter#175909) 2025-09-24 engine-flutter-autoroll@skia.org Roll Skia from 3191a822cf10 to 1c1b19f2ffc3 (2 revisions) (flutter/flutter#175896) 2025-09-24 engine-flutter-autoroll@skia.org Roll Skia from cabeab8cb22c to 3191a822cf10 (14 revisions) (flutter/flutter#175894) 2025-09-24 engine-flutter-autoroll@skia.org Roll Dart SDK from 14b4ced3022a to 899c7340cc4c (4 revisions) (flutter/flutter#175893) 2025-09-24 paulberry@google.com Roll `package:analyzer` forward to `8.2.0`. (flutter/flutter#175849) 2025-09-24 ahmedsameha1@gmail.com Make sure that a VerticalDivider doesn't crash at 0x0 environment (flutter/flutter#174761) 2025-09-24 ahmedsameha1@gmail.com Make sure that Drawer & DrawerHeader don't crash in 0x0 environment (flutter/flutter#174772) 2025-09-24 rmolivares@renzo-olivares.dev Add an assertion for the relationship between `Visibility.maintainState` and `Visibility.maintainFocusability` (flutter/flutter#175552) 2025-09-24 34465683+rkishan516@users.noreply.github.com fix: remove final class modifier on MenuController (flutter/flutter#174490) 2025-09-24 34465683+rkishan516@users.noreply.github.com fix: cupertino sheet broken example with programatic pop (flutter/flutter#175709) 2025-09-24 mdebbar@google.com [web] Fix assertion thrown when hot restarting during animation (flutter/flutter#175856) 2025-09-24 krca0220@gmail.com Add non uniform TableBorder (flutter/flutter#175773) 2025-09-23 mohellebiabdessalem@gmail.com fix small typo in test docs (flutter/flutter#175776) 2025-09-23 mohellebiabdessalem@gmail.com Use `assertNull` to simplify code (flutter/flutter#175720) If this roll has caused a breakage, revert this CL and stop the roller ...

…75612)

Cleans up navigator pop and remove logic

11b7c30

chunhtai requested a review from justinmc September 18, 2025 18:03

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: routes Navigator, Router, and related APIs. labels Sep 18, 2025

gemini-code-assist bot reviewed Sep 18, 2025

View reviewed changes

justinmc reviewed Sep 23, 2025

View reviewed changes

update

ccd2f27

chunhtai requested a review from justinmc September 23, 2025 17:02

justinmc approved these changes Sep 23, 2025

View reviewed changes

Merge branch 'master' into issues/170250

95978f9

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 24, 2025

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 24, 2025

chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 24, 2025

auto-submit bot added this pull request to the merge queue Sep 24, 2025

Merged via the queue into flutter:master with commit 13b4e3b Sep 25, 2025
80 of 81 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 25, 2025

engine-flutter-autoroll mentioned this pull request Sep 25, 2025

Roll Flutter from 4a042046a0f1 to 57b7c46d0120 (43 revisions) flutter/packages#10076

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 25, 2025

13b4e3bc3 Cleans up navigator pop and remove logic (flutter/flutter#1…

ac38c4a

…75612)

engine-flutter-autoroll mentioned this pull request Sep 25, 2025

Roll Flutter from 4a042046a0f1 to b1a28bc065b0 (44 revisions) flutter/packages#10077

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 25, 2025

13b4e3bc3 Cleans up navigator pop and remove logic (flutter/flutter#1…

97f2a8c

…75612)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 12, 2025

13b4e3bc3 Cleans up navigator pop and remove logic (flutter/flutter#1…

6c11bc4

…75612)

Cleans up navigator pop and remove logic #175612

Cleans up navigator pop and remove logic #175612

Uh oh!

Conversation

chunhtai commented Sep 18, 2025

Pre-launch Checklist

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai commented Sep 23, 2025

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Sep 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chunhtai Sep 23, 2025 •

edited

Loading