android, desktop: guard the picker's own dismissal and drop the redundant chatId write

Third review pass on this flow. Four things, all in code the previous two passes
touched.

The one-time link picker's close(). selectProfileAsync ended with an unconditional
close(), which pops whatever is on top of the start stack. The reassignment before it
can wait indefinitely on the retry alert, and back is not blocked while it does - so
backing out and then letting the call complete dismissed the New chat screen as well.
The picker now has its own ModalViewId and only closes if it is still the top, which is
what isLastModalOpenNotClosing was added for; the create-profile flow was using it and
this, the older hazard, was not.

The entry guard used hasModalOpen, which counts a modal that is still animating out, so
re-opening the form straight after backing out of it was a silent no-op for the length
of the animation - the exact bug the NotClosing variant exists for. Adds the matching
hasModalOpenNotClosing.

The old-core fallback switched the active user and returned without closing the form,
leaving it on top of a chat list belonging to a different profile, and did it off the
main thread. It now closes first and runs on Main like the rest of the flow.

chatModel.chatId is no longer written after the switch. changeActiveUser_ passes
keepingChatId and updateChats only clears chatId when the chat is missing from the
reloaded list, so on success the write was a no-op - and in the one case the guard does
fire it re-pointed the chat view at a chat that is not there. It also undid a
notification tap that had navigated to another chat of the same profile, which the
active-user check cannot catch.

Also restores the profileChangeProhibited guard that the previous commit dropped from
the new row while collapsing its click handler; keys the one-time link picker's profile
list on the active user as well as the count, so the checkmark is not stale after a
switch that leaves the count unchanged; dims the compose picker while it is busy, which
was the only picker with no indication at all; and logs the silent abort.
This commit is contained in:
Narasimha-sc
2026-08-06 15:31:55 +00:00
parent d610757831
commit f76b0b3efd
7 changed files with 69 additions and 30 deletions
@@ -123,8 +123,8 @@ object ChatModel {
// map of connections network statuses, key is agent connection id
val switchingUsersAndHosts = mutableStateOf(false)
/** True from the moment a profile is submitted in the "add profile for this invitation"
* form until the invitation has been moved onto it. Lives here rather than in the picker
* that started it: on Android every ModalManager placement is one stack rendering only
* form until the invitation has been moved onto it. Lives on the model rather than in
* the picker that starts it: on Android every ModalManager placement is one stack rendering only
* its top entry, so the one-time link picker - itself a modal - is disposed while the
* form is above it and returns with every remembered flag reset. (The compose picker is
* not a modal and does survive, on both platforms.) */
@@ -60,7 +60,7 @@ fun bioFitsLimit(bio: String): Boolean {
}
@Composable
fun CreateProfile(onSubmit: (displayName: String, shortDescr: String, image: String?) -> Unit) {
fun CreateProfile(submitting: Boolean = false, onSubmit: (displayName: String, shortDescr: String, image: String?) -> Unit) {
val scope = rememberCoroutineScope()
val scrollState = rememberScrollState()
val keyboardState by getKeyboardState()
@@ -159,7 +159,7 @@ fun CreateProfile(onSubmit: (displayName: String, shortDescr: String, image: Str
SettingsActionItem(
painterResource(MR.images.ic_check),
stringResource(MR.strings.create_another_profile_button),
disabled = !canCreateProfile(displayName.value) || !bioFitsLimit(shortDescr.value),
disabled = submitting || !canCreateProfile(displayName.value) || !bioFitsLimit(shortDescr.value),
textColor = MaterialTheme.colors.primary,
iconColor = MaterialTheme.colors.primary,
click = { onSubmit(displayName.value, shortDescr.value, profileImage.value) },
@@ -362,13 +362,17 @@ fun createProfileForInvitation(rhId: Long?, onCreated: suspend (User) -> Unit) {
// link picker but disposes it, losing what was typed in its search box. fullscreen is
// an opaque Surface over every pane, so no picker can be operated while the form is up
// and none of them is torn down. On Android all four are the same manager anyway.
// Two taps before the modal renders would otherwise stack two modals sharing one id,
// after which close() could dismiss the wrong one.
// Not while one is still being created: the in-flight job identifies its own form only
// by this id, so a second form opened underneath it would be the one closed and the
// wrong profile handed over.
if (chatModel.creatingProfileForInvitation.value) return
// Two taps before the modal renders would otherwise stack two modals sharing one id,
// after which close() could dismiss the wrong one. NotClosing, or re-opening the form
// right after backing out of it is a silent no-op until the animation ends.
val modalManager = ModalManager.fullscreen
if (modalManager.hasModalOpen(ModalViewId.CONTEXT_USER_PICKER_NEW_PROFILE)) return
if (modalManager.hasModalOpenNotClosing(ModalViewId.CONTEXT_USER_PICKER_NEW_PROFILE)) return
modalManager.showModalCloseable(id = ModalViewId.CONTEXT_USER_PICKER_NEW_PROFILE) { close ->
CreateProfile { displayName, shortDescr, image ->
CreateProfile(submitting = chatModel.creatingProfileForInvitation.value) { displayName, shortDescr, image ->
if (chatModel.creatingProfileForInvitation.value) return@CreateProfile
chatModel.creatingProfileForInvitation.value = true
withBGApi {
@@ -383,8 +387,14 @@ fun createProfileForInvitation(rhId: Long?, onCreated: suspend (User) -> Unit) {
// remote host ignoring the unknown field. Reassigning would now fail, so resync
// to what the host actually did and report it. Not switching_profile_error_message:
// that says the invitation was moved, and on this path it was not.
controller.changeActiveUser(newUser.remoteHostId, newUser.userId, null)
AlertManager.shared.showAlertMsg(generalGetString(MR.strings.error_changing_user))
// The form is dismissed first: the app is about to be showing a different
// profile, and leaving the form on top of it invites a second attempt that
// would fail the same way.
withContext(Dispatchers.Main) {
if (modalManager.isLastModalOpenNotClosing(ModalViewId.CONTEXT_USER_PICKER_NEW_PROFILE)) close()
controller.changeActiveUser(newUser.remoteHostId, newUser.userId, null)
AlertManager.shared.showAlertMsg(generalGetString(MR.strings.error_changing_user))
}
return@withBGApi
}
// Keep chatModel.users current even if onCreated's reassignment fails - it only
@@ -414,7 +424,10 @@ fun createProfileForInvitation(rhId: Long?, onCreated: suspend (User) -> Unit) {
// The form is gone - most likely the user backed out of it while the profile
// was being created. Don't move the invitation under a screen they have left,
// and don't report an error for something they did deliberately.
if (!modalManager.isLastModalOpenNotClosing(ModalViewId.CONTEXT_USER_PICKER_NEW_PROFILE)) return@withContext
if (!modalManager.isLastModalOpenNotClosing(ModalViewId.CONTEXT_USER_PICKER_NEW_PROFILE)) {
Log.i(TAG, "createProfileForInvitation: form closed before the invitation was moved, profile ${newUser.userId} left created")
return@withContext
}
close()
onCreated(newUser)
}
@@ -11,6 +11,7 @@ import androidx.compose.material.*
import androidx.compose.runtime.*
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.alpha
import androidx.compose.ui.draw.clip
import androidx.compose.ui.platform.LocalDensity
import androidx.compose.ui.text.font.FontWeight
@@ -116,17 +117,15 @@ fun ComposeContextProfilePickerView(
viewPwd = null,
keepingChatId = chat.id
)
// No chatId assignment here. changeActiveUser_ passes keepingChatId, and
// updateChats only clears chatId when the chat is missing from the reloaded
// list - so on success this would be a no-op, and in the one case the guard
// does fire it would re-point the chat view at a chat that is not there.
if (chatModel.currentUser.value?.userId != newUser.userId) {
AlertManager.shared.showAlertMsg(
generalGetString(MR.strings.switching_profile_error_title),
String.format(generalGetString(MR.strings.switching_profile_error_message), newUser.chatViewName)
)
} else {
// Reopen the chat under the new profile - only once the switch is known to
// have happened, or this would point the chat view at a chat that now belongs
// to a different profile, defeating the guard in updateChats that clears it.
// The id is unchanged by the reassignment - it is the contact/group id.
chatModel.chatId.value = chat.id
}
}
} finally {
@@ -257,7 +256,15 @@ fun ComposeContextProfilePickerView(
Modifier
.fillMaxWidth()
.sizeIn(minHeight = DEFAULT_MIN_SECTION_ITEM_HEIGHT + 8.dp)
.clickable(enabled = !busy, onClick = { createProfileForInvitation(rhId) { changeProfileTo(it) } })
.clickable(enabled = !busy, onClick = {
// Same guard as every other row: the flag is live state the receiver loop
// flips, so it can turn true between this row being laid out and the tap.
if (!chat.chatInfo.profileChangeProhibited) {
createProfileForInvitation(rhId) { changeProfileTo(it) }
} else {
showCantChangeProfileAlert()
}
})
.padding(horizontal = DEFAULT_PADDING_HALF, vertical = 4.dp),
horizontalArrangement = Arrangement.SpaceBetween,
verticalAlignment = Alignment.CenterVertically
@@ -286,7 +293,9 @@ fun ComposeContextProfilePickerView(
LazyColumnWithScrollBarNoAppBar(
Modifier
.heightIn(max = MAX_USER_PICKER_HEIGHT)
.background(MaterialTheme.colors.surface),
.background(MaterialTheme.colors.surface)
// The rows are already unclickable while busy; this is the only thing that says so.
.alpha(if (busy) 0.6f else 1f),
reverseLayout = true,
maxHeight = remember { mutableStateOf(MAX_USER_PICKER_HEIGHT) },
containerAlignment = Alignment.BottomEnd
@@ -340,7 +349,8 @@ fun ComposeContextProfilePickerView(
fun CurrentSelection() {
Column(
Modifier
.background(MaterialTheme.colors.surface),
.background(MaterialTheme.colors.surface)
.alpha(if (busy) 0.6f else 1f),
) {
Text(
generalGetString(MR.strings.context_user_picker_your_profile),
@@ -107,7 +107,7 @@ private fun ShareListToolbar(chatModel: ChatModel, stopped: Boolean, onSearchVal
.filter { u -> !u.user.activeUser && !u.user.hidden }
.all { u -> u.unreadCount == 0 }
UserProfileButton(chatModel.currentUser.value?.profile?.image, allRead) {
ModalManager.start.showCustomModal(keyboardCoversBar = false) { close ->
ModalManager.start.showCustomModal(keyboardCoversBar = false, id = ModalViewId.ACTIVE_PROFILE_PICKER) { close ->
val search = rememberSaveable { mutableStateOf("") }
ModalView(
{ close() },
@@ -92,7 +92,8 @@ class ModalData(val keyboardCoversBar: Boolean = true) {
enum class ModalViewId {
SECONDARY_CHAT,
CONTEXT_USER_PICKER_INCOGNITO,
CONTEXT_USER_PICKER_NEW_PROFILE
CONTEXT_USER_PICKER_NEW_PROFILE,
ACTIVE_PROFILE_PICKER
}
class ModalManager(private val placement: ModalPlacement? = null) {
@@ -125,14 +126,23 @@ class ModalManager(private val placement: ModalPlacement? = null) {
*
* Separate from [isLastModalOpen] rather than folded into it, because that one is also
* used to decide when to tear down secondary chats, where the existing behaviour is
* relied on. Indexed access rather than iteration, so it stays safe if a caller ever
* reaches it off the main thread while these lists are being mutated. */
* relied on. */
fun isLastModalOpenNotClosing(id: ModalViewId): Boolean {
var i = modalViews.size - 1
while (i >= 0 && i in toRemove) i--
return i >= 0 && modalViews.getOrNull(i)?.id == id
}
/** [hasModalOpen] with the same exclusion as [isLastModalOpenNotClosing]: a modal
* dismissed but still animating out does not count. Without it, re-opening something
* straight after closing it is a silent no-op for the length of the animation. */
fun hasModalOpenNotClosing(id: ModalViewId): Boolean {
for (i in modalViews.indices) {
if (i !in toRemove && modalViews.getOrNull(i)?.id == id) return true
}
return false
}
fun showModal(settings: Boolean = false, showClose: Boolean = true, id: ModalViewId? = null, forceAnimated: Boolean = false, cardScreen: Boolean = false, endButtons: @Composable RowScope.() -> Unit = {}, content: @Composable ModalData.() -> Unit) {
showCustomModal(id = id, forceAnimated = forceAnimated) { close ->
ModalView(close, showClose = showClose, cardScreen = cardScreen, endButtons = endButtons, content = { content() })
@@ -302,7 +302,7 @@ fun ActiveProfilePicker(
// Intentionally don't use derivedStateOf in order to NOT change an order after user was selected.
// Keyed on the profile count as well so a profile created from this picker appears in it
// without relying on the composition being torn down and rebuilt by the form on top.
val filteredProfiles = remember(searchTextOrPassword.value, chatModel.users.size) {
val filteredProfiles = remember(searchTextOrPassword.value, chatModel.users.size, chatModel.currentUser.value?.userId) {
filteredProfiles(chatModel.users.map { it.user }.sortedBy { !it.activeUser }, searchTextOrPassword.value)
}
@@ -367,7 +367,12 @@ fun ActiveProfilePicker(
}
}
close()
// Only if this picker is still the top of its stack. The reassignment above can
// wait indefinitely on the retry alert, and back is not blocked while it does, so
// an unconditional close() here would pop whatever the user moved on to.
if (ModalManager.start.isLastModalOpenNotClosing(ModalViewId.ACTIVE_PROFILE_PICKER)) {
close()
}
} finally {
switchingProfile.value = false
}
@@ -580,7 +585,7 @@ private fun InviteView(rhId: Long?, connLinkInvitation: CreatedConnLink, contact
end = 16.dp
),
click = {
ModalManager.start.showCustomModal(keyboardCoversBar = false) { close ->
ModalManager.start.showCustomModal(keyboardCoversBar = false, id = ModalViewId.ACTIVE_PROFILE_PICKER) { close ->
val search = rememberSaveable { mutableStateOf("") }
ModalView(
{ close() },
+4 -3
View File
@@ -4005,9 +4005,10 @@ testCreateUserKeepingActiveUser = testChat2 aliceProfile bobProfile test
-- ... and the active user is unchanged, with no switch back needed
bob ##> "/u"
showActiveUser bob "bob (Bob)"
-- the new user's record is not active either, which is the field both clients
-- branch on to detect a core that ignored the flag; /u only covers the TVar.
-- (/users sorts by display name, so the order here says nothing about active_order.)
-- the new user's row is not active either - /u only covers the currentUser TVar.
-- Note this reads the stored row, not the CRActiveUser payload the clients branch
-- on, and /users sorts by display name, so it says nothing about active_order.
-- Neither of those is covered by any test here.
bob ##> "/users"
bob <## "bob (Bob) (active)"
bob <## "robert"