fix: leave a same-document fragment jump to the browser - #1448
Conversation
Clicking an in-page fragment link re-fetched the current URL and re-swapped the page, so live DOM identity and hydrated component state outside the anchor were destroyed on what should be a jump the router never touches. Chromium fires popstate for a same-document fragment navigation and onPopState treated every popstate as back/forward, so an ordinary anchor click entered the full navigation pipeline. A traversal whose URL differs from the current page only by fragment is same-document by construction: the fragment never reaches the server, both entries resolve to the same response, and the browser has already performed the jump. onPopState now absorbs it. The bow-out also records the new URL, which is load-bearing rather than tidy, since currentPageUrl is otherwise written only by a completed navigation and a bow-out that skipped it left the reverse traversal comparing two equal hrefs and re-navigating after all. The click path had the same defect in a second spelling. Its bow-out tested URL.hash for truthiness, and the serializer reports both a null fragment and an empty one as '', so href="#" was intercepted rather than left alone. Both that line and its mirror in the prefetch eligibility check now test the href for a '#'. href="" keeps navigating, because it resolves to the current URL with the fragment removed, which the spec reloads rather than jumps. Closes #1437
The decision is unit-tested, but the behaviour it exists to protect is not observable in linkedom, which implements no layout, no scrolling and no history traversal, so every assertion about it would pass vacuously against the bug. DOM survival is asserted with an injected node rather than only an expando. Measured against the bug: an expando on a node the incoming response also contains survives the re-swap, because the morph reconciles that node in place and keeps its identity, so an expando alone is a test that passes with the defect present. The nav guard needed narrowing to make any of this observable. It cancelled the default of every anchor click, and preventDefault is exactly what suppresses a native fragment jump, so the suite could not tell a working bow-out from a broken one. Its own docstring already said a pure-fragment link needs no guard, since it never navigates the page away, so this makes the code match the documented contract. href="" stays guarded, because it carries no fragment and the spec reloads it. The nav-guard fixture also restored the URL in teardown without restoring the router's current-page tracker, so a case inherited whatever url the previous case had navigated to. Seeded in setup and cleared in teardown.
|
Design rationale: why the guard records, and why the test guard had to be narrowed Two things about this change are not obvious from the diff, and both cost a measurement to find. The bow-out has to write I checked the other side of the same question before shipping it: everything
The nav guard was cancelling the thing under test. While in there I found the same fixture restoring the URL in teardown without restoring the router's current-page tracker, so a case inherited whatever URL the previous case had navigated to. That is what made my first nav-guard case fetch when it should not have. Seeded in setup, cleared in teardown, which makes every case in that file deterministic rather than only the new one. |
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Went through this one properly, and it does not hold up on the second click.
The popstate guard requires the two hrefs to DIFFER before it absorbs anything. I had reasoned that two history entries can share a URL exactly and that such a traversal is real, so the guard should leave it alone. That reasoning is wrong in the one direction that matters: a repeat click of the same in-page anchor is a history REPLACE, and it still fires popstate, with location.href identical to what the router already believes is current. I measured it in Chromium, two clicks of one #sec link give two popstates, both at the same href, and history.length never moves. So the second click of a <a href="#">Back to top</a> falls straight through to performNavigation and does exactly what this PR is supposed to stop.
It is worse than a wasted fetch. cacheKey strips the fragment, so the popstate branch snapshots the live page under pathname + search and then immediately restores that same key, which means the router re-swaps the live DOM with a serialized clone of itself. Live node identity gone, components re-upgraded, plus a background revalidation. That is the #1437 symptom, reintroduced on the anchor a reader is most likely to click twice.
The tests do not catch it because every case clicks its link exactly once, and the unit case I wrote for the identical-href popstate asserts it DOES navigate, which pins the bug in rather than finding it.
Two smaller things: the helper I added to the nav guard landed between the doc block and the function it documents, and the traversal sentence went into the skill reference but not the docs site, so the two now disagree about the popstate half.
Fixing all three.
The guard required the two hrefs to DIFFER before it absorbed anything, on the reasoning that two history entries can share a url and that such a traversal is real. That reasoning was wrong in the direction that matters. A repeat click of the same in-page anchor REPLACES its history entry rather than pushing one, and it still fires popstate, arriving with location.href identical to what the tracker holds. Measured in Chromium: two clicks of one #sec link give two popstates at the same href with history.length unchanged. So the second click of a back-to-top link fell through to a full navigation and did exactly what this fix exists to prevent. The fall-through is also destructive rather than merely wasteful, since cacheKey strips the fragment, so the popstate branch snapshots the live page and immediately restores that same key, re-swapping the DOM with a clone of itself. Absorbing an identical-href popstate costs nothing. The case the inequality protected cannot be told apart by the router anyway: both entries key one snapshot, so falling through never restores the other one. The unit case asserting the identical-href popstate navigates was pinning the bug in, and is replaced by one asserting it is absorbed, plus separate cases proving a changed pathname and a changed search still navigate.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
The widening went too far, and this is my error rather than the previous round's.
Requiring the hrefs to differ was wrong, because a repeat anchor click replaces its entry and arrives unchanged. Dropping the requirement outright is also wrong, because a fragmentless popstate between two DISTINCT entries that share a url is a real traversal, and the framework's own no-JS write path produces exactly that pair: a bound form emits no action, so getSubmitAction falls back to location.href (form-encoder.js:33) and a 422 re-render pushes a duplicate entry (fetch-apply.js:311). Back from that used to fall through, hit the cache branch, and let the background revalidation swap the fresh render in. Under this commit it does nothing and the reader has to press Back twice.
The two cases are separable and I should have separated them the first time. A repeat anchor click always carries a #, since that is what it navigated to, and the 422 duplicate entry never does. So the guard absorbs when the pathname and search match AND either the hrefs differ or the destination carries a fragment. That keeps the repeat click fixed and leaves the fragmentless same-url traversal exactly where it was before this PR, which is the conservative side to land on for a case with no report against it.
The deleted unit case was the only thing pinning the fragmentless direction, which is why nothing went red. It comes back asserting it still navigates, alongside the fragment-bearing case asserting it is absorbed.
The stale counterfactual comments are a consequence of the same overreach: under the narrow predicate the recording IS load-bearing again for the Back leg, so those comments become true rather than needing rewriting. I will re-run that counterfactual and confirm rather than assume.
Absorbing every same-path popstate went too far. A fragmentless popstate between two DISTINCT history entries that share a url is a real traversal, and the no-JS write path produces exactly that pair: a bound form emits no action attribute, so getSubmitAction falls back to location.href and the 422 re-render pushes a duplicate entry at the page's own url. Back from a validation error then did nothing at all, and the reader had to press it twice to reach the previous page. The repeat-anchor-click case that motivated the widening is separable from it, because that click always CARRIES a fragment (it is what the click navigated to) while the duplicate 422 entry never does. So the guard absorbs when the hrefs differ, or when they match and the destination carries a fragment. The fragmentless same-url traversal keeps its pre-#1437 behaviour, which is the conservative side for a case nothing has reported against. Restores the unit case pinning that direction, which was the only coverage of it and whose removal is why the regression was silent. The recording line is load-bearing again under this predicate, re-verified by counterfactual: removing it reds the unit case and the Back leg of the browser traversal case, which is what the comments on both already claim.
Two popstates can arrive carrying the url the reader is already on, and they need opposite treatment. A repeat click of one in-page anchor replaces its history entry rather than pushing, so it fires popstate with the url unchanged and needs no fetch. A Back between two distinct entries sharing a url is a real traversal that must re-render, and the no-JS write path produces that pair, since a bound form emits no action attribute and its 422 re-render pushes a duplicate entry at the page's own url. The urls are identical in both, so no comparison can separate them, which is what sank the two previous attempts here. Requiring the hrefs to differ missed the repeat click. Allowing an identical href when it carried a fragment missed the 422 Back as soon as the reader had anchored in first: form.action reflects the node document's URL and KEEPS its fragment, measured in Chromium for a missing action attribute and an empty one alike, so that duplicate entry carries #sec too. What separates them is provenance rather than spelling. The router saw the click it bowed out of and never sees a traversal, so onClick leaves a mark and the next popstate consumes it. The mark is consumed whether or not it matched, so it cannot outlive its popstate, and a real navigation or submission drops it so it cannot leak into the 422 path. Adds the browser-layer case that pins the traversal direction, which the change had only at the unit layer, and which is where it belongs since linkedom drives no history traversal at all.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
You are right, and the premise I built the last commit on is false.
I measured form.action in Chromium before changing anything: with no action attribute, and with action="", it returns the document URL WITH its fragment. So a reader who uses an in-page anchor and then submits produces a 422 duplicate entry carrying #sec, and my fragment-presence clause swallowed the Back out of the validation error. That is the same defect the commit claimed to fix, one step further along.
The real lesson is that the urls in these two cases are byte-identical, so no comparison of them can work, and both of my previous rules were guesses at a discriminator that does not exist. What does separate them is provenance: the router SAW the click it bowed out of, and it never sees a traversal. onClick now leaves a mark, the next popstate consumes it, and a real navigation or submission drops it so it cannot leak into the 422 path. A popstate whose url CHANGED still needs no mark, because the pathname and search test already proves it is same-document.
The two unit cases that pin this now have byte-identical urls and opposite expectations, differing only in whether the click was marked, which is the honest shape of the problem.
Also added the browser case for the traversal direction. You were right that it belongs there and not only at the unit layer, since linkedom drives no history traversal at all, so the unit test only ever proved a return value. Reverting the provenance clause to the fragment-presence rule reds that browser case and its unit twin.
And the PR body is rewritten. It was still describing the first commit's rule, which matters because it becomes the squash commit message.
Five gaps around the provenance mark, all reachable. data-no-router was checked before the fragment bow-out, so a repeat click of a data-no-router in-page anchor left no mark, arrived with an unchanged url, and was re-navigated destructively. That attribute opts out of ROUTING, and the bow-out routes nothing either way, but the browser still performs the native jump and still fires the popstate that has to be recognised. Moved the check after the bow-out, which changes nothing else: every other path through it already returned. disableClientRouter tore down every other pending piece of router state and left the mark armed, so one left by a click whose popstate had not fired could survive a disable and absorb the first same-url popstate after a re-enable. The clear-on-miss line, and both clearFragmentNav calls, had no counterfactual: every marked test popped the href it marked, and nothing marked one then started real work. Reverting any of them left the suite green. Each now has a test that reds, including one proving a stale mark cannot absorb a later real traversal. pendingFragmentNav was an unprefixed test-only export, which this module's own convention says reads as public API. Renamed to _pendingFragmentNav.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
All five are real and all five are fixed.
The data-no-router one is the finding I would have missed longest, because the attribute reads as "the router is not involved here" and the whole point of the mark is that the router IS involved: it saw the click. The browser still jumps and still fires the popstate whatever that attribute says. Moved the check after the bow-out, which changes nothing else, since every other path through it already returned.
The three test gaps were fair and I had not noticed the shape of them: every marked case in the suite popped the href it had marked, so the clear-on-miss line and both clearFragmentNav calls could be deleted with the suite still green. Each now has a case that reds, and the miss one goes further and proves a stale mark cannot absorb a later real traversal, which is the failure the line actually prevents.
The disable leak is the same class as the miss, over a longer window. And _pendingFragmentNav is renamed.
Counterfactuals re-run for the two behavioural ones: putting data-no-router back before the bow-out reds the repeat-click case, and removing the disable clear reds both it and the disable case.
The assertion is pure module state (mark, disable, read), with no DOM, layout or history traversal in it, so the browser suite was the wrong home and left the node suite CI's unit gate runs with no coverage of the line at all. The browser file's own header states that discipline, and disableClientRouter's sibling teardown obligation for the scroll-anchor window is already a node test, which is where this one sits now. Counterfactual re-run at the new layer: removing the clear reds it.
The differing-href branch was absorbing unconditionally, on the reasoning that same pathname and search proves the two entries resolve to the same server response. They do, but that does not prove they hold the same DOM, and a swap in between makes them differ. The no-JS write path reaches that shape. getSubmitAction prefers the raw action ATTRIBUTE over form.action, and a raw attribute carries no fragment, so a bound-submitter form declaring action="/p" pushes its 422 re-render at /p while the reader sits at /p#sec. Back from that validation error differs only by fragment, was absorbed, and left the reader on the error DOM with only the url and scroll changing. The earlier form.action-keeps-its-fragment measurement holds only on the fallback branch, for a form with no action attribute, which is why this survived the previous round. So a popstate the router did not cause is now left alone whatever its url, and only the click it bowed out of is absorbed. That deliberately drops the traversal half: an ordinary Back or Forward between two fragment states re-renders as it does today, rather than being absorbed. Separating it from the 422 Back needs to know whether the DOM was replaced between the two ENTRIES, which is per-entry state the router does not keep, since every pushState here passes null. Turbo tags its entries for exactly this reason. Swallowing a validation-error Back is strictly worse than re-rendering one fragment step, so this stops at the click.
1 participant
Closes #1437
Clicking an in-page fragment link re-fetched the current URL and re-swapped the whole page, so live DOM identity and any hydrated component state outside the anchor were destroyed on a jump the router should never touch at all. Chromium fires
popstatefor a same-document fragment navigation, andonPopStatetreated every popstate as back/forward, so an ordinary anchor click entered the full navigation pipeline.What changed
onPopStatenow absorbs a popstate that cannot need a fetch. The necessary condition is that the destination shares the page's pathname and search, since only the fragment can then differ and the fragment never reaches the server, so both entries resolve to the same response.That condition is not sufficient, and the gap is the interesting part. Two popstates can arrive carrying the URL the reader is already on, and they need opposite treatment:
popstatewith the URL unchanged. Nothing to fetch.<form action=${fn}>emits noactionattribute, soform.actionreflects the document URL and the 422 re-render pushes a duplicate entry at it.As URLs those are identical, so no comparison can separate them. What separates them is provenance: the router saw the click it bowed out of, and it never sees a traversal.
onClickleaves a mark on its way out, the nextpopstateconsumes it, and a real navigation or submission drops it so it cannot leak. A popstate whose URL changed needs no mark, because the pathname and search test already proves it is same-document.The click path carried the defect in a second spelling. Its bow-out tested
url.hashfor truthiness, sohref="#", the back-to-top idiom, was intercepted rather than left to the browser. Both that line and its mirror ineligibleAnchorHrefnow test thehreffor a#.href=""keeps navigating, because it resolves to the current URL with the fragment removed, which the spec reloads rather than jumps.The shared browser-test nav guard needed narrowing to make any of this observable: it cancelled the default of every anchor click, and
preventDefaultis exactly what suppresses a native fragment jump. Its own docstring already said a pure-fragment link needs no guard, so this makes the code match the contract it documented.What the review changed
Worth recording, because two plausible rules were tried and measured wrong before the provenance one:
cacheKeystrips the fragment, so the popstate branch snapshots the live page and immediately restores the same key, re-swapping the DOM with a clone of itself.form.actionreturns the document URL with its fragment (measured in Chromium, for a missingactionattribute and an empty one alike), so the duplicate entry carries#sectoo.Test plan
packages/core/test/routing/router-client.test.js: seven popstate cases plus theeligibleAnchorHrefextension. 242/242 in that file. The pair that matters is two cases with byte-identical URLs and opposite expectations, differing only in whether the click was marked.packages/core/test/routing/browser/fragment-jump.test.js: nine cases, the headline layer, asserting DOM survival rather than only fetch counts, including the duplicate-entry traversal that must still re-render. Plus a case innav-guard.test.jspinning the narrowed guard.hashtest reds the bare-#case; and reverting the provenance clause to the fragment-presence rule reds the duplicate-entry browser case and its unit twin.Docs
Two surfaces documented the
href="#"trap as behaviour, and this reverses it. Both rewritten:.agents/skills/webjs/references/client-router-and-streaming.mdandwebsite/app/docs/client-router/page.ts.AGENTS.mdstates the rule, provenance clause included. The scaffold copy is synced from the repo root atprepack, so editing the canonical file is the scaffold update.