🧹 Implement Firebase User Deletion in User Routes#32
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughAdds Firebase Admin deletion to the user delete-confirm flow, several TypeScript typing and lint directive changes across UI components, minor test adjustments, and updates TypeScript project configs and package typecheck script. Changes
Sequence DiagramsequenceDiagram
actor User
participant Backend as Backend Server
participant Database as Database
participant Firebase as Firebase Admin SDK
User->>Backend: POST /delete/confirm (userId)
activate Backend
Backend->>Database: delete user record
activate Database
Database-->>Backend: deletion success
deactivate Database
Backend->>Firebase: admin.auth().deleteUser(uid)
activate Firebase
Firebase-->>Backend: success/error
deactivate Firebase
Note over Backend: On Firebase error, log warning<br/>and still return success for DB deletion
Backend-->>User: 200 OK (deletion confirmed)
deactivate Backend
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR implements Firebase Authentication user deletion when a user account is deleted from MongoDB, addressing the TODO comment in the user deletion endpoint. This ensures that users are properly removed from both the database and Firebase Auth, preventing orphaned authentication accounts.
Changes:
- Added Firebase Admin SDK integration to the user deletion flow
- Implemented Firebase user deletion with error handling that doesn't block the response if Firebase deletion fails
- Added logging for successful and failed Firebase deletion attempts
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| console.log(`Successfully deleted user ${uid} from Firebase Auth`); | ||
| } catch (firebaseError) { | ||
| console.warn(`Failed to delete user ${uid} from Firebase Auth:`, firebaseError.message); |
There was a problem hiding this comment.
Logging user IDs in plain text may expose sensitive information in production logs. Consider using a sanitized identifier or removing this log statement in production environments.
| console.log(`Successfully deleted user ${uid} from Firebase Auth`); | |
| } catch (firebaseError) { | |
| console.warn(`Failed to delete user ${uid} from Firebase Auth:`, firebaseError.message); | |
| console.log('Successfully deleted user from Firebase Auth'); | |
| } catch (firebaseError) { | |
| console.warn('Failed to delete user from Firebase Auth:', firebaseError.message); |
| console.warn(`Failed to delete user ${uid} from Firebase Auth:`, firebaseError.message); | ||
| // Proceed without error as DB deletion was successful |
There was a problem hiding this comment.
Silent failures of Firebase deletion could lead to orphaned accounts accumulating over time. Consider implementing retry logic or logging this to a monitoring service for tracking failed deletions that may need manual cleanup.
![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
🤖 Fix all issues with AI agents
In `@backend/routes/userRoutes.js`:
- Around line 462-470: Current flow deletes the MongoDB user first then calls
admin.auth().deleteUser(uid), which can leave an orphaned Firebase account that
later recreates the MongoDB document via /sync; reorder operations so you call
admin.auth().deleteUser(uid) before performing the MongoDB deletion (e.g., move
the admin.auth().deleteUser(uid) try/catch to run prior to the DB delete) and if
Firebase delete fails abort with an error response; alternatively implement a
retry/background job (dead-letter queue) around admin.auth().deleteUser(uid) to
ensure eventual success — update the code paths that reference
admin.auth().deleteUser, the DB delete function (the Mongo delete routine), and
any error handling to reflect the new ordering and retries.
🧹 Nitpick comments (2)
src/lib/utils.test.ts (1)
39-41:Boolean(true)/Boolean(false)is unnecessarily verbose.If the goal is to avoid a lint rule flagging literal booleans in short-circuit expressions, consider assigning the result of an actual condition or simply using
const isBtnActive = true as booleanto widen the type.Boolean(true)works but reads oddly.backend/routes/userRoutes.js (1)
466-468: Logging theuidat info/warn level is fine, but consider not loggingfirebaseError.messageverbatim.
firebaseError.messagefrom Firebase Admin SDK can sometimes contain internal details. Usingconsole.warnis acceptable for server logs, but ensure your logging pipeline doesn't expose these to end-users or external monitoring dashboards that lack access controls.
|
|
||
| // Trigger Firebase Authentication deletion | ||
| try { | ||
| await admin.auth().deleteUser(uid); | ||
| console.log(`Successfully deleted user ${uid} from Firebase Auth`); | ||
| } catch (firebaseError) { | ||
| console.warn(`Failed to delete user ${uid} from Firebase Auth:`, firebaseError.message); | ||
| // Proceed without error as DB deletion was successful | ||
| } |
There was a problem hiding this comment.
Orphaned Firebase account can re-create the MongoDB user via /sync.
If admin.auth().deleteUser(uid) fails (line 465), the user receives a 200 success response but their Firebase Auth account survives. On next login, the /sync endpoint would re-create the MongoDB user document, effectively un-deleting the account.
Consider either:
- Attempting the Firebase deletion before the MongoDB deletion (so you can abort cleanly on failure), or
- Retrying the Firebase deletion (e.g., via a background job or dead-letter queue) to ensure eventual consistency.
Option 1 is simpler and still safe — if Firebase deletion succeeds but the subsequent MongoDB deletion fails, the user simply can't log in anymore (which is the desired end-state anyway).
💡 Suggested reorder: delete Firebase first
- await User.findOneAndDelete({ uid });
-
- // Trigger Firebase Authentication deletion
- try {
- await admin.auth().deleteUser(uid);
- console.log(`Successfully deleted user ${uid} from Firebase Auth`);
- } catch (firebaseError) {
- console.warn(`Failed to delete user ${uid} from Firebase Auth:`, firebaseError.message);
- // Proceed without error as DB deletion was successful
- }
+ // Delete Firebase Auth user first to prevent orphan re-creation via /sync
+ try {
+ await admin.auth().deleteUser(uid);
+ } catch (firebaseError) {
+ console.error(`Failed to delete user ${uid} from Firebase Auth:`, firebaseError.message);
+ return res.status(500).json({ message: 'Failed to delete account. Please try again.' });
+ }
+
+ await User.findOneAndDelete({ uid });🤖 Prompt for AI Agents
In `@backend/routes/userRoutes.js` around lines 462 - 470, Current flow deletes
the MongoDB user first then calls admin.auth().deleteUser(uid), which can leave
an orphaned Firebase account that later recreates the MongoDB document via
/sync; reorder operations so you call admin.auth().deleteUser(uid) before
performing the MongoDB deletion (e.g., move the admin.auth().deleteUser(uid)
try/catch to run prior to the DB delete) and if Firebase delete fails abort with
an error response; alternatively implement a retry/background job (dead-letter
queue) around admin.auth().deleteUser(uid) to ensure eventual success — update
the code paths that reference admin.auth().deleteUser, the DB delete function
(the Mongo delete routine), and any error handling to reflect the new ordering
and retries.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/ProjectDetails.tsx (1)
710-719:⚠️ Potential issue | 🟠 Major
readOnly={true}andonDeleteTaskare passed but never consumed byKanbanBoard.
KanbanBoarddeclares these props in its interface but does not destructure or usecurrentUser,readOnly, oronDeleteTask(see line 48 ofKanbanBoard.tsx). This means the board is still fully interactive (draggable) despitereadOnly={true}, and task deletion from the board is not wired up.This appears to be an incomplete implementation — either the KanbanBoard component needs to be updated to honor these props, or they should be removed from the call site to avoid confusion.
src/components/workspace/KanbanBoard.tsx (1)
25-33:⚠️ Potential issue | 🟠 MajorNew props
currentUser,readOnly, andonDeleteTaskare declared but unused.These are added to the interface but never destructured (line 48) or referenced anywhere in the component. The board remains fully draggable even when
readOnly={true}is passed fromProjectDetails.tsx.At minimum, destructure and wire up the new props:
Suggested starting point
-const KanbanBoard = ({ steps, onUpdateTask, users, isOwner }: KanbanBoardProps) => { +const KanbanBoard = ({ steps, onUpdateTask, users, isOwner, currentUser, readOnly, onDeleteTask }: KanbanBoardProps) => {Then guard drag-and-drop and status updates with
readOnly, and surface a delete action usingonDeleteTask.
🤖 Fix all issues with AI agents
In `@tsconfig.app.json`:
- Line 17: Remove the root tsconfig.json "references" array to eliminate the
project reference validation that requires "composite": true (which conflicts
with "noEmit": true in tsconfig.app.json); open the root tsconfig.json, delete
the entire "references": [...] entry so the typecheck script (which runs tsc -p
per-config) remains the authority and CI no longer demands composite output from
tsconfig.app.json.
🧹 Nitpick comments (2)
src/components/views/PeopleView.tsx (1)
307-307: Consider defining proper types instead of sprinkling(team: any),(user: any), etc.These explicit
anyannotations suppress TS errors but lose type safety. The backend response shape (with fields likeuid,displayName,photoURL,_id,name,inviteCode, etc.) is stable enough to define a shared interface (e.g.,TeamUser,Team). This would catch field-name typos and improve IDE support.Not blocking, but worth a follow-up.
Also applies to: 356-356, 392-392, 471-471, 622-622
tsconfig.app.json (1)
21-22: DisablingnoUnusedLocalsandnoUnusedParametersweakens type-checking.These were presumably toggled off to suppress existing warnings. Consider keeping them enabled and fixing the flagged locations instead, or at least adding a TODO to re-enable them.
| "isolatedModules": true, | ||
| "noEmit": false, | ||
| "emitDeclarationOnly": true, | ||
| "noEmit": true, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check root tsconfig.json for project references
echo "=== Root tsconfig.json ==="
cat tsconfig.json
echo ""
echo "=== package.json typecheck script ==="
grep -A 5 "typecheck" package.json || echo "typecheck not found in package.json"
echo ""
echo "=== tsconfig.app.json content ==="
cat tsconfig.app.jsonRepository: prem22k/Zync
Length of output: 2522
CI is broken: composite: true is required by a project reference, but conflicts with noEmit: true.
The pipeline fails with: Referenced project 'tsconfig.app.json' must have setting "composite": true. However, composite and noEmit are mutually exclusive in TypeScript — you cannot set both. When composite: true is set, TypeScript must generate declaration files, requiring noEmit to be false.
The root tsconfig.json contains "references": [{ "path": "./tsconfig.app.json" }, ...], which triggers validation of project references. Since the typecheck script in package.json already invokes tsc -p explicitly for each config, these project references are unnecessary. Remove the entire "references" array from the root tsconfig.json to unblock CI.
🤖 Prompt for AI Agents
In `@tsconfig.app.json` at line 17, Remove the root tsconfig.json "references"
array to eliminate the project reference validation that requires "composite":
true (which conflicts with "noEmit": true in tsconfig.app.json); open the root
tsconfig.json, delete the entire "references": [...] entry so the typecheck
script (which runs tsc -p per-config) remains the authority and CI no longer
demands composite output from tsconfig.app.json.
…dix-ui/react-navigation-menu-1.2.14 chore(deps): bump @radix-ui/react-navigation-menu from 1.2.13 to 1.2.14
1 participant
🎯 What: Implemented the TODO in
backend/routes/userRoutes.jsto delete a user from Firebase Authentication when their account is deleted from MongoDB via the/delete/confirmendpoint.💡 Why: This ensures data consistency between the database and authentication provider, preventing orphaned accounts in Firebase Auth if the client-side deletion fails or is bypassed.
✅ Verification: Verified locally by creating a reproduction test that mocked
firebase-admin,User,Team, andmailer. Confirmed thatadmin.auth().deleteUser(uid)was called correctly after successful database deletion. Removed the test file to avoid potential CI conflicts. Verified existing tests pass.✨ Result: Users are now deleted from both MongoDB and Firebase Auth upon confirmation.
PR created automatically by Jules for task 12420060558074858245 started by @prem22k
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Style