Merge solutions #1770
Closed
Merge solutions #1770
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
Contributor
Reviewer's GuideThis PR merges multiple solutions by introducing JID normalization and session cleanup routines, overhauling message key generation with normalized JIDs, enhancing caching and status update logic in the Baileys WhatsApp integration, improving logging and error handling, refining on-WhatsApp number verification, updating default speechToText behavior and auto-setting credentials in the OpenAI controller with accompanying database migrations, adjusting dependency installation in Docker, and tuning messageId derivation in the EvoAI service. ER diagram for OpenaiSetting speechToText default changeerDiagram
OpenaiSetting {
BOOLEAN speechToText "DEFAULT true"
}
OpenaiSetting ||--o| OpenaiCreds : uses
OpenaiSetting ||--o| Instance : belongs_to
Class diagram for JID normalization and session cleanup utilitiesclassDiagram
class BaileysStartupService {
+normalizeJid(jid: string): string
+clearCorruptedSessionData(instanceId: string, baileysCache: CacheService): Promise<void>
}
class CacheService
class CacheEngine
class configService
BaileysStartupService --> CacheService
CacheService --> CacheEngine
CacheEngine --> configService
Class diagram for OpenaiController credential and speechToText logicclassDiagram
class OpenaiController {
+setCredentialAsDefault(instanceId: string, credentialId: string): Promise<void>
+createCreds(instance: InstanceDto, data: any): Promise<any>
}
class credsRepository
class settingsRepository
OpenaiController --> credsRepository
OpenaiController --> settingsRepository
Class diagram for onWhatsappCache number format logicclassDiagram
class onWhatsappCache {
+getAvailableNumbers(remoteJid: string): string[]
+saveOnWhatsappCache(data: ISaveOnWhatsappCacheParams[]): Promise<void>
}
class ISaveOnWhatsappCacheParams {
+remoteJid: string
+lid: string | null
}
onWhatsappCache --> ISaveOnWhatsappCacheParams
Class diagram for EvoaiService messageId logic updateclassDiagram
class EvoaiService {
+sendMessage(msg: any, remoteJid: string): Promise<any>
}
EvoaiService : - messageId = remoteJid.split('@')[0] || uuidv4()
File-Level Changes
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}
Contributor
There was a problem hiding this comment.
Hey @VCalazans - 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/channel/whatsapp/whatsapp.baileys.service.ts:178` </location>
<code_context>
+}
+
+// Function to clear corrupted session data
+async function clearCorruptedSessionData(instanceId: string, baileysCache: CacheService) {
+ try {
+ // Clear all baileys cache for this instance
</code_context>
<issue_to_address>
The cache clearing patterns in clearCorruptedSessionData may be overly broad.
The current cache deletion patterns may match unintended keys, risking accidental data loss. Please refine them to target only the necessary session data.
Suggested implementation:
```typescript
// Clear only session-related cache keys for this instance
const patterns = [
`${instanceId}:session*`,
`${instanceId}:prekey*`,
`${instanceId}:app-state-sync-key*`,
`${instanceId}:signal-identities*`
];
```
```typescript
for (const pattern of patterns) {
await baileysCache.deleteByPattern(pattern);
}
```
</issue_to_address>
### Comment 2
<location> `src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts:426` </location>
<code_context>
statusReason: (lastDisconnect?.error as Boom)?.output?.statusCode ?? 200,
};
+
+ this.logger.log(`Connection state changed to: ${connection}, instance: ${this.instance.id}`);
+ if (lastDisconnect?.error) {
+ this.logger.warn(`Connection error: ${JSON.stringify(lastDisconnect.error)}`);
+ }
</code_context>
<issue_to_address>
Logging sensitive error details may expose internal information.
Sanitize or redact sensitive fields from lastDisconnect.error before logging, particularly in production.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
this.logger.log(`Connection state changed to: ${connection}, instance: ${this.instance.id}`);
if (lastDisconnect?.error) {
this.logger.warn(`Connection error: ${JSON.stringify(lastDisconnect.error)}`);
}
=======
this.logger.log(`Connection state changed to: ${connection}, instance: ${this.instance.id}`);
if (lastDisconnect?.error) {
// Sanitize error object before logging
const error = lastDisconnect.error;
const sanitizedError = { ...error };
// Redact sensitive fields if present
if ('stack' in sanitizedError) sanitizedError.stack = '[REDACTED]';
if ('message' in sanitizedError && process.env.NODE_ENV === 'production') sanitizedError.message = '[REDACTED]';
if ('output' in sanitizedError && sanitizedError.output?.payload) sanitizedError.output.payload = '[REDACTED]';
this.logger.warn(`Connection error: ${JSON.stringify(sanitizedError)}`);
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts:709` </location>
<code_context>
this.endSession = false;
+ // Clear any corrupted session data before connecting
+ await clearCorruptedSessionData(this.instanceId, this.baileysCache);
+
this.client = makeWASocket(socketConfig);
</code_context>
<issue_to_address>
Clearing session data on every connect may impact performance.
If clearing session data isn't always required, consider making this operation conditional or rate-limited to avoid unnecessary latency, especially under high load.
Suggested implementation:
```typescript
// Rate-limit clearing corrupted session data to avoid unnecessary latency
const SESSION_CLEAR_INTERVAL_MS = 10 * 60 * 1000; // 10 minutes
if (!this.lastSessionClear || Date.now() - this.lastSessionClear > SESSION_CLEAR_INTERVAL_MS) {
await clearCorruptedSessionData(this.instanceId, this.baileysCache);
this.lastSessionClear = Date.now();
}
this.client = makeWASocket(socketConfig);
```
You must ensure that `this.lastSessionClear` is initialized in the class constructor, e.g.:
```ts
this.lastSessionClear = 0;
```
If you want a different condition (e.g., a flag or a more advanced rate limiter), adjust the logic accordingly.
</issue_to_address>
### Comment 4
<location> `src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts:4290` </location>
<code_context>
+ this.logger.warn(`Message sent with PENDING status - ID: ${message.key.id}, Instance: ${this.instance.id}, Status: ${message.status}, RemoteJid: ${message.key.remoteJid}`);
+ }
+
+ if (!messageRaw.status) {
+ if (message.key.fromMe === false) {
+ messageRaw.status = status[3]; // DELIVERED MESSAGE for received messages
+ } else {
+ messageRaw.status = status[2]; // SERVER_ACK for sent messages without status
+ }
}
</code_context>
<issue_to_address>
Defaulting sent message status to SERVER_ACK may mask delivery issues.
Review whether SERVER_ACK is the appropriate default for all sent messages, or if stricter checks are needed to avoid hiding delivery problems.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (!messageRaw.status) {
if (message.key.fromMe === false) {
messageRaw.status = status[3]; // DELIVERED MESSAGE for received messages
} else {
messageRaw.status = status[2]; // SERVER_ACK for sent messages without status
}
}
=======
if (!messageRaw.status) {
if (message.key.fromMe === false) {
messageRaw.status = status[3]; // DELIVERED MESSAGE for received messages
} else {
// Only set SERVER_ACK if we have evidence of server acknowledgement
if (message.status && message.status >= status[2]) {
messageRaw.status = status[2]; // SERVER_ACK for sent messages with server ack
} else {
// Otherwise, set to PENDING and log a warning
messageRaw.status = status[1]; // PENDING
this.logger.warn(
`Sent message without explicit server ack - defaulting to PENDING. ID: ${message.key.id}, Instance: ${this.instance.id}, Status: ${message.status}, RemoteJid: ${message.key.remoteJid}`
);
}
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `src/api/integrations/chatbot/openai/controllers/openai.controller.ts:239` </location>
<code_context>
if (!data.name) throw new BadRequestException('Name is required');
// Check if API key already exists
- const existingApiKey = await this.credsRepository.findFirst({
- where: {
- apiKey: data.apiKey,
- },
- });
+ // const existingApiKey = await this.credsRepository.findFirst({
+ // where: {
+ // apiKey: data.apiKey,
+ // },
+ // });
- if (existingApiKey) {
- throw new BadRequestException('This API key is already registered. Please use a different API key.');
- }
</code_context>
<issue_to_address>
Removing API key uniqueness check may allow duplicate credentials.
If allowing duplicate API keys is intentional, confirm that downstream systems can handle this and that it won't cause security or billing problems.
</issue_to_address>
### Comment 6
<location> `src/api/integrations/chatbot/openai/controllers/openai.controller.ts:303` </location>
<code_context>
return creds;
}
+ private async setCredentialAsDefault(instanceId: string, credentialId: string) {
+ try {
+ // Check if settings already exist for this instance
</code_context>
<issue_to_address>
setCredentialAsDefault swallows errors, which may hide failures.
Consider propagating or reporting errors instead of only logging them, to ensure failures in setting the default credential are detected and handled appropriately.
</issue_to_address>
### Comment 7
<location> `src/api/integrations/chatbot/evoai/services/evoai.service.ts:73` </location>
<code_context>
}
const callId = `req-${uuidv4().substring(0, 8)}`;
- const messageId = msg?.key?.id || uuidv4();
+ const messageId = remoteJid.split('@')[0] || uuidv4(); // Use phone number as messageId
// Prepare message parts
</code_context>
<issue_to_address>
Using the phone number as messageId may cause collisions.
This approach can lead to duplicate message IDs for the same recipient. To avoid tracking or deduplication issues, use a unique identifier such as a timestamp or random suffix.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| // Function to clear corrupted session data | ||
| async function clearCorruptedSessionData(instanceId: string, baileysCache: CacheService) { |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): The cache clearing patterns in clearCorruptedSessionData may be overly broad.
The current cache deletion patterns may match unintended keys, risking accidental data loss. Please refine them to target only the necessary session data.
Suggested implementation:
// Clear only session-related cache keys for this instance
const patterns = [
`${instanceId}:session*`,
`${instanceId}:prekey*`,
`${instanceId}:app-state-sync-key*`,
`${instanceId}:signal-identities*`
];
for (const pattern of patterns) {
await baileysCache.deleteByPattern(pattern);
}
Comment on lines
+426
to
+429
| this.logger.log(`Connection state changed to: ${connection}, instance: ${this.instance.id}`); | ||
| if (lastDisconnect?.error) { | ||
| this.logger.warn(`Connection error: ${JSON.stringify(lastDisconnect.error)}`); | ||
| } |
Contributor
There was a problem hiding this comment.
🚨 suggestion (security): Logging sensitive error details may expose internal information.
Sanitize or redact sensitive fields from lastDisconnect.error before logging, particularly in production.
Suggested change
| this.logger.log(`Connection state changed to: ${connection}, instance: ${this.instance.id}`); | |
| if (lastDisconnect?.error) { | |
| this.logger.warn(`Connection error: ${JSON.stringify(lastDisconnect.error)}`); | |
| } | |
| this.logger.log(`Connection state changed to: ${connection}, instance: ${this.instance.id}`); | |
| if (lastDisconnect?.error) { | |
| // Sanitize error object before logging | |
| const error = lastDisconnect.error; | |
| const sanitizedError = { ...error }; | |
| // Redact sensitive fields if present | |
| if ('stack' in sanitizedError) sanitizedError.stack = '[REDACTED]'; | |
| if ('message' in sanitizedError && process.env.NODE_ENV === 'production') sanitizedError.message = '[REDACTED]'; | |
| if ('output' in sanitizedError && sanitizedError.output?.payload) sanitizedError.output.payload = '[REDACTED]'; | |
| this.logger.warn(`Connection error: ${JSON.stringify(sanitizedError)}`); | |
| } |
| this.endSession = false; | ||
|
|
||
| // Clear any corrupted session data before connecting | ||
| await clearCorruptedSessionData(this.instanceId, this.baileysCache); |
Contributor
There was a problem hiding this comment.
suggestion (performance): Clearing session data on every connect may impact performance.
If clearing session data isn't always required, consider making this operation conditional or rate-limited to avoid unnecessary latency, especially under high load.
Suggested implementation:
// Rate-limit clearing corrupted session data to avoid unnecessary latency
const SESSION_CLEAR_INTERVAL_MS = 10 * 60 * 1000; // 10 minutes
if (!this.lastSessionClear || Date.now() - this.lastSessionClear > SESSION_CLEAR_INTERVAL_MS) {
await clearCorruptedSessionData(this.instanceId, this.baileysCache);
this.lastSessionClear = Date.now();
}
this.client = makeWASocket(socketConfig);You must ensure that this.lastSessionClear is initialized in the class constructor, e.g.:
this.lastSessionClear = 0;If you want a different condition (e.g., a flag or a more advanced rate limiter), adjust the logic accordingly.
Comment on lines
+4290
to
4296
| if (!messageRaw.status) { | ||
| if (message.key.fromMe === false) { | ||
| messageRaw.status = status[3]; // DELIVERED MESSAGE for received messages | ||
| } else { | ||
| messageRaw.status = status[2]; // SERVER_ACK for sent messages without status | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Defaulting sent message status to SERVER_ACK may mask delivery issues.
Review whether SERVER_ACK is the appropriate default for all sent messages, or if stricter checks are needed to avoid hiding delivery problems.
Suggested change
| if (!messageRaw.status) { | |
| if (message.key.fromMe === false) { | |
| messageRaw.status = status[3]; // DELIVERED MESSAGE for received messages | |
| } else { | |
| messageRaw.status = status[2]; // SERVER_ACK for sent messages without status | |
| } | |
| } | |
| if (!messageRaw.status) { | |
| if (message.key.fromMe === false) { | |
| messageRaw.status = status[3]; // DELIVERED MESSAGE for received messages | |
| } else { | |
| // Only set SERVER_ACK if we have evidence of server acknowledgement | |
| if (message.status && message.status >= status[2]) { | |
| messageRaw.status = status[2]; // SERVER_ACK for sent messages with server ack | |
| } else { | |
| // Otherwise, set to PENDING and log a warning | |
| messageRaw.status = status[1]; // PENDING | |
| this.logger.warn( | |
| `Sent message without explicit server ack - defaulting to PENDING. ID: ${message.key.id}, Instance: ${this.instance.id}, Status: ${message.status}, RemoteJid: ${message.key.remoteJid}` | |
| ); | |
| } | |
| } | |
| } |
Comment on lines
-239
to
-245
| const existingApiKey = await this.credsRepository.findFirst({ | ||
| where: { | ||
| apiKey: data.apiKey, | ||
| }, | ||
| }); | ||
| // const existingApiKey = await this.credsRepository.findFirst({ | ||
| // where: { | ||
| // apiKey: data.apiKey, | ||
| // }, | ||
| // }); | ||
|
|
||
| if (existingApiKey) { |
Contributor
There was a problem hiding this comment.
🚨 question (security): Removing API key uniqueness check may allow duplicate credentials.
If allowing duplicate API keys is intentional, confirm that downstream systems can handle this and that it won't cause security or billing problems.
| return creds; | ||
| } | ||
|
|
||
| private async setCredentialAsDefault(instanceId: string, credentialId: string) { |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): setCredentialAsDefault swallows errors, which may hide failures.
Consider propagating or reporting errors instead of only logging them, to ensure failures in setting the default credential are detected and handled appropriately.
| } | ||
|
|
||
| const callId = `req-${uuidv4().substring(0, 8)}`; | ||
| const messageId = msg?.key?.id || uuidv4(); |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using the phone number as messageId may cause collisions.
This approach can lead to duplicate message IDs for the same recipient. To avoid tracking or deduplication issues, use a unique identifier such as a timestamp or random suffix.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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
Standardize JID handling and enhance robustness, logging, and session management in the WhatsApp integration; automate message status updates; refine caching for WhatsApp contacts; default speech-to-text behavior and auto-select new OpenAI credentials; adjust Docker build flags; update messageId logic in EvoAI; and introduce database migrations for speechToText defaults
New Features:
Enhancements:
Build:
Chores: