feat(dev): take over a dev server already running on this project - #1414
feat(dev): take over a dev server already running on this project #1414danielroe wants to merge 17 commits into
Conversation
commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CLI adds Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/nuxt-cli/src/utils/lockfile.ts (1)
130-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTreat
NUXT_IGNORE_LOCK=0andNUXT_IGNORE_LOCK=falseas "do not ignore".
NUXT_IGNORE_LOCKuses presence semantics, andNUXT_LOCKuses value semantics. A user who setsNUXT_IGNORE_LOCK=0to keep locking enabled disables it instead. Align both variables on value semantics.♻️ Proposed change
export function isLockEnabled(): boolean { - return !process.env.NUXT_IGNORE_LOCK && process.env.NUXT_LOCK !== '0' && process.env.NUXT_LOCK !== 'false' + const ignore = process.env.NUXT_IGNORE_LOCK + if (ignore && ignore !== '0' && ignore !== 'false') { + return false + } + return process.env.NUXT_LOCK !== '0' && process.env.NUXT_LOCK !== 'false' }🤖 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/nuxt-cli/src/utils/lockfile.ts` around lines 130 - 136, Update isLockEnabled so NUXT_IGNORE_LOCK uses value semantics like NUXT_LOCK: values "0" and "false" must mean the lock is not ignored, while other set values continue disabling the lock. Preserve the existing NUXT_LOCK checks and default locking behavior.packages/nuxt-cli/test/unit/takeover.spec.ts (1)
206-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the spinner branch of
startProgress.Vitest runs without a TTY on
stdout, so these tests always take the plain-log branch ofstartProgress. The spinner branch, including the failure path, stays uncovered. Setprocess.stdout.isTTYtotruein one timeout test to exercise it. This also validates the spinner method concern raised onpackages/nuxt-cli/src/dev/takeover.tslines 179-189.🤖 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/nuxt-cli/test/unit/takeover.spec.ts` around lines 206 - 212, Update the timeout test around takeOverDevServer to temporarily set process.stdout.isTTY to true, exercising the spinner branch of startProgress and its failure handling while preserving the existing timeout assertions. Restore the original TTY value after the test so other tests remain unaffected.packages/nuxt-cli/src/dev/takeover.ts (1)
248-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare
isProcessAlivewithlockfile.ts.
isProcessAliveduplicates the identical helper frompackages/nuxt-cli/src/utils/lockfile.tsand repeats theEPERMliveness policy. Export it fromlockfile.tsand import it intakeover.tsso only one helper manages process liveness checks.🤖 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/nuxt-cli/src/dev/takeover.ts` around lines 248 - 260, Remove the local isProcessAlive helper from takeover.ts and import the shared implementation from lockfile.ts. Export isProcessAlive from lockfile.ts, preserving its existing process.kill and EPERM behavior, so both modules use the single liveness-check helper.
🤖 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/nuxt-cli/src/dev/takeover.ts`:
- Around line 155-156: Update the timeout failure path in performTakeover to
clear the takeover annotation after the holder survives and before returning the
refused result. Reset the existing takenOverBy state using the same lock/state
mechanism as markTakenOver, while preserving the current progress failure
message and return values.
In `@packages/nuxt-cli/src/dev/utils.ts`:
- Around line 82-89: The devForkParentPid function returns process.ppid without
validating whether it represents an actual supervisor process. When a supervisor
exits before the fork, the fork gets reparented to PID 1 (or equivalent init
process), and returning that PID could cause performTakeover to signal the init
system instead of the real supervisor. Add a guard condition that checks
process.ppid is not equal to 1 before returning it, ensuring only real
supervisor PIDs are recorded and preventing accidental signals to init
processes.
---
Nitpick comments:
In `@packages/nuxt-cli/src/dev/takeover.ts`:
- Around line 248-260: Remove the local isProcessAlive helper from takeover.ts
and import the shared implementation from lockfile.ts. Export isProcessAlive
from lockfile.ts, preserving its existing process.kill and EPERM behavior, so
both modules use the single liveness-check helper.
In `@packages/nuxt-cli/src/utils/lockfile.ts`:
- Around line 130-136: Update isLockEnabled so NUXT_IGNORE_LOCK uses value
semantics like NUXT_LOCK: values "0" and "false" must mean the lock is not
ignored, while other set values continue disabling the lock. Preserve the
existing NUXT_LOCK checks and default locking behavior.
In `@packages/nuxt-cli/test/unit/takeover.spec.ts`:
- Around line 206-212: Update the timeout test around takeOverDevServer to
temporarily set process.stdout.isTTY to true, exercising the spinner branch of
startProgress and its failure handling while preserving the existing timeout
assertions. Restore the original TTY value after the test so other tests remain
unaffected.
🪄 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: 5b11aef0-5576-4df5-a064-176b57d0b08b
📒 Files selected for processing (9)
packages/nuxt-cli/src/commands/dev.tspackages/nuxt-cli/src/commands/prepare.tspackages/nuxt-cli/src/dev/takeover.tspackages/nuxt-cli/src/dev/utils.tspackages/nuxt-cli/src/utils/lockfile.tspackages/nuxt-cli/test/unit/commands/prepare.spec.tspackages/nuxt-cli/test/unit/help.spec.tspackages/nuxt-cli/test/unit/lockfile.spec.tspackages/nuxt-cli/test/unit/takeover.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/nuxt-cli/src/commands/build.ts (2)
89-111: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOutput-lock conflict message hardcodes "build" instead of using the actual owning command.
At line 95, the build-lock conflict message uses
lock.existing.commandto correctly name the conflicting process. At line 108, the output-lock conflict message hardcodes the literal word"build"instead of referencingoutputLock.existing.command.formatLockError(called just above) already derives its label frominfo.command, so the thrownErrortext should follow the same pattern for consistency and to avoid misreporting the conflicting process type if a non-build command ever holds this output lock.🐛 Proposed fix
if (outputLock.existing) { logger.error(formatLockError(outputLock.existing)) - throw new Error(`Another Nuxt build is already writing to ${relative(process.cwd(), nitro.options.output.dir)} (PID ${outputLock.existing.pid}).`) + throw new Error(`Another Nuxt ${outputLock.existing.command} process is already writing to ${relative(process.cwd(), nitro.options.output.dir)} (PID ${outputLock.existing.pid}).`) }🤖 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/nuxt-cli/src/commands/build.ts` around lines 89 - 111, Update the output-lock conflict error in the acquireOutputLock block to use outputLock.existing.command instead of the hardcoded “build” label, matching the existing lock conflict handling and accurately reporting the owning command.
97-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd test coverage for the new output-lock acquisition and cleanup paths.
This segment introduces a new conflict path (output-directory lock) and extends the release logic to an array of callbacks.
lockfile.spec.tsandtakeover.spec.tscover the underlying lock primitives, but no test file is provided forbuild.tsitself to verify that: the output lock is acquired after the build lock, a conflict throws and logs correctly, and both locks are released in thefinallyblock on success and on failure.🤖 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/nuxt-cli/src/commands/build.ts` around lines 97 - 111, Add build-command tests covering the lock lifecycle around kit.useNitro: verify the output lock is acquired after the build lock, conflicts log formatLockError and throw, and release callbacks for both locks run in the finally path on both successful and failed builds.
🤖 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/nuxt-cli/src/utils/lockfile.ts`:
- Around line 95-103: Update getTakeoverPid to explicitly guard against a
missing current lock before evaluating current.takenOverBy, combining the
!current check with the existing PID and takeover checks so all subsequent
accesses are narrowed to LockInfo.
---
Nitpick comments:
In `@packages/nuxt-cli/src/commands/build.ts`:
- Around line 89-111: Update the output-lock conflict error in the
acquireOutputLock block to use outputLock.existing.command instead of the
hardcoded “build” label, matching the existing lock conflict handling and
accurately reporting the owning command.
- Around line 97-111: Add build-command tests covering the lock lifecycle around
kit.useNitro: verify the output lock is acquired after the build lock, conflicts
log formatLockError and throw, and release callbacks for both locks run in the
finally path on both successful and failed builds.
🪄 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: 09916f8e-dac1-4b89-8d27-c45bc309cbf5
📒 Files selected for processing (6)
packages/nuxt-cli/src/commands/build.tspackages/nuxt-cli/src/dev/takeover.tspackages/nuxt-cli/src/dev/utils.tspackages/nuxt-cli/src/utils/lockfile.tspackages/nuxt-cli/test/unit/lockfile.spec.tspackages/nuxt-cli/test/unit/takeover.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nuxt-cli/test/unit/takeover.spec.ts
- packages/nuxt-cli/src/dev/utils.ts
3642a1f to
cb24d28
Compare
Co-authored-by: Harlan Wilton <harlan@harlanzw.com>
Co-authored-by: Harlan Wilton <harlan@harlanzw.com>
7aa44a4 to
06d47a7
Compare
🔗 Linked issue
closes #1337 (and incorporates some of it ❤️)
📚 Description
this PR does a number of things. it enables the lock file (#1265) whether or not we're using an agent, and additionally records whether or not the dev server was started in an interactive console.
Video preview
Screen.Recording.2026-07-31.at.22.49.56.mov
we then are able to protect corruption of
.nuxt/and.output/with the following rules:.nuxt/dir existedif you start a second dev process, it's now possible for it to take over the existing port, in certain conditions (based on interactivity):
takeover1, prints where the other server is and that--takeoverforces itdon't startthis is designed to allow agents to take over easily, but not kill a dev server that a person is operating. and also to handle the 'too many terminals' syndrome that I have 😭
takeover is additionally restricted by the following conditions:
before taking over, we write the pid of the new process into the lockfile (so the outgoing dev server can blame why it's going silently into the dark night) and then kill it, first politely and then non-negotiably. but if that fails, we give an informative error and die.