fix: restore scroll position reliably after a hot-swap page reload - #25224
fix: restore scroll position reliably after a hot-swap page reload#25224Artur- wants to merge 3 commits into
Conversation
The positions captured before a full page reload were applied once, in a single animation frame after the Flow clients reported being idle. Idle only means that the initial UIDL has been processed, so on a slow load the positions were applied to a view that had not rendered yet: scrolling is clamped to the currently scrollable area, and the scroll containers were not in the DOM at all. The page was left at the top with no further attempt made. Keep re-applying each position until it takes effect instead, and give up on the ones that never become reachable after a timeout or once the user scrolls. The browser restores the scroll position of a reloaded page on its own, which is what made the restore look like it worked: it only ran in the cases where the browser had already given up. Take restoration over from the browser for the reload so that the two do not compete. The stored snapshot is now kept until restoring has settled, so that a page load interrupted by another reload does not lose it, and is ignored when it is older than the load it was written for. The test waited for an element that the page the reload was triggered on already had, so it could assert against the document being replaced, and it reported a timeout on a lambda instead of the scroll positions that were expected.
|
Disclaimer: the output below is AI generated - output of the FindingsThe core mechanism — verify-by-read-back with retry, plus taking 1.
|
|
🤖 AI-generated Adversarial Review: fix: restore scroll position reliably after a hot-swap page reloadThe verify-by-read-back approach is right, and I'm not repeating the five findings already on the PR. What's left are mostly consequences of two design choices this PR makes: deferring snapshot removal until settle, and driving the retry loop from 🔴 HighStale snapshot re-applied to an unrelated page load
Failure scenario: hot-swap reload at t=0 → new document loads at t=0.5 s, restore loop running → developer clicks a link or edits the URL at t=1 s → new document, dev tools Fix: 🟠 MediumThe retry loop does not run in a background tab, but its deadline does
That is the dominant hot-swap scenario: the developer saves in their IDE with the browser behind it. A single apply after refocus often does land, since the DOM has had time to render — but any view whose content is itself rAF-driven (a A restored position can be undone after it is spliced out of
Both apply paths honor
🟡 Low / NitpicksA third path to the already-reported stuck Nothing inside the rAF callback is guarded, and Shadow-DOM scroll containers are never captured
Two of the three new waits still report a lambda on timeout
No negative control in the UI-refresh test
✅ What is done well
Summary: Not mergeable as-is — the deferred snapshot removal is a regression that lets a stale snapshot hijack an unrelated page load, and it needs gating on the existing dev-tools-reload flag; the rAF/visibility and |
Keeping the stored snapshot until restoring has settled only helps if the save before the next reload does not remove it. That is exactly what it did: a reload interrupting a restore that had not landed yet captures nothing, since the page is still at the top, and the empty capture removed the snapshot. Leave a stored snapshot in place instead, and only take restoration over from the browser when there is one. Restoration was also left taken over from the browser when a reload had nothing to store, so neither mechanism restored the next load. Hand it back when there is no snapshot to apply. Waiting for idle clients and retrying shared one deadline, so clients that never report being idle used up the whole budget waiting and left a single apply attempt, which is the behaviour being replaced. Give the wait its own budget and start the retry budget when applying starts. Dragging the scrollbar produces neither a wheel nor a keydown event, so it did not count as the user scrolling, and cancelling was checked only after applying, which yanked the page back once more before stopping.
Keeping the snapshot until the restore has settled made it outlive the page load it belongs to: a document replaced while restoring, by a link or an edited URL, left a snapshot that the next load applied to a page the positions never came from. Restore only on the load the dev tools reload produced, which is what the existing triggered flag in session storage already marks, and discard the snapshot on any other load. The retry budget was wall-clock while the retries themselves run on animation frames, which a hidden document does not get. A tab in the background across the reload, which is what hot swapping from an IDE looks like, came back with the budget already spent and one attempt left. Charge the budget per frame instead, capped, so that time when nothing is being retried does not count against it. Applying a position resolved its behaviour from the computed scroll-behavior, so an application that sets it to smooth animated towards the position and read back the value it started from. Scroll explicitly instant. A position the element can reach but the browser places elsewhere, as scroll snapping does, is now taken as final rather than retried until the timeout. A position is confirmed over several frames before it is considered done, since content rendered afterwards can shrink the scrollable area and have the position clamped back down. A key that is not a usable selector no longer breaks out of the loop, which used to leave both the snapshot and the browser's scroll restoration disabled with nothing left to reset them. The two remaining waits in the test report what they saw on timeout.
|
Thanks for both rounds — all ten points were valid and are addressed in First review
Reproduced 1+2 together and 5 in a browser against the reviewed commit before fixing: the interrupted reload ended at Adversarial reviewStale snapshot on an unrelated load — agreed, this was a regression against Animation frames versus a wall-clock budget — agreed. The budget is now charged per frame and capped at 100 ms per frame, so time when no frames are delivered does not consume it. I could not reproduce an actually hidden document here (this headless Chromium keeps delivering frames to background tabs at 60 fps), so I verified the arithmetic instead by delivering frames 2 s apart: with content rendering at 12 s, the previous commit gave up at 0 and the current one restores.
Retiring on first match — a position is now confirmed over three consecutive frames before it is dropped, which covers the grow-then-shrink clamp. The positional-selector variant is real but needs the wrong element to also accept the exact offset; I left that as is rather than changing the key format in this PR.
The two waits reporting a lambda — both now fail with what they saw. Two I read differentlyNegative control in the UI-refresh test. The refresh test waits for the attach id to change, so the view is re-instantiated and both scroll containers are new elements at Shadow-DOM containers — agreed it is a real gap, and agreed it is pre-existing and out of scope here. |
|
|
@claude please review |
|
Claude finished @Artur-'s task in 2m 26s —— View job Review: restore scroll position reliably after a hot-swap page reloadI read the current state of all three files and traced the flows that the two earlier review rounds flagged. This is a re-review — the ten prior findings are addressed in Prior findings — verified fixed
I also re-traced the interrupted-reload → manual-navigation sequence end to end: reload #1 consumes Residual observations (minor — not blockers)
AssessmentThe verify-by-read-back-with-retry approach plus taking |
2 participants
The positions captured before a full page reload were applied once, in a single animation frame after the Flow clients reported being idle. Idle only means that the initial UIDL has been processed, so on a slow load the positions were applied to a view that had not rendered yet: scrolling is clamped to the currently scrollable area, and the scroll containers were not in the DOM at all. The page was left at the top with no further attempt made. Keep re-applying each position until it takes effect instead, and give up on the ones that never become reachable after a timeout or once the user scrolls.
The browser restores the scroll position of a reloaded page on its own, which is what made the restore look like it worked: it only ran in the cases where the browser had already given up. Take restoration over from the browser for the reload so that the two do not compete.
The stored snapshot is now kept until restoring has settled, so that a page load interrupted by another reload does not lose it, and is ignored when it is older than the load it was written for.
The test waited for an element that the page the reload was triggered on already had, so it could assert against the document being replaced, and it reported a timeout on a lambda instead of the scroll positions that were expected.