fix(llc): prevent sending empty messages #2389
Conversation
This commit prevents the sending of messages that are considered empty by the backend. A new private method `_isMessageValidForUpload` is added to the `Channel` class. This method checks if a message has: - Non-empty text - Attachments - A quoted message ID - A poll ID If none of these conditions are met, the message is considered invalid. The `sendMessage` method now calls `_isMessageValidForUpload` before attempting to send the message to the server. If the message is invalid, it is removed from the local state, a warning is logged, and a `StreamChatError` is thrown, preventing the message from being sent.
This commit adds tests to ensure correct behavior when sending messages with attachments that are subsequently cancelled. The new tests cover the following scenarios: - **All attachments cancelled, no other content:** The message should not be sent. - **Attachment cancelled, text exists:** The message should be sent without the attachment. - **Attachment cancelled, quoted message exists:** The message should be sent without the attachment. - **Attachment cancelled, poll exists:** The message should be sent without the attachment.
WalkthroughAdds validation to channel send flow to prevent sending empty messages when all attachments are cancelled. Implements a private helper to assess message validity, short-circuits send after attachments complete, removes the local message on invalid attempts, logs a warning, throws StreamChatError, and adds tests and changelog entry. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI
participant Channel
participant Uploader
participant ClientAPI
User->>UI: tap send
UI->>Channel: sendMessage(message)
Channel->>Uploader: uploadAttachments(message.attachments)
Uploader-->>Channel: uploadsComplete (finished / cancelled)
rect rgb(230,245,255)
note over Channel: validate after uploads
Channel->>Channel: _isMessageValidForUpload(message)?
end
alt valid
Channel->>ClientAPI: sendMessage(message)
ClientAPI-->>Channel: server response
Channel-->>UI: update state -> sent
else invalid
Channel->>Channel: log warning
Channel->>Channel: remove local message (hard delete)
Channel-->>UI: throw StreamChatError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/stream_chat/CHANGELOG.md (1)
1-7: Clarify scope and cross‑link PR for traceability.Call out that this also prevents ghost/empty local messages and unintended push notifications, and add a PR link like other entries.
Apply this diff:
## Upcoming 🐞 Fixed -- Fixed `Channel.sendMessage` to prevent sending empty messages when all attachments are cancelled - during upload. +- Fixed `Channel.sendMessage` to prevent sending empty messages when all attachments are cancelled + during upload, avoiding ghost messages in the timeline and unintended push notifications. + [[#2389]](https://github.com/GetStream/stream-chat-flutter/pull/2389)packages/stream_chat/lib/src/client/channel.dart (2)
661-669: Validation logic LGTM; add doc and optionally normalize zero‑width whitespace.The criteria match the intent. Optional hardening: strip zero‑width characters so messages with only ZWSPs don’t slip through.
Apply this diff to document and harden text detection:
-bool _isMessageValidForUpload(Message message) { - final hasText = message.text?.trim().isNotEmpty == true; +/// Returns true if the message has uploadable content. +/// Keep this in sync with backend "empty message" validation semantics. +bool _isMessageValidForUpload(Message message) { + // Treat only-whitespace or only zero-width characters as empty. + final normalizedText = message.text + ?.replaceAll(RegExp(r'[\s\u200B\u200C\u200D\u2060\uFEFF]+'), ''); + final hasText = normalizedText?.isNotEmpty == true; final hasAttachments = message.attachments.isNotEmpty; final hasQuotedMessage = message.quotedMessageId != null; final hasPoll = message.pollId != null; return hasText || hasAttachments || hasQuotedMessage || hasPoll; }
728-736: Right place to gate send; improve warning context.Early bail after uploads prevents empty sends and push. Log the message id and reason for easier debugging.
Please confirm tests include: (1) all‑attachments‑cancelled with only whitespace/ZWSP text, and (2) quoted‑only and poll‑only messages.
Apply this diff:
-// Validate the final message before sending it to the server. -if (_isMessageValidForUpload(message) == false) { - client.logger.warning('Message is not valid for sending, removing it'); +// Validate the final message before sending it to the server. +if (_isMessageValidForUpload(message) == false) { + client.logger.warning( + 'Message(${message.id}) invalid (no text, attachments, quote, or poll). ' + 'Removing locally and aborting send.', + ); // Remove the message from state as it is invalid. state!.deleteMessage(message, hardDelete: true); throw const StreamChatError('Message is not valid for sending'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/stream_chat/CHANGELOG.md(1 hunks)packages/stream_chat/lib/src/client/channel.dart(2 hunks)packages/stream_chat/test/src/client/channel_test.dart(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: format
- GitHub Check: build (android)
- GitHub Check: test
- GitHub Check: analyze
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat
- GitHub Check: stream_chat_persistence
🔇 Additional comments (7)
packages/stream_chat/test/src/client/channel_test.dart (7)
246-250: LGTM on the text addition.The existing test case correctly adds a text field to a Message for the basic sendMessage functionality test, which aligns with the updated validation logic.
464-475: Comprehensive test for invalid message validation.The test correctly verifies that sending an empty message (without text, attachments, quoted message ID, or poll ID) throws a
StreamChatErrorand doesn't callclient.sendMessage. This properly tests the new validation logic.
477-517: Excellent test for cancelled attachment scenario.This test perfectly covers the core issue from the PR: when all attachments are cancelled (simulated by throwing
StreamChatNetworkErrorwithisRequestCancelledError: true), the message becomes invalid and should not be sent. The test correctly verifies thatclient.sendMessageis never called and aStreamChatErroris thrown.
519-578: Good test for text presence overriding attachment cancellation.This test validates that when an attachment upload is cancelled but the message still contains text, the message should still be sent successfully. This covers an important edge case where the validation should pass despite attachment failures.
580-644: Proper test for quoted message scenario.This test verifies that a message with a quoted message ID should be sent even if attachment uploads are cancelled, since the quoted message ID makes it a valid message. The test correctly checks that
client.sendMessageis called and the response contains the quoted message ID.
646-705: Solid test for poll ID scenario.This test covers the case where a message with a poll ID should be sent even when attachments are cancelled, since the poll ID makes it valid. The verification correctly checks that the message is sent and the poll ID is preserved in the response.
464-705: Validation logic aligns with tests
_isMessageValidForUploadreturns true if the message has text, attachments, a quoted message, or a poll—exactly matching each of the test scenarios.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2389 +/- ##
==========================================
+ Coverage 63.77% 63.81% +0.03%
==========================================
Files 413 413
Lines 25829 25837 +8
==========================================
+ Hits 16473 16488 +15
+ Misses 9356 9349 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/stream_chat/test/src/client/channel_test.dart (5)
514-522: Assert exact invocation count for clarityVerify the cancellation path triggers a single upload attempt.
Apply this diff:
verify( () => client.sendImage( any(), channelId, channelType, onSendProgress: any(named: 'onSendProgress'), cancelToken: any(named: 'cancelToken'), extraData: any(named: 'extraData'), - ), - ); + ), + ).called(1);
581-590: Tighten verifications to single callsMake intent explicit and catch accidental retries.
Apply these diffs:
verify( () => client.sendImage( any(), channelId, channelType, onSendProgress: any(named: 'onSendProgress'), cancelToken: any(named: 'cancelToken'), extraData: any(named: 'extraData'), - ), - ); + ), + ).called(1);verify( () => client.sendMessage( any(that: isSameMessageAs(message)), channelId, channelType, - ), - ); + ), + ).called(1);Also applies to: 592-599
659-667: Also assert exact call counts in quoted message flowConsistent and precise expectations help avoid flakiness.
Apply these diffs:
verify( () => client.sendImage( any(), channelId, channelType, onSendProgress: any(named: 'onSendProgress'), cancelToken: any(named: 'cancelToken'), extraData: any(named: 'extraData'), - ), - ); + ), + ).called(1);verify( () => client.sendMessage( any(that: isSameMessageAs(message)), channelId, channelType, - ), - ); + ), + ).called(1);Also applies to: 669-676
731-739: Likewise for the poll flow: enforce single invocationsKeeps checks strict and consistent.
Apply these diffs:
verify( () => client.sendImage( any(), channelId, channelType, onSendProgress: any(named: 'onSendProgress'), cancelToken: any(named: 'cancelToken'), extraData: any(named: 'extraData'), - ), - ); + ), + ).called(1);verify( () => client.sendMessage( any(that: isSameMessageAs(message)), channelId, channelType, - ), - ); + ), + ).called(1);Also applies to: 741-748
464-475: Optional: Assert local-state cleanup for invalid messageTo fully cover FLU-263/2391, consider asserting the message isn’t retained in channel.state after the failure.
Example:
await expectLater( channel.sendMessage(message), throwsA(isA<StreamChatError>()), ); expect(channel.state!.messages.any((m) => m.id == message.id), isFalse);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/stream_chat/CHANGELOG.md(1 hunks)packages/stream_chat/test/src/client/channel_test.dart(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_chat/CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_persistence
- GitHub Check: build (android)
- GitHub Check: analyze_legacy_versions
- GitHub Check: analyze
- GitHub Check: test
🔇 Additional comments (1)
packages/stream_chat/test/src/client/channel_test.dart (1)
248-248: Good fix to keep the baseline sendMessage test valid under new rulesAdding non-empty text ensures the message passes the new validation.
Submit a pull request
Fixes: FLU-263
Fixes: #2391
Description of the pull request
This commit prevents the sending of messages that are considered empty by the backend.
A new private method
_isMessageValidForUploadis added to theChannelclass. This method checks if a message has:If none of these conditions are met, the message is considered invalid.
The
sendMessagemethod now calls_isMessageValidForUploadbefore attempting to send the message to the server. If the message is invalid, it is removed from the local state, a warning is logged, and aStreamChatErroris thrown, preventing the message from being sent.Summary by CodeRabbit