[Refactor/#160] Kakao Login 리팩토링 - #161
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough카카오 전용 로그인 흐름을 Changes소셜 로그인 리팩터링
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LoginScreen
participant LoginViewModel
participant SocialLoginLauncher
participant KakaoLoginProvider
User->>LoginScreen: 소셜 로그인 버튼 클릭
LoginScreen->>LoginViewModel: OnSocialLoginClick 전달
LoginViewModel-->>LoginScreen: LaunchSocialLogin 효과
LoginScreen->>SocialLoginLauncher: login(context, socialType)
SocialLoginLauncher->>KakaoLoginProvider: 카카오 로그인 요청
KakaoLoginProvider-->>LoginScreen: SocialLoginResult 전달
LoginScreen->>LoginViewModel: OnSocialLoginResult 전달
LoginViewModel-->>LoginScreen: 서버 로그인 및 내비게이션 효과
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![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 (5)
app/src/test/java/com/poti/android/presentation/auth/LoginViewModelTest.kt (1)
166-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
userAuth헬퍼가 중복 정의되어 있습니다.동일한 로직의
userAuth(isNewUser: Boolean)가 outer 클래스 멤버(Line 166-171)와companion object(Line 206-211) 양쪽에 정의되어 있습니다.FakeAuthRepository가inner class가 아니라 outer 인스턴스 멤버에 접근할 수 없어 companion에도 복사본을 둔 것으로 보이는데, companion 멤버는 outer 클래스 본문에서 별도 한정자 없이도 접근 가능하므로 Line 166-171의 인스턴스 멤버는 불필요합니다. 하나만 남기면 두 구현이 어긋날 위험도 없어집니다.♻️ 제안하는 수정
- private fun userAuth(isNewUser: Boolean) = UserAuth( - accessToken = SERVER_ACCESS_TOKEN, - refreshToken = SERVER_REFRESH_TOKEN, - isNewUser = isNewUser, - userId = USER_ID, - ) - private class FakeAuthRepository : AuthRepository {
Companion의userAuth는 그대로 두면 outer 클래스의 다른 테스트 메서드(Line 124, 138)에서도 한정자 없이 그대로 호출됩니다.Also applies to: 200-212
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/test/java/com/poti/android/presentation/auth/LoginViewModelTest.kt` around lines 166 - 171, Remove the duplicate instance-level userAuth(isNewUser: Boolean) helper from the outer test class, and retain the existing Companion.userAuth implementation so both FakeAuthRepository and outer test methods resolve to the single shared helper.app/src/main/java/com/poti/android/data/repository/AuthRepositoryImpl.kt (1)
40-40: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win서버 전송값을
enum.name에 직접 의존하지 말아 주세요.
AuthService는 이 DTO를/api/v1/auth/login으로 바로 전송하므로, 서버가 요구하는 값의 대소문자나 코드가 enum 식별자와 다르면 로그인 요청이 실패합니다.SocialType별 명시적인 wire value 또는 mapper를 사용하고, 서버 계약 테스트로 실제 값을 검증해 주세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/poti/android/data/repository/AuthRepositoryImpl.kt` at line 40, Update AuthRepositoryImpl’s DTO mapping so socialType does not use SocialType.name directly; define or reuse an explicit wire value/mapper for each SocialType and send that value through AuthService. Add or update a server-contract test to verify the exact values sent to /api/v1/auth/login.app/src/main/java/com/poti/android/presentation/main/MainScreen.kt (1)
10-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Modifier를 외부에서 주입받도록 추가해 주세요.현재
MainScreen은Modifier를 파라미터로 노출하지 않고Scaffold에서Modifier.fillMaxSize()를 고정합니다.modifier: Modifier = Modifier를navigator보다 앞에 추가하고 Scaffold에 적용해 주세요.제안
fun MainScreen( targetDestination: Route, socialLoginLauncher: SocialLoginLauncher, + modifier: Modifier = Modifier, navigator: MainNavigator = rememberPotiNavigator(), ) { Scaffold( - modifier = Modifier.fillMaxSize(), + modifier = modifier.fillMaxSize(),As per path instructions:
Modifier는 항상 컴포저블 함수의 첫 번째 선택적 파라미터로 받아야 하며, 재사용성을 위해 외부에서 주입받아야 합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/poti/android/presentation/main/MainScreen.kt` around lines 10 - 17, MainScreen의 파라미터에 modifier: Modifier = Modifier를 navigator보다 앞에 추가하고, Scaffold의 고정된 Modifier.fillMaxSize() 대신 주입받은 modifier를 적용하세요. 컴포저블 함수의 첫 번째 선택적 파라미터라는 기존 순서를 유지하면서 외부 Modifier 주입이 가능하도록 수정합니다.Source: Path instructions
app/src/main/java/com/poti/android/presentation/auth/LoginScreen.kt (1)
83-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModifier가 필수 파라미터보다 앞에 위치했어요.
Compose 공식 가이드라인상
modifier는 모든 필수 파라미터(트레일링 람다 제외) 다음, 다른 선택 파라미터보다 앞에 와야 해요. 여기선 필수값인isLoginInProgress보다modifier가 먼저 선언되어 있네요.♻️ 제안하는 수정
private fun LoginScreen( - modifier: Modifier = Modifier, - isLoginInProgress: Boolean, + isLoginInProgress: Boolean, + modifier: Modifier = Modifier, onKakaoClick: () -> Unit, onGoogleClick: () -> Unit, ) {As per path instructions, "Modifier는 항상 컴포저블 함수의 첫 번째 선택적 파라미터로 받아야 하며".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/poti/android/presentation/auth/LoginScreen.kt` around lines 83 - 88, Reorder the LoginScreen parameters so the required isLoginInProgress parameter comes before the optional modifier parameter, keeping modifier as the first optional parameter and leaving the callback parameters unchanged.Source: Path instructions
app/src/main/java/com/poti/android/presentation/auth/LoginViewModel.kt (1)
69-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win성공 분기에서 상태 업데이트 중복 제거를 제안드려요.
isNewUser분기 양쪽에서updateState { copy(phase = LoginPhase.SUCCESS) }가 똑같이 반복되고 있어요. 분기 밖으로 빼면 DRY 원칙에도 맞고 가독성도 좋아질 것 같아요.♻️ 제안하는 리팩터
.onSuccess { response -> + updateState { copy(phase = LoginPhase.SUCCESS) } if (response.isNewUser) { Timber.i("신규 회원입니다. 온보딩 상태: 미완료(false)로 저장 -> 온보딩으로 이동") - updateState { copy(phase = LoginPhase.SUCCESS) } sendEffect(LoginEffect.NavigateToOnboarding) } else { Timber.i("기존 회원입니다. 온보딩 상태: 완료(true)로 저장 -> 홈으로 이동") - updateState { copy(phase = LoginPhase.SUCCESS) } sendEffect(LoginEffect.NavigateToHome) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/poti/android/presentation/auth/LoginViewModel.kt` around lines 69 - 86, In the success handling of the login flow, move the shared updateState call that sets LoginPhase.SUCCESS outside the response.isNewUser conditional. Keep each branch’s logging and navigation effect unchanged, with the state update executed once before selecting the destination.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/poti/android/data/auth/KakaoLoginProvider.kt`:
- Around line 38-46: Update the login flow around awaitLogin, startLogin, and
loginWithKakaoAccount to propagate the awaitLogin continuation’s active state.
Check that state before starting the Kakao account fallback and in each
authentication callback, preventing any result delivery or browser UI launch
after cancellation.
In `@app/src/main/java/com/poti/android/data/auth/KakaoSdkAuthClient.kt`:
- Around line 48-55: Update the result mapping in KakaoSdkAuthClient’s callback
so error handling precedes accessToken handling. Preserve the
ClientErrorCause.Cancelled mapping to KakaoAuthResult.Cancelled, map all other
non-null errors to Failure, and only then return Success for a non-null,
non-empty token; map null or blank tokens to InvalidResponse.
In `@app/src/main/java/com/poti/android/presentation/auth/LoginScreen.kt`:
- Around line 52-65: Update the LaunchSocialLogin handling around
socialLoginLauncher.login in the LoginScreen flow to catch non-cancellation
exceptions and convert them into the existing SocialLoginResult.Failure
representation before dispatching OnSocialLoginResult. Preserve the current
OnSocialLoginAborted behavior and rethrow CancellationException so coroutine
cancellation remains intact.
---
Nitpick comments:
In `@app/src/main/java/com/poti/android/data/repository/AuthRepositoryImpl.kt`:
- Line 40: Update AuthRepositoryImpl’s DTO mapping so socialType does not use
SocialType.name directly; define or reuse an explicit wire value/mapper for each
SocialType and send that value through AuthService. Add or update a
server-contract test to verify the exact values sent to /api/v1/auth/login.
In `@app/src/main/java/com/poti/android/presentation/auth/LoginScreen.kt`:
- Around line 83-88: Reorder the LoginScreen parameters so the required
isLoginInProgress parameter comes before the optional modifier parameter,
keeping modifier as the first optional parameter and leaving the callback
parameters unchanged.
In `@app/src/main/java/com/poti/android/presentation/auth/LoginViewModel.kt`:
- Around line 69-86: In the success handling of the login flow, move the shared
updateState call that sets LoginPhase.SUCCESS outside the response.isNewUser
conditional. Keep each branch’s logging and navigation effect unchanged, with
the state update executed once before selecting the destination.
In `@app/src/main/java/com/poti/android/presentation/main/MainScreen.kt`:
- Around line 10-17: MainScreen의 파라미터에 modifier: Modifier = Modifier를
navigator보다 앞에 추가하고, Scaffold의 고정된 Modifier.fillMaxSize() 대신 주입받은 modifier를
적용하세요. 컴포저블 함수의 첫 번째 선택적 파라미터라는 기존 순서를 유지하면서 외부 Modifier 주입이 가능하도록 수정합니다.
In `@app/src/test/java/com/poti/android/presentation/auth/LoginViewModelTest.kt`:
- Around line 166-171: Remove the duplicate instance-level userAuth(isNewUser:
Boolean) helper from the outer test class, and retain the existing
Companion.userAuth implementation so both FakeAuthRepository and outer test
methods resolve to the single shared helper.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 04fed822-9779-46c4-9635-04018ba64525
📒 Files selected for processing (29)
app/build.gradle.ktsapp/src/main/java/com/poti/android/core/auth/SocialLoginLauncher.ktapp/src/main/java/com/poti/android/core/auth/SocialLoginResult.ktapp/src/main/java/com/poti/android/data/auth/DefaultSocialLoginLauncher.ktapp/src/main/java/com/poti/android/data/auth/KakaoAuthClient.ktapp/src/main/java/com/poti/android/data/auth/KakaoAuthResult.ktapp/src/main/java/com/poti/android/data/auth/KakaoLoginProvider.ktapp/src/main/java/com/poti/android/data/auth/KakaoSdkAuthClient.ktapp/src/main/java/com/poti/android/data/di/SocialAuthModule.ktapp/src/main/java/com/poti/android/data/local/datasource/PreferenceDataSource.ktapp/src/main/java/com/poti/android/data/repository/AuthRepositoryImpl.ktapp/src/main/java/com/poti/android/domain/model/auth/SocialType.ktapp/src/main/java/com/poti/android/domain/repository/AuthRepository.ktapp/src/main/java/com/poti/android/domain/usecase/auth/LoginUseCase.ktapp/src/main/java/com/poti/android/presentation/auth/KakaoLoginManager.ktapp/src/main/java/com/poti/android/presentation/auth/LoginScreen.ktapp/src/main/java/com/poti/android/presentation/auth/LoginViewModel.ktapp/src/main/java/com/poti/android/presentation/auth/component/LoginButton.ktapp/src/main/java/com/poti/android/presentation/auth/model/Contracts.ktapp/src/main/java/com/poti/android/presentation/auth/model/LoginPhase.ktapp/src/main/java/com/poti/android/presentation/auth/navigation/AuthNavigation.ktapp/src/main/java/com/poti/android/presentation/main/MainActivity.ktapp/src/main/java/com/poti/android/presentation/main/MainNavHost.ktapp/src/main/java/com/poti/android/presentation/main/MainScreen.ktapp/src/test/java/com/poti/android/MainDispatcherRule.ktapp/src/test/java/com/poti/android/data/auth/DefaultSocialLoginLauncherTest.ktapp/src/test/java/com/poti/android/data/auth/KakaoLoginProviderTest.ktapp/src/test/java/com/poti/android/presentation/auth/LoginViewModelTest.ktgradle/libs.versions.toml
💤 Files with no reviewable changes (2)
- app/src/main/java/com/poti/android/presentation/auth/KakaoLoginManager.kt
- app/src/main/java/com/poti/android/data/local/datasource/PreferenceDataSource.kt
doyeon0307
approved these changes
Jul 23, 2026
Comment on lines
+54
to
+56
| if (result.shouldFallbackToKakaoAccount()) { | ||
| loginWithKakaoAccount(context, isActive, onResult) | ||
| } else { |
Contributor
There was a problem hiding this comment.
p2 : result.shouldFallbackToKakaoAccount가 true 고정이어서 카톡 로그인 실패 시에 항상 loginWithKakaoAccount로 넘아가게 되는데,
그러면 loginWithKakaoAccount의 결과만 남고 카톡 로그인의 실패 원인은 추적이 안되는 것 같아요.
이 지점에서 로그라도 심어주면 어떨까요?
| } | ||
| } | ||
|
|
||
| private fun KakaoAuthResult.Failure.shouldFallbackToKakaoAccount(): Boolean = true |
Contributor
There was a problem hiding this comment.
p2 : 여기 never used로 뜨네요!!
Suggested change
| private fun KakaoAuthResult.Failure.shouldFallbackToKakaoAccount(): Boolean = true | |
| private fun shouldFallbackToKakaoAccount(): Boolean = true |
Contributor
Author
There was a problem hiding this comment.
shouldFallbackToKakaoAccount 함수를 사용하지 않고 KakaoAuthResult.Failure로 분기처리 하도록 수정했습니다!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/test/java/com/poti/android/data/auth/KakaoLoginProviderTest.kt`:
- Around line 77-93: Replace the scheduling-dependent yield() in the
cancellation test with explicit synchronization using a CompletableDeferred or
equivalent signal from DeferredKakaoAuthClient when loginWithKakaoTalk is
entered and its callback is registered. Await that signal before cancelAndJoin,
then complete the KakaoTalk login and preserve the assertions that fallback is
not started.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 31710d8a-ef8d-46ee-90a7-1ece67cc4178
📒 Files selected for processing (4)
app/src/main/java/com/poti/android/data/auth/KakaoLoginProvider.ktapp/src/main/java/com/poti/android/data/auth/KakaoSdkAuthClient.ktapp/src/main/java/com/poti/android/presentation/auth/LoginScreen.ktapp/src/test/java/com/poti/android/data/auth/KakaoLoginProviderTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/poti/android/presentation/auth/LoginScreen.kt
- app/src/main/java/com/poti/android/data/auth/KakaoSdkAuthClient.kt
Comment on lines
+77
to
+93
| @Test | ||
| fun `does not start fallback after login coroutine is cancelled`() = runBlocking { | ||
| val client = DeferredKakaoAuthClient() | ||
| val provider = KakaoLoginProvider(client) | ||
| val loginJob = launch { | ||
| provider.login(context) | ||
| } | ||
| yield() | ||
|
|
||
| loginJob.cancelAndJoin() | ||
| client.completeKakaoTalkLogin( | ||
| KakaoAuthResult.Failure(RuntimeException("talk login failed")), | ||
| ) | ||
|
|
||
| assertEquals(1, client.kakaoTalkLoginCallCount) | ||
| assertEquals(0, client.kakaoAccountLoginCallCount) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
yield() 대신 로그인 시작을 명시적으로 동기화해 주세요.
yield()만으로는 kakaoTalkCallback 등록이 완료됐음을 보장하지 않습니다. 스케줄링에 따라 Line 87에서 requireNotNull이 실패해 테스트가 간헐적으로 깨질 수 있습니다. CompletableDeferred 등으로 loginWithKakaoTalk 진입을 대기한 뒤 취소해 주세요.
수정 예시
+import kotlinx.coroutines.CompletableDeferred
+import kotlinx.coroutines.awaitAll
- yield()
+ client.kakaoTalkLoginStarted.await()
private class DeferredKakaoAuthClient : KakaoAuthClient {
+ val kakaoTalkLoginStarted = CompletableDeferred<Unit>()
private var kakaoTalkCallback: ((KakaoAuthResult) -> Unit)? = null
override fun loginWithKakaoTalk(
context: Context,
onResult: (KakaoAuthResult) -> Unit,
) {
kakaoTalkLoginCallCount += 1
kakaoTalkCallback = onResult
+ kakaoTalkLoginStarted.complete(Unit)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/test/java/com/poti/android/data/auth/KakaoLoginProviderTest.kt`
around lines 77 - 93, Replace the scheduling-dependent yield() in the
cancellation test with explicit synchronization using a CompletableDeferred or
equivalent signal from DeferredKakaoAuthClient when loginWithKakaoTalk is
entered and its callback is registered. Await that signal before cancelAndJoin,
then complete the KakaoTalk login and preserve the assertions that fallback is
not started.
cmj7271
approved these changes
Jul 26, 2026
|
|
||
| @Composable | ||
| private fun LoginScreen( | ||
| modifier: Modifier = Modifier, |
Contributor
There was a problem hiding this comment.
p3: 소소하지만 modifier 가 맨앞에 있네요
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
3 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.
Related issue 🛠️
Work Description ✏️
Screenshot 📸
N/A
Uncompleted Tasks 😅
To Reviewers 📢
Summary by CodeRabbit