SCRUM-272 feature: implement auto login logic#46
Conversation
Route token validation and reissue through the auth data layer so startup session restore and runtime 401 handling share the same refresh and token cleanup policy.
Add a SplashScreen-backed MainViewModel that checks the saved session before rendering navigation to avoid showing the login screen first.
Route startup session checks through a Compose splash screen so auth failure handling can show the login dialog while runtime token refresh keeps transient failures recoverable.
Pass startup token refresh errors to the login screen so users can see the session expiration reason when automatic login fails.
📝 WalkthroughWalkthroughAdds Android splash screen and theme, installs the splash at app startup, implements AuthTokenRefresher with concurrent-safe token refresh and error mapping, extends remote auth API, updates session restoration to return typed results, and updates navigation/UI to show splash and surface auto-login failures. ChangesSplash Screen and Session Restoration
Sequence DiagramsequenceDiagram
participant MainActivity
participant SplashScreen
participant SplashViewModel
participant AuthRepository
participant AuthTokenRefresher
participant AuthRemoteDataSource
MainActivity->>SplashScreen: render splash at startup
SplashScreen->>SplashViewModel: observeUiState
SplashViewModel->>AuthRepository: restoreSession()
AuthRepository->>AuthTokenRefresher: refreshServerToken(staleToken)
AuthTokenRefresher->>AuthRemoteDataSource: validateToken() or reIssueToken(refreshToken)
AuthRemoteDataSource-->>AuthTokenRefresher: validation or new token / error
alt token valid
AuthTokenRefresher-->>AuthRepository: Result.success(token)
AuthRepository-->>SplashViewModel: RestoreSessionResult.Authenticated
SplashViewModel-->>SplashScreen: NavigateToMain
else refresh succeeds
AuthTokenRefresher-->>AuthRepository: Result.success(newToken)
AuthRepository-->>SplashViewModel: RestoreSessionResult.Authenticated
SplashViewModel-->>SplashScreen: NavigateToMain
else refresh fails
AuthTokenRefresher-->>AuthRepository: Result.failure(error)
AuthRepository-->>SplashViewModel: RestoreSessionResult.Failed(errorCode)
SplashViewModel-->>SplashScreen: NavigateToLogin(errorCode)
else no tokens
AuthRepository-->>SplashViewModel: RestoreSessionResult.Unauthenticated
SplashViewModel-->>SplashScreen: NavigateToLogin(null)
end
SplashScreen->>SplashScreen: navigate and clear back stack
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
아오... AI 에이전트가 Github CLI 이용해서 PR을 처음 만들때 PR 제목이랑 본문이 제대로 안 들어갔었습니다. 그래서 해당 내용을 한번 수정했어요. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 594e9fae77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/main/java/com/lyrics/feelin/presentation/view/login/LoginScreen.kt (1)
54-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove lambda parameters to the end of the parameter list.
The function signature has lambda parameters (
onSignUp,onContinueToMain) before non-lambda parameters (autoLoginFailedErrorCode,loginViewModel). As per coding guidelines, lambda parameters should be the last parameters in Composable functions.📝 Proposed fix
`@Composable` fun LoginScreen( - onSignUp: () -> Unit, - onContinueToMain: () -> Unit, modifier: Modifier = Modifier, autoLoginFailedErrorCode: String? = null, - loginViewModel: LoginViewModel = hiltViewModel<LoginViewModel>() + loginViewModel: LoginViewModel = hiltViewModel<LoginViewModel>(), + onSignUp: () -> Unit, + onContinueToMain: () -> Unit ) {As per coding guidelines, "Keep lambda parameters as the last parameter in Composable functions".
🤖 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/lyrics/feelin/presentation/view/login/LoginScreen.kt` around lines 54 - 61, The LoginScreen Composable declares lambda params onSignUp and onContinueToMain before non-lambda params (autoLoginFailedErrorCode, loginViewModel); reorder the function signature in LoginScreen so that non-lambda parameters (modifier, autoLoginFailedErrorCode, loginViewModel) come first and the lambda parameters onSignUp and onContinueToMain are the final parameters, updating any call sites as needed to match the new parameter order.
🧹 Nitpick comments (3)
app/src/main/java/com/lyrics/feelin/core/data/repository/AuthRepository.kt (1)
82-101: 🏗️ Heavy liftConsider the UX impact of clearing tokens on all startup failures.
The current implementation clears stored tokens for any refresh failure during startup, including transient network errors. This means users with valid sessions but poor network connectivity at app launch will be forced to re-login, even though their session might still be valid.
While the comment on lines 82-88 indicates this is an intentional architectural decision to ensure definitive session state before showing the main screen, you may want to consider:
- Showing a retry option for network errors instead of forcing immediate logout
- Differentiating between network failures and authentication failures in the startup flow
- Providing user feedback about why they need to re-login (network vs. expired session)
🤖 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/lyrics/feelin/core/data/repository/AuthRepository.kt` around lines 82 - 101, The startup flow currently unconditionally clears tokens when restoreSessionWithRefresh() sees any refresh failure from authTokenRefresher.refreshServerToken(), which logs out users even on transient network errors; change restoreSessionWithRefresh() to distinguish network errors from auth errors by inspecting exception.toServerErrorCode() or exception type, and only call authManager.clearTokens() and return RestoreSessionResult.Failed for definitive auth failures (e.g., invalid/expired token codes), while for network/transient errors return a new retryable result (or a distinct RestoreSessionResult.RetryNetwork) so the caller can show a retry option or a user-facing message instead of forcing logout. Ensure CancellationException is still rethrown and keep existing RestoreSessionResult.Authenticated on success.app/src/main/java/com/lyrics/feelin/presentation/view/login/LoginScreen.kt (1)
68-78: 💤 Low valueConsider extracting hardcoded Korean dialog text to string resources.
The dialog title and description contain hardcoded Korean strings. If internationalization is planned, these should be moved to string resources. If the app is Korea-only, this can be deferred.
🤖 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/lyrics/feelin/presentation/view/login/LoginScreen.kt` around lines 68 - 78, The dialog in the LoginScreen Composable uses hardcoded Korean text; move the title and description strings into string resources and reference them from the Composable (e.g., replace the literal title and description passed to FeelinModalDialog with values obtained via stringResource or context.getString), formatting the description to include autoLoginFailedErrorCode/UNKNOWN_AUTO_LOGIN_ERROR_CODE; update any tests or previews to supply the resources if needed and keep the same behavior for isAutoLoginFailedDialogVisible and onConfirmButtonClick.app/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.kt (1)
91-122: 💤 Low valueVerify destination constants and consider reducing duplication.
The login screen now supports two routes: standard login and login with auto-login failure error code.
Ensure the following constants are defined in
FeelinDestination.Login:
ROUTE_WITH_AUTO_LOGIN_FAILED_ERROR_CODEAUTO_LOGIN_FAILED_ERROR_CODE_ARGUMENTThere is code duplication between lines 93-96 and 109-112 (parent entry and viewModel retrieval). While the current approach is clear, consider extracting this logic if similar patterns appear elsewhere in the navigation code.
Run the following script to verify the constants exist:
#!/bin/bash # Verify Login destination constants exist rg -nP --type=kt 'ROUTE_WITH_AUTO_LOGIN_FAILED_ERROR_CODE|AUTO_LOGIN_FAILED_ERROR_CODE_ARGUMENT' app/src/main/java/com/lyrics/feelin/navigation/FeelinDestination.kt🤖 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/lyrics/feelin/navigation/FeelinNavHost.kt` around lines 91 - 122, Ensure FeelinDestination.Login defines the constants ROUTE_WITH_AUTO_LOGIN_FAILED_ERROR_CODE and AUTO_LOGIN_FAILED_ERROR_CODE_ARGUMENT and update FeelinDestination.kt if they are missing; then remove the duplicated parentEntry/viewModel retrieval in NavGraphBuilder.loginScreen by extracting the shared logic (the remember(backStackEntry) { navController.getBackStackEntry(FeelinDestination.OnboardingGraph.route) } and val viewModel: OnboardingViewModel = hiltViewModel(parentEntry)) into a small helper used by both composable blocks so both routes (FeelinDestination.Login.route and FeelinDestination.Login.ROUTE_WITH_AUTO_LOGIN_FAILED_ERROR_CODE) reuse the same parentEntry and OnboardingViewModel resolution.
🤖 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/lyrics/feelin/core/data/di/NetworkModule.kt`:
- Line 61: The Retrofit provider in NetworkModule.kt currently hardcodes the dev
HTTP URL in the .baseUrl(...) call; change it to read a build-time config (e.g.
BuildConfig.BASE_URL) instead and ensure the default value in your Gradle
productFlavors/buildTypes is an HTTPS production endpoint while the dev
flavor/buildType overrides to the dev URL; add a buildConfigField("String",
"BASE_URL", "\"https://prod.feelinapp.com/\"") in the default config and
override in the dev flavor (or buildTypes) to "http://dev.feelinapp.com/" if
needed, then replace the literal in the Retrofit builder with the BuildConfig
symbol so non-dev builds use the secure HTTPS endpoint.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/splash/SplashScreen.kt`:
- Line 25: The background uses a hardcoded colorResource in the SplashScreen
composable; replace the direct colorResource usage in the Modifier.background
call with the theme token from LocalFeelinColors (e.g.,
LocalFeelinColors.brandPrimary or the equivalent token in your theme). Locate
the Modifier chain where .background(color = colorResource(id =
R.color.brand_primary)) is applied in SplashScreen.kt and swap it to read the
color from LocalFeelinColors so the composable uses the centralized theme token.
- Line 30: The contentDescription in SplashScreen (the composable setting
contentDescription = "Feelin") is hardcoded; replace it with a localized
resource by using stringResource(R.string.feelin) and add a corresponding entry
(e.g., <string name="feelin">Feelin</string>) in your strings.xml so the UI
supports localization.
---
Outside diff comments:
In `@app/src/main/java/com/lyrics/feelin/presentation/view/login/LoginScreen.kt`:
- Around line 54-61: The LoginScreen Composable declares lambda params onSignUp
and onContinueToMain before non-lambda params (autoLoginFailedErrorCode,
loginViewModel); reorder the function signature in LoginScreen so that
non-lambda parameters (modifier, autoLoginFailedErrorCode, loginViewModel) come
first and the lambda parameters onSignUp and onContinueToMain are the final
parameters, updating any call sites as needed to match the new parameter order.
---
Nitpick comments:
In `@app/src/main/java/com/lyrics/feelin/core/data/repository/AuthRepository.kt`:
- Around line 82-101: The startup flow currently unconditionally clears tokens
when restoreSessionWithRefresh() sees any refresh failure from
authTokenRefresher.refreshServerToken(), which logs out users even on transient
network errors; change restoreSessionWithRefresh() to distinguish network errors
from auth errors by inspecting exception.toServerErrorCode() or exception type,
and only call authManager.clearTokens() and return RestoreSessionResult.Failed
for definitive auth failures (e.g., invalid/expired token codes), while for
network/transient errors return a new retryable result (or a distinct
RestoreSessionResult.RetryNetwork) so the caller can show a retry option or a
user-facing message instead of forcing logout. Ensure CancellationException is
still rethrown and keep existing RestoreSessionResult.Authenticated on success.
In `@app/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.kt`:
- Around line 91-122: Ensure FeelinDestination.Login defines the constants
ROUTE_WITH_AUTO_LOGIN_FAILED_ERROR_CODE and
AUTO_LOGIN_FAILED_ERROR_CODE_ARGUMENT and update FeelinDestination.kt if they
are missing; then remove the duplicated parentEntry/viewModel retrieval in
NavGraphBuilder.loginScreen by extracting the shared logic (the
remember(backStackEntry) {
navController.getBackStackEntry(FeelinDestination.OnboardingGraph.route) } and
val viewModel: OnboardingViewModel = hiltViewModel(parentEntry)) into a small
helper used by both composable blocks so both routes
(FeelinDestination.Login.route and
FeelinDestination.Login.ROUTE_WITH_AUTO_LOGIN_FAILED_ERROR_CODE) reuse the same
parentEntry and OnboardingViewModel resolution.
In `@app/src/main/java/com/lyrics/feelin/presentation/view/login/LoginScreen.kt`:
- Around line 68-78: The dialog in the LoginScreen Composable uses hardcoded
Korean text; move the title and description strings into string resources and
reference them from the Composable (e.g., replace the literal title and
description passed to FeelinModalDialog with values obtained via stringResource
or context.getString), formatting the description to include
autoLoginFailedErrorCode/UNKNOWN_AUTO_LOGIN_ERROR_CODE; update any tests or
previews to supply the resources if needed and keep the same behavior for
isAutoLoginFailedDialogVisible and onConfirmButtonClick.
🪄 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 Plus
Run ID: 7d7190af-5545-4233-abaa-28346c14af07
📒 Files selected for processing (17)
app/build.gradle.ktsapp/src/main/AndroidManifest.xmlapp/src/main/java/com/lyrics/feelin/MainActivity.ktapp/src/main/java/com/lyrics/feelin/core/data/datasource/remote/AuthRemoteDataSource.ktapp/src/main/java/com/lyrics/feelin/core/data/di/NetworkModule.ktapp/src/main/java/com/lyrics/feelin/core/data/interceptor/AuthInterceptor.ktapp/src/main/java/com/lyrics/feelin/core/data/manager/AuthTokenRefresher.ktapp/src/main/java/com/lyrics/feelin/core/data/repository/AuthRepository.ktapp/src/main/java/com/lyrics/feelin/navigation/FeelinDestination.ktapp/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.ktapp/src/main/java/com/lyrics/feelin/navigation/SplashNavigation.ktapp/src/main/java/com/lyrics/feelin/presentation/view/login/LoginScreen.ktapp/src/main/java/com/lyrics/feelin/presentation/view/splash/SplashScreen.ktapp/src/main/java/com/lyrics/feelin/presentation/view/splash/SplashViewModel.ktapp/src/main/res/values/colors.xmlapp/src/main/res/values/themes.xmlgradle/libs.versions.toml
There was a problem hiding this comment.
Pull request overview
Implements an app-start “auto login” entry flow by introducing a Splash start destination that restores/validates the saved auth session and routes to Main or Login, and centralizes token refresh into a mutex single-flight component used by the OkHttp Authenticator.
Changes:
- Add system + Compose splash entry point (
Theme.Feelin.Starting,installSplashScreen(),SplashScreen/ViewModel/Navigation) and set it asFeelinNavHoststart destination. - Implement session restoration in
AuthRepository.restoreSession()with a stricter “fail = clear tokens + go to login” policy on app start. - Introduce
AuthTokenRefresherand wire it intoTokenAuthenticator; pass auto-login failure error codes intoLoginScreenfor dialog display.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/libs.versions.toml | Adds version/catalog entry for core-splashscreen dependency. |
| app/build.gradle.kts | Adds androidx.core:core-splashscreen dependency to the app module. |
| app/src/main/AndroidManifest.xml | Switches activity theme to splash starting theme. |
| app/src/main/res/values/themes.xml | Defines Theme.Feelin.Starting based on Theme.SplashScreen. |
| app/src/main/res/values/colors.xml | Adds brand_primary color for splash background. |
| app/src/main/java/com/lyrics/feelin/MainActivity.kt | Installs AndroidX splashscreen in onCreate. |
| app/src/main/java/com/lyrics/feelin/navigation/FeelinDestination.kt | Adds Splash destination; enhances Login route to accept an error-code argument. |
| app/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.kt | Sets Splash as start destination; adds login route variant with error-code argument. |
| app/src/main/java/com/lyrics/feelin/navigation/SplashNavigation.kt | Adds splash destination + navigation logic based on restored session state. |
| app/src/main/java/com/lyrics/feelin/presentation/view/splash/SplashScreen.kt | Adds Compose splash UI. |
| app/src/main/java/com/lyrics/feelin/presentation/view/splash/SplashViewModel.kt | Adds session-restore state machine to drive splash routing. |
| app/src/main/java/com/lyrics/feelin/presentation/view/login/LoginScreen.kt | Shows an “auto login failed” dialog when routed with an error code. |
| app/src/main/java/com/lyrics/feelin/core/data/datasource/remote/AuthRemoteDataSource.kt | Adds validateToken() and reIssueToken() wrappers. |
| app/src/main/java/com/lyrics/feelin/core/data/manager/AuthTokenRefresher.kt | Introduces mutex single-flight token refresh + terminal error code handling. |
| app/src/main/java/com/lyrics/feelin/core/data/interceptor/AuthInterceptor.kt | Updates TokenAuthenticator to use AuthTokenRefresher and avoid refresh-loop on the reissue endpoint. |
| app/src/main/java/com/lyrics/feelin/core/data/repository/AuthRepository.kt | Adds restoreSession() and maps refresh/validation failures to error codes for routing. |
| app/src/main/java/com/lyrics/feelin/core/data/di/NetworkModule.kt | Changes Retrofit base URL (noted as an issue in comments). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Move lambda parameters to the end of the parameter list. 이 리뷰는 수정하지 않겠습니다. |
Clear local tokens when restoreSession encounters unexpected non-cancellation errors and return a failed restore result.
Keep the dismissed auto-login failure dialog state across configuration changes so the same route argument does not show it again after rotation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 208020a426
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@hyunjung-choi 진짜로 병합하겠습니다. 다른 티켓으로 이미 선언된 요소들을 제외하고는 다 수정했어요. |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
What is the current behavior?
앱 시작 시 저장된 인증 상태를 확인하고 적절한 화면으로 라우팅하는 스플래시 화면 진입점이 없었으며, 토큰 갱신 로직이 개별적으로 처리되어 동시성 관리가 부족했습니다.
What is the new behavior (if this is a feature change)?
androidx.core:core-splashscreen의존성을 추가하고MainActivity에 시스템 스플래시 핸드오프를 적용했습니다.SplashScreen,SplashViewModel,SplashNavigation)를 추가하여 앱의 초기 진입점(FeelinNavHost의 start destination)으로 설정했습니다.MainGraph로, 실패 시Login화면으로 자동 라우팅되도록 구현했습니다.AuthTokenRefresher를 추가하여 중앙 집중화된 안전한 토큰 갱신을 지원하며, 이를TokenAuthenticator에 적용했습니다.LoginScreen에서 관련 에러 코드를 다이얼로그로 노출하도록 개선했습니다.Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
앱의 초기 진입 흐름이 Splash 화면을 거치도록 변경되었습니다. 기존 로그인 화면으로 직행하던 로직에 의존하는 부분이 있다면 확인이 필요할 수 있으나, 일반적인 사용자 경험 측면에서의 파괴적인 변경은 없습니다.
ScreenShots (If needed)
Agent: 인증 상태에 따른 내부 라우팅 동작 및 스플래시 화면 연동, 토큰 처리 등 논리적인 흐름 제어가 주된 변경 사항이므로 별도의 UI 스크린샷은 첨부하지 않았습니다.
User: 화면 녹화로 작동하는 것을 녹화해 첨부하겠습니다.
작동 영상 링크, 10MB를 초과해 별도 링크를 첨부합니다.
Other information:
Agent: 다음 검증 작업을 모두 성공적으로 통과했습니다:
cmd.exe /c gradlew.bat detekt: successcmd.exe /c gradlew.bat :app:assembleDebug: successcmd.exe /c gradlew.bat testDebugUnitTest: successUser: SplashScreen API를 사용하려다 토큰 검증 실패에 대한 에러를 넘겨주는게 너무 복잡해져서 커스텀 컴포저블 스플래시 스크린을 같이 도입하는 방향으로 변경했습니다.
지금 해당 화면들의 배경색이 다크모드의 primary 색상에 대응하지 않는 문제, 컴포저블 스플래시에서 앱 아이콘이 작게 표시되는 문제가 있는데 이번 범위에서 다루지 않고 별도의 디자인 수정 PR을 작성해 대응하겠습니다.
Summary by CodeRabbit
New Features
Bug Fixes