π :: (#880) μ½λ£¨ν΄ μ·¨μ μ νλ₯Ό μν runcatching κ°μ #892
Merged
Conversation
|
No actionable comments were generated in the recent review. π βΉοΈ Recent review infoβοΈ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: π Files selected for processing (1)
π§ Files skipped from review as they are similar to previous changes (1)
π WalkthroughWalkthroughReplaced suspend-run-catching helpers with a new cancellation-aware Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant VM as ViewModel
participant Repo as Repository
participant Run as runCatchingCancellable
participant API as Network/Data
VM->>Repo: call suspend operation
Repo->>Run: execute block
Run->>API: invoke network/database call
API-->>Run: success / throws
alt Success
Run-->>Repo: Result.success(value)
Repo-->>VM: return success result
else Exception (non-cancellation)
Run-->>Repo: Result.failure(error)
Repo-->>VM: propagate failure / fold handling
else CancellationException
Run-->>API: rethrow CancellationException
API-->>Repo: cancellation propagated
Repo-->>VM: cancellation propagated
end
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~25 minutes Suggested labels
Suggested reviewers
Poem
π₯ Pre-merge checks | β 2 | β 1β Failed checks (1 warning)
β Passed checks (2 passed)
βοΈ Tip: You can configure your own custom pre-merge checks in the settings. β¨ Finishing Touchesπ Generate docstrings
π§ͺ Generate unit tests (beta)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (1)
feature/src/main/kotlin/team/aliens/dms/android/feature/signup/viewmodel/EnterSchoolVerificationQuestionViewModel.kt (1)
46-63:β οΈ Potential issue | π‘ MinorMove loading reset to
finallyto handle cancellation safely.After this migration, cancellation no longer goes through
onFailure, soisLoadingmay remaintrueif the call is cancelled mid-flight.Suggested fix
internal fun onNextClick() { viewModelScope.launch { setState { it.copy(isLoading = true) } - runCatchingCancellable { - networkSchoolDataSource.examineSchoolVerificationQuestion( - schoolId = UUID.fromString(currentSignUpData.schoolId), - answer = uiState.value.schoolVerificationAnswer, - ) - }.onSuccess { - setState { it.copy(isLoading = false) } - sendEffect( - EnterSchoolVerificationQuestionSideEffect.MoveToEnterEmail( - signUpData = currentSignUpData.copy( - schoolAnswer = uiState.value.schoolVerificationAnswer, - ) - ) - ) - }.onFailure { - setState { it.copy(isLoading = false) } - sendEffect(EnterSchoolVerificationQuestionSideEffect.ShowErrorSnackBar) - } + try { + runCatchingCancellable { + networkSchoolDataSource.examineSchoolVerificationQuestion( + schoolId = UUID.fromString(currentSignUpData.schoolId), + answer = uiState.value.schoolVerificationAnswer, + ) + }.onSuccess { + sendEffect( + EnterSchoolVerificationQuestionSideEffect.MoveToEnterEmail( + signUpData = currentSignUpData.copy( + schoolAnswer = uiState.value.schoolVerificationAnswer, + ) + ) + ) + }.onFailure { + sendEffect(EnterSchoolVerificationQuestionSideEffect.ShowErrorSnackBar) + } + } finally { + setState { it.copy(isLoading = false) } + } } }π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/src/main/kotlin/team/aliens/dms/android/feature/signup/viewmodel/EnterSchoolVerificationQuestionViewModel.kt` around lines 46 - 63, The isLoading flag reset belongs in a finally block so cancellation (which no longer triggers onFailure) always clears loading; in EnterSchoolVerificationQuestionViewModel wrap the runCatchingCancellable call in a try { ... } catch { ... } finally { setState { it.copy(isLoading = false) } } (keep success path sending EnterSchoolVerificationQuestionSideEffect.MoveToEnterEmail and failure sending ShowErrorSnackBar) and remove the duplicated setState { isLoading = false } from the onSuccess/onFailure handlers so loading is cleared reliably even on cancellation.
π€ Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/widget/src/main/kotlin/team/aliens/dms/android/core/widget/MealWorker.kt`:
- Around line 69-84: The widget can remain stuck in MealInfo.Loading because
runCatchingCancellable rethrows CancellationException and onFailure won't run on
cancellation; modify MealWorker.kt to explicitly handle cancellation after
setting Loading by wrapping the fetch in a try/catch that catches
CancellationException, calls setWidgetState(glanceIds, MealInfo.Unavailable) (or
restores previous state) and then rethrows the CancellationException, and keep
the existing runCatchingCancellable block for other failures so non-cancellation
errors still hit onFailure.
In
`@data/src/main/kotlin/team/aliens/dms/android/data/meal/repository/MealRepositoryImpl.kt`:
- Around line 24-32: The fold in MealRepositoryImpl.kt currently maps any
non-CannotSaveMealException to a new CannotSaveMealException, which wrongly
wraps CannotFindMealException; change the onFailure handling for
updateMeal(date) so that if exception is CannotFindMealException you return
Result.failure(exception) (preserving the original exception), if it's
CannotSaveMealException return Result.failure(exception) as before, and only
wrap other unexpected exceptions as CannotSaveMealException(); reference the
updateMeal call and the exception types CannotFindMealException and
CannotSaveMealException when making this adjustment.
---
Outside diff comments:
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/signup/viewmodel/EnterSchoolVerificationQuestionViewModel.kt`:
- Around line 46-63: The isLoading flag reset belongs in a finally block so
cancellation (which no longer triggers onFailure) always clears loading; in
EnterSchoolVerificationQuestionViewModel wrap the runCatchingCancellable call in
a try { ... } catch { ... } finally { setState { it.copy(isLoading = false) } }
(keep success path sending
EnterSchoolVerificationQuestionSideEffect.MoveToEnterEmail and failure sending
ShowErrorSnackBar) and remove the duplicated setState { isLoading = false } from
the onSuccess/onFailure handlers so loading is cleared reliably even on
cancellation.
πͺ Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
βΉοΈ Review info
βοΈ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e468e34-fd14-44aa-8e15-e1cf4e87992c
π Files selected for processing (26)
core/datastore/src/main/java/team/aliens/dms/android/core/datastore/util/TransformHandler.ktcore/device/src/main/kotlin/team/aliens/dms/android/core/device/datastore/DeviceDataStoreDataSourceImpl.ktcore/network/src/main/java/team/aliens/dms/android/core/network/util/NetworkExceptionMapper.ktcore/widget/build.gradle.ktscore/widget/src/main/kotlin/team/aliens/dms/android/core/widget/MealWorker.ktdata/build.gradle.ktsdata/src/main/kotlin/team/aliens/dms/android/data/auth/repository/AuthRepositoryImpl.ktdata/src/main/kotlin/team/aliens/dms/android/data/meal/repository/MealRepositoryImpl.ktdata/src/main/kotlin/team/aliens/dms/android/data/student/repository/StudentRepositoryImpl.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/findid/ui/FindIdViewModel.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/onboarding/viewmodel/OnboardingViewModel.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/profile/viewmodel/AdjustProfileViewModel.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/signup/viewmodel/EnterSchoolVerificationCodeViewModel.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/signup/viewmodel/EnterSchoolVerificationQuestionViewModel.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/auth/datasource/NetworkAuthDataSourceImpl.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/file/datasource/NetworkFileDataSourceImpl.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/meal/datasource/NetworkMealDataSourceImpl.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/notice/datasource/NetworkNoticeDataSourceImpl.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/notification/datasource/NetworkNotificationDataSourceImpl.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/point/datasource/NetworkPointDataSourceImpl.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/remains/datasource/NetworkRemainsDataSourceImpl.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/student/datasource/NetworkStudentDataSourceImpl.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/user/datasource/NetworkUserDataSourceImpl.ktnetwork/src/main/kotlin/team/aliens/dms/android/network/voting/datasource/NetworkVotingDataSourceImpl.ktshared/exception/src/main/java/team/aliens/dms/android/shared/exception/util/RunCatchingCancellable.ktshared/exception/src/main/java/team/aliens/dms/android/shared/exception/util/SuspendRunCatching.kt
π€ Files with no reviewable changes (1)
- shared/exception/src/main/java/team/aliens/dms/android/shared/exception/util/SuspendRunCatching.kt
Comment on lines
+24
to
+32
| return updateMeal(date).fold( | ||
| onSuccess = { Result.success(it) }, | ||
| onFailure = { exception -> | ||
| when (exception) { | ||
| is CannotSaveMealException -> Result.failure(exception) | ||
| else -> Result.failure(CannotSaveMealException()) | ||
| } | ||
| }, | ||
| ) |
There was a problem hiding this comment.
𧩠Analysis chain
π Script executed:
#!/bin/bash
# Description: Check if CannotSaveMealException accepts a cause parameter
# Find and display the CannotSaveMealException class definition
fd -t f -e kt "CannotSaveMealException" --exec cat {}Repository: team-aliens/DMS-Android
Length of output: 185
π Script executed:
#!/bin/bash
# Check the parent class MealException
fd -t f -e kt "MealException" --exec cat {}Repository: team-aliens/DMS-Android
Length of output: 430
π Script executed:
#!/bin/bash
# Find and display the updateMeal method implementation
rg -t kt "fun updateMeal" -A 20Repository: team-aliens/DMS-Android
Length of output: 93
π Script executed:
#!/bin/bash
# Find updateMeal method implementation
rg "fun updateMeal" -A 20Repository: team-aliens/DMS-Android
Length of output: 1953
π Script executed:
#!/bin/bash
# Find the MealRepositoryImpl file
fd -t f "MealRepositoryImpl.kt" --exec cat {}Repository: team-aliens/DMS-Android
Length of output: 2031
Handle CannotFindMealException separately instead of wrapping it as CannotSaveMealException.
When updateMeal() throws CannotFindMealException (line 42), it gets caught by the else clause and incorrectly wrapped as CannotSaveMealException(), misrepresenting the actual failure reason to callers and logs.
Proposed fix
return updateMeal(date).fold(
onSuccess = { Result.success(it) },
onFailure = { exception ->
when (exception) {
is CannotSaveMealException -> Result.failure(exception)
+ is CannotFindMealException -> Result.failure(exception)
- else -> Result.failure(CannotSaveMealException())
+ else -> Result.failure(CannotSaveMealException())
}
},
)π 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.
Suggested change
| return updateMeal(date).fold( | |
| onSuccess = { Result.success(it) }, | |
| onFailure = { exception -> | |
| when (exception) { | |
| is CannotSaveMealException -> Result.failure(exception) | |
| else -> Result.failure(CannotSaveMealException()) | |
| } | |
| }, | |
| ) | |
| return updateMeal(date).fold( | |
| onSuccess = { Result.success(it) }, | |
| onFailure = { exception -> | |
| when (exception) { | |
| is CannotSaveMealException -> Result.failure(exception) | |
| is CannotFindMealException -> Result.failure(exception) | |
| else -> Result.failure(CannotSaveMealException()) | |
| } | |
| }, | |
| ) |
π€ Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@data/src/main/kotlin/team/aliens/dms/android/data/meal/repository/MealRepositoryImpl.kt`
around lines 24 - 32, The fold in MealRepositoryImpl.kt currently maps any
non-CannotSaveMealException to a new CannotSaveMealException, which wrongly
wraps CannotFindMealException; change the onFailure handling for
updateMeal(date) so that if exception is CannotFindMealException you return
Result.failure(exception) (preserving the original exception), if it's
CannotSaveMealException return Result.failure(exception) as before, and only
wrap other unexpected exceptions as CannotSaveMealException(); reference the
updateMeal call and the exception types CannotFindMealException and
CannotSaveMealException when making this adjustment.
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.
κ°μ
μμ μ¬ν
μΆκ° λ‘ ν λ§
Summary by CodeRabbit