From 9be3e01d8b4ada21f1a7e6c6fc40b564bf0be488 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Fri, 2 Oct 2026 20:51:26 +0200 Subject: [PATCH 1/2] fix(subgroups): hand an adoption's outcome back on the main thread "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 --- .../composable/SelectAdoptingGroupScreen.kt | 33 ++++++++++--------- .../ui/view/model/ChatRoomDetailViewModel.kt | 11 +++++-- .../model/SelectAdoptingGroupViewModel.kt | 27 +++++++++++++-- 3 files changed, 51 insertions(+), 20 deletions(-) diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/SelectAdoptingGroupScreen.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/SelectAdoptingGroupScreen.kt index 16313ef2..76fe9e43 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/SelectAdoptingGroupScreen.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/SelectAdoptingGroupScreen.kt @@ -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) } } ) { diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomDetailViewModel.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomDetailViewModel.kt index 9d7b7ade..6b7c16f3 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomDetailViewModel.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/ChatRoomDetailViewModel.kt @@ -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) } } } diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/SelectAdoptingGroupViewModel.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/SelectAdoptingGroupViewModel.kt index a46ef417..d63d1fff 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/SelectAdoptingGroupViewModel.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/view/model/SelectAdoptingGroupViewModel.kt @@ -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" From f46a7c627d1db8c9b2d2ca98a2963a77dd35c9ee Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Fri, 2 Oct 2026 21:22:29 +0200 Subject: [PATCH 2/2] test(subgroups): stop a ceremony-queue test throwing into another test's name `./gradlew :composeApp:jvmTest` failed `NavigationRoutingTest > an account already on the device does not stay on the loading screen` with `UncaughtExceptionsBeforeTest: There were uncaught exceptions before the test started`. **"Before the test started" is the tell.** `runTest` reports exceptions that reached the default handler before it began, so the name on the failure is whichever `runTest` ran next -- not the one that threw. `NavigationRoutingTest` passes alone and the subgroups work never touched it. The thrower is `CeremonyQueueRoutingJvmTest`, and the chain is five links with no branch in it: 1. `awaiting()` builds a `ChatMessageListViewModel` on the bare `NO_OP_NOSTR_REPOSITORY` and calls `initiate()`. 2. `initiate()`'s second line is `scheduleSynchronization()`. 3. That launches on `Dispatchers.IO` and asks `MemberProfileSync` who cannot be named. 4. The fixture's `room()` gives every `LocalParticipant` a `profile = null`, so the answer is never empty and the next line always runs. 5. `queueNegentropySynchronizeRequest` is one of the **forty-one** `TODO("Not yet implemented")` bodies on that no-op, out of forty-five members. It throws in a coroutine nobody awaits, four times a run -- once per test in the class. Fixed the way this file already fixes it for `DkgRepository`: an `object : X by NO_OP_X` with the one member overridden. The comment above it carries the chain, because the next person to see `UncaughtExceptionsBeforeTest` will be reading the name of the wrong test. **Verified under the condition that reproduced it.** It is order-dependent, so a filtered run is not evidence -- the culprit passes alone, and the pair passed too. The full suite under `--rerun-tasks` failed reproducibly before and is 982 of 982 after. **Not fixed here: the landmine itself.** Sixty `TODO()` bodies sit across four no-op repositories -- 41 of 45 on `NostrRepository`, 12 of 37 on `ChatRepository`, 6 of 7 on `MarmotRepository`, 1 of 1 on `SearchRepository` -- and seventeen jvmTest files build view models on the Nostr one alone, so this will recur; the reporter had already hit it through `getProfileWithPublicKey`. It is not a sweep, though: "no-op" means something different for each member, the return types vary from `Unit` to `Flow`, and a blanket empty body could make a test pass vacuously where it currently fails honestly. Sixty judgement calls about shared code do not belong in a commit about one flake, so it is flagged for its own change. 982 jvm tests, 0 failed, under `--rerun-tasks`. Co-Authored-By: Claude Opus 5 --- .../view/model/CeremonyQueueRoutingJvmTest.kt | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/view/model/CeremonyQueueRoutingJvmTest.kt b/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/view/model/CeremonyQueueRoutingJvmTest.kt index f6c876cb..faaec773 100644 --- a/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/view/model/CeremonyQueueRoutingJvmTest.kt +++ b/composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/view/model/CeremonyQueueRoutingJvmTest.kt @@ -13,6 +13,7 @@ import kotlinx.coroutines.withTimeout import kotlinx.coroutines.delay import press.mantra.compose.database.model.ChatRoom import press.mantra.compose.database.model.DkgSession +import press.mantra.compose.database.model.NegentropySynchronizeRequest import press.mantra.compose.database.model.Participant import press.mantra.compose.database.model.intermdiate.LocalChatRoom import press.mantra.compose.database.model.intermdiate.LocalParticipant @@ -151,7 +152,23 @@ class CeremonyQueueRoutingJvmTest { val viewModel = ChatMessageListViewModel( initialChatMessageListUIState = ChatMessageListUIState.Loading, localChatRoom = room(), - nostrRepository = NostrRepository.NO_OP_NOSTR_REPOSITORY, + // Not the bare no-op. `initiate` calls `scheduleSynchronization` on + // its second line, which launches on `Dispatchers.IO` and -- because + // `room()` gives every participant a null profile, so there is always + // somebody it cannot name -- reaches + // `queueNegentropySynchronizeRequest`. That member is one of the + // forty-one `TODO("Not yet implemented")` bodies on + // NO_OP_NOSTR_REPOSITORY, so it throws in a coroutine nobody awaits. + // + // Nothing catches it, so it reaches the default handler, and the next + // `runTest` in the same JVM reports it as `UncaughtExceptionsBeforeTest` + // -- against its own name. That is how this test failed + // `NavigationRoutingTest` four times a run while passing itself. + nostrRepository = object : NostrRepository by NostrRepository.NO_OP_NOSTR_REPOSITORY { + override suspend fun queueNegentropySynchronizeRequest( + negentropySynchronizeRequests: List + ) = Unit + }, chatRepository = ChatRepository.NO_OP_CHAT_REPOSITORY, frostSigningRepository = FrostSigningRepository.NO_OP_FROST_SIGNING_REPOSITORY, dkgRepository = object : DkgRepository by DkgRepository.NO_OP_DKG_REPOSITORY {