From b2a6ee1dcf2f2f4e3def4fbd58a5137c6ecf3bc4 Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Mon, 29 Jun 2026 17:49:34 -0400 Subject: [PATCH 1/5] fix(web): block close on incomplete EMA config, not just an invalid issuer (closes #1546) (#1547) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #1545. The close-block only fired for an invalid issuer, so an EMA config enabled with a valid issuer but a blank client ID was still silently dropped on close by the persist gate — the same no-feedback bug, different field. - validateClientSettings gains a requireComplete mode that also flags the required IdP fields (issuer, clientId) when blank, with distinct per-field messages. Inline (as-you-type) validation is unchanged — required-but-blank errors only surface on reveal, so the form never nags about not-yet-filled fields. - canPersistClientSettingsDraft is redefined in terms of that mode, so the persist gate and the close-time field errors share one source of truth. - ClientSettingsModal.handleClose now blocks + reveals on ANY require-complete error (invalid issuer OR blank required field) and keeps the modal open. The form prop revealIssuerError becomes revealErrors and surfaces the client-ID error too. Escape hatch: disable enterprise IdP (an empty issuer no longer closes, since that's itself incomplete). Tests cover the blank-clientId close path, fix-each-field, and disable-EMA escape, plus requireComplete validation. Co-authored-by: Claude Opus 4.8 (1M context) --- .../ClientSettingsForm.test.tsx | 54 +++++- .../ClientSettingsForm/ClientSettingsForm.tsx | 50 +++--- .../clientSettingsValues.test.ts | 91 ++++++++++ .../clientSettingsValues.ts | 87 +++++++--- .../ClientSettingsModal.test.tsx | 160 ++++++++++++++++-- .../ClientSettingsModal.tsx | 30 ++-- 6 files changed, 383 insertions(+), 89 deletions(-) diff --git a/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx b/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx index 4ea9c47c..7f3cbec2 100644 --- a/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx +++ b/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.test.tsx @@ -10,6 +10,8 @@ import { import { EMPTY_CLIENT_SETTINGS, ISSUER_URL_ERROR, + ISSUER_REQUIRED_ERROR, + CLIENT_ID_REQUIRED_ERROR, type ClientSettingsFormValues, } from "./clientSettingsValues"; @@ -139,7 +141,7 @@ describe("ClientSettingsForm EMA IdP session", () => { expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument(); }); - it("reveals the issuer error without blur when revealIssuerError is set", () => { + it("reveals the issuer error without blur when revealErrors is set", () => { renderWithMantine( { onExpandedSectionsChange={vi.fn()} onSettingsChange={vi.fn()} emaIdpLoginState="none" - revealIssuerError + revealErrors />, ); @@ -160,23 +162,63 @@ describe("ClientSettingsForm EMA IdP session", () => { expect(screen.getByText(ISSUER_URL_ERROR)).toBeInTheDocument(); }); - it("shows no error for a valid issuer even when revealIssuerError is set", () => { + it("reveals required errors for blank issuer and client ID when revealErrors is set", () => { + renderWithMantine( + , + ); + + // Both required IdP fields are blank; a close/save attempt surfaces them + // instead of silently dropping the config. + expect(screen.getByText(ISSUER_REQUIRED_ERROR)).toBeInTheDocument(); + expect(screen.getByText(CLIENT_ID_REQUIRED_ERROR)).toBeInTheDocument(); + }); + + it("does not show required errors for blank fields before revealErrors", () => { + renderWithMantine( + , + ); + + // Inline validation must not nag about not-yet-filled required fields. + expect(screen.queryByText(ISSUER_REQUIRED_ERROR)).not.toBeInTheDocument(); + expect( + screen.queryByText(CLIENT_ID_REQUIRED_ERROR), + ).not.toBeInTheDocument(); + }); + + it("shows no error for a valid issuer even when revealErrors is set", () => { renderWithMantine( , ); expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument(); + expect( + screen.queryByText(CLIENT_ID_REQUIRED_ERROR), + ).not.toBeInTheDocument(); }); it("shows no issuer error for a valid URL", () => { @@ -251,7 +293,7 @@ describe("ClientSettingsForm EMA IdP session", () => { ).not.toBeInTheDocument(); }); - it("reveals the CIMD URL error without blur when revealClientMetadataUrlError is set", () => { + it("reveals the CIMD URL error without blur when revealErrors is set", () => { renderWithMantine( { onExpandedSectionsChange={vi.fn()} onSettingsChange={vi.fn()} emaIdpLoginState="none" - revealClientMetadataUrlError + revealErrors />, ); diff --git a/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.tsx b/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.tsx index 209564d7..cf89e2a8 100644 --- a/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.tsx +++ b/clients/web/src/components/groups/ClientSettingsForm/ClientSettingsForm.tsx @@ -30,16 +30,12 @@ export interface ClientSettingsFormProps { emaIdpLoginState?: EmaIdpLoginState; onEmaIdpLogout?: () => void; /** - * Force the issuer error to show even before the field is blurred. The parent - * sets this when a save/close is attempted with an invalid issuer, so the user - * isn't left with a silently-dropped value and no explanation. + * Force all field errors to show, including required-but-blank fields (EMA's + * issuer/clientId, CIMD's metadata URL). The parent sets this when a + * save/close is attempted with an incomplete or invalid config, so nothing is + * silently dropped without explanation. */ - revealIssuerError?: boolean; - /** - * Force the CIMD metadata URL error to show even before the field is blurred. - * Same contract as {@link revealIssuerError} for the CIMD URL field. - */ - revealClientMetadataUrlError?: boolean; + revealErrors?: boolean; } const HintText = Text.withProps({ @@ -54,8 +50,7 @@ export function ClientSettingsForm({ onSettingsChange, emaIdpLoginState = "none", onEmaIdpLogout, - revealIssuerError = false, - revealClientMetadataUrlError = false, + revealErrors = false, }: ClientSettingsFormProps) { function patch(partial: Partial) { onSettingsChange((prev) => ({ ...prev, ...partial })); @@ -64,23 +59,31 @@ export function ClientSettingsForm({ // Defer the issuer error until the field has been blurred so it doesn't nag // mid-typing (e.g. while "https:/…" is still incomplete). Once touched it // updates live, so the error clears as soon as a valid URL is entered. The - // parent also forces it on via `revealIssuerError` when a close/save is - // attempted with an invalid value, so the value is never silently dropped - // without explanation. The persist gate (canPersistClientSettingsDraft) - // validates independently — an invalid issuer is never written regardless. + // parent forces all errors on via `revealErrors` when a close/save is + // attempted with an incomplete or invalid config — including the + // required-but-blank fields, which inline validation otherwise leaves silent. + // The persist gate (canPersistClientSettingsDraft) validates independently, so + // an incomplete/invalid config is never written regardless of these flags. const [issuerTouched, setIssuerTouched] = useState(false); const [clientMetadataUrlTouched, setClientMetadataUrlTouched] = useState(false); - const errors = validateClientSettings(settings); - const showIssuerError = - (issuerTouched || revealIssuerError) && errors.issuer - ? errors.issuer + // Inline errors flag only a filled-in-wrong field (issuer URL); the + // require-complete set adds the blank-required fields surfaced on reveal. + const inlineErrors = validateClientSettings(settings); + const revealedErrors = validateClientSettings(settings, { + requireComplete: true, + }); + const showIssuerError = revealErrors + ? revealedErrors.issuer + : issuerTouched + ? inlineErrors.issuer : undefined; - const showClientMetadataUrlError = - (clientMetadataUrlTouched || revealClientMetadataUrlError) && - errors.clientMetadataUrl - ? errors.clientMetadataUrl + const showClientIdError = revealErrors ? revealedErrors.clientId : undefined; + const showClientMetadataUrlError = revealErrors + ? revealedErrors.clientMetadataUrl + : clientMetadataUrlTouched + ? inlineErrors.clientMetadataUrl : undefined; const showIdpSession = @@ -134,6 +137,7 @@ export function ClientSettingsForm({ description="Client id registered with your enterprise IdP." value={settings.clientId} onChange={(e) => patch({ clientId: e.currentTarget.value })} + error={showClientIdError} rightSectionPointerEvents="auto" rightSection={ settings.clientId ? ( diff --git a/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.test.ts b/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.test.ts index ff4c8a9a..bd240bb0 100644 --- a/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.test.ts +++ b/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.test.ts @@ -10,6 +10,9 @@ import { EMPTY_CLIENT_SETTINGS, formValuesToClientConfig, ISSUER_URL_ERROR, + ISSUER_REQUIRED_ERROR, + CLIENT_ID_REQUIRED_ERROR, + CLIENT_METADATA_URL_REQUIRED_ERROR, validateClientSettings, } from "./clientSettingsValues"; @@ -457,4 +460,92 @@ describe("clientSettingsValues", () => { }), ).toEqual({}); }); + + it("validateClientSettings does not flag blank required fields by default", () => { + expect( + validateClientSettings({ + emaEnabled: true, + issuer: "", + clientId: "", + clientSecret: "", + ...emptyCimd, + }), + ).toEqual({}); + }); + + it("validateClientSettings with requireComplete flags blank issuer and clientId", () => { + expect( + validateClientSettings( + { + emaEnabled: true, + issuer: "", + clientId: "", + clientSecret: "", + ...emptyCimd, + }, + { requireComplete: true }, + ), + ).toEqual({ + issuer: ISSUER_REQUIRED_ERROR, + clientId: CLIENT_ID_REQUIRED_ERROR, + }); + }); + + it("validateClientSettings with requireComplete keeps the URL error over the required error", () => { + expect( + validateClientSettings( + { + emaEnabled: true, + issuer: "not-a-url", + clientId: "cid", + clientSecret: "", + ...emptyCimd, + }, + { requireComplete: true }, + ), + ).toEqual({ issuer: ISSUER_URL_ERROR }); + }); + + it("validateClientSettings with requireComplete flags only the blank clientId when the issuer is valid", () => { + expect( + validateClientSettings( + { + emaEnabled: true, + issuer: "https://idp.test", + clientId: "", + clientSecret: "", + ...emptyCimd, + }, + { requireComplete: true }, + ), + ).toEqual({ clientId: CLIENT_ID_REQUIRED_ERROR }); + }); + + it("validateClientSettings with requireComplete flags a blank CIMD URL", () => { + expect( + validateClientSettings( + { + emaEnabled: false, + issuer: "", + clientId: "", + clientSecret: "", + cimdEnabled: true, + clientMetadataUrl: "", + }, + { requireComplete: true }, + ), + ).toEqual({ clientMetadataUrl: CLIENT_METADATA_URL_REQUIRED_ERROR }); + }); + + it("canPersistClientSettingsDraft blocks a blank clientId with a valid issuer", () => { + expect( + canPersistClientSettingsDraft({ + emaEnabled: true, + issuer: "https://idp.test", + clientId: "", + clientSecret: "", + ...emptyCimd, + }), + ).toBe(false); + }); }); diff --git a/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.ts b/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.ts index ff468479..5e592368 100644 --- a/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.ts +++ b/clients/web/src/components/groups/ClientSettingsForm/clientSettingsValues.ts @@ -7,35 +7,72 @@ import { /** Field-level error message for an issuer that is not an http(s) URL. */ export const ISSUER_URL_ERROR = "Must be an http(s) URL, like https://idp.example.com"; +/** Field-level errors for a required field left blank while its feature is enabled. */ +export const ISSUER_REQUIRED_ERROR = + "Issuer is required when enterprise IdP configuration is enabled"; +export const CLIENT_ID_REQUIRED_ERROR = + "Client ID is required when enterprise IdP configuration is enabled"; +export const CLIENT_METADATA_URL_REQUIRED_ERROR = + "Metadata document URL is required when CIMD is enabled"; /** Field-level validation errors for the client settings form. */ export interface ClientSettingsErrors { issuer?: string; + clientId?: string; clientMetadataUrl?: string; } +export interface ValidateClientSettingsOptions { + /** + * Also flag required fields left blank. Off by default so inline (as-you-type) + * validation only flags a field the user has actually filled in wrong; turned + * on when a save/close is attempted so an incomplete config is surfaced rather + * than silently dropped. + */ + requireComplete?: boolean; +} + /** - * Inline validation for the client settings form. Only flags fields that the - * user has actually filled in — empty required fields are gated by - * {@link canPersistClientSettingsDraft} rather than surfaced as errors. + * Validation for the client settings form. By default only flags fields the + * user has filled in wrong (a non-empty, non-http(s) issuer; a malformed CIMD + * metadata URL). With `requireComplete`, also flags the required fields of an + * enabled feature (EMA's issuer/clientId, CIMD's metadata URL) when they are + * blank — used at save/close time so an enabled-but-incomplete config never + * persists silently. */ export function validateClientSettings( values: ClientSettingsFormValues, + options: ValidateClientSettingsOptions = {}, ): ClientSettingsErrors { const errors: ClientSettingsErrors = {}; - if ( - values.emaEnabled && - values.issuer.trim() !== "" && - !isAbsoluteHttpUrl(values.issuer) - ) { - errors.issuer = ISSUER_URL_ERROR; - } - if (values.cimdEnabled && values.clientMetadataUrl.trim() !== "") { - const cimdError = getCimdClientMetadataUrlError(values.clientMetadataUrl); - if (cimdError) { - errors.clientMetadataUrl = cimdError; + + if (values.emaEnabled) { + const issuer = values.issuer.trim(); + if (issuer === "") { + if (options.requireComplete) errors.issuer = ISSUER_REQUIRED_ERROR; + } else if (!isAbsoluteHttpUrl(values.issuer)) { + errors.issuer = ISSUER_URL_ERROR; + } + + if (options.requireComplete && values.clientId.trim() === "") { + errors.clientId = CLIENT_ID_REQUIRED_ERROR; } } + + if (values.cimdEnabled) { + const url = values.clientMetadataUrl.trim(); + if (url === "") { + if (options.requireComplete) { + errors.clientMetadataUrl = CLIENT_METADATA_URL_REQUIRED_ERROR; + } + } else { + const cimdError = getCimdClientMetadataUrlError(values.clientMetadataUrl); + if (cimdError) { + errors.clientMetadataUrl = cimdError; + } + } + } + return errors; } @@ -108,18 +145,18 @@ export function formValuesToClientConfig( return result; } -/** Skip debounced persist while required fields are blank for enabled features. */ +/** + * Skip the debounced persist while an enabled feature's required fields are + * blank or invalid (EMA's issuer/clientId, CIMD's metadata URL). Defers to + * {@link validateClientSettings} in `requireComplete` mode so the persist gate + * and the close-time field errors can never drift: the same condition that + * blocks the write also surfaces the field-level errors that explain why. + */ export function canPersistClientSettingsDraft( values: ClientSettingsFormValues, ): boolean { - if (values.emaEnabled) { - if (values.issuer.trim() === "" || values.clientId.trim() === "") - return false; - } - if (values.cimdEnabled) { - if (!values.clientMetadataUrl.trim()) return false; - } - // Defer to validateClientSettings so the persist gate and inline field errors - // can never drift — invalid values are never sent to the backend. - return Object.keys(validateClientSettings(values)).length === 0; + return ( + Object.keys(validateClientSettings(values, { requireComplete: true })) + .length === 0 + ); } diff --git a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx index 851efb18..5b2a2740 100644 --- a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx +++ b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.test.tsx @@ -6,6 +6,8 @@ import { ClientSettingsModal } from "./ClientSettingsModal"; import { EMPTY_CLIENT_SETTINGS, ISSUER_URL_ERROR, + CLIENT_ID_REQUIRED_ERROR, + CLIENT_METADATA_URL_REQUIRED_ERROR, type ClientSettingsFormValues, } from "../ClientSettingsForm/clientSettingsValues"; import { CIMD_METADATA_URL_INVALID_ERROR } from "@inspector/core/client/config-parse.js"; @@ -150,6 +152,9 @@ describe("ClientSettingsModal", () => { ).toBeInTheDocument(); }); + const clickClose = (user: ReturnType) => + user.click(document.querySelector("button.mantine-CloseButton-root")!); + it("blocks close and reveals the issuer error when the issuer is invalid", async () => { const user = userEvent.setup(); const onClose = vi.fn(); @@ -160,6 +165,7 @@ describe("ClientSettingsModal", () => { ...EMPTY_CLIENT_SETTINGS, emaEnabled: true, issuer: "not-a-url", + clientId: "client-1", }} onClose={onClose} onSettingsChange={vi.fn()} @@ -169,13 +175,35 @@ describe("ClientSettingsModal", () => { // Error is hidden initially (form gates it on blur)... expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument(); // ...but a close attempt reveals it and does NOT close (no silent drop). - await user.click( - document.querySelector("button.mantine-CloseButton-root")!, - ); + await clickClose(user); expect(onClose).not.toHaveBeenCalled(); expect(screen.getByText(ISSUER_URL_ERROR)).toBeInTheDocument(); }); + it("blocks close and reveals the required error when the client ID is blank", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + renderWithMantine( + , + ); + + expect( + screen.queryByText(CLIENT_ID_REQUIRED_ERROR), + ).not.toBeInTheDocument(); + await clickClose(user); + expect(onClose).not.toHaveBeenCalled(); + expect(screen.getByText(CLIENT_ID_REQUIRED_ERROR)).toBeInTheDocument(); + }); + it("allows close once the invalid issuer is corrected", async () => { const user = userEvent.setup(); const onClose = vi.fn(); @@ -184,6 +212,7 @@ describe("ClientSettingsModal", () => { ...EMPTY_CLIENT_SETTINGS, emaEnabled: true, issuer: "not-a-url", + clientId: "client-1", // complete except for the issuer }); return ( { } renderWithMantine(); - const close = () => - user.click(document.querySelector("button.mantine-CloseButton-root")!); - - await close(); // blocked, error revealed + await clickClose(user); // blocked, error revealed expect(onClose).not.toHaveBeenCalled(); const issuer = screen.getByLabelText("Issuer"); @@ -207,18 +233,18 @@ describe("ClientSettingsModal", () => { await user.type(issuer, "https://idp.test"); expect(screen.queryByText(ISSUER_URL_ERROR)).not.toBeInTheDocument(); - await close(); // now valid -> closes + await clickClose(user); // now complete + valid -> closes expect(onClose).toHaveBeenCalledTimes(1); }); - it("allows close when an invalid issuer is cleared (empty is valid to leave)", async () => { + it("allows close once a blank client ID is filled", async () => { const user = userEvent.setup(); const onClose = vi.fn(); function Host() { const [settings, setSettings] = useState({ ...EMPTY_CLIENT_SETTINGS, emaEnabled: true, - issuer: "not-a-url", + issuer: "https://idp.test", // complete except for the clientId }); return ( { } renderWithMantine(); - await user.clear(screen.getByLabelText("Issuer")); + await clickClose(user); // blocked on blank clientId + expect(onClose).not.toHaveBeenCalled(); + + await user.type(screen.getByLabelText("Client ID"), "client-1"); + await clickClose(user); // now complete -> closes + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("allows close by disabling enterprise IdP when the config is incomplete", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + function Host() { + const [settings, setSettings] = useState({ + ...EMPTY_CLIENT_SETTINGS, + emaEnabled: true, // enabled but issuer + clientId blank + }); + return ( + + ); + } + renderWithMantine(); + + await clickClose(user); // blocked: required fields blank + expect(onClose).not.toHaveBeenCalled(); + + // Disabling enterprise IdP is the escape hatch — nothing required to save. await user.click( - document.querySelector("button.mantine-CloseButton-root")!, + screen.getByRole("checkbox", { + name: "Enable enterprise IdP configuration", + }), ); + await clickClose(user); expect(onClose).toHaveBeenCalledTimes(1); }); @@ -304,14 +363,40 @@ describe("ClientSettingsModal", () => { expect(onClose).toHaveBeenCalledTimes(1); }); - it("allows close when an invalid CIMD URL is cleared (empty is valid to leave)", async () => { + it("blocks close and reveals the required error when the CIMD URL is blank", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + renderWithMantine( + , + ); + + expect( + screen.queryByText(CLIENT_METADATA_URL_REQUIRED_ERROR), + ).not.toBeInTheDocument(); + await user.click( + document.querySelector("button.mantine-CloseButton-root")!, + ); + expect(onClose).not.toHaveBeenCalled(); + expect( + screen.getByText(CLIENT_METADATA_URL_REQUIRED_ERROR), + ).toBeInTheDocument(); + }); + + it("allows close once a blank CIMD URL is filled", async () => { const user = userEvent.setup(); const onClose = vi.fn(); function Host() { const [settings, setSettings] = useState({ ...EMPTY_CLIENT_SETTINGS, - cimdEnabled: true, - clientMetadataUrl: "not-a-url", + cimdEnabled: true, // enabled but metadata URL blank }); return ( { } renderWithMantine(); + await user.click( + document.querySelector("button.mantine-CloseButton-root")!, + ); // blocked on blank URL + expect(onClose).not.toHaveBeenCalled(); + await expandCimdSection(user); - await user.clear(screen.getByLabelText("Client ID metadata document URL")); + await user.type( + screen.getByLabelText("Client ID metadata document URL"), + "https://example.com/cimd.json", + ); + await user.click( + document.querySelector("button.mantine-CloseButton-root")!, + ); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it("allows close by disabling CIMD when the URL is blank", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + function Host() { + const [settings, setSettings] = useState({ + ...EMPTY_CLIENT_SETTINGS, + cimdEnabled: true, // enabled but metadata URL blank + }); + return ( + + ); + } + renderWithMantine(); + + await user.click( + document.querySelector("button.mantine-CloseButton-root")!, + ); // blocked: URL required + expect(onClose).not.toHaveBeenCalled(); + + // Disabling CIMD is the escape hatch — nothing required to save. + await expandCimdSection(user); + await user.click( + screen.getByRole("checkbox", { name: "Use Client ID Metadata Document" }), + ); await user.click( document.querySelector("button.mantine-CloseButton-root")!, ); diff --git a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.tsx b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.tsx index 3b454b29..adf956bd 100644 --- a/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.tsx +++ b/clients/web/src/components/groups/ClientSettingsModal/ClientSettingsModal.tsx @@ -37,9 +37,7 @@ export function ClientSettingsModal({ const [expandedSections, setExpandedSections] = useState< ClientSettingsSection[] >(["ema"]); - const [revealIssuerError, setRevealIssuerError] = useState(false); - const [revealClientMetadataUrlError, setRevealClientMetadataUrlError] = - useState(false); + const [revealErrors, setRevealErrors] = useState(false); const allExpanded = expandedSections.length === ALL_SECTIONS.length; @@ -47,21 +45,16 @@ export function ClientSettingsModal({ setExpandedSections(allExpanded ? [] : ALL_SECTIONS); } - // Closing (X / Esc / overlay) is the implicit "save" for this auto-saving - // modal — the parent flushes the debounced persist on close. If a validated - // URL field is invalid the persist gate would silently drop it, so instead of - // closing we reveal the field error (overriding the form's on-blur gating). - // The user can fix the URL or clear the field — an empty value is valid to - // leave — and then close. Resets via the parent's open/close remount key. + // modal — the parent flushes the debounced persist on close. A client config + // that's incomplete (blank required field) or invalid (bad issuer / CIMD URL) + // would be silently dropped by the persist gate, so instead of closing we + // reveal the field errors (overriding the form's on-blur gating) and keep the + // modal open. The user fixes the fields, or disables the feature, then closes. + // Resets via the parent's open/close remount key. function handleClose() { - const errors = validateClientSettings(settings); - if (errors.issuer) { - setRevealIssuerError(true); - } - if (errors.clientMetadataUrl) { - setRevealClientMetadataUrlError(true); - } - if (errors.issuer || errors.clientMetadataUrl) { + const errors = validateClientSettings(settings, { requireComplete: true }); + if (Object.keys(errors).length > 0) { + setRevealErrors(true); return; } onClose(); @@ -94,8 +87,7 @@ export function ClientSettingsModal({ onSettingsChange={onSettingsChange} emaIdpLoginState={emaIdpLoginState} onEmaIdpLogout={onEmaIdpLogout} - revealIssuerError={revealIssuerError} - revealClientMetadataUrlError={revealClientMetadataUrlError} + revealErrors={revealErrors} /> From baf79b843f623ca115ae5c2e3b482611cd97cc83 Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Tue, 30 Jun 2026 09:18:43 -0400 Subject: [PATCH 2/5] feat(web): expand ContentViewer to render PDF, CSV, HTML, XML, JSON, CSS (closes #1329) (#1551) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(web): expand ContentViewer to render PDF, CSV, HTML, XML, JSON, CSS (closes #1329) Turn ContentViewer into a per-MIME dispatch surface for resource content. It now accepts raw TextResourceContents | BlobResourceContents (in addition to the existing ContentBlock path), so ResourcePreviewPanel.toContentBlock collapses to a passthrough and blob-typed resources render natively instead of falling back to opaque "[Binary content …]" text. New renderers (clients/web/src/components/elements/): - CodeHighlight: lazy react-syntax-highlighter prism-light element. Runtime, tomorrow theme, and each grammar dynamic-import on first use (one chunk per grammar). Module-level registeredLanguages / failedLoads / loadingPromises caches; plain Code block until a grammar resolves. - PdfFrame: iframe over a Blob URL (#view=FitH), revoked on unmount. - CsvTable: papaparse -> Mantine Table (first 100 rows), plain-Code fallback. - HtmlFrame: sandbox="" iframe over a Blob URL with an injected CSP meta (no script-src, falls through to default-src 'none'). - contentViewerUtils: getMimeKind, base64 decode helpers, formatXml, wrapHtmlWithCsp, SAFE_HREF markdown-anchor allowlist (unsafe href -> span). ResourcePreviewPanel: extend the URI-suffix MIME inference table (.csv/.json/.xml/.html/.htm/.css/.pdf alongside .md). Deps: papaparse + react-syntax-highlighter (+ @types). tsconfig paths stubs redirect the deep RSH imports for typecheck (the @types ambient declarations are shadowed by the real .js under bundler resolution) — same pattern the repo uses for react/pino. Tests cover PDF blob-URL creation + cleanup, CSV table render + fallback, HTML sandbox attributes + CSP injection, JSON/XML/CSS highlighted output, the lazy-load/alias/failed-load/shared-runtime paths, and the URI-suffix MIME-inference paths. validate, test:coverage (per-file gate), test:storybook, and integration all pass. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(web): harden ContentViewer blob/CSV rendering per review (#1551) Address review feedback on the ContentViewer MIME-dispatch PR: - useObjectUrl: fix a dev-only StrictMode bug where the committed object URL was revoked but never recreated (setup→cleanup→setup with no re-render), blanking PDF/HTML previews. Revocation is now deferred to a microtask and guarded by a "live URL" ref so a StrictMode re-setup cancels it, while a real unmount / blob change still releases the URL. Avoids set-state-in-effect; mirrors AppRenderer's deferred-disposal. - atob: guard base64 decoding with tryDecodeBase64ToUtf8 / tryDecodeBase64ToBytes so a malformed blob from an external server degrades to the binary-content notice instead of throwing during render and taking down the whole preview panel. - CsvTable: surface the 100-row display cap with a "Showing first N of M rows" caption (no silent truncation). - Extract BinaryNotice into its own module so PdfFrame can reuse it without an import cycle. Tests added for the StrictMode path, malformed-base64 fallbacks (PDF + textual blob), the safe-decode helpers, and the truncation caption. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- clients/web/package-lock.json | 161 +++++++ clients/web/package.json | 4 + .../CodeHighlight/CodeHighlight.stories.tsx | 42 ++ .../CodeHighlight/CodeHighlight.test.tsx | 172 ++++++++ .../elements/CodeHighlight/CodeHighlight.tsx | 168 ++++++++ .../elements/ContentViewer/BinaryNotice.tsx | 24 ++ .../ContentViewer/ContentViewer.stories.tsx | 65 +++ .../ContentViewer/ContentViewer.test.tsx | 265 +++++++++++- .../elements/ContentViewer/ContentViewer.tsx | 398 ++++++++++++++---- .../elements/ContentViewer/CsvTable.test.tsx | 56 +++ .../elements/ContentViewer/CsvTable.tsx | 76 ++++ .../elements/ContentViewer/HtmlFrame.test.tsx | 47 +++ .../elements/ContentViewer/HtmlFrame.tsx | 39 ++ .../elements/ContentViewer/PdfFrame.test.tsx | 41 ++ .../elements/ContentViewer/PdfFrame.tsx | 43 ++ .../ContentViewer/contentViewerUtils.test.ts | 189 +++++++++ .../ContentViewer/contentViewerUtils.ts | 219 ++++++++++ .../ContentViewer/useObjectUrl.test.tsx | 62 +++ .../elements/ContentViewer/useObjectUrl.ts | 40 ++ .../ResourcePreviewPanel.test.tsx | 110 ++++- .../ResourcePreviewPanel.tsx | 49 +-- .../react-syntax-highlighter-grammar.d.ts | 5 + .../react-syntax-highlighter-prism-light.d.ts | 8 + .../types/react-syntax-highlighter-style.d.ts | 4 + clients/web/tsconfig.app.json | 11 +- clients/web/tsconfig.test.json | 11 +- 26 files changed, 2196 insertions(+), 113 deletions(-) create mode 100644 clients/web/src/components/elements/CodeHighlight/CodeHighlight.stories.tsx create mode 100644 clients/web/src/components/elements/CodeHighlight/CodeHighlight.test.tsx create mode 100644 clients/web/src/components/elements/CodeHighlight/CodeHighlight.tsx create mode 100644 clients/web/src/components/elements/ContentViewer/BinaryNotice.tsx create mode 100644 clients/web/src/components/elements/ContentViewer/CsvTable.test.tsx create mode 100644 clients/web/src/components/elements/ContentViewer/CsvTable.tsx create mode 100644 clients/web/src/components/elements/ContentViewer/HtmlFrame.test.tsx create mode 100644 clients/web/src/components/elements/ContentViewer/HtmlFrame.tsx create mode 100644 clients/web/src/components/elements/ContentViewer/PdfFrame.test.tsx create mode 100644 clients/web/src/components/elements/ContentViewer/PdfFrame.tsx create mode 100644 clients/web/src/components/elements/ContentViewer/contentViewerUtils.test.ts create mode 100644 clients/web/src/components/elements/ContentViewer/contentViewerUtils.ts create mode 100644 clients/web/src/components/elements/ContentViewer/useObjectUrl.test.tsx create mode 100644 clients/web/src/components/elements/ContentViewer/useObjectUrl.ts create mode 100644 clients/web/src/types/react-syntax-highlighter-grammar.d.ts create mode 100644 clients/web/src/types/react-syntax-highlighter-prism-light.d.ts create mode 100644 clients/web/src/types/react-syntax-highlighter-style.d.ts diff --git a/clients/web/package-lock.json b/clients/web/package-lock.json index a14dc7d5..8553cda8 100644 --- a/clients/web/package-lock.json +++ b/clients/web/package-lock.json @@ -26,11 +26,13 @@ "commander": "^13.1.0", "hono": "^4.12.18", "open": "^10.2.0", + "papaparse": "^5.5.4", "pino": "^9.14.0", "react": "^19.2.4", "react-dom": "^19.2.4", "react-icons": "^5.6.0", "react-markdown": "^10.1.0", + "react-syntax-highlighter": "^16.1.1", "remark-gfm": "^4.0.1", "zod": "^4.3.6", "zustand": "^5.0.13" @@ -51,8 +53,10 @@ "@testing-library/user-event": "^14.6.1", "@types/express": "^5.0.6", "@types/node": "^24.12.4", + "@types/papaparse": "^5.5.2", "@types/react": "^19.2.14", "@types/react-dom": "^19.2.3", + "@types/react-syntax-highlighter": "^15.5.13", "@vitejs/plugin-react": "^6.0.0", "@vitest/browser-playwright": "^4.1.0", "@vitest/coverage-v8": "^4.1.0", @@ -2979,12 +2983,28 @@ "undici-types": "~7.16.0" } }, + "node_modules/@types/papaparse": { + "version": "5.5.2", + "resolved": "https://registry.npmjs.org/@types/papaparse/-/papaparse-5.5.2.tgz", + "integrity": "sha512-gFnFp/JMzLHCwRf7tQHrNnfhN4eYBVYYI897CGX4MY1tzY9l2aLkVyx2IlKZ/SAqDbB3I1AOZW5gTMGGsqWliA==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/node": "*" + } + }, "node_modules/@types/parse-json": { "version": "4.0.2", "resolved": "https://registry.npmjs.org/@types/parse-json/-/parse-json-4.0.2.tgz", "integrity": "sha512-dISoDXWWQwUquiKsyZ4Ng+HX2KsPL7LyHKHQwgGFEA3IaKac4Obd+h2a/a6waisAoepJlBcx9paWqjA8/HVjCw==", "license": "MIT" }, + "node_modules/@types/prismjs": { + "version": "1.26.6", + "resolved": "https://registry.npmjs.org/@types/prismjs/-/prismjs-1.26.6.tgz", + "integrity": "sha512-vqlvI7qlMvcCBbVe0AKAb4f97//Hy0EBTaiW8AalRnG/xAN5zOiWWyrNqNXeq8+KAuvRewjCVY1+IPxk4RdNYw==", + "license": "MIT" + }, "node_modules/@types/qs": { "version": "6.15.1", "resolved": "https://registry.npmjs.org/@types/qs/-/qs-6.15.1.tgz", @@ -3018,6 +3038,16 @@ "@types/react": "^19.2.0" } }, + "node_modules/@types/react-syntax-highlighter": { + "version": "15.5.13", + "resolved": "https://registry.npmjs.org/@types/react-syntax-highlighter/-/react-syntax-highlighter-15.5.13.tgz", + "integrity": "sha512-uLGJ87j6Sz8UaBAooU0T6lWJ0dBmjZgN1PZTrj05TNql2/XpC6+4HhMT5syIdFUUt+FASfCeLLv4kBygNU+8qA==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/react": "*" + } + }, "node_modules/@types/resolve": { "version": "1.20.6", "resolved": "https://registry.npmjs.org/@types/resolve/-/resolve-1.20.6.tgz", @@ -5128,6 +5158,19 @@ ], "license": "BSD-3-Clause" }, + "node_modules/fault": { + "version": "1.0.4", + "resolved": "https://registry.npmjs.org/fault/-/fault-1.0.4.tgz", + "integrity": "sha512-CJ0HCB5tL5fYTEA7ToAq5+kTwd++Borf1/bifxd9iT70QcXr4MRrO3Llf8Ifs70q+SJcGHFtnIE/Nw6giCtECA==", + "license": "MIT", + "dependencies": { + "format": "^0.2.0" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/wooorm" + } + }, "node_modules/fdir": { "version": "6.5.0", "resolved": "https://registry.npmjs.org/fdir/-/fdir-6.5.0.tgz", @@ -5246,6 +5289,14 @@ "dev": true, "license": "ISC" }, + "node_modules/format": { + "version": "0.2.2", + "resolved": "https://registry.npmjs.org/format/-/format-0.2.2.tgz", + "integrity": "sha512-wzsgA6WOq+09wrU1tsJ09udeR/YZRaeArL9e1wPbFg3GG2yDnC2ldKpxs4xunpFF9DgqCqOIra3bc1HWrJ37Ww==", + "engines": { + "node": ">=0.4.x" + } + }, "node_modules/forwarded": { "version": "0.2.0", "resolved": "https://registry.npmjs.org/forwarded/-/forwarded-0.2.0.tgz", @@ -5499,6 +5550,19 @@ "node": ">= 0.4" } }, + "node_modules/hast-util-parse-selector": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/hast-util-parse-selector/-/hast-util-parse-selector-4.0.0.tgz", + "integrity": "sha512-wkQCkSYoOGCRKERFWcxMVMOcYE2K1AaNLU8DXS9arxnLOUEWbOXKXiJUNzEpqZ3JOKpnha3jkFrumEjVliDe7A==", + "license": "MIT", + "dependencies": { + "@types/hast": "^3.0.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/unified" + } + }, "node_modules/hast-util-to-jsx-runtime": { "version": "2.3.6", "resolved": "https://registry.npmjs.org/hast-util-to-jsx-runtime/-/hast-util-to-jsx-runtime-2.3.6.tgz", @@ -5539,6 +5603,23 @@ "url": "https://opencollective.com/unified" } }, + "node_modules/hastscript": { + "version": "9.0.1", + "resolved": "https://registry.npmjs.org/hastscript/-/hastscript-9.0.1.tgz", + "integrity": "sha512-g7df9rMFX/SPi34tyGCyUBREQoKkapwdY/T04Qn9TDWfHhAYt4/I0gMVirzK5wEzeUqIjEB+LXC/ypb7Aqno5w==", + "license": "MIT", + "dependencies": { + "@types/hast": "^3.0.0", + "comma-separated-tokens": "^2.0.0", + "hast-util-parse-selector": "^4.0.0", + "property-information": "^7.0.0", + "space-separated-tokens": "^2.0.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/unified" + } + }, "node_modules/hermes-estree": { "version": "0.25.1", "resolved": "https://registry.npmjs.org/hermes-estree/-/hermes-estree-0.25.1.tgz", @@ -5556,6 +5637,21 @@ "hermes-estree": "0.25.1" } }, + "node_modules/highlight.js": { + "version": "10.7.3", + "resolved": "https://registry.npmjs.org/highlight.js/-/highlight.js-10.7.3.tgz", + "integrity": "sha512-tzcUFauisWKNHaRkN4Wjl/ZA07gENAjFl3J/c480dprkGTg5EQstgaNFqBfUqCq54kZRIEcreTsAgF/m2quD7A==", + "license": "BSD-3-Clause", + "engines": { + "node": "*" + } + }, + "node_modules/highlightjs-vue": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/highlightjs-vue/-/highlightjs-vue-1.0.0.tgz", + "integrity": "sha512-PDEfEF102G23vHmPhLyPboFCD+BkMGu+GuJe2d9/eH4FsCwvgBpnc9n0pGE+ffKdph38s6foEZiEjdgHdzp+IA==", + "license": "CC0-1.0" + }, "node_modules/hoist-non-react-statics": { "version": "3.3.2", "resolved": "https://registry.npmjs.org/hoist-non-react-statics/-/hoist-non-react-statics-3.3.2.tgz", @@ -6385,6 +6481,20 @@ "dev": true, "license": "MIT" }, + "node_modules/lowlight": { + "version": "1.20.0", + "resolved": "https://registry.npmjs.org/lowlight/-/lowlight-1.20.0.tgz", + "integrity": "sha512-8Ktj+prEb1RoCPkEOrPMYUN/nCggB7qAWe3a7OpMjWQkh3l2RD5wKRQ+o8Q8YuI9RG/xs95waaI/E6ym/7NsTw==", + "license": "MIT", + "dependencies": { + "fault": "^1.0.0", + "highlight.js": "~10.7.0" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/wooorm" + } + }, "node_modules/lru-cache": { "version": "5.1.1", "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-5.1.1.tgz", @@ -7623,6 +7733,12 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/papaparse": { + "version": "5.5.4", + "resolved": "https://registry.npmjs.org/papaparse/-/papaparse-5.5.4.tgz", + "integrity": "sha512-SwzWD9gl/ElwYLCI0nUja1mFJzjq2D8ziShfNBa7zCHzkOozeOGDwHWQ+tvCzEZcewecWZ5U7kUopDnG+DFYEQ==", + "license": "MIT" + }, "node_modules/parent-module": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/parent-module/-/parent-module-1.0.1.tgz", @@ -8055,6 +8171,15 @@ "license": "MIT", "peer": true }, + "node_modules/prismjs": { + "version": "1.30.0", + "resolved": "https://registry.npmjs.org/prismjs/-/prismjs-1.30.0.tgz", + "integrity": "sha512-DEvV2ZF2r2/63V+tK8hQvrR2ZGn10srHbXviTlcv7Kpzw8jWiNTqbVgjO3IY8RxrrOUF8VPMQQFysYYYv0YZxw==", + "license": "MIT", + "engines": { + "node": ">=6" + } + }, "node_modules/process-warning": { "version": "5.0.0", "resolved": "https://registry.npmjs.org/process-warning/-/process-warning-5.0.0.tgz", @@ -8334,6 +8459,26 @@ } } }, + "node_modules/react-syntax-highlighter": { + "version": "16.1.1", + "resolved": "https://registry.npmjs.org/react-syntax-highlighter/-/react-syntax-highlighter-16.1.1.tgz", + "integrity": "sha512-PjVawBGy80C6YbC5DDZJeUjBmC7skaoEUdvfFQediQHgCL7aKyVHe57SaJGfQsloGDac+gCpTfRdtxzWWKmCXA==", + "license": "MIT", + "dependencies": { + "@babel/runtime": "^7.28.4", + "highlight.js": "^10.4.1", + "highlightjs-vue": "^1.0.0", + "lowlight": "^1.17.0", + "prismjs": "^1.30.0", + "refractor": "^5.0.0" + }, + "engines": { + "node": ">= 16.20.2" + }, + "peerDependencies": { + "react": ">= 0.14.0" + } + }, "node_modules/react-textarea-autosize": { "version": "8.5.9", "resolved": "https://registry.npmjs.org/react-textarea-autosize/-/react-textarea-autosize-8.5.9.tgz", @@ -8443,6 +8588,22 @@ "node": ">=8" } }, + "node_modules/refractor": { + "version": "5.0.0", + "resolved": "https://registry.npmjs.org/refractor/-/refractor-5.0.0.tgz", + "integrity": "sha512-QXOrHQF5jOpjjLfiNk5GFnWhRXvxjUVnlFxkeDmewR5sXkr3iM46Zo+CnRR8B+MDVqkULW4EcLVcRBNOPXHosw==", + "license": "MIT", + "dependencies": { + "@types/hast": "^3.0.0", + "@types/prismjs": "^1.0.0", + "hastscript": "^9.0.0", + "parse-entities": "^4.0.0" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/wooorm" + } + }, "node_modules/remark-gfm": { "version": "4.0.1", "resolved": "https://registry.npmjs.org/remark-gfm/-/remark-gfm-4.0.1.tgz", diff --git a/clients/web/package.json b/clients/web/package.json index 9c3e075d..14f3c1cc 100644 --- a/clients/web/package.json +++ b/clients/web/package.json @@ -50,11 +50,13 @@ "commander": "^13.1.0", "hono": "^4.12.18", "open": "^10.2.0", + "papaparse": "^5.5.4", "pino": "^9.14.0", "react": "^19.2.4", "react-dom": "^19.2.4", "react-icons": "^5.6.0", "react-markdown": "^10.1.0", + "react-syntax-highlighter": "^16.1.1", "remark-gfm": "^4.0.1", "zod": "^4.3.6", "zustand": "^5.0.13" @@ -72,8 +74,10 @@ "@testing-library/user-event": "^14.6.1", "@types/express": "^5.0.6", "@types/node": "^24.12.4", + "@types/papaparse": "^5.5.2", "@types/react": "^19.2.14", "@types/react-dom": "^19.2.3", + "@types/react-syntax-highlighter": "^15.5.13", "@vitejs/plugin-react": "^6.0.0", "@vitest/browser-playwright": "^4.1.0", "@vitest/coverage-v8": "^4.1.0", diff --git a/clients/web/src/components/elements/CodeHighlight/CodeHighlight.stories.tsx b/clients/web/src/components/elements/CodeHighlight/CodeHighlight.stories.tsx new file mode 100644 index 00000000..8c91af13 --- /dev/null +++ b/clients/web/src/components/elements/CodeHighlight/CodeHighlight.stories.tsx @@ -0,0 +1,42 @@ +import type { Meta, StoryObj } from "@storybook/react-vite"; +import { CodeHighlight } from "./CodeHighlight"; + +const meta: Meta = { + title: "Elements/CodeHighlight", + component: CodeHighlight, +}; + +export default meta; +type Story = StoryObj; + +export const Json: Story = { + args: { + language: "json", + code: JSON.stringify( + { name: "my-app", version: "1.0.0", tags: ["a", "b"] }, + null, + 2, + ), + }, +}; + +export const Xml: Story = { + args: { + language: "xml", + code: '\n first\n second\n', + }, +}; + +export const Css: Story = { + args: { + language: "css", + code: ".card {\n color: var(--text);\n padding: 1rem;\n}", + }, +}; + +export const UnknownLanguage: Story = { + args: { + language: "brainfuck", + code: "++++++++[>++++[>++>+++<<-]>+>->]", + }, +}; diff --git a/clients/web/src/components/elements/CodeHighlight/CodeHighlight.test.tsx b/clients/web/src/components/elements/CodeHighlight/CodeHighlight.test.tsx new file mode 100644 index 00000000..91049089 --- /dev/null +++ b/clients/web/src/components/elements/CodeHighlight/CodeHighlight.test.tsx @@ -0,0 +1,172 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { + renderWithMantine, + screen, + waitFor, +} from "../../../test/renderWithMantine"; + +// --- Mock the lazily-imported react-syntax-highlighter chunks ----------------- +// The prism-light runtime: a component that renders its code into a probeable +// element, plus a `registerLanguage` spy so tests can assert load behavior. +const registerLanguage = vi.fn(); +vi.mock("react-syntax-highlighter/dist/esm/prism-light", () => ({ + default: Object.assign( + ({ language, children }: { language: string; children: string }) => ( + 
+        {children}
+
+ ), + { registerLanguage }, + ), +})); +vi.mock("react-syntax-highlighter/dist/esm/styles/prism/tomorrow", () => ({ + default: {}, +})); +vi.mock("react-syntax-highlighter/dist/esm/languages/prism/json", () => ({ + default: { __grammar: "json" }, +})); +vi.mock("react-syntax-highlighter/dist/esm/languages/prism/markup", () => ({ + default: { __grammar: "markup" }, +})); +vi.mock("react-syntax-highlighter/dist/esm/languages/prism/css", () => ({ + default: { __grammar: "css" }, +})); +vi.mock("react-syntax-highlighter/dist/esm/languages/prism/yaml", () => ({ + default: { __grammar: "yaml" }, +})); +// markdown's import rejects, exercising the failed-load path. +vi.mock("react-syntax-highlighter/dist/esm/languages/prism/markdown", () => { + throw new Error("grammar load failed"); +}); + +// Module-level caches in CodeHighlight must reset between tests; re-import fresh. +async function loadComponent() { + vi.resetModules(); + registerLanguage.mockClear(); + return (await import("./CodeHighlight")).CodeHighlight; +} + +beforeEach(() => { + vi.resetModules(); +}); + +describe("CodeHighlight", () => { + it("renders plain code initially, then upgrades to the highlighter", async () => { + const CodeHighlight = await loadComponent(); + renderWithMantine(); + // Plain Mantine Code before the grammar resolves. + expect(screen.queryByTestId("prism")).not.toBeInTheDocument(); + expect(screen.getByText('{"a":1}')).toBeInTheDocument(); + // After the lazy grammar loads, the prism runtime takes over. + const prism = await screen.findByTestId("prism"); + expect(prism).toHaveAttribute("data-language", "json"); + expect(registerLanguage).toHaveBeenCalledWith("json", { + __grammar: "json", + }); + }); + + it("resolves the xml alias to the markup grammar", async () => { + const CodeHighlight = await loadComponent(); + renderWithMantine(); + const prism = await screen.findByTestId("prism"); + expect(prism).toHaveAttribute("data-language", "markup"); + expect(registerLanguage).toHaveBeenCalledWith("markup", { + __grammar: "markup", + }); + }); + + it("stays plain for an unknown language and never imports a grammar", async () => { + const CodeHighlight = await loadComponent(); + renderWithMantine(); + // Give the effect a chance to run. + await waitFor(() => expect(screen.getByText("++.")).toBeInTheDocument()); + expect(screen.queryByTestId("prism")).not.toBeInTheDocument(); + expect(registerLanguage).not.toHaveBeenCalled(); + }); + + it("stays plain when a grammar import rejects", async () => { + const CodeHighlight = await loadComponent(); + renderWithMantine(); + await waitFor(() => expect(screen.getByText("# hi")).toBeInTheDocument()); + // markdown's import throws → failed load → never registered, stays plain. + expect(screen.queryByTestId("prism")).not.toBeInTheDocument(); + expect(registerLanguage).not.toHaveBeenCalledWith( + "markdown", + expect.anything(), + ); + }); + + it("shares one grammar load across concurrent mounts of the same language", async () => { + const CodeHighlight = await loadComponent(); + renderWithMantine( + <> + + + , + ); + await waitFor(() => expect(screen.getAllByTestId("prism")).toHaveLength(2)); + // Both mounts shared the in-flight promise → grammar registered once. + expect(registerLanguage).toHaveBeenCalledTimes(1); + expect(registerLanguage).toHaveBeenCalledWith("css", { __grammar: "css" }); + }); + + it("reuses an already-registered grammar without re-importing", async () => { + const CodeHighlight = await loadComponent(); + const { unmount } = renderWithMantine( + , + ); + await screen.findByTestId("prism"); + expect(registerLanguage).toHaveBeenCalledTimes(1); + unmount(); + // A second mount finds json already registered: ready synchronously, no + // second registerLanguage call. + renderWithMantine(); + expect(await screen.findByTestId("prism")).toBeInTheDocument(); + expect(registerLanguage).toHaveBeenCalledTimes(1); + }); + + it("loads a second language reusing the already-loaded runtime", async () => { + const CodeHighlight = await loadComponent(); + const { unmount } = renderWithMantine( + , + ); + await screen.findByTestId("prism"); + unmount(); + // Runtime is cached now; mounting css registers only the new grammar. + renderWithMantine(); + const prism = await screen.findByTestId("prism"); + expect(prism).toHaveAttribute("data-language", "css"); + expect(registerLanguage).toHaveBeenCalledTimes(2); + expect(registerLanguage).toHaveBeenLastCalledWith("css", { + __grammar: "css", + }); + }); + + it("shares one runtime load across two different languages mounted together", async () => { + const CodeHighlight = await loadComponent(); + renderWithMantine( + <> + + + , + ); + await waitFor(() => expect(screen.getAllByTestId("prism")).toHaveLength(2)); + // Both grammars registered, but the shared runtime promise loaded once. + expect(registerLanguage).toHaveBeenCalledTimes(2); + }); + + it("does not retry a language whose grammar previously failed", async () => { + const CodeHighlight = await loadComponent(); + const { unmount } = renderWithMantine( + , + ); + await waitFor(() => expect(screen.getByText("# a")).toBeInTheDocument()); + unmount(); + // markdown is in failedLoads now; a second mount short-circuits and never + // attempts the import again — still plain, still no registration. + renderWithMantine(); + await waitFor(() => expect(screen.getByText("# b")).toBeInTheDocument()); + expect(screen.queryByTestId("prism")).not.toBeInTheDocument(); + expect(registerLanguage).not.toHaveBeenCalled(); + }); +}); diff --git a/clients/web/src/components/elements/CodeHighlight/CodeHighlight.tsx b/clients/web/src/components/elements/CodeHighlight/CodeHighlight.tsx new file mode 100644 index 00000000..cfce77b8 --- /dev/null +++ b/clients/web/src/components/elements/CodeHighlight/CodeHighlight.tsx @@ -0,0 +1,168 @@ +import { Code } from "@mantine/core"; +import { useEffect, useState } from "react"; +import type { ComponentType } from "react"; + +/** + * Lazy syntax-highlighting code block. The `react-syntax-highlighter` + * prism-light runtime, its theme chunk, and each language grammar are + * dynamic-imported on first use so a session that never opens a highlightable + * resource never pays for them. While a grammar is still loading (or the + * language is unknown) the raw code is shown in a plain Mantine `Code` block so + * users never see a flash of unstyled tokens. + */ +export interface CodeHighlightProps { + /** Highlight.js / Prism language tag (or an alias — see {@link LANGUAGE_ALIASES}). */ + language: string; + /** The source text to render. */ + code: string; +} + +/** A Prism grammar object; opaque to us — registered with the runtime as-is. */ +type Grammar = unknown; + +/** The prism-light runtime component plus its `registerLanguage` static. */ +type PrismRuntime = ComponentType<{ + language: string; + style: Record; + customStyle?: Record; + wrapLongLines?: boolean; + children: string; +}> & { registerLanguage: (name: string, grammar: Grammar) => void }; + +/** + * Static per-language import thunks. Each is a separate dynamic import so Vite + * emits one lazily-loaded chunk per grammar. Add an entry here (and an alias + * below if the canonical Prism name differs) as the type matrix grows. + */ +const LANGUAGE_LOADERS: Record Promise<{ default: Grammar }>> = { + json: () => import("react-syntax-highlighter/dist/esm/languages/prism/json"), + markup: () => + import("react-syntax-highlighter/dist/esm/languages/prism/markup"), + css: () => import("react-syntax-highlighter/dist/esm/languages/prism/css"), + yaml: () => import("react-syntax-highlighter/dist/esm/languages/prism/yaml"), + markdown: () => + import("react-syntax-highlighter/dist/esm/languages/prism/markdown"), +}; + +/** Friendly language tags → the canonical Prism grammar name they resolve to. */ +const LANGUAGE_ALIASES: Record = { + xml: "markup", + html: "markup", + htm: "markup", + svg: "markup", + yml: "yaml", + md: "markdown", +}; + +/** Grammars successfully loaded + registered this session. */ +const registeredLanguages = new Set(); +/** Grammars whose import rejected / are unknown — never retried. */ +const failedLoads = new Set(); +/** In-flight grammar loads, so concurrent mounts share one async call. */ +const loadingPromises = new Map>(); + +/** The shared prism-light runtime component + theme. */ +interface Runtime { + Prism: PrismRuntime; + style: Record; +} + +/** The shared prism-light runtime + theme, loaded once. */ +let runtime: Runtime | null = null; +let runtimePromise: Promise | null = null; + +/** Resolve an alias to its canonical Prism grammar name. */ +function resolveLanguage(language: string): string { + return LANGUAGE_ALIASES[language] ?? language; +} + +/** Load (and cache) the prism-light runtime component and the `tomorrow` theme. */ +async function ensureRuntime(): Promise { + if (runtime) return runtime; + if (!runtimePromise) { + runtimePromise = (async () => { + const [prismMod, styleMod] = await Promise.all([ + import("react-syntax-highlighter/dist/esm/prism-light"), + import("react-syntax-highlighter/dist/esm/styles/prism/tomorrow"), + ]); + runtime = { + Prism: prismMod.default as PrismRuntime, + style: styleMod.default, + }; + return runtime; + })(); + } + return runtimePromise; +} + +/** + * Ensure the grammar for `language` is loaded and registered. Resolves once the + * language is ready, a prior load failed, or the language is unknown — callers + * re-check {@link isLanguageReady} afterward rather than relying on this throwing. + */ +async function ensureLanguage(language: string): Promise { + const name = resolveLanguage(language); + if (registeredLanguages.has(name) || failedLoads.has(name)) return; + const inFlight = loadingPromises.get(name); + if (inFlight) return inFlight; + + const loader = LANGUAGE_LOADERS[name]; + if (!loader) { + failedLoads.add(name); + return; + } + + const load = (async () => { + try { + const rt = await ensureRuntime(); + const { default: grammar } = await loader(); + rt.Prism.registerLanguage(name, grammar); + registeredLanguages.add(name); + } catch { + failedLoads.add(name); + } finally { + loadingPromises.delete(name); + } + })(); + loadingPromises.set(name, load); + return load; +} + +const PlainCode = Code.withProps({ block: true }); + +export function CodeHighlight({ language, code }: CodeHighlightProps) { + // Readiness is derived from the module-level caches during render (so a + // language already loaded this session highlights on first paint, including + // after the `language` prop changes). The effect only bumps a tick when an + // async load finishes, forcing a re-render that re-reads the caches. + const [, bumpTick] = useState(0); + + useEffect(() => { + let cancelled = false; + void ensureLanguage(language).then(() => { + if (!cancelled) bumpTick((t) => t + 1); + }); + return () => { + cancelled = true; + }; + }, [language]); + + const resolved = resolveLanguage(language); + const rt = runtime; + // Plain block until the runtime has loaded and this grammar is registered. + if (!rt || !registeredLanguages.has(resolved)) { + return {code}; + } + + const { Prism, style } = rt; + return ( + + {code} + + ); +} diff --git a/clients/web/src/components/elements/ContentViewer/BinaryNotice.tsx b/clients/web/src/components/elements/ContentViewer/BinaryNotice.tsx new file mode 100644 index 00000000..2a710c4e --- /dev/null +++ b/clients/web/src/components/elements/ContentViewer/BinaryNotice.tsx @@ -0,0 +1,24 @@ +import { Code, Flex, Stack } from "@mantine/core"; + +const ContentWrapper = Flex.withProps({ + pos: "relative", + direction: "column", +}); + +/** + * Fallback shown when content can't be previewed — an unsupported binary MIME + * type, or a blob whose base64 fails to decode. Kept in its own module so blob + * renderers (e.g. {@link PdfFrame}) can degrade to it without importing back + * into {@link ContentViewer} (which would form an import cycle). + */ +export function BinaryNotice({ mimeType }: { mimeType: string }) { + return ( + + + + {`[Binary content (${mimeType}) — preview not supported]`} + + + + ); +} diff --git a/clients/web/src/components/elements/ContentViewer/ContentViewer.stories.tsx b/clients/web/src/components/elements/ContentViewer/ContentViewer.stories.tsx index ac861fdd..a97ce01f 100644 --- a/clients/web/src/components/elements/ContentViewer/ContentViewer.stories.tsx +++ b/clients/web/src/components/elements/ContentViewer/ContentViewer.stories.tsx @@ -86,3 +86,68 @@ export const ResourceLinkBlock: Story = { }, }, }; + +// --- Resource contents (Resources screen) per-MIME dispatch ------------------- + +export const JsonHighlighted: Story = { + args: { + contents: { + uri: "file:///config.json", + mimeType: "application/json", + text: '{"name":"my-app","version":"1.0.0","tags":["a","b"]}', + }, + copyable: true, + }, +}; + +export const XmlHighlighted: Story = { + args: { + contents: { + uri: "file:///feed.xml", + mimeType: "application/xml", + text: "NewsOne", + }, + copyable: true, + }, +}; + +export const CssHighlighted: Story = { + args: { + contents: { + uri: "file:///styles.css", + mimeType: "text/css", + text: ".card { color: var(--text); padding: 1rem; border-radius: 8px; }", + }, + copyable: true, + }, +}; + +export const CsvTableContents: Story = { + args: { + contents: { + uri: "file:///people.csv", + mimeType: "text/csv", + text: "name,role,city\nAlice,Engineer,Berlin\nBob,Designer,Lisbon\nCarol,PM,Oslo", + }, + }, +}; + +export const HtmlSandboxed: Story = { + args: { + contents: { + uri: "file:///report.html", + mimeType: "text/html", + text: "

Report

This renders in a sandboxed iframe.

", + }, + }, +}; + +export const BinaryUnsupported: Story = { + args: { + contents: { + uri: "file:///archive.zip", + mimeType: "application/zip", + blob: "UEsDBAoAAAAAAA==", + }, + }, +}; diff --git a/clients/web/src/components/elements/ContentViewer/ContentViewer.test.tsx b/clients/web/src/components/elements/ContentViewer/ContentViewer.test.tsx index 7c2733a3..1b04403f 100644 --- a/clients/web/src/components/elements/ContentViewer/ContentViewer.test.tsx +++ b/clients/web/src/components/elements/ContentViewer/ContentViewer.test.tsx @@ -1,8 +1,26 @@ -import { describe, it, expect } from "vitest"; -import type { ContentBlock } from "@modelcontextprotocol/sdk/types.js"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import type { + BlobResourceContents, + ContentBlock, + TextResourceContents, +} from "@modelcontextprotocol/sdk/types.js"; import { renderWithMantine, screen } from "../../../test/renderWithMantine"; import { ContentViewer } from "./ContentViewer"; +// Stub the lazy highlighter so JSON/XML/CSS branches are assertable +// synchronously (its real dynamic-import behavior is covered in +// CodeHighlight.test.tsx). +vi.mock("../CodeHighlight/CodeHighlight", () => ({ + CodeHighlight: ({ language, code }: { language: string; code: string }) => ( + 
+      {code}
+
+ ), +})); + +const toBase64 = (text: string): string => + Buffer.from(text, "utf-8").toString("base64"); + describe("ContentViewer", () => { it("renders text content as-is when not JSON", () => { const block: ContentBlock = { type: "text", text: "hello world" }; @@ -185,4 +203,247 @@ describe("ContentViewer", () => { ); expect(screen.getByRole("button")).toBeInTheDocument(); }); + + it("highlights a JSON text block when mimeType is application/json", () => { + const block: ContentBlock = { type: "text", text: '{"a":1}' }; + renderWithMantine( + , + ); + const probe = screen.getByTestId("code-highlight"); + expect(probe).toHaveAttribute("data-language", "json"); + expect(probe.textContent).toContain('"a": 1'); + }); + + it("renders nothing when neither block nor contents is provided", () => { + const { container } = renderWithMantine(); + // MantineProvider injects a