Fix padding and add admin control for member location sharing #157
Conversation
WalkthroughThis pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
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: 3
🧹 Nitpick comments (2)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt (1)
95-109: Add loading state for member location updatesWhile the error handling is good, the function should manage loading state to provide better user feedback during the async operation.
Consider this enhancement:
fun updateMemberLocation(memberId: String, enableLocation: Boolean) { viewModelScope.launch(appDispatcher.IO) { try { + _state.emit(_state.value.copy(userLocationUpdatingId = memberId)) spaceRepository.enableLocation(spaceID, memberId, enableLocation) val spaceInfo = spaceRepository.getSpaceInfo(spaceID) _state.emit( _state.value.copy( spaceInfo = spaceInfo, - locationEnabledChanges = mapOf(memberId to enableLocation) + locationEnabledChanges = mapOf(memberId to enableLocation), + userLocationUpdatingId = null ) ) } catch (e: Exception) { Timber.e(e, "Failed to update member location") + _state.emit(_state.value.copy( + error = e, + userLocationUpdatingId = null + )) } } }app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileScreen.kt (1)
549-551: Consider adding haptic feedback for better user experience.The switch implementation could benefit from haptic feedback when toggling location settings.
onCheckedChange = { isChecked -> + HapticFeedback.performHapticFeedback(HapticFeedbackType.TextHandleMove) onCheckedChange(isChecked) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileScreen.kt(7 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt(4 hunks)app/src/test/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModelTest.kt(0 hunks)
💤 Files with no reviewable changes (1)
- app/src/test/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModelTest.kt
🔇 Additional comments (8)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt (3)
183-187: LGTM! Good state management after save operationThe implementation properly refreshes space info and resets states after saving.
327-331: LGTM! Well-structured state propertiesThe new state properties provide good separation of concerns and proper type safety.
82-82: Verify decoupling of location changes from save stateThe save state now only considers space name changes, ignoring location changes. While this aligns with separating location management, ensure this is intentional and won't prevent necessary location updates from being saved.
✅ Verification successful
Location changes are correctly decoupled from save state
The codebase shows that location updates are intentionally handled through dedicated methods (
enableLocation,updateMemberLocation) and don't need to be part of the general save operation. This separation is well-tested and maintains consistency across both user and admin-initiated location changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other location-related save operations rg -A 5 "enableLocation|locationEnabled.*save"Length of output: 13867
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileScreen.kt (5)
237-242: LGTM! Clean focus management implementation.The focus management implementation using
LocalFocusManagerandMutableInteractionSourcefollows Compose best practices.
250-296: Well-structured text field implementation with proper visual feedback.The BasicTextField implementation with dynamic outline color, keyboard actions, and cursor styling provides a better user experience. The check icon for saving is more intuitive than a top bar button.
Line range hint
350-363: LGTM! Clear implementation of user's location control.The implementation properly handles the current user's location sharing preferences with appropriate admin checks.
318-326: LGTM! Clear visual feedback for admin actions.The implementation properly shows loading state during invite code regeneration.
305-305: LGTM! Improved visual hierarchy with consistent padding.The padding adjustments for the invite code text and dividers create a better visual hierarchy.
Also applies to: 338-338, 344-344
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt (1)
82-82: Consider renaming variables for clarityIn the
onChange()function, bothspaceNameand_state.value.spaceNameare used, which can be confusing. To enhance readability, consider renamingspaceNametocurrentSpaceNameororiginalSpaceName.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt (1)
133-140: Error handling for invite code regeneration is well-implementedThe
regenerateInviteCodefunction now includes proper error handling and updates the loading state appropriately. This addresses previous concerns regarding error management in this function.
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt (1)
336-340: Add documentation for new state propertiesConsider adding KDoc comments to explain the purpose of the new state properties:
isCodeLoading: For invite code regeneration progresslocationEnabledChanges: For tracking member location setting changesisLocationSettingChange: For current user's location setting change progressuserLocationUpdatingId: For tracking which member's location is being updateddata class SpaceProfileState( // ... existing properties ... + /** Tracks the loading state during invite code regeneration */ val isCodeLoading: Boolean = false, + /** Maps member IDs to their updated location sharing settings */ val locationEnabledChanges: Map<String, Boolean> = emptyMap(), + /** Indicates if the current user's location setting is being updated */ val isLocationSettingChange: Boolean = false, + /** ID of the member whose location setting is being updated */ val userLocationUpdatingId: String? = null )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt(4 hunks)
🔇 Additional comments (3)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt (3)
134-141: LGTM! Good error handling and loading state management.The implementation properly manages loading states and handles errors for invite code regeneration.
192-196: LGTM! Good state management after save operation.The implementation properly updates the space info and handles the allowSave state in both success and error cases.
88-98: 🛠️ Refactor suggestionImprove state management in
onLocationEnabledChangedThe current implementation has potential issues:
- The state is updated before the API call, leading to UI inconsistency if the API fails.
- No loading state is managed during the operation.
Consider this implementation:
viewModelScope.launch { try { - _state.value = state.value.copy(locationEnabled = enable) + _state.value = state.value.copy(isLocationSettingChange = true) spaceRepository.enableLocation(spaceID, authService.currentUser?.id ?: "", enable) + _state.value = state.value.copy( + locationEnabled = enable, + isLocationSettingChange = false + ) } catch (e: Exception) { Timber.e(e, "Failed to update location") - _state.value = state.value.copy(error = e, locationEnabled = !enable) + _state.value = state.value.copy( + error = e, + isLocationSettingChange = false + ) } onChange() }Likely invalid or redundant comment.
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt
Outdated
Show resolved
Hide resolved
cp-megh-l
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt (2)
141-148: Add validation and timeout handling inregenerateInviteCodeThe function needs additional safeguards:
- No validation for spaceID
- No timeout handling for long-running operations
Consider this implementation:
fun regenerateInviteCode() = viewModelScope.launch(appDispatcher.IO) { + if (spaceID.isBlank()) { + _state.emit(_state.value.copy( + error = IllegalArgumentException("Space ID cannot be empty") + )) + return@launch + } _state.emit(_state.value.copy(isCodeLoading = true)) try { + withTimeout(TimeUnit.SECONDS.toMillis(30)) { spaceRepository.regenerateInviteCode(spaceRepository.currentSpaceId) fetchInviteCode(spaceID) + } _state.emit(_state.value.copy(isCodeLoading = false)) } catch (e: Exception) { Timber.e(e, "Failed to regenerate invite code") _state.emit(_state.value.copy(isCodeLoading = false, error = e)) } }
343-347: Improve property naming and add documentationConsider:
- Rename
isLocationSettingChangetoisLocationSettingUpdatingfor clarity- Add KDoc comments to explain the purpose of each property
Consider this implementation:
data class SpaceProfileState( // ... other properties ... + /** Time remaining until the invite code expires */ val codeExpireTime: String = "", + /** Loading state for invite code regeneration */ val isCodeLoading: Boolean = false, + /** Map of member IDs to their updated location sharing status */ val locationEnabledChanges: Map<String, Boolean> = emptyMap(), - val isLocationSettingChange: Boolean = false, + /** Indicates if a location setting update is in progress */ + val isLocationSettingUpdating: Boolean = false, + /** ID of the member whose location sharing status is being updated */ val userLocationUpdatingId: String? = null )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt (2)
199-203: LGTM! Good error handling and state managementThe implementation properly updates the space info after save operation and handles errors appropriately.
88-98: 🛠️ Refactor suggestionImprove state management in
onLocationEnabledChangedThe current implementation has potential state inconsistency issues:
- State is updated before the API call completes
- Loading state is not managed
isLocationSettingChangeis not handled in error caseConsider this implementation:
viewModelScope.launch { try { + _state.value = state.value.copy(isLocationSettingChange = true) spaceRepository.enableLocation(spaceID, authService.currentUser?.id ?: "", enable) - _state.value = state.value.copy(locationEnabled = enable) + _state.value = state.value.copy( + locationEnabled = enable, + isLocationSettingChange = false + ) } catch (e: Exception) { Timber.e(e, "Failed to update location") - _state.value = state.value.copy(error = e, locationEnabled = !enable) + _state.value = state.value.copy( + error = e, + isLocationSettingChange = false + ) } onChange() }Likely invalid or redundant comment.
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt
Outdated
Show resolved
Hide resolved
|
Also, the click ripple for save/check button in edit-name field needs to be updated |
|
There should be a loader(in place of check mark), when there is api call to update group name |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileScreen.kt (2)
287-291: Improve click handling for better accessibilityThe click handling on the check icon should be implemented using IconButton for better accessibility and consistent ripple effect.
- .clickable { - ripple(true) - viewModel.saveSpace() - focusManager.clearFocus() - } + modifier = Modifier.padding(horizontal = 8.dp) + } + IconButton( + onClick = { + viewModel.saveSpace() + focusManager.clearFocus() + } + ) { + Icon( + imageVector = Icons.Default.Check, + contentDescription = stringResource(R.string.save_changes), + tint = outlineColor + )
389-396: Add content description for location toggleThe Switch component should have a meaningful content description for better accessibility.
Switch( checked = isChecked, enabled = enable, + modifier = Modifier + .padding(end = 8.dp) + .semantics { + contentDescription = stringResource( + if (isChecked) R.string.location_sharing_enabled + else R.string.location_sharing_disabled, + userInfo.user.fullName + ) + }, onCheckedChange = { isChecked -> onCheckedChange(isChecked) }, - modifier = Modifier.padding(end = 8.dp) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileScreen.kt(6 hunks)app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt
🔇 Additional comments (2)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileScreen.kt (2)
364-366: Verify error handling for location updatesBased on previous review comments, ensure proper error handling is implemented in the ViewModel for failed location updates.
#!/bin/bash # Search for error handling in location update implementations rg "onLocationEnabledChanged|updateMemberLocation" -A 10 "app/src/main/java/com/canopas/yourspace/ui/flow/settings/space/SpaceProfileViewModel.kt"
310-310: LGTM! Padding improvementsThe padding adjustments improve the visual layout and maintain consistency throughout the screen.
Also applies to: 315-315, 343-343, 349-349
Changelog
New Features
Screen_recording_20250108_113037.mp4
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements