Skip to content

Only pop the pre-redirect controller when a redirect crosses contexts#251

Merged
svara merged 1 commit into
mainfrom
redirect-fix
Jul 7, 2026
Merged

Only pop the pre-redirect controller when a redirect crosses contexts#251
svara merged 1 commit into
mainfrom
redirect-fix

Conversation

@svara

@svara svara commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Redirects were popping one view controller too many. This narrows the pop so it only fires when a redirect crosses the defaultmodal navigation boundary — fixing the missing / incorrect back button in #190 while preserving the modal-redirect cleanup from #112.

The bug (#190)

Since the 1.3.0 beta, following an internal redirect could leave the navigation stack wrong:

  • Redirect on/near the first screen → the tab's root screen is lost and there's no back button.
  • Redirect after navigating → a back button appears but returns to the wrong screen (one too far back).

Works correctly in 1.2.x.

Root cause

session(_:didProposeVisit:) popped the top controller on every redirect (introduced in #160, generalizing the modal fix from #154 / #112):

if proposal.isRedirect {
    let animatePop = session === modalSession && proposal.context == .default
    pop(animated: animatePop)
}
route(proposal)

But Turbo already re-proposes a followed redirect with an action: "replace" — it also does history.replaceState on the web side, so the redirect never becomes a history entry. Verified in the bundled turbo.min.js (Visit#followRedirect):

followRedirect(){  this.response?.redirected &&
  this.adapter.visitProposedToLocation(this.redirectedToLocation,
    { action: "replace", response: this.response,}) }

So route(proposal) on its own already replaces the pre-redirect controller in place (pushOrReplacereplaceLastViewController). The extra unconditional pop therefore removes a second controller — the screen underneath — which is exactly the lost / wrong back button in #190.

The pop is only actually required when the redirect changes navigation context (defaultmodal): there the pre-redirect controller was pushed onto one stack while route targets the other, leaving it orphaned (the original #112 problem).

The fix

Pop only when the redirect crosses the context boundary:

if proposal.isRedirect {
    let sessionIsModal = session === modalSession
    let proposalIsModal = proposal.context == .modal
    if sessionIsModal != proposalIsModal {
        // Animate only when receding from the modal session to the default context.
        let animatePop = sessionIsModal && proposal.context == .default
        pop(animated: animatePop)
    }
}
route(proposal)
  • Same context (main → main, modal → modal): no pop; the replace action rewrites the pre-redirect controller in place. When the redirect targets the previous screen, pushOrReplace's visitingPreviousPage branch performs the animated back-pop — restoring the transition that went missing in the beta.
  • Cross context (default → modal, modal → default): pop the orphaned pre-redirect controller before routing, preserving the Redirect to Modal Shows Different Screens Between iOS and Android #112 fix.

Behavior

redirect before (current beta) after
main → main (after a screen) loses the underlying screen ❌ replaces in place ✅
main → main (at root) ok ok ✅
main → modal (#112) ok ok ✅
modal → modal loses the underlying screen ❌ replaces in place ✅
modal → main dismiss + route dismiss + route ✅

Relationship to #192

#192 also narrows the pop, but keys it off the session (session === modalSession). A default → modal redirect is re-proposed by the main session (which performed the request), so #192 skips the pop there and re-introduces #112 — the pre-redirect controller is left orphaned beneath the presented modal. It also still double-pops modal → modal. Keying off the context transition instead handles all four quadrants.

Tests

Adds redirect coverage across the full default / modal matrix to NavigationHierarchyControllerTests, driven through the real SessionDelegate entry point (session(_:didProposeVisit:)) rather than route(_:).

@svara
svara requested a review from joemasilotti July 6, 2026 11:11
@svara
svara marked this pull request as ready for review July 6, 2026 11:11

@joemasilotti joemasilotti 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.

Approved assuming this fixes #192!

@svara
svara merged commit 76e1e8c into main Jul 7, 2026
1 check passed
@svara
svara deleted the redirect-fix branch July 7, 2026 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants