Conversation
📝 WalkthroughWalkthrough목표 생성 API 연동을 위한 기능들이 추가되었습니다. 네트워크 계층에 Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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.
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/GoalEditorViewModel.kt (1)
39-43:⚠️ Potential issue | 🟠 Major
title.isBlank()체크로 인해 사용자가 제목을 지울 수 없는 버그가 있습니다.현재 로직에서
title.isBlank()이면return하기 때문에, 사용자가 입력한 텍스트를 모두 삭제하여 빈 문자열로 만들 수 없습니다. 텍스트 필드에 "abc"를 입력한 후 전체 선택 + 삭제를 해도 상태가 갱신되지 않아 UI와 상태가 불일치하게 됩니다.제목 유효성 검증은 이미
isEnabled에서 수행하고 있으므로, 여기서는 빈 문자열도 허용하는 것이 적절합니다.🐛 제안 수정
private fun setTitle(title: String) { - if (title.isBlank()) return - reduce { copy(goalTitle = title) } }
🧹 Nitpick comments (8)
core/ui/src/main/java/com/twix/ui/base/BaseViewModel.kt (1)
72-74:trySend실패 시 결과가 무시되고 있습니다.같은 클래스의
dispatch()메서드(Line 52-55)에서는trySend실패 시 로깅을 하고 있는데,tryEmitSideEffect에서는 실패 결과를 완전히 무시하고 있습니다. SideEffect는 네비게이션이나 토스트 등 사용자에게 직접 노출되는 동작을 포함할 수 있어서, 유실 시 디버깅이 매우 어려워질 수 있습니다.
dispatch()와 동일한 패턴으로 실패 시 최소한 로깅을 추가하는 것은 어떨까요?제안
protected fun tryEmitSideEffect(effect: SE) { - sideEffectHolder.tryEmit(effect) + val result = sideEffectHolder.tryEmit(effect) + if (result.isFailure) { + logger.w { "SideEffect 유실: $effect, 원인 = ${result.exceptionOrNull()}" } + } }이 경우
SideEffectHolder.tryEmit의 반환 타입도ChannelResult<Unit>으로 변경이 필요합니다.core/ui/src/main/java/com/twix/ui/base/SideEffectHolder.kt (1)
14-16:trySend의 반환값을 호출자에게 전달하는 것을 권장합니다.현재
ChannelResult를 버리고 있어서, 채널이 가득 차거나 닫힌 경우에도 호출자가 이를 알 수 없습니다. 반환값을 전달하면BaseViewModel.tryEmitSideEffect에서 실패 시 로깅 등의 처리를 할 수 있습니다.제안
- fun tryEmit(effect: S) { - channel.trySend(effect) + fun tryEmit(effect: S): ChannelResult<Unit> { + return channel.trySend(effect) }domain/src/main/java/com/twix/domain/model/enums/GoalIconType.kt (1)
14-24:toApi()와fromApi()간 매핑 문자열 중복 — 개선 여지 있음현재
toApi()와fromApi()에서"ICON_DEFAULT","ICON_CLEAN"등 동일한 문자열이 양쪽에 중복 정의되어 있습니다. 새 enum 값을 추가할 때 한쪽만 수정하면 불일치가 발생할 수 있습니다.
toApi()는when이 exhaustive하므로 컴파일 타임에 누락을 잡아주지만,fromApi()는else -> DEFAULT분기가 있어 새 값 누락 시 조용히DEFAULT로 매핑됩니다.매핑 테이블을 한 곳에서 관리하거나, 네이밍 규칙이 일정하므로 아래처럼 단순화할 수도 있습니다:
♻️ 선택적 개선 제안
+ fun toApi(): String = "ICON_$name" + companion object { - fun fromApi(icon: String): GoalIconType = - when (icon) { - "ICON_DEFAULT" -> DEFAULT - ... - else -> DEFAULT - } + fun fromApi(icon: String): GoalIconType = + entries.find { it.toApi() == icon } ?: DEFAULT }이렇게 하면 매핑이
toApi()한 곳에서만 관리되어 유지보수가 편해집니다. 다만 현재 방식도 명시적이고 안전하므로, 팀 선호에 따라 결정하시면 됩니다.build-logic/convention/src/main/kotlin/com/twix/convention/FeatureConventionPlugin.kt (1)
18-18: 모든 Feature 모듈에:core:util의존성 추가 — 범위 확인 필요Convention Plugin에 추가하면 모든 Feature 모듈이
core:util에 의존하게 됩니다. 현재core:util이GoalRefreshBus만 포함하고 있다면, 실제로 이를 사용하는 모듈(home, goal-editor)에서만 개별적으로 의존성을 선언하는 것이 더 적절할 수 있습니다.다만
:core:result,:core:analytics등과 동일한 패턴이고,core:util이 앞으로 범용 유틸리티 모듈로 확장될 예정이라면 현재 방식도 합리적입니다. 팀 내에서core:util의 역할 범위에 대해 합의가 되어 있다면 괜찮을 것 같습니다. 🙂core/network/src/main/java/com/twix/network/model/response/goal/mapper/GoalMapper.kt (1)
44-53:LocalDate.parse()호출 시 예외 처리가 없어 런타임 크래시 위험이 있습니다.
LocalDate.parse(startDate)는 기본적으로 ISO-8601 형식(yyyy-MM-dd)만 허용합니다. API 응답의 날짜 형식이 이와 다르거나 잘못된 값이 반환되면DateTimeParseException이 발생합니다.같은 파일 내
GoalIconType.fromApi()와RepeatCycle.fromApi()는runCatching으로 안전하게 처리하고 있는 반면, 날짜 파싱에는 이러한 방어 로직이 없습니다. API 응답 형식이 확실히 보장된다면 현재 방식도 괜찮지만, 일관성을 위해 방어적 처리를 고려해보시면 어떨까요?🛡️ 방어적 파싱 예시
fun CreateGoalResponse.toDomain(): CreatedGoal = CreatedGoal( goalId = goalId, name = name, icon = GoalIconType.fromApi(icon), repeatCycle = RepeatCycle.fromApi(repeatCycle), repeatCount = repeatCount, - startDate = LocalDate.parse(startDate), - endDate = endDate?.let(LocalDate::parse), + startDate = LocalDate.parse(startDate), // API 계약상 yyyy-MM-dd 보장 확인 필요 + endDate = runCatching { endDate?.let(LocalDate::parse) }.getOrNull(), createdAt = createdAt, )domain/src/main/java/com/twix/domain/model/goal/CreatedGoal.kt (1)
7-16:createdAt필드를String대신 타입이 있는 날짜/시간 객체로 정의하는 것을 고려해 보시겠어요?다른 날짜 필드(
startDate,endDate)는LocalDate로 타입이 지정되어 있는 반면,createdAt만String입니다. 만약 이 값이 날짜/시간 정보를 담고 있다면LocalDateTime이나Instant등으로 파싱하여 도메인 모델의 일관성을 높일 수 있습니다. 현재 이 필드를 직접 활용하는 곳이 없다면 추후 리팩터링으로 남겨두어도 괜찮습니다.core/util/src/main/java/com/twix/util/bus/GoalRefreshBus.kt (1)
6-14: 이벤트 버스 구현이 적절합니다.
replay = 0으로 새 구독자가 이전 이벤트를 받지 않고,extraBufferCapacity = 1과tryEmit으로 비동기 안전하게 알림을 보내는 패턴이 잘 적용되었습니다. 빠르게 연속 호출되더라도 "갱신 필요" 시그널이 하나로 합쳐지는 효과가 있어 불필요한 API 호출을 방지합니다.한 가지 참고 사항으로,
onBufferOverflow기본값이SUSPEND이므로tryEmit호출 시 버퍼가 가득 차면 이벤트가 조용히 드롭됩니다. 현재 용도(갱신 시그널)에서는 문제되지 않지만, 명시적으로onBufferOverflow = BufferOverflow.DROP_OLDEST를 설정하면 의도가 더 명확해질 수 있습니다.💡 의도를 명시적으로 표현하는 제안
+import kotlinx.coroutines.channels.BufferOverflow + private val _events = MutableSharedFlow<Unit>( replay = 0, extraBufferCapacity = 1, + onBufferOverflow = BufferOverflow.DROP_OLDEST, )feature/main/src/main/java/com/twix/home/HomeViewModel.kt (1)
26-34: init 블록 구성이 잘 되어 있습니다. 다만 동시 호출 가능성에 대해 한 가지 제안드립니다.현재
goalRefreshBus.events.collect에서fetchGoalList()를 호출할 때, 이미 진행 중인 fetch가 있다면 동시에 두 개의 네트워크 요청이 발생할 수 있습니다. 현재 규모에서는 큰 문제가 되지 않지만, 향후 요청이 빈번해질 경우를 대비해 이전 요청을 취소하는 패턴을 고려해 볼 수 있습니다.예를 들어
collectLatest를 사용하면, 새 이벤트가 들어올 때 이전fetchGoalList()호출을 자동으로 취소할 수 있습니다. 다만launchResult내부 구현에 따라 취소가 전파되지 않을 수도 있으므로, 현재 구조에서 이 부분이 필요한지는 팀 내 논의가 필요할 수 있습니다.💡 collectLatest 적용 예시
viewModelScope.launch { - goalRefreshBus.events.collect { + goalRefreshBus.events.collectLatest { fetchGoalList() } }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorViewModel.kt`:
- Around line 80-88: The onSuccess callback uses tryEmitSideEffect which can
silently drop the NavigateToHome event; change it to call the suspend
emitSideEffect inside a coroutine so the event is delivered reliably — e.g.,
inside the onSuccess body use viewModelScope.launch {
emitSideEffect(GoalEditorSideEffect.NavigateToHome) } instead of
tryEmitSideEffect; alternatively, make launchResult's onSuccess parameter a
suspend lambda and call emitSideEffect directly. Ensure references:
launchResult, onSuccess, tryEmitSideEffect, emitSideEffect, viewModelScope,
GoalEditorSideEffect.NavigateToHome.
|
|
||
| launchResult( | ||
| block = { goalRepository.createGoal(currentState.toCreateParam()) }, | ||
| onSuccess = { | ||
| goalRefreshBus.notifyChanged() | ||
| tryEmitSideEffect(GoalEditorSideEffect.NavigateToHome) | ||
| }, | ||
| onError = { emitSideEffect(GoalEditorSideEffect.ShowToast(R.string.toast_create_goal_failed, ToastType.ERROR)) }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the GoalEditorViewModel file and search for sideEffectHolder
find . -name "GoalEditorViewModel.kt" -type fRepository: YAPP-Github/Twix-Android
Length of output: 147
🏁 Script executed:
# Search for MutableSharedFlow configuration in the codebase
rg "MutableSharedFlow|sideEffectHolder" --type kotlin -B 2 -A 2Repository: YAPP-Github/Twix-Android
Length of output: 3386
🏁 Script executed:
# Check for ViewModel base class that might define sideEffectHolder
fd -e kt -path "*/ViewModel.kt" | head -20Repository: YAPP-Github/Twix-Android
Length of output: 236
🏁 Script executed:
# Find SideEffectHolder implementation
rg "class SideEffectHolder" --type kotlin -A 10Repository: YAPP-Github/Twix-Android
Length of output: 989
🏁 Script executed:
# Also check the BaseViewModel for more context
cat core/ui/src/main/java/com/twix/ui/base/BaseViewModel.ktRepository: YAPP-Github/Twix-Android
Length of output: 4374
tryEmitSideEffect 사용 시 이벤트 유실 가능성이 존재합니다.
네비게이션처럼 반드시 전달되어야 하는 NavigateToHome 이벤트에 tryEmitSideEffect를 사용하는 것이 문제입니다. SideEffectHolder는 내부적으로 Channel(Channel.BUFFERED) (용량: 64)를 사용하는데, tryEmit()은 버퍼가 가득 차면 이벤트를 조용히 실패시킵니다. 용량이 64로 충분하지만, 버퍼 오버플로우 상황에서는 사용자가 화면 전환 없이 멈춰있을 수 있습니다.
반면 onError에서 emitSideEffect(suspend)를 사용하고 있어 불일치가 발생합니다.
개선 방안:
onSuccess 콜백 내에서 viewModelScope.launch { emitSideEffect(...) }로 감싸면 suspend 호출이 가능해지고, 버퍼가 가득 찬 경우에도 안전하게 대기할 수 있습니다:
개선 예시
launchResult(
block = { goalRepository.createGoal(currentState.toCreateParam()) },
onSuccess = {
goalRefreshBus.notifyChanged()
viewModelScope.launch {
emitSideEffect(GoalEditorSideEffect.NavigateToHome)
}
},
onError = { emitSideEffect(GoalEditorSideEffect.ShowToast(R.string.toast_create_goal_failed, ToastType.ERROR)) },
)launchResult의 onSuccess 시그니처를 suspend 람다로 변경하는 것도 근본적인 해결 방안입니다.
🤖 Prompt for AI Agents
In
`@feature/goal-editor/src/main/java/com/twix/goal_editor/GoalEditorViewModel.kt`
around lines 80 - 88, The onSuccess callback uses tryEmitSideEffect which can
silently drop the NavigateToHome event; change it to call the suspend
emitSideEffect inside a coroutine so the event is delivered reliably — e.g.,
inside the onSuccess body use viewModelScope.launch {
emitSideEffect(GoalEditorSideEffect.NavigateToHome) } instead of
tryEmitSideEffect; alternatively, make launchResult's onSuccess parameter a
suspend lambda and call emitSideEffect directly. Ensure references:
launchResult, onSuccess, tryEmitSideEffect, emitSideEffect, viewModelScope,
GoalEditorSideEffect.NavigateToHome.
chanho0908
left a comment
chanho0908
left a comment
There was a problem hiding this comment.
고생했어 현수야 !
내가 남긴 리뷰는 시간을 가지고 서로 이야기 해봐야 할 것 들인데,
당장에 인증 화면이랑 연동해야해서 우선 머지할게 !
내가 배포 끝나면 같이 이야기하거나 컨벤션 맞추면 좋을 것들 정리해볼게 :)
| @Serializable | ||
| data class CreateGoalResponse( |
There was a problem hiding this comment.
서버도 코틀린을 사용하다보니 카멜 케이스를 사용하는게 똑같아서
파라미터 네이밍이 거의 같을 것 같은데 SerialName을 사용하는 것에 대해서 컨벤션을 정해보면 좋을 것 같아 !
There was a problem hiding this comment.
나중에 서버에서 변수명 변경할 수도 있어서 확장성 고려해서 다 붙여놨어요!
| import kotlinx.coroutines.flow.MutableSharedFlow | ||
| import kotlinx.coroutines.flow.SharedFlow | ||
|
|
||
| class GoalRefreshBus { |
There was a problem hiding this comment.
이 클래스의 역할이 어떤건지 설명해줄 수 있을ㄲ ㅏ?
There was a problem hiding this comment.
HomeScreen으로 진입할 때 LaunchedEffect로 매번 API를 부르면 비효율적이니까 다른 NavGraph에서 데이터가 생성되거나 삭제됐음을 알리는 용도입니다! 딱 필요할 때만 데이터를 불러오도록 돕는 역할이에요
| fun tryEmit(effect: S) { | ||
| channel.trySend(effect) | ||
| } |
There was a problem hiding this comment.
토끼 리뷰 참고해서 send와 trySend 어떤걸 사용할지 이야기 해보면 좋을 것 같아 !
나도 요거에 대해서 배포 끝나면 공부해볼태니 이야기 해보는거 어때 ?
2 participants
이슈 번호
작업내용
결과물
2026-02-09.5.40.11.mov
리뷰어에게 추가로 요구하는 사항 (선택)
지난번에 BaseViewModel onSuccess가 suspend가 아니라서 SideEffect 발생시키기 어렵다고 하셔서 tryEmit 추가했습니다! 더 좋은 방법 있으면 알려주세요