-
Notifications
You must be signed in to change notification settings - Fork 31
refactor: use NotificationQueue for async LS notification handlers [IDE-1661] #706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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 addresses a critical bug where rapid interaction with the auto organization setting in the HTML settings UI could lead to an infinite loop of configuration updates. This was due to a race condition between different components attempting to write to the same configuration simultaneously. The fix introduces a robust asynchronous processing queue and refined logic for configuration updates to prevent these circular dependencies and ensure stable behavior, significantly improving the reliability of configuration management. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a robust solution to prevent an infinite loop when toggling settings. The core of the fix is the new NotificationQueue, which serializes asynchronous operations coming from the language server, preventing race conditions. The implementation of the queue is well-designed and comes with a comprehensive set of unit tests. Additionally, the logic for updating settings has been improved to avoid redundant writes, further contributing to stability. The related refactoring from promise-based chains to async/await improves code readability and maintainability. I have a couple of suggestions to further improve the code quality, but overall this is a great set of changes.
| .updateTokenAndEndpoint(token, apiUrl) | ||
| .then(() => { | ||
| void this.notificationQueue.enqueue({ | ||
| id: `authenticated-${Date.now()}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using Date.now() for unique IDs is not fully reliable, as multiple notifications could arrive within the same millisecond, leading to duplicate IDs. This can make tracing and debugging difficult. To improve uniqueness, consider appending a random number. A similar change should be applied to other enqueue calls in this file (lines 252 and 270).
| id: `authenticated-${Date.now()}`, | |
| id: `authenticated-${Date.now()}-${Math.random()}`, |
| * Enqueues an item for processing. Items are processed sequentially in FIFO order. | ||
| * Uses a promise chain to ensure sequential processing without explicit locks or void. | ||
| */ | ||
| async enqueue<T>(item: QueueItem<T>): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enqueue method is marked as async but doesn't use await, which can be slightly misleading as it resolves immediately. Since the method's logic is synchronous, you can remove the async keyword and change the return type to void to more accurately reflect its behavior. Note that this would require removing the await from calls to this method in the corresponding test file.
| async enqueue<T>(item: QueueItem<T>): Promise<void> { | |
| enqueue<T>(item: QueueItem<T>): void { |
When rapidly toggling the auto organization checkbox in the HTML settings page, an infinite loop of settings.json updates occurred. This was caused by both the HTML autosave and ConfigurationWatcher writing to FOLDER_CONFIGS simultaneously. Changes: - Add flag-based prevention in ConfigurationWatcher to skip processing when HTML settings page is updating configuration - Refactor LanguageServer notification handlers to use NotificationQueue for sequential async processing, preventing race conditions - Make handleOrgSettingsFromFolderConfigs async and process folder configs sequentially - Update scopeDetectionService to check if values changed before writing - Add comprehensive tests for NotificationQueue The flag approach cleanly separates HTML-initiated updates from VS Code UI updates, preventing circular writes while maintaining normal functionality.
5fa8a1a to
b93f1b1
Compare
Summary
Refactors LanguageServer notification handlers to use a NotificationQueue for sequential async processing, eliminating race conditions and enabling proper async/await handling.
Changes
NotificationQueue (new)
LanguageServer.stop()LanguageServer
SNYK_HAS_AUTHENTICATED,SNYK_FOLDERCONFIG, andSNYK_ADD_TRUSTED_FOLDERSnotifications with NotificationQueuehandleOrgSettingsFromFolderConfigsasync and processes folder configs sequentially with for-loop instead of forEach.then()/.catch()chains with proper async/awaitscopeDetectionService
isSettingsWithDefaultValuetoshouldSkipSettingUpdatefor clarityTests
handleOrgSettingsFromFolderConfigscallsoutputChannelmocks and restoressinon.verify()callsBenefits
Test Plan