-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[VS Code] Edit missing variables / auth #9301
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
base: master
Are you sure you want to change the base?
Conversation
Instead of directly asking for an app description to generate a schema with Gemini, this change first asks the user to confirm if they want to use Gemini. If the user confirms, it then prompts for the app description with a default value of "an app for ${setup.projectId}".
… into feat-dataconnect-gemini-confirm
Summary of ChangesHello @fredzqm, 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 focuses on enhancing the developer experience within the Firebase VS Code extension and CLI, particularly for Data Connect features. It introduces a user-friendly confirmation for Gemini-powered schema generation, improves the accuracy of default variable assignments for GraphQL mutations, and streamlines internal analytics. Additionally, it refines user prompts and GA4 tracking to provide a more intuitive and informative setup process. 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.
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 several changes to the VS Code extension, primarily around handling missing variables, authentication, and improving the firebase init dataconnect
flow. My review focuses on some regressions in analytics tracking and a critical issue that would likely cause a build failure. I've also included a suggestion for improving a default value.
/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 valuable refactoring of the GraphQL execution flow in the VS Code extension. The changes improve the user experience for handling variables and authentication, and the code structure is now more modular and maintainable. Key improvements include renaming "Configuration" to "Parameters", introducing an ExecutionParamsService
to centralize parameter management, and enhancing the query re-run functionality.
My review focuses on a few areas for improvement:
- React component lifecycle: There are critical issues in React components where event listeners are not correctly managed, which will lead to memory leaks and bugs.
- Error handling: An edge case in JSON parsing for authentication claims is not handled, which could lead to unhandled exceptions.
- Code duplication: There's an opportunity to reduce code duplication in the CodeLens provider.
- UI regression: A small regression was found where error stack traces are no longer displayed in one of the views.
Overall, this is a solid pull request. Addressing these points will further improve the robustness and quality of the code.
firebase-vscode/webviews/data-connect/data-connect-execution-parameters.entry.tsx
Outdated
Show resolved
Hide resolved
firebase-vscode/webviews/data-connect/data-connect-execution-parameters.entry.tsx
Outdated
Show resolved
Hide resolved
firebase-vscode/webviews/data-connect/DataConnectExecutionResultsApp.tsx
Show resolved
Hide resolved
/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 valuable set of improvements to the Data Connect execution flow. The refactoring to centralize parameter handling into ExecutionParamsService
, the removal of the disruptive modal for missing variables in favor of an auto-fix mechanism, and the enhanced results panel are all excellent changes that will improve the developer experience. The code is cleaner and the state management is simpler with the removal of lastExecutedOperation
. I've found a couple of minor issues in the new logic, mainly related to analytics, error logging, and UI display, which I've detailed in my comments. Overall, this is a high-quality contribution.
firebase-vscode/webviews/data-connect/DataConnectExecutionResultsApp.tsx
Show resolved
Hide resolved
Hi, @hlshen Sorry about the massive PR. I went though the reworked the VS Code execution param and result penel with some incremental fixes (started from the bug missing variable logic). Not in a rush, but can use your expertise to review it~ Thank you! |
: new DataConnectError("Unknown error", error), | ||
}); | ||
if (item.state === ExecutionState.ERRORED) { | ||
await paramsService.applyDetectedFixes(ast); |
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.
Two biggest concerns with the auto generated fields + auto rerun:
-
This is also applying to production. Can we add a confirmation for production mutations specifically?
-
Users will encounter an error on a mutation w/ repeated ID the second time around. Not a huge concern, but users may now expect it to auto-magically work every time.
} | ||
} | ||
|
||
async applyDetectedFixes(ast: OperationDefinitionNode): Promise<void> { |
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.
It'd be great to unit test this file/function specifically, ensuring it doesn't overwrite user set varisbles.
Description
Scenarios Tested
https://screencast.googleplex.com/cast/NDYyMjE3NjYwMTc2NzkzNnw2YmY1OTg2YS1lNg
Sample Commands