From 2a7eb7eed2444cfd95e83b223d1cbcd59cd923e3 Mon Sep 17 00:00:00 2001 From: Narasimha-sc <166327228+Narasimha-sc@users.noreply.github.com> Date: Thu, 6 Aug 2026 17:27:35 +0000 Subject: [PATCH] fix the review's findings and cut the remaining API surface Correctness. The one-time link picker's profile list was keyed on the active user as well as the count. That defeats the "don't change order after a user was selected" behaviour its own comment protects - changeActiveUser_ sets currentUser first and only then reloads chats, so the list visibly re-sorted and stayed re-sorted for the whole of getUserChatData - and it was redundant: the count alone covers a profile being created from the picker. iOS, compose picker: the old-core fallback relied on the user switch tearing this view down, which only happens when the switch succeeded. If it threw, the form was left open over a profile that already existed, and the only way out was to swipe it away. It is now dismissed unconditionally. iOS: alerts raised right after a sheet is dismissed are presented on a controller that is going away and are dropped. The 0.5s wait was applied only to the old-core path, while the ordinary reassignment failures - the ones that actually happen - went unguarded. alertAfterDismissal now covers all of them, in one place. iOS: dropped the chatId write that Kotlin removed for the same reason two commits ago. It is a no-op on the happy path, but not if something legitimately closed the chat meanwhile - SimpleX lock re-auth clears it, and this would reopen the chat behind the lock screen. iOS: the create-profile row is now gated on profileChangeProhibited, as every other row on both platforms already was. Surface. apiCreateActiveUser takes keepActiveUser as a defaulted argument, exactly as it already takes pastTimestamp, instead of a public wrapper plus a private helper. Two symbols fewer on each platform, and every existing call site passes its arguments by name, so none of them changes. Reverted the activeOrder sort flip in the compose picker: it changes the order of an existing screen for every user, is not needed for this feature, and with active_order 0 would push a newly created profile to the bottom. It deserves its own commit if wanted. Also: apps/ios/spec/api.md tracks the command enum and had gone stale; the plan records the two limits of the flag the review surfaced - it is ignored when there is no active user, and active_order 0 ties rather than sorts last on migrated databases. --- apps/ios/Shared/Model/AppAPITypes.swift | 3 - apps/ios/Shared/Model/SimpleXAPI.swift | 17 ++---- .../ContextProfilePickerView.swift | 55 ++++++++----------- .../ios/Shared/Views/Helpers/ShareSheet.swift | 9 +++ .../Shared/Views/NewChat/NewChatView.swift | 9 +-- apps/ios/spec/api.md | 2 +- .../chat/simplex/common/model/SimpleXAPI.kt | 17 ++---- .../chat/simplex/common/views/WelcomeView.kt | 2 +- .../common/views/newchat/NewChatView.kt | 4 +- .../2026-07-30-new-profile-for-invitation.md | 11 ++++ 10 files changed, 59 insertions(+), 70 deletions(-) diff --git a/apps/ios/Shared/Model/AppAPITypes.swift b/apps/ios/Shared/Model/AppAPITypes.swift index c1430d51ff..0bd44e44c3 100644 --- a/apps/ios/Shared/Model/AppAPITypes.swift +++ b/apps/ios/Shared/Model/AppAPITypes.swift @@ -1363,9 +1363,6 @@ struct NewUser: Encodable { var profile: Profile? var pastTimestamp: Bool var userChatRelay: Bool = false - // when set, the user is created without becoming active, preserving the current one; - // absent/false activates it as before. The response is activeUser either way - it - // carries the created user, which is then not the active one. var keepActiveUser: Bool = false } diff --git a/apps/ios/Shared/Model/SimpleXAPI.swift b/apps/ios/Shared/Model/SimpleXAPI.swift index a1f342a023..1adbb5ca89 100644 --- a/apps/ios/Shared/Model/SimpleXAPI.swift +++ b/apps/ios/Shared/Model/SimpleXAPI.swift @@ -253,18 +253,11 @@ func apiGetActiveUser(ctrl: chat_ctrl? = nil) throws -> User? { } } -func apiCreateActiveUser(_ p: Profile?, pastTimestamp: Bool = false, ctrl: chat_ctrl? = nil) throws -> User { - try createUser(p, pastTimestamp: pastTimestamp, keepActiveUser: false, ctrl: ctrl) -} - -// Creates a profile *without* activating it: apiChangePreparedContactUser resolves the -// prepared chat under the active user, so the profile that owns it must stay active until -// the chat has moved. The returned user is therefore not the active one. -func apiCreateProfileKeepingActive(_ p: Profile) throws -> User { - try createUser(p, pastTimestamp: false, keepActiveUser: true, ctrl: nil) -} - -private func createUser(_ p: Profile?, pastTimestamp: Bool, keepActiveUser: Bool, ctrl: chat_ctrl?) throws -> User { +/// keepActiveUser creates the profile *without* activating it, for the invitation +/// pickers: the reassignment APIs resolve the prepared chat or connection under the active +/// user, so the profile that owns it has to stay active until it has moved. The response +/// is the created user either way, which is then not the active one. +func apiCreateActiveUser(_ p: Profile?, pastTimestamp: Bool = false, keepActiveUser: Bool = false, ctrl: chat_ctrl? = nil) throws -> User { let r: ChatResponse0 = try chatSendCmdSync(.createActiveUser(profile: p, pastTimestamp: pastTimestamp, keepActiveUser: keepActiveUser), ctrl: ctrl) if case let .activeUser(user) = r { return user } throw r.unexpected diff --git a/apps/ios/Shared/Views/Chat/ComposeMessage/ContextProfilePickerView.swift b/apps/ios/Shared/Views/Chat/ComposeMessage/ContextProfilePickerView.swift index 180c70b3b1..c21ef9b2f7 100644 --- a/apps/ios/Shared/Views/Chat/ComposeMessage/ContextProfilePickerView.swift +++ b/apps/ios/Shared/Views/Chat/ComposeMessage/ContextProfilePickerView.swift @@ -101,8 +101,7 @@ struct ContextProfilePickerView: View { let otherUsers = users .filter { u in u.userId != selectedUser.userId } - // Descending, as every other profile list sorts - .sorted(using: KeyPathComparator(\.activeOrder, order: .reverse)) + .sorted(using: KeyPathComparator(\.activeOrder)) ForEach(otherUsers) { p in profilerPickerUserOption(p) .contentShape(Rectangle()) @@ -221,7 +220,11 @@ struct ContextProfilePickerView: View { private func addProfileOption() -> some View { Button { - showAddProfile = true + if chat.chatInfo.profileChangeProhibited { + showCantChangeProfileAlert() + } else { + showAddProfile = true + } } label: { HStack { Image(systemName: "person.crop.circle.badge.plus") @@ -257,7 +260,7 @@ struct ContextProfilePickerView: View { if alreadyCreating { return } defer { Task { @MainActor in creatingProfile = false } } let profile = Profile(displayName: displayName, fullName: "", shortDescr: shortDescr, image: image) - let newUser = try apiCreateProfileKeepingActive(profile) + let newUser = try apiCreateActiveUser(profile, keepActiveUser: true) let updatedUsers = try? listUsers() await MainActor.run { if let updatedUsers = updatedUsers { @@ -281,23 +284,19 @@ struct ContextProfilePickerView: View { switched = false } await MainActor.run { + // Dismissed unconditionally: the switch removes this view - and the sheet + // it presents - only when it succeeded. If it threw, the form would be + // left over a profile that already exists. + showAddProfile = false // Only if the switch happened: the chat is then absent from the reloaded // list and would render blank. If it failed, the chat is still fine. if switched && chatModel.chatId == chat.id { chatModel.chatId = nil } } - // The switch replaces the chat list, which removes this view - and the sheet - // it presents - from the hierarchy. Both that teardown and an explicit - // dismissal animate, and getTopViewController() keeps returning the sheet - // until the transition ends, so an alert raised now is presented on a - // controller being dismissed and dropped. Let it settle first. - DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { - showAlert(NSLocalizedString("Error changing chat profile", comment: "alert title")) - } + alertAfterDismissal(NSLocalizedString("Error changing chat profile", comment: "alert title")) return } - // changingProfile set here, not only inside changeProfile's Task: the defer above - // clears creatingProfile as soon as this function returns, and that Task has not - // necessarily started by then - the rows would be live in between. + // changingProfile here too: the defer above clears creatingProfile as soon as this + // returns, and changeProfile's Task has not necessarily started by then. await MainActor.run { showAddProfile = false changingProfile = true @@ -307,9 +306,6 @@ struct ContextProfilePickerView: View { private func changeProfile(_ newUser: User) { Task { - // Two round trips follow; without this every row, including "Add profile", - // stays live and a second change can be started on top of this one. - await MainActor.run { changingProfile = true } defer { Task { @MainActor in changingProfile = false } } do { if let contact = chat.chatInfo.contact { @@ -331,29 +327,22 @@ struct ContextProfilePickerView: View { } do { try await changeActiveUserAsync_(newUser.userId, viewPwd: nil, keepingChatId: chat.id) - // Assert the open chat: nothing on this path clears chatId on iOS, so - // this is normally a no-op, but keepingChatId only keeps the chat's - // place in the reloaded list - it does not open it. The id is - // unchanged by the reassignment, it is the contact/group id. - await MainActor.run { chatModel.chatId = chat.id } } catch { - await MainActor.run { - showAlert( - NSLocalizedString("Error switching profile", comment: "alert title"), - message: String.localizedStringWithFormat(NSLocalizedString("Your chat was moved to %@ but an unexpected error occurred while redirecting you to the profile.", comment: "alert message"), newUser.chatViewName) - ) - } + alertAfterDismissal( + NSLocalizedString("Error switching profile", comment: "alert title"), + String.localizedStringWithFormat(NSLocalizedString("Your chat was moved to %@ but an unexpected error occurred while redirecting you to the profile.", comment: "alert message"), newUser.chatViewName) + ) } } catch let error { await MainActor.run { if let currentUser = chatModel.currentUser { selectedUser = currentUser } - showAlert( - NSLocalizedString("Error changing chat profile", comment: "alert title"), - message: responseError(error) - ) } + alertAfterDismissal( + NSLocalizedString("Error changing chat profile", comment: "alert title"), + responseError(error) + ) } } } diff --git a/apps/ios/Shared/Views/Helpers/ShareSheet.swift b/apps/ios/Shared/Views/Helpers/ShareSheet.swift index 670cc7cae0..4fdacc0ed0 100644 --- a/apps/ios/Shared/Views/Helpers/ShareSheet.swift +++ b/apps/ios/Shared/Views/Helpers/ShareSheet.swift @@ -70,6 +70,15 @@ func showAlert( } } +/// An alert raised while a sheet is dismissing is presented on a controller that is going +/// away, and is dropped - getTopViewController() keeps returning it until the transition +/// ends. Use this when the caller has just dismissed something. +func alertAfterDismissal(_ title: String, _ message: String? = nil) { + DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { + showAlert(title, message: message) + } +} + func showSheet( _ title: String?, message: String? = nil, diff --git a/apps/ios/Shared/Views/NewChat/NewChatView.swift b/apps/ios/Shared/Views/NewChat/NewChatView.swift index 8478ac299d..e7807e9efb 100644 --- a/apps/ios/Shared/Views/NewChat/NewChatView.swift +++ b/apps/ios/Shared/Views/NewChat/NewChatView.swift @@ -610,7 +610,7 @@ private struct ActiveProfilePicker: View { if alreadyCreating { return } defer { Task { @MainActor in creatingProfile = false } } let profile = Profile(displayName: displayName, fullName: "", shortDescr: shortDescr, image: image) - let newUser = try apiCreateProfileKeepingActive(profile) + let newUser = try apiCreateActiveUser(profile, keepActiveUser: true) let updatedUsers = try? listUsers() await MainActor.run { if let updatedUsers = updatedUsers { chatModel.users = updatedUsers } @@ -635,12 +635,7 @@ private struct ActiveProfilePicker: View { profileSwitchStatus = .idle selectedProfile = newUser } - // getTopViewController() keeps returning the sheet until its dismissal - // transition ends, and an alert presented on a controller being dismissed is - // dropped - so let it settle first. - DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) { - showAlert(NSLocalizedString("Error changing chat profile", comment: "alert title")) - } + alertAfterDismissal(NSLocalizedString("Error changing chat profile", comment: "alert title")) return } await MainActor.run { diff --git a/apps/ios/spec/api.md b/apps/ios/spec/api.md index f9a3c35917..02cd401077 100644 --- a/apps/ios/spec/api.md +++ b/apps/ios/spec/api.md @@ -50,7 +50,7 @@ The `ChatCommand` enum ([`AppAPITypes.swift` L15](../Shared/Model/AppAPITypes.sw | Command | Parameters | Description | Source | |---------|-----------|-------------|--------| | `showActiveUser` | -- | Get current active user | [L16](../Shared/Model/AppAPITypes.swift#L16) | -| `createActiveUser` | `profile: Profile?, pastTimestamp: Bool` | Create new user profile | [L17](../Shared/Model/AppAPITypes.swift#L17) | +| `createActiveUser` | `profile: Profile?, pastTimestamp: Bool, keepActiveUser: Bool` | Create new user profile; `keepActiveUser` creates it without activating it | [L17](../Shared/Model/AppAPITypes.swift#L17) | | `listUsers` | -- | List all user profiles | [L18](../Shared/Model/AppAPITypes.swift#L18) | | `apiSetActiveUser` | `userId: Int64, viewPwd: String?` | Switch active user | [L19](../Shared/Model/AppAPITypes.swift#L19) | | `apiHideUser` | `userId: Int64, viewPwd: String` | Hide user behind password | [L24](../Shared/Model/AppAPITypes.swift#L24) | diff --git a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/SimpleXAPI.kt b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/SimpleXAPI.kt index c40e2cbe7a..e7aa50695a 100644 --- a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/SimpleXAPI.kt +++ b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/model/SimpleXAPI.kt @@ -880,16 +880,11 @@ object ChatController { return null } - suspend fun apiCreateActiveUser(rh: Long?, p: Profile?, pastTimestamp: Boolean = false, ctrl: ChatCtrl? = null): User? = - createUser(rh, p, pastTimestamp = pastTimestamp, keepActiveUser = false, ctrl = ctrl) - - /** Creates a profile *without* activating it: [apiChangePreparedContactUser] resolves the - * prepared chat under the active user, so the profile that owns it must stay active until - * the chat has moved. The returned user is therefore not the active one. */ - suspend fun apiCreateProfileKeepingActive(rh: Long?, p: Profile): User? = - createUser(rh, p, pastTimestamp = false, keepActiveUser = true, ctrl = null) - - private suspend fun createUser(rh: Long?, p: Profile?, pastTimestamp: Boolean, keepActiveUser: Boolean, ctrl: ChatCtrl?): User? { + /** [keepActiveUser] creates the profile *without* activating it, for the invitation + * pickers: the reassignment APIs resolve the prepared chat or connection under the + * active user, so the profile that owns it has to stay active until it has moved. The + * response is the created user either way, which is then not the active one. */ + suspend fun apiCreateActiveUser(rh: Long?, p: Profile?, pastTimestamp: Boolean = false, keepActiveUser: Boolean = false, ctrl: ChatCtrl? = null): User? { val r = sendCmd(rh, CC.CreateActiveUser(p, pastTimestamp = pastTimestamp, keepActiveUser = keepActiveUser), ctrl) if (r is API.Result && r.res is CR.ActiveUser) return r.res.user.updateRemoteHostId(rh) val e = (r as? API.Error)?.err @@ -905,7 +900,7 @@ object ChatController { } else { AlertManager.shared.showAlertMsg(generalGetString(MR.strings.failed_to_create_user_title), r.details) } - Log.d(TAG, "createUser (keepActiveUser=$keepActiveUser): ${r.responseType} ${r.details}") + Log.d(TAG, "apiCreateActiveUser (keepActiveUser=$keepActiveUser): ${r.responseType} ${r.details}") return null } diff --git a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/WelcomeView.kt b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/WelcomeView.kt index 41d98cbdc1..d5bc150bbd 100644 --- a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/WelcomeView.kt +++ b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/WelcomeView.kt @@ -368,7 +368,7 @@ fun createProfileForInvitation(rhId: Long?, onCreated: suspend (User) -> Unit) { try { val ownerUserId = chatModel.currentUser.value?.userId val profile = Profile(displayName.trim(), "", shortDescr.trim().ifEmpty { null }, image = image) - val newUser = controller.apiCreateProfileKeepingActive(rhId, profile) ?: return@withApi + val newUser = controller.apiCreateActiveUser(rhId, profile, keepActiveUser = true) ?: return@withApi if (newUser.activeUser) { // An older remote host ignored the flag and activated it, so the reassignment // would fail. Resync to what the host did and report it, with the form diff --git a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/newchat/NewChatView.kt b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/newchat/NewChatView.kt index ffb7f16faf..cf1334a95a 100644 --- a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/newchat/NewChatView.kt +++ b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/newchat/NewChatView.kt @@ -300,8 +300,8 @@ fun ActiveProfilePicker( val selectedProfile by remember { chatModel.currentUser } val searchTextOrPassword = rememberSaveable { search } // Intentionally don't use derivedStateOf in order to NOT change an order after user was selected. - // Keyed on the users too, or a profile created from this picker is missing from it. - val filteredProfiles = remember(searchTextOrPassword.value, chatModel.users.size, chatModel.currentUser.value?.userId) { + // Keyed on the count too, or a profile created from this picker is missing from it. + val filteredProfiles = remember(searchTextOrPassword.value, chatModel.users.size) { filteredProfiles(chatModel.users.map { it.user }.sortedBy { !it.activeUser }, searchTextOrPassword.value) } diff --git a/plans/2026-07-30-new-profile-for-invitation.md b/plans/2026-07-30-new-profile-for-invitation.md index e8cd1b76b8..ac967198bc 100644 --- a/plans/2026-07-30-new-profile-for-invitation.md +++ b/plans/2026-07-30-new-profile-for-invitation.md @@ -72,6 +72,17 @@ window with the opposite outcome. and older callers are untouched. The flag is ignored when there is no active user to keep, which would otherwise leave none at all. +Two limits of the flag, both deliberate and neither covered by a test: + +- It is **ignored when there is no active user** (`isNothing curUser_`), which would + otherwise leave none at all. A client cannot distinguish that from a stale host that + dropped the field — both come back as `activeUser = True`. +- `active_order = 0` for a profile that was never activated sorts it **below** activated + ones, but `M20240920_user_order` back-filled every existing row with 0, so on a migrated + database it *ties* with them and the order falls back to row order. `userQuery` has no + `ORDER BY`, and the terminal harness cannot observe `activeOrder`, so this is stated + rather than tested. + Response stays `CRActiveUser` — it carries the created user, which on this path is not the active one. Documented at the field; no client decoder changes.