Feature/#78 login #83
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Walkthrough이 PR은 앱의 네비게이션과 로그인 기능 전반에 걸쳐 개선 사항을 도입합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant 사용자 as User
participant 화면 as LoginScreen
participant VM as LoginViewModel
participant UC as LoginUseCase
participant Repo as LoginRepositoryImpl
participant API as LoginApi
사용자->>화면: 로그인 버튼 클릭
화면->>VM: ClickLoginButton 인텐트 발생
VM->>UC: login(email, password) 호출
UC->>Repo: login(email, password) 요청
Repo->>API: POST /v1/auth/login (LoginRequest)
API-->>Repo: LoginResponse 반환
Repo-->>UC: 결과 전달
UC-->>VM: 결과 전달
VM->>화면: NavigateToHome 사이드 이펙트 발생
화면->>Navigator: 홈 화면으로 이동
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
core/data/src/main/java/com/yapp/core/data/remote/retrofit/ApiResultCallAdapter.kt
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/component/LoginInputSection.kt
Outdated
Show resolved
Hide resolved
|
@jinukeu |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🔭 Outside diff range comments (2)
feature/login/src/main/java/com/yapp/feature/login/LoginScreen.kt (2)
80-92: 🧹 Nitpick (assertive)접근성 개선이 필요합니다.
에러 메시지가 있는 입력 필드에 대해 TalkBack 사용자를 위한 접근성 설명을 추가하면 좋겠습니다.
LoginInputSection컴포넌트에 다음과 같은 접근성 속성을 추가해보세요:LoginInputSection( email = loginState.email, password = loginState.password, onEmailChange = { onIntent(LoginIntent.ChangeEmail(it)) }, onPasswordChange = { onIntent(LoginIntent.ChangePassword(it)) }, buttonEnable = loginState.enableLoginButton, onButtonClick = { keyboardController?.hide() onIntent(LoginIntent.ClickLoginButton) }, emailErrorDescription = loginState.emailErrorDescription, passwordErrorDescription = loginState.passwordErrorDescription, + modifier = Modifier.semantics { + if (loginState.emailErrorDescription != null) { + contentDescription = "이메일 입력 필드: ${loginState.emailErrorDescription}" + } + if (loginState.passwordErrorDescription != null) { + contentDescription = "비밀번호 입력 필드: ${loginState.passwordErrorDescription}" + } + } )
115-120: 🧹 Nitpick (assertive)프리뷰 기능을 개선해보세요.
현재 프리뷰는 기본적인 상태만 보여주고 있습니다. 다양한 상태(에러, 로딩 등)에 대한 프리뷰를 추가하면 좋겠습니다.
다음과 같이 다양한 상태의 프리뷰를 추가해보세요:
+@Preview(showBackground = true) +@Composable +private fun LoginScreenWithErrorPreview() { + YappTheme { + LoginScreen( + loginState = LoginState( + emailErrorDescription = "잘못된 이메일 형식입니다", + passwordErrorDescription = "비밀번호는 8자 이상이어야 합니다" + ) + ) + } +} +@Preview(showBackground = true) +@Composable +private fun LoginScreenWithDialogPreview() { + YappTheme { + LoginScreen( + loginState = LoginState( + showAgreementDialog = true + ) + ) + } +}
♻️ Duplicate comments (1)
feature/login/src/main/java/com/yapp/feature/login/LoginViewModel.kt (1)
91-115: 🛠️ Refactor suggestion에러 메시지 파싱 로직 개선 제안
문자열 검색(contains("이메일"),contains("올바르지"))은 서버 예외 메시지가 바뀌면 쉽게 깨질 수 있는 구조입니다. 서버로부터 명시적인ERROR_TYPE을 받거나, 에러 코드를 사용하는 방식을 사용하면 유지보수가 더 용이합니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (18)
app/src/main/java/com/yapp/app/official/navigation/YappNavHost.kt(1 hunks)core/data-api/src/main/java/com/yapp/dataapi/LoginRepository.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/data/di/RepositoryModule.kt(2 hunks)core/data/src/main/java/com/yapp/core/data/data/repository/LoginRepositoryImpl.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/remote/api/LoginApi.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/remote/di/ApiModule.kt(2 hunks)core/data/src/main/java/com/yapp/core/data/remote/model/request/LoginRequest.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/remote/model/response/LoginResponse.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/remote/retrofit/ApiResultCallAdapter.kt(1 hunks)core/data/src/main/java/com/yapp/core/data/remote/retrofit/OptionalApiResultCallAdapter.kt(1 hunks)core/domain/src/main/java/com/yapp/domain/LoginUseCase.kt(1 hunks)core/model/src/main/java/com/yapp/model/exceptions/YappExceptions.kt(1 hunks)feature/login/src/main/java/com/yapp/feature/login/LoginContract.kt(2 hunks)feature/login/src/main/java/com/yapp/feature/login/LoginScreen.kt(7 hunks)feature/login/src/main/java/com/yapp/feature/login/LoginViewModel.kt(1 hunks)feature/login/src/main/java/com/yapp/feature/login/component/AgreementBottomDialog.kt(5 hunks)feature/login/src/main/java/com/yapp/feature/login/component/LoginInputSection.kt(2 hunks)feature/login/src/main/java/com/yapp/feature/login/navigation/LoginNavigation.kt(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
core/data/src/main/java/com/yapp/core/data/remote/retrofit/OptionalApiResultCallAdapter.kt (1)
Learnt from: jinukeu
PR: YAPP-admin/yappu-world-android#30
File: core/data/src/main/java/com/yapp/core/data/remote/retrofit/OptionalApiResultCallAdapter.kt:21-24
Timestamp: 2025-01-24T12:06:26.854Z
Learning: The duplication between OptionalApiResultCall and ApiResultCall in the retrofit adapters is intentional to allow independent evolution of each adapter, despite their current structural similarities.
🔇 Additional comments (19)
core/model/src/main/java/com/yapp/model/exceptions/YappExceptions.kt (2)
13-16: 로그인 관련 예외 처리가 잘 구현되었습니다!각 예외 클래스에 명확하고 사용자 친화적인 오류 메시지가 포함되어 있으며, 오류 코드도 일관된 명명 규칙을 따르고 있습니다.
Also applies to: 24-27
30-39: 스레드 안전성 개선 필요
YappException클래스의 메시지 변경 기능이 스레드로부터 안전하지 않을 수 있습니다. 여러 스레드에서 동시에setMessage를 호출할 경우 경쟁 상태가 발생할 수 있습니다.다음 사항을 확인해 주세요:
- 메시지 변경이 스레드 안전해야 하는지 여부
- 필요한 경우
@Synchronized어노테이션 추가 또는 다른 동기화 메커니즘 적용 검토core/data/src/main/java/com/yapp/core/data/remote/retrofit/ApiResultCallAdapter.kt (1)
66-73: 오류 메시지 처리 개선이 잘 이루어졌습니다!공통 오류(COM_0001, COM_0002)에 대해 서버에서 전달된 메시지를 사용하도록 개선되었습니다. 이를 통해 더 구체적인 오류 정보를 사용자에게 전달할 수 있게 되었습니다.
참고:
OptionalApiResultCallAdapter와의 코드 중복은 의도적인 것으로, 각 어댑터의 독립적인 발전을 위해 유지되고 있습니다.core/data/src/main/java/com/yapp/core/data/remote/retrofit/OptionalApiResultCallAdapter.kt (1)
76-83: ApiResultCallAdapter와 일관된 오류 처리 구현이 잘 되었습니다!
ApiResultCallAdapter와 동일한 방식으로 오류 메시지 처리가 구현되어 있어 일관성이 잘 유지되었습니다.feature/login/src/main/java/com/yapp/feature/login/LoginContract.kt (1)
27-28: 약관 및 개인정보 처리방침 관련 Intent 구현이 잘 되었습니다!
ClickTerms와ClickPersonalPolicyIntent가 기존 패턴을 잘 따르고 있으며, 이름도 명확합니다.feature/login/src/main/java/com/yapp/feature/login/navigation/LoginNavigation.kt (1)
14-20: LGTM! 홈 화면 네비게이션 기능이 잘 추가되었습니다.로그인 성공 후 홈 화면으로 이동하는 네비게이션 기능이 깔끔하게 구현되었습니다.
app/src/main/java/com/yapp/app/official/navigation/YappNavHost.kt (1)
23-26: LGTM! 네비게이션 파라미터가 잘 추가되었습니다.LoginNavigation의 변경사항과 일관되게
navigateHome파라미터가 잘 추가되었습니다.feature/login/src/main/java/com/yapp/feature/login/component/LoginInputSection.kt (2)
29-35: LGTM! 에러 처리가 잘 구현되었습니다.이메일 입력 필드의 에러 상태와 설명이 적절하게 처리되었습니다.
59-68: 🧹 Nitpick (assertive)네임드 아규먼트 적용을 제안드립니다.
가독성 향상을 위해 네임드 아규먼트를 사용하는 것이 좋을 것 같습니다.
LoginInputSection( - "", - "", - {}, - {}, - false, - {}, - null, - null + email = "", + password = "", + onEmailChange = {}, + onPasswordChange = {}, + buttonEnable = false, + onButtonClick = {}, + emailErrorDescription = null, + passwordErrorDescription = null )feature/login/src/main/java/com/yapp/feature/login/component/AgreementBottomDialog.kt (1)
27-37: LGTM! 약관 및 개인정보처리방침 버튼 기능이 잘 구현되었습니다.이용약관과 개인정보처리방침으로의 이동 기능이 깔끔하게 추가되었습니다. PR 목적에 맞게 로그인 시 약관 동의 기능이 잘 구현되었습니다.
feature/login/src/main/java/com/yapp/feature/login/LoginScreen.kt (1)
3-5: 새로운 import 구문이 적절하게 추가되었습니다!필요한 Android 컴포넌트들과 Compose 관련 import가 깔끔하게 정리되어 있습니다.
Also applies to: 14-15, 22-22
feature/login/src/main/java/com/yapp/feature/login/LoginViewModel.kt (5)
4-4: 의존성 주입 설정이 적절해 보입니다.
이 로직 추가로viewModelScope및LoginUseCase등을 사용할 수 있어, 비동기 처리와 확장성을 확보하기에 적합합니다.Also applies to: 7-8, 10-10
14-16: Hilt 기반 ViewModel 주입 구조가 명확합니다.
LoginUseCase를 생성자에서 바로 주입하여, 테스트와 유지보수가 편해졌습니다.
20-21: MVI 구조 연결 확인
mviIntentStore초기화 시onIntent를 바인딩하는 방식은 유지보수에 유리합니다. 이후 인텐트 분기 처리를 추가할 때 유연하게 확장 가능합니다.
80-87: login 함수의 코루틴 사용 방식
viewModelScope.launch로 비동기 처리를 수행하는 방식이 옳습니다. 스코프가 ViewModel과 연동되어, 화면 전환 시점에 맞춰 job이 취소되어 리소스를 효율적으로 사용할 수 있습니다.
88-90: 로그인 성공 시 홈으로 이동
성공 시postSideEffect를 통해 내비게이션을 명확하게 분리한 점이 좋습니다. 로직의 가독성과 유지보수성이 높아집니다.core/data/src/main/java/com/yapp/core/data/remote/di/ApiModule.kt (1)
29-33: 구현이 잘 되었습니다!Dagger/Hilt의 모범 사례를 잘 따르고 있으며, 싱글톤 스코프와 의존성 주입이 적절하게 구현되어 있습니다.
core/data/src/main/java/com/yapp/core/data/data/di/RepositoryModule.kt (1)
5-5: 구현이 잘 되어있습니다!LoginRepository의 바인딩이 기존 Repository들과 일관된 패턴을 따르고 있으며, SingletonComponent 스코프와 @BINDS 어노테이션이 적절하게 사용되었습니다.
Also applies to: 9-9, 34-38
core/data/src/main/java/com/yapp/core/data/remote/model/response/LoginResponse.kt (1)
5-9: 기본 구현이 잘 되어있습니다!LoginResponse 클래스가 적절하게 internal로 표시되어 있고, kotlinx.serialization을 올바르게 사용하고 있습니다.
feature/login/src/main/java/com/yapp/feature/login/LoginContract.kt
Outdated
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/LoginScreen.kt
Outdated
Show resolved
Hide resolved
core/data/src/main/java/com/yapp/core/data/remote/model/request/LoginRequest.kt
Show resolved
Hide resolved
| override suspend fun login(email: String, password: String){ | ||
| val response = loginApi.login(LoginRequest(email = email, password = password)) | ||
| response.let { | ||
| securityPreferences.setAccessToken(it.accessToken) | ||
| securityPreferences.setRefreshToken(it.refreshToken) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
토큰 저장 로직을 개선하세요.
현재 구현에는 다음과 같은 개선이 필요합니다:
response.let블록이 불필요합니다. 직접response를 사용할 수 있습니다.- 토큰 저장 실패에 대한 오류 처리가 없습니다.
다음과 같이 개선하는 것을 제안합니다:
- override suspend fun login(email: String, password: String){
- val response = loginApi.login(LoginRequest(email = email, password = password))
- response.let {
- securityPreferences.setAccessToken(it.accessToken)
- securityPreferences.setRefreshToken(it.refreshToken)
- }
- }
+ override suspend fun login(email: String, password: String) {
+ val response = loginApi.login(LoginRequest(email = email, password = password))
+ try {
+ securityPreferences.setAccessToken(response.accessToken)
+ securityPreferences.setRefreshToken(response.refreshToken)
+ } catch (e: Exception) {
+ throw TokenStorageException("토큰 저장 실패", e)
+ }
+ }📝 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.
| override suspend fun login(email: String, password: String){ | |
| val response = loginApi.login(LoginRequest(email = email, password = password)) | |
| response.let { | |
| securityPreferences.setAccessToken(it.accessToken) | |
| securityPreferences.setRefreshToken(it.refreshToken) | |
| } | |
| } | |
| override suspend fun login(email: String, password: String) { | |
| val response = loginApi.login(LoginRequest(email = email, password = password)) | |
| try { | |
| securityPreferences.setAccessToken(response.accessToken) | |
| securityPreferences.setRefreshToken(response.refreshToken) | |
| } catch (e: Exception) { | |
| throw TokenStorageException("토큰 저장 실패", e) | |
| } | |
| } |
core/data/src/main/java/com/yapp/core/data/remote/model/response/LoginResponse.kt
Show resolved
Hide resolved
feature/login/src/main/java/com/yapp/feature/login/LoginViewModel.kt
Outdated
Show resolved
Hide resolved
| val errorMessage = it.message ?: "" | ||
| reduce{copy(isResponseLogin = false)} | ||
| when (it) { | ||
| is InvalidRequestArgument -> { |
There was a problem hiding this comment.
A: 아 비밀번호가 틀리면 COM 에러로 떨어지는군요 ,,, ?
There was a problem hiding this comment.
으음 ... 요건 서버쪽에서 바꿔주면 좋을 것 같은데 ... 일단 넘어가시죵
💡 Issue
🌱 Key changes
✅ To Reviewers
COM 으로 시작하는 API 공통 에러 Exeoption에 추가 해놓았고
해당 코드들만 message 바로 전달하도록 Adapter에 처리했습니다 ~
📸 스크린샷
KakaoTalk_Video_2025-02-18-17-39-18.mp4
Summary by CodeRabbit