feat: apply auto-refresh setting live + diff-aware list-changed indicator (#1444)

[cliffhall]

Summary

Closes #1444. Two follow-ups to #1402 (PR #1443), plus a consolidation refactor and burst handling.

1. Apply the Auto Refresh setting live (no reconnect)

InspectorClient gains setServerSettings() (added to InspectorClientProtocol + the fake). When the Server Settings modal closes for the currently connected server, App pushes the edited settings onto the live client (alongside the existing live-roots apply). Since the managed list state reads getServerSettings() at notification time, toggling Auto Refresh on List Changed Notifications now takes effect on the next list_changed without a reconnect.

2. List-changed indicator (auto-refresh off) — light without fetching

With auto-refresh off, a */list_changed lights the sidebar indicator with no list call — the displayed list is untouched and the user pulls the new one via Refresh. (This was initially built diff-aware — fetch + compare, light only on a real change — but a fetch-on-list_changed runs counter to "auto-refresh off = no automatic network," so it was reverted to the blind indicator. Accepted trade-off: the indicator lights even when the server re-sent an identical list.)

3. Debounce + coalesce list_changed bursts

The everything server emits a rapid burst of */list_changed; previously that produced one action per notification. The base now debounces (default 250ms, configurable): a notification restarts a timer and the action runs once the burst settles — collapsing N notifications into a single indicator light (off) or a single fetch (on). A second-line in-flight guard coalesces a post-debounce notification that lands during a slow auto-refresh. Timer cleared on disconnect / unsubscribe / destroy.

4. Consolidate the managed-list state managers

The four per-primitive managers (tools, prompts, resources, resource templates) were near-identical — extracted a generic ManagedListState<T, M> base parameterized by a config (change-event name, list_changed notification, capability key, page fetcher, supportsIndicator, debounceMs). Each manager is now a ~45-line subclass. Public contracts unchanged, so the hooks and existing flow tests pass untouched. Templates opt out of the indicator.

Testing

• State managers: lights-indicator-without-fetching (off), debounce-burst-into-a-single-light (off, no fetch), auto-refresh-when-opted-in + auto-refresh in-flight coalescing (on), and a live setServerSettings toggle honored without reconnect; plus the preserved flow tests (pagination, capability-gate, connect/disconnect).
• Integration: getServerSettings()/setServerSettings() on the real InspectorClient.
• npm run validate (lint/build/format/typecheck + unit & integration coverage gate) and npm run test:storybook green.

🤖 Generated with Claude Code

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 47s —— View job

---

Code Review — PR #1445

• Read changed files (state managers, InspectorClient, App)
• Analyze correctness, design, tests
• Post review

Solid, well-scoped follow-up. The fetch*/apply* split is a genuine improvement (the off-state build + atomic apply removes the half-built-list interleave that the old in-place this.tools = []; … accumulation allowed), the comments are accurate, and I verified the key safety claim: setServerSettings only mutates this.serverSettings, which at runtime is read in just two places — connect-time (settings payload + connectionTimeout) and getServerSettings() at notification time. roots (this.roots) and request timeout (this.requestTimeout) are independent fields, so the live push can't disturb roots-diffing or in-flight request timeouts. The "connection-time inputs still only apply on next connect" comment is correct.

A few observations, all minor / non-blocking:

1. peekForChange only ever lights the indicator, never clears it

peekForChange() calls setListChanged(true) but never (false). Consider: a list_changed mutates the list → indicator on (displayed list unchanged). A second list_changed reverts the server list back to exactly what's still displayed → next === this.tools → indicator stays lit even though the current server list now matches what the user sees. This is probably intentional ("indicator persists until acknowledged via Refresh"), but it's worth a one-line confirmation, since the new diff-aware framing ("light only when it differs") slightly implies the indicator tracks the live delta. If the persist-until-refresh semantics are intended, a comment in peekForChange saying so would prevent a future reader from "fixing" it.

2. JSON.stringify comparison is order-sensitive

JSON.stringify(next) !== JSON.stringify(this.tools) will report a change if a server returns the same items in a different order (or paginates them differently across calls). For tools/prompts/resources that are conceptually keyed by name/uri, an order-independent compare (sort by key before stringify, or compare a derived key set) would avoid that false-positive indicator — the very class of false positive this PR is trying to eliminate. Low priority since most servers are deterministic, but cheap to harden. Fix this →

3. No in-flight guard on overlapping peeks

A rapid burst of list_changed notifications fires multiple concurrent full paginated fetchTools() calls. It's not incorrect (each compares against current this.tools), but it's redundant network work. Not worth fixing unless servers are observed to burst.

4. Resource templates aren't part of the comparison

ManagedResourcesState tracks only resources, so peekForChange won't light the indicator for a templates-only change. This is pre-existing (not introduced here) — flagging only so it's a conscious omission.

5. Growing triplication

managedToolsState.ts, managedPromptsState.ts, and managedResourcesState.ts are now near-identical copies of the fetch/apply/peek/onListChanged machinery. Each addition widens the surface where the three can drift. A shared generic base (or a small helper parameterized by list/capabilityKey/changeEvent) would be worth considering, but that's a larger refactor outside this PR's scope.

Tests

Coverage looks appropriate: peek-without-replacing-display, no-indicator-when-unchanged, auto-refresh-when-opted-in, the live setServerSettings toggle honored without reconnect, and the integration round-trip on the real client. The migration of the existing flag/clear tests to await the now-async peek via the listChangedChange event is the right call.

The acknowledged trade-off (reintroducing a fetch on every list_changed in flag-only mode) is reasonable given it never replaces the displayed list under the user — good call documenting it in the PR body.
· feat/autorefresh-live-and-diff-1444

[cliffhall]

Thanks — good review. Addressed #1; reasoning on the rest below.

1. peek only lit, never cleared — fixed (6b4fb728). You're right, and rather than just a comment I made it genuinely diff-aware in both directions: peekForChange now does setListChanged(differs) instead of setListChanged(true). So if a later list_changed reverts the server back to the displayed list, the indicator clears — nothing left to pull. Added a clear-on-revert regression test. The docstring now spells out the persist-until-Refresh-or-revert semantics so a future reader won't "fix" it.

2. JSON.stringify order-sensitivity — keeping it, intentionally. I considered the key-sorted compare, but for this indicator I think order-sensitive is the more correct behavior: a reorder is a visible change — the displayed list would look different after Refresh — so it should light the indicator. The false positive this PR targets is the identical re-send (same items, same order), which the everything server does and which order-sensitive compare already suppresses. A genuinely non-deterministic server that reshuffles on every list would be the only downside, and that's rare enough I'd rather not hide real reorders to cover it. Documented the order-sensitivity in the peek docstring.

3. No in-flight guard on overlapping peeks — agreed it's redundant-but-correct; leaving it unless bursts are observed in practice.

4. Resource templates not in the comparison — confirmed pre-existing and a conscious omission; the Resources indicator tracks resources only (templates have no indicator of their own and are pulled via the Resources Refresh).

5. Triplication — agreed; the three managers are now near-identical and a generic base parameterized by list/capabilityKey/changeEvent would be the right consolidation, but that's a broader refactor than this PR. Noting it as a standing item.

validate (incl. integration + coverage) and test:storybook green.

@claude review

[claude]

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

---

Re-review — PR #1445 (after 6b4fb728)

• Read the revert-clear change (6b4fb728) and the peek docstrings
• Verify the clear-on-revert logic across all three managers
• Check the new regression test
• Post verdict

LGTM — the responses are all reasonable and the one change you made is correct.

#1 (clear-on-revert) — verified good. Collapsing the if (differs) setListChanged(true) into setListChanged(JSON.stringify(next) !== JSON.stringify(this.x)) is applied identically across managedToolsState, managedPromptsState, and managedResourcesState. The semantics are now genuinely bidirectional: a list_changed that reverts the server back to the displayed list clears the indicator. And because setListChanged short-circuits on an unchanged value (if (this.listChanged === value) return;), the new false-path doesn't introduce redundant listChangedChange dispatches when the indicator was already dark — so no spurious event churn from the repeated identical re-sends servers like everything emit. The regression test exercises the exact lit→revert→cleared transition and asserts both the event payload and getListChanged(). Docstring now spells out persist-until-Refresh-or-revert, which is what a future reader needs.

#2 (order-sensitivity) — agreed, keeping it is the right call. Your framing is the correct one: a reorder is a visible change post-Refresh, so lighting on it is a true positive, not the false positive this PR targets (identical same-order re-send, which order-sensitive compare already suppresses). Documenting it in the docstring closes the loop — the next reader won't mistake it for an oversight. The only loser is a server that non-deterministically reshuffles every list, which is rare enough not to design around.

#3 / #4 / #5 — all reasonable to defer. The in-flight peek redundancy is correct-but-wasteful only under bursts; resource-templates exclusion is pre-existing and conscious; and the triplication is real but a separate consolidation (a generic base parameterized by list/capabilityKey/changeEvent) — agreed it's outside this PR's scope and worth tracking as a standing item.

Nothing blocking. Nice, tight follow-up. ✅
· feat/autorefresh-live-and-diff-1444

[cliffhall]

Pushed b183d5e2 — consolidated the managed-list state managers, addressing the standing triplication item from the round-1 review.

Extracted a generic ManagedListState<T, M> base parameterized by a config (change-event name, list_changed notification, capability key, page fetcher, supportsIndicator). The four managers (tools/prompts/resources/templates) are now ~40-line subclasses: config + a typed getter alias. Public contracts (per-manager *Change / listChangedChange events, get*/refresh/getListChanged/clearListChanged) are unchanged, so the hooks and all existing tests pass untouched. Templates opt out of the indicator via supportsIndicator: false. Net −245 lines.

One small unavoidable cast: dispatchTypedEvent's EventMap[K] extends void ? [] : [detail] overload can't resolve for a generic keyof M key, so the base routes dispatches through a single emit(type, detail) helper that narrows the method signature once (documented inline). The concrete subclass event maps keep the call sites type-safe.

PR description updated to cover all three parts. validate (incl. integration + coverage) and test:storybook green.

@claude review

[claude]

Claude finished @cliffhall's task in 1m 23s —— View job

---

Re-review — PR #1445 (after b183d5e2, the consolidation refactor)

• Read the new ManagedListState<T, M> base
• Verify the four subclasses preserve public contracts
• Check the emit/dispatchTypedEvent cast and the supportsIndicator opt-out
• Post verdict

LGTM — the consolidation is faithful and the −245 lines are real. I diffed the new base against the pre-consolidation managers and confirmed behavior is preserved end-to-end.

Contract preservation — verified. Each subclass keeps its own typed event map (toolsChange/promptsChange/resourcesChange/resourceTemplatesChange + listChangedChange) and getter alias, so the hooks and tests bind to unchanged surface. The base's refresh/getListChanged/clearListChanged/setMetadata/destroy are line-for-line equivalent to the old per-manager versions, just generalized over config.capabilityKey/fetchPage/itemLabel. The connect / list_changed / statusChange subscription wiring and the MAX_PAGES pagination cap are identical.

supportsIndicator: false for templates — matches old behavior exactly. I checked the pre-consolidation templates manager: on list_changed it refreshed only under autoRefreshOnListChanged and otherwise did nothing (no peek, no indicator). The base reproduces this precisely: autoRefresh → refresh, else if supportsIndicator → peek — templates fall through to a no-op. And although the templates event map declares listChangedChange (to satisfy M extends ManagedListEventMap), it never fires: peekForChange is the only thing that sets it true and is gated behind supportsIndicator, while the onStatusChange setListChanged(false) short-circuits on the unchanged false value — so no spurious event churn. The "never fires" docstring is accurate.

The emit cast — sound and well-contained. The double-cast in emit(type, detail) only erases the variadic EventMap[K] extends void ? [] : [detail] overload that can't resolve over a generic keyof M; it still routes through dispatchTypedEvent, which constructs the real TypedEventGeneric with the detail intact. The unsafety is confined to one private helper, and the concrete subclass event maps keep every call site (emit("listChangedChange", value), emit(this.config.changeEvent, this.getItems())) type-checked against the right detail type. Documented inline as promised. Reasonable trade-off versus per-manager duplication.

Minor improvement, not a regression: the old applyTools dispatched the live this.tools reference in the change event; the new dispatchChange() emits this.getItems() (a defensive copy), so subscribers can no longer accidentally mutate the manager's internal array through the event detail. The disconnect → [] path emits an equivalent empty copy.

#3 (in-flight peek guard) remains the only open nit from the prior rounds, and it's correct-but-redundant-under-bursts — fine to defer as you noted.

Net: clean extraction, no contract drift, no behavioral change beyond the defensive-copy tightening. Nothing blocking. ✅
· feat/autorefresh-live-and-diff-1444

[cliffhall]

Implemented the remaining deferred nit (#3, in-flight peek guard) in 93e821c1.

peekForChange now coalesces a burst: an in-flight peeking flag means a list_changed arriving mid-fetch sets a single peekQueued re-run flag instead of launching a concurrent paginated fetch, and the running peek loops once more on completion to capture the latest server state. So a burst of N notifications collapses to sequential peeks (at most one extra fetch beyond the in-flight one), never N concurrent fetches — and the final state is still always captured. Lives entirely in the shared base, so all four managers get it.

Added a regression test: hang the first peek's fetch, fire a 3-notification burst, assert only one fetch starts while hung and exactly one coalesced re-run after release (2 total, not 3).

validate (incl. integration + coverage) and test:storybook green.

@claude review

[claude]

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

---

Re-review — PR #1445 (after 93e821c1, the peek-coalescing guard)

• Read the coalescing change in ManagedListState
• Verify the in-flight / queued semantics and final-state capture
• Check the new burst regression test
• Post verdict

LGTM — the guard is correct, minimal, and lives in exactly the right place (the shared base, so all four managers inherit it). I traced the state machine across the burst, re-entry, revert, and disconnect paths and found no stuck-flag or lost-update hazards.

Coalescing semantics — verified. The peeking / peekQueued pair does what the description claims:

• First list_changed → peeking = false, so it enters, sets peeking = true, and starts the paginated fetchAll.
• Any notification arriving mid-fetch → peeking is true, so it just flips peekQueued = true and returns — no concurrent fetch. N-1 notifications collapse into a single queued bit.
• On completion the do…while (this.peekQueued) loops exactly once more (with peekQueued re-cleared at the top of the iteration), capturing the latest server state, then finally resets peeking = false.

So a burst of N becomes at most 2 fetches (the in-flight one + one coalesced re-run), and the final state is always captured because the re-run reads fresh from the server after the burst settled. Matches the PR description precisely.

Early-return path is safe. The if (next === null) return; inside the loop exits through finally, so peeking is always reset even when fetchAll returns null (disconnected / capability gone). A peekQueued = true that was set during that fetch is left dangling, but it's harmless: it's unconditionally re-cleared at the top of the next peek's loop, and nothing reads it while peeking is false. No deadlock, no phantom re-run. (If you wanted to be pedantic you could clear peekQueued on the null path too, but it's genuinely a no-op — not worth a change.)

No event churn regression. The coalesced re-run still routes through setListChanged, which short-circuits on an unchanged value, so the repeated identical re-sends that motivated this whole line of work don't produce spurious listChangedChange dispatches.

Test — exercises the real race. Hanging the first listTools via mockImplementationOnce + a manually-captured release, firing a 3-notification burst, asserting toHaveBeenCalledTimes(1) while hung, then (2) after release, is the correct way to prove "1 in-flight + 1 coalesced, never 3 concurrent." The await Promise.resolve() to flush the microtask before the first assertion and the setTimeout(0) after release to let the re-run's promise chain settle are both right. Good that it's parked in the tools suite — since the logic is in the base, one representative test covers all four managers.

All five round-1 nits are now addressed (1 & 2 in 6b4fb728, 5 in b183d5e2, 3 here; 4 was a conscious pre-existing omission). Nothing blocking. Clean close-out. ✅
· feat/autorefresh-live-and-diff-1444

[cliffhall]

Pushed 66a4715c — you're right, the in-flight guard alone wasn't enough. When the everything server's burst is spaced just enough that each tools/list returns before the next notification, the in-flight guard never engages and you get one call per notification (exactly your screenshot).

Added a real debounce in the ManagedListState base: a */list_changed restarts a timer (default 250ms) and the refresh/peek runs once the burst settles — N notifications collapse to 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. Delay is injectable (subclasses take an optional arg; tests use 0 for fast determinism); timer cleared on disconnect/unsubscribe/destroy.

Tests: debounce-burst-into-one-fetch + in-flight-coalescing regression tests. validate + test:storybook green. PR description updated.

@claude review

[claude]

Claude finished @cliffhall's task in 1m 29s —— View job

---

Re-review — PR #1445 (after 66a4715c, the debounce)

• Read the debounce implementation in ManagedListState
• Verify timer lifecycle (set/clear on disconnect/unsubscribe/destroy)
• Check interaction with the in-flight peek-coalescing guard
• Review the new debounce + coalescing tests
• Post verdict

LGTM — the debounce is correct, well-placed in the shared base, and the two layers (debounce + in-flight coalescing) compose cleanly without overlap. Nothing blocking.

Debounce semantics — verified. onListChanged clears any pending timer and (re)arms setTimeout(runListChanged, debounceMs), so a burst of N notifications only ever leaves one live timer; it fires once the burst settles → one runListChanged. Reading getServerSettings()?.autoRefreshOnListChanged at fire time (not arm time) is the right call — it keeps the live-setServerSettings behavior intact, so a toggle that lands mid-burst is honored on the settled fetch.

Timer lifecycle — clean. The timer is cleared in all three teardown paths: onStatusChange → disconnected, unsubscribe, and destroy (which routes through unsubscribe). Each clears and nulls listChangedTimer, so there's no stale handle and no fetch fires after disconnect/destroy — the existing destroy regression test (fire toolsListChanged post-destroy, assert no refresh) still covers that, now also implicitly covering the timer path since the listener is gone.

The two layers don't overlap — and both are needed. Your framing in the commit is accurate and I confirmed it against the code:

• Debounce collapses notifications spaced closer than debounceMs — the spaced-but-rapid burst where each tools/list returns before the next notification, which the in-flight guard alone never caught (the fetches never actually overlapped).
• In-flight coalescing catches a post-debounce notification that settles its own timer while a previous peek's paginated fetchAll is still running → peekQueued re-run, never a concurrent fetch.

So a burst is one fetch; a burst-during-a-slow-fetch is at most two. Good.

Tests — exercise the real behavior. debounces a burst … into a single fetch (3 notifications → toHaveBeenCalledTimes(1)) and the retained coalesces a peek … cover the two layers distinctly. Running the suite with debounceMs = 0 is the right determinism trade-off: the timer still fires on a real macrotask (so ordering vs. the test's own setTimeout(0) is preserved — debounce arms synchronously during dispatch, fires before the test's awaited tick), which is why the assertions land deterministically. The , 0 threading through every subclass constructor keeps the rest of the suite fast without special-casing the production default.

Minor observations (all non-blocking)

1. The auto-refresh path has no in-flight guard — only the peek path does. runListChanged → void this.refresh() for auto-refresh; refresh()/fetchAll() have no peeking-style coalescing. If bursts arrive spaced wider than debounceMs and a paginated refresh() is slow, two refreshes can overlap, and since applyItems is last-write-wins, a slower-resolving older fetch could clobber a newer list. The debounce makes this much rarer than before (it was the common case pre-debounce), and it's only a transient display state corrected by the next notification/refresh — so low priority. If you ever want symmetry, the same peeking/peekQueued pattern could wrap refresh when invoked from runListChanged. Noting it as a conscious gap, not a request.

2. Single-notification latency. A lone list_changed now incurs up to debounceMs (250ms default) before the indicator lights / list refreshes. That's the intended trade-off and 250ms is imperceptible for an indicator, just worth stating explicitly.

3. Templates arm a no-op timer. With supportsIndicator: false and auto-refresh off, a resourceTemplatesListChanged still arms/fires a debounce timer whose runListChanged does nothing. Harmless (one cleared timer per settle), and folding the gate would complicate the base — fine as-is.

All five round-1 nits plus the debounce follow-up are now resolved. Clean close-out. ✅
· feat/autorefresh-live-and-diff-1444

[cliffhall]

Thanks — addressed observation #1 in 47239167, and acknowledging #2/#3 below.

#1 (auto-refresh path had no in-flight guard) — fixed. Lifted the overlap guard out of peekForChange up to runListChanged, so it now wraps both the auto-refresh and peek actions: while one is fetching, a new post-debounce notification queues a single re-run instead of a concurrent fetch. That closes the last-write-wins clobber window on the auto-refresh path you flagged. peekForChange is back to a simple fetch-and-diff. Added an auto-refresh coalescing test alongside the existing peek one (both assert "1 in-flight + 1 coalesced, never 2 concurrent").

#2 (single-notification latency, ~250ms) — intentional and imperceptible for an indicator; leaving as-is.

#3 (templates arm a no-op debounce timer) — harmless (one cleared timer per settle), and gating it would complicate the base for no real benefit; leaving as-is.

validate (incl. integration + coverage) and test:storybook green.

@claude review

[claude]

Claude finished @cliffhall's task in 1m 33s —— View job

---

Re-review — PR #1445 (after 47239167, the auto-refresh guard lift)

• Read the guard-lift change in runListChanged / peekForChange
• Verify both paths are now coalesced
• Run the state-manager tests (14 files / 225 tests green locally)
• Post verdict

LGTM — lifting the overlap guard from peekForChange up into runListChanged is the right fix, and it's faithfully done. The clobber window you flagged is genuinely closed and peekForChange is back to a clean fetch-and-diff.

Guard placement — verified. The running/runQueued pair now wraps the whole settled action (runListChangedOnce), so the do…while (this.runQueued) loop sits above the auto-refresh/peek branch. That means:

• Auto-refresh path: a post-debounce list_changed landing mid-refresh() now sets runQueued instead of launching a concurrent fetchAll → applyItems. That's the last-write-wins clobber I called out — an older slow fetch can no longer overwrite a newer list, because the two never run concurrently. Fixed.
• Peek path: same coalescing as before 93e821c1, just hoisted — semantics unchanged.
• The setting is still read inside the loop (runListChangedOnce), so a setServerSettings toggle landing mid-burst is still honored on the re-run, and a burst that toggles auto-refresh→off mid-flight will peek on the coalesced re-run rather than refresh. Correct.

Early-return safety preserved. peekForChange's if (next === null) return; now returns out of a plain method rather than out of the loop, but the loop's await this.peekForChange() resolves normally and finally { this.running = false } still runs — so a null fetch (disconnected/capability gone) can't strand running. refresh()'s own null path likewise just returns the current list. No stuck-flag hazard on either branch.

One conscious gap (pre-existing, non-blocking): the guard only covers the runListChanged path. A user-initiated refresh() (Refresh button / hook) can still overlap a notification-driven refresh(), since the public method doesn't consult running. That's unchanged by this PR and an explicit user pull racing a burst is a far rarer, self-correcting case — fine to leave. Worth a mention only so it's a known boundary of the guard.

Test — exercises the real race. The new auto-refresh coalescing test mirrors the peek one: hang the first listTools via mockImplementationOnce + captured release, fire two toolsListChanged, assert (1) while hung and (2) after release, and additionally assert the coalesced refresh applied the new list (getTools() → ["a"]) — that last assertion is the one that actually proves the auto-refresh branch ran end-to-end through applyItems, not just that the fetch fired. Good. Since the logic lives in the base, the one tools-suite test covers all four managers.

All round-1 nits plus the debounce and both observation rounds are now resolved (#1 here; #2/#3 acknowledged as intended). Clean close-out. ✅
· feat/autorefresh-live-and-diff-1444

[cliffhall]

Pushed 7927de41 — reverted item 2 from diff-aware to a blind indicator per testing feedback.

With auto-refresh off, a */list_changed now lights the indicator with no tools/list call (the diff-aware peek was issuing one per settled burst to compare, which runs counter to "auto-refresh off = no automatic network"). The user pulls the new list via Refresh. Accepted trade-off: the indicator lights even on identical re-sends, since we no longer fetch to compare.

Kept: the debounce (one indicator light per burst), the auto-refresh path + its in-flight guard, live setServerSettings, and the ManagedListState consolidation. Removed peekForChange and the diff-aware tests (peek/unchanged/revert/peek-coalesce); the off-mode tests now assert the indicator lights with no fetch. validate + test:storybook green; PR description + issue #1444 updated.

@claude review

[claude]

Claude finished @cliffhall's task in 1m 11s —— View job

---

Re-review — PR #1445 (after 7927de41, the revert to a blind indicator)

• Read the revert change (7927de41) — runListChanged / indicator path
• Verify no */list fetch fires in off mode
• Check the in-flight guard / debounce still hold for the auto-refresh path
• Verify tests + docstrings match the new blind-indicator semantics
• Post verdict

The revert itself is correct and faithful — off-mode is now setListChanged(true) with no network, the debounce and the auto-refresh in-flight guard are untouched, and the tests assert exactly the new contract (listTools not called, one debounced flip on a burst). Behaviorally this is right and matches the maintainer's "auto-refresh off = no automatic network."

One real cleanup miss, though:

🔸 Orphaned peekForChange docstring now misdescribes destroy() — core/mcp/state/managedListState.ts:262-271

The revert removed the peekForChange method body but left its doc comment, which has now slid down onto destroy():

/**
 * Fetch on `list_changed` and track whether the server's list differs from
 * what's displayed. The displayed list is left untouched — the user still
 * pulls the new one via Refresh (pull-on-demand). Many servers re-send an
 * identical list on `list_changed`; this keeps the indicator dark in that
 * case, and also clears it if a later notification reverts the server back
 * to the displayed list (nothing left to pull). The flag is order-sensitive:
 * a reorder is a visible change the user would see on Refresh, so it counts
 * (#1444).
 */
destroy(): void {

Every claim in it is now false under the blind indicator — there's no fetch-to-compare, the indicator does not stay dark on an identical re-send (the accepted trade-off this very commit introduced), it does not clear on revert, and order-sensitivity no longer applies. And it documents destroy(), which it doesn't describe at all. A future reader will read this as the current contract and be actively misled. This block should just be deleted (or replaced with a one-line destroy() doc). Fix this →

Minor (non-blocking)

• clearListChanged docstring — managedListState.ts:178-182 still says "The peek/auto-refresh on the notification leaves the indicator set…". There's no peek anymore; in off-mode it's the blind light. Worth tightening to "the blind light / auto-refresh" while you're in there.
• Templates still arm a no-op debounce timer (supportsIndicator: false, auto-refresh off → settled runListChangedOnce does nothing). Pre-existing and harmless — flagging only for continuity with the prior round.

Neither of the above changes behavior — they're stale docs from the revert. The code path is correct; tests are green and assert the right thing. Recommend just scrubbing the dangling docstring before merge. ✅ (no functional blockers)
· feat/autorefresh-live-and-diff-1444

[cliffhall]

Good catch — scrubbed both stale docs in 14e14aa9.

• Orphaned peekForChange docstring that slid onto destroy() → deleted, replaced with a one-line destroy() doc. (It was describing fetch-to-compare / stays-dark-on-identical / clear-on-revert / order-sensitivity, all of which the revert removed.)
• clearListChanged docstring → "peek/auto-refresh" → "blind light / auto-refresh".

Doc-only; validate green. Templates' no-op debounce timer left as-is (pre-existing, harmless) as before.

@claude review

[claude]

Claude finished @cliffhall's task in 1m 14s —— View job

---

Re-review — PR #1445 (after 14e14aa9, the doc scrub)

• Read the scrub change (14e14aa9)
• Verify the two flagged docstrings are fixed
• Grep the file for any remaining stale peek references
• Post verdict

The two docs I flagged last round are correctly fixed — the orphaned peekForChange block is gone (replaced with a clean one-line destroy() doc) and clearListChanged now says "blind light." Both good.

But the scrub was incomplete — the same peek / fetch-to-compare language survives in four more places, and one of them (#1) is just as actively misleading as the destroy() one was, because it still claims off-mode does a fetch-and-diff that the revert deleted.

🔸 supportsIndicator doc still claims off-mode "peeks-and-diffs" — core/mcp/state/managedListState.ts:55-61

   * Whether this list drives a list-changed indicator. When true, a
   * `list_changed` in non-auto-refresh mode peeks-and-diffs to light the
   * indicator; ...

"Peeks-and-diffs" is exactly the fetch-to-compare behavior 7927de41 removed. Off-mode now does setListChanged(true) with no network (confirmed at runListChangedOnce, managedListState.ts:159-160). This is the same false-contract problem the destroy() docstring had — a future reader will read "peeks-and-diffs" as the live behavior. Should read something like "lights the indicator blindly (no list call)."

🔸 fetchAll doc references a peek caller that no longer exists — managedListState.ts:217-219

   * Fetch all pages without mutating state or dispatching — used by both
   * refresh (apply) and peek (compare). ...

peekForChange is gone; fetchAll now has exactly one caller — refresh (line 210, verified by grep). The "and peek (compare)" half is dead.

Minor (cosmetic, same root cause)

• managedListState.ts:24 — DEFAULT_LIST_CHANGED_DEBOUNCE_MS doc: "collapses the burst into a single refresh/peek". Off-mode lights the indicator, it doesn't peek.
• managedListState.ts:98 — inline comment: "collapse a burst of notifications into one refresh/peek once it settles". Same.

None of these change behavior — the code path is correct and the tests assert the right blind-indicator contract. But since the whole point of 14e14aa9 was to scrub stale peek docs after the revert, it's worth finishing the sweep so #1/#2 don't mislead the next reader the way the destroy() block would have. A quick grep -in peek core/mcp/state/managedListState.ts surfaces all four. Fix this →

Recommend one more doc-only pass to clear these four before merge. No functional blockers. ✅
· feat/autorefresh-live-and-diff-1444

[cliffhall]

Finished the sweep in 79026a53 — you were right, four more stale peek references survived in managedListState.ts:

• supportsIndicator doc ("peeks-and-diffs" → "lights the indicator blindly, no list call")
• fetchAll doc ("used by both refresh and peek" → just refresh/applyItems)
• the DEFAULT_LIST_CHANGED_DEBOUNCE_MS doc and the onListChanged inline comment ("refresh/peek" → "indicator light when off, fetch when on")

Also scrubbed six "async peek" comments in the state tests. grep -in peek core/mcp/state and the test dirs now return nothing. Doc/comment-only; validate + state suite green (220 tests).

@claude review

[claude]

Claude finished @cliffhall's task in 1m 8s —— View job

---

Re-review — PR #1445 (after 79026a53, finishing the peek scrub)

• Read the scrub change (79026a53)
• Verify no stale peek references remain in core/mcp/state
• Spot-check the test-comment scrub
• Post verdict

LGTM — the sweep is complete and every rewritten docstring now matches the live blind-indicator behavior. grep -rin peek core/mcp/state clients/web/src/test/core/mcp/state returns nothing.

Verified each of the four managedListState.ts fixes against the code:

1. supportsIndicator doc (line 55-62) — now "lights the indicator blindly (no list call)." Matches runListChangedOnce (:159-163): off-mode + supportsIndicator → setListChanged(true) with no fetch; supportsIndicator: false (templates) falls through to a no-op. Accurate.
2. fetchAll doc (line 219-225) — "Fetch all pages, then applyItems commits them (see refresh)." refresh (:212) is now the sole caller (grep-confirmed). The dead "and peek (compare)" half is gone.
3. DEFAULT_LIST_CHANGED_DEBOUNCE_MS doc (line 21-26) — "one indicator light when auto-refresh is off, or one fetch when on." Correct for both branches.
4. onListChanged inline comment (line 99-101) — "indicator light when off, fetch when on." Matches.

The two earlier-round fixes still hold: destroy() has its clean one-line doc and clearListChanged (:180-184) reads "blind light." And runListChanged's own docstring (:136-142) already described the blind-off / guarded-on semantics correctly — nothing stale there.

Test-comment scrub — confirmed. The six // wait for the async peek to set the flag comments are now // wait for the debounced notification to set the flag, which is what the tests actually await (the debounce timer firing runListChanged), not a peek.

Doc/comment-only, no behavior change, and the assertions/code paths are untouched. This closes out the last review thread — all five round-1 nits, the debounce follow-up, both observation rounds, and now the full doc scrub are resolved. Clean to merge. ✅
· feat/autorefresh-live-and-diff-1444