Conversation
📝 WalkthroughWalkthrough내부 Dialog을 래핑하는 Changes
Sequence Diagram(s)(생성 조건 미충족 — 생략) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt (2)
39-101: Consider whether button clicks should auto-dismiss the modal.The current API separates
onDismissRequestfrom button click handlers (onDismissBtnClick,onConfirmBtnClick). This means callers must remember to call the dismiss logic within their button handlers if they want the modal to close on button press.While this provides flexibility, it's a common pattern for modal buttons to dismiss the modal automatically. Consider whether the button handlers should internally call
onDismissRequest()after invoking the user's callback, or document this expectation clearly in the KDoc.♻️ Optional: Auto-dismiss after button clicks
PotiModalButton( text = dismissBtnText, - onClick = onDismissBtnClick, + onClick = { + onDismissBtnClick() + onDismissRequest() + }, modifier = Modifier .weight(1f), type = ModalButtonType.SUB_1, ) PotiModalButton( text = confirmBtnText, - onClick = onConfirmBtnClick, + onClick = { + onConfirmBtnClick() + onDismissRequest() + }, modifier = Modifier .weight(1f), type = ModalButtonType.MAIN, )Alternatively, keep the current design but add a note in the KDoc that callers are responsible for dismissing the modal in their click handlers.
103-131: Preview note: Modal buttons won't dismiss in this preview.In the preview,
onDismissBtnClickandonConfirmBtnClickare empty lambdas, so clicking the buttons won't close the modal. For a more realistic preview, consider having them setshowModal = false:onDismissBtnClick = { showModal = false }, onConfirmBtnClick = { showModal = false },This would better demonstrate the expected usage pattern to developers viewing the preview.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.ktapp/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt (4)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (1)
PotiModal(44-88)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt (1)
PotiModalButton(25-59)app/src/main/java/com/poti/android/core/designsystem/theme/Theme.kt (1)
PotiTheme(48-71)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiFloatingButton.kt (1)
PotiFloatingButton(35-83)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (3)
app/src/main/java/com/poti/android/core/designsystem/theme/Theme.kt (1)
PotiTheme(48-71)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt (1)
PotiModalButton(25-59)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiFloatingButton.kt (1)
PotiFloatingButton(35-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR CI Check (Lint & Build)
🔇 Additional comments (3)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (2)
44-88: Well-structured modal implementation with custom dismiss handling.The approach of setting
dismissOnClickOutside = falseinPopupPropertiesand handling outside clicks manually vianoRippleClickableis a good design choice that allows for the custom scrim behavior. ThepointerInputwith emptydetectTapGesturescorrectly prevents tap propagation to the backdrop.One minor observation: the
rememberkey only includesdismissOnBackPress, which is correct sincedismissOnClickOutsideis handled separately in thenoRippleClickablemodifier and doesn't affect thePopupPropertiesobject.
90-131: Preview demonstrates the modal behavior effectively.The preview provides a good interactive example showing how to toggle the modal visibility using state. This is helpful for developers using the component.
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt (1)
58-99: Clean layout implementation following the design system.The content layout is well-structured with proper spacing and theming. The use of
weight(1f)for both buttons ensures equal width distribution, and the typography/color choices align with the theme.
| ) | ||
| } | ||
|
|
||
| Popup( |
There was a problem hiding this comment.
P1: Popup으로 하면 시스템 상태바와 내비게이션바 영역은 제외되고 dim 처리 되는 걸 아시나요.. 되도록이면 Dialog 쓰고 배경 커스텀하고 싶다면 시스템 기본 DimAmount를 40% 정도로 조절하면 될 것 같습니다
| detectTapGestures { } | ||
| }, | ||
| ) { | ||
| content() |
There was a problem hiding this comment.
P2: 여기도 아예 빈 칸으로 두는 것 보다 Text랑 Button 정도는 넣어두는 건 어떤가요????
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt:
- Around line 44-49: The PotiModal function accepts a modifier parameter but
never applies it, so caller modifiers have no effect; update the Surface
invocation inside PotiModal to include the passed-in modifier (e.g., combine the
incoming modifier with any existing modifiers using modifier.then(...) or
Modifier.then(modifier)) so that the Surface uses the provided modifier,
ensuring calls like Modifier.padding(...) in previews take effect; locate the
PotiModal function and the Surface composable within it and add modifier =
modifier (or modifier = modifier.then(existingModifier)) to the Surface
parameters.
- Around line 62-72: Wrap the imperative window mutations in a Compose
SideEffect: obtain the parent window via a safe cast ((LocalView.current.parent
as?) DialogWindowProvider)?.window, return early if null, then call
dialogWindow.setDimAmount(...) and dialogWindow.setBackgroundDrawable(...)
inside SideEffect to avoid running during recomposition; remove the redundant
dialogWindow.decorView.setBackgroundColor(...) call since
setBackgroundDrawable(ColorDrawable(Color.TRANSPARENT)) overrides it. Ensure you
reference the existing dialogWindow variable and use the safe cast as? to
prevent crashes.
🧹 Nitpick comments (4)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt (3)
33-33: Minor: Empty@modifierdescription in KDoc.The
@param modifierlacks a description. Consider adding a brief explanation or removing the line if it's self-explanatory.
52-53: Consider using a paired data class or requiring both sub-button parameters together.
subBtnTextandonSubBtnClickare independently nullable, but line 107 requires both to be non-null for the sub-button to render. If a caller provides only one, the button silently won't appear. Consider either:
- Documenting this requirement explicitly in the KDoc
- Using a single optional data class parameter (e.g.,
subButton: SubButtonConfig?)- Adding a runtime check during development
Option: Single parameter approach
data class SubButtonConfig(val text: String, val onClick: () -> Unit) // In function signature: subButton: SubButtonConfig? = null, // In usage: subButton?.let { PotiModalButton( text = it.text, onClick = it.onClick, modifier = Modifier.fillMaxWidth(), type = ModalButtonType.SUB_2, ) }
91-91: Consider adding a meaningfulcontentDescriptionfor accessibility.Using
contentDescription = nullmarks the image as decorative. If the image conveys meaningful information relevant to the modal's context, consider accepting a content description parameter for accessibility.app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (1)
35-38: Optional: Complete the@paramdescriptions.The
modifierandcontentparameters are missing descriptions. Consider adding brief descriptions for documentation consistency.-* @param modifier +* @param modifier 모달에 적용할 Modifier입니다. * @param dismissOnBackPress 시스템 뒤로가기 시 모달을 닫는 여부입니다. 기본값 true입니다. * @param dismissOnClickOutside 모달 바깥 영역 터치 시 모달을 닫는 여부입니다. 기본값 true입니다. -* @param content +* @param content 모달 내부에 표시할 컴포저블 콘텐츠입니다.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.ktapp/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt (3)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (1)
PotiModal(43-81)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt (1)
PotiModalButton(25-59)app/src/main/java/com/poti/android/core/designsystem/theme/Theme.kt (1)
PotiTheme(48-71)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (3)
app/src/main/java/com/poti/android/core/designsystem/theme/Theme.kt (1)
PotiTheme(48-71)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt (1)
PotiModalButton(25-59)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiFloatingButton.kt (1)
PotiFloatingButton(35-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR CI Check (Lint & Build)
🔇 Additional comments (3)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt (2)
88-97: Verify intended image sizing behavior.The modifier chain
.size(180.dp).padding(vertical = 24.dp)applies padding inside the 180dp box. This means:
- The total space is 180x180dp
- The image is inset by 24dp top and bottom, leaving only ~132dp height for actual image content
- With
ContentScale.Crop, the image will be cropped to fit this reduced spaceIf the intent is a 180dp image with 24dp vertical spacing around it, reverse the order or use a wrapper:
💡 If spacing around the image is intended
image?.let { Image( painter = painterResource(id = it), contentDescription = null, modifier = Modifier - .size(180.dp) - .padding(vertical = 24.dp), + .padding(vertical = 24.dp) + .size(180.dp), contentScale = ContentScale.Crop, ) }
120-141: LGTM!The preview functions adequately demonstrate both modal variants (lg-1 with content/sub-button and lg-2 with image). They're properly wrapped in
PotiTheme.Minor suggestion: Consider adding a background color to the placeholder
BoxinPotiLarge1Previewto make the content slot visible in the preview.app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (1)
83-124: Preview implementation looks good.The preview properly demonstrates the modal's usage pattern with state management for visibility and wraps content in
PotiTheme. Once themodifierparameter bug is fixed, the padding modifier passed at line 91 will work as expected.
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt
Show resolved
Hide resolved
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (1)
63-73: Window 설정 코드를SideEffect블록으로 감싸주세요.현재
dialogWindow설정 코드가 composition 중에 직접 실행되고 있습니다. 이는 매 recomposition마다 실행되며, Compose의 side-effect 처리 원칙에 맞지 않습니다. 또한Color.parseColor("#000000")대신Color.BLACK을 사용하면 더 간결합니다.♻️ SideEffect 적용 제안
Dialog( onDismissRequest = onDismissRequest, properties = dialogProperties, ) { val dialogWindow = (LocalView.current.parent as DialogWindowProvider).window - dialogWindow.setDimAmount(0.4f) - - dialogWindow.setBackgroundDrawable( - ColorDrawable(Color.TRANSPARENT), - ) + SideEffect { + dialogWindow.setDimAmount(0.4f) + dialogWindow.setBackgroundDrawable( + ColorDrawable(Color.TRANSPARENT), + ) + }
SideEffectimport 추가:import androidx.compose.runtime.SideEffect
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt
🧰 Additional context used
🧬 Code graph analysis (1)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (3)
app/src/main/java/com/poti/android/core/designsystem/theme/Theme.kt (1)
PotiTheme(48-71)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt (1)
PotiModalButton(25-59)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiFloatingButton.kt (1)
PotiFloatingButton(35-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR CI Check (Lint & Build)
🔇 Additional comments (3)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (3)
43-57: LGTM!함수 시그니처와
DialogProperties의remember사용이 적절합니다. dismissOnBackPress, dismissOnClickOutside를 key로 사용하여 불필요한 재생성을 방지한 점이 좋습니다.
67-73: 중복되거나 상충하는 배경 설정을 확인해 주세요.
decorView.setBackgroundColor(Color.BLACK)과setBackgroundDrawable(ColorDrawable(Color.TRANSPARENT))가 함께 설정되어 있습니다.setDimAmount(0.4f)가 이미 딤 효과를 처리하므로, 67-69번 줄의 검은색 배경 설정이 실제로 필요한지 확인이 필요합니다. 의도한 시각적 효과가 맞는지 검토해 주세요.
85-126: LGTM!Preview 구현이 적절합니다. 상태 관리와 모달 트리거 버튼을 통한 사용 예시가 잘 구성되어 있습니다.
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt
Show resolved
Hide resolved
| val dialogWindow = (LocalView.current.parent as DialogWindowProvider).window | ||
|
|
||
| dialogWindow.setDimAmount(0.4f) | ||
|
|
||
| dialogWindow.decorView.setBackgroundColor( | ||
| Color.parseColor("#000000"), | ||
| ) | ||
|
|
||
| dialogWindow.setBackgroundDrawable( | ||
| ColorDrawable(Color.TRANSPARENT), | ||
| ) |
There was a problem hiding this comment.
P1: 이 코드는 리컴포지션 때마다 계속 실행되기 때문에 컴포저블 안에 그냥 쓰면 안 돼요. SideEffect 블록으로 감싸서 사용해주세요!
| dialogWindow.decorView.setBackgroundColor( | ||
| Color.parseColor("#000000"), | ||
| ) | ||
|
|
||
| dialogWindow.setBackgroundDrawable( | ||
| ColorDrawable(Color.TRANSPARENT), | ||
| ) |
There was a problem hiding this comment.
안써주면 Surface 모서리 깎은 부분/패딩이 검정색이 되더라구욤,,
There was a problem hiding this comment.
어라랏 저는 안 뜨는데??
SideEffect {
dialogWindow?.setDimAmount(0.4f)
}이렇게 해도 뒤에 배경 안 나와요
There was a problem hiding this comment.
dialogWindow.decorView.setBackgroundColor(
Color.parseColor("#000000"),
)
검은 배경 이 코드 때문이엇음! 고로 필요없는 코드가 맞앗슴다
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(horizontal = 16.dp) | ||
| .padding(top = 36.dp, bottom = 12.dp), |
| btnText = "확인", | ||
| onBtnClick = {}, | ||
| modifier = Modifier | ||
| .padding(horizontal = 32.dp), |
There was a problem hiding this comment.
P1: 이거 large 모달일 경우 옆에 패딩 36.dp 고 small 모달일 경우는 32.dp 인데 지금 모달 나눠서 만들었으니까 컴포넌트에 고정시키는게 나을 것 같아요
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt:
- Around line 109-116: The condition using the intermediate boolean hasSubBtn
prevents Kotlin smart casts, causing nullable subBtnText and onSubBtnClick to
still be treated as nullable when calling PotiModalButton; update the if check
to directly test nullability (e.g., if (subBtnText != null && onSubBtnClick !=
null)) or explicitly assert non-null with !! when invoking PotiModalButton so
that subBtnText and onSubBtnClick are passed as non-null String and () -> Unit
(follow the pattern used in PotiBottomButton).
🧹 Nitpick comments (3)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (1)
64-71: Dialog window 설정이 적용되지 않는 경우에 대한 고려
LocalView.current.parent가DialogWindowProvider가 아닌 경우dialogWindow가 null이 되어 dim 및 배경 설정이 적용되지 않습니다. 현재 Compose Dialog 내에서 사용되므로 정상 동작하지만, 디버깅을 위해 로그를 추가하거나 주석으로 이 동작을 명시하는 것을 고려해 보세요.app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt (2)
41-41:@author태그가 비어 있습니다.문서화 일관성을 위해 작성자 이름을 추가하거나, 불필요한 경우 태그를 제거하세요.
90-99: 이미지 접근성을 위한contentDescription파라미터 추가 고려현재
contentDescription = null로 설정되어 있어 스크린 리더가 이미지를 설명하지 않습니다. 이미지가 순수하게 장식적인 경우에는 괜찮지만, 의미를 전달하는 경우 접근성에 문제가 될 수 있습니다.
imageContentDescription: String? = null파라미터를 추가하여 호출자가 접근성 설명을 제공할 수 있도록 하는 것을 고려해 보세요.♻️ 제안된 수정
@Composable fun PotiLargeModal( onDismissRequest: () -> Unit, title: String, text: String, btnText: String, onBtnClick: () -> Unit, modifier: Modifier = Modifier, subBtnText: String? = null, onSubBtnClick: (() -> Unit)? = null, @DrawableRes image: Int? = null, + imageContentDescription: String? = null, dismissOnBackPress: Boolean = true, dismissOnClickOutside: Boolean = true, content: (@Composable ColumnScope.() -> Unit)? = null, ) { // ... image?.let { Image( painter = painterResource(id = it), - contentDescription = null, + contentDescription = imageContentDescription, // ... ) } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.ktapp/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.ktapp/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt (3)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (1)
PotiModal(44-81)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt (1)
PotiModalButton(25-59)app/src/main/java/com/poti/android/core/designsystem/theme/Theme.kt (1)
PotiTheme(48-71)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (3)
app/src/main/java/com/poti/android/core/designsystem/theme/Theme.kt (1)
PotiTheme(48-71)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt (1)
PotiModalButton(25-59)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiFloatingButton.kt (1)
PotiFloatingButton(35-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR CI Check (Lint & Build)
🔇 Additional comments (4)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (2)
44-58: LGTM!
DialogProperties를remember로 캐싱하면서dismissOnBackPress,dismissOnClickOutside를 key로 사용한 것이 적절합니다. 값이 변경될 때만 새로운 인스턴스가 생성됩니다.
83-124: LGTM!Preview가 모달의 상태 토글과 실제 사용 패턴을 잘 보여주고 있습니다.
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt (2)
44-58: LGTM!함수 시그니처가 잘 설계되었습니다. 필수 파라미터와 선택적 파라미터가 명확히 구분되어 있고,
content람다가ColumnScope를 receiver로 받아 내부 정렬이 가능합니다.
122-156: LGTM!두 가지 주요 변형(lg-1: content + sub button, lg-2: image)을 모두 보여주는 Preview가 잘 구현되어 있습니다.
|
|
||
| PotiModal( | ||
| onDismissRequest = onDismissRequest, | ||
| modifier = modifier.padding(horizontal = 36.dp), |
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(horizontal = 16.dp) |
| contentDescription = null, | ||
| modifier = Modifier | ||
| .padding(vertical = 24.dp) | ||
| .size(180.dp), |
| ) { | ||
| PotiModal( | ||
| onDismissRequest = onDismissRequest, | ||
| modifier = modifier.padding(horizontal = 32.dp), |
| Column( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(all = 16.dp), |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt:
- Around line 112-119: The code passes nullable subBtnText and onSubBtnClick
into PotiModalButton even though it requires non-null parameters because the
smart cast on hasSubBtn doesn't apply; update the guard to directly check the
nullable variables (e.g., if (subBtnText != null && onSubBtnClick != null) {
PotiModalButton(text = subBtnText, onClick = onSubBtnClick, modifier =
Modifier.fillMaxWidth(), type = ModalButtonType.SUB_2) }) so the compiler can
smart-cast them to non-null, referencing the variables subBtnText,
onSubBtnClick, the boolean hasSubBtn, and the PotiModalButton invocation in
PotiLargeModal.kt.
🧹 Nitpick comments (1)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt (1)
61-61:hasSubBtn값을remember로 감싸는 것을 고려해 주세요.현재
hasSubBtn은 매 리컴포지션마다 재계산됩니다. 단순한 null 체크이므로 성능 영향은 미미하지만,subBtnText와onSubBtnClick이 변경될 때만 재계산되도록remember를 사용하면 Compose 모범 사례에 더 부합합니다.♻️ 제안하는 수정 사항
- val hasSubBtn = subBtnText != null && onSubBtnClick != null + val hasSubBtn = remember(subBtnText, onSubBtnClick) { + subBtnText != null && onSubBtnClick != null + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.ktapp/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: 코드를 리뷰할 때 다음 Kotlin 및 Jetpack Compose 모범 사례를 중점적으로 확인:
Jetpack Compose 성능 및 UI 최적화:
- Recomposition 방지: 불필요한 리컴포지션을 유발하는 코드를 지적해줘. 특히
LazyColumn등 리스트에서key파라미터가 누락되었는지 확인해줘.- 스마트한 계산: 계산 비용이 높은 로직은
remember나derivedStateOf로 감싸져 있는지 확인해줘.- 안정성(Stability): List나 일반 클래스를 State로 사용할 때, 불필요한 리컴포지션을 막기 위해
@Immutable이나@Stable어노테이션 사용을 제안해줘.- Modifier 규칙:
Modifier는 항상 컴포저블 함수의 첫 번째 선택적 파라미터로 받아야 하며, 재사용성을 위해 외부에서 주입받아야 해.- 리소스 사용: 하드코딩된 값(색상, 크기, 문자열) 대신
Theme,Dimens,strings.xml리소스를 사용하는지 체크해줘.- 접근성 예외: Icon이나 Image 컴포저블의
contentDescription이null인 경우는 장식용(Decorative) 이미지로 간주하고 지적하지 마.MVI (Model-View-Intent) 아키텍처 및 상태 관리:
- 단방향 데이터 흐름(UDF): UI는 상태를 직접 변경할 수 없고, 반드시
Intent(또는 Event)를 통해 ViewModel로 요청을 보내야 해.- 불변 상태(Immutable State):
UiState는 반드시data class+val프로퍼티로 구성되어야 하며, 상태 업데이트 시.copy()를 사용해야 해.- Side Effect 분리: 네비게이션, Toast 같은 일회성 이벤트는 UiState가 아닌
Channel이나SharedFlow로 처리하여 화면 회전 시 재발생하지 않도록 해야 해.- 안전한 수집: UI에서 Flow 수집 시
collectAsStateWithLifecycle()을 사용하여 생명주기를 준수하는지 확인해줘.- 상태 노출:
ViewModel내부에서StateFlow나SharedFlow를 올바르게 노출하고 있는지 확인해줘.- 호이스팅(Hoisting): 상태나 UI 이벤트 처리가 적절하게 호이스팅되었는지 확인해줘. UI 로직과 비즈니스 로직을 분리하고, UI가 로직을 직접 수행하지 않도록 확인해줘.
Clean Architecture (Dependency Rule) 준수:
- Domain 순수성: Domain 레이어(
domain패키지)는 절대로 안드로이드 의존성(android.*,R클래스 등)을 가져서는 안 돼. 순수 Kotlin 코드인지 엄격히 확인해줘.- 데이터 노출 금지: Data 레이어의 DTO(
@Entity,@SerializedName)가 UI나 Domain 레이어로 바로 노출되면 안 돼. 반드시 Mapper를 통해 변환했는지 확인해줘.- 계층 간 의존성:
UI -> Domain <- Data방향이 올바른지, UI가 Data 레이어를 직접 참조하고 있진 않은지 감시해줘.Kotlin & Coroutines 안전성:
- Scope 사용:
GlobalScope사용을 금지하고,viewModelScope나lifecycleScope사용을 권장해줘.- Dispatcher 주입:
Dispatchers.IO등을 하드코딩하지 말고, 테스트 용이성을 위해 생성자를 통해 주입(Injection)받도록 제안해줘.- 예외 처리: Coroutine 실행 시
runCatching이나CoroutineExceptionHandler등을 통해 예외 처리가 누락되지 않았는지 확인해줘.- Null-Safety: 불필요한
!!연산자 사용을 지적...
Files:
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt
🧬 Code graph analysis (1)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt (4)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (1)
PotiModal(44-81)app/src/main/java/com/poti/android/core/common/util/DimensionUtil.kt (1)
screenWidthDp(20-26)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt (1)
PotiModalButton(25-59)app/src/main/java/com/poti/android/core/designsystem/theme/Theme.kt (1)
PotiTheme(48-71)
🔇 Additional comments (2)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt (2)
27-45: 문서화가 잘 되어 있습니다!컴포넌트의 각 파라미터에 대한 KDoc 설명이 명확하게 작성되어 있어 사용자가 쉽게 이해할 수 있습니다.
@sample태그로 프리뷰 함수를 연결한 점도 좋습니다.참고로,
@author태그(Line 43)가 비어 있는데, 작성자 정보를 추가하시거나 필요 없으시면 제거하시는 것을 권장드립니다.
125-159: 프리뷰 함수가 잘 구성되어 있습니다!
PotiLarge1Preview는 content 슬롯 사용 케이스를,PotiLarge2Preview는 이미지 사용 케이스를 잘 보여주고 있습니다.PotiTheme으로 감싸서 테마가 적용된 상태로 프리뷰가 표시되도록 한 점도 Modern Android Development 권장 사항에 부합합니다.
| if (hasSubBtn) { | ||
| PotiModalButton( | ||
| text = subBtnText, | ||
| onClick = onSubBtnClick, | ||
| modifier = Modifier.fillMaxWidth(), | ||
| type = ModalButtonType.SUB_2, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "PotiLargeModal.kt" | head -5Repository: team-poti/POTI-ANDROID
Length of output: 154
🏁 Script executed:
find . -type f -name "*.kt" -path "*designsystem*" | grep -i modalRepository: team-poti/POTI-ANDROID
Length of output: 418
🏁 Script executed:
# Search for PotiModalButton definition
rg -n "fun PotiModalButton" --type=kt -A 5Repository: team-poti/POTI-ANDROID
Length of output: 92
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt | sed -n '100,130p'Repository: team-poti/POTI-ANDROID
Length of output: 1104
🏁 Script executed:
cat -n ./app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt | head -50Repository: team-poti/POTI-ANDROID
Length of output: 2419
🏁 Script executed:
# Get full view of PotiModalButton signature
cat ./app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt | head -100Repository: team-poti/POTI-ANDROID
Length of output: 3303
🏁 Script executed:
# Read full PotiLargeModal.kt to find function signature and parameter types
cat -n ./app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt | head -80Repository: team-poti/POTI-ANDROID
Length of output: 3734
🏁 Script executed:
# Check if there are any compilation issues or error reports in the codebase
find . -name "*.log" -o -name "build-log*" 2>/dev/null | head -5Repository: team-poti/POTI-ANDROID
Length of output: 48
🏁 Script executed:
# Search for similar patterns with hasSubBtn or sub button handling
rg "hasSubBtn" --type-list | head -20Repository: team-poti/POTI-ANDROID
Length of output: 455
🏁 Script executed:
# Look for other modal files that might show correct pattern
cat -n ./app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt | head -80Repository: team-poti/POTI-ANDROID
Length of output: 3679
🏁 Script executed:
# Search for actual usage patterns of PotiLargeModal to see if code compiles
rg "PotiLargeModal" -A 8 -B 2 --no-headingRepository: team-poti/POTI-ANDROID
Length of output: 3799
🏁 Script executed:
# Check if there are gradle build files that might show compilation settings
find . -name "build.gradle.kts" -o -name "build.gradle" | head -5Repository: team-poti/POTI-ANDROID
Length of output: 107
🏁 Script executed:
# Look at how parameters are passed when calling PotiLargeModal
rg "PotiLargeModal\(" -A 10 | head -50Repository: team-poti/POTI-ANDROID
Length of output: 3728
nullable 타입을 non-null 파라미터에 전달하고 있습니다.
hasSubBtn 변수로 null 체크를 했지만, Kotlin의 스마트 캐스트는 직접 null 검사한 변수에만 적용됩니다. 별도 boolean 변수 검사에서는 스마트 캐스트가 동작하지 않으므로, subBtnText는 String?로, onSubBtnClick은 (() -> Unit)?로 유지됩니다. 그런데 PotiModalButton은 각각 non-null String과 () -> Unit을 요구하므로 타입 불일치가 발생합니다.
아래 중 하나로 수정해주세요:
- 직접 null 검사로 스마트 캐스트 활성화:
if (subBtnText != null && onSubBtnClick != null) {
PotiModalButton(
text = subBtnText,
onClick = onSubBtnClick,
modifier = Modifier.fillMaxWidth(),
type = ModalButtonType.SUB_2,
)
}- Non-null assertion (권장하지 않음):
if (hasSubBtn) {
PotiModalButton(
text = subBtnText!!,
onClick = onSubBtnClick!!,
modifier = Modifier.fillMaxWidth(),
type = ModalButtonType.SUB_2,
)
}🤖 Prompt for AI Agents
In
@app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt
around lines 112 - 119, The code passes nullable subBtnText and onSubBtnClick
into PotiModalButton even though it requires non-null parameters because the
smart cast on hasSubBtn doesn't apply; update the guard to directly check the
nullable variables (e.g., if (subBtnText != null && onSubBtnClick != null) {
PotiModalButton(text = subBtnText, onClick = onSubBtnClick, modifier =
Modifier.fillMaxWidth(), type = ModalButtonType.SUB_2) }) so the compiler can
smart-cast them to non-null, referencing the variables subBtnText,
onSubBtnClick, the boolean hasSubBtn, and the PotiModalButton invocation in
PotiLargeModal.kt.
|
✅ Ktlint check passed. Run: https://github.com/team-poti/POTI-ANDROID/actions/runs/20963058836 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (2)
3-4: 사용되지 않는 import를 제거해 주세요.
android.graphics.Color와android.graphics.drawable.ColorDrawable이 import되어 있지만 실제 코드에서 사용되지 않고 있습니다. 코드 정리를 위해 제거하시는 것이 좋겠습니다.🧹 제안된 수정
package com.poti.android.core.designsystem.component.modal -import android.graphics.Color -import android.graphics.drawable.ColorDrawable import androidx.compose.foundation.layout.Arrangement
66-72: 하드코딩된 값들을 디자인 시스템 상수로 추출하는 것을 고려해 주세요.현재
dimAmount(0.4f)와RoundedCornerShape(12.dp)가 하드코딩되어 있습니다. 코딩 가이드라인에 따르면 이러한 값들은 Theme이나 Dimens 리소스로 관리하는 것이 좋습니다. 디자인 시스템의 일관성과 유지보수성을 위해 상수로 추출하시는 것을 권장드립니다.♻️ 제안된 수정 예시
// PotiModal.kt 상단 또는 별도 상수 파일에 정의 private const val MODAL_DIM_AMOUNT = 0.4f private val ModalCornerRadius = 12.dp // 사용 시 SideEffect { dialogWindow?.setDimAmount(MODAL_DIM_AMOUNT) } Surface( modifier = modifier, shape = RoundedCornerShape(ModalCornerRadius), color = PotiTheme.colors.white, )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: 코드를 리뷰할 때 다음 Kotlin 및 Jetpack Compose 모범 사례를 중점적으로 확인:
Jetpack Compose 성능 및 UI 최적화:
- Recomposition 방지: 불필요한 리컴포지션을 유발하는 코드를 지적해줘. 특히
LazyColumn등 리스트에서key파라미터가 누락되었는지 확인해줘.- 스마트한 계산: 계산 비용이 높은 로직은
remember나derivedStateOf로 감싸져 있는지 확인해줘.- 안정성(Stability): List나 일반 클래스를 State로 사용할 때, 불필요한 리컴포지션을 막기 위해
@Immutable이나@Stable어노테이션 사용을 제안해줘.- Modifier 규칙:
Modifier는 항상 컴포저블 함수의 첫 번째 선택적 파라미터로 받아야 하며, 재사용성을 위해 외부에서 주입받아야 해.- 리소스 사용: 하드코딩된 값(색상, 크기, 문자열) 대신
Theme,Dimens,strings.xml리소스를 사용하는지 체크해줘.- 접근성 예외: Icon이나 Image 컴포저블의
contentDescription이null인 경우는 장식용(Decorative) 이미지로 간주하고 지적하지 마.MVI (Model-View-Intent) 아키텍처 및 상태 관리:
- 단방향 데이터 흐름(UDF): UI는 상태를 직접 변경할 수 없고, 반드시
Intent(또는 Event)를 통해 ViewModel로 요청을 보내야 해.- 불변 상태(Immutable State):
UiState는 반드시data class+val프로퍼티로 구성되어야 하며, 상태 업데이트 시.copy()를 사용해야 해.- Side Effect 분리: 네비게이션, Toast 같은 일회성 이벤트는 UiState가 아닌
Channel이나SharedFlow로 처리하여 화면 회전 시 재발생하지 않도록 해야 해.- 안전한 수집: UI에서 Flow 수집 시
collectAsStateWithLifecycle()을 사용하여 생명주기를 준수하는지 확인해줘.- 상태 노출:
ViewModel내부에서StateFlow나SharedFlow를 올바르게 노출하고 있는지 확인해줘.- 호이스팅(Hoisting): 상태나 UI 이벤트 처리가 적절하게 호이스팅되었는지 확인해줘. UI 로직과 비즈니스 로직을 분리하고, UI가 로직을 직접 수행하지 않도록 확인해줘.
Clean Architecture (Dependency Rule) 준수:
- Domain 순수성: Domain 레이어(
domain패키지)는 절대로 안드로이드 의존성(android.*,R클래스 등)을 가져서는 안 돼. 순수 Kotlin 코드인지 엄격히 확인해줘.- 데이터 노출 금지: Data 레이어의 DTO(
@Entity,@SerializedName)가 UI나 Domain 레이어로 바로 노출되면 안 돼. 반드시 Mapper를 통해 변환했는지 확인해줘.- 계층 간 의존성:
UI -> Domain <- Data방향이 올바른지, UI가 Data 레이어를 직접 참조하고 있진 않은지 감시해줘.Kotlin & Coroutines 안전성:
- Scope 사용:
GlobalScope사용을 금지하고,viewModelScope나lifecycleScope사용을 권장해줘.- Dispatcher 주입:
Dispatchers.IO등을 하드코딩하지 말고, 테스트 용이성을 위해 생성자를 통해 주입(Injection)받도록 제안해줘.- 예외 처리: Coroutine 실행 시
runCatching이나CoroutineExceptionHandler등을 통해 예외 처리가 누락되지 않았는지 확인해줘.- Null-Safety: 불필요한
!!연산자 사용을 지적...
Files:
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt
🔇 Additional comments (2)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (2)
44-78: 잘 구현된 모달 컴포넌트입니다!
Modifier가 첫 번째 선택적 파라미터로 올바르게 배치되어 Compose 컨벤션을 잘 따르고 있습니다.DialogProperties를remember로 캐싱하고 적절한 key(dismissOnBackPress,dismissOnClickOutside)를 사용하여 불필요한 객체 재생성을 방지하고 있습니다.internal가시성을 사용하여 래퍼 모달들의 베이스 컴포넌트로 적절하게 설계되었습니다.content슬롯 패턴을 통해 유연한 확장성을 제공합니다.
80-121: Preview가 잘 구성되어 있습니다.
PotiTheme으로 감싸고 상태 토글을 통해 모달 동작을 시연하는 패턴이 적절합니다. 다른 디자인 시스템 컴포넌트(PotiFloatingButton,PotiModalButton)와의 연동도 잘 보여주고 있습니다.
sonyerim
left a comment
sonyerim
left a comment
There was a problem hiding this comment.
굳굳 저도 확장함수 적용하러 가보겠습니다 총총..
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (2)
62-66:SideEffect내 null 체크 개선을 고려해 보세요.현재
dialogWindow?.setDimAmount(0.4f)로 null-safe 호출을 하고 있지만,SideEffect는 매 리컴포지션마다 실행됩니다.dialogWindow가 유효할 때만 dim 설정을 하도록 명시적으로 처리하면 의도가 더 명확해집니다.또한, dim 값
0.4f를 상수로 추출하시면 향후 유지보수에 도움이 될 것 같습니다.♻️ 선택적 개선 제안
+private const val MODAL_DIM_AMOUNT = 0.4f + @Composable internal fun PotiModal( // ... ) { // ... Dialog( onDismissRequest = onDismissRequest, properties = dialogProperties, ) { val dialogWindow = (LocalView.current.parent as? DialogWindowProvider)?.window SideEffect { - dialogWindow?.setDimAmount(0.4f) + dialogWindow?.setDimAmount(MODAL_DIM_AMOUNT) } // ... } }
68-75:RoundedCornerShape객체 재생성 최적화를 고려해 보세요.
RoundedCornerShape(12.dp)는 매 리컴포지션마다 새로운 객체를 생성합니다. 디자인 시스템의 기반 컴포넌트로서 자주 사용될 것이므로, 상수로 추출하거나remember로 캐싱하시면 미세하지만 불필요한 객체 생성을 줄일 수 있습니다.♻️ 선택적 개선 제안
+private val ModalShape = RoundedCornerShape(12.dp) + @Composable internal fun PotiModal( // ... ) { // ... Surface( modifier = modifier, - shape = RoundedCornerShape(12.dp), + shape = ModalShape, color = PotiTheme.colors.white, ) { content() } // ... }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: 코드를 리뷰할 때 다음 Kotlin 및 Jetpack Compose 모범 사례를 중점적으로 확인:
Jetpack Compose 성능 및 UI 최적화:
- Recomposition 방지: 불필요한 리컴포지션을 유발하는 코드를 지적해줘. 특히
LazyColumn등 리스트에서key파라미터가 누락되었는지 확인해줘.- 스마트한 계산: 계산 비용이 높은 로직은
remember나derivedStateOf로 감싸져 있는지 확인해줘.- 안정성(Stability): List나 일반 클래스를 State로 사용할 때, 불필요한 리컴포지션을 막기 위해
@Immutable이나@Stable어노테이션 사용을 제안해줘.- Modifier 규칙:
Modifier는 항상 컴포저블 함수의 첫 번째 선택적 파라미터로 받아야 하며, 재사용성을 위해 외부에서 주입받아야 해.- 리소스 사용: 하드코딩된 값(색상, 크기, 문자열) 대신
Theme,Dimens,strings.xml리소스를 사용하는지 체크해줘.- 접근성 예외: Icon이나 Image 컴포저블의
contentDescription이null인 경우는 장식용(Decorative) 이미지로 간주하고 지적하지 마.MVI (Model-View-Intent) 아키텍처 및 상태 관리:
- 단방향 데이터 흐름(UDF): UI는 상태를 직접 변경할 수 없고, 반드시
Intent(또는 Event)를 통해 ViewModel로 요청을 보내야 해.- 불변 상태(Immutable State):
UiState는 반드시data class+val프로퍼티로 구성되어야 하며, 상태 업데이트 시.copy()를 사용해야 해.- Side Effect 분리: 네비게이션, Toast 같은 일회성 이벤트는 UiState가 아닌
Channel이나SharedFlow로 처리하여 화면 회전 시 재발생하지 않도록 해야 해.- 안전한 수집: UI에서 Flow 수집 시
collectAsStateWithLifecycle()을 사용하여 생명주기를 준수하는지 확인해줘.- 상태 노출:
ViewModel내부에서StateFlow나SharedFlow를 올바르게 노출하고 있는지 확인해줘.- 호이스팅(Hoisting): 상태나 UI 이벤트 처리가 적절하게 호이스팅되었는지 확인해줘. UI 로직과 비즈니스 로직을 분리하고, UI가 로직을 직접 수행하지 않도록 확인해줘.
Clean Architecture (Dependency Rule) 준수:
- Domain 순수성: Domain 레이어(
domain패키지)는 절대로 안드로이드 의존성(android.*,R클래스 등)을 가져서는 안 돼. 순수 Kotlin 코드인지 엄격히 확인해줘.- 데이터 노출 금지: Data 레이어의 DTO(
@Entity,@SerializedName)가 UI나 Domain 레이어로 바로 노출되면 안 돼. 반드시 Mapper를 통해 변환했는지 확인해줘.- 계층 간 의존성:
UI -> Domain <- Data방향이 올바른지, UI가 Data 레이어를 직접 참조하고 있진 않은지 감시해줘.Kotlin & Coroutines 안전성:
- Scope 사용:
GlobalScope사용을 금지하고,viewModelScope나lifecycleScope사용을 권장해줘.- Dispatcher 주입:
Dispatchers.IO등을 하드코딩하지 말고, 테스트 용이성을 위해 생성자를 통해 주입(Injection)받도록 제안해줘.- 예외 처리: Coroutine 실행 시
runCatching이나CoroutineExceptionHandler등을 통해 예외 처리가 누락되지 않았는지 확인해줘.- Null-Safety: 불필요한
!!연산자 사용을 지적...
Files:
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt
🧬 Code graph analysis (1)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (3)
app/src/main/java/com/poti/android/core/designsystem/theme/Theme.kt (1)
PotiTheme(48-71)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt (1)
PotiModalButton(25-59)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiFloatingButton.kt (1)
PotiFloatingButton(35-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR CI Check (Lint & Build)
🔇 Additional comments (3)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (3)
42-48: LGTM! Modifier 파라미터 위치가 올바릅니다.Jetpack Compose 컨벤션에 따라
Modifier가 첫 번째 선택적 파라미터로 올바르게 배치되어 있습니다.onDismissRequest는 필수 파라미터이므로 먼저 오는 것이 적절합니다.
50-56:remember를 활용한DialogProperties캐싱이 잘 되어 있습니다.
dismissOnBackPress와dismissOnClickOutside를 키로 사용하여 불필요한 재생성을 방지하고 있습니다. Modern Android Development 권장 사항을 잘 따르고 계시네요.
78-119: Preview 함수가 잘 구성되어 있습니다.
private접근 제한자를 사용하여 Preview가 외부로 노출되지 않도록 했고,PotiTheme으로 감싸서 테마가 올바르게 적용되고 있습니다. 실제 사용 시나리오를 잘 보여주는 좋은 Preview입니다.
| .heightForScreenPercentage(180.dp) | ||
| .widthForScreenPercentage(180.dp), |
There was a problem hiding this comment.
P1: 요거는 정사각형을 의도한 것 같아서 .widthForScreenPercentage(180.dp)를 aspectRatio를 1f로 설정해주세요!
| Row( | ||
| modifier = Modifier | ||
| .padding(top = 24.dp), | ||
| horizontalArrangement = Arrangement.spacedBy(8.dp), |
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding( | ||
| vertical = screenHeightDp(16.dp), |
There was a problem hiding this comment.
P3: 요기 근데 너비에 맞춰서 vertical이랑 horizontal이랑 다 screenWidthDp(16.dp)로 줘도 될 거 가타욤
There was a problem hiding this comment.
옹 패딩이 똑같이 적용되는거니까 all로 넣어주는게 맞겟네요!
doyeon0307
force-pushed
the
feat/#19-modal-component
branch
from
c70482c to
e8b4178
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt (1)
67-84: 패딩 값 사용 방식이 일관되지 않습니다.Line 63에서는
screenWidthDp(16.dp)를 사용하여 반응형 사이징을 적용하셨는데, Line 70-71, 83에서는 하드코딩된16.dp,8.dp,24.dp를 직접 사용하고 계십니다. 다양한 화면 크기에서 일관된 UI 스케일링을 위해screenWidthDp()를 일관되게 사용하시거나, 디자인 토큰으로 추출하시는 것을 권장드립니다.♻️ 일관된 반응형 패딩 적용 제안
Text( text = title, modifier = Modifier - .padding(top = 16.dp, bottom = 8.dp), + .padding(top = screenWidthDp(16.dp), bottom = screenWidthDp(8.dp)), color = PotiTheme.colors.black, style = PotiTheme.typography.body16sb, ) ... Row( modifier = Modifier - .padding(top = 24.dp), + .padding(top = screenWidthDp(24.dp)), horizontalArrangement = Arrangement.spacedBy(screenWidthDp(8.dp)), )app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (2)
62-66:SideEffect사용이 적절합니다만, dim 값 추출을 고려해 주세요.Dialog window의 속성을 수정하기 위해
SideEffect를 사용하신 것은 올바른 패턴입니다. 다만0.4f라는 매직 넘버는 상수로 추출하시면 향후 디자인 변경 시 관리가 용이해집니다.♻️ 상수 추출 제안
private const val MODAL_DIM_AMOUNT = 0.4f // 사용 시 dialogWindow?.setDimAmount(MODAL_DIM_AMOUNT)
68-75: 테마 색상 사용이 좋습니다. 모서리 반경도 디자인 토큰으로 관리하시면 더 좋을 것 같습니다.
PotiTheme.colors.white를 사용하여 테마 시스템을 잘 활용하고 계십니다.RoundedCornerShape(12.dp)역시 디자인 시스템의 일관성을 위해 별도 상수나 테마 값으로 추출하시는 것을 권장드립니다.♻️ 모서리 반경 상수화 제안
private val ModalCornerRadius = 12.dp // 사용 시 shape = RoundedCornerShape(ModalCornerRadius),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.ktapp/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.ktapp/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiLargeModal.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
⚙️ CodeRabbit configuration file
**/*.kt: 코드를 리뷰할 때 다음 Kotlin 및 Jetpack Compose 모범 사례를 중점적으로 확인:
Jetpack Compose 성능 및 UI 최적화:
- Recomposition 방지: 불필요한 리컴포지션을 유발하는 코드를 지적해줘. 특히
LazyColumn등 리스트에서key파라미터가 누락되었는지 확인해줘.- 스마트한 계산: 계산 비용이 높은 로직은
remember나derivedStateOf로 감싸져 있는지 확인해줘.- 안정성(Stability): List나 일반 클래스를 State로 사용할 때, 불필요한 리컴포지션을 막기 위해
@Immutable이나@Stable어노테이션 사용을 제안해줘.- Modifier 규칙:
Modifier는 항상 컴포저블 함수의 첫 번째 선택적 파라미터로 받아야 하며, 재사용성을 위해 외부에서 주입받아야 해.- 리소스 사용: 하드코딩된 값(색상, 크기, 문자열) 대신
Theme,Dimens,strings.xml리소스를 사용하는지 체크해줘.- 접근성 예외: Icon이나 Image 컴포저블의
contentDescription이null인 경우는 장식용(Decorative) 이미지로 간주하고 지적하지 마.MVI (Model-View-Intent) 아키텍처 및 상태 관리:
- 단방향 데이터 흐름(UDF): UI는 상태를 직접 변경할 수 없고, 반드시
Intent(또는 Event)를 통해 ViewModel로 요청을 보내야 해.- 불변 상태(Immutable State):
UiState는 반드시data class+val프로퍼티로 구성되어야 하며, 상태 업데이트 시.copy()를 사용해야 해.- Side Effect 분리: 네비게이션, Toast 같은 일회성 이벤트는 UiState가 아닌
Channel이나SharedFlow로 처리하여 화면 회전 시 재발생하지 않도록 해야 해.- 안전한 수집: UI에서 Flow 수집 시
collectAsStateWithLifecycle()을 사용하여 생명주기를 준수하는지 확인해줘.- 상태 노출:
ViewModel내부에서StateFlow나SharedFlow를 올바르게 노출하고 있는지 확인해줘.- 호이스팅(Hoisting): 상태나 UI 이벤트 처리가 적절하게 호이스팅되었는지 확인해줘. UI 로직과 비즈니스 로직을 분리하고, UI가 로직을 직접 수행하지 않도록 확인해줘.
Clean Architecture (Dependency Rule) 준수:
- Domain 순수성: Domain 레이어(
domain패키지)는 절대로 안드로이드 의존성(android.*,R클래스 등)을 가져서는 안 돼. 순수 Kotlin 코드인지 엄격히 확인해줘.- 데이터 노출 금지: Data 레이어의 DTO(
@Entity,@SerializedName)가 UI나 Domain 레이어로 바로 노출되면 안 돼. 반드시 Mapper를 통해 변환했는지 확인해줘.- 계층 간 의존성:
UI -> Domain <- Data방향이 올바른지, UI가 Data 레이어를 직접 참조하고 있진 않은지 감시해줘.Kotlin & Coroutines 안전성:
- Scope 사용:
GlobalScope사용을 금지하고,viewModelScope나lifecycleScope사용을 권장해줘.- Dispatcher 주입:
Dispatchers.IO등을 하드코딩하지 말고, 테스트 용이성을 위해 생성자를 통해 주입(Injection)받도록 제안해줘.- 예외 처리: Coroutine 실행 시
runCatching이나CoroutineExceptionHandler등을 통해 예외 처리가 누락되지 않았는지 확인해줘.- Null-Safety: 불필요한
!!연산자 사용을 지적...
Files:
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.ktapp/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt
🧬 Code graph analysis (1)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (3)
app/src/main/java/com/poti/android/core/designsystem/theme/Theme.kt (1)
PotiTheme(48-71)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiModalButton.kt (1)
PotiModalButton(25-59)app/src/main/java/com/poti/android/core/designsystem/component/button/PotiFloatingButton.kt (1)
PotiFloatingButton(35-83)
🔇 Additional comments (4)
app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiSmallModal.kt (2)
40-52: LGTM! 함수 시그니처가 잘 구성되어 있습니다.필수 파라미터들이 먼저 오고,
Modifier가 첫 번째 선택적 파라미터로 배치되어 Kotlin 및 Compose 컨벤션을 준수하고 있습니다.dismissOnBackPress와dismissOnClickOutside플래그로 dismiss 동작을 세밀하게 제어할 수 있어 좋습니다.
106-133: Preview 구현이 깔끔합니다.
remember와mutableStateOf를 활용한 상태 토글 패턴이 적절하며,PotiTheme으로 감싸서 테마 적용을 확인할 수 있도록 구성하셨네요. Preview 함수가private으로 선언되어 있어 외부 노출을 방지한 점도 좋습니다.app/src/main/java/com/poti/android/core/designsystem/component/modal/PotiModal.kt (2)
42-56: LGTM!internal가시성과remember를 통한 최적화가 잘 적용되어 있습니다.기본 모달 컴포넌트를
internal로 선언하여 모듈 내부에서만 사용하도록 캡슐화한 점이 좋습니다.DialogProperties를remember로 감싸고dismissOnBackPress,dismissOnClickOutside를 키로 사용하여 불필요한 재생성을 방지하신 것도 적절합니다.
78-119: Preview가 컴포넌트 사용 방법을 잘 보여주고 있습니다.
@sample태그로 KDoc에 연결된 Preview가 기본적인 사용 패턴을 명확하게 보여주고 있어 좋습니다.private선언과PotiTheme래핑도 적절합니다.
3 participants
Related issue 🛠️
Work Description ✏️
Screenshot 📸
기본 모달
default.mp4
스몰 모달
default.mp4
Uncompleted Tasks 😅
N/A
To Reviewers 📢
기본 Dialog 사용 시 scrimColor 커스텀이 불가해 Popup을 활용했습니다.모달 컴포넌트는 화면 전체를 덮는 팝업이며, 팝업 content로서 딤 영역은 Box, 모달 영역은 Box의 content인 Surface에 해당됩니다.
추가 사항
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.