fix(cli): update add command support id option#20
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughRefactors CLI parsing to structured command payloads, adds a --verbose/ui.debug mode, threads logger callbacks through git/fetch flows, decodes/normalizes include patterns, warns on unmatched includes and overwriting TOC.md, and updates tests to cover these behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Parser
participant Runner
participant Sync
participant Resolver
participant Fetcher
participant Materializer
participant UI
CLI->>Parser: parse args (includes --verbose)
Parser->>CLI: parsed: CliCommand
CLI->>UI: setVerboseMode(parsed.options.verbose)
CLI->>Runner: runCommand(parsed)
Runner->>Sync: run(parsed.command, options)
Sync->>Resolver: resolveRemoteCommit(...)
Resolver-->>Sync: commit/hash
Sync->>Fetcher: fetchSource(..., logger?)
Fetcher-->>Sync: repoDir (from cache or clone)
Sync->>Materializer: materializeSource(repoDir, includePatterns...)
Materializer-->>Sync: materialized files / manifest
UI-->>CLI: debug/warn messages (via logger)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for custom source IDs via the --id option in the add command, refactors CLI argument parsing into a dedicated module, and adds verbose logging capabilities. The PR also includes several related improvements to error messaging, pattern matching, and host validation.
Changes:
- Added
--idoption to allow custom IDs when adding sources, with support for both pre-specified and retroactive ID assignment - Refactored argument parsing logic from
src/cli/index.tstosrc/cli/parse-args.tswith type-safe command-specific argument structures - Added
--verboseflag with debug logging for git operations and pattern matching warnings - Enhanced host allowlist to support subdomains and SCP-style SSH URLs
- Added percent-decoding support for include patterns and improved glob pattern handling
- Removed git archive fallback in favor of consistent clone-based fetching
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/sync-toc.test.js |
Added test for TOC overwrite warning when regenerating existing TOC files |
tests/sync-materialize.test.js |
Added tests for include pattern warnings and percent-encoded pattern handling |
tests/resolve-remote.test.js |
Added tests for subdomain matching in allowHosts and SCP-style SSH URL parsing |
tests/cli-parse.test.js |
Added test for --verbose flag parsing |
tests/cli-add.test.js |
Added tests for --id option with single and multiple sources |
src/toc.ts |
Added warning when overwriting existing TOC.md files |
src/sync.ts |
Integrated verbose logging for resolve and fetch operations |
src/materialize.ts |
Added pattern normalization with percent-decoding, parentheses escaping, and no-match warnings |
src/git/resolve-remote.ts |
Enhanced host parsing for SCP-style SSH and subdomain matching in allowlist |
src/git/fetch-source.ts |
Removed git archive fallback, added verbose logging throughout git operations |
src/cli/ui.ts |
Added debug logging function with verbose mode support |
src/cli/types.ts |
Refactored CliCommand to use command-specific fields instead of generic args array |
src/cli/parse-args.ts |
Moved parseAddEntries from index.ts, added --id parsing, reorganized command definitions |
src/cli/index.ts |
Removed parseAddEntries (moved to parse-args.ts), added verbose mode initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/git/fetch-source.ts`:
- Around line 50-55: The current execa invocation in fetch-source.ts sets stdio:
"inherit" when options?.logger is present, which sends raw git output to the
terminal but doesn't forward it to the provided logger; change this to use
"pipe" and, if options?.logger is provided, attach listeners to the child
process stdout/stderr and forward their data to options.logger (include command
context using the same [...configs, ...args] string), otherwise keep "pipe"
behavior; update the execa call and any related error/exit handling in the
function that calls execa so stdout/stderr are captured and logged instead of
inheriting the terminal output.
In `@src/materialize.ts`:
- Around line 235-247: The include-pattern warning is being written
unconditionally via ui.line(...) and will corrupt JSON output when --json is
used; modify the materialization flow to accept a json boolean (add a json
parameter to materializeSource and propagate it from the caller) and change the
check around the warning (the block using includePatterns, files, ui.line,
symbols.warn and resolved.sourceId) to only call ui.line when json is false
(i.e., gate the warning behind !json); ensure callers that invoke
materializeSource pass the json flag so JSON mode can suppress this warning.
🧹 Nitpick comments (1)
src/cli/parse-args.ts (1)
36-119: Consider documenting the--idpositional binding rules.The
parseAddEntriesfunction handles complex stateful parsing where--idcan appear either before or after a repo:
add --id myid repo1→ id binds to following repoadd repo1 --id myid→ id binds to preceding repoThis is flexible but the behavior may not be obvious to users. Consider adding a comment explaining the binding semantics, or documenting it in the help text.
📝 Suggested documentation comment
+/** + * Parses add command arguments into AddEntry[]. + * Supports flexible --id placement: + * - `add --id myid repo1` → id binds to the following source + * - `add repo1 --id myid` → id binds to the preceding source + * - `add --source repo1 --id myid` → id binds to the preceding --source + */ const parseAddEntries = (rawArgs: string[]): AddEntry[] => {
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/cli/parse-args.ts`:
- Around line 88-99: The error text for missing value when handling the arg
variable in the parse-args block that checks for "--target" or "--target-dir"
should mention both option names (or the actual arg used) instead of always
saying "--target expects a value"; update the thrown Error in the branch that
checks !next || next.startsWith("-") so it reports either "`--target` or
`--target-dir` expects a value" or interpolates the actual arg (arg) into the
message; adjust the string thrown in that branch and ensure the change is
applied where entries[lastIndex].targetDir is set in the same switch/if
handling.
Summary
Add
--idparameter toaddcommand and refactor argument parsing.Changes
--idoption to associate custom IDs with sourcesparseAddEntriestoparse-args.tsmodule--verboseflag for debug logging supportCliCommandtypes with command-specific argumentsSummary by CodeRabbit
New Features
Improvements