Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Fixes an issue in CmdPal Dock where the same dock band could be pinned multiple times (persisting duplicates in settings and rendering duplicate UI items) by making pinning idempotent under concurrent updates and adding UI-time deduplication.
Changes:
- Moved the “already pinned” guard into the
ISettingsService.UpdateSettingsCAS transform to eliminate the TOCTOU window during retries. - Added an in-memory dedup guard when appending to
CommandProviderWrapper.DockBandItems. - Added render-time deduplication in
DockViewModel.SetupBandsto skip duplicate(ProviderId, CommandId)entries from settings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockViewModel.cs | Deduplicates dock band settings entries at render time to avoid duplicate UI items. |
| src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs | Makes dock-band pinning safe under CAS retries and avoids duplicate in-memory band entries. |
| 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)) |
There was a problem hiding this comment.
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)).
| 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))) |
| if (!seen.Add(key)) | ||
| { | ||
| Logger.LogWarning($"Skipping duplicate dock band entry {commandId} for provider {band.ProviderId}"); | ||
| continue; |
There was a problem hiding this comment.
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.
| 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; | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
| 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(); |
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| // retrieve the new items. | ||
| this.CommandsChanged?.Invoke(this, args); | ||
|
|
||
| internal void PinDockBand(TopLevelViewModel bandVm) |
There was a problem hiding this comment.
This is called only from an unused TopLevelCommandManager.PinDockBand (also unused in v0.98.1)
| ShowSubtitles = showSubtitles, | ||
| }; | ||
|
|
||
| settingsService.UpdateSettings( |
There was a problem hiding this comment.
This is not in 0.98.* line (added in 46451) (I mentioned this in relation of merging this to stable as a hotfix 0.98.2)
Summary of the Pull Request
Summary
Dock bands could be pinned multiple times, resulting in duplicate entries in settings and duplicate UI items in the dock.
Root Cause
CommandProviderWrapper.PinDockBandhad a TOCTOU (time-of-check-time-of-use) race: the duplicate check read a settings snapshot before entering theUpdateSettingsCAS loop. If concurrent or retried calls overlapped, the transform lambda would blindly.Add()without re-checking, producing duplicate entries inDockSettings.StartBands/CenterBands/EndBands.Additionally,
DockViewModel.SetupBandsrendered every entry from settings without deduplication, so any duplicates already persisted insettings.jsonwere faithfully displayed.TeamsExtension didn't set an explicit id
The TeamsExtension was missing a best practice: the dock band's MeetingControlPage had no explicit Id, so CmdPal generated an unstable hash-based ID from the band's title and subtitle. Setting a stable Id makes the band more resilient against CmdPal bugs and ensures pin entries in settings always match the band across title renames or extension updates.
Changes
CommandProviderWrapper.csUpdateSettingslambda so it is re-evaluated on every CAS retry against the latest snapshotCommandProviderWrapper.csPinDockBand(TopLevelViewModel)before appending toDockBandItemsDockViewModel.csHashSetinSetupBandsto skip duplicate(ProviderId, CommandId)entries at render timeValidation
settings.jsonare no longer rendered after theSetupBandsdedupPR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed