From 28016ea0c86ea235a71e26db020e5c64a0da3360 Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Sat, 25 Jul 2026 14:41:42 -0400 Subject: [PATCH] fix(web): degrade the connected header to the catalog name for an empty reported serverInfo name (#1774) (#1775) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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) 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) 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) 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) Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 --------- Co-authored-by: Claude Opus 4.8 (1M context) --- clients/web/src/App.tsx | 5 + .../ConnectionInfoContent.test.tsx | 36 +++++++ .../ConnectionInfoContent.tsx | 16 ++- .../InspectorView/InspectorView.test.tsx | 97 +++++++++++++++++++ .../views/InspectorView/InspectorView.tsx | 28 +++++- 5 files changed, 176 insertions(+), 6 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 68a8af15..1d089d60 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/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx index a17e380f..9f9a8ea4 100644 --- a/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx +++ b/clients/web/src/components/groups/ConnectionInfoContent/ConnectionInfoContent.test.tsx @@ -96,6 +96,42 @@ 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( + , + ); + // 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( { 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("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 + // 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( + 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( 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 ? (