mirror of
https://github.com/microsoft/PowerToys.git
synced 2026-04-03 01:36:31 +02:00
CmdPal: Fix duplicate "Pin to..." context commands on top-level items (#46458)
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 =
<extension>`) 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.
<!-- START COPILOT CODING AGENT TIPS -->
---
📍 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>
This commit is contained in:
@@ -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;
|
||||
|
||||
|
||||
@@ -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))
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user