From 0302d0efbf624e3c67f8a8b42cdfbd81f26b27c8 Mon Sep 17 00:00:00 2001 From: Michael Jolley Date: Wed, 15 Jul 2026 11:10:49 -0500 Subject: [PATCH] Address Phase 3 review: wire shapes and thread safety - Filter separators read the SDK "separator" flag instead of "_isSeparator" (list-item separators still use "_isSeparator" per the SDK serializer). - Grid layout reads the SDK "type" discriminator instead of "layout". - fallback/updateQuery is sent as a request, matching the protocol and SDK. - Scope listPage/itemsChanged routing per connection via a ConditionalWeakTable keyed by the JsonRpcConnection, holding proxies as weak references so ids from different extensions no longer collide and collected proxies are pruned. - Use a ConcurrentDictionary for the fallback adapter map shared between the request path and the JSON-RPC reader thread. - Add tests covering filter separators, grid type, and fallback updateQuery. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f9b90e37-dcfc-4ea7-8a79-87eb2af072b6 --- .../Models/JSCommandProviderProxy.cs | 3 +- .../Models/JSFallbackCommandItemAdapter.cs | 2 +- .../Models/JSListPageProxy.cs | 32 ++++++++-- .../Models/JSModelMapper.cs | 4 +- .../JSAdapterProxyTests.cs | 62 +++++++++++++++++++ 5 files changed, 94 insertions(+), 9 deletions(-) diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSCommandProviderProxy.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSCommandProviderProxy.cs index f51d47e1ae..0823b4370d 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSCommandProviderProxy.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSCommandProviderProxy.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System; +using System.Collections.Concurrent; using System.Collections.Generic; using System.Text.Json; using System.Text.Json.Nodes; @@ -26,7 +27,7 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider, IDisposab private readonly JSExtensionManifest _manifest; private readonly IconInfo _icon; private readonly Dictionary<(string Message, int State), StatusMessage> _shownStatusMessages = new(); - private readonly Dictionary _fallbackAdapters = new(); + private readonly ConcurrentDictionary _fallbackAdapters = new(); private IExtensionHost? _host; private ICommandSettings? _settingsCache; private bool _settingsQueried; diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSFallbackCommandItemAdapter.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSFallbackCommandItemAdapter.cs index dc824cd47e..139e3c112c 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSFallbackCommandItemAdapter.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSFallbackCommandItemAdapter.cs @@ -98,7 +98,7 @@ internal sealed partial class JSFallbackCommandItemAdapter : BaseObservable, IFa { try { - _connection.SendNotificationAsync( + _connection.SendRequestAsync( "fallback/updateQuery", new JsonObject { ["commandId"] = _commandId, ["query"] = query }, CancellationToken.None).GetAwaiter().GetResult(); diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSListPageProxy.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSListPageProxy.cs index 00f22bafa0..c2a55b5d27 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSListPageProxy.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSListPageProxy.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Runtime.CompilerServices; using System.Text.Json; using System.Text.Json.Nodes; using System.Threading; @@ -24,7 +25,10 @@ namespace Microsoft.CmdPal.UI.ViewModels.Models; /// internal sealed partial class JSListPageProxy : BaseObservable, IListPage { - private static readonly ConcurrentDictionary ProxyRegistry = new(); + // Routing is scoped per connection so that identical page ids from different + // extensions never collide. Proxies are held weakly so they can be collected + // without the registry keeping them alive. + private static readonly ConditionalWeakTable Registries = new(); private readonly string _pageId; private readonly JsonRpcConnection _connection; @@ -36,8 +40,14 @@ internal sealed partial class JSListPageProxy : BaseObservable, IListPage _connection = connection ?? throw new ArgumentNullException(nameof(connection)); _pageData = pageData; - ProxyRegistry[_pageId] = this; - _connection.RegisterNotificationHandler("listPage/itemsChanged", DispatchItemsChanged); + var registry = Registries.GetValue(_connection, static conn => + { + var created = new PageRegistry(); + conn.RegisterNotificationHandler("listPage/itemsChanged", paramsElement => DispatchItemsChanged(created, paramsElement)); + return created; + }); + + registry.Pages[_pageId] = new WeakReference(this); } public event TypedEventHandler? ItemsChanged; @@ -131,7 +141,7 @@ internal sealed partial class JSListPageProxy : BaseObservable, IListPage } } - private static void DispatchItemsChanged(JsonElement paramsElement) + private static void DispatchItemsChanged(PageRegistry registry, JsonElement paramsElement) { try { @@ -142,11 +152,18 @@ internal sealed partial class JSListPageProxy : BaseObservable, IListPage } var pageId = pageProp.GetString(); - if (pageId == null || !ProxyRegistry.TryGetValue(pageId, out var proxy)) + if (pageId == null || !registry.Pages.TryGetValue(pageId, out var weakProxy)) { return; } + if (!weakProxy.TryGetTarget(out var proxy)) + { + // The proxy has been collected; drop the stale entry. + registry.Pages.TryRemove(pageId, out _); + return; + } + var totalItems = -1; if (paramsElement.TryGetProperty("totalItems", out var totalItemsProp) && totalItemsProp.ValueKind == JsonValueKind.Number) @@ -202,4 +219,9 @@ internal sealed partial class JSListPageProxy : BaseObservable, IListPage return items.ToArray(); } + + private sealed class PageRegistry + { + public ConcurrentDictionary> Pages { get; } = new(); + } } diff --git a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSModelMapper.cs b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSModelMapper.cs index 58581895c1..77fc0f6868 100644 --- a/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSModelMapper.cs +++ b/src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSModelMapper.cs @@ -291,7 +291,7 @@ internal static class JSModelMapper return null; } - var layout = GetString(gridProp, "layout") ?? string.Empty; + var layout = GetString(gridProp, "type") ?? string.Empty; var showTitle = GetBool(gridProp, "showTitle", true); var showSubtitle = GetBool(gridProp, "showSubtitle", true); @@ -316,7 +316,7 @@ internal static class JSModelMapper var filters = new List(); foreach (var element in filtersProp.EnumerateArray()) { - if (GetBool(element, "_isSeparator", false)) + if (GetBool(element, "separator", false)) { filters.Add(new Separator(GetString(element, "title") ?? string.Empty)); continue; diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/JSAdapterProxyTests.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/JSAdapterProxyTests.cs index 3053049dc6..0e32bfce29 100644 --- a/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/JSAdapterProxyTests.cs +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/JSAdapterProxyTests.cs @@ -269,6 +269,68 @@ public class JSAdapterProxyTests Assert.IsNotNull(settings!.SettingsPage); } + [TestMethod] + public void ListPage_ExposesFiltersWithSeparatorAndGridType() + { + using var fake = new JSFakeExtension(); + var commandJson = + """ + { + "id": "list-fg", + "pageType": "listPage", + "name": "Filtered", + "gridProperties": { "type": "medium", "showTitle": true }, + "filters": { + "currentFilterId": "all", + "filters": [ + { "id": "all", "name": "All" }, + { "separator": true }, + { "id": "recent", "name": "Recent" } + ] + } + } + """; + fake.OnResult("provider/getCommand", commandJson); + + var provider = CreateProvider(fake); + var page = (IListPage)provider.GetCommand("list-fg")!; + + Assert.IsInstanceOfType(page.GridProperties, typeof(IMediumGridLayout)); + + Assert.IsNotNull(page.Filters); + var filters = page.Filters!.GetFilters(); + Assert.AreEqual(3, filters.Length); + Assert.IsInstanceOfType(filters[0], typeof(IFilter)); + Assert.AreEqual("all", ((IFilter)filters[0]).Id); + Assert.IsInstanceOfType(filters[1], typeof(ISeparatorFilterItem)); + Assert.IsInstanceOfType(filters[2], typeof(IFilter)); + Assert.AreEqual("recent", ((IFilter)filters[2]).Id); + } + + [TestMethod] + public async Task FallbackHandler_SendsUpdateQueryAsRequest() + { + using var fake = new JSFakeExtension(); + fake.OnResult( + "provider/getFallbackCommands", + """[ { "id": "fb-req", "displayTitle": "Initial", "title": "T" } ]"""); + + var received = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + fake.OnRequest("fallback/updateQuery", element => + { + received.TrySetResult(element.GetProperty("query").GetString() ?? string.Empty); + return null; + }); + + var provider = CreateProvider(fake); + var fallback = provider.FallbackCommands()![0]; + + await Task.Run(() => fallback.FallbackHandler.UpdateQuery("typed")); + + var query = await received.Task.WaitAsync(Timeout); + Assert.AreEqual("typed", query); + } + private static void AssertKind(JSFakeExtension fake, IInvokableCommand invokable, string resultJson, CommandResultKind expected) { fake.OnResult("command/invoke", resultJson);