Conversation
|
FYI - I'm just going to merge this, we can revisit when you are back @aharvard |
There was a problem hiding this comment.
Pull request overview
This PR simplifies the MCP Apps implementation by consolidating message handling logic and moving type definitions to be generated from Rust schemas, reducing code duplication and improving maintainability.
Key Changes
- Removed ~400 lines of TypeScript by consolidating JSON-RPC helper functions and message handlers into inline implementations
- Moved MCP App type definitions from manual TypeScript to Rust-generated types via OpenAPI schema
- Simplified message handling by replacing individual handler callbacks with a single unified
onMcpRequestcallback
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/desktop/src/components/McpApps/utils.ts | Removed all JSON-RPC helpers and message handlers; now only contains proxy URL fetching |
| ui/desktop/src/components/McpApps/types.ts | Simplified to re-export generated types from Rust; removed manual type definitions |
| ui/desktop/src/components/McpApps/useSandboxBridge.ts | Refactored to inline message construction; replaced individual handlers with unified callback |
| ui/desktop/src/components/McpApps/McpAppRenderer.tsx | Consolidated all MCP request handling into single switch statement |
| ui/desktop/src/api/types.gen.ts | Added generated types for MCP App resources from OpenAPI schema |
| ui/desktop/openapi.json | Added OpenAPI schemas for McpAppResource, CspMetadata, UiMetadata, ResourceMetadata |
| crates/goose/src/goose_apps/* | New Rust module defining MCP App resource types with OpenAPI schema annotations |
| crates/goose-server/src/routes/agent.rs | Added _meta field to CallToolResponse |
| crates/goose-server/src/openapi.rs | Registered new goose_apps types in OpenAPI documentation |
| useEffect(() => { | ||
| return () => { | ||
| if (isGuestInitializedRef.current) { | ||
| const { message } = createResourceTeardownRequest('Component unmounting'); | ||
| sendToSandbox(message); | ||
| if (isGuestInitialized) { | ||
| sendToSandbox({ | ||
| jsonrpc: '2.0', | ||
| id: Date.now(), | ||
| method: 'ui/resource-teardown', | ||
| params: { reason: 'Component unmounting' }, | ||
| }); | ||
| } | ||
| }; | ||
| }, [sendToSandbox]); | ||
| }, [isGuestInitialized, sendToSandbox]); |
There was a problem hiding this comment.
The cleanup effect now depends on isGuestInitialized state, but this will recreate the effect every time isGuestInitialized changes. This means the cleanup function will be recreated and will use stale state.
In the original code, the ref (isGuestInitializedRef) was used to capture the latest value at cleanup time. Without this, the cleanup may not run when the component unmounts if isGuestInitialized was false at the time the effect was created. Consider using a ref to track the initialized state for cleanup purposes.
| displayMode: 'inline' | 'fullscreen' | 'standalone'; | ||
| availableDisplayModes: ('inline' | 'fullscreen' | 'standalone')[]; |
There was a problem hiding this comment.
The HostContext type now includes displayMode options 'fullscreen' and 'standalone', but the actual implementation in useSandboxBridge.ts only sets displayMode to 'inline' and availableDisplayModes to ['inline']. This creates an inconsistency where the type promises functionality that isn't implemented.
Either update the implementation to support these display modes or restrict the type definition to only include 'inline' until these modes are actually supported.
| displayMode: 'inline' | 'fullscreen' | 'standalone'; | |
| availableDisplayModes: ('inline' | 'fullscreen' | 'standalone')[]; | |
| displayMode: 'inline'; | |
| availableDisplayModes: 'inline'[]; |
| safeAreaInsets?: { | ||
| top: number; | ||
| right: number; | ||
| bottom: number; | ||
| left: number; | ||
| }; |
There was a problem hiding this comment.
The safeAreaInsets field changed from required to optional. However, the implementation in useSandboxBridge.ts always provides these insets (set to 0). This change could break guest apps that rely on safeAreaInsets always being present. Consider whether this field should remain required or if the implementation should conditionally omit it.
| useEffect(() => { | ||
| if (!isGuestInitialized || !toolInput) return; | ||
| sendToSandbox(createToolInputNotification(toolInput)); | ||
| }, [isGuestInitialized, toolInput, sendToSandbox]); | ||
| if (!isGuestInitialized) return; | ||
|
|
||
| // Send partial tool input (streaming) when guest is initialized | ||
| useEffect(() => { | ||
| if (!isGuestInitialized || !toolInputPartial) return; | ||
| sendToSandbox(createToolInputPartialNotification(toolInputPartial)); | ||
| }, [isGuestInitialized, toolInputPartial, sendToSandbox]); | ||
| if (toolInput) { | ||
| sendToSandbox({ | ||
| jsonrpc: '2.0', | ||
| method: 'ui/notifications/tool-input', | ||
| params: { arguments: toolInput.arguments }, | ||
| }); | ||
| } | ||
|
|
||
| // Send tool result when guest is initialized and result is available | ||
| useEffect(() => { | ||
| if (!isGuestInitialized || !toolResult) return; | ||
| sendToSandbox(createToolResultNotification(toolResult)); | ||
| }, [isGuestInitialized, toolResult, sendToSandbox]); | ||
| if (toolInputPartial) { | ||
| sendToSandbox({ | ||
| jsonrpc: '2.0', | ||
| method: 'ui/notifications/tool-input-partial', | ||
| params: { arguments: toolInputPartial.arguments }, | ||
| }); | ||
| } | ||
|
|
||
| // Send tool cancelled notification when toolCancelled changes | ||
| useEffect(() => { | ||
| if (!isGuestInitialized || !toolCancelled) return; | ||
| sendToSandbox(createToolCancelledNotification(toolCancelled)); | ||
| }, [isGuestInitialized, toolCancelled, sendToSandbox]); | ||
| if (toolResult) { | ||
| sendToSandbox({ | ||
| jsonrpc: '2.0', | ||
| method: 'ui/notifications/tool-result', | ||
| params: toolResult, | ||
| }); | ||
| } | ||
|
|
||
| if (toolCancelled) { | ||
| sendToSandbox({ | ||
| jsonrpc: '2.0', | ||
| method: 'ui/notifications/tool-cancelled', | ||
| params: toolCancelled.reason ? { reason: toolCancelled.reason } : {}, | ||
| }); | ||
| } | ||
| }, [isGuestInitialized, toolInput, toolInputPartial, toolResult, toolCancelled, sendToSandbox]); |
There was a problem hiding this comment.
This combined useEffect will re-send ALL notifications whenever ANY of the dependencies change. For example, if toolInput changes, it will re-send toolInputPartial, toolResult, and toolCancelled if they are defined. This could lead to duplicate or out-of-order notifications being sent to the guest app.
The original implementation had separate useEffects for each notification type, which ensured each notification was sent only when that specific value changed. Consider splitting these back into separate useEffects, or add logic to track which values actually changed.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ck/goose into feat/add-mcp-app-renderer-v2
2 participants
Summary
Simplify things a bit. Frittered lives and all that