From d08f843c94f37cd928ef2cf5f3f60efef68197f9 Mon Sep 17 00:00:00 2001 From: Mauro Romito Date: Thu, 22 Jan 2026 12:48:55 +0100 Subject: [PATCH] pr suggestions + await PLs of the room to be ready before adding it to the space --- .../CreateRoomScreenViewModel.swift | 25 ++- .../View/CreateRoomScreen.swift | 163 +++++------------- .../View/CreateRoomSpaceSelectionSheet.swift | 1 + .../Services/Spaces/SpaceServiceProxy.swift | 4 +- .../Spaces/SpaceServiceProxyProtocol.swift | 2 +- .../Sources/CreateRoomViewModelTests.swift | 10 -- 6 files changed, 67 insertions(+), 138 deletions(-) diff --git a/ElementX/Sources/Screens/CreateRoomScreen/CreateRoomScreenViewModel.swift b/ElementX/Sources/Screens/CreateRoomScreen/CreateRoomScreenViewModel.swift index 3e5254330..2beeca0ae 100644 --- a/ElementX/Sources/Screens/CreateRoomScreen/CreateRoomScreenViewModel.swift +++ b/ElementX/Sources/Screens/CreateRoomScreen/CreateRoomScreenViewModel.swift @@ -282,10 +282,8 @@ class CreateRoomScreenViewModel: CreateRoomScreenViewModelType, CreateRoomScreen } } - if let selectedSpace = state.bindings.selectedSpace, - case .failure = await userSession.clientProxy.spaceService.addChild(roomID, to: selectedSpace.id) { - MXLog.error("Failed to add the created room with id: \(roomID) to the space with id: \(selectedSpace.id)") - userIndicatorController.submitIndicator(.init(title: L10n.errorUnknown)) + if let spaceID = state.bindings.selectedSpace?.id { + await addRoomToSpace(roomProxy: roomProxy, spaceID: spaceID) } actionsSubject.send(.createdRoom(roomProxy, spaceRoomListProxy)) @@ -296,6 +294,25 @@ class CreateRoomScreenViewModel: CreateRoomScreenViewModelType, CreateRoomScreen } } + private func addRoomToSpace(roomProxy: JoinedRoomProxyProtocol, spaceID: String) async { + roomProxy.subscribeToRoomInfoUpdates() + let runner = ExpiringTaskRunner { + // Necessary to build the room cache so that the space can be added as a parent. + _ = await roomProxy.infoPublisher.values.first { $0.powerLevels != nil } + } + + do { + try await runner.run(timeout: .seconds(30)) + if case .failure = await userSession.clientProxy.spaceService.addChild(roomProxy.id, to: spaceID) { + MXLog.error("Failed to add the created room with id: \(roomProxy.id) to the space with id: \(spaceID)") + userIndicatorController.submitIndicator(.init(title: L10n.errorUnknown)) + } + } catch { + MXLog.error("Timed out waiting for power levels to load for room with id: \(roomProxy.id)") + userIndicatorController.submitIndicator(.init(title: L10n.errorUnknown)) + } + } + // MARK: Loading indicator private static let loadingIndicatorIdentifier = "\(CreateRoomScreenViewModel.self)-Loading" diff --git a/ElementX/Sources/Screens/CreateRoomScreen/View/CreateRoomScreen.swift b/ElementX/Sources/Screens/CreateRoomScreen/View/CreateRoomScreen.swift index d92b8a660..39c798b80 100644 --- a/ElementX/Sources/Screens/CreateRoomScreen/View/CreateRoomScreen.swift +++ b/ElementX/Sources/Screens/CreateRoomScreen/View/CreateRoomScreen.swift @@ -238,8 +238,8 @@ struct CreateRoomScreen: View { description: L10n.screenCreateRoomSpaceSelectionNoSpaceDescription, icon: CompoundIcon(\.homeSolid, size: .small, relativeTo: .body) .foregroundColor(.compound.iconPrimary) - .background(.compound.bgSubtleSecondary) .scaledFrame(size: 32) + .background(.compound.bgSubtleSecondary) .clipAvatar(isSpace: true, size: 32)), kind: .navigationLink { context.showSpaceSelectionSheet = true @@ -336,166 +336,63 @@ private struct CreateRoomAccessRow: View { // MARK: - Previews struct CreateRoom_Previews: PreviewProvider, TestablePreview { - static let viewModel = { - AppSettings.resetAllSettings() - let userSession = UserSessionMock(.init(clientProxy: ClientProxyMock(.init(userID: "@userid:example.com")))) - return CreateRoomScreenViewModel(isSpace: false, - spaceSelectionMode: .list, - shouldShowCancelButton: false, - userSession: userSession, - analytics: ServiceLocator.shared.analytics, - userIndicatorController: UserIndicatorControllerMock(), - appSettings: ServiceLocator.shared.settings) - }() + static let viewModel = makeViewModel() static let avatarViewModel = { - AppSettings.resetAllSettings() - let userSession = UserSessionMock(.init(clientProxy: ClientProxyMock(.init(userID: "@userid:example.com")))) - let viewModel = CreateRoomScreenViewModel(isSpace: false, - spaceSelectionMode: .list, - shouldShowCancelButton: false, - userSession: userSession, - analytics: ServiceLocator.shared.analytics, - userIndicatorController: UserIndicatorControllerMock(), - appSettings: ServiceLocator.shared.settings) + let viewModel = makeViewModel() viewModel.updateAvatar(fileURL: Bundle.main.url(forResource: "preview_avatar_room", withExtension: "jpg") ?? .picturesDirectory) return viewModel }() - static let spaceViewModel = { - AppSettings.resetAllSettings() - let userSession = UserSessionMock(.init(clientProxy: ClientProxyMock(.init(userID: "@userid:example.com")))) - return CreateRoomScreenViewModel(isSpace: true, - spaceSelectionMode: nil, - shouldShowCancelButton: true, - userSession: userSession, - analytics: ServiceLocator.shared.analytics, - userIndicatorController: UserIndicatorControllerMock(), - appSettings: ServiceLocator.shared.settings) - }() + static let spaceViewModel = makeViewModel(isSpace: true, selectionMode: nil) static let spaceWithAvatarViewModel = { - AppSettings.resetAllSettings() - let userSession = UserSessionMock(.init(clientProxy: ClientProxyMock(.init(userID: "@userid:example.com")))) - let viewModel = CreateRoomScreenViewModel(isSpace: true, - spaceSelectionMode: nil, - shouldShowCancelButton: true, - userSession: userSession, - analytics: ServiceLocator.shared.analytics, - userIndicatorController: UserIndicatorControllerMock(), - appSettings: ServiceLocator.shared.settings) + let viewModel = makeViewModel(isSpace: true, selectionMode: nil) viewModel.updateAvatar(fileURL: Bundle.main.url(forResource: "preview_avatar_room", withExtension: "jpg") ?? .picturesDirectory) return viewModel }() static let publicRoomViewModel = { - AppSettings.resetAllSettings() - let userSession = UserSessionMock(.init(clientProxy: ClientProxyMock(.init(userIDServerName: "example.org", userID: "@userid:example.com")))) - let viewModel = CreateRoomScreenViewModel(isSpace: false, - spaceSelectionMode: .list, - shouldShowCancelButton: false, - userSession: userSession, - analytics: ServiceLocator.shared.analytics, - userIndicatorController: UserIndicatorControllerMock(), - appSettings: ServiceLocator.shared.settings) + let viewModel = makeViewModel() viewModel.context.selectedAccessType = .public return viewModel }() static let askToJoinViewModel = { - AppSettings.resetAllSettings() - let appSettings = AppSettings() - appSettings.knockingEnabled = true - let userSession = UserSessionMock(.init(clientProxy: ClientProxyMock(.init(userIDServerName: "example.org", userID: "@userid:example.com")))) - let viewModel = CreateRoomScreenViewModel(isSpace: false, - spaceSelectionMode: .list, - shouldShowCancelButton: false, - userSession: userSession, - analytics: ServiceLocator.shared.analytics, - userIndicatorController: UserIndicatorControllerMock(), - appSettings: appSettings) + let viewModel = makeViewModel(isKnockingEnabled: true) viewModel.context.selectedAccessType = .askToJoin return viewModel }() static let publicRoomInvalidAliasViewModel = { - AppSettings.resetAllSettings() - let userSession = UserSessionMock(.init(clientProxy: ClientProxyMock(.init(userIDServerName: "example.org", userID: "@userid:example.com")))) - let viewModel = CreateRoomScreenViewModel(isSpace: false, - spaceSelectionMode: .list, - shouldShowCancelButton: false, - userSession: userSession, - analytics: ServiceLocator.shared.analytics, - userIndicatorController: UserIndicatorControllerMock(), - appSettings: ServiceLocator.shared.settings) + let viewModel = makeViewModel() viewModel.context.selectedAccessType = .public viewModel.context.send(viewAction: .updateAliasLocalPart("#:")) return viewModel }() static let publicRoomExistingAliasViewModel = { - AppSettings.resetAllSettings() - let clientProxy = ClientProxyMock(.init(userIDServerName: "example.org", userID: "@userid:example.com")) - clientProxy.isAliasAvailableReturnValue = .success(false) - let userSession = UserSessionMock(.init(clientProxy: clientProxy)) - let viewModel = CreateRoomScreenViewModel(isSpace: false, - spaceSelectionMode: .list, - shouldShowCancelButton: false, - userSession: userSession, - analytics: ServiceLocator.shared.analytics, - userIndicatorController: UserIndicatorControllerMock(), - appSettings: ServiceLocator.shared.settings) + let viewModel = makeViewModel(isAliasAvailable: false) viewModel.context.selectedAccessType = .public viewModel.context.send(viewAction: .updateAliasLocalPart("existing")) return viewModel }() - static let selectedSpaceViewModel = { - AppSettings.resetAllSettings() - let mockedSpace = SpaceServiceRoomMock(.init(name: "Awesome Space", isSpace: true, joinRule: .private)) - let userSession = UserSessionMock(.init(clientProxy: ClientProxyMock(.init(userID: "@userid:example.com")))) - return CreateRoomScreenViewModel(isSpace: false, - spaceSelectionMode: .selected(mockedSpace), - shouldShowCancelButton: false, - userSession: userSession, - analytics: ServiceLocator.shared.analytics, - userIndicatorController: UserIndicatorControllerMock(), - appSettings: ServiceLocator.shared.settings) - }() + static let selectedSpaceViewModel = makeViewModel(selectionMode: .selected(SpaceServiceRoomMock(.init(name: "Awesome Space", + isSpace: true, + joinRule: .private)))) static let selectedSpaceWithListViewModel = { - let clientProxy = ClientProxyMock(.init(userID: "@userid:example.com")) - let spaces = [SpaceServiceRoomProtocol].mockJoinedSpaces2 - clientProxy.spaceService = SpaceServiceProxyMock(.init(editableSpaces: spaces)) - let userSession = UserSessionMock(.init(clientProxy: clientProxy)) - - let viewModel = CreateRoomScreenViewModel(isSpace: false, - spaceSelectionMode: .list, - shouldShowCancelButton: false, - userSession: userSession, - analytics: ServiceLocator.shared.analytics, - userIndicatorController: UserIndicatorControllerMock(), - appSettings: ServiceLocator.shared.settings) - - viewModel.context.selectedSpace = spaces[0] + let viewModel = makeViewModel() + viewModel.context.selectedSpace = [SpaceServiceRoomProtocol].mockJoinedSpaces2.first return viewModel }() static let selectedSpaceWithAskToJoinViewModel = { - AppSettings.resetAllSettings() - let appSettings = AppSettings() - appSettings.knockingEnabled = true - - let mockedSpace = SpaceServiceRoomMock(.init(name: "Awesome Space", isSpace: true, joinRule: .private)) - let userSession = UserSessionMock(.init(clientProxy: ClientProxyMock(.init(userID: "@userid:example.com")))) - let viewModel = CreateRoomScreenViewModel(isSpace: false, - spaceSelectionMode: .selected(mockedSpace), - shouldShowCancelButton: false, - userSession: userSession, - analytics: ServiceLocator.shared.analytics, - userIndicatorController: UserIndicatorControllerMock(), - appSettings: appSettings) - + let viewModel = makeViewModel(isKnockingEnabled: true, + selectionMode: .selected(SpaceServiceRoomMock(.init(name: "Awesome Space", + isSpace: true, + joinRule: .private)))) viewModel.context.selectedAccessType = .askToJoinWithSpaceMembers return viewModel }() @@ -560,4 +457,28 @@ struct CreateRoom_Previews: PreviewProvider, TestablePreview { } .previewDisplayName("Create Knockable Room with already selected Space") } + + private static func makeViewModel(isKnockingEnabled: Bool = false, + isSpace: Bool = false, + selectionMode: CreateRoomScreenSpaceSelectionMode? = .list, + isAliasAvailable: Bool = true) -> CreateRoomScreenViewModel { + AppSettings.resetAllSettings() + let appSettings = AppSettings() + appSettings.knockingEnabled = isKnockingEnabled + + let clientProxy = ClientProxyMock(.init(userIDServerName: "example.org", + userID: "@userid:example.com")) + clientProxy.isAliasAvailableReturnValue = .success(isAliasAvailable) + let spaces = [SpaceServiceRoomProtocol].mockJoinedSpaces2 + clientProxy.spaceService = SpaceServiceProxyMock(.init(editableSpaces: spaces)) + let userSession = UserSessionMock(.init(clientProxy: clientProxy)) + + return CreateRoomScreenViewModel(isSpace: isSpace, + spaceSelectionMode: selectionMode, + shouldShowCancelButton: isSpace, + userSession: userSession, + analytics: ServiceLocator.shared.analytics, + userIndicatorController: UserIndicatorControllerMock(), + appSettings: appSettings) + } } diff --git a/ElementX/Sources/Screens/CreateRoomScreen/View/CreateRoomSpaceSelectionSheet.swift b/ElementX/Sources/Screens/CreateRoomScreen/View/CreateRoomSpaceSelectionSheet.swift index 9e4666a9d..cc75f96cf 100644 --- a/ElementX/Sources/Screens/CreateRoomScreen/View/CreateRoomSpaceSelectionSheet.swift +++ b/ElementX/Sources/Screens/CreateRoomScreen/View/CreateRoomSpaceSelectionSheet.swift @@ -28,6 +28,7 @@ struct CreateRoomSpaceSelectionSheet: View { description: L10n.screenCreateRoomSpaceSelectionNoSpaceDescription, icon: CompoundIcon(\.homeSolid, size: .small, relativeTo: .body) .foregroundColor(.compound.iconPrimary) + .scaledFrame(size: 32) .background(.compound.bgSubtleSecondary) .scaledFrame(size: 32) .clipAvatar(isSpace: true, size: 32)), diff --git a/ElementX/Sources/Services/Spaces/SpaceServiceProxy.swift b/ElementX/Sources/Services/Spaces/SpaceServiceProxy.swift index cfa82cd3b..0fde5b02f 100644 --- a/ElementX/Sources/Services/Spaces/SpaceServiceProxy.swift +++ b/ElementX/Sources/Services/Spaces/SpaceServiceProxy.swift @@ -80,7 +80,7 @@ class SpaceServiceProxy: SpaceServiceProxyProtocol { func editableSpaces() async -> [SpaceServiceRoomProtocol] { await spaceService.editableSpaces().map(SpaceServiceRoom.init) } - + func addChild(_ childID: String, to spaceID: String) async -> Result { do { return try await .success(spaceService.addChildToSpace(childId: childID, spaceId: spaceID)) @@ -98,7 +98,7 @@ class SpaceServiceProxy: SpaceServiceProxyProtocol { return .failure(.sdkError(error)) } } - + // MARK: - Private private func handleSpaceListUpdates(_ updates: [SpaceListUpdate]) { diff --git a/ElementX/Sources/Services/Spaces/SpaceServiceProxyProtocol.swift b/ElementX/Sources/Services/Spaces/SpaceServiceProxyProtocol.swift index 2f7ab335c..fb99d2256 100644 --- a/ElementX/Sources/Services/Spaces/SpaceServiceProxyProtocol.swift +++ b/ElementX/Sources/Services/Spaces/SpaceServiceProxyProtocol.swift @@ -52,7 +52,7 @@ protocol SpaceServiceProxyProtocol { func leaveSpace(spaceID: String) async -> Result /// Returns all the parent spaces of a child that user has joined. func joinedParents(childID: String) async -> Result<[SpaceServiceRoomProtocol], SpaceServiceProxyError> - /// Returns all the parent spaces that can be edited by the user + /// Returns all the joined spaces that can be edited by the user func editableSpaces() async -> [SpaceServiceRoomProtocol] /// Adds a room (or space) as a child of another space. diff --git a/UnitTests/Sources/CreateRoomViewModelTests.swift b/UnitTests/Sources/CreateRoomViewModelTests.swift index ad504bb67..210df6375 100644 --- a/UnitTests/Sources/CreateRoomViewModelTests.swift +++ b/UnitTests/Sources/CreateRoomViewModelTests.swift @@ -226,9 +226,7 @@ class CreateRoomScreenViewModelTests: XCTestCase { let spaces = [SpaceServiceRoomProtocol].mockJoinedSpaces2 setup() - // Given a form with a blank topic. context.send(viewAction: .updateRoomName("A")) - context.roomTopic = "" context.selectedAccessType = .public XCTAssertTrue(context.viewState.canCreateRoom) XCTAssertNil(context.selectedSpace) @@ -268,21 +266,16 @@ class CreateRoomScreenViewModelTests: XCTestCase { await fulfillment(of: [expectation]) try await deferredAction.fulfill() - // Then the room should be created and a topic should not be set. XCTAssertTrue(clientProxy.createRoomNameTopicAccessTypeIsSpaceUserIDsAvatarURLAliasLocalPartCalled) XCTAssertEqual(clientProxy.createRoomNameTopicAccessTypeIsSpaceUserIDsAvatarURLAliasLocalPartReceivedArguments?.name, "A") XCTAssertEqual(clientProxy.createRoomNameTopicAccessTypeIsSpaceUserIDsAvatarURLAliasLocalPartReceivedArguments?.accessType, .private) - XCTAssertNil(clientProxy.createRoomNameTopicAccessTypeIsSpaceUserIDsAvatarURLAliasLocalPartReceivedArguments?.topic, - "The topic should be sent as nil when it is empty.") } func testCreateRoomInAnAlreadySelectedSpace() async throws { let space = SpaceServiceRoomMock(.init(isSpace: true, joinRule: .private)) setup(spacesSelectionMode: .selected(space)) - // Given a form with a blank topic. context.send(viewAction: .updateRoomName("A")) - context.roomTopic = "" context.selectedAccessType = .spaceMembers XCTAssertTrue(context.viewState.canCreateRoom) XCTAssertEqual(context.selectedSpace?.id, space.id) @@ -308,12 +301,9 @@ class CreateRoomScreenViewModelTests: XCTestCase { await fulfillment(of: [expectation]) try await deferredAction.fulfill() - // Then the room should be created and a topic should not be set. XCTAssertTrue(clientProxy.createRoomNameTopicAccessTypeIsSpaceUserIDsAvatarURLAliasLocalPartCalled) XCTAssertEqual(clientProxy.createRoomNameTopicAccessTypeIsSpaceUserIDsAvatarURLAliasLocalPartReceivedArguments?.name, "A") XCTAssertEqual(clientProxy.createRoomNameTopicAccessTypeIsSpaceUserIDsAvatarURLAliasLocalPartReceivedArguments?.accessType, .spaceMembers(spaceID: space.id)) - XCTAssertNil(clientProxy.createRoomNameTopicAccessTypeIsSpaceUserIDsAvatarURLAliasLocalPartReceivedArguments?.topic, - "The topic should be sent as nil when it is empty.") } func testCreateRoomInAnPublicSpaceAvailableTypes() async throws {