fix(llc, core, ui): isolate channel state from sub-route wraps and smooth thread-open scroll#2704
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoves Channel.getMessagesById side effects, adds StreamChannel.value for pre-initialized channels (skips positioning), makes getMessage search caches before fetching, prevents parent-channel reload when disposing thread views, and defers ScrollablePositionedList scroll decisions until item positions are available. ChangesChannel State & Message Initialization
Scroll Positioning Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 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 |
xsahil03x
force-pushed
the
fix/thread-state-portable
branch
2 times, most recently
from
e53b58a to
e84c8e9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_core/lib/src/stream_channel.dart`:
- Around line 66-74: Add a fail-fast assert in the StreamChannel.value
constructor to prevent passing an uninitialized channel: assert(channel.state !=
null, 'StreamChannel.value requires a channel with initialized state'). This
ensures callers can't set showLoading = false while giving a channel whose state
is null, avoiding silent returns of child and downstream null-state crashes;
update the StreamChannel.value constructor (referenced symbol:
StreamChannel.value and channel.state) to include this assert and keep current
default assignments.
🪄 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: 447f7bad-61d7-4a25-ae7e-96afb50a150b
📒 Files selected for processing (7)
packages/stream_chat/CHANGELOG.mdpackages/stream_chat/lib/src/client/channel.dartpackages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/lib/scrollable_positioned_list/src/scrollable_positioned_list.dartpackages/stream_chat_flutter_core/CHANGELOG.mdpackages/stream_chat_flutter_core/lib/src/message_list_core.dartpackages/stream_chat_flutter_core/lib/src/stream_channel.dart
xsahil03x
force-pushed
the
fix/thread-state-portable
branch
from
e84c8e9 to
628f661
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/stream_chat_flutter_core/lib/src/stream_channel.dart (1)
66-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider a fail-fast assert for an uninitialized channel.
Because
showLoadingis forced tofalse, passing a channel whosestateis stillnulllets the first build returnchildbefore_maybeInitChannel'swatch()completes, so descendants can hit a much less actionable null-state crash. The doc on Line 59-60 already states the channel must be initialized; an assert makes that contract enforceable at the callsite.Suggested guard
const StreamChannel.value({ super.key, required this.child, required this.channel, - }) : showLoading = false, + }) : assert( + channel.state != null, + 'StreamChannel.value requires an already-initialized channel.', + ), + showLoading = false, initialMessageId = null, errorBuilder = _defaultErrorBuilder, loadingBuilder = _defaultLoadingBuilder, _shouldPosition = false;🤖 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_core/lib/src/stream_channel.dart` around lines 66 - 74, Add a fail-fast assert to the StreamChannel.value constructor to ensure the provided channel is already initialized (channel.state != null) since showLoading is forced false; update the constructor (StreamChannel.value) to assert channel.state != null with a clear message so callers get an immediate error instead of downstream null crashes from _maybeInitChannel.watch().
🤖 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.
Duplicate comments:
In `@packages/stream_chat_flutter_core/lib/src/stream_channel.dart`:
- Around line 66-74: Add a fail-fast assert to the StreamChannel.value
constructor to ensure the provided channel is already initialized (channel.state
!= null) since showLoading is forced false; update the constructor
(StreamChannel.value) to assert channel.state != null with a clear message so
callers get an immediate error instead of downstream null crashes from
_maybeInitChannel.watch().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e12a87e-415d-4a36-aa26-ecdade2a14ab
📒 Files selected for processing (7)
packages/stream_chat/CHANGELOG.mdpackages/stream_chat/lib/src/client/channel.dartpackages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/lib/scrollable_positioned_list/src/scrollable_positioned_list.dartpackages/stream_chat_flutter_core/CHANGELOG.mdpackages/stream_chat_flutter_core/lib/src/message_list_core.dartpackages/stream_chat_flutter_core/lib/src/stream_channel.dart
✅ Files skipped from review due to trivial changes (3)
- packages/stream_chat/CHANGELOG.md
- packages/stream_chat_flutter/CHANGELOG.md
- packages/stream_chat_flutter_core/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/stream_chat/lib/src/client/channel.dart
- packages/stream_chat_flutter_core/lib/src/message_list_core.dart
- packages/stream_chat_flutter/lib/scrollable_positioned_list/src/scrollable_positioned_list.dart
xsahil03x
force-pushed
the
fix/thread-state-portable
branch
from
628f661 to
3a9b8d5
Compare
…ooth thread-open scroll A small cluster of related issues that surfaced together while investigating thread-page UX. All four are independent bugs touching the LLC, core, and the vendored SPL. - `Channel.getMessagesById` was merging fetched messages into `ChannelState.messages` as a side effect. Resolving a thread parent for an in-channel reply would silently inject the parent into the loaded channel window, typically at index 0. The fetch is now pure — callers that want to refresh the loaded window can pipe the response through `ChannelClientState.updateMessage`. - `StreamChannel.getMessage` previously hit the network for any message not in `channel.state.messages`, ignoring thread replies and pinned messages. Cache lookup now also scans `channel.state.threads.values` and `channel.state.pinnedMessages`. - Added `StreamChannel.value` constructor for wrapping an already-initialized channel purely to expose it via `StreamChannel.of` to a sub-route or overlay (thread page, channel info screen, long-press modal, attachment viewer). Skips the channel-page positioning the default constructor runs, which would otherwise overwrite the parent route's loaded window. - `MessageListCore` no longer reloads the parent channel from its dispose path when the disposing instance is in thread mode — a thread's lifecycle shouldn't be touching the channel's loaded window. - `MessageListCore` no longer refetches thread replies on first attach when `state.threads[parentId]` is already populated. WebSocket events keep that cache live; the redundant fetch caused a merge-rebuild flicker on warm cache. - `ScrollablePositionedList._startScroll` waits one frame for `itemPositions` to be published when it's empty. Previously a `scrollTo` requested from a post-frame callback right after mount would fall through to the dual-controller teleport path even for nearby targets, fake-scrolling two screens for what should have been a short animation. Matches the Compose `LazyListState` pattern of suspending until first composition before scrolling. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
xsahil03x
force-pushed
the
fix/thread-state-portable
branch
from
3a9b8d5 to
7946a52
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2704 +/- ##
==========================================
- Coverage 65.67% 65.64% -0.04%
==========================================
Files 423 423
Lines 26694 26708 +14
==========================================
+ Hits 17532 17533 +1
- Misses 9162 9175 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
VelikovPetar
approved these changes
Jun 2, 2026
Contributor
VelikovPetar
left a comment
VelikovPetar
left a comment
There was a problem hiding this comment.
Left a very small question, otherwise looks good!
…eeded Encapsulates the early-return conditions in one place so callers don't need to know which contexts skip the reload. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Small cluster of related fixes around channel-state isolation and thread-page UX.
Channel.getMessagesByIdis now a pure fetch — no longer merges the response intoChannelState.messages. Resolving a thread parent for an in-channel reply previously injected the parent into the loaded channel window (typically at index 0, since parents predate the loaded window).StreamChannel.getMessagecache lookup now also scansstate.threads.valuesandstate.pinnedMessagesbefore falling back to the network.StreamChannel.valueconstructor. Wraps a channel for context access without running channel-page positioning. Use for sub-route or overlay wraps (thread page, info screens, long-press modal, attachment viewer).MessageListCoreno longer reloads the parent channel from its dispose path when the disposing instance is in thread mode.ScrollablePositionedList._startScrollwaits one frame for layout whenitemPositionsis empty. Previously ascrollToinvoked right after mount took the dual-controller teleport path even for nearby targets, producing a multi-screen fake-scroll where a short animation was expected. Matches Compose'sLazyListState.animateScrollToItem(which suspends until first composition).Test plan
streamChannel.getMessage(threadReplyId)for a reply already loaded instate.threadsresolves from cache (no network call).Summary by CodeRabbit
New Features
Bug Fixes