[#134] 사용자 설정으로 시간을 바꿀 때 확인 버튼을 탭하면 서버에 변동사항이 저장되도록 개선한다#139
[#134] 사용자 설정으로 시간을 바꿀 때 확인 버튼을 탭하면 서버에 변동사항이 저장되도록 개선한다#139
Conversation
opficdev
commented
Feb 26, 2026
opficdev
commented
- closed PushNotificationSettingsView에서 사용자 설정으로 시간을 바꿀 때, 확인 버튼을 탭하면 서버로 변경되도록 변경한다 #134
Summary of ChangesHello @opficdev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 풀 리퀘스트는 푸시 알림 시간 설정 기능의 사용자 경험을 개선하기 위해 상태 관리 로직을 리팩토링합니다. 메인 화면에 표시되는 시간과 시간 선택기 시트에서 편집되는 시간을 분리하고, 사용자가 명시적으로 '확인' 버튼을 눌러야만 변경 사항이 서버에 저장되도록 하여, 의도치 않은 변경을 방지하고 데이터 일관성을 높입니다. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 사용자가 푸시 알림 시간을 설정할 때, 변경 사항을 즉시 저장하는 대신 '확인' 버튼을 눌러야 저장되도록 개선하는 작업을 담고 있습니다. 이를 위해 시간 상태를 화면에 표시되는 시간(viewPushNotificationTime)과 수정 중인 시간(sheetPushNotificationTime)으로 분리하고, sheet 뷰에 확인/취소 버튼을 추가하는 등 ViewModel과 View 양쪽에서 많은 변경이 이루어졌습니다. 전반적으로 의도한 기능 개선은 잘 이루어졌으나, 몇 가지 수정이 필요한 부분과 개선할 점이 보입니다. 특히, 푸시 알림 활성화 토글 시 변경사항이 저장되지 않는 문제와 iOS 버전 체크의 오타는 수정이 필요합니다. 자세한 내용은 각 코드 리뷰 코멘트를 참고해주세요.
|
|
||
| @ToolbarContentBuilder | ||
| private var toolbar: some ToolbarContent { | ||
| if #available(iOS 26.0, *) { |
There was a problem hiding this comment.
iOS 버전 분기 처리에 if #available(iOS 26.0, *)를 사용하셨습니다. iOS 26.0은 아직 출시되지 않은 버전이므로 이 조건문은 항상 false를 반환하게 됩니다. 아마 iOS 16.0의 오타인 것 같습니다. Button의 role 파라미터는 iOS 15.0부터 사용 가능하며, .cancel, .confirm 등의 역할(role)에 따른 기본 스타일과 동작을 제공합니다. 버전을 iOS 15.0 또는 iOS 16.0으로 수정해야 의도한 대로 동작합니다.
| if #available(iOS 26.0, *) { | |
| if #available(iOS 16.0, *) { |
| case .setPushNotificationEnable(let value): | ||
| self.state.pushNotificationEnable = value | ||
| return [.updatePushNotificationSettings] | ||
| state.pushNotificationEnable = value |
There was a problem hiding this comment.
setPushNotificationEnable 액션이 더 이상 updatePushNotificationSettings 사이드 이펙트를 발생시키지 않습니다. 이로 인해 사용자가 푸시 알림 활성화 토글을 눌러도 변경사항이 서버에 즉시 저장되지 않는 문제가 발생합니다. 토글 스위치는 즉시 반응하여 서버에 저장되는 것이 일반적인 사용자 경험에 부합하므로, 이전과 같이 사이드 이펙트를 발생시켜야 합니다.
| case .setPushNotificationEnable(let value): | |
| self.state.pushNotificationEnable = value | |
| return [.updatePushNotificationSettings] | |
| state.pushNotificationEnable = value | |
| case .setPushNotificationEnable(let value): | |
| state.pushNotificationEnable = value | |
| effects = [.updatePushNotificationSettings] |
| let dateComponents = calendar.dateComponents( | ||
| [.hour, .minute], | ||
| from: state.sheetPushNotificationTime | ||
| ) |
There was a problem hiding this comment.
updatePushNotificationSettings 사이드 이펙트에서 state.sheetPushNotificationTime을 사용하여 시간을 업데이트하고 있습니다. sheetPushNotificationTime은 사용자가 시간 선택 창에서 임시로 선택한 시간이며, 아직 확정되지 않은 값일 수 있습니다. 예를 들어, 사용자가 시간 선택 창을 열어 시간을 변경한 후 '확인'을 누르지 않고 닫은 다음, 푸시 알림 활성화 토글을 누르면 아직 확정되지 않은 시간이 서버에 저장될 수 있습니다. 항상 확정된 시간을 사용하도록 state.viewPushNotificationTime을 사용해야 합니다.
| let dateComponents = calendar.dateComponents( | |
| [.hour, .minute], | |
| from: state.sheetPushNotificationTime | |
| ) | |
| let dateComponents = calendar.dateComponents( | |
| [.hour, .minute], | |
| from: state.viewPushNotificationTime | |
| ) |
| case .setPushNotificationHour(let value): | ||
| // 시간만 변경 | ||
| if let newDate = calendar.date( | ||
| bySettingHour: value, | ||
| minute: 0, second: 0, | ||
| of: state.pushNotificationTime | ||
| of: state.viewPushNotificationTime | ||
| ) { | ||
| self.state.pushNotificationTime = newDate | ||
| return [.updatePushNotificationSettings] | ||
| state.viewPushNotificationTime = newDate | ||
| } |
There was a problem hiding this comment.
setPushNotificationHour 액션은 View에서 더 이상 사용되지 않는 것으로 보입니다. PushNotificationSettingsView에서는 프리셋 시간을 탭했을 때 setPushNotificationTime과 confirmUpdate 액션을 직접 호출하도록 변경되었습니다. Action 열거형의 setPushNotificationHour 케이스와 reduce 함수의 해당 case 문을 함께 제거하여 코드를 정리하는 것을 권장합니다.
| viewModel.send(.setPushNotificationTime(sheet: date)) | ||
| viewModel.send(.confirmUpdate) |
There was a problem hiding this comment.
프리셋 시간을 탭했을 때 View에서 두 개의 액션(setPushNotificationTime, confirmUpdate)을 순차적으로 보내고 있습니다. 이는 View가 ViewModel의 내부 동작(상태를 설정하고 바로 업데이트를 트리거하는)을 너무 많이 알고 있게 만듭니다. 이 로직을 ViewModel로 옮겨 View의 역할을 단순화하는 것이 좋습니다.
예를 들어, PushNotificationSettingsViewModel.Action에 selectPresetTime(Date) 케이스를 추가하고, View에서는 이 액션만 보내도록 수정할 수 있습니다.
ViewModel 수정 제안:
Action 열거형에 .selectPresetTime(Date)를 추가하고, reduce 메소드에 아래와 같이 로직을 구현합니다.
case .selectPresetTime(let date):
state.viewPushNotificationTime = date
state.sheetPushNotificationTime = date // 동기화
effects = [.updatePushNotificationSettings]| viewModel.send(.setPushNotificationTime(sheet: date)) | |
| viewModel.send(.confirmUpdate) | |
| viewModel.send(.selectPresetTime(date)) |
1 participant
