Repository navigation
Angular: Record per-component docgen baselines from built sandboxes - #35750
Conversation
26fc41b to
d499508
Compare
aa1a12e to
2619f60
Compare
2619f60 to
10e8409
Compare
e2de0f6 to
bb2ce93
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughThe PR adds an Angular Vite server-docgen sandbox, baseline reading and comparison utilities, committed Angular docgen snapshots, and CI verification. It also moves docgen worker ChangesDocgen worker lifecycle
Sandbox docgen baseline verification
Sequence Diagram(s)sequenceDiagram
participant CI
participant SandboxBuild
participant BaselineRunner
participant SnapshotReader
participant Comparator
CI->>SandboxBuild: Build Angular Vite sandbox
CI->>BaselineRunner: Run baselines:sandbox
BaselineRunner->>SnapshotReader: Read generated snapshots
SnapshotReader-->>BaselineRunner: Return normalized baselines
BaselineRunner->>Comparator: Compare committed fixtures
Comparator-->>CI: Return findings and exit status
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (5)
code/core/src/shared/open-service/services/docgen/worker/docgen-worker-client.test.ts (1)
105-110: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAssert that all message listeners are registered before
unref().The constructor currently registers two
messagelisteners.toBeGreaterThan(0)also passes ifunref()runs after only the first listener. AsserttoBe(2)so the test detects that ordering regression.🤖 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/worker/docgen-worker-client.test.ts` around lines 105 - 110, Update the assertion on worker.messageListenersAtUnref in the worker client test to require exactly two registered message listeners with toBe(2), ensuring both constructor listeners are attached before unref() executes.code/lib/docgen-harness/package.json (1)
29-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun the new runner with native Node.
- "baselines:sandbox": "node --import jiti/register ./src/sandbox-baselines/run.ts", + "baselines:sandbox": "node ./src/sandbox-baselines/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/lib/docgen-harness/package.json` at line 29, Update the baselines:sandbox script to invoke the new runner using native Node without the jiti/register import, while preserving the existing src/sandbox-baselines/run.ts entry point.Source: Coding guidelines
code/lib/docgen-harness/src/sandbox-baselines/compare-baselines.ts (1)
26-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSort keys by code unit, not by locale.
stableStringifyserializes the committed baseline files.localeComparewithout an explicit locale uses the host default locale and ICU version, so key order can differ between a developer machine and CI. That produces a re-record diff that reflects the environment rather than the content.Use a code-unit comparison so the output is byte-identical everywhere.
♻️ Proposed deterministic sort
return Object.fromEntries( Object.entries(input) - .sort(([a], [b]) => a.localeCompare(b)) + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) .map(([key, item]) => [key, sortKeys(item)]) );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/docgen-harness/src/sandbox-baselines/compare-baselines.ts` around lines 26 - 41, Update the key comparator in stableStringify’s sortKeys implementation to use deterministic code-unit ordering instead of localeCompare. Preserve the recursive array/object traversal and JSON.stringify behavior while ensuring identical key order across environments.code/lib/docgen-harness/src/sandbox-baselines/read-static-docgen.ts (1)
84-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the snapshot shape at the parse boundary.
Line 119 casts parsed JSON without validation.
toBaselinethen omits any field whose value isundefined. If a snapshot omitsname, line 85 throwsTypeError: Cannot read properties of undefined (reading 'startsWith'), and the message does not name the file or the component id.Add a cheap assertion next to the parse so a malformed snapshot fails at the source with the file path.
♻️ Proposed boundary assertion
for (const [id, payload] of Object.entries(components ?? {})) { + if (typeof payload?.name !== 'string') { + // eslint-disable-next-line local-rules/no-uncategorized-errors + throw new Error(`Snapshot ${path} holds a component (${id}) without a name.`); + } const baseline = toBaseline(payload, sandboxDir);As per coding guidelines: "Encode assumptions with TypeScript types and existing lint rules when possible; otherwise add a cheap runtime assertion close to the relevant boundary so violations fail at the source."
Also applies to: 119-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/docgen-harness/src/sandbox-baselines/read-static-docgen.ts` around lines 84 - 85, Validate the parsed snapshot immediately at the JSON parse boundary before the cast or call toBaseline, asserting that required fields such as name are present and reporting the snapshot file path and component id in the failure. Keep isGloballyReferenced unchanged, while ensuring malformed snapshots fail during parsing rather than later at name.startsWith.Source: Coding guidelines
code/lib/docgen-harness/src/sandbox-baselines/read-static-docgen.test.ts (1)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
memfsspy-redirection pattern.Replace the async factory mock with
vi.mock('node:fs', { spy: true }). RedirectreaddirSyncandreadFileSyncwithvi.mocked(...)inbeforeEach.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/lib/docgen-harness/src/sandbox-baselines/read-static-docgen.test.ts` around lines 8 - 11, Replace the async `vi.mock('node:fs', ...)` factory with the `{ spy: true }` mock form. In the test setup’s `beforeEach`, redirect `readdirSync` and `readFileSync` using `vi.mocked(...)` and the existing memfs implementations, preserving the current filesystem behavior.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/core/src/shared/open-service/services/docgen/worker/docgen-worker-client.test.ts`:
- Around line 105-110: Update the assertion on worker.messageListenersAtUnref in
the worker client test to require exactly two registered message listeners with
toBe(2), ensuring both constructor listeners are attached before unref()
executes.
In `@code/lib/docgen-harness/package.json`:
- Line 29: Update the baselines:sandbox script to invoke the new runner using
native Node without the jiti/register import, while preserving the existing
src/sandbox-baselines/run.ts entry point.
In `@code/lib/docgen-harness/src/sandbox-baselines/compare-baselines.ts`:
- Around line 26-41: Update the key comparator in stableStringify’s sortKeys
implementation to use deterministic code-unit ordering instead of localeCompare.
Preserve the recursive array/object traversal and JSON.stringify behavior while
ensuring identical key order across environments.
In `@code/lib/docgen-harness/src/sandbox-baselines/read-static-docgen.test.ts`:
- Around line 8-11: Replace the async `vi.mock('node:fs', ...)` factory with the
`{ spy: true }` mock form. In the test setup’s `beforeEach`, redirect
`readdirSync` and `readFileSync` using `vi.mocked(...)` and the existing memfs
implementations, preserving the current filesystem behavior.
In `@code/lib/docgen-harness/src/sandbox-baselines/read-static-docgen.ts`:
- Around line 84-85: Validate the parsed snapshot immediately at the JSON parse
boundary before the cast or call toBaseline, asserting that required fields such
as name are present and reporting the snapshot file path and component id in the
failure. Keep isGloballyReferenced unchanged, while ensuring malformed snapshots
fail during parsing rather than later at name.startsWith.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86af213a-7921-4873-ad33-f154f3fbc3ee
📒 Files selected for processing (50)
AGENTS.mdcode/core/src/shared/open-service/services/docgen/worker/docgen-worker-client.test.tscode/core/src/shared/open-service/services/docgen/worker/docgen-worker-client.tscode/lib/cli-storybook/src/sandbox-templates.tscode/lib/docgen-harness/package.jsoncode/lib/docgen-harness/src/sandbox-baselines/README.mdcode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/example-button.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/example-header.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/example-page.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-argtypes-doc-button.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-argtypes-doc-directive.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-argtypes-doc-injectable.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-argtypes-doc-pipe.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-angular-forms-customcontrolvalueaccessor-custom-cva-component.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-complex-selectors-attribute-selectors-component.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-complex-selectors-class-selector-component.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-complex-selectors-multiple-class-selector-component.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-complex-selectors-multiple-selector-component.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-enums-enums-component.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-inheritance-base-button.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-inheritance-icon-button.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-ng-content-ng-content-about-parent.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-ng-content-ng-content-simple.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-ng-on-destroy-component-with-on-destroy.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-on-push-on-push.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-pipe-custom-pipes.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-provider-di-component.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-style-styled-component.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-with-template-template.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-without-selector-without-selector-ng-component-outlet.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-component-without-selector-without-selector.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-ng-module-import-module-chip.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-ng-module-import-module-for-root.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-basics-ng-module-import-module.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-core-decorators-componentwrapperdecorator-decorators.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-core-decorators-theme-decorator-decorators.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-core-modulemetadata-in-export-default.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-core-modulemetadata-in-stories.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-core-modulemetadata-merge-default-and-story.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-core-parameters-bootstrap-options.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-core-styles-story-styles.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-others-app-initializer-use-factory.jsoncode/lib/docgen-harness/src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/stories-frameworks-angular-vite-others-issues-12009-unknown-component.jsoncode/lib/docgen-harness/src/sandbox-baselines/compare-baselines.test.tscode/lib/docgen-harness/src/sandbox-baselines/compare-baselines.tscode/lib/docgen-harness/src/sandbox-baselines/read-static-docgen.test.tscode/lib/docgen-harness/src/sandbox-baselines/read-static-docgen.tscode/lib/docgen-harness/src/sandbox-baselines/run.tsscripts/ci/sandboxes.tsscripts/knip.config.ts
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
a35c6b2 to
3d4273a
Compare
The fixture suites prove the Angular extractor against components written to exercise it. They cannot catch what only shows up across a whole project: component name collisions, imports that resolve to the wrong file, components the tsconfig never covered. A static Storybook build under experimentalDocgenServer already writes one docgen payload per component. This reads that output, strips the parts that are machine-specific or engine-specific, and keeps the result in the repo so a provider change becomes a reviewable diff. Findings are split by severity: a regression means docgen got worse and wants a fix, a change means it moved without getting worse and is adopted by re-recording. Both fail the run. CI verifies after building any sandbox that has baselines committed, derived from what is on disk so adding a directory is all it takes to gate a template.
The recorded baselines are captured from this sandbox's static build, and that build only writes per-component docgen snapshots when both flags are on. Until now no sandbox turned server-side Angular docgen on, so nothing exercised it end to end in CI.
…lines The monorepo's shared template stories reference their component as `globalThis.__TEMPLATE_COMPONENTS__.*`, so there is no import to resolve and no scanned file to find. All 74 of them errored by construction, which says something about the template-story harness rather than about docgen, and they buried the components that carry real signal. The Angular recording goes from 111 components to 37: 30 documented, 7 Angular classes declared inline in story files.
Attaching a `message` listener to a worker re-references its port, so calling `unref()` before the listeners are on leaves the worker holding the event loop open. `build-storybook` finished its work and then hung indefinitely, which on CI showed up as a ten-minute no-output timeout rather than as a failure. Only reachable with experimentalDocgenServer enabled, which no sandbox did until now.
Turning the flags on for the existing Angular sandbox would have swapped what that sandbox guards, leaving today's browser docgen untested while the server path is still experimental. A separate template keeps both covered. Runs daily rather than on every PR: it doubles the Angular sandbox cost and the configuration it guards is not the shipping one yet. Marked in-development so CI generates it from scratch until it is published to the sandboxes repository.
The sandbox now exists on the sandboxes repository, so CI clones it like every other template instead of scaffolding it from scratch on each run.
The sandbox differs from `angular-vite/default-ts` only by two feature flags, so visual output is already covered there on every run and repeating it doubles the Angular cost for no extra signal. `test-runner` is skipped alongside it because the job list adds a test-runner job precisely when chromatic is skipped, so skipping only chromatic would have swapped one job for another rather than dropping one.
…lags There was a hardcoded default template in the recorder and a separate disk-existence check in the CI config, so three places had to agree on which sandboxes carry docgen baselines. Now the flags on the template definition are the only source: a sandbox that turns on server docgen is baselined, and one that does not is not. A flagged template with nothing recorded yet fails instead of skipping quietly, which is the case the disk check used to swallow.
The derivation is exercised by the recorder and the generated CI config, so the unit test was duplicating that. `enablesDocgenServer` goes back to module-private with it, since exporting it was only for the test. Also reframes the cadence TODO: after 11.0 the standard sandboxes ship the new docgen approach by default, so this template gets removed rather than promoted.
Co-authored-by: Valentin Palkovic <dev@valentinpalkovic.dev>
Co-authored-by: Valentin Palkovic <dev@valentinpalkovic.dev>
3d4273a to
49adc42
Compare
Closes #
What I did
Angular docgen is currently tested against small components written to exercise the extractor. That misses the failures users actually hit, which only appear across a whole project: two components sharing a class name, a story importing one file while the docs render another, a component the tsconfig never covered.
This records what docgen produces for every component in a real sandbox and commits it, so a change to the extractor shows up as a reviewable diff.
How a baseline is captured
A committed baseline is just the portable payload:
{ "argTypes": { "aliasedUnionType": { "name": "aliasedUnionType", "description": "\nUnion Type assigned as a Type Alias", "table": { "category": "inputs", "type": { "summary": "TypeAlias", "required": true } }, "type": { "name": "enum", "value": ["Type Alias 1", "Type Alias 2", "Type Alias 3"] } } }, "id": "stories-frameworks-angular-vite-basics-component-with-enums-enums-component", "name": "EnumsComponent", "path": "./src/stories/.../enums.stories.ts" }37 components for the Angular sandbox: 30 documented, 7 Angular classes declared inline in story files that Compodoc does not scan. Recording the second group keeps that visible instead of silently absent.
Scripts introduced
yarn baselines:sandboxyarn baselines:sandbox --updateyarn baselines:sandbox --template <key>Run from
code/lib/docgen-harness. CI runs the verify form right afterBuild storybook.What a failure looks like
Findings are split by severity, and both fail the run:
regressionmeans docgen got worse and wants a fix, not a re-record.changeis neutral or better and is adopted with--update.Which sandboxes are covered
Derived from the templates themselves rather than a list, so there is nothing to keep in sync:
Both
experimentalDocgenServerandcomponentsManifestare required - without the second, nothing is written to disk. A template that is flagged but has nothing recorded fails rather than skipping quietly.A new Angular sandbox
Server docgen had no end-to-end coverage because no sandbox enabled it. Rather than flip the existing Angular sandbox, which would stop it guarding today's browser docgen, this adds
angular-vite/docgen-server-tsdiffering only by those two flags. It has been published to the sandboxes repository, so CI clones it like any other template.It runs daily rather than every PR while the feature is experimental, and skips
chromatic(andtest-runner, which the job list otherwise swaps in when chromatic is skipped). After 11.0 the standard sandboxes ship this docgen approach by default, at which point this template is removed rather than kept.A bug this found immediately
With server docgen on,
build-storybookfinished its work and then never exited - on CI a ten-minute no-output timeout rather than a failure. Attaching amessagelistener to a worker re-references its port, so unreferencing the docgen worker before its listeners were attached left it holding the event loop open:Only reachable with the feature enabled, which is why nothing caught it before.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn task build --template angular-vite/docgen-server-ts --start-from auto. It should exit on its own; before the worker fix it hung here.code/lib/docgen-harness, runyarn baselines:sandbox. It should reportbaselines match.src/sandbox-baselines/__baselines__/angular-vite-docgen-server-ts/that hasargTypes, delete one property, and re-run. It should fail, name that property, and label it a regression.yarn baselines:sandbox --updateto restore, then re-run to confirm it matches again.Documentation
MIGRATION.MD