feat/read-on-receipt #95
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
src/bridge.tsOops! Something went wrong! :( ESLint: 8.57.1 TypeError: prettier.resolveConfig.sync is not a function __tests__/services/client_baileys.tsOops! Something went wrong! :( ESLint: 8.57.1 TypeError: prettier.resolveConfig.sync is not a function src/index.tsOops! Something went wrong! :( ESLint: 8.57.1 TypeError: prettier.resolveConfig.sync is not a function
WalkthroughThis pull request introduces significant architectural changes to the messaging service, primarily focusing on removing the Changes
Poem
✨ 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: 11
🧹 Nitpick comments (7)
__tests__/services/transformer.ts (1)
91-93: Consider adding more test cases for edge scenarios.The test only covers the happy path. Consider adding test cases for:
- Missing caption
- Invalid document message format
- Empty document message
Here's an example of additional test cases:
test('return messageType documentWithCaptionMessage with missing caption', async () => { const payload = { message: { documentWithCaptionMessage: { message: { documentMessage: { url: "mock-url", mimetype: "video/mp4", title: "mock-video.mp4" } } } } } expect(getMessageType(payload)).toEqual('documentWithCaptionMessage') }) test('return messageType documentWithCaptionMessage with empty document message', async () => { const payload = { message: { documentWithCaptionMessage: { message: { documentMessage: {} } } } } expect(getMessageType(payload)).toEqual('documentWithCaptionMessage') })src/services/read_when_receipt.ts (3)
5-8: Clarify interface contract fortest()method.
Currently,test()does not explicitly define its return type or document its intended usage. Consider adding a return type or documentation to convey its purpose clearly, or remove it if it's not needed.
10-13: Confirm necessity of placeholder implementation.
readWhenReceiptDisabledimplementssendandtestas no-ops. If these methods are never called in production, that’s fine, but it may be helpful to add explanatory comments clarifying that these methods intentionally do nothing.
37-39: Rename the interface to avoid confusion.
Having an interface namedgetReadWhenReceiptand a function of the same name may cause confusion. Consider renaming the interface to something likeGetReadWhenReceiptFunctionor removing it if it’s unnecessary.src/services/reload_baileys.ts (1)
Line range hint
21-39: Refactor duplicate client creation logic.The method creates a client twice with identical parameters. Consider extracting the client creation logic to a private method to improve maintainability and reduce code duplication.
+ private async createClient(phone: string) { + return this.getClient({ + phone, + listener: this.listener, + getConfig: this.getConfig, + onNewLogin: this.onNewLogin, + }) + } + async run(phone: string) { - const currentClient = await this.getClient({ - phone, - listener: this.listener, - getConfig: this.getConfig, - onNewLogin: this.onNewLogin, - }) + const currentClient = await this.createClient(phone) const config = await this.getConfig(phone) const store = await config.getStore(phone, config) const { sessionStore } = store @@ -39,12 +31,7 @@ await currentClient.disconnect() } await super.run(phone) - const newClient = await this.getClient({ - phone, - listener: this.listener, - getConfig: this.getConfig, - onNewLogin: this.onNewLogin, - }) + const newClient = await this.createClient(phone) await newClient.connect(1) }src/standalone.ts (1)
159-162: Consider preserving stack traces in error handling.The new error handling loses stack trace information, which could make debugging more difficult.
process.on('unhandledRejection', (args) => { - // logger.error('unhandledRejection: %s', reason.stack) - logger.error('promise: %s', JSON.stringify(args)) - // throw reason + const reason = args instanceof Error ? args : new Error(JSON.stringify(args)) + logger.error('unhandledRejection: %s', reason.stack || reason.message) + throw reason })src/defaults.ts (1)
16-19: LGTM! Well-structured environment variable configuration.The new environment variables follow the established pattern and have sensible defaults.
Consider adding documentation for these new environment variables in the project's README or configuration guide.
Also applies to: 122-122
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
__tests__/services/client_baileys.ts(1 hunks)__tests__/services/incoming_baileys.ts(0 hunks)__tests__/services/transformer.ts(1 hunks)src/bridge.ts(2 hunks)src/defaults.ts(2 hunks)src/index.ts(1 hunks)src/jobs/bind_bridge.ts(1 hunks)src/services/auto_connect.ts(1 hunks)src/services/client.ts(0 hunks)src/services/client_baileys.ts(5 hunks)src/services/config.ts(2 hunks)src/services/config_by_env.ts(2 hunks)src/services/contact_baileys.ts(1 hunks)src/services/data_store_redis.ts(2 hunks)src/services/incoming_baileys.ts(0 hunks)src/services/instance.ts(1 hunks)src/services/listener_baileys.ts(4 hunks)src/services/logout_baileys.ts(1 hunks)src/services/read_when_receipt.ts(1 hunks)src/services/redis.ts(2 hunks)src/services/reload_baileys.ts(1 hunks)src/standalone.ts(5 hunks)
💤 Files with no reviewable changes (3)
- src/services/incoming_baileys.ts
- tests/services/incoming_baileys.ts
- src/services/client.ts
✅ Files skipped from review due to trivial changes (2)
- src/services/redis.ts
- src/services/config.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/jobs/bind_bridge.ts
[error] 30-30: Shouldn't redeclare 'getConfig'. Consider to delete it or rename it.
'getConfig' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (15)
src/services/logout_baileys.ts (1)
13-13: Verify removal ofincomingparameter.
The constructor no longer acceptsincoming. Ensure that all references to the old constructor signature have been updated. Consider searching the codebase to confirm no remaining references toLogoutBaileysexpectincoming.✅ Verification successful
Constructor signature update is complete
All instances ofLogoutBaileysinstantiation in the codebase use the new constructor signature with four parameters. No references to the old signature withincomingparameter were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no references use the old constructor signature with `incoming`. rg -A 1 'new LogoutBaileys\(.*\,\s*incoming'Length of output: 168
Script:
#!/bin/bash # Search for any instantiation of LogoutBaileys rg "new LogoutBaileys" -A 2 # Search for type definitions and interfaces related to LogoutBaileys rg "LogoutBaileys" -g "*.ts" -g "*.d.ts" -A 3 # Use ast-grep to find class instantiations ast-grep --pattern 'new LogoutBaileys($_)'Length of output: 2608
src/services/contact_baileys.ts (2)
1-1: Check imports for unused references.
SinceIncomingusage was removed, confirm that all imports are still necessary. Any unused imports increase maintenance overhead.✅ Verification successful
All imports are actively used in the codebase
TheContactimport and all other imports in this file are necessary and actively used. TheContactinterface is used in the class implementation, and there are no unused imports present.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Confirm that no references to `Incoming` remain in this file. rg 'Incoming' src/services/contact_baileys.tsLength of output: 45
Script:
#!/bin/bash # Check for Contact usage and get file contents echo "=== Searching for Contact usage ===" rg "Contact" src/services/contact_baileys.ts echo -e "\n=== File contents ===" cat src/services/contact_baileys.tsLength of output: 1229
13-13: Validate backward compatibility for constructor changes.
Removing theincomingparameter simplifies the constructor, but ensure that callers were updated accordingly. This might break internal or external code that expected the old signature.✅ Verification successful
Constructor changes are verified and safe
The removal of theincomingparameter is safe as the only usage ofContactBaileysconstructor instandalone.tsalready matches the new signature. No other code references were found that could be impacted by this change.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search all references to `ContactBaileys` constructor usage to confirm it matches the new signature. rg -A 3 'new ContactBaileys\(.*\)'Length of output: 246
Script:
#!/bin/bash # Check git history for the file git log -p src/services/contact_baileys.ts | grep -A 10 "constructor" # Search for type definitions and exports rg "export.*ContactBaileys" -A 3 # Search for test files fd "contact.*test" --type f fd "contact.*spec" --type fLength of output: 369
Script:
#!/bin/bash # Find Contact interface definition rg "interface Contact" -A 5 # Search for imports of ContactBaileys rg "import.*ContactBaileys" # Search for usage of Contact interface rg "Contact\s*[:{]" -A 3Length of output: 1647
src/services/reload_baileys.ts (1)
13-19: LGTM! Constructor changes align with removal ofincomingparameter.The constructor has been correctly updated to remove the
incomingparameter while maintaining essential dependencies.src/bridge.ts (1)
40-43: LGTM! Changes consistently remove theincomingparameter.The modifications correctly update the constructor calls and function parameters to align with the removal of the
incomingparameter across the codebase.Also applies to: 63-63
src/index.ts (1)
45-46: LGTM! Parameter removal aligns with PR objectives.The removal of the
incomingBaileysparameter fromReloadBaileys,LogoutBaileys, andautoConnectis consistent with the PR's goal of simplifying the architecture.Also applies to: 53-53
src/services/config_by_env.ts (1)
41-41: LGTM! Configuration changes are well-structured.The addition of
READ_WHEN_RECEIPTto the configuration is clean and follows the existing pattern.Also applies to: 73-73
__tests__/services/client_baileys.ts (1)
75-75: Consider adding tests for read receipt functionality.While the removal of the
incomingparameter is correct, the test file should be updated to include test cases for the new read receipt functionality.Consider adding the following test cases:
- Successful read receipt sending
- Failed read receipt sending
- Error handling scenarios
I can help generate these test cases if needed.
src/services/data_store_redis.ts (2)
85-88: LGTM! Improved JID handling logic.The change to use
isJidUserprovides more precise handling of user JIDs compared to the previous group-based check.
91-94: Verify consistent JID handling across the codebase.The JID handling logic has been updated. Let's verify this change is consistent with other JID transformations in the codebase.
✅ Verification successful
JID handling is consistent across the codebase ✅
The JID transformation logic is properly centralized in the transformer service and consistently used throughout the codebase, including data stores, client communications, and media handling. The changes maintain this consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other JID transformations that might need similar updates rg -A 2 "jidToPhoneNumber|phoneNumberToJid|isJidUser|isJidGroup" --type tsLength of output: 15438
src/standalone.ts (1)
68-76: LGTM! Well-structured read receipt implementation.The implementation correctly handles the read receipt functionality with proper debug logging and payload structure.
src/services/listener_baileys.ts (1)
99-101: LGTM! Well-implemented read receipt handling.The read receipt is correctly triggered only for incoming messages (
!i?.key?.fromMe).src/services/client_baileys.ts (3)
52-52: LGTM: Constructor changes align with PR objectives.The removal of the
incomingparameter from both the constructor call and signature is implemented consistently.Also applies to: 223-227
125-125: LGTM: Error handling refactored correctly.The error notification has been properly migrated from
this.incoming.sendtothis.sendMessage.
314-316: LGTM: Call rejection handling refactored correctly.The call rejection message has been properly migrated from
this.incoming.sendtothis.sendMessage.
| test('return messageType documentWithCaptionMessage', async () => { | ||
| const payload = {"key":{"remoteJid":"[email protected]","fromMe":false,"id":"3EB0AA0FA8CF9A5BD4D13D"},"messageTimestamp":1737746252,"pushName":"Emerson Casemiro","broadcast":false,"message":{"messageContextInfo":{"deviceListMetadata":{"senderKeyHash":"Cx7OGNKcbwqyqQ==","senderTimestamp":"1737203328","senderAccountType":"E2EE","receiverAccountType":"E2EE","recipientKeyHash":"S8o1LEWk33YRoA==","recipientTimestamp":"1737744851"},"deviceListMetadataVersion":2,"messageSecret":"+HGjutFitYLj4+gVC8F7S7CCM7fYeFhZM+SIdX0LYcM="},"documentWithCaptionMessage":{"message":{"documentMessage":{"url":"https://mmg.whatsapp.net/v/t62.7119-24/12161728_1172632827186041_246669122271751635_n.enc?ccb=11-4&oh=01_Q5AaIGXESRZeIDpQa_xrdaJw77EfYWwfgqkE6gf8fAXiD0-n&oe=66EDCD0C&_nc_sid=5e03e0&mms3=true","mimetype":"video/mp4","title":"video1981450644.mp4","fileSha256":"NW9GHqDFA8rZO6bFzeHxjFA7FSubZRkGHxGTz5CjqGA=","fileLength":"5740942","pageCount":0,"mediaKey":"tQoQcgWxymM12mmNlh3kcAgmOUo/lymVz3kPqcnFAGA=","fileName":"video1981450644.mp4","fileEncSha256":"KRf3aTRRWSp9gSclIu6iSjUk6ek6C4bFDCjBsrXQYSU=","directPath":"/v/t62.7119-24/32409775_1146339107002189_7733160102753112682_n.enc?ccb=11-4&oh=01_Q5AaIMPEs_9ILdnvkFSoZdM4zRZetoNGBM-a45BuI7vihuag&oe=67BB6FBC&_nc_sid=5e03e0&_nc_hot=1737746251","mediaKeyTimestamp":"1724272727","contactVcard":false,"caption":"video1981450644.mp4"},"messageContextInfo":{"messageSecret":"+HGjutFitYLj4+gVC8F7S7CCM7fYeFhZM+SIdX0LYcM="}}}},"profilePicture":"https://pps.whatsapp.net/v/t61.24694-24/389368984_315748354525689_7231214457442380254_n.jpg?stp=dst-jpg_s96x96_tt6&ccb=11-4&oh=01_Q5AaIGrFzQWqLHY6drciBsqcEvehTMtbjVVYtm3G7BwA4L51&oe=67A0FECA&_nc_sid=5e03e0&_nc_cat=106"} | ||
| expect(getMessageType(payload)).toEqual('documentWithCaptionMessage') |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize sensitive information in test data.
The test payload contains sensitive information such as phone numbers and URLs. Consider replacing them with mock data.
Apply this diff to sanitize the test data:
test('return messageType documentWithCaptionMessage', async () => {
- const payload = {"key":{"remoteJid":"[email protected]","fromMe":false,"id":"3EB0AA0FA8CF9A5BD4D13D"},"messageTimestamp":1737746252,"pushName":"Emerson Casemiro","broadcast":false,"message":{"messageContextInfo":{"deviceListMetadata":{"senderKeyHash":"Cx7OGNKcbwqyqQ==","senderTimestamp":"1737203328","senderAccountType":"E2EE","receiverAccountType":"E2EE","recipientKeyHash":"S8o1LEWk33YRoA==","recipientTimestamp":"1737744851"},"deviceListMetadataVersion":2,"messageSecret":"+HGjutFitYLj4+gVC8F7S7CCM7fYeFhZM+SIdX0LYcM="},"documentWithCaptionMessage":{"message":{"documentMessage":{"url":"https://mmg.whatsapp.net/v/t62.7119-24/12161728_1172632827186041_246669122271751635_n.enc?ccb=11-4&oh=01_Q5AaIGXESRZeIDpQa_xrdaJw77EfYWwfgqkE6gf8fAXiD0-n&oe=66EDCD0C&_nc_sid=5e03e0&mms3=true","mimetype":"video/mp4","title":"video1981450644.mp4","fileSha256":"NW9GHqDFA8rZO6bFzeHxjFA7FSubZRkGHxGTz5CjqGA=","fileLength":"5740942","pageCount":0,"mediaKey":"tQoQcgWxymM12mmNlh3kcAgmOUo/lymVz3kPqcnFAGA=","fileName":"video1981450644.mp4","fileEncSha256":"KRf3aTRRWSp9gSclIu6iSjUk6ek6C4bFDCjBsrXQYSU=","directPath":"/v/t62.7119-24/32409775_1146339107002189_7733160102753112682_n.enc?ccb=11-4&oh=01_Q5AaIMPEs_9ILdnvkFSoZdM4zRZetoNGBM-a45BuI7vihuag&oe=67BB6FBC&_nc_sid=5e03e0&_nc_hot=1737746251","mediaKeyTimestamp":"1724272727","contactVcard":false,"caption":"video1981450644.mp4"},"messageContextInfo":{"messageSecret":"+HGjutFitYLj4+gVC8F7S7CCM7fYeFhZM+SIdX0LYcM="}}}},"profilePicture":"https://pps.whatsapp.net/v/t61.24694-24/389368984_315748354525689_7231214457442380254_n.jpg?stp=dst-jpg_s96x96_tt6&ccb=11-4&oh=01_Q5AaIGrFzQWqLHY6drciBsqcEvehTMtbjVVYtm3G7BwA4L51&oe=67A0FECA&_nc_sid=5e03e0&_nc_cat=106"}
+ const payload = {
+ key: {
+ remoteJid: "[email protected]",
+ fromMe: false,
+ id: "mock-id"
+ },
+ message: {
+ documentWithCaptionMessage: {
+ message: {
+ documentMessage: {
+ url: "mock-url",
+ mimetype: "video/mp4",
+ title: "mock-video.mp4",
+ caption: "mock-caption"
+ }
+ }
+ }
+ }
+ }
expect(getMessageType(payload)).toEqual('documentWithCaptionMessage')
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('return messageType documentWithCaptionMessage', async () => { | |
| const payload = {"key":{"remoteJid":"[email protected]","fromMe":false,"id":"3EB0AA0FA8CF9A5BD4D13D"},"messageTimestamp":1737746252,"pushName":"Emerson Casemiro","broadcast":false,"message":{"messageContextInfo":{"deviceListMetadata":{"senderKeyHash":"Cx7OGNKcbwqyqQ==","senderTimestamp":"1737203328","senderAccountType":"E2EE","receiverAccountType":"E2EE","recipientKeyHash":"S8o1LEWk33YRoA==","recipientTimestamp":"1737744851"},"deviceListMetadataVersion":2,"messageSecret":"+HGjutFitYLj4+gVC8F7S7CCM7fYeFhZM+SIdX0LYcM="},"documentWithCaptionMessage":{"message":{"documentMessage":{"url":"https://mmg.whatsapp.net/v/t62.7119-24/12161728_1172632827186041_246669122271751635_n.enc?ccb=11-4&oh=01_Q5AaIGXESRZeIDpQa_xrdaJw77EfYWwfgqkE6gf8fAXiD0-n&oe=66EDCD0C&_nc_sid=5e03e0&mms3=true","mimetype":"video/mp4","title":"video1981450644.mp4","fileSha256":"NW9GHqDFA8rZO6bFzeHxjFA7FSubZRkGHxGTz5CjqGA=","fileLength":"5740942","pageCount":0,"mediaKey":"tQoQcgWxymM12mmNlh3kcAgmOUo/lymVz3kPqcnFAGA=","fileName":"video1981450644.mp4","fileEncSha256":"KRf3aTRRWSp9gSclIu6iSjUk6ek6C4bFDCjBsrXQYSU=","directPath":"/v/t62.7119-24/32409775_1146339107002189_7733160102753112682_n.enc?ccb=11-4&oh=01_Q5AaIMPEs_9ILdnvkFSoZdM4zRZetoNGBM-a45BuI7vihuag&oe=67BB6FBC&_nc_sid=5e03e0&_nc_hot=1737746251","mediaKeyTimestamp":"1724272727","contactVcard":false,"caption":"video1981450644.mp4"},"messageContextInfo":{"messageSecret":"+HGjutFitYLj4+gVC8F7S7CCM7fYeFhZM+SIdX0LYcM="}}}},"profilePicture":"https://pps.whatsapp.net/v/t61.24694-24/389368984_315748354525689_7231214457442380254_n.jpg?stp=dst-jpg_s96x96_tt6&ccb=11-4&oh=01_Q5AaIGrFzQWqLHY6drciBsqcEvehTMtbjVVYtm3G7BwA4L51&oe=67A0FECA&_nc_sid=5e03e0&_nc_cat=106"} | |
| expect(getMessageType(payload)).toEqual('documentWithCaptionMessage') | |
| test('return messageType documentWithCaptionMessage', async () => { | |
| const payload = { | |
| key: { | |
| remoteJid: "[email protected]", | |
| fromMe: false, | |
| id: "mock-id" | |
| }, | |
| message: { | |
| documentWithCaptionMessage: { | |
| message: { | |
| documentMessage: { | |
| url: "mock-url", | |
| mimetype: "video/mp4", | |
| title: "mock-video.mp4", | |
| caption: "mock-caption" | |
| } | |
| } | |
| } | |
| } | |
| } | |
| expect(getMessageType(payload)).toEqual('documentWithCaptionMessage') | |
| }) |
| export class ReadWhenReceiptEnabled implements ReadWhenReceipt { | ||
| private incoming: Incoming | ||
|
|
||
| constructor(incoming: Incoming) { | ||
| this.incoming = incoming | ||
| } | ||
|
|
||
| test() {} | ||
|
|
||
| async send(phone, from, messageId){ | ||
| logger.debug('Send reading phone: %s from: %s message: %s', phone, from, messageId) | ||
| const payload = { | ||
| messaging_product: 'whatsapp', | ||
| status: 'read', | ||
| message_id: messageId | ||
| } | ||
| await this.incoming.send(from, payload, {}) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure robust error handling in ReadWhenReceiptEnabled class.
When calling this.incoming.send, consider adding minimal error handling (e.g., logging error messages). This helps avoid silent failures if this.incoming.send rejects.
async send(phone, from, messageId) {
logger.debug('Send reading phone: %s from: %s message: %s', phone, from, messageId)
const payload = {
messaging_product: 'whatsapp',
status: 'read',
message_id: messageId
}
- await this.incoming.send(from, payload, {})
+ try {
+ await this.incoming.send(from, payload, {})
+ } catch (error) {
+ logger.error('Failed to send read receipt: %s', error)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export class ReadWhenReceiptEnabled implements ReadWhenReceipt { | |
| private incoming: Incoming | |
| constructor(incoming: Incoming) { | |
| this.incoming = incoming | |
| } | |
| test() {} | |
| async send(phone, from, messageId){ | |
| logger.debug('Send reading phone: %s from: %s message: %s', phone, from, messageId) | |
| const payload = { | |
| messaging_product: 'whatsapp', | |
| status: 'read', | |
| message_id: messageId | |
| } | |
| await this.incoming.send(from, payload, {}) | |
| } | |
| } | |
| export class ReadWhenReceiptEnabled implements ReadWhenReceipt { | |
| private incoming: Incoming | |
| constructor(incoming: Incoming) { | |
| this.incoming = incoming | |
| } | |
| test() {} | |
| async send(phone, from, messageId){ | |
| logger.debug('Send reading phone: %s from: %s message: %s', phone, from, messageId) | |
| const payload = { | |
| messaging_product: 'whatsapp', | |
| status: 'read', | |
| message_id: messageId | |
| } | |
| try { | |
| await this.incoming.send(from, payload, {}) | |
| } catch (error) { | |
| logger.error('Failed to send read receipt: %s', error) | |
| } | |
| } | |
| } |
| export const getReadWhenReceipt = (phone, config, incoming): ReadWhenReceipt => { | ||
| const instance = instances.get(phone)! | ||
| try { | ||
| instance.test() | ||
| } catch (e) { | ||
| if (e instanceof TypeError && e.message == "Cannot read properties of undefined (reading 'test')") { | ||
| const i: ReadWhenReceipt = config.readWhenReceipt ? new ReadWhenReceiptEnabled(incoming) : readWhenReceiptDisabled | ||
| instances.set(phone, i) | ||
| } else { | ||
| throw e | ||
| } | ||
| } | ||
| return instances.get(phone)! | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a straightforward existence check instead of exception-based logic.
Catching a TypeError to detect an uninitialized instance is potentially fragile if the error message changes or if other TypeErrors occur. Consider checking if (!instance) directly before trying instance.test().
export const getReadWhenReceipt = (phone, config, incoming): ReadWhenReceipt => {
const instance = instances.get(phone)!
- try {
- instance.test()
- } catch (e) {
- if (e instanceof TypeError && e.message == "Cannot read properties of undefined (reading 'test')") {
- const i: ReadWhenReceipt = config.readWhenReceipt ? new ReadWhenReceiptEnabled(incoming) : readWhenReceiptDisabled
- instances.set(phone, i)
- } else {
- throw e
- }
+ if (!instance) {
+ const i: ReadWhenReceipt = config.readWhenReceipt
+ ? new ReadWhenReceiptEnabled(incoming)
+ : readWhenReceiptDisabled
+ instances.set(phone, i)
}
return instances.get(phone)!
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getReadWhenReceipt = (phone, config, incoming): ReadWhenReceipt => { | |
| const instance = instances.get(phone)! | |
| try { | |
| instance.test() | |
| } catch (e) { | |
| if (e instanceof TypeError && e.message == "Cannot read properties of undefined (reading 'test')") { | |
| const i: ReadWhenReceipt = config.readWhenReceipt ? new ReadWhenReceiptEnabled(incoming) : readWhenReceiptDisabled | |
| instances.set(phone, i) | |
| } else { | |
| throw e | |
| } | |
| } | |
| return instances.get(phone)! | |
| } | |
| export const getReadWhenReceipt = (phone, config, incoming): ReadWhenReceipt => { | |
| const instance = instances.get(phone)! | |
| if (!instance) { | |
| const i: ReadWhenReceipt = config.readWhenReceipt | |
| ? new ReadWhenReceiptEnabled(incoming) | |
| : readWhenReceiptDisabled | |
| instances.set(phone, i) | |
| } | |
| return instances.get(phone)! | |
| } |
| import logger from './logger'; | ||
| import { Outgoing } from './outgoing' | ||
|
|
||
| export const intances: Map<string, Intance> = new Map() |
There was a problem hiding this comment.
Fix typo in Map variable name.
The variable name intances has a typo and should be instances.
-export const intances: Map<string, Intance> = new Map()
+export const instances: Map<string, Instance> = new Map()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const intances: Map<string, Intance> = new Map() | |
| export const instances: Map<string, Instance> = new Map() |
| export const getIntance = async <T>(phone: string, env: Env<T>): Promise<Intance> => { | ||
| if (!intances.has(phone)) { | ||
| logger.info('Creating instance %s', phone) | ||
| const getConfigVar: getConfig = env.REDIS_URL ? getConfigRedis : getConfigByEnv | ||
|
|
||
| } else { | ||
| logger.debug('Retrieving intances %s', phone) | ||
| } | ||
| return intances.get(phone)! | ||
| } |
There was a problem hiding this comment.
Complete the implementation of getIntance function.
The function is incomplete:
- It doesn't create a new instance when one doesn't exist
- It has a typo in its name
- The logging message has a typo
- It's missing error handling for non-existent instances
-export const getIntance = async <T>(phone: string, env: Env<T>): Promise<Intance> => {
+export const getInstance = async <T>(phone: string, env: Env<T>): Promise<Instance> => {
if (!instances.has(phone)) {
logger.info('Creating instance %s', phone)
const getConfigVar: getConfig = env.REDIS_URL ? getConfigRedis : getConfigByEnv
+ const config = await getConfigVar(phone)
+ const instance = new Instance(env, config)
+ instances.set(phone, instance)
} else {
- logger.debug('Retrieving intances %s', phone)
+ logger.debug('Retrieving instance %s', phone)
}
- return intances.get(phone)!
+ const instance = instances.get(phone)
+ if (!instance) {
+ throw new Error(`Instance not found for phone ${phone}`)
+ }
+ return instance
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getIntance = async <T>(phone: string, env: Env<T>): Promise<Intance> => { | |
| if (!intances.has(phone)) { | |
| logger.info('Creating instance %s', phone) | |
| const getConfigVar: getConfig = env.REDIS_URL ? getConfigRedis : getConfigByEnv | |
| } else { | |
| logger.debug('Retrieving intances %s', phone) | |
| } | |
| return intances.get(phone)! | |
| } | |
| export const getInstance = async <T>(phone: string, env: Env<T>): Promise<Instance> => { | |
| if (!instances.has(phone)) { | |
| logger.info('Creating instance %s', phone) | |
| const getConfigVar: getConfig = env.REDIS_URL ? getConfigRedis : getConfigByEnv | |
| const config = await getConfigVar(phone) | |
| const instance = new Instance(env, config) | |
| instances.set(phone, instance) | |
| } else { | |
| logger.debug('Retrieving instance %s', phone) | |
| } | |
| const instance = instances.get(phone) | |
| if (!instance) { | |
| throw new Error(`Instance not found for phone ${phone}`) | |
| } | |
| return instance | |
| } |
| @@ -39,7 +37,7 @@ export const autoConnect = async ( | |||
| logger.info(`Update session status to auto connect ${phone}...`) | |||
| await sessionStore.setStatus(phone, 'offline') | |||
| } | |||
| getClient({ phone, incoming, listener, getConfig, onNewLogin }) | |||
| getClient({ phone, listener, getConfig, onNewLogin }) | |||
There was a problem hiding this comment.
Fix missing await for async function call.
The getClient function is asynchronous but its result is not being awaited. This could lead to race conditions or unexpected behavior.
- getClient({ phone, listener, getConfig, onNewLogin })
+ await getClient({ phone, listener, getConfig, onNewLogin })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| getClient({ phone, listener, getConfig, onNewLogin }) | |
| await getClient({ phone, listener, getConfig, onNewLogin }) |
| const readWhenReceipt: ReadWhenReceipt = async (from, messageId) => { | ||
| console.log('>>>>>>>>>>>>>>>>>>>>>>>>>>') | ||
| const payload = { | ||
| messaging_product: 'whatsapp', | ||
| status: 'read', | ||
| message_id: messageId | ||
| } | ||
| await incomingBaileys.send(from, payload, {}) | ||
| } |
There was a problem hiding this comment.
Remove debug log and add error handling.
The implementation has several issues that need to be addressed:
- Debug console.log statement should be removed
- Using
incomingBaileysbefore it's defined could cause runtime issues - Missing error handling for the async operation
Apply this diff to fix the issues:
const readWhenReceipt: ReadWhenReceipt = async (from, messageId) => {
- console.log('>>>>>>>>>>>>>>>>>>>>>>>>>>')
const payload = {
messaging_product: 'whatsapp',
status: 'read',
message_id: messageId
}
- await incomingBaileys.send(from, payload, {})
+ try {
+ await incomingBaileys.send(from, payload, {})
+ } catch (error) {
+ logger.error('Failed to send read receipt', { error, from, messageId })
+ throw error
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const readWhenReceipt: ReadWhenReceipt = async (from, messageId) => { | |
| console.log('>>>>>>>>>>>>>>>>>>>>>>>>>>') | |
| const payload = { | |
| messaging_product: 'whatsapp', | |
| status: 'read', | |
| message_id: messageId | |
| } | |
| await incomingBaileys.send(from, payload, {}) | |
| } | |
| const readWhenReceipt: ReadWhenReceipt = async (from, messageId) => { | |
| const payload = { | |
| messaging_product: 'whatsapp', | |
| status: 'read', | |
| message_id: messageId | |
| } | |
| try { | |
| await incomingBaileys.send(from, payload, {}) | |
| } catch (error) { | |
| logger.error('Failed to send read receipt', { error, from, messageId }) | |
| throw error | |
| } | |
| } |
|
|
||
| const outgoingAmqp: Outgoing = new OutgoingAmqp(getConfigRedis) | ||
| const listenerAmqp: Listener = new ListenerAmqp() | ||
| const broadcastAmqp: Broadcast = new BroadcastAmqp() | ||
| const listenerBaileys: Listener = new ListenerBaileys(outgoingAmqp, broadcastAmqp, getConfigRedis) | ||
|
|
||
| const getConfig: getConfig = getConfigRedis |
There was a problem hiding this comment.
Rename redeclared getConfig variable.
The static analysis tool correctly flags that getConfig is being redeclared. This could lead to confusion.
Apply this diff to fix the issue:
-const getConfig: getConfig = getConfigRedis
+const configGetter: getConfig = getConfigRedis📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getConfig: getConfig = getConfigRedis | |
| const configGetter: getConfig = getConfigRedis |
🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Shouldn't redeclare 'getConfig'. Consider to delete it or rename it.
'getConfig' is defined here:
(lint/suspicious/noRedeclare)
| const readWhenReceipt: ReadWhenReceipt = async (from, messageId) => { | ||
| const payload = { | ||
| messaging_product: 'whatsapp', | ||
| status: 'read', | ||
| message_id: messageId | ||
| } | ||
| await incomingBaileys.send(from, payload, {}) | ||
| } |
There was a problem hiding this comment.
Fix read receipt implementation.
The implementation has the same issues as in index.ts:
- No error handling for the async operation
- Using
incomingBaileysbefore it's defined
Apply this diff to fix the issues:
const readWhenReceipt: ReadWhenReceipt = async (from, messageId) => {
const payload = {
messaging_product: 'whatsapp',
status: 'read',
message_id: messageId
}
- await incomingBaileys.send(from, payload, {})
+ try {
+ await incomingBaileys.send(from, payload, {})
+ } catch (error) {
+ logger.error('Failed to send read receipt', { error, from, messageId })
+ throw error
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const readWhenReceipt: ReadWhenReceipt = async (from, messageId) => { | |
| const payload = { | |
| messaging_product: 'whatsapp', | |
| status: 'read', | |
| message_id: messageId | |
| } | |
| await incomingBaileys.send(from, payload, {}) | |
| } | |
| const readWhenReceipt: ReadWhenReceipt = async (from, messageId) => { | |
| const payload = { | |
| messaging_product: 'whatsapp', | |
| status: 'read', | |
| message_id: messageId | |
| } | |
| try { | |
| await incomingBaileys.send(from, payload, {}) | |
| } catch (error) { | |
| logger.error('Failed to send read receipt', { error, from, messageId }) | |
| throw error | |
| } | |
| } |
| @@ -360,13 +351,15 @@ export class ClientBaileys implements Client { | |||
| async send(payload: any, options: any = {}) { | |||
| const { status, type, to } = payload | |||
| try { | |||
| console.log('!!!!!!!!!!!') | |||
There was a problem hiding this comment.
Remove debug console.log statements.
These debug statements should be removed:
- They use
console.loginstead of the establishedloggerutility - They contain non-descriptive content ('!!!!!!!!!!!')
- They may expose sensitive information (message IDs)
Apply this diff to remove the debug statements:
- console.log('!!!!!!!!!!!')
if (status) {
if (['sent', 'delivered', 'failed', 'progress', 'read'].includes(status)) {
if (status == 'read') {
const currentStatus = await this.store?.dataStore?.loadStatus(payload?.message_id)
if (currentStatus != status) {
const key = await this.store?.dataStore?.loadKey(payload?.message_id)
logger.debug('key %s for %s', JSON.stringify(key), payload?.message_id)
- console.log(payload?.message_id, key)
if (key?.id) {Also applies to: 362-362
clairton
force-pushed
the
feat/read-on-receipt
branch
from
d556fab to
f27c2ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/jobs/bind_bridge.ts (1)
30-30:⚠️ Potential issueFix variable redeclaration.
The variable
getConfigis being redeclared, which could lead to confusion.Apply this diff to fix the issue:
-const getConfig: getConfig = getConfigRedis +const configGetter: getConfig = getConfigRedis🧰 Tools
🪛 Biome (1.9.4)
[error] 30-30: Shouldn't redeclare 'getConfig'. Consider to delete it or rename it.
'getConfig' is defined here:
(lint/suspicious/noRedeclare)
src/services/client_baileys.ts (1)
354-354:⚠️ Potential issueRemove debug console.log statements.
These debug statements should be removed:
- They use
console.loginstead of the establishedloggerutility- They contain non-descriptive content ('!!!!!!!!!!!')
- They may expose sensitive information (message IDs)
Apply this diff to remove the debug statements:
- console.log('!!!!!!!!!!!') if (status) { if (['sent', 'delivered', 'failed', 'progress', 'read'].includes(status)) { if (status == 'read') { const currentStatus = await this.store?.dataStore?.loadStatus(payload?.message_id) if (currentStatus != status) { const key = await this.store?.dataStore?.loadKey(payload?.message_id) logger.debug('key %s for %s', JSON.stringify(key), payload?.message_id) - console.log(payload?.message_id, key) if (key?.id) {Also applies to: 362-362
🧹 Nitpick comments (1)
src/standalone.ts (1)
159-162: Improve unhandledRejection error handling.The current implementation removes the stack trace and doesn't rethrow the error, which could make debugging more difficult.
Apply this diff to improve error handling:
-process.on('unhandledRejection', (args) => { - // logger.error('unhandledRejection: %s', reason.stack) - logger.error('promise: %s', JSON.stringify(args)) - // throw reason +process.on('unhandledRejection', (reason, promise) => { + logger.error('Unhandled Rejection at:', { + promise, + reason: reason instanceof Error ? { + message: reason.message, + stack: reason.stack + } : reason + }) + // Optional: Exit process with error + process.exit(1) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
__tests__/services/client_baileys.ts(1 hunks)__tests__/services/incoming_baileys.ts(0 hunks)__tests__/services/transformer.ts(1 hunks)src/bridge.ts(2 hunks)src/defaults.ts(2 hunks)src/index.ts(1 hunks)src/jobs/bind_bridge.ts(1 hunks)src/services/auto_connect.ts(1 hunks)src/services/client.ts(0 hunks)src/services/client_baileys.ts(5 hunks)src/services/config.ts(2 hunks)src/services/config_by_env.ts(2 hunks)src/services/contact_baileys.ts(1 hunks)src/services/incoming_baileys.ts(0 hunks)src/services/instance.ts(1 hunks)src/services/listener_baileys.ts(4 hunks)src/services/logout_baileys.ts(1 hunks)src/services/read_when_receipt.ts(1 hunks)src/services/redis.ts(2 hunks)src/services/reload_baileys.ts(1 hunks)src/standalone.ts(5 hunks)
💤 Files with no reviewable changes (3)
- src/services/client.ts
- src/services/incoming_baileys.ts
- tests/services/incoming_baileys.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- src/defaults.ts
- tests/services/client_baileys.ts
- src/services/redis.ts
- src/services/contact_baileys.ts
- src/services/auto_connect.ts
- src/services/logout_baileys.ts
- src/services/config.ts
- src/services/instance.ts
- src/services/reload_baileys.ts
- src/services/config_by_env.ts
- src/services/listener_baileys.ts
- tests/services/transformer.ts
- src/index.ts
- src/services/read_when_receipt.ts
- src/bridge.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/jobs/bind_bridge.ts
[error] 30-30: Shouldn't redeclare 'getConfig'. Consider to delete it or rename it.
'getConfig' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (3)
src/jobs/bind_bridge.ts (2)
40-40: LGTM: Proper initialization of ListenerBaileys.The initialization includes all required parameters including the new readWhenReceipt function.
31-38:⚠️ Potential issueFix potential race condition in readWhenReceipt implementation.
The implementation has two issues:
- Using
incomingBaileysbefore it's defined (line 37)- No error handling for the async operation
Apply this diff to fix the issues:
const readWhenReceipt: ReadWhenReceipt = async (from, messageId) => { const payload = { messaging_product: 'whatsapp', status: 'read', message_id: messageId } - await incomingBaileys.send(from, payload, {}) + try { + await incomingBaileys.send(from, payload, {}) + } catch (error) { + logger.error('Failed to send read receipt', { error, from, messageId }) + throw error + } }Likely invalid or redundant comment.
src/services/client_baileys.ts (1)
223-223: LGTM: Simplified constructor signature.The constructor has been correctly simplified by removing the unused
incomingparameter.
| const readWhenReceipt: ReadWhenReceipt = !READ_WHEN_RECEIPT ? readWhenReceiptDefault : async (phone, from, messageId) => { | ||
| logger.debug('Send reading phone: %s from: %s message: %s', phone, from, messageId) | ||
| const payload = { | ||
| messaging_product: 'whatsapp', | ||
| status: 'read', | ||
| message_id: messageId | ||
| } | ||
| await incoming.send(from, payload, {}) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in readWhenReceipt implementation.
The implementation needs better error handling and logging.
Apply this diff to improve error handling:
const readWhenReceipt: ReadWhenReceipt = !READ_WHEN_RECEIPT ? readWhenReceiptDefault : async (phone, from, messageId) => {
logger.debug('Send reading phone: %s from: %s message: %s', phone, from, messageId)
const payload = {
messaging_product: 'whatsapp',
status: 'read',
message_id: messageId
}
- await incoming.send(from, payload, {})
+ try {
+ await incoming.send(from, payload, {})
+ } catch (error) {
+ logger.error('Failed to send read receipt', { error, phone, from, messageId })
+ throw error
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const readWhenReceipt: ReadWhenReceipt = !READ_WHEN_RECEIPT ? readWhenReceiptDefault : async (phone, from, messageId) => { | |
| logger.debug('Send reading phone: %s from: %s message: %s', phone, from, messageId) | |
| const payload = { | |
| messaging_product: 'whatsapp', | |
| status: 'read', | |
| message_id: messageId | |
| } | |
| await incoming.send(from, payload, {}) | |
| } | |
| const readWhenReceipt: ReadWhenReceipt = !READ_WHEN_RECEIPT ? readWhenReceiptDefault : async (phone, from, messageId) => { | |
| logger.debug('Send reading phone: %s from: %s message: %s', phone, from, messageId) | |
| const payload = { | |
| messaging_product: 'whatsapp', | |
| status: 'read', | |
| message_id: messageId | |
| } | |
| try { | |
| await incoming.send(from, payload, {}) | |
| } catch (error) { | |
| logger.error('Failed to send read receipt', { error, phone, from, messageId }) | |
| throw error | |
| } | |
| } |
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
readWhenReceiptto control message read statusImprovements
incomingparameterConfiguration
VALIDATE_MEDIA_LINK_BEFORE_SENDandREAD_WHEN_RECEIPTconfiguration optionsTechnical Enhancements