Conversation
- MviViewModel<STATE, INTENT, EFFECT> 베이스 클래스 생성 (reduce, postEffect, collectFlow) - MyPageContract 정의 (MyPageUiState, MyPageIntent, MyPageEffect) - MyPageViewModel: LiveData 7개 → StateFlow 단일 UiState로 통합 - MyPageFragment: observe 2개 + XML 바인딩 5개 → collectLatest + bindState()로 전환 - fragment_my_page.xml: DataBinding 표현식 제거, 코드로 이동
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds an MVI foundation ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt (1)
118-122: KeepbindState()free of follow-up state writes.
UpdateProfileImgis derived entirely from the current stamp, so sending it from the renderer forces a second state emission after every successful load. Prefer deriving the drawable when binding, or publish the final image state before callingbindState().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt` around lines 118 - 122, bindState() is mutating state by calling viewModel.intent(MyPageIntent.UpdateProfileImg(...)) after loading the profile image; instead avoid follow-up state writes in the renderer: derive the drawable locally in bindState() using getStampResourceId() and set the ImageView directly, or move the call to viewModel.intent(UpdateProfileImg(stampResId)) out of bindState() to the place where the stamp is selected/confirmed so the ViewModel publishes the final image state before bindState() runs (refer to bindState(), getStampResourceId(), MyPageIntent.UpdateProfileImg and viewModel.intent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/runnect/runnect/presentation/base/MviViewModel.kt`:
- Around line 28-29: The effect SharedFlow (_effect: MutableSharedFlow<EFFECT>
and public effect: SharedFlow<EFFECT>) is never collected in the UI so posted
effects like MyPageEffect.ShowError are dropped; in the Fragment that owns
MyPageViewModel (e.g., MyPageFragment) add a lifecycle-aware collector using the
existing repeatOnStarted pattern to collect viewModel.effect and dispatch each
emission to your effect handler (e.g., call handleEffect(effect) or the existing
effect routing method); keep the MutableSharedFlow as-is and ensure collection
is started in onViewCreated (or equivalent) so effects are received only while
the UI is active.
In `@app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt`:
- Around line 101-107: The current addObserver only collects viewModel.state and
calls bindState, so failures reported via state.error or via
MyPageEffect.ShowError from loadUserInfo() are never surfaced; update the
repeatOnStarted block in addObserver to also collect viewModel.effect (or
effects flow) with collectLatest and handle MyPageEffect.ShowError by invoking
the fragment's error UI (toast/dialog) and/or a method like showError(message),
and change bindState (or its caller) to render an explicit error view when
state.error is non-null instead of falling back to the empty default; ensure
both the state.error path and MyPageEffect.ShowError are consumed so spinner
removal shows the error UI consistently.
---
Nitpick comments:
In `@app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt`:
- Around line 118-122: bindState() is mutating state by calling
viewModel.intent(MyPageIntent.UpdateProfileImg(...)) after loading the profile
image; instead avoid follow-up state writes in the renderer: derive the drawable
locally in bindState() using getStampResourceId() and set the ImageView
directly, or move the call to viewModel.intent(UpdateProfileImg(stampResId)) out
of bindState() to the place where the stamp is selected/confirmed so the
ViewModel publishes the final image state before bindState() runs (refer to
bindState(), getStampResourceId(), MyPageIntent.UpdateProfileImg and
viewModel.intent).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a2d1cd7-b9bf-4ab4-b00a-6234b54221a8
📒 Files selected for processing (5)
app/src/main/java/com/runnect/runnect/presentation/base/MviViewModel.ktapp/src/main/java/com/runnect/runnect/presentation/mypage/MyPageContract.ktapp/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.ktapp/src/main/java/com/runnect/runnect/presentation/mypage/MyPageViewModel.ktapp/src/main/res/layout/fragment_my_page.xml
| private val _effect = MutableSharedFlow<EFFECT>() | ||
| val effect: SharedFlow<EFFECT> = _effect.asSharedFlow() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Kotlin Coroutines, what happens to values emitted to a default MutableSharedFlow() before any collector is active, and what pattern is recommended for one-off UI effects?
💡 Result:
In a default MutableSharedFlow (replay=0, extraBufferCapacity=0, onBufferOverflow=BufferOverflow.SUSPEND), values emitted before any collector is active are dropped/lost. The emit call returns immediately without suspending, as there is no buffer and no subscribers; the value is simply not stored. For one-off UI effects (e.g., toasts, navigation), the recommended pattern is to use MutableSharedFlow with replay=0 (to avoid replaying to new collectors after configuration changes) exposed as SharedFlow, collected in UI with lifecycle-aware scopes like repeatOnLifecycle(Lifecycle.State.STARTED) or LaunchedEffect. Some sources suggest Channel + receiveAsFlow for guaranteed delivery via buffering, but official docs and common practice favor SharedFlow configured with replay=0 for events.
Citations:
- 1: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/-mutable-shared-flow.html
- 2: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/-shared-flow/
- 3: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/-mutable-shared-flow/emit.html
- 4: https://medium.com/@yassine.sayah/default-vs-buffered-sharedflow-in-kotlin-a-practical-guide-6abe92aad7d7
- 5: https://www.kodeco.com/22030171-reactive-streams-on-kotlin-sharedflow-and-stateflow/page/2
- 6: https://coldfusion-example.blogspot.com/2026/02/kotlin-coroutines-safely-handling-one.html
- 7: https://blog.gouthamreddy.com/shareflows-in-android/
- 8: https://stackoverflow.com/questions/71033789/how-to-handle-one-shot-operations-in-jetpack-compose/71036376
- 9: https://developer.android.com/kotlin/flow/stateflow-and-sharedflow
🏁 Script executed:
# Get the file path and read relevant sections
fd -t f "MviViewModel.kt" --path "*/presentation/base/*"Repository: Runnect/Runnect-Android
Length of output: 300
🏁 Script executed:
# Read the MviViewModel.kt file to understand context around lines 28-29 and 50-53
cat -n app/src/main/java/com/runnect/runnect/presentation/base/MviViewModel.kt | sed -n '1,70p'Repository: Runnect/Runnect-Android
Length of output: 2735
🏁 Script executed:
# Search for where effect is collected in the codebase to see if lifecycle-aware collection is used
rg "\.effect\b" --type kotlin -A 3 -B 3 | head -100Repository: Runnect/Runnect-Android
Length of output: 49
🏁 Script executed:
# Search for effect collection pattern in the codebase
rg "\.effect\s*\." --type kotlin -B 2 -A 5Repository: Runnect/Runnect-Android
Length of output: 49
🏁 Script executed:
# Search for LaunchedEffect or repeatOnLifecycle patterns with effect
rg "LaunchedEffect|repeatOnLifecycle" --type kotlin -B 2 -A 5 | head -80Repository: Runnect/Runnect-Android
Length of output: 2099
🏁 Script executed:
# Look for specific MyPage files mentioned in the comment to see how effects are collected
fd "MyPage" --type f -e kt -e java | head -20Repository: Runnect/Runnect-Android
Length of output: 482
🏁 Script executed:
# Check MyPageFragment to see how effect is collected
cat -n app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt | head -150Repository: Runnect/Runnect-Android
Length of output: 7228
🏁 Script executed:
# Check MyPageViewModel to see what effects it posts
cat -n app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageViewModel.kt | head -100Repository: Runnect/Runnect-Android
Length of output: 2166
🏁 Script executed:
# Get the full MyPageFragment to see if effect is collected
cat -n app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt | tail -100Repository: Runnect/Runnect-Android
Length of output: 4430
🏁 Script executed:
# Search for any effect collection in MyPageFragment
rg "viewModel\.effect" app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt -A 5Repository: Runnect/Runnect-Android
Length of output: 49
🏁 Script executed:
# Check MyPageContract to see what effects are defined
cat app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageContract.ktRepository: Runnect/Runnect-Android
Length of output: 832
Collect effects in the UI layer with lifecycle awareness.
The effect flow is defined but never collected in MyPageFragment. Effects like MyPageEffect.ShowError are posted but will be lost if no collector is active. Add effect collection in the Fragment using the existing repeatOnStarted pattern: repeatOnStarted { viewModel.effect.collect { effect -> handleEffect(effect) } }. A default MutableSharedFlow() is appropriate for MVI when effects are properly collected with lifecycle-aware scopes; no need for Channel.
Also applies to: 50-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/com/runnect/runnect/presentation/base/MviViewModel.kt`
around lines 28 - 29, The effect SharedFlow (_effect: MutableSharedFlow<EFFECT>
and public effect: SharedFlow<EFFECT>) is never collected in the UI so posted
effects like MyPageEffect.ShowError are dropped; in the Fragment that owns
MyPageViewModel (e.g., MyPageFragment) add a lifecycle-aware collector using the
existing repeatOnStarted pattern to collect viewModel.effect and dispatch each
emission to your effect handler (e.g., call handleEffect(effect) or the existing
effect routing method); keep the MutableSharedFlow as-is and ensure collection
is started in onViewCreated (or equivalent) so effects are received only while
the UI is active.
There was a problem hiding this comment.
현재 코드 유지
postEffect()는viewModelScope.launch에서emit()하므로 ViewModel이 살아있는 한 emit 자체는 실행됨- Fragment는
repeatOnStarted로 collect하므로, Fragment가 STARTED 이상일 때만 수신 — lifecycle-safe extraBufferCapacity = 1은 effect가 "fire-and-forget" 성격일 때 적합하지만, 현재 effect가 에러 토스트뿐이고 이미state.error로도 에러를 보여주고 있어 effect 미수신 시 실질적 문제 없음- SharedFlow 공식 문서:
replay=0, extraBufferCapacity=0이 default이며, 이는 collector가 없으면 emit이 suspend되는 동작 — drop이 아님 - 추후 effect 종류가 늘어나면 그때
extraBufferCapacity추가 검토
There was a problem hiding this comment.
@unam98, 결정 존중합니다. 그리고 공식 문서 인용을 통한 중요한 정정 감사합니다.
제 원래 코멘트에서 "dropped"라고 표현한 것은 부정확했습니다. MutableSharedFlow의 기본 emit()은 collector가 없을 때 suspend 동작이며 (drop은 tryEmit()의 동작), viewModelScope.launch { _effect.emit(effect) }로 실행되므로 Fragment가 STARTED 상태가 되어 collect를 시작하면 해당 emission을 수신할 수 있습니다. 현재 구조에서 충분히 합리적입니다.
( ˘▾˘)~♪
✏️ Learnings added
Learnt from: unam98
Repo: Runnect/Runnect-Android PR: 373
File: app/src/main/java/com/runnect/runnect/presentation/base/MviViewModel.kt:25-26
Timestamp: 2026-04-02T17:59:43.992Z
Learning: In the MVI base class `MviViewModel.kt` (`app/src/main/java/com/runnect/runnect/presentation/base/MviViewModel.kt`), `postEffect()` uses `viewModelScope.launch { _effect.emit(effect) }` with the default `MutableSharedFlow(replay=0, extraBufferCapacity=0)`. The team intentionally keeps this configuration because: (1) `emit()` suspends (not drops) when no collector is active, (2) the Fragment collects with `repeatOnStarted` for lifecycle safety, and (3) current effects (ShowError) are also mirrored in `state.error`, making missed effects non-critical. `extraBufferCapacity` will be revisited if more effect types are added.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt
Show resolved
Hide resolved
| reduce { | ||
| copy( |
There was a problem hiding this comment.
이 파일에서 전반적으로 reduce랑 copy가 많이 쓰이는데 역할이 뭐고 이유가 뭔지?
There was a problem hiding this comment.
MVI에서 상태 변경은 반드시 reduce를 통해서만 이루어집니다.
reduce { copy(nickname = "새이름") }는 현재 상태를 기반으로 변경된 부분만 교체한 새 객체를 만드는 패턴copy()는 Kotlin data class의 기본 기능 — 불변 상태 유지하면서 특정 필드만 변경- 이렇게 하면 상태 변경 경로가 단일화되어, 어디서 상태가 바뀌었는지 추적이 쉬워짐 (LiveData처럼 여러 곳에서
.value =하는 것과 대비) - Kotlin data class copy 공식 문서
There was a problem hiding this comment.
reduce랑 copy는 세트인가? 그렇다면 이걸 하나로 합쳐서 편하게 쓸 수 있는 api를 하나 만들면 좋을 듯
There was a problem hiding this comment.
현재 reduce가 이미 그 역할입니다.
// reduce 하나로 "현재 상태에서 이 필드만 바꿔" 를 표현
reduce { copy(nickname = "새이름") }reduce는 베이스 클래스의 APIcopy는 Kotlin data class의 기본 기능 (별도 구현 아님)- 둘을 더 합치려면
updateState(nickname = "새이름")같은 형태가 되는데, data class 필드가 화면마다 다르므로 제네릭하게 만들 수 없음 - 결론: 현재
reduce { copy(...) }가 가장 간결한 형태입니다
app/src/main/java/com/runnect/runnect/presentation/mypage/MyPageFragment.kt
Show resolved
Hide resolved
| companion object { | ||
| const val STAMP_LOCK = "lock" | ||
| } |
There was a problem hiding this comment.
여기서 단순히 매직 리터럴을 없애겠단 이유 말고도 companion object를 쓴 이유가 있는지? 메모리 효율에 대한 고려는 했는지?
There was a problem hiding this comment.
companion object 안의 const val은 컴파일 타임에 인라인되어 메모리에 별도 객체를 만들지 않습니다.
const val STAMP_LOCK = "lock"은 바이트코드에서 사용처에 직접"lock"리터럴로 치환됨- 일반
val이었다면 companion object 인스턴스가 힙에 올라가겠지만,const는 그렇지 않음 - Kotlin const val 공식 문서
여기서 companion을 쓴 이유는 단순히 data class 바깥에 상수를 두면서도 MyPageUiState.STAMP_LOCK으로 접근할 수 있게 스코핑한 것입니다.
There was a problem hiding this comment.
팩트 체크는 안 해봤는데 너가 한 말에 따르면 아래와 같이 이해했는데 맞아?
- val : 일반적으로 스택에 쌓임
- companion object 안의 val : 싱글턴이라 힙에 쌓임
- companion object 안의 const val : 메모리 할당 안 됨
There was a problem hiding this comment.
거의 맞는데 하나만 정정:
val(로컬 변수): 스택에 할당 ✅val(클래스 프로퍼티): 힙에 할당 (객체의 일부이므로)companion object안의val: companion object 인스턴스가 힙에 올라가고, 그 안의val도 힙 ✅companion object안의const val: 컴파일 타임에 사용처에 인라인 → 런타임 메모리 할당 없음 ✅
정리하면 val의 위치(로컬 vs 클래스 프로퍼티)에 따라 스택/힙이 달라지고, const val만 인라인되어 메모리 할당이 없습니다.
app/src/main/java/com/runnect/runnect/presentation/base/MviViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/runnect/runnect/presentation/base/MviViewModel.kt
Outdated
Show resolved
Hide resolved
- handleException: 불필요한 when 분기 제거, Timber.e() 단일 호출로 통합 - MyPageFragment.bindState(): error 상태 시 Snackbar 표시 추가
- Google Android 공식 샘플 컨벤션에 맞춰 PascalCase로 변경 - STATE → State, INTENT → Intent, EFFECT → Effect
1 participant
작업 배경
변경 사항
MviViewModel.ktMviViewModel<STATE, INTENT, EFFECT>베이스 클래스 —reduce(),postEffect(),collectFlow()제공MyPageContract.ktMyPageUiState(단일 data class) +MyPageIntent(sealed interface) +MyPageEffect정의MyPageViewModel.ktStateFlow<MyPageUiState>단일 스트림,collectResult→collectFlowMyPageFragment.ktobserve()2개 →repeatOnStarted { state.collectLatest { bindState(it) } }, DataBinding 참조 제거fragment_my_page.xml<data>블록 비움, 바인딩 표현식 5개 제거 (코드로 이동)영향 범위
MviViewModel은 다른 화면에서 재사용할 베이스 클래스Test Plan
🤖 Generated with Claude Code
Summary by CodeRabbit