Fix auto scroll to bottom when exiting a thread#6165
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
WalkthroughThe change modifies how the scroll-to-bottom state is persisted by tying it to thread context via an additional condition in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/messages/list/Messages.kt (2)
313-318: Fix is correct — consider updating the accompanying comment to document the new keyThe
isMessageInThreadkey causesrememberSaveableto discard the saved state and re-runmutableStateOf(newMessageState)whenever the thread context changes. BecauselastScrollToBottomOnNewMessageis then equal to the currentnewMessageState, theLaunchedEffectat line 321 never sees a difference on the first evaluation, which is exactly what prevents the spurious scroll-to-bottom. The approach is sound for both the bug scenario and config-change resilience.The comment block above (lines 310–312) only mentions the config-change rationale. A one-liner noting why
isMessageInThreadis also a key input would help future maintainers:✏️ Suggested comment update
// Keep track of the last new message state that triggered a scroll to bottom. // If a configuration change happens, we want to keep the same state // and not scroll to bottom again if the newMessageState is the same as before the configuration change. + // The isMessageInThread key ensures the state is reset when switching between thread and main list contexts, + // preventing a stale comparison from triggering an unintended scroll after exiting a thread. var lastScrollToBottomOnNewMessage by rememberSaveable( isMessageInThread, saver = MutableStateNewMessageStateSaver, ) { mutableStateOf(newMessageState) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/messages/list/Messages.kt` around lines 313 - 318, Update the comment above the rememberSaveable block to explicitly note that including isMessageInThread as a key causes the saved state for lastScrollToBottomOnNewMessage (created via rememberSaveable and mutableStateOf(newMessageState)) to be discarded when thread context changes, ensuring the state is reset to newMessageState so the LaunchedEffect that compares it sees no spurious difference on first evaluation; reference the rememberSaveable call, the isMessageInThread key, lastScrollToBottomOnNewMessage, newMessageState, and the LaunchedEffect in the comment so future maintainers understand both the config-change and thread-context rationale.
280-355: Consider adding a functional test (and Paparazzi snapshot) for the thread-exit scroll-position preservationThe PR checklist notes tests are unchecked. Given this is a scroll-state bug, a lightweight instrumented or unit-composed test that verifies
lastScrollToBottomOnNewMessageis not stale after togglingisMessageInThreadwould guard against regressions. Based on learnings, Compose regressions in this module should also be covered by Paparazzi snapshots viaverifyPaparazziDebug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/messages/list/Messages.kt` around lines 280 - 355, Add a lightweight Compose unit/instrumented test plus a Paparazzi snapshot that verifies the scroll-position preservation logic in DefaultMessagesHelperContent when toggling thread state: create a test that renders DefaultMessagesHelperContent (or the Messages container that uses it), initialize messagesState so lastScrollToBottomOnNewMessage is set, toggle isMessageInThread by changing messagesState.parentMessageId (enter/exit thread) and then trigger a newMessageState change; assert that the component does not re-scroll when newMessageState equals the previous saved state (i.e., lastScrollToBottomOnNewMessage is preserved) and add a verifyPaparazziDebug snapshot to capture the UI before/after the toggle to prevent regressions; target helpers like shouldScrollToBottomOnNewMessage, lastScrollToBottomOnNewMessage, focusedItemIndex behavior, and the ScrollToBottomButton visibility in your test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/messages/list/Messages.kt`:
- Around line 313-318: Update the comment above the rememberSaveable block to
explicitly note that including isMessageInThread as a key causes the saved state
for lastScrollToBottomOnNewMessage (created via rememberSaveable and
mutableStateOf(newMessageState)) to be discarded when thread context changes,
ensuring the state is reset to newMessageState so the LaunchedEffect that
compares it sees no spurious difference on first evaluation; reference the
rememberSaveable call, the isMessageInThread key,
lastScrollToBottomOnNewMessage, newMessageState, and the LaunchedEffect in the
comment so future maintainers understand both the config-change and
thread-context rationale.
- Around line 280-355: Add a lightweight Compose unit/instrumented test plus a
Paparazzi snapshot that verifies the scroll-position preservation logic in
DefaultMessagesHelperContent when toggling thread state: create a test that
renders DefaultMessagesHelperContent (or the Messages container that uses it),
initialize messagesState so lastScrollToBottomOnNewMessage is set, toggle
isMessageInThread by changing messagesState.parentMessageId (enter/exit thread)
and then trigger a newMessageState change; assert that the component does not
re-scroll when newMessageState equals the previous saved state (i.e.,
lastScrollToBottomOnNewMessage is preserved) and add a verifyPaparazziDebug
snapshot to capture the UI before/after the toggle to prevent regressions;
target helpers like shouldScrollToBottomOnNewMessage,
lastScrollToBottomOnNewMessage, focusedItemIndex behavior, and the
ScrollToBottomButton visibility in your test.
|
aleksandar-apostolov
deleted the
bug/fix_auto_scroll_to_bottom_when_exiting_a_thread
branch
|
🚀 Available in v6.32.4 |
3 participants
Goal
Fix a bug where the message list would automatically scroll to the bottom when exiting a thread, losing the user's scroll position in the main message list.
Implementation
The issue was in
DefaultMessagesHelperContentwherelastScrollToBottomOnNewMessagestate was shared between thread and main list contexts. When exiting a thread:lastScrollToBottomOnNewMessagestill held the thread's last statenewMessageStatewas differentThe fix adds
isMessageInThreadas an input torememberSaveable. When the context switches (thread ↔ main list):newMessageStatelastScrollToBottomOnNewMessageequalsnewMessageState, the comparison fails🎨 UI Changes
thread-scroll-before.mp4
thread-scroll-after.mp4
Testing
Summary by CodeRabbit