Add cloud function for delete space #170
Conversation
WalkthroughThe pull request introduces a new Firebase Cloud Function Changes
Sequence DiagramsequenceDiagram
participant Firestore
participant DeleteSpaceFunction
participant SpaceCollections
Firestore->>DeleteSpaceFunction: Trigger on space document deletion
DeleteSpaceFunction->>SpaceCollections: Delete space_members
DeleteSpaceFunction->>SpaceCollections: Delete group_keys
DeleteSpaceFunction->>SpaceCollections: Delete space_places
DeleteSpaceFunction->>SpaceCollections: Delete space_invitations
alt Deletion Successful
DeleteSpaceFunction-->>Firestore: Deletion complete
else Deletion Failed
DeleteSpaceFunction-->>Firestore: Error logged and thrown
end
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
functions/index.js (2)
88-91: Update log messages for consistency.The log messages refer to "Group data" and "Group messages" which is inconsistent with the function's purpose of deleting space data.
- console.log('Group data deleted successfully.', spaceId); + console.log('Space data deleted successfully.', spaceId); - console.error('Error deleting Group messages:', error); + console.error('Error deleting Space data:', error); - throw new Error(`Failed to delete Group data for space: ${spaceId}`); + throw new Error(`Failed to delete Space data for space: ${spaceId}`);
57-80: Optimize deletion operations for better performance.The sequential deletion of collections could be slow for large spaces. Consider parallelizing these operations using
Promise.all().- await firebase_tools.firestore - .delete(`spaces/${spaceId}/space_members`, { - project: process.env.GCLOUD_PROJECT, - recursive: true, - yes: true, - force: true - }); - - await firebase_tools.firestore - .delete(`spaces/${spaceId}/group_keys`, { - project: process.env.GCLOUD_PROJECT, - recursive: true, - yes: true, - force: true - }); - - await firebase_tools.firestore - .delete(`spaces/${spaceId}/space_places`, { - project: process.env.GCLOUD_PROJECT, - recursive: true, - yes: true, - force: true - }); + const deleteOptions = { + project: process.env.GCLOUD_PROJECT, + recursive: true, + yes: true, + force: true + }; + + await Promise.all([ + firebase_tools.firestore + .delete(`spaces/${spaceId}/space_members`, deleteOptions), + firebase_tools.firestore + .delete(`spaces/${spaceId}/group_keys`, deleteOptions), + firebase_tools.firestore + .delete(`spaces/${spaceId}/space_places`, deleteOptions) + ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
functions/index.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
functions/index.js (2)
49-93: Overall implementation looks good!The
deleteSpacefunction follows the established patterns in the codebase and properly handles the cascading deletion of related collections. After addressing the suggested improvements, this implementation will be robust and maintainable.
53-54: Verify data access from deleted document.The function assumes the deleted document's data contains an
idfield. However, accessing data from a deleted document might be unreliable. Consider usingevent.params.spaceIdinstead, which is guaranteed to be available from the trigger path.- var spaceId = snap.data().id; + var spaceId = event.params.spaceId;✅ Verification successful
Confirmed: Use event.params.spaceId instead of snap.data().id
The review suggestion is correct. While both approaches are used in the codebase, using
event.params.spaceIdis more reliable as:
- It's guaranteed to be available from the trigger path "spaces/{spaceId}"
- It avoids potential issues with accessing deleted document data
- It's already used successfully in other parts of the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for similar delete triggers and their data access patterns echo "=== Searching for delete triggers ===" ast-grep --pattern 'functions.firestore.document($$$).onDelete((event) => { $$$ })' echo -e "\n=== Searching for snap.data() usage ===" rg "snap\.data\(\)" -A 2 echo -e "\n=== Searching for event.params usage ===" rg "event\.params" -A 2Length of output: 1093
Script:
#!/bin/bash # Search for the trigger path definition echo "=== Searching for space delete trigger definition ===" rg "spaces/\{.*\}" -A 2 # Search for any error handling around snap.data() echo -e "\n=== Searching for snap.data() error handling ===" rg "snap\.data\(\)" -B 2 -A 5Length of output: 2941
Summary by CodeRabbit