๐ :: (#835) ๋น๋ฐ๋ฒํธ ์ฌ์ค์ ๊ตฌํ#841
Merged
Conversation
๐ WalkthroughWalkthroughThis PR introduces a complete password reset feature for the DMS Android application. It adds a new navigation key, integrates a multi-step reset password screen with Jetpack Compose, includes a ViewModel managing server interactions and form validation, and wires the feature into the existing Sign In flow. Changes
Sequence DiagramsequenceDiagram
actor User
participant SignIn as SignIn Screen
participant ResetPW as ResetPassword Screen
participant ViewModel as ResetPassword ViewModel
participant AuthRepo as AuthRepository
participant StudentRepo as StudentRepository
participant Server as Server
User->>SignIn: Click "๋น๋ฐ๋ฒํธ ์ฌ์ค์ "
SignIn->>ResetPW: Navigate to Reset Password
rect rgba(100, 150, 255, 0.5)
Note over User,Server: Step 1: Check ID
User->>ResetPW: Enter account ID
User->>ResetPW: Click "๋ค์"
ResetPW->>ViewModel: moveNext(InputId)
ViewModel->>AuthRepo: checkIdExists(accountId)
AuthRepo->>Server: GET /check-id
Server-->>AuthRepo: {hashEmail, ...}
AuthRepo-->>ViewModel: Success with hashEmail
ViewModel->>ResetPW: Store hashEmail, advance to InputUserInfo
end
rect rgba(100, 150, 255, 0.5)
Note over User,Server: Step 2: Input User Info
User->>ResetPW: Enter name & email
User->>ResetPW: Click "๋ค์"
ResetPW->>ViewModel: moveNext(InputUserInfo)
ViewModel->>AuthRepo: sendEmailVerificationCode(email, PASSWORD)
AuthRepo->>Server: POST /send-verification-code
Server-->>AuthRepo: Success
AuthRepo-->>ViewModel: Success, start 5min timer
ViewModel->>ResetPW: Start countdown timer, advance to InputEmailVerificationCode
end
rect rgba(100, 150, 255, 0.5)
Note over User,Server: Step 3: Verify Email Code
User->>ResetPW: Enter verification code
User->>ResetPW: Click "๋ค์"
ResetPW->>ViewModel: moveNext(InputEmailVerificationCode)
ViewModel->>AuthRepo: checkEmailVerificationCode(code)
AuthRepo->>Server: POST /verify-code
Server-->>AuthRepo: Valid/Invalid response
AuthRepo-->>ViewModel: Success or error
ViewModel->>ResetPW: Advance to InputNewPassword or show error
end
rect rgba(100, 150, 255, 0.5)
Note over User,Server: Step 4: Reset Password
User->>ResetPW: Enter new password & confirm
User->>ResetPW: Click "์๋ฃ"
ResetPW->>ViewModel: moveNext(InputNewPassword)
ViewModel->>StudentRepo: resetPassword(newPassword)
StudentRepo->>Server: PUT /reset-password
Server-->>StudentRepo: Success/Error response
StudentRepo-->>ViewModel: Password updated
ViewModel->>ResetPW: Show success snackbar, navigate back
ResetPW->>SignIn: Navigate back to SignIn
end
Estimated code review effort๐ฏ 4 (Complex) | โฑ๏ธ ~45 minutes Possibly related PRs
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 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 (2)
feature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/ResetPasswordViewModel.kt (1)
31-44: Consider trimming whitespace from user inputs.The
accountIdis used directly from state without trimming. Users might accidentally include leading/trailing spaces. Consider trimming inputs before making API calls.โป๏ธ Optional improvement
private fun checkIdExists() = viewModelScope.launch { setState { it.copy(isLoading = true) } - authRepository.checkIdExists(accountId = uiState.value.accountId) + authRepository.checkIdExists(accountId = uiState.value.accountId.trim())Similar trimming could be applied to
nameand๐ค 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/resetpassword/ResetPasswordViewModel.kt` around lines 31 - 44, The checkIdExists coroutine reads accountId directly from uiState without trimming; before calling authRepository.checkIdExists in the checkIdExists function, trim whitespace from uiState.value.accountId (e.g., compute a local trimmedAccountId = uiState.value.accountId.trim()) and pass that to authRepository.checkIdExists(accountId = trimmedAccountId); also update any state uses (like setState or hashEmail assignment) or effects that should use the trimmed value so the repository call and subsequent behavior use the cleaned input.feature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/ResetPasswordScreen.kt (1)
66-73: Extract duplicated back-navigation branch into one handler.Line 66-73 and Line 139-145 implement the same logic; consolidating this avoids divergence later.
โป๏ธ Suggested refactor
+ val handleBack = { + if (currentStepIndex > 0) { + currentStepIndex-- + viewModel.onStepChanged(steps[currentStepIndex]) + } else { + onNavigateBack() + } + } + BackHandler { - if (currentStepIndex > 0) { - currentStepIndex-- - viewModel.onStepChanged(steps[currentStepIndex]) - } else { - onNavigateBack() - } + handleBack() } @@ - onBackClick = { - if (currentStepIndex > 0) { - currentStepIndex-- - viewModel.onStepChanged(steps[currentStepIndex]) - } else { - onNavigateBack() - } - }, + onBackClick = handleBack,Also applies to: 139-145
๐ค 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/resetpassword/ResetPasswordScreen.kt` around lines 66 - 73, Duplicate back-navigation logic in the BackHandler blocks should be extracted into a single helper to avoid divergence: create a function (e.g., handleBackNavigation or onBackPressed) that contains the current logic checking currentStepIndex, decrementing it, calling viewModel.onStepChanged(steps[currentStepIndex]) or calling onNavigateBack() when at index 0; then replace both BackHandler lambdas (the ones using currentStepIndex, viewModel.onStepChanged, steps, and onNavigateBack) with a single call to that helper to centralize behavior.
๐ค Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/ResetPasswordScreen.kt`:
- Around line 84-87: The handler for ResetPasswordSideEffect.MoveToNext
unconditionally increments currentStepIndex and then accesses
steps[currentStepIndex], which can throw IndexOutOfBoundsException; update the
logic to guard advancement by checking currentStepIndex < steps.lastIndex (or
compute nextIndex = min(currentStepIndex + 1, steps.lastIndex)) before
incrementing/using it, only call viewModel.onStepChanged(steps[nextIndex]) when
within bounds, and avoid advancing or handle completion when already on the
final step (references: ResetPasswordSideEffect.MoveToNext, currentStepIndex,
steps, viewModel.onStepChanged, ResetPasswordStep.InputEmailVerificationCode).
---
Nitpick comments:
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/ResetPasswordScreen.kt`:
- Around line 66-73: Duplicate back-navigation logic in the BackHandler blocks
should be extracted into a single helper to avoid divergence: create a function
(e.g., handleBackNavigation or onBackPressed) that contains the current logic
checking currentStepIndex, decrementing it, calling
viewModel.onStepChanged(steps[currentStepIndex]) or calling onNavigateBack()
when at index 0; then replace both BackHandler lambdas (the ones using
currentStepIndex, viewModel.onStepChanged, steps, and onNavigateBack) with a
single call to that helper to centralize behavior.
In
`@feature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/ResetPasswordViewModel.kt`:
- Around line 31-44: The checkIdExists coroutine reads accountId directly from
uiState without trimming; before calling authRepository.checkIdExists in the
checkIdExists function, trim whitespace from uiState.value.accountId (e.g.,
compute a local trimmedAccountId = uiState.value.accountId.trim()) and pass that
to authRepository.checkIdExists(accountId = trimmedAccountId); also update any
state uses (like setState or hashEmail assignment) or effects that should use
the trimmed value so the repository call and subsequent behavior use the cleaned
input.
โน๏ธ Review info
โ๏ธ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 589c59e8-4dfe-4972-8b82-80b2bb5720b8
๐ Files selected for processing (12)
app/src/main/kotlin/team/aliens/dms/android/app/navigation/DmsNavKeys.ktapp/src/main/kotlin/team/aliens/dms/android/app/ui/DmsApp.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/ResetPasswordScreen.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/ResetPasswordViewModel.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/component/EmailVerificationContent.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/component/InputIdContent.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/component/InputNewPasswordContent.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/component/InputUserInfoContent.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/model/ResetPasswordTextFieldError.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/resetpassword/navigation/ResetPasswordNavigation.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/signin/navigation/SignInRoute.ktfeature/src/main/kotlin/team/aliens/dms/android/feature/signin/ui/SigninScreen.kt
Comment on lines
+84
to
+87
| is ResetPasswordSideEffect.MoveToNext -> { | ||
| currentStepIndex++ | ||
| viewModel.onStepChanged(steps[currentStepIndex]) | ||
| if (steps[currentStepIndex] == ResetPasswordStep.InputEmailVerificationCode) { |
There was a problem hiding this comment.
Guard step advancement to prevent step-index overflow crash.
Line 85 increments first, then Line 86/87 dereference steps[currentStepIndex] without a bound check. If MoveToNext arrives on the final step, this can throw IndexOutOfBoundsException.
๐ก Suggested fix
is ResetPasswordSideEffect.MoveToNext -> {
- currentStepIndex++
- viewModel.onStepChanged(steps[currentStepIndex])
- if (steps[currentStepIndex] == ResetPasswordStep.InputEmailVerificationCode) {
- timerRunning = true
- }
+ if (currentStepIndex < steps.lastIndex) {
+ currentStepIndex++
+ val nextStep = steps[currentStepIndex]
+ viewModel.onStepChanged(nextStep)
+ if (nextStep == ResetPasswordStep.InputEmailVerificationCode) {
+ timerRunning = true
+ }
+ }
}๐ค 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/resetpassword/ResetPasswordScreen.kt`
around lines 84 - 87, The handler for ResetPasswordSideEffect.MoveToNext
unconditionally increments currentStepIndex and then accesses
steps[currentStepIndex], which can throw IndexOutOfBoundsException; update the
logic to guard advancement by checking currentStepIndex < steps.lastIndex (or
compute nextIndex = min(currentStepIndex + 1, steps.lastIndex)) before
incrementing/using it, only call viewModel.onStepChanged(steps[nextIndex]) when
within bounds, and avoid advancing or handle completion when already on the
final step (references: ResetPasswordSideEffect.MoveToNext, currentStepIndex,
steps, viewModel.onStepChanged, ResetPasswordStep.InputEmailVerificationCode).
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
1 participant
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