-
Notifications
You must be signed in to change notification settings - Fork 1
[FEAT/#396] 푸시알림 상태 변경 / 서버통신 구현 #398
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hyobeen-Park
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
우와아아아아아ㅏ 정말이지 우리 리드 너무 멋진데 어떡하죠?? 진짜 최고다💗💗💗
| val authType: String | ||
| ) | ||
| val authType: String, | ||
| val pushStatus: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거 네이밍을 alarmStatus 등으로 바꿔보는건 어떨까요? pushStatus를 처음 보면 어떤 데이터인지 알기 어려울 것 같아서요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
네이밍 천재 효빈언니! 서버 값에 맞추다 보니 실수해버렸네용,, 좋은 것 같아요! 반영하겠습니당
| if (result.isSuccess) { | ||
| lastSuccessfulAlarmStatus[info.id] = info.isAlarmAvailable | ||
| } else { | ||
| val previous = lastSuccessfulAlarmStatus[info.id] ?: !info.isAlarmAvailable | ||
| _state.update { currentState -> | ||
| currentState.copy(pushStatus = if (previous) ENABLED.value else DISABLED.value) | ||
| } | ||
| userRepository.setAlarmAvailable(previous) | ||
|
|
||
| _sideEffects.emit(MyPageSideEffect.ShowToast(DesignSystemR.string.server_failure)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요 부분 onSuccess / onFailure로 안하고 result.isSuccess로 구현하신 이유가 무엇인가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헙 그러게요! 왜 onSuccess가 안된다고 생각했을까요.. 고치겠습니다!
boiledeggg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimistic한 코드네요😄 고생하셨습니다 대장님~!
| viewModelScope.launch { | ||
| debounceFlow | ||
| .groupBy { it.id } | ||
| .flatMapMerge { (_, flow) -> flow.debounce(DEBOUNCE_DURATION) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확장함수 너무 좋네요👍 여러 아이템이 있을 때 각 아이템에 대한 디바운스를 걸기 위한 확장함수인 것으로 이해했는데 맞나요??
근데 제가 이해한게 맞다면 하나의 목록만 관리하기 위해선 groupBy 확장함수를 사용하지 않고 debounceFlow에 바로 디바운스를 걸어도 괜찮지 않나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
역시 예리하시네요!! 저도 처음엔 그렇게 생각해서 확장함수를 사용하지 않고 구현했었습니다!
단순히 한 화면에서 Optimistic UI를 구현하는 건 동작을 했었는데, 무슨 이유에서인지 처음 마이페이지 화면을 진입할 때는 바로 상태가 반영되지 않더라구여.. 두 번째로 화면에 진입해야 상태가 반영됐었습니다ㅜㅜ
그래서 groupBy로 분류를 해줘야 더 정확한 측정이 가능한가?라는 생각이 들어 반영을 했었는데요..
이 부분에 대해서는 저도 더 알아봐야 될 것 같아요! 같이 공부해봐요🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
=> 확인 결과 하나의 목록일 때는 별도의 확장함수를 안 써줘도 되는 것 같아요!
현재 로직상 어쩔 수 없이 포함해버렸는데 추후 더 간결하게 리팩하도록 하겠습니다!🥲
| val result = myPageRepository.updateAlarmState( | ||
| AlarmStatus(if (info.isAlarmAvailable) ENABLED.value else DISABLED.value) | ||
| ) | ||
|
|
||
| if (result.isSuccess) { | ||
| lastSuccessfulAlarmStatus[info.id] = info.isAlarmAvailable | ||
| } else { | ||
| val previous = lastSuccessfulAlarmStatus[info.id] ?: !info.isAlarmAvailable | ||
| _state.update { currentState -> | ||
| currentState.copy(pushStatus = if (previous) ENABLED.value else DISABLED.value) | ||
| } | ||
| userRepository.setAlarmAvailable(previous) | ||
|
|
||
| _sideEffects.emit(MyPageSideEffect.ShowToast(DesignSystemR.string.server_failure)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요기 내부에 있는 로직을 함수로 분리해주면 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 의견 감사합니다! 이 부분은 위 코드리뷰 반영하면서 같이 고쳐진 것 같아요!
| val notificationPermission = Manifest.permission.POST_NOTIFICATIONS | ||
| val permissionState = | ||
| rememberPermissionState(permission = notificationPermission) | ||
| var isChecked by remember { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 변수는 상태 객체 자체가 바뀌지 않고, 내부 값만 변경되는 구조라서 val로 선언해도 괜찮을 것 같아요!
| var isChecked by remember { | |
| val isChecked = remember { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
말씀해주신 부분 보고 저도 생각을 해 보았는데요..!
사실 저는 by를 써주면 isChecked 변수를 사용할 때 .value를 같이 사용하지 않아도 돼서 가독성이 좋아진다고 생각했었어요..!
그런데 이렇게 by를 써주게 되면 get/set에 대한 위임이 이루어지면서 값이 바뀔 수 있는 var로 사용되어야 하더라구요
val로 선언하고 .value를 같이 사용해주는 것이 좋을까요?
다른 분들의 의견도 궁금하네요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var의 경우 상태 객체 자체를 바꿀 수 있어서 조금의 실수라도 방지하기 위해 val을 사용하는게 좋다고 생각했습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 by에 한표 드립니당!! 가독성이 더 좋아지고 더 직관적인 것 같아서요!! 그리고 by 사용하면 var이어도 상태 객체 자체를 바꾸지는 못할걸요.....?(아마도... 그치만 아닐 수 있음 주의)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조금 더 찾아보니까 by를 사용하면 상태 값 자체에 접근할 수 있고 효빈이 말처럼 객체 자체를 바꾸진 못한다고 하네요!
이 상황에선 간결성을 위해 var을 유지하는게 맞는 것 같아요!
덕분에 하나 배워갑니다~
| collect { t -> | ||
| val key = getKey(t) | ||
| val channel = storage.getOrPut(key) { | ||
| Channel<T>(capacity = Channel.BUFFERED).also { | ||
| emit(key to it.consumeAsFlow()) | ||
| } | ||
| } | ||
| channel.send(t) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
블로그 코드에선 예외처리를 해뒀던데 제외하신 이유가 있나요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엇 이 부분은 제가 놓친 것 같네요! 구현하는 데에 집중하느라 예외처리에 신경을 못 썼네요ㅜㅜ 반영해놓겠습니다!!
| DisposableEffect(lifecycleOwner) { | ||
| onDispose { | ||
| systemUiController.setStatusBarColor(color = White) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
113부터 119가 예전 코드라 태그가 안되네요;;
LaunchedEffect와 DisposableEffect를 합쳐도 되지 않을까 싶네요!
| } | |
| DisposableEffect(lifecycleOwner) { | |
| systemUiController.setStatusBarColor(color = Back) | |
| onDispose { | |
| systemUiController.setStatusBarColor(color = White) | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 혹시 어떻게 합친다는 의미인지 여쭤봐도 될까용...??
LaunchedEffect는 컴포지션이 생성될 때, DisposableEffect는 컴포지션이 사라질 때 적용된다고 생각해서 마이페이지 전용 상태바 색인 Back을 넣구 다른 화면으로 갈 때는 원래 상태바 색인 White로 돌아가게 했었어요..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아! 코드를 제시해주셨군요!! 😅😅 반영해놓겠습니다~ 공부해올게요.
arinming
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다들 리뷰 꼼꼼히 해주시고 반영도 잘 해주셔서 완뵥하네요.. 고생했습니당 ㅜㅜ!
⛳️ Work Description
📸 Screenshot
2025-05-17.12.30.40.mov
📢 To Reviewers
기존에는 푸시 알림 허용 여부를 로컬에서 관리하고 있었지만, 서버 기반으로 상태를 관리해달라는 요청을 받아 해당 방향으로 수정하게 되었습니다!
마이페이지는 해당 블로그 글을 참고하여
Optimistic UI를 통해 서버 응답 전에 UI를 반영하도록 했습니다! (위 영상에서도 확인 가능해요!)이런 변경 사항에 따라 초반에는 로컬 상태를 완전히 제거하고 서버의 응답값에만 의존하려고 했는데, 처음 마이페이지에 진입할 때 서버 응답을 기다리며 토글 on/off 해주는 게 쉽지 않더라구요..
만약 서버 응답값에 완전히 의존하게 하더라도,
TerningMessagingService에서도 알림 허용 여부를 확인해야 하는데, 이 경우 앱이 꺼진 상태에서도 작동해야 하므로 항상 서버에 의존하는 것보다 로컬 값을 확인하는 게 더 안전하다는 판단을 했습니다..!결론은.. 서버 값과의 동기화는 유지하되, 기본 알림 허용 여부는 로컬에 저장된 값을 기준으로 하도록 작성했습니다! 더 나은 의견 있다면 남겨주세요!