fix: guard ListView dynamic-height probe against shrunk model (fixes #333550)

[VS Code PR Bot (vscodebot-pr)]

Summary

ListView.probeDynamicHeights dereferences this.items[index] for every index in the render range before the caller's shrink-detection guard runs. When a synchronous, reentrant model update (e.g. userDataProfilesEditor.ts set templates replacing the whole tree during a rerender) leaves the render range's end beyond the current item count, item is undefined and shouldProbeDynamicHeight throws TypeError: Cannot read properties of undefined (reading 'hasDynamicHeight'). It affects 843 users on stable 1.135.0 across all platforms.

Fixes #333550
Recommended reviewer: @connor4312

Culprit Commit

Field	Value
Commit	c2b336da
Author	@connor4312
PR	#330967
Message	list: batch dynamic height measurements (#330967)
Why	It rewrote _rerender to call probeDynamicHeights(renderRange, ...) (which eagerly reads this.items[index] across the whole range) before evaluating the new modelDidChange guard (this.items.length < renderRange.end). Prior to this change each index was probed one-at-a-time via probeDynamicHeight(i), so a shrunk model could not be read out of bounds.

Code Flow

sequenceDiagram
    participant Editor as userDataProfilesEditor (set templates)
    participant Tree as AsyncDataTree.rerender
    participant View as ListView._rerender
    participant Probe as probeDynamicHeights
    participant Guard as shouldProbeDynamicHeight

    Editor->>Tree: synchronous rerender after model replace
    Note over View: ⚠️ Root cause:<br/>renderRange.end > this.items.length<br/>after reentrant shrink
    View->>Probe: probeDynamicHeights(renderRange)
    Probe->>Guard: shouldProbeDynamicHeight(this.items[index]=undefined)
    Note over Guard: 💥 TypeError:<br/>reading 'hasDynamicHeight' of undefined

Loading

Affected Files

File	Role	Evidence
src/vs/base/browser/ui/list/listView.ts	crash site	L1853 (from stack): `if (!item.hasDynamicHeight
src/vs/base/browser/ui/list/listView.ts	root cause	L1768-1769: for (...; index < range.end; ...) { const item = this.items[index]; ...probeDynamicHeightFromDelegate(item) — probes full range before shrink guard
src/vs/base/browser/ui/list/listView.ts	recovery guard (runs too late)	L1618: const modelDidChange = this.items.length < renderRange.end — designed to restart the loop, but only checked after probeDynamicHeights already threw

Repro Steps

Non-deterministic (timing/reentrancy dependent). It occurs when a list/tree with dynamic heights has its model synchronously shrunk during a rerender while the render range still extends past the previous end:

1. Open the User Data Profiles editor with a tree that has more rows visible than after an update.
2. Trigger set templates (profile/template refresh) so the tree contents are replaced synchronously during an in-flight rerender.
3. If the new model has fewer items than the prior render range end, probeDynamicHeights reads this.items[index] === undefined and throws.

To increase likelihood: rapidly refresh/replace tree contents while scrolled so the render range covers rows near the end of the list.

How the Fix Works

Chosen approach (src/vs/base/browser/ui/list/listView.ts): Clamp the probe loop's upper bound to the current item count — const end = Math.min(range.end, this.items.length) — so probeDynamicHeights never dereferences a non-existent item. This is a fix at the data producer (the loop that reads this.items), not a guard bolted onto the crash site shouldProbeDynamicHeight. The existing recovery machinery already handles the shrink: after probeDynamicHeights returns, the caller's modelDidChange check (this.items.length < renderRange.end) fires and restarts the measurement against the updated range. The premature out-of-bounds read was the only thing preventing that intended path from running. The diffs array keeps its original range.end - range.start length, so clamped-out entries stay 0 and the caller's index arithmetic is unchanged.

After this change, listView.ts:1774 cannot pass undefined to shouldProbeDynamicHeight because the loop only iterates indices < this.items.length, and this.items[index] for index < this.items.length is always a defined IItem<T>.

Alternatives considered: Adding if (!item) continue; or item?.hasDynamicHeight at the crash site — rejected because it guards the symptom at the bottom of the stack rather than the producer, and would silently skip valid entries without letting the modelDidChange recovery restart the measurement correctly.

Recommended Owner

@connor4312 — author of the culprit commit #330967 and an active core VS Code maintainer (recent commits within the last 90 days). Owns the batched dynamic-height measurement logic in listView.ts.

Generated by errors-fix · opus48 · 443 AIC · ⌖ 11.4 AIC · ⊞ 18.6K · ◷

[vs-code-engineering]

📬 CODENOTIFY

The following users are being notified based on files changed in this PR:

Benjamin Christopher Simmonds (@benibenj)

Matched files:

• src/vs/base/browser/ui/list/listView.ts

[Copilot]

Pull request overview

Prevents ListView dynamic-height probing from reading beyond a model that shrinks during rerendering.

Changes:

• Limits probing to the current item count.
• Preserves existing shrink-recovery behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[vs-code-engineering]

Driver cycle recorded

_{errors-fix-driver:cycle head:7803cd151652cd7be9552de038a13303a77298b8}

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[vs-code-engineering]

Driver cycle recorded

_{errors-fix-driver:cycle head:a9a14d060dbca485dbb0aa1f39803e316c3b36a6}

[vs-code-engineering]

Driver cycle recorded

_{errors-fix-driver:cycle head:88ff6c9c1c9a38283b22ac9061023dc730d81d7a}

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[vs-code-engineering]

Driver cycle recorded

_{errors-fix-driver:cycle head:0bf8f42584e929e3f08dcc2280fbd1c33288f7ab}

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/vs/base/test/browser/ui/list/listView.test.ts:497

• This inline comment exceeds the project's one-line limit for comments inside method bodies. Please keep the reentrancy rationale on one line.

					// Drop the trailing items so the render range's end now extends past the
					// shrunk model while a later index is still pending in the probe loop.

[vs-code-engineering]

Driver cycle recorded

_{errors-fix-driver:cycle head:541d87bad4cab20a6c47b082c995f5734e2125ba}

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/vs/base/test/browser/ui/list/listView.test.ts:497

• This inline explanation exceeds the project's one-line limit for comments inside method bodies. Keep only the non-obvious stale-range rationale in a single line.

					// Drop the trailing items so the render range's end now extends past the
					// shrunk model while a later index is still pending in the probe loop.

[vs-code-engineering]

Driver cycle recorded

_{errors-fix-driver:cycle head:c6911d3ea2e47f14f8612a3255567ec9d5864074}

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/vs/base/test/browser/ui/list/listView.test.ts:504

• This regression still does not leave a later probe index pending. With a 100px viewport and 100px range-map entries, row 0 shrinking to 20px expands the next render range only to [0, 2). Row 1's new height is not measured until after its renderElement callback, so the splice fires on the final iteration and the old index < range.end loop also passes. Force the render range to include all four items only for the triggering layout so index 2 is genuinely pending.

		const elements: TestElement[] = range(4).map(index => ({ id: String(index), height: 100 }));
		const listView = listViewRef.value = new ListView<TestElement>(element, delegate, [renderer], { supportDynamicHeights: true });