Build: Cleanup space in sandbox generation runner #33223
Conversation
|
View your CI Pipeline Execution ↗ for commit f0e9e52
☁️ Nx Cloud last updated this comment at |
yannbf
changed the title
📝 WalkthroughWalkthroughThis pull request updates sandbox generation infrastructure and template configurations. It modifies GitHub Actions workflow to add disk cleanup and checkout a specific branch, removes beta rsbuild dependencies from templates, adds template initialization options for HTML rsbuild, improves sandbox script resilience with directory creation and proper IO inheritance, adds Yarn cache cleanup, and simplifies dependency checking to a single root-based installation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (5)
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
🧹 Nitpick comments (1)
scripts/sandbox/generate.ts (1)
199-215: Temp dir cleanup is solid, but tightencreateBaseDirtyping for TS strictnessThe new pattern of keeping
createBaseDirouter-scoped and cleaning it up infinallyis good for avoiding temp dir leaks. However, withlet createBaseDir: string | undefined;you’re passing astring | undefinedintojoin(createBaseDir, BEFORE_DIR_NAME)andlocalizeYarnConfigFiles(createBaseDir, createBeforeDir), which will trip strict null checks.Consider narrowing before use, e.g.:
createBaseDir = await temporaryDirectory(); if (!createBaseDir) { throw new Error('Failed to create temporary base directory'); } const createBeforeDir = join(createBaseDir, BEFORE_DIR_NAME); // ... await localizeYarnConfigFiles(createBaseDir, createBeforeDir);This keeps the runtime behavior and makes the type system happy.
Also applies to: 230-267, 303-316
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/generate-sandboxes.yml(3 hunks)code/lib/cli-storybook/src/sandbox-templates.ts(3 hunks)code/lib/cli-storybook/src/sandbox.ts(2 hunks)scripts/sandbox/generate.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/lib/cli-storybook/src/sandbox.tsscripts/sandbox/generate.tscode/lib/cli-storybook/src/sandbox-templates.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/lib/cli-storybook/src/sandbox.tsscripts/sandbox/generate.tscode/lib/cli-storybook/src/sandbox-templates.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/lib/cli-storybook/src/sandbox.tscode/lib/cli-storybook/src/sandbox-templates.ts
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/lib/cli-storybook/src/sandbox.tscode/lib/cli-storybook/src/sandbox-templates.ts
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/lib/cli-storybook/src/sandbox.tscode/lib/cli-storybook/src/sandbox-templates.ts
🧠 Learnings (7)
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-09-29T13:20:23.346Z
Learnt from: mrginglymus
Repo: storybookjs/storybook PR: 32556
File: code/core/package.json:309-313
Timestamp: 2025-09-29T13:20:23.346Z
Learning: The `fast-printf` dependency in Storybook's core package is bundled into the final distribution during the build process, so it should remain in devDependencies rather than being moved to dependencies, following the same pattern as other bundled dependencies like `open`.
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
📚 Learning: 2025-11-28T14:50:24.872Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.872Z
Learning: Follow existing patterns and conventions in the Storybook codebase
Applied to files:
code/lib/cli-storybook/src/sandbox-templates.ts
🧬 Code graph analysis (1)
scripts/sandbox/generate.ts (1)
code/core/src/common/utils/cli.ts (1)
temporaryDirectory(18-22)
⏰ 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). (3)
- GitHub Check: merged
- GitHub Check: nx
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/lib/cli-storybook/src/sandbox.ts (1)
2-2: Template destination creation and inherited stdio look correctCreating
templateDestinationup front and switchingspawnSynctostdio: 'inherit'align well with the rest of the CLI behavior and make the subsequent emptiness check viareaddir(templateDestination)more robust. I don’t see further changes needed here.Also applies to: 192-207
code/lib/cli-storybook/src/sandbox-templates.ts (2)
485-504: RsBuild templates: dependency cleanup and HTML init options look consistentFor the RsBuild templates:
react-rsbuild/default-tsdropping the explicit prereleasestorybook-react-rsbuild@...fromextraDependenciesand keeping onlyprop-typesmatches the intent to move off beta packages and let the init flow own the framework dependency.- Adding
initOptions: { type: ProjectType.HTML }tohtml-rsbuild/default-tsbrings it in line with thehtml-vitetemplates, so downstream init flows can treat it explicitly as an HTML project type.This all looks coherent with the rest of the template matrix. Please just double‑check that the init flow indeed adds the required
storybook-*-rsbuildpackages so we don’t regress the RsBuild sandboxes.Also applies to: 592-607
629-642: Angular prerelease template dependency updates align with CLI “next”, verify versionsUpdating the Angular CLI prerelease template to:
- Include
@standard-schema/spec@^1, and- Track
@angular/forms@nextinstead of a fixed@angular/forms@^21.xfits the
@angular/cli@nextusage in the script and should better follow the prerelease ecosystem.Please confirm that these versions match the Angular prerelease stack you’re targeting and that
@standard-schema/specis indeed required for the generated project (so we don’t carry an unused extra dependency).
| push: | ||
| branches: | ||
| - yann/fix-sandbox-generation-2 |
There was a problem hiding this comment.
Revert branch‑specific wiring in this workflow before merging
The added push trigger and the ref: yann/fix-sandbox-generation-2 in both jobs are great for debugging this PR, but if merged as‑is:
- Scheduled runs will still try to check out
yann/fix-sandbox-generation-2, which may not exist. - The “Generate to next/main” jobs won’t actually run against
next/main.
Before merging, it’d be good to:
- Remove or comment out the
on.pushblock targeting this feature branch. - Restore the checkout refs to whatever the previous values were (e.g.,
nextandmain).
The disk‑space cleanup steps themselves look fine; only tiny nit is the duplicated /usr/local/share/powershell path, which you can optionally dedupe.
Also applies to: 30-45, 49-49, 100-115, 118-118
🤖 Prompt for AI Agents
In .github/workflows/generate-sandboxes.yml around lines 9-11 (and also review
blocks at 30-45, 49, 100-115, 118), the workflow was wired to this feature
branch for debugging: remove or comment out the on.push block that targets
branch yann/fix-sandbox-generation-2, and restore the checkout actions in each
job to use the intended refs (e.g., ref: next for the next job and ref: main for
the main job) instead of the feature branch; additionally dedupe the duplicated
/usr/local/share/powershell path in the disk-cleanup steps if desired.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/check-dependencies.js (1)
23-27: Consider simplifying the stdio configuration.The stdio array
['inherit', 'inherit', 'inherit']can be replaced with the shorthandstdio: 'inherit', which is equivalent and more concise.Apply this diff:
- const task = spawn('yarn', ['install'], { - cwd: ROOT_DIRECTORY, - shell: true, - stdio: ['inherit', 'inherit', 'inherit'], - }); + const task = spawn('yarn', ['install'], { + cwd: ROOT_DIRECTORY, + shell: true, + stdio: 'inherit', + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/check-dependencies.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
scripts/check-dependencies.js
🧬 Code graph analysis (1)
scripts/check-dependencies.js (2)
scripts/reset.js (1)
logger(9-9)scripts/utils/tools.ts (1)
spawn(80-80)
⏰ 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). (5)
- GitHub Check: merged
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
- GitHub Check: Generate to next
- GitHub Check: Generate to main
🔇 Additional comments (2)
scripts/check-dependencies.js (2)
15-15: LGTM!The ROOT_DIRECTORY definition correctly points to the project root, simplifying the dependency installation to a single location.
19-47: Excellent refactoring of the dependency check logic.The simplification from multiple per-directory installations to a single root-based check significantly improves code clarity and maintainability. The straightforward control flow makes the script easier to understand and debug.
| await new Promise((res, rej) => { | ||
| task.on('exit', (code) => { | ||
| if (code !== 0) { | ||
| rej(); | ||
| } else { | ||
| res(); | ||
| } | ||
| }); | ||
| }).catch(() => { | ||
| task.kill(); | ||
| throw new Error('Failed to install dependencies'); | ||
| }); |
There was a problem hiding this comment.
Remove pointless kill() call on already-exited process.
The task.kill() call on line 38 attempts to kill a process that has already exited. The catch handler only runs after the 'exit' event fires, meaning the process has already terminated. While this doesn't break functionality (kill() on an exited process is a no-op), it represents a logic flaw.
Consider removing the kill() call:
}).catch(() => {
- task.kill();
throw new Error('Failed to install dependencies');
});📝 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.
| await new Promise((res, rej) => { | |
| task.on('exit', (code) => { | |
| if (code !== 0) { | |
| rej(); | |
| } else { | |
| res(); | |
| } | |
| }); | |
| }).catch(() => { | |
| task.kill(); | |
| throw new Error('Failed to install dependencies'); | |
| }); | |
| await new Promise((res, rej) => { | |
| task.on('exit', (code) => { | |
| if (code !== 0) { | |
| rej(); | |
| } else { | |
| res(); | |
| } | |
| }); | |
| }).catch(() => { | |
| throw new Error('Failed to install dependencies'); | |
| }); |
🤖 Prompt for AI Agents
In scripts/check-dependencies.js around lines 29 to 40, the catch block calls
task.kill() after the 'exit' event has already fired; remove the pointless
task.kill() call and instead simply rethrow or throw a new Error with a
descriptive message (include the original error if available) so the catch only
reports the failure without attempting to kill an already-exited process.
kasperpeulen
force-pushed
the
yann/fix-sandbox-generation-2
branch
from
ae66902 to
7006a24
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scripts/sandbox/utils/yarn.ts (1)
14-23: ScopeenableGlobalCachetoggle to Yarn berry or clarify classic behavior
enableGlobalCacheis a modern Yarn setting; whenversionis'classic', this line may be effectively a no-op (or just write an unused key) while giving the impression global caching is disabled there too. For clarity and to avoid relying on undefined behavior on classic, consider gating this like thenodeLinkerconfig:- await runCommand(`yarn config set enableGlobalCache false`, { cwd }); + if (version === 'berry') { + await runCommand('yarn config set enableGlobalCache false', { cwd }); + }This keeps the intent explicit for Yarn 2+/4 sandboxes and avoids confusing config in classic ones. Please double‑check against the Yarn config docs for v1 vs v4 to ensure this matches the expected behavior in your environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scripts/sandbox/generate.ts(4 hunks)scripts/sandbox/utils/yarn.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/sandbox/generate.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
scripts/sandbox/utils/yarn.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
scripts/sandbox/utils/yarn.ts
🧠 Learnings (2)
📚 Learning: 2025-11-28T14:50:24.872Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.872Z
Learning: Use Yarn 4.9.1 as the package manager
Applied to files:
scripts/sandbox/utils/yarn.ts
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
scripts/sandbox/utils/yarn.ts
🧬 Code graph analysis (1)
scripts/sandbox/utils/yarn.ts (1)
scripts/sandbox/generate.ts (1)
runCommand(124-135)
⏰ 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). (5)
- GitHub Check: merged
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
- GitHub Check: Generate to main
- GitHub Check: Generate to next
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/sandbox/generate.ts (1)
38-38:StorybookInitErroris now unused but still referenced in error handling.With
addStorybookcommented out,StorybookInitErroris never thrown, yet it's still checked in the summary table (lines 365-368). If the commented-out code is temporary, this is fine; otherwise, consider removing the dead code path.Also applies to: 365-368
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/sandbox/generate.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
scripts/sandbox/generate.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
scripts/sandbox/generate.ts
🧠 Learnings (2)
📚 Learning: 2025-11-28T14:50:24.872Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.872Z
Learning: Use Yarn 4.9.1 as the package manager
Applied to files:
scripts/sandbox/generate.ts
📚 Learning: 2025-11-28T14:50:24.872Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-28T14:50:24.872Z
Learning: Follow existing patterns and conventions in the Storybook codebase
Applied to files:
scripts/sandbox/generate.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). (5)
- GitHub Check: merged
- GitHub Check: Core Unit Tests, windows-latest
- GitHub Check: nx
- GitHub Check: Generate to main
- GitHub Check: Generate to next
🔇 Additional comments (4)
scripts/sandbox/generate.ts (4)
67-67: LGTM: Global registry configuration.Using the
-gflag ensures the npm registry setting is applied globally, which is appropriate for CI sandbox generation where consistency across all npm operations is needed.Also applies to: 73-73
113-113: LGTM: Yarn Berry adoption.Switching to
YARN2(Yarn Berry) aligns with the repository's use of Yarn 4.9.1. Based on learnings, this is the expected package manager.
199-200: LGTM: Proper resource cleanup pattern.Hoisting
createBaseDirto the outer scope and checking its existence in thefinallyblock ensures the temporary directory is always cleaned up, even if an error occurs. This is a sound pattern for resource management.Also applies to: 213-213, 318-321
303-316: LGTM: Yarn cache cleanup for disk space reduction.Adding
.yarn/cachecleanup alongsidenode_modulesremoval aligns well with the PR objective to reduce the ~15GB accumulated cache. Theforce: trueoption ensures no errors if the directories don't exist.
Closes #
What I did
This PR immensely reduces size usage in our GH actions by deleting 20Gb of unnecessary files. It also fixes other sandbox generation issues as well.
TODO: Clear yarn cache in between sandbox generation because it currently accumulates 15Gb of files!
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
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
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.