* refactor(web): sharpen src/lib vs src/utils rule and move misfiled modules (#1776) Codify the empirical rule the web client already followed loosely — utils = pure functions that compute; lib = things that instantiate, adapt, or touch the environment — in AGENTS.md and clients/web/README.md, and move the modules that sat on the wrong side of it. Moves (source + co-located test): - lib/sandbox-csp -> utils (pure CSP-string construction; type-only import) - utils/browserTabVisibility -> lib (registers visibilitychange listeners) - utils/oauthResume -> lib (reads/writes sessionStorage) - utils/clearServerOAuthState -> lib (drives live InspectorClient / OAuthStorage) Borderline audits (kept in place): pendingReauth (pure type declarations), deepLink / correlateTransportErrors / mcpNetworkHeaders / oauthUx / oauthFlow / oauthNetworkPhase (pure transforms / type-only core imports) stay in utils; downloadFile stays whole in lib (dominant side is DOM side effects). Also relocate the lone src/test/lib/downloadFile.test.ts straggler beside its source at src/lib/downloadFile.test.ts, and document the test-placement rule (side-by-side by default; src/test/ only for core/ tests, the integration project, and shared scaffolding). Pure move — no behavior change; no build/coverage config keys off the boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * refactor(web): address review — fix stale paths, move customHeaders, one-way arrows (#1776) Responds to the @claude review on PR #1784: 1. Stale path references (fixed): update the four in-repo pointers left at pre-move paths — specification/v2_auth_mid_session.md (oauthResume, browserTabVisibility → lib/), and the two load-bearing sandbox security comments (sandbox_proxy.html, sandbox-controller.ts → src/utils/sandbox-csp). 2. lib/types/customHeaders (moved): it's pure (types + pure transforms), the rule's clearest counterexample. Moved lib/types/customHeaders.{ts,test.ts} → utils/ (no external importers; only its own test) and removed the undocumented lib/types/ subdir. Documented that utils/ is the home for pure shared domain types + their constructors — no separate types/ bucket. 3. One-way arrows (item 3): declare OAuthResumeAuthKind in utils/pendingReauth and re-export it from lib/oauthResume, so the only cross-directory arrow now points lib → utils. Codified that invariant in AGENTS.md. Wording nit: AGENTS.md + README now state diagnostic logging (console.warn/error) doesn't count as a side effect for the utils rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 2 — stale customHeaders refs, rule wording (#1776) Doc-only follow-up to the round-2 @claude review on PR #1784 (no code changed): 1. Stale paths (same class as round-1 item 1): the customHeaders move left three references to the deleted lib/types/customHeaders.ts path in specification/ (v2_ux_interfaces_phase_0_4_inventory.md, v2_ux_interfaces_plan.md ×2) — updated to utils/customHeaders.ts. 2. customHeaders anchor: note in AGENTS.md that its shape is staged for ServerSettingsForm (per v2_ux_interfaces_plan.md), so its zero-importer state reads as intentional rather than rot; kept as the no-types/-bucket illustration and led the anchor list with load-bearing modules. 3. Type-only-import clause: split the conflated clause — a type-only import (pendingReauth) and re-exporting core's pure functions/constants (oauthUx/oauthFlow) are distinct reasons to stay in utils; what makes a module lib is wrapping core's stateful runtime. Fixed in both AGENTS.md and the web README. Cosmetic: re-padded the v2_auth_mid_session.md table row to column width. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 3 — scope the "no types/ bucket" claim (#1776) Doc-only follow-up to the round-3 @claude review on PR #1784 (no code changed): 1. "no separate types/ bucket" overreached: clients/web/src/types/ does exist (navigation.ts + react-syntax-highlighter .d.ts ambient stubs wired via tsconfig.app.json paths). Scoped the claim in both AGENTS.md and the web README to "no types/ sub-bucket INSIDE lib/utils", and noted src/types/ is for ambient .d.ts stubs, not domain types. 2. README example list drifted from AGENTS.md's round-2 load-bearing anchors — realigned it (inspectorTabs/mcpNetworkHeaders, dropped the zero-importer customHeaders from the headline list). 3. Clarified v2_ux_interfaces_plan.md:185 so the v2 path reads as the copy's destination, not as the v1.5 source location. Follow-up filed as #1785 (board Todo): src/types/navigation.ts InspectorTab is dead and duplicates utils/inspectorTabs.ts — kept out of this pure-move PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 4 — reconcile src/types/ claim with #1785 (#1776) Doc-only follow-up to the round-4 @claude review on PR #1784 (no code changed): The round-3 scope fix overshot: it asserted src/types/ holds "not domain types at all", but the dead navigation.ts InspectorTab is a plain-.ts domain type still living there, and specification/v2_ux_interfaces_plan.md:181-182 actively prescribes placing InspectorTab there — so AGENTS.md and that spec doc gave opposite guidance about the same directory. - AGENTS.md + web README: softened to "not the place for new domain types" and added a parenthetical that the one lingering domain type (dead navigation.ts) is tracked for removal in #1785; until then a pure domain type belongs in utils/, not src/types/. - v2_ux_interfaces_plan.md:181: marked the tab-enum placement guidance as superseded by #1785 (live type is InspectorTabId in utils/inspectorTabs.ts). Self-heals on merge of #1785. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs: address review round 5 — fix garbled #1785 clause, restructure utils rule (#1776) Doc-only follow-up to the round-5 @claude review on PR #1784 (no code changed): 1. Grammar (item 1): the round-4 #1785 clause read "superseding [#1785] will remove it" — a participle with no subject. Reworded to "the dead navigation.ts InspectorTab, which #1785 will remove". 2. Readability (item 2): the src/utils/ bullet had accreted into a single ~1,400-char blob across five review rounds. Promoted the carve-outs to scannable sub-bullets (Domain types / Diagnostic logging / Importing from core) and lifted the src/types/ note out to its own line, since it's a statement about a sibling directory, not about utils. AGENTS.md only; the README's parallel sentence was already clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XNdjEPKLG637X8YmhDiEk5 * docs+config: address review round 6 — correct the coverage-glob claim (#1776) Responds to the round-6 @claude review on PR #1784: 1. "no build/coverage glob" was factually wrong (should-fix): the web coverage include in clients/web/vite.config.ts DOES enumerate src/lib/** and src/utils/**. The conclusion (a move between them is coverage-neutral) survives, but the reason was wrong. Reworded both AGENTS.md and the web README to say nothing *enforces* the boundary — no path alias, and the include lists both dirs so a move is coverage-neutral — plus the important whitelist caveat: a module outside components/lib/utils/server falls out of the ≥90 gate entirely. 2. Removed a pre-existing duplicate glob in vite.config.ts (src/lib/**/*.{ts,tsx} appeared twice, lines 161 and 163). Inert (vitest de-dupes) but it sat three lines from the config the doc now describes. Nits: gave the README bullet the same round-5 sub-bullet restructure as AGENTS.md, and made its #1785 reference a real markdown link (bare #N doesn't autolink in rendered repo markdown). npm run ci green (config change re-verified through coverage + smokes + Storybook). 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>
187 lines
6.8 KiB
TypeScript
187 lines
6.8 KiB
TypeScript
/**
|
|
* Tests for the download helpers. Mocks URL.createObjectURL +
|
|
* URL.revokeObjectURL (happy-dom doesn't ship them) and asserts the temp
|
|
* anchor's attributes + click + deferred cleanup sequence.
|
|
*/
|
|
|
|
import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
|
|
import {
|
|
downloadBlob,
|
|
downloadJsonFile,
|
|
buildExportFilename,
|
|
fileNameFromUri,
|
|
isHttpUrl,
|
|
} from "./downloadFile";
|
|
|
|
describe("downloadBlob / downloadJsonFile", () => {
|
|
const originalCreate = URL.createObjectURL;
|
|
const originalRevoke = URL.revokeObjectURL;
|
|
let createMock: ReturnType<typeof vi.fn>;
|
|
let revokeMock: ReturnType<typeof vi.fn>;
|
|
|
|
beforeEach(() => {
|
|
vi.useFakeTimers();
|
|
createMock = vi.fn().mockReturnValue("blob:mock-url");
|
|
revokeMock = vi.fn();
|
|
// happy-dom doesn't implement URL.createObjectURL/revokeObjectURL.
|
|
URL.createObjectURL = createMock as unknown as typeof URL.createObjectURL;
|
|
URL.revokeObjectURL = revokeMock as unknown as typeof URL.revokeObjectURL;
|
|
});
|
|
|
|
afterEach(() => {
|
|
vi.useRealTimers();
|
|
URL.createObjectURL = originalCreate;
|
|
URL.revokeObjectURL = originalRevoke;
|
|
vi.restoreAllMocks();
|
|
});
|
|
|
|
it("creates an anchor with the right href + download attributes and clicks it", () => {
|
|
const clickedAnchors: HTMLAnchorElement[] = [];
|
|
const origCreateElement = document.createElement.bind(document);
|
|
vi.spyOn(document, "createElement").mockImplementation(((tag: string) => {
|
|
const el = origCreateElement(tag) as HTMLAnchorElement;
|
|
if (tag === "a") {
|
|
el.click = function click() {
|
|
clickedAnchors.push(el);
|
|
};
|
|
}
|
|
return el;
|
|
}) as typeof document.createElement);
|
|
|
|
downloadJsonFile("mcp.json", '{"hello":"world"}');
|
|
|
|
expect(createMock).toHaveBeenCalledOnce();
|
|
const blob = createMock.mock.calls[0]?.[0] as Blob;
|
|
expect(blob).toBeInstanceOf(Blob);
|
|
expect(blob.type).toBe("application/json");
|
|
|
|
expect(clickedAnchors).toHaveLength(1);
|
|
const anchor = clickedAnchors[0]!;
|
|
expect(anchor.getAttribute("href")).toBe("blob:mock-url");
|
|
expect(anchor.getAttribute("download")).toBe("mcp.json");
|
|
|
|
// After the call, the anchor must be removed and the URL revoked.
|
|
expect(document.body.contains(anchor)).toBe(false);
|
|
// Revoke is deferred so the scheduled download can read the blob first.
|
|
expect(revokeMock).not.toHaveBeenCalled();
|
|
vi.runAllTimers();
|
|
expect(revokeMock).toHaveBeenCalledWith("blob:mock-url");
|
|
});
|
|
|
|
it("still cleans up the anchor + URL when click() throws (try/finally guard)", () => {
|
|
const removed: Node[] = [];
|
|
const origCreateElement = document.createElement.bind(document);
|
|
vi.spyOn(document, "createElement").mockImplementation(((tag: string) => {
|
|
const el = origCreateElement(tag) as HTMLAnchorElement;
|
|
if (tag === "a") {
|
|
el.click = () => {
|
|
throw new Error("simulated click failure");
|
|
};
|
|
}
|
|
return el;
|
|
}) as typeof document.createElement);
|
|
// Stub removeChild outright so the throw doesn't double up with a
|
|
// "not a child" error from a partially-mocked DOM.
|
|
vi.spyOn(document.body, "removeChild").mockImplementation(((node: Node) => {
|
|
removed.push(node);
|
|
return node;
|
|
}) as typeof document.body.removeChild);
|
|
|
|
expect(() => downloadJsonFile("oops.json", '{"oops":true}')).toThrow(
|
|
/simulated click failure/,
|
|
);
|
|
|
|
// Cleanup still ran despite the throw.
|
|
expect(removed).toHaveLength(1);
|
|
vi.runAllTimers();
|
|
expect(revokeMock).toHaveBeenCalledWith("blob:mock-url");
|
|
});
|
|
|
|
it("passes the JSON string into the Blob constructor verbatim", async () => {
|
|
downloadJsonFile("test.json", '{"x":1}');
|
|
const blob = createMock.mock.calls[0]?.[0] as Blob;
|
|
const text = await blob.text();
|
|
expect(text).toBe('{"x":1}');
|
|
});
|
|
|
|
it("downloadBlob passes the given blob through and honours its type", async () => {
|
|
const blob = new Blob(["plain text"], { type: "text/plain" });
|
|
downloadBlob("note.txt", blob);
|
|
expect(createMock).toHaveBeenCalledWith(blob);
|
|
const passed = createMock.mock.calls[0]?.[0] as Blob;
|
|
expect(passed.type).toBe("text/plain");
|
|
expect(await passed.text()).toBe("plain text");
|
|
});
|
|
});
|
|
|
|
describe("buildExportFilename", () => {
|
|
const fixedNow = new Date("2026-03-17T10:00:42.123Z");
|
|
|
|
it("includes kind, server id, and ISO timestamp with `:` swapped for `-`", () => {
|
|
expect(buildExportFilename("protocol", "alpha", fixedNow)).toBe(
|
|
"inspector-protocol-alpha-2026-03-17T10-00-42.123Z.json",
|
|
);
|
|
});
|
|
|
|
it("omits the server-id segment when serverId is undefined", () => {
|
|
expect(buildExportFilename("logs", undefined, fixedNow)).toBe(
|
|
"inspector-logs-2026-03-17T10-00-42.123Z.json",
|
|
);
|
|
});
|
|
|
|
it("omits the server-id segment when serverId is an empty string", () => {
|
|
expect(buildExportFilename("logs", "", fixedNow)).toBe(
|
|
"inspector-logs-2026-03-17T10-00-42.123Z.json",
|
|
);
|
|
});
|
|
|
|
it("encodes server ids that contain filesystem-unsafe characters", () => {
|
|
expect(buildExportFilename("network", "my server/v2", fixedNow)).toBe(
|
|
"inspector-network-my%20server%2Fv2-2026-03-17T10-00-42.123Z.json",
|
|
);
|
|
});
|
|
|
|
it("defaults `now` to the current time when not provided", () => {
|
|
const name = buildExportFilename("protocol", "alpha");
|
|
expect(name).toMatch(
|
|
/^inspector-protocol-alpha-\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}\.\d{3}Z\.json$/,
|
|
);
|
|
});
|
|
});
|
|
|
|
describe("fileNameFromUri", () => {
|
|
it("returns the last path segment", () => {
|
|
expect(fileNameFromUri("https://x/y/z/report.pdf")).toBe("report.pdf");
|
|
});
|
|
it("splits on backslashes as well as forward slashes", () => {
|
|
expect(fileNameFromUri("C:\\Users\\me\\report.pdf")).toBe("report.pdf");
|
|
});
|
|
it("strips control/format chars and disallowed filename chars", () => {
|
|
// U+200B (zero-width space, Cf) and U+202E (RTL override, Cf) are
|
|
// stripped; the disallowed `*?` run collapses to a single `_`.
|
|
const uri = "https://x/a\u200bb\u202ec*?.txt";
|
|
expect(fileNameFromUri(uri)).toBe("abc_.txt");
|
|
});
|
|
it("truncates very long names to 255 characters", () => {
|
|
const long = "a".repeat(300) + ".txt";
|
|
expect(fileNameFromUri(`https://x/${long}`)).toHaveLength(255);
|
|
});
|
|
it("falls back to 'download' when nothing usable remains", () => {
|
|
expect(fileNameFromUri("https://x/")).toBe("download");
|
|
});
|
|
});
|
|
|
|
describe("isHttpUrl", () => {
|
|
it("accepts http and https", () => {
|
|
expect(isHttpUrl("https://example.com")?.href).toBe("https://example.com/");
|
|
expect(isHttpUrl("http://localhost:3000/a")?.href).toBe(
|
|
"http://localhost:3000/a",
|
|
);
|
|
});
|
|
it("rejects other schemes and unparsable input", () => {
|
|
expect(isHttpUrl("javascript:alert(1)")).toBeNull();
|
|
expect(isHttpUrl("data:text/html,<b>")).toBeNull();
|
|
expect(isHttpUrl("file:///etc/passwd")).toBeNull();
|
|
expect(isHttpUrl("not a url")).toBeNull();
|
|
});
|
|
});
|