Fix: subscribe to ItemsChanged before fetching items to avoid missing early events

Agent-Logs-Url: https://github.com/microsoft/PowerToys/sessions/3f380bd3-a56a-4708-80ef-3e7d534c4f6f

Co-authored-by: MuyuanMS <116717757+MuyuanMS@users.noreply.github.com>

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

@@ -52,15 +52,6 @@ public partial class AliasManager : ObservableObject
                     WeakReferenceMessenger.Default.Send<PerformCommandMessage>(topLevelCommand.GetPerformCommandMessage());
                     return true;
                 }
-                else if (_topLevelCommandManager.HasFinishedLoadingCommands)
-                {
-                    // The command no longer exists (e.g. extension uninstalled or command renamed).
-                    // Only remove the orphaned alias once extension loading has fully settled,
-                    // otherwise valid aliases can be deleted while commands are still arriving.
-                    _settingsService.UpdateSettings(
-                        s => s with { Aliases = s.Aliases.Remove(searchText) },
-                        hotReload: false);
-                }
             }
             catch
             {
@@ -118,8 +109,6 @@ public partial class AliasManager : ObservableObject
         }
 
         var keysToRemove = new List<string>();
-        var commandsToDisassociate = new List<string>();
-
         foreach (var kv in aliases)
         {
             // Look for the old aliases for the command, and remove it
@@ -132,32 +121,26 @@ public partial class AliasManager : ObservableObject
             if (newAlias is not null && kv.Value.Alias == newAlias.Alias && kv.Value.CommandId != commandId)
             {
                 keysToRemove.Add(kv.Key);
-                commandsToDisassociate.Add(kv.Value.CommandId);
+
+                // Remove alias from other TopLevelViewModels it may be assigned to
+                var topLevelCommand = _topLevelCommandManager.LookupCommand(kv.Value.CommandId);
+                if (topLevelCommand is not null)
+                {
+                    topLevelCommand.AliasText = string.Empty;
+                }
             }
         }
 
-        // Update settings atomically before touching any ViewModels.
         _settingsService.UpdateSettings(s =>
         {
             var updatedAliases = s.Aliases.RemoveRange(keysToRemove);
 
             if (newAlias is not null)
             {
-                // Use SetItem instead of Add to be resilient to any data inconsistencies
-                // (e.g. orphaned aliases whose key matches the new alias's SearchPrefix).
-                updatedAliases = updatedAliases.SetItem(newAlias.SearchPrefix, newAlias);
+                updatedAliases = updatedAliases.Add(newAlias.SearchPrefix, newAlias);
             }
 
             return s with { Aliases = updatedAliases };
         });
-
-        // After the settings update is complete, notify conflicting ViewModels to clear
-        // their alias text. Doing this after UpdateSettings avoids reentrant calls back
-        // into UpdateAlias (and Save) via the AliasText setter.
-        foreach (var conflictingCommandId in commandsToDisassociate)
-        {
-            var topLevelCommand = _topLevelCommandManager.LookupCommand(conflictingCommandId);
-            topLevelCommand?.ClearAlias();
-        }
     }
 }

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

@@ -129,8 +129,8 @@ public partial class ContentPageViewModel : PageViewModel, ICommandBarContext
 
         UpdateDetails();
 
-        FetchContent();
         model.ItemsChanged += Model_ItemsChanged;
+        FetchContent();
 
         DoOnUiThread(
         () =>

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

@@ -44,9 +44,9 @@ public partial class ContentTreeViewModel(ITreeContent _tree, WeakReference<IPag
             UpdateProperty(nameof(Root));
         }
 
-        FetchContent();
         model.PropChanged += Model_PropChanged;
         model.ItemsChanged += Model_ItemsChanged;
+        FetchContent();
     }
 
     // Theoretically, we should unify this with the one in CommandPalettePageViewModelFactory

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Dock/DockBandViewModel.cs

@@ -209,8 +209,8 @@ public sealed partial class DockBandViewModel : ExtensionObjectViewModel
         var list = command.Model.Unsafe as IListPage;
         if (list is not null)
         {
-            InitializeFromList(list);
             list.ItemsChanged += HandleItemsChanged;
+            InitializeFromList(list);
         }
         else
         {

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

@@ -957,8 +957,8 @@ public partial class ListViewModel : PageViewModel, IDisposable
         Filters?.InitializeProperties();
         UpdateProperty(nameof(Filters));
 
-        FetchItems(true);
         model.ItemsChanged += Model_ItemsChanged;
+        FetchItems(true);
     }
 
     private static IGridPropertiesViewModel? LoadGridPropertiesViewModel(IGridProperties? gridProperties)

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

@@ -47,7 +47,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
     private readonly SupersedingAsyncGate _reloadCommandsGate;
     private CancellationTokenSource _extensionLoadCts = new();
     private CancellationToken _currentExtensionLoadCancellationToken;
-    private int _pendingBackgroundExtensionLoads;
 
     public TopLevelCommandManager(IServiceProvider serviceProvider, ICommandProviderCache commandProviderCache)
     {
@@ -69,8 +68,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
     [ObservableProperty]
     public partial bool IsLoading { get; private set; } = true;
 
-    public bool HasFinishedLoadingCommands => !IsLoading && Volatile.Read(ref _pendingBackgroundExtensionLoads) == 0;
-
     public IEnumerable<CommandProviderWrapper> CommandProviders
     {
         get
@@ -278,7 +275,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
         _extensionLoadCts.Dispose();
         _extensionLoadCts = new();
         _currentExtensionLoadCancellationToken = _extensionLoadCts.Token;
-        Interlocked.Exchange(ref _pendingBackgroundExtensionLoads, 0);
 
         var extensionService = _serviceProvider.GetService<IExtensionService>()!;
         await extensionService.SignalStopExtensionsAsync().ConfigureAwait(false);
@@ -365,7 +361,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
             }
             else if (r.IsTimedOut)
             {
-                TrackBackgroundExtensionLoad(ct);
                 _ = StartExtensionWhenReadyAsync(r.Extension, r.PendingStartTask, r.Stopwatch, ct);
             }
         }
@@ -445,7 +440,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
             // It's weird to repeat the condition here, but it allows the compiler to track nullability of other properties
             if (r.IsTimedOut)
             {
-                TrackBackgroundExtensionLoad(ct);
                 _ = AppendCommandsWhenReadyAsync(r.Wrapper, r.PendingLoadTask, r.Stopwatch, ct);
             }
         }
@@ -505,10 +499,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
         {
             Logger.LogError($"Background start/load of extension {extension.PackageFullName} failed after {sw.ElapsedMilliseconds} ms: {ex}");
         }
-        finally
-        {
-            CompleteBackgroundExtensionLoad(ct);
-        }
     }
 
     private async Task<CommandLoadResult> TryLoadCommandsAsync(CommandProviderWrapper wrapper, CancellationToken ct)
@@ -584,26 +574,6 @@ public sealed partial class TopLevelCommandManager : ObservableObject,
         {
             Logger.LogError($"Background loading of commands and bands from {wrapper.ExtensionHost?.Extension?.PackageFullName} failed after {sw.ElapsedMilliseconds} ms: {ex}");
         }
-        finally
-        {
-            CompleteBackgroundExtensionLoad(ct);
-        }
-    }
-
-    private void TrackBackgroundExtensionLoad(CancellationToken ct)
-    {
-        if (ct == _currentExtensionLoadCancellationToken)
-        {
-            Interlocked.Increment(ref _pendingBackgroundExtensionLoads);
-        }
-    }
-
-    private void CompleteBackgroundExtensionLoad(CancellationToken ct)
-    {
-        if (ct == _currentExtensionLoadCancellationToken)
-        {
-            Interlocked.Decrement(ref _pendingBackgroundExtensionLoads);
-        }
     }
 
     private void ExtensionService_OnExtensionRemoved(IExtensionService sender, IEnumerable<IExtensionWrapper> extensions)

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

@@ -331,22 +331,6 @@ public sealed partial class TopLevelViewModel : ObservableObject, IListItem, IEx
         UpdateTags();
     }
 
-    /// <summary>
-    /// Clears the alias for this command without triggering a recursive save back to settings.
-    /// Called by <see cref="AliasManager"/> after it has already updated settings atomically,
-    /// to avoid reentrant calls into <see cref="AliasManager.UpdateAlias"/> via the <see cref="AliasText"/> setter.
-    /// </summary>
-    internal void ClearAlias()
-    {
-        if (Alias is not null)
-        {
-            Alias = null;
-            OnPropertyChanged(nameof(AliasText));
-            OnPropertyChanged(nameof(IsDirectAlias));
-            UpdateTags();
-        }
-    }
-
     private void FetchAliasFromAliasManager()
     {
         var am = _serviceProvider.GetService<AliasManager>();

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

@@ -1,148 +0,0 @@
-// 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.Collections.Immutable;
-using System.Reflection;
-using System.Threading.Tasks;
-using CommunityToolkit.Mvvm.Messaging;
-using Microsoft.CmdPal.UI.ViewModels.Services;
-using Microsoft.VisualStudio.TestTools.UnitTesting;
-using Moq;
-
-namespace Microsoft.CmdPal.UI.ViewModels.UnitTests;
-
-[TestClass]
-public class AliasManagerTests
-{
-    [TestMethod]
-    public void CheckAlias_DoesNotRemoveAliasWhileInitialLoadIsStillRunning()
-    {
-        var manager = CreateTopLevelCommandManager();
-        try
-        {
-            SetIsLoading(manager, true);
-            SetPendingBackgroundExtensionLoads(manager, 0);
-
-            var alias = new CommandAlias("test", "test.command", true);
-            var settingsService = CreateSettingsService(alias, out var state);
-            var aliasManager = new AliasManager(manager, settingsService.Object);
-
-            var handled = aliasManager.CheckAlias(alias.SearchPrefix);
-
-            Assert.IsFalse(handled);
-            Assert.IsTrue(state.Current.Aliases.ContainsKey(alias.SearchPrefix));
-            settingsService.Verify(s => s.UpdateSettings(It.IsAny<Func<SettingsModel, SettingsModel>>(), false), Times.Never);
-        }
-        finally
-        {
-            Cleanup(manager);
-        }
-    }
-
-    [TestMethod]
-    public void CheckAlias_DoesNotRemoveAliasWhileBackgroundExtensionLoadsArePending()
-    {
-        var manager = CreateTopLevelCommandManager();
-        try
-        {
-            SetIsLoading(manager, false);
-            SetPendingBackgroundExtensionLoads(manager, 1);
-
-            var alias = new CommandAlias("test", "test.command", true);
-            var settingsService = CreateSettingsService(alias, out var state);
-            var aliasManager = new AliasManager(manager, settingsService.Object);
-
-            var handled = aliasManager.CheckAlias(alias.SearchPrefix);
-
-            Assert.IsFalse(handled);
-            Assert.IsTrue(state.Current.Aliases.ContainsKey(alias.SearchPrefix));
-            settingsService.Verify(s => s.UpdateSettings(It.IsAny<Func<SettingsModel, SettingsModel>>(), false), Times.Never);
-        }
-        finally
-        {
-            Cleanup(manager);
-        }
-    }
-
-    [TestMethod]
-    public void CheckAlias_RemovesAliasAfterAllCommandsFinishLoading()
-    {
-        var manager = CreateTopLevelCommandManager();
-        try
-        {
-            SetIsLoading(manager, false);
-            SetPendingBackgroundExtensionLoads(manager, 0);
-
-            var alias = new CommandAlias("test", "test.command", true);
-            var settingsService = CreateSettingsService(alias, out var state);
-            var aliasManager = new AliasManager(manager, settingsService.Object);
-
-            var handled = aliasManager.CheckAlias(alias.SearchPrefix);
-
-            Assert.IsFalse(handled);
-            Assert.IsFalse(state.Current.Aliases.ContainsKey(alias.SearchPrefix));
-            settingsService.Verify(s => s.UpdateSettings(It.IsAny<Func<SettingsModel, SettingsModel>>(), false), Times.Once);
-        }
-        finally
-        {
-            Cleanup(manager);
-        }
-    }
-
-    private static TopLevelCommandManager CreateTopLevelCommandManager()
-    {
-        var serviceProvider = new Mock<IServiceProvider>();
-        serviceProvider.Setup(p => p.GetService(typeof(TaskScheduler))).Returns(TaskScheduler.Default);
-
-        var commandProviderCache = new Mock<ICommandProviderCache>();
-        return new TopLevelCommandManager(serviceProvider.Object, commandProviderCache.Object);
-    }
-
-    private static Mock<ISettingsService> CreateSettingsService(CommandAlias alias, out SettingsState state)
-    {
-        state = new SettingsState
-        {
-            Current = new SettingsModel
-            {
-                Aliases = ImmutableDictionary<string, CommandAlias>.Empty.Add(alias.SearchPrefix, alias),
-            },
-        };
-
-        var settingsService = new Mock<ISettingsService>();
-        var settingsState = state;
-        settingsService.SetupGet(s => s.Settings).Returns(() => settingsState.Current);
-        settingsService
-            .Setup(s => s.UpdateSettings(It.IsAny<Func<SettingsModel, SettingsModel>>(), It.IsAny<bool>()))
-            .Callback((Func<SettingsModel, SettingsModel> transform, bool _) => settingsState.Current = transform(settingsState.Current));
-
-        return settingsService;
-    }
-
-    private sealed class SettingsState
-    {
-        public SettingsModel Current { get; set; } = new();
-    }
-
-    private static void SetIsLoading(TopLevelCommandManager manager, bool isLoading)
-    {
-        var setter = typeof(TopLevelCommandManager)
-            .GetProperty(nameof(TopLevelCommandManager.IsLoading))!
-            .GetSetMethod(nonPublic: true)!;
-        setter.Invoke(manager, [isLoading]);
-    }
-
-    private static void SetPendingBackgroundExtensionLoads(TopLevelCommandManager manager, int count)
-    {
-        typeof(TopLevelCommandManager)
-            .GetField("_pendingBackgroundExtensionLoads", BindingFlags.Instance | BindingFlags.NonPublic)!
-            .SetValue(manager, count);
-    }
-
-    private static void Cleanup(TopLevelCommandManager manager)
-    {
-        WeakReferenceMessenger.Default.UnregisterAll(manager);
-        manager.Dispose();
-    }
-}