Expose live resource subscriptions via a hook (#1325)

[cliffhall]

Summary

• Add ResourceSubscriptionsState (core/mcp/state/resourceSubscriptionsState.ts): subscribes to the InspectorClient's resourceSubscriptionsChange (URI list) and resourceUpdated events, resolves each URI against the optional ManagedResourcesState so subscriptions carry the server-supplied Resource (name/title), stamps lastUpdated on notifications/resources/updated, and clears on disconnect.
• Add useResourceSubscriptions (core/react/useResourceSubscriptions.ts): subscribes to the state manager and returns the live InspectorResourceSubscription[].
• Wire App.tsx to construct/destroy the state manager alongside the other managed stores and pass the hook's output to InspectorView (replacing the subscriptions={[]} fallback added in Wire App.tsx to v2 core/ hook layer; remove demo stub from InspectorView #1244).

Closes #1325.

Test plan

• npm run validate (format, lint, build, unit + coverage gate)
• npm run test:integration (379 passed)
• npm run test:storybook (300 passed)
• New files at 100/100/100/100 coverage
• Manual: connect to the everything server, subscribe to a resource, verify it appears under "Subscriptions"; unsubscribe and verify it disappears; trigger a server-side update and verify the timestamp updates.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 0s —— View job

---

Claude PR Review

• Read changed files
• Compare against existing state stores / hooks patterns
• Check test coverage and edge cases
• Post review feedback

---

Overall: clean, well-scoped, and consistent with the existing v2 state-manager/hook layering. The design mirrors ManagedResourcesState + useManagedResources closely enough that a reader familiar with one will recognize the other. Tests are thorough — destroy idempotence, re-subscribe semantics, the resourcesChange optimization, and the statusChange teardown path are all exercised. Coverage claim looks plausible from the tests I read.

Nothing I'd call a blocker. A handful of nits and two small correctness questions below.

Notes

1. onResourceUpdated skips the dispatch on unsubscribed URIs but doesn't explain what would actually emit a stray one. core/mcp/state/resourceSubscriptionsState.ts:75

if (!this.subscribedUris.includes(uri)) return;

The comment says "the client already guards on subscribedResources before dispatching, but we re-check so out-of-order events can't resurrect a stale entry." Worth checking InspectorClient to confirm — the only dispatch site at inspectorClient.ts:825-826 is already guarded by this.subscribedResources.has(uri). The extra guard here is fine defense-in-depth, but the comment overstates the hazard; there is no known event ordering where the client dispatches resourceUpdated for a not-subscribed URI. Low-impact nit — either leave it, or soften the comment to "belt-and-braces; if a future change decouples dispatch from subscription state, this still stays correct."

2. Constructor does not prime from client.getSubscribedResources(). core/mcp/state/resourceSubscriptionsState.ts:37-50

The state starts with subscribedUris: [] unconditionally. If this store is ever constructed against an already-connected client with existing subscriptions (e.g. a future refactor that keeps the client across server-card reconnects), the subscription list will look empty until the next subscribe/unsubscribe. The current App.tsx wiring always creates the state alongside a fresh client, so this is not a live bug — but a one-liner would future-proof it:

this.subscribedUris = client.getSubscribedResources();
this.rebuild();

Not required for this PR; flagging so future work on client reuse doesn't get a silent empty-list regression.

3. statusChange handler dispatches a literal [] rather than this.getSubscriptions(). core/mcp/state/resourceSubscriptionsState.ts:85

this.dispatchTypedEvent("subscriptionsChange", []);

Functionally equivalent since this.subscriptions = [] was just set, but every other emission goes through getSubscriptions() (defensive copy). Tiny style nit — either use this.getSubscriptions() here for consistency, or note that the literal is intentional.

4. On resourcesChange with an active subscription whose resource disappeared from the server list, the subscription tile will "regress" to the synthetic { uri, name: uri } form — losing the previously-known server title. core/mcp/state/resourceSubscriptionsState.ts:131

This is consistent with the documented fallback for unknown URIs, but it may surprise users who subscribed when the resource was listed and then see the pretty name vanish after a list refresh. Two possible takes:

• Accept the regression (current behavior) — the managed list is the source of truth.
• Cache the last-seen Resource per URI inside the state (alongside lastUpdatedByUri) and prefer the cached value when the managed list no longer has the URI.

Either is defensible; the current choice is fine if it's deliberate. Worth a one-line rationale in the class doc comment if you keep it.

5. subscribedUris.includes(uri) is O(n) on every resourceUpdated. core/mcp/state/resourceSubscriptionsState.ts:75

Subscription counts are tiny in practice so this is meaningless performance-wise. Only mentioning because the onSubscriptionsChange handler already builds a Set for the unsubscribe-pruning pass (active), and it would be almost-free to keep that Set as a class field and reuse it for the includes check. Skip if you think it's not worth the bookkeeping.

6. Hook tests exercise the state prop transitioning to/from null, which is exactly the case App.tsx triggers across server switches. Good — the "resets to empty when the state prop becomes null" test at useResourceSubscriptions.test.tsx:44 is the kind of thing that's easy to skip and hard to debug later.

7. useResourceSubscriptions result shape — returns { subscriptions } while some sibling hooks return { items, refresh }. No refresh is needed here since subscriptions are event-driven; just confirming this was the intent. Consistent with useMessageLog's { messages } pattern.

Verdict

Ship it after deciding on the regression behavior in note 4 (keep or cache). The rest are nits that do not block merge.
· Branch: v2/issue-1325-resource-subscriptions-hook

[cliffhall]

Thanks for the review! Walking through each note:

1. Overstated hazard comment on onResourceUpdated — fixed in 3e94585. Softened to "belt-and-braces; stays correct if a future change ever decouples dispatch from subscription state." The client's dispatch site at inspectorClient.ts:825-826 is indeed already guarded.

2. Prime from client.getSubscribedResources() — skipping for this PR. InspectorClientProtocol doesn't currently expose a list getter (only isSubscribedToResource(uri)), so this would require a protocol change. The App.tsx wiring always creates the state alongside a fresh client today, so there's no live bug to fix. Worth a follow-up if/when we start reusing clients across server switches.

3. statusChange handler dispatches literal [] — fixed in 3e94585. Now uses this.getSubscriptions() so every emit goes through the defensive-copy path.

4. resourcesChange regression when a resource disappears — keeping current behavior, documented in the class doc (3e94585). Rationale: the managed list is the source of truth, and displaying a stale name for a server-removed resource is arguably worse than the URI fallback. If users complain in practice we can add the per-URI cache as a follow-up.

5. subscribedUris.includes(uri) is O(n) — skipping. Subscription counts are tiny and the bookkeeping to keep a parallel Set in sync isn't worth it.

6. Hook test for state-prop transitioning to null — noted, thanks.

7. Hook result shape { subscriptions } — intentional; matches useMessageLog's { messages } pattern since subscriptions are event-driven and there's nothing to refresh().

validate, test:integration, and test:storybook all still pass.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 53s —— View job

---

Claude finished @cliffhall's task — follow-up review

• Read updated files from commit 3e94585
• Verify previous review notes 1, 3, 4 were addressed correctly
• Fresh pass over the full changeset
• Post review feedback

---

Follow-up review

All three in-scope fixes from the prior round landed correctly and the rationale for skipping 2, 5, 7 is sound. I re-read the full changeset cold and don't see anything new worth flagging. Ready to merge from my end.

Verification of prior notes:

1. ✅ onResourceUpdated comment softened at core/mcp/state/resourceSubscriptionsState.ts:75-78. Reads as genuine defense-in-depth now rather than implying a known hazard. Good.
2. ⏭ Deliberately deferred — InspectorClientProtocol has no list getter today, and the current App.tsx wiring never hands this state a pre-populated client. Agreed this is a follow-up if/when client reuse becomes a thing.
3. ✅ statusChange handler now routes through this.getSubscriptions() at core/mcp/state/resourceSubscriptionsState.ts:89. Every emit path is consistent.
4. ✅ Class-doc comment at core/mcp/state/resourceSubscriptionsState.ts:12-15 now documents the regression-to-synthetic behavior as intentional. Rationale ("managed list is the source of truth") is the right call.
5. ⏭ Skipping the Set micro-opt is correct — subscription counts are tiny and the parallel-state bookkeeping would cost more than it saves.
6. ✅ Noted.
7. ⏭ { subscriptions } shape is the right match for an event-driven source with no refresh affordance. Matches useMessageLog's { messages } precedent.

Fresh pass — nothing new I'd block on. Two very minor observations that you are welcome to ignore:

• App.tsx:346-348 — the setResourceSubscriptionsState(new ResourceSubscriptionsState(client, nextResourcesState)) pattern threads the freshly-constructed nextResourcesState in to dodge the useState update lag. The inline comment explains this clearly, which is exactly the kind of "WHY" comment that earns its keep. No change.
• The subscriptions array is rebuilt on every resourcesChange, even when none of the subscribed URIs' resources actually changed shape. For real-world subscription counts this is a non-issue; mentioning only because a future perf pass on the resources list might notice it. Not for this PR.

Verdict

LGTM. Ship it.

--- · Branch: v2/issue-1325-resource-subscriptions-hook

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 2m 43s —— View job

---

Claude PR Review (round 3)

• Fetch latest branch state
• Review seven new commits beyond the subscriptions hook
• Inspect preview/template/markdown/auto-read changes
• Post review feedback

---

LGTM in aggregate. The subscriptions hook work from the prior rounds is unchanged; the new material (markdown rendering, sized-to-content preview, template completions, auto-read, compact subscribed tile) is clean and consistent with the project's Mantine-first + theme-variant conventions. Three small correctness notes and some scope/nits below.

Scope

The PR title + body still describe only the subscriptions hook (#1325), but seven commits landed on top that expand into the resource preview/template UI. Worth either updating the PR description to reflect the full scope (so reviewers/future-you can see what actually shipped), or splitting into follow-up PRs. Not a blocker; just surfacing it so the commit history stays findable.

Correctness notes

1. Possible stale-read race in onReadResource. clients/web/src/App.tsx:472-493

If the user clicks resource A, then clicks resource B before A's inspectorClient.readResource resolves, A's late resolution calls setReadResourceState({ status: "ok", uri: "A", result: A_contents, … }) — clobbering B's pending. ResourcesScreen renders <ResourcePreviewPanel resource={readResource} contents={readState.result.contents} /> with readResource derived from selectedResourceUri (= B) but contents from readState.result (= A's contents). Header says B, body is A.

Easy guard: tag each call with a request-id ref and only commit setReadResourceState if the in-flight id still matches the latest dispatch. Or, since readState.uri is already tracked, gate the setter on uri === latestUriRef.current. Follow-up is fine — I don't think this is reachable in the happy path described in the manual test plan, but rapid clicking in the sidebar (which now auto-reads per 13b03dd6) is the exact interaction that triggers it.

2. Completion "abort" is soft-abort only. clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx:143-164

requestsRef.current.get(varName)?.abort() flips the controller's signal.aborted, and runCompletion checks it before applying results — so stale responses can't overwrite fresh ones. But onCompleteArgument never receives the signal (the App.tsx:511 wrapper calls inspectorClient.getCompletions without one), so the network request still runs to completion. That's fine for correctness — just noting that the in-flight "abort" is purely a client-side discard. If getCompletions ever grows a signal parameter, plumb it through here. Not for this PR.

3. Fast-typing + pending fulfilled request ordering. Same file, same callback.

Sequence: user types "a", 300ms debounce fires, runCompletion("a") starts. User types "ab" during the in-flight request — debounce restarts, 300ms later runCompletion("ab") starts and aborts "a"'s controller. If the "a" network request resolves after the "ab" one, the "a" result is discarded (aborted) and the "ab" result is applied. Correct. If they resolve in order, the "ab" result overwrites. Also correct. The only corner is if runCompletion is called twice for the same varName synchronously (shouldn't happen under the current debounce), the second call aborts the first's controller and replaces it. Looks fine — confirming the invariant in passing.

Nits

• formatLastUpdated duplicated across ResourcePreviewPanel.tsx:50 (prefixed "Last updated: ") and ResourceSubscribedItem.tsx:37 (bare date.toLocaleString()). Different enough to keep local but worth a glance to confirm the prefix discrepancy is intentional.

• readResource fallback uses the URI for both name and uri. clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.tsx:123-127. Same pattern as ResourceSubscriptionsState's synthetic-resource fallback, so consistent — but the synthetic name-equals-URI will render in the tile whenever the user reads from a template before the resources list catches up. Fine for now; flagging so the two fallback sites stay in sync if one evolves.

• inferMimeFromUri at ResourcePreviewPanel.tsx:133 handles .md/.markdown. Worth considering .mdx too — low priority, MCP servers serving MDX is speculative.

• ResourcesScreen layout comments (lines 91-92, 217-221, 242-250) earn their keep — these explain the flex-shrink / mah=SCROLL_MAX_HEIGHT interplay that would otherwise look arbitrary. Good "WHY" comments.

• theme/Card.ts preview variant correctly placed in the theme (flat CSS properties only) rather than in App.css — follows the CLAUDE.md guidance well.

• Markdown styles in App.css (.markdown-content > :first-child, pre, table, img nested selectors) are legitimately in App.css per the rule about "nested child-element selectors for third-party HTML output." Correct placement.

• Autocomplete filter={({ options }) => options} is the right call — the server already did the filtering against the typed prefix, and the comment explains why. Mantine's default substring filter would hide valid server suggestions when the input doesn't locally substring-match.

Tests

Coverage on the new surface looks solid. ResourceTemplatePanel.test.tsx exercises the debounce + context-passing + completionsSupported gate. ResourcePreviewPanel.test.tsx covers the markdown branch, mime inference, and blob fallbacks. ResourcesScreen.test.tsx covers auto-read on click and template → preview handoff.

Missing but not critical: an explicit test for the runCompletion abort-on-new-keystroke path (ensuring the earlier response is dropped), and one for the unmount cleanup cancelling pending timers + aborting in-flight controllers. If you want belt-and-braces, add; otherwise defer.

Verdict

Ship it. The stale-read race in note 1 is the only thing I'd prioritize, and only as a follow-up — it's a corner case that's easy to hit now that sidebar click auto-reads.
· Branch: v2/issue-1325-resource-subscriptions-hook