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

fix(chat): a tablet's room list can pick a second room, not just the first

Browse Source 
The home screen's detail pane answered with the first room it was ever given. Tap
another and the list highlighted nothing, the transcript did not change, and the app
bar kept the first room's title -- so from the expanded breakpoint up, which is a
tablet in landscape and every desktop window, the split was a one-shot: the first
conversation you opened was the only one you could read without leaving the screen.

**`key(chatRoomId)` does not rebuild a view model, and the comment claiming it did was
the bug.** `viewModel()` resolves against `LocalViewModelStoreOwner.current` under a
key derived from the *class* -- `ViewModelProvider.get(modelClass)`, no key, straight
to the default `DEFAULT_KEY:canonicalName`. A composition key replaces a group:
`remember`s are forgotten and `LaunchedEffect`s restart, which is real but is not where
a view model lives. The store belongs to `HomeRoute`'s `NavBackStackEntry` and does not
change while the reader is on the home screen, so the second tap re-ran the
composition, asked for a `ChatRoomMessagingViewModel`, and got back the one holding
room one -- its `chatRoomId` a constructor val, its factory never invoked a second
time. `initiateChatRoomDetail()` then re-read room one and the pane redrew what it
already had.

**This is a list-detail-only failure, which is why nothing caught it.** Every other
screen in the app reaches the chat transcript by `navController.navigate(...)`, and a
pushed route is its own `NavBackStackEntry` and so its own store -- one subject per
owner, the class key never collides. The pane is the one place one screen is composed
for a succession of subjects without the entry changing. Below expanded the home screen
still pushes, so a phone never had this; the four existing cases in
`ChatPaneLayoutJvmTest` assert the widths and the empty state, which were right.

**`ViewModelStoreScopedTo` gives the pane a store of its own rather than keying the two
`viewModel()` calls by room id.** Both put the right room on screen. Keying would also
leave every room visited holding a live view model in the home entry's store until the
reader left the screen, each with its relay subscriptions and database collectors still
running -- a leak that grows with how much a reader browses, which on this layout is
the point of the layout. A scope that clears on the way out is the same correctness and
none of that.

**It is hand-rolled because the stock API is not here.** `rememberViewModelStoreOwner`
lands in `androidx.lifecycle:lifecycle-viewmodel-compose` 2.11.0 and does exactly this,
including surviving an Android configuration change by consulting the parent's
lifecycle; the build resolves `org.jetbrains.androidx.lifecycle`, and the class is in
neither the 2.9.6 nor the 2.11.0 artifact in the cache. A bare `ViewModelStoreOwner` is
the whole contract a scope needs here -- nothing in this app puts a `SavedStateHandle`
in a view model, so there is no saved state to delegate, and it deliberately does not
implement `HasDefaultViewModelProviderFactory` because every call inside passes its own
factory. The cost of hand-rolling is that a rotation rebuilds the pane's view models
instead of keeping them; the selection itself is `rememberSaveable` and survives, so
what that costs is a re-read of one room.

**Both wrappers stay, and they are not redundant.** The store scope alone would leave
the composer's draft text in place as the reader moved to the next room -- which on this
screen is the failure the composer is built around three separate signals to prevent,
one person's words going to another -- and would not restart the effect that loads a
room, so the pane would sit on `Loading`. `key` alone was the bug. One resets the
composition, the other the view models.

**Tests: a fifth case in `ChatPaneLayoutJvmTest`, checked against the bug reinstated.**
Two rooms told apart by their subject, which both panes draw as a title, so two nodes
saying it means "this is the room on the right" and one means "this room is merely
listed" -- an assertion that needs no new test tag and no reach into the view model.
Tap room one, wait for two; tap room two, wait for two of *it* and one of room one.
With `ViewModelStoreScopedTo` reduced to `content()` the second wait fails with
`ComposeTimeoutException` while the other four cases still pass, so the case pins the
view model scope rather than the layout. The waits are generous, because what they wait
for is a database read on another dispatcher and the cost of being wrong about a thread
hop is a test that fails on a loaded machine.

**Not done.** Nobody has looked at this running on a tablet; the test proves the pane
follows the selection, which is not the same thing. The list pane also still draws no
selected state -- M3's list-detail expects the selected row to carry one, and its
absence is its own reason the pane read as unresponsive. Left out deliberately: it is a
separate change to a different file, and this one is the part that was broken.

974 jvm tests, 454 common tests, `m3Audit` meeting every budget.

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

15
composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/HomeScreen.kt
View File

@@ -80,6 +80,7 @@ import mantra.composeapp.generated.resources.messages_need_the_secret_key_this_p
import mantra.composeapp.generated.resources.sign_in_with_the_nsec
import press.mantra.compose.ui.composable.widgets.LocalCanSign
import press.mantra.compose.ui.composable.widgets.ScreenStateTransition
import press.mantra.compose.ui.composable.widgets.ViewModelStoreScopedTo
import press.mantra.compose.ui.composable.widgets.buttons.ExtendedFab
import press.mantra.compose.ui.theme.ConformancePreviews
@@ -311,10 +312,17 @@ fun HomeScreen(
message = stringResource(Res.string.pick_a_conversation_to_read_it_here)
)
} else {
// Keyed on the room, so switching rooms rebuilds the
// screen's view models rather than feeding a new id to
// ones already subscribed to another room's relay.
// Two things, and the pane needs both. `key` rebuilds
// the composition -- the composer's draft text does not
// follow you into the next room, and the effect that
// loads a room runs again. `ViewModelStoreScopedTo`
// rebuilds the view models, which `key` cannot: they
// hang off the store, and on this screen the store is
// `HomeRoute`'s and never changes, so without it the
// pane kept answering with the first room it was given
// however many were tapped after it.
key(chatRoomId) {
ViewModelStoreScopedTo(chatRoomId) {
ChatRoomMessagingScreen(
activeUserPublicKey = activeUserPublicKey,
chatRoomId = chatRoomId,
@@ -333,6 +341,7 @@ fun HomeScreen(
onNavigateBack = null,
)
}
}
}
}
}

51
composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/widgets/ViewModelStoreScopedTo.kt Normal file
View File

@@ -0,0 +1,51 @@
package press.mantra.compose.ui.composable.widgets
import androidx.compose.runtime.Composable
import androidx.compose.runtime.CompositionLocalProvider
import androidx.compose.runtime.DisposableEffect
import androidx.compose.runtime.remember
import androidx.lifecycle.ViewModelStore
import androidx.lifecycle.ViewModelStoreOwner
import androidx.lifecycle.viewmodel.compose.LocalViewModelStoreOwner
/**
* Gives [content] a `ViewModelStore` of its own, cleared whenever [scopeKey] changes.
*
* **`key(...)` does not do this, which is the whole reason this exists.** A composition
* key replaces a group -- `remember`s are forgotten, `LaunchedEffect`s restart -- but
* `viewModel()` is not composition state. It resolves against
* `LocalViewModelStoreOwner.current` under a key derived from the *class*, so two
* compositions of one screen under one owner get the same instance no matter how they are
* keyed, and the factory is never invoked for the second. Being inside `key(someId)` buys
* nothing: the store is the navigation entry's, and it outlives every group in it.
*
* That is only a problem where one screen is composed for a succession of subjects without
* the navigation entry changing, which in this app means a list-detail pane: the home
* screen stays on `HomeRoute` while the detail pane is asked for one chat room after
* another. A pushed route has no such problem, because each `NavBackStackEntry` is already
* a store of its own -- which is why this only wraps the pane.
*
* The store is cleared on the way out rather than left to the parent, so the room just
* left does not keep its relay subscriptions and database collectors running. That is the
* other half of the job: keying the `viewModel()` calls by room id instead would fix what
* is on screen and quietly accumulate a live view model per room visited.
*
* Nothing in this app puts a `SavedStateHandle` in a view model, so a bare
* `ViewModelStoreOwner` is the whole contract a scope needs here; it deliberately does not
* implement `HasDefaultViewModelProviderFactory`, because every call inside passes its own
* factory.
*/
@Composable
fun ViewModelStoreScopedTo(scopeKey: Any?, content: @Composable () -> Unit) {
val owner = remember(scopeKey) {
object : ViewModelStoreOwner {
override val viewModelStore = ViewModelStore()
}
}
DisposableEffect(owner) {
onDispose { owner.viewModelStore.clear() }
}
CompositionLocalProvider(LocalViewModelStoreOwner provides owner, content = content)
}

87
composeApp/src/jvmTest/kotlin/press/mantra/compose/ui/composable/ChatPaneLayoutJvmTest.kt
View File

@@ -3,12 +3,16 @@ package press.mantra.compose.ui.composable
import androidx.compose.ui.test.ExperimentalTestApi
import androidx.compose.ui.test.assertIsDisplayed
import androidx.compose.ui.test.assertWidthIsEqualTo
import androidx.compose.ui.test.onAllNodesWithText
import androidx.compose.ui.test.onNodeWithTag
import androidx.compose.ui.test.onNodeWithText
import androidx.compose.ui.test.performClick
import androidx.compose.ui.test.runDesktopComposeUiTest
import androidx.compose.ui.unit.dp
import com.vitorpamplona.quartz.nip01Core.core.HexKey
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.flowOf
import press.mantra.compose.database.model.ChatRoom
import press.mantra.compose.database.model.NostrEvent
import press.mantra.compose.database.model.Profile
import press.mantra.compose.database.model.intermdiate.LocalChatRoom
@@ -21,12 +25,14 @@ import press.mantra.compose.ui.composable.widgets.ProvideSnackbarHost
import press.mantra.compose.ui.theme.MantraTheme
import press.mantra.compose.ui.view.state.HomeScreenUIState
import com.vitorpamplona.quartz.nip10Notes.TextNoteEvent
import androidx.compose.ui.test.ComposeUiTest
import kotlin.test.Test
import kotlin.test.assertEquals
/**
* The chat list-detail split, measured in windows of the widths the phase names.
*
* Three claims, and none of them can be read off the source:
* Four claims, and none of them can be read off the source:
*
* - below expanded the home screen is unchanged. That is the promise that let the split
* land as one commit -- the chat room is a navigation destination reached from eleven
@@ -35,7 +41,10 @@ import kotlin.test.Test
* `calculatePaneScaffoldDirective` gives it, so a hand-built pair measures the same as
* a `ListDetailPaneScaffold` would;
* - the detail pane says what it is for while nothing is selected, rather than being an
* unexplained empty half of the window.
* unexplained empty half of the window;
* - and a second tap in the list changes what the detail pane shows. That one is not a
* layout claim at all -- it is about view model scope -- but it is only reachable
* through this layout, and it is the way the split visibly failed on a tablet.
*
* The repositories are the no-op ones with the two reads this screen makes delegated to a
* fixed answer. Kotlin's interface delegation makes that ten lines rather than a
@@ -47,6 +56,15 @@ class ChatPaneLayoutJvmTest {
private val publicKey = "de1a1e64d1c4e0d6bd97b0e73d4dfd0e1ec1cdd1e6d8d0e2b9a7f3c5d0e1a2b3"
private val emptyDetail = "Pick a conversation to read it here."
private val roomOne = "Room one"
private val roomTwo = "Room two"
/**
* Opening a room is a database read on another dispatcher, so the assertions wait
* rather than assume. Generous, because what it is waiting for is a thread hop and the
* cost of being wrong about it is a test that fails on a loaded machine.
*/
private val OpenTimeout = 5_000L
@Test
fun `a phone window is one pane, as it has always been`() = runDesktopComposeUiTest(400, 900) {
@@ -81,8 +99,32 @@ class ChatPaneLayoutJvmTest {
onNodeWithTag(ChatListPaneTag).assertWidthIsEqualTo(412.dp)
}
@Test
fun `tapping a second room in the list changes what the detail pane shows`() =
runDesktopComposeUiTest(1000, 900) {
setContent { Home(chatRepository = TwoRooms) }
// Each room's subject is drawn twice once it is the selection -- on its card in
// the list, and as the title of the detail pane's app bar -- so two nodes is
// "this is the room on the right" and one is "this room is merely listed".
onNodeWithText(roomOne).performClick()
waitUntil("room one opens", OpenTimeout) { nodesSaying(roomOne) == 2 }
onNodeWithText(roomTwo).performClick()
// Before the pane scoped its view models this timed out: `key(chatRoomId)` was
// rebuilding the composition around a `ChatRoomMessagingViewModel` that
// `viewModel()` kept resolving, by class, out of the home route's store -- so
// the pane answered with room one however many rooms were tapped after it.
waitUntil("room two replaces it", OpenTimeout) {
nodesSaying(roomTwo) == 2 && nodesSaying(roomOne) == 1
}
assertEquals(2, nodesSaying(roomTwo), "room two is listed and shown")
assertEquals(1, nodesSaying(roomOne), "room one is listed, not shown")
}
@androidx.compose.runtime.Composable
private fun Home() {
private fun Home(chatRepository: ChatRepository = NoRooms) {
MantraTheme {
// Required, and deliberately so: `LocalSnackbarHostState` throws rather than
// defaulting, because a default would make every `notify` on a screen that
@@ -95,7 +137,7 @@ class ChatPaneLayoutJvmTest {
onNavigateToDirectMessageDetail = {},
onNavigateToChatRoomCreation = {},
nostrRepository = FixedProfile,
chatRepository = NoRooms,
chatRepository = chatRepository,
frostSigningRepository = FrostSigningRepository.NO_OP_FROST_SIGNING_REPOSITORY,
dkgRepository = DkgRepository.NO_OP_DKG_REPOSITORY,
)
@@ -128,9 +170,36 @@ class ChatPaneLayoutJvmTest {
}
/** The no-op repository, plus the one read the room list makes of it. */
private val NoRooms = object : ChatRepository by ChatRepository.NO_OP_CHAT_REPOSITORY {
override suspend fun observeChatRoomListByPublicKey(
publicKey: String,
): Flow<List<LocalChatRoom>> = flowOf(emptyList())
}
private val NoRooms = rooms()
/** Two rooms, told apart by their subject, which is what both panes draw as a title. */
private val TwoRooms = rooms(room("1", roomOne), room("2", roomTwo))
/**
* The no-op repository, plus the two reads this pair of panes makes of it: the list
* observes every room, and the detail pane looks one up by id.
*/
private fun rooms(vararg rooms: LocalChatRoom) =
object : ChatRepository by ChatRepository.NO_OP_CHAT_REPOSITORY {
override suspend fun observeChatRoomListByPublicKey(
publicKey: String,
): Flow<List<LocalChatRoom>> = flowOf(rooms.toList())
override suspend fun getChatRoomByIdentifier(id: HexKey): LocalChatRoom? =
rooms.firstOrNull { it.chatRoom.id == id }
}
private fun room(id: String, subject: String) = LocalChatRoom(
chatRoom = ChatRoom(
id = id,
userPublicKey = publicKey,
subject = subject,
description = null,
initialGiftWrapPayloadId = null,
mlsGroupState = null,
),
)
private fun ComposeUiTest.nodesSaying(text: String) =
onAllNodesWithText(text).fetchSemanticsNodes().size
}