feat(mcp): revamp firebase init with orchestration API integration #9174
Conversation
… before real API integration
Summary of ChangesHello @TrCaM, 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 significantly enhances the Firebase initialization experience by revamping the 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-architected feature for provisioning Firebase resources through an orchestration API. The changes are extensive, including new modules for provisioning, utility functions, and comprehensive tests. The core logic in init.ts is well-structured to handle the new provisioning flow, including conflict resolution and configuration file management. My review focuses on improving error handling and code robustness. I've identified a couple of areas for improvement: ensuring consistent error propagation by using FirebaseError and adding a default case to a switch statement for better robustness. Overall, this is a solid contribution.
| async ({ features, provisioning, project, app }, { projectId, config, rc }) => { | ||
| validateProvisioningInputs(provisioning, project, app); | ||
|
|
||
| if (provisioning?.enable) { |
There was a problem hiding this comment.
Haven't looked in-depth yet.
High-level thought is the orchestration API specific logic should be pulled out to a separate package.
Files in commands/* are entry point to CLI commands. Those are entry point to MCP tools. We should keep it focused on defining the input argument of the tool and delegate involved logics to other modules.
There is a great chance the orchestration API could be re-used in other part of the CLI repos.
| askQuestions: features.apptestingAskQuestions, | ||
| actuate: features.apptestingAcutate, | ||
| }, | ||
| { |
There was a problem hiding this comment.
If it's not ready to launch, we should gate it by an experiment or leave it commented out now.
| /** | ||
| * Finds existing Android app with the given package name | ||
| */ | ||
| export async function findExistingAndroidApp( |
There was a problem hiding this comment.
BTW, there is a detectApps helper that we added for FDC.
Sounds like we are trying to achieve the same goal here.
If possible, consider re-use.
Feel free to ignore this comment if the use case is significantly different.
| try { | ||
| const request = buildProvisionRequest(options); | ||
|
|
||
| const response = await apiClient.request<unknown, { name: string }>({ |
There was a problem hiding this comment.
Same though as above - we build the request ourselves, it is not unknown. Use a stronger type.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant revamp of the firebase init tool for MCP, integrating it with an orchestration API for provisioning Firebase projects, apps, and AI Logic. The changes are extensive and well-structured, including new modules for provisioning logic (provision.ts), utility functions (utils.ts), and comprehensive test suites for both. The implementation correctly handles various scenarios, including conflict resolution for existing projects and configuration files.
My review focuses on improving documentation, robustness, and maintainability. I've suggested adding JSDoc comments to new functions, making error messages more specific for easier debugging, and replacing brittle parsing logic with more robust alternatives. Additionally, I've pointed out opportunities to improve code clarity by removing magic numbers and logging suppressed errors for better diagnostics.
| if (project?.parent) { | ||
| const parts = project.parent.split("/"); | ||
| if (parts.length === 2) { | ||
| const [type, id] = parts; | ||
| switch (type) { | ||
| case "projects": | ||
| projectOptions.parent = { type: "existing_project", projectId: id }; | ||
| break; | ||
| case "folders": | ||
| projectOptions.parent = { type: "folder", folderId: id }; | ||
| break; | ||
| case "organizations": | ||
| projectOptions.parent = { type: "organization", organizationId: id }; | ||
| break; | ||
| default: | ||
| // This should be caught by validation, but as a safeguard: | ||
| throw new Error(`Unsupported parent type: ${type}`); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for parsing the parent string relies on split('/') and assumes a specific structure. While this is validated earlier, it's more robust to use a regular expression with capture groups for both validation and parsing. This ensures the parsing logic is always in sync with the expected format and makes the code more self-contained.
if (project?.parent) {
// The validation regex can be reused here with capture groups for parsing.
const parentMatch = project.parent.match(/^(projects|folders|organizations)\/([\w-]+)$/);
if (parentMatch) {
const type = parentMatch[1];
const id = parentMatch[2];
switch (type) {
case "projects":
projectOptions.parent = { type: "existing_project", projectId: id };
break;
case "folders":
projectOptions.parent = { type: "folder", folderId: id };
break;
case "organizations":
projectOptions.parent = { type: "organization", organizationId: id };
break;
}
}
}| export function extractBundleIdFromPlist(plistPath: string): string { | ||
| try { | ||
| const fileContent = fs.readFileSync(plistPath, "utf8"); | ||
| const bundleIdMatch = /<key>BUNDLE_ID<\/key>\s*<string>(.*?)<\/string>/u.exec(fileContent); |
There was a problem hiding this comment.
Using a regular expression to parse the GoogleService-Info.plist file can be brittle. If the file's formatting changes slightly (e.g., different whitespace or newlines), the regex might fail. For more robust parsing, consider using a dedicated plist parsing library, such as plist, which would make this code more resilient to changes in the file structure.
| } catch (error) { | ||
| throw new Error(`Failed to parse iOS plist file: ${plistPath}`); | ||
| } |
There was a problem hiding this comment.
The error handling in extractBundleIdFromPlist catches any error and re-throws a generic "Failed to parse" error. This can make debugging difficult as it hides the original cause (e.g., file not found vs. BUNDLE_ID key missing). It would be more helpful to provide a more specific error message by including the original error's message.
| } catch (error) { | |
| throw new Error(`Failed to parse iOS plist file: ${plistPath}`); | |
| } | |
| } catch (error: any) { | |
| throw new Error(`Failed to parse iOS plist file: ${plistPath}. Reason: ${error.message}`); | |
| } |
| let directoryName: string = platform; | ||
| let counter = 1; | ||
|
|
||
| while (fs.existsSync(path.join(projectDirectory, directoryName)) && counter < 1000) { |
There was a problem hiding this comment.
The number 1000 is used as a magic number to limit the loop iterations. It's better to define it as a constant with a descriptive name (e.g., MAX_DIR_NAME_ATTEMPTS) at the top of the file or function. This clarifies its intent and makes it easier to modify if needed.
| } catch (error) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The catch block currently swallows any errors that occur during plist parsing. While this allows the function to continue searching other files, it hides potentially useful debugging information if a file is malformed. Consider logging the error to a debug channel to aid in troubleshooting user issues with misconfigured files.
| } catch (error) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Similar to findExistingIosApp, the catch block here swallows errors during JSON parsing. This can hide issues with malformed google-services.json files. Consider logging the error to a debug channel for better diagnostics.
Conflict Handling
.firebasercprojects unlessoverwrite_project: trueoverwrite_configs: trueTesting with Gemini CLI
Clone PR with GitHub CLI:
Install and link:
Update
~/.gemini/settings.jsonwith:{ "mcpServers": { "firebase": { "command": "npx", "args": [ "-y", "firebase-tools", "experimental:mcp", "--dir", "." ] } } }Start Gemini CLI and try a prompt like:
Limitations