Skip to content

Dispose notebook output toolbar on detach#319882

Open
vivekjm wants to merge 2 commits into
microsoft:mainfrom
vivekjm:fix-notebook-output-toolbar-leak
Open

Dispose notebook output toolbar on detach#319882
vivekjm wants to merge 2 commits into
microsoft:mainfrom
vivekjm:fix-notebook-output-toolbar-leak

Conversation

@vivekjm
Copy link
Copy Markdown
Contributor

@vivekjm vivekjm commented Jun 4, 2026

Fixes #186361

Summary

Notebook output toolbars were not fully reset when an output element was detached, hidden, re-rendered, or switched to another mime type. The toolbar attachment flag also was never set after attaching a toolbar, so reactive visibility changes could keep creating toolbar/action item resources across notebook scroll and render cycles.

This change:

  • marks output toolbars as attached when they are created
  • centralizes toolbar cleanup so the attached flag and disposable store stay in sync
  • clears toolbar disposables on detach, hide, re-render, and mime type switch
  • detaches owned output DOM and webview inset resources during disposal

Validation

Validated in a clean checkout at /tmp/vscode-pr-check because this local working checkout cannot run the pre-commit hook due to missing event-stream in its incomplete dependency install.

  • npm run gulp -- compile-client
  • ./node_modules/.bin/tsgo --project ./src/tsconfig.json --noEmit --skipLibCheck --pretty false
  • npm run eslint -- src/vs/workbench/contrib/notebook/browser/view/cellParts/cellOutput.ts
  • git diff --check

Copilot AI review requested due to automatic review settings June 4, 2026 09:25
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR improves notebook cell output toolbar lifecycle management by centralizing toolbar cleanup logic and ensuring cleanup occurs during detach/dispose flows.

Changes:

  • Introduces _clearToolbar() to consistently reset toolbarAttached and clear toolbar disposables.
  • Replaces direct toolbarDisposables.clear() calls with _clearToolbar() across rerender/visibility paths.
  • Ensures dispose() triggers detach() cleanup.

Comment thread src/vs/workbench/contrib/notebook/browser/view/cellParts/cellOutput.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scrolling in a notebook leaks memory

3 participants