Repository navigation
Core: Define shared public toolsets (defineToolset) - #35516
Merged
Merged
Conversation
ghengeveld
added
feature request
ci:normal
ghengeveld
changed the title
Expose selected open services through the core CLI while enforcing unambiguous operation names at registration.
Avoid exposing low-level docgen services before a capability-level CLI API is available.
ghengeveld
force-pushed
the
osa-generated-tools-cli
branch
from
July 21, 2026 08:50
93420ec to
64f09e8
Compare
Introduce core/docs, core/stories, core/test, and core/review definitions with transport-neutral schemas, plus docs composition over docgen/story-docs/mdx.
Wire the docs capability beside the MDX service so list/show/showStory can compose live docgen, story-docs, and MDX data in the dev server.
Implement preview, changed, and findByComponent handlers with injected index/origin/status/module-graph deps. Unreachable-file detection still stubbed.
Queue channel-driven runs through addon-vitest's trigger API and register from the vitest server channel preset.
Validate story ids against the live index, emit PUSH_REVIEW, and return a review URL from the common-preset channel registration.
ghengeveld
changed the title
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.22 MB | 21.39 MB | 🚨 +161 KB 🚨 |
| Dependency size | 36.75 MB | 36.75 MB | 🚨 +3 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 205 | 205 | 0 |
| Self size | 827 KB | 827 KB | 0 B |
| Dependency size | 91.44 MB | 91.60 MB | 🚨 +164 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 | 89.92 MB | 90.08 MB | 🚨 +164 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 74 | 74 | 0 |
| Self size | 1.09 MB | 1.09 MB | 0 B |
| Dependency size | 57.97 MB | 58.14 MB | 🚨 +164 KB 🚨 |
| Bundle Size Analyzer | node | node |
ghengeveld
changed the title
Rename preview.ts so core-service-types membership no longer treats core/stories as a preview-runtime service.
Compare STORYBOOK_BUILDER to the 'vite' string literal so CI's package check does not fail on dual SupportedBuilder enum identities.
ghengeveld
marked this pull request as ready for review
July 21, 2026 12:32
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:
WalkthroughChangesDefine API capabilities
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
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: 2
🧹 Nitpick comments (4)
code/core/src/core-server/presets/common-preset.ts (2)
380-381: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the "ponytail:" prefix from the comment.
The rationale itself is useful (deferred pending git/working-tree wiring), but the leading "ponytail:" tag reads like a leftover codename/marker rather than maintenance-relevant context.
✏️ Proposed fix
- // ponytail: full detectUnreachableChanges port deferred — empty until git/working-tree wiring lands + // full detectUnreachableChanges port deferred — empty until git/working-tree wiring lands detectUnreachableFiles: async () => [],As per coding guidelines, "Comments should explain maintenance-relevant rationale, not investigation transcripts, ticket or acceptance-criteria codes, provenance claims, or cross-file line references."
🤖 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 380 - 381, Remove the “ponytail:” prefix from the comment above detectUnreachableFiles, preserving the remaining maintenance rationale about deferring the full detectUnreachableChanges port until git/working-tree wiring is available.Source: Coding guidelines
386-391: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the swallowed module-graph query failure.
On
moduleGraph.queries.storiesForFiles.loaded(...)throwing, this silently falls back to "no matches for any path," which is indistinguishable from a legitimate empty result. A log via the node-side Storybook logger would help diagnose module-graph failures without changing behavior.🪵 Proposed fix
+import { logger } from 'storybook/internal/node-logger'; ... try { const moduleGraph = getService('core/module-graph'); storiesForFiles = await moduleGraph.queries.storiesForFiles.loaded({ files: paths }); } catch (err) { + logger.warn(`core/stories: findByComponent module-graph query failed: ${err}`); return paths.map((componentPath) => ({ componentPath, matches: [] })); }As per coding guidelines, "Use Storybook loggers instead of raw console calls in normal code paths."
🤖 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 386 - 391, Update the catch around moduleGraph.queries.storiesForFiles.loaded in the relevant preset flow to accept the thrown error and log it through the node-side Storybook logger before returning the existing empty matches fallback. Preserve the current fallback behavior and avoid raw console calls.Source: Coding guidelines
code/addons/docs/src/docs-service/server.ts (1)
1-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
RegisterDocsServiceOptionstype — see consolidated comment.This type is redeclared identically from the core service's exported type; see the consolidated comment for the suggested fix.
🤖 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/docs/src/docs-service/server.ts` around lines 1 - 12, Remove the duplicate RegisterDocsServiceOptions declaration from registerDocsService and reuse the identical exported options type from the core docs service module. Update the import and function signature so registerDocsService continues accepting the same getIndex contract without maintaining a second type definition.code/core/src/shared/open-service/services/test/run.test.ts (1)
15-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize typed mock setup in
beforeEach.These tests directly replace or configure mocks inside test cases, bypassing
vi.mocked()and risking leaked global spies when assertions fail.
code/core/src/shared/open-service/services/test/run.test.ts#L15-L35: exposeemitas a Vitest mock from the channel factory.code/core/src/shared/open-service/services/test/run.test.ts#L97-L97: access the mock throughvi.mocked().code/core/src/shared/open-service/services/test/run.test.ts#L111-L122: move the completed-response mock implementation intobeforeEach.code/core/src/shared/open-service/services/test/run.test.ts#L135-L146: move the error-response mock implementation intobeforeEach.code/core/src/shared/open-service/services/test/run.test.ts#L161-L166: move the cancelled-response mock implementation intobeforeEach.code/core/src/shared/open-service/services/test/run.test.ts#L178-L190: move the request-ID response mock implementation intobeforeEach.code/core/src/shared/open-service/services/stories/find-story-ids.test.ts#L138-L152: set up and restore theprocess.cwdspy throughbeforeEach/afterEach.As per coding guidelines, “Use
vi.mocked()to type and access the mocked functions,” “Implement mock behaviors inbeforeEachblocks,” and “Avoid inline mock implementations within test cases.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/core/src/shared/open-service/services/test/run.test.ts` around lines 15 - 35, Centralize mock configuration in beforeEach and ensure cleanup after each test. In code/core/src/shared/open-service/services/test/run.test.ts:15-35, expose createMockChannel().emit as a Vitest mock; at 97-97 access it via vi.mocked(); and move the completed, error, cancelled, and request-ID response implementations at 111-122, 135-146, 161-166, and 178-190 into beforeEach blocks, avoiding inline test-case implementations. In code/core/src/shared/open-service/services/stories/find-story-ids.test.ts:138-152, configure and restore the process.cwd spy through beforeEach/afterEach.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/core/src/shared/open-service/services/docs/runtime.ts`:
- Around line 9-10: Move lastClassification out of module-level state and scope
it to each docs request, preferably by storing it on the service instance or
passing it through the query context. Update the related DocsRuntime
classification and read paths so overlapping core/docs loads cannot overwrite
one another, while preserving the existing classification behavior.
In `@code/core/src/shared/open-service/services/test/run.ts`:
- Around line 75-120: Update the Promise flow around settle and handleResponse
to add a configurable timeout or cancellation path for requests that receive no
matching response. Start the timer when the listener is registered, reject with
an appropriate timeout error when it expires, and ensure settle cleanup removes
the response listener and clears the timer so later test.run requests are not
blocked.
---
Nitpick comments:
In `@code/addons/docs/src/docs-service/server.ts`:
- Around line 1-12: Remove the duplicate RegisterDocsServiceOptions declaration
from registerDocsService and reuse the identical exported options type from the
core docs service module. Update the import and function signature so
registerDocsService continues accepting the same getIndex contract without
maintaining a second type definition.
In `@code/core/src/core-server/presets/common-preset.ts`:
- Around line 380-381: Remove the “ponytail:” prefix from the comment above
detectUnreachableFiles, preserving the remaining maintenance rationale about
deferring the full detectUnreachableChanges port until git/working-tree wiring
is available.
- Around line 386-391: Update the catch around
moduleGraph.queries.storiesForFiles.loaded in the relevant preset flow to accept
the thrown error and log it through the node-side Storybook logger before
returning the existing empty matches fallback. Preserve the current fallback
behavior and avoid raw console calls.
In `@code/core/src/shared/open-service/services/test/run.test.ts`:
- Around line 15-35: Centralize mock configuration in beforeEach and ensure
cleanup after each test. In
code/core/src/shared/open-service/services/test/run.test.ts:15-35, expose
createMockChannel().emit as a Vitest mock; at 97-97 access it via vi.mocked();
and move the completed, error, cancelled, and request-ID response
implementations at 111-122, 135-146, 161-166, and 178-190 into beforeEach
blocks, avoiding inline test-case implementations. In
code/core/src/shared/open-service/services/stories/find-story-ids.test.ts:138-152,
configure and restore the process.cwd spy through beforeEach/afterEach.
🪄 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: da1d6c3d-6b95-4f22-a0a7-f4e6a1e5bf3d
📒 Files selected for processing (38)
AGENTS.mdcode/addons/docs/src/docs-service/server.tscode/addons/docs/src/preset.tscode/addons/vitest/src/manager.tsxcode/addons/vitest/src/preset.tscode/addons/vitest/src/typings.d.tscode/core/src/cli/tools/generate-cli.test.tscode/core/src/cli/tools/generate-cli.tscode/core/src/core-server/presets/common-preset.tscode/core/src/server-errors.tscode/core/src/shared/open-service/core-service-types.tscode/core/src/shared/open-service/service-registration.test.tscode/core/src/shared/open-service/service-registry.tscode/core/src/shared/open-service/services/capability-services.test.tscode/core/src/shared/open-service/services/docs/classify-index.tscode/core/src/shared/open-service/services/docs/definition.tscode/core/src/shared/open-service/services/docs/map.test.tscode/core/src/shared/open-service/services/docs/map.tscode/core/src/shared/open-service/services/docs/runtime.tscode/core/src/shared/open-service/services/docs/server.tscode/core/src/shared/open-service/services/review/definition.tscode/core/src/shared/open-service/services/review/server.test.tscode/core/src/shared/open-service/services/review/server.tscode/core/src/shared/open-service/services/stories/changed.test.tscode/core/src/shared/open-service/services/stories/changed.tscode/core/src/shared/open-service/services/stories/definition.tscode/core/src/shared/open-service/services/stories/find-by-component.test.tscode/core/src/shared/open-service/services/stories/find-by-component.tscode/core/src/shared/open-service/services/stories/find-story-ids.test.tscode/core/src/shared/open-service/services/stories/find-story-ids.tscode/core/src/shared/open-service/services/stories/preview-stories.test.tscode/core/src/shared/open-service/services/stories/preview-stories.tscode/core/src/shared/open-service/services/stories/server.tscode/core/src/shared/open-service/services/stories/story-input.tscode/core/src/shared/open-service/services/test/definition.tscode/core/src/shared/open-service/services/test/run.test.tscode/core/src/shared/open-service/services/test/run.tscode/core/src/shared/open-service/services/test/server.ts
Concurrent docs loads could overwrite each other's index classification; key serializable state by operation+input so handlers stay race-safe.
A missing addon-vitest response could hang forever and block the serial test.run queue; fail after a configurable timeout and clean up listeners.
Closed
…elete the review store Implements #35659: service-derived values become core/review queries (flattenedEntries, bannerKind), the manager module store is deleted in favor of a ReviewContext provider mounted above the Layout, actions receive their inputs as arguments, and dismissal propagates as service state so each tab returns to its own pre-review story. The review events file shrinks to PAGEVIEW plus the Milestone-4-marked PUSH_REVIEW.
…ers, setReview wording
The manager re-projects the active review on every mount, so comparing the incoming createdAt against an in-memory ref treated an ordinary page reload as a new review and cleared the one-time latch. A reviewer who had explicitly left the review was then auto-entered again on their next visit to the summary. Storing the review's createdAt in the latch makes it survive reloads while a genuinely newer review still auto-enters once.
Contributor
|
Failed to publish canary version of this pull request, triggered by @kasperpeulen. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/30554101233 |
…ng components with absence - core/review setReview now requires an own index entry of type 'story'. A docs entry previously passed validation and produced review slots whose navigation and previews could not resolve. - The docs toolset translates the typed missing-component error into absence, so standalone MDX ids reach the branch that formats them and unknown ids return the documented not-found result instead of throwing. The test doubles now throw like the real extraction service, which is what hid this. - Exhaustiveness assertions on the two test-run response switches, whose default branches return a value and so could not catch a new status at compile time. format.ts needs none: its string return type already fails the build (verified). - Drop the unused changeDetectionTypeId parameter; no caller supplies it. - Share one attached-MDX mapper between the Markdown and JSON docs paths.
…opped review The review service now refuses non-story entries, and the channel adapter only warns on failure — so without the matching pre-check the tool would report success while the review silently never landed. Both messages now name the docs case.
kasperpeulen
approved these changes
Jul 30, 2026
2 tasks done
This was referenced Jul 30, 2026
This was referenced Aug 3, 2026
This was referenced Aug 19, 2026
This was referenced Aug 24, 2026
JReinhold
added
qa:skip
Contributor
LogDetails |
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
5 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.
Summary
Brings Milestone 2 of #35526 to the agreed end-state per the finishing spec in #35656:
storybook/open-servicehosts two sibling constructs: services (defineService/registerService, internal state sync) and toolsets (defineToolset/registerToolset, the public agent surface). The separatestorybook/public-apientry is removed; the docs/stories/test/review toolsets live underopen-service/toolsets/, mirroringopen-service/services/.servicespreset hook. Theexperimental_toolsetspreset property is deleted. Core registers the docs toolset, and the review toolset registers inside the same feature-gated branch as the review service. Adapters read the registry viagetRegisteredToolsets()— it is adapter-facing and consumed only from Milestone 4 (MCP) / Milestone 5 (CLI) on. Double application of theserviceshook throws a typedOpenServiceServicesAppliedTwiceError.ctx.formatreplaces the per-methodjsonflag.ToolsetCtxgains a requiredformat: 'markdown' | 'json'andoriginbecomes optional. Thejsonproperty is gone from all eight method schemas; adapters own the format mapping (CLI--json, MCPjsontool input).@storybook/mcpmanifest formatter (toolsets/docs/manifest-formatter/: props section, 3-story cap with remaining-story references, subcomponents, docs-manifest formatting), assembling its input the same way addon-mcp's in-processresolveEntrydoes. The formatter's tests, fixture, and file snapshots are ported as the parity suite; the previous divergent formatter is deleted.core/reviewstate is{ current, pending }: publishing while a review is current defers the update topending;acceptPendingpromotes it; dismissal clears both; staleness (including the grace window) is enforced in the service, which subscribes to the module-graph revision at registration. The legacy channel keeps one thin adapter (applyPUSH_REVIEW), marked for Milestone 4 deletion.core/reviewqueries (flattenedEntries, andbannerKindwith pending-update outranking stale). The manager's module store (review-store.ts) is deleted in favor of aReviewContextprovider mounted above the Layout: it owns the session-backed per-tab review-mode flag (the only storage key that drives render) and hands components state plus bound actions (openSummary/openEntry/leaveReview/dismiss); the underlying actions receive their inputs as arguments, with no store reads. Dismissal now propagates as service state (current → null) and each tab reacts locally with its own stored return URL — fixing the bug where dismissing in one tab navigated other tabs to the dismissing tab's story; tabs that never entered the review stay put. The review events file shrinks toPAGEVIEWplus the Milestone-4-markedPUSH_REVIEW. One deliberate deviation from the spec's wording:isExitingis a ref rather thanuseState— nothing renders from it, and the auto-enter guard needs a fresh mid-flight read that a non-dependency state value wouldn't give.Closes #35656. Closes #35659. Tracking: #35526.
Follow-ups (not this PR)
testtoolset toaddon-vitestas OSA + preset contributionstoriestoolset when its boot-time deps (git, index, statuses) are wiredgetRegisteredToolsets(); Milestone 5:storybook toolsCLITest plan
@storybook/mcp)ctx.formatvariants across docs/stories/test/reviewyarn nx run-many -t checkgreen across all 44 projectscore/reviewquery tests (flattenedEntries,bannerKind) in the existing server-test pattern