Conversation
- `PhotologDetailUiModel` 삭제 - `GoalPhotologUiModel` 추가하여 목표별 인증 정보를 관리하도록 변경 - 날짜 기반으로 전체 목표 인증 현황을 가져오도록 API 호출 방식 수정 - UI Model 구조 변경에 따른 화면 및 ViewModel 로직 수정
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough포토로그 기능의 데이터 흐름과 네비게이션이 재구조화되었습니다. 네트워크 응답이 목표별 복수 포토로그를 반환하도록 PhotoLogsResponse와 GoalPhotologResponse가 추가되고 기존 PhotoLogResponse는 제거되었습니다. 도메인 모델은 GoalPhotolog과 PhotoLogs로 재정의되었고, PhotologDetail의 필드 타입과 반응(reaction) 처리가 추가되었습니다. 중간 UI 모델 계층(PhotologDetailUiModel, PhotoLogsUiModel)을 제거하고 도메인 모델을 UI에서 직접 사용하도록 변경했으며, TaskCertificationDetail 경로와 관련 네비게이션에 date 인자가 추가되어 targetDate 기반 조회로 전환되었습니다. Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@core/network/src/main/java/com/twix/network/model/response/photolog/PhotologDetailResponse.kt`:
- Line 15: The PhotologDetailResponse data class declares
`@SerialName`("reaction") val reaction: String? without a default which can cause
kotlinx.serialization MissingFieldException when the server omits the key;
update the declaration for reaction in PhotologDetailResponse to provide a
default (e.g., reaction: String? = null) similar to the comment field so
deserialization is safe and consistent.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt`:
- Line 87: The constant TARGET_DATE_NOT_FOUND contains an extra "Id" from a copy
of GOAL_ID_NOT_FOUND; update the string value of TARGET_DATE_NOT_FOUND (in
TaskCertificationDetailViewModel) to remove "Id" so it reads "Target Date
Argument Not Found" (ensure the constant name TARGET_DATE_NOT_FOUND is
unchanged).
- Around line 69-75: 요약: TaskCertificationDetailIntent.Sting가 TODO("찌르기 API
연동")로 남아 있어 NotImplementedError로 크래시를 발생시킵니다; handleIntent에서 이를 안전한 처리를 하도록
교체하세요. 수정 방법: TaskCertificationDetailViewModel의 handleIntent에서
TaskCertificationDetailIntent.Sting 분기를 TODO로 남기지 말고 삭제하고, 대신 크래시를 유발하지 않는 동작으로
바꿉니다 — 예: 기존 효과/이벤트 채널이 있으면 _effect.tryEmit(UIEffect.ShowToast("준비 중")) 또는
viewState 이벤트(예: emitEvent/showToast)로 "준비 중" 토스트를 발행하거나, 간단한 no-op 핸들러
handleStingPlaceholder()를 추가해 호출하도록 변경하세요; 관련 심볼: handleIntent,
TaskCertificationDetailIntent.Sting, reduceReaction, reduceShownCard, (또는 사용 중인
이벤트 채널 이름)_effect/_events를 찾아 적용하세요.
🧹 Nitpick comments (9)
feature/task-certification/src/main/java/com/twix/task_certification/detail/component/CertificatedCard.kt (1)
37-37: 더 관용적인 null/empty 체크 사용을 권장합니다
comment?.isNotEmpty() == true는 동작상 문제는 없지만, Kotlin에서는!comment.isNullOrEmpty()가 더 관용적이고 가독성이 좋습니다.♻️ 제안하는 변경
- if (comment?.isNotEmpty() == true) { + if (!comment.isNullOrEmpty()) {feature/task-certification/src/main/java/com/twix/task_certification/detail/model/GoalPhotologUiModel.kt (1)
24-28: computed property의 재계산에 대해 — 현재는 괜찮습니다
myUpdatedDate와partnerUpdatedDate가get()으로 매 접근 시RelativeTimeFormatter.format()을 호출합니다. 현재는 가벼운 연산이라 문제없지만, 만약RelativeTimeFormatter.format()이 무거워지거나 Compose recomposition이 빈번해진다면 캐싱을 고려해볼 수 있습니다.core/network/src/main/java/com/twix/network/service/PhotoLogService.kt (1)
22-25: 파라미터 이름request가 의미를 전달하지 못합니다.
@Query("targetDate")의 값으로 날짜 문자열을 전달하는데, 파라미터명이request로 되어 있어 실제 의미와 괴리가 있습니다. 다른 메서드(getUploadUrl)에서는goalId처럼 의미 있는 이름을 사용하고 있으므로, 일관성을 위해서도targetDate로 변경하는 것이 좋겠습니다.♻️ 제안하는 변경
`@GET`("api/v1/photologs") suspend fun fetchPhotoLogs( - `@Query`("targetDate") request: String, + `@Query`("targetDate") targetDate: String, ): PhotoLogsResponsedomain/src/main/java/com/twix/domain/repository/PhotoLogRepository.kt (1)
19-19:targetDate의 타입으로String대신LocalDate사용을 고려해 보세요.현재
HomeScreen에서는LocalDate로 날짜를 관리하고, 네비게이션을 통해String으로 변환된 후 도메인 레이어까지String으로 전달됩니다. 도메인 레이어는 외부 포맷에 의존하지 않는 것이 이상적이므로,targetDate를LocalDate로 정의하고 Data 레이어(Repository 구현체)에서String으로 변환하면 다음과 같은 이점이 있습니다:
- 타입 안전성: 잘못된 날짜 형식의 문자열이 전달되는 것을 컴파일 타임에 방지
- 도메인 순수성: 도메인 모델이 API의 날짜 포맷(예:
"yyyy-MM-dd")에 결합되지 않음다만
java.time의존성을 도메인에 두는 것이 프로젝트 방침에 맞는지 확인이 필요합니다. 이미 다른 곳에서LocalDate를 사용하고 있다면 일관성 있게 적용하는 것이 좋겠습니다.feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt (1)
213-217: 주석 처리된ReactionSection코드에 대해 — TODO 추적이 필요합니다.ViewModel에서
Sting에 대한TODO가 있고, 여기서ReactionSection이 주석 처리되어 있는데, 이 코드가 추후 복원될 예정이라면 관련 이슈 번호를 주석에 함께 남겨두면 추적이 용이합니다. 만약 제거 대상이라면 완전히 삭제하는 것이 코드 가독성 측면에서 더 좋습니다.// TODO(`#이슈번호`): ReactionSection 복원 - 찌르기 API 연동 후 활성화domain/src/main/java/com/twix/domain/model/photolog/PhotologDetail.kt (1)
5-18: Non-null 전환과 불변 업데이트 패턴이 잘 적용되었습니다.핵심 필드들(
photologId,imageUrl)의 nullable 제거는 도메인 계층에서의 null 안전성을 높이고,updateReaction()과updateComment()가copy()를 활용하여 불변성을 유지하는 점이 좋습니다.isMine필드 제거도 새로운GoalPhotolog구조에서 my/partner를 구조적으로 분리한 것과 잘 맞습니다.한 가지 향후 개선 포인트로,
verificationDate,uploadedAt등 날짜/시간 필드가String으로 되어 있는데, 도메인 레벨에서LocalDate/LocalDateTime같은 타입을 사용하면 비즈니스 로직에서 날짜 비교나 포맷팅이 더 안전해질 수 있습니다. 현재 구조에서 급한 사항은 아니지만, 참고해 주세요.feature/task-certification/src/main/java/com/twix/task_certification/detail/model/TaskCertificationDetailUiState.kt (2)
14-43: computed property들이 매 접근마다goalCacheMap을 재생성하는 구조입니다.
currentGoal은photoLogs[currentGoalId]를 호출하고, 이는 내부적으로goalPhotolog.associateBy { it.goalId }로 Map을 매번 새로 생성합니다. 한 번의 recomposition에서isDisplayedGoalCertificated,displayedGoalUpdateAt,displayedGoalImageUrl,displayedGoalComment,canModify,canReaction등이 각각currentGoal에 접근하므로, Map이 여러 번 불필요하게 재생성됩니다.현재 goal 수가 적다면 실질적 성능 영향은 크지 않겠지만,
PhotoLogsUiModel의goalCache를 lazy 초기화하거나currentGoal을 state 필드로 직접 관리하면 더 효율적입니다. 이 부분은PhotoLogsUiModel리뷰에서 더 자세히 다루겠습니다.
9-10:currentGoalId의 기본값-1L에 대해 — sentinel value 사용의 안전성을 확인해 주세요.
currentGoalId = -1L은 초기 상태에서photoLogs[-1L]이 호출되면 빈GoalPhotologUiModel()을 반환하여 크래시는 방지됩니다. 다만,reduceGoalId()가 호출되기 전에 UI가 렌더링되면 빈 데이터가 잠시 표시될 수 있습니다. init 블록에서reduceGoalId()가 먼저 호출되므로 현재 구조에서는 문제가 없지만, 의도를 명확히 하기 위해 상수로 분리하는 것도 고려해 볼 수 있습니다.feature/task-certification/src/main/java/com/twix/task_certification/detail/model/PhotoLogsUiModel.kt (1)
14-17:goalCache가 getter로 정의되어 접근할 때마다 Map이 재생성됩니다.
goalCache가get() = goalPhotolog.associateBy { ... }로 정의되어 있어,operator fun get(goalId)가 호출될 때마다 전체 리스트를 순회하여 새로운 Map을 생성합니다.TaskCertificationDetailUiState의 computed property들이currentGoal을 반복적으로 참조하므로, 한 번의 recomposition에서 Map이 여러 차례 불필요하게 생성됩니다.goal 수가 적으므로, Map을 생성하지 않고
find로 직접 검색하는 것이 더 간결하고 효율적입니다.♻️ 개선 제안
- private val goalCache: Map<Long, GoalPhotologUiModel> - get() = goalPhotolog.associateBy { it.goalId } - - operator fun get(goalId: Long): GoalPhotologUiModel = goalCache[goalId] ?: GoalPhotologUiModel() + operator fun get(goalId: Long): GoalPhotologUiModel = + goalPhotolog.find { it.goalId == goalId } ?: GoalPhotologUiModel()
| @SerialName("verificationDate") val verificationDate: String, | ||
| @SerialName("uploaderName") val uploaderName: String, | ||
| @SerialName("uploadedAt") val uploadedAt: String, | ||
| @SerialName("reaction") val reaction: String?, |
There was a problem hiding this comment.
reaction 필드에 기본값 누락 — 역직렬화 실패 위험
comment은 String? = null로 기본값이 있지만, reaction은 String?만 선언되어 기본값이 없습니다. 서버 응답에서 "reaction" 키가 아예 없는 경우, kotlinx.serialization은 MissingFieldException을 발생시킵니다.
서버가 항상 이 필드를 포함한다면 괜찮지만, comment과의 일관성 및 방어적 코딩 관점에서 기본값 추가를 권장합니다.
🛡️ 제안하는 변경
- `@SerialName`("reaction") val reaction: String?,
+ `@SerialName`("reaction") val reaction: String? = null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @SerialName("reaction") val reaction: String?, | |
| `@SerialName`("reaction") val reaction: String? = null, |
🤖 Prompt for AI Agents
In
`@core/network/src/main/java/com/twix/network/model/response/photolog/PhotologDetailResponse.kt`
at line 15, The PhotologDetailResponse data class declares
`@SerialName`("reaction") val reaction: String? without a default which can cause
kotlinx.serialization MissingFieldException when the server omits the key;
update the declaration for reaction in PhotologDetailResponse to provide a
default (e.g., reaction: String? = null) similar to the comment field so
deserialization is safe and consistent.
...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
Outdated
Show resolved
Hide resolved
dogmania
left a comment
dogmania
left a comment
There was a problem hiding this comment.
상태 업데이트하는 로직이 UiModel, UiState 여기저기 퍼져있는 것만 뷰모델로 통합하면 좋을 것 같은데 다른 의견 있으시면 말씀해주세요!
| private val goalCache: Map<Long, GoalPhotologUiModel> | ||
| get() = goalPhotolog.associateBy { it.goalId } |
There was a problem hiding this comment.
캐시를 활용하려고 하신 거 같은데 되게 좋은 거 같아요! 그런데 지금 형태는 매번 associateBy가 실행될 수도 있을 거 같아서 의도치 않게 반복 생성될 수 있어요. 그래서 차라리 goalCache를 PhotoLogUiModel의 생성자 필드로 옮겨서 copy할 때 같이 갱신하도록 하고 toUiModel에서 추가해주는 건 어떨까요
|
|
||
| operator fun get(goalId: Long): GoalPhotologUiModel = goalCache[goalId] ?: GoalPhotologUiModel() | ||
|
|
||
| fun updatePartnerReaction( |
There was a problem hiding this comment.
실제 상태 업데이트하는 로직은 뷰모델로 옮겨주세요! 지금 UiState, UiModel이 여러개로 분산되어 있고 update도 퍼져 있어서 유지보수 과정에서 어려움이 있을 거 같아요
There was a problem hiding this comment.
다른 리뷰도 비슷한 내용이라 요기에 한번에 답변 남길게 👍
현수가 말한 것 처럼 상태 업데이트 로직이 분산되어 있는건 맞아 ㅇㅅㅇ,,,
다만 이렇게 구현했던 이유는 내가 일전에 ViewModel에 모든 상태 변환을 두니까
ViewModel이 너~~~무 비대해져서 이걸 리팩터링 하는데 너무 힘들었던 경험이 있어서 UiState내에 위치 시키고
ViewModel은 이 함수들을 사용해서 상태를 업데이트하면 ViewModel 다이어트가 가능하더라구
다만 이렇게 하니까 단순히 값 하나만 바꿔도 함수를 만들어야 하더라구 ...
그래서 이 상태 업데이트 로직을 만드는 기준이 되게 애매해지는걸 나도 코드 짜면서 느꼈어
근데 다시 고민해보니 이것도 결국에 ViewModel에서 함수를 잘 분리하면 해결되는 것 같네?
이번 리뷰를 반영하면서 PhotoLogsUiModel, GoalPhotologUiModel 등 복잡하게 나누어진 UiState, UiModel 들을 TaskCertificationDetailUiState 하나로 통일 하는 방향으로 리팩터링 했어
이유는 UiModel과 UiState에 있는 상태 업데이트를 ViewModel로 옮기니까 이란 괴랄한 코드가 나오더라구.. 😓
private fun reduceReaction(reaction: GoalReactionType) {
reduce {
currentState.copy(
photoLogs = currentState.photoLogs.copy(
goalPhotolog = currentState.photoLogs.goalPhotolog.copy(
partnerPhotolog = currentState.photoLogs.goalPhotolog.myPhotolog?.copy(
reaction = reaction,
),
)
)
)
}
}
이걸 보니 지금 UiState가 너무 과하게 분리되었단 생각이 들어서 하나의 UiState로 통일했어 !
리뷰 반영 커밋
TODO
내가 다른 코드들도 다 이렇게 짜놔서 어제 이야기한 컨벤션 내용에 추가해서 나중에 한번에 리팩터링할게 !
| get() = partnerPhotolog?.uploadedAt?.let { RelativeTimeFormatter.format(it) } ?: "" | ||
|
|
||
| fun updatePartnerReaction(reaction: GoalReactionType): GoalPhotologUiModel = | ||
| copy(partnerPhotolog = partnerPhotolog?.updateReaction(reaction)) |
| photoLogs.partnerPhotologs.isCertificated | ||
| currentShow == BetweenUs.PARTNER && currentGoal.isPartnerCertificated | ||
|
|
||
| fun toggleBetweenUs() = |
There was a problem hiding this comment.
UiModel이나 UiState에서는 파생 상태 정도만 관리하는 걸로 하고 상태 업데이트 로직은 다 뷰모델로 옮겨주세요
| } | ||
|
|
||
| private fun reduceReaction(reaction: GoalReactionType) { | ||
| reduce { updatePartnerReaction(reaction) } |
There was a problem hiding this comment.
서버랑 통신하는 로직이 빠져있는 것 같은데 아직 구현이 안된 건가요??
There was a problem hiding this comment.
요건 이거 바로 다음 PR에 적용 되어있어 !
굳이 PR을 나눈 이유는 추후 PR을 찾아보고 싶을 때 내가 구현했던 핵심적인 코드들을 찾아보기 쉬워서야 !
예를 들면 이번 리액션 API를 적용하면서 디바운싱을 적용했는데 나중에 이 내용만 찾아보고 싶으면 바로 필터링 해서 찾기 편하더라구 🙂
| ?: error(GOAL_ID_NOT_FOUND) | ||
|
|
||
| private val targetDate: String = | ||
| savedStateHandle[NavRoutes.TaskCertificationDetailRoute.ARG_DATE] | ||
| ?: error(TARGET_DATE_NOT_FOUND) |
There was a problem hiding this comment.
오 이런식으로 네비게이션 파라미터 값을 꺼낼 수 있다는 건 처음 알았어요 신기하네요
There was a problem hiding this comment.
savedStateHandle을 사용하면 Navigation BackStackEntry에서 값을 꺼내서
UI → ViewModel로 전달하는 과정을 줄일 수 있어서 편한 것 같더라!
무엇보다 savedStateHandle은 프로세스가 죽어도 재생성 시 자동으로 상태를 복원해줘서 안정성 면에서도 좋은 것 같아 👍
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
feature/task-certification/src/main/java/com/twix/task_certification/detail/model/TaskCertificationDetailUiState.kt (1)
22-64: 표시 대상 선택 로직 중복을 줄이면 일관성 관리가 쉬워집니다.
현재currentShow기준 분기가 여러 프로퍼티에 반복되어, 추후 분기 로직 변경 시 누락 위험이 있습니다. 공통 선택 결과를 하나의 프로퍼티로 묶어 재사용하면 유지보수가 쉬워집니다.♻️ 제안 리팩터링
data class TaskCertificationDetailUiState( val goalId: Long = -1L, val currentShow: BetweenUs = BetweenUs.PARTNER, @@ val partnerPhotolog: PhotologDetail? = null, ) : State { + private val displayedPhotolog: PhotologDetail? + get() = when (currentShow) { + BetweenUs.ME -> myPhotolog + BetweenUs.PARTNER -> partnerPhotolog + } + val isDisplayedGoalCertificated: Boolean get() = when (currentShow) { BetweenUs.ME -> myPhotolog != null BetweenUs.PARTNER -> partnerPhotolog != null } @@ val displayedGoalUpdateAt: String get() = - when (currentShow) { - BetweenUs.ME -> - myPhotolog?.uploadedAt?.let { - RelativeTimeFormatter.format(it) - } ?: "" - - BetweenUs.PARTNER -> - partnerPhotolog?.uploadedAt?.let { - RelativeTimeFormatter.format( - it, - ) - } ?: "" - } + displayedPhotolog?.uploadedAt?.let { RelativeTimeFormatter.format(it) } ?: "" @@ val displayedGoalImageUrl: String? - get() = - when (currentShow) { - BetweenUs.ME -> myPhotolog?.imageUrl - BetweenUs.PARTNER -> partnerPhotolog?.imageUrl - } + get() = displayedPhotolog?.imageUrl @@ val displayedGoalComment: String? - get() = - when (currentShow) { - BetweenUs.ME -> myPhotolog?.comment - BetweenUs.PARTNER -> partnerPhotolog?.comment - } + get() = displayedPhotolog?.comment🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/task-certification/src/main/java/com/twix/task_certification/detail/model/TaskCertificationDetailUiState.kt` around lines 22 - 64, Multiple properties duplicate the same when(currentShow) branching (BetweenUs.ME / BetweenUs.PARTNER) causing maintenance risk; introduce one or two shared computed properties (e.g., selectedPhotolog: Photolog? and selectedNickname: String) that return myPhotolog/partnerPhotolog and myNickname/partnerNickname based on currentShow, then rewrite displayedGoalCertificated, displayedGoalUpdateAt, displayedGoalImageUrl, displayedGoalComment, and displayedNickname to read from selectedPhotolog/selectedNickname (use RelativeTimeFormatter.format on selectedPhotolog?.uploadedAt where needed) so branching is centralized in the selected* accessors (keep current property names intact for external callers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/model/TaskCertificationDetailUiState.kt`:
- Around line 75-89: The current PhotoLogs.toUiState(goalId: Long) returns a
default TaskCertificationDetailUiState() when the goal isn't found, which drops
the requested goalId; change the fallback to preserve the requested id (e.g.,
return TaskCertificationDetailUiState(goalId = goalId)) or return an explicit
error state field on TaskCertificationDetailUiState if you prefer signalling
failure; update the toUiState function to return
TaskCertificationDetailUiState(goalId = goalId, /* optionally isError = true */)
instead of the parameterless constructor so downstream actions/navigation can
still identify the requested goal.
---
Nitpick comments:
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/model/TaskCertificationDetailUiState.kt`:
- Around line 22-64: Multiple properties duplicate the same when(currentShow)
branching (BetweenUs.ME / BetweenUs.PARTNER) causing maintenance risk; introduce
one or two shared computed properties (e.g., selectedPhotolog: Photolog? and
selectedNickname: String) that return myPhotolog/partnerPhotolog and
myNickname/partnerNickname based on currentShow, then rewrite
displayedGoalCertificated, displayedGoalUpdateAt, displayedGoalImageUrl,
displayedGoalComment, and displayedNickname to read from
selectedPhotolog/selectedNickname (use RelativeTimeFormatter.format on
selectedPhotolog?.uploadedAt where needed) so branching is centralized in the
selected* accessors (keep current property names intact for external callers).
...ion/src/main/java/com/twix/task_certification/detail/model/TaskCertificationDetailUiState.kt
Show resolved
Hide resolved
…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
2 participants
이슈 번호
#83
리뷰/머지 희망 기한 (선택)
작업내용
결과물
default.mp4
리뷰어에게 추가로 요구하는 사항 (선택)
사진을 업로드하기까지 시간이 좀 걸려서 이 부분 개선이 필요해보여 🥲
사진 업로드 전에 뒤로가기를 하거나 화면을 이탈하면 인증 화면으로 돌아올 때 렌더링이 안되는데 이 부분은 추후 수정할게 !
목표를 상하로 스와이프 하는 기능은 아직 미구현이야 !