Conversation
- `PhotologDetailUiModel` 삭제 - `GoalPhotologUiModel` 추가하여 목표별 인증 정보를 관리하도록 변경 - 날짜 기반으로 전체 목표 인증 현황을 가져오도록 API 호출 방식 수정 - UI Model 구조 변경에 따른 화면 및 ViewModel 로직 수정
📝 WalkthroughWalkthrough포토로그(photolog) 기능이 목표 단위(goals)와 날짜(targetDate) 중심으로 재구성되었습니다. 네트워크 모델에 Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 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)
No actionable comments were generated in the recent review. 🎉 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.
| override suspend fun handleIntent(intent: TaskCertificationDetailIntent) { | ||
| when (intent) { | ||
| is TaskCertificationDetailIntent.Reaction -> reduceReaction(intent.type) | ||
| TaskCertificationDetailIntent.Sting -> TODO("찌르기 API 연동") | ||
| TaskCertificationDetailIntent.SwipeCard -> reduceShownCard() | ||
| } | ||
| } |
There was a problem hiding this comment.
TODO("찌르기 API 연동")는 런타임에 NotImplementedError를 발생시켜 앱이 크래시됩니다.
TaskCertificationDetailIntent.Sting은 UI에서 파트너 카드의 "찌르기" 버튼 클릭 시 dispatch되는데, TODO()는 NotImplementedError를 throw하므로 사용자가 해당 버튼을 누르면 앱이 즉시 크래시됩니다.
아직 API가 준비되지 않았다면, 크래시 대신 사용자에게 "준비 중" 토스트를 보여주거나 no-op으로 처리하는 것을 권장합니다.
🐛 크래시 방지를 위한 수정 제안
is TaskCertificationDetailIntent.Reaction -> reduceReaction(intent.type)
- TaskCertificationDetailIntent.Sting -> TODO("찌르기 API 연동")
+ TaskCertificationDetailIntent.Sting -> {
+ // TODO: 찌르기 API 연동 (`#이슈번호`)
+ }
TaskCertificationDetailIntent.SwipeCard -> reduceShownCard()📝 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.
| override suspend fun handleIntent(intent: TaskCertificationDetailIntent) { | |
| when (intent) { | |
| is TaskCertificationDetailIntent.Reaction -> reduceReaction(intent.type) | |
| TaskCertificationDetailIntent.Sting -> TODO("찌르기 API 연동") | |
| TaskCertificationDetailIntent.SwipeCard -> reduceShownCard() | |
| } | |
| } | |
| override suspend fun handleIntent(intent: TaskCertificationDetailIntent) { | |
| when (intent) { | |
| is TaskCertificationDetailIntent.Reaction -> reduceReaction(intent.type) | |
| TaskCertificationDetailIntent.Sting -> { | |
| // TODO: 찌르기 API 연동 (`#이슈번호`) | |
| } | |
| TaskCertificationDetailIntent.SwipeCard -> reduceShownCard() | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt`
around lines 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를 찾아 적용하세요.
...ication/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt
Outdated
Show resolved
Hide resolved
1 participant
이슈 번호
#83
리뷰/머지 희망 기한 (선택)
작업내용
결과물
default.mp4
리뷰어에게 추가로 요구하는 사항 (선택)
사진을 업로드하기까지 시간이 좀 걸려서 이 부분 개선이 필요해보여 🥲
사진 업로드 전에 뒤로가기를 하거나 화면을 이탈하면 인증 화면으로 돌아올 때 렌더링이 안되는데 이 부분은 추후 수정할게 !
목표를 상하로 스와이프 하는 기능은 아직 미구현이야 !