-
Notifications
You must be signed in to change notification settings - Fork 2.3k
WIP #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
base: main
Are you sure you want to change the base?
WIP #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} |
Summary
Hook the client up to the actual sever