Refactor: :feature:general MVI 전환 - #188
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
워크스루일정 생성 기능의 상태 관리를 MVVM에서 MVI 패턴으로 전환합니다. 디자인 시스템 컴포넌트 간소화부터 시작해 저장소 확장, MVI 계약 정의, 복잡한 비즈니스 로직 ViewModel, 라우팅 통합까지 단계적으로 구성됩니다. 변경 사항디자인 시스템 컴포넌트 간소화
저장소 및 도메인 레이어 확장
MVI 네비게이션 및 분석
MVI 아키텍처 계약 정의
MVI 비즈니스 로직 ViewModel
MVI 라우트 Composable 및 화면 통합
네비게이션 그래프 및 의존성 주입
코드 리뷰 예상 소요 시간🎯 4 (복잡함) | ⏱️ ~45분 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 2
🧹 Nitpick comments (5)
feature/general/src/commonMain/kotlin/com/ondot/general/navigation/GeneralScheduleMviNavGraph.kt (1)
28-99: 💤 Low value공유 ViewModel 획득 로직 중복 — 헬퍼로 추출 권장
parentEntry계산 +koinViewModel(viewModelStoreOwner = parentEntry)패턴이 3개 목적지(28-33, 51-56, 82-87)에 동일하게 반복됩니다. 목적지가 늘어날수록getBackStackEntry인자 실수 등의 위험이 커지므로 작은@Composable헬퍼로 묶는 것을 권장합니다.♻️ 제안 리팩터링
`@Composable` private fun NavGraphContributor.sharedGeneralScheduleViewModel( navController: NavHostController, backStackEntry: NavBackStackEntry, ): GeneralScheduleViewModel { val parentEntry = remember(backStackEntry) { navController.getBackStackEntry(graphRoute.route) } return koinViewModel(viewModelStoreOwner = parentEntry) }composable(NavRoutes.ScheduleRepeatSettingMvi.route) { backStackEntry -> - val parentEntry = - remember(backStackEntry) { - navController.getBackStackEntry(graphRoute.route) - } - val viewModel: GeneralScheduleViewModel = koinViewModel(viewModelStoreOwner = parentEntry) + val viewModel = sharedGeneralScheduleViewModel(navController, backStackEntry)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/general/src/commonMain/kotlin/com/ondot/general/navigation/GeneralScheduleMviNavGraph.kt` around lines 28 - 99, The repeated pattern that computes parentEntry and calls koinViewModel in the composable destinations (used in ScheduleRepeatSettingMvi, PlacePickerMvi, and CheckScheduleMvi) should be extracted into a small `@Composable` helper to avoid duplication and mistakes; add a private `@Composable` function (e.g., sharedGeneralScheduleViewModel(navController: NavHostController, backStackEntry: NavBackStackEntry): GeneralScheduleViewModel) inside the NavGraphContributor that does the remember(backStackEntry){ navController.getBackStackEntry(graphRoute.route) } and returns koinViewModel(viewModelStoreOwner = parentEntry), then replace the three inline blocks that compute parentEntry and call koinViewModel with a single call to this helper in ScheduleRepeatSettingRoute, PlacePickerRoute, and CheckScheduleRoute.domain/src/commonMain/kotlin/com/ondot/domain/repository/PlaceRepository.kt (1)
18-25: 💤 Low valueMVI 블록 내 메서드 네이밍 규칙이 일관되지 않습니다.
searchPlaceAppResult는AppResult접미사를 쓰지만, 같은 MVI 블록의deleteHistory,fetchHistory,saveHistory는 접미사가 없습니다. 동일한AppResult반환 계열인데 규칙이 섞여 있어 호출부에서 혼동을 줄 수 있습니다. 한 가지 규칙으로 통일하는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@domain/src/commonMain/kotlin/com/ondot/domain/repository/PlaceRepository.kt` around lines 18 - 25, The MVI block has inconsistent method naming: searchPlaceAppResult uses the AppResult suffix while deleteHistory, fetchHistory, and saveHistory do not; pick one convention and make them consistent (either rename searchPlaceAppResult -> searchPlace or rename deleteHistory/fetchHistory/saveHistory -> deleteHistoryAppResult/fetchHistoryAppResult/saveHistoryAppResult), then update all callers, interfaces, and tests that reference the affected symbols (searchPlaceAppResult, deleteHistory, fetchHistory, saveHistory) to the chosen names so compilation and call sites remain correct.domain/src/commonMain/kotlin/com/ondot/domain/repository/ScheduleRepository.kt (1)
46-48: 💤 Low valueFlow 기반 메서드와 기능이 중복됩니다 — 제거 계획을 명확히 해 주세요.
fetchScheduleAlarms는 기존getScheduleAlarms(Line 22)와,createScheduleAppResult는createSchedule(Line 24)과 동일한 원격 엔드포인트를 호출하는 중복 경로입니다. MVI 전환 과도기에는 공존이 불가피하지만, 전환 완료 후 레거시Flow<Result<...>>버전을 제거할 계획인지 확인이 필요합니다. 또한fetchScheduleAlarms(접미사 없음)와createScheduleAppResult(AppResult접미사)의 네이밍 규칙이 엇갈립니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@domain/src/commonMain/kotlin/com/ondot/domain/repository/ScheduleRepository.kt` around lines 46 - 48, There are duplicate API pathways and inconsistent naming between Flow-based and AppResult-based methods; decide which API style to keep (e.g., migrate fully to AppResult or Flow), remove the duplicate legacy methods after migration (either remove getScheduleAlarms/createSchedule or fetchScheduleAlarms/createScheduleAppResult), and make names consistent (rename fetchScheduleAlarms -> getScheduleAlarms or createScheduleAppResult -> createSchedule for parity). Update the repository interface methods (fetchScheduleAlarms, getScheduleAlarms, createScheduleAppResult, createSchedule), their implementations, and any callers/tests to the chosen canonical names, and add a short deprecation comment on the variants being phased out so reviewers know the removal plan.domain/testing/src/commonMain/kotlin/com/ondot/testing/fake/FakeScheduleRepository.kt (1)
155-176: ⚡ Quick win
shouldFailNetwork플래그가 새 AppResult 메서드에 반영되지 않습니다.ViewModel의
searchPlace/fetchScheduleAlarms/createSchedule은 모두onError에서 토스트 SideEffect를 방출하는데, 이 fake의fetchScheduleAlarms/createScheduleAppResult는shouldFailNetwork를 무시하고 항상 성공을 반환합니다. 그러면 실패 경로(에러 토스트)에 대한 테스트를 작성할 수 없습니다. 실패 시AppResult.Failure를 반환하도록 분기를 추가하는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@domain/testing/src/commonMain/kotlin/com/ondot/testing/fake/FakeScheduleRepository.kt` around lines 155 - 176, The fake repository methods ignore the shouldFailNetwork flag and always return success; update FakeScheduleRepository so fetchScheduleAlarms(request: ScheduleAlarmRequest) and createScheduleAppResult(request: CreateScheduleRequest) check the shouldFailNetwork flag and return AppResult.Failure when it’s true (e.g., construct a suitable error/exception or AppError) otherwise proceed with the existing success behavior; ensure you reference the shouldFailNetwork property inside those methods and return AppResult.Failure in the failure branch so tests can exercise error-toasting paths.feature/general/src/commonMain/kotlin/com/ondot/general/contract/GeneralScheduleViewModel.kt (1)
54-56: 💤 Low value
debounce(100)은 검색 API 호출에 다소 짧습니다.입력 한 글자마다 100ms 후 호출되어 불필요한 네트워크 요청이 잦을 수 있습니다. 300ms 정도로 늘리는 것을 고려해 보세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/general/src/commonMain/kotlin/com/ondot/general/contract/GeneralScheduleViewModel.kt` around lines 54 - 56, Update the debounce duration on the query Flow in GeneralScheduleViewModel by changing query.debounce(100) to query.debounce(300); locate the debounce call in the GeneralScheduleViewModel (the chain starting with "query .debounce(100) .distinctUntilChanged()") and replace 100 with 300 to reduce overly frequent search API calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@core/design-system/src/commonMain/kotlin/com/ondot/designsystem/components/DateSettingSection.kt`:
- Line 122: TimeSectionHeader와 DateSectionHeader에서 매 리컴포지션마다 새로운
MutableInteractionSource 인스턴스가 생성되고 있으니 각 컴포저블 내의 val interactionSource =
MutableInteractionSource()를 remember { MutableInteractionSource() }로 감싸서 인스턴스를
재사용하도록 변경하세요; TimeSectionHeader와 DateSectionHeader의 interactionSource 선언부를 찾아
remember로 래핑하면 리컴포지션 시 상태가 안정적으로 유지됩니다.
In
`@feature/general/src/commonMain/kotlin/com/ondot/general/contract/GeneralScheduleViewModel.kt`:
- Around line 52-70: When the query Flow emits a blank value the code only
clears placePickerState.placeList but does not cancel the ongoing search job, so
a previous search can still complete and overwrite the cleared list; in the init
block's collector (the debounce/distinctUntilChanged/onEach chain) cancel the
running search job (searchPlaceJob) when value.isBlank() before reducing state,
e.g. check if searchPlaceJob is active/null and call cancel() (and optionally
join or invokeOnCompletion) so searchPlace(q) results cannot race to update
placePickerState after you clear it; ensure the same searchPlaceJob is
used/updated inside searchPlace(...) so cancellation takes effect.
---
Nitpick comments:
In `@domain/src/commonMain/kotlin/com/ondot/domain/repository/PlaceRepository.kt`:
- Around line 18-25: The MVI block has inconsistent method naming:
searchPlaceAppResult uses the AppResult suffix while deleteHistory,
fetchHistory, and saveHistory do not; pick one convention and make them
consistent (either rename searchPlaceAppResult -> searchPlace or rename
deleteHistory/fetchHistory/saveHistory ->
deleteHistoryAppResult/fetchHistoryAppResult/saveHistoryAppResult), then update
all callers, interfaces, and tests that reference the affected symbols
(searchPlaceAppResult, deleteHistory, fetchHistory, saveHistory) to the chosen
names so compilation and call sites remain correct.
In
`@domain/src/commonMain/kotlin/com/ondot/domain/repository/ScheduleRepository.kt`:
- Around line 46-48: There are duplicate API pathways and inconsistent naming
between Flow-based and AppResult-based methods; decide which API style to keep
(e.g., migrate fully to AppResult or Flow), remove the duplicate legacy methods
after migration (either remove getScheduleAlarms/createSchedule or
fetchScheduleAlarms/createScheduleAppResult), and make names consistent (rename
fetchScheduleAlarms -> getScheduleAlarms or createScheduleAppResult ->
createSchedule for parity). Update the repository interface methods
(fetchScheduleAlarms, getScheduleAlarms, createScheduleAppResult,
createSchedule), their implementations, and any callers/tests to the chosen
canonical names, and add a short deprecation comment on the variants being
phased out so reviewers know the removal plan.
In
`@domain/testing/src/commonMain/kotlin/com/ondot/testing/fake/FakeScheduleRepository.kt`:
- Around line 155-176: The fake repository methods ignore the shouldFailNetwork
flag and always return success; update FakeScheduleRepository so
fetchScheduleAlarms(request: ScheduleAlarmRequest) and
createScheduleAppResult(request: CreateScheduleRequest) check the
shouldFailNetwork flag and return AppResult.Failure when it’s true (e.g.,
construct a suitable error/exception or AppError) otherwise proceed with the
existing success behavior; ensure you reference the shouldFailNetwork property
inside those methods and return AppResult.Failure in the failure branch so tests
can exercise error-toasting paths.
In
`@feature/general/src/commonMain/kotlin/com/ondot/general/contract/GeneralScheduleViewModel.kt`:
- Around line 54-56: Update the debounce duration on the query Flow in
GeneralScheduleViewModel by changing query.debounce(100) to query.debounce(300);
locate the debounce call in the GeneralScheduleViewModel (the chain starting
with "query .debounce(100) .distinctUntilChanged()") and replace 100 with 300 to
reduce overly frequent search API calls.
In
`@feature/general/src/commonMain/kotlin/com/ondot/general/navigation/GeneralScheduleMviNavGraph.kt`:
- Around line 28-99: The repeated pattern that computes parentEntry and calls
koinViewModel in the composable destinations (used in ScheduleRepeatSettingMvi,
PlacePickerMvi, and CheckScheduleMvi) should be extracted into a small
`@Composable` helper to avoid duplication and mistakes; add a private `@Composable`
function (e.g., sharedGeneralScheduleViewModel(navController: NavHostController,
backStackEntry: NavBackStackEntry): GeneralScheduleViewModel) inside the
NavGraphContributor that does the remember(backStackEntry){
navController.getBackStackEntry(graphRoute.route) } and returns
koinViewModel(viewModelStoreOwner = parentEntry), then replace the three inline
blocks that compute parentEntry and call koinViewModel with a single call to
this helper in ScheduleRepeatSettingRoute, PlacePickerRoute, and
CheckScheduleRoute.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c69f898-b3cd-4490-9cdf-1aa2da62bd54
📒 Files selected for processing (22)
core/design-system/src/commonMain/kotlin/com/ondot/designsystem/components/DateSettingSection.ktcore/navigation/src/commonMain/kotlin/com/ondot/navigation/AppNavHost.ktcore/navigation/src/commonMain/kotlin/com/ondot/navigation/NavRoutes.ktdata/src/commonMain/kotlin/com/ondot/data/repository/PlaceRepositoryImpl.ktdata/src/commonMain/kotlin/com/ondot/data/repository/ScheduleRepositoryImpl.ktdomain/src/commonMain/kotlin/com/ondot/domain/repository/PlaceRepository.ktdomain/src/commonMain/kotlin/com/ondot/domain/repository/ScheduleRepository.ktdomain/testing/src/commonMain/kotlin/com/ondot/testing/fake/FakeScheduleRepository.ktfeature/edit/src/commonMain/kotlin/com/ondot/edit/bottomSheet/EditDateBottomSheet.ktfeature/edit/src/commonMain/kotlin/com/ondot/edit/bottomSheet/EditTimeBottomSheet.ktfeature/general/src/commonMain/kotlin/com/ondot/general/check/CheckScheduleScreen.ktfeature/general/src/commonMain/kotlin/com/ondot/general/contract/GeneralScheduleIntent.ktfeature/general/src/commonMain/kotlin/com/ondot/general/contract/GeneralScheduleSideEffect.ktfeature/general/src/commonMain/kotlin/com/ondot/general/contract/GeneralScheduleState.ktfeature/general/src/commonMain/kotlin/com/ondot/general/contract/GeneralScheduleViewModel.ktfeature/general/src/commonMain/kotlin/com/ondot/general/di/GeneralModule.ktfeature/general/src/commonMain/kotlin/com/ondot/general/navigation/GeneralScheduleMviNavGraph.ktfeature/general/src/commonMain/kotlin/com/ondot/general/repeat/ScheduleRepeatSettingScreen.ktfeature/general/src/commonMain/kotlin/com/ondot/general/ui/check/CheckScheduleRoute.ktfeature/general/src/commonMain/kotlin/com/ondot/general/ui/place/PlacePickerRoute.ktfeature/general/src/commonMain/kotlin/com/ondot/general/ui/repeat/ScheduleRepeatSettingRoute.ktfeature/main/src/commonMain/kotlin/com/ondot/main/navigation/MainNavGraph.kt
💤 Files with no reviewable changes (3)
- feature/edit/src/commonMain/kotlin/com/ondot/edit/bottomSheet/EditTimeBottomSheet.kt
- feature/general/src/commonMain/kotlin/com/ondot/general/repeat/ScheduleRepeatSettingScreen.kt
- feature/edit/src/commonMain/kotlin/com/ondot/edit/bottomSheet/EditDateBottomSheet.kt
Merged
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
1 participant
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.
이슈 번호
작업내용
Summary by CodeRabbit
릴리스 노트
새로운 기능
개선 사항