feat(nuxi): add q and quit shortcuts to dev server#1378
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe development server now supports TTY-only interactive quitting, emits a typed Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
commit: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1378 +/- ##
=======================================
Coverage ? 46.68%
=======================================
Files ? 52
Lines ? 1752
Branches ? 504
=======================================
Hits ? 818
Misses ? 751
Partials ? 183 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/nuxi/src/dev/index.ts`:
- Around line 150-157: Update the readline quit handler in initialize to invoke
the complete shutdown callback supplied by the command layer instead of the
initializer’s local close(). Ensure the callback remains the public wrapper
returned by the dev command, including cleanupCurrentFork?.(), before
process.exit(0), while preserving the existing quit aliases and exit behavior.
- Around line 136-145: Update the close function so devServer.releaseLock()
always runs in a finally block, even when listener or watcher shutdown rejects.
Ensure shutdown failures propagate to the surrounding finally/error handling and
result in a non-zero process exit status instead of being masked as successful
completion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4a085477-5ae0-4dab-9f96-a35c07b8c020
📒 Files selected for processing (1)
packages/nuxi/src/dev/index.ts
There was a problem hiding this comment.
Pull request overview
Adds interactive quit shortcuts to the nuxi dev server so users can stop the server by typing q or quit (similar to Vite’s behavior), gated behind an interactive TTY check.
Changes:
- Add a TTY-only
readlinelistener that triggers shutdown when the user entersqorquit. - Refactor the dev server shutdown logic into a shared
close()function and ensure the readline interface is closed during shutdown. - Introduce
std-envchecks (hasTTY,isCI) to avoid enabling the shortcut in CI/non-interactive environments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@packages/nuxi/src/dev/utils.ts`:
- Around line 538-541: Update close() and the reload flow around `#load`() so
readline remains available across reloads and the quit handler is re-registered
on each ready event. Separate final-shutdown cleanup from reload cleanup, or
recreate `#rl` before handler registration, while preserving full cleanup when the
dev server actually shuts down.
- Around line 523-524: Guard the quit prompt console.log in the relevant dev
utility flow so it only executes when readline is active. Reuse the existing
readline-active state or check available in the surrounding code, while
preserving the current prompt formatting when a listener is installed.
- Around line 520-521: Bind the `#quitListener` method to the current
NuxtDevServer instance before registering it with `#rl` in the listener setup.
Preserve the existing removeAllListeners('line') behavior and register the bound
callback so its this context invokes NuxtDevServer.close() reliably.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9b9044df-37b4-4e71-a000-de46833ef12f
📒 Files selected for processing (2)
packages/nuxi/src/dev/index.tspackages/nuxi/src/dev/utils.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuxi/src/dev/utils.ts (1)
534-538: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRoute
qthrough the complete shutdown path.This handler calls
NuxtDevServer.close()directly, bypassing the shutdown callback inpackages/nuxi/src/dev/index.tsthat closes watchers and the listener and releases the lock. Additionally,process.exit(0)infinallyconverts any close failure into a successful exit. Use the shared final-shutdown callback and exit only after cleanup succeeds.🤖 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 `@packages/nuxi/src/dev/utils.ts` around lines 534 - 538, Update the `q` handler in `NuxtDevServer` to invoke the shared final-shutdown callback from the dev entrypoint instead of calling `this.close()` directly, ensuring watchers, the listener, and lock are released. Remove the unconditional `process.exit(0)` from `finally`; exit successfully only after the complete shutdown callback resolves, while allowing close failures to propagate.
🤖 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.
Outside diff comments:
In `@packages/nuxi/src/dev/utils.ts`:
- Around line 534-538: Update the `q` handler in `NuxtDevServer` to invoke the
shared final-shutdown callback from the dev entrypoint instead of calling
`this.close()` directly, ensuring watchers, the listener, and lock are released.
Remove the unconditional `process.exit(0)` from `finally`; exit successfully
only after the complete shutdown callback resolves, while allowing close
failures to propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b21f8b3c-0316-4a4a-ae4a-f8d9c3270978
📒 Files selected for processing (1)
packages/nuxi/src/dev/utils.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nuxi/src/dev/utils.ts (1)
517-527: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose readline on every shutdown path.
#rlis only cleaned up inside#quitListener, but the programmatic shutdown path inpackages/nuxi/src/dev/index.tscallsdevServer.close()without closing#rl. Centralize readline cleanup and invoke it fromclose()as well so callers do not leave stdin active and block clean process termination.🤖 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 `@packages/nuxi/src/dev/utils.ts` around lines 517 - 527, Centralize the readline interface cleanup currently performed by `#quitListener` into a dedicated cleanup method, then invoke it from close() before or during shutdown. Preserve the existing quit behavior while ensuring programmatic devServer.close() also removes listeners and closes `#rl` so stdin does not remain active.
🧹 Nitpick comments (1)
packages/nuxi/src/dev/utils.ts (1)
517-536: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd coverage for the interactive lifecycle.
Please cover TTY/non-TTY and CI gating, prompt registration,
q/quit/exithandling, readline cleanup, and the emittedclosingevent. Codecov reports only 33.33% patch coverage for this file.🤖 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 `@packages/nuxi/src/dev/utils.ts` around lines 517 - 536, Add tests covering the interactive lifecycle around the readline setup and `#quitListener` methods: verify TTY/non-CI gating, prompt and line-listener registration, and no setup for non-TTY or CI environments. Exercise q, quit, and exit inputs to confirm readline listeners are removed, the interface is closed and cleared, and closing is emitted.
🤖 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.
Outside diff comments:
In `@packages/nuxi/src/dev/utils.ts`:
- Around line 517-527: Centralize the readline interface cleanup currently
performed by `#quitListener` into a dedicated cleanup method, then invoke it from
close() before or during shutdown. Preserve the existing quit behavior while
ensuring programmatic devServer.close() also removes listeners and closes `#rl` so
stdin does not remain active.
---
Nitpick comments:
In `@packages/nuxi/src/dev/utils.ts`:
- Around line 517-536: Add tests covering the interactive lifecycle around the
readline setup and `#quitListener` methods: verify TTY/non-CI gating, prompt and
line-listener registration, and no setup for non-TTY or CI environments.
Exercise q, quit, and exit inputs to confirm readline listeners are removed, the
interface is closed and cleared, and closing is emitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84280014-3925-4d3c-9bb7-cff0bd28953c
📒 Files selected for processing (2)
packages/nuxi/src/dev/index.tspackages/nuxi/src/dev/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxi/src/dev/index.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
packages/nuxi/src/dev/utils.ts:543
NuxtDevServer.close()does not dispose thereadlineinterface created onready. If consumers callinitialize(...).close()(or a shutdown path other than typingq), the open readline handle can keep stdin in flowing mode and prevent the process from exiting cleanly.
async close(): Promise<void> {
if (this.#currentNuxt) {
await this.#currentNuxt.close()
}
}
packages/nuxi/src/dev/index.ts:125
closeWatchers()is called before the try/finally, so if it throws, the dev lock may not be released. Wrapping it in the same try/finally makes lock release robust even if watcher cleanup errors.
async function close() {
devServer.closeWatchers()
try {
await Promise.all([
devServer.listener.close(),
devServer.close(),
])
}
finally {
devServer.releaseLock()
}
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
packages/nuxi/src/dev/index.ts:126
close()releases the dev lock in afinallyblock, so the lock can be removed even iflistener.close()/devServer.close()fails. In non-exit paths (e.g. restart handling), this can leave a partially running process unlocked and allow concurrent starts, making the lock state unreliable. Releasing the lock only after a successful shutdown keeps the lock consistent with actual server state.
async function close() {
devServer.closeWatchers()
try {
await Promise.all([
devServer.listener.close(),
devServer.close(),
])
}
finally {
devServer.releaseLock()
}
}
packages/nuxi/src/dev/utils.ts:536
- The new interactive quit shortcut (
q/quit/exit) and the newclosingevent are user-visible behavior, but there’s no unit/integration test asserting that enteringqemitsclosingand disposes the readline handler. This module already has vitest unit coverage (e.g.packages/nuxi/test/unit/file-watcher.spec.tsimports fromsrc/dev/utils), so adding a focused test would help prevent regressions (listener leaks, accidental double-close, etc.).
if (!this.#rl && hasTTY && !isCI) {
this.#rl = readline.createInterface({ input: process.stdin })
}
if (this.#rl) {
this.#rl.removeAllListeners('line')
this.#rl.addListener('line', this.#quitListener.bind(this))
// eslint-disable-next-line no-console
console.log(`\n${colors.dim(' press ')}${colors.bold(`q + enter`)}${colors.dim(` to quit`)}\n`)
}
}
#quitListener(line: string) {
if (line === 'q' || line === 'quit' || line === 'exit') {
this.#rl?.removeAllListeners('line')
this.#rl?.close()
this.#rl = undefined
this.emit('closing')
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
packages/nuxi/src/dev/index.ts:125
releaseLock()is executed in afinallyblock, so the lock is removed even iflistener.close()ordevServer.close()rejects. Ifinitialize().close()is used programmatically (e.g. during restarts) and shutdown fails, releasing the lock can allow concurrent starts while the previous server is still partially running. Consider only releasing the lock after a successful shutdown (and let the processexithook handle cleanup on hard termination).
finally {
devServer.releaseLock()
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/nuxi/test/unit/initialize.spec.ts (1)
85-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the failure/exit-code branch.
This test only asserts
exitSpywas called, but doesn't verifyprocess.exitCode. The source setsexitCode = 0on success andexitCode = 1ifclose()rejects — neither branch's exit code is asserted here, and the failure path (close()throwing) isn't exercised at all.expect(onBeforeQuit).toHaveBeenCalledWith(instance) expect(exitSpy).toHaveBeenCalled() + expect(process.exitCode).toBe(0) } finally { exitSpy.mockRestore() } }) + + it('sets exitCode to 1 and still exits when close() rejects during shutdown', async () => { + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never) + try { + const result = await initialize(baseDevContext()) + const instance = vi.mocked(NuxtDevServer).mock.results[0]!.value + instance.close.mockRejectedValueOnce(new Error('boom')) + + instance.emit('closing') + await vi.waitFor(() => expect(exitSpy).toHaveBeenCalled()) + + expect(process.exitCode).toBe(1) + } + finally { + exitSpy.mockRestore() + } + })🤖 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 `@packages/nuxi/test/unit/initialize.spec.ts` around lines 85 - 106, The shutdown test around initialize and the “closing” handler needs coverage for both exit-code outcomes. Assert process.exitCode is 0 after a successful close, then add a failure-path test where instance.close rejects and verify process.exitCode is 1 while preserving the existing onBeforeQuit and process.exit assertions.
🤖 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 `@packages/nuxi/src/commands/dev.ts`:
- Around line 140-147: Update the onRestart callback in initialize so
initializeOptions.onBeforeQuit delegates to the current cleanupCurrentFork
binding rather than capturing its value once. Preserve the existing temporary
disable during close() and ensure later IPC-triggered fork restarts invoke
cleanupCurrentFork for the active fork when quitting.
---
Nitpick comments:
In `@packages/nuxi/test/unit/initialize.spec.ts`:
- Around line 85-106: The shutdown test around initialize and the “closing”
handler needs coverage for both exit-code outcomes. Assert process.exitCode is 0
after a successful close, then add a failure-path test where instance.close
rejects and verify process.exitCode is 1 while preserving the existing
onBeforeQuit and process.exit assertions.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c24c454-29b1-4b6d-b0cf-14dd8801aaf4
📒 Files selected for processing (4)
packages/nuxi/src/commands/dev.tspackages/nuxi/src/dev/index.tspackages/nuxi/src/dev/utils.tspackages/nuxi/test/unit/initialize.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nuxi/src/dev/index.ts
- packages/nuxi/src/dev/utils.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
packages/nuxi/src/dev/index.ts:119
releaseLock()is called in afinallyblock, which removes the lock file (and unregisters the processexitcleanup) even iflistener.close()/devServer.close()fails. If shutdown is only partially successful and the process continues (e.g. during restarts), this can leave the dev server running while unlocked, allowing concurrent starts and making the lock state inconsistent. Release the lock only after a successful shutdown completes.
async function close() {
devServer.closeWatchers()
try {
await Promise.all([
devServer.listener.close(),
packages/nuxi/src/commands/dev.ts:144
initializeOptions.onBeforeQuitis assigned to the currentcleanupCurrentForkfunction by value. IfrestartWithFork()runs again later (it reassignscleanupCurrentFork), the stored hook will still point at the old fork’s cleanup function, so quitting can leave the active fork running. Use a wrapper that calls the latestcleanupCurrentForkat invocation time.
onRestart(async () => {
// Temporarily disable the quit hook during restart to avoid double-cleanup
Object.assign(initializeOptions, { onBeforeQuit: undefined })
// Close the in-process dev server
await close()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/nuxi/src/dev/index.ts:125
releaseLock()is executed in afinally, so the lock file can be released even whenlistener.close()ordevServer.close()fails (e.g. during an in-process restart where the process continues running). That can leave a still-running/partially-running server without a lock, enabling concurrent starts and making the lock state inconsistent.
async function close() {
devServer.closeWatchers()
try {
await Promise.all([
devServer.listener.close(),
devServer.close(),
])
}
finally {
devServer.releaseLock()
}
packages/nuxi/src/dev/utils.ts:536
- There’s currently no test that verifies the interactive quit shortcut behavior (typing
q/quitcauses the readline listener to be detached and emitsclosing). The newinitializeunit test covers the downstreamclosinghandler, but it doesn’t protect the stdin/readline path against regressions or listener leaks.
#quitListener(line: string) {
if (line === 'q' || line === 'quit' || line === 'exit') {
this.#rl?.removeAllListeners('line')
this.#rl?.close()
this.#rl = undefined
this.emit('closing')
}
Thanks for flagging this. I looked into adding a dedicated test for the Given the low-confidence flag, I'd rather not bolt a heavy integration test onto this PR for that. If we want solid coverage of the interactive quit path (detach on |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
packages/nuxi/src/dev/index.ts:126
releaseLock()is currently executed in afinallyblock, so it will run even iflistener.close()ordevServer.close()fails. That can remove the lock while the dev server is still partially running, allowing concurrent starts and making the lock state inconsistent. Release the lock only after a successful shutdown; if shutdown fails, keep the lock (theprocess.on('exit')handler installed byacquireLock()will still clean up on actual process exit).
async function close() {
devServer.closeWatchers()
try {
await Promise.all([
devServer.listener.close(),
devServer.close(),
])
}
finally {
devServer.releaseLock()
}
}
packages/nuxi/src/dev/utils.ts:536
- The new quit shortcut installs a stdin
readlinelistener and emits a newclosingevent, but there is still no test that validates the user-facing behavior (i.e., that enteringq/quiton stdin triggersclosingand that the readline listener is detached). The newinitialize.spec.tstests theclosingevent handler, but it doesn't cover the readline-driven path introduced here.
if (!this.#rl && hasTTY && !isCI) {
this.#rl = readline.createInterface({ input: process.stdin })
}
if (this.#rl) {
this.#rl.removeAllListeners('line')
this.#rl.addListener('line', this.#quitListener.bind(this))
// eslint-disable-next-line no-console
console.log(`\n${colors.dim(' press ')}${colors.bold(`q + enter`)}${colors.dim(` to quit`)}\n`)
}
}
#quitListener(line: string) {
if (line === 'q' || line === 'quit' || line === 'exit') {
this.#rl?.removeAllListeners('line')
this.#rl?.close()
this.#rl = undefined
this.emit('closing')
}
| onRestart(async () => { | ||
| // Temporarily disable the quit hook during restart to avoid double-cleanup | ||
| Object.assign(initializeOptions, { onBeforeQuit: undefined }) | ||
| // Close the in-process dev server | ||
| await close() | ||
| await restartWithFork() | ||
| // Delegate to the live `cleanupCurrentFork` binding so later fork | ||
| // restarts (triggered via IPC) are always cleaned up correctly on quit | ||
| Object.assign(initializeOptions, { onBeforeQuit: () => cleanupCurrentFork?.() }) | ||
| }) |
There was a problem hiding this comment.
why not just make onRestart callback void | Promise<void> ? if so we can also change onBeforeQuit, but not sure if we should await
/cc @danielroe
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
packages/nuxi/src/dev/index.ts:125
close()always callsdevServer.releaseLock()in afinallyblock, so the lock is released even whenlistener.close()/devServer.close()rejects. This can leave the process still running (e.g. during restart flows) without a lock, and it also disables the lock'sprocess.on('exit')cleanup handler while the server may still be active. Release the lock only after a successful shutdown.
finally {
devServer.releaseLock()
}
🔗 Linked issue
📚 Description
This PR adds the same shortcut at Vite where pressing
q+enterstops the server.