Add UI-backed whoami tool to MCP server#1894
Conversation
chelojimenez
temporarily deployed
to
preview-pr-1894
GitHub Actions
Inactive
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
MCP worker previewPreview worker |
Internal previewPreview URL: https://mcp-inspector-pr-1894.up.railway.app |
dosubot
Bot
added
the
size:XL
chelojimenez
temporarily deployed
to
preview-pr-1894
GitHub Actions
Inactive
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a Vite+React whoami UI (HTML, CSS module, TSX), global styles, Vite config with single-file bundling, and Vite typings. Introduces a bundle script that inlines built app HTML into a generated TypeScript file plus a Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 2f04093. Configure here.
|
|
||
| export function bundleMcpAppsHtml(): void; | ||
| export function bundleMcpAppsHtmlPlugin(): Plugin; | ||
| } |
There was a problem hiding this comment.
Wrong relative path in declare module specifier
Low Severity
The declare module specifier "./scripts/bundle-mcp-app-html.mjs" is resolved relative to the .d.ts file's own location (mcp/scripts/), so it declares types for the non-existent path mcp/scripts/scripts/bundle-mcp-app-html.mjs. This means the type declaration has no effect — it doesn't match the import "./scripts/bundle-mcp-app-html.mjs" used in vite.config.ts (which resolves from mcp/). The @ts-expect-error on that import line masks the issue today, but the .d.ts file is dead code. Using top-level exports (without the declare module wrapper) would let TypeScript co-locate the types with the .mjs file automatically.
Reviewed by Cursor Bugbot for commit 2f04093. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
mcp/tsconfig.json (1)
6-28: Consider separating worker and UI type contexts.By widening
libwithDOMandDOM.Iterablein a single config that also type-checks the Cloudflare Worker entrypoint, you allow browser-only globals (e.g.,window,document,HTMLElement) to silently satisfy type-checks inside worker code. The Workers runtime types and DOM types overlap on several globals (fetch,Request,Response,WebSocket,URL, etc.) with subtly different shapes — collisions here are a classic source of “works in dev, fails at deploy” surprises.A cleaner arrangement is a base
tsconfig.jsonfor the worker (noDOMlib) plus atsconfig.ui.jsonvia project references that includes onlysrc/ui/**with DOM libs. Not a blocker; the build still works thanks toskipLibCheck, but the separation preserves the sandboxing the worker target deserves.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/tsconfig.json` around lines 6 - 28, The current tsconfig includes DOM libs in "lib" which lets browser globals leak into Cloudflare Worker code; create a base tsconfig (remove "DOM" and "DOM.Iterable" from the "lib" array in the existing tsconfig and keep worker-specific settings like "strict", "noEmit", "esModuleInterop", "skipLibCheck", and "worker-configuration.d.ts"), then add a separate tsconfig.ui.json that extends the base and adds "DOM" and "DOM.Iterable" to "lib" and restricts "include" to UI files (e.g., "src/ui/**" or "src/**/*.tsx") so UI TSX sees DOM types while worker entrypoints are type-checked without DOM globals; update any build/test scripts to reference the new tsconfig.ui.json for UI tasks and keep the original tsconfig for worker compilation.mcp/package.json (1)
15-15:typecheckperforms a full Vite build — heavier than it needs to be.Because
McpAppsHtml.bundled.tsis authored by the Vite build pipeline,tsc --noEmitdepends on the artifact existing, so the ordering is defensible. Worth noting, though, that this makestypecheckas slow as a full production build — inconvenient in CI and pre-commit hooks. Consider committing a stub (or lazy-loading the bundled HTML from disk at runtime) so that typecheck can run without the Vite build step. Optional; flagged for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/package.json` at line 15, The "typecheck" npm script currently runs a full Vite build before running "tsc --noEmit" because the generated artifact McpAppsHtml.bundled.ts is required; to avoid the heavy build, add a committed stub of McpAppsHtml.bundled.ts (or change the runtime to lazy-load/read the bundled HTML from disk) so TypeScript can typecheck without running the Vite build. Locate the "typecheck" script in package.json and either (a) add a minimal committed McpAppsHtml.bundled.ts that satisfies the type imports/exports used by the code, or (b) modify the code paths that import McpAppsHtml.bundled.ts to use a runtime file-read/lazy import that does not require the build artifact during typecheck; ensure the chosen approach preserves existing types and update imports/exports accordingly.mcp/vite.config.ts (1)
6-7: The.d.tsfile's relative module specifier doesn't resolve via ambient declarations; remove it or convert the helper to TypeScript.The declaration file at
mcp/scripts/bundle-mcp-app-html.d.tsuses a relative specifier ("./scripts/bundle-mcp-app-html.mjs"), but TypeScript's ambientdeclare moduleresolution only matches non-relative specifiers. Consequently, the.d.tsprovides no actual typing for this import, and the@ts-expect-errorremains necessary.Two clean options: (a) port the helper to TypeScript (
.mts) for native type-checking, eliminating both the shim and the suppression, or (b) restructure the declaration to use a non-relative specifier. The current pairing is redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/vite.config.ts` around lines 6 - 7, The import of bundleMcpAppsHtmlPlugin uses a relative ESM path ("./scripts/bundle-mcp-app-html.mjs") but the ambient declaration in mcp/scripts/bundle-mcp-app-html.d.ts only matches non-relative module names, so the .d.ts doesn't provide types and the `@ts-expect-error` is still required; fix by either (A) porting the helper to TypeScript (rename to .mts, update export in the helper, change the import of bundleMcpAppsHtmlPlugin to the .mts module, remove the `@ts-expect-error` and delete the redundant .d.ts), or (B) change the declaration to a non-relative module name that matches the import (or change the import to a matching non-relative specifier), update the .d.ts to declare that exact module name, and then remove the `@ts-expect-error` so TypeScript picks up the ambient types.mcp/scripts/bundle-mcp-app-html.mjs (2)
24-28: Surface a friendlier error when the expected dist HTML is missing.If the Vite build hasn't produced
dist/src/ui/whoami.htmlyet (e.g. the script is run standalone, or a future refactor relocates the output),readFileSyncthrows a rawENOENTthat gives no clue this is about MCP app bundling. A one-line wrap makes the failure self-describing and cuts down debugging time for anyone who extends theAPPSlist.♻️ Proposed wrap
${APPS.map(({ exportName, fileName }) => { - const html = readFileSync(join(DIST_DIR, fileName), "utf-8"); + const absolutePath = join(DIST_DIR, fileName); + let html; + try { + html = readFileSync(absolutePath, "utf-8"); + } catch (cause) { + throw new Error( + `Failed to read bundled MCP app HTML for ${exportName} at ${absolutePath}. Did the Vite build run first?`, + { cause } + ); + } return `export const ${exportName} = ${JSON.stringify(html)};`; }).join("\n\n")}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/scripts/bundle-mcp-app-html.mjs` around lines 24 - 28, The script uses readFileSync(DIST_DIR, fileName) inside the APPS.map and will surface a raw ENOENT if an expected dist HTML is missing; wrap the readFileSync call in a try/catch around the APPS mapping (or inside the map) and when catching an error for a missing file (e.g., from readFileSync) rethrow a new, friendlier Error that includes the missing fileName and DIST_DIR and mentions this is during MCP app bundling (refer to APPS, exportName, fileName, readFileSync, DIST_DIR) so consumers see a clear message like "MCP bundling failed: missing built HTML for <fileName> in <DIST_DIR>" instead of the raw ENOENT.
9-12: Consider gitignoring the generated bundle.
src/generated/McpAppsHtml.bundled.tsis a build artefact — the HTML it embeds will churn whenever the React app changes, producing very noisy diffs (every CSS/JS byte of the bundled app ends up as a JSON-escaped string in the committed file). Addingmcp/src/generated/to.gitignoreand ensuring it is regenerated beforedev,deploy, andtypecheck(as the PR description already promises) keeps review diffs focused on source.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/scripts/bundle-mcp-app-html.mjs` around lines 9 - 12, Add the generated bundle directory to gitignore and ensure the build step regenerates it: add "mcp/src/generated/" to .gitignore so the file referenced by OUTPUT_TS_PATH (McpAppsHtml.bundled.ts) is not committed, and update the relevant scripts (invoke the bundle-mcp-app-html.mjs script) so the generated file is produced automatically before running dev, deploy, and typecheck (or add it to CI pre-steps) to keep the repo free of noisy generated diffs while still guaranteeing the bundle exists when needed.mcp/src/ui/whoami.tsx (2)
113-121:structuredContentis taken on faith — a shallow guard would harden the UI.Casting
toolResult.structuredContent as WhoamiPayload | undefinedwill happily accept any object the host happens to hand back (including legacy servers, malformed payloads, or another tool's result delivered mid-session). A cheap structural check avoids a confusing partial render where, say,payload.idisundefinedand the "Raw JSON" panel shows something entirely unrelated.🛡️ Suggested guard
- const payload = toolResult.structuredContent as WhoamiPayload | undefined; - if (!payload) { + const payload = isWhoamiPayload(toolResult.structuredContent) + ? toolResult.structuredContent + : undefined; + if (!payload) { return ( <MessageBox label="Missing structured content" message="The whoami tool did not include structured content for the UI view." /> ); }function isWhoamiPayload(value: unknown): value is WhoamiPayload { return ( !!value && typeof value === "object" && "id" in value && typeof (value as { id: unknown }).id === "string" ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/whoami.tsx` around lines 113 - 121, Replace the blind cast of toolResult.structuredContent to WhoamiPayload with a runtime type guard: add an isWhoamiPayload(value: unknown): value is WhoamiPayload that checks value is an object and that value.id exists and is a string (and any other required fields), then use if (!isWhoamiPayload(toolResult.structuredContent)) return the existing MessageBox; otherwise assign payload = toolResult.structuredContent (narrowed by the guard) and proceed — reference toolResult.structuredContent, WhoamiPayload, and the new isWhoamiPayload helper when making the change.
210-214: Non-null assertion on#rootsilently hides mount failures.
document.getElementById("root")!will coerce a missing element to anull-that-pretends-not-to-be, andcreateRoot(null).render(...)throws from deep inside React with a stack that points nowhere useful. A guarded lookup gives whoever is embedding this bundle a crisp error message.♻️ Small guard
-createRoot(document.getElementById("root")!).render( - <StrictMode> - <WhoamiApp /> - </StrictMode> -); +const rootElement = document.getElementById("root"); +if (!rootElement) { + throw new Error("whoami UI: missing `#root` mount point in bundled HTML."); +} +createRoot(rootElement).render( + <StrictMode> + <WhoamiApp /> + </StrictMode> +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/ui/whoami.tsx` around lines 210 - 214, Replace the non-null assertion on the root element lookup so we fail fast with a clear message instead of letting React throw deep errors: check document.getElementById("root") before calling createRoot and, if missing, throw or log an explicit error like "Missing root element '#root' - mount failed" and skip calling createRoot(...). Update the code around createRoot(...).render(...) / WhoamiApp to use the guarded root element variable.mcp/src/tools/sessionToolRegistrar.ts (1)
85-90: The pair ofas anycasts masks a real type mismatch withregisterAppTool.Once lands the exact
registerAppToolsignature is known (see verification in the sibling comment), these casts should be replaced with a properly typed call — future schema changes in@modelcontextprotocol/ext-apps/serverwill otherwise land as runtime errors rather than type errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/tools/sessionToolRegistrar.ts` around lines 85 - 90, The call to registerAppTool in sessionToolRegistrar.ts currently uses two "as any" casts around the argument object (containing resourceUri: ui.resourceUri) and the callback ((ui.callback ?? callback) as any), which hides a real type mismatch; replace those casts by aligning the call with the actual registerAppTool signature from `@modelcontextprotocol/ext-apps/server`: import and use the correct parameter and callback types (or the exported Request/Tool/Callback types) and shape the first argument to match the expected Tool/Register payload (e.g., properly typed resourceUri field and any nested properties) and type the callback parameter to the expected handler type instead of force-casting to any so TypeScript will surface schema changes as compile errors.mcp/src/server.ts (1)
59-67: Unsafe cast aroundClientCapabilities.extensions— pin it to a named type.The inline intersection
(ClientCapabilities & { extensions?: Record<string, unknown> })will silently decouple from any future SDK-provided extension typings. Hoisting a named type (or a tiny type guard) keeps the contract visible and lets the compiler help if the SDK eventually typesextensionsnatively.♻️ Suggested tidy-up
+type CapabilitiesWithExtensions = ClientCapabilities & { + extensions?: Record<string, unknown>; +}; + function getUiCapability( clientCapabilities: ClientCapabilities | undefined ): { mimeTypes?: string[] } | undefined { - const extensions = (clientCapabilities as - | (ClientCapabilities & { extensions?: Record<string, unknown> }) - | undefined)?.extensions; - - return extensions?.[UI_EXTENSION_ID] as { mimeTypes?: string[] } | undefined; + const extensions = (clientCapabilities as CapabilitiesWithExtensions | undefined) + ?.extensions; + return extensions?.[UI_EXTENSION_ID] as { mimeTypes?: string[] } | undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/server.ts` around lines 59 - 67, The inline intersection in getUiCapability should be replaced by a named type or small type guard to make the contract visible and catch future SDK changes; define a type like ClientCapabilitiesWithExtensions = ClientCapabilities & { extensions?: Record<string, unknown> } (or implement function isClientCapabilitiesWithExtensions(obj): obj is ClientCapabilitiesWithExtensions) and then use that named type or guard in getUiCapability instead of the inline (ClientCapabilities & { extensions?: Record<string, unknown> }) cast so the compiler can track the shape of extensions via the symbol ClientCapabilitiesWithExtensions (or isClientCapabilitiesWithExtensions) and avoid silent decoupling.mcp/src/tools/whoami.ts (1)
22-55: Consolidate the twin callbacks into a single source of truth.The plain and UI callbacks are near-identical twins: same fetch, same text payload, the UI variant merely sprinkles in
structuredContent. PersessionToolRegistrar.ts, the UI registrar already falls back to the primary callback whenui.callbackis omitted (ui.callback ?? callback). AttachingstructuredContentto the primary response lets you drop the duplicated UI callback entirely — the registrar will reuse it for both the text-fallback tool and the app-UI tool, and future edits won't need to be mirrored in two places.♻️ Proposed consolidation
async () => { const payload = await getWhoamiPayload(agent); return { content: [ { type: "text", text: JSON.stringify(payload, null, 2), }, ], + structuredContent: payload, }; }, { resourceUri: WHOAMI_RESOURCE_URI, html: WHOAMI_APP_HTML, resourceName: "MCPJam whoami UI", resourceMeta: { ui: { prefersBorder: true, }, }, - callback: async () => { - const payload = await getWhoamiPayload(agent); - - return { - content: [ - { - type: "text", - text: JSON.stringify(payload, null, 2), - }, - ], - structuredContent: payload, - }; - }, }Worth confirming whether the text-fallback tool is expected to emit
structuredContent(some hosts log or display it). If emitting it there is undesirable, keep the split but extract a sharedbuildResponse(payload, { withStructured })helper to eliminate the string-formatting duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/tools/whoami.ts` around lines 22 - 55, The two nearly identical callbacks (the top-level callback and the ui.callback under WHOAMI_RESOURCE_URI/WHOAMI_APP_HTML) should be consolidated into a single source of truth: remove the duplicated ui.callback and let the registrar fallback use the primary callback; modify the primary callback (the one that calls getWhoamiPayload(agent)) to include structuredContent: payload in its returned object along with the text JSON so both the text-fallback tool and the UI tool receive the same data; if emitting structuredContent in the text-fallback is undesirable instead extract a shared helper (e.g., buildResponse(payload, { withStructured })) and call it from both callback and ui.callback to avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp/package.json`:
- Around line 11-12: The current "dev" and "start" scripts run a one-shot
"build:ui" then "wrangler dev --env staging", so UI edits under src/ui/** don't
retrigger Vite or rebuild McpAppsHtml.bundled.ts; add the "concurrently"
devDependency and change the "dev" and "start" scripts to run "npm run ui:watch"
in parallel with "wrangler dev --env staging" (e.g., via concurrently running
"npm:ui:watch" and "wrangler dev --env staging") so Vite keeps rebuilding and
the worker serves updated bundled HTML without restarting the process. Ensure
"ui:watch" remains defined and used by the new scripts.
In `@mcp/src/ui/global.css`:
- Around line 33-35: The font-family declaration uses a quoted identifier
"SFMono-Regular" which Stylelint flags; update the font-family property in
global.css (the rule setting font-family) to remove the quotes around
SFMono-Regular so it reads SFMono-Regular, "SF Mono", ui-monospace, Menlo,
Consolas, monospace and keep quotes only for family names that require them.
In `@mcp/src/ui/whoami.module.css`:
- Around line 186-190: Insert a blank line between the custom-property cluster
and the following declaration inside the dark-mode .shell block: move the
background rule so there's an empty line after the --code-text: `#eef3fb`; line;
specifically update the block containing --code-text and the background
declaration (referencing the --code-text custom property and the background
rule) to include one blank line before the background declaration to satisfy
Stylelint's declaration-empty-line-before rule.
---
Nitpick comments:
In `@mcp/package.json`:
- Line 15: The "typecheck" npm script currently runs a full Vite build before
running "tsc --noEmit" because the generated artifact McpAppsHtml.bundled.ts is
required; to avoid the heavy build, add a committed stub of
McpAppsHtml.bundled.ts (or change the runtime to lazy-load/read the bundled HTML
from disk) so TypeScript can typecheck without running the Vite build. Locate
the "typecheck" script in package.json and either (a) add a minimal committed
McpAppsHtml.bundled.ts that satisfies the type imports/exports used by the code,
or (b) modify the code paths that import McpAppsHtml.bundled.ts to use a runtime
file-read/lazy import that does not require the build artifact during typecheck;
ensure the chosen approach preserves existing types and update imports/exports
accordingly.
In `@mcp/scripts/bundle-mcp-app-html.mjs`:
- Around line 24-28: The script uses readFileSync(DIST_DIR, fileName) inside the
APPS.map and will surface a raw ENOENT if an expected dist HTML is missing; wrap
the readFileSync call in a try/catch around the APPS mapping (or inside the map)
and when catching an error for a missing file (e.g., from readFileSync) rethrow
a new, friendlier Error that includes the missing fileName and DIST_DIR and
mentions this is during MCP app bundling (refer to APPS, exportName, fileName,
readFileSync, DIST_DIR) so consumers see a clear message like "MCP bundling
failed: missing built HTML for <fileName> in <DIST_DIR>" instead of the raw
ENOENT.
- Around line 9-12: Add the generated bundle directory to gitignore and ensure
the build step regenerates it: add "mcp/src/generated/" to .gitignore so the
file referenced by OUTPUT_TS_PATH (McpAppsHtml.bundled.ts) is not committed, and
update the relevant scripts (invoke the bundle-mcp-app-html.mjs script) so the
generated file is produced automatically before running dev, deploy, and
typecheck (or add it to CI pre-steps) to keep the repo free of noisy generated
diffs while still guaranteeing the bundle exists when needed.
In `@mcp/src/server.ts`:
- Around line 59-67: The inline intersection in getUiCapability should be
replaced by a named type or small type guard to make the contract visible and
catch future SDK changes; define a type like ClientCapabilitiesWithExtensions =
ClientCapabilities & { extensions?: Record<string, unknown> } (or implement
function isClientCapabilitiesWithExtensions(obj): obj is
ClientCapabilitiesWithExtensions) and then use that named type or guard in
getUiCapability instead of the inline (ClientCapabilities & { extensions?:
Record<string, unknown> }) cast so the compiler can track the shape of
extensions via the symbol ClientCapabilitiesWithExtensions (or
isClientCapabilitiesWithExtensions) and avoid silent decoupling.
In `@mcp/src/tools/sessionToolRegistrar.ts`:
- Around line 85-90: The call to registerAppTool in sessionToolRegistrar.ts
currently uses two "as any" casts around the argument object (containing
resourceUri: ui.resourceUri) and the callback ((ui.callback ?? callback) as
any), which hides a real type mismatch; replace those casts by aligning the call
with the actual registerAppTool signature from
`@modelcontextprotocol/ext-apps/server`: import and use the correct parameter and
callback types (or the exported Request/Tool/Callback types) and shape the first
argument to match the expected Tool/Register payload (e.g., properly typed
resourceUri field and any nested properties) and type the callback parameter to
the expected handler type instead of force-casting to any so TypeScript will
surface schema changes as compile errors.
In `@mcp/src/tools/whoami.ts`:
- Around line 22-55: The two nearly identical callbacks (the top-level callback
and the ui.callback under WHOAMI_RESOURCE_URI/WHOAMI_APP_HTML) should be
consolidated into a single source of truth: remove the duplicated ui.callback
and let the registrar fallback use the primary callback; modify the primary
callback (the one that calls getWhoamiPayload(agent)) to include
structuredContent: payload in its returned object along with the text JSON so
both the text-fallback tool and the UI tool receive the same data; if emitting
structuredContent in the text-fallback is undesirable instead extract a shared
helper (e.g., buildResponse(payload, { withStructured })) and call it from both
callback and ui.callback to avoid duplication.
In `@mcp/src/ui/whoami.tsx`:
- Around line 113-121: Replace the blind cast of toolResult.structuredContent to
WhoamiPayload with a runtime type guard: add an isWhoamiPayload(value: unknown):
value is WhoamiPayload that checks value is an object and that value.id exists
and is a string (and any other required fields), then use if
(!isWhoamiPayload(toolResult.structuredContent)) return the existing MessageBox;
otherwise assign payload = toolResult.structuredContent (narrowed by the guard)
and proceed — reference toolResult.structuredContent, WhoamiPayload, and the new
isWhoamiPayload helper when making the change.
- Around line 210-214: Replace the non-null assertion on the root element lookup
so we fail fast with a clear message instead of letting React throw deep errors:
check document.getElementById("root") before calling createRoot and, if missing,
throw or log an explicit error like "Missing root element '#root' - mount
failed" and skip calling createRoot(...). Update the code around
createRoot(...).render(...) / WhoamiApp to use the guarded root element
variable.
In `@mcp/tsconfig.json`:
- Around line 6-28: The current tsconfig includes DOM libs in "lib" which lets
browser globals leak into Cloudflare Worker code; create a base tsconfig (remove
"DOM" and "DOM.Iterable" from the "lib" array in the existing tsconfig and keep
worker-specific settings like "strict", "noEmit", "esModuleInterop",
"skipLibCheck", and "worker-configuration.d.ts"), then add a separate
tsconfig.ui.json that extends the base and adds "DOM" and "DOM.Iterable" to
"lib" and restricts "include" to UI files (e.g., "src/ui/**" or "src/**/*.tsx")
so UI TSX sees DOM types while worker entrypoints are type-checked without DOM
globals; update any build/test scripts to reference the new tsconfig.ui.json for
UI tasks and keep the original tsconfig for worker compilation.
In `@mcp/vite.config.ts`:
- Around line 6-7: The import of bundleMcpAppsHtmlPlugin uses a relative ESM
path ("./scripts/bundle-mcp-app-html.mjs") but the ambient declaration in
mcp/scripts/bundle-mcp-app-html.d.ts only matches non-relative module names, so
the .d.ts doesn't provide types and the `@ts-expect-error` is still required; fix
by either (A) porting the helper to TypeScript (rename to .mts, update export in
the helper, change the import of bundleMcpAppsHtmlPlugin to the .mts module,
remove the `@ts-expect-error` and delete the redundant .d.ts), or (B) change the
declaration to a non-relative module name that matches the import (or change the
import to a matching non-relative specifier), update the .d.ts to declare that
exact module name, and then remove the `@ts-expect-error` so TypeScript picks up
the ambient types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41e85e66-643a-4e04-afd2-9bd405459b14
⛔ Files ignored due to path filters (2)
mcp/src/generated/McpAppsHtml.bundled.tsis excluded by!**/generated/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
mcp/package.jsonmcp/scripts/bundle-mcp-app-html.d.tsmcp/scripts/bundle-mcp-app-html.mjsmcp/src/server.tsmcp/src/shared/whoami.tsmcp/src/tools/doctor.tsmcp/src/tools/getOrg.tsmcp/src/tools/getWorkspaces.tsmcp/src/tools/sessionToolRegistrar.tsmcp/src/tools/whoami.tsmcp/src/ui/global.cssmcp/src/ui/vite-env.d.tsmcp/src/ui/whoami.htmlmcp/src/ui/whoami.module.cssmcp/src/ui/whoami.tsxmcp/tsconfig.jsonmcp/vite.config.ts
| "dev": "npm run build:ui && wrangler dev --env staging", | ||
| "start": "npm run build:ui && wrangler dev --env staging", |
There was a problem hiding this comment.
Dev loop won't pick up UI edits after the first build.
dev and start do a one-shot build:ui and then hand off to wrangler dev. Edits under src/ui/** will neither retrigger Vite nor regenerate McpAppsHtml.bundled.ts, so the bundled HTML the worker serves will grow stale until the process is restarted. You already have ui:watch; wiring it alongside wrangler dev (e.g., via concurrently or npm-run-all -p) restores a proper inner loop.
🔧 One possible shape
- "dev": "npm run build:ui && wrangler dev --env staging",
- "start": "npm run build:ui && wrangler dev --env staging",
+ "dev": "npm run build:ui && concurrently -k -n ui,wrangler \"npm run ui:watch\" \"wrangler dev --env staging\"",
+ "start": "npm run build:ui && concurrently -k -n ui,wrangler \"npm run ui:watch\" \"wrangler dev --env staging\"",(Adding concurrently as a devDependency would be required.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp/package.json` around lines 11 - 12, The current "dev" and "start" scripts
run a one-shot "build:ui" then "wrangler dev --env staging", so UI edits under
src/ui/** don't retrigger Vite or rebuild McpAppsHtml.bundled.ts; add the
"concurrently" devDependency and change the "dev" and "start" scripts to run
"npm run ui:watch" in parallel with "wrangler dev --env staging" (e.g., via
concurrently running "npm:ui:watch" and "wrangler dev --env staging") so Vite
keeps rebuilding and the worker serves updated bundled HTML without restarting
the process. Ensure "ui:watch" remains defined and used by the new scripts.
| font-family: | ||
| "SFMono-Regular", "SF Mono", ui-monospace, Menlo, Consolas, monospace; | ||
| } |
There was a problem hiding this comment.
Drop the quotes from SFMono-Regular.
Per the CSS spec, unquoted identifier-style family names are preferred when no whitespace or reserved characters are present. Stylelint flags this one accordingly.
🎨 Proposed tweak
code,
pre {
font-family:
- "SFMono-Regular", "SF Mono", ui-monospace, Menlo, Consolas, monospace;
+ SFMono-Regular, "SF Mono", ui-monospace, Menlo, Consolas, monospace;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| font-family: | |
| "SFMono-Regular", "SF Mono", ui-monospace, Menlo, Consolas, monospace; | |
| } | |
| font-family: | |
| SFMono-Regular, "SF Mono", ui-monospace, Menlo, Consolas, monospace; | |
| } |
🧰 Tools
🪛 Stylelint (17.9.0)
[error] 34-34: Expected no quotes around "SFMono-Regular" (font-family-name-quotes)
(font-family-name-quotes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp/src/ui/global.css` around lines 33 - 35, The font-family declaration uses
a quoted identifier "SFMono-Regular" which Stylelint flags; update the
font-family property in global.css (the rule setting font-family) to remove the
quotes around SFMono-Regular so it reads SFMono-Regular, "SF Mono",
ui-monospace, Menlo, Consolas, monospace and keep quotes only for family names
that require them.
| --code-text: #eef3fb; | ||
| background: | ||
| radial-gradient(circle at top left, rgba(45, 212, 191, 0.18), transparent 42%), | ||
| radial-gradient(circle at top right, rgba(251, 191, 36, 0.16), transparent 35%), | ||
| linear-gradient(180deg, #0c1219 0%, #111a24 100%); |
There was a problem hiding this comment.
Stylelint: missing blank line before background declaration.
Stylelint's declaration-empty-line-before rule expects a blank line separating the custom-property cluster from the subsequent background rule inside the dark-mode .shell block.
🎨 Proposed fix
--code-bg: rgba(7, 10, 16, 0.96);
--code-text: `#eef3fb`;
+
background:
radial-gradient(circle at top left, rgba(45, 212, 191, 0.18), transparent 42%),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --code-text: #eef3fb; | |
| background: | |
| radial-gradient(circle at top left, rgba(45, 212, 191, 0.18), transparent 42%), | |
| radial-gradient(circle at top right, rgba(251, 191, 36, 0.16), transparent 35%), | |
| linear-gradient(180deg, #0c1219 0%, #111a24 100%); | |
| --code-text: `#eef3fb`; | |
| background: | |
| radial-gradient(circle at top left, rgba(45, 212, 191, 0.18), transparent 42%), | |
| radial-gradient(circle at top right, rgba(251, 191, 36, 0.16), transparent 35%), | |
| linear-gradient(180deg, `#0c1219` 0%, `#111a24` 100%); |
🧰 Tools
🪛 Stylelint (17.9.0)
[error] 187-190: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp/src/ui/whoami.module.css` around lines 186 - 190, Insert a blank line
between the custom-property cluster and the following declaration inside the
dark-mode .shell block: move the background rule so there's an empty line after
the --code-text: `#eef3fb`; line; specifically update the block containing
--code-text and the background declaration (referencing the --code-text custom
property and the background rule) to include one blank line before the
background declaration to satisfy Stylelint's declaration-empty-line-before
rule.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mcp/src/tools/sessionToolRegistrar.ts (1)
131-138: Resource display name can collide when two tools share a title.Defaulting the resource name to
${config.title ?? name} UImeans any two tools whose human-readable title happens to match will register resources under the same name. Defaulting to the tool's uniquename(titles are for display only, names are the identifiers) removes the surprise at zero cost.♻️ Proposed fix
- const resource = server.registerResource( - ui.resourceName ?? `${config.title ?? name} UI`, + const resource = server.registerResource( + ui.resourceName ?? `${name}-ui`, ui.resourceUri,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/src/tools/sessionToolRegistrar.ts` around lines 131 - 138, The resource naming currently defaults to human-readable titles which can collide; update the logic around server.registerResource so that when ui.resourceName is not provided it uses the tool's unique identifier (name) rather than config.title — e.g. derive the display identifier from name (e.g. `${name} UI`) and keep ui.resourceDescription fallback unchanged; update references where ui.resourceName is computed so server.registerResource uses ui.resourceName ?? `${name} UI` instead of `${config.title ?? name} UI` to avoid collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp/src/tools/sessionToolRegistrar.ts`:
- Around line 88-95: createSessionToolRegistrar currently doesn't remember the
last UI state so late-registered ToolUiToggle instances default to
uiEnabled=false; add a registrar-level variable (e.g., lastUiEnabled)
initialized from resolveUiEnabled() (or false if unavailable) and use that value
when constructing new ToolUiToggle objects so they are seeded with the current
UI state; update setUiEnabled, syncUiState, and any code paths that create
ToolUiToggle (references: createSessionToolRegistrar, SessionToolRegistrar,
ToolUiToggle, setUiEnabled, syncUiState) to read/write lastUiEnabled so
newly-registered tools reflect the last known UI state immediately.
---
Nitpick comments:
In `@mcp/src/tools/sessionToolRegistrar.ts`:
- Around line 131-138: The resource naming currently defaults to human-readable
titles which can collide; update the logic around server.registerResource so
that when ui.resourceName is not provided it uses the tool's unique identifier
(name) rather than config.title — e.g. derive the display identifier from name
(e.g. `${name} UI`) and keep ui.resourceDescription fallback unchanged; update
references where ui.resourceName is computed so server.registerResource uses
ui.resourceName ?? `${name} UI` instead of `${config.title ?? name} UI` to avoid
collisions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 505665e3-0843-49d2-a351-5d4d332ba0c5
📒 Files selected for processing (1)
mcp/src/tools/sessionToolRegistrar.ts
| export function createSessionToolRegistrar(server: McpServer): SessionToolRegistrar { | ||
| const uiToggles: ToolUiToggle[] = []; | ||
| const resolveUiEnabled = () => | ||
| getUiCapability( | ||
| server.server.getClientCapabilities() as | ||
| | { extensions?: Record<string, unknown> } | ||
| | undefined | ||
| )?.mimeTypes?.includes(RESOURCE_MIME_TYPE) ?? false; |
There was a problem hiding this comment.
Late-registered tools can miss the current UI state.
createSessionToolRegistrar never remembers the last value passed to setUiEnabled, and every new ToolUiToggle starts with uiEnabled = false. If a tool is registered after setUiEnabled(true) — which is exactly the ordering described in the PR (init() flips the flag, oninitialized flips it again) — the new tool will advertise its text-fallback variant until the next inbound request happens to trip syncUiState. A client that has already cached tools/list will never see the app-UI variant for that tool.
Cache the last state at the registrar level and seed new toggles with it.
♻️ Proposed fix
export function createSessionToolRegistrar(server: McpServer): SessionToolRegistrar {
const uiToggles: ToolUiToggle[] = [];
+ let currentUiEnabled = false;
const resolveUiEnabled = () =>
getUiCapability(
server.server.getClientCapabilities() as
| { extensions?: Record<string, unknown> }
| undefined
)?.mimeTypes?.includes(RESOURCE_MIME_TYPE) ?? false;
@@
- uiToggles.push(
- createToolUiToggle({
+ const toggle = createToolUiToggle({
name,
hiddenPlainName,
hiddenUiName,
plainTool,
uiTool,
resource,
- })
- );
+ });
+ toggle.setUiEnabled(currentUiEnabled);
+ uiToggles.push(toggle);
@@
setUiEnabled(enabled) {
+ currentUiEnabled = enabled;
for (const toggle of uiToggles) {
toggle.setUiEnabled(enabled);
}
},Also applies to: 153-178, 234-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp/src/tools/sessionToolRegistrar.ts` around lines 88 - 95,
createSessionToolRegistrar currently doesn't remember the last UI state so
late-registered ToolUiToggle instances default to uiEnabled=false; add a
registrar-level variable (e.g., lastUiEnabled) initialized from
resolveUiEnabled() (or false if unavailable) and use that value when
constructing new ToolUiToggle objects so they are seeded with the current UI
state; update setUiEnabled, syncUiState, and any code paths that create
ToolUiToggle (references: createSessionToolRegistrar, SessionToolRegistrar,
ToolUiToggle, setUiEnabled, syncUiState) to read/write lastUiEnabled so
newly-registered tools reflect the last known UI state immediately.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp/src/tools/sessionToolRegistrar.ts`:
- Around line 67-100: The tool registration is currently non-atomic:
registerAppTool mutates server state before registerAppResource and leaves a
dangling tool if the resource call throws; wrap the registerAppResource call in
a try/catch and on any error perform rollback by un-registering the tool (call
the inverse/unregister function or remove the entry from server state for the
tool returned by registerAppTool) and then rethrow or surface a clear combined
error; ensure you reference registerAppTool, registerAppResource, server, the
tool returned by registerAppTool, and ui.resourceUri when implementing the
rollback so the tool+UI registration is atomic from the caller's perspective.
- Around line 104-108: The setUiEnabled() method currently does nothing, so
calls like registrar.setUiEnabled(isUiEnabled(...)) are dead; either remove the
dead plumbing (delete setUiEnabled() and all registrar.setUiEnabled(...) calls
and leave advertising behavior unchanged) or implement real state tracking in
the SessionToolRegistrar class (add a private uiEnabled boolean, implement
setUiEnabled(ui: boolean) to store it, and make the code that advertises the app
tool consult this.uiEnabled so the isUiEnabled(...) result actually gates
advertising). Use the existing symbol names setUiEnabled,
registrar.setUiEnabled, and isUiEnabled to locate and update the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e187609-2295-4455-9e60-1ca42ea583c7
📒 Files selected for processing (1)
mcp/src/tools/sessionToolRegistrar.ts
| const tool = registerAppTool( | ||
| server, | ||
| name, | ||
| { | ||
| ...config, | ||
| _meta: { | ||
| ...(config._meta ?? {}), | ||
| ui: { | ||
| resourceUri: ui.resourceUri, | ||
| }, | ||
| }, | ||
| } as any, | ||
| (ui.callback ?? callback) as any | ||
| ); | ||
|
|
||
| registerAppResource( | ||
| server, | ||
| ui.resourceName ?? `${config.title ?? name} UI`, | ||
| ui.resourceUri, | ||
| { | ||
| description: | ||
| ui.resourceDescription ?? `${config.title ?? name} interactive UI`, | ||
| _meta: ui.resourceMeta, | ||
| }, | ||
| async () => ({ | ||
| contents: [ | ||
| { | ||
| uri: ui.resourceUri, | ||
| text: ui.html, | ||
| _meta: ui.resourceMeta, | ||
| }, | ||
| ], | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Partial-registration hazard when the resource call throws.
registerAppTool runs first and mutates server state; if registerAppResource subsequently throws (duplicate URI, validation, etc.), the tool is left registered without its companion UI resource — clients would then see a tool whose _meta.ui.resourceUri dangles. Consider wrapping the resource registration in a try/catch that either rolls back the tool registration or surfaces a clearer error, so the two registrations remain atomic from the caller's perspective.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp/src/tools/sessionToolRegistrar.ts` around lines 67 - 100, The tool
registration is currently non-atomic: registerAppTool mutates server state
before registerAppResource and leaves a dangling tool if the resource call
throws; wrap the registerAppResource call in a try/catch and on any error
perform rollback by un-registering the tool (call the inverse/unregister
function or remove the entry from server state for the tool returned by
registerAppTool) and then rethrow or surface a clear combined error; ensure you
reference registerAppTool, registerAppResource, server, the tool returned by
registerAppTool, and ui.resourceUri when implementing the rollback so the
tool+UI registration is atomic from the caller's perspective.
| setUiEnabled() { | ||
| // The hosted example server always advertises its app tool and relies on | ||
| // the host to ignore UI metadata when unsupported. Keep the method for | ||
| // compatibility with existing call sites. | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm setUiEnabled has no other implementations and inspect all call sites.
rg -nP -C3 '\bsetUiEnabled\b'
rg -nP -C3 '\bisUiEnabled\b'Repository: MCPJam/inspector
Length of output: 2563
🏁 Script executed:
sed -n '66,80p' mcp/src/server.tsRepository: MCPJam/inspector
Length of output: 268
🏁 Script executed:
rg -nP 'registerAppTool|registerAppResource' mcp/src/tools/sessionToolRegistrar.ts -A5 -B2Repository: MCPJam/inspector
Length of output: 582
setUiEnabled() is a no-op — capability detection in server.ts serves no purpose.
The method's comment is explicit: always advertise the app tool and let the host ignore UI metadata when unsupported. However, this means registrar.setUiEnabled(isUiEnabled(...)) does nothing. The capability check runs but its result is discarded, leaving dead code in server.ts that implies feature-flagged behavior without delivering it.
If capability-based gating was a PR objective, either remove the dead plumbing or implement real state tracking. Otherwise, the current posture—always advertise, host filters—is legitimate but should not masquerade as gated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcp/src/tools/sessionToolRegistrar.ts` around lines 104 - 108, The
setUiEnabled() method currently does nothing, so calls like
registrar.setUiEnabled(isUiEnabled(...)) are dead; either remove the dead
plumbing (delete setUiEnabled() and all registrar.setUiEnabled(...) calls and
leave advertising behavior unchanged) or implement real state tracking in the
SessionToolRegistrar class (add a private uiEnabled boolean, implement
setUiEnabled(ui: boolean) to store it, and make the code that advertises the app
tool consult this.uiEnabled so the isUiEnabled(...) result actually gates
advertising). Use the existing symbol names setUiEnabled,
registrar.setUiEnabled, and isUiEnabled to locate and update the code.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95c87d7772
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| key: string | ||
| ): string | undefined { | ||
| const value = record?.[key]; | ||
| return typeof value === "string" && value.length > 0 ? value : undefined; |
There was a problem hiding this comment.
getStringValue only returns values when the field is a non-empty string, but Convex system/user timestamps like _creationTime are numeric. In the current flow, createdAt falls back through getStringValue(userRecord, "_creationTime"), so valid numeric timestamps are treated as missing and the UI shows "Not provided" even when the payload contains creation time data.
Useful? React with 👍 / 👎.
1 participant
Summary
whoamiand wire it into the tool responseTesting
Note
Medium Risk
Adds a new Vite/React build step that is now required for dev/deploy/typecheck, so misconfigurations could break CI/builds or runtime asset availability. The new HTML-to-TS bundling script relies on
distoutput paths and could fail if the UI build output structure changes.Overview
Introduces a UI build pipeline for the
@mcpjam/mcpCloudflare Worker package: newvite-based scripts are added anddev/deploy/start/typechecknow runbuild:uifirst.Adds
scripts/bundle-mcp-app-html.mjs(and a.d.tsshim) to convert built UI HTML fromdist/into a generated TS module (src/generated/McpAppsHtml.bundled.ts), exposed both as a CLI script and a Vite plugin hook.Updates dependencies/devDependencies to support this (
react/react-dom,@modelcontextprotocol/ext-apps,vite,@vitejs/plugin-react,vite-plugin-singlefile, and React type packages).Reviewed by Cursor Bugbot for commit 95c87d7. Bugbot is set up for automated code reviews on this repo. Configure here.