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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw
This commit is contained in:
parent
ea8715c9c4
commit
a26cbac609
4 changed files with 47 additions and 6 deletions
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue