feat: process Baileys-API images in message.upsert event #26
Conversation
WalkthroughThis pull request modifies the HTTP response status for the Changes
Suggested labels
Suggested reviewers
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.73)app/models/channel/whatsapp.rbError: spec/models/channel/whatsapp_spec.rbError: ✨ 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:
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.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
app/controllers/webhooks/whatsapp_controller.rb:29
- Verify that using :not_found for MessageNotFoundError aligns with the intended API behavior. If clients are expecting a :bad_request response, consider updating the error handling logic.
head :not_found
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/services/whatsapp/incoming_message_baileys_service.rb (2)
89-92: Image message handling looks good, but consider consistent save patternThe implementation for handling image messages is correctly structured, creating the text message first, then attaching media, and finally saving the message. However, I noticed that text messages don't have an explicit
save!call while image messages do. This inconsistency might indicate that text messages are saved implicitly in thecreate_text_messagemethod.Consider making the pattern consistent for all message types to improve maintainability.
172-191: Consider adding file extension based on content typeCurrently, the attachment filename doesn't include a file extension, which might cause issues with some clients. Consider adding an appropriate extension ba 8000 sed on the content type.
@message.attachments.new( account_id: @message.account_id, file_type: file_content_type(message_type), file: { io: io, - filename: "attachment_#{message_id}", + filename: "attachment_#{message_id}#{file_extension(message_type)}", content_type: file_content_type(message_type) } ) # Add this helper method def file_extension(type) case type when 'image' '.jpg' when 'video' '.mp4' when 'audio' '.ogg' when 'document' '.bin' else '' 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(3 hunks)
🔇 Additional comments (3)
app/services/whatsapp/incoming_message_baileys_service.rb (3)
159-166: Good abstraction with message_content methodCentralizing the logic for extracting message content in a dedicated method is a good approach that makes the code more maintainable and extensible. The case statement makes it easy to add support for more message types in the future.
148-148: LGTM: Refactored to use message_content methodThe refactoring to use the new
message_contentmethod is a good improvement.
172-191:❓ Verification inconclusive
Verify the media attachment workflow with tests
This is a significant new feature that processes media attachments. Make sure to add integration or unit tests that verify:
- The entire flow from receiving an image message to creating and saving it with attachment
- Handling of missing media data
- Proper decoding and storage of the media file
To verify how complete the testing coverage is, you can run:
🏁 Script executed:
#!/bin/bash # Look for tests covering the image handling functionality rg -A 5 "def test.*image" spec/services/whatsapp/ # Check if there are tests for the new attach_media method rg -A 5 "attach_media" spec/services/whatsapp/Length of output: 98
Test Coverage Action Required for Media Attachments
It looks like our automated search didn’t find tests addressing the
attach_mediamethod or image-related workflows. Please manually verify whether tests exist elsewhere and, if not, add comprehensive integration or unit tests to cover:
- The full flow from receiving an image message to creating and saving the attachment.
- Scenarios where media data is missing.
- Correct decoding and storage of the media file.
| def attach_media | ||
| media = @processed_params.dig(:extra, :media) | ||
| return if media.blank? | ||
|
|
||
| attachment_payload = media[message_id] | ||
| return if attachment_payload.blank? | ||
|
|
||
| decoded_data = Base64.decode64(attachment_payload) | ||
| io = StringIO.new(decoded_data) | ||
|
|
||
| @message.attachments.new( | ||
| account_id: @message.account_id, | ||
| file_type: file_content_type(message_type), | ||
| file: { | ||
| io: io, | ||
| filename: "attachment_#{message_id}", | ||
| content_type: file_content_type(message_type) | ||
| } | ||
| ) | ||
| end |
There was a problem hiding this comment.
Verify file_content_type method exists and fix @processed_params reference
There are a couple of potential issues in this method:
- The method references
file_content_type(message_type)but I don't see this method defined in the class - The variable
@processed_paramsis used, but elsewhere the methodprocessed_paramsis called instead
Fix the variable reference with this change:
- media = @processed_params.dig(:extra, :media)
+ media = processed_params.dig(:extra, :media)Also, please ensure that file_content_type method exists or implement it if missing:
def file_content_type(type)
case type
when 'image'
'image/jpeg' # or use a more specific content type if available from the message
when 'video'
'video/mp4'
when 'audio'
'audio/ogg'
when 'document'
'application/octet-stream'
else
'application/octet-stream'
end
end🛠️ Refactor suggestion
Consider adding error handling for media processing
The media attachment code lacks error handling for potential failures during Base64 decoding or file attachment. Consider adding rescue blocks to gracefully handle any errors.
def attach_media
media = processed_params.dig(:extra, :media)
return if media.blank?
attachment_payload = media[message_id]
return if attachment_payload.blank?
begin
decoded_data = Base64.decode64(attachment_payload)
io = StringIO.new(decoded_data)
@message.attachments.new(
account_id: @message.account_id,
file_type: file_content_type(message_type),
file: {
io: io,
filename: "attachment_#{message_id}",
content_type: file_content_type(message_type)
}
)
rescue => e
Rails.logger.error "Failed to attach media for message #{message_id}: #{e.message}"
end
endCommittable suggestion skipped: line range outside the PR's diff.
…ment specs for images and videos
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
159-177:⚠️ Potential issueMissing file_content_type method and incorrect variable reference
There are a couple of issues in this method:
- The
file_content_typemethod is referenced but not defined- The instance variable
@processed_paramsis used, but elsewhere the methodprocessed_paramsis called- Missing content_type in the file hash
- No error handling for potential Base64 decoding failures
Fix the variable reference and implement the missing method:
def attach_media - media = @processed_params.dig(:extra, :media) + media = processed_params.dig(:extra, :media) return if media.blank? attachment_payload = media[message_id] return if attachment_payload.blank? begin decoded_data = Base64.decode64(attachment_payload) io = StringIO.new(decoded_data) @message.attachments.new( account_id: @message.account_id, file_type: file_content_type(message_type), file: { io: io, - filename: filename + filename: filename, + content_type: file_content_type(message_type) } ) rescue => e Rails.logger.error "Failed to attach media for message #{message_id}: #{e.message}" end end def file_content_type(type) case type when 'image' 'image/png' when 'video' 'video/mp4' when 'audio' 'audio/ogg' when 'file', 'document' 'application/octet-stream' when 'sticker' 'image/webp' else 'application/octet-stream' end end
🧹 Nitpick comments (1)
app/services/whatsapp/incoming_message_baileys_service.rb (1)
87-93: Consider making save behavior consistent across message typesFor text messages, you rely on
create_messageto save the record (which callscreate!), but for media messages you explicitly call@message.save!after attaching media. Consider making this behavior consistent to improve code maintainability.when 'text' create_message when 'image', 'file', 'video', 'audio', 'sticker' create_message attach_media - @message.save! + @message.save! # This is redundant if you modify attach_media to call save instead
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/services/whatsapp/incoming_message_baileys_service.rb(3 hunks)spec/models/attachment_spec.rb(1 hunks)spec/services/whatsapp/incoming_message_baileys_service_spec.rb(1 hunks)
🔇 Additional comments (3)
spec/models/attachment_spec.rb (1)
128-129: Good addition to strengthen test validationsThese new assertions enhance the test coverage by validating two additional important properties: the filename and content type of the file attachment. This helps ensure that media files processed by the service maintain the expected attributes.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb (2)
323-375: Excellent test coverage for image messagesThe test cases for image messages are well-structured and comprehensive. They properly test:
- Message creation with captions
- Media attachment validation including file type, presence, and filename format
This provides good coverage for the new image handling functionality.
377-429: Good test coverage for video messagesSimilar to the image tests, these tests for video messages provide thorough coverage of the video handling functionality, ensuring the caption is properly set and media is correctly attached.
| 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) | ||
| end | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incomplete message content handling
The message_content method only handles 'text', 'image', and 'video' types, but the handle_create_message method also processes 'file', 'audio', and 'sticker' types. This could lead to nil content for those 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 'file'
+ @raw_message.dig(:message, :documentMessage, :caption)
+ when 'audio'
+ @raw_message.dig(:message, :audioMessage, :caption)
+ when 'sticker'
+ @raw_message.dig(:message, :stickerMessage, :caption)
+ else
+ 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 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) | |
| 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 'file' | |
| @raw_message.dig(:message, :documentMessage, :caption) | |
| when 'audio' | |
| @raw_message.dig(:message, :audioMessage, :caption) | |
| when 'sticker' | |
| @raw_message.dig(:message, :stickerMessage, :caption) | |
| else | |
| nil | |
| end | |
| end |
There was a problem hiding this comment.
These types don’t have captions
There was a problem hiding this comment.
🧩 Analysis chain
@CayoPOliveira Thank you for the clarification. I apologize for the incorrect suggestion. Since these message types (file, audio, sticker) don't have captions in the WhatsApp Baileys API, should they return nil for their message content, or is there another field that should be used instead?
🏁 Script executed:
#!/bin/bash
# Look for any relevant documentation or tests that might show the structure of different message types
cd app || exit
grep -r "documentMessage\|audioMessage\|stickerMessage" --include="*.rb" .
cd ../spec || exit
grep -r "documentMessage\|audioMessage\|stickerMessage" --include="*.rb" .
Length of output: 951
🏁 Script executed:
#!/bin/bash
# Let's look for any tests that might simulate these message types
grep -r "raw_message.*documentMessage\|raw_message.*audioMessage\|raw_message.*stickerMessage" --include="*.rb" .
# Search for test files that might contain examples
find spec -name "*whatsapp*" -o -name "*baileys*" | xargs grep -l "audio\|document\|sticker"
# Let's examine the IncomingMessageBaileysService spec to see how message types are tested
cat spec/services/whatsapp/incoming_message_baileys_service_spec.rb
Length of output: 29681
🏁 Script executed:
#!/bin/bash
# Let's check the Baileys documentation or code to understand message structures
# First, try to find any imported Baileys documentation or type definitions
grep -r "Baileys" --include="*.md" --include="*.js" --include="*.ts" .
# Check if there are any relevant packages in package.json
if [ -f package.json ]; then
grep -A 10 -B 10 "baileys\|whatsapp" package.json
fi
# Let's look for specific examples in the spec files for the message formats
grep -A 20 "raw_message.*image\|raw_message.*video" --include="*.rb" spec/
Length of output: 802
🏁 Script executed:
#!/bin/bash
# Examine a wider context around where message types are processed
grep -r -A 10 -B 10 "message_type" --include="*.rb" app/services/whatsapp/
# Look at spec file with more context
cat spec/services/whatsapp/incoming_message_baileys_service_spec.rb
# Look for examples of test messages with different types
cd spec
grep -r -B 10 -A 30 "message:" --include="*.rb" .
Length of output: 131070
@CayoPOliveira Thank you for the clarification. I apologize for the incorrect suggestion. Since these message types (file, audio, sticker) don't have captions in the WhatsApp Baileys API, should they return nil for their message content, or is there another field that should be used instead?
🧬 Code Graph Analysis Results
Relevant snippets for app/services/whatsapp/incoming_message_baileys_service.rb
File: app/jobs/webhooks/whatsapp_events_job.rb
Lines: 4-19
def perform(params = {})
channel = find_channel(params)
if channel_is_inactive?(channel)
Rails.logger.warn("Inactive WhatsApp channel: #{channel&.phone_number || "unknown - #{params[:phone_number]}"}")
return
end
case channel.provider
when 'whatsapp_cloud'
Whatsapp::IncomingMessageWhatsappCloudService.new(inbox: channel.inbox, params: params).perform
when 'baileys'
Whatsapp::IncomingMessageBaileysService.new(inbox: channel.inbox, params: params).perform
else
Whatsapp::IncomingMessageService.new(inbox: channel.inbox, params: params).perform
end
endThis snippet shows how the Whatsapp::IncomingMessageBaileysService is invoked when the provider is baileys. It provides context on how this service fits into the webhook processing flow.
File: app/models/channel/whatsapp.rb
Lines: 65-69
def update_provider_connection!(provider_connection)
assign_attributes(provider_connection: provider_connection)
# NOTE: Skip `validate_provider_config?` check
save!(validate: false)
endThis method is called from process_connection_update in the Baileys service to update the provider connection status on the WhatsApp channel.
Summary of key methods in Whatsapp::IncomingMessageBaileysService (from the provided file):
perform: Validates webhook token, dispatches event processing based on event type.process_messages_upsert: Processes incoming messages batch, callshandle_messagefor each.handle_message: Skips non-user JIDs, deduplicates messages, sets contact and conversation, creates message.set_contact: Parses phone number and push name from JID, creates or updates contact inbox and contact.handle_create_message: Creates message record, attaches media if applicable.message_type: Determines message type from raw message keys (e.g., text, image, video, sticker).message_content: Extracts message content or caption for text, image, video messages.attach_media: Decodes base64 media payload and attaches it to the message.filename: Generates filename for attachments, falling back to a timestamped name.process_messages_update: Handles message updates such as status changes or content edits.update_status: Maps Baileys status codes to internal statuses and updates message status.status_mapper: Maps Baileys numeric status codes to string statuses.status_transition_allowed?: Prevents invalid status transitions (e.g., from read back to sent).- Redis caching methods to prevent duplicate message processing.
Note on captions (related to user comment):
The message_content method returns captions for image and video message types by digging into :caption keys inside the raw message. Other media types like file, audio, sticker do not have caption extraction logic here, which aligns with the user comment "These types don’t have captions."
[End of relevant snippets]
✏️ Learnings added
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#26
File: app/services/whatsapp/incoming_message_baileys_service.rb:198-207
Timestamp: 2025-04-16T11:55:12.186Z
Learning: In the WhatsApp Baileys API integration, only 'image' and 'video' message types support captions. The 'file', 'aud
8000
io', and 'sticker' message types do not have caption fields.
Learnt from: CayoPOliveira
PR: fazer-ai/chatwoot#26
File: app/services/whatsapp/incoming_message_baileys_service.rb:198-207
Timestamp: 2025-04-16T11:55:12.186Z
Learning: In the WhatsApp Baileys API integration, only 'image' and 'video' message types support captions. The 'file', 'audio', and 'sticker' message types do not have caption fields.
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.
-work-in-progress +(review needed)
Reviewed 1 of 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @coderabbitai[bot])
| 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) | ||
| end | ||
| end |
There was a problem hiding this comment.
These types don’t have captions
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the message creation flow for Baileys-API messages and expands support for processing various media types (images, videos, files, audio, stickers) associated with the messages.upsert event. Key changes include updating the test suite for media attachments, refactoring the message creation and attachment logic, and adjusting the error response code in the WhatsApp controller.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| spec/services/whatsapp/incoming_message_baileys_service_spec.rb | Added test cases covering image, video, file, audio, and sticker message scenarios |
| spec/models/attachment_spec.rb | Updated attachment tests to verify filename and content type metadata |
| app/services/whatsapp/incoming_message_baileys_service.rb | Refactored message creation (renamed create_text_message to create_message) and added attach_media |
| app/controllers/webhooks/whatsapp_controller.rb | Adjusted error response to return 404 instead of 400 for MessageNotFoundError |
| return :image if %w[image sticker].include?(message_type) | ||
| return :audio if ['audio'].include?(message_type) | ||
| return :video if ['video'].include?(message_type) | ||
|
|
||
| :file |
There was a problem hiding this comment.
The file_content_type method returns symbols (e.g., :image, :audio, :video, :file) while the tests expect string values (e.g., 'image', 'audio', 'video', 'file'). Consider converting these symbols to strings for consistency with test expectations.
| return :image if %w[image sticker].include?(message_type) | |
| return :audio if ['audio'].include?(message_type) | |
| return :video if ['video'].include?(message_type) | |
| :file | |
| return 'image' if %w[image sticker].include?(message_type) | |
| return 'audio' if ['audio'].include?(message_type) | |
| return 'video' if ['video'].include?(message_type) | |
| 'file' |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @gabrieljablonski)
There was a problem hiding this comment.
-(review needed) +(questions/changes requested)
Reviewed 1 of 1 files at r1, 1 of 3 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Dismissed @Copilot from a discussion.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @CayoPOliveira)
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 0 at r5 (raw file):
Please attach the source files on Notion, and provide the method you used to encode it to base64.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 357 at r5 (raw file):
end it 'attaches the media with expected behavior' do
NIT
Suggestion:
it 'creates message attachment' dospec/services/whatsapp/incoming_message_baileys_service_spec.rb line 376 at r5 (raw file):
expect(attachment.file.filename.to_s).to eq(filename) expect(attachment.file.content_type).to eq(mimetype) end
- Try using variables more sparingly (don't forget to remove
let(:mimetype) - Use
freeze_time
Suggestion:
it 'attaches the media with expected behavior' do
freeze_time
described_class.new(inbox: inbox, params: params).perform
conversation = inbox.conversations.last
message = conversation.messages.last
attachment = message.attachments.last
expect(attachment).to be_present
expect(attachment.file).to be_present
expect(attachment.file_type).to eq('image')
expect(attachment.file.filename.to_s).to eq("image_#{message.id}_#{Time.current.strftime('%Y%m%d')}")
expect(attachment.file.content_type).to eq('image/png')
endspec/services/whatsapp/incoming_message_baileys_service_spec.rb line 379 at r5 (raw file):
end context 'when message type is video' do
Apply suggestions to be consistent with image specs.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 435 at r5 (raw file):
end context 'when message type is file' do
Keep consistent with suggestions for image type.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 477 at r5 (raw file):
end context 'when message type is audio' do
Keep consistent with suggestions for image type.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 524 at r5 (raw file):
end context 'when message type is sticker' do
Keep consistent with suggestions for image type.
app/services/whatsapp/incoming_message_baileys_service.rb line 163 at r5 (raw file):
attachment_payload = media[message_id] return if attachment_payload.blank?
We should raise an error (and log it) and return 422. This shouldn't happen.
app/services/whatsapp/incoming_message_baileys_service.rb line 190 at r5 (raw file):
:file end
NIT
Suggestion:
def file_content_type
return :image if message_type.in?(%w[image sticker])
return :audio if message_type == 'audio'
return :video if message_type == 'video'
:file
endapp/services/whatsapp/incoming_message_baileys_service.rb line 196 at r5 (raw file):
return filename if filename.present? "#{file_content_type}_#{@message[:id]}#{Time.current.strftime('%Y%m%d%H%M%S%s')}"
This timestamp is unnecessarily complex. Date should be good enough.
Also probably best to use _ between id and timestamp.
Suggestion:
"#{file_content_type}_#{@message[:id]}_#{Time.current.strftime('%Y%m%d')}"There was a problem hiding this comment.
-(questions/changes requested) +(review needed)
Reviewed all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @CayoPOliveira and @gabrieljablonski)
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 376 at r5 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
- Try using variables more sparingly (don't forget to remove
let(:mimetype)- Use
freeze_time
Done.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 379 at r5 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Apply suggestions to be consistent with image specs.
Done.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 435 at r5 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Keep consistent with suggestions for image type.
Done.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 477 at r5 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Keep consistent with suggestions for image type.
Done.
spec/services/whatsapp/incoming_message_baileys_service_spec.rb line 524 at r5 (raw file):
Previously, gabrieljablonski (Gabriel Jablonski) wrote…
Keep consistent with suggestions for image type.
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)
323-547: Consider adding error case testingWhile the happy path tests are well covered, consider adding tests for error scenarios such as invalid base64 data, missing media information, or unsupported media types to ensure robust error handling.
This could be implemented with contexts like:
context 'when media data is invalid' do let(:params) do { webhookVerifyToken: webhook_verify_token, event: 'messages.upsert', data: { type: 'notify', messages: [raw_message] }, extra: { media: { 'msg_123' => 'invalid-base64' } } } end it 'handles the error gracefully' do expect { described_class.new(inbox: inbox, params: params).perform }.not_to raise_error # Additional assertions for logging or fallback behavior end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/controllers/webhooks/whatsapp_controller.rb(1 hunks)app/services/whatsapp/incoming_message_baileys_service.rb(4 hunks)spec/services/whatsapp/incoming_message_baileys_service_spec.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/controllers/webhooks/whatsapp_controller.rb
- app/services/whatsapp/incoming_message_baileys_service.rb
🔇 Additional comments (5)
spec/services/whatsapp/incoming_message_baileys_service_spec.rb (5)
323-372: Great implementation for image message processing tests!The test cases for image message processing effectively validate both content and attachment handling. You've properly used
freeze_timeto ensure consistent filename generation and the assertions cover all critical aspects including file presence, type, filename format, and content type detection.
374-423: Well-structured tests for video messages!The video message tests are comprehensive, checking both caption handling and attachment properties. The base64 sample data appears valid for testing purposes, and all necessary assertions are in place to verify correct processing.
425-464: Effective file type handling and filename preservation!The file message tests correctly validate that original filenames are preserved and content types are accurately detected. This is important functionality for document sharing in WhatsApp.
466-506: Audio message tests look good!The audio message test implementation is well-structured and properly verifies audio file attachment characteristics. The use of
freeze_timeensures consistent test behavior for filename generation.
508-547: Correctly handling stickers as image attachments!The test confirms that stickers are properly treated as image attachments, which aligns with how they should be displayed in the UI. All the necessary properties are verified including file type, content type and filename format.
There was a problem hiding this comment.
-(review needed) +(questions/changes requested)
Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @CayoPOliveira)
There was a problem hiding this comment.
Reviewed 1 of 3 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @CayoPOliveira)
* feat: create alert message for unsupported message types * chore: fail message when try send not supported type in baileys * chore: add tests for unsupported message handling in Baileys service * chore: correct spelling * chore: NIT in spec name * chore: remove unnecessary message reload in unsupported message type test * feat: baileys support to send attachments (#28) * feat: enhance message sending logic with support for attachments and interactive messages * fix: update message format to use messageContent for text messages * feat: attachment message sending * fix: use strict encoding for attachment file download * chore: streamline message sending logic and remove unused error classes * chore: remove type from message sending logic * chore: update message sending specs * chore: raise MessageNotSentError instead of updating message status to failed * chore: change baileys contact name preferences (#30) * refactor: improve contact name handling and extraction from JID * test: enhance message handling to update contact names based on pushName and verifiedBizName * chore: update contact name condition to match phone number from JID * chore: correct method name typo * chore: correct variable names for phone number consistency in tests * chore: update message payload structure and improve error handling in send_message * test: re-add testing for error handling * feat: enhance contact name handling and add self-message detection * fix: ensure presence check for verified business name in contact name retrieval * test: improve specs --------- Co-authored-by: gabrieljablonski <contact@gabrieljablonski.com> --------- Co-authored-by: gabrieljablonski <contact@gabrieljablonski.com> Co-authored-by: Gabriel Jablonski <gabriel@fazer.ai> --------- Co-authored-by: gabrieljablonski <contact@gabrieljablonski.com> Co-authored-by: Gabriel Jablonski <gabriel@fazer.ai>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
spec/services/whatsapp/providers/whatsapp_baileys_service_spec.rb (1)
183-191: Prefer single source of truth for unsupported-message testSee duplication note above; choose either this block or the earlier one to avoid diverging expectations in future edits.
🧹 Nitpick comments (5)
app/services/whatsapp/providers/whatsapp_baileys_service.rb (2)
40-41: Method exits without feedback to callerWhen neither content nor attachments are present the code marks the DB record as failed but does not return any explicit result (
nil), whereas the success path returns the WhatsApp message id.
Consider returning a boolean/identifier consistently so that upstream code can rely on it:- message.update!(content: I18n.t('errors.messages.send.unsupported'), status: 'failed') + message.update!(content: I18n.t('errors.messages.send.unsupported'), status: 'failed') + return false
90-111: Edge cases inmessage_content
fileNameis anActiveStorage::Filenameobject; convert explicitly to string to avoid JSON serialisation surprises.- If the
file_typeis not one of the five whitelisted values the payload will miss the actual media key – the provider will reject it but the code won’t notice. Consider guarding with an explicit else-branch:content = { - fileName: @attachment.file.filename, + fileName: @attachment.file.filename.to_s, caption: @message.content } case @attachment.file_type ... +else + raise MessageContentTypeNotSupported, @attachment.file_type endspec/services/whatsapp/providers/whatsapp_baileys_service_spec.rb (2)
145-153: Duplicated context – already covered belowThis “when message does not have content nor attachments” example is conceptually identical to the block starting at line 183. Keeping both increases test runtime without adding coverage.
Recommend deleting one of them or extracting a shared example.
225-244: Misplaced 5XX-re 10000 sponse scenarioThe context “when provider responds with 5XX” lives inside the
#validate_provider_config? → when response is unsuccessfulbranch, but it actually exercises#send_message.
Readers (and future maintainers) will look for send-message-specific failures inside the#send_messagedescribe block.Move this example under the
describe '#send_message'group to keep the spec structure intuitive.spec/services/whatsapp/incoming_message_baileys_service_spec.rb (1)
376-601: Spec bloating – consider shared examples to DRY up media-type testsThe five media contexts (image, video, file, audio, sticker) repeat the exact Arrange-Act-Assert pattern with only:
• the
raw_messageinner key
• the expectedfile_type
• optional caption / filename differencesExtracting a shared example:
shared_examples 'media message' do |media_key, file_type, extra_expectations = {}| it "creates #{file_type} attachment" do freeze_time described_class.new(inbox: inbox, params: params).perform attachment = inbox.conversations.last.messages.last.attachments.last expect(attachment.file_type).to eq(file_type) extra_expectations.call(attachment) if extra_expectations end end…would shrink the file substantially and improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/services/whatsapp/incoming_message_baileys_service.rb(5 hunks)app/services/whatsapp/providers/whatsapp_baileys_service.rb(3 hunks)config/locales/en.yml(1 hunks)config/locales/pt_BR.yml(3 hunks)spec/services/whatsapp/incoming_message_baileys_service_spec.rb(4 hunks)spec/services/whatsapp/providers/whatsapp_baileys_service_spec.rb(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- config/locales/pt_BR.yml
- config/locales/en.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- app/services/whatsapp/incoming_message_baileys_service.rb
| def send_attachment_message | ||
| @attachment = @message.attachments.first | ||
|
|
||
| response = HTTParty.post( | ||
| "#{provider_url}/connections/#{whatsapp_channel.phone_number}/send-message", | ||
| headers: api_headers, | ||
| body: { | ||
| recipient: @phone_number, | ||
| messageContent: message_content | ||
| }.to_json | ||
| ) | ||
|
|
||
| return response.parsed_response.dig('data', 'key', 'id') if process_response(response) | ||
|
|
||
| raise MessageNotSentError | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing timeouts and large-file memory blow-up
HTTParty.postcalls do not specifytimeout:– a slow provider can block Sidekiq workers indefinitely.Base64.strict_encode64(@attachment.file.download)loads the full blob into memory. A 20 MB video becomes ~27 MB Base64 string, which might blow the job memory limit.
response = HTTParty.post(
"#{provider_url}/connections/#{whatsapp_channel.phone_number}/send-message",
headers: api_headers,
+ timeout: 15,
body: {For large files, stream-encode (e.g. Base64.encode64(IO.read(path, 16.kilobytes))) or enforce a max-size threshold.
| def send_message(phone_number, message) | ||
| raise MessageContentTypeNotSupported unless message.content_type == 'text' | ||
|
|
||
| response = HTTParty.post( | ||
| "#{provider_url}/connections/#{whatsapp_channel.phone_number}/send-message", | ||
| headers: api_headers, | ||
| body: { | ||
| type: 'text', | ||
| recipient: phone_number, | ||
| message: message.content | ||
| }.to_json | ||
| ) | ||
|
|
||
| return unless process_response(response) | ||
|
|
||
| response.parsed_response.dig('data', 'key', 'id') | ||
| @message = message | ||
| @phone_number = phone_number | ||
| if message.attachments.present? | ||
| send_attachment_message | ||
| elsif message.content.present? | ||
| send_text_message | ||
| else | ||
| message.update!(content: I18n.t('errors.messages.send.unsupported'), status: 'failed') | ||
| end |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Attachment-first dispatch silently ignores additional attachments
Only the first attachment (@message.attachments.first) is considered when deciding the branch and when actually building the payload later on.
If a customer sends multiple files (fairly common on WhatsApp), the remaining files will be silently dropped.
-if message.attachments.present?
- send_attachment_message
+if message.attachments.many?
+ # Iterate and send each attachment or switch to WhatsApp “media-collection” API once it’s supported.
+ message.attachments.each { |att| send_attachment_message(att) }
+elsif message.attachments.one?
+ send_attachment_message(message.attachments.first)(You would also need to adapt send_attachment_message’s signature accordingly.)
Failing to deliver all media will create confusing UX and support tickets.
🏁 Script executed:
#!/bin/bash
# Locate the send_attachment_message implementation
rg -n "def send_attachment_message" -3 app/services/whatsapp/providers/whatsapp_baileys_service.rb
# See how attachments are accessed in this file
rg -n "attachments\." -n app/services/whatsapp/providers/whatsapp_baileys_service.rb
# Check for explicit use of .first on attachments
rg -n "attachments.first" -n app/services/whatsapp/providers/whatsapp_baileys_service.rbLength of output: 816
Handle multiple attachments in WhatsApp Baileys service
The current implementation in
app/services/whatsapp/providers/whatsapp_baileys_service.rb
always picks only the first attachment (@message.attachments.first), dropping any additional files. Customers uploading multiple media will see only one file delivered, leading to confusion and extra support work.
Please update as follows:
– In send_message (around line 35), branch on the number of attachments:
• many? – iterate over message.attachments and call send_attachment_message(att) for each item (or switch to WhatsApp’s media‐collection API when available)
• one? – call send_attachment_message(message.attachments.first)
• else – fall back to the unsupported message error
– Change send_attachment_message to accept an attachment argument instead of relying on @message.attachments.first.
Suggested diff:
- if message.attachments.present?
- send_attachment_message
+ if message.attachments.many?
+ message.attachments.each { |att| send_attachment_message(att) }
+ elsif message.attachments.one?
+ send_attachment_message(message.attachments.first)
elsif message.content.present?
send_text_message
else
message.update!(content: I18n.t('errors.messages.send.unsupported'), status: 'failed')• Adjust the signature of send_attachment_message to accept attachment and build the payload accordingly.
• Add specs to verify delivery of multiple attachments.
This will ensure all uploaded media reach the recipient.
📝 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 send_message(phone_number, message) | |
| raise MessageContentTypeNotSupported unless message.content_type == 'text' | |
| response = HTTParty.post( | |
| "#{provider_url}/connections/#{whatsapp_channel.phone_number}/send-message", | |
| headers: api_headers, | |
| body: { | |
| type: 'text', | |
| recipient: phone_number, | |
| message: message.content | |
| }.to_json | |
| ) | |
| return unless process_response(response) | |
| response.parsed_response.dig('data', 'key', 'id') | |
| @message = message | |
| @phone_number = phone_number | |
| if message.attachments.present? | |
| send_attachment_message | |
| elsif message.content.present? | |
| send_text_message | |
| else | |
| message.update!(content: I18n.t('errors.messages.send.unsupported') |
|
| end | |
| def send_message(phone_number, message) | |
| @message = message | |
| @phone_number = phone_number | |
| if message.attachments.many? | |
| message.attachments.each { |att| send_attachment_message(att) } | |
| elsif message.attachments.one? | |
| send_attachment_message(message.attachments.first) | |
| elsif message.content.present? | |
| send_text_message | |
| else | |
| message.update!(content: I18n.t('errors.messages.send.unsupported'), status: 'failed') | |
| end | |
| end |
There was a problem hiding this comment.
-(questions/changes requested) +(approved for merge)
Reviewed 6 of 6 files at r7, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @CayoPOliveira)
There was a problem hiding this comment.
Dismissed @coderabbitai[bot] from 2 discussions.
Reviewable status:
* fix: ensure proper disconnection of Baileys provider on channel destruction * test: add callback tests for disconnecting channel provider in Whatsapp spec * refactor: simplify condition for disconnecting Baileys provider on channel destruction * refactor: enhance disconnect_channel_provider specs for Baileys provider * test: verify channel destruction does not call disconnect_channel_provider --------- Co-authored-by: gabrieljablonski <contact@gabrieljablonski.com>
There was a problem hiding this comment.
Reviewed 2 of 2 files at r8, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spec/models/channel/whatsapp_spec.rb (1)
65-100: Well-structured and comprehensive test coverageThe tests thoroughly cover the callback behavior, including success and failure scenarios for the Baileys provider and verification that non-Baileys providers don't trigger the callback. This ensures the implementation is robust against API failures.
One suggestion: Consider adding an assertion to verify that the DELETE request is actually made when the provider is 'baileys', using
expect(a_request(:delete, disconnect_url)).to have_been_made.once.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models/channel/whatsapp.rb(1 hunks)spec/models/channel/whatsapp_spec.rb(1 hunks)
🔇 Additional comments (1)
app/models/channel/whatsapp.rb (1)
40-40: Good addition of provider-specific cleanup on destroyThe conditional before_destroy callback is a good practice for ensuring proper resource cleanup. It correctly targets only the Baileys provider and delegates to the existing provider service method, maintaining separation of concerns.
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
Bug Fixes
New Features
Tests