From cc98f3ba89038fa04affbe7d20815ce1e5e386ad Mon Sep 17 00:00:00 2001 From: m57 <41645758+Hy4ri@users.noreply.github.com> Date: Mon, 28 Sep 2026 15:59:29 +0300 Subject: [PATCH] fix(chat): serialize scroll follow and timeline jumps (#1322, #1323) (#1359) --- .../chat/fullbleed/FullBleedChatListTest.kt | 36 +++++++-- .../m57/hermescontrol/ui/chat/ChatScreen.kt | 3 +- .../chat/components/ChatScrollController.kt | 79 +++++++------------ .../ui/chat/fullbleed/FullBleedChatList.kt | 3 + .../components/ChatScrollControllerTest.kt | 79 +++++++++++++++++++ 5 files changed, 142 insertions(+), 58 deletions(-) diff --git a/app/src/androidTest/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedChatListTest.kt b/app/src/androidTest/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedChatListTest.kt index a40a1c10..cf323c1e 100644 --- a/app/src/androidTest/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedChatListTest.kt +++ b/app/src/androidTest/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedChatListTest.kt @@ -3,6 +3,8 @@ package com.m57.hermescontrol.ui.chat.fullbleed import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.width import androidx.compose.foundation.lazy.LazyListState +import androidx.compose.runtime.SideEffect +import androidx.compose.runtime.remember import androidx.compose.ui.Modifier import androidx.compose.ui.platform.testTag import androidx.compose.ui.test.assertCountEquals @@ -12,6 +14,9 @@ import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.test.onAllNodesWithTag import androidx.compose.ui.test.onNodeWithTag import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.onRoot +import androidx.compose.ui.test.performTouchInput +import androidx.compose.ui.test.swipeDown import androidx.compose.ui.unit.dp import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.filters.MediumTest @@ -25,6 +30,7 @@ import com.m57.hermescontrol.ui.chat.ToolStatus import com.m57.hermescontrol.ui.chat.components.ChatScrollController import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import org.junit.Assert.assertFalse import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -72,9 +78,18 @@ class FullBleedChatListTest { showUserMessageTokens: Boolean = true, showAssistantMessageTokens: Boolean = true, showTokensPerSecond: Boolean = true, + onController: (ChatScrollController) -> Unit = {}, ) { composeTestRule.setContent { - val listState = LazyListState() + val listState = remember { LazyListState() } + val scrollController = + remember(listState) { + ChatScrollController( + listState = listState, + scope = CoroutineScope(Dispatchers.Main.immediate), + ) + } + SideEffect { onController(scrollController) } FullBleedChatList( transcript = TranscriptUiState.resolve( @@ -106,15 +121,24 @@ class FullBleedChatListTest { actions = testTranscriptActions(), searchState = ChatSearchState(), listState = listState, - scrollController = - ChatScrollController( - listState = listState, - scope = CoroutineScope(Dispatchers.Main.immediate), - ), + scrollController = scrollController, ) } } + @Test + fun upwardUserGesture_pausesBottomFollow() { + lateinit var scrollController: ChatScrollController + render( + messages = List(40) { index -> msg("u$index", MessageRole.USER) }, + onController = { scrollController = it }, + ) + composeTestRule.runOnIdle { scrollController.jumpToBottom() } + composeTestRule.waitForIdle() + composeTestRule.onRoot().performTouchInput { swipeDown() } + composeTestRule.runOnIdle { assertFalse(scrollController.isFollowingBottom) } + } + @Test fun userMessages_keepBubble_agentProse_isFullBleed() { render( diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatScreen.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatScreen.kt index 6f5baa49..dcd8fbcf 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatScreen.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/ChatScreen.kt @@ -329,8 +329,7 @@ fun ChatScreen( } LaunchedEffect(timelineState.historyAnchorRowId) { if (timelineState.isHistorical && displayedMessages.isNotEmpty()) { - scrollController.pauseFollowing() - listState.scrollToItem(0) + scrollController.jumpToHistoryStart() } } val showScrollToBottom by remember(timelineState.isHistorical, displayedMessages) { diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/components/ChatScrollController.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/components/ChatScrollController.kt index 60a6c506..1875ad62 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/components/ChatScrollController.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/components/ChatScrollController.kt @@ -10,10 +10,9 @@ import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import androidx.compose.runtime.snapshotFlow import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch -import kotlinx.coroutines.withTimeoutOrNull import kotlinx.coroutines.yield /** @@ -60,14 +59,8 @@ class ChatScrollController( private var lastTailKey: Any? = null private var lastMessageCount: Int = 0 - /** - * Guard flag: true while a programmatic scroll-to-bottom is in flight. - * Prevents [observeUserScrollPosition] from flipping [isFollowingBottom] - * to false during the layout race — the scroll targets the old last item, - * Compose lays out the new item below, and [isAtBottom] momentarily - * returns false before the second scroll pass catches it. - */ - private var isProgrammaticScroll by mutableStateOf(false) + /** The sole active scroll command. Replaced by newer tail, search or explicit jumps. */ + private var scrollJob: Job? = null /** * Pixel tolerance for "at bottom" detection. Covers the LazyColumn's @@ -77,35 +70,31 @@ class ChatScrollController( */ var bottomPixelTolerance by mutableStateOf(48) - /** Serialized scroll scope — all scroll jobs funnel through here. */ - fun launchScroll(block: suspend CoroutineScope.() -> Unit) { - scope.launch(block = block) + /** Replace the previous scroll command; user gestures can cancel the same job. */ + private fun launchScroll(block: suspend CoroutineScope.() -> Unit) { + scrollJob?.cancel() + scrollJob = scope.launch(block = block) } - /** - * Read the live bottom-follow state from [LazyListState] and react to - * user-driven scroll position changes. Should be called inside a - * `LaunchedEffect(Unit)` so it observes the list for the screen's lifetime. - */ + /** Observe only arrival at the bottom; departures are owned by actual user input. */ fun observeUserScrollPosition() { scope.launch { + var previouslyAtBottom = listState.isAtBottom(bottomPixelTolerance) snapshotFlow { listState.isAtBottom(bottomPixelTolerance) } .distinctUntilChanged() .collect { atBottom -> - if (atBottom) { + if (atBottom && !previouslyAtBottom) { isFollowingBottom = true - // Reaching the bottom means the reader caught up. pendingCount = 0 - } else if (!isProgrammaticScroll) { - // Only break follow on USER-initiated scrolls. - // During programmatic scrolls the viewport is - // transiently not-at-bottom while new items lay out. - isFollowingBottom = false } + previouslyAtBottom = atBottom } } } + /** An upward user gesture takes ownership from auto-follow immediately. */ + fun onUserScrollUp() = pauseFollowing() + /** * Call on every tail-content change (new message, streaming token, thinking * toggle, subagent card, clarify prompt). [tailKey] must be a stable value @@ -132,14 +121,7 @@ class ChatScrollController( // Pin instantly so rapid streamed tokens don't queue competing // animations; the reader stays glued to the growing tail. // Use layout-aware scrolling to survive the Compose layout race. - scope.launch { - isProgrammaticScroll = true - try { - scrollToBottomAwaitingLayout() - } finally { - isProgrammaticScroll = false - } - } + launchScroll { scrollToBottomAwaitingLayout() } } else { pendingCount += newMessages } @@ -163,14 +145,10 @@ class ChatScrollController( yield() yield() listState.scrollToBottom(animated = animated) - // If we're still not at the bottom after the first scroll (e.g. a - // new item was laid out between the scroll and now), do one more pass. + // Yield once more if layout changed during the first pass, then align + // the current last row; newer tails replace this job instead of racing it. if (!listState.isAtBottom(bottomPixelTolerance)) { - // Wait up to 150ms for the layout to settle with the new items. - withTimeoutOrNull(150L) { - snapshotFlow { listState.layoutInfo.totalItemsCount } - .first { it > 0 } - } + yield() listState.scrollToBottom(animated = animated) } } @@ -179,19 +157,19 @@ class ChatScrollController( fun jumpToBottom(animated: Boolean = false) { pendingCount = 0 isFollowingBottom = true - scope.launch { - isProgrammaticScroll = true - try { - scrollToBottomAwaitingLayout(animated = animated) - } finally { - isProgrammaticScroll = false - } - } + launchScroll { scrollToBottomAwaitingLayout(animated = animated) } } /** An explicit history gesture must not be undone by a short list's bottom-follow. */ fun pauseFollowing() { isFollowingBottom = false + scrollJob?.cancel() + } + + /** Historical timeline navigation shares the scroll command with follow and search. */ + fun jumpToHistoryStart() { + pauseFollowing() + launchScroll { listState.scrollToItem(0) } } /** FAB tap: resume following + clear unread. */ @@ -218,10 +196,11 @@ class ChatScrollController( contentOffset: Int, contentLength: Int, ) { - scope.launch { + pauseFollowing() + launchScroll { listState.animateScrollToItem(index) val info = listState.layoutInfo - val item = info.visibleItemsInfo.firstOrNull { it.index == index } ?: return@launch + val item = info.visibleItemsInfo.firstOrNull { it.index == index } ?: return@launchScroll val viewportHeight = info.viewportEndOffset - info.viewportStartOffset val fraction = if (contentLength > 0) contentOffset.toFloat() / contentLength else 0f val targetTop = (viewportHeight / 3) - (item.size * fraction).toInt() diff --git a/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedChatList.kt b/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedChatList.kt index 998facc2..c49c0eae 100644 --- a/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedChatList.kt +++ b/app/src/main/java/com/m57/hermescontrol/ui/chat/fullbleed/FullBleedChatList.kt @@ -237,6 +237,9 @@ fun FullBleedChatList( available: Offset, source: NestedScrollSource, ): Offset { + if (source == NestedScrollSource.UserInput && available.y > 0f) { + scrollController.onUserScrollUp() + } if (prefetch.onScroll( firstVisibleIndex = listState.firstVisibleItemIndex, deltaY = available.y, diff --git a/app/src/test/java/com/m57/hermescontrol/ui/chat/components/ChatScrollControllerTest.kt b/app/src/test/java/com/m57/hermescontrol/ui/chat/components/ChatScrollControllerTest.kt index 60e888d5..0f675376 100644 --- a/app/src/test/java/com/m57/hermescontrol/ui/chat/components/ChatScrollControllerTest.kt +++ b/app/src/test/java/com/m57/hermescontrol/ui/chat/components/ChatScrollControllerTest.kt @@ -3,16 +3,22 @@ package com.m57.hermescontrol.ui.chat.components import androidx.compose.foundation.lazy.LazyListItemInfo import androidx.compose.foundation.lazy.LazyListLayoutInfo import androidx.compose.foundation.lazy.LazyListState +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.setValue +import androidx.compose.runtime.snapshots.Snapshot import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every import io.mockk.mockk import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.delay import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import kotlinx.coroutines.test.setMain import org.junit.After @@ -151,6 +157,79 @@ class ChatScrollControllerTest { coVerify(exactly = 0) { listState.animateScrollToItem(any(), any()) } } + @Test + fun `layout growth does not pause follow but returning to bottom after user gesture resumes`() = + runTest(testDispatcher) { + var atBottom by mutableStateOf(true) + val listState = mockLazyListStateAtBottom() + val layoutInfo = listState.layoutInfo + val itemInfo = layoutInfo.visibleItemsInfo.single() + every { itemInfo.index } answers { if (atBottom) 4 else 3 } + val controller = ChatScrollController(listState, backgroundScope) + + controller.observeUserScrollPosition() + runCurrent() + atBottom = false // A new row laid out below the viewport, not a user scroll. + Snapshot.sendApplyNotifications() + runCurrent() + assertTrue(controller.isFollowingBottom) + + controller.onUserScrollUp() + assertFalse(controller.isFollowingBottom) + atBottom = true + Snapshot.sendApplyNotifications() + runCurrent() + assertTrue(controller.isFollowingBottom) + } + + @Test + fun `user upward gesture cancels in-flight follow and counts new messages`() = + runTest(testDispatcher) { + val listState = mockLazyListStateAtBottom() + coEvery { listState.scrollToItem(4, any()) } coAnswers { delay(1_000) } + val controller = ChatScrollController(listState, this) + + controller.onTailChanged(tailKey = "first", messageCount = 1) + runCurrent() + controller.onUserScrollUp() + controller.onTailChanged(tailKey = "second", messageCount = 2) + advanceUntilIdle() + + assertFalse(controller.isFollowingBottom) + assertEquals(1, controller.pendingCount) + coVerify(exactly = 1) { listState.scrollToItem(4, any()) } + } + + @Test + fun `historical jump replaces an in-flight follow and leaves follow paused`() = + runTest(testDispatcher) { + val listState = mockLazyListStateAtBottom() + coEvery { listState.scrollToItem(4, any()) } coAnswers { delay(1_000) } + val controller = ChatScrollController(listState, this) + + controller.onTailChanged(tailKey = "tail", messageCount = 1) + runCurrent() + controller.jumpToHistoryStart() + advanceUntilIdle() + + assertFalse(controller.isFollowingBottom) + coVerify(exactly = 1) { listState.scrollToItem(0, any()) } + } + + @Test + fun `rapid tails replace older jobs rather than queueing scrolls`() = + runTest(testDispatcher) { + val listState = mockLazyListStateAtBottom() + val controller = ChatScrollController(listState, this) + + controller.onTailChanged(tailKey = "one", messageCount = 1) + controller.onTailChanged(tailKey = "two", messageCount = 2) + advanceUntilIdle() + + assertTrue(controller.isFollowingBottom) + coVerify(exactly = 1) { listState.scrollToItem(4, any()) } + } + @Test fun `showFab returns true only when paused and content is present`() { val listState = mockk(relaxed = true)