Skip to content

Thread cacheMode through managed list refresh via SDK cache-aware verbs (#1721)#1723

Merged
cliffhall merged 8 commits into
v2/mainfrom
v2/1721-cachemode-refresh
Jul 19, 2026
Merged

Thread cacheMode through managed list refresh via SDK cache-aware verbs (#1721)#1723
cliffhall merged 8 commits into
v2/mainfrom
v2/1721-cachemode-refresh

Conversation

@cliffhall

@cliffhall cliffhall commented Jul 18, 2026

Copy link
Copy Markdown
Member

Closes #1721

Split from #1628 (SDK V2 + New Spec — card 5). This delivers the deferred raw-request cacheMode control and the one-page-at-a-time fetch UI.

Two capabilities in this PR

1. Real cacheMode on refresh (the acceptance)

InspectorClient's list/read path uses the SDK's single-page client.request verbs (deliberate — it surfaces the server's nextCursor for pagination debugging). The SDK response cache is only consulted/written by its high-level wrappers, so threading cacheMode through getRequestOptions() (as the issue proposed) would have been inert. Instead this adds an aggregate path through the SDK's cache-aware verbs:

  • InspectorClient.listAllTools/Prompts/Resources/ResourceTemplates({ cacheMode, metadata })client.listTools() etc. (all-page aggregate + SEP-2243 x-mcp-header exclusion + cache consult/write).
  • ManagedListState delegates page-walking to those verbs; refresh(metadata?, cacheMode?) threads cacheMode. A user-initiated refresh (and a list_changed auto-refresh) uses cacheMode: "refresh", so the existing per-tab Refresh becomes genuinely cache-bypassing on modern servers. Legacy is byte-identical (nothing is cached).

2. Page-by-page list fetching — NOT deferred, delivered here

The optional one-page-at-a-time fetch UI (originally deferred) is included, per follow-up direction. The sidebar control is a switch labeled Paginated:

  • Paginated on → the list fetches one page at a time; a Load next page button appears and each click appends the next page.
  • Paginated off (default) → the list auto-aggregates every page on load (all items at once), exactly as before this PR.

(Under the hood the switch drives the paginatedLists server setting — true = paginated/one-page-at-a-time, false = aggregate-all.)

  • paginatedLists server setting (persisted, server-wide) — a defensive default for servers with very large lists. Plumbed through ServerConfig/InspectorServerSettings, serverList convert/default, the node + remote settings paths, and the Server Settings form ("Fetch Lists One Page at a Time").
  • ListPaginationControls element in each list sidebar (Tools/Resources/Prompts): the Paginated switch (fully left) and Load next page button (fully right) on one row, with a centered N pages loaded status below. The control hides entirely once a list is known to be a single page (nothing to paginate). The sidebar switch edits the setting live (optimistic) and persists it.
  • The paged state stores own loading: when Paginated is on they auto-load page 1 on connect and track nextCursor + page count as observable state; the managed (aggregate) stores skip their all-page walk in that mode (nothing is over-fetched) while still lighting the list-changed indicator. usePaginatedList selects the display source per mode — pure, no data-loading in React effects.
  • Refresh is mode-aware (Paginated on → reload page 1 and clear the indicator; off → refetch the whole aggregate). Sidebars widened 340→360 so the controls don't crowd list entries.

Test server

test-servers/configs/pagination-http.json — 12 tools / resources / prompts, maxPageSize 4 → three pages each — exercises the flow end to end (documented in the README).

Screenshots

Captured against that test server (also committed under pr-screenshots/).

Paginated off (default) — the full aggregated list loads at once

01-tools-all-pages-default

Paginated on — page 1 only (4 tools), with "Load next page" and a centered "1 page loaded"
02-tools-single-page-first

After "Load next page": next page appended (8 tools), "2 pages loaded"
03-tools-single-page-load-more

Server Settings: "Fetch Lists One Page at a Time" — the sidebar Paginated switch persisted it
04-server-setting-checkbox

Notes / deferred

Acceptance

  • ✅ Force a cache-bypassing refresh of tools/resources/prompts on modern servers, visible in the Network tab.
  • ✅ Page-by-page fetch: per-list Paginated switch + server setting + Load-next-page surfacing nextCursor.
  • npm run ci green (validate + coverage ≥90/file + smoke + Storybook).

🤖 Generated with Claude Code

https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5

…bs (#1721)

Manual/auto refresh of tools/resources/prompts now forces a cache-bypassing
round trip on modern servers, delivering the raw-request cacheMode control
deferred from #1628.

The InspectorClient list/read path uses the SDK's single-page `client.request`
verbs (for pagination debugging), which never consult the SDK response cache —
so `cacheMode` was inert there. This adds a separate aggregate path that routes
through the SDK's cache-aware high-level verbs, where `cacheMode` is honored:

- InspectorClient: new `listAllTools/Prompts/Resources/ResourceTemplates(
  { cacheMode, metadata })` that call `client.listTools()` etc. (all-page
  aggregate + SEP-2243 x-mcp-header exclusion + cache consult/write). A shared
  `getCacheableRequestOptions` / `aggregateListParams` back all four.
- InspectorClientProtocol: the four `listAll*` methods added.
- ManagedListState: delegates page walking to the SDK verb (config `fetchPage`
  loop → `fetchAll`); `refresh(metadata?, cacheMode?)` threads cacheMode. A
  `list_changed` auto-refresh bypasses the cache (`cacheMode: "refresh"`).
- Managed hooks: user-initiated `refresh()` passes `cacheMode: "refresh"`, so
  the existing per-tab Refresh buttons become genuinely cache-bypassing — no
  App wiring change. Connect-time load stays default (`'use'`).

The single-page `listTools(cursor)` primitive is unchanged; the paged hooks
still surface per-page `nextCursor`.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall cliffhall added the v2 Issues and PRs for v2 label Jul 18, 2026
cliffhall and others added 2 commits July 18, 2026 21:35
Delivers the deferred single-page fetch UI: Tools/Resources/Prompts can be
loaded one page at a time instead of auto-aggregating every page — a defensive
option for servers with very large lists.

- New `singlePageLists` server setting (persisted, server-wide), plumbed through
  ServerConfig/InspectorServerSettings, serverList convert/default, the node +
  remote settings paths, and the Server Settings form ("Fetch Lists One Page at
  a Time").
- New `ListPaginationControls` element (a "Single page" switch + "Load next
  page" button + "N pages loaded" status) rendered in each list sidebar; the
  toggle edits the setting live (optimistic) and persists it.
- The paged state stores (PagedTools/Prompts/ResourcesState) now own loading:
  they auto-load page 1 on connect in single-page mode and track nextCursor +
  page count as observable state; the managed stores skip their all-page walk
  in single-page mode (so nothing is over-fetched) while still lighting the
  list-changed indicator. `usePaginatedList` selects the display source and
  pagination controls per mode — pure, no data-loading in React effects.
- Refresh is mode-aware: reload page 1 in single-page mode, refetch the whole
  aggregate otherwise. Sidebars widened 340→360 so the controls don't crowd
  list entries.
- Test server: `test-servers/configs/pagination-http.json` (12 tools/resources/
  prompts, page size 4 → 3 pages each) exercises the flow end to end.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

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


Code review — PR #1723 (cacheMode + single-page pagination)

  • Read the diff and core changes
  • Review cacheMode threading (inspectorClient, managedListState)
  • Review single-page pagination (paged states, hooks, UI)
  • Review tests & coverage
  • Post review

Nice work — the architecture is clean: the aggregate/cache-aware path (listAll* → SDK high-level verbs) is cleanly separated from the single-page client.request debugging path, loading stays out of React effects, and the singlePageLists setting is threaded end-to-end (types → serverList convert/default → node + remote → form). The cacheMode: "refresh" on user/auto refresh is threaded consistently across all four managed hooks. Below are one substantive issue and a few minor observations.

🐛 Resource templates never load on connect in single-page mode

ManagedListState's connect handler skips the all-page walk for every managed list when singlePageLists is on:

// core/mcp/state/managedListState.ts:90-98
const onConnect = (): void => {
  if (this.client?.getServerSettings()?.singlePageLists) return;
  void this.refresh();
};

For tools/prompts/resources that's correct — the paged store drives the display instead. But resource templates have no paged counterpart: ManagedResourceTemplatesState extends the base and inherits this skip, yet nothing else loads templates on connect. In App.tsx, refreshResourceTemplates() is only invoked from the Refresh handlers (App.tsx:3275,3281) — never on connect — and the single-page toggle-on handler loads only loadToolsPage/loadPromptsPage/loadResourcesPage, not templates.

Result: connect to a server with singlePageLists persisted true → the Resources screen shows page 1 of resources but the resource-templates list is empty until the user manually hits Refresh. Templates aren't paginated in the UI (no Load-next-page control), so there's no reason to defer them.

Suggested fix: make the connect-time skip apply only to lists that actually have a paged counterpart (e.g. a deferInSinglePage / hasPagedCounterpart flag on ManagedListConfig, set false for resource templates), so ManagedResourceTemplatesState still aggregates on connect regardless of the setting. Fix this →

Minor observations

  1. loadPage has no in-flight guard — double-click can duplicate a page. ListPaginationControls' "Load next page" button is disabled only by canLoadMore (pagedNextCursor !== undefined), which doesn't clear until the fetch resolves and state re-renders. Two fast clicks both read the same pagedNextCursor and both append that page (core/mcp/state/pagedToolsState.ts:102-107), producing duplicate entries. Consider a loading flag in the paged store (ignore/queue a loadPage while one is in flight) or disabling the button while a load is pending.

  2. Single-page refresh / load-more bypass ambient auth recovery. In App.tsx, all-pages refresh wraps the fetch in runWithCommandAuthRecovery(...), but the single-page branches (toolsPagination.onRefresh()loadPage, and onLoadMore) call loadPage directly. On a modern server that needs mid-session re-auth, a single-page Refresh or Load-next-page will surface a raw 401 instead of triggering the recovery flow the all-pages path gets. Probably acceptable for now, but worth a note or a follow-up.

  3. Naming nit: PagedToolsState.getTools() returns "an aggregated list" per the doc comment while the whole point is one-page-at-a-time — the word "aggregated" (carried over from the ported v1.5 comment) reads oddly next to the new single-page semantics. Purely cosmetic.

Tests / coverage

Test coverage is solid and broad — paged-state tests assert page-count/cursor tracking and reset-on-disconnect, usePaginatedList is unit-tested for mode selection, and the new listAll* verbs get integration coverage. The one gap that maps to the bug above: there's no test asserting resource templates load on connect while singlePageLists is on — worth adding alongside the fix.
· branch v2/1721-cachemode-refresh

…ery (#1721)

- Fix: resource templates never loaded on connect in single-page mode. The
  connect-time all-page skip now applies only to lists with a paged counterpart
  (new `deferInSinglePage` flag on ManagedListConfig; false for resource
  templates), so templates still aggregate on connect regardless of the setting.
- Guard `PagedTools/Prompts/ResourcesState.loadPage` against a concurrent load
  (fast double-click on "Load next page") so the same page can't be appended
  twice; the in-flight load owns the cursor and the second call is a no-op.
- Thread ambient auth recovery through the single-page refresh, Load-next-page,
  and mode-switch loads (they previously bypassed it) — usePaginatedList's
  onRefresh/onLoadMore now return their promises so App can wrap them.
- Drop the misleading "aggregated list" wording from the paged-store docs.

Tests added for each; npm run coverage green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the thorough review — all four points addressed in b111649.

🐛 Resource templates never load on connect in single-page mode — fixed

Great catch. Added a deferInSinglePage flag to ManagedListConfig (true for tools/prompts/resources, false for resource templates) and gated both the connect-time skip and the list_changed auto-aggregate skip on it. So resource templates — which have no paged counterpart — now aggregate on connect regardless of singlePageLists. Added a test asserting exactly this (ManagedResourceTemplatesState "STILL loads on connect in single-page mode").

1. loadPage double-click can duplicate a page — fixed

Added an in-flight loading guard to all three paged stores: while a loadPage is pending, the next call is a no-op that returns the current cursor (so a caller can't misread it as "the end"). The page is appended exactly once. Added a concurrent-loadPage test to each paged-state test.

2. Single-page refresh / load-more bypass auth recovery — fixed

usePaginatedList.onRefresh / onLoadMore now return their underlying promise, and App wraps the single-page Refresh, Load next page, and the mode-switch loads in runWithCommandAuthRecovery(...) — so a mid-session 401 on any single-page fetch triggers the same ambient re-auth flow the all-pages path gets.

3. Naming nit — fixed

Dropped the carried-over "aggregated list" wording from the PagedTools/Prompts/ResourcesState doc comments (now "the tools/prompts/resources accumulated so far").

npm run coverage is green with the added tests. 🤖

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

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


Re-review — PR #1723 (cacheMode + single-page pagination)

  • Read the diff since the last review
  • Verify the four addressed points (template load, double-click guard, auth recovery, naming)
  • Look for new issues in the added/changed code
  • Post review

All four points from the previous review are addressed correctly and cleanly, with tests. Verified in the code:

✅ 1. Resource templates now load on connect in single-page mode

ManagedListConfig gained a deferInSinglePage flag (managedListState.ts:70), and it gates both the connect-time skip (:109-114) and the list_changed auto-aggregate skip (:185-186). It's true for tools/prompts/resources and false for resource templates (managedResourceTemplatesState.ts:44), so templates — which have no paged counterpart — still aggregate on connect regardless of singlePageLists. Backed by the "STILL loads on connect in single-page mode" test. Good, targeted fix.

✅ 2. loadPage double-click guard

All three paged stores now carry an in-flight loading flag; a concurrent loadPage is a no-op that returns the current cursor rather than { nextCursor: undefined }, so a caller can't misread the drop as "end of list" (pagedToolsState.ts:104-107, mirrored in prompts/resources). Cleared in finally. Concurrent-loadPage tests added to each store.

✅ 3. Single-page fetches now go through auth recovery

usePaginatedList.onRefresh / onLoadMore return the underlying promise, and App wraps every single-page fetch — Refresh (App.tsx:3247-3255), Load-next-page (:3364-3387), and the mode-switch loads (:3320-3337) — in runWithCommandAuthRecovery(..., "ambient"), matching the all-pages path. A mid-session 401 on any single-page fetch now triggers re-auth instead of surfacing raw.

✅ 4. Naming nit

The "aggregated list" wording is dropped from the paged-state doc comments (pagedToolsState.ts:1-2 now reads "the tools accumulated so far").

Minor observations (non-blocking)

  1. Toggle-persist failure leaves an un-reset optimistic override. In onToggleSinglePageLists (App.tsx:3303), the override is reset by an effect keyed on persistedSinglePageLists/activeServerId (:978-980). If updateServerSettings rejects, persistedSinglePageLists never changes, so singlePageListsOverride is never cleared — the UI stays in the toggled mode (and setServerSettings already pushed it live) even though persistence failed and a reconnect will revert. The user does get a red "Failed to save pagination setting" notification, so this is a cosmetic inconsistency, not a correctness bug. Worth a follow-up only if it bites.

  2. Three parallel runWithCommandAuthRecovery("ambient") calls on mode-switch / a single-page resource Refresh. The all-pages resource Refresh serializes its two fetches inside one recovery wrapper (:3288-3291), but the mode-switch fires three independent wrappers (:3320-3337). This matches other multi-list refresh spots and ambient recovery is presumably idempotent under concurrency, so likely fine — just noting the asymmetry.

Nice work — the architecture holds up on the second pass: cache-aware aggregate path cleanly separated from the single-page client.request debug path, loading stays out of React effects, and deferInSinglePage is a clean way to express "has a paged counterpart." No blocking issues from me.
· branch v2/1721-cachemode-refresh

…fails (#1721)

Non-blocking review follow-up. If updateServerSettings rejects, the optimistic
`singlePageListsOverride` is now cleared and the live client setting rolled back
(the effect only clears the override when the persisted value changes, which
doesn't happen on failure), so the UI and client reflect what's on disk rather
than the failed edit. Added an App test for the revert path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks — both non-blocking observations handled in c23209f.

1. Toggle-persist failure left the optimistic override un-reset — fixed

onToggleSinglePageLists now, on updateServerSettings rejection, clears singlePageListsOverride and rolls the live client setting back to the previous value (captured as prevSettings). So on a persist failure the UI and the client both reflect what's actually on disk, alongside the existing red error toast — no more "stuck in the toggled mode" inconsistency. Added an App test ("reverts the optimistic toggle when persisting the setting fails").

2. Three parallel runWithCommandAuthRecovery("ambient") on mode-switch — intentional, left as-is

This is deliberate: the three lists are independent and there's no ordering dependency, so firing their loads concurrently is the right latency behavior (the all-pages resource Refresh only serializes because its two fetches share the resource capability). Ambient recovery is safe under concurrency — each call independently detects a 401 and drives the same step-up, matching the other multi-list refresh spots. No change needed.

npm run coverage green. 🤖

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

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


Re-review (round 3) — PR #1723 (cacheMode + single-page pagination)

  • Read the diff since the last review
  • Verify cacheMode threading (listAll* verbs, managedListState)
  • Verify single-page pagination (paged states, usePaginatedList, UI)
  • Verify the round-3 fix (revert optimistic toggle on persist failure)
  • Look for new issues
  • Post review

The round-2/round-3 fixes all landed cleanly and the architecture continues to hold up: the cache-aware aggregate path (listAll* → SDK high-level verbs, cacheMode threaded through getCacheableRequestOptions) is cleanly separated from the single-page client.request debug path; the loadPage in-flight guard, deferInSinglePage gating, and auth-recovery wrapping are all correct; and the persist-failure rollback in onToggleSinglePageLists (revert override and setServerSettings(prevSettings)) is exactly right. One substantive bug found this pass.

🐛 Single-page Refresh never clears the list-changed indicator

In single-page mode, a list_changed deliberately only lights the indicator — the design intent is stated in the code itself:

// core/mcp/state/managedListState.ts:179-186
// … only light the indicator so the user can pull page 1 fresh via Refresh (#1721).
const skipAggregate = this.config.deferInSinglePage && settings?.singlePageLists;
if (!skipAggregate && settings?.autoRefreshOnListChanged) {
  await this.refresh(undefined, "refresh");
} else if (this.config.supportsIndicator) {
  this.setListChanged(true);   // ← tools/prompts/resources all light here in single-page mode
}

All three lists have supportsIndicator: true and deferInSinglePage: true, so skipAggregate is true and the indicator lights (verified in managedToolsState.ts:36-37, prompts, resources).

The indicator is meant to be acknowledged by Refresh. In all-pages mode that works: useManagedTools.refresh() calls managedToolsState.clearListChanged() before fetching (useManagedTools.ts:66). But the single-page Refresh path bypasses useManagedTools entirely:

// App.tsx onRefreshTools (single-page branch)
void runWithCommandAuthRecovery(() => toolsPagination.onRefresh(), "ambient");
// usePaginatedList.onRefresh → loadPage(undefined)  — reloads page 1 of the PAGED store only

loadPage(undefined) reloads the paged store's page 1 but never touches managedToolsState.clearListChanged(). So on a modern server that emits list_changed, in single-page mode:

  1. list_changed → indicator lights ✅ (intended)
  2. User clicks Refresh → page 1 reloads, but the indicator stays lit

The exact "light the indicator so the user can pull page 1 fresh via Refresh" flow the comment describes is broken — the indicator is stuck on until the user switches back to all-pages mode and refreshes, or reconnects. Affects tools, prompts, and resources.

Suggested fix: clear the managed indicator in the single-page Refresh path. E.g. expose clearListChanged from useManagedTools/useManagedPrompts/useManagedResources and call it in the single-page branch of onRefreshTools/onRefreshPrompts/onRefreshResources (before/around toolsPagination.onRefresh()), or thread a managedClearListChanged callback into usePaginatedList and have onRefresh call it in single-page mode. Add a test asserting a single-page Refresh clears the indicator. Fix this →

Minor observations (non-blocking)

  1. MAX_PAGES safety cap removed. The old fetchAll in managedListState threw after 100 pages, guarding against a server that returns a never-terminating nextCursor. Delegating page-walking to the SDK's client.listTools()/etc. drops that Inspector-side bound (managedListState.tsMAX_PAGES and the throw are gone). This is fine iff the SDK's high-level verbs bound their own aggregation loop; worth a quick confirm, since a malicious/buggy server could otherwise loop indefinitely where it previously errored out.

  2. Brief stale flash on toggle-on within a connection. Toggling single-page off then on while connected switches the display source to pagedItems immediately (optimistic override), showing the previously-accumulated paged list for a frame before loadPage(undefined) replaces it. Cosmetic; the replace lands quickly.

Nice work overall — the cacheMode: "refresh" semantics, the deferInSinglePage abstraction, and the persist-failure rollback are all clean. The indicator-clearing gap is the one thing I'd want fixed before merge (it directly breaks the documented single-page acknowledge flow).
· branch v2/1721-cachemode-refresh

…sh (#1721)

Round-3 review fix (blocking): in single-page mode a `list_changed` lights the
list-changed indicator, but the single-page Refresh reloads page 1 of the paged
store and bypassed `useManaged*.refresh()`, so the indicator was never cleared.
Expose `clearListChanged` from useManagedTools/Prompts/Resources and call it in
the single-page branch of onRefreshTools/Prompts/Resources. Tests added.

UI polish (user feedback on the sidebar control):
- Rename the switch label "Single page" → "Paginated".
- Put "Load next page" on the same row as the switch, space-between (switch
  fully left, button fully right).
- Move the "N pages loaded" status to its own line below the row, centered.
- Hide the whole control once the list is known to be a single page
  (single-page mode, page 1 loaded, no nextCursor) — nothing to paginate.

pr-screenshots refreshed to the new layout.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall

Copy link
Copy Markdown
Member Author

Thanks — the blocking bug is fixed and both non-blocking notes addressed in 7574b9f.

🐛 Single-page Refresh never clears the list-changed indicator — fixed

Exactly right, and a real break of the documented acknowledge flow. clearListChanged is now exposed from useManagedTools / useManagedPrompts / useManagedResources (acknowledges the indicator without fetching), and the single-page branch of onRefreshTools / onRefreshPrompts / onRefreshResources calls it before the paged onRefresh(). So in single-page mode: list_changed → indicator lights → Refresh reloads page 1 and clears the indicator. Tests added at both layers (useManaged* "clearListChanged() acknowledges the indicator without fetching", and an App-level assertion that single-page Refresh clears it).

1. MAX_PAGES cap removed — confirmed safe

The SDK's high-level verbs bound their own aggregation: client.listTools()/etc. walk pages up to ClientOptions.listMaxPages (default 64) and throw SdkError(ListPaginationExceeded) on a non-terminating nextCursor (verified in _listAllPages in the client dist). So delegating page-walking to the SDK didn't drop the guard — it moved it into the SDK with an equivalent bound. No Inspector-side cap needed.

2. Brief stale flash on toggle-on — acknowledged (cosmetic)

Confirmed cosmetic: toggling on shows the previously-accumulated paged list for a frame before loadPage(undefined) replaces it. The replace lands within a frame or two; not worth adding state to suppress. Left as-is.

Also folded in some sidebar-control UI polish from live feedback (label → "Paginated", switch left / "Load next page" right on one row, page-count centered below, and the control hides entirely once the list is a single page). npm run coverage green; PR screenshots refreshed. 🤖

@cliffhall

Copy link
Copy Markdown
Member Author

@claude review

@claude

claude Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

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


Re-review (round 4) — PR #1723 (cacheMode + single-page pagination)

  • Read the diff since the last review (7574b9f)
  • Verify the round-3 fix (single-page Refresh clears list-changed indicator)
  • Review the sidebar-control UI polish
  • Look for new issues in changed code
  • Post review

The round-3 blocking fix and the UI polish both landed cleanly. Verified in the code, no blocking issues this pass.

✅ Single-page Refresh now clears the list-changed indicator

clearListChanged is exposed from useManagedTools/Prompts/Resources as an acknowledge-without-fetch (useManagedTools.ts:84-86), and the single-page branch of each refresh handler calls it before the paged reload — onRefreshTools (App.tsx:3255), onRefreshPrompts (:3272), onRefreshResources (:3289). So in single-page mode the documented flow now works: list_changed lights the indicator (managedListState.ts:192-193), Refresh reloads page 1 and clears it. Backed by the App-level test (App.test.tsx:197) and the per-hook clearListChanged() acknowledges the indicator without fetching tests.

✅ UI polish

ListPaginationControls reads cleanly: switch left / "Load next page" right on one space-between row, page-count centered below, · end marker when !canLoadMore. The single-page hide (singlePage && !canLoadMore && loadedPages === 1) is guarded against the not-yet-loaded case (loadedPages === 0 still shows the switch — tested at ListPaginationControls.test.tsx:82). Good test coverage for every branch (hide, singular/plural label, end marker, load-more click).

Spot-checked the rest of the surface again and it continues to hold: the getCacheableRequestOptions(cacheMode)client.listTools() aggregate path (inspectorClient.ts:690-698, 2147-2161) is cleanly separated from the single-page client.request debug path; the loadPage in-flight loading guard returns the current cursor on a dropped concurrent call (pagedToolsState.ts:104-107); deferInSinglePage gates both the connect-time and list_changed aggregate skips; and the persist-failure rollback reverts both the override and the live client setting.

Minor observations (non-blocking)

  1. Hiding the whole control removes the only in-sidebar way to turn single-page mode off for a single-page list. When singlePage && !canLoadMore && loadedPages === 1, the entire Stack (including the "Paginated" switch) is hidden (ListPaginationControls.tsx:62). If a server has singlePageLists persisted on and every list fits in one page, all three sidebar toggles vanish and the setting is only reachable from Server Settings. Functionally harmless (one page == aggregate when there's one page), and it's the intended "nothing to paginate" behavior — just noting the discoverability tradeoff vs. the prior always-visible switch.

  2. Toggle-on fires three independent loads directly, not through the pagination hook. onToggleSinglePageLists calls loadToolsPage/loadPromptsPage/loadResourcesPage(undefined) directly (App.tsx:3334-3346) rather than the *Pagination.onRefresh() path the Refresh handlers use. Correct here (the paged stores' connect-time auto-load didn't run since the mode was off at connect, so this is the initial load, and the loading guard covers any overlap), and it matches the asymmetry noted last round — just an observation, no change needed.

Nice work — clean fix, well-tested, and the architecture has held up across all four passes. No blocking issues from me.
· branch v2/1721-cachemode-refresh

@cliffhall

Copy link
Copy Markdown
Member Author

Thanks for the four passes 🙏 — both remaining observations are intentional, so no code change:

  1. Hiding the whole control (incl. the switch) for a single-page list. This is the requested behavior — the switch is meant to disappear when there's nothing to paginate (one page == the aggregate view). The discoverability tradeoff you note is deliberate and acceptable: single-page mode stays reachable/tunable from Server Settings → "Fetch Lists One Page at a Time" for the (rare) case where a user wants to force it back off on an all-single-page server.

  2. Toggle-on fires the three loads directly. Correct as you say — the paged stores' connect-time auto-load didn't run (mode was off at connect), so this is the initial load, and the loading guard covers any overlap. Intentional; no change.

Appreciate the thorough review — no outstanding items on my end.

cliffhall and others added 2 commits July 18, 2026 23:26
`singlePageLists` read backwards from the "Paginated" switch label (it sounds
like paginated=false). Rename the server setting to `paginatedLists` with the
same value meaning — true = paginated/one-page-at-a-time, false = aggregate-all
— and follow through on the related identifiers for consistency:

- Setting field `paginatedLists` across ServerConfig / InspectorServerSettings,
  serverList convert/default, node + remote settings paths, and the form/modal
  callbacks (onPaginatedListsChange).
- `usePaginatedList` / `ListPaginationControls` prop `singlePage` → `paginated`,
  `onSinglePageChange` → `onPaginatedChange`.
- `ManagedListConfig.deferInSinglePage` → `deferWhenPaginated`.
- App state (paginatedListsOverride, onTogglePaginatedLists, …) and all
  tests/fixtures + mode-referring comments.

"single-page" wording that denotes the SDK's one-page `client.request` verb
(inspectorClient) is intentionally left as-is. Purely a rename — no behavior
change; unit + integration + cli + tui + coverage all green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5
@cliffhall
cliffhall merged commit b3b04c6 into v2/main Jul 19, 2026
3 checks passed
@cliffhall
cliffhall deleted the v2/1721-cachemode-refresh branch July 19, 2026 05:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v2 Issues and PRs for v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Primitive lists: raw-request controls — cacheMode (bypass/refresh) + single-page fetch

1 participant