Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe changes add portless proxy support to the nuxt dev command. A new utility module ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nuxi/src/commands/dev.ts`:
- Around line 104-128: If registerPortless or listener.showURL fails we
currently restore PORTLESS_URL and call close() but never remove a previously
created alias; modify the catch block around registerPortless/listener.showURL
to call removePortlessAlias(cwd, portlessName!) (awaiting it and ignoring
errors) before restoring process.env.PORTLESS_URL and rethrowing; ensure the
existing closePortless assignment (which also calls removePortlessAlias) remains
for the success path and reference registerPortless, closePortless,
removePortlessAlias, listener.showURL, close, and previousPortlessURL when
implementing the cleanup.
🪄 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
Run ID: 7899ac77-5dc4-4f1f-bd7e-6cf32c2fc6e5
📒 Files selected for processing (4)
packages/nuxi/src/commands/dev.tspackages/nuxi/src/dev/portless.tspackages/nuxi/test/unit/dev/portless.spec.tspackages/nuxt-cli/test/e2e/dev.spec.ts
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nuxt-cli/test/e2e/dev.spec.ts`:
- Around line 99-107: The extractLoggedURLs function returns raw matches that
may include formatting artifacts or trailing slashes causing flaky CI
assertions; update extractLoggedURLs to normalize each URL before returning by
trimming whitespace, stripping ANSI escape sequences, removing surrounding
punctuation (like trailing ')' or '.'), and trimming a single trailing slash
(and optionally lowercasing) so comparisons are consistent; keep the function
name extractLoggedURLs and apply normalization to every match returned from
value.match(...) so tests comparing those URLs are stable.
- Around line 173-196: The test currently calls await close() inside the try
block so a thrown assertion can leak the spawned dev process; refactor to
capture the close function from runCommand into an outer-scoped variable (e.g.,
let close: (() => Promise<void>) | undefined), assign it when runCommand
resolves, and then in the finally block await close() if defined before
restoring consoleLog.mockRestore() and removing binDir; reference runCommand and
the close function to locate where to move the cleanup.
🪄 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
Run ID: 3cb523d0-edc2-49db-b161-68f2d2e7db57
📒 Files selected for processing (4)
packages/nuxi/src/commands/dev.tspackages/nuxi/src/dev/portless.tspackages/nuxi/test/unit/commands/dev.spec.tspackages/nuxt-cli/test/e2e/dev.spec.ts
✅ Files skipped from review due to trivial changes (1)
- packages/nuxi/src/dev/portless.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxi/src/commands/dev.ts
onmax
changed the title
onmax
force-pushed
the
feat/nuxi-portless
branch
2 times, most recently
from
ac9337f to
54f8395
Compare
onmax
force-pushed
the
feat/nuxi-portless
branch
from
54f8395 to
eb6d9ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/commands/dev.ts (1)
188-195:⚠️ Potential issue | 🟠 MajorRemove the redundant
listener.close()call—it's already closed by theclose()function.The
close()function returned frominitialize()already callslistener.close()(viadevServer.listener.close()). Calling it again in thePromise.allcreates a double-close race condition on shutdown. Remove line 192.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/nuxi/src/commands/dev.ts` around lines 188 - 195, The async close method is calling listener.close() directly and also calling the close() function returned from initialize() which already calls devServer.listener.close(), causing a double-close race; edit the async close implementation to remove the explicit listener.close() entry from the Promise.all so it only awaits cleanupCurrentFork, closePortless and the single close() function (keep references to cleanupCurrentFork, closePortless and close() as they are).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/nuxi/src/dev/portless.ts`:
- Around line 118-123: readNameFromFile currently swallows all errors (parse,
permission, etc.) and should only ignore missing files; update the
readNameFromFile function so that its promise rejection handler returns
undefined only when the error indicates a missing file (error.code === 'ENOENT'
or equivalent), and rethrow any other errors (so JSON.parse and permission
errors bubble up); keep the existing JSON.parse and type-check logic but replace
the blanket .catch(() => undefined) with conditional error handling that
references readNameFromFile.
---
Outside diff comments:
In `@packages/nuxi/src/commands/dev.ts`:
- Around line 188-195: The async close method is calling listener.close()
directly and also calling the close() function returned from initialize() which
already calls devServer.listener.close(), causing a double-close race; edit the
async close implementation to remove the explicit listener.close() entry from
the Promise.all so it only awaits cleanupCurrentFork, closePortless and the
single close() function (keep references to cleanupCurrentFork, closePortless
and close() as they are).
🪄 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
Run ID: a14b92ee-85da-41ef-b815-8130d6122c44
📒 Files selected for processing (5)
packages/nuxi/src/commands/dev.tspackages/nuxi/src/dev/portless.tspackages/nuxi/test/unit/dev/portless-exit.spec.tspackages/nuxi/test/unit/dev/portless.spec.tspackages/nuxt-cli/test/e2e/dev.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/nuxt-cli/test/e2e/dev.spec.ts
- packages/nuxi/test/unit/dev/portless.spec.ts
| function readNameFromFile(cwd: string, filename: string) { | ||
| return readFile(join(cwd, filename), 'utf8') | ||
| .then(contents => JSON.parse(contents)) | ||
| .then(config => typeof config.name === 'string' ? config.name : undefined) | ||
| .catch(() => undefined) | ||
| } |
There was a problem hiding this comment.
Only ignore missing files when resolving the Portless name.
readNameFromFile() currently swallows parse and permission errors as well as missing-file cases. A malformed portless.json/package.json will silently fall back to basename(cwd), which can expose the dev server under the wrong Portless hostname.
Suggested fix
function readNameFromFile(cwd: string, filename: string) {
return readFile(join(cwd, filename), 'utf8')
.then(contents => JSON.parse(contents))
.then(config => typeof config.name === 'string' ? config.name : undefined)
- .catch(() => undefined)
+ .catch((error) => {
+ if (error instanceof Error && 'code' in error && error.code === 'ENOENT') {
+ return undefined
+ }
+ throw error
+ })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/nuxi/src/dev/portless.ts` around lines 118 - 123, readNameFromFile
currently swallows all errors (parse, permission, etc.) and should only ignore
missing files; update the readNameFromFile function so that its promise
rejection handler returns undefined only when the error indicates a missing file
(error.code === 'ENOENT' or equivalent), and rethrow any other errors (so
JSON.parse and permission errors bubble up); keep the existing JSON.parse and
type-check logic but replace the blanket .catch(() => undefined) with
conditional error handling that references readNameFromFile.
2 participants
Adds
nuxi dev --portless, precomputes the Portless hostname so Nuxt/Vite allowlists include it, and exposes the dev server through the external Portless CLI.