[PBE-3749] Implement ThreadList state and UI #5441
Conversation
| @CheckResult | ||
| @InternalStreamChatApi | ||
| public fun queryThreadsResult(query: QueryThreadsRequest): Call<QueryThreadsResult> { | ||
| return queryThreadsInternal(query) |
There was a problem hiding this comment.
We don't need this extra method, we can call the API directly from here
| return queryThreadsInternal(query) | |
| return api.queryThreads(query) |
| val cid: String, | ||
| val channelInfo: ChannelInfo, | ||
| val channel: Channel?, |
There was a problem hiding this comment.
Can't we keep ChannelInfo instance here?
Instead of having a nullable Channel?
There was a problem hiding this comment.
As I was able to see from the API responses, we are getting a full Channel object, but again, it is not marked as mandatory in the API specs. Should we still keep this as a mandatory ChannelInfo? (The Channel object is later used to resolve the channel name in ThreadItem via the channelNameFormatter)
| val parentMessageId: String, | ||
| val parentMessage: Message, | ||
| val parentMessage: Message?, |
There was a problem hiding this comment.
A thread should always start with a parentMessage, it shouldn't be null
There was a problem hiding this comment.
Here I followed the swagger api spec where the parent_message field was not marked as mandatory. But you are right, this will probably never be null, I will update this.
| val createdBy: User?, | ||
| val replyCount: Int?, | ||
| val participantCount: Int?, | ||
| val threadParticipants: List<User>?, |
There was a problem hiding this comment.
Our domain model should avoid nullability as far as possible.
On the case of collections, we should fallback to emptyCollections instead.
| val replyCount: Int?, | ||
| val participantCount: Int?, |
There was a problem hiding this comment.
Is backend sending us those properties as null??
They shouldn't
There was a problem hiding this comment.
Same as the 'parent_message' param, but I will revert this change, as it doesn't make sense to get nulls here.
| val oldThreads = getThreads() | ||
| val oldThread = oldThreads.find { it.parentMessageId == parent.id } | ||
| oldThread ?: return false // no matching parent message was found | ||
| val newThread = oldThread.copy( | ||
| parentMessage = parent, | ||
| deletedAt = parent.deletedAt, | ||
| updatedAt = parent.updatedAt, | ||
| replyCount = parent.replyCount, | ||
| ) | ||
| val newThreads = oldThreads.map { | ||
| if (it.parentMessageId == newThread.parentMessageId) { | ||
| newThread | ||
| } else { | ||
| it | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of performing a search and after mapping all elements, what about only mapping elements?
The copy() method can be invoke within the map lambda
| val thread = oldThreads.find { it.parentMessageId == reply.parentId } | ||
| thread ?: return | ||
| val oldReplies = thread.latestReplies | ||
| val newReplies = if (oldReplies.any { it.id == reply.id }) { | ||
| // update | ||
| oldReplies.map { | ||
| if (it.id == reply.id) { | ||
| reply | ||
| } else { | ||
| it | ||
| } | ||
| } | ||
| } else { | ||
| // insert | ||
| oldReplies + listOf(reply) | ||
| } | ||
| val sortedNewReplies = newReplies.sortedBy { | ||
| it.createdAt ?: it.createdLocallyAt | ||
| } | ||
| val newThread = thread.copy(latestReplies = sortedNewReplies) | ||
| val newThreads = oldThreads.map { | ||
| if (it.parentMessageId == newThread.parentMessageId) { | ||
| newThread | ||
| } else { | ||
| it | ||
| } | ||
| } |
| private companion object { | ||
| private val INITIAL_STATE = ThreadListState( | ||
| threads = emptyList(), | ||
| isLoading = true, | ||
| isLoadingMore = false, | ||
| unseenThreadsCount = 0, | ||
| ) | ||
| private const val QUERY_LIMIT = 25 | ||
| private const val QUERY_REPLY_LIMIT = 10 | ||
| private const val QUERY_PARTICIPANT_LIMIT = 10 | ||
| } |
There was a problem hiding this comment.
Could we move companion object to the end of the class?
| private const val QUERY_LIMIT = 25 | ||
| private const val QUERY_REPLY_LIMIT = 10 | ||
| private const val QUERY_PARTICIPANT_LIMIT = 10 |
There was a problem hiding this comment.
Those constants should be the default ones, but allowing our customers to inject their own one by the constructor
| public val state: StateFlow<ThreadListState> | ||
| get() = _state | ||
|
|
||
| private val scope = CoroutineScope(DispatcherProvider.Main + SupervisorJob()) |
There was a problem hiding this comment.
We shouldn't create scopes internally in our classes.
It should be injected.
On that case I think we should use the VM scope
# Conflicts: # stream-chat-android-compose/src/main/res/values/strings.xml
# Conflicts: # stream-chat-android-compose/src/main/res/values/strings.xml
🎯 Goal
This PR is the first request related to the Threads V2 feature. It covers the
ThreadListcomponent, and the logic fetching the data behind it.🛠 Implementation details
QueryThreadsRequestto accept additional params as per the documentationQueryThreadsResponseDtoto include theprevandnextcursorsChatApi.queryThreadsmethod to returnQueryThreadsResultinstead ofList<Thread>(now includes theprevandnextcursors)DownstreamThreadDtoto include some additional fields delivered from back-end, and make some of the fields optional (as per the endpoint documentation)Threadmodel based on the changes ofDownstreamThreadDtoChatEventchanges:NotificationThreadMessageNewEventDtoevent of type:notification.thread_message_new. This event is now delivered when a new reply is added to a thread.NotificationThreadMessageNewevent based onNotificationThreadMessageNewEventDtoChannelLogicStatePluginchanges:QueryThreadsListenerfor applying side effects to theChatClient.queryThreadsoperation.QueryThreadsListenerState- Implementation of theQueryThreadsListenerapplying side effects as part of theStatePluginQueryThreadsLogic-> single point of entry for the new threads-related logic. Handles the logic of updating the threads-related state caused by theChatClient.queryThreadsand byChatEvents. (Accessible via theLogicRegistry)QueryThreadsState/QueryThreadsMutableState-> state holder for the new threads-related data (Accessible via theStateRegistry).QueryThreadsStateLogic-> interactor between theQueryThreadsLogicand theQueryThreadsMutableStateChatClient.queryThreadsAsState-> extension which initializes and exposes the 'global'QueryThreadsStateThreadListController-> a controller to be shared by theXMLandComposeSDKs which handles theloadandloadMorerequests for the thread list, and exposes a reactive render-ableThreadListState.ThreadListViewModelbacked by theThreadListControllerThreadListbacked byThreadListViewModelwhich shows the list of threads, and the optional unread threads banner (with default thread item, unread threads banner and default loading and empty states)🎨 UI Changes
🧪 Testing
This is currently not testable with the sample app (I will commit the updates to the sample app once this PR is approved)
Provide a patch below if it is necessary for testing
Provide the patch summary here
☑️Contributor Checklist
General
developbranchCode & documentation
☑️Reviewer Checklist
🎉 GIF
Please provide a suitable gif that describes your work on this pull request