fix(ui): preserve last-message preview during channel-state reloads#2774
Conversation
|
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)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough
ChangesLast-message preview flicker fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: Ping-pong health check failed 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 |
…during_channel_state_reloads
| // (isUpToDate). When isUpToDate is false (e.g. Channel.query(idAround:) | ||
| // truncates state mid-load), the previous value keeps rendering instead | ||
| // of false rendering the empty state. | ||
| if (channelState.isUpToDate) { |
There was a problem hiding this comment.
Should we move this after we calculate latestLastMessage? and set the _currentLastMessage to latestLastMessage?
There was a problem hiding this comment.
There is an interesting edge-case which kinda prevents us doing this: While the _currentLastMessage will be properly populated in both cases, there is a subtle difference in the calculation of latestLastMessage, visible in the case where a channel is actually truncated (all messages removed).
If we move the statement after calculating the new latestLastMessage, the latestLastMessage will take into consideration the old _currentLastMessage. And for the truncation case, it means that it will wrongfully be calculated as final latestLastMessage = [message (null), _currentLastMessage (oldCachedMessage)].latest; - which would be wrong.
By keeping the if (channelState.isUpToDate) _currentLastMessage = message; before calculating latestLastMessage, it means that the new latest message will also be taken into consideration for the calculation: final latestLastMessage = [message (null), _currentLastMessage (null)].latest;
The concrete example where this logic fails:
-
If we assume a real channel truncation (
channel.truncatedevent) - (messages = [],isUpToDate=true,cache=msg1from the previous build):Order A — assign first, then snapshot:
if (channelState.isUpToDate) { _currentLastMessage = message; // cache = null } final latestLastMessage = [message, _currentLastMessage].latest; // = [null, null].latest = null → empty text shown ✓
Order B — snapshot first, then assign:
final latestLastMessage = [message, _currentLastMessage].latest; // = [null, msg1].latest = msg1 ← captured BEFORE the assignment if (channelState.isUpToDate) { _currentLastMessage = message; // cache = null, but latestLastMessage is already msg1 } // display msg1 → stale "hello" still shown ❌
But this whole discussion made me realise that we can make this whole thing a bit more readable:
final Message? latestLastMessage; if (channelState.isUpToDate) { latestLastMessage = message; _currentLastMessage = latestLastMessage; } else { latestLastMessage = [message, _currentLastMessage].latest; }
I will make this update!
…e_preview_during_channel_state_reloads' into bug/FLU-549_preserve_last_message_preview_during_channel_state_reloads
…during_channel_state_reloads
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/test/src/scroll_view/channel_scroll_view/stream_channel_list_item_test.dart`:
- Line 11: The import statement for mocks.dart at the top of
stream_channel_list_item_test.dart uses a relative import path
(../../mocks.dart) which violates the repo's linting rule requiring package
imports. Replace the relative import with a package import by converting it to
use the package:stream_chat_flutter syntax, pointing to the mocks.dart file
location relative to the package root rather than using relative path traversal.
🪄 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: 0451c4f0-180b-4c98-b7cd-a7068bf7dcf2
📒 Files selected for processing (3)
packages/stream_chat_flutter/CHANGELOG.mdpackages/stream_chat_flutter/lib/src/scroll_view/channel_scroll_view/stream_channel_list_item.dartpackages/stream_chat_flutter/test/src/scroll_view/channel_scroll_view/stream_channel_list_item_test.dart
| import 'package:mocktail/mocktail.dart'; | ||
| import 'package:stream_chat_flutter/stream_chat_flutter.dart'; | ||
|
|
||
| import '../../mocks.dart'; |
There was a problem hiding this comment.
Use a package import instead of a relative import.
import '../../mocks.dart'; violates the Dart import rule configured for this repo; switch it to a package: import so linting stays consistent across the package.
As per coding guidelines, **/*.dart: “Always use package imports instead of relative imports (always_use_package_imports)”.
🤖 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/scroll_view/channel_scroll_view/stream_channel_list_item_test.dart`
at line 11, The import statement for mocks.dart at the top of
stream_channel_list_item_test.dart uses a relative import path
(../../mocks.dart) which violates the repo's linting rule requiring package
imports. Replace the relative import with a package import by converting it to
use the package:stream_chat_flutter syntax, pointing to the mocks.dart file
location relative to the package root rather than using relative path traversal.
Source: Coding guidelines
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2774 +/- ##
==========================================
+ Coverage 69.59% 69.77% +0.17%
==========================================
Files 426 426
Lines 25676 25680 +4
==========================================
+ Hits 17869 17917 +48
+ Misses 7807 7763 -44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…during_channel_state_reloads
…during_channel_state_reloads
…e_preview_during_channel_state_reloads' into bug/FLU-549_preserve_last_message_preview_during_channel_state_reloads
VelikovPetar
deleted the
bug/FLU-549_preserve_last_message_preview_during_channel_state_reloads
branch
2 participants
Submit a pull request
Linear: FLU-547
CLA
Description of the pull request
Summary
When the user opens a channel that has unread messages, the corresponding channel-list cell briefly renders the empty-state text ("No messages yet") before snapping back to the real last-message preview. This PR fixes the flicker and adds regression tests for the underlying state-emission scenarios.
The bug
StreamChannel._maybeInitChannelcallsloadChannelAtMessage(lastReadMessageId)→_queryAtMessage→Channel.query(messagesPagination: PaginationParams(idAround: …))when entering a channel with unreads (packages/stream_chat_flutter_core/lib/src/stream_channel.dart).Inside
Channel.query(packages/stream_chat/lib/src/client/channel.dart), theidAroundpath runsstate.truncate()(which clearsmessagesto[]and emits a newChannelState) immediately followed byupdateChannelState(...)(which emits the populated state). Both emissions go through_channelStateController(aBehaviorSubject) and reach listeners in separate microtasks — so every subscriber tomessagesStreamsees an empty list, then the populated list.The channel-list cell's preview widget (
_ChannelLastMessageWithStatusinstream_channel_list_item.dart, plus its public twinChannelLastMessageText) was relying on aMessage? _currentLastMessagecache field to ride out exactly this kind of transient empty emission via a.latestselector. The cache field was declared but never assigned, so the absorber was a no-op and the empty emission produced the visible flash.The fix
Assign
_currentLastMessage = message(includingnull) on every build, gated onchannelState.isUpToDate. This is done before computing.latest, so the freshly-stored value participates in the selector.The gate matters:
_queryAtMessageflipsisUpToDate = falsebefore truncating, so the transient empty list is skipped by the cache. A realchannel.truncatedevent (_listenChannelTruncated) clears messages while leavingisUpToDate = true, so the cache correctly clears tonulland the empty-state surfaces.Tests
New file:
packages/stream_chat_flutter/test/src/scroll_view/channel_scroll_view/stream_channel_list_item_test.dart. Three scenarios:Manual verification
UI Changes
flash-before.mp4
flash-after.mp4
Summary by CodeRabbit