From a26cbac609dbf4a9efb1583949abcebb5c6329f4 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 10 Jul 2026 22:40:34 -0400 Subject: [PATCH] review: harden version read + fix MonitoringScreen docstring Address PR #1641 review feedback: - Add readInspectorVersionSafe() (non-throwing) in core/node/version.ts and use it in the web backend, so a failure to resolve the root manifest hides the (cosmetic) version badge instead of crashing the backend at startup. Covered by tests for both the resolved and unresolvable paths. - Update MonitoringScreen's docstrings: it now wraps every supplied screen in a ScreenStage (only the active tab mounts, cross-fading) rather than rendering just the active tab's node. The third fetch-consolidation suggestion (one useInitialConfig instead of three GET /api/config hooks) is tracked as a follow-up in #1643. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw --- clients/web/server/web-server-config.ts | 8 +++++--- .../MonitoringScreen/MonitoringScreen.tsx | 8 +++++--- .../src/test/integration/node/version.test.ts | 17 ++++++++++++++++ core/node/version.ts | 20 +++++++++++++++++++ 4 files changed, 47 insertions(+), 6 deletions(-) diff --git a/clients/web/server/web-server-config.ts b/clients/web/server/web-server-config.ts index bc34588e..d40a18ac 100644 --- a/clients/web/server/web-server-config.ts +++ b/clients/web/server/web-server-config.ts @@ -14,13 +14,15 @@ import { LEGACY_AUTH_TOKEN_ENV, } from "../../../core/mcp/remote/constants.ts"; import type { InitialConfigPayload } from "../../../core/mcp/remote/node/server.ts"; -import { readInspectorVersion } from "../../../core/node/version.ts"; +import { readInspectorVersionSafe } from "../../../core/node/version.ts"; import { resolveSandboxPort } from "./sandbox-controller.js"; // The single-source Inspector version (root package.json), read once at load. // The browser can't read the filesystem the way the CLI/TUI do, so the backend -// reads it here and hands it to the client via GET /api/config. -const inspectorVersion = readInspectorVersion(import.meta.url); +// reads it here and hands it to the client via GET /api/config. Uses the +// non-throwing read: the version is cosmetic, so a resolution failure hides the +// badge (version omitted from the payload) rather than crashing the backend. +const inspectorVersion = readInspectorVersionSafe(import.meta.url); export interface WebServerConfig { port: number; diff --git a/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.tsx b/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.tsx index 1667382e..2e577ca9 100644 --- a/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.tsx +++ b/clients/web/src/components/groups/MonitoringScreen/MonitoringScreen.tsx @@ -6,7 +6,8 @@ import { ScreenStage } from "../../elements/ScreenStage/ScreenStage"; export interface MonitoringScreenProps { /** Available monitor tabs (Logs/Protocol/Network, filtered by capability). */ tabs: string[]; - /** Active monitor tab; keys into `screens` to pick what renders below. */ + /** Active monitor tab; the matching screen is the mounted one below (the rest + * are cross-faded out via `ScreenStage`). */ value: string; onChange: (tab: string) => void; /** Search text for the active screen; wired by the caller to its filter state. */ @@ -38,8 +39,9 @@ const ScreenSlot = Stack.withProps({ /** * The pinned monitoring column's content (#1616): a `MonitoringControls` tab row - * over the currently-selected monitor screen. Layout-only — it renders whichever - * embedded screen node the caller supplies for the active tab. + * over the selected monitor screen. Layout-only — it wraps each supplied screen + * node in a `ScreenStage`, so switching tabs cross-fades (only the active tab's + * screen is mounted) the same way the primary pane does. */ export function MonitoringScreen({ tabs, diff --git a/clients/web/src/test/integration/node/version.test.ts b/clients/web/src/test/integration/node/version.test.ts index 6bb8d0bb..459822f7 100644 --- a/clients/web/src/test/integration/node/version.test.ts +++ b/clients/web/src/test/integration/node/version.test.ts @@ -11,6 +11,7 @@ import { join, dirname } from "path"; import { pathToFileURL, fileURLToPath } from "url"; import { readInspectorVersion, + readInspectorVersionSafe, ROOT_PACKAGE_NAME, } from "@inspector/core/node/version.js"; @@ -67,3 +68,19 @@ describe("readInspectorVersion", () => { } }); }); + +describe("readInspectorVersionSafe", () => { + it("returns the version when the root manifest resolves", () => { + expect(readInspectorVersionSafe(import.meta.url)).toBe(readRootVersion()); + }); + + it("returns undefined instead of throwing when the root can't be resolved", () => { + // A caller rooted in the OS temp dir (outside the repo) has no inspector + // root manifest above it — readInspectorVersion would throw, but the safe + // variant swallows it so a cosmetic version read never crashes the caller. + const callerUrl = pathToFileURL( + join(tmpdir(), "nowhere", "caller.js"), + ).href; + expect(readInspectorVersionSafe(callerUrl)).toBeUndefined(); + }); +}); diff --git a/core/node/version.ts b/core/node/version.ts index 063612cc..367f8cef 100644 --- a/core/node/version.ts +++ b/core/node/version.ts @@ -64,3 +64,23 @@ export function readInspectorVersion(callerUrl: string): string { `Could not locate the ${ROOT_PACKAGE_NAME} root package.json above ${startDir}`, ); } + +/** + * Non-throwing variant of {@link readInspectorVersion}: returns `undefined` + * instead of throwing when the root manifest can't be resolved. For callers + * where the version is cosmetic (e.g. the web backend surfacing it to the + * browser) — a resolution failure should hide the version, not take the caller + * down at startup. + * + * @param callerUrl the calling module's `import.meta.url` + * @returns the root version, or `undefined` if it can't be resolved + */ +export function readInspectorVersionSafe( + callerUrl: string, +): string | undefined { + try { + return readInspectorVersion(callerUrl); + } catch { + return undefined; + } +}