Conversation
📝 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)
⚔️ Resolve merge conflicts (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 요청에 실패했을 경우 사용자가 이 화면에 다시 왔을 때
롤백 되는게 자연스러울 것 같은데 현수는 어떻게 생각해 ?
1 participant
이슈 번호
#83
리뷰/머지 희망 기한 (선택)
작업내용
결과물
ww.mp4
리뷰어에게 추가로 요구하는 사항 (선택)
600L를 기준으로 잡은 이유는 1000L로 설정했을 때 리액션을 누른 후 진짜 빠르게 뒤로가기 버튼을 누를 경우
API 요청이 되기 전에 화면이 죽어버리는 현상이 발생하더라구
그래서 내가 테스트 했을 때 600L 정도면 뒤로가기 버튼을 누르기 전까지 API 요청을 보내기 충분해서 600L로 정했어 !
혹시 디바운싱 말고 좋은 방법이 있다면 제안해줘도 좋아☺️
참고로 가영이가 리액션 해제 기능은 없다고해 1
TODO
버튼을 10번정도 눌렀을 때 발생하는 리컴포지션 횟순데 리액션 버튼을 누를 때 마다 리컴포지션이 발생해서
버벅임이 발생하는게 아닌지 추측 ...