Repository navigation
Skills M5a: CLI: Add the public storybook tools command derived at runtime from the OSA toolsets - #35719
Closed
kasperpeulen wants to merge 13 commits into
Closed
Skills M5a: CLI: Add the public storybook tools command derived at runtime from the OSA toolsets#35719kasperpeulen wants to merge 13 commits into
storybook tools command derived at runtime from the OSA toolsets#35719kasperpeulen wants to merge 13 commits into
Conversation
kasperpeulen
added
feature request
ci:normal
Contributor
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 21.77 MB | 21.82 MB | 🚨 +50 KB 🚨 |
| Dependency size | 31.24 MB | 31.24 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 205 | 205 | 0 |
| Self size | 833 KB | 833 KB | 0 B |
| Dependency size | 86.50 MB | 86.55 MB | 🚨 +50 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 198 | 0 |
| Self size | 32 KB | 32 KB | 0 B |
| Dependency size | 84.98 MB | 85.03 MB | 🚨 +50 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 74 | 74 | 0 |
| Self size | 1.09 MB | 1.09 MB | 🎉 -66 B 🎉 |
| Dependency size | 53.00 MB | 53.05 MB | 🚨 +50 KB 🚨 |
| Bundle Size Analyzer | node | node |
Contributor
|
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 PR adds the public ChangesStorybook tools CLI
Sequence Diagram(s)sequenceDiagram
participant User
participant StorybookDispatcher
participant ToolsPassthrough
participant runToolsCommand
participant StorybookRuntime
participant VitestResponder
User->>StorybookDispatcher: invoke storybook tools
StorybookDispatcher->>ToolsPassthrough: route tools command
ToolsPassthrough->>runToolsCommand: pass toolset, method, and flags
runToolsCommand->>StorybookRuntime: load configuration and resolve runtime
StorybookRuntime-->>runToolsCommand: toolset context and optional origin
runToolsCommand->>VitestResponder: dispatch test.run request
VitestResponder-->>runToolsCommand: test result, error, or cancellation
runToolsCommand-->>ToolsPassthrough: outcome, output, and exit code
ToolsPassthrough-->>User: print stdout or write output file
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
code/core/src/cli/tools/bootstrap.ts (2)
99-105: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider surfacing why the builder adapter failed, not just at debug level.
The
try/catchtreats "builder doesn't implementchangeDetectionAdapter" the same as "builder threw an unexpected error," and only records the real cause vialogger.debug, which most users never see. If a builder throws because of a genuine misconfiguration (not because it lacks support), the resulting "module graph unavailable" message gives no hint why.Consider logging at a more visible level (or attaching the caught error's message to the readiness reason) so a misconfigured project doesn't look identical to "builder doesn't support change detection."
🤖 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/cli/tools/bootstrap.ts` around lines 99 - 105, Update the catch handling around previewBuilder.changeDetectionAdapter in the bootstrap flow to surface unexpected adapter-construction failures beyond debug logging, while preserving the unsupported-builder behavior for absent adapters. Include the caught error’s message or stack in a visible log or in the readiness reason passed through resolveChangeDetectionAdapter so misconfiguration is distinguishable from lack of support.
1-111: 🎯 Functional Correctness | 🔵 TrivialAdd direct unit tests for
bootstrapToolsRuntime/hostModuleGraphInProcess.
run.test.tsonly exercisesrunToolsCommandagainst a mockedbootstrap, so this file's own branching (hosting vs. not hosting the module graph, theworkingDirvalue passed toChangeDetectionService, and the deferred-adapter-must-always-settle invariant called out in the doc comment) has no direct test coverage. Given the doc comment's explicit warning that "any graph query would hang forever" if the deferred adapter never settles, a focused test asserting the resolution path in both branches would meaningfully reduce risk.🤖 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/cli/tools/bootstrap.ts` around lines 1 - 111, Add focused unit tests for bootstrapToolsRuntime and hostModuleGraphInProcess, covering both hostModuleGraph branches and asserting the deferred change-detection adapter is resolved so readiness does not hang. Verify the hosted path constructs ChangeDetectionService with process.cwd() as workingDir, while the non-hosted path resolves the adapter with undefined. Mock the Storybook loading, builder, preset, store, and readiness dependencies to isolate these flows.code/builders/builder-vite/src/change-detection-adapter/headless.test.ts (1)
10-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign mocking with the project's Vitest guidelines.
This file mocks
viteand../vite-config.tswithoutspy: true(Lines 15-16), accesses the mocks through rawvi.hoisted()references instead ofvi.mocked(), and sets mock return values (mockResolvedValue) inside individualit()bodies (Lines 31-35, 49-50) instead of a sharedbeforeEach.Move the mock setup into a
beforeEachblock, and usevi.mocked()to access the mocked functions with type safety.As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests," "Implement mock behaviors inbeforeEachblocks in Vitest tests," and "Usevi.mocked()to type and access the mocked functions 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/builders/builder-vite/src/change-detection-adapter/headless.test.ts` around lines 10 - 63, Align the Vitest setup in createHeadlessViteChangeDetectionAdapter tests with project conventions: add spy: true to the vite and vite-config mocks, access resolveConfig and commonConfig through vi.mocked(), and move their shared mockResolvedValue setup into beforeEach. Keep each test focused on assertions, retaining only test-specific setup such as createOptions and apply expectations.Source: Coding guidelines
code/core/src/cli/tools/register.ts (1)
167-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the issue-number references from comments in both files. Both comments cite a GitHub issue number as a provenance claim. As per coding guidelines for
**/*.{ts,tsx}, comments must carry maintenance-relevant rationale, not ticket codes: "not investigation transcripts, ticket codes, acceptance-criteria codes, provenance claims, or cross-file line references."
code/core/src/cli/tools/register.ts#L167-L172: keep "modeled on theai-commandevent" and remove(storybookjs/storybook#35131).code/core/src/bin/core.ts#L279-L280: keep the "disconnected from any dev server" rationale and remove(storybookjs/storybook#35716).🤖 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/cli/tools/register.ts` around lines 167 - 172, Remove the GitHub issue reference from the comment around the tools-command event in code/core/src/cli/tools/register.ts lines 167-172, preserving the rationale that it is modeled on the ai-command event. Also remove the GitHub issue reference from the comment in code/core/src/bin/core.ts lines 279-280, preserving the rationale about being disconnected from any dev server.Source: Coding guidelines
code/core/src/cli/tools/run.ts (1)
68-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd method-level traits for CLI requirements.
MODULE_GRAPH_METHODSandORIGIN_ONLY_METHODSduplicate metadata that is absent fromToolsetMethod. Add explicit traits and derive these dispatch decisions from each method definition. Otherwise, new methods can usecore/module-graphor require only an origin without updatingcode/core/src/cli/tools/run.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/cli/tools/run.ts` around lines 68 - 82, Extend ToolsetMethod with explicit CLI requirement traits for module-graph and origin-only behavior, then remove the hardcoded MODULE_GRAPH_METHODS and ORIGIN_ONLY_METHODS sets. Update the dispatch logic in the CLI runner to derive these decisions from each method definition’s traits, ensuring newly added core/module-graph or origin-only methods are handled automatically.
🤖 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/cli/tools/bootstrap.ts`:
- Around line 47-78: Thread the resolved cwd from bootstrapToolsRuntime into
hostModuleGraphInProcess instead of relying on process.cwd(). Update the
ChangeDetectionService construction within hostModuleGraphInProcess to use that
passed cwd as workingDir, while preserving the existing target.cwd fallback
resolution.
In `@code/core/src/cli/tools/help.ts`:
- Around line 115-119: Update toJsonSchemaObject to call toJsonSchema with
errorMode: 'throw' so unsupported schemas enter the existing catch path and
return undefined, allowing methodBodyLines to print the unavailable-schema
message instead of treating them as having no arguments.
In `@code/core/src/cli/tools/run.test.ts`:
- Around line 68-81: Update makeDeps so a caller-supplied bootstrap override is
preserved, matching discoverInstance’s override behavior. Resolve
overrides.bootstrap before creating the default mock and ensure the returned
deps and bootstrap reference the caller’s implementation when provided.
In `@code/core/src/cli/tools/tool-tokens.ts`:
- Around line 103-109: Update the `key === 'output'` handling in the token
parser to reject both undefined and empty-string values with the existing
“`--output` requires a file path.” error, while preserving valid paths. Add a
focused test beside the existing `-o`/`--output` tests covering `--output=` and
asserting the parser returns the expected failure.
In `@code/core/src/shared/open-service/toolsets/review/definition.ts`:
- Around line 147-148: Add review.create to the ORIGIN_ONLY_METHODS allowlist
used by runToolsCommand, alongside stories.preview, so its requiresDevServer
requirement does not produce attach-unavailable. Preserve the existing handling
for other methods.
---
Nitpick comments:
In `@code/builders/builder-vite/src/change-detection-adapter/headless.test.ts`:
- Around line 10-63: Align the Vitest setup in
createHeadlessViteChangeDetectionAdapter tests with project conventions: add
spy: true to the vite and vite-config mocks, access resolveConfig and
commonConfig through vi.mocked(), and move their shared mockResolvedValue setup
into beforeEach. Keep each test focused on assertions, retaining only
test-specific setup such as createOptions and apply expectations.
In `@code/core/src/cli/tools/bootstrap.ts`:
- Around line 99-105: Update the catch handling around
previewBuilder.changeDetectionAdapter in the bootstrap flow to surface
unexpected adapter-construction failures beyond debug logging, while preserving
the unsupported-builder behavior for absent adapters. Include the caught error’s
message or stack in a visible log or in the readiness reason passed through
resolveChangeDetectionAdapter so misconfiguration is distinguishable from lack
of support.
- Around line 1-111: Add focused unit tests for bootstrapToolsRuntime and
hostModuleGraphInProcess, covering both hostModuleGraph branches and asserting
the deferred change-detection adapter is resolved so readiness does not hang.
Verify the hosted path constructs ChangeDetectionService with process.cwd() as
workingDir, while the non-hosted path resolves the adapter with undefined. Mock
the Storybook loading, builder, preset, store, and readiness dependencies to
isolate these flows.
In `@code/core/src/cli/tools/register.ts`:
- Around line 167-172: Remove the GitHub issue reference from the comment around
the tools-command event in code/core/src/cli/tools/register.ts lines 167-172,
preserving the rationale that it is modeled on the ai-command event. Also remove
the GitHub issue reference from the comment in code/core/src/bin/core.ts lines
279-280, preserving the rationale about being disconnected from any dev server.
In `@code/core/src/cli/tools/run.ts`:
- Around line 68-82: Extend ToolsetMethod with explicit CLI requirement traits
for module-graph and origin-only behavior, then remove the hardcoded
MODULE_GRAPH_METHODS and ORIGIN_ONLY_METHODS sets. Update the dispatch logic in
the CLI runner to derive these decisions from each method definition’s traits,
ensuring newly added core/module-graph or origin-only methods are handled
automatically.
🪄 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: 20603d34-543b-49a4-9ab1-26c23b46674d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (26)
AGENTS.mdcode/builders/builder-vite/src/change-detection-adapter/headless.test.tscode/builders/builder-vite/src/change-detection-adapter/headless.tscode/builders/builder-vite/src/index.tscode/core/package.jsoncode/core/src/bin/core.tscode/core/src/bin/dispatcher.tscode/core/src/cli/ai/mcp/run-tool.tscode/core/src/cli/tools/bootstrap.tscode/core/src/cli/tools/discover-instance.tscode/core/src/cli/tools/help.tscode/core/src/cli/tools/register.tscode/core/src/cli/tools/run.test.tscode/core/src/cli/tools/run.tscode/core/src/cli/tools/test-support/register-core-toolsets.tscode/core/src/cli/tools/tool-tokens.test.tscode/core/src/cli/tools/tool-tokens.tscode/core/src/core-server/index.tscode/core/src/shared/open-service/README.mdcode/core/src/shared/open-service/toolset-definition.tscode/core/src/shared/open-service/toolset-names.tscode/core/src/shared/open-service/toolsets/review/definition.tscode/core/src/shared/open-service/toolsets/stories/definition.tscode/core/src/shared/open-service/toolsets/test/definition.tscode/core/src/telemetry/types.tscode/core/src/types/modules/core-common.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
code/addons/vitest/src/node/test-run-responder.ts (1)
42-52: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReset the memo when setup fails, so a later request can retry.
storePromise ??=also caches a rejected promise. If the first call tocreateTestRunnerStorefails, for example because story-index generation throws, every later request receives the same "Failed to set up the test runner" error for the lifetime of the process. The dev server shares this memo throughensureTestRunnerStoreinpreset.ts, so a transient startup failure disables the responder permanently.♻️ Proposed fix
export const ensureTestRunnerStore = ({ channel, options }: ResponderOptions): Promise<Store> => - (storePromise ??= createTestRunnerStore({ channel, options })); + (storePromise ??= createTestRunnerStore({ channel, options }).catch((error) => { + // Allow a later request to retry instead of replaying the cached rejection forever. + storePromise = undefined; + throw error; + }));🤖 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/vitest/src/node/test-run-responder.ts` around lines 42 - 52, Update ensureTestRunnerStore so a rejected createTestRunnerStore promise clears storePromise before propagating the setup error, allowing subsequent requests to retry while preserving successful store memoization for the responder and dev server.code/addons/vitest/src/node/test-run-responder.test.ts (1)
15-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the module mocks with the repository spy-mocking rules.
The four
vi.mock()calls use plain factories. The guidelines require thespy: trueoption and mock behaviors defined inbeforeEachwithvi.mocked(). For./boot-test-runner.tsand../logger.tsthe migration is mechanical, because spy mode keeps the real module shape and only needs the behavior assigned per test.♻️ Example for `./boot-test-runner.ts`
-vi.mock('./boot-test-runner.ts', () => ({ - runTestRunner: vi.fn(), -})); +vi.mock('./boot-test-runner.ts', { spy: true });beforeEach(() => { vi.clearAllMocks(); + vi.mocked(runTestRunner).mockResolvedValue(undefined); });As per coding guidelines: "Use
vi.mock()with thespy: trueoption for all package and file mocks in Vitest tests" and "Implement mock behaviors inbeforeEachblocks".🤖 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/vitest/src/node/test-run-responder.test.ts` around lines 15 - 46, Update all four vi.mock calls in the test setup to use spy mode, preserving the real module shapes instead of supplying plain factory replacements. Move the mock behaviors for experimental_UniversalStore.create, experimental_getTestProviderStore, createFileSystemCache, loadPreviewOrConfigFile, runTestRunner, and log into beforeEach blocks using vi.mocked(), while retaining the existing per-test behavior.Source: Coding guidelines
code/core/src/cli/tools/bootstrap.test.ts (1)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove mock return values into
beforeEach.Lines 26-28 set
mockReturnValue/mockResolvedValueinside theitblock. The coding guidelines for Vitest tests require implementing mock behaviors inbeforeEachblocks and avoiding inline mock implementations within test cases.Move the
channelconstant and the two mock setups intobeforeEach, then referencechannelfrom outer scope in the assertion.As per coding guidelines, "Implement mock behaviors in
beforeEachblocks in Vitest tests" and "Avoid inline mock implementations within test cases in Vitest tests."♻️ Proposed refactor
+let channel: Channel; + beforeEach(() => { vi.mocked(prepareHeadlessUniversalStores).mockReset(); vi.mocked(experimental_loadStorybook).mockReset(); vi.mocked(resolveChangeDetectionAdapter).mockImplementation(() => {}); + channel = { isPreparedChannel: true } as unknown as Channel; + vi.mocked(prepareHeadlessUniversalStores).mockReturnValue(channel); + vi.mocked(experimental_loadStorybook).mockResolvedValue({} as never); }); describe('bootstrapToolsRuntime', () => { it('loads the configuration on the same channel the stores were prepared on', async () => { - const channel = { isPreparedChannel: true } as unknown as Channel; - vi.mocked(prepareHeadlessUniversalStores).mockReturnValue(channel); - vi.mocked(experimental_loadStorybook).mockResolvedValue({} as never); - await bootstrapToolsRuntime( { cwd: process.cwd(), configDir: '.storybook' }, { hostModuleGraph: false } ); expect(experimental_loadStorybook).toHaveBeenCalledWith(expect.objectContaining({ channel })); }); });Also applies to: 26-28
🤖 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/cli/tools/bootstrap.test.ts` around lines 14 - 18, Move the channel fixture and the mockReturnValue/mockResolvedValue setups from the affected test into the existing beforeEach block, keeping the mocks reset and configured before each test. Declare channel in the surrounding scope so the test assertion can reference it without defining mock behavior inline.Source: Coding guidelines
🤖 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/vitest/src/node/test-run-responder.ts`:
- Around line 231-272: Add a module-level in-flight request marker around the
run-start handling to prevent concurrent handlers from both passing the
startedAt guard; reject subsequent requests as already running, set the marker
before dispatching TRIGGER_RUN, and clear it in each terminal
TEST_RUN_COMPLETED, FATAL_ERROR, and CANCEL_RUN branch alongside unsubscribe().
- Around line 99-113: Update the TRIGGER_RUN subscription’s runTestRunner
invocation to consume rejected promises by attaching a rejection handler, while
avoiding any additional FATAL_ERROR dispatch because bootTestRunner already
reports startup failures. Preserve the existing runner arguments and state-reset
behavior.
In `@code/core/src/core-server/load.ts`:
- Around line 41-43: Update the returned Options object in the load flow to use
the resolved channel variable declared as channel, rather than preserving
options.channel. Ensure preset callbacks and callers receive this initialized
channel when no caller-supplied channel exists.
---
Nitpick comments:
In `@code/addons/vitest/src/node/test-run-responder.test.ts`:
- Around line 15-46: Update all four vi.mock calls in the test setup to use spy
mode, preserving the real module shapes instead of supplying plain factory
replacements. Move the mock behaviors for experimental_UniversalStore.create,
experimental_getTestProviderStore, createFileSystemCache,
loadPreviewOrConfigFile, runTestRunner, and log into beforeEach blocks using
vi.mocked(), while retaining the existing per-test behavior.
In `@code/addons/vitest/src/node/test-run-responder.ts`:
- Around line 42-52: Update ensureTestRunnerStore so a rejected
createTestRunnerStore promise clears storePromise before propagating the setup
error, allowing subsequent requests to retry while preserving successful store
memoization for the responder and dev server.
In `@code/core/src/cli/tools/bootstrap.test.ts`:
- Around line 14-18: Move the channel fixture and the
mockReturnValue/mockResolvedValue setups from the affected test into the
existing beforeEach block, keeping the mocks reset and configured before each
test. Declare channel in the surrounding scope so the test assertion can
reference it without defining mock behavior inline.
🪄 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: 0fb9297a-0f7e-47dc-8693-af375e46e8e3
📒 Files selected for processing (12)
AGENTS.mdcode/addons/vitest/src/node/test-run-responder.test.tscode/addons/vitest/src/node/test-run-responder.tscode/addons/vitest/src/preset.test.tscode/addons/vitest/src/preset.tscode/core/src/cli/tools/bootstrap.test.tscode/core/src/cli/tools/bootstrap.tscode/core/src/cli/tools/register.tscode/core/src/cli/tools/run.test.tscode/core/src/core-server/load.tscode/core/src/core-server/utils/get-server-channel.tscode/core/src/shared/open-service/toolsets/test/definition.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- code/core/src/core-server/utils/get-server-channel.ts
- AGENTS.md
- code/core/src/cli/tools/bootstrap.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/addons/vitest/src/node/test-run-responder.test.ts (1)
240-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the preset mock behavior into
beforeEach.Line 240 installs a mock implementation inside the test case. Configure this implementation in
beforeEach, then let the test select the failure state. This keeps mock setup consistent and isolated.As per coding guidelines, “Implement mock behaviors in
beforeEachblocks 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/vitest/src/node/test-run-responder.test.ts` around lines 240 - 246, Move the options.presets.apply mockImplementation setup from the individual test into its surrounding beforeEach block, while preserving the storyIndexGenerator failure behavior. Let each test configure failSetup to select whether the mocked setup throws, and keep the mock reset and defaultApply behavior isolated per test.Source: Coding guidelines
🤖 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/addons/vitest/src/node/test-run-responder.test.ts`:
- Around line 240-246: Move the options.presets.apply mockImplementation setup
from the individual test into its surrounding beforeEach block, while preserving
the storyIndexGenerator failure behavior. Let each test configure failSetup to
select whether the mocked setup throws, and keep the mock reset and defaultApply
behavior isolated per test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93bf9ee0-44d6-4be1-9a3e-201862795e0d
📒 Files selected for processing (3)
code/addons/vitest/src/node/test-run-responder.test.tscode/addons/vitest/src/node/test-run-responder.tscode/core/src/core-server/load.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- code/addons/vitest/src/node/test-run-responder.ts
- code/core/src/core-server/load.ts
2 of 9 tasks
kasperpeulen
changed the title
storybook tools command derived at runtime from the OSA toolsetsstorybook tools command derived at runtime from the OSA toolsets
kasperpeulen
force-pushed
the
m4-addon-mcp-core-services
branch
from
August 4, 2026 11:54
b1a8dee to
28d2853
Compare
5 of 9 tasks
kasperpeulen
force-pushed
the
m5a-tools-cli-osa
branch
from
August 4, 2026 12:20
ec57872 to
f2b1ef4
Compare
Closed
2 of 9 tasks
JReinhold
approved these changes
Aug 9, 2026
JReinhold
force-pushed
the
m5a-tools-cli-osa
branch
from
August 10, 2026 11:23
f2b1ef4 to
8f076a3
Compare
JReinhold
force-pushed
the
m4-addon-mcp-core-services
branch
from
August 10, 2026 11:23
28d2853 to
1da8a9f
Compare
…server plumbing What the rest of the branch builds on, all of it small and declarative: - `ToolsetMethod` gains the optional `requiresDevServer` trait; `stories.preview` and `review.create` declare it (with their rationale), `test.run` explicitly does not. The CLI derives its one uniform dev-server contract from this trait alone. - `toolset-names.ts` exports `toCliMethodName` — the single authority for how a method is spelled on the CLI — and `getRef` renders CLI cross-references through it, so command names and description prose cannot disagree. - The `tools-command` telemetry event type, modeled on `ai-command`. - Core-server plumbing the CLI's own-process bootstrap needs: `experimental_loadStorybook` honors a caller-supplied channel, and `prepareHeadlessUniversalStores` returns the channel it prepared the UniversalStore singleton on. One bus from store preparation to presets is load-bearing for local test runs (review 4/6).
builder-vite implements its already-declared `changeDetectionAdapter` hook for the server-less case: the same `commonConfig` + `viteFinal` assembly the dev server uses, resolved with Vite's server-less `resolveConfig`, and a no-op watcher. This is what lets the CLI host the module graph in its own process, so `stories changed` and `stories find-by-component` work with no dev server on Vite projects. Non-Vite builders keep failing with the existing typed `OpenServiceModuleGraphUnavailableError`. The test pins the same three resolve-config fields the server-bound adapter is pinned on, so the two assemblies cannot drift apart silently.
The public, unflagged CLI, running the OSA core toolsets in its own process,
fully disconnected from any dev server:
- `run.ts` is the whole command behind the commander wiring: it derives the
surface from `getRegisteredToolsets()` at runtime, parses `--key value`
flags (JSON-coerced, `--input` escape hatch, `-o/--output`), validates
against the method's schema, and maps `ToolsetOutcome` mechanically —
markdown to stdout, `--json` prints `data`, `ok` drives the exit code,
agent-facing errors surface verbatim. The `requiresDevServer` trait renders
as one uniform contract; `stories preview` resolves its origin from the
runtime instance registry.
- `bootstrap.ts` loads the target Storybook configuration (registering every
service and toolset via the `services` hook) and hosts the module graph for
the graph-bound methods by repeating the dev server's change-detection
bootstrap against the review-2/6 adapter.
- `help.ts` renders commander's conventional shape — Usage, Options, a
Commands listing with per-command summaries and execution badges — followed
by the full per-tool reference derived from live descriptions and schemas
(valibot to JSON Schema, reusing the ai CLI's schema-line renderer, exported
rather than copied).
- `register.ts` + `bin/` wire `tools` into the program and dispatcher, with
one `tools-command` telemetry event per invocation and an explicit exit once
the result is printed (the vitest child's IPC pipe would otherwise hold the
event loop open forever).
The primary test seam is the run function: tokens in, `{ exitCode, output }`
out, against the real core toolsets with stubbed dependencies.
…vices hook `storybook tools test run` runs story tests end to end with no Storybook running, so `test.run` carries no `requiresDevServer` trait and shows as `[local]` in help. The only dev-server-bound piece was the responder answering `TRIGGER_TEST_RUN_REQUEST` over the channel, which lived exclusively in addon-vitest's `experimental_serverChannel` hook. It moves to `node/test-run-responder.ts` and is wired from the same `services` hook that registers the `test` toolset, behind the same gate — every consumer that can offer the tool can also answer it. The heavy machinery (fs cache, leader UniversalStore seeded with the story index, vitest child boot) is created lazily on first request; the dev server's channel hook runs the same memoized setup eagerly and keeps its dev-only extras, so dev behavior is unchanged. Gates: skipped inside the vitest child process (a run can never recursively boot another child); non-Vite builders get an immediate error response instead of an unanswered request. Includes the AGENTS.md section documenting the CLI-and-toolsets end state.
…e tools CLI
The tools CLI reached into `cli/ai/mcp/` for instance discovery, config-dir
resolution and the schema help renderer, so the surviving code lived inside
the tree that is slated for deletion. Relocate everything both CLIs need into
`cli/tools/` and have `storybook ai` import it from there, so removing the ai
CLI later is a pure deletion of its directory plus its registration lines.
ai/mcp/registry.ts -> tools/instances/registry.ts
ai/mcp/resolve-instance.ts -> tools/instances/resolve.ts
ai/mcp/types.ts -> split: instance records to tools/instances/types.ts,
MCP wire shapes into tools/mcp-client.ts,
JSON Schema nodes to tools/schema-lines.ts
ai/mcp/client.ts -> tools/mcp-client.ts
resolveStorybookConfigDir -> tools/config-dir.ts
schemaLines -> tools/schema-lines.ts
CommandFailureHandler -> tools/register.ts
Tests move with their modules. Behavior-neutral: no runtime change on either
CLI, and no `cli/tools` -> `cli/ai` import remains.
…er's MCP endpoint `review create` needs the running dev server's review state, which the tools CLI has no way to reach until connect mode (Milestone 5b). Until then it was the one tool in the surface that could not work at all. Forward it to that Storybook's `@storybook/addon-mcp` endpoint instead: the dispatch discovers the instance, validates the arguments locally against the method schema, calls the frozen `display-review` tool, and unwraps the reply mechanically — text to stdout, `structuredContent` for `--json`, `isError` to the exit code. The handler still runs exactly once, in the dev server, so its telemetry and side effects stay in the process that owns them. A running Storybook without the addon gets its own message naming the addon to install, rather than the generic cannot-attach one; nothing is sent until the input validates. Deliberately disposable: 5b deletes `PROXY_VIA_MCP_METHODS`, its dispatch branch and the injectable dependency, so nothing outside that branch knows a method is proxied. One consequence worth knowing: the handler executes server-side, where `ctx.consumer` is `mcp`, so a proxied method renders its MCP prose variant even on the CLI. Descriptions and `--help` are unaffected — those render locally with the CLI consumer.
JReinhold
force-pushed
the
m5a-tools-cli-osa
branch
from
August 11, 2026 11:34
8f076a3 to
b537099
Compare
JReinhold
force-pushed
the
m4-addon-mcp-core-services
branch
from
August 11, 2026 11:34
1da8a9f to
0a5ad88
Compare
Base automatically changed from
m4-addon-mcp-core-services
to
m2b-core-toolsets
August 12, 2026 11:16
JReinhold
force-pushed
the
m2b-core-toolsets
branch
from
August 12, 2026 11:17
7c209cf to
a5ab56c
Compare
5 of 9 tasks
Contributor
|
Closing to restore stack base onto m4-addon-mcp-core-services after #35677 was accidentally marked merged by a merges-API probe. Replacement PR incoming; same head branch |
This was referenced Aug 12, 2026
1 of 6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #35716 · Closes #35724 · Closes #35747
Note
Stacked on #35677 (which stacks on #35726). This PR targets
m4-addon-mcp-core-services, so its diff is the CLI work only — 54 files, +3,384 −467. Review #35726 (the toolset layer) and #35677 (the MCP adapters) first; this PR is the third consumer of that same layer. Merge order: #35726 → #35677 → this one; GitHub retargets automatically.What I did
storybook tools— a public, unflagged CLI whose entire command surface is derived at runtime from the core toolsets, running in its own process, with no dev server. This is whatnpx storybook tools --helpprints in a react-vite sandbox:Not one of these commands is defined in the CLI. It loads the target project's Storybook configuration (
experimental_loadStorybook), which applies theservicespreset and registers every toolset in-process; commands, help text, schemas and summaries all come fromgetRegisteredToolsets()at runtime. Install addon-vitest andtest runappears; edit a description in a toolset and the help changes. The CLI's own job is mechanical:"Own process, no dev server" required solving three real problems, and each is its own commit:
stories changed,stories find-by-component): builder-vite implements its already-declaredchangeDetectionAdapterhook for the server-less case, and the CLI repeats the dev server's change-detection bootstrap against it.test run): the responder answering test-run requests moves out of addon-vitest's dev-server-only channel hook onto the sameserviceshook that registers thetesttoolset — every consumer that can offer the tool can now answer it.review create) genuinely needs the dev server's state. It is the one exception: the CLI forwards it to the running Storybook's@storybook/addon-mcpendpoint — the same mechanismstorybook aiuses — as an explicitly disposable stopgap until connect mode (Milestone 5b) exists.(
stories previewneeds no exception: it only needs a live origin, which the runtime instance registry provides.)Along the way, the modules both CLIs share (instance registry reader, instance matcher, MCP client, config-dir resolution, schema help renderer) move out of
cli/ai/intocli/tools/, becausestorybook aiis slated for removal and living code must not sit in a doomed tree. After this PR, deletingstorybook aiis a pure deletion of its directory plus two registration lines — and a lint rule fails any futurecli/tools→cli/aiimport.The one review question
Is the CLI a consumer with zero opinions — or does anything in
cli/tools/re-derive meaning that belongs in a toolset definition? If you find dispatch branching on domain data, help rendering prose a definition should own, or proxy-awareness leaking outside its one branch — that is the review finding to raise. Everything else should read as parsing, lookup, and mechanical mapping.How to review (~1½ hours)
The branch's six commits are six blocks, in dependency order — review one commit at a time; each heading links to that commit's diff.
storybook toolscommand — 40 minstorybook ai— 10 minreview createproxy — 10 minBlock 1 · The contract — 10 min
Files:
toolset-definition.ts,toolset-names.ts, the three definition annotations, telemetry types,core-server/load.ts(11 files, +76 −12)Everything the branch builds on, in ~90 lines: the optional
requiresDevServertrait onToolsetMethod(declared bystories.previewandreview.create; deliberately not bytest.run);toCliMethodNameas the single authority for how a method is spelled on the CLI (getRefrenders cross-references through it, so prose and dispatch cannot disagree); thetools-commandtelemetry event; andexperimental_loadStorybookhonoring a caller-supplied channel.What to check: is the trait on the right methods, and is one boolean enough to express "needs a dev server"? The channel parameter looks innocent but is load-bearing for block 4.
Block 2 · The headless module graph — 10 min
Files:
builder-vite/src/change-detection-adapter/headless.ts(+ test),builder-vite/src/index.ts(3 files, +129 −10)headless.tsis 38 lines: the samecommonConfig+viteFinalassembly the dev server uses, resolved with Vite's server-lessresolveConfig, and a no-op watcher. Non-Vite builders keep failing with the existing typed error.What to check: the test pins the same three resolved-config fields as the server-bound adapter, so the two assemblies cannot drift silently.
Block 3 · The
storybook toolscommand — 40 minFiles:
cli/tools/{run,help,bootstrap,register,tool-tokens,discover-instance}.ts+ tests +bin/wiring (15 files, +1,975 −10)The heart of the PR. Read
run.ts(355 lines — the whole command behind the commander wiring) andhelp.ts(209 lines) in full.run.test.tsdoubles as the behavior spec: tokens in,{ exitCode, output }out, against the real core toolsets with stubbed dependencies — including a parity test assertingdocs listprints byte-for-byte what an MCP client receives for the same call.Two deliberate oddities, so they don't read as bugs:
What to check: the one review question, applied line by line. Telemetry: one
tools-commandevent per invocation,helplookups excluded so they cannot skew success rates.Block 4 · Local story tests — 15 min
Files:
addons/vitest/src/node/test-run-responder.ts(+ test),preset.ts(+ test),AGENTS.md(5 files, +637 −185)The responder was the only dev-server-bound piece of
test run. It moves tonode/test-run-responder.ts, wired from the sameserviceshook that registers thetesttoolset, behind the same gate. Heavy machinery (fs cache, leader UniversalStore, vitest child) is created lazily on first request; the dev server's channel hook runs the same memoized setup eagerly and keeps its dev-only extras, so dev behavior is unchanged.What to check: the gates — skipped inside the vitest child (a run can never recursively boot another child); non-Vite builders get an immediate error response instead of an unanswered request. And the channel invariant: the responder relays the vitest child's store events onto the channel its preset hook received — leader stores only hear the channel they were prepared on, so a second channel on either side hangs the run silently (that is what block 1's channel parameter prevents).
Block 5 · The relocation out of
storybook ai— 10 minFiles:
cli/tools/{instances/,mcp-client,config-dir,schema-lines}.ts+ moved tests, thecli/ai/import rewires, the lint rule (27 files, +392 −265)Behavior-neutral by construction:
git mvplus import rewrites, no logic changes.cli/ai/mcp/types.tsis deleted, split across the new homes;schema-lines.tsgains the unit tests it never had (its only coverage used to live in the ai CLI's suite — the tree slated for deletion).What to check: skim for a logic change hiding among the moves (there are none); the new
no-restricted-importsrule incode/.oxlintrc.jsonthat fails anycli/tools→cli/aiimport — dependency direction is dying-code-depends-on-living-code, never the reverse.Block 6 · The
review createproxy — 10 minFiles:
run.ts,mcp-client.ts,run.test.ts,AGENTS.md(4 files, +193 −3)One self-contained branch in the dispatch:
The handler runs exactly once, in the dev server, so its telemetry and side effects stay in the process that owns them. Milestone 5b deletes the set, the branch, and the injectable dependency wholesale.
What to check: disposability — nothing outside the branch may know a method is proxied. Five seam tests pin the contract (frozen name + args,
--json,isError→ exit 1, validation before any send, missing-endpoint guidance).Known limitation, on purpose: the proxied handler executes server-side where
ctx.consumerismcp, soreview createprints its MCP prose variant on the CLI (descriptions and--helprender locally and are unaffected). Fixing it means mapping the header to acliconsumer in addon-mcp, which would also change whatstorybook aiand the Claude/Codex plugins receive — a separate call, and 5b removes the mismatch entirely. Noted on #35747.What you do not need to review, and why that is safe
docs listparity test asserts byte-equality with the handler markdown the MCP adapter renders verbatimtest run247/247 matching the manager UI,review createpublishing end-to-end with the review page visually verified (including the pending-review → Update flow), unknown-ID refusal, missing-addon guidance, no-dev-server contractstorybook aiis unchangedai display-review,ai list-all-documentation) behind its env flagOut of scope (per #35747): connect mode (5b), deprecating
storybook ai(a later PR — this PR only makes that deletion pure), proxying any method beyondreview.create, mapping the trusted-local-client header to acliconsumer.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn nx run-many -t compile, then in any project (a sandbox works):npx storybook tools— the Commands listing and full tool reference print from that project's configuration, with[local]/[requires running Storybook]badges.npx storybook tools docs listandnpx storybook tools stories changed— both work with no dev server (docs from manifests, the module graph hosted in-process).npx storybook tools test run --stories '[{"storyId":"example-button--primary"}]'— addon-vitest boots its vitest child, the report prints, the process exits with the mapped code.storybook dev(with@storybook/addon-mcp):npx storybook tools review create --input '<review object>'publishes the review; the page renders it at/?path=/review/;--jsonprints{ "reviewUrl": … }. Without the addon, the command names it and exits 1. Without a dev server, the uniform "start the dev server first" message.STORYBOOK_FEATURE_AI_CLI=true npx storybook ai list-all-documentation— the ai CLI still works identically on the relocated modules.Documentation
--helpis the CLI's documentation until the surface stabilizes, per the spec's out-of-scope list)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.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>