chore: add guarded on-device levers to A/B the iOS back-swipe blank - #1430
chore: add guarded on-device levers to A/B the iOS back-swipe blank#1430vivek7405 wants to merge 3 commits into
Conversation
#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.
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.
vivek7405
left a comment
vivek7405
left a comment
There was a problem hiding this comment.
Went through the whole diff against the router's own invariants. The shape holds up: the levers are inert unless an app writes window.__webjsDiag, the yield is tied to recordHistoryNow being non-null so no background path picks up a frame of latency, and putting the yield in the caller rather than at the four swap.js commit points is the right call given applySwap is synchronous and its return value is consumed synchronously. Making it async to host a diagnostic would be a bigger change than the thing being diagnosed.
One real problem, on the ?scrolllast path, commented inline.
Two things I checked and am satisfied with. The early return after the yield sits inside the try whose finally calls clearFrameBusy, so it cannot leak a busy frame, and it matches the supersede guard directly above it. And the claim in the comment that a discard can never see a non-null thunk holds: revalidating: true is passed at exactly one call site, navigator.js:583, which passes recordHistory: false.
Worth restating what this does NOT do, because it is the part that is easy to misread from the diff: it fixes nothing. The levers exist so the device can tell us whether the snapshot is bound at the push or at a later composited frame, and until that run happens we do not know which. That is why the body references #1428 without a Closes.
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.
1 participant
Re #1428. Deliberately NOT
Closes: see "What this PR does not do" below.The iOS edge back-swipe still previews a blank page on
webjs.dev/blogand/compare, and #1410 is not the explanation. The live site serves core 0.7.51,8b90f95bis an ancestor of the release commitb15bbb40, and the fix isvisible in the served bundle as the one-shot thunk handed into
applySwaprather than a push after it. Both reported routes take the replace tier from a
scrolled index into a shorter child, which is the exact shape #1406 measured,
and
website/has noloading.{js,ts}, so the one documented partial-coveragegap does not apply either.
What #1410 never had is the on-device check. Its own body says so: "Not
verified, and it cannot be from here: the on-device iOS check ... the acceptance
criterion asking for it is still open." Everything green on it ran in Chromium,
Firefox, and headless WebKit, none of which renders a gesture preview. So the
report is the first real verification of that fix, and it says correct ordering
was necessary and is not sufficient.
The assumption still untested
#1406 assumed WebKit binds the gesture snapshot synchronously at the
pushStatecall. The competing hypothesis is that it does not:didSameDocumentNavigationForFrameposts to the UI process, which then capturesthe current compositing surface, and that IPC lands after the whole
push -> swap -> scrollTo(0,0)task. Under that hypothesis the surface capturedis the destination at offset 0, moving the push a few statements earlier changes
nothing a device can observe, and the fix needs a composited frame rather than a
reorder.
I am not implementing against that hypothesis. #1109 cost a PR and three issues
by building against an unmeasured mechanism, and #610 cost five failed
header-CSS attempts and a GPU-promotion attempt the same way. So this ships the
candidate timings behind default-off levers and lets the device decide, which is
the method that finally isolated #610.
What ships
Three levers, each a separate query param so a run moves one variable:
?rafwaits one animation frame between the push and the DOM mutation.?raf2waits two, since a single rAF can land before the commit WebKit needs.?scrolllastdefers the scroll-to-top past the frame, isolating the clampfrom the swap.
website/app/layout.tscaptures them intowindow.__webjsDiagonce on load,inline and in the head so it lands before the router boots. Captured once
because the param is gone from the URL the moment the first soft nav replaces
it.
Why the yield is in the caller, not at the four commit points
#1428 asks for the yield at all four
swap.jscommit points.applySwapissynchronous and its return value is consumed synchronously, so it cannot await,
and making it async to host a diagnostic would be a far larger change than the
thing being diagnosed.
The one-shot thunk makes that unnecessary. Calling
recordHistoryNow()infetchAndApplyfires the push for whichever commit point the swap goes on toreach and leaves that call a no-op, so every path gets the same ordering rather
than the subset I remembered to edit. It also keeps the yield off the background
paths (a revalidation, a refresh, the popstate restore), which record no entry
and so have no snapshot to compose.
Test plan
Run in a linked worktree with
packages/core/distREBUILT there, not symlinkedfrom the primary, so the e2e and dist consumers exercise this branch's bundle.
The lever is present in the built browser bundle, so a deployed run really gets
it.
packages/core/test/routing/router-client.test.jscovering the inertdefault, one lever reading true without its siblings, the
nullreturn whenthere is no rAF (the unit runner's shim, which is the case the call sites
guard), and the double-frame count.
packages/core/test/routing/browser/nav-swipe-ab-levers.test.jscarries theheadline assertion, that a frame boundary falls between the push and the
mutation under a lever and does NOT without one, plus the
?scrolllastdeferral and its supersede guard, alongside the fix: record the history entry before the swap, not after #1410 test it must not
disturb.
/,/blog,/compare,/docs/client-router,/ui,/ui/button, with no broken modulepreloadhints (50 on
/ui/button) and the lever capture present in every page.npx webjs checkclean.touches no listener, serializer, SSR dispatch, stream, or
node:*surface.The six failures, none of them from this branch
All six are linked-worktree artifacts. I checked each by restoring
packages/core/src/router-client/andwebsite/app/layout.tstoorigin/mainin this same worktree, against the same built dist, and every one still fails
there: three differential-elision assertions,
test/bun/listener.test.mjs, the Bun listener-overhead assertion, and oneasset()prod-handler assertion. The e2e failure is the same story,prefetch: hovering a FRAME link warms the subtree (#1407), which also fails atorigin/mainhere.One that WAS from this branch, and what it cost to find
Three full browser runs reported a single Firefox failure in an unrelated file,
a Back restore survives late layout growth (#1310) > the window closes once the restore is over. It passed 23 of 23 in isolation, and my first read wasthat it was load-dependent flake, on the strength of those isolated runs and of
#1410 having recorded unreproduced Firefox failures in the same suite.
That read was wrong, and the isolated runs are exactly why it was wrong. A
baseline run of the whole browser suite at
origin/mainin this worktree isGREEN, which is the check that settles it, and the two isolated-run signals
could not have. Bisecting the branch against that baseline put it in the new
test FILE rather than the source: the supersede assertion drove its second
navigation with a fetch that never settled, so a navigation stayed in flight
for the rest of the page's life holding the router's token and its own frame
state, and under full-suite concurrency that leak reached another file. Fixed
in
cf5519c0by rejectingwith an
AbortError, the shape the router already treats as a supersedednavigation. Suite green twice after.
Worth keeping the sequence in view: the two files pass when run TOGETHER, so
nothing short of the full concurrent suite reproduces it, and "passes in
isolation" was never evidence of anything.
What this PR does not do
It does not fix the bug, and it must not be read as fixing it. The deliverable
in #1428 is the on-device verdict, which needs a real iPhone, so the issue stays
open after this merges and the reference above is a plain
#1428rather than aCloses. Merging a diagnostic under aClosesis how #1410 came to be recordedas resolving a symptom it had never been tested against.
The levers are temporary. They come out, or one of them is promoted to real
behaviour, in the follow-up once the device has answered.