From 7f10fa971c85d6f56ffcc1f75fbdc5bb428d49a2 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 23 Aug 2026 17:36:43 -0400 Subject: [PATCH] fix: address the Copilot review of the Ace editor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `isProgressToken` used `Number.isInteger`, but zod 4's `.int()` rejects anything past `MAX_SAFE_INTEGER` — verified against the pinned zod — so an unsafe integer passed the guard and was left for the server to reject. Now `isSafeInteger`, with the boundary covered in both directions. - `echoed` held only what this component last emitted, so it went stale the moment an external value was applied: A → B → A stranded the editor on B, because the second A matched the entry left from before B arrived and was dismissed as our own echo. It now advances to whatever is on screen. - `Input.Wrapper` associates a *Mantine* input through context, and Ace is not one — it renders its own DOM. The wrapper's label and error node therefore pointed at nothing: the editor was never marked `aria-invalid`, never described by the error text, and its visible label did not focus it. Wired explicitly through the wrapper's id in an effect. - Removed two leftover `[dbg]` logs an earlier regex cleanup missed. - Documented the editor integration and the shared `cssVariablesResolver` in the web README, per the rule about new dependencies and patterns. Both footguns are recorded: the `?url` worker import, and the gutter override keying off Ace's cssClass rather than the theme module name. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0177mPHAdECD18nLLCTwR5rh Signed-off-by: cliffhall --- clients/web/README.md | 100 ++++++++++-------- .../JsonObjectInput/JsonObjectInput.test.tsx | 75 ++++++++++++- .../JsonObjectInput/JsonObjectInput.tsx | 40 ++++++- .../integration/mcp/inspectorClient.test.ts | 6 ++ core/mcp/inspectorClient.ts | 8 +- 5 files changed, 178 insertions(+), 51 deletions(-) diff --git a/clients/web/README.md b/clients/web/README.md index 19f77385..db71915c 100644 --- a/clients/web/README.md +++ b/clients/web/README.md @@ -6,10 +6,10 @@ This README covers what's specific to the web client. For the repo-wide picture ## Two halves: `src/` (browser) and `server/` (Node) -| Path | Runs in | Purpose | -| --- | --- | --- | -| `src/` | browser | The React SPA — components, hooks, theme, entry (`main.tsx`). | -| `server/` | Node | The dev/prod backend wiring (never imported by the browser). | +| Path | Runs in | Purpose | +| --------- | ------- | ------------------------------------------------------------- | +| `src/` | browser | The React SPA — components, hooks, theme, entry (`main.tsx`). | +| `server/` | Node | The dev/prod backend wiring (never imported by the browser). | The `server/` directory holds the Node-only backend: @@ -47,12 +47,12 @@ Two artifacts come out, both of which ship in the published package: Components live under `src/components/` in four layers, smallest to largest: -| Layer | Count | What it is | -| --- | --- | --- | -| `elements/` | ~31 | Leaf presentational pieces (badges, buttons, toggles) over Mantine primitives. | -| `groups/` | ~63 | Composite pieces (cards, panels, modals, control bars). | -| `screens/` | ~11 | Full tab screens (Tools, Resources, Servers, monitoring screens…). | -| `views/` | 1 | `InspectorView` — the top-level layout that composes the screens. | +| Layer | Count | What it is | +| ----------- | ----- | ------------------------------------------------------------------------------ | +| `elements/` | ~31 | Leaf presentational pieces (badges, buttons, toggles) over Mantine primitives. | +| `groups/` | ~63 | Composite pieces (cards, panels, modals, control bars). | +| `screens/` | ~11 | Full tab screens (Tools, Resources, Servers, monitoring screens…). | +| `views/` | 1 | `InspectorView` — the top-level layout that composes the screens. | Every screen and element has a `*.stories.tsx` (see [Storybook](#storybook)). Styling follows the Mantine-first rules in [`AGENTS.md`](../../AGENTS.md) — theme variants and component props over CSS, `--inspector-*` tokens over raw colors. @@ -74,23 +74,23 @@ Nothing _enforces_ the boundary — no path alias keys off it, and the coverage The Apps screen exposes a small, stable set of `data-testid` / `data-*` attributes so an automated driver (deep-link auto-open, CI review harness) can `waitForSelector` on a deterministic signal instead of sleeping. Treat these as a public contract — drivers depend on them staying stable: -| Attribute | Where | Meaning | -| --- | --- | --- | -| `data-testid="apps-form"` | Apps content card | The container that carries the status/error attributes below. | -| `data-app-status` | on `apps-form` | Renderer lifecycle: `idle` (nothing running) → `loading` (bridge building / `ui/initialize` in flight) → `ready` (view fired `notifications/initialized`) → `error` (bridge factory threw/rejected). Poll for `ready`. | -| `data-app-error` | on `apps-form` | The failure reason string when `data-app-status="error"` (e.g. no connected client); absent otherwise. | -| `data-testid="apps-error"` | error panel | Rendered below the frame when the app fails to load (factory throw/reject); shows the reason so the failure isn't a silent blank frame. | -| `data-testid="open-app"` | Open App button | Launches the selected app. | -| `data-testid="apps-stage"` | Stage-partial button | Snapshots the current form values for progressive-render testing. | -| `data-testid="apps-messages"` | messages panel | `ui/message` submissions from the running view. | -| `data-testid="apps-logs"` | app-logs panel | `notifications/message` log entries (default-expanded). | +| Attribute | Where | Meaning | +| ----------------------------- | -------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `data-testid="apps-form"` | Apps content card | The container that carries the status/error attributes below. | +| `data-app-status` | on `apps-form` | Renderer lifecycle: `idle` (nothing running) → `loading` (bridge building / `ui/initialize` in flight) → `ready` (view fired `notifications/initialized`) → `error` (bridge factory threw/rejected). Poll for `ready`. | +| `data-app-error` | on `apps-form` | The failure reason string when `data-app-status="error"` (e.g. no connected client); absent otherwise. | +| `data-testid="apps-error"` | error panel | Rendered below the frame when the app fails to load (factory throw/reject); shows the reason so the failure isn't a silent blank frame. | +| `data-testid="open-app"` | Open App button | Launches the selected app. | +| `data-testid="apps-stage"` | Stage-partial button | Snapshots the current form values for progressive-render testing. | +| `data-testid="apps-messages"` | messages panel | `ui/message` submissions from the running view. | +| `data-testid="apps-logs"` | app-logs panel | `notifications/message` log entries (default-expanded). | App-rendered **elicitations** (#1854) render through the same `AppRenderer` but outside the Apps screen — one modal per request, from `AppElicitationHost` — and carry their own pair: -| Attribute | Where | Meaning | -| --- | --- | --- | +| Attribute | Where | Meaning | +| ------------------------------- | --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | `data-testid="app-elicitation"` | the elicitation modal | One per in-flight app-rendered elicitation. **Absent** means the request was answered by the native elicitation form instead, which is what a driver asserts to prove the negotiation gate held. | -| `data-app-elicitation-status` | on `app-elicitation` | The same `AppRendererStatus` for that modal's app. `ready` is when the host forwards the `elicitation/create` through its bridge. | +| `data-app-elicitation-status` | on `app-elicitation` | The same `AppRendererStatus` for that modal's app. `ready` is when the host forwards the `elicitation/create` through its bridge. | `scripts/smoke-web-elicitation.mjs` drives both halves against the public fixture (`test-servers/configs/app-elicitation-http.json` and its `-native-` sibling). @@ -104,20 +104,20 @@ A driver (launcher, CLI `--print-handoff`, CI review harness) can reach a **conn http://127.0.0.1:6274/?serverUrl=&transport=http|sse&autoConnect= ``` -| Param | Meaning | -| --- | --- | -| `serverUrl` | The MCP server URL. Restricted to `http:` / `https:` (a crafted `javascript:` / `data:` / `file:` value is rejected). Canonicalized via `URL.href` so it matches the OAuth store's key form. | -| `transport` | `http` (streamable-HTTP, the default) or `sse`. Unknown values fall back to `http`. | +| Param | Meaning | +| ------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `serverUrl` | The MCP server URL. Restricted to `http:` / `https:` (a crafted `javascript:` / `data:` / `file:` value is rejected). Canonicalized via `URL.href` so it matches the OAuth store's key form. | +| `transport` | `http` (streamable-HTTP, the default) or `sse`. Unknown values fall back to `http`. | | `autoConnect` | **CSRF gate.** Must equal the per-launch `MCP_INSPECTOR_API_TOKEN`. The token is random per launch and only known to whatever started the server, so a third-party-minted link cannot satisfy it — this is the same exposure surface as the existing `?MCP_INSPECTOR_API_TOKEN=` param. Without a match the link is ignored. | The deep link upserts a stable `deep-link` catalog row (so a reload reconnects to the same row instead of accumulating duplicates) and connects. Connection-level outcomes are surfaced on the `AppShell.Header` as a machine-readable contract, so a driver can `waitForSelector` and read the failure reason without scraping a transient toast: -| Attribute | Where | Meaning | -| --- | --- | --- | -| `data-testid="connection-status"` | header | The element carrying the attributes below. | -| `data-status` | on `connection-status` | The live `ConnectionStatus` (`disconnected` → `connecting` → `connected` / `error`). Poll for `connected`. | -| `data-error-message` | on `connection-status` | Why the last connect failed (handshake error, OAuth-start failure, deep-link automation failure); absent when there is no error. | -| `data-deeplink` | on `connection-status` | `parsed` (a valid deep link drove this load), `rejected` (deep-link params present but the token/serverUrl gate failed), or `none`. Lets a driver distinguish "no deep link" from "rejected" — both otherwise leave `data-status` idle. | +| Attribute | Where | Meaning | +| --------------------------------- | ---------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `data-testid="connection-status"` | header | The element carrying the attributes below. | +| `data-status` | on `connection-status` | The live `ConnectionStatus` (`disconnected` → `connecting` → `connected` / `error`). Poll for `connected`. | +| `data-error-message` | on `connection-status` | Why the last connect failed (handshake error, OAuth-start failure, deep-link automation failure); absent when there is no error. | +| `data-deeplink` | on `connection-status` | `parsed` (a valid deep link drove this load), `rejected` (deep-link params present but the token/serverUrl gate failed), or `none`. Lets a driver distinguish "no deep link" from "rejected" — both otherwise leave `data-status` idle. | ### Landing on a rendered app @@ -127,11 +127,11 @@ Three further params extend the deep link to pre-select — and optionally auto- …&openApp=&appArgs=&autoOpen= ``` -| Param | Meaning | -| --- | --- | -| `openApp` | The app-tool name. Once the connection is up and the tool appears in the app list, the inspector switches to the Apps tab and pre-selects it. | -| `appArgs` | `base64url(JSON)` object of form values. Merged **over** the tool's schema defaults (`collectSchemaDefaults`) so a required-with-default field isn't left blank — which would otherwise disable "Open App". Malformed / non-object values fall back to `{}`. | -| `autoOpen` | **Same CSRF gate as `autoConnect`** — must equal the session token. When set, "Open App" fires automatically (a tool call from a URL), so the token gate is mandatory. Without a match the app is pre-selected but not opened. | +| Param | Meaning | +| ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `openApp` | The app-tool name. Once the connection is up and the tool appears in the app list, the inspector switches to the Apps tab and pre-selects it. | +| `appArgs` | `base64url(JSON)` object of form values. Merged **over** the tool's schema defaults (`collectSchemaDefaults`) so a required-with-default field isn't left blank — which would otherwise disable "Open App". Malformed / non-object values fall back to `{}`. | +| `autoOpen` | **Same CSRF gate as `autoConnect`** — must equal the session token. When set, "Open App" fires automatically (a tool call from a URL), so the token gate is mandatory. Without a match the app is pre-selected but not opened. | The app-side render lifecycle is observable through the [MCP Apps screen automation contract](#mcp-apps-screen-automation-contract) above (`data-app-status="ready"`), so a driver can `waitForSelector` the whole `connect → open → ready` chain deterministically. @@ -139,15 +139,29 @@ The app-side render lifecycle is observable through the [MCP Apps screen automat Each customized Mantine component has a `Theme.ts` file (`Button.ts`, `Text.ts`, …, ~21 total) exporting a `Theme` constant; the barrel `index.ts` re-exports them and `theme.ts` assembles the `MantineProvider` theme. Theme files hold app-wide defaults and **variants** (flat CSS-in-JS); only pseudo-selectors, nested child selectors, keyframes, and native-HTML styling belong in `App.css`. Element components import from `@mantine/core` (never from `theme/`) — the theme layer is applied transparently by the provider. +**`cssVariables.ts` is the third piece, beside the component files and `App.css`.** It holds overrides for the CSS variables `MantineProvider` injects at runtime, which `App.css` cannot reach: the provider appends its generated `