Conversation
- Add Tab Groups API support for more reliable non-text file detection - Enhance error handling for Git API interactions with better type-checking - Fix handling of repository discovery edge cases (empty/null repos, non-array refs) - Add context menu options in Explorer for easier non-text file URL generation - Update documentation with troubleshooting section for non-text files - Improve test coverage for non-text file handling and edge cases Fixes #61
WalkthroughThis pull request updates documentation, configuration, source code, and tests for the VS Code extension that copies GitHub URLs. The documentation now clearly explains the behavior for both text and non-text files, including troubleshooting steps. In the configuration files, a new debug command is registered, and keybinding conditions are enhanced to handle non-text files. The source code has been refactored to improve telemetry data collection, error handling, and repository retrieval logic. The tests have been updated and expanded to simulate non-text file scenarios and simplify URL generation verification. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VSCode
participant ExtensionEngine
participant URLGenerator
participant RepositoryFinder
User ->> VSCode: Trigger "Copy GitHub URL" command
VSCode ->> ExtensionEngine: Route command execution
ExtensionEngine ->> URLGenerator: Call getGithubUrl (pass active editor/fileUri)
URLGenerator ->> RepositoryFinder: Request repository info (with fileUri if needed)
RepositoryFinder -->> URLGenerator: Return repository details
URLGenerator -->> ExtensionEngine: Generate and return GitHub URL
ExtensionEngine ->> VSCode: Display/copy the GitHub URL to clipboard
sequenceDiagram
participant User
participant VSCode
participant DebugCommand
User ->> VSCode: Trigger "Debug Copy GitHub URL (Non-text files)" command
VSCode ->> DebugCommand: Execute debug command
DebugCommand ->> DebugCommand: Gather telemetry and active file data
DebugCommand -->> VSCode: Log details and show notification
VSCode ->> User: Display diagnostic debug information
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
PR Overview
This PR adds support for non-text files by integrating Tab Groups API, improving error handling and type‐checking in Git API interactions, and enhancing test coverage and documentation for non-text file scenarios.
- Added tests for non-text file handling scenarios
- Refactored URL generation to conditionally omit line references for non-text files
- Updated documentation and telemetry data gathering to support both text and non-text files
Reviewed Changes
| File | Description |
|---|---|
| test/unit/non-text-files.test.js | New tests added for various non-text file types and edge cases. |
| src/main.js | Modified URL generation and repository discovery logic to support non-text files. |
| src/extension.js | Updated activation and telemetry data gathering to better handle non-text files. |
| test/integration/commands.test.js | Adjusted command tests to stub getGithubUrl and bypass clipboard dependency for testing. |
| README.md | Improved usage descriptions and added troubleshooting guidance for non-text files. |
| test/helpers/mockFactory.js | Updated mock setup to simulate non-text file scenarios with activeEditorPane and Tab Groups. |
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
test/helpers/mockFactory.js:61
- Using options.filePath.split('/').pop() assumes that options.filePath is always defined. Consider using path.basename(fullPath) or ensuring that options.filePath is provided to avoid potential runtime errors.
label: options.filePath.split('/').pop(),
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
src/extension.js (4)
11-23: Ensure compatibility checks or fallback for older VS Code versions.
This block attempts to derive a file path from newer VS Code APIs (activeEditorPane,tabGroups) as a fallback for non-text files. Since theengines.vscodeinpackage.jsonis pinned to^1.74.0, this is likely fine. However, if older versions are ever supported or tested, you may want to conditionally check for API existence before usage to prevent runtime errors.
38-38: Telemetry data consistency.
IncludinghasCustomDomainOverride,fileExtension, andisTextFilein telemetry is helpful. Be mindful thatisTextFileonly reflects the current presence of an active text editor. Non-text editors reportingfalseis correct, but consider labeling it more explicitly (e.g.,fromTextEditor) to avoid confusion.Also applies to: 42-43
94-136: Consolidate multi-path retrieval logic for clarity.
This robust logic handles text vs. non-text scenarios by checking multiple editor APIs and, if needed, tab groups. Consider extracting these checks into a helper function, such asgetActiveFileUri(), to reduce nesting ingenerateCommandBody. This would improve readability and maintainability.
164-197: Consider reducing sensitive path data in debug logs.
Logging file paths and URIs is great for troubleshooting, but can inadvertently expose sensitive information if a user shares debug logs. Consider masking or sanitizing certain path segments if secrecy is required.Also applies to: 203-203
package.json (1)
51-52: Enhanced keybinding conditions.
Allowing the command to run under “editorFocus || activeEditorIsNotText || resourceScheme == 'file' || activeViewlet == 'workbench.view.explorer'” broadens usability for non-text files. Make sure to revisit any old documentation or references that mentioneditorTextFocusonly.Also applies to: 56-57, 61-62
src/main.js (5)
28-39: Robust Git extension check and activation.
Activating the Git extension at runtime is a good fallback, but consider surfacing an informative error if activation fails repeatedly. The existing error messaging is helpful.
67-67: Branch fallback approach.
Using commits for permalinks and deferring togetDefaultBranch()or the HEAD name covers typical usage. Keep an eye on performance in large repos if multiple calls are made in quick succession.Also applies to: 79-79
189-193: Warn on empty remote arrays.
Providing a console warning helps debug missing remotes. Consider also surfacing this in the UI for user clarity.
324-324: Catching errors from rootGitFolder usage.
Continue to log partial errors if needed. This is fine as is, but check if you want to surface them as user-facing messages for clarity.
366-387: Retry logic for Git remotes.
Waiting up to 10 attempts, each 500 ms, is a decent trade-off between user responsiveness and final readiness. Consider a user notification after a few attempts if the process is going to take seconds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(2 hunks)package.json(1 hunks)src/extension.js(3 hunks)src/main.js(6 hunks)test/helpers/mockFactory.js(2 hunks)test/integration/commands.test.js(4 hunks)test/unit/non-text-files.test.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (38)
test/helpers/mockFactory.js (5)
18-18: Good addition of theisNonTextFileparameter.The new parameter enhances the mock factory to simulate non-text files in VS Code's environment, making it possible to test the new functionality thoroughly.
31-43: Nice implementation of the editor mock for different file types.The code correctly simulates VS Code's behavior by setting
editorMockto null for non-text files, which is exactly how VS Code behaves when opening binary files.
45-54: Good approach for simulating non-text files.Adding the
activeEditorPaneproperty with the appropriate structure provides a realistic simulation of how VS Code represents non-text files in its API.
56-80: Well-structured Tab Groups API simulation.The implementation correctly mirrors VS Code's Tab Groups API structure, which is essential for the extension to detect and handle non-text files properly. The detailed mock includes the proper nesting of properties and methods.
86-95: Good addition of Uri methods and window properties.Adding the Uri object with file and parse methods, along with updating the window mock properties, creates a comprehensive testing environment for non-text file scenarios.
README.md (3)
11-22: Clear documentation of non-text file support.The documentation now clearly explains the extension's capabilities for both text and non-text files, with specific examples for each. This will help users understand the expected behavior.
25-33: Good consistency in updating all command descriptions.The changes maintain consistency across all command descriptions, ensuring users understand that all commands work with both text and non-text files.
60-79: Excellent troubleshooting section addition.The new troubleshooting section provides clear, actionable steps for users who encounter issues with non-text files. The section acknowledges limitations and offers workarounds, which is very helpful for user experience.
test/integration/commands.test.js (5)
15-24: Good test refactoring approach.Moving from clipboard stubbing to direct
getGithubUrlstubbing is a cleaner approach that isolates the tests from clipboard interactions, making them more reliable.
47-62: Well-implemented test refactoring for the first command.The test now focuses on the correct execution of the command without relying on clipboard functionality, making it more robust and focused on what's important.
84-99: Consistent test refactoring pattern.Maintaining the same pattern across all command tests ensures consistency and makes the test suite easier to maintain.
121-136: Clean implementation of the default branch URL test.The test correctly verifies the functionality without unnecessary dependencies on external systems.
138-154: Good test organization with skipped tests.Explicitly skipping tests with clear comments explaining why they're skipped (functionality tested elsewhere) is better than removing them, as it documents the test coverage strategy.
test/unit/non-text-files.test.js (10)
7-25: Well-structured test suite setup.The test suite is properly organized with clear documentation of its purpose and setup/teardown hooks that ensure a clean testing environment for each test.
27-46: Good test for PNG file URL generation.The test correctly verifies that URLs for PNG files are generated without line numbers and includes assertions to explicitly check this behavior.
48-69: Thorough testing of permalink functionality for images.The test ensures that permalinks for JPG files work correctly and do not include line numbers, covering an important use case.
71-90: Comprehensive test for default branch URLs with PDF files.The test verifies that default branch URLs work correctly with PDF files and don't include line numbers, ensuring consistent behavior across different file types.
92-112: Good direct API testing with SVG files.Testing direct API calls instead of command execution ensures the core functionality works correctly independent of the command layer.
114-137: Thorough repository location testing for non-text files.This test ensures the extension can correctly locate the repository for non-text files, which is crucial for generating accurate GitHub URLs.
139-146: Good error handling verification.Testing the error case when neither editor nor fileUri is provided ensures robust error handling in the extension.
149-171: Well-implemented TabGroups API test.This test confirms that the extension can correctly use the TabGroups API to generate URLs for non-text files, which is essential for the new functionality.
174-204: Excellent type safety testing for non-array references.This test ensures the code handles unexpected data structures gracefully, preventing potential runtime errors in edge cases.
207-245: Thorough testing of null repository handling.These tests verify that the code can handle both null repositories and non-array repositories without throwing errors, improving robustness.
src/extension.js (2)
25-33: Double-check workspace root detection logic.
The code setsisWorkspaceRootto true if the file’s directory matches exactly one of theworkspaceFolders. This may exclude files that are in subdirectories of the root. If the intent is to detect whether this file is anywhere in the same folder as.git, consider refining the check (e.g., usingvscode.workspace.getWorkspaceFolderfor the file).
144-145: Telemetry event detail for line ranges.
StoringhasLineRange: url.includes('-L')andisTextFile: !!activeTextEditorensures good insight into user behavior. Confirm that-Luniquely identifies line references, as URLs might include it for non-line reasons in a rarer format.package.json (2)
41-41: New debug command addition.
Registeringextension.gitHubUrlDebughelps users troubleshoot non-text file handling. This is a nice addition for diagnosing issues.Also applies to: 43-44
64-82: Explorer context menu for commands.
Providing quick access to URL copying in the explorer context is a user-friendly improvement. Nicely placed under"group": "7_modification". Looks good.src/main.js (11)
19-23: Useful docstring updates.
Explicitly stating non-text file support and describingfileUriusage clarifies the extended functionality well.
26-26: Expanded function signature.
getGithubUrl(editor, type = {}, fileUri = null)effectively covers both text and non-text workflows. Verify all internal calls are updated to handle thefileUriparameter properly.
40-49: URI derivation logic.
Separating text vs. non-text (editorvs.fileUri) is straightforward. Make sure the thrown error when neither is available is tested.
50-54: Repository retrieval call.
Directly passingfileUriintogetRepositoryis a clean approach. Good job bridging the text vs. non-text path.
218-221: Branch config detection.
Fetching references from either.getRefs()or.state.refsis a solid approach to remain compatible with older/later API versions. Good fallback logic.
235-238: Domain override heuristics.
Replacing the domain portion ofremote.fetchUrlwithdomainOverrideis straightforward. If your enterprise or SSH domain has unusual subpath usage, ensure this simple substitution logic is sufficient.
291-293: Workspace folder presence check.
Throwing an error for files outside a VS Code workspace is correct. Make sure tests include a scenario with no workspace.
297-308: Fallback torootGitFolder
This fallback is helpful for multi-root or custom setups. Logging a warning on error is a practical fallback if the user misconfigures or the folder is missing.
360-364: Guarding against missing repository state.
Nicely warns that the Git data might still be loading or is corrupted. This explicit error helps debugging.
389-390: Clear final error if remotes never populate.
Throwing an explicit error is good. This ensures that users or logs show exactly why the operation failed.
392-409: Graceful command execution wrapper.
safeExecuteCommandis a good pattern for ignoring non-critical or known inline chat errors. The inline chat usage note is helpful.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/extension.js (1)
136-147: Consider enhancing the error message for end usersWhile the detailed debug information is excellent for troubleshooting, the error message shown to users ("No active file found") could be more helpful.
Consider enhancing the user-facing error message:
- throw new Error('No active file found') + throw new Error('No active file found. Please ensure a file is open and selected in the editor.')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/extension.js(3 hunks)
🔇 Additional comments (6)
src/extension.js (6)
10-45: Good improvements to telemetry data collection for non-text filesThe changes to
getBaseTelemetryData()now properly handle non-text files by using both the activeEditorPane and tabGroups APIs as fallbacks when no text editor is active. The addition of workspace root detection and file extension tracking will provide valuable telemetry insights.One minor suggestion - consider adding a comment explaining what "isWorkspaceRoot" actually indicates, as it's checking if the file is directly inside a workspace folder rather than anywhere in the workspace.
90-119: Well-structured helper function with comprehensive file detectionThe new
getActiveFileInfo()helper follows a good progressive fallback pattern to identify the active file across multiple VS Code APIs. The function is well-documented and the return value clearly indicates whether we found a text editor or just a file URI.The code properly handles each API check and gracefully returns when no file is found without exposing sensitive paths in logs.
122-170: Good implementation of file type-aware URL generationThe updated
generateCommandBodynow correctly handles both text files and non-text files by leveraging the new helper function. The error logging when no file is found is comprehensive and will help with troubleshooting.The telemetry updates properly track whether the URL is from a text editor or other file type, which will be valuable for understanding extension usage.
177-212: Excellent debug command implementationThe debug command is well-implemented and provides comprehensive information about the editor state, which will be extremely valuable for troubleshooting issues related to non-text file handling.
The approach of both logging to console and showing in a notification is user-friendly, and including the full path information for local debugging is thoughtful.
194-200: Be cautious about exposing file paths in user-visible notificationsWhile debug information is valuable, showing full file paths in information messages might expose sensitive information if users share screenshots.
Consider separating sensitive path information from what gets shown in the notification:
- // Show complete debug info in the notification for easier troubleshooting - await vscode.window.showInformationMessage(JSON.stringify(info, null, 2)) + // Show non-sensitive debug info in the notification for easier troubleshooting + const safeInfo = {...info}; + delete safeInfo.uri; + delete safeInfo.fsPath; + await vscode.window.showInformationMessage(JSON.stringify(safeInfo, null, 2))
217-218: Properly registered debug commandGood job adding the debug command to the list of disposables to ensure proper cleanup when the extension is deactivated.
|
Nearly all changes in this PR were written by Claude Code. I made 2-3 small code tweaks at most. 🪄 |
Fixes #61
Summary by CodeRabbit
Documentation
New Features
Bug Fixes / Improvements