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 significantly refactors the extension update mechanism to enhance robustness, testability, and maintainability. By transitioning to a reducer-based approach for managing update states and scheduling, it centralizes the logic for checking and applying updates. This change also streamlines the API for individual update checks and ensures correct working directory context is consistently applied, paving the way for future command implementations. 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
|
There was a problem hiding this comment.
Code Review
This pull request is a great refactoring that centralizes the extension update logic using a reducer pattern, which significantly improves maintainability and testability. The checkForExtensionUpdate function is now purer by returning a value directly, and the slash command has been simplified to just dispatch actions. Overall, these are excellent changes that align well with modern state management practices. I've identified one critical syntax issue that would lead to a crash and a related high-severity logical flaw in the new reducer logic for handling concurrent updates. My review comments provide details and suggestions for fixes.
|
Size Change: +970 B (+0.01%) Total Size: 17.6 MB ℹ️ View Unchanged
|
| ); | ||
| }, [ | ||
| extensions, | ||
| extensionsUpdateState.extensionStatuses, |
There was a problem hiding this comment.
Note that this is a new dependency - but it doesn't create a render loop even though we also call dispatchExtensionStateUdpate - because these states are linear.
This ensures we don't keep checking for updates to extensions that we have already checked or are currently checking.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| if (action.type === 'SET_STATE') { | ||
| extensionState.set(action.payload.name, { | ||
| status: action.payload.state, | ||
| notified: true, // No need to process as we will force the update. |
There was a problem hiding this comment.
update the comment to align with the code.
Optional nit: this is now a lie as we haven't notified.
to be overly pedantic we could label this
notificationRequested or something similar. Then it can be false here to exactly indicate the state.
There was a problem hiding this comment.
I could also just remove this entirely, its not necessary in this code path, and this code path does not do any notifications of available updates, it always installs them.
There was a problem hiding this comment.
I went ahead and just made that change, to just not indicate we notified. This code path is for the regular update command outside the CLI itself.
jacob314
left a comment
There was a problem hiding this comment.
Congrats on your first big reducer change! Approved once these comments are addressed.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

TLDR
Avoids repeatedly checking for updates when we have already done so and leans into the reducer approach more heavily.
Dive deeper
Various cleanups to rely more heavily on the reducer, and handle all updates from within
useExtensionUpdates- the slash command also just dispatches an event to the reducer now.Also cleaned up the
checkForExtensionUpdatefunction so it just returns an extension state instead of internally calling a function to update the state, this makes it more testable/portable outside of react.checkForAllExtensionUpdatesnow also takes a current working directory, which fixes a potential bug in at least one place where we should have been flowing through the working dir from the context instead of usingprocess.cwd().This also paves the way for a future
/extensions checkcommand which can similarly just fire an event on a reducer.We also have a centralized place handling all the update checks as well as actual updates, and this should help with any race conditions that may have existed before.
Testing Matrix
Linked issues / bugs