From badd1506457978d483eb413cdf841a568dd69699 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 12:08:11 -0400 Subject: [PATCH 1/4] fix(web): degrade the connected header to the catalog name for an empty reported serverInfo name (#1774) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- .../InspectorView/InspectorView.test.tsx | 47 +++++++++++++++++++ .../views/InspectorView/InspectorView.tsx | 26 +++++++++- 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index 86907e2dd..f617ab3af 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -326,6 +326,53 @@ 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( + , + ); + expect( + within(screen.getByRole("banner")).getByText("Alpha"), + ).toBeInTheDocument(); + }); + + it("renders the connected header without borrowing a name when the reported name is empty and no catalog entry matches (#1774)", () => { + // Empty reported name AND no catalog server to fall back to: the header + // still renders (connection is live) but has no name to show, rather than + // crashing or inventing a label. Exercises the no-catalog-match branch. + renderWithMantine( + , + ); + const header = screen.getByRole("banner"); + expect(header).toBeInTheDocument(); + expect(within(header).queryByText("Alpha")).not.toBeInTheDocument(); + }); + it("surfaces the negotiated protocol version on the active connected card", () => { renderWithMantine( s.id === activeServerId)?.name; + return catalogName ? { ...serverInfo, name: catalogName } : serverInfo; +} + const ALL_TABS: string[] = [ SERVERS_TAB, "Apps", @@ -1274,7 +1294,11 @@ export function InspectorView({ {connectionStatus === "connected" && initializeResult ? ( Date: Sat, 25 Jul 2026 12:42:22 -0400 Subject: [PATCH 2/4] fix(web): degrade a whitespace-only reported server name; strengthen tests + cross-reference (#1774) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- clients/web/src/App.tsx | 5 +++ .../InspectorView/InspectorView.test.tsx | 40 ++++++++++++++++--- .../views/InspectorView/InspectorView.tsx | 20 +++++----- 3 files changed, 50 insertions(+), 15 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 68a8af159..1d089d602 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -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(() => { if (connectionStatus !== "connected") return undefined; const resolvedServerInfo = serverInfo ?? { diff --git a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx index f617ab3af..dac532d44 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.test.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.test.tsx @@ -351,10 +351,35 @@ describe("InspectorView", () => { ).toBeInTheDocument(); }); - it("renders the connected header without borrowing a name when the reported name is empty and no catalog entry matches (#1774)", () => { - // Empty reported name AND no catalog server to fall back to: the header - // still renders (connection is live) but has no name to show, rather than - // crashing or inventing a label. Exercises the no-catalog-match branch. + 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( + , + ); + 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( { />, ); const header = screen.getByRole("banner"); - expect(header).toBeInTheDocument(); - expect(within(header).queryByText("Alpha")).not.toBeInTheDocument(); + 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", () => { diff --git a/clients/web/src/components/views/InspectorView/InspectorView.tsx b/clients/web/src/components/views/InspectorView/InspectorView.tsx index 357b51fa3..fd34445c0 100644 --- a/clients/web/src/components/views/InspectorView/InspectorView.tsx +++ b/clients/web/src/components/views/InspectorView/InspectorView.tsx @@ -172,20 +172,22 @@ const NETWORK_TAB = "Network"; const CONSOLE_TAB = "Console"; // Resolve the server name the connected header renders. A server may report -// `serverInfo` with an empty `name` (#1774) — the header must never show a -// nameless title, so 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`, 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-empty-name case that `??` fallback doesn't reach.) +// `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) return serverInfo; + if (serverInfo.name.trim()) return serverInfo; const catalogName = servers.find((s) => s.id === activeServerId)?.name; return catalogName ? { ...serverInfo, name: catalogName } : serverInfo; } From 2f508ffa8add1fb73d36e951ebadd2860e16dbde Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 12:52:46 -0400 Subject: [PATCH 3/4] fix(web): trim whitespace-only reported name/version in the Connection Info modal too (#1774) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- .../ConnectionInfoContent.test.tsx | 18 ++++++++++++++++++ .../ConnectionInfoContent.tsx | 16 +++++++++++----- .../views/InspectorView/InspectorView.tsx | 2 +- 3 files changed, 30 insertions(+), 6 deletions(-) diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx index a17e380f6..594bd3978 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -96,6 +96,24 @@ describe("ConnectionInfoContent", () => { expect(screen.getAllByText("—")).toHaveLength(3); }); + it("renders an em-dash when the reported name is whitespace-only", () => { + renderWithMantine( + , + ); + // 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( s.id === activeServerId)?.name; return catalogName ? { ...serverInfo, name: catalogName } : serverInfo; } From dfdb50f38cd3fa3c2c136560ab5bd3e4bd930b2d Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 25 Jul 2026 13:00:57 -0400 Subject: [PATCH 4/4] test(web): pin the runtime-absent-name tolerance in header + modal (#1774) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --- .../ConnectionInfoContent.test.tsx | 18 +++++++++++++++ .../InspectorView/InspectorView.test.tsx | 22 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx index 594bd3978..9f9a8ea4a 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -96,6 +96,24 @@ describe("ConnectionInfoContent", () => { expect(screen.getAllByText("—")).toHaveLength(3); }); + it("renders an em-dash when the reported name is missing", () => { + renderWithMantine( + , + ); + 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( { ).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( + , + ); + 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