AppNav: introduce a owner param so we avoid crash on AppNavigationState when switching quickly between screens

appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt

@@ -56,10 +56,7 @@ import io.element.android.libraries.matrix.api.core.RoomId
 import io.element.android.libraries.matrix.ui.di.MatrixUIBindings
 import io.element.android.services.appnavstate.api.AppNavigationStateService
 import kotlinx.coroutines.CoroutineScope
-import kotlinx.coroutines.Dispatchers
-import kotlinx.coroutines.runBlocking
 import kotlinx.parcelize.Parcelize
-import kotlin.coroutines.coroutineContext
 
 @ContributesNode(AppScope::class)
 class LoggedInFlowNode @AssistedInject constructor(
@@ -110,17 +107,17 @@ class LoggedInFlowNode @AssistedInject constructor(
                 val imageLoaderFactory = bindings<MatrixUIBindings>().loggedInImageLoaderFactory()
                 Coil.setImageLoader(imageLoaderFactory)
                 inputs.matrixClient.startSync()
-                appNavigationStateService.onNavigateToSession(inputs.matrixClient.sessionId)
+                appNavigationStateService.onNavigateToSession(id, inputs.matrixClient.sessionId)
                 // TODO We do not support Space yet, so directly navigate to main space
-                appNavigationStateService.onNavigateToSpace(MAIN_SPACE)
+                appNavigationStateService.onNavigateToSpace(id, MAIN_SPACE)
                 loggedInFlowProcessor.observeEvents(coroutineScope)
             },
             onDestroy = {
                 val imageLoaderFactory = bindings<MatrixUIBindings>().notLoggedInImageLoaderFactory()
                 Coil.setImageLoader(imageLoaderFactory)
                 plugins<LifecycleCallback>().forEach { it.onFlowReleased(inputs.matrixClient) }
-                appNavigationStateService.onLeavingSpace()
+                appNavigationStateService.onLeavingSpace(id)
-                appNavigationStateService.onLeavingSession()
+                appNavigationStateService.onLeavingSession(id)
                 loggedInFlowProcessor.stopObserving()
             }
         )

appnav/src/main/kotlin/io/element/android/appnav/RoomFlowNode.kt

@@ -82,13 +82,13 @@ class RoomFlowNode @AssistedInject constructor(
             onCreate = {
                 Timber.v("OnCreate")
                 plugins<LifecycleCallback>().forEach { it.onFlowCreated(inputs.room) }
-                appNavigationStateService.onNavigateToRoom(inputs.room.roomId)
+                appNavigationStateService.onNavigateToRoom(id, inputs.room.roomId)
             },
             onDestroy = {
                 Timber.v("OnDestroy")
                 inputs.room.close()
                 plugins<LifecycleCallback>().forEach { it.onFlowReleased(inputs.room) }
-                appNavigationStateService.onLeavingRoom()
+                appNavigationStateService.onLeavingRoom(id)
             }
         )
 

services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppNavigationState.kt

@@ -21,26 +21,34 @@ import io.element.android.libraries.matrix.api.core.SessionId
 import io.element.android.libraries.matrix.api.core.SpaceId
 import io.element.android.libraries.matrix.api.core.ThreadId
 
-sealed interface AppNavigationState {
+/**
-    object Root : AppNavigationState
+ * Can represent the current global app navigation state.
+ * @param owner mostly a Node identifier associated with the state.
+ */
+sealed class AppNavigationState(open val owner: String) {
+    object Root : AppNavigationState("ROOT")
 
     data class Session(
+        override val owner: String,
         val sessionId: SessionId,
-    ) : AppNavigationState
+    ) : AppNavigationState(owner)
 
     data class Space(
+        override val owner: String,
         // Can be fake value, if no space is selected
         val spaceId: SpaceId,
         val parentSession: Session,
-    ) : AppNavigationState
+    ) : AppNavigationState(owner)
 
     data class Room(
+        override val owner: String,
         val roomId: RoomId,
         val parentSpace: Space,
-    ) : AppNavigationState
+    ) : AppNavigationState(owner)
 
     data class Thread(
+        override val owner: String,
         val threadId: ThreadId,
         val parentRoom: Room,
-    ) : AppNavigationState
+    ) : AppNavigationState(owner)
 }

services/appnavstate/api/src/main/kotlin/io/element/android/services/appnavstate/api/AppNavigationStateService.kt

@@ -25,15 +25,15 @@ import kotlinx.coroutines.flow.StateFlow
 interface AppNavigationStateService {
     val appNavigationStateFlow: StateFlow<AppNavigationState>
 
-    fun onNavigateToSession(sessionId: SessionId)
+    fun onNavigateToSession(owner: String, sessionId: SessionId)
-    fun onLeavingSession()
+    fun onLeavingSession(owner: String)
 
-    fun onNavigateToSpace(spaceId: SpaceId)
+    fun onNavigateToSpace(owner: String, spaceId: SpaceId)
-    fun onLeavingSpace()
+    fun onLeavingSpace(owner: String)
 
-    fun onNavigateToRoom(roomId: RoomId)
+    fun onNavigateToRoom(owner: String, roomId: RoomId)
-    fun onLeavingRoom()
+    fun onLeavingRoom(owner: String)
 
-    fun onNavigateToThread(threadId: ThreadId)
+    fun onNavigateToThread(owner: String, threadId: ThreadId)
-    fun onLeavingThread()
+    fun onLeavingThread(owner: String)
 }

services/appnavstate/impl/src/main/kotlin/io/element/android/services/appnavstate/impl/DefaultAppNavigationStateService.kt

@@ -40,11 +40,10 @@ private val loggerTag = LoggerTag("Navigation")
 @SingleIn(AppScope::class)
 class DefaultAppNavigationStateService @Inject constructor() : AppNavigationStateService {
 
-    private val currentAppNavigationState = MutableStateFlow<AppNavigationState>(AppNavigationState.Root)
+    private val currentAppNavigationState: MutableStateFlow<AppNavigationState> = MutableStateFlow(AppNavigationState.Root)
-
     override val appNavigationStateFlow: StateFlow<AppNavigationState> = currentAppNavigationState
 
-    override fun onNavigateToSession(sessionId: SessionId) {
+    override fun onNavigateToSession(owner: String, sessionId: SessionId) {
         val currentValue = currentAppNavigationState.value
         Timber.tag(loggerTag.value).d("Navigating to session $sessionId. Current state: $currentValue")
         val newValue: AppNavigationState.Session = when (currentValue) {
@@ -52,53 +51,54 @@ class DefaultAppNavigationStateService @Inject constructor() : AppNavigationStat
             is AppNavigationState.Space,
             is AppNavigationState.Room,
             is AppNavigationState.Thread,
-            AppNavigationState.Root -> AppNavigationState.Session(sessionId)
+            is AppNavigationState.Root -> AppNavigationState.Session(owner, sessionId)
         }
         currentAppNavigationState.value = newValue
     }
 
-    override fun onNavigateToSpace(spaceId: SpaceId) {
+    override fun onNavigateToSpace(owner: String, spaceId: SpaceId) {
         val currentValue = currentAppNavigationState.value
         Timber.tag(loggerTag.value).d("Navigating to space $spaceId. Current state: $currentValue")
         val newValue: AppNavigationState.Space = when (currentValue) {
             AppNavigationState.Root -> error("onNavigateToSession() must be called first")
-            is AppNavigationState.Session -> AppNavigationState.Space(spaceId, currentValue)
+            is AppNavigationState.Session -> AppNavigationState.Space(owner, spaceId, currentValue)
-            is AppNavigationState.Space -> AppNavigationState.Space(spaceId, currentValue.parentSession)
+            is AppNavigationState.Space -> AppNavigationState.Space(owner, spaceId, currentValue.parentSession)
-            is AppNavigationState.Room -> AppNavigationState.Space(spaceId, currentValue.parentSpace.parentSession)
+            is AppNavigationState.Room -> AppNavigationState.Space(owner, spaceId, currentValue.parentSpace.parentSession)
-            is AppNavigationState.Thread -> AppNavigationState.Space(spaceId, currentValue.parentRoom.parentSpace.parentSession)
+            is AppNavigationState.Thread -> AppNavigationState.Space(owner, spaceId, currentValue.parentRoom.parentSpace.parentSession)
         }
         currentAppNavigationState.value = newValue
     }
 
-    override fun onNavigateToRoom(roomId: RoomId) {
+    override fun onNavigateToRoom(owner: String, roomId: RoomId) {
         val currentValue = currentAppNavigationState.value
         Timber.tag(loggerTag.value).d("Navigating to room $roomId. Current state: $currentValue")
         val newValue: AppNavigationState.Room = when (currentValue) {
             AppNavigationState.Root -> error("onNavigateToSession() must be called first")
             is AppNavigationState.Session -> error("onNavigateToSpace() must be called first")
-            is AppNavigationState.Space -> AppNavigationState.Room(roomId, currentValue)
+            is AppNavigationState.Space -> AppNavigationState.Room(owner, roomId, currentValue)
-            is AppNavigationState.Room -> AppNavigationState.Room(roomId, currentValue.parentSpace)
+            is AppNavigationState.Room -> AppNavigationState.Room(owner, roomId, currentValue.parentSpace)
-            is AppNavigationState.Thread -> AppNavigationState.Room(roomId, currentValue.parentRoom.parentSpace)
+            is AppNavigationState.Thread -> AppNavigationState.Room(owner, roomId, currentValue.parentRoom.parentSpace)
         }
         currentAppNavigationState.value = newValue
     }
 
-    override fun onNavigateToThread(threadId: ThreadId) {
+    override fun onNavigateToThread(owner: String, threadId: ThreadId) {
         val currentValue = currentAppNavigationState.value
         Timber.tag(loggerTag.value).d("Navigating to thread $threadId. Current state: $currentValue")
         val newValue: AppNavigationState.Thread = when (currentValue) {
             AppNavigationState.Root -> error("onNavigateToSession() must be called first")
             is AppNavigationState.Session -> error("onNavigateToSpace() must be called first")
             is AppNavigationState.Space -> error("onNavigateToRoom() must be called first")
-            is AppNavigationState.Room -> AppNavigationState.Thread(threadId, currentValue)
+            is AppNavigationState.Room -> AppNavigationState.Thread(owner, threadId, currentValue)
-            is AppNavigationState.Thread -> AppNavigationState.Thread(threadId, currentValue.parentRoom)
+            is AppNavigationState.Thread -> AppNavigationState.Thread(owner, threadId, currentValue.parentRoom)
         }
         currentAppNavigationState.value = newValue
     }
 
-    override fun onLeavingThread() {
+    override fun onLeavingThread(owner: String) {
         val currentValue = currentAppNavigationState.value
         Timber.tag(loggerTag.value).d("Leaving thread. Current state: $currentValue")
+        if (!currentValue.assertOwner(owner)) return
         val newValue: AppNavigationState.Room = when (currentValue) {
             AppNavigationState.Root -> error("onNavigateToSession() must be called first")
             is AppNavigationState.Session -> error("onNavigateToSpace() must be called first")
@@ -109,9 +109,10 @@ class DefaultAppNavigationStateService @Inject constructor() : AppNavigationStat
         currentAppNavigationState.value = newValue
     }
 
-    override fun onLeavingRoom() {
+    override fun onLeavingRoom(owner: String) {
         val currentValue = currentAppNavigationState.value
         Timber.tag(loggerTag.value).d("Leaving room. Current state: $currentValue")
+        if (!currentValue.assertOwner(owner)) return
         val newValue: AppNavigationState.Space = when (currentValue) {
             AppNavigationState.Root -> error("onNavigateToSession() must be called first")
             is AppNavigationState.Session -> error("onNavigateToSpace() must be called first")
@@ -122,9 +123,10 @@ class DefaultAppNavigationStateService @Inject constructor() : AppNavigationStat
         currentAppNavigationState.value = newValue
     }
 
-    override fun onLeavingSpace() {
+    override fun onLeavingSpace(owner: String) {
         val currentValue = currentAppNavigationState.value
         Timber.tag(loggerTag.value).d("Leaving space. Current state: $currentValue")
+        if (!currentValue.assertOwner(owner)) return
         val newValue: AppNavigationState.Session = when (currentValue) {
             AppNavigationState.Root -> error("onNavigateToSession() must be called first")
             is AppNavigationState.Session -> error("onNavigateToSpace() must be called first")
@@ -135,9 +137,18 @@ class DefaultAppNavigationStateService @Inject constructor() : AppNavigationStat
         currentAppNavigationState.value = newValue
     }
 
-    override fun onLeavingSession() {
+    override fun onLeavingSession(owner: String) {
         val currentValue = currentAppNavigationState.value
         Timber.tag(loggerTag.value).d("Leaving session. Current state: $currentValue")
+        if (!currentValue.assertOwner(owner)) return
         currentAppNavigationState.value = AppNavigationState.Root
     }
+
+    private fun AppNavigationState.assertOwner(owner: String): Boolean {
+        if (this.owner != owner) {
+            Timber.tag(loggerTag.value).d("Can't leave current state as the owner is not the same (current = ${this.owner}, new = $owner)")
+            return false
+        }
+        return true
+    }
 }

services/appnavstate/impl/src/test/kotlin/io/element/android/services/appnavstate/impl/DefaultAppNavigationStateServiceTest.kt

@@ -30,23 +30,31 @@ import kotlinx.coroutines.test.runTest
 import org.junit.Assert.assertThrows
 import org.junit.Test
 
+private const val aSessionOwner = "aSessionOwner"
+private const val aSpaceOwner = "aSpaceOwner"
+private const val aRoomOwner = "aRoomOwner"
+private const val aThreadOwner = "aThreadOwner"
+
 class DefaultAppNavigationStateServiceTest {
 
     @Test
     fun testNavigation() = runTest {
         val service = DefaultAppNavigationStateService()
-        service.onNavigateToSession(A_SESSION_ID)
+        service.onNavigateToSession(aSessionOwner, A_SESSION_ID)
-        service.onNavigateToSpace(A_SPACE_ID)
+        service.onNavigateToSpace(aSpaceOwner, A_SPACE_ID)
-        service.onNavigateToRoom(A_ROOM_ID)
+        service.onNavigateToRoom(aRoomOwner, A_ROOM_ID)
-        service.onNavigateToThread(A_THREAD_ID)
+        service.onNavigateToThread(aThreadOwner, A_THREAD_ID)
         assertThat(service.appNavigationStateFlow.first()).isEqualTo(
             AppNavigationState.Thread(
-                A_THREAD_ID,
+                aThreadOwner, A_THREAD_ID,
                 AppNavigationState.Room(
+                    aRoomOwner,
                     A_ROOM_ID,
                     AppNavigationState.Space(
+                        aSpaceOwner,
                         A_SPACE_ID,
                         AppNavigationState.Session(
+                            aSessionOwner,
                             A_SESSION_ID
                         )
                     )
@@ -58,6 +66,6 @@ class DefaultAppNavigationStateServiceTest {
     @Test
     fun testFailure() = runTest {
         val service = DefaultAppNavigationStateService()
-        assertThrows(IllegalStateException::class.java) { service.onNavigateToSpace(A_SPACE_ID) }
+        assertThrows(IllegalStateException::class.java) { service.onNavigateToSpace(aSpaceOwner, A_SPACE_ID) }
     }
 }