fix(virtual-core): remove incorrect elementsCache cleanup using getItemKey#1148
fix(virtual-core): remove incorrect elementsCache cleanup using getItemKey#1148piecyk merged 2 commits intoTanStack:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 6e47f8e The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis PR fixes a stale-index bug where ResizeObserver callbacks could call Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
📝 Coding Plan
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
|
View your CI Pipeline Execution ↗ for commit 6e47f8e
☁️ 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/stale-index.spec.ts (1)
16-16: Fixed timeouts are acceptable here but could cause flakiness.The
waitForTimeoutcalls are a pragmatic solution for waiting on ResizeObserver callbacks, which don't have a deterministic signal. If tests become flaky in CI, consider increasing these values or usingpage.waitForFunctionto poll for a stable state.Also applies to: 25-25, 35-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-virtual/e2e/app/test/stale-index.spec.ts` at line 16, The fixed short timeouts (await page.waitForTimeout(100)) in stale-index.spec.ts can cause flakiness; replace these calls (all occurrences at the three locations) with a polling wait that asserts the expected stable DOM/size state using page.waitForFunction instead of a hard sleep: implement a page.waitForFunction that reads the relevant element's computed size, index text, or a test-specific attribute until it stabilizes (e.g., compare last and current boundingClientRect or innerText over two consecutive ticks) and use that in place of the waitForTimeout calls so ResizeObserver-driven updates are reliably awaited.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-virtual/e2e/app/test/stale-index.spec.ts`:
- Line 16: The fixed short timeouts (await page.waitForTimeout(100)) in
stale-index.spec.ts can cause flakiness; replace these calls (all occurrences at
the three locations) with a polling wait that asserts the expected stable
DOM/size state using page.waitForFunction instead of a hard sleep: implement a
page.waitForFunction that reads the relevant element's computed size, index
text, or a test-specific attribute until it stabilizes (e.g., compare last and
current boundingClientRect or innerText over two consecutive ticks) and use that
in place of the waitForTimeout calls so ResizeObserver-driven updates are
reliably awaited.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ead7d156-5397-42f4-8069-7e2e4507bce3
📒 Files selected for processing (5)
packages/react-virtual/e2e/app/stale-index/index.htmlpackages/react-virtual/e2e/app/stale-index/main.tsxpackages/react-virtual/e2e/app/test/stale-index.spec.tspackages/react-virtual/e2e/app/vite.config.tspackages/virtual-core/src/index.ts
💤 Files with no reviewable changes (1)
- packages/virtual-core/src/index.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.changeset/quick-rings-happen.md (1)
1-5: Consider adding more context to the changeset description.The changeset format is correct and the description matches the PR title. However, users reading the release notes might benefit from understanding the impact of this fix. Consider expanding the description to briefly mention that this resolves potential assertion failures or out-of-bounds errors when items are removed from a virtualized list.
📝 Example enhanced description
--- '@tanstack/virtual-core': patch --- -fix(virtual-core): remove incorrect elementsCache cleanup using getItemKey +fix(virtual-core): remove incorrect elementsCache cleanup using getItemKey + +Fixes a bug where ResizeObserver could call getItemKey with stale indices for disconnected DOM nodes, potentially causing assertion failures or out-of-bounds access when items are removed from virtualized lists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/quick-rings-happen.md around lines 1 - 5, Update the changeset description for the '@tanstack/virtual-core' patch (the entry that says "fix(virtual-core): remove incorrect elementsCache cleanup using getItemKey") to include brief context about the impact: state that this fix prevents potential assertion failures or out-of-bounds errors when items are removed from a virtualized list and briefly note that it corrects elementsCache cleanup logic that used getItemKey incorrectly so users understand why they should upgrade.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.changeset/quick-rings-happen.md:
- Around line 1-5: Update the changeset description for the
'@tanstack/virtual-core' patch (the entry that says "fix(virtual-core): remove
incorrect elementsCache cleanup using getItemKey") to include brief context
about the impact: state that this fix prevents potential assertion failures or
out-of-bounds errors when items are removed from a virtualized list and briefly
note that it corrects elementsCache cleanup logic that used getItemKey
incorrectly so users understand why they should upgrade.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4aeab723-607c-4595-9a19-0229bc899853
📒 Files selected for processing (1)
.changeset/quick-rings-happen.md
1 participant
🎯 Changes
Fix incorrect
elementsCachecleanup logic introduced when handling disconnected nodes.The code attempted to call:
However this is unnecessary and unsafe because the cache cleanup for disconnected nodes is already handled in measureElement(null), which iterates the elementsCache by reference and removes stale entries.
This PR removes the redundant getItemKey call and relies on the existing cleanup logic. The unobserve call remains unchanged.
closes #1147
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests
New Features
Chores