From a5b1ec812435df71aab351d2aedef25b865cbe73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ji=C5=99=C3=AD=20Pol=C3=A1=C5=A1ek?= Date: Fri, 7 Aug 2026 19:28:40 +0200 Subject: [PATCH] CmdPal SDK: Fix weak command property subscriptions (#49731) ## Summary of the Pull Request This PR fixes a weak-event subscription in the CmdPal toolkit that still captured its owning `CommandItem` through an instance callback. That strong reference defeated the weak listener, while command replacement could also leave a stale handler attached to the outgoing command. - Make the command property-change callback static. - Resolve the owning `CommandItem` through the listener's weak reference. - Explicitly unsubscribe from the outgoing command during replacement. - Retain the detach callback that removes dead listeners from long-lived commands. --- .../CommandItem.cs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/CommandItem.cs b/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/CommandItem.cs index fc2de06548..f408de4a94 100644 --- a/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/CommandItem.cs +++ b/src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/CommandItem.cs @@ -55,8 +55,12 @@ public partial class CommandItem : BaseObservable, ICommandItem var oldTitle = Title; + // Unsubscribe the outgoing command explicitly. OnDetachAction only + // runs once this CommandItem has been collected, so it can't cover + // the case where the command is simply replaced. if (_commandListener is not null) { + _command?.PropChanged -= _commandListener.OnEvent; _commandListener.Detach(); _commandListener = null; } @@ -65,6 +69,16 @@ public partial class CommandItem : BaseObservable, ICommandItem if (value is not null) { + // OnCommandPropertyChanged must be static so the delegate's Target is null. + // An instance method group would bind `this` into OnEventAction, + // giving the listener a strong ref back to this CommandItem and + // defeating the weak reference. That was the actual leak. + // + // OnDetachAction does capture `value`, but that is not a leak: the + // only thing keeping the listener alive is `value`'s own PropChanged + // list, so the two form a cycle the GC reclaims together. Without it + // the listener could never unsubscribe itself, and every collected + // CommandItem would leave a dead handler on a long-lived command. _commandListener = new(this, OnCommandPropertyChanged, listener => value.PropChanged -= listener.OnEvent); value.PropChanged += _commandListener.OnEvent; } @@ -77,10 +91,10 @@ public partial class CommandItem : BaseObservable, ICommandItem } } - private void OnCommandPropertyChanged(CommandItem instance, object source, IPropChangedEventArgs args) + private static void OnCommandPropertyChanged(CommandItem instance, object source, IPropChangedEventArgs args) { // command's name affects Title only if Title wasn't explicitly set - if (args.PropertyName == nameof(ICommand.Name) && string.IsNullOrEmpty(_title)) + if (args.PropertyName == nameof(ICommand.Name) && string.IsNullOrEmpty(instance._title)) { instance.OnPropertyChanged(nameof(Title)); }