Skip to content

Commit 67db591

Browse files
committed
fix: address CodeRabbit findings on PR #1168
Real bugs: - iOS deferred flush now rolls its delta into scrollAdjustments so any resize landing before the resulting scroll event sees the correct effective offset (previously the running accumulator stayed at 0 and a follow-up correction would compute from the stale pre-flush offset). - measure() now resets pendingMin so the rebuild starts from index 0. Without this, a prior resizeItem() that left pendingMin > 0 would cause the next getMeasurements() to preserve stale entries before that index, partially defeating the invalidation. Tests: - Add a regression test for the measure() / pendingMin interaction. - Add a regression test that asserts scrollAdjustments tracks the flushed iOS delta. - Replace the wall-clock perf budget on the 1M-item lazy-path test with deterministic functional assertions (length + spot-checks of start/size/end across the range). Benchmarks: - VirtuaPage.getTotalSize() now actually uses the queried sized node before falling back to firstElementChild / host. - Runner reads scenarios from window.bench.scenarios instead of a runtime import('/src/scenarios/types.ts'), which wouldn't resolve under vite preview (only the built dist is served). - Persist the full scenario object on every result row (success and error) and add landingErrorPx to the error-path metrics so the schema is consistent. - Use Array<T> annotations in dataset.ts / scenarios/types.ts to satisfy @typescript-eslint/array-type. - README: language hint on the tree fence (MD040) and React 18 in the fairness notes.
1 parent c09bcab commit 67db591

8 files changed

Lines changed: 140 additions & 21 deletions

File tree

benchmarks/README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ Results land in `benchmarks/results/<timestamp>.json` (raw, every run) and
2828

2929
## How it works
3030

31-
```
31+
```text
3232
benchmarks/
3333
├── src/
3434
│ ├── main.tsx Reads ?lib=... &scenario=...
@@ -159,7 +159,7 @@ it's measuring — it just calls one global function per page.
159159
TanStack uses `useVirtualizer` + `measureElement`; virtua uses `VList` with
160160
the `data`/`item` props; virtuoso uses `Virtuoso` with `fixedItemHeight`
161161
when applicable; react-window uses `List` + `useDynamicRowHeight`.
162-
- React 19 runs in production mode (no `<StrictMode>`).
162+
- React 18 runs in production mode (no `<StrictMode>`).
163163
- Dataset is deterministic (LCG-seeded) and identical across libraries.
164164
- `--enable-precise-memory-info` + `--js-flags=--expose-gc` are passed to
165165
Chromium so memory readings aren't bucketed and we can force GC between

benchmarks/runner/run.mjs

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,18 +86,20 @@ async function runScenario(page, lib, scenarioId) {
8686
timeout: 15_000,
8787
})
8888
// Pull the scenario object back from the page so we run with the exact same
89-
// shape the page is using.
89+
// shape the page is using. We read from window.bench.scenarios (populated
90+
// at mount) rather than a runtime `import('/src/scenarios/types.ts')`,
91+
// since `vite preview` only serves the built dist, not source files.
9092
const result = await page.evaluate(async (id) => {
91-
const mod = await import('/src/scenarios/types.ts')
92-
const scenario = mod.SCENARIOS.find((s) => s.id === id)
93+
const scenario = window.bench?.scenarios.find((s) => s.id === id)
9394
if (!scenario) throw new Error('unknown scenario: ' + id)
9495
// Force GC where supported so memory readings aren't poisoned by previous run.
9596
if ('gc' in globalThis) {
9697
try {
9798
globalThis.gc()
9899
} catch {}
99100
}
100-
return await window.bench.run(scenario)
101+
const metrics = await window.bench.run(scenario)
102+
return { scenario, metrics }
101103
}, scenarioId)
102104
return result
103105
}
@@ -264,10 +266,14 @@ async function main() {
264266
`\n ${lib.padEnd(9)} ${scenarioId.padEnd(28)} run ${r + 1}/${opts.runs} ... `,
265267
)
266268
try {
267-
const metrics = await runScenario(page, lib, scenarioId)
269+
const { scenario, metrics } = await runScenario(
270+
page,
271+
lib,
272+
scenarioId,
273+
)
268274
results.push({
269275
library: lib,
270-
scenario: { id: scenarioId },
276+
scenario,
271277
metrics,
272278
ranAt: new Date().toISOString(),
273279
})
@@ -287,6 +293,7 @@ async function main() {
287293
longFrames: null,
288294
jankMs: null,
289295
memoryBytes: null,
296+
landingErrorPx: null,
290297
},
291298
ranAt: new Date().toISOString(),
292299
notes: 'error: ' + e.message,

benchmarks/src/lib/dataset.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,25 +54,25 @@ export function makeDataset(
5454
count: number,
5555
dynamic: boolean,
5656
wideVariance = false,
57-
): Item[] {
57+
): Array<Item> {
5858
const rand = lcg(424242)
59-
const items: Item[] = new Array(count)
59+
const items: Array<Item> = new Array(count)
6060
for (let i = 0; i < count; i++) {
6161
if (dynamic) {
6262
if (wideVariance) {
6363
// Wide-variance dataset: heights span ~30..500 px (≈16× ratio).
6464
// 1 → 50 words distributed log-normally so most items are short
6565
// but a meaningful tail is very tall.
6666
const wc = 1 + Math.floor(Math.pow(rand(), 2) * 49)
67-
const parts: string[] = new Array(wc)
67+
const parts: Array<string> = new Array(wc)
6868
for (let w = 0; w < wc; w++) {
6969
parts[w] = WORDS[Math.floor(rand() * WORDS.length)]!
7070
}
7171
items[i] = { id: i, text: `#${i} ${parts.join(' ')}` }
7272
} else {
7373
// 5..14 words → ~ one line; lengths picked deterministically.
7474
const wc = 5 + Math.floor(rand() * 10)
75-
const parts: string[] = new Array(wc)
75+
const parts: Array<string> = new Array(wc)
7676
for (let w = 0; w < wc; w++) {
7777
parts[w] = WORDS[Math.floor(rand() * WORDS.length)]!
7878
}

benchmarks/src/lib/harness.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { SCENARIOS } from '../scenarios/types'
12
import type { ScenarioInput, ScenarioMetrics } from '../scenarios/types'
23

34
// Each library page mounts and waits, then a global driver runs the scripted
@@ -31,6 +32,10 @@ declare global {
3132
bench?: {
3233
run: (scenario: ScenarioInput) => Promise<ScenarioMetrics>
3334
ready: () => boolean
35+
// Exposed so the Node-side Playwright runner can resolve a scenario
36+
// id to its full object without a runtime source-file import (which
37+
// wouldn't survive `vite preview`'s built-only serving).
38+
scenarios: ReadonlyArray<ScenarioInput>
3439
}
3540
}
3641
}
@@ -128,6 +133,7 @@ export function markFirstPaint(): void {
128133
export function installBenchAPI(): void {
129134
window.bench = {
130135
ready: () => !!window.__bench?.ready,
136+
scenarios: SCENARIOS,
131137
run: async (scenario: ScenarioInput): Promise<ScenarioMetrics> => {
132138
const h = await waitFor(() => window.__bench?.handle ?? null)
133139
const mountStart = window.__bench?.mountStart ?? 0

benchmarks/src/pages/VirtuaPage.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,13 @@ export function VirtuaPage({ scenario }: Props) {
3636
align: opts?.align ?? 'start',
3737
}),
3838
getTotalSize: () => {
39+
// VList sets scrollSize implicitly on its sized inner div; prefer
40+
// that node's scrollHeight, then firstElementChild, then host.
3941
const el = hostRef.current?.querySelector(
4042
'[style*="height:"]',
4143
) as HTMLElement | null
42-
// VList sets scrollSize implicitly; fall back to scrollHeight.
4344
return (
45+
el?.scrollHeight ??
4446
(hostRef.current?.firstElementChild as HTMLElement | null)
4547
?.scrollHeight ??
4648
hostRef.current?.scrollHeight ??

benchmarks/src/scenarios/types.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ export interface ScenarioResult {
5656

5757
// The fixed scenarios all libraries run. Adding scenarios here surfaces them
5858
// in the runner without further plumbing.
59-
export const SCENARIOS: ScenarioInput[] = [
59+
export const SCENARIOS: Array<ScenarioInput> = [
6060
{
6161
id: 'mount-fixed-1k',
6262
count: 1_000,

packages/virtual-core/src/index.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -702,8 +702,11 @@ export class Virtualizer<
702702
if (cur < 0 || cur > max) return
703703
const delta = this._iosDeferredAdjustment
704704
this._iosDeferredAdjustment = 0
705+
// Roll the deferred delta into the running accumulator so any resize
706+
// landing between now and the resulting scroll event computes from the
707+
// post-flush offset rather than the stale one.
705708
this._scrollToOffset(cur, {
706-
adjustments: delta,
709+
adjustments: (this.scrollAdjustments += delta),
707710
behavior: undefined,
708711
})
709712
}
@@ -1616,6 +1619,10 @@ export class Virtualizer<
16161619
}
16171620

16181621
measure = () => {
1622+
// Reset pendingMin so the next getMeasurements rebuilds from index 0.
1623+
// Without this, a prior resizeItem() that left pendingMin > 0 would
1624+
// cause the rebuild to preserve stale items before that index.
1625+
this.pendingMin = null
16191626
this.itemSizeCache.clear()
16201627
this.laneAssignments.clear() // Clear lane cache for full re-layout
16211628
this.itemSizeCacheVersion++

packages/virtual-core/tests/index.test.ts

Lines changed: 103 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,39 @@ test('measure() should clear size cache and lane assignments', () => {
638638
expect(measurements[1]!.size).toBe(50)
639639
})
640640

641+
test('measure() should fully invalidate when a later index was dirtied without an intervening getMeasurements()', () => {
642+
// Regression: measure() used to clear itemSizeCache but not pendingMin.
643+
// If resizeItem() had been called without a subsequent getMeasurements()
644+
// to flush pendingMin, the next rebuild would preserve measurementsCache
645+
// entries before that index — even though measure() is supposed to wipe
646+
// everything.
647+
const virtualizer = new Virtualizer({
648+
count: 6,
649+
estimateSize: () => 50,
650+
getScrollElement: () => null,
651+
scrollToFn: vi.fn(),
652+
observeElementRect: vi.fn(),
653+
observeElementOffset: vi.fn(),
654+
})
655+
656+
// Seed item 0 with a non-estimate size, then flush so it's in measurementsCache.
657+
virtualizer.resizeItem(0, 999)
658+
virtualizer['getMeasurements']()
659+
// Now dirty a later index without flushing — pendingMin will be 2.
660+
virtualizer.resizeItem(2, 888)
661+
expect(virtualizer['pendingMin']).toBe(2)
662+
663+
virtualizer.measure()
664+
665+
// After measure(), pendingMin must be null so the rebuild starts at 0
666+
// and discards the stale item-0 entry.
667+
expect(virtualizer['pendingMin']).toBe(null)
668+
669+
const m = virtualizer['getMeasurements']()
670+
expect(m[0]!.size).toBe(50)
671+
expect(m[2]!.size).toBe(50)
672+
})
673+
641674
test('measure() should trigger a re-measurement on subsequent getMeasurements', () => {
642675
let sizeFn = (i: number) => 50
643676
const virtualizer = new Virtualizer({
@@ -1320,8 +1353,13 @@ test('lazy fast path: getVirtualItemForOffset binary search returns correct item
13201353
expect(found?.end).toBe(510)
13211354
})
13221355

1323-
test('lazy fast path: large list (1M items) does not allocate per-item objects upfront', () => {
1324-
const start = performance.now()
1356+
test('lazy fast path: 1M-item list returns a sparse view, not an eagerly-allocated array', () => {
1357+
// Functional contract for the lazy fast path: a 1M-item virtualizer
1358+
// returns measurements that report the correct total length and produce
1359+
// exact start/size/end values on indexed access without requiring the
1360+
// whole array to be materialized. Sparse spot-checks across the range
1361+
// would fail if the fast path were silently allocating N VirtualItems
1362+
// (or if the typed-array backing computed offsets incorrectly).
13251363
const v = new Virtualizer({
13261364
count: 1_000_000,
13271365
estimateSize: () => 30,
@@ -1330,10 +1368,15 @@ test('lazy fast path: large list (1M items) does not allocate per-item objects u
13301368
observeElementRect: vi.fn(),
13311369
observeElementOffset: vi.fn(),
13321370
})
1333-
v['getMeasurements']()
1334-
const elapsed = performance.now() - start
1335-
// Should be sub-50ms even at 1M items (typed array fill + proxy alloc only)
1336-
expect(elapsed).toBeLessThan(50)
1371+
const m = v['getMeasurements']()
1372+
expect(m.length).toBe(1_000_000)
1373+
expect(m[0]!.start).toBe(0)
1374+
expect(m[0]!.size).toBe(30)
1375+
expect(m[0]!.end).toBe(30)
1376+
expect(m[500_000]!.start).toBe(15_000_000)
1377+
expect(m[500_000]!.end).toBe(15_000_030)
1378+
expect(m[999_999]!.start).toBe(29_999_970)
1379+
expect(m[999_999]!.end).toBe(30_000_000)
13371380
})
13381381

13391382
// ─── iOS momentum-safe scroll adjustments ───────────────────────────────────
@@ -1447,6 +1490,60 @@ test('iOS deferral: multiple resizes during scroll accumulate and flush as one',
14471490
})
14481491
})
14491492

1493+
test('iOS deferral: flushed delta is rolled into scrollAdjustments so back-to-back resizes stay consistent', () => {
1494+
// Regression: the deferred flush used to write `adjustments: delta`
1495+
// directly without updating `this.scrollAdjustments`. If a second resize
1496+
// landed before the resulting scroll event fired (and reset the
1497+
// accumulator), the comparison `itemStart < getScrollOffset() +
1498+
// scrollAdjustments` would miss the flushed delta and the next correction
1499+
// would compute from the stale offset.
1500+
withFakeIOSUserAgent(() => {
1501+
const scrollToFn = vi.fn()
1502+
let scrollCallback:
1503+
| ((offset: number, isScrolling: boolean) => void)
1504+
| null = null
1505+
const v = new Virtualizer({
1506+
count: 10,
1507+
estimateSize: () => 50,
1508+
getScrollElement: () =>
1509+
({
1510+
scrollTop: 200,
1511+
scrollLeft: 0,
1512+
scrollHeight: 500,
1513+
clientHeight: 200,
1514+
offsetHeight: 200,
1515+
}) as any,
1516+
scrollToFn,
1517+
observeElementRect: () => {},
1518+
observeElementOffset: (_inst, cb) => {
1519+
scrollCallback = cb
1520+
cb(200, true)
1521+
return () => {}
1522+
},
1523+
})
1524+
v._willUpdate()
1525+
v['getMeasurements']()
1526+
scrollToFn.mockClear()
1527+
1528+
// Build up a deferred adjustment of 50 during scroll.
1529+
v.resizeItem(0, 100)
1530+
expect(v['_iosDeferredAdjustment']).toBe(50)
1531+
expect(v['scrollAdjustments']).toBe(0)
1532+
1533+
// Settle: scroll event resets scrollAdjustments to 0, then the flush
1534+
// runs and must roll the deferred delta back into scrollAdjustments.
1535+
scrollCallback!(200, false)
1536+
1537+
expect(scrollToFn).toHaveBeenCalledTimes(1)
1538+
const [, opts] = scrollToFn.mock.calls[0]!
1539+
expect(opts.adjustments).toBe(50)
1540+
// The running accumulator must now reflect the flushed delta — any
1541+
// resize landing before the resulting scroll event fires has to see
1542+
// the correct effective offset.
1543+
expect(v['scrollAdjustments']).toBe(50)
1544+
})
1545+
})
1546+
14501547
// ─── Phase 1: touch event distinction ────────────────────────────────────────
14511548

14521549
// Mock EventTarget that records listeners so tests can dispatch events

0 commit comments

Comments
 (0)