Skip to content

Fix duplicate dock band pins #46637

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.

Sign up for GitHub

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

Open
niels9001 wants to merge 2 commits into
base: main
Choose a base branch
from
+30 −11
Open

Fix duplicate dock band pins #46637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
490e489
Deduping
niels9001 Mar 30, 2026
63bb079
Merge branch 'main' into niels9001/deduping-bands
niels9001 Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,17 +499,6 @@ public void UnpinCommand(string commandId, IServiceProvider serviceProvider)
public void PinDockBand(string commandId, IServiceProvider serviceProvider, Dock.DockPinSide side = Dock.DockPinSide.Start, bool? showTitles = null, bool? showSubtitles = null)
{
var settingsService = serviceProvider.GetRequiredService<ISettingsService>();
var settings = settingsService.Settings;
var dockSettings = settings.DockSettings;

// Prevent duplicate pins — check all sections
if (dockSettings.StartBands.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId) ||
dockSettings.CenterBands.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId) ||
dockSettings.EndBands.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId))
{
Logger.LogDebug($"Dock band '{commandId}' from provider '{this.ProviderId}' is already pinned; skipping.");
return;
}

var bandSettings = new DockBandSettings
{
Expand All @@ -523,6 +512,18 @@ public void PinDockBand(string commandId, IServiceProvider serviceProvider, Dock
s =>
{
var dockSettings = s.DockSettings;

// Prevent duplicate pins — check inside the CAS lambda so
// the guard is re-evaluated on every retry against the
// latest snapshot.
if (dockSettings.StartBands.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId) ||
dockSettings.CenterBands.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId) ||
dockSettings.EndBands.Any(b => b.CommandId == commandId && b.ProviderId == this.ProviderId))
{
Logger.LogDebug($"Dock band '{commandId}' from provider '{this.ProviderId}' is already pinned; skipping.");
return s;
}
Comment on lines +519 to +525
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

Because this CAS lambda can now return the original settings when the band is already pinned, the overall PinDockBand call becomes a no-op in that case — but the method still raises CommandsChanged afterwards (outside this hunk). This can trigger unnecessary reload work/UI churn; consider adding a fast-path pre-check before UpdateSettings (while keeping this in-lambda guard) or otherwise only raising CommandsChanged when a pin was actually added.

Copilot uses AI. Check for mistakes.

return s with
{
DockSettings = side switch
Expand Down Expand Up @@ -583,6 +584,14 @@ internal void PinDockBand(TopLevelViewModel bandVm)
Logger.LogDebug($"CommandProviderWrapper.PinDockBand: {ProviderId} - {bandVm.Id}");

var bands = this.DockBandItems.ToList();

// Prevent duplicate entries in the in-memory band list
if (bands.Any(b => b.Id == bandVm.Id))
{
Logger.LogDebug($"Dock band '{bandVm.Id}' already exists in DockBandItems; skipping.");
return;
}

Comment on lines 586 to +594
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

This method materializes DockBandItems into a List before checking for duplicates. If the band is already present, the ToList allocation is wasted; consider checking DockBandItems first and only allocating a List when you actually need to append.

Suggested change
var bands = this.DockBandItems.ToList();
// Prevent duplicate entries in the in-memory band list
if (bands.Any(b => b.Id == bandVm.Id))
{
Logger.LogDebug($"Dock band '{bandVm.Id}' already exists in DockBandItems; skipping.");
return;
}
var existingBands = this.DockBandItems;
// Prevent duplicate entries in the in-memory band list
if (existingBands.Any(b => b.Id == bandVm.Id))
{
Logger.LogDebug($"Dock band '{bandVm.Id}' already exists in DockBandItems; skipping.");
return;
}
var bands = existingBands.ToList();

Copilot uses AI. Check for mistakes.
bands.Add(bandVm);
this.DockBandItems = bands.ToArray();
this.CommandsChanged?.Invoke(this, new ItemsChangedEventArgs());
Expand Down
10 changes: 10 additions & 0 deletions src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,19 @@ private void SetupBands(
ObservableCollection<DockBandViewModel> target)
{
List<DockBandViewModel> newBands = new();
HashSet<string> seen = new(StringComparer.Ordinal);
foreach (var band in bands)
{
var commandId = band.CommandId;

// Skip duplicate entries that share the same provider + command id
var key = $"{band.ProviderId}\0{commandId}";
if (!seen.Add(key))
Comment on lines +88 to +95
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

Building a composite string key here allocates on every iteration of SetupBands. Since this can run on every settings change / DockBands collection change, consider avoiding the per-item string allocation (e.g., use a HashSet of tuples or a custom comparer over (ProviderId, CommandId)).

Suggested change
HashSet<string> seen = new(StringComparer.Ordinal);
foreach (var band in bands)
{
var commandId = band.CommandId;
// Skip duplicate entries that share the same provider + command id
var key = $"{band.ProviderId}\0{commandId}";
if (!seen.Add(key))
HashSet<(string ProviderId, string CommandId)> seen = new();
foreach (var band in bands)
{
var commandId = band.CommandId;
// Skip duplicate entries that share the same provider + command id
if (!seen.Add((band.ProviderId, commandId)))

Copilot uses AI. Check for mistakes.
{
Logger.LogWarning($"Skipping duplicate dock band entry {commandId} for provider {band.ProviderId}");
continue;
Comment on lines +95 to +98
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

Logging a warning for each duplicate entry inside this loop can spam logs if settings.json already contains duplicates (and SetupBands runs repeatedly). Consider downgrading to Debug/Info or aggregating (log once with a count) to keep this path quieter.

Copilot uses AI. Check for mistakes.
}

var topLevelCommand = _topLevelCommandManager.LookupDockBand(commandId);

if (topLevelCommand is null)
Expand Down
Loading