Repository navigation
Bench: Move the docgen perf suite into the docgen harness - #35689
valentinpalkovic merged 1 commit into
Conversation
WalkthroughThe PR moves docgen performance and memory benchmarks into ChangesDocgen harness benchmarks
Sequence Diagram(s)sequenceDiagram
participant CLI as parseCliOptions
participant Scenario as setUpVueScenario
participant Runner as runSeriesChild
participant Engine as Docgen engine
participant Results as JSON result file
CLI->>Scenario: validated Vue scenario options
Scenario->>Runner: deterministic save mutations
Runner->>Engine: start isolated Node process
Engine->>Results: write SeriesResult JSON
Runner->>Results: parse benchmark result
Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
code/lib/docgen-harness/package.json (2)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
"latest"with a reviewedvue-tscversion.The lockfile currently resolves
vue-tscto3.3.7through thelatestdescriptor. A future lockfile refresh can change this version and its diagnostics with"typescript": "^6.0.3".🤖 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 55, Update the vue-tsc dependency in package.json from the floating "latest" descriptor to the reviewed version 3.3.7, preserving the existing dependency key and formatting.
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse native Node for the memory-gate entrypoint.
The gate uses erasable TypeScript syntax. Change its script and
gate.tsexample tonode ./src/perf/docgen-memory/gate.ts. Keepjitibecause the gate launchesmemory-harness.tswithjiti/register.🤖 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` around lines 28 - 30, Update the bench:docgen-memory script and its gate.ts example to invoke gate.ts with native Node using “node ./src/perf/docgen-memory/gate.ts” instead of jiti/register. Keep jiti/register usage for the memory-harness.ts launch within the gate flow.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/lib/docgen-harness/src/perf/docgen-perf/config.ts`:
- Around line 9-17: Add a cheap runtime assertion immediately after the PINNED_N
declaration to verify that its value is even, and fail clearly when it is odd.
Keep the existing PINNED_N value and surrounding benchmark configuration
unchanged.
In `@code/lib/docgen-harness/src/perf/docgen-perf/engines/vue-scenario.ts`:
- Around line 126-131: Update the non-flat targetPaths selection near
targetPackage so componentsPerPackage equal to 0 returns an empty array instead
of calling slice(-0); preserve the existing slice behavior for positive counts
and leave the flat scenario unchanged.
In `@code/lib/docgen-harness/src/perf/docgen-perf/registry.test.ts`:
- Around line 88-89: Update the assertion in the registry test around specFor so
it verifies spec.childPath is a regular file rather than merely an existing
path. Use the filesystem stat check that distinguishes files from directories,
while preserving the existing engine and path context in the failure message.
---
Nitpick comments:
In `@code/lib/docgen-harness/package.json`:
- Line 55: Update the vue-tsc dependency in package.json from the floating
"latest" descriptor to the reviewed version 3.3.7, preserving the existing
dependency key and formatting.
- Around line 28-30: Update the bench:docgen-memory script and its gate.ts
example to invoke gate.ts with native Node using “node
./src/perf/docgen-memory/gate.ts” instead of jiti/register. Keep jiti/register
usage for the memory-harness.ts launch within the gate flow.
🪄 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: 6ae49bc2-b080-409a-91af-503cde677a89
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (51)
AGENTS.mdcode/.oxlintrc.jsoncode/lib/docgen-harness/README.mdcode/lib/docgen-harness/package.jsoncode/lib/docgen-harness/project.jsoncode/lib/docgen-harness/src/perf/PERF-METHODOLOGY.mdcode/lib/docgen-harness/src/perf/docgen-memory/gate.tscode/lib/docgen-harness/src/perf/docgen-memory/generate-project.tscode/lib/docgen-harness/src/perf/docgen-memory/memory-harness.tscode/lib/docgen-harness/src/perf/docgen-perf/aggregate.test.tscode/lib/docgen-harness/src/perf/docgen-perf/aggregate.tscode/lib/docgen-harness/src/perf/docgen-perf/cli.tscode/lib/docgen-harness/src/perf/docgen-perf/config.tscode/lib/docgen-harness/src/perf/docgen-perf/engine.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.test.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-component-meta.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-docgen-api.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-scenario.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/angular.test.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/angular.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/vue.test.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/vue.tscode/lib/docgen-harness/src/perf/docgen-perf/ratios.test.tscode/lib/docgen-harness/src/perf/docgen-perf/ratios.tscode/lib/docgen-harness/src/perf/docgen-perf/registry.test.tscode/lib/docgen-harness/src/perf/docgen-perf/registry.tscode/lib/docgen-harness/src/perf/docgen-perf/report.test.tscode/lib/docgen-harness/src/perf/docgen-perf/report.tscode/lib/docgen-harness/src/perf/docgen-perf/run.tscode/lib/docgen-harness/src/perf/docgen-perf/spawn.tscode/lib/docgen-harness/src/perf/docgen-perf/types.tscode/lib/docgen-harness/src/perf/docgen-shared/args.test.tscode/lib/docgen-harness/src/perf/docgen-shared/args.tscode/lib/docgen-harness/src/perf/docgen-shared/budgets.tscode/lib/docgen-harness/src/perf/docgen-shared/child-output.tscode/lib/docgen-harness/src/perf/docgen-shared/engine-ids.tscode/lib/docgen-harness/src/perf/docgen-shared/paths.tscode/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.test.tscode/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.tscode/lib/docgen-harness/src/perf/docgen-shared/samples.tscode/lib/docgen-harness/src/perf/docgen-shared/sampling.tscode/lib/docgen-harness/src/perf/docgen-shared/series.tscode/lib/docgen-harness/src/perf/docgen-shared/stats.test.tscode/lib/docgen-harness/src/perf/docgen-shared/stats.tsscripts/bench/docgen-shared/paths.tsscripts/ci/common-jobs.tsscripts/knip.config.tsscripts/package.json
💤 Files with no reviewable changes (2)
- scripts/bench/docgen-shared/paths.ts
- scripts/package.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🧹 Nitpick comments (2)
code/lib/docgen-harness/package.json (2)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
"latest"with a reviewedvue-tscversion.The lockfile currently resolves
vue-tscto3.3.7through thelatestdescriptor. A future lockfile refresh can change this version and its diagnostics with"typescript": "^6.0.3".🤖 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 55, Update the vue-tsc dependency in package.json from the floating "latest" descriptor to the reviewed version 3.3.7, preserving the existing dependency key and formatting.
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse native Node for the memory-gate entrypoint.
The gate uses erasable TypeScript syntax. Change its script and
gate.tsexample tonode ./src/perf/docgen-memory/gate.ts. Keepjitibecause the gate launchesmemory-harness.tswithjiti/register.🤖 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` around lines 28 - 30, Update the bench:docgen-memory script and its gate.ts example to invoke gate.ts with native Node using “node ./src/perf/docgen-memory/gate.ts” instead of jiti/register. Keep jiti/register usage for the memory-harness.ts launch within the gate flow.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/lib/docgen-harness/src/perf/docgen-perf/config.ts`:
- Around line 9-17: Add a cheap runtime assertion immediately after the PINNED_N
declaration to verify that its value is even, and fail clearly when it is odd.
Keep the existing PINNED_N value and surrounding benchmark configuration
unchanged.
In `@code/lib/docgen-harness/src/perf/docgen-perf/engines/vue-scenario.ts`:
- Around line 126-131: Update the non-flat targetPaths selection near
targetPackage so componentsPerPackage equal to 0 returns an empty array instead
of calling slice(-0); preserve the existing slice behavior for positive counts
and leave the flat scenario unchanged.
In `@code/lib/docgen-harness/src/perf/docgen-perf/registry.test.ts`:
- Around line 88-89: Update the assertion in the registry test around specFor so
it verifies spec.childPath is a regular file rather than merely an existing
path. Use the filesystem stat check that distinguishes files from directories,
while preserving the existing engine and path context in the failure message.
---
Nitpick comments:
In `@code/lib/docgen-harness/package.json`:
- Line 55: Update the vue-tsc dependency in package.json from the floating
"latest" descriptor to the reviewed version 3.3.7, preserving the existing
dependency key and formatting.
- Around line 28-30: Update the bench:docgen-memory script and its gate.ts
example to invoke gate.ts with native Node using “node
./src/perf/docgen-memory/gate.ts” instead of jiti/register. Keep jiti/register
usage for the memory-harness.ts launch within the gate flow.
🪄 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: 6ae49bc2-b080-409a-91af-503cde677a89
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (51)
AGENTS.mdcode/.oxlintrc.jsoncode/lib/docgen-harness/README.mdcode/lib/docgen-harness/package.jsoncode/lib/docgen-harness/project.jsoncode/lib/docgen-harness/src/perf/PERF-METHODOLOGY.mdcode/lib/docgen-harness/src/perf/docgen-memory/gate.tscode/lib/docgen-harness/src/perf/docgen-memory/generate-project.tscode/lib/docgen-harness/src/perf/docgen-memory/memory-harness.tscode/lib/docgen-harness/src/perf/docgen-perf/aggregate.test.tscode/lib/docgen-harness/src/perf/docgen-perf/aggregate.tscode/lib/docgen-harness/src/perf/docgen-perf/cli.tscode/lib/docgen-harness/src/perf/docgen-perf/config.tscode/lib/docgen-harness/src/perf/docgen-perf/engine.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.test.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-component-meta.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-docgen-api.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-scenario.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/angular.test.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/angular.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/vue.test.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/vue.tscode/lib/docgen-harness/src/perf/docgen-perf/ratios.test.tscode/lib/docgen-harness/src/perf/docgen-perf/ratios.tscode/lib/docgen-harness/src/perf/docgen-perf/registry.test.tscode/lib/docgen-harness/src/perf/docgen-perf/registry.tscode/lib/docgen-harness/src/perf/docgen-perf/report.test.tscode/lib/docgen-harness/src/perf/docgen-perf/report.tscode/lib/docgen-harness/src/perf/docgen-perf/run.tscode/lib/docgen-harness/src/perf/docgen-perf/spawn.tscode/lib/docgen-harness/src/perf/docgen-perf/types.tscode/lib/docgen-harness/src/perf/docgen-shared/args.test.tscode/lib/docgen-harness/src/perf/docgen-shared/args.tscode/lib/docgen-harness/src/perf/docgen-shared/budgets.tscode/lib/docgen-harness/src/perf/docgen-shared/child-output.tscode/lib/docgen-harness/src/perf/docgen-shared/engine-ids.tscode/lib/docgen-harness/src/perf/docgen-shared/paths.tscode/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.test.tscode/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.tscode/lib/docgen-harness/src/perf/docgen-shared/samples.tscode/lib/docgen-harness/src/perf/docgen-shared/sampling.tscode/lib/docgen-harness/src/perf/docgen-shared/series.tscode/lib/docgen-harness/src/perf/docgen-shared/stats.test.tscode/lib/docgen-harness/src/perf/docgen-shared/stats.tsscripts/bench/docgen-shared/paths.tsscripts/ci/common-jobs.tsscripts/knip.config.tsscripts/package.json
💤 Files with no reviewable changes (2)
- scripts/bench/docgen-shared/paths.ts
- scripts/package.json
🛑 Comments failed to post (2)
code/lib/docgen-harness/src/perf/docgen-perf/config.ts (1)
9-17: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a runtime assertion for the
PINNED_Nevenness requirement.The comment states that
PINNED_Nmust stay even, but nothing enforces it. If a future change sets it to an odd number, the control-pair alternation breaks silently, and the benchmark ratios pick up a systematic bias with no failure to signal it.Add a cheap runtime assertion near the declaration.
🛡️ Proposed fix
export const PINNED_N = 6; + +if (PINNED_N % 2 !== 0) { + throw new Error('PINNED_N must be even to preserve control-pair alternation'); +}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Fresh-process spawns per cold/scan median. One value for all engines. * * Must stay even. The two sides of a control pair alternate which one runs first on odd and even * repetitions, so an odd N gives one side the first slot once more than the other, and the cold * figure - a median - then lands on a repetition from the majority slot. That turns the ordering * effect the alternation exists to cancel into a systematic, directional bias on the headline ratio. */ export const PINNED_N = 6; if (PINNED_N % 2 !== 0) { throw new Error('PINNED_N must be even to preserve control-pair alternation'); }🤖 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/perf/docgen-perf/config.ts` around lines 9 - 17, Add a cheap runtime assertion immediately after the PINNED_N declaration to verify that its value is even, and fail clearly when it is odd. Keep the existing PINNED_N value and surrounding benchmark configuration unchanged.Source: Coding guidelines
code/lib/docgen-harness/src/perf/docgen-perf/engines/vue-scenario.ts (1)
126-131: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against
componentsPerPackage: 0producing the full array instead of an empty slice.
countOptionexplicitly accepts0as a valid count (see the sharedargs.test.tscase "accepts zero, which is a meaningful count for most flags"). If a user passes--components-per-package 0with theworkspaceorbase-type-touchscenario,project.componentPaths.slice(-options.componentsPerPackage)evaluates toslice(-0), which behaves likeslice(0)and returns the entire array instead of an empty one. This silently defeats the intended "measure zero components" behavior and can make later indexing (targetPaths[0],targetPaths[t]) operate on the wrong package's data instead of failing clearly.🐛 Proposed fix
const targetPaths = options.scenario === 'flat' ? project.componentPaths - : project.componentPaths.slice(-options.componentsPerPackage); + : options.componentsPerPackage === 0 + ? [] + : project.componentPaths.slice(-options.componentsPerPackage);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const targetPackage = packages - 1; const targetPaths = options.scenario === 'flat' ? project.componentPaths : options.componentsPerPackage === 0 ? [] : project.componentPaths.slice(-options.componentsPerPackage);🤖 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/perf/docgen-perf/engines/vue-scenario.ts` around lines 126 - 131, Update the non-flat targetPaths selection near targetPackage so componentsPerPackage equal to 0 returns an empty array instead of calling slice(-0); preserve the existing slice behavior for positive counts and leave the flat scenario unchanged.
2df0580 to
f9d3364
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/lib/docgen-harness/src/perf/docgen-perf/run.ts (1)
44-170: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the Storybook logger instead of raw
console.*calls. All four sites print benchmark progress and results with rawconsole.log/console.errorinstead of the repository's shared logger, which is one root cause repeated across the orchestrator and CLI generators.
code/lib/docgen-harness/src/perf/docgen-perf/run.ts#L44-L170: replace theconsole.log/console.errorcalls used for suite progress, results, and failure reporting withstorybook/internal/node-logger.code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts#L149-L149: replace the project-generation timingconsole.logwith the shared logger.code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.ts#L163-L165: replace the CLI entryconsole.logwith the shared logger.code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.ts#L318-L321: replace the CLI entryconsole.logwith the shared logger.As per coding guidelines, "Use Storybook loggers instead of raw
console.log,console.warn, andconsole.errorin normal code paths; usestorybook/internal/node-loggerserver-side andstorybook/internal/client-loggerclient-side."🤖 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/perf/docgen-perf/run.ts` around lines 44 - 170, Replace raw console.log and console.error calls with the server-side Storybook logger from storybook/internal/node-logger at all affected sites: update suite progress, results, and failure reporting in code/lib/docgen-harness/src/perf/docgen-perf/run.ts (lines 44-170), project-generation timing in code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts (line 149), and CLI entry logging in code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.ts (lines 163-165) and code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.ts (lines 318-321). Preserve the existing messages and severity while routing them through the shared logger.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/lib/docgen-harness/src/perf/docgen-perf/spawn.ts`:
- Around line 38-58: Update runSeriesChild’s spawnSync call to use the
established series-child timeout configuration, including a matching killSignal.
Detect the timeout-specific result before the generic non-zero status check and
throw a distinct timeout error that includes the captured output, while
preserving existing handling for ordinary exits and missing result JSON.
---
Outside diff comments:
In `@code/lib/docgen-harness/src/perf/docgen-perf/run.ts`:
- Around line 44-170: Replace raw console.log and console.error calls with the
server-side Storybook logger from storybook/internal/node-logger at all affected
sites: update suite progress, results, and failure reporting in
code/lib/docgen-harness/src/perf/docgen-perf/run.ts (lines 44-170),
project-generation timing in
code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts (line 149),
and CLI entry logging in
code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.ts (lines
163-165) and code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.ts
(lines 318-321). Preserve the existing messages and severity while routing them
through the shared logger.
🪄 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: 9162d20e-0a21-4a70-8235-730cb341a72b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (50)
AGENTS.mdcode/.oxlintrc.jsoncode/lib/docgen-harness/README.mdcode/lib/docgen-harness/package.jsoncode/lib/docgen-harness/project.jsoncode/lib/docgen-harness/src/perf/PERF-METHODOLOGY.mdcode/lib/docgen-harness/src/perf/docgen-memory/gate.tscode/lib/docgen-harness/src/perf/docgen-memory/generate-project.tscode/lib/docgen-harness/src/perf/docgen-memory/memory-harness.tscode/lib/docgen-harness/src/perf/docgen-perf/aggregate.test.tscode/lib/docgen-harness/src/perf/docgen-perf/aggregate.tscode/lib/docgen-harness/src/perf/docgen-perf/cli.tscode/lib/docgen-harness/src/perf/docgen-perf/config.tscode/lib/docgen-harness/src/perf/docgen-perf/engine.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.test.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-component-meta.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-docgen-api.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-scenario.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/angular.test.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/angular.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/vue.test.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/vue.tscode/lib/docgen-harness/src/perf/docgen-perf/ratios.test.tscode/lib/docgen-harness/src/perf/docgen-perf/ratios.tscode/lib/docgen-harness/src/perf/docgen-perf/registry.test.tscode/lib/docgen-harness/src/perf/docgen-perf/registry.tscode/lib/docgen-harness/src/perf/docgen-perf/report.test.tscode/lib/docgen-harness/src/perf/docgen-perf/report.tscode/lib/docgen-harness/src/perf/docgen-perf/run.tscode/lib/docgen-harness/src/perf/docgen-perf/spawn.tscode/lib/docgen-harness/src/perf/docgen-perf/types.tscode/lib/docgen-harness/src/perf/docgen-shared/args.test.tscode/lib/docgen-harness/src/perf/docgen-shared/args.tscode/lib/docgen-harness/src/perf/docgen-shared/budgets.tscode/lib/docgen-harness/src/perf/docgen-shared/child-output.tscode/lib/docgen-harness/src/perf/docgen-shared/engine-ids.tscode/lib/docgen-harness/src/perf/docgen-shared/paths.tscode/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.test.tscode/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.tscode/lib/docgen-harness/src/perf/docgen-shared/samples.tscode/lib/docgen-harness/src/perf/docgen-shared/sampling.tscode/lib/docgen-harness/src/perf/docgen-shared/series.tscode/lib/docgen-harness/src/perf/docgen-shared/stats.test.tscode/lib/docgen-harness/src/perf/docgen-shared/stats.tsscripts/ci/common-jobs.tsscripts/knip.config.tsscripts/package.json
🚧 Files skipped from review as they are similar to previous changes (39)
- code/.oxlintrc.json
- AGENTS.md
- scripts/ci/common-jobs.ts
- code/lib/docgen-harness/src/perf/docgen-perf/report.ts
- code/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.test.ts
- code/lib/docgen-harness/src/perf/docgen-shared/stats.test.ts
- code/lib/docgen-harness/src/perf/docgen-perf/types.ts
- code/lib/docgen-harness/src/perf/docgen-perf/registry.ts
- code/lib/docgen-harness/src/perf/docgen-perf/report.test.ts
- code/lib/docgen-harness/project.json
- code/lib/docgen-harness/src/perf/docgen-memory/memory-harness.ts
- code/lib/docgen-harness/src/perf/docgen-shared/args.ts
- code/lib/docgen-harness/src/perf/docgen-shared/budgets.ts
- code/lib/docgen-harness/src/perf/docgen-perf/ratios.test.ts
- code/lib/docgen-harness/package.json
- code/lib/docgen-harness/src/perf/docgen-perf/ratios.ts
- code/lib/docgen-harness/src/perf/docgen-memory/gate.ts
- code/lib/docgen-harness/src/perf/docgen-perf/aggregate.test.ts
- code/lib/docgen-harness/src/perf/docgen-shared/child-output.ts
- code/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.test.ts
- code/lib/docgen-harness/src/perf/docgen-shared/sampling.ts
- code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.test.ts
- scripts/knip.config.ts
- code/lib/docgen-harness/src/perf/docgen-perf/cli.ts
- code/lib/docgen-harness/src/perf/docgen-shared/paths.ts
- code/lib/docgen-harness/src/perf/docgen-shared/args.test.ts
- code/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.ts
- code/lib/docgen-harness/src/perf/docgen-shared/series.ts
- code/lib/docgen-harness/src/perf/docgen-perf/aggregate.ts
- code/lib/docgen-harness/src/perf/docgen-shared/samples.ts
- code/lib/docgen-harness/src/perf/docgen-perf/registry.test.ts
- code/lib/docgen-harness/src/perf/docgen-shared/stats.ts
- code/lib/docgen-harness/src/perf/docgen-shared/engine-ids.ts
- code/lib/docgen-harness/src/perf/docgen-memory/generate-project.ts
- code/lib/docgen-harness/src/perf/docgen-perf/engines/vue-scenario.ts
- code/lib/docgen-harness/src/perf/docgen-perf/config.ts
- code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.test.ts
- code/lib/docgen-harness/README.md
- scripts/package.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/lib/docgen-harness/src/perf/docgen-perf/run.ts (1)
44-170: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse the Storybook logger instead of raw
console.*calls. All four sites print benchmark progress and results with rawconsole.log/console.errorinstead of the repository's shared logger, which is one root cause repeated across the orchestrator and CLI generators.
code/lib/docgen-harness/src/perf/docgen-perf/run.ts#L44-L170: replace theconsole.log/console.errorcalls used for suite progress, results, and failure reporting withstorybook/internal/node-logger.code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts#L149-L149: replace the project-generation timingconsole.logwith the shared logger.code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.ts#L163-L165: replace the CLI entryconsole.logwith the shared logger.code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.ts#L318-L321: replace the CLI entryconsole.logwith the shared logger.As per coding guidelines, "Use Storybook loggers instead of raw
console.log,console.warn, andconsole.errorin normal code paths; usestorybook/internal/node-loggerserver-side andstorybook/internal/client-loggerclient-side."🤖 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/perf/docgen-perf/run.ts` around lines 44 - 170, Replace raw console.log and console.error calls with the server-side Storybook logger from storybook/internal/node-logger at all affected sites: update suite progress, results, and failure reporting in code/lib/docgen-harness/src/perf/docgen-perf/run.ts (lines 44-170), project-generation timing in code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts (line 149), and CLI entry logging in code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.ts (lines 163-165) and code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.ts (lines 318-321). Preserve the existing messages and severity while routing them through the shared logger.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/lib/docgen-harness/src/perf/docgen-perf/spawn.ts`:
- Around line 38-58: Update runSeriesChild’s spawnSync call to use the
established series-child timeout configuration, including a matching killSignal.
Detect the timeout-specific result before the generic non-zero status check and
throw a distinct timeout error that includes the captured output, while
preserving existing handling for ordinary exits and missing result JSON.
---
Outside diff comments:
In `@code/lib/docgen-harness/src/perf/docgen-perf/run.ts`:
- Around line 44-170: Replace raw console.log and console.error calls with the
server-side Storybook logger from storybook/internal/node-logger at all affected
sites: update suite progress, results, and failure reporting in
code/lib/docgen-harness/src/perf/docgen-perf/run.ts (lines 44-170),
project-generation timing in
code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts (line 149),
and CLI entry logging in
code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.ts (lines
163-165) and code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.ts
(lines 318-321). Preserve the existing messages and severity while routing them
through the shared logger.
🪄 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: 9162d20e-0a21-4a70-8235-730cb341a72b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (50)
AGENTS.mdcode/.oxlintrc.jsoncode/lib/docgen-harness/README.mdcode/lib/docgen-harness/package.jsoncode/lib/docgen-harness/project.jsoncode/lib/docgen-harness/src/perf/PERF-METHODOLOGY.mdcode/lib/docgen-harness/src/perf/docgen-memory/gate.tscode/lib/docgen-harness/src/perf/docgen-memory/generate-project.tscode/lib/docgen-harness/src/perf/docgen-memory/memory-harness.tscode/lib/docgen-harness/src/perf/docgen-perf/aggregate.test.tscode/lib/docgen-harness/src/perf/docgen-perf/aggregate.tscode/lib/docgen-harness/src/perf/docgen-perf/cli.tscode/lib/docgen-harness/src/perf/docgen-perf/config.tscode/lib/docgen-harness/src/perf/docgen-perf/engine.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.test.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-component-meta.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-docgen-api.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-scenario.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/angular.test.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/angular.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/vue.test.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/vue.tscode/lib/docgen-harness/src/perf/docgen-perf/ratios.test.tscode/lib/docgen-harness/src/perf/docgen-perf/ratios.tscode/lib/docgen-harness/src/perf/docgen-perf/registry.test.tscode/lib/docgen-harness/src/perf/docgen-perf/registry.tscode/lib/docgen-harness/src/perf/docgen-perf/report.test.tscode/lib/docgen-harness/src/perf/docgen-perf/report.tscode/lib/docgen-harness/src/perf/docgen-perf/run.tscode/lib/docgen-harness/src/perf/docgen-perf/spawn.tscode/lib/docgen-harness/src/perf/docgen-perf/types.tscode/lib/docgen-harness/src/perf/docgen-shared/args.test.tscode/lib/docgen-harness/src/perf/docgen-shared/args.tscode/lib/docgen-harness/src/perf/docgen-shared/budgets.tscode/lib/docgen-harness/src/perf/docgen-shared/child-output.tscode/lib/docgen-harness/src/perf/docgen-shared/engine-ids.tscode/lib/docgen-harness/src/perf/docgen-shared/paths.tscode/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.test.tscode/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.tscode/lib/docgen-harness/src/perf/docgen-shared/samples.tscode/lib/docgen-harness/src/perf/docgen-shared/sampling.tscode/lib/docgen-harness/src/perf/docgen-shared/series.tscode/lib/docgen-harness/src/perf/docgen-shared/stats.test.tscode/lib/docgen-harness/src/perf/docgen-shared/stats.tsscripts/ci/common-jobs.tsscripts/knip.config.tsscripts/package.json
🚧 Files skipped from review as they are similar to previous changes (39)
- code/.oxlintrc.json
- AGENTS.md
- scripts/ci/common-jobs.ts
- code/lib/docgen-harness/src/perf/docgen-perf/report.ts
- code/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.test.ts
- code/lib/docgen-harness/src/perf/docgen-shared/stats.test.ts
- code/lib/docgen-harness/src/perf/docgen-perf/types.ts
- code/lib/docgen-harness/src/perf/docgen-perf/registry.ts
- code/lib/docgen-harness/src/perf/docgen-perf/report.test.ts
- code/lib/docgen-harness/project.json
- code/lib/docgen-harness/src/perf/docgen-memory/memory-harness.ts
- code/lib/docgen-harness/src/perf/docgen-shared/args.ts
- code/lib/docgen-harness/src/perf/docgen-shared/budgets.ts
- code/lib/docgen-harness/src/perf/docgen-perf/ratios.test.ts
- code/lib/docgen-harness/package.json
- code/lib/docgen-harness/src/perf/docgen-perf/ratios.ts
- code/lib/docgen-harness/src/perf/docgen-memory/gate.ts
- code/lib/docgen-harness/src/perf/docgen-perf/aggregate.test.ts
- code/lib/docgen-harness/src/perf/docgen-shared/child-output.ts
- code/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.test.ts
- code/lib/docgen-harness/src/perf/docgen-shared/sampling.ts
- code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.test.ts
- scripts/knip.config.ts
- code/lib/docgen-harness/src/perf/docgen-perf/cli.ts
- code/lib/docgen-harness/src/perf/docgen-shared/paths.ts
- code/lib/docgen-harness/src/perf/docgen-shared/args.test.ts
- code/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.ts
- code/lib/docgen-harness/src/perf/docgen-shared/series.ts
- code/lib/docgen-harness/src/perf/docgen-perf/aggregate.ts
- code/lib/docgen-harness/src/perf/docgen-shared/samples.ts
- code/lib/docgen-harness/src/perf/docgen-perf/registry.test.ts
- code/lib/docgen-harness/src/perf/docgen-shared/stats.ts
- code/lib/docgen-harness/src/perf/docgen-shared/engine-ids.ts
- code/lib/docgen-harness/src/perf/docgen-memory/generate-project.ts
- code/lib/docgen-harness/src/perf/docgen-perf/engines/vue-scenario.ts
- code/lib/docgen-harness/src/perf/docgen-perf/config.ts
- code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.test.ts
- code/lib/docgen-harness/README.md
- scripts/package.json
🛑 Comments failed to post (1)
code/lib/docgen-harness/src/perf/docgen-perf/spawn.ts (1)
38-58: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout guard to
runSeriesChild.
spawnSyncat line 49 has notimeoutoption. If a series-harness child hangs, this call blocks forever.engines/compodoc.tsguards against exactly this failure mode withCOMPODOC_TIMEOUT_MS, but every series-child engine (react-legacy,vue-docgen-api,vue-component-meta) goes through this function unguarded. A hang here stalls the whole suite with no way to recover.Add a
timeoutoption (with a matchingkillSignal) to thespawnSynccall, and treat a timed-out child the same way compodoc does: report it as a distinct failure rather than an ordinary non-zero exit.🛡️ Proposed fix to add a timeout guard
- const proc = spawnSync(process.execPath, nodeArgs, { encoding: 'utf8' }); + const proc = spawnSync(process.execPath, nodeArgs, { + encoding: 'utf8', + timeout: SERIES_CHILD_TIMEOUT_MS, + killSignal: 'SIGKILL', + }); const output = `${proc.stdout ?? ''}${proc.stderr ?? ''}`; + if (proc.signal === 'SIGKILL' && proc.error === undefined) { + throw new Error( + `child did not finish within ${SERIES_CHILD_TIMEOUT_MS}ms:\n${outputTail(output, 4)}` + ); + } if (proc.status !== 0) { throw new Error(`child exited with status ${proc.status}:\n${outputTail(output, 4)}`); }🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 57-57: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(jsonPath, 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').(detect-non-literal-fs-filename-typescript)
🤖 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/perf/docgen-perf/spawn.ts` around lines 38 - 58, Update runSeriesChild’s spawnSync call to use the established series-child timeout configuration, including a matching killSignal. Detect the timeout-specific result before the generic non-zero status check and throw a distinct timeout error that includes the captured output, while preserving existing handling for ordinary exits and missing result JSON.
17d4517 to
f54d04f
Compare
Correctness baselines and performance measurement answer two halves of one question about the same engines, and the split across scripts/ and code/ was already leaking: the compodoc engine test reached four levels up to read this package's committed fixtures. scripts/ is build and release tooling. A measurement suite for product code is not that, so the bench moves to src/perf/ and the two halves sit together. Nothing is re-exported from src/index.ts; these are CLIs, not API. The suite now runs from code/lib/docgen-harness rather than scripts. Two incidental wins. nx.json's sharedGlobals covers scripts/**/*, so every bench edit used to bust the compile and check cache of every project in the repo. And oxfmt skips any path segment named bench, which is why the destination is perf/ - these files are formatted for the first time here. SANDBOX_DIRECTORY is now anchored on package resolution rather than a counted walk to the repo root. The old count was three; from this depth it would be six, and getting it wrong writes generated projects to the wrong place silently.
f54d04f to
32f6748
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
code/lib/docgen-harness/src/perf/PERF-METHODOLOGY.md (1)
31-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify the median definition for cold and warm latency.
The first sentence says that every latency metric uses cold-extraction samples. The next sentence says that warm extraction uses per-save durations. State that all latency metrics use medians, then describe the separate cold and warm sample sources. This prevents readers from applying the cold sampling rule to warm results.
🤖 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/perf/PERF-METHODOLOGY.md` at line 31, Revise the latency methodology paragraph so it first states that all latency metrics use medians, then clearly distinguishes cold samples as one extraction per fresh process spawn and warm samples as per-save durations collected within a single run.code/lib/docgen-harness/src/perf/docgen-memory/memory-harness.ts (2)
143-151: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftValidate the generated project before
--reuse.
tsconfig.jsonalone does not prove that the project matchescomponents,variants,props,heavyTypes,heavyFactor, orbase64Kb. A changed component count can create paths for missing files. Changed generation options can silently benchmark stale data. Store generation parameters in a manifest and regenerate or reject reuse when they differ.🤖 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/perf/docgen-memory/memory-harness.ts` around lines 143 - 151, Update the reuse branch in the memory harness around the existing configPath check to validate a manifest containing components, variants, props, heavyTypes, heavyFactor, and base64Kb against the current options. Write the manifest when generating the project, and regenerate or reject reuse when the manifest is missing or any parameter differs; only return the reused componentPaths and storyPaths after validation succeeds.
102-105: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve the recycling mode in JSON output.
When
--recycle offis parsed,options.recycleHeapPressureRatiobecomesNumber.POSITIVE_INFINITY, whichJSON.stringifywrites asnull. When recycling is on, JSON omits theundefinedfield. Record the CLI mode asonoroff, or encode the sentinel explicitly.🤖 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/perf/docgen-memory/memory-harness.ts` around lines 102 - 105, Update the recycleHeapPressureRatio configuration transform so serialized options preserve the CLI recycling mode instead of producing null for off and omitting the field for on. Store an explicit on/off representation, or otherwise encode the infinity sentinel explicitly, while retaining the existing parsing behavior.code/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc.ts (1)
80-86: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not record empty
psoutput as a zero RSS sample.Line 84 converts an empty
ps.stdoutvalue to0becauseNumber('')is0. If the child exits before theclosehandler clears the interval, Line 86 records a false zero peak. This can let a memory budget pass with invalid data.Require a successful
psstatus, a non-empty value, and a positive finite RSS value.Suggested fix
- const rssKb = Number(ps.stdout.trim()); - if (Number.isFinite(rssKb) && (peakRssKb === undefined || rssKb > peakRssKb)) { + const rssText = ps.stdout.trim(); + if (ps.status !== 0 || !rssText) { + return; + } + const rssKb = Number(rssText); + if ( + Number.isFinite(rssKb) && + rssKb > 0 && + (peakRssKb === undefined || rssKb > peakRssKb) + ) {🤖 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/perf/docgen-perf/engines/compodoc.ts` around lines 80 - 86, Update the RSS sampling logic in the child monitoring interval around spawnSync so it only records samples when ps succeeds, stdout is non-empty after trimming, and the parsed RSS value is finite and positive. Preserve the existing peakRssKb update behavior for valid samples, while ignoring empty or invalid output.code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts (1)
121-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winResolve
jsonOutbeforesetup()or restore the working directory.
runSeriesHarnesswritesspec.jsonOutaftersetup(). A relativejsonOuttherefore resolves underproject.outDirafterprocess.chdir(project.outDir). Resolve the path beforesetup(), or restore the previous working directory before writing the result.🤖 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/perf/docgen-perf/engines/react-legacy.ts` around lines 121 - 123, Update runSeriesHarness so spec.jsonOut is resolved to an absolute path before setup() changes the working directory, or restore the original working directory before writing the result. Preserve the parser’s process.cwd() behavior while ensuring jsonOut always targets the caller’s intended location.
🧹 Nitpick comments (3)
code/lib/docgen-harness/src/perf/docgen-memory/memory-harness.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the issue identifier from the JSDoc.
The JSDoc includes a GitHub issue URL and
#35260. Keep the maintenance rationale for retained and transient memory, but remove ticket and provenance references.As per coding guidelines, comments should explain maintenance rationale and must not contain ticket or provenance references.
Also applies to: 17-19
🤖 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/perf/docgen-memory/memory-harness.ts` around lines 1 - 3, Update the top-level JSDoc for the deterministic in-process memory harness to remove the GitHub URL and issue identifier while retaining the explanation of its retained and transient memory maintenance rationale. Apply the same cleanup to the corresponding repeated JSDoc section.Source: Coding guidelines
code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.ts (1)
168-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEmit an explicit
.tsextension in the generated SFC import.Line 169 emits a relative TypeScript import as
../types. Use../types.ts; the.vueand.svelteexception does not apply because the target is a TypeScript file.Suggested fix
-import type { Pkg${p}Props } from '../types'; +import type { Pkg${p}Props } from '../types.ts';As per coding guidelines, use explicit file extensions for relative TypeScript and JavaScript imports; preserve the package convention only for
.vueand.svelteimports.🤖 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/perf/docgen-perf/generators/vue.ts` around lines 168 - 170, Update the generated import in the Vue generator template returned by the relevant function to reference ../types.ts instead of ../types. Keep the existing import and package naming unchanged, and apply the extension exception only to .vue or .svelte targets.Source: Coding guidelines
code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts (1)
149-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the raw
console.logcalls with the Storybook server logger.The same logging guideline is violated at all three sites. Use the server-side Storybook logger, or document why importing it is unreasonable.
- code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts#L149-L149: replace the project-generation
console.logcall.- code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.ts#L163-L165: replace the Angular generator
console.logcall.- code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.ts#L318-L320: replace the Vue generator
console.logcall.As per coding guidelines, use Storybook loggers instead of raw
console.*in normal server-side 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/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts` at line 149, Replace the raw console.log calls with the Storybook server logger at all three sites: code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts lines 149-149, code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.ts lines 163-165, and code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.ts lines 318-320. Preserve each existing message and use the appropriate logger import; document why importing it is unreasonable only if it cannot be used.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/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc.ts`:
- Around line 121-128: Update the close-listener logic around countDocumentation
to wrap documentation counting and result construction in try/catch; resolve
successful results as before, but call reject with the original caught error
when file access or JSON parsing fails, preventing exceptions from escaping the
EventEmitter callback.
---
Outside diff comments:
In `@code/lib/docgen-harness/src/perf/docgen-memory/memory-harness.ts`:
- Around line 143-151: Update the reuse branch in the memory harness around the
existing configPath check to validate a manifest containing components,
variants, props, heavyTypes, heavyFactor, and base64Kb against the current
options. Write the manifest when generating the project, and regenerate or
reject reuse when the manifest is missing or any parameter differs; only return
the reused componentPaths and storyPaths after validation succeeds.
- Around line 102-105: Update the recycleHeapPressureRatio configuration
transform so serialized options preserve the CLI recycling mode instead of
producing null for off and omitting the field for on. Store an explicit on/off
representation, or otherwise encode the infinity sentinel explicitly, while
retaining the existing parsing behavior.
In `@code/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc.ts`:
- Around line 80-86: Update the RSS sampling logic in the child monitoring
interval around spawnSync so it only records samples when ps succeeds, stdout is
non-empty after trimming, and the parsed RSS value is finite and positive.
Preserve the existing peakRssKb update behavior for valid samples, while
ignoring empty or invalid output.
In `@code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts`:
- Around line 121-123: Update runSeriesHarness so spec.jsonOut is resolved to an
absolute path before setup() changes the working directory, or restore the
original working directory before writing the result. Preserve the parser’s
process.cwd() behavior while ensuring jsonOut always targets the caller’s
intended location.
In `@code/lib/docgen-harness/src/perf/PERF-METHODOLOGY.md`:
- Line 31: Revise the latency methodology paragraph so it first states that all
latency metrics use medians, then clearly distinguishes cold samples as one
extraction per fresh process spawn and warm samples as per-save durations
collected within a single run.
---
Nitpick comments:
In `@code/lib/docgen-harness/src/perf/docgen-memory/memory-harness.ts`:
- Around line 1-3: Update the top-level JSDoc for the deterministic in-process
memory harness to remove the GitHub URL and issue identifier while retaining the
explanation of its retained and transient memory maintenance rationale. Apply
the same cleanup to the corresponding repeated JSDoc section.
In `@code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts`:
- Line 149: Replace the raw console.log calls with the Storybook server logger
at all three sites:
code/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.ts lines
149-149, code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.ts
lines 163-165, and
code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.ts lines 318-320.
Preserve each existing message and use the appropriate logger import; document
why importing it is unreasonable only if it cannot be used.
In `@code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.ts`:
- Around line 168-170: Update the generated import in the Vue generator template
returned by the relevant function to reference ../types.ts instead of ../types.
Keep the existing import and package naming unchanged, and apply the extension
exception only to .vue or .svelte targets.
🪄 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: 0f75e695-b754-4d3b-8289-2bd84161e0b7
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (50)
AGENTS.mdcode/.oxlintrc.jsoncode/lib/docgen-harness/README.mdcode/lib/docgen-harness/package.jsoncode/lib/docgen-harness/project.jsoncode/lib/docgen-harness/src/perf/PERF-METHODOLOGY.mdcode/lib/docgen-harness/src/perf/docgen-memory/gate.tscode/lib/docgen-harness/src/perf/docgen-memory/generate-project.tscode/lib/docgen-harness/src/perf/docgen-memory/memory-harness.tscode/lib/docgen-harness/src/perf/docgen-perf/aggregate.test.tscode/lib/docgen-harness/src/perf/docgen-perf/aggregate.tscode/lib/docgen-harness/src/perf/docgen-perf/cli.tscode/lib/docgen-harness/src/perf/docgen-perf/config.tscode/lib/docgen-harness/src/perf/docgen-perf/engine.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.test.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/react-legacy.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-component-meta.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-docgen-api.tscode/lib/docgen-harness/src/perf/docgen-perf/engines/vue-scenario.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/angular.test.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/angular.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/vue.test.tscode/lib/docgen-harness/src/perf/docgen-perf/generators/vue.tscode/lib/docgen-harness/src/perf/docgen-perf/ratios.test.tscode/lib/docgen-harness/src/perf/docgen-perf/ratios.tscode/lib/docgen-harness/src/perf/docgen-perf/registry.test.tscode/lib/docgen-harness/src/perf/docgen-perf/registry.tscode/lib/docgen-harness/src/perf/docgen-perf/report.test.tscode/lib/docgen-harness/src/perf/docgen-perf/report.tscode/lib/docgen-harness/src/perf/docgen-perf/run.tscode/lib/docgen-harness/src/perf/docgen-perf/spawn.tscode/lib/docgen-harness/src/perf/docgen-perf/types.tscode/lib/docgen-harness/src/perf/docgen-shared/args.test.tscode/lib/docgen-harness/src/perf/docgen-shared/args.tscode/lib/docgen-harness/src/perf/docgen-shared/budgets.tscode/lib/docgen-harness/src/perf/docgen-shared/child-output.tscode/lib/docgen-harness/src/perf/docgen-shared/engine-ids.tscode/lib/docgen-harness/src/perf/docgen-shared/paths.tscode/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.test.tscode/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.tscode/lib/docgen-harness/src/perf/docgen-shared/samples.tscode/lib/docgen-harness/src/perf/docgen-shared/sampling.tscode/lib/docgen-harness/src/perf/docgen-shared/series.tscode/lib/docgen-harness/src/perf/docgen-shared/stats.test.tscode/lib/docgen-harness/src/perf/docgen-shared/stats.tsscripts/ci/common-jobs.tsscripts/knip.config.tsscripts/package.json
🚧 Files skipped from review as they are similar to previous changes (35)
- code/.oxlintrc.json
- code/lib/docgen-harness/src/perf/docgen-shared/args.ts
- code/lib/docgen-harness/src/perf/docgen-shared/child-output.ts
- scripts/knip.config.ts
- AGENTS.md
- code/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.ts
- code/lib/docgen-harness/src/perf/docgen-perf/ratios.ts
- code/lib/docgen-harness/src/perf/docgen-perf/aggregate.ts
- code/lib/docgen-harness/src/perf/docgen-perf/types.ts
- code/lib/docgen-harness/src/perf/docgen-shared/stats.test.ts
- code/lib/docgen-harness/src/perf/docgen-perf/generators/vue.test.ts
- code/lib/docgen-harness/src/perf/docgen-shared/series.ts
- code/lib/docgen-harness/src/perf/docgen-shared/react-renderer-module.test.ts
- scripts/package.json
- code/lib/docgen-harness/src/perf/docgen-shared/budgets.ts
- code/lib/docgen-harness/src/perf/docgen-shared/samples.ts
- code/lib/docgen-harness/src/perf/docgen-perf/registry.test.ts
- code/lib/docgen-harness/src/perf/docgen-perf/engines/compodoc-doc.test.ts
- scripts/ci/common-jobs.ts
- code/lib/docgen-harness/src/perf/docgen-perf/registry.ts
- code/lib/docgen-harness/src/perf/docgen-shared/engine-ids.ts
- code/lib/docgen-harness/src/perf/docgen-perf/ratios.test.ts
- code/lib/docgen-harness/src/perf/docgen-perf/generators/angular.test.ts
- code/lib/docgen-harness/README.md
- code/lib/docgen-harness/src/perf/docgen-shared/sampling.ts
- code/lib/docgen-harness/src/perf/docgen-perf/aggregate.test.ts
- code/lib/docgen-harness/project.json
- code/lib/docgen-harness/src/perf/docgen-perf/cli.ts
- code/lib/docgen-harness/src/perf/docgen-perf/config.ts
- code/lib/docgen-harness/src/perf/docgen-shared/stats.ts
- code/lib/docgen-harness/src/perf/docgen-memory/generate-project.ts
- code/lib/docgen-harness/src/perf/docgen-perf/report.test.ts
- code/lib/docgen-harness/package.json
- code/lib/docgen-harness/src/perf/docgen-perf/engines/vue-scenario.ts
- code/lib/docgen-harness/src/perf/docgen-perf/report.ts
Closes #
What I did
Moved the docgen performance bench from
scripts/bench/intocode/lib/docgen-harness/src/perf/, so correctness baselines and performance measurement for the same engines live in one package. 42 renames, 9 content edits, no logic rewrites.scripts/is build and release tooling; a measurement suite for product code is not. The split was already leaking -engines/compodoc-doc.test.tsreached four levels up to read this package's committed compodoc captures.Two incidental wins:
nx.json'ssharedGlobalscontains{workspaceRoot}/scripts/**/*, so every bench edit busted thecompileandcheckcache for every project in the repo. Moving the files out removes that with no config change.@compodoc/compodocbecomes a declared dependency of the package whose fixtures it generates, instead of a README instruction to runnpx -y @compodoc/compodoc@2.0.0, which bypasses the lockfile.The one hazard.
docgen-shared/paths.tshand-counted..to the repo root. The old count was three; from the new depth it is six, and a wrong root writes generated projects and results JSON to the wrong place with no error. It is now anchored onrequire.resolve('@storybook/docgen-harness/package.json')rather than a count. This is also why it is the one file of 42 git will not pair as a rename at the default threshold -git diff -M20%pairs all 42.Destination is
src/perf/, notsrc/bench/:.oxfmtrc.jsonignores any path segment namedbench, so abench/directory undercode/would be silently skipped byyarn fmt:write. These files are therefore formatted for the first time, which is roughly 150 of the inserted lines.Notes for review:
src/index.ts; theexportsmap andbuild-config.tsare untouched. These are CLIs, not API.project.json'scheckgetsdependsOn: [], mirroringscripts/project.json. Without it, typechecking a benchmark gates on compiling ~48 projects.code/.oxlintrc.jsongets a scopedno-uncategorized-errors: offforsrc/perf/**, matching the existing**/*.compat.tsprecedent. Otherwise the rule fires 21 times, hidden only by CI's--quiet..circleci/config.ymlneeds no regeneration - it is a bootstrap using the continuation orb, soscripts/ci/common-jobs.tsis the source of truth.scripts/." That section is rewritten.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Expect 10 test files / 117 tests, and a results JSON written to
<repo>/../storybook-sandboxes/docgen-perf/, never inside the repo.To confirm the path anchor is what makes that true, move
paths.tsto a different depth inside the package and re-run; the sandbox directory should not move.All of the following were run rather than inferred:
yarn install --frozen-lockfile,yarn dedupe --check,yarn nx check docgen-harness(vue-tsc),tsc -p scripts/tsconfig.json(0 bench errors),yarn lint:js:cmd lib/docgen-harness(0 warnings),yarn fmt:check(clean, 5640 files), both the react-legacy and vue-component-meta engines end to end, and the generated CI config emittingworking_directory: code/lib/docgen-harness. Test count went 116 to 117:registry.test.tsgained one test asserting every spawning engine points at a child script that exists.Not verified: knip would not run locally (unbuilt
@storybook/react-vite), so thelib/docgen-harnessentry added toscripts/knip.config.tsis reasoned, not executed. It cannot fail CI (--no-exit-code).Documentation
MIGRATION.MD
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.