fix: guard notebook cell relayout against stale cell during re-entrant scroll (fixes #328987)

[VS Code PR Bot (vscodebot-pr)]

Summary

Telemetry reports ListError [NotebookCellList] Invalid index -1 thrown from NotebookCellList.getCellViewScrollTop (notebookCellList.ts:754). The crash is a re-entrant event-delivery lifecycle race: a synchronous cell height update (updateElementHeight2) drives the scrollable to fire onDidScroll while the notebook list is mid-mutation. A CodeCell listener reacts by calling CodeCellLayout.layoutEditor('nbDidScroll'), which asks for the cell's absolute top. At that transient moment the cell is no longer resolvable in the view model, so _getViewIndexUpperBound(cell) returns -1 and getCellViewScrollTop throws. This is a new-type stable anomaly appearing in 1.131.0 (absent in 1.130.0), affecting ~875 users.

The fix skips the scroll-driven relayout for a cell that is no longer present in the view model, so the throwing path is never reached during the transient state.

Fixes #328987
Recommended reviewer: @DonJayamanne

Culprit Commit

bf56edffb59c43fa0de636c3aa1d548770b168b8 — "Limit Notebook Cell Editor to Viewport Height (#267089)" by Don Jayamanne (2025-10-13), verified as an ancestor of the shipped commit e4c7e7b1d6d060162f4aa7f8225271b67ce1df75 (v1.131.0). This change introduced the new CodeCellLayout layout path (_useNewApproachForEditorLayout = true) whose onDidScroll handler calls layoutEditor('nbDidScroll') and, via getAbsoluteTopOfElement, reaches the throwing getCellViewScrollTop. This matches the 1.131 first-seen window.

Code Flow

flowchart TD
  A[updateElementHeight2 notebookCellList.ts:1242] --> B[listView.updateElementHeight / setScrollTop]
  B --> C[Scrollable._setState fires onScroll re-entrantly]
  C --> D[notebookEditorWidget onDidScroll]
  D --> E[codeCell.ts onDidScroll listener :283]
  E --> F[CodeCellLayout.layoutEditor 'nbDidScroll' :850]
  F --> G[getAbsoluteTopOfElement viewCell :873]
  G --> H[notebookEditorWidget.getAbsoluteTopOfElement :2131]
  H --> I[getCellViewScrollTop notebookCellList.ts:751]
  I --> J{_getViewIndexUpperBound cell === -1?}
  J -->|cell absent from view model| K[throw ListError Invalid index -1 :754]

Loading

Affected Files

• src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts — CodeCellLayout.layoutEditor (crash-triggering consumer; fix applied here).
• src/vs/workbench/contrib/notebook/browser/view/notebookCellList.ts — getCellViewScrollTop (:751) / _getViewIndexUpperBound (:688) is the crash site (unchanged; per fix principles the throw is a correct invariant check and must not be softened).
• src/vs/workbench/contrib/notebook/browser/notebookEditorWidget.ts — getAbsoluteTopOfElement (:2131) forwards to the list (unchanged).

Repro Steps

Not reliably reproducible on demand (race-dependent). Occurs when a cell's height is updated (e.g. output growth, paste, execution) while the notebook is scrolling, such that a re-entrant scroll event is delivered for a cell that has just been removed / re-indexed in the view model. Telemetry confirms a stable, high-volume anomaly across 1.131.0.

How the Fix Works

Chosen approach — CodeCellLayout.layoutEditor (codeCell.ts): add an early guard that returns when notebookEditor.getCellIndex(this.viewCell) is undefined or -1, placed before any absolute-position computation. This targets the re-entrant event-delivery pattern: the scroll event is delivered synchronously while the list is mutating, and the consumer must re-resolve the cell's presence under the current state before dereferencing it. Because the guard runs before getAbsoluteTopOfElement, the getCellViewScrollTop invariant check (file getCellViewScrollTop) can no longer be reached with a -1 index from this scroll path — the throwing path is made unreachable during the transient state rather than the thrown error being swallowed. The crash-site throw and all logService/telemetry paths are left intact.

After this change, the layoutEditor scroll path cannot call getAbsoluteTopOfElement for a cell absent from the view model, because getCellIndex returning undefined/-1 short-circuits the method before the position lookup.

Alternatives considered:

• Softening getCellViewScrollTop to return 0 instead of throwing — rejected: it hides a genuine invariant violation from telemetry and masks the symptom at the crash site instead of the producer.
• Wrapping the onDidScroll listener body in try/catch — rejected: it silences the error without addressing why a stale cell reaches layout, and would suppress telemetry.

Recommended Owner

@DonJayamanne — author of the CodeCellLayout new-layout path (culprit commit bf56edffb59c, #267089) and of the recent notebook cell layout/scroll fixes touching this file.

Generated by errors-fix · opus48 · 627.2 AIC · ⌖ 11.4 AIC · ⊞ 18.1K · ◷

errors-fix-driver — cycle 2

Trigger: cron_review_comments · Head: b4cc5951401 (b4cc595)

Item	Action
Review on codeCell.ts:860 — missing regression test (in scope)	Fixed in b4cc595 (replied + resolved)
Review on codeCell.ts:859 — handle vs identity resolution (in scope)	Fixed in b4cc595: guard resolves via getViewModel().getCellIndex identity indexOf (replied + resolved)
Review on codeCell.ts:857 — inline comment too long (in scope)	Fixed in b4cc595: condensed to one line (replied + resolved)

Push: yes — b4cc595 · Copilot rerequested: ok

Note: The cycle-1 replies cited fa80dd6, which never landed on the branch head (e226f0c). This cycle re-implements those fixes for real and pushes them as b4cc595.

Ready gate: CI pending on new head + Copilot not yet re-run on new SHA → not marking ready this cycle

Generated by errors-fix-driver · opus48 · 320.3 AIC · ⌖ 21.5 AIC · ⊞ 21.1K · ◷

[Copilot]

Pull request overview

Guards notebook cell layout against stale cells during re-entrant scrolling.

Changes:

• Checks cell membership before computing its absolute position.
• Skips layout when the cell is absent.

Suppressed comments (1)

src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts:858

• This unconditional call breaks the existing CodeCellLayout tests: the editor delegates in cellPart.test.ts are cast through unknown and none implements getCellIndex, so every existing layoutEditor(...) test now throws a TypeError before its assertions. The test delegates must be updated along with this production change.

		const cellIndex = this.notebookEditor.getCellIndex(this.viewCell);

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts:858

• This guard does not enforce the invariant required by the following position lookup: notebookEditor.getCellIndex searches by handle, while getCellViewScrollTop searches for this exact cell object. A stale cell from a previous model can share a handle with a current cell, pass this check, and still throw Invalid index -1. Resolve through the current view model's identity-based lookup instead.

		const cellIndex = this.notebookEditor.getCellIndex(this.viewCell);

src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts:857

• This method-body comment exceeds the repository's one-line limit for inline comments. Keep only the non-obvious re-entrancy constraint; the guard itself documents the return behavior.

		// A scroll-driven relayout can be delivered re-entrantly while the notebook list is mutating
		// (e.g. during a cell height update). In that transient state the cell may no longer be part of
		// the view model, in which case computing its absolute position throws `Invalid index -1`.
		// Skip the relayout for cells that are no longer present rather than reaching the throwing path.

src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts:860

• The stale-cell branch is not covered by the existing CodeCellLayout tests. Add a regression test where the current view model reports this cell as absent and all absolute-position methods throw if invoked, proving the relayout returns before the crash path.

		if (cellIndex === undefined || cellIndex === -1) {
			return;

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts:859

• This guard uses INotebookEditor.getCellIndex, which resolves by handle (notebookEditorWidget.ts:2980), but the throwing list lookup resolves the exact object (notebookCellList.ts:693). If a replacement/current cell has reused the stale cell's handle, this check passes and getAbsoluteTopOfElement(this.viewCell) still throws. Use the view model's identity-based lookup so the guard enforces the same invariant as the called API.

		const cellIndex = this.notebookEditor.getCellIndex(this.viewCell);
		if (cellIndex === undefined || cellIndex === -1) {

src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts:858

• The new unconditional method call breaks the existing CodeCellLayout unit tests: every delegate fixture in cellPart.test.ts omits getCellIndex, and the unknown casts hide that omission from TypeScript, so each enabled layoutEditor invocation now throws TypeError before testing its behavior. Update those fixtures and include the stale-cell short-circuit case described by this change.

		const cellIndex = this.notebookEditor.getCellIndex(this.viewCell);

src/vs/workbench/contrib/notebook/browser/view/cellParts/codeCell.ts:857

• This four-line inline comment exceeds the repository's one-line limit for comments inside method bodies. Keep only the non-obvious ordering/identity constraint; the guard itself explains the return behavior.

		// A scroll-driven relayout can be delivered re-entrantly while the notebook list is mutating
		// (e.g. during a cell height update). In that transient state the cell may no longer be part of
		// the view model, in which case computing its absolute position throws `Invalid index -1`.
		// Skip the relayout for cells that are no longer present rather than reaching the throwing path.