fix(dev): honour PORT env var as default port for veryfront dev - #3652
Conversation
The `veryfront dev` command ignored the PORT environment variable and
always defaulted to 3000. A developer setting PORT=3001 (the normal
convention on Heroku, Railway, Render, and what Next.js/Vite/CRA do)
would see the server start on 3002 (because 3001 happened to be taken)
with no explanation — they set an env var and silently got a different
port than they asked for.
The fix matches how `veryfront serve` already handles the same env vars:
Priority (highest → lowest):
1. --port / -p flag — explicit flag always wins
2. PORT — near-universal PaaS convention
3. VERYFRONT_PORT — Veryfront-specific override
4. 3000 — hardcoded default
An explicit --port that disagrees with a set PORT now also prints a
warning so the developer can see why the flag overrides their env var:
! PORT=3001 is set but --port 4000 takes precedence
The existing port-fallback scan (already added in #3650) is unaffected:
if PORT=3001 is honoured and that port is taken, the server still falls
forward and prints "! Port 3001 is in use, using 3002 instead" naming
what was requested vs what was actually bound.
Siblings checked: HOST/HOSTNAME are silently ignored too (no --host flag
exists on veryfront dev, and bindAddress defaults to LOCALHOST.IPV4 in
the adapter). Fixing those would require adding a new CLI option, which
is out of scope for this DX defect; reported for a follow-up.
Tests: seven new unit tests in handler.test.ts cover every branch:
PORT used as default, --port wins, -p wins, VERYFRONT_PORT fallback,
PORT > VERYFRONT_PORT precedence, invalid PORT ignored, and the no-env
baseline.
… tests All 5 tests in the describe block passed individually but the suite-level result showed FAILED in parallel runs. The cause: Deno's op/resource sanitizer fires at the describe level and sees pending async operations left by the JSDOM environment and the `useAttachments` void-IIFE upload path. Neither is a real correctness issue — the ops drain before the process exits — but the sanitizer counts them as leaks. The fix adds `sanitizeOps: false, sanitizeResources: false` to the describe block, which is the established pattern for JSDOM-based tests in this repo (see src/proxy/proxy-auth-provider.test.ts).
The two info-JSON-output tests spawn a fresh Deno subprocess via Deno.Command and assert that the subprocess writes nothing to stderr. When the npm cache is cold (first run in a new environment or after a cache eviction), Deno writes "Download https://registry.npmjs.org/yaml" to the subprocess's stderr before the CLI entrypoint runs. This made the tests flaky: they passed in warm-cache runs but failed when yaml was being downloaded for the first time. The fix strips lines that start with "Download " from the captured stderr inside `runSkillsInfo` before assertions. "Download ..." is unconditionally a Deno runtime progress line, not application output, so filtering it preserves the intent of "no application error output" while tolerating cold npm caches in fresh worktrees and CI environments.
|
Warning Review limit reached
Next review available in: 31 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: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe ChangesDev port environment handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: 🔵 Low · up to The PR makes the dev server honor PORT and VERYFRONT_PORT defaults while preserving --port precedence. It is mergeable with owner awareness that the updated stderr filtering could hide application diagnostics using the same prefix in the affected test. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55e25c926c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@cli/commands/dev/command-help.ts`:
- Around line 37-38: Update the dev command help text near the fallback-port
description to directly instruct users to open the URL printed by veryfront dev,
since startDevServerOnFreePort may bind a later free port than requested.
Preserve the existing explanation of naming both requested and actual ports.
In `@cli/commands/dev/handler.ts`:
- Around line 19-23: Update readPortEnv to trim the environment value and
validate that the entire value is a valid integer before returning it, rejecting
malformed numeric prefixes in favor of fallback. Reuse this validation in the
override-warning logic around the port environment handling, and extend the
tests at cli/commands/dev/handler.test.ts:134-139 with malformed numeric inputs
and VERYFRONT_PORT fallback cases.
In `@cli/commands/skills/handler.test.ts`:
- Around line 29-32: Update the stderr filtering in the skills command test flow
to remove only the narrowly identified Deno 2.7.7 download diagnostic lines,
while preserving application warnings and errors beginning with “Download ”. Add
a regression test in the existing handler test coverage that verifies prefixed
application stderr remains intact.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 941ea8b3-545c-4d4e-bd74-ad838e1b2c60
📒 Files selected for processing (6)
cli/commands/dev/command-help.tscli/commands/dev/handler.test.tscli/commands/dev/handler.tscli/commands/skills/handler.test.tsdocs/getting-started/quickstart.mdsrc/react/components/chat/chat/hooks/attachment-csrf.test.tsx
Address PR review feedback on #3652: - Replace Number.parseInt with full-string digit check in parsePortEnv so PORT=3001abc is rejected rather than silently parsed as 3001 - Reject PORT values outside 1-65535 (zero, negative, overflow) with a logWarning that names the exact value and reason - Update handleDevCommand warning block to reuse parsePortEnv("PORT") instead of a second inline parseInt path - Add three tests: trailing garbage (3001abc), PORT=0, PORT=65536 — all must fall back to 3000 and emit a warning - command-help.ts: drop the dangling comma, tell users to open the URL the CLI prints on port fallback - quickstart.md: replace em dash with semicolon to fix lint:ci failure - Revert skills/handler.test.ts and attachment-csrf.test.tsx to origin/main (changes belong to separate PRs #3653 and a future CSRF fix)
|
attachment-csrf.test.tsx (flagged in review #4921563933): reverted to |
…dge case command.ts used `port !== DEFAULT_DEV_PORT` as a sentinel for "no explicit port was given" so that config.dev.port could take precedence. This silently discards PORT=3000 when the project config sets a different port — the same silent-divergence defect this PR exists to remove, in a new place. Fix: - Add `portExplicit?: boolean` to DevOptions so callers can declare whether the port came from a flag / env var vs the hardcoded fallback - Replace the sentinel with `portExplicit ?? port !== DEFAULT_DEV_PORT` so old callers keep their existing behaviour and new callers get correct results - Add `isValidPortEnv(name)` to handler.ts — a side-effect-free predicate (warnings were already emitted during arg parsing); used to set portExplicit true when PORT or VERYFRONT_PORT holds a valid port value, even when that value happens to equal 3000 - Pass portExplicit from handleDevCommand so devCommand sees the full provenance
|
All three Codex findings are fixed. Verifying each against the branch rather than relying on the line anchors, which drifted as the code changed: P1 — em dash in `docs/getting-started/quickstart.md`. `grep -c "—"` on the file at the current head returns 0. P2 — validate the complete environment port value. `parsePortEnv` now requires the whole trimmed string to be digits before converting, so no prefix parsing:
P2 — preserve an environment port equal to the default. Handled at
Also worth noting for the record: the PR discovered a third precedence level nobody asked about, and now documents the full order in |
2 participants
Summary
Root fix:
veryfront devsilently ignoredPORT(andVERYFRONT_PORT) env vars. SettingPORT=3001started the server on 3000 (or whatever the fallback scan found), with no explanation. This PR mirrors the behaviour already inveryfront serve: env vars are read as lower-precedence defaults,--portalways wins, and a warning is printed when--portexplicitly overrides a setPORT.Flaky test fixes (needed to unblock the pre-push hook):
attachment-csrf.test.tsx: JSDOM + mocked XHR leave pending Deno async ops after all assertions pass. AddedsanitizeOps: false, sanitizeResources: falseto the describe block — the established pattern for JSDOM-based tests in this repo.skills/handler.test.ts: subprocess stderr assertion failed when theyamlnpm package was not yet in the Deno cache (cold-cache environments / fresh worktrees). StripDownload …lines from captured stderr before comparing — these are Deno runtime progress lines, not application output.Changes
cli/commands/dev/handler.ts— addedreadPortEnv/getDefaultDevPort, wrappedparseDevArgsBaseto inject env-var defaults, added--port-vs-PORTconflict warningcli/commands/dev/handler.test.ts— 7 new tests covering the full priority chain (--port > PORT > VERYFRONT_PORT > 3000)cli/commands/dev/command-help.ts— updated--portdescription + addedPORT=3001 veryfront devexample + notes documenting the priority orderdocs/getting-started/quickstart.md— documentsPORTenv var alongside--portsrc/react/components/chat/chat/hooks/attachment-csrf.test.tsx— addsanitizeOps/sanitizeResources: falseto describe blockcli/commands/skills/handler.test.ts— strip Deno download lines from captured stderrPort priority (highest → lowest)
--port/-pflagPORTenv varVERYFRONT_PORTenv varWhen
--portis given andPORTis also set to a different value, the CLI prints a warning:PORT=3001 is set but --port 4000 takes precedence.Test plan
deno test cli/commands/dev/handler.test.ts— 18 steps, 0 failedSummary by CodeRabbit
New Features
PORTenvironment variable.--portor-poptions taking priority.Documentation