refactor!: rewamp cli#175
Merged
Merged
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughRefactors CLI by extracting in-file logic into modular Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/cli/fetch.ts`:
- Around line 58-62: The URL construction missed including cliOpts.port so
requests always use default ports; update the base used for new URL in the fetch
logic to include the port when provided (e.g., build the origin as
`http${cliOpts.tls ? "s" : ""}://${cliOpts.host || "localhost"}${cliOpts.port ?
`:${cliOpts.port}` : ""}`) and keep using cliOpts.url for the path; adjust the
code around the URL creation (the const url assignment that references cliOpts,
cliOpts.tls, cliOpts.host, and cliOpts.url) to conditionally append
`:${cliOpts.port}` only when cliOpts.port is set.
In `@src/cli/main.ts`:
- Around line 16-20: The child-process branch runs setupProcessErrorHandlers()
and awaits cliServe(cliOpts) but then continues executing parent logic; add an
early return immediately after the awaited cliServe(cliOpts) call so the child
path exits (e.g., return or process.exit) to prevent the child from falling
through into the parent flow; update the block containing process.send,
setupProcessErrorHandlers, and cliServe(cliOpts) accordingly.
In `@src/cli/serve.ts`:
- Around line 38-70: The code unconditionally sets port: cliOpts.port when
calling srvxServe, which overwrites any port exported in serverOptions; change
this to only set the port when cliOpts.port is provided (e.g., conditionally
include port from cliOpts or merge cliOpts after serverOptions), so
serverOptions.port (from the entry module) remains effective unless the user
passed --port; update the srvxServe call where serverOptions and port:
cliOpts.port are combined to apply the conditional logic (symbols:
serverOptions, cliOpts.port, srvxServe).
- Around line 85-99: The dev error page currently injects raw error text into
the HTML (see the html string building in serve.ts inside the cliOpts.prod
branch where the template includes <pre>${error instanceof Error ? error.stack
|| error.message : String(error)}</pre>); fix this by HTML-escaping the error
content before interpolation (create/use an escapeHtml helper that replaces &,
<, >, ", ' and backticks) and replace the direct interpolation with the escaped
value (e.g., escapedError = escapeHtml(error instanceof Error ? error.stack ||
error.message : String(error)) and use <pre>${escapedError}</pre>); ensure the
helper is used consistently for both Error stack and message cases.
🧹 Nitpick comments (3)
src/cli/usage.ts (1)
2-2: Remove unused importCLIOptions.
CLIOptionsis imported but not referenced in this file. OnlyMainOptionsis used.🧹 Suggested fix
-import type { CLIOptions, MainOptions } from "./types.ts"; +import type { MainOptions } from "./types.ts";automd.config.ts (1)
20-20: Consider typing_ctxmore precisely if automd exports context types.Using
anyworks but loses type safety. If the automd library exports a context type, consider using it instead.src/cli/fetch.ts (1)
172-189: Consider expanding binary content-type detection.The
isBinarycheck covers common cases but misses some binary types likeapplication/x-tar,application/x-7z-compressed,font/*, andmodel/*. This is a minor gap since unknown types default to text handling, which is generally safe.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/cli/main.ts`:
- Around line 86-91: The exit handler currently only accepts `code` and calls
`process.exit(code)`, which misbehaves when a child is killed by a signal (code
=== null). Update the listener signature to (code, signal) in the
child.on("exit", ...) handler, then: if code !== null use process.exit(code);
else if signal != null propagate the signal to the current process (e.g.,
process.kill(process.pid, signal)); otherwise handle as a non-zero failure
(process.exit(1)). This avoids calling process.exit(null) and correctly
propagates signal-based termination.
- Around line 160-165: The call to statSync(maybeEntryOrDir) can throw if the
path doesn't exist; wrap the statSync call in a try-catch around the block that
sets values.dir / values.entry, catch ENOENT (and other FS errors) and surface a
user-friendly error message (or throw a controlled error) instead of letting the
raw stack trace propagate; modify the logic around statSync, maybeEntryOrDir,
and the assignments to values.dir / values.entry to handle missing paths
gracefully and log a clear message indicating the provided positional argument
was not found.
- Around line 70-78: The Node version check that pushes
"--experimental-strip-types" should be narrowed so it only applies to Node 22
(where the flag is still experimental); update the condition around
runtimeArgs.push("--experimental-strip-types") to require major === "22" (and
+minor >= 6 if you want the minor guard), or alternately explicitly skip for
major >= "23". Locate the block that reads process.versions.node and the symbols
isNode, runtimeArgs and adjust the if condition so Node 23+ does not receive the
experimental flag.
In `@src/cli/serve.ts`:
- Line 53: The TLS config currently sets tls: cliOpts.tls ? { cert:
cliOpts.cert, key: cliOpts.key } : undefined which can pass undefined cert/key
when --tls is set without files; update the logic in the serve initialization to
validate that when cliOpts.tls is truthy both cliOpts.cert and cliOpts.key are
present (or else throw/exit with a clear error or print a warning), and only
then set tls: { cert: cliOpts.cert, key: cliOpts.key }; otherwise set tls to
undefined (or abort). Reference cliOpts.tls, cliOpts.cert, cliOpts.key and the
tls property in the serve setup to locate and change the code.
🧹 Nitpick comments (1)
src/cli/main.ts (1)
24-24: Redundant ternary; simplify toprocess.exit(0).Since we're inside the
if (cliOpts.help)block,cliOpts.helpis always truthy here, making the ternary unnecessary.✨ Suggested fix
- process.exit(cliOpts.help ? 0 : 1); + process.exit(0);
This was referenced Jul 16, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The CLI codebase had been growing without maintenance. This PR adds a clear
srvx servenew command with backward compatibility.(still releasing as major since there might be slight behavior changes for args)
Summary by CodeRabbit
Documentation
New Features
Refactor