-
Notifications
You must be signed in to change notification settings - Fork 4.8k
design-system-mcp: Update get_components to optionally support multiple names #78185
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
Changes from all commits
02c1271
e23c69f
b15e255
d97a8c7
30ca8b1
484a927
9fcbb05
bd08310
c59f6f5
47fabb5
b4c7534
c5fd61f
26b98f0
3c016f1
d851ecb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file is currently published in the npm release. We could move it to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Moved to Separately, I noticed that we don't have any type-checking on these files, both
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah agreed |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| #!/usr/bin/env node | ||
| import { readFile } from 'node:fs/promises'; | ||
| import assert from 'node:assert'; | ||
| import { | ||
| parseComponents, | ||
| parseComponentDetail, | ||
| } from '@wordpress/design-system-mcp'; | ||
|
|
||
| const path = process.argv[ 2 ] ?? 'storybook/build/manifests/components.json'; | ||
| const { components } = JSON.parse( await readFile( path, 'utf8' ) ); | ||
| const names = parseComponents( components ).map( ( { name } ) => name ); | ||
|
|
||
| assert( | ||
| names.length > 0, | ||
| `No components parsed from ${ path }. Manifest shape may have changed.` | ||
| ); | ||
|
|
||
| assert( | ||
| names.some( | ||
| ( name ) => parseComponentDetail( components, name )?.props.length > 0 | ||
| ), | ||
| `No components have parsed props. Manifest shape may have changed.` | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,60 @@ import { z } from 'zod'; | |
| import { getComponentDetail } from '../data'; | ||
| import { formatComponentDetail } from '../format'; | ||
|
|
||
| const inputSchema = z.object( { | ||
| name: z | ||
| .union( [ | ||
| z.string().min( 1 ), | ||
| z.array( z.string().min( 1 ) ).min( 1 ).max( 10 ), | ||
| ] ) | ||
| .describe( | ||
| 'A component name, or an array of component names to fetch in a single call (e.g. "Button" or ["Button", "Tabs"]).' | ||
| ), | ||
| } ); | ||
|
|
||
| export async function handler( { name }: z.infer< typeof inputSchema > ) { | ||
| const names = Array.isArray( name ) ? name : [ name ]; | ||
| const sections: string[] = []; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apologies in case this comment doesn't make much sense, but — seeing that Also, separately — how do we differentiate between components with the same name from different packages?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In other words, what I believe may be happening as of now:
Am I understanding correctly?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had designed it as an intentional constraint that there's at most one component of any given name in the design system (i.e. one "Button"), forcing us to choose between That being said, I've had it in my mind that the MCP could be more context-aware in ways that would reveal this kind of duplication:
As far as duplication, I think it's technically possible, but seems both unlikely and unproblematic to preemptively address. If we start seeing agents issuing tool calls with duplicate names, I feel like that suggests a bigger problem for why it chose to do that that should be addressed at the source rather than deduplicating the responses.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the extended context 👌 I agree with everything you said. Also, none of the above should be a blocker for this PR, anyway. |
||
| const missing: string[] = []; | ||
|
|
||
| for ( const componentName of names ) { | ||
| const detail = await getComponentDetail( componentName ); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small nit (feel free to ignore): the Conceptually, it could make sense to rewrite using
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I see what you're saying, that this is fine, but only because we're piercing an abstraction to know how I think it makes sense to change, but after starting to dig into it, it reveals a separate problem knock-on requirement to
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I created a separate pull request at #78311 with this refactor. We could either merge it into this branch or separately after this one lands.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely good as a follow-up after this PR gets merged, IMO |
||
| if ( detail ) { | ||
| sections.push( formatComponentDetail( detail ) ); | ||
| } else { | ||
| missing.push( componentName ); | ||
| } | ||
| } | ||
|
|
||
| if ( sections.length === 0 ) { | ||
| const list = missing.map( ( n ) => `"${ n }"` ).join( ', ' ); | ||
| return { | ||
| content: [ | ||
| { | ||
| type: 'text' as const, | ||
| text: `No components were found for: ${ list }.`, | ||
| }, | ||
| ], | ||
| isError: true, | ||
| }; | ||
| } | ||
|
|
||
| let text = sections.join( '\n\n---\n\n' ); | ||
| if ( missing.length > 0 ) { | ||
| const list = missing.map( ( n ) => `"${ n }"` ).join( ', ' ); | ||
| text += `\n\n---\n\n_No components were found for: ${ list }._`; | ||
| } | ||
|
|
||
| return { | ||
| content: [ | ||
| { | ||
| type: 'text' as const, | ||
| text, | ||
| }, | ||
| ], | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Register the get_component_details tool. | ||
| * | ||
|
|
@@ -14,38 +68,12 @@ export function register( server: McpServer ): void { | |
| { | ||
| title: 'Get Component Details', | ||
| description: | ||
| 'Get detailed documentation for a WordPress Design System component including props, usage examples, and import statements.', | ||
| inputSchema: z.object( { | ||
| name: z | ||
| .string() | ||
| .min( 1 ) | ||
| .describe( 'The component name (e.g. "Button", "Tabs")' ), | ||
| } ), | ||
| 'Get detailed documentation for one or more WordPress Design System components including props, usage examples, and import statements. Pass multiple names to fetch several components in a single call instead of making repeated calls.', | ||
| inputSchema, | ||
| annotations: { | ||
| readOnlyHint: true, | ||
| }, | ||
| }, | ||
| async ( { name } ) => { | ||
| const detail = await getComponentDetail( name ); | ||
| if ( ! detail ) { | ||
| return { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: `No component named "${ name }" was found.`, | ||
| }, | ||
| ], | ||
| isError: true, | ||
| }; | ||
| } | ||
| return { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: formatComponentDetail( detail ), | ||
| }, | ||
| ], | ||
| }; | ||
| } | ||
| handler | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| import { handler } from '../get-component-details'; | ||
| import { getComponentDetail } from '../../data'; | ||
| import { formatComponentDetail } from '../../format'; | ||
| import type { ComponentDetail } from '../../types'; | ||
|
|
||
| jest.mock( '../../data' ); | ||
|
|
||
| const mockGetComponentDetail = getComponentDetail as jest.MockedFunction< | ||
| typeof getComponentDetail | ||
| >; | ||
|
|
||
| function fakeDetail( name: string ): ComponentDetail { | ||
| return { | ||
| name, | ||
| description: `${ name } description.`, | ||
| packageName: '@wordpress/ui', | ||
| importStatement: `import { ${ name } } from '@wordpress/ui';`, | ||
| props: [], | ||
| stories: [], | ||
| }; | ||
| } | ||
|
Comment on lines
+12
to
+21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a chance that, since we're mocking this data, any real API response breakage won't be flagged by tests? Maybe we could generate mock data by running a real API call?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, I waffled quite a bit on just how much integration testing made sense here to give us confidence. With how this is set up currently, what you're describing probably falls more in the domain of the tests for Some other ideas we could consider to improve our confidence around the type of issue you describe:
Since the manifest we're loading is managed in the same project, this isn't really an external change outside our realm of control. The scenario you're describing would be a process failure where someone updates the manifest without updating dependencies in other parts of the same project.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we generate the manifest locally and validate its schema? Alternatively, boosting our confidence via type checks (using Storybook types), that would be a nice improvement. As a last resort, we could also consider adding some runtime validation — ie, when we receive the data payload, we analyze its shape and throw a warning / error. It won't prevent breakages, but at least it makes them obvious and allows us to fix them quickly. We could even instruct the agent to take that report and open a GH issue directly in the MCP response
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To address this, I did a bit of both:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like the manifest validation check is failing in CI, likely because the Maybe we can change the check from
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think you're right. I was a bit confused because this was working locally for me, which would somehow imply that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although I think in newer versions Storybook now defaults |
||
|
|
||
| describe( 'handler', () => { | ||
| beforeEach( () => { | ||
| mockGetComponentDetail.mockReset(); | ||
| } ); | ||
|
|
||
| it( 'returns a single formatted section for a string name', async () => { | ||
| const button = fakeDetail( 'Button' ); | ||
| mockGetComponentDetail.mockResolvedValueOnce( button ); | ||
|
|
||
| const result = await handler( { name: 'Button' } ); | ||
|
|
||
| expect( mockGetComponentDetail ).toHaveBeenCalledTimes( 1 ); | ||
| expect( mockGetComponentDetail ).toHaveBeenCalledWith( 'Button' ); | ||
| expect( result ).toEqual( { | ||
| content: [ | ||
| { type: 'text', text: formatComponentDetail( button ) }, | ||
| ], | ||
| } ); | ||
| } ); | ||
|
|
||
| it( 'joins multiple components with the section separator', async () => { | ||
| const button = fakeDetail( 'Button' ); | ||
| const tabs = fakeDetail( 'Tabs' ); | ||
| mockGetComponentDetail | ||
| .mockResolvedValueOnce( button ) | ||
| .mockResolvedValueOnce( tabs ); | ||
|
|
||
| const result = await handler( { name: [ 'Button', 'Tabs' ] } ); | ||
|
|
||
| expect( mockGetComponentDetail.mock.calls ).toEqual( [ | ||
| [ 'Button' ], | ||
| [ 'Tabs' ], | ||
| ] ); | ||
| expect( result ).toEqual( { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: `${ formatComponentDetail( | ||
| button | ||
| ) }\n\n---\n\n${ formatComponentDetail( tabs ) }`, | ||
| }, | ||
| ], | ||
| } ); | ||
| } ); | ||
|
|
||
| it( 'appends a missing footer when some names are not found', async () => { | ||
| const button = fakeDetail( 'Button' ); | ||
| mockGetComponentDetail | ||
| .mockResolvedValueOnce( button ) | ||
| .mockResolvedValueOnce( null ); | ||
|
|
||
| const result = await handler( { name: [ 'Button', 'Nope' ] } ); | ||
|
|
||
| expect( result ).toEqual( { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: `${ formatComponentDetail( | ||
| button | ||
| ) }\n\n---\n\n_No components were found for: "Nope"._`, | ||
| }, | ||
| ], | ||
| } ); | ||
| } ); | ||
|
|
||
| it( 'quotes and comma-joins multiple missing names in the footer', async () => { | ||
| const button = fakeDetail( 'Button' ); | ||
| mockGetComponentDetail | ||
| .mockResolvedValueOnce( button ) | ||
| .mockResolvedValueOnce( null ) | ||
| .mockResolvedValueOnce( null ); | ||
|
|
||
| const result = await handler( { | ||
| name: [ 'Button', 'Nope', 'AlsoNope' ], | ||
| } ); | ||
|
|
||
| expect( result ).toEqual( { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: `${ formatComponentDetail( | ||
| button | ||
| ) }\n\n---\n\n_No components were found for: "Nope", "AlsoNope"._`, | ||
| }, | ||
| ], | ||
| } ); | ||
| } ); | ||
|
|
||
| it( 'returns isError when no components are found', async () => { | ||
| mockGetComponentDetail.mockResolvedValue( null ); | ||
|
|
||
| const result = await handler( { name: [ 'Foo', 'Bar' ] } ); | ||
|
|
||
| expect( result ).toEqual( { | ||
| content: [ | ||
| { | ||
| type: 'text', | ||
| text: 'No components were found for: "Foo", "Bar".', | ||
| }, | ||
| ], | ||
| isError: true, | ||
| } ); | ||
| } ); | ||
| } ); | ||
Uh oh!
There was an error while loading. Please reload this page.