Code review comments on one notebook cell leak into other notebook cells as the Browser renderer reuses DOM which is not properly cleaned up

[rehmsen]

• VS Code Version: vscode.dev/ (Browser code)
• OS Version: Irrelevant

This is an issue with the browser code, which happens for code review comments on notebook cells. I do not have a public repro, but I can describe line by line why this happens, and am happy to send a PR to fix it:

• Cells in a notebook are rendered as a ListView. ListView uses a cache to reuse widgets and DOM:

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

  Line 905 in 6e6d380

  const result = this.cache.alloc(item.templateId);

• As a result, CellComments, a CellContentPart, is reused, by calling renderCell/unrenderCell:

  vscode/src/vs/workbench/contrib/notebook/browser/view/cellParts/markupCell.ts

  Line 73 in 6facfe2

  this.templateData.cellParts.scheduleRenderCell(this.viewCell);

• The HTMLElement container is passed in upon construction and held on to:

  vscode/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts

  Lines 28 to 30 in 6facfe2

  	constructor(
  	private readonly notebookEditor: INotebookEditorDelegate,
  	private readonly container: HTMLElement,

• For existing comments, a CommentThreadWidget is constructed during initialization of a CellComments, which is called on the first renderCell, and then skipped for subsequent calls:

  vscode/src/vs/workbench/contrib/notebook/browser/view/cellParts/cellComments.ts

  Lines 47 to 58 in 6facfe2

  	private async initialize(element: ICellViewModel) {
  	if (this._initialized) {
  	return;
  	}
  	this._initialized = true;
  	const info = await this._getCommentThreadForCell(element);
  	if (info) {
  	await this._createCommentTheadWidget(info.owner, info.thread);
  	}
  	}

• CommentThreadWidget appends to container and installs some listeners:

  vscode/src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts

  Line 106 in 6facfe2

  container.appendChild(bodyElement);

• CommentThreadWidget.dispose() cleans up listeners, but the appended DOM elements are not removed:

  vscode/src/vs/workbench/contrib/comments/browser/commentThreadWidget.ts

  Lines 263 to 267 in 6facfe2

  	override dispose() {
  	super.dispose();
  	dispose(this._commentThreadDisposables);
  	this.updateCurrentThread(false, false);
  	}

• Similarly, CellComments does not override unrenderCell(), and the base class CellContentPart.unrenderCell() removes listeners (that CellComents put into the corresponding DisposableStore), but does not unset _initialized, clean up _commentThreadWidget, or call CommentThreadWidget.dispose()- they all leak
• Only when onDidUpdateCommentThreads fires, will _commentThreadWidget be disposed (which as seen above does not remove the elements from the DOM - only the layout is reset to hide it, or replaced

I think we need to do the following to fix this issue:

• CommentThreadWidget and its children (CommentThreadHeader) should remove the DOM elements they added to container when they are disposed(). That seems like good practice in general, and seems to be done at least in some other places in VSCode, e.g.

  vscode/src/vs/base/browser/ui/tree/abstractTree.ts

  Lines 798 to 799 in 6125cc2

  	container.appendChild(this.elements.root);
  	this._register(toDisposable(() => this.elements.root.remove()));

• CellComments should, instead of initializing only once, check if the ICellViewModel has changed in on didRender, and if so, run the same logic it runs onDidUpdateCommentThreads, which will remove, created, or update the comment thread.

I'll send a pull request to make this more concrete, happy to talk about other solutions to the problem, too.

@alexr00 @rebornix