feat: 공통 바텀시트 및 휠피커 구현#30
Conversation
Gemini AI 코드리뷰안녕하세요, @[PR Author's GitHub Username]님! 이번 PR을 통해 다음은 코드 리뷰 내용입니다. 전반적인 피드백
세부 피드백 및 제안 사항
|
📝 WalkthroughWalkthrough디자인 시스템에 확인용 바텀시트와 제네릭 휠 피커를 추가하고 팝업 콜백 계약을 변경했습니다. 홈 기능에는 날짜·시간 변경 바텀시트와 캐릭터 변경 다이얼로그를 추가했으며, 관련 아이콘과 프리뷰를 포함했습니다. Changes디자인 시스템 컴포넌트
홈 선택 UI
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/designsystem/src/main/java/kr/co/call/designsystem/component/bottomsheet/ConfirmBottomSheet.kt (1)
78-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win스크림 색상도 디자인 토큰을 사용하세요.
Color.Black직접 사용은 테마 토큰 경로를 우회합니다. 공통 스크림 토큰을 사용하거나, 없다면 토큰으로 정의해 테마 변경 시 함께 반영되게 하세요. As per path instructions,Theme/Color/Typography는 하드코딩 값 대신 디자인 토큰을 참조하는지.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/designsystem/src/main/java/kr/co/call/designsystem/component/bottomsheet/ConfirmBottomSheet.kt` at line 78, Update the scrimColor assignment in ConfirmBottomSheet to use the shared design-system scrim color token instead of Color.Black, preserving the 0.2f alpha unless the token already defines the intended opacity. If no shared scrim token exists, add and reference one through the theme color-token path so theme changes propagate consistently.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/designsystem/src/main/java/kr/co/call/designsystem/component/bottomsheet/ConfirmBottomSheet.kt`:
- Around line 101-143: Update the ConfirmBottomSheet content area around
content() so it occupies the remaining height within the fixed 387.dp Column by
placing it in a Modifier.weight(1f) container before the Spacer and
PrimaryButton. Allow the body content to scroll internally when it exceeds the
allocated space, while keeping the existing button visible and unchanged.
In
`@core/designsystem/src/main/java/kr/co/call/designsystem/component/picker/WheelPicker.kt`:
- Around line 119-129: Update the LaunchedEffect flow in WheelPicker to skip the
initial snapshotFlow emission before filtering for !isScrolling, adding the
required drop operator import. Preserve callback delivery only after an actual
scroll transition has stopped.
---
Nitpick comments:
In
`@core/designsystem/src/main/java/kr/co/call/designsystem/component/bottomsheet/ConfirmBottomSheet.kt`:
- Line 78: Update the scrimColor assignment in ConfirmBottomSheet to use the
shared design-system scrim color token instead of Color.Black, preserving the
0.2f alpha unless the token already defines the intended opacity. If no shared
scrim token exists, add and reference one through the theme color-token path so
theme changes propagate consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7288279-7b8d-4348-8fd1-a2b3c647b8f3
📒 Files selected for processing (3)
core/designsystem/src/main/java/kr/co/call/designsystem/component/bottomsheet/ConfirmBottomSheet.ktcore/designsystem/src/main/java/kr/co/call/designsystem/component/picker/WheelPicker.ktcore/designsystem/src/main/res/drawable/ic_close_circle.xml
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .height(387.dp), | ||
| ) { | ||
| Box( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .height(82.dp), | ||
| ) { | ||
| Text( | ||
| text = title, | ||
| modifier = Modifier | ||
| .align(Alignment.BottomStart) | ||
| .padding(start = 16.dp, bottom = 24.dp), | ||
| style = CallTheme.typography.bodyLargeBold, | ||
| color = CallTheme.colors.black, | ||
| ) | ||
|
|
||
| IconButton( | ||
| onClick = onDismissRequest, | ||
| modifier = Modifier | ||
| .align(Alignment.TopEnd) | ||
| .padding(top = 5.dp, end = 5.dp) | ||
| .size(48.dp), | ||
| ) { | ||
| Image( | ||
| painter = painterResource(R.drawable.ic_close_circle), | ||
| contentDescription = "닫기", | ||
| modifier = Modifier.size(27.dp), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| content() | ||
|
|
||
| Spacer(modifier = Modifier.height(24.dp)) | ||
|
|
||
| PrimaryButton( | ||
| text = confirmText, | ||
| onClick = onConfirmClick, | ||
| modifier = Modifier.padding(horizontal = 29.dp), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect the relevant range with line numbers.
target="core/designsystem/src/main/java/kr/co/call/designsystem/component/bottomsheet/ConfirmBottomSheet.kt"
wc -l "$target"
sed -n '1,220p' "$target" | cat -n
# Find similar bottom sheet implementations for comparison.
fd -a 'BottomSheet.kt' core/designsystem/src/main/java/kr/co/call/designsystem/component | sed -n '1,120p'
rg -n "content\(\)|weight\(1f\)|height\([0-9]+\.dp\)|CallTheme\.colors\.black|scrim" core/designsystem/src/main/java/kr/co/call/designsystem/component -SRepository: call-from-ai/CallFromAi_Android
Length of output: 8990
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file's imports and structure more precisely.
target="core/designsystem/src/main/java/kr/co/call/designsystem/component/bottomsheet/ConfirmBottomSheet.kt"
ast-grep outline "$target" --view expandedRepository: call-from-ai/CallFromAi_Android
Length of output: 399
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe whether the repo contains design token usage and bottom sheet layout patterns.
rg -n "CallTheme\.colors\.black|Color\.Black|scrimColor|weight\(1f\)|verticalScroll|LazyColumn|BottomSheet" core/designsystem/src/main/java -SRepository: call-from-ai/CallFromAi_Android
Length of output: 3341
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the exact lines around the comment snippet after mapping the file.
target="core/designsystem/src/main/java/kr/co/call/designsystem/component/bottomsheet/ConfirmBottomSheet.kt"
sed -n '80,170p' "$target" | cat -nRepository: call-from-ai/CallFromAi_Android
Length of output: 3127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
target="core/designsystem/src/main/java/kr/co/call/designsystem/component/bottomsheet/ConfirmBottomSheet.kt"
python3 - <<'PY'
from pathlib import Path
p = Path("core/designsystem/src/main/java/kr/co/call/designsystem/component/bottomsheet/ConfirmBottomSheet.kt")
for i, line in enumerate(p.read_text().splitlines(), 1):
if 1 <= i <= 220:
print(f"{i:4d}: {line}")
PYRepository: call-from-ai/CallFromAi_Android
Length of output: 6415
본문 슬롯에 남은 높이를 먼저 할당하세요. ConfirmBottomSheet.kt:101-143에서 content()가 고정 높이(387dp) 안에서 버튼보다 먼저 무제한으로 측정되어, 본문이 길면 확인 버튼이 시트 밖으로 밀릴 수 있습니다. content()를 Modifier.weight(1f) 영역에 넣고, 필요하면 내부 스크롤을 허용하세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/designsystem/src/main/java/kr/co/call/designsystem/component/bottomsheet/ConfirmBottomSheet.kt`
around lines 101 - 143, Update the ConfirmBottomSheet content area around
content() so it occupies the remaining height within the fixed 387.dp Column by
placing it in a Modifier.weight(1f) container before the Spacer and
PrimaryButton. Allow the body content to scroll internally when it exceeds the
allocated space, while keeping the existing button visible and unchanged.
| // 스크롤이 끝난 시점의 선택값만 외부에 전달 | ||
| LaunchedEffect(listState, items) { | ||
| snapshotFlow { listState.isScrollInProgress } | ||
| .distinctUntilChanged() | ||
| .filter { isScrolling -> !isScrolling } | ||
| .collect { | ||
| items.getOrNull(selectedIndex)?.let { selectedItem -> | ||
| currentOnItemSelected(selectedIndex, selectedItem) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
마운트 시점에 onItemSelected가 즉시 호출됩니다.
listState.isScrollInProgress의 초기값은 false이므로, snapshotFlow의 첫 방출값이 distinctUntilChanged()를 그대로 통과해 filter { !isScrolling }에 걸려 컴포저블이 마운트되자마자 onItemSelected가 호출됩니다. KDoc에는 "스크롤이 멈춘 뒤에만 호출되어 ViewModel에 과도한 이벤트가 전달되지 않습니다"라고 명시되어 있는데, 실제로는 사용자가 스크롤을 하지 않아도 초기 선택값에 대해 콜백이 한 번 더 발생합니다. 콜백이 네트워크 호출/상태 갱신 등 부수효과를 유발하는 호출부가 있다면 의도치 않은 초기 실행이 발생할 수 있습니다.
초기 방출을 건너뛰도록 수정하는 것을 제안합니다.
🐛 제안하는 수정
LaunchedEffect(listState, items) {
snapshotFlow { listState.isScrollInProgress }
.distinctUntilChanged()
+ .drop(1) // 마운트 시점의 초기 상태(false) 방출은 건너뜀
.filter { isScrolling -> !isScrolling }
.collect {
items.getOrNull(selectedIndex)?.let { selectedItem ->
currentOnItemSelected(selectedIndex, selectedItem)
}
}
}kotlinx.coroutines.flow.drop import 추가가 필요합니다.
📝 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(listState, items) { | |
| snapshotFlow { listState.isScrollInProgress } | |
| .distinctUntilChanged() | |
| .filter { isScrolling -> !isScrolling } | |
| .collect { | |
| items.getOrNull(selectedIndex)?.let { selectedItem -> | |
| currentOnItemSelected(selectedIndex, selectedItem) | |
| } | |
| } | |
| } | |
| // 스크롤이 끝난 시점의 선택값만 외부에 전달 | |
| LaunchedEffect(listState, items) { | |
| snapshotFlow { listState.isScrollInProgress } | |
| .distinctUntilChanged() | |
| .drop(1) // 마운트 시점의 초기 상태(false) 방출은 건너뜀 | |
| .filter { isScrolling -> !isScrolling } | |
| .collect { | |
| items.getOrNull(selectedIndex)?.let { selectedItem -> | |
| currentOnItemSelected(selectedIndex, selectedItem) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/designsystem/src/main/java/kr/co/call/designsystem/component/picker/WheelPicker.kt`
around lines 119 - 129, Update the LaunchedEffect flow in WheelPicker to skip
the initial snapshotFlow emission before filtering for !isScrolling, adding the
required drop operator import. Preserve callback delivery only after an actual
scroll transition has stopped.
Gemini AI 코드리뷰안녕하세요! PR 잘 봤습니다. 전반적으로 Jetpack Compose와 Kotlin의 고급 기능들을 잘 활용하고 계시네요. 특히 💡 전반적인 피드백 (Overall Feedback)전반적으로 Clean Architecture 원칙과 Jetpack Compose 모범 사례를 잘 따르고 계십니다. 특히 📝 상세 코드 리뷰 (Detailed Code Review)1.
|
WAcAW9
left a comment
There was a problem hiding this comment.
으아니 세상에나 wheelpicker 너무 감사드립니다,,,최고,,🫶
유용하게 잘 쓸게요! 고생하셨어요오
| /** | ||
| * 목록을 세로로 스크롤해 가운데 항목을 선택하는 공통 휠 피커입니다. | ||
| * | ||
| * 이 컴포넌트는 날짜나 시간 같은 값의 의미를 알지 않습니다. 호출부가 [items]와 | ||
| * [itemText]를 제공하면 스크롤, 중앙 항목 계산, 스냅, 선택 색상 변경을 담당합니다. | ||
| * 따라서 화면마다 필요한 휠 개수가 다르면 이 컴포넌트를 하나 이상 조합해서 사용합니다. | ||
| * | ||
| * 날짜 한 개를 선택하는 예시: |
Gemini AI 코드리뷰안녕하세요, 시니어 Android 개발자입니다. 이번 PR에서 새롭게 추가된 디자인 시스템 컴포넌트들과 홈 기능 관련 컴포넌트들을 리뷰했습니다. 전반적으로 Jetpack Compose의 모범 사례들을 잘 적용하고 계신 점이 인상 깊습니다. 특히 다만, 몇 가지 개선할 점들이 있어 피드백을 드립니다. 1.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
feature/home/impl/src/main/java/kr/co/call/impl/component/TimeChangeBottomSheet.kt (1)
50-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win초기 인덱스 fallback을 목록 생성 로직에서 파생시키세요.
?: 30은-30L..90L범위에 종속된 매직 넘버로, 범위가 바뀌면 조용히 깨집니다.dates.indexOf(today)로 계산하면 더 안전합니다. 또한selectedTime이 30분 단위 격자에 없을 경우(?: 0) 자정으로 튀는 대신 가장 가까운 슬롯으로 보정하는 편이 사용자 기대에 더 부합합니다.♻️ 제안 수정
- val initialDateIndex = dates.indexOf(selectedDate).takeIf { index -> index >= 0 } - ?: 30 - val initialTimeIndex = times.indexOf(selectedTime).takeIf { index -> index >= 0 } ?: 0 + val initialDateIndex = dates.indexOf(selectedDate).takeIf { index -> index >= 0 } + ?: dates.indexOf(today) + val initialTimeIndex = times.indexOf(selectedTime).takeIf { index -> index >= 0 } + ?: (selectedTime.toSecondOfDay() / 1800).coerceIn(0, times.lastIndex)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/home/impl/src/main/java/kr/co/call/impl/component/TimeChangeBottomSheet.kt` around lines 50 - 52, Update the initial index calculations in the bottom-sheet setup: derive the date fallback from the generated dates list using today’s index instead of the hardcoded 30, and when selectedTime is absent from the 30-minute slots, select the nearest available time slot rather than defaulting to index 0. Preserve exact matches and ensure both fallbacks remain valid for the generated lists.feature/home/impl/src/main/java/kr/co/call/impl/component/dialog/CharacterChangeDialog.kt (1)
56-70: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value고정 너비(321dp)를 의도대로 적용하려면
usePlatformDefaultWidth = false를 명시하세요.
DialogProperties.usePlatformDefaultWidth의 기본값은true라, 이 설정만 쓰면 플랫폼 기본 다이얼로그 폭(대부분 모바일 기준 약 320dp) 기준으로 크기 제약이 적용됩니다.321.dp가 이를 약간 초과하지만 이 제약이 항상 적용되지 않도록properties = DialogProperties(usePlatformDefaultWidth = false)로 명시하는 편이 의도한 레이아웃이 일관되게 나타납니다.🐛 제안 수정
- Dialog(onDismissRequest = onDismissRequest) { + Dialog( + onDismissRequest = onDismissRequest, + properties = DialogProperties(usePlatformDefaultWidth = false), + ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/home/impl/src/main/java/kr/co/call/impl/component/dialog/CharacterChangeDialog.kt` around lines 56 - 70, Update the Dialog invocation in CharacterChangeDialog to pass DialogProperties with usePlatformDefaultWidth set to false, ensuring the Column’s explicit 321.dp width is applied consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/home/impl/src/main/java/kr/co/call/impl/component/dialog/CharacterChangeDialog.kt`:
- Around line 58-69: Update the dialog modifier around the background in
CharacterChangeDialog to clip content to the same 30.dp RoundedCornerShape,
ensuring CharacterOption pressed-state backgrounds remain inside the rounded
corners while preserving the existing shadow and background styling.
---
Nitpick comments:
In
`@feature/home/impl/src/main/java/kr/co/call/impl/component/dialog/CharacterChangeDialog.kt`:
- Around line 56-70: Update the Dialog invocation in CharacterChangeDialog to
pass DialogProperties with usePlatformDefaultWidth set to false, ensuring the
Column’s explicit 321.dp width is applied consistently.
In
`@feature/home/impl/src/main/java/kr/co/call/impl/component/TimeChangeBottomSheet.kt`:
- Around line 50-52: Update the initial index calculations in the bottom-sheet
setup: derive the date fallback from the generated dates list using today’s
index instead of the hardcoded 30, and when selectedTime is absent from the
30-minute slots, select the nearest available time slot rather than defaulting
to index 0. Preserve exact matches and ensure both fallbacks remain valid for
the generated lists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 81f39958-4d96-4053-865f-990bd889a5d2
📒 Files selected for processing (6)
core/designsystem/src/main/java/kr/co/call/designsystem/component/button/PrimaryButton.ktcore/designsystem/src/main/java/kr/co/call/designsystem/component/picker/WheelPicker.ktcore/designsystem/src/main/java/kr/co/call/designsystem/component/popup/OneButtonPopup.ktcore/designsystem/src/main/java/kr/co/call/designsystem/component/popup/TwoButtonPopup.ktfeature/home/impl/src/main/java/kr/co/call/impl/component/TimeChangeBottomSheet.ktfeature/home/impl/src/main/java/kr/co/call/impl/component/dialog/CharacterChangeDialog.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/designsystem/src/main/java/kr/co/call/designsystem/component/picker/WheelPicker.kt
| modifier = modifier | ||
| .width(321.dp) | ||
| .shadow( | ||
| elevation = 15.dp, | ||
| shape = RoundedCornerShape(30.dp), | ||
| ambientColor = Color.Black.copy(alpha = 0.08f), | ||
| spotColor = Color.Black.copy(alpha = 0.08f), | ||
| ) | ||
| .background( | ||
| color = CallTheme.colors.background, | ||
| shape = RoundedCornerShape(30.dp), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
둥근 모서리 배경에 .clip이 없어 눌림 상태 배경이 모서리를 벗어날 수 있습니다.
Modifier.background(color, shape)는 지정한 도형 안에만 색을 채울 뿐, 자식 뷰의 드로잉을 잘라내지 않습니다. CharacterOption의 눌림 배경(사각형, 라인 149-166)이 첫/마지막 항목일 경우 다이얼로그의 둥근 모서리(30.dp) 밖으로 보일 수 있습니다.
🎨 제안 수정
modifier = modifier
.width(321.dp)
.shadow(
elevation = 15.dp,
shape = RoundedCornerShape(30.dp),
ambientColor = Color.Black.copy(alpha = 0.08f),
spotColor = Color.Black.copy(alpha = 0.08f),
)
+ .clip(RoundedCornerShape(30.dp))
.background(
color = CallTheme.colors.background,
shape = RoundedCornerShape(30.dp),
)📝 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.
| modifier = modifier | |
| .width(321.dp) | |
| .shadow( | |
| elevation = 15.dp, | |
| shape = RoundedCornerShape(30.dp), | |
| ambientColor = Color.Black.copy(alpha = 0.08f), | |
| spotColor = Color.Black.copy(alpha = 0.08f), | |
| ) | |
| .background( | |
| color = CallTheme.colors.background, | |
| shape = RoundedCornerShape(30.dp), | |
| ) | |
| modifier = modifier | |
| .width(321.dp) | |
| .shadow( | |
| elevation = 15.dp, | |
| shape = RoundedCornerShape(30.dp), | |
| ambientColor = Color.Black.copy(alpha = 0.08f), | |
| spotColor = Color.Black.copy(alpha = 0.08f), | |
| ) | |
| .clip(RoundedCornerShape(30.dp)) | |
| .background( | |
| color = CallTheme.colors.background, | |
| shape = RoundedCornerShape(30.dp), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@feature/home/impl/src/main/java/kr/co/call/impl/component/dialog/CharacterChangeDialog.kt`
around lines 58 - 69, Update the dialog modifier around the background in
CharacterChangeDialog to clip content to the same 30.dp RoundedCornerShape,
ensuring CharacterOption pressed-state backgrounds remain inside the rounded
corners while preserving the existing shadow and background styling.
There was a problem hiding this comment.
@codebidoof 네, 적용하는 편이 좋습니다. .shadow(...) 다음에 동일한 RoundedCornerShape(30.dp)로 .clip(...)을 추가하면 그림자는 유지하면서 항목의 눌림 배경이 다이얼로그 모서리 밖으로 렌더링되지 않습니다.
📄 작업 내용 요약
제네릭 WheelPicker 컴포넌트
ConfirmBottomSheet 컴포넌트 - 공통 바텀시트
사용 예시 ) 휠 피커 개수만큼 선언해서 바텀시트에 추가해서 사용하시면 됩니다!
2026-07-22.3.33.08.mov
📎 Issue 번호
✅ 작업 목록
📝 기타 참고사항
Summary by CodeRabbit
Summary