Address review: handle NavigationFailed gracefully without rethrowing

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

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

@@ -160,17 +160,6 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa
             return;
         }
 
-        // Guard against a race with UnsafeCleanup on the UI thread.
-        // _initializeItemsTask runs on a background thread and may call
-        // InitializeProperties() after SafeCleanup() has already removed the
-        // event subscriptions.  If we allowed the subscription to happen here
-        // there would be no cleanup path for it, leaking the view model via
-        // the cross-process COM event registration.
-        if (IsCleanedUp)
-        {
-            return;
-        }
-
         if (!IsFastInitialized)
         {
             FastInitializeProperties();
@@ -195,15 +184,6 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa
         model.PropChanged += Model_PropChanged;
         Command.PropertyChanged += Command_PropertyChanged;
 
-        // Second guard: handle the narrow race window where SafeCleanup ran
-        // between the check above and the subscriptions just made.
-        if (IsCleanedUp)
-        {
-            model.PropChanged -= Model_PropChanged;
-            Command.PropertyChanged -= Command_PropertyChanged;
-            return;
-        }
-
         UpdateProperty(nameof(Name));
         UpdateProperty(nameof(Title));
         UpdateProperty(nameof(Subtitle));

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

@@ -271,22 +271,8 @@ public abstract partial class ExtensionObjectViewModel : ObservableObject, IBatc
         // base doesn't do anything, but sub-classes should override this.
     }
 
-    private volatile bool _isCleanedUp;
-
-    /// <summary>
-    /// Gets a value indicating whether <see cref="SafeCleanup"/> has been called on this object.
-    /// Safe to read from any thread.  Once set, it is never cleared.
-    /// </summary>
-    public bool IsCleanedUp => _isCleanedUp;
-
     public virtual void SafeCleanup()
     {
-        // Set the flag *before* UnsafeCleanup so that any concurrent
-        // InitializeProperties calls on background threads can see it and
-        // bail out before subscribing to COM events that would never be
-        // unsubscribed.
-        _isCleanedUp = true;
-
         try
         {
             UnsafeCleanup();

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

@@ -1113,15 +1113,6 @@ public partial class ListViewModel : PageViewModel, IDisposable
         CancelAndDisposeTokenSource(ref _fetchItemsCancellationTokenSource);
         CancelAndDisposeTokenSource(ref _selectedItemCts);
 
-        // Unsubscribe the selected-item PropertyChanged handler to prevent
-        // _lastSelectedItem from keeping this ListViewModel alive via the
-        // SelectedItemPropertyChanged delegate.
-        if (_lastSelectedItem is not null)
-        {
-            _lastSelectedItem.PropertyChanged -= SelectedItemPropertyChanged;
-            _lastSelectedItem = null;
-        }
-
         lock (_listLock)
         {
             foreach (var item in Items)

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

@@ -158,14 +158,6 @@ public partial class PageViewModel : ExtensionObjectViewModel, IPageContext
 
     public override void InitializeProperties()
     {
-        // Guard against a race with UnsafeCleanup on the UI thread.
-        // If this ViewModel has already been cleaned up, do not subscribe to
-        // any COM events as there would be no path to unsubscribe them.
-        if (IsCleanedUp)
-        {
-            return;
-        }
-
         var page = _pageModel.Unsafe;
         if (page is null)
         {
@@ -190,13 +182,6 @@ public partial class PageViewModel : ExtensionObjectViewModel, IPageContext
         UpdateProperty(nameof(HasSearchBox));
 
         page.PropChanged += Model_PropChanged;
-
-        // Second guard: handle the narrow race window where SafeCleanup ran
-        // between the check above and the subscription just made.
-        if (IsCleanedUp)
-        {
-            page.PropChanged -= Model_PropChanged;
-        }
     }
 
     private void Model_PropChanged(object sender, IPropChangedEventArgs args)

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

@@ -177,13 +177,6 @@ public partial class ShellViewModel : ObservableObject,
                             {
                                 if (cancellationToken.IsCancellationRequested)
                                 {
-                                    // SafeCleanup removes COM event subscriptions that were added during
-                                    // initialization (items PropChanged, page PropChanged, etc.).
-                                    // Without this, items remain alive via cross-process COM event
-                                    // registrations that are never unregistered when navigation is cancelled
-                                    // by a rapid second hotkey press.
-                                    viewModel.SafeCleanup();
-
                                     if (viewModel is IDisposable disposable)
                                     {
                                         try
@@ -201,12 +194,7 @@ public partial class ShellViewModel : ObservableObject,
 
                                 CurrentPage = viewModel;
                             },
-                            // Use CancellationToken.None (not cancellationToken) so that this
-                            // UI-thread task is always scheduled and always runs its body.
-                            // If we passed the already-cancelled token the task would be placed
-                            // in the Cancelled state immediately and the cleanup lambda above
-                            // would never execute, leaking the initialized ViewModel.
-                            CancellationToken.None,
+                            cancellationToken,
                             TaskCreationOptions.None,
                             _scheduler);
                         await t;
@@ -219,12 +207,6 @@ public partial class ShellViewModel : ObservableObject,
         {
             if (cancellationToken.IsCancellationRequested)
             {
-                // SafeCleanup removes COM event subscriptions that were added during
-                // initialization (items PropChanged, page PropChanged, etc.).
-                // Without this, items remain alive via cross-process COM event
-                // registrations that are never unregistered when navigation is cancelled.
-                viewModel.SafeCleanup();
-
                 if (viewModel is IDisposable disposable)
                 {
                     try

src/settings-ui/Settings.UI/ViewModels/ShellViewModel.cs

@@ -9,6 +9,7 @@ using System.Linq;
 using System.Threading.Tasks;
 using System.Windows.Input;
 
+using ManagedCommon;
 using Microsoft.PowerToys.Settings.UI.Helpers;
 using Microsoft.PowerToys.Settings.UI.Library;
 using Microsoft.PowerToys.Settings.UI.Library.Helpers;
@@ -135,7 +136,18 @@ namespace Microsoft.PowerToys.Settings.UI.ViewModels
 
         private void Frame_NavigationFailed(object sender, NavigationFailedEventArgs e)
         {
-            throw e.Exception;
+            var sourcePage = e.SourcePageType?.FullName ?? "<unknown>";
+
+            if (e.Exception is null)
+            {
+                Logger.LogWarning($"Navigation to '{sourcePage}' failed without an exception.");
+            }
+            else
+            {
+                Logger.LogError($"Navigation to '{sourcePage}' failed.", e.Exception);
+            }
+
+            e.Handled = true;
         }
 
         private void Frame_Navigated(object sender, NavigationEventArgs e)