Skip to content

Fix listener leak: move onDidChangeConfiguration out of onDidProgressStep callback#314636

Merged
bhavyaus merged 2 commits into
microsoft:mainfrom
SebTardif:fix/getting-started-listener-leak
May 12, 2026
Merged

Fix listener leak: move onDidChangeConfiguration out of onDidProgressStep callback#314636
bhavyaus merged 2 commits into
microsoft:mainfrom
SebTardif:fix/getting-started-listener-leak

Conversation

@SebTardif
Copy link
Copy Markdown
Contributor

@SebTardif SebTardif commented May 6, 2026

Problem

In GettingStartedPage, a configurationService.onDidChangeConfiguration listener for REDUCED_MOTION_KEY was registered inside the onDidProgressStep event callback using this._register(). Each time a walkthrough step was completed, a new permanent config listener accumulated in the disposable store and was never cleaned up.

After completing N walkthrough steps, N identical listeners existed, each toggling the same CSS class on config changes. This is the exact anti-pattern the project's coding guidelines warn about:

Do NOT register a disposable to the containing class if the object is created within a method that is called repeatedly to avoid leaks.

Fix

Move the onDidChangeConfiguration subscription to the constructor, alongside the other config/event listeners, so it is registered exactly once for the lifetime of the page.

Fixes #315168

…Step callback

The onDidChangeConfiguration listener for REDUCED_MOTION_KEY was registered
inside the onDidProgressStep event callback using this._register(). Each time
a walkthrough step was completed, a new permanent config listener accumulated
in the disposable store, never cleaned up. After completing N steps, N
identical listeners existed.

Move the listener registration to the constructor so it is registered exactly
once, consistent with the other config/event listeners in the constructor.
Copilot AI review requested due to automatic review settings May 6, 2026 04:26
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 disposable/listener leak in the Getting Started editor by ensuring the REDUCED_MOTION_KEY configuration listener is registered exactly once for the lifetime of GettingStartedPage, rather than once per walkthrough step progress event.

Changes:

  • Move configurationService.onDidChangeConfiguration (for REDUCED_MOTION_KEY) out of the onDidProgressStep callback.
  • Register that configuration listener in the constructor alongside other long-lived listeners.
Copy link
Copy Markdown
Collaborator

@bhavyaus bhavyaus left a comment

Choose a reason for hiding this comment

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

Thanks!

@bhavyaus bhavyaus enabled auto-merge (squash) May 11, 2026 18:33
@bhavyaus bhavyaus merged commit 37dbf81 into microsoft:main May 12, 2026
29 of 48 checks passed
@vs-code-engineering vs-code-engineering Bot added this to the 1.121.0 milestone May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants