Merge pull request #7935 from element-hq/feature/bma/fixBadRoomId

Fix invalid room id built from thread notification tags

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

@@ -821,7 +821,7 @@ class RustMatrixClient(
 
     override suspend fun getRoomInfo(roomId: RoomId): Result<RoomInfo?> = withContext(sessionDispatcher) {
         runCatchingExceptions {
-            innerRoomListService.roomOrNull(roomId.value)?.use { room ->
+            innerRoomListService.roomOrNull(roomId)?.use { room ->
                 roomInfoMapper.map(room.roomInfo())
             }
         }
@@ -829,7 +829,7 @@ class RustMatrixClient(
 
     override fun getRoomInfoFlow(roomId: RoomId): Flow<Optional<RoomInfo>> {
         return mxCallbackFlow {
-            val roomNotFound = innerRoomListService.roomOrNull(roomId.value).use { it == null }
+            val roomNotFound = innerRoomListService.roomOrNull(roomId).use { it == null }
             if (roomNotFound) {
                 channel.send(Optional.empty())
             }

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

@@ -49,7 +49,7 @@ class RoomContentForwarder(
             val message = content.content.kind as? MsgLikeKind.Message ?: throw ForwardEventException(toRoomIds)
             contentWithoutRelationFromMessage(message.content)
         }
-        val targetRooms = toRoomIds.toSet().mapNotNull { roomId -> roomListService.roomOrNull(roomId.value) }
+        val targetRooms = toRoomIds.toSet().mapNotNull { roomId -> roomListService.roomOrNull(roomId) }
         val failedForwardingTo = mutableSetOf<RoomId>()
         targetRooms.parallelMap { room ->
             room.use { targetRoom ->

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

@@ -187,11 +187,11 @@ class RustRoomFactory(
      * Get the Rust room for a room, retrying after the room list is loaded if necessary.
      */
     private suspend fun awaitRoomInRoomList(roomId: RoomId): Room? {
-        var sdkRoom = innerRoomListService.roomOrNull(roomId.value)
+        var sdkRoom = innerRoomListService.roomOrNull(roomId)
         if (sdkRoom == null) {
             // ... otherwise, lets wait for the SS to load all rooms and check again.
             roomListService.allRooms.awaitLoaded()
-            sdkRoom = innerRoomListService.roomOrNull(roomId.value)
+            sdkRoom = innerRoomListService.roomOrNull(roomId)
         }
 
         if (sdkRoom == null) {

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

@@ -9,6 +9,7 @@
 package io.element.android.libraries.matrix.impl.roomlist
 
 import io.element.android.libraries.core.data.tryOrNull
+import io.element.android.libraries.matrix.api.core.RoomId
 import io.element.android.libraries.matrix.impl.util.cancelAndDestroy
 import io.element.android.libraries.matrix.impl.util.mxCallbackFlow
 import kotlinx.coroutines.channels.Channel
@@ -109,10 +110,10 @@ internal fun RoomListServiceInterface.syncIndicator(): Flow<RoomListServiceSyncI
         )
     }.buffer(Channel.UNLIMITED)
 
-internal fun RoomListServiceInterface.roomOrNull(roomId: String): Room? {
+internal fun RoomListServiceInterface.roomOrNull(roomId: RoomId): Room? {
     return tryOrNull(
         onException = { Timber.e(it, "Failed finding room with id=$roomId.") }
     ) {
-        room(roomId)
+        room(roomId.value)
     }
 }

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

@@ -8,6 +8,7 @@
 
 package io.element.android.libraries.matrix.impl.roomlist
 
+import io.element.android.libraries.matrix.api.core.RoomId
 import io.element.android.libraries.matrix.api.roomlist.RoomSummary
 import io.element.android.services.analytics.api.AnalyticsService
 import kotlinx.coroutines.flow.MutableSharedFlow
@@ -43,7 +44,7 @@ class RoomSummaryListProcessor(
     suspend fun rebuildRoomSummaries() {
         updateRoomSummaries(emptyList()) {
             forEachIndexed { i, summary ->
-                val result = buildRoomSummaryForIdentifier(summary.roomId.value)
+                val result = buildRoomSummaryForIdentifier(summary.roomId)
                 if (result != null) {
                     this[i] = result
                 }
@@ -103,8 +104,8 @@ class RoomSummaryListProcessor(
         return entry.use { roomSummaryFactory.create(room = it) }
     }
 
-    private suspend fun buildRoomSummaryForIdentifier(identifier: String): RoomSummary? {
-        return roomListService.roomOrNull(identifier)?.let { room ->
+    private suspend fun buildRoomSummaryForIdentifier(roomId: RoomId): RoomSummary? {
+        return roomListService.roomOrNull(roomId)?.let { room ->
             buildSummaryForRoomListEntry(room)
         }
     }

libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManager.kt

@@ -12,7 +12,6 @@ import dev.zacsweers.metro.AppScope
 import dev.zacsweers.metro.ContributesBinding
 import dev.zacsweers.metro.SingleIn
 import io.element.android.features.lockscreen.api.LockScreenService
-import io.element.android.libraries.core.data.tryOrNull
 import io.element.android.libraries.di.annotations.AppCoroutineScope
 import io.element.android.libraries.matrix.api.MatrixClientProvider
 import io.element.android.libraries.matrix.api.core.EventId
@@ -191,7 +190,7 @@ class DefaultNotificationDrawerManager(
     override fun clearReadRoomsNotifications(sessionId: SessionId) {
         val notifications = activeNotificationsProvider.getNotificationsForSession(sessionId)
         val notificationsGroupedByRoomId = notifications.mapNotNull {
-            tryOrNull { RoomId(it.tag) }?.let { roomId -> roomId to it }
+            it.tag?.let(NotificationCreator::getMessageNotificationRoomId)?.let { roomId -> roomId to it }
         }
 
         if (notificationsGroupedByRoomId.isEmpty()) return

libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreator.kt

@@ -24,6 +24,7 @@ import io.element.android.libraries.designsystem.components.avatar.AvatarData
 import io.element.android.libraries.designsystem.components.avatar.AvatarSize
 import io.element.android.libraries.designsystem.utils.CommonDrawables
 import io.element.android.libraries.di.annotations.ApplicationContext
+import io.element.android.libraries.matrix.api.core.MatrixPatterns
 import io.element.android.libraries.matrix.api.core.RoomId
 import io.element.android.libraries.matrix.api.core.ThreadId
 import io.element.android.libraries.matrix.api.timeline.item.event.EventType
@@ -102,12 +103,26 @@ interface NotificationCreator {
         /**
          * Creates a tag for a message notification given its [roomId] and optional [threadId].
          */
-        fun messageTag(roomId: RoomId, threadId: ThreadId?): String = if (threadId != null) {
-            "$roomId|$threadId"
-        } else {
-            roomId.value
+        fun messageTag(roomId: RoomId, threadId: ThreadId?): String = buildString {
+            append(roomId)
+            threadId?.let {
+                append(SEPARATOR)
+                append(it)
             }
         }
+
+        /**
+         * Extracts the [RoomId] from a message notification [tag] created by [messageTag].
+         * Returns null if the tag does not start with a valid room id.
+         */
+        fun getMessageNotificationRoomId(tag: String): RoomId? {
+            return tag.substringBefore(SEPARATOR)
+                .takeIf(MatrixPatterns::isRoomId)
+                ?.let(::RoomId)
+        }
+
+        private const val SEPARATOR = "|"
+    }
 }
 
 @ContributesBinding(AppScope::class)

libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/notifications/DefaultNotificationDrawerManagerTest.kt

@@ -636,6 +636,43 @@ class DefaultNotificationDrawerManagerTest {
         cancelNotificationResult.assertions().isNeverCalled()
     }
 
+    @Test
+    fun `clearReadRoomsNotifications cancels thread notifications of rooms without unread counts`() = runTest {
+        val cancelNotificationResult = lambdaRecorder<String?, Int, Unit> { _, _ -> }
+        val threadTag = "$A_ROOM_ID|$A_THREAD_ID"
+        val sut = createClearReadRoomsManager(
+            cancelNotificationResult = cancelNotificationResult,
+            notifications = listOf(
+                mockk<StatusBarNotification> {
+                    every { id } returns 1
+                    every { tag } returns threadTag
+                },
+            ),
+            roomInfo = aRoomInfo(id = A_ROOM_ID),
+        )
+        sut.clearReadRoomsNotifications(A_SESSION_ID)
+        runCurrent()
+        cancelNotificationResult.assertions().isCalledOnce().with(value(threadTag), value(1))
+    }
+
+    @Test
+    fun `clearReadRoomsNotifications ignores notifications whose tag is not a room id`() = runTest {
+        val cancelNotificationResult = lambdaRecorder<String?, Int, Unit> { _, _ -> }
+        val sut = createClearReadRoomsManager(
+            cancelNotificationResult = cancelNotificationResult,
+            notifications = listOf(
+                mockk<StatusBarNotification> {
+                    every { id } returns 1
+                    every { tag } returns AN_EVENT_ID.value
+                },
+            ),
+            roomInfo = aRoomInfo(id = A_ROOM_ID),
+        )
+        sut.clearReadRoomsNotifications(A_SESSION_ID)
+        runCurrent()
+        cancelNotificationResult.assertions().isNeverCalled()
+    }
+
     @Test
     fun `clearReadRoomsNotifications clears the summary when it is the only notification left`() = runTest {
         val cancelNotificationResult = lambdaRecorder<String?, Int, Unit> { _, _ -> }
@@ -681,7 +718,7 @@ class DefaultNotificationDrawerManagerTest {
         count: Int = notifications.size,
     ): DefaultNotificationDrawerManager {
         val matrixClient = FakeMatrixClient().apply {
-            getRoomInfoLambda = { Result.success(roomInfo) }
+            getRoomInfoLambda = { roomId -> Result.success(roomInfo?.takeIf { it.id == roomId }) }
         }
         return createDefaultNotificationDrawerManager(
             notificationDisplayer = FakeNotificationDisplayer(cancelNotificationResult = cancelNotificationResult),

libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreatorTest.kt

@@ -0,0 +1,36 @@
+/*
+ * 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.libraries.push.impl.notifications.factories
+
+import com.google.common.truth.Truth.assertThat
+import io.element.android.libraries.matrix.test.AN_EVENT_ID
+import io.element.android.libraries.matrix.test.A_ROOM_ID
+import io.element.android.libraries.matrix.test.A_THREAD_ID
+import org.junit.Test
+
+class NotificationCreatorTest {
+    @Test
+    fun `getMessageNotificationRoomId returns the room id of a room message tag`() {
+        val tag = NotificationCreator.messageTag(A_ROOM_ID, null)
+        assertThat(tag).isEqualTo(A_ROOM_ID.value)
+        assertThat(NotificationCreator.getMessageNotificationRoomId(tag)).isEqualTo(A_ROOM_ID)
+    }
+
+    @Test
+    fun `getMessageNotificationRoomId returns the room id of a thread message tag`() {
+        val tag = NotificationCreator.messageTag(A_ROOM_ID, A_THREAD_ID)
+        assertThat(tag).isEqualTo("${A_ROOM_ID.value}|${A_THREAD_ID.value}")
+        assertThat(NotificationCreator.getMessageNotificationRoomId(tag)).isEqualTo(A_ROOM_ID)
+    }
+
+    @Test
+    fun `getMessageNotificationRoomId returns null for a tag which is not a room id`() {
+        assertThat(NotificationCreator.getMessageNotificationRoomId(AN_EVENT_ID.value)).isNull()
+        assertThat(NotificationCreator.getMessageNotificationRoomId("")).isNull()
+    }
+}