-
Notifications
You must be signed in to change notification settings - Fork 10
remove source API and use the request instead #54
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: next
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 68dcf87 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Bundle ReportChanges will increase total bundle size by 611 bytes (1.62%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: @storybook/mcp-esmAssets Changed:
Files in
view changes for bundle: @storybook/addon-mcp-esmAssets Changed:
Files in
|
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #54 +/- ##
==========================================
+ Coverage 81.91% 82.12% +0.21%
==========================================
Files 15 15
Lines 846 856 +10
Branches 159 154 -5
==========================================
+ Hits 693 703 +10
Misses 145 145
Partials 8 8 ☔ View full report in Codecov by Sentry. |
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 refactors the MCP manifest loading mechanism to use the HTTP Request object instead of a string source URL. This change makes the API more flexible by allowing custom manifest providers to extract additional context (headers, origin, etc.) from the request rather than just a URL string.
Key changes:
- The
getManifest()function now accepts aRequestobject instead of asourceURL string - A new
getManifestUrlFromRequest()helper constructs the manifest URL by replacing/mcpwith/manifests/components.json - The
manifestProvidersignature changed from(source: string) => Promise<string>to(request: Request) => Promise<string>
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/mcp/src/utils/get-manifest.ts | Refactored to accept Request object, added URL construction helper, updated default provider |
| packages/mcp/src/utils/get-manifest.test.ts | Updated all tests to pass Request objects via helper function |
| packages/mcp/src/types.ts | Changed StorybookContext to use request property instead of source |
| packages/mcp/src/tools/list-all-components.ts | Updated to pass request instead of source to getManifest |
| packages/mcp/src/tools/list-all-components.test.ts | Updated tests to include request in context |
| packages/mcp/src/tools/get-component-documentation.ts | Updated to pass request instead of source to getManifest |
| packages/mcp/src/tools/get-component-documentation.test.ts | Updated tests to include request in context |
| packages/mcp/src/index.ts | Modified handler to pass request object to transport.respond() |
| packages/mcp/serve.ts | Updated development server to use request parameter in manifestProvider |
| packages/addon-mcp/src/mcp-handler.ts | Changed context to pass request instead of source URL |
| .github/instructions/mcp.instructions.md | Added documentation for request-based context and manifest provider API |
| .github/copilot-instructions.md | Updated architecture descriptions to reflect request-based approach |
| .changeset/hip-sloths-jog.md | Added changeset documenting the breaking change |
| // Custom logic: read from local filesystem based on request | ||
| const url = new URL(request.url); | ||
| const manifestPath = `/path/to/manifests${url.pathname.replace('/mcp', '/components.json')}`; |
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.
Longer term we'll be splitting the manifests up right? So it doesn't make sense for the user to do this substitution -- we should be telling them what file we expect them to grab.
tmeasday
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.
@JReinhold I think we are misaligned on this. Perhaps we need to quickly sync on a call.
|
We discussed that figuring out the path should be handled internally. Eg. a consumer of this API, can use |
Replace the
sourceproperty in the context withrequest.Now you don't pass in a source string that might be fetched or handled by your custom
manifestProvider, but instead you pass in the whole web request. (This is automatically handled if you use the createStorybookMcpHandler() function).The default action is now to fetch the manifest from
../manifests/components.jsonassuming the server is running at./mcp. Your custommanifestProvider()-function then also does not get a source string as an argument, but gets the whole web request, that you can use to get information about where to fetch the manifest from.