SCRUM-273 feat: Connect settings logout flow#49
Conversation
- add oh-my-openagent generated files
Share MyPageViewModel across the MyPage graph, show logout confirmation and loading states, and clear local session data before navigating to onboarding.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughImplements user logout: AuthRepository no longer fails the flow, MyPageViewModel manages logoutStatus, SettingScreen adds confirmation and blocking loading overlay, and navigation returns to onboarding on successful logout. ChangesLogout Flow Implementation
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingScreen
participant Dialog
participant MyPageViewModel
participant AuthRepository
participant Navigation
User->>SettingScreen: tap logout row
SettingScreen->>Dialog: show confirmation
User->>Dialog: confirm
Dialog->>MyPageViewModel: logout()
MyPageViewModel->>AuthRepository: logout()
AuthRepository-->>MyPageViewModel: return (errors logged, not propagated)
MyPageViewModel->>Navigation: emit logoutStatus=SUCCESS -> navigate to onboarding
Navigation->>MyPageViewModel: clearLogoutStatus()
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)
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 |
|
아 그리고 이번 PR은 본문작성, 등록을 GPT5.5의 영혼을 담은 @sisyphus-dev-ai 가 해줬어요 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86b2fc6800
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt (1)
142-158: ⚡ Quick winAdd a
modifierparameter toLogoutLoadingOverlay.This composable renders UI but hardcodes its root modifier, which makes it harder to reuse or place from callers. Thread
modifierinto the rootBoxand keep it as the first optional parameter.♻️ Suggested change
`@Composable` -private fun LogoutLoadingOverlay() { +private fun LogoutLoadingOverlay( + modifier: Modifier = Modifier, +) { val feelinColors = LocalFeelinColors.current Box( - modifier = Modifier + modifier = modifier .fillMaxSize() .background(feelinColors.dim) .clickable( interactionSource = remember { MutableInteractionSource() }, indication = null,As per coding guidelines, "Include
modifier: Modifier = Modifieron all composables that render UI" and "Keepmodifieras the first optional 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/mypage/setting/SettingScreen.kt` around lines 142 - 158, Add a Modifier parameter to LogoutLoadingOverlay and thread it into the root Box as the first optional parameter: change the signature to include modifier: Modifier = Modifier (keeping LogoutLoadingOverlay composable and LocalFeelinColors usage), then replace the hardcoded Modifier on the Box with the passed-in modifier chained with the existing calls (e.g. modifier.fillMaxSize().background(feelinColors.dim).clickable(...)) so callers can override or extend layout while preserving the current fill/background/clickable behavior.
🤖 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/navigation/FeelinNavHost.kt`:
- Around line 312-319: The logout-status observer (LaunchedEffect watching
logoutStatus and performing viewModel.clearLogoutStatus() +
navController.navigate(FeelinDestination.OnboardingGraph.route) on
MyPageLogoutStatus.SUCCESS) must be moved out of the composable that is torn
down (FeelinDestination.Setting) into a stable parent that remains composed for
the entire My Page/logout flow (e.g., the My Page graph host or FeelinNavHost).
Relocate the LaunchedEffect and its logic to a parent composable that holds or
can access MyPageViewModel and navController so the SUCCESS transition always
runs, and keep the same calls to viewModel.clearLogoutStatus() and
navController.navigate(FeelinDestination.OnboardingGraph.route) with the same
popUpTo behavior.
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/mypage/MyPageViewModel.kt`:
- Around line 67-72: The coroutine in MyPageViewModel (viewModelScope.launch)
currently calls authRepository.logout() and userRepository.logout() without
handling failures, leaving _logoutStatus at LOADING if either throws; wrap those
calls in a try/catch (or use runCatching/Result) around authRepository.logout()
and userRepository.logout(), set _logoutStatus to MyPageLogoutStatus.ERROR (or
IDLE) on exception and optionally log the error, and only set _logoutStatus =
MyPageLogoutStatus.SUCCESS and update _myPageScreenStatus/_logoutSample after
the logout calls succeed; ensure any cleanup/rollback runs in finally if needed
so the UI never remains stuck in LOADING.
---
Nitpick comments:
In
`@app/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt`:
- Around line 142-158: Add a Modifier parameter to LogoutLoadingOverlay and
thread it into the root Box as the first optional parameter: change the
signature to include modifier: Modifier = Modifier (keeping LogoutLoadingOverlay
composable and LocalFeelinColors usage), then replace the hardcoded Modifier on
the Box with the passed-in modifier chained with the existing calls (e.g.
modifier.fillMaxSize().background(feelinColors.dim).clickable(...)) so callers
can override or extend layout while preserving the current
fill/background/clickable behavior.
🪄 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: 9a8b82dc-2f67-4d46-972c-eea1adcf3616
📒 Files selected for processing (9)
.gitignoreapp/src/main/java/com/lyrics/feelin/core/data/datasource/local/UserPreferencesDataStore.ktapp/src/main/java/com/lyrics/feelin/core/data/repository/AuthRepository.ktapp/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.ktapp/src/main/java/com/lyrics/feelin/presentation/designsystem/theme/Color.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/MyPageScreenState.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/MyPageViewModel.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/component/SettingInfoItem.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt
There was a problem hiding this comment.
Pull request overview
Connects the Settings “로그아웃” row to the actual logout flow by confirming intent, running logout via the MyPage graph-scoped ViewModel, clearing local auth/session data, and navigating back to onboarding.
Changes:
- Added a logout confirmation dialog + loading overlay to
SettingScreen. - Introduced
logoutStatusstate inMyPageViewModeland wired logout/navigation handling inFeelinNavHost. - Updated
AuthRepository.logout()behavior and adjusted dim color/token definitions used by the overlay.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt | Adds logout confirmation dialog, triggers logout callback, and shows a loading overlay while logging out. |
| app/src/main/java/com/lyrics/feelin/presentation/view/mypage/MyPageViewModel.kt | Adds logout state flow and logout action invoked from Settings/MyPage graph. |
| app/src/main/java/com/lyrics/feelin/presentation/view/mypage/MyPageScreenState.kt | Introduces MyPageLogoutStatus enum for logout state tracking. |
| app/src/main/java/com/lyrics/feelin/presentation/view/mypage/component/SettingInfoItem.kt | Makes info rows optionally clickable (used for the logout item). |
| app/src/main/java/com/lyrics/feelin/presentation/designsystem/theme/Color.kt | Updates the common dim/scrim color used by overlays. |
| app/src/main/java/com/lyrics/feelin/navigation/FeelinNavHost.kt | Scopes MyPage ViewModel to the graph and navigates to onboarding on logout success. |
| app/src/main/java/com/lyrics/feelin/core/data/repository/AuthRepository.kt | Adjusts logout flow to clear tokens regardless of backend/SDK logout outcome. |
| app/src/main/java/com/lyrics/feelin/core/data/datasource/local/UserPreferencesDataStore.kt | Adds a TODO note about evolving the stored user model. |
| .gitignore | Ignores .sisyphus files. |
Comments suppressed due to low confidence (1)
app/src/main/java/com/lyrics/feelin/core/data/repository/AuthRepository.kt:240
logout()always returnsResult.success(Unit)even if earlier steps failed. Returning an unconditional success while keeping aResult<Unit>return type is misleading for API consumers; either propagate failures (while still clearing tokens) or change the API to returnUnit/a dedicated status so callers can't mistakenly assume remote/SDK logout succeeded.
authManager.clearTokens()
return Result.success(Unit)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
이것 뭐에요 . . 짱신기하네요 , , ? |
gdaegeun539
changed the title
에이전트가 Github CLI도 다룰 수 있으니까 작업 마지막 과정에서 PR 등록까지 실행해달라고 하면 CLI 통해서 등록할 수 있더라고요. |
|
이슈는 내일(2026.05.18.) 이내로 수정하겠습니다 |
| * 1. Backend에 로그아웃 요청 | ||
| * 2. SDK 로그아웃 (Kakao/Google) | ||
| * 3. AuthManager에서 토큰 삭제 | ||
| * 3. 성공/실패 여부와 무관하게 AuthManager에서 토큰 삭제 |
There was a problem hiding this comment.
iOS 앱에서는 로그아웃 엔드포인트를 호출하지 않고 로컬 토큰만 삭제하더라고요. 안드로이드가 해당 방식을 동일하게 따라가지는 않더라도 서버 실패를 무조건 로그아웃 실패로 취급할 필요는 없어 보여서 로그아웃 메소드의 세부 구현을 변경했습니다.
| private fun NavGraphBuilder.myPageNavGraph(navController: NavHostController) { | ||
| navigation(startDestination = FeelinDestination.MyPage.route, route = FeelinDestination.MyPageGraph.route) { | ||
| composable(FeelinDestination.MyPage.route) { | ||
| composable(FeelinDestination.MyPage.route) { backStackEntry -> |
There was a problem hiding this comment.
마이페이지, 설정 화면, 프로필 변경 화면 등이 동일한 사용자 정보를 공유하는 것을 고려해 마이페이지 뷰모델을 마이페이지 그래프의 공용 뷰모델로 사용하는 방향으로 정했습니다.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
This reverts commit 70b3083.
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/main/java/com/lyrics/feelin/core/data/repository/AuthRepository.kt`:
- Around line 221-248: The logout() flow can be interrupted by a
CancellationException from authRemoteDataSource.signOut() because safeApiCall
only catches IOExceptions; to ensure tokens are always cleared, wrap the
signOut() + provider-specific SDK logout calls in a try/finally and call
authManager.clearTokens() in the finally block (leave provider
branches—kakaoAuthDataSource.logout(), Google branch, null—inside the try), or
alternatively catch CancellationException explicitly to rethrow after clearing
tokens; reference logout(), authRemoteDataSource.signOut(),
kakaoAuthDataSource.logout(), authManager.clearTokens(), and safeApiCall
(ResponseExtension.kt) when making the change.
🪄 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: 3bc9ddc4-fdae-4d5f-b152-7aaa3a9c321e
📒 Files selected for processing (2)
app/src/main/java/com/lyrics/feelin/core/data/repository/AuthRepository.ktapp/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/lyrics/feelin/presentation/view/mypage/setting/SettingScreen.kt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8cdd34a0e9
ℹ️ 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".
| if (isLogoutLoading) { | ||
| // FIXME(@이대근): 화면 전역으로 오버레이가 적용되지 않아 하단바를 선택할 수 있음 2026.05.17. | ||
| LogoutLoadingOverlay() |
There was a problem hiding this comment.
로그아웃 로딩 오버레이가 SettingScreen 내부에만 그려져 MainScaffold의 bottomBar는 계속 터치 가능합니다. 로그아웃 요청 중 사용자가 홈/검색 탭을 누르면 설정 composable이 dispose되어 logoutStatus == SUCCESS를 관찰하는 LaunchedEffect도 사라지므로, 토큰과 사용자 데이터는 지워졌는데 OnboardingGraph로 이동하지 못하고 메인 화면에 남을 수 있습니다. 로딩 상태는 하단 내비게이션까지 포함한 상위 MainScaffold 레벨에서 차단하거나, 성공 내비게이션 관찰을 탭 전환으로 사라지지 않는 위치로 올려야 합니다.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
#49 (comment) 참조.
구현 범위가 넓어지는 문제가 있어서 별도의 작업으로 등록했어.
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
@codex re-review except #49 (comment) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e844836b1
ℹ️ 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".
| LaunchedEffect(logoutStatus) { | ||
| if (logoutStatus == MyPageLogoutStatus.SUCCESS) { | ||
| viewModel.clearLogoutStatus() | ||
| navController.navigate(FeelinDestination.OnboardingGraph.route) { | ||
| popUpTo(FeelinDestination.MainGraph.route) { inclusive = true } |
There was a problem hiding this comment.
로그아웃 완료 내비게이션을 Setting destination 내부에서만 관찰하면, 로그아웃 요청 후 네트워크가 느린 동안 사용자가 시스템 Back으로 설정 화면을 벗어난 경우 이 LaunchedEffect가 dispose되어 SUCCESS 상태를 처리할 곳이 없어집니다. 이때 토큰과 사용자 DataStore는 이미 지워졌지만 사용자는 메인 그래프에 남아 하단 탭을 계속 사용할 수 있으므로, 로그아웃 상태 관찰/내비게이션을 MyPageGraph처럼 설정 화면 생명주기에 묶이지 않는 위치로 올리거나 로딩 중 Back을 막아야 합니다.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
SCRUM-276 의 전역 로딩 인디케이터에서 뒤로가기 버튼/제스처를 막도록 같이 구현할 예정.
|
SCRUM-276의 범위에 속하는 부분(전역 로딩 오버레이)을 제외하고 리뷰 수정을 완료했습니다. 우선 이 PR을 머지하고, 로그아웃 중 발생 가능한 문제를 방지하기 위해 해당 티켓을 우선 구현하겠습니다. |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce?
What is the current behavior?
Settings shows a logout row, but it is not connected to the actual logout flow.
What is the new behavior (if this is a feature change)?
Settings logout now shows a confirmation dialog, runs the logout flow through the MyPage graph-scoped ViewModel, clears local auth and user session data, and navigates back to the onboarding/login flow.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No.
ScreenShots (If needed)
Other information:
Agent(GPT 5.5 w/ OmO Sishpyus)
cmd.exe /c gradlew.bat detekt,cmd.exe /c gradlew.bat :app:assembleDebug, andcmd.exe /c gradlew.bat testDebugUnitTest.User
로그아웃 관련 다이얼로그의 스크린샷, 로그아웃 작동의 화면 녹화를 추가하겠습니다.
추가 완료시에는 이 답글을 삭제하겠습니다.
Summary by CodeRabbit
New Features
Style
Chores