chore: make blabsy admin auth more robust #421
Conversation
WalkthroughThe PR refactors Firebase Admin SDK initialization in the auth API to detect existing initialization via Changes
Sequence DiagramsequenceDiagram
participant App as Application Start
participant Init as Initialization Logic
participant Firebase as Firebase Admin SDK
participant FileSystem as File System
participant AppDefault as Application Default
App->>Init: Start initialization
Init->>Firebase: Check getApps()
alt Already Initialized
Firebase-->>Init: Apps exist
Init->>Init: Set firebaseInitialized = true
else Not Initialized
Firebase-->>Init: No apps
Init->>FileSystem: Load credentials from path
alt File Exists & Valid
FileSystem-->>Init: Credentials parsed
Init->>Firebase: Initialize with credentials
Firebase-->>Init: ✓ Initialized
Init->>Init: Set firebaseInitialized = true
else File Missing or Parse Error
Init->>AppDefault: Fall back to applicationDefault()
alt Success
AppDefault-->>Init: Credentials available
Init->>Firebase: Initialize
Firebase-->>Init: ✓ Initialized
Init->>Init: Set firebaseInitialized = true
else Failure
AppDefault-->>Init: ✗ Error
Init->>Init: Log error & set firebaseInitialized = false
Init->>Init: Log warning: Firebase disabled
end
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
platforms/blabsy-w3ds-auth-api/src/index.ts (3)
41-41: Redundant truthiness check.Since we're inside the
else ifblock from Line 37 wherecredentialsPathis guaranteed to be truthy, the first part of this condition is redundant.Apply this diff to simplify:
- if (credentialsPath && fs.existsSync(credentialsPath)) { + if (fs.existsSync(credentialsPath)) {
42-48: Consider async file I/O for non-blocking startup.Using
fs.readFileSyncblocks the event loop during startup. While acceptable for initialization code, using async alternatives (fs.promises.readFile) would align with Node.js best practices for non-blocking I/O.
37-77: Consider extracting initialization logic into a testable function.The nested branching and try-catch blocks are functional but complex. Extracting this into a separate async function (e.g.,
initializeFirebase()) would improve testability, readability, and maintainability.Example structure:
async function initializeFirebase(): Promise<boolean> { // Check existing initialization if (getApps().length > 0) { console.log("✅ Firebase Admin SDK already initialized"); return true; } // Attempt initialization with credentials const credentialsPath = process.env.GOOGLE_APPLICATION_CREDENTIALS || process.env.FIREBASE_CREDENTIALS_PATH; if (credentialsPath) { return await initializeWithCredentials(credentialsPath); } console.warn("⚠️ Firebase credentials not configured..."); return false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
infrastructure/eid-wallet/src-tauri/gen/android/.idea/kotlinc.xmlis excluded by!**/gen/**
📒 Files selected for processing (1)
platforms/blabsy-w3ds-auth-api/src/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: lint
🔇 Additional comments (5)
platforms/blabsy-w3ds-auth-api/src/index.ts (5)
7-8: LGTM! Appropriate imports for robust initialization.The addition of
getAppsandfssupports the enhanced initialization logic with proper checks for existing Firebase instances and file-based credential loading.
31-31: LGTM! Explicit state tracking improves clarity.The
firebaseInitializedflag provides clear state management for Firebase initialization status, which is useful for the final guard check and potential future conditional logic.
34-36: LGTM! Proper check prevents re-initialization.Using
getApps().length > 0is the standard pattern to detect existing Firebase Admin SDK initialization and avoid re-initialization errors.
78-86: LGTM! Comprehensive error handling and graceful degradation.The warning messages clearly communicate when Firebase features are disabled, and the error logging includes helpful context (message and stack trace) for debugging initialization failures.
32-90: Request testing details for authentication infrastructure changes.The PR description lacks information about how these Firebase initialization changes were tested. Given that this affects authentication infrastructure and webhook/watcher features, please provide details on:
- How the different initialization paths (existing app, file credentials, applicationDefault, no credentials) were tested
- How webhook and watcher features behave when Firebase is not initialized
- Whether error scenarios (invalid JSON, missing file, invalid credentials) were validated
| if (!firebaseInitialized) { | ||
| console.warn("⚠️ Firebase Admin SDK not initialized. Webhook and watcher features may fail."); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify impact on webhook and watcher features when Firebase is not initialized.
The warning correctly identifies potential failures in webhook and watcher features. Ensure that these features handle the uninitialized state gracefully without crashing or exposing error details to clients.
Run the following script to check how webhook and watcher features handle Firebase dependencies:
🏁 Script executed:
#!/bin/bash
# Description: Check Firebase usage in webhook and watcher features
# Search for Firebase Admin usage in WebhookController and Web3Adapter
echo "=== Checking WebhookController for Firebase usage ==="
rg -nP --type=ts -C5 'firebase|getAuth|getFirestore' platforms/blabsy-w3ds-auth-api/src/controllers/WebhookController.ts
echo -e "\n=== Checking Web3Adapter for Firebase usage ==="
rg -nP --type=ts -C5 'firebase|getAuth|getFirestore' platforms/blabsy-w3ds-auth-api/src/web3adapter.ts
echo -e "\n=== Checking for Firebase initialization guards ==="
rg -nP --type=ts 'firebaseInitialized|getApps\(\)' platforms/blabsy-w3ds-auth-api/src/Length of output: 1578
🏁 Script executed:
#!/bin/bash
# Check how WebhookController is instantiated and error handling in index.ts
echo "=== Full context around WebhookController instantiation ==="
rg -nP --type=ts -C10 'new WebhookController|WebhookController\(' platforms/blabsy-w3ds-auth-api/src/index.ts
echo -e "\n=== Check for try-catch around initialization ==="
rg -nP --type=ts -C8 'try|catch' platforms/blabsy-w3ds-auth-api/src/index.ts | head -50
echo -e "\n=== Find watcher feature implementations ==="
rg -nP --type=ts 'watcher|watch' platforms/blabsy-w3ds-auth-api/src/ -i
echo -e "\n=== View full index.ts around warning message (lines 85-95) ==="
sed -n '85,95p' platforms/blabsy-w3ds-auth-api/src/index.tsLength of output: 10982
🏁 Script executed:
#!/bin/bash
# Check WebhookController and FirestoreWatcher Firebase handling
echo "=== WebhookController full constructor ==="
sed -n '80,88p' platforms/blabsy-w3ds-auth-api/src/controllers/WebhookController.ts
echo -e "\n=== FirestoreWatcher getFirestore usage ==="
rg -nP --type=ts -C5 'getFirestore' platforms/blabsy-w3ds-auth-api/src/web3adapter/watchers/firestoreWatcher.ts
echo -e "\n=== Check if getFirestore throws synchronously ==="
rg -nP --type=ts -C3 'getFirestore|new FirestoreWatcher' platforms/blabsy-w3ds-auth-api/src/web3adapter/index.ts | head -30Length of output: 2372
Add error handling for Firebase dependencies in WebhookController and Web3Adapter instantiation.
The warning at lines 88-90 is correct but incomplete. Both WebhookController (line 102) and Web3Adapter (line 93) will crash during instantiation if Firebase is not initialized, because their constructors call getFirestore() without guards:
WebhookControllerconstructor (line 84):this.db = getFirestore()Web3Adapterclass property (line 17):private readonly db = getFirestore()FirestoreWatcherconstructor (line 49):this.db = getFirestore()
These instantiations lack try-catch wrappers. The app will crash with an unhandled error instead of gracefully degrading. Either wrap instantiations in try-catch blocks or add guards to prevent getFirestore() calls when Firebase is uninitialized.
🤖 Prompt for AI Agents
platforms/blabsy-w3ds-auth-api/src/index.ts lines 88-90: the current warning
about Firebase not being initialized is insufficient because Web3Adapter
(instantiated around line 93), WebhookController (around line 102) and
FirestoreWatcher (constructor at line 49) call getFirestore() during
construction and will throw if Firebase isn't initialized; modify the file to
guard or wrap those instantiations in try-catch blocks (or check
firebaseInitialized before creating those objects) and provide safe fallbacks
(null/no-op implementations) and clear error logs so the app degrades gracefully
instead of crashing.
Description of change
fixes and makes the logic of running firebase more robust
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
Bug Fixes
Refactor