Mirrors/PowerToys
1
0
Fork 0
mirror of https://github.com/microsoft/PowerToys.git synced 2026-08-29 10:09:43 +02:00
Code Issues Packages Projects Releases Wiki Activity

reword phase-3 comments in my voice

Browse Source 
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ddb0a034-5ea7-4b54-bb62-f925fffa2419
This commit is contained in:
Michael Jolley
2026-08-20 19:10:43 -05:00
parent e6d553b888
commit 6bdf3ff465
20 changed files with 186 additions and 252 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSCommandFactory.cs
View File

@@ -9,9 +9,9 @@ using Microsoft.CommandPalette.Extensions;
namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Creates the appropriate <see cref="ICommand"/> implementation from a JSON
/// command payload. A <c>pageType</c> (or legacy <c>_type</c>) discriminator
/// selects a page proxy; otherwise an invokable command adapter is returned.
/// Builds the right <see cref="ICommand"/> from a JSON command payload.
/// <c>pageType</c>, or legacy <c>_type</c>, selects a page proxy. Everything
/// else becomes an invokable command adapter.
/// </summary>
internal static class JSCommandFactory
{

4
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSCommandItemAdapter.cs
View File

@@ -10,8 +10,8 @@ using Microsoft.CommandPalette.Extensions.Toolkit;
namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Adapts a JSON command item payload to <see cref="ICommandItem"/>. The nested
/// command is resolved lazily so page proxies are only created on demand.
/// Adapts a JSON command item payload to <see cref="ICommandItem"/>.
/// The nested command is resolved lazily so page proxies are created only when needed.
/// </summary>
internal sealed partial class JSCommandItemAdapter : BaseObservable, ICommandItem
{

102
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSCommandProviderProxy.cs
View File

@@ -17,9 +17,9 @@ using Windows.Foundation;
namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Presents a Node.js extension as an <see cref="ICommandProvider"/> by forwarding
/// provider calls over JSON-RPC. Fallback display titles, host status messages,
/// log messages and clipboard requests raised by the extension are handled here.
/// Lets Command Palette treat a Node.js extension as an <see cref="ICommandProvider"/>.
/// Provider calls go over JSON-RPC. Fallback titles, host status, log messages,
/// and clipboard requests from the extension are handled here.
/// </summary>
public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposable
{
@@ -30,22 +30,19 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
private readonly string _displayName;
private readonly IIconInfo _icon;
// Host status messages are tracked by their client-minted statusId so an
// update to the same status refreshes the existing message in place instead
// of creating a duplicate, and a hide targets exactly the right message.
// Host status messages use the statusId minted by the client. That lets an
// update refresh the same message and lets hide target the right one.
private readonly Dictionary<string, StatusMessage> _shownStatusMessages = new();
// Guards _shownStatusMessages. Host status notifications and Dispose can run
// on different threads, so every read, mutation and enumeration of the map
// is serialized to avoid enumerating it while another thread mutates it.
// on different threads, so reads, writes, and enumeration share one gate.
private readonly object _statusLock = new();
private readonly ConcurrentDictionary<string, JSFallbackCommandItemAdapter> _fallbackAdapters = new();
// Host notifications can arrive between this proxy subscribing (in the
// constructor) and the host being attached via InitializeWithHost. Until the
// host is attached they are buffered here in arrival order and replayed once
// the host is bound, so a status or log raised during startup is not dropped.
// A null buffer means the host is attached and notifications run inline.
// Host notifications can arrive after this proxy subscribes but before
// InitializeWithHost attaches the host. Buffer them in arrival order so
// startup status and log messages are not dropped. A null buffer means the
// host is attached and notifications can run inline.
private readonly object _preInitLock = new();
private List<BufferedHostNotification>? _preInitNotifications = new();
private IExtensionHost? _host;
@@ -59,10 +56,8 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
_manifest = manifest ?? throw new ArgumentNullException(nameof(manifest));
_providerMetadata = providerMetadata;
// The initialize handshake response carries the extension's declared
// identity and icon. Prefer those when present so the palette reflects
// what the extension reports at runtime, falling back to the static
// package manifest values when the handshake omits a field.
// Prefer the identity and icon from the initialize handshake when they are
// present. If the extension omits a field, use the package manifest value.
_id = ReadHandshakeString(providerMetadata, "id", "Id") ?? _manifest.Name ?? "unknown";
_displayName = ReadHandshakeString(providerMetadata, "displayName", "DisplayName") ?? _manifest.EffectiveDisplayName;
_icon = ReadHandshakeIcon(providerMetadata) ?? new IconInfo(_manifest.Icon ?? string.Empty);
@@ -78,9 +73,8 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
public IIconInfo Icon => _icon;
// Whether the provider's top-level command set is fixed. The value is carried
// from the initialize handshake metadata; the wire default is true when the
// extension does not specify it.
// True means the provider's top-level command set is fixed. If the extension
// leaves it out of the handshake, the wire default is true.
public bool Frozen => ReadFrozen(_providerMetadata);
public ICommandSettings? Settings
@@ -242,19 +236,16 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
{
_host = host;
// Flip the buffer to null under the same lock that the notification
// handlers use to decide whether to buffer. Writing _host before
// clearing the buffer (and readers taking _preInitLock before reading
// _host) guarantees a handler that runs inline observes the attached
// host rather than a stale null.
// Swap the buffer to null under the same lock used by notification handlers.
// _host is written first so any handler that now runs inline sees the host,
// not a stale null.
buffered = _preInitNotifications ?? new List<BufferedHostNotification>();
_preInitNotifications = null;
}
Logger.LogDebug($"JSCommandProviderProxy initialized with host for {DisplayName}");
// Replay the host notifications that arrived before the host was attached,
// in their original arrival order, now that the host can receive them.
// Replay startup notifications in arrival order now that the host can receive them.
foreach (var notification in buffered)
{
DispatchBufferedHostNotification(notification.Method, notification.Parameters);
@@ -270,20 +261,16 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
_isDisposed = true;
// Detach every notification handler this proxy registered so late
// notifications from the connection are no longer routed here. Process
// teardown and the protocol dispose request are owned by the extension
// service, so this proxy only releases its own subscriptions and host
// references. See the W4 coordination note in the remediation report.
// Detach this proxy's handlers so late connection notifications stop here.
// The extension service owns process teardown and protocol dispose, so this
// proxy only releases its subscriptions and host references.
foreach (var method in RegisteredNotificationMethods)
{
_connection.UnregisterNotificationHandler(method);
}
// Hide any status messages that are still visible so a disposed provider
// does not leave stale status in the host UI. Snapshot and clear the map
// under the lock so a status notification racing dispose cannot mutate it
// while it is being enumerated.
// Hide any status messages still on screen. Snapshot and clear under the lock
// so a status notification racing Dispose cannot change the map during enumeration.
var host = _host;
List<StatusMessage> pendingStatuses;
lock (_statusLock)
@@ -330,11 +317,8 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
_connection.RegisterNotificationHandler("host/copyText", HandleCopyTextNotification);
}
// Buffers a host notification that arrived before InitializeWithHost attached
// the host. Returns true when the notification was buffered (the caller must
// stop) and false when the host is already attached and the caller should
// handle it inline. The params element is cloned so the buffered copy stays
// valid after the connection recycles the source document.
// Buffers a host notification until InitializeWithHost attaches the host.
// The params element is cloned because the connection may recycle the source document.
private bool TryBufferUntilHostAttached(string method, JsonElement paramsElement)
{
lock (_preInitLock)
@@ -349,9 +333,8 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
}
}
// Replays a buffered host notification through its handler once the host is
// attached. The gate is already open, so the handler runs inline instead of
// buffering again.
// Runs the buffered notification now that the host is attached. The open gate
// keeps this pass from buffering the same notification again.
private void DispatchBufferedHostNotification(string method, JsonElement paramsElement)
{
switch (method)
@@ -518,8 +501,8 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
if (!string.IsNullOrEmpty(statusId) &&
_shownStatusMessages.TryGetValue(statusId, out var existing))
{
// Same status shown again: refresh it in place so the host
// keeps a single message rather than stacking duplicates.
// Same statusId again. Update the existing message instead of
// stacking another one.
existing.Message = message;
existing.State = (MessageState)state;
existing.Progress = progress;
@@ -538,14 +521,11 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
_shownStatusMessages[statusId] = statusMessage;
}
// Dispatch the show while still holding the status lock. Dispose
// hides every tracked status and must acquire this same lock, so
// keeping the map insertion and the ShowStatus call atomic
// guarantees a racing dispose either runs entirely before this
// show (and the disposed guard above cancels it) or entirely
// after (and hides the status the show has already dispatched).
// Releasing the lock between the insertion and the call would let
// dispose observe the status and hide it before it was ever shown.
// Keep the map update and ShowStatus under one lock. Dispose uses
// this same lock to hide tracked statuses, so it either runs before
// this show starts or after the shown status is tracked. Releasing
// the lock between those steps would let Dispose hide a status that
// was not shown yet.
_ = host.ShowStatus(statusMessage, ReadStatusContext(paramsElement));
}
}
@@ -640,8 +620,8 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
return string.Empty;
}
// Maps a status progress payload (indeterminate spinner or a percentage) onto
// a toolkit progress state. Returns null when no progress is reported.
// Turns the wire progress payload into the toolkit shape. Null means no
// progress was reported.
private static IProgressState? ReadProgress(JsonElement paramsElement)
{
if (paramsElement.ValueKind != JsonValueKind.Object ||
@@ -668,9 +648,7 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
return progress;
}
// Reads a non-empty string field (id or displayName) from the initialize
// handshake metadata. Returns null when the field is absent or blank so the
// caller falls back to the static package manifest value.
// Blank or missing handshake fields fall back to the package manifest value.
private static string? ReadHandshakeString(JsonElement metadata, string camel, string pascal)
{
if (metadata.ValueKind == JsonValueKind.Object &&
@@ -684,9 +662,7 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
return null;
}
// Reads the icon declared in the initialize handshake metadata. Returns null
// when the handshake omits an icon so the caller falls back to the manifest
// icon rather than replacing it with an empty glyph.
// Missing handshake icons fall back to the manifest icon, not an empty glyph.
private static IIconInfo? ReadHandshakeIcon(JsonElement metadata)
{
if (metadata.ValueKind == JsonValueKind.Object &&
@@ -714,7 +690,7 @@ public sealed partial class JSCommandProviderProxy : ICommandProvider4, IDisposa
}
}
// The wire default when the extension omits the flag is frozen.
// The wire default is frozen when the extension leaves the flag out.
return true;
}

15
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSCommandResultParser.cs
View File

@@ -11,8 +11,8 @@ namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Translates a JSON-RPC command result payload into a toolkit
/// <see cref="ICommandResult"/>, mapping every <c>Kind</c> value (0-7) and its
/// associated arguments. Accepts both PascalCase and camelCase keys.
/// <see cref="ICommandResult"/>. It maps <c>Kind</c> values 0 through 7 and
/// accepts both PascalCase and camelCase keys.
/// </summary>
internal static class JSCommandResultParser
{
@@ -100,9 +100,8 @@ internal static class JSCommandResultParser
toastArgs.Icon = JSModelMapper.ParseIconInfo(iconProp);
}
// Action commands require the live connection used by the command
// adapter. If parsing is used without one, keep the toast usable and
// omit only the unavailable action.
// Toast action commands need the same live connection as the command adapter.
// If no connection is available, keep the toast and skip only the action.
if (connection != null &&
args.ValueKind == JsonValueKind.Object &&
JSModelMapper.TryGetAnyCase(args, "command", "Command", out var commandProp) &&
@@ -111,10 +110,8 @@ internal static class JSCommandResultParser
toastArgs.Command = JSCommandFactory.CreateCommandFromJson(commandProp, connection);
}
// A toast can carry a nested continuation result that the shell executes
// after the toast is shown. Parse it recursively so every nested kind
// (including confirm, which needs the connection for its primary command,
// and even another toast) round-trips faithfully.
// A toast can carry a continuation result for the shell to run after display.
// Parse it recursively so nested confirm and toast results keep working.
if (args.ValueKind == JsonValueKind.Object &&
JSModelMapper.TryGetAnyCase(args, "result", "Result", out var resultProp) &&
resultProp.ValueKind == JsonValueKind.Object)

4
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSCommandSettingsProxy.cs
View File

@@ -10,8 +10,8 @@ namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Exposes a Node.js extension's settings page as <see cref="ICommandSettings"/>.
/// The complete serialized settings page (title, name, icon, details, commands)
/// is presented as a content page rather than reconstructing one from an id.
/// The full settings page payload is kept intact, including title, name, icon,
/// details, and commands.
/// </summary>
internal sealed partial class JSCommandSettingsProxy : ICommandSettings
{

6
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSContentPageProxy.cs
View File

@@ -19,9 +19,9 @@ using Windows.Foundation;
namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Proxy that presents a Node.js extension content page as <see cref="IContentPage"/>.
/// Content is fetched with <c>contentPage/getContent</c>; details and commands are
/// materialized from the page payload.
/// Exposes a Node.js extension content page as <see cref="IContentPage"/>.
/// Content comes from <c>contentPage/getContent</c>. Details and commands come
/// from the page payload.
/// </summary>
internal sealed partial class JSContentPageProxy : JSObservableProxyBase, IContentPage
{

6
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSDynamicListPageProxy.cs
View File

@@ -14,9 +14,9 @@ using Windows.Foundation;
namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Proxy that presents a Node.js extension page as <see cref="IDynamicListPage"/>.
/// Setting the search text forwards a <c>listPage/setSearchText</c> request so the
/// extension can perform its own filtering.
/// Exposes a Node.js extension page as <see cref="IDynamicListPage"/>.
/// Search text is sent with <c>listPage/setSearchText</c> so the extension can
/// filter its own items.
/// </summary>
internal sealed partial class JSDynamicListPageProxy : IDynamicListPage, IDisposable
{

5
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSFallbackCommandItemAdapter.cs
View File

@@ -15,9 +15,8 @@ namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Adapts a JSON fallback command payload to <see cref="IFallbackCommandItem2"/>.
/// The display title can be updated when the extension pushes a
/// <c>command/propChanged</c> notification, and query updates are forwarded via
/// <c>fallback/updateQuery</c>.
/// The display title can change when the extension sends <c>command/propChanged</c>,
/// and query updates go through <c>fallback/updateQuery</c>.
/// </summary>
internal sealed partial class JSFallbackCommandItemAdapter : BaseObservable, IFallbackCommandItem2
{

4
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSFiltersAdapter.cs
View File

@@ -11,8 +11,8 @@ using Microsoft.CommandPalette.Extensions;
namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Adapts a JSON filters payload to <see cref="IFilters"/>. Changing the current
/// filter forwards a <c>listPage/setFilter</c> request to the extension.
/// Adapts a JSON filters payload to <see cref="IFilters"/>.
/// Changing the current filter sends <c>listPage/setFilter</c> to the extension.
/// </summary>
internal sealed partial class JSFiltersAdapter : IFilters
{

12
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSFormContentProxy.cs
View File

@@ -14,9 +14,8 @@ using Microsoft.CommandPalette.Extensions.Toolkit;
namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Proxy that presents a Node.js extension form as <see cref="IFormContent"/>.
/// Submitting the form forwards a <c>form/submit</c> request and maps the
/// response to a toolkit command result.
/// Exposes a Node.js extension form as <see cref="IFormContent"/>.
/// Submit sends <c>form/submit</c> and maps the response to a toolkit command result.
/// </summary>
internal sealed partial class JSFormContentProxy : BaseObservable, IFormContent
{
@@ -31,10 +30,9 @@ internal sealed partial class JSFormContentProxy : BaseObservable, IFormContent
_data = data;
_connection = connection;
// Each serialized form carries a required formId that is unique within its
// page. Capturing it here lets a page with multiple forms, or a form nested
// inside tree content, route its submission back to the correct handler
// instead of relying on the SDK first-form fallback.
// Each form carries a formId that is unique within its page. Keep it so pages
// with multiple forms, or forms nested in tree content, submit to the correct
// handler instead of the SDK first-form fallback.
_formId = JSModelMapper.GetString(_data, "formId") ?? JSModelMapper.GetString(_data, "FormId") ?? string.Empty;
}

4
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSInvokableCommandAdapter.cs
View File

@@ -15,8 +15,8 @@ namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Adapts a JSON command payload to <see cref="ICommand"/> and
/// <see cref="IInvokableCommand"/>. Invoking sends a <c>command/invoke</c>
/// request and maps the response to a toolkit command result.
/// <see cref="IInvokableCommand"/>. Invoke sends <c>command/invoke</c> and maps
/// the response to a toolkit command result.
/// </summary>
internal sealed partial class JSInvokableCommandAdapter : JSObservableProxyBase, IInvokableCommand
{

6
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSListItemAdapter.cs
View File

@@ -10,9 +10,9 @@ using Microsoft.CommandPalette.Extensions.Toolkit;
namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Adapts a JSON list item payload to <see cref="IListItem"/>. The nested
/// command is resolved lazily; tags, details and context items are materialized
/// through <see cref="JSModelMapper"/>.
/// Adapts a JSON list item payload to <see cref="IListItem"/>.
/// The nested command is resolved lazily. Tags, details, and context items are
/// built through <see cref="JSModelMapper"/>.
/// </summary>
internal sealed partial class JSListItemAdapter : BaseObservable, IListItem
{

92
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSListPageProxy.cs
View File

@@ -20,18 +20,16 @@ using Windows.Foundation;
namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Proxy that presents a Node.js extension list page as <see cref="IListPage"/>.
/// Items are fetched with <c>listPage/getItems</c> and the extension can push
/// <c>listPage/itemsChanged</c> notifications to refresh the view.
/// Exposes a Node.js extension list page as <see cref="IListPage"/>.
/// Items come from <c>listPage/getItems</c>. The extension can send
/// <c>listPage/itemsChanged</c> to refresh the view.
/// </summary>
internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage
{
// Routing is scoped per connection so that identical page ids from different
// extensions never collide. Each page id maps to the set of live proxies that
// share it, so a notification reaches every visible reference (the same page
// can be materialized more than once) instead of only the most recent proxy.
// Proxies are held weakly so they can be collected without the registry
// keeping them alive, and dead references are pruned on dispatch and dispose.
// Routing is scoped per connection so identical page ids from different
// extensions do not collide. A page id can have more than one live proxy, so
// notifications go to every visible reference instead of only the newest one.
// Weak references let old proxies be collected, then pruned on dispatch and dispose.
private static readonly ConditionalWeakTable<JsonRpcConnection, PageRegistry> Registries = new();
private readonly string _pageId;
@@ -45,13 +43,10 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage
{
_pageId = pageId ?? throw new ArgumentNullException(nameof(pageId));
// Establish the retained registry first. The factory must stay free of
// side effects: ConditionalWeakTable can invoke it on a thread that then
// loses the race and has its result discarded, so subscribing inside it
// could leave the connection's notification handler bound to a registry
// that is thrown away while proxies register into a different one. The
// handler is wired exactly once below, against the registry actually
// retained.
// Get the retained registry before subscribing. ConditionalWeakTable may run
// the factory on a thread that loses the race and discards its result. If the
// factory subscribed, the connection could keep a handler for a discarded
// registry while proxies register with the retained one.
_registry = Registries.GetValue(Connection, static _ => new PageRegistry());
_registry.EnsureSubscribed(Connection);
@@ -98,10 +93,9 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage
public IGridProperties? GridProperties => JSModelMapper.ParseGridProperties(Data);
// Pagination state is mutable: the extension reports whether more pages
// remain via the getItems / loadMore responses and itemsChanged
// notifications. The seeded page metadata is only the initial value; once the
// extension reports the final page we stop and never issue another loadMore.
// Pagination state can change after construction. The extension reports
// whether more pages remain through getItems, loadMore, and itemsChanged.
// The seeded page metadata is only the starting value.
public bool HasMoreItems
{
get
@@ -156,7 +150,7 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage
{
lock (_stateLock)
{
// The extension has already reported the final page; do not ask again.
// The extension already reported the final page, so do not ask again.
if (_hasMoreItemsState == false)
{
return;
@@ -179,15 +173,12 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage
UpdatePageState(response.Result);
// A loadMore response that does not explicitly report more pages
// means the extension has no further items, so settle HasMoreItems to
// false rather than leaving the previous (true) value in place.
// If loadMore does not report more pages, treat it as the final page
// instead of keeping an old true value.
SettleHasMoreItemsAfterLoadMore(response.Result);
// The host waits on ItemsChanged after LoadMore (see
// ListViewModel.LoadMoreIfNeeded) to re-query GetItems and clear its
// loading state, so raise it once the newly loaded page has been
// folded into the pagination state.
// The host waits for ItemsChanged after LoadMore before it asks for
// items again and clears its loading state.
RaiseItemsChanged(ReadTotalItems(response.Result));
}
catch (Exception ex)
@@ -197,11 +188,9 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage
}
}
// A failed loadMore (an RPC error response or a transport exception) must not
// leave the host stuck in its loading state. Settle paging to false so no
// further LoadMore is issued, and raise ItemsChanged so the host clears its
// loading spinner and re-queries the items it already has. The total is
// reported as unknown (-1) because the failed page delivered no count.
// A failed loadMore must not leave the host stuck loading. Stop paging and
// raise ItemsChanged so the host can clear its spinner and keep the items it
// already has. The total is unknown because the failed page delivered no count.
private void SettleLoadMoreFailure()
{
var changed = false;
@@ -222,8 +211,8 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage
RaiseItemsChanged(-1);
}
// Reads the mutable page state (currently HasMoreItems) from a getItems /
// loadMore response envelope and raises a change notification when it moves.
// Applies mutable page state from getItems or loadMore and raises a change
// notification when HasMoreItems changes.
private void UpdatePageState(JsonElement? envelope)
{
if (!envelope.HasValue || envelope.Value.ValueKind != JsonValueKind.Object)
@@ -264,12 +253,9 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage
}
}
// After a loadMore round trip, the flag is authoritative: an explicit
// hasMoreItems:true keeps paging alive, while false or an omitted flag means
// the extension has delivered its final page and no further LoadMore should
// be issued. This differs from UpdatePageState, which leaves the flag
// untouched when the field is absent so that itemsChanged notifications do
// not accidentally stop paging.
// After loadMore, an explicit hasMoreItems true keeps paging alive. False or a
// missing flag means the extension delivered its final page. itemsChanged keeps
// the old value when the flag is missing, since it can be only a refresh.
private void SettleHasMoreItemsAfterLoadMore(JsonElement? envelope)
{
var hasMore = false;
@@ -361,8 +347,8 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage
totalItems = totalItemsProp.GetInt32();
}
// Snapshot the live proxies and prune any that were collected so the
// registry does not grow without bound as pages come and go.
// Snapshot live proxies and prune collected ones so the registry does
// not grow as pages come and go.
List<JSListPageProxy> targets = new();
lock (proxyRefs)
{
@@ -464,10 +450,9 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage
public ConcurrentDictionary<string, List<WeakReference<JSListPageProxy>>> Pages { get; } = new();
// Wires the connection's itemsChanged handler to this retained registry
// exactly once. Binding here rather than inside the ConditionalWeakTable
// factory guarantees the handler can never target a registry that lost
// the creation race and was discarded.
// Binds the itemsChanged handler to the retained registry once. Binding here,
// instead of inside the ConditionalWeakTable factory, keeps the handler from
// pointing at a registry that lost the creation race.
public void EnsureSubscribed(JsonRpcConnection connection)
{
lock (_subscribeLock)
@@ -477,15 +462,10 @@ internal sealed partial class JSListPageProxy : JSObservableProxyBase, IListPage
return;
}
// Register the handler before marking the registry subscribed.
// Publication must be atomic: no caller may observe the registry
// as subscribed (and then publish its proxy into Pages) until the
// connection is guaranteed to route itemsChanged notifications
// here. Setting the flag first and registering afterwards left a
// window where a notification could arrive after a concurrent
// caller saw the flag but before the handler was bound, and be
// dropped. Holding the lock across both steps closes that window,
// so EnsureSubscribed never returns before the handler is live.
// Register the handler before marking the registry subscribed. No caller
// should see the registry as subscribed and add a proxy until the
// connection can route itemsChanged here. Holding the lock across both
// steps closes the drop window.
connection.RegisterNotificationHandler(
"listPage/itemsChanged",
paramsElement => DispatchItemsChanged(this, paramsElement));

40
src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/Models/JSModelMapper.cs
View File

@@ -17,10 +17,9 @@ using Windows.System;
namespace Microsoft.CmdPal.UI.ViewModels.Models;
/// <summary>
/// Static helpers that materialize JSON-RPC payloads into the Command Palette
/// toolkit data types (icons, tags, details, content, grid layouts, filters).
/// Keeping the JSON key literals inside these helpers (rather than inside
/// properties named after the keys) keeps the adapters analyzer clean.
/// Helpers that turn JSON-RPC payloads into Command Palette toolkit types,
/// including icons, tags, details, content, grid layouts, and filters. Keeping
/// JSON key literals here instead of property names keeps the adapters analyzer clean.
/// </summary>
internal static class JSModelMapper
{
@@ -85,10 +84,9 @@ internal static class JSModelMapper
}
/// <summary>
/// Materializes an icon only when the payload actually carries the field.
/// Returns <c>true</c> when an icon key is present (even if it resolves to an
/// empty glyph), so callers can distinguish an explicitly empty icon from an
/// absent one and fall back to another source when it is absent.
/// Builds an icon only when the payload carries the field. Returns <c>true</c>
/// when an icon key is present, even if it resolves to an empty glyph. That lets
/// callers tell an explicitly empty icon from a missing one.
/// </summary>
internal static bool TryGetIcon(JsonElement parent, string camel, string pascal, out IIconInfo icon)
{
@@ -139,10 +137,8 @@ internal static class JSModelMapper
return new IconInfo(string.Empty);
}
// Exactly one theme variant was supplied here (both-absent already
// returned above). Mirror the supplied variant onto the missing theme so
// the icon renders in both light and dark rather than disappearing in
// whichever theme was omitted.
// Only one theme variant was supplied. Copy it to the missing theme so the
// icon renders in both light and dark.
var supplied = light ?? dark;
if (supplied is null)
{
@@ -291,10 +287,8 @@ internal static class JSModelMapper
var command = JSCommandFactory.CreateCommandFromJson(commandData, connection);
// A context item that omits its own icon inherits the command's icon,
// matching how list items fall back. Reading the item icon with
// TryGetIcon distinguishes an absent icon (fall back) from an explicitly
// empty one (which stays empty and is not replaced).
// A context item without its own icon inherits the command icon, matching
// list item fallback. TryGetIcon tells a missing icon from an explicitly empty one.
var icon = TryGetIcon(element, "icon", "Icon", out var ownIcon)
? ownIcon
: command.Icon ?? new IconInfo(string.Empty);
@@ -317,9 +311,8 @@ internal static class JSModelMapper
item.Subtitle = subtitle;
}
// Context items can carry their own nested context menu via "moreCommands".
// The wire omits the field entirely when empty, so only assign when the
// recursive parse yields children and otherwise leave the default.
// moreCommands is omitted when empty. Leave the default unless the recursive
// parse finds children.
var moreCommands = ParseContextItems(element, "moreCommands", "MoreCommands", connection);
if (moreCommands.Length > 0)
{
@@ -335,10 +328,9 @@ internal static class JSModelMapper
}
/// <summary>
/// Parses a requested context-menu shortcut (modifiers, virtual key and scan
/// code) into a <see cref="KeyChord"/>. Returns <c>false</c> when the field is
/// absent or malformed so the caller leaves the default (no) shortcut. Never
/// throws on unexpected shapes.
/// Parses a requested context menu shortcut into a <see cref="KeyChord"/>.
/// Returns <c>false</c> when the field is absent or malformed so the caller can
/// leave the default shortcut. Unexpected shapes do not throw.
/// </summary>
internal static bool TryParseKeyChord(JsonElement parent, out KeyChord keyChord)
{
@@ -353,7 +345,7 @@ internal static class JSModelMapper
var vkey = ReadInt32OrNull(chordProp, "vkey", "Vkey") ?? ReadInt32OrNull(chordProp, "vKey", "VKey");
if (vkey is null)
{
// A shortcut with no virtual key is not actionable; treat as absent.
// Without a virtual key, the shortcut cannot be invoked.
return false;
}

12
src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/JSAdapterProxyTests.cs
View File

@@ -13,8 +13,8 @@ using Microsoft.VisualStudio.TestTools.UnitTesting;
namespace Microsoft.CmdPal.UI.ViewModels.UnitTests;
/// <summary>
/// Exercises the JSON-RPC adapters and proxies end to end against an in-memory
/// fake extension driving a real JsonRpcConnection.
/// Exercises JSON-RPC adapters and proxies against an in-memory fake extension
/// running through a real JsonRpcConnection.
/// </summary>
[TestClass]
public class JSAdapterProxyTests
@@ -159,7 +159,7 @@ public class JSAdapterProxyTests
Assert.AreEqual("DetailTitle", items[0].Details!.Title);
Assert.AreEqual(1, items[0].MoreCommands.Length);
// Separator items expose no command.
// Separator items have no command.
Assert.IsNull(items[1].Command);
Assert.AreEqual("Item B", items[2].Title);
}
@@ -197,19 +197,19 @@ public class JSAdapterProxyTests
Assert.AreEqual(2, items.Length);
// The root item carries a first-level nested command.
// The root item has the first nested command.
var firstLevel = items[0].MoreCommands;
Assert.AreEqual(1, firstLevel.Length);
var firstLevelCommand = (ICommandContextItem)firstLevel[0];
Assert.AreEqual("Level 1", firstLevelCommand.Title);
// That first-level command carries its own second-level nested command.
// That command has a second nested command.
Assert.AreEqual(1, firstLevelCommand.MoreCommands.Length);
var secondLevelCommand = (ICommandContextItem)firstLevelCommand.MoreCommands[0];
Assert.AreEqual("Level 2", secondLevelCommand.Title);
Assert.AreEqual(0, secondLevelCommand.MoreCommands.Length);
// The leaf item with no moreCommands yields no children.
// The leaf item has no moreCommands, so it yields no children.
Assert.AreEqual(0, items[1].MoreCommands.Length);
}

32
src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/JSAdapterRemediationTests.Round2.cs
View File

@@ -13,16 +13,16 @@ using Microsoft.VisualStudio.TestTools.UnitTesting;
namespace Microsoft.CmdPal.UI.ViewModels.UnitTests;
/// <summary>
/// Round-2 phase-3 adapter remediation (r2-p3-01 pagination drive-through,
/// r2-p3-03 registry init ordering, r2-p3-04 context icon fallback and
/// r2-p3-05 status disposal synchronization). Shared helpers and the recording
/// Round 2, phase 3 adapter remediation. Covers r2-p3-01 pagination load,
/// r2-p3-03 registry init ordering, r2-p3-04 context icon fallback, and
/// r2-p3-05 status disposal synchronization. Shared helpers and the recording
/// host live in the primary <see cref="JSAdapterRemediationTests"/> partial.
/// </summary>
public partial class JSAdapterRemediationTests
{
// r2-p3-01: LoadMore folds the loaded page into the pagination state and
// raises ItemsChanged so the host re-queries GetItems and surfaces the
// appended items, then stops once the extension reports the final page.
// r2-p3-01: LoadMore folds the loaded page into pagination state and raises
// ItemsChanged so the host asks GetItems again and sees the appended items.
// It stops once the extension reports the final page.
[TestMethod]
public async Task ListPage_LoadMoreRaisesItemsChangedAndAppendsItems()
{
@@ -68,8 +68,8 @@ public partial class JSAdapterRemediationTests
Assert.AreEqual(2, secondItems.Length);
}
// r2-p3-01: a loadMore response that omits hasMoreItems is treated as the
// final page so no further LoadMore is issued.
// r2-p3-01: a loadMore response with no hasMoreItems flag is the final page,
// so no further LoadMore is issued.
[TestMethod]
public async Task ListPage_LoadMoreWithoutHasMoreItemsStopsPaging()
{
@@ -100,10 +100,9 @@ public partial class JSAdapterRemediationTests
Assert.AreEqual(1, Volatile.Read(ref loadMoreCount));
}
// r2-p3-03: proxies created concurrently on one connection share the retained
// registry, so the itemsChanged handler is bound to the registry the proxies
// register into and the notification is delivered rather than lost to a
// discarded registry.
// r2-p3-03: concurrent proxies on one connection share the retained registry.
// The itemsChanged handler binds to the same registry the proxies use, so the
// notification is not lost to a discarded registry.
[TestMethod]
public async Task ListPage_ConcurrentInitBindsItemsChangedToRetainedRegistry()
{
@@ -175,11 +174,10 @@ public partial class JSAdapterRemediationTests
Assert.AreEqual("OWN", item.Icon!.Light.Icon);
}
// r2-p3-05: status notifications racing dispose never enumerate the status
// map while it is mutated. Pre-fix this could throw a collection-modified
// exception out of Dispose; the synchronization keeps it safe. This is a
// best-effort concurrency guard: the race is timing dependent, so the check
// is that Dispose completes without throwing and leaves consistent counts.
// r2-p3-05: status notifications racing Dispose never enumerate the status map
// while it is changing. Before the fix, Dispose could throw when the collection
// changed mid-enumeration. This check is best effort because the race depends
// on timing. Dispose should complete and leave consistent counts.
[TestMethod]
public async Task Status_ConcurrentNotificationsDuringDisposeStaySynchronized()
{

39
src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/JSAdapterRemediationTests.Round3.cs
View File

@@ -16,10 +16,10 @@ using Windows.Foundation;
namespace Microsoft.CmdPal.UI.ViewModels.UnitTests;
/// <summary>
/// Round-3 phase-3 adapter remediation. Covers the swallowed LoadMore failure
/// (r3-p3-01), atomic registry subscription (r3-p3-02), pre-init host
/// notification buffering (r3-p3-03), handshake metadata adoption (r3-p3-04),
/// pending-show versus dispose ordering (r3-p3-05) and single-theme icon
/// Round 3, phase 3 adapter remediation. Covers swallowed LoadMore failures
/// (r3-p3-01), registry subscription ordering (r3-p3-02), host notification
/// buffering before InitializeWithHost (r3-p3-03), handshake metadata adoption
/// (r3-p3-04), show versus dispose ordering (r3-p3-05), and single-theme icon
/// mirroring (r3-p3-06). Shared helpers live in the primary
/// <see cref="JSAdapterRemediationTests"/> partial.
/// </summary>
@@ -61,8 +61,7 @@ public partial class JSAdapterRemediationTests
}
// r3-p3-02: the itemsChanged handler is bound before the constructor returns,
// so a notification pushed immediately after subscribing is delivered rather
// than dropped because the connection had no handler yet.
// so a notification pushed immediately after subscribing is not dropped.
[TestMethod]
public async Task ListPage_NotificationRightAfterSubscriptionIsDelivered()
{
@@ -81,8 +80,8 @@ public partial class JSAdapterRemediationTests
}
// r3-p3-02: constructing proxies concurrently on one connection keeps the
// subscription atomic, so a notification is delivered no matter which
// constructor won the registration race.
// subscription under one lock, so the notification is delivered no matter
// which constructor wins the registration race.
[TestMethod]
public async Task ListPage_ConcurrentSubscriptionNeverDropsNotification()
{
@@ -114,9 +113,8 @@ public partial class JSAdapterRemediationTests
}
// r3-p3-03: a host notification that arrives before InitializeWithHost is
// buffered and replayed once the host is attached, so a startup status is not
// dropped. The itemsChanged pushed afterward drains behind it (FIFO), proving
// the status was processed while the host was still detached.
// buffered and replayed once the host is attached. The itemsChanged pushed
// afterward runs behind it, which proves FIFO order.
[TestMethod]
public async Task Provider_HostNotificationBeforeInitIsReplayedAfterInit()
{
@@ -135,8 +133,8 @@ public partial class JSAdapterRemediationTests
["message"] = new JsonObject { ["Message"] = "Booting", ["State"] = 0 },
});
// The itemsChanged handler needs no host, so observing it confirms the
// preceding showStatus has already been processed (and buffered).
// The itemsChanged handler needs no host. Seeing it means the earlier
// showStatus has already been processed and buffered.
await fake.PushNotificationAsync("provider/itemsChanged", new JsonObject { ["totalItems"] = 1 });
await ordered.Task.WaitAsync(Timeout);
@@ -186,9 +184,9 @@ public partial class JSAdapterRemediationTests
Assert.AreEqual("Test Extension", provider.DisplayName);
}
// r3-p3-05: a pending show holds the status lock, so a dispose-triggered hide
// cannot run until the show has been dispatched. The recorded order is always
// show then hide, never hide before show.
// r3-p3-05: a pending show holds the status lock, so a hide from Dispose
// cannot run until the show has been dispatched. The order is always show
// then hide.
[TestMethod]
public async Task Provider_DisposeHidesStrictlyAfterPendingShow()
{
@@ -209,8 +207,8 @@ public partial class JSAdapterRemediationTests
var dispose = Task.Run(() => provider.Dispose());
// The pending show still holds the status lock, so dispose cannot hide the
// status yet. Give it time to prove it stays blocked rather than racing.
// The pending show still holds the status lock, so Dispose cannot hide the
// status yet. Give it time to prove the hide waits.
await Task.Delay(200);
Assert.AreEqual(0, host.HiddenCount);
@@ -265,9 +263,8 @@ public partial class JSAdapterRemediationTests
}
/// <summary>
/// A host whose ShowStatus blocks until released, and which records the order
/// of show and hide calls. Used to make the pending-show-versus-dispose
/// ordering deterministic.
/// A host whose ShowStatus blocks until released and records show and hide
/// order. This makes the show versus dispose ordering deterministic.
/// </summary>
private sealed partial class OrderedGatingHost : IExtensionHost, IDisposable
{

32
src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/JSAdapterRemediationTests.cs
View File

@@ -18,11 +18,10 @@ using Windows.System;
namespace Microsoft.CmdPal.UI.ViewModels.UnitTests;
/// <summary>
/// Covers the phase-3 adapter remediation items (form identity, toast
/// continuation, page routing, pagination, settings metadata, context
/// shortcuts, icon fallback, status identity, provider dispose, frozen and
/// accent color). The parser assertions consume the shared TS SDK wire
/// fixtures so the C# adapters stay byte-compatible with the SDK.
/// Covers phase 3 adapter fixes: form identity, toast continuation, page
/// routing, pagination, settings metadata, context shortcuts, icon fallback,
/// status identity, provider dispose, frozen state, and accent color. Parser
/// assertions use shared TS SDK wire fixtures so the C# adapters match the SDK.
/// </summary>
[TestClass]
public partial class JSAdapterRemediationTests
@@ -163,7 +162,7 @@ public partial class JSAdapterRemediationTests
Assert.AreEqual(1, loadMoreCount);
}
// p3-05: the settings page exposes the full serialized metadata, not just id.
// p3-05: the settings page exposes full metadata, not just id.
[TestMethod]
public void Settings_ExposesFullPageMetadata()
{
@@ -277,7 +276,7 @@ public partial class JSAdapterRemediationTests
Assert.AreNotEqual("CMDICON", adapter.Icon.Light.Icon);
}
// p3-07: light and dark icon variants both round-trip from the shared fixture.
// p3-07: light and dark icon variants both match the shared fixture.
[TestMethod]
public void Icon_LightAndDarkVariantsFromFixture()
{
@@ -370,7 +369,7 @@ public partial class JSAdapterRemediationTests
Assert.AreEqual(1, host.ShownCount);
}
// p3-10: frozen and non-frozen providers surface their actual value.
// p3-10: frozen and non-frozen providers return their actual value.
[TestMethod]
public void Frozen_ReflectsProviderMetadata()
{
@@ -387,7 +386,7 @@ public partial class JSAdapterRemediationTests
Assert.IsTrue(defaultProvider.Frozen);
}
// p3-11: a page with accentColor surfaces the parsed color; a page without
// p3-11: a page with accentColor returns the parsed color; a page without
// stays NoColor.
[TestMethod]
public void AccentColor_SurfacesParsedColorAndDefaultsToNoColor()
@@ -406,10 +405,9 @@ public partial class JSAdapterRemediationTests
Assert.IsFalse(withoutAccent.AccentColor.HasValue);
}
// p3-12: a tag whose color components are out of byte range or fractional must
// not throw out of the WinRT-visible Tags getter. Every numeric component that
// does not fit is dropped to its default, and the surrounding item metadata
// (title, subtitle, other tags) is preserved rather than collapsing to Error.
// p3-12: a tag with fractional or out-of-range color components must not throw
// from the WinRT-visible Tags getter. Invalid components fall back to defaults,
// while the rest of the item metadata stays intact.
[TestMethod]
public void Tags_OutOfRangeOrFractionalColorComponentsDefaultInsteadOfThrowing()
{
@@ -425,8 +423,8 @@ public partial class JSAdapterRemediationTests
["text"] = "over",
["foreground"] = new JsonObject
{
// 256 overflows a byte and 1.5 is fractional; both would throw
// from JsonElement.GetByte, so they must fall back to defaults.
// 256 overflows a byte and 1.5 is fractional. JsonElement.GetByte
// would throw, so both must fall back to defaults.
["r"] = 256,
["g"] = 1.5,
["b"] = 12,
@@ -510,8 +508,8 @@ public partial class JSAdapterRemediationTests
private static JsonNode ParseNode(string json) => JsonNode.Parse(json)!;
/// <summary>
/// Records the status and log calls a provider makes on its host so tests can
/// assert status identity, update-in-place, and hide-on-dispose behavior.
/// Records status and log calls from a provider host so tests can assert status
/// identity, in-place updates, and hiding during Dispose.
/// </summary>
private sealed partial class RecordingExtensionHost : IExtensionHost
{

10
src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/JSFakeExtension.cs
View File

@@ -17,9 +17,9 @@ using Microsoft.CmdPal.UI.ViewModels.Services.JsonRpc;
namespace Microsoft.CmdPal.UI.ViewModels.UnitTests;
/// <summary>
/// In-memory fake Node.js extension. It drives a real <see cref="JsonRpcConnection"/>
/// over paired pipes, answering requests with canned JSON responses and pushing
/// notifications on demand. No external process is started.
/// Fake Node.js extension that runs in memory. It drives a real
/// <see cref="JsonRpcConnection"/> over paired pipes, answers requests with
/// canned JSON, and can push notifications. No external process is started.
/// </summary>
internal sealed class JSFakeExtension : IDisposable
{
@@ -49,8 +49,8 @@ internal sealed class JSFakeExtension : IDisposable
public void OnResult(string method, string resultJson) => _handlers[method] = _ => JsonNode.Parse(resultJson);
// Answers a request method with a JSON-RPC error response so tests can drive
// the error-handling branches of the proxies.
// Answers a request method with a JSON-RPC error so tests can drive proxy
// error handling.
public void OnError(string method, int code, string message) => _errors[method] = (code, message);
public async Task PushNotificationAsync(string method, JsonNode? parameters)

7
src/modules/cmdpal/Tests/Microsoft.CmdPal.UI.ViewModels.UnitTests/JSStatusNotificationTests.cs
View File

@@ -14,10 +14,9 @@ using Windows.Foundation;
namespace Microsoft.CmdPal.UI.ViewModels.UnitTests;
/// <summary>
/// Exercises the host/showStatus notification path end to end against an in-memory
/// fake extension, verifying that the host reads the SDK status wire shape correctly:
/// the indeterminate progress payload and the Pascal-case State severity nested in
/// the message object.
/// Exercises the host/showStatus notification path against an in-memory fake
/// extension. It verifies the SDK status wire shape: indeterminate progress and
/// PascalCase State severity nested in the message object.
/// </summary>
[TestClass]
public partial class JSStatusNotificationTests