Scripts: Fallback to CP + rm for cross-disk dir move in generate task#34755
Conversation
📝 WalkthroughWalkthroughThe PR adds a ChangesSandbox Generation Robustness
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches📝 Generate docstrings
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)
88-100: 💤 Low valueLGTM! The cross-device fallback logic is sound.
The implementation correctly catches the
EXDEVerror fromrename()and falls back to copy+remove, solving the cross-disk move issue described in the PR.The approach is safe: only the specific cross-device error triggers the fallback, and all other errors are re-thrown. The non-atomicity of the fallback (cp → rm) is an inherent limitation but acceptable for this use case.
Optional: Consider adding debug logging for the fallback path.
Since the fallback happens silently, it may be helpful to log when the cross-device scenario occurs for debugging and visibility, especially given the non-atomic nature of the fallback operation.
📝 Optional logging enhancement
const moveDir = async (from: string, to: string): Promise<void> => { try { await rename(from, to); } catch (error) { // On some platforms (notably Windows), rename doesn't work across different disks volumes if ((error as NodeJS.ErrnoException).code !== 'EXDEV') { throw error; } + console.log(`⚠️ Cross-device move detected, falling back to copy+remove: ${from} → ${to}`); await cp(from, to, { recursive: true }); await rm(from, { recursive: true, force: true }); } };🤖 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 88 - 100, Add a debug/info log in moveDir to report when the EXDEV fallback path is taken and include the source and destination paths and the caught error; specifically, inside the catch block for rename in function moveDir, before calling cp and rm, emit a concise log (using the project's logger convention or console if none exists) like "rename EXDEV fallback: moving from {from} to {to}" plus the error details so operators can see cross-device moves and the associated error.
🤖 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 88-100: Add a debug/info log in moveDir to report when the EXDEV
fallback path is taken and include the source and destination paths and the
caught error; specifically, inside the catch block for rename in function
moveDir, before calling cp and rm, emit a concise log (using the project's
logger convention or console if none exists) like "rename EXDEV fallback: moving
from {from} to {to}" plus the error details so operators can see cross-device
moves and the associated error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41e6610b-33ba-4604-8a9b-e656fe7a150b
📒 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 | 🚨 +665 B 🚨 |
| Bundle Size Analyzer | Link | Link |
Closes #
What I did
This PR fallback
renametocp+rm.rename()can throw if it tries to rename dir across disk volumes, especially on windowsChecklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Try a generate task on windows but with temp dir being on C: and the storybook repo on another disk
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