Update message grouping to take time difference into account#6328
Open
Update message grouping to take time difference into account#6328
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
SDK Size Comparison 📏
|
gpunto
force-pushed
the
time-based-message-grouping
branch
from
3b41166 to
67cb599
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
WalkthroughThis pull request removes the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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
🧹 Nitpick comments (2)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageFooter.kt (1)
80-137: Add snapshot coverage for the edited-label-only branch.The new
!showMessageFooter && showEditLabelpath changes visible layout and spacing, but there isn't matching Compose regression coverage in this PR. A Paparazzi case for an edited message in the middle of a group would lock this down.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/components/messages/MessageFooter.kt` around lines 80 - 137, Add a Paparazzi snapshot test that exercises the new branch where messageItem.showMessageFooter is false and showEditLabel is true: create a test in the Compose UI test suite (matching other tests under /stream-chat-android-compose/**/*Test.kt) which composes the MessageFooter (or the top-level composable that renders this footer) with a message whose isMine flag and/or grouping produce showMessageFooter = false and showEditLabel = true, positioned as a middle-of-group message to match spacing; capture and verify the snapshot (run verifyPaparazziDebug) so the edited-label-only layout and spacing are covered and will catch regressions.stream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/feature/messages/list/MessagePositionHandlerTest.kt (1)
179-231: Add the equality boundary case at 60_000 ms.The new rule is “more than 60 seconds apart”, but these tests only cover
> 60s. An exact60_000Lcase would catch a future>=regression.🧪 Suggested test case
+ `@Test` + fun `Message should stay grouped when time difference is exactly 60 seconds`() { + val previousMessage = createMessage(user1, timeOffset = 0L) + val currentMessage = createMessage(user1, timeOffset = 60_000L) + val nextMessage = createMessage(user1, timeOffset = 120_000L) + + val result = defaultHandler.handleMessagePosition( + previousMessage, + currentMessage, + nextMessage, + isAfterDateSeparator = false, + isBeforeDateSeparator = false, + isInThread = false, + ) + + result `should be equal to` MessagePosition.MIDDLE + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/feature/messages/list/MessagePositionHandlerTest.kt` around lines 179 - 231, Add tests that cover the exact 60_000 ms boundary by mirroring the three existing cases but using timeOffset differences equal to 60_000L (e.g., previous=0L, current=60_000L, next=120_000L for the NONE-like scenario; previous=0L, current=60_000L, next=121_000L for the TOP-like scenario; previous=0L, current=1_000L, next=61_000L for the BOTTOM-like scenario) and assert the expected MessagePosition via defaultHandler.handleMessagePosition (keep isAfterDateSeparator/isBeforeDateSeparator/isInThread the same and use createMessage and MessagePosition symbols to locate the code). Ensure these tests will fail if the comparison becomes >= 60_000 instead of > 60_000.
🤖 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-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/feature/messages/list/MessagePositionHandler.kt`:
- Around line 19-28: The file-level import of
MessagePositionHandler.Companion.DEFAULT_MAX_GROUP_TIME_MILLIS is invalid
because DEFAULT_MAX_GROUP_TIME_MILLIS is declared private in the companion;
remove that import and reference DEFAULT_MAX_GROUP_TIME_MILLIS directly where
needed within MessagePositionHandler (or make the companion constant
internal/public if you intend to keep the import). Locate
DEFAULT_MAX_GROUP_TIME_MILLIS in the MessagePositionHandler companion and either
delete the file-scope import line for
MessagePositionHandler.Companion.DEFAULT_MAX_GROUP_TIME_MILLIS or change the
constant's visibility to internal/public so the import becomes valid.
---
Nitpick comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageFooter.kt`:
- Around line 80-137: Add a Paparazzi snapshot test that exercises the new
branch where messageItem.showMessageFooter is false and showEditLabel is true:
create a test in the Compose UI test suite (matching other tests under
/stream-chat-android-compose/**/*Test.kt) which composes the MessageFooter (or
the top-level composable that renders this footer) with a message whose isMine
flag and/or grouping produce showMessageFooter = false and showEditLabel = true,
positioned as a middle-of-group message to match spacing; capture and verify the
snapshot (run verifyPaparazziDebug) so the edited-label-only layout and spacing
are covered and will catch regressions.
In
`@stream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/feature/messages/list/MessagePositionHandlerTest.kt`:
- Around line 179-231: Add tests that cover the exact 60_000 ms boundary by
mirroring the three existing cases but using timeOffset differences equal to
60_000L (e.g., previous=0L, current=60_000L, next=120_000L for the NONE-like
scenario; previous=0L, current=60_000L, next=121_000L for the TOP-like scenario;
previous=0L, current=1_000L, next=61_000L for the BOTTOM-like scenario) and
assert the expected MessagePosition via defaultHandler.handleMessagePosition
(keep isAfterDateSeparator/isBeforeDateSeparator/isInThread the same and use
createMessage and MessagePosition symbols to locate the code). Ensure these
tests will fail if the comparison becomes >= 60_000 instead of > 60_000.
🪄 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: 92769122-b1ef-4762-8ff7-7e91f50af391
📒 Files selected for processing (11)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/components/messages/MessageFooter.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/messages/MessagesViewModelFactory.ktstream-chat-android-ui-common/api/stream-chat-android-ui-common.apistream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/feature/messages/list/MessageListController.ktstream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/feature/messages/list/MessagePositionHandler.ktstream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/state/messages/list/MessageFooterVisibility.ktstream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/utils/extensions/MessageFooterVisibility.ktstream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/feature/messages/list/MessageListControllerTests.ktstream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/feature/messages/list/MessagePositionHandlerTest.ktstream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/utils/extensions/MessageFooterVisibilityTest.ktstream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/viewmodel/messages/MessageListViewModelFactory.kt
💤 Files with no reviewable changes (3)
- stream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/utils/extensions/MessageFooterVisibility.kt
- stream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/state/messages/list/MessageFooterVisibility.kt
- stream-chat-android-ui-common/api/stream-chat-android-ui-common.api
...n/kotlin/io/getstream/chat/android/ui/common/feature/messages/list/MessagePositionHandler.kt
Show resolved
Hide resolved
gpunto
commented
Apr 2, 2026
| * @param formatType The type of formatting to provide. By default, it's [DateFormatType.RELATIVE]. | ||
| */ | ||
| @Composable | ||
| internal fun MessageEditedTimestamp( |
Contributor
Author
There was a problem hiding this comment.
Showing the edited timestamp is a feature that doesn't exist anymore in the new designs, so I'm also removing that
| MessageEditedTimestamp(message = message) | ||
| } | ||
| } else if (showEditLabel) { | ||
| Row( |
Contributor
Author
There was a problem hiding this comment.
"Edited" should be shown even when the rest of the footer is not shown
| * | ||
| * @param timeDifferenceMillis Time duration after which we show the message footer. | ||
| */ | ||
| public data class WithTimeDifference( |
Contributor
Author
There was a problem hiding this comment.
As mentioned in Slack, this now conflicted with the logic in MessagePositionHandler so I removed it. Note: we still have the option to remove the whole MessageFooterVisibility before releasing v7. It seems kind of an outdated concept now that:
- MessagePositionHandler handles time-based grouping
- the footer can be replaced through the component factory
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Messages from the same user that are far apart in time should not be visually grouped together. The
default
MessagePositionHandlernow breaks groups when messages are more than 60 seconds apart.Implementation
MessagePositionHandler.defaultHandlerto check the time difference between consecutivemessages when determining grouping
Testing
Open a conversation where the same user sent messages with large time gaps and verify they are no
longer grouped
Summary by CodeRabbit