인증샷 보기 UI 구현 #73
인증샷 보기 UI 구현 #73
Conversation
…-certification-detail
- 상세 보기 화면 진입 시 `goalTitle`을 직접 전달하는 방식에서, `goalId`를 통해 서버에서 `goalTitle`을 조회하는 방식으로 변경했습니다. - 이에 따라 `TaskCertificationDetailRoute`의 탐색 인자에서 `goalTitle`이 제거되었습니다.
# Conflicts: # core/design-system/src/main/res/values/strings.xml # core/network/src/main/java/com/twix/network/di/ApiServiceModule.kt # data/src/main/java/com/twix/data/di/RepositoryModule.kt
📝 WalkthroughWalkthrough포토로그 조회 기능을 구현하기 위해 네트워크, 도메인, 데이터, UI 계층을 확장하는 변경사항입니다. 새로운 Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentTextField.kt (1)
83-90:⚠️ Potential issue | 🟡 Minor
enabled = false일 때 클릭 이벤트가 여전히 포커스를 요청합니다.
enabled파라미터를BasicTextField에 전달하여 입력을 막고 있지만,Box의noRippleClickable(Line 88-89)과CommentCircle의noRippleClickable(Line 143-145)에서는enabled상태와 무관하게focusRequester.requestFocus()를 호출합니다.비활성 상태에서 포커스가 요청되면 키보드가 불필요하게 표시되거나 예상치 못한 UI 동작이 발생할 수 있습니다.
🛠️ 개선 제안
modifier = modifier .onGloballyPositioned { coordinates -> onPositioned(coordinates.boundsInRoot()) }.noRippleClickable { - focusRequester.requestFocus() + if (enabled) focusRequester.requestFocus() },
CommentCircle의 클릭 핸들러도 동일하게enabled체크를 추가해 주세요.
🤖 Fix all issues with AI agents
In `@core/design-system/src/main/res/drawable/ic_keepi_angry.xml`:
- Around line 1-5: 벡터 루트 태그 (<vector>)의 android:width 속성(132dp)과
android:viewportWidth 속성(133)이 불일치합니다; <vector>의 android:width 또는
android:viewportWidth 중 하나를 다른 값에 맞춰 동일한 숫자로 맞추어 (예: android:viewportWidth="132"
또는 android:width="133dp") 수정하여 수평 비율 일관성을 보장하세요. 참조 대상: <vector> 태그의
android:width 및 android:viewportWidth 속성.
In `@core/design-system/src/main/res/drawable/ic_keepi_sting.xml`:
- Around line 1-5: The review notes that ic_keepi_sting is intentionally larger
because BackgroundCard uses it as a decorative background, but ic_keepi_angry
(132x113dp) is unused; either remove ic_keepi_angry or normalize it to the
52x52dp reaction size and wire it into ReactionUiModel if you intend to use it.
Locate the drawable resource ic_keepi_angry.xml, confirm there are no
references, then either delete the file or update its
android:width/android:height and viewport attributes to match the 52x52dp
convention and add the appropriate entry in ReactionUiModel (and any mapping
used by Reaction UI) so sizes are consistent with other reaction icons; if
keeping ic_keepi_sting as a background, leave it unchanged and add a clarifying
comment in BackgroundCard.
In
`@core/network/src/main/java/com/twix/network/model/response/photolog/mapper/PhotologMapper.kt`:
- Line 17: The mapper currently uses uploadedAt.toString() which turns a null
uploadedAt into the literal string "null"; in PhotologMapper (the mapping that
builds PhotologDetailResponse.uploadedAt) change the assignment to preserve
nulls (e.g., use uploadedAt?.toString() or uploadedAt?.let { /* desired
formatting */ } ) so PhotologDetailResponse.uploadedAt remains null when the
source is null and does not propagate the string "null".
In `@core/network/src/main/java/com/twix/network/service/PhotoLogService.kt`:
- Around line 10-18: The `@GET` annotation in PhotoLogService.kt is inconsistent:
change the fetchPhotoLogs endpoint from `@GET`("/api/v1/photologs/goals/{goalId}")
to match the project convention without a leading slash (i.e.,
"api/v1/photologs/goals/{goalId}"), so getUploadUrl and fetchPhotoLogs resolve
relative to the base URL the same way; update only the annotation on
fetchPhotoLogs (function fetchPhotoLogs) to remove the leading '/' and ensure no
other endpoints in this file use a leading slash.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/certification/component/ReactionBox.kt`:
- Around line 28-91: ReactionBox duplicates ReactionBar UI logic; extract the
shared UI into a single reusable composable (e.g., ReactionSelector) that takes
parameters for onSelectReaction: (GoalReactionType)->Unit, selectedReaction:
GoalReactionType?, modifier: Modifier, and any visual tweaks (shape, height,
shadow offsets, divider color) so both ReactionBox and ReactionBar simply call
ReactionSelector. Move the loop over ReactionUiModel.entries, the Row layout,
per-item Box/Image rendering and the VerticalDivider logic into
ReactionSelector, and update ReactionBox and ReactionBar to delegate to it while
passing their specific parameters (if any).
- Around line 76-79: The Image in ReactionBox currently sets contentDescription
= null; update accessibility by adding a `@StringRes` property (e.g.,
contentDescriptionRes: Int) to ReactionUiModel and use
stringResource(reaction.contentDescriptionRes) for the Image's
contentDescription in ReactionBox (and any other place constructing the Image),
ensuring all ReactionUiModel instances are provided appropriate string resource
IDs like "happy", "love", etc.; also update tests/usages where ReactionUiModel
is constructed to include the new contentDescriptionRes.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/component/PhotologCard.kt`:
- Around line 29-44: In PhotologCard's Box modifier chain the
.clip(RoundedCornerShape(12.dp)) is applied before .border(...), causing the
border to be clipped; move .border(...) earlier than .clip(...) (i.e., apply
.border(...) then .clip(...)) so the full 1.6.dp border is visible, or
alternatively remove .clip(...) and supply the shape to background (use
background(shape = RoundedCornerShape(12.dp))) to preserve the rounded
background and border correctly.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/model/PhotoLogsUiModel.kt`:
- Around line 19-27: The current PhotoLogs.toUiModel() uses
photologDetails.first { it.isMine } and first { !it.isMine } which will throw if
no element matches; change to use firstOrNull and provide safe fallbacks: obtain
val my = photologDetails.firstOrNull { it.isMine } and val partner =
photologDetails.firstOrNull { !it.isMine }, then call my?.toUiModel(myNickname)
?: emptyList() (or an appropriate empty/default photolog list) and
partner?.toUiModel(partnerNickname) ?: emptyList() when creating the
PhotoLogsUiModel so myPhotologs/partnerPhotologs never cause a crash.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt`:
- Around line 93-107: The passed-in modifier is being applied twice (to the
outer Column and again to the inner Box), which can cause external
paddings/sizes to be duplicated; update the Box usage inside
TaskCertificationDetail to stop reusing the parameterized modifier and instead
use a fresh Modifier (e.g., Modifier.fillMaxWidth()) so only the Column receives
the external modifier and the Box uses a local Modifier to control width.
- Around line 107-144: BackgroundCard and ForegroundCard are being passed the
same PhotologDetailUiModel based on uiState.currentShow; change BackgroundCard
to receive the opposite side's data so the two stacked cards show different
people. Specifically, in the BackgroundCard call swap the conditional mapping so
that when uiState.currentShow == BetweenUs.ME you pass
uiState.photoLogs.partnerPhotologs, and when currentShow == BetweenUs.PARTNER
you pass uiState.photoLogs.myPhotologs; leave ForegroundCard mapping (and
rotation/currentShow props) unchanged.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt`:
- Line 32: In TaskCertificationDetailViewModel update the reduce call inside the
photo logs success handler so it uses the API response's goalTitle rather than
the current UI state's value; replace the current use of goalTitle in reduce {
copy(photoLogs = it.toUiModel(), goalTitle = goalTitle) } with the response
field (use it.goalTitle) so the copied TaskCertificationDetailUiState reflects
the API-provided goalTitle.
- Line 24: The current handler for TaskCertificationDetailIntent.Sting uses
TODO("찌르기 API 연동") which throws NotImplementedError at runtime when the button
dispatches that intent; replace the TODO with a safe no-op or emit a UI effect
(e.g., a "준비 중" toast/event) so the app doesn't crash—update the
TaskCertificationDetailViewModel's handling of
TaskCertificationDetailIntent.Sting to either do nothing or send a user-facing
"coming soon" UI event instead of calling TODO().
In `@feature/task-certification/src/main/res/values/strings.xml`:
- Line 6: task_certification_image_upload currently has the same text as
task_certification_title; decide whether they should be distinct or
consolidated: if they are intended to be the same, replace the duplicate string
usage with a single resource reference (use task_certification_title from
callers or vice versa) or add an XML comment next to
task_certification_image_upload to document the intentional duplication; if they
should differ, update the value of task_certification_image_upload to the
correct, distinct text and ensure callers use the appropriate resource name
(task_certification_image_upload vs task_certification_title).
🧹 Nitpick comments (16)
feature/task-certification/src/main/res/values/strings.xml (1)
2-2: XML 주석 닫기 태그가 비표준입니다.
<!-- 인증샷 보기 촬영 화면 !-->과<!-- 인증샷 보기 화면 !-->에서 닫기 태그가!-->로 되어 있습니다. 표준 XML 주석 닫기 태그는-->입니다. Android에서 동작에는 문제가 없지만, 표준을 따르는 것이 좋습니다.✏️ 수정 제안
- <!-- 인증샷 보기 촬영 화면 !--> + <!-- 인증샷 보기 촬영 화면 --> ... - <!-- 인증샷 보기 화면 !--> + <!-- 인증샷 보기 화면 -->Also applies to: 9-9
feature/task-certification/src/main/java/com/twix/task_certification/detail/component/PhotologCard.kt (1)
47-57: Preview 함수의 가시성이 컴포넌트와 불일치합니다.
PhotologCard는internal로 선언되어 있지만,PhotologCardPreview는public입니다. Preview 함수는 외부에 노출될 필요가 없으므로private으로 변경하는 것이 좋습니다.♻️ 수정 제안
`@Preview` `@Composable` -fun PhotologCardPreview() { +private fun PhotologCardPreview() {core/util/src/main/java/com/twix/util/RelativeTimeFormatter.kt (1)
8-30:Instant.now()직접 호출로 인해 단위 테스트가 어렵습니다.현재
Instant.now()를 직접 호출하고 있어 시간 기반 로직을 테스트하기 어렵습니다.Clock을 주입받도록 하면 테스트에서 시간을 제어할 수 있습니다.또한
uploadedAt이 미래 시각인 경우,duration이 음수가 되어minutes < 1이 참이 되므로 "방금 전"으로 표시됩니다. 의도된 동작인지 확인이 필요합니다.♻️ Clock 주입 제안
+import java.time.Clock + object RelativeTimeFormatter { - fun format(uploadedAt: String): String = + fun format(uploadedAt: String, clock: Clock = Clock.systemUTC()): String = runCatching { val uploaded = Instant.parse(uploadedAt) - val now = Instant.now() + val now = clock.instant() val duration = Duration.between(uploaded, now)feature/task-certification/src/main/java/com/twix/task_certification/detail/model/ReactionUiModel.kt (1)
6-15: 프로퍼티 이름imageResources→imageResource로 변경 권장단일 리소스 ID를 담고 있으므로 복수형
imageResources보다 단수형imageResource가 의미를 더 정확히 전달합니다. 복수형을 사용하면 리스트나 배열을 기대하게 되어 혼동을 줄 수 있습니다.♻️ 이름 변경 제안
enum class ReactionUiModel( val type: GoalReactionType, - val imageResources: Int, + val imageResource: Int, ) {feature/task-certification/src/main/java/com/twix/task_certification/certification/component/ReactionBox.kt (1)
82-82: 매직 넘버4대신ReactionUiModel.entries.size - 1사용 권장리액션 종류가 추가되거나 제거될 때 이 숫자를 함께 수정해야 한다는 점을 놓치기 쉽습니다.
ReactionUiModel.entries.size - 1로 대체하면 enum 변경에 자동으로 대응됩니다.♻️ 수정 제안
- if (index < 4) { + if (index < ReactionUiModel.entries.size - 1) {feature/task-certification/src/main/java/com/twix/task_certification/detail/component/TaskCertificationDetailTopBar.kt (2)
82-99:showModify가false일 때 불필요한 클릭 영역이 남아있습니다.현재
noRippleClickable이showModify값과 무관하게 항상 적용되고 있어, 수정 버튼이 보이지 않을 때도 해당 영역이 클릭 가능합니다. 실질적으로onClickModify가null이면 아무 동작도 하지 않지만, 접근성(TalkBack 등) 측면에서 빈 클릭 영역은 혼란을 줄 수 있습니다.showModify가true일 때만 clickable을 적용하는 것이 더 적절합니다.♻️ 개선 제안
Box( modifier = Modifier .width(60.dp) .fillMaxHeight() .then( if (showModify) Modifier.background(GrayColor.C100) else Modifier, - ).noRippleClickable { onClickModify?.invoke() }, + ).then( + if (showModify) Modifier.noRippleClickable { onClickModify?.invoke() } else Modifier, + ), contentAlignment = Alignment.Center, ) {
63-67:contentDescription을 문자열 리소스로 분리하는 것을 권장합니다.
"back"이 하드코딩된 영문 문자열로 되어 있어, 다국어 지원 및 접근성 측면에서stringResource를 사용하는 것이 바람직합니다.feature/task-certification/src/main/java/com/twix/task_certification/detail/component/ForegroundCard.kt (1)
34-36:stringResource의 포맷 인자를 직접 전달하는 방식이 더 관용적입니다.현재
stringResource(...).format(...)방식도 동작하지만, Compose에서는stringResource(resId, formatArgs)오버로드를 사용하는 것이 더 관용적이고 안전합니다..format()은 Locale 설정이 반영되지 않을 수 있기 때문입니다.♻️ 개선 제안
BetweenUs.PARTNER -> - stringResource(R.string.task_certification_detail_partner_not_task_certification) - .format(uiModel.nickName) + stringResource( + R.string.task_certification_detail_partner_not_task_certification, + uiModel.nickName, + )domain/src/main/java/com/twix/domain/model/photolog/PhotologDetail.kt (1)
3-12: 도메인 모델에서verificationDate가String?타입인 점은 토론해볼 만합니다.현재 대부분의 nullable 필드는 인증 전 상태를 표현하기 위한 것으로 이해됩니다. 다만
verificationDate를String?으로 두면, 도메인 레이어에서 날짜에 대한 타입 안전성이 없어지고, 포맷 의존성이 생깁니다.LocalDate나Instant같은 타입을 사용하고 포맷팅은 UI/Presentation 레이어에서 처리하는 방식은 어떨까요?물론 현재 단계에서는 API 응답을 그대로 전달하는 것이 간결할 수 있으므로, 향후 리팩토링 시 고려해주시면 좋겠습니다.
feature/task-certification/src/main/java/com/twix/task_certification/detail/component/CertificatedCard.kt (3)
50-50: Preview 함수의 가시성이private이 아닙니다.같은 PR의
ForegroundCardPreview는private으로 선언되어 있는데, 이 Preview는public입니다. Preview 함수는 외부에서 호출될 필요가 없으므로private으로 통일하는 것이 좋습니다.♻️ 개선 제안
-fun CertificatedCardPreview() { +private fun CertificatedCardPreview() {
37-38:CommentUiModel이 매 recomposition마다 새로 생성됩니다.
enabled = false인 읽기 전용 텍스트 필드이므로 기능상 문제는 없지만,uiModel.comment가 변경되지 않는 한 매번 새TextFieldValue와CommentUiModel객체를 생성하는 것은 불필요한 할당입니다.remember로 감싸면 불필요한 객체 생성을 줄일 수 있습니다.
25-35: AsyncImage에 placeholder/error 이미지가 없습니다.네트워크 지연이나 로드 실패 시 빈 화면이 표시됩니다. 사용자 경험을 위해
placeholder와error파라미터 추가를 고려해주세요. 향후 PR에서 처리할 계획이라면 무시하셔도 됩니다.feature/task-certification/src/main/java/com/twix/task_certification/navigation/TaskCertificationGraph.kt (1)
33-35: fallback 값의 타입이 일관되지 않습니다.Line 29에서
defaultValue = -1L(Long)을 사용하고 있지만, Line 35의 fallback은?: -1(Int)입니다. Kotlin에서 자동 확장되므로 버그는 아니지만, 타입 일관성을 위해-1L로 통일하는 것이 좋습니다.♻️ 개선 제안
val id = backStackEntry.arguments?.getLong(NavRoutes.TaskCertificationDetailRoute.ARG_GOAL_ID) - ?: -1 + ?: -1Lcore/network/src/main/java/com/twix/network/model/response/photolog/mapper/PhotologMapper.kt (1)
7-9: 불필요한@OptIn(ExperimentalTime::class)어노테이션이 매퍼 함수에서
kotlin.time관련 API를 사용하지 않으므로,@OptIn(ExperimentalTime::class)와 해당 import는 불필요합니다. 사용하지 않는 opt-in은 코드 독자에게 혼란을 줄 수 있으므로 제거하는 것이 좋습니다.🧹 제거 제안
-import kotlin.time.ExperimentalTime - -@OptIn(ExperimentalTime::class) fun PhotologDetailResponse.toDomain(): PhotologDetail =feature/task-certification/src/main/java/com/twix/task_certification/detail/component/BackgroundCard.kt (2)
32-37:currentShow파라미터가 사용되지 않습니다
BackgroundCard에 전달되는currentShow: BetweenUs파라미터가 함수 본문에서 전혀 참조되지 않고 있습니다. 사용하지 않는 파라미터는 API 표면을 불필요하게 넓히고, 호출부에서 값을 전달해야 하는 부담을 줍니다. 현재 사용 계획이 없다면 제거하고, 향후 필요할 때 추가하는 것이 YAGNI 원칙에 부합합니다.♻️ 제거 제안
`@Composable` fun BackgroundCard( uiModel: PhotologDetailUiModel, - currentShow: BetweenUs = BetweenUs.PARTNER, buttonTitle: String, onClick: () -> Unit, rotation: Float, ) {
86-101: Preview 함수의 가시성과 네이밍Preview 함수가
public으로 노출되어 있습니다. 내부 컴포넌트의 Preview이므로private으로 제한하면 모듈 외부 노출을 방지할 수 있습니다. 사소한 부분이지만 일관성을 위해 참고해 주세요.
core/network/src/main/java/com/twix/network/model/response/photolog/mapper/PhotologMapper.kt
Show resolved
Hide resolved
...rtification/src/main/java/com/twix/task_certification/certification/component/ReactionBox.kt
Show resolved
Hide resolved
...rtification/src/main/java/com/twix/task_certification/certification/component/ReactionBox.kt
Show resolved
Hide resolved
...sk-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt
Outdated
Show resolved
Hide resolved
...sk-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt
Outdated
Show resolved
Hide resolved
...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
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@feature/task-certification/src/main/res/values/strings.xml`:
- Line 2: Replace the nonstandard XML comment closing sequences in the
strings.xml comments (e.g., change "<!-- 인증샷 보기 촬영 화면 !-->" and the similar
comment at line 8) to use the standard "-->" closing token; locate the comment
strings in the file (search for "인증샷 보기 촬영 화면" and the other flagged comment)
and update "!-->" to "-->" so the comments conform to XML syntax.
🧹 Nitpick comments (3)
feature/task-certification/src/main/java/com/twix/task_certification/certification/TaskCertificationScreen.kt (1)
267-317:DimmedScreen의 가시성(visibility) 검토를 제안드립니다.
DimmedScreen이public으로 선언되어 있지만, 이 파일 내에서만 사용되고 있습니다. 모듈 외부에서 재사용할 계획이 아니라면internal또는private으로 제한하면 API 표면을 줄이고 의도를 명확히 할 수 있습니다.♻️ 제안
`@Composable` -fun DimmedScreen( +private fun DimmedScreen( isFocused: Boolean, textFieldRect: Rect, guideTextRect: Rect, focusManager: FocusManager, ) {feature/task-certification/src/main/res/values/strings.xml (1)
9-9: 다국어 지원을 위해%s대신%1$s사용을 권장합니다.
%s님은\n아직…에서 포맷 인자가 하나뿐이지만, Android 다국어 지원 시 언어별로 인자 순서가 달라질 수 있으므로 위치 지정 인자(%1$s)를 사용하는 것이 안전합니다. 이는 Android Lint에서도 권장하는 패턴입니다.♻️ 수정 제안
- <string name="task_certification_detail_partner_not_task_certification">%s님은\n아직…</string> + <string name="task_certification_detail_partner_not_task_certification">%1$s님은\n아직…</string>As per coding guidelines, "다국어 지원을 고려했는가?"
feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt (1)
154-169: ReactionSection 잘 분리되어 있습니다 👍표시 여부(
visible)에 따른 조건부 렌더링이 깔끔하고, private 접근 제한자로 캡슐화도 적절합니다.작은 참고: Line 157의
reaction파라미터에 기본값null이 있는데, 현재 호출부(Line 148)에서 항상 값을 전달하고 있으므로 기본값이 불필요합니다. 필수 파라미터로 변경하면 호출부에서의 누락을 컴파일 타임에 잡을 수 있습니다.
1 participant
이슈 번호
#52
리뷰/머지 희망 기한 (선택)
작업내용
결과물
리뷰어에게 추가로 요구하는 사항 (선택)
이번 PR은 기본 UI 만 구현했어 !
화면 이동과 인터렉션은 새로운 PR로 만드는게 나중에 참고하기 편할 것 같아서 이번 PR에선 API 연동과 기본 UI 구현쪽만 봐주면 될 것 같아