Suggest recently visited rooms when forwarding messages (#6251)

* Suggest recently visited rooms when forwarding messages

The Forward to screen now shows up to 5 recently visited rooms in a
Suggestions section, above a Chats section that lists every room. The
suggestions are never searched: while searching, only the Chats section
is shown and filtered. The source room, invites, spaces and upgraded
rooms are never suggested, and the section is hidden when there is
nothing to suggest.

The suggestions are resolved from the room summaries rather than room
proxies, so they show up without holding back the list, and they are
re-resolved as the room list loads after a cold start. ClientProxy now
exposes the recently visited room IDs for this.

The recently visited rooms filter is now async, so callers choose its
isolation instead of asserting it at runtime.

* Apply review suggestions to the forwarding suggestions

Rooms:
- Each room is listed once. Suggested rooms are left out of Chats unless
  searching, where Chats lists every result. This also removes the
  duplicated row whose selection changed without the tapped row's
  animation.
- The Chats header is hidden when the section has no rooms.

Suggestions:
- Resolve them once when the screen opens instead of on every room list
  update.

ElementX/Resources/Localizations/en-US.lproj/Localizable.strings

@@ -240,6 +240,7 @@
 "common_call_started" = "Call started";
 "common_call_you_declined" = "You declined a call";
 "common_chat_backup" = "Chat backup";
+"common_chats" = "Chats";
 "common_code_block" = "Code block";
 "common_copied_to_clipboard" = "Copied to clipboard";
 "common_copyright" = "Copyright";

ElementX/Resources/Localizations/en.lproj/Localizable.strings

@@ -240,6 +240,7 @@
 "common_call_started" = "Call started";
 "common_call_you_declined" = "You declined a call";
 "common_chat_backup" = "Chat backup";
+"common_chats" = "Chats";
 "common_code_block" = "Code block";
 "common_copied_to_clipboard" = "Copied to clipboard";
 "common_copyright" = "Copyright";

ElementX/Sources/Generated/Strings.swift

@@ -536,6 +536,8 @@ internal nonisolated enum L10n {
   internal static var commonCallYouDeclined: String { return L10n.tr("Localizable", "common_call_you_declined") }
   /// Chat backup
   internal static var commonChatBackup: String { return L10n.tr("Localizable", "common_chat_backup") }
+  /// Chats
+  internal static var commonChats: String { return L10n.tr("Localizable", "common_chats") }
   /// Code block
   internal static var commonCodeBlock: String { return L10n.tr("Localizable", "common_code_block") }
   /// Copied to clipboard

ElementX/Sources/Mocks/ClientProxyMock.swift

@@ -106,6 +106,7 @@ extension ClientProxyMock {
         unignoreUserReturnValue = .success(())
         
         trackRecentlyVisitedRoomReturnValue = .success(())
+        recentlyVisitedRoomIDsReturnValue = .success([])
         recentlyVisitedRoomsFilterReturnValue = []
         recentConversationCounterpartsReturnValue = []
         

ElementX/Sources/Mocks/Generated/GeneratedMocks.swift

@@ -4225,6 +4225,34 @@ nonisolated class ClientProxyMock: ClientProxyProtocol, @unchecked Sendable {
             return trackRecentlyVisitedRoomReturnValue
         }
     }
+    //MARK: - recentlyVisitedRoomIDs
+
+    private let recentlyVisitedRoomIDsCallsCountLock = NSLock()
+    private nonisolated(unsafe) var recentlyVisitedRoomIDsUnderlyingCallsCount = 0
+    var recentlyVisitedRoomIDsCallsCount: Int {
+        get { recentlyVisitedRoomIDsCallsCountLock.withLock { recentlyVisitedRoomIDsUnderlyingCallsCount } }
+        set { recentlyVisitedRoomIDsCallsCountLock.withLock { recentlyVisitedRoomIDsUnderlyingCallsCount = newValue } }
+    }
+    var recentlyVisitedRoomIDsCalled: Bool {
+        return recentlyVisitedRoomIDsCallsCount > 0
+    }
+
+    private let recentlyVisitedRoomIDsReturnValueLock = NSLock()
+    private nonisolated(unsafe) var recentlyVisitedRoomIDsUnderlyingReturnValue: Result<[String], ClientProxyError>!
+    var recentlyVisitedRoomIDsReturnValue: Result<[String], ClientProxyError>! {
+        get { recentlyVisitedRoomIDsReturnValueLock.withLock { recentlyVisitedRoomIDsUnderlyingReturnValue } }
+        set { recentlyVisitedRoomIDsReturnValueLock.withLock { recentlyVisitedRoomIDsUnderlyingReturnValue = newValue } }
+    }
+    nonisolated(unsafe) var recentlyVisitedRoomIDsClosure: (() async -> Result<[String], ClientProxyError>)?
+
+    @concurrent func recentlyVisitedRoomIDs() async -> Result<[String], ClientProxyError> {
+        recentlyVisitedRoomIDsCallsCountLock.withLock { recentlyVisitedRoomIDsUnderlyingCallsCount += 1 }
+        if let recentlyVisitedRoomIDsClosure = recentlyVisitedRoomIDsClosure {
+            return await recentlyVisitedRoomIDsClosure()
+        } else {
+            return recentlyVisitedRoomIDsReturnValue
+        }
+    }
     //MARK: - recentlyVisitedRooms
 
     private let recentlyVisitedRoomsFilterCallsCountLock = NSLock()
@@ -4243,9 +4271,9 @@ nonisolated class ClientProxyMock: ClientProxyProtocol, @unchecked Sendable {
         get { recentlyVisitedRoomsFilterReturnValueLock.withLock { recentlyVisitedRoomsFilterUnderlyingReturnValue } }
         set { recentlyVisitedRoomsFilterReturnValueLock.withLock { recentlyVisitedRoomsFilterUnderlyingReturnValue = newValue } }
     }
-    nonisolated(unsafe) var recentlyVisitedRoomsFilterClosure: ((@Sendable (JoinedRoomProxyProtocol) -> Bool) async -> [JoinedRoomProxyProtocol])?
+    nonisolated(unsafe) var recentlyVisitedRoomsFilterClosure: ((@Sendable (JoinedRoomProxyProtocol) async -> Bool) async -> [JoinedRoomProxyProtocol])?
 
-    @concurrent func recentlyVisitedRooms(filter: @Sendable (JoinedRoomProxyProtocol) -> Bool) async -> [JoinedRoomProxyProtocol] {
+    @concurrent func recentlyVisitedRooms(filter: @Sendable (JoinedRoomProxyProtocol) async -> Bool) async -> [JoinedRoomProxyProtocol] {
         recentlyVisitedRoomsFilterCallsCountLock.withLock { recentlyVisitedRoomsFilterUnderlyingCallsCount += 1 }
         if let recentlyVisitedRoomsFilterClosure = recentlyVisitedRoomsFilterClosure {
             return await recentlyVisitedRoomsFilterClosure(filter)

ElementX/Sources/Mocks/RoomSummaryProviderMock.swift

@@ -72,13 +72,14 @@ extension RoomSummaryProviderMock {
 extension RoomSummary {
     static func mock(id: String,
                      name: String,
+                     isSpace: Bool = false,
                      canonicalAlias: String? = nil) -> RoomSummary {
         RoomSummary(room: RoomSDKMock(.init()),
                     id: id,
                     joinRequestType: nil,
                     name: name,
                     isDirect: false,
-                    isSpace: false,
+                    isSpace: isSpace,
                     avatarURL: nil,
                     heroes: [],
                     activeMembersCount: 0,

ElementX/Sources/Screens/MessageForwardingScreen/MessageForwardingScreenModels.swift

@@ -15,11 +15,25 @@ enum MessageForwardingScreenViewModelAction {
 }
 
 struct MessageForwardingScreenViewState: BindableState {
+    /// The recently visited rooms, never filtered by the search query.
+    var suggestedRooms: [MessageForwardingRoom] = []
     var rooms: [MessageForwardingRoom] = []
     var selectedRoomIDs: Set<String> = []
     let maxRoomSelectionCount = 5
     var bindings = MessageForwardingScreenViewStateBindings()
     
+    var showsSuggestions: Bool {
+        bindings.searchQuery.isEmpty && !suggestedRooms.isEmpty
+    }
+    
+    /// Every room appears once: the suggested ones are left out unless searching.
+    var chats: [MessageForwardingRoom] {
+        guard showsSuggestions else { return rooms }
+        
+        let suggestedRoomIDs = Set(suggestedRooms.map(\.id))
+        return rooms.filter { !suggestedRoomIDs.contains($0.id) }
+    }
+    
     var isAtRoomSelectionLimit: Bool {
         selectedRoomIDs.count >= maxRoomSelectionCount
     }
@@ -44,6 +58,15 @@ struct MessageForwardingRoom: Identifiable, Equatable {
     let avatar: RoomAvatar
 }
 
+extension MessageForwardingRoom {
+    init(summary: RoomSummary) {
+        self.init(id: summary.id,
+                  title: summary.name,
+                  description: summary.roomListDescription,
+                  avatar: summary.avatar)
+    }
+}
+
 struct MessageForwardingPayload: Hashable {
     /// The source items' timeline IDs. Only necessary for a rough Hashable conformance.
     let ids: [TimelineItemIdentifier]

ElementX/Sources/Screens/MessageForwardingScreen/MessageForwardingScreenViewModel.swift

@@ -12,6 +12,8 @@ import SwiftUI
 typealias MessageForwardingScreenViewModelType = StateStoreViewModel<MessageForwardingScreenViewState, MessageForwardingScreenViewAction>
 
 class MessageForwardingScreenViewModel: MessageForwardingScreenViewModelType, MessageForwardingScreenViewModelProtocol {
+    private static let maxSuggestedRoomCount = 5
+    
     private let forwardingPayload: MessageForwardingPayload
     private let clientProxy: ClientProxyProtocol
     private let roomSummaryProvider: RoomSummaryProviderProtocol
@@ -54,6 +56,8 @@ class MessageForwardingScreenViewModel: MessageForwardingScreenViewModelType, Me
             .store(in: &cancellables)
         
         updateRooms()
+        
+        Task { await loadSuggestedRooms() }
     }
     
     override func process(viewAction: MessageForwardingScreenViewAction) {
@@ -83,20 +87,21 @@ class MessageForwardingScreenViewModel: MessageForwardingScreenViewModelType, Me
     // MARK: - Private
     
     private func updateRooms() {
-        var rooms = [MessageForwardingRoom]()
+        state.rooms = roomSummaryProvider.roomListPublisher.value
+            .filter { $0.id != forwardingPayload.roomID }
+            .map(MessageForwardingRoom.init(summary:))
+    }
+    
+    /// Built from the room summaries: building room proxies is slow and would shift the list down when the section appears.
+    private func loadSuggestedRooms() async {
+        guard case let .success(roomIDs) = await clientProxy.recentlyVisitedRoomIDs() else { return }
         
-        for summary in roomSummaryProvider.roomListPublisher.value {
-            if summary.id == forwardingPayload.roomID {
-                continue
-            }
-            
-            rooms.append(.init(id: summary.id,
-                               title: summary.name,
-                               description: summary.roomListDescription,
-                               avatar: summary.avatar))
-        }
-        
-        state.rooms = rooms
+        state.suggestedRooms = roomIDs
+            .filter { $0 != forwardingPayload.roomID }
+            .compactMap(clientProxy.roomSummaryForIdentifier)
+            .filter { $0.joinRequestType == nil && !$0.isSpace && !$0.isTombstoned }
+            .prefix(Self.maxSuggestedRoomCount)
+            .map(MessageForwardingRoom.init(summary:))
     }
     
     /// The actual range values don't matter as long as they contain the lower

ElementX/Sources/Screens/MessageForwardingScreen/View/MessageForwardingScreen.swift

@@ -15,19 +15,26 @@ struct MessageForwardingScreen: View {
     
     var body: some View {
         Form {
-            Section {
-                ForEach(context.viewState.rooms) { room in
-                    MessageForwardingListRow(room: room,
-                                             isSelected: context.viewState.selectedRoomIDs.contains(room.id),
-                                             isDisabled: context.viewState.isAtRoomSelectionLimit && !context.viewState.selectedRoomIDs.contains(room.id),
-                                             context: context)
+            if context.viewState.showsSuggestions {
+                Section {
+                    ForEach(context.viewState.suggestedRooms, content: row)
+                } header: {
+                    Text(L10n.commonSuggestions)
+                        .compoundListSectionHeader()
                 }
+            }
+            
+            Section {
+                ForEach(context.viewState.chats, content: row)
                 // Replace these with ScrollView's `scrollPosition` when dropping iOS 16.
             } header: {
-                emptyRectangle
-                    .onAppear {
-                        context.send(viewAction: .reachedTop)
-                    }
+                if !context.viewState.chats.isEmpty {
+                    Text(L10n.commonChats)
+                        .compoundListSectionHeader()
+                        .onAppear {
+                            context.send(viewAction: .reachedTop)
+                        }
+                }
             } footer: {
                 emptyRectangle
                     .onAppear {
@@ -56,6 +63,15 @@ struct MessageForwardingScreen: View {
         .disableAutocorrection(true)
     }
     
+    private func row(for room: MessageForwardingRoom) -> some View {
+        let isSelected = context.viewState.selectedRoomIDs.contains(room.id)
+        
+        return MessageForwardingListRow(room: room,
+                                        isSelected: isSelected,
+                                        isDisabled: context.viewState.isAtRoomSelectionLimit && !isSelected,
+                                        context: context)
+    }
+    
     /// The greedy size of Rectangle can create an issue with the navigation bar when the search is highlighted, so is best to use a fixed frame instead of hidden() or EmptyView()
     private var emptyRectangle: some View {
         Rectangle()
@@ -97,17 +113,40 @@ private struct MessageForwardingListRow: View {
 // MARK: - Previews
 
 struct MessageForwardingScreen_Previews: PreviewProvider, TestablePreview {
+    static let viewModel = makeViewModel()
+    static let searchingViewModel = makeViewModel(searchQuery: "Foundation")
+    
     static var previews: some View {
-        let summaryProvider = RoomSummaryProviderMock(.init(state: .loaded(.mockRooms)))
-        let viewModel = MessageForwardingScreenViewModel(forwardingPayload: .init(ids: [.randomEvent],
-                                                                                  roomID: "",
-                                                                                  contents: [RoomMessageEventContentWithoutRelationSDKMock()]),
-                                                         userSession: UserSessionMock(.init()),
-                                                         roomSummaryProvider: summaryProvider,
-                                                         userIndicatorController: UserIndicatorControllerMock())
-        
         ElementNavigationStack {
             MessageForwardingScreen(context: viewModel.context)
         }
+        .previewDisplayName("Suggestions")
+        .snapshotPreferences(expect: viewModel.context.$viewState.map(\.showsSuggestions))
+        
+        ElementNavigationStack {
+            MessageForwardingScreen(context: searchingViewModel.context)
+        }
+        .previewDisplayName("Searching")
+        .snapshotPreferences(expect: searchingViewModel.context.$viewState.map { !$0.suggestedRooms.isEmpty && !$0.showsSuggestions },
+                             precision: 0.999) // The search field's clear button renders inconsistently in snapshots.
+    }
+    
+    static func makeViewModel(searchQuery: String? = nil) -> MessageForwardingScreenViewModel {
+        let clientProxy = ClientProxyMock(.init())
+        clientProxy.recentlyVisitedRoomIDsReturnValue = .success(["2", "5", "3"])
+        clientProxy.roomSummaryForIdentifierClosure = { roomID in [RoomSummary].mockRooms.first { $0.id == roomID } }
+        
+        let viewModel = MessageForwardingScreenViewModel(forwardingPayload: .init(ids: [.randomEvent],
+                                                                                  roomID: "",
+                                                                                  contents: [RoomMessageEventContentWithoutRelationSDKMock()]),
+                                                         userSession: UserSessionMock(.init(clientProxy: clientProxy)),
+                                                         roomSummaryProvider: RoomSummaryProviderMock(.init(state: .loaded(.mockRooms))),
+                                                         userIndicatorController: UserIndicatorControllerMock())
+        
+        if let searchQuery {
+            viewModel.context.searchQuery = searchQuery
+        }
+        
+        return viewModel
     }
 }

ElementX/Sources/Screens/Spaces/SpaceAddRoomsScreen/SpaceAddRoomsScreenViewModel.swift

@@ -48,14 +48,11 @@ class SpaceAddRoomsScreenViewModel: SpaceAddRoomsScreenViewModelType, SpaceAddRo
         Task {
             let existingRooms = spaceRoomListProxy.spaceRoomsPublisher.value
             suggestedRooms = await userSession.clientProxy
-                .recentlyVisitedRooms { roomProxy in
-                    // The filter is Sendable for the mock's sake but the client proxy always calls it on the main actor.
-                    MainActor.assumeIsolated {
-                        !roomProxy.infoPublisher.value.isDirect
-                            && !roomProxy.infoPublisher.value.isSpace
-                            && roomProxy.infoPublisher.value.membership == .joined
-                            && !existingRooms.contains { $0.id == roomProxy.id }
-                    }
+                .recentlyVisitedRooms { @MainActor roomProxy in
+                    !roomProxy.infoPublisher.value.isDirect
+                        && !roomProxy.infoPublisher.value.isSpace
+                        && roomProxy.infoPublisher.value.membership == .joined
+                        && !existingRooms.contains { $0.id == roomProxy.id }
                 }
                 .map { .init(roomProxy: $0) }
             

ElementX/Sources/Services/Client/ClientProxy.swift

@@ -981,7 +981,17 @@ class ClientProxy: ClientProxyProtocol {
         }
     }
     
-    func recentlyVisitedRooms(filter: @Sendable (JoinedRoomProxyProtocol) -> Bool) async -> [JoinedRoomProxyProtocol] {
+    func recentlyVisitedRoomIDs() async -> Result<[String], ClientProxyError> {
+        do {
+            let result = try await client.getRecentlyVisitedRooms()
+            return .success(result)
+        } catch {
+            MXLog.error("Failed retrieving recently visited rooms with error: \(error)")
+            return .failure(.sdkError(error))
+        }
+    }
+    
+    func recentlyVisitedRooms(filter: @Sendable (JoinedRoomProxyProtocol) async -> Bool) async -> [JoinedRoomProxyProtocol] {
         let maxResultsToReturn = 5
         
         guard case let .success(roomIdentifiers) = await recentlyVisitedRoomIDs() else {
@@ -992,7 +1002,7 @@ class ClientProxy: ClientProxyProtocol {
         
         for roomID in roomIdentifiers {
             guard case let .joined(roomProxy) = await roomForIdentifier(roomID),
-                  filter(roomProxy) else {
+                  await filter(roomProxy) else {
                 continue
             }
             
@@ -1035,16 +1045,6 @@ class ClientProxy: ClientProxyProtocol {
         return users.elements
     }
     
-    private func recentlyVisitedRoomIDs() async -> Result<[String], ClientProxyError> {
-        do {
-            let result = try await client.getRecentlyVisitedRooms()
-            return .success(result)
-        } catch {
-            MXLog.error("Failed retrieving recently visited rooms with error: \(error)")
-            return .failure(.sdkError(error))
-        }
-    }
-    
     // MARK: Moderation & Safety
     
     func setTimelineMediaVisibility(_ value: TimelineMediaVisibility) async -> Result<Void, ClientProxyError> {

ElementX/Sources/Services/Client/ClientProxyProtocol.swift

@@ -283,7 +283,9 @@ protocol ClientProxyProtocol: AnyObject {
     
     func trackRecentlyVisitedRoom(_ roomID: String) async -> Result<Void, ClientProxyError>
     
-    func recentlyVisitedRooms(filter: @Sendable (JoinedRoomProxyProtocol) -> Bool) async -> [JoinedRoomProxyProtocol]
+    /// The identifiers of the rooms the user visited most recently, newest first.
+    func recentlyVisitedRoomIDs() async -> Result<[String], ClientProxyError>
+    func recentlyVisitedRooms(filter: @Sendable (JoinedRoomProxyProtocol) async -> Bool) async -> [JoinedRoomProxyProtocol]
     func recentConversationCounterparts() async -> [UserProfile]
     
     // MARK: - Crypto