mantra/mantra-kmp
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(subgroups): hand an adoption's outcome back on the main thread

Browse Source 
"Add a parent group", then tapping any group, crashed outright:

    IllegalStateException: Method setCurrentState must be called on the main thread
      at androidx.lifecycle.LifecycleRegistry.enforceMainThreadIfNeeded
      ...
      at NavController.popBackStack
      at SelectAdoptingGroupScreen.kt:153
      at SelectAdoptingGroupViewModel$offerTo$1.invokeSuspend

`offerTo` does its work in `viewModelScope.launch(Dispatchers.IO)` and called
`onDone` from inside it. The screen's callback called `onNavigateBack()` straight
through, which is `navController.popBackStack()` -- and navigation moves a back stack
entry's lifecycle, which is main-thread-only. Nothing in the chain switched.

**The dispatcher is the view model's business, not every caller's**, which is the
shape the app already uses in a dozen places and in both of the files this one was
modelled on: `SelectSubgroupAdminsViewModel.confirm` wraps its navigation in
`withContext(Dispatchers.Main)` and `AddArtifactViewModel` launches its `onSuccess`
there. A `finish` helper does it here, with the crash written above it so the next
person to add a callback to this class sees why. `ChatRoomDetailViewModel.acceptParent`
gets the same treatment: its only caller shows a snackbar, which happens to be
main-safe already, but a callback contract that is "safe if you are careful" is the
same bug waiting for a second caller.

**And the screen no longer navigates away on success**, which fixes a second thing
found on the way to the first. The snackbar was launched in
`rememberCoroutineScope()` -- this screen's own -- and then the screen popped, so the
message was racing its own scope's cancellation; `rememberNotifier` exists for exactly
that hazard and was not being used. Staying put is the better answer anyway, because
the list is the confirmation: the row just asked leaves it, since a certificate now
stands for the pair and `refuseAdoption` says so. The way out is the back button, which
a pushed screen has to have regardless. `offerTo` reloads before handing back so that
is true by the time the message appears.

The outer `scope.launch` around `offerTo` goes too -- it never suspended, and wrapping
a main-thread call in a launch was part of what made the threading hard to see.

No test covers this and none of the 1,416 would have. It is a dispatcher boundary
between a view model and a navigation callback, which is to say it lives in the one
layer this work had never run: compile-and-test verified, never driven through the app.
That was named as the risk when the feature landed, and this is it.

454 common tests, 962 jvm tests, `m3Audit` meets every budget.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Kgothatso Ngako
2026-10-02 20:51:26 +02:00
parent 7a28b227d9
commit 9be3e01d8b
3 changed files with 51 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

33
composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/SelectAdoptingGroupScreen.kt
View File

@@ -26,7 +26,6 @@ import androidx.compose.ui.text.font.FontFamily
import androidx.compose.ui.text.style.TextOverflow
import androidx.lifecycle.viewmodel.compose.viewModel
import com.vitorpamplona.quartz.nip01Core.core.HexKey
import kotlinx.coroutines.launch
import mantra.composeapp.generated.resources.Res
import mantra.composeapp.generated.resources.asked_that_group_to_adopt_this_one
import mantra.composeapp.generated.resources.no_group_here_can_adopt_this_one
@@ -40,6 +39,7 @@ import press.mantra.compose.ui.composable.widgets.ErrorState
import press.mantra.compose.ui.composable.widgets.LoadingDataIndicator
import press.mantra.compose.ui.composable.widgets.LocalSnackbarHostState
import press.mantra.compose.ui.composable.widgets.NavigateBackButton
import press.mantra.compose.ui.composable.widgets.rememberNotifier
import press.mantra.compose.ui.composable.widgets.ScreenStateTransition
import press.mantra.compose.ui.theme.readableContent
import press.mantra.compose.ui.theme.spacing
@@ -52,8 +52,15 @@ import press.mantra.compose.ui.view.state.SelectAdoptingGroupUIState
* Tapping a row asks that group's quorum to certify this room as its child. That
* is an **offer** and nothing more: this room becomes their subgroup only once
* its own quorum re-announces its key state naming them, which is a card back on
* the room's own screen. So this hands back rather than going anywhere new, and
* the snackbar says what was asked rather than what was done.
* the room's own screen. So the snackbar says what was *asked* rather than what
* was done.
*
* **It does not navigate away on success**, and that is the fix for two things at
* once. A message launched here while the screen was being popped was racing its
* own scope's cancellation; and the list is the better confirmation anyway -- the
* row that was just asked leaves it, because a certificate now stands for the
* pair and `refuseAdoption` says so. The way out is the back button, which a
* pushed screen has to have regardless.
*
* The list is the profile's own rooms -- see `SelectAdoptingGroupViewModel` for
* why it can be nothing else -- so empty is an ordinary answer and the empty
@@ -80,12 +87,10 @@ fun SelectAdoptingGroupScreen(
LaunchedEffect(chatRoomId) { viewModel.initiate() }
val snackbarHostState = LocalSnackbarHostState.current
val scope = rememberCoroutineScope()
// Read outside the handler: both of these are composable and an onClick
// lambda is not. The notifier takes the caller's scope because the message is
// shown as this screen navigates away, and one launched in the departing
// composable's scope would be cancelled with it.
// Both read outside the handler, because both are composable and an onClick
// lambda is not.
val notify = rememberNotifier(rememberCoroutineScope())
val asked = stringResource(Res.string.asked_that_group_to_adopt_this_one)
ScreenStateTransition(viewModel.uiState) { uiState ->
@@ -145,13 +150,11 @@ fun SelectAdoptingGroupScreen(
Card(
modifier = Modifier.fillMaxWidth(),
onClick = {
scope.launch {
viewModel.offerTo(candidate.chatRoom.id) { refusal ->
scope.launch {
snackbarHostState.showSnackbar(refusal ?: asked)
}
if (refusal == null) onNavigateBack()
}
// `offerTo` is not suspending and calls back
// on the main thread, so there is nothing to
// launch around it.
viewModel.offerTo(candidate.chatRoom.id) { message ->
notify(message ?: asked)
}
}
) {

11
composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomDetailViewModel.kt
View File

@@ -22,6 +22,7 @@ import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.IO
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import press.mantra.compose.database.model.types.MarmotReindexReport
class ChatRoomDetailViewModel(
@@ -118,7 +119,13 @@ class ChatRoomDetailViewModel(
if (!accepted) {
logger.e("Failed to ask $chatRoomId to accept $parentChatRoomId")
onDone("Couldn't ask this group to accept that parent. Please try again.")
// On the main thread, because a caller's callback may touch
// things an IO coroutine must not -- navigation most of all. See
// `SelectAdoptingGroupViewModel.finish`, where calling back from
// IO crashed outright.
withContext(Dispatchers.Main) {
onDone("Couldn't ask this group to accept that parent. Please try again.")
}
return@launch
}
@@ -126,7 +133,7 @@ class ChatRoomDetailViewModel(
// which is a quorum away -- so this reloads rather than crossing it
// off, and the card stays until the group has actually agreed.
initiateChatRoomDetail()
onDone(null)
withContext(Dispatchers.Main) { onDone(null) }
}
}

27
composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/SelectAdoptingGroupViewModel.kt
View File

@@ -14,6 +14,7 @@ import com.vitorpamplona.quartz.nip01Core.core.HexKey
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.IO
import kotlinx.coroutines.launch
import kotlinx.coroutines.withContext
import press.mantra.compose.repository.ChatRepository
import press.mantra.compose.ui.view.state.SelectAdoptingGroupUIState
@@ -94,7 +95,7 @@ class SelectAdoptingGroupViewModel(
if (refusal != null) {
isActionPending.value = false
onDone(refusal)
finish(refusal, onDone)
return@launch
}
@@ -108,14 +109,34 @@ class SelectAdoptingGroupViewModel(
if (!proposed) {
logger.e("Failed to ask $adoptingChatRoomId to adopt $chatRoomId")
onDone("Couldn't ask that group to adopt this one. Please try again.")
finish("Couldn't ask that group to adopt this one. Please try again.", onDone)
return@launch
}
onDone(null)
// Reload before handing back, so the row that was just asked leaves
// the list: a certificate now stands for the pair and
// `refuseAdoption` says so. That is the confirmation, and it is a
// better one than a message -- the list is the state.
initiate()
finish(null, onDone)
}
}
/**
* Hands [message] back **on the main thread**.
*
* The work runs on `Dispatchers.IO` and a caller's callback routinely touches
* things that may not be touched from there -- the first version of this
* screen called `popBackStack()` straight out of the IO coroutine and crashed
* on `Method setCurrentState must be called on the main thread`. The
* dispatcher is this function's business rather than every caller's, which is
* the shape `AddArtifactViewModel` and `SelectSubgroupAdminsViewModel` already
* use for the same handoff.
*/
private suspend fun finish(message: String?, onDone: (String?) -> Unit) {
withContext(Dispatchers.Main) { onDone(message) }
}
companion object {
private const val TAG = "SelectAdoptingGroupViewModel"