Add web support for Crashlytics MCP tools and prompts#10822
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
![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 extends Firebase Crashlytics support to web applications, introducing web-specific reports (topBrowsers and webMetrics), metrics (sessionsCount), and filters (browserFilterDisplayNames). It also updates MCP prompts, guides, tools, and availability checks to distinguish between mobile and web platforms. The reviewer feedback highlights a critical issue in the web Crashlytics detection logic, which incorrectly expects "firebase/crashlytics" in package.json instead of the standard "firebase" or "@firebase/crashlytics" dependencies, and points out a few minor typos in comments and documentation.
| async function webAppUsesCrashlytics(appPath: string): Promise<boolean> { | ||
| const packageJsonFiles = await detectFiles(appPath, "package.json"); | ||
| for (const file of packageJsonFiles) { | ||
| const content = await fs.readFile(path.join(appPath, file), "utf8"); | ||
| if (content.includes("firebase/crashlytics")) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Checking for "firebase/crashlytics" directly in package.json will fail for standard Firebase Web projects. In a typical project, "firebase" is listed as a dependency in package.json, and "firebase/crashlytics" is imported in the source files (as it is a subpath export, not a standalone npm package).
To correctly detect if a web app uses Crashlytics, we should check if "firebase" or "@firebase/crashlytics" is listed as a dependency in package.json.
| async function webAppUsesCrashlytics(appPath: string): Promise<boolean> { | |
| const packageJsonFiles = await detectFiles(appPath, "package.json"); | |
| for (const file of packageJsonFiles) { | |
| const content = await fs.readFile(path.join(appPath, file), "utf8"); | |
| if (content.includes("firebase/crashlytics")) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| async function webAppUsesCrashlytics(appPath: string): Promise<boolean> { | |
| const packageJsonFiles = await detectFiles(appPath, "package.json"); | |
| const pattern = /"(firebase|@firebase\/crashlytics)"\s*:/; | |
| for (const file of packageJsonFiles) { | |
| const content = await fs.readFile(path.join(appPath, file), "utf8"); | |
| if (pattern.test(content)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } |
There was a problem hiding this comment.
Is that true? How is the js sdk bundled?
There was a problem hiding this comment.
So during the EAP, they’ll need to install and import “@firebase/crashlytics” explicitly due to how things are packaged/exposed. This will be detectable in package.json.
After we merge to main and are part of the larger SDK release, then they can just import “firebase/crashlytics” in their code as it will be bundled in the larger “firebase” dependency in package.json. At this point, there is no longer a guarantee that they have anything with "crashlytics" in their package.json.
Happy to chat this out more in person if it's still unclear.
There was a problem hiding this comment.
Updated this to check to see if firebase is installed and firebase/crashlytics is imported.
| .array(z.enum(["SIGNAL_EARLY", "SIGNAL_FRESH", "SIGNAL_REGRESSED", "SIGNAL_REPETITIVE"])) | ||
| .optional() | ||
| .describe(`Counts events matching the given signals`), | ||
| .describe(`Counts events matching the given signals. Only supported for mobile apps.`), |
There was a problem hiding this comment.
This looks like the kind of thing that an agent is really going to mess up. Have you tested this one out locally? It might perform better if you say mobile apps and then qualify it like "mobile apps (e.g. iOS and Android)"
There was a problem hiding this comment.
I think this instruction will mostly work. If the api can just discard invalid filters, this is probably fine.
| async function webAppUsesCrashlytics(appPath: string): Promise<boolean> { | ||
| const packageJsonFiles = await detectFiles(appPath, "package.json"); | ||
| for (const file of packageJsonFiles) { | ||
| const content = await fs.readFile(path.join(appPath, file), "utf8"); | ||
| if (content.includes("firebase/crashlytics")) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Is that true? How is the js sdk bundled?
| .array(z.enum(["SIGNAL_EARLY", "SIGNAL_FRESH", "SIGNAL_REGRESSED", "SIGNAL_REPETITIVE"])) | ||
| .optional() | ||
| .describe(`Counts events matching the given signals`), | ||
| .describe(`Counts events matching the given signals. Only supported for mobile apps.`), |
There was a problem hiding this comment.
I think this instruction will mostly work. If the api can just discard invalid filters, this is probably fine.
| return false; | ||
| } | ||
|
|
||
| const sourceFiles = await detectFiles(appPath, "*.{js,jsx,ts,tsx,mjs,cjs,html,vue,svelte}"); |
There was a problem hiding this comment.
This could be very costly to the MCP start up time on a big app. For the other options, we try to do something that works for most folks and then give the workaround that they can force the crashlytics tools to be there even if not detected. Do we have any conventions around where someone would import the crashlytics dep that we could use to cover that 90% case?
There was a problem hiding this comment.
Agreed-- I'm worried about the performance of this as well. @tonybaroneee is there a particular file where we expect most developers to import crashlytics?
I just remembered for the app testing agent that we just do a check to see if the app distribution API is enabled on the project to determine if we expose the app testing agent MCP tools. I wonder if is there an API or onboarding step that we can check to determine if they're onboarded for crash for web. That may be easier than trying to pinpoint a specific file that has a crashlytics import.
There was a problem hiding this comment.
Yeah, maybe let's just call a GET to the telemetry admin API config endpoint to see if they've onboarded. That's a pretty strong signal (even though they still might not have provisioned the SDK in their app yet).
5 participants
Description
Add support for web apps with Crashlytics MCP tools and prompts.
Scenarios Tested
Sample Commands