chore: add qol script for sync #655
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughA new synchronization script is introduced that retrieves user and group documents from Firestore collections, processes them sequentially through an adapter with error handling and progress tracking, and outputs final statistics. A corresponding npm script entry is added to enable execution. Changes
Sequence DiagramsequenceDiagram
participant Script as syncUsersAndGroups()
participant Firebase as Firebase Admin
participant Firestore as Firestore
participant Adapter as Adapter
participant Logger as Logging/Output
Script->>Firebase: Initialize Admin SDK<br/>(credentials fallback)
activate Firebase
Firebase-->>Script: Ready
deactivate Firebase
Script->>Firestore: Get users collection
activate Firestore
Firestore-->>Script: User documents
deactivate Firestore
rect rgb(200, 220, 240)
Note over Script,Adapter: Process Users
loop Each user
Script->>Script: Enrich with ID
Script->>Adapter: handleChange(user, tableName="user")
alt Success
Adapter-->>Script: ✓
Script->>Logger: Log progress
else Error
Script->>Logger: Log error, track failure
end
Script->>Script: Delay throttle
end
end
Script->>Firestore: Get groups collection<br/>(tolerant to failure)
activate Firestore
Firestore-->>Script: Group documents<br/>or empty on error
deactivate Firestore
rect rgb(220, 240, 200)
Note over Script,Adapter: Process Groups
loop Each group
Script->>Script: Enrich with ID
Script->>Adapter: handleChange(group, tableName="group")
alt Success
Adapter-->>Script: ✓
Script->>Logger: Log progress
else Error
Script->>Logger: Log error, track failure
end
Script->>Script: Delay throttle
end
end
Script->>Logger: Output final statistics<br/>(totals, successes, failures)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
coodos
force-pushed
the
feat/qol-sync-script
branch
from
a5bd8e6 to
b2b8c29
Compare
coodos
force-pushed
the
feat/qol-sync-script
branch
from
b2b8c29 to
ef8cf01
Compare
xPathin
approved these changes
Jan 6, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI Agents
In @platforms/blabsy-w3ds-auth-api/src/scripts/syncUsersAndGroups.ts:
- Around line 86-92: Remove the redundant per-call .catch() handlers that log
and re-throw around adapter.handleChange calls (e.g., the call to
adapter.handleChange({ data: enrichedData, tableName: "user" }) and the
analogous call in processGroup); the outer try-catch in the processing loops
already logs errors, so delete the .catch(...) wrappers and let exceptions
propagate to the existing try-catch so each error is logged only once.
- Line 218: The success-rate log can produce NaN when totalItems is 0; modify
the code around the console.log that uses processedCount and totalItems so it
guards against division-by-zero (e.g., if totalItems === 0, print a sensible
fallback like "0.0%" or "N/A" instead of doing (processedCount/totalItems));
update the expression that builds the string for the console.log to compute
percentage only when totalItems > 0 and otherwise output the fallback.
- Line 9: The dotenv.config call in syncUsersAndGroups.ts uses a brittle
hardcoded relative path ("../../../../../.env"); change it to load .env more
robustly by allowing an override and a sensible fallback: check for a
DOTENV_PATH env var and, if present, call dotenv.config({ path:
path.resolve(process.cwd(), process.env.DOTENV_PATH) }), otherwise call
dotenv.config({ path: path.resolve(process.cwd(), '.env') }) and if that fails
fall back to dotenv.config() with no args so dotenv can auto-discover; update
the dotenv.config invocation (and related imports) in syncUsersAndGroups.ts
accordingly so the script works regardless of its location.
- Line 154: Remove the unused local variable `remaining` (the line `const
remaining = users.length - userNumber;`) from the syncUsersAndGroups.ts code;
locate the declaration (variable name `remaining`) in the function that iterates
`users` and delete it, or if its value was intended to be used, replace the
deletion by actually using `remaining` in the relevant log/message instead.
🧹 Nitpick comments (3)
platforms/blabsy-w3ds-auth-api/src/scripts/syncUsersAndGroups.ts (3)
12-68: Consider simplifying the nested credential fallback logic.The multiple nested try-catch blocks make the initialization flow difficult to follow. Consider refactoring into a cleaner chain of credential strategies.
💡 Suggested refactor for improved readability
function initializeFirebase(): void { if (getApps().length > 0) { console.log("✅ Firebase Admin SDK already initialized"); return; } const credentialsPath = process.env.GOOGLE_APPLICATION_CREDENTIALS || process.env.FIREBASE_CREDENTIALS_PATH; // Strategy 1: Try loading from file if (credentialsPath && fs.existsSync(credentialsPath)) { try { const serviceAccount = JSON.parse(fs.readFileSync(credentialsPath, "utf8")); initializeApp({ credential: cert(serviceAccount) }); console.log("✅ Firebase Admin SDK initialized with service account file"); return; } catch (error: unknown) { console.error("❌ Failed to load service account file:", error); } } // Strategy 2: Try application default if (credentialsPath || process.env.GOOGLE_APPLICATION_CREDENTIALS) { try { initializeApp({ credential: applicationDefault() }); console.log("✅ Firebase Admin SDK initialized with applicationDefault"); return; } catch (error: unknown) { console.error("❌ Failed to initialize with applicationDefault:", error); } } // Strategy 3: Fail with clear message throw new Error("Firebase credentials not configured. Set GOOGLE_APPLICATION_CREDENTIALS or FIREBASE_CREDENTIALS_PATH"); }
82-109: Consolidate duplicate user and group processing logic.
processUserandprocessGroupare nearly identical and differ only in thetableNameparameter. This violates DRY principles.🔎 Proposed refactor to eliminate duplication
-async function processUser(doc: FirebaseFirestore.QueryDocumentSnapshot<DocumentData>): Promise<void> { - const data = doc.data(); - const enrichedData: DocumentData & { id: string } = { ...data, id: doc.id }; - - await adapter.handleChange({ - data: enrichedData, - tableName: "user", - }).catch((e) => { - console.error(`Error processing user ${doc.id}:`, e); - throw e; - }); -} - -async function processGroup(doc: FirebaseFirestore.QueryDocumentSnapshot<DocumentData>): Promise<void> { - const data = doc.data(); - const enrichedData: DocumentData & { id: string } = { ...data, id: doc.id }; - - await adapter.handleChange({ - data: enrichedData, - tableName: "group", - }).catch((e) => { - console.error(`Error processing group ${doc.id}:`, e); - throw e; - }); -} +async function processDocument( + doc: FirebaseFirestore.QueryDocumentSnapshot<DocumentData>, + tableName: "user" | "group" +): Promise<void> { + const data = doc.data(); + const enrichedData: DocumentData & { id: string } = { ...data, id: doc.id }; + + await adapter.handleChange({ + data: enrichedData, + tableName, + }); +}Then update the calls:
await processDocument(user, "user"); await processDocument(group, "group");
114-220: Consider performance and operational improvements for production use.The current implementation has several operational limitations:
- Sequential processing with fixed delays takes considerable time for large datasets (e.g., 1,000 items × 3.33s = ~55 minutes)
- Memory usage: Loading all documents at once could cause issues with large collections
- No resume capability: If the script fails midway, it must restart from the beginning
- Hardcoded delay: The 3.33s delay isn't configurable
Consider these enhancements for production resilience:
- Implement batch processing with controlled parallelism (e.g., process 5-10 items concurrently)
- Use Firestore query cursors for pagination instead of loading everything into memory
- Add checkpointing to track progress and enable resuming from the last successful item
- Make the delay configurable via environment variable
- Add a dry-run mode to validate without making changes
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
platforms/blabsy-w3ds-auth-api/package.jsonplatforms/blabsy-w3ds-auth-api/src/scripts/syncUsersAndGroups.ts
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (2)
platforms/blabsy-w3ds-auth-api/package.json (1)
15-15: LGTM! Script entry properly configured.The new npm script follows existing patterns and correctly references the TypeScript file.
platforms/blabsy-w3ds-auth-api/src/scripts/syncUsersAndGroups.ts (1)
223-235: LGTM! Script execution and export properly implemented.The script guard correctly detects direct execution, handles both success and error cases with appropriate exit codes, and the function is properly exported for programmatic use.
| import dotenv from "dotenv"; | ||
| import { adapter } from "../controllers/WebhookController"; | ||
|
|
||
| dotenv.config({ path: path.resolve(__dirname, "../../../../../.env") }); |
Contributor
There was a problem hiding this comment.
Fragile path traversal for environment configuration.
The hardcoded relative path "../../../../../.env" is brittle and will break if the script moves or the directory structure changes. If the .env file isn't found, Firebase initialization will fail.
🔎 Recommended approach
Consider these alternatives:
- Use
path.resolve(process.cwd(), '.env')to load from the repository root - Allow the path to be overridden via an environment variable
- Let dotenv search for .env files automatically by calling
dotenv.config()without arguments
-dotenv.config({ path: path.resolve(__dirname, "../../../../../.env") });
+// Load from repository root or allow DOTENV_CONFIG_PATH override
+dotenv.config({ path: process.env.DOTENV_CONFIG_PATH || path.resolve(process.cwd(), '.env') });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| dotenv.config({ path: path.resolve(__dirname, "../../../../../.env") }); | |
| // Load from repository root or allow DOTENV_CONFIG_PATH override | |
| dotenv.config({ path: process.env.DOTENV_CONFIG_PATH || path.resolve(process.cwd(), '.env') }); |
🤖 Prompt for AI Agents
In @platforms/blabsy-w3ds-auth-api/src/scripts/syncUsersAndGroups.ts at line 9,
The dotenv.config call in syncUsersAndGroups.ts uses a brittle hardcoded
relative path ("../../../../../.env"); change it to load .env more robustly by
allowing an override and a sensible fallback: check for a DOTENV_PATH env var
and, if present, call dotenv.config({ path: path.resolve(process.cwd(),
process.env.DOTENV_PATH) }), otherwise call dotenv.config({ path:
path.resolve(process.cwd(), '.env') }) and if that fails fall back to
dotenv.config() with no args so dotenv can auto-discover; update the
dotenv.config invocation (and related imports) in syncUsersAndGroups.ts
accordingly so the script works regardless of its location.
Comment on lines
+86
to
+92
| await adapter.handleChange({ | ||
| data: enrichedData, | ||
| tableName: "user", | ||
| }).catch((e) => { | ||
| console.error(`Error processing user ${doc.id}:`, e); | ||
| throw e; | ||
| }); |
Contributor
There was a problem hiding this comment.
Remove redundant error logging in catch blocks.
The .catch() blocks log errors and re-throw them, but the outer try-catch in the processing loops (lines 161-164, 195-198) also logs errors. This creates duplicate error messages in the console output.
🔎 Proposed fix
Remove the .catch() blocks since the outer try-catch already handles logging:
async function processUser(doc: FirebaseFirestore.QueryDocumentSnapshot<DocumentData>): Promise<void> {
const data = doc.data();
const enrichedData: DocumentData & { id: string } = { ...data, id: doc.id };
- await adapter.handleChange({
+ return adapter.handleChange({
data: enrichedData,
tableName: "user",
- }).catch((e) => {
- console.error(`Error processing user ${doc.id}:`, e);
- throw e;
});
}Apply the same change to processGroup.
Also applies to: 102-108
🤖 Prompt for AI Agents
In @platforms/blabsy-w3ds-auth-api/src/scripts/syncUsersAndGroups.ts around
lines 86 - 92, Remove the redundant per-call .catch() handlers that log and
re-throw around adapter.handleChange calls (e.g., the call to
adapter.handleChange({ data: enrichedData, tableName: "user" }) and the
analogous call in processGroup); the outer try-catch in the processing loops
already logs errors, so delete the .catch(...) wrappers and let exceptions
propagate to the existing try-catch so each error is logged only once.
| for (let i = 0; i < users.length; i++) { | ||
| const user = users[i]; | ||
| const userNumber = i + 1; | ||
| const remaining = users.length - userNumber; |
Contributor
There was a problem hiding this comment.
Remove unused variable.
The remaining variable is declared but never used in the code.
🔎 Proposed fix
for (let i = 0; i < users.length; i++) {
const user = users[i];
const userNumber = i + 1;
- const remaining = users.length - userNumber;
try {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const remaining = users.length - userNumber; | |
| for (let i = 0; i < users.length; i++) { | |
| const user = users[i]; | |
| const userNumber = i + 1; | |
| try { |
🤖 Prompt for AI Agents
In @platforms/blabsy-w3ds-auth-api/src/scripts/syncUsersAndGroups.ts at line
154, Remove the unused local variable `remaining` (the line `const remaining =
users.length - userNumber;`) from the syncUsersAndGroups.ts code; locate the
declaration (variable name `remaining`) in the function that iterates `users`
and delete it, or if its value was intended to be used, replace the deletion by
actually using `remaining` in the relevant log/message instead.
| console.log(` Total items: ${totalItems}`); | ||
| console.log(` ✅ Successfully processed: ${processedCount}`); | ||
| console.log(` ❌ Failed: ${failedCount}`); | ||
| console.log(` 📈 Success rate: ${((processedCount / totalItems) * 100).toFixed(1)}%`); |
Contributor
There was a problem hiding this comment.
Handle division by zero for empty collections.
If both users and groups collections are empty, totalItems will be 0, causing the success rate calculation to produce NaN.
🔎 Proposed fix
- console.log(` 📈 Success rate: ${((processedCount / totalItems) * 100).toFixed(1)}%`);
+ console.log(` 📈 Success rate: ${totalItems > 0 ? ((processedCount / totalItems) * 100).toFixed(1) : '0.0'}%`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log(` 📈 Success rate: ${((processedCount / totalItems) * 100).toFixed(1)}%`); | |
| console.log(` 📈 Success rate: ${totalItems > 0 ? ((processedCount / totalItems) * 100).toFixed(1) : '0.0'}%`); |
🤖 Prompt for AI Agents
In @platforms/blabsy-w3ds-auth-api/src/scripts/syncUsersAndGroups.ts at line
218, The success-rate log can produce NaN when totalItems is 0; modify the code
around the console.log that uses processedCount and totalItems so it guards
against division-by-zero (e.g., if totalItems === 0, print a sensible fallback
like "0.0%" or "N/A" instead of doing (processedCount/totalItems)); update the
expression that builds the string for the console.log to compute percentage only
when totalItems > 0 and otherwise output the fallback.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of change
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.