Repository navigation
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the Firebase MCP server to asynchronously detect and cache the availability of the local firebase CLI, resolving an issue where it always suggested the npx fallback. It also defers project setup detection from the constructor to the start method and adds corresponding unit tests. Feedback on these changes suggests adding a timeout to the spawned CLI version check to prevent potential hangs, extracting magic strings and numbers into static constants as per the style guide, and catching errors on the unawaited detectProjectSetup call to avoid unhandled promise rejections.
| } | ||
|
|
||
| async start(options?: { useSSE?: boolean; port?: number }): Promise<void> { | ||
| void this.detectProjectSetup(); |
There was a problem hiding this comment.
Calling void this.detectProjectSetup() without awaiting or catching errors can lead to unhandled promise rejections if any of the underlying async operations (such as file system access or API calls) fail. This can crash the Node.js process or lead to silent failures. Let's catch and log any errors using this.logger.error.
void this.detectProjectSetup().catch((err) => {
this.logger.error(`Failed to detect project setup: ${err instanceof Error ? err.message : String(err)}`);
});There was a problem hiding this comment.
Pre-existing behavior unrelated to my test fix. Though maybe worth someone who works on MCP addressing.
### Description
`FirebaseMcpServer` previously ran `void this.detectProjectSetup()` in its constructor. In unit tests that instantiate `FirebaseMcpServer` without calling `start()`, the 2-second fallback timers all fired mid-suite and ran repo-wide `glob` scans against `process.cwd()`, blocking the event loop and causing flaky timeouts in downstream suites such as `throttler.spec.ts`.
This moves `detectProjectSetup()` from the constructor to `start()` so constructing a server in tests starts no background timers or filesystem scans. Availability specs also reuse a single server instance per suite.
### Scenarios Tested
- Added unit tests verifying `new FirebaseMcpServer({})` does not run project or feature detection until `start()` is called.
- Ran the 23-server reproduction spec and the full Mocha suite under `--trace-gc`, confirming 0 allocation storms and all unit tests passing.
### Sample Commands
```bash
firebase mcp
```
359743b to
332c571
Compare
There was a problem hiding this comment.
firebase-tools/src/mcp/index.ts
Line 333 in 332c571
Note on line 333: crossSpawn.sync("firebase --version") passes --version inside the executable name instead of the args array, so it always fails with ENOENT and falls back to npx firebase-tools@latest. We left this out of this PR to keep it focused on the flaky test fix. (When fixed separately, it should also run asynchronously and be cached once per process so it does not add a synchronous CLI startup.)
Description
FirebaseMcpServerpreviously ranvoid this.detectProjectSetup()in its constructor. In unit tests that instantiateFirebaseMcpServerwithout callingstart(), the 2-second fallback timers all fired mid-suite and ran repo-wideglobscans againstprocess.cwd(), blocking the event loop and causing flaky timeouts in downstream suites such asthrottler.spec.ts.This moves
detectProjectSetup()from the constructor tostart()so constructing a server in tests starts no background timers or filesystem scans. Availability specs also reuse a single server instance per suite.Scenarios Tested
new FirebaseMcpServer({})does not run project or feature detection untilstart()is called.--trace-gc, confirming 0 allocation storms and all unit tests passing.Sample Commands