perf(llc): Reduce the number of DB reads in ChatPersistenceClient.getChannelStates()#2664
perf(llc): Reduce the number of DB reads in ChatPersistenceClient.getChannelStates()#2664VelikovPetar wants to merge 11 commits into
ChatPersistenceClient.getChannelStates()#2664Conversation
📝 WalkthroughWalkthroughMigrate persistence isolate wiring from Moor to Drift, update entity docs to reference Drift, add a batched member DAO, refactor getChannelStates to envelope, sort, paginate, and hydrate only the requested page with conditional membership preloading, and update tests and changelog. ChangesDrift migration and channel states optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
|
ChatPersistenceClient.getClientStates()ChatPersistenceClient.getChannelStates()
# Conflicts: # packages/stream_chat_persistence/CHANGELOG.md
…offline' into feature/optimize-query-channels-offline
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/stream_chat_persistence/lib/src/stream_chat_persistence_client.dart (1)
294-299: 💤 Low valuePagination slicing LGTM, but
s.channel!.cidis doubly-asserted — consider tightening.
s.channel!.cidassumes bothchanneland itscidare non-null. The first is safe because envelopes are constructed locally asChannelState(channel: m). The second relies on every row fromchannelQueryDao.getChannelshaving a non-null cid — which is true today, but a future schema/DAO change that filters or projects channels differently would surface here as a runtimeNull check operatorcrash insideFuture.wait. AwhereType<String>on the cid stream (like you already do in_attachMemberships) would be consistent and defensive.♻️ Minor consistency tweak
- final pagedCids = - envelopes.skip(offset).take(limit).map((s) => s.channel!.cid).toList(); + final pagedCids = envelopes + .skip(offset) + .take(limit) + .map((s) => s.channel?.cid) + .whereType<String>() + .toList(growable: 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_persistence/lib/src/stream_chat_persistence_client.dart` around lines 294 - 299, The code is using a double null-assertion s.channel!.cid when building pagedCids; replace this with a defensive projection that skips null cids (e.g., map to s.channel?.cid and filter non-null) so we don't crash if cid becomes null in future schema/DAO changes—apply the same pattern you used in _attachMemberships (whereType<String>) to the pagedCids construction that reads from envelopes and paginationParams to ensure only valid String cids are collected.packages/stream_chat_persistence/lib/src/dao/member_dao.dart (2)
50-56: 💤 Low valueAvoid reading the same row table twice.
row.readTable(members)is invoked twice for each row in the map literal. Cache it once for clarity and a tiny CPU win.♻️ Proposed refactor
- return { - for (final row in rows) - row.readTable(members).channelCid: row.readTable(members).toMember( - user: row.readTableOrNull(users)?.toUser(), - ), - }; + return { + for (final row in rows) + if (row.readTable(members) case final memberEntity) + memberEntity.channelCid: memberEntity.toMember( + user: row.readTableOrNull(users)?.toUser(), + ), + };Or use a plain
forloop with a localfinal memberEntity = row.readTable(members);.🤖 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_persistence/lib/src/dao/member_dao.dart` around lines 50 - 56, The map literal is calling row.readTable(members) twice per row (in the key and value); cache the result in a local variable (e.g., memberEntity) or switch to an explicit for loop so you call row.readTable(members) once, then use memberEntity.channelCid for the key and memberEntity.toMember(user: row.readTableOrNull(users)?.toUser()) for the value; update references to members, toMember, and readTableOrNull(users) accordingly.
42-48: 💤 Low valueConsider chunking
cidsingetMembershipsForChannelswhen preloading memberships
MemberDao.getMembershipsForChannelsuses a singleWHERE members.channelCid IN (cids)query; with largecids, this can hit SQLite bound-variable limits. The current offline path usually keepscidssmall (defaultPaginationParams.limit = 10), butgetChannelStatesaccepts nullablepaginationParamsand preloads memberships for all cached channels when it’s omitted—so chunking/batching would prevent failures in large/unpaginated reuse cases. (packages/stream_chat_persistence/lib/src/dao/member_dao.dart:42-48)🤖 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_persistence/lib/src/dao/member_dao.dart` around lines 42 - 48, getMembershipsForChannels currently builds a single query using members.channelCid.isIn(cids) which can exceed SQLite bound-variable limits for large cids; modify MemberDao.getMembershipsForChannels to chunk the cids list into safe-sized batches (e.g., 500–900 ids per batch), run the select(join(leftOuterJoin(users, members.userId.equalsExp(users.id)))) with the existing where clause (members.channelCid.isIn(batch) & members.userId.equals(userId)) for each batch, collect and concatenate all rows across batches, and return the combined result so large/unpaginated preloads won’t hit the bind-variable limit.packages/stream_chat_persistence/test/stream_chat_persistence_client_test.dart (1)
332-360: ⚡ Quick winMock/verification updates are correct, but the new branches are uncovered.
The updated
messagePagination: const PaginationParams(limit: 25)matchers match the implementation. However, the refactor introduced two notable new code paths that this test does not exercise:
_sortRequiresMembership+_attachMemberships(triggered whenchannelStateSortcontainspinnedAt) →memberDao.getMembershipsForChannelsshould be called exactly once with the page-relevant cids and the connected userId.paginationParamsslicing (offset/limit/empty page) — previously hydration was unconditional, now an out-of-range offset should produce zerogetChannelStateByCidcalls.Consider adding a couple of focused tests so future regressions in either branch are caught.
🤖 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_persistence/test/stream_chat_persistence_client_test.dart` around lines 332 - 360, The test misses two new branches introduced by the refactor: add focused tests for getChannelStates that (1) exercise the _sortRequiresMembership + _attachMemberships path by making channelStateSort include "pinnedAt" and then verify memberDao.getMembershipsForChannels is called exactly once with only the page-relevant cids and the connected userId, and (2) exercise pagination slicing by calling getChannelStates with an out-of-range offset so that paginationParams yields an empty page and verify getChannelStateByCid is not called (zero calls); update the mocked responses and verifications around getChannelStateByCid, memberDao.getMembershipsForChannels, and paginationParams to assert these behaviors.packages/stream_chat_persistence/CHANGELOG.md (1)
1-5: ⚡ Quick winDocument the offline-message-window behavior change.
The new
getChannelStatesimplementation now loads at most 25 messages per channel from persistence (previously it returned all cached messages per channel). That's not strictly a perf-only change — consumers who relied on the persistence layer returning the full cached history will observe fewer messages after this upgrade. Worth adding a 🔄 Changed bullet noting the cap so users aren't surprised.📝 Suggested addition
# Upcoming 🚀 Performance - Reduce the number of DB reads in the `ChatPersistenceClient.getChannelStates` method. + +🔄 Changed + +- `ChatPersistenceClient.getChannelStates` now hydrates at most 25 messages per channel + (matching the API default) instead of returning every cached message. Channels not in + the requested page are no longer hydrated at all.🤖 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_persistence/CHANGELOG.md` around lines 1 - 5, Update the CHANGELOG.md to document the behavioral change in ChatPersistenceClient.getChannelStates: add a "🔄 Changed" bullet under Upcoming or Performance explaining that getChannelStates now loads at most 25 messages per channel from persistence (previously returned full cached history), mention the offline-message-window cap (25 messages) and note that consumers relying on full history may see fewer messages after upgrading. Keep the note concise and add a short migration/impact hint for consumers to adjust if they relied on full cached history.
🤖 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_persistence/lib/src/stream_chat_persistence_client.dart`:
- Around line 301-308: getChannelStates currently hard-caps per-channel
hydration to 25 messages by calling getChannelStateByCid(cid, messagePagination:
const PaginationParams(limit: 25)); make this behavior opt-in by adding a
messagePagination (or at least limit) parameter to the public getChannelStates
signature and pass that through to getChannelStateByCid instead of the
hard-coded PaginationParams; also update
packages/stream_chat_persistence/CHANGELOG.md to call out the new default
per-channel truncation and document the new option so callers can opt in to
larger hydration windows.
---
Nitpick comments:
In `@packages/stream_chat_persistence/CHANGELOG.md`:
- Around line 1-5: Update the CHANGELOG.md to document the behavioral change in
ChatPersistenceClient.getChannelStates: add a "🔄 Changed" bullet under Upcoming
or Performance explaining that getChannelStates now loads at most 25 messages
per channel from persistence (previously returned full cached history), mention
the offline-message-window cap (25 messages) and note that consumers relying on
full history may see fewer messages after upgrading. Keep the note concise and
add a short migration/impact hint for consumers to adjust if they relied on full
cached history.
In `@packages/stream_chat_persistence/lib/src/dao/member_dao.dart`:
- Around line 50-56: The map literal is calling row.readTable(members) twice per
row (in the key and value); cache the result in a local variable (e.g.,
memberEntity) or switch to an explicit for loop so you call
row.readTable(members) once, then use memberEntity.channelCid for the key and
memberEntity.toMember(user: row.readTableOrNull(users)?.toUser()) for the value;
update references to members, toMember, and readTableOrNull(users) accordingly.
- Around line 42-48: getMembershipsForChannels currently builds a single query
using members.channelCid.isIn(cids) which can exceed SQLite bound-variable
limits for large cids; modify MemberDao.getMembershipsForChannels to chunk the
cids list into safe-sized batches (e.g., 500–900 ids per batch), run the
select(join(leftOuterJoin(users, members.userId.equalsExp(users.id)))) with the
existing where clause (members.channelCid.isIn(batch) &
members.userId.equals(userId)) for each batch, collect and concatenate all rows
across batches, and return the combined result so large/unpaginated preloads
won’t hit the bind-variable limit.
In
`@packages/stream_chat_persistence/lib/src/stream_chat_persistence_client.dart`:
- Around line 294-299: The code is using a double null-assertion s.channel!.cid
when building pagedCids; replace this with a defensive projection that skips
null cids (e.g., map to s.channel?.cid and filter non-null) so we don't crash if
cid becomes null in future schema/DAO changes—apply the same pattern you used in
_attachMemberships (whereType<String>) to the pagedCids construction that reads
from envelopes and paginationParams to ensure only valid String cids are
collected.
In
`@packages/stream_chat_persistence/test/stream_chat_persistence_client_test.dart`:
- Around line 332-360: The test misses two new branches introduced by the
refactor: add focused tests for getChannelStates that (1) exercise the
_sortRequiresMembership + _attachMemberships path by making channelStateSort
include "pinnedAt" and then verify memberDao.getMembershipsForChannels is called
exactly once with only the page-relevant cids and the connected userId, and (2)
exercise pagination slicing by calling getChannelStates with an out-of-range
offset so that paginationParams yields an empty page and verify
getChannelStateByCid is not called (zero calls); update the mocked responses and
verifications around getChannelStateByCid, memberDao.getMembershipsForChannels,
and paginationParams to assert these behaviors.
🪄 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: 7825573d-f714-4ea0-9fde-b95632444682
📒 Files selected for processing (21)
packages/stream_chat_persistence/CHANGELOG.mdpackages/stream_chat_persistence/README.mdpackages/stream_chat_persistence/lib/src/dao/member_dao.dartpackages/stream_chat_persistence/lib/src/db/drift_chat_database.dartpackages/stream_chat_persistence/lib/src/db/shared/native_db.dartpackages/stream_chat_persistence/lib/src/entity/channel_queries.dartpackages/stream_chat_persistence/lib/src/entity/channels.dartpackages/stream_chat_persistence/lib/src/entity/connection_events.dartpackages/stream_chat_persistence/lib/src/entity/draft_messages.dartpackages/stream_chat_persistence/lib/src/entity/members.dartpackages/stream_chat_persistence/lib/src/entity/messages.dartpackages/stream_chat_persistence/lib/src/entity/pinned_message_reactions.dartpackages/stream_chat_persistence/lib/src/entity/pinned_messages.dartpackages/stream_chat_persistence/lib/src/entity/poll_votes.dartpackages/stream_chat_persistence/lib/src/entity/polls.dartpackages/stream_chat_persistence/lib/src/entity/reactions.dartpackages/stream_chat_persistence/lib/src/entity/reads.dartpackages/stream_chat_persistence/lib/src/entity/users.dartpackages/stream_chat_persistence/lib/src/stream_chat_persistence_client.dartpackages/stream_chat_persistence/test/src/db/drift_chat_database_test.dartpackages/stream_chat_persistence/test/stream_chat_persistence_client_test.dart
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2664 +/- ##
==========================================
+ Coverage 65.27% 65.33% +0.05%
==========================================
Files 423 423
Lines 26622 26646 +24
==========================================
+ Hits 17377 17408 +31
+ Misses 9245 9238 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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_persistence/test/stream_chat_persistence_client_test.dart`:
- Around line 382-387: The assertion uses the hardcoded fixture 'test-user-id'
instead of the connected client user variable; update the expectation for the
second captured argument from the literal 'test-user-id' to the testUserId
variable so verify(() =>
mockDatabase.memberDao.getMembershipsForChannels(captureAny(),
captureAny())).captured is asserted as first == cids and last == testUserId.
🪄 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: 59ac3d35-b6cb-4611-bcaa-90da821418fe
📒 Files selected for processing (1)
packages/stream_chat_persistence/test/stream_chat_persistence_client_test.dart
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_persistence/lib/src/stream_chat_persistence_client.dart`:
- Around line 301-303: The inline comment is stale: it claims messagePagination
is passed but getChannelStateByCid is invoked without it; either remove the
misleading text or actually thread a PaginationParams through getChannelStates
and into getChannelStateByCid so callers can opt into a limit. To fix, choose
one of two options: (A) update the comment above the
Future.wait(pagedCids.map(getChannelStateByCid)) line to remove the "Pass
messagePagination" clause so it accurately reflects current behavior, or (B) add
a messagePagination parameter to getChannelStates and modify
getChannelStateByCid (and its callers) to accept and use that PaginationParams
value when hydrating channel state; reference the functions getChannelStates and
getChannelStateByCid and the messagePagination/PaginationParams symbol when
making the change.
🪄 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: cc90da6e-62d0-444a-ad9d-0d0699e17688
📒 Files selected for processing (1)
packages/stream_chat_persistence/lib/src/stream_chat_persistence_client.dart
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/stream_chat_persistence/test/stream_chat_persistence_client_test.dart (1)
312-313:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign membership assertion with the connected client user id.
At Line 387, the assertion is tied to
currentUserId(Line 312), which is a fixture value and can diverge from the connected user (userId). Use the connected id to keep this test validating the real contract ofgetChannelStates.Proposed fix
- const currentUserId = 'test-user-id'; + const currentUserId = userId;Also applies to: 320-329, 387-387
🤖 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_persistence/test/stream_chat_persistence_client_test.dart` around lines 312 - 313, The test uses the fixture constant currentUserId when building the membership Filter and asserting channel membership, but the test connects a client as userId — replace references to currentUserId with the actual connected user identifier (use the test's userId variable or the connected client's id, e.g., client.state.currentUser!.id) so the Filter.in_('members', [ ... ]) and all assertions that check membership (including those in the same test's channel creation/verification blocks) assert against the real connected user id instead of the fixture constant; update occurrences around the getChannelStates test and the related assertions to use userId/client.state.currentUser!.id accordingly.
🤖 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_persistence/test/stream_chat_persistence_client_test.dart`:
- Around line 312-313: The test uses the fixture constant currentUserId when
building the membership Filter and asserting channel membership, but the test
connects a client as userId — replace references to currentUserId with the
actual connected user identifier (use the test's userId variable or the
connected client's id, e.g., client.state.currentUser!.id) so the
Filter.in_('members', [ ... ]) and all assertions that check membership
(including those in the same test's channel creation/verification blocks) assert
against the real connected user id instead of the fixture constant; update
occurrences around the getChannelStates test and the related assertions to use
userId/client.state.currentUser!.id accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bdb7a334-4fe9-4207-82c2-decd2b9be03e
📒 Files selected for processing (3)
packages/stream_chat_persistence/lib/src/stream_chat_persistence_client.dartpackages/stream_chat_persistence/test/src/dao/member_dao_test.dartpackages/stream_chat_persistence/test/stream_chat_persistence_client_test.dart
Submit a pull request
Linear: FLU-484
CLA
Description of the pull request
Reduces the amount of DB reads done in
ChatPersistenceClient.getChannelStatesbyRenames some leftover docs / private methods referencing
Moorinstead ofDriftScreenshots / Videos
NA
Testing
The following patch contains a benchmark test - comparing the new and old implementations. You can run the bench locally to verify the changes in performance.
Benchmark
Summary by CodeRabbit
Performance Improvements
New Features
Documentation
Tests