목표 수정, 삭제, 끝내기 API 연동 #74
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough이 변경사항은 목표 API 통신 기능을 확장합니다. 네트워크 모델 계층에서 Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 상세 리뷰 코멘트✅ 잘 구현된 부분계층별 책임 분리가 명확합니다
낙관적 UI 업데이트 구현이 견고합니다
📋 검토가 필요한 부분1. 상태 관리의 복잡성 증가 현재 상황: 고려할 점:
제안: // 이런 식으로 상호배제 상태를 모델링하는 것도 고려해볼 수 있습니다
sealed class GoalManageDialogState {
object None : GoalManageDialogState()
data class DeleteConfirm(val goalDialogState: GoalDialogState) : GoalManageDialogState()
data class EndConfirm(val goalDialogState: GoalDialogState) : GoalManageDialogState()
}2. 에러 처리와 토스트 메시지 현재 상황:
고려할 점:
제안: // 에러 타입별 메시지 매핑을 고려해보세요
private fun getErrorMessageResId(error: Throwable): Int = when (error) {
is NetworkException -> R.string.error_network
is TimeoutException -> R.string.error_timeout
else -> R.string.toast_update_goal_failed
}3. Save Intent의 goalId 파라미터 현재 상황: 확인 사항:
4. 날짜 유효성 검증 현재 상황: if (endDate.isBefore(startDate)) {
sendSideEffect(ShowToast(...))
return
}제안 검토 항목:
5. 빈 상태 UI 컴포넌트 현재 상황: when (isDetail) {
true -> /* 목표 없음만 표시 */
false -> /* 추가 안내 텍스트와 화살표 표시 */
}제안:
enum class EmptyGoalGuideMode { FULL, COMPACT }
fun EmptyGoalGuide(mode: EmptyGoalGuideMode = FULL, ...)🔍 추가 검토 사항6. 메뉴 열림 상태와 pending 상태 동시성 // GoalSummaryItem에서
if (isPending) {
// 상호작용 비활성화
}질문:
7. 새로운 drawable 리소스
제안:
전반적으로 이 PR은 API 연동의 주요 기능들을 체계적으로 구현했습니다. 위의 검토 항목들은 추후 유지보수성과 확장성을 더욱 높이기 위한 제안입니다. 🎯 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorScreen.kt (1)
130-130:⚠️ Potential issue | 🟠 Major수정 모드에서
internalSelectedIcon이 서버에서 가져온 아이콘과 동기화되지 않는 문제가 있습니다.
remember { mutableStateOf(uiState.selectedIcon) }는 최초 컴포지션 시점의 값만 캡처합니다. 수정 모드에서는initGoal이 비동기로GoalDetail을 가져온 후uiState.selectedIcon을 갱신하지만,internalSelectedIcon은 초기 기본값(예:FIRE)으로 남게 됩니다. 결과적으로 아이콘 편집 다이얼로그를 열면 서버에서 가져온 아이콘이 아닌 기본 아이콘이 선택된 상태로 표시됩니다.
LaunchedEffect를 활용하여uiState.selectedIcon변경 시internalSelectedIcon을 동기화하는 방법을 제안합니다.🐛 수정 제안
var internalSelectedIcon by remember { mutableStateOf(uiState.selectedIcon) } + + LaunchedEffect(uiState.selectedIcon) { + internalSelectedIcon = uiState.selectedIcon + }
🤖 Fix all issues with AI agents
In `@feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageScreen.kt`:
- Around line 296-299: The IntOffset used in the CommonPopup call is hardcoded
in pixels (IntOffset(x = -180, y = 68)) which breaks on different screen
densities; update the CommonPopup anchorOffset to compute pixel values from dp
using LocalDensity (convert -180.dp and 68.dp to pixels inside the composable
scope) so the popup position scales correctly across devices, e.g., obtain
density via LocalDensity.current, convert the dp offsets to px and pass those
ints into IntOffset while keeping the same parameters (CommonPopup, menuVisible,
onDismiss = onCloseMenu).
🧹 Nitpick comments (8)
domain/src/main/java/com/twix/domain/model/goal/GoalDetail.kt (1)
7-16:GoalDetailResponse와의 필드 불일치를 확인해 주세요.
GoalDetailResponse에는goalStatus필드가 있지만, 도메인 모델인GoalDetail에는 해당 필드가 없습니다. 현재 사용하지 않더라도, 목표의 상태(진행중/완료 등)는 향후 UI에서 필요할 가능성이 높아 보입니다. 의도적으로 제외한 것이라면 괜찮지만, 누락이라면 추가를 검토해 주세요.또한
createdAt이String타입인데, 다른 날짜 필드(startDate,endDate)는LocalDate를 사용하고 있어 일관성이 다소 떨어집니다. 도메인 모델에서는LocalDateTime등의 타입을 사용하는 것이 타입 안전성 측면에서 더 좋을 수 있습니다.feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorIntent.kt (1)
37-39:Save에id를 전달하는 방식에 대해 논의가 필요해 보입니다.
InitGoal에서 이미id를 전달받아 ViewModel이 목표 ID를 알고 있을 텐데,Save에서도 다시id를 전달하는 이유가 있을까요? ViewModel의 State에 이미 저장된goalId를 활용하면 Intent의 중복 파라미터를 줄이고, 호출부에서 잘못된 ID를 전달하는 실수도 방지할 수 있습니다.만약 create(
id == -1등)와 update를 구분하기 위한 용도라면, sealed class나 nullable 타입(Long?)을 사용하는 것이 매직 넘버보다 의도가 명확해질 수 있습니다.feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorViewModel.kt (1)
83-111:save함수의 create/update 분기 로직이 명확합니다. 다만 중복 요청 방지 고려가 필요합니다.
id == -1L기준의 create/update 분기, 날짜 유효성 검증,launchResult를 활용한 에러 처리 모두 잘 구성되어 있습니다.onSuccess에서tryEmitSideEffect(non-suspend),onError에서emitSideEffect(suspend)를 사용한 것도launchResult콜백 시그니처에 맞게 적절합니다.한 가지 우려 사항은 중복 요청 방지 입니다. 사용자가 "완료" 버튼을 빠르게 여러 번 탭하면
isEnabled체크만으로는 중복 API 호출을 막을 수 없습니다.launchResult의onStart/onFinally콜백으로 로딩 상태를 관리하여 중복 요청을 방지하는 것은 어떨까요?♻️ 로딩 상태 기반 중복 요청 방지 예시
private fun save(id: Long) { - if (!currentState.isEnabled) return + if (!currentState.isEnabled || currentState.isLoading) return if (currentState.endDateEnabled && currentState.endDate.isBefore(currentState.startDate)) { // ... } if (id == -1L) { launchResult( + onStart = { reduce { copy(isLoading = true) } }, + onFinally = { reduce { copy(isLoading = false) } }, block = { goalRepository.createGoal(currentState.toCreateParam()) }, // ... ) } else { launchResult( + onStart = { reduce { copy(isLoading = true) } }, + onFinally = { reduce { copy(isLoading = false) } }, block = { goalRepository.updateGoal(currentState.toUpdateParam(id)) }, // ... ) } }참고:
GoalEditorUiState에isLoading: Boolean = false필드 추가가 필요하며, UI 측에서도 로딩 중 버튼 비활성화 등의 처리를 연결할 수 있습니다.feature/goal-manage/src/main/java/com/twix/goal_manage/model/GoalManageUiState.kt (1)
14-48: 하드코딩된 더미 데이터가LocalDate.now()를 사용하고 있습니다.
goalSummaries의 기본값이LocalDate.now()를 사용하고 있어, State 생성 시점에 따라 다른 값을 가지게 됩니다. 이는 테스트와 Compose Preview에서 비결정적(non-deterministic) 동작을 유발할 수 있습니다. 개발용 더미 데이터라면emptyList()를 기본값으로 두고 Preview나 테스트에서 별도로 주입하는 방식이 더 안전합니다.혹시 API 연동 후에 제거할 예정인 임시 데이터인지요?
feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageViewModel.kt (1)
25-46:ShowEndDialog와ShowDeleteDialog의 중복 코드를 추출하는 것을 고려해보세요.두 분기의 로직이 거의 동일합니다(pending 체크 → goal 조회 → reduce로 dialog 상태 설정 + 메뉴 닫기). 현재 2개여서 관리 가능하지만, 향후 유사한 다이얼로그가 추가될 경우 공통 헬퍼로 추출하면 유지보수가 용이해질 수 있습니다.
♻️ 예시: 공통 헬퍼 추출
private fun showDialog( goalId: Long, setDialog: GoalManageUiState.(GoalDialogState) -> GoalManageUiState, ) { if (goalId in currentState.pendingGoalIds) return val goal = currentState.goalSummaries.firstOrNull { it.goalId == goalId } ?: return reduce { setDialog(GoalDialogState(goal.goalId, goal.name, goal.icon)) .copy(openedMenuGoalId = null) } }feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageScreen.kt (3)
191-210: 다이얼로그onConfirm에서 dismiss → confirm 순서에 주의가 필요합니다.현재 흐름은 다음과 같습니다:
val id = endDialog?.goalId— 현재 다이얼로그 상태에서 id 캡처onDismissEndDialog()— ViewModel에 DismissEndDialog dispatch →endDialog = nullid?.let(onConfirmEnd)— 캡처된 id로 EndGoal dispatch
id를 dismiss 전에 캡처하므로 현재는 안전하게 동작합니다. 다만endDialog가 Compose 리컴포지션 시점의uiState.endDialog를 참조하므로, 이론적으로onConfirm호출 직전에 외부 요인으로endDialog가 null이 되면id도 null이 됩니다.id?.let가드가 이를 방어하고 있어 크래시는 없지만, 의도한 동작이 무시될 수 있습니다.더 견고하게 하려면
endDialogSnapshot에서 id를 읽는 것도 방법입니다 (dismiss 애니메이션 중에도 값을 유지하므로).♻️ 스냅샷에서 id를 읽는 방식
onConfirm = { - val id = endDialog?.goalId + val id = endDialogSnapshot?.goalId onDismissEndDialog() id?.let(onConfirmEnd) },deleteDialog에도 동일하게 적용:
onConfirm = { - val id = deleteDialog?.goalId + val id = deleteDialogSnapshot?.goalId onDismissDeleteDialog() id?.let(onConfirmDelete) },
267-277:GoalSummaryItem에 대한 Preview Composable 제공을 고려해주세요.코딩 가이드라인에 따라 Compose UI에 Preview가 제공되는 것이 권장됩니다.
GoalSummaryItem,GoalManageScreen등 주요 컴포저블에@Preview를 추가하면 UI 변경 시 빠르게 검증할 수 있습니다.As per coding guidelines, "Preview Composable이 제공되는가?"
86-108:GoalManageScreen에openedMenuGoalId와pendingIds를uiState와 별도로 전달하고 있습니다.
uiState에 이미 포함된 필드를 별도 파라미터로 추출하는 것은 가독성 측면에서 의도는 이해되지만, 컴포저블 파라미터가 많아지면서 오히려 관리가 어려워질 수 있습니다.GoalManageScreen내부에서uiState로부터 직접 접근하는 방식과 비교해, 현재 방식을 선택한 특별한 이유가 있는지요? 예를 들어 향후GoalManageScreen을 public으로 변경하거나 별도 Preview에서 사용할 계획이라면 현재 방식이 합리적입니다.
feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageViewModel.kt (1)
142-151:⚠️ Potential issue | 🟠 Major주간 이동 시 목표 리스트 갱신 로직 누락
shiftWeek에서referenceDate만 변경되고selectedDate와fetchGoalSummaryList가 갱신되지 않아 다음과 같은 문제가 발생합니다:
문제 상황: 이번 주의 특정 날짜를 선택한 상태에서 "다음 주" 버튼을 누르면:
referenceDate는 다음 주로 변경됨 (WeeklyCalendar의 요일 표시는 갱신됨)- 하지만
selectedDate는 이전 주 날짜 그대로 유지LaunchedEffect(selectedDate)트리거 안 됨 →setSelectedDate호출 안 됨fetchGoalSummaryList호출 안 됨 → 목표 리스트는 이전 주 데이터 표시- 시각적으로도 WeeklyCalendar에서 선택 표시가 사라짐 (selectedDate가 새 주 범위 밖)
개선 방안:
- 권장:
shiftWeek에서referenceDate갱신 후 새로운 주의 첫날(또는 선택했던 요일) 자동 설정. 예:reduce { copy(referenceDate = newReference, selectedDate = newReference) }- 또는: WeeklyCalendar의
LaunchedEffect(referenceDate)감시하여 범위 벗어난selectedDate자동 조정- 또는: NextWeek/PreviousWeek Intent에 함께 설정할 기본 날짜 정보 포함
현재 구조에서는 사용자가 주간 이동 후 명시적으로 날짜를 다시 클릭해야만 목표 리스트가 갱신되므로, UX 관점에서도 개선이 필요합니다.
🤖 Fix all issues with AI agents
In
`@core/network/src/main/java/com/twix/network/model/response/goal/mapper/GoalMapper.kt`:
- Around line 58-67: The map in GoalSummaryListResponse.toDomain() is failing CI
due to a missing trailing comma and also passes endDate positionally; update the
GoalSummary construction to use the named parameter endDate =
it.endDate?.let(LocalDate::parse) and add the trailing comma after that argument
so the code is consistent with the other named arguments and satisfies KtLint;
locate this in the GoalSummaryListResponse.toDomain() function where GoalSummary
is instantiated.
In
`@core/network/src/main/java/com/twix/network/model/response/goal/model/GoalSummaryListResponse.kt`:
- Around line 6-9: The data class GoalSummaryListResponse is missing a trailing
comma in its primary constructor parameter list; update the class declaration
for GoalSummaryListResponse by adding a trailing comma after the parameter "val
goals: List<GoalSummaryResponse>," so it conforms to ktlint multiline-parameter
rules and re-run formatter/CI.
In
`@feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageViewModel.kt`:
- Line 117: The KtLint error indicates a missing trailing comma in the
emitSideEffect call; update the invocation of
emitSideEffect(GoalManageSideEffect.ShowToast(...)) so the argument list ends
with a trailing comma (e.g., after ToastType.ERROR) to satisfy KtLint formatting
rules; locate the call inside GoalManageViewModel (the onError lambda) and add
the trailing comma.
🧹 Nitpick comments (2)
feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageViewModel.kt (2)
25-46: ShowEndDialog / ShowDeleteDialog 중복 로직 추출 제안두 다이얼로그 표시 로직(Lines 25-34, 36-45)이 거의 동일한 패턴입니다:
pendingGoalIds가드 → 목표 조회 →reduce로 다이얼로그 상태 설정. 현재 두 개뿐이라 큰 문제는 아니지만, 추후 다이얼로그 종류가 늘어날 경우 유지보수가 어려워질 수 있습니다.공통 로직을 헬퍼로 추출하면 중복을 줄일 수 있습니다:
♻️ 리팩토링 예시
+ private fun showDialog( + goalId: Long, + setDialog: GoalManageUiState.(GoalDialogState) -> GoalManageUiState, + ) { + if (goalId in currentState.pendingGoalIds) return + val goal = currentState.goalSummaries.firstOrNull { it.goalId == goalId } ?: return + reduce { + setDialog(GoalDialogState(goal.goalId, goal.name, goal.icon)) + .copy(openedMenuGoalId = null) + } + }이후 Intent 처리에서:
is GoalManageIntent.ShowEndDialog -> showDialog(intent.goalId) { copy(endDialog = it) } is GoalManageIntent.ShowDeleteDialog -> showDialog(intent.goalId) { copy(deleteDialog = it) }
50-88: endGoal과 deleteGoal의 중복 코드 통합 제안두 함수의 구조가 거의 동일합니다 (가드 → 낙관적 제거 → pending 마킹 → launchResult → 에러 시 복원 + 토스트). 차이점은 repository 호출과 토스트 리소스뿐입니다. 현재 두 개뿐이므로 급하지는 않지만, 공통 패턴을 추출하면 향후 유지보수가 편해집니다.
♻️ 통합 예시
+ private fun executeGoalAction( + id: Long, + action: suspend () -> Unit, + errorMessageRes: Int, + ) { + if (id in currentState.pendingGoalIds) return + val removed = removeGoalOptimistically(id) ?: return + markPending(id, true) + launchResult( + block = { action() }, + onSuccess = { markPending(id, false) }, + onError = { + markPending(id, false) + restoreGoal(removed) + emitSideEffect(GoalManageSideEffect.ShowToast(errorMessageRes, ToastType.ERROR)) + }, + ) + } + private fun endGoal(id: Long) { - if (id in currentState.pendingGoalIds) return - val removed = removeGoalOptimistically(id) ?: return - markPending(id, true) - launchResult( - block = { goalRepository.completeGoal(id) }, - onSuccess = { markPending(id, false) }, - onError = { - markPending(id, false) - restoreGoal(removed) - emitSideEffect(GoalManageSideEffect.ShowToast(R.string.toast_complete_goal_failed, ToastType.ERROR)) - }, - ) + executeGoalAction(id, { goalRepository.completeGoal(id) }, R.string.toast_complete_goal_failed) } private fun deleteGoal(id: Long) { - if (id in currentState.pendingGoalIds) return - val removed = removeGoalOptimistically(id) ?: return - markPending(id, true) - launchResult( - block = { goalRepository.deleteGoal(id) }, - onSuccess = { markPending(id, false) }, - onError = { - markPending(id, false) - restoreGoal(removed) - emitSideEffect(GoalManageSideEffect.ShowToast(R.string.toast_delete_goal_failed, ToastType.ERROR)) - }, - ) + executeGoalAction(id, { goalRepository.deleteGoal(id) }, R.string.toast_delete_goal_failed) }
core/network/src/main/java/com/twix/network/model/response/goal/mapper/GoalMapper.kt
Show resolved
Hide resolved
.../network/src/main/java/com/twix/network/model/response/goal/model/GoalSummaryListResponse.kt
Show resolved
Hide resolved
feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageViewModel.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageScreen.kt (1)
288-295:⚠️ Potential issue | 🟡 Minor미트볼 메뉴 아이콘의
contentDescription이null입니다.접근성(Accessibility)을 위해 스크린 리더 사용자가 이 버튼의 용도를 알 수 있도록 적절한
contentDescription을 제공하는 것이 좋겠습니다.🔧 수정 제안
Image( painter = painterResource(R.drawable.ic_meatball), - contentDescription = null, + contentDescription = stringResource(R.string.action_edit), // 또는 "더보기 메뉴" 등의 적절한 문자열 리소스 modifier =As per coding guidelines,
core/design-system/**: "접근성(Accessibility) 고려 여부"
🤖 Fix all issues with AI agents
In
`@feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorViewModel.kt`:
- Around line 68-79: The setGoal function is missing mapping of the
GoalDetail.startDate into the UI state, causing the UI to show the default
LocalDate.now(); update the reduce { copy(...) } call inside setGoal to include
startDate = goal.startDate so GoalEditorUiState reflects the actual start date
when loading an existing GoalDetail; locate the setGoal method and its reduce {
copy(...) } block (referencing setGoal, GoalDetail, startDate,
GoalEditorUiState) and add that single field assignment while keeping existing
endDate and endDateEnabled logic unchanged.
In `@feature/main/src/main/java/com/twix/home/component/EmptyGoalGuide.kt`:
- Around line 31-36: The Image composable using
painterResource(R.drawable.ic_empty_goal_home) still has the old
contentDescription "empty face"; update the contentDescription to match the new
drawable (e.g., "empty goal home") or, preferably, use a localized string
resource via stringResource (create R.string.empty_goal_home) and set
contentDescription = stringResource(R.string.empty_goal_home) so accessibility
reflects the current image; locate the Image invocation (painterResource,
contentDescription) and replace the hardcoded text accordingly.
🧹 Nitpick comments (6)
core/network/src/main/java/com/twix/network/service/GoalService.kt (1)
49-52: 엔드포인트 경로(detail)와 메서드/타입 이름(Summary)의 불일치가 있습니다.URL 경로는
api/v1/goals/detail인데, 메서드명은fetchGoalSummaryList, 반환 타입은GoalSummaryListResponse로 "summary"를 사용하고 있습니다.물론 서버 API 경로는 백엔드에서 정한 것이므로 클라이언트가 변경할 수 없지만, 이런 불일치는 유지보수 시 혼란을 줄 수 있습니다. 두 가지 중 하나를 고려해볼 수 있습니다:
- 서버 경로가 변경 가능하다면: 백엔드 팀과 논의하여
/goals/summary같은 경로로 맞추기- 서버 경로를 변경할 수 없다면: 메서드에 간단한 KDoc 주석을 추가하여 경로와 이름이 다른 이유를 명시하기
이 부분은 당장 기능에 영향을 주지는 않으니, 추후 정리 시 참고하시면 좋겠습니다.
feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorViewModel.kt (2)
81-111:save메서드의 전체적인 구조가 잘 설계되어 있습니다 👍유효성 검사 → 분기 처리 → 결과 핸들링의 흐름이 명확하고,
endDate검증도 적절합니다.onSuccess에서tryEmitSideEffect(non-suspend),onError에서emitSideEffect(suspend)를BaseViewModel의 콜백 시그니처에 맞게 올바르게 사용하고 있습니다.한 가지 제안:
id == -1L은 "새 목표 생성"을 의미하는 매직 넘버입니다. Android에서 흔히 사용되는 패턴이긴 하지만, 가독성과 유지보수를 위해 상수로 추출하면 의도가 더 명확해집니다.companion object { private const val NEW_GOAL_ID = -1L }
113-119:initGoal이 새 목표(id == -1L) 케이스에서 호출될 가능성을 확인해주세요.
initGoal에id == -1L에 대한 가드가 없어서, 새 목표 생성 플로우에서 실수로InitGoal(-1L)이 dispatch되면 서버에 잘못된 요청을 보내고 에러 토스트가 표시됩니다. UI 레이어에서 이미 분기 처리가 되어 있다면 괜찮지만, 방어적으로 early return을 추가하면 더 안전합니다.🛡️ 방어 코드 제안
private fun initGoal(id: Long) { + if (id == -1L) return launchResult( block = { goalRepository.fetchGoalDetail(id) }, onSuccess = { setGoal(it) }, onError = { emitSideEffect(GoalEditorSideEffect.ShowToast(R.string.toast_goal_fetch_failed, ToastType.ERROR)) }, ) }feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageIntent.kt (1)
6-44: 파라미터 네이밍 일관성에 대해 검토해 보면 좋겠습니다.
EndGoal과DeleteGoal은id를, 나머지 새로운 Intent들(OpenMenu,ShowEndDialog,ShowDeleteDialog,EditGoal)은goalId를 사용하고 있습니다. 모두 같은 목표 ID를 가리키므로 일관된 네이밍으로 통일하면 가독성이 향상될 것 같습니다. 기존 코드를 존중하여 필수는 아니지만, 향후 통일을 고려해 주세요.♻️ 통일 예시 (선택사항)
data class EndGoal( - val id: Long, + val goalId: Long, ) : GoalManageIntent data class DeleteGoal( - val id: Long, + val goalId: Long, ) : GoalManageIntent이 경우
GoalManageViewModel에서intent.id→intent.goalId로의 변경도 필요합니다.feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageScreen.kt (2)
87-109:openedMenuGoalId와pendingIds가uiState와 별도로 전달되고 있습니다 — 의도된 설계인지 확인이 필요합니다.
GoalManageScreen은uiState: GoalManageUiState를 받으면서, 동시에uiState.openedMenuGoalId와uiState.pendingGoalIds를 별도 파라미터로 전달하고 있습니다 (lines 89-90, 115, 129). 이미uiState안에 포함된 값을 중복 전달하면 API 표면이 넓어지고, 실수로 다른 값을 전달할 여지가 생깁니다.의도적인 설계(예: 테스트 용이성)라면 괜찮지만, 그렇지 않다면
GoalManageScreen내부에서uiState에서 직접 추출하는 것이 더 간결합니다.♻️ 간소화 예시
GoalManageScreen( uiState = uiState, - openedMenuGoalId = uiState.openedMenuGoalId, - pendingIds = uiState.pendingGoalIds, onBack = popBackStack, ... ) // GoalManageScreen 내부에서: +val openedMenuGoalId = uiState.openedMenuGoalId +val pendingIds = uiState.pendingGoalIds
309-314:onEdit호출 후onCloseMenu()가 중복 실행됩니다.
GoalManageViewModel의EditGoalIntent 핸들러(ViewModel line 49-51)에서 이미openedMenuGoalId = null로 메뉴를 닫고 있습니다. 따라서 line 313의onCloseMenu()호출은CloseMenuIntent를 중복 디스패치하게 됩니다. 동작에 문제는 없지만, 불필요한 상태 업데이트를 유발합니다.♻️ 중복 제거 제안
CommonPopupItem( text = stringResource(R.string.action_edit), onClick = { onEdit(item.goalId) - onCloseMenu() }, )
feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorViewModel.kt
Show resolved
Hide resolved
core/design-system/src/main/java/com/twix/designsystem/components/goal/EmptyGoalGuide.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/main/src/main/java/com/twix/home/HomeScreen.kt (1)
148-148:⚠️ Potential issue | 🟡 Minor
remember { LocalDate.now() }는 Composition 생존 기간 동안 갱신되지 않습니다.
remember로 캐싱된today는 Composition이 유지되는 동안 다시 계산되지 않습니다. 만약 사용자가 자정을 넘겨 앱을 계속 사용하는 경우,today가 이전 날짜로 남아 있어 "오늘의 목표" 타이틀이 잘못 표시될 수 있습니다.현재로서는 드문 엣지 케이스이고, 화면 재진입 시 recompose되므로 실제 영향은 적지만, 필요하다면
uiState에서 현재 날짜를 제공하거나selectedDate의 변경 시 함께 갱신되는 방식을 고려해볼 수 있습니다.
🧹 Nitpick comments (4)
core/design-system/src/main/java/com/twix/designsystem/components/goal/EmptyGoalGuide.kt (3)
62-68:contentDescription에 하드코딩된 영문 문자열 사용Line 64의
"empty goal arrow"도 하드코딩된 영문 문자열입니다. 이 화살표 이미지가 순수 장식(decorative) 용도라면contentDescription = null로 설정하여 스크린 리더가 건너뛰도록 하는 것이 접근성 측면에서 더 적절합니다. 만약 의미 있는 콘텐츠라면stringResource로 교체하는 것을 권장합니다.🔧 장식 이미지인 경우 수정 제안
Image( painter = painterResource(R.drawable.ic_empty_goal_arrow), - contentDescription = "empty goal arrow", + contentDescription = null, modifier = Modifier .offset(x = 60.dp), )As per coding guidelines,
core/design-system/**: "접근성(Accessibility) 고려 여부"
22-71: Preview Composable이 제공되지 않고 있습니다.디자인 시스템 컴포넌트는 다양한 상태를 빠르게 확인할 수 있도록
@Preview를 제공하는 것이 좋습니다.isDetail = true와false두 가지 상태에 대한 Preview를 추가하면 개발 시 시각적 피드백을 빠르게 얻을 수 있고, UI 회귀를 방지하는 데도 도움이 됩니다.💡 Preview 추가 예시
`@Preview`(showBackground = true) `@Composable` private fun EmptyGoalGuideHomePreview() { EmptyGoalGuide( text = "아직 목표가 없어요", isDetail = false, ) } `@Preview`(showBackground = true) `@Composable` private fun EmptyGoalGuideDetailPreview() { EmptyGoalGuide( text = "아직 목표가 없어요", isDetail = true, ) }As per coding guidelines,
core/design-system/**: 디자인 시스템 컴포넌트 및feature/**: "Preview Composable이 제공되는가?"
65-67:offset(x = 60.dp)매직 넘버 사용화살표 이미지의
offset(x = 60.dp)은 의도가 명확하지 않은 매직 넘버입니다. 이 값이 특정 디자인 시스템 스펙에서 나온 것인지, 아니면 시각적으로 맞추기 위한 임의 값인지 간단한 주석이나 상수 추출로 의도를 명확히 해주면 이후 유지보수에 도움이 될 것 같습니다. 혹시 디자인 시스템에서 정의된 간격 토큰이 있다면 그것을 사용하는 것도 고려해볼 수 있을까요?feature/goal-manage/src/main/java/com/twix/goal_manage/GoalManageScreen.kt (1)
113-131:openedMenuGoalId와pendingIds가uiState와 중복으로 전달되고 있습니다.
GoalManageScreen이uiState: GoalManageUiState를 받으면서 동시에openedMenuGoalId와pendingIds를 별도 파라미터로 받고 있습니다. 호출부(Line 90-91)를 보면uiState.openedMenuGoalId,uiState.pendingGoalIds에서 추출한 값을 다시 넘기고 있어 불필요한 중복입니다.두 가지 방향 중 하나를 선택하면 좋겠습니다:
uiState내부에서 직접 접근 (uiState.openedMenuGoalId)- 또는 개별 파라미터만 전달하고
uiState에서 제거현재는 동작에 문제가 없지만, 파라미터가 많아지면 유지보수 시 혼란을 줄 수 있습니다. 어떤 방향이 더 적합할지 의견이 궁금합니다.
1 participant
이슈 번호
작업내용
리뷰어에게 추가로 요구하는 사항 (선택)
GoalManageScreen에서 CommonDialog 렌더링하는 부분 보시면 snapshot 변수가 두개 있습니다. 얘네들의 목적은 다이얼로그가 해제되는 동안 내용이 null로 초기화돼서 아주 잠깐 빈 상태로 보이는 현상을 방지하려고 추가했어요!