Conversation
📝 WalkthroughWalkthrough이 PR은 새로운 온보딩 기능 모듈을 추가하고 기존 시스템과 통합합니다. 온보딩 피처는 커플 연결(CoupleConnection), 초대 코드(InviteCode), 프로필 설정(Profile), D-day 등록(Dday) 등 네 단계의 네비게이션 그래프로 구성됩니다. 도메인 모델(NickName, InviteCode)을 새로 추가하고, 저장소 계층을 AppResult로 감싸서 에러 처리를 강화합니다. 로그인 플로우도 온보딩 상태를 확인한 뒤 적절한 단계로 라우팅하도록 수정했으며, API 엔드포인트도 api/v1 버전으로 업데이트합니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 상세 리뷰 의견긍정적인 부분 ✨아키텍처 계층화가 명확합니다
네비게이션 구조가 체계적입니다
에러 처리 개선
개선 제안 사항 🔍1. 상태 관리 로직의 복잡도 검토 필요현황:
왜 문제인가:
개선 방안: // 상태 머신 패턴이나 Redux 같은 명확한 상태 전이를 고려해보세요
when (intent) {
is OnBoardingIntent.SubmitNickName -> {
// 단일 책임: nickname 제출만 처리
// fetchOnboardingStatus는 side effect로 분리
}
}질문: 2. 예외 처리 일관성현황:
개선 방안: // 현재 코드
InviteCode.create(value).fold(
onSuccess = { copy(partnerInviteCode = value, isValid = true) },
onFailure = { copy(partnerInviteCode = value, isValid = false) }
)
// 고려사항: 왜 실패한 이유(length vs regex)를 구분하지 않나요?
// UI에서 다양한 에러 메시지를 표시할 수 있도록 하면 UX가 개선될 것 같습니다.질문: 초대 코드가 유효하지 않을 때 사용자에게 어떤 피드백을 주나요? "올바른 초대 코드를 입력하세요" 같은 일반적인 메시지인가요? 3. 네비게이션 백스택 관리현황:
왜 문제인가:
개선 방안: // ProfileRoute에도 navigateToBack을 추가하는 것은 어떨까요?
data class ProfileRoute(
val viewModel: OnBoardingViewModel,
val navigateToDday: () -> Unit,
val navigateToHome: () -> Unit,
val navigateToBack: () -> Unit, // 추가 제안
)4. OnBoardingViewModel의 유닛 테스트 커버리지현황:
제안:
5. API 엔드포인트 버전 업데이트현황: // Before: `@POST`("onboarding/anniversary")
// After: `@POST`("api/v1/onboarding/anniversary")좋은 점: 버전 관리가 명확해집니다. 확인 사항:
6. 문자열 리소스의 순서현황:
제안: <!-- 섹션별 주석으로 그룹화하면 유지보수가 쉬워집니다 -->
<!-- Onboarding - Profile Setup -->
<string name="onboarding_profile_title">...</string>
<!-- Onboarding - Couple Connection -->
<string name="onboarding_couple_connect">...</string>보안 & 성능 체크리스트
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | 프로필 설정 화면 구현이 주요 변경이며, 온보딩 네비게이션 설정도 포함되어 PR 제목과 일치합니다. |
| Description check | ✅ Passed | PR 설명에서 프로필 설정 화면 구현과 온보딩 네비게이션 설정을 명시하고 있어 변경 내용과 관련이 있습니다. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing touches
- 📝 Generate docstrings
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feat/#35-profile-setting
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt`:
- Around line 3-24: Replace hardcoded Korean strings in CoupleConnectRoute with
string resources: use stringResource(R.string.onboarding_couple_connect_title)
for the screen title (fix the typo to "커플 등록 화면") and
stringResource(R.string.onboarding_next) for the button label; add the import
androidx.compose.ui.res.stringResource and reference the resource IDs
onboarding_couple_connect_title and onboarding_next. Also add a simple Preview
composable (e.g., CoupleConnectRoutePreview) that calls CoupleConnectRoute {} so
the UI can be previewed during development.
In `@feature/onboarding/src/main/java/com/twix/onboarding/dday/DdayRouete.kt`:
- Around line 12-14: Rename the misspelled composable and file: change the
function name DdayRouete to DdayRoute and rename the file DdayRouete.kt to
DdayRoute.kt; update all references/usages and imports where DdayRouete is
referenced (including any navigation registration or tests) so they point to
DdayRoute, and run a quick compile to ensure no remaining symbol references
remain.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/profile/ProfileScreen.kt`:
- Around line 151-160: The AppButton currently looks disabled visually but
remains clickable; update its behavior so it is actually disabled when
uiModel.isValid is false by wiring the enabled state (or passing null/NO-OP for
onClick) from uiModel.isValid into the button control; locate the AppButton
usage in ProfileScreen.kt and either set an enabled parameter (or conditionally
supply onClick = if (uiModel.isValid) { onCompleted } else { null }) so visual
and interactive states match, ensuring any ripple/press behavior is suppressed
when disabled.
🧹 Nitpick comments (12)
core/design-system/src/main/res/drawable/ic_arrow1_m_left.xml (1)
11-11: Dark Mode 지원을 위해 테마 색상 사용을 고려해 주세요.현재
#171717로 하드코딩된 색상은 다크 모드에서 어두운 배경과 대비가 부족하여 아이콘이 보이지 않을 수 있습니다.개선 방안:
android:strokeColor="?attr/colorOnSurface"와 같은 테마 속성을 사용하거나@color/icon_primary같은 색상 리소스를 정의하여values-night폴더에서 다크 모드 대응 색상을 지정할 수 있습니다.- android:strokeColor="#171717" + android:strokeColor="@color/icon_primary"다크 모드 지원이 현재 범위에 포함되지 않는다면, 추후 대응을 위해 이슈로 트래킹하는 것도 좋은 방법입니다.
core/design-system/src/main/res/drawable/ic_check_success.xml (1)
8-8: 디자인 시스템 일관성을 위해 색상 리소스 사용을 권장합니다.성공 색상
#1ED45A가 하드코딩되어 있습니다. 이 녹색은 밝은/어두운 테마 모두에서 비교적 잘 보일 수 있지만, 디자인 시스템의 일관성과 향후 유지보수를 위해 색상 리소스로 분리하는 것이 좋습니다.이점:
- 디자인 시스템에서 success 색상을 한 곳에서 관리할 수 있습니다
- 브랜드 색상 변경 시 일괄 수정이 가능합니다
- 접근성 요구사항에 따른 대비 조정이 용이합니다
♻️ 개선 제안
colors.xml에 색상 정의:<color name="success_green">#1ED45A</color>drawable에서 참조:
- android:fillColor="#1ED45A"/> + android:fillColor="@color/success_green"/>feature/onboarding/src/main/java/com/twix/onboarding/dday/DdayRouete.kt (1)
12-36: Preview Composable 추가를 고려해주세요.코딩 가이드라인에 따르면 Compose UI에는 Preview Composable이 제공되어야 합니다. 현재는 placeholder 화면이지만, 향후 실제 구현 시 UI 검증을 위해 Preview를 추가하면 개발 편의성이 향상됩니다.
♻️ Preview 추가 예시
`@Preview`(showBackground = true) `@Composable` private fun DdayRoutePreview() { TwixTheme { DdayRoute( onComplete = {}, onBack = {}, ) } }feature/onboarding/src/main/java/com/twix/onboarding/profile/component/OnBoardingTopbar.kt (2)
28-37: 접근성 개선:contentDescription을 리소스 문자열로 변경해주세요.스크린 리더 사용자를 위해
contentDescription이 하드코딩된 영문 "back" 대신 다국어 지원이 가능한 리소스 문자열을 사용하는 것이 좋습니다. 디자인 시스템 리뷰 가이드라인에서 접근성(Accessibility) 고려를 권장하고 있습니다.♻️ 수정 제안
strings.xml에 추가:<string name="content_description_back">뒤로 가기</string>코드 수정:
Icon( painter = painterResource(id = DesR.drawable.ic_arrow1_m_left), - contentDescription = "back", + contentDescription = stringResource(id = R.string.content_description_back), modifier = Modifier .size(44.dp) .padding(start = 10.dp) .noRippleClickable { onBack() } .align(Alignment.CenterStart), )
17-39: Preview Composable 추가를 권장합니다.코딩 가이드라인에 따르면 Compose UI에는 Preview가 제공되어야 합니다. TopBar 컴포넌트는 여러 화면에서 재사용될 수 있으므로 Preview를 통해 독립적으로 UI를 검증하면 좋겠습니다.
♻️ Preview 추가 예시
`@Preview`(showBackground = true) `@Composable` private fun OnBoardingTopbarPreview() { TwixTheme { OnBoardingTopbar( onBack = {}, ) } }core/design-system/src/main/java/com/twix/designsystem/components/button/AppButton.kt (2)
19-49: 비활성화 상태(enabled = false)에 대한 시각적 피드백이 필요합니다.현재 구현에서는
enabled = false일 때도 동일한backgroundColor가 사용됩니다. 사용자가 버튼이 비활성화되었음을 인지할 수 있도록 시각적 차이를 제공하는 것이 좋습니다.♻️ 비활성화 상태 처리 예시
`@Composable` fun AppButton( modifier: Modifier = Modifier, text: String, textColor: Color = CommonColor.White, backgroundColor: Color = GrayColor.C500, + disabledBackgroundColor: Color = GrayColor.C200, + disabledTextColor: Color = GrayColor.C300, enabled: Boolean = true, cornerRadius: Dp = 12.dp, border: BorderStroke? = null, onClick: () -> Unit = {}, ) { + val actualBackgroundColor = if (enabled) backgroundColor else disabledBackgroundColor + val actualTextColor = if (enabled) textColor else disabledTextColor + Surface( - color = backgroundColor, + color = actualBackgroundColor, shape = RoundedCornerShape(cornerRadius), border = border, onClick = onClick, enabled = enabled, modifier = modifier, ) { AppText( text = text, - color = textColor, + color = actualTextColor, style = AppTextStyle.T2, textAlign = TextAlign.Center, modifier = Modifier .fillMaxWidth() .padding(vertical = 14.dp), ) } }
19-49: Preview Composable과 Dark Mode 지원을 고려해주세요.디자인 시스템 가이드라인에서 Dark Mode 지원과 Preview 제공을 권장하고 있습니다. AppButton은 여러 곳에서 재사용될 공통 컴포넌트이므로 다양한 상태의 Preview를 제공하면 디자인 검증에 도움이 됩니다.
♻️ Preview 추가 예시
`@Preview`(showBackground = true, name = "Enabled") `@Preview`(showBackground = true, name = "Disabled") `@Composable` private fun AppButtonPreview() { TwixTheme { Column { AppButton( text = "다음", enabled = true, onClick = {}, ) AppButton( text = "다음", enabled = false, onClick = {}, ) } } }feature/onboarding/src/main/res/values/strings.xml (1)
3-9: 중복 문자열 재사용과 다국어 지원 여부를 확인해주세요.동일 문구가
onboarding_name_*와onboarding_profile_*로 중복되어 있어 추후 문구 변경 시 불일치가 생길 수 있습니다. 공통 키로 통합하거나 한쪽이 다른 키를 참조하도록 정리하는 방법을 검토해 주세요. 다국어 지원 계획이 있다면 values-en 등 번역 리소스도 추가할지 확인해 볼까요?
As per coding guidelines, "다국어 지원을 고려했는가?".feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt (1)
3-40: 사이드이펙트를 새 코루틴에 분리하면 순서 보장이 약해질 수 있어요.
handleIntent가 이미 suspend인데viewModelScope.launch로 분리하면 이후 Intent가 먼저 처리되어 네비게이션/토스트 순서가 뒤바뀔 여지가 있습니다.handleSubmitNickname을 suspend로 변경하고emitSideEffect를 직접 호출해 순차 흐름을 유지하는 방향은 어떨까요?🔧 제안 수정안
- private fun handleSubmitNickname() { + private suspend fun handleSubmitNickname() { val sideEffect = if (uiState.value.isValidNickName) { OnBoardingSideEffect.ProfileSetting.NavigateToNext } else { OnBoardingSideEffect.ProfileSetting.ShowInvalidNickNameToast } - viewModelScope.launch { - emitSideEffect(sideEffect) - } + emitSideEffect(sideEffect) }As per coding guidelines, "단방향 데이터 플로우(Intent → ViewModel → State → View)가 유지되는가?".
feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt (1)
12-27: 간단한 Preview를 추가하면 화면 확인이 더 쉬워집니다.현재는 앱 실행 없이 레이아웃을 확인하기 어려워요. 기본 상태용
@Preview를하나 두면 빠르게 확인할 수 있습니다. 필요하시면 추가해볼까요?🔧 제안 수정안
+import androidx.compose.ui.tooling.preview.Preview + +@Preview(showBackground = true) +@Composable +private fun CoupleConnectRoutePreview() { + CoupleConnectRoute(onNext = {}) +}As per coding guidelines, "Preview Composable이 제공되는가?".
domain/src/main/java/com/twix/domain/model/nickname/NickNameException.kt (1)
3-4: Throwable 대신 Exception 상속 의도가 있는지 확인 부탁드립니다.
Throwable은 Error까지 포함하는 최상위 타입이라 catch 범위가 넓어질 수 있습니다. 특별한 목적이 없다면Exception(또는RuntimeException) 상속으로 좁히는 편이 일반적입니다—에러까지 잡아야 하는 시나리오가 있을까요?🔧 제안 수정안
-sealed class NickNameException : Throwable() { +sealed class NickNameException : Exception() { data object InvalidLengthException : NickNameException() }feature/onboarding/src/main/java/com/twix/onboarding/navigation/OnboardingNavGraph.kt (1)
10-11:DdayRouete명칭은 의도된 건가요?
오탈자라면DdayRoute로 정리하면 검색성과 유지보수성이 좋아집니다. 리네이밍 의도가 있는지 확인해도 될까요?Also applies to: 38-40
feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt
Outdated
Show resolved
Hide resolved
feature/onboarding/src/main/java/com/twix/onboarding/dday/DdayRoute.kt
Outdated
Show resolved
Hide resolved
feature/onboarding/src/main/java/com/twix/onboarding/profile/ProfileScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/onboarding/src/main/java/com/twix/onboarding/navigation/OnboardingNavGraph.kt`:
- Line 10: Rename the misspelled route symbols from DdayRouete to DdayRoute:
rename the file DdayRouete.kt to DdayRoute.kt, update the composable function
declaration fun DdayRouete(...) to fun DdayRoute(...), change the import
com.twix.onboarding.dday.DdayRouete to com.twix.onboarding.dday.DdayRoute, and
update any call sites that reference DdayRouete (the composable
registration/usage that should match NavRoutes.DdayRoute) so names are
consistent.
feature/onboarding/src/main/java/com/twix/onboarding/navigation/OnboardingNavGraph.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # app/src/main/java/com/yapp/twix/main/MainActivity.kt
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
data/src/main/java/com/twix/data/repository/DefaultOnboardingRepository.kt (1)
14-17:⚠️ Potential issue | 🟡 Minor서버에서 받은 초대 코드도 도메인 불변식을 통해 생성하기
검증 결과,
InviteCode클래스는create()팩토리 메서드를 통해 8자리 길이 검증을 수행합니다. 그런데 현재 코드에서는 생성자를 직접 호출하여 이 검증을 우회하고 있습니다:// InviteCode.kt 의 create() 메서드 fun create(value: String): Result<InviteCode> = runCatching { if (value.length != INVITE_CODE_LENGTH) { throw InviteCodeException.InvalidLengthException } InviteCode(value) } // 현재 repository 구현 return InviteCode(response.inviteCode) // ← 검증 우회왜 중요한가:
- 도메인 모델의 불변식(invariant)을 항상 보장해야 합니다
- 사용자 입력값은
create()로 검증하는데, 서버 응답값만 생성자로 직접 생성하는 것은 일관성이 떨어집니다- 테스트에서도 검증 로직을 확인할 때는
create()를 사용합니다개선 방안:
override suspend fun fetchInviteCode(): InviteCode { val response = service.fetchInviteCode() return InviteCode.create(response.inviteCode).getOrThrow() // 또는 Result를 처리하여 에러 매핑 }서버 데이터는 신뢰할 수 있지만, 도메인 모델의 일관성을 위해 항상 팩토리 메서드를 통한 생성을 권장합니다.
domain/src/test/java/com/twix/domain/model/InviteCodeTest.kt (1)
10-15: 🛠️ Refactor suggestion | 🟠 Major유효한 코드 테스트에서
create()팩토리 메서드를 사용하는 것이 좋겠습니다.현재
InviteCode("AB12CD34")로 생성자를 직접 호출하고 있는데,InviteCode는value class이므로 생성자가 public이고 검증 로직을 거치지 않습니다. Happy path 역시InviteCode.create()를 통해 테스트해야 팩토리 메서드의 정상 동작을 검증할 수 있습니다.또한 코딩 가이드라인에 따라 Given-When-Then 구조를 일관되게 적용하면 가독성이 높아집니다.
🔧 제안하는 수정
`@Test` fun `유효한 초대 코드는 정상적으로 생성된다`() { - val inviteCode = InviteCode("AB12CD34") + // given + val code = "AB12CD34" - assertThat(inviteCode.value).isEqualTo("AB12CD34") + // when + val result = InviteCode.create(code) + + // then + assertThat(result.isSuccess).isTrue() + assertThat(result.getOrNull()?.value).isEqualTo(code) }As per coding guidelines, "[테스트 코드 리뷰 가이드] - Given-When-Then 구조를 따르는가? - Happy / Sad Path가 모두 커버되는가?"
🤖 Fix all issues with AI agents
In `@domain/src/main/java/com/twix/domain/model/nickname/NickName.kt`:
- Around line 10-11: 현재 닉네임 검증은 길이만 확인해 공백 문자만 있는 값이나 앞뒤 공백을 포함한 값이 통과합니다;
NickName 검증(값을 받는 생성자나 setValue 메서드에서, 참조되는 변수명 value)을 변경해 value.isBlank() 또는
value.trim().length 체크를 추가하여 공백 전용 입력을 거부하고, 공백으로 인한 실패 시 적절한 예외(예:
NickNameException.InvalidBlankException 또는 기존의
NickNameException.InvalidLengthException와 구분되게 메시지 포함)를 던지도록 하세요; 또한 trim()을 사용해
내부 저장 전에 앞뒤 공백을 제거하는 방안도 고려하세요.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/couple/component/ConnectButton.kt`:
- Around line 69-76: ConnectButton.kt에서 현재 직접 사용되고 있는 Text(문자열
"onboarding_couple_connect_direct")는 다른 텍스트들과 달리 AppText를 쓰지 않아 시스템 폰트 스케일과 다르게
동작하므로 동일하게 AppText로 교체하세요; 만약 AppText가 fontWeight 파라미터를 지원하지 않으면 AppText에
fontWeight: FontWeight? (또는 필요한 스타일 선택자)를 추가하거나 AppTextStyle에 ExtraBold 스타일을 정의한
뒤 ConnectButton의 해당 텍스트를 AppText(text = stringResource(...), fontWeight =
ExtraBold, color = GrayColor.C500, fontSize = 16.sp, lineHeight = 22.2.sp) 형태로
통일해 레이아웃/폰트 스케일 동작을 일관되게 유지하도록 수정하세요.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/couple/component/RestoreCoupleBottomSheetContent.kt`:
- Around line 53-64: Replace the fixed Modifier.height(78.dp) on the Column that
wraps the three BulletItem calls with a minimum-height approach so content can
expand for longer translations or larger font sizes; for example, change the
Modifier chain used in the Column (the one with .fillMaxWidth() and
.height(78.dp)) to use a min height (e.g., .heightIn(min = 78.dp) or
.defaultMinSize(minHeight = 78.dp)) and add appropriate vertical padding inside
the Column so items don’t get clipped when they expand; update the Column
modifier where BulletItem(stringResource(...)) is used and keep the existing
verticalArrangement and RoundedCornerShape.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt`:
- Around line 108-115: The bottom sheet is being conditionally composed with an
if (showRestoreSheet) which prevents CommonBottomSheet from observing visible
changes and running its exit animation; remove the surrounding if and always
compose CommonBottomSheet, passing showRestoreSheet into its visible parameter
(keep onDismissRequest = onDismissSheet and content = {
RestoreCoupleBottomSheetContent() }) so CommonBottomSheet can animate its
internalVisible → delay(200) → rendering = false exit sequence correctly.
- Around line 39-53: The CoupleConnectRoute currently injects an
OnBoardingViewModel via the viewModel parameter which is unused and causes
OnBoardingViewModel (and its fetchMyInviteCode()) to be created prematurely;
remove the unused parameter from the CoupleConnectRoute signature (the
viewModel: OnBoardingViewModel = koinViewModel() parameter) and any references
to it so the composable no longer instantiates the ViewModel on composition;
if/when you need the ViewModel later, obtain it lazily inside the relevant click
handlers (e.g., onClickSend) or restore the parameter then.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/invite/InviteCodeScreen.kt`:
- Around line 166-179: The clipboard copy handler in InviteCodeScreen only sets
the clipboard and does not dispatch the MVI intent, so add an onCopyInviteCode
callback and invoke it when the copy icon is clicked (inside the
noRippleClickable coroutine after clipboard.setClipEntry) to dispatch
OnBoardingIntent.CopyInviteCode to the ViewModel (handled by OnBoardingViewModel
to emit ShowCopyInviteCodeSuccessToast); wire InviteCodeRoute to pass a lambda
that calls viewModel.dispatch(OnBoardingIntent.CopyInviteCode) so the
single-direction Intent → ViewModel → State → View flow is preserved.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/invite/InViteCodeUiModel.kt`:
- Line 7: Rename the class InViteCodeUiModel to InviteCodeUiModel and update the
source file name to InviteCodeUiModel.kt; search for usages of InViteCodeUiModel
(e.g., where InviteCodeScreen, InviteCodeTextField or other components reference
it) and update those references to InviteCodeUiModel to maintain consistency
with Kotlin PascalCase naming conventions. Ensure constructor, imports, and any
serializers/parsers or tests that reference the old name are updated accordingly
and run a build to catch remaining references.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/model/OnBoardingIntent.kt`:
- Around line 5-19: OnBoardingIntent에 D-Day 화면용 Intent가 빠져 있어 D-Day가 MVI 플로우에
통합되지 않았습니다; 파일의 sealed interface OnBoardingIntent에 D-Day 입력과 제출을 표현할 Intent(예:
data class UpdateDday(val date: LocalDate) 또는 data object SubmitDday)를 추가하고, 해당
Intent를 받는 ViewModel의 handleIntent(...) 구현에 분기 케이스를 추가해 상태 갱신/유효성 검사/내비게이션 처리를
이어주십시오 (또는 D-Day를 별도 ViewModel로 유지하려면 그 설계 의도를 주석으로 명시).
In
`@feature/onboarding/src/main/java/com/twix/onboarding/model/OnBoardingSideEffect.kt`:
- Around line 14-18: The sealed type InviteCode has its nested object
NavigateToNext incorrectly inheriting CoupleConnection, causing pattern-match
and routing bugs; change the declaration of NavigateToNext so it implements
InviteCode (i.e., declare NavigateToNext as : InviteCode instead of :
CoupleConnection) so that OnBoardingSideEffect.InviteCode pattern checks
correctly, and keep any semantic comment that this event represents "navigate to
couple connection" but preserve the side-effect owner as InviteCode.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/profile/ProfileScreen.kt`:
- Around line 59-75: The current LaunchedEffect(viewModel.sideEffect) + collect
is not lifecycle-safe; replace that block with the same pattern used in
InviteCodeScreen by using ObserveAsEvents/observeAsEvents to observe
viewModel.sideEffect lifecycle-aware (uses repeatOnLifecycle STARTED). Move the
when-handling of OnBoardingSideEffect.ProfileSetting (including
toastManager.tryShow(...) for ShowInvalidNickNameToast and onNext() for
NavigateToNext) into the ObserveAsEvents callback so events are consumed only
while the UI is STARTED; keep the existing side-effect cases and remove the
direct LaunchedEffect.collect usage.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt`:
- Around line 18-23: fetchMyInviteCode currently makes a network call without
error handling, which can crash the coroutine if
onBoardingRepository.fetchInviteCode() throws; wrap the repository call inside a
try-catch in fetchMyInviteCode (the viewModelScope.launch block), catch
exceptions from onBoardingRepository.fetchInviteCode(), log the error and call
reduce to emit an error/empty state (e.g., call reduce with
updateMyInviteCode(null) or an updateError state) so the ViewModel handles
failures gracefully instead of letting the coroutine crash.
- Around line 58-67: connectCouple currently calls
onBoardingRepository.coupleConnection inside viewModelScope.launch with no error
handling, so failures stop the coroutine and never emit feedback; wrap the
network call in a try/catch inside connectCouple's viewModelScope.launch, catch
exceptions from onBoardingRepository.coupleConnection, emit a failure side
effect (e.g., OnBoardingSideEffect.ShowError or a toast side effect) on error
and only emit OnBoardingSideEffect.InviteCode.NavigateToNext on success, and
optionally log the exception for diagnostics.
🧹 Nitpick comments (16)
domain/src/main/java/com/twix/domain/model/nickname/NickName.kt (2)
3-6:value class의 public 생성자로 인해create()검증을 우회할 수 있습니다.현재
NickName의 primary constructor가 public이므로,NickName.create()를 거치지 않고NickName("a")처럼 직접 생성하면 길이 검증이 우회됩니다.Kotlin의
value class는 언어 제약상 private constructor를 지원하지 않기 때문에, 만약 엄격한 불변식 보장이 필요하다면 일반data class+private constructor+ companion factory 패턴을 고려해 볼 수 있습니다. 현재 구조에서는 팀 내 컨벤션으로 "항상create()를 통해 생성한다"는 규칙을 명시하는 것도 방법입니다.이 부분은 트레이드오프(value class의 성능 이점 vs. 캡슐화)가 있으므로, 팀에서 어떤 방향이 더 적합한지 논의해 보시면 좋겠습니다.
8-14:runCatching대신 명시적Result반환을 권장합니다.
runCatching은 내부에서 발생하는 모든Throwable을 포착합니다. 현재 블록 안에서는 직접 throw하는 예외만 존재하지만, 코루틴 컨텍스트에서 호출될 경우CancellationException까지 삼켜버릴 수 있어 구조적 동시성(Structured Concurrency)을 깨뜨릴 위험이 있습니다.검증 로직이 단순하므로,
Result.success/Result.failure를 명시적으로 반환하면 의도가 더 명확해지고 예기치 않은 예외 포착을 방지할 수 있습니다.♻️ 개선 제안
companion object { fun create(value: String): Result<NickName> = - runCatching { - if (value.length !in MIN_LENGTH..MAX_LENGTH) { - throw NickNameException.InvalidLengthException - } - NickName(value) - } + if (value.length !in MIN_LENGTH..MAX_LENGTH) { + Result.failure(NickNameException.InvalidLengthException) + } else { + Result.success(NickName(value)) + } private const val MIN_LENGTH = 2 private const val MAX_LENGTH = 8 }core/design-system/src/main/java/com/twix/designsystem/keyboard/KeyBoardAsState.kt (2)
10-10: TODO가 남아 있습니다 — 추적 이슈 생성을 권장드립니다.키보드 상태 감지 유틸리티는 디자인 시스템보다는
core:ui모듈에 더 적합합니다. 개발자분도 이미 인지하고 계신 부분이지만, TODO가 잊히지 않도록 별도 이슈로 추적하시면 좋겠습니다.이 TODO를 추적할 이슈를 생성해 드릴까요?
23-33:rememberUpdatedState대신derivedStateOf사용을 고려해봐도 좋습니다.현재 구현은
WindowInsets.ime.getBottom(density)을 컴포지션 본문에서 직접 읽고 있어, 키보드 애니메이션 중 매 프레임마다 불필요한 리컴포지션이 발생합니다.rememberUpdatedState는 장수명 사이드이펙트(LaunchedEffect등) 내부에서 최신 값을 캡처하기 위한 API이므로, 여기서는 의미론적으로 적절하지 않습니다.
remember { derivedStateOf { ... } }를 사용하면:
- 내부적으로 스냅샷 상태를 추적하여 파생된 enum 값이 실제로 변경될 때만 하위 리더를 업데이트합니다
- 키보드 애니메이션 중 픽셀 단위 변화에도 불구하고
Opened↔Closed변화만 전파됩니다다만 한 가지 더 고려할 점: Compose 가이드라인상 WindowInsets는 컴포지션 단계에서 읽지 않는 것이 권장됩니다. 가능하다면
Modifier.imePadding()같은 WindowInsets 전용 Modifier를 활용하거나, 비-레이아웃 로직이라면snapshotFlow를 통해 컴포지션 외부에서 관찰하는 방식을 검토해볼 가치가 있습니다.현재 함수 설계상 위 패턴이 어렵다면,
derivedStateOf는 합리적인 개선입니다.♻️ derivedStateOf를 활용한 개선안
`@Composable` fun keyboardAsState(): State<Keyboard> { val density = LocalDensity.current - val keyboard = - if (WindowInsets.ime.getBottom(density) > 0) { - Keyboard.Opened - } else { - Keyboard.Closed - } - return rememberUpdatedState(keyboard) + return remember { + derivedStateOf { + if (WindowInsets.ime.getBottom(density) > 0) { + Keyboard.Opened + } else { + Keyboard.Closed + } + } + } }추가 import가 필요합니다:
import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.rememberfeature/onboarding/src/main/java/com/twix/onboarding/couple/component/InvitationButton.kt (1)
23-44: 접근성(Accessibility) 시맨틱 누락 가능성
noRippleClickable는 일반적으로clickable의 indication을 제거하거나pointerInput을 사용하는 커스텀 확장인데, 이 경우 Compose의 시맨틱 트리에Role.Button이 설정되지 않을 수 있습니다. 스크린 리더(TalkBack)가 이 요소를 버튼으로 인식하지 못할 수 있어요.
Modifier.semantics { role = Role.Button }또는contentDescription을 추가하면 접근성이 개선됩니다. 다만, 현재 앱의 접근성 지원 범위에 따라 우선순위를 판단해 주세요.feature/onboarding/src/main/java/com/twix/onboarding/couple/component/RestoreCoupleBottomSheetContent.kt (2)
29-30: 로컬 색상 상수 — 디자인 시스템과의 일관성 검토 필요
RestoreBgColor와BulletColor가 파일 내 로컬로 정의되어 있습니다. 디자인 시스템(GrayColor,CommonColor등)에 이미 유사한 색상이 있다면 해당 값을 재사용하는 것이 색상 일관성 유지에 도움됩니다. 만약 새로운 디자인 토큰이라면 디자인 시스템 모듈에 추가하는 것을 고려해 주세요. As per coding guidelines, "색상과 테마가 디자인 시스템에 포함되는가?"에 해당합니다.
91-97: Preview 함수가public으로 선언됨 —private으로 변경 권장다른 컴포넌트(
InvitationButtonPreview,ConnectButtonPreview)는private fun으로 선언되어 있는데, 여기만fun(public)입니다. Preview 함수는 외부에서 호출될 필요가 없으므로 일관성을 위해private으로 변경하는 것이 좋습니다.💡 수정 제안
`@Preview`(showBackground = true) `@Composable` -fun RestoreCoupleBottomSheetContentPreview() { +private fun RestoreCoupleBottomSheetContentPreview() { TwixTheme { RestoreCoupleBottomSheetContent() } }domain/src/test/java/com/twix/domain/model/InviteCodeTest.kt (1)
24-30: 실패 시 예외 타입 검증도 추가하면 더 견고한 테스트가 됩니다.현재
result.isFailure만 검증하고 있는데, 실패 원인이InviteCodeException.InvalidLengthException인지도 확인하면 예외 계층이 의도대로 동작하는지 보장할 수 있습니다.🔧 제안하는 수정
fun `8자리가 아닌 초대 코드는 예외가 발생한다`(invalidLengthCode: String) { // when val result = InviteCode.create(invalidLengthCode) // then assertThat(result.isFailure).isTrue() + assertThat(result.exceptionOrNull()) + .isInstanceOf(InviteCodeException.InvalidLengthException::class.java) }domain/src/main/java/com/twix/domain/model/invitecode/InviteCode.kt (1)
3-17:runCatching대신 명시적Result반환을 고려해보면 어떨까요?현재
runCatching은 내부에서 throw한 예외뿐만 아니라 예상치 못한 모든 예외(예:OutOfMemoryError)까지 catch합니다. 이 경우처럼 검증 로직이 단순하면Result.success/Result.failure를 명시적으로 반환하는 방식이 의도를 더 명확하게 표현합니다.🔧 제안하는 수정
companion object { fun create(value: String): Result<InviteCode> = - runCatching { - if (value.length != INVITE_CODE_LENGTH) { - throw InviteCodeException.InvalidLengthException - } - InviteCode(value) - } + if (value.length != INVITE_CODE_LENGTH) { + Result.failure(InviteCodeException.InvalidLengthException) + } else { + Result.success(InviteCode(value)) + } const val INVITE_CODE_LENGTH = 8 }이렇게 하면
throw+runCatching패턴의 불필요한 예외 생성/포착 비용도 피할 수 있고, 예외 처리 범위가 정확히 의도한 대로 한정됩니다.feature/onboarding/src/main/java/com/twix/onboarding/invite/component/InviteCodeTextField.kt (1)
57-105:rememberInfiniteTransition이 모든 CodeBox에서 실행되고 있어 불필요한 리소스가 소비됩니다.현재 8개의
CodeBox가 각각rememberInfiniteTransition+animateFloat를 실행하지만, 실제로 애니메이션이 보이는 것은isFocused인 박스 하나뿐입니다. 나머지 7개는 애니메이션 값을 계산만 하고 사용하지 않으므로, 불필요한 recomposition과 리소스 소비가 발생합니다.애니메이션을
isFocused일 때만 실행하도록 조건부로 변경하면 개선됩니다.🔧 제안하는 수정
`@Composable` private fun CodeBox( index: Int, code: String, ) { val isFocused = code.length == index - val infiniteTransition = rememberInfiniteTransition() - val alpha by infiniteTransition.animateFloat( - 0f, - 1f, - infiniteRepeatable(tween(500), RepeatMode.Reverse), - ) Box( modifier = Modifier .width(36.dp) .height(58.dp) .border( 1.dp, when { isFocused -> GrayColor.C500 else -> GrayColor.C200 }, RoundedCornerShape(8.dp), ), contentAlignment = Alignment.Center, ) { when { index < code.length -> { AppText( text = code[index].toString(), style = AppTextStyle.H3, color = GrayColor.C500, ) } isFocused -> { + val infiniteTransition = rememberInfiniteTransition() + val alpha by infiniteTransition.animateFloat( + 0f, + 1f, + infiniteRepeatable(tween(500), RepeatMode.Reverse), + ) Box( modifier = Modifier .width(2.dp) .height(24.dp) .background(GrayColor.C500.copy(alpha = alpha)), ) } } } }이렇게 하면 포커스된 박스에서만 애니메이션이 생성/실행되어 불필요한 리컴포지션을 방지할 수 있습니다.
As per coding guidelines, "[Feature 모듈 - MVI 패턴 리뷰 가이드] - 불필요한 recomposition 가능성은 없는가?"
feature/onboarding/src/main/java/com/twix/onboarding/invite/InviteCodeScreen.kt (1)
96-109:InViteCodeUiModel네이밍이 다소 일관성이 없어 보입니다 — 의도된 것인가요?
InViteCodeUiModel에서V가 대문자인데, 일반적인 네이밍 컨벤션으로는InviteCodeUiModel이 자연스럽습니다. 이 타입이 다른 파일에서 정의되어 있으므로 이 PR의 범위를 벗어날 수 있지만, 가능하다면 통일하면 좋겠습니다.feature/onboarding/src/main/java/com/twix/onboarding/profile/ProfileScreen.kt (2)
49-53:ProfileRoute의 접근 제한자가InviteCodeRoute와 일관되지 않습니다.
InviteCodeRoute는internal fun으로 선언되어 모듈 외부 접근을 제한하고 있지만,ProfileRoute는public fun입니다. Route composable은 navigation graph 내부에서만 호출되므로internalvisibility가 적절해 보입니다. 이 부분은 다른 Route들(CoupleConnectRoute,DdayRoute)도 함께 통일하면 좋겠습니다.
171-187: Preview에서 불필요한Row래핑이 있습니다.
UnValidProfileScreenPreview에서ProfileScreen을Row로 감싸고 있는데, 특별한 이유가 없다면 불필요합니다.ValidProfileScreenPreview처럼TwixTheme안에 직접 배치하는 것이 자연스럽습니다.feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt (1)
55-62:CoupleConnectScreen의 visibility를private으로 변경하는 것을 고려해 보세요.
CoupleConnectRoute가 외부 진입점 역할을 하고 있으므로,CoupleConnectScreen은private으로 제한하는 것이 캡슐화에 좋습니다. Preview에서 사용할 수 있도록 같은 파일 내에서private이면 충분합니다.feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt (1)
41-52:handleSubmitNickname에서viewModelScope.launch가 불필요할 수 있습니다.
handleIntent가 이미suspend fun이므로,emitSideEffect가 suspend function이라면 직접 호출할 수 있습니다.viewModelScope.launch로 별도 코루틴을 시작하면 호출 순서가 보장되지 않을 수 있습니다.♻️ 수정 제안
- private fun handleSubmitNickname() { + private suspend fun handleSubmitNickname() { val sideEffect = if (currentState.isValidNickName) { OnBoardingSideEffect.ProfileSetting.NavigateToNext } else { OnBoardingSideEffect.ProfileSetting.ShowInvalidNickNameToast } - viewModelScope.launch { - emitSideEffect(sideEffect) - } + emitSideEffect(sideEffect) }feature/onboarding/src/main/java/com/twix/onboarding/navigation/OnboardingNavGraph.kt (1)
23-64: 온보딩 화면 간 ViewModel 인스턴스가 분리되어 중복 API 호출이 발생하고 있습니다.각
composable목적지에서koinViewModel()을 호출하면, 기본적으로 각NavBackStackEntry의ViewModelStoreOwner에 ViewModel이 스코프됩니다. 현재 네비게이션 구조에서:
- CoupleConnectRoute: ViewModel 인스턴스 생성 →
init{}에서fetchMyInviteCode()호출- InviteRoute로 이동 (
popUpTo inclusive적용): CoupleConnectRoute 제거, 새로운 ViewModel 인스턴스 생성 →init{}에서fetchMyInviteCode()다시 호출- ProfileRoute로 이동: 또 다른 새로운 ViewModel 인스턴스 생성
결과적으로 같은 데이터(
inviteCode)를 여러 번 fetch하게 되어 불필요한 네트워크 요청이 발생합니다.
OnBoardingUiState가profile과inviteCode를 하나의 상태로 정의한 것은 화면 간 데이터 공유를 의도한 것으로 보이는데, 현재 ViewModel 스코핑 방식으로는 각 화면이 독립적으로 동작하고 있습니다.개선 방안:
- 네비게이션 그래프 단위 ViewModel 스코핑 사용:
koinNavViewModel()또는 Koin의scopeAPI를 활용하여 온보딩 그래프 전체에서 하나의 ViewModel 인스턴스를 공유하도록 변경- 또는 각 화면이 필요한 부분만 처리하도록 상태를 분리하고, 필요시 화면 간 데이터 전달 메커니즘 구현
이렇게 하면 불필요한 API 호출을 줄이고, 설계 의도와 구현을 일치시킬 수 있습니다.
| if (value.length !in MIN_LENGTH..MAX_LENGTH) { | ||
| throw NickNameException.InvalidLengthException |
There was a problem hiding this comment.
공백 문자에 대한 검증이 누락되어 있습니다.
현재 길이 검증만 수행하므로, " "(공백 2자)이나 " ab "(앞뒤 공백 포함)과 같은 입력도 유효한 닉네임으로 통과합니다. 의도된 동작인지 확인이 필요합니다.
일반적으로 닉네임 검증에서는 trim() 후 길이를 체크하거나, isBlank() 검사를 추가하는 것이 안전합니다.
🛡️ 개선 예시
fun create(value: String): Result<NickName> =
- if (value.length !in MIN_LENGTH..MAX_LENGTH) {
- Result.failure(NickNameException.InvalidLengthException)
- } else {
- Result.success(NickName(value))
- }
+ value.trim().let { trimmed ->
+ if (trimmed.length !in MIN_LENGTH..MAX_LENGTH) {
+ Result.failure(NickNameException.InvalidLengthException)
+ } else {
+ Result.success(NickName(trimmed))
+ }
+ }🤖 Prompt for AI Agents
In `@domain/src/main/java/com/twix/domain/model/nickname/NickName.kt` around lines
10 - 11, 현재 닉네임 검증은 길이만 확인해 공백 문자만 있는 값이나 앞뒤 공백을 포함한 값이 통과합니다; NickName 검증(값을 받는
생성자나 setValue 메서드에서, 참조되는 변수명 value)을 변경해 value.isBlank() 또는 value.trim().length
체크를 추가하여 공백 전용 입력을 거부하고, 공백으로 인한 실패 시 적절한 예외(예:
NickNameException.InvalidBlankException 또는 기존의
NickNameException.InvalidLengthException와 구분되게 메시지 포함)를 던지도록 하세요; 또한 trim()을 사용해
내부 저장 전에 앞뒤 공백을 제거하는 방안도 고려하세요.
| Text( | ||
| text = stringResource(R.string.onboarding_couple_connect_direct), | ||
| fontFamily = NanumSquareNeoFamily, | ||
| fontWeight = ExtraBold, | ||
| color = GrayColor.C500, | ||
| fontSize = 16.sp, | ||
| lineHeight = 22.2.sp, | ||
| ) |
There was a problem hiding this comment.
AppText 대신 Text를 직접 사용 — 폰트 스케일 동작 불일치
이 컴포넌트 내 다른 텍스트는 모두 AppText를 사용하고 있는데, Line 69의 "직접 연결하기" 텍스트만 Text를 직접 사용하고 있습니다.
AppText는 내부적으로 fontScale = 1f을 강제하여 시스템 글꼴 크기 설정에 영향을 받지 않지만, 원시 Text는 시스템 설정에 따라 크기가 변합니다. 같은 버튼 안에서 일부 텍스트만 시스템 폰트 크기에 반응하면 레이아웃이 깨질 수 있습니다.
ExtraBold 같은 커스텀 weight가 필요하다면, AppText에 fontWeight 파라미터를 추가하거나, AppTextStyle에 해당 스타일을 정의하는 방향을 검토해 보시면 어떨까요?
💡 AppText 사용으로 통일하는 방안
AppText가 fontWeight 파라미터를 지원하도록 확장하는 것이 이상적이지만, 우선 현재 구조에서 빠르게 대응하려면:
- Text(
- text = stringResource(R.string.onboarding_couple_connect_direct),
- fontFamily = NanumSquareNeoFamily,
- fontWeight = ExtraBold,
- color = GrayColor.C500,
- fontSize = 16.sp,
- lineHeight = 22.2.sp,
- )
+ // TODO: AppText에 fontWeight 파라미터 지원 추가 후 통일
+ CompositionLocalProvider(
+ LocalDensity provides Density(LocalDensity.current.density, fontScale = 1f)
+ ) {
+ Text(
+ text = stringResource(R.string.onboarding_couple_connect_direct),
+ fontFamily = NanumSquareNeoFamily,
+ fontWeight = ExtraBold,
+ color = GrayColor.C500,
+ fontSize = 16.sp,
+ lineHeight = 22.2.sp,
+ )
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Text( | |
| text = stringResource(R.string.onboarding_couple_connect_direct), | |
| fontFamily = NanumSquareNeoFamily, | |
| fontWeight = ExtraBold, | |
| color = GrayColor.C500, | |
| fontSize = 16.sp, | |
| lineHeight = 22.2.sp, | |
| ) | |
| // TODO: AppText에 fontWeight 파라미터 지원 추가 후 통일 | |
| CompositionLocalProvider( | |
| LocalDensity provides Density(LocalDensity.current.density, fontScale = 1f) | |
| ) { | |
| Text( | |
| text = stringResource(R.string.onboarding_couple_connect_direct), | |
| fontFamily = NanumSquareNeoFamily, | |
| fontWeight = ExtraBold, | |
| color = GrayColor.C500, | |
| fontSize = 16.sp, | |
| lineHeight = 22.2.sp, | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
`@feature/onboarding/src/main/java/com/twix/onboarding/couple/component/ConnectButton.kt`
around lines 69 - 76, ConnectButton.kt에서 현재 직접 사용되고 있는 Text(문자열
"onboarding_couple_connect_direct")는 다른 텍스트들과 달리 AppText를 쓰지 않아 시스템 폰트 스케일과 다르게
동작하므로 동일하게 AppText로 교체하세요; 만약 AppText가 fontWeight 파라미터를 지원하지 않으면 AppText에
fontWeight: FontWeight? (또는 필요한 스타일 선택자)를 추가하거나 AppTextStyle에 ExtraBold 스타일을 정의한
뒤 ConnectButton의 해당 텍스트를 AppText(text = stringResource(...), fontWeight =
ExtraBold, color = GrayColor.C500, fontSize = 16.sp, lineHeight = 22.2.sp) 형태로
통일해 레이아웃/폰트 스케일 동작을 일관되게 유지하도록 수정하세요.
| Column( | ||
| modifier = | ||
| Modifier | ||
| .fillMaxWidth() | ||
| .height(78.dp) | ||
| .background(RestoreBgColor, RoundedCornerShape(12.dp)), | ||
| verticalArrangement = Arrangement.Center, | ||
| ) { | ||
| BulletItem(stringResource(R.string.onboarding_couple_restore_content_my_email)) | ||
| BulletItem(stringResource(R.string.onboarding_couple_restore_content_partner_email)) | ||
| BulletItem(stringResource(R.string.onboarding_couple_restore_content_restore_date)) | ||
| } |
There was a problem hiding this comment.
고정 높이(78.dp) 사용 — 콘텐츠 잘림 위험
Bullet 아이템 3개를 담는 컨테이너에 height(78.dp)가 고정되어 있습니다. 현재 한국어 텍스트에서는 괜찮을 수 있지만, 다국어 지원 시 텍스트가 길어지거나, 접근성 설정으로 폰트 크기가 커지면 콘텐츠가 잘릴 수 있습니다.
height(78.dp) 대신 최소 높이를 설정하고 콘텐츠에 맞게 늘어나도록 하는 것이 안전합니다. As per coding guidelines, "다국어 지원을 고려했는가?"에 해당합니다.
💡 최소 높이 + padding 방식으로 변경
Column(
modifier =
Modifier
.fillMaxWidth()
- .height(78.dp)
- .background(RestoreBgColor, RoundedCornerShape(12.dp)),
+ .background(RestoreBgColor, RoundedCornerShape(12.dp))
+ .padding(vertical = 12.dp),
verticalArrangement = Arrangement.Center,
) {🤖 Prompt for AI Agents
In
`@feature/onboarding/src/main/java/com/twix/onboarding/couple/component/RestoreCoupleBottomSheetContent.kt`
around lines 53 - 64, Replace the fixed Modifier.height(78.dp) on the Column
that wraps the three BulletItem calls with a minimum-height approach so content
can expand for longer translations or larger font sizes; for example, change the
Modifier chain used in the Column (the one with .fillMaxWidth() and
.height(78.dp)) to use a min height (e.g., .heightIn(min = 78.dp) or
.defaultMinSize(minHeight = 78.dp)) and add appropriate vertical padding inside
the Column so items don’t get clipped when they expand; update the Column
modifier where BulletItem(stringResource(...)) is used and keep the existing
verticalArrangement and RoundedCornerShape.
| @Composable | ||
| fun CoupleConnectRoute( | ||
| onNext: () -> Unit, | ||
| viewModel: OnBoardingViewModel = koinViewModel(), | ||
| ) { | ||
| var showRestoreSheet by rememberSaveable { mutableStateOf(false) } | ||
|
|
||
| CoupleConnectScreen( | ||
| showRestoreSheet = showRestoreSheet, | ||
| onClickSend = { }, | ||
| onClickConnect = onNext, | ||
| onClickRestore = { showRestoreSheet = true }, | ||
| onDismissSheet = { showRestoreSheet = false }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
viewModel 파라미터가 주입되었지만 사용되지 않습니다.
OnBoardingViewModel이 생성자에서 fetchMyInviteCode()를 호출하므로, 이 화면에서 불필요하게 ViewModel을 생성하면 아직 필요하지 않은 API 호출이 발생합니다. 현재 onClickSend도 빈 핸들러이므로, 실제 기능이 구현될 때 추가하는 것이 좋겠습니다.
🛠️ 수정 제안: 미사용 viewModel 제거
`@Composable`
fun CoupleConnectRoute(
onNext: () -> Unit,
- viewModel: OnBoardingViewModel = koinViewModel(),
) {🤖 Prompt for AI Agents
In
`@feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt`
around lines 39 - 53, The CoupleConnectRoute currently injects an
OnBoardingViewModel via the viewModel parameter which is unused and causes
OnBoardingViewModel (and its fetchMyInviteCode()) to be created prematurely;
remove the unused parameter from the CoupleConnectRoute signature (the
viewModel: OnBoardingViewModel = koinViewModel() parameter) and any references
to it so the composable no longer instantiates the ViewModel on composition;
if/when you need the ViewModel later, obtain it lazily inside the relevant click
handlers (e.g., onClickSend) or restore the parameter then.
| if (showRestoreSheet) { | ||
| CommonBottomSheet( | ||
| visible = true, | ||
| config = CommonBottomSheetConfig(showHandle = false), | ||
| onDismissRequest = onDismissSheet, | ||
| content = { RestoreCoupleBottomSheetContent() }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
CommonBottomSheet의 exit 애니메이션이 동작하지 않을 수 있습니다.
현재 if (showRestoreSheet) 조건으로 CommonBottomSheet를 마운트/언마운트하고 있는데, CommonBottomSheet(CommonBottomSheet.kt Line 52-105)는 visible 파라미터의 변화를 감지하여 내부적으로 internalVisible → delay(200) → rendering = false 순서로 exit 애니메이션을 처리합니다.
if문으로 제거하면 visible이 false로 바뀌기 전에 Composable 자체가 composition에서 사라지기 때문에, exit 애니메이션(scrim fade-out 등)이 재생되지 않습니다.
CommonBottomSheet의 visible 파라미터에 showRestoreSheet를 직접 전달하고, if문을 제거하는 방식이 더 적합합니다.
🛠️ 수정 제안: visible 파라미터 활용
- if (showRestoreSheet) {
- CommonBottomSheet(
- visible = true,
- config = CommonBottomSheetConfig(showHandle = false),
- onDismissRequest = onDismissSheet,
- content = { RestoreCoupleBottomSheetContent() },
- )
- }
+ CommonBottomSheet(
+ visible = showRestoreSheet,
+ config = CommonBottomSheetConfig(showHandle = false),
+ onDismissRequest = onDismissSheet,
+ content = { RestoreCoupleBottomSheetContent() },
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (showRestoreSheet) { | |
| CommonBottomSheet( | |
| visible = true, | |
| config = CommonBottomSheetConfig(showHandle = false), | |
| onDismissRequest = onDismissSheet, | |
| content = { RestoreCoupleBottomSheetContent() }, | |
| ) | |
| } | |
| CommonBottomSheet( | |
| visible = showRestoreSheet, | |
| config = CommonBottomSheetConfig(showHandle = false), | |
| onDismissRequest = onDismissSheet, | |
| content = { RestoreCoupleBottomSheetContent() }, | |
| ) |
🤖 Prompt for AI Agents
In
`@feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt`
around lines 108 - 115, The bottom sheet is being conditionally composed with an
if (showRestoreSheet) which prevents CommonBottomSheet from observing visible
changes and running its exit animation; remove the surrounding if and always
compose CommonBottomSheet, passing showRestoreSheet into its visible parameter
(keep onDismissRequest = onDismissSheet and content = {
RestoreCoupleBottomSheetContent() }) so CommonBottomSheet can animate its
internalVisible → delay(200) → rendering = false exit sequence correctly.
feature/onboarding/src/main/java/com/twix/onboarding/model/OnBoardingIntent.kt
Show resolved
Hide resolved
feature/onboarding/src/main/java/com/twix/onboarding/model/OnBoardingSideEffect.kt
Show resolved
Hide resolved
| LaunchedEffect(viewModel.sideEffect) { | ||
| viewModel.sideEffect.collect { sideEffect -> | ||
| when (sideEffect) { | ||
| OnBoardingSideEffect.ProfileSetting.ShowInvalidNickNameToast -> { | ||
| toastManager.tryShow( | ||
| ToastData( | ||
| message = notValidNickNameMessage, | ||
| type = ToastType.ERROR, | ||
| ), | ||
| ) | ||
| } | ||
|
|
||
| OnBoardingSideEffect.ProfileSetting.NavigateToNext -> onNext() | ||
| else -> Unit | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Side effect 수집 방식이 lifecycle-safe하지 않고, InviteCodeScreen과 일관성이 없습니다.
현재 LaunchedEffect(viewModel.sideEffect) 내부에서 직접 collect를 호출하고 있는데, 이 방식은 repeatOnLifecycle을 사용하지 않기 때문에 앱이 백그라운드에 있을 때도 side effect를 수집합니다. 예를 들어, 백그라운드에서 NavigateToNext가 emit되면 비정상적인 네비게이션이 발생할 수 있습니다.
InviteCodeScreen.kt에서는 ObserveAsEvents를 사용하여 lifecycle-aware하게 처리하고 있으므로, 동일한 패턴을 적용하는 것이 좋겠습니다.
🛠️ 수정 제안: ObserveAsEvents 사용
- val notValidNickNameMessage =
- stringResource(R.string.onboarding_profile_invalid_name_length_toast)
- LaunchedEffect(viewModel.sideEffect) {
- viewModel.sideEffect.collect { sideEffect ->
- when (sideEffect) {
- OnBoardingSideEffect.ProfileSetting.ShowInvalidNickNameToast -> {
- toastManager.tryShow(
- ToastData(
- message = notValidNickNameMessage,
- type = ToastType.ERROR,
- ),
- )
- }
-
- OnBoardingSideEffect.ProfileSetting.NavigateToNext -> onNext()
- else -> Unit
- }
- }
- }
+ val notValidNickNameMessage =
+ stringResource(R.string.onboarding_profile_invalid_name_length_toast)
+ ObserveAsEvents(viewModel.sideEffect) { sideEffect ->
+ when (sideEffect) {
+ OnBoardingSideEffect.ProfileSetting.ShowInvalidNickNameToast -> {
+ toastManager.tryShow(
+ ToastData(
+ message = notValidNickNameMessage,
+ type = ToastType.ERROR,
+ ),
+ )
+ }
+
+ OnBoardingSideEffect.ProfileSetting.NavigateToNext -> onNext()
+ else -> Unit
+ }
+ }ObserveAsEvents는 내부적으로 repeatOnLifecycle(Lifecycle.State.STARTED)를 사용하므로 (core/ui/src/main/java/com/twix/ui/base/ObserveAsEvents.kt 참조), STOPPED 상태에서는 자동으로 수집을 취소합니다.
As per coding guidelines, "SideEffect가 일회성 이벤트로 분리되어 있는가?"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LaunchedEffect(viewModel.sideEffect) { | |
| viewModel.sideEffect.collect { sideEffect -> | |
| when (sideEffect) { | |
| OnBoardingSideEffect.ProfileSetting.ShowInvalidNickNameToast -> { | |
| toastManager.tryShow( | |
| ToastData( | |
| message = notValidNickNameMessage, | |
| type = ToastType.ERROR, | |
| ), | |
| ) | |
| } | |
| OnBoardingSideEffect.ProfileSetting.NavigateToNext -> onNext() | |
| else -> Unit | |
| } | |
| } | |
| } | |
| ObserveAsEvents(viewModel.sideEffect) { sideEffect -> | |
| when (sideEffect) { | |
| OnBoardingSideEffect.ProfileSetting.ShowInvalidNickNameToast -> { | |
| toastManager.tryShow( | |
| ToastData( | |
| message = notValidNickNameMessage, | |
| type = ToastType.ERROR, | |
| ), | |
| ) | |
| } | |
| OnBoardingSideEffect.ProfileSetting.NavigateToNext -> onNext() | |
| else -> Unit | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@feature/onboarding/src/main/java/com/twix/onboarding/profile/ProfileScreen.kt`
around lines 59 - 75, The current LaunchedEffect(viewModel.sideEffect) + collect
is not lifecycle-safe; replace that block with the same pattern used in
InviteCodeScreen by using ObserveAsEvents/observeAsEvents to observe
viewModel.sideEffect lifecycle-aware (uses repeatOnLifecycle STARTED). Move the
when-handling of OnBoardingSideEffect.ProfileSetting (including
toastManager.tryShow(...) for ShowInvalidNickNameToast and onNext() for
NavigateToNext) into the ObserveAsEvents callback so events are consumed only
while the UI is STARTED; keep the existing side-effect cases and remove the
direct LaunchedEffect.collect usage.
feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt
Outdated
Show resolved
Hide resolved
feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt
Outdated
Show resolved
Hide resolved
- 프로필 설정 후 온보딩 상태에 따라 분기 처리 - 불필요한 `launchSingleTop` 옵션 제거 - 파라미터명 `onNext`에서 `navigateTo...`로 명확하게 변경
- 디데이 등록 화면 UI 및 로직을 구현했습니다. - 사용자는 기념일을 선택하고 등록할 수 있습니다. - 날짜 선택을 위해 캘린더 바텀시트를 사용합니다. - 디데이 등록 후 메인 화면으로 이동하도록 네비게이션 로직을 수정했습니다. - 관련 ViewModel 로직, Intent, SideEffect 및 UiState를 추가했습니다.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@feature/onboarding/src/main/java/com/twix/onboarding/dday/DdayRoute.kt`:
- Around line 110-120: The AppButton currently only changes color based on
uiModel.isSelected but remains clickable; update the AppButton call (the
instance that passes text, onClick = { onCompleted() }, backgroundColor,
textColor, modifier) to also set enabled = uiModel.isSelected so the button is
disabled when no date is selected; keep onClick as-is (or optionally guard
inside onCompleted if needed) so the UI prevents emitting SubmitDday when
uiModel.isSelected is false.
- Around line 136-153: Rename the preview function DdayScreenPrview to the
correctly spelled DdayScreenPreview; update the function declaration and any
references/calls (keeping `@Preview` and `@Composable` annotations and existing
parameter setup) so the IDE preview and searches reflect the correct name.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/model/OnBoardingSideEffect.kt`:
- Around line 6-12: The two data objects inside the ProfileSetting sealed
interface are declared with the wrong supertype: change NavigateToDDaySetting
(currently declared as ": CoupleConnection") and NavigateToHome (currently
declared as ": InviteCode") so they inherit from ProfileSetting instead of
CoupleConnection/InviteCode; locate the sealed interface ProfileSetting and
update the declarations of NavigateToDDaySetting and NavigateToHome to extend
ProfileSetting (keeping ShowInvalidNickNameToast as-is) so pattern matching
against OnBoardingSideEffect.ProfileSetting works correctly.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/navigation/OnboardingNavGraph.kt`:
- Around line 28-35: The current forward navigations (e.g., in
CoupleConnectRoute -> navigateToNext using
navController.navigate(NavRoutes.InviteRoute.route) { popUpTo(graphRoute.route)
{ inclusive = true } }) are removing previous back stack entries causing Profile
data loss when navigateToBack is used; remove the inclusive popUpTo for flows
that must preserve back navigation (specifically between Profile and Dday) or
alternatively implement the reviewer’s Direction 2: keep forward navigation
without popUpTo for Profile→Dday and change the back handler (navigateToBack) to
call navController.popBackStack() (referencing navigateToBack and
navController.popBackStack()) so the existing Profile entry is restored instead
of recreating it.
- Around line 23-76: The routes create new ViewModel instances because each
Route calls koinViewModel() with the screen's own backStackEntry; update each
composable block (CoupleConnectRoute, InviteCodeRoute, ProfileRoute, DdayRoute)
to obtain the parent graph backStackEntry via
navController.getBackStackEntry(graphRoute.route) (store it with
remember(backStackEntry)) and pass that as viewModelStoreOwner into
koinViewModel(viewModelStoreOwner = parentEntry) so all screens share the same
OnBoardingViewModel instance for the navigation graph.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt`:
- Around line 97-102: The anniversarySetup function must handle failures like
other network calls: wrap the call to onBoardingRepository.anniversarySetup(...)
inside runCatching within the existing viewModelScope.launch in
anniversarySetup, and on success call
emitSideEffect(OnBoardingSideEffect.DdaySetting.NavigateToHome) while on failure
emit an appropriate error side effect (or log) — update anniversarySetup to use
runCatching { onBoardingRepository.anniversarySetup(...) }.onSuccess {
emitSideEffect(OnBoardingSideEffect.DdaySetting.NavigateToHome) }.onFailure { /*
emit or log error side effect */ }.
- Around line 70-91: profileSetup currently launches a coroutine and calls
fetchOnboardingStatus which itself launches another coroutine and neither
handles errors; change fetchOnboardingStatus to a suspend function (remove its
internal viewModelScope.launch), have profileSetup call it directly so execution
is sequential, and add try/catch around both network calls
(onBoardingRepository.profileSetup and
onBoardingRepository.fetchOnboardingStatus) to catch exceptions and emit an
error side effect (e.g., emitSideEffect(OnBoardingSideEffect.Error(...))) or set
an error state via currentState; update OnBoardingViewModel methods profileSetup
and fetchOnboardingStatus accordingly to ensure exceptions don't crash the
coroutine and the UI receives feedback.
🧹 Nitpick comments (8)
feature/onboarding/src/main/java/com/twix/onboarding/dday/component/DdayTopBar.kt (1)
18-40: 접근성(Accessibility)을 위해contentDescription을 제공하고, Preview Composable을 추가해 주세요.
Line 33의
contentDescription = null은 뒤로가기 버튼처럼 인터랙티브한 요소에는 적절하지 않습니다. 스크린 리더 사용자가 해당 버튼의 용도를 알 수 없게 됩니다.stringResource를 통해 "뒤로가기" 등의 설명을 제공하는 것이 좋습니다.코딩 가이드라인에 따라 Preview Composable이 필요합니다. 다른 컴포넌트(
DDayField)에는 Preview가 제공되어 있으므로 일관성을 맞춰주세요.🛠️ 개선 제안
Image( imageVector = ImageVector.vectorResource(R.drawable.ic_arrow1_m_left), - contentDescription = null, + contentDescription = stringResource(R.string.navigate_back), modifier = Modifier .size(44.dp) .noRippleClickable(onClick = navigateToBack), ) } } + +@Preview(showBackground = true) +@Composable +private fun DdayTopBarPreview() { + TwixTheme { + DdayTopBar(navigateToBack = {}) + } +}As per coding guidelines, "Preview Composable이 제공되는가?"
feature/onboarding/src/main/java/com/twix/onboarding/dday/component/DDayField.kt (2)
44-54: 날짜 표시 형식을 사용자 친화적으로 변경하는 것을 고려해 주세요.
uiModel.anniversaryDate.toString()은 ISO-8601 형식(2024-01-01)을 출력합니다. 한국 사용자에게는2024년 1월 1일또는2024.01.01같은 로컬라이즈된 형식이 더 자연스럽습니다.♻️ 개선 제안
+import java.time.format.DateTimeFormatter + +private val dateFormatter = DateTimeFormatter.ofPattern("yyyy년 M월 d일") + // DDayField 내부에서: text = if (uiModel.isSelected) { - uiModel.anniversaryDate.toString() + uiModel.anniversaryDate.format(dateFormatter) } else {
61-68: 캘린더 아이콘에contentDescription을 추가해 주세요.인터랙티브한 요소(
noRippleClickable이 적용된 캘린더 아이콘)에contentDescription = null은 스크린 리더 사용자가 해당 요소의 기능을 인식할 수 없게 합니다. "날짜 선택" 등의 설명을 제공하면 접근성이 향상됩니다.feature/onboarding/src/main/java/com/twix/onboarding/profile/ProfileScreen.kt (1)
173-205: 유효/무효 상태에 대한 Preview가 모두 제공되어 있어 좋습니다. 👍한 가지 사소한 점:
UnValidProfileScreenPreview(Line 175)의 네이밍이InvalidProfileScreenPreview가 더 자연스럽습니다. "UnValid"는 영어에서 일반적으로 사용되지 않는 표현입니다.feature/onboarding/src/main/java/com/twix/onboarding/dday/DdayRoute.kt (1)
42-71: 잘 구성된 MVI 패턴의 Route 함수입니다 👍
ObserveAsEvents를 통한 SideEffect 처리,collectAsStateWithLifecycle을 통한 상태 수집, 그리고 State Hoisting이 잘 적용되어 있습니다.다만 두 가지 개선 포인트가 있습니다:
toastManager가 주입만 되고 사용되지 않습니다. 현재 사이드이펙트 핸들러에서 toast를 표시하는 로직이 없으므로, 실제로 사용하기 전까지는 제거하거나, 향후 에러 토스트 표시 등을 위해 남겨두시려면 주석으로 의도를 명시해 주세요.
showCalendarBottomSheet가remember로 선언되어 있어 configuration change(화면 회전 등) 시 상태가 유실됩니다.CoupleConnectRoute에서는rememberSaveable을 사용하고 있으므로 일관성을 위해서도rememberSaveable이 적절합니다.♻️ 수정 제안
+import androidx.compose.runtime.saveable.rememberSaveable + `@Composable` fun DdayRoute( navigateToHome: () -> Unit, navigateToBack: () -> Unit, - toastManager: ToastManager = koinInject(), viewModel: OnBoardingViewModel = koinViewModel(), ) { val uiState by viewModel.uiState.collectAsStateWithLifecycle() - var showCalendarBottomSheet by remember { mutableStateOf(false) } + var showCalendarBottomSheet by rememberSaveable { mutableStateOf(false) }feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt (2)
51-57:onClickSend가 빈 핸들러로 전달되고 있으며,InvitationButton은 주석 처리되어 있습니다.Line 93에서
InvitationButton이 주석 처리되어 있고,onClickSend는 빈 람다({ })로 전달됩니다.CoupleConnectScreen의 파라미터로도 선언되어 있지만 실제 사용처가 없는 상태입니다. 추후 구현 예정이라면 TODO 주석을 남겨 의도를 명시하면 좋겠습니다. 구현 계획이 없다면 파라미터를 제거하여 혼란을 줄이는 것도 방법입니다.
60-121: 전반적으로 잘 구성된 Screen 함수입니다. State hoisting이 적절히 적용되었고, 디자인 시스템 컴포넌트(AppText,ConnectButton,CommonBottomSheet)를 잘 활용하고 있습니다.한 가지 질문: Line 75와 84의
80.24.dp,11.76.dp같은 소수점 dp 값은 디자인 스펙에서 그대로 가져온 것인가요? 일반적으로 dp는 정수 단위로 사용하는 것이 관례이고, 하위 픽셀 렌더링에서 예측하기 어려운 결과가 나올 수 있습니다. 디자인 시스템과 확인 후 정수로 라운딩하는 것을 고려해 보세요.feature/onboarding/src/main/res/values/strings.xml (1)
3-10: 중복 문자열 리소스가 있습니다.
onboarding_name_placeholder(Line 4)와onboarding_profile_placeholder(Line 7)가 동일한 값("닉네임을 입력해 주세요.")이고,onboarding_name_helper(Line 5)와onboarding_profile_helper(Line 8)도 동일합니다.의도적으로 화면별 독립성을 위해 분리한 것일 수 있지만, 동일한 값을 두 곳에서 관리하면 향후 번역 시 불일치가 발생할 수 있습니다. 만약 항상 같은 값을 유지해야 한다면, 하나의 리소스로 통합하는 것을 고려해 보세요. 반대로 화면별로 다른 문구로 분기할 가능성이 있다면 현재 구조가 적절합니다.
feature/onboarding/src/main/java/com/twix/onboarding/model/OnBoardingSideEffect.kt
Show resolved
Hide resolved
feature/onboarding/src/main/java/com/twix/onboarding/navigation/OnboardingNavGraph.kt
Show resolved
Hide resolved
feature/onboarding/src/main/java/com/twix/onboarding/navigation/OnboardingNavGraph.kt
Outdated
Show resolved
Hide resolved
feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt
Show resolved
Hide resolved
feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt
Show resolved
Hide resolved
`graphViewModel`을 추가하여 Navigation Graph 범위의 ViewModel을 주입받을 수 있도록 구현했습니다.
초대코드 입력 화면(`InviteCodeScreen`)에 뒤로가기 기능을 구현하고, 키보드가 닫혔을 때의 상단 UI를 조정했습니다.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/onboarding/src/main/java/com/twix/onboarding/navigation/OnboardingNavGraph.kt`:
- Around line 76-78: The navigateToBack lambda currently calls
navController.navigate(NavRoutes.ProfileRoute.route), which pushes a new
ProfileRoute onto the back stack; change it to use navController.popBackStack
(or navController::popBackStack) so the Dday screen is popped and the existing
ProfileRoute instance is resumed (preserving graph-scoped state). Update the
navigateToBack implementation where it references NavRoutes.ProfileRoute.route
to call popBackStack instead, matching the InviteRoute usage.
🧹 Nitpick comments (3)
feature/onboarding/src/main/java/com/twix/onboarding/model/OnBoardingSideEffect.kt (1)
14-14:CoupleConnection이 비어 있는 sealed interface인데, 현재 불필요한 상태입니다.OnBoardingViewModel의
connectCouple()메서드를 보면, 커플 연결 완료 후OnBoardingSideEffect.InviteCode.NavigateToNext를 발행하고 있습니다(라인 52).CoupleConnection은 정의되어 있지만 실제로는 어떤 SideEffect도 포함하지 않으며, 어디에서도 사용되지 않습니다.코딩 가이드의 MVI 패턴 요구사항("SideEffect가 일회성 이벤트로 분리되어 있는가?")을 고려하면, 다음 두 가지 중 선택할 수 있습니다:
- 현재 불필요하다면: YAGNI 원칙에 따라 제거하고, 향후 커플 연결 화면에서 추가 SideEffect가 필요하면 그 때 다시 정의합니다.
- 향후 확장을 위해 유지한다면: 주석으로 의도를 명확히 해 줍니다 (예:
// 향후 에러 처리 등의 SideEffect 추가 예정).빈 sealed interface는 코드를 읽는 사람에게 "무언가 빠진 건가?"라는 혼동을 주고,
when분기에서 불필요한 빈 케이스를 만들 수 있습니다.feature/onboarding/src/main/java/com/twix/onboarding/dday/DdayRoute.kt (1)
42-69:toastManager가 주입되었지만 사용되지 않고 있습니다.
DdayRoute에서ToastManager를koinInject()로 주입받고 있지만,ObserveAsEvents블록 내에서 토스트를 표시하는 로직이 없습니다. 현재는NavigateToHome만 처리하고 있어 불필요한 의존성입니다.향후 에러 토스트 등이 추가될 예정이라면 유지해도 되지만, 현재 시점에서는 YAGNI 원칙에 따라 제거하는 것을 권장합니다. 필요해질 때 다시 추가하면 됩니다.
🛠️ 수정 제안
`@Composable` fun DdayRoute( viewModel: OnBoardingViewModel, navigateToHome: () -> Unit, navigateToBack: () -> Unit, - toastManager: ToastManager = koinInject(), ) {feature/onboarding/src/main/java/com/twix/onboarding/invite/InviteCodeScreen.kt (1)
114-136: 뒤로가기 버튼이 별도Box에 있어 Column 콘텐츠와 겹칠 수 있습니다.뒤로가기 버튼은
Box루트의 첫 번째 자식으로,Column은 두 번째 자식으로 배치되어 있습니다.Column내부의Spacer(8.dp)다음에 오는AnimatedVisibility콘텐츠에top = 80.dp패딩이 있어 현재는 시각적으로 겹치지 않을 수 있지만, 키보드가 열려서AnimatedVisibility가 숨겨지면 Column 상단이 72.dp 높이의 뒤로가기 버튼과 겹칠 수 있습니다.
DdayRoute에서처럼DdayTopBar를 Column 내부에 배치하는 방식이 레이아웃 안정성 측면에서 더 안전합니다. 현재 동작에 문제가 없다면 참고 수준으로 봐주세요.
| navigateToBack = { | ||
| navController.navigate(NavRoutes.ProfileRoute.route) | ||
| }, |
There was a problem hiding this comment.
navigateToBack에서 popBackStack()을 사용해야 합니다.
현재 Dday 화면의 뒤로가기는 navController.navigate(NavRoutes.ProfileRoute.route)를 사용하고 있습니다. 이 방식은 기존 ProfileRoute 엔트리로 돌아가는 것이 아니라 새로운 ProfileRoute 엔트리를 back stack에 추가합니다.
문제점:
- Back stack이 계속 쌓입니다 (Profile → Dday → Profile → Dday → ...)
ProfileScreen의LaunchedEffect(Unit)이 재실행되어 focusRequester가 다시 포커스를 요청합니다- 같은 파일의
InviteRoute(Line 47)에서는 올바르게navController::popBackStack을 사용하고 있어 일관성도 맞지 않습니다
graphViewModel으로 ViewModel이 graph 스코프에서 공유되므로, popBackStack()을 사용해도 상태가 보존됩니다.
🛠️ popBackStack 사용
DdayRoute(
viewModel = vm,
navigateToHome = {
navController.navigate(NavRoutes.MainGraph.route) {
popUpTo(graphRoute.route) { inclusive = true }
}
},
navigateToBack = {
- navController.navigate(NavRoutes.ProfileRoute.route)
+ navController.popBackStack()
},
)🤖 Prompt for AI Agents
In
`@feature/onboarding/src/main/java/com/twix/onboarding/navigation/OnboardingNavGraph.kt`
around lines 76 - 78, The navigateToBack lambda currently calls
navController.navigate(NavRoutes.ProfileRoute.route), which pushes a new
ProfileRoute onto the back stack; change it to use navController.popBackStack
(or navController::popBackStack) so the Dday screen is popped and the existing
ProfileRoute instance is resumed (preserving graph-scoped state). Update the
navigateToBack implementation where it references NavRoutes.ProfileRoute.route
to call popBackStack instead, matching the InviteRoute usage.
- 초대 코드 조회 실패 시 에러 토스트를 표시합니다. - 잘못된 초대 코드 입력 시 에러 토스트를 표시합니다. - 기념일 설정 실패 시 에러 토스트를 표시합니다. - API 통신 로직을 `launchResult`로 리팩터링하여 예외 처리를 구현합니다.
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)
feature/login/src/main/java/com/twix/login/LoginViewModel.kt (1)
24-38:⚠️ Potential issue | 🟠 Major
authRepository.login()호출에 대한 예외 처리가 누락되어 있어 사용자 피드백이 부족합니다.
authRepository.login()은DefaultAuthRepository에서 네트워크 호출과 토큰 저장 작업을 수행하는데, 실패 시 예외를 throw합니다. 현재 코드는viewModelScope.launch블록 내에서 직접 호출되고 있으며,BaseViewModel의handleIntent예외 처리(try-catch)가 예외를 로깅하기만 할 뿐 사용자에게 에러 피드백을 제공하지 않습니다.반면
checkOnboardingStatus()는launchResult()를 통해 에러 시onError핸들러로ShowFetchOnBoardingStatusFailToast사이드 이펙트를 발생시킵니다. 두 메서드 모두 실패 가능성이 있으므로 일관된 에러 처리 패턴이 필요합니다.개선 방안:
authRepository.login()을try-catch로 감싸고 실패 시emitSideEffect(LoginSideEffect.ShowLoginFailToast)발생- 또는
authRepository.login()의 반환 타입을AppResult<Unit>으로 변경하여launchResult()로 래핑하기
🤖 Fix all issues with AI agents
In `@feature/login/src/main/res/values/strings.xml`:
- Around line 4-5: The two string resources are identical but represent
different failures; update the value of fetch_onboarding_status_fail_message
(currently identical to login_fail_message) to a message that reflects
onboarding-status retrieval failure—for example use a Korean string like "온보딩
상태를 불러오지 못했습니다. 잠시 후 다시 시도해주세요"—so UI shows the correct error context when
onBoarding status lookup (fetch_onboarding_status_fail_message) fails while
login (login_fail_message) succeeded.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/model/OnBoardingSideEffect.kt`:
- Around line 26-30: ShowAnniversarySetupFailToast is declared inside the sealed
interface DdaySetting but incorrectly implements ProfileSetting, causing it to
be misclassified in pattern matches; update ShowAnniversarySetupFailToast to
implement DdaySetting (i.e., change its supertype from ProfileSetting to
DdaySetting or make it a member object of DdaySetting) so that when matching
OnBoardingSideEffect.DdaySetting it will be correctly recognized; check the
sealed interfaces DdaySetting, ProfileSetting and the data object
ShowAnniversarySetupFailToast to ensure the inheritance aligns with the intended
side-effect grouping.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt`:
- Around line 100-122: The onError block in fetchOnboardingStatus is empty and
must surface failure to the UI; update the launchResult(onError = { ... })
handler (in fetchOnboardingStatus) to emit a user-visible side effect or state,
e.g., call emitSideEffect with a new OnBoardingSideEffect.ProfileSetting.Error
or Retry action (or trigger a Toast via a UI event), and optionally include
logging of the throwable; ensure the chosen symbol (emitSideEffect and
OnBoardingSideEffect.ProfileSetting.*) is used so the UI can show an error
message and/or a retry option when fetchOnboardingStatus fails.
🧹 Nitpick comments (7)
feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt (1)
97-99: 소수점 dp 값(80.24.dp,11.76.dp)이 의도된 값인지 확인이 필요합니다.
80.24.dp와11.76.dp는 일반적이지 않은 소수점 값입니다. 디자인 시스템의 스펙에서 가져온 값이라면 괜찮지만, 렌더링 시 소수점 픽셀은 반올림되어 디바이스에 따라 미세한 차이가 발생할 수 있습니다. 혹시80.dp+12.dp등 정수값이 의도된 것은 아닌지 디자인 팀과 확인해 보시는 것을 권장합니다.feature/onboarding/src/main/java/com/twix/onboarding/dday/DdayRoute.kt (1)
51-51:showCalendarBottomSheet에remember대신rememberSaveable사용을 고려해 보세요.
CoupleConnectRoute에서는 유사한 bottom sheet 상태(showRestoreSheet)에rememberSaveable을 사용하고 있는데, 여기서는remember를 사용하고 있습니다.remember는 configuration change(화면 회전 등) 시 상태가 초기화되므로, 캘린더가 열린 상태에서 회전하면 닫히게 됩니다.캘린더 bottom sheet가 열려 있는 도중 회전 시 닫히는 것이 의도된 UX라면 괜찮지만, 그렇지 않다면 일관성을 위해
rememberSaveable로 변경하는 것이 좋겠습니다.♻️ 제안 수정안
- var showCalendarBottomSheet by remember { mutableStateOf(false) } + var showCalendarBottomSheet by rememberSaveable { mutableStateOf(false) }
rememberSaveableimport도 추가해주세요:import androidx.compose.runtime.saveable.rememberSaveablefeature/login/src/main/java/com/twix/login/LoginViewModel.kt (1)
41-63:onSuccess내부의viewModelScope.launch중첩이 개선 가능합니다.
launchResult가 이미viewModelScope.launch내부에서 실행되는데,onSuccess콜백에서 다시viewModelScope.launch를 사용하고 있습니다. 이는onSuccess가 non-suspend 타입((D) -> Unit)으로 정의되어 있어emitSideEffect(suspend)를 직접 호출할 수 없기 때문인 것으로 보입니다.반면
onError는suspend (AppError) -> Unit으로 정의되어 있어 Line 60에서emitSideEffect를 직접 호출할 수 있습니다. 이 비대칭이onSuccess에서만 추가 코루틴 실행이 필요한 원인입니다.
BaseViewModel.launchResult의onSuccess타입을suspend (D) -> Unit으로 변경하면 이 중첩을 제거할 수 있습니다. 당장은 동작에 문제가 없으나, 향후 리팩터링 시 고려해 보시면 좋겠습니다.♻️ BaseViewModel 변경 시 LoginViewModel 개선안
BaseViewModel.launchResult의onSuccess를 suspend로 변경한다면:// BaseViewModel.kt -onSuccess: (D) -> Unit, +onSuccess: suspend (D) -> Unit,그러면 LoginViewModel에서 중첩을 제거할 수 있습니다:
private fun checkOnboardingStatus() { launchResult( block = { onBoardingRepository.fetchOnboardingStatus() }, onSuccess = { onboardingStatus -> - viewModelScope.launch { - val sideEffect = - when (onboardingStatus) { - OnboardingStatus.COUPLE_CONNECTION, - OnboardingStatus.PROFILE_SETUP, - OnboardingStatus.ANNIVERSARY_SETUP, - -> LoginSideEffect.NavigateToOnBoarding(onboardingStatus) - - OnboardingStatus.COMPLETED -> LoginSideEffect.NavigateToHome - } - - emitSideEffect(sideEffect) - } + val sideEffect = + when (onboardingStatus) { + OnboardingStatus.COUPLE_CONNECTION, + OnboardingStatus.PROFILE_SETUP, + OnboardingStatus.ANNIVERSARY_SETUP, + -> LoginSideEffect.NavigateToOnBoarding(onboardingStatus) + + OnboardingStatus.COMPLETED -> LoginSideEffect.NavigateToHome + } + emitSideEffect(sideEffect) }, onError = { emitSideEffect(LoginSideEffect.ShowFetchOnBoardingStatusFailToast) }, ) }feature/onboarding/src/main/java/com/twix/onboarding/invite/InviteCodeScreen.kt (1)
107-114: 클래스명InViteCodeUiModel을InviteCodeUiModel로 수정하세요.Kotlin 명명 규칙(PascalCase)에 따르면 클래스명의 각 단어는 첫 글자만 대문자여야 합니다. 현재
InViteCodeUiModel에서 "Invite"의 V가 대문자로 되어 있어 규칙에 맞지 않습니다. 올바른 이름은InviteCodeUiModel입니다.다행히 이 클래스는 여러 파일에서 import되고 사용되지만, IDE의 Rename 리팩터링 기능으로 한 번에 수정할 수 있습니다:
- 클래스 정의:
InViteCodeUiModel.kt파일의 클래스명- 영향받는 파일:
OnBoardingUiState.kt,InviteCodeScreen.kt,InViteCodeUiModel.kt내 메서드 반환 타입feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt (3)
51-59:onSuccess내부의viewModelScope.launch—BaseViewModel.launchResult의onSuccess시그니처가 non-suspend이기 때문에 발생하는 패턴입니다
emitSideEffect가suspend함수인 반면launchResult의onSuccess는(D) -> Unit(non-suspend)으로 정의되어 있어,emitSideEffect호출을 위해viewModelScope.launch로 감싸야 하는 상황입니다. 이 패턴이 Line 54-56, 104-116, 132에서 반복됩니다.현재 구조에서는 불가피하지만,
onSuccess를suspend람다로 변경하면 이 중첩을 제거할 수 있습니다.BaseViewModel의launchResult시그니처 변경은 이 PR의 범위를 벗어나므로, 향후 개선 사항으로 고려해보시면 어떨까요?
62-77: 서버 에러 메시지 문자열 비교는 취약한 패턴입니다 — 서버 응답 변경 시 조용히 실패할 수 있습니다서버 에러 메시지(
"이미 사용된 초대 코드입니다.","유효하지 않은 초대 코드입니다.")를 문자열 비교로 분기하고 있습니다. 서버의 메시지 문구가 변경되거나 다국어 지원이 추가되면 이 분기 로직이 조용히 실패하게 됩니다.가능하다면
AppError.Http의code필드(에러 코드)를 기준으로 분기하는 것이 더 안정적입니다. 만약 서버가 에러 코드를 아직 제공하지 않는다면, 서버 팀과 협의하여 에러 코드 기반 분기를 도입하는 것을 권장합니다.♻️ 에러 코드 기반 분기 예시
private suspend fun handleCoupleConnectException(error: AppError) { - if (error is AppError.Http && error.status == 404) { - if (error.message == INVALID_INVITE_CODE_MESSAGE) { + if (error is AppError.Http) { + when (error.code) { + INVALID_INVITE_CODE_ERROR_CODE -> { emitSideEffect(OnBoardingSideEffect.InviteCode.ShowInvalidInviteCodeToast) } - if (error.message == ALREADY_USED_INVITE_CODE_MESSAGE) { + ALREADY_USED_INVITE_CODE_ERROR_CODE -> { emitSideEffect(OnBoardingSideEffect.InviteCode.NavigateToNext) } + } } }
83-91:handleSubmitNickname에서emitSideEffect호출 시viewModelScope.launch사용에 대해Line 87-89에서
viewModelScope.launch로 감싸서emitSideEffect를 호출하고 있습니다.handleSubmitNickname이handleIntent에서 호출되고,handleIntent는 이미suspend함수이므로 직접emitSideEffect를 호출할 수 있지 않은지 확인이 필요합니다.만약
handleIntent의 코루틴 컨텍스트에서 직접 호출 가능하다면viewModelScope.launch를 제거하여 불필요한 코루틴 생성을 줄일 수 있습니다.♻️ 제안
- private fun handleSubmitNickname() { + private suspend fun handleSubmitNickname() { if (currentState.isValidNickName) { profileSetup() } else { - viewModelScope.launch { - emitSideEffect(OnBoardingSideEffect.ProfileSetting.ShowInvalidNickNameToast) - } + emitSideEffect(OnBoardingSideEffect.ProfileSetting.ShowInvalidNickNameToast) } }
| <string name="login_fail_message">로그인에 실패했습니다. 다시 시도해주세요</string> | ||
| <string name="fetch_onboarding_status_fail_message">로그인에 실패했습니다. 다시 시도해주세요</string> |
There was a problem hiding this comment.
온보딩 상태 조회 실패 메시지가 로그인 실패 메시지와 동일합니다.
fetch_onboarding_status_fail_message의 값이 login_fail_message와 완전히 동일한 "로그인에 실패했습니다. 다시 시도해주세요"로 되어 있습니다. 이 메시지는 로그인은 성공했지만 온보딩 상태 조회에 실패한 경우에 표시되므로, 사용자 입장에서 실제 원인과 다른 피드백을 받게 됩니다.
예를 들어, 사용자가 로그인에 성공한 직후 "로그인에 실패했습니다"라는 토스트를 보면 혼란스러울 수 있습니다. 메시지를 실제 실패 원인에 맞게 변경하는 것이 좋겠습니다.
🛠️ 제안 수정안
- <string name="fetch_onboarding_status_fail_message">로그인에 실패했습니다. 다시 시도해주세요</string>
+ <string name="fetch_onboarding_status_fail_message">초기 설정 정보를 불러오지 못했습니다. 다시 시도해주세요</string>As per coding guidelines, **/res/**: "하드코딩 문자열이 없는가?" — 리소스 자체의 문자열 내용은 적절한 의미를 전달해야 합니다.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <string name="login_fail_message">로그인에 실패했습니다. 다시 시도해주세요</string> | |
| <string name="fetch_onboarding_status_fail_message">로그인에 실패했습니다. 다시 시도해주세요</string> | |
| <string name="login_fail_message">로그인에 실패했습니다. 다시 시도해주세요</string> | |
| <string name="fetch_onboarding_status_fail_message">초기 설정 정보를 불러오지 못했습니다. 다시 시도해주세요</string> |
🤖 Prompt for AI Agents
In `@feature/login/src/main/res/values/strings.xml` around lines 4 - 5, The two
string resources are identical but represent different failures; update the
value of fetch_onboarding_status_fail_message (currently identical to
login_fail_message) to a message that reflects onboarding-status retrieval
failure—for example use a Korean string like "온보딩 상태를 불러오지 못했습니다. 잠시 후 다시
시도해주세요"—so UI shows the correct error context when onBoarding status lookup
(fetch_onboarding_status_fail_message) fails while login (login_fail_message)
succeeded.
| sealed interface DdaySetting : OnBoardingSideEffect { | ||
| data object NavigateToHome : DdaySetting | ||
|
|
||
| data object ShowAnniversarySetupFailToast : ProfileSetting | ||
| } |
There was a problem hiding this comment.
ShowAnniversarySetupFailToast가 DdaySetting이 아닌 ProfileSetting을 상속하고 있습니다 — 이전에 수정된 버그와 동일한 패턴입니다
Line 29에서 ShowAnniversarySetupFailToast가 DdaySetting sealed interface 내부에 선언되어 있지만, : ProfileSetting을 상속하고 있습니다. 이전 리뷰에서 InviteCode.NavigateToNext : CoupleConnection과 ProfileSetting.NavigateToDDaySetting : CoupleConnection 등 동일한 패턴의 버그가 지적되어 수정되었는데, 이 케이스는 아직 남아있습니다.
왜 문제인가요?
is OnBoardingSideEffect.DdaySetting으로 패턴 매칭 시ShowAnniversarySetupFailToast가 매칭되지 않습니다.- 대신
is OnBoardingSideEffect.ProfileSetting으로 매칭되어, D-day 설정 실패 토스트가 프로필 설정 화면의 사이드이펙트로 잘못 처리될 수 있습니다.
As per coding guidelines, "SideEffect가 일회성 이벤트로 분리되어 있는가?"
🐛 수정 제안
sealed interface DdaySetting : OnBoardingSideEffect {
data object NavigateToHome : DdaySetting
- data object ShowAnniversarySetupFailToast : ProfileSetting
+ data object ShowAnniversarySetupFailToast : DdaySetting
}🤖 Prompt for AI Agents
In
`@feature/onboarding/src/main/java/com/twix/onboarding/model/OnBoardingSideEffect.kt`
around lines 26 - 30, ShowAnniversarySetupFailToast is declared inside the
sealed interface DdaySetting but incorrectly implements ProfileSetting, causing
it to be misclassified in pattern matches; update ShowAnniversarySetupFailToast
to implement DdaySetting (i.e., change its supertype from ProfileSetting to
DdaySetting or make it a member object of DdaySetting) so that when matching
OnBoardingSideEffect.DdaySetting it will be correctly recognized; check the
sealed interfaces DdaySetting, ProfileSetting and the data object
ShowAnniversarySetupFailToast to ensure the inheritance aligns with the intended
side-effect grouping.
| private fun fetchOnboardingStatus() { | ||
| launchResult( | ||
| block = { onBoardingRepository.fetchOnboardingStatus() }, | ||
| onSuccess = { onboardingStatus -> | ||
| viewModelScope.launch { | ||
| val sideEffect = | ||
| when (onboardingStatus) { | ||
| OnboardingStatus.ANNIVERSARY_SETUP -> | ||
| OnBoardingSideEffect.ProfileSetting.NavigateToDDaySetting | ||
|
|
||
| OnboardingStatus.COMPLETED -> | ||
| OnBoardingSideEffect.ProfileSetting.NavigateToHome | ||
|
|
||
| else -> return@launch | ||
| } | ||
| emitSideEffect(sideEffect) | ||
| } | ||
| }, | ||
| onError = { | ||
| // 에러처리 추가 | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
fetchOnboardingStatus의 onError 핸들러가 비어 있습니다 — 사용자가 진행 불가 상태에 빠질 수 있습니다
Line 118-120에서 onError 블록에 주석만 있고 실제 에러 처리가 없습니다. profileSetup() → fetchOnboardingStatus() 순서로 호출되므로, 프로필 설정은 성공했지만 온보딩 상태 조회가 실패하면 사용자는 어떤 피드백도 없이 화면에 머무르게 됩니다.
최소한 에러 토스트를 표시하거나, 재시도 메커니즘을 제공하는 것이 사용자 경험 측면에서 중요합니다.
🤖 Prompt for AI Agents
In
`@feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt`
around lines 100 - 122, The onError block in fetchOnboardingStatus is empty and
must surface failure to the UI; update the launchResult(onError = { ... })
handler (in fetchOnboardingStatus) to emit a user-visible side effect or state,
e.g., call emitSideEffect with a new OnBoardingSideEffect.ProfileSetting.Error
or Retry action (or trigger a Toast via a UI event), and optionally include
logging of the throwable; ensure the chosen symbol (emitSideEffect and
OnBoardingSideEffect.ProfileSetting.*) is used so the UI can show an error
message and/or a retry option when fetchOnboardingStatus fails.
2 participants
이슈 번호
#35
리뷰/머지 희망 기한 (선택)
작업내용
결과물
default.mp4
리뷰어에게 추가로 요구하는 사항 (선택)