fix(virtual-core): adjust scroll on first measurement during backward scroll#1199
Merged
piecyk merged 1 commit intoJun 15, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe default ChangesBackward Scroll First-Measurement Compensation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 ba5d095
☁️ 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.
🧹 Nitpick comments (2)
packages/react-virtual/e2e/app/scroll-anchor/main.tsx (1)
15-17: ⚡ Quick winHarden
initialOffsetparsing againstNaNinput.Line 15–Line 17 converts a query param with
Number(...)and can passNaNintoinitialOffset. Guarding with a finite check makes this fixture resilient to malformed URLs during local/debug runs.Proposed patch
- const initialOffset = Number( - new URLSearchParams(window.location.search).get('initialOffset') ?? 0, - ) + const rawInitialOffset = new URLSearchParams(window.location.search).get( + 'initialOffset', + ) + const parsedInitialOffset = + rawInitialOffset == null ? 0 : Number(rawInitialOffset) + const initialOffset = Number.isFinite(parsedInitialOffset) + ? parsedInitialOffset + : 0🤖 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/scroll-anchor/main.tsx` around lines 15 - 17, The initialOffset variable assignment uses Number() which can result in NaN if the query parameter is not a valid number. Add a finite check after the Number conversion using Number.isFinite() to validate that the parsed value is not NaN or Infinity, and provide a fallback to the default value of 0 if the check fails.packages/react-virtual/e2e/app/test/scroll-anchor.spec.ts (1)
47-47: ⚡ Quick winReplace fixed sleeps with condition-based waits to reduce flakiness.
Line 47 and Line 59 use hard-coded
waitForTimeout(500), which can be brittle under variable CI load. Polling for the measurable condition keeps this regression stable and faster.Proposed patch
- await page.waitForTimeout(500) + await expect + .poll(async () => { + return await page.evaluate(() => { + const container = document.querySelector('`#scroll-container`') + if (!container) return 0 + return container.querySelectorAll('[data-index]').length + }) + }) + .toBeGreaterThan(0) @@ - // Let the async ResizeObserver measurements + scroll adjustments settle. - await page.waitForTimeout(500) + // Wait until the anchor displacement converges near expected delta. + await expect + .poll(async () => { + const top = await page.evaluate(topOfIndex, anchor.index) + if (top == null) return Number.POSITIVE_INFINITY + return Math.abs(top - (anchor.top + SCROLL_UP)) + }) + .toBeLessThan(8) @@ - expect(Math.abs(newTop! - (anchor.top + SCROLL_UP))).toBeLessThan(8) + expect(Math.abs(newTop! - (anchor.top + SCROLL_UP))).toBeLessThan(8)Also applies to: 59-59
🤖 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/test/scroll-anchor.spec.ts` at line 47, Replace the hard-coded waitForTimeout(500) calls in the scroll-anchor.spec.ts test with condition-based waits to eliminate flakiness under variable CI load. Instead of sleeping for a fixed duration, identify the measurable condition that should be satisfied (such as an element reaching a specific state, becoming visible, or stabilizing in position) and use page.waitForFunction() or page.locator().waitFor() to poll for that condition. This approach will make the test more reliable and potentially faster since it won't unnecessarily wait the full timeout if the condition is met earlier.
🤖 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.
Nitpick comments:
In `@packages/react-virtual/e2e/app/scroll-anchor/main.tsx`:
- Around line 15-17: The initialOffset variable assignment uses Number() which
can result in NaN if the query parameter is not a valid number. Add a finite
check after the Number conversion using Number.isFinite() to validate that the
parsed value is not NaN or Infinity, and provide a fallback to the default value
of 0 if the check fails.
In `@packages/react-virtual/e2e/app/test/scroll-anchor.spec.ts`:
- Line 47: Replace the hard-coded waitForTimeout(500) calls in the
scroll-anchor.spec.ts test with condition-based waits to eliminate flakiness
under variable CI load. Instead of sleeping for a fixed duration, identify the
measurable condition that should be satisfied (such as an element reaching a
specific state, becoming visible, or stabilizing in position) and use
page.waitForFunction() or page.locator().waitFor() to poll for that condition.
This approach will make the test more reliable and potentially faster since it
won't unnecessarily wait the full timeout if the condition is met earlier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5deb8ed-b464-4949-a88c-53b2a09a5aeb
📒 Files selected for processing (8)
.changeset/adjust-scroll-first-measurement-backward.mdexamples/react/chat/src/index.csspackages/react-virtual/e2e/app/scroll-anchor/index.htmlpackages/react-virtual/e2e/app/scroll-anchor/main.tsxpackages/react-virtual/e2e/app/test/scroll-anchor.spec.tspackages/react-virtual/e2e/app/vite.config.tspackages/virtual-core/src/index.tspackages/virtual-core/tests/index.test.ts
… scroll The default shouldAdjustScroll predicate skipped scroll compensation for any above-viewport resize while scrolling backward. That over-applied to items being measured for the first time: their estimate→actual delta lives above the viewport and must be compensated, or the anchored content jumps when scrolling up into never-seen rows. Distinguish first measurement (!itemSizeCache.has(key)) — always compensate — from re-measurement, which still skips during backward scroll to avoid the cascading "items fight the scroll" jank.
piecyk
force-pushed
the
fix/scroll-adjust-first-measurement-backward
branch
from
ba5d095 to
779fa6f
Compare
Merged
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.
The default shouldAdjustScroll predicate skipped scroll compensation for any above-viewport resize while scrolling backward. That over-applied to items being measured for the first time: their estimate→actual delta lives above the viewport and must be compensated, or the anchored content jumps when scrolling up into never-seen rows.
Distinguish first measurement (!itemSizeCache.has(key)) — always compensate — from re-measurement, which still skips during backward scroll to avoid the cascading "items fight the scroll" jank.
🎯 Changes
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit