fix(virtual-core): eagerly adjust scrollOffset on prepend to prevent jump#1176
fix(virtual-core): eagerly adjust scrollOffset on prepend to prevent jump#1176piecyk wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe virtualizer eagerly corrects scrollOffset in setOptions when an append/prepend anchor is resolvable, a changeset entry is added, and _willUpdate now directly syncs the browser scroll position for pending anchors instead of computing/applying a deferred anchor delta; tests updated accordingly. ChangesScroll Anchor Lifecycle Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/virtual-core/tests/index.test.tsParsing error: "parserOptions.project" has been provided for Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 5b972c5
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/virtual-core/src/index.ts`:
- Around line 823-834: Tests expect numeric options.adjustments but
_scrollToOffset now forwards undefined for the _willUpdate anchor sync path;
update the two assertions in packages/virtual-core/tests/index.test.ts that
reference anchorTo:end to expect undefined for options.adjustments instead of
100 and 70 (specifically the tests "anchorTo:end keeps visible content stable
when older items are prepended" and "anchorTo:end keeps a pinned streaming
message pinned as it grows"); leave the iOS deferred-flush test that asserts 50
unchanged because it exercises the iOS accumulator path, and ensure references
to _scrollToOffset and scrollToFn are used when reviewing the behavior change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f21eedb-e3bd-4e6c-a809-62dc0e290649
📒 Files selected for processing (1)
packages/virtual-core/src/index.ts
piecyk
force-pushed
the
damian/fix/eager-scroll-anchor-on-prepend
branch
from
f2ec547 to
5b972c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/virtual-core/tests/index.test.ts (1)
2267-2280: ⚡ Quick winAssert the eager correction before
_willUpdate().This only proves the later DOM sync. The regression here was the one-frame wrong range during render, so it would be better to assert
scrollOffsetorgetVirtualItems()immediately aftersetOptions()and before_willUpdate()runs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/virtual-core/tests/index.test.ts` around lines 2267 - 2280, The test currently only checks the post-DOM-sync scrollTo call; add an assertion that verifies the eager correction happened synchronously before the internal update by checking the virtualizer's scrollOffset or getVirtualItems() immediately after calling setMessages (and before _willUpdate() runs). Specifically, after calling setMessages([{ id: 'm--2' }, { id: 'm--1' }, ...messages]) assert virtualizer.scrollOffset (or virtualizer.getVirtualItems()) has the expected value/range to prove the one-frame correction occurred, and keep the existing scrollToFn assertions for the later DOM sync; locate this in the same test around the setMessages call and place the new assertion before any code that triggers or waits for _willUpdate().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/virtual-core/src/index.ts`:
- Around line 830-838: The direct call to _scrollToOffset bypasses the iOS
deferral path and can be cancelled by a prepend-triggered anchor sync; instead,
gate this DOM sync through the same iOS deferral used elsewhere or use the
existing applyScrollAdjustment path. Replace the direct
this._scrollToOffset(this.getScrollOffset(), ...) with the code path that honors
_iosDeferredAdjustment (i.e., call applyScrollAdjustment / the helper that
defers/queues adjustments when _iosDeferredAdjustment is set) so the scroll
write is deferred during iOS touch/momentum; ensure you still pass the same
offset from getScrollOffset() and preserve the adjustments/behavior options when
delegating.
---
Nitpick comments:
In `@packages/virtual-core/tests/index.test.ts`:
- Around line 2267-2280: The test currently only checks the post-DOM-sync
scrollTo call; add an assertion that verifies the eager correction happened
synchronously before the internal update by checking the virtualizer's
scrollOffset or getVirtualItems() immediately after calling setMessages (and
before _willUpdate() runs). Specifically, after calling setMessages([{ id:
'm--2' }, { id: 'm--1' }, ...messages]) assert virtualizer.scrollOffset (or
virtualizer.getVirtualItems()) has the expected value/range to prove the
one-frame correction occurred, and keep the existing scrollToFn assertions for
the later DOM sync; locate this in the same test around the setMessages call and
place the new assertion before any code that triggers or waits for
_willUpdate().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7da3ba74-d701-4a3a-962b-68f55dc9942b
📒 Files selected for processing (2)
packages/virtual-core/src/index.tspackages/virtual-core/tests/index.test.ts
| if (key !== null && !followOnAppend) { | ||
| // scrollOffset was eagerly adjusted in setOptions so the | ||
| // virtualizer already computed the correct range during render. | ||
| // Now sync the browser's actual scroll position to match. | ||
| // Skip when followOnAppend is set — scrollToEnd will handle it. | ||
| this._scrollToOffset(this.getScrollOffset(), { | ||
| adjustments: undefined, | ||
| behavior: undefined, | ||
| }) |
There was a problem hiding this comment.
Preserve the iOS deferral path for pending anchor sync.
This direct _scrollToOffset call skips applyScrollAdjustment, so prepend-triggered anchor sync can still write scrollTop during iOS touch/momentum and cancel the in-flight scroll. The eager logical scrollOffset fix is good, but the DOM sync still needs the same _iosDeferredAdjustment gating as the other mid-scroll corrections.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/virtual-core/src/index.ts` around lines 830 - 838, The direct call
to _scrollToOffset bypasses the iOS deferral path and can be cancelled by a
prepend-triggered anchor sync; instead, gate this DOM sync through the same iOS
deferral used elsewhere or use the existing applyScrollAdjustment path. Replace
the direct this._scrollToOffset(this.getScrollOffset(), ...) with the code path
that honors _iosDeferredAdjustment (i.e., call applyScrollAdjustment / the
helper that defers/queues adjustments when _iosDeferredAdjustment is set) so the
scroll write is deferred during iOS touch/momentum; ensure you still pass the
same offset from getScrollOffset() and preserve the adjustments/behavior options
when delegating.
…jump When items are prepended with anchorTo: 'end' and dynamic sizes, the virtualizer would compute the wrong visible range for one frame (using stale estimate-based positions) and then correct in the next frame via _willUpdate, producing a visible "jump". Fix by eagerly adjusting scrollOffset in setOptions during the render pass so calculateRange/getVirtualItems return the correct items immediately. _willUpdate then only syncs the browser's actual scroll position to match.
piecyk
force-pushed
the
damian/fix/eager-scroll-anchor-on-prepend
branch
from
5b972c5 to
2b35365
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/virtual-core/src/index.ts (1)
830-838:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve the iOS deferred-scroll path for this DOM sync.
This direct
_scrollToOffsetwrite bypassesapplyScrollAdjustment'sisIOSWebKit()gate. If a prepend lands during touch or momentum scrolling, this path still writesscrollTopimmediately and cancels the native scroll on iOS, even though the logicalscrollOffsetwas already fixed eagerly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/virtual-core/src/index.ts` around lines 830 - 838, The direct call to this._scrollToOffset bypasses applyScrollAdjustment's isIOSWebKit() logic and can cancel native iOS momentum; instead of calling _scrollToOffset directly when key !== null && !followOnAppend, route the synchronization through applyScrollAdjustment (or its equivalent helper) so the iOS-specific deferred-scroll gate is respected; use this.getScrollOffset() as the target offset and pass through the adjustments/behavior params (or undefined) to applyScrollAdjustment so iOS WebKit will defer the DOM write while other platforms still sync immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/virtual-core/src/index.ts`:
- Around line 604-615: The code may read stale measurement data when computing
newOffset via newMeasurements[idx].start; before using newMeasurements[idx]
ensure measurements are rebuilt for the current key ordering: either call your
measurement invalidation/rebuild helper (e.g., this.invalidateMeasurements() or
this.rebuildMeasurements()) or include the item key identity in the measurement
memo so getMeasurements() returns a fresh array for the current getItemKey
order; specifically, after computing idx using getItemKey and before accessing
newMeasurements[idx].start, force a measurements refresh (or verify
newMeasurements[idx] corresponds to getItemKey(idx) and rebuild if it does not)
so this.scrollOffset is computed from up-to-date measurements.
---
Duplicate comments:
In `@packages/virtual-core/src/index.ts`:
- Around line 830-838: The direct call to this._scrollToOffset bypasses
applyScrollAdjustment's isIOSWebKit() logic and can cancel native iOS momentum;
instead of calling _scrollToOffset directly when key !== null &&
!followOnAppend, route the synchronization through applyScrollAdjustment (or its
equivalent helper) so the iOS-specific deferred-scroll gate is respected; use
this.getScrollOffset() as the target offset and pass through the
adjustments/behavior params (or undefined) to applyScrollAdjustment so iOS
WebKit will defer the DOM write while other platforms still sync immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fad320ff-17ef-4872-9c13-0986c5a892c7
📒 Files selected for processing (3)
.changeset/eager-scroll-anchor.mdpackages/virtual-core/src/index.tspackages/virtual-core/tests/index.test.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/eager-scroll-anchor.md
| const newMeasurements = this.getMeasurements() | ||
| const { count, getItemKey } = this.options | ||
| let idx = 0 | ||
| while (idx < count && getItemKey(idx) !== anchorKey) { | ||
| idx++ | ||
| } | ||
| if (idx < count) { | ||
| const anchorItem = newMeasurements[idx] | ||
| if (anchorItem) { | ||
| const newOffset = anchorItem.start + anchorOffset | ||
| if (newOffset !== this.scrollOffset) { | ||
| this.scrollOffset = newOffset |
There was a problem hiding this comment.
Force a fresh measurement rebuild before reading anchorItem.start.
If count stays the same and the caller keeps a stable getItemKey reference, getMeasurements() can still be serving the previous layout here. idx is found from the new key order, but newMeasurements[idx].start can still belong to the old item/order, so the eager correction picks the wrong offset for prepend+trim or other edge-key swaps with dynamic sizes. Invalidate measurements on edge-key changes, or include key identity in the measurement memo deps before using newMeasurements[idx].
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/virtual-core/src/index.ts` around lines 604 - 615, The code may read
stale measurement data when computing newOffset via newMeasurements[idx].start;
before using newMeasurements[idx] ensure measurements are rebuilt for the
current key ordering: either call your measurement invalidation/rebuild helper
(e.g., this.invalidateMeasurements() or this.rebuildMeasurements()) or include
the item key identity in the measurement memo so getMeasurements() returns a
fresh array for the current getItemKey order; specifically, after computing idx
using getItemKey and before accessing newMeasurements[idx].start, force a
measurements refresh (or verify newMeasurements[idx] corresponds to
getItemKey(idx) and rebuild if it does not) so this.scrollOffset is computed
from up-to-date measurements.
1 participant
When items are prepended with anchorTo: 'end' and dynamic sizes, the virtualizer would compute the wrong visible range for one frame (using stale estimate-based positions) and then correct in the next frame via _willUpdate, producing a visible "jump".
Fix by eagerly adjusting scrollOffset in setOptions during the render pass so calculateRange/getVirtualItems return the correct items immediately. _willUpdate then only syncs the browser's actual scroll position to match.
🎯 Changes
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit