Skip to content

[go_router] fix: PopScope.onPopInvokedWithResult not called in branch routes - #9245

Merged
Piinks merged 16 commits into
flutter:mainfrom
Gustl22:138737-go-router-pop-scope
Jun 19, 2025
Merged

Piinks merged 16 commits into
flutter:mainfrom
Gustl22:138737-go-router-pop-scope

Conversation

@Gustl22

@Gustl22 Gustl22 commented May 13, 2025 •

Copy link
Copy Markdown
Contributor

If using PopScope inside StatefulShellRoute.indexedStack it is not called when on top most route.

flutter/flutter#138737 (comment)

Pre-Review Checklist

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

Footnotes

  1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3

@Gustl22
Gustl22 force-pushed the 138737-go-router-pop-scope branch from 4dc074a to f2b12ec Compare May 14, 2025 09:29
@Gustl22
Gustl22 marked this pull request as ready for review May 14, 2025 09:30
@Gustl22
Gustl22 requested a review from chunhtai as a code owner May 14, 2025 09:30

@chunhtai chunhtai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the approach is fine. just a nitpick

if (potentialCandidate.canPop()) {
state = walker.navigatorKey.currentState;
}
states.insert(0, potentialCandidate);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can do a reversed at the end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I updated the code to use reversed and iterable as return type.

@Gustl22
Gustl22 requested a review from chunhtai May 19, 2025 12:49

@chunhtai chunhtai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chunhtai
chunhtai requested a review from hannah-hyj May 22, 2025 19:22
…p-scope

# Conflicts:
#	packages/go_router/CHANGELOG.md

@hannah-hyj hannah-hyj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Gustl22

Gustl22 commented Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

@chunhtai any chance to have this get merged ;D Thx in advance :D

…p-scope

# Conflicts:
#	packages/go_router/CHANGELOG.md
#	packages/go_router/pubspec.yaml
@Gustl22

Gustl22 commented Jun 18, 2025

Copy link
Copy Markdown
Contributor Author

@chunhtai @hannah-hyj reminder to get this merged. Unfortunately, as I am not part of that organization, I cannot add the autosubmit label.

@Piinks

Piinks commented Jun 18, 2025

Copy link
Copy Markdown
Contributor

reminder to get this merged. Unfortunately, as I am not part of that organization, I cannot add the autosubmit label.

Ack! This will likely have a merge conflict once #8352 lands. I should be able to fix the conflict here and get this merged once that one lands.

Comment thread packages/go_router/CHANGELOG.md Outdated
Comment thread packages/go_router/CHANGELOG.md Outdated
@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 18, 2025
@auto-submit auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 18, 2025
@auto-submit

auto-submit Bot commented Jun 18, 2025

Copy link
Copy Markdown
Contributor

autosubmit label was removed for flutter/packages/9245, because - The status or check suite Linux_android android_build_all_packages master has failed. Please fix the issues identified (or deflake) before re-applying this label.

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 19, 2025
@Piinks
Piinks merged commit 8dad1d6 into flutter:main Jun 19, 2025
@Gustl22

Gustl22 commented Jun 19, 2025

Copy link
Copy Markdown
Contributor Author

Thank you @Piinks for landing this 🎉

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 19, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 19, 2025
github-merge-queue Bot pushed a commit to flutter/flutter that referenced this pull request Jun 19, 2025
flutter/packages@715a0a5...0ec4053

2025-06-19 [email protected] Roll Flutter from
8303a96 to 85a9b4f (93 revisions) (flutter/packages#9457)
2025-06-19 [email protected] [go_router]
Update sype safe routing topic to use mixin from go_router_builder 3.0.0
(flutter/packages#9422)
2025-06-19 [email protected] [go_router] fix:
PopScope.onPopInvokedWithResult not called in branch routes
(flutter/packages#9245)
2025-06-18 [email protected] [pigeon]
Create a message call free InstanceManager when running unit tests
(flutter/packages#9395)
2025-06-18 [email protected] [go_router] Use
library prefix for meta (flutter/packages#9434)
2025-06-18 [email protected] [go_router] fix
Popping state and re-rendering scaffold at the same time doesn't update
the URL on web [new] (flutter/packages#8352)
2025-06-18 [email protected] [camera_avfoundation] Fix
incorrect types in image stream events (flutter/packages#9418)
2025-06-18 [email protected] [go_router_builder] Temporarily
restrict `build` (flutter/packages#9453)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected] on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Ortes pushed a commit to Ortes/packages that referenced this pull request Jun 25, 2025
mboetger pushed a commit to mboetger/flutter that referenced this pull request Jul 21, 2025
flutter/packages@715a0a5...0ec4053

2025-06-19 [email protected] Roll Flutter from
8303a96 to 85a9b4f (93 revisions) (flutter/packages#9457)
2025-06-19 [email protected] [go_router]
Update sype safe routing topic to use mixin from go_router_builder 3.0.0
(flutter/packages#9422)
2025-06-19 [email protected] [go_router] fix:
PopScope.onPopInvokedWithResult not called in branch routes
(flutter/packages#9245)
2025-06-18 [email protected] [pigeon]
Create a message call free InstanceManager when running unit tests
(flutter/packages#9395)
2025-06-18 [email protected] [go_router] Use
library prefix for meta (flutter/packages#9434)
2025-06-18 [email protected] [go_router] fix
Popping state and re-rendering scaffold at the same time doesn't update
the URL on web [new] (flutter/packages#8352)
2025-06-18 [email protected] [camera_avfoundation] Fix
incorrect types in image stream events (flutter/packages#9418)
2025-06-18 [email protected] [go_router_builder] Temporarily
restrict `build` (flutter/packages#9453)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected] on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
vashworth pushed a commit to vashworth/packages that referenced this pull request Jul 30, 2025
creatorpiyush pushed a commit to creatorpiyush/packages that referenced this pull request Jun 10, 2026
bisor0627 pushed a commit to bisor0627/packages that referenced this pull request Jun 19, 2026
Nakiha pushed a commit to Nakiha/VoidPlayer-Flutter that referenced this pull request Sep 28, 2026
flutter/packages@715a0a5...0ec4053

2025-06-19 [email protected] Roll Flutter from
2f9715c to 2beb872 (93 revisions) (flutter/packages#9457)
2025-06-19 [email protected] [go_router]
Update sype safe routing topic to use mixin from go_router_builder 3.0.0
(flutter/packages#9422)
2025-06-19 [email protected] [go_router] fix:
PopScope.onPopInvokedWithResult not called in branch routes
(flutter/packages#9245)
2025-06-18 [email protected] [pigeon]
Create a message call free InstanceManager when running unit tests
(flutter/packages#9395)
2025-06-18 [email protected] [go_router] Use
library prefix for meta (flutter/packages#9434)
2025-06-18 [email protected] [go_router] fix
Popping state and re-rendering scaffold at the same time doesn't update
the URL on web [new] (flutter/packages#8352)
2025-06-18 [email protected] [camera_avfoundation] Fix
incorrect types in image stream events (flutter/packages#9418)
2025-06-18 [email protected] [go_router_builder] Temporarily
restrict `build` (flutter/packages#9453)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected] on the revert to ensure that a
human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App p: go_router

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants