Conversation
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
gpunto
force-pushed
the
redesign/update-media-attachment-sizing
branch
from
5bcf28f to
b10882b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR introduces message alignment support throughout the message rendering pipeline. The changes add a Changes
Sequence Diagram(s)sequenceDiagram
participant MessageContainer as MessageContainer<br/>(Composable)
participant Theme as ChatTheme
participant Provider as messageAlignmentProvider
participant MessageContent as MessageContent<br/>(Composable)
participant DefaultMessageContent as DefaultMessageContent<br/>(Composable)
participant Layout as Column Layout
MessageContainer->>Theme: Access ChatTheme
Theme->>Provider: Query messageAlignmentProvider
Provider-->>Theme: Return MessageAlignment
Theme-->>MessageContainer: Receive alignment
MessageContainer->>MessageContent: Call with messageAlignment
MessageContent->>DefaultMessageContent: Pass messageAlignment param
DefaultMessageContent->>Layout: Apply alignment to Column<br/>(horizontalAlignment)
Layout-->>DefaultMessageContent: Render aligned message content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
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)
71-97:⚠️ Potential issue | 🟡 MinorAdd missing KDoc for the new
messageAlignmentpublic parameter.
MessageContentnow exposesmessageAlignment, but the KDoc block doesn’t describe it.As per coding guidelines: "`**/*.kt`: Document public APIs with KDoc, including thread expectations and state notes".💡 Suggested doc patch
/** * Represents the default message content within the bubble that can show different UI based on the message state. * * `@param` message The message to show. * `@param` currentUser The currently logged in user. * `@param` modifier Modifier for styling. * `@param` onLongItemClick Handler when the item is long clicked. * `@param` onGiphyActionClick Handler for Giphy actions. * `@param` onQuotedMessageClick Handler for quoted message click action. + * `@param` messageAlignment The horizontal alignment of regular message content within the bubble. * `@param` onLinkClick Handler for clicking on a link in the message. * `@param` onMediaGalleryPreviewResult Handler when the user selects an option in the Media Gallery Preview screen. * `@param` giphyEphemeralContent Composable that represents the default Giphy message content. * `@param` deletedMessageContent Composable that represents the default content of a deleted message. * `@param` regularMessageContent Composable that represents the default regular message content, such as attachments and * text. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageContent.kt` around lines 71 - 97, The KDoc for the public composable MessageContent is missing documentation for the new parameter messageAlignment; update the KDoc block above the MessageContent function to add an `@param` entry describing messageAlignment (its type MessageAlignment, what it controls—bubble alignment/start vs end, default behavior MessageAlignment.Start—and any thread/state expectations or side effects), ensuring the `@param` is consistent with other parameter descriptions and follows project KDoc conventions.stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactoryParams.kt (1)
798-807:⚠️ Potential issue | 🟠 MajorAdd changelog entry for MessageRegularContentParams constructor change.
The
messageAlignmentparameter added toMessageRegularContentParamsis not yet documented in CHANGELOG.md. Add an entry to the unreleasedstream-chat-android-composesection documenting this breaking API change so integrators can quickly identify and address it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactoryParams.kt` around lines 798 - 807, Add a changelog entry documenting the breaking API change: the MessageRegularContentParams data class now includes a new messageAlignment parameter. Update CHANGELOG.md in the unreleased "stream-chat-android-compose" section with a short bullet under "Changed" (or "Breaking changes") that names MessageRegularContentParams and describes the new required parameter so integrators know to update their usages.
🧹 Nitpick comments (1)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/MediaAttachmentContent.kt (1)
649-668: Please lock the new ratio branches down with a snapshot test.This change introduces distinct landscape / portrait / square sizing paths, so a Paparazzi case for ratios like
1f,1.1f, and0.9fwould make future UI regressions much easier to catch.Based on learnings: Applies to /stream-chat-android-compose//*Test.kt : Add Paparazzi snapshots for Compose UI regressions and run
verifyPaparazziDebug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/MediaAttachmentContent.kt` around lines 649 - 668, Add Paparazzi snapshot tests that lock the three sizing branches of singleMediaAttachmentSize by rendering a Compose component that uses singleMediaAttachmentSize with aspect ratios 1f (square), 1.1f (landscape), and 0.9f (portrait) and verifying their layouts; create tests in a Compose test class (e.g., MediaAttachmentContentTest) asserting snapshots for each ratio and run verifyPaparazziDebug to commit the golden images so future changes to singleMediaAttachmentSize (and related UI in MediaAttachmentContent composables) will be caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@stream-chat-android-compose/api/stream-chat-android-compose.api`:
- Line 1669: The public API change inserts MessageAlignment into MessageContent
and reorders MessageRegularContentParams so messageAlignment is now
component3(), which breaks implementations of
ChatComponentFactory.MessageRegularContent that rely on prior positional args or
destructuring; update the repository docs (CHANGELOG.md or add
MIGRATION_TO_V7.md) to document the breaking change and provide concrete
migration steps: explain that MessageAlignment must be accepted as a new
parameter in MessageContent call sites and component constructors, show how to
update destructuring (or replace with named parameters) to account for the new
third position, and include example before/after snippets and note the same
update applies to the other occurrences referenced (around the same API region).
---
Outside diff comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageContent.kt`:
- Around line 71-97: The KDoc for the public composable MessageContent is
missing documentation for the new parameter messageAlignment; update the KDoc
block above the MessageContent function to add an `@param` entry describing
messageAlignment (its type MessageAlignment, what it controls—bubble
alignment/start vs end, default behavior MessageAlignment.Start—and any
thread/state expectations or side effects), ensuring the `@param` is consistent
with other parameter descriptions and follows project KDoc conventions.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactoryParams.kt`:
- Around line 798-807: Add a changelog entry documenting the breaking API
change: the MessageRegularContentParams data class now includes a new
messageAlignment parameter. Update CHANGELOG.md in the unreleased
"stream-chat-android-compose" section with a short bullet under "Changed" (or
"Breaking changes") that names MessageRegularContentParams and describes the new
required parameter so integrators know to update their usages.
---
Nitpick comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/MediaAttachmentContent.kt`:
- Around line 649-668: Add Paparazzi snapshot tests that lock the three sizing
branches of singleMediaAttachmentSize by rendering a Compose component that uses
singleMediaAttachmentSize with aspect ratios 1f (square), 1.1f (landscape), and
0.9f (portrait) and verifying their layouts; create tests in a Compose test
class (e.g., MediaAttachmentContentTest) asserting snapshots for each ratio and
run verifyPaparazziDebug to commit the golden images so future changes to
singleMediaAttachmentSize (and related UI in MediaAttachmentContent composables)
will be caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c8734775-ba13-4eb5-a593-afb1ccb046ab
⛔ Files ignored due to path filters (1)
stream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.attachments.content_AttachmentsContentTest_single_media_attachment_content.pngis excluded by!**/*.png
📒 Files selected for processing (6)
stream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/attachments/content/MediaAttachmentContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageContent.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/messages/list/MessageContainer.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactory.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactoryParams.kt
| 400.dp | ||
| } else { | ||
| 600.dp | ||
| private fun singleMediaAttachmentSize(aspectRatio: Float): DpSize = when { |
There was a problem hiding this comment.
Now that we have somewhat fixed dimensions for the image, is it possible to also size the loading item in the same way (currently it seems it is always displayed as 256x256)?
I didn't look too much in how difficult this would be to extract the width/height from local files, but perhaps it is something that we can consider as an improvement in the future!
There was a problem hiding this comment.
Good point. I also don't know what it'd take, but let me see!
There was a problem hiding this comment.
I added a step to resolve originalWidth/Height locally when resolving attachment files so they're always there even during upload
There was a problem hiding this comment.
Awesome, thank you very much! I assume this doesn't cause any StrictMode violations, or should we run it on a background scope?
There was a problem hiding this comment.
We are fine because resolveAttachmentFiles (which calls this new function) is marked with @WorkerThread and is executed with the IO dispatcher at the usage sites.
TBH the usage is a bit confusing because resolveAttachmentFiles is passed around as a suspend lambda, which would signal that it's safe to call from any thread. As an improvement I think we could make it suspend at the source and set the correct dispatcher, but I guess that could be a different PR. WDYT?
There was a problem hiding this comment.
Oh I missed that part sorry. Of course, no need to cram it in this PR. BTW I just realised, since we are calculating the oh/ow on Attachment level - are those values submitted to the BE as well? (for example in sendMessage) Is it possible that this breaks some of the existing behaviour?
There was a problem hiding this comment.
So, this is the summary as I understand it (with Claude's help exploring the backend codebase):
The values now get effectively sent to the bakcend when we send the message, but this shouldn't be problematic because the scenario is:
- The SDK uploads files to CDN, getting back a URL that contains ow/oh
- The message is then sent with both the CDN URL and the locally-resolved dimensions
- The backend overwrites original_width/original_height by extracting them from the CDN URL during RefreshMessageAttachmentURLs()
So the locally-resolved values are transient (and they should be correct while they exist). Still, we could reset them to null in augmentAttachmentOnSuccess, but that sounds a bit confusing to me
gpunto
force-pushed
the
redesign/update-media-attachment-sizing
branch
from
173f43b to
7fb0c77
Compare
|
2 participants
Goal
The sizing logic for media attachment was incorrect. They should have their wider side limited to a max of 256dp and the smaller side to 192dp.
Implementation
🎨 UI Changes
Testing
Check messages with media attachments to see that they match the expected size
Summary by CodeRabbit
New Features
Bug Fixes