Add messageSearchSort option to ChannelListViewModel and ChannelViewModelFactory#6187
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
…odelFactory Co-Authored-By: Claude <noreply@anthropic.com>
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
SDK Size Comparison 📏
|
…_messages' into feature/AND-1087_add_messagesearch_sort_option_to_channel_list # Conflicts: # stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.kt
Co-Authored-By: Claude <noreply@anthropic.com>
VelikovPetar
changed the title
messageSearchSort option to ChannelListViewModel and ChannelViewModelFactory
|
WalkthroughThis pull request migrates message search pagination from offset-based to cursor-based (next-token) pagination across both the compose and UI components modules. The ChannelListViewModel and SearchViewModel are updated with a messageSearchSort parameter and a next field to support server-side message sorting and continued pagination. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.kt (1)
356-356:⚠️ Potential issue | 🟡 MinorBug: Logger interpolates the literal string
'query'instead of the variable$query.Line 356 logs the string
"query"literally instead of the variable's value.🐛 Fix string interpolation
- logger.v { "[loadMoreQueryMessages] query: 'query'" } + logger.v { "[loadMoreQueryMessages] query: '$query'" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.kt` at line 356, In ChannelListViewModel inside the loadMoreQueryMessages logging call, the logger currently prints the literal text 'query' instead of the variable; update the logger.v lambda to interpolate the actual query variable (e.g., use $query or ${query} in the message) so the runtime value of the query is logged; ensure you reference the correct variable name used in the surrounding function (loadMoreQueryMessages / query) and keep the existing log prefix "[loadMoreQueryMessages]" unchanged.
🧹 Nitpick comments (2)
stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.kt (1)
372-372:channelLimitis reused as the message search page size.
searchMessagesuseschannelLimit(which the user sets for channel pagination) as thelimitfor message search. If someone configures a smaller or largerchannelLimitfor channels, it silently changes the message search page size too. Consider introducing a dedicatedmessageSearchLimitparameter or a local constant if the intent is a fixed page size.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.kt` at line 372, The current use of channelLimit as the limit for message searches (the val limit = channelLimit used before calling searchMessages) causes message search page size to be tied to channel pagination; fix this by introducing a dedicated messageSearchLimit (or a local constant) and use that when calling searchMessages instead of channelLimit—update ChannelListViewModel (where searchMessages is invoked) to reference messageSearchLimit (or a new constant) so channelLimit remains only for channel pagination.stream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/viewmodel/search/SearchViewModel.kt (1)
160-167: On error,canLoadMoreis left unchanged — verify this is intentional.When
handleSearchMessagesErrorfires,canLoadMoreis not reset (nor isnext). On initial load (wherecanLoadMoredefaults totrue), this means the user can callloadMore()after an error, which will retry the same request (withnext = null). This is arguably reasonable retry behavior, but it differs from the old offset-based approach wherecanLoadMorewas not forcibly set totrueon error either. Just flagging for awareness — if you want errors to be non-retryable vialoadMore, you'd need to setcanLoadMore = falsehere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/viewmodel/search/SearchViewModel.kt` around lines 160 - 167, handleSearchMessagesError currently logs and clears loading flags but leaves SearchState.canLoadMore and next untouched; to prevent loadMore() from retrying the same failed page, update handleSearchMessagesError to also set _state.value = _state.value!!.copy(isLoading = false, isLoadingMore = false, canLoadMore = false) and reset next if desired (e.g., next = null) so downstream loadMore() won't retry the failed request; locate this logic in the handleSearchMessagesError function and modify the SearchState copy to include canLoadMore and optionally next.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModelTest.kt`:
- Around line 485-494: The test's QuerySorter<Message> captor can't capture a
null sort argument; update the capture to allow nulls or assert with a null
matcher. Replace argumentCaptor<QuerySorter<Message>>() with a nullable captor
argumentCaptor<QuerySorter<Message>?>() (or change the verify call to use sort =
isNull()) when verifying chatClient.searchMessages(..., sort = ...), then keep
assertNull(sortCaptor.firstValue) to correctly assert the null messageSearchSort
in ChannelListViewModelTest.
- Around line 417-427: The test uses an argumentCaptor<String>() named captor to
capture the two calls to ChatClient.searchMessages (in ChannelListViewModelTest)
but capture() won't match a null `next` argument; replace with a nullable
capture or verify calls separately: either use captureNullable() (from
mockito-kotlin) on the captor so captor.firstValue can be null, or split into
two verifications of searchMessages (one verifying next == null and the other
verifying next == "cursor_page2") to avoid relying on capture() matching null.
Update the verify block referencing searchMessages, captor, and the assertions
accordingly.
In
`@stream-chat-android-ui-components/src/test/kotlin/io/getstream/chat/android/ui/viewmodels/search/SearchViewModelTest.kt`:
- Around line 115-125: The test uses a non-null argumentCaptor for the nullable
`next` parameter, causing incorrect capture of null values; update the captor
declaration in SearchViewModelTest (the `captor` used in the verify of
`chatClient.searchMessages`) to a nullable captor by using
`nullableArgumentCaptor<String>()` or `argumentCaptor<String?>()` so the `next`
parameter can properly capture null and non-null values for the assertions.
---
Outside diff comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.kt`:
- Line 356: In ChannelListViewModel inside the loadMoreQueryMessages logging
call, the logger currently prints the literal text 'query' instead of the
variable; update the logger.v lambda to interpolate the actual query variable
(e.g., use $query or ${query} in the message) so the runtime value of the query
is logged; ensure you reference the correct variable name used in the
surrounding function (loadMoreQueryMessages / query) and keep the existing log
prefix "[loadMoreQueryMessages]" unchanged.
---
Nitpick comments:
In
`@stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.kt`:
- Line 372: The current use of channelLimit as the limit for message searches
(the val limit = channelLimit used before calling searchMessages) causes message
search page size to be tied to channel pagination; fix this by introducing a
dedicated messageSearchLimit (or a local constant) and use that when calling
searchMessages instead of channelLimit—update ChannelListViewModel (where
searchMessages is invoked) to reference messageSearchLimit (or a new constant)
so channelLimit remains only for channel pagination.
In
`@stream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/viewmodel/search/SearchViewModel.kt`:
- Around line 160-167: handleSearchMessagesError currently logs and clears
loading flags but leaves SearchState.canLoadMore and next untouched; to prevent
loadMore() from retrying the same failed page, update handleSearchMessagesError
to also set _state.value = _state.value!!.copy(isLoading = false, isLoadingMore
= false, canLoadMore = false) and reset next if desired (e.g., next = null) so
downstream loadMore() won't retry the failed request; locate this logic in the
handleSearchMessagesError function and modify the SearchState copy to include
canLoadMore and optionally next.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
stream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModel.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channels/ChannelViewModelFactory.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModelTest.ktstream-chat-android-ui-components/api/stream-chat-android-ui-components.apistream-chat-android-ui-components/src/main/kotlin/io/getstream/chat/android/ui/viewmodel/search/SearchViewModel.ktstream-chat-android-ui-components/src/test/kotlin/io/getstream/chat/android/ui/viewmodels/search/SearchViewModelTest.kt
| val captor = argumentCaptor<String>() | ||
| verify(chatClient, times(2)).searchMessages( | ||
| channelFilter = any(), | ||
| messageFilter = any(), | ||
| offset = anyOrNull(), | ||
| limit = anyOrNull(), | ||
| next = captor.capture(), | ||
| sort = anyOrNull(), | ||
| ) | ||
| assertNull(captor.firstValue) | ||
| assertEquals("cursor_page2", captor.secondValue) |
There was a problem hiding this comment.
Same nullable argumentCaptor<String>.capture() concern as in SearchViewModelTest.
captor.capture() (line 423) may not match a null argument for the next parameter, causing the assertNull(captor.firstValue) on line 426 to behave unexpectedly. Consider using captureNullable() from mockito-kotlin if available, or verify the two invocations separately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModelTest.kt`
around lines 417 - 427, The test uses an argumentCaptor<String>() named captor
to capture the two calls to ChatClient.searchMessages (in
ChannelListViewModelTest) but capture() won't match a null `next` argument;
replace with a nullable capture or verify calls separately: either use
captureNullable() (from mockito-kotlin) on the captor so captor.firstValue can
be null, or split into two verifications of searchMessages (one verifying next
== null and the other verifying next == "cursor_page2") to avoid relying on
capture() matching null. Update the verify block referencing searchMessages,
captor, and the assertions accordingly.
| val sortCaptor = argumentCaptor<QuerySorter<Message>>() | ||
| verify(chatClient).searchMessages( | ||
| channelFilter = any(), | ||
| messageFilter = any(), | ||
| offset = anyOrNull(), | ||
| limit = anyOrNull(), | ||
| next = anyOrNull(), | ||
| sort = sortCaptor.capture(), | ||
| ) | ||
| assertNull(sortCaptor.firstValue) |
There was a problem hiding this comment.
Same nullable capture concern for QuerySorter<Message> captor.
When messageSearchSort is null, sortCaptor.capture() (line 492) may not match the null argument. The assertNull(sortCaptor.firstValue) on line 494 may not behave as expected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/viewmodel/channels/ChannelListViewModelTest.kt`
around lines 485 - 494, The test's QuerySorter<Message> captor can't capture a
null sort argument; update the capture to allow nulls or assert with a null
matcher. Replace argumentCaptor<QuerySorter<Message>>() with a nullable captor
argumentCaptor<QuerySorter<Message>?>() (or change the verify call to use sort =
isNull()) when verifying chatClient.searchMessages(..., sort = ...), then keep
assertNull(sortCaptor.firstValue) to correctly assert the null messageSearchSort
in ChannelListViewModelTest.
| val captor = argumentCaptor<String>() | ||
| verify(chatClient, times(2)).searchMessages( | ||
| channelFilter = any(), | ||
| messageFilter = any(), | ||
| offset = anyOrNull(), | ||
| limit = anyOrNull(), | ||
| next = captor.capture(), | ||
| sort = anyOrNull(), | ||
| ) | ||
| assertNull(captor.firstValue) | ||
| assertEquals("cursor_page2", captor.secondValue) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check build.gradle files for mockito-kotlin version
fd -e gradle | xargs grep -l "mockito-kotlin" | head -5Repository: GetStream/stream-chat-android
Length of output: 55
🏁 Script executed:
# Look at the test file to understand context better
fd -t f "SearchViewModelTest.kt" | head -1 | xargs wc -lRepository: GetStream/stream-chat-android
Length of output: 196
🌐 Web query:
mockito-kotlin argumentCaptor capture nullable parameters
💡 Result:
In Kotlin, Mockito’s ArgumentCaptor.capture() is declared as returning T, but Mockito may return null during matching, which triggers Kotlin’s generated null-checks and results in ...capture() must not be null (even if the real argument was non-null) [1][2].
Best fix: use Mockito-Kotlin captors (handles Kotlin null-safety)
Mockito-Kotlin provides Kotlin-friendly captors (a KArgumentCaptor) and also a nullable variant (nullableArgumentCaptor()), so capturing nullable parameters works cleanly [3][4]:
import org.mockito.kotlin.argumentCaptor
import org.mockito.kotlin.nullableArgumentCaptor
import org.mockito.kotlin.verify
val captor = nullableArgumentCaptor<String>() // for String?
// or: val captor = argumentCaptor<String?>()
verify(service).method(captor.capture())
val captured: String? = captor.firstValueIf you’re matching nullable args in stubbing/verifying, use anyOrNull() (Mockito-Kotlin) rather than any() [5].
If you must use plain Mockito ArgumentCaptor
Use a tiny helper that “hides” the platform type so Kotlin doesn’t insert a null-check [1]:
fun <T> capture(captor: org.mockito.ArgumentCaptor<T>): T = captor.capture()
val captor = org.mockito.ArgumentCaptor.forClass(String::class.java) // for String?
verify(service).method(capture(captor))Sources: Stack Overflow explanations + helper pattern [1][2], Mockito-Kotlin repo + nullable captor mention [3][4], anyOrNull() note [5].
[1] https://stackoverflow.com/questions/34773958/kotlin-and-argumentcaptor-illegalstateexception
[2] https://stackoverflow.com/questions/65931441/mockito-kotlin-test-throws-null-pointer-exception-when-trying-to-capture-pagea
[3] https://github.com/mockito/mockito-kotlin
[4] https://stackoverflow.com/questions/41945675/mocktito-argumentcaptor-for-kotlin-lambda-with-arguments
[5] https://stackoverflow.com/questions/59230041/argumentmatchers-any-must-not-be-null
🏁 Script executed:
# Check the imports and actual captor usage in the test file
head -30 stream-chat-android-ui-components/src/test/kotlin/io/getstream/chat/android/ui/viewmodels/search/SearchViewModelTest.ktRepository: GetStream/stream-chat-android
Length of output: 1438
🏁 Script executed:
# Check if nullableArgumentCaptor is used elsewhere in the test file
rg "nullableArgumentCaptor|captureNullable" stream-chat-android-ui-components/src/test/kotlin/io/getstream/chat/android/ui/viewmodels/search/SearchViewModelTest.ktRepository: GetStream/stream-chat-android
Length of output: 55
🏁 Script executed:
# Look at lines 115-130 specifically to see the exact context
sed -n '110,130p' stream-chat-android-ui-components/src/test/kotlin/io/getstream/chat/android/ui/viewmodels/search/SearchViewModelTest.ktRepository: GetStream/stream-chat-android
Length of output: 733
🏁 Script executed:
# Check mockito-kotlin version in dependencies
fd build.gradle | xargs grep -A 2 -B 2 "mockito.kotlin"Repository: GetStream/stream-chat-android
Length of output: 4991
🏁 Script executed:
# Check all imports related to mockito in the test file
grep -n "import.*mockito\|import.*org.mockito" stream-chat-android-ui-components/src/test/kotlin/io/getstream/chat/android/ui/viewmodels/search/SearchViewModelTest.ktRepository: GetStream/stream-chat-android
Length of output: 401
🏁 Script executed:
# Check if nullableArgumentCaptor is used anywhere in the ui-components module tests
rg "nullableArgumentCaptor" stream-chat-android-ui-components/Repository: GetStream/stream-chat-android
Length of output: 55
Use nullableArgumentCaptor<String>() instead of argumentCaptor<String>() to properly capture nullable parameters.
The next parameter can be null, but argumentCaptor<String>() declares a non-nullable String type, which may fail to properly match and capture null values in Kotlin due to null-safety checks. Change line 115 to val captor = nullableArgumentCaptor<String>() or val captor = argumentCaptor<String?>() to correctly handle the nullable next parameter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@stream-chat-android-ui-components/src/test/kotlin/io/getstream/chat/android/ui/viewmodels/search/SearchViewModelTest.kt`
around lines 115 - 125, The test uses a non-null argumentCaptor for the nullable
`next` parameter, causing incorrect capture of null values; update the captor
declaration in SearchViewModelTest (the `captor` used in the verify of
`chatClient.searchMessages`) to a nullable captor by using
`nullableArgumentCaptor<String>()` or `argumentCaptor<String?>()` so the `next`
parameter can properly capture null and non-null values for the assertions.
1 participant
Goal
Allow consumers to configure the sort order of message search results in the channel list. Previously, the sort order was always the server-side default with no way to customize it.
Review after: #6179 and #6180
Docs PR: https://github.com/GetStream/docs-content/pull/1038
Implementation
messageSearchSort: QuerySorter<Message>?parameter toChannelListViewModel(defaults tonull, which uses the server-side default)ChannelViewModelFactory, which passes it through toChannelListViewModelsearchMessagescall insideChannelListViewModelUI Changes
search-sort-asc.mp4
search-sort-desc.mp4
Testing
QuerySorter<Message>viaChannelViewModelFactoryand verify message search results respect the provided sort order (ex.messageSearchSort = QuerySortByField.ascByName("created_at"))null(default) and verify behavior is unchanged from beforeSummary by CodeRabbit
Release Notes
New Features
Tests