인증샷 수정 화면 UI 구현 및 API 연동#87
인증샷 수정 화면 UI 구현 및 API 연동#87chanho0908 wants to merge 46 commits intofeat/#84-certification-modifyfrom
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
Show resolved
Hide resolved
...ication/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorViewModel.kt
Outdated
Show resolved
Hide resolved
- `navigateToCertification` 호출 시 `selectedDate`를 함께 전달하도록 수정 - `TaskCertificationRoute`의 경로 생성 방식을 `DetailSerializer`를 사용하여 JSON으로 직렬화하도록 변경 - `From`을 sealed class에서 enum class로 변경하고 관련 로직 수정
- 기존에 여러 인자를 각각 전달하던 방식에서 `DetailSerializer`를 사용하여 데이터를 전달하도록 변경 - 이에 따라 ViewModel에서 `savedStateHandle`을 통해 데이터를 파싱하고 사용하는 로직 수정 - 변경된 데이터 전달 방식에 맞춰 화면 이동 로직 업데이트
…ask-certification-update
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In
`@core/navigation/src/main/java/com/twix/navigation/serializer/EditorSerializer.kt`:
- Line 19: The enum NavRoutes.TaskCertificationRoute.From is not annotated for
kotlinx.serialization causing DetailSerializer (which is `@Serializable`) to fail;
update the enum declaration for From by adding the `@Serializable` annotation to
the enum class definition (i.e., annotate enum class From { HOME, DETAIL, EDITOR
} with `@Serializable`) so DetailSerializer's from property can be
serialized/deserialized correctly.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/certification/TaskCertificationViewModel.kt`:
- Around line 160-172: Remove the unreachable EDITOR branch inside
handleUploadPhotologSuccess: since upload() calls modifyPhotolog() when
serializer.from == NavRoutes.TaskCertificationRoute.From.EDITOR and
handleUploadPhotologSuccess is only invoked from uploadPhotolog's onSuccess, the
NavRoutes.TaskCertificationRoute.From.EDITOR case (the
detailRefreshBus.notifyChanged(TaskCertificationRefreshBus.Publisher.EDITOR)
branch) can be deleted; leave the DETAIL and HOME branches and the
tryEmitSideEffect(TaskCertificationSideEffect.NavigateToBack) intact and ensure
no other callers expect the EDITOR behavior.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorRoute.kt`:
- Around line 130-202: The AsyncImage inside TaskCertificationEditorScreen’s
PhotologCard is missing a Modifier.fillMaxSize(), so the image may not fully
occupy the PhotologCard box; update the AsyncImage call in
TaskCertificationEditorScreen to supply a modifier (e.g.,
Modifier.fillMaxSize()) so it fills the parent content area (preserve any
existing modifiers by chaining if needed) to ensure the image covers the
PhotologCard fully.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorViewModel.kt`:
- Around line 61-78: modifyComment() currently notifies detailRefreshBus and
shows success toast on onSuccess but does not navigate back; update onSuccess to
also perform the intended navigation (e.g., call the ViewModel/Router navigation
method or emit an event to pop the Detail screen) so the user is returned to the
previous screen after a successful modify. Locate modifyComment(), the
launchResult onSuccess block, and add a call alongside
detailRefreshBus.notifyChanged(...) and showToast(...) to trigger navigation
(reference the app's existing navigation helper or event emitter used by other
ViewModel actions).
- Around line 27-35: serializer 프로퍼티는 ViewModel 내부에서만 사용되므로 가시성을 제한하세요:
TaskCertificationEditorViewModel에서 현재 공개된 val serializer를 private val
serializer로 변경하여 외부 접근을 차단하고, 이 값이 사용되는 init 블록과 reduceInitialState() 메서드에 영향을
주지 않도록 선언 위치를 유지하세요.
In
`@feature/task-certification/src/main/java/com/twix/task_certification/navigation/TaskCertificationGraph.kt`:
- Around line 66-90: The navigation from TaskCertificationEditorRoute to
TaskCertificationRoute is creating a DetailSerializer without a selectedDate,
causing LocalDate.parse to crash in TaskCertificationViewModel; update the
lambda inside TaskCertificationEditorRoute (where you build DetailSerializer) to
include selectedDate (e.g., selectedDate = LocalDate.now().toString() or,
better, preserve and pass the actual selected date from EditorSerializer if
available) so DetailSerializer.selectedDate is never an empty string; ensure
references: TaskCertificationEditorRoute,
NavRoutes.TaskCertificationRoute.createRoute, DetailSerializer, and
TaskCertificationViewModel are used to locate and fix the code.
- Around line 101-110: 현재 코드에서 TaskCertificationRoute 호출부가
navController.popBackStack(route = NavRoutes.TaskCertificationDetailRoute.route,
...)처럼 패턴 문자열(예: "task_certification_detail/{goalId}/{date}/{betweenUs}")을 넘기고
있어 백스택의 resolved route와 매칭되지 않습니다; TaskCertificationRoute 블록(특히
navController::popBackStack 사용 부분)을 수정해 navController.popBackStack()로 단순히 상위
엔트리를 pop하도록 변경하거나, 만약 특정 Detail 화면으로 정확히 돌아가야 한다면
NavRoutes.TaskCertificationDetailRoute.createRoute(goalId, date, betweenUs)를 사용해
resolved route 문자열을 만들고 그 값을 popBackStack(route = resolvedRoute, inclusive =
false)에 넘기거나, 인증 화면 진입 시 goalId/date/betweenUs를 저장해 이후에 이를 사용해 resolved route를
구성해 호출하도록 구현하세요.
🧹 Nitpick comments (12)
domain/src/main/java/com/twix/domain/repository/PhotoLogRepository.kt (1)
8-8: 도메인 모듈에서java.time.LocalDate사용에 대한 고려코딩 가이드라인에서 도메인 모듈은 "Android 및 외부 프레임워크 의존성이 없는가?"를 확인하도록 되어 있습니다.
java.time.LocalDate는 Java 표준 라이브러리이므로 엄밀히 Android 프레임워크 의존성은 아니지만, Android API 26 미만에서는 desugaring이 필요합니다. 프로젝트의 minSdk가 26 이상이거나 desugaring이 활성화되어 있다면 문제없습니다.혹시 도메인 모듈의 순수성을 더 엄격하게 유지하고 싶다면,
kotlinx-datetime의LocalDate나 도메인 전용 날짜 타입을 고려해 볼 수도 있습니다. 현재 구조에서 문제가 되는지는 프로젝트 컨벤션에 따라 판단해 주세요. As per coding guidelines, "Domain 모듈: Android 및 외부 프레임워크 의존성이 없는가?"#!/bin/bash # minSdk 확인 및 desugaring 설정 확인 rg -n "minSdk|coreLibraryDesugaring|isCoreLibraryDesugaringEnabled" --type=groovy --type-add 'kts:*.kts' --type=kts -C 2 | head -30core/design-system/src/main/java/com/twix/designsystem/components/comment/model/CommentUiModel.kt (1)
13-17:canUpload로직에서 불필요한 조건 제거 가능
hasMaxCommentLength는value.length == COMMENT_COUNT이므로, 이것이true이면value.isNotEmpty()는 항상true입니다. 따라서value.isNotEmpty() &&부분은 중복 검사입니다. 또한,||와&&의 연산자 우선순위가 혼동될 수 있으므로 명시적 괄호를 추가하면 가독성이 향상됩니다.♻️ 간소화 제안
val canUpload: Boolean get() = - value.isEmpty() || - value.isNotEmpty() && - hasMaxCommentLength + value.isEmpty() || hasMaxCommentLengthcore/navigation/src/main/java/com/twix/navigation/serializer/EditorSerializer.kt (2)
7-14: "Serializer"라는 클래스명이 실제 역할과 다소 다릅니다.kotlinx.serialization에서
Serializer는 보통KSerializer<T>구현체를 지칭합니다. 이 클래스들은 네비게이션 라우트 간 데이터를 전달하기 위한 데이터 홀더(DTO)이므로,EditorRouteData/DetailRouteData또는EditorRouteArgs/DetailRouteArgs와 같은 이름이 역할을 더 명확히 전달할 수 있습니다. 현재 이름은 코드를 읽는 다른 개발자가KSerializer와 혼동할 수 있어요.Also applies to: 17-23
17-23:photologId의 센티널 값(-1) 대신 nullable 타입 사용을 고려해 보세요.
photologId = -1은 "값 없음"을 나타내는 매직 넘버입니다. 소비하는 측에서-1을 특별한 값으로 처리해야 하므로, 실수로 유효한 ID처럼 사용될 수 있습니다.Long? = null로 선언하면 컴파일러가 null 체크를 강제하여 더 안전합니다.`@Serializable` data class DetailSerializer( val goalId: Long, val from: NavRoutes.TaskCertificationRoute.From, - val photologId: Long = -1, + val photologId: Long? = null, val selectedDate: String = "", val comment: String = "", )다만, 이미 프로젝트 전반에서
-1을 센티널로 사용하는 컨벤션이 있다면 현 상태도 괜찮습니다.#!/bin/bash # -1을 photologId 센티널 값으로 사용하는 패턴이 프로젝트에 있는지 확인 rg -n 'photologId.*=.*-1' --type=kotlinfeature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetailViewModel.kt (1)
37-41:LocalDate.parse()의 파싱 실패 시 예외 처리가 없습니다.
savedStateHandle에서 가져온 문자열이 null이 아니지만 유효하지 않은 날짜 형식일 경우,LocalDate.parse()가DateTimeParseException을 던집니다.error()로 null 케이스는 처리했지만, 잘못된 형식의 문자열에 대한 방어가 없어요.네비게이션 인자가 항상
LocalDate.toString()결과로 전달된다면 실제 문제가 될 가능성은 낮지만, 방어적 코딩 관점에서 고려해 볼 수 있습니다.🛡️ 방어적 파싱 예시
private val selectedDate: LocalDate = - LocalDate.parse( - savedStateHandle[NavRoutes.TaskCertificationDetailRoute.ARG_DATE] - ?: error(TARGET_DATE_NOT_FOUND), - ) + runCatching { + LocalDate.parse( + savedStateHandle[NavRoutes.TaskCertificationDetailRoute.ARG_DATE] + ?: error(TARGET_DATE_NOT_FOUND), + ) + }.getOrElse { error("Invalid date format: ${it.message}") }feature/main/src/main/java/com/twix/main/MainScreen.kt (1)
38-38:HomeViewModel을MainRoute에서 생성하여MainScreen에 전달하는 구조에 대해 — 현재로서는 괜찮습니다.
koinViewModel()로 생성한HomeViewModel을 파라미터로 전달하여MainScreen과HomeRoute가 동일 인스턴스를 공유하는 패턴인데, 캘린더 상태 동기화 목적이라면 합리적입니다. 다만 향후 탭이 늘어나면MainScreen의 파라미터가 비대해질 수 있으니, 그때는 네비게이션 스코프 기반의 공유 ViewModel 등을 고려해 보시면 좋겠습니다.As per coding guidelines, "State Hoisting이 적절히 적용되었는가?"
Also applies to: 54-54
core/navigation/src/main/java/com/twix/navigation/NavRoutes.kt (2)
60-65:createRoute메서드의 JSON 인코딩 로직이 중복됩니다.
TaskCertificationRoute.createRoute와TaskCertificationEditorRoute.createRoute가 "JSON 직렬화 → URI 인코딩 → 문자열 조합"이라는 동일한 패턴을 반복합니다. 공통 헬퍼로 추출하면 향후 다른 라우트가 추가될 때도 일관성을 유지할 수 있습니다.♻️ 공통 헬퍼 추출 예시
// NavRoutes 내부 또는 util에 추가 private inline fun <reified T> encodeRouteData(data: T): String = Uri.encode(Json.encodeToString(data))그러면 각
createRoute에서:fun createRoute(data: DetailSerializer): String { - val json = Json.encodeToString(data) - val encoded = Uri.encode(json) - return "task_certification/$encoded" + return "task_certification/${encodeRouteData(data)}" }Also applies to: 71-76
51-65: 직렬화된 JSON blob을 단일 route argument로 전달하는 방식에 대한 참고 사항입니다.이 접근법은 여러 파라미터를 구조화된 객체로 전달할 수 있어서 편리하지만, 몇 가지 트레이드오프가 있습니다:
- 딥링크를 수동으로 구성하기 어려워집니다.
- 로그에서 라우트 경로의 가독성이 떨어집니다.
- URI 길이 제한에 도달할 수 있습니다 (데이터가 커질 경우).
현재 데이터 크기가 작으므로 실용적인 선택이지만, 향후
imageUrl이 매우 긴 URL일 경우 URI 인코딩 후 길이가 문제될 수 있다는 점은 인지해 두시면 좋겠습니다. 필요시Navigation Compose의 type-safe navigation 또는 shared ViewModel 패턴도 대안이 될 수 있습니다.feature/task-certification/src/main/java/com/twix/task_certification/detail/TaskCertificationDetail.kt (1)
64-64:navigateToEditor에 전체 UI State를 전달하는 것이 적절한지 검토해 보시면 좋겠습니다.
navigateToEditor: (TaskCertificationDetailUiState) -> Unit으로 전체 UI 상태를 콜백에 전달하고 있습니다. 이 방식은 호출하는 네비게이션 계층이TaskCertificationDetailUiState의 내부 구조에 의존하게 됩니다.네비게이션에 필요한 최소한의 데이터(예: goalId, photologId, imageUrl, comment 등)만 전달하는 별도의 모델이나 파라미터 목록으로 변경하면:
- 모듈 간 결합도가 낮아지고
- UI State 구조 변경 시 네비게이션 계층에 미치는 영향을 줄일 수 있습니다
다만 현재
TaskCertificationGraph에서EditorSerializer로 변환하는 중간 계층이 있다면 실용적인 선택일 수 있으므로, 팀 내에서 논의해 보시는 것을 권합니다.As per coding guidelines, "State Hoisting이 적절히 적용되었는가?"
#!/bin/bash # navigateToEditor가 TaskCertificationGraph에서 어떻게 사용되는지 확인 rg -n -A10 'navigateToEditor' --type=kotlin -g '**TaskCertificationGraph*'Also applies to: 117-117
feature/task-certification/src/main/java/com/twix/task_certification/editor/model/TaskCertificationEditorUiState.kt (2)
18-19:isCommentNotChanged는 이중 부정으로 가독성이 떨어질 수 있습니다.사용처에서
if (!isCommentNotChanged)처럼 이중 부정이 발생하면 코드를 읽기 어려워집니다. 긍정형 네이밍을 고려해 보세요.- val isCommentNotChanged: Boolean - get() = comment.value == originComment + val isCommentChanged: Boolean + get() = comment.value != originComment사용처도
!isCommentNotChanged→isCommentChanged로 단순해집니다.#!/bin/bash # isCommentNotChanged 사용처 확인 - 이중 부정 패턴이 있는지 rg -n 'isCommentNotChanged' --type=kotlin
25-26:imageName에서 URL이/로 끝나는 경우 빈 문자열이 반환됩니다.
imageUrl.split("/").last()는 URL이https://example.com/path/처럼/로 끝나면 빈 문자열을 반환합니다. 현재 사용처에서 이런 URL이 올 가능성이 낮다면 문제 없지만, 방어적으로 처리하려면:val imageName: String - get() = imageUrl.split(IMAGE_NAME_SEPARATOR).last() + get() = imageUrl.trimEnd('/').substringAfterLast(IMAGE_NAME_SEPARATOR)
substringAfterLast를 사용하면 구분자가 없는 경우 원본 문자열을 반환하므로 더 안전합니다.feature/task-certification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorRoute.kt (1)
59-128: MVI 패턴이 잘 적용되어 있습니다.Route 컴포저블에서 ViewModel의
uiState를collectAsStateWithLifecycle로 수집하고, 사용자 액션을dispatch로 전달하며, 일회성 이벤트를ObserveAsEvents로 처리하는 단방향 데이터 플로우가 잘 유지되고 있습니다. 카메라 퍼미션 처리 로직도 적절하게 분리되어 있네요.다만, 이 퍼미션 핸들링 패턴(
permissionLauncher+ rationale 처리)이TaskCertificationDetailRoute와 거의 동일하게 반복되고 있습니다. 현재 단계에서는 큰 문제가 아니지만, 향후 유지보수 시 공통 유틸/컴포저블로 추출하면 중복을 줄일 수 있습니다.
core/navigation/src/main/java/com/twix/navigation/serializer/EditorSerializer.kt
Show resolved
Hide resolved
...cation/src/main/java/com/twix/task_certification/certification/TaskCertificationViewModel.kt
Show resolved
Hide resolved
...rtification/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorRoute.kt
Show resolved
Hide resolved
...ication/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorViewModel.kt
Outdated
Show resolved
Hide resolved
...ication/src/main/java/com/twix/task_certification/editor/TaskCertificationEditorViewModel.kt
Show resolved
Hide resolved
...certification/src/main/java/com/twix/task_certification/navigation/TaskCertificationGraph.kt
Show resolved
Hide resolved
...certification/src/main/java/com/twix/task_certification/navigation/TaskCertificationGraph.kt
Show resolved
Hide resolved
1 participant
이슈 번호
리뷰/머지 희망 기한 (선택)
작업내용
결과물
🛠 코멘트 수정 / 인증샷 수정
코멘트 수정
default.mp4
인증샷 수정
default.mp4
리뷰어에게 추가로 요구하는 사항 (선택)