-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Move IdeClient.connect() to initializeApp(). #8282
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
Size Change: -307 B (0%) Total Size: 13.2 MB ℹ️ View Unchanged
|
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. |
418a861
to
0de7874
Compare
/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 couple of nice refactorings. First, it moves the IdeClient
connection logic from Config.initialize()
into initializeApp()
. This is a good architectural improvement that ensures the IDE client is ready before the UI starts, and it also improves the separation of concerns between the core
and cli
packages. Second, it removes the getIdeTrust
helper function and its associated file, inlining the logic where it's used. This simplifies the codebase by removing an unnecessary layer of abstraction. The changes are well-implemented and the tests have been updated accordingly. Overall, this is a solid improvement to the codebase.
TLDR
Moves IdeClient initialization out of
config.initialize()
and intoinitializeApp()
.Also, refactor to remove unnecessary file that contained one method used in exactly one other file.
Dive Deeper
IdeClient needs to be initialized before react starts because we might need to show a popup right away.
Reviewer Test Plan
Testing Matrix
Linked issues / bugs
Fixes #8274