Simplify scroll restoration with shared ScrollRef on CacheNode#91348
Open
acdlite wants to merge 1 commit intovercel:canaryfrom
Open
Simplify scroll restoration with shared ScrollRef on CacheNode#91348acdlite wants to merge 1 commit intovercel:canaryfrom
acdlite wants to merge 1 commit intovercel:canaryfrom
Conversation
Collaborator
Tests Passed |
Contributor
Author
|
Note: the diff for In both The old scroll-decision logic: const { focusAndScrollRef, segmentPath } = this.props
if (focusAndScrollRef.apply) {
if (
focusAndScrollRef.segmentPaths.length !== 0 &&
!focusAndScrollRef.segmentPaths.some((scrollRefSegmentPath) =>
segmentPath.every((segment, index) =>
matchSegment(segment, scrollRefSegmentPath[index])
)
)
) {
return
}
// ... scroll execution (unchanged) ...
focusAndScrollRef.apply = false
focusAndScrollRef.hashFragment = null
focusAndScrollRef.segmentPaths = []
}Was replaced with: const { focusAndScrollRef, cacheNode } = this.props
const scrollRef = focusAndScrollRef.scrollRef ?? cacheNode.scrollRef
if (scrollRef === null || !scrollRef.current) return
// ... scroll execution (unchanged) ...
scrollRef.current = false
focusAndScrollRef.hashFragment = nullOther changes:
Everything else in the file (the scroll execution logic inside |
2618f59 to
b9e4866
Compare
b9e4866 to
ad4256d
Compare
a1ea28c to
0090f51
Compare
The scroll system previously relied on accumulating segment paths during navigation, then matching those paths in layout-router to decide which segments should scroll. This was necessary when CacheNodes were created lazily during render, since we couldn't mark scroll targets on the nodes themselves. But now that we construct the entire CacheNode tree immediately upon navigation, we can use a much simpler approach. Each navigation creates a single shared mutable ref (ScrollRef) and assigns it to every new leaf CacheNode. When any segment's scroll handler fires, it sets the ref to false, preventing other segments from scrolling. This replaces all the segment path accumulation and matching logic with a straightforward per-node flag. The motivation for this refactor was a bug where calling refresh() from a server action would scroll the page to the top. The old path-based system had a semantic gap between null (no segments to scroll) and an empty array (scroll everything) — when a server action triggered a refresh with no new segments, the null value fell through to a code path that scrolled unconditionally. The new model avoids this entirely: refresh creates no new CacheNodes, so no ScrollRef is assigned, and nothing scrolls. There is extensive existing test coverage for scroll restoration behavior. This adds one additional test for the server action refresh bug.
0090f51 to
96fc02e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the segment-path-matching scroll system with a simpler model based on a shared mutable ScrollRef on CacheNode.
The old system accumulated segment paths during navigation and matched them in layout-router to decide which segments should scroll. This was necessary when CacheNodes were created lazily during render. Now that we construct the entire CacheNode tree immediately upon navigation, we can assign a shared ScrollRef directly to each new leaf node. When any segment scrolls, it flips the ref to false, preventing other segments from also scrolling. This removes all the segment path accumulation and matching logic.
Fixes a regression where calling
refresh()from a server action scrolled the page to the top. The old system had a semantic gap betweennull(no segments) and[](scroll everything) — a server action refresh with no new segments fell through to a path that scrolled unconditionally. The new model avoids this: refresh creates no new CacheNodes, so no ScrollRef is assigned, and nothing scrolls.Repro: https://github.com/stipsan/nextjs-refresh-regression-repro
There is extensive existing test coverage for scroll restoration behavior. This adds one additional test for the server action refresh bug.