Cleans up navigator pop and remove logic by chunhtai · Pull Request #175612 · flutter/flutter

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.

)

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

)

fixes flutter#170250 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](https://developers.google.com/gemini-code-assist/docs/review-github-code). 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.  [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md

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

13b4e3b Cleans up navigator pop and remove logic (flutter/flutter#175612

ac38c4a

)

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

13b4e3b Cleans up navigator pop and remove logic (flutter/flutter#175612

97f2a8c

)

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

13b4e3b Cleans up navigator pop and remove logic (flutter/flutter#175612

6c11bc4

)

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