fix(llc): only include uploaded attachments in draft #2388
Conversation
This commit modifies the `toDraftMessage` method in the `Message` class to ensure that only attachments that have been successfully uploaded are included in the resulting `DraftMessage`. Previously, all attachments, regardless of their upload status, were included. This change filters the attachments, keeping only those where `uploadState.isSuccess` is true.
WalkthroughRestricts Message.toDraftMessage to include only attachments with a successful uploadState when creating DraftMessage. Updates tests to set explicit uploadState and adds a new test verifying the filtering. Adds a CHANGELOG entry documenting the change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Composer as Message Composer
participant Model as Message.toDraftMessage()
participant Draft as DraftMessage
User->>Composer: Compose message with attachments
Composer->>Model: Convert Message to Draft
rect rgb(230,245,255)
note over Model: New behavior
Model->>Model: Filter attachments where uploadState == success
end
Model->>Draft: Create DraftMessage(attachments: filtered)
Draft-->>Composer: Return DraftMessage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (6)
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 (5)
packages/stream_chat/CHANGELOG.md (1)
10-10: Tighten wording and reference the scope (method + state).Consider clarifying the entry and referencing the ticket explicitly.
-- Fixed `toDraftMessage` to only include successfully uploaded attachments in draft messages. +- Fixed `Message.toDraftMessage` to include only attachments with `UploadState.success` in draft messages (FLU-262).packages/stream_chat/lib/src/core/models/draft_message.dart (2)
187-191: Docstring mismatch (extension and method direction).This converts a Message to a DraftMessage, not the other way around.
- /// Converts this [DraftMessage] to a [Message]. + /// Converts this [Message] to a [DraftMessage].
192-196: Optional: inline filter to reduce temp allocation.No behavioral change; simplifies the method.
- final uploadedAttachments = attachments - .where((it) => it.uploadState?.isSuccess ?? true) - .toList(); - return DraftMessage( id: id, text: text, type: type, - attachments: uploadedAttachments, + attachments: + attachments.where((it) => it.uploadState?.isSuccess ?? true).toList(),Also applies to: 201-201
packages/stream_chat/test/src/core/models/draft_message_test.dart (2)
4-4: Remove unused import.
attachment_file.dartisn’t referenced in this test suite.-import 'package:stream_chat/src/core/models/attachment_file.dart';
359-399: Add a case for attachments with no uploadState (e.g., OG/link embeds).To prevent regressions, cover attachments where
uploadStateis null—they should remain included.Suggested test (add alongside this group):
test('includes attachments with null uploadState (remote/OG)', () { final remoteImage = Attachment(id: 'og-1', type: 'image'); // no uploadState final msg = Message(id: 'm1', text: 'link', attachments: [remoteImage]); final draft = msg.toDraftMessage(); expect(draft.attachments, hasLength(1)); expect(draft.attachments.first.id, 'og-1'); });
📜 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/core/models/draft_message.dart(1 hunks)packages/stream_chat/test/src/core/models/draft_message_test.dart(3 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). (10)
- GitHub Check: stream_chat
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_persistence
- GitHub Check: analyze_legacy_versions
- GitHub Check: build (ios)
- GitHub Check: build (android)
- GitHub Check: test
- GitHub Check: analyze
🔇 Additional comments (3)
packages/stream_chat/test/src/core/models/draft_message_test.dart (2)
307-309: LGTM: align fixtures with new filter.Marking attachments as
UploadState.success()keeps parity with the updated conversion behavior.
402-404: LGTM: round‑trip consistency.Success-state fixtures ensure symmetry for DraftMessage → Message.
packages/stream_chat/lib/src/core/models/draft_message.dart (1)
192-196: No NPE — uploadState is defaulted to success; change not neededGenerated deserialization and the OG-constructor set missing upload_state to const UploadState.success (packages/stream_chat/lib/src/core/models/attachment.g.dart:38–40; packages/stream_chat/lib/src/core/models/attachment.dart:81–84), so uploadState is effectively non-null and calling .isSuccess is safe; the proposed null-guard is unnecessary and would alter semantics.
Likely an incorrect or invalid review comment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2388 +/- ##
==========================================
+ Coverage 63.76% 63.78% +0.01%
==========================================
Files 412 412
Lines 25821 25823 +2
==========================================
+ Hits 16466 16470 +4
+ Misses 9355 9353 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xsahil03x
force-pushed
the
fix/draft-attachments
branch
from
21411ee to
c5edb31
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
packages/stream_chat/lib/src/client/channel.dart (1)
728-736: Block empty final payloads after uploads; also pre‑validate to avoid UI flickerThe post-upload guard and hard delete are correct and align with the tests. Two small nits:
- Prefer
!_isMessageValidForUpload(message)over== false.- Optional: add an early guard (before
state!.updateMessage(message)) when there are no attachments to upload, to avoid briefly emitting a “sending” message that is immediately deleted.Example tweak:
@@ - state!.updateMessage(message); + // If there are no attachments to upload, we can pre-validate to avoid flicker. + if (message.attachments.isEmpty && !_isMessageValidForUpload(message)) { + throw const StreamChatError('Message is not valid for sending'); + } + state!.updateMessage(message); @@ - if (_isMessageValidForUpload(message) == false) { + if (!_isMessageValidForUpload(message)) { client.logger.warning('Message is not valid for sending, removing it'); state!.deleteMessage(message, hardDelete: true); throw const StreamChatError('Message is not valid for sending'); }packages/stream_chat/test/src/client/channel_test.dart (5)
464-475: Test: reject invalid empty messageCovers the new guard and ensures the client API isn’t called. Consider also asserting the message is removed from state to fully validate the hard-delete behavior.
478-517: Test: all attachments cancelled → rejectGreat coverage of the cancellation path. Same optional enhancement: assert the message is removed from
messagesStreamafter the error.
519-578: Test: cancelled attachment but text exists → send succeedsNice. To strengthen it, verify the payload sent to
client.sendMessagehas attachments filtered out (empty), not just that the call occurred.
580-644: Test: cancelled attachment but quoted message exists → send succeedsLooks good. Same suggestion: assert sent message’s attachments are empty.
646-705: Test: cancelled attachment but poll exists → send succeedsGood scenario coverage. Consider verifying that attachments are removed in the sent payload.
📜 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)
🚧 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: test
- GitHub Check: analyze
- GitHub Check: build (android)
- GitHub Check: build (ios)
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat
- GitHub Check: stream_chat_persistence
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter_core
🔇 Additional comments (2)
packages/stream_chat/lib/src/client/channel.dart (1)
661-668: Solid pre-send validity check helperClear, fast check for non-empty text, attachments, quoted message, or poll. Reads well and matches product intent.
packages/stream_chat/test/src/client/channel_test.dart (1)
248-248: Add text to baseline sendMessage testGood adjustment to keep the message valid under the new guard.
Submit a pull request
Fixes: FLU-262
Description of the pull request
This commit modifies the
toDraftMessagemethod in theMessageclass to ensure that only attachments that have been successfully uploaded are included in the resultingDraftMessage.Previously, all attachments, regardless of their upload status, were included. This change filters the attachments, keeping only those where
uploadState.isSuccessis true.Summary by CodeRabbit
Bug Fixes
Documentation