chore: refactor incoming_message_baileys_service using composition and fix some failure points #53
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe codebase has been refactored to extract WhatsApp Baileys event handling logic from Changes
Sequence Diagram(s)sequenceDiagram
participant Webhook
participant IncomingMessageBaileysService
participant ConnectionUpdate
participant MessagesUpsert
participant MessagesUpdate
Webhook->>IncomingMessageBaileysService: perform(event_name, params)
alt event_name == "connection.update"
IncomingMessageBaileysService->>ConnectionUpdate: process_connection_update(params)
else event_name == "messages.upsert"
IncomingMessageBaileysService->>MessagesUpsert: process_messages_upsert(params)
else event_name == "messages.update"
IncomingMessageBaileysService->>MessagesUpdate: process_messages_update(params)
end
Possibly related PRs
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.75.5)app/services/whatsapp/baileys_handlers/helpers.rbrubocop-performance extension supports plugin, specify app/services/whatsapp/baileys_handlers/messages_update.rbrubocop-performance extension supports plugin, specify app/services/whatsapp/baileys_handlers/messages_upsert.rbrubocop-performance extension supports plugin, specify
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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.
Pull Request Overview
This PR refactors the IncomingMessageBaileysService to use composition by extracting connection, message upsert, and message update responsibilities into dedicated modules.
- Introduces modules for connection updates, messages upsert, and messages update.
- Moves common helper methods into a shared Helpers module.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/services/whatsapp/incoming_message_baileys_service.rb | Removes duplicated logic and includes specialized handler modules. |
| app/services/whatsapp/baileys_handlers/messages_upsert.rb | Extracts message-upsert logic with improved clarity and separation of concerns. |
| app/services/whatsapp/baileys_handlers/messages_update.rb | Extracts message update logic into its own module. |
| app/services/whatsapp/baileys_handlers/helpers.rb | Consolidates common helper methods used across Baileys handlers. |
| app/services/whatsapp/baileys_handlers/connection_update.rb | New module to encapsulate connection update behavior. |
|
|
||
| def cache_message_source_id_in_redis | ||
| key = format(Redis::RedisKeys::MESSAGE_SOURCE_KEY, id: message_id) | ||
| ::Redis::Alfred.setex(key, true) |
There was a problem hiding this comment.
The call to setex appears to be missing a TTL parameter. Typically, setex requires a key, a TTL (in seconds), and a value; consider verifying this usage to ensure the key expires as expected.
| ::Redis::Alfred.setex(key, true) | |
| ttl = 86400 # Set TTL to 24 hours (in seconds) | |
| ::Redis::Alfred.setex(key, ttl, true) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
@copilot-pull-request-reviewer[bot] Ignore this, the default TTL is already set in lib/redis/alfred.rb
Code snippet:
def setex(key, value, expiry = 1.day)
$alfred.with { |conn| conn.setex(key, expiry, value) }
end| def handle_attach_media | ||
| attachment_file = download_attachment_file | ||
| unless attachment_file | ||
| @message.save! |
There was a problem hiding this comment.
Saving a message when the attachment file is missing (right before raising AttachmentNotFoundError) might lead to an inconsistent state; consider handling the error before persisting the incomplete message.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I decided to mark the message as unsupported and save it
There was a problem hiding this comment.
Why assign + save! and not update!(is_unsupported: true)?
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
app/services/whatsapp/baileys_handlers/connection_update.rb (1)
1-21: Well-structured connection update handler with good separation of concerns.The module cleanly handles connection state updates with appropriate use of I18n for error messages and defensive programming by preserving existing connection status when not provided in the data.
Consider adding error handling for edge cases where
inboxorchannelmight be nil to prevent potential runtime errors.Consider adding a guard clause:
def process_connection_update + return unless inbox&.channel data = processed_params[:data]app/services/whatsapp/baileys_handlers/messages_upsert.rb (2)
54-54: Remove duplicate protocol message check.This check is redundant as
message_type == 'protocol'is already checked inhandle_messageat line 21.-def handle_create_message - return if message_type == 'protocol' || - (message_type == 'reaction' && message_content.blank?) +def handle_create_message + return if message_type == 'reaction' && message_content.blank?
41-41: Technical debt: Update source_id to complete JID.The FIXME comment indicates that the source_id should be updated to use the complete JID format in the future. This might be important for proper message tracking and identification.
Would you like me to create an issue to track this technical debt item for future implementation?
app/services/whatsapp/incoming_message_baileys_service.rb (1)
15-15: TODO: Implement methods for all expected events.The comment indicates that not all Baileys events are currently handled. Consider documenting which events are supported and which are pending implementation.
Would you like me to help create a comprehensive list of Baileys events that need to be implemented?
app/services/whatsapp/baileys_handlers/helpers.rb (2)
110-112: Address the TODO for Brazilian phone number handling.The TODO comment indicates incomplete handling of denormalized Brazilian phone numbers, which could lead to incorrect
self_message?detection for Brazilian users.This could impact message processing logic. Would you like me to research the Brazilian phone number normalization requirements and generate a solution?
95-99: Complex phone number parsing may be fragile.The phone number extraction logic handles multiple JID formats but could be brittle if the format changes. Consider adding validation or error handling.
def phone_number_from_jid # NOTE: jid shape is `<user>_<agent>:<device>@<server>` # https://github.com/WhiskeySockets/Baileys/blob/v6.7.16/src/WABinary/jid-utils.ts#L19 - @phone_number_from_jid ||= @raw_message[:key][:remoteJid].split('@').first.split(':').first.split('_').first + @phone_number_from_jid ||= begin + jid = @raw_message[:key][:remoteJid] + return nil unless jid.present? + + jid.split('@').first.split(':').first.split('_').first + rescue StandardError => e + Rails.logger.warn "Failed to parse phone number from JID: #{jid}, error: #{e.message}" + nil + end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/services/whatsapp/baileys_handlers/connection_update.rb(1 hunks)app/services/whatsapp/baileys_handlers/helpers.rb(1 hunks)app/services/whatsapp/baileys_handlers/messages_update.rb(1 hunks)app/services/whatsapp/baileys_handlers/messages_upsert.rb(1 hunks)app/services/whatsapp/incoming_message_baileys_service.rb(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/services/whatsapp/baileys_handlers/connection_update.rb (1)
app/models/channel/whatsapp.rb (1)
update_provider_connection!(67-71)
app/services/whatsapp/baileys_handlers/messages_upsert.rb (2)
app/services/whatsapp/baileys_handlers/helpers.rb (12)
jid_type(12-34)message_id(4-6)message_under_process?(114-117)message_type(36-57)cache_message_source_id_in_redis(119-122)clear_message_source_id_from_redis(124-127)contact_name(101-107)phone_number_from_jid(95-99)message_content(59-70)incoming?(8-10)file_content_type(72-78)message_mimetype(80-93)app/helpers/baileys_helper.rb (1)
extract_baileys_message_timestamp(2-12)
app/services/whatsapp/baileys_handlers/messages_update.rb (1)
app/services/whatsapp/baileys_handlers/helpers.rb (2)
message_id(4-6)message_content(59-70)
🔇 Additional comments (6)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
1-21: Excellent refactoring using the composition pattern!The service class is now much cleaner and focused solely on webhook validation and event dispatching. The extraction of event handling logic into dedicated modules significantly improves maintainability and testability.
The dynamic event method dispatch (lines 12-19) is well-implemented with appropriate logging for unsupported events.
app/services/whatsapp/baileys_handlers/messages_update.rb (3)
27-51: LGTM: Comprehensive status mapping with proper logging.The status mapping correctly handles all Baileys status codes according to the referenced documentation, with appropriate logging for unsupported statuses.
53-58: LGTM: Proper status transition validation.The status transition logic correctly prevents status regression (e.g., from 'read' back to 'sent' or 'delivered') which maintains message status integrity.
16-16:✅ Verification successful
Verify that
find_message_by_source_idmethod is available.The code calls
find_message_by_source_id(message_id)but this method is not defined in the current module or the includedHelpersmodule. This method must be defined in the class that includes this module.
🏁 Script executed:
#!/bin/bash # Description: Search for the find_message_by_source_id method definition # Expected: Find the method definition in the including service class ast-grep --pattern 'def find_message_by_source_id($_)'Length of output: 492
find_message_by_source_id is defined in Helpers
- The method
find_message_by_source_idis implemented inapp/services/whatsapp/incoming_message_service_helpers.rb:94–98, so the call inmessages_update.rbis valid and no missing-definition issue remains.app/services/whatsapp/baileys_handlers/helpers.rb (2)
12-34: LGTM: Comprehensive JID type classification.The JID type detection properly handles all WhatsApp entity types according to Baileys documentation with clear mapping logic.
119-127: LGTM: Proper Redis key management.The Redis operations use consistent key formatting and provide both caching and cleanup methods for message source IDs.
| def process_messages_upsert | ||
| messages = processed_params[:data][:messages] | ||
| messages.each do |message| | ||
| @message = nil | ||
| @contact_inbox = nil | ||
| @contact = nil | ||
| @raw_message = message | ||
| handle_message | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for individual message failures.
The current implementation could fail the entire batch if one message encounters an error. This might cause valid messages to be lost.
Wrap individual message processing in error handling:
def process_messages_upsert
messages = processed_params[:data][:messages]
messages.each do |message|
@message = nil
@contact_inbox = nil
@contact = nil
@raw_message = message
- handle_message
+ begin
+ handle_message
+ rescue StandardError => e
+ Rails.logger.error "Failed to process message #{message[:key][:id]}: #{e.message}"
+ Rails.logger.error e.backtrace.join("\n")
+ end
end
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 process_messages_upsert | |
| messages = processed_params[:data][:messages] | |
| messages.each do |message| | |
| @message = nil | |
| @contact_inbox = nil | |
| @contact = nil | |
| @raw_message = message | |
| handle_message | |
| end | |
| end | |
| def process_messages_upsert | |
| messages = processed_params[:data][:messages] | |
| messages.each do |message| | |
| @message = nil | |
| @contact_inbox = nil | |
| @contact = nil | |
| @raw_message = message | |
| begin | |
| handle_message | |
| rescue StandardError => e | |
| Rails.logger.error "Failed to process message #{message[:key][:id]}: #{e.message}" | |
| Rails.logger.error e.backtrace.join("\n") | |
| end | |
| end | |
| end |
🤖 Prompt for AI Agents
In app/services/whatsapp/baileys_handlers/messages_upsert.rb around lines 7 to
16, the process_messages_upsert method processes multiple messages but lacks
error handling for individual messages, risking the failure of the entire batch
if one message errors. To fix this, wrap the processing of each message inside a
begin-rescue block to catch and log errors for that specific message, allowing
the loop to continue processing subsequent messages without interruption.
There was a problem hiding this comment.
I want the error to propagate to the WhatsappController
There was a problem hiding this comment.
@CayoPOliveira, understood! You prefer to let errors propagate up to the WhatsappController level rather than handling them individually within the message processing loop. This approach allows for centralized error handling and ensures that any processing failures are handled consistently at the controller level.
With this approach, if any message in the batch fails, the entire operation will fail and the controller can decide how to respond (e.g., return an appropriate HTTP status code, log the error, etc.). This is a valid architectural choice for fail-fast error handling.
✏️ Learnings added
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#53
File: app/services/whatsapp/baileys_handlers/messages_upsert.rb:7-16
Timestamp: 2025-05-26T20:14:33.170Z
Learning: In the Whatsapp Baileys handlers for this codebase, errors should propagate to the WhatsappController level rather than being caught and handled individually within message processing methods. The user prefers centralized error handling at the controller level over individual message-level error handling.
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.
| def download_attachment_file | ||
| Down.download(@conversation.inbox.channel.media_url(@raw_message.dig(:key, :id)), headers: @conversation.inbox.channel.api_headers) | ||
| end |
There was a problem hiding this comment.
Add error handling for attachment download failures.
The download operation could fail due to network issues, invalid URLs, or authentication problems, which would cause an unhandled exception.
def download_attachment_file
- Down.download(@conversation.inbox.channel.media_url(@raw_message.dig(:key, :id)), headers: @conversation.inbox.channel.api_headers)
+ begin
+ Down.download(@conversation.inbox.channel.media_url(@raw_message.dig(:key, :id)), headers: @conversation.inbox.channel.api_headers)
+ rescue Down::Error => e
+ Rails.logger.error "Failed to download attachment for message #{message_id}: #{e.message}"
+ nil
+ end
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 download_attachment_file | |
| Down.download(@conversation.inbox.channel.media_url(@raw_message.dig(:key, :id)), headers: @conversation.inbox.channel.api_headers) | |
| end | |
| def download_attachment_file | |
| begin | |
| Down.download( | |
| @conversation.inbox.channel.media_url(@raw_message.dig(:key, :id)), | |
| headers: @conversation.inbox.channel.api_headers | |
| ) | |
| rescue Down::Error => e | |
| Rails.logger.error "Failed to download attachment for message #{message_id}: #{e.message}" | |
| nil | |
| end | |
| end |
🤖 Prompt for AI Agents
In app/services/whatsapp/baileys_handlers/messages_upsert.rb around lines 104 to
106, the download_attachment_file method lacks error handling for potential
failures during the download operation. Wrap the Down.download call in a
begin-rescue block to catch exceptions such as network errors, invalid URLs, or
authentication failures. Log the error or handle it appropriately to prevent
unhandled exceptions from crashing the application.
| end | ||
|
|
||
| def handle_edited_content | ||
| @raw_message = @raw_message.dig(:update, :message, :editedMessage) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid reassigning instance variables mid-method.
Reassigning @raw_message to a nested value can be confusing and may break other method calls that depend on the original structure. Consider using a local variable instead.
- @raw_message = @raw_message.dig(:update, :message, :editedMessage)
- content = message_content
+ edited_message = @raw_message.dig(:update, :message, :editedMessage)
+ # Temporarily set @raw_message for message_content helper method
+ original_message = @raw_message
+ @raw_message = edited_message
+ content = message_content
+ @raw_message = original_message📝 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.
| @raw_message = @raw_message.dig(:update, :message, :editedMessage) | |
| edited_message = @raw_message.dig(:update, :message, :editedMessage) | |
| # Temporarily set @raw_message for message_content helper method | |
| original_message = @raw_message | |
| @raw_message = edited_message | |
| content = message_content | |
| @raw_message = original_message |
🤖 Prompt for AI Agents
In app/services/whatsapp/baileys_handlers/messages_update.rb at line 61, avoid
reassigning the instance variable @raw_message to a nested value as it may cause
confusion and break other method calls relying on the original data. Instead,
assign the nested value to a new local variable to preserve the original
@raw_message structure while still accessing the edited message data.
There was a problem hiding this comment.
There’s no problem doing this here; this method is executed at the end of the corresponding flow.
There was a problem hiding this comment.
@CayoPOliveira, thank you for the clarification! You're absolutely right - since handle_edited_content is executed at the end of the flow after update_status, reassigning @raw_message doesn't pose any risk of breaking subsequent method calls. I appreciate you providing this context about the execution order.
🐰💭 (Understanding the execution flow better...)
✏️ Learnings added
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#53
File: app/services/whatsapp/baileys_handlers/messages_update.rb:61-61
Timestamp: 2025-05-26T20:14:34.792Z
Learning: In Whatsapp::BaileysHandlers::MessagesUpdate#handle_edited_content, reassigning @raw_message to the nested edited message data is acceptable because this method executes at the end of the corresponding flow, with no subsequent operations depending on the original @raw_message structure.
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.
There’s no problem doing this here; this method is executed at the end of the corresponding flow.
What about when it isn't anymore? For example, if we start doing more stuff after handle_edited_content.
Original coderabbit suggestion should be followed.
Also, can we simplify to this? Editing should only be possible for text messages.
Suggestion:
def handle_edited_content
content = @raw_message.dig(:update, :message, :editedMessage, :conversation)
@message.update!(content: content, is_edited: true, previous_content: @message.content)
endThere was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Previous suggestion was missing a hash key.
Suggestion:
def handle_edited_content
content = @raw_message.dig(:update, :message, :editedMessage, :message, :conversation)
@message.update!(content: content, is_edited: true, previous_content: @message.content)
endThere was a problem hiding this comment.
The text message could be conversation or extendedTextMessage, also its possible to edit image and video messages too
| def message_content | ||
| case message_type | ||
| when 'text' | ||
| @raw_message.dig(:message, :conversation) || @raw_message.dig(:message, :extendedTextMessage, :text) | ||
| when 'image' | ||
| @raw_message.dig(:message, :imageMessage, :caption) | ||
| when 'video' | ||
| @raw_message.dig(:message, :videoMessage, :caption) | ||
| when 'reaction' | ||
| @raw_message.dig(:message, :reactionMessage, :text) | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incomplete message content extraction for all supported message types.
The message_content method only handles text, image, video, and reaction messages, but the message_type method supports additional types like audio, file, and sticker. This inconsistency could lead to nil content for supported message types.
def message_content
case message_type
when 'text'
@raw_message.dig(:message, :conversation) || @raw_message.dig(:message, :extendedTextMessage, :text)
when 'image'
@raw_message.dig(:message, :imageMessage, :caption)
when 'video'
@raw_message.dig(:message, :videoMessage, :caption)
+ when 'audio'
+ @raw_message.dig(:message, :audioMessage, :caption) || 'Audio message'
+ when 'file'
+ @raw_message.dig(:message, :documentMessage, :caption) || 'Document'
+ when 'sticker'
+ 'Sticker'
when 'reaction'
@raw_message.dig(:message, :reactionMessage, :text)
+ when 'protocol'
+ 'Protocol message'
+ else
+ 'Unsupported message type'
end
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 message_content | |
| case message_type | |
| when 'text' | |
| @raw_message.dig(:message, :conversation) || @raw_message.dig(:message, :extendedTextMessage, :text) | |
| when 'image' | |
| @raw_message.dig(:message, :imageMessage, :caption) | |
| when 'video' | |
| @raw_message.dig(:message, :videoMessage, :caption) | |
| when 'reaction' | |
| @raw_message.dig(:message, :reactionMessage, :text) | |
| end | |
| end | |
| def message_content | |
| case message_type | |
| when 'text' | |
| @raw_message.dig(:message, :conversation) || @raw_message.dig(:message, :extendedTextMessage, :text) | |
| when 'image' | |
| @raw_message.dig(:message, :imageMessage, :caption) | |
| when 'video' | |
| @raw_message.dig(:message, :videoMessage, :caption) | |
| when 'audio' | |
| @raw_message.dig(:message, :audioMessage, :caption) || 'Audio message' | |
| when 'file' | |
| @raw_message.dig(:message, :documentMessage, :caption) || 'Document' | |
| when 'sticker' | |
| 'Sticker' | |
| when 'reaction' | |
| @raw_message.dig(:message, :reactionMessage, :text) | |
| when 'protocol' | |
| 'Protocol message' | |
| else | |
| 'Unsupported message type' | |
| end | |
| end |
🤖 Prompt for AI Agents
In app/services/whatsapp/baileys_handlers/helpers.rb around lines 59 to 70, the
message_content method only extracts content for text, image, video, and
reaction types, but does not handle other supported types like audio, file, and
sticker. Update the method to include extraction logic for these additional
message types by accessing the appropriate fields in @raw_message for audio,
file, and sticker messages to ensure content is correctly retrieved for all
supported types.
There was a problem hiding this comment.
@coderabbitai[bot] you can ignore this types, they don't have content
There was a problem hiding this comment.
@CayoPOliveira Thank you for the clarification! You're absolutely right - those message types represent media or protocol content rather than extractable text content. The current implementation correctly handles only the message types that actually have text content to extract. I appreciate you sharing this domain knowledge about WhatsApp message structure.
✏️ Learnings added
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#53
File: app/services/whatsapp/baileys_handlers/helpers.rb:59-70
Timestamp: 2025-05-26T20:14:33.133Z
Learning: In WhatsApp Baileys message handling, audio, file, sticker, protocol, and unsupported message types don't have extractable text content. Only text messages, image captions, video captions, and reaction text should be handled in message_content extraction methods.
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.
Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @gabrieljablonski)
| def message_content | ||
| case message_type | ||
| when 'text' | ||
| @raw_message.dig(:message, :conversation) || @raw_message.dig(:message, :extendedTextMessage, :text) | ||
| when 'image' | ||
| @raw_message.dig(:message, :imageMessage, :caption) | ||
| when 'video' | ||
| @raw_message.dig(:message, :videoMessage, :caption) | ||
| when 'reaction' | ||
| @raw_message.dig(:message, :reactionMessage, :text) | ||
| end | ||
| end |
There was a problem hiding this comment.
@coderabbitai[bot] you can ignore this types, they don't have content
|
|
||
| def cache_message_source_id_in_redis | ||
| key = format(Redis::RedisKeys::MESSAGE_SOURCE_KEY, id: message_id) | ||
| ::Redis::Alfred.setex(key, true) |
There was a problem hiding this comment.
@copilot-pull-request-reviewer[bot] Ignore this, the default TTL is already set in lib/redis/alfred.rb
Code snippet:
def setex(key, value, expiry = 1.day)
$alfred.with { |conn| conn.setex(key, expiry, value) }
end| end | ||
|
|
||
| def handle_edited_content | ||
| @raw_message = @raw_message.dig(:update, :message, :editedMessage) |
There was a problem hiding this comment.
There’s no problem doing this here; this method is executed at the end of the corresponding flow.
| def process_messages_upsert | ||
| messages = processed_params[:data][:messages] | ||
| messages.each do |message| | ||
| @message = nil | ||
| @contact_inbox = nil | ||
| @contact = nil | ||
| @raw_message = message | ||
| handle_message | ||
| end | ||
| end |
There was a problem hiding this comment.
I want the error to propagate to the WhatsappController
| def handle_attach_media | ||
| attachment_file = download_attachment_file | ||
| unless attachment_file | ||
| @message.save! |
There was a problem hiding this comment.
I decided to mark the message as unsupported and save it
There was a problem hiding this comment.
looks mostly good, with a few suggestions.
-(review needed) +(questions/changes requested)
Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Dismissed @coderabbitai[bot] from a discussion.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @CayoPOliveira)
app/services/whatsapp/baileys_handlers/messages_update.rb line 6 at r2 (raw file):
class MessageNotFoundError < StandardError; end def process_messages_update
Suggestion:
module Whatsapp::BaileysHandlers::MessagesUpdate
include Whatsapp::BaileysHandlers::Helpers
class MessageNotFoundError < StandardError; end
private
def process_messages_updateapp/services/whatsapp/baileys_handlers/helpers.rb line 4 at r2 (raw file):
include Whatsapp::IncomingMessageServiceHelpers def message_id
Suggestion:
module Whatsapp::BaileysHandlers::Helpers # rubocop:disable Metrics/ModuleLength
include Whatsapp::IncomingMessageServiceHelpers
private
def message_idapp/services/whatsapp/incoming_message_baileys_service.rb line 4 at r2 (raw file):
include ::Whatsapp::BaileysHandlers::ConnectionUpdate include ::Whatsapp::BaileysHandlers::MessagesUpsert include ::Whatsapp::BaileysHandlers::MessagesUpdate
NIT
Leading :: are used to avoid namespace colisions. For instance, if Whatsapp::Whatsapp namespace existed, this would be needed.
Since that's not the case, can be simplified.
Suggestion:
include Whatsapp::BaileysHandlers::ConnectionUpdate
include Whatsapp::BaileysHandlers::MessagesUpsert
include Whatsapp::BaileysHandlers::MessagesUpdateapp/services/whatsapp/baileys_handlers/connection_update.rb line 4 at r2 (raw file):
include Whatsapp::BaileysHandlers::Helpers def process_connection_update
Method definitions should be private, as it was before applying composition.
Suggestion:
module Whatsapp::BaileysHandlers::ConnectionUpdate
include Whatsapp::BaileysHandlers::Helpers
private
def process_connection_update| end | ||
|
|
||
| def handle_edited_content | ||
| @raw_message = @raw_message.dig(:update, :message, :editedMessage) |
There was a problem hiding this comment.
There’s no problem doing this here; this method is executed at the end of the corresponding flow.
What about when it isn't anymore? For example, if we start doing more stuff after handle_edited_content.
Original coderabbit suggestion should be followed.
Also, can we simplify to this? Editing should only be possible for text messages.
Suggestion:
def handle_edited_content
content = @raw_message.dig(:update, :message, :editedMessage, :conversation)
@message.update!(content: content, is_edited: true, previous_content: @message.content)
end| def handle_attach_media | ||
| attachment_file = download_attachment_file | ||
| unless attachment_file | ||
| @message.save! |
There was a problem hiding this comment.
Why assign + save! and not update!(is_unsupported: true)?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @CayoPOliveira)
| end | ||
|
|
||
| def handle_edited_content | ||
| @raw_message = @raw_message.dig(:update, :message, :editedMessage) |
There was a problem hiding this comment.
Previous suggestion was missing a hash key.
Suggestion:
def handle_edited_content
content = @raw_message.dig(:update, :message, :editedMessage, :message, :conversation)
@message.update!(content: content, is_edited: true, previous_content: @message.content)
end| end | ||
|
|
||
| def handle_edited_content | ||
| @raw_message = @raw_message.dig(:update, :message, :editedMessage) |
There was a problem hiding this comment.
The text message could be conversation or extendedTextMessage, also its possible to edit image and video messages too
There was a problem hiding this comment.
-(questions/changes requested) +(review needed)
Reviewed 2 of 5 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @gabrieljablonski)
app/services/whatsapp/baileys_handlers/connection_update.rb line 4 at r2 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Method definitions should be private, as it was before applying composition.
Done.
class MessageNotFoundError < StandardError; end def process_messages_update
Done.
app/services/whatsapp/baileys_handlers/helpers.rb line 4 at r2 (raw file):
include Whatsapp::IncomingMessageServiceHelpers def message_id
Done.
| def handle_attach_media | ||
| attachment_file = download_attachment_file | ||
| unless attachment_file | ||
| @message.save! |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
15-16: 💡 Verification agent🧩 Analysis chain
Address the TODO comment or remove if no longer relevant.
The TODO comment suggests that not all expected event methods have been implemented. Since this refactoring moves event handling to dedicated modules, please verify:
- Are there any missing event handlers that still need to be implemented?
- Are all expected events now covered by the included modules?
If all events are properly handled by the modules, this TODO can be removed.
🏁 Script executed:
#!/bin/bash # Description: Check what event types are mentioned in tests and logs to verify coverage # Expected: All event types should have corresponding handler methods # Search for event types in the codebase rg -A 2 -B 2 "event.*:" --type rb # Search for process_ methods that might indicate expected events rg "process_.*event" --type rb # Search for unsupported event log messages to see what events are encountered rg "unsupported event" --type rbLength of output: 184
Let’s first locate the
IncomingMessageBaileysServicefile and inspect its includes to confirm which modules are handling events:
🏁 Script executed:
#!/bin/bash # Locate the service file service_file=$(rg -l 'class IncomingMessageBaileysService' -g '*.rb') echo "Service file: $service_file" # Show the first 50 lines to see included modules and comments/TODO sed -n '1,50p' "$service_file"Length of output: 173
Let’s first locate and inspect the service file to see which modules it includes (and whether the TODO is still needed):
🏁 Script executed:
#!/bin/bash # Find the service file filepath=$(find . -type f -name 'incoming_message_baileys_service.rb') echo "Service file path: $filepath" # Print the first 100 lines to show includes and the TODO if [ -n "$filepath" ]; then sed -n '1,100p' "$filepath" else echo "Error: incoming_message_baileys_service.rb not found" fiLength of output: 1312
🏁 Script executed:
#!/bin/bash # List and display the handler modules to see which events they cover handler_dir="app/services/whatsapp/baileys_handlers" echo "Handler files found:" ls "$handler_dir"/*.rb echo for file in "$handler_dir"/*.rb; do echo "=== $file ===" sed -n '1,200p' "$file" echo doneLength of output: 13098
Remove the stale TODO in IncomingMessageBaileysService
The
process_*handlers for the only supported Baileys events—
connection.update(ConnectionUpdate)messages.upsert(MessagesUpsert)messages.update(MessagesUpdate)—are already implemented in
app/services/whatsapp/baileys_handlers. Any other events will simply be logged as unsupported. You can safely delete the# TODO: Implement the methods for all expected eventscomment (and its surrounding comment block) in
app/services/whatsapp/incoming_message_baileys_service.rb(around line 15), and rely on the fallback log for unhandled events. Add newprocess_*methods only when you onboard additional Baileys event types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/services/whatsapp/baileys_handlers/connection_update.rb(1 hunks)app/services/whatsapp/baileys_handlers/helpers.rb(1 hunks)app/services/whatsapp/baileys_handlers/messages_update.rb(1 hunks)app/services/whatsapp/baileys_handlers/messages_upsert.rb(1 hunks)app/services/whatsapp/incoming_message_baileys_service.rb(1 hunks)spec/services/whatsapp/incoming_message_baileys_service_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- app/services/whatsapp/baileys_handlers/connection_update.rb
- app/services/whatsapp/baileys_handlers/messages_update.rb
- app/services/whatsapp/baileys_handlers/messages_upsert.rb
- app/services/whatsapp/baileys_handlers/helpers.rb
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#53
File: app/services/whatsapp/baileys_handlers/messages_upsert.rb:7-16
Timestamp: 2025-05-26T20:14:33.170Z
Learning: In the Whatsapp Baileys handlers for this codebase, errors should propagate to the WhatsappController level rather than being caught and handled individually within message processing methods. The user prefers centralized error handling at the controller level over individual message-level error handling.
app/services/whatsapp/incoming_message_baileys_service.rb (2)
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#53
File: app/services/whatsapp/baileys_handlers/messages_upsert.rb:7-16
Timestamp: 2025-05-26T20:14:33.170Z
Learning: In the Whatsapp Baileys handlers for this codebase, errors should propagate to the WhatsappController level rather than being caught and handled individually within message processing methods. The user prefers centralized error handling at the controller level over individual message-level error handling.
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#53
File: app/services/whatsapp/baileys_handlers/messages_update.rb:61-61
Timestamp: 2025-05-26T20:14:34.792Z
Learning: In Whatsapp::BaileysHandlers::MessagesUpdate#handle_edited_content, reassigning @raw_message to the nested edited message data is acceptable because this method executes at the end of the corresponding flow, with no subsequent operations depending on the original @raw_message structure.
🔇 Additional comments (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
1-4: Excellent refactoring using composition pattern.The refactoring successfully extracts event-specific logic into dedicated handler modules, following the single responsibility principle and improving maintainability. The composition approach is much cleaner than having all logic in a single large service class.
| it 'throws an error if attachment download fails' do | ||
| allow(Down).to receive(:download).with('https://baileys.api/media/msg_123', headers: inbox.channel.api_headers) | ||
| .and_raise(Down::ResponseError.new('Attachment not found')) | ||
|
|
||
| expect do | ||
| described_class.new(inbox: inbox, params: params).perform | ||
| end.to raise_error(Whatsapp::IncomingMessageBaileysService::AttachmentNotFoundError) | ||
|
|
||
| message = inbox.conversations.last.messages.last | ||
| expect(message).to be_present | ||
| expect(message.is_unsupported).to be(true) | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error class accessibility after refactoring.
The new test case for attachment download failures looks well-structured and validates important error handling behavior. However, there's a potential issue with the error class reference.
The test expects Whatsapp::IncomingMessageBaileysService::AttachmentNotFoundError to be raised, but according to the AI summary, this error class has been moved to the MessagesUpsert handler module. This could cause the test to fail since the error class may no longer be accessible through the main service class namespace.
🏁 Script executed:
#!/bin/bash
# Description: Verify that AttachmentNotFoundError is accessible through the main service class
# Expected: The error class should be accessible either directly or through included modules
# Search for AttachmentNotFoundError definitions
ast-grep --pattern 'class AttachmentNotFoundError < $_'
# Search for error class references in the codebase
rg -A 3 'AttachmentNotFoundError'Length of output: 1587
Correct error class namespace in spec
The test currently expects
raise_error(Whatsapp::IncomingMessageBaileysService::AttachmentNotFoundError)but the exception is defined only in the handler module as
Whatsapp::BaileysHandlers::MessagesUpsert::AttachmentNotFoundError. Update the spec (and any rescue clauses in the controller) to reference the correct constant. For example:
- expect {
- described_class.new(inbox: inbox, params: params).perform
- }.to raise_error(Whatsapp::IncomingMessageBaileysService::AttachmentNotFoundError)
+ expect {
+ described_class.new(inbox: inbox, params: params).perform
+ }.to raise_error(Whatsapp::BaileysHandlers::MessagesUpsert::AttachmentNotFoundError)Alternatively, if you’d rather keep the service‐scoped constant, add an alias in the service class:
# app/services/whatsapp/incoming_message_baileys_service.rb
class Whatsapp::IncomingMessageBaileysService
AttachmentNotFoundError = Whatsapp::BaileysHandlers::MessagesUpsert::AttachmentNotFoundError
…
end🤖 Prompt for AI Agents
In spec/services/whatsapp/incoming_message_baileys_service_spec.rb around lines
427 to 438, the test expects the error AttachmentNotFoundError to be raised from
Whatsapp::IncomingMessageBaileysService, but this error class has been moved to
Whatsapp::BaileysHandlers::MessagesUpsert. Update the test to expect the error
from the correct namespace
Whatsapp::BaileysHandlers::MessagesUpsert::AttachmentNotFoundError, or
alternatively, add an alias for AttachmentNotFoundError inside the
IncomingMessageBaileysService class pointing to the new location to keep the
existing test unchanged.
| class Whatsapp::IncomingMessageBaileysService < Whatsapp::IncomingMessageBaseService | ||
| include Whatsapp::BaileysHandlers::ConnectionUpdate | ||
| include Whatsapp::BaileysHandlers::MessagesUpsert | ||
| include Whatsapp::BaileysHandlers::MessagesUpdate | ||
|
|
||
| class InvalidWebhookVerifyToken < StandardError; end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider exposing handler module error classes.
With the refactoring, custom error classes like AttachmentNotFoundError and MessageNotFoundError have been moved to their respective handler modules. However, existing tests (and potentially other code) expect these errors to be accessible through the main service class namespace.
Consider adding class aliases or delegating these error classes to maintain backward compatibility:
class Whatsapp::IncomingMessageBaileysService < Whatsapp::IncomingMessageBaseService
include Whatsapp::BaileysHandlers::ConnectionUpdate
include Whatsapp::BaileysHandlers::MessagesUpsert
include Whatsapp::BaileysHandlers::MessagesUpdate
class InvalidWebhookVerifyToken < StandardError; end
+
+ # Expose handler module error classes for backward compatibility
+ AttachmentNotFoundError = Whatsapp::BaileysHandlers::MessagesUpsert::AttachmentNotFoundError
+ MessageNotFoundError = Whatsapp::BaileysHandlers::MessagesUpdate::MessageNotFoundError📝 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.
| class InvalidWebhookVerifyToken < StandardError; end | |
| class Whatsapp::IncomingMessageBaileysService < Whatsapp::IncomingMessageBaseService | |
| include Whatsapp::BaileysHandlers::ConnectionUpdate | |
| include Whatsapp::BaileysHandlers::MessagesUpsert | |
| include Whatsapp::BaileysHandlers::MessagesUpdate | |
| class InvalidWebhookVerifyToken < StandardError; end | |
| # Expose handler module error classes for backward compatibility | |
| AttachmentNotFoundError = Whatsapp::BaileysHandlers::MessagesUpsert::AttachmentNotFoundError | |
| MessageNotFoundError = Whatsapp::BaileysHandlers::MessagesUpdate::MessageNotFoundError |
🤖 Prompt for AI Agents
In app/services/whatsapp/incoming_message_baileys_service.rb at line 6, the
custom error classes like AttachmentNotFoundError and MessageNotFoundError have
been moved to handler modules, but existing tests and code expect these errors
to be accessible via the main service class. To fix this, add class aliases or
delegate these error classes in the main service class to point to their new
locations in the handler modules, ensuring backward compatibility without
breaking existing references.
There was a problem hiding this comment.
-(review needed) +(approved for merge)
Reviewed 6 of 6 files at r3, 4 of 4 files at r4, all commit messages.
Dismissed @coderabbitai[bot] from 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @CayoPOliveira)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/controllers/webhooks/whatsapp_controller.rb(0 hunks)app/services/whatsapp/baileys_handlers/messages_update.rb(1 hunks)app/services/whatsapp/baileys_handlers/messages_upsert.rb(1 hunks)spec/services/whatsapp/incoming_message_baileys_service_spec.rb(1 hunks)
💤 Files with no reviewable changes (1)
- app/controllers/webhooks/whatsapp_controller.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/services/whatsapp/incoming_message_baileys_service_spec.rb
- app/services/whatsapp/baileys_handlers/messages_update.rb
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#53
File: app/services/whatsapp/baileys_handlers/messages_upsert.rb:7-16
Timestamp: 2025-05-26T20:14:33.177Z
Learning: In the Whatsapp Baileys handlers for this codebase, errors should propagate to the WhatsappController level rather than being caught and handled individually within message processing methods. The user prefers centralized error handling at the controller level over individual message-level error handling.
app/services/whatsapp/baileys_handlers/messages_upsert.rb (1)
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#53
File: app/services/whatsapp/baileys_handlers/messages_upsert.rb:7-16
Timestamp: 2025-05-26T20:14:33.177Z
Learning: In the Whatsapp Baileys handlers for this codebase, errors should propagate to the WhatsappController level rather than being caught and handled individually within message processing methods. The user prefers centralized error handling at the controller level over individual message-level error handling.
🧬 Code Graph Analysis (1)
app/services/whatsapp/baileys_handlers/messages_upsert.rb (2)
app/services/whatsapp/baileys_handlers/helpers.rb (12)
jid_type(14-36)message_id(6-8)message_under_process?(116-119)message_type(38-59)cache_message_source_id_in_redis(121-124)clear_message_source_id_from_redis(126-129)contact_name(103-109)phone_number_from_jid(97-101)message_content(61-72)incoming?(10-12)file_content_type(74-80)message_mimetype(82-95)app/helpers/baileys_helper.rb (1)
extract_baileys_message_timestamp(2-12)
🔇 Additional comments (4)
app/services/whatsapp/baileys_handlers/messages_upsert.rb (4)
7-16: LGTM: Error handling aligns with established preferencesThe method correctly allows errors to propagate up to the controller level, which aligns with the established preference for centralized error handling. The instance variable resets ensure clean state for each message.
18-36: Well-structured message processing flow with proper cleanupThe method has a clear flow with appropriate early returns and proper Redis cache management. The cleanup of Redis cache when contact is not found (line 27) prevents memory leaks and ensures processing state consistency.
89-102: Appropriate error handling for recoverable attachment failuresThe error handling for
Down::Erroris well-implemented and aligns with the business logic of marking messages as unsupported when attachments can't be downloaded. This is a specific, recoverable error case that doesn't warrant failing the entire operation.
108-114: Robust filename generation with fallback logicThe method properly handles cases where the document filename is missing and generates a meaningful fallback using message metadata. The MIME type parsing is defensive with proper fallbacks.
| account_id: @inbox.account_id, | ||
| inbox_id: @inbox.id, | ||
| source_id: message_id, | ||
| sender: incoming? ? @contact : @inbox.account.account_users.first.user, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential nil reference risk with account users
The code assumes at least one account user exists when accessing @inbox.account.account_users.first.user. This could raise a NoMethodError if the account has no users.
Consider adding a guard clause or default fallback:
- sender: incoming? ? @contact : @inbox.account.account_users.first.user,
+ sender: incoming? ? @contact : @inbox.account.account_users.first&.user || @inbox.account.users.first,🏁 Script executed:
#!/bin/bash
# Check if there are any safeguards or validations ensuring accounts have users
ast-grep --pattern 'account_users.first'Length of output: 40
🏁 Script executed:
#!/bin/bash
# Locate all references to `account_users` in the repo
rg 'account_users' -n .
# Find the Account model and its user associations
rg 'class Account' -n app/models
rg 'has_many.*account_users' -n app/models
rg 'has_many.*users.*through' -n app/modelsLength of output: 13627
🏁 Script executed:
#!/bin/bash
# Inspect the assignment and usage of @inbox and any guards around account_users in this service
rg -n '@inbox' -n app/services/whatsapp/baileys_handlers/messages_upsert.rb
sed -n '1,200p' app/services/whatsapp/baileys_handlers/messages_upsert.rbLength of output: 3928
🏁 Script executed:
#!/bin/bash
# Inspect Account model for validations or callbacks related to account_users
sed -n '1,200p' app/models/account.rb
rg -n 'validate' -n app/models/account.rb
rg -n 'after_create' -n app/models/account.rb
rg -n 'before_save' -n app/models/account.rbLength of output: 7550
🏁 Script executed:
#!/bin/bash
# Inspect the AccountsController for default account_user creation on account signup
rg -n 'class AccountsController' -n app/controllers/api/v1/accounts_controller.rb
rg -n 'def create' -n app/controllers/api/v1/accounts_controller.rb
sed -n '1,200p' app/controllers/api/v1/accounts_controller.rbLength of output: 4116
Guard against empty account_users
The current implementation in app/services/whatsapp/baileys_handlers/messages_upsert.rb:66 assumes there’s always at least one user on the account:
sender: incoming? ? @contact : @inbox.account.account_users.first.user,If account_users is empty, this will blow up with NoMethodError. Since nothing in the model or controller prevents an account from ending up without any users (they can all be destroyed via the API), we need to guard here.
Suggestions:
• At the top of create_message (before @conversation.messages.build), do:
# app/services/whatsapp/baileys_handlers/messages_upsert.rb
def create_message(attach_media: false)
# Guard missing account_user
account_user = @inbox.account.account_users.first&.user
unless account_user
Rails.logger.error("No account_user found for Account #{@inbox.account.id}, skipping outgoing message")
return
end
@message = @conversation.messages.build(
content: message_content,
account_id: @inbox.account_id,
inbox_id: @inbox.id,
source_id: message_id,
sender: incoming? ? @contact : account_user,
sender_type: incoming? ? 'Contact' : 'User',
message_type: incoming? ? :incoming : :outgoing,
content_attributes: message_content_attributes
)
…
end• At minimum, switch to safe navigation to avoid the crash:
- sender: incoming? ? @contact : @inbox.account.account_users.first.user,
+ sender: incoming? ? @contact : @inbox.account.account_users.first&.user,But the explicit guard and early return is preferred so you can log and handle the missing‐user case cleanly.
🤖 Prompt for AI Agents
In app/services/whatsapp/baileys_handlers/messages_upsert.rb at line 66, the
code assumes @inbox.account.account_users.first.user always exists, risking a
NoMethodError if account_users is empty. To fix this, add a guard clause at the
start of the create_message method to safely fetch the first account_user's user
with safe navigation, log an error if none is found, and return early to skip
message creation. Then use this guarded user variable in place of the direct
call in the sender assignment to prevent crashes and handle missing users
gracefully.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/services/whatsapp/baileys_handlers/messages_update.rb(1 hunks)spec/services/whatsapp/incoming_message_baileys_service_spec.rb(4 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#53
File: app/services/whatsapp/baileys_handlers/messages_upsert.rb:7-16
Timestamp: 2025-05-26T20:14:33.177Z
Learning: In the Whatsapp Baileys handlers for this codebase, errors should propagate to the WhatsappController level rather than being caught and handled individually within message processing methods. The user prefers centralized error handling at the controller level over individual message-level error handling.
app/services/whatsapp/baileys_handlers/messages_update.rb (1)
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#53
File: app/services/whatsapp/baileys_handlers/messages_update.rb:61-61
Timestamp: 2025-05-26T20:14:34.814Z
Learning: In Whatsapp::BaileysHandlers::MessagesUpdate#handle_edited_content, reassigning @raw_message to the nested edited message data is acceptable because this method executes at the end of the corresponding flow, with no subsequent operations depending on the original @raw_message structure.
🔇 Additional comments (6)
app/services/whatsapp/baileys_handlers/messages_update.rb (6)
1-5: LGTM! Good module structure and error handling setup.The module follows clear naming conventions and includes the necessary helpers. The custom
MessageNotFoundErrorprovides specific error handling for message lookup failures.
8-15: LGTM! Clean iteration logic with proper variable reset.The method properly resets instance variables for each update, ensuring clean state between iterations.
30-54: Excellent status mapping implementation!The comprehensive documentation and mapping between Baileys and Chatwoot status codes is very helpful. The logging for unsupported statuses aids in monitoring and debugging.
56-62: LGTM! Appropriate use of update_columns for performance.The use of
update_columnsis correctly flagged by rubocop and disabled with good reason - these timestamp updates don't need full model validations and callbacks.
64-69: LGTM! Sound business logic for status transitions.The status transition rules correctly prevent regression (read → other statuses, delivered → sent) which maintains data integrity.
71-78: Good error handling for missing content.The method appropriately handles cases where edited content might be missing and logs a warning for debugging purposes.
Note: The
@raw_messagereassignment on line 72 has been previously discussed extensively in past review comments. Based on the retrieved learning, this reassignment is acceptable since this method executes at the end of the flow.
| def handle_update | ||
| raise MessageNotFoundError unless find_message_by_source_id(message_id) | ||
|
|
||
| update_status if @raw_message.dig(:update, :status).present? | ||
| handle_edited_content if @raw_message.dig(:update, :message).present? | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error message with context for debugging.
The error handling logic is sound, but the raised exception lacks context about which message ID failed, making debugging more difficult.
- raise MessageNotFoundError unless find_message_by_source_id(message_id)
+ raise MessageNotFoundError, "Message not found with ID: #{message_id}" unless find_message_by_source_id(message_id)📝 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 handle_update | |
| raise MessageNotFoundError unless find_message_by_source_id(message_id) | |
| update_status if @raw_message.dig(:update, :status).present? | |
| handle_edited_content if @raw_message.dig(:update, :message).present? | |
| end | |
| def handle_update | |
| raise MessageNotFoundError, "Message not found with ID: #{message_id}" unless find_message_by_source_id(message_id) | |
| update_status if @raw_message.dig(:update, :status).present? | |
| handle_edited_content if @raw_message.dig(:update, :message).present? | |
| end |
🤖 Prompt for AI Agents
In app/services/whatsa
F438
pp/baileys_handlers/messages_update.rb around lines 17 to
22, the raised MessageNotFoundError does not include the message ID that caused
the failure. Modify the raise statement to include the message_id in the error
message or as part of the exception to provide context for debugging which
specific message was not found.
There was a problem hiding this comment.
Reviewed 4 of 6 files at r3, 1 of 4 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @CayoPOliveira)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r5, 1 of 1 files at r6, all commit messages.
Dismissed @coderabbitai[bot] from 2 discussions.
Reviewable status:
…:Helpers in connection_update, messages_update, and messages_upsert modules
…pers, messages_update, and messages_upsert modules
…ssages update and upsert
d922c2c to
5cbae02
Compare
There was a problem hiding this comment.
Reviewed 4 of 4 files at r7, all commit messages.
Reviewable status:
Pull Request Template
Description
Please include a summary of the change and issue(s) fixed. Also, mention relevant motivation, context, and any dependencies that this change requires.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Checklist:
This change is
Summary by CodeRabbit
New Features
Refactor
Tests