fix(app-router): settle superseded link navigations - #2025
Conversation
commit: |
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: settle superseded link navigations
I traced the two changes against the navigation runtime, the link-status registry, and the existing navId-supersession guards. The implementation is sound and well-scoped. No blocking issues.
What works correctly
shims/navigation.ts — synchronous notifyLinkNavigationStart() in push/replace
The comment captures the real motivation: clearing the link-owned pending state before React.startTransition so React doesn't defer the idle setPending(false) behind the suspended destination render. The double-call (synchronous here + the existing call inside navigateClientSide) is safe:
- Link-initiated nav:
setLinkForCurrentNavigationsetscurrentNavigationIsLinkInitiated = true; the firstnotifyLinkNavigationStart()consumes the marker and keeps the link pending. - Interrupting
router.push: the synchronous call (marker alreadyfalse) callslinkSetterForMostRecentNavigation(false)and nulls the ref; the second call insidenavigateClientSideis a no-op.
This matches the registry's documented semantics (link-status-registry.ts:62) and is covered by tests/link-status-registry.test.ts.
server/app-browser-entry.ts — per-navigation AbortController
activeNavigationAbortController?.abort()at navigation start actively cancels the superseded live RSC fetch instead of merely ignoring its result via the navId guard — a genuine improvement.- The aborted
fetchthrowsAbortError, lands in thecatch, and theisCurrentNavigation(navId)guard (line 2222) returns early, so no stale commit or hard-nav to a stale URL. - The
finallyreset is correctly guarded (if (activeNavigationAbortController === navigationAbortController)), so a newer navigation's controller is never clobbered. - The controller correctly spans redirect-following hops (reused across the internal
continueloop) and is freed after the final navigation completes — no dangling reference. void cacheBufferPromise.catch(() => {})(line 2156) prevents an unhandled rejection when the teed cache branch rejects on an aborted/superseded stream before the early-return at line 2158 — a good defensive add.
Non-blocking observations
-
No unit coverage for the App Router abort path. The new abort logic is exercised only by the added Playwright fixture. The Pages Router has a dedicated unit test (
tests/shims.test.ts:16040) that asserts the abort signal fires andAbortErroris converted; an equivalent focused test for the App RouternavigateRscpath would harden against regressions, given how much subtle supersession logic lives inapp-browser-entry.ts. Not required for this PR. -
router.refresh()does not clear sticky link-pending state.refresh()calls the navigation runtime'snavigate(navigateRsc) directly rather thannavigateClientSide, so it never invokesnotifyLinkNavigationStart(). Arefresh()that interrupts a pending<Link>click would leave the link'suseLinkStatus()indicator stuck. This is pre-existing and outside the scope of this PR (which targets imperativepush/replaceper the title and the ported Next.js test), but worth a follow-up to confirm Next.js parity — Next.js's refresh-reducer also begins a navigation the tracked link did not initiate.
Validation performed
vp checkon both modified source files: passtests/link-status-registry.test.ts,tests/link-navigation.test.ts,tests/navigation-runtime.test.ts, link-scopedtests/link.test.ts/tests/shims.test.ts: all pass (60+ tests)
LGTM.
|
Review posted to PR #2025. SummaryI reviewed PR #2025 ("settle superseded link navigations") by tracing both changes against the navigation runtime, the link-status registry, and the existing navId-supersession guards. No blocking issues — the implementation is correct and well-scoped. FindingsBoth changes are sound:
Two non-blocking observations:
I verified |
Summary
router.pushorrouter.replacesupersedes a link navigationNext.js parity
Fixes failures from
test/e2e/use-link-status/index.test.ts, including pending state survivingrouter.pushand rapid overlapping navigations.Validation
vp checkand vinext build