Support getting remote ref by parsing url #335
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #335 +/- ##
==========================================
+ Coverage 90.19% 90.36% +0.16%
==========================================
Files 48 48
Lines 2602 2626 +24
Branches 536 541 +5
==========================================
+ Hits 2347 2373 +26
+ Misses 255 253 -2 ☔ View full report in Codecov by Sentry. |
📝 WalkthroughWalkthroughThis pull request introduces a new dependency on Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/cli/actions/remoteAction.ts (1)
69-98: Console output vs. logger usage.
Line 81 usesconsole.log(parsedFields);, which may clutter output in production. Consider usinglogger.traceor removing it.Here’s a suggested patch to align with the rest of the logging approach:
const parsedFields = GitUrlParse(remoteValue); - console.log(parsedFields); + logger.trace(parsedFields);package.json (1)
72-72: New dependencies are appropriately declared.
Be sure to monitor these versions as^allows minor and patch updates. If stability is critical, consider pinning to exact versions.Also applies to: 89-89
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.json(2 hunks)src/cli/actions/remoteAction.ts(6 hunks)tests/cli/actions/remoteAction.test.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/cli/actions/remoteAction.ts
[warning] 129-129: src/cli/actions/remoteAction.ts#L129
Added line #L129 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (windows-latest, 23.x)
- GitHub Check: Test (windows-latest, 22.x)
- GitHub Check: Test (windows-latest, 21.x)
- GitHub Check: Build and run (windows-latest, 22.x)
- GitHub Check: Test (windows-latest, 20.x)
- GitHub Check: Test (windows-latest, 19.x)
- GitHub Check: Build and run (windows-latest, 20.x)
- GitHub Check: Test (windows-latest, 18.x)
- GitHub Check: Test (windows-latest, 18.0.0)
🔇 Additional comments (14)
src/cli/actions/remoteAction.ts (10)
4-4: Good addition of a dedicated URL parser library.
Usinggit-url-parseis a reliable choice for robust Git URL parsing and removes the need to maintain a custom regex for complex URL patterns.
26-27: Check for correct error handling flow.
The call toparseRemoteValue(repoUrl)will throw on invalid input, immediately exiting the function. Ensure that higher-level error handlers or logs clearly indicate the failure withinrunRemoteAction.
28-29: Nice fallback assignment for remote branch.
If the user doesn’t specifyremoteBranch, this assignment from the parsed result is straightforward and maintains clarity of logic.
62-67: Validation regex looks correct.
TheVALID_NAME_PATTERNpermits a wide range of alphanumeric, dot, underscore, and hyphen characters, which is suitable for typical GitHub shorthand names. The logic inisValidShorthandis concise.
100-102: Minimal approach to checking validity.
This is a straightforward way to reuseparseRemoteValue. Safe fallback for either a successful parse or a thrown error.
123-123: Informative log message.
Listing the directory path in the log is helpful for debugging—just be sure not to expose sensitive file paths in logs if relevant.
129-129: Add test coverage for cloning error scenario.
This throw is untested according to static analysis. Consider adding a test that simulates a Git clone failure to validate error handling and improve coverage.Would you like help creating a negative test in the existing suite to cover this scenario?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 129-129: src/cli/actions/remoteAction.ts#L129
Added line #L129 was not covered by tests
134-134: Trace log for cleanup is consistent.
This logging is consistent with the approach of providing full context for debugging.
147-147: Good trace log for file copy steps.
No concerns here—this helps pinpoint copy steps in the event of errors.
150-150: Well-defined error throw on file copy failure.
The error is properly wrapped in aRepomixErrorto match the rest of the error handling flow.tests/cli/actions/remoteAction.test.ts (4)
8-8: Ensures parseRemoteValue is exported and tested.
ImportingparseRemoteValuehere centralizes test coverage. Looks good.
58-61: Thorough shorthand format checks.
These test cases validate a variety of alphanumeric/dot/hyphen/underscore combos. Good coverage forparseRemoteValue.
65-66: HTTPS URLs are tested comprehensively.
Confirming both.git-suffixed and non-suffixed forms ensures correct URL modifications inparseRemoteValue.
71-71: SSH URL scenario verified.
Verifying that SSH URLs pass through without.gitadded covers an important use case.
|
Hi, @huy-trn ! I've taken a quick look at the code and it looks great! Just need to clean up those console.log statements and we should be good to go. I've also tested it out and everything works smoothly. Thanks a lot! This is really helpful 🙌 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/cli/actions/remoteAction.ts (2)
71-116: Remove console.log statement.As mentioned in the PR feedback, remove the console.log statement on line 73.
- logger.trace(`Formatting GitHub shorthand: ${remoteValue}`);LGTM! Comprehensive URL parsing implementation.
The function handles various URL formats well and properly extracts branch and commit information.
141-141: Remove unnecessary trailing spaces in log messages.The added trailing spaces in log messages are unnecessary and inconsistent with other logging in the codebase.
- logger.log(`Clone repository: ${url} to temporary directory.${pc.dim(`path: ${directory}`)} `); + logger.log(`Clone repository: ${url} to temporary directory.${pc.dim(`path: ${directory}`)}`); - throw new RepomixError(`Failed to clone repository: ${(error as Error).message} `); + throw new RepomixError(`Failed to clone repository: ${(error as Error).message}`); - logger.trace(`Cleaning up temporary directory: ${directory} `); + logger.trace(`Cleaning up temporary directory: ${directory}`); - logger.trace(`Copying output file from: ${sourcePath} to: ${targetPath} `); + logger.trace(`Copying output file from: ${sourcePath} to: ${targetPath}`); - throw new RepomixError(`Failed to copy output file: ${(error as Error).message} `); + throw new RepomixError(`Failed to copy output file: ${(error as Error).message}`);Also applies to: 147-147, 152-152, 165-165, 168-168
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cli/actions/remoteAction.ts(6 hunks)tests/cli/actions/remoteAction.test.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Test (macos-latest, 22.x)
- GitHub Check: Test (macos-latest, 21.x)
- GitHub Check: Test (windows-latest, 23.x)
- GitHub Check: Test (windows-latest, 22.x)
- GitHub Check: Test (windows-latest, 21.x)
- GitHub Check: Test (windows-latest, 20.x)
- GitHub Check: Test (windows-latest, 19.x)
- GitHub Check: Test (windows-latest, 18.x)
- GitHub Check: Test (windows-latest, 18.0.0)
- GitHub Check: Test coverage
🔇 Additional comments (5)
src/cli/actions/remoteAction.ts (4)
4-4: LGTM! Good choice of dependency and type extension.The
git-url-parselibrary is a well-maintained solution for parsing Git URLs, and theCorrectedGitUrlinterface properly extends the base type to support commit hashes.Also applies to: 12-14
28-32: LGTM! Clean implementation of remote value parsing.The changes improve URL parsing reliability by using the
git-url-parselibrary, and the branch extraction logic is handled correctly.
64-69: LGTM! Well-structured regex pattern.The regex pattern correctly implements GitHub's naming rules, and the use of constants improves readability.
118-125: LGTM! Clean refactor of validation logic.Good use of the new
parseRemoteValuefunction to simplify validation logic.tests/cli/actions/remoteAction.test.ts (1)
56-119: LGTM! Excellent test coverage.The test suite thoroughly covers:
- GitHub shorthand formats
- HTTPS URLs
- SSH URLs
- Branch name extraction
- Commit hash extraction
- Error cases
The test descriptions are clear and the assertions are comprehensive.
|
Thanks so much, @yamadashy! I’ve polished it up a bit. However, I haven’t had the chance to test it with the website yet, but I’ll make sure to do so soon to ensure everything works well together. Please let me know if you have any concerns or if there’s anything else I can do! |
|
@huy-trn # Standard patterns
npm run repomix -- --remote https://github.com/yamadashy/repomix
npm run repomix -- --remote yamadashy/repomix
# Branch name
npm run repomix -- --remote https://github.com/yamadashy/repomix/tree/0.1.x
# Tag name
npm run repomix -- --remote https://github.com/yamadashy/repomix/tree/v0.1.6
# Old commit
npm run repomix -- --remote https://github.com/yamadashy/repomix/commit/9a5985e2de734a388de4f6ed46504070668ffc7bRegarding the website implementation: Since the website/server currently installs Repomix directly from npm, I'll need to release a new version first. I'll handle the website update after merging and releasing this code! Could you also add some example patterns to the |
|
The update to the README.md is on the way! I realized I almost forgot to mention an issue with the current implementation: it doesn't work properly if the branch name contains a slash (/). Unfortunately, I couldn’t find a good way to handle this without an overkill solution. For example: If the branch is feat/git-metrics, the URL to the branch is https://github.com/yamadashy/repomix/tree/feat/git-metrics (1). If we fix this issue, we lost the ability to identify branch names when users submit a URL pointing to a subfolder. In my opinion, supporting slashes in branch names is more important, since without this fix, users won't be able to select the correct branch on the website. There's no --remote-branch CLI option on the website! The fix is on the way! I'd love to hear your thoughts on this. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cli/actions/remoteAction.test.ts (1)
56-126: Add test cases for error scenarios.While the happy path test cases are comprehensive, consider adding these test cases:
- Invalid owner/repo format validation
- Specific error messages
- Edge cases with special characters in branch names
Add these test cases:
test('should throw when the URL is invalid or harmful', () => { expect(() => parseRemoteValue('some random string')).toThrowError(); + expect(() => parseRemoteValue('invalid@owner/repo')).toThrowError('Invalid owner/repo format'); + expect(() => parseRemoteValue('owner/')).toThrowError('Invalid remote repository URL'); + expect(() => parseRemoteValue('https://github.com/-invalid/repo')).toThrowError('Invalid owner/repo format'); }); + +test('should handle special characters in branch names', () => { + expect(parseRemoteValue('https://github.com/user/repo/tree/feature#123')).toEqual({ + repoUrl: 'https://github.com/user/repo.git', + remoteBranch: 'feature#123', + }); + expect(parseRemoteValue('https://github.com/user/repo/tree/release@1.0')).toEqual({ + repoUrl: 'https://github.com/user/repo.git', + remoteBranch: 'release@1.0', + }); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/cli/actions/remoteAction.ts(3 hunks)tests/cli/actions/remoteAction.test.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test (macos-latest, 21.x)
- GitHub Check: Test (windows-latest, 23.x)
- GitHub Check: Test (windows-latest, 22.x)
- GitHub Check: Test (windows-latest, 21.x)
- GitHub Check: Test (windows-latest, 20.x)
- GitHub Check: Test (windows-latest, 19.x)
- GitHub Check: Test (windows-latest, 18.x)
- GitHub Check: Test (windows-latest, 18.0.0)
- GitHub Check: Test coverage
🔇 Additional comments (4)
src/cli/actions/remoteAction.ts (4)
12-14: LGTM! Good type extension.The
CorrectedGitUrlinterface properly extendsGitUrlto include the missingcommitfield, ensuring type safety when accessing commit information.
28-28: Verify branch precedence behavior.The implementation uses
options.remoteBranch || parsedFields.remoteBranch, which means the CLI option takes precedence over the branch specified in the URL. This might be counterintuitive as users explicitly specifying a branch in the URL might expect it to override CLI options.Would you like me to propose an alternative implementation that gives precedence to the branch specified in the URL?
Also applies to: 37-37
58-63: LGTM! Well-defined validation.The implementation correctly validates GitHub shorthand format using a precise regex pattern that aligns with GitHub's repository naming rules.
112-119: LGTM! Good reuse of parseRemoteValue.The implementation elegantly reuses
parseRemoteValuefor validation, ensuring consistent validation rules across the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
166-169: Enhanced Remote Repository ExamplesThe newly added examples clearly demonstrate how users can specify a branch's URL and a commit's URL using the
--remoteoption. This directly addresses the feedback to include additional example patterns in the "Usage" and "Remote Repository Processing" sections.For added clarity, consider including a brief note about handling branch names that contain slashes, as mentioned in the PR discussion. This will help users avoid potential ambiguities when branch names conflict with subfolder URL patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test (macos-latest, 23.x)
- GitHub Check: Test (windows-latest, 23.x)
- GitHub Check: Test (windows-latest, 22.x)
- GitHub Check: Test (windows-latest, 21.x)
- GitHub Check: Test (windows-latest, 20.x)
- GitHub Check: Test (windows-latest, 19.x)
- GitHub Check: Test (windows-latest, 18.x)
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/cli/actions/remoteAction.ts (1)
84-84: 🛠️ Refactor suggestionEnhance error messages with more context.
The error messages could be more helpful by including the problematic values.
- throw new RepomixError('Invalid owner/repo in repo URL'); + throw new RepomixError(`Invalid owner/repo format: "${ownerSlashRepo}". Must match pattern: ${VALID_NAME_PATTERN}/${VALID_NAME_PATTERN}`); - throw new RepomixError('Invalid remote repository URL or repository shorthand (owner/repo)'); + throw new RepomixError(`Failed to parse "${remoteValue}": ${error instanceof Error ? error.message : 'Invalid format'}`);Also applies to: 108-108
🧹 Nitpick comments (2)
src/cli/actions/remoteAction.ts (2)
80-81: Simplify owner/repo extraction.The current extraction of owner/repo is more complex than necessary.
- const ownerSlashRepo = - parsedFields.full_name.split('/').length > 1 ? parsedFields.full_name.split('/').slice(-2).join('/') : ''; + const ownerSlashRepo = parsedFields.full_name;
112-119: Add debug logging to isValidRemoteValue.Consider adding trace logging to help with debugging validation failures.
export const isValidRemoteValue = (remoteValue: string): boolean => { try { parseRemoteValue(remoteValue); + logger.trace(`Valid remote value: ${remoteValue}`); return true; } catch (error) { + logger.trace(`Invalid remote value: ${remoteValue}. Error: ${error instanceof Error ? error.message : 'Unknown error'}`); return false; } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/cli/actions/remoteAction.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test (windows-latest, 23.x)
- GitHub Check: Test (windows-latest, 22.x)
- GitHub Check: Test (windows-latest, 21.x)
- GitHub Check: Test (windows-latest, 20.x)
- GitHub Check: Test (windows-latest, 19.x)
- GitHub Check: Test (windows-latest, 18.x)
- GitHub Check: Test coverage
- GitHub Check: Lint TypeScript
🔇 Additional comments (4)
src/cli/actions/remoteAction.ts (4)
4-4: LGTM! Good type extension.The IGitUrl interface properly extends GitUrl to add support for commit hashes.
Also applies to: 12-14
28-28: LGTM! Good precedence handling.The changes correctly prioritize the CLI option
--remote-branchover the branch parsed from the URL.Also applies to: 37-37
59-63: LGTM! Well-defined validation pattern.The regex pattern correctly validates GitHub repository naming rules.
89-94: Review branch name handling with slashes.Per the PR comments discussion about branch names containing slashes, the current implementation might need adjustment to handle these cases correctly.
Let's verify how git-url-parse handles various branch name patterns:
|
@huy-trn |
|
@huy-trn I've also tested it with a branch name containing slashes, and it works perfectly: npm run repomix -- --remote https://github.com/yamadashy/repomix/tree/feat/git-metricsThe README updates are clear and well-organized, showing both the Let me merge and release this. 🚀 |
|
nice 😄 |
|
@huy-trn 
Thank you very much for your excellent contributions! |
This PR helps resolving #311, adding the ability to parse the cli option value --remote (remote repository url) to get branch/commit/tag.
This PR does not modify any code of the website.
For example:
Works exactly the same as:
New dependency:
Checklist
npm run testnpm run lint