[go_router] Fix chained top-level redirects not being fully resolved

[davidmigloz]

Fixes flutter/flutter#178984

The onEnter refactor (commit 9ec29b6d23, PR #8339) split the unified redirect() method into applyTopLegacyRedirect() (top-level, runs once) and redirect() (route-level only). This introduced two regressions:

1. Top-level redirect chains broken: applyTopLegacyRedirect() returned after one hop — a chain like / → /a → /b stopped at /a instead of resolving to /b.
2. Route-level → top-level chains broken: processRouteLevelRedirect() recursed into redirect() without re-evaluating the top-level redirect on the new location.

Fix

Unify top-level and route-level redirects back into redirect() as the single entry point for all redirect processing, while keeping the separation of concerns via applyTopLegacyRedirect() as an internal helper.

Note: An earlier revision of this PR (v1) kept the split but added recursive calls in both methods. Based on reviewer feedback, the approach was refactored to move applyTopLegacyRedirect() inside redirect() so the caller (parseRouteInformationWithDependencies) doesn't need to know about the two-phase process.

Before (broken)

sequenceDiagram
    participant Parser as parseRouteInformationWithDependencies
    participant TopRedir as applyTopLegacyRedirect
    participant Redir as redirect (route-level only)

    Parser->>TopRedir: / (one hop only)
    TopRedir-->>Parser: /a (stops here ✗)
    Parser->>Redir: /a
    Redir->>Redir: route-level redirects
    Note right of Redir: No top-level re-evaluation
    Redir-->>Parser: result

Loading

After (fix)

sequenceDiagram
    participant Parser as parseRouteInformationWithDependencies
    participant Redir as redirect (unified)
    participant TopRedir as applyTopLegacyRedirect

    Parser->>Redir: / (initial matches)
    Redir->>TopRedir: / → top-level redirect chain
    TopRedir->>TopRedir: / → /a → /b (self-recursive)
    TopRedir-->>Redir: /b (fully resolved ✓)
    Redir->>Redir: route-level redirects on /b
    Note right of Redir: If route-level changes location,<br/>recurse → top-level re-evaluated
    Redir-->>Parser: final result

Loading

Key changes

• configuration.dart — redirect(): Now calls applyTopLegacyRedirect() first at every cycle, then processes route-level redirects on the post-top-level result. Route-level _processRouteLevelRedirects extracted as a helper.
• configuration.dart — applyTopLegacyRedirect(): Self-recursive to fully resolve top-level chains. No functional change from v1.
• parser.dart — parseRouteInformationWithDependencies(): Simplified — no longer calls applyTopLegacyRedirect separately. Just passes initial matches to _navigate().
• parser.dart — _navigate(): Removed preSharedHistory parameter. Added context.mounted guard in the result .then() to protect the relocated async boundary.

Both fixes share the existing redirectHistory for loop detection and respect redirectLimit. The onEnter system is completely unaffected — it runs before redirects in the pipeline.

Tests

• 19 redirect chain tests (redirect_chain_test.dart): top-level chains, async chains, loop detection (including loop-to-initial), route→top cross-type chains, async cross-type chains (async top→route, async route→sync top, sync route→async top), context disposal during async top-level and route-level redirects, redirect limit boundary (exact limit succeeds, limit+1 fails), shared limit across redirect types.
• 3 onEnter interaction tests (on_enter_test.dart): onEnter called once when chains resolve, onEnter block prevents redirect evaluation.
• Full suite: 418 tests pass, 0 regressions.

Pre-Review Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene page, which explains my responsibilities.
• I read and followed the relevant style guides and ran the auto-formatter.
• I signed the CLA.
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I linked to at least one issue that this PR fixes in the description above.
• I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under¹.
• I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under¹.
• I updated/added any relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or I have commented below to indicate which test exemption this PR falls under¹.
• All existing and new tests are passing.

Footnotes

1. This PR uses pending_changelogs/ for versioning and changelog, following the go_router batch release process. ↩ ↩² ↩³

[gemini-code-assist]

Code Review

This pull request addresses a regression where chained top-level redirects were not fully resolved and route-level redirects failed to trigger re-evaluation of top-level redirects. The changes modify the redirect and applyTopLegacyRedirect methods in RouteConfiguration to recursively re-evaluate top-level redirects when a new location is produced, ensuring all hops in a redirect chain are resolved and that route-level redirects correctly trigger top-level re-evaluation. The update also clarifies the documentation for applyTopLegacyRedirect regarding recursive evaluation, loop detection, and redirect limits. New tests were added to validate various chained redirect scenarios, including async redirects, loop detection, redirect limit enforcement, and interactions with onEnter hooks. The reviewer specifically noted that the addition of the isError check and the recursive call to applyTopLegacyRedirect correctly implements the fix, ensuring the router's correct functioning.

[chunhtai]

I remembered this behavior was intentional to preserve previous behavior before onEnter is introduced
cc @omar-hanafy

will take a closer look

[omar-hanafy]

Hey @davidmigloz, thanks for the detailed write-up and for putting in the work on this fix.

For context, as @chunhtai mentioned, the single-pass behavior in applyTopLegacyRedirect() was intentional during the onEnter refactor (#8339). The goal then was to simplify the pipeline: run top-level redirect once, then route-level redirects, with onEnter earlier than both. The doc comment explicitly called this out as "at most once".

From the user side though, you are right - this is a regression. Chained top-level redirects worked before v16 and stopped after. Even if the internal change was intentional, this is still a behavior break without a deprecation path.

Your fix looks right to me. I support landing this. Long term, onEnter may become the cleaner model, but while redirect is still supported, it should keep the behavior users expect.

[absar]

@davidmigloz thank you for fixing the regression. when you have some time could you look into the review, v16+ is breaking redirection for us as well

[davidmigloz]

Yes! Sorry, it’s still on my TODO list. I need to find some time to do it.

[davidmigloz]

@chunhtai feedback addressed!