Repository navigation
Replace calculator MCP app with generic generated UI shell - #29
Merged
kentcdodds merged 5 commits intoMar 24, 2026
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR replaces the calculator widget with a generic "generated UI shell" architecture. It removes the Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Shell as Generated UI Shell
participant HostBridge as Host Bridge
participant MCP as MCP Server
participant DB as Database
rect rgba(100, 200, 150, 0.5)
Note over Browser,DB: Inline Code Rendering Flow
Browser->>Shell: Initialize with render data
Shell->>HostBridge: initialize()
HostBridge->>MCP: ui/initialize
MCP-->>HostBridge: hostContext, initial render data
HostBridge->>Shell: onRenderData callback
Shell->>Shell: renderEnvelope(inline_code)
Shell->>Browser: Generate iframe with inline code
Browser->>Browser: Execute embedded code in sandbox
end
rect rgba(150, 150, 200, 0.5)
Note over Browser,DB: Saved App Rendering Flow
Browser->>Shell: renderEnvelope(saved_app, appId)
Shell->>HostBridge: callTool(ui_load_app_source)
HostBridge->>MCP: tools/call ui_load_app_source
MCP->>DB: getUiArtifactById(userId, appId)
DB-->>MCP: artifact source code
MCP-->>HostBridge: structured content with code
HostBridge-->>Shell: artifact source
Shell->>Browser: Generate iframe with saved source
end
rect rgba(200, 150, 100, 0.5)
Note over Browser,DB: App Saving Flow
Browser->>HostBridge: sendMessage to host
HostBridge->>MCP: Send message to agent
MCP->>MCP: execute.codemode.ui_save_app()
MCP->>DB: insertUiArtifact(userId, metadata, code)
MCP->>MCP: upsertUiArtifactVector(appId, embedText)
DB-->>MCP: artifact persisted with id
MCP-->>HostBridge: artifact saved response
HostBridge-->>Browser: UI artifact ready message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
kentcdodds
marked this pull request as ready for review
March 24, 2026 20:43
Contributor
|
🔎 Preview deployed: https://kody-pr-29.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/mcp/tools/search.ts (1)
43-50:⚠️ Potential issue | 🟡 MinorClarify auth requirement for saved apps in the description.
Line 50 says only skills require authentication, but Lines 106-109 also gate apps on authenticated user context. This can mislead callers about unauthenticated search behavior.
✏️ Suggested wording tweak
-Pass a **query** string describing what you want to do. Results are ranked with semantic (Vectorize) and lexical fusion. **Skills** require an authenticated MCP user. +Pass a **query** string describing what you want to do. Results are ranked with semantic (Vectorize) and lexical fusion. **Skills and saved apps** require an authenticated MCP user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/search.ts` around lines 43 - 50, Update the user-facing description in packages/worker/src/mcp/tools/search.ts so it accurately reflects that both saved "skills" and saved "apps" require an authenticated MCP user (not just skills), i.e., change the sentence that currently reads "Skills require an authenticated MCP user" to mention "Skills and apps require an authenticated MCP user" and ensure this wording aligns with the gating logic used elsewhere in this module (e.g., the behavior around open_generated_ui and meta_run_skill/meta_get_skill) so callers are not misled about unauthenticated search behavior.
🧹 Nitpick comments (10)
packages/worker/migrations/0006-ui-artifacts.sql (1)
14-14: Consider a composite index for user-scoped listing performance.If list/reopen queries sort by recency, indexing
(user_id, updated_at)will scale better than filtering byuser_idthen sorting.Suggested migration adjustment
-CREATE INDEX IF NOT EXISTS idx_ui_artifacts_user_id ON ui_artifacts(user_id); +CREATE INDEX IF NOT EXISTS idx_ui_artifacts_user_id_updated_at +ON ui_artifacts(user_id, updated_at DESC);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/migrations/0006-ui-artifacts.sql` at line 14, The current single-column index idx_ui_artifacts_user_id on table ui_artifacts should be replaced or supplemented with a composite index on (user_id, updated_at) to improve user-scoped list/reopen queries that sort by recency; update the migration to create a composite index (e.g., idx_ui_artifacts_user_id_updated_at) on ui_artifacts(user_id, updated_at) and remove or keep the single-column index as appropriate for other query patterns so that ORDER BY updated_at FILTER BY user_id can use an index scan.e2e/chat.spec.ts (1)
126-137: Extract the tool-command literal to avoid drift in this test.The exact command string is duplicated; a local constant keeps the fill/assertion in sync when syntax changes.
♻️ Suggested refactor
+ const toolCommand = + 'tool:open_generated_ui;code=<main><h1>Mock App</h1><p>Opened from chat.</p></main>;title=Mock App' await page .getByRole('textbox', { name: 'Message' }) - .fill( - 'tool:open_generated_ui;code=<main><h1>Mock App</h1><p>Opened from chat.</p></main>;title=Mock App', - ) + .fill(toolCommand) @@ await expect( messages - .getByText( - 'tool:open_generated_ui;code=<main><h1>Mock App</h1><p>Opened from chat.</p></main>;title=Mock App', - ) + .getByText(toolCommand) .last(), ).toBeVisible()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/chat.spec.ts` around lines 126 - 137, Extract the duplicated command string into a local constant (e.g., toolCommand) and use that constant in the .fill(...) call and in the messages.getByText(...).last() assertion to prevent drift; update the occurrences around the .fill(...) call and the messages.getByText(...) usage in the e2e/chat.spec.ts test so both reference the single symbol (toolCommand) instead of repeating the literal.packages/worker/src/mcp/skills/skill-embed-and-flags.test.ts (1)
12-25: Prefer reusing canonicalui_save_appspec instead of hand-maintained fixture fields.Manually duplicating capability metadata here increases drift risk as the capability contract evolves.
♻️ Suggested approach
+import { capabilityMap } from '#mcp/capabilities/registry.ts' import { type CapabilitySpec } from '#mcp/capabilities/types.ts' @@ const fakeSpecs: Record<string, CapabilitySpec> = { - ui_save_app: { - name: 'ui_save_app', - domain: capabilityDomainNames.apps, - description: 'Save a generated UI artifact.', - keywords: ['app', 'ui', 'artifact'], - readOnly: true, - idempotent: true, - destructive: false, - inputFields: ['title', 'description', 'keywords', 'source'], - requiredInputFields: ['title', 'description', 'keywords', 'source'], - outputFields: ['app_id'], - inputSchema: {}, - }, + ui_save_app: capabilityMap['ui_save_app'], github_rest: { // test-local fixture }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/skills/skill-embed-and-flags.test.ts` around lines 12 - 25, Replace the hand-maintained fakeSpecs.ui_save_app object with a reference to the canonical capability spec used by the system (e.g., import the existing ui_save_app CapabilitySpec or the capabilitySpecs registry and assign fakeSpecs.ui_save_app = ui_save_appSpec); if the test needs to mutate fields, clone the imported spec first to avoid mutating global state. Update references to CapabilitySpec, fakeSpecs, and ui_save_app in the test so the fixture reuses the authoritative spec instead of duplicating fields.e2e/calculator-widget.spec.ts (1)
1-2: Consider renaming the test file to match the new functionality.The file is named
calculator-widget.spec.tsbut now tests the generated UI shell. Consider renaming togenerated-ui-shell.spec.tsfor consistency with the PR's refactoring effort.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/calculator-widget.spec.ts` around lines 1 - 2, The test file name no longer matches its behavior — rename the file from calculator-widget.spec.ts to generated-ui-shell.spec.ts and update any internal references: change the test file path in Playwright configs or test globs, update any import or require paths that reference calculator-widget.spec.ts, and adjust top-level test titles/describe strings inside the file if they still mention "calculator" so they reflect "generated UI shell" for consistency with the refactor.packages/worker/public/dev/generated-ui-shell-test.html (1)
299-319: Consider handling unrecognizedtools/callrequests.The
tools/callhandler only responds toui_load_app_source. Other tool names will fall through without a response, potentially causing the bridge to hang waiting for a reply. For a more robust test harness, consider returning an error for unrecognized tools.♻️ Proposed fix to handle unrecognized tools
if (message.method === 'tools/call') { if (message.params?.name === 'ui_load_app_source') { event.source.postMessage( { jsonrpc: '2.0', id: message.id, result: { structuredContent: { app_id: 'demo-app', title: 'Loaded app', description: 'Loaded from app-only tool.', runtime: 'javascript', code: demoCode, }, }, }, '*', ) return } + event.source.postMessage( + { + jsonrpc: '2.0', + id: message.id, + error: { + code: -32601, + message: `Unknown tool: ${message.params?.name}`, + }, + }, + '*', + ) + return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/public/dev/generated-ui-shell-test.html` around lines 299 - 319, The tools/call branch currently only handles message.params?.name === 'ui_load_app_source' and returns a result, leaving other tools unhandled which can leave the bridge waiting; inside the same if (message.method === 'tools/call') block add an else (or default) branch that posts back an error response via event.source.postMessage with the same jsonrpc version and message.id, including an error object (code and message) indicating "unrecognized tool" or the tool name (message.params?.name) so callers receive a proper error instead of hanging.packages/worker/src/mcp/capabilities/apps/ui-load-app-source.ts (1)
6-6: Inconsistent import path style.This file uses a relative import
'../meta/require-user.ts'while the sibling fileui-get-app.tsuses the alias'#mcp/capabilities/meta/require-user.ts'. Consider using the alias consistently across the capability modules.♻️ Proposed fix
-import { requireMcpUser } from '../meta/require-user.ts' +import { requireMcpUser } from '#mcp/capabilities/meta/require-user.ts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/apps/ui-load-app-source.ts` at line 6, The import in ui-load-app-source.ts uses a relative path for requireMcpUser; update the import to use the same project alias as its sibling (e.g., '#mcp/capabilities/meta/require-user.ts') so requireMcpUser is imported consistently across capability modules; locate the requireMcpUser import statement in ui-load-app-source.ts and replace the relative path with the alias.packages/worker/src/mcp/tools/open-generated-ui.ts (1)
68-71: Unnecessaryvoid agentstatement.Line 69 uses
void agentto suppress unused variable warnings, butagentis actually used on line 71 (agent.server). This statement can be removed.♻️ Proposed fix
export async function registerOpenGeneratedUiTool(agent: MCP) { - void agent registerAppTool( agent.server,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/open-generated-ui.ts` around lines 68 - 71, Remove the redundant "void agent" statement in registerOpenGeneratedUiTool — the parameter agent is used (see agent.server passed to registerAppTool), so simply delete the standalone void agent line in the registerOpenGeneratedUiTool function to eliminate the unnecessary no-op. Ensure the function still accepts the MCP parameter and calls registerAppTool(agent.server, ...) as before.packages/worker/src/mcp/ui-artifacts-vectorize.ts (1)
24-31: Inconsistent offline mode handling between upsert and delete.
upsertUiArtifactVectorchecksisCapabilitySearchOffline(env)before proceeding (line 13), butdeleteUiArtifactVectordoes not. If the system is offline, upsert silently skips, but delete would still attempt the operation and could fail.Consider adding the offline check for consistency:
♻️ Proposed fix
export async function deleteUiArtifactVector( env: Env, appId: string, ): Promise<void> { const index = getCapabilityVectorIndex(env) - if (!index) return + if (!index || isCapabilitySearchOffline(env)) return await index.deleteByIds([uiArtifactVectorId(appId)]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/ui-artifacts-vectorize.ts` around lines 24 - 31, The deleteUiArtifactVector function is missing the same offline guard used by upsertUiArtifactVector; add a check using isCapabilitySearchOffline(env) at the top of deleteUiArtifactVector and return early if true, so deletion only runs when capability search is online. Keep the existing logic that resolves the index via getCapabilityVectorIndex(env) and calls index.deleteByIds([uiArtifactVectorId(appId)]) unchanged; this ensures consistent offline handling between upsertUiArtifactVector and deleteUiArtifactVector.packages/worker/src/mcp/capabilities/apps/ui-get-app.ts (1)
26-26: Inconsistent runtime schema between capabilities.
ui_get_appdefinesruntime: z.string()(line 26) whileui_load_app_sourceusesruntime: z.enum(['javascript'])(line 12 in that file). This inconsistency meansui_get_appcould return arbitrary runtime strings whileui_load_app_sourcevalidates against the enum.Consider aligning the schemas for consistency:
♻️ Proposed fix
- runtime: z.string(), + runtime: z.enum(['javascript']),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/apps/ui-get-app.ts` at line 26, The ui_get_app capability defines runtime as z.string() which is inconsistent with ui_load_app_source's runtime: z.enum(['javascript']); update ui_get_app's schema to match by using the same enum (e.g., replace runtime: z.string() with runtime: z.enum(['javascript'])) or extract a shared constant like RUNTIME_SCHEMA = z.enum(['javascript']) and use it in both ui_get_app and ui_load_app_source so they validate the same runtime values; adjust any imports/exports accordingly.packages/worker/client/mcp-apps/generated-ui-shell.ts (1)
159-166:toggleFullscreenalways returns'fullscreen'regardless of actual toggle direction.The
kodyWidget.toggleFullscreen()method returns'fullscreen'unconditionally, even when the user might be toggling from fullscreen back to inline. This could be misleading to consumers relying on the return value.Consider returning the requested mode or awaiting the actual result from the parent:
♻️ Potential improvement
async toggleFullscreen() { if (window.parent === window) return 'inline' + const currentMode = document.documentElement.dataset.displayMode || 'inline' + const nextMode = currentMode === 'fullscreen' ? 'inline' : 'fullscreen' window.parent.postMessage({ type: '${childMessagePrefix}toggle-fullscreen', payload: {}, }, '*') - return 'fullscreen' + return nextMode },Alternatively, if the actual resolved mode matters, consider making this a proper async request-response pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/client/mcp-apps/generated-ui-shell.ts` around lines 159 - 166, toggleFullscreen currently always returns 'fullscreen' which is misleading; update the method (toggleFullscreen) to return the actual requested mode or the parent's response instead of a hardcoded value: either add an input (e.g., mode: 'fullscreen'|'inline') and post that in the payload and return that mode, or implement a request-response pattern using postMessage with a correlation id (use childMessagePrefix in the message type) and await the parent's reply to return the resolved mode; ensure window.parent === window still returns 'inline' when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/client/mcp-apps/generated-ui-shell.ts`:
- Line 26: Remove the unused constant inlineScriptMimeType by deleting its
declaration (const inlineScriptMimeType = 'text/javascript') from the
generated-ui-shell.ts file; verify there are no remaining references to
inlineScriptMimeType in functions or templates (e.g., any inline script handling
logic) and run the build/tests to ensure no regressions after removal.
In `@packages/worker/src/mcp/observability.test.ts`:
- Around line 147-152: Update the test mock metadata that currently uses the
obsolete input name "source" to the canonical "code" so it matches the
ui_save_app capability; specifically, in the mock objects where inputFields and
requiredInputFields list "source" (in skill-embed-and-flags.test), replace those
entries with "code" to align with the inputSchema/handler in ui_save_app and the
observability test which already uses code.
In `@packages/worker/src/mcp/ui-artifacts-repo.ts`:
- Around line 130-143: The mapRow function currently force-casts
row['source_type'] to UiArtifactRow['runtime'] which can produce invalid runtime
values; update mapRow to validate row['source_type'] against the allowed runtime
union (e.g., via an isValidRuntime(value) helper or a Set/array of permitted
runtime strings) and only assign it when valid, otherwise use a safe fallback
(null/default) or throw an explicit error; reference the runtime assignment in
mapRow and add the validation helper near that function so unexpected DB values
are handled safely.
---
Outside diff comments:
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 43-50: Update the user-facing description in
packages/worker/src/mcp/tools/search.ts so it accurately reflects that both
saved "skills" and saved "apps" require an authenticated MCP user (not just
skills), i.e., change the sentence that currently reads "Skills require an
authenticated MCP user" to mention "Skills and apps require an authenticated MCP
user" and ensure this wording aligns with the gating logic used elsewhere in
this module (e.g., the behavior around open_generated_ui and
meta_run_skill/meta_get_skill) so callers are not misled about unauthenticated
search behavior.
---
Nitpick comments:
In `@e2e/calculator-widget.spec.ts`:
- Around line 1-2: The test file name no longer matches its behavior — rename
the file from calculator-widget.spec.ts to generated-ui-shell.spec.ts and update
any internal references: change the test file path in Playwright configs or test
globs, update any import or require paths that reference
calculator-widget.spec.ts, and adjust top-level test titles/describe strings
inside the file if they still mention "calculator" so they reflect "generated UI
shell" for consistency with the refactor.
In `@e2e/chat.spec.ts`:
- Around line 126-137: Extract the duplicated command string into a local
constant (e.g., toolCommand) and use that constant in the .fill(...) call and in
the messages.getByText(...).last() assertion to prevent drift; update the
occurrences around the .fill(...) call and the messages.getByText(...) usage in
the e2e/chat.spec.ts test so both reference the single symbol (toolCommand)
instead of repeating the literal.
In `@packages/worker/client/mcp-apps/generated-ui-shell.ts`:
- Around line 159-166: toggleFullscreen currently always returns 'fullscreen'
which is misleading; update the method (toggleFullscreen) to return the actual
requested mode or the parent's response instead of a hardcoded value: either add
an input (e.g., mode: 'fullscreen'|'inline') and post that in the payload and
return that mode, or implement a request-response pattern using postMessage with
a correlation id (use childMessagePrefix in the message type) and await the
parent's reply to return the resolved mode; ensure window.parent === window
still returns 'inline' when appropriate.
In `@packages/worker/migrations/0006-ui-artifacts.sql`:
- Line 14: The current single-column index idx_ui_artifacts_user_id on table
ui_artifacts should be replaced or supplemented with a composite index on
(user_id, updated_at) to improve user-scoped list/reopen queries that sort by
recency; update the migration to create a composite index (e.g.,
idx_ui_artifacts_user_id_updated_at) on ui_artifacts(user_id, updated_at) and
remove or keep the single-column index as appropriate for other query patterns
so that ORDER BY updated_at FILTER BY user_id can use an index scan.
In `@packages/worker/public/dev/generated-ui-shell-test.html`:
- Around line 299-319: The tools/call branch currently only handles
message.params?.name === 'ui_load_app_source' and returns a result, leaving
other tools unhandled which can leave the bridge waiting; inside the same if
(message.method === 'tools/call') block add an else (or default) branch that
posts back an error response via event.source.postMessage with the same jsonrpc
version and message.id, including an error object (code and message) indicating
"unrecognized tool" or the tool name (message.params?.name) so callers receive a
proper error instead of hanging.
In `@packages/worker/src/mcp/capabilities/apps/ui-get-app.ts`:
- Line 26: The ui_get_app capability defines runtime as z.string() which is
inconsistent with ui_load_app_source's runtime: z.enum(['javascript']); update
ui_get_app's schema to match by using the same enum (e.g., replace runtime:
z.string() with runtime: z.enum(['javascript'])) or extract a shared constant
like RUNTIME_SCHEMA = z.enum(['javascript']) and use it in both ui_get_app and
ui_load_app_source so they validate the same runtime values; adjust any
imports/exports accordingly.
In `@packages/worker/src/mcp/capabilities/apps/ui-load-app-source.ts`:
- Line 6: The import in ui-load-app-source.ts uses a relative path for
requireMcpUser; update the import to use the same project alias as its sibling
(e.g., '#mcp/capabilities/meta/require-user.ts') so requireMcpUser is imported
consistently across capability modules; locate the requireMcpUser import
statement in ui-load-app-source.ts and replace the relative path with the alias.
In `@packages/worker/src/mcp/skills/skill-embed-and-flags.test.ts`:
- Around line 12-25: Replace the hand-maintained fakeSpecs.ui_save_app object
with a reference to the canonical capability spec used by the system (e.g.,
import the existing ui_save_app CapabilitySpec or the capabilitySpecs registry
and assign fakeSpecs.ui_save_app = ui_save_appSpec); if the test needs to mutate
fields, clone the imported spec first to avoid mutating global state. Update
references to CapabilitySpec, fakeSpecs, and ui_save_app in the test so the
fixture reuses the authoritative spec instead of duplicating fields.
In `@packages/worker/src/mcp/tools/open-generated-ui.ts`:
- Around line 68-71: Remove the redundant "void agent" statement in
registerOpenGeneratedUiTool — the parameter agent is used (see agent.server
passed to registerAppTool), so simply delete the standalone void agent line in
the registerOpenGeneratedUiTool function to eliminate the unnecessary no-op.
Ensure the function still accepts the MCP parameter and calls
registerAppTool(agent.server, ...) as before.
In `@packages/worker/src/mcp/ui-artifacts-vectorize.ts`:
- Around line 24-31: The deleteUiArtifactVector function is missing the same
offline guard used by upsertUiArtifactVector; add a check using
isCapabilitySearchOffline(env) at the top of deleteUiArtifactVector and return
early if true, so deletion only runs when capability search is online. Keep the
existing logic that resolves the index via getCapabilityVectorIndex(env) and
calls index.deleteByIds([uiArtifactVectorId(appId)]) unchanged; this ensures
consistent offline handling between upsertUiArtifactVector and
deleteUiArtifactVector.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 453fb428-3296-455a-b3f1-a5b06cab1c54
📒 Files selected for processing (51)
docs/agents/mcp-apps-starter-guide.mddocs/mcp-server-patterns.mde2e/calculator-widget.spec.tse2e/chat.spec.tspackage.jsonpackages/shared/src/mock-ai.test.tspackages/shared/src/mock-ai.tspackages/worker/client/mcp-apps/calculator-widget.tspackages/worker/client/mcp-apps/generated-ui-shell.tspackages/worker/client/mcp-apps/widget-host-bridge.test.tspackages/worker/client/mcp-apps/widget-host-bridge.tspackages/worker/migrations/0006-ui-artifacts.sqlpackages/worker/public/dev/generated-ui-shell-test.htmlpackages/worker/src/ai-runtime.test.tspackages/worker/src/chat-agent.tspackages/worker/src/index.tspackages/worker/src/mcp/apps/calculator-ui-entry-point.tspackages/worker/src/mcp/apps/generated-ui-shell-entry-point.tspackages/worker/src/mcp/capabilities/apps/domain.tspackages/worker/src/mcp/capabilities/apps/index.tspackages/worker/src/mcp/capabilities/apps/ui-get-app.tspackages/worker/src/mcp/capabilities/apps/ui-list-apps.tspackages/worker/src/mcp/capabilities/apps/ui-load-app-source.tspackages/worker/src/mcp/capabilities/apps/ui-save-app.tspackages/worker/src/mcp/capabilities/build-capability-registry.test.tspackages/worker/src/mcp/capabilities/builtin-domains.tspackages/worker/src/mcp/capabilities/capability-search.test.tspackages/worker/src/mcp/capabilities/domain-metadata.tspackages/worker/src/mcp/capabilities/math/do-math.tspackages/worker/src/mcp/capabilities/math/domain.tspackages/worker/src/mcp/capabilities/math/index.tspackages/worker/src/mcp/capabilities/unified-search.test.tspackages/worker/src/mcp/capabilities/unified-search.tspackages/worker/src/mcp/index.tspackages/worker/src/mcp/mcp-server-e2e.test.tspackages/worker/src/mcp/observability.test.tspackages/worker/src/mcp/observability.tspackages/worker/src/mcp/register-resources.tspackages/worker/src/mcp/register-tools.tspackages/worker/src/mcp/resources/generated-ui-app-resource.tspackages/worker/src/mcp/skills/infer-codemode-capabilities.test.tspackages/worker/src/mcp/skills/skill-embed-and-flags.test.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/mcp/tools/open-calculator-ui.tspackages/worker/src/mcp/tools/open-generated-ui.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/mcp/ui-artifacts-embed.tspackages/worker/src/mcp/ui-artifacts-repo.tspackages/worker/src/mcp/ui-artifacts-search.tspackages/worker/src/mcp/ui-artifacts-types.tspackages/worker/src/mcp/ui-artifacts-vectorize.ts
💤 Files with no reviewable changes (6)
- packages/worker/src/mcp/capabilities/math/domain.ts
- packages/worker/src/mcp/capabilities/math/index.ts
- packages/worker/src/mcp/apps/calculator-ui-entry-point.ts
- packages/worker/src/mcp/tools/open-calculator-ui.ts
- packages/worker/src/mcp/capabilities/math/do-math.ts
- packages/worker/client/mcp-apps/calculator-widget.ts
Comment on lines
+130
to
+143
| function mapRow(row: Record<string, unknown>): UiArtifactRow { | ||
| return { | ||
| id: String(row['id']), | ||
| user_id: String(row['user_id']), | ||
| title: String(row['title']), | ||
| description: String(row['description']), | ||
| keywords: String(row['keywords']), | ||
| code: String(row['source_code']), | ||
| runtime: String(row['source_type']) as UiArtifactRow['runtime'], | ||
| search_text: row['search_text'] == null ? null : String(row['search_text']), | ||
| created_at: String(row['created_at']), | ||
| updated_at: String(row['updated_at']), | ||
| } | ||
| } |
There was a problem hiding this comment.
Unsafe type cast for runtime field.
Line 138 casts the database value directly to UiArtifactRow['runtime'] without validating that the value is actually a valid member of the union type. If the database contains an unexpected value, this will silently produce an invalid typed object.
🛡️ Proposed fix with validation
+const VALID_RUNTIMES = ['browser', 'node'] as const
+type ValidRuntime = (typeof VALID_RUNTIMES)[number]
+
+function parseRuntime(value: unknown): UiArtifactRow['runtime'] {
+ const str = String(value)
+ return VALID_RUNTIMES.includes(str as ValidRuntime)
+ ? (str as ValidRuntime)
+ : 'browser' // or throw if strict validation is preferred
+}
+
function mapRow(row: Record<string, unknown>): UiArtifactRow {
return {
id: String(row['id']),
user_id: String(row['user_id']),
title: String(row['title']),
description: String(row['description']),
keywords: String(row['keywords']),
code: String(row['source_code']),
- runtime: String(row['source_type']) as UiArtifactRow['runtime'],
+ runtime: parseRuntime(row['source_type']),
search_text: row['search_text'] == null ? null : String(row['search_text']),
created_at: String(row['created_at']),
updated_at: String(row['updated_at']),
}
}📝 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.
Suggested change
| function mapRow(row: Record<string, unknown>): UiArtifactRow { | |
| return { | |
| id: String(row['id']), | |
| user_id: String(row['user_id']), | |
| title: String(row['title']), | |
| description: String(row['description']), | |
| keywords: String(row['keywords']), | |
| code: String(row['source_code']), | |
| runtime: String(row['source_type']) as UiArtifactRow['runtime'], | |
| search_text: row['search_text'] == null ? null : String(row['search_text']), | |
| created_at: String(row['created_at']), | |
| updated_at: String(row['updated_at']), | |
| } | |
| } | |
| const VALID_RUNTIMES = ['browser', 'node'] as const | |
| type ValidRuntime = (typeof VALID_RUNTIMES)[number] | |
| function parseRuntime(value: unknown): UiArtifactRow['runtime'] { | |
| const str = String(value) | |
| return VALID_RUNTIMES.includes(str as ValidRuntime) | |
| ? (str as ValidRuntime) | |
| : 'browser' // or throw if strict validation is preferred | |
| } | |
| function mapRow(row: Record<string, unknown>): UiArtifactRow { | |
| return { | |
| id: String(row['id']), | |
| user_id: String(row['user_id']), | |
| title: String(row['title']), | |
| description: String(row['description']), | |
| keywords: String(row['keywords']), | |
| code: String(row['source_code']), | |
| runtime: parseRuntime(row['source_type']), | |
| search_text: row['search_text'] == null ? null : String(row['search_text']), | |
| created_at: String(row['created_at']), | |
| updated_at: String(row['updated_at']), | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/ui-artifacts-repo.ts` around lines 130 - 143, The
mapRow function currently force-casts row['source_type'] to
UiArtifactRow['runtime'] which can produce invalid runtime values; update mapRow
to validate row['source_type'] against the allowed runtime union (e.g., via an
isValidRuntime(value) helper or a Set/array of permitted runtime strings) and
only assign it when valid, otherwise use a safe fallback (null/default) or throw
an explicit error; reference the runtime assignment in mapRow and add the
validation helper near that function so unexpected DB values are handled safely.
| }) | ||
|
|
||
| export async function registerOpenGeneratedUiTool(agent: MCP) { | ||
| void agent |
Contributor
There was a problem hiding this comment.
Redundant no-op void agent statement
Low Severity
void agent on line 69 is a no-op expression — agent is already referenced on the very next line via agent.server. This looks like a leftover debugging artifact or an unused-variable suppression that is no longer needed. The deleted open-calculator-ui.ts had no equivalent line.
| } catch { | ||
| return [] | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicated parseStringArray across multiple new files
Low Severity
The parseStringArray function is identically defined in both ui-get-app.ts and ui-list-apps.ts. An equivalent function named parseJsonStringArray also exists in ui-artifacts-search.ts and unified-search.ts. All four are new additions with the same logic. A shared utility would reduce duplication and ensure consistent bug fixes.
Additional Locations (1)
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/mock-servers/resend/src/mock-messages-do.ts (1)
81-86:getMessagewill returnnullfor older messages after truncation.Since
getMessagesearches onlyrecentMessages(capped at 100), any message that gets pushed out due to truncation will become unretrievable, returningnull(which the worker surfaces as 404). This differs from the previous D1 behavior where messages persisted indefinitely.Given the "best-effort" semantics documented in
docs/agents/mock-api-servers.md, this may be acceptable for a mock server. However, consider adding a comment or updating the/__mocks/messages/:idendpoint description to clarify this limitation.📝 Suggested documentation improvement in worker.ts dashboardEndpoints
{ method: 'GET', path: '/__mocks/messages/:id', - description: 'Get a stored message by ID (JSON)', + description: 'Get a stored message by ID (JSON, limited to most recent 100)', requiresAuth: true, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mock-servers/resend/src/mock-messages-do.ts` around lines 81 - 86, The current 'get' case in mock-messages-do.ts (the logic that implements getMessage) only searches storage.recentMessages (a 100-item ring) so messages older than the truncation are returned as null; update the code comments and the API docs to make this best-effort limitation explicit: add a clear inline comment beside the case 'get' block referencing storage.recentMessages and noting that messages are capped at 100 and older messages are unretrievable, and update the /__mocks/messages/:id entry in docs/agents/mock-api-servers.md (or the dashboardEndpoints description in worker.ts) to document that getMessage is best-effort and may return 404 for truncated messages.packages/worker/src/mcp/capabilities/apps/ui-get-app.ts (1)
56-58: Minor inconsistency in error message.This capability uses
'Saved UI artifact not found for this user.'whileui-load-app-source.tsuses'Saved app not found for this user.'. Consider aligning the wording for consistent error responses across similar capabilities.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/apps/ui-get-app.ts` around lines 56 - 58, The error message thrown when no saved UI artifact is found ("Saved UI artifact not found for this user.") should be made consistent with the other capability; update the thrown Error in the row check inside the handler in ui-get-app.ts to use the same wording as ui-load-app-source.ts ("Saved app not found for this user.") so both capabilities return the identical message when a saved app is missing.packages/worker/src/mcp/ui-artifacts-search.ts (2)
15-23: Consider extracting shared JSON array parsing utility.
parseJsonStringArrayis nearly identical toparseStringArrayinui-get-app.ts. Consider extracting this to a shared utility (e.g., inui-artifacts-types.tsor a common utils module) to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/ui-artifacts-search.ts` around lines 15 - 23, The parseJsonStringArray function duplicates parseStringArray from ui-get-app.ts; extract a shared utility (e.g., export a new function parseStringArrayFromJson or reuse a common parseStringArray) into a shared module such as ui-artifacts-types.ts or a common utils file, update both mcp/ui-artifacts-search.ts (replace parseJsonStringArray usage) and ui-get-app.ts (replace its local parseStringArray) to import and call the shared utility, and ensure the exported function preserves the current behavior (JSON.parse with Array.isArray check and string filtering) and retains the same TypeScript signature (returns Array<string>).
128-131: Non-null assertion assumes vector index availability when not offline.Line 129 uses
!assuminggetCapabilityVectorIndexreturns non-null when not offline. If the relationship betweenisCapabilitySearchOfflineand index availability changes, this could cause a runtime error.🛡️ Proposed defensive fix
} else { const index = getCapabilityVectorIndex(input.env)! + if (!index) { + // Fallback to lexical-only if index unexpectedly unavailable + vectorOrder = ids + } else { const queryVector = await embedTextForVectorize(input.env, query) const topK = Math.min(Math.max(ids.length, input.limit * 5), 100) + // ... rest of vector logic + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/ui-artifacts-search.ts` around lines 128 - 131, The code currently uses a non-null assertion on getCapabilityVectorIndex(input.env) which can crash if the index is absent; update the block around getCapabilityVectorIndex, embedTextForVectorize and subsequent logic to defensively handle a null/undefined index: call getCapabilityVectorIndex(input.env) into a local (e.g., capabilityIndex), check if it is null/undefined and if so either use the offline fallback path (respecting isCapabilitySearchOffline) or return/throw a clear error/early-exit with a logged message; only call embedTextForVectorize and proceed to compute topK and run the vector search when capabilityIndex is present. Ensure you reference getCapabilityVectorIndex and embedTextForVectorize when modifying control flow.packages/worker/src/mcp/ui-artifacts-repo.ts (1)
54-88: Remove or export unused functions.
updateUiArtifactandlistAllUiArtifactsare defined but never referenced anywhere in the codebase. Remove them if no longer needed, or export them if intended for future use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/ui-artifacts-repo.ts` around lines 54 - 88, The functions updateUiArtifact and listAllUiArtifacts are defined but unused; either remove their declarations to clean up dead code or export them so they can be consumed elsewhere—if exporting, add them to the module's exports (e.g., export async function updateUiArtifact(...) and export function listAllUiArtifacts(...)) and update any barrel/index export files as needed so other modules can import them; if removing, delete both function definitions and any related types/params (e.g., the fields param and the D1Database usage) and run a repo-wide search to ensure there are no remaining references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/mock-servers/resend/src/mock-messages-do.ts`:
- Around line 81-86: The current 'get' case in mock-messages-do.ts (the logic
that implements getMessage) only searches storage.recentMessages (a 100-item
ring) so messages older than the truncation are returned as null; update the
code comments and the API docs to make this best-effort limitation explicit: add
a clear inline comment beside the case 'get' block referencing
storage.recentMessages and noting that messages are capped at 100 and older
messages are unretrievable, and update the /__mocks/messages/:id entry in
docs/agents/mock-api-servers.md (or the dashboardEndpoints description in
worker.ts) to document that getMessage is best-effort and may return 404 for
truncated messages.
In `@packages/worker/src/mcp/capabilities/apps/ui-get-app.ts`:
- Around line 56-58: The error message thrown when no saved UI artifact is found
("Saved UI artifact not found for this user.") should be made consistent with
the other capability; update the thrown Error in the row check inside the
handler in ui-get-app.ts to use the same wording as ui-load-app-source.ts
("Saved app not found for this user.") so both capabilities return the identical
message when a saved app is missing.
In `@packages/worker/src/mcp/ui-artifacts-repo.ts`:
- Around line 54-88: The functions updateUiArtifact and listAllUiArtifacts are
defined but unused; either remove their declarations to clean up dead code or
export them so they can be consumed elsewhere—if exporting, add them to the
module's exports (e.g., export async function updateUiArtifact(...) and export
function listAllUiArtifacts(...)) and update any barrel/index export files as
needed so other modules can import them; if removing, delete both function
definitions and any related types/params (e.g., the fields param and the
D1Database usage) and run a repo-wide search to ensure there are no remaining
references.
In `@packages/worker/src/mcp/ui-artifacts-search.ts`:
- Around line 15-23: The parseJsonStringArray function duplicates
parseStringArray from ui-get-app.ts; extract a shared utility (e.g., export a
new function parseStringArrayFromJson or reuse a common parseStringArray) into a
shared module such as ui-artifacts-types.ts or a common utils file, update both
mcp/ui-artifacts-search.ts (replace parseJsonStringArray usage) and
ui-get-app.ts (replace its local parseStringArray) to import and call the shared
utility, and ensure the exported function preserves the current behavior
(JSON.parse with Array.isArray check and string filtering) and retains the same
TypeScript signature (returns Array<string>).
- Around line 128-131: The code currently uses a non-null assertion on
getCapabilityVectorIndex(input.env) which can crash if the index is absent;
update the block around getCapabilityVectorIndex, embedTextForVectorize and
subsequent logic to defensively handle a null/undefined index: call
getCapabilityVectorIndex(input.env) into a local (e.g., capabilityIndex), check
if it is null/undefined and if so either use the offline fallback path
(respecting isCapabilitySearchOffline) or return/throw a clear error/early-exit
with a logged message; only call embedTextForVectorize and proceed to compute
topK and run the vector search when capabilityIndex is present. Ensure you
reference getCapabilityVectorIndex and embedTextForVectorize when modifying
control flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5242948-b166-4a18-a10e-03f8d918c089
📒 Files selected for processing (27)
docs/agents/d1-legacy-export.mddocs/agents/mcp-apps-starter-guide.mddocs/agents/mock-api-servers.mddocs/agents/setup.mddocs/mcp-server-patterns.mdpackages/mock-servers/ai/src/mock-requests-do.tspackages/mock-servers/ai/src/worker.tspackages/mock-servers/ai/wrangler.jsoncpackages/mock-servers/resend/src/mock-messages-do.tspackages/mock-servers/resend/src/resend-mock.test.tspackages/mock-servers/resend/src/worker.tspackages/mock-servers/resend/wrangler.jsoncpackages/worker/client/mcp-apps/generated-ui-shell.tspackages/worker/client/mcp-apps/widget-host-bridge.test.tspackages/worker/migrations/0001-init.sqlpackages/worker/migrations/0007-drop-mock-request-tables.sqlpackages/worker/src/db.tspackages/worker/src/index.tspackages/worker/src/mcp/capabilities/apps/ui-get-app.tspackages/worker/src/mcp/capabilities/apps/ui-load-app-source.tspackages/worker/src/mcp/capabilities/apps/ui-save-app.tspackages/worker/src/mcp/capabilities/unified-search.tspackages/worker/src/mcp/mcp-server-e2e.test.tspackages/worker/src/mcp/tools/open-generated-ui.tspackages/worker/src/mcp/ui-artifacts-repo.tspackages/worker/src/mcp/ui-artifacts-search.tspackages/worker/src/mcp/ui-artifacts-vectorize.ts
💤 Files with no reviewable changes (2)
- docs/agents/d1-legacy-export.md
- packages/worker/src/db.ts
✅ Files skipped from review due to trivial changes (4)
- packages/worker/migrations/0001-init.sql
- packages/worker/migrations/0007-drop-mock-request-tables.sql
- packages/mock-servers/resend/src/resend-mock.test.ts
- docs/mcp-server-patterns.md
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/worker/src/index.ts
- packages/worker/src/mcp/ui-artifacts-vectorize.ts
- packages/worker/src/mcp/capabilities/apps/ui-load-app-source.ts
- docs/agents/mcp-apps-starter-guide.md
- packages/worker/src/mcp/tools/open-generated-ui.ts
- packages/worker/src/mcp/capabilities/apps/ui-save-app.ts
- packages/worker/client/mcp-apps/widget-host-bridge.test.ts
- packages/worker/client/mcp-apps/generated-ui-shell.ts
- packages/worker/src/mcp/capabilities/unified-search.ts
Co-authored-by: me <me@kentcdodds.com>
Co-authored-by: me <me@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
…ocumentation - Updated `outputSchema` and related types to accept both 'html' and 'javascript' runtimes. - Revised documentation to emphasize the preference for self-contained HTML documents in generated apps. - Added tests for host messaging and tool result notifications to ensure proper integration. - Refactored inline code examples in the generated UI shell for clarity and consistency.
kentcdodds
force-pushed
the
cursor/mcp-app-specification-for-generative-ui-7d68
branch
from
March 24, 2026 22:14
ab8be92 to
d26c8f7
Compare
kentcdodds
deleted the
cursor/mcp-app-specification-for-generative-ui-7d68
branch
March 24, 2026 22:18
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 4 total unresolved issues (including 2 from previous reviews).
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
| await registerSearchTool(agent) | ||
| await registerExecuteTool(agent) | ||
| await registerOpenCalculatorUiTool(agent) | ||
| await registerOpenGeneratedUiTool(agent) |
Contributor
There was a problem hiding this comment.
Saved app source tool never registered
High Severity
The shell reopens saved apps by calling tools/call with ui_load_app_source, but that tool is never registered on the MCP server. Only search, execute, and open_generated_ui are registered, so saved-app reopen fails in real hosts.
Additional Locations (1)
| } | ||
| if (/<html[\s>]/i.test(code)) { | ||
| return code.replace(/<html([\s>])/i, `<html$1><head>${injection}</head>`) | ||
| } |
Contributor
There was a problem hiding this comment.
HTML injection regex corrupts document tags
High Severity
injectIntoHtmlDocument uses regexes that capture the closing > (/<head([\s>]*)/ and /<html([\s>])/), then adds another > in replacement. This can emit malformed tags like <head>> and can strip <html> attributes.
This was referenced Mar 25, 2026
This was referenced Apr 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
/dev/generated-ui-shell-testcovering default iframe render, sandboxed iframe render, host message, open-link logging, and fullscreen mode textbun run inspectremains available for inspector-driven MCP app testingSummary by CodeRabbit
Release Notes
New Features
Chores