Fix navigation back from room's details (#988)

* Fix navigation to room details

* Improve tests

* Delete unused action in RoomDetailsScreen

* Cleanup

* Try fix flaky tests

ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift

@@ -86,12 +86,12 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol {
                 return .initial
                 
             case (.presentRoomDetails(let roomID), .initial):
-                return .roomDetails(roomID: roomID)
+                return .roomDetails(roomID: roomID, isRoot: true)
-            case (.presentRoomDetails(let roomID), .room):
+            case (.presentRoomDetails(let roomID), .room(let currentRoomID)):
-                return .roomDetails(roomID: roomID)
+                return .roomDetails(roomID: roomID, isRoot: roomID != currentRoomID)
-            case (.presentRoomDetails(let roomID), .roomDetails):
+            case (.presentRoomDetails(let roomID), .roomDetails(let currentRoomID, _)):
-                return .roomDetails(roomID: roomID)
+                return .roomDetails(roomID: roomID, isRoot: roomID != currentRoomID)
-            case (.dismissRoomDetails, .roomDetails(let roomID)):
+            case (.dismissRoomDetails, .roomDetails(let roomID, _)):
                 return .room(roomID: roomID)
             case (.dismissRoom, .roomDetails):
                 return .initial
@@ -134,17 +134,20 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol {
             let animated = (context.userInfo as? EventUserInfo)?.animated ?? true
             
             switch (context.fromState, context.event, context.toState) {
+            case (.roomDetails(roomID: let currentRoomID, true), .presentRoom(let roomID), .room) where currentRoomID == roomID:
+                dismissRoom(animated: animated)
+                presentRoom(roomID, animated: animated)
             case (_, .presentRoom(let roomID), .room):
                 presentRoom(roomID, animated: animated)
             case (.room, .dismissRoom, .initial):
                 dismissRoom(animated: animated)
             
-            case (.initial, .presentRoomDetails, .roomDetails(let roomID)),
+            case (.roomDetails(let currentRoomID, _), .presentRoomDetails, .roomDetails(let roomID, _)) where currentRoomID == roomID:
-                 (.room, .presentRoomDetails, .roomDetails(let roomID)),
+                break
-                 (.roomDetails, .presentRoomDetails, .roomDetails(let roomID)):
+            case (.initial, .presentRoomDetails, .roomDetails(let roomID, let isRoot)),
-                Task {
+                 (.room, .presentRoomDetails, .roomDetails(let roomID, let isRoot)),
-                    await self.presentRoomDetails(roomID: roomID, animated: animated)
+                 (.roomDetails, .presentRoomDetails, .roomDetails(let roomID, let isRoot)):
-                }
+                self.presentRoomDetails(roomID: roomID, isRoot: isRoot, animated: animated)
             case (.roomDetails, .dismissRoomDetails, .room):
                 break
             case (.roomDetails, .dismissRoom, .initial):
@@ -275,20 +278,30 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol {
     }
     
     private func dismissRoom(animated: Bool) {
-        navigationStackCoordinator.popToRoot(animated: true)
+        navigationStackCoordinator.popToRoot(animated: animated)
-        navigationSplitCoordinator.setDetailCoordinator(nil)
+        navigationSplitCoordinator.setDetailCoordinator(nil, animated: animated)
         roomProxy = nil
         
         actionsSubject.send(.dismissedRoom)
     }
     
-    private func presentRoomDetails(roomID: String, animated: Bool) async {
+    private func presentRoomDetails(roomID: String, isRoot: Bool, animated: Bool) {
-        if roomProxy?.id != roomID {
+        Task {
-            await asyncPresentRoom(roomID, animated: true)
+            await asyncPresentRoomDetails(roomID: roomID, isRoot: isRoot, animated: animated)
+        }
+    }
+    
+    private func asyncPresentRoomDetails(roomID: String, isRoot: Bool, animated: Bool) async {
+        if isRoot {
+            roomProxy = await userSession.clientProxy.roomForIdentifier(roomID)
+        } else {
+            await asyncPresentRoom(roomID, animated: animated)
         }
         
         guard let roomProxy else {
-            fatalError()
+            MXLog.error("Invalid room identifier: \(roomID)")
+            stateMachine.tryEvent(.dismissRoom)
+            return
         }
         
         let params = RoomDetailsScreenCoordinatorParameters(navigationStackCoordinator: navigationStackCoordinator,
@@ -298,18 +311,30 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol {
         let coordinator = RoomDetailsScreenCoordinator(parameters: params)
         coordinator.callback = { [weak self] action in
             switch action {
-            case .cancel:
-                self?.navigationStackCoordinator.pop()
             case .leftRoom:
                 self?.dismissRoom(animated: animated)
             }
         }
         
-        navigationStackCoordinator.push(coordinator) { [weak self] in
+        if isRoot {
-            guard let self else { return }
+            navigationStackCoordinator.setRootCoordinator(coordinator, animated: animated) { [weak self] in
+                guard let self else { return }
+                if case .roomDetails(let detailsRoomID, _) = stateMachine.state, detailsRoomID == roomID {
+                    stateMachine.tryEvent(.dismissRoom)
+                }
+            }
             
-            if case .roomDetails = stateMachine.state {
+            if navigationSplitCoordinator.detailCoordinator == nil {
-                stateMachine.tryEvent(.dismissRoomDetails)
+                navigationSplitCoordinator.setDetailCoordinator(navigationStackCoordinator, animated: animated)
+            }
+            
+            actionsSubject.send(.presentedRoom(roomID))
+        } else {
+            navigationStackCoordinator.push(coordinator, animated: animated) { [weak self] in
+                guard let self else { return }
+                if case .roomDetails = stateMachine.state {
+                    stateMachine.tryEvent(.dismissRoomDetails)
+                }
             }
         }
     }
@@ -445,7 +470,7 @@ private extension RoomFlowCoordinator {
         case room(roomID: String)
         case mediaViewer(roomID: String, file: MediaFileHandleProxy, title: String?)
         case reportContent(roomID: String, itemID: String, senderID: String)
-        case roomDetails(roomID: String)
+        case roomDetails(roomID: String, isRoot: Bool)
         case mediaUploadPicker(roomID: String, source: MediaPickerScreenSource)
         case mediaUploadPreview(roomID: String, fileURL: URL)
         case emojiPicker(roomID: String, itemID: String)

ElementX/Sources/Screens/RoomDetailsScreen/RoomDetailsScreenCoordinator.swift

@@ -25,7 +25,6 @@ struct RoomDetailsScreenCoordinatorParameters {
 }
 
 enum RoomDetailsScreenCoordinatorAction {
-    case cancel
     case leftRoom
 }
 
@@ -58,8 +57,6 @@ final class RoomDetailsScreenCoordinator: CoordinatorProtocol {
                 self.presentRoomMembersList(members)
             case .requestInvitePeoplePresentation(let members):
                 self.presentInviteUsersScreen(members: members)
-            case .cancel:
-                self.callback?(.cancel)
             case .leftRoom:
                 self.callback?(.leftRoom)
             case .requestEditDetailsPresentation(let accountOwner):

ElementX/Sources/Screens/RoomDetailsScreen/RoomDetailsScreenModels.swift

@@ -25,7 +25,6 @@ enum RoomDetailsScreenViewModelAction {
     case requestMemberDetailsPresentation([RoomMemberProxyProtocol])
     case requestInvitePeoplePresentation([RoomMemberProxyProtocol])
     case leftRoom
-    case cancel
     case requestEditDetailsPresentation(RoomMemberProxyProtocol)
 }
 

UnitTests/Sources/RoomFlowCoordinatorTests.swift

@@ -60,9 +60,7 @@ class RoomFlowCoordinatorTests: XCTestCase {
     
     func testRoomDetailsPresentation() async {
         await process(route: .roomDetails(roomID: "1"), expectedAction: .presentedRoom("1"))
-        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator)
+        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator)
-        XCTAssertEqual(navigationStackCoordinator.stackCoordinators.count, 1)
-        XCTAssert(navigationStackCoordinator.stackCoordinators.first is RoomDetailsScreenCoordinator)
         
         await process(route: .roomList, expectedAction: .dismissedRoom)
         XCTAssertNil(navigationStackCoordinator.rootCoordinator)
@@ -70,18 +68,58 @@ class RoomFlowCoordinatorTests: XCTestCase {
     
     func testStackUnwinding() async {
         await process(route: .roomDetails(roomID: "1"), expectedAction: .presentedRoom("1"))
-        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator)
+        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator)
-        XCTAssertEqual(navigationStackCoordinator.stackCoordinators.count, 1)
-        XCTAssert(navigationStackCoordinator.stackCoordinators.first is RoomDetailsScreenCoordinator)
         
         await process(route: .room(roomID: "2"), expectedAction: .presentedRoom("2"))
         XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator)
     }
     
+    func testNoOp() async {
+        await process(route: .roomDetails(roomID: "1"), expectedAction: .presentedRoom("1"))
+        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator)
+        
+        await process(route: .roomDetails(roomID: "1"), expectedAction: nil)
+        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator)
+    }
+    
+    func testSwitchToDifferentDetails() async {
+        await process(route: .roomDetails(roomID: "1"), expectedAction: .presentedRoom("1"))
+        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator)
+        
+        await process(route: .roomDetails(roomID: "2"), expectedAction: .presentedRoom("2"))
+        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator)
+    }
+    
+    func testPushDetails() async {
+        await process(route: .room(roomID: "1"), expectedAction: .presentedRoom("1"))
+        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator)
+        
+        await process(route: .roomDetails(roomID: "1"), expectedAction: nil)
+        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator)
+        XCTAssertEqual(navigationStackCoordinator.stackCoordinators.count, 1)
+        XCTAssert(navigationStackCoordinator.stackCoordinators.first is RoomDetailsScreenCoordinator)
+    }
+    
+    func testReplaceDetailsWithTimeline() async {
+        await process(route: .roomDetails(roomID: "1"), expectedAction: .presentedRoom("1"))
+        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomDetailsScreenCoordinator)
+        
+        await process(route: .room(roomID: "1"), expectedAction: .presentedRoom("1"))
+        XCTAssert(navigationStackCoordinator.rootCoordinator is RoomScreenCoordinator)
+    }
+    
     // MARK: - Private
     
-    func process(route: AppRoute, expectedAction: RoomFlowCoordinatorAction) async {
+    func process(route: AppRoute, expectedAction: RoomFlowCoordinatorAction?) async {
-        Task { try? await Task.sleep(for: .seconds(0.1)); roomFlowCoordinator.handleAppRoute(route, animated: true) }
+        let routeTask = Task.detached(priority: .low) {
-        _ = await roomFlowCoordinator.actions.values.first(where: { $0 == expectedAction })
+            try? await Task.sleep(for: .seconds(0.2))
+            await self.roomFlowCoordinator.handleAppRoute(route, animated: true)
+        }
+        
+        if let expectedAction {
+            _ = await roomFlowCoordinator.actions.values.first(where: { $0 == expectedAction })
+        } else {
+            await routeTask.value
+        }
     }
 }