Repository navigation
fix: duplicate chats being created for eCurrency - #526
Conversation
WalkthroughAdds eCurrency Chat deduplication logic across chat creation flows. When creating a chat with a name starting with or containing "eCurrency Chat", the system now checks for existing chats with the same name and reuses them, updating mappings and lock lists accordingly, rather than creating duplicates. Changes
Sequence DiagramsequenceDiagram
participant Controller
participant ChatService
participant Database
Controller->>ChatService: createChat(name, participants, creator)
alt name contains "eCurrency Chat"
ChatService->>Database: Query chat by name (with relations)
alt Chat exists
Database-->>ChatService: Return existing chat
ChatService-->>Controller: Return reused chat
else Chat not found
ChatService->>Database: Resolve participant IDs
Database-->>ChatService: Validate participants exist
ChatService->>Database: Create & save new chat (in transaction)
Database-->>ChatService: Return created chat
ChatService-->>Controller: Return new chat
end
else Non-eCurrency name
ChatService->>Database: Create new group chat
Database-->>ChatService: Return created chat
ChatService-->>Controller: Return chat
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
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: 0
🧹 Nitpick comments (5)
platforms/pictique-api/src/services/ChatService.ts (1)
66-66: Redundant condition:startsWithis covered byincludes.The check
name.startsWith("eCurrency Chat") || name.includes("eCurrency Chat")is redundant sinceincludesalready covers all cases wherestartsWithwould return true.- if (name && (name.startsWith("eCurrency Chat") || name.includes("eCurrency Chat"))) { + if (name?.includes("eCurrency Chat")) {platforms/pictique-api/src/controllers/WebhookController.ts (2)
300-346: Significant code duplication across branches.The three branches (existing eCurrency Chat, new eCurrency Chat, regular group chat) share nearly identical post-processing logic (lines 312-318, 325-331, 339-345). This increases maintenance burden.
Consider extracting the common logic:
} else { - // Group chat - check for eCurrency Chat duplicates by name - const chatName = local.data.name as string; - if (chatName && (chatName.startsWith("eCurrency Chat") || chatName.includes("eCurrency Chat"))) { - // Check if chat with this name already exists - const existingChat = await this.chatService.chatRepository.findOne({ - where: { name: chatName }, - relations: ["participants", "admins"] - }); - - if (existingChat) { - console.log(`⚠️ eCurrency Chat with name "${chatName}" already exists, using existing: ${existingChat.id}`); - chat = existingChat; - chat.admins = admins; - await this.chatService.chatRepository.save(chat); - this.adapter.addToLockedIds(chat.id); - await this.adapter.mappingDb.storeMapping({ - localId: chat.id, - globalId: req.body.id, - }); - } else { - // Create new chat - chat = await this.chatService.createChat( - chatName, - participantIds - ); - chat.admins = admins; - await this.chatService.chatRepository.save(chat); - this.adapter.addToLockedIds(chat.id); - await this.adapter.mappingDb.storeMapping({ - localId: chat.id, - globalId: req.body.id, - }); - } - } else { - // Regular group chat - always create new - chat = await this.chatService.createChat( - chatName, - participantIds - ); - chat.admins = admins; - await this.chatService.chatRepository.save(chat); - this.adapter.addToLockedIds(chat.id); - await this.adapter.mappingDb.storeMapping({ - localId: chat.id, - globalId: req.body.id, - }); - } + // Group chat - createChat handles eCurrency deduplication internally + const chatName = local.data.name as string; + chat = await this.chatService.createChat( + chatName, + participantIds + ); + chat.admins = admins; + await this.chatService.chatRepository.save(chat); + this.adapter.addToLockedIds(chat.id); + await this.adapter.mappingDb.storeMapping({ + localId: chat.id, + globalId: req.body.id, + }); }Since
ChatService.createChatalready handles eCurrency Chat deduplication transactionally (lines 64-102 in ChatService.ts), the redundant check here can be removed, simplifying this code significantly.
302-307: Same redundant condition and direct repository access.
The condition
chatName.startsWith("eCurrency Chat") || chatName.includes("eCurrency Chat")is redundant (same as in ChatService).Accessing
this.chatService.chatRepository.findOnedirectly breaks encapsulation. TheChatServicealready exposes afindByIdmethod; consider adding afindByNamemethod if needed.platforms/blabsy-w3ds-auth-api/src/controllers/WebhookController.ts (2)
167-167: Same redundant condition.
chatName.startsWith("eCurrency Chat") || chatName.includes("eCurrency Chat")is redundant—includescovers allstartsWithcases.- if (chatName && (chatName.startsWith("eCurrency Chat") || chatName.includes("eCurrency Chat"))) { + if (chatName?.includes("eCurrency Chat")) {
168-183: Non-atomic check-then-skip could still allow race conditions.The query and conditional skip are not atomic. Under concurrent webhook calls, two requests could both pass the
existingChatsSnapshot.emptycheck before either stores the mapping.For Firestore, consider using a transaction or setting the document with a deterministic ID (e.g., derived from the chat name) to leverage Firestore's idempotent
setoperation.Additionally, if multiple documents somehow exist with the same name (from a prior race), only
docs[0]is used silently. Consider logging a warning ifexistingChatsSnapshot.size > 1.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
platforms/blabsy-w3ds-auth-api/src/controllers/WebhookController.ts(1 hunks)platforms/pictique-api/src/controllers/WebhookController.ts(1 hunks)platforms/pictique-api/src/services/ChatService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
platforms/pictique-api/src/services/ChatService.ts (1)
platforms/pictique-api/src/database/data-source.ts (1)
AppDataSource(15-29)
platforms/blabsy-w3ds-auth-api/src/controllers/WebhookController.ts (3)
platforms/pictique-api/src/web3adapter/watchers/subscriber.ts (1)
adapter(17-22)platforms/evoting-api/src/web3adapter/watchers/subscriber.ts (1)
adapter(15-20)platforms/group-charter-manager-api/src/web3adapter/watchers/subscriber.ts (1)
adapter(15-20)
⏰ 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/src/controllers/WebhookController.ts (1)
164-184: Approve the deduplication approach with suggestions above.The early-exit pattern for existing eCurrency Chats is clean and aligns with the pictique-api changes. The implementation correctly reuses the existing chat and stores the global-to-local mapping.
platforms/pictique-api/src/services/ChatService.ts (1)
67-102: Consider database-level constraints for chat name uniqueness.The transactional approach in the code is a solid improvement, but under PostgreSQL's default READ COMMITTED isolation level, a race condition could occur where two concurrent transactions both pass the
findOnecheck before either commits. Add a unique constraint on thenamecolumn, or useSERIALIZABLEisolation for this operation to ensure robust protection against duplicate chat creation.
2 participants
Description of change
Issue Number
Type of change
How the change has been tested
Change checklist
Summary by CodeRabbit
New Features
✏️ Tip: You can customize this high-level summary in your review settings.