Docgen: Add mock open service#34954
Open
JReinhold wants to merge 13 commits into
Open
Conversation
First concrete open-service consumer: a per-component docgen service backed by an `experimental_docgen` middleware-style preset. Renderers and addons can register extractors that wrap the previous accumulated extractor and merge results. Phase 1 ships the service definition, registration, static-snapshot wiring under `docgen/<componentId>.json`, and a mock extractor in the React renderer. Phase 2 will teach RCM per-component extraction; phase 3 replaces the mock with the real RCM-backed extractor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rovider Rename DocgenExtractor → DocgenProvider (and matching field/variable names) to better reflect that registrants supply data rather than extract it from a single source. Adds a second docgen provider in addon-docs that enriches description and appends a synthetic prop. With the React mock provider also registered, this exercises the middleware-merge path end-to-end across two packages, validating that the chosen preset chaining pattern composes correctly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…er; drop per-provider identity fallback The preset slot, type re-exports, Presets.apply overload, and every preset file's named export now use `experimental_docgenProvider` — keeps the preset name consistent with the DocgenProvider type. Each provider file no longer carries its own identity-provider seed. Providers call `nextDocgen?.(input)` and treat the downstream payload as optional, falling back to literals where needed. Core still seeds the chain with an identity provider so the composed result is always a defined function, but individual provider files no longer need to know about that detail. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
JReinhold
changed the base branch from
next
to
jeppe/service-registration-attempt-2
JReinhold
changed the base branch from
jeppe/service-registration-attempt-2
to
claude/practical-jones-e3d0c1
JReinhold
changed the title
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 72 | 72 | 0 |
| Self size | 20.39 MB | 20.41 MB | 🚨 +14 KB 🚨 |
| Dependency size | 36.11 MB | 36.11 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 203 | 203 | 0 |
| Self size | 908 KB | 908 KB | 🚨 +144 B 🚨 |
| Dependency size | 88.58 MB | 88.59 MB | 🚨 +15 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 196 | 0 |
| Self size | 32 KB | 32 KB | 🎉 -36 B 🎉 |
| Dependency size | 87.07 MB | 87.08 MB | 🚨 +14 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 1.08 MB | 1.08 MB | 🚨 +66 B 🚨 |
| Dependency size | 56.50 MB | 56.52 MB | 🚨 +14 KB 🚨 |
| Bundle Size Analyzer | node | node |
The open-service registration API recently moved from a single
`static: { path, inputs }` block to a definition-time `filePath`
plus a registration-time `staticInputs`. Snapshot paths now also
auto-prefix by service id, so the docgen `filePath` only needs to
produce `<componentId>.json` (the runtime turns it into
`core/docgen/<componentId>.json`).
Updates server.ts, the definition, and the matching static-build
test to the new shape. Phase-1 tests all pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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:
📝 WalkthroughWalkthroughAdds an experimental core docgen open service: provider middleware types/schemas, server registration and handler wiring, core services preset registration, docs-addon and React renderer provider presets, componentId utility centralization, tests, and conditional static-build export. ChangesExperimental Docgen Provider System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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: 1
🧹 Nitpick comments (1)
code/addons/docs/src/preset.ts (1)
228-228: ⚡ Quick winUse explicit extension in the new re-export.
Line 228 should use an explicit
.tsextension to stay consistent with the TS module import/export guideline.Proposed fix
-export { experimental_docgenProvider } from './docgen'; +export { experimental_docgenProvider } from './docgen.ts';As per coding guidelines "For TypeScript source code in the repo, prefer explicit file extensions for relative code imports and exports such as ./foo.ts or ./bar.tsx when the target is another TS/JS module".
🤖 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/preset.ts` at line 228, The re-export for experimental_docgenProvider uses a bare specifier; update the export in preset.ts so the relative module path includes the explicit .ts extension (export { experimental_docgenProvider } from './docgen.ts') to comply with the TypeScript module import/export guideline and keep consistency with other TS source files.
🤖 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/core-server/presets/common-preset.ts`:
- Around line 332-351: The services preset sets
globalThis.STORYBOOK_SERVICES_LOADED before awaiting async work in services; if
any await (e.g., options.presets.apply('storyIndexGenerator') or
apply('experimental_docgenProvider')) throws, the flag remains true and prevents
retries—wrap the async initialization in a try/catch/finally within services and
in the finally reset globalThis.STORYBOOK_SERVICES_LOADED to false when
initialization failed (only keep it true on successful completion) so that
failed initialization does not block subsequent attempts to apply services;
ensure the logic around generator, provider, and registerDocgenService is inside
the try and the flag-reset happens on error.
---
Nitpick comments:
In `@code/addons/docs/src/preset.ts`:
- Line 228: The re-export for experimental_docgenProvider uses a bare specifier;
update the export in preset.ts so the relative module path includes the explicit
.ts extension (export { experimental_docgenProvider } from './docgen.ts') to
comply with the TypeScript module import/export guideline and keep consistency
with other TS source files.
🪄 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: 9c9407d5-9dcf-40a6-bab4-8e74b38c61b9
📒 Files selected for processing (14)
code/addons/docs/src/docgen.tscode/addons/docs/src/preset.tscode/core/src/common/index.tscode/core/src/common/utils/component-id.tscode/core/src/core-server/presets/common-preset.tscode/core/src/server-errors.tscode/core/src/shared/open-service/services/docgen/definition.tscode/core/src/shared/open-service/services/docgen/server.test.tscode/core/src/shared/open-service/services/docgen/server.tscode/core/src/shared/open-service/services/docgen/types.tscode/core/src/types/modules/core-common.tscode/renderers/react/src/componentManifest/generator.tscode/renderers/react/src/docgen/preset.tscode/renderers/react/src/preset.ts
8 tasks
Matches the existing "Loading presets" / "Building manager.." / "Building preview.." CLI breadcrumbs so users can see when the open-service snapshot step is running. Only emits the log (and only schedules the work) when at least one service is registered, so installations without any service consumers stay quiet. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Review — multi-model pass (architecture · correctness/concurrency · DX · craft)Verdict: no blockers. Sound phase-1 scaffolding; the design (sync-read query + command-does-work + middleware preset chain + per-component static files) fits the open-service framework well. Recommend addressing the HIGH items below, or deferring them as tracked TODOs in the phase-2/3 plan. ✅ Verified fine (so they don't need re-checking)
🔴 HIGH
🟠 MEDIUM
🟡 LOW / NIT
Suggested "before merge" set
Strongly recommended (cheap, prevents phase-3 pain): H1 (cache guard + invalidation seam), M2 (entry filtering), M3/M5 (unify merge semantics). 🤖 Generated with Claude Code — synthesized from a Claude + Codex + Gemini + pragmatic-engineering review pass. |
…perimentalDocgenServer
Three changes to the phase-1 docgen surface:
1. DocgenProvider input is now { importPath } instead of { componentId, entries }.
The service resolves an entry for the requested componentId and hands the
raw importPath to the provider chain — providers that don't know how to
handle a given path (e.g. the React mock provider seeing an .mdx attached
docs file) bail to nextDocgen.
2. DocgenProvider can return undefined. The identity seed in core's services
preset now returns undefined so a chain with no real providers signals
"no docgen here" instead of producing an empty payload. The addon-docs
enricher mirrors this — it returns undefined when nothing downstream
produced a payload, rather than fabricating one.
3. The whole docgen-service registration is gated behind a new
`experimentalDocgenServer` feature flag (sibling of
experimentalReactComponentMeta). When the flag is off, no service is
registered, no preset chain is built, and the static-build log step
already correctly skips itself via the existing registered-services
length check.
Also picks up an upstream rename: query `filePath` → `staticPath`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The command's output schema was `void`, so callers had to follow up with a separate `getDocgen` read to see what was extracted. The work was already in scope — return the payload (or undefined) directly so a single extractDocgen call gives consumers the data alongside the state write. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
code/core/src/shared/open-service/services/docgen/server.test.ts (1)
145-176: ⚡ Quick winAdd a mixed-entry regression test for shared
componentIdresolution.Consider adding a case where one
componentIdhas both a story entry and an attached-docs entry with differentimportPathvalues. That will lock in selection semantics and prevent regressions around unsupported-path provider bailouts.🤖 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/docgen/server.test.ts` around lines 145 - 176, Add a regression test to registerDocgenService that includes a mixed index where one componentId appears twice with different importPaths (use makeStoryEntry for e.g. 'button--primary' with importPath './button.stories.tsx' and makeAttachedDocsEntry for an attached-docs entry that maps to the same componentId but a different importPath like './attached/button.docs.tsx'), keep the provider behavior the same (use importPath.includes('button') to pick componentId 'button'), call buildStaticFiles and assert that only one core/docgen/button.json is emitted and that its chosen entry matches the desired selection semantics (verify the description or importPath-derived field to confirm which importPath was used); reference registerDocgenService, makeGetIndex, makeStoryEntry, makeAttachedDocsEntry, provider, and buildStaticFiles when locating where to add the test.
🤖 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/docs/src/docgen.ts`:
- Around line 19-25: The current return in docgen.ts unconditionally replaces
empty descriptions and always appends a marker to props; change it to use
nullish coalescing for description (preserve downstream.description when it is
an empty string) and merge props with deduplication (only add the marker if
downstream.props does not already contain an object with source
'`@storybook/addon-docs`' and kind 'docs-marker'), or better yet extract and call
a shared mergeDocgen(base, patch) helper used by the React provider so both
providers use the same nullish-coalesce semantics and a single-place dedupe rule
for props.
In `@code/core/src/shared/open-service/services/docgen/server.test.ts`:
- Around line 33-38: Tests repeatedly recreate the provider mock and inspect
untyped mock internals; centralize the mock by declaring const provider =
vi.fn<DocgenProvider>(...) at top-level and move per-test return/error behavior
into a beforeEach that calls provider.mockImplementationOnce(...) or
provider.mockResolvedValueOnce(...); replace direct access like
provider.mock.calls[...] with typed vi.mocked(provider) when asserting
calls/returns (e.g., vi.mocked(provider).mock.calls / .mock.results) so callers
of DocgenProvider are consistently typed and per-test setups live in beforeEach.
In `@code/core/src/shared/open-service/services/docgen/server.ts`:
- Around line 51-53: Replace the single .find(...) lookup with a two-step
selection: collect all matches from Object.values(index.entries) where
getComponentIdFromEntry(e) === input.componentId, then prefer and return an
entry that represents a story (e.g., an entry whose type/marker indicates a
story — check entry.type or other story-specific field) and fall back to the
first matched entry if no story entry exists; update the code around the
variable entry and the lookup over index.entries/getComponentIdFromEntry to
implement this preference so story entries are chosen over attached-docs .mdx
entries.
In `@code/renderers/react/src/docgen/preset.ts`:
- Around line 24-28: The merge currently mixes || and ?? causing empty strings
for name/description to be treated differently than componentId/props; update
the merge in preset.ts so name and description use nullish coalescing like the
others (change downstream?.name || componentId to downstream?.name ??
componentId and downstream?.description || `Mocked docgen for
${input.importPath}` to downstream?.description ?? `Mocked docgen for
${input.importPath}`) to make behavior consistent for downstream, and as a
follow-up per reviewer M5 consider aligning with the docs addon provider by
making description be always overwritten by downstream?.description (use ??) and
changing props merge logic to append downstream props rather than replace
(adjust the props handling on the props field accordingly).
---
Nitpick comments:
In `@code/core/src/shared/open-service/services/docgen/server.test.ts`:
- Around line 145-176: Add a regression test to registerDocgenService that
includes a mixed index where one componentId appears twice with different
importPaths (use makeStoryEntry for e.g. 'button--primary' with importPath
'./button.stories.tsx' and makeAttachedDocsEntry for an attached-docs entry that
maps to the same componentId but a different importPath like
'./attached/button.docs.tsx'), keep the provider behavior the same (use
importPath.includes('button') to pick componentId 'button'), call
buildStaticFiles and assert that only one core/docgen/button.json is emitted and
that its chosen entry matches the desired selection semantics (verify the
description or importPath-derived field to confirm which importPath was used);
reference registerDocgenService, makeGetIndex, makeStoryEntry,
makeAttachedDocsEntry, provider, and buildStaticFiles when locating where to add
the test.
🪄 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: 367645ee-80d8-4cc7-867e-604bca144d12
📒 Files selected for processing (8)
code/addons/docs/src/docgen.tscode/core/src/core-server/presets/common-preset.tscode/core/src/shared/open-service/services/docgen/definition.tscode/core/src/shared/open-service/services/docgen/server.test.tscode/core/src/shared/open-service/services/docgen/server.tscode/core/src/shared/open-service/services/docgen/types.tscode/core/src/types/modules/core-common.tscode/renderers/react/src/docgen/preset.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- code/core/src/types/modules/core-common.ts
- code/core/src/core-server/presets/common-preset.ts
- code/core/src/shared/open-service/services/docgen/definition.ts
…ttern Captures a lesson from building the docgen service: when query.load and a command both look plausible for the same work, the work belongs in the command. The load body should be a one-line passthrough that calls it. Extends the Load concept section with the rationale (reusability, testability, clarity) and adds a matching bullet to the Design Rules. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four review nits, addressed together because they overlap: H2 — Docgen static build wasn't gated by ignorePreview, so a manager-only build forced full story-index generation (staticInputs calls getIndex()). Mirror the !options.ignorePreview gate around index.json / writeManifests at the registration boundary: don't register the service at all when previewing is off. L1 — `nextDocgen?.(input)` defended an impossible state (core always seeds the chain). New `defineDocgenProvider` helper from `storybook/internal/common` types `next` as required and throws a clear error if the seed is ever missing, instead of silently degrading to empty payloads. M3 + M5 — The two phase-1 mocks merged downstream inconsistently (React rebuilt the payload field-by-field; addon-docs used spread). A future provider adding a `DocgenPayload` field would be silently dropped by the React mock. Document the spread + `??` convention on the DocgenProvider type's JSDoc and on `defineDocgenProvider`, and update the React mock to match. The convention preserves both unknown fields and explicit downstream values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lets us dogfood the docgen service against real components in storybook:ui:build and storybook:ui dev runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
What I did
First concrete consumer of the open-service architecture: the
core/docgenservice exposes per-component documentation keyed by componentId. Renderers and addons contribute through anexperimental_docgenProviderpreset using middleware-style chaining (each provider wraps the previously accumulated one).This PR ships scaffolding + a mock React provider so the wiring is exercised end-to-end. Real RCM-backed extraction lands in #34963.
Feature-flagged
Everything is gated behind
experimentalDocgenServer(sibling ofexperimentalReactComponentMeta). When the flag is off:build-static.ts'sgetRegisteredServices().length > 0check correctly suppresses the "Building open services.." logThe internal Storybook (
code/.storybook/main.ts) opts in so we can dogfood the static build.What's in this PR
core/docgenopen service — definition inservices/docgen/{definition,server,types}.ts. The query is a tolerant sync read ofstate.components[componentId](returnsundefineduntil extracted). The real work — story-index lookup, provider invocation, error handling — lives in theextractDocgencommand, which returns the payload it just stored.experimental_docgenProviderpreset with aDocgenProviderPresettype that typesnextDocgenas non-nullable (core's identity seed is always supplied). Providers don't need?.noise.DocgenProvider's JSDoc: providers should merge with downstream via{ ...downstream, ...yourOverrides }anddownstream?.field ?? yoursso future fields aren't silently dropped and explicit downstream values (incl. empty strings) are preserved.{ importPath }(taken fromIndexEntry.importPath). Providers that can only read CSF bail on.mdxpaths by forwarding tonextDocgen. The service resolves index → entry → importPath before calling.<outputDir>/services/core/docgen/<componentId>.jsonvia the open-servicestaticInputs+staticPathmachinery. Gated by!options.ignorePreview.code/renderers/react/src/docgen/preset.tsand an addon-docs description enricher incode/addons/docs/src/docgen.ts, both wired throughexperimental_docgenProvider. The chain is exercised in tests so two-provider composition is covered before phase 2/3 swaps in real data.getComponentIdFromEntryhelper extracted tocode/core/src/common/utils/component-id.tsand adopted by both the docgen service and the existing manifests generator.OpenServiceDocgenMissingComponentErrortyped error (code 12) for unknown componentIds.code/core/src/shared/open-service/README.md) captures a lesson from building the service:query.loadbodies should be minimal — ideally a one-liner that calls a command — so the real work stays reusable, testable, and reachable outside the load drain. Added to both the Load concept section and the Design Rules.Telescoping plan
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Ten tests in
services/docgen/server.test.tscover: extractDocgen end-to-end (return value + state write + provider input shape), undefined-on-no-payload semantics, missing-componentId error, provider error propagation, sync query returning undefined before load,.loaded()driving the load body,.loaded()surfacing missing-component errors, static-build output paths and contents, two-provider middleware composition, and the bottom-of-chain undefined passthrough.Manual testing
cd code && yarn storybook:ui:buildcode/storybook-static/services/core/docgen/<componentId>.jsonfiles exist for components in the internal Storybook{ components: { <componentId>: { componentId, name, description, props: [] } } }with the mocked placeholder strings — phase 2/3 swaps these for real RCM-extracted dataDocumentation
Checklist for Maintainers
🦋 Canary release
This PR does not have a canary release associated.
🤖 Generated with Claude Code