dev to main #27
dev to main #27
Conversation
* fix: update default port number for the server from 8008 to 2311 * fix: bump up version * fix: enhance error handling for ZenStack module loading and server startup
jiashengguo
changed the title
📝 WalkthroughWalkthroughThe pull request updates the package version to 0.2.7, changes the CLI default port from 8008 to 2311, and refactors server initialization with improved error handling for port conflicts and ZenStack module loading failures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI/server default port, improves robustness of ZenStack module loading, and bumps the package version.
Changes:
- Change the CLI default server port from
8008to2311. - Harden ZenStack model-meta loading with clearer
CliErrormessages and a more explicit resolution path. - Add an HTTP server
errorhandler for port conflicts and general startup failures, and bump@zenstackhq/proxyversion to0.2.7.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/server.ts | Improves ZenStack module loading error handling and adds an HTTP server error handler with colored output for port conflicts and startup errors. |
| src/index.ts | Updates the -p/--port CLI option default value to 2311 so new runs use the new port by default. |
| package.json | Bumps package version to 0.2.7 and reformats the keywords array. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| ) | ||
| } else { | ||
| throw new CliError(`Failed to start the server: ${err.message}`) |
There was a problem hiding this comment.
Throwing a CliError inside the server.on('error', ...) handler means the subsequent process.exit(1) is unreachable for the non-EADDRINUSE branch, and the error will surface as an unhandled exception outside the CLI's try/catch around program.parseAsync(). To keep error handling consistent and avoid unhandled exceptions, this handler should either log the error and call process.exit(1) without throwing, or propagate the failure through a rejected promise from startServer so the top-level CLI error handling can catch and report it.
| throw new CliError(`Failed to start the server: ${err.message}`) | |
| console.error(red(`Failed to start the server: ${err.message}`)) |
![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
🤖 Fix all issues with AI agents
In `@src/server.ts`:
- Around line 256-267: The server 'error' handler currently throws a CliError
for non-EADDRINUSE errors which prevents the subsequent process.exit(1) from
running; change the handler in server.on('error', ...) so it logs the error
message the same way as the EADDRINUSE branch (e.g., console.error(red(`Failed
to start the server: ${err.message}`))) instead of throwing CliError, then allow
the existing process.exit(1) to execute; update references to CliError usage in
this handler accordingly.
| server.on('error', (err: NodeJS.ErrnoException) => { | ||
| if (err.code === 'EADDRINUSE') { | ||
| console.error( | ||
| red( | ||
| `Port ${options.port} is already in use. Please choose a different port using -p option.` | ||
| ) | ||
| ) | ||
| } else { | ||
| throw new CliError(`Failed to start the server: ${err.message}`) | ||
| } | ||
| process.exit(1) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for global exception handlers that might intercept throws from server "error" events
rg -n "uncaughtException|unhandledRejection" -SRepository: zenstackhq/proxy
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Read the relevant section of src/server.ts to verify the code
if [ -f "src/server.ts" ]; then
wc -l src/server.ts
echo "---"
sed -n '250,275p' src/server.ts
else
echo "File not found, searching for server files..."
find . -name "server.ts" -type f | head -10
fiRepository: zenstackhq/proxy
Length of output: 844
🏁 Script executed:
#!/bin/bash
# Search for CliError definition and usage to understand its purpose
rg -n "class CliError|new CliError" -A 2 -B 2 | head -50Repository: zenstackhq/proxy
Length of output: 2727
🏁 Script executed:
#!/bin/bash
# Find CliError class definition
rg -n "class CliError|export class CliError" -A 5Repository: zenstackhq/proxy
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Check if there are other server error handlers and how they handle errors
rg -n "\.on\('error'" -A 10 -B 2Repository: zenstackhq/proxy
Length of output: 653
Avoid throwing in the server "error" handler; process.exit(1) is unreachable for non-EADDRINUSE errors.
Throwing CliError here bypasses the subsequent process.exit(1) and causes an uncaught exception with a stack trace instead of a clean error message. No global exception handlers exist to catch this, so adopt the same console.error pattern used for the EADDRINUSE case.
🔧 Suggested fix
server.on('error', (err: NodeJS.ErrnoException) => {
if (err.code === 'EADDRINUSE') {
console.error(
red(
`Port ${options.port} is already in use. Please choose a different port using -p option.`
)
)
} else {
- throw new CliError(`Failed to start the server: ${err.message}`)
+ console.error(red(`Failed to start the server: ${err.message}`))
}
process.exit(1)
})🤖 Prompt for AI Agents
In `@src/server.ts` around lines 256 - 267, The server 'error' handler currently
throws a CliError for non-EADDRINUSE errors which prevents the subsequent
process.exit(1) from running; change the handler in server.on('error', ...) so
it logs the error message the same way as the EADDRINUSE branch (e.g.,
console.error(red(`Failed to start the server: ${err.message}`))) instead of
throwing CliError, then allow the existing process.exit(1) to execute; update
references to CliError usage in this handler accordingly.
fix: update default port number for the server from 8008 to 2311
fix: bump up version
fix: enhance error handling for ZenStack module loading and server startup
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.