From 7cb0f3861ad7c5113c2d492dd60d50e1b3de8e43 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Wed, 25 Mar 2026 18:16:18 +0100 Subject: [PATCH] CmdPal: Fix duplicate "Pin to..." context commands on top-level items (#46458) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Top-level commands on the home page showed duplicate pin context entries — e.g., "Pin to Dock" appearing twice, or contradictory "Pin to Dock" + "Unpin from dock" on the same command. ## Summary of the Pull Request When the window is hidden while a sub-page is active, `ShellViewModel.Receive(WindowHiddenMessage)` re-navigates to the root page while `CurrentPage` still points to the sub-page. `GetProviderContextForCommand` therefore returns the sub-page's `ProviderContext` (which has `SupportsPinning = true`, `ProviderId = `) for the new home-page `ListViewModel`. With that wrong context, `UnsafeBuildAndInitMoreCommands` runs for each `ListItemViewModel` wrapping a `TopLevelViewModel` and injects a second set of pin commands — using the wrong provider's dock/top-level state — on top of the ones `TopLevelViewModel.BuildContextMenu()` already injected via `AddMoreCommandsToTopLevel` with the correct per-item provider context. **Changes:** - **`ShellViewModel.cs` (root cause):** Move `isMainPage` evaluation before `providerContext` is computed; use `CommandProviderContext.Empty` when navigating to the root page, regardless of what `CurrentPage` is at that moment. ```csharp var isMainPage = command == _rootPage; var providerContext = isMainPage ? CommandProviderContext.Empty : _appHostService.GetProviderContextForCommand(message.Context, CurrentPage.ProviderContext); ``` - **`CommandPaletteContextMenuFactory.cs` (defensive guard):** In `UnsafeBuildAndInitMoreCommands`, bail early when the page context supports pinning and `commandItem.Model.Unsafe is TopLevelViewModel`. `BuildContextMenu()` on `TopLevelViewModel` already populates pin commands via `AddMoreCommandsToTopLevel` with the item's own provider context; adding them again here is always wrong regardless of how the page context ended up. The `SupportsPinning` check is evaluated first to skip the `.Unsafe` type-test in the common non-pinning case. ## PR Checklist - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [ ] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments The two fixes are complementary: the `ShellViewModel` change prevents the wrong `ProviderContext` from ever reaching the home-page `ListViewModel`; the `CommandPaletteContextMenuFactory` guard ensures `TopLevelViewModel`-backed items are never double-processed even if some other future code path sets the page context incorrectly. The guard in `CommandPaletteContextMenuFactory.UnsafeBuildAndInitMoreCommands` is ordered so that `providerContext.SupportsPinning` (a cheap bool property read) is evaluated before `commandItem.Model.Unsafe is TopLevelViewModel`. This means the field access and type check are skipped entirely for the common non-pinning case, addressing reviewer feedback about unnecessary work on the hot path. ## Validation Steps Performed Manually reproduced by opening CmdPal, navigating into a sub-page (e.g., "Search the Web"), closing the window, reopening, and verifying the context menu for top-level commands shows exactly one "Pin to Dock" (or "Unpin from dock") entry and at most one top-level pin action — no duplicates or contradictory pairs. --- 📍 Connect Copilot coding agent with [Jira](https://gh.io/cca-jira-docs), [Azure Boards](https://gh.io/cca-azure-boards-docs) or [Linear](https://gh.io/cca-linear-docs) to delegate work to Copilot in one click without leaving your project management tool. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: michaeljolley <1228996+michaeljolley@users.noreply.github.com> --- .../ShellViewModel.cs | 15 +++++++++++++-- .../CommandPaletteContextMenuFactory.cs | 13 +++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ShellViewModel.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ShellViewModel.cs index c22126818e..2220b836e5 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ShellViewModel.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ShellViewModel.cs @@ -261,8 +261,20 @@ public partial class ShellViewModel : ObservableObject, return; } + // Determine whether this is the root/home page navigation BEFORE + // computing providerContext. When navigating back to the root page we + // must use an empty provider context so that home-page list items don't + // inherit a pinning-capable context left over from the previous sub-page + // (which can happen e.g. when the window is hidden while on a sub-page). + // isMainPage must be evaluated here; if it were moved inside the + // "if (command is IPage)" block below, it would be too late to affect + // the providerContext that is passed to the new page view-model. + var isMainPage = command == _rootPage; + var host = _appHostService.GetHostForCommand(message.Context, CurrentPage.ExtensionHost); - var providerContext = _appHostService.GetProviderContextForCommand(message.Context, CurrentPage.ProviderContext); + var providerContext = isMainPage + ? CommandProviderContext.Empty + : _appHostService.GetProviderContextForCommand(message.Context, CurrentPage.ProviderContext); _rootPageService.OnPerformCommand(message.Context, CurrentPage.IsRootPage, host); @@ -272,7 +284,6 @@ public partial class ShellViewModel : ObservableObject, { CoreLogger.LogDebug($"Navigating to page"); - var isMainPage = command == _rootPage; _isNested = !isMainPage; _currentlyTransient = message.TransientPage; diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI/CommandPaletteContextMenuFactory.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI/CommandPaletteContextMenuFactory.cs index 7cf91b0d21..108bf2b676 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI/CommandPaletteContextMenuFactory.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI/CommandPaletteContextMenuFactory.cs @@ -71,6 +71,19 @@ internal sealed partial class CommandPaletteContextMenuFactory : IContextMenuFac var supportsPinning = providerContext.SupportsPinning; + // Also bail early for ListItemViewModels that wrap a TopLevelViewModel. + // For those items, TopLevelViewModel.BuildContextMenu() already includes + // the correct pin commands by calling AddMoreCommandsToTopLevel with the + // item's own provider context. Adding them again here (using the page's + // potentially incorrect provider context) would produce duplicate pin + // entries such as two "Pin to Dock" buttons. + // Check SupportsPinning first to avoid the .Unsafe type-check in the + // common non-pinning case. + if (supportsPinning && commandItem.Model.Unsafe is TopLevelViewModel) + { + return results; + } + if (supportsPinning && !string.IsNullOrEmpty(itemId)) {