fix: cache extension enablement config to prevent error spam #13674
Conversation
Summary of ChangesHello @jduncan-rva, 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 improves the robustness and performance of the extension enablement configuration management. By implementing a caching strategy, it prevents excessive file system operations and reduces the volume of error logs that occur when the configuration file is either missing or malformed. The changes also provide more informative error feedback, making it easier to diagnose configuration-related issues. 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 caching for the extension enablement configuration, which is a good approach to prevent repeated file reads and error logging, especially for invalid or missing files. The implementation correctly caches the configuration on read and updates the cache on write. I've included one suggestion to refactor the error handling logic in readConfig. This refactoring will make the code more concise by removing duplication and will also enhance error reporting for JSON syntax errors, which aligns with one of the stated goals of this PR.
| } catch (error) { | ||
| if ( | ||
| error instanceof Error && | ||
| 'code' in error && | ||
| error.code === 'ENOENT' | ||
| (error as { code: string }).code === 'ENOENT' | ||
| ) { | ||
| return {}; | ||
| this.cachedConfig = {}; | ||
| return this.cachedConfig; | ||
| } | ||
| coreEvents.emitFeedback( | ||
| 'error', | ||
| 'Failed to read extension enablement config.', | ||
| `Failed to read extension enablement config at ${this.configFilePath}.`, | ||
| error, | ||
| ); | ||
| return {}; | ||
| this.cachedConfig = {}; | ||
| return this.cachedConfig; | ||
| } |
There was a problem hiding this comment.
The catch block can be refactored to be more concise and to better align with the PR's goal of improving error reporting for syntax errors. The current implementation contains duplicated code for caching an empty configuration and uses a generic error message for parsing failures.
By restructuring this block, we can:
- Eliminate code duplication for caching an empty config (
this.cachedConfig = {}). - Provide a more specific and helpful error message when
JSON.parsefails due to aSyntaxError. - Consistently cache an empty config on any read failure to prevent error spam, while selectively logging errors (i.e., not logging for a missing file, which is an expected condition).
| } catch (error) { | |
| if ( | |
| error instanceof Error && | |
| 'code' in error && | |
| error.code === 'ENOENT' | |
| (error as { code: string }).code === 'ENOENT' | |
| ) { | |
| return {}; | |
| this.cachedConfig = {}; | |
| return this.cachedConfig; | |
| } | |
| coreEvents.emitFeedback( | |
| 'error', | |
| 'Failed to read extension enablement config.', | |
| `Failed to read extension enablement config at ${this.configFilePath}.`, | |
| error, | |
| ); | |
| return {}; | |
| this.cachedConfig = {}; | |
| return this.cachedConfig; | |
| } | |
| } catch (error) { | |
| const isEnoent = | |
| error instanceof Error && | |
| 'code' in error && | |
| (error as { code: string }).code === 'ENOENT'; | |
| if (!isEnoent) { | |
| let message = `Failed to read or parse extension enablement config at ${this.configFilePath}.`; | |
| if (error instanceof SyntaxError) { | |
| message += `\nDetails: ${error.message}`; | |
| } | |
| coreEvents.emitFeedback('error', message, error); | |
| } | |
| this.cachedConfig = {}; | |
| return this.cachedConfig; | |
| } |
1 participant
Summary
This PR implements caching for the extension enablement configuration to prevent excessive file reads and error logging when the configuration file is invalid or missing. It also improves error reporting to include specific syntax errors.
Details
See above
Related Issues
N/A - uncovered this bug testing new extensions combinations.
How to Validate
have a typo in extension-enablement.json with an enable/disable list
start gemini (interactive or not)
you get multiple reports of an issue, but no details as to what/why
this gives you a better output and cuts out repetitive error noise.
Pre-Merge Checklist