Merged
Conversation
Closed
1 task
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
📝 WalkthroughWalkthrough온보딩 흐름에 마케팅 정보 수신 동의를 처리하는 새로운 기능이 추가되었습니다. 디자인 시스템에 MarketingDialog 컴포넌트와 관련 UI 리소스(벡터 드로어블, 문자열)가 추가되었으며, CoupleConnectRoute에 다이얼로그 표시 및 알림 권한 확인 로직이 통합되었습니다. OnBoardingIntent에 SubmitMarketingConsent 인텐트가 추가되었고, OnBoardingViewModel의 initNotificationSettings 메서드가 사용자 선택값을 받아 처리하도록 수정되었습니다. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 주요 검토 사항MarketingDialog 컴포넌트 설계마케팅 다이얼로그 구현이 명확하고 재사용 가능한 구조로 설계된 점이 좋습니다. 다만 다음을 확인해주세요: 질문: 사용자가 다이얼로그를 명시적으로 거절하는 경우 (다이얼로그를 닫기)의 처리 방식은 어떻게 되나요? 현재 구현에서는 권한 처리 로직 검증CoupleConnectRoute에서 확인 포인트:
상태 관리 흐름의 일관성Intent 기반의 단방향 데이터 흐름 구조가 명확합니다: 제안: SubmitMarketingConsent 인텐트에서 세 개의 Boolean 파라미터를 받고 있는데, 파라미터 이름이 매우 유사합니다. 각 파라미터의 의미를 명확히 하는 것을 권장합니다:
이러한 의미가 코드에 충분히 반영되어 있는지 확인해 주세요. 문자열 리소스 검토한국어 문자열들이 일관되게 추가되었습니다. 다이얼로그 설명 문자열 CoupleConnectScreenPreview 가시성 변경프리뷰 함수를 🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt (1)
63-64: 사용되지 않는currentContext변수
currentContext가rememberUpdatedState로 생성되었지만, Line 100에서context가 직접 사용되고 있습니다.rememberUpdatedState는 콜백 내에서 최신 값을 참조하기 위해 사용되는데, 여기서는 일관성을 위해currentContext를 사용하거나, 불필요하다면 제거하는 것이 좋겠습니다.♻️ 수정 제안
val context = LocalContext.current - val currentContext by rememberUpdatedState(context) // ... MarketingDialog( visible = showMarketingDialog, onConfirm = { marketing, nightMarketing -> showMarketingDialog = false val isPushEnabled = isNotificationPermissionGranted(context)또는 콜백 내에서 최신 context가 필요하다면:
MarketingDialog( visible = showMarketingDialog, onConfirm = { marketing, nightMarketing -> showMarketingDialog = false - val isPushEnabled = isNotificationPermissionGranted(context) + val isPushEnabled = isNotificationPermissionGranted(currentContext)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt` around lines 63 - 64, The variable currentContext created with rememberUpdatedState(LocalContext.current) is unused while the code continues to reference context directly; either replace uses of context inside any callbacks (e.g., lambdas that may run later) with currentContext.value to ensure the latest Context is read, or remove the rememberUpdatedState call and the currentContext declaration if you don't need a stable reference for callbacks; update references to LocalContext.current, currentContext, rememberUpdatedState and any callback/lambda that currently uses context accordingly.core/design-system/src/main/java/com/twix/designsystem/components/dialog/MarketingDialog.kt (1)
114-134: 클릭 영역 확장 고려현재 클릭 영역이 아이콘(24dp)에만 적용되어 있습니다. UX 개선을 위해 텍스트를 포함한 전체 Row를 클릭 가능하게 만드는 것을 고려해 보시는 것은 어떨까요?
💡 클릭 영역 확장 제안
Row( verticalAlignment = Alignment.CenterVertically, - modifier = Modifier.fillMaxWidth(), + modifier = Modifier + .fillMaxWidth() + .noRippleClickable(onClick = onClick), ) { Image( painter = painterResource(icon), contentDescription = "$text $checkboxDescription", - modifier = - Modifier - .size(24.dp) - .noRippleClickable(onClick = onClick), + modifier = Modifier.size(24.dp), )🤖 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/dialog/MarketingDialog.kt` around lines 114 - 134, The Row's click handling is currently applied only to the Image via .noRippleClickable(onClick) so expand the tappable area by moving or adding the click handler to the Row (e.g., apply Modifier.noRippleClickable(onClick) on the Row's modifier or wrap the Row with a clickable container) so that the entire Row including AppText responds to onClick; keep the Image size and spacing as-is and ensure you remove the duplicate .noRippleClickable from Image to avoid overlapping handlers.feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt (1)
152-161: 에러 처리 누락 확인 필요
initNotificationSettings호출에서onSuccess가 빈 람다이고onError핸들러가 정의되어 있지 않습니다. 알림 설정 초기화가 실패할 경우, 사용자에게 피드백이 전달되지 않아 마케팅 동의가 실제로 저장되었는지 알 수 없습니다.의도적인 fire-and-forget 패턴이라면 괜찮지만, 사용자 경험을 위해 실패 시 토스트 메시지나 재시도 로직을 고려해 보시는 것은 어떨까요?
💡 에러 처리 추가 제안
private fun initNotificationSettings( isPushEnabled: Boolean, isMarketingEnabled: Boolean, isNightMarketingEnabled: Boolean, ) { launchResult( block = { notificationRepository.initNotificationSettings( isPushEnabled, isMarketingEnabled, isNightMarketingEnabled, ) }, onSuccess = {}, + onError = { + // 필요시 에러 처리 로직 추가 + // emitSideEffect(OnBoardingSideEffect.Marketing.ShowSettingsFailToast) + }, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt` around lines 152 - 161, The current call to launchResult uses notificationRepository.initNotificationSettings with an empty onSuccess and no onError, so failures are silent; update the launchResult invocation that wraps notificationRepository.initNotificationSettings to provide a non-empty onSuccess (e.g., confirm UI state or show success toast) and add an onError lambda that handles failures (show a toast/error dialog and optionally trigger a retry or revert local state). Specifically modify the launchResult call site (the block invoking initNotificationSettings) to supply onSuccess and onError handlers that call your ViewModel methods or UI event dispatcher to surface errors to the user.core/design-system/src/main/res/drawable/ic_empty_check.xml (1)
1-18: 하드코딩된 색상 및 다크 모드 미지원 확인 필요현재 색상이 drawable 내에 직접 하드코딩되어 있습니다 (
#ffffff,#C6C6C6). 디자인 시스템 가이드라인에 따르면, 색상은 테마에서 참조하는 것이 권장됩니다. 또한 체크마크 stroke 색상이#ffffff로 배경 fill 색상과 동일하여 체크마크가 보이지 않을 수 있는데, 이것이 "빈 체크박스" 상태를 의도한 것인지 확인 부탁드립니다.다크 모드 지원을 위해
res/drawable-night/폴더에 별도의 리소스를 추가하거나, 테마 색상을 참조하는 방식을 고려해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/res/drawable/ic_empty_check.xml` around lines 1 - 18, The drawable currently hardcodes colors in the vector paths (first path's android:fillColor and android:strokeColor using `#ffffff` and `#C6C6C6`, and the check path's android:strokeColor using `#ffffff`) which breaks theming and can make the check invisible; update both path elements to reference theme attributes (e.g., use ?attr/colorSurface / ?attr/colorOnSurface or a custom color attribute like ?attr/dsCheckboxBorderColor and ?attr/dsCheckboxCheckColor) instead of hex literals, and ensure the check stroke color differs from the fill so an “empty” vs “checked” state is visually correct; also provide a dark-mode variant (or ensure the theme attrs adapt in night mode) so colors render correctly in dark theme.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/design-system/src/main/java/com/twix/designsystem/components/dialog/MarketingDialog.kt`:
- Around line 118-125: The Image composable currently sets contentDescription =
null which prevents screen readers from announcing the checkbox state; update
the Image in MarketingDialog.kt (the Image using painterResource(icon) and
noRippleClickable(onClick)) to supply a meaningful description that reflects the
current checked state (e.g., use an existing state variable like isChecked or
checkedState to choose between "checked" / "unchecked" or better, use
stringResource(R.string.xxx_checked) and stringResource(R.string.xxx_unchecked)
for localization), so accessibility tools can convey the checkbox status.
---
Nitpick comments:
In
`@core/design-system/src/main/java/com/twix/designsystem/components/dialog/MarketingDialog.kt`:
- Around line 114-134: The Row's click handling is currently applied only to the
Image via .noRippleClickable(onClick) so expand the tappable area by moving or
adding the click handler to the Row (e.g., apply
Modifier.noRippleClickable(onClick) on the Row's modifier or wrap the Row with a
clickable container) so that the entire Row including AppText responds to
onClick; keep the Image size and spacing as-is and ensure you remove the
duplicate .noRippleClickable from Image to avoid overlapping handlers.
In `@core/design-system/src/main/res/drawable/ic_empty_check.xml`:
- Around line 1-18: The drawable currently hardcodes colors in the vector paths
(first path's android:fillColor and android:strokeColor using `#ffffff` and
`#C6C6C6`, and the check path's android:strokeColor using `#ffffff`) which breaks
theming and can make the check invisible; update both path elements to reference
theme attributes (e.g., use ?attr/colorSurface / ?attr/colorOnSurface or a
custom color attribute like ?attr/dsCheckboxBorderColor and
?attr/dsCheckboxCheckColor) instead of hex literals, and ensure the check stroke
color differs from the fill so an “empty” vs “checked” state is visually
correct; also provide a dark-mode variant (or ensure the theme attrs adapt in
night mode) so colors render correctly in dark theme.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.kt`:
- Around line 63-64: The variable currentContext created with
rememberUpdatedState(LocalContext.current) is unused while the code continues to
reference context directly; either replace uses of context inside any callbacks
(e.g., lambdas that may run later) with currentContext.value to ensure the
latest Context is read, or remove the rememberUpdatedState call and the
currentContext declaration if you don't need a stable reference for callbacks;
update references to LocalContext.current, currentContext, rememberUpdatedState
and any callback/lambda that currently uses context accordingly.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt`:
- Around line 152-161: The current call to launchResult uses
notificationRepository.initNotificationSettings with an empty onSuccess and no
onError, so failures are silent; update the launchResult invocation that wraps
notificationRepository.initNotificationSettings to provide a non-empty onSuccess
(e.g., confirm UI state or show success toast) and add an onError lambda that
handles failures (show a toast/error dialog and optionally trigger a retry or
revert local state). Specifically modify the launchResult call site (the block
invoking initNotificationSettings) to supply onSuccess and onError handlers that
call your ViewModel methods or UI event dispatcher to surface errors to the
user.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/design-system/src/main/java/com/twix/designsystem/components/dialog/MarketingDialog.ktcore/design-system/src/main/res/drawable/ic_empty_check.xmlcore/design-system/src/main/res/values/strings.xmlfeature/onboarding/src/main/java/com/twix/onboarding/couple/CoupleConnectRoute.ktfeature/onboarding/src/main/java/com/twix/onboarding/model/OnBoardingIntent.ktfeature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt
core/design-system/src/main/java/com/twix/designsystem/components/dialog/MarketingDialog.kt
Show resolved
Hide resolved
dogmania
approved these changes
Feb 26, 2026
| modifier = | ||
| Modifier | ||
| .size(24.dp) | ||
| .noRippleClickable(onClick = onClick), |
Member
There was a problem hiding this comment.
클릭 영역을 텍스트를 클릭해도 가능하도록 Row 전체로 잡아도 좋을 것 같아요!
# Conflicts: # core/design-system/src/main/java/com/twix/designsystem/components/dialog/MarketingDialog.kt
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.
이슈 번호
#114
작업내용
결과물
리뷰어에게 추가로 요구하는 사항 (선택)
온보딩 갈 수 있는 계정이 카카오 계정뿐인데 계속 테스트하다가 차단당함...
몇 번 테스트해서 기능 구현은 다된거 같은데 디테일하게 못해가지구 확인 부탁해 ㅠㅠ