-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(chatwoot): comprehensive improvements to message handling, editing, deletion and i18n (translate messages) #2048
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
…ng, deletion and i18n - Fix bidirectional message deletion between Chatwoot and WhatsApp - Support deletion of multiple attachments sent together - Implement proper message editing with 'Edited Message:' prefix format - Enable deletion of edited messages by updating chatwootMessageId - Skip cache for deleted messages (messageStubType === 1) to prevent duplicates - Fix i18n translation path detection for production environment - Add automatic dev/prod path resolution for translation files - Improve error handling and logging for message operations Technical improvements: - Changed Chatwoot deletion query from findFirst to findMany for multiple attachments - Fixed instanceId override issue in message deletion payload - Added retry logic with Prisma MessageUpdate validation - Implemented cache bypass for revoked messages to ensure proper processing - Enhanced i18n to detect dist/ folder in production vs src/ in development Resolves issues with: - Message deletion not working from Chatwoot to WhatsApp - Multiple attachments causing incomplete deletion - Edited messages showing raw i18n keys instead of translated text - Cache collision preventing deletion of edited messages - Production environment not loading translation files correctly Note: Tested and validated with Chatwoot v4.1 in production environment
🧙 Sourcery has finished reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts:1160-1161` </location>
<code_context>
+ // Vamos tentar com retry se receber 404 (arquivo ainda não disponível)
+ this.logger.verbose('Downloading from S3/MinIO...');
+
+ let s3Response;
+ let retryCount = 0;
+ const maxRetries = 3;
+ const retryDelay = 2000; // 2 segundos entre tentativas
</code_context>
<issue_to_address>
**suggestion:** Consider using exponential backoff for S3/MinIO retries.
Switching to exponential backoff instead of a fixed delay will reduce load on the storage service during transient failures and enhance reliability.
</issue_to_address>
### Comment 2
<location> `src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts:1729-1730` </location>
<code_context>
+ `Updating message with chatwootMessageId: ${chatwootMessageIds.messageId}, keyId: ${key.id}, instanceId: ${instanceId}`,
+ );
+
+ // Aguarda um pequeno delay para garantir que a mensagem foi criada no banco
+ await new Promise((resolve) => setTimeout(resolve, 100));
+
+ // Verifica se a mensagem existe antes de atualizar
</code_context>
<issue_to_address>
**suggestion:** Using fixed delays for database consistency may be unreliable.
Consider replacing the fixed delay with a mechanism that verifies the database write, such as event-based confirmation or polling with backoff.
Suggested implementation:
```typescript
// Polling para garantir que a mensagem foi criada no banco antes de atualizar
let retries = 0;
const maxRetries = 5;
const backoffMs = 100;
let messageExists = false;
while (retries < maxRetries && !messageExists) {
const existingMessage = await this.prismaRepository.message.findFirst({
where: {
instanceId: instanceId,
key: {
path: ['id'],
```
If the polling loop does not already include a delay between retries, you should add:
```typescript
if (!existingMessage) {
await new Promise((resolve) => setTimeout(resolve, backoffMs));
}
```
inside the `while` loop after checking for `existingMessage`.
Also, ensure that the rest of the code properly handles the case when the message is not found after all retries.
</issue_to_address>
### Comment 3
<location> `src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts:1734-1756` </location>
<code_context>
+ const maxRetries = 5;
+ let messageExists = false;
+
+ while (retries < maxRetries && !messageExists) {
+ const existingMessage = await this.prismaRepository.message.findFirst({
+ where: {
+ instanceId: instanceId,
+ key: {
+ path: ['id'],
+ equals: key.id,
+ },
+ },
+ });
+
+ if (existingMessage) {
+ messageExists = true;
+ this.logger.verbose(`Message found in database after ${retries} retries`);
+ } else {
+ retries++;
+ this.logger.verbose(`Message not found, retry ${retries}/${maxRetries}`);
+ await new Promise((resolve) => setTimeout(resolve, 200));
+ }
+ }
+
</code_context>
<issue_to_address>
**suggestion (performance):** Consider increasing retry delay or using exponential backoff for database polling.
A longer or adaptive delay between retries can help prevent excessive database load and improve robustness if the database is slow to respond.
```suggestion
const maxRetries = 5;
let messageExists = false;
let delay = 200; // initial delay in ms
const maxDelay = 2000; // maximum delay in ms
while (retries < maxRetries && !messageExists) {
const existingMessage = await this.prismaRepository.message.findFirst({
where: {
instanceId: instanceId,
key: {
path: ['id'],
equals: key.id,
},
},
});
if (existingMessage) {
messageExists = true;
this.logger.verbose(`Message found in database after ${retries} retries`);
} else {
retries++;
this.logger.verbose(`Message not found, retry ${retries}/${maxRetries}, waiting ${delay}ms`);
await new Promise((resolve) => setTimeout(resolve, delay));
delay = Math.min(delay * 2, maxDelay); // exponential backoff
}
}
```
</issue_to_address>
### Comment 4
<location> `src/utils/i18n.ts:6-9` </location>
<code_context>
import path from 'path';
-const __dirname = path.resolve(process.cwd(), 'src', 'utils');
+// Detect if running from dist/ (production) or src/ (development)
+const isProduction = fs.existsSync(path.join(process.cwd(), 'dist'));
+const baseDir = isProduction ? 'dist' : 'src/utils';
+const translationsPath = path.join(process.cwd(), baseDir, 'translations');
const languages = ['en', 'pt-BR', 'es'];
</code_context>
<issue_to_address>
**suggestion:** Production/development path detection may fail in some deployment scenarios.
Relying on the 'dist' directory for environment detection may not work in all setups. Making this logic configurable would improve reliability across different deployment environments.
```suggestion
import fs from 'fs';
// Make translations base directory configurable via environment variable
const envBaseDir = process.env.TRANSLATIONS_BASE_DIR;
let baseDir: string;
if (envBaseDir) {
baseDir = envBaseDir;
} else {
// Fallback to auto-detection if env variable is not set
const isProduction = fs.existsSync(path.join(process.cwd(), 'dist'));
baseDir = isProduction ? 'dist' : 'src/utils';
}
const translationsPath = path.join(process.cwd(), baseDir, 'translations');
```
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements comprehensive improvements to Chatwoot-WhatsApp bidirectional message handling, focusing on deletion, editing, and internationalization features. The changes address critical issues with message synchronization between platforms and improve production environment compatibility.
- Fixed bidirectional message deletion between Chatwoot and WhatsApp with support for multiple attachments
- Implemented proper message editing with deletion/recreation approach and "Edited Message:" prefix
- Enhanced i18n system to work correctly in both development and production environments
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
File | Description |
---|---|
src/utils/i18n.ts | Fixed i18n path detection for production environments by detecting dist/ vs src/ directories |
src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts | Major overhaul of message handling including deletion, editing, attachment processing, and error handling |
src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts | Enhanced message processing with cache bypass for deleted messages and validation checks |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts
Outdated
Show resolved
Hide resolved
src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts
Outdated
Show resolved
Hide resolved
src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts
Outdated
Show resolved
Hide resolved
src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts
Outdated
Show resolved
Hide resolved
src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts
Outdated
Show resolved
Hide resolved
src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts
Outdated
Show resolved
Hide resolved
src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts
Outdated
Show resolved
Hide resolved
Please resolve the conflicts. |
…mbers to constants - Extract HTTP timeout constant (60s for large file downloads) - Extract S3/MinIO retry configuration (3 retries, 1s-8s exponential backoff) - Extract database polling retry configuration (5 retries, 100ms-2s exponential backoff) - Extract webhook and lock polling delays to named constants - Extract cache TTL values (5min for messages, 30min for updates) in Baileys service - Implement exponential backoff for S3/MinIO downloads following webhook controller pattern - Implement exponential backoff for database polling removing fixed delays - Add deletion event lock to prevent race conditions with duplicate webhooks - Process deletion events immediately (no delay) to fix Chatwoot local storage red error - Make i18n translations path configurable via TRANSLATIONS_BASE_DIR env variable - Add detailed logging for deletion events debugging Addresses code review suggestions from Sourcery AI and Copilot AI: - Magic numbers extracted to well-documented constants - Retry configurations consolidated and clearly separated by use case - S3/MinIO retry uses longer delays (external storage) - Database polling uses shorter delays (internal operations) - Fixes Chatwoot local storage deletion error (red message issue) - Maintains full compatibility with S3/MinIO storage (tested) Breaking changes: None - all changes are internal improvements
Problem: - Chatwoot shows red error when deleting messages with 5+ images - Cause: Chatwoot webhook timeout of 5 seconds - Processing 5 images takes ~9 seconds - Duplicate webhooks arrive during processing Solution: - Implemented async processing with setImmediate() - Webhook responds immediately (< 100ms) - Deletion processes in background without blocking - Maintains idempotency with cache (1 hour TTL) - Maintains lock mechanism (60 seconds TTL) Benefits: - Scales infinitely (10, 20, 100+ images) - No timeout regardless of quantity - No error messages in Chatwoot - Reliable background processing Tested: - 5 images: 9s background processing - Webhook response: < 100ms - No red error in Chatwoot - Deletion completes successfully BREAKING CHANGE: Fixed assertSessions signature to accept force parameter
Problem: - GitHub Actions shows: Expected 1 arguments, but got 2 - Local environment shows: Expected 2 arguments, but got 1 - Different Baileys versions/definitions between environments Solution: - Use 'as any' type assertion for force parameter - Maintains compatibility with both signature variations - Allows code to work in all environments Technical notes: - Local: [email protected] expects 2 arguments (jids, force) - GitHub Actions: May have different version/cache expecting 1 argument - Type assertion bypasses strict type checking for cross-version compatibility
Problem: - GitHub Actions failing: Expected 1 arguments, but got 2 - Local had outdated Baileys 7.0.0-rc.3 in node_modules - assertSessions signature changed between versions Solution: - Fresh npm install with Baileys 7.0.0-rc.5 - Updated assertSessions to pass only jids (no force param) - Regenerated Prisma Client after reinstall - Updated package-lock.json for version consistency Changes: - assertSessions now receives 1 argument (jids only) - Kept force param in method signature for API compatibility - Removed @ts-expect-error directives (no longer needed) Tested: - ✅ Server starts successfully - ✅ Build passes without errors - ✅ Lint passes
📋 Description
Technical improvements:
Resolves issues with:
Note: Tested and validated with Chatwoot v4.1 in production environment
🔗 Related Issue
Closes #(issue_number)
🧪 Type of Change
🧪 Testing
📸 Screenshots (if applicable)
✅ Checklist
📝 Additional Notes
Note: Tested and validated with Chatwoot v4.1 in production environment
Summary by Sourcery
Enhance the Chatwoot-WhatsApp integration by overhauling message handling workflows, including robust deletion and editing support, improved attachment download and processing, dynamic i18n path resolution, and enriched error handling and logging
New Features:
Bug Fixes:
Enhancements: