Feature/skybridge client #1
Conversation
![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.
ℹ️ 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".
| // Wait for HTML content from parent | ||
| window.addEventListener('message', (event) => { | ||
| if (event.source === window.parent) { | ||
| if (event.data && event.data.type === 'ui-html-content') { | ||
| const payload = event.data.payload || {}; | ||
| const html = payload.html; | ||
| const sandbox = payload.sandbox; | ||
| if (typeof sandbox === 'string') { | ||
| inner.setAttribute('sandbox', sandbox); | ||
| } | ||
| if (typeof html === 'string') { | ||
| inner.srcdoc = html; | ||
| } | ||
| } else { | ||
| if (inner && inner.contentWindow) { | ||
| inner.contentWindow.postMessage(event.data, '*'); | ||
| if (event.source === window.parent && event.data && event.data.type === 'ui-html-content') { | ||
| const payload = event.data.payload || {}; | ||
| const html = payload.html; |
There was a problem hiding this comment.
In raw HTML proxy mode the message listener now only processes parent messages whose type is ui-html-content and otherwise returns, so any subsequent messages sent by the host (e.g., render data or UI action responses) are never relayed to the inner iframe. Previously the proxy forwarded all parent messages to the content window, which is required for postMessage-based interactions. This regression prevents the embedded widget from receiving follow‑up messages after the initial HTML payload.
Useful? React with 👍 / 👎.
| const openaiAPI = { | ||
| toolInput: ${JSON.stringify(toolInput)}, | ||
| toolOutput: ${JSON.stringify(initialRenderData)}, | ||
| toolResponseMetadata: ${JSON.stringify(toolResponseMetadata ?? null)}, |
There was a problem hiding this comment.
The apiScript helper embeds JSON.stringify(toolInput) and related values directly inside a <script> element. If any of those JSON strings contain </script> (or similar HTML), the browser will terminate the script early and treat the rest as markup, allowing injection and breaking the initialization of window.openai. The values should be escaped (e.g., replace </script> with <\/script>) before interpolation into the script block.
Useful? React with 👍 / 👎.
feat: refactor and normalise props
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds MCP and host context propagation to HTML resource rendering, introduces skybridge HTML support, refactors processHTMLResource to accept an options object, adds an OpenAI widget runtime script and bundling, updates components to pass new context, and expands tests and docs to cover these changes. Changes
Sequence Diagram(s)sequenceDiagram
actor UI as UIResourceRenderer
participant HTML as HTMLResourceRenderer
participant Proc as processHTMLResource
participant Iframe as iframe
UI->>HTML: pass resource, mcp, host
HTML->>Proc: processHTMLResource(resource, { proxy, mcp, host })
alt content is skybridge
Proc->>Proc: create OpenAI runtime script via createOpenAiScript
Proc->>Proc: inject runtime into HTML (head/html/prepend)
Proc-->>HTML: return enhanced HTML + renderData
else standard HTML
Proc-->>HTML: return html or src/srcdoc info
end
HTML->>Iframe: render with renderData (iframeRenderData + metadata)
note right of Iframe: toolOutput from mcp is not merged into renderData
sequenceDiagram
participant Window as widget window
participant Runtime as OpenAI widget runtime
participant Parent as window.parent
participant Storage as localStorage
Window->>Runtime: read __MCP_WIDGET_CONFIG__
Runtime->>Runtime: normalize defaults (theme, locale, displayMode, capabilities)
Runtime->>Storage: restore widgetState
Runtime->>Window: attach openai & webplus globals
Runtime->>Parent: postMessage webplus:set_globals
Window->>Runtime: openai.callTool(tool, params)
Runtime->>Parent: postMessage openai:callTool (with requestId)
Parent-->>Runtime: postMessage response
Runtime->>Window: resolve callTool promise
Window->>Runtime: openai.setWidgetState(state)
Runtime->>Storage: persist state
Runtime->>Parent: postMessage openai:setWidgetState
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
sdks/typescript/client/scripts/proxy/index.html (1)
73-86: Restore proxy forwarding for parent messages.The message listener now only processes parent messages whose
typeisui-html-content(line 74) and returns early for all other message types. This means subsequent messages sent by the host (e.g., render data viaui-lifecycle-iframe-render-data, or UI action responses viaui-message-response) are never relayed to the inner iframe. The proxy must forward all non-ui-html-contentparent messages to the content window to support postMessage-based interactions.Apply this diff to forward other parent messages to the inner iframe:
window.addEventListener('message', (event) => { if (event.source === window.parent && event.data && event.data.type === 'ui-html-content') { const payload = event.data.payload || {}; const html = payload.html; if (typeof html === 'string') { if (!renderHtmlInIframe(html)) { pendingHtml = html; } } - } else if (event.source === inner.contentWindow) { + } else if (event.source === window.parent) { + // Forward other messages from parent to inner iframe + inner.contentWindow.postMessage(event.data, '*'); + } else if (event.source === inner.contentWindow) { // Relay messages from inner to parent window.parent.postMessage(event.data, '*'); } });sdks/typescript/client/src/utils/processResource.ts (1)
23-27: Context wiring and skybridge script injection look solid; verify widgetStateKey scopingThe new
ProcessHTMLResourceOptionsshape and the extraction of MCP (toolName,toolInput,toolOutput,toolResponseMetadata) and host (theme,locale,userAgent,model,displayMode,maxHeight,safeArea,capabilities) context into thetext/html+skybridgebranch are coherent, and passing everything throughcreateOpenAiScriptkeeps the skybridge runtime nicely encapsulated.Because
createOpenAiScriptserializes the config and runs it throughescapeForInlineScriptbefore embedding it into<script>tags, the earlier P1 concern about unescaped JSON breaking out of the inline script block should be addressed by this flow.One thing to double-check:
widgetStateKeyis derived only fromtoolName:const widgetStateKey = `openai-widget-state:${toolName ?? ''}:`;That means all widget instances for the same
toolNameon a given origin will share a single localStorage entry and overwrite each other’swidgetState. If you expect multiple independent widgets per tool (e.g., multiple HTML resources or conversations using the same tool), you may want to incorporate an instance-specific identifier (resource URI, message ID, etc.) into the key to avoid collisions.Also applies to: 33-46, 167-191
🧹 Nitpick comments (6)
sdks/typescript/client/src/types.ts (1)
106-133: Context prop shapes look good; consider sharing with widget config types
MCPContextPropsandClientContextPropsare flexible and line up with howmcp/hostare forwarded into the HTML render pipeline. As a small follow‑up, consider reusing or mirroring these shapes in the widget config/runtime types (e.g.SafeArea,capabilities) to avoid future drift between public types and the runtime script.sdks/typescript/client/src/adapters/appssdk/open-ai-runtime-script.ts (1)
123-172: Consider tighteningpostMessagehandling for tool calls
callToolworks correctly (per‑call listener, timeout cleanup, safe Promise resolution), but it currently:
- Listens on
windowand matches only bytypeandrequestId.- Sends messages with
targetOrigin: '*'.If you have access to the parent origin (or another trusted origin) at config time, it would be safer to:
- Restrict the
postMessagecalls to that origin.- Optionally ignore responses whose
event.sourceisn’t the expected parent window / origin.Not critical given the random
requestId, but worth tightening when possible.sdks/typescript/client/src/components/UIResourceRendererWC.tsx (1)
62-63: Avoid long‑lived@ts-expect-errorby typingnormalizeJsonPropgenericallyThe suppression here is understandable, but it hides real drift between the JSON shape and
UIResourceRendererProps['remoteDomProps']. You can usually remove the@ts-expect-errorby makingnormalizeJsonPropgeneric at the call site, e.g.:-function normalizeJsonProp(prop: unknown): Record<string, unknown> | undefined { +function normalizeJsonProp<T extends Record<string, unknown>>(prop: unknown): T | undefined { if (typeof prop === 'object' && prop !== null) { - return prop as Record<string, unknown>; + return prop as T; } if (typeof prop === 'string' && prop.trim() !== '') { try { - return JSON.parse(prop); + return JSON.parse(prop) as T; } catch (e) { console.error('Failed to parse JSON prop:', { prop, error: e }); return undefined; } } }and then:
const remoteDomProps = normalizeJsonProp<UIResourceRendererProps['remoteDomProps']>(rawRemoteDomProps);This keeps runtime behavior the same while regaining type safety.
sdks/typescript/client/src/adapters/appssdk/open-ai-runtime-script.bundled.ts (1)
4-4: TightenpostMessagehandler checks incallToolThe bundled runtime’s
callToolimplementation listens to"message"events onwindowand matches only ondata.typeandrequestId. It doesn’t verifyevent.sourceorevent.origin, so a script that can obtain a reference to this frame could spoof aopenai:callTool:responseand incorrectly resolve/reject the Promise.Since this file is generated, consider updating the source (
open-ai-runtime-script.ts) so the handler only processes events from the expected peer, e.g.:
- Check
event.source === window.parentbefore inspectingevent.data.- Optionally validate
event.originagainst an expected/allowed origin.That would harden the channel without changing the public API.
sdks/typescript/client/src/utils/processResource.ts (2)
111-135: Proxy handling for URL and HTML paths is consistent; consider DRY’ing the same-origin guardThe proxy logic for both
text/uri-listand HTML (text/html/text/html+skybridge) correctly:
- Validates the proxy string with
new URL(proxy).- Rejects proxies whose
hostmatcheswindow.location.host, avoiding an easy sandbox escape.- Logs and falls back gracefully on invalid proxy URLs.
The same-origin guard and error messaging are duplicated across the two branches. If this utility keeps evolving, you might consider extracting a small helper like
getSafeProxyUrl(proxy)(encapsulating thewindowcheck, host comparison, and logging) to ensure future changes stay in sync between the URL and HTML flows.Also applies to: 194-219
239-251: Remote DOM base64 decoding is consistent with other branchesThe
processRemoteDOMResourcepath now uses the same base64 →Uint8Array→TextDecoderpattern as the HTML and URI-list handlers, which keeps behavior and error handling aligned across resource types. If this pattern appears elsewhere, a small shared helper (e.g.,decodeBase64ToString(blob)) could reduce duplication and centralize future changes (like handling non‑browser environments).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.gitignore(1 hunks)README.md(1 hunks)docs/src/guide/client/html-resource.md(2 hunks)docs/src/guide/client/resource-renderer.md(2 hunks)pnpm-workspace.yaml(1 hunks)sdks/typescript/client/README.md(1 hunks)sdks/typescript/client/scripts/bundle-open-ai-script.js(1 hunks)sdks/typescript/client/scripts/proxy/index.html(1 hunks)sdks/typescript/client/src/adapters/appssdk/open-ai-runtime-script.bundled.ts(1 hunks)sdks/typescript/client/src/adapters/appssdk/open-ai-runtime-script.ts(1 hunks)sdks/typescript/client/src/adapters/appssdk/open-ai-script.ts(1 hunks)sdks/typescript/client/src/components/HTMLResourceRenderer.tsx(8 hunks)sdks/typescript/client/src/components/UIResourceRenderer.tsx(5 hunks)sdks/typescript/client/src/components/UIResourceRendererWC.tsx(1 hunks)sdks/typescript/client/src/components/__tests__/HTMLResourceRenderer.test.tsx(3 hunks)sdks/typescript/client/src/components/__tests__/UIResourceRenderer.test.tsx(4 hunks)sdks/typescript/client/src/types.ts(2 hunks)sdks/typescript/client/src/utils/__tests__/processResource.test.ts(10 hunks)sdks/typescript/client/src/utils/processResource.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
sdks/typescript/client/src/types.ts (1)
sdks/typescript/client/src/index.ts (1)
ALL_RESOURCE_CONTENT_TYPES(28-28)
sdks/typescript/client/src/components/__tests__/UIResourceRenderer.test.tsx (2)
sdks/typescript/client/src/components/HTMLResourceRenderer.tsx (1)
HTMLResourceRenderer(42-267)sdks/typescript/client/src/components/UIResourceRenderer.tsx (1)
UIResourceRenderer(38-72)
sdks/typescript/client/src/components/__tests__/HTMLResourceRenderer.test.tsx (2)
sdks/typescript/client/src/types.ts (1)
UI_METADATA_PREFIX(99-99)sdks/typescript/client/src/components/HTMLResourceRenderer.tsx (2)
HTMLResourceRenderer(42-267)InternalMessageType(22-36)
sdks/typescript/client/src/components/UIResourceRenderer.tsx (3)
sdks/typescript/client/src/components/HTMLResourceRenderer.tsx (2)
HTMLResourceRendererProps(7-20)HTMLResourceRenderer(42-267)sdks/typescript/client/src/components/RemoteDOMResourceRenderer.tsx (1)
RemoteDOMResourceProps(22-27)sdks/typescript/client/src/types.ts (2)
MCPContextProps(106-111)ClientContextProps(113-133)
sdks/typescript/client/src/utils/__tests__/processResource.test.ts (1)
sdks/typescript/client/src/utils/processResource.ts (1)
processHTMLResource(29-231)
sdks/typescript/client/src/components/HTMLResourceRenderer.tsx (4)
sdks/typescript/client/src/types.ts (2)
MCPContextProps(106-111)ClientContextProps(113-133)sdks/typescript/client/src/utils/metadataUtils.ts (1)
getUIResourceMetadata(8-19)sdks/typescript/client/src/utils/processResource.ts (1)
processHTMLResource(29-231)sdks/typescript/server/src/index.ts (2)
ReservedUrlParams(143-145)InternalMessageType(131-141)
sdks/typescript/client/src/adapters/appssdk/open-ai-script.ts (2)
sdks/typescript/client/src/types.ts (1)
ClientContextProps(113-133)sdks/typescript/client/src/adapters/appssdk/open-ai-runtime-script.bundled.ts (1)
API_RUNTIME_SCRIPT(4-4)
sdks/typescript/client/src/utils/processResource.ts (2)
sdks/typescript/client/src/types.ts (2)
MCPContextProps(106-111)ClientContextProps(113-133)sdks/typescript/client/src/adapters/appssdk/open-ai-script.ts (1)
createOpenAiScript(31-59)
🔇 Additional comments (23)
.gitignore (1)
52-52: LGTM!Adding
.pnpm-home/to the PNPM ignore section is standard practice; this directory contains machine-specific PNPM configuration that shouldn't be committed.sdks/typescript/client/src/types.ts (1)
6-11:skybridgecontent type addition looks consistentExtending
ALL_RESOURCE_CONTENT_TYPESwith'skybridge'keepsResourceContentTypein sync and matches the updated HTML tests and docs aroundtext/html+skybridge. No issues from this file’s side.pnpm-workspace.yaml (1)
2-6: Workspace andonlyBuiltDependenciesconfiguration looks appropriateIncluding
sdks/typescript/*,examples/*, anddocsas workspaces and markingesbuildas anonlyBuiltDependenciesentry is aligned with the new TS client tooling and bundling flow. Nothing concerning here.sdks/typescript/client/src/adapters/appssdk/open-ai-runtime-script.ts (3)
12-27: Widget config, defaults, and API surface are coherentThe
WidgetConfig/OpenAIWidgetAPIshapes, plus the resolved defaults for display mode, sizing, theme/locale, safe area, and capabilities, line up cleanly and give HTML widgets a predictable environment. Defaulting capabilities and merging them with any provided config is a nice touch for host feature detection.Also applies to: 32-45, 54-67, 95-107
123-137: Widget state persistence and hydration look robustPersisting
widgetStatebehind a keyed localStorage entry, wrapping bothsetItemand latergetItem/JSON.parsein try/catch, and notifying the parent viaopenai:setWidgetStategives a solid, failure‑tolerant state channel. The async hydration after initialization is also a good fit to avoid blocking setup.Also applies to: 252-282
238-251: Global exposure and config cleanup are reasonableDefining non‑writable, non‑configurable
window.openaiandwindow.webplusplus deleting/undefining__MCP_WIDGET_CONFIG__keeps the public API stable and avoids leaking config. Just be aware this will override any pre‑existingwindow.openaiin the iframe if one exists, which is likely the intended behavior for this runtime.Also applies to: 284-288
sdks/typescript/client/src/components/__tests__/HTMLResourceRenderer.test.tsx (2)
74-75: Updated error messaging matches new HTML mime supportThe test expectations for the invalid mimeType error now mention
text/html+skybridgealongsidetext/htmlandtext/uri-list. This keeps the error copy aligned with the expanded content types and will guard against regressions in the user‑facing message.Also applies to: 362-363
537-575: Good coverage to keep render data distinct from MCP tool outputThe
"should keep render data separate from tool output context"test clearly asserts thatiframeRenderData+ metadata initial render data are the only fields sent in theUI_LIFECYCLE_IFRAME_RENDER_DATApayload, even whenmcp.toolOutputis present. This directly guards the new design where tool context flows through widget config rather than being merged into render data.sdks/typescript/client/src/components/__tests__/UIResourceRenderer.test.tsx (1)
32-33: Tests correctly track HTML renderer props and context forwardingSwitching expectations to
expect.objectContaining({ resource })/{ resource, proxy: ... }makes the tests robust to additional props onHTMLResourceRenderer, which is useful now thatmcp/hostexist. The new"should forward context props to HTMLResourceRenderer"test nicely validates thatUIResourceRendererpassesmcpandhostthrough as intended.Also applies to: 40-41, 78-79, 88-103, 105-118
sdks/typescript/client/README.md (1)
101-103: Client README accurately documents newmcp/hostpropsThe added bullets for
mcpandhostclearly describe the MCP invocation context and host context (including default{ hover: true, touch: false }capabilities) and note that HTML‑specific overrides can be supplied viahtmlProps. This aligns with how the props are threaded intoHTMLResourceRenderer.README.md (1)
101-102: Root README clarifies tool output handling for HTML runtimesThe updated
mcp/hostdocs call out thattoolOutputis injected into supported runtimes (e.g. Skybridge) via widget config and explicitly not merged intoiframeRenderData. That matches the new test coverage and is a useful nuance for integrators.docs/src/guide/client/html-resource.md (1)
9-18: LGTM!The documentation clearly describes the new
mcpandhostprops, including their types, purpose, and how they integrate with the HTML resource rendering flow.docs/src/guide/client/resource-renderer.md (1)
45-54: LGTM!The documentation accurately describes the new
mcpandhostprops and their integration withhtmlProps. The explanation of how these props can be provided either at the top level or withinhtmlPropsis clear.sdks/typescript/client/src/utils/__tests__/processResource.test.ts (2)
313-353: LGTM!The test comprehensively verifies that MCP and host context are correctly injected into skybridge HTML, checking for all expected fields including
toolInput,toolOutput,toolResponseMetadata,toolName,theme,locale,userAgent,model,displayMode,maxHeight,safeArea, andcapabilities.
355-371: LGTM!The test correctly verifies that the runtime script is injected even when
<html>and<head>tags are missing, which is an important edge case for skybridge HTML snippets.sdks/typescript/client/src/components/UIResourceRenderer.tsx (2)
11-14: LGTM!The props are correctly typed, with
htmlPropsproperly excludingmcpandhostto prevent duplication, allowing these contexts to be provided either at the top level or withinhtmlProps.
46-59: LGTM!The
skybridgecontent type is correctly handled alongsiderawHtmlandexternalUrl, withmcpandhostprops properly forwarded toHTMLResourceRenderer.sdks/typescript/client/src/adapters/appssdk/open-ai-script.ts (2)
22-29: LGTM!The escaping function correctly sanitizes JSON for inline script embedding by escaping angle brackets, ampersands, and Unicode line terminators (U+2028, U+2029), which prevents script injection and ensures the JSON is safely embedded in HTML context.
31-59: API_RUNTIME_SCRIPT is safe and properly sourced from trusted, controlled code.Verification confirms the runtime script is securely generated from version-controlled TypeScript source code. The bundling script reads exclusively from the source file with no external or user-controlled inputs. The bundled output is JSON.stringify()'d before being embedded as a constant, and user configuration data is separately protected with escapeForInlineScript() which covers XSS vectors (<, >, &, U+2028, U+2029). No injection vectors were identified.
sdks/typescript/client/src/components/HTMLResourceRenderer.tsx (3)
61-69: LGTM!The
renderDatacomputation correctly merges metadata-derived initial render data with theiframeRenderDataprop, with prop values taking precedence for duplicate keys. This maintains backward compatibility while supporting the new context propagation.
71-79: LGTM!The
processHTMLResourcecall is correctly updated to use the new options-based API, passingproxy,mcp, andhostas an options object. TheuseMemodependencies are also correctly updated to include the new props.
120-222: LGTM!The message handling logic correctly uses
renderDatathroughout for all lifecycle events (ui-proxy-iframe-ready,ui-lifecycle-iframe-ready,ui-request-render-data), ensuring consistent data delivery to iframes. TheuseEffectdependencies are properly updated to includerenderData.sdks/typescript/client/scripts/bundle-open-ai-script.js (1)
1-1: No action needed — esbuild is properly declared.The review comment's concern about a missing esbuild dependency is incorrect. esbuild is already declared in
devDependencieswith version^0.25.5insdks/typescript/client/package.json, so the script will execute without dependency-related errors.Likely an incorrect or invalid review comment.
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)
sdks/typescript/client/src/utils/processResource.ts (1)
111-135: <!-- [scratchpad_start]
[task verify_proxy_url_handling]
Status: Analyzing grep results for proxy URL patterns[observations]
- Documentation (docs/src/guide/client/using-a-proxy.md) consistently shows absolute URLs: 'https://proxy.mcpui.dev'
- Line 60 explicitly states: "For security reasons, you MUST NOT host the proxy script on the same origin"
- Line 66: Requires validation as "http:" or "https:" (absolute URLs)
- Test cases (processResource.test.ts lines 193-279) all use absolute URLs for passing tests
- Test line 217 uses 'not-a-valid-url' (relative-like) and verifies fallback behavior
- Code at lines 113 and 196:
new URL(proxy)- constructor requires absolute URL per spec- Error handling catches thrown errors from invalid proxy strings
- All examples in docs use full absolute URLs
[analysis]
- The review comment's concern is VALID: new URL(proxy) without a base parameter throws on relative URLs
- Current behavior: silent fallback with console.error when relative URL is passed
- Documentation shows only absolute URL usage patterns
- No explicit documentation stating proxy must be absolute
- Tests confirm relative URLs do fall back (not a new bug, existing behavior)
- The silent fallback could confuse developers who try to use relative paths
[conclusion]
The original review comment accurately identifies the issue. The code requires absolute URLs due to new URL() constructor requirements. The suggestion to either:
- Support relative URLs via resolving against window.location.origin, OR
- Document the absolute URL requirement
...is a valid refactoring suggestion, not a critical bug.[done]
Verification complete - original review is sound.
[scratchpad_end]-->
Consider documenting that
proxymust be an absolute URL, or support relative URLsThe current code silently falls back when a relative proxy path (e.g.,
'/mcp-proxy') is provided, sincenew URL(proxy)requires an absolute URL. This could confuse developers who expect relative paths to work.You have two options:
Document the constraint: Add a JSDoc comment or guide entry explicitly stating that
proxymust be an absolute URL (e.g.,https://proxy.example.com).Support relative URLs: Resolve relative proxy URLs against
window.location.originwhen available:const base = typeof window !== 'undefined' ? window.location.origin : undefined; const proxyUrl = base ? new URL(proxy, base) : new URL(proxy);Both approaches apply to lines 111–135 (for
text/uri-list) and 194–219 (fortext/html).
♻️ Duplicate comments (1)
sdks/typescript/client/src/adapters/appssdk/open-ai-script.ts (1)
4-18: Inline script escaping correctly fixes the prior injection riskThis refactor cleanly addresses the earlier P1 about embedding JSON into an inline
<script>:
OpenAiScriptOptionscaptures exactly the MCP/Host fields you need.createOpenAiScriptbuilds a config object,JSON.stringify’s it, then runs it throughescapeForInlineScript, which escapes<,>,&, and the problematic U+2028/U+2029 characters.- The escaped JSON is inlined as a JavaScript object literal:
window.__MCP_WIDGET_CONFIG__ = ${escapedConfig};so any
</script>or similar sequences coming from tool input/output cannot break out of the script block.Given the runtime bundle reads
window.__MCP_WIDGET_CONFIG__and then deletes it, the lifecycle is tight and the surface area small. This looks correct and resolves the previously reported injection concern.Also applies to: 20-29, 31-59
🧹 Nitpick comments (1)
sdks/typescript/client/src/components/HTMLResourceRenderer.tsx (1)
120-222: AddautoResizeIframeto theuseEffectdependency listThe
handleMessageeffect readsautoResizeIframewhen handlingUI_SIZE_CHANGE(lines 180–187), butautoResizeIframeis not included in the dependency array on line 222:}, [onUIAction, initialRenderData, iframeRenderMode, htmlString, iframeSrcToRender, sandbox]);If
autoResizeIframechanges over the component's lifetime, the effect will continue using the old value. While low-risk in practice (the prop is typically static), this violates the React best practice enforced by thereact-hooks/exhaustive-depsrule. Add it to the dependencies:- }, [onUIAction, initialRenderData, iframeRenderMode, htmlString, iframeSrcToRender, sandbox]); + }, [onUIAction, initialRenderData, iframeRenderMode, htmlString, iframeSrcToRender, sandbox, autoResizeIframe]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/src/guide/client/html-resource.md(2 hunks)docs/src/guide/client/resource-renderer.md(2 hunks)sdks/typescript/client/src/adapters/appssdk/open-ai-script.ts(1 hunks)sdks/typescript/client/src/components/HTMLResourceRenderer.tsx(4 hunks)sdks/typescript/client/src/components/UIResourceRenderer.tsx(5 hunks)sdks/typescript/client/src/types.ts(2 hunks)sdks/typescript/client/src/utils/processResource.ts(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/src/guide/client/html-resource.md
🧰 Additional context used
🧬 Code graph analysis (5)
sdks/typescript/client/src/adapters/appssdk/open-ai-script.ts (2)
sdks/typescript/client/src/types.ts (1)
HostProps(113-133)sdks/typescript/client/src/adapters/appssdk/open-ai-runtime-script.bundled.ts (1)
API_RUNTIME_SCRIPT(4-4)
sdks/typescript/client/src/components/HTMLResourceRenderer.tsx (2)
sdks/typescript/client/src/types.ts (2)
MCPProps(106-111)HostProps(113-133)sdks/typescript/client/src/utils/processResource.ts (1)
processHTMLResource(29-231)
sdks/typescript/client/src/utils/processResource.ts (2)
sdks/typescript/client/src/types.ts (2)
MCPProps(106-111)HostProps(113-133)sdks/typescript/client/src/adapters/appssdk/open-ai-script.ts (1)
createOpenAiScript(31-59)
sdks/typescript/client/src/types.ts (1)
sdks/typescript/client/src/index.ts (1)
ALL_RESOURCE_CONTENT_TYPES(28-28)
sdks/typescript/client/src/components/UIResourceRenderer.tsx (3)
sdks/typescript/client/src/components/HTMLResourceRenderer.tsx (2)
HTMLResourceRendererProps(7-20)HTMLResourceRenderer(42-267)sdks/typescript/client/src/components/RemoteDOMResourceRenderer.tsx (1)
RemoteDOMResourceProps(22-27)sdks/typescript/client/src/types.ts (2)
MCPProps(106-111)HostProps(113-133)
🔇 Additional comments (11)
sdks/typescript/client/src/utils/processResource.ts (3)
23-46: Options object + MCP/Host extraction looks solidSwitching
processHTMLResourceto a typed options object (ProcessHTMLResourceOptions) and immediately extractingproxy,mcp, andhostinto local variables keeps the signature extensible and makes the downstream skybridge integration straightforward. No functional or typing issues stand out here.Also applies to: 71-79
47-56: Skybridge HTML handling and script injection are correct and guardedThe new
text/html+skybridgebranch correctly:
- Reuses the existing HTML decoding path (text or base64
blob).- Builds a stable
widgetStateKeyand passes all MCP/Host fields intocreateOpenAiScript.- Injects the resulting
<script>blocks into<head>,<html>, or prepends them if no wrapper tags exist.Combined with
createOpenAiScript’s escaping, this addresses the earlier injection concern around embedding JSON into inline scripts. The regex replacements are conservative (<head[^>]*>/<html[^>]*>withiflag) and should behave well on typical HTML.Also applies to: 141-193
233-266: Remote DOM base64 decode helper remains correctThe
processRemoteDOMResourcelogic for preferring non-emptytext, then base64-decodingblobviaUint8Array.from(atob(...)), and returning structured errors on failure is consistent with the HTML path and looks correct.sdks/typescript/client/src/components/HTMLResourceRenderer.tsx (1)
3-20: MCP/Host context plumbing into HTML pipeline looks goodExposing
mcp?: MCPPropsandhost?: HostPropsonHTMLResourceRendererProps, then threading them throughprocessHTMLResource(resource, { proxy, mcp, host })(memoized on[resource, proxy, mcp, host]) is clean and keeps all skybridge/OpenAI wiring confined to the processing layer. No issues with the typing or memoization pattern here.Also applies to: 42-53, 71-79
sdks/typescript/client/src/types.ts (1)
105-133: MCPProps and HostProps shapes align with downstream usageThe new
MCPPropsandHostPropsdefinitions match whatHTMLResourceRenderer,processHTMLResource, andcreateOpenAiScriptexpect (toolInput/output/name/metadata plus theming, locale, userAgent, model, displayMode, maxHeight, safeArea, capabilities).Reusing
HostProps['safeArea']/['capabilities']inOpenAiScriptOptionskeeps everything in sync and avoids duplication.sdks/typescript/client/src/components/UIResourceRenderer.tsx (6)
2-2: LGTM! Imports added correctly.The addition of
HostPropsandMCPPropsimports aligns with the new context propagation feature and the component props defined below.
11-11: LGTM! Props updated correctly for context propagation.The
htmlPropstype correctly omitsmcpandhostsince these are now passed as separate props to the component and forwarded directly toHTMLResourceRenderer. The addition of optionalmcpandhostprops enables proper context propagation.Also applies to: 13-14
27-29: LGTM! Skybridge mime type support added correctly.The new branch for
text/html+skybridgemime type follows the same pattern as existing mime type checks and correctly returns theskybridgecontent type.
39-39: LGTM! Props destructured correctly.The destructuring properly includes the new
mcpandhostprops for use in the component body.
48-48: LGTM! Skybridge case and context propagation implemented correctly.The
skybridgecase is appropriately grouped withrawHtmlandexternalUrlsince all three useHTMLResourceRenderer. The explicit passing ofmcpandhostprops toHTMLResourceRendererenables proper context propagation for skybridge resources.Also applies to: 50-58
60-68: Verify whether RemoteDOMResourceRenderer should receive context props.The
mcpandhostprops are passed toHTMLResourceRendererbut not toRemoteDOMResourceRenderer. While this appears intentional (sinceRemoteDOMResourcePropsdoesn't include these fields), please confirm whether Remote DOM resources should also receive MCP/Host context for consistency.If Remote DOM resources need context propagation, you'll need to:
- Update
RemoteDOMResourcePropstype to includemcp?andhost?- Pass these props to
RemoteDOMResourceRenderersimilar to the HTML case
| import type { MCPProps, HostProps } from '@mcp-ui/client'; | ||
|
|
||
| interface UIResourceRendererProps { | ||
| resource: Partial<Resource>; | ||
| onUIAction?: (result: UIActionResult) => Promise<unknown>; | ||
| supportedContentTypes?: ResourceContentType[]; | ||
| htmlProps?: Omit<HTMLResourceRendererProps, 'resource' | 'onUIAction'>; | ||
| htmlProps?: Omit<HTMLResourceRendererProps, 'resource' | 'onUIAction' | 'mcp' | 'host'>; | ||
| remoteDomProps?: Omit<RemoteDOMResourceProps, 'resource' | 'onUIAction'>; | ||
| mcp?: MCPProps; | ||
| host?: HostProps; | ||
| } |
There was a problem hiding this comment.
Doc/type mismatch for mcp/host vs htmlProps
The type snippet defines:
htmlProps?: Omit<HTMLResourceRendererProps, 'resource' | 'onUIAction' | 'mcp' | 'host'>;so mcp and host cannot be passed via htmlProps. The bullets for mcp and host still say “These can also be provided via htmlProps…”, which is no longer accurate.
Either:
- keep
htmlPropsexcludingmcp/hostand update the text to say these are only top‑level props onUIResourceRenderer, or - allow
mcp/hostthroughhtmlPropsagain and adjust theOmit<>accordingly.
Also applies to: 82-83
🤖 Prompt for AI Agents
In docs/src/guide/client/resource-renderer.md around lines 45 to 55 (and also
apply the same change at lines 82-83), the prose incorrectly states that mcp and
host can be provided via htmlProps while the type snippet excludes them via
Omit<...,'mcp'|'host'>; either update the documentation bullets to say mcp and
host are top-level props only, or change the type so htmlProps allows mcp and
host (remove them from the Omit), and make the same corresponding change to the
text at lines 82-83 so code and docs remain consistent.
| htmlProps?: Omit<HTMLResourceRendererProps, 'resource' | 'onUIAction'>; | ||
| remoteDomProps?: Omit<RemoteDOMResourceProps, 'resource' | 'onUIAction'>; | ||
| htmlProps?: Omit<HTMLResourceRendererProps, 'resource' | 'onUIAction' | 'mcp' | 'host'>; | ||
| remoteDomProps?: RemoteDOMResourceProps; |
There was a problem hiding this comment.
Potential prop override issue with remoteDomProps type.
Changing remoteDomProps from an Omit variant to the full RemoteDOMResourceProps type could cause unintended prop overrides. Since RemoteDOMResourceProps includes resource and onUIAction fields, and at lines 63-66 the spread {...remoteDomProps} occurs after the explicit resource={resource} and onUIAction={onUIAction} props, any values in remoteDomProps would override the component's props.
Consider reverting to the omitted variant:
- remoteDomProps?: RemoteDOMResourceProps;
+ remoteDomProps?: Omit<RemoteDOMResourceProps, 'resource' | 'onUIAction'>;Or adjust the spread order in the RemoteDOMResourceRenderer invocation to ensure the explicit props take precedence.
📝 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.
| remoteDomProps?: RemoteDOMResourceProps; | |
| remoteDomProps?: Omit<RemoteDOMResourceProps, 'resource' | 'onUIAction'>; |
🤖 Prompt for AI Agents
In sdks/typescript/client/src/components/UIResourceRenderer.tsx around line 12,
the change from an Omit variant to full RemoteDOMResourceProps for
remoteDomProps can allow resource and onUIAction in remoteDomProps to override
the explicit props passed later; either revert remoteDomProps back to
Omit<RemoteDOMResourceProps, 'resource' | 'onUIAction'> to prevent those keys
being present, or keep the full type but move the {...remoteDomProps} spread so
it occurs before the explicit resource={resource} and onUIAction={onUIAction}
props in the RemoteDOMResourceRenderer invocation so the explicit props take
precedence.
| export const ALL_RESOURCE_CONTENT_TYPES = [ | ||
| 'rawHtml', | ||
| 'externalUrl', | ||
| 'remoteDom', | ||
| 'skybridge', | ||
| ] as const; | ||
| export type ResourceContentType = (typeof ALL_RESOURCE_CONTENT_TYPES)[number]; |
There was a problem hiding this comment.
Expose of 'skybridge' in ALL_RESOURCE_CONTENT_TYPES vs docs
Adding 'skybridge' to ALL_RESOURCE_CONTENT_TYPES and thus ResourceContentType makes sense for the new skybridge path.
However, the docs for supportedContentTypes (in docs/src/guide/client/resource-renderer.md, Line 71) still only mention ['rawHtml', 'externalUrl', 'remoteDom']. Consider updating the docs to either:
- Include
'skybridge'as a valid content type, or - Clarify that
'skybridge'is internal/experimental if you don’t want it used directly.
🤖 Prompt for AI Agents
In sdks/typescript/client/src/types.ts around lines 6 to 12, the new 'skybridge'
value is added to ALL_RESOURCE_CONTENT_TYPES (and thus ResourceContentType) but
the docs (docs/src/guide/client/resource-renderer.md, around line 71) still list
only ['rawHtml','externalUrl','remoteDom']; update the documentation to reflect
the code change by either adding 'skybridge' to the supportedContentTypes list
and describing its use/behavior, or explicitly marking 'skybridge' as
internal/experimental with guidance not to use it directly; ensure the docs text
and any examples are consistent with the exported type so users aren’t misled.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.