Skip to content

Conversation

@schiller-manuel
Copy link
Contributor

No description provided.

@schiller-manuel schiller-manuel requested a review from pi0 as a code owner December 30, 2025 12:48
@vercel
Copy link

vercel bot commented Dec 30, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
nitro.build Ready Ready Preview, Comment Dec 30, 2025 0:53am

@coderabbitai
Copy link

coderabbitai bot commented Dec 30, 2025

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Centralizes preview subprocess termination in src/build/vite/preview.ts by adding killChild(signal), replacing inline signal handlers, using child.once("exit", ...), and registering the HTTP server "close" listener to invoke cleanup. (48 words)

Changes

Cohort / File(s) Summary
Preview server termination refactor
src/build/vite/preview.ts
Adds killChild(signal) helper to terminate the preview child process; replaces inline child.kill calls in SIGINT/SIGHUP handlers with killChild(...); switches child.on("exit", ...) to child.once("exit", ...) while preserving exit-code logging; adds server.httpServer.once("close", ...) to call killChild("SIGTERM").

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive No pull request description was provided by the author, which makes it impossible to assess relevance to the changeset. Add a pull request description that explains the motivation, changes made, and any relevant context for reviewers.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title follows conventional commits format with 'fix' type and clearly describes the main change: handling child process termination when the preview server closes.

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b580fd6 and feb83c4.

📒 Files selected for processing (1)
  • src/build/vite/preview.ts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pi0 pi0 changed the title fix: Kill prerendering child process when the vite preview server closes fix(vite): kill prerender child process when the vite preview server closes Dec 30, 2025
@pkg-pr-new
Copy link

pkg-pr-new bot commented Dec 30, 2025

Open in StackBlitz

npm i https://pkg.pr.new/nitrojs/nitro@3907

commit: b580fd6

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/build/vite/preview.ts (1)

89-95: LGTM! Signal handlers correctly use the new killChild helper.

The refactor improves code consistency by centralizing child process termination logic.

Optional: Consider process.exit(0) for explicit success code.

🔎 Optional: Make exit code explicit
       process.once(sig, () => {
         consola.info(`Stopping preview server...`);
         killChild(sig);
-        process.exit();
+        process.exit(0);
       });
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9e2ab3f and b580fd6.

📒 Files selected for processing (1)
  • src/build/vite/preview.ts
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{ts,js,tsx,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

src/**/*.{ts,js,tsx,jsx}: Use pathe for cross-platform path operations instead of Node.js node:path
Use ESM and modern JavaScript
Do not add comments explaining what the line does unless prompted

Files:

  • src/build/vite/preview.ts
src/{build,dev,runner,cli}/**/*.{ts,js}

📄 CodeRabbit inference engine (AGENTS.md)

Use consola for logging in build/dev code; use nitro.logger when available

Files:

  • src/build/vite/preview.ts
src/**/*.{ts,js}

📄 CodeRabbit inference engine (AGENTS.md)

Use unstorage for storage abstraction

Files:

  • src/build/vite/preview.ts
src/build/**/*.{ts,js}

📄 CodeRabbit inference engine (AGENTS.md)

Virtual modules must be registered in src/build/virtual.ts

Files:

  • src/build/vite/preview.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). (3)
  • GitHub Check: tests-rollup (windows-latest)
  • GitHub Check: tests-rolldown (windows-latest)
  • GitHub Check: tests-rollup (ubuntu-latest)
🔇 Additional comments (2)
src/build/vite/preview.ts (2)

83-87: LGTM! Good abstraction for child process cleanup.

The killChild helper centralizes termination logic and includes a defensive check against killing an already-terminated process.


97-99: No changes needed. According to Vite's type definitions, PreviewServer.httpServer is typed as http.Server (non-nullable), not http.Server | null as stated in the review. The code is safe as written and will not throw a runtime error. The configurePreviewServer hook is invoked after the server is created and listening, so httpServer is guaranteed to be present.

Likely an incorrect or invalid review comment.

@pi0 pi0 merged commit eecf725 into main Dec 30, 2025
3 of 9 checks passed
@pi0 pi0 deleted the fix-preview-kill-child-process branch December 30, 2025 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants