fix: record the history entry before the swap, not after - #1410
Conversation
WebKit binds a same-document pushState entry's back-forward gesture snapshot to the page state at the moment the entry is recorded. The router pushed after applySwap, so the entry for the outgoing url was finalized against the incoming document at a scroll offset the browser had already clamped to that document's height (measured on the gallery: 1600 to 252). On iOS the edge back-swipe then previews a page that never existed and renders blank for the whole gesture. The push now rides into applySwap as a commit-time callback and fires at each commit point, immediately before the DOM mutation, which is Turbo Drive's ordering (PageView.renderPage calls visit.changeHistory() ahead of this.render). The thunk is one-shot, like Turbo's historyChanged guard, and fetchAndApply still calls it on the way out, so the three paths that return 'none' after hard-navigating or dispatching webjs:frame-missing record history exactly as they do today. Doc gate bypassed with WEBJS_NO_DOC_GATE=1: this is an ordering fix inside the client router with an unchanged public surface and unchanged observable behaviour, whose only effect is on a WebKit back-forward gesture snapshot. No doc surface makes a claim this contradicts.
Three halves of the #1406 contract: the callback fires before the DOM mutation on a committing swap, it does not fire on either path that commits nothing, and the caller's deliberate fall-through still records history there so a degradation advances the URL exactly as before.
Sibling of the #1310 back-button restore test, on the forward half. It wraps pushState before an in-page click and asserts the rendered H1 at the moment the entry is recorded. The signal is the H1 rather than location.pathname: the url does not change until the wrapped pushState is applied, so a pathname read inside the wrapper returns the outgoing route whatever the ordering is, and the first draft of this test passed with the fix reverted for exactly that reason. Verified discriminating: with the source change reverted the recorded heading is Card rather than Button.
|
Design rationale: the view-transition path was already correct, and that is evidence for the diagnosis I went in expecting the view-transition path to need its own handling and it does not, so writing down why before someone goes hunting there.
That is worth more than a note about an unaffected path. The one code path that already recorded history ahead of the mutation is the one path this class of defect should not have been reachable on, which is a second, independent line of evidence for the mechanism on top of the instrumented measurement. The popstate restore is a separate mechanism and is untouched. It records no history at all, since the browser has already moved it, so it passes no callback and gets |
vivek7405
left a comment
There was a problem hiding this comment.
Went through this again with fresh eyes. The mechanism holds and the 'none' handling is right, but two things in it are stated more strongly than the code supports, and one of them means the doc gate was bypassed on a justification that is wrong for one surface.
The bigger one is the optimistic loading skeleton. On any route with a loading.{js,ts} covering the deepest live boundary, applyOptimisticLoading has already deleted the outgoing content and inserted the skeleton before the fetch is even issued, so by the time the callback fires the page is chrome plus a skeleton and the offset is already clamped. The reorder is still an improvement there (the snapshot is at least this route's own shell rather than the destination's), but the comments claim an invariant that route class does not have, and neither the browser fixture nor the /ui/button e2e route has a loading template, so nothing would ever catch it. The gallery route the bug was measured on has none either, so the measurement stands; the general claim does not.
The other is website/app/docs/client-router/page.ts:20. It is item 4 of an explicitly ordered "How it works" list and ends "custom elements upgrade, URL updates via pushState". That ordering is now backwards, and it is exactly the kind of factual claim the doc gate exists to catch, so WEBJS_NO_DOC_GATE=1 was the wrong call for that one file.
Third thing, not a defect but worth being explicit about: every test here asserts the proxy (what the DOM and scrollY were at the push), which restates the code change rather than proving WebKit binds the gesture snapshot at that moment. That premise is only confirmable on a real device, and the acceptance criteria say so.
Two claims were stated more strongly than the code supports. The JSDoc and the call-site comment said the callback runs while the outgoing page is still in the DOM. On a route whose deepest live boundary is covered by a loading template, applyOptimisticLoading has already replaced that range with the skeleton and let the engine clamp the offset before the fetch is issued, so the entry is recorded against the shell plus a skeleton. Still better than the destination document, but not the page the reader left, and recording ahead of the optimistic swap is a separate change on a path nothing here measured. The docs site and the router's own module header both listed pushState last in the swap sequence, after the head merge, script re-execution, and custom-element upgrade. It now runs first, so both said the opposite of what the code does.
|
Decision: the docs surface was wrong, so the doc gate bypass was too Recording this because the commit that opened this PR bypassed the doc gate on a justification I no longer stand behind. The reasoning was that the observable behaviour is unchanged, so nothing documented could contradict it. That is true of the app-visible contract (one entry, same url, same event, same scroll) and false of the docs that describe the swap SEQUENCE, which I did not think to check. Two of them listed Both are corrected in The general lesson for the bypass: "no observable behaviour change" is not the same test as "no documented claim changes". An ordering fix has no observable behaviour change by construction, and ordering is precisely what a how-it-works doc enumerates. |
The previous commit corrected the content of the swap-sequence step without moving the step, so both numbered lists asserted that their later step ran before their earlier one. The push is now its own positioned step in each: the docstring separates the tier DECISION from the COMMIT that applies it, and the docs list places the push ahead of the keyed-diff item instead of after it. The carve-out added last commit also named the wrong boundary. applyOptimisticLoading walks deepest-first and takes the first segment that HAS a loading template, skipping the page's own boundary, which normally has none. So the affected class is the innermost LAYOUT boundary carrying a template, which is by construction usually not the deepest live boundary. Dropped the unqualified 'before any mutation' phrasing everywhere in favour of 'before the incoming content replaces what is on screen'. The strong form is false twice: the app-source-drift branch rewrites data-webjs-src ahead of every commit point, and an optimistic loading skeleton has already replaced its range before the fetch. Also updates the blog post, which transcribes the docstring and still carried the old order plus the pre-split module path.
vivek7405
left a comment
There was a problem hiding this comment.
Second read, scoped to the prose commit. It was worse than the thing it fixed, so this round is all on me.
Correcting the CONTENT of a step without moving the STEP left both numbered lists self-contradicting: the docstring still had the push as item 5, sitting after item 3 which is the replace/morph, and the docs list had it as item 4 right after item 3's description of the keyed diff. Each list now asserted its later step runs before its earlier one. Fixed by giving the push its own positioned step: the docstring separates the tier DECISION from the COMMIT that applies it, and the docs list moves the push ahead of the diff item.
The carve-out I added named the wrong boundary too, which is the part that stings in a commit whose whole purpose was prose accuracy. applyOptimisticLoading walks deepest-first and takes the first segment that HAS a template, deliberately skipping the page's own boundary because that one has no loading.{js,ts} next to it. So the affected class is the innermost LAYOUT boundary carrying a template, which is by construction usually not the deepest live boundary. I had written the opposite.
And "before any mutation" was too strong wherever I wrote it. It is false twice over: the app-source-drift branch rewrites data-webjs-src on the importmap tag ahead of every commit point, and the optimistic skeleton has already replaced its range before the fetch goes out. Everything now says "before the incoming content replaces what is on screen", which is the claim the code actually supports.
Last one: blog/client-router-turbo-drive-style.md transcribes that docstring and still carried the old step 5. It is a live page at /blog/client-router-turbo-drive-style, so it was publishing an ordering the router no longer has. Updated, along with the module path in the same paragraph, which still pointed at the pre-split router-client.js rather than the directory behind the barrel.
The head merge runs before the content swap, not after it, so the docs list had the two inverted even after the last pass. The list now states the real sequence: push, head merge, swap, then scripts and upgrade. The frame-path comment still carried the 'before any mutation' phrasing the previous commit claimed to have dropped everywhere. It is false at that site specifically, since applyOptimisticLoading gates on !isPopState && !refresh rather than on frameId, so a frame nav can also have a skeleton on screen already. Dropped the data-webjs-src example from the JSDoc rather than qualifying it. It was overreaching (the rewrite sits behind href && !frameId && !revalidating, so it cannot precede the frame or background commit points) and it was carrying no weight the loading-skeleton case does not already carry. The two streaming descriptions said a soft nav applies the shell and then advances the URL. The streamed shell goes through the same applySwap call, so the URL now advances first.
vivek7405
left a comment
There was a problem hiding this comment.
Third read, scoped to the previous prose commit. Four more, and the pattern in them is the point: I kept correcting individual sentences instead of checking the sequence they describe against the code end to end.
The head merge runs BEFORE the content swap (addNewHeadElements then runWithTransition), and the docs list had those two inverted even after the last pass corrected the item around them. So the list I "fixed" was still asserting a later step ran before an earlier one, which is the exact defect the previous commit's subject claimed to close. The list now walks the real sequence: push, head merge, swap, then scripts and upgrade.
The frame-path comment still said "before any mutation", after a commit message that claimed I had dropped that phrasing everywhere. Worse than sloppy: applyOptimisticLoading gates on !isPopState && !refresh, not on frameId, so a frame nav can have a skeleton on screen already and that site is one where the strong claim is specifically false.
I also pulled the data-webjs-src example out of the JSDoc rather than qualifying it. It claimed to run ahead of "every commit point" and sits behind href && !frameId && !revalidating, so it cannot precede the frame or background ones. It was carrying no weight the loading-skeleton case does not already carry, and a second example is a second thing to get wrong.
Last, two places described a streamed soft nav as applying the shell and then advancing the URL. The streamed shell goes through the same applySwap, so the URL now leads there too.
I swept the repo for every remaining pushState and URL-advance claim rather than only fixing what was reported. Four surfaces mention it and all four now agree with the code.
…docs list The new browser suite really commits its pushState (the wrapper delegates to the original), so it left the runner on a fake path with its wtr-session-id query string gone, where it leaks into the next test and the next file. Every sibling committing-soft-nav suite restores it in teardown and nav-guard.test.js documents why; this one did not. The docs list still described the swap as happening in step 2, the same reading the docstring and the blog were amended to kill, so the list contradicted its own later steps. Step 2 now decides and step 5 applies. Splitting the script/upgrade clause out last commit also left it duplicated as two consecutive steps; they are one step again. The shell-refresh branch reads as a contradiction with the comment three lines above it, so it now says which one is authoritative and why the unreachable call is written anyway.
vivek7405
left a comment
There was a problem hiding this comment.
Fourth read. The one that matters here is not a prose finding: the new browser suite commits its pushState for real and never put the runner's URL back, so it left every following test in that run on a fake path with the wtr-session-id query string gone. Every sibling committing-soft-nav suite restores it in teardown and nav-guard.test.js says why in a comment. Mine was the only one that did not, and cross-file leakage is the kind of thing that surfaces later as an unrelated flake.
The rest is the docs list again. Step 2 still described the swap as HAPPENING, which is the exact reading I amended the docstring and the blog to kill two commits ago, so the list contradicted its own steps 3 through 5. Step 2 decides, step 5 applies. Splitting the script and upgrade clause out last time also left it duplicated as two consecutive steps, which is now one again.
The shell-refresh branch had a comment saying no history entry is written sitting three lines above a recordHistoryNow() call. The call is genuinely unreachable there (a refresh always passes recordHistory false), but the file said one thing and appeared to do another with nothing marking which was authoritative, so it now says so and says why the unreachable call is written anyway.
I also rewrote the PR body's doc-surfaces section. It still claimed N/A from the first commit while the PR now changes four doc surfaces, so it was misdescribing its own change to anyone reading it later.
Merging the two script items last commit inverted the pair. reactivateScripts runs ahead of upgradeCustomElements on every path (activateSwappedRange, the frame swap, and swapFullBody), and the docstring and the blog both already said so, leaving the docs page the only surface with it backwards.
vivek7405
left a comment
There was a problem hiding this comment.
Fifth read, scoped to the teardown-and-list commit. One finding, and it is mine again: merging the two script bullets inverted the pair inside the step. reactivateScripts runs ahead of upgradeCustomElements on every path (activateSwappedRange, the frame swap, and swapFullBody), and the text I replaced had it right. The docstring and the blog both already said scripts first, so the docs page was the lone surface with it backwards, which is the same defect class the list rewrite exists to close, one level down.
Fixed. The list now reads: SSR emits the boundaries, scan and decide, push, head merge, swap, scripts then upgrade, webjs:navigate.
The rest of that commit was independently checked and holds: the unreachability claim on the shell-refresh branch is correct (the only caller passing refresh also passes recordHistory false, and the popstate call site passes neither), and the teardown addition matches the convention in six sibling suites, including capturing the href in setup and restoring it after the pushState wrapper is removed so it uses the native method.
Both one-directional statements were wrong. Verified in Chromium, Firefox, and WebKit: an element whose definition is already registered upgrades synchronously as the swap inserts it, before any script re-executes, while an element a swapped-in script defines cannot upgrade until that script runs. So neither 'scripts then upgrade' nor 'upgrade then scripts' describes it. The explicit upgradeCustomElements call is a safety net for what the insertion reactions did not catch, which is what upgrade.js already says about itself. All three surfaces now say that.
vivek7405
left a comment
There was a problem hiding this comment.
Sixth read, and it contradicted the fifth on the same line, so I stopped taking either at face value and measured it.
Round 5 said scripts re-execute before custom elements upgrade, reading the call order (reactivateScripts then upgradeCustomElements). Round 6 said the opposite, that insertion upgrades the element before any script runs. I probed both in Chromium, Firefox, and WebKit, and they are each right about a different case:
- An element whose definition the page ALREADY holds upgrades synchronously as the swap inserts it, as a custom-element reaction, before
reactivateScriptsis reached. A script running afterwards sees it constructed and connected. - An element that a swapped-in script DEFINES cannot upgrade at insertion, because there is no definition yet. It upgrades when that script runs, and the explicit pass catches it.
So neither one-directional sentence was correct, and my last three edits to this line were all picking a side of a false dichotomy. It is two waves, and upgrade.js already describes its own explicit pass as a safety net, which is the tell I should have read the first time. All three surfaces now say that, and the docs version says it in the terms that matter to someone writing a swapped-in script.
This is the fifth delta round, which is the cap. The fix above is on the branch UNREVIEWED, so I am stopping here rather than running a sixth, and this PR is not ready for review.
Rewriting the upgrade step left it saying scripts re-execute twice, once in the new two-wave explanation and again as the lead-in to the pre-existing detail. The detail now continues from the explanation instead of restating it.
The previous commit got the ordering right and the mechanism wrong. It said the explicit upgrade pass exists chiefly for an element a just-re-executed script has only now defined, which is the one case it can never serve: customElements.define upgrades matching elements already in the document itself, synchronously, before the router's call is reached. Measured on the router's real insertion path this time, rather than through innerHTML, which creates nodes in the live document and so proves nothing about an imported one. In Chromium, Firefox, and WebKit: importing a node whose definition is registered and inserting it upgrades it synchronously, replaceChildren of imported nodes does too, and a later define upgrades what is already inserted. So the explicit calls are backstops for what the platform's insertion reactions miss, and it is not one final pass either: activateSwappedRange interleaves scripts and upgrades per top-level node, while only the frame and full-body tiers do all scripts then upgrade. All three surfaces say that now. Also corrects the e2e comment that asserted the opposite mechanism. It was the only other place explaining why upgradeCustomElements exists, and it now records the observable guarantee it really pins.
vivek7405
left a comment
There was a problem hiding this comment.
Seventh read. It caught something I had asserted and not actually tested, which makes it the most useful round so far.
My previous commit fixed the ordering and then stated the wrong reason for it. I wrote that the explicit upgrade call exists chiefly for an element a just-re-executed script has only now defined. That is precisely the one case it can never serve, because customElements.define() upgrades matching elements already in the document itself, synchronously, before the router's call is reached.
The deeper problem was my verification. I probed with innerHTML, which creates nodes in the live document, and then generalised to the router's path, which imports from a detached DOMParser document. Those are not the same thing and only one of them was the claim. Re-measured properly in Chromium, Firefox, and WebKit: importing a node whose definition is registered and inserting it upgrades synchronously, replaceChildren of imported nodes does too, and a later define upgrades what is already inserted. The ordering claim survives; the mechanism claim did not.
It is also not "a final pass". activateSwappedRange interleaves scripts and upgrades per top-level node, so on both boundary tiers the upgrade for one node runs before a later sibling script executes. Only the frame and full-body tiers do all-scripts-then-upgrade. All three surfaces now say backstop, and the docs version stops asking the reader to sequence the two at all, since the platform makes that unnecessary in both ordinary cases.
Last one, and the blast-radius miss I should have caught: test/e2e/e2e.test.mjs carried the opposite mechanism claim, that upgrades did not fire on replaceChildren and the explicit call is what makes connectedCallback happen. It was the only other place in the repo explaining why that function exists, so it was exactly the occurrence the sweep had to reach. It now records the observable guarantee those tests really pin, without inventing a replacement history for a regression I cannot reconstruct.
…rse path The slots suite carried the same characterisation the previous commit declared wrong, and its own construction was justified by it. Worse, the test does not isolate the observer it is named for: the element is created by innerHTML into a CONNECTED host with the definition already registered, which the platform upgrades synchronously, so it would pass with the observer removed. The comment now says that plainly and says what the test does pin, the end state. It is not rewritten around a platform gap, because none has been measured and a test built on a guessed one would be worse than a narrow honest one. The e2e comment I rewrote last commit named Document.parseHTMLUnsafe as the parse API in present tense. parseHTML uses createHTMLDocument plus setHTMLUnsafe for a partial-nav fragment, which is the common soft-nav case, and reaches parseHTMLUnsafe only for a full document with lossless comments, else DOMParser. The nodes are also cloned by importNode rather than moved, which the same block already said correctly two sentences later. A repo-wide sweep for the superseded wording now returns only the deliberate back-reference in that block.
vivek7405
left a comment
There was a problem hiding this comment.
Eighth read. Two findings, and the first turned out worse than reported.
packages/core/test/slots/browser/light-dom-hydration.test.js still carried the characterisation the last commit declared wrong, and its own construction was justified by it. Reading it to fix the comment, the test does not isolate the observer it is named for at all: the element is created by innerHTML into a CONNECTED host with the definition already registered, which the platform upgrades synchronously, so it would pass with the observer deleted. The comment now says that and says what the test does pin, which is the end state.
I deliberately did NOT rewrite it into a discriminating observer test. That would need a case the platform does not cover, none has been measured here, and a test built on a guessed one is worse than a narrow one that is honest about its reach. Flagging that as a judgement call rather than burying it.
Second, my own e2e rewrite named Document.parseHTMLUnsafe as the parse API in present tense. parseHTML uses createHTMLDocument plus setHTMLUnsafe for a partial-nav fragment, which is the common soft-nav case these very tests produce, and only reaches parseHTMLUnsafe for a full document whose comments survive, else DOMParser. The nodes are cloned by importNode rather than moved, which the same block already said correctly two sentences down. I converted a past-tense historical note into a present-tense mechanism claim and kept the wrong API name inside the one block whose stated job is stating the mechanism correctly.
A repo-wide grep for the superseded wording now returns only the deliberate back-reference.
Root cause of a long correction chain, and it is mine. This PR moves one history push. In documenting that I started explaining custom-element upgrade timing, a subsystem it does not touch, and every round since has been correcting a claim in that explanation rather than anything about the fix. The last one rested on a View Transition example nothing here measured, and which the code suggests is wrong: runWithTransition passes the same swap thunk through, so those insertions are ordinary ones. So the explanation is gone rather than corrected again. The three ordering surfaces now state only what changed, which is that the push moved ahead of the swap, and keep their pre-existing wording for the rest of the sequence. The two test files are reverted to main outright. Their comments about upgrade mechanics predate this branch, are unrelated to the push ordering, and rewriting them was scope creep that produced two more rounds of findings. They carry claims I now believe are inaccurate, but correcting them needs evidence about what the observer is actually for that I do not have, and guessing in a comment is how this started. Kept: the head-merge position fix, since it is the same list and now verified, and the swap.js JSDoc bounding what the fix does and does not achieve, which is about this change and was checked against applyOptimisticLoading.
vivek7405
left a comment
There was a problem hiding this comment.
Ninth read, and it found the thing I should have seen five rounds ago, so I am fixing the cause rather than the sentence.
This PR moves one history push. Documenting that, I started explaining custom-element upgrade timing, which it does not touch, and every round since has been correcting a claim inside that explanation rather than anything about the fix. The reviewer's second finding is the tell: my "a subtree a View Transition deferred" was never measured, and the code argues against it, since runWithTransition passes the SAME swap thunk to startViewTransition, so those insertions are ordinary ones and the API snapshots pixels rather than replacing DOM. I had written an unmeasured mechanism into a source docstring, a docs page, and a published blog post, and then cited it as the reason for leaving a test alone.
So the explanation is deleted rather than corrected a fourth time. The three ordering surfaces now say only what actually changed. The two test files are reverted to main outright: their upgrade-mechanics comments predate this branch, have nothing to do with push ordering, and rewriting them cost two rounds.
That leaves those comments carrying claims I now believe are inaccurate, and I am saying so rather than quietly reverting: light-dom-hydration.test.js describes an observer safety net for a case the platform handles, its test does not isolate the observer at all and would pass with it removed, e2e.test.mjs says upgrades do not fire on replaceChildren, and blog/betting-on-lits-mental-model.md says the observer catches components rendered before their definition loads, which customElements.define does itself. Correcting them needs evidence about what that observer is genuinely for, which I do not have, and inventing it in a comment is exactly how this chain started.
Kept: the head-merge position fix, same list and now verified, and the swap.js JSDoc bounding what this fix does and does not achieve, which is about this change and was checked against applyOptimisticLoading.
Diff is back to the shape the change deserves: the source fix, its tests, and the ordering statements it invalidated.
|
Decision: the loading-skeleton case stays out, and the reason is the supersession guard Raised again that the push is still too late on a route with a Both supersession guards in So the trade is a wrong URL and a junk history entry, on every browser, on failure and on fast clicks, to remove a cosmetic gesture preview artifact on iOS. Doing it safely needs a rollback protocol coordinated across supersession, in the hottest path the router has. There is an ordering argument too. The base fix here is not confirmed on a device yet, because the pixels only exist on a real iPhone. Building a riskier second change on top of an unverified first one is backwards. If the device check says the reorder works, the skeleton case is worth revisiting on its own evidence, with the rollback designed deliberately rather than folded into this. |
#1410 moved the history push ahead of the DOM mutation and merged with its iOS acceptance criterion openly unmet, because the gesture preview exists only on a real iPhone. The blank survived there, so the assumption behind that fix is still untested: that WebKit binds the back-forward snapshot synchronously at the pushState call. If it instead captures the compositing surface when the didSameDocumentNavigation IPC lands in the UI process, that happens after the whole push-swap-scroll task, and reordering inside the task changes nothing the device can see. Rather than guess again, ship the two candidate timings behind default-off levers and let the device choose, which is the method that finally isolated #610. ?raf and ?raf2 hand WebKit one or two frames to paint the outgoing page between the push and the swap; ?scrolllast defers the scroll-to-top past the frame to isolate the clamp from the swap. The yield sits in fetchAndApply rather than at the four commit points inside applySwap, because applySwap is synchronous and cannot await. The thunk is one-shot, so firing it in the caller covers whichever commit point the swap reaches and leaves that call a no-op, which is the same ordering at every one of them rather than at a chosen few.
#1410 moved the history push ahead of the DOM mutation and merged with its iOS acceptance criterion openly unmet, because the gesture preview exists only on a real iPhone. The blank survived there, so the assumption behind that fix is still untested: that WebKit binds the back-forward snapshot synchronously at the pushState call. If it instead captures the compositing surface when the didSameDocumentNavigation IPC lands in the UI process, that happens after the whole push-swap-scroll task, and reordering inside the task changes nothing the device can see. Rather than guess again, ship the two candidate timings behind default-off levers and let the device choose, which is the method that finally isolated #610. ?raf and ?raf2 hand WebKit one or two frames to paint the outgoing page between the push and the swap; ?scrolllast defers the scroll-to-top past the frame to isolate the clamp from the swap. The yield sits in fetchAndApply rather than at the four commit points inside applySwap, because applySwap is synchronous and cannot await. The thunk is one-shot, so firing it in the caller covers whichever commit point the swap reaches and leaves that call a no-op, which is the same ordering at every one of them rather than at a chosen few.
An on-device prior-art run reordered what this A/B should test. Three frameworks, same iPhone, same gesture: WebJs sets scrollRestoration to 'manual' and blanks the preview, Turbo Drive sets 'manual' and blanks it too, and Next's App Router leaves 'auto' and is clean. That kills the assumption the existing levers were built around. #1410 shipped Turbo's exact ordering, and Turbo has the symptom, so ordering was never going to be sufficient. Push timing in fact runs the other way across the three: both frameworks that push ahead of the swap blank, and the one that pushes after does not. Turbo also wraps its whole commit in a requestAnimationFrame and blanks anyway, which is most of the prior probability the two rAF levers had. scrollRestoration is the only property that tracks the symptom. Under 'auto' WebKit records a scroll position per history entry and can compose the gesture preview from it; under 'manual' the app owns that and the browser records nothing, leaving the preview no scroll state to render against. That also explains why the symptom follows the offset at navigation time and why the back button is fine. The lever is a measurement, not a candidate fix: leaving 'auto' lets the browser restore alongside the router's own restore, and reconciling the two is the design work a positive result buys (#1310 / #1313 guard it). Re #1428.
…swipe The router set history.scrollRestoration = 'manual' on boot (the Turbo Drive assumeControlOfScrollRestoration pattern) to stop the browser's own popstate restore racing the snapshot restore. That mode is also what stops the browser RECORDING a scroll position per history entry, and the recorded offset is what WebKit composes the edge back-swipe gesture preview from, so every scrolled page previewed BLANK for the whole gesture. Measured on a real iPhone across frameworks: WebJs and Turbo Drive both set 'manual' and both blank; Next's App Router leaves 'auto' and is clean; and this app flips from blank to correct on that one property. Push ORDERING, which #1410 changed, runs the other way (both frameworks pushing ahead of the swap blank, the one pushing after does not), so it was never the mechanism. The attribute is left alone and the race is settled where it happens instead. The UA's write lands a frame after the popstate handler, inside the restore window that is already open, so that window now writes back an off-target programmatic displacement for its duration. The write-back cannot fight a reader. Every window release event is a CAPTURE-phase input listener, and an input event precedes the scroll it causes, so a user-driven scroll always arrives with the window already closed; what remains is exactly a programmatic write inside the restore's own span. Both properties are asserted, and removing the write-back reds tests on all three engines. Two #1310 assertions now read at frame granularity rather than task granularity. The contract is the offset the reader sees by the next paint: the stale UA write and its correction land in the same task, so a task-granularity read could catch a transient between two writes that was never user-visible. Re #1428.
Every surface that described the router as taking manual control of scroll restoration, or that explained the blank iOS back-swipe preview as a consequence of push ordering, said something now measured false. The contract surfaces (the skill's client-router and muscle-memory references, and the docs site) now state what the router actually does: it leaves history.scrollRestoration at auto so the browser keeps recording per-entry offsets, restores from its own snapshot, and absorbs the browser's own late restore inside the restore window. Each also tells an app not to set 'manual' itself, since doing so re-breaks the gesture preview app-wide, and that line is the first thing most ported scroll-restoration recipes do. The mechanism claim from #1406 is corrected in place rather than deleted, in constants.js, swap.js, fetch-apply.js and the #1410 browser guard. All four asserted that WebKit binds a same-document entry's gesture snapshot at the moment the entry is recorded. Turbo Drive uses that same ordering and previews blank identically on a real iPhone, and the preview was fixed by the scrollRestoration change instead. The ordering is kept on its own merits (an entry should be recorded against the page it belongs to), and each site now says so, including what the guard does NOT prove. The blog post transcribing the router contract carried the same claim on a live page, and gains the Turbo divergence: WebJs borrowed this router from Turbo, inherited its manual-scroll pattern, and this is the first place the two part company. The published changelog for core 0.7.51 repeats the old claim and is left alone: it is a historical record of what that release believed. Re #1428.
…-swipe (#1430) * chore: add guarded on-device levers to A/B the iOS back-swipe blank #1410 moved the history push ahead of the DOM mutation and merged with its iOS acceptance criterion openly unmet, because the gesture preview exists only on a real iPhone. The blank survived there, so the assumption behind that fix is still untested: that WebKit binds the back-forward snapshot synchronously at the pushState call. If it instead captures the compositing surface when the didSameDocumentNavigation IPC lands in the UI process, that happens after the whole push-swap-scroll task, and reordering inside the task changes nothing the device can see. Rather than guess again, ship the two candidate timings behind default-off levers and let the device choose, which is the method that finally isolated #610. ?raf and ?raf2 hand WebKit one or two frames to paint the outgoing page between the push and the swap; ?scrolllast defers the scroll-to-top past the frame to isolate the clamp from the swap. The yield sits in fetchAndApply rather than at the four commit points inside applySwap, because applySwap is synchronous and cannot await. The thunk is one-shot, so firing it in the caller covers whichever commit point the swap reaches and leaves that call a no-op, which is the same ordering at every one of them rather than at a chosen few. * fix: guard the deferred back-swipe scroll on the navigation token The ?scrolllast lever defers the scroll-to-top by a frame, which puts it outside the navigation's own task. A newer navigation can start in that frame, and the deferred callback would then scroll ITS page: worst on the hash branch, where scrollIntoView hunts the old URL's anchor in the new document and lands somewhere arbitrary if that id happens to exist. The synchronous path cannot do this, so the lever was adding a failure mode rather than isolating one, and a diagnostic that exists to measure scroll behaviour must not write scroll into a page it has nothing to do with. * test: abort the superseding navigation instead of leaving it in flight The supersede assertion drove its second navigation with a fetch that never settled, which left a navigation in flight for the rest of the page's life holding the router's token and its own frame state. Under the full browser suite that leak reded an unrelated file, the #1310 back-restore residue assertion, on Firefox, while both files passed in isolation and while the branch's own file passed everywhere. Rejecting with an AbortError settles the navigation down the path the router already takes for a superseded one, so the assertion observes the same thing with nothing left running. Full browser suite green twice at this commit, against a baseline that was green before the file was added. * chore: add a scrollRestoration lever for the back-swipe A/B An on-device prior-art run reordered what this A/B should test. Three frameworks, same iPhone, same gesture: WebJs sets scrollRestoration to 'manual' and blanks the preview, Turbo Drive sets 'manual' and blanks it too, and Next's App Router leaves 'auto' and is clean. That kills the assumption the existing levers were built around. #1410 shipped Turbo's exact ordering, and Turbo has the symptom, so ordering was never going to be sufficient. Push timing in fact runs the other way across the three: both frameworks that push ahead of the swap blank, and the one that pushes after does not. Turbo also wraps its whole commit in a requestAnimationFrame and blanks anyway, which is most of the prior probability the two rAF levers had. scrollRestoration is the only property that tracks the symptom. Under 'auto' WebKit records a scroll position per history entry and can compose the gesture preview from it; under 'manual' the app owns that and the browser records nothing, leaving the preview no scroll state to render against. That also explains why the symptom follows the offset at navigation time and why the back button is fine. The lever is a measurement, not a candidate fix: leaving 'auto' lets the browser restore alongside the router's own restore, and reconciling the two is the design work a positive result buys (#1310 / #1313 guard it). Re #1428. * docs: correct the phantom-entry claim in the raf lever guard The comment said a newer navigation pushes its entry over the superseded one. pushState appends rather than overwrites, so a superseded ?raf navigation leaves an entry for a url that was never rendered and Back lands on it. Left in place deliberately (undoing it races the navigation that just superseded this one, and moving the push later defeats the lever), but a diagnostic measuring back-navigation must not misdescribe what it does to the back stack. A superseded cell should be discarded and re-run. * fix: stop taking manual scroll restoration, so iOS previews the back-swipe The router set history.scrollRestoration = 'manual' on boot (the Turbo Drive assumeControlOfScrollRestoration pattern) to stop the browser's own popstate restore racing the snapshot restore. That mode is also what stops the browser RECORDING a scroll position per history entry, and the recorded offset is what WebKit composes the edge back-swipe gesture preview from, so every scrolled page previewed BLANK for the whole gesture. Measured on a real iPhone across frameworks: WebJs and Turbo Drive both set 'manual' and both blank; Next's App Router leaves 'auto' and is clean; and this app flips from blank to correct on that one property. Push ORDERING, which #1410 changed, runs the other way (both frameworks pushing ahead of the swap blank, the one pushing after does not), so it was never the mechanism. The attribute is left alone and the race is settled where it happens instead. The UA's write lands a frame after the popstate handler, inside the restore window that is already open, so that window now writes back an off-target programmatic displacement for its duration. The write-back cannot fight a reader. Every window release event is a CAPTURE-phase input listener, and an input event precedes the scroll it causes, so a user-driven scroll always arrives with the window already closed; what remains is exactly a programmatic write inside the restore's own span. Both properties are asserted, and removing the write-back reds tests on all three engines. Two #1310 assertions now read at frame granularity rather than task granularity. The contract is the offset the reader sees by the next paint: the stale UA write and its correction land in the same task, so a task-granularity read could catch a transient between two writes that was never user-visible. Re #1428. * docs: correct the scroll-restoration contract and the #1406 mechanism Every surface that described the router as taking manual control of scroll restoration, or that explained the blank iOS back-swipe preview as a consequence of push ordering, said something now measured false. The contract surfaces (the skill's client-router and muscle-memory references, and the docs site) now state what the router actually does: it leaves history.scrollRestoration at auto so the browser keeps recording per-entry offsets, restores from its own snapshot, and absorbs the browser's own late restore inside the restore window. Each also tells an app not to set 'manual' itself, since doing so re-breaks the gesture preview app-wide, and that line is the first thing most ported scroll-restoration recipes do. The mechanism claim from #1406 is corrected in place rather than deleted, in constants.js, swap.js, fetch-apply.js and the #1410 browser guard. All four asserted that WebKit binds a same-document entry's gesture snapshot at the moment the entry is recorded. Turbo Drive uses that same ordering and previews blank identically on a real iPhone, and the preview was fixed by the scrollRestoration change instead. The ordering is kept on its own merits (an entry should be recorded against the page it belongs to), and each site now says so, including what the guard does NOT prove. The blog post transcribing the router contract carried the same claim on a live page, and gains the Turbo divergence: WebJs borrowed this router from Turbo, inherited its manual-scroll pattern, and this is the first place the two part company. The published changelog for core 0.7.51 repeats the old claim and is left alone: it is a historical record of what that release believed. Re #1428. * chore: remove the back-swipe diagnostic levers The device has answered, so the instrumentation comes out and the branch is left carrying only the fix. Gone: diagFlag and diagFrameYield from diagnostics.js, the ?raf / ?raf2 frame-yield block and the ?scrolllast deferral in fetch-apply.js, the query-string capture in the website's root layout, the lever browser suite, and the lever unit tests. The forward-nav scroll block returns to its original inline shape, since the thunk existed only to give the lever a second call site. The levers cost two device rounds and never moved the symptom, which was the right outcome to get cheaply: they tested paint timing, and the cause was that the router had taken manual scroll restoration away from the browser. Verified after removal: 233 unit, 215 routing browser tests on Chromium, Firefox and WebKit, and the full browser suite green on all three (76 files). The whole node suite is 4461/4468, the 6 failures being the documented linked-worktree baseline (elision differentials, both Bun listener tests, one asset() prod-handler assertion), none of them client router. Re #1428. * test: pin that a frame nav cannot scroll a restore in progress The write-back added for the back-swipe fix lives in the #1310 restore window, and a frame-targeted navigation is the ONE navigation that deliberately leaves that window open. So the two features meet, and nothing covered the meeting point: the existing frame case asserts only that the window survives, never where the reader ends up. It matters because a click-driven frame nav reaches fetchAndApply with recordHistory: true (unlike loadFrame, which passes false) and the scroll block carries no frameId guard, so it runs the forward-nav scroll-to-top even though it swaps a single region. Inside an open restore that would drop the reader to the top of the page they just came back to. It does not, because the write-back corrects it. Green on Chromium, Firefox and WebKit; disabling the write-back reds this case specifically, so it is not passing vacuously. Re #1428. * refactor: reserve the restored height instead of chasing a clamped offset A Back restore re-inserts an outerHTML snapshot, and that markup is shorter than the page it came from until its components upgrade and render. Every scroll defect this path has had lived in that window: the recorded offset was unreachable, the browser clamped to whatever the short document allowed, and the router healed it afterwards by CHASING the offset once the page grew tall enough to hold it. The snapshot now records the settled scrollHeight alongside the offset, and the restore reserves that height on the root element across the swap. The offset is reachable on the first frame, so the restore lands exactly, once. The clamp cannot occur, so there is nothing to chase. Removed: the clamped branch, catchUpToRestoredScroll and cancelScrollCatchUp with their supersede wiring, and the now-unused ANCHOR_SUPPRESS_FLOOR_MS import. scroll.js drops from 279 to ~200 lines and suppression becomes unconditional. Kept, because the reservation does not subsume them: the anchoring window (content still SHIFTS above the viewport within a constant total height, and anchoring adds that shift to the offset just replayed) and the window's write-back. Two things the migration surfaced, both worth knowing: The restore needs an explicit layout flush before its write. The reservation and the swap both just changed layout, and Chromium and WebKit clamp a scroll against the stale layout and land at 0 while Firefox flushes on its own. The old conditional-suppression code got that flush by accident, since deciding clamped-or-landed read scrollY right after the write. Unconditional suppression removes the accident, so the read is now deliberate. The suite's fixture was building snapshots WITHOUT scrollHeight, which left the reservation inert and every assertion passing for the wrong reason. Fixed, and the fixture now models the real invariant that a recorded offset is always reachable within its own recorded height. The 8 clamp/chase cases are replaced by 4 reservation outcomes: the offset is reachable on the first frame with no clamp and no chase, the reservation leaves no residue, a second navigation releases it, and a deferred view-transition restore still lands on the offset. 212 routing browser tests green on Chromium, Firefox and WebKit; 233 unit. Re #1428. * test: pin the restore against a browser-recorded offset Every case in this suite injects the recorded offset into the snapshot cache while the page sits at 0, so the browser has only ever recorded 0 for that entry. That is fine while the router owns the restore, but it makes the UA's own restoration unobservable, and the UA is a live participant now that scrollRestoration is left at auto. The new `uaRecords` fixture option scrolls before pushing the next entry, so the browser records a real offset, and the new case asserts the reader still lands on it through a SHORT snapshot swap. This is also the gate for the single-writer question, and it answers it NEGATIVELY. Disabling the router's own write reds 8 cases per engine on all three, so the browser cannot carry the restore alone even with the height reserved. Their code says why. Next restores from a cached tree (segmentCacheMap / bfcache in restore-reducer) and React RECONCILES, so the document is never torn down under the UA's restore. Turbo caches live DOM clones (cloneNode(true)) and separately owns scroll outright via manual. Remix 3 tolerates destruction by deferring the UA restore past the swap with event.intercept(). WebJs's replace tier destroys and rebuilds the range by design (a changed route-key is a remount, Next parity), and intercept() is the one mechanism measured to break the iOS gesture preview, so the router keeps the restore. The guard stays because it is what would catch this changing: if the swap ever becomes non-destructive, disabling the router write stops reding and the single-writer design reopens. Re #1428. * docs: describe the height reservation, and drop the chase it replaced The clamp/chase paragraphs were the longest in both the skill reference and the docs site, and they described machinery that no longer exists. Both now describe the reservation instead: the snapshot records the page's settled height, the restore holds it across the swap so the offset is always reachable, and the anchoring window still covers the other half (content shifting within a constant height). Both state the release rules, including why user input deliberately does NOT release the reservation even though it closes the window. Also corrects the last two copies of the #1406 mechanism claim, missed in the earlier sweep: the docs site's how-it-works list and the unit suite's section docstring. Both asserted that WebKit binds the gesture snapshot when the entry is recorded, which #1428 measured false. The ordering is kept and each site now says why it is correct anyway. The published changelog for core 0.7.51 keeps the old claim, being a record of what that release believed. Re #1428. * test: retarget the frame guard's rationale at #1429 main landed #1429, which excludes frame-scoped responses from the forward-nav scroll block outright. That removes the stray scroll this guard was written against: a click-driven frame nav no longer runs the page scroll-to-top at all, so it can no longer disturb an open restore by that route. The case is kept, because it asserts the OUTCOME (a frame swap must never move a restore in progress) rather than the mechanism, and the outcome has to hold however the internals move. It is now defended twice, by #1429's guard and by the restore window. Only the comment changes; the assertions are untouched and still pass. Re #1428. * refactor: let the browser own the Back/Forward scroll restore One writer instead of two. The router no longer replays the snapshot's offset on a popstate restore; under scrollRestoration 'auto' the browser replays the offset IT recorded, against a document the reservation holds at its recorded height, so the UA's write is simply correct and the router's was redundant. This is Next's and Remix 3's model. Neither scrolls on a traverse (Next's restore reducer sets scrollRef: null and its scroll handler bails; Remix 3 gates its only scrollTo on isNewEntry). Turbo is single-writer too but the other way round, taking 'manual' and replaying itself, which is exactly the choice that costs it the iOS gesture preview and is the bug this PR started from. An earlier gate concluded the opposite and it was WRONG. Disabling the router's write reds 8 cases, so I read that as the browser being unable to carry the restore. It was the fixture: 22 of 23 cases injected the offset into the snapshot cache while the page sat at 0, so the browser had recorded 0 for those entries and had nothing real to replay. The fixture now scrolls before pushing the next entry, which is what a reader does, and with a realistic recording all 23 pass with the router's write removed, on all three engines. The whole routing suite passes too. What stays, each re-gated by counterfactual under the new design rather than assumed: - the height reservation, without which 4 cases red per engine (it is what makes the UA's replay land on a document that can hold the offset); - the anchoring window, without which 11 to 12 cases red per engine (content still SHIFTS above the viewport within a constant height as the restored components render, and anchoring would add that shift to the offset the UA just replayed); - the window's write-back, which now guards the restore's span against a programmatic intruder rather than against the UA. The #601 restore assertion is re-pointed rather than deleted: it asserted the router's instant scrollTo form, and the router no longer writes. That guarantee survives and strengthens, since native scroll restoration is not a scrolling API call and `scroll-behavior: smooth` cannot animate it. The forward-nav half of #601 is a separate write and keeps its own assertions. Re #1428. * docs: the browser owns the Back/Forward restore The scroll-restoration docs described the two-writer arrangement that the previous commit deleted: the browser replaying a frame after the router, with the restore window absorbing the difference. There is one writer now, and it is the browser. All three surfaces that describe the mechanism now say so, and each keeps the part authors actually need: do not set scrollRestoration to 'manual' yourself, because that suppresses the per-entry recording the iOS gesture preview is composed from. Turbo is named precisely rather than loosely. It is single-writer too, but its writer is the APP, and that is exactly the choice that costs it the blank preview, so "single writer" alone does not distinguish the two designs. Next and Remix 3 are the browser-owned precedent WebJs now matches. AGENTS.md needs no change: it says only that scroll is restored on back/forward, which was and remains true, and never described the mechanism. Re #1428. * test: wait for the restore to land before the reader interrupts it CI caught a real ordering difference this test had stopped describing. It was written against the two-writer design, where the router replayed the offset SYNCHRONOUSLY inside the popstate handler, so "input on the next frame" was unambiguously after the restore. The browser owns the restore now and its replay arrives a frame or so later, so a reader modelled as input-on-the-next-frame can outrun the restore itself. The scroll then lands before the replay does, the replay overwrites it, and the case reads as the router fighting a reader when nothing of the sort happened. It now polls for the restore to land before interrupting, rather than waiting a fixed number of frames, so the case does not encode one engine's replay latency. That is also the honest statement of the property: a reader can only take over once the page has actually come back, which is exactly what the on-device cell exercises. Reproduced only on CI's Chromium and never across six local full-suite runs, which is why it survived to CI. Green three times on all three engines after the fix. Note for anyone reading CI on this branch: the Bun matrix job is ALSO red, and it is red on origin/main at 5268785 too, with the same stack overflow in server-side form-action attribute serialization. It is unrelated to this PR and predates it. Re #1428. * test: scope the cyclic-array case to engines with a working join guard Bun 1.4.0 regressed Array.prototype.join's cycle guard, which ECMA-262 requires. Six lines, no framework involved: const a = []; a.push(a); String(a) node 26 "" bun 1.3.14 "" bun 1.4.0 RangeError: Maximum call stack size exceeded CI installs bun-version: latest, so it got 1.4.0 and this assertion, which exists to pin that a self-referential array renders rather than overflowing, started failing. The same job is red on origin/main at 5268785 for the same reason, so it is neither new nor caused by this branch. Scoped rather than worked around. The alternative is a cycle-safe stringify on the per-attribute SSR hot path, which is real cost carried forever for someone else's bug, and the case is only reachable by deliberately building a self-referential array, so nothing an app does hits it. The skip is keyed to the BEHAVIOUR rather than to a version, so the assertion stays live on every spec-compliant engine and returns on Bun automatically once the regression is fixed. Verified on node 26, bun 1.3.14, and bun 1.4.0. * fix: apply the review findings, and bound the restore write-back Thirteen findings from the review round. The two that were defects: The router now writes history.scrollRestoration = 'auto' EXPLICITLY on enable rather than relying on it being the default. The restore has no writer of its own any more, so an app that had set 'manual' (the first line of most ported scroll-restoration recipes, and what this router did until #1428) would have got NO Back restore at all: the UA replays nothing, the reservation prevents the clamp that would fire a scroll event, and the write-back is scroll-event-driven so it never runs. Prose in the docs cannot prevent that; the line can. And the suite's fixture options were dead. `injectOffset` was read but never passed, `uaRecords` was passed but never read, so the case the PR presented as the single-writer proof was configured identically to the default and proved nothing extra. The option is deleted and the case now states what it actually covers. The write-back is now armed for 250ms rather than for the restore window's whole life. It reconciles ONE event, the UA's replay landing about a frame after the popstate handler, and standing guard for up to two seconds would also revert legitimate programmatic scrolls in that span: a component's scrollIntoView() during upgrade, an autofocus on a below-fold control, or find-in-page from the browser chrome, which fires no page keydown and so does not close the window the way a key press would. Bounded by time rather than by a correction count, and that distinction was earned: a single-shot bound (the first thing tried) can be spent on an unrelated scroll event arriving before the UA's replay, leaving the stale replay uncorrected. It reproduced on Firefox about one run in three. Also removed a test added earlier in this round that modelled a snapshot offset diverging from the UA's recording. That divergence cannot occur (the router snapshots the offset at the same navigation the UA records it), and the case only passed when an incidental scroll event happened to fire, so it was flaky by construction. The deterministic write-back case covers the mechanism. The rest were comments and docs that outlived the code they described: the fetch-apply branch comments asserting a router write that is gone and a UA ordering that is backwards; the write-back's rationale citing the removed Navigation API interception; the teardown comment naming the deleted catch-up; the cache-miss claim that the UA landing its offset is "strictly better than top" (it is not guaranteed to be, and the trade is now stated); a leftover A/B-lever section header; two adjacent skill bullets giving opposite answers about who restores scroll; and a docs claim that the back/forward restore is forced behavior:'instant' by the router, which no longer writes it. Plus the missing JSDoc on suppressScrollAnchoring's two new params and a Snapshot typedef violation in the legacy-string branch. Verified: 220 routing browser tests green on Chromium, Firefox and WebKit across four consecutive runs; 233 unit; disabling the write-back still reds 5 cases, so it remains load-bearing. Re #1428. * test: scope the other two cyclic-array cases for Bun 1.4.0 The first pass at this fixed only test/bun/form-action-guard.mjs, which is the file CI happened to name, and did not grep for the same assertion elsewhere. Two more test files build a self-referential array and assert the render survives it, and both are in the Bun matrix: packages/core/test/rendering/form-action-attr-guard.test.js packages/core/test/rendering/form-action-attr-guard-client.test.js Same cause: Bun 1.4.0 regressed Array.prototype.join's cycle guard, so `String(a)` throws RangeError for `const a = []; a.push(a)`. Node and Bun 1.3.14 both return ''. Same treatment: keyed to the behaviour rather than to a version, so each returns automatically once the engine is fixed. Verified by running the WHOLE matrix the way CI does, on 1.4.0 rather than on the 1.3.14 that could not see the bug: BUN=/tmp/bun140/bin/bun node scripts/run-bun-tests.js -> 331 pass, 2 fail Both remaining failures are the documented linked-worktree artifacts (the asset() ?v= case and test/bun/listener.test.mjs), which fail identically at origin/main in this worktree and pass in CI. The lesson, since it cost a round trip: a CI job reports the first file that fails, not every file with the defect. Fixing what the log names and pushing is how a two-instance bug becomes two red builds. * fix: put the app's scrollRestoration back, and make its guard discriminating Round two of review, and its two real findings were both defects the round-one fixes introduced. The router forces scrollRestoration to 'auto' (the restore is the browser's now, and 'manual' means no restore at all), but it did so WITHOUT saving what the app had. So `disableClientRouter()`, the documented runtime opt-out, left an app that had its own popstate restoration stranded on 'auto' forever, double-restoring with no way to detect why. The value is saved at enable and put back on disable, the same contract the anchoring window and the height reservation keep for the inline styles they touch. And the test guarding that write was vacuous: its mock started at 'auto', so the assertion passed whether the router wrote 'auto' or wrote nothing. It even carried a title asserting the opposite of the implementation. Seeded with 'manual' now, and the counterfactual confirms it: deleting the write reds both halves. Cache-miss popstate is deterministic again. The handler scrolls to top, but the browser replays its own recorded offset a frame later, measured against the OUTGOING document and landing before the fetched content arrives, and `fetchAndApply` skips its scroll block on `recordHistory: false`, so nothing corrected it. The fallback is now re-asserted after the response commits, guarded on still being the active navigation. Under the old 'manual' mode this path was deterministic; that property is restored rather than traded away. Two smaller mechanism fixes. The height reservation now supersedes a held one BEFORE its own height guard, so a restore with no recorded height (a legacy string snapshot) cannot leave the previous page's min-height pinned until the ceiling. And the reservation is released two frames AFTER the anchoring window rather than in the same tick, because the revalidation's swap re-inserts short markup and dropping the height alongside anchoring can clamp the reader down and then anchor the regrowth on top. The write-back comment claimed a protection it does not provide. A component's scrollIntoView() or an autofocus during upgrade runs inside the 250ms arming span and IS reverted. That is the deliberate precedence on a Back (the reader asked for the page they left), but the comment asserted the opposite, so it now states the real trade and what the bound actually buys. Rest were drift: comments still naming the deleted catch-up, a test comment citing the removed Navigation API interception, and two doc surfaces saying the router "leaves scrollRestoration at its default" when it now writes it and overrides the app. Verified: 220 routing browser tests green on three engines across three consecutive runs; full browser suite green; 233 unit; 4461/4468 node (six known worktree artifacts); e2e nested-layout 2/2 and form-submission-and-race 8/8; webjs check clean on all three apps; and the Bun matrix on 1.4.0 at 331 pass with only the two documented worktree artifacts. Re #1428. * fix: defer the cache-miss re-assert past the UA's replay Inline review of the previous commit found its determinism fix incomplete. The re-assert ran synchronously after fetchAndApply resolved, which wins only when the fetch is slower than the UA's replay. A popstate cache-miss CAN consume a prefetched entry (GET, no body, no refresh, no noPrefetch), and a warmed entry resolves the whole fetch-and-apply inside the popstate task, so the re-assert landed in that same task and the UA's replay a frame later overwrote it, exactly the ordering the fix claimed to correct. Deferred two frames instead, which is past the replay on every tested engine whichever path resolved the fetch, with the active-navigation guard moved INSIDE the deferred callback so a superseded miss never scrolls the page that replaced it. Coverage note, stated rather than hidden: no browser test drives the cache-miss-plus-prefetch-hit combination, so this ordering is covered by the reasoning above and the suites' absence of regression, not by a dedicated case. Building that fixture needs a warmed prefetch keyed to a back-entry URL and was judged not worth a new rig in this PR. Re #1428.
Closes #1406
The client router recorded a forward navigation's history entry after it swapped the DOM. WebKit binds a same-document
pushStateentry's back-forward gesture snapshot to the page state at the moment the entry is recorded, so the entry for the outgoing url was finalized against the incoming document at a scroll offset the browser had already clamped to that document's height. On a real iPhone the edge back-swipe then previews a page that never existed and renders blank for the whole gesture, which is what a reader sees on gallery.webjs.dev every time they scroll the index, open a demo, and swipe back.The push now rides into
applySwapas a commit-time callback and fires at each commit point, immediately before the DOM mutation. That is Turbo Drive's ordering (PageView.renderPagecallsvisit.changeHistory()ahead ofthis.render(renderer)).Why a callback rather than moving the line
Moving the single
history.pushStateline above theapplySwapcall would fix the measured defect and is the smaller diff, but it silently changes three paths thatfetch-apply.jsdocuments as deliberately unchanged.applySwapreturns'none'after doing work on a deploy mismatch, an integrity degradation, and a frame-missing response, and those paths record history today via the fall-through. A blanket push ahead of the call would record the entry before those decisions instead of after them, on paths nothing here measured.The thunk is one-shot, exactly like Turbo's
historyChangedguard, andfetchAndApplystill calls it on the way out, so every non-committing path behaves byte for byte as it does today. This shape is also already in the file for the same reason:ingestSeedsis a lazily-invoked thunk called at each commit point because the function can still throw the response away after parsing it.Test plan
All green at head
a430efbb, and all 10 CI checks pass on that exact commit.packages/core/test/routing/router-client.test.js: 219 pass. Four new: the callback fires before the mutation on a committing swap, does not fire on either path that commits nothing, and the caller's fall-through still records history there.packages/core/test/routing/browser/nav-history-before-swap.test.js: the headline assertion, that the outgoing DOM and the outgoingscrollYare both still live at thepushStatecall. Full browser suite 75 files green on Chromium, Firefox, and WebKit.scrollYclamped from 800 to 0.test/e2e/form-submission-and-race.test.mjs: 8 pass, including the dogfood: back-button scroll restores ~763px too low on pages that grow after swap #1310 back-button restore guard. The new sibling asserts the rendered H1 at the push.Cardrather thanButton. The first draft of this test assertedlocation.pathname, which is tautological inside apushStatewrapper and passed with the fix reverted; the H1 is the real discriminator.test/e2e/e2e.test.mjs: 94 pass.test/bun/listener-overhead.mjsassertion fails in a linked worktree and passes in the primary checkout at the same commit; it asserts an action redirect carrying the seen client IP, a server-side path this browser-only change never reaches.( cd gallery && npx webjs check )and( cd website && npx webjs check )clean; website boots in prod mode with 200 on/,/docs/client-router,/ui,/ui/button, 50 modulepreload hints and none broken.Not verified, and it cannot be from here: the on-device iOS check. The gesture preview only exists on a real iPhone, so the acceptance criterion asking for it is still open.
One intermittent, unreproduced: a single browser run reported 3 Firefox failures. Four subsequent full runs were green (872 passed, 0 failed) and CI's browser job passes. I could not identify the tests, because that run's log was truncated to its tail, so it is recorded as unreproduced rather than diagnosed.
Doc surfaces
The first commit bypassed the doc gate on the reasoning that no documented claim could contradict an ordering fix with no observable behaviour change. That was wrong, and the review rounds are the record of it: four surfaces describe the swap SEQUENCE, and every one of them listed
pushStatelast.Updated here:
website/app/docs/client-router/page.ts: the "How it works" list now walks the real order (scan and decide, push, head merge, swap, upgrade and scripts,webjs:navigate), and the streaming paragraph no longer says the shell is applied before the URL advances.website/app/docs/suspense/page.ts: same streaming correction.packages/core/src/router-client/constants.js: the router contract docstring, which separates the tier decision from the commit that applies it.blog/client-router-turbo-drive-style.md: a live page at/blog/client-router-turbo-drive-stylethat transcribes that docstring, so it was publishing an ordering the router no longer has. Its stale pre-split module path is corrected in the same paragraph.The lesson, recorded because the bypass looked reasonable at the time: "no observable behaviour change" is not the same test as "no documented claim changes". An ordering fix has no observable change by construction, and ordering is exactly what a how-it-works doc enumerates.
MCP, editor plugins, scaffold templates, marketing copy, and the
.agents/skill references: N/A, verified none of them describes this sequence.