Add AddMembersBottomSheet component to the GroupChannelInfoScreen#6276
Open
VelikovPetar wants to merge 6 commits intov7from
Open
Add AddMembersBottomSheet component to the GroupChannelInfoScreen#6276VelikovPetar wants to merge 6 commits intov7from
AddMembersBottomSheet component to the GroupChannelInfoScreen#6276VelikovPetar wants to merge 6 commits intov7from
Conversation
Contributor
PR checklist ✅All required conditions are satisfied:
🎉 Great job! This PR is ready for review. |
Contributor
SDK Size Comparison 📏
|
VelikovPetar
commented
Mar 20, 2026
| currentUser: User? = ChatClient.instance().getCurrentUser(), | ||
| onNavigationIconClick: () -> Unit = {}, | ||
| @Suppress("UNUSED_PARAMETER") | ||
| onAddMembersClick: () -> Unit = {}, |
Contributor
Author
There was a problem hiding this comment.
Point to discuss: Do we remove this completely, or allow customers to override it?
They can now override the whole add members, so I am not sure if it makes sense to keep it.
Contributor
There was a problem hiding this comment.
I'd remove it given that we don't call it anymore, it seems misleading.
| /** | ||
| * @see [ChannelInfoViewController.addMembers] | ||
| */ | ||
| public fun addMembers(userIds: Set<String>) { |
Contributor
Author
There was a problem hiding this comment.
Delegated the addMembers call from the AddMembersViewModel to here: The idea is to keep the AddMembers simpler, and if customised, the integrators don't need to worry about calling the addMembers operation manually, they can just call params.onConfirm.
This is also aligned with the SwiftUI API, but I am open for discussion.
(it is also NOT defined as a ChannelInfoViewAction because I believe it is a separate type of action).
|
WalkthroughThis pull request relocates the "Add Members" feature from the sample app into the main Compose library, converting the dialog-based UI to a bottom sheet, and establishing a layered architecture with a shared business logic controller in the ui-common module. Changes
Sequence DiagramsequenceDiagram
participant User
participant BottomSheet as Bottom Sheet UI
participant ViewModel as AddMembersViewModel
participant Controller as AddMembersViewController
participant ChatClient
participant ChannelState
User->>BottomSheet: Type search query
BottomSheet->>ViewModel: QueryChanged(query)
ViewModel->>Controller: onViewAction(QueryChanged)
activate Controller
Controller->>ChatClient: queryUsers(request)
ChatClient-->>Controller: searchResult
Controller->>Controller: Update state (isLoading=false)
deactivate Controller
Controller-->>ViewModel: state updated
ViewModel-->>BottomSheet: state emitted
BottomSheet->>User: Show search results
User->>BottomSheet: Click user to select
BottomSheet->>ViewModel: UserClick(user)
ViewModel->>Controller: onViewAction(UserClick)
activate Controller
Controller->>Controller: Toggle selectedUserIds
deactivate Controller
Controller-->>ViewModel: state updated
ViewModel-->>BottomSheet: state emitted
BottomSheet->>User: Show selection indicator
User->>BottomSheet: Scroll to bottom
BottomSheet->>ViewModel: LoadMore
ViewModel->>Controller: onViewAction(LoadMore)
activate Controller
Controller->>ChatClient: queryUsers(offset=current size)
ChatClient-->>Controller: nextPage results
Controller->>Controller: Append to searchResult
deactivate Controller
Controller-->>ViewModel: state updated
ViewModel-->>BottomSheet: state emitted
BottomSheet->>User: Show appended results
User->>BottomSheet: Confirm selection
BottomSheet->>ViewModel: Confirm button clicked
ViewModel->>ChannelState: infoViewModel.addMembers(selectedUserIds)
activate ChannelState
ChannelState->>ChatClient: addMembers(AddMembersParams)
ChatClient-->>ChannelState: Success
deactivate ChannelState
ChannelState-->>BottomSheet: onConfirm callback
BottomSheet->>User: Dismiss & show updated member list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🧹 Nitpick comments (1)
stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/viewmodel/channel/AddMembersViewModelTest.kt (1)
65-74: Missing mock setup for state flow in action delegation tests.The action delegation tests (lines 65-95) call
fixture.get()without first callinggivenControllerStateFlow(). This could cause issues if accessingmockController.stateis needed during ViewModel initialization or if the mock returns null by default.Consider adding a default state flow setup in the Fixture or ensuring all tests that create the ViewModel also set up the state flow:
♻️ Proposed fix
private class Fixture { private val mockController: AddMembersViewController = mock() + init { + // Provide a default state flow to avoid null issues + whenever(mockController.state) doReturn MutableStateFlow(AddMembersViewState()) + } + fun givenControllerStateFlow(stateFlow: MutableStateFlow<AddMembersViewState>) = apply { whenever(mockController.state) doReturn stateFlow }🤖 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/channel/AddMembersViewModelTest.kt` around lines 65 - 74, The test creates the ViewModel via Fixture.get() without initializing the mocked controller's state Flow, which can return null or cause initialization failures; update the Fixture to provide a default state Flow by calling or invoking givenControllerStateFlow() inside Fixture.get() (or ensure every test calls fixture.givenControllerStateFlow() before fixture.get()), so mockController.state returns a non-null Flow during AddMembersViewModel construction and methods like onViewAction/verifyControllerOnViewAction work reliably.
🤖 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-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/feature/channel/info/ChannelInfoViewController.kt`:
- Around line 238-240: The addMembers onError currently only logs the error;
update the addMembers error handler to emit a
ChannelInfoViewEvent.AddMembersError (using the same event emission mechanism
used elsewhere in this controller, e.g., the existing emit/postEvent/eventSink
calls) and include the error details so the UI can react; also add the
AddMembersError variant to the ChannelInfoViewEvent sealed class/interface to
match other operations like renameChannel, setChannelMute, and banMember.
---
Nitpick comments:
In
`@stream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/viewmodel/channel/AddMembersViewModelTest.kt`:
- Around line 65-74: The test creates the ViewModel via Fixture.get() without
initializing the mocked controller's state Flow, which can return null or cause
initialization failures; update the Fixture to provide a default state Flow by
calling or invoking givenControllerStateFlow() inside Fixture.get() (or ensure
every test calls fixture.givenControllerStateFlow() before fixture.get()), so
mockController.state returns a non-null Flow during AddMembersViewModel
construction and methods like onViewAction/verifyControllerOnViewAction work
reliably.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e1b60bb5-86a5-4a25-a0d7-890f600bfaad
⛔ Files ignored due to path filters (14)
stream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_empty.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_empty_in_dark_mode.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_loading.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_loading_in_dark_mode.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_loading_more.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_loading_more_in_dark_mode.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_results.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_results_in_dark_mode.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_results_with_existing_member.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_results_with_existing_member_in_dark_mode.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_results_with_query.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_results_with_query_in_dark_mode.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_results_with_selection.pngis excluded by!**/*.pngstream-chat-android-compose/src/test/snapshots/images/io.getstream.chat.android.compose.ui.channel.info_AddMembersBottomSheetTest_results_with_selection_in_dark_mode.pngis excluded by!**/*.png
📒 Files selected for processing (23)
stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/channel/AddMembersDialog.ktstream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/channel/AddMembersViewModel.ktstream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/channel/GroupChannelInfoActivity.ktstream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/chats/ChatsActivity.ktstream-chat-android-compose/api/stream-chat-android-compose.apistream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/channel/info/AddMembersBottomSheet.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/channel/info/GroupChannelInfoScreen.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactory.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/theme/ChatComponentFactoryParams.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channel/AddMembersViewModel.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channel/ChannelInfoViewModel.ktstream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/viewmodel/channel/ChannelInfoViewModelFactory.ktstream-chat-android-compose/src/main/res/values/strings.xmlstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/ui/channel/info/AddMembersBottomSheetTest.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/viewmodel/channel/AddMembersViewModelTest.ktstream-chat-android-compose/src/test/kotlin/io/getstream/chat/android/compose/viewmodel/channel/ChannelInfoViewModelFactoryTest.ktstream-chat-android-previewdata/src/main/kotlin/io/getstream/chat/android/previewdata/PreviewUserData.ktstream-chat-android-ui-common/api/stream-chat-android-ui-common.apistream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/feature/channel/info/AddMembersViewAction.ktstream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/feature/channel/info/AddMembersViewController.ktstream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/feature/channel/info/ChannelInfoViewController.ktstream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/state/channel/info/AddMembersViewState.ktstream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/feature/channel/info/AddMembersViewControllerTest.kt
💤 Files with no reviewable changes (4)
- stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/chats/ChatsActivity.kt
- stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/channel/AddMembersDialog.kt
- stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/channel/GroupChannelInfoActivity.kt
- stream-chat-android-compose-sample/src/main/java/io/getstream/chat/android/compose/sample/ui/channel/AddMembersViewModel.kt
👮 Files not reviewed due to content moderation or server errors (5)
- stream-chat-android-ui-common/api/stream-chat-android-ui-common.api
- stream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/state/channel/info/AddMembersViewState.kt
- stream-chat-android-compose/src/main/java/io/getstream/chat/android/compose/ui/channel/info/AddMembersBottomSheet.kt
- stream-chat-android-ui-common/src/test/kotlin/io/getstream/chat/android/ui/common/feature/channel/info/AddMembersViewControllerTest.kt
- stream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/feature/channel/info/AddMembersViewController.kt
Comment on lines
+238
to
+240
| .onError { error -> | ||
| logger.e { "[addMembers] error: ${error.message}" } | ||
| } |
There was a problem hiding this comment.
Consider emitting an error event on failure.
Unlike other operations in this controller (e.g., renameChannel, setChannelMute, banMember), addMembers only logs the error without emitting a ChannelInfoViewEvent. This means the UI cannot inform the user when adding members fails.
For consistency and better UX, consider adding an AddMembersError event:
Proposed fix
.onError { error ->
logger.e { "[addMembers] error: ${error.message}" }
+ _events.tryEmit(ChannelInfoViewEvent.AddMembersError)
}You'll also need to add the corresponding event to ChannelInfoViewEvent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@stream-chat-android-ui-common/src/main/kotlin/io/getstream/chat/android/ui/common/feature/channel/info/ChannelInfoViewController.kt`
around lines 238 - 240, The addMembers onError currently only logs the error;
update the addMembers error handler to emit a
ChannelInfoViewEvent.AddMembersError (using the same event emission mechanism
used elsewhere in this controller, e.g., the existing emit/postEvent/eventSink
calls) and include the error details so the UI can react; also add the
AddMembersError variant to the ChannelInfoViewEvent sealed class/interface to
match other operations like renameChannel, setChannelMute, and banMember.
gpunto
reviewed
Mar 20, 2026
Contributor
gpunto
left a comment
gpunto
left a comment
There was a problem hiding this comment.
Looks great, just left a few comments
| /** | ||
| * The list of users selected to be added as members, derived from [searchResult] and [selectedUserIds]. | ||
| */ | ||
| public val selectedUsers: List<User> get() = searchResult.filter { it.id in selectedUserIds } |
Contributor
There was a problem hiding this comment.
Here we're dropping all selected users that are not in the current search results. This can happen for example if I do query1 -> select users -> query2 -> select more users. Is that intended?
If not, I see a couple of options: store users instead of ids (maybe a map from ID to User), or just propagate the set of IDs instead of a list of users.
|
|
||
| private fun searchUsers(query: String) { | ||
| scope.launch { | ||
| _state.update { it.copy(isLoading = true) } |
Contributor
There was a problem hiding this comment.
Should we cancel ongoing calls here? I think atm we are exposed to a race condition where it could happen something like this:
- we start loading some page (or some search)
- the user perform another search
- we get first results from 2 and then from 1
We could store a reference to the current operation and cancel it before doing any other?
| } | ||
|
|
||
| private fun loadMore() { | ||
| if (_state.value.isLoadingMore) return |
Contributor
There was a problem hiding this comment.
Should we also check isLoading or is that an invalid scenario?
| init { | ||
| // Keep loadedMemberIds in sync with the live channel member list. | ||
| channelMembers | ||
| .map { members -> members.map(Member::getUserId).toSet() } |
Contributor
There was a problem hiding this comment.
nit: just to avoid an allocation
Suggested change
| .map { members -> members.map(Member::getUserId).toSet() } | |
| .map { members -> members.mapTo(mutableSetOf(), Member::getUserId) } |
| public fun onViewAction(action: AddMembersViewAction) { | ||
| when (action) { | ||
| is AddMembersViewAction.QueryChanged -> { | ||
| _state.update { it.copy(query = action.query.trim()) } |
Contributor
There was a problem hiding this comment.
Should we move this trim elsewhere? Given that this is observed by the ui, if I type a space, it will be immediately removed. Maybe we could put it above, in the listener in the init block, like .map { it.query.trim() }
| bottom = StreamTokens.spacing3xl, | ||
| ), | ||
| ) { | ||
| itemsIndexed( |
Contributor
There was a problem hiding this comment.
Could this be items? It seems we're not using the index 🤔
| verticalAlignment = Alignment.CenterVertically, | ||
| horizontalArrangement = Arrangement.spacedBy(StreamTokens.spacingSm), | ||
| ) { | ||
| UserAvatar( |
Contributor
There was a problem hiding this comment.
Should we pass through ChatComponentFactory?
| currentUser: User? = ChatClient.instance().getCurrentUser(), | ||
| onNavigationIconClick: () -> Unit = {}, | ||
| @Suppress("UNUSED_PARAMETER") | ||
| onAddMembersClick: () -> Unit = {}, |
Contributor
There was a problem hiding this comment.
I'd remove it given that we don't call it anymore, it seems misleading.
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
Adds the new
AddMembersBottomSheetcomposable to the Compose UI SDK.Implementation
AddMembersBottomSheetcomposable (+ VM / Controller)ChatComponentFactory🎨 UI Changes
See snapshots.
Testing
Add membersSummary by CodeRabbit
Release Notes