Add validation to check document ids before api calling #164
Conversation
WalkthroughThis pull request focuses on improving null safety, error handling, and input validation across several services and repositories in the YourSpace application. The changes primarily involve modifying how user IDs, space IDs, and current user references are handled, ensuring more robust code by preventing potential null pointer exceptions and enforcing stricter input validation. The modifications span multiple files, including location and journey services, authentication service, and space repository. 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: 1
🧹 Nitpick comments (11)
data/src/main/java/com/canopas/yourspace/data/service/location/ApiJourneyService.kt (3)
40-44: Enhance error message for better debugging.While the validation is correct, consider making the error message more descriptive to aid in debugging.
- throw IllegalArgumentException("Space ID is empty") + throw IllegalArgumentException("Space ID cannot be blank when creating space member reference")
46-50: Enhance error message for better debugging.Consider making the error message more descriptive to aid in debugging.
- throw IllegalArgumentException("User ID is empty") + throw IllegalArgumentException("User ID cannot be blank when creating journey reference")
52-57: Multiple improvements suggested.
- The error message could be more descriptive.
- The indentation on line 57 is inconsistent.
- Consider extracting the hardcoded document ID to a constant.
+ private companion object { + private const val GROUP_KEYS_DOC_ID = FIRESTORE_COLLECTION_SPACE_GROUP_KEYS + } private fun spaceGroupKeysRef(spaceId: String) = if (spaceId.isBlank()) { - throw IllegalArgumentException("Space ID is empty") + throw IllegalArgumentException("Space ID cannot be blank when creating group keys reference") } else { spaceRef.document(spaceId).collection(FIRESTORE_COLLECTION_SPACE_GROUP_KEYS) - .document(FIRESTORE_COLLECTION_SPACE_GROUP_KEYS) + .document(GROUP_KEYS_DOC_ID) }data/src/main/java/com/canopas/yourspace/data/service/messages/ApiMessagesService.kt (2)
31-35: Consider enhancing error message and documentation.While the validation is good, we can make it more helpful for debugging.
Consider this enhancement:
- private fun threadMessagesRef(threadId: String) = if (threadId.isBlank()) { - throw IllegalArgumentException("Thread ID cannot be empty") + /** + * Returns a reference to the messages collection for a thread. + * @param threadId The ID of the thread + * @throws IllegalArgumentException if threadId is blank + */ + private fun threadMessagesRef(threadId: String) = if (threadId.isBlank()) { + throw IllegalArgumentException("Thread ID cannot be empty or blank. Provided: '$threadId'")
31-35: Consider consistent error handling across the service.While we're validating
threadIdinthreadMessagesRef, other methods likecreateThread,joinThread, etc. might benefit from similar input validation.Consider adding similar validation for:
spaceIdandadminIdincreateThreadthreadIdanduserIdsinjoinThread- Other methods accepting IDs as parameters
Would you like me to propose specific validation changes for these methods?
data/src/main/java/com/canopas/yourspace/data/service/location/ApiLocationService.kt (2)
54-58: LGTM! Consider adding KDoc documentation.The validation logic is correct and aligns with the PR objectives. Early validation of blank IDs will prevent downstream issues.
Consider adding KDoc documentation to clarify the function's behavior and the conditions under which it throws exceptions:
/** * Returns a reference to the space members collection. * @param spaceId The ID of the space * @throws IllegalArgumentException if spaceId is blank */
67-72: LGTM! Consider extracting the duplicate constant.The validation logic is correct, but there's a minor improvement possible.
The constant
FIRESTORE_COLLECTION_SPACE_GROUP_KEYSis used twice. Consider extracting it to a local variable for better maintainability:private fun spaceGroupKeysRef(spaceId: String) = if (spaceId.isBlank()) { throw IllegalArgumentException("Space ID cannot be blank") } else { + val groupKeysCollection = FIRESTORE_COLLECTION_SPACE_GROUP_KEYS spaceRef.document(spaceId).collection(FIRESTORE_COLLECTION_SPACE_GROUP_KEYS) - .document(FIRESTORE_COLLECTION_SPACE_GROUP_KEYS) + .document(groupKeysCollection) }data/src/main/java/com/canopas/yourspace/data/service/user/ApiUserService.kt (1)
54-55: Consider documenting the null return case.The early validation for blank user IDs is good. Consider adding a KDoc comment to document that the function returns null for blank IDs.
+ /** + * Retrieves a user from Firestore by their ID. + * @param userId The ID of the user to retrieve + * @return null if userId is blank, user doesn't exist, or an error occurs + */ suspend fun getUser(userId: String): ApiUser? {data/src/main/java/com/canopas/yourspace/data/service/space/ApiSpaceService.kt (2)
46-52: Consider documenting error handling for cryptographic operationsGood validation. Since this function is used in cryptographic operations (e.g.,
distributeSenderKeyToSpaceMembers), consider adding documentation about error handling requirements for security-critical flows.Add KDoc to document the validation:
+ /** + * Returns the document reference for space group keys. + * @param spaceId The space identifier + * @throws IllegalArgumentException if spaceId is blank + */ private fun spaceGroupKeysDoc(spaceId: String) = if (spaceId.isBlank()) {
40-52: Consider standardizing error handling across servicesThe validation approach is good, but since this is part of a broader effort to improve error handling across services (ApiJourneyService, ApiLocationService, etc.), consider:
- Creating a common validation utility class for consistent ID validation
- Standardizing error messages across services
- Documenting the error handling strategy for the entire data layer
This would ensure consistency and make the validation behavior more maintainable.
Example utility class:
object ValidationUtils { fun validateSpaceId(spaceId: String, context: String = "") { if (spaceId.isBlank()) { throw IllegalArgumentException("${context}Space ID cannot be empty") } } // Add other validation methods as needed }data/src/main/java/com/canopas/yourspace/data/service/place/ApiPlaceService.kt (1)
31-38: LGTM! Consider improving readability.The improved validation for blank IDs is good. However, the indentation of the collection chain could be more readable.
Consider this formatting for better readability:
private fun spacePlacesSettingsRef(spaceId: String, placeId: String) = if (spaceId.isEmpty() || placeId.isEmpty()) { throw IllegalStateException("Space ID or Place ID is empty") } else { - spaceRef.document(spaceId).collection(Config.FIRESTORE_COLLECTION_SPACE_PLACES) - .document(placeId).collection( - Config.FIRESTORE_COLLECTION_SPACE_PLACES_MEMBER_SETTINGS - ) + spaceRef + .document(spaceId) + .collection(Config.FIRESTORE_COLLECTION_SPACE_PLACES) + .document(placeId) + .collection(Config.FIRESTORE_COLLECTION_SPACE_PLACES_MEMBER_SETTINGS) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
data/src/main/java/com/canopas/yourspace/data/service/location/ApiJourneyService.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/service/location/ApiLocationService.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/service/messages/ApiMessagesService.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/service/place/ApiPlaceService.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/service/space/ApiSpaceService.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/service/user/ApiUserService.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (8)
data/src/main/java/com/canopas/yourspace/data/service/location/ApiJourneyService.kt (1)
40-57: Overall implementation looks good!The validation checks for blank IDs are well-placed and will prevent potential issues with invalid Firestore references. The implementation is consistent across all reference-creating functions and aligns well with similar validation patterns in other files.
data/src/main/java/com/canopas/yourspace/data/service/messages/ApiMessagesService.kt (1)
31-35: LGTM! Good addition of input validation.The change improves error handling by preventing operations with invalid thread IDs early, which aligns well with the PR objectives.
Let's verify the impact on callers of this function:
✅ Verification successful
Validation change is safe and properly handled
The change is verified to be safe because:
- All calls to
threadMessagesRefare wrapped in try-catch blocks in the ViewModel layer- Exceptions are properly propagated and displayed to users via UI state updates
- The validation is consistently applied across all message operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct calls to threadMessagesRef or methods that use it ast-grep --pattern 'threadMessagesRef($$$)' # Search for error handling patterns in callers rg -A 5 'getLatestMessages|getMessages|sendMessage|generateMessage'Length of output: 11391
data/src/main/java/com/canopas/yourspace/data/service/location/ApiLocationService.kt (1)
Line range hint
74-190: Review error handling in dependent functions.The reference functions now throw
IllegalArgumentExceptioninstead of returning null references. The dependent functions (saveLastKnownLocation, saveCurrentLocation, getCurrentLocation) should be updated to handle these exceptions appropriately.Please verify the error handling in these functions. Here's a script to find all usages:
Consider updating the error handling in dependent functions:
try { // Existing code } catch (e: Exception) { when (e) { is IllegalArgumentException -> { Timber.e(e, "Invalid ID provided") // Handle invalid ID appropriately } is NoSessionException -> { // Existing handling } // Other existing cases } }✅ Verification successful
Existing error handling is appropriate
The dependent functions already implement comprehensive error handling through null checks, early returns, and try-catch blocks. The
IllegalArgumentExceptionfrom reference functions serves as input validation, preventing invalid states from propagating. No changes are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of the reference functions to verify error handling # Search for function calls rg -A 5 "spaceMemberRef|spaceMemberLocationRef|spaceGroupKeysRef" --type kotlin # Search for try-catch blocks that might need updating rg -B 2 -A 5 "catch \((Exception|Throwable)" --type kotlinLength of output: 16082
data/src/main/java/com/canopas/yourspace/data/service/user/ApiUserService.kt (2)
Line range hint
1-276: Consider consistent ID validation across all methods.Several other methods in this class accept
userIdas a parameter but don't validate for blank IDs:
deleteUserupdateUserregisterFcmTokenaddSpaceIdupdateBatteryPctupdateSessionStategetUserSessiongetUserNetworkStatusConsider adding similar validation to maintain consistency across the service.
Let's verify which methods might need validation:
✅ Verification successful
Validation needed in methods using userId parameter
The review comment is correct. Several methods in
ApiUserServiceuseuserIdwithout validation:
deleteUser(userId)directly uses it inuserRef.document(userId).delete()registerFcmToken(userId, token)uses it in document referenceaddSpaceId(userId, spaceId)uses it for document updateupdateBatteryPct(userId, batteryPct)uses it in document referenceupdateSessionState(userId, state)uses it for document updategetUserSession(userId)uses it insessionRefgetUserNetworkStatus(userId, lastUpdatedTime, onStatusChecked)uses it in function parametersSince
sessionRefandgetUseralready validate for blank userId, the same validation should be added to maintain consistency and prevent potential issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all public methods that take userId as parameter ast-grep --pattern 'fun $methodName($$$userId: String$$$)' | rg -B 2 -A 2 'fun.*userId.*String'Length of output: 97
Script:
#!/bin/bash # Try different patterns to find methods with userId parameter ast-grep --pattern 'suspend fun $name($$$userId: String$$$)' echo "=== Using ripgrep as fallback ===" rg "fun.*\(.*userId:\s*String" -A 2Length of output: 17706
46-50: LGTM! Verify impact on dependent methods.The validation for blank user IDs is a good addition that aligns with the PR objectives. The error message is clear and descriptive.
Let's verify the impact on methods that call
sessionRef:✅ Verification successful
Validation in
sessionRefis appropriately placed and handledThe IllegalArgumentException for blank user IDs is well-placed in this private utility method. Some calling methods like
getUserandsaveUseralready validate their inputs, while others appropriately allow the exception to propagate for invalid inputs, which is the correct behavior for these internal service methods.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all methods that call sessionRef to ensure they handle the new exception ast-grep --pattern 'sessionRef($$$)' | rg -A 5 'sessionRef'Length of output: 852
Script:
#!/bin/bash # Get the complete implementation of methods containing sessionRef calls ast-grep --pattern 'fun $$$sessionRef($$$)' | rg -B 5 -A 15 'sessionRef'Length of output: 72
Script:
#!/bin/bash # Get implementations of methods containing sessionRef with more context rg -B 10 -A 20 "fun (get|sign|register|.*Session)" data/src/main/java/com/canopas/yourspace/data/service/user/ApiUserService.ktLength of output: 5168
data/src/main/java/com/canopas/yourspace/data/service/space/ApiSpaceService.kt (1)
40-44: Validate error handling in calling methodsGood addition of input validation. However, since this is a private function used by multiple public methods, we should ensure proper error handling in the calling methods.
Let's verify the error handling in calling methods:
✅ Verification successful
Input validation implementation is appropriate
The validation in
spaceMemberRefis well-implemented:
- Validates input at the service layer where the space ID is first used
- Uses appropriate exception type (IllegalArgumentException) for invalid arguments
- Follows Kotlin's suspend function error propagation patterns
- Consistent with other methods in the class (e.g.,
spaceGroupKeysDoc)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all methods that call spaceMemberRef ast-grep --pattern 'fun $FUNC($ARGS) { $$$ spaceMemberRef($SPACE_ID) $$$ }'Length of output: 83
Script:
#!/bin/bash # View the content of the file to understand its structure cat data/src/main/java/com/canopas/yourspace/data/service/space/ApiSpaceService.ktLength of output: 9942
Script:
#!/bin/bash # Search for ViewModel classes that might be handling these service calls rg -l "ApiSpaceService" --type kotlin | grep -i "viewmodel"Length of output: 57
data/src/main/java/com/canopas/yourspace/data/service/place/ApiPlaceService.kt (2)
24-38: Consider standardizing empty ID handling across all methods.I notice that
listenAllPlaceshandles emptyspaceIddifferently by returningemptyFlow()instead of throwing an exception. Consider standardizing the approach across all methods for consistency:
- Either make all methods throw exceptions for empty IDs
- Or make all methods handle empty IDs gracefully (return null/empty)
Let's check for inconsistent empty ID handling:
✅ Verification successful
Empty ID handling is correctly implemented
The current implementation follows a good pattern:
- Internal reference methods fail fast with exceptions to prevent invalid database operations
- Public-facing methods handle empty IDs gracefully by returning empty collections
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all methods that check for empty IDs rg -A 2 "isEmpty\(\)" data/src/main/java/com/canopas/yourspace/data/service/place/ApiPlaceService.ktLength of output: 706
24-28: LGTM! Verify exception handling in callers.The improved error handling for blank space IDs is a good change. However, since this is a breaking change (throwing exception instead of returning null), ensure that all calling methods properly handle the
IllegalStateException.Let's verify the exception handling in all callers:
✅ Verification successful
Exception handling approach is appropriate
The verification shows that:
- Most methods properly propagate exceptions through suspend functions
listenAllPlaces()already handles empty space IDs defensively- Service layer should fail fast on invalid inputs rather than silently continuing
The change from returning null to throwing an exception is appropriate here as it:
- Prevents silent failures
- Forces proper error handling in the presentation layer
- Maintains consistency with Kotlin's fail-fast principles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find all methods that call spacePlacesRef ast-grep --pattern 'spacePlacesRef($$$)'Length of output: 793
Script:
#!/bin/bash # Get context of methods containing spacePlacesRef calls ast-grep --pattern 'suspend fun $$$($$$) { $$$ spacePlacesRef($$$) $$$ }' # Also check for try-catch blocks around these calls ast-grep --pattern 'try { $$$ spacePlacesRef($$$) $$$ } catch'Length of output: 152
Script:
#!/bin/bash # Get context around spacePlacesRef calls with ripgrep rg "spacePlacesRef" data/src/main/java/com/canopas/yourspace/data/service/place/ApiPlaceService.kt -B 5 -A 5Length of output: 1920
data/src/main/java/com/canopas/yourspace/data/service/location/ApiLocationService.kt
Outdated
Show resolved
Hide resolved
cp-sneh-s
changed the title
data/src/main/java/com/canopas/yourspace/data/service/location/ApiJourneyService.kt
Outdated
Show resolved
Hide resolved
This reverts commit 909e7f8.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
data/src/main/java/com/canopas/yourspace/data/service/location/ApiLocationService.kt (1)
58-59:⚠️ Potential issueAdd validation for both spaceId and userId.
The function needs validation for both parameters to prevent invalid document references.
As suggested in the previous review, implement validation for both parameters:
private fun spaceMemberLocationRef(spaceId: String, userId: String) = + when { + userId.isBlank() -> throw IllegalArgumentException("User ID cannot be blank") + spaceId.isBlank() -> throw IllegalArgumentException("Space ID cannot be blank") + else -> spaceMemberRef(spaceId).document(userId) .collection(Config.FIRESTORE_COLLECTION_USER_LOCATIONS) + }
🧹 Nitpick comments (4)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/SettingsViewModel.kt (1)
40-42: LGTM! Consider enhancing error handling.The null-safe operator is a good addition for preventing NPEs. However, consider adding explicit error handling and logging for the null case.
Here's a suggested improvement:
- authService.getUserFlow()?.collectLatest { user -> - _state.emit(_state.value.copy(user = user)) - } + try { + val userFlow = authService.getUserFlow() + if (userFlow == null) { + Timber.e("getUserFlow() returned null") + _state.emit(_state.value.copy(error = "Failed to get user data")) + return@launch + } + userFlow.collectLatest { user -> + _state.emit(_state.value.copy(user = user, error = null)) + } + } catch (e: Exception) { + Timber.e(e, "Failed to get user") + _state.emit(_state.value.copy(error = e.localizedMessage)) + }data/src/main/java/com/canopas/yourspace/data/service/location/ApiLocationService.kt (1)
55-63: Consider implementing a unified validation approach.The validation logic is duplicated across multiple functions. Consider extracting the validation into a reusable extension function or utility method.
+ private fun requireValidId(id: String, type: String) { + if (id.isBlank()) { + throw IllegalArgumentException("$type ID cannot be blank") + } + } private fun spaceMemberRef(spaceId: String) = + requireValidId(spaceId, "Space").let { spaceRef.document(spaceId).collection(FIRESTORE_COLLECTION_SPACE_MEMBERS) + } private fun spaceMemberLocationRef(spaceId: String, userId: String) = + requireValidId(spaceId, "Space").also { requireValidId(userId, "User") }.let { spaceMemberRef(spaceId).document(userId) .collection(Config.FIRESTORE_COLLECTION_USER_LOCATIONS) + } private fun spaceGroupKeysRef(spaceId: String) = + requireValidId(spaceId, "Space").let { spaceRef.document(spaceId).collection(FIRESTORE_COLLECTION_SPACE_GROUP_KEYS) .document(FIRESTORE_COLLECTION_SPACE_GROUP_KEYS) + }data/src/main/java/com/canopas/yourspace/data/repository/SpaceRepository.kt (2)
Line range hint
188-200: Consider adding error logging for null userId caseWhile the null safety improvements are good, consider adding error logging when
userIdis null to help with debugging and monitoring.val userId = authService.currentUser?.id + if (userId == null) { + Timber.w("Attempted to delete user spaces with null userId") + return + } val allSpace = userId?.let { getUserSpaces(it).firstOrNull() } ?: emptyList()
207-212: Consider extracting space selection logicThe chain of operations for selecting the next current space is complex. Consider extracting it into a separate method for better readability and maintainability.
+ private suspend fun getNextAvailableSpaceId(userId: String): String { + return getUserSpaces(userId) + .firstOrNull() + ?.sortedBy { it.created_at } + ?.firstOrNull() + ?.id ?: "" + } val userId = authService.currentUser?.id -currentSpaceId = - userId?.let { getUserSpaces(it).firstOrNull()?.sortedBy { it.created_at }?.firstOrNull()?.id } - ?: "" +currentSpaceId = userId?.let { getNextAvailableSpaceId(it) } ?: ""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/main/java/com/canopas/yourspace/ui/flow/settings/SettingsViewModel.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/repository/SpaceRepository.kt(4 hunks)data/src/main/java/com/canopas/yourspace/data/service/auth/AuthService.kt(2 hunks)data/src/main/java/com/canopas/yourspace/data/service/location/ApiJourneyService.kt(1 hunks)data/src/main/java/com/canopas/yourspace/data/service/location/ApiLocationService.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- data/src/main/java/com/canopas/yourspace/data/service/location/ApiJourneyService.kt
🔇 Additional comments (2)
data/src/main/java/com/canopas/yourspace/data/service/location/ApiLocationService.kt (1)
62-63:⚠️ Potential issueAdd validation for spaceId in encryption-related reference.
This function is used in encryption operations (getGroupCipherAndDistributionMessage), making it critical to validate the spaceId to prevent security issues.
private fun spaceGroupKeysRef(spaceId: String) = + if (spaceId.isBlank()) { + throw IllegalArgumentException("Space ID cannot be blank") + } else { spaceRef.document(spaceId).collection(FIRESTORE_COLLECTION_SPACE_GROUP_KEYS) .document(FIRESTORE_COLLECTION_SPACE_GROUP_KEYS) + }Likely invalid or redundant comment.
data/src/main/java/com/canopas/yourspace/data/repository/SpaceRepository.kt (1)
118-119: LGTM! Improved null safety handlingThe changes properly handle the case where
currentUser.idmight be null, preventing potential NPEs and aligning with the PR's objective of adding validation before API calls.
| @@ -52,16 +52,14 @@ class ApiLocationService @Inject constructor( | |||
| private val spaceRef by lazy { db.collection(FIRESTORE_COLLECTION_SPACES) } | |||
|
|
|||
| private fun spaceMemberRef(spaceId: String) = | |||
| spaceRef.document(spaceId.takeIf { it.isNotBlank() } ?: "null") | |||
| .collection(FIRESTORE_COLLECTION_SPACE_MEMBERS) | |||
| spaceRef.document(spaceId).collection(FIRESTORE_COLLECTION_SPACE_MEMBERS) | |||
There was a problem hiding this comment.
Add validation for blank spaceId.
As per the PR objectives, we need to validate document IDs before making API calls. Add a validation check for blank spaceId to prevent potential runtime errors.
private fun spaceMemberRef(spaceId: String) =
+ if (spaceId.isBlank()) {
+ throw IllegalArgumentException("Space ID cannot be blank")
+ } else {
spaceRef.document(spaceId).collection(FIRESTORE_COLLECTION_SPACE_MEMBERS)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spaceRef.document(spaceId).collection(FIRESTORE_COLLECTION_SPACE_MEMBERS) | |
| private fun spaceMemberRef(spaceId: String) = | |
| if (spaceId.isBlank()) { | |
| throw IllegalArgumentException("Space ID cannot be blank") | |
| } else { | |
| spaceRef.document(spaceId).collection(FIRESTORE_COLLECTION_SPACE_MEMBERS) | |
| } |
| val userId = authService.currentUser?.id | ||
| if (userId != null) { | ||
| spaceService.removeUserFromSpace(spaceId, userId) | ||
| } | ||
| val user = userId?.let { userService.getUser(it) } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and consider code reuse
- The user space update logic is duplicated between
leaveSpace()anddeleteSpace(). Consider extracting this common functionality. - Add error logging for the case when userId is null.
+ private suspend fun updateUserSpaceIds(userId: String, spaceId: String) {
+ val user = userService.getUser(userId) ?: return
+ val updatedSpaceIds = user.space_ids.toMutableList().apply {
+ remove(spaceId)
+ }
+ user.copy(space_ids = updatedSpaceIds).let {
+ userService.updateUser(it)
+ }
+ }
suspend fun leaveSpace(spaceId: String) {
val userId = authService.currentUser?.id
+ if (userId == null) {
+ Timber.w("Attempted to leave space with null userId")
+ return
+ }
- if (userId != null) {
- spaceService.removeUserFromSpace(spaceId, userId)
- }
- val user = userId?.let { userService.getUser(it) }
- val updatedSpaceIds = user?.space_ids?.toMutableList()?.apply {
- remove(spaceId)
- } ?: return
-
- user.copy(space_ids = updatedSpaceIds).let {
- userService.updateUser(it)
- }
+ spaceService.removeUserFromSpace(spaceId, userId)
+ updateUserSpaceIds(userId, spaceId)
}Committable suggestion skipped: line range outside the PR's diff.
| signOut() | ||
| } | ||
|
|
||
| suspend fun generateAndSaveUserKeys(passKey: String) { | ||
| val user = currentUser ?: throw IllegalStateException("No user logged in") | ||
| val user = currentUser |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add blank ID validation and simplify null check.
While the null safety is improved, consider:
- Adding validation for blank user ID as per PR objectives
- Simplifying by removing the redundant local variable
- val user = currentUser
- val updatedUser = user?.let { apiUserService.generateAndSaveUserKeys(it, passKey) }
+ val updatedUser = currentUser?.let { user ->
+ require(user.id.isNotBlank()) { "User ID cannot be blank" }
+ apiUserService.generateAndSaveUserKeys(user, passKey)
+ }Also applies to: 131-131
| val currentUser = currentUser | ||
| if (currentUser != null) { | ||
| apiUserService.updateSessionState(currentUser.id, state) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve null safety pattern and add blank ID validation.
Consider:
- Using Kotlin's
?.letfor consistency with other methods - Adding validation for blank user ID
- Removing redundant local variable
- val currentUser = currentUser
- if (currentUser != null) {
- apiUserService.updateSessionState(currentUser.id, state)
- }
+ currentUser?.let { user ->
+ require(user.id.isNotBlank()) { "User ID cannot be blank" }
+ apiUserService.updateSessionState(user.id, state)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val currentUser = currentUser | |
| if (currentUser != null) { | |
| apiUserService.updateSessionState(currentUser.id, state) | |
| } | |
| currentUser?.let { user -> | |
| require(user.id.isNotBlank()) { "User ID cannot be blank" } | |
| apiUserService.updateSessionState(user.id, state) | |
| } |
| suspend fun getUser(): ApiUser? = currentUser?.id?.let { apiUserService.getUser(it) } | ||
| suspend fun getUserFlow() = currentUser?.id?.let { apiUserService.getUserFlow(it) } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add blank ID validation to user retrieval methods.
While the null safety is improved, add validation for blank user IDs as per PR objectives:
- suspend fun getUser(): ApiUser? = currentUser?.id?.let { apiUserService.getUser(it) }
- suspend fun getUserFlow() = currentUser?.id?.let { apiUserService.getUserFlow(it) }
+ suspend fun getUser(): ApiUser? = currentUser?.id?.let { id ->
+ require(id.isNotBlank()) { "User ID cannot be blank" }
+ apiUserService.getUser(id)
+ }
+ suspend fun getUserFlow() = currentUser?.id?.let { id ->
+ require(id.isNotBlank()) { "User ID cannot be blank" }
+ apiUserService.getUserFlow(id)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| suspend fun getUser(): ApiUser? = currentUser?.id?.let { apiUserService.getUser(it) } | |
| suspend fun getUserFlow() = currentUser?.id?.let { apiUserService.getUserFlow(it) } | |
| suspend fun getUser(): ApiUser? = currentUser?.id?.let { id -> | |
| require(id.isNotBlank()) { "User ID cannot be blank" } | |
| apiUserService.getUser(id) | |
| } | |
| suspend fun getUserFlow() = currentUser?.id?.let { id -> | |
| require(id.isNotBlank()) { "User ID cannot be blank" } | |
| apiUserService.getUserFlow(id) | |
| } |
| val currentUser = currentUser | ||
| currentUser?.let { apiUserService.deleteUser(it.id) } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add blank ID validation and simplify null check.
While the null safety is improved, consider:
- Adding validation for blank user ID as per PR objectives
- Simplifying by removing the redundant local variable
- val currentUser = currentUser
- currentUser?.let { apiUserService.deleteUser(it.id) }
+ currentUser?.let { user ->
+ require(user.id.isNotBlank()) { "User ID cannot be blank" }
+ apiUserService.deleteUser(user.id)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val currentUser = currentUser | |
| currentUser?.let { apiUserService.deleteUser(it.id) } | |
| currentUser?.let { user -> | |
| require(user.id.isNotBlank()) { "User ID cannot be blank" } | |
| apiUserService.deleteUser(user.id) | |
| } |
| val user = currentUser | ||
| val validationResult = user?.let { apiUserService.validatePasskey(it, passKey) } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add blank ID validation and simplify null check.
While the null safety is improved, consider:
- Adding validation for blank user ID as per PR objectives
- Simplifying by removing the redundant local variable
- val user = currentUser
- val validationResult = user?.let { apiUserService.validatePasskey(it, passKey) }
+ val validationResult = currentUser?.let { user ->
+ require(user.id.isNotBlank()) { "User ID cannot be blank" }
+ apiUserService.validatePasskey(user, passKey)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val user = currentUser | |
| val validationResult = user?.let { apiUserService.validatePasskey(it, passKey) } | |
| val validationResult = currentUser?.let { user -> | |
| require(user.id.isNotBlank()) { "User ID cannot be blank" } | |
| apiUserService.validatePasskey(user, passKey) | |
| } |
Changelog
Summary by CodeRabbit
Bug Fixes
Refactor