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
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
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
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
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.
vivek7405
left a comment
There was a problem hiding this comment.
Confirmed and fixed, and it is the finding that settles the design.
I had measured form.action keeping its fragment and built on it. That measurement is real but it only covers the FALLBACK branch: getSubmitAction prefers the raw action ATTRIBUTE (form-encoder.js:33), 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, the two differ only by fragment, my un-gated branch absorbed the Back, and the reader stayed on the validation-error DOM with only the url and scroll moving. That is a regression against main, so it could not ship.
Fixed by gating EVERY absorb on provenance: only the click the router bowed out of is absorbed, and a popstate with no click behind it is left alone whatever its url.
That drops the traversal half of the issue on purpose, and it is worth writing down why rather than burying it. A Back between two fragment states is indistinguishable from the Back out of a 422 re-render: same pathname, same search, differing only by fragment, no click behind either. Separating them means knowing whether the DOM was replaced between the two ENTRIES, which is per-entry state this router does not keep, since every pushState passes null. Turbo tags its entries for exactly that reason, and my own plan rejected that approach, which is what has been costing rounds. Swallowing a validation-error Back is strictly worse than re-rendering one fragment step, so this stops at the click.
The unit and browser cases for the traversal now assert the behaviour it actually has, with the reason inline, rather than being deleted.
Fallout from narrowing the gate to provenance, all in one place. The onPopState call-site comment still described the pathname-and-search rule that narrowing deleted, which is the first comment a reader hits. The data-no-router ordering comment understated its own consequence: with the mark now the only absorber, checking that attribute first would strand every click of such an anchor, not just the repeat. The AGENTS.md headline still claimed the jump is the browser's "on the click and on the traversal alike", contradicting the rest of its own paragraph. The docs-site paragraph lost its closing tag in the rewrite. Two coverage gaps, both introduced by the same narrowing. The browser case for the data-no-router ordering was deleted along with the traversal case that sat beside it, leaving its fixture anchor unreferenced and that ordering untested in the commit that made it matter for both clicks; it is back and now asserts both. And the pathname/search cross-check went untested once the mark short-circuited ahead of it, because every marked case in the suite matched by construction; the two cases that used to claim to be narrowness proofs, and had become duplicates of other no-mark cases, now carry a mark for a url on a different path and search and exercise it. Counterfactuals: deleting the cross-check reds both unit cases, and moving data-no-router back above the bow-out reds the browser case.
vivek7405
left a comment
There was a problem hiding this comment.
All seven are right, and five of them are fallout from the narrowing commit rather than from the original change, which is a useful thing to see.
The two I would call real misses rather than tidying. The data-no-router browser case was deleted along with the traversal case that sat next to it, so the ordering it covered lost its only test in the very commit that made that ordering matter for BOTH clicks rather than only the repeat; its fixture anchor was left unreferenced, which is the tell. And the pathname/search cross-check went untested the moment the mark started short-circuiting ahead of it, because every marked case in the suite matches by construction. The two cases still labelled narrowness proofs had quietly become duplicates of other no-mark cases; they now carry a mark for a url on a different path and search, so they exercise the line they sit next to.
Counterfactuals for both: deleting the cross-check reds the two unit cases, and moving data-no-router back above the bow-out reds the browser case.
The comment findings are the same class as the code ones here, since the onPopState comment is the first thing a reader of this path sees and it described the deleted rule. Fixed, along with the AGENTS.md headline that contradicted its own paragraph and the docs-site closing tag.
vivek7405
left a comment
There was a problem hiding this comment.
Stopping the cycle here rather than fixing and re-checking again, and saying plainly why.
Of the five, one is a real defect and I introduced it in the fix for the PREVIOUS check's finding. I verified the reachability claim rather than taking it: onClick marks only when url.pathname === location.pathname && url.search === location.search, and every writer of currentPageUrl either clears the mark first (performNavigation, performSubmission) or nulls it (disableClientRouter, and enable re-seeds). So the pathname/search branch inside the absorber cannot be reached in production. My two rewritten cases construct a state the code cannot produce, and the two they replaced pinned behaviour that IS reachable, so the unmarked different-search case lost its unit coverage. That is a coverage regression dressed as added coverage, which is worse than the gap it was meant to close.
The other four I read as minor and they are not what stops this: the section banner and the browser-case comment describe the pre-narrowing design, absorbSameDocumentTraversal is now named after the one case it must never absorb, and the PR body says eight browser cases where there are nine.
This is the second consecutive fix-check to surface something must-fix, and the rule I work to says that is where I stop rather than start another round. The signal is real and not about any one finding: each of the last three commits fixed what the previous round found and introduced something smaller of its own. That is a pattern about the change, not about the reviews, and it is the owner's call what to do with it rather than mine to grind through.
The branch, the commits, and the card stay exactly as they are. Not marking this ready for review.
absorbSameDocumentTraversal was named after the one case it must never absorb: a traversal with no click behind it re-renders. Renamed to absorbFragmentClickPopState, which is what it does. Internal to the router-client directory, three references, no exported surface. The tracker cross-check inside it now says what it is. It is unreachable today, since onClick marks only when the anchor's pathname and search already match location and every writer of currentPageUrl clears the mark first, so it carries no counterfactual and claiming one would be false. It stays because that invariant spans three files, and a fourth writer added without clearing the mark would make it reachable. Restores the two unmarked-popstate unit cases that pin reachable behaviour, a different pathname and a different search, which the previous commit replaced with marked cases that assert a state production cannot produce. Those marked cases stay too, relabelled as what they are: coverage of the defensive branch, driven through the test-only tracker setter. Also corrects the section banner, which still claimed a fragment-only traversal is not a navigation, and the cross-document browser case's comment, which described a url-comparison design this PR abandoned.
#1446 (data-preserve-scroll) landed on main and touches the same client-router files. Two real conflicts, both additive rather than contradictory. events.js: keep both imports, resolvePreserveScroll from scroll.js and absorbFragmentClickPopState from navigator.js. The two changes sit in different parts of onClick, since the fragment bow-out returns before the preventDefault that the preserve-scroll opts bag rides on. navigator.js: performSubmission gains their opts parameter and keeps the clearFragmentNav() this branch added at its head. The docs, the test file and the rest merged cleanly with both sides present.
vivek7405
left a comment
There was a problem hiding this comment.
Read the whole merged diff once more, end to end, with #1446 now underneath it.
The final shape is small for what it went through: one module-scope mark in state.js with three tiny accessors, a consume-first absorber in navigator.js, the click bow-out widened to catch the bare #, data-no-router moved below it, and the prefetch mirror kept in lockstep. The merge with data-preserve-scroll is genuinely orthogonal: the bow-out returns before the preventDefault its opts bag rides on, and both features' suites pass against the merged files.
I traced the ordering of every early return in onClick against the mark. Modifier clicks, defaultPrevented, download, and target all bail before the bow-out, and in each of those cases the browser either does not jump in this tab or does not fire a popstate here, so no mark is left and none is needed. data-no-router is the one that had to sit after it, and does.
One niche worth recording rather than fixing: if app code at window-bubble level calls preventDefault on a fragment click AFTER the router marked it, the jump never happens and the mark sits pending. The consume-on-miss design bounds that to exactly one popstate, and the worst case is one wrongly absorbed Back in an app that is already fighting the router at a layer the router's own defaultPrevented contract does not cover. Acceptable, and the consume-once comment in state.js is where a future reader will find the reasoning.
Nothing must-fix. CI is green on this head across all ten checks.
mise ~/.config/mise/config.toml tools: gh@2.97.0
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 the popstate an in-page fragment CLICK produces, and only that. The gate is provenance: the router marks the click it bowed out of, and the nextpopstateconsumes that mark. A popstate with no click behind it is left on the normal path whatever its URL.Provenance rather than a URL comparison, because the URLs carry no signal that separates the cases:
location.hrefunchanged, exactly like a Back between two distinct entries sharing a URL. So an unchanged URL cannot mean "absorb".getSubmitActionprefers the rawactionattribute, which carries no fragment, so a bound-submitter form declaringaction="/p"pushes its 422 re-render at/pwhile the reader sits at/p#sec. That Back differs only by fragment and still has to re-render.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.data-no-routermoved below the bow-out, since that attribute opts out of routing but the browser still jumps and still fires the popstate.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.Scope: the click, not the traversal
This fixes the reported bug and deliberately leaves an ordinary Back or Forward between two fragment states re-rendering, as it does today. That is one of the issue's acceptance criteria and it is not delivered here.
A fragment traversal is indistinguishable from the Back out of a 422 re-render: same pathname, same search, differing only by fragment, no click behind either. Separating them means knowing whether the DOM was replaced between the two entries, which is per-entry state this router does not keep, since every
pushStatepassesnull. Turbo tags its entries for exactly that reason. Swallowing a validation-error Back is strictly worse than re-rendering one fragment step, so this stops at the click. The unit and browser cases assert the behaviour it actually has, with the reason inline.Test plan
packages/core/test/routing/router-client.test.js: 247/247. The load-bearing pair 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 asserting DOM survival rather than only fetch counts, covering the named anchor, the bare#, repeat clicks of both,href="", inside a<webjs-frame>,data-no-router, and the traversal that must still re-render. Plus a case innav-guard.test.jspinning the narrowed guard.hashtest, puttingdata-no-routerback above the bow-out, and removing the disable teardown each red exactly their own case and nothing else./,/docs/client-router,/ui,/ui/buttonwith no broken modulepreloads;webjs checkclean.Known linked-worktree/pre-existing failures, each verified not caused by this branch:
test/bun/listener*(pass in the primary checkout), threedifferential-elisionassertions (fail identically with this branch's source reverted toorigin/main), andasset-helper-serve(fails in the clean primary at main).Docs
.agents/skills/webjs/references/client-router-and-streaming.md,website/app/docs/client-router/page.ts, andAGENTS.mdall state the provenance rule and the traversal carve-out. The two client-router surfaces previously documented thehref="#"trap as behaviour, which this reverses. The scaffold copy is synced from the repo root atprepack.