* feat: apply auto-refresh setting live + diff-aware list-changed indicator (#1444) Two follow-ups to #1402. 1. Live setting application — InspectorClient gains setServerSettings() (on the protocol + fake); App pushes edited settings onto the live client when the Server Settings modal closes for the connected server, so toggling auto-refresh-on-list-changed takes effect without a reconnect (the managed state reads getServerSettings() at notification time). 2. Diff-aware indicator — in flag-only mode, on */list_changed the managed Tools/Prompts/Resources state now fetches the new list and compares it to the current one, lighting the indicator only when it actually differs. The displayed list stays put until the user pulls via Refresh (pull-on-demand preserved). Servers like everything re-send identical lists on list_changed; this suppresses the false-positive indicator. Refactors each manager's refresh into fetch + apply helpers shared with the peek path. Trade-off: item 2 reintroduces a fetch on */list_changed (to compare), but unlike the pre-#1402 behavior it never replaces the displayed list under the user — only the indicator reflects a real change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(core): make the diff-aware peek clear the indicator on revert (#1444 review) peekForChange now sets the list-changed flag to whether the server list differs from the displayed one (setListChanged(differs)) instead of only ever setting it true. So a later notification that reverts the server back to the displayed list clears the indicator — nothing left to pull. Adds a clear-on-revert regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(core): consolidate managed-list state managers onto a shared base (#1444) The four per-primitive list managers (tools, prompts, resources, resource templates) were near-identical copies of the same fetch/apply/peek/connect/ disconnect/list_changed machinery, a standing drift risk. Extract a generic ManagedListState<T, M> base parameterized by a config (change event, list_changed notification, capability key, page fetcher, and whether the list drives an indicator). Each manager is now a ~40-line subclass: its config plus a typed getter alias. Public contracts are unchanged — same per-manager *Change / listChangedChange events and get*/refresh/getListChanged/clearListChanged methods — so the hooks and all existing tests pass untouched. Templates opt out of the indicator (supportsIndicator: false): a list_changed auto-refreshes only when the server opts in, else does nothing. Net -245 lines. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * perf(core): coalesce bursts of list_changed peeks (#1444 review) A rapid burst of */list_changed notifications fired one concurrent paginated fetch per notification. Guard peekForChange with an in-flight flag: while a peek is fetching, a new notification queues a single re-run instead of launching a concurrent fetch, and the running peek loops once more on completion to capture the latest server state. Collapses a burst into sequential peeks. Adds a coalescing test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(core): debounce list_changed bursts into a single fetch (#1444) The in-flight guard only coalesced fetches that actually overlapped — when a server (e.g. everything) emits a burst of */list_changed spaced just far enough apart that each list call returns before the next notification, we still fired one list call per notification. Add a real debounce in the ManagedListState base: a notification (re)starts a timer and the refresh/peek runs once the burst settles, collapsing N notifications into one fetch. The in-flight coalescing guard stays as a second line of defense for a post-debounce notification landing during a slow paginated fetch. The delay is injectable (DEFAULT_LIST_CHANGED_DEBOUNCE_MS = 250; subclasses take an optional constructor arg) so tests run with 0 for fast, determinist ic behavior. Timer is cleared on disconnect, unsubscribe, and destroy. Adds debounce + in-flight regression tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(core): extend the in-flight guard to the auto-refresh path (#1444 review) The overlap guard was only on the peek path; a burst-during-a-slow auto- refresh could let a slower older fetch clobber a newer list (applyItems is last-write-wins). Lift the guard from peekForChange up to runListChanged so it wraps both the auto-refresh and peek actions: while one is fetching, a new post-debounce notification queues a single re-run. Adds an auto-refresh coalescing test alongside the peek one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(core): light the indicator without fetching when auto-refresh is off (#1444) Per maintainer feedback: with auto-refresh off, a list_changed should not issue any list call. The diff-aware peek (fetch-to-compare) meant one tools/list went out per settled burst even with auto-refresh off, which the maintainer doesn't want. Revert flag-only mode to lighting the indicator blindly on a (debounced) list_changed — no network — and the user pulls the new list via Refresh. Trade-off (accepted): the indicator lights even when the server re-sent an identical list (the everything-server case), since we no longer fetch to compare. Keeps the debounce (one indicator light per burst), the auto-refresh path + its in-flight guard, the live setServerSettings, and the ManagedListState consolidation. Removes peekForChange and its tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(core): scrub stale peek docstrings after the blind-indicator revert (#1445 review) The diff-aware revert removed peekForChange's body but left its docstring, which orphaned onto destroy() and described fetch-to-compare/clear-on-revert behavior that no longer exists. Replace it with a one-line destroy() doc, and update clearListChanged's doc (peek -> blind light). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(core): finish scrubbing stale peek references after the revert (#1445 review) Round-7's scrub missed four more peek/fetch-to-compare mentions in managedListState.ts (supportsIndicator, fetchAll, the debounce-default and onListChanged comments) plus six 'async peek' comments in the state tests. Update them all to the blind-indicator semantics. Doc/comment-only; no behavior change. grep -i peek over core/mcp/state now returns nothing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
b0a024269e
commit
721dae08c6
1 changed files with 597 additions and 560 deletions
|
|
@ -1,133 +1,58 @@
|
|||
/**
|
||||
* ManagedResourceTemplatesState: holds the full resource template list, loaded
|
||||
* on connect and on demand via refresh().
|
||||
* ManagedResourceTemplatesState: holds the full resource template list, in sync
|
||||
* with the server. A thin subclass of ManagedListState (#1444).
|
||||
*
|
||||
* Ported from v1.5/main. v2 substitutes `InspectorClientProtocol` for the
|
||||
* concrete `InspectorClient` since the runtime class is not yet ported.
|
||||
* Templates have no list-changed indicator of their own — the Resources
|
||||
* screen's indicator (driven by `resourcesListChanged`) covers them, and its
|
||||
* Refresh re-fetches templates too. So `supportsIndicator` is false: a
|
||||
* `resourceTemplatesListChanged` auto-refreshes only when the server opts in
|
||||
* via `autoRefreshOnListChanged`; otherwise it does nothing and the user pulls
|
||||
* via the Resources Refresh (#1402).
|
||||
*/
|
||||
|
||||
import type { InspectorClientProtocol } from "../inspectorClientProtocol.js";
|
||||
import type { ResourceTemplate } from "@modelcontextprotocol/sdk/types.js";
|
||||
import { TypedEventTarget } from "../typedEventTarget.js";
|
||||
|
||||
const MAX_PAGES = 100;
|
||||
import {
|
||||
ManagedListState,
|
||||
DEFAULT_LIST_CHANGED_DEBOUNCE_MS,
|
||||
} from "./managedListState.js";
|
||||
|
||||
export interface ManagedResourceTemplatesStateEventMap {
|
||||
resourceTemplatesChange: ResourceTemplate[];
|
||||
/**
|
||||
* Carried only to satisfy the ManagedListState base; templates have no
|
||||
* indicator, so this never fires.
|
||||
*/
|
||||
listChangedChange: boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
* State manager that keeps a full resource template list. Subscribes to connect
|
||||
* (initial load) and statusChange (clear on disconnect); fetches all pages on
|
||||
* refresh. Templates have no list-changed indicator of their own — the
|
||||
* Resources screen's indicator (driven by `resourcesListChanged`) covers them,
|
||||
* and its Refresh re-fetches templates too. So on `resourceTemplatesListChanged`
|
||||
* it auto-refreshes ONLY when the server opts in via `autoRefreshOnListChanged`;
|
||||
* otherwise it does nothing and the user pulls via the Resources Refresh (#1402).
|
||||
*/
|
||||
export class ManagedResourceTemplatesState extends TypedEventTarget<ManagedResourceTemplatesStateEventMap> {
|
||||
private resourceTemplates: ResourceTemplate[] = [];
|
||||
private client: InspectorClientProtocol | null = null;
|
||||
private unsubscribe: (() => void) | null = null;
|
||||
private _metadata: Record<string, string> | undefined = undefined;
|
||||
|
||||
constructor(client: InspectorClientProtocol) {
|
||||
super();
|
||||
this.client = client;
|
||||
const onConnect = (): void => {
|
||||
void this.refresh();
|
||||
};
|
||||
const onResourceTemplatesListChanged = (): void => {
|
||||
// Auto-refresh only when the server opts in. With auto-refresh off,
|
||||
// templates stay as-is until the user pulls via the Resources Refresh —
|
||||
// templates have no indicator of their own (the Resources indicator,
|
||||
// driven by `resourcesListChanged`, covers this same notification).
|
||||
if (this.client?.getServerSettings()?.autoRefreshOnListChanged) {
|
||||
void this.refresh();
|
||||
}
|
||||
};
|
||||
const onStatusChange = (): void => {
|
||||
if (this.client?.getStatus() === "disconnected") {
|
||||
this.resourceTemplates = [];
|
||||
this.dispatchTypedEvent("resourceTemplatesChange", []);
|
||||
}
|
||||
};
|
||||
this.client.addEventListener("connect", onConnect);
|
||||
this.client.addEventListener(
|
||||
"resourceTemplatesListChanged",
|
||||
onResourceTemplatesListChanged,
|
||||
);
|
||||
this.client.addEventListener("statusChange", onStatusChange);
|
||||
this.unsubscribe = () => {
|
||||
if (this.client) {
|
||||
this.client.removeEventListener("connect", onConnect);
|
||||
this.client.removeEventListener(
|
||||
"resourceTemplatesListChanged",
|
||||
onResourceTemplatesListChanged,
|
||||
);
|
||||
this.client.removeEventListener("statusChange", onStatusChange);
|
||||
}
|
||||
this.client = null;
|
||||
};
|
||||
export class ManagedResourceTemplatesState extends ManagedListState<
|
||||
ResourceTemplate,
|
||||
ManagedResourceTemplatesStateEventMap
|
||||
> {
|
||||
constructor(
|
||||
client: InspectorClientProtocol,
|
||||
debounceMs = DEFAULT_LIST_CHANGED_DEBOUNCE_MS,
|
||||
) {
|
||||
super(client, {
|
||||
changeEvent: "resourceTemplatesChange",
|
||||
listChangedEvent: "resourceTemplatesListChanged",
|
||||
// Templates are gated on the broader `resources` capability.
|
||||
capabilityKey: "resources",
|
||||
itemLabel: "resource templates",
|
||||
supportsIndicator: false,
|
||||
debounceMs,
|
||||
fetchPage: async (c, cursor, metadata) => {
|
||||
const result = await c.listResourceTemplates(cursor, metadata);
|
||||
return {
|
||||
items: result.resourceTemplates,
|
||||
nextCursor: result.nextCursor,
|
||||
};
|
||||
},
|
||||
});
|
||||
}
|
||||
|
||||
getResourceTemplates(): ResourceTemplate[] {
|
||||
return [...this.resourceTemplates];
|
||||
}
|
||||
|
||||
setMetadata(metadata?: Record<string, string>): void {
|
||||
this._metadata = metadata;
|
||||
}
|
||||
|
||||
async refresh(
|
||||
metadata?: Record<string, string>,
|
||||
): Promise<ResourceTemplate[]> {
|
||||
const client = this.client;
|
||||
if (!client || client.getStatus() !== "connected") {
|
||||
return this.getResourceTemplates();
|
||||
}
|
||||
// Gate on the server's `resources` capability — the MCP spec doesn't define
|
||||
// a separate `resourceTemplates` capability; resources/templates/list is
|
||||
// part of the resources surface. Calling it against a server that doesn't
|
||||
// advertise resources returns -32601 "Method not found", which then
|
||||
// surfaces in the console for every connect against a resources-less
|
||||
// server. Empty list is the right semantics for "this server doesn't
|
||||
// support resources."
|
||||
if (!client.getCapabilities()?.resources) {
|
||||
this.resourceTemplates = [];
|
||||
this.dispatchTypedEvent(
|
||||
"resourceTemplatesChange",
|
||||
this.resourceTemplates,
|
||||
);
|
||||
return this.getResourceTemplates();
|
||||
}
|
||||
const effectiveMetadata = metadata ?? this._metadata;
|
||||
this.resourceTemplates = [];
|
||||
let cursor: string | undefined;
|
||||
let pageCount = 0;
|
||||
do {
|
||||
const result = await client.listResourceTemplates(
|
||||
cursor,
|
||||
effectiveMetadata,
|
||||
);
|
||||
this.resourceTemplates = cursor
|
||||
? [...this.resourceTemplates, ...result.resourceTemplates]
|
||||
: result.resourceTemplates;
|
||||
cursor = result.nextCursor;
|
||||
pageCount++;
|
||||
if (pageCount >= MAX_PAGES) {
|
||||
throw new Error(
|
||||
`Maximum pagination limit (${MAX_PAGES} pages) reached while listing resource templates`,
|
||||
);
|
||||
}
|
||||
} while (cursor);
|
||||
this.dispatchTypedEvent("resourceTemplatesChange", this.resourceTemplates);
|
||||
return this.getResourceTemplates();
|
||||
}
|
||||
|
||||
destroy(): void {
|
||||
this.unsubscribe?.();
|
||||
this.unsubscribe = null;
|
||||
this.resourceTemplates = [];
|
||||
return this.getItems();
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue