-
Notifications
You must be signed in to change notification settings - Fork 371
fix(llc): improve persistence updates for channel state and threads #2401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds debounced persistence for channel state and channel threads, loads threads from offline persistence during channel initialization and retries failed messages, adds early-return guards in persistence client for empty inputs, updates tests to the new async persistence flow, and updates the changelog with a bugfix note. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Channel
participant Persistence as PersistenceClient
participant Storage as OfflineStorage
Note over App,Channel: Channel initialization with persisted threads (new flow)
App->>Channel: initialize(cid)
Channel->>Channel: seed in-memory ChannelState
Channel-->>Channel: debounce(triggerPersistChannelState)
Channel->>Persistence: getChannelThreads([cid])
Persistence->>Storage: read threads for cid
Storage-->>Persistence: threads
Persistence-->>Channel: threads
Channel-->>Channel: debounce(triggerPersistChannelThreads)
Channel->>Channel: retryFailedMessages()
App->>Channel: dispose()
Channel->>Channel: cancel debouncers
sequenceDiagram
autonumber
participant Channel
participant Persistence as PersistenceClient
participant Storage as OfflineStorage
Note over Channel: Debounced thread update path (on thread changes)
Channel->>Channel: onThreadsChanged(cid, threads)
Channel-->>Channel: debounce(updateChannelThreadsPersistence)
Channel->>Persistence: updateChannelThreads(cid, threads)
alt threads empty
Persistence-->>Channel: return early (no-op)
else threads non-empty
Persistence->>Storage: persist flattened messages
Storage-->>Persistence: ack
Persistence-->>Channel: done
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/stream_chat/CHANGELOG.md(1 hunks)packages/stream_chat/lib/src/client/channel.dart(5 hunks)packages/stream_chat/lib/src/db/chat_persistence_client.dart(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-08T14:27:59.621Z
Learnt from: xsahil03x
PR: GetStream/stream-chat-flutter#2348
File: packages/stream_chat_flutter_core/lib/src/stream_channel.dart:383-406
Timestamp: 2025-08-08T14:27:59.621Z
Learning: In stream_chat_flutter_core/lib/src/stream_channel.dart, threads (replies) do not support around-anchor loading. Thread replies are fetched as: initial latest page via StreamChannelState.getReplies(), and further pagination via StreamChannelState.queryReplies(parentId, direction: top|bottom). Anchored loads apply only to channel messages, not to threads.
Applied to files:
packages/stream_chat/lib/src/client/channel.dartpackages/stream_chat/lib/src/db/chat_persistence_client.dart
📚 Learning: 2025-09-25T08:19:01.469Z
Learnt from: xsahil03x
PR: GetStream/stream-chat-flutter#2394
File: packages/stream_chat_flutter/lib/src/message_action/message_actions_builder.dart:82-92
Timestamp: 2025-09-25T08:19:01.469Z
Learning: In the Stream Chat Flutter library, when deleting a message that hasn't been sent to the server yet (message.remoteCreatedAt == null) or is bounced with error, the _deleteMessage method in channel.dart automatically handles deletion locally via _deleteLocalMessage without making API calls, preventing 404 errors and deletingFailed states.
Applied to files:
packages/stream_chat/lib/src/client/channel.dartpackages/stream_chat/lib/src/db/chat_persistence_client.dart
📚 Learning: 2025-09-25T08:19:01.469Z
Learnt from: xsahil03x
PR: GetStream/stream-chat-flutter#2394
File: packages/stream_chat_flutter/lib/src/message_action/message_actions_builder.dart:82-92
Timestamp: 2025-09-25T08:19:01.469Z
Learning: In the Stream Chat Flutter library, when deleting a message with MessageSendingStatus.failed or MessageSendingStatus.failed_update status, the _deleteMessage method in channel.dart automatically handles deletion locally via _deleteLocalMessage without making API calls, preventing 404 errors and deletingFailed states.
Applied to files:
packages/stream_chat/lib/src/client/channel.dartpackages/stream_chat/lib/src/db/chat_persistence_client.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build (android)
- GitHub Check: build (ios)
- GitHub Check: analyze
- GitHub Check: format
- GitHub Check: test
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_persistence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
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/test/src/client/client_test.dart (1)
3655-3658: Fix persistence mock so the “same user” path doesn’t throwThe new guard in
StreamChatClient.connectUserchecks the user already associated withchatPersistenceClient. In this test we instantiate a freshMockPersistenceClient, but we never program it to report the user passed viaopenPersistenceConnection(user). As a result the guard believes a different user is attached and throws, matching the failure we’re seeing in CI (StreamChatError: 'Chat persistence client is already connected to a different user…'in thePersistenceConnectionTests connectUserstage).Please update the mock setup in this test to keep the connected-user metadata in sync with the
userused here (e.g. stub the getter the client relies on, or have theconnectUserstub capture the provided user ID). Once the mock mirrors the real persistence client’s behavior, this expectation should pass again.
🧹 Nitpick comments (1)
packages/stream_chat/test/src/client/client_test.dart (1)
11-13: Drop the strayabchelperThis top-level helper isn’t referenced anywhere, adds noise to the test suite, and can be safely removed.
-void abc(String abc) { - print(abc); -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/stream_chat/test/src/client/client_test.dart(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T14:27:59.621Z
Learnt from: xsahil03x
PR: GetStream/stream-chat-flutter#2348
File: packages/stream_chat_flutter_core/lib/src/stream_channel.dart:383-406
Timestamp: 2025-08-08T14:27:59.621Z
Learning: In stream_chat_flutter_core/lib/src/stream_channel.dart, threads (replies) do not support around-anchor loading. Thread replies are fetched as: initial latest page via StreamChannelState.getReplies(), and further pagination via StreamChannelState.queryReplies(parentId, direction: top|bottom). Anchored loads apply only to channel messages, not to threads.
Applied to files:
packages/stream_chat/test/src/client/client_test.dart
🪛 GitHub Actions: legacy_version_analyze
packages/stream_chat/test/src/client/client_test.dart
[error] 3669-3669: Test failed: PersistenceConnectionTests connectUser threw StreamChatError: 'Chat persistence client is already connected to a different user, please close the connection before connecting a new one.'
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: stream_chat_flutter_core
- GitHub Check: build (android)
- GitHub Check: build (ios)
- GitHub Check: stream_chat_localizations
- GitHub Check: analyze
- GitHub Check: test
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat_persistence
- GitHub Check: stream_chat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/stream_chat/test/src/client/client_test.dart (1)
668-669: Avoid 1 s sleeps in the tests.Bumping the wait to 1050 ms makes these specs noticeably slower and still leaves them at the mercy of scheduler jitter. Could we drive the debounced writer deterministically instead (e.g. inject a shorter debounce duration for tests or tick the clock with
fakeAsync) so we don’t depend on wall‑clock timing here?Also applies to: 749-749
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/stream_chat/test/src/client/client_test.dart(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T14:27:59.621Z
Learnt from: xsahil03x
PR: GetStream/stream-chat-flutter#2348
File: packages/stream_chat_flutter_core/lib/src/stream_channel.dart:383-406
Timestamp: 2025-08-08T14:27:59.621Z
Learning: In stream_chat_flutter_core/lib/src/stream_channel.dart, threads (replies) do not support around-anchor loading. Thread replies are fetched as: initial latest page via StreamChannelState.getReplies(), and further pagination via StreamChannelState.queryReplies(parentId, direction: top|bottom). Anchored loads apply only to channel messages, not to threads.
Applied to files:
packages/stream_chat/test/src/client/client_test.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: build (android)
- GitHub Check: analyze
- GitHub Check: build (ios)
- GitHub Check: test
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat_persistence
- GitHub Check: analyze_legacy_versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/stream_chat/test/src/client/client_test.dart (1)
668-669: Replace the hardcoded debounce wait withuntilCalledSleeping for 1.05 s just to let the debounced persistence flush makes the test slow and still timing‑sensitive.
mocktailalready exposesawait untilCalled(() => persistence.updateChannelState(any()));, so we can wait deterministically for the interaction (and likewise in the sister test) instead of burning a second and hoping the timer fired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/stream_chat/test/src/client/client_test.dart(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-08T14:27:59.621Z
Learnt from: xsahil03x
PR: GetStream/stream-chat-flutter#2348
File: packages/stream_chat_flutter_core/lib/src/stream_channel.dart:383-406
Timestamp: 2025-08-08T14:27:59.621Z
Learning: In stream_chat_flutter_core/lib/src/stream_channel.dart, threads (replies) do not support around-anchor loading. Thread replies are fetched as: initial latest page via StreamChannelState.getReplies(), and further pagination via StreamChannelState.queryReplies(parentId, direction: top|bottom). Anchored loads apply only to channel messages, not to threads.
Applied to files:
packages/stream_chat/test/src/client/client_test.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test
- GitHub Check: build (android)
- GitHub Check: build (ios)
- GitHub Check: analyze
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat_persistence
- GitHub Check: analyze_legacy_versions
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2401 +/- ##
==========================================
+ Coverage 63.84% 63.86% +0.02%
==========================================
Files 413 413
Lines 25862 25865 +3
==========================================
+ Hits 16511 16519 +8
+ Misses 9351 9346 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| await delay(300); | ||
| await delay(1050); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this can also be done with expectAsync(1/2/3) instead?
Submit a pull request
Fixes: FLU-258, FLU-235
Fixes: #2397
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation
Tests