Internal: Repair local check command#33493
Conversation
Sidnioulz
added
build
|
View your CI Pipeline Execution ↗ for commit 4daf489
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughThree build scripts are modified: Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Script as check-package.ts
participant Watchpack as Watchpack
participant Runner as Package Runner
participant Checks as Package Checks
User->>Script: yarn exec jiti check-package.ts --watch
Script->>Script: Parse CLI args & collect TS files per package
Script->>Watchpack: Initialize with aggregated file list
Watchpack->>Script: Initial scan complete
Script->>Runner: Run checks in parallel per package
Runner->>Checks: Execute package check
Checks-->>Runner: Results with per-package prefix
Script->>User: Output status + results
Note over Watchpack,User: Watch mode active...
Watchpack->>Script: File change detected
Script->>Script: Emit file change notification
Script->>Runner: Trigger re-check for affected package
Runner->>Checks: Execute package check
Checks-->>Runner: Updated results
Script->>User: Output changes + new results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In @scripts/check-package.ts:
- Around line 193-221: The current use of selection.forEach(async (v) => { ...
}) launches all checks concurrently causing interleaved output and lack of
sequencing; replace the forEach with a sequential for...of loop (for (const v of
selection) { ... }) and await the child process (await execaCommand(...)) after
wiring up sub.stdout/.stderr handlers (symbols: selection.forEach -> replace,
lastName remains for labeling, sub = execaCommand(...) -> await sub, watchMode
ternary remove or ensure it's false since you're in non-watch branch) so each
package finishes before the next starts and the dead `${watchMode ? ' --watch' :
''}` fragment is removed.
🧹 Nitpick comments (6)
scripts/get-sandbox-dir.ts (1)
8-10: Update type to reflect required option.Since
--templateis now a required option (line 21), the type should reflect this to maintain type consistency and eliminate the implicitundefinedpossibility.Suggested fix
type RunOptions = { - template?: string; + template: string; };scripts/check-package.ts (5)
4-5: Consider consolidating path imports.The file imports from both
node:path(lines 4-5) andpath(line 10). Consider consolidating these for consistency.Suggested consolidation
-import { join } from 'node:path'; -import { relative } from 'node:path'; +import { join, relative, resolve } from 'node:path'; import { program } from 'commander'; // eslint-disable-next-line depend/ban-dependencies import { execaCommand } from 'execa'; -import { resolve } from 'path';Also applies to: 10-10
174-185: ConcurrentrunAllChecksinvocations possible during rapid file changes.When
runAllChecks()is called (lines 174, 184), it's not awaited. If a file change occurs while a check is still running, a newrunAllCheckswill start concurrently, leading to interleaved output.Consider adding a guard to prevent overlapping check runs:
Suggested fix with running guard
+ let isRunning = false; + let pendingRun = false; + async function runAllChecks() { + if (isRunning) { + pendingRun = true; + return; + } + isRunning = true; + const timestamp = new Date().toLocaleTimeString(); // ... existing check logic ... + + isRunning = false; + if (pendingRun) { + pendingRun = false; + runAllChecks(); + } } // Run initial check then watch all files - runAllChecks(); + await runAllChecks();
158-167: Consider typing the caught error more specifically.Using
error: anybypasses TypeScript's type checking. SinceexecaCommandthrowsExecaErrorwhich has typedstdoutandstderrproperties, consider using a type guard or the execa error type. As per coding guidelines, TypeScript strict mode is preferred.Suggested improvement
- } catch (error: any) { + } catch (error: unknown) { const prefix = `\n\n${picocolors.cyan(v.name)}:\n`; process.stdout.write(prefix); - if (error.stdout) { - process.stdout.write(error.stdout); + if (error && typeof error === 'object' && 'stdout' in error && error.stdout) { + process.stdout.write(String(error.stdout)); } - if (error.stderr) { - process.stderr.write(error.stderr); + if (error && typeof error === 'object' && 'stderr' in error && error.stderr) { + process.stderr.write(String(error.stderr)); } }
67-72: Remove redundantprogram.opts()call inside the loop.The
optsvariable is already obtained on line 64. The inner declaration on line 68 shadows it and makes an unnecessary repeated call.Suggested fix
Object.keys(tasks).forEach((key) => { - const opts = program.opts(); // checks if a flag is passed e.g. yarn check addon-docs --watch const containsFlag = program.args.includes(tasks[key].suffix); tasks[key].value = containsFlag || opts.all; });
76-103: Consider addingonCancelhandler for consistent behavior with build-package.ts.The
build-package.tsfile includes anonCancelhandler (line 141) to exit gracefully when the user cancels the prompt. This file doesn't have one, which could lead to inconsistent behavior or unhandled promise rejections.Suggested fix
- ]).then(({ watch, todo }: { watch: boolean; todo: Array<string> }) => { + ], { onCancel: () => process.exit(0) }).then(({ watch, todo }: { watch: boolean; todo: Array<string> }) => { watchMode = watch; return todo?.map((key) => tasks[key]); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/build-package.tsscripts/check-package.tsscripts/get-sandbox-dir.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx,json,md,html,css,scss}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Format code using Prettier with
yarn prettier --write <file>
Files:
scripts/check-package.tsscripts/get-sandbox-dir.tsscripts/build-package.ts
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run ESLint checks using
yarn lint:js:cmd <file>or the full commandcross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directivesto fix linting errors before committing
Files:
scripts/check-package.tsscripts/get-sandbox-dir.tsscripts/build-package.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode across all packages
Files:
scripts/check-package.tsscripts/get-sandbox-dir.tsscripts/build-package.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx}: Export functions from modules if they need to be tested
Do not useconsole.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
scripts/check-package.tsscripts/get-sandbox-dir.tsscripts/build-package.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{js,jsx,json,html,ts,tsx,mjs} : Run ESLint checks using `yarn lint:js:cmd <file>` or the full command `cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives` to fix linting errors before committing
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{ts,tsx} : Enable TypeScript strict mode across all packages
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{ts,tsx} : Enable TypeScript strict mode across all packages
Applied to files:
scripts/check-package.ts
📚 Learning: 2025-12-22T22:03:40.123Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-22T22:03:40.123Z
Learning: Applies to **/*.{js,jsx,json,html,ts,tsx,mjs} : Run ESLint checks using `yarn lint:js:cmd <file>` or the full command `cross-env NODE_ENV=production eslint --cache --cache-location=../.cache/eslint --ext .js,.jsx,.json,.html,.ts,.tsx,.mjs --report-unused-disable-directives` to fix linting errors before committing
Applied to files:
scripts/check-package.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
- GitHub Check: nx
🔇 Additional comments (3)
scripts/build-package.ts (1)
185-188: LGTM! Proper top-level error handling added.The catch handler appropriately surfaces unhandled promise rejections to stderr and exits with a non-zero code, ensuring build failures are properly reported to CI and calling processes.
scripts/get-sandbox-dir.ts (1)
21-21: Good improvement to enforce template requirement at parse time.Using
requiredOptionensures validation happens during argument parsing rather than at runtime, providing clearer error messages to users.scripts/check-package.ts (1)
225-228: LGTM! Error handling is consistent with the build-package.ts pattern.
| selection.forEach(async (v) => { | ||
| const script = join(ROOT_DIRECTORY, 'scripts', 'check', 'check-package.ts'); | ||
| const command = `yarn exec jiti ${script}`; | ||
|
|
||
| const cwd = resolve(__dirname, '..', 'code', v.location); | ||
| const sub = execaCommand(`${command}${watchMode ? ' --watch' : ''}`, { | ||
| cwd, | ||
| env: { | ||
| NODE_ENV: 'production', | ||
| }, | ||
| }); | ||
|
|
||
| sub.stdout?.on('data', (data) => { | ||
| if (lastName !== v.name) { | ||
| const prefix = `${picocolors.cyan(v.name)}:\n`; | ||
| process.stdout.write(prefix); | ||
| } | ||
| lastName = v.name; | ||
| process.stdout.write(data); | ||
| }); | ||
| sub.stderr?.on('data', (data) => { | ||
| if (lastName !== v.name) { | ||
| const prefix = `${picocolors.cyan(v.name)}:\n`; | ||
| process.stdout.write(prefix); | ||
| } | ||
| lastName = v.name; | ||
| process.stderr.write(data); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Uncontrolled concurrency with forEach + async may cause race conditions in output.
Using forEach with an async callback fires off all package checks concurrently without awaiting. While the process stays alive due to the child process streams, this leads to:
- Interleaved output from multiple packages (the
lastNameguard helps but isn't sufficient for true interleaving) - No guarantee of completion order
- If an error occurs, subsequent packages still run
If sequential execution is intended (as the comment on line 192 suggests), consider using a for...of loop with await. If parallel execution is acceptable, the current approach works but the comment should be updated.
Additionally, line 198's ${watchMode ? ' --watch' : ''} is dead code since we're in the else branch where watchMode is falsy.
Suggested fix for sequential execution
} else {
// If watch mode is off, check each individual package sequentially.
- selection.forEach(async (v) => {
+ for (const v of selection) {
const script = join(ROOT_DIRECTORY, 'scripts', 'check', 'check-package.ts');
const command = `yarn exec jiti ${script}`;
const cwd = resolve(__dirname, '..', 'code', v.location);
- const sub = execaCommand(`${command}${watchMode ? ' --watch' : ''}`, {
+ const sub = await execaCommand(command, {
cwd,
env: {
NODE_ENV: 'production',
},
});
- sub.stdout?.on('data', (data) => {
- if (lastName !== v.name) {
- const prefix = `${picocolors.cyan(v.name)}:\n`;
- process.stdout.write(prefix);
- }
- lastName = v.name;
- process.stdout.write(data);
- });
- sub.stderr?.on('data', (data) => {
- if (lastName !== v.name) {
- const prefix = `${picocolors.cyan(v.name)}:\n`;
- process.stdout.write(prefix);
- }
- lastName = v.name;
- process.stderr.write(data);
- });
- });
+ if (sub.stdout) {
+ const prefix = `${picocolors.cyan(v.name)}:\n`;
+ process.stdout.write(prefix);
+ process.stdout.write(sub.stdout);
+ }
+ if (sub.stderr) {
+ process.stderr.write(sub.stderr);
+ }
+ }
}📝 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.
| selection.forEach(async (v) => { | |
| const script = join(ROOT_DIRECTORY, 'scripts', 'check', 'check-package.ts'); | |
| const command = `yarn exec jiti ${script}`; | |
| const cwd = resolve(__dirname, '..', 'code', v.location); | |
| const sub = execaCommand(`${command}${watchMode ? ' --watch' : ''}`, { | |
| cwd, | |
| env: { | |
| NODE_ENV: 'production', | |
| }, | |
| }); | |
| sub.stdout?.on('data', (data) => { | |
| if (lastName !== v.name) { | |
| const prefix = `${picocolors.cyan(v.name)}:\n`; | |
| process.stdout.write(prefix); | |
| } | |
| lastName = v.name; | |
| process.stdout.write(data); | |
| }); | |
| sub.stderr?.on('data', (data) => { | |
| if (lastName !== v.name) { | |
| const prefix = `${picocolors.cyan(v.name)}:\n`; | |
| process.stdout.write(prefix); | |
| } | |
| lastName = v.name; | |
| process.stderr.write(data); | |
| }); | |
| }); | |
| for (const v of selection) { | |
| const script = join(ROOT_DIRECTORY, 'scripts', 'check', 'check-package.ts'); | |
| const command = `yarn exec jiti ${script}`; | |
| const cwd = resolve(__dirname, '..', 'code', v.location); | |
| const sub = await execaCommand(command, { | |
| cwd, | |
| env: { | |
| NODE_ENV: 'production', | |
| }, | |
| }); | |
| if (sub.stdout) { | |
| const prefix = `${picocolors.cyan(v.name)}:\n`; | |
| process.stdout.write(prefix); | |
| process.stdout.write(sub.stdout); | |
| } | |
| if (sub.stderr) { | |
| process.stderr.write(sub.stderr); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @scripts/check-package.ts around lines 193 - 221, The current use of
selection.forEach(async (v) => { ... }) launches all checks concurrently causing
interleaved output and lack of sequencing; replace the forEach with a sequential
for...of loop (for (const v of selection) { ... }) and await the child process
(await execaCommand(...)) after wiring up sub.stdout/.stderr handlers (symbols:
selection.forEach -> replace, lastName remains for labeling, sub =
execaCommand(...) -> await sub, watchMode ternary remove or ensure it's false
since you're in non-watch branch) so each package finishes before the next
starts and the dead `${watchMode ? ' --watch' : ''}` fragment is removed.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 534 | 538 | 🚨 +4 🚨 |
| Self size | 646 KB | 646 KB | 0 B |
| Dependency size | 59.16 MB | 59.19 MB | 🚨 +27 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 274 | 278 | 🚨 +4 🚨 |
| Self size | 24 KB | 24 KB | 🚨 +12 B 🚨 |
| Dependency size | 44.09 MB | 44.12 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-create-react-app
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 64 | 68 | 🚨 +4 🚨 |
| Self size | 32 KB | 32 KB | 🚨 +30 B 🚨 |
| Dependency size | 5.95 MB | 5.98 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 166 | 170 | 🚨 +4 🚨 |
| Self size | 18 KB | 18 KB | 🚨 +28 B 🚨 |
| Dependency size | 31.23 MB | 31.26 MB | 🚨 +28 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
2 participants
What I did
Continuation of the
yarn buildfix PR. This PR fixesyarn check, also improves the handling of--watchand implements a global watch mode on top of TS diagnostics using watchpack (which is already a direct dep).I settled on this approach to watch mode to ensure that we never forget to check a package that depends on types from another package. It's slow, but reliable and reasonably fast to implement.
Checklist for Contributors
Testing
Manual testing
Documentation
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.tsMake 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>Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.