Pr1705 #1713
Closed
Pr1705 #1713
+2,294
−2,096
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
CONFIG_SESSION_PHONE_VERSION updated
CONFIG_SESSION_PHONE_VERSION Updated
Update EvolutionApi DockerHub Repository and Delete Config Session Variable
…ssar-mensagens-com-variáveis Fix Typebot formatting function
fix: corrige versão inválida do swagger-ui-express
Update Dockerhub Repository and Delete Config Session Variable
…age-upsert feat: Adiciona mesclagem de contatos @lid no Chatwoot
Correção do envio de variáveis pelo typeboy
Update Dockerfile
add unreadMessages in the response
🐛 fix: Phone number as message ID for Evo AI #ISSUE 28
…or valid media content
…services to enhance error handling
…n and streamlining file copy operations
Create railway.json
Corrige ref. instance typebot.controller.ts
- Implement broken event checking before duplicate message checking. (Do not process failed events). - Implement error handling when downloading media with a fallback mechanism.
…Base64FromMediaMessage method
Reviewer's GuideThis PR upgrades the WhatsApp Baileys integration to the latest API types, hardens media handling with new validation/fallback logic, refines LID‐based JID remapping and contact merging, and bumps package and Docker images to version 2.3.1. Sequence diagram for media message upload with validation and fallbacksequenceDiagram
participant Service as BaileysStartupService
participant S3 as s3Service
participant Prisma as prismaRepository
participant Logger as logger
participant Baileys as Baileys API
participant Fallback as downloadContentFromMessage
Service->>Service: hasValidMediaContent(message)
alt No valid media
Service->>Logger: warn('Message detected as media but contains no valid media content')
Service-->>Service: Skip upload
else Valid media
Service->>Service: getBase64FromMediaMessage()
alt DownloadMediaMessage fails
Service->>Logger: error('Download Media failed, trying to retry in 5 seconds...')
Service->>Fallback: downloadContentFromMessage()
alt Fallback fails
Service->>Logger: error('Download Media with downloadContentFromMessage also failed!')
else Fallback succeeds
Service->>Logger: info('Download Media with downloadContentFromMessage was successful!')
end
else DownloadMediaMessage succeeds
end
Service->>S3: uploadFile(fullName, buffer, size, {Content-Type})
Service->>Prisma: media.create({ ... })
Service->>S3: getObjectUrl(fullName)
Service->>Prisma: message.update({ ... })
end
Sequence diagram for LID-based JID remapping and contact merging in ChatwootServicesequenceDiagram
participant Service as ChatwootService
participant Instance as InstanceDto
participant Chatwoot as chatwootRequest
participant Logger as logger
Service->>Service: createConversation(instance, body)
alt isLid and senderPn !== previousRemoteJid
Service->>Service: findContact(instance, remoteJid)
alt contact.identifier !== senderPn
Service->>Service: updateContact(instance, contact.id, ...)
alt updateContact === null
Service->>Service: findContact(instance, senderPn)
alt baseContact exists
Service->>Service: mergeContacts(baseContact.id, contact.id)
Service->>Logger: verbose('Merge contacts ...')
end
end
end
end
Class diagram for BaileysStartupService media handling changesclassDiagram
class BaileysStartupService {
+hasValidMediaContent(message): boolean
+mapMediaType(mediaType): Promise<string|null>
+getBase64FromMediaMessage(data, getBuffer): Promise<any>
}
BaileysStartupService --|> ChannelStartupService
Class diagram for ChatwootService contact merging changesclassDiagram
class ChatwootService {
-mergeContacts(baseId: number, mergeId: number)
+createConversation(instance: InstanceDto, body: any)
}
Class diagram for unreadMessages addition in ChannelStartupServiceclassDiagram
class ChannelStartupService {
+hasValidMediaContent(message): boolean
}
Class diagram for EvoaiService messageId logic changeclassDiagram
class EvoaiService {
+sendMessage(...)
-messageId = msg?.key?.id || uuidv4()
+messageId = remoteJid.split('@')[0] || uuidv4()
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @alkimimcitty - I've reviewed your changes - here's some feedback:
- There’s duplicated media validation and S3 upload code across services; consider extracting it into a shared helper to reduce repetition and improve maintainability.
- The '@lid' JID normalization logic appears in multiple handlers; centralizing that transformation into a single utility would ensure consistency.
- The fallback path in getBase64FromMediaMessage retries the download once but lacks a retry limit or clear error propagation—consider adding a defined retry policy and proper error handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s duplicated media validation and S3 upload code across services; consider extracting it into a shared helper to reduce repetition and improve maintainability.
- The '@lid' JID normalization logic appears in multiple handlers; centralizing that transformation into a single utility would ensure consistency.
- The fallback path in getBase64FromMediaMessage retries the download once but lacks a retry limit or clear error propagation—consider adding a defined retry policy and proper error handling.
## Individual Comments
### Comment 1
<location> `src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts:1047` </location>
<code_context>
},
});
- if (
- openAiDefaultSettings &&
- openAiDefaultSettings.openaiCredsId &&
</code_context>
<issue_to_address>
Logging entire received object may expose sensitive data.
Log only necessary identifiers or error details to avoid exposing sensitive information.
</issue_to_address>
### Comment 2
<location> `src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts:3511` </location>
<code_context>
+ let buffer: Buffer;
</code_context>
<issue_to_address>
Fallback download logic does not propagate errors or handle all failure scenarios.
If both download attempts fail, ensure the function returns or throws an error to prevent undefined buffer usage.
</issue_to_address>
### Comment 3
<location> `src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts:570` </location>
<code_context>
public async createConversation(instance: InstanceDto, body: any) {
- const isLid = body.key.remoteJid.includes('@lid') && body.key.senderPn;
- const remoteJid = isLid ? body.key.senderPn : body.key.remoteJid;
+ const isLid = body.key.previousRemoteJid?.includes('@lid') && body.key.senderPn;
+ const remoteJid = body.key.remoteJid;
const cacheKey = `${instance.instanceName}:createConversation-${remoteJid}`;
const lockKey = `${instance.instanceName}:lock:createConversation-${remoteJid}`;
</code_context>
<issue_to_address>
Switching to previousRemoteJid for isLid logic may break compatibility with older message formats.
If previousRemoteJid is missing, this logic could fail. Please ensure previousRemoteJid is always set or add a fallback mechanism.
</issue_to_address>
### Comment 4
<location> `src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts:592` </location>
<code_context>
phone_number: `+${body.key.senderPn.split('@')[0]}`,
});
+
+ if (updateContact === null) {
+ const baseContact = await this.findContact(instance, body.key.senderPn.split('@')[0]);
+ if (baseContact) {
+ await this.mergeContacts(baseContact.id, contact.id);
+ this.logger.verbose(
+ `Merge contacts: (${baseContact.id}) ${baseContact.phone_number} and (${contact.id}) ${contact.phone_number}`,
+ );
+ }
+ }
</code_context>
<issue_to_address>
mergeContacts is called without checking for race conditions or merge conflicts.
Concurrent merges may cause data inconsistencies. Please implement locking or conflict resolution if concurrent updates are possible.
Suggested implementation:
```typescript
if (updateContact === null) {
const baseContact = await this.findContact(instance, body.key.senderPn.split('@')[0]);
if (baseContact) {
// Acquire lock before merging
const lockAcquired = await this.acquireContactMergeLock([baseContact.id, contact.id]);
if (!lockAcquired) {
this.logger.warn(
`Could not acquire lock for merging contacts: (${baseContact.id}) and (${contact.id}). Merge skipped to avoid race condition.`
);
return;
}
try {
await this.mergeContacts(baseContact.id, contact.id);
this.logger.verbose(
`Merge contacts: (${baseContact.id}) ${baseContact.phone_number} and (${contact.id}) ${contact.phone_number}`,
);
} finally {
this.releaseContactMergeLock([baseContact.id, contact.id]);
}
}
}
```
```typescript
// In-memory lock for contact merges
private contactMergeLocks: Set<number> = new Set();
private async acquireContactMergeLock(contactIds: number[]): Promise<boolean> {
// Sort to avoid deadlocks
contactIds.sort((a, b) => a - b);
for (const id of contactIds) {
if (this.contactMergeLocks.has(id)) {
return false;
}
}
contactIds.forEach(id => this.contactMergeLocks.add(id));
return true;
}
private releaseContactMergeLock(contactIds: number[]) {
contactIds.forEach(id => this.contactMergeLocks.delete(id));
}
private async mergeContacts(baseId: number, mergeId: number) {
try {
const contact = await chatwootRequest(this.getClientCwConfig(), {
method: 'POST',
url: `/api/v1/accounts/${this.provider.accountId}/actions/contact_merge`,
body: {
base_contact_id: baseId,
mergee_contact_id: mergeId,
```
</issue_to_address>
### Comment 5
<location> `src/api/integrations/chatbot/chatwoot/services/chatwoot.service.ts:2138` </location>
<code_context>
+ w: 320,
+ h: 180,
+ });
+ const processedBuffer = await img.getBuffer(JimpMime.png);
const fileStream = new Readable();
</code_context>
<issue_to_address>
JimpMime.png may not be defined if JimpMime is not imported or initialized.
Please import and initialize JimpMime before using it here to prevent runtime errors. Also, confirm compatibility with previous usage of Jimp.MIME_PNG.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1047
to
+1056
| if ( | ||
| received?.messageStubParameters?.some?.((param) => | ||
| [ | ||
| 'No matching sessions found for message', | ||
| 'Bad MAC', | ||
| 'failed to decrypt message', | ||
| 'SessionError', | ||
| 'Invalid PreKey ID', | ||
| ].some((err) => param?.includes?.(err)), | ||
| ) |
There was a problem hiding this comment.
🚨 issue (security): Logging entire received object may expose sensitive data.
Log only necessary identifiers or error details to avoid exposing sensitive information.
Comment on lines
+3511
to
+3520
| let buffer: Buffer; | ||
|
|
||
| try { | ||
| buffer = await downloadMediaMessage( | ||
| { key: msg?.key, message: msg?.message }, | ||
| 'buffer', | ||
| {}, | ||
| { logger: P({ level: 'error' }) as any, reuploadRequest: this.client.updateMediaMessage }, | ||
| ); | ||
| } catch (err) { |
There was a problem hiding this comment.
issue (bug_risk): Fallback download logic does not propagate errors or handle all failure scenarios.
If both download attempts fail, ensure the function returns or throws an error to prevent undefined buffer usage.
Comment on lines
+570
to
+571
| const isLid = body.key.previousRemoteJid?.includes('@lid') && body.key.senderPn; | ||
| const remoteJid = body.key.remoteJid; |
There was a problem hiding this comment.
issue (bug_risk): Switching to previousRemoteJid for isLid logic may break compatibility with older message formats.
If previousRemoteJid is missing, this logic could fail. Please ensure previousRemoteJid is always set or add a fallback mechanism.
| const contact = await this.findContact(instance, body.key.remoteJid.split('@')[0]); | ||
| if (contact && contact.identifier !== body.key.senderPn) { | ||
| this.logger.verbose( | ||
| `Identifier needs update: (contact.identifier: ${contact.identifier}, body.key.remoteJid: ${body.key.remoteJid}, body.key.senderPn: ${body.key.senderPn})`, | ||
| `Identifier needs update: (contact.identifier: ${contact.identifier}, body.key.remoteJid: ${body.key.remoteJid}, body.key.senderPn: ${body.key.senderPn}`, | ||
| ); |
There was a problem hiding this comment.
suggestion (bug_risk): mergeContacts is called without checking for race conditions or merge conflicts.
Concurrent merges may cause data inconsistencies. Please implement locking or conflict resolution if concurrent updates are possible.
Suggested implementation:
if (updateContact === null) {
const baseContact = await this.findContact(instance, body.key.senderPn.split('@')[0]);
if (baseContact) {
// Acquire lock before merging
const lockAcquired = await this.acquireContactMergeLock([baseContact.id, contact.id]);
if (!lockAcquired) {
this.logger.warn(
`Could not acquire lock for merging contacts: (${baseContact.id}) and (${contact.id}). Merge skipped to avoid race condition.`
);
return;
}
try {
await this.mergeContacts(baseContact.id, contact.id);
this.logger.verbose(
`Merge contacts: (${baseContact.id}) ${baseContact.phone_number} and (${contact.id}) ${contact.phone_number}`,
);
} finally {
this.releaseContactMergeLock([baseContact.id, contact.id]);
}
}
} // In-memory lock for contact merges
private contactMergeLocks: Set<number> = new Set();
private async acquireContactMergeLock(contactIds: number[]): Promise<boolean> {
// Sort to avoid deadlocks
contactIds.sort((a, b) => a - b);
for (const id of contactIds) {
if (this.contactMergeLocks.has(id)) {
return false;
}
}
contactIds.forEach(id => this.contactMergeLocks.add(id));
return true;
}
private releaseContactMergeLock(contactIds: number[]) {
contactIds.forEach(id => this.contactMergeLocks.delete(id));
}
private async mergeContacts(baseId: number, mergeId: number) {
try {
const contact = await chatwootRequest(this.getClientCwConfig(), {
method: 'POST',
url: `/api/v1/accounts/${this.provider.accountId}/actions/contact_merge`,
body: {
base_contact_id: baseId,
mergee_contact_id: mergeId,| w: 320, | ||
| h: 180, | ||
| }); | ||
| const processedBuffer = await img.getBuffer(JimpMime.png); |
There was a problem hiding this comment.
issue (bug_risk): JimpMime.png may not be defined if JimpMime is not imported or initialized.
Please import and initialize JimpMime before using it here to prevent runtime errors. Also, confirm compatibility with previous usage of Jimp.MIME_PNG.
| mimetype = !mimetype ? 'image/png' : mimetype; | ||
| } else if (messageRaw.messageType === 'audioMessage') { | ||
| mediaType = 'audio'; | ||
| mimetype = !mimetype ? 'audio/mp4' : mimetype; |
There was a problem hiding this comment.
suggestion (code-quality): Invert ternary operator to remove negation (invert-ternary)
Suggested change
| mimetype = !mimetype ? 'audio/mp4' : mimetype; | |
| mimetype = mimetype ? mimetype : 'audio/mp4'; |
Explanation
Negated conditions are more difficult to read than positive ones, so it is bestto avoid them where we can. By inverting the ternary condition and swapping the
expressions we can simplify the code.
| mimetype = !mimetype ? 'audio/mp4' : mimetype; | ||
| } else if (messageRaw.messageType === 'videoMessage') { | ||
| mediaType = 'video'; | ||
| mimetype = !mimetype ? 'video/mp4' : mimetype; |
There was a problem hiding this comment.
suggestion (code-quality): Invert ternary operator to remove negation (invert-ternary)
Suggested change
| mimetype = !mimetype ? 'video/mp4' : mimetype; | |
| mimetype = mimetype ? mimetype : 'video/mp4'; |
Explanation
Negated conditions are more difficult to read than positive ones, so it is bestto avoid them where we can. By inverting the ternary condition and swapping the
expressions we can simplify the code.
| } else { | ||
| mediaType = 'video'; | ||
| } | ||
| const id = message.messages[0][message.messages[0].type].id; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const id = message.messages[0][message.messages[0].type].id; | |
| const {id} = message.messages[0][message.messages[0].type]; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
Comment on lines
+462
to
+471
| const contact = await chatwootRequest(this.getClientCwConfig(), { | ||
| method: 'POST', | ||
| url: `/api/v1/accounts/${this.provider.accountId}/actions/contact_merge`, | ||
| body: { | ||
| base_contact_id: baseId, | ||
| mergee_contact_id: mergeId, | ||
| }, | ||
| }); | ||
|
|
||
| return contact; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| const contact = await chatwootRequest(this.getClientCwConfig(), { | |
| method: 'POST', | |
| url: `/api/v1/accounts/${this.provider.accountId}/actions/contact_merge`, | |
| body: { | |
| base_contact_id: baseId, | |
| mergee_contact_id: mergeId, | |
| }, | |
| }); | |
| return contact; | |
| return await chatwootRequest(this.getClientCwConfig(), { | |
| method: 'POST', | |
| url: `/api/v1/accounts/${this.provider.accountId}/actions/contact_merge`, | |
| body: { | |
| base_contact_id: baseId, | |
| mergee_contact_id: mergeId, | |
| }, | |
| }); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| const isLid = body.key.remoteJid.includes('@lid') && body.key.senderPn; | ||
| const remoteJid = isLid ? body.key.senderPn : body.key.remoteJid; | ||
| const isLid = body.key.previousRemoteJid?.includes('@lid') && body.key.senderPn; | ||
| const remoteJid = body.key.remoteJid; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const remoteJid = body.key.remoteJid; | |
| const {remoteJid} = body.key; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
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.
Summary by Sourcery
Improve media handling robustness, normalize LID contact identifiers, expose unread message counts, and prepare release v2.3.1
New Features:
Bug Fixes:
Enhancements:
Build:
Deployment:
Documentation:
Chores: