-
Notifications
You must be signed in to change notification settings - Fork 8k
Pull contentGenerator out of GeminiClient and into Config. #7825
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
/gemini review |
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 significant and well-executed refactoring by moving the ContentGenerator
from GeminiClient
to the Config
class. This change improves the architecture by decoupling the GeminiClient
's lifecycle from authentication, allowing auth to be refreshed without losing chat history. The implementation is consistent across the codebase, with related classes and tests updated to reflect the new structure. The refactoring of the test suites, particularly moving to mock internal abstractions rather than the external SDK, is a notable improvement in test quality and robustness. The changes align with the pull request's goals and I have no major concerns.
15d6305
to
35baa93
Compare
35baa93
to
4c8e43e
Compare
Code Coverage Summary
CLI Package - Full Text Report
Core Package - Full Text Report
For detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
The Check Bundle Size error is a preexisting bug that will be fixed by #7833 |
bfe90fd
to
03f2ae4
Compare
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.
LGTM! Just a couple of questions. Thanks again for doing this! Makes my refactor so much easier ❤️
0562e96
to
6f3bb2c
Compare
getContentGenerator(): ContentGenerator { | ||
if (!this.contentGenerator) { | ||
private getContentGeneratorOrFail(): ContentGenerator { | ||
if (!this.config.getContentGenerator()) { |
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.
optional nit: add a local variable to avoid calling config.getContentGenerator()
twice.
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.
6f3bb2c
to
2f15683
Compare
Size Change: -376 B (0%) Total Size: 12.5 MB ℹ️ View Unchanged
|
e441e88
to
0135134
Compare
TLDR
Move ContentGenerator from
config.geminiClient.contenGenerator
toconfig.contentGenerator
.Dive Deeper
Necessary for #4544 which requires that we refreshAuth before loading the mcp servers.
Config.refreshAuth()
and intoConfig.initialize()
. You still need to call both before getting started but now you can call them in either order.GeminiClient
is now created exactly once in theConfig
constructor and is never reset (instead of a new GeminiClient being built every time we callrefreshAuth()
).stripThoughtsFromHistory
code intoGeminiChat
(where the history lives).Reviewer Test Plan
Testing Matrix
Linked issues / bugs
Contributes to #4544