Skip to content

fix: memory leak in menu hidden defaults - #348413

Open
Simon Siefke (SimonSiefke) wants to merge 3 commits into
microsoft:mainfrom
SimonSiefke:fix/memory-leak-menu-hidden-defaults
Open

Simon Siefke (SimonSiefke) wants to merge 3 commits into
microsoft:mainfrom
SimonSiefke:fix/memory-leak-menu-hidden-defaults

Conversation

@SimonSiefke

Copy link
Copy Markdown
Contributor

Details

The menu service caches false defaults under combined menu and command IDs. The cache outlives dynamic command registrations and keeps their key strings after the commands are disposed.

Change

Store only true hidden defaults and delete false entries, since a missing entry already means false.

Before

When registering commands, opening the Command Palette, and disposing the commands 17 times, the cache holds 1,052 redundant false entries. These include 17 disposed-command keys occupying 1,020 bytes. Total string growth is 200,784 bytes and 5,056 strings.

Before

After

The cache has zero visible-default entries, including zero disposed-command keys. Total string growth is 155,664 bytes and 4,380 strings; other UI allocations remain.

After

Test Video

menu-hidden-defaults-native.webm

AI disclosure: Model: GPT 6.1 Sol. Worktime: 15 min

Copilot AI balanced review requested due to automatic review settings November 15, 2026 17:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

No active review issues remain for the menu-service change to reduce retained cache entries.

No open issues 🎉

What changed in this PR

This PR reduces memory retention in the menu service by removing redundant visible-default cache entries, so disposed dynamic commands no longer leave those keys behind.

Changes:

  • Cache only hidden (true) defaults and delete entries for visible (false) defaults.
  • Add regression tests for default changes, user overrides, disposed registrations, and replacement registrations.

No actionable findings remain. Tests were not run during this review.

File Description
src/​vs/​platform/​actions/​test/​common/​menuService.test.ts Adds coverage for cache retention and hidden-state behavior.
src/​vs/​platform/​actions/​common/​menuService.ts Deletes false-default entries instead of retaining them.

Review effort: Balanced

This branch has not been deployed

No deployments
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.

3 participants