[Feature] 콜밴 글쓰기 날짜 선택 필드 추가#1324
[Feature] 콜밴 글쓰기 날짜 선택 필드 추가#1324JaeYoung290 wants to merge 15 commits intofeature/#1283-callvan-create-location-pickerfrom
Conversation
Walkthrough새로운 공개 컴포저블 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
JaeYoung290
force-pushed
the
feature/#1283-callvan-create-location-picker
branch
from
546a4d2 to
a42ae2a
Compare
JaeYoung290
force-pushed
the
feature/#1283-callvan-create-date-field
branch
from
a30bf04 to
e5a40df
Compare
| onFieldClick: () -> Unit, | ||
| onYearIndexChange: (Int) -> Unit, | ||
| onMonthIndexChange: (Int) -> Unit, | ||
| onDayIndexChange: (Int) -> Unit, | ||
| onReset: () -> Unit, | ||
| onConfirm: () -> Unit |
| isPickerVisible: Boolean, | ||
| selectedYear: Int, | ||
| selectedMonth: Int, | ||
| selectedDay: Int, |
| Row( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .background(KoinTheme.colors.neutral100, RoundedCornerShape(8.dp)) |
There was a problem hiding this comment.
RebrandKoinTheme, 그리고 8.dp는 디자인시스템에 있습니다.
| selectedYear: Int, | ||
| selectedMonth: Int, | ||
| selectedDay: Int, | ||
| onYearIndexChange: (Int) -> Unit, | ||
| onMonthIndexChange: (Int) -> Unit, | ||
| onDayIndexChange: (Int) -> Unit, | ||
| onReset: () -> Unit, | ||
| onConfirm: () -> Unit |
| val currentYear = Calendar.getInstance().get(Calendar.YEAR) | ||
| val years = listOf("${currentYear}년", "${currentYear + 1}년") | ||
| val months = (1..12).map { "${it}월" } | ||
| val daysCount = Calendar.getInstance().apply { | ||
| set(Calendar.YEAR, selectedYear) | ||
| set(Calendar.MONTH, selectedMonth - 1) | ||
| }.getActualMaximum(Calendar.DAY_OF_MONTH) | ||
| val days = (1..daysCount).map { "${it}일" } | ||
|
|
||
| val yearIndex = (selectedYear - currentYear).coerceIn(0, years.size - 1) | ||
| val monthIndex = (selectedMonth - 1).coerceIn(0, months.size - 1) | ||
| val dayIndex = (selectedDay - 1).coerceIn(0, days.size - 1) |
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(horizontal = 24.dp), | ||
| shape = RoundedCornerShape(8.dp), |
| items = years, | ||
| selectedIndex = yearIndex, | ||
| onIndexChange = onYearIndexChange, | ||
| modifier = Modifier.width(53.dp), |
| style = KoinTheme.typography.medium14, | ||
| color = RebrandKoinTheme.colors.primary500, | ||
| modifier = Modifier.clickable(onClick = onReset) | ||
| ) | ||
| Text( | ||
| text = stringResource(R.string.callvan_create_picker_confirm), | ||
| style = KoinTheme.typography.medium14, | ||
| color = RebrandKoinTheme.colors.primary500, |
|
✅ CI 완료. CodeRabbit 리뷰를 요청합니다. |
|
@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: 2
🤖 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/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.kt`:
- Around line 121-125: selectedDay can become invalid when month/year change
because only dayIndex is clamped locally; immediately clamp selectedDay against
the recomputed daysCount and call the parent updater so the shared state never
stays at e.g. 31 after switching to February. In CallvanDateField.kt, after
computing daysCount (and similarly in the block covering lines ~133-170),
compute newDay = min(selectedDay, daysCount), update the parent-selectedDay
state (the prop/updater used by the parent) with newDay right away, and ensure
this mirrors the clamp you already apply to dayIndex; also note
CallvanScrollPicker.onIndexChange only fires on scroll stop so proactively
syncing the parent state here prevents saving invalid dates.
- Around line 121-125: The calculation of daysCount in CallvanDateField.kt is
affected by Calendar's lenient behavior when only YEAR and MONTH are set on a
Calendar with today's day still present; to fix, create a non-ambiguous calendar
for the target month before calling getActualMaximum(Calendar.DAY_OF_MONTH) —
e.g., build/obtain a Calendar with its fields cleared or set DAY_OF_MONTH to 1
(or call setLenient(false)) then set YEAR and MONTH using selectedYear and
selectedMonth - 1, and finally call getActualMaximum(Calendar.DAY_OF_MONTH) to
compute daysCount and then map to days; update the code paths that compute
daysCount to use this corrected Calendar construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae95e128-a238-4cf1-8cc3-60ce81c7ff2c
📒 Files selected for processing (1)
feature/callvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.kt
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.kt
Outdated
Show resolved
Hide resolved
…ure/#1283-callvan-create-date-field
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| val yearIndex = remember(selectedYear, currentYear) { | ||
| (selectedYear - currentYear).coerceIn(0, years.size - 1) | ||
| } | ||
| val monthIndex = remember(selectedMonth) { | ||
| (selectedMonth - 1).coerceIn(0, months.size - 1) | ||
| } | ||
| val dayIndex = remember(selectedDay, days.size) { | ||
| (selectedDay - 1).coerceIn(0, days.size - 1) | ||
| } |
There was a problem hiding this comment.
[Minor] remember + 키 대신 derivedStateOf 사용 권장
yearIndex, monthIndex, dayIndex는 다른 상태로부터 파생된 값입니다. remember(key) { ... } 패턴도 동작하지만, Compose 공식 가이드라인에서는 이처럼 다른 상태 값에서 파생되는 값에는 derivedStateOf를 권장합니다. derivedStateOf는 결과값이 실제로 변경된 경우에만 리컴포지션을 발생시켜 불필요한 재계산을 방지합니다.
| val yearIndex = remember(selectedYear, currentYear) { | |
| (selectedYear - currentYear).coerceIn(0, years.size - 1) | |
| } | |
| val monthIndex = remember(selectedMonth) { | |
| (selectedMonth - 1).coerceIn(0, months.size - 1) | |
| } | |
| val dayIndex = remember(selectedDay, days.size) { | |
| (selectedDay - 1).coerceIn(0, days.size - 1) | |
| } | |
| val yearIndex by remember { derivedStateOf { (selectedYear - currentYear).coerceIn(0, years.size - 1) } } | |
| val monthIndex by remember { derivedStateOf { (selectedMonth - 1).coerceIn(0, months.size - 1) } } | |
| val dayIndex by remember { derivedStateOf { (selectedDay - 1).coerceIn(0, days.size - 1) } } |
| onReset: () -> Unit = {}, | ||
| onConfirm: () -> Unit = {} | ||
| ) { | ||
| val currentYear = remember { Year.now().value } |
There was a problem hiding this comment.
[Minor] currentYear — 자정 이후 컴포지션이 유지되면 값이 stale해짐
remember { Year.now().value }는 컴포저블이 컴포지션에 처음 진입할 때 단 한 번 평가됩니다. 앱을 12월 31일 23:59에 열어두고 자정을 넘기면 여전히 이전 연도를 표시합니다.
실제 서비스에서 발생 가능성은 낮지만, 아래처럼 key 없이 remember를 사용하는 상황에서 years 리스트도 동일하게 stale해지므로 잠재적 불일치가 생깁니다. 연도 목록은 ViewModel에서 관리하거나, 최소한 currentYear 계산을 ViewModel 레이어로 올리는 것을 고려해 보세요.
| onConfirm: () -> Unit = {} | ||
| ) { | ||
| val currentYear = remember { Year.now().value } | ||
| val years = remember(currentYear) { persistentListOf("${currentYear}년", "${currentYear + 1}년") } |
There was a problem hiding this comment.
[Trivial] remember(currentYear) 키가 실질적으로 불필요
currentYear 자체가 remember { ... } (키 없음)로 선언되어 있어 컴포지션 생애 동안 절대 바뀌지 않습니다. 따라서 remember(currentYear) { ... }에서 currentYear를 키로 줘도 재계산이 발생하지 않습니다. 코드의 의도를 명확히 하려면 키를 제거하거나, 위 Minor 코멘트처럼 currentYear 자체를 외부(ViewModel)에서 주입받는 형태로 바꾸는 것이 좋습니다.
| val years = remember(currentYear) { persistentListOf("${currentYear}년", "${currentYear + 1}년") } | |
| val years = remember { persistentListOf("${currentYear}년", "${currentYear + 1}년") } |
코드 리뷰 요약전체적으로 Compose 상태 호이스팅 패턴을 잘 따르고 있으며, 발견된 이슈 목록
핵심 수정 필요 사항Major 이슈 (머지 전 반드시 수정): 월/연도 변경 시 선택된 일(day)이 새 월의 최대 일수를 초과하는 경우를 ViewModel에서 처리해야 합니다. // ViewModel에서:
fun onMonthChange(month: Int) {
_selectedMonth.value = month
val maxDay = YearMonth.of(_selectedYear.value, month).lengthOfMonth()
if (_selectedDay.value > maxDay) {
_selectedDay.value = maxDay
}
}이 처리가 없으면 "2월 31일" 같은 유효하지 않은 날짜가 그대로 서버에 전송됩니다. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
최신 커밋 기준 전체 수정 사항 재검토 완료 후 승인합니다.
최종 확인 항목:
- ✅ Calendar lenient 버그 —
YearMonth.of(selectedYear, selectedMonth).lengthOfMonth()로 완전 해결 - ✅ remember 최적화 —
currentYear,years,months무조건 캐싱,days는remember(selectedYear, selectedMonth)키 의존 캐싱 - ✅ 화살표 애니메이션 —
animateFloatAsState적용 - ✅ 콜백 시그니처 —
onYearChange(Int)/onMonthChange(Int)/onDayChange(Int)실제 값 전달 - ✅ selectedDay invalid 처리 — ViewModel(updateYear/updateMonth 내 blockingIntent + coerceAtMost)에서 처리
- ✅ @Preview 함수 모두 private
- ✅ UI 문자열 strings.xml 분리 완료
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.kt
Show resolved
Hide resolved
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.kt
Show resolved
Hide resolved
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.kt
Show resolved
Hide resolved
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.kt
Show resolved
Hide resolved
| onConfirm: () -> Unit = {} | ||
| ) { | ||
| val currentYear = remember { Year.now().value } | ||
| val years = remember { persistentListOf("${currentYear}년", "${currentYear + 1}년") } |
There was a problem hiding this comment.
[Major] 과거 날짜 선택 가능 - 비즈니스 로직 검증 누락
현재 연도와 다음 연도만 표시하지만, 같은 연도 내에서 이미 지난 날짜를 선택하는 것을 막는 로직이 없습니다. 콜밴 출발 날짜를 오늘 이전으로 설정하면 서버 요청 실패 또는 잘못된 데이터가 저장될 수 있습니다.
최소한 onConfirm 시점에 상위 ViewModel/UseCase에서 날짜 유효성 검증을 수행하거나, 피커 자체에서 과거 날짜 선택을 비활성화해야 합니다. 예시:
// ViewModel 또는 UseCase에서
val today = LocalDate.now()
val selectedDate = LocalDate.of(selectedYear, selectedMonth, selectedDay)
require(selectedDate >= today) { "출발 날짜는 오늘 이후여야 합니다." }UI 레벨에서도 현재 연도를 선택했을 때 지난 달을 선택하지 못하도록 months를 동적으로 제한하는 것을 고려해보세요.
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.kt
Show resolved
Hide resolved
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.kt
Show resolved
Hide resolved
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.kt
Show resolved
Hide resolved
...lvan/src/main/java/in/koreatech/koin/feature/callvan/ui/create/component/CallvanDateField.kt
Show resolved
Hide resolved
|
3 participants
PR 개요
이슈 번호: #1283
PR 체크리스트
작업사항
작업사항의 상세한 설명
출발 날짜를 선택할 수 있는 필드와 날짜 선택 카드를 추가합니다.
논의 사항
스크린샷
추가내용
Summary by CodeRabbit