[go_router] Fix key collisions when sibling ShellRoutes coexist during push#11143
[go_router] Fix key collisions when sibling ShellRoutes coexist during push#11143Gorniv wants to merge 2 commits intoflutter:mainfrom
Conversation
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 resolves key collision issues within go_router that occurred when navigating between sibling ShellRoute branches. The fix involves generating unique navigator and page keys for each ShellRouteMatch and using the navigator key object directly for _CustomNavigator's key instead of its hash code. These changes are well-tested with a new regression test that forces key hash collisions to verify the fix's robustness. The implementation is sound and effectively addresses the described problem.
| if (subRouteMatches?.isEmpty ?? true) { | ||
| return _empty; | ||
| } | ||
| final matchNavigatorKey = route is ShellRoute |
There was a problem hiding this comment.
does this mean the shell route navigator won't have stable key anymore? this may cause the navigator to keep rebuild from scratch every time the route is reparsed
| context, | ||
| _CustomNavigator( | ||
| // The state needs to persist across rebuild. | ||
| key: GlobalObjectKey(configuration.navigatorKey.hashCode), |
There was a problem hiding this comment.
just want to double check, this is purely clean up right? I would be surprised if this causes different behavior
| return Scaffold( | ||
| body: TextButton( | ||
| onPressed: () async { | ||
| shellNavigatorKey.currentState!.push( |
There was a problem hiding this comment.
This doesn't feel right. maybe we want a different way to create the persistent globalkey?
|
From triage: @Gorniv Are you planning on updating this PR based on the feedback above? |
3 participants
This PR fixes key collisions in
go_routerwhenpush()navigates from oneShellRoutebranch to a siblingShellRoutebranch under the same parent.In that scenario, both shell branches can temporarily coexist in the widget tree. Before this change, that could cause framework assertions such as
Multiple widgets used the same GlobalKeybecause:_CustomNavigatorwas keyed withGlobalObjectKey(navigatorKey.hashCode), which could collide when two navigator keys had the samehashCodeShellRouteMatchreused the configuration-timeShellRoute.navigatorKeyShellRouteMatch.pageKeywas derived only fromroute.hashCode, so multiple matches for the sameShellRoutecould share page identityThis PR fixes the problem for regular
ShellRouteby:GlobalKey<NavigatorState>for eachShellRouteMatchpageKeyfrom that per-match navigator key_CustomNavigatorwithGlobalObjectKey(navigatorKey)instead ofGlobalObjectKey(navigatorKey.hashCode)The change is intentionally scoped to
ShellRouteonly.StatefulShellRoutekeeps its existing stablepageKeybehavior because applying per-match page identity there can create duplicateStatefulNavigationShellinstances with the same internalGlobalKey.This PR also adds a regression test that reproduces the old crash by forcing a navigator key hash collision, verifies that pushing to a sibling shell route no longer crashes, and confirms that the previous shell route state is preserved after
pop().No screenshots are included since this is a routing/runtime fix rather than a visual UI change.
Pre-Review Checklist
[shared_preferences]///).