Use component factory when rendering media attachment in messages#6124
Use component factory when rendering media attachment in messages#6124
Conversation
gpunto
force-pushed
the
redesign/message-restructure-2
branch
from
104591f to
54a3609
Compare
SDK Size Comparison 📏
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR refactors Stream Chat Android Compose's attachment handling by removing callback dependencies from factory constructors, centralizing message and interaction state through Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageContent.kt (1)
188-242:⚠️ Potential issue | 🟡 MinorMixed attachments with different types are dropped when not uploading.
The refactoring correctly prevents re-rendering media/files—
MediaAttachmentFactoryandFileAttachmentFactoryboth have emptycontent()implementations. However, when a message contains mixed attachment types (e.g., images + audio recordings) without active uploads, attachments that don't match a single factory'scanHandle()logic are silently dropped.For example, a message with one image and one audio recording will:
- Render the image directly via
componentFactory.MediaAttachmentContent()- Call
MessageAttachmentsContent(message)with the full message- Fail to match any factory for the mixed list (
MediaAttachmentFactoryrequires all images/video;AudioRecordAttachmentFactoryrequires all recordings)- Fall back to
UnsupportedAttachmentFactory, displaying a generic error while the recording is lostUpload flows are unaffected—when
hasUploads=true, onlyMessageAttachmentsContentprocesses the message and factories can extract relevant attachments from the fullmessage.attachments.stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/StreamAttachmentFactories.kt (1)
97-136:⚠️ Potential issue | 🟡 MinorUnused parameters in deprecated method.
The
skipEnrichUrl(line 97) andonMediaContentItemClick(lines 104-113) parameters are still accepted and documented in KDoc but are no longer passed toMediaAttachmentFactory(). This means users providing custom values will have them silently ignored.Consider either:
- Removing these parameters from the deprecated signature (breaking change, but acceptable for deprecated API), or
- Updating the KDoc to explicitly note these parameters are ignored in this deprecation cycle
🤖 Fix all issues with AI agents
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/MessageAttachmentsContent.kt`:
- Around line 63-69: attachmentState is created with filteredAttachments =
emptyList(), which causes attachment content to render nothing; instead set
filteredAttachments to the attachments that match the chosen factory (e.g.,
filter message.attachments by the selected factory’s predicate or call the
factory’s handler method). Locate where attachmentState is constructed in
MessageAttachmentsContent.kt and replace emptyList() with the list of
attachments for the selected factory (use the existing
selectedFactory/attachmentFactory variable or call the factory’s
canHandle/matches method to filter message.attachments) so
state.filteredAttachments contains the actual attachments to render.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageContent.kt`:
- Around line 163-164: Add a brief inline comment above the
`@Suppress`("LongMethod") on the DefaultMessageContent function explaining why the
suppression is necessary: note that DefaultMessageContent contains multiple
distinct conditional rendering paths for different attachment types (media,
files, quoted messages, reactions, previews), which increases method length for
clarity and to keep rendering logic co-located; document that refactoring would
split UI semantics and reduce readability. Reference: DefaultMessageContent.
🧹 Nitpick comments (6)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/GiphyAttachmentContent.kt (1)
319-346: Use@StreamPreviewfor Compose previews.The preview still relies on
@Preview. Please switch to the Stream preview helper to follow project conventions.♻️ Suggested change
-@Preview(showBackground = true) +@StreamPreview `@Composable` private fun GiphyAttachmentContentPreview() {Based on learnings: Compose previews should use
StreamPreviewhelpers.stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/LinkAttachmentContent.kt (1)
288-315: Use@StreamPreviewfor Compose previews.Please replace the
@Previewannotation with the Stream preview helper to match project conventions.♻️ Suggested change
-@Preview(showBackground = true) +@StreamPreview `@Composable` private fun LinkAttachmentContentPreview() {Based on learnings: Compose previews should use
StreamPreviewhelpers.stream-chat-android-compose/api/stream-chat-android-compose.api (4)
317-327: Potential source/binary break from reorderedAttachmentStatecomponents.
filteredAttachmentsis inserted ahead of existing fields, shifting constructor/copy/component ordering. External destructuring or positional copy calls will break. If compatibility within v7 minors is a goal, consider appending the new field at the end (with a default) or providing a deprecated overload + migration notes.
471-472: Public API change inStreamAttachmentFactories.defaults—confirm migration path.The signature now takes a
Listand removes/reshuffles callback parameters, which will break callers. Please confirm sample usage/docs are updated or keep a deprecated overload that delegates to the new signature to ease upgrades.
2840-2840: NewChatComponentFactory.MediaAttachmentContentrequires implementer updates.Adding an abstract method is binary/source breaking for custom factories. Please ensure migration guidance is included, or provide a fallback adapter/default to reduce friction for external implementers.
2919-2919:MessageTextContentsignature reorder is source-breaking.Moving
Modifierto the first parameter changes positional/named call sites and custom factory implementations. Consider a deprecated overload for one release to smooth migration.
...n/java/io/getstream/chat/android/compose/ui/attachments/content/MessageAttachmentsContent.kt
Show resolved
Hide resolved
...ose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageContent.kt
Show resolved
Hide resolved
...rc/main/java/io/getstream/chat/android/compose/state/messages/attachments/AttachmentState.kt
Outdated
Show resolved
Hide resolved
gpunto
force-pushed
the
redesign/message-restructure-2
branch
from
f6ee076 to
37e1d72
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/AudioRecordAttachmentContent.kt (1)
140-145:⚠️ Potential issue | 🟡 MinorUnused parameter
getCurrentUserId.The
getCurrentUserIdparameter is defined but never used in the function body. SinceisMineis now derived fromattachmentState.isMine, this parameter appears to be dead code.Consider deprecating or removing this parameter, or updating the KDoc to indicate it's no longer used.
🧹 Nitpick comments (3)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactory.kt (2)
1275-1291: Update KDoc to include the newmodifierparameter (and param docs).The signature now adds
modifier, but the public API KDoc doesn’t reflect it. Consider updating the KDoc to list parameters (and any thread/state expectations if relevant).📝 Suggested KDoc update
/** * The default message text content. * Usually with extra styling and padding for the chat bubble. + * + * `@param` modifier Modifier for styling. + * `@param` message Message to show. + * `@param` currentUser The currently logged in user. + * `@param` onLongItemClick Handler used for long pressing on the message text. + * `@param` onLinkClick Handler used for clicking on a link in the message. + * `@param` onUserMentionClick Handler used for clicking on a user mention. */As per coding guidelines: Document public APIs with KDoc, including thread expectations and state notes.
2931-2940: Add KDoc for the new publicMediaAttachmentContent.This is a new public API entry point and should be documented with parameters and any state/thread expectations.
📝 Suggested KDoc addition
+ /** + * Factory method for creating media attachment content. + * + * `@param` state The attachment state for the message being rendered. + * `@param` modifier Modifier for styling. + */ `@Composable` public fun MediaAttachmentContent( state: AttachmentState, modifier: Modifier, ) {As per coding guidelines: Document public APIs with KDoc, including thread expectations and state notes.
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/MediaAttachmentContent.kt (1)
800-832: Use@StreamPreviewfor Compose previews.
These previews still use@Preview; switch to the StreamPreview helpers for consistency.Based on learnings: Compose previews should use
StreamPreviewhelpers.
gpunto
force-pushed
the
redesign/message-restructure-2
branch
from
6731eb7 to
14d8866
Compare
gpunto
force-pushed
the
redesign/message-restructure-2
branch
from
14d8866 to
e7a7ba9
Compare
|
2 participants
🎯 Goal
Continuing with the message content structural changes started in #6119, this time for media attachments.
🛠 Implementation details
AddfilteredAttachmentsproperty toAttachmentStateto avoid having to filter multiple times and simplify implementation of attachment composables🎨 UI Changes
The most notable change is that now images & files are not shown with the file UI in case of mixed content
🧪 Testing
Can be checked in the sample
Summary by CodeRabbit
New Features
Improvements
Refactor