Zustand store spike

[BobDickinson]

Draft PR for discussion

I ported one state manager (ManagedToolsState) and its hook (useManagedTools) to use a Zustand store. If this design works, or if we can arrive at one that does, I'll convert the rest of the state managers and hooks.

I tried a couple of approaches. I think this one lines up the best with the idea that the underlying state data comes from InspectorClient and the state managers were designed to manage the state through InspectorClient, while at the same time exposing a Zustand store for the hook to use. And because the state manager is managing the underlying state, I chose to have it export a read-only Zustand state (the state should never be set from anywhere except the state manager). This should give us the same state manager benefits and the expected synchronization benefits of Zustand (without the extra event mechanism between the state manager and the hook). It also makes the hooks much simpler.

Feedback appreciated.

[BobDickinson]

@claude review

[claude]

Claude finished @BobDickinson's task in 2m 30s —— View job

---

PR Review: Zustand Store Spike

• Gather context and understand the request
• Read changed files and understand the Zustand design
• Analyze the implementation for correctness and design quality
• Post comprehensive review feedback

---

Overview

The design direction is sound. Replacing the custom TypedEventTarget event system with a Zustand vanilla store and useSyncExternalStore is idiomatic React 18+ and eliminates boilerplate. The hook simplification is real and welcome. One notable correctness issue needs fixing before broader rollout.

---

Critical Issue: Unstable subscribe reference causes re-subscription on every render

core/mcp/state/managedToolsState.ts, lines 58–63 and core/react/useManagedTools.ts, line 21

getStore() creates a new object with new arrow function references on every call:

getStore(): ManagedToolsReadOnlyStore {
  return {
    getState: () => this.store.getState(),
    subscribe: (listener) => this.store.subscribe(listener),  // new function each call
  };
}

React's useSyncExternalStore stores the subscribe argument in a ref and compares it with === on each render. If the reference changes, React will unsubscribe and resubscribe. Since managedToolsState?.getStore()?.subscribe returns a new arrow function every render, this re-subscribes on every single render — a real bug that can cause subscription thrashing (and potentially subtle state tearing in concurrent React).

Fix: Cache the read-only store object so getStore() always returns the same reference:

private readonly readOnlyStore: ManagedToolsReadOnlyStore;

constructor(client: InspectorClient) {
  // ...other setup...
  this.readOnlyStore = {
    getState: () => this.store.getState(),
    subscribe: (listener) => this.store.subscribe(listener),
  };
}

getStore(): ManagedToolsReadOnlyStore {
  return this.readOnlyStore;
}

Fix this →

---

Design Observations

useSyncExternalStore is the right primitive. It's exactly what Zustand's useStore uses internally. The explicit usage here is fine and makes the contract visible. An alternative would be to expose the raw StoreApi and use import { useStore } from 'zustand/react' directly — no wrapper object needed — but the current approach's read-only encapsulation is a reasonable trade-off.

The ManagedToolsReadOnlyStore interface is good. Hiding setState from consumers enforces the invariant that only the state manager mutates state. This should carry over to all future converted managers.

The client parameter removal from useManagedTools is a net improvement. The old hook's signature was (client, managedToolsState) but client was only used to guard refresh(), which is already guarded inside ManagedToolsState.refresh(). Removing it simplifies all call sites. The TUI change from useManagedTools(client, selectedManagedToolsState) → useManagedTools(selectedManagedToolsState) reflects this cleanly.

EMPTY_TOOLS stable reference is good. Prevents re-renders when manager is null.

refresh() now does a single bulk setState instead of incremental updates. The old code updated this.tools per page but only fired toolsChange once. The new code accumulates locally and calls setState once after pagination. This is strictly better — no intermediate partial states are observable.

---

Minor Issues

getSnapshot function is recreated on every render (useManagedTools.ts line 22):

() => managedToolsState?.getStore()?.getState()?.tools ?? EMPTY_TOOLS,

This is a new closure each render. Unlike subscribe, React's useSyncExternalStore doesn't require getSnapshot to be stable (it calls it on every render by design), so this isn't a correctness issue. But if you fix getStore() to cache its return value, you could simplify to:

const store = managedToolsState?.getStore() ?? null;
const tools = useSyncExternalStore(
  store?.subscribe ?? NOOP_SUBSCRIBE,
  store ? () => store.getState().tools : () => EMPTY_TOOLS,
);

Test mock duplicates internal implementation details. MockManagedToolsState in useManagedTools.test.tsx mirrors the internal Zustand store setup. This is acceptable for a unit test, but if the ManagedToolsReadOnlyStore interface were narrower or the mock diverged, tests could pass while real integration fails. The existing integration tests in managedToolsState.test.ts cover this well.

waitForTools in tests doesn't handle the case where tools are non-empty at call time gracefully in sequential tests — but looking at the test structure, this is fine since waitForTools is only called when tools are expected to be empty initially.

---

On Rolling This Out to All State Managers

The pattern is solid once the getStore() caching issue is fixed. For consistency, all converted managers should:

1. Cache readOnlyStore in the constructor (not lazily in getStore())
2. Use the same ManagedToolsReadOnlyStore-style interface naming: ManagedXxxReadOnlyStore
3. Keep getXxx() returning a spread copy (for callers that want a snapshot outside React)

The other Managed*State managers (ManagedResourcesState, ManagedResourceTemplatesState, ManagedPromptsState) that still use TypedEventTarget have hooks with the same useState + useEffect subscription pattern and will benefit equally from this conversion.

---

Verdict: Good approach with one real bug. Fix getStore() caching and this pattern is ready to use for all state managers.

[BobDickinson]

Fixed both review suggestions.

[cliffhall]

@BobDickinson How do we go about testing this? I'm not deeply familiar with the state managers and their hooks yet, so It would be good to have a little more explanation of the previous architecture and how this change works.

Also, please tag issues/prs for v1.5 and v2and add them to Project V1.5 or V2, it will help us not to lose things in the shuffle. I wish we had separate repos and these issues and prs didn't have to mix with v1 stuff.