feat: add cli log level #1864
feat: add cli log level #1864
Conversation
|
📝 WalkthroughWalkthroughAdded a new optional CLI option Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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}
| .option( | ||
| '--log-level <level>', | ||
| 'Set the log level (info | warn | error | silent)', | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify type safety and implementation of logLevel option.
The new --log-level option is added, but several aspects need verification:
- Type safety: Ensure
BuildOptionsincludes alogLevelproperty with the appropriate type (likely a union of 'info' | 'warn' | 'error' | 'silent'). - Implementation: Confirm that
build.jsactually consumes and applies this option. - Validation: Consider adding runtime validation to ensure only valid values are accepted. Commander.js will accept any string by default.
- Consistency: Should
dev,inspect, andpreviewcommands also support--log-levelfor consistent logging control across all commands?
Run the following script to verify the implementation:
🏁 Script executed:
#!/bin/bash
# Description: Verify BuildOptions type definition and build.js implementation
# Check BuildOptions type definition
echo "=== Checking BuildOptions type definition ==="
ast-grep --pattern 'export interface BuildOptions {
$$$
}'
# Check if build.js uses logLevel
echo "=== Checking build.js for logLevel usage ==="
rg -n -C5 'logLevel' packages/rspeedy/core/src/cli/build.js packages/rspeedy/core/src/cli/build.ts
# Check DevOptions, InspectOptions, and PreviewOptions for comparison
echo "=== Checking other Options interfaces ==="
ast-grep --pattern 'export interface DevOptions {
$$$
}'
ast-grep --pattern 'export interface InspectOptions {
$$$
}'
ast-grep --pattern 'export interface PreviewOptions {
$$$
}'Length of output: 211
The previous script didn’t locate BuildOptions or build.js. Let’s first find the correct build file and interface definitions:
🏁 Script executed:
#!/bin/bash
echo "=== Finding build files ==="
fd -e ts -e js build --strip-cwd-prefix packages/rspeedy/core/src/cli
echo "=== Finding BuildOptions interface ==="
rg -n "export interface BuildOptions" -g "packages/rspeedy/core/src/cli/**/*.ts"
echo "=== Searching for logLevel usage ==="
rg -n "logLevel" -g "packages/rspeedy/core/src/cli/**/*.ts"Length of output: 279
🏁 Script executed:
#!/bin/bash
echo "=== Finding build files in CLI directory ==="
find packages/rspeedy/core/src/cli -type f | grep -E 'build\.(ts|js)$'
echo "=== Searching for BuildOptions interface in build.ts ==="
rg -n "export interface BuildOptions" packages/rspeedy/core/src/cli/build.ts
echo "=== Printing build.ts head ==="
sed -n '1,200p' packages/rspeedy/core/src/cli/build.ts
echo "=== Searching for logLevel in build.ts ==="
rg -n "logLevel" packages/rspeedy/core/src/cli/build.ts
echo "=== Searching for logLevel in commands.ts ==="
rg -n "logLevel" packages/rspeedy/core/src/cli/commands.tsLength of output: 2454
🏁 Script executed:
#!/bin/bash
# Locate commands.ts
echo "=== commands.ts path ==="
find packages/rspeedy/core/src/cli -type f | grep 'commands\\.ts$'
# Print first 200 lines of commands.ts
echo "=== commands.ts head ==="
sed -n '1,200p' packages/rspeedy/core/src/cli/commands.ts
# Extract CommonOptions interface
echo "=== CommonOptions interface ==="
rg -n "export interface CommonOptions" -C5 packages/rspeedy/core/src/cli/commands.tsLength of output: 4108
Add and implement the log-level option across all commands.
- Extend CommonOptions in
packages/rspeedy/core/src/cli/commands.tsto include
logLevel?: 'info' | 'warn' | 'error' | 'silent'so it’s available on BuildOptions, DevOptions, InspectOptions, and PreviewOptions. - In
packages/rspeedy/core/src/cli/build.ts, configure the logger with the provided level (e.g.logger.setLevel(buildOptions.logLevel)) before running the build. - Enforce valid values at parse time—use Commander’s choice support or add a manual check to reject anything outside
info|warn|error|silent. - Mirror the
--log-leveloption and its handling in thedev,inspect, andpreviewcommands for consistent logging control.
🤖 Prompt for AI Agents
packages/rspeedy/core/src/cli/commands.ts around lines 68-71: the CLI currently
defines a --log-level option but CommonOptions type lacks logLevel and other
command handlers don't consume it; update CommonOptions to include logLevel?:
'info' | 'warn' | 'error' | 'silent' so BuildOptions, DevOptions,
InspectOptions, and PreviewOptions inherit it; in build/dev/inspect/preview
command entrypoints (e.g. packages/rspeedy/core/src/cli/build.ts and analogous
files) read the parsed option and call the logger configuration before running
the command (e.g. logger.setLevel(buildOptions.logLevel)); enforce allowed
values at parse time by adding Commander choices for the option or by adding an
early validation that rejects values not in ['info','warn','error','silent'];
add the same --log-level option/choices and handling to the dev, inspect, and
preview command definitions to ensure consistent behavior.
There was a problem hiding this comment.
Thank you for your PR. Just like @coderabbitai said in #1864 (comment), we need to implement the logLevel option alone the the CLI flag.
kedevked
force-pushed
the
feature/add-cli-log-level
branch
from
6f43730 to
fd7e775
Compare
|
@colinaaa , the logLevel is now implemented accross |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/rspeedy/core/src/cli/build.ts(1 hunks)packages/rspeedy/core/src/cli/commands.ts(2 hunks)packages/rspeedy/core/src/cli/dev.ts(1 hunks)packages/rspeedy/core/src/cli/inspect.ts(1 hunks)packages/rspeedy/core/src/cli/preview.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/rspeedy/core/src/cli/dev.ts
- packages/rspeedy/core/src/cli/preview.ts
- packages/rspeedy/core/src/cli/build.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/rspeedy/core/src/cli/inspect.ts (1)
packages/rspeedy/core/src/index.ts (1)
logger(29-29)
🔇 Additional comments (2)
packages/rspeedy/core/src/cli/commands.ts (1)
22-22: LGTM! Type definition is correct.The
logLevelproperty is properly typed with a union of valid log levels and will be inherited by all command option interfaces (BuildOptions, DevOptions, InspectOptions, PreviewOptions).packages/rspeedy/core/src/cli/inspect.ts (1)
23-25: LGTM! Logger configuration is correctly implemented.The log level is set early in the flow, before any initialization logic, which ensures consistent logging behavior throughout the command execution. The conditional check prevents setting the level when no value is provided.
Note: Input validation should be added at the CLI option parsing level (see comment on
commands.ts), so invalid values never reach this point.
| .option( | ||
| '--log-level <level>', | ||
| 'Set the log level (info | warn | error | silent)', | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Add validation for log level values.
Commander.js will accept any string for --log-level, but only 'info' | 'warn' | 'error' | 'silent' are valid. Without validation, invalid values will be silently passed through, potentially causing runtime errors.
Apply this diff to add validation using Commander's .choices():
.option(
'--log-level <level>',
'Set the log level (info | warn | error | silent)',
)
+ .addOption(
+ new Option('--log-level <level>', 'Set the log level')
+ .choices(['info', 'warn', 'error', 'silent'])
+ )Note: You'll need to import Option from commander:
import type { Command } from 'commander'
+import { Option } from 'commander'Run the following script to verify that all command implementations (build, dev, preview) consume the logLevel option consistently:
🏁 Script executed:
#!/bin/bash
# Description: Verify that all CLI commands implement logLevel option handling
echo "=== Checking build.ts for logLevel usage ==="
rg -n -C3 'buildOptions\.logLevel|logger\.level' packages/rspeedy/core/src/cli/build.ts
echo "=== Checking dev.ts for logLevel usage ==="
rg -n -C3 'devOptions\.logLevel|logger\.level' packages/rspeedy/core/src/cli/dev.ts
echo "=== Checking preview.ts for logLevel usage ==="
rg -n -C3 'previewOptions\.logLevel|logger\.level' packages/rspeedy/core/src/cli/preview.ts
echo "=== Checking inspect.ts for logLevel usage (for comparison) ==="
rg -n -C3 'inspectOptions\.logLevel|logger\.level' packages/rspeedy/core/src/cli/inspect.tsLength of output: 1035
Add validation for --log-level via Commander.js .choices()
Restrict allowed values to info | warn | error | silent by replacing the loose .option() call. For example:
import type { Command } from 'commander'
+import { Option } from 'commander'
command
- .option(
- '--log-level <level>',
- 'Set the log level (info | warn | error | silent)',
- )
+ .addOption(
+ new Option('--log-level <level>', 'Set the log level')
+ .choices(['info', 'warn', 'error', 'silent']),
+ )All commands (build, dev, preview, inspect) already read logLevel and set logger.level.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .option( | |
| '--log-level <level>', | |
| 'Set the log level (info | warn | error | silent)', | |
| ) | |
| import type { Command } from 'commander' | |
| import { Option } from 'commander' | |
| command | |
| .addOption( | |
| new Option('--log-level <level>', 'Set the log level') | |
| .choices(['info', 'warn', 'error', 'silent']), | |
| ) |
🤖 Prompt for AI Agents
In packages/rspeedy/core/src/cli/commands.ts around lines 47 to 50, the
--log-level option is currently defined without validation; change the Commander
option to restrict allowed values to the set ['info','warn','error','silent']
using Commander.js .choices() (and optionally provide a sensible default like
'info'), so parsing will fail on invalid inputs and downstream code that reads
logLevel/logger.level always receives one of the allowed values.
2 participants
This PR add the logLevel to the cli following this issue
Summary by CodeRabbit