Merged
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
|
DB Entities have been updated. Do we need to upgrade DB Version? |
Contributor
SDK Size Comparison 📏
|
gpunto
approved these changes
Feb 12, 2026
# Conflicts: # stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/offline/repository/database/converter/internal/ListConverter.kt # stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/offline/repository/domain/threads/internal/ThreadParticipantEntity.kt # stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/offline/Mother.kt # stream-chat-android-offline/src/main/java/io/getstream/chat/android/offline/repository/database/internal/ChatDatabase.kt
WalkthroughThis PR introduces ThreadUpdatedEvent to handle thread update notifications, consolidates thread query APIs by removing queryThreadsResult() in favor of queryThreads(), changes partialUpdateThread return types from Thread to ThreadInfo, and restructures thread-related DTOs to include richer participant and channel context while bumping the database schema version. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 (5)
stream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/feature/threads/ThreadListControllerTest.kt (1)
80-80:⚠️ Potential issue | 🟡 MinorTest name references the old API method name.
The test name still says
queryThreadsResultbut the method under test is nowqueryThreads. Same applies to the test on Line 155. Consider updating both test names for consistency.Suggested fix
Line 80:
- fun `load calls queryThreadsResult with correct query`() = runTest { + fun `load calls queryThreads with correct query`() = runTest {Line 155:
- fun `loadNextPage calls queryThreadsResult with next page query if shouldLoadNextPage returns true`() = runTest { + fun `loadNextPage calls queryThreads with next page query if shouldLoadNextPage returns true`() = runTest {stream-chat-android-client/src/test/java/io/getstream/chat/android/client/ChatClientThreadsApiTest.kt (1)
47-85:⚠️ Potential issue | 🟡 MinorUse backtick test names in src/test (e.g., queryThreadsError).
Rename the updated test to the backtick style to match the guideline.
♻️ Suggested rename
-fun queryThreadsError() = runTest { +fun `query threads error`() = runTest {As per coding guidelines:
**/src/test/**/*.kt: Use backtick test names (for example:funmessage list filters muted channels()) for readabilitystream-chat-android-client/src/main/java/io/getstream/chat/android/client/ChatClient.kt (2)
4518-4535:⚠️ Potential issue | 🟡 MinorKDoc should reflect
QueryThreadsResult+ pagination.The doc still says “matching users” and doesn’t mention the paginated result type.
✏️ Suggested KDoc update
- * `@param` query [QueryThreadsRequest] with query parameters to get matching users. + * `@param` query [QueryThreadsRequest] with query parameters to get matching threads. + * + * `@return` Executable async [Call] returning [QueryThreadsResult] (includes pagination data).As per coding guidelines, "Document public APIs with KDoc, including thread expectations and state notes".
4555-4574:⚠️ Potential issue | 🟡 MinorKDoc still references
Thread/message after the return type change.Update the doc to reflect
ThreadInfoand “thread” semantics.✏️ Suggested KDoc update
- * Partially updates specific [Thread] fields retaining the fields which were set previously. + * Partially updates specific [ThreadInfo] fields retaining the fields which were set previously. ... - * `@return` Executable async [Call] responsible for partially updating the message. + * `@return` Executable async [Call] responsible for partially updating the thread (returns base [ThreadInfo]).As per coding guidelines, "Document public APIs with KDoc, including thread expectations and state notes".
stream-chat-android-core/src/testFixtures/kotlin/io/getstream/chat/android/Mother.kt (1)
1010-1048:⚠️ Potential issue | 🟡 MinorAdd KDoc for the new/updated public thread helpers.
randomThreadInfonow exposeschannelandthreadParticipants, andrandomThreadParticipantis new; both should be documented with thread expectations/state notes.Suggested KDoc
+/** + * Creates a [ThreadInfo] for tests. + * + * `@param` channel Optional channel context for the thread. + * `@param` threadParticipants Participants included in the thread snapshot. + */ public fun randomThreadInfo( @@ +/** + * Creates a [ThreadParticipant] for the provided [user]. + */ public fun randomThreadParticipant(As per coding guidelines: Document public APIs with KDoc, including thread expectations and state notes.
🤖 Fix all issues with AI agents
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/mapping/DomainMappingTest.kt`:
- Line 743: Locate the `@Suppress`("LongMethod") annotation introduced in
DomainMappingTest (the suppression at line with `@Suppress`("LongMethod")) and
either remove it by refactoring the long test into smaller helper functions
(extract logical sections into private methods) or replace the suppression with
a short KDoc comment immediately above the annotation that documents why the
method must remain long (what complex behavior it covers and why extraction
isn't practical); if the suppression was intended for an experimental API
instead, use the appropriate `@OptIn` annotation for that API (naming the
experimental marker) rather than suppressing LongMethod.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/MoshiChatApiTest.kt`:
- Around line 2115-2116: Rename the test function testPartialUpdateThread in
MoshiChatApiTest to a backtick-style name for test readability/consistency
(e.g., fun `partial update thread`(call: RetrofitCall<ThreadInfoResponse>,
expected: KClass<*>) = runTest { ... }); locate the declaration by the symbol
testPartialUpdateThread and update its name while keeping the signature and body
unchanged so test references compile.
In
`@stream-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/MoshiChatApiTestArguments.kt`:
- Around line 797-808: In threadInfoResponseArguments(), the error case uses the
wrong generic type; replace RetroError<ThreadResponse> with
RetroError<ThreadInfoResponse> so the error result matches the success case and
the partialUpdateThread return type; update the Arguments.of(...) call that
constructs the error Retrofit call (the RetroError instantiation) to use
ThreadInfoResponse and keep the surrounding Result.Failure::class unchanged.
🧹 Nitpick comments (3)
stream-chat-android-client/src/main/java/io/getstream/chat/android/client/internal/offline/repository/database/internal/ChatDatabase.kt (1)
134-153: Pre-existing TOCTOU race ingetDatabase— not introduced by this PR, noting for awareness.The
containsKeycheck on line 135 is outsidesynchronized, so two concurrent callers for the sameuserIdcan both enter the block and create separate DB instances. The second overwrites the first, potentially leaking connections. This is pre-existing and out of scope for this PR, but worth a follow-up.♻️ Sketch of a safer pattern (double-checked locking)
fun getDatabase(context: Context, userId: String): ChatDatabase { - if (!INSTANCES.containsKey(userId)) { - synchronized(this) { + return INSTANCES[userId] ?: synchronized(this) { + INSTANCES[userId] ?: run { val db = Room.databaseBuilder( context.applicationContext, ChatDatabase::class.java, "stream_chat_database_$userId", ).fallbackToDestructiveMigration() .addCallback( object : Callback() { override fun onOpen(db: SupportSQLiteDatabase) { db.execSQL("PRAGMA synchronous = 1") } }, ) .build() INSTANCES[userId] = db + db } } - return INSTANCES[userId] ?: error("DB not created") }stream-chat-android-client/src/test/java/io/getstream/chat/android/client/extensions/internal/ThreadExtensionsTests.kt (1)
423-512: Test coverage is thorough and well-structured.The three tests correctly validate:
- Updatable fields (
title,updatedAt,extraData) are applied- Non-updatable fields remain unchanged
- Guard clause rejects mismatched
parentMessageIdMinor naming nit: the test method names reference
applyThreadInfoUpdate(e.g., Line 424, 454, 488) while the function under test isapplyThreadUpdatedEventChanges. Consider aligning the names for discoverability and grep-ability.Suggested rename
- fun `applyThreadInfoUpdate should update title extraData and updatedAt`() { + fun `applyThreadUpdatedEventChanges should update title extraData and updatedAt`() {- fun `applyThreadInfoUpdate should not update non-updatable fields`() { + fun `applyThreadUpdatedEventChanges should not update non-updatable fields`() {- fun `applyThreadInfoUpdate should not update when thread info parent message id does not match`() { + fun `applyThreadUpdatedEventChanges should not update when thread info parent message id does not match`() {stream-chat-android-client/src/test/java/io/getstream/chat/android/client/internal/offline/repository/domain/threads/internal/ThreadMapperTest.kt (1)
41-71: Strengthen participant mapping assertions with per-user lookups.Using a single
userfor all participants doesn’t verify that mapping respects participant IDs.♻️ Suggested test tightening
- val user = randomUser() + val createdBy = randomUser() val message = randomMessage() val channel = randomChannel() val entity = randomThreadEntity() + val userById = entity.threadParticipants + .associate { it.userId to randomUser(it.userId) } + .toMutableMap() + .apply { put(entity.createdByUserId, createdBy) } val expected = Thread( parentMessageId = entity.parentMessageId, parentMessage = message, cid = entity.cid, channel = channel, createdByUserId = entity.createdByUserId, - createdBy = user, + createdBy = createdBy, activeParticipantCount = entity.activeParticipantCount, participantCount = entity.participantCount, - threadParticipants = entity.threadParticipants.map { - ThreadParticipant( - user = user, - ) - }, + threadParticipants = entity.threadParticipants.map { participant -> + ThreadParticipant(user = userById.getValue(participant.userId)) + }, lastMessageAt = entity.lastMessageAt, createdAt = entity.createdAt, updatedAt = entity.updatedAt, deletedAt = entity.deletedAt, title = entity.title, read = entity.read.map { it.toModel { user } }, latestReplies = emptyList(), extraData = entity.extraData, draft = null, ) // When val model = entity.toModel( - getUser = { user }, + getUser = { userById.getValue(it) }, getMessage = { message }, getChannel = { channel }, getDraftMessage = { null }, )
...roid-client/src/test/java/io/getstream/chat/android/client/api2/mapping/DomainMappingTest.kt
Outdated
Show resolved
Hide resolved
...-chat-android-client/src/test/java/io/getstream/chat/android/client/api2/MoshiChatApiTest.kt
Show resolved
Hide resolved
...roid-client/src/test/java/io/getstream/chat/android/client/api2/MoshiChatApiTestArguments.kt
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal
Aligns the Threads public API with the API specs.
Implementation
ChatClient.queryThreadsResulttoChatClient.queryThreadsand delete the currentChatClient.queryThreads(it didn't support pagination)ChatClient.partialUpdateThreadfromThreadtoThreadInfo(it returns the base Thread data (ThreadInfo), not the enrichedThreaddata)thread.updatedevent and it's handlingThreadInfoQueryThreadsRequest🎨 UI Changes
NA
Testing
There are no visible changes that could be tested. However you can any of the following:
ChatClient.getThreadChatClient.partialUpdateThreadand verify it doesn't fail ANDthread.updatedevent is triggered and properly parsedSummary by CodeRabbit
New Features
Chores