feat: ghost existing settings, implement handlers for custom html set… #688
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello @nick-y-snyk, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors how Snyk extension settings are managed and presented to the user. It transitions from relying on VS Code's native settings UI to a custom HTML-based webview, which fetches its content from the language server. This change aims to provide a more tailored and potentially richer configuration experience, centralizing Snyk-specific settings within a dedicated interface and offering robust handling for saving user preferences. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a new webview-based settings page, moving configuration out of package.json. The implementation is mostly solid, but there are a few critical areas for improvement. The most significant issue is the custom, unsafe parsing of settings.json, which could lead to user data loss. I've flagged this as critical. Additionally, the error logging has some redundancy, and the new unit tests are incomplete, missing coverage for the file-writing logic and including an obsolete test case. Addressing these points will make the new feature much more robust and safe for users.
| try { | ||
| // Use VS Code's jsonc-parser for proper JSONC parsing | ||
| // For now, use a more conservative approach to strip comments | ||
| const lines = originalContent.split('\n'); | ||
| const cleanedLines = lines.map(line => { | ||
| // Remove inline comments but preserve strings with // | ||
| const stringMatches: string[] = []; | ||
| let cleaned = line.replace(/"(?:[^"\\]|\\.)*"/g, match => { | ||
| stringMatches.push(match); | ||
| return `__STRING_${stringMatches.length - 1}__`; | ||
| }); | ||
|
|
||
| // Now remove comments | ||
| cleaned = cleaned.replace(/\/\/.*$/, ''); | ||
|
|
||
| // Restore strings | ||
| cleaned = cleaned.replace(/__STRING_(\d+)__/g, (_, index) => stringMatches[parseInt(index)]); | ||
|
|
||
| return cleaned; | ||
| }); | ||
|
|
||
| let cleanContent = cleanedLines.join('\n'); | ||
| // Remove block comments | ||
| cleanContent = cleanContent.replace(/\/\*[\s\S]*?\*\//g, ''); | ||
| // Remove trailing commas | ||
| cleanContent = cleanContent.replace(/,(\s*[}\]])/g, '$1'); | ||
|
|
||
| currentSettings = JSON.parse(cleanContent); | ||
| } catch (e) { | ||
| // Parsing failed - fail fast to avoid data loss | ||
| this.logger.error(`Failed to parse settings.json: ${e}`); | ||
| throw new Error(`Cannot parse settings.json. Please check for syntax errors: ${e}`); | ||
| } |
There was a problem hiding this comment.
The custom implementation for parsing settings.json (which is a JSONC file, allowing comments) is very risky. It uses a series of regular expressions to strip comments and trailing commas. This approach is brittle and can easily fail on edge cases, leading to corruption of the user's settings.json file. A corrupted settings file can break the user's entire VS Code environment.
Instead of a custom parser, please use a robust, well-tested library built for this purpose, such as jsonc-parser. The comment on line 252 already suggests this. Using a proper library will eliminate the risk of corrupting user settings.
| import { LoggerMock } from '../../mocks/logger.mock'; | ||
| import { IVSCodeCommands } from '../../../../snyk/common/vscode/commands'; | ||
|
|
||
| suite('WorkspaceConfigurationWebviewProvider', () => { |
There was a problem hiding this comment.
The current tests only cover fetching HTML. The most critical functionality in WorkspaceConfigurationWebviewProvider—the writeToSettingsJson method—is completely untested. This method performs file I/O and modifies a critical user file (settings.json). Without tests, there's a high risk of introducing bugs that could corrupt user settings.
Please add comprehensive unit tests for writeToSettingsJson that cover:
- Reading and parsing
settings.json(especially with comments). - Merging new settings correctly.
- The atomic write logic (backup, write, rename).
- Error handling scenarios (e.g., file not found, parse errors, write errors).
Mocking the fs/promises module would be necessary for these tests.
| test('processHtml returns HTML as-is for complete documents', () => { | ||
| const processed = (provider as any).processHtml(sampleHtml); | ||
|
|
||
| strictEqual(processed, sampleHtml); | ||
| }); |
There was a problem hiding this comment.
This test case, processHtml returns HTML as-is for complete documents, seems to be for a method processHtml that no longer exists in WorkspaceConfigurationWebviewProvider. This test should be removed to avoid confusion and keep the test suite clean.
| private async handleSaveConfig(configJson: string): Promise<void> { | ||
| try { | ||
| const config = JSON.parse(configJson); | ||
| this.logger.info('Saving workspace configuration'); | ||
|
|
||
| await this.writeToSettingsJson(config); | ||
|
|
||
| this.logger.info('Workspace configuration saved successfully'); | ||
| } catch (e) { | ||
| this.logger.error(`Failed to save workspace configuration: ${e}`); | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
The error handling in handleSaveConfig is redundant. It catches, logs, and re-throws an error. The calling function, handleMessage, also has a try...catch block that will log the same error again. This creates noise in the logs. It's better to have a single point of error logging in the caller, which in this case is handleMessage.
private async handleSaveConfig(configJson: string): Promise<void> {
const config = JSON.parse(configJson);
this.logger.info('Saving workspace configuration');
await this.writeToSettingsJson(config);
this.logger.info('Workspace configuration saved successfully');
}
…or by Snyk Language Server
…tings page
Description
Provide description of this PR and changes, if linked Jira ticket doesn't cover it in full.
Checklist
Screenshots / GIFs
Visuals that may help the reviewer. Please add screenshots for any UI change. GIFs are most welcome!