Add functionality to check group members and show relevant content on PlacesList screen. #131
Conversation
WalkthroughThe pull request introduces several enhancements across multiple files, primarily focusing on managing and displaying user interface components related to member presence in a space. A new composable function, Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (5)
app/src/main/java/com/canopas/yourspace/ui/component/NoMemberEmptyContent.kt (2)
23-29: Add parameter documentation and preview composable.The implementation looks good! Consider these enhancements:
- Add KDoc to document parameters
- Add a preview composable for testing different states
+/** + * A composable that displays content when there are no members in a group + * + * @param loadingInviteCode Boolean indicating if invite code is being loaded + * @param title Resource ID for the title text + * @param subtitle Resource ID for the subtitle text + * @param addMember Callback invoked when add member button is clicked + */ @Composable fun NoMemberEmptyContent( loadingInviteCode: Boolean, title: Int, subtitle: Int, addMember: () -> Unit ) +@Preview(showBackground = true) +@Composable +private fun PreviewNoMemberEmptyContent() { + NoMemberEmptyContent( + loadingInviteCode = false, + title = R.string.place_list_screen_no_members_title, + subtitle = R.string.place_list_screen_no_members_subtitle, + addMember = {} + ) +}
37-42: Add content description for accessibility.The icon should have a meaningful content description for accessibility.
Icon( painter = painterResource(id = R.drawable.ic_thread_no_member), - contentDescription = null, + contentDescription = stringResource(id = R.string.content_description_no_members_icon), modifier = Modifier.size(64.dp), tint = AppTheme.colorScheme.textPrimary )app/src/main/java/com/canopas/yourspace/ui/flow/geofence/places/PlacesListViewModel.kt (1)
44-61: Add retry mechanism for space fetch.The implementation looks good, but consider adding retry logic for better reliability.
private fun getCurrentSpace() = viewModelScope.launch(appDispatcher.IO) { + var retryCount = 0 + val maxRetries = 3 + + while (retryCount < maxRetries) { try { _state.emit(_state.value.copy(loadingSpace = true)) val space = spaceRepository.getCurrentSpaceInfo() val members = space?.members ?: emptyList() _state.emit( _state.value.copy( currentSpace = space, loadingSpace = false, hasMembers = members.size > 1 ) ) + return@launch } catch (e: Exception) { Timber.e(e, "Failed to fetch current space") + retryCount++ + if (retryCount == maxRetries) { _state.emit(_state.value.copy(error = e.message, loadingSpace = false)) + } else { + delay(1000L * retryCount) + } } } }app/src/main/java/com/canopas/yourspace/ui/flow/geofence/places/PlacesListScreen.kt (2)
154-163: Make LoadingContent more reusable.Consider making the loading content more flexible for reuse across the app.
+/** + * A reusable loading indicator that can be customized with size and colors + * + * @param modifier Modifier for the container + * @param size Size of the progress indicator + * @param color Color of the progress indicator + */ @Composable -fun LoadingContent(modifier: Modifier) { +fun LoadingContent( + modifier: Modifier = Modifier, + size: Dp = 48.dp, + color: Color = AppTheme.colorScheme.primary +) { Column( horizontalAlignment = Alignment.CenterHorizontally, verticalArrangement = Arrangement.Center, modifier = modifier.fillMaxSize() ) { - AppProgressIndicator() + AppProgressIndicator( + modifier = Modifier.size(size), + color = color + ) } }
Line range hint
89-99: Extract visibility condition to a function.The floating action button visibility logic could be clearer if extracted to a function.
+private fun shouldShowAddPlaceButton( + placesLoading: Boolean, + connectivityStatus: ConnectivityObserver.Status, + hasMembers: Boolean +): Boolean = !placesLoading && + connectivityStatus == ConnectivityObserver.Status.Available && + hasMembers -if (!state.placesLoading && state.connectivityStatus == ConnectivityObserver.Status.Available && state.hasMembers) { +if (shouldShowAddPlaceButton( + state.placesLoading, + state.connectivityStatus, + state.hasMembers +)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/src/main/java/com/canopas/yourspace/ui/component/NoMemberEmptyContent.kt(1 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/geofence/places/PlacesListScreen.kt(5 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/geofence/places/PlacesListViewModel.kt(3 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/messages/thread/ThreadsScreen.kt(2 hunks)app/src/main/res/values/strings.xml(1 hunks)
🔇 Additional comments (1)
app/src/main/java/com/canopas/yourspace/ui/flow/messages/thread/ThreadsScreen.kt (1)
163-167: LGTM! Consistent usage of NoMemberEmptyContent
The component is appropriately used to display a message when there are no members, with proper title and subtitle parameters.
app/src/main/java/com/canopas/yourspace/ui/flow/geofence/places/PlacesListViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/placename/ChoosePlaceNameViewModel.kt (1)
Line range hint
76-90: Implement single-member group validationPer the PR objectives, places should not be added when a group has only one member. However, this validation is missing in the current implementation.
Here's a suggested implementation:
val memberIds = spaceRepository.getMemberBySpaceId(currentSpaceId)?.map { it.user_id } ?: emptyList() + if (memberIds.size <= 1) { + _state.value = _state.value.copy( + error = Exception("Cannot add places in groups with only one member.") + ) + return@launch + } apiPlaceService.addPlace( spaceId = currentSpaceId, name = state.value.placeName,app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/placename/ChoosePlaceNameScreen.kt (2)
Line range hint
143-146: Add member count validation before enabling place creationAccording to the PR objectives, places should not be added when a group has only one member. However, the "Add Place" button is currently enabled based only on the place name being non-empty.
Consider updating the button's enabled state to include the member count check:
PrimaryButton( label = stringResource(R.string.common_btn_add_place), modifier = Modifier.align(Alignment.CenterHorizontally), onClick = { onNext() keyboardController?.hide() }, - enabled = placeName.trim().isNotEmpty(), + enabled = placeName.trim().isNotEmpty() && state.memberCount > 1, showLoader = showLoader )This will require:
- Adding
memberCountto the screen's state- Updating the ViewModel to track member count
Line range hint
119-121: Add user guidance about member requirementsUsers should be informed about the requirement of having multiple members before adding a place. Consider adding explanatory text near the suggestions.
Add informative text below the existing caption:
Text( text = stringResource(R.string.home_create_space_suggestions), style = AppTheme.appTypography.caption.copy(color = AppTheme.colorScheme.textDisabled), modifier = Modifier .fillMaxWidth() .padding(horizontal = 16.dp) ) + Text( + text = stringResource(R.string.place_creation_member_requirement), + style = AppTheme.appTypography.caption.copy(color = AppTheme.colorScheme.textDisabled), + modifier = Modifier + .fillMaxWidth() + .padding(horizontal = 16.dp, top = 4.dp) + )This will require adding a new string resource:
<string name="place_creation_member_requirement">Note: Places can only be added when the group has more than one member</string>app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/locate/LocateOnMapScreen.kt (2)
150-154: Consider using Kotlin's safe call operatorThe error handling implementation can be more concise using Kotlin's safe call operator.
-if (state.error != null) { - AppBanner(msg = state.error!!.message!!) { - viewModel.resetErrorState() - } -} +state.error?.message?.let { errorMessage -> + AppBanner(msg = errorMessage) { + viewModel.resetErrorState() + } +}
Based on the code analysis, I can now provide the final response:
Member validation check is missing in place addition flow
The place addition implementation in both
LocateOnMapViewModelandChoosePlaceNameViewModellacks the required validation to prevent place addition in single-member groups. WhilePlacesListViewModeltracks member count withhasMembersstate, this validation isn't enforced in the actual place addition flow.Required changes:
- Add member count validation in
LocateOnMapViewModel.onNextClick()before proceeding with place addition- Implement similar validation in
ChoosePlaceNameViewModel.addPlace()- Consider moving the validation logic to a shared location since it's used in multiple places
🔗 Analysis chain
Line range hint
80-85: Verify implementation of group member check requirementLet's verify if the codebase has any other implementations of the group member check, as the current changes don't fully address the PR's main objective.
Let me gather more specific information about place addition validation.
Let me check the place addition implementation in ChoosePlaceNameViewModel since that's where the actual place addition logic resides.
Let me check the LocateOnMapViewModel since it's the entry point for place addition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other implementations of member count validation rg -l "memberIds?.size|members?.size|count.*member|member.*count" # Search for other place addition implementations that might need the same validation ast-grep --pattern 'fun $name($$$) { $$$ addPlace($$$) $$$ }'Length of output: 568
Script:
#!/bin/bash # Check PlacesListViewModel for member validation logic cat "app/src/main/java/com/canopas/yourspace/ui/flow/geofence/places/PlacesListViewModel.kt" # Search for place addition related functions in the codebase rg -g "*.kt" "fun.*addPlace|fun.*savePlace" -A 5 # Look for group member validation patterns rg -g "*.kt" "if.*member.*size|when.*member.*size" -A 3Length of output: 12992
Script:
#!/bin/bash # Check ChoosePlaceNameViewModel for the complete implementation cat "app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/placename/ChoosePlaceNameViewModel.kt" # Also check if there are any member validation checks in this path rg -g "*/geofence/add/**/*.kt" "member.*size|hasMembers"Length of output: 5317
Script:
#!/bin/bash # Check LocateOnMapViewModel implementation cat "app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/locate/LocateOnMapViewModel.kt" # Check if there are any member validation checks in this specific file rg -g "LocateOnMapViewModel.kt" "member.*size|hasMembers|members"Length of output: 5832
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/locate/LocateOnMapScreen.kt(2 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/locate/LocateOnMapViewModel.kt(2 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/placename/ChoosePlaceNameScreen.kt(1 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/placename/ChoosePlaceNameViewModel.kt(1 hunks)
🔇 Additional comments (2)
app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/locate/LocateOnMapViewModel.kt (1)
142-144: LGTM: Error state reset implementation
The error state reset implementation follows proper state management practices.
app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/locate/LocateOnMapScreen.kt (1)
39-39: LGTM: AppBanner import
The import is correctly added for the new error handling UI component.
...c/main/java/com/canopas/yourspace/ui/flow/geofence/add/placename/ChoosePlaceNameViewModel.kt
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/placename/ChoosePlaceNameScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/flow/geofence/add/locate/LocateOnMapViewModel.kt
Show resolved
Hide resolved
…e-member-groups # Conflicts: # app/src/main/res/values/strings.xml
Changelog
Implemented a check to prevent adding places in a group with only one member. This change ensures that place management is only possible when there are multiple members in the group.
New Features
Enhancements
Screen_recording_20241209_123116.mp4
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation