Repository navigation
Skills M4: Run addon-mcp and @storybook/mcp on the shared core toolsets - #35858
Conversation
JReinhold
added
maintenance
WalkthroughThe MCP packages now adapt shared Storybook toolsets. The addon registry and hosted documentation package use shared metadata, schemas, outcomes, telemetry, manifest access, and composition. Obsolete tool implementations, manifest utilities, review-channel plumbing, and local package overrides were removed. ChangesShared MCP toolset migration
Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant AddonMcp
participant ToolRegistry
participant SharedToolset
participant DocsAccess
MCPClient->>AddonMcp: send MCP request
AddonMcp->>ToolRegistry: resolve registered tool
ToolRegistry->>SharedToolset: invoke method with MCP context
SharedToolset->>DocsAccess: read local or composed documentation data
DocsAccess-->>SharedToolset: return data or outcome
SharedToolset-->>MCPClient: return MCP content and structured output
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
code/lib/mcp/src/dist-contract.test.ts (1)
88-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnchor the
reactcheck to module references.The comment on lines 88-90 states the intent is to reject module references to
react. The regex on line 91 matches any quoted occurrence ofreactfollowed by a quote or a slash. A plain string literal type in the declarations, for examplerenderer: 'react', therefore fails the assertion even though it is not a module reference. Reuse the same anchoring thatSTORYBOOK_IMPORT_REalready applies.♻️ Proposed change to anchor the react check
+/** Same import forms as {`@link` STORYBOOK_IMPORT_RE}, for `react` and its subpaths. */ +const REACT_IMPORT_RE = + /(?:\bfrom\s*|\brequire\(\s*|\bimport\(\s*|\bimport\s+)["']react(?:["']|\/)/; + it.runIf(DTS_BUILT)('ships declarations a pure-Node consumer can check', () => { const declarations = readFileSync(DTS_ENTRY, 'utf-8'); // No react (nor any module reference to it, including subpaths like react/jsx-runtime): // consumers of this package have no `@types/react`, and `skipLibCheck: false` must keep // working for them. - expect(declarations).not.toMatch(/["']react(?:["']|\/)/); + expect(declarations).not.toMatch(REACT_IMPORT_RE); expect(declarations).not.toMatch(STORYBOOK_IMPORT_RE); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/mcp/src/dist-contract.test.ts` around lines 88 - 92, Update the `declarations` assertion in `dist-contract.test.ts` to reuse the module-reference anchoring pattern used by `STORYBOOK_IMPORT_RE`, so only imports or module references to `react` and its subpaths are rejected. Preserve acceptance of ordinary declaration string literals such as `renderer: 'react'`.test-storybooks/mcp/tests/mcp-addon-vitest-not-enabled.e2e.test.ts (1)
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit
.tsextensions for both relative imports.
test-storybooks/mcp/tests/mcp-addon-vitest-not-enabled.e2e.test.ts#L3-L3: change./helpersto./helpers.ts.test-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts#L3-L3: change./helpersto./helpers.ts.As per coding guidelines: “For TypeScript source in the repo, prefer explicit file extensions for relative code imports and exports.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-storybooks/mcp/tests/mcp-addon-vitest-not-enabled.e2e.test.ts` at line 3, Update the relative helpers imports to use the explicit TypeScript extension in both test-storybooks/mcp/tests/mcp-addon-vitest-not-enabled.e2e.test.ts:3-3 and test-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts:3-3, changing ./helpers to ./helpers.ts in each file.Source: Coding guidelines
code/addons/mcp/src/preset.test.ts (1)
422-422: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMock
./utils/addon-vitest.tswithvi.mock(..., { spy: true })instead ofvi.spyOninside the test.The changed line mocks a module member directly from within a test case. Declare the file mock at the top of the file, then set the behavior with
vi.mocked(isAddonVitestEnabled).♻️ Proposed refactor
- vi.spyOn(runStoryTests, 'isAddonVitestEnabled').mockResolvedValue(false); + vi.mocked(isAddonVitestEnabled).mockResolvedValue(false);Add the mock declaration at the top of the file:
vi.mock('./utils/addon-vitest.ts', { spy: true });As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests", "Avoid inline mock implementations within test cases in Vitest tests", and "Avoid direct function mocking withoutvi.mocked()in Vitest tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/mcp/src/preset.test.ts` at line 422, Replace the inline runStoryTests.isAddonVitestEnabled spy in the test with a top-level vi.mock('./utils/addon-vitest.ts', { spy: true }) declaration, then configure the imported isAddonVitestEnabled through vi.mocked(...).mockResolvedValue(false) within the test while preserving the existing behavior.Source: Coding guidelines
code/addons/mcp/src/storybook-ai-metadata.test.ts (1)
155-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the disabled-addon mock behavior into setup.
Line 155 configures
isAddonVitestEnabledinside the disabled-addon test. Put this behavior in a nestedbeforeEachblock for that test group. Keep the test body limited to the observable tool-surface assertion.As per coding guidelines, implement mock behaviors in
beforeEachblocks and avoid inline mock implementations within test cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/mcp/src/storybook-ai-metadata.test.ts` at line 155, Move the vi.mocked(isAddonVitestEnabled).mockResolvedValue(false) setup from the disabled-addon test body into a nested beforeEach for that test group, leaving the test body focused only on asserting the observable tool surface.Source: Coding guidelines
🔇 Additional comments (50)
AGENTS.md (1)
99-178: 📐 Maintainability & Code Quality
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the documented paths and symbol names.
This section states several concrete facts:
MCP_TOOL_NAMESlives inopen-service/toolset-names.ts,portable-dist.test.tssits next to the portable entry source,code/core/build-config.tsdeclaresportable: { external: [...] }fortoolsets-docswithvalibotas the only allowlist entry, and the docs accesses are namedcreateServiceDocsAccess,createManifestDocsAccess,createProviderDocsAccess,createLocalDocsAccess,createCompositionDocsSources, andcreateDocsToolset. Stale documentation of this kind is hard to detect later. Confirm each name and path matches the code in this PR.code/lib/mcp/src/types.ts (1)
2-9: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify every re-exported name exists in
storybook/internal/toolsets-docswith the correct value or type kind.Lines 121-129 re-export
ComponentManifestMap,ComponentManifestMapV0,ComponentManifestMapV1,DocsManifestMap,DocsManifestMapV0,DocsManifestMapV1, andJsonRefas values. Lines 130-142 re-export the remaining names as types. If any of the names on lines 121-129 is type-only in the source entry, the value re-export fails underisolatedModules. This is a public export surface for embedders, so a mismatch breaks consumers rather than only this package.Also applies to: 121-142
code/lib/mcp/src/index.ts (1)
17-42: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the public export surface stayed complete after the module consolidation.
Lines 17-27 forward
getDocumentationToolMetadata,getListAllDocumentationToolMetadata,getStoryDocumentationToolMetadata,GET_STORY_TOOL_NAME,GET_TOOL_NAME, andLIST_TOOL_NAMEfrom./tools/register.ts. Lines 30-40 move the manifest adapters and resolvers tostorybook/internal/toolsets-docs. Embedders consume this root entry, so a name dropped during the consolidation is a breaking change. Confirm that./tools/register.tsexports each forwarded name and that no previously public name disappeared.code/lib/mcp/bin.ts (1)
19-24: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm the built
binartifact bundlesstorybook/internal/toolsets-docs.
package.jsonlistsstorybookas a devDependency only. So every shipped artifact that importsstorybook/internal/toolsets-docsmust inline it.code/lib/mcp/src/dist-contract.test.tsasserts this fordist/index.jsalone. It does not check thebinoutput. If thebinbundle keeps the bare specifier, the CLI fails at a consumer's runtime with an unresolvable module, and no test catches it.Consider extending the contract test to the
binartifact.code/lib/mcp/package.json (1)
52-52: LGTM!code/lib/mcp/src/dist-contract.test.ts (1)
53-77: LGTM!agent-eval/lib/templates.test.ts (1)
79-79: LGTM!agent-eval/lib/templates.ts (1)
559-589: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify no sandbox manifest still needs a local
file:override.The removal of the override is correct only if
code/addons/mcp/package.jsonno longer lists@storybook/mcp. If that dependency remains,rewriteDependencySpecnow converts itsworkspace:*spec to a registry version, so the sandbox installs the published@storybook/mcpinstead of the locally built copy and the eval silently tests the wrong code.A second point needs the same check.
code/lib/mcp/package.jsonnow adds"storybook": "workspace:*"todevDependencies, andrewritePackageSpecsForNpmrewritesdevDependenciesas well. The sandbox manifest will therefore requeststorybookat the monorepo version. Confirm that the sandbox install either skips devDependencies or can resolve that version from the registry.Also applies to: 614-629
code/addons/mcp/src/types.ts (1)
45-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Reuse
ManifestProviderin the hosted package too.This file now types the provider through the shared
ManifestProvideralias.code/lib/mcp/src/types.tsstill declares the same contract inline on itsStorybookContext.manifestProviderfield as(request: Request | undefined, path: string, source?: Source) => Promise<string>. Both packages feed providers into the same docs toolset, so two hand-maintained copies of one signature can drift. ImportManifestProviderfromstorybook/internal/toolsets-docsin the hosted package as well, so the compiler enforces the single shape.code/lib/mcp/src/tools/register.ts (1)
41-46: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Add the required
formatfield toctx.
ctxis passed to every shared toolset handler incall(). It omits the requiredformatfield. This violates the toolset handler contract and can prevent compilation or select an incorrect rendering mode.Set
formatto the format required by the MCP adapter.Based on learnings: “handlers receive
(input, ctx)withconsumer, optionalorigin, requiredformat, andgetService.”code/lib/mcp/src/tools/outcome-unwrap.test.ts (2)
26-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Use the required Vitest spy mock pattern.
Lines 26-42 replace a package module without
{ spy: true }. They also define the mock behavior outsidebeforeEach. Use a spy mock, accesscreateDocsToolsetwithvi.mocked(), and configure its behavior inbeforeEach.As per coding guidelines, use
vi.mock()withspy: true, usevi.mocked(), and implement mock behaviors inbeforeEach.
67-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Move manifest-provider mock behavior into
beforeEach.These helpers create
vi.fn(async ...)implementations outsidebeforeEach. Create the provider spy inbeforeEach, configure it withvi.mocked(), and keep each test limited to fixture-state changes.
code/lib/mcp/src/tools/outcome-unwrap.test.ts#L67-L74: initialize the manifest-provider spy and its implementation inbeforeEach.code/lib/mcp/src/tools/get-documentation-for-story.test.ts#L25-L44: initialize the manifest-provider spy and its implementation inbeforeEach.code/lib/mcp/src/tools/get-documentation.test.ts#L26-L45: initialize the manifest-provider spy and its implementation inbeforeEach.code/lib/mcp/src/tools/list-all-documentation.test.ts#L31-L50: initialize the manifest-provider spy and its implementation inbeforeEach.As per coding guidelines, implement mock behaviors in
beforeEachand usevi.mocked()for mocked functions.test-storybooks/mcp/.storybook-no-vitest/main.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Use explicit
.tsextensions for local TypeScript imports.Both paths target local TypeScript modules. Make the module format explicit.
test-storybooks/mcp/.storybook-no-vitest/main.ts#L2-L2: import../.storybook/main.ts.test-storybooks/mcp/.storybook-no-vitest/preview.ts#L1-L1: re-export from../.storybook/preview.ts.As per coding guidelines, prefer explicit file extensions for local TypeScript imports and exports.
code/addons/mcp/src/tools/toolset-tools.ts (4)
9-31: LGTM!
97-117: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm the
ToolsetCtxcontract no longer requiresformat.
buildContextsuppliesconsumer,origin,uiRoot,getService, andtelemetry, but noformat. A retrieved learning recordsformatas a required context field for toolset handlers. If the field is still required, this adapter andgetToolsetToolMetadatado not satisfy the contract.Verify the current
ToolsetCtxdefinition:
119-149: LGTM!
159-161: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.The zero-input detection reads a Valibot-internal property.
Line 160 inspects
schema.entriesto decide whether to publish an input schema.entriesis a Valibot object-schema field, not part of the Standard Schema surface thatStandardSchemaV1guarantees. Any method whose schema is wrapped (for examplev.pipe(v.object({}), ...)) or produced by another Standard Schema library reports noentries, sohasInputbecomestrueand the tool publishes an input schema. That changes the observable MCP metadata.Confirm that every toolset method schema is a plain Valibot object schema, or key the decision off an explicit method-level declaration.
code/addons/mcp/src/tools/ui-root.ts (1)
35-50: LGTM!code/addons/mcp/src/tools/tool-names.ts (1)
1-13: LGTM!code/addons/mcp/src/tools/tool-registry.ts (6)
5-8: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm
storybook/internal/toolsets-docsis a valid entry point here.This file imports
createDocsToolsetandcreateCompositionDocsSourcesfromstorybook/internal/toolsets-docs, whilecode/addons/mcp/src/test-support/register-core-toolsets.tsimportscreateDocsToolsetfromstorybook/internal/core-server. Two entry points for the same factory can produce two bundle copies, which the comment on Lines 18-20 identifies as a real hazard for cross-entry identity. The repository guideline also states that toolset factories are exported fromstorybook/internal/core-server.Verify that the
toolsets-docsentry exists in the core package exports and that both import paths resolve to one module.
87-121: LGTM!
123-140: LGTM!
238-287: LGTM!
289-316: LGTM!
212-227: 🔒 Security & PrivacyKeep the async
enabledcallback.tmcpsupportsenabledcallbacks that returnboolean | Promise<boolean>and awaits the result when it lists tools.> Likely an incorrect or invalid review comment.code/addons/mcp/src/test-support/register-core-toolsets.ts (2)
10-21: LGTM!
23-60: LGTM!code/addons/mcp/scripts/generate-tools-api-doc.ts (3)
45-52: 🩺 Stability & Availability
⚠️ Unverified finding
Sandbox verification was unavailable.
toolMetadataFormutates a shared registry, so concurrent section rendering can mix modes.
toolMetadataForcallsregisterCoreToolsetsForTest, which clears and rebuilds the global toolset registry. The returned metadata is only correct while the registry still holds thatreviewEnabledsetting.renderServerSectionreads metadata synchronously, then awaitsrenderTool, so a caller that renders the review-off and review-on sections withPromise.allwould leave the registry in the last-registered state during those awaits. Confirm that the top-level orchestration renders the two modes sequentially.
231-231: LGTM!Also applies to: 248-250
315-315: LGTM!code/addons/mcp/src/tools/toolset-tools.test.ts (3)
1-13: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Add a spy mock for
storybook/internal/node-logger.Line 66 calls
vi.mocked(logger.error).mockClear(), and Lines 202, 217, and 218 assert onlogger.error. This file mocks onlystorybook/internal/core-serverand../telemetry.ts. If no shared setup file mocks the logger,logger.erroris the real function andmockClearis undefined, sobeforeEachthrows.🔧 Proposed fix
vi.mock('storybook/internal/core-server', { spy: true }); +vi.mock('storybook/internal/node-logger', { spy: true }); vi.mock('../telemetry.ts', { spy: true });Verify whether a setup file already mocks the logger:
The spy-mocking guideline requires
vi.mock()withspy: truefor all package and file mocks, and requires mocking all dependencies the test subject uses.
60-71: LGTM!
73-261: LGTM!code/core/src/shared/review/events.ts (1)
1-6: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Confirm no remaining references to
PUSH_REVIEW.This change removes an exported channel event from the public review contract. Any leftover emitter, subscriber, or documentation reference now points at a symbol that no longer exists.
code/addons/mcp/src/tools/tool-registry.test.ts (1)
130-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Move mock setup into
beforeEach.
code/addons/mcp/src/tools/tool-registry.test.ts#L130-L144: Create resource spies and attach them to the server fixture inbeforeEach.code/addons/mcp/src/telemetry.test.ts#L54-L61: Add a top-levelvi.mock(..., { spy: true })for telemetry and configure its default resolved value inbeforeEach. Keep the test case limited to settingcliClientand asserting the emitted payload.As per coding guidelines, “Implement mock behaviors in
beforeEachblocks in Vitest tests,” “Avoid inline mock implementations within test cases in Vitest tests,” and “Avoid mocking without thespy: trueoption in Vitest tests.”code/addons/mcp/src/tools/tool-registry-composition.test.ts (1)
14-15: LGTM!Also applies to: 70-134
code/addons/mcp/src/auth/composition-auth.ts (1)
9-13: LGTM!Also applies to: 155-159
code/addons/mcp/src/auth/resolve-composition-sources.ts (1)
1-1: LGTM!code/addons/mcp/src/preset.ts (1)
17-18: LGTM!Also applies to: 43-75
code/addons/mcp/src/mcp-handler.ts (1)
5-10: LGTM!Also applies to: 121-121, 134-134, 144-144, 154-161
code/addons/mcp/src/utils/addon-vitest.ts (1)
1-18: LGTM!code/addons/mcp/src/utils/get-tool-availability.ts (1)
5-5: LGTM!Also applies to: 28-31, 97-102, 114-114
code/addons/mcp/src/instructions/build-server-instructions.ts (1)
6-6: LGTM!Also applies to: 92-97
code/addons/mcp/src/storybook-ai-metadata.test.ts (1)
2-13: LGTM!Also applies to: 44-49, 65-65, 490-539
code/addons/mcp/src/mcp-handler.test.ts (1)
2-2: LGTM!Also applies to: 209-210, 445-445
code/addons/mcp/src/tools/get-storybook-story-instructions.test.ts (1)
4-4: LGTM!Also applies to: 17-17, 27-27, 67-67
code/addons/mcp/src/tools/get-storybook-story-instructions.ts (1)
3-3: LGTM!Also applies to: 50-54, 202-203
code/addons/mcp/src/tools/preview-stories.ts (1)
2-18: LGTM!Also applies to: 60-60
code/addons/mcp/src/tools/preview-stories/preview-stories-app-script.ts (1)
9-9: LGTM!code/addons/mcp/src/utils/module-graph.ts (1)
109-110: LGTM!
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/addons/mcp/src/preset.test.ts`:
- Around line 14-16: Update the test setup around beforeEach and the disabled
addon-vitest case so registerCoreToolsetsForTest uses { testToolset: false }
when isAddonVitestEnabled resolves to false. Keep default registration for tests
that require the test toolset, ensuring the assertion exercises the real
disabled-addon registry state.
In `@code/addons/mcp/src/telemetry.ts`:
- Around line 21-24: Update the telemetry payload construction so the spread of
payload occurs before the derived consumer field, ensuring caller-provided
payload.consumer cannot override the CLI-versus-MCP classification.
In `@code/addons/mcp/src/tools/ui-root.ts`:
- Around line 19-23: Update the root-path calculation around normalizedEndpoint
and normalizedPathname to compare the pathname suffix against the endpoint with
a leading slash, ensuring matching occurs on a path-segment boundary. Preserve
the existing slicing behavior for valid endpoint matches and fallback behavior
for non-matches.
In `@code/lib/mcp/src/tools/outcome-unwrap.test.ts`:
- Around line 184-195: Update the unexpected-error handling exercised by
callListTool so MCP clients receive a fixed generic error message instead of the
exception text. Keep the complete exception details in the server-side
consoleError log, and adjust the test assertion while preserving the existing
isError and single-log expectations.
---
Nitpick comments:
In `@code/addons/mcp/src/preset.test.ts`:
- Line 422: Replace the inline runStoryTests.isAddonVitestEnabled spy in the
test with a top-level vi.mock('./utils/addon-vitest.ts', { spy: true })
declaration, then configure the imported isAddonVitestEnabled through
vi.mocked(...).mockResolvedValue(false) within the test while preserving the
existing behavior.
In `@code/addons/mcp/src/storybook-ai-metadata.test.ts`:
- Line 155: Move the vi.mocked(isAddonVitestEnabled).mockResolvedValue(false)
setup from the disabled-addon test body into a nested beforeEach for that test
group, leaving the test body focused only on asserting the observable tool
surface.
In `@code/lib/mcp/src/dist-contract.test.ts`:
- Around line 88-92: Update the `declarations` assertion in
`dist-contract.test.ts` to reuse the module-reference anchoring pattern used by
`STORYBOOK_IMPORT_RE`, so only imports or module references to `react` and its
subpaths are rejected. Preserve acceptance of ordinary declaration string
literals such as `renderer: 'react'`.
In `@test-storybooks/mcp/tests/mcp-addon-vitest-not-enabled.e2e.test.ts`:
- Line 3: Update the relative helpers imports to use the explicit TypeScript
extension in both
test-storybooks/mcp/tests/mcp-addon-vitest-not-enabled.e2e.test.ts:3-3 and
test-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts:3-3, changing ./helpers
to ./helpers.ts in each file.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 69dc0ee6-4ac6-4765-953e-8b41039062d1
⛔ Files ignored due to path filters (3)
code/lib/mcp/src/utils/manifest-formatter/__snapshots__/markdown.test.ts.snapis excluded by!**/*.snaptest-storybooks/mcp/yarn.lockis excluded by!**/yarn.lock,!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (101)
AGENTS.mdagent-eval/lib/templates.test.tsagent-eval/lib/templates.tscode/addons/mcp/package.jsoncode/addons/mcp/scripts/generate-tools-api-doc.tscode/addons/mcp/src/auth/composition-auth.test.tscode/addons/mcp/src/auth/composition-auth.tscode/addons/mcp/src/auth/resolve-composition-sources.tscode/addons/mcp/src/constants.tscode/addons/mcp/src/instructions/build-server-instructions.tscode/addons/mcp/src/manifests/__fixtures__/core-manifest.fixture.jsoncode/addons/mcp/src/manifests/core-format-contract.test.tscode/addons/mcp/src/manifests/in-process-provider.test.tscode/addons/mcp/src/manifests/in-process-provider.tscode/addons/mcp/src/manifests/service-access.test.tscode/addons/mcp/src/manifests/service-access.tscode/addons/mcp/src/manifests/vendored.tscode/addons/mcp/src/mcp-handler.test.tscode/addons/mcp/src/mcp-handler.tscode/addons/mcp/src/preset.test.tscode/addons/mcp/src/preset.tscode/addons/mcp/src/storybook-ai-metadata.test.tscode/addons/mcp/src/telemetry.test.tscode/addons/mcp/src/telemetry.tscode/addons/mcp/src/test-support/register-core-toolsets.tscode/addons/mcp/src/tools/display-review.test.tscode/addons/mcp/src/tools/display-review.tscode/addons/mcp/src/tools/get-changed-stories.test.tscode/addons/mcp/src/tools/get-changed-stories.tscode/addons/mcp/src/tools/get-stories-by-component.test.tscode/addons/mcp/src/tools/get-stories-by-component.tscode/addons/mcp/src/tools/get-storybook-story-instructions.test.tscode/addons/mcp/src/tools/get-storybook-story-instructions.tscode/addons/mcp/src/tools/preview-stories.test.tscode/addons/mcp/src/tools/preview-stories.tscode/addons/mcp/src/tools/preview-stories/preview-stories-app-script.tscode/addons/mcp/src/tools/run-story-tests.test.tscode/addons/mcp/src/tools/run-story-tests.tscode/addons/mcp/src/tools/tool-names.tscode/addons/mcp/src/tools/tool-registry-composition.test.tscode/addons/mcp/src/tools/tool-registry.test.tscode/addons/mcp/src/tools/tool-registry.tscode/addons/mcp/src/tools/toolset-tools.test.tscode/addons/mcp/src/tools/toolset-tools.tscode/addons/mcp/src/tools/ui-root.tscode/addons/mcp/src/types.tscode/addons/mcp/src/utils/addon-vitest.tscode/addons/mcp/src/utils/build-args-param.test.tscode/addons/mcp/src/utils/build-args-param.tscode/addons/mcp/src/utils/detect-unreachable-changes.test.tscode/addons/mcp/src/utils/detect-unreachable-changes.tscode/addons/mcp/src/utils/estimate-tokens.test.tscode/addons/mcp/src/utils/estimate-tokens.tscode/addons/mcp/src/utils/find-story-ids.test.tscode/addons/mcp/src/utils/find-story-ids.tscode/addons/mcp/src/utils/get-tool-availability.tscode/addons/mcp/src/utils/module-graph.tscode/addons/mcp/src/utils/resolve-component-stories.test.tscode/addons/mcp/src/utils/resolve-component-stories.tscode/addons/mcp/src/utils/slash.tscode/core/src/core-server/presets/common-preset.tscode/core/src/core-server/server-channel/review-channel.test.tscode/core/src/core-server/server-channel/review-channel.tscode/core/src/shared/review/events.tscode/lib/mcp/bin.tscode/lib/mcp/package.jsoncode/lib/mcp/src/dist-contract.test.tscode/lib/mcp/src/globals.d.tscode/lib/mcp/src/index.tscode/lib/mcp/src/instructions.mdcode/lib/mcp/src/tools/get-documentation-for-story.test.tscode/lib/mcp/src/tools/get-documentation-for-story.tscode/lib/mcp/src/tools/get-documentation.test.tscode/lib/mcp/src/tools/get-documentation.tscode/lib/mcp/src/tools/list-all-documentation.test.tscode/lib/mcp/src/tools/list-all-documentation.tscode/lib/mcp/src/tools/outcome-unwrap.test.tscode/lib/mcp/src/tools/register.tscode/lib/mcp/src/types.tscode/lib/mcp/src/utils/adapt-core-manifest.test.tscode/lib/mcp/src/utils/adapt-core-manifest.tscode/lib/mcp/src/utils/dedent.tscode/lib/mcp/src/utils/error-to-mcp-content.test.tscode/lib/mcp/src/utils/error-to-mcp-content.tscode/lib/mcp/src/utils/get-manifest.test.tscode/lib/mcp/src/utils/get-manifest.tscode/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.test.tscode/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.tscode/lib/mcp/src/utils/manifest-formatter/markdown.test.tscode/lib/mcp/src/utils/manifest-formatter/markdown.tscode/lib/mcp/src/utils/map-with-concurrency.tscode/lib/mcp/src/utils/multi-source-manifests.tscode/lib/mcp/src/utils/parse-react-docgen.test.tscode/lib/mcp/src/utils/parse-react-docgen.tscode/lib/mcp/src/utils/requires-own-mcp.test.tscode/lib/mcp/src/utils/requires-own-mcp.tstest-storybooks/mcp/.storybook-no-vitest/main.tstest-storybooks/mcp/.storybook-no-vitest/preview.tstest-storybooks/mcp/tests/helpers.tstest-storybooks/mcp/tests/mcp-addon-vitest-not-enabled.e2e.test.tstest-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts
💤 Files with no reviewable changes (52)
- code/addons/mcp/src/tools/display-review.test.ts
- code/lib/mcp/src/utils/requires-own-mcp.ts
- code/addons/mcp/src/utils/estimate-tokens.test.ts
- code/addons/mcp/src/utils/slash.ts
- code/lib/mcp/src/utils/map-with-concurrency.ts
- code/addons/mcp/src/utils/build-args-param.ts
- code/addons/mcp/src/utils/build-args-param.test.ts
- code/core/src/core-server/presets/common-preset.ts
- code/addons/mcp/src/manifests/service-access.test.ts
- code/addons/mcp/src/utils/detect-unreachable-changes.ts
- code/addons/mcp/src/tools/get-stories-by-component.test.ts
- code/addons/mcp/src/tools/get-changed-stories.test.ts
- code/lib/mcp/src/utils/manifest-formatter/markdown.test.ts
- code/lib/mcp/src/tools/get-documentation-for-story.ts
- code/lib/mcp/src/instructions.md
- code/lib/mcp/src/globals.d.ts
- code/addons/mcp/src/utils/find-story-ids.ts
- code/addons/mcp/src/manifests/in-process-provider.ts
- code/addons/mcp/src/manifests/in-process-provider.test.ts
- code/core/src/core-server/server-channel/review-channel.test.ts
- code/addons/mcp/src/tools/get-changed-stories.ts
- code/addons/mcp/src/tools/run-story-tests.test.ts
- code/lib/mcp/src/tools/get-documentation.ts
- code/lib/mcp/src/utils/dedent.ts
- code/addons/mcp/src/tools/display-review.ts
- code/lib/mcp/src/utils/adapt-core-manifest.ts
- code/lib/mcp/src/utils/parse-react-docgen.ts
- code/addons/mcp/src/utils/resolve-component-stories.ts
- code/addons/mcp/src/utils/resolve-component-stories.test.ts
- code/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.test.ts
- code/addons/mcp/src/tools/preview-stories.test.ts
- code/lib/mcp/src/tools/list-all-documentation.ts
- code/addons/mcp/src/manifests/fixtures/core-manifest.fixture.json
- code/lib/mcp/src/utils/requires-own-mcp.test.ts
- code/lib/mcp/src/utils/parse-react-docgen.test.ts
- code/addons/mcp/src/manifests/core-format-contract.test.ts
- code/lib/mcp/src/utils/adapt-core-manifest.test.ts
- code/addons/mcp/src/tools/get-stories-by-component.ts
- code/addons/mcp/src/manifests/service-access.ts
- code/addons/mcp/src/utils/estimate-tokens.ts
- code/addons/mcp/src/utils/find-story-ids.test.ts
- code/lib/mcp/src/utils/manifest-formatter/markdown.ts
- code/core/src/core-server/server-channel/review-channel.ts
- code/addons/mcp/src/auth/composition-auth.test.ts
- code/addons/mcp/src/tools/run-story-tests.ts
- code/lib/mcp/src/utils/get-manifest.ts
- code/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.ts
- code/addons/mcp/package.json
- code/addons/mcp/src/utils/detect-unreachable-changes.test.ts
- code/addons/mcp/src/manifests/vendored.ts
- code/addons/mcp/src/constants.ts
- code/lib/mcp/src/utils/get-manifest.test.ts
Co-authored-by: Cursor <cursoragent@cursor.com>
28e9284 to
2a585e1
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code/addons/mcp/src/tools/tool-registry-composition.test.ts (1)
101-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the failing source in the partial-failure test.
The test name states that one source fails. The assertions only confirm that the readable source appears and that
sourceCountis 2. The listing still passes if the unreadablelocalsource is silently dropped instead of reported as failed. Add an assertion on the failure text, so the partial-failure contract is covered.♻️ Proposed assertion
expect(result.content[0].text).toContain('# Design System'); + expect(result.content[0].text).toContain('Local'); + expect(result.content[0].text).toMatch(/Failed to fetch manifest for local/i); expect(collectTelemetry).toHaveBeenCalledWith( expect.objectContaining({ event: 'tool:listAllDocumentation', sourceCount: 2 }) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/mcp/src/tools/tool-registry-composition.test.ts` around lines 101 - 111, Add an assertion in the partial-failure test around makeServer and list-all-documentation to verify the result content includes the failure text for the unreadable local source, while preserving the existing readable-source and telemetry assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/lib/mcp/src/tools/register.ts`:
- Around line 234-243: Update the block normalization in call so
outcome.markdown always yields at least one text block: replace an empty
markdown array or an undefined single value with an appropriate fallback text
value before mapping. Preserve existing markdown content when present, ensuring
callers reading result.content[0].text cannot encounter an empty array or
undefined text.
---
Nitpick comments:
In `@code/addons/mcp/src/tools/tool-registry-composition.test.ts`:
- Around line 101-111: Add an assertion in the partial-failure test around
makeServer and list-all-documentation to verify the result content includes the
failure text for the unreadable local source, while preserving the existing
readable-source and telemetry assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4db8548-35dd-448f-b175-f70f5c8d4a14
⛔ Files ignored due to path filters (2)
code/lib/mcp/src/utils/manifest-formatter/__snapshots__/markdown.test.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (100)
AGENTS.mdagent-eval/lib/templates.test.tsagent-eval/lib/templates.tscode/addons/mcp/package.jsoncode/addons/mcp/scripts/generate-tools-api-doc.tscode/addons/mcp/src/auth/composition-auth.test.tscode/addons/mcp/src/auth/composition-auth.tscode/addons/mcp/src/auth/resolve-composition-sources.tscode/addons/mcp/src/constants.tscode/addons/mcp/src/instructions/build-server-instructions.tscode/addons/mcp/src/manifests/__fixtures__/core-manifest.fixture.jsoncode/addons/mcp/src/manifests/core-format-contract.test.tscode/addons/mcp/src/manifests/in-process-provider.test.tscode/addons/mcp/src/manifests/in-process-provider.tscode/addons/mcp/src/manifests/service-access.test.tscode/addons/mcp/src/manifests/service-access.tscode/addons/mcp/src/manifests/vendored.tscode/addons/mcp/src/mcp-handler.test.tscode/addons/mcp/src/mcp-handler.tscode/addons/mcp/src/preset.test.tscode/addons/mcp/src/preset.tscode/addons/mcp/src/storybook-ai-metadata.test.tscode/addons/mcp/src/telemetry.test.tscode/addons/mcp/src/telemetry.tscode/addons/mcp/src/test-support/register-core-toolsets.tscode/addons/mcp/src/tools/display-review.test.tscode/addons/mcp/src/tools/display-review.tscode/addons/mcp/src/tools/get-changed-stories.test.tscode/addons/mcp/src/tools/get-changed-stories.tscode/addons/mcp/src/tools/get-stories-by-component.test.tscode/addons/mcp/src/tools/get-stories-by-component.tscode/addons/mcp/src/tools/get-storybook-story-instructions.test.tscode/addons/mcp/src/tools/get-storybook-story-instructions.tscode/addons/mcp/src/tools/preview-stories.test.tscode/addons/mcp/src/tools/preview-stories.tscode/addons/mcp/src/tools/preview-stories/preview-stories-app-script.tscode/addons/mcp/src/tools/run-story-tests.test.tscode/addons/mcp/src/tools/run-story-tests.tscode/addons/mcp/src/tools/tool-names.tscode/addons/mcp/src/tools/tool-registry-composition.test.tscode/addons/mcp/src/tools/tool-registry.test.tscode/addons/mcp/src/tools/tool-registry.tscode/addons/mcp/src/tools/toolset-tools.test.tscode/addons/mcp/src/tools/toolset-tools.tscode/addons/mcp/src/tools/ui-root.tscode/addons/mcp/src/types.tscode/addons/mcp/src/utils/addon-vitest.tscode/addons/mcp/src/utils/build-args-param.test.tscode/addons/mcp/src/utils/build-args-param.tscode/addons/mcp/src/utils/detect-unreachable-changes.test.tscode/addons/mcp/src/utils/detect-unreachable-changes.tscode/addons/mcp/src/utils/estimate-tokens.test.tscode/addons/mcp/src/utils/estimate-tokens.tscode/addons/mcp/src/utils/find-story-ids.test.tscode/addons/mcp/src/utils/find-story-ids.tscode/addons/mcp/src/utils/get-tool-availability.tscode/addons/mcp/src/utils/module-graph.tscode/addons/mcp/src/utils/resolve-component-stories.test.tscode/addons/mcp/src/utils/resolve-component-stories.tscode/core/src/core-server/presets/common-preset.tscode/core/src/core-server/server-channel/review-channel.test.tscode/core/src/core-server/server-channel/review-channel.tscode/core/src/shared/review/events.tscode/lib/mcp/bin.tscode/lib/mcp/package.jsoncode/lib/mcp/src/dist-contract.test.tscode/lib/mcp/src/globals.d.tscode/lib/mcp/src/index.tscode/lib/mcp/src/instructions.mdcode/lib/mcp/src/tools/get-documentation-for-story.test.tscode/lib/mcp/src/tools/get-documentation-for-story.tscode/lib/mcp/src/tools/get-documentation.test.tscode/lib/mcp/src/tools/get-documentation.tscode/lib/mcp/src/tools/list-all-documentation.test.tscode/lib/mcp/src/tools/list-all-documentation.tscode/lib/mcp/src/tools/outcome-unwrap.test.tscode/lib/mcp/src/tools/register.tscode/lib/mcp/src/types.tscode/lib/mcp/src/utils/adapt-core-manifest.test.tscode/lib/mcp/src/utils/adapt-core-manifest.tscode/lib/mcp/src/utils/dedent.tscode/lib/mcp/src/utils/error-to-mcp-content.test.tscode/lib/mcp/src/utils/error-to-mcp-content.tscode/lib/mcp/src/utils/get-manifest.test.tscode/lib/mcp/src/utils/get-manifest.tscode/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.test.tscode/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.tscode/lib/mcp/src/utils/manifest-formatter/markdown.test.tscode/lib/mcp/src/utils/manifest-formatter/markdown.tscode/lib/mcp/src/utils/map-with-concurrency.tscode/lib/mcp/src/utils/multi-source-manifests.tscode/lib/mcp/src/utils/parse-react-docgen.test.tscode/lib/mcp/src/utils/parse-react-docgen.tscode/lib/mcp/src/utils/requires-own-mcp.test.tscode/lib/mcp/src/utils/requires-own-mcp.tstest-storybooks/mcp/.storybook-no-vitest/main.tstest-storybooks/mcp/.storybook-no-vitest/preview.tstest-storybooks/mcp/tests/helpers.tstest-storybooks/mcp/tests/mcp-addon-vitest-not-enabled.e2e.test.tstest-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts
💤 Files with no reviewable changes (51)
- code/lib/mcp/src/instructions.md
- code/lib/mcp/src/utils/parse-react-docgen.test.ts
- code/addons/mcp/src/utils/resolve-component-stories.test.ts
- code/core/src/core-server/server-channel/review-channel.ts
- code/addons/mcp/src/tools/get-stories-by-component.ts
- code/lib/mcp/src/utils/requires-own-mcp.test.ts
- code/addons/mcp/src/manifests/core-format-contract.test.ts
- code/core/src/core-server/presets/common-preset.ts
- code/addons/mcp/src/tools/get-changed-stories.test.ts
- code/lib/mcp/src/globals.d.ts
- code/addons/mcp/src/tools/display-review.test.ts
- code/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.test.ts
- code/addons/mcp/src/utils/build-args-param.test.ts
- code/core/src/core-server/server-channel/review-channel.test.ts
- code/addons/mcp/src/auth/composition-auth.test.ts
- code/addons/mcp/src/manifests/fixtures/core-manifest.fixture.json
- code/lib/mcp/src/utils/dedent.ts
- code/addons/mcp/src/tools/preview-stories.test.ts
- code/addons/mcp/src/manifests/in-process-provider.test.ts
- code/lib/mcp/src/utils/manifest-formatter/markdown.test.ts
- code/lib/mcp/src/utils/map-with-concurrency.ts
- code/lib/mcp/src/utils/get-manifest.test.ts
- code/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.ts
- code/addons/mcp/src/utils/find-story-ids.test.ts
- code/lib/mcp/src/utils/adapt-core-manifest.ts
- code/lib/mcp/src/utils/adapt-core-manifest.test.ts
- code/addons/mcp/src/utils/detect-unreachable-changes.test.ts
- code/lib/mcp/src/tools/get-documentation-for-story.ts
- code/lib/mcp/src/utils/parse-react-docgen.ts
- code/addons/mcp/src/utils/estimate-tokens.test.ts
- code/addons/mcp/package.json
- code/lib/mcp/src/utils/requires-own-mcp.ts
- code/addons/mcp/src/tools/run-story-tests.ts
- code/addons/mcp/src/utils/build-args-param.ts
- code/addons/mcp/src/tools/run-story-tests.test.ts
- code/lib/mcp/src/utils/manifest-formatter/markdown.ts
- code/addons/mcp/src/tools/get-stories-by-component.test.ts
- code/addons/mcp/src/utils/detect-unreachable-changes.ts
- code/addons/mcp/src/manifests/vendored.ts
- code/addons/mcp/src/utils/find-story-ids.ts
- code/lib/mcp/src/tools/list-all-documentation.ts
- code/addons/mcp/src/utils/resolve-component-stories.ts
- code/addons/mcp/src/tools/get-changed-stories.ts
- code/addons/mcp/src/manifests/service-access.test.ts
- code/lib/mcp/src/tools/get-documentation.ts
- code/addons/mcp/src/utils/estimate-tokens.ts
- code/addons/mcp/src/tools/display-review.ts
- code/addons/mcp/src/manifests/in-process-provider.ts
- code/addons/mcp/src/manifests/service-access.ts
- code/lib/mcp/src/utils/get-manifest.ts
- code/addons/mcp/src/constants.ts
🚧 Files skipped from review as they are similar to previous changes (45)
- code/addons/mcp/src/telemetry.ts
- test-storybooks/mcp/tests/mcp-git-unusable.e2e.test.ts
- code/lib/mcp/src/utils/error-to-mcp-content.test.ts
- code/lib/mcp/package.json
- test-storybooks/mcp/.storybook-no-vitest/preview.ts
- code/addons/mcp/src/utils/addon-vitest.ts
- code/addons/mcp/src/tools/ui-root.ts
- code/lib/mcp/src/utils/error-to-mcp-content.ts
- code/lib/mcp/src/utils/multi-source-manifests.ts
- code/lib/mcp/src/tools/outcome-unwrap.test.ts
- code/addons/mcp/src/auth/resolve-composition-sources.ts
- code/lib/mcp/bin.ts
- code/addons/mcp/src/tools/get-storybook-story-instructions.test.ts
- test-storybooks/mcp/.storybook-no-vitest/main.ts
- code/addons/mcp/src/auth/composition-auth.ts
- test-storybooks/mcp/tests/helpers.ts
- test-storybooks/mcp/tests/mcp-addon-vitest-not-enabled.e2e.test.ts
- code/addons/mcp/src/tools/preview-stories.ts
- agent-eval/lib/templates.ts
- code/addons/mcp/src/test-support/register-core-toolsets.ts
- code/addons/mcp/src/utils/get-tool-availability.ts
- code/addons/mcp/src/telemetry.test.ts
- code/addons/mcp/src/utils/module-graph.ts
- code/addons/mcp/src/instructions/build-server-instructions.ts
- code/addons/mcp/src/preset.ts
- code/addons/mcp/src/preset.test.ts
- code/addons/mcp/src/tools/preview-stories/preview-stories-app-script.ts
- code/lib/mcp/src/dist-contract.test.ts
- code/addons/mcp/src/tools/toolset-tools.ts
- AGENTS.md
- code/addons/mcp/src/tools/tool-names.ts
- code/addons/mcp/src/mcp-handler.test.ts
- code/addons/mcp/src/tools/get-storybook-story-instructions.ts
- code/addons/mcp/scripts/generate-tools-api-doc.ts
- code/addons/mcp/src/tools/tool-registry.ts
- code/addons/mcp/src/types.ts
- code/lib/mcp/src/index.ts
- code/core/src/shared/review/events.ts
- code/lib/mcp/src/tools/list-all-documentation.test.ts
- code/addons/mcp/src/tools/toolset-tools.test.ts
- code/addons/mcp/src/mcp-handler.ts
- code/lib/mcp/src/tools/get-documentation-for-story.test.ts
- code/addons/mcp/src/storybook-ai-metadata.test.ts
- code/lib/mcp/src/tools/get-documentation.test.ts
- agent-eval/lib/templates.test.ts
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 13 | 11 | 🎉 -2 🎉 |
| Self size | 191 KB | 129 KB | 🎉 -63 KB 🎉 |
| Dependency size | 2.79 MB | 2.64 MB | 🎉 -145 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/mcp
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 11 | 11 | 0 |
| Self size | 107 KB | 144 KB | 🚨 +36 KB 🚨 |
| Dependency size | 2.64 MB | 2.64 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
|
"Self-merging" as this is originally a PR by @kasperpeulen that I approved but had to recreate here. |
3 participants
Replacement for #35856 to clear a false GitHub CONFLICTING state.
Evidence the merge is clean:
git merge-base --is-ancestorm2b ⊂ m4 (fast-forward)git mergeof head into base succeeds with no conflictsReplacement for #35854 / #35849 / #35677 with a new head branch name (
m4-mcp-on-core-toolsets) so GitHub stops treating this layer as already-merged.Same commits as
m4-addon-mcp-core-servicestipb0ac81925ed.Replaces accidentally closed #35677 (a mergeability probe via the GitHub merges API marked that PR merged and deleted this branch; the merge commit on
m2b-core-toolsetswas reverted and this branch tip was restored to0a5ad88a6a4).No intentional code change versus #35677.
Closes #35673 · Closes #35725
Stacked: this PR's base is #35726 — the core toolsets layer this PR consumes; review that one first. #35719 (the
storybook toolsCLI) stacks on this one. Merge #35726 first; GitHub retargets this PR tonextautomatically.What I did
#35726 built Storybook's agent-facing capabilities as core toolsets. This PR is the switch: both MCP surfaces —
@storybook/addon-mcp(the dev-server MCP) and@storybook/mcp(the hosted/composition MCP) — now render their tools from those shared definitions, and the engines they each carried are deleted. Net: −11,199 lines.What remains in each package is an adapter. It resolves a toolset method and maps the outcome onto the MCP reply mechanically: schemas and prose come from the definition,
markdownbecomes the text blocks,databecomesstructuredContent,okbecomesisError, and agent-facing errors surface verbatim. Meaning lives in the toolset — the adapter is not allowed to re-derive it from the data.Concretely, an MCP tool is now a registry row pointing at a toolset method (real code):
The one review question
Does the way the MCP uses the toolsets make sense — and is there business logic still in the MCP that belongs in a toolset? Everything in commits 1–3 should read as mechanical: naming, gating, unwrapping, transport. If you find an adapter interpreting a result — branching on domain data, rewriting prose, computing anything the future CLI consumer would have to duplicate — that is the review finding to raise.
You do not need to review the deleted engines: commit 4/7 is −12,570 lines of pure deletions, and behavioral equality with them is pinned by the e2e wire snapshots (evidence below), not by reading them.
How to review (~1½ hours)
The branch's seven commits are these seven blocks, in this order. Each block heading links straight to its commit's diff — review one commit at a time, and read the block's intro here before opening any file.
Block 1 · The addon adapter — 30 min
Files:
toolset-tools.ts,tool-registry.ts,ui-root.ts,tool-names.ts(4 files, +425 −109)The entire addon-side integration is two ideas. The first is one generic unwrap that turns any toolset outcome into an MCP result —
toolset-tools.ts, 187 lines, read it in full:The second is a declarative registry where every MCP tool is a row pointing at a toolset method — the
docsRowsnippet above is one real row.tool-registry.tswalks the rows; the availability gate × thetoolsetsconfig decides what registers.What to check: nothing in this block may interpret
data— no branching on domain fields, no re-deriving prose from the payload. If you find the adapter deciding what a result means, that is the finding to raise.Block 2 · The hosted adapter — 25 min
Files:
register.ts,error-to-mcp-content.ts,multi-source-manifests.ts,index.ts,types.ts,bin.ts,package.json(7 files, +497 −225)The same unwrap for
@storybook/mcp, with one structural difference: the hosted server builds its docs toolset per request, because the provider and sources belong to the request:What to check:
register.ts(346 lines) in full — the per-request construction, the twin error mapping, and that the package boundary holds.Block 3 · Addon rewiring — 15 min
Files: preset, mcp-handler, availability, telemetry, instructions, auth (16 files, +131 −386)
Mechanical re-pointing: everything that used to call an engine now consults the registry. Net −255 lines. Skim with one question: did any logic sneak in here, or is it all naming and plumbing?
Block 4 · Delete the replaced engines — 0 min
Files: the old tools, manifest pipeline, formatter and utils of both packages (47 files, −12,570, zero insertions)
Don't review. This is the point of the PR: every deleted line is an engine the core toolsets replaced. Behavioral equality is proven by block 6, not by reading these.
Block 5 · Retire the PUSH_REVIEW channel adapter — 5 min
Files:
common-preset.ts,review-channel.ts(+ test),events.ts(4 files, +3 −200)#35726 deliberately kept this adapter alive so released addon-mcp versions kept working against the new core; with the addon switched over in this PR, it retires. One glance.
Block 6 · The adapter tests and the e2e proof — 15 min
Files: adapter unit tests,
dist-contract.test.ts,test-storybooks/mcp(22 files, +1,569 −366)Skim as evidence, not as code under review: the contract tests pin the unwrap (outcome →
isError,structuredContentnarrowing), gating (including the preview app resource), telemetry, andagentFacingerror surfacing; the e2e suite exercises the real wire against real servers.Block 7 · Docs and bookkeeping — 5 min
Files:
AGENTS.md, agent-eval templates,yarn.lock(4 files, +89 −57)The AGENTS.md architecture section documents the end state of the whole layer — worth an actual read: it is also the text future agents working in this repo obey.
Why skipping the deletions is safe:
isErrormapping,structuredContentnarrowing, telemetry,agentFacingerror surfacing, and toolset gating (including the preview app resource)@storybook/mcpships withoutstorybookat runtimedist-contract.test.tspins the published dependency surface; the docs toolset arrives through the portablestorybook/internal/toolsets-docsentryChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn nx run-many -t compilecd test-storybooks/mcp && yarn install && yarn vitest run --project=e2e— 46 tests, inline snapshots byte-identical to the old enginesyarn storybookintest-storybooks/mcp, connect an MCP client tohttp://localhost:6006/mcp— the tool list and every tool response match the released addondisplay-review→ UI) — now served by the review toolset; the PUSH_REVIEW channel event no longer exists@storybook/mcpagainst the Chromatic-hosted Storybooks — listing groups per source, lookups takestorybookIdDocumentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsDeclare whether manual QA will be needed for this PR during the next release, through
qa:neededorqa:skipMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.Made with Cursor
Made with Cursor
Made with Cursor
Made with Cursor