refactor: 세션 만료 및 로그아웃 처리를 AuthState 기반으로 재구현#88
Open
Jooman-Lee wants to merge 2 commits intodevelopfrom
Open
Conversation
- SessionEventBus(domain 인터페이스) 제거 — domain이 인프라 배관 역할을 하던 문제 해결 - AuthState sealed interface 추가 (Authenticated / Unauthenticated / SessionExpired) - TokenProvider에 getAuthStateFlow(), expireSession() 추가 - TokenProviderImpl: clearToken() → Unauthenticated, expireSession() → SessionExpired emit - TokenRefresher: clearToken() 대신 expireSession() 호출 - AppViewModel 신설: Channel(CONFLATED)로 일회성 AuthNavEvent 변환 - PickleApp composable 신설: AppViewModel 구독, 로그인 이동 + Snackbar 처리 - MainActivity 단순화: PickleApp() 호출만 존재, domain 직접 의존 제거 - GlobalNavEvent 삭제, onGlobalNavEvent 콜백 drill-down 전체 제거 - app/build.gradle.kts에서 :domain 의존성 제거 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughGlobalNavEvent 기반 전역 네비게이션을 제거하고, AuthState 중심의 세션 상태 모델을 추가했습니다. TokenProvider API와 구현에 세션 만료 흐름이 추가되었고, AppViewModel 및 PickleApp에서 인증 상태를 구독해 네비게이션·스낵바 처리를 통합합니다. UI 컴포저블의 전역 콜백은 제거되었습니다. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45분 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request centralizes authentication-related navigation and session management by introducing a global AuthState flow. It adds an AppViewModel to observe this state and a top-level PickleApp composable to handle navigation events and snackbar notifications, effectively removing manual navigation logic from individual screens. Feedback focuses on ensuring the SnackbarHost overlays correctly by using a Box layout in PickleApp and suggests refactoring TokenProviderImpl to reduce code duplication between the token clearing and session expiration logic.
Comment on lines
+36
to
+37
| PickleNavHost(navController = navController) | ||
| SnackbarHost(snackbarState) |
There was a problem hiding this comment.
PickleNavHost와 SnackbarHost가 Box와 같은 레이아웃 컨테이너 없이 함께 배치되었습니다. SnackbarHost가 화면 콘텐츠 위에 오버레이되어야 한다면, 두 컴포저블을 Box로 감싸야 합니다. 그렇지 않으면 SnackbarHost가 PickleNavHost 아래에 표시되는 등 의도치 않은 UI가 발생할 수 있습니다.
참고: import androidx.compose.foundation.layout.Box 추가가 필요할 수 있습니다.
Box {
PickleNavHost(navController = navController)
SnackbarHost(snackbarState)
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
data/src/main/java/com/smtm/pickle/data/source/local/provider/TokenProviderImpl.kt (2)
39-49:clearToken()과expireSession()의 공통 로직 추출을 고려해 볼 수 있습니다.두 함수 모두 토큰 클리어 로직이 동일하고, 상태 값만 다릅니다. 현재 코드가 충분히 간결하지만, 향후 토큰 클리어 로직이 복잡해질 경우를 대비해 내부 헬퍼로 추출하는 것도 방법입니다.
♻️ 공통 로직 추출 예시 (선택적)
+ private suspend fun clearTokenInternal() { + cachedToken = null + tokenDataStore.clearToken() + } + override suspend fun clearToken() { - cachedToken = null - tokenDataStore.clearToken() + clearTokenInternal() _authState.value = AuthState.Unauthenticated } override suspend fun expireSession() { - cachedToken = null - tokenDataStore.clearToken() + clearTokenInternal() _authState.value = AuthState.SessionExpired }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/src/main/java/com/smtm/pickle/data/source/local/provider/TokenProviderImpl.kt` around lines 39 - 49, Extract the duplicated token-clearing logic in clearToken() and expireSession() into a single private helper (e.g., clearTokenInternal or performTokenClear) that sets cachedToken = null and calls tokenDataStore.clearToken(), then have clearToken() and expireSession() call that helper and only set _authState to AuthState.Unauthenticated or AuthState.SessionExpired respectively; ensure you keep the same suspend semantics and that the helper is invoked before updating _authState to preserve current behavior.
21-25: 초기_authState값이 실제 토큰 존재 여부를 반영하지 않습니다.
_authState가AuthState.Authenticated로 초기화되지만,init()함수에서 토큰 존재 여부에 따라 상태를 업데이트하지 않습니다. 토큰이 없는 상태(신규 설치, 로그아웃 후 앱 재시작)에서도Authenticated상태로 남게 됩니다.현재 네비게이션 흐름상
AuthGraphRoute가 시작점이라 즉각적인 문제는 없을 수 있지만, 향후Authenticated상태를 신뢰하는 로직이 추가될 경우 버그가 발생할 수 있습니다.♻️ init()에서 토큰 존재 여부에 따라 상태 설정 제안
override suspend fun init() { cachedToken = tokenDataStore.getToken() + _authState.value = if (cachedToken != null) { + AuthState.Authenticated + } else { + AuthState.Unauthenticated + } }또는 초기값을 별도의 상태(예:
Loading또는Unknown)로 두고init()후에 실제 상태로 전환하는 방법도 고려해 볼 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@data/src/main/java/com/smtm/pickle/data/source/local/provider/TokenProviderImpl.kt` around lines 21 - 25, 현재 _authState가 AuthState.Authenticated로 하드코딩되어 있어 실제 토큰 존재 여부와 불일치합니다; 수정 방법은 init()에서 tokenDataStore.getToken() 결과에 따라 _authState를 설정하거나(예: set to AuthState.Authenticated if cachedToken non-null else AuthState.Unauthenticated), 또는 초기값을 AuthState.Loading/Unknown으로 변경한 뒤 init()에서 cachedToken을 읽고 _authState를 실제 상태로 전환하도록 변경하세요; 참고 심볼: _authState, AuthState, init(), cachedToken, tokenDataStore.getToken().presentation/src/main/java/com/smtm/pickle/presentation/AppViewModel.kt (1)
24-27:trySend()결과를 확인하지 않고 있습니다.현재
CONFLATED채널에서는 일반적으로trySend()가 성공하지만, 디버깅을 위해 실패 시 로그를 남기는 것을 고려해 볼 수 있습니다.♻️ 실패 로깅 추가 예시 (선택적)
when (state) { - AuthState.SessionExpired -> _authNavEvent.trySend(AuthNavEvent.ToLoginWithMessage) - AuthState.Unauthenticated -> _authNavEvent.trySend(AuthNavEvent.ToLogin) + AuthState.SessionExpired -> { + val result = _authNavEvent.trySend(AuthNavEvent.ToLoginWithMessage) + if (result.isFailure) { + // Log or handle failure + } + } + AuthState.Unauthenticated -> { + val result = _authNavEvent.trySend(AuthNavEvent.ToLogin) + if (result.isFailure) { + // Log or handle failure + } + } else -> {} }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/com/smtm/pickle/presentation/AppViewModel.kt` around lines 24 - 27, The trySend() calls on _authNavEvent inside AppViewModel's when(state) (sending AuthNavEvent.ToLoginWithMessage and AuthNavEvent.ToLogin) ignore the ChannelResult; update these to capture the result (e.g., val result = _authNavEvent.trySend(...)) and check for failure (result.isFailure / result.exceptionOrNull()) and log a descriptive message if it fails so we get debugging information when the CONFLATED channel send doesn't succeed.presentation/src/main/java/com/smtm/pickle/presentation/PickleApp.kt (2)
36-37: Snackbar 오버레이를 위해Box로 감싸는 것을 고려해 주세요.현재
PickleNavHost와SnackbarHost가 부모 컨테이너 없이 순차적으로 배치되어 있습니다. Compose의 기본 동작상 오버레이되긴 하지만,Box나Scaffold로 명시적으로 감싸면 의도가 더 명확해지고 유지보수성이 향상됩니다.♻️ Box를 활용한 개선 제안
+import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.fillMaxSize +import androidx.compose.ui.Modifier- PickleNavHost(navController = navController) - SnackbarHost(snackbarState) + Box(modifier = Modifier.fillMaxSize()) { + PickleNavHost(navController = navController) + SnackbarHost(snackbarState) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/com/smtm/pickle/presentation/PickleApp.kt` around lines 36 - 37, Wrap the sequential PickleNavHost(navController = navController) and SnackbarHost(snackbarState) in a parent Box (e.g., Box(modifier = Modifier.fillMaxSize())) so the snackbar is an explicit overlay; inside the Box place PickleNavHost first and then SnackbarHost with a BoxScope alignment (e.g., align(Alignment.BottomCenter)) so the SnackbarHost overlays the nav content as intended. Ensure you update the surrounding composable to import and use Box and Modifier and keep the existing PickleNavHost and snackbarState references unchanged.
23-34:when표현식으로 이벤트 처리를 명시적으로 구성하는 것을 제안드립니다.현재 모든 이벤트에 대해 로그인 화면으로의 네비게이션이 무조건 실행됩니다. 향후
AuthNavEvent에 로그인 네비게이션이 필요 없는 새로운 타입이 추가될 경우, 의도치 않은 동작이 발생할 수 있습니다.when표현식을 사용하면 각 이벤트 타입별 처리를 명확히 하고, sealed class/interface의 exhaustive check 이점도 활용할 수 있습니다.♻️ when 표현식을 활용한 개선 제안
LaunchedEffect(Unit) { appViewModel.authNavEvent.collect { event -> - navController.navigate(LoginRoute) { - popUpTo(navController.graph.id) { inclusive = true } - } - if (event is AuthNavEvent.ToLoginWithMessage) { - snackbarState.show( - PickleSnackbar.snackbarShort(message = sessionExpiredMessage) - ) + when (event) { + is AuthNavEvent.ToLogin -> { + navController.navigate(LoginRoute) { + popUpTo(navController.graph.id) { inclusive = true } + } + } + is AuthNavEvent.ToLoginWithMessage -> { + navController.navigate(LoginRoute) { + popUpTo(navController.graph.id) { inclusive = true } + } + snackbarState.show( + PickleSnackbar.snackbarShort(message = sessionExpiredMessage) + ) + } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@presentation/src/main/java/com/smtm/pickle/presentation/PickleApp.kt` around lines 23 - 34, The current collect block inside LaunchedEffect unconditionally calls navController.navigate for every AuthNavEvent, which can cause incorrect behavior when new event types are added; change the lambda handling appViewModel.authNavEvent.collect to use a when (event) { ... } expression over the sealed AuthNavEvent and handle each case explicitly (e.g., for AuthNavEvent.ToLogin and AuthNavEvent.ToLoginWithMessage perform navController.navigate(LoginRoute) with popUpTo(navController.graph.id) { inclusive = true }, and for ToLoginWithMessage also call snackbarState.show(PickleSnackbar.snackbarShort(message = sessionExpiredMessage)); ensure other AuthNavEvent variants are handled or explicitly ignored so future additions won't trigger unintended navigation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@data/src/main/java/com/smtm/pickle/data/source/local/provider/TokenProviderImpl.kt`:
- Around line 39-49: Extract the duplicated token-clearing logic in clearToken()
and expireSession() into a single private helper (e.g., clearTokenInternal or
performTokenClear) that sets cachedToken = null and calls
tokenDataStore.clearToken(), then have clearToken() and expireSession() call
that helper and only set _authState to AuthState.Unauthenticated or
AuthState.SessionExpired respectively; ensure you keep the same suspend
semantics and that the helper is invoked before updating _authState to preserve
current behavior.
- Around line 21-25: 현재 _authState가 AuthState.Authenticated로 하드코딩되어 있어 실제 토큰 존재
여부와 불일치합니다; 수정 방법은 init()에서 tokenDataStore.getToken() 결과에 따라 _authState를
설정하거나(예: set to AuthState.Authenticated if cachedToken non-null else
AuthState.Unauthenticated), 또는 초기값을 AuthState.Loading/Unknown으로 변경한 뒤 init()에서
cachedToken을 읽고 _authState를 실제 상태로 전환하도록 변경하세요; 참고 심볼: _authState, AuthState,
init(), cachedToken, tokenDataStore.getToken().
In `@presentation/src/main/java/com/smtm/pickle/presentation/AppViewModel.kt`:
- Around line 24-27: The trySend() calls on _authNavEvent inside AppViewModel's
when(state) (sending AuthNavEvent.ToLoginWithMessage and AuthNavEvent.ToLogin)
ignore the ChannelResult; update these to capture the result (e.g., val result =
_authNavEvent.trySend(...)) and check for failure (result.isFailure /
result.exceptionOrNull()) and log a descriptive message if it fails so we get
debugging information when the CONFLATED channel send doesn't succeed.
In `@presentation/src/main/java/com/smtm/pickle/presentation/PickleApp.kt`:
- Around line 36-37: Wrap the sequential PickleNavHost(navController =
navController) and SnackbarHost(snackbarState) in a parent Box (e.g.,
Box(modifier = Modifier.fillMaxSize())) so the snackbar is an explicit overlay;
inside the Box place PickleNavHost first and then SnackbarHost with a BoxScope
alignment (e.g., align(Alignment.BottomCenter)) so the SnackbarHost overlays the
nav content as intended. Ensure you update the surrounding composable to import
and use Box and Modifier and keep the existing PickleNavHost and snackbarState
references unchanged.
- Around line 23-34: The current collect block inside LaunchedEffect
unconditionally calls navController.navigate for every AuthNavEvent, which can
cause incorrect behavior when new event types are added; change the lambda
handling appViewModel.authNavEvent.collect to use a when (event) { ... }
expression over the sealed AuthNavEvent and handle each case explicitly (e.g.,
for AuthNavEvent.ToLogin and AuthNavEvent.ToLoginWithMessage perform
navController.navigate(LoginRoute) with popUpTo(navController.graph.id) {
inclusive = true }, and for ToLoginWithMessage also call
snackbarState.show(PickleSnackbar.snackbarShort(message =
sessionExpiredMessage)); ensure other AuthNavEvent variants are handled or
explicitly ignored so future additions won't trigger unintended navigation).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 269d8746-2cc9-4c5f-bf46-c29637e73a98
📒 Files selected for processing (15)
app/build.gradle.ktsapp/src/main/java/com/smtm/pickle/MainActivity.ktdata/src/main/java/com/smtm/pickle/data/source/local/provider/TokenProviderImpl.ktdata/src/main/java/com/smtm/pickle/data/source/remote/auth/TokenRefresher.ktdomain/src/main/java/com/smtm/pickle/domain/model/auth/AuthState.ktdomain/src/main/java/com/smtm/pickle/domain/provider/TokenProvider.ktpresentation/src/main/java/com/smtm/pickle/presentation/AppViewModel.ktpresentation/src/main/java/com/smtm/pickle/presentation/PickleApp.ktpresentation/src/main/java/com/smtm/pickle/presentation/main/MainScreen.ktpresentation/src/main/java/com/smtm/pickle/presentation/mypage/navigation/MyPageNavigation.ktpresentation/src/main/java/com/smtm/pickle/presentation/navigation/GlobalNavEvent.ktpresentation/src/main/java/com/smtm/pickle/presentation/navigation/PickleNavHost.ktpresentation/src/main/java/com/smtm/pickle/presentation/setting/SettingScreen.ktpresentation/src/main/java/com/smtm/pickle/presentation/setting/SettingViewModel.ktpresentation/src/main/res/values/strings.xml
💤 Files with no reviewable changes (6)
- app/build.gradle.kts
- presentation/src/main/java/com/smtm/pickle/presentation/mypage/navigation/MyPageNavigation.kt
- presentation/src/main/java/com/smtm/pickle/presentation/setting/SettingScreen.kt
- presentation/src/main/java/com/smtm/pickle/presentation/navigation/GlobalNavEvent.kt
- presentation/src/main/java/com/smtm/pickle/presentation/main/MainScreen.kt
- presentation/src/main/java/com/smtm/pickle/presentation/setting/SettingViewModel.kt
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.
♟️ Issue
✨ 주요 변경 사항
AuthState추가:TokenProvider에 인증 상태 Flow를 도입하여 로그아웃/세션 만료를 상태로 표현Authenticated/Unauthenticated/SessionExpired세 가지 상태로 구분clearToken()호출 시Unauthenticated,expireSession()호출 시SessionExpiredemitAppViewModel신설:AuthState변화를Channel(CONFLATED)기반의 일회성AuthNavEvent로 변환Channel.CONFLATED선택 이유: 복수의 401 응답이 동시에 오더라도 로그인 이동은 단 한 번만 실행되어야 하므로PickleAppcomposable 신설: 앱 전체 관심사(네비게이션, Snackbar)를 담당하는 최상위 ComposableMainActivity는PickleApp()호출만 담당하도록 단순화GlobalNavEvent및onGlobalNavEvent콜백 drill-down 전체 제거PickleNavHost → MainScreen → MyPageNavigation → SettingScreen까지 콜백 전파AppViewModel이AuthState를 구독하여 네비게이션 이벤트를 직접 처리SettingViewModel에서NavigateToLoginEffect 제거AuthState변화가 자동으로 처리app모듈에서:domain의존성 제거MainActivity가 domain 인터페이스를 직접 참조하던 구조 해소✅ 체크리스트
🔍 중점 리뷰 사항
기존
GlobalNavEvent설계를 제거한 이유:GlobalNavEvent는 원래 사용자 인터랙션에서 시작되는 네비게이션을 상위로 위임하는 용도로 설계됐는데, 세션 만료는 data 레이어(TokenRefresher)에서 시작되는 이벤트라 이 패턴에 맞지 않습니다. 억지로 끼워 맞추다 보니SessionEventBus가 domain에 들어가고MainActivity가 domain을 직접 참조하는 문제가 생겨 방향을 전환했습니다. ([Feature] 로그아웃 및 세션 만료 GlobalNavEvent 적용 #87 PR 참고)State vs Event 레이어 분리:
TokenProvider(data)는AuthStateFlow로 상태를 노출하고,AppViewModel(presentation)이 이를Channel로 일회성 이벤트로 변환합니다. 각 레이어가 자신의 책임에 맞는 방식을 사용합니다.SettingViewModel의 로그아웃 성공 처리: 기존에는onSuccess에서NavigateToLogineffect를 emit했지만, 이제는LogoutUseCase → clearToken() → AuthState.Unauthenticated흐름으로 자동 처리됩니다. ViewModel에서 명시적으로 네비게이션을 트리거하지 않아도 됩니다.📸 스크린샷