[go_router] Fix pages dropped when switching to an unloaded branch - #12422
[go_router] Fix pages dropped when switching to an unloaded branch#12422Yusufihsangorgel wants to merge 5 commits into
Conversation
Grafting the shell match for a branch's initial location kept the current match list's extra, so an object passed to the outer location reached a branch that never asked for it. Take the extra from the match list built for the initial location instead, which is what GoRouter.go would produce.
findMatch returns an error only when it matched nothing, and an empty match list already leaves newShellMatch null, so the second disjunct never decides the outcome.
github-actions
Bot
added
p: go_router
triage-framework
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies StatefulNavigationShellState in go_router to preserve parent navigator pages when switching to an unloaded branch of a StatefulShellRoute by grafting the branch's initial match list into the current match list. It also adds tests to verify this behavior and ensure that extra parameters do not leak. The review feedback suggests simplifying the loop logic in _initialMatchListForBranch for finding newShellMatch to improve readability.
| ShellRouteMatch? newShellMatch; | ||
| initialMatchList.visitRouteMatches((RouteMatchBase match) { | ||
| newShellMatch = match is ShellRouteMatch && match.route == route ? match : newShellMatch; | ||
| return newShellMatch == null; | ||
| }); |
There was a problem hiding this comment.
The current logic for finding newShellMatch and returning the loop control boolean can be simplified to improve readability and reduce cognitive load. Using an explicit if statement with an early return of false is more idiomatic and easier to understand at a glance.
| ShellRouteMatch? newShellMatch; | |
| initialMatchList.visitRouteMatches((RouteMatchBase match) { | |
| newShellMatch = match is ShellRouteMatch && match.route == route ? match : newShellMatch; | |
| return newShellMatch == null; | |
| }); | |
| ShellRouteMatch? newShellMatch; | |
| initialMatchList.visitRouteMatches((RouteMatchBase match) { | |
| if (match is ShellRouteMatch && match.route == route) { | |
| newShellMatch = match; | |
| return false; | |
| } | |
| return true; | |
| }); |
There was a problem hiding this comment.
Done. The old return inferred the stop from newShellMatch instead of saying it at the match.
1 participant
Switching a
StatefulShellRouteto a branch that has not been loaded yet drops the pages a parent Navigator holds below the shell, which is why the back button disappears in the issue's repro.goBranchwithinitialLocation: truetakes the same path. Both call_router.gowith the branch's initial location.gorebuilds the match list from that location alone.This grafts the shell match for the initial location into the current match list so the parent's matches survive, and takes
extrafrom that location's match list so the outerextradoes not leak in. A location with no shell match still goes throughgo.A branch initial location that redirects still loses the graft, since
restorekeeps the supplied list only when the URI is unchanged. The shell lookup also repeats the one in_preloadBranches. I raised both on the issue on 07-31 and took the smaller change here.Fixes flutter/flutter#188295
Pre-Review Checklist
[shared_preferences]///).Footnotes
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