Merged
Conversation
- `StatsDetailGraph` 및 `StatsDetailRoute`를 `NavRoutes`에 정의 - `StatsDetailRoute`에 `goalId`와 `date`를 인자로 받는 `createRoute` 함수 추가
- `StatsRepository`에 월별 통계 상세 정보를 조회하는 `fetchStatsDetail` 추가 - 테스트용 `FakeStatsRepository`에 `fetchStatsDetail` 구현
- `StatsRoute`에 `navigateToDetail` 람다 함수를 추가하여 화면 전환 로직을 구현 - 각 통계 카드(진행 중, 완료)에 `goalId`와 `LocalDate`를 전달하는 클릭 이벤트 핸들러 추가
- `statsDetailModule`을 새로 추가하고 `NavGraphContributor`를 주입하여 통계 상세 화면으로의 네비게이션을 구현 - `FeatureModules`에 `statsDetailModule`을 추가하여 앱 전체에 적용
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/design-system/src/main/java/com/twix/designsystem/components/calendar/CalendarNavigator.kt (2)
40-49:⚠️ Potential issue | 🟠 Major비활성화된 화살표가 여전히 클릭 이벤트를 발생시킵니다
hasPrevious = false일 때 화살표가 시각적으로 회색으로 처리되지만,noRippleClickable(onClick = onPreviousMonth)는 조건 없이 항상 적용되어 있습니다. 즉, 사용자가 비활성화된 화살표를 탭하면onPreviousMonth가 그대로 호출됩니다.이는 시각적 피드백과 실제 동작이 불일치하는 UX 버그입니다. 아래와 같이
hasPrevious가false일 때 콜백이 발동되지 않도록 처리하는 방법을 권장합니다:🐛 수정 제안
Image( painter = painterResource(R.drawable.ic_arrow_m_left), contentDescription = "previous month", modifier = Modifier - .noRippleClickable(onClick = onPreviousMonth) + .then( + if (hasPrevious) Modifier.noRippleClickable(onClick = onPreviousMonth) + else Modifier + ) .padding(6.dp) .size(24.dp), colorFilter = ColorFilter.tint(if (hasPrevious) GrayColor.C500 else GrayColor.C200), )같은 방식으로 Line 58–66의
hasNext화살표에도 동일한 수정이 필요합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/calendar/CalendarNavigator.kt` around lines 40 - 49, The left and right arrow Image composables in CalendarNavigator.kt use noRippleClickable unconditionally so tapping a visually disabled arrow still calls onPreviousMonth/onNextMonth; update the modifier application to only attach the clickable when hasPrevious/hasNext is true (or wrap the callback to a no-op when false) so the onPreviousMonth and onNextMonth callbacks are not invoked for disabled arrows, and keep the existing tint logic (GrayColor.C200 when disabled) unchanged; apply the same change for the hasNext branch (lines referencing onPreviousMonth, onNextMonth, hasPrevious, hasNext, and the Image modifiers).
40-48:⚠️ Potential issue | 🟡 Minor접근성(Accessibility): 비활성화 상태가
contentDescription에 반영되어 있지 않습니다현재
contentDescription = "previous month"/"next month"로 고정되어 있어, 접근성 서비스(TalkBack 등)가 비활성화된 화살표도 일반 버튼과 동일하게 안내합니다. 비활성화 상태일 때 사용자에게 의미 있는 정보를 제공하거나, 클릭 불가 요소로 명확히 표시하는 것이 좋습니다.-contentDescription = "previous month", +contentDescription = if (hasPrevious) "previous month" else "previous month (disabled)",Also applies to: 58-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/calendar/CalendarNavigator.kt` around lines 40 - 48, Update the Image composables used for month navigation (the ones using painterResource(R.drawable.ic_arrow_m_left)/ic_arrow_m_right and modifiers with noRippleClickable(onPreviousMonth)/noRippleClickable(onNextMonth)) so their contentDescription reflects disabled state instead of a fixed "previous month"/"next month": conditionally set contentDescription to include "disabled" (e.g., "previous month, disabled") or set it to null and add a semantics modifier marking the element as disabled when hasPrevious/hasNext is false, and ensure the semantics/role reflects non-clickable when disabled so accessibility services announce the correct state.
🧹 Nitpick comments (11)
feature/stats/detail/src/main/java/com/yapp/detail/contract/StatsDetailSideEffect.kt (1)
6-12:ShowToast.message타입 안정성 강화 제안
현재message: Int는 문자열 리소스 ID로 보이는데, 타입만으로는 보장이 없어 잘못된 리소스나 임의의 Int가 들어갈 위험이 있습니다. 리소스 ID를 의도하신 거라면@StringRes를 붙이거나 아예String으로 바꿔 전달 책임을 명확히 하는 편이 안전합니다. 이런 방향이 맞는지 확인해 주실 수 있을까요?🔧 예시 수정안
+import androidx.annotation.StringRes import com.twix.designsystem.components.toast.model.ToastType import com.twix.ui.base.SideEffect sealed interface StatsDetailSideEffect : SideEffect { data class ShowToast( - val message: Int, + `@StringRes` val message: Int, val type: ToastType, ) : StatsDetailSideEffect🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/yapp/detail/contract/StatsDetailSideEffect.kt` around lines 6 - 12, ShowToast의 message가 Int로 선언되어 있어 리소스 ID가 아닌 임의의 정수가 들어갈 수 있으니 타입 안정성을 강화해 주세요: 의도대로 문자열 리소스 ID를 전달하려면 StatsDetailSideEffect.ShowToast의 message에 `@StringRes` 어노테이션을 추가해 컴파일 시 검증되게 하거나, 리소스를 호출한 쪽에서 문자열을 resolve하도록 하고 message 타입을 String으로 변경해 호출 책임을 명확히 하세요; 변경 대상는 sealed interface StatsDetailSideEffect 내 data class ShowToast의 message 프로퍼티입니다.feature/stats/detail/src/main/java/com/yapp/detail/contract/StatsDetailIntent.kt (1)
5-5:StatsDetailIntent에 사용자 액션이 아직 정의되지 않았습니다.현재
sealed interface가 비어 있어, 실질적인 사용자 액션 계약이 존재하지 않습니다. MVI 가이드라인에서는 "Intent가 사용자 액션 단위로 명확히 정의되어 있는가?" 를 중요 기준으로 삼고 있습니다.PR 스크린샷을 보면, 이미 달력 UI에서 월 이동(이전/다음) 및 날짜 선택 등의 인터랙션이 보입니다. 향후 PR에서 아래와 같은 Intent 항목들을 추가해 주시면 계약이 명확해질 것 같습니다:
sealed interface StatsDetailIntent : Intent { + data object OnPreviousMonthClick : StatsDetailIntent + data object OnNextMonthClick : StatsDetailIntent + data class OnDateSelected(val date: LocalDate) : StatsDetailIntent }초기 스켈레톤 세팅임을 감안해 지금 당장 필수는 아니지만, 다음 PR에서 UI 인터랙션이 추가될 때 함께 채워 주시면 리뷰하기 훨씬 수월할 것 같습니다. 😊
As per coding guidelines,
feature/**경로에 대해 "Intent가 사용자 액션 단위로 명확히 정의되어 있는가?" 를 확인합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/yapp/detail/contract/StatsDetailIntent.kt` at line 5, The sealed interface StatsDetailIntent is currently empty—define explicit user-action intents to represent calendar interactions (e.g., month navigation and date selection) so the MVI contract is clear: add implementations under StatsDetailIntent such as objects for NavigateToPreviousMonth and NavigateToNextMonth and a data class (e.g., DateSelected) carrying the selected LocalDate (or equivalent) and optionally an Initialize/Load intent for initial screen load; update any callers (view/event handlers) to emit these specific intents instead of raw events.domain/src/main/java/com/twix/domain/model/stats/detail/StatsSummary.kt (1)
16-27:EMPTY의startDate에LocalDate.now()사용 — 날짜가 고정될 수 있습니다
companion object의val EMPTY는 JVM에서 클래스가 처음 로드될 때 딱 한 번만 초기화됩니다. 즉,LocalDate.now()는 앱 실행 시점의 날짜로 고정되고, 이후에EMPTY를 참조해도 날짜가 갱신되지 않습니다.이는 테스트 환경에서 날짜에 의존하는 단위 테스트를 작성할 때 문제가 되거나, 자정을 넘어 앱이 실행 중인 경우 예기치 않은 동작을 일으킬 수 있습니다.
EMPTY의 의도가 "빈 기본값(sentinel)"이라면, 아래 두 가지 방식 중 하나를 권장합니다:♻️ 개선 방법
방법 1: 고정된 sentinel 날짜 사용 (불변, 결정적)
-startDate = LocalDate.now(), +startDate = LocalDate.of(1970, 1, 1),방법 2:
fun empty()팩토리 함수로 변경 (매번 새 인스턴스 생성)-companion object { - val EMPTY = - StatsSummary( - myNickname = "", - partnerNickname = "", - totalCount = 0, - myCompletedCount = 0, - partnerCompletedCount = 0, - repeatCycle = RepeatCycle.DAILY, - startDate = LocalDate.now(), - endDate = null, - ) -} +companion object { + fun empty() = StatsSummary( + myNickname = "", + partnerNickname = "", + totalCount = 0, + myCompletedCount = 0, + partnerCompletedCount = 0, + repeatCycle = RepeatCycle.DAILY, + startDate = LocalDate.now(), + endDate = null, + ) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/src/main/java/com/twix/domain/model/stats/detail/StatsSummary.kt` around lines 16 - 27, The EMPTY companion val in StatsSummary currently sets startDate = LocalDate.now() at class-load time which freezes the date; change this to a safe sentinel by either (A) using a fixed constant date (e.g., LocalDate.MIN or a clearly documented constant) for EMPTY.startDate, or (B) convert EMPTY into a factory function fun empty(): StatsSummary that constructs a new instance with startDate = LocalDate.now() each call; update the companion object to replace val EMPTY with your chosen approach and ensure callers use StatsSummary.EMPTY or StatsSummary.empty() accordingly.core/design-system/src/main/java/com/twix/designsystem/components/stats/PictureDayCell.kt (1)
41-42:borderColor의else분기가 사용되지 않는 코드입니다
borderColor는if (showBackgroundCard)블록 안에서만 사용되므로,showBackgroundCard가false일 때 계산되는GrayColor.C400은 실제로 참조되지 않는 데드 코드입니다.현재 구조를 보면
hasImage는true이지만showBackgroundCard는false인 경우(이미지가 하나만 있는 경우)에도 테두리가 전혀 표시되지 않습니다. 이것이 의도된 디자인이라면 변수를 단순화하거나 인라인하는 게 더 명확합니다. 만약 단일 이미지 케이스에도 테두리를 보여주려는 의도였다면 Modifier 조건을 수정해야 합니다.♻️ 의도가 "백그라운드 카드일 때만 흰색 테두리"라면 단순화 가능
-val borderColor = if (showBackgroundCard) CommonColor.White else GrayColor.C400 ... .then( if (showBackgroundCard) { - Modifier.border(1.dp, borderColor, cornerShape) + Modifier.border(1.dp, CommonColor.White, cornerShape) } else { Modifier }, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/stats/PictureDayCell.kt` around lines 41 - 42, The else branch for borderColor is dead because borderColor is only used when showBackgroundCard is true; locate the borderColor declaration and its usage (borderColor, showBackgroundCard, hasImage in PictureDayCell) and either inline/simplify by setting borderColor = CommonColor.White only when showBackgroundCard is true (removing the GrayColor.C400 else) or change the Modifier condition so the border is applied when you also want a border for single-image cases (e.g., use hasImage || showBackgroundCard to decide on the border color) — pick the approach matching the intended design and adjust the border usage accordingly.domain/src/main/java/com/twix/domain/model/stats/detail/StatsDetail.kt (1)
15-26:EMPTY의monthDate에LocalDate.now()사용 시 주의가 필요합니다.
companion object의val EMPTY는 클래스 로드 시점에 한 번만 평가됩니다. 따라서LocalDate.now()가 클래스 최초 접근 시점의 날짜로 고정되어, 이후에는 항상 같은 값을 반환합니다.왜 문제인가:
- 테스트에서 비결정적(non-deterministic) 동작을 유발할 수 있습니다.
- 앱이 자정을 넘겨 실행되더라도 EMPTY의 날짜는 갱신되지 않습니다.
개선 방향: 고정된 sentinel 값(예:
LocalDate.MIN또는LocalDate.of(1970, 1, 1))을 사용하거나, EMPTY 대신 팩토리 함수를 제공하는 방법을 고려해 보시면 어떨까요?💡 예시
val EMPTY = StatsDetail( goalId = -1, goalName = "", goalIcon = GoalIconType.DEFAULT, status = "", - monthDate = LocalDate.now(), + monthDate = LocalDate.MIN, completedDate = emptyList(), statsSummary = StatsSummary.EMPTY, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@domain/src/main/java/com/twix/domain/model/stats/detail/StatsDetail.kt` around lines 15 - 26, The companion object singleton EMPTY in StatsDetail currently sets monthDate = LocalDate.now(), which is evaluated once at class load and causes non-deterministic/frozen dates; change EMPTY to use a fixed sentinel date (e.g., LocalDate.MIN or LocalDate.of(1970,1,1)) for the monthDate field, or instead remove the singleton and add a factory (e.g., a function like StatsDetail.empty() or StatsDetail.withDefaultDate()) that returns a new instance using LocalDate.now() when needed; update references to StatsDetail.EMPTY accordingly if you choose the factory approach.feature/main/src/main/java/com/twix/stats/component/InProgressStatsContent.kt (1)
63-73:EndStatsContent와 동일한 패턴으로 클릭 처리가 일관되게 구현되어 있습니다.named parameter 사용과
noRippleClickablemodifier 적용이 깔끔합니다. Preview에도 새 파라미터가 반영되어 있어 좋습니다. 👍한 가지 사소한 제안으로,
items블록의 trailing lambda에서it을 사용하고 있는데,it.goalId가 클릭 람다 안에서도 쓰이다 보니 가독성 측면에서 named parameter(예:statsGoal ->)를 고려해볼 수 있습니다. 이 부분은EndStatsContent에서도 동일하게 적용됩니다.✨ 가독성 개선 예시 (optional)
items( items = stats.statsGoals, key = { it.goalId }, - ) { + ) { statsGoal -> StatsGoalCard( - statsGoal = it, + statsGoal = statsGoal, showStamp = true, modifier = Modifier - .noRippleClickable(onClick = { onClickStatsCard(it.goalId) }), + .noRippleClickable(onClick = { onClickStatsCard(statsGoal.goalId) }), ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/main/src/main/java/com/twix/stats/component/InProgressStatsContent.kt` around lines 63 - 73, The trailing lambda for items(items = stats.statsGoals, key = { it.goalId }) uses the implicit it which reduces readability when reused inside the click lambda; change the lambda signature to a named parameter (e.g., statsGoal ->) in the items block and update usages inside the block (StatsGoalCard(statsGoal = statsGoal, ... .noRippleClickable(onClick = { onClickStatsCard(statsGoal.goalId) }))) and apply the same change in EndStatsContent so both components consistently use a named parameter instead of implicit it.feature/stats/detail/src/main/java/com/yapp/detail/navigation/StatsDetailGraph.kt (1)
12-38: 네비게이션 그래프 등록 구조가 적절합니다.단일 화면 feature 모듈에서
navigation()블록 없이composable()을 직접 등록하는 방식은 심플하고 효과적입니다. nav argument 정의도ARG_GOAL_ID(LongType)과ARG_DATE(StringType, nullable)로 명확하게 구성되어 있습니다.다만, 앞서
StatsDetailRoute.kt에서 언급한 것처럼 nav argument를 composable 블록 내에서 추출하여StatsDetailRoute에 전달하는 부분이 누락되어 있습니다. 현재backStackEntry(it)의 arguments에서 goalId와 date를 꺼내지 않고 있으므로, 화면 구현 시 반드시 추가해주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/yapp/detail/navigation/StatsDetailGraph.kt` around lines 12 - 38, The composable registration in StatsDetailGraph is missing extraction of nav arguments from the backStackEntry and passing them into StatsDetailRoute; update the composable lambda to read val goalId = it.arguments?.getLong(NavRoutes.StatsDetailRoute.ARG_GOAL_ID) and val date = it.arguments?.getString(NavRoutes.StatsDetailRoute.ARG_DATE) (handling nullable date and absent goalId appropriately), then pass these values into StatsDetailRoute along with onBack = navController::popBackStack so StatsDetailRoute receives the expected goalId and date.feature/stats/detail/src/main/java/com/yapp/detail/StatsDetailRoute.kt (1)
5-9: 네비게이션 인자(goalId, date)가 Route에 전달되지 않고 있습니다.
StatsDetailGraph에서ARG_GOAL_ID(Long)과ARG_DATE(String?)를 네비게이션 인자로 정의하고 있지만,StatsDetailRoute에는onBack콜백만 전달되고 있습니다. 초기 세팅이라 빈 상태인 것은 이해하지만, 이후 ViewModel이나 화면 구성 시 이 인자들이 반드시 필요하므로 지금부터 파라미터로 받아두면 나중에 누락 없이 연결할 수 있습니다.
StatsDetailGraph.kt의 composable 블록에서backStackEntry를 통해 인자를 추출하고 Route에 전달하는 패턴을 미리 준비해두시면 좋겠습니다.💡 제안하는 방향
StatsDetailRoute시그니처 확장:`@Composable` fun StatsDetailRoute( + goalId: Long, + date: String?, onBack: () -> Unit, ) { }
StatsDetailGraph.kt에서 인자 전달:) { + val goalId = it.arguments?.getLong(NavRoutes.StatsDetailRoute.ARG_GOAL_ID) ?: return@composable + val date = it.arguments?.getString(NavRoutes.StatsDetailRoute.ARG_DATE) StatsDetailRoute( + goalId = goalId, + date = date, onBack = navController::popBackStack, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/yapp/detail/StatsDetailRoute.kt` around lines 5 - 9, StatsDetailRoute currently only accepts onBack but must also receive the navigation args: add parameters goalId: Long and date: String? to the StatsDetailRoute signature and use them in the UI/ViewModel setup; in the composable that registers the route (StatsDetailGraph) extract ARG_GOAL_ID and ARG_DATE from backStackEntry (using the same keys ARG_GOAL_ID and ARG_DATE) and pass those values into StatsDetailRoute when calling it so the route receives the required arguments for future ViewModel/screen wiring.feature/stats/detail/build.gradle.kts (1)
5-6: Android namespace와 소스 파일의 package를 정렬하세요.현재 빌드 파일의 namespace는
com.yapp.stats.detail로 설정되어 있지만, 모든 소스 파일의 package 선언은com.yapp.detail형태입니다 (예:StatsDetailRoute.kt,StatsDetailGraph.kt,StatsDetailModule.kt등이 모두com.yapp.detail.*패키지 사용).namespace와 package명이 일치하지 않으면 R 클래스 참조 시 어느 네임스페이스에서 자동 생성되는지 혼란을 줄 수 있고, 모듈 간 패키지 충돌 가능성도 있습니다. 다음 중 하나로 정렬하는 것을 권장합니다:
- Option 1: namespace를
com.yapp.detail로 수정- Option 2: 모든 소스 파일의 package를
com.yapp.stats.detail.*로 통일특히 모듈 경로가
feature/stats/detail이므로, Option 2로 package를com.yapp.stats.detail로 통일하면 모듈 구조와의 일관성도 높아집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/build.gradle.kts` around lines 5 - 6, The Android module namespace in build.gradle.kts is com.yapp.stats.detail but the Kotlin source files (e.g., StatsDetailRoute.kt, StatsDetailGraph.kt, StatsDetailModule.kt) declare package com.yapp.detail; align them by either changing the namespace value in the android { namespace = "..."} to com.yapp.detail or (preferred for module path consistency) update the package declarations in those source files to com.yapp.stats.detail so package and namespace match across the module.feature/stats/detail/src/main/java/com/yapp/detail/component/StatsDetailTopbar.kt (1)
40-41:onClickAction콜백이 두 가지 서로 다른 역할을 담당합니다.
isInProgressStatsDetail = true일 때는 팝업 토글,false일 때는 삭제 액션으로 사용됩니다. 동일한 콜백이 상황에 따라 다른 역할을 하면 ViewModel의 Intent 처리 시 어떤 동작을 해야 하는지 구분하기 어렵고, 향후 요구사항 변화 시 혼란을 초래할 수 있습니다.
onTogglePopupMenu: () -> Unit와onClickDelete: () -> Unit처럼 명시적으로 분리하는 것을 고려해보세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/yapp/detail/component/StatsDetailTopbar.kt` around lines 40 - 41, The onClickAction callback in StatsDetailTopbar is being used for two different responsibilities depending on isInProgressStatsDetail (toggling a popup vs performing delete), which makes intent handling ambiguous; replace the single onClickAction with two explicit callbacks (e.g., onTogglePopupMenu: () -> Unit and onClickDelete: () -> Unit) in the StatsDetailTopbar signature and body (use onTogglePopupMenu when isInProgressStatsDetail is true, onClickDelete when false), then update all call sites and the ViewModel/intent wiring to call the correct new callback so each action has a single clear handler.feature/stats/detail/src/main/java/com/yapp/detail/preview/StatsDetailUiStatePreviewProvider.kt (1)
13-74:LocalDate.now()사용으로 프리뷰 데이터가 매일 달라집니다.
LocalDate.now()를 기반으로 날짜를 계산하면 프리뷰가 날마다 다른 결과를 보여줍니다. 특히 캘린더 UI처럼 날짜에 민감한 컴포넌트에서는 고정된 날짜를 사용하는 것이 재현 가능하고 안정적인 프리뷰를 보장합니다.+private val fixedDate = LocalDate.of(2026, 2, 1) private val completedDates = listOf( CompletedDate( - date = LocalDate.now().minusDays(1), + date = fixedDate.minusDays(1), ... ), ... )또한 이 클래스는 프리뷰 전용이므로 모듈 외부에서 불필요하게 노출되지 않도록
internal로 선언하는 것도 고려해보세요.-class StatsDetailUiStatePreviewProvider : PreviewParameterProvider<StatsDetailUiState> { +internal class StatsDetailUiStatePreviewProvider : PreviewParameterProvider<StatsDetailUiState> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/yapp/detail/preview/StatsDetailUiStatePreviewProvider.kt` around lines 13 - 74, The preview uses LocalDate.now() in many places (completedDates, baseDetail.monthDate/startDate, baseCalendarUiModel.currentDate, StatsDetailUiState.selectedDate) which makes previews non-deterministic; replace all LocalDate.now() usages in StatsDetailUiStatePreviewProvider with a fixed constant date (e.g., val fixedDate = LocalDate.of(2025, 1, 15)) and compute minusDays/minusMonths from that fixedDate for completedDates, baseDetail.monthDate/startDate, baseCalendarUiModel creation and the selectedDate values, and also change the class declaration of StatsDetailUiStatePreviewProvider to internal so the preview provider is not exposed outside the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/yapp/twix/di/FeatureModules.kt`:
- Line 12: The import for statsDetailModule is using the wrong package
namespace; update the import in FeatureModules.kt to reference the module's
actual package com.yapp.stats.detail.di and ensure the statsDetailModule symbol
is imported from there so the DI package mirrors the project's namespace
convention (replace import com.yapp.detail.di.statsDetailModule with import
com.yapp.stats.detail.di.statsDetailModule).
In
`@core/design-system/src/main/java/com/twix/designsystem/components/stats/PictureDayCell.kt`:
- Around line 31-37: PictureDayCell에 Preview가 빠져 있으니 PictureDayCell을 호출하는
`@Preview` Composable을 추가하세요; 예를 들어 PictureDayCellPreview라는
`@Preview`(showBackground = true) 함수 내에서 TwixTheme로 감싸고 Row로 여러 상태(완료 없음,
myImageUrl만 있는 CompletedDate 등)를 렌더링하여 date, completed, onDateSelected, modifier
인자를 전달해 미리보기를 확인할 수 있게 만드세요; Preview 이름과 내부 케이스는 PictureDayCell, CompletedDate
같은 식별자를 사용해 쉽게 찾을 수 있게 구현하세요.
- Around line 45-49: The Box in PictureDayCell is clickable via
noRippleClickable but lacks accessibility semantics; update PictureDayCell to
add semantics on the clickable container (use
androidx.compose.ui.semantics.semantics and set contentDescription with the date
label and semantics { this.role = Role.Button } or semantics {
contentDescription = ...; role = Role.Button }) so TalkBack announces which date
and that it is clickable, and import androidx.compose.ui.semantics.semantics and
androidx.compose.ui.semantics.Role accordingly.
In
`@core/design-system/src/main/java/com/twix/designsystem/components/stats/StatsCalendar.kt`:
- Around line 41-64: The layout misalignment comes from inconsistent Modifiers
inside the Row: PictureDayCell uses Modifier.weight(1f) while null placeholders
(Box with Modifier.size(44.dp)) and trailing empty boxes use fixed size, so
cells get different widths; update the null branch in the uiModel.weeks.forEach
loop and the trailing repeat block to use Box(modifier = Modifier.weight(1f))
(matching PictureDayCell’s Modifier.weight(1f)) so every column shares equal
width; touch the code around PictureDayCell, the null placeholder Box, and the
trailing repeat to ensure all calendar cells use Modifier.weight(1f).
In
`@feature/stats/detail/src/main/java/com/yapp/detail/component/StatsDetailTopbar.kt`:
- Around line 91-99: The Image in StatsDetailTopbar is interactive but sets
contentDescription = null, which breaks accessibility; change it to provide a
meaningful description (preferably via stringResource) and wire it into the
Image's contentDescription (e.g., a "more options" or "open menu" label) so
TalkBack users know the control's purpose; locate the Image
painterResource(R.drawable.ic_meatball) with Modifier.noRippleClickable(onClick
= onClickAction) and replace the null description with an appropriate
stringResource reference.
- Around line 50-58: In StatsDetailTopbar, the Image composable uses a hardcoded
contentDescription and has the click modifier applied after size, making the
clickable area only 24×24 dp; replace the hardcoded string with a string
resource via stringResource(R.string.<appropriate_back>) for accessibility and
localization, and move the noRippleClickable(onClick = onBack) modifier to come
before padding (i.e., apply clickable then padding/size) so the touch target
covers the full padded area (meeting the 48dp minimum).
- Around line 101-103: The popup anchor currently uses hardcoded pixel offsets
(IntOffset(x = -100, y = 140)) which vary with screen density; in
StatsDetailTopbar where CommonPopup is called (visible = popupMenuVisibility,
anchorOffset = ...), replace the literal IntOffset with values converted from Dp
to pixels using LocalDensity.current (e.g. convert Dp(-100).dp and 140.dp via
LocalDensity.current.run { dp.toPx().roundToInt() } or equivalent) so the
anchorOffset uses density-correct IntOffset; ensure you import
androidx.compose.ui.unit.dp and kotlin.math.roundToInt and perform the
conversion in a composable scope before passing anchorOffset to CommonPopup.
In
`@feature/stats/detail/src/main/java/com/yapp/detail/contract/StatsDetailUiState.kt`:
- Around line 15-16: The computed property isInProgressStatsDetail (based on
selectedDate) can be false during first composition because selectedDate
defaults to null, causing StatsDetailTopbar to briefly show the wrong action;
instead add an explicit boolean (e.g., isInProgress) to the State or derive the
value from detail.status inside StatsDetailUiState (replace selectedDate != null
usage), and update the ViewModel to initialize and emit State with that explicit
in-progress value (or compute it from detail.status) so the top bar consistently
shows the correct UI from first composition.
---
Outside diff comments:
In
`@core/design-system/src/main/java/com/twix/designsystem/components/calendar/CalendarNavigator.kt`:
- Around line 40-49: The left and right arrow Image composables in
CalendarNavigator.kt use noRippleClickable unconditionally so tapping a visually
disabled arrow still calls onPreviousMonth/onNextMonth; update the modifier
application to only attach the clickable when hasPrevious/hasNext is true (or
wrap the callback to a no-op when false) so the onPreviousMonth and onNextMonth
callbacks are not invoked for disabled arrows, and keep the existing tint logic
(GrayColor.C200 when disabled) unchanged; apply the same change for the hasNext
branch (lines referencing onPreviousMonth, onNextMonth, hasPrevious, hasNext,
and the Image modifiers).
- Around line 40-48: Update the Image composables used for month navigation (the
ones using painterResource(R.drawable.ic_arrow_m_left)/ic_arrow_m_right and
modifiers with
noRippleClickable(onPreviousMonth)/noRippleClickable(onNextMonth)) so their
contentDescription reflects disabled state instead of a fixed "previous
month"/"next month": conditionally set contentDescription to include "disabled"
(e.g., "previous month, disabled") or set it to null and add a semantics
modifier marking the element as disabled when hasPrevious/hasNext is false, and
ensure the semantics/role reflects non-clickable when disabled so accessibility
services announce the correct state.
---
Nitpick comments:
In
`@core/design-system/src/main/java/com/twix/designsystem/components/stats/PictureDayCell.kt`:
- Around line 41-42: The else branch for borderColor is dead because borderColor
is only used when showBackgroundCard is true; locate the borderColor declaration
and its usage (borderColor, showBackgroundCard, hasImage in PictureDayCell) and
either inline/simplify by setting borderColor = CommonColor.White only when
showBackgroundCard is true (removing the GrayColor.C400 else) or change the
Modifier condition so the border is applied when you also want a border for
single-image cases (e.g., use hasImage || showBackgroundCard to decide on the
border color) — pick the approach matching the intended design and adjust the
border usage accordingly.
In `@domain/src/main/java/com/twix/domain/model/stats/detail/StatsDetail.kt`:
- Around line 15-26: The companion object singleton EMPTY in StatsDetail
currently sets monthDate = LocalDate.now(), which is evaluated once at class
load and causes non-deterministic/frozen dates; change EMPTY to use a fixed
sentinel date (e.g., LocalDate.MIN or LocalDate.of(1970,1,1)) for the monthDate
field, or instead remove the singleton and add a factory (e.g., a function like
StatsDetail.empty() or StatsDetail.withDefaultDate()) that returns a new
instance using LocalDate.now() when needed; update references to
StatsDetail.EMPTY accordingly if you choose the factory approach.
In `@domain/src/main/java/com/twix/domain/model/stats/detail/StatsSummary.kt`:
- Around line 16-27: The EMPTY companion val in StatsSummary currently sets
startDate = LocalDate.now() at class-load time which freezes the date; change
this to a safe sentinel by either (A) using a fixed constant date (e.g.,
LocalDate.MIN or a clearly documented constant) for EMPTY.startDate, or (B)
convert EMPTY into a factory function fun empty(): StatsSummary that constructs
a new instance with startDate = LocalDate.now() each call; update the companion
object to replace val EMPTY with your chosen approach and ensure callers use
StatsSummary.EMPTY or StatsSummary.empty() accordingly.
In
`@feature/main/src/main/java/com/twix/stats/component/InProgressStatsContent.kt`:
- Around line 63-73: The trailing lambda for items(items = stats.statsGoals, key
= { it.goalId }) uses the implicit it which reduces readability when reused
inside the click lambda; change the lambda signature to a named parameter (e.g.,
statsGoal ->) in the items block and update usages inside the block
(StatsGoalCard(statsGoal = statsGoal, ... .noRippleClickable(onClick = {
onClickStatsCard(statsGoal.goalId) }))) and apply the same change in
EndStatsContent so both components consistently use a named parameter instead of
implicit it.
In `@feature/stats/detail/build.gradle.kts`:
- Around line 5-6: The Android module namespace in build.gradle.kts is
com.yapp.stats.detail but the Kotlin source files (e.g., StatsDetailRoute.kt,
StatsDetailGraph.kt, StatsDetailModule.kt) declare package com.yapp.detail;
align them by either changing the namespace value in the android { namespace =
"..."} to com.yapp.detail or (preferred for module path consistency) update the
package declarations in those source files to com.yapp.stats.detail so package
and namespace match across the module.
In
`@feature/stats/detail/src/main/java/com/yapp/detail/component/StatsDetailTopbar.kt`:
- Around line 40-41: The onClickAction callback in StatsDetailTopbar is being
used for two different responsibilities depending on isInProgressStatsDetail
(toggling a popup vs performing delete), which makes intent handling ambiguous;
replace the single onClickAction with two explicit callbacks (e.g.,
onTogglePopupMenu: () -> Unit and onClickDelete: () -> Unit) in the
StatsDetailTopbar signature and body (use onTogglePopupMenu when
isInProgressStatsDetail is true, onClickDelete when false), then update all call
sites and the ViewModel/intent wiring to call the correct new callback so each
action has a single clear handler.
In
`@feature/stats/detail/src/main/java/com/yapp/detail/contract/StatsDetailIntent.kt`:
- Line 5: The sealed interface StatsDetailIntent is currently empty—define
explicit user-action intents to represent calendar interactions (e.g., month
navigation and date selection) so the MVI contract is clear: add implementations
under StatsDetailIntent such as objects for NavigateToPreviousMonth and
NavigateToNextMonth and a data class (e.g., DateSelected) carrying the selected
LocalDate (or equivalent) and optionally an Initialize/Load intent for initial
screen load; update any callers (view/event handlers) to emit these specific
intents instead of raw events.
In
`@feature/stats/detail/src/main/java/com/yapp/detail/contract/StatsDetailSideEffect.kt`:
- Around line 6-12: ShowToast의 message가 Int로 선언되어 있어 리소스 ID가 아닌 임의의 정수가 들어갈 수
있으니 타입 안정성을 강화해 주세요: 의도대로 문자열 리소스 ID를 전달하려면 StatsDetailSideEffect.ShowToast의
message에 `@StringRes` 어노테이션을 추가해 컴파일 시 검증되게 하거나, 리소스를 호출한 쪽에서 문자열을 resolve하도록 하고
message 타입을 String으로 변경해 호출 책임을 명확히 하세요; 변경 대상는 sealed interface
StatsDetailSideEffect 내 data class ShowToast의 message 프로퍼티입니다.
In
`@feature/stats/detail/src/main/java/com/yapp/detail/navigation/StatsDetailGraph.kt`:
- Around line 12-38: The composable registration in StatsDetailGraph is missing
extraction of nav arguments from the backStackEntry and passing them into
StatsDetailRoute; update the composable lambda to read val goalId =
it.arguments?.getLong(NavRoutes.StatsDetailRoute.ARG_GOAL_ID) and val date =
it.arguments?.getString(NavRoutes.StatsDetailRoute.ARG_DATE) (handling nullable
date and absent goalId appropriately), then pass these values into
StatsDetailRoute along with onBack = navController::popBackStack so
StatsDetailRoute receives the expected goalId and date.
In
`@feature/stats/detail/src/main/java/com/yapp/detail/preview/StatsDetailUiStatePreviewProvider.kt`:
- Around line 13-74: The preview uses LocalDate.now() in many places
(completedDates, baseDetail.monthDate/startDate,
baseCalendarUiModel.currentDate, StatsDetailUiState.selectedDate) which makes
previews non-deterministic; replace all LocalDate.now() usages in
StatsDetailUiStatePreviewProvider with a fixed constant date (e.g., val
fixedDate = LocalDate.of(2025, 1, 15)) and compute minusDays/minusMonths from
that fixedDate for completedDates, baseDetail.monthDate/startDate,
baseCalendarUiModel creation and the selectedDate values, and also change the
class declaration of StatsDetailUiStatePreviewProvider to internal so the
preview provider is not exposed outside the module.
In `@feature/stats/detail/src/main/java/com/yapp/detail/StatsDetailRoute.kt`:
- Around line 5-9: StatsDetailRoute currently only accepts onBack but must also
receive the navigation args: add parameters goalId: Long and date: String? to
the StatsDetailRoute signature and use them in the UI/ViewModel setup; in the
composable that registers the route (StatsDetailGraph) extract ARG_GOAL_ID and
ARG_DATE from backStackEntry (using the same keys ARG_GOAL_ID and ARG_DATE) and
pass those values into StatsDetailRoute when calling it so the route receives
the required arguments for future ViewModel/screen wiring.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
app/build.gradle.ktsapp/src/main/java/com/yapp/twix/di/FeatureModules.ktcore/design-system/src/main/java/com/twix/designsystem/components/calendar/CalendarNavigator.ktcore/design-system/src/main/java/com/twix/designsystem/components/stats/PictureDayCell.ktcore/design-system/src/main/java/com/twix/designsystem/components/stats/StatsCalendar.ktcore/design-system/src/main/java/com/twix/designsystem/components/stats/model/StatsCalendarUiModel.ktcore/navigation/src/main/java/com/twix/navigation/NavRoutes.ktdata/src/main/java/com/twix/data/repository/FakeStatsRepository.ktdomain/src/main/java/com/twix/domain/model/stats/detail/CompletedDate.ktdomain/src/main/java/com/twix/domain/model/stats/detail/StatsDetail.ktdomain/src/main/java/com/twix/domain/model/stats/detail/StatsSummary.ktdomain/src/main/java/com/twix/domain/repository/StatsRepository.ktfeature/main/src/main/java/com/twix/main/MainScreen.ktfeature/main/src/main/java/com/twix/main/navigation/MainNavGraph.ktfeature/main/src/main/java/com/twix/stats/StatsScreen.ktfeature/main/src/main/java/com/twix/stats/component/EndStatsContent.ktfeature/main/src/main/java/com/twix/stats/component/InProgressStatsContent.ktfeature/stats/detail/.gitignorefeature/stats/detail/build.gradle.ktsfeature/stats/detail/consumer-rules.profeature/stats/detail/proguard-rules.profeature/stats/detail/src/main/AndroidManifest.xmlfeature/stats/detail/src/main/java/com/yapp/detail/StatsDetailRoute.ktfeature/stats/detail/src/main/java/com/yapp/detail/component/StatsDetailTopbar.ktfeature/stats/detail/src/main/java/com/yapp/detail/contract/StatsDetailIntent.ktfeature/stats/detail/src/main/java/com/yapp/detail/contract/StatsDetailSideEffect.ktfeature/stats/detail/src/main/java/com/yapp/detail/contract/StatsDetailUiState.ktfeature/stats/detail/src/main/java/com/yapp/detail/di/StatsDetailModule.ktfeature/stats/detail/src/main/java/com/yapp/detail/navigation/StatsDetailGraph.ktfeature/stats/detail/src/main/java/com/yapp/detail/preview/StatsDetailUiStatePreviewProvider.ktsettings.gradle.kts
core/design-system/src/main/java/com/twix/designsystem/components/stats/PictureDayCell.kt
Show resolved
Hide resolved
core/design-system/src/main/java/com/twix/designsystem/components/stats/PictureDayCell.kt
Show resolved
Hide resolved
core/design-system/src/main/java/com/twix/designsystem/components/stats/StatsCalendar.kt
Show resolved
Hide resolved
feature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.kt
Show resolved
Hide resolved
feature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.kt
Show resolved
Hide resolved
feature/stats/detail/src/main/java/com/yapp/detail/component/StatsDetailTopbar.kt
Outdated
Show resolved
Hide resolved
feature/stats/detail/src/main/java/com/yapp/stats/detail/contract/StatsDetailUiState.kt
Show resolved
Hide resolved
- 고정된 `44.dp` 크기를 사용하는 대신 `weight(1f)`를 적용하여 유연하게 채워지도록 수정
- `LocalDensity`를 사용하여 dp 단위를 px로 변환하도록 수정 - `Column`을 `Box`로 변경하고 `contentAlignment`를 사용하여 중앙 정렬
- `com.yapp.detail.navigation.StatsDetailGraph`에서 `com.yapp.stats.detail.navigation.StatsDetailGraph`로 잘못된 임포트 경로를 수정
- `isEnd` 상태에 따라 시작 날짜와 종료 날짜를 다르게 설정 - 종료된 챌린지의 경우 월간 목표 횟수를 15로 수정 - 시작일과 종료일을 고정된 날짜로 설정하여 테스트 데이터 일관성 유지
- 이전/다음 달로 이동이 가능한지 확인하기 위해 `hasNext`와 `hasPrevious` 속성을 추가 - 기존 `isInProgressStatsDetail` 로직을 `selectedDate` null 체크에서 초기 로딩 상태를 나타내는 boolean 값으로 변경
# Conflicts: # feature/main/src/main/java/com/twix/stats/StatsScreen.kt
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
dogmania
requested changes
Feb 25, 2026
| uiModel.weeks.forEach { week -> | ||
| Row( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| horizontalArrangement = Arrangement.SpaceAround, |
Member
There was a problem hiding this comment.
이미 안쪽에서 weight(1f)를 주고 있어서 이 옵션은 제거해도 될 거 같아요!
Comment on lines
81
to
85
| ImageRequest | ||
| .Builder(LocalContext.current) | ||
| .data(displayImageUrl) | ||
| .crossfade(true) | ||
| .build(), |
Member
There was a problem hiding this comment.
이 부분은 따로 remember로 감싸서 재구성될 때마다 불필요하게 객체 생성하지 않도록 수정할 수 있을 거 같아요!
Member
There was a problem hiding this comment.
이게 지금 프리뷰로 봤을 때 팝업 배경에 튀어나온 흰색 부분이 보이는데 확인 부탁드려요!
Member
Author
There was a problem hiding this comment.
실제 UI에선 흰색 부분이 안 보이는 것 같아 궁금해서 팝업 사용하는 곳을 프리뷰로 확인해봤는데
동일한 현상이 발생하는 것 같아 ! 프리뷰에서만 그러는 것 같은데 왜일까 ... 😅
| } | ||
|
|
||
| android { | ||
| namespace = "com.yapp.stats.detail" |
Member
There was a problem hiding this comment.
다른 feature들은 twix로 되어있는데 여기만 yapp이라서 수정해주세요!
| PictureDayCell( | ||
| date = date, | ||
| completed = uiModel.completedDateMap[date], | ||
| onDateSelected = { onSelectedDate(it) }, |
- `remember`를 사용하여 `ImageRequest`를 캐싱함으로써 불필요한 재생성을 방지
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- `com.yapp.stats.detail` 패키지 이름을 `com.twix.stats.detail`로 변경
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
feature/stats/detail/src/main/java/com/twix/stats/detail/di/StatsDetailModule.kt (1)
11-11:⚠️ Potential issue | 🟡 MinorNavGraph qualifier를 Graph 기준으로 통일해 주세요.
왜 문제인가: Line 11에서
StatsDetailRoute.route로 qualifier를 등록하면,StatsDetailGraph기준으로 조회/확장하는 흐름과 불일치가 생길 수 있습니다.
어떻게 개선하나: qualifier를StatsDetailGraph.graphRoute(또는NavRoutes.StatsDetailGraph.route)로 맞춰 일관성을 유지해 주세요.수정 예시
- single<NavGraphContributor>(named(NavRoutes.StatsDetailRoute.route)) { StatsDetailGraph } + single<NavGraphContributor>(named(StatsDetailGraph.graphRoute)) { StatsDetailGraph }As per coding guidelines,
3. 의존성 주입 - Koin을 통한 DI가 책임에 맞게 구성되었는가?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/twix/stats/detail/di/StatsDetailModule.kt` at line 11, The DI qualifier is registered using NavRoutes.StatsDetailRoute.route but the contributor is StatsDetailGraph; change the qualifier to the graph-based route to keep lookups consistent (use StatsDetailGraph.graphRoute or NavRoutes.StatsDetailGraph.route) in the single<NavGraphContributor> registration so the provider and consumer use the same unique identifier (update the registration that currently references NavRoutes.StatsDetailRoute.route to reference the StatsDetailGraph route symbol instead).feature/stats/detail/src/main/java/com/twix/stats/detail/navigation/StatsDetailGraph.kt (1)
19-38:⚠️ Potential issue | 🟠 Major중첩 그래프 래핑이 없어
graphRoute/startDestination선언 의도가 코드에 반영되지 않습니다.왜 문제인가요: 현재 구조는 목적지 하나만 직접 등록하고 있어
NavGraphContributor로 선언한 그래프 경계가 약해집니다. 결과적으로 백스택/그래프 단위 관리가 기대와 달라질 수 있습니다.
어떻게 개선할까요: 기존composable등록을navigation(route, startDestination)내부로 감싸 중첩 그래프로 명확히 등록해 주세요. 이 방식이면 선언한graphRoute,startDestination가 실제 동작에 연결됩니다.🔧 제안 코드
import androidx.navigation.NavGraphBuilder import androidx.navigation.NavHostController import androidx.navigation.NavType import androidx.navigation.compose.composable +import androidx.navigation.compose.navigation import androidx.navigation.navArgument import com.twix.navigation.NavRoutes import com.twix.navigation.base.NavGraphContributor import com.twix.stats.detail.StatsDetailRoute @@ override fun NavGraphBuilder.registerGraph(navController: NavHostController) { - composable( - route = NavRoutes.StatsDetailRoute.route, - arguments = - listOf( - navArgument(NavRoutes.StatsDetailRoute.ARG_GOAL_ID) { - type = NavType.LongType - }, - navArgument(NavRoutes.StatsDetailRoute.ARG_DATE) { - type = NavType.StringType - nullable = true - defaultValue = null - }, - ), - ) { - StatsDetailRoute( - onBack = navController::popBackStack, - ) + navigation( + route = graphRoute.route, + startDestination = startDestination, + ) { + composable( + route = NavRoutes.StatsDetailRoute.route, + arguments = + listOf( + navArgument(NavRoutes.StatsDetailRoute.ARG_GOAL_ID) { + type = NavType.LongType + }, + navArgument(NavRoutes.StatsDetailRoute.ARG_DATE) { + type = NavType.StringType + nullable = true + defaultValue = null + }, + ), + ) { + StatsDetailRoute( + onBack = navController::popBackStack, + ) + } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/twix/stats/detail/navigation/StatsDetailGraph.kt` around lines 19 - 38, The current registerGraph implementation registers a single composable directly so the declared graph boundary (graphRoute/startDestination) from NavRoutes.StatsDetailRoute is not applied; wrap the existing composable(...) call inside a navigation(route = NavRoutes.StatsDetailRoute.graphRoute, startDestination = NavRoutes.StatsDetailRoute.startDestination) block so the NavGraphBuilder.registerGraph creates a nested graph; keep the existing composable that calls StatsDetailRoute(onBack = navController::popBackStack) unchanged but move it into the navigation { ... } closure to ensure graph-level routing and backstack behavior are honored.feature/stats/detail/src/main/java/com/twix/stats/detail/StatsDetailRoute.kt (1)
5-7:⚠️ Potential issue | 🟠 Major빈 Route 구현으로 상세 화면 진입 시 아무것도 렌더링되지 않습니다.
왜 문제인가요: 네비게이션은 연결되어도 실제 UI가 없어 사용자 입장에서는 “고장난 화면”처럼 보입니다.
어떻게 개선할까요: 최소한StatsDetailScreen을 연결하거나, 준비 중이라면 임시 플레이스홀더라도 렌더링해 주세요. 지금 단계에서 의도적으로 비워둔 상태라면 연결 시점을 TODO로 남겨두는 방식이 좋겠습니다.🔧 제안 코드
import androidx.compose.runtime.Composable `@Composable` fun StatsDetailRoute(onBack: () -> Unit) { + // TODO: 실제 StatsDetailScreen 연결 전까지 임시 UI라도 렌더링 + StatsDetailScreen( + onBack = onBack, + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/twix/stats/detail/StatsDetailRoute.kt` around lines 5 - 7, StatsDetailRoute is currently empty so navigation lands on a blank screen; update StatsDetailRoute to render the actual UI (call StatsDetailScreen(onBack = onBack)) or a temporary placeholder composable indicating "Coming soon" and keep a TODO comment if intentionally unfinished; ensure you pass through the onBack lambda to the rendered screen (refer to StatsDetailRoute and StatsDetailScreen) so the back navigation continues to work.feature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.kt (2)
91-103:⚠️ Potential issue | 🟡 Minor메뉴 아이콘도 접근성 라벨과 터치 영역 보강이 필요합니다.
왜 문제인지: Line 97의
contentDescription = null은 클릭 가능한 요소의 목적을 스크린리더가 전달하지 못합니다. 또한 현재 클릭 영역이 24dp라 접근성 터치 타겟에 불리합니다.
어떻게 개선할지: 의미 있는 문자열 리소스를 넣고, 클릭 처리를 부모Box(60dp 영역)로 올려 터치 타겟을 넓히는 구성이 좋습니다.수정 예시
Box( modifier = Modifier - .fillMaxSize(), + .fillMaxSize() + .noRippleClickable(onClick = onClickAction), contentAlignment = Alignment.Center, ) { Image( painter = painterResource(R.drawable.ic_meatball), - contentDescription = null, + contentDescription = stringResource(R.string.content_description_more_options), modifier = Modifier .size(24.dp) - .rotate(90f) - .noRippleClickable(onClick = onClickAction), + .rotate(90f), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.kt` around lines 91 - 103, The menu Image in StatsDetailTopbar currently uses contentDescription = null and a 24.dp click target; change it to provide a meaningful accessibility label (use a string resource, e.g., R.string.menu_description) and move the click handler from the Image to the parent Box so the Box handles onClickAction with a larger touch area (wrap Box with Modifier.size(60.dp).noRippleClickable(onClick = onClickAction) or equivalent), leaving the Image as a purely decorative element (no clickable) and ensuring the Box has contentDescription/semantics for screen readers.
50-58:⚠️ Potential issue | 🟡 Minor뒤로가기 접근성 문구와 터치 영역을 함께 보완해 주세요.
왜 문제인지: Line 52의
"back"하드코딩은 로컬라이징/접근성 대응이 어렵고, Line 55-57의 modifier 순서로 인해 실제 클릭 영역이 아이콘(24dp) 중심으로 축소됩니다.
어떻게 개선할지: 문자열 리소스로 교체하고, 클릭 modifier를 padding 앞에 배치해 최소 터치 영역을 확보하는 쪽이 안전합니다.수정 예시
Image( painter = painterResource(R.drawable.ic_arrow3_left), - contentDescription = "back", + contentDescription = stringResource(R.string.content_description_back), modifier = Modifier + .noRippleClickable(onClick = onBack) .padding(18.dp) - .size(24.dp) - .noRippleClickable(onClick = onBack), + .size(24.dp), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.kt` around lines 50 - 58, Replace the hardcoded contentDescription "back" with a localized string resource (use stringResource(R.string.some_back_label)) and move the click modifier so the tappable area includes padding—apply noRippleClickable(onClick = onBack) (or Modifier.clickable) before padding/size in the Modifier chain for the Image in StatsDetailTopbar; update the resource name (e.g., R.string.back) and ensure the Image composable (painterResource, contentDescription, modifier) uses the reordered modifiers so the touch target meets minimum accessibility size.
🧹 Nitpick comments (5)
feature/stats/detail/src/main/java/com/twix/stats/detail/contract/StatsDetailSideEffect.kt (1)
7-10:ShowToast가StatsSideEffect와 동일하게 중복 정의되어 있습니다.
feature/main/src/main/java/com/twix/stats/contract/StatsSideEffect.kt의ShowToast와 구조가 완전히 동일합니다. 현재는 모듈 독립성을 위한 의도적 중복일 수 있지만, 여러 feature 모듈에서 동일한 패턴이 반복될 경우 유지보수 비용이 증가할 수 있습니다.공통 UI 기반 모듈(
core:ui혹은designsystem등)이 이미 존재한다면, 아래와 같이 공용ShowToastside effect를 해당 모듈로 추출하는 것을 고려해 보실 수 있습니다.💡 토론 포인트: 앞으로 feature 모듈이 더 추가될 때도 동일 패턴이 반복될 가능성이 높은데, 공통 SideEffect 추상화를 지금 시점에 논의해 보시는 건 어떨까요?
#!/bin/bash # Description: 공통 ShowToast 또는 유사 구조가 다른 feature 모듈에도 존재하는지 확인 rg -n "data class ShowToast" --type kotlin -A 3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/twix/stats/detail/contract/StatsDetailSideEffect.kt` around lines 7 - 10, The ShowToast data class is duplicated between StatsDetailSideEffect and StatsSideEffect; extract a single shared ShowToast (e.g., data class ShowToast(val message: Int, val type: ToastType)) into a common UI module (core:ui or designsystem) and replace the local definitions: remove the ShowToast in StatsDetailSideEffect, update references to use the shared ShowToast type, and ensure the shared class implements the appropriate SideEffect sealed interface (or make it a standalone shared SideEffect type) so StatsDetailSideEffect and the feature/main StatsSideEffect can reference the common symbol.feature/stats/detail/src/main/java/com/twix/stats/detail/preview/StatsDetailUiStatePreviewProvider.kt (2)
14-57: 프리뷰 기준일은 고정값으로 통일하는 것을 권장합니다.
LocalDate.now()를 여러 곳에서 호출하면 날짜 경계 시점에 프리뷰 결과가 흔들릴 수 있습니다. 기준일(referenceDate) 하나를 두고 파생시키면 재현성이 좋아집니다.예시 변경안
class StatsDetailUiStatePreviewProvider : PreviewParameterProvider<StatsDetailUiState> { + private val referenceDate = LocalDate.of(2026, 2, 1) + private val completedDates = listOf( CompletedDate( - date = LocalDate.now().minusDays(1), + date = referenceDate.minusDays(1), myImageUrl = "https://picsum.photos/200", partnerImageUrl = "https://picsum.photos/201", ), CompletedDate( - date = LocalDate.now().minusDays(3), + date = referenceDate.minusDays(3), myImageUrl = "https://picsum.photos/202", partnerImageUrl = null, ), CompletedDate( - date = LocalDate.now().minusDays(5), + date = referenceDate.minusDays(5), myImageUrl = null, partnerImageUrl = "https://picsum.photos/203", ), ) @@ - monthDate = LocalDate.now(), + monthDate = referenceDate, @@ - startDate = LocalDate.now().minusMonths(1), + startDate = referenceDate.minusMonths(1), @@ StatsCalendarUiModel.create( - currentDate = LocalDate.now(), + currentDate = referenceDate, completedDate = completedDates, ) @@ - selectedDate = LocalDate.now(), + selectedDate = referenceDate,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/twix/stats/detail/preview/StatsDetailUiStatePreviewProvider.kt` around lines 14 - 57, The preview uses LocalDate.now() in multiple places (see completedDates, baseDetail, baseCalendarUiModel), which can cause non-deterministic previews; introduce a single referenceDate val (e.g., referenceDate = LocalDate.of(...) or LocalDate.now()) at top of the file and replace all LocalDate.now() usages in CompletedDate(date), StatsDetail(monthDate, startDate), and StatsCalendarUiModel.create(currentDate, completedDate derivations) with values derived from that referenceDate so the preview is stable and reproducible.
18-30: 프리뷰 이미지 URL은 네트워크 비의존 형태도 함께 고려해보면 좋겠습니다.외부 URL 기반 프리뷰는 오프라인/느린 네트워크에서 화면 확인이 불안정해질 수 있습니다. 로컬 placeholder 리소스나
null케이스 중심으로도 한 세트를 추가해보는 건 어떨까요?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/twix/stats/detail/preview/StatsDetailUiStatePreviewProvider.kt` around lines 18 - 30, Update the preview data in StatsDetailUiStatePreviewProvider to include non-network image cases: add CompletedDate entries that use null for myImageUrl and partnerImageUrl and at least one entry that references a local placeholder resource (e.g., a drawable resource name or local URI) so previews render offline; ensure the preview consumer code that reads CompletedDate (the preview list used by the provider) handles nulls and local resource identifiers consistently.feature/stats/detail/src/main/java/com/twix/stats/detail/contract/StatsDetailUiState.kt (1)
16-16: 파생 상태명은 실제 의미에 맞게 더 명확히 바꾸는 것을 권장합니다.현재 값은 “진행중 여부”가 아니라 “날짜 선택 여부”를 나타내므로, 이후 유지보수 시 오해로 잘못된 분기 로직이 생길 수 있습니다.
hasSelectedDate같은 이름으로 바꾸는 방향은 어떨까요?예시 변경안
- val isInProgressStatsDetail get() = selectedDate != null + val hasSelectedDate: Boolean get() = selectedDate != null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/twix/stats/detail/contract/StatsDetailUiState.kt` at line 16, The derived-state property isInProgressStatsDetail misleads by implying "in-progress" when it simply checks selectedDate; rename the property to something clearer like hasSelectedDate and update its definition to use selectedDate (e.g., val hasSelectedDate get() = selectedDate != null), then refactor all usages of isInProgressStatsDetail across the codebase (including view bindings, viewmodels, tests, and any serialization or mappings) to the new name to avoid breaking references and keep behavior identical.feature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.kt (1)
154-169: Preview 제공 좋습니다.Topbar 상태를 빠르게 확인할 수 있게 Preview를 포함한 점이 좋습니다. 이후 케이스별(
isInProgressStatsDetail=false,popupMenuVisibility=false) Preview도 추가하면 회귀 확인이 더 쉬워집니다.As per coding guidelines
Compose UI - Preview Composable이 제공되는가?.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.kt` around lines 154 - 169, Add additional `@Preview` composables to cover the other states of StatsDetailTopbar so reviewers can visually verify regressions: create at least one preview with isInProgressStatsDetail = false (e.g., StatsDetailTopbarPreview_NotInProgress) and one with popupMenuVisibility = false (e.g., StatsDetailTopbarPreview_PopupHidden); each should call StatsDetailTopbar with the same stub callbacks used in StatsDetailTopbarPreview so the UI variations for those boolean flags are visible in the IDE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.kt`:
- Around line 86-90: The popup uses a fixed IntOffset (popupOffset with
LocalDensity and -40.dp/55.dp) which can misplace the CommonPopup relative to
its trigger; instead, measure the trigger icon's position using
onGloballyPositioned on the Box/Icon composable to capture its window
coordinates, convert those coordinates with LocalDensity if needed, and compute
a dynamic IntOffset for popupOffset based on the measured x/y (and size) so the
CommonPopup is positioned relative to the actual icon location across devices
and rotations.
---
Duplicate comments:
In
`@feature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.kt`:
- Around line 91-103: The menu Image in StatsDetailTopbar currently uses
contentDescription = null and a 24.dp click target; change it to provide a
meaningful accessibility label (use a string resource, e.g.,
R.string.menu_description) and move the click handler from the Image to the
parent Box so the Box handles onClickAction with a larger touch area (wrap Box
with Modifier.size(60.dp).noRippleClickable(onClick = onClickAction) or
equivalent), leaving the Image as a purely decorative element (no clickable) and
ensuring the Box has contentDescription/semantics for screen readers.
- Around line 50-58: Replace the hardcoded contentDescription "back" with a
localized string resource (use stringResource(R.string.some_back_label)) and
move the click modifier so the tappable area includes padding—apply
noRippleClickable(onClick = onBack) (or Modifier.clickable) before padding/size
in the Modifier chain for the Image in StatsDetailTopbar; update the resource
name (e.g., R.string.back) and ensure the Image composable (painterResource,
contentDescription, modifier) uses the reordered modifiers so the touch target
meets minimum accessibility size.
In
`@feature/stats/detail/src/main/java/com/twix/stats/detail/di/StatsDetailModule.kt`:
- Line 11: The DI qualifier is registered using NavRoutes.StatsDetailRoute.route
but the contributor is StatsDetailGraph; change the qualifier to the graph-based
route to keep lookups consistent (use StatsDetailGraph.graphRoute or
NavRoutes.StatsDetailGraph.route) in the single<NavGraphContributor>
registration so the provider and consumer use the same unique identifier (update
the registration that currently references NavRoutes.StatsDetailRoute.route to
reference the StatsDetailGraph route symbol instead).
In
`@feature/stats/detail/src/main/java/com/twix/stats/detail/navigation/StatsDetailGraph.kt`:
- Around line 19-38: The current registerGraph implementation registers a single
composable directly so the declared graph boundary (graphRoute/startDestination)
from NavRoutes.StatsDetailRoute is not applied; wrap the existing
composable(...) call inside a navigation(route =
NavRoutes.StatsDetailRoute.graphRoute, startDestination =
NavRoutes.StatsDetailRoute.startDestination) block so the
NavGraphBuilder.registerGraph creates a nested graph; keep the existing
composable that calls StatsDetailRoute(onBack = navController::popBackStack)
unchanged but move it into the navigation { ... } closure to ensure graph-level
routing and backstack behavior are honored.
In
`@feature/stats/detail/src/main/java/com/twix/stats/detail/StatsDetailRoute.kt`:
- Around line 5-7: StatsDetailRoute is currently empty so navigation lands on a
blank screen; update StatsDetailRoute to render the actual UI (call
StatsDetailScreen(onBack = onBack)) or a temporary placeholder composable
indicating "Coming soon" and keep a TODO comment if intentionally unfinished;
ensure you pass through the onBack lambda to the rendered screen (refer to
StatsDetailRoute and StatsDetailScreen) so the back navigation continues to
work.
---
Nitpick comments:
In
`@feature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.kt`:
- Around line 154-169: Add additional `@Preview` composables to cover the other
states of StatsDetailTopbar so reviewers can visually verify regressions: create
at least one preview with isInProgressStatsDetail = false (e.g.,
StatsDetailTopbarPreview_NotInProgress) and one with popupMenuVisibility = false
(e.g., StatsDetailTopbarPreview_PopupHidden); each should call StatsDetailTopbar
with the same stub callbacks used in StatsDetailTopbarPreview so the UI
variations for those boolean flags are visible in the IDE.
In
`@feature/stats/detail/src/main/java/com/twix/stats/detail/contract/StatsDetailSideEffect.kt`:
- Around line 7-10: The ShowToast data class is duplicated between
StatsDetailSideEffect and StatsSideEffect; extract a single shared ShowToast
(e.g., data class ShowToast(val message: Int, val type: ToastType)) into a
common UI module (core:ui or designsystem) and replace the local definitions:
remove the ShowToast in StatsDetailSideEffect, update references to use the
shared ShowToast type, and ensure the shared class implements the appropriate
SideEffect sealed interface (or make it a standalone shared SideEffect type) so
StatsDetailSideEffect and the feature/main StatsSideEffect can reference the
common symbol.
In
`@feature/stats/detail/src/main/java/com/twix/stats/detail/contract/StatsDetailUiState.kt`:
- Line 16: The derived-state property isInProgressStatsDetail misleads by
implying "in-progress" when it simply checks selectedDate; rename the property
to something clearer like hasSelectedDate and update its definition to use
selectedDate (e.g., val hasSelectedDate get() = selectedDate != null), then
refactor all usages of isInProgressStatsDetail across the codebase (including
view bindings, viewmodels, tests, and any serialization or mappings) to the new
name to avoid breaking references and keep behavior identical.
In
`@feature/stats/detail/src/main/java/com/twix/stats/detail/preview/StatsDetailUiStatePreviewProvider.kt`:
- Around line 14-57: The preview uses LocalDate.now() in multiple places (see
completedDates, baseDetail, baseCalendarUiModel), which can cause
non-deterministic previews; introduce a single referenceDate val (e.g.,
referenceDate = LocalDate.of(...) or LocalDate.now()) at top of the file and
replace all LocalDate.now() usages in CompletedDate(date),
StatsDetail(monthDate, startDate), and StatsCalendarUiModel.create(currentDate,
completedDate derivations) with values derived from that referenceDate so the
preview is stable and reproducible.
- Around line 18-30: Update the preview data in
StatsDetailUiStatePreviewProvider to include non-network image cases: add
CompletedDate entries that use null for myImageUrl and partnerImageUrl and at
least one entry that references a local placeholder resource (e.g., a drawable
resource name or local URI) so previews render offline; ensure the preview
consumer code that reads CompletedDate (the preview list used by the provider)
handles nulls and local resource identifiers consistently.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/src/main/java/com/yapp/twix/di/FeatureModules.ktfeature/stats/detail/build.gradle.ktsfeature/stats/detail/src/main/java/com/twix/stats/detail/StatsDetailRoute.ktfeature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.ktfeature/stats/detail/src/main/java/com/twix/stats/detail/contract/StatsDetailSideEffect.ktfeature/stats/detail/src/main/java/com/twix/stats/detail/contract/StatsDetailUiState.ktfeature/stats/detail/src/main/java/com/twix/stats/detail/di/StatsDetailModule.ktfeature/stats/detail/src/main/java/com/twix/stats/detail/navigation/StatsDetailGraph.ktfeature/stats/detail/src/main/java/com/twix/stats/detail/preview/StatsDetailUiStatePreviewProvider.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/stats/detail/build.gradle.kts
feature/stats/detail/src/main/java/com/twix/stats/detail/component/StatsDetailTopbar.kt
Show resolved
Hide resolved
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.
이슈 번호
#88
작업내용
결과물
리뷰어에게 추가로 요구하는 사항 (선택)
PR이 너무 커져서 나누기 위해 간단한 세팅 먼저 PR 만들었어 !