Refactor: v1.3.0 QA 수정사항 반영 - #174
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 44 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough캘린더 삭제 흐름이 확장되어 항목이 과거인지 여부를 전달받아 과거/미래를 분기 처리하고, AppResult 기반의 새로운 삭제 API( Changes
Sequence DiagramsequenceDiagram
participant UI as CalendarScreen
participant VM as CalendarViewModel
participant Repo as ScheduleRepository
participant API as Network
UI->>VM: dispatch(DeleteHistory(id, isPast))
activate VM
alt isPast == true
VM->>VM: deleteHistory(scheduleId) (기존 흐름)
else isPast == false
VM->>VM: deleteSchedule(scheduleId)
VM->>VM: 낙관적 업데이트 (selectedDateSchedules, schedulesByDate 수정)
VM->>VM: 예정 알람 취소
VM->>Repo: deleteScheduleAppResult(scheduleId)
activate Repo
Repo->>API: DELETE /schedules/{id}
API-->>Repo: 응답(성공/실패)
deactivate Repo
alt 성공
VM->>VM: SUCCESS_DELETE_SCHEDULE 토스트 발생
else 실패
VM->>VM: schedulesByDate 복원 및 알람 재적용, 에러 로깅
end
end
deactivate VM
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 3
🧹 Nitpick comments (1)
feature/calendar/src/commonMain/kotlin/com/ondot/calendar/ui/component/CalendarDay.kt (1)
109-109: 매직 넘버를 상수로 분리해 의도를 명확히 해주세요.Line 109의
take(3)자체는 문제 없지만, 같은 컴포넌트에서 dot는take(2)를 사용하고 있어 이후 QA 변경 시 누락 위험이 있습니다. 제한 개수를 상수로 분리하면 유지보수가 쉬워집니다.🔧 제안 코드
+private const val MAX_LABEL_MARKERS = 3 +private const val MAX_DOT_MARKERS = 2 ... - cell.markers.take(3).forEach { marker -> + cell.markers.take(MAX_LABEL_MARKERS).forEach { marker -> ... - cell.markers.take(2).forEach { _ -> + cell.markers.take(MAX_DOT_MARKERS).forEach { _ ->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/calendar/src/commonMain/kotlin/com/ondot/calendar/ui/component/CalendarDay.kt` at line 109, Replace the magic literal in CalendarDay.kt (the cell.markers.take(3) call) with a named constant to clarify intent and prevent future mismatches with the dot logic; define a constant (e.g., MARKER_DISPLAY_LIMIT) in the same scope as the CalendarDay composable or its companion object and use it in place of 3, and likewise replace the dot take(2) with a DOT_DISPLAY_LIMIT constant (or unify under a single constant if they should match) so both marker and dot display limits are explicit and easy to maintain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/util/src/commonMain/kotlin/com/ondot/util/DateTimeFormatter.kt`:
- Around line 328-332: Update the KDoc in DateTimeFormatter.kt to use standard
Kotlin syntax: replace lines like "@param: date 선택된 날짜" and "@param: time 선택된
시간" with "@param date 선택된 날짜" and "@param time 선택된 시간", and change "@return:
LocalDate, LocalTime을 조합한 ISO8601 문자열" to "@return LocalDate와 LocalTime을 조합한
ISO8601 문자열" so the `@param` and `@return` tags do not include colons and follow
Kotlin KDoc conventions.
In
`@feature/calendar/src/commonMain/kotlin/com/ondot/calendar/contract/CalendarViewModel.kt`:
- Around line 389-390: The code stores previousSchedulesByDate =
currentState.schedulesByDate and on error replaces the whole schedulesByDate
map, which can overwrite concurrent updates; instead capture only the prior
value for the specific date key(s) you are modifying (e.g., val previousForDate
= currentState.schedulesByDate[dateKey]) before applying changes, and on failure
restore only that entry (create a new map from currentState.schedulesByDate with
.toMutableMap(), set map[dateKey] = previousForDate or remove if it was null,
and assign it back) rather than reassigning previousSchedulesByDate; update the
rollback logic wherever currentState.schedulesByDate is fully replaced (also at
the other occurrence around lines 434-437) to use this targeted restore.
In `@feature/edit/src/commonMain/kotlin/com/ondot/edit/EditScheduleViewModel.kt`:
- Around line 86-87: The current flow calls cancelAlarms() before the network
save completes, risking alarm loss on failure; move the cancelAlarms() call to
the success branch of the save (after the API confirms the schedule update) or
implement a rollback in onFailSaveSchedule that restores previous alarms
(recreate them from the pre-save state). Locate the cancelAlarms() invocation in
EditScheduleViewModel (the save/update method) and either shift it into the
success callback where you handle the API response, or capture the existing
alarms before cancelling and use that snapshot in onFailSaveSchedule to
re-create the alarms if the API call fails.
---
Nitpick comments:
In
`@feature/calendar/src/commonMain/kotlin/com/ondot/calendar/ui/component/CalendarDay.kt`:
- Line 109: Replace the magic literal in CalendarDay.kt (the
cell.markers.take(3) call) with a named constant to clarify intent and prevent
future mismatches with the dot logic; define a constant (e.g.,
MARKER_DISPLAY_LIMIT) in the same scope as the CalendarDay composable or its
companion object and use it in place of 3, and likewise replace the dot take(2)
with a DOT_DISPLAY_LIMIT constant (or unify under a single constant if they
should match) so both marker and dot display limits are explicit and easy to
maintain.
🪄 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: 712cd39a-571e-4fe7-8b5a-ae42d124d703
📒 Files selected for processing (10)
core/util/src/commonMain/kotlin/com/ondot/util/DateTimeFormatter.ktdata/src/commonMain/kotlin/com/ondot/data/repository/ScheduleRepositoryImpl.ktdomain/src/commonMain/kotlin/com/ondot/domain/repository/ScheduleRepository.ktfeature/calendar/src/commonMain/kotlin/com/ondot/calendar/contract/CalendarIntent.ktfeature/calendar/src/commonMain/kotlin/com/ondot/calendar/contract/CalendarViewModel.ktfeature/calendar/src/commonMain/kotlin/com/ondot/calendar/ui/CalendarScreen.ktfeature/calendar/src/commonMain/kotlin/com/ondot/calendar/ui/component/CalendarBottomSheet.ktfeature/calendar/src/commonMain/kotlin/com/ondot/calendar/ui/component/CalendarDay.ktfeature/edit/src/commonMain/kotlin/com/ondot/edit/EditScheduleViewModel.ktfeature/general/src/commonMain/kotlin/com/ondot/general/check/CheckScheduleScreen.kt
💤 Files with no reviewable changes (1)
- feature/general/src/commonMain/kotlin/com/ondot/general/check/CheckScheduleScreen.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@domain/testing/src/commonMain/kotlin/com/ondot/testing/fake/FakeScheduleRepository.kt`:
- Around line 162-164: The TODO stub in
FakeScheduleRepository.deleteScheduleAppResult is invoked from production path
(CalendarViewModel -> deleteSchedule call) so replace the TODO with a minimal
fake implementation that mirrors the existing tested deleteSchedule(Flow)
behavior: perform any in-memory removal of the schedule by scheduleId and return
AppResult.Success(Unit) (or the appropriate AppResult failure when id not found)
so CalendarViewModel can exercise that path; alternatively, add a unit test in
CalendarViewModel that covers the deleteSchedule flow to justify keeping the
TODO, but preferred fix is to implement deleteScheduleAppResult in
FakeScheduleRepository to update the fake store and return a success/failure
AppResult.
🪄 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: 7f0ba56d-56f5-468a-a7e7-3c91a1d5a28a
📒 Files selected for processing (1)
domain/testing/src/commonMain/kotlin/com/ondot/testing/fake/FakeScheduleRepository.kt
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
릴리스 노트
새로운 기능
개선 사항
제거
문서화
테스트