Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaces local runner implementations with the external env-runner package, switches FS watchers to chokidar, moves Vite dev communication to RPC, centralizes runner lifecycle via a RunnerManager, adds a Vite dev-worker runtime, and removes legacy node-runner and node-based Vite runner modules. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/internal/error/dev.ts (1)
1-6:⚠️ Potential issue | 🟠 MajorMove the Node-only stack helpers behind a runtime-specific boundary.
Lazy-loading
youchhelps, but this module still statically importsnode:fs/promises/node:pathand usesprocess.cwd(). With this PR enabling non-Node dev runners, the error handler can fail before it renders the original exception. Please extract the sourcemap/ANSI enrichment into a Node-only helper that is imported conditionally, and fall back to plain HTML/JSON errors elsewhere.As per coding guidelines,
src/runtime/**/*.{js,ts}: Code insrc/runtime/must not use Node.js-specific APIs unless behind runtime checks.Also applies to: 47-57, 101-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/internal/error/dev.ts` around lines 1 - 6, Move Node-specific source-map/ANSI enrichment code out of src/runtime/internal/error/dev.ts into a separate Node-only helper (e.g., devNodeHelpers) that exports functions used for sourcemap lookup and ANSI/color stripping; remove static imports of "node:fs/promises" and "node:path" and any direct process.cwd() usage from dev.ts. In dev.ts (where functions like the HTML/JSON error renderer and any ErrorParser/youch integration live) perform a runtime check for Node (e.g., typeof process !== "undefined" && process.versions?.node) and conditionally dynamic-import the new helper (await import('./devNodeHelpers')) to call its enrichment functions; if the helper is unavailable or the runtime check fails, fall back to plain HTML/JSON error output without ANSI/sourcemap enrichment. Ensure unique symbols referenced in dev.ts (the error rendering entry, any call sites that previously used fs/path or process.cwd()) are updated to call the conditional helper functions so src/runtime/* contains no direct Node APIs.
🧹 Nitpick comments (4)
src/types/runner.ts (1)
1-9: Re-export the newly exposed runner-facing types as well.
NitroOptions.devServer.runnerandNitroHooks["dev:reload"]now depend onRunnerNameandEnvRunnerData, but this façade doesn't expose either one. Re-exporting them here would let downstream code stay on Nitro's type surface instead of importingenv-runnerdirectly.As per coding guidelines,
**/*.{js,ts}: Update types and JSDoc for API changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/runner.ts` around lines 1 - 9, The current re-export block in src/types/runner.ts misses the newly exposed runner-facing types; add RunnerName and EnvRunnerData to the export list so Nitro's surface exposes them (alongside existing FetchHandler, EnvRunner, etc.), and update any related JSDoc/comments for NitroOptions.devServer.runner and NitroHooks["dev:reload"] to reference RunnerName and EnvRunnerData instead of requiring direct imports from env-runner.src/build/vite/env.ts (1)
12-25: Memoize the firstinitEnvRunner()call.
ctx._envRunneris assigned only afterwaitForReady()resolves, so parallel callers can both pass Line 13 and create separate runners. Stashing the in-flight initialization promise, or assigning the instance before awaiting readiness, would keep startup idempotent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/vite/env.ts` around lines 12 - 25, initEnvRunner can race because ctx._envRunner is set only after await runner.waitForReady(); to fix, make the init idempotent by storing the in-flight initialization before awaiting: when starting in initEnvRunner, assign a placeholder (either the runner instance immediately after loadRunner or a promise representing the async init) into ctx._envRunner so concurrent callers see the same value, then await runner.waitForReady() (or await the stored promise) and replace/confirm the final runner; update references to loadRunner, runner.waitForReady, and ctx._envRunner accordingly to ensure only one runner is created.src/types/config.ts (1)
169-174: DocumentdevServer.runnerin the public config type.This adds a new user-facing option, but the type entry still doesn't say what values are expected, what the default is, or that
NITRO_DEV_RUNNERcan affect it. A short JSDoc block here would keep the API self-describing in IntelliSense.As per coding guidelines,
**/*.{js,ts}: Update types and JSDoc for API changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/config.ts` around lines 169 - 174, Add a short JSDoc comment above the devServer.runner property describing that it accepts values defined by the RunnerName union (list the allowed runners or reference RunnerName), state the default runner used when unspecified, and note that the NITRO_DEV_RUNNER environment variable can override this option; update the docblock to appear immediately above devServer.runner so IntelliSense shows the expected values, default, and env override.src/dev/server.ts (1)
69-71: Don't silently dropwriteDevBuildInfofailures.If this write fails, the dev server loses build metadata with no signal. Please log a warning with the cause instead of
.catch(() => {}).As per coding guidelines, "Prefer explicit errors over silent failures" and "Use warnings for recoverable situations; throw for invalid states."💡 Suggested fix
this.#manager.onReady = async (_runner, addr) => { - writeDevBuildInfo(this.nitro, addr).catch(() => {}); + writeDevBuildInfo(this.nitro, addr).catch((error) => { + this.nitro.logger.warn( + `Failed to write dev build info: ${ + error instanceof Error ? error.message : String(error) + }` + ); + }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dev/server.ts` around lines 69 - 71, Replace the silent .catch on the writeDevBuildInfo call in this.#manager.onReady so failures are logged: catch the error from writeDevBuildInfo(this.nitro, addr) and emit a warning including the error message and context (e.g., "writeDevBuildInfo failed for addr: <addr>") instead of using .catch(() => {}); reference the writeDevBuildInfo call inside the this.#manager.onReady handler and use console.warn (or the existing logger if available) to record the error and addr/nitro context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/build/rolldown/dev.ts`:
- Around line 43-48: Replace the manual POSIX-only basename extraction in the
rootDirWatcher callback with pathe's cross-platform basename: where the code
uses path.split("/").pop() inside the chokidarWatch "all" handler (affecting
serverEntryRe matching and root entry reloads), call pathe.basename(path)
instead and ensure pathe is imported; update references in the callback
(serverEntryRe.test(...)) so filename extraction works on Windows and
non-normalized paths and preserves existing logic that checks watchReloadEvents.
In `@src/build/rollup/dev.ts`:
- Around line 45-50: Replace the manual POSIX-only basename extraction in the
file-watcher callback with pathe.basename to handle Windows paths: in the
chokidarWatch callback where you test serverEntryRe against
path.split("/").pop(), call basename(path) (pathe's basename) instead; keep the
surrounding logic using watchReloadEvents, serverEntryRe and rootDirWatcher
unchanged so reload detection works cross-platform.
In `@src/build/vite/dev.ts`:
- Around line 135-146: The handler under nitroEnv.devServer.onMessage for
"__rpc" === "transformHTML" currently catches errors by returning an object ({
error }) into the html variable which then gets sent in data; instead, wrap the
server.transformIndexHtml(...) call in a try/catch and on catch call
nitroEnv.devServer.sendMessage({ __rpc_id: message.__rpc_id, error }) (not put
the error into data) and return early; keep successful responses as
nitroEnv.devServer.sendMessage({ __rpc_id: message.__rpc_id, data: htmlString })
so server.transformIndexHtml, message.__rpc_id and
nitroEnv.devServer.sendMessage are used to locate and implement the fix.
In `@src/dev/server.ts`:
- Around line 151-165: The reload() method currently overwrites `#reloadPromise`
and can start concurrent `#reload`() executions; change reload() to serialize
calls by chaining new reloads onto the existing `#reloadPromise` instead of
replacing it: if `#reloadPromise` exists, set `#reloadPromise` =
`#reloadPromise.then`(() => this.#reload()).finally(() => { this.#reloadPromise =
undefined; }) otherwise start it with this.#reload().finally(...). Ensure
chaining wraps the call to the private async `#reload`() (which calls loadRunner
and this.#manager.reload) so loadRunner/manager.reload runs never overlap and
the latest `#entry/`#workerData and incremented `#workerIdCtr` are applied
deterministically.
In `@src/presets/_nitro/runtime/nitro-dev.ts`:
- Line 2: The current top-level import "import wsAdapter from
\"crossws/adapters/node\"" causes eager evaluation and makes nitro-dev
Node-specific; change it to a runtime-aware approach by removing the static
import and loading the adapter conditionally at runtime (e.g., replace the
static import with a dynamic import when import.meta._websocket is true) or
switch to using "crossws/server" which resolves the correct adapter
automatically; update any usages of wsAdapter in this module (the symbol
wsAdapter and the dev entry initialization) to await the dynamically imported
adapter or use the server entry so non-Node runtimes (Bun, Deno, Cloudflare
Workers) are not broken and unnecessary bundles are avoided.
In `@src/runtime/internal/vite/dev-worker.mjs`:
- Around line 49-53: Unguarded access to Node-only globals causes Worker
runtimes to throw; wrap any use of process (e.g. the ModuleRunner constructor
third arg `process.env.NITRO_DEBUG` and the `process.on(...)` calls around lines
referencing process) with a runtime check like `typeof process !== "undefined"`
and null-safe access to env (e.g. check `process.env &&
process.env.NITRO_DEBUG`) before reading or passing the value, and likewise only
call `process.on(...)` when `typeof process !== "undefined" && typeof process.on
=== "function"`, so ModuleRunner and any event handlers are only wired in Node
environments.
---
Outside diff comments:
In `@src/runtime/internal/error/dev.ts`:
- Around line 1-6: Move Node-specific source-map/ANSI enrichment code out of
src/runtime/internal/error/dev.ts into a separate Node-only helper (e.g.,
devNodeHelpers) that exports functions used for sourcemap lookup and ANSI/color
stripping; remove static imports of "node:fs/promises" and "node:path" and any
direct process.cwd() usage from dev.ts. In dev.ts (where functions like the
HTML/JSON error renderer and any ErrorParser/youch integration live) perform a
runtime check for Node (e.g., typeof process !== "undefined" &&
process.versions?.node) and conditionally dynamic-import the new helper (await
import('./devNodeHelpers')) to call its enrichment functions; if the helper is
unavailable or the runtime check fails, fall back to plain HTML/JSON error
output without ANSI/sourcemap enrichment. Ensure unique symbols referenced in
dev.ts (the error rendering entry, any call sites that previously used fs/path
or process.cwd()) are updated to call the conditional helper functions so
src/runtime/* contains no direct Node APIs.
---
Nitpick comments:
In `@src/build/vite/env.ts`:
- Around line 12-25: initEnvRunner can race because ctx._envRunner is set only
after await runner.waitForReady(); to fix, make the init idempotent by storing
the in-flight initialization before awaiting: when starting in initEnvRunner,
assign a placeholder (either the runner instance immediately after loadRunner or
a promise representing the async init) into ctx._envRunner so concurrent callers
see the same value, then await runner.waitForReady() (or await the stored
promise) and replace/confirm the final runner; update references to loadRunner,
runner.waitForReady, and ctx._envRunner accordingly to ensure only one runner is
created.
In `@src/dev/server.ts`:
- Around line 69-71: Replace the silent .catch on the writeDevBuildInfo call in
this.#manager.onReady so failures are logged: catch the error from
writeDevBuildInfo(this.nitro, addr) and emit a warning including the error
message and context (e.g., "writeDevBuildInfo failed for addr: <addr>") instead
of using .catch(() => {}); reference the writeDevBuildInfo call inside the
this.#manager.onReady handler and use console.warn (or the existing logger if
available) to record the error and addr/nitro context.
In `@src/types/config.ts`:
- Around line 169-174: Add a short JSDoc comment above the devServer.runner
property describing that it accepts values defined by the RunnerName union (list
the allowed runners or reference RunnerName), state the default runner used when
unspecified, and note that the NITRO_DEV_RUNNER environment variable can
override this option; update the docblock to appear immediately above
devServer.runner so IntelliSense shows the expected values, default, and env
override.
In `@src/types/runner.ts`:
- Around line 1-9: The current re-export block in src/types/runner.ts misses the
newly exposed runner-facing types; add RunnerName and EnvRunnerData to the
export list so Nitro's surface exposes them (alongside existing FetchHandler,
EnvRunner, etc.), and update any related JSDoc/comments for
NitroOptions.devServer.runner and NitroHooks["dev:reload"] to reference
RunnerName and EnvRunnerData instead of requiring direct imports from
env-runner.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3401a849-a265-41d2-adbf-e3f0fa3b2f16
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
package.jsonsrc/build/rolldown/dev.tssrc/build/rollup/dev.tssrc/build/vite/dev.tssrc/build/vite/env.tssrc/build/vite/plugin.tssrc/dev/server.tssrc/presets/_nitro/runtime/nitro-dev.tssrc/runner/node.tssrc/runtime/internal/error/dev.tssrc/runtime/internal/vite/dev-worker.mjssrc/runtime/internal/vite/node-runner.mjssrc/types/config.tssrc/types/hooks.tssrc/types/runner.ts
💤 Files with no reviewable changes (2)
- src/runtime/internal/vite/node-runner.mjs
- src/runner/node.ts
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/build/vite/env.ts (1)
6-6: Usepatheinstead ofnode:path.The coding guidelines require using
pathefor cross-platform path operations. Line 6 importsjoinandresolvefromnode:path.♻️ Suggested fix
-import { join, resolve } from "node:path"; +import { join, resolve } from "pathe";As per coding guidelines:
**/*.{js,ts}: Usepathefor cross-platform path operations instead ofnode:path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/build/vite/env.ts` at line 6, The import currently pulls join and resolve from node:path which violates the cross-platform path guideline; replace the node:path import so that join and resolve are imported from the pathe package (i.e., change the import that references join and resolve to import them from "pathe"), and update any usage in this module (functions/variables named join and resolve) to use the pathe-provided implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/build/vite/env.ts`:
- Around line 12-44: The module-level singleton _initPromise in initEnvRunner
captures the first NitroPluginContext and causes subsequent contexts to reuse
that runner; change to a keyed cache (e.g., Map keyed by runnerName or by a
context-specific key) instead of a single _initPromise so each
NitroPluginContext gets its own initialization Promise, ensure the Promise
factory references the current ctx and that ctx._envRunner is assigned on
resolution, and update callers (getEnvRunner / initEnvRunner) to use the keyed
cache so multiple Nitro instances in the same process do not share the wrong
runner.
---
Nitpick comments:
In `@src/build/vite/env.ts`:
- Line 6: The import currently pulls join and resolve from node:path which
violates the cross-platform path guideline; replace the node:path import so that
join and resolve are imported from the pathe package (i.e., change the import
that references join and resolve to import them from "pathe"), and update any
usage in this module (functions/variables named join and resolve) to use the
pathe-provided implementations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 826cfc52-649a-4f8b-9f18-e223a5ed0ff5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
package.jsonsrc/build/rolldown/dev.tssrc/build/rollup/dev.tssrc/build/vite/dev.tssrc/build/vite/env.tssrc/build/vite/types.tssrc/dev/server.tssrc/presets/_nitro/runtime/nitro-dev.tssrc/runtime/internal/vite/dev-worker.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use https://github.com/unjs/env-runner to allow running dev server in any custom runtime.
Works with both Nitro dev server and Vite envs.