Feat: 알람 시간 재계산 기능 구현 - #192
Merged
Merged
Conversation
Walkthrough일정 수정 흐름에 PlacePicker 및 RouteLoading 라우트를 추가하고, RouterType을 포함한 라우트 입력 콜백으로 확장합니다. General 일정 확인 화면에 알람 시간 편집 기능을 추가하고, 알람 요청에 교통수단 타입을 포함시키며, 알람 ID 유효성 검증을 도입합니다. ChangesEdit 및 General 일정 관리 흐름
알람 요청 및 검증
네비게이션 및 빌드 설정
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 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: 5
🧹 Nitpick comments (2)
feature/edit/src/commonMain/kotlin/com/ondot/edit/EditScheduleViewModel.kt (1)
66-79: 💤 Low value빈 입력 처리 로직을 개선해주세요.
Line 72의
if (value.isBlank())조건에서searchPlaceJob?.cancel()을 호출하고 있으나, 이미 진행 중인 작업이 없는 경우 불필요한 호출입니다. 또한filter { it.isNotBlank() }(Line 76)로 인해 빈 값은 이후 collect로 전달되지 않으므로, Line 72-75의 처리는 사용자가 입력을 지울 때 장소 목록을 즉시 비우기 위한 것으로 보입니다.현재 로직은 정확하지만,
onEach내부의 조건 처리와filter의 역할이 명확하지 않을 수 있습니다. 의도가 "입력이 비어있으면 진행 중인 검색을 취소하고 결과 목록을 즉시 비움"이라면 현재 구현이 적절합니다.가독성을 위해 주석 추가를 고려해보세요:
💡 주석 추가 제안
query .debounce(100) .distinctUntilChanged() .onEach { value -> + // 입력이 비어있으면 진행 중인 검색을 취소하고 목록을 즉시 비움 if (value.isBlank()) { searchPlaceJob?.cancel() updatePlacePickerState { copy(placeList = emptyList()) } } }.filter { it.isNotBlank() }🤖 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/edit/src/commonMain/kotlin/com/ondot/edit/EditScheduleViewModel.kt` around lines 66 - 79, Add a brief comment above the query pipeline (the viewModelScope.launch block that uses query.debounce/distinctUntilChanged/onEach/filter.collect) explaining the intent: that onEach handles immediate behavior for blank input (cancel any ongoing search via searchPlaceJob and clear the UI via updatePlacePickerState) while filter prevents blank values from reaching collect(::searchPlace) which runs searchPlace; also make the cancel explicit by only cancelling when searchPlaceJob is active (e.g., guard on searchPlaceJob.isActive) so intent is clear to future readers and avoid a redundant cancel call.core/design-system/src/commonMain/kotlin/com/ondot/designsystem/components/RouteInputSection.kt (1)
144-144: RouteInputSection의enabled = !readOnly동작 의도 재확인
readOnly=true는CheckScheduleScreen과EditScheduleScreen에서만 사용되며,EditScheduleScreen에서는RouteInputSection을 감싸noRippleClickable { onClickRouteInput() }로 place picker를 여는 흐름이라enabled=false가 “직접 입력 차단” 의도일 가능성이 큽니다.- 다만
CheckScheduleScreen(조회/보기)에서 텍스트 선택/복사가 필요하다면enabled=false로 상호작용이 전부 막히므로,readOnly만으로 제어하도록enabled처리(라인 143~145) 재검토가 필요합니다.🤖 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 `@core/design-system/src/commonMain/kotlin/com/ondot/designsystem/components/RouteInputSection.kt` at line 144, The current enabled = !readOnly conflates "prevent direct editing" with "block all interaction"; change RouteInputSection to accept a new Boolean parameter (e.g. allowTextSelection) and set enabled = !readOnly || allowTextSelection so read-only screens that need text selection/copy (CheckScheduleScreen) can pass allowTextSelection = true while EditScheduleScreen (which opens place picker via noRippleClickable { onClickRouteInput() }) passes false; update callers (CheckScheduleScreen, EditScheduleScreen) accordingly so EditScheduleScreen remains non-editable but clickable via noRippleClickable and CheckScheduleScreen allows selection/copy.
🤖 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 `@feature/edit/src/commonMain/kotlin/com/ondot/edit/EditRouteLoadingRoute.kt`:
- Around line 17-21: EditRouteLoadingRoute's LaunchedEffect currently navigates
immediately when EditScheduleUiState.isAlarmRecalculating is false, which can
trigger unintended navigation if fetchScheduleAlarms early-returns; fix by
either (A) ensuring fetchScheduleAlarms (called from
applyRouteChangesAndFetchAlarms in EditPlacePickerRoute) sets
updateStateSync(... isAlarmRecalculating = true) at the start and clears it only
after completion (including on early returns), or (B) tighten the LaunchedEffect
guard in EditRouteLoadingRoute to check an explicit "recalculationStarted" /
"shouldNavigateAfterRecalc" flag on EditScheduleUiState (add and set this flag
in applyRouteChangesAndFetchAlarms/fetchScheduleAlarms), and only call
navigateToEdit() when recalculationStarted is true and isAlarmRecalculating is
false; pick one approach and apply consistently to avoid the edge-case immediate
navigation.
In `@feature/edit/src/commonMain/kotlin/com/ondot/edit/EditScheduleViewModel.kt`:
- Around line 630-698: onSuccessGetScheduleAlarms currently overwrites
result.preparationAlarm/ departureAlarm alarmId with
originalSchedule.*Alarm.alarmId unconditionally, which can hide cases where
original IDs are <= 0 and later filtered out by
DefaultScheduleAlarmManager.validAlarmInfos; update onSuccessGetScheduleAlarms
to preserve originalSchedule alarmId only if it is > 0, otherwise use
result.*Alarm.alarmId (i.e., set alarmId = if
(originalSchedule.preparationAlarm.alarmId > 0)
originalSchedule.preparationAlarm.alarmId else result.preparationAlarm.alarmId
and similarly for departureAlarm), and/or add an assertion/log to ensure
originalSchedule exists and its IDs are validated before calling
DefaultScheduleAlarmManager.validAlarmInfos so invalid (<=0) IDs aren’t silently
kept.
In
`@feature/general/src/commonMain/kotlin/com/ondot/general/check/CheckScheduleScreen.kt`:
- Around line 218-241: The APPOINTMENT branch in CheckScheduleScreen.kt
incorrectly maps TimeType.APPOINTMENT to uiState.departureAlarm while
GeneralScheduleViewModel.updateAlarmTime ignores APPOINTMENT; either
remove/guard the APPOINTMENT branch so it cannot be executed (e.g., only let
PREPARATION/DEPARTURE open GeneralAlarmTimeBottomSheet) or update the ViewModel
(updateAlarmTime) and the alarm mapping so APPOINTMENT is handled consistently;
additionally, make calls to alarm.triggeredAt.toLocalDateFromIso() /
toLocalTimeFromIso() defensive by wrapping parsing via the DateTimeFormatter
helpers (or try-catch around those calls) and provide a safe fallback/null so
the UI does not crash on empty/invalid ISO strings and the bottom sheet is not
opened with invalid values.
In
`@feature/general/src/commonMain/kotlin/com/ondot/general/contract/GeneralScheduleViewModel.kt`:
- Around line 493-519: The updateAlarmTime function currently ignores
TimeType.APPOINTMENT, causing selected times not to be applied while the UI may
map APPOINTMENT to departureAlarm; either (A) handle APPOINTMENT inside
updateAlarmTime by treating it like DEPARTURE (update departureAlarm.triggeredAt
and clear activeAlarmTimeBottomSheet) to match CheckScheduleScreen's mapping, or
(B) enforce a defensive contract so activeAlarmTimeBottomSheet can only be
PREPARATION or DEPARTURE (validate in the code path that opens the bottom sheet
and in UpdateAlarmTime dispatch), adding a clear error/log or early return for
unexpected TimeType values; locate and modify
GeneralScheduleViewModel.updateAlarmTime, references to
activeAlarmTimeBottomSheet, and the code that opens the bottom sheet/dispatches
UpdateAlarmTime to implement the chosen fix.
In
`@feature/general/src/commonMain/kotlin/com/ondot/general/GeneralScheduleViewModel.kt`:
- Line 429: GeneralScheduleViewModel currently hardcodes transportType to
TransportType.PUBLIC_TRANSPORT.name and does not wire PlacePickerRoute's
onTransportTypeChanged callback, so user selection isn't applied; fix by
connecting PlacePickerRoute(onTransportTypeChanged = {
viewModel.placePickerState.selectedTransportType = it }) (or call the viewModel
updater) so the placePickerState.selectedTransportType is updated when the user
changes it, and update the request builders in GeneralScheduleViewModel that
create ScheduleAlarmRequest/CreateScheduleRequest to use
uiState.placePickerState.selectedTransportType.name instead of the hardcoded
TransportType.PUBLIC_TRANSPORT.name; ensure you reference the placePickerState,
onTransportTypeChanged, ScheduleAlarmRequest, CreateScheduleRequest, and
GeneralScheduleViewModel symbols when making the changes.
---
Nitpick comments:
In
`@core/design-system/src/commonMain/kotlin/com/ondot/designsystem/components/RouteInputSection.kt`:
- Line 144: The current enabled = !readOnly conflates "prevent direct editing"
with "block all interaction"; change RouteInputSection to accept a new Boolean
parameter (e.g. allowTextSelection) and set enabled = !readOnly ||
allowTextSelection so read-only screens that need text selection/copy
(CheckScheduleScreen) can pass allowTextSelection = true while
EditScheduleScreen (which opens place picker via noRippleClickable {
onClickRouteInput() }) passes false; update callers (CheckScheduleScreen,
EditScheduleScreen) accordingly so EditScheduleScreen remains non-editable but
clickable via noRippleClickable and CheckScheduleScreen allows selection/copy.
In `@feature/edit/src/commonMain/kotlin/com/ondot/edit/EditScheduleViewModel.kt`:
- Around line 66-79: Add a brief comment above the query pipeline (the
viewModelScope.launch block that uses
query.debounce/distinctUntilChanged/onEach/filter.collect) explaining the
intent: that onEach handles immediate behavior for blank input (cancel any
ongoing search via searchPlaceJob and clear the UI via updatePlacePickerState)
while filter prevents blank values from reaching collect(::searchPlace) which
runs searchPlace; also make the cancel explicit by only cancelling when
searchPlaceJob is active (e.g., guard on searchPlaceJob.isActive) so intent is
clear to future readers and avoid a redundant cancel call.
🪄 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: 2b334b33-c753-4a5e-a775-c4638c208966
📒 Files selected for processing (30)
build-logic/src/main/java/com/ondot/build_logic/convention/ComposeMultiplatformConventionPlugin.ktbuild-logic/src/main/java/com/ondot/build_logic/convention/internal/Extensions.ktcore/design-system/src/commonMain/kotlin/com/ondot/designsystem/components/RouteInputSection.ktcore/design-system/src/commonMain/kotlin/com/ondot/designsystem/preview/OnDotPreview.ktcore/navigation/src/commonMain/kotlin/com/ondot/navigation/AppNavHost.ktcore/navigation/src/commonMain/kotlin/com/ondot/navigation/NavRoutes.ktcore/ui/src/commonMain/kotlin/com/ondot/ui/screen/loading/RouteLoadingScreen.ktcore/ui/src/commonMain/kotlin/com/ondot/ui/screen/placepicker/PlacePicker.ktcore/util/src/commonMain/kotlin/com/ondot/util/DefaultScheduleAlarmManager.ktdomain/src/commonMain/kotlin/com/ondot/domain/model/request/ScheduleAlarmRequest.ktfeature/edit/src/commonMain/kotlin/com/ondot/edit/EditPlacePickerRoute.ktfeature/edit/src/commonMain/kotlin/com/ondot/edit/EditRouteLoadingRoute.ktfeature/edit/src/commonMain/kotlin/com/ondot/edit/EditScheduleEvent.ktfeature/edit/src/commonMain/kotlin/com/ondot/edit/EditScheduleScreen.ktfeature/edit/src/commonMain/kotlin/com/ondot/edit/EditScheduleUiState.ktfeature/edit/src/commonMain/kotlin/com/ondot/edit/EditScheduleViewModel.ktfeature/edit/src/commonMain/kotlin/com/ondot/edit/navigation/EditScheduleNavGraph.ktfeature/general/src/commonMain/kotlin/com/ondot/general/GeneralScheduleUiState.ktfeature/general/src/commonMain/kotlin/com/ondot/general/GeneralScheduleViewModel.ktfeature/general/src/commonMain/kotlin/com/ondot/general/check/CheckScheduleScreen.ktfeature/general/src/commonMain/kotlin/com/ondot/general/check/GeneralAlarmTimeBottomSheet.ktfeature/general/src/commonMain/kotlin/com/ondot/general/contract/GeneralScheduleIntent.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/place/PlacePickerScreen.ktfeature/general/src/commonMain/kotlin/com/ondot/general/ui/check/CheckScheduleRoute.ktfeature/general/src/commonMain/kotlin/com/ondot/general/ui/place/PlacePickerRoute.ktgradle.propertiesgradle/libs.versions.tomliosApp/iosApp.xcodeproj/project.pbxproj
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
릴리스 노트
New Features
Bug Fixes
Chores