[Feat/#266] 이어쓰기 알림설정을 추가한 UI를 구현합니다. #267
Conversation
- 컴포넌트 개선 : 각 알람 종류에 따라 별개였던 토글 스위치 컴포넌트 단일화, 알람 시간 선택 및 피커 부분 일부 개선 - 상태 호이스팅 : 뷰모델 관련한 부분 전부 Route로 끌어올려 Stateless 하게 호이스팅. 책임분리 및 가독성 향상
|
""" WalkthroughThis update introduces a "draft alarm" (이어쓰기 알림) toggle to the notification settings UI and underlying logic. It adds new Boolean fields in relevant DTOs, updates the ViewModel and UI components to support the new alarm, and refactors the notification setting screen for improved modularity and clarity. Supporting string resources and time conversion utilities are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NotificationSettingScreen
participant NotificationSettingViewModel
participant Server
User->>NotificationSettingScreen: Toggle "Draft Alarm" switch
NotificationSettingScreen->>NotificationSettingViewModel: onClickDraftSwitch()
NotificationSettingViewModel->>NotificationSettingViewModel: Toggle draftAlarm state
NotificationSettingViewModel->>Server: Send updated notification settings (incl. isDraftAlarm)
Server-->>NotificationSettingViewModel: Response (success/failure)
NotificationSettingViewModel-->>NotificationSettingScreen: Update UI state
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt (1)
92-128: 🛠️ Refactor suggestionSignificant code duplication across alarm change methods.
The three alarm change methods (
changeDiaryAlarm,changeDraftAlarm,changeReplyAlarm) have nearly identical implementations with only the toggled alarm state being different. This violates the DRY principle.Consider refactoring to a single method:
+ enum class AlarmType { + DIARY, DRAFT, REPLY + } + + fun changeAlarmState(context: Context, alarmType: AlarmType) { + _notificationChangeState.value = NotificationChangeState.Loading + viewModelScope.launch { + if (!networkUtil.isNetworkAvailable()) { + _failureDialogMessage.value = FAILURE_NETWORK_MESSAGE + _showFailureDialog.value = true + return@launch + } + val fcmToken = getTokenFromPreferences(context) + if (fcmToken.isNullOrBlank()) { + _notificationChangeState.value = NotificationChangeState.Failure("FCM Token을 가져오는데 실패했습니다.") + return@launch + } + + val (isDiaryAlarm, isDraftAlarm, isReplyAlarm) = when (alarmType) { + AlarmType.DIARY -> Triple(!_diaryAlarm.value, _draftAlarm.value, _replyAlarm.value) + AlarmType.DRAFT -> Triple(_diaryAlarm.value, !_draftAlarm.value, _replyAlarm.value) + AlarmType.REPLY -> Triple(_diaryAlarm.value, _draftAlarm.value, !_replyAlarm.value) + } + + val requestDto = SendNotificationRequestDto( + isDiaryAlarm = isDiaryAlarm, + isDraftAlarm = isDraftAlarm, + isReplyAlarm = isReplyAlarm, + time = _notificationTime.value, + fcmToken = fcmToken, + ) + notificationRepository.sendNotification(requestDto).fold( + onSuccess = { + _notificationChangeState.value = NotificationChangeState.Success(it) + getNotificationInfo() + }, + onFailure = { + _failureDialogMessage.value = if (it.message?.contains("200") == false) { + FAILURE_TEMPORARY_MESSAGE + } else { + UNKNOWN_ERROR + } + _showFailureDialog.value = true + _notificationChangeState.value = NotificationChangeState.Failure(_failureDialogMessage.value) + }, + ) + } + }Then replace the individual methods with delegating calls or remove them entirely and update the UI to use the new method.
Also applies to: 130-166, 206-242
🧹 Nitpick comments (3)
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt (3)
54-57: Consider moving the initial data fetch out of the init block.Triggering network calls in the init block can lead to unnecessary resource usage. Consider calling
getNotificationInfo()from the UI layer when the screen is ready to display data.Remove the init block and let the UI trigger the initial data fetch:
- init { - getNotificationInfo() - }Then call
getNotificationInfo()from theNotificationSettingRoutewhen the composable enters the composition.
253-256: Consider caching the FCM token.The FCM token is retrieved from SharedPreferences in multiple methods. Since FCM tokens don't change frequently during a session, consider caching it to avoid repeated SharedPreferences access.
+ private var cachedFcmToken: String? = null + private fun getTokenFromPreferences(context: Context): String? { + cachedFcmToken?.let { return it } val sharedPreferences = context.getSharedPreferences("fcm_prefs", Context.MODE_PRIVATE) - return sharedPreferences.getString("fcm_token", null) + return sharedPreferences.getString("fcm_token", null).also { + cachedFcmToken = it + } }
244-246: Add reset method for notification change state.For consistency with
resetNotificationTimeChangeState(), consider adding a reset method fornotificationChangeState.fun resetNotificationTimeChangeState() { _notificationTimeChangeState.value = NotificationTimeChangeState.Idle } + + fun resetNotificationChangeState() { + _notificationChangeState.value = NotificationChangeState.Idle + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/src/main/java/com/sopt/clody/data/remote/dto/request/SendNotificationRequestDto.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/dto/response/NotificationInfoResponseDto.kt(1 hunks)app/src/main/java/com/sopt/clody/data/remote/dto/response/SendNotificationResponseDto.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderViewModel.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSettingTimePicker.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSwitch.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationTimeSelector.kt(2 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/ReplyAlarmSwitch.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/DiaryAlarmChangeState.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationChangeState.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt(4 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt(7 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/ReplyAlarmChangeState.kt(0 hunks)app/src/main/java/com/sopt/clody/presentation/utils/extension/StringExt.kt(1 hunks)app/src/main/res/values/strings.xml(1 hunks)
💤 Files with no reviewable changes (3)
- app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/DiaryAlarmChangeState.kt
- app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/ReplyAlarmChangeState.kt
- app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/ReplyAlarmSwitch.kt
🧰 Additional context used
🧬 Code Graph Analysis (1)
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt (7)
app/src/main/java/com/sopt/clody/presentation/ui/component/popup/ClodyPopupBottomSheet.kt (1)
ClodyPopupBottomSheet(28-77)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSettingTimePicker.kt (1)
NotificationSettingTimePicker(34-155)app/src/main/java/com/sopt/clody/presentation/ui/setting/component/SettingTopAppBar.kt (1)
SettingTopAppBar(15-38)app/src/main/java/com/sopt/clody/presentation/ui/component/LoadingScreen.kt (1)
LoadingScreen(15-30)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSwitch.kt (1)
NotificationSwitch(20-56)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationTimeSelector.kt (1)
NotificationTimeSelector(19-54)app/src/main/java/com/sopt/clody/presentation/ui/component/FailureScreen.kt (1)
FailureScreen(27-69)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (17)
app/src/main/res/values/strings.xml (1)
130-130: LGTM! Well-integrated string resource addition.The new string resource follows the existing naming convention and properly supports the draft diary notification feature. The Korean text is clear and consistent with the app's terminology.
app/src/main/java/com/sopt/clody/presentation/ui/auth/timereminder/TimeReminderViewModel.kt (1)
48-48: LGTM! Consistent initialization of draft alarm permission.The addition properly initializes the draft alarm flag with the same permission state as other notification types, maintaining logical consistency in the initial setup flow.
app/src/main/java/com/sopt/clody/data/remote/dto/response/NotificationInfoResponseDto.kt (1)
9-9: LGTM! Proper DTO extension for draft alarm support.The new property follows the established pattern with correct serialization annotation and appropriate Boolean type. This properly extends the notification info response to support the draft alarm feature.
app/src/main/java/com/sopt/clody/data/remote/dto/response/SendNotificationResponseDto.kt (1)
9-9: LGTM! Consistent DTO extension.The new
isDraftAlarmproperty follows the established pattern with proper serialization annotation and logical positioning between diary and reply alarm flags.app/src/main/java/com/sopt/clody/data/remote/dto/request/SendNotificationRequestDto.kt (1)
9-9:Details
✅ Verification successful
LGTM! Consistent with response DTO.
The new
isDraftAlarmproperty maintains consistency with the response DTO structure and follows established patterns.Verify that all code that constructs
SendNotificationRequestDtohas been updated to include the newisDraftAlarmparameter:
🏁 Script executed:
#!/bin/bash # Description: Find all usages of SendNotificationRequestDto to ensure they include the new isDraftAlarm parameter # Expected: All constructor calls should have 5 parameters now # Search for SendNotificationRequestDto constructor calls ast-grep --pattern 'SendNotificationRequestDto($$$)' # Also search for any instantiation patterns rg -A 3 "SendNotificationRequestDto"Length of output: 15748
Approval: All
SendNotificationRequestDtoinstantiations includeisDraftAlarmI’ve verified that every constructor call of
SendNotificationRequestDtonow supplies the newisDraftAlarmparameter in:
TimeReminderViewModel.ktNotificationSettingViewModel.kt(all occurrences)No further changes are required.
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationChangeState.kt (1)
5-10:Details
✅ Verification successful
Excellent state consolidation refactoring!
The unified
NotificationChangeStatesealed class properly consolidates notification change operations into a single, well-structured state representation. This follows standard async operation patterns and should simplify state management.Verify that the old separate state classes are no longer referenced in the codebase:
🏁 Script executed:
#!/bin/bash # Description: Ensure old DiaryAlarmChangeState and ReplyAlarmChangeState are no longer used # Expected: No references to the old state classes should be found # Search for old state class references rg "DiaryAlarmChangeState|ReplyAlarmChangeState" # Also check for any remaining imports rg -A 2 -B 2 "import.*DiaryAlarmChangeState|import.*ReplyAlarmChangeState"Length of output: 127
Great refactoring—old state classes successfully removed
No references to
DiaryAlarmChangeStateorReplyAlarmChangeStatewere found in the codebase after running a global search. The newNotificationChangeStatesealed class cleanly consolidates all notification state management. Approving these changes.app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationTimeSelector.kt (3)
20-24: Excellent refactoring to stateless component!The parameter changes make this component more reusable and follow Compose best practices by removing internal state management. The addition of the
modifierparameter with a default value improves flexibility.
26-26: Good modifier application pattern.Applying the passed
modifierto the root composable allows for better composition and styling flexibility from parent components.
37-37: Improved callback design.Using the passed
onClicklambda directly instead of a hardcoded function call makes the component more flexible and reusable.app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSettingTimePicker.kt (1)
31-31: LGTM! Good refactoring to improve component reusability.The changes successfully decouple the time picker from the ViewModel by:
- Using the
to24HourFormatextension function for time conversion- Simplifying the callback interface to accept a formatted time string
- Keeping the conversion logic local to the component
This aligns well with the broader refactoring effort to lift state management to the Route layer.
Also applies to: 37-37, 140-146
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSwitch.kt (1)
21-26: Excellent refactoring to a generic, reusable component!The transformation from
DiaryAlarmSwitchtoNotificationSwitchsuccessfully:
- Removes tight coupling to ViewModel and Context
- Uses
@StringResfor type-safe string resources- Simplifies the parameter interface
Note: The switch scale was reduced from 0.8f to 0.7f. Please verify this change maintains visual consistency across the app.
Also applies to: 40-40
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt (3)
44-47: Great separation of concerns in the Route composable!The refactoring successfully:
- Collects individual state flows for better granularity
- Simplifies callbacks by removing unnecessary parameters
- Properly delegates UI state to the stateless Screen composable
This makes the state management more explicit and easier to test.
Also applies to: 61-65
108-121: Excellent implementation of a stateless, testable Screen composable!The refactoring demonstrates best practices:
- Pure composable function with no side effects
- Clear parameter naming and organization
- Proper integration of the new draft alarm feature
- Consistent UI spacing and structure
The use of
convertTo12HourFormat()extension maintains consistency with the time picker's 24-hour format output.Also applies to: 145-166
181-206: Well-structured preview for UI development!The preview composable properly demonstrates the notification settings UI with realistic test data, making it easy to verify the layout and interactions during development.
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt (3)
24-35: State flow declarations look good!The new state flows for individual alarm types and notification time follow the proper pattern and align well with the PR objective of adding draft alarm functionality.
70-74: Proper state updates from notification info.The method correctly updates all individual alarm states and notification time from the response.
182-191: Good adaptation to use current alarm states.The method correctly uses the current alarm states from the state flows when sending the notification update request.
| fun String.convertTo12HourFormat(): String { | ||
| val (hourBefore, minuteBefore) = this.split(":").map { it.toInt() } | ||
|
|
||
| val amPm = if (hourBefore < 12) "오전" else "오후" | ||
|
|
||
| val hourAfter = when { | ||
| hourBefore == 0 -> 12 | ||
| hourBefore > 12 -> hourBefore - 12 | ||
| else -> hourBefore | ||
| } | ||
|
|
||
| val minuteAfter = if (minuteBefore == 0) "00" else minuteBefore | ||
|
|
||
| return String.format("$amPm ${hourAfter}시 ${minuteAfter}분") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling for robust time parsing.
The function lacks input validation and could throw NumberFormatException if the input string doesn't match the expected "HH:mm" format. Additionally, there's a minor formatting inconsistency in the minute handling.
Apply this diff to improve error handling and consistency:
fun String.convertTo12HourFormat(): String {
+ if (!this.matches(Regex("\\d{1,2}:\\d{1,2}"))) {
+ throw IllegalArgumentException("Invalid time format. Expected HH:mm format.")
+ }
+
val (hourBefore, minuteBefore) = this.split(":").map { it.toInt() }
+
+ if (hourBefore !in 0..23 || minuteBefore !in 0..59) {
+ throw IllegalArgumentException("Invalid time values. Hour must be 0-23, minute must be 0-59.")
+ }
val amPm = if (hourBefore < 12) "오전" else "오후"
val hourAfter = when {
hourBefore == 0 -> 12
hourBefore > 12 -> hourBefore - 12
else -> hourBefore
}
- val minuteAfter = if (minuteBefore == 0) "00" else minuteBefore
+ val minuteAfter = String.format("%02d", minuteBefore)
return String.format("$amPm ${hourAfter}시 ${minuteAfter}분")
}📝 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.
| fun String.convertTo12HourFormat(): String { | |
| val (hourBefore, minuteBefore) = this.split(":").map { it.toInt() } | |
| val amPm = if (hourBefore < 12) "오전" else "오후" | |
| val hourAfter = when { | |
| hourBefore == 0 -> 12 | |
| hourBefore > 12 -> hourBefore - 12 | |
| else -> hourBefore | |
| } | |
| val minuteAfter = if (minuteBefore == 0) "00" else minuteBefore | |
| return String.format("$amPm ${hourAfter}시 ${minuteAfter}분") | |
| } | |
| fun String.convertTo12HourFormat(): String { | |
| if (!this.matches(Regex("\\d{1,2}:\\d{1,2}"))) { | |
| throw IllegalArgumentException("Invalid time format. Expected HH:mm format.") | |
| } | |
| val (hourBefore, minuteBefore) = this.split(":").map { it.toInt() } | |
| if (hourBefore !in 0..23 || minuteBefore !in 0..59) { | |
| throw IllegalArgumentException("Invalid time values. Hour must be 0-23, minute must be 0-59.") | |
| } | |
| val amPm = if (hourBefore < 12) "오전" else "오후" | |
| val hourAfter = when { | |
| hourBefore == 0 -> 12 | |
| hourBefore > 12 -> hourBefore - 12 | |
| else -> hourBefore | |
| } | |
| val minuteAfter = String.format("%02d", minuteBefore) | |
| return String.format("$amPm ${hourAfter}시 ${minuteAfter}분") | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/utils/extension/StringExt.kt
around lines 3 to 17, the convertTo12HourFormat function lacks input validation
and error handling, which can cause NumberFormatException if the input is not in
"HH:mm" format. Add validation to check if the string splits into exactly two
parts and both parts are valid integers within appropriate ranges (0-23 for
hours, 0-59 for minutes). Also, fix the minute formatting to always display two
digits by formatting minuteAfter as a two-digit string instead of conditionally
replacing 0 with "00". Wrap parsing logic in a try-catch block to handle
exceptions gracefully and return a default or error string if parsing fails.
| fun Triple<String, String, String>.to24HourFormat(): String { | ||
| val (amPm, hour, minute) = this | ||
| val hourInt = when { | ||
| amPm == "오후" && hour.toInt() != 12 -> hour.toInt() + 12 | ||
| amPm == "오전" && hour.toInt() == 12 -> 0 | ||
| else -> hour.toInt() | ||
| } | ||
| return String.format("%02d:%02d", hourInt, minute.toInt()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the Triple conversion function.
The function should validate the input Triple components to prevent runtime errors and ensure data integrity.
Apply this diff to add proper validation:
fun Triple<String, String, String>.to24HourFormat(): String {
val (amPm, hour, minute) = this
+
+ if (amPm !in listOf("오전", "오후")) {
+ throw IllegalArgumentException("Invalid AM/PM value. Expected '오전' or '오후'.")
+ }
+
+ val hourInt = hour.toIntOrNull() ?: throw IllegalArgumentException("Invalid hour format.")
+ val minuteInt = minute.toIntOrNull() ?: throw IllegalArgumentException("Invalid minute format.")
+
+ if (hourInt !in 1..12 || minuteInt !in 0..59) {
+ throw IllegalArgumentException("Invalid time values. Hour must be 1-12, minute must be 0-59.")
+ }
+
- val hourInt = when {
- amPm == "오후" && hour.toInt() != 12 -> hour.toInt() + 12
- amPm == "오전" && hour.toInt() == 12 -> 0
- else -> hour.toInt()
+ val hour24 = when {
+ amPm == "오후" && hourInt != 12 -> hourInt + 12
+ amPm == "오전" && hourInt == 12 -> 0
+ else -> hourInt
}
- return String.format("%02d:%02d", hourInt, minute.toInt())
+ return String.format("%02d:%02d", hour24, minuteInt)
}📝 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.
| fun Triple<String, String, String>.to24HourFormat(): String { | |
| val (amPm, hour, minute) = this | |
| val hourInt = when { | |
| amPm == "오후" && hour.toInt() != 12 -> hour.toInt() + 12 | |
| amPm == "오전" && hour.toInt() == 12 -> 0 | |
| else -> hour.toInt() | |
| } | |
| return String.format("%02d:%02d", hourInt, minute.toInt()) | |
| } | |
| fun Triple<String, String, String>.to24HourFormat(): String { | |
| val (amPm, hour, minute) = this | |
| if (amPm !in listOf("오전", "오후")) { | |
| throw IllegalArgumentException("Invalid AM/PM value. Expected '오전' or '오후'.") | |
| } | |
| val hourInt = hour.toIntOrNull() ?: throw IllegalArgumentException("Invalid hour format.") | |
| val minuteInt = minute.toIntOrNull() ?: throw IllegalArgumentException("Invalid minute format.") | |
| if (hourInt !in 1..12 || minuteInt !in 0..59) { | |
| throw IllegalArgumentException("Invalid time values. Hour must be 1-12, minute must be 0-59.") | |
| } | |
| val hour24 = when { | |
| amPm == "오후" && hourInt != 12 -> hourInt + 12 | |
| amPm == "오전" && hourInt == 12 -> 0 | |
| else -> hourInt | |
| } | |
| return String.format("%02d:%02d", hour24, minuteInt) | |
| } |
🤖 Prompt for AI Agents
In app/src/main/java/com/sopt/clody/presentation/utils/extension/StringExt.kt
around lines 19 to 27, the to24HourFormat function lacks input validation for
the Triple components, which can cause runtime errors. Add validation to check
that amPm is either "오전" or "오후", and that hour and minute strings can be safely
converted to integers within valid ranges (hour 1-12, minute 0-59). If
validation fails, handle it gracefully by throwing an appropriate exception or
returning a default value.
MoonsuKang
left a comment
MoonsuKang
left a comment
There was a problem hiding this comment.
수고하셨습니다~!! 궁금한 점 몇가지 리뷰 남겼습니다~
| }, | ||
| modifier = Modifier.scale(0.8f), | ||
| checked = checkedState, | ||
| onCheckedChange = { onClick() }, |
There was a problem hiding this comment.
P3
Switch Component의 Semantic한 핸들러는 onCheckedChange: ((Boolean) -> Unit)?, 인데 onClick? 이 조금 의미가 다를 수 있다고 생각합니데이.
parameter를 onclick -> onCheckedChange: (Boolean) -> Unit,
으로 하는게 어떨까요?
There was a problem hiding this comment.
좋다고 생각해서 변경하기는 했는데 한 가지 의문점이 생겼네요.
MVI 구조를 차용하게 되면 토글 스위치의 true, false 상태나 변경해야할 때 모두 화면의 State를 기반으로 할 것 같은데요,
그렇다보니 (Boolean) -> Unit 으로 선언할 수는 있지만 사실상 { newState -> ~ } 이런 형식으로 사용할 필요가 없다고 생각이 됩니다 .. 타입만 고치고 실제 구현상에서는 사용하지 않는 것에 대해 어떻게 생각하십니까 ..!
There was a problem hiding this comment.
그쵸, 나중에 아마도 viewModel.onEvent(ToggleDiaryAlarm) 같은 형태로 Contract 기반의 이벤트를 전달하게 될 것 같네용.
Boolean 인자를 직접 쓰진 않더라도, Switch 컴포넌트의 시멘틱에 맞춰 onCheckedChange를 사용하는 게 구조적으로 더 자연스러워 보입니다.
그래서 의미상 onClick보다는 onCheckedChange로 표현해두는 게 좋겠다고 생각했습니다 ~
| onConfirm = { newNotificationTime -> | ||
| notificationInfo?.let { | ||
| notificationSettingViewModel.changeNotificationTime(context, it, newNotificationTime) | ||
| notificationSettingViewModel.changeNotificationTime(context, newNotificationTime) | ||
| } | ||
| updateNotificationTimePicker(false) | ||
| showNotificationTimePicker = false |
There was a problem hiding this comment.
p3
onConfirm = { newNotificationTime ->
notificationSettingViewModel.changeNotificationTime(context, newNotificationTime)
showNotificationTimePicker = false
},이렇게 해도 되지 않을까요? notificationInfo 가 dead code 인거 같은데 혹시 아닌가요??
There was a problem hiding this comment.
맞습니다 ㅎ Screen 단에서 사용하던거 호이스팅하면서 제거해도 됐는데 깜빡했네요!
| @Preview(showBackground = true) | ||
| @Composable | ||
| private fun PreviewNotificationSettingScreen() { | ||
| ClodyTheme { |
There was a problem hiding this comment.
P5
@ClodyPreview도 한번 사용해보면 좋습니다 ㅋㅎㅋㅎ
There was a problem hiding this comment.
앗 ㅋ 앞으로 애용하겠습니다. 지금 써보니까 기가 막히네요
| } | ||
|
|
||
| fun changeNotificationTime(context: Context, notificationInfo: NotificationInfoResponseDto, time: String) { | ||
| fun changeDraftAlarm(context: Context) { |
There was a problem hiding this comment.
P3
changeDraftAlarm, changeNotificationTime, changeReplyAlarm 이거 세개 코드가 80~90%는 동일한 거 같은데
뭔가 changeAlarm이라는 function 하나 만들어서
fun change???Alarm(context: Context) {
changeAlarm(context) { copy(is???Alarm = !is???Alarm) }
}이런식으로 하는거 어떻게 생각하시나요?
There was a problem hiding this comment.
안그래도 그것때문에 불편했는데 추후 MVI 마이그레이션 때 제대로 바꾸려고 놔뒀습니다 .. ㅎ
지금은 중복 코드 제거할 수 있게 하나의 함수로 묶어보긴 했습니다!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt (1)
106-119: Consider parameter object pattern for better maintainability.While the stateless composable design is excellent, the large number of parameters (11 total) could become unwieldy as the feature grows. Consider using a data class to group related parameters.
+data class NotificationSettingScreenState( + val notificationInfoState: NotificationInfoState, + val diaryAlarm: Boolean, + val draftAlarm: Boolean, + val replyAlarm: Boolean, + val notificationTime: String, +) + +data class NotificationSettingScreenCallbacks( + val onClickBack: () -> Unit, + val onClickDiarySwitch: (Boolean) -> Unit, + val onClickDraftSwitch: (Boolean) -> Unit, + val onClickNotificationTime: () -> Unit, + val onClickReplySwitch: (Boolean) -> Unit, + val onClickRetry: () -> Unit, +) + @Composable fun NotificationSettingScreen( - notificationInfoState: NotificationInfoState, - diaryAlarm: Boolean, - draftAlarm: Boolean, - replyAlarm: Boolean, - notificationTime: String, - onClickBack: () -> Unit, - onClickDiarySwitch: (Boolean) -> Unit, - onClickDraftSwitch: (Boolean) -> Unit, - onClickNotificationTime: () -> Unit, - onClickReplySwitch: (Boolean) -> Unit, - onClickRetry: () -> Unit, + state: NotificationSettingScreenState, + callbacks: NotificationSettingScreenCallbacks, ) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/com/sopt/clody/domain/Notification.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSwitch.kt(1 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt(3 hunks)app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/src/main/java/com/sopt/clody/domain/Notification.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/component/NotificationSwitch.kt
- app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingViewModel.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (7)
app/src/main/java/com/sopt/clody/presentation/ui/setting/notificationsetting/screen/NotificationSettingScreen.kt (7)
24-24: LGTM: Clean import additions for new functionality.The new imports are well-organized and necessary for the added draft alarm functionality and refactored components.
Also applies to: 32-36
45-48: Excellent state management separation.The refactoring to collect individual alarm states directly from ViewModel StateFlows improves the architecture by making state dependencies explicit and the UI more reactive.
61-64: Clean callback implementation with proper domain abstraction.The use of the
Notificationenum for thechangeAlarmmethod calls provides good type safety and domain abstraction.
179-204: Great addition of preview functionality!The preview composable follows best practices by using realistic data and the
@ClodyPreviewannotation as suggested in the past review comments.
93-93: Improved method naming for better clarity.The change from a generic reset to
resetNotificationTimeChangeState()provides better specificity about what state is being reset.
149-153:Details
❓ Verification inconclusive
Verify string resource exists for draft alarm.
Ensure that the string resource
R.string.notification_setting_draft_diaryhas been added to all localization files.
🏁 Script executed:
#!/bin/bash # Description: Verify that the draft diary string resource exists in all string resource files # Expected: The string resource should be present in all locale directories echo "Checking for notification_setting_draft_diary string resource:" fd -e xml -x grep -l "notification_setting_draft_diary" {} \; echo -e "\nContent of the string resources:" fd -e xml -x grep "notification_setting_draft_diary" {} \;Length of output: 452
🏁 Script executed:
#!/bin/bash # Verify that notification_setting_draft_diary exists in every locale-specific strings.xml echo "Checking notification_setting_draft_diary in all values* directories:" for xml in app/src/main/res/values*/strings*.xml; do echo "→ $xml" if grep -q "notification_setting_draft_diary" "$xml"; then grep -H "notification_setting_draft_diary" "$xml" else echo " ✗ Missing in $xml" fi doneLength of output: 550
Ensure string resource is localized across all supported locales
The resource
notification_setting_draft_diaryis present in your default strings file:
- app/src/main/res/values/strings.xml
<string name="notification_setting_draft_diary">이어쓰기 알림 받기</string>If your app supports additional languages, please confirm that this entry has been added to each locale’s
values-<locale>/strings.xml.
156-156:Details
✅ Verification successful
Verify time format conversion works correctly.
Ensure the
convertTo12HourFormat()extension function properly handles edge cases like midnight (00:00) and noon (12:00).
🏁 Script executed:
#!/bin/bash # Description: Find and examine the convertTo12HourFormat extension function # Expected: The function should handle 24-hour to 12-hour conversion correctly echo "Finding convertTo12HourFormat extension function:" ast-grep --pattern $'fun String.convertTo12HourFormat(): String { $$$ }' echo -e "\nAlternatively searching with ripgrep:" rg -A 10 "fun.*convertTo12HourFormat"Length of output: 3046
Time conversion edge cases verified and correct
The
convertTo12HourFormat()function maps:
- “00:00” → “오전 12시 00분”
- “12:00” → “오후 12시 00분”
No changes required.
📌 ISSUE
closed #266
📄 Work Description
1. 알림설정 화면에 이어쓰기 알림설정 추가한 UI 구현
2. 코드 리팩토링
✨ PR Point
아직 테스트를 하지는 못했으나, 기능적으로 이상은 .. 없지 않을까 합니다!
추후 MVI 마이그레이션 생각해서 최대한 Intent도 모으고 하는 식으로 리팩토링 진행하였습니다!
📸 ScreenShot/Video
Summary by CodeRabbit
New Features
Improvements
Refactor