Skip to content

Commit 9505bae

Browse files
mtioclaude
andcommitted
fix: address code review feedback from coderabbitai
- Fix stale tool count in CLAUDE.md - Remove extra blank line in .gitignore - Add projectId and credential to fakeApp mock so list_databases happy path is properly asserted - Resolve projectId via getProjectId() fallback instead of casting credential object - Add 10s timeout to Firestore Admin API fetch call - Fix broken error-handling test by adding missing module mocks Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent c02995f commit 9505bae

5 files changed

Lines changed: 40 additions & 16 deletions

File tree

.gitignore

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,5 @@ firestore.indexes.json
7070
firestore.rules
7171
storage.rules
7272

73-
7473
# claude settings
7574
.claude/settings.local.json

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ Single test file: `npx vitest run path/to/file.test.ts`
2727
### Server Core (`src/index.ts`)
2828
Single `FirebaseMcpServer` class that:
2929
1. Initializes Firebase Admin SDK from `SERVICE_ACCOUNT_KEY_PATH`
30-
2. Registers 12 MCP tools via `setupToolHandlers()` (ListTools + CallTool handlers)
30+
2. Registers MCP tools via `setupToolHandlers()` (ListTools + CallTool handlers)
3131
3. Dispatches tool calls through a switch statement
3232
4. Connects to a transport (stdio or HTTP)
3333

src/__tests__/index.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2594,6 +2594,15 @@ describe('Firebase MCP Server', () => {
25942594
}));
25952595
vi.doMock('../utils/logger', () => ({ logger: loggerMock }));
25962596
vi.doMock('firebase-admin', () => adminMock);
2597+
vi.doMock('firebase-admin/firestore', () => ({
2598+
Timestamp: { fromDate: vi.fn() },
2599+
getFirestore: vi.fn().mockImplementation(() => {
2600+
throw genericError;
2601+
}),
2602+
}));
2603+
vi.doMock('../lib/firebase/firebaseConfig.js', () => ({
2604+
getProjectId: vi.fn().mockReturnValue('test-project'),
2605+
}));
25972606

25982607
await import('../index');
25992608

src/__tests__/multi-db.test.ts

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,15 @@ describe('Multi-database support', () => {
107107
* including a working Firebase app so getFirestoreInstance succeeds.
108108
*/
109109
async function setupModule(configOverrides: Record<string, unknown> = {}) {
110-
const fakeApp = { name: '[DEFAULT]' };
110+
const fakeApp = {
111+
name: '[DEFAULT]',
112+
options: {
113+
projectId: 'test-project',
114+
credential: {
115+
getAccessToken: vi.fn().mockResolvedValue({ access_token: 'test-token' }),
116+
},
117+
},
118+
};
111119

112120
const adminMock = {
113121
default: {
@@ -179,6 +187,9 @@ describe('Multi-database support', () => {
179187
vi.doMock('../transports/index.js', () => ({
180188
initializeTransport: vi.fn().mockResolvedValue(undefined),
181189
}));
190+
vi.doMock('../lib/firebase/firebaseConfig.js', () => ({
191+
getProjectId: vi.fn().mockReturnValue('test-project'),
192+
}));
182193
vi.doMock('fs', () => fsMock);
183194

184195
await import('../index');
@@ -387,15 +398,11 @@ describe('Multi-database support', () => {
387398

388399
const content = JSON.parse(result.content[0].text);
389400

390-
// In test env, app may not be fully initialized — accept either outcome
391-
if (content.error) {
392-
expect(content).toHaveProperty('error');
393-
} else {
394-
expect(content.databases).toHaveLength(2);
395-
expect(content.databases[0].id).toBe('(default)');
396-
expect(content.databases[1].id).toBe('my-other-db');
397-
expect(content.databases[0].location).toBe('us-east1');
398-
}
401+
expect(content.databases).toHaveLength(2);
402+
expect(content.databases[0].id).toBe('(default)');
403+
expect(content.databases[1].id).toBe('my-other-db');
404+
expect(content.databases[0].location).toBe('us-east1');
405+
expect(content.databases[0].type).toBe('FIRESTORE_NATIVE');
399406
});
400407

401408
it('should handle API errors gracefully', async () => {

src/index.ts

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import { logger } from './utils/logger.js';
2222
import { Timestamp, getFirestore } from 'firebase-admin/firestore';
2323
import config from './config.js';
2424
import { initializeTransport } from './transports/index.js';
25+
import { getProjectId } from './lib/firebase/firebaseConfig.js';
2526
import * as fs from 'fs';
2627

2728
// Initialize Firebase
@@ -1585,7 +1586,7 @@ class FirebaseMcpServer {
15851586
const tokenResult = await credential.getAccessToken();
15861587
const projectId =
15871588
app.options.projectId ||
1588-
(app.options.credential as unknown as { projectId?: string }).projectId;
1589+
(config.serviceAccountKeyPath ? getProjectId(config.serviceAccountKeyPath) : null);
15891590

15901591
if (!projectId) {
15911592
return {
@@ -1599,9 +1600,17 @@ class FirebaseMcpServer {
15991600
}
16001601

16011602
const url = `https://firestore.googleapis.com/v1/projects/${projectId}/databases`;
1602-
const response = await fetch(url, {
1603-
headers: { Authorization: `Bearer ${tokenResult.access_token}` },
1604-
});
1603+
const controller = new AbortController();
1604+
const timeoutId = setTimeout(() => controller.abort(), 10000);
1605+
let response: Response;
1606+
try {
1607+
response = await fetch(url, {
1608+
headers: { Authorization: `Bearer ${tokenResult.access_token}` },
1609+
signal: controller.signal,
1610+
});
1611+
} finally {
1612+
clearTimeout(timeoutId);
1613+
}
16051614

16061615
if (!response.ok) {
16071616
const errorBody = await response.text();

0 commit comments

Comments
 (0)