From 27fafe6acbbc98a1c095f1c69221ed1e5ff90b72 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 4 Sep 2026 22:57:51 -0400 Subject: [PATCH] fix(web): make the Connection Info extension sections say something MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things the Skills work got wrong in that modal, all found by review of the PR screenshots. - The "Skills Extension" section repeated `io.modelcontextprotocol/skills` as its value — the identifier the "Server Extensions" section two rows above already lists, so the section added nothing. What a flat key list *cannot* show is the extension's sub-options, which is the fact a server author opens this modal to check. Renamed "Skills Extension Options" and rendered as a ✓/✗ row for `directoryRead`, in the same vocabulary as the capability columns, so it reads as the same kind of claim. - The extension sections' contents were bold (`ValueText`, the value half of a label/value pair) while sitting directly beneath the capability checklists, which are plain. They are lists of items, not values, so they now use the same `Text` the checklist rows do. - Those lists were comma-joined into one line, which wraps mid-identifier in a half-width column. One row per identifier. The `skills-directory-read` tests now assert `data-supported` rather than the copy: "Not supported" contains "Supported", so a text assertion passed for either answer. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018kfrH3uRNUM4JAk1reKtyY Signed-off-by: cliffhall --- .../ConnectionInfoContent.test.tsx | 28 +++++-- .../ConnectionInfoContent.tsx | 82 ++++++++++++------- 2 files changed, 73 insertions(+), 37 deletions(-) diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx index d495f8f2..d181c36a 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -427,11 +427,12 @@ describe("ConnectionInfoContent", () => { expect( screen.getByText("Client Advertised Extensions"), ).toBeInTheDocument(); + // One row per identifier, not a comma-joined string: two ~30-character + // ids wrap mid-name in a half-width column. expect( - screen.getByText( - "io.modelcontextprotocol/tasks, io.modelcontextprotocol/ui", - ), + screen.getByText("io.modelcontextprotocol/tasks"), ).toBeInTheDocument(); + expect(screen.getByText("io.modelcontextprotocol/ui")).toBeInTheDocument(); }); it("renders em-dashes for the extensions sections when neither side advertises any (#1740)", () => { @@ -461,7 +462,9 @@ describe("ConnectionInfoContent", () => { protocolEra="legacy" />, ); - expect(screen.queryByText("Skills Extension")).not.toBeInTheDocument(); + expect( + screen.queryByText("Skills Extension Options"), + ).not.toBeInTheDocument(); }); it("shows the Skills extension and its directoryRead sub-flag (#2234)", () => { @@ -484,10 +487,16 @@ describe("ConnectionInfoContent", () => { protocolEra="legacy" />, ); - expect(screen.getByText("Skills Extension")).toBeInTheDocument(); - expect(screen.getByTestId("skills-directory-read")).toHaveTextContent( - "Supported", + expect(screen.getByText("Skills Extension Options")).toBeInTheDocument(); + // Asserted on the attribute, not the copy: "Not supported" contains + // "Supported", so a text check would pass for either answer. + expect(screen.getByTestId("skills-directory-read")).toHaveAttribute( + "data-supported", + "true", ); + // The section states the sub-option, not the identifier — that is already + // in "Server Extensions" and repeating it would add nothing. + expect(screen.getByText("resources/directory/read")).toBeInTheDocument(); }); it("reports directory read as unsupported for a bare skills declaration (#2234)", () => { @@ -505,8 +514,9 @@ describe("ConnectionInfoContent", () => { protocolEra="legacy" />, ); - expect(screen.getByTestId("skills-directory-read")).toHaveTextContent( - "Not supported", + expect(screen.getByTestId("skills-directory-read")).toHaveAttribute( + "data-supported", + "false", ); }); diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx index 20aa40e1..a9f567dc 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.tsx @@ -3,6 +3,7 @@ import { Button, Code, Flex, + Group, ScrollArea, SimpleGrid, Stack, @@ -18,7 +19,6 @@ import type { } from "@modelcontextprotocol/client"; import type { ServerType } from "@inspector/core/mcp/types.js"; import { TASKS_EXTENSION_KEY } from "@inspector/core/mcp/modernTaskSchemas.js"; -import { SKILLS_EXTENSION_KEY } from "@inspector/core/mcp/skillsSchemas.js"; import { getSkillsExtension } from "@inspector/core/mcp/skills.js"; import type { OAuthClientRegistrationKind } from "@inspector/core/auth/types.js"; import { @@ -106,6 +106,15 @@ const SectionHeading = Title.withProps({ // `scrollable-region-focusable`). const ValueCode = Code.withProps({ variant: "wrapping" }); +// One declared sub-option of an extension. Mirrors `CapabilityItem`'s ✓/✗ row +// rather than reusing it: that element's `capability` prop is the closed union +// of spec capability keys, and widening it to accept an arbitrary extension +// sub-option name would collapse it to `string` and lose the typo protection +// the union buys every other caller. +const SubOptionRow = Group.withProps({ gap: "xs", wrap: "nowrap" }); + +const SubOptionMark = Text.withProps({ fw: 600 }); + const ClearOAuthButton = Button.withProps({ variant: "subtle", color: "red", @@ -161,16 +170,20 @@ function formatSession( return isModernEra(era) ? "Sessionless" : "Session-based"; } -// Render an `extensions` capability map (SEP-2133) as a comma-separated list of -// its extension identifiers, or an em dash when none are present. Works for -// either side's map: the server's negotiated `capabilities.extensions` (present -// on both eras via `getServerCapabilities()`) or the Inspector's own advertised +// The extension identifiers in an `extensions` capability map (SEP-2133), one +// per rendered row, or a single em dash when none are present. Works for either +// side's map: the server's negotiated `capabilities.extensions` (present on both +// eras via `getServerCapabilities()`) or the Inspector's own advertised // `clientCapabilities.extensions`. (#1740) +// +// A list rather than a comma-joined string (#2234): an identifier is ~30 +// characters and two of them wrap mid-name in a half-width column, which is +// what made the joined form hard to read at a glance. function formatExtensions( extensions: Record | undefined, -): string { +): string[] { const keys = extensions ? Object.keys(extensions) : []; - return keys.length > 0 ? keys.join(", ") : "—"; + return keys.length > 0 ? keys : ["\u2014"]; } const SERVER_CAPABILITY_KEYS: CapabilityKey[] = [ @@ -357,34 +370,47 @@ export function ConnectionInfoContent({ Server Extensions - {formatExtensions(capabilities.extensions)} + {/* A plain `Text`, not the bold `ValueText`: these sections list + *items*, the way the capability columns above do, rather than + giving the value half of a label/value pair. Bolding them made + them read as emphasized answers to a question the section never + asks, and set them in a different font from the checklist rows + they sit directly beneath. */} + {formatExtensions(capabilities.extensions).map((extension) => ( + {extension} + ))} Client Advertised Extensions - - {formatExtensions(clientCapabilities.extensions)} - + {formatExtensions(clientCapabilities.extensions).map((extension) => ( + {extension} + ))} - {/* Skills (SEP-2640). The generic "Server Extensions" row above lists the - identifier, but not the one sub-option the extension defines — - `directoryRead`, which gates `resources/directory/read`. That flag is - exactly what a server author opens this modal to confirm, so it gets a - row of its own rather than being flattened into a key list (#2234). */} + {/* Skills (SEP-2640). The "Server Extensions" row above already names the + identifier, so repeating it here would say nothing: what this section + adds is the extension's SUB-OPTIONS, which a flat list of keys cannot + show. `directoryRead` is the only one SEP-2640 defines, and whether a + server declared it is the fact a server author opens this modal to + check — it gates `resources/directory/read` (#2234). Rendered with the + same ✓/✗ vocabulary as the capability columns above so it reads as the + same kind of claim. */} {skillsExtension && ( - - - Skills Extension - {SKILLS_EXTENSION_KEY} - - - Directory Read - - {skillsExtension.directoryRead ? "Supported" : "Not supported"} - - - + + Skills Extension Options + + + {skillsExtension.directoryRead ? "\u2713" : "\u2717"} + + + Directory read — resources/directory/read + + + )} {instructions && (