[QA#67] Recording Screen 수정 #68
Conversation
Summary of ChangesHello @rhkrwngud445, 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! 이 PR은 QA에서 발견된 'Recording Screen' 관련 이슈를 해결합니다. 주요 목적은 친구를 '챙김' 버튼으로 기록했을 때, 업데이트된 챙김 날짜가 홈 화면에 정확하게 반영되도록 하는 것입니다. 이를 위해 친구 프로필 화면에서 홈 화면으로 친구 업데이트 이벤트를 전달하는 네비게이션 로직을 개선하고, 뷰모델에서 친구 데이터의 Highlights
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은 친구 프로필 화면에서 '챙기기' 완료 후 홈 화면에 변경사항이 바로 반영되도록 수정하고, 시스템 뒤로가기 버튼 동작을 개선하는 내용을 담고 있습니다. 전반적으로 Navigation Component의 SavedStateHandle을 이용해 화면 간 결과를 전달하는 방식으로 리팩토링한 점이 좋습니다. 다만, 몇 가지 잠재적인 버그와 개선점을 발견하여 리뷰에 포함했습니다. 특히 SavedStateHandle에서 이벤트를 처리한 후 제거하지 않아 발생하는 문제와, onClickBackButton 콜백의 타입 불일치 문제는 수정이 필요해 보입니다. 또한, 일부 모델의 속성명과 변수명이 달라 혼란을 줄 수 있는 부분과 날짜 업데이트 로직의 잠재적인 오류도 확인 부탁드립니다.
| friendShipRecordState: FriendShipRecordState, | ||
| recordSuccessDialogState: Boolean = false, | ||
| onClickBackButton: () -> Unit = {}, | ||
| onClickBackButton: (Friend) -> Unit = {}, |
There was a problem hiding this comment.
FriendProfileScreen의 onClickBackButton 파라미터 타입이 (Friend) -> Unit으로 선언되어 있습니다. 하지만 FriendProfileRoute에서는 (Friend?) -> Unit 타입의 콜백을 전달하고 있어 타입 불일치로 인해 컴파일 오류가 발생할 수 있습니다. BackHandler에서도 nullable한 Friend 객체를 전달할 수 있으므로, FriendProfileScreen의 onClickBackButton 타입을 (Friend?) -> Unit으로 변경하여 일관성을 맞추고 잠재적인 오류를 해결하는 것이 좋습니다.
| onClickBackButton: (Friend) -> Unit = {}, | |
| onClickBackButton: (Friend?) -> Unit = {}, |
| val homeEvent = backStackEntry.savedStateHandle.get<HomeNavigationEvent>(HOME_RESULT_EVENT) | ||
| when (homeEvent) { | ||
| is HomeNavigationEvent.FriendDeleted -> { | ||
| viewModel.deleteFriend(homeEvent.friendId) | ||
| } | ||
|
|
||
| is HomeNavigationEvent.FriendReminderUpdated -> { | ||
| viewModel.updateFriendReminder(homeEvent.friendId, homeEvent.friendReminderUpdatedAt) | ||
| } | ||
| null -> Unit | ||
| } |
There was a problem hiding this comment.
SavedStateHandle에서 homeEvent를 가져온 후, 이벤트를 처리하고 나서 remove()를 호출하여 이벤트를 소비하는 로직이 필요합니다. 현재 코드에서는 이벤트를 제거하지 않기 때문에, 화면 회전과 같은 설정 변경이나 다른 이유로 리컴포지션이 발생할 때마다 deleteFriend 또는 updateFriendReminder 함수가 반복적으로 호출될 수 있습니다. 이는 의도치 않은 동작이나 앱의 오작동을 유발할 수 있습니다.
val homeEvent = backStackEntry.savedStateHandle.get<HomeNavigationEvent>(HOME_RESULT_EVENT)
if (homeEvent != null) {
when (homeEvent) {
is HomeNavigationEvent.FriendDeleted -> {
viewModel.deleteFriend(homeEvent.friendId)
}
is HomeNavigationEvent.FriendReminderUpdated -> {
viewModel.updateFriendReminder(homeEvent.friendId, homeEvent.friendReminderUpdatedAt)
}
null -> Unit
}
backStackEntry.savedStateHandle.remove<HomeNavigationEvent>(HOME_RESULT_EVENT)
}| completedReminders.value = listOf( | ||
| recordedFriend.copy( | ||
| nextContactAt = | ||
| LocalDate | ||
| .now() | ||
| .format(DateTimeFormatter.ofPattern("yy.MM.dd")), | ||
| ), | ||
| ) + completedReminders.value |
There was a problem hiding this comment.
챙김(record)이 완료된 친구를 completedReminders 목록으로 옮길 때 nextContactAt 속성을 현재 날짜로 설정하고 있습니다. nextContactAt이라는 속성명은 '다음 연락할 날짜'를 의미하는 것으로 보입니다. 만약 그렇다면, 이 값은 연락 주기에 따라 미래의 날짜로 계산되어야 합니다. 현재 날짜를 설정하는 것은 논리적으로 맞지 않을 수 있습니다. 만약 이 속성이 '마지막으로 연락한 날짜'를 표시하기 위한 것이라면, lastContactAt과 같이 속성명을 변경하여 의도를 명확히 하는 것이 좋겠습니다. 현재 구현은 버그일 가능성이 높아 보입니다.
| onAddContactClick: () -> Unit = {}, | ||
| onMonthlyReminderAllClick: () -> Unit = {}, | ||
| ) { | ||
| composable<RouteHome> { backStackEntry -> |
There was a problem hiding this comment.
제미나이가 remove()로 이벤트를 소비하라는 리뷰를 주었네요!
네비게이션에서는 라우팅만 담당하는 것을 선호하여 ViewModel 사용을 자제하고 있었는데
네비게이션에서 ViewModel을 생성한다면 이점이 있나요?
There was a problem hiding this comment.
연락 일시 데이터를 이전 화면에서 가지고 와서 viewModel 상태에 반영하기 위함이었습니다!
ViewModel에서 바로 데이터를 받아 쓰고싶은데,saveStateHandle collect로 데이터를 못 받아왔던 것으로 기억합니다🥲
혹시 가능한데, 제가 잘못쓰고 있었던 것일까요?!
There was a problem hiding this comment.
앗 아닙니다! Navigation 쪽에서 ViewModel을 생성하는 것이 낯설어서 여쭤봤어요 :)
구조적으로 문제가 없다면 이 상태도 좋을 것 같습니다!
saveStateHandle collect로 데이터를 못 받는 현상이 있군요.. 의견으로는 그렇다면
LaunchEffect와 lifecycleOwner를 활용해 RESUME 상태일 때 데이터를 반영하면 어떻까요? 문제가 있을까요?
There was a problem hiding this comment.
답변이 늦었네요😅 lifecycleOwner는 액티비티 생명주기 변경에 대한 중복 이벤트가 막아지는 것으로 알고있습니다!
현재 문제는 리컴포지션에 문제가 되는 것이라 LaunchedEffect에 키를 주어서 대응이 가능해보여요!
저도 안 viewModel에서 못받는 것에 대해서 테스트를 조금 해봤는데, backStackEntry.savedStateHandle 는 이벤트가 넘어 오는데, viewModel saveStateHandle는 이벤트가 안넘어오네요!
정확한 원리는 아직 파악은 못했는데, viewModel에서 쓰이는 saveStateHandle과 composable에서 쓰이는 saveStateHandle이 다른 객체네요! SharedViewModel을 고려해서 그렇게 설계가 된 것일 수도 있겠네요!
참고한 링크 공유드려요!
리컴포지션 문제에 대해서는 LaunchedEffect로 해결하겠습니다!
There was a problem hiding this comment.
아 링크 감사합니다! 저도 더 자세히 알게 되었네요 :D
동작에는 문제 없는거 확인해서 머지해도 좋을 것 같습니다!
작업 내용
HO-100000 2차 QA 수정
3번 항목(이번달 챙길 사람은 백엔드 수정이 필요해 보입니다!)
확인 방법
참고 사항
3번 항목은 서버 리스폰스 변경 후 별도로 처리하겠습니다!
관련 이슈