feat(virtual-core): add useCachedMeasurements option to preserve sizes when list is hidden#1186
Merged
Conversation
…zes when list is hidden
📝 WalkthroughWalkthroughThis PR adds a ChangesCached Measurements Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
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 320f97c
☁️ 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 (1)
packages/react-virtual/e2e/app/test/cached-measurements.spec.ts (1)
10-10: ⚡ Quick winReplace fixed timeouts with condition-based waits.
The test uses
waitForTimeoutat three points (200ms, 300ms, 200ms) to wait for rendering and ResizeObserver callbacks. Fixed timeouts make tests fragile—too short and they flake; too long and they slow down the suite.Consider waiting for specific conditions instead:
// Instead of: await page.waitForTimeout(200) // Wait for total-size to stabilize await page.waitForFunction(() => { const size = document.querySelector('[data-testid="total-size"]')?.textContent; return size && Number(size) > 0; });Or use Playwright's built-in retry mechanisms by directly asserting on the expected state—assertions automatically retry until timeout.
Also applies to: 23-23, 34-34
🤖 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/cached-measurements.spec.ts` at line 10, The test uses fixed page.waitForTimeout calls which are flaky; replace each await page.waitForTimeout(...) with condition-based waits that assert the expected DOM state (e.g., waitForFunction or expect(locator).toHaveText()/toHaveCount()) so Playwright will retry until stable; target the specific selectors used in the test (for example the element with data-testid="total-size" or the list/container locators referenced in cached-measurements.spec.ts) and wait for the numeric/text value or item count to reach the expected value instead of sleeping, updating all three occurrences to use these condition-based assertions.
🤖 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/test/cached-measurements.spec.ts`:
- Line 10: The test uses fixed page.waitForTimeout calls which are flaky;
replace each await page.waitForTimeout(...) with condition-based waits that
assert the expected DOM state (e.g., waitForFunction or
expect(locator).toHaveText()/toHaveCount()) so Playwright will retry until
stable; target the specific selectors used in the test (for example the element
with data-testid="total-size" or the list/container locators referenced in
cached-measurements.spec.ts) and wait for the numeric/text value or item count
to reach the expected value instead of sleeping, updating all three occurrences
to use these condition-based assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97a605b7-5359-4c03-9a7a-4614ce3d89b0
📒 Files selected for processing (7)
.changeset/use-cached-measurements.mddocs/api/virtualizer.mdpackages/react-virtual/e2e/app/cached-measurements/index.htmlpackages/react-virtual/e2e/app/cached-measurements/main.tsxpackages/react-virtual/e2e/app/test/cached-measurements.spec.tspackages/react-virtual/e2e/app/vite.config.tspackages/virtual-core/src/index.ts
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.
🎯 Changes
Add
useCachedMeasurementsoption to skip DOM measurement when the list is hidden (e.g.display: none). When enabled, the defaultmeasureElementreturns the cached size orestimateSizefallback instead of reading the DOM, preventing ResizeObserver from resetting measurements to zero.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
useCachedMeasurementsoption to virtualizer configuration that uses previously cached item sizes instead of DOM measurements.Documentation
useCachedMeasurementsconfiguration option.Tests