fix(ui): auto-scroll on optimistic local messages#2683
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughStreamMessageListView now subscribes to derived ChannelClientState streams that emit only when the message list bottom grows (count increases + bottom ID changes). didChangeDependencies wires the appropriate stream, preserving the in-progress-scroll guard and own-message vs other-message auto-scroll rules. Tests and a changelog entry are added. ChangesAuto-scroll stream-based detection
Sequence DiagramsequenceDiagram
participant View as StreamMessageListView
participant ChannelState as ChannelClientState
participant Stream as messagesStream/threadsStream
participant Scroll as ScrollController
View->>ChannelState: subscribe to newMessageStream/newThreadMessageStream
ChannelState->>Stream: new messages list arrives
Stream->>View: onData(updatedMessageList)
View->>View: compare list length & last message ID
alt List length increased AND last ID changed
alt Own message
View->>Scroll: auto-scroll to bottom
else Other message AND at bottom
View->>Scroll: auto-scroll to bottom
else Other message AND scrolled up
View->>View: skip scroll (FAB visible)
end
else No length change or ID unchanged
View->>View: skip scroll (edit/reaction/delete)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 (1)
packages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart (1)
118-118: 💤 Low valueMinor inconsistency:
.reversed.toList()differs from other test files.Other tests like
mark_read_test.dartusegenerateConversation(...)without reversing. While this doesn't break the tests (new message detection works regardless of initial ordering), consistency with the existing test patterns would improve maintainability.♻️ Suggested change for consistency
- final messages = generateConversation(20, users: [other]).reversed.toList(); + final messages = generateConversation(20, users: [other]);Apply similar changes to lines 140, 165, 194, and 217.
Also applies to: 140-140, 165-165, 194-194, 217-217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart` at line 118, The test creates the messages list by calling generateConversation(...).reversed.toList(), which is inconsistent with other tests; remove the .reversed.toList() so messages is assigned directly from generateConversation(20, users: [other]) (and do the same for the other occurrences where messages is built from generateConversation(...)). Locate the assignments to the messages variable that use generateConversation and strip the .reversed.toList() call (occurrences near the current ones using generateConversation(..., users: [other])) to match the pattern used in mark_read_test.dart.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart`:
- Line 118: The test creates the messages list by calling
generateConversation(...).reversed.toList(), which is inconsistent with other
tests; remove the .reversed.toList() so messages is assigned directly from
generateConversation(20, users: [other]) (and do the same for the other
occurrences where messages is built from generateConversation(...)). Locate the
assignments to the messages variable that use generateConversation and strip the
.reversed.toList() call (occurrences near the current ones using
generateConversation(..., users: [other])) to match the pattern used in
mark_read_test.dart.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fa72ed1-9719-48d3-87f8-9d653e24a440
📒 Files selected for processing (3)
packages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dartpackages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2683 +/- ##
==========================================
+ Coverage 65.33% 65.37% +0.04%
==========================================
Files 423 423
Lines 26648 26669 +21
==========================================
+ Hits 17410 17436 +26
+ Misses 9238 9233 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xsahil03x
force-pushed
the
fix/auto-scroll-on-local-message
branch
from
f27b413 to
0fd46bd
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dart (1)
441-462:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRebind the listener when
parentMessagechanges.Lines 441-462 choose the source stream from
widget.parentMessage?.id, but the subscription is only recreated whenStreamChannel.of(context)changes. If the sameStreamMessageListViewstate is rebuilt with a different thread parent on the same channel, auto-scroll will stay wired to the previous stream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dart` around lines 441 - 462, When rebinding the new-message subscription, also detect changes to widget.parentMessage (compare previousWidget.parentMessage?.id to widget.parentMessage?.id) and, if different, cancel the existing _messageNewListener and recreate it exactly like you do when streamChannel changes: compute newMessageStream from widget.parentMessage?.id (using streamChannel!.channel.state?.newThreadMessageStream(parentId) or .newMessageStream) and call newMessageStream?.listen(...) to assign _messageNewListener; ensure debouncedMarkRead/MarkThreadRead and _userReadListener handling remains unchanged.
🧹 Nitpick comments (1)
packages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart (1)
63-84: ⚡ Quick winAdd coverage for the new thread and re-seed branches.
This harness only renders
const StreamMessageListView()withisUpToDate = true, so the newnewThreadMessageStream(parentId)path and theisUpToDate == false -> truere-seed logic never run. A small test for thread mode and one for the historic-load transition would catch the easiest regressions in this PR.Also applies to: 113-239
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart` around lines 63 - 84, The test helper pumpMessageList currently always renders const StreamMessageListView with isUpToDate=true so newThreadMessageStream(parentId) and the re-seed path when channelClientState.isUpToDate transitions false->true are never exercised; add two tests that call pumpMessageList with (1) a thread mode setup: mock channelClientState.newThreadMessageStream(parentId) to return a stream of thread messages and render StreamMessageListView in thread mode (e.g., pass parentId or the API that enables thread view) to assert thread messages are shown, and (2) a historic-load transition: initially pumpMessageList with isUpToDate=false and some historic messages, then update the mock to isUpToDate=true and inject new messages (or call the widget update path) to verify the re-seed logic runs (e.g., by asserting the message list updates); use the existing pumpMessageList, channelClientState, and StreamMessageListView symbols to locate where to extend tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dart`:
- Around line 441-462: When rebinding the new-message subscription, also detect
changes to widget.parentMessage (compare previousWidget.parentMessage?.id to
widget.parentMessage?.id) and, if different, cancel the existing
_messageNewListener and recreate it exactly like you do when streamChannel
changes: compute newMessageStream from widget.parentMessage?.id (using
streamChannel!.channel.state?.newThreadMessageStream(parentId) or
.newMessageStream) and call newMessageStream?.listen(...) to assign
_messageNewListener; ensure debouncedMarkRead/MarkThreadRead and
_userReadListener handling remains unchanged.
---
Nitpick comments:
In
`@packages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart`:
- Around line 63-84: The test helper pumpMessageList currently always renders
const StreamMessageListView with isUpToDate=true so
newThreadMessageStream(parentId) and the re-seed path when
channelClientState.isUpToDate transitions false->true are never exercised; add
two tests that call pumpMessageList with (1) a thread mode setup: mock
channelClientState.newThreadMessageStream(parentId) to return a stream of thread
messages and render StreamMessageListView in thread mode (e.g., pass parentId or
the API that enables thread view) to assert thread messages are shown, and (2) a
historic-load transition: initially pumpMessageList with isUpToDate=false and
some historic messages, then update the mock to isUpToDate=true and inject new
messages (or call the widget update path) to verify the re-seed logic runs
(e.g., by asserting the message list updates); use the existing pumpMessageList,
channelClientState, and StreamMessageListView symbols to locate where to extend
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b1437629-0015-4f11-9f23-36fd9e66055b
📒 Files selected for processing (3)
packages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dartpackages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart
✅ Files skipped from review due to trivial changes (1)
- packages/stream_chat_flutter/CHANGELOG.md
xsahil03x
force-pushed
the
fix/auto-scroll-on-local-message
branch
from
0fd46bd to
66b53cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart (1)
113-239: ⚡ Quick winAdd one thread-mode auto-scroll test.
These tests only exercise the channel path via
messagesStream, but Line 457 switches tonewThreadMessageStream(parentId)whenparentMessageis set. A small case that mountsStreamMessageListView(parentMessage: ...)and drivesthreadsStreamwould cover the new branch directly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart` around lines 113 - 239, Add a new widget test that mounts StreamMessageListView with parentMessage set to exercise the thread branch: create a parent Message, call pumpMessageList(parentMessage: parent) (or the test helper that mounts StreamMessageListView with parentMessage), then drive the threads stream by delivering a new thread message (use deliverNewMessage with newMessage.user and existing list and ensure the helper routes to threadsStream/newThreadMessageStream), and assert auto-scroll behavior (e.g., find.text(newThread.text!) and FloatingActionButton presence/absence) so the code path that calls newThreadMessageStream(parentId) is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dart`:
- Around line 1530-1559: The newMessageStream currently ignores emissions when
the list length stays the same, but trimming old head items can remove earlier
messages while a new bottom message is appended so length doesn't grow; update
the logic in Stream<Message> get newMessageStream to treat a changed last
message as a new message even when emitted.length == lastLength by checking
newLast.id != lastSeen?.id and yielding newLast in that case (i.e. don't require
lengthGrew to be true to yield if lastChanged is true), and ensure lastSeen and
lastLength are updated after handling this condition; refer to the
newMessageStream getter, variables wasUpToDate, lastSeen, lastLength, emitted,
newLast, lengthGrew and lastChanged to locate and implement the change.
---
Nitpick comments:
In
`@packages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart`:
- Around line 113-239: Add a new widget test that mounts StreamMessageListView
with parentMessage set to exercise the thread branch: create a parent Message,
call pumpMessageList(parentMessage: parent) (or the test helper that mounts
StreamMessageListView with parentMessage), then drive the threads stream by
delivering a new thread message (use deliverNewMessage with newMessage.user and
existing list and ensure the helper routes to
threadsStream/newThreadMessageStream), and assert auto-scroll behavior (e.g.,
find.text(newThread.text!) and FloatingActionButton presence/absence) so the
code path that calls newThreadMessageStream(parentId) is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84c7790f-db4b-4a74-b923-d6c772596250
📒 Files selected for processing (3)
packages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dartpackages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart
✅ Files skipped from review due to trivial changes (1)
- packages/stream_chat_flutter/CHANGELOG.md
xsahil03x
force-pushed
the
fix/auto-scroll-on-local-message
branch
2 times, most recently
from
7b1dbbc to
68b0611
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dart (1)
437-457:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResubscribe when
parentMessagechanges.This listener is only rebuilt when the inherited
StreamChannelchanges. Rebuilding the sameStreamMessageListViewwith a differentparentMessagekeeps_messageNewListenerattached to the previous stream, so thread auto-scroll follows the wrong conversation until the widget is remounted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dart` around lines 437 - 457, The code only rebuilds _messageNewListener when the inherited StreamChannel changes, so changing widget.parentMessage leaves the listener on the old stream; update the logic that recreates the listener (where streamChannel, debouncedMarkRead/ThreadRead, _messageNewListener, _userReadListener, and _unreadState are reset) to also run when widget.parentMessage?.id changes (compare previous parent id to current), and then re-subscribe by creating the correct newMessageStream using the current widget.parentMessage (as done with switch on widget.parentMessage?.id) and assigning _messageNewListener to its listener so thread auto-scroll follows the current parent thread.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@packages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dart`:
- Around line 437-457: The code only rebuilds _messageNewListener when the
inherited StreamChannel changes, so changing widget.parentMessage leaves the
listener on the old stream; update the logic that recreates the listener (where
streamChannel, debouncedMarkRead/ThreadRead, _messageNewListener,
_userReadListener, and _unreadState are reset) to also run when
widget.parentMessage?.id changes (compare previous parent id to current), and
then re-subscribe by creating the correct newMessageStream using the current
widget.parentMessage (as done with switch on widget.parentMessage?.id) and
assigning _messageNewListener to its listener so thread auto-scroll follows the
current parent thread.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18805226-f41e-4856-9373-c04dee582a62
📒 Files selected for processing (3)
packages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/lib/src/message_list_view/message_list_view.dartpackages/stream_chat_flutter/test/src/message_list_view/auto_scroll_test.dart
✅ Files skipped from review due to trivial changes (1)
- packages/stream_chat_flutter/CHANGELOG.md
xsahil03x
force-pushed
the
fix/auto-scroll-on-local-message
branch
4 times, most recently
from
181c0b6 to
d442be8
Compare
|
Triage of CodeRabbit findings:
|
xsahil03x
force-pushed
the
fix/auto-scroll-on-local-message
branch
from
d442be8 to
77ea726
Compare
Subscribe to `channel.state.messagesStream` (or `threadsStream[parentId]` in thread mode) instead of `channel.on(EventType.messageNew)` so the list follows to the new bottom message the moment it lands in state. The event path only fires on server-confirmed messages, which meant the user's own send wasn't auto-scrolled until the server round-trip completed. The data-source-driven pattern matches what the Android, iOS, and React Native SDKs do. New-message detection uses a `lengthGrew && lastChanged` check between emissions; the bottom-most snapshot is seeded from current state on subscribe so we don't auto-scroll on the BehaviorSubject replay. The synchronous `controller.scrollTo(index: 0)` call still clears SPL's anchor key before `didUpdateWidget` (no race). Adds an `auto_scroll_test.dart` covering: other-user-at-bottom, other-user-scrolled-up, own-message-scrolled-up, optimistic local send, and rapid burst. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xsahil03x
force-pushed
the
fix/auto-scroll-on-local-message
branch
from
77ea726 to
76ad959
Compare
1 participant
Closes FLU-499 (QA item #97 from Flutter QA — New Design).
Summary
StreamMessageListView's auto-scroll-to-bottom listener now subscribes tochannel.state.newMessageStream(ornewThreadMessageStream(parentId)in thread mode), a private extension that diffschannel.state.messagesStreamfor "a new message landed at the bottom" — covering both server-confirmedmessage.newevents AND optimistic local sends. The previouschannel.on(EventType.messageNew)path missed local sends because no WS event fires for them.isUpToDate(matching Android'sareNewestMessagesLoadedand iOS'sisFirstPageLoaded): no auto-scroll while the channel is loaded around a historic message. Thefalse → trueflip is a wholesale replacement and is also suppressed.(newLast.id != lastSeen.id) && newLast.createdAt.isAfter(lastSeen.createdAt)— handles retention pruning + append in the same emission, deletion of the tail, optimistic→server confirm (same id, no double-scroll), and failed→retry.widget.parentMessagechanges viadidUpdateWidget(thread parent switch on the same channel).Test plan
flutter test test/src/message_list_view/auto_scroll_test.dart— 7/7 pass (new file).flutter test test/src/message_list_view/— full MLV suite passes.flutter analyze lib/src/message_list_view/— clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation