로딩 에러 UI 적용 및 상태 인터페이스 정리#160
Merged
Merged
Conversation
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/result/src/main/java/com/twix/result/extension.kt (1)
1-9:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winKtLint 위반으로 CI가 실패하고 있어 머지 블로커입니다.
현재 파일명(
extension.kt)과 마지막 불필요 공백 줄 때문에 빌드가 깨집니다. 파일명을 PascalCase(예:AppResultExtensions.kt)로 바꾸고 Line 9 빈 줄을 제거해 주세요.🤖 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 `@core/result/src/main/java/com/twix/result/extension.kt` around lines 1 - 9, Rename the file from extension.kt to a PascalCase name (e.g., AppResultExtensions.kt) and remove the trailing blank line at the end of the file; update any imports/usages if needed, and ensure the Kotlin extension function fun <T> AppResult<T>.errorOrNull() (and references to AppResult.Success / AppResult.Error) remains unchanged so the build passes KtLint and CI.feature/onboarding/src/main/java/com/twix/onboarding/invite/InviteCodeScreen.kt (1)
84-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick win초기 초대코드가 로딩/에러 상태 전환 시 다시 주입되어 사용자 입력을 덮어쓸 수 있습니다.
Line 84의
LaunchedEffect는showLoading,showError상태 변화에 의존하고 있습니다. 이들 값이true에서false로 전환될 때마다 Effect가 재실행되어,initialInviteCode가 여전히 값을 가지고 있다면 Line 88의WriteInviteCode가 재디스패치됩니다. 초기값 주입은 1회만 수행되도록 제한하는 것이 안전합니다.현재 흐름:
- 사용자가 TextField에 코드 입력 →
WriteInviteCode(userInput)디스패치- 로딩/에러 상태 변화 발생
- LaunchedEffect 재실행 →
WriteInviteCode(initialInviteCode)재디스패치- 사용자 입력이 초기값으로 덮어씌워짐
수정 예시
+ var hasAppliedInitialInviteCode by rememberSaveable(initialInviteCode) { mutableStateOf(false) } + LaunchedEffect(initialInviteCode, uiState.showLoading, uiState.showError) { if (uiState.showLoading) return@LaunchedEffect if (uiState.showError) return@LaunchedEffect - if (!initialInviteCode.isNullOrBlank()) { + if (!hasAppliedInitialInviteCode && !initialInviteCode.isNullOrBlank()) { viewModel.dispatch(OnBoardingIntent.WriteInviteCode(initialInviteCode)) + hasAppliedInitialInviteCode = true } }🤖 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 `@feature/onboarding/src/main/java/com/twix/onboarding/invite/InviteCodeScreen.kt` around lines 84 - 90, The LaunchedEffect currently depends on showLoading and showError so it can re-run and overwrite user input by re-dispatching OnBoardingIntent.WriteInviteCode(initialInviteCode); change the injection to run only once by removing showLoading/showError from the LaunchedEffect keys and adding a one-time guard: use LaunchedEffect(initialInviteCode) (or keep current keys but add a remember { mutableStateOf(false) } flag like initialInjectDone) and only call viewModel.dispatch(OnBoardingIntent.WriteInviteCode(initialInviteCode)) if initialInviteCode is not null/blank and initialInjectDone is false, then set initialInjectDone = true after dispatch to prevent future overwrites.feature/notification/src/main/java/com/twix/notification/NotificationScreen.kt (1)
92-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
LaunchedEffect내부에서 최신 상태를 반영하지 못해 페이지네이션 요청이 중복될 수 있습니다.Line 92의
LaunchedEffect의존성 배열에uiState.notificationList.size만 포함되어 있지만, Line 103에서uiState.canLoadNextPage를 직접 참조하고 있습니다.canLoadNextPage는hasNext && !isLoading로 계산되기 때문에,isLoading이나hasNext만 변경되고notificationList.size는 변경되지 않을 때,LaunchedEffect가 재실행되지 않아 closure 내부의uiState참조가 오래된 상태로 남아있게 됩니다.이로 인해 스크롤 이벤트 시점에
canLoadNextPage가 이미false가 되었음에도 불구하고, 오래된uiState참조로 인해onNextPage()가 호출되어 중복 요청이 발생할 수 있습니다.Compose의
rememberUpdatedStateAPI를 사용하여 항상 최신 상태를 참조하도록 수정해 주세요:수정 예시
+ val currentCanLoadNextPage by rememberUpdatedState(uiState.canLoadNextPage) + LaunchedEffect(listState, uiState.notificationList.size) { snapshotFlow { val layoutInfo = listState.layoutInfo val totalCount = layoutInfo.totalItemsCount val lastVisibleIndex = layoutInfo.visibleItemsInfo.lastOrNull()?.index ?: -1 totalCount > 0 && lastVisibleIndex >= totalCount - 3 }.distinctUntilChanged() .filter { it } .collect { - if (uiState.canLoadNextPage) { + if (currentCanLoadNextPage) { onNextPage() } } }🤖 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 `@feature/notification/src/main/java/com/twix/notification/NotificationScreen.kt` around lines 92 - 106, The LaunchedEffect currently captures stale uiState.canLoadNextPage and can trigger duplicate requests; fix it by using rememberUpdatedState to hold the latest values and reference those inside the snapshotFlow/collect: create val currentCanLoadNextPage = rememberUpdatedState(uiState.canLoadNextPage) (and also wrap onNextPage with val currentOnNextPage = rememberUpdatedState(onNextPage) if needed), keep LaunchedEffect dependencies as before (e.g., listState and uiState.notificationList.size), and inside the collect check currentCanLoadNextPage.value and call currentOnNextPage.value() so the coroutine always sees the latest canLoadNextPage and onNextPage.
🧹 Nitpick comments (5)
feature/notification/src/main/java/com/twix/notification/NotificationViewModel.kt (1)
78-85: 💤 Low value주석과 실제 구현 간의 불일치를 확인해 주세요.
Line 108의 주석에서는 "알림 읽음 처리는 best effort가 정책이므로 에러 처리는 생략"이라고 명시되어 있습니다. 그러나 실제 구현(Line 82, 122)에서는
handleError(result.error)를 호출하고 있어 주석과 코드가 일치하지 않습니다.
handleError가 단순 로깅만 수행한다면 괜찮을 수 있지만, 주석을 업데이트하거나 의도에 맞게 코드를 수정하는 것이 좋겠습니다.- // 알림 읽음 처리는 best effort가 정책이므로 에러 처리는 생략 + // 알림 읽음 처리는 best effort 정책이므로 에러 발생 시 로깅만 수행Also applies to: 108-125
🤖 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 `@feature/notification/src/main/java/com/twix/notification/NotificationViewModel.kt` around lines 78 - 85, The comment says "read marking is best-effort, skip error handling" but markAllNotificationAsRead (and the similar method around lines 108-125) calls handleError(result.error); either update the comment to state that errors are handled via handleError, or change the code to match the best-effort intent by removing handleError calls in markAllNotificationAsRead (and the related method) and instead perform a no-op or a light debug/log-only action (e.g., swallow the error or log at debug level) so behavior and comment are consistent; reference functions: markAllNotificationAsRead, handleError, and notificationRepository.markAllNotificationsAsRead().feature/onboarding/src/main/java/com/twix/onboarding/contract/OnBoardingUiState.kt (1)
23-33: ⚡ Quick win액션별 로딩 플래그는
isLoading까지 함께 조건에 넣는 편이 안전합니다.현재는
loadingAction만으로isConnectingCouple/isSubmitting*가 결정됩니다. 이 경우isLoading만 false로 내려가고loadingAction정리가 누락되면 UI 가드가 남을 수 있습니다.
isLoading && loadingAction == ...형태로 계산하면 상태 불일치에 더 강해집니다.🔧 제안 diff
- val isConnectingCouple: Boolean - get() = loadingAction == OnBoardingLoadingAction.CONNECT_COUPLE + val isConnectingCouple: Boolean + get() = isLoading && loadingAction == OnBoardingLoadingAction.CONNECT_COUPLE - val isSubmittingProfile: Boolean - get() = loadingAction == OnBoardingLoadingAction.SUBMIT_PROFILE + val isSubmittingProfile: Boolean + get() = isLoading && loadingAction == OnBoardingLoadingAction.SUBMIT_PROFILE - val isSubmittingDday: Boolean - get() = loadingAction == OnBoardingLoadingAction.SUBMIT_DDAY + val isSubmittingDday: Boolean + get() = isLoading && loadingAction == OnBoardingLoadingAction.SUBMIT_DDAY - val isSubmittingMarketingConsent: Boolean - get() = loadingAction == OnBoardingLoadingAction.SUBMIT_MARKETING_CONSENT + val isSubmittingMarketingConsent: Boolean + get() = isLoading && loadingAction == OnBoardingLoadingAction.SUBMIT_MARKETING_CONSENT🤖 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 `@feature/onboarding/src/main/java/com/twix/onboarding/contract/OnBoardingUiState.kt` around lines 23 - 33, The computed loading flags (isConnectingCouple, isSubmittingProfile, isSubmittingDday, isSubmittingMarketingConsent) should also check the global isLoading flag to avoid stale UI when loadingAction isn't cleared; update each getter in OnBoardingUiState so it returns isLoading && loadingAction == OnBoardingLoadingAction.<ACTION> (e.g., isLoading && loadingAction == OnBoardingLoadingAction.CONNECT_COUPLE) instead of just comparing loadingAction, and run/adjust any related unit/UI checks that assume the previous behavior.feature/stats/detail/src/main/java/com/twix/stats/detail/StatsDetailViewModel.kt (1)
71-73: 💤 Low value타입 캐스팅 패턴 검토
copyState(isLoading = true, error = null) as StatsDetailUiState캐스팅이 있습니다.copyState가DefaultLoadableState또는ContentLoadableState인터페이스를 반환하기 때문에 발생하는 것으로 보입니다.현재 동작에는 문제가 없지만,
StatsDetailUiState에 직접copy(isLoading = true, error = null)를 사용하면 타입 안전성이 높아집니다:reduce { copy(isLoading = true, error = null) }베이스 클래스의
reduceLoadableState헬퍼가 이 패턴을 위해 설계된 것이라면 현재 방식도 일관성 측면에서 괜찮습니다.🤖 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 `@feature/stats/detail/src/main/java/com/twix/stats/detail/StatsDetailViewModel.kt` around lines 71 - 73, The reduction is using an unsafe cast from copyState(...) to StatsDetailUiState; replace the cast by calling StatsDetailUiState's own copy method inside the reduce block (i.e., use copy(isLoading = true, error = null)) so the result is type-safe and avoids relying on copyState's return type; if reduceLoadableState was intentionally used for consistency, you can instead adjust that helper to return the concrete StatsDetailUiState type, but the simplest fix is to change the reduce lambda to call copy(...) on StatsDetailUiState rather than casting the copyState(...) result.feature/main/src/main/java/com/twix/stats/StatsViewModel.kt (1)
72-76: 💤 Low value초기 로딩 중복 실행 방지 검토
fetchInitialStats()가init과Retry인텐트 양쪽에서 호출됩니다. 빠른 연속 재시도 시fetchInProgressStats와fetchCompletedStats가 중복 실행될 수 있습니다.
latestInProgressRequestId가 진행 중 통계의 race condition을 방지하지만, 완료 통계(fetchCompletedStats)에는 유사한 보호가 없습니다. 현재 구조에서 큰 문제는 아니지만, 일관성을 위해 초기 로딩 전체에 대한 중복 실행 가드를 고려해볼 수 있습니다.private var isInitialFetchInProgress = false private fun fetchInitialStats() { if (isInitialFetchInProgress) return isInitialFetchInProgress = true // ... fetch logic ... // reset flag in onSuccess/onError }🤖 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 `@feature/main/src/main/java/com/twix/stats/StatsViewModel.kt` around lines 72 - 76, fetchInitialStats() can be invoked from both init and a Retry intent, causing fetchInProgressStats and fetchCompletedStats to run twice; add a guard boolean (e.g., private var isInitialFetchInProgress = false) at the top of fetchInitialStats() to return immediately if already true, set it true before calling fetchInProgressStats(currentMonth) and fetchCompletedStats(), and ensure you reset isInitialFetchInProgress = false in all terminal paths (onSuccess, onError, or finally callbacks) for the existing fetchInProgressStats/fetchCompletedStats flows; keep using latestInProgressRequestId for in-progress protection and apply this new flag to cover the entire initial-load sequence.feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageViewModel.kt (1)
144-152: 💤 Low value
refreshGoalSummaryList의 에러 처리 정책 확인 필요현재
refreshGoalSummaryList에서showToastOnError = false로 설정하여 버스 이벤트로 인한 갱신 실패 시 사용자에게 아무런 피드백도 주지 않습니다. 이것이 의도된 동작인지 확인이 필요합니다.만약 백그라운드 갱신 실패를 조용히 처리하는 것이 의도라면 현재 코드가 적절합니다. 그러나 사용자가 목표 완료/삭제 후 목록이 갱신되지 않았을 때 혼란스러울 수 있으므로, 토스트로 실패를 알려주는 것이 UX 측면에서 더 나을 수 있습니다.
🤖 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 `@feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageViewModel.kt` around lines 144 - 152, The refreshGoalSummaryList currently suppresses user feedback on fetch errors by calling fetchGoalSummaryList(date = currentState.selectedDate, showInitialLoading = false, showToastOnError = false); decide the intended policy and update the call in refreshGoalSummaryList accordingly—either keep showToastOnError = false if silent background retries are required, or set showToastOnError = true so users see a toast when the fetchGoalSummaryList call (used to refresh summaries after bus events) fails; alternatively, add a parameter to refreshGoalSummaryList to control showToastOnError and pass it through to fetchGoalSummaryList for explicit behavior control.
🤖 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 `@feature/onboarding/src/main/java/com/twix/onboarding/OnBoardingViewModel.kt`:
- Around line 144-150: The current pattern sets loadingAction = null
unconditionally in onFinally which can clear another concurrently started
action; update the connectCouple flow (and the similar blocks using
launchResult/reduce) to use a compare-and-clear: when starting, set
loadingAction to OnBoardingLoadingAction.CONNECT_COUPLE only if
currentState.loadingAction == null (or block any start when loadingAction !=
null), and in onFinally only clear loadingAction if currentState.loadingAction
== OnBoardingLoadingAction.CONNECT_COUPLE; reference symbols: loadingAction,
OnBoardingLoadingAction.CONNECT_COUPLE, connectCoupleJob, launchResult, reduce
(apply same change to the other launchResult blocks mentioned).
In
`@feature/photolog/detail/src/main/java/com/twix/photolog/detail/contract/PhotologDetailUiState.kt`:
- Around line 30-31: The default PhotologDetailUiState has isLoading = true
which causes the UI to stay loading when you return a fallback
PhotologDetailUiState(); update the fallback return(s) where goalId is missing
(the branches that currently return PhotologDetailUiState()) to explicitly
return PhotologDetailUiState(isLoading = false) so the loading state is cleared;
locate the PhotologDetailUiState constructor usages in the code paths around the
goal lookup (the fallback returns near the goalId handling) and set isLoading =
false on those instances.
---
Outside diff comments:
In `@core/result/src/main/java/com/twix/result/extension.kt`:
- Around line 1-9: Rename the file from extension.kt to a PascalCase name (e.g.,
AppResultExtensions.kt) and remove the trailing blank line at the end of the
file; update any imports/usages if needed, and ensure the Kotlin extension
function fun <T> AppResult<T>.errorOrNull() (and references to AppResult.Success
/ AppResult.Error) remains unchanged so the build passes KtLint and CI.
In
`@feature/notification/src/main/java/com/twix/notification/NotificationScreen.kt`:
- Around line 92-106: The LaunchedEffect currently captures stale
uiState.canLoadNextPage and can trigger duplicate requests; fix it by using
rememberUpdatedState to hold the latest values and reference those inside the
snapshotFlow/collect: create val currentCanLoadNextPage =
rememberUpdatedState(uiState.canLoadNextPage) (and also wrap onNextPage with val
currentOnNextPage = rememberUpdatedState(onNextPage) if needed), keep
LaunchedEffect dependencies as before (e.g., listState and
uiState.notificationList.size), and inside the collect check
currentCanLoadNextPage.value and call currentOnNextPage.value() so the coroutine
always sees the latest canLoadNextPage and onNextPage.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/invite/InviteCodeScreen.kt`:
- Around line 84-90: The LaunchedEffect currently depends on showLoading and
showError so it can re-run and overwrite user input by re-dispatching
OnBoardingIntent.WriteInviteCode(initialInviteCode); change the injection to run
only once by removing showLoading/showError from the LaunchedEffect keys and
adding a one-time guard: use LaunchedEffect(initialInviteCode) (or keep current
keys but add a remember { mutableStateOf(false) } flag like initialInjectDone)
and only call
viewModel.dispatch(OnBoardingIntent.WriteInviteCode(initialInviteCode)) if
initialInviteCode is not null/blank and initialInjectDone is false, then set
initialInjectDone = true after dispatch to prevent future overwrites.
---
Nitpick comments:
In
`@feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageViewModel.kt`:
- Around line 144-152: The refreshGoalSummaryList currently suppresses user
feedback on fetch errors by calling fetchGoalSummaryList(date =
currentState.selectedDate, showInitialLoading = false, showToastOnError =
false); decide the intended policy and update the call in refreshGoalSummaryList
accordingly—either keep showToastOnError = false if silent background retries
are required, or set showToastOnError = true so users see a toast when the
fetchGoalSummaryList call (used to refresh summaries after bus events) fails;
alternatively, add a parameter to refreshGoalSummaryList to control
showToastOnError and pass it through to fetchGoalSummaryList for explicit
behavior control.
In `@feature/main/src/main/java/com/twix/stats/StatsViewModel.kt`:
- Around line 72-76: fetchInitialStats() can be invoked from both init and a
Retry intent, causing fetchInProgressStats and fetchCompletedStats to run twice;
add a guard boolean (e.g., private var isInitialFetchInProgress = false) at the
top of fetchInitialStats() to return immediately if already true, set it true
before calling fetchInProgressStats(currentMonth) and fetchCompletedStats(), and
ensure you reset isInitialFetchInProgress = false in all terminal paths
(onSuccess, onError, or finally callbacks) for the existing
fetchInProgressStats/fetchCompletedStats flows; keep using
latestInProgressRequestId for in-progress protection and apply this new flag to
cover the entire initial-load sequence.
In
`@feature/notification/src/main/java/com/twix/notification/NotificationViewModel.kt`:
- Around line 78-85: The comment says "read marking is best-effort, skip error
handling" but markAllNotificationAsRead (and the similar method around lines
108-125) calls handleError(result.error); either update the comment to state
that errors are handled via handleError, or change the code to match the
best-effort intent by removing handleError calls in markAllNotificationAsRead
(and the related method) and instead perform a no-op or a light debug/log-only
action (e.g., swallow the error or log at debug level) so behavior and comment
are consistent; reference functions: markAllNotificationAsRead, handleError, and
notificationRepository.markAllNotificationsAsRead().
In
`@feature/onboarding/src/main/java/com/twix/onboarding/contract/OnBoardingUiState.kt`:
- Around line 23-33: The computed loading flags (isConnectingCouple,
isSubmittingProfile, isSubmittingDday, isSubmittingMarketingConsent) should also
check the global isLoading flag to avoid stale UI when loadingAction isn't
cleared; update each getter in OnBoardingUiState so it returns isLoading &&
loadingAction == OnBoardingLoadingAction.<ACTION> (e.g., isLoading &&
loadingAction == OnBoardingLoadingAction.CONNECT_COUPLE) instead of just
comparing loadingAction, and run/adjust any related unit/UI checks that assume
the previous behavior.
In
`@feature/stats/detail/src/main/java/com/twix/stats/detail/StatsDetailViewModel.kt`:
- Around line 71-73: The reduction is using an unsafe cast from copyState(...)
to StatsDetailUiState; replace the cast by calling StatsDetailUiState's own copy
method inside the reduce block (i.e., use copy(isLoading = true, error = null))
so the result is type-safe and avoids relying on copyState's return type; if
reduceLoadableState was intentionally used for consistency, you can instead
adjust that helper to return the concrete StatsDetailUiState type, but the
simplest fix is to change the reduce lambda to call copy(...) on
StatsDetailUiState rather than casting the copyState(...) result.
🪄 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.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: b2f1ebb9-7bff-4e94-a809-6d222bb66eda
📒 Files selected for processing (55)
.gitignorecore/result/src/main/java/com/twix/result/extension.ktcore/ui/src/main/java/com/twix/ui/base/BaseViewModel.ktcore/ui/src/main/java/com/twix/ui/base/ContentLoadableState.ktcore/ui/src/main/java/com/twix/ui/base/DefaultLoadableState.ktcore/ui/src/main/java/com/twix/ui/base/LoadableState.ktfeature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorScreen.ktfeature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorViewModel.ktfeature/goal-editor/src/main/java/com/twix/goal_editor/model/GoalEditorUiState.ktfeature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageIntent.ktfeature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageScreen.ktfeature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageViewModel.ktfeature/goal-manage/src/main/java/com/twix/goal_manage/model/GoalManageUiState.ktfeature/login/src/main/java/com/twix/login/LoginScreen.ktfeature/login/src/main/java/com/twix/login/LoginViewModel.ktfeature/login/src/main/java/com/twix/login/contract/LoginUiState.ktfeature/main/src/main/java/com/twix/home/HomeScreen.ktfeature/main/src/main/java/com/twix/home/HomeViewModel.ktfeature/main/src/main/java/com/twix/home/model/HomeUiState.ktfeature/main/src/main/java/com/twix/stats/StatsScreen.ktfeature/main/src/main/java/com/twix/stats/StatsViewModel.ktfeature/main/src/main/java/com/twix/stats/contract/StatsIntent.ktfeature/main/src/main/java/com/twix/stats/contract/StatsUiState.ktfeature/main/src/main/java/com/twix/stats/preview/StatsUiStatePreviewProvider.ktfeature/notification/src/main/java/com/twix/notification/NotificationScreen.ktfeature/notification/src/main/java/com/twix/notification/NotificationViewModel.ktfeature/notification/src/main/java/com/twix/notification/contract/NotificationIntent.ktfeature/notification/src/main/java/com/twix/notification/contract/NotificationUiState.ktfeature/onboarding/src/main/java/com/twix/onboarding/OnBoardingViewModel.ktfeature/onboarding/src/main/java/com/twix/onboarding/contract/OnBoardingIntent.ktfeature/onboarding/src/main/java/com/twix/onboarding/contract/OnBoardingLoadingAction.ktfeature/onboarding/src/main/java/com/twix/onboarding/contract/OnBoardingUiState.ktfeature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectScreen.ktfeature/onboarding/src/main/java/com/twix/onboarding/dday/DdayRoute.ktfeature/onboarding/src/main/java/com/twix/onboarding/invite/InviteCodeScreen.ktfeature/onboarding/src/main/java/com/twix/onboarding/profile/ProfileScreen.ktfeature/photolog/detail/src/main/java/com/twix/photolog/detail/PhotologDetailScreen.ktfeature/photolog/detail/src/main/java/com/twix/photolog/detail/PhotologDetailViewModel.ktfeature/photolog/detail/src/main/java/com/twix/photolog/detail/contract/PhotologDetailIntent.ktfeature/photolog/detail/src/main/java/com/twix/photolog/detail/contract/PhotologDetailUiState.ktfeature/photolog/detail/src/main/java/com/twix/photolog/detail/preview/PhotologDetailPreviewProvider.ktfeature/photolog/editor/src/main/java/com/twix/photolog/editor/PhotologEditorRoute.ktfeature/photolog/editor/src/main/java/com/twix/photolog/editor/PhotologEditorViewModel.ktfeature/photolog/editor/src/main/java/com/twix/photolog/editor/contract/PhotologEditorIntent.ktfeature/photolog/editor/src/main/java/com/twix/photolog/editor/contract/PhotologEditorUiState.ktfeature/settings/src/main/java/com/twix/settings/SettingsIntent.ktfeature/settings/src/main/java/com/twix/settings/SettingsScreen.ktfeature/settings/src/main/java/com/twix/settings/SettingsViewModel.ktfeature/settings/src/main/java/com/twix/settings/account/SettingsAccountScreen.ktfeature/settings/src/main/java/com/twix/settings/model/SettingsUiState.ktfeature/stats/detail/src/main/java/com/twix/stats/detail/StatsDetailScreen.ktfeature/stats/detail/src/main/java/com/twix/stats/detail/StatsDetailViewModel.ktfeature/stats/detail/src/main/java/com/twix/stats/detail/contract/StatsDetailIntent.ktfeature/stats/detail/src/main/java/com/twix/stats/detail/contract/StatsDetailUiState.ktfeature/stats/detail/src/main/java/com/twix/stats/detail/preview/StatsDetailUiStatePreviewProvider.kt
💤 Files with no reviewable changes (1)
- core/ui/src/main/java/com/twix/ui/base/LoadableState.kt
dogmania
requested changes
May 19, 2026
Contributor
dogmania
left a comment
dogmania
left a comment
There was a problem hiding this comment.
고생하셨습니다~!
이제 화면 구현할 떄 초기 데이터 로드가 필요한 경우는 ContentLoadableState, 데이터 없이 로딩이 필요한 경우는 DefaultLoadableState 이렇게 구현하면 되는 걸까요??
| val hasNext: Boolean = true, | ||
| override val isLoading: Boolean = false, | ||
| override val hasLoadedContent: Boolean = false, | ||
| override val isLoading: Boolean = true, |
Contributor
There was a problem hiding this comment.
초기값을 true로 설정하고 있고 뷰모델에 있는 fetchInitialNotificationList()에서는 currentState.isLoading이면 return하고 있어요! 그래서 호출이 안되는 문제가 있을 것 같습니다.
Contributor
Author
There was a problem hiding this comment.
맞아 ! 초기값이 true라서 fetchInitialNotificationList()의 currentState.isLoading guard에 막히는 문제가 있었네
NotificationUiState의 isLoading 기본값을 false로 변경해서 초기 목록 요청이 정상적으로 실행되도록 수정했어 :)
반영 커밋: c3a4660
Comment on lines
+119
to
+124
| viewModelScope.launch { | ||
| when (val result = notificationRepository.markNotificationAsRead(notification.id)) { | ||
| is AppResult.Success -> Unit | ||
| is AppResult.Error -> handleError(result.error) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
지금 markNotificationAsRead() 호출하고 있는 메서드가 suspend 메서드예요 그래서 markNotificationAsRead 내부에서 코루틴 스코프를 새로 생성하는 것보다는 markNotificationAsRead를 suspend로 변경하고 뷰모델 스코프를 생성하지 않는 게 더 좋을 것 같아요!
Comment on lines
+79
to
+84
| viewModelScope.launch { | ||
| when (val result = notificationRepository.markAllNotificationsAsRead()) { | ||
| is AppResult.Success -> Unit | ||
| is AppResult.Error -> handleError(result.error) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
여기에서 기존에 사용하던 launchResult를 바꾼 이유가 뭔지 알 수 있을까요? 지금 BaseViewModel 코드를 보면 launchResult 내부에서 handleApiResult에서 api 호출 결과를 가지고 에러 로깅을 하고 있어서 그대로 둬도 로깅은 될 것 같습니다
Contributor
There was a problem hiding this comment.
혹시 launchResult가 공용으로 에러, 로딩을 처리하고 있는 것 때문에 추가하신 거면 아래쪽에도 동일한 코드가 있어서 BaseViewModel에 별도 헬퍼 메서드를 추가하는 게 더 좋을 것 같아요
Contributor
Author
There was a problem hiding this comment.
의도는 읽음 처리 요청이 best-effort 성격이라 launchResult의 공통 loading/error 상태 갱신을 타지 않도록 하려는 것이어 !
현수가 말한 것처럼 같은 패턴이 아래에도 있어서, BaseViewModel에 handleResultWithoutLoadableStateUpdate() 헬퍼를 추가하고 알림 읽음 처리 두 곳에서 사용하도록 수정했오
이 헬퍼는 AppResult 에러 로깅은 유지하되 isLoading, error 상태는 갱신하지 않는데,
적절한 방법인지 의견 부탁해 :)
Comment on lines
+20
to
+21
| override val hasLoadedContent | ||
| get() = isLoadedInProgressStats || isLoadedCompletedStats |
Contributor
There was a problem hiding this comment.
showError를 계산할 때 hasLoadedContent를 사용하고 있는데 하나만 성공하고 하나는 실패한 경우에는 showError가 false가 되어서 에러 화면이 따로 렌더링되지 않을 것 같아요! 에러는 두가지 모두 실패한 경우에만 렌더링되는 건가요??
Contributor
Author
There was a problem hiding this comment.
앗 이 부분은 둘중 하나가 실패했을 경우 실패로 봐야하는데, 내가잘못 설정했어 !!
반영 커밋 : fbc525b
Merged
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/notification/src/main/java/com/twix/notification/NotificationViewModel.kt (1)
105-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick win알림 클릭 후 네비게이션이 “읽음 처리” 네트워크 응답을 기다리지 않도록 분리 필요
handleNotificationClick(suspend)에서markNotificationAsRead(suspend)를 호출한 다음에야emitSideEffect(NavigateTo...)가 실행됩니다.markNotificationAsRead내부에서handleResultWithoutLoadableStateUpdate(notificationRepository.markNotificationAsRead(...))를 suspend로 대기하므로, 읽음 API 응답 지연이 그대로 네비게이션 지연으로 이어질 수 있습니다.- 주석/정책상 “best effort”이면 로컬 상태 반영은 즉시 하고(현재
reduce), 원격 호출은 fire-and-forget으로 분리하는 편이 UX에 더 안전합니다. 네비게이션을 먼저 내보내도 정합성 문제가 없을까요?제안 수정안
- private suspend fun markNotificationAsRead(notification: Notification) { + private fun markNotificationAsRead(notification: Notification) { reduce { copy( notificationList = currentState.notificationList.map { item -> if (item.id == notification.id) item.copy(isRead = true) else item }, ) } - handleResultWithoutLoadableStateUpdate(notificationRepository.markNotificationAsRead(notification.id)) + viewModelScope.launch { + handleResultWithoutLoadableStateUpdate(notificationRepository.markNotificationAsRead(notification.id)) + } }🤖 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 `@feature/notification/src/main/java/com/twix/notification/NotificationViewModel.kt` around lines 105 - 115, The network call in markNotificationAsRead is currently awaited, causing handleNotificationClick to delay navigation; change it to fire-and-forget by performing the local reduce immediately and launching the remote call asynchronously (e.g., use viewModelScope.launch or a background coroutine) so handleNotificationClick can emitSideEffect(NavigateTo...) without waiting; move the call to handleResultWithoutLoadableStateUpdate(notificationRepository.markNotificationAsRead(...)) into that launched coroutine and ensure any errors are caught/logged there to preserve best-effort behavior while keeping the UI responsive.
🤖 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.
Outside diff comments:
In
`@feature/notification/src/main/java/com/twix/notification/NotificationViewModel.kt`:
- Around line 105-115: The network call in markNotificationAsRead is currently
awaited, causing handleNotificationClick to delay navigation; change it to
fire-and-forget by performing the local reduce immediately and launching the
remote call asynchronously (e.g., use viewModelScope.launch or a background
coroutine) so handleNotificationClick can emitSideEffect(NavigateTo...) without
waiting; move the call to
handleResultWithoutLoadableStateUpdate(notificationRepository.markNotificationAsRead(...))
into that launched coroutine and ensure any errors are caught/logged there to
preserve best-effort behavior while keeping the UI responsive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7380ef93-16be-4f36-9b59-bb2841663369
📒 Files selected for processing (3)
core/ui/src/main/java/com/twix/ui/base/BaseViewModel.ktfeature/notification/src/main/java/com/twix/notification/NotificationViewModel.ktfeature/notification/src/main/java/com/twix/notification/contract/NotificationUiState.kt
…leted stats loaded
chanho0908
commented
May 24, 2026
Contributor
Author
chanho0908
left a comment
chanho0908
left a comment
There was a problem hiding this comment.
응 맞아!
ContentLoadableState는 초기 데이터 로드가 필요한 화면에서 최초 로딩/에러와 이후 콘텐츠 유지 상태를 구분하기 위한 용도로 사용하고,
DefaultLoadableState는 데이터 유지 여부와 관계없이 단순히 로딩/에러 상태만 필요한 화면에서 사용하면 될 것 같아
예를 들어 무한 스크롤 화면처럼 첫 로드 이후 기존 UI 위에 loading overlay를 보여줘야 하는 화면은 ContentLoadableState,
버튼 액션이나 폼 제출처럼 별도 콘텐츠 로드 상태가 필요 없는 경우는 DefaultLoadableState로 보면 될 것 같아
| val hasNext: Boolean = true, | ||
| override val isLoading: Boolean = false, | ||
| override val hasLoadedContent: Boolean = false, | ||
| override val isLoading: Boolean = true, |
Contributor
Author
There was a problem hiding this comment.
맞아 ! 초기값이 true라서 fetchInitialNotificationList()의 currentState.isLoading guard에 막히는 문제가 있었네
NotificationUiState의 isLoading 기본값을 false로 변경해서 초기 목록 요청이 정상적으로 실행되도록 수정했어 :)
반영 커밋: c3a4660
Comment on lines
+119
to
+124
| viewModelScope.launch { | ||
| when (val result = notificationRepository.markNotificationAsRead(notification.id)) { | ||
| is AppResult.Success -> Unit | ||
| is AppResult.Error -> handleError(result.error) | ||
| } | ||
| } |
Comment on lines
+79
to
+84
| viewModelScope.launch { | ||
| when (val result = notificationRepository.markAllNotificationsAsRead()) { | ||
| is AppResult.Success -> Unit | ||
| is AppResult.Error -> handleError(result.error) | ||
| } | ||
| } |
Contributor
Author
There was a problem hiding this comment.
의도는 읽음 처리 요청이 best-effort 성격이라 launchResult의 공통 loading/error 상태 갱신을 타지 않도록 하려는 것이어 !
현수가 말한 것처럼 같은 패턴이 아래에도 있어서, BaseViewModel에 handleResultWithoutLoadableStateUpdate() 헬퍼를 추가하고 알림 읽음 처리 두 곳에서 사용하도록 수정했오
이 헬퍼는 AppResult 에러 로깅은 유지하되 isLoading, error 상태는 갱신하지 않는데,
적절한 방법인지 의견 부탁해 :)
Comment on lines
+20
to
+21
| override val hasLoadedContent | ||
| get() = isLoadedInProgressStats || isLoadedCompletedStats |
Contributor
Author
There was a problem hiding this comment.
앗 이 부분은 둘중 하나가 실패했을 경우 실패로 봐야하는데, 내가잘못 설정했어 !!
반영 커밋 : fbc525b
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
2 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.
이슈 번호
#157
작업내용
LoadableState를DefaultLoadableState/ContentLoadableState구조로 정리하고, 관련 ViewModel 상태 업데이트 로직을 맞췄습니다.LoadableState를 분리한 이유는 화면마다 필요한 상태가 달랐기 때문입니다.isLoading(지금 요청 중인지),error(에러가 났는지)만 알면 충분했습니다.이 역할이
hasLoadedContenthasLoadedContent가 필요한 이유는 로딩/에러를 두 가지 방식으로 다르게 보여줘야 했기 때문입니다.DefaultLoadableState와, 이미 데이터를 보여준 적 있는지까지 아는ContentLoadableState를 분리했습니다.결과물
리뷰어에게 추가로 요구하는 사항 (선택)
DefaultLoadableState,ContentLoadableState) 방향이 팀 컨벤션에 잘 맞는지 중점적으로 봐주시면 좋겠습니다.