feat: add message update processing in IncomingMessageBaileysService #20
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change introduces several new methods to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. 🔧 RuboCop (1.73)spec/controllers/webhooks/whatsapp_controller_spec.rbError: app/controllers/webhooks/whatsapp_controller.rbError: 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/services/whatsapp/incoming_message_baileys_service.rb (2)
48-54: Implementation follows service pattern, but consider a few improvements.The
process_messages_updatemethod correctly follows the service's pattern of processing batches of updates. However, consider these improvements:
- Add a nil/empty check for
processed_params[:data]similar to line 6 to avoid errors with empty updates- Reset instance variables before each iteration (similar to lines 40-43 in
process_messages_upsert)- Add debug logging for visibility, especially during the initial implementation phases
def process_messages_update + updates = processed_params[:data] || [] - updates = processed_params[:data] + Rails.logger.debug "Processing #{updates.length} message updates" updates.each do |update| + @message = nil + @contact_inbox = nil + @contact = nil @raw_update = update handle_update end end
56-56: Add documentation for empty placeholder method.The
handle_updatemethod is currently empty, which is fine for an initial implementation. However, consider adding a TODO comment to document the intended functionality and implementation timeline.-def handle_update; end +# TODO: Implement message update handling logic to process status updates +# Expected implementation in PR #XX +def handle_update; end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/services/whatsapp/incoming_message_baileys_service.rb(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
app/controllers/api/v1/accounts/inboxes_controller.rb (1)
update(44-48)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/services/whatsapp/incoming_message_baileys_service.rb (4)
48-54: Validateupdatesstructure.Although this works when
processed_params[:data]is an array of updates, it may raise errors or silently do nothing if the data structure differs or isnil. Consider verifying it's an array before iterating, or handling the potentialnilscenario more gracefully.
56-61: Consider wrapping updates in a transaction.Calls to
update_statusandupdate_message_contentmodify the same message. If you need to preserve data integrity when multiple updates arrive nearly simultaneously, consider wrapping these calls in a database transaction to ensure atomic consistency.
63-67: Consider logging a warning for missing messages.Currently, when
valid_update_message?returns false, the method simply returns. For better visibility, consider logging a warning when a message ID is present but the corresponding record cannot be found (e.g.,@message_updateisnil).
82-97: Return a placeholder for unsupported statuses.Currently, an unsupported status returns
nil. If you want to avoid storing partial data, consider returning'unknown'or a similar placeholder. This would help distinguish between an “unrecognized” status and “no status at all.”def status_handler status = @raw_update.dig(:update, :status) case status when 0 'failed' when 2 'sent' when 3 'delivered' when 4 'read' else Rails.logger.warn "Baileys unsupported message update status: #{status}" - nil + 'unknown' end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/services/whatsapp/incoming_message_baileys_service.rb(1 hunks)
🔇 Additional comments (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
69-73: Status update logic looks good.This logic is straightforward, updating the message status only if a valid status is returned. The approach is consistent with the existing code style and should work as intended.
…n IncomingMessageBaileysService
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
70-81: Guard against a nilmessage.As previously flagged, referencing
message[:conversation]andmessage.dig(...)may raise an error ifmessageisnil. You may want to return early or log a warning in that scenario.
🧹 Nitpick comments (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
57-63: Wrap status and content updates in a transaction if atomicity is required.
If a failure occurs inupdate_statusorupdate_message_content, you might leave the record in a partial state. Consider wrapping these calls in a transaction if consistent updates are necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/services/whatsapp/incoming_message_baileys_service.rb(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/services/whatsapp/incoming_message_baileys_service.rb (3)
app/javascript/dashboard/store/modules/conversations/index.js (1)
updates(222-222)app/controllers/api/v1/accounts/inboxes_controller.rb (1)
update(44-48)app/models/message.rb (1)
content(184-189)
🔇 Additional comments (3)
app/services/whatsapp/incoming_message_baileys_service.rb (3)
3-3: Prefer dedicated error file or naming.
DefiningMessageNotFoundErrorinline is acceptable if it’s strictly scoped to this service. If you foresee reusing it elsewhere, consider extracting it to its own file or renaming it more generically.
49-55: Consider input validation and fallback logic for updates.
processed_params[:data]is iterated without additional checks; ensure it's always an array. You might safely skip or log when encountering unexpected or malformed updates.Do you want to verify if there are any calls passing invalid or empty arrays? I can generate a script to grep for references to
process_messages_updateusage across the codebase.
64-68: Validation approach looks sound.
Thevalid_update_message?method effectively ensures that only updates with an existing message are processed.
| def status_handler | ||
| # https://github.com/WhiskeySockets/Baileys/blob/v6.7.16/WAProto/index.d.ts#L36694 | ||
| status = @raw_update.dig(:update, :status) | ||
| case status | ||
| when 0 | ||
| 'failed' | ||
| when 2 | ||
| 'sent' | ||
| when 3 | ||
| 'delivered' | ||
| when 4 | ||
| 'read' | ||
| else | ||
| Rails.logger.warn "Baileys unsupported message update status: #{status}" | ||
| end | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm omission of status=1.
Baileys might define a code for status=1. If that’s relevant, consider handling it here rather than logging an unsupported status.
🌐 Web query:
Does Baileys define a status=1 for message updates, and if so, what does it represent?
💡 Result:
Baileys does define a status=1 for message updates, which corresponds to the initial acknowledgment that a message has been sent from the client but has not yet been received by the WhatsApp server. This is part of a three-stage status system observed in WhatsApp's messaging protocol:
- Status 1: Indicates the message has been successfully sent from the client device (your Baileys implementation) but hasn't been acknowledged by WhatsApp's servers yet[1][3].
- Status 2 (when properly implemented): Represents server receipt confirmation[1][3].
- Status 3: Final delivery confirmation to the recipient's device[1][3].
The messages.update event in Baileys emits these status changes through its WebSocket connection. However, some users have reported implementations where Status 2 (server receipt) is occasionally skipped, particularly when using certain Baileys configurations[1][5]. Proper state management through events like creds.update and careful session handling are required to ensure reliable status tracking[3][5][7].
Citations:
- 1: [BUG] messages.update dont listen to status 2 WhiskeySockets/Baileys#620
- 2: https://baileys.wiki/docs/socket/receiving-updates
- 3: https://pokoke-01.github.io/owh/index.html
- 4: https://github.com/andresayac/bot-wa-baileys
- 5: https://www.npmjs.com/package/baileys
- 6: https://eurovision.tv/about/rules
- 7: https://baileys.wiki/docs/intro/
- 8: https://publications.jrc.ec.europa.eu/repository/bitstream/JRC121476/jrc121476_jrc_commonshareholding_final_1.pdf
- 9: https://www.americanrevolutioninstitute.org/masterpieces-in-detail/american-officers-sword-made-by-john-bailey/
- 10: https://dnr.maryland.gov/criticalarea/Documents/chesapeakenatives.pdf
Action Required: Address missing handling for status=1.
Our web query confirms that Baileys defines a status=1 for message updates, corresponding to the initial acknowledgment that 8000 a message has been sent from the client but not yet confirmed by WhatsApp's servers. If this state is relevant for your workflow, consider adding an explicit branch for it (for example, mapping it to "pending" or another appropriate status) rather than allowing it to fall into the default warning case. If the omission is intentional, please document the rationale.
- File:
app/services/whatsapp/incoming_message_baileys_service.rb - Suggested update:
case status when 0 'failed' when 1 'pending' # Message sent from client, awaiting server acknowledgement when 2 'sent' when 3 'delivered' when 4 'read' else Rails.logger.warn "Baileys unsupported message update status: #{status}" end
There was a problem hiding this comment.
Message status in Chatwoot supports only four states. The statuses "PENDING" and "PLAYED" are specific to Baileys and aren't recognized by Chatwoot.
Code snippet (i):
# Chatwoot
# app/models/message.rb:97
enum status: { sent: 0, delivered: 1, read: 2, failed: 3 }Code snippet (ii):
// Baileys
// @whiskeysockets/baileys/WAProto/index.d.ts:36694
enum Status {
ERROR = 0,
PENDING = 1,
SERVER_ACK = 2,
DELIVERY_ACK = 3,
READ = 4,
PLAYED = 5
}There was a problem hiding this comment.
@CayoPOliveira Thank you for clarifying the status mapping constraints. The current implementation correctly handles the four status states supported by Chatwoot and logs warnings for unsupported Baileys states.
I noticed that in the diff, the method is named status_handler but appears as status_mapper in the full file content. Was this an intentional rename? The status_mapper name seems more descriptive of the function's purpose.
✏️ Learnings added
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#20
File: app/services/whatsapp/incoming_message_baileys_service.rb:0-0
Timestamp: 2025-04-08T21:26:25.028Z
Learning: Chatwoot message model (app/models/message.rb) only supports four status states: sent (0), delivered (1), read (2), and failed (3). When mapping from Baileys WhatsApp statuses, only ERROR (0), SERVER_ACK (2), DELIVERY_ACK (3), and READ (4) can be mapped to Chatwoot's statuses. Baileys statuses like PENDING (1) and PLAYED (5) don't have Chatwoot equivalents and should be handled as special cases or logged as unsupported.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/services/whatsapp/incoming_message_baileys_service_spec.rb (2)
357-369: Consider testing more status code mappings.Currently, only status code 3 (delivered) is tested. Consider adding tests for other status codes to ensure the
status_mappermethod works correctly for all possible status values.it 'updates the message status' do update_payload = { key: { id: message_id }, update: { status: 3 } } # delivered params = { webhookVerifyToken: webhook_verify_token, event: 'messages.update', data: [update_payload] } described_class.new(inbox: inbox, params: params).perform message.reload expect(message.status).to eq('delivered') end +it 'updates the message status to seen when status is 4' do + update_payload = { key: { id: message_id }, update: { status: 4 } } # seen + params = { + webhookVerifyToken: webhook_verify_token, + event: 'messages.update', + data: [update_payload] + } + + described_class.new(inbox: inbox, params: params).perform + message.reload + + expect(message.status).to eq('read') +end + +it 'logs a warning for unsupported status codes' do + update_payload = { key: { id: message_id }, update: { status: 999 } } # unsupported + params = { + webhookVerifyToken: webhook_verify_token, + event: 'messages.update', + data: [update_payload] + } + + allow(Rails.logger).to receive(:warn).with(/Baileys unsupported status code: 999/) + + described_class.new(inbox: inbox, params: params).perform + + expect(Rails.logger).to have_received(:warn) +end
371-388: Add a test for simultaneous status and content updates.The implementation likely supports updating both status and content in a single update event, but this isn't tested. Consider adding a test case for this scenario to ensure the functionality works correctly.
it 'updates the message content' do update_payload = { key: { id: message_id }, update: { message: { editedMessage: { message: { conversation: 'New message content' } } } } } params = { webhookVerifyToken: webhook_verify_token, event: 'messages.update', data: [update_payload] } described_class.new(inbox: inbox, params: params).perform message.reload expect(message.content).to eq('New message content') end + +it 'updates both message status and content in a single update' do + update_payload = { + key: { id: message_id }, + update: { + status: 3, # delivered + message: { editedMessage: { message: { conversation: 'Updated content' } } } + } + } + params = { + webhookVerifyToken: webhook_verify_token, + event: 'messages.update', + data: [update_payload] + } + + described_class.new(inbox: inbox, params: params).perform + message.reload + + expect(message.status).to eq('delivered') + expect(message.content).to eq('Updated content') +end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/services/whatsapp/incoming_message_baileys_service.rb(2 hunks)spec/services/whatsapp/incoming_message_baileys_service_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/services/whatsapp/incoming_message_baileys_service.rb
🔇 Additional comments (2)
spec/services/whatsapp/incoming_message_baileys_service_spec.rb (2)
324-390: Well-designed tests for messages.update event handling.The tests cover essential functionality for message update processing, including error handling for non-existent messages and successful updating of both message status and content. This will ensure that the IncomingMessageBaileysService properly implements message update functionality.
343-346:❓ Verification inconclusive
Verify exception behavior for batch updates.
The current test confirms an exception is raised when a message isn't found, but doesn't verify what happens to other messages in the batch. Does the processing stop entirely, or continue with other messages?
Consider investigating how multiple updates in the data array are handled when one fails:
🏁 Script executed:
#!/bin/bash # Search for the implementation of process_messages_update method ast-grep --pattern "def process_messages_update" -A 30Length of output: 56
Action Required: Verify Batch Update Behavior
The current test only confirms that a
MessageNotFoundErroris raised when a single message is missing. However, it doesn’t clarify what happens to the remaining messages in a batch update. We couldn’t locate any implementation (e.g. aprocess_messages_updatemethod) that explicitly handles batch processing in this context. Please manually verify whether processing stops entirely when a message isn’t found or if the system continues to process the rest of the messages. It would be beneficial to add tests to confirm the intended behavior under batch update scenarios.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @coderabbitai[bot] and @gabrieljablonski)
| def status_handler | ||
| # https://github.com/WhiskeySockets/Baileys/blob/v6.7.16/WAProto/index.d.ts#L36694 | ||
| status = @raw_update.dig(:update, :status) | ||
| case status | ||
| when 0 | ||
| 'failed' | ||
| when 2 | ||
| 'sent' | ||
| when 3 | ||
| 'delivered' | ||
| when 4 | ||
| 'read' | ||
| else | ||
| Rails.logger.warn "Baileys unsupported message update status: #{status}" | ||
| end | ||
| end |
There was a problem hiding this comment.
Message status in Chatwoot supports only four states. The statuses "PENDING" and "PLAYED" are specific to Baileys and aren't recognized by Chatwoot.
Code snippet (i):
# Chatwoot
# app/models/message.rb:97
enum status: { sent: 0, delivered: 1, read: 2, failed: 3 }Code snippet (ii):
// Baileys
// @whiskeysockets/baileys/WAProto/index.d.ts:36694
enum Status {
ERROR = 0,
PENDING = 1,
SERVER_ACK = 2,
DELIVERY_ACK = 3,
READ = 4,
PLAYED = 5
}There was a problem hiding this comment.
-work-in-progress +(review needed)
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @gabrieljablonski)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @CayoPOliveira)
app/services/whatsapp/incoming_message_baileys_service.rb line 47 at r4 (raw file):
handle_message end end
WDYT of grouping related methods closer together:
process_* method (e.g. process_messages_upsert) -> related methods (e.g. handle_message, set_contact, ...)
And repeat for each process_* method.
app/services/whatsapp/incoming_message_baileys_service.rb line 52 at r4 (raw file):
updates = processed_params[:data] updates.each do |update| @raw_update = update
Why not reuse @raw_message (also @message_update -> @message)?
app/services/whatsapp/incoming_message_baileys_service.rb line 52 at r4 (raw file):
updates = processed_params[:data] updates.each do |update| @raw_update = update
Should we reset @message_update here (as done with @message/@contact, ..., in process_messages_upsert)?
app/services/whatsapp/incoming_message_baileys_service.rb line 83 at r4 (raw file):
end def status_mapper
WDYT?
Suggestion:
def update_message_statusapp/services/whatsapp/incoming_message_baileys_service.rb line 96 at r4 (raw file):
else Rails.logger.warn "Baileys unsupported message update status: #{status}" end
Please handle baileys status 1 and 5 explicitly, even if we're not going to use them.
app/services/whatsapp/incoming_message_baileys_service.rb line 96 at r4 (raw file):
else Rails.logger.warn "Baileys unsupported message update status: #{status}" end
It's possible for a delivered status to be received after read.
[2025-04-08 21:13:44.574 -0300] DEBUG (87155) (baileys/connection.ts:235:11): [+553484455937] [sendToWebhook] {"event":"messages.update","data":[{"key":{"remoteJid":"553484072960@s.whatsapp.net","id":"CE6022AC56BAF9BA8F057853DCB08FB2","fromMe":true},"update":{"status":4}}]}
[2025-04-08 21:13:44.863 -0300] DEBUG (87155) (baileys/connection.ts:235:11): [+553484455937] [sendToWebhook] {"event":"messages.update","data":[{"key":{"remoteJid":"553484072960:70@s.whatsapp.net","id":"CE6022AC56BAF9BA8F057853DCB08FB2","fromMe":true},"update":{"status":3}}]}
Evaluate why this happens and let me know what you think we should do.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 355 at r4 (raw file):
before do message.update!(source_id: message_id, status: 'sent') end
Why not this?
Suggestion:
let(:message) { create(:message, source_id: message_id, status: 'sent') }spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 358 at r4 (raw file):
it 'updates the message status' do update_payload = { key: { id: message_id }, update: { status: 3 } } # delivered
NIT
redundant comment
Suggestion:
update_payload = { key: { id: message_id }, update: { status: 3 } }spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 368 at r4 (raw file):
message.reload expect(message.status).to eq('delivered')
NIT
also apply below
Suggestion:
expect(message.reload.status).to eq('delivered')spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 390 at r4 (raw file):
end end end
Add spec for logger being called on unhandled status
There was a problem hiding this comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @CayoPOliveira)
app/services/whatsapp/incoming_message_baileys_service.rb line 83 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
WDYT?
I misunderstood the naming scheme. Keep as is, but see task later about cleaning this up
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/services/whatsapp/incoming_message_baileys_service.rb(2 hunks)spec/services/whatsapp/incoming_message_baileys_service_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/services/whatsapp/incoming_message_baileys_service_spec.rb
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#20
File: app/services/whatsapp/incoming_message_baileys_service.rb:0-0
Timestamp: 2025-04-08T21:26:26.063Z
Learning: Chatwoot message model (app/models/message.rb) only supports four status states: sent (0), delivered (1), read (2), and failed (3). When mapping from Baileys WhatsApp statuses, only ERROR (0), SERVER_ACK (2), DELIVERY_ACK (3), and READ (4) can be mapped to Chatwoot's statuses. Baileys statuses like PENDING (1) and PLAYED (5) don't have Chatwoot equivalents and should be handled as special cases or logged as unsupported.
app/services/whatsapp/incoming_message_baileys_service.rb (1)
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#20
File: app/services/whatsapp/incoming_message_baileys_service.rb:0-0
Timestamp: 2025-04-08T21:26:26.063Z
Learning: Chatwoot message model (app/models/message.rb) only supports four status states: sent (0), delivered (1), read (2), and failed (3). When mapping from Baileys WhatsApp statuses, only ERROR (0), SERVER_ACK (2), DELIVERY_ACK (3), and READ (4) can be mapped to Chatwoot's statuses. Baileys statuses like PENDING (1) and PLAYED (5) don't have Chatwoot equivalents and should be handled as special cases or logged as unsupported.
🔇 Additional comments (5)
app/services/whatsapp/incoming_message_baileys_service.rb (5)
3-3: Good addition of the MessageNotFoundError class.This custom error class helps with clear error handling when a message targeted for update isn't found.
176-183: Implementation follows established pattern consistently.The
process_messages_updatemethod follows the same pattern as the existingprocess_messages_upsertmethod, maintaining code consistency throughout the service.
185-190: Good implementation of handle_update method.The method properly validates the message before attempting to update it, and handles both status updates and content updates separately.
192-195: Clear and concise validation method.The method name accurately reflects its purpose and it properly sets
@messagefor use by other methods.
197-200: Well-structured status update method.This method correctly delegates status mapping to the
status_mappermethod and only updates the message when a valid status is returned.
| def update_message_content | ||
| message = @raw_message.dig(:update, :message, :editedMessage, :message) | ||
| Rails.logger.warn "Baileys unsupported message update type: #{message_type}" and return if message.blank? | ||
|
|
||
| content = message[:conversation] || message.dig(:extendedTextMessage, :text) | ||
|
|
||
| @message.update!(content: content) if content.present? | ||
| end |
There was a problem hiding this comment.
Guard against nil message and fix incorrect method usage.
There are two issues in this method:
- No nil check for
messagebefore accessing its properties - Incorrect usage of
message_typewhich expects a different@raw_messagestructure
def update_message_content
message = @raw_message.dig(:update, :message, :editedMessage, :message)
- Rails.logger.warn "Baileys unsupported message update type: #{message_type}" and return if message.blank?
+ if message.blank?
+ Rails.logger.warn "No valid message content found in the update event"
+ return
+ end
content = message[:conversation] || message.dig(:extendedTextMessage, :text)
@message.update!(content: content) if content.present?
end📝 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.
| def update_message_content | |
| message = @raw_message.dig(:update, :message, :editedMessage, :message) | |
| Rails.logger.warn "Baileys unsupported message update type: #{message_type}" and return if message.blank? | |
| content = message[:conversation] || message.dig(:extendedTextMessage, :text) | |
| @message.update!(content: content) if content.present? | |
| end | |
| def update_message_content | |
| message = @raw_message.dig(:update, :message, :editedMessage, :message) | |
| if message.blank? | |
| Rails.logger.warn "No valid message content found in the update event" | |
| return | |
| end | |
| content = message[:conversation] || message.dig(:extendedTextMessage, :text) | |
| @message.update!(content: content) if content.present? | |
| end |
There was a problem hiding this comment.
-(questions/changes requested) +(review needed)
Reviewed 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @gabrieljablonski)
app/services/whatsapp/incoming_message_baileys_service.rb line 47 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
WDYT of grouping related methods closer together:
process_*method (e.g.process_messages_upsert) -> related methods (e.g.handle_message,set_contact, ...)And repeat for each
process_*method.
Good point! Currently, the class is becoming a bit large, so grouping related methods makes sense. Perhaps later we'll consider extracting these groups into helper classes or modules per event type to keep things well-organized. WDYT?
app/services/whatsapp/incoming_message_baileys_service.rb line 52 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Why not reuse
@raw_message(also@message_update->@message)?
No strong reason, honestly. The main thing is that the update structure didn't immediately suggest a message to me, so I felt it was clearer to specify it explicitly as an update. Open to renaming if that makes sense to you!
app/services/whatsapp/incoming_message_baileys_service.rb line 52 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Should we reset
@message_updatehere (as done with@message/@contact, ..., inprocess_messages_upsert)?
Done
app/services/whatsapp/incoming_message_baileys_service.rb line 96 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Please handle baileys status 1 and 5 explicitly, even if we're not going to use them.
Done
app/services/whatsapp/incoming_message_baileys_service.rb line 96 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
It's possible for a
deliveredstatus to be received afterread.[2025-04-08 21:13:44.574 -0300] DEBUG (87155) (baileys/connection.ts:235:11): [+553484455937] [sendToWebhook] {"event":"messages.update","data":[{"key":{"remoteJid":"553484072960@s.whatsapp.net","id":"CE6022AC56BAF9BA8F057853DCB08FB2","fromMe":true},"update":{"status":4}}]} [2025-04-08 21:13:44.863 -0300] DEBUG (87155) (baileys/connection.ts:235:11): [+553484455937] [sendToWebhook] {"event":"messages.update","data":[{"key":{"remoteJid":"553484072960:70@s.whatsapp.net","id":"CE6022AC56BAF9BA8F057853DCB08FB2","fromMe":true},"update":{"status":3}}]}Evaluate why this happens and let me know what you think we should do.
Done
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 355 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Why not this?
The factory doesn't receive the source_id parameter.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 390 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Add spec for logger being called on unhandled status
Done.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/services/whatsapp/incoming_message_baileys_service.rb (3)
192-195: Suggest logging to ease troubleshooting
Currently, the code raisesMessageNotFoundErrorwith no prior logs to clarify which message ID was not found. Consider adding a warning log for better debugging, as shown below.def valid_update_message? @message = find_message_by_source_id(message_id) + Rails.logger.warn("No existing message found with source_id=#{message_id}") unless @message @message.present? end
202-225: Consider explicitly returning nil for unsupported statuses
The method logs a warning for unsupported statuses (likePLAYED), but it returnsnilimplicitly. For clarity, you may explicitly returnnilto indicate no mapped status. This is optional and won't change behavior.when 5 Rails.logger.warn 'Baileys unsupported message update status: PLAYED(5)' + return nil else Rails.logger.warn "Baileys unsupported message update status: #{@raw_message.dig(:update, :status)}" + return nil end
227-232: Handle possible out-of-sequence events
Currently, transitions fromread→deliveredor other out-of-order updates are disallowed by default. If you foresee scenarios where the sequence might arrive out of order, consider a more flexible approach or log a warning when rejecting transitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/services/whatsapp/incoming_message_baileys_service.rb(2 hunks)spec/services/whatsapp/incoming_message_baileys_service_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/services/whatsapp/incoming_message_baileys_service_spec.rb
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#20
File: app/services/whatsapp/incoming_message_baileys_service.rb:0-0
Timestamp: 2025-04-08T21:26:26.063Z
Learning: Chatwoot message model (app/models/message.rb) only supports four status states: sent (0), delivered (1), read (2), and failed (3). When mapping from Baileys WhatsApp statuses, only ERROR (0), SERVER_ACK (2), DELIVERY_ACK (3), and READ (4) can be mapped to Chatwoot's statuses. Baileys statuses like PENDING (1) and PLAYED (5) don't have Chatwoot equivalents and should be handled as special cases or logged as unsupported.
app/services/whatsapp/incoming_message_baileys_service.rb (1)
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#20
File: app/services/whatsapp/incoming_message_baileys_service.rb:0-0
Timestamp: 2025-04-08T21:26:26.063Z
Learning: Chatwoot message model (app/models/message.rb) only supports four status states: sent (0), delivered (1), read (2), and failed (3). When mapping from Baileys WhatsApp statuses, only ERROR (0), SERVER_ACK (2), DELIVERY_ACK (3), and READ (4) can be mapped to Chatwoot's statuses. Baileys statuses like PENDING (1) and PLAYED (5) don't have Chatwoot equivalents and should be handled as special cases or logged as unsupported.
🔇 Additional comments (4)
app/services/whatsapp/incoming_message_baileys_service.rb (4)
176-183: Well-structured iteration for message updates
This method cleanly iterates over the updates and callshandle_updatefor each. No concerns at this time.
185-190: Clear error handling for missing messages
RaisingMessageNotFoundErroris straightforward and ensures that invalid updates are flagged early. Implementation looks good.
197-200: Status update logic is robust
The usage ofstatus_transition_allowedensures valid progression of message statuses. No concerns.
234-244: Message content update aligns with best practices
Guarding against blank content and preserving the existing message if new content is missing is a prudent approach. Implementation looks good.
…ges.update event spec
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/services/whatsapp/incoming_message_baileys_service_spec.rb (2)
411-446: Consider verifying message state remains unchanged for unsupported statuses.While you're correctly logging warnings for unsupported statuses, the tests don't verify what happens to the message state in these cases. It would be valuable to add assertions to confirm the message status remains unchanged when an unsupported status is received.
it 'logs warning for unsupported played status' do update_payload = { key: { id: message_id }, update: { status: 5 } } params = { webhookVerifyToken: webhook_verify_token, event: 'messages.update', data: [update_payload] } allow(Rails.logger).to receive(:warn).with('Baileys unsupported message update status: PLAYED(5)') + original_status = message.status described_class.new(inbox: inbox, params: params).perform expect(Rails.logger).to have_received(:warn) + expect(message.reload.status).to eq(original_status) end
337-346: Ensure descriptive error message for MessageNotFoundError.The test verifies that a
MessageNotFoundErroris raised when a message isn't found, but it might be helpful to ensure the error message includes details like the message ID to aid in debugging.Consider enhancing the test to verify the error message contains the message ID:
expect do described_class.new(inbox: inbox, params: params).perform - end.to raise_error(Whatsapp::IncomingMessageBaileysService::MessageNotFoundError) + end.to raise_error(Whatsapp::IncomingMessageBaileysService::MessageNotFoundError, /#{message_id}/)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/services/whatsapp/incoming_message_baileys_service_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#20
File: app/services/whatsapp/incoming_message_baileys_service.rb:0-0
Timestamp: 2025-04-08T21:26:26.063Z
Learning: Chatwoot message model (app/models/message.rb) only supports four status states: sent (0), delivered (1), read (2), and failed (3). When mapping from Baileys WhatsApp statuses, only ERROR (0), SERVER_ACK (2), DELIVERY_ACK (3), and READ (4) can be mapped to Chatwoot's statuses. Baileys statuses like PENDING (1) and PLAYED (5) don't have Chatwoot equivalents and should be handled as special cases or logged as unsupported.
🔇 Additional comments (3)
spec/services/whatsapp/incoming_message_baileys_service_spec.rb (3)
324-447: Well-structured test cases for message update processing.This is a comprehensive set of tests for the new message update processing functionality. The tests cover key scenarios including error handling for missing messages, updating both status and content, preventing status downgrades, and handling unsupported statuses.
The approach aligns well with the Chatwoot message model limitations mentioned in the learnings, where only four status states are supported (sent, delivered, read, failed) and special handling is needed for unsupported Baileys statuses.
389-409: Good prevention of status downgrades.This test verifies that messages with "read" status aren't downgraded to "delivered", which is an important business rule to maintain data integrity and accurate message tracking.
373-376:✅ Verification successful
Verify the nested structure for edited message content.
The path to extract the new message content is quite deep (
message: { editedMessage: { message: { conversation: 'New message content' } } }). Ensure this matches exactly what the Baileys API returns for edited messages.
🏁 Script executed:
#!/bin/bash # Check if there are any examples or documentation of the message update structure # in the implementation or elsewhere in the codebase # Look for any examples or documentation of message update handling in the implementation rg -A 5 -B 5 "process_messages_update|update_message_content" --type rubyLength of output: 2961
Extraction Structure Verified – No Further Changes Required
The nested structure used to extract edited message content is confirmed to be correct. The implementation in
app/services/whatsapp/incoming_message_baileys_service.rb(in theupdate_message_contentmethod) digs into:update, :message, :editedMessage, :message, which exactly matches what the test snippet inspec/services/whatsapp/incoming_message_baileys_service_spec.rbexpects. No modifications are necessary.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r7.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @CayoPOliveira)
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 355 at r4 (raw file):
Previously, CayoPOliveira (Cayo P. R. Oliveira) wrote…
The factory doesn't receive the
source_idparameter.
It does. The issue here is message is not being evaluated until after .perform is called.
To solve that, use let!(:message)
Don't forget to do the same for all other relevant specs.
Suggestion:
context 'when message is found' do
let(:message_id) { 'msg_123' }
let!(:message) { create(:message, source_id: message_id, status: 'sent') }app/services/whatsapp/incoming_message_baileys_service.rb line 224 at r7 (raw file):
else Rails.logger.warn "Baileys unsupported message update status: #{@raw_message.dig(:update, :status)}" end
NIT
Suggestion:
# NOTE: Baileys status codes vs. Chatwoot support:
# - (0) ERROR → (3) failed
# - (1) PENDING → (0) sent
# - (2) SERVER_ACK → (0) sent
# - (3) DELIVERY_ACK → (1) delivered
# - (4) READ → (2) read
# - (5) PLAYED → (unsupported: PLAYED)
# For details: https://github.com/WhiskeySockets/Baileys/blob/v6.7.16/WAProto/index.d.ts#L36694
status = @raw_message.dig(:update, :status)
case status
when 0
'failed'
when 1, 2
'sent'
when 3
'delivered'
when 4
'read'
when 5
Rails.logger.warn 'Baileys unsupported message update status: PLAYED(5)'
else
Rails.logger.warn "Baileys unsupported message update status: #{status}"
endapp/services/whatsapp/incoming_message_baileys_service.rb line 232 at r7 (raw file):
true end
NIT
Convention for methods that return bool
Suggestion:
def status_transition_allowed?(new_status)
return false if @message.status == 'read'
return false if @message.status == 'delivered' && new_status == 'sent'
true
endThere was a problem hiding this comment.
-(review needed) +(questions/changes requested)
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @CayoPOliveira)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @CayoPOliveira)
There was a problem hiding this comment.
-(questions/changes requested) +(review needed)
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gabrieljablonski)
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 355 at r4 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
It does. The issue here is
messageis not being evaluated until after.performis called.To solve that, use
let!(:message)Don't forget to do the same for all other relevant specs.
Done.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/services/whatsapp/incoming_message_baileys_service_spec.rb (1)
324-434: Consider adding test cases for additional edge cases.While the current test coverage is good, consider adding tests for these scenarios:
- An update that changes both status and content simultaneously
- Updates with status codes 1 and 2 to verify they both map to 'sent'
- Attempting to change a 'delivered' message back to 'sent' to test that restriction
These additional tests would provide even more comprehensive coverage of the functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/services/whatsapp/incoming_message_baileys_service.rb(2 hunks)spec/services/whatsapp/incoming_message_baileys_service_spec.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#20
File: app/services/whatsapp/incoming_message_baileys_service.rb:0-0
Timestamp: 2025-04-08T21:26:26.063Z
Learning: Chatwoot message model (app/models/message.rb) only supports four status states: sent (0), delivered (1), read (2), and failed (3). When mapping from Baileys WhatsApp statuses, only ERROR (0), SERVER_ACK (2), DELIVERY_ACK (3), and READ (4) can be mapped to Chatwoot's statuses. Baileys statuses like PENDING (1) and PLAYED (5) don't have Chatwoot equivalents and should be handled as special cases or logged as unsupported.
app/services/whatsapp/incoming_message_baileys_service.rb (1)
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#20
File: app/services/whatsapp/incoming_message_baileys_service.rb:0-0
Timestamp: 2025-04-08T21:26:26.063Z
Learning: Chatwoot message model (app/models/message.rb) only supports four status states: sent (0), delivered (1), read (2), and failed (3). When mapping from Baileys WhatsApp statuses, only ERROR (0), SERVER_ACK (2), DELIVERY_ACK (3), and READ (4) can be mapped to Chatwoot's statuses. Baileys statuses like PENDING (1) and PLAYED (5) don't have Chatwoot equivalents and should be handled as special cases or logged as unsupported.
🔇 Additional comments (12)
app/services/whatsapp/incoming_message_baileys_service.rb (8)
3-3: LGTM: MessageNotFoundError error class.Well-defined error class that will be raised when a message that should be updated is not found.
176-183: LGTM: Process messages update method.This method follows the same pattern as
process_messages_upsertby iterating through each update, clearing state variables, and delegating tohandle_update.
185-190: LGTM: Handle update method.Good implementation that checks for message validity before attempting to update status or content. The method correctly raises
MessageNotFoundErrorwhen the message is not found and properly separates status updates from content updates.
192-195: LGTM: Message validation method.This method verifies message existence by source ID before proceeding with updates, which prevents errors from occurring later in the process.
197-200: LGTM: Status update method.The method checks if the status is present and if the transition is allowed before updating, which prevents invalid state transitions.
202-226: Good implementation of status mapping with comprehensive documentation.The status mapping is well-documented with clear comments explaining the correspondence between Baileys status codes and Chatwoot supported statuses. The implementation correctly handles unsupported statuses by logging warnings.
I see you've incorporated the feedback from previous reviews about handling status code 1 (PENDING) by mapping it to 'sent' and adding clear documentation about the status mappings.
228-233: LGTM: Status transition validation.Good implementation that prevents illogical status transitions:
- Already read messages can't change status
- Delivered messages can't be marked as sent
This maintains data integrity by enforcing sensible state transitions.
235-245: LGTM: Message content update method.The method includes proper null checks for the message content and only updates when new content is present. I see you've addressed the previous comment about guarding against nil message values.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb (4)
324-347: LGTM: Tests for message not found scenario.Good test case that verifies the
MessageNotFoundErroris raised when attempting to update a message that doesn't exist.
349-382: LGTM: Tests for message update scenarios.Comprehensive tests that verify both status updates and content updates work correctly. This provides good coverage for the core functionality.
385-401: LGTM: Test for status transition restrictions.Good test verifying that a message already in 'read' status cannot be downgraded to 'delivered', enforcing proper status transition rules.
403-434: LGTM: Tests for unsupported status handling.Good tests that verify warnings are logged for unsupported status codes (5 - PLAYED and 6 - undefined). This ensures the system gracefully handles unexpected status values.
There was a problem hiding this comment.
-(review needed) +(approved for merge)
Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:
* feat: enhance WhatsApp webhook processing and error responses handling * chore: correct spelling of 'WhatsApp' in webhook controller specs * refactor: rename webhook processing method and improve error handling * chore: update error handling in WhatsApp controller specs for specific exceptions * refactor: remove handling for StandardError in WhatsApp controller specs * refactor: simplify perform_whatsapp_events_job method * chore: update response status from not_found to bad_request for invalid message * refactor: update expectations for job processing in WhatsApp controller specs
There was a problem hiding this comment.
Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status:
This change is
Summary by CodeRabbit