diff --git a/ElementX/Sources/Screens/Spaces/SpaceAddRoomsScreen/SpaceAddRoomsScreenViewModel.swift b/ElementX/Sources/Screens/Spaces/SpaceAddRoomsScreen/SpaceAddRoomsScreenViewModel.swift index 4d7393d69..d1b3a7eea 100644 --- a/ElementX/Sources/Screens/Spaces/SpaceAddRoomsScreen/SpaceAddRoomsScreenViewModel.swift +++ b/ElementX/Sources/Screens/Spaces/SpaceAddRoomsScreen/SpaceAddRoomsScreenViewModel.swift @@ -17,9 +17,8 @@ class SpaceAddRoomsScreenViewModel: SpaceAddRoomsScreenViewModelType, SpaceAddRo private let userIndicatorController: UserIndicatorControllerProtocol private var suggestedRooms: [SpaceAddRoomsScreenRoom] = [] - /// A place to track rooms that were sucessfully added to the space in order to filter them out from - /// the roomsSection when a failure occurs part way through the array. - private var addedRoomIDs: Set = [] + /// Keep known children excluded while the space list resets and reloads after saving. + private var existingRoomIDs: Set private var actionsSubject: PassthroughSubject = .init() var actions: AnyPublisher { @@ -31,6 +30,7 @@ class SpaceAddRoomsScreenViewModel: SpaceAddRoomsScreenViewModelType, SpaceAddRo roomSummaryProvider: RoomSummaryProviderProtocol, userIndicatorController: UserIndicatorControllerProtocol) { self.spaceRoomListProxy = spaceRoomListProxy + existingRoomIDs = Set(spaceRoomListProxy.spaceRoomsPublisher.value.map(\.id)) spaceServiceProxy = userSession.clientProxy.spaceService self.roomSummaryProvider = roomSummaryProvider self.userIndicatorController = userIndicatorController @@ -38,6 +38,15 @@ class SpaceAddRoomsScreenViewModel: SpaceAddRoomsScreenViewModelType, SpaceAddRo super.init(initialViewState: SpaceAddRoomsScreenViewState(roomsSection: .init(type: .suggestions, rooms: [])), mediaProvider: userSession.mediaProvider) + spaceRoomListProxy.spaceRoomsPublisher + .receive(on: DispatchQueue.main) + .sink { [weak self] spaceRooms in + guard let self else { return } + existingRoomIDs.formUnion(spaceRooms.map(\.id)) + updateRooms() + } + .store(in: &cancellables) + roomSummaryProvider.roomListPublisher .receive(on: DispatchQueue.main) .sink { [weak self] _ in @@ -45,20 +54,18 @@ class SpaceAddRoomsScreenViewModel: SpaceAddRoomsScreenViewModelType, SpaceAddRo } .store(in: &cancellables) - Task { - let existingRooms = spaceRoomListProxy.spaceRoomsPublisher.value + Task { [existingRoomIDs] in + // Filter before the recent-room limit so existing children do not displace suggestions. suggestedRooms = await userSession.clientProxy .recentlyVisitedRooms { @MainActor roomProxy in !roomProxy.infoPublisher.value.isDirect && !roomProxy.infoPublisher.value.isSpace && roomProxy.infoPublisher.value.membership == .joined - && !existingRooms.contains { $0.id == roomProxy.id } + && !existingRoomIDs.contains(roomProxy.id) } .map { .init(roomProxy: $0) } - if state.roomsSection.type == .suggestions { - state.roomsSection = .init(type: .suggestions, rooms: suggestedRooms) - } + updateRooms() } } @@ -90,16 +97,14 @@ class SpaceAddRoomsScreenViewModel: SpaceAddRoomsScreenViewModelType, SpaceAddRo private func updateRooms() { guard !state.bindings.searchQuery.isEmpty else { - state.roomsSection = .init(type: .suggestions, rooms: suggestedRooms) + state.roomsSection = .init(type: .suggestions, rooms: suggestedRooms.filter { !existingRoomIDs.contains($0.id) }) return } - let existingRooms = spaceRoomListProxy.spaceRoomsPublisher.value - let rooms = roomSummaryProvider.roomListPublisher.value .lazy - .filter { [addedRoomIDs] summary in - !summary.isDirect && !existingRooms.contains { $0.id == summary.id } && !addedRoomIDs.contains(summary.id) + .filter { [existingRoomIDs] summary in + !summary.isDirect && !existingRoomIDs.contains(summary.id) } .map(SpaceAddRoomsScreenRoom.init) @@ -142,13 +147,13 @@ class SpaceAddRoomsScreenViewModel: SpaceAddRoomsScreenViewModelType, SpaceAddRo for room in state.selectedRooms { switch await spaceServiceProxy.addChild(room.id, to: spaceRoomListProxy.id) { case .success: - addedRoomIDs.insert(room.id) + existingRoomIDs.insert(room.id) case .failure(let error): MXLog.error("Failed adding room to space: \(error)") showErrorIndicator() // Hide rooms that were successfully added. - state.selectedRooms.removeAll { addedRoomIDs.contains($0.id) } + state.selectedRooms.removeAll { existingRoomIDs.contains($0.id) } updateRooms() return diff --git a/UnitTests/Sources/SpaceAddRoomsScreenViewModelTests.swift b/UnitTests/Sources/SpaceAddRoomsScreenViewModelTests.swift index 199d9dc43..c80942259 100644 --- a/UnitTests/Sources/SpaceAddRoomsScreenViewModelTests.swift +++ b/UnitTests/Sources/SpaceAddRoomsScreenViewModelTests.swift @@ -25,7 +25,7 @@ struct SpaceAddRoomsScreenViewModelTests { spaceRoomListProxy = SpaceRoomListProxyMock(.init(spaceServiceRoom: SpaceServiceRoom.mock(isSpace: true))) let clientProxy = ClientProxyMock(.init()) - clientProxy.recentlyVisitedRoomsFilterReturnValue = .init(repeating: JoinedRoomProxyMock(.init()), count: 5) + clientProxy.recentlyVisitedRoomsFilterReturnValue = (0..<5).map { JoinedRoomProxyMock(.init(id: "suggestion-\($0)")) } spaceServiceProxy = clientProxy.spaceService as? SpaceServiceProxyMock ?? SpaceServiceProxyMock(.init()) viewModel = SpaceAddRoomsScreenViewModel(spaceRoomListProxy: spaceRoomListProxy, @@ -63,21 +63,23 @@ struct SpaceAddRoomsScreenViewModelTests { #expect(spaceRoomListProxy.resetCalled, "The room list should be reset to pick up the changes.") } - @Test - func failureWithMultipleRoomsSelected() async throws { - // Given a view model with 4 selected rooms. + @Test(arguments: [true, false]) + func failureWithMultipleRoomsSelected(search: Bool) async throws { + let initialCount = search ? 4 : 5 + let sectionType: SpaceAddRoomsScreenViewState.Section.SectionType = search ? .searchResults : .suggestions + // Given a view model with several selected rooms. var deferred = deferFulfillment(context.observe(\.viewState.roomsSection), - message: "There should be 4 search results.") { section in - section.type == .searchResults && section.rooms.count == 4 + message: "The candidate rooms should be shown.") { section in + section.type == sectionType && section.rooms.count == initialCount } - context.searchQuery = "f" + context.searchQuery = search ? "f" : "" context.send(viewAction: .searchQueryChanged) try await deferred.fulfill() for room in context.viewState.roomsSection.rooms { context.send(viewAction: .toggleRoom(room)) } - #expect(context.viewState.selectedRooms.count == 4, "All of the rooms should be selected.") + #expect(context.viewState.selectedRooms.count == initialCount, "All of the rooms should be selected.") // When there's a failure half way through saving. let successfulIDs = context.viewState.roomsSection.rooms.map(\.id).prefix(2) @@ -90,8 +92,8 @@ struct SpaceAddRoomsScreenViewModelTests { } deferred = deferFulfillment(context.observe(\.viewState.roomsSection), - message: "The search results should update.") { section in - section.type == .searchResults && section.rooms.count == 2 + message: "The candidates should update after a partial save.") { section in + section.type == sectionType && section.rooms.count == initialCount - 2 } context.send(viewAction: .save) try await deferred.fulfill() @@ -103,4 +105,50 @@ struct SpaceAddRoomsScreenViewModelTests { #expect(!context.viewState.roomsSection.rooms.contains { successfulIDs.contains($0.id) }, "The added rooms should no longer be listed for selection.") } + + @Test(arguments: [true, false]) + func spaceMembershipUpdatesCandidates(search: Bool) async throws { + let summaries = [RoomSummary.mock(id: "first", name: "First room"), + .mock(id: "existing", name: "Existing room"), + .mock(id: "second", name: "Second room")] + let summariesSubject = CurrentValueSubject<[RoomSummary], Never>(summaries) + let summaryProvider = RoomSummaryProviderMock(.init(state: .loaded([]))) + summaryProvider.roomListPublisher = summariesSubject.asCurrentValuePublisher() + let spaceRoomsSubject = CurrentValueSubject<[SpaceServiceRoom], Never>([.mock(id: "existing", isSpace: false)]) + spaceRoomListProxy.spaceRoomsPublisher = spaceRoomsSubject.asCurrentValuePublisher() + let clientProxy = ClientProxyMock(.init()) + let recentRooms = summaries.map { JoinedRoomProxyMock(.init(id: $0.id)) } + clientProxy.recentlyVisitedRoomsFilterClosure = { filter in + var rooms: [JoinedRoomProxyProtocol] = [] + for room in recentRooms where await filter(room) { + rooms.append(room) + } + return rooms + } + let viewModel = SpaceAddRoomsScreenViewModel(spaceRoomListProxy: spaceRoomListProxy, + userSession: UserSessionMock(.init(clientProxy: clientProxy)), + roomSummaryProvider: summaryProvider, + userIndicatorController: UserIndicatorControllerMock()) + let context = viewModel.context + context.searchQuery = search ? "room" : "" + var deferred = deferFulfillment(context.observe(\.viewState.roomsSection)) { + $0.rooms.map(\.id) == ["first", "second"] + } + try await deferred.fulfill() + + deferred = deferFulfillment(context.observe(\.viewState.roomsSection)) { + $0.rooms.map(\.id) == ["second"] + } + spaceRoomsSubject.send([.mock(id: "existing", isSpace: false), .mock(id: "first", isSpace: false)]) + try await deferred.fulfill() + + // An empty snapshot during reload must not make known children selectable again. + deferred = deferFulfillment(context.observe(\.viewState.roomsSection)) { + $0.type == .searchResults && $0.rooms.map(\.id) == ["second", "third"] + } + spaceRoomsSubject.send([]) + context.searchQuery = "room" + summariesSubject.send(summaries + [.mock(id: "third", name: "Third room")]) + try await deferred.fulfill() + } }