Open
Conversation
- 인증샷 편집 화면 및 ViewModel을 추가했습니다. - 촬영된 인증샷 이미지와 코멘트를 표시합니다. - '다시찍기' 버튼을 통해 카메라 화면으로 이동할 수 있습니다. - DI 설정에 `TaskCertificationEditorViewModel`을 추가했습니다.
- TopBar 높이 값 추가
# Conflicts: # feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@feature/task-certification/build.gradle.kts`:
- Line 13: The module only adds implementation(libs.kotlinx.serialization.json)
but doesn't apply the Kotlin serialization plugin; fix by either updating
FeatureConventionPlugin to apply<SerializationConventionPlugin>() (or apply the
plugin in the same style as DataConventionPlugin within FeatureConventionPlugin)
so the org.jetbrains.kotlin.plugin.serialization is available at compile time,
or add a plugins block in this module's build.gradle.kts that declares
alias(libs.plugins.kotlinx.serialization); ensure the `@Serializable-using`
classes are compiled with the serialization plugin enabled.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/certification/TaskCertificationViewModel.kt`:
- Around line 143-147: The when branch handling
NavRoutes.TaskCertificationRoute.From currently does nothing for EDITOR; update
the branch in TaskCertificationViewModel so that
NavRoutes.TaskCertificationRoute.From.EDITOR calls
taskCertificationRefreshBus.notifyChanged() (same as the DETAIL branch) so
TaskCertificationDetailViewModel (which subscribes to
taskCertificationRefreshBus.events and invokes fetchPhotolog()) will receive the
refresh signal after an upload from the editor; leave the HOME branch invoking
goalRefreshBus.notifyGoalListChanged() unchanged.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorRoute.kt`:
- Around line 143-179: The UI uses hardcoded Spacer heights (Spacer(103.dp) and
Spacer(101.dp)) around PhotologCard which can overflow on small screens; replace
these fixed gaps by either making the parent Column scrollable (e.g., add
verticalScroll) or use flexible layout like Modifier.weight on surrounding
containers to distribute space (adjust PhotologCard, Spacer, and AppRoundButton
placement accordingly), and if you must keep fixed values extract them as named
constants (e.g., PHOTolog_TOP_GAP, PHOTO_BOTTOM_GAP) with comments to clarify
intent; update references to PhotologCard, AppRoundButton, and the
onGloballyPositioned logic so the layout adapts without clipping.
- Around line 57-114: Implement the Save flow: in the ViewModel's handleIntent()
replace the TODO in the TaskCertificationEditorIntent.Save case to call
PhotoLogRepository (inject it into the ViewModel), perform the API call in a
coroutine, and emit appropriate TaskCertificationEditorSideEffect values for
success and failure (e.g., NavigateBack on success and ShowError(message) on
failure); add those SideEffect sealed subclasses to
TaskCertificationEditorSideEffect. Then update TaskCertificationEditorRoute to
collect viewModel.sideEffects (or uiState.sideEffect flow) and handle them by
calling navigateToBack on NavigateBack and using toastManager.show(...) for
ShowError, keeping existing permission and navigation logic intact so Save
triggers the network call and UI reacts to the emitted side effects.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorViewModel.kt`:
- Line 42: The current handler for TaskCertificationEditorIntent.Save uses
TODO("인증샷 수정 API 연동") which throws NotImplementedError at runtime; replace that
TODO with a safe temporary implementation in the
TaskCertificationEditorViewModel: handle TaskCertificationEditorIntent.Save by
logging the action and emitting a UI event or state (e.g., showToast /
SaveNotAvailable / SaveFailed) instead of throwing, so pressing Save won't crash
until the real API integration is implemented; update references around
TaskCertificationEditorIntent.Save and any existing event/state emission helpers
in the ViewModel to surface the toast/log to the UI.
🧹 Nitpick comments (20)
feature/task-certification/src/main/java/com/twix/task_certification/detail/component/TaskCertificationDetailTopBar.kt (1)
81-92:actionTitle이 null일 때 불필요한 클릭 핸들러가 설정됩니다현재
actionTitle이null이고onClickModify도null인 경우에도.noRippleClickable이 항상 적용됩니다. 기능적으로 문제는 없지만, 접근성(Accessibility) 관점에서 빈 박스가 클릭 가능 요소로 인식될 수 있습니다.
onClickModify가 null이 아닐 때만 클릭 핸들러를 연결하면 의도가 더 명확해질 수 있는데, 어떻게 생각하시나요?♻️ 개선 제안
Box( modifier = Modifier .width(60.dp) .fillMaxHeight() .background( if (actionTitle == null) { CommonColor.White } else { GrayColor.C100 }, - ).noRippleClickable { onClickModify?.invoke() }, + ).then( + if (onClickModify != null) { + Modifier.noRippleClickable(onClick = onClickModify) + } else { + Modifier + } + ), contentAlignment = Alignment.Center, ) {core/design-system/src/main/java/com/twix/designsystem/components/button/AppRoundButton.kt (1)
28-29: 다크 모드 지원 검토가 필요합니다현재
textColor와backgroundColor를 호출부에서 직접 하드코딩된 색상(GrayColor.C500,CommonColor.White)으로 전달하고 있습니다. 이 컴포저블 자체는Color파라미터를 받으므로 구조적으로는 문제가 없지만, 다크 모드에서 적절한 색상이 전달되는지는 호출부의 책임이 됩니다.디자인 시스템 컴포넌트로서 다크 모드 안전성을 높이려면, 기본값을 테마 기반 색상으로 제공하는 방안을 고려해볼 수 있습니다. 예를 들어:
textColor: Color = TwixTheme.colors.onSurface, backgroundColor: Color = TwixTheme.colors.surface,현재 필수 파라미터로 두는 설계도 유효하지만, 프리뷰에 다크 모드 프리뷰를 추가하면 시각적 검증에 도움이 됩니다.
As per coding guidelines, "Dark Mode 지원 여부" 확인이 필요합니다.
core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentTextField.kt (2)
95-106: 숨겨진 TextField의 접근성(Accessibility) 고려가 필요합니다
width(0.dp)와alpha(0f)로 숨겨진TextField는 TalkBack 등 스크린 리더에서 여전히 포커스를 받을 수 있어, 시각 장애 사용자에게 혼란을 줄 수 있습니다.직접 변경된 코드는 아니지만, 접근성 개선 시
semantics { invisibleToUser() }또는 커스텀contentDescription을 통해 스크린 리더가 이 컴포넌트의 역할을 올바르게 안내하도록 개선하는 것을 고려해보시면 좋겠습니다.As per coding guidelines,
core/design-system/**: 접근성(Accessibility) 고려 여부.
119-124: Material Design 3 및 다크 모드 지원을 위해 색상 토큰 도입이 필요합니다현재
GrayColor.C500(#171717- 매우 어두운 색상)이 하드코딩되어 있어 다크 모드에서 배경과의 명도 대비가 부족합니다. 이는 접근성 측면에서도 문제가 될 수 있습니다.설계 시스템이 Material Design 3를 준수하려면, 현재의 정적 색상 객체 대신
MaterialTheme.colorScheme의 동적 토큰(예:outline,outlineVariant)을 사용하여 라이트/다크 모드를 자동으로 지원해야 합니다.개선 방향:
- 테마 시스템을 Material Design 3 ColorScheme 기반으로 구축
- 고정 색상 대신
isSystemInDarkTheme()기반의 조건부 색상 선택 또는 ColorScheme 토큰 사용- 현재 디자인 시스템 전체에서 이 패턴이 반복되므로, 아키텍처 차원의 개선을 검토하면 좋겠습니다
core/navigation/build.gradle.kts (1)
5-5: Serialization 플러그인 및 의존성 추가가 적절합니다.Navigation 모듈에서
TaskCertificationSerializer를 통해 경로 데이터를 직렬화하기 위한 설정으로,implementation스코프 사용도 적절합니다.다만, 프로젝트에 serialization을 사용하는 모듈이 늘어날 경우,
twix.koin처럼 serialization 전용 Convention Plugin을 만드는 것도 고려해볼 수 있습니다. 현재feature/task-certification/build.gradle.kts에서도 동일한 의존성을 추가하고 있으므로 중복 설정을 줄일 수 있습니다.Also applies to: 12-14
core/design-system/src/main/java/com/twix/designsystem/components/photolog/PhotologCard.kt (1)
24-47: 재사용성을 높이기 위한 public 전환과 기본값 설정이 잘 되어 있습니다.
modifier파라미터를 외부에서 주입 가능하게 하고, 색상에 기본값을 제공한 점이 좋습니다.한 가지 고려할 점은,
CommonColor.White와GrayColor.C500같은 하드코딩된 색상이 다크 모드에서 적절하지 않을 수 있다는 것입니다. 디자인 시스템 가이드라인에 따르면 다크 모드 지원 여부를 확인해야 합니다. 현재 앱이 다크 모드를 지원하지 않는다면 괜찮지만, 향후 지원 계획이 있다면MaterialTheme.colorScheme기반의 테마 색상 사용을 검토해보시는 것이 좋겠습니다.feature/task-certification/src/main/java/com/twix/task_certification/certification/component/TaskCertificationTopBar.kt (1)
32-39: 닫기 버튼의contentDescription이null입니다 — 접근성 개선이 필요합니다.스크린 리더 사용자가 닫기 버튼의 용도를 인식할 수 없습니다. 디자인 시스템 리뷰 가이드에서 접근성(Accessibility) 고려를 요구하고 있으므로,
contentDescription에 의미 있는 문자열을 제공하는 것이 좋겠습니다.♿ 접근성 개선 제안
Image( imageVector = ImageVector.vectorResource(R.drawable.ic_close_c100), - contentDescription = null, + contentDescription = stringResource(R.string.word_cancel),As per coding guidelines, 디자인 시스템 리뷰 가이드: "접근성(Accessibility) 고려 여부".
core/design-system/src/main/java/com/twix/designsystem/components/photolog/CertificatedCard.kt (2)
20-48: Public 디자인 시스템 컴포넌트에modifier파라미터가 없습니다.
PhotologCard는modifier파라미터를 노출하고 있는 반면,CertificatedCard는 그렇지 않습니다. Public 컴포저블에서modifier를 노출하는 것은 Compose의 API 가이드라인에서 권장하는 패턴입니다. 호출부에서 패딩, 크기 등을 조정할 수 없게 됩니다.♻️ modifier 파라미터 추가 제안
`@Composable` fun CertificatedCard( imageUrl: String?, comment: String?, + modifier: Modifier = Modifier, ) { - Box(Modifier.fillMaxSize()) { + Box(modifier.fillMaxSize()) {
33-34: 인증샷 이미지의contentDescription이null입니다.디자인 시스템 가이드라인의 접근성 항목에 따라, 인증샷 이미지에 적절한 콘텐츠 설명을 제공하는 것이 좋겠습니다.
As per coding guidelines, 디자인 시스템 리뷰 가이드: "접근성(Accessibility) 고려 여부".
core/design-system/src/main/java/com/twix/designsystem/components/photolog/BackgroundCard.kt (1)
70-77: 찌르기 아이콘의contentDescription이null입니다.스크린 리더 사용자를 위해 의미 있는 설명을 추가하면 좋겠습니다.
♿ 접근성 개선 제안
Image( imageVector = ImageVector.vectorResource(R.drawable.ic_keepi_sting), - contentDescription = null, + contentDescription = stringResource(R.string.partner_sting),As per coding guidelines, 디자인 시스템 리뷰 가이드: "접근성(Accessibility) 고려 여부".
core/navigation/src/main/java/com/twix/navigation/serializer/TaskCertificationSerializer.kt (1)
5-12: 네이밍에 대해 한 가지 제안드립니다.
TaskCertificationSerializer라는 이름이 kotlinx.serialization의KSerializer<T>와 혼동될 수 있습니다. 이 클래스는 실제로 네비게이션 간 데이터를 전달하는 DTO(Data Transfer Object) 역할을 하므로,TaskCertificationEditorArgs또는TaskCertificationNavArgs같은 이름이 역할을 더 명확히 전달할 수 있습니다.기존 프로젝트 컨벤션이
*Serializer라면 유지해도 무방합니다.feature/task-certification/src/main/java/com/twix/task_certification/editor/model/TaskCertificationEditorUiState.kt (1)
9-16: State 정의가 MVI 패턴에 잘 부합합니다.
@Immutable어노테이션과copy()를 활용한 불변 상태 관리가 올바릅니다. 두 가지 의견을 드립니다:
isImageChanged: 현재 어떤 mutation helper에서도 업데이트되지 않습니다. "다시 찍기" 플로우에서 사용될 예정이라면 괜찮지만, 향후 구현 계획이 없다면 YAGNI 원칙에 따라 제거를 고려해 주세요.
photologId = -1: 센티널 값(-1) 대신Long?(nullable)을 사용하면 초기화되지 않은 상태를 타입 시스템으로 보호할 수 있습니다. 다만 기존 프로젝트 컨벤션이 -1을 사용한다면 일관성을 위해 유지해도 됩니다.core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentAnchorFrame.kt (2)
48-48:anchorBottom == 0fearly return에 대한 엣지 케이스 고려현재
0f를 "아직 측정되지 않은 상태"로 사용하고 있는데, 이론적으로 앵커 요소가 화면 최상단에 위치하여bottom이 실제로0f인 경우도 early return되어 CommentBox가 렌더링되지 않습니다. 실제 사용 맥락에서는 TopBar 등이 있어0f가 될 가능성은 극히 낮지만, 명시적으로null가능 타입(Float?)을 사용하면 "미측정"과 "실제 0" 상태를 구분할 수 있습니다.현재 구조에서 실질적 문제가 될 가능성은 낮으므로 참고만 해주세요.
40-104: 디자인 시스템 컴포넌트에 Preview가 빠져 있습니다.코딩 가이드라인에 따르면 Composable 함수에 Preview가 제공되어야 합니다.
CommentAnchorFrame은anchorBottom과CommentUiModel등의 파라미터가 필요하므로, 다양한 상태(포커스 on/off, 키보드 활성화 등)에 대한 Preview를 추가하면 디자인 검증에 도움이 됩니다.As per coding guidelines:
core/design-system/**— Compose UI 가이드라인에서 "Preview Composable이 제공되는가?" 항목에 해당합니다.feature/task-certification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorRoute.kt (2)
136-141: TopBar의onClickModify파라미터에onClickSave를 전달하는 부분이 혼란스러울 수 있습니다.
TaskCertificationDetailTopBar는 상세 화면용으로 설계되어onClickModify라는 파라미터명을 사용하지만, 편집 화면에서는 "저장" 기능으로 사용되고 있습니다. 현재 기능적으로는 문제없지만, 향후 유지보수를 위해 TopBar의 파라미터명을onClickAction처럼 범용적으로 변경하거나, 편집 화면 전용 TopBar를 만드는 것을 고려해 볼 수 있습니다.
169-179:AppRoundButton이onClick파라미터를 지원하도록 API를 개선해볼 수 있습니다.현재 코드는
AppRoundButton이onClick파라미터를 제공하지 않아 외부 Modifier로 클릭 핸들러를 분리하고 있습니다. 기능적으로는 정상 작동하며,noRippleClickable은 내부적으로Modifier.clickable()을 사용하므로 접근성 시맨틱 정보(버튼 역할, 클릭 액션)도 자동으로 제공됩니다.다만 컴포넌트 사용 입장에서 보면, 버튼이 클릭 핸들러를 직접 파라미터로 받을 수 있다면 더 직관적이고 명확한 API가 될 것입니다. 예를 들어:
fun AppRoundButton( text: String, textColor: Color, backgroundColor: Color, onClick: () -> Unit, // 추가 modifier: Modifier = Modifier, // ... 기타 파라미터 )이렇게 개선하면 사용처에서
modifier에noRippleClickable을 명시적으로 추가할 필요 없이, 더 간결한 코드를 작성할 수 있습니다.core/navigation/src/main/java/com/twix/navigation/NavRoutes.kt (1)
68-77:Json인스턴스 일관성 확인 필요Line 73에서
Json.encodeToString(data)는 기본Json설정을 사용합니다.TaskCertificationEditorViewModel의 디코딩 측(Line 26)에서도 기본Json을 사용하고 있어 현재는 동작하지만, 프로젝트 내 다른 곳에서 커스텀Json설정(예:ignoreUnknownKeys = true)을 사용한다면 인코딩/디코딩 불일치가 발생할 수 있습니다.공통
Json인스턴스를 DI나 싱글톤으로 관리하는 것을 고려해 보시면 향후 직렬화 설정 변경 시 일관성을 유지하기 쉬워집니다.feature/task-certification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorViewModel.kt (1)
20-28:serializer프로퍼티의 가시성과 에러 처리두 가지 포인트를 남깁니다:
serializer가public val로 노출되어 있는데, ViewModel 내부 초기화(reduceInitialState)에서만 사용된다면private으로 제한하는 것이 캡슐화에 더 적합합니다. 외부에서 접근이 필요한 경우라면 현재 그대로 유지해도 됩니다.
requireNotNull은 데이터가 없을 때IllegalArgumentException으로 크래시합니다. 딥링크나 프로세스 복원 등 예외적 상황에서SavedStateHandle에 데이터가 없을 수 있는데, 이 경우 사용자에게 에러 화면을 보여주거나 뒤로 이동하는 것이 더 나은 UX일 수 있습니다. 현재 Navigation 흐름에서 항상 보장된다면 괜찮지만, 방어적 처리를 고려해 보시는 것도 좋겠습니다.feature/task-certification/src/main/java/com/twix/task_certification/detail/model/TaskCertificationDetailUiState.kt (1)
65-72:toSerializer()에서myPhotolog가 null인 경우의 방어 처리가 잘 되어 있습니다
canModify가true일 때만 수정 버튼이 노출되므로, 실제로myPhotolog가 null인 상태에서 이 함수가 호출될 가능성은 낮습니다. 다만photologId = -1이 서버 API에 그대로 전달되면 의도치 않은 결과가 발생할 수 있으니, 에디터 ViewModel의 Save 로직 구현 시 유효성 검증을 추가하시면 좋겠습니다.feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt (1)
63-63:navigateToEditor에TaskCertificationDetailUiState를 전달하는 설계에 대해
navigateToEditor의 파라미터로 전체UiState를 전달하고, navigation 레이어에서toSerializer()를 호출하는 구조입니다. 이 방식은 동작상 문제는 없지만, Navigation 콜백이 UI 상태 전체를 알아야 한다는 점에서 결합도가 다소 높습니다.대안으로
navigateToEditor: (TaskCertificationSerializer) -> Unit으로 시그니처를 변경하고, 호출부에서navigateToEditor(uiState.toSerializer())로 변환하면 navigation 레이어가 UiState 타입에 의존하지 않게 됩니다. 다만 현재 구조에서도 충분히 동작하므로, 리팩터링 우선순위에 따라 판단하시면 됩니다.
...cation/src/main/java/com/twix/task_certification/certification/TaskCertificationViewModel.kt
Outdated
Show resolved
Hide resolved
...rtification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorRoute.kt
Show resolved
Hide resolved
...rtification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorRoute.kt
Outdated
Show resolved
Hide resolved
...ication/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorViewModel.kt
Outdated
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
dogmania
approved these changes
Feb 18, 2026
Member
dogmania
left a comment
dogmania
left a comment
There was a problem hiding this comment.
고생하셨습니다! 네비게이션 파라미터로 데이터 넘기는 방식은 장기적으로 봤을 때 id 기반으로 Room, 서버에서 데이터를 가져오는 식으로 수정하는 게 확장하거나 수정에 더 안정적일 거 같아요 지금 당장 수정할지 말지는 작업이 아직 남아있어서 같이 고민해봐요
Member
There was a problem hiding this comment.
네비게이션 전용으로 사용되는 거면 NavArgs로 네이밍을 짓는 건 어떨까요 패키지명도 args로 잡으면 좋을 거 같아요!
| ) = "task_certification/$goalId/${from.name}" | ||
| fun createRoute(data: DetailSerializer): String { | ||
| val json = Json.encodeToString(data) | ||
| val encoded = Uri.encode(json) |
Member
There was a problem hiding this comment.
네비게이션 전용으로 이렇게 인코딩해서 넣는 게 지금 당장에는 문제가 없을 거 같긴 하지만 나중에 데이터에 변경이 생기는 경우를 고려하면 좋을 거 같아요. 예를 들어서 앱 버전이 올라가면서 nullable이 추가되거나, 있던 변수가 사라지거나, 새로 추가되는 변수가 있거나 이러면 serialization 과정에서 앱이 죽어버릴 수도 있을 거 같아요
id랑 필요하면 날짜 정도만 넣어서 넘기는 걸로 하고 뷰모델에서 새로 데이터를 조회하거나 아니면 뷰모델을 공유하는 방식이 안정성 측면에서는 더 좋을 것 같습니다!
Member
Author
There was a problem hiding this comment.
나도 동의해 ! 현수가 말한 데이터들이나 반드시 필요한 값들을 전달하면 좋을 것 같아 !
ex) 코멘트 수정 화면 -> 인증샷 수정 화면 이동시 코멘트 수정 화면에서 수정한 코멘트 전달
요긴 인증샷 단일 조회 API가 생기면 변경점이 좀 생길 것 같아서 단일 조회 API로 수정되면 다시 이야기 해보자 🙂
Member
There was a problem hiding this comment.
형이 전체적으로 수정한다고 하셨는데 리마인드 차원에서 여기 업데이트 로직도 뷰모델로 옮겨주세요!
Comment on lines
150
to
155
| TaskCertificationDetailTopBar( | ||
| actionTitle = stringResource(DesR.string.word_save), | ||
| goalTitle = uiState.goalName, | ||
| onBack = onBack, | ||
| onClickModify = onClickSave, | ||
| ) |
Comment on lines
27
to
35
| private val serializer = | ||
| requireNotNull( | ||
| savedStateHandle | ||
| .get<String>(NavRoutes.TaskCertificationEditorRoute.ARG_DATA) | ||
| ?.let { encoded -> | ||
| val json = Uri.decode(encoded) | ||
| Json.decodeFromString<EditorSerializer>(json) | ||
| }, | ||
| ) { SERIALIZER_NOT_FOUND } |
Member
There was a problem hiding this comment.
지금 에러가 발생했을 때 로깅이나 예외처리가 없어서 앱이 그냥 죽어버릴 거 같은데 원인은 파악할 수 있게 try catch를 추가하거나 최소한의 안전 장치가 있으면 좋을 거 같아요
- `certification`, `detail`, `editor` 각 화면의 MVI (Model-View-Intent) 관련 클래스들을 `contact` 패키지로 이동하여 구조를 통일합니다. - 패키지 구조 변경에 따라 관련 import 구문을 수정합니다.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
chanho0908
changed the base branch from
feat/#84-certification-modify
to
develop
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
feature/task-certification/src/main/java/com/twix/task_certification/detail/component/swipe/SwipeableCard.kt (3)
173-175:⚠️ Potential issue | 🟡 Minoropacity 스프링에
stiffness누락 → 좌우/상하 애니메이션과 타이밍 불일치
offsetX,offsetY의 복귀 스프링(157~171번 줄)은dampingRatio와stiffness를 모두spec에서 가져오는데,opacity의 스프링만dampingRatio만 전달하고stiffness는 기본값을 사용합니다. 결과적으로 투명도 복귀 속도가 위치 복귀 속도와 달라져 시각적 불일치가 생깁니다.🛠 제안 수정
- launch { - opacity.animateTo(1f, spring(spec.springDamping)) - } + launch { + opacity.animateTo( + 1f, + spring( + dampingRatio = spec.springDamping, + stiffness = spec.springStiffness, + ), + ) + }🤖 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/component/swipe/SwipeableCard.kt` around lines 173 - 175, The opacity spring is missing the stiffness setting so its return timing differs from position springs; update the opacity.animateTo call (where opacity is animated) to use the same spring spec as offsetX/offsetY by passing both damping and stiffness from spec (e.g., use the same springDamping and springStiffness values or the same spec object used for offsetX/offsetY) so opacity’s timing matches the position animations.
54-56:⚠️ Potential issue | 🟠 Major🔴 드래그 중 매 프레임마다 불필요한 리컴포지션 발생
rotation값이 컴포지션 스코프(54~56번 줄)에서offsetX.value를 직접 읽고 있습니다.Animatable은MutableState로 백킹되기 때문에, 드래그 중snapTo가 호출될 때마다SwipeableCard전체가 리컴포지션됩니다.
offset { }람다(64번 줄)와graphicsLayer { }람다(73번 줄)는 각각 레이아웃/드로우 단계에서만 state를 읽으므로 리컴포지션을 유발하지 않습니다.rotation계산도graphicsLayer람다 내부로 옮겨 드로우 단계에서만 읽히도록 해야 합니다.공식 문서에 따르면
graphicsLayer람다 블록 안에서 State를 읽으면 레이어 속성 업데이트만 발생하고, 리컴포지션과 리레이아웃은 트리거되지 않습니다.⚡ 제안 수정: rotation 계산을 graphicsLayer 람다 내부로 이동
- /** - * 드래그 거리 기반 회전 - */ - val rotation = - (offsetX.value / spec.rotationFactor) - .coerceIn(-spec.maxRotation, spec.maxRotation) - Box( modifier = modifier ... /** * 회전 + 투명도 적용 */ .graphicsLayer { - rotationZ = rotation + rotationZ = (offsetX.value / spec.rotationFactor) + .coerceIn(-spec.maxRotation, spec.maxRotation) alpha = opacity.value }As per coding guidelines: "불필요한 recomposition 가능성은 없는가?" 항목 위반.
Also applies to: 73-76
🤖 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/component/swipe/SwipeableCard.kt` around lines 54 - 56, The current Compose code computes a top-level rotation val by reading offsetX.value (val rotation = (offsetX.value / spec.rotationFactor).coerceIn(...)) which causes recomposition on every snapTo; move that calculation into the graphicsLayer { } lambda so rotation is read during the draw phase only (use the same formula with offsetX.value / spec.rotationFactor and coerceIn using spec.maxRotation) and remove the top-level rotation val; keep offset { } and graphicsLayer { } usage intact so only layer properties update and the SwipeableCard composable no longer recomposes on every Animatable change.
77-89:⚠️ Potential issue | 🟡 Minor
onDragCancel미처리 → 제스처 취소 시 카드가 중간 위치에 고착
onDragCancel은 다른 제스처가 드래그를 가로채는 등 외부 요인으로 드래그가 취소될 때 호출됩니다. 현재onDragCancel이 지정되지 않아 기본값(빈 람다)이 사용되므로, 예를 들어 부모 스크롤러가 제스처를 가로채거나 전화 수신 등의 인터럽트가 발생하면 카드가 오프셋 위치에 그대로 남게 됩니다.🛠 제안 수정: onDragEnd의 복귀 로직과 동일하게 처리
detectDragGestures( onDrag = { change, dragAmount -> ... }, + onDragCancel = { + coroutineScope.launch { + launch { offsetX.animateTo(0f, spring()) } + launch { offsetY.animateTo(0f, spring()) } + launch { opacity.animateTo(1f, spring()) } + } + }, onDragEnd = { ... }, )Also applies to: 93-184
🤖 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/component/swipe/SwipeableCard.kt` around lines 77 - 89, The drag-cancel path is unhandled: add an onDragCancel handler to the detectDragGestures call(s) so cancellations run the same revert logic as onDragEnd — i.e., launch the coroutineScope and call offsetX.snapTo(...) and offsetY.snapTo(...) (the same values/animation used in onDragEnd) so the card returns to its intended position when a gesture is intercepted; update every detectDragGestures instance that currently omits onDragCancel to mirror the onDragEnd behavior.feature/task-certification/src/main/java/com/twix/task_certification/certification/component/CameraControlBar.kt (1)
85-113:⚠️ Potential issue | 🟡 Minor클릭 가능한
Image컴포저블에contentDescription = null— 접근성 개선을 권장합니다
ic_gallery,ic_camera_shutter,ic_camera_toggle,ic_camera_retake버튼은 모두 사용자와 상호작용하는 인터랙티브 요소임에도 불구하고contentDescription = null로 설정되어 있습니다. TalkBack과 같은 스크린 리더는 이 버튼들을 "버튼"으로 읽어줄 수 없어 시각 장애 사용자에게 어떤 역할인지 전달되지 않습니다.단순 장식용(decorative) 이미지에만
null을 사용하고, 인터랙티브 요소에는 의미 있는 설명을 제공하는 것이 좋습니다. 문자열 리소스는 이미 design-system으로 이전 중이므로, 함께 추가하는 것을 제안드립니다:♻️ 개선 예시 (ic_gallery 버튼 기준)
Image( imageVector = ImageVector.vectorResource(R.drawable.ic_gallery), - contentDescription = null, + contentDescription = stringResource(R.string.cd_gallery), modifier = Modifier .size(56.dp) .noRippleClickable(enabled = enabled, onClick = onClickGallery), )Also applies to: 128-136
🤖 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/certification/component/CameraControlBar.kt` around lines 85 - 113, In CameraControlBar, several interactive Image composables (the gallery, shutter, toggle and retake buttons referenced by the Image usages and callbacks onClickGallery, onCaptureClick, onToggleCameraClick and the retake image) set contentDescription = null; update each to provide meaningful contentDescription strings (use string resources) instead of null so screen readers announce the button purpose, e.g. add resource keys like R.string.cd_gallery_button, R.string.cd_capture_button, R.string.cd_toggle_camera_button, R.string.cd_retake_button and pass them into the Image composables' contentDescription via stringResource(...) or equivalent.core/design-system/src/main/java/com/twix/designsystem/components/button/AppRoundButton.kt (1)
25-34:⚠️ Potential issue | 🟠 Major디자인 시스템 컴포넌트에
Role.Button시맨틱과onClick파라미터가 없습니다
AppRoundButton은 시각적으로 버튼처럼 동작하지만, 클릭 처리를 modifier를 통해 호출 측에 완전히 위임합니다. 이 구조에서는 디자인 시스템 라이브러리에서 커스텀 컴포넌트를 만들 때Role시맨틱 속성을 직접 설정해야 하며,semanticsmodifier나clickablemodifier를 통해 설정할 수 있습니다.현재 설계대로면 이 컴포넌트를 사용하는 모든 호출 측이 직접
Role.Button시맨틱을 설정해야 하므로, 누락되기 쉬운 구조입니다. 디자인 시스템 컴포넌트는 접근성을 내부에서 보장하는 것이 바람직합니다.권장 개선 방향:
onClick람다를 파라미터로 받아 컴포넌트 내부에서role = Role.Button과 함께clickable을 처리하도록 API를 개선하는 것을 고려해 보세요.♿ API 개선 제안
+import androidx.compose.foundation.clickable +import androidx.compose.ui.semantics.Role `@Composable` fun AppRoundButton( text: String, textColor: Color, backgroundColor: Color, + onClick: (() -> Unit)? = null, modifier: Modifier = Modifier, textStyle: AppTextStyle = AppTextStyle.T2, borderColor: Color = GrayColor.C500, hasBorder: Boolean = true, ) { Box( - modifier = modifier, + modifier = modifier.then( + if (onClick != null) { + Modifier.clickable(role = Role.Button, onClick = onClick) + } else Modifier + ), ) {As per coding guidelines:
core/design-system/**— "접근성(Accessibility) 고려 여부"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/button/AppRoundButton.kt` around lines 25 - 34, AppRoundButton currently lacks an onClick parameter and does not set Role.Button semantics internally, forcing callers to add semantics manually; modify the AppRoundButton signature to accept an onClick: ()->Unit (nullable if needed) and inside the composable apply Modifier.clickable(role = Role.Button, onClick = onClick) (or use semantics with role = Role.Button if non-clickable variant) so the component itself manages accessibility; update usages of modifier in AppRoundButton to chain the clickable/semantics modifier before other modifiers and ensure text/content remains unchanged.feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt (1)
122-122:⚠️ Potential issue | 🟠 Major
TODO("찌르기 API 연동")는 런타임에NotImplementedError를 발생시킵니다.Kotlin의
TODO()는 실행 시 예외를 던지므로, 사용자가 "찌르기" 버튼을 누르면 앱이 크래시됩니다. 아직 미구현 상태라면 빈 동작이나 "준비 중" 토스트를 보여주는 것이 안전합니다.🔧 크래시 방지 개선안
- TaskCertificationDetailIntent.Sting -> TODO("찌르기 API 연동") + TaskCertificationDetailIntent.Sting -> { + showToast(R.string.task_certification_detail_sting_not_ready, ToastType.ERROR) + }🤖 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/TaskCertificationDetailViewModel.kt` at line 122, The TaskCertificationDetailIntent.Sting branch currently calls TODO("찌르기 API 연동") which throws NotImplementedError at runtime; replace this TODO in the TaskCertificationDetailViewModel (the handler for TaskCertificationDetailIntent.Sting) with a safe implementation such as a no-op or emitting a UI effect/event (e.g., show "준비 중" toast or log and return) so the app doesn't crash when the user taps the "찌르기" button; update the Sting intent handler to call the appropriate method (or emit an intent/result) instead of TODO to safely handle the unimplemented feature until the API integration is added.
🧹 Nitpick comments (32)
feature/task-certification/src/main/java/com/twix/task_certification/editor/contract/TaskCertificationEditorSideEffect.kt (1)
7-9:message: Int에@StringRes어노테이션을 추가하는 것이 좋습니다.
Int타입만으로는 이 값이 반드시 문자열 리소스 ID여야 한다는 제약이 없습니다. 실수로 임의의 정수값이 전달될 경우, 런타임에서야 오류가 발생합니다.@StringRes를 추가하면 IDE와 lint가 컴파일 타임에 이를 검증해 줍니다.✨ 개선 제안
+import androidx.annotation.StringRes + sealed interface TaskCertificationEditorSideEffect : SideEffect { data class ShowToast( - val message: Int, + `@StringRes` val message: Int, val type: ToastType, ) : TaskCertificationEditorSideEffect }🤖 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/editor/contract/TaskCertificationEditorSideEffect.kt` around lines 7 - 9, The ShowToast data class's message property is currently an Int but not annotated, so add the `@StringRes` annotation to the message parameter (data class ShowToast(val message: Int, ...)) to ensure IDE/lint enforce it is a string resource ID; import androidx.annotation.StringRes and update the constructor parameter to `@StringRes` message: Int so callers are validated at compile time.feature/task-certification/src/main/java/com/twix/task_certification/detail/component/TaskCertificationDetailTopBar.kt (3)
68-88: Preview 함수의 가시성을private으로 제한하는 것을 권장합니다.
TaskCertificationDetailTopBar자체는internal로 선언되어 있는데, 그 Preview 함수는 아무 수식어 없이public입니다. Preview 함수는 IDE 전용으로만 사용되므로private으로 제한하면 의도가 명확해지고 모듈 외부로 불필요하게 노출되는 것을 막을 수 있습니다.💡 Preview 가시성 제한 제안
-@Preview -@Composable -fun TaskCertificationDetailTopBarPreview() { +@Preview +@Composable +private fun TaskCertificationDetailTopBarPreview() {🤖 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/component/TaskCertificationDetailTopBar.kt` around lines 68 - 88, The Preview function TaskCertificationDetailTopBarPreview is currently public while the composable TaskCertificationDetailTopBar is internal; change the preview's declaration to private (i.e., private fun TaskCertificationDetailTopBarPreview()) so the preview is IDE-only and not exposed outside the module, keeping the existing `@Preview` and `@Composable` annotations and the inner usage of TwixTheme/Column unchanged.
17-17: 리소스 별칭(alias) 제거 — 미래 충돌 가능성 고려해보세요.
com.twix.designsystem.R을 별칭 없이R로 import하면, 이 파일이 feature 모듈 자체의 리소스(com.twix.task_certification.R)를 참조해야 할 때 동일한 이름으로 충돌이 생깁니다. 기존의DesR별칭은 바로 이 문제를 예방하기 위한 패턴이었습니다.현재는
ic_arrow3_left와word_modify모두 designsystem 모듈에 있으므로 동작에는 문제가 없지만, 이후 이 파일에 feature 모듈 고유 리소스가 추가될 경우 이름 충돌로 컴파일 오류가 발생할 수 있습니다.별칭을 유지하는 방식을 고려해보는 건 어떨까요?
💡 별칭 유지 방식 제안
-import com.twix.designsystem.R +import com.twix.designsystem.R as DesR그리고 사용 부분도 함께:
- painter = painterResource(R.drawable.ic_arrow3_left), + painter = painterResource(DesR.drawable.ic_arrow3_left),- text = stringResource(R.string.word_modify), + text = stringResource(DesR.string.word_modify),🤖 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/component/TaskCertificationDetailTopBar.kt` at line 17, Restore a namespaced alias for the designsystem resources to avoid future R name collisions: change the import in TaskCertificationDetailTopBar.kt from "import com.twix.designsystem.R" to the aliased form (e.g., "import com.twix.designsystem.R as DesR") and update any resource references that use the plain R (notably usages of ic_arrow3_left and word_modify) to use the alias (DesR) so they won't conflict with com.twix.task_certification.R.
43-53:noRippleClickable람다 전달 방식의 일관성을 맞춰주세요.Line 43에서는
noRippleClickable(onClick = onBack)처럼 함수 레퍼런스를 직접 전달하고 있지만, Line 53에서는noRippleClickable { onClickModify() }처럼 람다로 한 번 더 감싸고 있습니다. 불필요한 래핑은 미세하게나마 recomposition 비용을 높이고, 가독성도 떨어집니다.💡 일관된 스타일로 정리
- .noRippleClickable { onClickModify() }, + .noRippleClickable(onClick = onClickModify),🤖 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/component/TaskCertificationDetailTopBar.kt` around lines 43 - 53, 현재 TaskCertificationDetailTopBar의 클릭 핸들러 전달 방식이 일관되지 않습니다: noRippleClickable(onClick = onBack)는 함수 레퍼런스를 직접 전달하는 반면 noRippleClickable { onClickModify() }는 불필요한 래핑 람다를 사용합니다. noRippleClickable 호출들을 모두 동일하게 변경하여 레퍼런스를 직접 전달하도록 통일하세요 (예: replace the lambda form for the onClickModify usage with noRippleClickable(onClick = onClickModify)); 참조할 심볼: noRippleClickable, onBack, onClickModify, Modifier/Box where the modifier is applied.feature/task-certification/src/main/java/com/twix/task_certification/detail/component/swipe/SwipeableCard.kt (1)
34-39:@PreviewComposable 미제공
SwipeableCard는 공용 컴포넌트임에도@Preview가 없어 IDE에서 렌더링을 확인할 수 없습니다. 드래그/스와이프 동작을 인터랙티브하게 미리 보기 어렵더라도, 정적 상태(기본 위치)라도 미리보기를 추가하면 레이아웃 검증과 팀원 이해에 도움이 됩니다.💡 Preview 예시
`@Preview`(showBackground = true) `@Composable` private fun SwipeableCardPreview() { SwipeableCard(onSwipe = {}) { Box( modifier = Modifier .fillMaxWidth() .height(200.dp) .background(Color.LightGray), contentAlignment = Alignment.Center, ) { Text("Swipe Me") } } }As per coding guidelines: "Preview Composable이 제공되는가?" 항목 위반.
🤖 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/component/swipe/SwipeableCard.kt` around lines 34 - 39, Add a Preview Composable for the public SwipeableCard so IDEs can render its default/static state: create a private `@Preview` function (e.g., SwipeableCardPreview) that calls SwipeableCard(onSwipe = {}) with a simple static child (Box with fixed height, fillMaxWidth, background color and centered Text) and use `@Preview`(showBackground = true) to validate layout; ensure the preview is in the same Kotlin file as the SwipeableCard composable so it renders correctly in the IDE.feature/task-certification/src/main/java/com/twix/task_certification/certification/model/camera/CaptureCamera.kt (2)
134-134:companion object Companion— 기본 이름 명시는 불필요합니다.Kotlin에서 컴패니언 오브젝트의 기본 이름이
Companion이므로, 굳이 명시할 필요가 없습니다. 이름을 제거하면 코드가 더 간결해집니다.♻️ 불필요한 이름 제거
- companion object Companion { + companion object {🤖 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/certification/model/camera/CaptureCamera.kt` at line 134, The companion object is explicitly named "Companion" which is redundant; in CaptureCamera.kt remove the explicit name so the declaration uses an unnamed companion object (change "companion object Companion" to "companion object") to make the code more concise while keeping all existing members and behavior in the companion.
73-83:suspendCancellableCoroutine취소 처리가 누락되어 있습니다.코루틴이 취소될 경우(예: 사용자가 촬영 도중 화면을 이탈),
imageCapture.takePicture()로 시작된 CameraX 작업이 중단 없이 계속 실행됩니다.continuation.invokeOnCancellation {}을 등록하면 해당 상황에서 리소스를 정리할 수 있습니다. 현재 CameraX API에서는 진행 중인 캡처를 직접 취소하는 메서드가 없으므로, 최소한 카메라 바인딩 해제(unbind())나 로그 기록을 통해 의도를 명시하는 것이 좋습니다.♻️ 취소 처리 추가 예시
override suspend fun takePicture(): Result<Uri> = suspendCancellableCoroutine { continuation -> val contentValues = contentValues() val outputOptions = outputFileOptions(contentValues) imageCapture.takePicture( outputOptions, ContextCompat.getMainExecutor(context), capture(continuation), ) + + continuation.invokeOnCancellation { + // CameraX는 진행 중인 캡처를 직접 취소하는 API를 제공하지 않으나, + // 바인딩 해제 또는 로깅으로 취소 의도를 명시할 수 있습니다. + unbind() + } }🤖 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/certification/model/camera/CaptureCamera.kt` around lines 73 - 83, The suspendCancellableCoroutine in takePicture() is missing cancellation handling; register continuation.invokeOnCancellation { ... } so that when the coroutine is cancelled you stop or clean up the CameraX work started by imageCapture.takePicture(...). Inside the cancellation handler (referencing imageCapture, capture(continuation) and any available camera provider or binding object) call the appropriate unbind/unbindAll method (or at minimum log that the capture was cancelled) to release resources and avoid orphaned capture work; ensure the handler is added alongside the suspendCancellableCoroutine call that wraps imageCapture.takePicture().feature/task-certification/src/main/java/com/twix/task_certification/certification/model/camera/CameraPreview.kt (1)
6-9:@Immutable애너테이션이SurfaceRequest(가변 객체)를 감싸는 클래스에 사용되고 있습니다.
@Immutable은 Compose 컴파일러에게 해당 클래스와 그 모든 공개 프로퍼티가 깊이(deep) 불변임을 보장합니다. 그러나SurfaceRequest는provideSurface(),willNotProvideSurface()등을 가진 가변 CameraX 객체이므로 이 계약을 충족하지 못합니다.현재는
CaptureCamera에서 항상 새 인스턴스(CameraPreview(request))를 생성하므로 실제 동작에 문제가 없지만, Compose 컴파일러에게 잘못된 안정성 힌트를 주는 것이기 때문에 미래 코드 변경 시 예기치 않은 재구성 스킵이 발생할 수 있습니다.단순히 안정성 힌트를 주고 싶다면
@Stable이 더 적합하거나, 컴파일러의 자동 추론에 맡기는 것을 고려해 보세요.♻️ 개선 방법
-import androidx.compose.runtime.Immutable - -@Immutable data class CameraPreview( val request: SurfaceRequest, )또는 더 정확한 의미를 전달하려면:
-import androidx.compose.runtime.Immutable +import androidx.compose.runtime.Stable -@Immutable +@Stable data class CameraPreview( val request: SurfaceRequest, )🤖 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/certification/model/camera/CameraPreview.kt` around lines 6 - 9, The class CameraPreview is annotated with `@Immutable` while it wraps CameraX's mutable SurfaceRequest (which has provideSurface()/willNotProvideSurface()), violating `@Immutable`'s deep immutability contract; update the annotation to `@Stable` or remove the `@Immutable` on CameraPreview and rely on Compose's inference (adjust usage in CaptureCamera which constructs new CameraPreview(request) as needed) so the compiler isn't given a false immutability guarantee.feature/task-certification/src/main/java/com/twix/task_certification/editor/component/RetakeButton.kt (1)
36-41: Preview에 다크 모드 변형을 추가하면 더 좋습니다현재
@Preview만 있어 라이트 모드만 확인할 수 있습니다. 디자인 시스템 가이드라인의 "Dark Mode 지원 여부"에 따라@PreviewLightDark로 교체하면 두 가지 테마를 한 번에 검증할 수 있습니다.🌙 수정 제안: `@PreviewLightDark` 적용
-import androidx.compose.ui.tooling.preview.Preview +import androidx.compose.ui.tooling.preview.PreviewLightDark -@Preview +@PreviewLightDark `@Composable` private fun RetakeButtonPreview() { TwixTheme { RetakeButton(onClickRetake = {}) } }🤖 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/editor/component/RetakeButton.kt` around lines 36 - 41, 현재 RetakeButtonPreview 데코레이터가 `@Preview로` 되어 있어 다크 모드 확인이 불가능합니다; RetakeButtonPreview(프리뷰 함수)를 찾아 `@Preview` 대신 디자인 시스템에서 사용하는 `@PreviewLightDark로` 교체해 라이트/다크 테마를 동시에 렌더링하도록 변경하고, 필요 시 TwixTheme 래핑이나 매개변수(예: uiMode) 설정이 `@PreviewLightDark와` 충돌하지 않는지 확인하세요.feature/task-certification/src/main/java/com/twix/task_certification/certification/component/TaskCertificationTopBar.kt (1)
25-41: 높이 값 하드코딩에 대해 – 선택적 개선 제안
72.dp가 직접 하드코딩되어 있는데, 디자인 시스템 차원에서 TopBar 높이를 상수로 관리하면 일관성 유지에 도움이 될 수 있습니다. 다른 TopBar 컴포넌트에서도 동일한 높이를 사용한다면, 공통 상수로 추출하는 것을 고려해 보시는 건 어떨까요?현재 구현 자체에는 문제가 없으며, Preview도 잘 제공되어 있습니다.
🤖 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/certification/component/TaskCertificationTopBar.kt` around lines 25 - 41, The Box in TaskCertificationTopBar uses a hardcoded height of 72.dp; extract this magic value into a shared constant (e.g., TOP_BAR_HEIGHT) in the component or a UI constants file and replace the literal in the height(...) call so other TopBar implementations can reuse it (update any imports/usages and keep the modifier chain and behavior the same).feature/task-certification/src/main/java/com/twix/task_certification/certification/contract/TaskCertificationUiState.kt (1)
41-41:TorchStatus.Companion.toggle(...)대신TorchStatus.toggle(...)이 더 관용적입니다Kotlin에서 companion object 함수를 호출할 때
.Companion.을 명시적으로 붙일 필요가 없습니다. 이 변경은 기존의 더 관용적인 호출 방식에서 퇴보한 것으로 보입니다.♻️ 개선 제안
- val newFlashMode = TorchStatus.Companion.toggle(torch) + val newFlashMode = TorchStatus.toggle(torch)🤖 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/certification/contract/TaskCertificationUiState.kt` at line 41, The call uses the verbose form TorchStatus.Companion.toggle(torch); replace it with the idiomatic Kotlin call TorchStatus.toggle(torch) by removing ".Companion." wherever TorchStatus.Companion.toggle(...) appears (e.g., in TaskCertificationUiState where newFlashMode is assigned) so the companion function is invoked in the conventional, concise way.core/design-system/src/main/res/values/strings.xml (1)
100-100: XML 주석 닫기 태그 오타:!-->→-->Line 100과 Line 109의 주석이
!-->로 끝나고 있습니다. 기술적으로 파싱에는 문제없지만!는 의도하지 않은 문자로 보이며, 통일성을 위해 수정하는 것이 좋습니다.♻️ 개선 제안
- <!-- 인증샷 촬영 화면 !--> + <!-- 인증샷 촬영 화면 --> ... - <!-- 인증샷 보기 화면 !--> + <!-- 인증샷 보기 화면 -->Also applies to: 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/res/values/strings.xml` at line 100, Fix the malformed XML comment closers in strings.xml where the comments for the 인증샷 촬영 화면 (and the other comment ending at the same spot) use "!-->" instead of the correct "-->"; locate the comment nodes containing "인증샷 촬영 화면" (and the one at the other occurrence) and replace the closing token "!-->" with the valid XML comment terminator "-->" so comments are syntactically consistent.core/network/src/main/java/com/twix/network/model/request/photolog/model/PhotologModifyRequest.kt (2)
1-1: 패키지 경로에model이 중복됩니다
com.twix.network.model.request.photolog.model은model이 외부(network.model)와 내부(photolog.model) 두 곳에 등장합니다.com.twix.network.model.request.photolog로 정리하는 것이 더 간결하고 일관성 있는 경로입니다.♻️ 패키지 경로 개선 제안
-package com.twix.network.model.request.photolog.model +package com.twix.network.model.request.photolog🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/network/src/main/java/com/twix/network/model/request/photolog/model/PhotologModifyRequest.kt` at line 1, The package declaration in PhotologModifyRequest.kt redundantly contains "model" twice; change the package from com.twix.network.model.request.photolog.model to com.twix.network.model.request.photolog and update the file's package line accordingly, then refactor any imports/usages referencing com.twix.network.model.request.photolog.model to the new package (search for PhotologModifyRequest and adjust references) so all declarations and consumers compile with the consolidated package name.
9-9:@SerialName("comment")는 필드명과 동일하므로 생략 가능합니다kotlinx.serialization에서
@SerialName의 값이 필드명과 동일할 때는 어노테이션이 없어도 동작이 같습니다. 노이즈를 줄이기 위해 생략을 고려해보세요.♻️ 불필요한 어노테이션 제거 제안
`@Serializable` data class PhotologModifyRequest( `@SerialName`("fileName") val fileName: String, - `@SerialName`("comment") val comment: String, + val comment: String, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/network/src/main/java/com/twix/network/model/request/photolog/model/PhotologModifyRequest.kt` at line 9, The `@SerialName`("comment") annotation on the PhotologModifyRequest property is redundant because the serialized name matches the Kotlin property name; remove the annotation on the property declaration (the `@SerialName("comment") val comment: String` line) so the serializer uses the default name and reduces noise while leaving the property and class unchanged.core/navigation/src/main/java/com/twix/navigation/savedstate/SafeDecode.kt (2)
9-22:null체크를try블록 밖으로 분리하면 로그 가독성이 향상됩니다현재
error("Missing nav arg: $key")가try블록 내부에 있어서,raw가null인 경우에도catch가 이를 잡아 "decode failed" 로 기록합니다. "누락된 인자" 오류와 "JSON 디코딩 실패" 오류가 같은 메시지로 보이면 디버깅 시 혼란을 줄 수 있습니다.♻️ null 체크 분리 제안
inline fun <reified T> SavedStateHandle.decodeNavArgs(key: String): T { - val raw = get<String>(key) - + val raw = get<String>(key) ?: error("Missing nav arg: $key") try { - val decoded = - raw?.let(Uri::decode) - ?: error("Missing nav arg: $key") - + val decoded = Uri.decode(raw) Logger.d { "NavArgs[$key] decoded=$decoded" } - return Json.decodeFromString(decoded) } catch (e: Exception) { Logger.e(e) { "NavArgs[$key] decode failed raw=$raw" } throw e } }이렇게 하면
try블록은 실제 URI 디코딩·JSON 역직렬화 실패만 처리하고,null누락 케이스는 별도로 즉시 크래시를 냅니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/navigation/src/main/java/com/twix/navigation/savedstate/SafeDecode.kt` around lines 9 - 22, The null-check for the nav argument should be moved out of the try block so missing-arg errors are logged/raised separately from decode/deserialization failures: after retrieving raw via get<String>(key) check raw == null and call error("Missing nav arg: $key") (so the missing-arg case never enters the try), then wrap only the Uri::decode and Json.decodeFromString calls (using Uri::decode, Logger.d, Json.decodeFromString) in the try/catch to ensure Logger.e in the catch only reports true decode/deserialization errors for raw values that existed.
18-18: 기본Json인스턴스는ignoreUnknownKeys = false로 설정되어 있습니다현재 코드는
Json.decodeFromString(decoded)로 기본 Json 인스턴스를 사용 중입니다. 이 경우 JSON에 역직렬화 대상 클래스에 없는 필드가 있으면SerializationException이 발생합니다.프로젝트에서는 이미 여러 곳에서
ignoreUnknownKeys = true로 설정된 공통 Json 인스턴스를 사용하고 있습니다:
HttpClientProvider.kt(line 44)AuthConfigure.kt(line 20)ApiCall.kt(line 15)이 패턴을 따라 SafeDecode에서도 프로젝트의 기본 Json 설정을 사용하거나, 필요시 설정된 Json 인스턴스를 주입받도록 개선하면 어떨까요? 이렇게 하면 앱 내 데이터 처리의 일관성을 유지할 수 있고, 향후 스키마 변경에도 더 안전해집니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/navigation/src/main/java/com/twix/navigation/savedstate/SafeDecode.kt` at line 18, The current SafeDecode.kt uses the default Json.decodeFromString(decoded) which throws on unknown fields; update SafeDecode to use the project's shared Json configuration that has ignoreUnknownKeys = true (or accept a Json instance via parameter/injection) instead of the default Json object so deserialization is tolerant of extra fields; change the call site in the SafeDecode function to use the injected/shared Json instance (refer to the SafeDecode function and the return line using Json.decodeFromString) so it aligns with the Json instances used in HttpClientProvider, AuthConfigure, and ApiCall.core/design-system/src/main/java/com/twix/designsystem/components/photolog/CertificatedCard.kt (2)
37-37:comment?.isNotEmpty() == true→!comment.isNullOrEmpty()로 간결하게 표현할 수 있습니다.두 표현 모두 동일하게 동작하지만, Kotlin 표준 라이브러리의
isNullOrEmpty()확장 함수를 사용하면 의도가 더 명확하고 관용적입니다.♻️ 개선 제안
- if (comment?.isNotEmpty() == true) { + if (!comment.isNullOrEmpty()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/photolog/CertificatedCard.kt` at line 37, Replace the verbose null-check in CertificatedCard (the conditional using comment?.isNotEmpty() == true) with the idiomatic Kotlin check using comment.isNullOrEmpty() (negated) — i.e., change the condition to use !comment.isNullOrEmpty() so the intent is clearer and more concise wherever that expression appears in the CertificatedCard composable/logic.
26-36: 인증샷 이미지에contentDescription을 제공하는 것이 좋습니다.인증샷 이미지는 카드의 핵심 콘텐츠입니다. 스크린 리더 사용자를 위해 적절한
contentDescription을 추가하면 접근성이 향상됩니다. 디자인 시스템 리뷰 가이드에서 접근성(Accessibility) 고려를 권장하고 있습니다. As per coding guidelines,core/design-system/**파일은 접근성(Accessibility) 고려 여부를 검토해야 합니다.♻️ 개선 제안
AsyncImage( model = ImageRequest .Builder(LocalContext.current) .data(imageUrl) .crossfade(true) .build(), - contentDescription = null, + contentDescription = "인증샷 이미지", contentScale = ContentScale.Crop, modifier = Modifier.fillMaxSize(), )또는 외부에서
contentDescription파라미터를 전달받도록 하면 더 유연합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/photolog/CertificatedCard.kt` around lines 26 - 36, The AsyncImage call inside CertificatedCard is missing a contentDescription which harms accessibility; update the CertificatedCard composable to accept a contentDescription: String? parameter (or a descriptive default) and pass that value into AsyncImage's contentDescription argument, ensuring the composable signature (CertificatedCard(...)) and the AsyncImage call are updated accordingly so callers can provide a meaningful description for the certification image.core/design-system/src/main/java/com/twix/designsystem/components/photolog/ForegroundCard.kt (1)
34-38:stringResource의 포맷 인자 전달 방식을 개선할 수 있습니다.현재
stringResource(...).format(nickName)패턴을 사용하고 있는데, Compose에서는stringResource(resId, formatArgs)를 직접 사용하는 것이 더 관용적(idiomatic)입니다.stringResource오버로드가 내부적으로 포맷팅을 처리하므로 불필요한 중간 String 생성을 줄이고, 의도가 더 명확해집니다.♻️ 개선 제안
BetweenUs.PARTNER -> - stringResource(R.string.partner_not_task_certification).format( - nickName, - ) + stringResource(R.string.partner_not_task_certification, nickName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/photolog/ForegroundCard.kt` around lines 34 - 38, The string formatting for the BetweenUs.PARTNER branch in ForegroundCard.kt uses stringResource(...).format(nickName); change this to use the stringResource overload that accepts format args directly (stringResource(R.string.partner_not_task_certification, nickName)) so Compose performs the formatting without creating an intermediate String; locate the mapping for BetweenUs.PARTNER and replace the current .format(...) usage with passing nickName as the second argument to stringResource.core/design-system/src/main/java/com/twix/designsystem/components/photolog/BackgroundCard.kt (1)
70-77: 접근성(Accessibility) 검토:contentDescription = null이 적절한지 확인해 주세요.
ic_keepi_sting이미지가 순수 장식용(decorative)이라면contentDescription = null은 올바릅니다. 다만 이 이미지가 사용자에게 의미 있는 정보를 전달한다면 적절한 설명을 추가하는 것이 좋습니다. 디자인 시스템 가이드에서 접근성 고려를 권장하고 있으므로 확인 부탁드립니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/photolog/BackgroundCard.kt` around lines 70 - 77, The Image in BackgroundCard.kt uses imageVector = R.drawable.ic_keepi_sting with contentDescription = null which may hide meaningful info from assistive tech; verify whether ic_keepi_sting is purely decorative—if it conveys information, replace the null with a localized description via a string resource (e.g., provide a descriptive string from resources) and set contentDescription accordingly on the Image composable; if it is purely decorative, add a comment explaining why contentDescription is null to make the intent explicit.feature/task-certification/src/main/java/com/twix/task_certification/detail/component/reaction/ReactionBar.kt (1)
81-86: 매직 넘버4대신 동적으로 마지막 인덱스를 계산하는 것이 안전합니다.현재
index < 4는ReactionUiModel.entries가 정확히 5개라고 가정합니다. 향후 리액션 타입이 추가되거나 제거되면 구분선(divider) 렌더링이 깨질 수 있습니다. 방어적으로entries.lastIndex를 활용하면 항목 수 변경에도 안전합니다.♻️ 개선 제안
- if (index < 4) { + if (index < ReactionUiModel.entries.lastIndex) {🤖 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/component/reaction/ReactionBar.kt` around lines 81 - 86, The divider rendering uses a hard-coded magic number ("index < 4") which assumes ReactionUiModel.entries has exactly five items; change the condition to compare against the actual last index (use ReactionUiModel.entries.lastIndex or entries.size - 1) so that in the ReactionBar (where the loop uses index) you only render the VerticalDivider when index < entries.lastIndex, making divider rendering resilient to added/removed reaction entries.core/navigation/src/main/java/com/twix/navigation/args/TaskCertificationNavArgs.kt (1)
17-24:DetailNavArgs의photologId기본값-1은 매직 넘버입니다.
-1이라는 값이 "미설정"을 의미하는 것으로 보이는데, 이 의도가 코드에서 명시적으로 드러나지 않습니다. 이후 이 값을 사용하는 쪽에서-1인지 비교하는 로직이 흩어질 수 있어 유지보수에 불리합니다.두 가지 방법을 제안합니다:
photologId를Long?(nullable)로 변경하여 "값이 없음"을 타입 시스템으로 표현- 혹은 companion object에 상수를 정의하여 의미를 명시
♻️ 제안 (옵션 2 — 상수 정의)
`@Serializable` data class DetailNavArgs( val goalId: Long, val from: NavRoutes.TaskCertificationRoute.From, - val photologId: Long = -1, + val photologId: Long = UNSET_PHOTOLOG_ID, val selectedDate: String = "", val comment: String = "", -) +) { + companion object { + const val UNSET_PHOTOLOG_ID = -1L + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/navigation/src/main/java/com/twix/navigation/args/TaskCertificationNavArgs.kt` around lines 17 - 24, The current DetailNavArgs data class uses a magic-number default of -1 for photologId; change photologId to a nullable Long (photologId: Long? = null) in DetailNavArgs so "unset" is represented by the type system, then update all call sites and consumers that access DetailNavArgs.photologId (e.g., any code referencing NavRoutes.TaskCertificationRoute.From and places that compare photologId to -1) to handle null instead of comparing to -1 (use safe calls, elvis, or explicit null checks) and ensure serialization/deserialization logic accepts a nullable photologId.feature/main/src/main/java/com/twix/main/navigation/MainNavGraph.kt (1)
53-63:navigateToCertificationDetail도 동일한 패턴으로 리팩터링 하는 것을 고려해 볼 수 있습니다.현재
navigateToCertificationDetail은 여전히goalId,date,betweenUs.name을 개별 인자로 전달하고 있습니다.navigateToCertification이DetailNavArgs로 통합된 것처럼, 이쪽도 유사한 NavArgs 패턴을 적용하면 네비게이션 인자 전달 방식이 일관적으로 유지됩니다. 물론 현재 라우트(TaskCertificationDetailRoute)가 다른 인자 구조를 사용하고 있다면 별도 작업이 필요할 수 있으니, 추후 리팩터링 시 고려해 주시면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/main/src/main/java/com/twix/main/navigation/MainNavGraph.kt` around lines 53 - 63, 현재 navigateToCertificationDetail 콜이 goalId, date, betweenUs.name을 개별 인자로 전달해 네비게이션 인자 방식이 일관되지 않습니다; navigateToCertification에서 사용한 DetailNavArgs 패턴을 동일하게 적용하도록 수정하세요: TaskCertificationDetailRoute.createRoute의 시그니처를 DetailNavArgs(또는 적절한 CertificationDetailNavArgs) 객체를 받도록 변경하거나 해당 객체를 새로 생성한 뒤 navigateToCertificationDetail 내부에서 그 객체를 생성해 전달하고 navController.navigate(destination) 호출은 기존처럼 유지하지만 인자 묶음을 사용하도록 업데이트하세요 (참조 심볼: navigateToCertificationDetail, navigateToCertification, DetailNavArgs, TaskCertificationDetailRoute.createRoute, navController.navigate).core/design-system/src/main/java/com/twix/designsystem/components/comment/model/CommentUiModel.kt (1)
13-17:canUpload의value.isNotEmpty()조건이 불필요합니다.
hasMaxCommentLength가true이면(value.length == 5)value.isNotEmpty()는 항상true이므로, 해당 조건은 논리적으로 중복됩니다. 제거하면 가독성이 향상됩니다.♻️ 간소화 제안
val canUpload: Boolean get() = - value.isEmpty() || - value.isNotEmpty() && - hasMaxCommentLength + value.isEmpty() || hasMaxCommentLength🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/comment/model/CommentUiModel.kt` around lines 13 - 17, The canUpload getter contains a redundant value.isNotEmpty() check; simplify its logic by replacing the current expression with a direct boolean combining value.isEmpty() and hasMaxCommentLength (i.e., return value.isEmpty() || hasMaxCommentLength) so the getter for canUpload (in CommentUiModel) is clearer and equivalent.feature/task-certification/src/main/java/com/twix/task_certification/detail/contract/TaskCertificationDetailUiState.kt (1)
77-85:toSerializer()메서드 명이 실제 역할과 다소 괴리가 있어 보입니다.이 메서드는 UI 상태를
EditorNavArgs로 변환하는 역할을 합니다.toSerializer라는 이름은 직렬화 로직 자체를 반환하는 것처럼 읽힐 수 있어,toEditorNavArgs()와 같이 반환 타입을 반영한 이름이 의도를 더 명확히 전달할 수 있습니다.또한
myPhotolog가null일 때photologId = -1,imageUrl = ""로 폴백되는데,canModify가true일 때만 호출된다면 실질적인 문제는 없지만, 방어적으로 호출 가능 조건을 문서화하거나require(myPhotolog != null)같은 사전 조건을 추가하는 것도 고려해볼 수 있습니다.🤖 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/contract/TaskCertificationDetailUiState.kt` around lines 77 - 85, Rename the misleading toSerializer() to toEditorNavArgs() to reflect that it converts the UI state into an EditorNavArgs instance; update all call sites to use toEditorNavArgs(). Additionally make the null-contract explicit by adding a precondition check such as require(myPhotolog != null) (or document that this is only called when canModify == true) before building EditorNavArgs so photologId/imageUrl aren’t silently defaulted, and reference the EditorNavArgs, toSerializer (old name), toEditorNavArgs (new name), myPhotolog, and canModify symbols when making the change.core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentAnchorFrame.kt (2)
40-104: 키보드 대응 로직이 잘 분리되어 재사용 가능한 컴포넌트로 구현되었습니다.
WindowInsets.ime를 활용한 키보드 감지,AnimatedVisibility를 통한 딤 처리, 동적 offset 계산 등 복잡한 로직이 하나의 컴포저블로 깔끔하게 캡슐화되어 있습니다. 기존TaskCertificationScreen에서 인라인으로 존재하던 로직을 design-system으로 분리한 것은 좋은 리팩터링입니다.한 가지 제안: 디자인 시스템 모듈의 컴포넌트로서
@Preview함수를 제공하면 UI 확인과 유지보수에 도움이 됩니다. As per coding guidelines, "Preview Composable이 제공되는가?"♻️ Preview 추가 제안
`@Preview` `@Composable` private fun CommentAnchorFramePreview() { CommentAnchorFrame( uiModel = CommentUiModel(), anchorBottom = 800f, onCommentChanged = {}, onFocusChanged = {}, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentAnchorFrame.kt` around lines 40 - 104, Add a Preview composable for the CommentAnchorFrame so designers/devs can quickly inspect it in Android Studio; create a private `@Preview` `@Composable` function (e.g., CommentAnchorFramePreview) that calls CommentAnchorFrame with a default CommentUiModel instance, a sensible anchorBottom (e.g., 800f), and no-op lambdas for onCommentChanged and onFocusChanged, and mark it private so it lives alongside the existing CommentAnchorFrame composable for design-system previews.
48-48:anchorBottom == 0f비교는 "아직 측정되지 않은 상태"의 센티넬 값으로 사용되고 있는 것으로 보입니다.Float의
==비교는 일반적으로 부동소수점 정밀도 문제로 권장되지 않지만, 여기서는mutableFloatStateOf(0f)의 초기값과의 정확한 비교이므로 의도대로 동작합니다. 다만, 의도를 더 명확히 하기 위해 nullable(Float?)로 변경하고null체크를 하는 방안도 고려해볼 수 있습니다. 현재 구현도 문제없으므로 참고 수준입니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentAnchorFrame.kt` at line 48, The sentinel check using exact float equality on anchorBottom (if (anchorBottom == 0f) return) should be made explicit by turning anchorBottom into a nullable Float sentinel: change the state backing anchorBottom (where mutableFloatStateOf(0f) is used) to a nullable state (e.g., mutableStateOf<Float?>(null)), update any assignments to set the measured float value, and replace the equality check with a null check (if (anchorBottom == null) return) so intent is clearer and avoids ambiguous float-equality semantics; update usages of anchorBottom to handle the non-null Float after the null check.feature/task-certification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorViewModel.kt (1)
80-85:currentState에서 직접 값을 읽어 API를 호출하는 부분에 대해 한 가지 고려 사항이 있습니다.
launchModifyComment()가 호출되는 시점과 실제 실행 시점 사이에currentState가 변경될 가능성은 낮지만,launchResult의block람다가 비동기로 실행되므로 호출 시점의 상태 값을 캡처해두는 것이 방어적으로 더 안전할 수 있습니다.현재 구조에서 실질적인 문제가 발생할 가능성은 낮으므 참고 수준의 제안입니다.
♻️ 방어적 코딩 제안
private fun modifyComment() { + val state = currentState if (currentState.comment.canUpload.not()) { showToast(R.string.comment_error_message, ToastType.ERROR) } else if (currentState.isCommentNotChanged) { showToast(R.string.task_certification_editor_not_modified, ToastType.ERROR) } else { launchResult( - block = { launchModifyComment() }, + block = { + photologRepository.modifyPhotolog( + state.photologId, + state.imageName, + state.comment.value, + ) + },🤖 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/editor/TaskCertificationEditorViewModel.kt` around lines 80 - 85, The code reads currentState properties directly inside launchModifyComment which risks using mutated state when the async block runs; update launchModifyComment to first capture needed values into local vals (e.g., val photologId = currentState.photologId, val imageName = currentState.imageName, val comment = currentState.comment.value) and then call photologRepository.modifyPhotolog(photologId, imageName, comment) so the repository call uses the snapshot of state captured at call time (this also covers cases where this method is invoked from within a launchResult block).core/network/src/main/java/com/twix/network/service/PhotoLogService.kt (1)
27-30:@Query파라미터로LocalDate대신String을 사용하여 코드베이스 일관성을 맞추세요.
GoalService에서는 날짜 쿼리 파라미터를String으로 정의하고 있으나,PhotoLogService에서만LocalDate를 직접 사용하고 있습니다.LocalDate.toString()이"yyyy-MM-dd"형식을 반환하므로 동작하겠지만, 코드베이스 전체의 패턴과 일치시키는 것이 유지보수성을 높입니다.또한 파라미터 이름을
@Query어노테이션과 일치시켜야 합니다. 현재@Query("targetDate") request: LocalDate로 되어 있으나, 파라미터명은targetDate로 변경하는 것이 관례를 따릅니다.`@GET`("api/v1/photologs") suspend fun fetchPhotoLogs( `@Query`("targetDate") targetDate: String, ): PhotoLogsResponse호출 시
LocalDate를 전달해야 한다면, 인자 변환은 호출부에서.toString()으로 처리하는 것이 명시적입니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/network/src/main/java/com/twix/network/service/PhotoLogService.kt` around lines 27 - 30, Change the fetchPhotoLogs signature in PhotoLogService so the `@Query` parameter uses a String and the parameter name matches the query key: replace request: LocalDate with targetDate: String in the suspend fun fetchPhotoLogs method; this aligns with GoalService's pattern and keeps date-to-string conversion explicit (callers should pass LocalDate.toString() when needed).core/navigation/src/main/java/com/twix/navigation/NavRoutes.kt (1)
38-49:TaskCertificationDetailRoute만 기존 다중 경로 파라미터 방식을 유지하고 있습니다.
TaskCertificationRoute와TaskCertificationEditorRoute는{data}기반 JSON 인코딩 방식으로 전환되었는데,TaskCertificationDetailRoute는 여전히{goalId}/{date}/{betweenUs}방식을 사용하고 있습니다. 현재 기능상 문제는 없지만, 일관성을 위해 향후 마이그레이션을 고려해볼 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/navigation/src/main/java/com/twix/navigation/NavRoutes.kt` around lines 38 - 49, TaskCertificationDetailRoute currently uses multiple path params ("task_certification_detail/{goalId}/{date}/{betweenUs}") while TaskCertificationRoute and TaskCertificationEditorRoute use a single "{data}" JSON-encoded param; to align them, refactor TaskCertificationDetailRoute (object TaskCertificationDetailRoute and its createRoute) to accept a single JSON-encoded data string param (e.g., "{data}") and update createRoute to serialize the goalId, date, and betweenUs into that JSON string, ensuring ARG constants reflect the new "data" arg name and parsing/decoding is implemented where this route is consumed.feature/task-certification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorRoute.kt (1)
125-133:TaskCertificationEditorScreen의 가시성(visibility)을internal또는private으로 제한하는 것을 권장합니다.이 Screen 컴포저블은 모듈 외부에서 직접 사용될 필요가 없고,
TaskCertificationEditorRoute를 통해서만 접근하면 됩니다.internal가시성으로 제한하면 API surface를 줄이고 모듈 경계를 명확히 할 수 있습니다.🤖 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/editor/TaskCertificationEditorRoute.kt` around lines 125 - 133, Limit the visibility of the TaskCertificationEditorScreen composable to the module by changing its declaration from public to internal (or private if only used within the same file); update the function signature for TaskCertificationEditorScreen accordingly and ensure callers use TaskCertificationEditorRoute to access the screen so the composable is no longer exposed from the module API.core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentTextField.kt (1)
100-105: 접근성(Accessibility) 관련 고려가 필요합니다.숨겨진
TextField(width=0, alpha=0)와 커스텀 원형 UI로 구성되어 있어, 스크린 리더 사용자가 이 컴포넌트를 인식하고 상호작용하기 어려울 수 있습니다.Box에semanticsmodifier를 추가하여 접근성 정보를 제공하는 것을 권장합니다.As per coding guidelines: "접근성(Accessibility) 고려 여부"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/design-system/src/main/java/com/twix/designsystem/components/comment/CommentTextField.kt` around lines 100 - 105, The Box using modifier.noRippleClickable (around the custom circular UI that focuses the hidden TextField via focusRequester) lacks accessibility semantics; update the Box to include a semantics modifier that exposes an accessible label/role and clickable state (e.g., contentDescription or text, role = Button or TextField as appropriate, and stateDescription/disabled if needed) and connect it to the hidden TextField (use onClick semantics to request focus) so screen readers can discover and interact with the control; locate the Box declaration and the focusRequester usage in CommentTextField.kt and add the appropriate semantics modifiers to mirror the visible UI behavior.
core/navigation/src/main/java/com/twix/navigation/savedstate/SafeDecode.kt
Show resolved
Hide resolved
...cation/src/main/java/com/twix/task_certification/certification/component/CameraControlBar.kt
Show resolved
Hide resolved
...cation/src/main/java/com/twix/task_certification/certification/TaskCertificationViewModel.kt
Show resolved
Hide resolved
...cation/src/main/java/com/twix/task_certification/certification/TaskCertificationViewModel.kt
Show resolved
Hide resolved
...ask-certification/src/main/java/com/twix/task_certification/editor/component/RetakeButton.kt
Show resolved
Hide resolved
.../src/main/java/com/twix/task_certification/editor/component/TaskCertificationEditorTopBar.kt
Show resolved
Hide resolved
.../src/main/java/com/twix/task_certification/editor/contract/TaskCertificationEditorUiState.kt
Show resolved
Hide resolved
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/task-certification/src/main/java/com/twix/task_certification/detail/contract/TaskCertificationDetailUiState.kt (1)
92-112:⚠️ Potential issue | 🟡 Minor
BetweenUs.valueOf(betweenUs)에서 유효하지 않은 문자열이 전달되면IllegalArgumentException이 발생합니다.Line 104의
BetweenUs.valueOf(betweenUs)는 네비게이션 인자로 전달받은 문자열을 enum으로 변환하는데, 값이 유효하지 않으면 런타임 크래시로 이어집니다. 네비게이션 인자는 외부 입력이므로 방어 로직이 있으면 안정성이 높아집니다.
runCatching이나BetweenUs.entries.find { it.name == betweenUs }패턴으로 안전하게 파싱하는 것을 고려해 보시면 좋겠습니다.🛡️ 안전한 enum 변환 예시
return TaskCertificationDetailUiState( goalId = goalId, - currentShow = BetweenUs.valueOf(betweenUs), + currentShow = BetweenUs.entries.firstOrNull { it.name == betweenUs } ?: BetweenUs.PARTNER, selectedDate = selectedDate,🤖 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/contract/TaskCertificationDetailUiState.kt` around lines 92 - 112, The current PhotoLogs.toUiState function calls BetweenUs.valueOf(betweenUs) which will throw IllegalArgumentException for invalid nav input; replace that unsafe call with a safe parse (e.g., BetweenUs.entries.find { it.name == betweenUs } or runCatching { BetweenUs.valueOf(betweenUs) }.getOrNull()) and supply a sensible fallback (use an appropriate default enum constant from BetweenUs) before assigning to currentShow so malformed navigation strings no longer crash the app.
🧹 Nitpick comments (3)
feature/task-certification/src/main/java/com/twix/task_certification/detail/component/TaskCertificationCardContent.kt (1)
27-32:buttonTitle결정 로직이 다른 부분과 일관성이 없습니다.Line 31-32에서는
uiState.isDisplayedMyPhotolog를 활용하여 ME/PARTNER 분기를 깔끔하게 처리하고 있는데, Line 27-30의buttonTitle은 여전히uiState.currentShow == BetweenUs.ME를 직접 비교하고 있습니다.동일한 의미의 분기를 한 곳은 computed property로, 다른 곳은 raw enum 비교로 처리하면 추후 조건이 변경될 때 누락 가능성이 있습니다.
isDisplayedMyPhotolog로 통일하면 일관성이 높아질 것 같은데, 의도적으로 분리하신 건지 확인 부탁드립니다.♻️ 일관성 개선 제안
buttonTitle = - when (uiState.currentShow) { - BetweenUs.ME -> stringResource(R.string.task_certification_take_picture) - BetweenUs.PARTNER -> stringResource(R.string.task_certification_detail_partner_sting) - }, + if (uiState.isDisplayedMyPhotolog) { + stringResource(R.string.task_certification_take_picture) + } else { + stringResource(R.string.task_certification_detail_partner_sting) + },🤖 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/component/TaskCertificationCardContent.kt` around lines 27 - 32, The buttonTitle branch uses uiState.currentShow == BetweenUs.ME while rotation and onClick use uiState.isDisplayedMyPhotolog, causing inconsistency; update the buttonTitle logic in TaskCertificationCardContent (the code that sets buttonTitle) to use uiState.isDisplayedMyPhotolog for its ME/PARTNER decision (mirroring rotation and onClick), keeping the same stringResource choices and preserving handlers onClickUpload and onClickSting so all three decisions (buttonTitle, rotation, onClick) derive from the same uiState.isDisplayedMyPhotolog condition.feature/task-certification/src/main/java/com/twix/task_certification/detail/contract/TaskCertificationDetailUiState.kt (2)
81-89:toSerializer()호출 시myPhotolog가 null인 경우의 방어 로직을 확인해 주세요.Line 86에서
myPhotolog?.photologId ?: -1로 sentinel value를 사용하고 있습니다.toSerializer()가canModify == true(즉,myPhotolog != null) 상태에서만 호출된다면 안전하지만, 만약 null 상태에서 호출되면-1이 Editor 화면으로 전달됩니다.방어적으로 호출부에서
canModify체크가 보장되는지 확인하시거나, 혹은toSerializer()의 반환 타입을EditorNavArgs?로 변경하여 null-safety를 컴파일 타임에 보장하는 방법도 고려해 볼 수 있습니다.💡 Nullable 반환 타입 대안
- fun toSerializer() = - EditorNavArgs( + fun toSerializer(): EditorNavArgs? { + val photolog = myPhotolog ?: return null + return EditorNavArgs( goalId = goalId, nickname = myNickname, goalName = goalName, - photologId = myPhotolog?.photologId ?: -1, - imageUrl = myPhotolog?.imageUrl ?: "", - comment = myPhotolog?.comment, + photologId = photolog.photologId, + imageUrl = photolog.imageUrl, + comment = photolog.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/contract/TaskCertificationDetailUiState.kt` around lines 81 - 89, toSerializer() currently uses sentinel values when myPhotolog is null which can leak invalid data to the Editor; change the signature of toSerializer() to return EditorNavArgs? (nullable) and inside return null when myPhotolog == null (or alternatively require(canModify) at the top if you prefer a hard failure), and then update all callers to handle the nullable return (or ensure canModify is checked before calling). Reference symbols: toSerializer(), myPhotolog, EditorNavArgs, canModify.
69-79:canModify와canReaction에서도isDisplayedMyPhotolog를 활용하면 일관성이 높아집니다.
isDisplayedMyPhotolog를 새로 도입하셨는데, 바로 아래의canModify(Line 74)와canReaction(Line 78)에서는 여전히currentShow == BetweenUs.ME/BetweenUs.PARTNER를 직접 비교하고 있습니다. 동일한 의미의 조건이므로 통일하면 추후BetweenUs분기 조건이 변경될 때 한 곳만 수정하면 되어 유지보수에 유리합니다.♻️ 일관성 개선 제안
val canModify: Boolean get() = - currentShow == BetweenUs.ME && isDisplayedGoalCertificated + isDisplayedMyPhotolog && isDisplayedGoalCertificated val canReaction: Boolean get() = - currentShow == BetweenUs.PARTNER && isDisplayedGoalCertificated + !isDisplayedMyPhotolog && isDisplayedGoalCertificated🤖 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/contract/TaskCertificationDetailUiState.kt` around lines 69 - 79, Refactor the boolean checks to use shared helpers: add a new computed property isDisplayedPartnerPhotolog (val isDisplayedPartnerPhotolog: Boolean get() = currentShow == BetweenUs.PARTNER) and change canModify to use isDisplayedMyPhotolog && isDisplayedGoalCertificated and canReaction to use isDisplayedPartnerPhotolog && isDisplayedGoalCertificated (replace direct currentShow == BetweenUs.* comparisons with the new helpers) so the BetweenUs branching is centralized via isDisplayedMyPhotolog / isDisplayedPartnerPhotolog.
🤖 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/certification/TaskCertificationViewModel.kt`:
- Around line 151-168: The uploadPhotolog function currently calls
LocalDate.parse(navArgs.selectedDate) inside the launchResult block which can
throw DateTimeParseException and crash; before calling launchResult, validate or
safely parse navArgs.selectedDate (e.g., check for blank and attempt parsing
inside a try/catch) and if parsing fails show the same error toast and return
early; then pass the successfully parsed verificationDate into PhotologParam
when calling photologRepository.uploadPhotolog so uploadPhotolog,
navArgs.selectedDate, LocalDate.parse, PhotologParam, launchResult and
handleUploadPhotologSuccess are updated accordingly to avoid unhandled parsing
exceptions.
---
Outside diff comments:
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/contract/TaskCertificationDetailUiState.kt`:
- Around line 92-112: The current PhotoLogs.toUiState function calls
BetweenUs.valueOf(betweenUs) which will throw IllegalArgumentException for
invalid nav input; replace that unsafe call with a safe parse (e.g.,
BetweenUs.entries.find { it.name == betweenUs } or runCatching {
BetweenUs.valueOf(betweenUs) }.getOrNull()) and supply a sensible fallback (use
an appropriate default enum constant from BetweenUs) before assigning to
currentShow so malformed navigation strings no longer crash the app.
---
Duplicate comments:
In
`@feature/task-certification/src/main/java/com/twix/task_certification/certification/TaskCertificationViewModel.kt`:
- Around line 181-198: The onSuccess path in modifyPhotolog currently calls
detailRefreshBus.notifyChanged(TaskCertificationRefreshBus.Publisher.PHOTOLOG)
which triggers unnecessary goal list refreshes; change the publisher to
TaskCertificationRefreshBus.Publisher.EDITOR in the modifyPhotolog function
(inside the onSuccess lambda) so only editor-related subscribers are notified
when photolog is modified, keeping the existing
tryEmitSideEffect(TaskCertificationSideEffect.NavigateToDetail) and error
handling intact.
- Around line 170-179: The EDITOR branch in handleUploadPhotologSuccess() is
unreachable because upload() calls modifyPhotolog() for
NavRoutes.TaskCertificationRoute.From.EDITOR, so remove the dead branch or make
it defensive: replace the NavRoutes.TaskCertificationRoute.From.EDITOR -> Unit
branch with a clear error/log (e.g., processLogger.error or throw
IllegalStateException) to surface unexpected calls; update
handleUploadPhotologSuccess() and add a brief comment referencing upload() and
NavRoutes.TaskCertificationRoute.From to explain why the branch was changed.
---
Nitpick comments:
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/component/TaskCertificationCardContent.kt`:
- Around line 27-32: The buttonTitle branch uses uiState.currentShow ==
BetweenUs.ME while rotation and onClick use uiState.isDisplayedMyPhotolog,
causing inconsistency; update the buttonTitle logic in
TaskCertificationCardContent (the code that sets buttonTitle) to use
uiState.isDisplayedMyPhotolog for its ME/PARTNER decision (mirroring rotation
and onClick), keeping the same stringResource choices and preserving handlers
onClickUpload and onClickSting so all three decisions (buttonTitle, rotation,
onClick) derive from the same uiState.isDisplayedMyPhotolog condition.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/detail/contract/TaskCertificationDetailUiState.kt`:
- Around line 81-89: toSerializer() currently uses sentinel values when
myPhotolog is null which can leak invalid data to the Editor; change the
signature of toSerializer() to return EditorNavArgs? (nullable) and inside
return null when myPhotolog == null (or alternatively require(canModify) at the
top if you prefer a hard failure), and then update all callers to handle the
nullable return (or ensure canModify is checked before calling). Reference
symbols: toSerializer(), myPhotolog, EditorNavArgs, canModify.
- Around line 69-79: Refactor the boolean checks to use shared helpers: add a
new computed property isDisplayedPartnerPhotolog (val
isDisplayedPartnerPhotolog: Boolean get() = currentShow == BetweenUs.PARTNER)
and change canModify to use isDisplayedMyPhotolog && isDisplayedGoalCertificated
and canReaction to use isDisplayedPartnerPhotolog && isDisplayedGoalCertificated
(replace direct currentShow == BetweenUs.* comparisons with the new helpers) so
the BetweenUs branching is centralized via isDisplayedMyPhotolog /
isDisplayedPartnerPhotolog.
Comment on lines
+151
to
+168
| private fun uploadPhotolog(fileName: String) { | ||
| launchResult( | ||
| block = { | ||
| photologRepository.uploadPhotoLog( | ||
| photologRepository.uploadPhotolog( | ||
| PhotologParam( | ||
| goalId = goalId, | ||
| goalId = navArgs.goalId, | ||
| fileName = fileName, | ||
| comment = currentState.commentUiModel.comment, | ||
| verificationDate = LocalDate.now(), | ||
| comment = currentState.comment.value, | ||
| verificationDate = LocalDate.parse(navArgs.selectedDate), | ||
| ), | ||
| ) | ||
| }, | ||
| onSuccess = { handleUploadPhotologSuccess() }, | ||
| onError = { | ||
| showToast(R.string.task_certification_upload_fail, ToastType.ERROR) | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find launchResult function definition
rg "fun launchResult" --type kotlin -A 20Repository: YAPP-Github/Twix-Android
Length of output: 50
🏁 Script executed:
#!/bin/bash
# Find the ViewModel base class and its structure
fd "TaskCertificationViewModel.kt" -x cat -n {}Repository: YAPP-Github/Twix-Android
Length of output: 9039
🏁 Script executed:
#!/bin/bash
# Look for navArgs definition and selectedDate format
rg "selectedDate|navArgs" --type kotlin -B 2 -A 2Repository: YAPP-Github/Twix-Android
Length of output: 30341
🏁 Script executed:
#!/bin/bash
# Find BaseViewModel implementation
fd "BaseViewModel.kt" -x cat -n {}Repository: YAPP-Github/Twix-Android
Length of output: 5361
🏁 Script executed:
#!/bin/bash
# Search for launchResult function definition in BaseViewModel
rg "fun launchResult" --type kotlin -B 2 -A 15Repository: YAPP-Github/Twix-Android
Length of output: 50
LocalDate.parse(navArgs.selectedDate) 파싱 실패에 대한 보호가 필요합니다.
Line 159에서 LocalDate.parse(navArgs.selectedDate)를 호출하는데, launchResult의 예외 처리가 충분하지 않습니다. launchResult의 try-catch 블록은 CancellationException만 처리하고, DateTimeParseException과 같은 다른 예외는 처리하지 않아 런타임 크래시로 이어질 수 있습니다.
개선 방안:
navArgs.selectedDate가 비어있거나 잘못된 형식일 경우를 대비한 사전 검증 추가- 파싱을 시도하기 전에 형식 유효성 검사
- 예외 처리를 명시적으로 추가하거나, 파싱 로직을 별도 함수로 분리하여 안전하게 처리
예를 들어, block 밖에서 미리 검증하는 방식:
private fun uploadPhotolog(fileName: String) {
val verificationDate = try {
LocalDate.parse(navArgs.selectedDate)
} catch (e: Exception) {
showToast(R.string.task_certification_upload_fail, ToastType.ERROR)
return
}
launchResult(
block = {
photologRepository.uploadPhotolog(
PhotologParam(
goalId = navArgs.goalId,
fileName = fileName,
comment = currentState.comment.value,
verificationDate = verificationDate,
),
)
},
onSuccess = { handleUploadPhotologSuccess() },
onError = {
showToast(R.string.task_certification_upload_fail, ToastType.ERROR)
},
)
}🤖 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/certification/TaskCertificationViewModel.kt`
around lines 151 - 168, The uploadPhotolog function currently calls
LocalDate.parse(navArgs.selectedDate) inside the launchResult block which can
throw DateTimeParseException and crash; before calling launchResult, validate or
safely parse navArgs.selectedDate (e.g., check for blank and attempt parsing
inside a try/catch) and if parsing fails show the same error toast and return
early; then pass the successfully parsed verificationDate into PhotologParam
when calling photologRepository.uploadPhotolog so uploadPhotolog,
navArgs.selectedDate, LocalDate.parse, PhotologParam, launchResult and
handleUploadPhotologSuccess are updated accordingly to avoid unhandled parsing
exceptions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
이슈 번호
리뷰/머지 희망 기한 (선택)
작업내용
결과물
🛠 코멘트 수정 / 인증샷 수정
코멘트 수정
default.mp4
인증샷 수정
default.mp4
리뷰어에게 추가로 요구하는 사항 (선택)