From b5e95e27d495a56d1572c2b6f73644904d28178c Mon Sep 17 00:00:00 2001 From: hayaksi1 <193020925+hayaksi1@users.noreply.github.com> Date: Sun, 9 Aug 2026 03:51:38 +0300 Subject: [PATCH] Do not group timeline messages sent more than five minutes apart Consecutive messages from the same sender were grouped into one bubble block no matter how far apart they were sent, so a reply hours later looked like part of the earlier burst and lost its own timestamp header. computeGroupPosition now folds a five minute limit into the two groupability flags it already computes. The delta is symmetric between a pair of neighbours, so both items always agree on where the block splits and every branch of the existing decision table keeps working unchanged. Breaking a group on reactions, which the iOS client also does, is a separate divergence and is not part of this. Relates to https://github.com/element-hq/element-x-android/issues/4757 --- .../event/TimelineItemEventFactory.kt | 17 ++- .../factories/TimelineItemsFactoryTest.kt | 124 ++++++++++++++++++ 2 files changed, 139 insertions(+), 2 deletions(-) create mode 100644 features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/TimelineItemsFactoryTest.kt diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemEventFactory.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemEventFactory.kt index f29f23e903..d0aecbb8a0 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemEventFactory.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemEventFactory.kt @@ -38,6 +38,9 @@ import io.element.android.libraries.matrix.ui.messages.reply.map import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toImmutableList +/** Events further apart than this are never grouped together, even when sent by the same person. */ +private const val GROUPING_TIMEOUT_MS = 5 * 60 * 1000L + @AssistedInject class TimelineItemEventFactory( @Assisted private val config: TimelineItemsFactoryConfig, @@ -221,8 +224,18 @@ class TimelineItemEventFactory( val previousSender = prevTimelineItem?.event?.sender val nextSender = nextTimelineItem?.event?.sender - val previousIsGroupable = prevTimelineItem?.canBeDisplayedInBubbleBlock().orTrue() - val nextIsGroupable = nextTimelineItem?.canBeDisplayedInBubbleBlock().orTrue() + // Two events from the same sender are only grouped together if they were sent within + // GROUPING_TIMEOUT_MS of each other. The delta is symmetric between a pair of neighbours, so + // both items always agree on where the group is split. + // See https://github.com/element-hq/element-x-android/issues/4757 + val previousIsGroupable = prevTimelineItem?.let { + it.canBeDisplayedInBubbleBlock() && + currentTimelineItem.event.timestamp - it.event.timestamp <= GROUPING_TIMEOUT_MS + }.orTrue() + val nextIsGroupable = nextTimelineItem?.let { + it.canBeDisplayedInBubbleBlock() && + it.event.timestamp - currentTimelineItem.event.timestamp <= GROUPING_TIMEOUT_MS + }.orTrue() return when { previousSender != currentSender && nextSender == currentSender -> { diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/TimelineItemsFactoryTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/TimelineItemsFactoryTest.kt new file mode 100644 index 0000000000..0000ea1f07 --- /dev/null +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/factories/TimelineItemsFactoryTest.kt @@ -0,0 +1,124 @@ +/* + * Copyright (c) 2026 Element Creations Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial. + * Please see LICENSE files in the repository root for full details. + */ + +package io.element.android.features.messages.impl.timeline.factories + +import app.cash.turbine.test +import com.google.common.truth.Truth.assertThat +import io.element.android.features.messages.impl.fixtures.aTimelineItemsFactory +import io.element.android.features.messages.impl.timeline.model.TimelineItem +import io.element.android.features.messages.impl.timeline.model.TimelineItemGroupPosition +import io.element.android.libraries.matrix.api.core.UniqueId +import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem +import io.element.android.libraries.matrix.test.A_USER_ID +import io.element.android.libraries.matrix.test.timeline.aMessageContent +import io.element.android.libraries.matrix.test.timeline.anEventTimelineItem +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runTest +import org.junit.Test + +class TimelineItemsFactoryTest { + @Test + fun `messages sent close together are grouped`() = runTest { + val positions = groupPositionsOf( + timestamps = listOf(0L, ONE_MINUTE, 2 * ONE_MINUTE), + ) + assertThat(positions).containsExactly( + TimelineItemGroupPosition.First, + TimelineItemGroupPosition.Middle, + TimelineItemGroupPosition.Last, + ).inOrder() + } + + @Test + fun `messages sent more than five minutes apart are not grouped`() = runTest { + val positions = groupPositionsOf( + timestamps = listOf(0L, 6 * ONE_MINUTE, 12 * ONE_MINUTE), + ) + assertThat(positions).containsExactly( + TimelineItemGroupPosition.None, + TimelineItemGroupPosition.None, + TimelineItemGroupPosition.None, + ).inOrder() + } + + @Test + fun `a gap of more than five minutes splits the group`() = runTest { + val positions = groupPositionsOf( + timestamps = listOf(0L, ONE_MINUTE, 8 * ONE_MINUTE, 9 * ONE_MINUTE), + ) + assertThat(positions).containsExactly( + TimelineItemGroupPosition.First, + TimelineItemGroupPosition.Last, + TimelineItemGroupPosition.First, + TimelineItemGroupPosition.Last, + ).inOrder() + } + + @Test + fun `a gap of exactly five minutes still groups`() = runTest { + val positions = groupPositionsOf( + timestamps = listOf(0L, FIVE_MINUTES), + ) + assertThat(positions).containsExactly( + TimelineItemGroupPosition.First, + TimelineItemGroupPosition.Last, + ).inOrder() + } + + @Test + fun `messages with the same timestamp are grouped`() = runTest { + val positions = groupPositionsOf( + timestamps = listOf(0L, 0L, 0L), + ) + assertThat(positions).containsExactly( + TimelineItemGroupPosition.First, + TimelineItemGroupPosition.Middle, + TimelineItemGroupPosition.Last, + ).inOrder() + } + + private suspend fun TestScope.groupPositionsOf(timestamps: List): List { + val factory = aTimelineItemsFactory( + config = TimelineItemsFactoryConfig( + computeReadReceipts = false, + computeReactions = false, + ) + ) + val items = timestamps.mapIndexed { index, timestamp -> + MatrixTimelineItem.Event( + uniqueId = UniqueId("event-$index"), + event = anEventTimelineItem( + sender = A_USER_ID, + timestamp = timestamp, + content = aMessageContent(body = "Message $index"), + ), + ) + } + var positions: List = emptyList() + factory.timelineItems.test { + factory.replaceWith( + timelineItems = items, + roomMembers = emptyList(), + renderReadReceipts = false, + ) + // The factory emits the timeline newest-first; reverse it so the assertions below read + // in the same chronological order as the timestamps that were fed in. + positions = awaitItem() + .filterIsInstance() + .map { it.groupPosition } + .reversed() + cancelAndIgnoreRemainingEvents() + } + return positions + } + + private companion object { + const val ONE_MINUTE = 60 * 1000L + const val FIVE_MINUTES = 5 * ONE_MINUTE + } +}