Conversation
SDK Size Comparison 📏
|
gpunto
force-pushed
the
redesign/message
branch
from
5941f3f to
5343d83
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR refactors the core theming architecture for Stream Chat Android Compose by introducing a centralized Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
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/theme/StreamTypography.kt (1)
42-46: Add KDoc formetadataDefault.
StreamTypographyis a public API and the new property should be documented alongsidebodyDefaultto keep the constructor contract clear. As per coding guidelines.✍️ Suggested KDoc update
- * `@param` bodyDefault Used for body text, like message text. + * `@param` bodyDefault Used for body text, like message text. + * `@param` metadataDefault Used for metadata text, like timestamps.stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/QuotedMessage.kt (1)
184-196: Potential inconsistency:isMinecomputed differently inQuotedMessageUserName.In the parent
QuotedMessage,isMineis derived fromreplyMessage.isMine(currentUser)whenreplyMessage != null, but here inQuotedMessageUserName,isMineis always computed frommessage.isMine(currentUser). This means the text color could differ from the background color logic in certain edge cases (e.g., when quoting someone else's message in your own reply).Consider passing the already-computed
isMinefrom the parent to ensure consistent styling, similar to howQuotedMessageTextreceives theoutgoingparameter.Proposed fix
`@Composable` private fun QuotedMessageUserName( message: Message, replyMessage: Message?, currentUser: User?, + outgoing: Boolean, ) { - val isMine = message.isMine(currentUser) - val userName = if (isMine) { + val userName = if (message.isMine(currentUser)) { stringResource(R.string.stream_compose_quoted_message_you) } else if (replyMessage == null) { stringResource(R.string.stream_compose_quoted_message_reply_to, message.user.name) } else { message.user.name } Text( text = userName, fontWeight = FontWeight.SemiBold, - color = MessageStyling.textColor(outgoing = isMine), + color = MessageStyling.textColor(outgoing = outgoing), maxLines = 1, overflow = TextOverflow.Ellipsis, ) }And update the call site at line 145:
- QuotedMessageUserName(message, replyMessage, currentUser) + QuotedMessageUserName(message, replyMessage, currentUser, outgoing = isMine)
🤖 Fix all issues with AI agents
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/StreamColors.kt`:
- Around line 161-165: Update the KDoc for StreamColors to reflect the current
public color tokens: remove mentions of removed properties like linkBackground
and chatTextMessage, and add descriptions for textTertiary and the new chatText*
properties (e.g., chatTextPrimary, chatTextSecondary, etc.) as well as
stateBgDisabled and stateTextDisabled; ensure each property (textPrimary,
textSecondary, textTertiary, stateBgDisabled, stateTextDisabled and all
chatText* tokens) has a concise, guideline-compliant one-line description that
matches their usage and intent in the API.
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/util/MessageTextFormatter.kt`:
- Line 34: The TODO comment in MessageTextFormatter.kt ("// TODO [G.] do we need
this?") must be resolved before merging: either remove the TODO if it's no
longer relevant, or replace it with a clear actionable comment referencing an
issue ID (create an issue if none exists) and a short rationale for why the code
remains; locate the placeholder in MessageTextFormatter (the top-level
file/comment) and update or remove the line so production code no longer
contains an unanswered TODO.
🧹 Nitpick comments (2)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageText.kt (1)
182-191: Consider using@StreamPreviewfor Compose previews.Based on learnings, Compose previews in this module should use
@StreamPreviewhelpers for consistency.stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageBubble.kt (1)
75-81: Consider simplifying redundant branches.The
whenexpression has three branches (isDeleted,isErrorOrFailed,else) that all return the same value:MessageStyling.backgroundColor(outgoing). This could be simplified unless the explicit branches are intentional placeholders for future differentiation.Simplified version
`@Composable` internal fun getMessageBubbleColor(outgoing: Boolean, message: Message): Color { return when { message.isGiphyEphemeral() -> ChatTheme.colors.giphyMessageBackground - message.isDeleted() -> MessageStyling.backgroundColor(outgoing) - message.isErrorOrFailed() -> MessageStyling.backgroundColor(outgoing) else -> MessageStyling.backgroundColor(outgoing) } }
...hat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/StreamColors.kt
Show resolved
Hide resolved
...roid-compose/src/main/java/io/getstream/chat/android/compose/ui/util/MessageTextFormatter.kt
Outdated
Show resolved
Hide resolved
gpunto
force-pushed
the
redesign/message
branch
from
5343d83 to
8940844
Compare
gpunto
force-pushed
the
redesign/message
branch
from
8940844 to
89f7c5a
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/theme/StreamTypography.kt (1)
42-46: DocumentmetadataDefaultin the KDoc.Line 42-46 lists
bodyDefaultbut omitsmetadataDefault. Please add it to keep the public API documentation complete.✍️ Suggested KDoc update
- * `@param` bodyDefault Used for body text, like message text. + * `@param` bodyDefault Used for body text, like message text. + * `@param` metadataDefault Used for low-emphasis metadata, like timestamps or labels.As per coding guidelines, please keep public API KDoc in sync.
🧹 Nitpick comments (2)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageFooter.kt (1)
119-123: Consider reusing thetextStylevariable.Line 89 already computes
val textStyle = MessageStyling.timestampStyle(). Consider reusing it here instead of callingMessageStyling.timestampStyle()again.♻️ Suggested change
Timestamp( date = date, formatType = DateFormatType.TIME, - textStyle = MessageStyling.timestampStyle() + textStyle = textStyle )stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageBubble.kt (1)
74-81: SimplifygetMessageBubbleColorcontrol flow.Line 78-80 return the same value; you can collapse to a single
elseto reduce redundancy.♻️ Suggested simplification
return when { message.isGiphyEphemeral() -> ChatTheme.colors.giphyMessageBackground - message.isDeleted() -> MessageStyling.backgroundColor(outgoing) - message.isErrorOrFailed() -> MessageStyling.backgroundColor(outgoing) - else -> MessageStyling.backgroundColor(outgoing) + else -> MessageStyling.backgroundColor(outgoing) }
gpunto
force-pushed
the
redesign/message
branch
from
89f7c5a to
3208e73
Compare
|
andremion
left a comment
andremion
left a comment
There was a problem hiding this comment.
Good job!
I'm wondering why that e2e testing is failing here... 🤔
It seems it was flakiness 🙃 |
2 participants
🎯 Goal
Initial work for updating the message design. This PR includes updating bubble color & shapes, text styles, spacing.
🛠 Implementation details
MessageTheme, since we're not going to have intermediate themes anymore.🎨 UI Changes
🧪 Testing
Can be seen in action in the sample
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.