Repository navigation
Angular: resolve story-docs snippets through the core/docgen service instead of a second analyzer - #35843
Conversation
…instead of a second analyzer
Alternative to the shared-manager approach: instead of widening the docgen
worker protocol, story-docs-preset.ts now resolves the already-registered
core/docgen OSA service (getService('core/docgen', { internal: true })) and
reads the raw analyzer fields (selector, inputsClass/outputsClass, the enum
table) that build-docgen.ts now also stores on AngularDocgenPayload alongside
argTypes. No second AngularComponentMetaManager, no protocol change, no
common-preset.ts wiring: querying core/docgen for a component id transparently
triggers the same docgenWorker.extract() docgen already runs.
|
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 32 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAngular story-docs generation now retrieves metadata asynchronously through ChangesAngular story-docs integration
Sequence Diagram(s)sequenceDiagram
participant StoryDocsProvider
participant CoreDocgen
participant BuildStoryDocsPayload
StoryDocsProvider->>CoreDocgen: query component docgen payload
CoreDocgen-->>StoryDocsProvider: AngularDocgenPayload or undefined
StoryDocsProvider->>BuildStoryDocsPayload: await payload construction
BuildStoryDocsPayload->>BuildStoryDocsPayload: create snippet context from entry and enums
Possibly related PRs
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/frameworks/angular-vite/src/docgen/story-docs-build.test.ts`:
- Around line 86-99: Replace direct getDocgenPayload resolver implementations
with a shared resolver spy declared before tests, and configure each mock
behavior in beforeEach using vi.mocked(). In
code/frameworks/angular-vite/src/docgen/story-docs-build.test.ts#L86-L99,
configure the successful payload; at `#L114-L116`, configure the rejected query;
in
code/lib/docgen-harness/src/angular/angular-story-docs-snippets.test.ts#L37-L63
and
code/lib/docgen-harness/src/angular/story-docs/build-story-docs.test.ts#L26-L52,
use the shared typed spy instead of direct resolver callbacks.
In `@code/frameworks/angular-vite/src/docgen/story-docs-preset.ts`:
- Around line 9-11: Update the comment near the `core/docgen` registration to
retain the rationale for defensively handling registration order and avoiding a
type assertion, but remove the explicit `common-preset.ts` cross-file reference.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0520050-71ee-42a9-aba7-acc42b3ac264
📒 Files selected for processing (7)
code/frameworks/angular-vite/src/docgen/build-docgen.tscode/frameworks/angular-vite/src/docgen/story-docs-build.test.tscode/frameworks/angular-vite/src/docgen/story-docs-build.tscode/frameworks/angular-vite/src/docgen/story-docs-preset.tscode/lib/angular-cm/src/index.tscode/lib/docgen-harness/src/angular/angular-story-docs-snippets.test.tscode/lib/docgen-harness/src/angular/story-docs/build-story-docs.test.ts
| const getDocgenPayload = async (): Promise<AngularDocgenPayload> => | ||
| ({ | ||
| id: 'example-button', | ||
| name: 'ButtonComponent', | ||
| path: STORY_PATH, | ||
| jsDocTags: {}, | ||
| angularComponentMeta: { | ||
| name: 'ButtonComponent', | ||
| selector: 'sb-button', | ||
| inputsClass: [{ name: 'label' }], | ||
| outputsClass: [], | ||
| }, | ||
| angularComponentMetaJson: { miscellaneous: { typealiases: [], enumerations: [] } }, | ||
| }) as AngularDocgenPayload; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the required Vitest spy setup for getDocgenPayload.
Declare the resolver spy before the test cases. Configure its behavior in beforeEach. Use vi.mocked() to access the typed mock.
code/frameworks/angular-vite/src/docgen/story-docs-build.test.ts#L86-L99: Configure the successful payload response inbeforeEach.code/frameworks/angular-vite/src/docgen/story-docs-build.test.ts#L114-L116: Configure the rejected query response inbeforeEach.code/lib/docgen-harness/src/angular/angular-story-docs-snippets.test.ts#L37-L63: Replace the direct resolver callback with the shared typed spy.code/lib/docgen-harness/src/angular/story-docs/build-story-docs.test.ts#L26-L52: Replace the direct resolver callback with the shared typed spy.
As per coding guidelines, implement mock behaviors in beforeEach blocks, use vi.mocked() to type and access mocked functions, and avoid direct function mocking without vi.mocked().
📍 Affects 3 files
code/frameworks/angular-vite/src/docgen/story-docs-build.test.ts#L86-L99(this comment)code/frameworks/angular-vite/src/docgen/story-docs-build.test.ts#L114-L116code/lib/docgen-harness/src/angular/angular-story-docs-snippets.test.ts#L37-L63code/lib/docgen-harness/src/angular/story-docs/build-story-docs.test.ts#L26-L52
🤖 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/frameworks/angular-vite/src/docgen/story-docs-build.test.ts` around
lines 86 - 99, Replace direct getDocgenPayload resolver implementations with a
shared resolver spy declared before tests, and configure each mock behavior in
beforeEach using vi.mocked(). In
code/frameworks/angular-vite/src/docgen/story-docs-build.test.ts#L86-L99,
configure the successful payload; at `#L114-L116`, configure the rejected query;
in
code/lib/docgen-harness/src/angular/angular-story-docs-snippets.test.ts#L37-L63
and
code/lib/docgen-harness/src/angular/story-docs/build-story-docs.test.ts#L26-L52,
use the shared typed spy instead of direct resolver callbacks.
Source: Coding guidelines
| // `core/docgen` is only registered when `experimentalDocgenServer` set up its worker (see | ||
| // `common-preset.ts`); both services are gated by the same feature, but registration order isn't | ||
| // a type-level guarantee, so this stays defensive rather than asserting the service exists. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the cross-file source reference.
Keep the registration-order rationale. Remove the common-preset.ts reference from this comment.
As per coding guidelines, comments should explain maintenance-relevant rationale, not 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/frameworks/angular-vite/src/docgen/story-docs-preset.ts` around lines 9
- 11, Update the comment near the `core/docgen` registration to retain the
rationale for defensively handling registration order and avoiding a type
assertion, but remove the explicit `common-preset.ts` cross-file reference.
Source: Coding guidelines
…yzer MetadataJson
angularComponentMetaJson mirrored the analyzer's own {entry, json} split
mechanically instead of asking what story-docs actually reads off json: only
miscellaneous.enumerations, to resolve Enum.Member story args. The rest of
MetadataJson (every other component/directive/pipe/class in the file) never
gets touched, so storing all of it on core/docgen's service state duplicated
data already present elsewhere.
Replaces angularComponentMetaJson?: MetadataJson with
angularComponentEnums?: EnumType[], the one slice that's needed. No longer
need to export MetadataJson from @storybook/angular-cm's public API either,
since angular-vite already imports EnumType from @storybook/angular-compodoc
directly.
|
Pushed a follow-up commit addressing feedback on the first version of this: Replaced it with |
angularComponentMeta and angularComponentEnums were two independently-optional
sibling fields that only ever get set or read together, relying on the two
call sites to keep them in sync by convention rather than the type enforcing
it. Collapses them into one field, angularComponentMeta: { entry, enums },
still narrower than the analyzer's own AngularComponentMetaResult (drops the
unused rest of MetadataJson) but a single unit instead of two.
|
Pushed a second follow-up: collapsed `angularComponentMeta`/`angularComponentEnums` into one field, `angularComponentMeta: { entry, enums }`. The two were always set and read together, but nothing in the type said so, only the two call sites agreeing by convention. Storing the full analyzer `AngularComponentMetaResult` (`{ entry, json }`) as a single field would also solve that, but reopens the original problem: `json` carries every other component/directive/pipe the analyzer found in the file, none of which story-docs touches. This keeps the narrowing (only `enums`, not the whole `json`) while making "these travel together" structural instead of conventional. |
436a709
into
storybookjs:valentin/angular-story-docs-snippets
1 participant
Closes #
What I did
Alternative to #35841 for the same problem: story-docs built its own
AngularComponentMetaManagerright next to the onedocgen-worker.tsalready owns, so we ran two warm TypeScript language services over the same files, and story-docs's copy never calledstartWatching(), so its snippets went stale after a component edit while docgen kept updating live.#35841 fixes that by widening the docgen worker protocol with a third message kind so story-docs can query the worker directly. This PR takes a different angle:
core/docgenis already a registered OSA service, and story-docs already runs in the same main-thread process that service lives in. So instead of reaching into the worker,story-docs-preset.tsjust resolvescore/docgenas an internal service dependency:build-docgen.tsalready attached the analyzer's class record to the stored docgen payload asangularComponentMeta. It now carries{ entry, enums }rather than the bare class record:enumsis the file's enum declarations, needed to resolve args likekind: ButtonKind.Secondary(enums aren't a property of the class - the analyzer collects them once per file, as a sibling of the class record, not nested inside it). That's deliberately just the one slice of the analyzer's file-level output story-docs reads, not the whole file metadata (which also lists every other component/directive/pipe the analyzer found in the file) - no point duplicating data already stored elsewhere for those.entryandenumsare bundled into one field rather than two independent ones because they're always set or read together; two siblings would only rely on the two call sites agreeing by convention.core/docgen's schema is av.looseObject, so the extra field passes through validation into service state untouched.Because the lookup is now keyed by component id (matching how
core/docgenstores its state) rather than component path plus export/local name,buildStoryDocsPayloadderives the id the same way docgen does (getComponentIdFromEntry) and no longer needs a manager-shaped dependency at all.One extraction serves both consumers: querying story-docs for a component transparently triggers (or reuses the cached result of) the exact same
docgenWorker.extract()call docgen would have made anyway. No second manager, no protocol change, andcommon-preset.tsis untouched, sincegetServiceresolves against the process-global registry at query time rather than needing anything threaded through preset composition.Where this differs from #35841 (worth comparing before picking one)
angular-cm/angular-vite/docgen-harness. Angular: share one component-meta analyzer between docgen and story-docs #35841 touches those plus 7 files incode/core(protocol, worker, client, preset wiring). If we want the smallest diff, this wins.core/docgen's cross-framework payload schema (alooseObject, so it's already loosely typed, but it does meancore/docgen's state now carries two fields no other framework's docgen provider populates). Angular: share one component-meta analyzer between docgen and story-docs #35841 keeps that data on a purpose-built, explicitly "framework-specific and opaque to core" worker message instead, so the generic docgen service stays framework-agnostic.startWatching()live-reload fix, since both ultimately route through the onedocgenWorker.extract()call.core/docgenhaving successfully extracted for the same component; if docgen's own extraction failed for an unrelated reason, story-docs loses its snippet too (falls back the same way it already does whenmetais undefined, so behavior isn't broken, just the failure surface is now shared). Angular: share one component-meta analyzer between docgen and story-docs #35841 keeps the two failure paths independent.I don't have a strong preference yet, happy to close whichever we don't go with once we've talked it through.
What a bad run looks like
Dropping the enum table in
createSnippetContextinstory-docs-build.ts(passing[]instead of the realenumsoffangularComponentMeta, simulating "forgot to carry the enum table through") breaks the harness parity gate on the one fixture that actually exercises enum resolution:Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn && yarn nx run-many -t compile.yarn task sandbox --template angular-vite/docgen-server-ts --start-from auto(the sandbox withexperimentalDocgenServerandcomponentsManifeston). Open a story's Docs page - argTypes and the Source block snippet should render exactly as they did before this PR.@Input()). Both the argTypes table and the Source block snippet should pick up the change on the next load.yarn test docgen-worker story-docs-build angular-story-docs-snippets build-story-docs common-preset docgenshould all pass.Documentation
MIGRATION.MD
Internal worker plumbing behind an experimental flag, so nothing user-facing to document here.
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>