feat(vscode): Rewrite extension as LSP client#29
Conversation
- Rewrite extension.js to use vscode-languageclient LSP client (v9 Executable API) - Update package.json: version 0.2.0, repository, commands, configuration settings - Add LICENSE, .vscodeignore, CHANGELOG.md - Add .vscode/launch.json and .vscode/tasks.json - Update README.md with LSP configuration docs - Install dependencies: vscode-languageclient ^9, @vscode/vsce All 4/4 red test suites pass.
- Add 5 test suites (52 assertions) validating the LSP client rewrite - Clean up extension.js: remove unused path import, extract error message variable - Tests cover: extension activation, file structure, manifest, package.json, edge cases
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 39 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
diffguard found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the DiffGuard VS Code extension to utilize the Language Server Protocol (LSP), providing real-time diagnostics and new configuration options. A major issue was noted regarding the accidental inclusion of the node_modules directory in the repository. Key feedback includes suggestions to exclude the entire node_modules folder in .vscodeignore to optimize package size, add support for unsaved files in the documentSelector, and use more reliable error code checking when verifying the presence of the language server binary.
| .vscode/ | ||
| .gitignore | ||
| *.vsix | ||
| node_modules/.cache/ |
There was a problem hiding this comment.
The .vscodeignore file should exclude the entire node_modules/ directory rather than just the cache. Including node_modules in the extension package (VSIX) significantly increases its size and can lead to installation issues. If you are not using a bundler, vsce will automatically include only production dependencies if node_modules is ignored correctly.
node_modules/
| @@ -0,0 +1 @@ | |||
| ../glob/dist/esm/bin.mjs No newline at end of file | |||
There was a problem hiding this comment.
It appears that the node_modules directory has been committed to the repository. This is generally discouraged as it bloats the repository size and can cause platform-specific issues with binary dependencies. Dependencies should be managed via package.json and ignored by git. Please remove the node_modules directory from the repository and ensure it is added to your .gitignore file.
| }; | ||
|
|
||
| const clientOptions = { | ||
| documentSelector: [{ scheme: "file" }], |
There was a problem hiding this comment.
The documentSelector is currently restricted to the file scheme. Since DiffGuard is intended to analyze all file types for PII and secrets, it should also include the untitled scheme to provide diagnostics for new, unsaved files. This ensures that users get real-time feedback even before saving a file for the first time.
| documentSelector: [{ scheme: "file" }], | |
| documentSelector: [{ scheme: "file" }, { scheme: "untitled" }], |
|
|
||
| client.start().then(null, (err) => { | ||
| const errMsg = String(err); | ||
| if (errMsg.includes("ENOENT") || errMsg.includes("not found") || errMsg.includes("cannot find")) { |
There was a problem hiding this comment.
Checking for a missing binary using string matching on the error message is brittle and may fail depending on the platform or system locale (e.g., localized error messages). It is more reliable to check the error code (e.g., err?.code === 'ENOENT') if the error object provides it.
| if (errMsg.includes("ENOENT") || errMsg.includes("not found") || errMsg.includes("cannot find")) { | |
| if (err?.code === "ENOENT" || errMsg.includes("ENOENT") || errMsg.includes("not found") || errMsg.includes("cannot find")) { |
💡 Codex Reviewdiffguard/editors/vscode-diffguard/package.json Lines 25 to 26 in 630d421  Stop exposing explainRule without collecting its required arg
The manifest advertises diffguard/editors/vscode-diffguard/package.json Lines 56 to 57 in 630d421 Restrict maxFindings setting to integer values
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Full post-mortem covering gate-by-gate assessment, design quality analysis, and 5 specific conveyor pipeline fixes needed. Updated wisdom.md with concise learnings pointing to the detailed doc.
2 participants
Closes #28
What Changed
Rewrites the VS Code extension from a custom diagnostic implementation to a standard Language Server Protocol (LSP) client using v9 Executable API.
Extension Rewrite (extension.js)
Package & Manifest Updates (package.json)
New Files
Why
The previous implementation used a custom diagnostic collection approach that was tightly coupled and limited. Moving to LSP provides:
Test Results
All 5/5 test suites pass (52 assertions):
Key Decisions