-
Notifications
You must be signed in to change notification settings - Fork 2.4k
MCP Apps Plumbing #6286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MCP Apps Plumbing #6286
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR connects the desktop client to the actual server for MCP Apps functionality, replacing mock data with real API calls.
- Adds sessionId prop threading from GooseMessage through ToolCallWithResponse to McpAppRenderer
- Implements dynamic resource fetching via readResource API
- Enables MCP apps to call tools and read resources through the backend
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ui/desktop/src/components/GooseMessage.tsx | Passes sessionId to ToolCallWithResponse component for API authentication |
| ui/desktop/src/components/ToolCallWithResponse.tsx | Adds maybeRenderMCPApp helper that extracts resourceUri from tool metadata and conditionally renders MCP apps |
| ui/desktop/src/components/McpApps/McpAppRenderer.tsx | Replaces static resource prop with dynamic fetching via readResource API; implements tools/call and resources/read handlers for guest app requests |
| if (response.data) { | ||
| setResourceHtml(response.data.html); | ||
| // TODO: Extract CSP from resource metadata when backend supports it | ||
| // For now, CSP will be null and the proxy will use default restrictions | ||
| } | ||
| } catch (err) { |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect is missing proper error handling for the case when response.data is null or undefined. If the API returns a successful response but with null data, setResourceHtml will be called with undefined, but no error state will be set. Consider adding a check and setting an error if data is missing.
| if (response.data) { | |
| setResourceHtml(response.data.html); | |
| // TODO: Extract CSP from resource metadata when backend supports it | |
| // For now, CSP will be null and the proxy will use default restrictions | |
| } | |
| } catch (err) { | |
| if (response.data && typeof response.data.html === 'string') { | |
| setResourceHtml(response.data.html); | |
| setError(null); | |
| // TODO: Extract CSP from resource metadata when backend supports it | |
| // For now, CSP will be null and the proxy will use default restrictions | |
| } else { | |
| setResourceHtml(null); | |
| setError('Resource data is missing or invalid'); | |
| } | |
| } catch (err) { | |
| setResourceHtml(null); |
| toolInput={{ arguments: toolCall.arguments }} | ||
| extensionName={extensionName} | ||
| sessionId={sessionId} | ||
| append={append} |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The toolResult prop is not being passed to McpAppRenderer. This will prevent the guest app from receiving tool result notifications via the ui/notifications/tool-result method. Consider passing the toolResult by extracting it from resultWithMeta.value, which should match the ToolResult type expected by the component.
| append={append} | |
| append={append} | |
| toolResult={value} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
|
|
||
| if (msg.method === 'ui/notifications/initialized') { | ||
| setIsGuestInitialized(true); | ||
| console.log('initialized -> True'); |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console.log statement should be removed before merging to production.
| return { | ||
| content: response.data?.content || [], | ||
| isError: response.data?.is_error || false, | ||
| structuredContent: (response.data as any)?.structured_content, |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using 'as any' bypasses type safety. Consider defining a proper type for structured_content in the response type or use a more specific type assertion.
| return { | |
| content: response.data?.content || [], | |
| isError: response.data?.is_error || false, | |
| structuredContent: (response.data as any)?.structured_content, | |
| type ToolsCallStructuredContent = McpMethodResponse['tools/call']['structuredContent']; | |
| const structuredContent = | |
| (response.data as { structured_content?: ToolsCallStructuredContent } | undefined) | |
| ?.structured_content; | |
| return { | |
| content: response.data?.content || [], | |
| isError: response.data?.is_error || false, | |
| structuredContent, |
|
|
||
| useEffect(() => { | ||
| setIsGuestInitialized(false); | ||
| console.log('initialized -> False'); |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console.log statement should be removed before merging to production.
| }, [resourceUri]); | ||
|
|
||
| const sendToSandbox = useCallback((message: JsonRpcMessage) => { | ||
| console.log('>>>', message); |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console.log statement should be removed before merging to production.
|
|
||
| const handleJsonRpcMessage = useCallback( | ||
| async (data: unknown) => { | ||
| console.log('jsonResponse', data); |
Copilot
AI
Dec 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Console.log statement should be removed before merging to production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this file change need to be part of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really; we're dragging in session-ids into sessionviews and so this would have to maybe updated but then it is not used
aharvard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
nice - cli code isn't entirely happy: error[E0063]: missing field is best way to test it what @aharvard did? |
| icons: None, | ||
| title: None, | ||
| meta: None, | ||
| icons: tool.icons, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh - so will show the tool it is adding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is just for completion sake. we're not really using icons as far as I can tell, but dropping these values on the floor can only be bad :)
|
|
||
| export type ReadResourceResponse = { | ||
| html: string; | ||
| _meta?: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this seems a reasonable extension to what I remember from mcp ui, nice, a generalisation for apps
@michaelneale if you want to give it a spin... endpoint: https://mcp-app-bench.onrender.com/mcp prompt: "run mcp app bench" |
| cancellation_token.clone(), | ||
| true, | ||
| ) | ||
| let read_result = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not your code but if thing.is_some() { ... thing.unwrap() ... } should be an if let Some(inner) = thing {...
|
|
||
| let toolResult: CallToolResponse | undefined; | ||
| if (toolResponse) { | ||
| const resultWithMeta = toolResponse.toolResult as ToolResultWithMeta; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we type this properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you see a way? I've been going back and forth here, but toolResponse comes through the API from the rcmp classes which have a hard to time getting converted to proper typescript, so we end up with something semi-typed that we cast

Summary
Hook the client up to the actual sever. Works now with at least some MCP servers