CmdPal: Address all reviewer feedback for DetailsViewModel PropChanged subscription

Implement all three suggestions from jiripolasek:

1. Cache INotifyPropChanged check result in _isObservable field to avoid
   redundant QueryInterface calls during cleanup.

2. Clean up the old Details reference AFTER replacing it with the new one
   to minimize the window where Details is null and avoid potential races.

3. Defer PropChanged subscription to InitializeProperties instead of the
   constructor to avoid unnecessary cross-process QI calls during construction.
   Added _isSubscribed flag to prevent multiple subscriptions.

Changes:
- DetailsViewModel: Cache observable check, defer subscription to InitializeProperties,
  track subscription state with _isSubscribed flag
- ListItemViewModel: Reorder Details cleanup to happen after replacement

This addresses all reviewer feedback while maintaining correct resource cleanup
and avoiding expensive QueryInterface operations in constructors.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/DetailsViewModel.cs

@@ -7,9 +7,11 @@ using Microsoft.CommandPalette.Extensions;
 
 namespace Microsoft.CmdPal.UI.ViewModels;
 
-public partial class DetailsViewModel(IDetails _details, WeakReference<IPageContext> context) : ExtensionObjectViewModel(context)
+public partial class DetailsViewModel : ExtensionObjectViewModel
 {
-    private readonly ExtensionObject<IDetails> _detailsModel = new(_details);
+    private readonly ExtensionObject<IDetails> _detailsModel;
+    private readonly bool _isObservable;
+    private bool _isSubscribed;
 
     // Remember - "observable" properties from the model (via PropChanged)
     // cannot be marked [ObservableProperty]
@@ -25,6 +27,82 @@ public partial class DetailsViewModel(IDetails _details, WeakReference<IPageCont
     //   where IDetailsElement = {IDetailsTags, IDetailsLink, IDetailsSeparator}
     public List<DetailsElementViewModel> Metadata { get; private set; } = [];
 
+    public DetailsViewModel(IDetails details, WeakReference<IPageContext> context)
+        : base(context)
+    {
+        _detailsModel = new(details);
+        _isObservable = details is INotifyPropChanged;
+    }
+
+    private void Model_PropChanged(object sender, IPropChangedEventArgs args)
+    {
+        try
+        {
+            FetchProperty(args.PropertyName);
+        }
+        catch (Exception ex)
+        {
+            ShowException(ex);
+        }
+    }
+
+    private void FetchProperty(string propertyName)
+    {
+        var model = _detailsModel.Unsafe;
+        if (model is null)
+        {
+            return;
+        }
+
+        switch (propertyName)
+        {
+            case nameof(IDetails.Title):
+                Title = model.Title ?? string.Empty;
+                UpdateProperty(nameof(Title));
+                break;
+            case nameof(IDetails.Body):
+                Body = model.Body ?? string.Empty;
+                UpdateProperty(nameof(Body));
+                break;
+            case nameof(IDetails.HeroImage):
+                HeroImage = new(model.HeroImage);
+                HeroImage.InitializeProperties();
+                UpdateProperty(nameof(HeroImage));
+                break;
+            case nameof(IDetails.Metadata):
+                RebuildMetadata(model);
+                UpdateProperty(nameof(Metadata));
+                break;
+        }
+    }
+
+    private void RebuildMetadata(IDetails model)
+    {
+        var newMetadata = new List<DetailsElementViewModel>();
+        var meta = model.Metadata;
+        if (meta is not null)
+        {
+            foreach (var element in meta)
+            {
+                DetailsElementViewModel? vm = element.Data switch
+                {
+                    IDetailsSeparator => new DetailsSeparatorViewModel(element, this.PageContext),
+                    IDetailsLink => new DetailsLinkViewModel(element, this.PageContext),
+                    IDetailsCommands => new DetailsCommandsViewModel(element, this.PageContext),
+                    IDetailsTags => new DetailsTagsViewModel(element, this.PageContext),
+                    _ => null,
+                };
+                if (vm is not null)
+                {
+                    vm.InitializeProperties();
+                    newMetadata.Add(vm);
+                }
+            }
+        }
+
+        Metadata = newMetadata;
+    }
+
     public override void InitializeProperties()
     {
         var model = _detailsModel.Unsafe;
@@ -33,6 +111,13 @@ public partial class DetailsViewModel(IDetails _details, WeakReference<IPageCont
             return;
         }
 
+        // Subscribe to PropChanged if the model supports it (only subscribe once)
+        if (_isObservable && !_isSubscribed)
+        {
+            ((INotifyPropChanged)model).PropChanged += Model_PropChanged;
+            _isSubscribed = true;
+        }
+
         Title = model.Title ?? string.Empty;
         Body = model.Body ?? string.Empty;
         HeroImage = new(model.HeroImage);
@@ -57,25 +142,22 @@ public partial class DetailsViewModel(IDetails _details, WeakReference<IPageCont
 
         UpdateProperty(nameof(Size));
 
-        var meta = model.Metadata;
-        if (meta is not null)
+        RebuildMetadata(model);
+    }
+
+    protected override void UnsafeCleanup()
+    {
+        base.UnsafeCleanup();
+
+        if (_isObservable && _isSubscribed)
         {
-            foreach (var element in meta)
+            var model = _detailsModel.Unsafe;
+            if (model is not null)
             {
-                DetailsElementViewModel? vm = element.Data switch
-                {
-                    IDetailsSeparator => new DetailsSeparatorViewModel(element, this.PageContext),
-                    IDetailsLink => new DetailsLinkViewModel(element, this.PageContext),
-                    IDetailsCommands => new DetailsCommandsViewModel(element, this.PageContext),
-                    IDetailsTags => new DetailsTagsViewModel(element, this.PageContext),
-                    _ => null,
-                };
-                if (vm is not null)
-                {
-                    vm.InitializeProperties();
-                    Metadata.Add(vm);
-                }
+                ((INotifyPropChanged)model).PropChanged -= Model_PropChanged;
             }
+
+            _isSubscribed = false;
         }
     }
 }

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ListItemViewModel.cs

@@ -158,11 +158,13 @@ public partial class ListItemViewModel : CommandItemViewModel
                 UpdateProperty(nameof(Type), nameof(IsInteractive));
                 break;
             case nameof(Details):
+                var existingReference = Details;
                 var extensionDetails = model.Details;
                 Details = extensionDetails is not null ? new(extensionDetails, PageContext) : null;
                 Details?.InitializeProperties();
                 UpdateProperty(nameof(Details), nameof(HasDetails));
                 UpdateShowDetailsCommand();
+                existingReference?.SafeCleanup();
                 break;
             case nameof(model.MoreCommands):
                 AddShowDetailsCommands();

src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/DetailsViewModelTests.cs

@@ -0,0 +1,137 @@
+// Copyright (c) Microsoft Corporation
+// The Microsoft Corporation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Threading.Tasks;
+using Microsoft.CommandPalette.Extensions;
+using Microsoft.CommandPalette.Extensions.Toolkit;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace Microsoft.CmdPal.UI.ViewModels.UnitTests;
+
+[TestClass]
+public partial class DetailsViewModelTests
+{
+    private sealed class TestPageContext : IPageContext
+    {
+        public TaskScheduler Scheduler => TaskScheduler.Default;
+
+        public ICommandProviderContext ProviderContext => CommandProviderContext.Empty;
+
+        public void ShowException(Exception ex, string? extensionHint = null)
+        {
+            throw new AssertFailedException($"Unexpected exception from view model: {ex}");
+        }
+    }
+
+    private static WeakReference<IPageContext> CreatePageContext()
+    {
+        var ctx = new TestPageContext();
+        return new WeakReference<IPageContext>(ctx);
+    }
+
+    [TestMethod]
+    public void InitializeProperties_SetsBodyAndTitle()
+    {
+        var details = new Details { Title = "Hello", Body = "World" };
+        var vm = new DetailsViewModel(details, CreatePageContext());
+
+        vm.InitializeProperties();
+
+        Assert.AreEqual("Hello", vm.Title);
+        Assert.AreEqual("World", vm.Body);
+    }
+
+    [TestMethod]
+    public void PropChanged_Body_UpdatesViewModelProperty()
+    {
+        var details = new Details { Title = "Initial", Body = "Initial body" };
+        var vm = new DetailsViewModel(details, CreatePageContext());
+        vm.InitializeProperties();
+
+        // Act — toolkit Details raises PropChanged synchronously on set
+        details.Body = "Updated body";
+
+        // The property value is set synchronously in FetchProperty;
+        // ApplyPendingUpdates flushes the PropertyChanged notification queue.
+        vm.ApplyPendingUpdates();
+
+        Assert.AreEqual("Updated body", vm.Body);
+    }
+
+    [TestMethod]
+    public void PropChanged_Title_UpdatesViewModelProperty()
+    {
+        var details = new Details { Title = "Original", Body = "Text" };
+        var vm = new DetailsViewModel(details, CreatePageContext());
+        vm.InitializeProperties();
+
+        details.Title = "New Title";
+        vm.ApplyPendingUpdates();
+
+        Assert.AreEqual("New Title", vm.Title);
+    }
+
+    [TestMethod]
+    public void PropChanged_Metadata_RebuildsList()
+    {
+        var details = new Details
+        {
+            Title = "T",
+            Body = "B",
+            Metadata = [],
+        };
+        var vm = new DetailsViewModel(details, CreatePageContext());
+        vm.InitializeProperties();
+        Assert.AreEqual(0, vm.Metadata.Count);
+
+        // Act — update metadata with a link element
+        details.Metadata = [new DetailsElement { Key = "link", Data = new DetailsLink("http://example.com", "Example") }];
+        vm.ApplyPendingUpdates();
+
+        Assert.AreEqual(1, vm.Metadata.Count);
+    }
+
+    [TestMethod]
+    public void Cleanup_UnsubscribesFromPropChanged()
+    {
+        var details = new Details { Title = "T", Body = "Original" };
+        var vm = new DetailsViewModel(details, CreatePageContext());
+        vm.InitializeProperties();
+
+        // Act — cleanup unsubscribes, then change should not propagate
+        vm.SafeCleanup();
+        details.Body = "After cleanup";
+
+        Assert.AreEqual("Original", vm.Body);
+    }
+
+    [TestMethod]
+    public void NonObservableDetails_DoesNotThrow()
+    {
+        // IDetails that does NOT implement INotifyPropChanged
+        var details = new NonObservableDetails();
+        var vm = new DetailsViewModel(details, CreatePageContext());
+
+        // Should not throw — just doesn't subscribe to anything
+        vm.InitializeProperties();
+
+        Assert.AreEqual("Static Title", vm.Title);
+        Assert.AreEqual("Static Body", vm.Body);
+    }
+
+    /// <summary>
+    /// A minimal IDetails that does NOT implement INotifyPropChanged.
+    /// </summary>
+    private sealed partial class NonObservableDetails : IDetails
+    {
+        public IIconInfo HeroImage => new IconInfo(string.Empty);
+
+        public string Title => "Static Title";
+
+        public string Body => "Static Body";
+
+        public IDetailsElement[] Metadata => [];
+    }
+}

src/modules/cmdpal/ext/SamplePagesExtension/Pages/SampleLiveDetailsPage.cs

@@ -0,0 +1,103 @@
+// Copyright (c) Microsoft Corporation
+// The Microsoft Corporation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Globalization;
+using System.Timers;
+using Microsoft.CommandPalette.Extensions;
+using Microsoft.CommandPalette.Extensions.Toolkit;
+
+#nullable enable
+
+namespace SamplePagesExtension;
+
+internal sealed partial class SampleLiveDetailsPage : ListPage, IDisposable
+{
+    private readonly Details _clockDetails = new()
+    {
+        Title = "Current Time",
+        Body = "Loading...",
+    };
+
+    private readonly Details _counterDetails = new()
+    {
+        Title = "Count: 0",
+        Body = "Elapsed: 0 seconds",
+    };
+
+    private readonly Details _staticDetails = new()
+    {
+        Title = "Static Details",
+        Body = "This item does not update. Select the items above to see live updates in the details pane.",
+    };
+
+    private readonly ListItem[] _items;
+    private Timer? _timer;
+    private int _counter;
+    private bool _disposed;
+
+    public SampleLiveDetailsPage()
+    {
+        Icon = new IconInfo("\uE916"); // Refresh
+        Name = Title = "Live Updating Details";
+        ShowDetails = true;
+
+        _items = [
+            new ListItem(new NoOpCommand())
+            {
+                Title = "Live Clock",
+                Subtitle = "Details pane shows current time, updating every second",
+                Details = _clockDetails,
+            },
+            new ListItem(new NoOpCommand())
+            {
+                Title = "Counter",
+                Subtitle = "Details pane increments a counter every second",
+                Details = _counterDetails,
+            },
+            new ListItem(new NoOpCommand())
+            {
+                Title = "Static Item",
+                Subtitle = "This item's details do not change",
+                Details = _staticDetails,
+            },
+        ];
+    }
+
+    public override IListItem[] GetItems()
+    {
+        if (_timer is null)
+        {
+            _timer = new Timer(1000);
+            _timer.Elapsed += Timer_Elapsed;
+            _timer.AutoReset = true;
+            _timer.Enabled = true;
+        }
+
+        return _items;
+    }
+
+    private void Timer_Elapsed(object? sender, ElapsedEventArgs e)
+    {
+        _counter++;
+
+        // Updating Details properties fires INotifyPropChanged automatically
+        // (Details extends BaseObservable). DetailsViewModel picks up the change
+        // live without requiring the user to reselect the item.
+        _clockDetails.Body = DateTime.Now.ToString("HH:mm:ss", CultureInfo.CurrentCulture);
+
+        _counterDetails.Title = $"Count: {_counter}";
+        _counterDetails.Body = $"Elapsed: {_counter} second{(_counter == 1 ? string.Empty : "s")}";
+    }
+
+    public void Dispose()
+    {
+        if (!_disposed)
+        {
+            _timer?.Dispose();
+            _timer = null;
+            _disposed = true;
+        }
+    }
+}

src/modules/cmdpal/ext/SamplePagesExtension/SamplesListPage.cs

@@ -24,6 +24,11 @@ public partial class SamplesListPage : ListPage
             Title = "List Page With Details",
             Subtitle = "A list of items, each with additional details to display",
         },
+        new ListItem(new SampleLiveDetailsPage())
+        {
+            Title = "Live Updating Details",
+            Subtitle = "Details pane updates in real time without reselecting",
+        },
         new ListItem(new SectionsIndexPage())
         {
             Title = "List Pages With Sections",