Conversation
wotjs020708
requested review from
soseoyo12,
sum130 and
y-eonee
and removed request for
a team
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthrough캘린더 네비게이션에 Changes
Sequence DiagramsequenceDiagram
participant User
participant CalendarView
participant Coordinator as CalendarCoordinator
participant Select as SelectTreatmentView
participant Treatment as TreatmentView
participant NoT as NoTreatmentView
participant TabBar as TabBarCoordinator
User->>CalendarView: 플러스 또는 빈 버튼 탭
CalendarView->>Coordinator: push(.selectTreatment)
Coordinator->>Select: present SelectTreatmentView
Select->>TabBar: isTabbarHidden = true (onAppear)
User->>Select: 시술 선택 후 "다음" 탭
alt 선택됨 (Available)
Select->>Coordinator: push(.treatment)
Coordinator->>Treatment: present TreatmentView
Treatment->>TabBar: isTabbarHidden = true (onAppear)
alt 완료(우측 버튼)
User->>Treatment: 우측 버튼 탭
Treatment->>Coordinator: popToRoot()
Treatment->>TabBar: isTabbarHidden = false
else 이전(step==1, 좌측 버튼)
User->>Treatment: 좌측 버튼 탭
Treatment->>Coordinator: pop()
Treatment->>Treatment: viewModel.previous()
end
else 미선택 (NotSelected)
Select->>Coordinator: push(.noTreatment)
Coordinator->>NoT: present NoTreatmentView
NoT->>TabBar: isTabbarHidden = true (onAppear)
end
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: 2
🤖 Fix all issues with AI agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/Treatment/TreatmentView.swift`:
- Around line 19-28: The navigation logic in TreatmentView's leftButtonAction
and rightButtonAction duplicates NoTreatmentView; extract the shared behavior
into a reusable helper (e.g., a static function or a protocol + extension) that
takes the viewModel, calendarCoordinator, and tabBarCoordinator and performs the
actions, then call that helper from both TreatmentView and NoTreatmentView to
remove duplication; specifically consolidate the logic that checks
viewModel.step == 1 then calls calendarCoordinator.pop() and
viewModel.previous(), and the logic that calls calendarCoordinator.popToRoot()
and sets tabBarCoordinator.isTabbarHidden = false; also verify the intended
behavior of calling viewModel.previous() immediately after
calendarCoordinator.pop() when viewModel.step == 1 and adjust the helper to
avoid unintended state transitions if that call is incorrect.
- Line 75: The .id usage is inconsistent with NoTreatmentView; change
.id(viewModel.state) in TreatmentView.swift to .id(String(describing:
viewModel.state)) to match the rest of the codebase, or alternatively make the
TreatmentStep type explicitly Hashable and document that convention; update the
.id call to use String(describing: viewModel.state) (or add Hashable conformance
to the TreatmentStep enum/struct referenced by viewModel.state) so the usage is
consistent across both TreatmentView and NoTreatmentView.
| leftButtonAction: { | ||
| if viewModel.step == 1 { | ||
| calendarCoordinator.pop() | ||
| } | ||
| viewModel.previous() | ||
| }, | ||
| rightButtonAction: { | ||
| calendarCoordinator.popToRoot() | ||
| tabBarCoordinator.isTabbarHidden = false | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
NoTreatmentView와 동일한 네비게이션 로직 중복
leftButtonAction과 rightButtonAction의 로직이 NoTreatmentView.swift와 완전히 동일합니다. 두 뷰 모두 같은 네비게이션 패턴을 사용하므로, 공통 로직을 헬퍼 함수나 프로토콜 익스텐션으로 추출하는 것을 고려해 보세요.
또한 NoTreatmentView에서 언급한 것처럼, step == 1일 때 pop() 후 previous() 호출이 의도된 것인지 확인이 필요합니다.
🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/Treatment/TreatmentView.swift`
around lines 19 - 28, The navigation logic in TreatmentView's leftButtonAction
and rightButtonAction duplicates NoTreatmentView; extract the shared behavior
into a reusable helper (e.g., a static function or a protocol + extension) that
takes the viewModel, calendarCoordinator, and tabBarCoordinator and performs the
actions, then call that helper from both TreatmentView and NoTreatmentView to
remove duplication; specifically consolidate the logic that checks
viewModel.step == 1 then calls calendarCoordinator.pop() and
viewModel.previous(), and the logic that calls calendarCoordinator.popToRoot()
and sets tabBarCoordinator.isTabbarHidden = false; also verify the intended
behavior of calling viewModel.previous() immediately after
calendarCoordinator.pop() when viewModel.step == 1 and adjust the helper to
avoid unintended state transitions if that call is incorrect.
| .padding(.leading, 34.adjustedW) | ||
| .padding(.trailing, 33.adjustedW) | ||
| .id(String(describing: viewModel.state)) | ||
| .id(viewModel.state) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
.id() 사용 방식 불일치
NoTreatmentView.swift에서는 .id(String(describing: viewModel.state))를 사용하지만, 여기서는 .id(viewModel.state)를 사용합니다. TreatmentStep이 Hashable을 준수한다면 동작은 하지만, 코드베이스 전체에서 일관된 방식을 사용하는 것이 좋습니다.
🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/Treatment/TreatmentView.swift`
at line 75, The .id usage is inconsistent with NoTreatmentView; change
.id(viewModel.state) in TreatmentView.swift to .id(String(describing:
viewModel.state)) to match the rest of the codebase, or alternatively make the
TreatmentStep type explicitly Hashable and document that convention; update the
.id call to use String(describing: viewModel.state) (or add Hashable conformance
to the TreatmentStep enum/struct referenced by viewModel.state) so the usage is
consistent across both TreatmentView and NoTreatmentView.
There was a problem hiding this comment.
Actionable comments posted: 0
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/SelectedTreatmentSheetView.swift (1)
56-88:maxVisibleCount상수로 조건을 통일하세요.
3하드코딩은maxVisibleCount변경 시 불일치 위험이 큽니다. 동일한 상수를 재사용하세요.♻️ 제안 변경
- if selectedTreatments.count > 3 { + if selectedTreatments.count > maxVisibleCount { scrollViewTopMarkerView .allowsHitTesting(false) } @@ - if selectedTreatments.count > 3 { + if selectedTreatments.count > maxVisibleCount { scrollViewBottomMarkerView .allowsHitTesting(false) } @@ - if selectedTreatments.count > 3 { + if selectedTreatments.count > maxVisibleCount { GradientBox(isTop: true) @@ - GradientBox(isTop: false) + GradientBox(isTop: false) .frame(height: 42) .allowsHitTesting(false) .opacity(shouldShowGradientBottom ? 1 : 0) .frame(maxHeight: .infinity, alignment: .bottom) }
soseoyo12
left a comment
soseoyo12
left a comment
There was a problem hiding this comment.
수고하셨숨니당 제 뷰 연결도 해주세요 ㅠㅜ
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/NoTreatment/NoTreatmentView.swift`:
- Around line 19-29: leftButtonAction incorrectly calls viewModel.previous() and
fails to restore the tab bar when leaving from the first step; change the
leftButtonAction so that when viewModel.step == 1 it calls
calendarCoordinator.pop() and also sets tabBarCoordinator.isTabbarHidden = false
(mirroring rightButtonAction), and avoid calling viewModel.previous() in that
branch since step == 1 has no previous state; keep viewModel.previous() only for
non-first-step branches.
- Around line 55-57: The view hides the tab bar in NoTreatmentView using
.onAppear by setting tabBarCoordinator.isTabbarHidden = true but never restores
it on all exit paths; add a .onDisappear modifier on the same view to set
tabBarCoordinator.isTabbarHidden = false so the tab bar is restored when leaving
via back button, swipe-back gesture, or other system navigation (also ensure any
existing left-button actions do not conflict with this restoration logic such as
the explicit popToRoot handler).
| leftButtonAction: { | ||
| if viewModel.step == 1 { | ||
| calendarCoordinator.pop() | ||
| } | ||
| viewModel.previous() | ||
|
|
||
| }, | ||
| rightButtonAction: { | ||
| calendarCoordinator.popToRoot() | ||
| tabBarCoordinator.isTabbarHidden = false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "AppCoordinatorProtocol.swift" -type fRepository: TEAM-Cherrish/Cherrish-iOS
Length of output: 151
🏁 Script executed:
find . -name "NoTreatmentView.swift" -type fRepository: TEAM-Cherrish/Cherrish-iOS
Length of output: 176
🏁 Script executed:
find . -name "*Coordinator*.swift" -type f | head -20Repository: TEAM-Cherrish/Cherrish-iOS
Length of output: 1479
🏁 Script executed:
cat -n ./Cherrish-iOS/Cherrish-iOS/Presentation/Coordinator/AppCoordinatorProtocol.swiftRepository: TEAM-Cherrish/Cherrish-iOS
Length of output: 773
🏁 Script executed:
cat -n ./Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/NoTreatment/NoTreatmentView.swiftRepository: TEAM-Cherrish/Cherrish-iOS
Length of output: 7164
🏁 Script executed:
cat -n ./Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Coordinator/CalendarCoordinator.swift | head -100Repository: TEAM-Cherrish/Cherrish-iOS
Length of output: 724
🏁 Script executed:
cat -n ./Cherrish-iOS/Cherrish-iOS/Presentation/Global/Components/CherrishTabbar/TabBarCoordinator.swiftRepository: TEAM-Cherrish/Cherrish-iOS
Length of output: 1008
첫 단계에서 뒤로가기 시 탭바 미복구 문제 해결 필요
Line 19-25의 leftButtonAction에서 viewModel.step == 1일 때 calendarCoordinator.pop()을 호출한 후 계속해서 viewModel.previous()를 실행합니다. 그러나 onAppear에서 탭바가 숨겨진(line 56) 상태이고, 첫 단계에서 이탈할 때 탭바가 복구되지 않습니다. 오른쪽 버튼의 rightButtonAction(line 28)에서는 popToRoot() 후 tabBarCoordinator.isTabbarHidden = false로 탭바를 복구하지만, 왼쪽 버튼에서는 누락되어 있습니다. 또한 step == 1은 첫 단계이므로 이전 상태가 없어 viewModel.previous() 호출이 불필요합니다.
🔧 제안 수정안
leftButtonAction: {
if viewModel.step == 1 {
calendarCoordinator.pop()
+ tabBarCoordinator.isTabbarHidden = false
+ return
}
viewModel.previous()
}🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/NoTreatment/NoTreatmentView.swift`
around lines 19 - 29, leftButtonAction incorrectly calls viewModel.previous()
and fails to restore the tab bar when leaving from the first step; change the
leftButtonAction so that when viewModel.step == 1 it calls
calendarCoordinator.pop() and also sets tabBarCoordinator.isTabbarHidden = false
(mirroring rightButtonAction), and avoid calling viewModel.previous() in that
branch since step == 1 has no previous state; keep viewModel.previous() only for
non-first-step branches.
There was a problem hiding this comment.
왼쪽 버튼 같은 경우는 시술 여부 선택 -> 시술 플로우 -> 홈 흐름입니다. 그래서 시슬 풀로우에서는 다시 시술 여부 선택으로 가야 하는데 시술 여부 선택 뷰에서는 탭바가 필요하지 않습니다. 그래서 지금 래빗이 말하는 것처럼 할 경우에는 탭가 나오게 되고 시술 여부 선택에 있는.onAppear에서 다시 탭바를 지우려고 하는데 이 두 개의 호출 시점이 순서가 완벽하게 보장되지 않아서 탭바가 사라질 수도있고 다시 나타날수도 있습니다. 화면 흐름상 지금 화면에서 버튼 두 개의 제스처로만 화면을 이탈할수 있기때문에 이렇게 하였습니다. 더 좋은 방법 있으면 알려주세요!
| .onAppear { | ||
| tabBarCoordinator.isTabbarHidden = true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "NoTreatmentView.swift" | head -5Repository: TEAM-Cherrish/Cherrish-iOS
Length of output: 176
🏁 Script executed:
find . -type f -name "*Coordinator*" | grep -i tabbar | head -10Repository: TEAM-Cherrish/Cherrish-iOS
Length of output: 269
🏁 Script executed:
find . -type f -name "*Coordinator*" | head -20Repository: TEAM-Cherrish/Cherrish-iOS
Length of output: 1479
🏁 Script executed:
cat -n "./Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/NoTreatment/NoTreatmentView.swift" | head -100Repository: TEAM-Cherrish/Cherrish-iOS
Length of output: 3746
🏁 Script executed:
cat -n "./Cherrish-iOS/Cherrish-iOS/Presentation/Global/Components/CherrishTabbar/TabBarCoordinator.swift"Repository: TEAM-Cherrish/Cherrish-iOS
Length of output: 1008
🏁 Script executed:
rg "isTabbarHidden\s*=" -A 2 -B 2Repository: TEAM-Cherrish/Cherrish-iOS
Length of output: 9886
탭바 복구 누락: 스와이프 백/제스처 이탈 시 탭바가 숨겨진 상태로 유지됨
Line 55-57에서 .onAppear로 탭바를 숨기지만, 우측 버튼(popToRoot)을 통한 명시적 복구만 있고 좌측 버튼 액션에서는 복구하지 않습니다. 더 중요하게는 iOS의 스와이프 백 제스처나 시스템 백 버튼으로 이 뷰를 이탈할 경우, 어떤 복구 로직도 호출되지 않아 탭바가 숨겨진 상태로 유지됩니다.
.onDisappear를 추가하여 뷰 이탈 시점에 탭바를 복구해주세요.
예시
.onAppear {
tabBarCoordinator.isTabbarHidden = true
}
+.onDisappear {
+ tabBarCoordinator.isTabbarHidden = false
+}🤖 Prompt for AI Agents
In
`@Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/NoTreatment/NoTreatmentView.swift`
around lines 55 - 57, The view hides the tab bar in NoTreatmentView using
.onAppear by setting tabBarCoordinator.isTabbarHidden = true but never restores
it on all exit paths; add a .onDisappear modifier on the same view to set
tabBarCoordinator.isTabbarHidden = false so the tab bar is restored when leaving
via back button, swipe-back gesture, or other system navigation (also ensure any
existing left-button actions do not conflict with this restoration logic such as
the explicit popToRoot handler).
sum130
left a comment
sum130
left a comment
There was a problem hiding this comment.
드디어 뷰연결!! 완만히 연결되어 넽웤까지 팟팅해보자구요~~~~ 아자스!
| action: { } | ||
| action: { calendarCoordinator.push(.selectTreatment) } | ||
| ) | ||
| .padding(.horizontal, 24) |
| leftButtonAction: { | ||
| if viewModel.step == 1 { | ||
| calendarCoordinator.pop() | ||
| } | ||
| viewModel.previous() | ||
|
|
||
| }, | ||
| rightButtonAction: { | ||
| calendarCoordinator.popToRoot() | ||
| tabBarCoordinator.isTabbarHidden = false | ||
| } |
| .onAppear { | ||
| tabBarCoordinator.isTabbarHidden = true | ||
| } |
| .frame(height: 42) | ||
| .allowsHitTesting(false) | ||
| .opacity(shouldShowGradientTop ? 1 : 0) | ||
| .frame(maxHeight: .infinity, alignment: .top) | ||
|
|
||
| GradientBox(isTop: false) | ||
| .frame(height: 42) | ||
| .allowsHitTesting(false) | ||
| .opacity(shouldShowGradientBottom ? 1 : 0) | ||
| .frame(maxHeight: .infinity, alignment: .bottom) | ||
| } |
| .id(String(describing: viewModel.state)) | ||
| .id(viewModel.state) |
There was a problem hiding this comment.
큰 차이가 없을 거 같아서 삭제했습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
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/SelectedTreatmentSheetView.swift (1)
56-88:maxVisibleCount를 사용하여 일관성 유지Line 16에
maxVisibleCount = 3이 정의되어 있지만, 조건문에서 하드코딩된3을 사용하고 있습니다. 향후maxVisibleCount값이 변경될 경우 버그가 발생할 수 있으므로 상수를 활용해 주세요.♻️ 수정 제안
VStack(spacing: spacing) { - if selectedTreatments.count > 3 { + if selectedTreatments.count > maxVisibleCount { scrollViewTopMarkerView .allowsHitTesting(false) } ForEach(selectedTreatments, id: \.id) { treatment in TreatmentRowView( displayMode: .summary, treatmentEntity: treatment, isSelected: .constant(true), action: { removeTreatment(treatment) } ) .frame(height: itemHeight) } - if selectedTreatments.count > 3 { + if selectedTreatments.count > maxVisibleCount { scrollViewBottomMarkerView .allowsHitTesting(false) } } .padding(.vertical, 14.adjustedH) } - if selectedTreatments.count > 3 { + if selectedTreatments.count > maxVisibleCount { GradientBox(isTop: true)
♻️ Duplicate comments (1)
Cherrish-iOS/Cherrish-iOS/Presentation/Feature/Calendar/Treatment/View/SelectedTreatmentSheetView.swift (1)
40-42: 기기 대응 적용 완료이전 리뷰 코멘트에서 요청한 기기 대응이 반영되었습니다. 다만, 작은 화면 기기에서
1.adjustedH가 1pt 미만의 값으로 계산될 경우 렌더링이 불안정할 수 있습니다. 필요 시max(1, 1.adjustedH)를 고려해 주세요.
y-eonee
left a comment
y-eonee
left a comment
There was a problem hiding this comment.
굿 나중에 날짜 텍스트필드 예외처리도 해주세용
| leadingIcon: Image(.plus), | ||
| trailingIcon: nil, | ||
| action: { } | ||
| action: { calendarCoordinator.push(.selectTreatment) } |
| .onAppear { | ||
| tabBarCoordinator.isTabbarHidden = true | ||
| } |
4 participants
🔗 연결된 이슈
📄 작업 내용