Address search PR review comments

- Drop the redundant searchService coordinator/view model parameter; derive it from the client proxy. - Rename the globalSearch avatar size to search. - Lift the query observation streams out of their tasks. - Restore the appeared and updateVisibleRange comments. - Use CompoundIcon defaults and a scaledFrame for media previews. - Simplify the result mapping with compactMap(makeResult) and drop unsupported events by returning nil. - Move the preview helpers beneath the previews.
2026-11-03 06:55:59 +00:00 · 2026-07-03 11:57:23 +03:00
parent 4c2243cda1
commit 02d9ed1f0e
7 changed files with 50 additions and 50 deletions
@@ -10,7 +10,6 @@ import SwiftUI

 struct SearchScreenCoordinatorParameters {
    let roomSummaryProvider: RoomSummaryProviderProtocol
-    let searchService: SearchServiceProxyProtocol
    let clientProxy: ClientProxyProtocol
    let mediaProvider: MediaProviderProtocol
 }
@@ -32,7 +31,6 @@ final class SearchScreenCoordinator: CoordinatorProtocol {
    
    init(parameters: SearchScreenCoordinatorParameters) {
        viewModel = SearchScreenViewModel(roomSummaryProvider: parameters.roomSummaryProvider,
-                                          searchService: parameters.searchService,
                                          clientProxy: parameters.clientProxy,
                                          mediaProvider: parameters.mediaProvider)
    }
@@ -25,14 +25,13 @@ class SearchScreenViewModel: SearchScreenViewModelType, SearchScreenViewModelPro
    }
    
    init(roomSummaryProvider: RoomSummaryProviderProtocol,
-         searchService: SearchServiceProxyProtocol,
         clientProxy: ClientProxyProtocol,
         mediaProvider: MediaProviderProtocol,
         initialSearchQuery: String = "",
         initialSearchMode: SearchScreenMode = .rooms) {
        self.roomSummaryProvider = roomSummaryProvider
-        self.searchService = searchService
        self.clientProxy = clientProxy
+        searchService = clientProxy.searchService
        
        super.init(initialViewState: SearchScreenViewState(bindings: .init(searchQuery: initialSearchQuery, searchMode: initialSearchMode)),
                   mediaProvider: mediaProvider)
@@ -68,18 +67,18 @@ class SearchScreenViewModel: SearchScreenViewModelType, SearchScreenViewModelPro
            }
            .store(in: &cancellables)
        
+        let debouncedSearchQueryStream = context.observe(\.viewState.bindings.searchQuery).debounce(for: .milliseconds(250)).removeDuplicates()
        searchQueryObservationTask = Task { [weak self] in
-            guard let stream = self?.context.observe(\.viewState.bindings.searchQuery).debounce(for: .milliseconds(250)).removeDuplicates() else { return }
-            for await searchQuery in stream {
+            for await searchQuery in debouncedSearchQueryStream {
                self?.updateFilter(for: searchQuery)
            }
        }
        
        // Flip the loading indicator on the moment the user starts typing, ahead of the debounced
        // query above, so the empty state doesn't flash while the first search is still pending.
+        let searchQueryStream = context.observe(\.viewState.bindings.searchQuery).removeDuplicates()
        loadingObservationTask = Task { [weak self] in
-            guard let stream = self?.context.observe(\.viewState.bindings.searchQuery).removeDuplicates() else { return }
-            for await searchQuery in stream {
+            for await searchQuery in searchQueryStream {
                self?.state.isLoadingRooms = !searchQuery.isEmpty
                self?.state.isLoadingMessages = !searchQuery.isEmpty
            }
@@ -101,6 +100,8 @@ class SearchScreenViewModel: SearchScreenViewModelType, SearchScreenViewModelPro
        
        switch viewAction {
        case .appeared:
+            // The provider is shared, so other consumers may have changed its filter while we were off-screen.
+            // Re-apply ours on every appearance to keep the displayed results in sync with the query.
            updateFilter(for: state.bindings.searchQuery)
        case .selectRoom(let roomID):
            actionsSubject.send(.presentRoom(roomID: roomID, eventID: nil))
@@ -156,6 +157,12 @@ class SearchScreenViewModel: SearchScreenViewModelType, SearchScreenViewModelPro
        }
    }
    
+    /// The actual range values don't matter as long as they contain the lower
+    /// or upper bounds. updateVisibleRange is a hybrid API that powers both
+    /// sliding sync visible range update and list paginations.
+    /// For lists other than the home screen one we don't care about visible ranges,
+    /// we just need the respective bounds to be there to trigger a next page load or
+    /// a reset to just one page.
    private func updateVisibleRange(edge: UIRectEdge) {
        switch edge {
        case .top:
@@ -267,7 +267,7 @@ private struct SearchScreenRoomCell: View {
    private var avatar: some View {
        if dynamicTypeSize < .accessibility3 {
            RoomAvatarImage(avatar: room.avatar,
-                            avatarSize: .room(on: .globalSearch),
+                            avatarSize: .room(on: .search),
                            mediaProvider: context.mediaProvider)
                .dynamicTypeSize(dynamicTypeSize < .accessibility1 ? dynamicTypeSize : .accessibility1)
                .accessibilityHidden(true)
@@ -338,7 +338,7 @@ private struct SearchScreenMessageCell: View {
    private var avatar: some View {
        if dynamicTypeSize < .accessibility3 {
            RoomAvatarImage(avatar: message.roomAvatar,
-                            avatarSize: .room(on: .globalSearch),
+                            avatarSize: .room(on: .search),
                            mediaProvider: context.mediaProvider)
                .dynamicTypeSize(dynamicTypeSize < .accessibility1 ? dynamicTypeSize : .accessibility1)
                .accessibilityHidden(true)
@@ -350,12 +350,10 @@ private struct SearchScreenMediaPreviewView: View {
    let preview: SearchScreenMediaPreview
    let mediaProvider: MediaProviderProtocol?
    
-    private static let mediaSize: CGFloat = 36
-    
    var body: some View {
        HStack(spacing: 8) {
            media
-                .frame(width: Self.mediaSize, height: Self.mediaSize)
+                .scaledFrame(width: 36, height: 36)
            
            VStack(alignment: .leading, spacing: 0) {
                Text(preview.title)
@@ -390,9 +388,9 @@ private struct SearchScreenMediaPreviewView: View {
    }
    
    private func icon(_ icon: KeyPath<CompoundIcons, Image>) -> some View {
-        CompoundIcon(icon, size: .medium, relativeTo: .body)
+        CompoundIcon(icon)
            .foregroundStyle(.compound.iconPrimary)
-            .frame(width: Self.mediaSize, height: Self.mediaSize)
+            .scaledFrame(width: 36, height: 36)
            .background(.compound.iconOnSolidPrimary,
                        in: RoundedRectangle(cornerRadius: 4, style: .continuous))
    }
@@ -470,49 +468,27 @@ private final class KeyNavigatingSearchTextField: UISearchTextField {

 struct SearchScreen_Previews: PreviewProvider, TestablePreview {
    static let emptyViewModel = SearchScreenViewModel(roomSummaryProvider: RoomSummaryProviderMock(.init(state: .loaded([]))),
-                                                      searchService: makeSearchService(),
                                                      clientProxy: makeClientProxy(),
                                                      mediaProvider: MediaProviderMock(.init()))
    static let noResultsViewModel = SearchScreenViewModel(roomSummaryProvider: RoomSummaryProviderMock(.init(state: .loaded([]))),
-                                                          searchService: makeSearchService(),
                                                          clientProxy: makeClientProxy(),
                                                          mediaProvider: MediaProviderMock(.init()),
                                                          initialSearchQuery: "John Doe")
    static let roomsViewModel = SearchScreenViewModel(roomSummaryProvider: RoomSummaryProviderMock(.init(state: .loaded(.mockRooms))),
-                                                      searchService: makeSearchService(),
                                                      clientProxy: makeClientProxy(),
                                                      mediaProvider: MediaProviderMock(.init()),
                                                      initialSearchQuery: "Foundation")
    static let messagesViewModel = SearchScreenViewModel(roomSummaryProvider: RoomSummaryProviderMock(.init(state: .loaded([]))),
-                                                         searchService: makeSearchService(results: .mockResults),
-                                                         clientProxy: makeClientProxy(),
+                                                         clientProxy: makeClientProxy(searchService: makeSearchService(results: .mockResults)),
                                                         mediaProvider: MediaProviderMock(.init()),
                                                         initialSearchQuery: "Foundation",
                                                         initialSearchMode: .messages)
    static let loadingMessagesViewModel = SearchScreenViewModel(roomSummaryProvider: RoomSummaryProviderMock(.init(state: .loaded([]))),
-                                                                searchService: makeSearchService(paginationState: .loading),
-                                                                clientProxy: makeClientProxy(),
+                                                                clientProxy: makeClientProxy(searchService: makeSearchService(paginationState: .loading)),
                                                                mediaProvider: MediaProviderMock(.init()),
                                                                initialSearchQuery: "Foundation",
                                                                initialSearchMode: .messages)
    
-    private static func makeSearchService(results: [SearchServiceResult] = [], paginationState: SearchServicePaginationState = .idle(endReached: true)) -> SearchServiceProxyMock {
-        let mock = SearchServiceProxyMock()
-        mock.underlyingResultsPublisher = CurrentValueSubject<[SearchServiceResult], Never>(results).asCurrentValuePublisher()
-        mock.underlyingPaginationStatePublisher = CurrentValueSubject(paginationState).asCurrentValuePublisher()
-        mock.setQueryReturnValue = .success(())
-        return mock
-    }
-    
-    private static func makeClientProxy() -> ClientProxyMock {
-        let mock = ClientProxyMock(.init(userID: "@alice:matrix.org"))
-        let names: [String: String] = ["!room1:matrix.org": "Alice", "!room2:matrix.org": "Bob", "!room3:matrix.org": "Coline",
-                                       "!room4:matrix.org": "Bob", "!room5:matrix.org": "Office", "!room6:matrix.org": "Data analytics",
-                                       "!room7:matrix.org": "Alice", "!room8:matrix.org": "Bob", "!room9:matrix.org": "Coline"]
-        mock.roomSummaryForIdentifierClosure = { id in .mock(id: id, name: names[id] ?? id) }
-        return mock
-    }
-    
    static var previews: some View {
        ElementNavigationStack {
            SearchScreen(context: emptyViewModel.context)
@@ -539,6 +515,24 @@ struct SearchScreen_Previews: PreviewProvider, TestablePreview {
        }
        .previewDisplayName("Loading messages")
    }
+    
+    private static func makeSearchService(results: [SearchServiceResult] = [], paginationState: SearchServicePaginationState = .idle(endReached: true)) -> SearchServiceProxyMock {
+        let mock = SearchServiceProxyMock()
+        mock.underlyingResultsPublisher = CurrentValueSubject<[SearchServiceResult], Never>(results).asCurrentValuePublisher()
+        mock.underlyingPaginationStatePublisher = CurrentValueSubject(paginationState).asCurrentValuePublisher()
+        mock.setQueryReturnValue = .success(())
+        return mock
+    }
+    
+    private static func makeClientProxy(searchService: SearchServiceProxyMock = makeSearchService()) -> ClientProxyMock {
+        let mock = ClientProxyMock(.init(userID: "@alice:matrix.org"))
+        mock.searchService = searchService
+        let names: [String: String] = ["!room1:matrix.org": "Alice", "!room2:matrix.org": "Bob", "!room3:matrix.org": "Coline",
+                                       "!room4:matrix.org": "Bob", "!room5:matrix.org": "Office", "!room6:matrix.org": "Data analytics",
+                                       "!room7:matrix.org": "Alice", "!room8:matrix.org": "Bob", "!room9:matrix.org": "Coline"]
+        mock.roomSummaryForIdentifierClosure = { id in .mock(id: id, name: names[id] ?? id) }
+        return mock
+    }
 }

 private extension [SearchServiceResult] {