기념일 설정 화면 폴링 처리 추가#166
Merged
Merged
Conversation
chanho0908
force-pushed
the
feature/#162-onboarding-anniversary-policy
branch
from
374a8dd to
0ba0a22
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthrough온보딩 기념일 정책 변경 작업으로서, 상대방이 먼저 기념일을 등록한 경우를 감지하는 기능을 구현합니다. D-day 화면에 상태 폴링 메커니즘을 추가하여 온보딩 상태가 COMPLETED로 변경될 때까지 주기적으로 서버에서 상태를 확인합니다. 폴링 중 COMPLETED 상태를 감지하면 "짝꿍이 기념일을 먼저 등록했어요" 토스트 메시지를 표시한 후 사용자를 홈 화면으로 이동시킵니다. 온보딩 ViewModel의 폴링 로직을 startStatusPolling 통합 메서드로 리팩터링하여 커플 연결 대기와 기념일 등록 완료 대기 두 가지 시나리오를 조건 기반으로 처리합니다. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
feature/stats/detail/src/main/java/com/twix/stats/detail/StatsDetailViewModel.kt (1)
67-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick win초기 통계 fetch는 동시 실행 방지가 필요합니다.
Line [67]~Line [76], Line [210]에서
fetchInitialStats()가 호출될 때마다 새 코루틴을 시작해, 빠른 재시도 시 요청이 중첩되고 늦게 끝난 이전 응답이 최신 상태를 덮어쓸 수 있습니다. 초기 fetch Job을 1개로 관리해 취소/중복방지 처리해 주세요.As per coding guidelines "feature/**: 단방향 데이터 플로우(Intent → ViewModel → State → View)가 유지되는가?".제안 diff
import kotlinx.coroutines.FlowPreview +import kotlinx.coroutines.Job import kotlinx.coroutines.async @@ class StatsDetailViewModel( @@ ) : BaseViewModel<StatsDetailUiState, StatsDetailIntent, StatsDetailSideEffect>( StatsDetailUiState(), ) { + private var initialFetchJob: Job? = null @@ private fun fetchInitialStats() { + if (initialFetchJob?.isActive == true) return val initialDate = LocalDate.parse(argDate).let(YearMonth::from) - viewModelScope.launch { + initialFetchJob = viewModelScope.launch { reduce { copyState(isLoading = true, error = null) as StatsDetailUiState } val (summary, detail) = fetchStats(initialDate) handleFetchStatsDetailResult(summary, detail, initialDate) } }Also applies to: 210-210
🤖 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 67 - 76, Add single-job management to fetchInitialStats by introducing a private Job property (e.g., initialFetchJob) and use it to prevent concurrent fetches: before calling viewModelScope.launch in fetchInitialStats cancel any previous job or return when initialFetchJob?.isActive == true, then assign the new coroutine to initialFetchJob; ensure the coroutine still updates state and calls fetchStats(initialDate) and handleFetchStatsDetailResult(summary, detail, initialDate). This keeps viewModelScope.launch usage but ensures duplicate/cancel behavior for fetchInitialStats (also applied to the other call site at line ~210).feature/notification/src/main/java/com/twix/notification/NotificationScreen.kt (1)
92-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLaunchedEffect 내부에서
uiState.canLoadNextPage가 오래된 값으로 캡처될 수 있어 페이징 트리거가 흔들립니다(중복 Intent/요청 위험).
NotificationScreen.kt의LaunchedEffect(listState, uiState.notificationList.size)는 키에canLoadNextPage(=hasNext && !isLoading)가 없어서, 로딩/다음페이지 여부가 바뀌어도 effect 내부의if (uiState.canLoadNextPage)는 이전 캡처값을 사용할 수 있습니다. 그 결과 스크롤 조건이true → false → true로 다시 변하는 경우 로딩 중이거나hasNext=false인데도FetchNextPage가 다시 dispatch될 수 있습니다(뷰모델의currentState.hasNext/isLoading가드로 완전한 중복 요청은 완화되더라도, 타이밍 의존의 불필요한 트리거가 생길 여지가 있습니다).
rememberUpdatedState로 최신canLoadNextPage를 effect 내부에서 참조하도록 바꾸는 방식이 가장 안전합니다.수정 예시
- LaunchedEffect(listState, uiState.notificationList.size) { + val canLoadNextPage by rememberUpdatedState(uiState.canLoadNextPage) + + LaunchedEffect(listState, uiState.notificationList.size, canLoadNextPage) { snapshotFlow { val layoutInfo = listState.layoutInfo val totalCount = layoutInfo.totalItemsCount val lastVisibleIndex = layoutInfo.visibleItemsInfo.lastOrNull()?.index ?: -1 // 마지막 3개쯤 보일 때 미리 조회 totalCount > 0 && lastVisibleIndex >= totalCount - 3 }.distinctUntilChanged() .filter { it } .collect { - if (uiState.canLoadNextPage) { + if (canLoadNextPage) { onNextPage() } } }로딩 중에도 마지막 아이템 근처 스크롤이 다시 “조건 true”로 바뀌면, 추가 fetch를 막는 게 의도된 동작인가요?
🤖 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 effect captures an outdated uiState.canLoadNextPage causing spurious triggers; wrap uiState.canLoadNextPage with rememberUpdatedState (e.g. val canLoadNextPageState = rememberUpdatedState(uiState.canLoadNextPage)) and then inside the LaunchedEffect(listState, uiState.notificationList.size) replace direct checks of uiState.canLoadNextPage with canLoadNextPageState.value so snapshotFlow/collect uses the latest value when deciding to call onNextPage(); keep the existing keys (listState and uiState.notificationList.size) for the LaunchedEffect and only swap the conditional to use the remembered updated state to avoid stale captures.feature/photolog/editor/src/main/java/com/twix/photolog/editor/PhotologEditorViewModel.kt (1)
57-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick win저장 요청 중복 실행 방지 가드가 필요합니다.
왜 문제인지: Line 63-75는
isSaving을 표시만 하고, 재진입을 막지는 않습니다. 사용자가 빠르게 저장을 연타하면modifyPhotolog가 중복 호출될 수 있어요.
어떻게 개선할지:modifyComment()시작 시currentState.isSaving가드를 추가해 단일 in-flight 요청만 허용해 주세요.수정 예시
private fun modifyComment() { + if (currentState.isSaving) return + if (currentState.comment.canUpload.not()) { showToast(R.string.toast_comment_length_guide, ToastType.DEFAULT) } else if (currentState.isCommentNotChanged) { showToast(R.string.toast_comment_not_modified, ToastType.DEFAULT) } else {🤖 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/photolog/editor/src/main/java/com/twix/photolog/editor/PhotologEditorViewModel.kt` around lines 57 - 75, Add a reentrancy guard at the start of modifyComment(): check currentState.isSaving and return immediately if true so only one in-flight save is allowed; keep the existing reduce { copy(isSaving = true) }/onFinally logic and ensure the guard references the same state flag (currentState.isSaving / isSaving) used around launchResult and launchModifyComment so rapid repeated taps won’t trigger duplicate launches.
🧹 Nitpick comments (4)
feature/login/src/main/java/com/twix/login/LoginScreen.kt (1)
76-89: 💤 Low value상태 기반 로딩 가드 구현이 잘 되어 있습니다.
uiState.isLoading가드와 버튼 레벨의isAuthenticating체크를 통해 중복 로그인 요청을 이중으로 방지하고 있어 안정적입니다.한 가지 참고 사항: OAuth 제공자(
kakaoLoginProvider.login(),googleLoginProvider.login()) 호출 중에는 ViewModel의isLoading상태가 아직true가 아니므로, 이 구간에서 빠른 더블 클릭이 발생할 수 있습니다. 다만 대부분의 OAuth SDK가 내부적으로 중복 요청을 처리하므로 실질적인 문제는 적을 것으로 보입니다. 더 엄격한 제어가 필요하다면 로컬 상태나MutableStateFlow<Boolean>을 활용한 락 패턴을 고려해볼 수 있습니다.🤖 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/login/src/main/java/com/twix/login/LoginScreen.kt` around lines 76 - 89, The current double-click protection uses uiState.isLoading and the LoginScreen isAuthenticating prop but because uiState.isLoading may not be set true until after kakaoLoginProvider.login()/googleLoginProvider.login() returns, rapid double clicks can trigger multiple OAuth calls; fix by adding a local in-scope guard (e.g., a boolean or a MutableStateFlow<Boolean> "isSubmitting" or lock) around the coroutineScope.launch branch so you set the lock immediately before calling kakaoLoginProvider.login()/googleLoginProvider.login() and clear it after dispatching viewModel.dispatch(LoginIntent.Login(result)), ensuring LoginScreen/coroutineScope.launch and the provider calls are protected atomically from rapid re-entry.feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageScreen.kt (1)
96-123: ⚡ Quick win로딩/에러 분기 추가에 맞춘 상태별 Preview를 함께 두면 유지보수가 더 쉬워집니다.
Line 150 이후로
showLoading/showError/else분기가 생겨 UI 회귀 가능성이 커졌는데, 현재 파일 기준으로 상태별 Preview가 없습니다.loading,error,content3가지를 분리해서 Preview를 추가해두면 QA 전에 빠르게 확인할 수 있어요. 이 방향으로 보강해보실까요?As per coding guidelines
feature/**: "Preview Composable이 제공되는가?"Also applies to: 150-257
🤖 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/GoalManageScreen.kt` around lines 96 - 123, Add three Preview composables for GoalManageScreen to cover loading, error, and content states: create `@Preview` functions that call GoalManageScreen with GoalManageUiState variants (e.g., a uiState representing loading (isLoading=true), an error state (error message set), and a populated content state), supply openedMenuGoalId as null or a sample id and use no-op lambdas for onBack/onRetry/onSelectDate/etc.; ensure each Preview is named clearly (e.g., PreviewGoalManageLoading, PreviewGoalManageError, PreviewGoalManageContent) so they live alongside the GoalManageScreen/GoalManageUiState definitions and make UI regressions visible.core/ui/src/main/java/com/twix/ui/base/BaseViewModel.kt (1)
218-226: 💤 Low value중복 타입 체크로 인한 불필요한 코드
라인 219에서
currentState !is DefaultLoadableState를 체크하고 early return하므로, 라인 222의 safe castthis as? DefaultLoadableState는 항상 성공합니다. 따라서 null 체크가 불필요합니다.개선 제안:
♻️ 중복 체크 제거
private inline fun reduceLoadableState(crossinline reducer: DefaultLoadableState.() -> DefaultLoadableState) { if (currentState !is DefaultLoadableState) return reduce { - val loadableState = this as? DefaultLoadableState ?: return@reduce this + val loadableState = this as DefaultLoadableState `@Suppress`("UNCHECKED_CAST") loadableState.reducer() as S } }참고: 라인 224의
UNCHECKED_CAST는DefaultLoadableState.copyState의 반환 타입 설계 이슈로 인한 것이므로, DefaultLoadableState.kt의 self-type 패턴 적용으로 근본적으로 해결할 수 있습니다.🤖 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/ui/src/main/java/com/twix/ui/base/BaseViewModel.kt` around lines 218 - 226, 현재 reduceLoadableState에서 외부에선 currentState 타입을 확인한 뒤(reduceLoadableState) 내부 reduce 블록에서 다시 안전 캐스트(this as? DefaultLoadableState)를 수행하고 있어 중복 검사와 불필요한 null handling이 발생합니다; reduce 람다 내의 캐스트를 안전 캐스트 대신 단순 non-null 캐스트로 변경하여(val loadableState = this as DefaultLoadableState) 불필요한 반환 경로를 제거하고 기존 `@Suppress`("UNCHECKED_CAST")는 유지한 채 DefaultLoadableState.copyState 관련 self-type 이슈는 별도 파일에서 해결하도록 남겨두세요 (참조 심볼: reduceLoadableState, DefaultLoadableState, reduce).feature/settings/src/main/java/com/twix/settings/account/SettingsAccountScreen.kt (1)
141-150: ⚡ Quick winCommonDialog의 confirm/dismiss 의미를 표준에 맞춰 정렬해 주세요.
Line [143]~Line [149]처럼 버튼 텍스트와 콜백을 역으로 매핑하면, 지금은 동작해도 유지보수 시 실수 가능성이 커집니다.
confirm=탈퇴,dismiss=취소로 맞추는 쪽이 안전합니다. 이 방향으로 통일해보실까요?제안 diff
CommonDialog( visible = showWithdrawDialog, - confirmText = stringResource(R.string.word_cancel), - dismissText = stringResource(R.string.action_withdraw_account), + confirmText = stringResource(R.string.action_withdraw_account), + dismissText = stringResource(R.string.word_cancel), onDismiss = { showWithdrawDialog = false - onWithdrawAccount() }, - onConfirm = { showWithdrawDialog = false }, + onConfirm = { + showWithdrawDialog = false + onWithdrawAccount() + }, onDismissRequest = { showWithdrawDialog = false },🤖 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/settings/src/main/java/com/twix/settings/account/SettingsAccountScreen.kt` around lines 141 - 150, The confirm/dismiss labels and callbacks in CommonDialog are reversed; update CommonDialog invocation so confirmText is the withdraw label and triggers the withdraw action, and dismissText is the cancel label and only hides the dialog: set confirmText to stringResource(R.string.action_withdraw_account) and call onWithdrawAccount() from onConfirm (then hide dialog), set dismissText to stringResource(R.string.word_cancel) and make onDismiss only set showWithdrawDialog = false; keep onDismissRequest = { showWithdrawDialog = false } as-is. Reference: CommonDialog, showWithdrawDialog, onWithdrawAccount.
🤖 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 `@core/ui/src/main/java/com/twix/ui/base/DefaultLoadableState.kt`:
- Around line 15-18: The copyState method currently returns the concrete
DefaultLoadableState interface which loses concrete types and forces unsafe
casts in BaseViewModel.reduceLoadableState; change DefaultLoadableState to a
generic self-type (e.g., interface DefaultLoadableState<S :
DefaultLoadableState<S>>) and update copyState to return S so each
implementation (e.g., LoginUiState) can return its own type, then update
BaseViewModel.reduceLoadableState to work with the generic S and remove the
`@Suppress`("UNCHECKED_CAST")/as S casts; adjust all implementing classes to
implement DefaultLoadableState<TheirType> and implement copyState returning
their concrete type.
In
`@feature/photolog/detail/src/main/java/com/twix/photolog/detail/contract/PhotologDetailUiState.kt`:
- Line 182: The current fallback returns PhotologDetailUiState(isLoading =
false) when the goal lookup fails, causing goalId to become -1L and selectedDate
to default to now(), which leads to invalid subsequent actions; instead modify
the fallback in the logic that builds PhotologDetailUiState (the branch that
currently does "?: return PhotologDetailUiState(isLoading = false)") to preserve
the original route inputs (goalId, selectedDate, betweenUs) and set an explicit
error state (e.g., isError = true or an errorMessage) so callers know the goal
was missing rather than receiving reset default identifiers; update the
construction of PhotologDetailUiState accordingly to use the incoming parameters
and an error flag so upload/edit flows won't proceed with invalid params.
In
`@feature/photolog/detail/src/main/java/com/twix/photolog/detail/PhotologDetailViewModel.kt`:
- Around line 201-204: The handlePokeCooldown function currently only clears
isPoking and shows a toast but doesn't persist the remaining cooldown to state;
update the reduce call inside handlePokeCooldown to set pokeCooldownRemaining =
remainingMs while keeping isPoking = false, then continue to emit the
PhotologDetailSideEffect.ShowPokeCooldownToast(remainingMs) as before (function:
handlePokeCooldown, state field: pokeCooldownRemaining).
- Around line 172-176: Add a guard to prevent concurrent poke executions in
pokeToPartner(): check an isPoking boolean at the start (return early if true),
set isPoking = true immediately before launching the operation (or before
calling startPokeLoading()), and ensure isPoking is reset to false in all
completion paths after handlePokeResult returns (and optionally implement a
short cooldown if desired). Update references to pokeToPartner(),
viewModelScope.launch, startPokeLoading(), handlePokeResult, and
pokeGoalUseCase.invoke(argGoalId) so the flag wraps the invoke call to guarantee
only one in-flight poke.
---
Outside diff comments:
In
`@feature/notification/src/main/java/com/twix/notification/NotificationScreen.kt`:
- Around line 92-106: The effect captures an outdated uiState.canLoadNextPage
causing spurious triggers; wrap uiState.canLoadNextPage with
rememberUpdatedState (e.g. val canLoadNextPageState =
rememberUpdatedState(uiState.canLoadNextPage)) and then inside the
LaunchedEffect(listState, uiState.notificationList.size) replace direct checks
of uiState.canLoadNextPage with canLoadNextPageState.value so
snapshotFlow/collect uses the latest value when deciding to call onNextPage();
keep the existing keys (listState and uiState.notificationList.size) for the
LaunchedEffect and only swap the conditional to use the remembered updated state
to avoid stale captures.
In
`@feature/photolog/editor/src/main/java/com/twix/photolog/editor/PhotologEditorViewModel.kt`:
- Around line 57-75: Add a reentrancy guard at the start of modifyComment():
check currentState.isSaving and return immediately if true so only one in-flight
save is allowed; keep the existing reduce { copy(isSaving = true) }/onFinally
logic and ensure the guard references the same state flag (currentState.isSaving
/ isSaving) used around launchResult and launchModifyComment so rapid repeated
taps won’t trigger duplicate launches.
In
`@feature/stats/detail/src/main/java/com/twix/stats/detail/StatsDetailViewModel.kt`:
- Around line 67-76: Add single-job management to fetchInitialStats by
introducing a private Job property (e.g., initialFetchJob) and use it to prevent
concurrent fetches: before calling viewModelScope.launch in fetchInitialStats
cancel any previous job or return when initialFetchJob?.isActive == true, then
assign the new coroutine to initialFetchJob; ensure the coroutine still updates
state and calls fetchStats(initialDate) and
handleFetchStatsDetailResult(summary, detail, initialDate). This keeps
viewModelScope.launch usage but ensures duplicate/cancel behavior for
fetchInitialStats (also applied to the other call site at line ~210).
---
Nitpick comments:
In `@core/ui/src/main/java/com/twix/ui/base/BaseViewModel.kt`:
- Around line 218-226: 현재 reduceLoadableState에서 외부에선 currentState 타입을 확인한
뒤(reduceLoadableState) 내부 reduce 블록에서 다시 안전 캐스트(this as? DefaultLoadableState)를
수행하고 있어 중복 검사와 불필요한 null handling이 발생합니다; reduce 람다 내의 캐스트를 안전 캐스트 대신 단순
non-null 캐스트로 변경하여(val loadableState = this as DefaultLoadableState) 불필요한 반환 경로를
제거하고 기존 `@Suppress`("UNCHECKED_CAST")는 유지한 채 DefaultLoadableState.copyState 관련
self-type 이슈는 별도 파일에서 해결하도록 남겨두세요 (참조 심볼: reduceLoadableState,
DefaultLoadableState, reduce).
In `@feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageScreen.kt`:
- Around line 96-123: Add three Preview composables for GoalManageScreen to
cover loading, error, and content states: create `@Preview` functions that call
GoalManageScreen with GoalManageUiState variants (e.g., a uiState representing
loading (isLoading=true), an error state (error message set), and a populated
content state), supply openedMenuGoalId as null or a sample id and use no-op
lambdas for onBack/onRetry/onSelectDate/etc.; ensure each Preview is named
clearly (e.g., PreviewGoalManageLoading, PreviewGoalManageError,
PreviewGoalManageContent) so they live alongside the
GoalManageScreen/GoalManageUiState definitions and make UI regressions visible.
In `@feature/login/src/main/java/com/twix/login/LoginScreen.kt`:
- Around line 76-89: The current double-click protection uses uiState.isLoading
and the LoginScreen isAuthenticating prop but because uiState.isLoading may not
be set true until after kakaoLoginProvider.login()/googleLoginProvider.login()
returns, rapid double clicks can trigger multiple OAuth calls; fix by adding a
local in-scope guard (e.g., a boolean or a MutableStateFlow<Boolean>
"isSubmitting" or lock) around the coroutineScope.launch branch so you set the
lock immediately before calling
kakaoLoginProvider.login()/googleLoginProvider.login() and clear it after
dispatching viewModel.dispatch(LoginIntent.Login(result)), ensuring
LoginScreen/coroutineScope.launch and the provider calls are protected
atomically from rapid re-entry.
In
`@feature/settings/src/main/java/com/twix/settings/account/SettingsAccountScreen.kt`:
- Around line 141-150: The confirm/dismiss labels and callbacks in CommonDialog
are reversed; update CommonDialog invocation so confirmText is the withdraw
label and triggers the withdraw action, and dismissText is the cancel label and
only hides the dialog: set confirmText to
stringResource(R.string.action_withdraw_account) and call onWithdrawAccount()
from onConfirm (then hide dialog), set dismissText to
stringResource(R.string.word_cancel) and make onDismiss only set
showWithdrawDialog = false; keep onDismissRequest = { showWithdrawDialog = false
} as-is. Reference: CommonDialog, showWithdrawDialog, onWithdrawAccount.
🪄 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: dbadb2d2-d9ed-4801-9ad9-a4782e724d24
📒 Files selected for processing (58)
.gitignoreapp/src/main/AndroidManifest.xmlcore/design-system/src/main/res/values/strings.xmlcore/result/src/main/java/com/twix/result/Extension.ktcore/share/src/main/java/com/twix/share/InviteLaunchDispatcher.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
Comment on lines
+15
to
+18
| fun copyState( | ||
| isLoading: Boolean = this.isLoading, | ||
| error: AppError? = this.error, | ||
| ): DefaultLoadableState |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
copyState 반환 타입으로 인한 타입 안정성 저하 우려
현재 copyState 메서드가 DefaultLoadableState 인터페이스 타입을 반환하도록 정의되어 있는데, 이로 인해 몇 가지 문제가 발생할 수 있습니다:
- 구체 타입 손실: 구현체(예: data class)가
copyState를 호출할 때 자신의 타입이 아닌 인터페이스 타입을 받게 됩니다 - Unsafe cast 강제: BaseViewModel.reduceLoadableState (line 224)에서
@Suppress("UNCHECKED_CAST")및as S캐스팅이 필수적으로 필요합니다 - 런타임 오류 위험: 타입 캐스팅이 컴파일 타임에 검증되지 않아 잠재적 ClassCastException 위험이 있습니다
개선 방안: Self-type 패턴을 사용하여 구현체가 자신의 타입을 반환하도록 개선할 수 있습니다:
♻️ Self-type 패턴 적용 예시
-interface DefaultLoadableState : State {
+interface DefaultLoadableState<out T : DefaultLoadableState<T>> : State {
val isLoading: Boolean
val error: AppError?
fun copyState(
isLoading: Boolean = this.isLoading,
error: AppError? = this.error,
- ): DefaultLoadableState
+ ): T
}구현 예시:
data class LoginUiState(
override val isLoading: Boolean = false,
override val error: AppError? = null,
val isAuthenticating: Boolean = false
) : DefaultLoadableState<LoginUiState> {
override fun copyState(
isLoading: Boolean,
error: AppError?
): LoginUiState = copy(isLoading = isLoading, error = error)
}이렇게 하면 BaseViewModel에서 unsafe cast 없이 타입 안전하게 처리할 수 있습니다.
다만 이미 여러 화면에서 구현이 진행 중인 것으로 보이므로, 팀 내 논의를 통해 마이그레이션 범위와 우선순위를 결정하는 것이 좋겠습니다.
🤖 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/ui/src/main/java/com/twix/ui/base/DefaultLoadableState.kt` around lines
15 - 18, The copyState method currently returns the concrete
DefaultLoadableState interface which loses concrete types and forces unsafe
casts in BaseViewModel.reduceLoadableState; change DefaultLoadableState to a
generic self-type (e.g., interface DefaultLoadableState<S :
DefaultLoadableState<S>>) and update copyState to return S so each
implementation (e.g., LoginUiState) can return its own type, then update
BaseViewModel.reduceLoadableState to work with the generic S and remove the
`@Suppress`("UNCHECKED_CAST")/as S casts; adjust all implementing classes to
implement DefaultLoadableState<TheirType> and implement copyState returning
their concrete type.
| goals.firstOrNull { | ||
| it.goalId == goalId | ||
| } ?: return PhotologDetailUiState() | ||
| } ?: return PhotologDetailUiState(isLoading = false) |
There was a problem hiding this comment.
목표 미조회 fallback에서 식별자/날짜가 초기화되어 잘못된 후속 액션으로 이어질 수 있습니다.
Line 182는 goal을 찾지 못했을 때 기본 상태를 반환해서 goalId = -1L, selectedDate = now()가 됩니다. 이 상태에서 업로드/수정 액션이 실행되면 잘못된 파라미터로 이동/요청될 수 있습니다.
fallback에서도 라우트 입력값(goalId, selectedDate, betweenUs)은 유지하고, 필요하면 에러 상태로 전환하는 쪽이 안전합니다. 이 케이스는 에러 화면으로 보내는 게 더 의도에 맞을까요?
수정 예시
- } ?: return PhotologDetailUiState(isLoading = false)
+ } ?: return PhotologDetailUiState(
+ goalId = goalId,
+ currentShow = BetweenUs.valueOf(betweenUs),
+ selectedDate = selectedDate,
+ isCompletedGoal = isCompletedGoal,
+ hasLoadedContent = true,
+ isLoading = false,
+ )🤖 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/photolog/detail/src/main/java/com/twix/photolog/detail/contract/PhotologDetailUiState.kt`
at line 182, The current fallback returns PhotologDetailUiState(isLoading =
false) when the goal lookup fails, causing goalId to become -1L and selectedDate
to default to now(), which leads to invalid subsequent actions; instead modify
the fallback in the logic that builds PhotologDetailUiState (the branch that
currently does "?: return PhotologDetailUiState(isLoading = false)") to preserve
the original route inputs (goalId, selectedDate, betweenUs) and set an explicit
error state (e.g., isError = true or an errorMessage) so callers know the goal
was missing rather than receiving reset default identifiers; update the
construction of PhotologDetailUiState accordingly to use the incoming parameters
and an error flag so upload/edit flows won't proceed with invalid params.
Comment on lines
172
to
176
| private fun pokeToPartner() { | ||
| viewModelScope.launch { | ||
| reduce { copy(isPoking = true) } | ||
| when (val result = pokeGoalUseCase.invoke(argGoalId)) { | ||
| is PokeGoalResult.Success -> { | ||
| reduce { copy(isPoking = false, pokeCooldownRemaining = PokeGoalUseCase.COOLDOWN_MS) } | ||
| tryEmitSideEffect(PhotologDetailSideEffect.ShowPokeToast) | ||
| } | ||
| is PokeGoalResult.OnCooldown -> { | ||
| reduce { copy(isPoking = false) } | ||
| tryEmitSideEffect(PhotologDetailSideEffect.ShowPokeCooldownToast(result.remainingMs)) | ||
| } | ||
| PokeGoalResult.Error -> { | ||
| reduce { copy(isPoking = false) } | ||
| showToast(R.string.toast_poke_goal_failed, ToastType.ERROR) | ||
| } | ||
| } | ||
| startPokeLoading() | ||
| handlePokeResult(pokeGoalUseCase.invoke(argGoalId)) | ||
| } |
There was a problem hiding this comment.
Poke 중복 실행을 막는 가드가 필요합니다.
왜 문제인지: Line 173-175 구간은 탭 연타 시 pokeGoalUseCase.invoke(argGoalId)가 병렬로 여러 번 실행될 수 있어요. 외부 쓰기 호출 중복으로 토스트/상태가 꼬일 수 있습니다.
어떻게 개선할지: isPoking(및 필요 시 cooldown) 기준으로 조기 반환 가드를 넣어 단일 실행만 허용해 주세요.
수정 예시
private fun pokeToPartner() {
+ if (currentState.isPoking || currentState.pokeCooldownRemaining > 0L) return
viewModelScope.launch {
startPokeLoading()
handlePokeResult(pokeGoalUseCase.invoke(argGoalId))
}
}🤖 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/photolog/detail/src/main/java/com/twix/photolog/detail/PhotologDetailViewModel.kt`
around lines 172 - 176, Add a guard to prevent concurrent poke executions in
pokeToPartner(): check an isPoking boolean at the start (return early if true),
set isPoking = true immediately before launching the operation (or before
calling startPokeLoading()), and ensure isPoking is reset to false in all
completion paths after handlePokeResult returns (and optionally implement a
short cooldown if desired). Update references to pokeToPartner(),
viewModelScope.launch, startPokeLoading(), handlePokeResult, and
pokeGoalUseCase.invoke(argGoalId) so the flag wraps the invoke call to guarantee
only one in-flight poke.
Comment on lines
+201
to
+204
| private fun handlePokeCooldown(remainingMs: Long) { | ||
| reduce { copy(isPoking = false) } | ||
| tryEmitSideEffect(PhotologDetailSideEffect.ShowPokeCooldownToast(remainingMs)) | ||
| } |
There was a problem hiding this comment.
Cooldown 응답 시 남은 시간을 상태에 반영해야 합니다.
왜 문제인지: Line 201-204에서 remainingMs를 토스트에만 쓰고 상태(pokeCooldownRemaining)에는 저장하지 않아, UI가 cooldown 정보를 잃을 수 있습니다. 그 결과 불필요한 재요청이 반복될 수 있어요.
어떻게 개선할지: cooldown 분기에서 pokeCooldownRemaining = remainingMs를 함께 업데이트해 주세요.
수정 예시
private fun handlePokeCooldown(remainingMs: Long) {
- reduce { copy(isPoking = false) }
+ reduce {
+ copy(
+ isPoking = false,
+ pokeCooldownRemaining = remainingMs,
+ )
+ }
tryEmitSideEffect(PhotologDetailSideEffect.ShowPokeCooldownToast(remainingMs))
}🤖 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/photolog/detail/src/main/java/com/twix/photolog/detail/PhotologDetailViewModel.kt`
around lines 201 - 204, The handlePokeCooldown function currently only clears
isPoking and shows a toast but doesn't persist the remaining cooldown to state;
update the reduce call inside handlePokeCooldown to set pokeCooldownRemaining =
remainingMs while keeping isPoking = false, then continue to emit the
PhotologDetailSideEffect.ShowPokeCooldownToast(remainingMs) as before (function:
handlePokeCooldown, state field: pokeCooldownRemaining).
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/onboarding/src/main/java/com/twix/onboarding/OnBoardingViewModel.kt (1)
104-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick win첫 상태 조회는 지연 없이 시작해 주세요.
Line 111에서 첫
fetchOnboardingStatus()전에delay()를 걸어서, 프로필 단계 직후 상태가COMPLETED로 바뀐 레이스에서는 D-day 화면이 최대 3초 동안 노출됩니다. 요구사항이 “즉시” 홈 이동이라면 첫 회 조회는 바로 실행하고, 이후 반복에만 interval을 두는 쪽이 안전합니다.예시 수정
onboardingStatusJob = viewModelScope.launch { var consecutiveFailureCount = 0 while (isActive) { - delay(POLLING_INTERVAL_MS) when (val result = onBoardingRepository.fetchOnboardingStatus()) { is AppResult.Success -> { consecutiveFailureCount = 0 if (onStatusFetched(result.data)) { onboardingStatusJob = null break } } is AppResult.Error -> { if (++consecutiveFailureCount >= MAX_POLLING_FAILURE_COUNT) { onboardingStatusJob = null break } } } + delay(POLLING_INTERVAL_MS) } }🤖 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/OnBoardingViewModel.kt` around lines 104 - 129, The polling currently delays before the first fetch which can hide an immediate COMPLETED status; in startStatusPolling, call onBoardingRepository.fetchOnboardingStatus() immediately once (invoking onStatusFetched with the result and handling success/error and consecutiveFailureCount), and then enter the while(isActive) loop that delays POLLING_INTERVAL_MS before subsequent fetches; ensure you still set onboardingStatusJob = null when onStatusFetched returns true or when consecutiveFailureCount >= MAX_POLLING_FAILURE_COUNT, and preserve existing error handling and job lifecycle 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.
Outside diff comments:
In `@feature/onboarding/src/main/java/com/twix/onboarding/OnBoardingViewModel.kt`:
- Around line 104-129: The polling currently delays before the first fetch which
can hide an immediate COMPLETED status; in startStatusPolling, call
onBoardingRepository.fetchOnboardingStatus() immediately once (invoking
onStatusFetched with the result and handling success/error and
consecutiveFailureCount), and then enter the while(isActive) loop that delays
POLLING_INTERVAL_MS before subsequent fetches; ensure you still set
onboardingStatusJob = null when onStatusFetched returns true or when
consecutiveFailureCount >= MAX_POLLING_FAILURE_COUNT, and preserve existing
error handling and job lifecycle behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 272e56f6-d422-429a-bb33-2e4ced3d4065
📒 Files selected for processing (4)
core/design-system/src/main/res/values/strings.xmlfeature/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/dday/DdayRoute.kt
✅ Files skipped from review due to trivial changes (1)
- core/design-system/src/main/res/values/strings.xml
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.
이슈 번호
작업내용
-온폴링을 통해 온보딩 상태가 기념일 설정 단계에서 완료 상태로 변경되면 즉시 홈으로 이동하도록 처리
결과물