Repository navigation
Skills M4: Run addon-mcp and @storybook/mcp on the shared core toolsets - #35677
Conversation
kasperpeulen
changed the title
kasperpeulen
added
maintenance
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 188 KB | 🚨 +188 KB 🚨 |
| Dependency size | 0 B | 3.12 MB | 🚨 +3.12 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-docs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 18 | 🚨 +18 🚨 |
| Self size | 0 B | 1.29 MB | 🚨 +1.29 MB 🚨 |
| Dependency size | 0 B | 9.29 MB | 🚨 +9.29 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-links
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 1 | 🚨 +1 🚨 |
| Self size | 0 B | 14 KB | 🚨 +14 KB 🚨 |
| Dependency size | 0 B | 5 KB | 🚨 +5 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-mcp
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 11 | 🚨 +11 🚨 |
| Self size | 0 B | 129 KB | 🚨 +129 KB 🚨 |
| Dependency size | 0 B | 2.74 MB | 🚨 +2.74 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-onboarding
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 0 B | 332 KB | 🚨 +332 KB 🚨 |
| Dependency size | 0 B | 670 B | 🚨 +670 B 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook-addon-pseudo-states
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 0 B | 21 KB | 🚨 +21 KB 🚨 |
| Dependency size | 0 B | 689 B | 🚨 +689 B 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-themes
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 1 | 🚨 +1 🚨 |
| Self size | 0 B | 18 KB | 🚨 +18 KB 🚨 |
| Dependency size | 0 B | 28 KB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-vitest
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 430 KB | 🚨 +430 KB 🚨 |
| Dependency size | 0 B | 350 KB | 🚨 +350 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/builder-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 11 | 🚨 +11 🚨 |
| Self size | 0 B | 136 KB | 🚨 +136 KB 🚨 |
| Dependency size | 0 B | 1.33 MB | 🚨 +1.33 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/builder-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 182 | 🚨 +182 🚨 |
| Self size | 0 B | 79 KB | 🚨 +79 KB 🚨 |
| Dependency size | 0 B | 37.43 MB | 🚨 +37.43 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 73 | 🚨 +73 🚨 |
| Self size | 0 B | 21.77 MB | 🚨 +21.77 MB 🚨 |
| Dependency size | 0 B | 30.98 MB | 🚨 +30.98 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 185 | 🚨 +185 🚨 |
| Self size | 0 B | 253 KB | 🚨 +253 KB 🚨 |
| Dependency size | 0 B | 30.28 MB | 🚨 +30.28 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 29 | 🚨 +29 🚨 |
| Self size | 0 B | 23.04 MB | 🚨 +23.04 MB 🚨 |
| Dependency size | 0 B | 12.69 MB | 🚨 +12.69 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 181 | 🚨 +181 🚨 |
| Self size | 0 B | 13 KB | 🚨 +13 KB 🚨 |
| Dependency size | 0 B | 32.78 MB | 🚨 +32.78 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/html-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 14 | 🚨 +14 🚨 |
| Self size | 0 B | 22 KB | 🚨 +22 KB 🚨 |
| Dependency size | 0 B | 1.50 MB | 🚨 +1.50 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 527 | 🚨 +527 🚨 |
| Self size | 0 B | 641 KB | 🚨 +641 KB 🚨 |
| Dependency size | 0 B | 64.12 MB | 🚨 +64.12 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 94 | 🚨 +94 🚨 |
| Self size | 0 B | 1.42 MB | 🚨 +1.42 MB 🚨 |
| Dependency size | 0 B | 23.88 MB | 🚨 +23.88 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preact-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 14 | 🚨 +14 🚨 |
| Self size | 0 B | 12 KB | 🚨 +12 KB 🚨 |
| Dependency size | 0 B | 1.52 MB | 🚨 +1.52 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 125 | 🚨 +125 🚨 |
| Self size | 0 B | 29 KB | 🚨 +29 KB 🚨 |
| Dependency size | 0 B | 25.90 MB | 🚨 +25.90 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 83 | 🚨 +83 🚨 |
| Self size | 0 B | 32 KB | 🚨 +32 KB 🚨 |
| Dependency size | 0 B | 21.19 MB | 🚨 +21.19 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 268 | 🚨 +268 🚨 |
| Self size | 0 B | 23 KB | 🚨 +23 KB 🚨 |
| Dependency size | 0 B | 49.86 MB | 🚨 +49.86 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 194 | 🚨 +194 🚨 |
| Self size | 0 B | 15 KB | 🚨 +15 KB 🚨 |
| Dependency size | 0 B | 38.69 MB | 🚨 +38.69 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/svelte-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 20 | 🚨 +20 🚨 |
| Self size | 0 B | 54 KB | 🚨 +54 KB 🚨 |
| Dependency size | 0 B | 26.67 MB | 🚨 +26.67 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/sveltekit
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 21 | 🚨 +21 🚨 |
| Self size | 0 B | 56 KB | 🚨 +56 KB 🚨 |
| Dependency size | 0 B | 26.73 MB | 🚨 +26.73 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/tanstack-react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 84 | 🚨 +84 🚨 |
| Self size | 0 B | 118 KB | 🚨 +118 KB 🚨 |
| Dependency size | 0 B | 21.22 MB | 🚨 +21.22 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 103 | 🚨 +103 🚨 |
| Self size | 0 B | 31 KB | 🚨 +31 KB 🚨 |
| Dependency size | 0 B | 19.60 MB | 🚨 +19.60 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/web-components-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 15 | 🚨 +15 🚨 |
| Self size | 0 B | 19 KB | 🚨 +19 KB 🚨 |
| Dependency size | 0 B | 1.56 MB | 🚨 +1.56 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 205 | 🚨 +205 🚨 |
| Self size | 0 B | 833 KB | 🚨 +833 KB 🚨 |
| Dependency size | 0 B | 87.11 MB | 🚨 +87.11 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 198 | 🚨 +198 🚨 |
| Self size | 0 B | 32 KB | 🚨 +32 KB 🚨 |
| Dependency size | 0 B | 85.59 MB | 🚨 +85.59 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/core-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 1 | 🚨 +1 🚨 |
| Self size | 0 B | 11 KB | 🚨 +11 KB 🚨 |
| Dependency size | 0 B | 28 KB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 74 | 🚨 +74 🚨 |
| Self size | 0 B | 1.09 MB | 🚨 +1.09 MB 🚨 |
| Dependency size | 0 B | 52.75 MB | 🚨 +52.75 MB 🚨 |
| Bundle Size Analyzer | node | node |
@storybook/csf-plugin
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 9 | 🚨 +9 🚨 |
| Self size | 0 B | 7 KB | 🚨 +7 KB 🚨 |
| Dependency size | 0 B | 1.29 MB | 🚨 +1.29 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
eslint-plugin-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 20 | 🚨 +20 🚨 |
| Self size | 0 B | 137 KB | 🚨 +137 KB 🚨 |
| Dependency size | 0 B | 3.70 MB | 🚨 +3.70 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/mcp
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 11 | 🚨 +11 🚨 |
| Self size | 0 B | 144 KB | 🚨 +144 KB 🚨 |
| Dependency size | 0 B | 2.74 MB | 🚨 +2.74 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-dom-shim
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 0 B | 19 KB | 🚨 +19 KB 🚨 |
| Dependency size | 0 B | 1 KB | 🚨 +1 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-create-react-app
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 68 | 🚨 +68 🚨 |
| Self size | 0 B | 32 KB | 🚨 +32 KB 🚨 |
| Dependency size | 0 B | 6.12 MB | 🚨 +6.12 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 157 | 🚨 +157 🚨 |
| Self size | 0 B | 18 KB | 🚨 +18 KB 🚨 |
| Dependency size | 0 B | 34.39 MB | 🚨 +34.39 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-server-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 10 | 🚨 +10 🚨 |
| Self size | 0 B | 7 KB | 🚨 +7 KB 🚨 |
| Dependency size | 0 B | 1.20 MB | 🚨 +1.20 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/html
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 29 KB | 🚨 +29 KB 🚨 |
| Dependency size | 0 B | 33 KB | 🚨 +33 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preact
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 2 | 🚨 +2 🚨 |
| Self size | 0 B | 47 KB | 🚨 +47 KB 🚨 |
| Dependency size | 0 B | 33 KB | 🚨 +33 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 59 | 🚨 +59 🚨 |
| Self size | 0 B | 1.46 MB | 🚨 +1.46 MB 🚨 |
| Dependency size | 0 B | 12.28 MB | 🚨 +12.28 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 3 | 🚨 +3 🚨 |
| Self size | 0 B | 9 KB | 🚨 +9 KB 🚨 |
| Dependency size | 0 B | 719 KB | 🚨 +719 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/svelte
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 3 | 🚨 +3 🚨 |
| Self size | 0 B | 49 KB | 🚨 +49 KB 🚨 |
| Dependency size | 0 B | 601 KB | 🚨 +601 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 91 | 🚨 +91 🚨 |
| Self size | 0 B | 103 KB | 🚨 +103 KB 🚨 |
| Dependency size | 0 B | 18.15 MB | 🚨 +18.15 MB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/web-components
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 3 | 🚨 +3 🚨 |
| Self size | 0 B | 80 KB | 🚨 +80 KB 🚨 |
| Dependency size | 0 B | 48 KB | 🚨 +48 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe MCP migration centralizes Storybook tools in shared toolsets. It adds hosted documentation adapters, request-scoped manifest access, structured outcomes, feature gating, CLI consumer telemetry, and package contract tests. Legacy MCP implementations and review-channel wiring are removed. ChangesMCP toolset migration
Possibly related issues
Possibly related PRs
✨ 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: 9
🧹 Nitpick comments (9)
code/addons/mcp/src/tools/toolset-tools.test.ts (1)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueModule mocks don't use
spy: true/beforeEachper repo Vitest guidelines.
vi.mock('storybook/internal/core-server', ...)andvi.mock('../telemetry.ts', ...)use plain factory functions instead of thespy: trueoption, and the mock return values are set at module scope instead of inside abeforeEachblock.As per coding guidelines, "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests" and "Implement mock behaviors inbeforeEachblocks 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/tools/toolset-tools.test.ts` around lines 1 - 11, Update the module mocks in the toolset-tools test to use Vitest’s spy mode with spy: true instead of factory functions, and move mock behavior/configuration into a beforeEach block. Preserve the existing mocked getService and collectTelemetry behavior while ensuring each test starts with freshly configured mocks.Source: Coding guidelines
code/core/src/shared/open-service/toolset-definition.ts (1)
53-60: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
outputSchemais unconstrained relative toTOutput.
handleroutput and the publishedoutputSchemacan drift silently; since the adapter validatesstructuredContentagainst that schema, a mismatch only surfaces as an MCP runtime validation failure. Consider tying the schema's inferred input to the handler's data.♻️ Sketch
-export type ToolsetMethod<TSchema extends AnySchema = AnySchema, TOutput = unknown> = { +export type ToolsetMethod<TSchema extends AnySchema = AnySchema, TOutput = unknown> = { description: ToolsetMethodDescription; schema: TSchema; /** Published as the MCP tool's `outputSchema`. Declare it only where the JSON is contractual. */ - outputSchema?: AnySchema; + outputSchema?: StandardSchemaV1<Awaited<TOutput>, unknown>; handler: (input: StandardSchemaV1.InferOutput<TSchema>, context: ToolsetCtx) => TOutput; format: (data: Awaited<TOutput>, context: ToolsetCtx) => string | string[]; };🤖 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/core/src/shared/open-service/toolset-definition.ts` around lines 53 - 60, Update ToolsetMethod so the optional outputSchema is generically tied to TOutput, constraining its schema type to one whose inferred output matches the handler’s Awaited<TOutput>. Preserve outputSchema as optional and keep the existing handler and format signatures unchanged.code/core/src/shared/open-service/toolsets/docs/definition.test.ts (1)
68-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the exported
resolveToolsetDescriptionhere.This re-implements the helper that ships in
toolset-definition.ts(and is used by the stories toolset tests), so the test can drift from production resolution semantics.♻️ Proposed change
- const describe_ = toolset.methods.list.description; - const resolved = typeof describe_ === 'function' ? describe_(mcpCtx) : describe_; - const resolvedCli = typeof describe_ === 'function' ? describe_(cliCtx) : describe_; + const resolved = resolveToolsetDescription(toolset.methods.list.description, mcpCtx); + const resolvedCli = resolveToolsetDescription(toolset.methods.list.description, cliCtx);Plus the import:
-import type { ToolsetCtx } from '../../toolset-definition.ts'; +import { resolveToolsetDescription, type ToolsetCtx } from '../../toolset-definition.ts';🤖 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/core/src/shared/open-service/toolsets/docs/definition.test.ts` around lines 68 - 75, Update the test around “cross-references the show tool per consumer in its description” to import and use the exported resolveToolsetDescription helper from toolset-definition.ts for both mcpCtx and cliCtx, removing the inline typeof/function resolution so the test follows production semantics.code/core/src/shared/open-service/toolset-definition.test-d.ts (1)
39-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssignability checks won't catch an
anyregression.If inference degraded to
anyfor handler input or format data, these assignments would still compile andtoBeFunction()would still pass. Exact-type assertions on the parameters would actually pin the contract, e.g.:♻️ Suggested assertion style
- const greet: (input: { name: string }, context: ToolsetCtx) => Promise<{ greeting: string }> = - exampleToolset.methods.greet.handler; + expectTypeOf(exampleToolset.methods.greet.handler) + .parameter(0) + .toEqualTypeOf<{ name: string }>(); + expectTypeOf(exampleToolset.methods.greet.handler).returns.resolves.toEqualTypeOf<{ + greeting: string; + }>();🤖 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/core/src/shared/open-service/toolset-definition.test-d.ts` around lines 39 - 60, Strengthen the type assertions in the handler and format tests around exampleToolset.methods.greet.handler and reviewToolset.methods.create.handler, plus their format counterparts, so parameter types are checked exactly rather than only through assignability. Use exact-type assertions for the input/data parameters to detect any regression to any, while retaining the existing return and context contract checks.code/core/src/shared/open-service/toolsets/docs/access-parity.test.ts (1)
155-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParity assertions can pass vacuously.
If both modes regressed to empty (or identical error) text, every
toBehere still passes. Anchoring one side to expected content keeps the comparison meaningful.💚 Suggested guard
it.each([false, true])('list with withStoryIds=%s', async (withStoryIds) => { - expect(await renderList(serviceToolset(), withStoryIds)).toBe( - await renderList(manifestToolset(), withStoryIds) - ); + const serviceText = await renderList(serviceToolset(), withStoryIds); + expect(serviceText).toContain('button'); + expect(serviceText).toBe(await renderList(manifestToolset(), withStoryIds)); });🤖 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/core/src/shared/open-service/toolsets/docs/access-parity.test.ts` around lines 155 - 173, Update the parity tests for renderList, renderShow, and showStory so each assertion also verifies non-empty, expected output independently of the other toolset. Retain the serviceToolset-versus-manifestToolset equality checks, but anchor at least one side of each comparison to meaningful content to prevent both implementations returning identical empty or error text from passing vacuously.code/core/src/shared/open-service/index.ts (1)
24-29: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep
clearToolsetRegistryout ofstorybook/open-serviceThis is a test-only reset helper; export it from./toolset-registry.tsinstead of widening the shared API surface.🤖 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/core/src/shared/open-service/index.ts` around lines 24 - 29, Remove clearToolsetRegistry from the export list in the open-service index while retaining the other toolset-registry exports, so the test-only reset helper is not exposed through the shared storybook/open-service API.Source: Coding guidelines
code/core/src/shared/open-service/toolsets/docs/access-service.ts (1)
146-160: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse the batch story-docs query here.
storyDocsForAllComponentsalready exists, sowithStoryIds: truecan load once and pick the requested ids instead of spawning one extraction per component.🤖 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/core/src/shared/open-service/toolsets/docs/access-service.ts` around lines 146 - 160, Replace the per-ID Promise.all loading in the storiesById construction with the existing storyDocsForAllComponents batch query when withStoryIds is true, then select only the requested storyBasedIds from the batch result. Preserve the empty-map behavior when withStoryIds is false and keep the existing ID-to-payload mapping contract.code/core/src/shared/open-service/toolsets/test/definition.ts (1)
193-210: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueTelemetry is awaited while the run queue lock is still held.
reportRunTelemetryruns inside thetryblock beforedone(), so a slow/hanging telemetry call delays the next queued run even though the test run itself has finished. Consider reporting after releasing the lock (or not awaiting it).♻️ Release the queue before reporting
handler: async (input, ctx): Promise<TestRunData> => { const done = await queue.wait(); + let data: TestRunData; try { const output = await runStoryTests({ channel, getIndex: storyIndex.getIndex, stories: input.stories, a11y: input.a11y, }); - const data: TestRunData = { ...output, a11y: input.a11y }; - - await reportRunTelemetry(data, input, ctx); - - return data; + data = { ...output, a11y: input.a11y }; } finally { done(); } + + await reportRunTelemetry(data, input, ctx); + + return data; },🤖 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/core/src/shared/open-service/toolsets/test/definition.ts` around lines 193 - 210, Update the handler around queue.wait and reportRunTelemetry so done() releases the queue immediately after runStoryTests and TestRunData creation, before telemetry reporting. Preserve returning the completed data and ensure the lock is still released when the test run throws.code/core/src/core-server/utils/manifests/manifests.ts (1)
126-134: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the manifest loader here.
listandresolveboth callgetManifests()again, so the docs path re-runsgetManifestEntries()andexperimental_manifestson every request. If this stays on the hot path, memoize the watched result instead of rebuilding it each time.🤖 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/core/src/core-server/utils/manifests/manifests.ts` around lines 126 - 134, Update loadManifests to memoize the watched manifest result per Presets input, so repeated list and resolve requests reuse the same getManifests output instead of rerunning getManifestEntries and experimental_manifests. Preserve the existing watch behavior and ensure distinct preset configurations do not incorrectly share cached data.
🤖 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/storybook-ai-metadata.test.ts`:
- Around line 44-47: Update the mock for ./utils/addon-vitest.ts to use Vitest’s
spy mode with { spy: true } instead of an async factory. Configure
vi.mocked(getAddonVitestConstants) in the test file’s beforeEach while
preserving the existing mocked behavior.
In `@code/core/src/core-server/presets/common-preset.ts`:
- Around line 398-405: Update the docs toolset registration around
createDocsToolset and createServiceDocsAccess so experimentalDocgenServer uses
service-backed docs only when core/docgen is registered, including ignorePreview
and missing-worker cases. Otherwise provide the established unavailable
docs-access path or retain manifest-backed access, ensuring registered docs
tools cannot invoke getService('core/docgen') when that service is absent.
In `@code/core/src/shared/open-service/toolsets/docs/definition.ts`:
- Around line 123-130: Update the matched-story path in the tool definition
around formatStoryDocumentation so a story whose documentation result is empty
falls back to a clear message indicating that the story has no snippet, instead
of returning blank text. Preserve the existing not-found handling and normal
formatted documentation for stories with content.
- Around line 35-38: Update describeList to remove the storybookId guidance from
its returned description, including the multi-Storybook scoping instruction,
while preserving the existing withStoryIds guidance and all other documentation
workflow text.
In `@code/core/src/shared/open-service/toolsets/review/definition.ts`:
- Around line 64-77: Update reviewCreateOutputSchema to include numeric
collectionCount and storyCount fields alongside reviewUrl, matching the
ReviewCreateOutput type and the handler/format() output so structuredContent
preserves both counters.
In `@code/core/src/shared/open-service/toolsets/stories/definition.ts`:
- Around line 237-239: Update the changed method definition in the stories
toolset to declare outputSchema using the existing changedOutputSchema, matching
preview and findByComponent. Keep the handler’s ChangedStoriesOutput return
behavior unchanged so stories.changed exposes schema-validated structured
content.
In `@code/core/src/shared/open-service/toolsets/stories/story-input.ts`:
- Around line 11-23: Update the storyInputProps.props description to reference a
capability backed by an MCP_TOOL_NAMES entry instead of the hardcoded
get-storybook-story-instructions tool name, preserving the guidance about
locating component props while keeping the description meaningful for non-MCP
consumers.
In `@code/core/src/shared/open-service/toolsets/test/format.ts`:
- Around line 56-92: Harden getA11yViolations by validating each violation’s
nodes value is an array before mapping it, returning an empty node list
otherwise. In countA11yViolations, validate each a11yReports value is an array
before iterating, and apply the same guard to the mirrored counting path, so
malformed payloads degrade without throwing.
In `@code/lib/mcp/src/dist-contract.test.ts`:
- Around line 10-42: Update the filesystem setup in the published package
contract tests to use memfs instead of direct node:fs access. Add the standard
memfs volume setup, reset vol in beforeEach, and seed package.json and
dist/index.js fixtures so existsSync, readFileSync, and statSync operate on
isolated test data while preserving the existing assertions.
---
Nitpick comments:
In `@code/addons/mcp/src/tools/toolset-tools.test.ts`:
- Around line 1-11: Update the module mocks in the toolset-tools test to use
Vitest’s spy mode with spy: true instead of factory functions, and move mock
behavior/configuration into a beforeEach block. Preserve the existing mocked
getService and collectTelemetry behavior while ensuring each test starts with
freshly configured mocks.
In `@code/core/src/core-server/utils/manifests/manifests.ts`:
- Around line 126-134: Update loadManifests to memoize the watched manifest
result per Presets input, so repeated list and resolve requests reuse the same
getManifests output instead of rerunning getManifestEntries and
experimental_manifests. Preserve the existing watch behavior and ensure distinct
preset configurations do not incorrectly share cached data.
In `@code/core/src/shared/open-service/index.ts`:
- Around line 24-29: Remove clearToolsetRegistry from the export list in the
open-service index while retaining the other toolset-registry exports, so the
test-only reset helper is not exposed through the shared storybook/open-service
API.
In `@code/core/src/shared/open-service/toolset-definition.test-d.ts`:
- Around line 39-60: Strengthen the type assertions in the handler and format
tests around exampleToolset.methods.greet.handler and
reviewToolset.methods.create.handler, plus their format counterparts, so
parameter types are checked exactly rather than only through assignability. Use
exact-type assertions for the input/data parameters to detect any regression to
any, while retaining the existing return and context contract checks.
In `@code/core/src/shared/open-service/toolset-definition.ts`:
- Around line 53-60: Update ToolsetMethod so the optional outputSchema is
generically tied to TOutput, constraining its schema type to one whose inferred
output matches the handler’s Awaited<TOutput>. Preserve outputSchema as optional
and keep the existing handler and format signatures unchanged.
In `@code/core/src/shared/open-service/toolsets/docs/access-parity.test.ts`:
- Around line 155-173: Update the parity tests for renderList, renderShow, and
showStory so each assertion also verifies non-empty, expected output
independently of the other toolset. Retain the
serviceToolset-versus-manifestToolset equality checks, but anchor at least one
side of each comparison to meaningful content to prevent both implementations
returning identical empty or error text from passing vacuously.
In `@code/core/src/shared/open-service/toolsets/docs/access-service.ts`:
- Around line 146-160: Replace the per-ID Promise.all loading in the storiesById
construction with the existing storyDocsForAllComponents batch query when
withStoryIds is true, then select only the requested storyBasedIds from the
batch result. Preserve the empty-map behavior when withStoryIds is false and
keep the existing ID-to-payload mapping contract.
In `@code/core/src/shared/open-service/toolsets/docs/definition.test.ts`:
- Around line 68-75: Update the test around “cross-references the show tool per
consumer in its description” to import and use the exported
resolveToolsetDescription helper from toolset-definition.ts for both mcpCtx and
cliCtx, removing the inline typeof/function resolution so the test follows
production semantics.
In `@code/core/src/shared/open-service/toolsets/test/definition.ts`:
- Around line 193-210: Update the handler around queue.wait and
reportRunTelemetry so done() releases the queue immediately after runStoryTests
and TestRunData creation, before telemetry reporting. Preserve returning the
completed data and ensure the lock is still released when the test run throws.
🪄 Autofix (Beta)
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: 9f070aee-18b2-4346-b8cb-abea88bbc116
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (92)
AGENTS.mdcode/addons/mcp/src/constants.tscode/addons/mcp/src/mcp-handler.test.tscode/addons/mcp/src/preset.test.tscode/addons/mcp/src/preset.tscode/addons/mcp/src/storybook-ai-metadata.test.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/review-origin.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.tscode/addons/mcp/src/tools/toolset-tools.test.tscode/addons/mcp/src/tools/toolset-tools.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/find-story-ids.test.tscode/addons/mcp/src/utils/find-story-ids.tscode/addons/mcp/src/utils/get-tool-availability.tscode/addons/vitest/src/preset.tscode/core/build-config.tscode/core/package.jsoncode/core/src/core-server/index.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/core-server/utils/manifests/manifests.tscode/core/src/server-errors.tscode/core/src/shared/open-service/index.tscode/core/src/shared/open-service/toolset-definition.test-d.tscode/core/src/shared/open-service/toolset-definition.tscode/core/src/shared/open-service/toolset-names.tscode/core/src/shared/open-service/toolset-registry.test.tscode/core/src/shared/open-service/toolset-registry.tscode/core/src/shared/open-service/toolset-types.tscode/core/src/shared/open-service/toolsets/docs/access-manifest.test.tscode/core/src/shared/open-service/toolsets/docs/access-manifest.tscode/core/src/shared/open-service/toolsets/docs/access-parity.test.tscode/core/src/shared/open-service/toolsets/docs/access-service.test.tscode/core/src/shared/open-service/toolsets/docs/access-service.tscode/core/src/shared/open-service/toolsets/docs/access.tscode/core/src/shared/open-service/toolsets/docs/classify-services.test.tscode/core/src/shared/open-service/toolsets/docs/classify-services.tscode/core/src/shared/open-service/toolsets/docs/definition.test.tscode/core/src/shared/open-service/toolsets/docs/definition.tscode/core/src/shared/open-service/toolsets/docs/map.test.tscode/core/src/shared/open-service/toolsets/docs/map.tscode/core/src/shared/open-service/toolsets/docs/public.tscode/core/src/shared/open-service/toolsets/docs/runtime-agnostic.test.tscode/core/src/shared/open-service/toolsets/review/definition.test.tscode/core/src/shared/open-service/toolsets/review/definition.tscode/core/src/shared/open-service/toolsets/stories/definition.test.tscode/core/src/shared/open-service/toolsets/stories/definition.tscode/core/src/shared/open-service/toolsets/stories/find-by-component.test.tscode/core/src/shared/open-service/toolsets/stories/find-by-component.tscode/core/src/shared/open-service/toolsets/stories/format.tscode/core/src/shared/open-service/toolsets/stories/resolve-component-stories.test.tscode/core/src/shared/open-service/toolsets/stories/resolve-component-stories.tscode/core/src/shared/open-service/toolsets/stories/story-input.tscode/core/src/shared/open-service/toolsets/stories/unreachable-files.tscode/core/src/shared/open-service/toolsets/test/definition.test.tscode/core/src/shared/open-service/toolsets/test/definition.tscode/core/src/shared/open-service/toolsets/test/format.tscode/core/src/shared/open-service/toolsets/test/run.test.tscode/core/src/shared/open-service/toolsets/test/run.tscode/core/src/shared/review/events.tscode/core/src/shared/review/review-state.tscode/lib/mcp/package.jsoncode/lib/mcp/src/dist-contract.test.tscode/lib/mcp/src/tools/get-documentation-for-story.tscode/lib/mcp/src/tools/get-documentation.tscode/lib/mcp/src/utils/adapt-core-manifest.tscode/lib/mcp/src/utils/dedent.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/parse-react-docgen.ts
💤 Files with no reviewable changes (24)
- code/addons/mcp/src/tools/display-review.ts
- code/core/src/shared/open-service/toolsets/docs/map.test.ts
- code/core/src/core-server/server-channel/review-channel.ts
- code/addons/mcp/src/tools/preview-stories.test.ts
- code/lib/mcp/src/utils/dedent.ts
- code/core/src/shared/open-service/toolsets/docs/classify-services.test.ts
- code/addons/mcp/src/tools/get-changed-stories.test.ts
- code/addons/mcp/src/utils/build-args-param.test.ts
- code/addons/mcp/src/utils/detect-unreachable-changes.test.ts
- code/core/src/shared/open-service/toolsets/docs/classify-services.ts
- code/addons/mcp/src/utils/find-story-ids.ts
- code/addons/mcp/src/utils/build-args-param.ts
- code/addons/mcp/src/tools/run-story-tests.test.ts
- code/addons/mcp/src/utils/find-story-ids.test.ts
- code/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.ts
- code/addons/mcp/src/tools/display-review.test.ts
- code/addons/mcp/src/tools/get-changed-stories.ts
- code/addons/mcp/src/utils/detect-unreachable-changes.ts
- code/addons/mcp/src/tools/run-story-tests.ts
- code/addons/mcp/src/tools/get-stories-by-component.ts
- code/addons/mcp/src/constants.ts
- code/core/src/shared/open-service/toolsets/docs/map.ts
- code/addons/mcp/src/tools/get-stories-by-component.test.ts
- code/core/src/core-server/server-channel/review-channel.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/core-server/presets/common-preset.ts (1)
320-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the empty review-channel branch.
After removing
initReviewChannel(channel), thisifcontains only stale comments describing a listener and teardown that no longer exist. Delete the conditional so the server-channel code reflects its actual behavior.🤖 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/core/src/core-server/presets/common-preset.ts` around lines 320 - 325, Remove the empty isReviewFeatureEnabled conditional and its stale teardown comments from the server-channel initialization flow. Keep the surrounding server-channel setup and other init*Channel calls unchanged.
🤖 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.
Nitpick comments:
In `@code/core/src/core-server/presets/common-preset.ts`:
- Around line 320-325: Remove the empty isReviewFeatureEnabled conditional and
its stale teardown comments from the server-channel initialization flow. Keep
the surrounding server-channel setup and other init*Channel calls unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0ea7bff-8708-4c32-aafe-e441fe9cb02a
📒 Files selected for processing (4)
code/addons/mcp/src/storybook-ai-metadata.test.tscode/core/src/core-server/presets/common-preset.tscode/core/src/shared/review/features.test.tscode/core/src/shared/review/features.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/src/shared/open-service/README.md`:
- Around line 108-110: Complete the outputSchema documentation sentence in the
open-service README by stating that structuredContent is narrowed to the
declared outputSchema. Leave the surrounding handler documentation unchanged.
🪄 Autofix (Beta)
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: 83221765-b166-4f5f-afc0-62613efa4964
📒 Files selected for processing (12)
AGENTS.mdcode/addons/mcp/src/constants.tscode/addons/vitest/src/preset.test.tscode/addons/vitest/src/preset.tscode/core/src/core-server/presets/common-preset.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/toolsets/docs/classify-services.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/parse-react-docgen.test.tscode/core/src/shared/open-service/toolsets/review/definition.tscode/core/src/shared/open-service/toolsets/stories/story-input.tscode/lib/mcp/src/utils/parse-react-docgen.ts
💤 Files with no reviewable changes (2)
- code/addons/mcp/src/constants.ts
- code/lib/mcp/src/utils/parse-react-docgen.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/shared/open-service/toolsets/stories/story-input.ts
- AGENTS.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/addons/mcp/src/instructions/build-server-instructions.ts (1)
92-100: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDrop the CI run-ID from the comment.
Line 96 embeds a run ID (
run 28673251562)) inside the maintenance comment. As per coding guidelines, comments should explain rationale, not carry provenance/ticket-style references.✏️ Proposed fix
// Validation Workflow from the latest release (plus the run-story-tests-only - // rule added after agents substituted `npm run test:stories`, run - // 28673251562), and the shared docs-toolset Documentation Workflow. With + // rule added after agents substituted `npm run test:stories`), and the + // shared docs-toolset Documentation Workflow. With🤖 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/instructions/build-server-instructions.ts` around lines 92 - 100, Remove the CI run ID “28673251562” from the maintenance comment in the test/docs section, while preserving the surrounding explanation of the legacy Validation Workflow and run-story-tests-only rule.Source: Coding guidelines
🧹 Nitpick comments (2)
code/lib/mcp/src/tools/get-documentation-for-story.test.ts (2)
398-398: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale test name.
getManifestsno longer exists in this flow; rename to reflect the manifest provider (e.g. "should pass remote source to the manifest provider"). Same wording appears incode/lib/mcp/src/tools/get-documentation.test.ts.🤖 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/tools/get-documentation-for-story.test.ts` at line 398, Rename the test case around the remote-source assertion in get-documentation-for-story.test.ts to refer to the manifest provider instead of getManifests, and apply the same wording update to the corresponding test in get-documentation.test.ts.
25-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
createManifestProvidertest helper. The identicalServedManifeststype and provider factory are copied into three test files; they will drift as the manifest paths/errors evolve.
code/lib/mcp/src/tools/get-documentation-for-story.test.ts#L25-L45: move the type and factory into a shared test util (e.g.code/lib/mcp/src/tools/test-manifest-provider.ts) and import it here.code/lib/mcp/src/tools/get-documentation.test.ts#L26-L46: delete the local copy and import the shared helper.code/lib/mcp/src/tools/list-all-documentation.test.ts#L31-L51: delete the local copy and import the shared helper.🤖 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/tools/get-documentation-for-story.test.ts` around lines 25 - 45, Extract the shared ServedManifests type and createManifestProvider factory into a test utility, then import and use it in code/lib/mcp/src/tools/get-documentation-for-story.test.ts#L25-L45, code/lib/mcp/src/tools/get-documentation.test.ts#L26-L46, and code/lib/mcp/src/tools/list-all-documentation.test.ts#L31-L51; remove each duplicated local definition while preserving the existing manifest paths and error behavior.
🤖 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/core/src/shared/open-service/toolsets/docs/access-provider.ts`:
- Around line 223-232: Update the JSON-pointer traversal loop around the target
lookup to use Object.hasOwn for key membership instead of the in operator, so
inherited properties such as constructor and __proto__ are rejected as missing
while own properties continue resolving normally.
---
Outside diff comments:
In `@code/addons/mcp/src/instructions/build-server-instructions.ts`:
- Around line 92-100: Remove the CI run ID “28673251562” from the maintenance
comment in the test/docs section, while preserving the surrounding explanation
of the legacy Validation Workflow and run-story-tests-only rule.
---
Nitpick comments:
In `@code/lib/mcp/src/tools/get-documentation-for-story.test.ts`:
- Line 398: Rename the test case around the remote-source assertion in
get-documentation-for-story.test.ts to refer to the manifest provider instead of
getManifests, and apply the same wording update to the corresponding test in
get-documentation.test.ts.
- Around line 25-45: Extract the shared ServedManifests type and
createManifestProvider factory into a test utility, then import and use it in
code/lib/mcp/src/tools/get-documentation-for-story.test.ts#L25-L45,
code/lib/mcp/src/tools/get-documentation.test.ts#L26-L46, and
code/lib/mcp/src/tools/list-all-documentation.test.ts#L31-L51; remove each
duplicated local definition while preserving the existing manifest paths and
error behavior.
🪄 Autofix (Beta)
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: 4de2b4e3-601f-4c8f-a810-9fa6d936b90b
⛔ 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 (49)
AGENTS.mdagent-eval/lib/templates.test.tsagent-eval/lib/templates.tscode/addons/mcp/package.jsoncode/addons/mcp/src/auth/composition-auth.tscode/addons/mcp/src/auth/resolve-composition-sources.tscode/addons/mcp/src/instructions/build-server-instructions.tscode/addons/mcp/src/manifests/in-process-provider.tscode/addons/mcp/src/mcp-handler.tscode/addons/mcp/src/telemetry.test.tscode/addons/mcp/src/telemetry.tscode/addons/mcp/src/tools/tool-registry.tscode/addons/mcp/src/tools/toolset-tools.tscode/addons/mcp/src/types.tscode/core/src/shared/open-service/toolsets/docs/access-manifest.tscode/core/src/shared/open-service/toolsets/docs/access-parity.test.tscode/core/src/shared/open-service/toolsets/docs/access-provider.test.tscode/core/src/shared/open-service/toolsets/docs/access-provider.tscode/core/src/shared/open-service/toolsets/docs/definition.test.tscode/core/src/shared/open-service/toolsets/docs/definition.tscode/core/src/shared/open-service/toolsets/docs/instructions.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/adapt-core-manifest.test.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/manifest-types.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.test.tscode/core/src/shared/open-service/toolsets/docs/manifest-formatter/markdown.tscode/core/src/shared/open-service/toolsets/docs/multi-source.tscode/core/src/shared/open-service/toolsets/docs/public.tscode/core/src/shared/open-service/toolsets/docs/sources.test.tscode/core/src/shared/open-service/toolsets/docs/sources.tscode/lib/mcp/bin.tscode/lib/mcp/src/dist-contract.test.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/register.tscode/lib/mcp/src/utils/adapt-core-manifest.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.tscode/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.test.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/requires-own-mcp.ts
💤 Files with no reviewable changes (8)
- code/lib/mcp/src/instructions.md
- code/addons/mcp/package.json
- code/lib/mcp/src/tools/list-all-documentation.ts
- code/lib/mcp/src/tools/get-documentation.ts
- code/lib/mcp/src/utils/get-manifest.ts
- code/lib/mcp/src/utils/manifest-formatter/extract-docs-summary.test.ts
- code/lib/mcp/src/tools/get-documentation-for-story.ts
- code/lib/mcp/src/utils/adapt-core-manifest.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- AGENTS.md
- code/core/src/shared/open-service/toolsets/docs/access-manifest.ts
kasperpeulen
left a comment
kasperpeulen
left a comment
There was a problem hiding this comment.
🤖 Review Codex:
Reviewed current head cc487c61188b5f20a25907e3fa3a44249e1ebcb2. The inline findings below are the issues I believe still need attention.
kasperpeulen
left a comment
kasperpeulen
left a comment
There was a problem hiding this comment.
🤖 Review Codex:
Second-pass review on head 146a52e1a46a99ab41b1bfc7c79758962e8c1778. The new inline findings below are additional to the earlier review threads.
|
🤖 Review Codex: [P1] Refresh downstream canary QA from the final fix-round head #35688 user story 17 requires a fresh canary containing all fixes, followed by re-running the chromaui/mcp-server#66 suite and production build. npm currently exposes only |
kasperpeulen
left a comment
kasperpeulen
left a comment
There was a problem hiding this comment.
🤖 Review Codex:
Re-review of the latest fix delta at cf401786a4; one new inline correctness finding follows.
|
🤖 Review Codex: [P1] Refresh downstream proof from the new final head The previous canary follow-up was satisfied for [P3] Keep the documented The PR description still says the type grows “from three fields to five” and lists only |
|
Re the two [P1] downstream-refresh comments: done from the final head. Canary 🤖 Addressed by Claude Code |
kasperpeulen
left a comment
kasperpeulen
left a comment
There was a problem hiding this comment.
🤖 Review Codex:
Re-review of the final follow-up at 5d4368ba; the functional fix and final-head canary verification pass. One new Standards comment follows.
|
🤖 Review Codex: [P3] Correct the final-head SHA in the downstream QA record The substantive #35688 US17 proof now checks out: npm reports The earlier [P3] on this PR’s “three fields to five” construct remains unresolved and is already recorded in the prior Codex comment, so I have not duplicated it here. |
kasperpeulen
left a comment
kasperpeulen
left a comment
There was a problem hiding this comment.
🤖 Review Codex: Standards re-review of af29f015. I found two new actionable standards issues; the inline comments contain the concrete fixes.
|
🤖 Review Codex: [P1] Publish and prove a canary from the final head before merging The final head is |
|
Re the [P1] final-head canary: done. 🤖 Addressed by Claude Code |
|
Real-project QA on canary
One QA-step correction folded into the description: the stock dev server answers MCP only at the root 🤖 Addressed by Claude Code |
|
Full local eval matrix on canary
81/84 (96%) overall; every failure classifies as Harness note, for whoever runs evals on cross-cutting core+addon PRs: with core pinned to npm 🤖 Addressed by Claude Code |
storybook tools CLI derived at runtime from the OSA toolsets (own realm)
#35716
b1a8dee to
28d2853
Compare
JReinhold
left a comment
JReinhold
left a comment
There was a problem hiding this comment.
One important thing I don't fully get here, is whether or not addon-mcp now includes MCP tools for any toolsets registered, or if it only has a pre-defined list of tools still. We want the former, eventually.
28d2853 to
1da8a9f
Compare
1da8a9f to
0a5ad88
Compare
JReinhold
added
qa:skip
2 participants
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.