Skip to content

Conversation

@infoxicator
Copy link

@infoxicator infoxicator commented Oct 27, 2025

Adds skybridge integration for clients

  • double iframe implementation (tested)
  • openAi script injection and global initialisation
  • bundle openAi script
  • update documentation
  • add tests
  • refactor UiResourceRenderer to streamline interface Normalising renderData payload #136

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@idosal @liady this is the main change to support the double iframe exactly the same way as chatGTP.

also I think line 6 the meta CSP is not required, I had to remove it since the CSP can be set on the actual remote html and needs to be super relaxed otherwise nothing load

inner.setAttribute('sandbox', sandbox);
}
if (typeof html === 'string') {
inner.srcdoc = html;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the key was to use document.write() instead of srcdoc.

srcdoc changes the root url and breaks client side navigation.

inner.src: 'about:blank'

and document.write() do the trick

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@idosal you recently added the double proxy. what client was that for? I don't think this is a breaking change since it is the code clients need to add to their CDN anyway but curious if you tested the previous implementation

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not used in production yet, so we can change it.

'rawHtml',
'externalUrl',
'remoteDom',
'skybridge',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@idosal @liady any preference with this name? skybridge is what they call it

htmlString?: string;
};

const apiScript = (
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liady @idosal want me to move this script to a different file?

was also considering bundling it but thought might be overkill

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd move this to a different file, or ideally bundle it (similarly to how we handle the server adapter).

}

if (resource.mimeType === 'text/html+skybridge') {
const widgetStateKey = `openai-widget-state:${toolName}:`;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main thing I am not sure about. Is the widgetState persisted per tool invocation or per widget? if it is per widget this works but if it is per invocation then we need the host to pass something like a toolId or unique identifier.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liady @idosal made all the changes here and added a new condition to HTMLResourceRenderer since is practically the same but let me know if you'd prefer to create a new file SkybridgeResourceRenderer and put the rendering there instead

@infoxicator
Copy link
Author

UiResourceRenderer interface change infoxicator#2

theme: 'dark',
locale: 'en-US',
safeArea: { insets: { top: 0, bottom: 0, left: 0, right: 0 } },
userAgent: {},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have userAgent set to

{
  device: { type: 'desktop' },
  capabilities: { hover: true, touch: false }
}

cc: @chelojimenez what was the reason we had userAgent set to this?

toolResponseMetadata: ${JSON.stringify(toolResponseMetadata ?? null)},
displayMode: 'inline',
maxHeight: 600,
theme: 'dark',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set theme to be dynamic. User should be able to toggle the theme. Pass in a theme: "light" | "dark" to the api.

Comment on lines 13 to 15
toolInput?: Record<string, unknown>;
toolName?: string;
toolResponseMetadata?: Record<string, unknown>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@infoxicator WDYT about formalizing these fields to be in the render data?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@idosal I am leaning more towards a cleaner interface like here: so we can extend it and add more fields in the future if needed without having to pollute the props of the main renderUIResource component.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been working on shipping the postman integration. But still giving this some thought. I like the new props jsut ensuring they align with mcp-ui and all the other resource types not just chatgpt.

For example. ToolResponse and widget state in the context of chatgpt is the initial data. but I think there might be a case where we would like to pass arbitrary data to a widget that are not those. Currently I am merging them. into a single object for chatgpt but considering all the implications of that.

Maybe is safer just to keep them separate?

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

7 issues found across 19 files

Prompt for AI agents (all 7 issues)

Understand the root cause of the following 7 issues and fix them.


<file name="sdks/typescript/client/src/adapters/appssdk/open-ai-script.ts">

<violation number="1" location="sdks/typescript/client/src/adapters/appssdk/open-ai-script.ts:39">
Escape the serialized config before inlining it so that user-provided values cannot terminate the script tag and execute arbitrary JavaScript.</violation>
</file>

<file name="sdks/typescript/client/src/utils/__tests__/processResource.test.ts">

<violation number="1" location="sdks/typescript/client/src/utils/__tests__/processResource.test.ts:342">
The injected config is JSON-stringified, so the HTML contains `&quot;toolInput&quot;:{&quot;foo&quot;:&quot;bar&quot;}` with quoted keys—`&#39;toolInput: {&quot;foo&quot;:&quot;bar&quot;}&#39;` never appears and this assertion will fail. Update this and the other similar expectations in the block to match the JSON string.</violation>

<violation number="2" location="sdks/typescript/client/src/utils/__tests__/processResource.test.ts:353">
The script only serializes the provided capabilities object, so with `{ hover: false }` the HTML contains `&quot;capabilities&quot;:{&quot;hover&quot;:false}`—there is no `touch` entry, making this assertion fail.</violation>

<violation number="3" location="sdks/typescript/client/src/utils/__tests__/processResource.test.ts:370">
Because the injected config is JSON output, the HTML includes `&quot;toolOutput&quot;:{&quot;fallback&quot;:true}` rather than `toolOutput: {…}`. This assertion will always fail and should match the JSON-formatted substring.</violation>
</file>

<file name="README.md">

<violation number="1" location="README.md:102">
`htmlProps` cannot accept these context props, so the README guidance to pass them there is incorrect. Please remove or revise that sentence.</violation>
</file>

<file name="docs/src/guide/client/html-resource.md">

<violation number="1" location="docs/src/guide/client/html-resource.md:46">
The docs state that `mcpContextProps.toolOutput` merges with `iframeRenderData`, but `processHTMLResource` simply overrides `initialRenderData` when `toolOutput` exists, so no merge occurs. Please update the description to reflect the actual override behavior.</violation>
</file>

<file name="sdks/typescript/client/src/utils/processResource.ts">

<violation number="1" location="sdks/typescript/client/src/utils/processResource.ts:188">
Skybridge HTML without a &lt;html&gt; tag never gets the runtime script injected, so those resources fail to initialize. Please add a fallback that prepends the script when neither &lt;head&gt; nor &lt;html&gt; is present.</violation>
</file>

Since this is your first cubic review, here's how it works:

  • cubic automatically reviews your code and comments on bugs and improvements
  • Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
  • Ask questions if you need clarification on any suggestion

React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.

});

expect(result.error).toBeUndefined();
expect(result.htmlString).toContain('toolOutput: {"fallback":true}');
Copy link

@cubic-dev-ai cubic-dev-ai bot Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the injected config is JSON output, the HTML includes "toolOutput":{"fallback":true} rather than toolOutput: {…}. This assertion will always fail and should match the JSON-formatted substring.

Prompt for AI agents
Address the following comment on sdks/typescript/client/src/utils/__tests__/processResource.test.ts at line 370:

<comment>Because the injected config is JSON output, the HTML includes `&quot;toolOutput&quot;:{&quot;fallback&quot;:true}` rather than `toolOutput: {…}`. This assertion will always fail and should match the JSON-formatted substring.</comment>

<file context>
@@ -309,6 +309,66 @@ describe(&#39;processHTMLResource&#39;, () =&gt; {
+      });
+
+      expect(result.error).toBeUndefined();
+      expect(result.htmlString).toContain(&#39;toolOutput: {&quot;fallback&quot;:true}&#39;);
+    });
   });
</file context>
Suggested change
expect(result.htmlString).toContain('toolOutput: {"fallback":true}');
expect(result.htmlString).toContain('"toolOutput":{"fallback":true}');
Fix with Cubic

expect(result.htmlString).toContain('displayMode: "overlay"');
expect(result.htmlString).toContain('maxHeight: 720');
expect(result.htmlString).toContain('"insets":{"top":10,"bottom":20,"left":0,"right":5}');
expect(result.htmlString).toContain('capabilities: {"hover":false,"touch":false}');
Copy link

@cubic-dev-ai cubic-dev-ai bot Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script only serializes the provided capabilities object, so with { hover: false } the HTML contains "capabilities":{"hover":false}—there is no touch entry, making this assertion fail.

Prompt for AI agents
Address the following comment on sdks/typescript/client/src/utils/__tests__/processResource.test.ts at line 353:

<comment>The script only serializes the provided capabilities object, so with `{ hover: false }` the HTML contains `&quot;capabilities&quot;:{&quot;hover&quot;:false}`—there is no `touch` entry, making this assertion fail.</comment>

<file context>
@@ -309,6 +309,66 @@ describe(&#39;processHTMLResource&#39;, () =&gt; {
+      expect(result.htmlString).toContain(&#39;displayMode: &quot;overlay&quot;&#39;);
+      expect(result.htmlString).toContain(&#39;maxHeight: 720&#39;);
+      expect(result.htmlString).toContain(&#39;&quot;insets&quot;:{&quot;top&quot;:10,&quot;bottom&quot;:20,&quot;left&quot;:0,&quot;right&quot;:5}&#39;);
+      expect(result.htmlString).toContain(&#39;capabilities: {&quot;hover&quot;:false,&quot;touch&quot;:false}&#39;);
+    });
+
</file context>
Suggested change
expect(result.htmlString).toContain('capabilities: {"hover":false,"touch":false}');
expect(result.htmlString).toContain('"capabilities":{"hover":false}');
Fix with Cubic


expect(result.error).toBeUndefined();
expect(result.htmlString).toContain('openai-widget-state:demo-tool:');
expect(result.htmlString).toContain('toolInput: {"foo":"bar"}');
Copy link

@cubic-dev-ai cubic-dev-ai bot Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The injected config is JSON-stringified, so the HTML contains "toolInput":{"foo":"bar"} with quoted keys—'toolInput: {"foo":"bar"}' never appears and this assertion will fail. Update this and the other similar expectations in the block to match the JSON string.

Prompt for AI agents
Address the following comment on sdks/typescript/client/src/utils/__tests__/processResource.test.ts at line 342:

<comment>The injected config is JSON-stringified, so the HTML contains `&quot;toolInput&quot;:{&quot;foo&quot;:&quot;bar&quot;}` with quoted keys—`&#39;toolInput: {&quot;foo&quot;:&quot;bar&quot;}&#39;` never appears and this assertion will fail. Update this and the other similar expectations in the block to match the JSON string.</comment>

<file context>
@@ -309,6 +309,66 @@ describe(&#39;processHTMLResource&#39;, () =&gt; {
+
+      expect(result.error).toBeUndefined();
+      expect(result.htmlString).toContain(&#39;openai-widget-state:demo-tool:&#39;);
+      expect(result.htmlString).toContain(&#39;toolInput: {&quot;foo&quot;:&quot;bar&quot;}&#39;);
+      expect(result.htmlString).toContain(&#39;toolOutput: {&quot;override&quot;:&quot;yes&quot;}&#39;);
+      expect(result.htmlString).toContain(&#39;toolResponseMetadata: {&quot;traceId&quot;:&quot;abc&quot;}&#39;);
</file context>
Suggested change
expect(result.htmlString).toContain('toolInput: {"foo":"bar"}');
expect(result.htmlString).toContain('"toolInput":{"foo":"bar"}');
Fix with Cubic

README.md Outdated
- **`iframeRenderData`**: Optional `Record<string, unknown>` to pass data to the iframe upon rendering. This enables advanced use cases where the parent application needs to provide initial state or configuration to the sandboxed iframe content.
- **`autoResizeIframe`**: Optional `boolean | { width?: boolean; height?: boolean }` to automatically resize the iframe to the size of the content.
- **`mcpContextProps`**: Optional MCP invocation context forwarded to HTML resources (e.g., `toolInput`, `toolOutput`, `toolName`, `toolResponseMetadata`). When `toolOutput` is provided, it is merged with `iframeRenderData` for the sandbox payload (with `toolOutput` fields taking precedence).
- **`clientContextProps`**: Optional host context (e.g., `theme`, `locale`, `userAgent`, `model`, `displayMode`, `maxHeight`, `safeArea`, `capabilities`) exposed to sandboxed HTML content. Both context props can also be supplied via `htmlProps` when you need HTML-specific overrides. `capabilities` defaults to `{ hover: true, touch: false }` when unspecified.
Copy link

@cubic-dev-ai cubic-dev-ai bot Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

htmlProps cannot accept these context props, so the README guidance to pass them there is incorrect. Please remove or revise that sentence.

Prompt for AI agents
Address the following comment on README.md at line 102:

<comment>`htmlProps` cannot accept these context props, so the README guidance to pass them there is incorrect. Please remove or revise that sentence.</comment>

<file context>
@@ -98,6 +98,8 @@ It accepts the following props:
   - **`iframeRenderData`**: Optional `Record&lt;string, unknown&gt;` to pass data to the iframe upon rendering. This enables advanced use cases where the parent application needs to provide initial state or configuration to the sandboxed iframe content.
   - **`autoResizeIframe`**: Optional `boolean | { width?: boolean; height?: boolean }` to automatically resize the iframe to the size of the content.
+- **`mcpContextProps`**: Optional MCP invocation context forwarded to HTML resources (e.g., `toolInput`, `toolOutput`, `toolName`, `toolResponseMetadata`). When `toolOutput` is provided, it is merged with `iframeRenderData` for the sandbox payload (with `toolOutput` fields taking precedence).
+- **`clientContextProps`**: Optional host context (e.g., `theme`, `locale`, `userAgent`, `model`, `displayMode`, `maxHeight`, `safeArea`, `capabilities`) exposed to sandboxed HTML content. Both context props can also be supplied via `htmlProps` when you need HTML-specific overrides. `capabilities` defaults to `{ hover: true, touch: false }` when unspecified.
 - **`remoteDomProps`**: Optional props for the internal `&lt;RemoteDOMResourceRenderer&gt;`
   - **`library`**: Optional component library for Remote DOM resources (defaults to `basicComponentLibrary`)
</file context>
Suggested change
- **`clientContextProps`**: Optional host context (e.g., `theme`, `locale`, `userAgent`, `model`, `displayMode`, `maxHeight`, `safeArea`, `capabilities`) exposed to sandboxed HTML content. Both context props can also be supplied via `htmlProps` when you need HTML-specific overrides. `capabilities` defaults to `{ hover: true, touch: false }` when unspecified.
- **`clientContextProps`**: Optional host context (e.g., `theme`, `locale`, `userAgent`, `model`, `displayMode`, `maxHeight`, `safeArea`, `capabilities`) exposed to sandboxed HTML content. `capabilities` defaults to `{ hover: true, touch: false }` when unspecified.
Fix with Cubic

- **`style`**: (Optional) Custom styles for the iframe.
- **`proxy`**: (Optional) A URL to a proxy script. This is useful for hosts with a strict Content Security Policy (CSP). When provided, external URLs will be rendered in a nested iframe hosted at this URL. For example, if `proxy` is `https://my-proxy.com/`, the final URL will be `https://my-proxy.com/?url=<encoded_original_url>`. For your convenience, mcp-ui hosts a proxy script at `https://proxy.mcpui.dev`, which you can use as a the prop value without any setup (see `examples/external-url-demo`).
- **`iframeProps`**: (Optional) Custom props for the iframe.
- **`iframeRenderData`**: (Optional) Additional data merged into the render payload forwarded to the iframe. When `mcpContextProps.toolOutput` is provided, it merges with this data for the sandbox payload (with `toolOutput` fields taking precedence).
Copy link

@cubic-dev-ai cubic-dev-ai bot Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs state that mcpContextProps.toolOutput merges with iframeRenderData, but processHTMLResource simply overrides initialRenderData when toolOutput exists, so no merge occurs. Please update the description to reflect the actual override behavior.

Prompt for AI agents
Address the following comment on docs/src/guide/client/html-resource.md at line 46:

<comment>The docs state that `mcpContextProps.toolOutput` merges with `iframeRenderData`, but `processHTMLResource` simply overrides `initialRenderData` when `toolOutput` exists, so no merge occurs. Please update the description to reflect the actual override behavior.</comment>

<file context>
@@ -40,6 +43,8 @@ The component accepts the following props:
 - **`style`**: (Optional) Custom styles for the iframe.
 - **`proxy`**: (Optional) A URL to a proxy script. This is useful for hosts with a strict Content Security Policy (CSP). When provided, external URLs will be rendered in a nested iframe hosted at this URL. For example, if `proxy` is `https://my-proxy.com/`, the final URL will be `https://my-proxy.com/?url=&lt;encoded_original_url&gt;`. For your convenience, mcp-ui hosts a proxy script at `https://proxy.mcpui.dev`, which you can use as a the prop value without any setup (see `examples/external-url-demo`).
 - **`iframeProps`**: (Optional) Custom props for the iframe.
+- **`iframeRenderData`**: (Optional) Additional data merged into the render payload forwarded to the iframe. When `mcpContextProps.toolOutput` is provided, it merges with this data for the sandbox payload (with `toolOutput` fields taking precedence).
+- **`clientContextProps`**: (Optional) Host configuration forwarded to the sandboxed HTML (supports `theme`, `locale`, `userAgent`, `model`, `displayMode`, `maxHeight`, `safeArea`, and `capabilities`). When omitted, defaults are applied—`capabilities` defaults to `{ hover: true, touch: false }`.
 - **`autoResizeIframe`**: (Optional) When enabled, the iframe will automatically resize based on messages from the iframe&#39;s content. This prop can be a boolean (to enable both width and height resizing) or an object (`{width?: boolean, height?: boolean}`) to control dimensions independently.
</file context>
Suggested change
- **`iframeRenderData`**: (Optional) Additional data merged into the render payload forwarded to the iframe. When `mcpContextProps.toolOutput` is provided, it merges with this data for the sandbox payload (with `toolOutput` fields taking precedence).
- **`iframeRenderData`**: (Optional) Additional data merged into the render payload forwarded to the iframe. When `mcpContextProps.toolOutput` is provided, it replaces this data for the sandbox payload.
Fix with Cubic

capabilities,
})}\n`,
);
if (!/<head[^>]*>/i.test(htmlContent)) {
Copy link

@cubic-dev-ai cubic-dev-ai bot Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skybridge HTML without a tag never gets the runtime script injected, so those resources fail to initialize. Please add a fallback that prepends the script when neither nor is present.

Prompt for AI agents
Address the following comment on sdks/typescript/client/src/utils/processResource.ts at line 188:

<comment>Skybridge HTML without a &lt;html&gt; tag never gets the runtime script injected, so those resources fail to initialize. Please add a fallback that prepends the script when neither &lt;head&gt; nor &lt;html&gt; is present.</comment>

<file context>
@@ -132,13 +165,58 @@ export function processHTMLResource(
+          capabilities,
+        })}\n`,
+      );
+      if (!/&lt;head[^&gt;]*&gt;/i.test(htmlContent)) {
+        htmlContent = htmlContent.replace(
+          /&lt;html([^&gt;]*)&gt;/i,
</file context>
Fix with Cubic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants