Repository navigation
[go_router] Fix onException recovery with onEnter - #12216
davidmigloz wants to merge 3 commits into
Conversation
github-actions
Bot
added
p: go_router
triage-framework
…avigation When onEnter blocks the very first navigation there is no prior route to restore, and onException handling returned an empty configuration that tripped the setNewRoutePath assertion. Falls back to the error match list when the current configuration is empty, defers onParserException via a microtask in the no-prior-route branch only, and preserves valid error configurations in parserExceptionHandler. Squash of the two commits on the upstream PR branch: flutter#12216
|
From traige: Is this pr still being worked on? |
tarrinneal
added
the
waiting for response
00aeed1 to
3303cf8
Compare
|
Yes, thanks for the ping. I have rebased this onto current main ( |
github-actions
Bot
removed
the
waiting for response
![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 defers the execution of onParserException using a microtask in GoRouteInformationParser to prevent recovery navigations from being discarded during an initial parse. Additionally, it updates parserExceptionHandler in GoRouter to fall back to the error match list when the current configuration is empty and not an error, preventing an assertion failure in GoRouterDelegate.setNewRoutePath. Relevant unit tests and a changelog entry have been added. There are no review comments, so no further feedback is provided.
|
@Piinks ping from triage :) |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Thanks for tracking this down and adding test coverage for the blocked initial navigation flow.
While tracing through how Flutter's Router handles transaction tokens during parseRouteInformationWithDependencies, I noticed that the same token-stealing race still happens on other parser error paths whenever onEnter is configured, and there is a small inconsistency between how onCanNotEnter in parser.dart and parserExceptionHandler in router.dart handle already-installed error configurations. I left inline comments with details and reproduction cases.
…avigation When onException fires during the initial navigation (e.g., an initial deep link blocked by onEnter returning Block.stop()), routerDelegate.currentConfiguration is empty. Returning it directly from parserExceptionHandler causes GoRouterDelegate.setNewRoutePath to hit `assert(configuration.isNotEmpty || configuration.isError)`. Fall back to the error match list (isError=true) when the current configuration is empty, satisfying the assertion while any recovery navigation queued from onException is processed. Additionally, defer the onParserException call itself to a microtask on that same initial-navigation, blocked-with-no-prior-route path. Calling it synchronously let the app crash-prevention fix land, but a router.go() made synchronously from onException was silently discarded: Flutter's Router mints a new intent token for the re-entrant parse while the initial parse is still in-flight, dropping the outer result. The deferral is scoped to that one parser.dart call site so ordinary initial navigation exceptions (e.g. a plain unmatched route) keep calling onException synchronously, as existing tests rely on.
Move the deferral to the central parser exception handler so every parser error path reached through onEnter gets the same transaction ordering. Restore the parser handler return value so an installed error configuration is preserved when a later navigation is blocked. Add initial, subsequent, and installed-error regression coverage.
3303cf8 to
1124f35
Compare
davidmigloz
changed the title
When
onExceptionstarts a recovery navigation while a parse involvingonEnteris still completing, the recovery can be overwritten by the outer navigation.With
onEnter, parser completion crosses an async boundary. CallingonExceptionbefore returning the parser result can start a re-entrant navigation before Flutter'sRoutercaptures the outer transaction. The outer parse can then reuse the transaction token and apply its stale result after the recovery navigation.There is a second edge case in the same handler: returning
routerDelegate.currentConfigurationunconditionally is invalid during initial navigation, when no configuration has committed yet. It can leave the app blank or trip the delegate's configuration assertion. Conversely, once a route or error configuration has committed, a later parsing failure should not replace it whenonExceptiondoes not navigate.This change handles both cases centrally in
parserExceptionHandler:onEnteris configured, scheduleonExceptionin a microtask so Flutter'sRoutercaptures the outer navigation before recovery begins. WithoutonEnter,onExceptionremains synchronous.The parser continues to invoke this handler synchronously and uses its returned
RouteMatchList; there is no path-specific exception handling inparser.dart.Regression coverage includes:
onEnterconfigured;onExceptionon unmatched initial navigation;onEnter.Validated after rebasing onto current
main(go_router18.1.2):exception_handling_test.dartsuite;Fixes flutter/flutter#189582
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