When a duplicate room list entry is found, report it and remove it (#6006)

* When a duplicate room list entry is found, report it and remove it

* Fix tests and fixtures

* Simplify how the updates are described in the Sentry reports

features/home/impl/src/main/kotlin/io/element/android/features/home/impl/datasource/RoomListDataSource.kt

@@ -19,6 +19,7 @@ import io.element.android.libraries.matrix.api.core.RoomId
 import io.element.android.libraries.matrix.api.notificationsettings.NotificationSettingsService
 import io.element.android.libraries.matrix.api.roomlist.RoomListService
 import io.element.android.libraries.matrix.api.roomlist.RoomSummary
+import io.element.android.services.analytics.api.AnalyticsService
 import kotlinx.collections.immutable.ImmutableList
 import kotlinx.collections.immutable.toImmutableList
 import kotlinx.coroutines.CoroutineScope
@@ -31,7 +32,7 @@ import kotlinx.coroutines.flow.onEach
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
 import kotlinx.coroutines.withContext
-import timber.log.Timber
+import java.lang.IllegalStateException
 import kotlin.time.Duration.Companion.seconds
 
 @Inject
@@ -43,6 +44,7 @@ class RoomListDataSource(
     @SessionCoroutineScope
     private val sessionCoroutineScope: CoroutineScope,
     private val dateTimeObserver: DateTimeObserver,
+    private val analyticsService: AnalyticsService,
 ) {
     init {
         observeNotificationSettings()
@@ -139,11 +141,19 @@ class RoomListDataSource(
         // TODO remove once https://github.com/element-hq/element-x-android/issues/5031 has been confirmed as fixed
         val duplicates = cachingResults.filter { (_, operations) -> operations.size > 1 }
         if (duplicates.isNotEmpty()) {
-            Timber.e("Found duplicates in room summaries after an UI update: $duplicates. This could be a race condition/caching issue of some kind")
+            analyticsService.trackError(
-        }
+                IllegalStateException(
+                    "Found duplicates in room summaries after a local UI update: $duplicates. " +
+                    "This could be a race condition/caching issue of some kind"
+                )
+            )
 
+            // Remove duplicates before emitting the new values
+            _allRooms.emit(roomListRoomSummaries.distinctBy { it.roomId }.toImmutableList())
+        } else {
             _allRooms.emit(roomListRoomSummaries.toImmutableList())
         }
+    }
 
     private fun buildAndCacheItem(roomSummaries: List<RoomSummary>, index: Int): RoomListRoomSummary? {
         val roomListSummary = roomSummaries.getOrNull(index)?.let { roomListRoomSummaryFactory.create(it) }

features/home/impl/src/main/kotlin/io/element/android/features/home/impl/roomlist/RoomListStateProvider.kt

@@ -91,7 +91,7 @@ internal fun aRoomListRoomSummaryList(): ImmutableList<RoomListRoomSummary> {
             timestamp = "14:18",
             latestEvent = LatestEvent.Synced("A very very very very long message which suites on two lines"),
             avatarData = AvatarData("!id", "R", size = AvatarSize.RoomListItem),
-            id = "!roomId:domain",
+            id = "!roomId5:domain",
         ),
         aRoomListRoomSummary(
             name = "Room#2",

features/home/impl/src/test/kotlin/io/element/android/features/home/impl/datasource/RoomListDataSourceTest.kt

@@ -17,6 +17,7 @@ import io.element.android.libraries.matrix.api.roomlist.RoomListService
 import io.element.android.libraries.matrix.test.notificationsettings.FakeNotificationSettingsService
 import io.element.android.libraries.matrix.test.room.aRoomSummary
 import io.element.android.libraries.matrix.test.roomlist.FakeRoomListService
+import io.element.android.services.analytics.test.FakeAnalyticsService
 import io.element.android.tests.testutils.testCoroutineDispatchers
 import kotlinx.coroutines.test.TestScope
 import kotlinx.coroutines.test.runTest
@@ -103,5 +104,6 @@ class RoomListDataSourceTest {
         notificationSettingsService = notificationSettingsService,
         sessionCoroutineScope = backgroundScope,
         dateTimeObserver = dateTimeObserver,
+        analyticsService = FakeAnalyticsService(),
     )
 }

features/home/impl/src/test/kotlin/io/element/android/features/home/impl/roomlist/RoomListPresenterTest.kt

@@ -662,6 +662,7 @@ class RoomListPresenterTest {
             notificationSettingsService = client.notificationSettingsService,
             sessionCoroutineScope = backgroundScope,
             dateTimeObserver = FakeDateTimeObserver(),
+            analyticsService = FakeAnalyticsService(),
         ),
         searchPresenter = searchPresenter,
         sessionPreferencesStore = sessionPreferencesStore,

libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt

@@ -203,6 +203,7 @@ class RustMatrixClient(
         roomMembershipObserver = roomMembershipObserver,
         sessionCoroutineScope = sessionCoroutineScope,
         sessionDispatcher = sessionDispatcher,
+        analyticsService = analyticsService,
     )
 
     override val sessionVerificationService = RustSessionVerificationService(

libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomListFactory.kt

@@ -56,7 +56,7 @@ internal class RoomListFactory(
         val loadingStateFlow: MutableStateFlow<RoomList.LoadingState> = MutableStateFlow(RoomList.LoadingState.NotLoaded)
         val filteredSummariesFlow = MutableSharedFlow<List<RoomSummary>>(replay = 1, extraBufferCapacity = 1)
         val summariesFlow = MutableSharedFlow<List<RoomSummary>>(replay = 1, extraBufferCapacity = 1)
-        val processor = RoomSummaryListProcessor(summariesFlow, innerRoomListService, coroutineContext, roomSummaryFactory)
+        val processor = RoomSummaryListProcessor(summariesFlow, innerRoomListService, coroutineContext, roomSummaryFactory, analyticsService)
         // Makes sure we don't miss any events
         val dynamicEvents = MutableSharedFlow<RoomListDynamicEvents>(replay = 100)
         val currentFilter = MutableStateFlow(initialFilter)

libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomSummaryListProcessor.kt

@@ -9,6 +9,7 @@
 package io.element.android.libraries.matrix.impl.roomlist
 
 import io.element.android.libraries.matrix.api.roomlist.RoomSummary
+import io.element.android.services.analytics.api.AnalyticsService
 import kotlinx.coroutines.flow.MutableSharedFlow
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
@@ -18,6 +19,7 @@ import org.matrix.rustcomponents.sdk.RoomListEntriesUpdate
 import org.matrix.rustcomponents.sdk.RoomListServiceInterface
 import org.matrix.rustcomponents.sdk.use
 import timber.log.Timber
+import kotlin.collections.groupingBy
 import kotlin.coroutines.CoroutineContext
 
 class RoomSummaryListProcessor(
@@ -25,26 +27,21 @@ class RoomSummaryListProcessor(
     private val roomListService: RoomListServiceInterface,
     private val coroutineContext: CoroutineContext,
     private val roomSummaryFactory: RoomSummaryFactory,
+    private val analyticsService: AnalyticsService,
 ) {
     private val mutex = Mutex()
 
     suspend fun postUpdate(updates: List<RoomListEntriesUpdate>) {
-        updateRoomSummaries {
+        updateRoomSummaries(updates) {
             Timber.v("Update rooms from postUpdates (with ${updates.size} items) on ${Thread.currentThread()}")
             updates.forEach { update ->
                 applyUpdate(update)
             }
-
-            // TODO remove once https://github.com/element-hq/element-x-android/issues/5031 has been confirmed as fixed
-            val duplicates = groupingBy { it.roomId }.eachCount().filter { it.value > 1 }
-            if (duplicates.isNotEmpty()) {
-                Timber.e("Found duplicates in room summaries after a list update from the SDK: $duplicates. Updates: $updates")
-            }
         }
     }
 
     suspend fun rebuildRoomSummaries() {
-        updateRoomSummaries {
+        updateRoomSummaries(emptyList()) {
             forEachIndexed { i, summary ->
                 val result = buildRoomSummaryForIdentifier(summary.roomId.value)
                 if (result != null) {
@@ -112,12 +109,32 @@ class RoomSummaryListProcessor(
         }
     }
 
-    private suspend fun updateRoomSummaries(block: suspend MutableList<RoomSummary>.() -> Unit) = withContext(coroutineContext) {
+    private suspend fun updateRoomSummaries(updates: List<RoomListEntriesUpdate>, block: suspend MutableList<RoomSummary>.() -> Unit) = withContext(
+        coroutineContext
+    ) {
         mutex.withLock {
             val current = roomSummaries.replayCache.lastOrNull()
             val mutableRoomSummaries = current.orEmpty().toMutableList()
             block(mutableRoomSummaries)
-            roomSummaries.emit(mutableRoomSummaries)
+
+            // TODO remove once https://github.com/element-hq/element-x-android/issues/5031 has been confirmed as fixed
+            val uniqueRooms = mutableRoomSummaries.distinctBy { it.roomId }
+
+            if (uniqueRooms.size != mutableRoomSummaries.size) {
+                val duplicates = mutableRoomSummaries.groupingBy { it.roomId }.eachCount().filter { it.value > 1 }
+                if (duplicates.isNotEmpty()) {
+                    analyticsService.trackError(
+                        IllegalStateException(
+                            "Found duplicates in room summaries after a list update from the SDK: $duplicates. " +
+                                "Updates: ${updates.description()}"
+                        )
+                    )
+                }
+            }
+
+            roomSummaries.emit(uniqueRooms)
         }
     }
 }
+
+private fun List<RoomListEntriesUpdate>.description(): String = joinToString { it.describe() }

libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/spaces/RustSpaceRoomList.kt

@@ -12,6 +12,7 @@ import io.element.android.libraries.core.extensions.runCatchingExceptions
 import io.element.android.libraries.matrix.api.core.RoomId
 import io.element.android.libraries.matrix.api.spaces.SpaceRoom
 import io.element.android.libraries.matrix.api.spaces.SpaceRoomList
+import io.element.android.services.analytics.api.AnalyticsService
 import kotlinx.coroutines.CompletableDeferred
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.ExperimentalCoroutinesApi
@@ -32,6 +33,7 @@ class RustSpaceRoomList(
     private val innerProvider: suspend () -> InnerSpaceRoomList,
     private val coroutineScope: CoroutineScope,
     spaceRoomMapper: SpaceRoomMapper,
+    private val analyticsService: AnalyticsService,
 ) : SpaceRoomList {
     private val innerCompletable = CompletableDeferred<InnerSpaceRoomList>()
 
@@ -43,7 +45,8 @@ class RustSpaceRoomList(
         MutableStateFlow(SpaceRoomList.PaginationStatus.Idle(hasMoreToLoad = false))
     private val spaceListUpdateProcessor = SpaceListUpdateProcessor(
         spaceRoomsFlow = spaceRoomsFlow,
-        mapper = spaceRoomMapper
+        mapper = spaceRoomMapper,
+        analyticsService = analyticsService,
     )
 
     init {

libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/spaces/RustSpaceService.kt

@@ -17,6 +17,7 @@ import io.element.android.libraries.matrix.api.spaces.SpaceRoom
 import io.element.android.libraries.matrix.api.spaces.SpaceRoomList
 import io.element.android.libraries.matrix.api.spaces.SpaceService
 import io.element.android.libraries.matrix.impl.util.cancelAndDestroy
+import io.element.android.services.analytics.api.AnalyticsService
 import kotlinx.coroutines.CoroutineDispatcher
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.channels.Channel
@@ -41,12 +42,14 @@ class RustSpaceService(
     private val sessionCoroutineScope: CoroutineScope,
     private val sessionDispatcher: CoroutineDispatcher,
     private val roomMembershipObserver: RoomMembershipObserver,
+    private val analyticsService: AnalyticsService,
 ) : SpaceService {
     private val spaceRoomMapper = SpaceRoomMapper()
     override val spaceRoomsFlow = MutableSharedFlow<List<SpaceRoom>>(replay = 1, extraBufferCapacity = 1)
     private val spaceListUpdateProcessor = SpaceListUpdateProcessor(
         spaceRoomsFlow = spaceRoomsFlow,
-        mapper = spaceRoomMapper
+        mapper = spaceRoomMapper,
+        analyticsService = analyticsService,
     )
 
     override suspend fun joinedSpaces(): Result<List<SpaceRoom>> = withContext(sessionDispatcher) {
@@ -80,6 +83,7 @@ class RustSpaceService(
             innerProvider = { innerSpaceService.spaceRoomList(id.value) },
             coroutineScope = childCoroutineScope,
             spaceRoomMapper = spaceRoomMapper,
+            analyticsService = analyticsService,
         )
     }
 

libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/spaces/SpaceListUpdateProcessor.kt

@@ -9,6 +9,7 @@
 package io.element.android.libraries.matrix.impl.spaces
 
 import io.element.android.libraries.matrix.api.spaces.SpaceRoom
+import io.element.android.services.analytics.api.AnalyticsService
 import kotlinx.coroutines.flow.MutableSharedFlow
 import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.sync.Mutex
@@ -19,17 +20,18 @@ import timber.log.Timber
 internal class SpaceListUpdateProcessor(
     private val spaceRoomsFlow: MutableSharedFlow<List<SpaceRoom>>,
     private val mapper: SpaceRoomMapper,
+    private val analyticsService: AnalyticsService,
 ) {
     private val mutex = Mutex()
 
     suspend fun postUpdates(updates: List<SpaceListUpdate>) {
         Timber.v("Update space rooms from postUpdates (with ${updates.size} items) on ${Thread.currentThread()}")
-        updateSpaceRooms {
+        updateSpaceRooms(updates) {
             updates.forEach { update -> applyUpdate(update) }
         }
     }
 
-    private suspend fun updateSpaceRooms(block: MutableList<SpaceRoom>.() -> Unit) =
+    private suspend fun updateSpaceRooms(updates: List<SpaceListUpdate>, block: MutableList<SpaceRoom>.() -> Unit) =
         mutex.withLock {
             val spaceRooms = if (spaceRoomsFlow.replayCache.isNotEmpty()) {
                 spaceRoomsFlow.first().toMutableList()
@@ -37,7 +39,17 @@ internal class SpaceListUpdateProcessor(
                 mutableListOf()
             }
             block(spaceRooms)
-            spaceRoomsFlow.emit(spaceRooms)
+            val uniqueRooms = spaceRooms.distinctBy { it.roomId }
+
+            // TODO remove once https://github.com/element-hq/element-x-android/issues/5031 has been confirmed as fixed
+            if (spaceRooms.size != uniqueRooms.size) {
+                val duplicateKeys = spaceRooms.groupBy { it.roomId }.filter { it.value.size > 1 }.keys
+                analyticsService.trackError(
+                    IllegalStateException("Found duplicate keys in space rooms list ($duplicateKeys) after SDK updates: ${updates.description()}")
+                )
+            }
+
+            spaceRoomsFlow.emit(uniqueRooms)
         }
 
     private fun MutableList<SpaceRoom>.applyUpdate(update: SpaceListUpdate) {
@@ -83,3 +95,19 @@ internal class SpaceListUpdateProcessor(
         }
     }
 }
+
+private fun List<SpaceListUpdate>.description(): String = joinToString { it.description() }
+
+private fun SpaceListUpdate.description(): String = when (this) {
+    is SpaceListUpdate.Append -> "Append(${values.map { it.roomId }})"
+    SpaceListUpdate.Clear -> "Clear"
+    is SpaceListUpdate.Insert -> "Insert($index, ${value.roomId})"
+    SpaceListUpdate.PopBack -> "PopBack"
+    SpaceListUpdate.PopFront -> "PopFront"
+    is SpaceListUpdate.PushBack -> "PushBack(${value.roomId})"
+    is SpaceListUpdate.PushFront -> "PushFront(${value.roomId})"
+    is SpaceListUpdate.Remove -> "Remove($index)"
+    is SpaceListUpdate.Reset -> "Reset(${values.map { it.roomId }})"
+    is SpaceListUpdate.Set -> "Set($index, ${value.roomId})"
+    is SpaceListUpdate.Truncate -> "Truncate($length)"
+}

libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/roomlist/RoomSummaryListProcessorTest.kt

@@ -16,7 +16,9 @@ import io.element.android.libraries.matrix.impl.fixtures.fakes.FakeFfiRoomListSe
 import io.element.android.libraries.matrix.test.A_ROOM_ID
 import io.element.android.libraries.matrix.test.A_ROOM_ID_2
 import io.element.android.libraries.matrix.test.A_ROOM_ID_3
+import io.element.android.libraries.matrix.test.A_ROOM_ID_4
 import io.element.android.libraries.matrix.test.room.aRoomSummary
+import io.element.android.services.analytics.test.FakeAnalyticsService
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.test.StandardTestDispatcher
 import kotlinx.coroutines.test.TestScope
@@ -33,11 +35,10 @@ class RoomSummaryListProcessorTest {
         summaries.value = listOf(aRoomSummary())
         val processor = createProcessor()
 
-        val newEntry = aRustRoom(A_ROOM_ID_2)
+        processor.postUpdate(listOf(RoomListEntriesUpdate.Append(listOf(aRustRoom(A_ROOM_ID_2), aRustRoom(A_ROOM_ID_3), aRustRoom(A_ROOM_ID_4)))))
-        processor.postUpdate(listOf(RoomListEntriesUpdate.Append(listOf(newEntry, newEntry, newEntry))))
 
         assertThat(summaries.value.count()).isEqualTo(4)
-        assertThat(summaries.value.subList(1, 4).all { it.roomId == A_ROOM_ID_2 }).isTrue()
+        assertThat(summaries.value.subList(1, 4).map { it.roomId }).isEqualTo(listOf(A_ROOM_ID_2, A_ROOM_ID_3, A_ROOM_ID_4))
     }
 
     @Test
@@ -182,5 +183,6 @@ class RoomSummaryListProcessorTest {
         FakeFfiRoomListService(),
         coroutineContext = StandardTestDispatcher(testScheduler),
         roomSummaryFactory = RoomSummaryFactory(),
+        analyticsService = FakeAnalyticsService(),
     )
 }

libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/spaces/RoomSummaryListProcessorTest.kt

@@ -14,7 +14,9 @@ import io.element.android.libraries.matrix.impl.fixtures.factories.aRustSpaceRoo
 import io.element.android.libraries.matrix.test.A_ROOM_ID
 import io.element.android.libraries.matrix.test.A_ROOM_ID_2
 import io.element.android.libraries.matrix.test.A_ROOM_ID_3
+import io.element.android.libraries.matrix.test.A_ROOM_ID_4
 import io.element.android.libraries.previewutils.room.aSpaceRoom
+import io.element.android.services.analytics.test.FakeAnalyticsService
 import kotlinx.coroutines.flow.MutableSharedFlow
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.test.runTest
@@ -29,11 +31,14 @@ class RoomSummaryListProcessorTest {
         spaceRoomsFlow.value = listOf(aSpaceRoom())
         val processor = createProcessor()
 
-        val newEntry = aRustSpaceRoom(roomId = A_ROOM_ID_2)
+        processor.postUpdates(
-        processor.postUpdates(listOf(SpaceListUpdate.Append(listOf(newEntry, newEntry, newEntry))))
+            listOf(
+                SpaceListUpdate.Append(listOf(aRustSpaceRoom(roomId = A_ROOM_ID_2), aRustSpaceRoom(roomId = A_ROOM_ID_3), aRustSpaceRoom(roomId = A_ROOM_ID_4)))
+            )
+        )
 
         assertThat(spaceRoomsFlow.value.count()).isEqualTo(4)
-        assertThat(spaceRoomsFlow.value.subList(1, 4).all { it.roomId == A_ROOM_ID_2 }).isTrue()
+        assertThat(spaceRoomsFlow.value.subList(1, 4).map { it.roomId }).isEqualTo(listOf(A_ROOM_ID_2, A_ROOM_ID_3, A_ROOM_ID_4))
     }
 
     @Test
@@ -186,5 +191,6 @@ class RoomSummaryListProcessorTest {
     ) = SpaceListUpdateProcessor(
         spaceRoomsFlow = spaceRoomsFlow,
         mapper = SpaceRoomMapper(),
+        analyticsService = FakeAnalyticsService(),
     )
 }

libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/spaces/RustSpaceRoomListTest.kt

@@ -18,6 +18,7 @@ import io.element.android.libraries.matrix.impl.fixtures.factories.aRustSpaceRoo
 import io.element.android.libraries.matrix.impl.fixtures.fakes.FakeFfiSpaceRoomList
 import io.element.android.libraries.matrix.test.A_ROOM_ID
 import io.element.android.libraries.matrix.test.A_ROOM_ID_2
+import io.element.android.services.analytics.test.FakeAnalyticsService
 import io.element.android.tests.testutils.lambda.lambdaRecorder
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.test.TestScope
@@ -97,6 +98,7 @@ class RustSpaceRoomListTest {
             innerProvider = innerProvider,
             coroutineScope = backgroundScope,
             spaceRoomMapper = spaceRoomMapper,
+            analyticsService = FakeAnalyticsService(),
         )
     }
 }

libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/TestData.kt

@@ -46,6 +46,7 @@ val A_SPACE_ID_2 = SpaceId("!aSpaceId2:domain")
 val A_ROOM_ID = RoomId("!aRoomId:domain")
 val A_ROOM_ID_2 = RoomId("!aRoomId2:domain")
 val A_ROOM_ID_3 = RoomId("!aRoomId3:domain")
+val A_ROOM_ID_4 = RoomId("!aRoomId4:domain")
 val A_THREAD_ID = ThreadId("\$aThreadId")
 val A_THREAD_ID_2 = ThreadId("\$aThreadId2")
 val AN_EVENT_ID = EventId("\$anEventId")

services/analyticsproviders/sentry/src/main/kotlin/io/element/android/services/analyticsproviders/sentry/SentryAnalyticsProvider.kt

@@ -109,6 +109,7 @@ class SentryAnalyticsProvider(
     }
 
     override fun trackError(throwable: Throwable) {
+        Timber.e(throwable, "Sending error to Sentry")
         Sentry.captureException(throwable)
     }