fix(virtual-core): keep a travelling smooth scroll alive through a prepend - #1248
Merged
piecyk merged 4 commits intoSep 11, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds a smooth-prepend virtualized list example, registers it in the Vite build, and adds a Playwright regression test for scrolling interrupted by prepended history. ChangesSmooth prepend end-to-end scenario
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 ba2c744
☁️ 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 670-684: The anchorResolved handling must not clear an active
smooth scroll. Update the scroll-state retirement logic near reconcileScroll so
anchor correction only retires stale programmatic targets after the destination
has been reached and the user has moved away, while preserving in-flight
scrollToIndex({ behavior: "smooth" }) state through prepend/setOptions updates;
add regression coverage for prepending during an active smooth scroll.
🪄 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 Plus
Run ID: 28b4746c-99bc-4c31-84d9-5e8c279082c0
📒 Files selected for processing (3)
.changeset/lucky-pandas-listen.mdpackages/virtual-core/src/index.tspackages/virtual-core/tests/index.test.ts
piecyk
force-pushed
the
fix/anchor-clobbered-by-stale-scroll-state
branch
from
July 31, 2026 10:59
4bf3ab9 to
709d666
Compare
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 1132-1142: Update the reachedTarget re-arm logic in the scroll
target handling block to use the same approximately 1.01 measurement-settling
tolerance as the retirement check, rather than the viewport distance. Re-arm
this.scrollState.reachedTarget whenever the target changes beyond that
tolerance, including changes smaller than a viewport but larger than the
settling threshold; preserve the existing behavior for minor measurement nudges.
🪄 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 Plus
Run ID: ae39a292-8f42-4251-a538-6cd0c1e82b54
📒 Files selected for processing (3)
.changeset/lucky-pandas-listen.mdpackages/virtual-core/src/index.tspackages/virtual-core/tests/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/lucky-pandas-listen.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/react-virtual/e2e/app/smooth-prepend/main.tsx`:
- Line 34: Remove the unnecessary non-null assertions from the array accesses in
the getItemKey callback and the corresponding access at the second reported
location, preserving their existing indexing and key behavior.
In `@packages/react-virtual/e2e/app/test/smooth-prepend.spec.ts`:
- Around line 42-45: Remove the top-level test.fail() call before the
smooth-scroll test declaration; leave the test('a prepend mid-flight does not
abandon a smooth scrollToIndex', ...) registration and body unchanged.
🪄 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 Plus
Run ID: 512dd6ae-228e-488e-95e4-7b06c3040ef4
📒 Files selected for processing (4)
packages/react-virtual/e2e/app/smooth-prepend/index.htmlpackages/react-virtual/e2e/app/smooth-prepend/main.tsxpackages/react-virtual/e2e/app/test/smooth-prepend.spec.tspackages/react-virtual/e2e/app/vite.config.ts
| count: messages.length, | ||
| getScrollElement: () => parentRef.current, | ||
| estimateSize: () => 50, | ||
| getItemKey: (index) => messages[index]!.id, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unnecessary non-null assertions.
ESLint reports errors at Line 34 and Line 88. Remove ! from both array accesses.
Proposed fix
- getItemKey: (index) => messages[index]!.id,
+ getItemKey: (index) => messages[index].id,
...
- const message = messages[item.index]!
+ const message = messages[item.index]Also applies to: 88-88
🧰 Tools
🪛 ESLint
[error] 34-34: This assertion is unnecessary since it does not change the type of the expression.
(@typescript-eslint/no-unnecessary-type-assertion)
🤖 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/react-virtual/e2e/app/smooth-prepend/main.tsx` at line 34, Remove
the unnecessary non-null assertions from the array accesses in the getItemKey
callback and the corresponding access at the second reported location,
preserving their existing indexing and key behavior.
Source: Linters/SAST tools
piecyk
force-pushed
the
fix/anchor-clobbered-by-stale-scroll-state
branch
from
July 31, 2026 12:58
9f9632b to
b96bba2
Compare
…epend With anchorTo: 'end', the prepend anchor sync in _willUpdate writes scrollTop instantly. That cancels a smooth scrollToIndex the browser is still animating, and Chromium drops a smooth request re-issued in the frame right after such a cancel, so re-driving from reconcileScroll cannot recover it: "jump to the oldest message" died halfway whenever history streamed in mid-animation. Skip the anchor DOM write while a smooth programmatic scroll is still travelling. Its target is index-based and recomputes against the new layout every reconcile frame, so the animation simply continues; the next scroll event re-syncs the tracked offset. A smooth scroll that has already landed but has not been retired yet still receives the sync, so a reader's position is preserved in that window. Two core regression tests cover both branches, and the react-virtual smooth-prepend e2e is promoted from an expected failure to a regular guard. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
piecyk
changed the title
4 tasks
Merged
4 tasks
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
1 participant
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.
With
anchorTo: 'end', a smoothscrollToIndexthat is still travelling when history is prepended dies partway. "Jump to the oldest message" strands the reader mid-list whenever older messages stream in during the animation.🎯 Changes
Root cause. The prepend anchor sync in
_willUpdatewritesscrollTopinstantly to keep the reader's row in place. That write cancels the browser's in-flight smooth animation.reconcileScrollcannot recover it: it only re-drives when the target moves, and with an index-based target the destination is unchanged. Re-driving unconditionally does not help either. Instrumented runs show Chromium silently drops a smooth request issued in the frame right after an instant write cancelled its animation, while the same request issued later completes.Fix. Skip the anchor DOM write while a smooth programmatic scroll is still travelling. The target recomputes against the new layout on every reconcile frame, so the animation simply continues; the next scroll event re-syncs the tracked offset. A smooth scroll that has already landed but not yet been retired still receives the sync, so a reader's position is preserved in that window.
Tests.
main, the landed case guards the refinement.smooth-prepende2e page and spec, which reproduce the strand in a real browser, promoted fromtest.fail()to a regular guard.Verified locally: virtual-core 164/164, react e2e 37/37 (the new spec 3/3 on repeat), marko chat/chat-pretext/option-gates 26 passed, angular e2e 14/14, tsc/eslint/prettier clean.
Not in scope. A finished programmatic scroll can stay live for up to the 5 s reconcile safety valve if the user scrolls away before it is retired; a prepend inside that window now skips the sync instead of re-firing the old target. Retiring stalled scrolls early would risk dynamic-size
scrollToIndex, which relies on the loop re-asserting as measurements arrive, so it is left for a follow-up.✅ Checklist
pnpm run test:pr.🚀 Release Impact
🤖 Generated with Claude Code