Pull master in jojwang-localbuild#9147
Pull master in jojwang-localbuild#9147annajowang wants to merge 7 commits intojojwang-localbuildfrom
Conversation
* handle error * m
- Improving instructions and return values in crashlytics tools - Making interval handling more robust - Adding return messages to avoid undefined tool responses
…ager to do more than check your login status (#9133) * Tweak the connect prompt to look at gitignored files and to be less eager to do more than check your login status * Loosen prescriptiveness, ease testing proactivity, come up with multiple root causes * Respond to prompt review comments.
… in Firebase ToS (#9143) * update_environment can be used to accept Gemini ToS * changelog * Update src/mcp/tools/core/update_environment.ts Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * m * m * m * m * m --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Summary of ChangesHello @annajowang, 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 significant enhancements to the Firebase CLI's Crashlytics API interactions and the Model Context Protocol (MCP) tooling, while also cleaning up the Data Connect VSCode extension. The changes focus on improving the developer experience by providing more granular control over Crashlytics data, offering clearer guidance within the MCP environment, and refining internal error handling and project configuration management. 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 introduces a significant number of updates and refactorings, primarily centered around the Model Context Protocol (MCP) tools. Key changes include adding support for accepting Gemini Terms of Service, introducing a new batchGetEvents function for Crashlytics, and extensive improvements to the prompts and logic for AI-driven features. The codebase has been refactored for better clarity and robustness, such as centralizing error handling and API checks. Overall, the changes are positive and enhance the functionality and maintainability of the tools. I've identified a couple of areas for improvement: a misleading comment in the tool definition and a potential null reference bug in the update_environment tool.
| if (active_user_account) { | ||
| assertAccount(active_user_account, { mcp: true }); | ||
| setProjectAccount(host.cachedProjectRoot!, active_user_account); | ||
| setProjectAccount(host.cachedProjectDir!, active_user_account); | ||
| output += `- Updated active account to '${active_user_account}'\n`; | ||
| } |
There was a problem hiding this comment.
There's a potential for a runtime error here. The update_environment tool has optionalProjectDir: true, which means host.cachedProjectDir can be undefined. However, setProjectAccount is called with a non-null assertion (host.cachedProjectDir!). If active_user_account is provided without a project directory being set (either in this call or previously), this will cause a crash. You should add a check to ensure host.cachedProjectDir is defined before calling setProjectAccount.
| if (active_user_account) { | |
| assertAccount(active_user_account, { mcp: true }); | |
| setProjectAccount(host.cachedProjectRoot!, active_user_account); | |
| setProjectAccount(host.cachedProjectDir!, active_user_account); | |
| output += `- Updated active account to '${active_user_account}'\n`; | |
| } | |
| if (active_user_account) { | |
| if (!host.cachedProjectDir) { | |
| return mcpError( | |
| "Cannot set active user account without a project directory. Please specify 'project_dir'.", | |
| ); | |
| } | |
| assertAccount(active_user_account, { mcp: true }); | |
| setProjectAccount(host.cachedProjectDir, active_user_account); | |
| output += `- Updated active account to '${active_user_account}'\n`; | |
| } |
| /** Set this on a tool if it cannot work without a Firebase project directory. */ | ||
| optionalProjectDir?: boolean; |
There was a problem hiding this comment.
The comment for optionalProjectDir is misleading. It states "Set this on a tool if it cannot work without a Firebase project directory.", but the implementation logic (if (!tool.mcp._meta?.optionalProjectDir)) implies it should be set to true if the project directory is optional. This can cause confusion for developers creating new tools. The comment should be updated to reflect the actual behavior.
| /** Set this on a tool if it cannot work without a Firebase project directory. */ | |
| optionalProjectDir?: boolean; | |
| /** Set this to true on a tool if it can work without a Firebase project directory. */ | |
| optionalProjectDir?: boolean; |
Description
Scenarios Tested
Sample Commands