Conversation
dmandar
requested review from
ditman,
gspencergoog,
jacobsimionato,
nan-yu,
yjbanov and
zeroasterisk
as code owners
- Adds McpAppsCustomComponent using the official @modelcontextprotocol/ext-apps SDK. - Implements double-iframe sandbox isolation (sandbox.html, sandbox.ts). - Upgrades the backend MCP server (floor_plan_server.py) to a persistent SSE architecture using Starlette. - Configures agent.py to connect via mcp.client.sse. - Refines Vite configuration for seamless dual-origin local development.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates MCP Apps into A2UI by adding a new McpAppsCustomComponent, a double-iframe sandbox for security, and a persistent SSE backend. The changes are significant and well-structured. My review focuses on improving security and configuration management. I've identified critical security issues related to postMessage usage that should be addressed. Additionally, there are opportunities to improve maintainability by removing hardcoded URLs and making the code more robust. The repository's style guide requires tests for new code (line 17), which seem to be missing for the new components and server logic. Please consider adding tests to ensure the stability and correctness of these new features.
samples/agent/adk/contact_multiple_surfaces/floor_plan_server.py
Outdated
Show resolved
Hide resolved
samples/agent/adk/contact_multiple_surfaces/floor_plan_server.py
Outdated
Show resolved
Hide resolved
samples/agent/adk/contact_multiple_surfaces/floor_plan_server.py
Outdated
Show resolved
Hide resolved
samples/client/lit/contact/ui/custom-components/register-components.ts
Outdated
Show resolved
Hide resolved
samples/client/lit/contact/ui/custom-components/mcp-apps-component.ts
Outdated
Show resolved
Hide resolved
- Extract location action logic in ContactAgent.stream into _handle_action - Provide code comments clarifying action intent for UI surfaces - Extract location-surface into LOCATION_SURFACE_ID constant - Rename McpAppsCustomComponent to McpApp
docs/scripts/convert_docs.py
Outdated
|
|
||
| # Registry for bidirectional format conversion: | ||
| # | ||
| # |
There was a problem hiding this comment.
Can we revert lines that are non-operational to reduce the change needed for review to be limited to operational changes?
…d fix a2ui schema validation for multi-surface payloads
gspencergoog
left a comment
gspencergoog
left a comment
There was a problem hiding this comment.
Let's get this in.
| # Load the first inline catalog schema. | ||
| inline_catalog_schema = inline_catalogs[0] | ||
| inline_catalog_schema = self._apply_modifiers(inline_catalog_schema) | ||
|
|
There was a problem hiding this comment.
This is a bit of a code smell to me.
In line 139 we add a prescription that client can only provide one or the other; inline or supported catalog.
But here, we are now implicitly baking the standard catalog.
1 ) Clients might NOT want standard catalog components included in responses.
2) component name overlaps could result in unexpected behavior without safe-guards
Can we revert this change and perhaps consider a more hi-fi solution to achieve this effect?
| surface_id = None | ||
| if "surfaceUpdate" in message: # v0.8 | ||
| components = message["surfaceUpdate"].get(COMPONENTS) | ||
| surface_id = message["surfaceUpdate"].get("surfaceId") |
There was a problem hiding this comment.
We should use constants for "surfaceId"
| ref_map = _extract_component_ref_fields(self._catalog) | ||
| root_id = _find_root_id(messages, surface_id) | ||
| _validate_component_integrity(root_id, components, ref_map) | ||
| _validate_topology(root_id, components, ref_map) |
There was a problem hiding this comment.
Making root_id optional and the validation to be prone to pass when root id is absent seems making this validator brittle and false-negative prone.
| inner.style.cssText = "width:100%; height:100%; border:none;"; | ||
| inner.setAttribute("sandbox", "allow-scripts allow-same-origin allow-forms"); | ||
| document.body.appendChild(inner); | ||
|
|
There was a problem hiding this comment.
Can't we import these from the library so that we aren't exposed to misalignment in the future?
import {
SANDBOX_PROXY_READY_METHOD,
SANDBOX_RESOURCE_READY_METHOD
} from '@modelcontextprotocol/ext-apps/app-bridge';
4 participants
Description
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. For larger changes, raising an issue first helps reduce redundant work.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.