[chore] Make dev server ports configurable via PORT env var#2549
Conversation
📝 WalkthroughWalkthroughThe PR makes development server ports configurable by updating the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
@claude review |
|
Claude finished @Will-Howard's task —— View job Code Review: Make dev server ports configurableTodo List:
✅ Overall AssessmentThis is a clean, well-implemented change that solves a real developer pain point. The solution is minimal, portable, and maintains backward compatibility perfectly. 📋 Changes ReviewedFiles Modified:
✅ Strengths
🔍 Technical AnalysisShell Parameter Expansion:
Existing Patterns:
📝 Minor Considerations
🎯 Recommendation✅ APPROVE - This change is ready to merge. It's a targeted fix that:
The broader question of applying this pattern to other apps can be addressed separately if needed. |
Greptile SummaryThis PR makes the dev-server port configurable via a
Confidence Score: 4/5Safe to merge for Unix/macOS/Linux environments; the change is minimal and preserves existing defaults. Both changes are small and correct on Unix-like systems. The ${PORT:-default} syntax is not understood by Windows cmd.exe, so developers running npm run start natively on Windows would see an invalid-port error. This is a real but narrowly-scoped limitation for a dev-only script in a Docker-centric project. No production behaviour is affected. No files require special attention beyond the noted Windows shell-syntax concern in both package.json scripts. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[npm run start] --> B{PORT env var set?}
B -- Yes --> C[Use PORT value]
B -- No --> D[Use default port]
D --> E1[website: 8000]
D --> E2[storybook: 6006]
C --> F[Start dev server on specified port]
E1 --> F
E2 --> F
F --> G{OS?}
G -- Unix/macOS/Linux --> H[Works correctly]
G -- Windows cmd.exe --> I[Fails: literal string passed as port]
Reviews (1): Last reviewed commit: "[chore] Make dev server ports configurab..." | Re-trigger Greptile |
| "scripts": { | ||
| "postinstall": "shx cp -n .env.local.template .env.local", | ||
| "start": "next dev -p 8000", | ||
| "start": "next dev -p ${PORT:-8000}", |
There was a problem hiding this comment.
Windows incompatibility with
${PORT:-…} syntax
${PORT:-8000} is POSIX shell (bash/sh) parameter expansion. On Windows, npm scripts run through cmd.exe by default, which does not understand this syntax — it would be passed literally to next, resulting in an invalid port error. If any developer on the team uses Windows to run npm run start directly (outside Docker), the command will fail. A cross-platform alternative is to use cross-env with a wrapper script, or rely on Next.js's built-in PORT env-var support via next dev without -p and a documented default in .env.local.template. The same issue applies to apps/storybook/package.json.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/storybook/package.json (1)
6-6: 💤 Low valueBe aware: shell parameter expansion may not work on Windows.
The
${PORT:-6006}syntax is bash/sh-specific and won't work in Windows cmd.exe or PowerShell (it will work in Git Bash or WSL). If Windows developers are part of your target audience, consider usingcross-envor a similar cross-platform solution.🤖 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 `@apps/storybook/package.json` at line 6, The start script in package.json uses shell-specific parameter expansion ("start": "storybook dev -p ${PORT:-6006}") which fails on Windows; update the "start" script to use a cross-platform approach (e.g., use cross-env or npm package config) so the PORT fallback works on all OSes—locate the "start" script entry in package.json and replace the bash-style "${PORT:-6006}" usage with a cross-platform pattern (e.g., read process.env.PORT via cross-env or an npm script that sets PORT) ensuring Storybook still listens on the fallback 6006 when PORT is unset.apps/website/package.json (2)
7-7: 💤 Low valueBe aware: shell parameter expansion may not work on Windows.
The
${PORT:-8000}syntax is bash/sh-specific and won't work in Windows cmd.exe or PowerShell (it will work in Git Bash or WSL). If Windows developers are part of your target audience, consider usingcross-envor a similar cross-platform solution.🤖 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 `@apps/website/package.json` at line 7, The "start" script uses bash-only parameter expansion ("${PORT:-8000}") which breaks on Windows; replace it with a cross-platform approach by creating a small Node launcher (e.g., scripts/start.js) that reads process.env.PORT || 8000 and spawns "next dev -p <port>" (or sets process.env.PORT and uses require('child_process').spawn to run "next"), then update the package.json "start" script to "node ./scripts/start.js"; alternatively, install and use cross-env/cross-env-shell and adjust the "start" script accordingly if you prefer that dependency.
7-7: ⚖️ Poor tradeoffNext.js 15
next devhonoursPORT; keep-ponly for the repo-specific default.Next.js 15’s
next devcan read the port from thePORTenvironment variable (default is3000). Your script’s-p ${PORT:-8000}is therefore mainly enforcing the required default of8000whenPORTisn’t set. Consider simplifying by setting a default via env instead, e.g.PORT=${PORT:-8000} next dev.
https://vercel-next-js.mintlify.app/api-reference/cli/next-dev
[apps/website/package.json: line 7]🤖 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 `@apps/website/package.json` at line 7, The package.json "start" script currently uses "next dev -p ${PORT:-8000}" which redundantly passes -p; instead set the PORT env default and call next dev so Next.js reads it natively—update the "start" script entry (in package.json) to export PORT with a default (e.g. PORT=${PORT:-8000} next dev) and remove the -p flag so the Next.js CLI uses the PORT env variable as intended.
🤖 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.
Nitpick comments:
In `@apps/storybook/package.json`:
- Line 6: The start script in package.json uses shell-specific parameter
expansion ("start": "storybook dev -p ${PORT:-6006}") which fails on Windows;
update the "start" script to use a cross-platform approach (e.g., use cross-env
or npm package config) so the PORT fallback works on all OSes—locate the "start"
script entry in package.json and replace the bash-style "${PORT:-6006}" usage
with a cross-platform pattern (e.g., read process.env.PORT via cross-env or an
npm script that sets PORT) ensuring Storybook still listens on the fallback 6006
when PORT is unset.
In `@apps/website/package.json`:
- Line 7: The "start" script uses bash-only parameter expansion
("${PORT:-8000}") which breaks on Windows; replace it with a cross-platform
approach by creating a small Node launcher (e.g., scripts/start.js) that reads
process.env.PORT || 8000 and spawns "next dev -p <port>" (or sets
process.env.PORT and uses require('child_process').spawn to run "next"), then
update the package.json "start" script to "node ./scripts/start.js";
alternatively, install and use cross-env/cross-env-shell and adjust the "start"
script accordingly if you prefer that dependency.
- Line 7: The package.json "start" script currently uses "next dev -p
${PORT:-8000}" which redundantly passes -p; instead set the PORT env default and
call next dev so Next.js reads it natively—update the "start" script entry (in
package.json) to export PORT with a default (e.g. PORT=${PORT:-8000} next dev)
and remove the -p flag so the Next.js CLI uses the PORT env variable as
intended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bab789c1-d75e-4aef-97f0-7ea4d57401a7
📒 Files selected for processing (2)
apps/storybook/package.jsonapps/website/package.json
Description
The
startscript in bothapps/websiteandapps/storybookhardcoded the dev-server port (-p 8000/-p 6006), so a second checkout/worktree couldn't run its own dev server without clashing on the port. This makes the port configurable via thePORTenv var, keeping the current values as defaults.Issue
Fixes #2547
Developer checklist
Screenshot
N/A