fix(web): degrade the connected header to the catalog name for an empty reported serverInfo name (#1774) (#1775)
* fix(web): degrade the connected header to the catalog name for an empty reported serverInfo name (#1774) A server that reports serverInfo with an empty name string (an empty-but-present name) left ViewHeader rendering a nameless title. App's `??` fallback only fires when the whole serverInfo object is absent (the #1772 modern-omitted case), so a reported `{ name: "" }` slipped through with `serverInfo.name === ""`. Resolve the header's display name in InspectorView: fall back to the active server's catalog name when the reported name is blank. This is a display-only fallback — the Connection Info modal stays faithful to the raw report because it reads App's untouched `initializeResult` + `serverInfoReported`, so `serverInfoReported` keeps meaning "the server sent a serverInfo object" (a reported-but-empty name still reads as "—" in the modal, and any reported version stays intact — driving the flag off name emptiness would wrongly mask the version). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * fix(web): degrade a whitespace-only reported server name; strengthen tests + cross-reference (#1774) Review follow-ups on PR #1775: - resolveHeaderServerInfo now guards with `serverInfo.name.trim()`, so a whitespace-only reported name (" ") — the same non-conforming class as an empty string — also degrades to the catalog name instead of rendering a blank-looking header title. - Strengthen the no-catalog-match test: assert the connected header chrome (Disconnect control) actually renders inside role="banner", making it a real regression guard rather than a vacuous absent-string check. - Add a whitespace-only header-fallback test. - Add a back-reference in App's initializeResult memo comment pointing down to InspectorView's resolveHeaderServerInfo, closing the loop between the two fallbacks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * fix(web): trim whitespace-only reported name/version in the Connection Info modal too (#1774) Second-review follow-up on PR #1775. The header now degrades a whitespace-only reported name; the modal had the same gap — `serverInfo.name || "—"` treats " " as truthy and renders a visually blank row, the exact outcome the `||` exists to prevent. Trim both the name and version rows before the fallback so a blank-but-present value reads as "—". Uses optional chaining (`?.trim()`) to preserve the prior tolerance of a non-conforming runtime-absent field (the type is non-null, but a server can omit it — covered by the existing "version missing" test). Stays faithful: the fallback is still "—", never a borrowed catalog name. Applied the same `?.` hardening to the header's resolveHeaderServerInfo guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * test(web): pin the runtime-absent-name tolerance in header + modal (#1774) Third-review follow-up on PR #1775. Commit 2f508ffa added `serverInfo.name?.trim()` in both ConnectionInfoContent and resolveHeaderServerInfo, but only the version `?.` short-circuit was exercised (by the pre-existing "version missing" test) — the symmetric name `?.` was uncovered. Add two tests passing a serverInfo that omits `name` (`{ version: "1.0.0" } as never`): the modal Name row reads "—", and the header degrades to the catalog name. Pins the documented tolerance and makes the two new branches explicit rather than relying on gate headroom. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
820d38c438
commit
28016ea0c8
5 changed files with 176 additions and 6 deletions
|
|
@ -1504,6 +1504,11 @@ function App() {
|
|||
// any connect (legacy included) between the `connected` status dispatch and the
|
||||
// `serverInfo` dispatch, which land in a single React render. The modal uses
|
||||
// `serverInfoReported` (above), not this name, to stay faithful.
|
||||
//
|
||||
// This `??` only covers an *absent* serverInfo. A server that *reports* a
|
||||
// blank name (`{ name: "" }`) is degraded for display a layer below, in
|
||||
// InspectorView's `resolveHeaderServerInfo` (#1774) — kept there so this
|
||||
// faithful object (and thus the modal) never carries a borrowed name.
|
||||
const initializeResult = useMemo<InitializeResult | undefined>(() => {
|
||||
if (connectionStatus !== "connected") return undefined;
|
||||
const resolvedServerInfo = serverInfo ?? {
|
||||
|
|
|
|||
|
|
@ -96,6 +96,42 @@ describe("ConnectionInfoContent", () => {
|
|||
expect(screen.getAllByText("—")).toHaveLength(3);
|
||||
});
|
||||
|
||||
it("renders an em-dash when the reported name is missing", () => {
|
||||
renderWithMantine(
|
||||
<ConnectionInfoContent
|
||||
initializeResult={{
|
||||
...fullResult,
|
||||
// Non-conforming server: `name` is runtime-absent (the field is typed
|
||||
// non-null). Pins the `?.trim()` tolerance — symmetric with the
|
||||
// "version is missing" test above — so it reads "—", not a crash.
|
||||
serverInfo: { version: "1.0.0" } as never,
|
||||
}}
|
||||
clientCapabilities={fullClientCaps}
|
||||
transport="stdio"
|
||||
/>,
|
||||
);
|
||||
expect(screen.getByText("1.0.0")).toBeInTheDocument();
|
||||
expect(screen.getAllByText("—")).toHaveLength(3);
|
||||
});
|
||||
|
||||
it("renders an em-dash when the reported name is whitespace-only", () => {
|
||||
renderWithMantine(
|
||||
<ConnectionInfoContent
|
||||
initializeResult={{
|
||||
...fullResult,
|
||||
serverInfo: { name: " ", version: "1.0.0" },
|
||||
}}
|
||||
clientCapabilities={fullClientCaps}
|
||||
transport="stdio"
|
||||
/>,
|
||||
);
|
||||
// A whitespace-only reported name is the same non-conforming class as an
|
||||
// empty one (#1774): it must read as unknown ("—"), not a visually blank
|
||||
// row. Stays faithful — never borrows the catalog name.
|
||||
expect(screen.getByText("1.0.0")).toBeInTheDocument();
|
||||
expect(screen.getAllByText("—")).toHaveLength(3);
|
||||
});
|
||||
|
||||
it("shows 'not reported' for name and version when serverInfo was not reported", () => {
|
||||
renderWithMantine(
|
||||
<ConnectionInfoContent
|
||||
|
|
|
|||
|
|
@ -206,14 +206,20 @@ export function ConnectionInfoContent({
|
|||
initializeResult;
|
||||
|
||||
// Only trust `serverInfo` when the server actually reported it; otherwise the
|
||||
// name is a catalog fallback. Both rows use `||` (not `??`) so a reported-but-
|
||||
// empty name/version reads as unknown ("—") rather than a blank row —
|
||||
// `initialize` mandates the fields, not non-empty values.
|
||||
// name is a catalog fallback. Both rows `?.trim()` before the `||` (not `??`)
|
||||
// so a reported-but-blank name/version — empty, whitespace-only (" "), or a
|
||||
// non-conforming runtime-absent field — reads as unknown ("—") rather than a
|
||||
// blank row. The optional chain preserves the prior tolerance of a missing
|
||||
// field (the field is typed non-null, but a non-conforming server can omit
|
||||
// it); whitespace-only is the same class InspectorView's
|
||||
// `resolveHeaderServerInfo` handles for the header (#1774). `initialize`
|
||||
// mandates the fields, not non-empty values. Stays faithful: the fallback is
|
||||
// "—", never a borrowed catalog name.
|
||||
const displayName = serverInfoReported
|
||||
? serverInfo.name || "—"
|
||||
? serverInfo.name?.trim() || "—"
|
||||
: SERVER_INFO_NOT_REPORTED_LABEL;
|
||||
const displayVersion = serverInfoReported
|
||||
? serverInfo.version || "—"
|
||||
? serverInfo.version?.trim() || "—"
|
||||
: SERVER_INFO_NOT_REPORTED_LABEL;
|
||||
|
||||
const serverCaps = getCapabilityEntries(capabilities, SERVER_CAPABILITY_KEYS);
|
||||
|
|
|
|||
|
|
@ -326,6 +326,103 @@ describe("InspectorView", () => {
|
|||
expect(screen.getByRole("switch")).toBeChecked();
|
||||
});
|
||||
|
||||
it("falls back to the catalog name in the header when the reported serverInfo name is empty (#1774)", () => {
|
||||
// A non-conforming server reports serverInfo with an empty name string.
|
||||
// `App`'s `??` fallback only fires when the whole object is absent, so the
|
||||
// header would otherwise render a nameless title. The view degrades to the
|
||||
// active server's catalog name ("Alpha") so the header still identifies the
|
||||
// server. Scoped to the header (role="banner") to exclude the ServerCard,
|
||||
// which shows the catalog name unconditionally.
|
||||
renderWithMantine(
|
||||
<StatefulInspectorViewHost
|
||||
{...makeProps({
|
||||
servers: [sampleServer],
|
||||
activeServer: "alpha",
|
||||
connectionStatus: "connected",
|
||||
initializeResult: {
|
||||
...connectedInit,
|
||||
serverInfo: { name: "", version: "1.0.0" },
|
||||
},
|
||||
})}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
within(screen.getByRole("banner")).getByText("Alpha"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("falls back to the catalog name in the header when the reported serverInfo name is missing (#1774)", () => {
|
||||
// Non-conforming server: `serverInfo` omits `name` entirely (the field is
|
||||
// typed non-null). Pins the `?.trim()` tolerance in resolveHeaderServerInfo
|
||||
// — a runtime-absent name degrades to the catalog name, it doesn't throw.
|
||||
renderWithMantine(
|
||||
<StatefulInspectorViewHost
|
||||
{...makeProps({
|
||||
servers: [sampleServer],
|
||||
activeServer: "alpha",
|
||||
connectionStatus: "connected",
|
||||
initializeResult: {
|
||||
...connectedInit,
|
||||
serverInfo: { version: "1.0.0" } as never,
|
||||
},
|
||||
})}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
within(screen.getByRole("banner")).getByText("Alpha"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("falls back to the catalog name in the header when the reported serverInfo name is whitespace-only (#1774)", () => {
|
||||
// A whitespace-only reported name (" ") is the same non-conforming class
|
||||
// as an empty string — truthy, so a naive `if (serverInfo.name)` guard would
|
||||
// let it through and render a blank-looking title. The `.trim()` guard
|
||||
// degrades it to the catalog name like the empty case.
|
||||
renderWithMantine(
|
||||
<StatefulInspectorViewHost
|
||||
{...makeProps({
|
||||
servers: [sampleServer],
|
||||
activeServer: "alpha",
|
||||
connectionStatus: "connected",
|
||||
initializeResult: {
|
||||
...connectedInit,
|
||||
serverInfo: { name: " ", version: "1.0.0" },
|
||||
},
|
||||
})}
|
||||
/>,
|
||||
);
|
||||
expect(
|
||||
within(screen.getByRole("banner")).getByText("Alpha"),
|
||||
).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("still renders the connected header when the reported name is blank and no catalog entry matches (#1774)", () => {
|
||||
// Blank reported name AND no catalog server to borrow from: the connected
|
||||
// header must still render (the connection is live) rather than crash or
|
||||
// invent a label — it just shows no server name. Asserting the Disconnect
|
||||
// control inside the banner makes this a real regression guard for the
|
||||
// no-catalog-match branch, not merely a coverage-only test.
|
||||
renderWithMantine(
|
||||
<StatefulInspectorViewHost
|
||||
{...makeProps({
|
||||
servers: [],
|
||||
activeServer: "ghost",
|
||||
connectionStatus: "connected",
|
||||
initializeResult: {
|
||||
...connectedInit,
|
||||
serverInfo: { name: "", version: "1.0.0" },
|
||||
},
|
||||
})}
|
||||
/>,
|
||||
);
|
||||
const header = screen.getByRole("banner");
|
||||
expect(
|
||||
within(header).getByRole("button", { name: "Disconnect from server" }),
|
||||
).toBeInTheDocument();
|
||||
// No reported name and nothing to borrow, so the header shows no catalog name.
|
||||
expect(within(header).queryByText("ghost")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("surfaces the negotiated protocol version on the active connected card", () => {
|
||||
renderWithMantine(
|
||||
<StatefulInspectorViewHost
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ import {
|
|||
} from "@mantine/core";
|
||||
import { useLocalStorage } from "@mantine/hooks";
|
||||
import type {
|
||||
Implementation,
|
||||
InitializeResult,
|
||||
LoggingLevel,
|
||||
Prompt,
|
||||
|
|
@ -170,6 +171,27 @@ const PROTOCOL_TAB = "Protocol";
|
|||
const NETWORK_TAB = "Network";
|
||||
const CONSOLE_TAB = "Console";
|
||||
|
||||
// Resolve the server name the connected header renders. A server may report
|
||||
// `serverInfo` with a blank `name` (#1774) — empty or whitespace-only — and the
|
||||
// header must never show a nameless title, so fall back to the active server's
|
||||
// catalog name when the reported name is blank (`.trim()` catches " ", the
|
||||
// same non-conforming class an empty string is). This is a display-only
|
||||
// fallback: the Connection Info modal stays faithful to the raw report because
|
||||
// it reads App's untouched `initializeResult` + `serverInfoReported`, not this
|
||||
// resolved value. (App already folds the catalog name into
|
||||
// `initializeResult.serverInfo` for the modern-omitted case where serverInfo is
|
||||
// absent entirely (#1772); this covers the reported-but-blank-name case that
|
||||
// `??` fallback doesn't reach.)
|
||||
function resolveHeaderServerInfo(
|
||||
serverInfo: Implementation,
|
||||
servers: ServerEntry[],
|
||||
activeServerId: string | undefined,
|
||||
): Implementation {
|
||||
if (serverInfo.name?.trim()) return serverInfo;
|
||||
const catalogName = servers.find((s) => s.id === activeServerId)?.name;
|
||||
return catalogName ? { ...serverInfo, name: catalogName } : serverInfo;
|
||||
}
|
||||
|
||||
const ALL_TABS: string[] = [
|
||||
SERVERS_TAB,
|
||||
"Apps",
|
||||
|
|
@ -1274,7 +1296,11 @@ export function InspectorView({
|
|||
{connectionStatus === "connected" && initializeResult ? (
|
||||
<ViewHeader
|
||||
connected
|
||||
serverInfo={initializeResult.serverInfo}
|
||||
serverInfo={resolveHeaderServerInfo(
|
||||
initializeResult.serverInfo,
|
||||
serversInput,
|
||||
activeServer,
|
||||
)}
|
||||
status={connectionStatus}
|
||||
latencyMs={latencyMs}
|
||||
activeTab={activeTab}
|
||||
|
|
|
|||
Loading…
Reference in a new issue