fix/ #164 QA 수정 완료 #165
Open
fix/ #164 QA 수정 완료 #165
Conversation
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
wotjs020708
requested review from
soseoyo12,
sum130 and
y-eonee
and removed request for
a team
Open
1 task
📝 WalkthroughWalkthroughSelectTreatment과 Treatment 관련 뷰들의 레이아웃 조정(텍스트 프레임 높이, 스페이서/스크롤뷰 높이 계산)과 함께 NoTreatment/Treatment 뷰모델의 날짜 유효성 검사(isDateTextFieldNotEmpty) 로직이 오늘(today) 및 scheduledDate 비교를 명확히 하도록 수정되었습니다. 동작 흐름 변경은 경계 검사 위주입니다. Changes
Sequence Diagram(s)(생략) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/Treatment/TreatmentFilterView.swift (1)
85-88:.task내부 중첩 Task 제거
.task안에서 새로운Task {}를 생성하면 해당 Task는 구조화되지 않은 최상위 Task가 되어.task의 취소 신호를 받지 않습니다. 뷰가 사라질 때 외부.task는 취소되지만 내부Task는 계속 실행되어 불필요한 호출이 남을 수 있습니다..task블록에서 직접await를 호출하여 구조화된 동시성을 유지해주세요.🔧 제안 수정안
- .task { - Task { - try await viewModel.fetchTreatments() - } - } + .task { + try? await viewModel.fetchTreatments() + }
🤖 Fix all issues with AI agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/NoTreatment/NoTreatmentFilterView.swift`:
- Around line 14-22: The computed property scrollViewHeight currently sums
values where itemHeight and spacing already use adjustedH, but elsewhere the
value is being re-scaled again (scrollViewHeight.adjustedH); fix by ensuring
adjustedH is applied exactly once: keep adjustedH on the base constants
(itemHeight, spacing, any fixed paddings like 24 and 40) and remove any extra
.adjustedH call where scrollViewHeight is used (or alternatively remove
adjustedH from the constants and apply it once to scrollViewHeight) so
scrollViewHeight calculation in NoTreatmentFilterView and its usage uses a
single scaling application; locate symbols itemHeight, spacing, scrollViewHeight
and any usages of scrollViewHeight.adjustedH to update consistently.
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/Treatment/TreatmentFilterView.swift`:
- Around line 13-21: The computed property scrollViewHeight already uses values
with .adjustedH applied (itemHeight, spacing and the added paddings), so remove
the extra scaling where scrollViewHeight is consumed (do not call
scrollViewHeight.adjustedH); update usages (e.g., where scrollViewHeight is
referenced in layout code around the previous Line 78–81) to use
scrollViewHeight directly, or alternatively compute itemHeight/spacing without
.adjustedH and apply .adjustedH only once—ensure scrollViewHeight remains
consistently scaled exactly once.
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/ViewModel/NoTreatment/NoTreatmentViewModel.swift`:
- Around line 141-156: The inner check for `date < today` inside the `else`
branch is unreachable because the `else` guarantees `date >= today`; simplify
the branching in NoTreatmentViewModel by removing that impossible condition and
collapsing logic to: if `date < today` -> call `updateWarning(state: .pastDate)`
and return false; else if `date < scheduledDate` -> call `updateWarning(state:
.beforeProcedureDate)` and return false; keep references to `date`, `today`,
`scheduledDate`, and `updateWarning(state:)` intact so behavior remains the same
but with the unreachable branch removed.
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/ViewModel/Treatment/TreatmentViewModel.swift`:
- Around line 132-147: The else branch contains an unreachable check (date <
today) and should be simplified: in the else (where date >= today) remove the
nested "if date < today" and, when date < scheduledDate, call
updateWarning(state: .beforeProcedureDate) and return false; keep the earlier
branch that sets updateWarning(state: .pastDate) when date < today && date <
scheduledDate so .pastDate logic remains intact. Reference: variables date,
today, scheduledDate and the updateWarning(state:
.pastDate)/updateWarning(state: .beforeProcedureDate) calls in
TreatmentViewModel.
Comment on lines
+14
to
+22
| private let itemHeight: CGFloat = 34.adjustedH | ||
| private let spacing: CGFloat = 8.adjustedH | ||
| private let maxVisibleCount = 3 | ||
|
|
||
| private var scrollViewHeight: CGFloat { | ||
| let count = min(viewModel.selectedTreatments.count, maxVisibleCount) | ||
| let contentHeight = CGFloat(count) * itemHeight + CGFloat(max(count - 1, 0)) * spacing | ||
| return contentHeight + 24.adjustedH + 40.adjustedH | ||
| } |
There was a problem hiding this comment.
scrollViewHeight 이중 스케일링 가능성
Line 19-21에서 adjustedH가 이미 적용된 값을 계산하고 있는데, Line 53에서 scrollViewHeight.adjustedH를 다시 적용하고 있습니다. adjustedH가 스케일링이라면 높이가 과대 계산될 수 있으니 한 번만 적용해주세요.
🔧 제안 수정안
- 24.adjustedH : scrollViewHeight.adjustedH + 24.adjustedH
+ 24.adjustedH : scrollViewHeight + 24.adjustedHAlso applies to: 51-54
🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/NoTreatment/NoTreatmentFilterView.swift`
around lines 14 - 22, The computed property scrollViewHeight currently sums
values where itemHeight and spacing already use adjustedH, but elsewhere the
value is being re-scaled again (scrollViewHeight.adjustedH); fix by ensuring
adjustedH is applied exactly once: keep adjustedH on the base constants
(itemHeight, spacing, any fixed paddings like 24 and 40) and remove any extra
.adjustedH call where scrollViewHeight is used (or alternatively remove
adjustedH from the constants and apply it once to scrollViewHeight) so
scrollViewHeight calculation in NoTreatmentFilterView and its usage uses a
single scaling application; locate symbols itemHeight, spacing, scrollViewHeight
and any usages of scrollViewHeight.adjustedH to update consistently.
Comment on lines
+13
to
+21
| private let itemHeight: CGFloat = 34.adjustedH | ||
| private let spacing: CGFloat = 8.adjustedH | ||
| private let maxVisibleCount = 3 | ||
|
|
||
| private var scrollViewHeight: CGFloat { | ||
| let count = min(viewModel.selectedTreatments.count, maxVisibleCount) | ||
| let contentHeight = CGFloat(count) * itemHeight + CGFloat(max(count - 1, 0)) * spacing | ||
| return contentHeight + 24.adjustedH + 40.adjustedH | ||
| } |
There was a problem hiding this comment.
scrollViewHeight 재스케일링으로 높이 과대 가능
Line 18-21에서 이미 adjustedH가 적용된 값을 만들고 있는데, Line 80에서 scrollViewHeight.adjustedH를 다시 적용하고 있습니다. 스케일링 중복을 제거해 주세요.
🔧 제안 수정안
- 24.adjustedH : scrollViewHeight.adjustedH + 24.adjustedH
+ 24.adjustedH : scrollViewHeight + 24.adjustedHAlso applies to: 78-81
🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/Treatment/TreatmentFilterView.swift`
around lines 13 - 21, The computed property scrollViewHeight already uses values
with .adjustedH applied (itemHeight, spacing and the added paddings), so remove
the extra scaling where scrollViewHeight is consumed (do not call
scrollViewHeight.adjustedH); update usages (e.g., where scrollViewHeight is
referenced in layout code around the previous Line 78–81) to use
scrollViewHeight directly, or alternatively compute itemHeight/spacing without
.adjustedH and apply .adjustedH only once—ensure scrollViewHeight remains
consistently scaled exactly once.
Comment on lines
141
to
+156
| let today = Calendar.current.startOfDay(for: Date()) | ||
|
|
||
| if date < today { | ||
| updateWarning(state: .pastDate) | ||
| return false | ||
| if date < scheduledDate { | ||
| updateWarning(state: .pastDate) | ||
| return false | ||
| } | ||
|
|
||
| } else { | ||
| if date < scheduledDate { | ||
| if date < today { | ||
| updateWarning(state: .pastDate) | ||
| } | ||
| updateWarning(state: .beforeProcedureDate) | ||
| return false | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
도달 불가능한 분기 제거로 경고 로직 단순화
Line 149의 else는 date >= today를 보장하므로 Line 151의 date < today는 항상 false입니다. 현재 구조는 경고 업데이트 의도를 흐릴 수 있어 분기 정리를 권장합니다.
🔧 제안 수정안
- if date < today {
- if date < scheduledDate {
- updateWarning(state: .pastDate)
- return false
- }
-
- } else {
- if date < scheduledDate {
- if date < today {
- updateWarning(state: .pastDate)
- }
- updateWarning(state: .beforeProcedureDate)
- return false
- }
- }
+ if date < today {
+ if date < scheduledDate {
+ updateWarning(state: .pastDate)
+ return false
+ }
+ } else if date < scheduledDate {
+ updateWarning(state: .beforeProcedureDate)
+ return false
+ }🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/ViewModel/NoTreatment/NoTreatmentViewModel.swift`
around lines 141 - 156, The inner check for `date < today` inside the `else`
branch is unreachable because the `else` guarantees `date >= today`; simplify
the branching in NoTreatmentViewModel by removing that impossible condition and
collapsing logic to: if `date < today` -> call `updateWarning(state: .pastDate)`
and return false; else if `date < scheduledDate` -> call `updateWarning(state:
.beforeProcedureDate)` and return false; keep references to `date`, `today`,
`scheduledDate`, and `updateWarning(state:)` intact so behavior remains the same
but with the unreachable branch removed.
Comment on lines
132
to
+147
| let today = Calendar.current.startOfDay(for: Date()) | ||
|
|
||
| if date < today { | ||
| updateWarning(state: .pastDate) | ||
| return false | ||
| if date < scheduledDate { | ||
| updateWarning(state: .pastDate) | ||
| return false | ||
| } | ||
|
|
||
| } else { | ||
| if date < scheduledDate { | ||
| if date < today { | ||
| updateWarning(state: .pastDate) | ||
| } | ||
| updateWarning(state: .beforeProcedureDate) | ||
| return false | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
도달 불가능한 분기 제거로 경고 로직 단순화
Line 140의 else는 date >= today 조건이므로 Line 142의 date < today는 항상 false입니다. 분기를 단순화하면 의도가 명확해집니다.
🔧 제안 수정안
- if date < today {
- if date < scheduledDate {
- updateWarning(state: .pastDate)
- return false
- }
-
- } else {
- if date < scheduledDate {
- if date < today {
- updateWarning(state: .pastDate)
- }
- updateWarning(state: .beforeProcedureDate)
- return false
- }
- }
+ if date < today {
+ if date < scheduledDate {
+ updateWarning(state: .pastDate)
+ return false
+ }
+ } else if date < scheduledDate {
+ updateWarning(state: .beforeProcedureDate)
+ return false
+ }🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/ViewModel/Treatment/TreatmentViewModel.swift`
around lines 132 - 147, The else branch contains an unreachable check (date <
today) and should be simplified: in the else (where date >= today) remove the
nested "if date < today" and, when date < scheduledDate, call
updateWarning(state: .beforeProcedureDate) and return false; keep the earlier
branch that sets updateWarning(state: .pastDate) when date < today && date <
scheduledDate so .pastDate logic remains intact. Reference: variables date,
today, scheduledDate and the updateWarning(state:
.pastDate)/updateWarning(state: .beforeProcedureDate) calls in
TreatmentViewModel.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🔗 연결된 이슈
📄 작업 내용