feat(llc, core, persistence): Add support for PredefinedFilters on QueryChannels#2709
feat(llc, core, persistence): Add support for PredefinedFilters on QueryChannels#2709VelikovPetar wants to merge 19 commits into
PredefinedFilters on QueryChannels#2709Conversation
… dynamic> to Filter.
|
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 (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds predefined filter support across the SDK. A new ChangesPredefined Filter Feature
Xcode Scheme Pre-Action
Sequence Diagram(s)sequenceDiagram
participant App as StreamChannelListController
participant Client as StreamChatClient
participant Persistence as StreamChatPersistenceClient
participant API as ChannelApi
rect rgba(100, 149, 237, 0.5)
note over App,Persistence: Offline-first pass
App->>Client: queryChannelsWithResult(predefinedFilter, filterValues, sortValues)
Client->>Persistence: queryChannelStates(predefinedFilter, filterValues, sortValues)
Persistence->>Persistence: getChannelsAndSpecByPredefinedFilter → (channels, filter, sort)
Persistence-->>Client: QueryChannelsResponse (offline)
Client-->>App: emit QueryChannelsResult (channels, predefinedFilter) if non-empty
end
rect rgba(60, 179, 113, 0.5)
note over Client,API: Online fetch and persist
Client->>API: queryChannels(predefinedFilter, filterValues, sortValues)
API-->>Client: QueryChannelsResponse (resolved predefinedFilter, channels)
Client->>Persistence: saveChannelQueries(cids, resolvedFilter, resolvedSort, predefinedFilter)
Persistence->>Persistence: updateChannelQueriesByPredefinedFilter(name, cids, filter, sort)
Client-->>App: emit QueryChannelsResult (channels, predefinedFilter)
App->>App: _resolveSort(result) → update _resolvedChannelStateSort
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 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 |
|
| /// Yields the offline-cached result first (when available), followed by | ||
| /// the online result. Concurrent identical online queries are coalesced | ||
| /// via [_queryChannelsCache]. | ||
| Stream<QueryChannelsResult> queryChannelsWithResult({ |
There was a problem hiding this comment.
I wasn't sure about the naming here, I tried to make it as explicit as possible. (it wasn't really possible to use the existing queryChannels without a breaking change, as we must change the return type).
| /// Carries the live [Channel] instances matching the query alongside the | ||
| /// server-resolved [PredefinedFilter] spec (when one is associated with the | ||
| /// query). | ||
| class QueryChannelsResult { |
There was a problem hiding this comment.
I introduced this new model to serve as a container for the result of the StreamChatClient.queryChannelsWithResult.
| Future<List<Channel>> queryChannelsOnline({ | ||
| Filter? filter, | ||
| SortOrder<ChannelState>? sort, | ||
| String? predefinedFilter, |
There was a problem hiding this comment.
Note: At the moment I didn't expose public queryChannelsWithResultOnline/queryChannelsWithResultOffline -> only the new queryChannelsWithResults is public. I didn't want to pollute the public API, with methods which most likely will not be used. Let me know if you think we should expose them as well.
| /// Default implementation returns an empty response; persistence | ||
| /// implementations that support predefined-filter caching should override | ||
| /// this. | ||
| Future<QueryChannelsResponse> getChannelStatesByPredefinedFilter({ |
There was a problem hiding this comment.
Do you think it is OK to return QueryChannelsResponse here - or should we introduce a new specific model for this?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2709 +/- ##
==========================================
+ Coverage 68.42% 68.75% +0.32%
==========================================
Files 413 417 +4
Lines 24944 25081 +137
==========================================
+ Hits 17069 17244 +175
+ Misses 7875 7837 -38 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
# Conflicts: # packages/stream_chat_persistence/lib/src/db/drift_chat_database.dart # packages/stream_chat_persistence/lib/src/stream_chat_persistence_client.dart
|
|
||
| void _resolveSort(QueryChannelsResult result) { | ||
| final resolved = result.predefinedFilter?.sort; | ||
| if (resolved != null) _resolvedChannelStateSort = resolved; |
There was a problem hiding this comment.
Should we handle the null case here or always rely on the value returned from backend?
There was a problem hiding this comment.
Good point, I only added the fallback sort when predefinedFilter.sort == null when persisting the sort in the DB. I will fix this, we should also apply the same fallback here (only when predefinedFilter != null && predefinedFilter.sort == null).
Edit: A bit more info, if we get a PredefinedFilter with null sort, it means that there is no sort template provided for that predefined filter. So the BE will use a fallback sorter, calculated as predefinedFilterFallbackSort
There was a problem hiding this comment.
Addressed here: 784e39c
Note: I also refactored the fallbackSort calculation a bit, to avoid exposing the algorithm as a public API.
There was a problem hiding this comment.
Edit: A bit more info, if we get a PredefinedFilter with null sort, it means that there is no sort template provided for that predefined filter. So the BE will use a fallback sorter, calculated as predefinedFilterFallbackSort
Not sure, but in case there's no sort template provided for the predefined filter, then the backend will use a fallback sorter, so shouldn't we also set it if we don't receive it in the result? Currently we are handling the case only when its not null.
Related: https://github.com/GetStream/stream-chat-flutter/pull/2709/changes#r3414280459
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/stream_chat/CHANGELOG.md (1)
11-12: 💤 Low valueChangelog entries accurately document the predefined filter feature—consider explicitly naming the new
QueryChannelsResulttype.The entries describe the
queryChannelsandqueryChannelsWithResultAPI expansions and the new unified persistence methods clearly. However, the PR introduces a newQueryChannelsResulttype that is not explicitly mentioned. While it may be implicit viaqueryChannelsWithResult, calling it out would improve clarity for developers consulting the changelog.💡 Optional enhancement to Line 11
- Added support for predefined filters for `QueryChannels` on `StreamChatClient` (`StreamChatClient.queryChannels` and `StreamChatClient.queryChannelsWithResult`). + Added support for predefined filters for `QueryChannels` on `StreamChatClient` via `StreamChatClient.queryChannels` and `StreamChatClient.queryChannelsWithResult` (returns `QueryChannelsResult`).🤖 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/CHANGELOG.md` around lines 11 - 12, Update the CHANGELOG.md entries at lines 11-12 to explicitly mention the new QueryChannelsResult type that was introduced in this PR. While the current entries describe the queryChannels and queryChannelsWithResult method expansions, adding a specific reference to the QueryChannelsResult type will make the changelog clearer for developers. Add an entry that clearly documents the introduction of this new type alongside or near the existing entries about the queryChannels API changes.packages/stream_chat_flutter_core/lib/src/stream_channel_list_controller.dart (1)
203-233: ⚡ Quick winConsider calling
_resolveSort(result)inloadMorefor consistency.
doInitialLoadcalls_resolveSort(result)after each query response to keep_resolvedChannelStateSortsynchronized with the server (line 184), butloadMoredoes not. If the server changes the resolved sort between initial load and pagination (unlikely but possible), the controller will use a stale sort for subsequent operations.For correctness and consistency with
doInitialLoad, add the same call here after line 217:🔄 Suggested addition
)) { + _resolveSort(result); final channels = result.channels;🤖 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_list_controller.dart` around lines 203 - 233, The loadMore method is missing a call to _resolveSort(result) to keep _resolvedChannelStateSort synchronized with the server, which is inconsistently different from the doInitialLoad method that does call _resolveSort(result) after each query response. Add a call to _resolveSort(result) inside the await for loop in loadMore after receiving the result from client.queryChannelsWithResult to ensure the resolved sort is updated consistently and prevents using stale sort values for subsequent operations.
🤖 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 428-453: The saveChannelQueries method is missing a required doc
comment on this public API. Add a doc comment above the method that documents
its purpose, explains all parameters, and clarifies when to use this method
versus the deprecated updateChannelQueries method to help developers understand
the API better.
- Around line 302-323: The queryChannelStates method is a public API that is
missing a required doc comment. Add a documentation comment above the method
(before the `@override` annotation) that explains what the method does, describes
each parameter (filter, sort, predefinedFilter, filterValues, sortValues,
messageLimit, paginationParams), and documents the Future<QueryChannelsResponse>
return value to meet the coding guidelines for public APIs.
---
Nitpick comments:
In
`@packages/stream_chat_flutter_core/lib/src/stream_channel_list_controller.dart`:
- Around line 203-233: The loadMore method is missing a call to
_resolveSort(result) to keep _resolvedChannelStateSort synchronized with the
server, which is inconsistently different from the doInitialLoad method that
does call _resolveSort(result) after each query response. Add a call to
_resolveSort(result) inside the await for loop in loadMore after receiving the
result from client.queryChannelsWithResult to ensure the resolved sort is
updated consistently and prevents using stale sort values for subsequent
operations.
In `@packages/stream_chat/CHANGELOG.md`:
- Around line 11-12: Update the CHANGELOG.md entries at lines 11-12 to
explicitly mention the new QueryChannelsResult type that was introduced in this
PR. While the current entries describe the queryChannels and
queryChannelsWithResult method expansions, adding a specific reference to the
QueryChannelsResult type will make the changelog clearer for developers. Add an
entry that clearly documents the introduction of this new type alongside or near
the existing entries about the queryChannels API changes.
🪄 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: d4f485b0-756d-4881-a34b-cdfc1d4ac3f9
📒 Files selected for processing (42)
packages/stream_chat/CHANGELOG.mdpackages/stream_chat/lib/src/client/client.dartpackages/stream_chat/lib/src/client/query_channels_result.dartpackages/stream_chat/lib/src/core/api/channel_api.dartpackages/stream_chat/lib/src/core/api/responses.dartpackages/stream_chat/lib/src/core/api/responses.g.dartpackages/stream_chat/lib/src/core/api/sort_order.dartpackages/stream_chat/lib/src/core/models/predefined_filter.dartpackages/stream_chat/lib/src/core/models/predefined_filter.g.dartpackages/stream_chat/lib/src/db/chat_persistence_client.dartpackages/stream_chat/lib/stream_chat.dartpackages/stream_chat/test/src/client/client_test.dartpackages/stream_chat/test/src/core/api/channel_api_test.dartpackages/stream_chat/test/src/core/models/predefined_filter_test.dartpackages/stream_chat_flutter_core/CHANGELOG.mdpackages/stream_chat_flutter_core/lib/src/stream_channel_list_controller.dartpackages/stream_chat_flutter_core/test/stream_channel_list_controller_test.dartpackages/stream_chat_persistence/CHANGELOG.mdpackages/stream_chat_persistence/lib/src/converter/converter.dartpackages/stream_chat_persistence/lib/src/converter/filter_converter.dartpackages/stream_chat_persistence/lib/src/converter/sort_order_converter.dartpackages/stream_chat_persistence/lib/src/dao/channel_query_dao.dartpackages/stream_chat_persistence/lib/src/dao/channel_query_dao.g.dartpackages/stream_chat_persistence/lib/src/dao/draft_message_dao.g.dartpackages/stream_chat_persistence/lib/src/dao/location_dao.g.dartpackages/stream_chat_persistence/lib/src/dao/member_dao.g.dartpackages/stream_chat_persistence/lib/src/dao/message_dao.g.dartpackages/stream_chat_persistence/lib/src/dao/pinned_message_reaction_dao.g.dartpackages/stream_chat_persistence/lib/src/dao/poll_vote_dao.g.dartpackages/stream_chat_persistence/lib/src/dao/reaction_dao.g.dartpackages/stream_chat_persistence/lib/src/dao/read_dao.g.dartpackages/stream_chat_persistence/lib/src/db/drift_chat_database.dartpackages/stream_chat_persistence/lib/src/db/drift_chat_database.g.dartpackages/stream_chat_persistence/lib/src/entity/channel_queries_metadata.dartpackages/stream_chat_persistence/lib/src/entity/entity.dartpackages/stream_chat_persistence/lib/src/stream_chat_persistence_client.dartpackages/stream_chat_persistence/test/src/converter/filter_converter_test.dartpackages/stream_chat_persistence/test/src/converter/sort_order_converter_test.dartpackages/stream_chat_persistence/test/src/dao/channel_query_dao_test.dartpackages/stream_chat_persistence/test/stream_chat_persistence_client_test.dartsample_app/ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcschemesample_app/lib/widgets/channel_list.dart
💤 Files with no reviewable changes (7)
- packages/stream_chat_persistence/lib/src/dao/member_dao.g.dart
- packages/stream_chat_persistence/lib/src/dao/message_dao.g.dart
- packages/stream_chat_persistence/lib/src/dao/location_dao.g.dart
- packages/stream_chat_persistence/lib/src/dao/reaction_dao.g.dart
- packages/stream_chat_persistence/lib/src/dao/read_dao.g.dart
- packages/stream_chat_persistence/lib/src/dao/pinned_message_reaction_dao.g.dart
- packages/stream_chat_persistence/lib/src/dao/poll_vote_dao.g.dart
| const defaultChannelPagedLimit = 10; | ||
|
|
||
| /// The default sort used for the channel list. | ||
| const defaultChannelListSort = [ |
There was a problem hiding this comment.
I think we also need to update this and have the default one based on if we are passing predefined filters or not. wdyt?
There was a problem hiding this comment.
Let me check how we can improve this. In the predefined filters case, we need to already have the QueryChannels response, so that we can derive the default sort based on the filter fields (if sort is not already returned by the response)
There was a problem hiding this comment.
What do you think about something like:
StreamChannelListController({
required this.client,
StreamChannelListEventHandler? eventHandler,
this.filter,
this.predefinedFilter,
this.filterValues,
this.sortValues,
this.presence = true,
this.limit = defaultChannelPagedLimit,
this.messageLimit,
this.memberLimit,
}) : _eventHandler = eventHandler ?? StreamChannelListEventHandler(),
_resolvedChannelStateSort =
predefinedFilter == null ? channelStateSort : null,
super(const PagedValue.loading()); I was also considering something like:
StreamChannelListController({
required this.client,
StreamChannelListEventHandler? eventHandler,
this.filter,
SortOrder<ChannelState>? channelStateSort, // ← no inline default, nullable named param
this.predefinedFilter,
this.filterValues,
this.sortValues,
this.presence = true,
this.limit = defaultChannelPagedLimit,
this.messageLimit,
this.memberLimit,
}) : channelStateSort = channelStateSort ??
(predefinedFilter == null ? defaultChannelListSort : null),
_resolvedChannelStateSort = channelStateSort ??
(predefinedFilter == null ? defaultChannelListSort : null),
_eventHandler = eventHandler ?? StreamChannelListEventHandler(),
super(const PagedValue.loading());However this could be a breaking change, if a customer is instantiating the StreamChannelListController as:
StreamChannelListController(
client: client,
channelStateSort: null, // explicit "I don't want a sort"
)In which case they will now fallback to the defaultChannelListSort, instead of the explicit null.
2 participants
Submit a pull request
Linear: FLU-357
CLA
Description of the pull request
Adds support for server-defined predefined filters on
queryChannelsacross the LLC, persistence, and core layers.A predefined filter is a named filter/sort preset that lives on the server. The client sends
predefined_filter(plus optionalfilter_values/sort_valuesto interpolate placeholders) instead of an inline filter, and the server echoes back the materializedfilterandsortit applied.stream_chat(LLC)ChannelApi.queryChannelsandStreamChatClient.queryChannelsacceptpredefinedFilter,filterValues,sortValues.StreamChatClient.queryChannelsWithResultreturns aQueryChannelsResultexposing both the live channels and the server-resolvedPredefinedFilter(name + materialized filter + sort).PredefinedFiltermodel andpredefined_filterfield onQueryChannelsResponse.SortOption<T>.fromJsonadded so sort specs can round-trip through persistence.ChatPersistenceClienthooks:getChannelStatesByPredefinedFilterandupdateChannelQueriesByPredefinedFilter(default no-op so existing implementations keep compiling).stream_chat_persistencechannel_query_metadatatable that stores the server-resolvedfilter+sortkeyed by the predefined-filter query hash, so offline reads reconstruct the exact resolved spec the server applied.FilterConverterandChannelStateSortOrderConverterDrift converters.StreamChatPersistenceClientimplements the new hooks;ChannelQueryDaogainsupdateChannelQueriesByPredefinedFilterandgetChannelsAndSpecByPredefinedFilter.stream_chat_flutter_coreStreamChannelListControlleracceptspredefinedFilter,filterValues,sortValues. When set, they take precedence overfilter/channelStateSort._resolvedChannelStateSortthat is overwritten with the server-resolved sort on every successful query, so event-driven inserts keep matching the server's ordering even when the caller only specifies apredefinedFilter.Sample app
ChannelListnow uses thestream_chat_flutter_sample_apppredefined filter (interpolated with the currentuser_id) to demonstrate the flow end-to-end.How to test
Test coverage
predefined_filter_test.dart— model.predefined_filter_defaults_test.dart— fallback-sort selection.client_test.dart— online/offlinequeryChannelspaths andqueryChannelsWithResult.channel_api_test.dart— request payload assertions.filter_converter_test.dart/sort_order_converter_test.dart— Drift converters.channel_query_dao_test.dart— DAO read/write of cids + metadata.stream_chat_persistence_client_test.dart— persistence client integration.stream_channel_list_controller_test.dart— controller path including resolved-sort tracking.Summary by CodeRabbit
predefinedFilterwithfilterValues/sortValues, including server-resolved filter/sort materialization.queryChannelsWithResult(and theQueryChannelsResultreturn type) to provide both channels and the resolved predefined filter details.StreamChannelListControllerto accept predefined filters and automatically apply the resolved sort.