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
This commit is contained in:
Michael Jolley
2026-07-15 11:10:49 -05:00
parent b296d05700
commit 0302d0efbf
5 changed files with 94 additions and 9 deletions

View File

@@ -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<string, JSFallbackCommandItemAdapter> _fallbackAdapters = new();
private readonly ConcurrentDictionary<string, JSFallbackCommandItemAdapter> _fallbackAdapters = new();
private IExtensionHost? _host;
private ICommandSettings? _settingsCache;
private bool _settingsQueried;

View File

@@ -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();

View File

@@ -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;
/// </summary>
internal sealed partial class JSListPageProxy : BaseObservable, IListPage
{
private static readonly ConcurrentDictionary<string, JSListPageProxy> 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<JsonRpcConnection, PageRegistry> 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<JSListPageProxy>(this);
}
public event TypedEventHandler<object, IItemsChangedEventArgs>? 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<string, WeakReference<JSListPageProxy>> Pages { get; } = new();
}
}

View File

@@ -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<IFilterItem>();
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;

View File

@@ -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<string>(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);