-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[mcp] fix issue where crashlytics isn't detected for ios apps that use project.pbxproj #9515
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
Summary of ChangesHello @aalej, 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 resolves an issue where the Mobile Client Platform (MCP) server failed to detect Crashlytics integration in iOS applications that added Firebase packages through the Xcode user interface. The previous detection mechanism did not account for package references stored within 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
|
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 fixes an issue where Crashlytics was not detected in iOS projects that use project.pbxproj for package management. The change adds logic to scan project.pbxproj files for Crashlytics dependencies, and includes a new test case to verify this. The change is correct, but I've identified an opportunity to refactor the iosAppUsesCrashlytics function to reduce code duplication and improve performance. My feedback includes a suggestion for this refactoring.
| const xcodeProjectFiles = await detectFiles(appPath, "project.pbxproj"); | ||
| for (const file of xcodeProjectFiles) { | ||
| const content = await fs.readFile(path.join(appPath, file), "utf8"); | ||
| if (content.includes("Crashlytics")) { | ||
| return true; | ||
| } | ||
| } |
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.
This block of code is nearly identical to the three preceding blocks that check for other file types. This code repetition makes the function less maintainable and can be inefficient as it checks for each file type sequentially.
To improve this, you could refactor the logic to avoid code duplication and improve performance by reading files in parallel. This would make iosAppUsesCrashlytics more concise, performant, and easier to extend.
For example, you could refactor the function to something like this:
async function iosAppUsesCrashlytics(appPath: string): Promise<boolean> {
const filePatterns = ["Podfile", "Package.swift", "Cartfile*", "project.pbxproj"];
for (const pattern of filePatterns) {
const files = await detectFiles(appPath, pattern);
if (files.length === 0) {
continue;
}
const readPromises = files.map(async (file) => {
const content = await fs.readFile(path.join(appPath, file), "utf8");
return content.includes("Crashlytics");
});
const results = await Promise.all(readPromises);
if (results.some((found) => found)) {
return true;
}
}
return false;
}While this refactoring is larger than the current change, it would significantly improve the quality of this function.
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.
I don't love Gemini's approach, but we could concat all of the detected files into a single list and loop over that single list. Not a blocking comment, but might be a nice clean up.
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.
Thanks for the feedback! Going to go ahead and merge this for now, and will work on the clean up in another PR. Just to verify the process, do we want to first detect each file then read through each one? so it would be something like
Detect Podfile
Detect Package.swift
Detect Cartfile*
Detect project.pbxproj
Read Podfile // If using pods, will return after getting all 4 files
Read Package.swift
Read Cartfile*
Read project.pbxproj
I think it would be better if we do something like the current implementation since we can return early once we read the file has Crashlytics
Detect Podfile
Read Podfile // If using pods, returns after getting Podfile
Detect Package.swift
Read Package.swift
Detect Cartfile*
Read Cartfile*
Detect project.pbxproj
Read project.pbxproj
Let me know if I misunderstood anything here.
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.
I was suggesting the first one --
Detect Podfile
Detect Package.swift
Detect Cartfile*
Detect project.pbxproj
Read Podfile // If using pods, will exit here
Read Package.swift
Read Cartfile*
Read project.pbxproj
The file detection is just looking matching file names. It is slightly less efficient in that we are going to try to detect all the different file patterns, but we still have the opportunity to exit early if we find what we are looking for. It would make the code a lot cleaner because it would be something like --
const fileArrays = await Promises.all(
detectFiles(appPath, "Podfile"),
detectFiles(appPath, "Package.swift"),
detectFiles(appPath, "Cartfile*"),
detectFiles(appPath, "project.pbxproj"));
const files = fileArrays.flat();
if (files.length === 0) {
return false;
}
for (const file of files) {
const content = await fs.readFile(path.join(appPath, file), "utf8");
if (content.includes("Crashlytics")) {
return true;
}
}
return false;
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.
Thanks, this definitely does look cleaner. I will work on this!
schnecle
left a comment
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.
Thanks for responding to this one! LGTM!
| const xcodeProjectFiles = await detectFiles(appPath, "project.pbxproj"); | ||
| for (const file of xcodeProjectFiles) { | ||
| const content = await fs.readFile(path.join(appPath, file), "utf8"); | ||
| if (content.includes("Crashlytics")) { | ||
| return true; | ||
| } | ||
| } |
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.
I don't love Gemini's approach, but we could concat all of the detected files into a single list and loop over that single list. Not a blocking comment, but might be a nice clean up.
Description
It seems like when adding a package via the Xcode UI https://firebase.google.com/docs/ios/installation-methods#via-xcode, the packages are listed in a different file,
project.pbxproj.potential fix for #9495 if they are using the Xcode UI to add packages
Scenarios Tested
Ran the ff script on an iOS project that has crashlytics
Sample Commands