-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Support context injection via SessionStart hook. #15746
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
Conversation
Summary of ChangesHello @gundermanc, 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 introduces a significant enhancement by enabling the 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
|
|
Size Change: +1.5 kB (+0.01%) Total Size: 22.2 MB
ℹ️ View Unchanged
|
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 successfully implements context injection via the SessionStart hook. The changes are well-structured, covering both non-interactive and interactive modes. In non-interactive mode, context is correctly prepended to the input, and in interactive mode, it's added to the conversation history. The addition of a comprehensive integration test validates the new functionality effectively. A notable improvement is the change to wrap the FakeContentGenerator with LoggingContentGenerator, which enhances testability by allowing API request inspection even with fake responses. Overall, the implementation is robust and the code quality is high.
5ab9c61 to
935dccd
Compare
fcd35b0 to
1af770f
Compare
| // item, including the ones from the start session hook will cause a | ||
| // re-render and an error when we try to reload config. | ||
| // | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
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.
One thing I want to call out is that our use of useEffect() for first-time initialization is pretty brittle and possible bad practice, though I'm not a react expert.
- We load the config inside of the useEffect()
- Config expects to only get loaded once, so if the use effect runs multiple times, it causes an error.
- Adding the new item to history with historyManager trips a linter error that wants us to include historyManager in the dependency array.
- Doing so causes us to react to the
addItem()call, causing the useEffect() to run again immediately after the first time.
For now I'm just excluding the historyManager from the dependencies because I think we're really just trying to cause a side effect here, not cause a redundant repaint, but I think we should rethink this in the future.
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.
cc @jacob314, @scidomino do either of you have any insights?
| if (result?.systemMessage) { | ||
| context.ui.addItem( | ||
| { | ||
| type: MessageType.INFO, |
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.
I wonder if we should have a new message type for information like this. Right now we utilize this for "updates pending" or "warning xyz", it's a bit of everything.
Don't block on this comment (i.e. can always do in another PR) but food for thought. It's worth putting out a parallel feeler to UX folks to see their take.
| 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.
Should this also be adding a message to history post-clear?
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.
Ah, great catch!
Summary
Finishes wiring up systemMessage and additionalContext fields from the SessionStart hook.
Now you can inject context and info messages into the history using a SessionStart hook.
Details
Related Issues
Fixes #15413.
How to Validate
Pre-Merge Checklist