chore(web): extract App.tsx's server commands into useServerCommands

[cliffhall]

Closes #2155

Phase-2 step 4 of the App.tsx decomposition (#2129 for the phase, #2126 for the whole effort). Step 3 (#2154 / PR #2169) has landed, so this targets v2/main rather than that branch.

What moved

clients/web/src/hooks/useServerCommands.tsx — every command the screens issue against the live server:

• onCallTool, onClearToolResult, onToolsUiChange
• onGetPrompt, onReadResource, onReadResourceContents, onSubscribeResource, onUnsubscribeResource, onCompleteArgument
• onCancelTask, onCancelToolCall, onClearCompletedTasks, onSetLogLevel, onSetModernLogLevel
• the refresh and load-more callbacks, onRefreshTasks, and onTogglePaginatedLists

It is one hook because every one of these routes through the command-scoped auth recovery useOAuthRecovery publishes (#2153). A command that hits a lapsed authorization has to reach the same recovery, or two of them would prompt differently for one server — which is also why this cluster cannot precede the OAuth one.

useResultPanels

toolCallState / getPromptState / readResourceState move too, but behind a small useResultPanels exported from the same file. That split is call order, not taste — the same reason useHandshakeTelemetry sits beside useConnectionLifecycle: useOAuthRecovery drops those panels on a session reset (clearResultPanels) and routes a step-up failure into whichever one issued the command (setSourceScopedError), and it runs before the commands hook that consumes its wrappers. So the state has to exist earlier than the hook that otherwise owns it. App.tsx calls useResultPanels() up top and hands the whole surface down.

Inert by construction

No behavior change. In particular #2095's per-server pagination-override behavior is moved verbatim — the override record, the lastPersistedSettings baseline, the settle/rollback pair, and the sessionRef guard that keeps a rollback from pushing one server's value into another server's live client. Anything further belongs on top of this, per the constraint carried from #2129: a behavior change buried in a 135-line relocation is unreviewable.

Numbers

• App.tsx: 2395 → 1811 lines (−681 / +82)
• useServerCommands.tsx: 872 lines, useServerCommands.test.tsx: 72 tests
• Coverage on the new file: 100% statements / lines / functions, 96.58% branches

Verification

npm run local:gate green end to end — validate (all three durable guards, test:scripts, validate:core, four client validations), the per-file ≥90 coverage gate, verify:build-gate, verify:bundle-externals, the smokes including all three web smokes, smoke:web:firefox, and the Storybook play functions.

No screenshots: this is a pure relocation with no rendered change.

🤖 Generated with Claude Code

https://claude.ai/code/session_013hzwzS8UvBsr4yZm7o7sev

[Copilot]

Pull request overview

Extracts live-server command handling from App.tsx into a dedicated, tested hook as part of the ongoing application decomposition.

Changes:

• Adds useServerCommands and useResultPanels.
• Moves command, pagination, logging, and result-panel logic out of App.tsx.
• Adds comprehensive hook tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
clients/web/src/App.tsx	Wires the extracted hooks into the composition root.
clients/web/src/hooks/useServerCommands.tsx	Implements server commands and result-panel state.
clients/web/src/hooks/useServerCommands.test.tsx	Tests command and pagination behavior.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Copilot review round 1 — responses

Mirrored at PR level because inline replies get hidden once the fix is pushed and the thread goes outdated.

1. Unjustified double cast in the paginated-list test fixture — fixed (5c7c518)

Right, and for a better reason than the style rule: the as unknown as PaginatedListModel<T> was hiding that the fixture omitted error, so a change to that interface would not have been type-checked at this call site. The helper now declares : PaginatedListModel<T> and supplies error: null (items: [] as T[] also stops being needed once the return type is annotated).

2 & 3. void inspectorClient.subscribeToResource(uri) / unsubscribeFromResource(uri) — agreed, deferred to #2174

The finding is correct on the merits. Both methods throw — on "Client is not connected", on "Server does not support resource subscriptions", and after rolling subscription state back on a transport failure — so the callee does not own its failures, and AGENTS.md only permits a bare void when it does. These are also the only two commands in the hook that skip the shared recovery, so a mid-session 401 on a subscribe does not trigger re-auth the way a 401 on a refresh does.

It is not fixed in this PR because both lines are moved verbatim. #2155 is a decomposition step whose stated contract is that it changes no behavior, and routing these through runCommandInBackground adds auth recovery and an error toast — a real behavior change, and exactly the kind that is unreviewable buried inside a 680-line relocation. Fixing it separately keeps this diff readable as a move and gives the fix its own tests and its own review.

Filed as #2174 (v2 / bug / v2.5.0, on the board), with the repro, the rule it breaks, and the proposed fix.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

[cliffhall]

Correct, and a good catch — the comment above that stand-in claimed something it did not do. Fixed in f7fe4bc.

The gap was exactly as described: onCompleteArgument and onCancelTask have no auth handling of their own — the wrapper is their recovery — so with a pass-through they would have passed every test here while bypassing it entirely.

The stand-in now mirrors useOAuthRecovery's implementation rather than approximating it: it returns operation() directly when there is no client or no active server, otherwise catches an AuthRecoveryRequiredError, calls handleCommandScopedAuthRecovery(err, { serverId, source, retryOperation: operation }), retries once when it reports satisfied, returns undefined when it does not, and rethrows anything else. runCommandInBackground is now built on that same wrapper — as the real one is — so a background command reaches the recovery on the same terms an awaited one does, and only the reporting is its own.

Three tests pin it, asserting the source rather than merely that some recovery ran (the source is what decides which panel a step-up failure lands in):

• onCompleteArgument — recovers, retries, returns the post-reauth values; source: "tool", retryOperation present
• onCompleteArgument — returns [] when the recovery is not satisfied
• onCancelTask — recovers, retries, and stays silent because the recovery owns the prompt

The old "stays silent when the cancel raised a recovery" test was keeping its own name while testing the no-active-server path by accident; it is renamed to say what it actually covers.

75 tests, still 100% statements/lines/functions and 96.58% branches on the hook. npm run local:gate green.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/hooks/useServerCommands.tsx:895

• Remove the duplicated word in this comment.

  // Wrap Load-next-page in ambient auth recovery too, so a paginated
  // paginated fetch that hits a 401 recovers like the all-pages path (#1721).

[cliffhall]

Copilot review round 3 — response

No new comments. The one suppressed comment was real and is fixed in 71cd113:

clients/web/src/hooks/useServerCommands.tsx:895 — Remove the duplicated word in this comment.
// … so a paginated / // paginated fetch that hits a 401 …

The line came across verbatim with onLoadMoreTools, so the typo predates this PR — worth fixing here anyway, since the comment is one of the things being moved. Now reads "so a paginated fetch that hits a 401 recovers like the all-pages path (#1721)".

npm run local:gate green again on the change.

---

Round summary

Round	Finding	Disposition
1	Unjustified double cast in the paginated-list test fixture	Fixed — 5c7c518
1	void subscribeToResource / void unsubscribeFromResource	Agreed; deferred to #2174 (behavior change, and this step is an inert move)
2	Harness recovery wrapper was a pass-through claiming to model the real one	Fixed — f7fe4bc, plus three tests pinning source and retry
3	Duplicated word in a moved comment	Fixed — 71cd113

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/App.tsx:781

• “Every one” is false while the extracted subscribe/unsubscribe handlers bypass command-scoped recovery. Reword this ordering comment to describe the commands that participate in recovery rather than asserting coverage the hook does not yet provide.

  // --- Action handlers that route directly to the InspectorClient. Every one
  // of them routes through the command-scoped auth recovery above, which is
  // why they sit after it (#2155). ---

clients/web/src/hooks/useServerCommands.tsx:249

• This rationale is currently inaccurate: onSubscribeResource and onUnsubscribeResource below still call the client directly and do not use the shared recovery surface (the follow-up is tracked separately). Please document those exceptions so future callers do not assume subscription 401s already recover through this hook.

 * It is one hook because every one of these routes through the same edge —
 * the command-scoped auth recovery `useOAuthRecovery` publishes (#2153). A
 * command that hits a lapsed authorization has to reach the *same* recovery,
 * or two of them would prompt differently for one server. That dependency is
 * also why this cluster cannot precede the OAuth one.