Fix duplicate info messages for extension updates#9760
Conversation
Summary of ChangesHello @jakemac53, 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 resolves a race condition that caused inconsistent and often duplicate notifications for extension updates during application startup. By serializing the update check process and refining state management within the update functions, it ensures that users receive a single, accurate notification when extensions require updates, significantly improving the user experience and system stability. 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
|
|
Size Change: +133 B (0%) Total Size: 17.4 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Code Review
This pull request aims to fix a race condition that caused duplicate notifications for extension updates. The approach involves adding a locking mechanism to prevent multiple update checks from running concurrently. While the intent is correct, the implementation in the useExtensionUpdates hook has a critical flaw that will cause an infinite render loop. Additionally, a related change in checkForAllExtensionUpdates introduces a bug that could lead to loss of state. I have provided detailed comments on both of these issues.
| extensionsUpdateState, | ||
| setExtensionsUpdateState, | ||
| ); | ||
| let extensionsWithUpdatesCount = 0; |
There was a problem hiding this comment.
Just double-checking, how will this work if there are also some auto-updatable extensions?
There was a problem hiding this comment.
They don't contribute to this count
|
|
||
| (async () => { | ||
| if (isChecking) return; | ||
| setIsChecking(true); |
There was a problem hiding this comment.
be very careful adding logic like this that unconditionally updates react state. This code causes us to render frames continuously causing flicker because we are continuously setting setIsChecking to true and then back to false.
TLDR
Previously there was a race condition where you could get zero, one, or multiple possible notifications for updates to extensions. This fixes that so you just get one notification reliably.
I also combined all the info messages into one generic message like this:
Dive Deeper
The primary issue is this useExtensionState function being called many times on startup in quick succession, which was resulting in simultaneous ongoing calls to check the extension update states. These were all mutating the same extension state object and getting in race conditions.
I added a fix to ensure we only have one ongoing check at a time by adding an extra boolean state object to track if we have an ongoing update check already happening.
I also added a fix to not edit the original extension state object but instead return a new one from
checkForAllExtensionUpdates, this was creating the situation where we would have no log at all sometimes due to the previous and new states being equal (the same object).Reviewer Test Plan
Run the CLI with extensions installed that need updates, you should just see one message to update.
Testing Matrix
Linked issues / bugs