-
Notifications
You must be signed in to change notification settings - Fork 170
feat: implement MCP-UI support #783
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?
Conversation
d484367 to
e223056
Compare
- Added UIRegistry class to manage custom and default UI HTML for tools. - Integrated custom UI support in the Server and ToolBase classes. - Created ListDatabases tool with a corresponding UI component. - Introduced Vite configuration for building UI components and generating HTML entries. - Updated TypeScript configuration to support JSX and include UI components. - Enhanced package.json with new dependencies for React and Vite. - Updated .gitignore to exclude generated UI files.
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 adds MCP-UI support to the MongoDB MCP server, enabling tools to render rich React-based UIs in compatible clients. The implementation includes build tooling for React components, a UI registry system for managing component HTML, and an initial UI component for the list-databases tool.
Key changes:
- Adds build infrastructure using Vite to compile React components into self-contained HTML files
- Implements a UI registry system that maps tool names to UI components and manages HTML loading
- Updates the
list-databasestool to return structured content and automatically attach UI resources
Reviewed changes
Copilot reviewed 18 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.ui.config.ts | New Vite configuration for building React UI components into standalone HTML files |
| tsconfig.build.json | Enables JSX compilation for React components |
| src/ui/registry/uiMap.ts | Maps tool names to UI component names |
| src/ui/registry/registry.ts | Core UIRegistry class for managing and loading UI HTML strings |
| src/ui/registry/index.ts | Registry module exports |
| src/ui/index.ts | Main UI module exports |
| src/ui/hooks/useRenderData.ts | React hook for receiving render data via postMessage |
| src/ui/hooks/index.ts | Hooks module exports |
| src/ui/components/ListDatabases/schema.ts | Zod schema defining list-databases output contract |
| src/ui/components/ListDatabases/index.ts | ListDatabases component module exports |
| src/ui/components/ListDatabases/ListDatabases.tsx | React component rendering database list as a table |
| src/ui/components/ListDatabases/ListDatabases.styles.ts | Styles for ListDatabases component |
| src/ui/build/template.html | HTML template for generated component entry files |
| src/ui/build/mount.tsx | Entry point that mounts React components in the browser |
| src/tools/tool.ts | Adds UIRegistry integration and automatic UI resource attachment to tools |
| src/tools/mongodb/metadata/listDatabases.tsx | Updates tool to return structured content for UI rendering |
| src/server.ts | Adds UIRegistry instantiation and customUIs configuration option |
| package.json | Adds React, Vite, and MCP-UI dependencies plus UI build script |
src/tools/tool.ts
Outdated
| } | ||
|
|
||
| const uiResource = createUIResource({ | ||
| uri: `ui://${this.name}/${Date.now()}`, |
Copilot
AI
Dec 5, 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 Date.now() for URI generation creates non-deterministic URIs that change on each execution. This makes testing difficult and prevents URI-based caching. Consider using a deterministic approach or accepting a URI parameter that can be controlled in tests.
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.
I'm not sure we'd want this deterministic. If the same tool is called multiple times, each invocation generates new data, so unique URIs ensure each result is treated as a distinct resource. Open to other ideas here if my thinking is incorrect!
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 sure how the caching works exactly, but I think a static uri would be more performant? I don't think it caches the uiMetadata
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.
I would also experiment without the Date component - since the html is static, it lends itself well to caching, as long as the client correctly provides the distinct initial-render-data for each tool call.
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 examples in the spec always show an instance ID but removing date altogether doesn't appear to break anything, so this seems to be ok 🚀
Pull Request Test Coverage Report for Build 20070067456Details
💛 - Coveralls |
package.json
Outdated
| "dist" | ||
| ], | ||
| "scripts": { | ||
| "start": "node dist/index.js --transport http --loggers stderr mcp --previewFeatures vectorSearch", |
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.
looks like this was renamed
| "start": "node dist/index.js --transport http --loggers stderr mcp --previewFeatures search", |
vite.ui.config.ts
Outdated
| * Discover all component directories in src/ui/components/ | ||
| * Each directory should have an index.ts that exports the component | ||
| */ | ||
| function discoverComponents(): string[] { |
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.
nit: why discover?
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.
Removed this completely in the change to inline HTML strings
vite.ui.config.ts
Outdated
| }; | ||
| } | ||
|
|
||
| const components = discoverComponents(); |
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.
nit: we're running this search here, and then again in the generateHtmlEntries plugin. Could either use this pre-computed value in the plugin, or memoize it to speed up builds a bit
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.
Fixed this completely in the change to inline HTML strings
package.json
Outdated
| "react": "^18.3.0", | ||
| "react-dom": "^18.3.0", |
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.
dependencies or devDependencies?
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.
Oh yeah, good point! Will update to dev
src/tools/tool.ts
Outdated
| * @param result - The result from the tool's `execute()` method | ||
| * @returns The result with UIResource appended if conditions are met, otherwise unchanged | ||
| */ | ||
| private appendUIResourceIfAvailable(result: CallToolResult): CallToolResult { |
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.
nit: not sure if this is a naming convention across the repo, but just appendUIResource with docs for what happens if it's not available feels more correct
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 argument on this from me. "if available" just seemed more technically correct based on what the method does, but I don't see an issue with shortening it.
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 need to be .tsx?
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.
Good catch! I changed this at the very beginning so I could inline the jsx and validate things were working locally. Will change back
nirinchev
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.
Did a first pass at this - I haven't looked at the actual UI logic at all, just reviewed the integration points.
| import type * as bson from "bson"; | ||
| import type { OperationType } from "../../tool.js"; | ||
| import { formatUntrustedData } from "../../tool.js"; | ||
| import { ListDatabasesOutputSchema, type ListDatabasesOutput } from "../../../ui/components/ListDatabases/schema.js"; |
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.
listDatabases should not depend/reference the UI components. Instead, these schemas should be exported here and imported in the UI component.
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.
This unfortunately isn't possible, I don't think. The UI gets rendered off of the server and wouldn't have access to a server export in the bundle, so it creates a dependency issue on bundle. I understand that not all tools will have UI's but all tools will have schemas, but I couldn't see a way around this aside from duplicating the schema. Definitely open to ideas here though!
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.
@TheSonOfThomp Curious if you have any ideas here?
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.
Actually another kind of related issue.
In reality since this is feature flagged, there might need to be two separate schemas, for now. Obviously we want to support outputSchema for non-UI augmented responses. But if our outputSchema is generated expecting UI in the response, like in list-databases, and the feature is disabled, the client will deem the response invalid. So I think for now at least, UI schemas, and tool schemas, kind of need to be separate my necessity.
This doesn't change the fact that the tool is importing a UI schema, which I think you were trying to avoid.
I'm pushing up a suggested change where there is a separate ToolBase.uiOutputSchema prop that's overrides the outputSchema in the response if it there's UI and the feature is enabled. This will allow a text only schema as well.
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.
UI gets rendered off of the server
my impression was that all UI-related things are at built time and not the bundle, do I not understand that correctly?
very roughly what I imagining can we not make tool.uiSchema is imported by i.e. component-generator and then the generated getComponent() is imported by tool?
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.
alternative thing that maybe matches what you're saying:
why have UI <-> tool be so deeply tied at all?
can't we just have strongly typed UI text generators function and let any tool import them as needed?
i.e. (again very rough idea)
// in UI src
function getListDatabasesHtml({ databases: Database[]): string;
// ...
// in a tool class
import { getListDatabasesHtml } from 'mongodb-mcp-server/ui';
class SomeTool { getUI() => getListDatabasesHtml({ databases }); }}; // this returns a stringthen these generated UI components are a lot more reusable
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.
my impression was that all UI-related things are at built time and not the bundle, do I not understand that correctly?
As in, they're JIT bundling as opposed to doing so at build? This was something we went back and forth on in our last sync, but we ended up landing on bundling at build. I forget fully why, but we're not bundling on request. Please let me know if I'm misunderstanding the question!
can't we just have strongly typed UI text generators function and let any tool import them as needed?
i.e. (again very rough idea)
If I'm understanding correctly, this was the original design to some extent in 0f9a812 where the tool itself was getting the UI. The issue communicated was that this pushed the UI business logic up to the tool itself. I do think the current design of not having to do anything in the tool to get the UI is quite nice.
src/tools/tool.ts
Outdated
| /** | ||
| * Get the UI HTML string for this tool from the registry. | ||
| * Returns the registered UI HTML, or undefined if no UI exists for this tool. | ||
| */ | ||
| protected getUI(): string | undefined { | ||
| return this.uiRegistry?.get(this.name); | ||
| } |
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.
Do we need this now that it's all abstracted away in the base tool? I don't suppose inheritors need to reference getUI, so we should probably just fetch the HTML when needed.
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.
Good call, its only ever used in the class now. Will remove!
src/tools/tool.ts
Outdated
| if (this.outputSchema) { | ||
| const schema = z.object(this.outputSchema); | ||
| const validation = schema.safeParse(result.structuredContent); | ||
| if (!validation.success) { | ||
| this.session.logger.warning({ | ||
| id: LogId.toolExecute, | ||
| context: `tool - ${this.name}`, | ||
| message: `structuredContent failed validation against outputSchema, skipping UI resource: ${validation.error.message}`, | ||
| }); | ||
| return result; | ||
| } | ||
| } |
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.
I don't think we should be doing this validation here. Right now we're in a bit of a flux since we don't have fully-fleshed out structured content support, but eventually, we'd like to use the type system to ensure structuredContent matches the schema. For this PR, I'd suggest that we just trust that it's adhering to the schema, without doing any manual validations.
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.
That works for me. This was sort of an "extra layer" I added since we now had outputSchema that would prevent a UI response from being appended at all if we knew the validation was broken. Without this the only downside is that a UI could be theoretically be sent and rendered in an error state since data is invalid. I'm ok with trusting it at this point though. Will remove.
src/tools/tool.ts
Outdated
| } | ||
|
|
||
| const uiResource = createUIResource({ | ||
| uri: `ui://${this.name}/${Date.now()}`, |
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.
I would also experiment without the Date component - since the html is static, it lends itself well to caching, as long as the client correctly provides the distinct initial-render-data for each tool call.
| * Custom UIs can be provided at runtime to override or extend the defaults. | ||
| */ | ||
| export class UIRegistry { | ||
| private customUIs: Map<string, string> = new Map(); |
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.
We should probably make this lazy, otherwise it will load a bunch of long strings on startup. I'm fine with caching the values, but we should only load them in memory when needed.
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.
That makes sense, will update
src/ui/registry/registry.ts
Outdated
| @@ -0,0 +1,47 @@ | |||
| import { uiHtml } from "../generated/uiHtml.js"; | |||
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.
This seems to be .gitignored - while I know there's different opinions about generated code, I think I am more in favor of committing it as it makes it easier to review what's going on without checking out the repo and building it locally. If you are worried about it polluting the diff view, you can add the linguist-generated git attribute.
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.
I'm okay with this. Yeah, I knew these bundles were quite large so I didn't know if you all would want them committed to the repo. Happy to go that way with it though
src/ui/registry/uiMap.ts
Outdated
| export const uiMap: Record<string, string> = { | ||
| "list-databases": "ListDatabases", |
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.
Should we just make it a convention instead of keeping a map like this? Then at build time, we can discover all components and flag cases where a component name doesn't have a corresponding tool name.
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.
Convention was actually the original design. I changed it to use a map in order to hedge against a possible scenario where a component has a different name then a tool but I guess there is always a 1:1 mapping so perhaps that will/should never exist. I was also thinking the explicit config/map prevented "magic" lol, since adding it to the map was explicit, whereas with convention once the component exists, its being bundled. This all said, I was on the fence about this already so definitely okay with making it a convention. It will make creating a UI one step easier. Will update the build and remove the mapping.
package.json
Outdated
| "@emotion/css": "^11.13.5", | ||
| "@leafygreen-ui/table": "^15.2.2", |
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.
[Noob question] Should these be dependencies or devDependencies?
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.
Nope that's a great question, they are definitely dev dependencies in this case. Updating!
| * }); | ||
| * ``` | ||
| */ | ||
| customUIs?: Record<string, string>; |
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 this information live in the tool itself?
.gitignore
Outdated
| .accuracy | ||
|
|
||
| # Generated UI module (rebuilt by `pnpm build:ui`) | ||
| src/ui/generated |
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.
would it be more conventional for this to be in like lib/?
…o remove generated UI module path
…TML generation logic
Summary
list-databasetool callsTesting
I've been using MCPJam Inspector which has MCP-UI implemented in the client.
Screenshot