Scripts: Log error cause for generate task#34754
Conversation
📝 WalkthroughWalkthroughThe pull request improves error propagation in the sandbox generation script by adding Error ChangesError Cause Propagation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches📝 Generate docstrings
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/sandbox/generate.ts (1)
334-341: ⚡ Quick winConsider using
AggregateErrorinstead of an array incause.Passing an array of errors as the
causeproperty is non-standard. Error monitoring and logging tools typically expectError.causeto be a singleErrorobject, not an array. Node.js providesAggregateError(available since v15.0.0) specifically for representing multiple errors.♻️ Refactor to use AggregateError
- throw new Error(`Some sandboxes failed to generate`, { - cause: generationResults - .filter((result) => result.status === 'rejected') - .map((result) => { - const generationError = (result as PromiseRejectedResult).reason as Error; - return generationError; - }), - }); + throw new AggregateError( + generationResults + .filter((result) => result.status === 'rejected') + .map((result) => (result as PromiseRejectedResult).reason), + 'Some sandboxes failed to generate' + );🤖 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 `@scripts/sandbox/generate.ts` around lines 334 - 341, Replace the thrown Error that uses an array in its cause with an AggregateError: collect the rejected promises from generationResults (the current .filter/.map that produces generationError from PromiseRejectedResult) into an array of Error objects and throw new AggregateError(errorsArray, 'Some sandboxes failed to generate') instead of throw new Error(...); ensure you remove the array-as-cause usage and reference the existing generationResults and generationError variables when building the AggregateError.
🤖 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 `@scripts/sandbox/generate.ts`:
- Around line 334-341: Replace the thrown Error that uses an array in its cause
with an AggregateError: collect the rejected promises from generationResults
(the current .filter/.map that produces generationError from
PromiseRejectedResult) into an array of Error objects and throw new
AggregateError(errorsArray, 'Some sandboxes failed to generate') instead of
throw new Error(...); ensure you remove the array-as-cause usage and reference
the existing generationResults and generationError variables when building the
AggregateError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7602194f-caf2-4e83-a29d-b40ab69a5549
📒 Files selected for processing (1)
scripts/sandbox/generate.ts
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 194 | 194 | 0 |
| Self size | 906 KB | 890 KB | 🎉 -16 KB 🎉 |
| Dependency size | 84.49 MB | 84.49 MB | 🚨 +1 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
Closes #
What I did
When Aa generate task fails, there's no information on what error caused the task to fail;
This PR adds source errors to throwned errors
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
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.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