Skip to content

timeline: fix memory leak when toggling pane visibility#304668

Merged
bpasero merged 4 commits intomicrosoft:mainfrom
xingsy97:wt/timeline-visibility-dispose
Mar 27, 2026
Merged

timeline: fix memory leak when toggling pane visibility#304668
bpasero merged 4 commits intomicrosoft:mainfrom
xingsy97:wt/timeline-visibility-dispose

Conversation

@xingsy97
Copy link
Copy Markdown
Member

Problem

TimelinePane.setVisible(true) creates a new DisposableStore assigned to this.visibilityDisposables without first disposing the old one. If setVisible(true) is called twice in succession (e.g. view becomes visible → user switches away and back quickly), the first set of disposables leaks.

Fix

Dispose the existing visibilityDisposables before creating a new store, preventing the leak.

Copilot AI review requested due to automatic review settings March 25, 2026 06:56
@vs-code-engineering vs-code-engineering bot added this to the 1.114.0 milestone Mar 25, 2026
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

Fixes a DisposableStore leak in the Timeline view by ensuring previously-created visibility-scoped disposables are disposed before creating a new set when the pane is shown again.

Changes:

  • Dispose this.visibilityDisposables before reinitializing it in TimelinePane.setVisible(true) to prevent leaking event listeners/disposables across repeated setVisible(true) calls.

@xingsy97 xingsy97 changed the title timeline: fix visibilityDisposables leak on repeated setVisible timeline: fix memory leak when toggling pane visibility Mar 25, 2026
@xingsy97 xingsy97 marked this pull request as ready for review March 25, 2026 07:19
@xingsy97
Copy link
Copy Markdown
Member Author

xingsy97 commented Mar 25, 2026

According to CI error log, it seems the CI pipeline itself is broken, not caused by PR change. Re-run didn't help. Could someone help take a look? Thanks! @bryanchen-d @bpasero @rzhao271

@rzhao271
Copy link
Copy Markdown
Collaborator

We've filed an internal incident report for the GitHub runners. Nothing mitigated yet.

@bpasero bpasero merged commit befae3e into microsoft:main Mar 27, 2026
18 checks passed
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.

5 participants