Migrate to VS Code Git API and reduce external dependencies #53
Conversation
Use vscode.env.clipboard instead of external dependency clipboardy Use vscode.git extension API more instead of .git/config directly (.git/config still used for default branch lookup) Stop using npm default-branch because it does not handle private repos Stop using npm git-branch because available via vscode.git extension Stop using git-rev-sync because available via vscode.git extension Stop using parse-git-config because overkill for simple main/master default branch name lookups Updated all dependency versions to latest Updated vscode engine to 1.74.0 Updated launch.json version to 0.2.0 Add support for opening parent workspace directories with multiple repo subdirectories Refactored and expanded test coverage Fixes #22 - detect private repo default branch Fixes #40 - update command title, master => main
WalkthroughThis pull request introduces a comprehensive overhaul of the VSCode Copy GitHub URL extension, focusing on modernizing the codebase, improving error handling, and updating dependencies. The changes include migrating from legacy ESLint configuration, updating testing infrastructure, enhancing Git repository detection, and refactoring URL generation logic. The extension now leverages VSCode's Git API more directly and provides more robust handling of different repository scenarios. Changes
Assessment against linked issues
Poem
🪧 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: 4
🔭 Outside diff range comments (1)
README.md (1)
Line range hint
21-25: Update example URL to use "main" instead of "master".To align with the PR objective of updating from "master" to "main" branch naming, the example URL should be updated.
-Example: [`https://github.com/differentmatt/vscode-copy-github-url/blob/master/extension.js#L4`](https://github.com/differentmatt/vscode-copy-github-url/blob/master/extension.js#L4) +Example: [`https://github.com/differentmatt/vscode-copy-github-url/blob/main/extension.js#L4`](https://github.com/differentmatt/vscode-copy-github-url/blob/main/extension.js#L4)
🧹 Nitpick comments (13)
src/main.js (3)
21-55: Consider clarifying error handling
When the Git extension is missing or inactive, the error is thrown, but the user could benefit from a more instructive message. You might surface a user-facing notification or steps to enable/install the Git extension, improving the user experience.
183-183: Remove unnecessarycontinuestatement
Static analysis indicates that thiscontinueis redundant. Removing it simplifies control flow without changing behavior.- continue // Try next remote if this one fails🧰 Tools
🪛 Biome (1.9.4)
[error] 183-183: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
208-267: Potential timing pitfall
Using a hard-coded 5-second timeout for repository discovery may cause unexpected failures in larger or more complex projects. Consider making this timeout configurable or dynamically adjusting based on repository size.test/unit/github.test.js (1)
59-99: Timeout length
A 10-second timeout for repository checks might be excessive, prolonging test runs. If appropriate, consider reducing it to 5 seconds or introducing conditional retries to avoid slowing down your overall CI.eslint.config.mjs (1)
12-19: JSX parser settings
Parser options with JSX enabled can be handy for React or similar frameworks. If this project remains purely Node-based, consider disabling JSX to avoid confusion.webpack.config.js (1)
1-1: Remove redundant'use strict'
Webpack and modern JavaScript modules enforce strict mode by default. This directive can be removed.- 'use strict'🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
src/extension.js (2)
30-38: Good use of async/await with proper error handling.
This pattern helps maintain readability and ensures that the catch block can forward errors tohandleError. Consider adding user feedback in caseurlis falsy.if (url) { await vscode.env.clipboard.writeText(url) vscode.window.showInformationMessage('GitHub URL copied to clipboard!') } else { + vscode.window.showWarningMessage('No URL available to copy.') }
43-46: Consider a more descriptive property name thandefault.
Using a property nameddefaultmight cause confusion in future maintenance. A more descriptive key (e.g.,useDefaultBranch) may increase clarity.-const defaultDisposable = vscode.commands.registerCommand('extension.gitHubUrlDefault', generateCommandBody({ default: true })) +const defaultDisposable = vscode.commands.registerCommand('extension.gitHubUrlDefault', generateCommandBody({ useDefaultBranch: true }))test/helpers/mockFactory.js (1)
19-56: Robust mock generator with ample configuration flexibility.
ThegetVsCodeMockfunction is a well-executed approach for simulating the VSCode environment. Consider adding checks in caseprojectDirectoryis undefined, and ensuresepdefaults to the environment’s path separator if none is provided.function getVsCodeMock (options) { + if (!options.projectDirectory) { + throw new Error('projectDirectory is required for getVsCodeMock.') + } const projectRoot = options.projectDirectory ... }test/helpers/stubs.js (1)
31-83: Workspace stubbing approach is comprehensive.
Stubbing workspace folders, path operations, and normalizations is well structured. Just be mindful of complicated path scenarios (e.g., symbolic links).test/unit/url.test.js (1)
30-47: Useassert.strictEqualfor stricter checks.
Mocha’sassert.equalperforms type-coercive equality checks. Preferassert.strictEqualto avoid subtle type issues.-assert.equal( +assert.strictEqual( url, 'https://github.com/foo/bar-baz/blob/test-branch/subdir1/subdir2/myFileName.txt#L5' )test/index.js (1)
1-1: Synchronous file matching is simpler, but consider async for large test suites.
UsingglobSyncimproves clarity, but for very large suites, asynchronous operations may be more performant. If the suite grows, switching back to async might ease performance constraints.Also applies to: 3-5, 7-32, 37-37
README.md (1)
Line range hint
5-25: Consider adding platform-specific keyboard shortcuts.The keyboard shortcuts (e.g.,
Ctrl+L C) should include macOS alternatives (e.g.,Cmd+L C) for better cross-platform documentation.
📜 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 (18)
.eslintrc.json(0 hunks).vscode/launch.json(2 hunks)README.md(1 hunks)eslint.config.mjs(1 hunks)package.json(3 hunks)src/extension.js(1 hunks)src/main.js(1 hunks)test/extension.test.js(0 hunks)test/helpers/gitApiFactory.js(1 hunks)test/helpers/mockFactory.js(1 hunks)test/helpers/stubs.js(1 hunks)test/index.js(1 hunks)test/integration/commands.test.js(1 hunks)test/runTest.js(1 hunks)test/unit/github.test.js(1 hunks)test/unit/url.test.js(1 hunks)vsc-extension-quickstart.md(0 hunks)webpack.config.js(2 hunks)
💤 Files with no reviewable changes (3)
- test/extension.test.js
- .eslintrc.json
- vsc-extension-quickstart.md
🧰 Additional context used
🪛 GitHub Actions: CI Tests
test/unit/url.test.js
[error] 27-27: TypeError: Cannot read properties of undefined (reading 'setTestEnvironment')
test/unit/github.test.js
[error] 29-29: TypeError: Cannot read properties of undefined (reading 'setTestEnvironment')
test/integration/commands.test.js
[error] 26-26: TypeError: Cannot read properties of undefined (reading 'setTestEnvironment')
🪛 Biome (1.9.4)
webpack.config.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
src/main.js
[error] 183-183: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🔇 Additional comments (27)
src/main.js (2)
9-12: Use "isTestEnvironment" with caution
Consider verifying that the "isTestEnvironment" flag is consistent across your modules to avoid unexpected or partial coverage in integration tests.
64-135: Layered fallback strategy
The approach of looking in.git/config, then runninggit branch -r, and finally using the fallback configuration is commendably robust. This covers various setups and gracefully handles corner cases.test/unit/github.test.js (2)
101-116: Robust testing for SSH-based remotes
This test verifies SSH URLs properly, which is crucial for users who use SSH-based Git remotes. Good job covering alternative remote formats.
494-505: Enterprise domain coverage
This test ensures that enterprise or custom domain URLs convert correctly, which is vital for corporate environments. Good that you're not restricting to GitHub-only domains.eslint.config.mjs (1)
1-3: Neat import of global definitions
Importing and merging globals fromglobalsis a clean approach. This configuration is succinct and easy to maintain.webpack.config.js (2)
8-11: Multiple entry points
Defining separate entry points for extension and test bundles is a smart way to keep build artifacts organized and debugging simpler.
19-21: Externals for test dependencies
Markingmochaas an external avoids bundling test libraries that aren't needed in production. This keeps the output lightweight.test/helpers/gitApiFactory.js (2)
16-30: Flexible test Git API
Accepting anoptionsobject allows for customizable commits, branches, and remotes, greatly enhancing test coverage and scenarios.
32-35: Custom repository discovery hook
Providing a customizableonDidOpenRepositoryfunction is valuable for simulating asynchronous repository detection in tests, broadening scenario coverage.src/extension.js (3)
13-26: Centralized error handling is well-structured.
ThehandleErrorfunction neatly consolidates logging and error messages. This approach simplifies error handling and improves user feedback.
53-53: Validate whether returningmainis necessary.
Returningmainmakes the entire module directly accessible to the caller. If this is intended, keep it; otherwise, consider a narrower or more focused return.
56-56: ES modules transition looks good.
Usingmodule.exports = { activate }is appropriate if you need CommonJS compatibility. No concerns here.test/helpers/mockFactory.js (1)
58-58: Export strategy is straightforward.
No issues with exportinggetVsCodeMockhere.test/helpers/stubs.js (2)
9-22:stubGitExtensioneffectively mocks the Git extension.
This is a clean approach to provide a controlled environment for testing. Ensure you cover edge cases where users may not have a Git extension installed.
85-88: Exports for test utilities are clear.
No concerns here; this keeps test stubs modular and maintainable.test/integration/commands.test.js (5)
29-59: Integration test forextension.gitHubUrl.
Excellent coverage of verifying the command copies the correct URL.
60-94:extension.gitHubUrlPermatest ensures commit-specific URLs.
Ensuring that the clipboard receives a commit-based URL is a solid test. No concerns here.
96-130: Test for default branch handling.
This thoroughly checks how the command interacts with the default branch logic. The stubbedgetDefaultBranchmethod is a good technique for verifying expected behavior.
131-131: Test suite ends gracefully.
No issues here. The structure is well organized.
12-22: Confirm extension ID alignment.
If the extension is actually published under a different ID (e.g.,mattlott.copy-github-urlvs.differentmatt.copy-github-url), ensure these match. Mismatches can causeextensionorextension.exportsto be undefined, leading to errors such asTypeError: Cannot read properties of undefined.✅ Verification successful
Extension ID matches the package.json configuration
The extension ID
mattlott.copy-github-urlused in the test matches exactly with the configuration in package.json where:
"publisher": "mattlott""name": "copy-github-url"These combine to form the extension ID
mattlott.copy-github-url, confirming proper alignment.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the `package.json` publisher and name match "mattlott.copy-github-url" exactly. rg '"publisher":' rg '"name":'Length of output: 383
test/runTest.js (1)
1-2: Migration to@vscode/test-electronis sound.
The updates for running tests via@vscode/test-electronlook good. SettingNODE_ENV='test'ensures a clear environment for test execution, and disabling other extensions prevents unintended interactions.Also applies to: 6-7, 8-9, 11-15, 17-18, 22-22
.vscode/launch.json (1)
3-3: Validate tests discovery after changing--extensionTestsPath.
By changing the tests path fromtest/indextotest, ensure that all test files are still discovered. If any file is not being picked up, consider adjusting the pattern or the path.Also applies to: 22-23, 24-24
✅ Verification successful
The previous command might have failed because either the test files don't exist or they're in a different location. Let's verify the test file structure and patterns first.
Test discovery is working correctly under the new path structure
The verification shows that the test files are properly organized and discoverable:
- Integration tests in
test/integration/- Unit tests in
test/unit/- Helper files in
test/helpers/- All test files use standard test patterns (
suite,test,it,describe)The change from
test/indextotestin--extensionTestsPathis safe as:
test/index.jsexists and serves as the entry point- All test files are properly structured under the
testdirectory- Test patterns are correctly used across all test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verifying if test files remain discoverable under the new path rg 'describe\(|suite\(' . --glob '**/test/*.js' -A 5Length of output: 54
Script:
#!/bin/bash # Let's first check what test files exist in the repository fd -e js -p "test/" # Then check for test patterns in all JavaScript files under test directory rg "describe\(|suite\(|test\(|it\(" "test/" -t jsLength of output: 5711
README.md (1)
1-4: LGTM! Clear organization of commands.The restructuring into three separate commands with a clear introduction improves documentation clarity.
package.json (4)
81-90: LGTM! Clean migration to Standard.js.The migration from ESLint to Standard.js with proper global configurations for test frameworks looks good. This aligns with the PR objective of reducing external dependencies while maintaining code quality.
107-109: LGTM! Reduced external dependencies.Successfully reduced external dependencies to only
github-url-from-git, which aligns with the PR objective of minimizing external dependencies.
19-19: Verify compatibility with VS Code engine update.The VS Code engine requirement has been updated from ^1.59.0 to ^1.74.0. This is a significant version jump that might affect users with older VS Code versions.
✅ Verification successful
VS Code engine update is safe to proceed
The extension uses only stable VS Code APIs that have remained consistent across versions:
- Basic extension activation and command registration
- Window APIs (
showErrorMessage,showInformationMessage)- Workspace configuration
- Git extension API (version 1)
- Editor and document APIs
- Clipboard API
- Extension mode checks
No breaking changes between 1.59.0 and 1.74.0 affect the functionality of this extension. The update is primarily for maintenance purposes and doesn't introduce compatibility risks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check VS Code API changes between versions 1.59.0 and 1.74.0 that might affect this extension # Get the VS Code API changes curl -s "https://raw.githubusercontent.com/microsoft/vscode-docs/main/api/references/vscode-api-1.74.md" | grep -A 5 "### Breaking Changes"Length of output: 139
Script:
#!/bin/bash # Let's analyze the extension's usage of VS Code API to understand potential compatibility issues # First, let's find all TypeScript/JavaScript files that might use VS Code API fd -e ts -e js | xargs rg -l "vscode" # Then, let's look for specific VS Code API usage patterns rg "require\(['\"]vscode['\"]\)" -A 2 rg "from ['\"]vscode['\"]\)" -A 2 # Let's also check the extension's activation events and contribution points cat package.json | jq '.activationEvents, .contributes'Length of output: 3077
Script:
#!/bin/bash # Let's examine the actual VS Code API usage in the source code to check for any potentially breaking changes # Check main extension code for VS Code API usage cat src/extension.js src/main.js # Also check the package.json for the complete list of dependencies and dev dependencies cat package.json | jq '.dependencies, .devDependencies'Length of output: 12549
93-105: Verify impact of dependency updates.Significant updates to development dependencies have been made. While updating to latest versions is good, we should verify:
- Compatibility with the current codebase
- No breaking changes that could affect the build process
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/ci-tests.yml(2 hunks)package.json(3 hunks)webpack.config.js(2 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/ci-tests.yml
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Biome (1.9.4)
webpack.config.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (7)
.github/workflows/ci-tests.yml (2)
23-30: Well-structured system dependencies installationGood practice to differentiate between local and CI environments. The conditional installation ensures that local development with
acthas all required dependencies while keeping the CI environment lean.
1-3: Helpful local testing instructionsGreat addition of instructions for running the workflow locally using
act. This improves the developer experience by making it easier to test workflow changes locally before pushing.webpack.config.js (2)
8-15: Well-structured conditional entry pointsGood separation of concerns between test and production builds using NODE_ENV. This allows for better testing integration while keeping the production build clean.
37-39: Smart build optimization strategyGood practice to disable minification for test builds while keeping it enabled for production. This improves debugging experience during testing while maintaining optimal bundle size for production.
package.json (3)
40-40: Updated command title aligns with modern conventionsGood update from "Master" to "Main" in the command title, addressing issue #40 and aligning with current Git branch naming conventions.
94-109: Well-organized dependency updatesExcellent cleanup of dependencies:
- Removed unnecessary packages
- Updated to latest versions
- Added required testing dependencies
- Simplified production dependencies
This aligns well with the PR objective to reduce external dependencies.
19-20: Verify VS Code API compatibilityThe VS Code engine requirement has been updated to ^1.74.0. Please ensure that all VS Code API features used in the codebase are compatible with this version.
Run this script to check API usage:
✅ Verification successful
Let me analyze the VS Code API usage found in the codebase to verify compatibility with version 1.74.0.
The search results show the following VS Code API features being used:
Extensions API:
vscode.extensions.getExtensionvscode.ExtensionMode.Testvscode.ExtensionKind.WorkspaceWindow API:
vscode.window.showErrorMessagevscode.window.showInformationMessagevscode.window.activeTextEditorWorkspace API:
vscode.workspace.getConfigurationvscode.workspace.getWorkspaceFoldervscode.workspace.workspaceFoldersCommands API:
vscode.commands.registerCommandvscode.commands.executeCommandEnvironment API:
vscode.env.clipboard.writeTextURI API:
vscode.Uri.fileAll these APIs were introduced well before VS Code 1.74.0 (December 2022) and have remained stable since then. These are core VS Code APIs that haven't undergone breaking changes. The usage patterns in the code are compatible with version 1.74.0.
VS Code API usage is compatible with v1.74.0
All VS Code APIs used in the codebase are core features that were stable and available in version 1.74.0. No compatibility issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check VS Code API usage compatibility # Search for vscode.* API calls and verify against v1.74.0 API docs # Search for VS Code API usage rg "vscode\." --type jsLength of output: 5473
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/unit/url.test.js (1)
17-27:⚠️ Potential issueFix undefined
setTestEnvironmentcalls causing test failures.The test setup and teardown are failing due to undefined
setTestEnvironmentcalls. This matches the pipeline error: "TypeError: Cannot read properties of undefined (reading 'setTestEnvironment')".setup(async () => { sandbox = sinon.createSandbox() extension = await vscode.extensions.getExtension('mattlott.copy-github-url') _main = await extension.activate() - _main.setTestEnvironment(true) }) teardown(() => { sandbox.restore() - _main.setTestEnvironment(false) })
🧹 Nitpick comments (8)
test/unit/url.test.js (2)
29-241: Enhance test coverage and improve test structure.While the test suite is comprehensive for happy paths, consider the following improvements:
Add error case tests:
- Invalid file paths
- Missing git repository
- Network failures
- Invalid line numbers
Reduce code duplication:
- Extract common setup into helper functions
- Create shared test fixtures
Standardize assertions:
- Use
assert.strictEqualconsistently instead of mixing withassert.equal- Add descriptive error messages to all assertions
Example helper function to reduce duplication:
function setupTestEnvironment({ pathSeparator = '/', projectDirectory, filePath, startLine, endLine, branch = 'test-branch', commit = null }) { const vsCodeMock = getVsCodeMock({ startLine, endLine: endLine || startLine, projectDirectory, filePath, sep: pathSeparator }) stubWorkspace(sandbox, _main, projectDirectory, pathSeparator) stubGitExtension(sandbox, { branch, commit, projectDirectory, repoUrl: 'https://github.com/foo/bar-baz.git' }) return vsCodeMock }
174-210: Add test cases for additional special character scenarios.While the special character tests are thorough, consider adding test cases for:
- Unicode characters (e.g., 你好.js, über.js)
- Emoji in file names (e.g., 👋.js)
- Maximum length file paths
- File names with leading/trailing spaces
Example test case:
test('getGithubUrl should handle unicode and emoji in file names', async function () { const projectDirectory = '/foo' const vsCodeMock = getVsCodeMock({ startLine: 0, endLine: 1, projectDirectory, filePath: '你好👋über.js' }) stubWorkspace(sandbox, _main, projectDirectory) stubGitExtension(sandbox, { commit: '75bf4eea9aa1a7fd6505d0d0aa43105feafa92ef', projectDirectory }) const url = await _main.getGithubUrl(vsCodeMock.window.activeTextEditor, { perma: true }) assert.strictEqual( url, 'https://github.com/foo/bar-baz/blob/75bf4eea9aa1a7fd6505d0d0aa43105feafa92ef/%E4%BD%A0%E5%A5%BD%F0%9F%91%8B%C3%BCber.js#L1-L2', 'Should properly encode unicode and emoji characters' ) })test/unit/github.test.js (6)
19-24: Add error handling for extension activation.The setup could be more robust by handling potential activation failures.
setup(async () => { sandbox = sinon.createSandbox() extension = await vscode.extensions.getExtension('mattlott.copy-github-url') + if (!extension) { + throw new Error('Extension not found') + } _main = await extension.activate() + if (!_main) { + throw new Error('Extension activation failed') + } _main.setTestEnvironment(true) })
31-40: Enhance test coverage with additional assertions.The repository test could be more thorough by validating additional repository properties.
const repository = await _main.getRepository(vsCodeMock.extensions.getExtension().exports.getAPI(), vsCodeMock.window.activeTextEditor) assert.strictEqual(repository.state.remotes[0].fetchUrl, 'https://github.com/user/repo.git') +assert.strictEqual(repository.rootUri.fsPath, '/Users/user1/GitHub/project1') +assert.ok(repository.state.remotes.length === 1, 'Should have exactly one remote') +assert.strictEqual(repository.state.remotes[0].name, 'origin', 'Remote name should be origin')
57-98: Strengthen error assertions.The error handling tests could be more specific about the expected error messages.
try { await urlPromise assert.fail('Should have thrown an error') } catch (error) { - assert(error.message.includes('Timeout waiting for repository')) + assert.strictEqual( + error.message, + 'Timeout waiting for repository discovery after 5000ms', + 'Should throw specific timeout error message' + ) }
361-416: Add negative test cases for URL parsing.The URL format test cases should include invalid URL formats to ensure proper error handling.
const testCases = [ // ... existing test cases ... + // Invalid URL formats + { + input: 'not-a-git-url', + expected: null, + shouldThrow: true + }, + { + input: 'git@github.com:invalid-format', + expected: null, + shouldThrow: true + }, + { + input: 'https://', + expected: null, + shouldThrow: true + } ] await Promise.all(testCases.map(async ({ input, expected, shouldThrow }) => { const repository = { state: { HEAD: { name: 'main' }, refs: [], remotes: [{ name: 'origin', fetchUrl: input }] } } + if (shouldThrow) { + await assert.rejects( + async () => await _main.getGithubUrlFromRemotes(repository), + /Invalid Git URL format/ + ) + return + } const url = await _main.getGithubUrlFromRemotes(repository) assert.strictEqual(url, expected, `Failed to handle URL format: ${input}`) }))
594-705: Add edge cases for repository discovery.The multiple repository tests should include edge cases such as nested repositories and symlinks.
Consider adding the following test cases:
- Nested Git repositories (repository within another repository)
- Symlinked directories
- Repository paths with special characters
- Case-sensitive vs case-insensitive filesystem scenarios
Example for nested repositories:
test('getRepository should handle nested repositories', async function () { const parentRepoPath = '/workspace/parent-repo'; const nestedRepoPath = '/workspace/parent-repo/nested/child-repo'; const vsCodeMock = getVsCodeMock({ projectDirectory: parentRepoPath, filePath: 'nested/child-repo/src/file.js' }); // Create parent and nested repositories const parentRepo = { rootUri: vscode.Uri.file(parentRepoPath), state: { remotes: [{ name: 'origin', fetchUrl: 'https://github.com/user/parent.git' }] } }; const nestedRepo = { rootUri: vscode.Uri.file(nestedRepoPath), state: { remotes: [{ name: 'origin', fetchUrl: 'https://github.com/user/child.git' }] } }; const gitApi = { repositories: [parentRepo, nestedRepo], onDidOpenRepository: () => ({ dispose: () => {} }) }; const result = await _main.getRepository(gitApi, vsCodeMock.window.activeTextEditor); assert.strictEqual(result, nestedRepo, 'Should find the most specific repository'); });
1-705: Consider architectural improvements for test maintainability.The test suite is well-structured but could benefit from the following architectural improvements:
- Extract common test setup into shared fixtures
- Group related test cases using
describeblocks- Create helper functions for commonly used assertions
- Consider using parameterized tests for URL format variations
Example structure:
describe('GitHub Integration', () => { describe('Repository Discovery', () => { // Repository related tests }); describe('URL Parsing', () => { // URL format related tests }); describe('Error Handling', () => { // Error handling related tests }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/integration/commands.test.js(1 hunks)test/unit/github.test.js(1 hunks)test/unit/url.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/integration/commands.test.js
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
webpack.config.js (1)
Line range hint
24-33: Consider adding explicit loader configurations.While the current configuration works, consider adding explicit loader configurations for different file types (e.g., JavaScript, JSON) to ensure consistent handling across environments.
module: { rules: [ { + test: /\.js$/, + use: 'babel-loader', exclude: /node_modules/ - } + }, + { + test: /\.json$/, + type: 'json' + } ] }.github/workflows/ci-tests.yml (4)
1-3: Enhance local development setup instructions.While the command for running locally is provided, it would be helpful to add more comprehensive setup instructions:
# To run locally: +# Prerequisites: +# 1. Install act: https://github.com/nektos/act#installation +# 2. Ensure Docker is running +# # act -W .github/workflows/ci-tests.yml --container-architecture linux/amd64 +# Note: First run may take time to download container images
19-21: Consider pinning specific Node.js version.While using Node.js 18 is correct, consider pinning to a specific version (e.g., '18.19.0') to ensure consistent builds across environments.
with: - node-version: '18' + node-version: '18.19.0' cache: 'npm'
23-30: Optimize system dependencies installation.The current implementation can be improved in several ways:
- Pin package versions for consistency
- Use a base set of packages with additional ones for local development
- Simplify the conditional logic
- name: Install system dependencies run: | sudo apt-get update + # Base packages required for all environments + BASE_PACKAGES="xvfb=2.* libnss3=2:* libgbm1=*" + # Additional packages required for local development + LOCAL_PACKAGES="libasound2=1.* libatk1.0-0=2.* libatk-bridge2.0-0=2.* libcups2=2.* libdrm2=2.* libgtk-3-0=3.*" + if [ "$ACT" = "true" ]; then - sudo apt-get install -y xvfb libnss3 libasound2 libatk1.0-0 libatk-bridge2.0-0 libcups2 libdrm2 libgtk-3-0 libgbm1 + sudo apt-get install -y $BASE_PACKAGES $LOCAL_PACKAGES else - sudo apt-get install -y xvfb libnss3 libgbm1 + sudo apt-get install -y $BASE_PACKAGES fi
38-39: Enhance test execution error handling.Consider adding timeout and retry mechanisms to handle potential test flakiness in CI:
- name: Run tests - run: xvfb-run npm run test + run: | + # Add retry mechanism for flaky tests + max_attempts=3 + attempt=1 + until xvfb-run --server-args="-screen 0 1024x768x24" npm run test || [ $attempt -eq $max_attempts ]; do + echo "Test attempt $attempt failed. Retrying..." + attempt=$(( attempt + 1 )) + sleep 5 + done + if [ $attempt -eq $max_attempts ]; then + echo "Tests failed after $max_attempts attempts" + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/ci-tests.yml(2 hunks)webpack.config.js(2 hunks)
🔇 Additional comments (6)
webpack.config.js (5)
1-1: LGTM! Good use of TypeScript configuration type.The path module import and TypeScript configuration type annotation provide good maintainability and type safety.
Also applies to: 3-3
14-19: LGTM! Proper output configuration for multiple entry points.The output configuration correctly supports multiple entry points with dynamic naming and appropriate library target for VS Code extensions.
20-23: LGTM! Appropriate external dependencies configuration.The externals configuration correctly excludes VS Code and Mocha from bundling, which is appropriate for VS Code extensions and test environments.
34-37: LGTM! Appropriate build optimization strategy.The configuration correctly handles source maps and disables minification for test builds, which is ideal for debugging and testing scenarios.
6-13: Verify test entry point configuration.The conditional entry point configuration is well-structured and supports separate test builds. However, let's verify that the test entry point is properly configured.
✅ Verification successful
Test entry point configuration is properly set up
The test entry point at
test/index.jsis correctly configured:
- Sets up the test environment by setting
NODE_ENVto 'test'- Uses Mocha test framework with TDD UI
- Properly discovers and runs all test files matching the pattern
**/**.test.js- Exports a
runfunction that executes the test suite and handles failures appropriately🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test entry point configuration # Check if test/index.js exists and exports the necessary test setup fd -t f "index.js" test/ -x cat {}Length of output: 791
.github/workflows/ci-tests.yml (1)
39-39: Add newline at end of file.Add a newline character at the end of the file to comply with POSIX standards.
run: xvfb-run npm run test +
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
test/helpers/mockFactory.js (4)
3-18: Enhance return type documentation.The JSDoc documentation is thorough, but could be improved for better clarity:
- * @returns {Object} An `vscode` alike object + * @returns {Object} A mock VSCode API object containing workspace, window, extensions, and authentication properties
22-24: Consider making the default file path more configurable.The hardcoded default path
'subdir1/subdir2/myFileName.txt'might be too specific. Consider making this configurable or using a more generic default.- : [projectRoot, 'subdir1', 'subdir2', 'myFileName.txt'].join(options.sep || '/') + : [projectRoot, 'default.txt'].join(options.sep || '/')
54-54: Consider making the mock authentication token configurable.The hardcoded 'fake-token' might not be suitable for all test scenarios. Consider making this configurable through the options parameter.
- getSession: async () => ({ accessToken: 'fake-token' }) + getSession: async () => ({ accessToken: options.accessToken || 'fake-token' })Don't forget to update the JSDoc to include the new option:
* @param {String} [options.accessToken] Mock authentication token
1-59: Well-structured mock factory implementation.The mock factory effectively supports the PR's objective of migrating to VSCode's Git API by providing a comprehensive test environment. The implementation is thorough and follows good practices for test helpers.
Consider adding more mock factory functions in this file for other common VSCode API objects that might be needed in tests, following the same pattern of configurability through options.
test/index.js (1)
27-30: Improve error handling.The current error handling includes a redundant console.error before rejection, which might interfere with proper error reporting in test runners.
Simplify error handling:
- } catch (err) { - console.error(err) - reject(err) - } + } catch (err) { + reject(err) + }src/main.js (3)
31-33: Ensure cross-platform path handling.While
path.relative()handles the path separation correctly, it's good practice to explicitly handle Windows paths to ensure consistent URL generation.Consider using the following implementation:
- let relativePath = path.relative(repository.rootUri.fsPath, document.uri.fsPath) - relativePath = module.exports.normalizePathForGitHub(relativePath) + const relativePath = path.relative(repository.rootUri.fsPath, document.uri.fsPath) + .split(path.sep) + .join('/') + .split(' ') + .join('%20')
77-120: Refactor retry logic and improve error handling.The retry logic uses magic numbers and could be more configurable. Also, the error handling could be improved.
Consider these improvements:
+ const MAX_RETRIES = 3 + const RETRY_DELAY = 500 try { let attempts = 0 - while (attempts < 3) { // Fewer retries since this is less common + while (attempts < MAX_RETRIES) { try { const defaultBranch = await new Promise((resolve, reject) => { cp.exec('git branch -r', { cwd: repository.rootUri.fsPath }, (err, stdout) => { if (err) { - reject(err) + reject(new Error(`Failed to execute git branch -r: ${err.message}`)) return } // ... rest of the code ... }) }) if (defaultBranch) return defaultBranch } catch (error) { - if (attempts === 2) throw error // Re-throw on final attempt + if (attempts === MAX_RETRIES - 1) { + throw new Error(`Failed to get default branch after ${MAX_RETRIES} attempts: ${error.message}`) + } } finally { - await new Promise(resolve => setTimeout(resolve, 500)) + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)) attempts++ } }
197-199: Enhance path normalization for special characters.The current implementation handles some special characters but could be more comprehensive.
Consider handling more special characters:
- return inputPath.split(pathSeparator).map((p) => encodeURI(p).replace('#', '%23').replace('?', '%3F')).join('/') + return inputPath.split(pathSeparator) + .map((p) => encodeURIComponent(p) + .replace(/[!'()*]/g, c => `%${c.charCodeAt(0).toString(16).toUpperCase()}`)) + .join('/')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/ci-tests.yml(2 hunks)README.md(1 hunks)eslint.config.mjs(1 hunks)src/main.js(1 hunks)test/helpers/mockFactory.js(1 hunks)test/index.js(1 hunks)test/integration/commands.test.js(1 hunks)test/unit/url.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- eslint.config.mjs
- test/unit/url.test.js
- .github/workflows/ci-tests.yml
- test/integration/commands.test.js
🧰 Additional context used
🪛 Biome (1.9.4)
test/index.js
[error] 15-31: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
🪛 GitHub Actions: CI Tests
test/index.js
[error] 15-15: Promise executor functions should not be async. (no-async-promise-executor)
🔇 Additional comments (6)
test/helpers/mockFactory.js (2)
1-1: LGTM!The import statement correctly imports the required dependency for creating Git API mocks.
59-59: LGTM!The export statement is clean and follows the appropriate module export pattern.
test/index.js (2)
1-5: LGTM! Well-structured imports and environment setup.The environment setup and imports are well-organized, following good practices:
- Setting NODE_ENV for test isolation
- Using modern ES6 destructuring for glob
- Following conventional import ordering
33-36: LGTM! Clean module exports.The exports use modern object shorthand notation and are well-structured.
src/main.js (2)
1-12: LGTM! Clean imports and test setup.The module imports are appropriate for the functionality, and the test environment setup is well-implemented.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
271-278: LGTM! Clean and well-organized exports.All necessary functions are exported, and the inclusion of the path module for testing purposes is appropriate.
test/index.js
Outdated
| return new Promise(async (resolve, reject) => { | ||
| try { | ||
| const files = await glob('**/**.test.js', { cwd: testsRoot }) | ||
| files.forEach(f => mocha.addFile(path.resolve(testsRoot, f))) | ||
|
|
||
| try { | ||
| // Run the mocha test | ||
| mocha.run(failures => { | ||
| if (failures > 0) { | ||
| e(new Error(`${failures} tests failed.`)); | ||
| } else { | ||
| c(); | ||
| } | ||
| }); | ||
| } catch (err) { | ||
| console.error(err); | ||
| e(err); | ||
| } | ||
| }); | ||
| }); | ||
| mocha.run(failures => { | ||
| if (failures > 0) { | ||
| reject(new Error(`${failures} tests failed.`)) | ||
| } else { | ||
| resolve() | ||
| } | ||
| }) | ||
| } catch (err) { | ||
| console.error(err) | ||
| reject(err) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Fix async Promise executor anti-pattern.
Using an async function as a Promise executor is an anti-pattern that can lead to memory leaks and unhandled promise rejections. This is flagged by both static analysis and CI pipeline.
Refactor the code to handle async operations properly:
- return new Promise(async (resolve, reject) => {
- try {
- const files = await glob('**/**.test.js', { cwd: testsRoot })
- files.forEach(f => mocha.addFile(path.resolve(testsRoot, f)))
-
- mocha.run(failures => {
- if (failures > 0) {
- reject(new Error(`${failures} tests failed.`))
- } else {
- resolve()
- }
- })
- } catch (err) {
- console.error(err)
- reject(err)
- }
- })
+ return glob('**/**.test.js', { cwd: testsRoot })
+ .then(files => {
+ files.forEach(f => mocha.addFile(path.resolve(testsRoot, f)))
+ return new Promise((resolve, reject) => {
+ mocha.run(failures => {
+ if (failures > 0) {
+ reject(new Error(`${failures} tests failed.`))
+ } else {
+ resolve()
+ }
+ })
+ })
+ })📝 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.
| return new Promise(async (resolve, reject) => { | |
| try { | |
| const files = await glob('**/**.test.js', { cwd: testsRoot }) | |
| files.forEach(f => mocha.addFile(path.resolve(testsRoot, f))) | |
| try { | |
| // Run the mocha test | |
| mocha.run(failures => { | |
| if (failures > 0) { | |
| e(new Error(`${failures} tests failed.`)); | |
| } else { | |
| c(); | |
| } | |
| }); | |
| } catch (err) { | |
| console.error(err); | |
| e(err); | |
| } | |
| }); | |
| }); | |
| mocha.run(failures => { | |
| if (failures > 0) { | |
| reject(new Error(`${failures} tests failed.`)) | |
| } else { | |
| resolve() | |
| } | |
| }) | |
| } catch (err) { | |
| console.error(err) | |
| reject(err) | |
| } | |
| }) | |
| return glob('**/**.test.js', { cwd: testsRoot }) | |
| .then(files => { | |
| files.forEach(f => mocha.addFile(path.resolve(testsRoot, f))) | |
| return new Promise((resolve, reject) => { | |
| mocha.run(failures => { | |
| if (failures > 0) { | |
| reject(new Error(`${failures} tests failed.`)) | |
| } else { | |
| resolve() | |
| } | |
| }) | |
| }) | |
| }) |
🧰 Tools
🪛 Biome (1.9.4)
[error] 15-31: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
🪛 GitHub Actions: CI Tests
[error] 15-15: Promise executor functions should not be async. (no-async-promise-executor)
| const getBranch = async () => { | ||
| if (type.perma) return repository.state.HEAD.commit | ||
| return type.default | ||
| ? await module.exports.getDefaultBranch(repository) | ||
| : repository.state.HEAD?.name || 'main' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify module exports reference and add commit validation.
The code can be improved in two ways:
- Using direct function reference instead of going through module.exports
- Adding commit hash validation for permalinks
Consider this implementation:
const getBranch = async () => {
- if (type.perma) return repository.state.HEAD.commit
+ if (type.perma) {
+ const commit = repository.state.HEAD.commit
+ if (!commit || !/^[0-9a-f]{40}$/i.test(commit)) {
+ throw new Error('Invalid commit hash for permalink')
+ }
+ return commit
+ }
return type.default
- ? await module.exports.getDefaultBranch(repository)
+ ? await getDefaultBranch(repository)
: repository.state.HEAD?.name || 'main'
}📝 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.
| const getBranch = async () => { | |
| if (type.perma) return repository.state.HEAD.commit | |
| return type.default | |
| ? await module.exports.getDefaultBranch(repository) | |
| : repository.state.HEAD?.name || 'main' | |
| const getBranch = async () => { | |
| if (type.perma) { | |
| const commit = repository.state.HEAD.commit | |
| if (!commit || !/^[0-9a-f]{40}$/i.test(commit)) { | |
| throw new Error('Invalid commit hash for permalink') | |
| } | |
| return commit | |
| } | |
| return type.default | |
| ? await getDefaultBranch(repository) | |
| : repository.state.HEAD?.name || 'main' | |
| } |
| repository = await new Promise((resolve, reject) => { | ||
| const timeout = setTimeout(() => { | ||
| disposable.dispose() | ||
| reject(new Error('Timeout waiting for Git repository')) | ||
| }, 5000) | ||
|
|
||
| const disposable = git.onDidOpenRepository(repo => { | ||
| if (activeDoc.uri.fsPath.toLowerCase().startsWith(repo.rootUri.fsPath.toLowerCase())) { | ||
| clearTimeout(timeout) | ||
| disposable.dispose() | ||
| resolve(repo) | ||
| } | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Fix potential memory leak in repository discovery.
The current implementation might leak memory if the timeout occurs before the repository is discovered.
Apply this fix:
if (!repository) {
+ let disposable
repository = await new Promise((resolve, reject) => {
const timeout = setTimeout(() => {
- disposable.dispose()
+ if (disposable) {
+ disposable.dispose()
+ }
reject(new Error('Timeout waiting for Git repository'))
}, 5000)
- const disposable = git.onDidOpenRepository(repo => {
+ disposable = git.onDidOpenRepository(repo => {
if (activeDoc.uri.fsPath.toLowerCase().startsWith(repo.rootUri.fsPath.toLowerCase())) {
clearTimeout(timeout)
disposable.dispose()
resolve(repo)
}
})
})
}📝 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.
| repository = await new Promise((resolve, reject) => { | |
| const timeout = setTimeout(() => { | |
| disposable.dispose() | |
| reject(new Error('Timeout waiting for Git repository')) | |
| }, 5000) | |
| const disposable = git.onDidOpenRepository(repo => { | |
| if (activeDoc.uri.fsPath.toLowerCase().startsWith(repo.rootUri.fsPath.toLowerCase())) { | |
| clearTimeout(timeout) | |
| disposable.dispose() | |
| resolve(repo) | |
| } | |
| }) | |
| }) | |
| } | |
| if (!repository) { | |
| let disposable | |
| repository = await new Promise((resolve, reject) => { | |
| const timeout = setTimeout(() => { | |
| if (disposable) { | |
| disposable.dispose() | |
| } | |
| reject(new Error('Timeout waiting for Git repository')) | |
| }, 5000) | |
| disposable = git.onDidOpenRepository(repo => { | |
| if (activeDoc.uri.fsPath.toLowerCase().startsWith(repo.rootUri.fsPath.toLowerCase())) { | |
| clearTimeout(timeout) | |
| disposable.dispose() | |
| resolve(repo) | |
| } | |
| }) | |
| }) | |
| } |
src/main.js
Outdated
| const gitConfig = await fs.readFile(configPath, 'utf8') | ||
| if (gitConfig.toLowerCase().includes('[branch "main"]'.toLowerCase())) return 'main' | ||
| if (gitConfig.toLowerCase().includes('[branch "master"]'.toLowerCase())) return 'master' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve branch detection in git config.
The current implementation has two issues:
- Hardcoded branch names could miss other valid default branches
- Case-insensitive string search could lead to false positives
Consider using regex for more precise matching:
- if (gitConfig.toLowerCase().includes('[branch "main"]'.toLowerCase())) return 'main'
- if (gitConfig.toLowerCase().includes('[branch "master"]'.toLowerCase())) return 'master'
+ const branchMatch = gitConfig.match(/^\s*\[branch "([^"]+)"\]\s*$/m)
+ if (branchMatch) {
+ const branchName = branchMatch[1]
+ if (['main', 'master'].includes(branchName)) {
+ return branchName
+ }
+ }📝 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.
| const gitConfig = await fs.readFile(configPath, 'utf8') | |
| if (gitConfig.toLowerCase().includes('[branch "main"]'.toLowerCase())) return 'main' | |
| if (gitConfig.toLowerCase().includes('[branch "master"]'.toLowerCase())) return 'master' | |
| const gitConfig = await fs.readFile(configPath, 'utf8') | |
| const branchMatch = gitConfig.match(/^\s*\[branch "([^"]+)"\]\s*$/m) | |
| if (branchMatch) { | |
| const branchName = branchMatch[1] | |
| if (['main', 'master'].includes(branchName)) { | |
| return branchName | |
| } | |
| } |
| // TODO: Using a hard-coded 5-second timeout for repository discovery may cause unexpected failures in larger or more complex projects. | ||
| // TODO: Consider making this timeout configurable or dynamically adjusting based on repository size. | ||
| let attempts = 0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address TODO comments regarding timeout configuration.
The hardcoded timeout values could cause issues in larger repositories.
Consider implementing configurable timeouts:
+ const config = vscode.workspace.getConfiguration('copyGithubUrl')
+ const discoveryTimeout = config.get('repositoryDiscoveryTimeout') || 5000
+ const remotePopulationTimeout = config.get('remotePopulationTimeout') || 5000
+ const retryDelay = config.get('retryDelay') || 500Don't forget to add these settings to your package.json configuration schema.
Committable suggestion skipped: line range outside the PR's diff.
src/main.js
Outdated
| } catch (e) { | ||
| // Try next remote if this one fails | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in remote URL processing.
The empty catch block could hide important errors that might help diagnose issues with remote URL processing.
Add error logging:
- } catch (e) {
+ } catch (error) {
+ if (!isTestEnvironment) {
+ console.warn(`Failed to process remote ${remote.name}: ${error.message}`)
+ }
// Try next remote if this one fails
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/main.js (2)
258-278:⚠️ Potential issueFix potential memory leak in repository discovery.
The current implementation might leak memory if the timeout occurs before the repository is discovered.
if (!repository) { + let disposable repository = await new Promise((resolve, reject) => { const timeout = setTimeout(() => { - disposable.dispose() + if (disposable) { + disposable.dispose() + } reject(new Error('Timeout waiting for Git repository')) }, 5000) - const disposable = git.onDidOpenRepository(repo => { + disposable = git.onDidOpenRepository(repo => { if (activeDoc.uri.fsPath.toLowerCase().startsWith(repo.rootUri.fsPath.toLowerCase())) { clearTimeout(timeout) disposable.dispose() resolve(repo) } }) }) }
281-283: 🛠️ Refactor suggestionAddress TODO comments regarding timeout configuration.
The hardcoded timeout values could cause issues in larger repositories.
Consider implementing configurable timeouts through VSCode settings.
🧹 Nitpick comments (4)
src/main.js (1)
88-90: Extract retry configuration to constants.The magic numbers for retry configuration should be extracted to named constants at the module level for better maintainability.
+'use strict' + +const BRANCH_DISCOVERY_MAX_RETRIES = 3 +const BRANCH_DISCOVERY_RETRY_DELAY = 500 + const githubUrlFromGit = require('github-url-from-git') - const MAX_RETRIES = 3 - const RETRY_DELAY = 500 + const MAX_RETRIES = BRANCH_DISCOVERY_MAX_RETRIES + const RETRY_DELAY = BRANCH_DISCOVERY_RETRY_DELAYtest/helpers/mockFactory.js (3)
3-19: Fix JSDoc formatting and enhance parameter descriptions.The documentation is comprehensive but has some minor issues:
- The
@paramforaccessTokenhas incorrect spacing- Some parameters could benefit from more detailed descriptions
-*@param {String} [options.accessToken] Mock authentication token +* @param {String} [options.accessToken] Mock authentication token for GitHub API
21-21: Enhance error message for required parameter.The error message could be more descriptive to help developers understand the expected format of
projectDirectory.- if (!options.projectDirectory) throw new Error('projectDirectory is required for getVsCodeMock.') + if (!options.projectDirectory) throw new Error('projectDirectory is required for getVsCodeMock. Please provide an absolute path to the project directory.')
23-25: Consider making the default file path configurable.The default file path is hardcoded with specific subdirectories. Consider making this configurable or moving it to a constants file for better maintainability.
+const DEFAULT_TEST_FILE_PATH = ['subdir1', 'subdir2', 'myFileName.txt'] + const fullPath = options.filePath ? [projectRoot, options.filePath].join(options.sep || '/') - : [projectRoot, 'subdir1', 'subdir2', 'myFileName.txt'].join(options.sep || '/') + : [projectRoot, ...DEFAULT_TEST_FILE_PATH].join(options.sep || '/')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.vscode/launch.json(2 hunks)package.json(3 hunks)src/main.js(1 hunks)test/helpers/mockFactory.js(1 hunks)test/unit/github.test.js(1 hunks)webpack.config.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unit/github.test.js
- package.json
🔇 Additional comments (15)
src/main.js (6)
5-12: LGTM! Clean imports and test environment setup.The imports are appropriate for the functionality, and the test environment setup is clean and simple.
14-61: LGTM! Well-structured main function with comprehensive error handling.The function effectively orchestrates the GitHub URL generation with proper error handling and logging.
43-54: Simplify module exports reference and add commit validation.The code can be improved in two ways:
- Using direct function reference instead of going through module.exports
- Adding commit hash validation for permalinks
const getBranch = async () => { - if (type.perma) return repository.state.HEAD.commit + if (type.perma) { + const commit = repository.state.HEAD.commit + if (!commit || !/^[0-9a-f]{40}$/i.test(commit)) { + throw new Error('Invalid commit hash for permalink') + } + return commit + } return type.default - ? await module.exports.getDefaultBranch(repository) + ? await getDefaultBranch(repository) : repository.state.HEAD?.name || 'main' }
153-204: LGTM! Well-implemented remote URL discovery.The function implements a robust strategy for finding the GitHub URL with proper prioritization and error handling.
206-219: LGTM! Correct implementation of path normalization.The function properly implements RFC 3986 for percent-encoding and handles path separators correctly.
295-302: LGTM! Clean and well-organized exports.All necessary functions are exported, and the inclusion of the path module for testing purposes is appropriate.
test/helpers/mockFactory.js (2)
1-1: LGTM! Import statement is well-structured.The local import of
createGitApialigns with the PR's objective of reducing external dependencies.
60-60: LGTM! Clean module export.The export follows modern JavaScript practices and makes the module's API clear.
.vscode/launch.json (3)
3-3: LGTM: Version bump is appropriate.The version increment to 0.2.0 aligns with the significant changes being made to migrate to VS Code Git API.
24-27: LGTM: Build configuration is well structured.The addition of webpack pre-launch task and development environment configuration aligns well with the modernized build process. The NODE_ENV setting ensures proper development-time behavior.
22-22: Verify test discovery with new path.The test path has been changed from
test/indextotest. Please ensure that test discovery still works correctly with this new path configuration.✅ Verification successful
Let me verify the test configuration further by checking the content of the test index file and webpack configuration.
Let me check the webpack configuration to complete the verification.
Let me correct the test file search command.
Test discovery works correctly with the new path configuration
The verification shows that:
test/index.jsexists and contains the correct test runner configuration- The test runner uses glob pattern
**/**.test.jsto discover tests- Test files are properly organized in
test/integrationandtest/unitdirectories- Webpack configuration correctly references
test/index.jsfor test buildsThe path change from
test/indextotestin launch.json is valid as it points to the directory containing the test runner, which can discover all test files in the subdirectories.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if test/index.js exists and verify the new test structure fd -t f "index.js" test/Length of output: 38
Script:
#!/bin/bash # Check the content of test/index.js and webpack config cat test/index.js # Look for webpack test configuration fd -t f "webpack.config.js"Length of output: 806
Script:
#!/bin/bash # Check webpack configuration cat webpack.config.js # Also check for test files to verify the test discovery pattern fd -t f "*.test.js" test/Length of output: 1223
Script:
#!/bin/bash # Search for test files using the correct fd syntax fd --glob "*.test.js" test/Length of output: 110
webpack.config.js (4)
1-1: LGTM: Well-structured module configuration.Good practices observed:
- TypeScript configuration type for better IDE support
- Clean separation of test and production entry points
- Modern ES module setup
Also applies to: 3-13
16-22: LGTM: Output and externals properly configured.The configuration correctly:
- Uses dynamic filenames to support multiple entry points
- Declares both VSCode and Mocha as external dependencies
33-37: LGTM: Excellent build optimization setup.The configuration properly handles different environments:
- Development mode with source maps for testing
- Production mode with minification for deployment
- Clear optimization rules based on environment
Line range hint
27-32: Module rules configuration appears incomplete.The module rules section only specifies an exclude pattern without defining any loaders or processing rules. Consider adding appropriate loader configurations if needed for your JavaScript files.
test/helpers/mockFactory.js
Outdated
| getExtension: () => ({ | ||
| exports: { | ||
| getAPI: () => createGitApi(options) | ||
| } | ||
| }) | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for getExtension.
The getExtension mock should handle the case where the Git extension is not available, similar to how it would behave in VSCode.
extensions: {
- getExtension: () => ({
+ getExtension: (extensionId) => {
+ if (extensionId !== 'vscode.git') {
+ return undefined
+ }
+ return {
exports: {
getAPI: () => createGitApi(options)
}
- })
+ }
+ }
},📝 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.
| getExtension: () => ({ | |
| exports: { | |
| getAPI: () => createGitApi(options) | |
| } | |
| }) | |
| }, | |
| getExtension: (extensionId) => { | |
| if (extensionId !== 'vscode.git') { | |
| return undefined | |
| } | |
| return { | |
| exports: { | |
| getAPI: () => createGitApi(options) | |
| } | |
| } | |
| } | |
| }, |
| authentication: { | ||
| getSession: async () => ({ accessToken: 'fake-token' }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make authentication token configurable.
The authentication mock uses a hardcoded 'fake-token'. This should use the accessToken from options to allow for different test scenarios.
authentication: {
- getSession: async () => ({ accessToken: 'fake-token' })
+ getSession: async () => ({ accessToken: options.accessToken || 'fake-token' })
}📝 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.
| authentication: { | |
| getSession: async () => ({ accessToken: 'fake-token' }) | |
| } | |
| authentication: { | |
| getSession: async () => ({ accessToken: options.accessToken || 'fake-token' }) | |
| } |
| const { createGitApi } = require('./gitApiFactory') | ||
|
|
||
| /** | ||
| * A helper function to return a vscode object imitation. | ||
| * | ||
| * @param {Object} [options] | ||
| *@param {String} [options.accessToken] Mock authentication token | ||
| * @param {String} [options.branch] Branch name | ||
| * @param {String} [options.commit] Commit hash | ||
| * @param {Number} [options.endLine] Line number where the current selection ends | ||
| * @param {String} [options.filePath] File path **relative to** `projectDirectory` | ||
| * @param {String} [options.gitRoot] Root URI for the Git repository | ||
| * @param {String} [options.projectDirectory] Absolute path to the project directory | ||
| * @param {String} [options.repoUrl] Repository URL | ||
| * @param {String} [options.sep] Separator to use for the path | ||
| * @param {Number} [options.startLine] Current, focused line number | ||
| * @param {Array} [options.workspaceFolders] Array of workspace folders | ||
| * @returns {Object} An `vscode` alike object | ||
| */ | ||
| function getVsCodeMock (options) { | ||
| if (!options.projectDirectory) throw new Error('projectDirectory is required for getVsCodeMock.') | ||
| const projectRoot = options.projectDirectory | ||
| const fullPath = options.filePath | ||
| ? [projectRoot, options.filePath].join(options.sep || '/') | ||
| : [projectRoot, 'subdir1', 'subdir2', 'myFileName.txt'].join(options.sep || '/') | ||
|
|
||
| const startLine = options.startLine !== undefined ? options.startLine : 1 | ||
| const endLine = options.endLine !== undefined ? options.endLine : startLine | ||
|
|
||
| const editorMock = { | ||
| selection: { | ||
| active: { line: startLine }, | ||
| start: { line: startLine }, | ||
| end: { line: endLine }, | ||
| isSingleLine: startLine === endLine | ||
| }, | ||
| document: { | ||
| uri: { fsPath: fullPath } | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| workspace: { | ||
| workspaceFolders: options.workspaceFolders || [{ uri: { fsPath: projectRoot } }] | ||
| }, | ||
| window: { activeTextEditor: editorMock }, | ||
| extensions: { | ||
| getExtension: () => ({ | ||
| exports: { | ||
| getAPI: () => createGitApi(options) | ||
| } | ||
| }) | ||
| }, | ||
| authentication: { | ||
| getSession: async () => ({ accessToken: 'fake-token' }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| module.exports = { getVsCodeMock } |
There was a problem hiding this comment.
💡 Codebase verification
The mock factory needs dedicated unit tests for error handling and edge cases
Based on the analysis of the codebase:
- The mock factory is widely used across multiple test files (
url.test.js,github.test.js, andcommands.test.js) - While there's good coverage of happy paths with various combinations of options, there are no tests specifically verifying:
- The error case when
projectDirectoryis missing - Edge cases like empty or invalid options
- The default behavior when optional parameters are not provided
- The error case when
🔗 Analysis chain
Consider adding unit tests for the mock factory.
While this is a test helper, it's complex enough to warrant its own unit tests to ensure it correctly handles edge cases and invalid inputs.
Let's check if there are any existing tests for this mock factory:
Would you like me to help create unit tests for this mock factory?
Let me check the content of these files to see if they contain tests for the mock factory.
Let me check how the mock factory is actually used in these tests to see if they cover its functionality.
Let me check if there are any tests that verify the error case when projectDirectory is missing, as that's the only validation in the mock factory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for test files that might be testing the mock factory
rg -l "mockFactory" "test/" --type js
Length of output: 116
Script:
#!/bin/bash
# Search for actual usage or testing of mockFactory in the found files
rg "mockFactory" "test/unit/url.test.js" "test/unit/github.test.js" "test/integration/commands.test.js" -B 2 -A 2
Length of output: 1133
Script:
#!/bin/bash
# Look for actual test cases using getVsCodeMock
rg "getVsCodeMock\(" -A 5 "test/unit/url.test.js" "test/unit/github.test.js" "test/integration/commands.test.js"
# Also check for error cases testing
rg "throw.*Error" "test/unit/url.test.js" "test/unit/github.test.js" "test/integration/commands.test.js"
Length of output: 8020
Script:
#!/bin/bash
# Look for tests that might be checking the error case of missing projectDirectory
rg -B 5 -A 5 "projectDirectory.*required" "test/"
Length of output: 914
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/main.js (3)
47-53: 🛠️ Refactor suggestionImprove commit hash validation for permalinks.
The current implementation could allow invalid commit hashes.
Apply this diff to add proper validation:
if (type.perma) { const commit = repository.state.HEAD?.commit if (!commit) { throw new Error('No commit hash found. Repository may be empty or still loading.') } + if (!/^[0-9a-f]{40}$/i.test(commit)) { + throw new Error('Invalid commit hash for permalink') + } return commit }
284-286: 🛠️ Refactor suggestionAddress TODO comments regarding timeout configuration.
The hardcoded timeout values could cause issues in larger repositories.
Add these settings to your package.json configuration schema and update the code to use them.
262-281:⚠️ Potential issueFix potential memory leak in repository discovery.
The current implementation might leak memory if the timeout occurs before the repository is discovered.
Apply the suggested fix from the previous review.
🧹 Nitpick comments (9)
src/main.js (4)
3-4: Consider making retry constants configurable.These hardcoded values might not be suitable for all repository sizes and network conditions.
Consider moving these to the extension's configuration:
-const BRANCH_DISCOVERY_MAX_RETRIES = 3 -const BRANCH_DISCOVERY_RETRY_DELAY = 500 +const config = vscode.workspace.getConfiguration('copyGithubUrl') +const BRANCH_DISCOVERY_MAX_RETRIES = config.get('branchDiscoveryMaxRetries') || 3 +const BRANCH_DISCOVERY_RETRY_DELAY = config.get('branchDiscoveryRetryDelay') || 500
79-85: Improve branch detection regex pattern.The current regex pattern might match branch names in comments or other contexts.
Consider using a more precise pattern:
- const branchRegex = /^\[branch "(.*?)"\]\s*$/mg + const branchRegex = /^\s*\[branch "([^"]+)"\]\s*$/mg
95-139: Simplify retry mechanism using async/await.The current implementation is overly complex with nested try-catch blocks.
Consider this simpler implementation:
- while (attempts < MAX_RETRIES) { - try { - const defaultBranch = await new Promise((resolve, reject) => { + const getBranchName = async () => { + return new Promise((resolve, reject) => { cp.exec('git branch -r', { cwd: repository.rootUri.fsPath }, (err, stdout) => { if (err) { reject(new Error(`Failed to execute git branch -r: ${err.message}`)) return } // ... rest of the branch detection logic ... }) - }) - if (defaultBranch) return defaultBranch - } catch (error) { - if (attempts === 2) throw error - if (attempts === MAX_RETRIES - 1) { - throw new Error(`Failed to get default branch after ${MAX_RETRIES} attempts: ${error.message}`) - } - } finally { + }) + } + + while (attempts < MAX_RETRIES) { + try { + const defaultBranch = await getBranchName() + if (defaultBranch) return defaultBranch + } catch (error) { + if (attempts === MAX_RETRIES - 1) { + throw new Error(`Failed to get default branch after ${MAX_RETRIES} attempts: ${error.message}`) + } + } await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)) attempts++ - } }
176-176: Improve domain extraction regex pattern.The current regex pattern might not handle all valid Git URL formats.
Consider using a more comprehensive pattern:
- const domain = remote.fetchUrl.match(/(?:https?:\/\/|git@)([^:/]+)/)?.[1] + const domain = remote.fetchUrl.match(/(?:https?:\/\/|git@|ssh:\/\/(?:[^@]+@)?)([^:/]+)/)?.[1]Also applies to: 195-195
test/helpers/mockFactory.js (5)
3-19: Enhance JSDoc documentation for better clarity.Consider improving the documentation:
- Add more details about the structure of the returned mock object.
- Clarify the expected shape of
options.workspaceFolders./** * A helper function to return a vscode object imitation. * * @param {Object} [options] * @param {String} [options.accessToken] Mock authentication token * @param {String} [options.branch] Branch name * @param {String} [options.commit] Commit hash * @param {Number} [options.endLine] Line number where the current selection ends * @param {String} [options.filePath] File path **relative to** `projectDirectory` * @param {String} [options.gitRoot] Root URI for the Git repository * @param {String} [options.projectDirectory] Absolute path to the project directory * @param {String} [options.repoUrl] Repository URL * @param {String} [options.sep] Separator to use for the path * @param {Number} [options.startLine] Current, focused line number - * @param {Array} [options.workspaceFolders] Array of workspace folders - * @returns {Object} An `vscode` alike object + * @param {Array<{uri: {fsPath: string}}>} [options.workspaceFolders] Array of workspace folders with URI paths + * @returns {Object} A mock VSCode API object containing workspace, window, extensions, and authentication properties */
20-21: Enhance input validation for required Git API options.While
projectDirectoryvalidation is good, consider validating other options that might be required for Git API functionality.function getVsCodeMock (options) { - if (!options.projectDirectory) throw new Error('projectDirectory is required for getVsCodeMock. Please provide an absolute path to the project directory.') + if (!options.projectDirectory) { + throw new Error('projectDirectory is required for getVsCodeMock. Please provide an absolute path to the project directory (e.g., "/path/to/project").') + } + if (options.gitRoot && !options.repoUrl) { + throw new Error('repoUrl is required when gitRoot is provided for Git API functionality.') + }
22-26: Improve path construction robustness.The current path construction could be more robust:
- The default file path is hardcoded with specific subdirectories.
- No validation of the separator character.
- No path normalization.
+ const validSep = options.sep || '/' + if (!['/','\\'].includes(validSep)) { + throw new Error('Invalid path separator. Must be "/" or "\\".') + } const projectRoot = options.projectDirectory const fullPath = options.filePath - ? [projectRoot, options.filePath].join(options.sep || '/') - : [projectRoot, 'subdir1', 'subdir2', 'myFileName.txt'].join(options.sep || '/') + ? [projectRoot, options.filePath].join(validSep) + : [projectRoot, 'test-file.txt'].join(validSep)
27-29: Add validation for line numbers.Consider adding validation to ensure line numbers are valid:
- Positive integers only
- End line not less than start line
const startLine = options.startLine !== undefined ? options.startLine : 1 const endLine = options.endLine !== undefined ? options.endLine : startLine + if (!Number.isInteger(startLine) || startLine < 1) { + throw new Error('startLine must be a positive integer.') + } + if (!Number.isInteger(endLine) || endLine < startLine) { + throw new Error('endLine must be a positive integer greater than or equal to startLine.') + }
30-40: Enhance editor mock with additional properties.Consider adding commonly used editor properties to make the mock more complete:
const editorMock = { selection: { active: { line: startLine }, start: { line: startLine }, end: { line: endLine }, isSingleLine: startLine === endLine }, document: { - uri: { fsPath: fullPath } + uri: { + fsPath: fullPath, + scheme: 'file' + }, + fileName: fullPath, + isDirty: false, + isUntitled: false } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main.js(1 hunks)test/helpers/mockFactory.js(1 hunks)test/unit/github.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/github.test.js
🔇 Additional comments (5)
src/main.js (2)
217-222: LGTM! Well-implemented path normalization.The function correctly follows RFC 3986 for percent-encoding and properly handles path separators.
298-305: LGTM! Well-organized exports.All necessary functions are properly exported and organized clearly.
test/helpers/mockFactory.js (3)
48-61: Previous review concerns have been addressed.The implementation now correctly:
- Handles non-git extensions in
getExtension- Makes authentication token configurable through options
65-65: LGTM!Clean and modern export syntax.
1-65: Add dedicated tests for the mock factory.While this is a test helper, it's complex enough to warrant its own unit tests to ensure it correctly handles edge cases and invalid inputs. This is particularly important given its role in testing the migration to VS Code Git API.
Let's verify the current test coverage:
#!/bin/bash # Description: Check for existing tests of the mock factory # Look for test files that might be testing the mock factory rg -l "mockFactory" "test/" # Search for actual usage or testing of mockFactory rg "describe.*mockFactory|it.*mockFactory" "test/"
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main.js (2)
47-53: 🛠️ Refactor suggestionStrengthen commit hash validation.
The commit hash validation could be more robust to ensure it's a valid Git SHA-1.
if (type.perma) { const commit = repository.state.HEAD?.commit if (!commit) { throw new Error('No commit hash found. Repository may be empty or still loading.') } + if (!/^[0-9a-f]{40}$/i.test(commit)) { + throw new Error('Invalid commit hash format') + } return commit }
263-281:⚠️ Potential issueImprove repository discovery timeout handling.
The current implementation could leak memory if the timeout occurs before the repository is discovered.
if (!repository) { const MAX_TIMEOUT = 5000 + let disposable repository = await new Promise((resolve, reject) => { - let isResolved = false - const disposable = git.onDidOpenRepository(repo => { + disposable = git.onDidOpenRepository(repo => { if (activeDoc.uri.fsPath.toLowerCase().startsWith(repo.rootUri.fsPath.toLowerCase())) { - isResolved = true disposable.dispose() resolve(repo) } }) setTimeout(() => { - if (!isResolved) { + if (disposable) { disposable.dispose() - reject(new Error('Timeout waiting for Git repository')) + reject(new Error(`Timeout waiting for Git repository after ${MAX_TIMEOUT}ms`)) } }, MAX_TIMEOUT) }) }
🧹 Nitpick comments (3)
src/main.js (3)
3-4: Consider making retry constants configurable.These hardcoded values could be insufficient for larger repositories or slower networks. Consider moving them to the extension's configuration.
-const BRANCH_DISCOVERY_MAX_RETRIES = 3 -const BRANCH_DISCOVERY_RETRY_DELAY = 500 +const config = vscode.workspace.getConfiguration('copyGithubUrl') +const BRANCH_DISCOVERY_MAX_RETRIES = config.get('branchDiscoveryMaxRetries') || 3 +const BRANCH_DISCOVERY_RETRY_DELAY = config.get('branchDiscoveryRetryDelay') || 500
60-63: Enhance error handling with specific error types.Consider categorizing errors to help users better understand and resolve issues.
} catch (error) { - if (!isTestEnvironment) console.error('Failed to get GitHub URL:', error) + if (!isTestEnvironment) { + const errorType = error.message.includes('Git extension not found') ? 'Extension Error' : + error.message.includes('No commit hash found') ? 'Repository Error' : + 'URL Generation Error' + console.error(`${errorType}: Failed to get GitHub URL:`, error) + } throw error }
176-179: Improve remote URL parsing and validation.The current domain extraction regex could be more robust for handling various Git URL formats.
- const domain = remote.fetchUrl.match(/(?:https?:\/\/|git@|ssh:\/\/(?:[^@]+@)?)([^:/]+)/)?.[1] + const domain = remote.fetchUrl.match( + /^(?:https?:\/\/|git@|ssh:\/\/(?:[^@]+@)?)(([^:/]+?)(?::[0-9]+)??)(?::|\/)/ + )?.[1] return Promise.resolve(githubUrlFromGit(remote.fetchUrl, { - extraBaseUrls: [gitUrl, domain].filter(Boolean) + extraBaseUrls: [gitUrl, domain].filter(Boolean), + transformUrl: url => url.replace(/\.git$/, '') }))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main.js(1 hunks)
🔇 Additional comments (3)
src/main.js (3)
217-222: LGTM! Well-implemented path normalization.The function correctly implements RFC 3986 for percent-encoding and handles path separators appropriately.
284-286: Address TODO comments regarding timeout configuration.The hardcoded timeout values could cause issues in larger repositories.
#!/bin/bash # Check if there are any existing timeout configurations in the codebase rg -l "getConfiguration\('copyGithubUrl'\)" | xargs rg "timeout"
298-305: LGTM! Well-structured exports.All necessary functions are properly exported, and the organization is clean.
src/main.js
Outdated
| let attempts = 0 | ||
| while (attempts < MAX_RETRIES) { // Fewer retries since this is less common | ||
| try { | ||
| const defaultBranch = await new Promise((resolve, reject) => { | ||
| cp.exec('git branch -r', { cwd: repository.rootUri.fsPath }, (err, stdout) => { | ||
| if (err) { | ||
| reject(new Error(`Failed to execute git branch -r: ${err.message}`)) | ||
| return | ||
| } | ||
|
|
||
| if (!selection.isSingleLine) { | ||
| // Selection might be spanned across multiple lines. | ||
| lineQuery += ('-L' + (selection.end.line + 1)) | ||
| // Get list of branches, removing any whitespace/empty lines | ||
| const branches = stdout.split('\n').map(b => b.trim()).filter(Boolean) | ||
|
|
||
| // Look for HEAD pointer first as it's the most reliable indicator | ||
| const headPointer = branches.find(b => b.startsWith('origin/HEAD')) | ||
| if (headPointer) { | ||
| const match = headPointer.match(/origin\/HEAD -> origin\/(.+)/) | ||
| if (match) { | ||
| resolve(match[1]) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Fallback to other branch detection methods | ||
| if (branches.length === 1) { | ||
| resolve(branches[0].replace('origin/', '')) | ||
| } else if (branches.some(b => b.toLowerCase() === 'origin/main')) { | ||
| resolve('main') | ||
| } else if (branches.some(b => b.toLowerCase() === 'origin/master')) { | ||
| resolve('master') | ||
| } else { | ||
| resolve(undefined) | ||
| } | ||
| }) | ||
| }) | ||
| if (defaultBranch) return defaultBranch | ||
| } catch (error) { | ||
| if (attempts === 2) throw error // Re-throw on final attempt | ||
| if (attempts === MAX_RETRIES - 1) { | ||
| throw new Error(`Failed to get default branch after ${MAX_RETRIES} attempts: ${error.message}`) | ||
| } | ||
| } finally { | ||
| await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)) | ||
| attempts++ | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify retry logic and improve error handling.
The nested try-catch blocks and retry logic could be simplified for better maintainability.
- let attempts = 0
- while (attempts < MAX_RETRIES) {
- try {
- const defaultBranch = await new Promise((resolve, reject) => {
+ const executeGitCommand = async (attempt) => {
+ return new Promise((resolve, reject) => {
cp.exec('git branch -r', { cwd: repository.rootUri.fsPath }, (err, stdout) => {
if (err) {
reject(new Error(`Failed to execute git branch -r: ${err.message}`))
return
}
// ... rest of the command execution logic ...
})
- })
- if (defaultBranch) return defaultBranch
- } catch (error) {
- if (attempts === 2) throw error
- if (attempts === MAX_RETRIES - 1) {
- throw new Error(`Failed to get default branch after ${MAX_RETRIES} attempts: ${error.message}`)
- }
- } finally {
- await new Promise(resolve => setTimeout(resolve, RETRY_DELAY))
- attempts++
+ })
+ }
+ for (let attempt = 0; attempt < MAX_RETRIES; attempt++) {
+ try {
+ const defaultBranch = await executeGitCommand(attempt)
+ if (defaultBranch) return defaultBranch
+ } catch (error) {
+ if (attempt === MAX_RETRIES - 1) {
+ throw new Error(`Failed to get default branch after ${MAX_RETRIES} attempts: ${error.message}`)
+ }
+ await new Promise(resolve => setTimeout(resolve, RETRY_DELAY))
}
}📝 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.
| let attempts = 0 | |
| while (attempts < MAX_RETRIES) { // Fewer retries since this is less common | |
| try { | |
| const defaultBranch = await new Promise((resolve, reject) => { | |
| cp.exec('git branch -r', { cwd: repository.rootUri.fsPath }, (err, stdout) => { | |
| if (err) { | |
| reject(new Error(`Failed to execute git branch -r: ${err.message}`)) | |
| return | |
| } | |
| if (!selection.isSingleLine) { | |
| // Selection might be spanned across multiple lines. | |
| lineQuery += ('-L' + (selection.end.line + 1)) | |
| // Get list of branches, removing any whitespace/empty lines | |
| const branches = stdout.split('\n').map(b => b.trim()).filter(Boolean) | |
| // Look for HEAD pointer first as it's the most reliable indicator | |
| const headPointer = branches.find(b => b.startsWith('origin/HEAD')) | |
| if (headPointer) { | |
| const match = headPointer.match(/origin\/HEAD -> origin\/(.+)/) | |
| if (match) { | |
| resolve(match[1]) | |
| return | |
| } | |
| } | |
| // Fallback to other branch detection methods | |
| if (branches.length === 1) { | |
| resolve(branches[0].replace('origin/', '')) | |
| } else if (branches.some(b => b.toLowerCase() === 'origin/main')) { | |
| resolve('main') | |
| } else if (branches.some(b => b.toLowerCase() === 'origin/master')) { | |
| resolve('master') | |
| } else { | |
| resolve(undefined) | |
| } | |
| }) | |
| }) | |
| if (defaultBranch) return defaultBranch | |
| } catch (error) { | |
| if (attempts === 2) throw error // Re-throw on final attempt | |
| if (attempts === MAX_RETRIES - 1) { | |
| throw new Error(`Failed to get default branch after ${MAX_RETRIES} attempts: ${error.message}`) | |
| } | |
| } finally { | |
| await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)) | |
| attempts++ | |
| } | |
| } | |
| const executeGitCommand = async (attempt) => { | |
| return new Promise((resolve, reject) => { | |
| cp.exec('git branch -r', { cwd: repository.rootUri.fsPath }, (err, stdout) => { | |
| if (err) { | |
| reject(new Error(`Failed to execute git branch -r: ${err.message}`)) | |
| return | |
| } | |
| // Get list of branches, removing any whitespace/empty lines | |
| const branches = stdout.split('\n').map(b => b.trim()).filter(Boolean) | |
| // Look for HEAD pointer first as it's the most reliable indicator | |
| const headPointer = branches.find(b => b.startsWith('origin/HEAD')) | |
| if (headPointer) { | |
| const match = headPointer.match(/origin\/HEAD -> origin\/(.+)/) | |
| if (match) { | |
| resolve(match[1]) | |
| return | |
| } | |
| } | |
| // Fallback to other branch detection methods | |
| if (branches.length === 1) { | |
| resolve(branches[0].replace('origin/', '')) | |
| } else if (branches.some(b => b.toLowerCase() === 'origin/main')) { | |
| resolve('main') | |
| } else if (branches.some(b => b.toLowerCase() === 'origin/master')) { | |
| resolve('master') | |
| } else { | |
| resolve(undefined) | |
| } | |
| }) | |
| }) | |
| } | |
| for (let attempt = 0; attempt < MAX_RETRIES; attempt++) { | |
| try { | |
| const defaultBranch = await executeGitCommand(attempt) | |
| if (defaultBranch) return defaultBranch | |
| } catch (error) { | |
| if (attempt === MAX_RETRIES - 1) { | |
| throw new Error(`Failed to get default branch after ${MAX_RETRIES} attempts: ${error.message}`) | |
| } | |
| await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)) | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main.js (2)
265-281:⚠️ Potential issueFix potential memory leak in repository discovery.
The current implementation might leak memory if the timeout occurs before the repository is discovered.
284-286: 🛠️ Refactor suggestionAddress TODO comments regarding timeout configuration.
The hardcoded timeout values could cause issues in larger repositories.
🧹 Nitpick comments (3)
src/main.js (3)
3-4: Consider making retry configuration values configurable via VSCode settings.These hardcoded values could be insufficient for larger repositories or slower networks.
Add these settings to your VSCode configuration:
-const BRANCH_DISCOVERY_MAX_RETRIES = 3 -const BRANCH_DISCOVERY_RETRY_DELAY = 500 +const config = vscode.workspace.getConfiguration('copyGithubUrl') +const BRANCH_DISCOVERY_MAX_RETRIES = config.get('branchDiscoveryMaxRetries') || 3 +const BRANCH_DISCOVERY_RETRY_DELAY = config.get('branchDiscoveryRetryDelay') || 500
73-157: Enhance error messages with more specific information.The error messages could provide more context about which strategy failed.
Consider enhancing the error handling:
} catch (error) { - if (!isTestEnvironment) console.error('Failed to read git config:', error) + if (!isTestEnvironment) { + console.error('Strategy 1 - Git config read failed:', error.message) + console.error('Falling back to git branch command...') + } } // ... similar changes for other catch blocks
164-210: Consider caching GitHub URLs for performance.Repeatedly processing remote URLs could be inefficient for large repositories with many remotes.
Consider implementing a simple cache:
+const urlCache = new Map() async function getGithubUrlFromRemotes (repository) { + const cacheKey = repository.rootUri.fsPath + if (urlCache.has(cacheKey)) { + return urlCache.get(cacheKey) + } // ... existing code ... + const url = githubUrlFromGit(remote.fetchUrl, { extraBaseUrls: [gitUrl, domain].filter(Boolean) }) + if (url) { + urlCache.set(cacheKey, url) + return Promise.resolve(url) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main.js(1 hunks)
🔇 Additional comments (3)
src/main.js (3)
220-225: LGTM! Well-implemented path normalization.The implementation correctly follows RFC 3986 for percent-encoding and handles path separators appropriately.
298-305: LGTM! Well-organized exports.All necessary functions are properly exported, and the organization is clean.
47-53: 🛠️ Refactor suggestionAdd commit hash validation for permalinks.
The commit hash should be validated to ensure it's a valid SHA-1 hash.
Apply this validation:
if (type.perma) { const commit = repository.state.HEAD?.commit if (!commit) { throw new Error('No commit hash found. Repository may be empty or still loading.') } + if (!/^[0-9a-f]{40}$/i.test(commit)) { + throw new Error('Invalid commit hash format for permalink') + } return commit }Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main.js (2)
47-53: 🛠️ Refactor suggestionAdd commit hash validation for permalinks.
The commit hash should be validated to ensure it's a valid SHA-1 hash.
Apply this diff to add validation:
if (type.perma) { const commit = repository.state.HEAD?.commit if (!commit) { throw new Error('No commit hash found. Repository may be empty or still loading.') } + if (!/^[0-9a-f]{40}$/i.test(commit)) { + throw new Error('Invalid commit hash format for permalink') + } return commit }
286-288: 🛠️ Refactor suggestionAddress TODO comments regarding timeout configuration.
The hardcoded timeout values could cause issues in larger repositories.
Consider implementing configurable timeouts through the extension's settings.
🧹 Nitpick comments (1)
src/main.js (1)
3-4: Consider making retry configuration values configurable.These hardcoded values could be insufficient for larger repositories or slower networks.
Consider moving these to the extension's configuration:
-const BRANCH_DISCOVERY_MAX_RETRIES = 3 -const BRANCH_DISCOVERY_RETRY_DELAY = 500 +const config = vscode.workspace.getConfiguration('copyGithubUrl') +const BRANCH_DISCOVERY_MAX_RETRIES = config.get('branchDiscoveryMaxRetries') || 3 +const BRANCH_DISCOVERY_RETRY_DELAY = config.get('branchDiscoveryRetryDelay') || 500
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main.js(1 hunks)
🔇 Additional comments (4)
src/main.js (4)
129-142: Simplify retry logic.The retry logic could be simplified by removing the unnecessary
finallyblock.Apply this diff to simplify:
for (let attempt = 0; attempt < MAX_RETRIES; attempt++) { try { const defaultBranch = await executeGitBranch() if (defaultBranch) return defaultBranch } catch (error) { if (attempt === MAX_RETRIES - 1) { throw new Error(`Failed to get default branch after ${MAX_RETRIES} attempts: ${error.message}`) } - } finally { - if (attempt < MAX_RETRIES - 1) { // Don't delay after last attempt - await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)) - } + await new Promise(resolve => setTimeout(resolve, RETRY_DELAY)) } }
164-210: Well-structured remote URL detection logic!The implementation properly handles:
- Current branch's remote first
- Enterprise GitHub URLs
- Multiple remote fallbacks
- Domain extraction and validation
220-225: Excellent path normalization implementation!The function correctly implements RFC 3986 for percent-encoding and handles path separators appropriately.
300-307: Clean and well-organized exports!All necessary functions are properly exported, and the organization is clear.
| let attempts = 0 | ||
| while (attempts < 10) { | ||
| if (repository.state.remotes && repository.state.remotes.length > 0) { | ||
| return repository | ||
| } | ||
| await new Promise(resolve => setTimeout(resolve, 500)) | ||
| attempts++ | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve remote population retry logic.
The current implementation uses magic numbers for attempts and delay.
Apply this diff to improve the implementation:
-let attempts = 0
-while (attempts < 10) {
+const config = vscode.workspace.getConfiguration('copyGithubUrl')
+const maxAttempts = config.get('remotePopulationMaxAttempts') || 10
+const retryDelay = config.get('remotePopulationRetryDelay') || 500
+let attempts = 0
+while (attempts < maxAttempts) {
if (repository.state.remotes && repository.state.remotes.length > 0) {
return repository
}
- await new Promise(resolve => setTimeout(resolve, 500))
+ await new Promise(resolve => setTimeout(resolve, retryDelay))
attempts++
}📝 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.
| let attempts = 0 | |
| while (attempts < 10) { | |
| if (repository.state.remotes && repository.state.remotes.length > 0) { | |
| return repository | |
| } | |
| await new Promise(resolve => setTimeout(resolve, 500)) | |
| attempts++ | |
| } | |
| const config = vscode.workspace.getConfiguration('copyGithubUrl') | |
| const maxAttempts = config.get('remotePopulationMaxAttempts') || 10 | |
| const retryDelay = config.get('remotePopulationRetryDelay') || 500 | |
| let attempts = 0 | |
| while (attempts < maxAttempts) { | |
| if (repository.state.remotes && repository.state.remotes.length > 0) { | |
| return repository | |
| } | |
| await new Promise(resolve => setTimeout(resolve, retryDelay)) | |
| attempts++ | |
| } |
Use vscode.env.clipboard instead of external dependency clipboardy
Use vscode.git extension API more instead of .git/config directly (.git/config still used for default branch lookup)
Stop using npm default-branch because it does not handle private repos
Stop using npm git-branch because available via vscode.git extension
Stop using git-rev-sync because available via vscode.git extension
Stop using parse-git-config because overkill for simple main/master default branch name lookups
Updated all dependency versions to latest
Updated vscode engine to 1.74.0
Updated launch.json version to 0.2.0
Add support for opening parent workspace directories with multiple repo subdirectories
Refactored and expanded test coverage
Fixes #22 - detect private repo default branch
Fixes #40 - update command title, master => main
Summary by CodeRabbit
Release Notes
New Features
Improvements
Dependency Updates
Bug Fixes