fix(start): skip errored Rspack modules during import protection#7685
fix(start): skip errored Rspack modules during import protection#7685SyMind wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughBumps Changesrsbuild 2.1.0 upgrade, import-protection fix, and rsbuild E2E fixture
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
# Conflicts: # benchmarks/bundle-size/package.json # e2e/react-router/rspack-basic-file-based/package.json # e2e/react-router/rspack-basic-virtual-named-export-config-file-based/package.json # e2e/react-start/basic/package.json # e2e/react-start/css-inline/package.json # e2e/react-start/custom-server-rsbuild/package.json # e2e/react-start/deferred-hydration/package.json # e2e/react-start/hmr/package.json # e2e/react-start/import-protection/package.json # e2e/react-start/rsc/package.json # e2e/react-start/server-functions/package.json # e2e/solid-router/rspack-basic-file-based/package.json # e2e/solid-router/rspack-basic-virtual-named-export-config-file-based/package.json # e2e/solid-start/basic/package.json # e2e/solid-start/deferred-hydration/package.json # e2e/vue-router/rspack-basic-file-based/package.json # e2e/vue-router/rspack-basic-virtual-named-export-config-file-based/package.json # e2e/vue-start/basic/package.json # examples/react/quickstart-rspack-file-based/package.json # examples/solid/quickstart-rspack-file-based/package.json # packages/react-start/package.json # packages/solid-start/package.json # packages/start-plugin-core/package.json # packages/vue-start/package.json # pnpm-lock.yaml
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![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
🤖 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 `@e2e/react-start/rsc-rsbuild/tests/error-overlay.spec.ts`:
- Around line 28-44: The DevServer startup path currently waits only on
waitUntilReady(), so a spawn failure from pnpm/rsbuild can hang until timeout
instead of failing immediately. In the DevServer.start method, add an error
listener to the spawned process and surface that error right away, ideally
before or alongside the waitUntilReady() flow, so startup failures are reported
immediately instead of being masked by the timeout.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 666b748c-f5f5-4855-9201-a1019bf10caf
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (28)
benchmarks/bundle-size/package.jsone2e/react-router/rspack-basic-file-based/package.jsone2e/react-router/rspack-basic-virtual-named-export-config-file-based/package.jsone2e/react-start/basic/package.jsone2e/react-start/css-inline/package.jsone2e/react-start/custom-server-rsbuild/package.jsone2e/react-start/deferred-hydration/package.jsone2e/react-start/hmr/package.jsone2e/react-start/import-protection/package.jsone2e/react-start/rsc-rsbuild/package.jsone2e/react-start/rsc-rsbuild/tests/error-overlay.spec.tse2e/react-start/rsc/package.jsone2e/react-start/server-functions/package.jsone2e/solid-router/rspack-basic-file-based/package.jsone2e/solid-router/rspack-basic-virtual-named-export-config-file-based/package.jsone2e/solid-start/basic/package.jsone2e/solid-start/deferred-hydration/package.jsone2e/vue-router/rspack-basic-file-based/package.jsone2e/vue-router/rspack-basic-virtual-named-export-config-file-based/package.jsone2e/vue-start/basic/package.jsonexamples/react/quickstart-rspack-file-based/package.jsonexamples/solid/quickstart-rspack-file-based/package.jsonpackages/react-start/package.jsonpackages/router-plugin/package.jsonpackages/solid-start/package.jsonpackages/start-plugin-core/package.jsonpackages/start-plugin-core/src/rsbuild/import-protection.tspackages/vue-start/package.json
|
@codex review |
|
View your CI Pipeline Execution ↗ for commit 66fc253
☁️ Nx Cloud last updated this comment at |
Merging this PR will improve performance by 10.42%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem streaming-peak chunked (vue) |
11.9 MB | 12.3 MB | -3.27% |
| ⚡ | Memory | mem aborted-requests (solid) |
2.4 MB | 1.8 MB | +31.66% |
| ⚡ | Memory | mem aborted-requests (vue) |
1,021 KB | 912.1 KB | +11.93% |
| ⚡ | Memory | mem serialization-payload (solid) |
6.8 MB | 6.5 MB | +4.29% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing SyMind:fix-tanstack-router-dev (66fc253) with main (ba52d2b)1
Footnotes
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nx Cloud has identified a possible root cause for your failed CI:
We investigated this failure and found it to be an environmental issue unrelated to the PR's changes. The test shard encountered EADDRINUSE: address already in use :::40891, meaning a stale process had already claimed the dynamically assigned port before the web server could start. A clean re-run should resolve this without any code changes needed.
No code changes were suggested for this issue.
You can trigger a rerun by pushing an empty commit:
git commit --allow-empty -m "chore: trigger rerun"
git push
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@e2e/react-start/rsbuild/server.js`:
- Around line 32-49: The start() helper currently only handles the child process
“exit” path, so a failed srvx launch from spawn() can surface as an uncaught
“error” event instead of a controlled failure. Update start() to attach an
“error” handler on the spawned child and terminate the process deterministically
with a clear startup failure when srvx cannot be launched, alongside the
existing exit handling in child.on('exit').
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f2cab84-c020-40b7-a57d-4ab8abbce359
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
e2e/react-start/rsbuild/.gitignoree2e/react-start/rsbuild/.prettierignoree2e/react-start/rsbuild/package.jsone2e/react-start/rsbuild/playwright.config.tse2e/react-start/rsbuild/rsbuild.config.tse2e/react-start/rsbuild/server.jse2e/react-start/rsbuild/src/routeTree.gen.tse2e/react-start/rsbuild/src/router.tsxe2e/react-start/rsbuild/src/routes/__root.tsxe2e/react-start/rsbuild/src/routes/index.tsxe2e/react-start/rsbuild/tests/error-overlay.spec.tse2e/react-start/rsbuild/tsconfig.json
💤 Files with no reviewable changes (1)
- e2e/react-start/rsbuild/tests/error-overlay.spec.ts
✅ Files skipped from review due to trivial changes (4)
- e2e/react-start/rsbuild/.prettierignore
- e2e/react-start/rsbuild/.gitignore
- e2e/react-start/rsbuild/playwright.config.ts
- e2e/react-start/rsbuild/src/routeTree.gen.ts
| export function start() { | ||
| const child = spawn( | ||
| 'srvx', | ||
| ['--prod', '-s', resolveDistClientDir(), resolveDistServerEntryPath()], | ||
| { | ||
| stdio: 'inherit', | ||
| shell: process.platform === 'win32', | ||
| }, | ||
| ) | ||
|
|
||
| child.on('exit', (code, signal) => { | ||
| if (signal) { | ||
| process.kill(process.pid, signal) | ||
| return | ||
| } | ||
|
|
||
| process.exit(code ?? 0) | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Handle spawn() failures explicitly.
If srvx cannot be launched, spawn() emits 'error' instead of 'exit'. Right now that path is unhandled, so the fixture can die with an uncaught exception instead of a deterministic startup failure.
Proposed fix
export function start() {
const child = spawn(
'srvx',
['--prod', '-s', resolveDistClientDir(), resolveDistServerEntryPath()],
{
stdio: 'inherit',
shell: process.platform === 'win32',
},
)
+ child.on('error', (error) => {
+ console.error('Failed to start srvx:', error)
+ process.exit(1)
+ })
+
child.on('exit', (code, signal) => {
if (signal) {
process.kill(process.pid, signal)
return
}📝 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.
| export function start() { | |
| const child = spawn( | |
| 'srvx', | |
| ['--prod', '-s', resolveDistClientDir(), resolveDistServerEntryPath()], | |
| { | |
| stdio: 'inherit', | |
| shell: process.platform === 'win32', | |
| }, | |
| ) | |
| child.on('exit', (code, signal) => { | |
| if (signal) { | |
| process.kill(process.pid, signal) | |
| return | |
| } | |
| process.exit(code ?? 0) | |
| }) | |
| export function start() { | |
| const child = spawn( | |
| 'srvx', | |
| ['--prod', '-s', resolveDistClientDir(), resolveDistServerEntryPath()], | |
| { | |
| stdio: 'inherit', | |
| shell: process.platform === 'win32', | |
| }, | |
| ) | |
| child.on('error', (error) => { | |
| console.error('Failed to start srvx:', error) | |
| process.exit(1) | |
| }) | |
| child.on('exit', (code, signal) => { | |
| if (signal) { | |
| process.kill(process.pid, signal) | |
| return | |
| } | |
| process.exit(code ?? 0) | |
| }) |
🤖 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 `@e2e/react-start/rsbuild/server.js` around lines 32 - 49, The start() helper
currently only handles the child process “exit” path, so a failed srvx launch
from spawn() can surface as an uncaught “error” event instead of a controlled
failure. Update start() to attach an “error” handler on the spawned child and
terminate the process deterministically with a clear startup failure when srvx
cannot be launched, alongside the existing exit handling in child.on('exit').
2 participants
Fixes a dev-mode recovery bug where invalid JS/TS code could produce a Rspack compiler-level error from import protection, leaving
rsbuild devunable to recover even after the user fixed the source code.Changes:
Compilation.hooks.processAssetsplugin error.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores