Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing command duplicates when loading commands from provider #38091

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

guimafelipe
Copy link

There is a bug in command palette where the top level commands from extensions gets duplicated after the extension raises items changed, or when it is requested by CmdPal for any reason. This didn't happen when we kill CmdPal and open it again.

When investigating, I noticed that the UpdateCommandsForProvider method was not deleting the top level commands for the extension.
This seems to be happening because the comparison var isTheSame = wrapper == firstCommand; is not comparing objects of the same type. It was comparing a TopLevelViewModel with a CommandItem, and it was never set to true.

I change it to compare the TopLevelViewModel of both commands, and now it seems to be detecting correctly.

Another option of fix could be comparing the CommandItems.

zadjii-msft and others added 2 commits March 21, 2025 21:22
This regressed in the toplevel view model change. I checked the command
against the commanditem. derp.
@zadjii-msft
Copy link
Member

I took this one step further, and I may just merge that into here: 96c390e

you're right, we can just check the CommandItems

@zadjii-msft zadjii-msft added Product-Command Palette Refers to the Command Palette utility in for .90 labels Mar 22, 2025
@zadjii-msft
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@yeelam-gordon yeelam-gordon requested review from moooyo and Copilot March 24, 2025 03:17

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses a bug in the command palette where top-level commands from extensions are duplicated due to inaccurate object comparison.

  • Changed the object comparison in the UpdateCommandsForProvider method to compare TopLevelViewModel objects instead of underlying command models.
  • Removed outdated comments and unused code paths related to direct access to ICommandItem.
Comments suppressed due to low confidence (1)

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelCommandManager.cs:136

  • Ensure that both 'wrapper' and 'firstCommand' are of the same type before comparing; if not, consider explicitly comparing their relevant TopLevelViewModel properties to avoid any type mismatch issues.
var isTheSame = wrapper == firstCommand;
@guimafelipe
Copy link
Author

Another bug that I found while testing today is that when a user uninstalled or updated an extension, it was still being listed as an enabled extension. This was generating duplicates in the top level commands in case of updating an extension, and resulting in unpredictable behavior and occasional crashes in CmdPal.

Added a fix for that on ExtensionService, removing the uninstalled extensions also from the _enabledExtensions list.

@guimafelipe guimafelipe requested a review from zadjii-msft March 24, 2025 06:00
@crutkas
Copy link
Member

crutkas commented Mar 24, 2025

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in for .90 Product-Command Palette Refers to the Command Palette utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants