From 519ecf49eee195d99a4a73cde6615f6606fc66fa Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 20:57:32 -0400 Subject: [PATCH 1/4] chore(lint): enable no-floating-promises in all five ESLint scopes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A floated promise is invisible at review time — the call reads like an awaited one minus four characters — and the unhandled rejection it produces surfaces in a different test, in a different file, as a stack pointing at SDK internals. In #1947 two un-held `callTool` calls made `npm run ci` unpassable and took a full investigation to attribute; the fix was two lines. `@typescript-eslint/no-floating-promises` catches the class outright, so enable it at `error` everywhere. The rule is type-aware and `tseslint.configs.recommended` is not, so each scope's config gains a parser project. The four clients name their own tsconfig projects (both of them — `src` and `__tests__`/`server` live in different ones). The root scope has no tsconfig at all, so add `tsconfig.lint.json`: a lint-only project covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts`, emitting nothing and gating nothing. It sets `moduleResolution: bundler` deliberately — under NodeNext `core/`'s extensionless imports fail to resolve, every import degrades to `any`, and the rule silently stops seeing promises at all. Measured surface, and the fixes: - core (2) — `stream.close()` in two Hono `onAbort` listeners. Hono invokes subscribers with a bare `subscriber()`, so a promise returned from the listener is floated one layer up; `closeAbortedStream` owns the rejection where it can actually be owned. - tui (5) — `handleDisconnect` had no catch of its own and its only caller is a key handler that cannot await, so a failing disconnect was a live unhandled rejection; it now surfaces the message the way `handleConnect` does. The rest are `void` at callers that genuinely cannot await, over callees that already own their failures. - web (92) — 83 are Storybook `expect(...)`, which is instrumented and returns a promise, now awaited (`scrollAreaStoryAssertions` is async for the same reason). Two tests were asserting against a provider call they never awaited. `open(url)` in both web servers now warns instead of escaping a synchronous callback. - cli, launcher — already clean. Type-aware linting costs time: `clients/web`'s `eslint .` goes from ~8s to ~19s, inside `validate`. Narrowing the loaded projects is the lever if that needs reducing later; dropping the rule is not. Closes #1959 Signed-off-by: cliffhall --- .github/copilot-instructions.md | 1 + AGENTS.md | 6 ++++ README.md | 2 ++ clients/cli/eslint.config.js | 18 ++++++++++ clients/launcher/eslint.config.js | 15 ++++++++ clients/tui/__tests__/App.test.tsx | 29 +++++++++++++++ clients/tui/__tests__/tsupConfig.test.ts | 4 ++- clients/tui/eslint.config.js | 15 ++++++++ clients/tui/src/App.tsx | 18 +++++++--- clients/tui/src/components/PromptsTab.tsx | 6 ++-- clients/tui/src/components/ResourcesTab.tsx | 4 ++- clients/web/eslint.config.js | 27 ++++++++++++++ clients/web/server/server.ts | 8 ++++- clients/web/server/vite-hono-plugin.ts | 7 +++- .../CategoryBadge/CategoryBadge.stories.tsx | 4 +-- .../EmbeddableScrollArea.stories.tsx | 4 +-- .../ExpandToggle/ExpandToggle.stories.tsx | 6 ++-- .../McpErrorBadge/McpErrorBadge.stories.tsx | 8 ++--- .../MessageDirectionBadge.stories.tsx | 4 +-- .../MethodBadge/MethodBadge.stories.tsx | 4 +-- .../MonitoringToggle.stories.tsx | 4 +-- .../elements/PinToggle/PinToggle.stories.tsx | 8 +++-- .../ReplayButton/ReplayButton.stories.tsx | 4 ++- .../ResizeHandle/ResizeHandle.stories.tsx | 2 +- .../ScreenStage/ScreenStage.stories.tsx | 8 ++--- .../ConnectionInfoContent.stories.tsx | 10 +++--- .../LogStreamPanel/LogStreamPanel.stories.tsx | 2 +- .../MessageDirectionFilter.stories.tsx | 4 +-- .../MonitoringControls.stories.tsx | 4 +-- .../MonitoringScreen.stories.tsx | 4 +-- .../NetworkStreamPanel.stories.tsx | 2 +- .../OutputValidationModal.stories.tsx | 6 ++-- .../PendingClientRequestModal.stories.tsx | 14 ++++---- .../ProtocolListPanel.stories.tsx | 2 +- .../ResourceControls.stories.tsx | 18 +++++----- .../ResourceLink/ResourceLink.stories.tsx | 8 +++-- .../StructuredOutputPanel.stories.tsx | 8 ++--- .../UrlElicitationErrorModal.stories.tsx | 6 ++-- .../ResourcesScreen.stories.tsx | 26 ++++++++------ .../ServerListScreen.stories.tsx | 4 +-- .../InspectorView/InspectorView.stories.tsx | 10 +++--- .../src/test/PreviewAnnotations.stories.tsx | 18 +++++----- .../core/auth/ema/transportProvider.test.ts | 4 +-- .../src/test/core/mcp/oauthManager.test.ts | 4 +-- .../web/src/test/scrollAreaStoryAssertions.ts | 10 +++--- core/mcp/remote/node/server.ts | 20 +++++++++-- eslint.config.js | 36 +++++++++++++++++++ tsconfig.lint.json | 34 ++++++++++++++++++ 48 files changed, 359 insertions(+), 111 deletions(-) create mode 100644 tsconfig.lint.json diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 321222058f..63b3edef2f 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -11,6 +11,7 @@ The Inspector ships as one package with three clients (**Web**, **CLI**, **TUI** - **Avoid double casts (`as unknown as T`).** They erase all type safety and usually mean the real type is being worked around. Prefer a type guard, a narrower single cast, or fixing the underlying type. If genuinely unavoidable (a documented gap in a third-party type, or bridging structurally-identical shapes TS can't relate), it **must** carry an inline comment justifying why it's safe and why nothing better exists. An unjustified `as unknown as` is not acceptable in review. - Prefer inference, type guards, and precise annotations over assertions. - An `_` prefix is the intentionally-unused marker (`argsIgnorePattern` / `varsIgnorePattern` / `caughtErrorsIgnorePattern`). +- **Never float a promise.** `@typescript-eslint/no-floating-promises` is `error` in all five ESLint scopes, so every promise must be awaited, `.catch(…)`-terminated, returned, or explicitly discarded with `void`. Prefer **holding and settling** it — a floated rejection surfaces in an unrelated file as an SDK-internal stack, which is how two un-held `callTool` calls failed the whole `npm run ci` chain (#1947). Reach for `void` only when the callee already owns its failures (it ends in a `catch`) or the caller genuinely cannot await (a sync effect body, an Ink key handler), and say which in a comment. In Storybook play functions, `expect(...)` from `storybook/test` is instrumented and returns a promise — it must be awaited. ## React and UI (web client) diff --git a/AGENTS.md b/AGENTS.md index d07e84f4da..93d0eaacac 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -719,6 +719,12 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - Use type guards and type assertions to handle potential type mismatches and ensure type safety - Take advantage of TypeScript's advanced features like generics, type aliases, and conditional types to write more expressive and reusable code - Regularly review and refactor TypeScript code to ensure it remains well-structured and adheres to evolving best practices +- **NEVER leave a promise floating.** `@typescript-eslint/no-floating-promises` is enabled at `error` in **all five** ESLint scopes — `clients/{web,cli,tui,launcher}` and the root `core/` + shared gate (#1959). Every promise must be awaited, returned, terminated with `.catch(…)`, or explicitly discarded with the `void` operator. + - **The class it catches is invisible at review time.** A floated call reads like an awaited one minus four characters, and the unhandled rejection it produces surfaces somewhere else entirely — a different test, a different file, a stack pointing at SDK internals. Two un-held `client.callTool(...)` promises made `npm run ci` unpassable in #1947: `disconnect()` rejected them with `Connection closed`, the unhandled rejection failed the whole vitest run, and the chain aborted at `coverage`, silently skipping `verify:build-gate`, `smoke`, and Storybook. Attributing it took a full investigation; the fix was two lines. + - **Prefer holding and settling the promise.** `void` is an escape hatch, not a fix — it is visible at review time (strictly better than nothing) but still discards the rejection. Reach for it only when the callee already owns its failures (it ends in a `catch` that surfaces the message) or the caller genuinely cannot await — a synchronous `useEffect` body, an Ink `useInput` key handler, a Hono `stream.onAbort` listener. Say **which** of those it is in a one-line comment; an unexplained `void` is a review finding. Where the callee does *not* own its failures, give it a `catch` rather than voiding the call (see `handleDisconnect` in `clients/tui/src/App.tsx`), or terminate with `.catch(…)` at the call site (see `open(url)` in `clients/web/server/{server,vite-hono-plugin}.ts`). + - **In Storybook play functions, `expect(...)` from `storybook/test` returns a promise.** Storybook instruments it so the interactions panel can trace each assertion, so every `expect` in a play function is awaited — as is any shared helper that wraps one (`src/test/scrollAreaStoryAssertions.ts` is `async` for this reason). + - **The rule is type-aware, so each scope's ESLint config carries a parser project.** The four clients point at their own tsconfig projects (both of them, since `src` and `__tests__`/`server` live in different ones); the root config points at **`tsconfig.lint.json`**, which exists solely to give the parser a program covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts` — none of which is rooted in a tsconfig of its own. That file emits nothing and gates nothing: type *checking* for those sources stays where it was (`core/` through `clients/web`'s `tsc -b`, `test-servers/src` through `clients/cli`'s test project). It sets `moduleResolution: bundler` deliberately — `core/` uses extensionless relative imports, which NodeNext fails to resolve, degrading every import to `any` so the rule silently stops seeing promises at all. **When adding a first-party TS location to the root lint scope, add it to `tsconfig.lint.json`'s `include` too**, or the parser fails it outright with "was not found in any of the provided project(s)". Note also that a tsconfig `include` does **not** expand braces — `core/**/*.{ts,tsx}` matches nothing; list the extensions separately. + - Type-aware linting costs real time: `clients/web`'s `eslint .` went from ~8s to ~19s, and `lint` runs inside `validate`, the fast inner-loop check. That is the price of the guarantee; if it needs reducing later, narrowing the projects each scope loads is the lever, not dropping the rule. ## Web source layout: `src/lib` vs `src/utils` diff --git a/README.md b/README.md index cb57dd3593..1c81b9cd59 100644 --- a/README.md +++ b/README.md @@ -307,6 +307,8 @@ Each client self-validates from its own folder; the root scripts chain them. The Per-client scripts exist too (`validate:web`, `coverage:cli`, `smoke:tui`, …), plus root `validate:core` / `format:core` for the shared `core/` package, `format:scripts` for the root `scripts/` tooling, and `format:shared` / `lint:shared` for the root "shared" surface (`test-servers/src/**`, `vitest.shared.mts`, the root `eslint.config.js`). Run `npm run format` before committing — the root `format` fixes `core/`, the root `scripts/`, the shared surface, and every client; `validate` runs the non-fixing `format:check` and fails CI on any unformatted file. +**Linting is type-aware.** All five ESLint scopes (`clients/{web,cli,tui,launcher}` plus the root `core/` + shared gate) enable `@typescript-eslint/no-floating-promises` at `error`, so a promise that is neither awaited, returned, `.catch(…)`-terminated, nor explicitly discarded with `void` fails `lint` — and therefore `validate` ([#1959](https://github.com/modelcontextprotocol/inspector/issues/1959)). The rule needs type information, so each scope's config names a parser project; the root scope's is **`tsconfig.lint.json`**, a lint-only project covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts`, which have no tsconfig of their own. It emits nothing and changes no typecheck — but a new first-party TS location added to the root lint scope must be added to its `include`. See **TypeScript instructions** in [`AGENTS.md`](./AGENTS.md) for when `void` is acceptable. + For the full testing rules — the ≥90% per-file gate, where test files live, the unit vs. integration vs. storybook projects, and the `v8 ignore` policy — see [`AGENTS.md`](./AGENTS.md). ## Publishing diff --git a/clients/cli/eslint.config.js b/clients/cli/eslint.config.js index 9407f99754..699b202f0f 100644 --- a/clients/cli/eslint.config.js +++ b/clients/cli/eslint.config.js @@ -16,4 +16,22 @@ export default defineConfig([ globals: globals.node, }, }, + { + // Type-aware pass for `no-floating-promises` (#1959). The rule needs type + // information, which `tseslint.configs.recommended` does not provide, and + // the parser needs a project that literally contains the linted file — so + // both of this client's tsconfig projects are listed, exactly as + // `npm run typecheck` runs them (`src` is in the first, `__tests__` only + // in the second). + files: ["**/*.ts"], + languageOptions: { + parserOptions: { + project: ["./tsconfig.json", "./tsconfig.test.json"], + tsconfigRootDir: import.meta.dirname, + }, + }, + rules: { + "@typescript-eslint/no-floating-promises": "error", + }, + }, ]); diff --git a/clients/launcher/eslint.config.js b/clients/launcher/eslint.config.js index 535689a1f5..b13efaba4f 100644 --- a/clients/launcher/eslint.config.js +++ b/clients/launcher/eslint.config.js @@ -16,4 +16,19 @@ export default defineConfig([ globals: globals.node, }, }, + { + // Type-aware pass for `no-floating-promises` (#1959) — see the CLI config + // for the reasoning. Both tsconfig projects are listed because `src` and + // `__tests__` live in different ones. + files: ["**/*.{ts,mts}"], + languageOptions: { + parserOptions: { + project: ["./tsconfig.json", "./tsconfig.test.json"], + tsconfigRootDir: import.meta.dirname, + }, + }, + rules: { + "@typescript-eslint/no-floating-promises": "error", + }, + }, ]); diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index d70b9c2741..e4879b0226 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -661,6 +661,35 @@ describe("App (foundation)", () => { expect(h.disconnect).toHaveBeenCalled(); }); + it("surfaces a disconnect failure instead of floating the rejection", async () => { + // 'd' is a key handler, so it cannot await handleDisconnect — the handler + // has to own the failure itself or it escapes as an unhandled rejection + // and fails the whole run from somewhere else (#1959). + // There is nothing to assert on screen — the message only renders once the + // client reports `status: "error"`, which a rejected disconnect does not + // do. The assertion is the run itself: vitest fails on an unhandled + // rejection, so this test passing is the proof the catch owns it. + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValue(new Error("discfail")); + const { stdin } = await mount(oneStdio()); + stdin.write("d"); + await tick(); + await tick(); + expect(h.disconnect).toHaveBeenCalled(); + }); + + it("owns a non-Error disconnect rejection too", async () => { + // The catch stringifies a non-Error rejection rather than reading + // `.message` off it; a throw here would escape the same way. + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValue("plainstring"); + const { stdin } = await mount(oneStdio()); + stdin.write("d"); + await tick(); + await tick(); + expect(h.disconnect).toHaveBeenCalled(); + }); + it("switches tabs via accelerator keys", async () => { const r = await mount(stdioServer()); await press(r, ["t"]); // tools tab (server is auto-selected) diff --git a/clients/tui/__tests__/tsupConfig.test.ts b/clients/tui/__tests__/tsupConfig.test.ts index a9bfc35f3b..8c61e9e933 100644 --- a/clients/tui/__tests__/tsupConfig.test.ts +++ b/clients/tui/__tests__/tsupConfig.test.ts @@ -207,7 +207,9 @@ describe("tui tsup config", () => { // the double cast is bridging a real structural gap, not hiding a mismatch. // A hook the patch called but the stub lacks fails as undefined here rather // than passing silently. - inkFormLabelPatch.setup( + // esbuild types `setup` as `void | Promise`; await it so the + // assertions below can't run before an async setup has registered. + await inkFormLabelPatch.setup( build as unknown as Parameters[0], ); diff --git a/clients/tui/eslint.config.js b/clients/tui/eslint.config.js index b4f8a251ad..d37c6b7d3c 100644 --- a/clients/tui/eslint.config.js +++ b/clients/tui/eslint.config.js @@ -30,4 +30,19 @@ export default defineConfig([ "react-hooks/exhaustive-deps": "warn", }, }, + { + // Type-aware pass for `no-floating-promises` (#1959) — see the CLI config + // for the reasoning. Both tsconfig projects are listed because `src` and + // `__tests__` live in different ones. + files: ["**/*.{ts,tsx}"], + languageOptions: { + parserOptions: { + project: ["./tsconfig.json", "./tsconfig.test.json"], + tsconfigRootDir: import.meta.dirname, + }, + }, + rules: { + "@typescript-eslint/no-floating-promises": "error", + }, + }, ]); diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 62c9e7432f..3f0f931d08 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -887,8 +887,15 @@ function App({ // Disconnect handler const handleDisconnect = useCallback(async () => { if (!selectedServer) return; - await disconnectInspector(); - // InspectorClient will update status automatically, and data is preserved + try { + await disconnectInspector(); + // InspectorClient will update status automatically, and data is preserved + } catch (err) { + // Nothing above this catches: the only caller is the key handler, which + // cannot await. Surface the failure the same way handleConnect does + // rather than letting it escape as an unhandled rejection. + setConnectError(err instanceof Error ? err.message : String(err)); + } }, [selectedServer, disconnectInspector]); const handleClearOAuth = useCallback(async () => { @@ -1428,12 +1435,15 @@ function App({ input.toLowerCase() === "c" && (inspectorStatus === "disconnected" || inspectorStatus === "error") ) { - handleConnect(); + // Both handlers own their failures internally (each ends in a catch + // that surfaces the message), so there is nothing left for a key + // handler — which cannot await — to do with the promise. + void handleConnect(); } else if ( input.toLowerCase() === "d" && (inspectorStatus === "connected" || inspectorStatus === "connecting") ) { - handleDisconnect(); + void handleDisconnect(); } } }); diff --git a/clients/tui/src/components/PromptsTab.tsx b/clients/tui/src/components/PromptsTab.tsx index 4f90c1a73f..46db93b567 100644 --- a/clients/tui/src/components/PromptsTab.tsx +++ b/clients/tui/src/components/PromptsTab.tsx @@ -53,8 +53,10 @@ export function PromptsTab({ if (selectedPrompt.arguments && selectedPrompt.arguments.length > 0) { onFetchPrompt(selectedPrompt); } else { - // No arguments, fetch directly - (async () => { + // No arguments, fetch directly. The IIFE catches everything it can + // throw, so there is no rejection for this key handler — which + // cannot await — to own. + void (async () => { try { const invocation = await inspectorClient.getPrompt( selectedPrompt.name, diff --git a/clients/tui/src/components/ResourcesTab.tsx b/clients/tui/src/components/ResourcesTab.tsx index 86ba91acfa..4d0fa05dbb 100644 --- a/clients/tui/src/components/ResourcesTab.tsx +++ b/clients/tui/src/components/ResourcesTab.tsx @@ -186,7 +186,9 @@ export function ResourcesTab({ } }; - fetchContent(); + // An effect body is synchronous, and fetchContent already catches + // everything it can throw, so there is no rejection left to own. + void fetchContent(); }, [shouldFetchResource, inspectorClient, onAuthRecoveryRequired]); const listWidth = Math.floor(width * 0.4); diff --git a/clients/web/eslint.config.js b/clients/web/eslint.config.js index 86602ed311..21726da5a8 100644 --- a/clients/web/eslint.config.js +++ b/clients/web/eslint.config.js @@ -31,5 +31,32 @@ export default defineConfig([ "react-refresh/only-export-components": "off", }, }, + { + // Type-aware pass for `no-floating-promises` (#1959). The rule needs type + // information, which `tseslint.configs.recommended` does not provide, and + // the parser needs a project that literally contains the linted file — so + // all four leaf projects are listed (`tsconfig.json` itself is a + // solution file with `files: []`, so it contains nothing). + // + // `.d.ts` is excluded: `vitest.shims.d.ts` is pulled in through a `types` + // reference rather than an `include`, so no project owns it, and a + // declaration file cannot float a promise anyway. + files: ["**/*.{ts,tsx}"], + ignores: ["**/*.d.ts"], + languageOptions: { + parserOptions: { + project: [ + "./tsconfig.app.json", + "./tsconfig.node.json", + "./tsconfig.storybook.json", + "./tsconfig.test.json", + ], + tsconfigRootDir: import.meta.dirname, + }, + }, + rules: { + "@typescript-eslint/no-floating-promises": "error", + }, + }, ...storybook.configs["flat/recommended"], ]); diff --git a/clients/web/server/server.ts b/clients/web/server/server.ts index 9f6ff13476..9d864ce221 100644 --- a/clients/web/server/server.ts +++ b/clients/web/server/server.ts @@ -125,7 +125,13 @@ export async function startHonoServer( sandboxUrl ?? undefined, ); if (config.autoOpen) { - open(url); + // A headless or otherwise browser-less host rejects here. The server + // is already listening and the banner above printed the URL, so warn + // and carry on rather than letting the rejection escape this + // synchronous listen callback and take the process down. + open(url).catch((err: unknown) => { + console.warn("Could not open a browser automatically:", err); + }); } }, ); diff --git a/clients/web/server/vite-hono-plugin.ts b/clients/web/server/vite-hono-plugin.ts index 1331930ce4..f07b1af6e1 100644 --- a/clients/web/server/vite-hono-plugin.ts +++ b/clients/web/server/vite-hono-plugin.ts @@ -114,7 +114,12 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { ); if (config.autoOpen) { - open(url); + // Same as the prod server: a browser-less host rejects, the dev + // server is already up, and the banner above printed the URL — so + // warn rather than let the rejection escape this sync helper. + open(url).catch((err: unknown) => { + console.warn("Could not open a browser automatically:", err); + }); } }; diff --git a/clients/web/src/components/elements/CategoryBadge/CategoryBadge.stories.tsx b/clients/web/src/components/elements/CategoryBadge/CategoryBadge.stories.tsx index f9118c4923..efbeab2837 100644 --- a/clients/web/src/components/elements/CategoryBadge/CategoryBadge.stories.tsx +++ b/clients/web/src/components/elements/CategoryBadge/CategoryBadge.stories.tsx @@ -14,7 +14,7 @@ export const Transport: Story = { args: { category: "transport" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("transport")).toBeInTheDocument(); + await expect(canvas.getByText("transport")).toBeInTheDocument(); }, }; @@ -22,6 +22,6 @@ export const Auth: Story = { args: { category: "auth" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("auth")).toBeInTheDocument(); + await expect(canvas.getByText("auth")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/EmbeddableScrollArea/EmbeddableScrollArea.stories.tsx b/clients/web/src/components/elements/EmbeddableScrollArea/EmbeddableScrollArea.stories.tsx index e85f9b6524..87da3cdb30 100644 --- a/clients/web/src/components/elements/EmbeddableScrollArea/EmbeddableScrollArea.stories.tsx +++ b/clients/web/src/components/elements/EmbeddableScrollArea/EmbeddableScrollArea.stories.tsx @@ -24,7 +24,7 @@ export const FullSize: Story = { ), play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("Row 1")).toBeInTheDocument(); + await expect(canvas.getByText("Row 1")).toBeInTheDocument(); }, }; @@ -38,6 +38,6 @@ export const Embedded: Story = { ), play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("Row 1")).toBeInTheDocument(); + await expect(canvas.getByText("Row 1")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/ExpandToggle/ExpandToggle.stories.tsx b/clients/web/src/components/elements/ExpandToggle/ExpandToggle.stories.tsx index df4054c8ce..90369ab1b4 100644 --- a/clients/web/src/components/elements/ExpandToggle/ExpandToggle.stories.tsx +++ b/clients/web/src/components/elements/ExpandToggle/ExpandToggle.stories.tsx @@ -15,7 +15,9 @@ export const Collapsed: Story = { args: { expanded: false }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByRole("button", { name: "Expand" })).toBeInTheDocument(); + await expect( + canvas.getByRole("button", { name: "Expand" }), + ).toBeInTheDocument(); }, }; @@ -23,7 +25,7 @@ export const Expanded: Story = { args: { expanded: true }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("button", { name: "Collapse" }), ).toBeInTheDocument(); }, diff --git a/clients/web/src/components/elements/McpErrorBadge/McpErrorBadge.stories.tsx b/clients/web/src/components/elements/McpErrorBadge/McpErrorBadge.stories.tsx index 4616b4fa86..c189d65420 100644 --- a/clients/web/src/components/elements/McpErrorBadge/McpErrorBadge.stories.tsx +++ b/clients/web/src/components/elements/McpErrorBadge/McpErrorBadge.stories.tsx @@ -14,7 +14,7 @@ export const HeaderMismatch: Story = { args: { code: -32020, name: "HeaderMismatch" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("-32020 HeaderMismatch")).toBeInTheDocument(); + await expect(canvas.getByText("-32020 HeaderMismatch")).toBeInTheDocument(); }, }; @@ -22,7 +22,7 @@ export const MissingCapability: Story = { args: { code: -32021, name: "MissingRequiredClientCapability" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByText("-32021 MissingRequiredClientCapability"), ).toBeInTheDocument(); }, @@ -32,7 +32,7 @@ export const UnsupportedVersion: Story = { args: { code: -32022, name: "UnsupportedProtocolVersion" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByText("-32022 UnsupportedProtocolVersion"), ).toBeInTheDocument(); }, @@ -47,6 +47,6 @@ export const MethodNotFound: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("-32601 MethodNotFound")).toBeInTheDocument(); + await expect(canvas.getByText("-32601 MethodNotFound")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/MessageDirectionBadge/MessageDirectionBadge.stories.tsx b/clients/web/src/components/elements/MessageDirectionBadge/MessageDirectionBadge.stories.tsx index a4aa55e0de..c99b8a122f 100644 --- a/clients/web/src/components/elements/MessageDirectionBadge/MessageDirectionBadge.stories.tsx +++ b/clients/web/src/components/elements/MessageDirectionBadge/MessageDirectionBadge.stories.tsx @@ -14,7 +14,7 @@ export const Outgoing: Story = { args: { direction: "outgoing" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("client → server")).toBeInTheDocument(); + await expect(canvas.getByText("client → server")).toBeInTheDocument(); }, }; @@ -22,6 +22,6 @@ export const Incoming: Story = { args: { direction: "incoming" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("server → client")).toBeInTheDocument(); + await expect(canvas.getByText("server → client")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/MethodBadge/MethodBadge.stories.tsx b/clients/web/src/components/elements/MethodBadge/MethodBadge.stories.tsx index 91e3abb515..312d249ab7 100644 --- a/clients/web/src/components/elements/MethodBadge/MethodBadge.stories.tsx +++ b/clients/web/src/components/elements/MethodBadge/MethodBadge.stories.tsx @@ -14,7 +14,7 @@ export const ToolsList: Story = { args: { method: "tools/list" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("tools/list")).toBeInTheDocument(); + await expect(canvas.getByText("tools/list")).toBeInTheDocument(); }, }; @@ -22,6 +22,6 @@ export const TasksList: Story = { args: { method: "tasks/list" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("tasks/list")).toBeInTheDocument(); + await expect(canvas.getByText("tasks/list")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/MonitoringToggle/MonitoringToggle.stories.tsx b/clients/web/src/components/elements/MonitoringToggle/MonitoringToggle.stories.tsx index 9541812e42..109a47d8d7 100644 --- a/clients/web/src/components/elements/MonitoringToggle/MonitoringToggle.stories.tsx +++ b/clients/web/src/components/elements/MonitoringToggle/MonitoringToggle.stories.tsx @@ -15,7 +15,7 @@ export const Closed: Story = { args: { open: false }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("button", { name: "Open monitoring sidebar" }), ).toBeInTheDocument(); }, @@ -25,7 +25,7 @@ export const Open: Story = { args: { open: true }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("button", { name: "Close monitoring sidebar" }), ).toBeInTheDocument(); }, diff --git a/clients/web/src/components/elements/PinToggle/PinToggle.stories.tsx b/clients/web/src/components/elements/PinToggle/PinToggle.stories.tsx index d259eb84f6..a15900d4c9 100644 --- a/clients/web/src/components/elements/PinToggle/PinToggle.stories.tsx +++ b/clients/web/src/components/elements/PinToggle/PinToggle.stories.tsx @@ -15,7 +15,9 @@ export const Unpinned: Story = { args: { pinned: false }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByRole("button", { name: "Pin" })).toBeInTheDocument(); + await expect( + canvas.getByRole("button", { name: "Pin" }), + ).toBeInTheDocument(); }, }; @@ -23,6 +25,8 @@ export const Pinned: Story = { args: { pinned: true }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByRole("button", { name: "Unpin" })).toBeInTheDocument(); + await expect( + canvas.getByRole("button", { name: "Unpin" }), + ).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/ReplayButton/ReplayButton.stories.tsx b/clients/web/src/components/elements/ReplayButton/ReplayButton.stories.tsx index 6391f2b428..a7bd9b1713 100644 --- a/clients/web/src/components/elements/ReplayButton/ReplayButton.stories.tsx +++ b/clients/web/src/components/elements/ReplayButton/ReplayButton.stories.tsx @@ -14,6 +14,8 @@ type Story = StoryObj; export const Default: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByRole("button", { name: "Replay" })).toBeInTheDocument(); + await expect( + canvas.getByRole("button", { name: "Replay" }), + ).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/elements/ResizeHandle/ResizeHandle.stories.tsx b/clients/web/src/components/elements/ResizeHandle/ResizeHandle.stories.tsx index 4fb1c122bd..7b8072dc8b 100644 --- a/clients/web/src/components/elements/ResizeHandle/ResizeHandle.stories.tsx +++ b/clients/web/src/components/elements/ResizeHandle/ResizeHandle.stories.tsx @@ -44,7 +44,7 @@ export const Default: Story = { render: () => , play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("separator", { name: "Resize demo panel" }), ).toBeInTheDocument(); }, diff --git a/clients/web/src/components/elements/ScreenStage/ScreenStage.stories.tsx b/clients/web/src/components/elements/ScreenStage/ScreenStage.stories.tsx index ab2153035a..c7af29bc89 100644 --- a/clients/web/src/components/elements/ScreenStage/ScreenStage.stories.tsx +++ b/clients/web/src/components/elements/ScreenStage/ScreenStage.stories.tsx @@ -48,7 +48,7 @@ export const FillsHostWidth: Story = { // sharper discriminator: a row flex leaves the layer full-width while // collapsing the child to the single "x"). const layer = child.parentElement as HTMLElement; - expect(child.getBoundingClientRect().width).toBeCloseTo( + await expect(child.getBoundingClientRect().width).toBeCloseTo( layer.getBoundingClientRect().width, 0, ); @@ -77,11 +77,11 @@ export const FillVariant: Story = { // Width still stretches onto the child (relative to the layer, as above); // the `fill` height comes from `bottom: 0` making the layer span the host. const layer = child.parentElement as HTMLElement; - expect(child.getBoundingClientRect().width).toBeCloseTo( + await expect(child.getBoundingClientRect().width).toBeCloseTo( layer.getBoundingClientRect().width, 0, ); - expect(layer.getBoundingClientRect().height).toBeGreaterThan(160 - 4); + await expect(layer.getBoundingClientRect().height).toBeGreaterThan(160 - 4); }, }; @@ -97,6 +97,6 @@ export const Inactive: Story = { ), play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.queryByTestId("stage-child")).not.toBeInTheDocument(); + await expect(canvas.queryByTestId("stage-child")).not.toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx index c6f1e59816..0b1d79fe19 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.stories.tsx @@ -106,8 +106,10 @@ export const ServerInfoNotReported: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.queryByText("my-catalog-name")).not.toBeInTheDocument(); - expect(canvas.getAllByText(SERVER_INFO_NOT_REPORTED_LABEL)).toHaveLength(2); + await expect(canvas.queryByText("my-catalog-name")).not.toBeInTheDocument(); + await expect( + canvas.getAllByText(SERVER_INFO_NOT_REPORTED_LABEL), + ).toHaveLength(2); }, }; @@ -187,11 +189,11 @@ export const WithOAuthIdToken: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("ID Token")).toBeInTheDocument(); + await expect(canvas.getByText("ID Token")).toBeInTheDocument(); // Only the ID token is a JWT here, so exactly one decode toggle is offered. await userEvent.click( canvas.getByRole("button", { name: "Decode JWT for ID Token" }), ); - expect(canvas.getByText(/"sub": "user-42"/)).toBeInTheDocument(); + await expect(canvas.getByText(/"sub": "user-42"/)).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/groups/LogStreamPanel/LogStreamPanel.stories.tsx b/clients/web/src/components/groups/LogStreamPanel/LogStreamPanel.stories.tsx index 111f54e6ff..a7545dc375 100644 --- a/clients/web/src/components/groups/LogStreamPanel/LogStreamPanel.stories.tsx +++ b/clients/web/src/components/groups/LogStreamPanel/LogStreamPanel.stories.tsx @@ -77,6 +77,6 @@ export const WithEntries: Story = { }, // List scrollbar reserves a gutter and stays hidden when idle (#1474). play: async ({ canvasElement }) => { - expectScrollbarGutterIdleHidden(canvasElement); + await expectScrollbarGutterIdleHidden(canvasElement); }, }; diff --git a/clients/web/src/components/groups/MessageDirectionFilter/MessageDirectionFilter.stories.tsx b/clients/web/src/components/groups/MessageDirectionFilter/MessageDirectionFilter.stories.tsx index 362b30bd1f..8296dd6344 100644 --- a/clients/web/src/components/groups/MessageDirectionFilter/MessageDirectionFilter.stories.tsx +++ b/clients/web/src/components/groups/MessageDirectionFilter/MessageDirectionFilter.stories.tsx @@ -31,10 +31,10 @@ export const AllVisible: Story = { args: { visibleDirections: { client: true, server: true } }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("button", { name: "client → server" }), ).toBeInTheDocument(); - expect( + await expect( canvas.getByRole("button", { name: "Deselect All" }), ).toBeInTheDocument(); }, diff --git a/clients/web/src/components/groups/MonitoringControls/MonitoringControls.stories.tsx b/clients/web/src/components/groups/MonitoringControls/MonitoringControls.stories.tsx index 3a3eb7ffdd..eca1065ade 100644 --- a/clients/web/src/components/groups/MonitoringControls/MonitoringControls.stories.tsx +++ b/clients/web/src/components/groups/MonitoringControls/MonitoringControls.stories.tsx @@ -20,7 +20,7 @@ type Story = StoryObj; export const AllTabs: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByRole("radio", { name: "Logs" })).toBeChecked(); + await expect(canvas.getByRole("radio", { name: "Logs" })).toBeChecked(); }, }; @@ -28,6 +28,6 @@ export const StdioServer: Story = { args: { tabs: ["Logs", "Protocol"], value: "Protocol" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.queryByRole("radio", { name: "Network" })).toBeNull(); + await expect(canvas.queryByRole("radio", { name: "Network" })).toBeNull(); }, }; diff --git a/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.stories.tsx b/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.stories.tsx index 56c1cca21f..b91d5d3b9e 100644 --- a/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.stories.tsx +++ b/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.stories.tsx @@ -47,7 +47,7 @@ type Story = StoryObj; export const Logs: Story = { play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("Log stream")).toBeInTheDocument(); + await expect(canvas.getByText("Log stream")).toBeInTheDocument(); }, }; @@ -55,6 +55,6 @@ export const Protocol: Story = { args: { value: "Protocol" }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("Request history")).toBeInTheDocument(); + await expect(canvas.getByText("Request history")).toBeInTheDocument(); }, }; diff --git a/clients/web/src/components/groups/NetworkStreamPanel/NetworkStreamPanel.stories.tsx b/clients/web/src/components/groups/NetworkStreamPanel/NetworkStreamPanel.stories.tsx index a4248d1e28..e006e26132 100644 --- a/clients/web/src/components/groups/NetworkStreamPanel/NetworkStreamPanel.stories.tsx +++ b/clients/web/src/components/groups/NetworkStreamPanel/NetworkStreamPanel.stories.tsx @@ -50,7 +50,7 @@ export const WithEntries: Story = { args: { entries: sample }, // List scrollbar reserves a gutter and stays hidden when idle (#1474). play: async ({ canvasElement }) => { - expectScrollbarGutterIdleHidden(canvasElement); + await expectScrollbarGutterIdleHidden(canvasElement); }, }; diff --git a/clients/web/src/components/groups/OutputValidationModal/OutputValidationModal.stories.tsx b/clients/web/src/components/groups/OutputValidationModal/OutputValidationModal.stories.tsx index 0942c7dd43..4054435216 100644 --- a/clients/web/src/components/groups/OutputValidationModal/OutputValidationModal.stories.tsx +++ b/clients/web/src/components/groups/OutputValidationModal/OutputValidationModal.stories.tsx @@ -45,14 +45,14 @@ export const Default: Story = { // Mantine renders the modal in a portal at document.body. const body = within(canvasElement.ownerDocument.body); await body.findByText("Output schema validation"); - expect(body.getByText(/open_pattern_editor/)).toBeInTheDocument(); + await expect(body.getByText(/open_pattern_editor/)).toBeInTheDocument(); const details = body.getByLabelText( "Validation details", ) as HTMLTextAreaElement; - expect(details.value).toContain( + await expect(details.value).toContain( "data/samples/0 must NOT have additional properties", ); - expect(details.readOnly).toBe(true); + await expect(details.readOnly).toBe(true); }, }; diff --git a/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.stories.tsx b/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.stories.tsx index 5130e98c1a..ee81a68708 100644 --- a/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.stories.tsx +++ b/clients/web/src/components/groups/PendingClientRequestModal/PendingClientRequestModal.stories.tsx @@ -122,11 +122,11 @@ export const Sampling: Story = { play: async ({ canvasElement, args }) => { const body = within(canvasElement.ownerDocument.body); await body.findByText("Sampling Request"); - expect( + await expect( body.getByText("The server is requesting an LLM completion."), ).toBeInTheDocument(); await userEvent.click(body.getByRole("button", { name: "Send Response" })); - expect(args.onSamplingRespond).toHaveBeenCalled(); + await expect(args.onSamplingRespond).toHaveBeenCalled(); }, }; @@ -144,8 +144,8 @@ export const SamplingInputRequired: Story = { play: async ({ canvasElement }) => { const body = within(canvasElement.ownerDocument.body); await body.findByText("Sampling Request"); - expect(body.getByText("input_required")).toBeInTheDocument(); - expect(body.getByText(/sent back as a retry/i)).toBeInTheDocument(); + await expect(body.getByText("input_required")).toBeInTheDocument(); + await expect(body.getByText(/sent back as a retry/i)).toBeInTheDocument(); }, }; @@ -172,11 +172,11 @@ export const ElicitationForm: Story = { play: async ({ canvasElement, args }) => { const body = within(canvasElement.ownerDocument.body); await body.findByText("Elicitation Request"); - expect( + await expect( body.getByText(/Please provide your database connection details/), ).toBeInTheDocument(); await userEvent.click(body.getByRole("button", { name: "Submit" })); - expect(args.onElicitationRespond).toHaveBeenCalledWith({ + await expect(args.onElicitationRespond).toHaveBeenCalledWith({ action: "accept", content: {}, }); @@ -207,7 +207,7 @@ export const ElicitationUrl: Story = { play: async ({ canvasElement }) => { const body = within(canvasElement.ownerDocument.body); await body.findByText("Elicitation Request"); - expect( + await expect( body.getByText("https://example.com/authorize?token=abc123"), ).toBeInTheDocument(); }, diff --git a/clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.stories.tsx b/clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.stories.tsx index 2e1dfe8df1..861f23c249 100644 --- a/clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.stories.tsx +++ b/clients/web/src/components/groups/ProtocolListPanel/ProtocolListPanel.stories.tsx @@ -107,7 +107,7 @@ export const WithEntries: Story = { // bar never overlays the cards, and uses type="scroll" so it stays hidden // when idle rather than appearing on hover (#1474). play: async ({ canvasElement }) => { - expectScrollbarGutterIdleHidden(canvasElement); + await expectScrollbarGutterIdleHidden(canvasElement); }, }; diff --git a/clients/web/src/components/groups/ResourceControls/ResourceControls.stories.tsx b/clients/web/src/components/groups/ResourceControls/ResourceControls.stories.tsx index 958ec6aa29..e7cd4f903b 100644 --- a/clients/web/src/components/groups/ResourceControls/ResourceControls.stories.tsx +++ b/clients/web/src/components/groups/ResourceControls/ResourceControls.stories.tsx @@ -86,8 +86,8 @@ export const Default: Story = { // chevron points down. play: async ({ canvasElement }) => { const transforms = chevronTransforms(canvasElement); - expect(transforms).toHaveLength(3); - for (const t of transforms) expect(t).toBe(ROTATED_DOWN); + await expect(transforms).toHaveLength(3); + for (const t of transforms) await expect(t).toBe(ROTATED_DOWN); }, }; @@ -122,9 +122,9 @@ export const Collapsed: Story = { }, play: async ({ canvasElement }) => { const transforms = chevronTransforms(canvasElement); - expect(transforms).toHaveLength(3); + await expect(transforms).toHaveLength(3); // Identity matrix = the right-pointing arrow's natural 0° (not rotated). - for (const t of transforms) expect(t).toBe(NOT_ROTATED); + for (const t of transforms) await expect(t).toBe(NOT_ROTATED); }, }; @@ -140,10 +140,10 @@ export const EmptySectionCollapsed: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText("Subscriptions (0)")).toBeInTheDocument(); + await expect(canvas.getByText("Subscriptions (0)")).toBeInTheDocument(); const transforms = chevronTransforms(canvasElement); const downCount = transforms.filter((t) => t === ROTATED_DOWN).length; - expect(downCount).toBe(2); // URIs + Templates down, Subscriptions right + await expect(downCount).toBe(2); // URIs + Templates down, Subscriptions right }, }; @@ -158,11 +158,11 @@ export const SubscriptionsUnsupported: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.queryByText(/Subscriptions/)).not.toBeInTheDocument(); + await expect(canvas.queryByText(/Subscriptions/)).not.toBeInTheDocument(); // Only the two remaining sections render, both open → both chevrons down. const transforms = chevronTransforms(canvasElement); - expect(transforms).toHaveLength(2); - for (const t of transforms) expect(t).toBe(ROTATED_DOWN); + await expect(transforms).toHaveLength(2); + for (const t of transforms) await expect(t).toBe(ROTATED_DOWN); }, }; diff --git a/clients/web/src/components/groups/ResourceLink/ResourceLink.stories.tsx b/clients/web/src/components/groups/ResourceLink/ResourceLink.stories.tsx index 655a5e7aed..ac6b6ca2e5 100644 --- a/clients/web/src/components/groups/ResourceLink/ResourceLink.stories.tsx +++ b/clients/web/src/components/groups/ResourceLink/ResourceLink.stories.tsx @@ -45,10 +45,12 @@ export const Static: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.getByText(URI)).toBeInTheDocument(); + await expect(canvas.getByText(URI)).toBeInTheDocument(); // Copy button is present; there's no expand control in the static card. - expect(canvas.getByRole("button", { name: "Copy" })).toBeInTheDocument(); - expect( + await expect( + canvas.getByRole("button", { name: "Copy" }), + ).toBeInTheDocument(); + await expect( canvas.queryByRole("button", { name: /^Expand resource/ }), ).not.toBeInTheDocument(); }, diff --git a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx index c66760f00f..78f1ea8dfa 100644 --- a/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx +++ b/clients/web/src/components/groups/StructuredOutputPanel/StructuredOutputPanel.stories.tsx @@ -41,7 +41,7 @@ export const Nested: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect( + await expect( canvas.getByRole("heading", { name: "Structured Output" }), ).toBeInTheDocument(); // The section starts expanded, so the payload is visible without a click. @@ -78,9 +78,9 @@ export const Large: Story = { } // Capped: the visible height stays at the section's `mah`, well under the // payload's natural height… - expect(viewport.clientHeight).toBeLessThanOrEqual(400); + await expect(viewport.clientHeight).toBeLessThanOrEqual(400); // …and the overflow is reachable by scrolling rather than clipped away. - expect(viewport.scrollHeight).toBeGreaterThan(viewport.clientHeight); + await expect(viewport.scrollHeight).toBeGreaterThan(viewport.clientHeight); }, }; @@ -95,7 +95,7 @@ export const Collapsed: Story = { const toggle = canvas.getByRole("button", { name: "Expand structured output", }); - expect(toggle).toHaveAttribute("aria-expanded", "false"); + await expect(toggle).toHaveAttribute("aria-expanded", "false"); await userEvent.click(toggle); await waitFor(() => expect( diff --git a/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.stories.tsx b/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.stories.tsx index 4a6c64e8b3..bba498b330 100644 --- a/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.stories.tsx +++ b/clients/web/src/components/groups/UrlElicitationErrorModal/UrlElicitationErrorModal.stories.tsx @@ -47,10 +47,10 @@ export const Default: Story = { // Mantine renders the modal in a portal at document.body. const body = within(canvasElement.ownerDocument.body); await body.findByText("URL elicitation required"); - expect(body.getByText(/trigger-url-elicitation/)).toBeInTheDocument(); + await expect(body.getByText(/trigger-url-elicitation/)).toBeInTheDocument(); const details = body.getByLabelText("Error details") as HTMLTextAreaElement; - expect(details.value).toContain("-32042"); - expect(details.readOnly).toBe(true); + await expect(details.value).toContain("-32042"); + await expect(details.readOnly).toBe(true); }, }; diff --git a/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.stories.tsx b/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.stories.tsx index b1de0328c6..ebcdeb0551 100644 --- a/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.stories.tsx +++ b/clients/web/src/components/screens/ResourcesScreen/ResourcesScreen.stories.tsx @@ -235,7 +235,7 @@ export const SubscriptionsUnsupported: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(canvas.queryByText(/Subscriptions/)).not.toBeInTheDocument(); + await expect(canvas.queryByText(/Subscriptions/)).not.toBeInTheDocument(); }, }; @@ -256,7 +256,7 @@ export const ModernSubscriptionStream: Story = { }, play: async ({ canvasElement }) => { const canvas = within(canvasElement); - expect(await canvas.findByText("Listening")).toBeInTheDocument(); + await expect(await canvas.findByText("Listening")).toBeInTheDocument(); }, }; @@ -281,18 +281,22 @@ export const ManyResources: Story = { const sidebar = sidebarCard.getBoundingClientRect(); const detail = detailCard.getBoundingClientRect(); // The sidebar matches the detail panel's height (same bottom baseline). - expect(Math.abs(sidebar.bottom - detail.bottom)).toBeLessThanOrEqual(1); - expect(Math.abs(sidebar.height - detail.height)).toBeLessThanOrEqual(1); + await expect(Math.abs(sidebar.bottom - detail.bottom)).toBeLessThanOrEqual( + 1, + ); + await expect(Math.abs(sidebar.height - detail.height)).toBeLessThanOrEqual( + 1, + ); // The section headers stay pinned (visible, in document order), so the // whole accordion doesn't scroll as one block. const controls = canvasElement.querySelectorAll( ".disclosure-sections .mantine-Accordion-control", ); - expect(controls).toHaveLength(3); + await expect(controls).toHaveLength(3); const tops = [...controls].map((c) => c.getBoundingClientRect().top); - expect(tops[0]).toBeLessThan(tops[1]); - expect(tops[1]).toBeLessThan(tops[2]); + await expect(tops[0]).toBeLessThan(tops[1]); + await expect(tops[1]).toBeLessThan(tops[2]); // The long URIs section scrolls within its own panel, with a thin styled // scrollbar matching the Mantine ScrollArea look (#1466). @@ -302,13 +306,15 @@ export const ManyResources: Story = { if (!(urisPanel instanceof HTMLElement)) { throw new Error("URIs panel not found"); } - expect(urisPanel.scrollHeight).toBeGreaterThan(urisPanel.clientHeight); + await expect(urisPanel.scrollHeight).toBeGreaterThan( + urisPanel.clientHeight, + ); const panelStyle = getComputedStyle(urisPanel); - expect(panelStyle.scrollbarWidth).toBe("thin"); + await expect(panelStyle.scrollbarWidth).toBe("thin"); // The themed translucent thumb resolves (the `--inspector-scrollbar-thumb` // token, an rgba value, vs. a transparent track). WebKit pseudo-element // styles aren't queryable, so `scrollbar-color` is the assertable proxy. - expect(panelStyle.scrollbarColor).toContain("rgba"); + await expect(panelStyle.scrollbarColor).toContain("rgba"); }, }; diff --git a/clients/web/src/components/screens/ServerListScreen/ServerListScreen.stories.tsx b/clients/web/src/components/screens/ServerListScreen/ServerListScreen.stories.tsx index 89e1e6912f..3eca532b85 100644 --- a/clients/web/src/components/screens/ServerListScreen/ServerListScreen.stories.tsx +++ b/clients/web/src/components/screens/ServerListScreen/ServerListScreen.stories.tsx @@ -83,7 +83,7 @@ export const MultipleServers: Story = { // The server grid scroll region reserves a scrollbar gutter and hides the // bar when idle, matching the Protocol/Network/Logging list panels (#1474). play: async ({ canvasElement }) => { - expectScrollbarGutterIdleHidden(canvasElement); + await expectScrollbarGutterIdleHidden(canvasElement); }, }; @@ -148,7 +148,7 @@ export const KeyboardReorder: Story = { await waitFor(() => expect(args.onReorder).toHaveBeenCalled()); // The first card swapped places with the second; the third is unmoved. - expect(args.onReorder).toHaveBeenCalledWith([ + await expect(args.onReorder).toHaveBeenCalledWith([ disconnectedStdioServer.id, connectedStdioServer.id, failedHttpServer.id, diff --git a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx index 5b21872646..fd7f25ac33 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.stories.tsx @@ -452,10 +452,10 @@ export const Default: Story = { if (!(main instanceof HTMLElement)) { throw new Error("AppShell main not found"); } - expect(getComputedStyle(main).overflowY).toBe("hidden"); + await expect(getComputedStyle(main).overflowY).toBe("hidden"); // Clamped to the viewport (within a pixel of rounding), so the shell can't // grow past it and push the page into scrolling. - expect(main.getBoundingClientRect().height).toBeLessThanOrEqual( + await expect(main.getBoundingClientRect().height).toBeLessThanOrEqual( window.innerHeight + 1, ); }, @@ -489,8 +489,8 @@ export const ManyServers: Story = { if (!(main instanceof HTMLElement)) { throw new Error("AppShell main not found"); } - expect(getComputedStyle(main).overflowY).toBe("hidden"); - expect(main.getBoundingClientRect().bottom).toBeLessThanOrEqual( + await expect(getComputedStyle(main).overflowY).toBe("hidden"); + await expect(main.getBoundingClientRect().bottom).toBeLessThanOrEqual( window.innerHeight + 1, ); // The overflow lives in the inner server-grid ScrollArea, which is actually @@ -499,7 +499,7 @@ export const ManyServers: Story = { if (!(viewport instanceof HTMLElement)) { throw new Error("server-grid scroll viewport not found"); } - expect(viewport.scrollHeight).toBeGreaterThan(viewport.clientHeight); + await expect(viewport.scrollHeight).toBeGreaterThan(viewport.clientHeight); }, }; diff --git a/clients/web/src/test/PreviewAnnotations.stories.tsx b/clients/web/src/test/PreviewAnnotations.stories.tsx index bca4fb481c..c05b0ae461 100644 --- a/clients/web/src/test/PreviewAnnotations.stories.tsx +++ b/clients/web/src/test/PreviewAnnotations.stories.tsx @@ -69,25 +69,27 @@ type Story = StoryObj; export const ProjectAnnotationsApplied: Story = { play: async ({ canvasElement, parameters }) => { const canvas = within(canvasElement); - expect(canvas.getByRole("button", { name: "Themed" })).toBeInTheDocument(); + await expect( + canvas.getByRole("button", { name: "Themed" }), + ).toBeInTheDocument(); const root = getComputedStyle(document.documentElement); // `MantineProvider` (the `./preview` decorator) injects the theme's CSS // variables onto `:root`. No provider, no variables. - expect(root.getPropertyValue("--mantine-primary-color-filled").trim()).toBe( - expectedPrimaryColor(), - ); + await expect( + root.getPropertyValue("--mantine-primary-color-filled").trim(), + ).toBe(expectedPrimaryColor()); // `--inspector-brand-primary` is defined in `App.css`, which only reaches // the story through `./preview`'s stylesheet import — and it resolves // *through* the Mantine variable above, so this covers both layers. - expect(root.getPropertyValue("--inspector-brand-primary").trim()).toBe( - expectedPrimaryColor(), - ); + await expect( + root.getPropertyValue("--inspector-brand-primary").trim(), + ).toBe(expectedPrimaryColor()); // The preview `parameters` merged into the story context — the same channel // that carries `a11y: { test: "error" }` to the a11y addon. - expect(parameters.a11y).toMatchObject({ test: "error" }); + await expect(parameters.a11y).toMatchObject({ test: "error" }); }, }; diff --git a/clients/web/src/test/core/auth/ema/transportProvider.test.ts b/clients/web/src/test/core/auth/ema/transportProvider.test.ts index b6a3b32a4a..ff66653e40 100644 --- a/clients/web/src/test/core/auth/ema/transportProvider.test.ts +++ b/clients/web/src/test/core/auth/ema/transportProvider.test.ts @@ -117,12 +117,12 @@ describe("EmaTransportOAuthProvider", () => { expect(await provider.clientInformation()).toEqual({ client_id: "abc" }); expect(await provider.codeVerifier()).toBe("verifier-xyz"); - provider.saveClientInformation({ client_id: "new" } as never); + await provider.saveClientInformation({ client_id: "new" } as never); expect(inner.saveClientInformation).toHaveBeenCalledWith({ client_id: "new", }); - provider.saveCodeVerifier("cv"); + await provider.saveCodeVerifier("cv"); expect(inner.saveCodeVerifier).toHaveBeenCalledWith("cv"); }); diff --git a/clients/web/src/test/core/mcp/oauthManager.test.ts b/clients/web/src/test/core/mcp/oauthManager.test.ts index 99af81bfdc..04d9b80f44 100644 --- a/clients/web/src/test/core/mcp/oauthManager.test.ts +++ b/clients/web/src/test/core/mcp/oauthManager.test.ts @@ -1588,7 +1588,7 @@ describe("OAuthManager", () => { }); const provider = await manager.createOAuthProviderForTransport(); - provider.redirectToAuthorization( + await provider.redirectToAuthorization( new URL("https://as.example.com/authorize?client_id=abc&state=xyz"), ); @@ -1610,7 +1610,7 @@ describe("OAuthManager", () => { }); const provider = await manager.createOAuthProviderForTransport(); - provider.redirectToAuthorization( + await provider.redirectToAuthorization( new URL("https://as.example.com/authorize?client_id=abc&state=xyz"), ); diff --git a/clients/web/src/test/scrollAreaStoryAssertions.ts b/clients/web/src/test/scrollAreaStoryAssertions.ts index cd3fe28e44..b3de5bbfb1 100644 --- a/clients/web/src/test/scrollAreaStoryAssertions.ts +++ b/clients/web/src/test/scrollAreaStoryAssertions.ts @@ -6,22 +6,24 @@ import { expect } from "storybook/test"; * hidden when idle (`type="scroll"`, so it shows only while scrolling rather * than on hover). Shared by the Protocol/Network/Logging panel stories (#1474). */ -export function expectScrollbarGutterIdleHidden(canvasElement: HTMLElement) { +export async function expectScrollbarGutterIdleHidden( + canvasElement: HTMLElement, +) { const viewport = canvasElement.querySelector(".mantine-ScrollArea-viewport"); if (!(viewport instanceof HTMLElement)) { throw new Error("scroll viewport not found"); } // offsetScrollbars reserves a non-zero inline-end gutter for the scrollbar. const pad = parseFloat(getComputedStyle(viewport).paddingInlineEnd); - expect(pad).toBeGreaterThan(0); + await expect(pad).toBeGreaterThan(0); // type="scroll" → scrollbars are mounted but hidden until the user scrolls. // Assert at least one exists first so the loop isn't trivially-true if a // future change stops rendering the scrollbar element. const scrollbars = canvasElement.querySelectorAll( ".mantine-ScrollArea-scrollbar", ); - expect(scrollbars.length).toBeGreaterThan(0); + await expect(scrollbars.length).toBeGreaterThan(0); for (const bar of scrollbars) { - expect(bar.getAttribute("data-state")).toBe("hidden"); + await expect(bar.getAttribute("data-state")).toBe("hidden"); } } diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index d0ee6ea175..0a2408f97a 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -99,6 +99,22 @@ import { ZodError } from "zod"; */ const SSE_PRIMING_COMMENT = ":\n\n"; +/** + * Close an SSE stream from inside an `onAbort` listener, discarding the + * rejection. + * + * `close()` is async and the peer is, by definition, already gone here — a + * write that loses the race rejects with nothing left to recover. The listener + * itself cannot own the promise either: Hono's `abort()` invokes subscribers + * with a bare `subscriber()` (`hono/utils/stream`), so a promise *returned* + * from the listener is floated by Hono rather than awaited, turning an + * `async` listener into the same unhandled rejection one layer up. Swallowing + * it here is the only place the rejection has an owner. + */ +function closeAbortedStream(stream: { close(): Promise }): void { + stream.close().catch(() => {}); +} + /** * Shape of the initial config returned by GET /api/config (defaults for client). */ @@ -854,7 +870,7 @@ export function createRemoteApp( stream.onAbort(() => { // Client disconnected - clear event consumer const shouldCleanup = session.clearEventConsumer(); - stream.close(); + closeAbortedStream(stream); // If transport is dead and no client connected, cleanup session if (shouldCleanup || session.isTransportDead()) { @@ -2460,7 +2476,7 @@ export function createRemoteApp( stream.onAbort(() => { serverEventSubscribers.delete(send); void maybeStopWatcher(); - stream.close(); + closeAbortedStream(stream); }); // Hono closes the stream the moment this callback returns, so hold the diff --git a/eslint.config.js b/eslint.config.js index dfa945d1b7..b133e61252 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -10,6 +10,31 @@ import { defineConfig, globalIgnores } from "eslint/config"; // isomorphic TypeScript (browser-side OAuth + Node backends + shared runtime); // the shared surface is Node-only tooling/tests. Neither has JSX, so no React // plugin is needed. +// Type-aware linting for the two blocks below. `no-floating-promises` needs +// type information, which `tseslint.configs.recommended` (the non-type-aware +// set) does not provide — so each block adds a parser project (#1959). +// +// `tsconfig.lint.json` exists for this and nothing else: the root has no +// tsconfig of its own (`core/` is typechecked through `clients/web`'s +// `tsc -b`, `test-servers/src` through `clients/cli`'s test project), and a +// parser project must literally *contain* every file it is asked to lint. +const typeAware = { + languageOptions: { + parserOptions: { + project: ["./tsconfig.lint.json"], + tsconfigRootDir: import.meta.dirname, + }, + }, + rules: { + // The class this catches is invisible at review time: the call reads like + // an awaited one minus four characters, and the unhandled rejection it + // produces surfaces in a different test, in a different file, as a stack + // pointing at SDK internals — which is how #1947 came to fail the whole + // `npm run ci` chain from two un-held `callTool` promises. + "@typescript-eslint/no-floating-promises": "error", + }, +}; + const sharedRules = { // An `_`-prefix is the explicit "intentionally unused" marker — // interface-conformance params in fakes, destructuring-rest omissions, @@ -59,4 +84,15 @@ export default defineConfig([ }, rules: sharedRules, }, + { + // Type-aware pass over the same surface, minus `eslint.config.js` — it is + // JavaScript, so no tsconfig contains it and asking the parser for a + // project would fail it outright rather than lint it. + files: [ + "core/**/*.{ts,tsx}", + "test-servers/src/**/*.{ts,tsx,mts,cts}", + "vitest.shared.mts", + ], + ...typeAware, + }, ]); diff --git a/tsconfig.lint.json b/tsconfig.lint.json new file mode 100644 index 0000000000..1a119f9650 --- /dev/null +++ b/tsconfig.lint.json @@ -0,0 +1,34 @@ +{ + // Parser project for the ROOT eslint config's type-aware rules (#1959), and + // nothing else — it emits nothing and gates nothing. + // + // `@typescript-eslint/no-floating-promises` needs a program containing the + // file being linted, and the root scope (`core/**`, `test-servers/src/**`, + // `vitest.shared.mts`) has no tsconfig of its own: `core/` is typechecked + // through `clients/web`'s `tsc -b` and `test-servers/src` through + // `clients/cli`'s test project, neither of which is rooted here. Type + // *checking* stays where it is; this file only gives the parser a program. + // + // `moduleResolution: bundler` because `core/` is written for a bundler + // (extensionless relative imports) — under NodeNext those fail to resolve, + // every import degrades to `any`, and the rule silently stops seeing + // promises at all. + "extends": "./tsconfig.base.json", + "compilerOptions": { + "noEmit": true, + "module": "ESNext", + "moduleResolution": "bundler", + "lib": ["ES2023", "DOM", "DOM.Iterable"], + "jsx": "react-jsx", + "types": ["node"], + "typeRoots": ["./clients/web/node_modules/@types", "./node_modules/@types"] + }, + "include": [ + "core/**/*.ts", + "core/**/*.tsx", + "test-servers/src/**/*.ts", + "test-servers/src/**/*.mts", + "test-servers/src/**/*.cts", + "vitest.shared.mts" + ] +} From 5d8094357de7bd37841bc07a3e5576bae2c9ddba Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:16:34 -0400 Subject: [PATCH 2/4] fix(tui): surface a disconnect failure independently of connection status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch (Copilot, PR #2039): folding the rejected disconnect into `connectError` meant nobody ever saw it. InfoTab renders that value only when `serverState.status === "error"`, and a failed disconnect leaves the status "connected" — so the message existed in state and never on screen. Worse, it fed `connectError ?? inspectorLastError`, where a stale value would go on masking a later, real connection error. Give it its own `disconnectError` state and a header banner that renders regardless of status, cleared on the next connect or disconnect attempt and on a server switch, so it can neither linger nor be attributed to the wrong server. The tests now assert the rendered banner rather than noting that nothing is visible. Signed-off-by: cliffhall --- clients/tui/__tests__/App.test.tsx | 35 ++++++++++++++++++------------ clients/tui/src/App.tsx | 26 +++++++++++++++++++--- 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index e4879b0226..8a5124f556 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -665,17 +665,14 @@ describe("App (foundation)", () => { // 'd' is a key handler, so it cannot await handleDisconnect — the handler // has to own the failure itself or it escapes as an unhandled rejection // and fails the whole run from somewhere else (#1959). - // There is nothing to assert on screen — the message only renders once the - // client reports `status: "error"`, which a rejected disconnect does not - // do. The assertion is the run itself: vitest fails on an unhandled - // rejection, so this test passing is the proof the catch owns it. + // The banner is deliberately independent of connection status: a rejected + // disconnect leaves the status "connected", so anything gated on + // `status === "error"` would never be seen. h.ctrl.status = "connected"; h.disconnect.mockRejectedValue(new Error("discfail")); - const { stdin } = await mount(oneStdio()); - stdin.write("d"); - await tick(); - await tick(); - expect(h.disconnect).toHaveBeenCalled(); + const r = await mount(oneStdio()); + r.stdin.write("d"); + await expectFrame(r, "Disconnect failed: discfail"); }); it("owns a non-Error disconnect rejection too", async () => { @@ -683,11 +680,21 @@ describe("App (foundation)", () => { // `.message` off it; a throw here would escape the same way. h.ctrl.status = "connected"; h.disconnect.mockRejectedValue("plainstring"); - const { stdin } = await mount(oneStdio()); - stdin.write("d"); - await tick(); - await tick(); - expect(h.disconnect).toHaveBeenCalled(); + const r = await mount(oneStdio()); + r.stdin.write("d"); + await expectFrame(r, "Disconnect failed: plainstring"); + }); + + it("clears a disconnect failure once a retry succeeds", async () => { + // A stale banner would keep reporting a failure the user has since fixed. + h.ctrl.status = "connected"; + h.disconnect.mockRejectedValueOnce(new Error("discfail")); + const r = await mount(oneStdio()); + r.stdin.write("d"); + await expectFrame(r, "Disconnect failed: discfail"); + r.stdin.write("d"); + await waitUntil(() => !(r.lastFrame() ?? "").includes("Disconnect failed")); + expect(r.lastFrame() ?? "").not.toContain("Disconnect failed"); }); it("switches tabs via accelerator keys", async () => { diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 3f0f931d08..0c779ab7de 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -170,6 +170,15 @@ function App({ pendingStepUpRef.current = pendingStepUp; }, [pendingStepUp]); const [connectError, setConnectError] = useState(null); + // A failed disconnect is deliberately NOT folded into `connectError`. That + // one is only rendered by InfoTab when the status is "error", which a + // rejected disconnect leaves untouched (the status stays "connected"), so + // the message would never be seen — and it feeds + // `connectError ?? inspectorLastError`, where a stale value would go on + // masking a later, real connection error. This renders in the header + // regardless of status and is cleared on the next connect/disconnect + // attempt and on a server switch. + const [disconnectError, setDisconnectError] = useState(null); const oauthInProgressRef = useRef(false); const callbackServerRef = useRef(null); const selectedServerRef = useRef(null); @@ -434,6 +443,9 @@ function App({ useEffect(() => { setOauthStatus("idle"); setOauthMessage(null); + // The header banner is server-scoped, so a failure from the server we just + // left must not be read as this one's. + setDisconnectError(null); const stepUp = pendingStepUpRef.current; if (stepUp && selectedServer && stepUp.serverName !== selectedServer) { setPendingStepUp(null); @@ -757,6 +769,8 @@ function App({ if (!selectedServer || !selectedInspectorClient || !selectedServerConfig) { return; } + // A connect attempt supersedes whatever the last disconnect reported. + setDisconnectError(null); const finishConnect = async () => { await connectInspector(); @@ -887,14 +901,15 @@ function App({ // Disconnect handler const handleDisconnect = useCallback(async () => { if (!selectedServer) return; + // Clear first, so a retry that succeeds leaves no stale message behind. + setDisconnectError(null); try { await disconnectInspector(); // InspectorClient will update status automatically, and data is preserved } catch (err) { // Nothing above this catches: the only caller is the key handler, which - // cannot await. Surface the failure the same way handleConnect does - // rather than letting it escape as an unhandled rejection. - setConnectError(err instanceof Error ? err.message : String(err)); + // cannot await, so without this the rejection escapes unhandled. + setDisconnectError(err instanceof Error ? err.message : String(err)); } }, [selectedServer, disconnectInspector]); @@ -1627,6 +1642,11 @@ function App({ OAuth: {oauthMessage} )} + {disconnectError && ( + + Disconnect failed: {disconnectError} + + )} From d830213796e579a5764924cbabd20ab9853959e3 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 21:52:43 -0400 Subject: [PATCH 3/4] =?UTF-8?q?fix:=20address=20round-2=20review=20?= =?UTF-8?q?=E2=80=94=20stale=20disconnect,=20lint-project=20gap,=20coverag?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 2 (PR #2039) surfaced no new visible comments but six suppressed ones. Four were actionable. `clients/tui/src/App.tsx` — a stale-completion race in the new banner: start disconnecting server A, switch to B, then let A's disconnect reject, and A's failure appears under B. Rapid same-server retries had the same shape, and the stale attempt is the one most likely to reject. Claim a monotonic attempt token before awaiting and publish only if it is still current and the server has not changed; `handleConnect` bumps the same counter so a connect retires an in-flight disconnect. The rejection is still owned when superseded — just not published. The regression test drives a deferred rejection across a server switch, and fails without the guard. `tsconfig.lint.json` — the root type-aware block matches `test-servers/src/**/*.tsx` but the parser project omitted that extension, so such a file would have failed with "not found in any of the provided project(s)" instead of being linted. Added, and AGENTS.md now states the invariant: the include must stay a superset of the lint globs. `clients/web/server/{server,vite-hono-plugin}.ts` — the `open()` rejection branch was untested, and duplicated. Both were fixed by removing the duplication: `openBrowser` now lives beside `printServerBanner` in `web-server-config.ts`, with one implementation to test. Two copies of a one-line `.catch` is two chances for one to drift back to a bare `open(url)`. The dev plugin is not booted in tests — it returns early under VITEST by design — but it calls the same tested helper. `core/mcp/remote/node/server.ts` — `closeAbortedStream`'s rejecting-close path, the entire reason the helper exists, had no test. Exported it (as this file already does for `requestIdForSendWait` / `mcpParamHeadersOnly`) and covered both legs, asserting no unhandledRejection fires. Also corrects AGENTS.md, which said each client lists "both" tsconfig projects — true for cli/tui/launcher, but web lists four leaf projects. Signed-off-by: cliffhall --- AGENTS.md | 2 +- clients/tui/__tests__/App.test.tsx | 25 +++ clients/tui/src/App.tsx | 23 ++- clients/web/server/server.ts | 12 +- clients/web/server/vite-hono-plugin.ts | 11 +- clients/web/server/web-server-config.ts | 19 +++ .../mcp/remote/server-extra-coverage.test.ts | 34 ++++ .../server/server-auto-open.test.ts | 149 ++++++++++++++++++ core/mcp/remote/node/server.ts | 2 +- tsconfig.lint.json | 1 + 10 files changed, 259 insertions(+), 19 deletions(-) create mode 100644 clients/web/src/test/integration/server/server-auto-open.test.ts diff --git a/AGENTS.md b/AGENTS.md index 93d0eaacac..db73995c56 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -723,7 +723,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab - **The class it catches is invisible at review time.** A floated call reads like an awaited one minus four characters, and the unhandled rejection it produces surfaces somewhere else entirely — a different test, a different file, a stack pointing at SDK internals. Two un-held `client.callTool(...)` promises made `npm run ci` unpassable in #1947: `disconnect()` rejected them with `Connection closed`, the unhandled rejection failed the whole vitest run, and the chain aborted at `coverage`, silently skipping `verify:build-gate`, `smoke`, and Storybook. Attributing it took a full investigation; the fix was two lines. - **Prefer holding and settling the promise.** `void` is an escape hatch, not a fix — it is visible at review time (strictly better than nothing) but still discards the rejection. Reach for it only when the callee already owns its failures (it ends in a `catch` that surfaces the message) or the caller genuinely cannot await — a synchronous `useEffect` body, an Ink `useInput` key handler, a Hono `stream.onAbort` listener. Say **which** of those it is in a one-line comment; an unexplained `void` is a review finding. Where the callee does *not* own its failures, give it a `catch` rather than voiding the call (see `handleDisconnect` in `clients/tui/src/App.tsx`), or terminate with `.catch(…)` at the call site (see `open(url)` in `clients/web/server/{server,vite-hono-plugin}.ts`). - **In Storybook play functions, `expect(...)` from `storybook/test` returns a promise.** Storybook instruments it so the interactions panel can trace each assertion, so every `expect` in a play function is awaited — as is any shared helper that wraps one (`src/test/scrollAreaStoryAssertions.ts` is `async` for this reason). - - **The rule is type-aware, so each scope's ESLint config carries a parser project.** The four clients point at their own tsconfig projects (both of them, since `src` and `__tests__`/`server` live in different ones); the root config points at **`tsconfig.lint.json`**, which exists solely to give the parser a program covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts` — none of which is rooted in a tsconfig of its own. That file emits nothing and gates nothing: type *checking* for those sources stays where it was (`core/` through `clients/web`'s `tsc -b`, `test-servers/src` through `clients/cli`'s test project). It sets `moduleResolution: bundler` deliberately — `core/` uses extensionless relative imports, which NodeNext fails to resolve, degrading every import to `any` so the rule silently stops seeing promises at all. **When adding a first-party TS location to the root lint scope, add it to `tsconfig.lint.json`'s `include` too**, or the parser fails it outright with "was not found in any of the provided project(s)". Note also that a tsconfig `include` does **not** expand braces — `core/**/*.{ts,tsx}` matches nothing; list the extensions separately. + - **The rule is type-aware, so each scope's ESLint config carries a parser project.** Each client's config must name **every leaf project covering its lint surface**, since the parser needs a program that literally *contains* the file — for cli, tui, and launcher that is the two they already typecheck (`tsconfig.json` + `tsconfig.test.json`, `src` in the first and `__tests__` only in the second), and for **web it is four** (`tsconfig.app.json`, `tsconfig.node.json`, `tsconfig.storybook.json`, `tsconfig.test.json`) — web's `tsconfig.json` is a solution file with `files: []`, so naming it alone would contain nothing. Adding a leaf project to a client means adding it here too. The root config instead points at **`tsconfig.lint.json`**, which exists solely to give the parser a program covering `core/**`, `test-servers/src/**`, and `vitest.shared.mts` — none of which is rooted in a tsconfig of its own. That file emits nothing and gates nothing: type *checking* for those sources stays where it was (`core/` through `clients/web`'s `tsc -b`, `test-servers/src` through `clients/cli`'s test project). It sets `moduleResolution: bundler` deliberately — `core/` uses extensionless relative imports, which NodeNext fails to resolve, degrading every import to `any` so the rule silently stops seeing promises at all. **Its `include` must stay a superset of the root config's type-aware `files` globs** — a file the lint block matches but the project omits fails outright with "was not found in any of the provided project(s)" rather than being checked, so widening one means widening the other (that is why the `include` carries `test-servers/src/**/*.tsx`, which nothing has produced yet). Note also that a tsconfig `include` does **not** expand braces — `core/**/*.{ts,tsx}` matches nothing; list the extensions separately. - Type-aware linting costs real time: `clients/web`'s `eslint .` went from ~8s to ~19s, and `lint` runs inside `validate`, the fast inner-loop check. That is the price of the guarantee; if it needs reducing later, narrowing the projects each scope loads is the lever, not dropping the rule. ## Web source layout: `src/lib` vs `src/utils` diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index 8a5124f556..4f7a25c89c 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -697,6 +697,31 @@ describe("App (foundation)", () => { expect(r.lastFrame() ?? "").not.toContain("Disconnect failed"); }); + it("drops a disconnect rejection that lands after the user switched servers", async () => { + // The stale attempt is the one that usually rejects, so without an + // attempt token server alpha's failure would surface in beta's header. + h.ctrl.status = "connected"; + let rejectDisconnect: (err: Error) => void = () => {}; + h.disconnect.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectDisconnect = reject; + }), + ); + const r = await mount(stdioServer()); + r.stdin.write("d"); // alpha's disconnect starts, and hangs + await tick(); + await press(r, [DOWN]); // switch to beta + await expectFrame(r, "beta"); + rejectDisconnect(new Error("stale-alpha-failure")); + // Give the rejection every chance to be published before asserting it + // wasn't — a bare tick would pass even with the guard removed. + await waitUntil(() => + (r.lastFrame() ?? "").includes("stale-alpha-failure"), + ); + expect(r.lastFrame() ?? "").not.toContain("stale-alpha-failure"); + }); + it("switches tabs via accelerator keys", async () => { const r = await mount(stdioServer()); await press(r, ["t"]); // tools tab (server is auto-selected) diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 0c779ab7de..6e04f04e00 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -170,6 +170,8 @@ function App({ pendingStepUpRef.current = pendingStepUp; }, [pendingStepUp]); const [connectError, setConnectError] = useState(null); + // Monotonic token for in-flight disconnects — see handleDisconnect. + const disconnectAttemptRef = useRef(0); // A failed disconnect is deliberately NOT folded into `connectError`. That // one is only rendered by InfoTab when the status is "error", which a // rejected disconnect leaves untouched (the status stays "connected"), so @@ -769,7 +771,9 @@ function App({ if (!selectedServer || !selectedInspectorClient || !selectedServerConfig) { return; } - // A connect attempt supersedes whatever the last disconnect reported. + // A connect attempt supersedes whatever the last disconnect reported — + // including one still in flight, which is what the counter bump retires. + disconnectAttemptRef.current++; setDisconnectError(null); const finishConnect = async () => { @@ -901,6 +905,14 @@ function App({ // Disconnect handler const handleDisconnect = useCallback(async () => { if (!selectedServer) return; + // Claim this attempt before awaiting. A disconnect can still be in flight + // when the user switches servers or presses 'd' again, and it is the + // *stale* one that usually rejects — so the catch below must be able to + // tell "my failure" from "one that has since been superseded", or server + // A's error lands in server B's header. `handleConnect` bumps the same + // counter for the same reason. + const attempt = ++disconnectAttemptRef.current; + const attemptServer = selectedServer; // Clear first, so a retry that succeeds leaves no stale message behind. setDisconnectError(null); try { @@ -908,7 +920,14 @@ function App({ // InspectorClient will update status automatically, and data is preserved } catch (err) { // Nothing above this catches: the only caller is the key handler, which - // cannot await, so without this the rejection escapes unhandled. + // cannot await, so without this the rejection escapes unhandled. A + // superseded one is still owned here — just not published. + if ( + disconnectAttemptRef.current !== attempt || + selectedServerRef.current !== attemptServer + ) { + return; + } setDisconnectError(err instanceof Error ? err.message : String(err)); } }, [selectedServer, disconnectInspector]); diff --git a/clients/web/server/server.ts b/clients/web/server/server.ts index 9d864ce221..fe46553042 100644 --- a/clients/web/server/server.ts +++ b/clients/web/server/server.ts @@ -9,7 +9,6 @@ import { readFileSync } from "node:fs"; import { join, dirname } from "node:path"; import { fileURLToPath } from "node:url"; import { randomBytes } from "node:crypto"; -import open from "open"; import { serve } from "@hono/node-server"; import { serveStatic } from "@hono/node-server/serve-static"; import { Hono } from "hono"; @@ -22,6 +21,7 @@ import type { WebServerConfig } from "./web-server-config.js"; import { webServerConfigToInitialPayload, printServerBanner, + openBrowser, } from "./web-server-config.js"; import type { WebServerHandle } from "./types.js"; @@ -125,13 +125,9 @@ export async function startHonoServer( sandboxUrl ?? undefined, ); if (config.autoOpen) { - // A headless or otherwise browser-less host rejects here. The server - // is already listening and the banner above printed the URL, so warn - // and carry on rather than letting the rejection escape this - // synchronous listen callback and take the process down. - open(url).catch((err: unknown) => { - console.warn("Could not open a browser automatically:", err); - }); + // Never a bare `open(url)`: this callback is synchronous, so a + // rejection would escape it. See openBrowser. + openBrowser(url); } }, ); diff --git a/clients/web/server/vite-hono-plugin.ts b/clients/web/server/vite-hono-plugin.ts index f07b1af6e1..880913e663 100644 --- a/clients/web/server/vite-hono-plugin.ts +++ b/clients/web/server/vite-hono-plugin.ts @@ -9,7 +9,6 @@ import type { IncomingMessage, ServerResponse } from "node:http"; import type { Plugin } from "vite"; -import open from "open"; // Direct leaf import — the `core/mcp/remote/node/index.ts` barrel re-exports // from `../constants.js`, which would otherwise drag the broader // `core/mcp/remote/index.ts` barrel (including `createRemoteLogger`, with its @@ -22,6 +21,7 @@ import type { WebServerConfig } from "./web-server-config.js"; import { webServerConfigToInitialPayload, printServerBanner, + openBrowser, } from "./web-server-config.js"; export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { @@ -114,12 +114,9 @@ export function honoMiddlewarePlugin(config: WebServerConfig): Plugin { ); if (config.autoOpen) { - // Same as the prod server: a browser-less host rejects, the dev - // server is already up, and the banner above printed the URL — so - // warn rather than let the rejection escape this sync helper. - open(url).catch((err: unknown) => { - console.warn("Could not open a browser automatically:", err); - }); + // Never a bare `open(url)`: `logBanner` is synchronous, so a + // rejection would escape it. See openBrowser. + openBrowser(url); } }; diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index 6e77bd28e1..6bb13be24d 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -2,6 +2,7 @@ * Config object for the web server (dev and prod). Passed in-process; no env handoff. */ +import open from "open"; import pino from "pino"; import type { Logger } from "pino"; import type { MCPConfig, MCPServerConfig } from "../../../core/mcp/types.ts"; @@ -203,6 +204,24 @@ export function printServerBanner( return url; } +/** + * Launch the user's browser at `url`, reporting a failure rather than throwing. + * + * Both callers — the prod Hono server's `serve()` listen callback and the dev + * plugin's `logBanner` — are synchronous, so a rejection has nowhere to go and + * escapes as an unhandled rejection (#1959). On a headless or otherwise + * browser-less host that rejection is the ordinary outcome, not an edge case, + * and by the time it happens the server is already listening and + * `printServerBanner` has printed a perfectly usable URL. So warn and carry + * on. Shared between the two servers deliberately: two copies of a one-line + * `.catch` is two chances for one of them to drift back to a bare `open(url)`. + */ +export function openBrowser(url: string): void { + open(url).catch((err: unknown) => { + console.warn("Could not open a browser automatically:", err); + }); +} + export interface BuildWebServerConfigOptions { initialMcpConfig?: MCPServerConfig | null; /** Catalog/session file the backend should serve; see {@link WebServerConfig.mcpConfigPath}. */ diff --git a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts index 9a44f07c0a..427800a67b 100644 --- a/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts +++ b/clients/web/src/test/integration/mcp/remote/server-extra-coverage.test.ts @@ -20,6 +20,7 @@ import { serve } from "@hono/node-server"; import type { ServerType } from "@hono/node-server"; import type pinoType from "pino"; import { + closeAbortedStream, createRemoteApp, requestIdForSendWait, mcpParamHeadersOnly, @@ -1107,4 +1108,37 @@ describe("server.ts supplemental coverage", () => { } }); }); + describe("closeAbortedStream", () => { + it("owns a rejected close instead of letting it go unhandled", async () => { + // The whole reason the helper exists. Both SSE `onAbort` listeners run + // after the peer is already gone, so `close()` can lose the race and + // reject — and Hono invokes abort subscribers with a bare + // `subscriber()`, so nothing upstream would catch it. A regression here + // does not fail at the call site; it fails the whole vitest run from + // wherever the rejection happens to surface. + const rejected = Promise.reject(new Error("peer already gone")); + const unhandled: unknown[] = []; + const onUnhandled = (err: unknown) => unhandled.push(err); + process.on("unhandledRejection", onUnhandled); + try { + expect(closeAbortedStream({ close: () => rejected })).toBeUndefined(); + // An unhandledRejection fires on a later macrotask, not this one. + await new Promise((resolve) => setTimeout(resolve, 20)); + } finally { + process.off("unhandledRejection", onUnhandled); + } + expect(unhandled).toEqual([]); + }); + + it("returns without waiting on a close that resolves", async () => { + let closed = false; + closeAbortedStream({ + close: async () => { + closed = true; + }, + }); + await Promise.resolve(); + expect(closed).toBe(true); + }); + }); }); diff --git a/clients/web/src/test/integration/server/server-auto-open.test.ts b/clients/web/src/test/integration/server/server-auto-open.test.ts new file mode 100644 index 0000000000..527f8b560b --- /dev/null +++ b/clients/web/src/test/integration/server/server-auto-open.test.ts @@ -0,0 +1,149 @@ +/** + * `autoOpen` browser launch (#1959). + * + * Both servers call `openBrowser` from a synchronous callback — the prod + * server from `serve()`'s listen callback, the dev plugin from `logBanner` — + * so a rejection there has nowhere to go. On a headless or otherwise + * browser-less host that rejection is the *ordinary* outcome, not an edge + * case, and before #1959 it escaped as an unhandled rejection from a server + * that had already started listening and printed a perfectly usable URL. + * + * The helper is shared precisely so this behavior has one implementation to + * pin. The unit tests below cover it directly; the end-to-end test then + * proves the prod server routes through it and survives the failure. The dev + * plugin is not booted here — it returns early under `VITEST` by design (see + * `vite-hono-plugin.ts`) — but it calls the same tested helper. + */ +import { describe, it, expect, afterEach, vi } from "vitest"; +import { createServer } from "node:net"; +import { mkdtemp, writeFile, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import type { WebServerConfig } from "../../../../server/web-server-config.js"; +import type { WebServerHandle } from "../../../../server/types.js"; + +const openMock = vi.hoisted(() => vi.fn()); +vi.mock("open", () => ({ default: openMock })); + +// Imported after the mock is registered so both bind to the double. +const { openBrowser } = await import("../../../../server/web-server-config.js"); +const { startHonoServer } = await import("../../../../server/server.js"); + +const WARNING = "Could not open a browser automatically:"; + +async function findFreePort(): Promise { + return new Promise((resolve, reject) => { + const srv = createServer(); + srv.unref(); + srv.on("error", reject); + srv.listen(0, "127.0.0.1", () => { + const addr = srv.address(); + if (addr && typeof addr === "object") { + const { port } = addr; + srv.close(() => resolve(port)); + } else { + srv.close(() => reject(new Error("Could not resolve a free port"))); + } + }); + }); +} + +/** An unhandledRejection fires on a later macrotask than the rejection. */ +async function settle() { + await new Promise((resolve) => setTimeout(resolve, 50)); +} + +const INDEX_HTML = + "
"; + +let handle: WebServerHandle | undefined; +let staticRoot: string | undefined; + +afterEach(async () => { + await handle?.close(); + handle = undefined; + if (staticRoot) await rm(staticRoot, { recursive: true, force: true }); + staticRoot = undefined; + openMock.mockReset(); + vi.restoreAllMocks(); +}); + +describe("openBrowser", () => { + it("owns a rejection and warns instead of letting it go unhandled", async () => { + // Suppress the expected warning rather than letting it pollute the run, + // but assert on it — the warning IS the user-visible half of the fix. + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const unhandled: unknown[] = []; + const onUnhandled = (err: unknown) => unhandled.push(err); + process.on("unhandledRejection", onUnhandled); + try { + openMock.mockRejectedValue(new Error("no browser on this host")); + expect(openBrowser("http://127.0.0.1:6274")).toBeUndefined(); + await settle(); + } finally { + process.off("unhandledRejection", onUnhandled); + } + expect(unhandled).toEqual([]); + expect(warn).toHaveBeenCalledWith(WARNING, expect.any(Error)); + }); + + it("stays quiet when the browser opens", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + openMock.mockResolvedValue(undefined); + openBrowser("http://127.0.0.1:6274"); + await settle(); + expect(openMock).toHaveBeenCalledWith("http://127.0.0.1:6274"); + expect(warn).not.toHaveBeenCalledWith(WARNING, expect.anything()); + }); +}); + +describe("startHonoServer autoOpen", () => { + it("keeps serving when the browser cannot be opened", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const unhandled: unknown[] = []; + const onUnhandled = (err: unknown) => unhandled.push(err); + process.on("unhandledRejection", onUnhandled); + + try { + openMock.mockRejectedValue(new Error("no browser on this host")); + + staticRoot = await mkdtemp(join(tmpdir(), "inspector-autoopen-")); + await writeFile(join(staticRoot, "index.html"), INDEX_HTML, "utf-8"); + const port = await findFreePort(); + const baseUrl = `http://127.0.0.1:${port}`; + const config: WebServerConfig = { + port, + hostname: "127.0.0.1", + authToken: "auto-open-token", + dangerouslyOmitAuth: false, + initialMcpConfig: null, + mcpConfigPath: undefined, + writable: true, + initialServers: null, + storageDir: undefined, + allowedOrigins: [baseUrl], + sandboxPort: 0, + sandboxHost: "127.0.0.1", + logger: undefined, + autoOpen: true, + staticRoot, + }; + handle = await startHonoServer(config); + + // `serve()`'s listen callback fires after startHonoServer resolves. + for (let i = 0; i < 100 && openMock.mock.calls.length === 0; i++) { + await new Promise((resolve) => setTimeout(resolve, 10)); + } + await settle(); + + expect(openMock).toHaveBeenCalledWith(expect.stringContaining(baseUrl)); + expect(unhandled).toEqual([]); + expect(warn).toHaveBeenCalledWith(WARNING, expect.any(Error)); + // The point of warning rather than throwing: the server is still up. + const res = await fetch(`${baseUrl}/`); + expect(res.status).toBe(200); + } finally { + process.off("unhandledRejection", onUnhandled); + } + }); +}); diff --git a/core/mcp/remote/node/server.ts b/core/mcp/remote/node/server.ts index 0a2408f97a..9494b0f917 100644 --- a/core/mcp/remote/node/server.ts +++ b/core/mcp/remote/node/server.ts @@ -111,7 +111,7 @@ const SSE_PRIMING_COMMENT = ":\n\n"; * `async` listener into the same unhandled rejection one layer up. Swallowing * it here is the only place the rejection has an owner. */ -function closeAbortedStream(stream: { close(): Promise }): void { +export function closeAbortedStream(stream: { close(): Promise }): void { stream.close().catch(() => {}); } diff --git a/tsconfig.lint.json b/tsconfig.lint.json index 1a119f9650..4fb3447a11 100644 --- a/tsconfig.lint.json +++ b/tsconfig.lint.json @@ -27,6 +27,7 @@ "core/**/*.ts", "core/**/*.tsx", "test-servers/src/**/*.ts", + "test-servers/src/**/*.tsx", "test-servers/src/**/*.mts", "test-servers/src/**/*.cts", "vitest.shared.mts" From 85e3215534a203a74e4225f599388632a52cead6 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 22:11:40 -0400 Subject: [PATCH 4/4] fix(tui): retire the disconnect token on a server switch, not just clear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot round 3 (PR #2039): clearing the banner when `selectedServer` changes is not enough. Start disconnecting A, switch A → B → A, and by the time the original request rejects both guards match again — the attempt token is unchanged and the selected server is A once more — so the stale failure lands on the freshly re-selected A. Bump the token on every switch, so a round trip invalidates prior disconnects the same way a one-way switch does. New test drives exactly that interleaving; it fails without the bump (the one-way test does not, which is why this needed its own). Also attaches the before/after TUI screenshots the review asked for — the review was right that "no UI change" stopped being true once the banner was added — and corrects the PR description accordingly. Signed-off-by: cliffhall --- clients/tui/__tests__/App.test.tsx | 24 ++++++++++++++++++++++++ clients/tui/src/App.tsx | 6 +++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/clients/tui/__tests__/App.test.tsx b/clients/tui/__tests__/App.test.tsx index 4f7a25c89c..11b65af583 100644 --- a/clients/tui/__tests__/App.test.tsx +++ b/clients/tui/__tests__/App.test.tsx @@ -722,6 +722,30 @@ describe("App (foundation)", () => { expect(r.lastFrame() ?? "").not.toContain("stale-alpha-failure"); }); + it("drops a stale disconnect rejection across an A → B → A round trip", async () => { + // The server-name check alone passes here: by the time the rejection + // lands, alpha is selected again. Only retiring the attempt token on + // every switch catches it. + h.ctrl.status = "connected"; + let rejectDisconnect: (err: Error) => void = () => {}; + h.disconnect.mockImplementationOnce( + () => + new Promise((_resolve, reject) => { + rejectDisconnect = reject; + }), + ); + const r = await mount(stdioServer()); + r.stdin.write("d"); // alpha's disconnect starts, and hangs + await tick(); + await press(r, [DOWN]); // alpha -> beta + await expectFrame(r, "b.js"); + await press(r, [UP]); // beta -> alpha again + await expectFrame(r, "s.js"); + rejectDisconnect(new Error("round-trip-failure")); + await waitUntil(() => (r.lastFrame() ?? "").includes("round-trip-failure")); + expect(r.lastFrame() ?? "").not.toContain("round-trip-failure"); + }); + it("switches tabs via accelerator keys", async () => { const r = await mount(stdioServer()); await press(r, ["t"]); // tools tab (server is auto-selected) diff --git a/clients/tui/src/App.tsx b/clients/tui/src/App.tsx index 6e04f04e00..240bbd82fc 100644 --- a/clients/tui/src/App.tsx +++ b/clients/tui/src/App.tsx @@ -446,7 +446,11 @@ function App({ setOauthStatus("idle"); setOauthMessage(null); // The header banner is server-scoped, so a failure from the server we just - // left must not be read as this one's. + // left must not be read as this one's. Clearing is not enough: a switch + // away and back (A → B → A) would leave both the attempt token and the + // server name matching again, so the stale rejection would land on the + // re-selected A. Retiring the token on every switch is what closes that. + disconnectAttemptRef.current++; setDisconnectError(null); const stepUp = pendingStepUpRef.current; if (stepUp && selectedServer && stepUp.serverName !== selectedServer) {