fix interactive menu#162
Conversation
|
Warning Review limit reached
Next review available in: 17 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesInteractive CLI and node targeting
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant REPL
participant Commander
participant Command
User->>REPL: Enter command
REPL->>REPL: Tokenize and normalize input
REPL->>Commander: parseAsync(tokens)
Commander->>Command: Execute command
Command-->>Commander: Result or error
Commander-->>REPL: Completion
REPL-->>User: Display output and prompt
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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
🧹 Nitpick comments (4)
src/index.ts (4)
148-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
main()is missing an explicit return type annotation.As per coding guidelines,
**/*.{ts,tsx}: "Always provide explicit type annotations for function parameters and return types."♻️ Proposed fix
-async function main() { +async function main(): Promise<void> {🤖 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 `@src/index.ts` at line 148, Add an explicit return type annotation to the async main() function, using the appropriate Promise-based type for its existing return behavior. Do not alter the function’s implementation or surrounding control flow.Source: Coding guidelines
182-182: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTop-level error log doesn't use
chalk, unlike the rest of the CLI's error output.As per coding guidelines,
**/*.{ts,tsx}: "Use chalk library for colored console output in error messages."🤖 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 `@src/index.ts` at line 182, Update the top-level error logging around the console.error call to use chalk for colored error output, matching the CLI’s existing error-message conventions. Preserve the “Program Error:” context and error.message content while applying the established chalk symbol or import.Source: Coding guidelines
1-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
programmodule-state variable has no type annotation.
let program(line 6) is later assignedawait createCLI()but declared without a type, so it's implicitly typed loosely through inference gaps rather than an explicitCommandannotation.As per coding guidelines,
**/*.{ts,tsx}: "Always provide explicit type annotations for function parameters and return types" / "Avoidanytype; useunknownif necessary."♻️ Proposed fix
-let program +let program: Command🤖 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 `@src/index.ts` around lines 1 - 19, Annotate the module-level program variable as Command in src/index.ts, matching the Command instance returned by createCLI and the type expected by enableExitOverride. Keep its existing assignment and command setup behavior unchanged.Source: Coding guidelines
67-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUser-facing error output bypasses
chalkstyling used elsewhere in the CLI.
console.log(\Invalid option: ...`)(line 81) andconsole.error("Command error:", ...)(line 91) print unstyled text, unlike the rest of the codebase (e.g.src/cli.tsconsistently wraps errors inchalk.red(...)`).As per coding guidelines,
**/*.{ts,tsx}: "Use chalk library for colored console output in error messages."🤖 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 `@src/index.ts` around lines 67 - 94, The user-facing errors in runTokens are printed without the CLI’s required chalk styling. Wrap the invalid-command message in chalk.red before console.log, and wrap the unexpected runtime error output in chalk.red before console.error, preserving the existing CommanderError filtering and message content.Source: Coding guidelines
🤖 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 `@README.md`:
- Around line 443-444: Update the “Named Options” allowAlgo example to include
the required datasetDid and algoDid positional arguments alongside the named
options, matching the current src/cli.ts contract.
In `@src/cli.ts`:
- Around line 469-478: Update the node option definition in the command action
to require a string value rather than allowing an optional argument, ensuring
options.node cannot become true when --node is passed without a value. Preserve
the existing fallback between options.node and the positional node argument when
calling Commands.getComputeEnvironments.
In `@src/index.ts`:
- Around line 157-176: Return immediately after handling --help or -h in the
startup flow, before the AVOID_LOOP_RUN branch and initial command processing,
so help is printed once and neither parseAsync nor runTokens/runLoop executes.
---
Nitpick comments:
In `@src/index.ts`:
- Line 148: Add an explicit return type annotation to the async main() function,
using the appropriate Promise-based type for its existing return behavior. Do
not alter the function’s implementation or surrounding control flow.
- Line 182: Update the top-level error logging around the console.error call to
use chalk for colored error output, matching the CLI’s existing error-message
conventions. Preserve the “Program Error:” context and error.message content
while applying the established chalk symbol or import.
- Around line 1-19: Annotate the module-level program variable as Command in
src/index.ts, matching the Command instance returned by createCLI and the type
expected by enableExitOverride. Keep its existing assignment and command setup
behavior unchanged.
- Around line 67-94: The user-facing errors in runTokens are printed without the
CLI’s required chalk styling. Wrap the invalid-command message in chalk.red
before console.log, and wrap the unexpected runtime error output in chalk.red
before console.error, preserving the existing CommanderError filtering and
message content.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8bed695-ad00-4bb5-b5ab-40361de90f2a
📒 Files selected for processing (7)
README.mdpackage.jsonsrc/cli.tssrc/commands.tssrc/index.tstest/paidComputeFlow.test.tstest/replMenu.test.ts
|
/run-security-scan |
alexcos20
left a comment
alexcos20
left a comment
There was a problem hiding this comment.
AI automated code review (Gemini 3).
Overall risk: low
Summary:
Excellent refactoring of the interactive REPL to use readline/promises and async iterators, significantly improving stability and developer experience. Command execution is handled robustly without killing the REPL loop. Added new functionality and better argument parsing. LGTM!
Comments:
• [INFO][style] The custom tokenize function is a great addition for handling string arguments with spaces, like JSON strings. Note that it doesn't currently support backslash escaping (e.g., "{\"key\": \"value\"}"), so users must use single quotes to wrap JSON (e.g., '{"key": "value"}'). This is standard practice and acceptable, but worth keeping in mind if users report issues with nested quotes.
• [INFO][style] Using for await (const rawLine of rl) is an excellent and modern approach to handling the REPL loop. It cleanly manages backpressure and eliminates the need for recursive prompt functions.
• [WARNING][other] Bumping the engine requirement to node: >=22 is quite aggressive, as Node 20 is still in LTS until April 2026. Unless there are specific Node 22 features required, consider lowering this to >=20 to support a wider range of environments. If this is an intentional project-wide decision, feel free to ignore.
• [INFO][style] Good design choice allowing both a positional argument [node] and a flag -n, --node <node>. This greatly enhances flexibility and ease of use for CLI users.
2 participants
Fix #161 (Interactive menu (REPL) mode)
Summary
Rewrites the interactive command loop in
src/index.ts. In menu mode (npm run cliwith no
AVOID_LOOP_RUN), many commands silently failed or behaved differently thantheir one-shot equivalents. This PR makes menu mode behave identically to one-shot mode,
fixes several ways the REPL could hang or drop input, and lets
getComputeEnvironmentsaccept an optional target node.
Menu-mode bugs fixed
Bare start broke every menu command. The fake
node/script-path prefix thatCommander's default parsing expects was only populated when the process was started
with an initial command. Starting with plain
npm run clileft it empty, so thefirst two tokens of every typed command were consumed as "node binary" + "script
path" and the command dumped help instead of running. Fixed by parsing with
{ from: "user" }, which removes the prefix mechanism entirely — bare start andinitial-command start now behave the same.
Commands with ≥2 arguments were silently discarded. Input with ≥3 tokens was
only handled if it literally began with
npm run cli; otherwise it was dropped withno output. Affected most of the CLI in menu mode (
download,startCompute,editAsset,stopCompute,authorizeEscrow, …). Fixed by treating all tokensuniformly after stripping an optional
npm run cliprefix.An empty Enter bricked the session.
readLine()busy-waitedwhile (answer == "")on an already-closed interface, spinning forever on one emptyline. Fixed: empty input simply re-prompts.
A command error could kill the whole session.
exitOverride()was only set onthe root program, so a subcommand parse error (e.g.
getDDOwith nodid) was raisedby the subcommand itself and called
process.exit(1), dropping the user back to theshell.
exitOverride()is now applied recursively to every subcommand, so any parseerror is caught, reported, and the loop continues waiting for the next command.
Initial-command failures (previously swallowed by an empty
catch) are surfaced thesame way.
Naive tokenization.
commandLine.split(" ")could not parse quotes, so JSONarguments (
startComputeresources/datasets,editAssetbodies) never worked in themenu, and double spaces injected empty tokens. Replaced with a small quote-aware
tokenizer that honors
"…"/'…'and drops empty tokens. (Wrap JSON in singlequotes, per the documented convention, so its inner double quotes are preserved.)
New in menu mode:
Tabcompletes aunique command (e.g.
getComputeE→getComputeEnvironments) or lists the candidateswhen several match (e.g.
get→getDDO,getComputeEnvironments,getJobStatus, …).Completion applies to the command name only, not its arguments.
Also fixed as part of the rewrite:
discards buffered stdin beyond the first line. The loop now uses a single persistent
interface consumed via its async iterator, which respects backpressure and never drops
lines — important for scripted/piped usage and the regression tests below.
helpno longer prints a stack trace. BenignCommanderErrorcodes(
commander.helpDisplayed,commander.help,commander.version) are suppressed;real errors are shown.
exit, the loopterminates instead of hanging.
undefinedaliases are no longer pushed into the supported-commands list;unknown commands suggest
help; option-style first tokens (--help,--version) arerouted to Commander instead of being rejected as "Invalid option"; the dead
length < 1check was removed. Commander already prints its own message for parseerrors, so those are no longer double-reported. A single descriptive prompt
(
Enter command ('exit' | 'quit' or CTRL-C to terminate):) is shown at startup andafter every command, with no separate redundant banner.
Coordination with the payment prompt
The
startComputeflow opens its own stdin interface for payment confirmation(
src/cli.ts). The REPL pauses its interface around command execution so the two nevercompete for stdin. (The payment prompt is disabled on non-TTY stdin anyway.)
getComputeEnvironmentsaccepts an optional nodegetComputeEnvironments(aliasgetC2DEnvs) now takes an optional node argument —getComputeEnvironments <nodeUrlOrPeerId>or--node <nodeUrlOrPeerId>— to query aspecific Ocean Node, defaulting to
NODE_URLwhen omitted. Previously the commanddeclared zero arguments, so any extra token (e.g.
getComputeEnvironments peerID)hard-errored with "too many arguments" under Commander v13. Every other command keeps
strict excess-argument checking, which gives users a clear error rather than silently
ignoring typos.
Behavior preserved
AVOID_LOOP_RUN=true) is unchanged: it still parsesprocess.argvdirectly with Commander's default parsing.
Regression tests
Adds
test/replMenu.test.ts, an infra-free system test that spawns the CLI with pipedstdin and drives the REPL. It points
PRIVATE_KEY/RPC/NODE_URLat an unreachableport, so a command that actually parses and runs surfaces a
Command error(connectionrefused) while a dropped or parse-rejected command does not — letting the test
distinguish "ran" from "silently ignored" without any live node. Covered cases:
npm run cliprefixexitterminates cleanlygetComputeEnvironments <node>accepts the optional node argumentHousekeeping
enginesfield ("node": ">=22") topackage.jsonso the Node version in.nvmrcis actually enforced by npm. The wrong Node version silently produces a stalenode_modules/ boot failure, which is easy to miss.getComputeEnvironmentsdocs to show the new optional node argumentand the
-n, --nodeoption.Other verification
npm run build:tscandnpm run lint— pass (only pre-existingno-explicit-anywarnings remain; all touched files are clean).
resolveComputeInputsunit test — 11 passing.setup.test.ts— 4 passing (thegetComputeEnvironmentshelp line now readsgetComputeEnvironments|getC2DEnvs [options] [node]; existing assertions still hold).Summary by CodeRabbit
New Features
getComputeEnvironmentsnow supports targeting a specific node by URL or peer ID.npm run cliprefixes.Documentation
Chores