Skip to content

Commit 7ece2d5

Browse files
piecykpiecyk
authored
fix(virtual-core): remove incorrect elementsCache cleanup using getItemKey (#1148)
1 parent 6360f79 commit 7ece2d5

File tree

6 files changed

+149
-1
lines changed

6 files changed

+149
-1
lines changed

‎.changeset/quick-rings-happen.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@tanstack/virtual-core': patch
3+
---
4+
5+
fix(virtual-core): remove incorrect elementsCache cleanup using getItemKey

‎packages/react-virtual/e2e/app/stale-index/index.html‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<!doctype html>
2+
<html lang="en">
3+
<head>
4+
<meta charset="UTF-8" />
5+
</head>
6+
<body>
7+
<div id="root"></div>
8+
<script type="module" src="./main.tsx"></script>
9+
</body>
10+
</html>

‎packages/react-virtual/e2e/app/stale-index/main.tsx‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
import React from 'react'
2+
import ReactDOM from 'react-dom/client'
3+
import { useVirtualizer } from '@tanstack/react-virtual'
4+
5+
/**
6+
* Regression test app for stale index bug:
7+
* When items are removed from the end of the list, the ResizeObserver may fire
8+
* for a disconnected node whose data-index >= the new count. If getItemKey
9+
* indexes into the items array, this causes an out-of-bounds error.
10+
*/
11+
12+
interface Item {
13+
id: string
14+
label: string
15+
}
16+
17+
function makeItems(count: number): Array<Item> {
18+
return Array.from({ length: count }, (_, i) => ({
19+
id: `item-${i}`,
20+
label: `Row ${i}`,
21+
}))
22+
}
23+
24+
const App = () => {
25+
const parentRef = React.useRef<HTMLDivElement>(null)
26+
const [items, setItems] = React.useState(() => makeItems(20))
27+
const [error, setError] = React.useState<string | null>(null)
28+
29+
const rowVirtualizer = useVirtualizer({
30+
count: items.length,
31+
getScrollElement: () => parentRef.current,
32+
estimateSize: () => 50,
33+
getItemKey: (index) => {
34+
if (index < 0 || index >= items.length) {
35+
const msg = `getItemKey called with stale index ${index} (count=${items.length})`
36+
setError(msg)
37+
throw new Error(msg)
38+
}
39+
return items[index].id
40+
},
41+
})
42+
43+
const removeLastFive = () => {
44+
setItems((prev) => prev.slice(0, Math.max(0, prev.length - 5)))
45+
}
46+
47+
return (
48+
<div>
49+
<button data-testid="remove-items" onClick={removeLastFive}>
50+
Remove last 5
51+
</button>
52+
<div data-testid="item-count">Count: {items.length}</div>
53+
{error && <div data-testid="error">{error}</div>}
54+
<div
55+
ref={parentRef}
56+
data-testid="scroll-container"
57+
style={{ height: 300, overflow: 'auto' }}
58+
>
59+
<div
60+
style={{
61+
height: rowVirtualizer.getTotalSize(),
62+
position: 'relative',
63+
}}
64+
>
65+
{rowVirtualizer.getVirtualItems().map((v) => {
66+
const item = items[v.index]
67+
return (
68+
<div
69+
key={item.id}
70+
data-testid={item.id}
71+
ref={rowVirtualizer.measureElement}
72+
data-index={v.index}
73+
style={{
74+
position: 'absolute',
75+
top: 0,
76+
left: 0,
77+
transform: `translateY(${v.start}px)`,
78+
width: '100%',
79+
height: 50,
80+
borderBottom: '1px solid #ccc',
81+
padding: 8,
82+
boxSizing: 'border-box',
83+
}}
84+
>
85+
{item.label}
86+
</div>
87+
)
88+
})}
89+
</div>
90+
</div>
91+
</div>
92+
)
93+
}
94+
95+
ReactDOM.createRoot(document.getElementById('root')!).render(<App />)

‎packages/react-virtual/e2e/app/test/stale-index.spec.ts‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
import { expect, test } from '@playwright/test'
2+
3+
test('does not call getItemKey with stale index after removing items', async ({
4+
page,
5+
}) => {
6+
await page.goto('/stale-index/')
7+
8+
// Verify initial state
9+
await expect(page.locator('[data-testid="item-count"]')).toHaveText(
10+
'Count: 20',
11+
)
12+
13+
// Scroll to the bottom so the last items are rendered and observed by ResizeObserver
14+
const container = page.locator('[data-testid="scroll-container"]')
15+
await container.evaluate((el) => (el.scrollTop = el.scrollHeight))
16+
await page.waitForTimeout(100)
17+
18+
// Remove 5 items from the end — the RO may still fire for the now-disconnected nodes
19+
await page.click('[data-testid="remove-items"]')
20+
await expect(page.locator('[data-testid="item-count"]')).toHaveText(
21+
'Count: 15',
22+
)
23+
24+
// Wait for any pending ResizeObserver callbacks
25+
await page.waitForTimeout(200)
26+
27+
// No error should have been thrown
28+
await expect(page.locator('[data-testid="error"]')).not.toBeVisible()
29+
30+
// Remove 5 more to stress it
31+
await page.click('[data-testid="remove-items"]')
32+
await expect(page.locator('[data-testid="item-count"]')).toHaveText(
33+
'Count: 10',
34+
)
35+
await page.waitForTimeout(200)
36+
37+
await expect(page.locator('[data-testid="error"]')).not.toBeVisible()
38+
})

‎packages/react-virtual/e2e/app/vite.config.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ export default defineConfig({
1414
'measure-element/index.html',
1515
),
1616
'smooth-scroll': path.resolve(__dirname, 'smooth-scroll/index.html'),
17+
'stale-index': path.resolve(__dirname, 'stale-index/index.html'),
1718
},
1819
},
1920
},

‎packages/virtual-core/src/index.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -414,7 +414,6 @@ export class Virtualizer<
414414

415415
if (!node.isConnected) {
416416
this.observer.unobserve(node)
417-
this.elementsCache.delete(this.options.getItemKey(index))
418417
return
419418
}
420419

0 commit comments

Comments
 (0)