Skip to content

Commit 186b3bd

Browse files
committed
fix(virtual-core): don't call getItemKey with a stale index in RO disconnect cleanup
Commit 843690b added an elementsCache cleanup in the ResizeObserver disconnect path that looked up the cache key via getItemKey(index). When items have been removed from the end of the list, that index can be past items.length, so any user-supplied getItemKey that indexes into the data array throws — exactly the bug PR #1148 had fixed for the non-cleanup paths. Fix: find the cache entry by node identity instead. Iterating elementsCache is O(visible-window), which is fine for a path that only fires on disconnect, and it naturally handles the React-replaced-the- node-under-the-same-key case (the === check just won't match). The stale-index e2e test now passes on both react-virtual and angular-virtual, and the two RO-cleanup unit tests still pass since they were written against node identity, not key lookup.
1 parent ab9c00f commit 186b3bd

2 files changed

Lines changed: 21 additions & 7 deletions

File tree

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
---
2+
'@tanstack/virtual-core': patch
3+
---
4+
5+
Don't call `getItemKey` with a possibly-stale index when cleaning up
6+
`elementsCache` for a disconnected node. The cleanup now finds the
7+
matching entry by node identity, so removing items from the end of
8+
the list while a `ResizeObserver` still has the now-detached node
9+
queued no longer throws (regression of #1148).

packages/virtual-core/src/index.ts

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -429,13 +429,18 @@ export class Virtualizer<
429429

430430
if (!node.isConnected) {
431431
this.observer.unobserve(node)
432-
if (index >= 0) {
433-
const key = this.options.getItemKey(index)
434-
// Only delete if this node is still the cached one — guard
435-
// against the case where React mounted a new node for the
436-
// same key after this one disconnected.
437-
if (this.elementsCache.get(key) === node) {
438-
this.elementsCache.delete(key)
432+
// Find the cache entry pointing to this exact node and remove
433+
// it. We can't call getItemKey(index) here because items may
434+
// have been removed since this node was rendered — the index
435+
// could be stale and out-of-bounds in the user's data array
436+
// (regression test in e2e/.../stale-index.spec.ts, fix #1148).
437+
// The === comparison naturally handles the React-replaced-
438+
// a-node-for-the-same-key case: that entry now points to a
439+
// different node, so this loop won't match.
440+
for (const [cacheKey, cachedNode] of this.elementsCache) {
441+
if (cachedNode === node) {
442+
this.elementsCache.delete(cacheKey)
443+
break
439444
}
440445
}
441446
return

0 commit comments

Comments
 (0)