Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 Walkthrough워크스루이번 PR은 포토로그(Photolog) 기능에 반응(Reaction) 기능을 추가하는 계층별 기능 구현입니다. 네트워크 계층에서 새로운 API 엔드포인트( 추정 코드 리뷰 노력도🎯 3 (Moderate) | ⏱️ ~25 minutes 상세 리뷰✅ 우수한 점계층별 책임 분리가 명확합니다.
반응 이벤트 처리가 잘 설계되어 있습니다.
💭 검토가 필요한 부분TaskCertificationDetailScreen의 데이터 소스 변경 현재 변경: uiState.currentGoal.partnerPhotolog?.reaction
// 이전: uiState.photoLogs.partnerPhotologs.reaction질문: TaskCertificationDetailViewModel의 초기화 순서 // init 블록 내부
collectReactionFlow()
// + 초기 페치 및 이벤트 수집개선 제안: DefaultPhotoLogRepository의 에러 처리 override suspend fun reactToPhotolog(
photologId: Long,
reaction: GoalReactionType
): AppResult<Unit> {
return safeApiCall {
service.reactToPhotolog(photologId, reaction.toRequest())
}
}개선 제안: 현재 구현이 ReactionFlow의 버퍼 설정 코드에 명시되지 않았지만,
📋 체크리스트
전반적으로 아키텍처가 잘 설계되어 있으며, 위의 세부 사항들을 확인하시면 안정적인 기능이 될 것 같습니다! 👍 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 review |
✅ Actions performedReview triggered.
|
![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
🤖 Fix all issues with AI agents
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt`:
- Around line 53-63: collectReactionFlow currently uses distinctUntilChanged
which suppresses consecutive identical GoalReactionType emits and causes
toggle-off actions to be dropped; update the flow to include the reaction
intent/state or remove distinctUntilChanged: either (1) change the type emitted
by reactionFlow to carry both the GoalReactionType and a nullable/boolean
intended state (e.g., ReactionIntent or Pair<GoalReactionType, Boolean?>) and
update reduceReaction to emit that combined value so collectReactionFlow
collects and passes it to reactToPhotolog, or (2) simply remove
distinctUntilChanged from collectReactionFlow so every emit (including rapid
toggles) reaches debounce and reactToPhotolog; adjust reactToPhotolog signature
accordingly if you choose option (1). Ensure DEBOUNCE_INTERVAL,
collectReactionFlow, reactionFlow, reactToPhotolog, and reduceReaction are
updated consistently.
- Around line 65-72: reactToPhotolog currently performs an optimistic update but
provides no rollback on failure (onSuccess = {}), so add error handling in
launchResult: implement onError to revert the optimistic UI change (restore the
previous reaction/state for currentState.currentGoal.partnerPhotolog) and/or
show a user-facing message (toast/snackbar). Locate reactToPhotolog, the
photologId retrieval (currentState.currentGoal.partnerPhotolog?.photologId), and
the launchResult call (block = { photologRepository.reactToPhotolog(...) }) and
in the launchResult options set an onError handler that restores the prior state
and surfaces an error to the user.
🧹 Nitpick comments (1)
feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt (1)
222-251:ReactionSection의 리컴포지션 최적화를 고려해 보세요.PR에서 언급된 Jank 현상과 관련하여, 현재
ReactionSection이GoalReactionType?을 직접 받고 있어 리액션이 변경될 때마다 전체가 리컴포지션됩니다.
ReactionBar와ReactionEffect가 독립적으로 리컴포지션될 수 있도록, 각각을 별도의 scope로 분리하면 불필요한 리컴포지션을 줄일 수 있습니다. 예를 들어effectTarget변경이ReactionBar의 리컴포지션을 유발하지 않도록 분리하는 것을 검토해 보세요.또한,
onSelectReaction람다 내에서effectTarget상태 변경과onClickReaction호출이 동시에 일어나면서 한 프레임에 두 번의 상태 변경이 발생할 수 있습니다. 이것이 PR에서 언급된 리컴포지션 횟수 증가의 원인일 수 있습니다.
...ication/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt
Show resolved
Hide resolved
...ication/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt
Show resolved
Hide resolved
| }, | ||
| block = { photologRepository.reactToPhotolog(photologId, reaction) }, | ||
| onSuccess = {}, |
There was a problem hiding this comment.
낙관적 업데이트를 하는 상황에서 UI는 업데이트 됐는데 실패 메시지가 뜨는게 이상하다고 생각해서 에러 토스트는 안띄우게 만들었어
만약 에러 토스트를 띄운다면 동시에 이전에 선택했던 리액션으로 롤백도 같이 되야할거 같은데
그 것 보다 UI는 무조건 업데이트 되면서 API 요청에 실패했을 경우 사용자가 이 화면에 다시 왔을 때
롤백 되는게 자연스러울 것 같은데 현수는 어떻게 생각해 ?
There was a problem hiding this comment.
저는 Optimistic UI 적용할 때, 이전 값을 들고 있다가 통신 실패하면 이전 값으로 UI 상태를 되돌려서 최대한 UI <-> 서버 불일치가 발생하지 않도록 구현했어요.
지금 토스트를 안 띄우고 있는 상태에서 유저가 마지막으로 누른 값이랑 화면 밖으로 나갔다가 다시 들어왔을 때 값이랑 차이가 나면 왜 다른 건지 인지가 안될 수도 있지 않을까요? 그래서 저는 토스트 + 롤백하는 구조로 구현하는 게 낫지 않을까 싶긴 해요.
There was a problem hiding this comment.
유저가 마지막으로 누른 값이랑 화면 밖으로 나갔다가 다시 들어왔을 때 값이랑 차이가 나면 왜 다른 건지 인지가 안될 수도 있지 않을까요?
오우 이건 생각 못해봤는데 굉장히 치명적일 것 같아 😓
토스트 + 롤백 방식으로 수정했어
리뷰 반영 커밋 : add9aec
dogmania
left a comment
dogmania
left a comment
There was a problem hiding this comment.
고생하셨습니다! 리액션 처리할 때 스코프 매번 생성되는 것 위주로 코멘트 봐주시면 될 거 같아요
There was a problem hiding this comment.
GoalIconType enum에 보면 toApi라는 메서드가 이미 만들어져 있는데 새로 만드신 이유가 있나요??
There was a problem hiding this comment.
GoalIconType 이랑 GoalReactionType은 다른게 아닌감..? 🤔
대신 현수가 사용하는 컨벤션과 동일하게 수정했어 !
요것두 사전에 컨벤션을 정했으면 좋았을 것 같은데 현수가 사용하는 방식이 좋은 것 같아서 동일하게 진행하면 될 것 같아 👍
리뷰 반영 커밋 : dfb2d74
| }, | ||
| block = { photologRepository.reactToPhotolog(photologId, reaction) }, | ||
| onSuccess = {}, |
There was a problem hiding this comment.
저는 Optimistic UI 적용할 때, 이전 값을 들고 있다가 통신 실패하면 이전 값으로 UI 상태를 되돌려서 최대한 UI <-> 서버 불일치가 발생하지 않도록 구현했어요.
지금 토스트를 안 띄우고 있는 상태에서 유저가 마지막으로 누른 값이랑 화면 밖으로 나갔다가 다시 들어왔을 때 값이랑 차이가 나면 왜 다른 건지 인지가 안될 수도 있지 않을까요? 그래서 저는 토스트 + 롤백하는 구조로 구현하는 게 낫지 않을까 싶긴 해요.
| reactionFlow | ||
| .distinctUntilChanged() | ||
| .debounce(DEBOUNCE_INTERVAL) | ||
| .collect { reaction -> |
There was a problem hiding this comment.
가장 최근 값만 전송하면 되는 거니까 collectLatest를 사용해도 괜찮을 거 같아요!
|
|
||
| private fun reduceReaction(reaction: GoalReactionType) { | ||
| reduce { updatePartnerReaction(reaction) } | ||
| viewModelScope.launch { reactionFlow.emit(reaction) } |
There was a problem hiding this comment.
리액션을 연타하게 되면 매번 Job을 생성하게 되는 구조라서 개선하면 좋을 거 같아요.
지금 reduceReaction이 suspend 메서드 내부에서 호출되고 있으니까 이 메서드도 suspend로 설정하고 새로운 스코프 생성은 없앨 수 있어요. 다른 방법으로는 tryeEmit을 사용해서 suspend 없이 스코프 생성 제거하는 것도 가능합니다!
There was a problem hiding this comment.
전혀 생각 못했는데 좋은 의견이네 👍
tryeEmit은 이벤트가 유실될 위험이 있다고 해서 suspend함수로 수정했어 !
리뷰 반영 커밋 : f3e5764
# Conflicts: # feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt
…fication-update # Conflicts: # data/src/main/java/com/twix/data/repository/DefaultPhotoLogRepository.kt # feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt # feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt # feature/task-certification/src/main/java/com/twix/task_certification/detail/model/TaskCertificationDetailUiState.kt # feature/task-certification/src/main/res/values/strings.xml
chanho0908
changed the base branch from
feat/#83-phopolog-api-change
to
develop
2 participants
이슈 번호
#83
리뷰/머지 희망 기한 (선택)
작업내용
결과물
ww.mp4
리뷰어에게 추가로 요구하는 사항 (선택)
600L를 기준으로 잡은 이유는 1000L로 설정했을 때 리액션을 누른 후 진짜 빠르게 뒤로가기 버튼을 누를 경우
API 요청이 되기 전에 화면이 죽어버리는 현상이 발생하더라구
그래서 내가 테스트 했을 때 600L 정도면 뒤로가기 버튼을 누르기 전까지 API 요청을 보내기 충분해서 600L로 정했어 !
혹시 디바운싱 말고 좋은 방법이 있다면 제안해줘도 좋아☺️
참고로 가영이가 리액션 해제 기능은 없다고해 1
TODO
버튼을 10번정도 눌렀을 때 발생하는 리컴포지션 횟순데 리액션 버튼을 누를 때 마다 리컴포지션이 발생해서
버벅임이 발생하는게 아닌지 추측 ...