Open
Conversation
📝 WalkthroughWalkthroughThe changes introduce comprehensive error handling across Call Van features with toast notifications, refactor date picker initialization to use precomputed date ranges, enhance post request validation with time and location constraints, and restructure home screen login checks to be explicitly triggered rather than automatic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 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 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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Koin/Presentation/Home/Home/HomeViewModel.swift (1)
196-203:⚠️ Potential issue | 🟠 MajorPotential retain cycle: missing
[weak self]in sink closure.The
sinkclosure at line 198 capturesselfstrongly. Since the subscription is stored inself.subscriptions, this creates a retain cycle that preventsHomeViewModelfrom being deallocated.🔧 Proposed fix
private func checkLogin() { checkLoginUseCase.execute() - .sink { isLoggedIn in + .sink { [weak self] isLoggedIn in + guard let self = self else { return } self.isLoggedIn = isLoggedIn UserDefaults.standard.set(isLoggedIn ? 1 : 0, forKey: "loginFlag") } .store(in: &subscriptions) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Koin/Presentation/Home/Home/HomeViewModel.swift` around lines 196 - 203, The sink closure in checkLogin() captures self strongly (checkLoginUseCase.execute().sink { ... }.store(in: &subscriptions)), creating a retain cycle because the AnyCancellable is stored in self.subscriptions; change the closure to capture self weakly (use [weak self]) and unwrap safely (e.g., guard let self = self else { return } or use self?) before setting self.isLoggedIn and writing to UserDefaults to break the retain cycle.Koin/Presentation/CallVan/CallVanPost/Subviews/CallVanPostPlaceBottomSheetView.swift (1)
143-174:⚠️ Potential issue | 🟠 MajorDon't dismiss on the custom path until the input is valid.
When the user switches from a preset place to
기타, this path never revalidatescustomPlaceTextField, so the apply button can stay enabled from the previous selection.applyButtonTapped()then skips both callbacks for an empty custom value but still dismisses the sheet at Lines 173-174.💡 Suggested fix
`@objc` private func customButtonTapped(_ sender: UIButton) { (buttons1 + buttons2).forEach { $0.isSelected = false } sender.isSelected = true updateCustomPlaceTextField(isVisible: true) + valiate(customPlaceTextField) UIView.animate( withDuration: 0.2, animations: { [weak self] in self?.superview?.layoutIfNeeded() self?.layoutIfNeeded() self?.updateApplyButtonTitle(isCustomSelected: true) }, completion: { [weak self] _ in self?.customPlaceTextField.becomeFirstResponder() } ) } `@objc` private func applyButtonTapped() { if let selectedButton = (buttons1 + buttons2).first(where: { $0.isSelected }), let selectedPlace = selectedButton.filterState as? CallVanPlace { onApplyButtonTapped?(selectedPlace, nil) - } - else if customButton.isSelected, + } else if customButton.isSelected, let customPlace = customPlaceTextField.text?.trimmingCharacters(in: .whitespacesAndNewlines), !customPlace.isEmpty { onApplyButtonTapped?(.custom, customPlace) + } else { + return } customPlaceTextField.resignFirstResponder() delegate?.dismiss() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Koin/Presentation/CallVan/CallVanPost/Subviews/CallVanPostPlaceBottomSheetView.swift` around lines 143 - 174, The dismiss and resignFirstResponder calls currently run unconditionally in applyButtonTapped(), which allows the sheet to dismiss even when the custom input is empty; modify applyButtonTapped() so that delegate?.dismiss() and customPlaceTextField.resignFirstResponder() are only called after a successful onApplyButtonTapped? invocation (i.e., inside the first branch that sends a selectedPlace and inside the else-if branch that sends .custom with a non-empty trimmed customPlace). Keep the existing selected-button detection logic and the customButtonTapped(_:) behavior (including updateCustomPlaceTextField and updateApplyButtonTitle) but ensure the fallback case (customButton.selected but customPlace empty) does not call the callbacks or dismiss/resign; instead, leave the sheet open so the user can correct input.
🧹 Nitpick comments (1)
Koin/Presentation/CallVan/CallVanChat/CallVanChatViewModel.swift (1)
126-137: Typo in parameter name:comepltionshould becompletion.The error handling logic is correct and consistent with other methods. However, the variable name has a typo.
✏️ Suggested fix
private func fetchData() { fetchCallVanDataUseCase.execute(postId: postId).sink( - receiveCompletion: { [weak self] comepltion in - if case .failure(let error) = comepltion { + receiveCompletion: { [weak self] completion in + if case .failure(let error) = completion { self?.outputSubject.send(.showToast(error.message)) } },Note: The same typo exists on line 98 in
postCallVanChat()- consider fixing both for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Koin/Presentation/CallVan/CallVanChat/CallVanChatViewModel.swift` around lines 126 - 137, Rename the misspelled sink closure parameter `comepltion` to `completion` in the fetchData() method (where fetchCallVanDataUseCase.execute(...).sink is called) and make the identical correction in postCallVanChat() so both receiveCompletion closures use `completion` and the existing error handling (if case .failure(let error) = completion { ... }) compiles and reads correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Koin/Core/View/BottomSheetViewControllerB.swift`:
- Around line 127-128: The bottom constraint uses contentView.bounds.height
before the injected view has completed layout, so initialize the
hidden/off-screen offset after a layout pass instead of using
contentView.bounds.height immediately. Move or defer setting
contentViewBottomConstraint (the constraint created against
view.keyboardLayoutGuide.snp.top) until after layout (e.g., in
viewDidLayoutSubviews or after calling view.layoutIfNeeded()) and calculate the
offset from a stable container height (view.bounds.height or the fully laid-out
contentView.frame.height) so present() will animate from off-screen correctly.
In
`@Koin/Core/View/KoinPickerDropDownView/Delegates/KoinPickerDropDownViewDateDelegate.swift`:
- Around line 14-19: The cached dates dictionary in
KoinPickerDropDownViewDateDelegate is only populated in init(range:) so the
picker never refreshes across midnight; ensure resetDates(range:) is invoked
whenever the picker or its range is reset/updated (for example inside the
delegate's public reset/updateRange method and whenever the selected
offset/visible range changes, including when offset == 0), so the dates cache
(dates) is rebuilt before reloading components (e.g., before calling
reloadAllComponents). Locate resetDates(range:) and init(range:) in
KoinPickerDropDownViewDateDelegate and add calls to resetDates(range:) in the
delegate methods that handle range/offset changes or picker resets so the cached
dates reflect the current day.
In `@Koin/Presentation/CallVan/CallVanPost/CallVanPostViewModel.swift`:
- Around line 101-136: In validate(), nils for departureDate/departureTime are
not treated as invalid; update the empty-value checks to explicitly reject nil
or empty strings for request.departureDate and request.departureTime (e.g., use
optional binding/guards to require non-nil, non-empty values before parsing), so
the parse branch always runs only when both date and time are present, and keep
the existing time-interval check using the bound values; reference validate(),
request.departureDate, request.departureTime, and the formatter/date parsing
logic.
---
Outside diff comments:
In
`@Koin/Presentation/CallVan/CallVanPost/Subviews/CallVanPostPlaceBottomSheetView.swift`:
- Around line 143-174: The dismiss and resignFirstResponder calls currently run
unconditionally in applyButtonTapped(), which allows the sheet to dismiss even
when the custom input is empty; modify applyButtonTapped() so that
delegate?.dismiss() and customPlaceTextField.resignFirstResponder() are only
called after a successful onApplyButtonTapped? invocation (i.e., inside the
first branch that sends a selectedPlace and inside the else-if branch that sends
.custom with a non-empty trimmed customPlace). Keep the existing selected-button
detection logic and the customButtonTapped(_:) behavior (including
updateCustomPlaceTextField and updateApplyButtonTitle) but ensure the fallback
case (customButton.selected but customPlace empty) does not call the callbacks
or dismiss/resign; instead, leave the sheet open so the user can correct input.
In `@Koin/Presentation/Home/Home/HomeViewModel.swift`:
- Around line 196-203: The sink closure in checkLogin() captures self strongly
(checkLoginUseCase.execute().sink { ... }.store(in: &subscriptions)), creating a
retain cycle because the AnyCancellable is stored in self.subscriptions; change
the closure to capture self weakly (use [weak self]) and unwrap safely (e.g.,
guard let self = self else { return } or use self?) before setting
self.isLoggedIn and writing to UserDefaults to break the retain cycle.
---
Nitpick comments:
In `@Koin/Presentation/CallVan/CallVanChat/CallVanChatViewModel.swift`:
- Around line 126-137: Rename the misspelled sink closure parameter `comepltion`
to `completion` in the fetchData() method (where
fetchCallVanDataUseCase.execute(...).sink is called) and make the identical
correction in postCallVanChat() so both receiveCompletion closures use
`completion` and the existing error handling (if case .failure(let error) =
completion { ... }) compiles and reads correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a75ec328-69ea-4184-a227-ad0f6997a42a
📒 Files selected for processing (12)
Koin/Core/View/BottomSheetViewControllerB.swiftKoin/Core/View/KoinPickerDropDownView/Delegates/KoinPickerDropDownViewDateDelegate.swiftKoin/Data/DTOs/Decodable/CallVan/CallVanDataDto.swiftKoin/Presentation/CallVan/CallVanChat/CallVanChatViewModel.swiftKoin/Presentation/CallVan/CallVanData/CallVanDataViewController.swiftKoin/Presentation/CallVan/CallVanData/CallVanDataViewModel.swiftKoin/Presentation/CallVan/CallVanList/CallVanListViewModel.swiftKoin/Presentation/CallVan/CallVanPost/CallVanPostViewModel.swiftKoin/Presentation/CallVan/CallVanPost/Subviews/CallVanPostDateView.swiftKoin/Presentation/CallVan/CallVanPost/Subviews/CallVanPostPlaceBottomSheetView.swiftKoin/Presentation/Home/Home/HomeViewController.swiftKoin/Presentation/Home/Home/HomeViewModel.swift
Comment on lines
+127
to
+128
| contentView.layoutIfNeeded() | ||
| contentViewBottomConstraint = $0.bottom.equalTo(view.keyboardLayoutGuide.snp.top).offset(contentView.bounds.height).constraint |
There was a problem hiding this comment.
Initialize the hidden offset after the sheet has a real height.
contentView.bounds.height is often still 0 here because the injected view has not completed an Auto Layout pass yet. In that case the sheet starts on-screen and the first present() animation becomes a no-op. Use an off-screen offset derived after layout, or from the container height instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Koin/Core/View/BottomSheetViewControllerB.swift` around lines 127 - 128, The
bottom constraint uses contentView.bounds.height before the injected view has
completed layout, so initialize the hidden/off-screen offset after a layout pass
instead of using contentView.bounds.height immediately. Move or defer setting
contentViewBottomConstraint (the constraint created against
view.keyboardLayoutGuide.snp.top) until after layout (e.g., in
viewDidLayoutSubviews or after calling view.layoutIfNeeded()) and calculate the
offset from a stable container height (view.bounds.height or the fully laid-out
contentView.frame.height) so present() will animate from off-screen correctly.
Comment on lines
+14
to
+19
| private var dates: [Int: [Int: [Int]]] = [:] | ||
| private let columnWidths: [CGFloat] = [53, 31, 31] | ||
|
|
||
| private let inputFormatter = DateFormatter().then { | ||
| $0.dateFormat = "yyyy-MM-dd" | ||
| } | ||
|
|
||
| private let outputFormatter = DateFormatter().then { | ||
| $0.dateFormat = "yyyy년*M월*d일" | ||
| // MARK: - Initialzier | ||
| init(range: Range<Int>) { | ||
| resetDates(range: range) |
There was a problem hiding this comment.
Refresh the cached dates when the picker resets.
resetDates(range:) now runs only in init(range:). Because Koin/Presentation/CallVan/CallVanPost/Subviews/CallVanPostDateView.swift creates this delegate once at Line 28, leaving the form open across midnight makes offset 0 still point to yesterday and the range never advances until the view is recreated.
💡 Suggested fix
final class KoinPickerDropDownViewDateDelegate {
// MARK: - Properties
private var dates: [Int: [Int: [Int]]] = [:]
private let columnWidths: [CGFloat] = [53, 31, 31]
+ private let range: Range<Int>
// MARK: - Initialzier
init(range: Range<Int>) {
+ self.range = range
resetDates(range: range)
}
}
extension KoinPickerDropDownViewDateDelegate: KoinPickerDropDownViewDelegate {
@@
func reset(koinPicker: KoinPickerDropDownView, initialDate: Date) {
+ resetDates(range: range)
let year = initialDate.year
let month = initialDate.month
let day = initialDate.dayAlso applies to: 29-37, 72-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Koin/Core/View/KoinPickerDropDownView/Delegates/KoinPickerDropDownViewDateDelegate.swift`
around lines 14 - 19, The cached dates dictionary in
KoinPickerDropDownViewDateDelegate is only populated in init(range:) so the
picker never refreshes across midnight; ensure resetDates(range:) is invoked
whenever the picker or its range is reset/updated (for example inside the
delegate's public reset/updateRange method and whenever the selected
offset/visible range changes, including when offset == 0), so the dates cache
(dates) is rebuilt before reloading components (e.g., before calling
reloadAllComponents). Locate resetDates(range:) and init(range:) in
KoinPickerDropDownViewDateDelegate and add calls to resetDates(range:) in the
delegate methods that handle range/offset changes or picker resets so the cached
dates reflect the current day.
Comment on lines
+101
to
+136
| private func validate() { | ||
| var isValid = true | ||
|
|
||
| outputSubject.send(.enablePostButton(validation)) | ||
| let formatter = DateFormatter() | ||
| formatter.dateFormat = "yyyy-MM-dd HH:mm" | ||
| let tenMinutes: TimeInterval = 1 * 60 * 10 | ||
|
|
||
| /// 빈 값이 있는지 확인 | ||
| if request.departureType == nil | ||
| || request.arrivalType == nil | ||
| || request.departureDate?.isEmpty == true | ||
| || request.departureTime?.isEmpty == true { | ||
| isValid = false | ||
| } | ||
|
|
||
| /// 출발지와 도착지가 다른지 확인 | ||
| switch (request.departureType, request.arrivalType) { | ||
| case (.custom, .custom): | ||
| if request.departureCustomName == request.arrivalCustomName { | ||
| isValid = false | ||
| } | ||
| default: | ||
| if request.departureType == request.arrivalType { | ||
| isValid = false | ||
| } | ||
| } | ||
|
|
||
| /// 현재 시각으로부터 최소 10분 이후 일정인지 확인 | ||
| let date = request.departureDate ?? "" | ||
| let time = request.departureTime ?? "" | ||
| if let requestDate = formatter.date(from: "\(date) \(time)"), | ||
| requestDate.timeIntervalSince(Date()) < tenMinutes { | ||
| isValid = false | ||
| } | ||
|
|
||
| outputSubject.send(.enablePostButton(isValid)) |
There was a problem hiding this comment.
nil date/time values still pass this validation.
departureDate?.isEmpty == true and departureTime?.isEmpty == true only reject empty strings. If either field is still nil, isValid stays true and the parse branch is skipped, so the post button can enable before those fields are populated.
💡 Suggested fix
private func validate() {
var isValid = true
let formatter = DateFormatter()
formatter.dateFormat = "yyyy-MM-dd HH:mm"
- let tenMinutes: TimeInterval = 1 * 60 * 10
+ let tenMinutes: TimeInterval = 10 * 60
- /// 빈 값이 있는지 확인
- if request.departureType == nil
- || request.arrivalType == nil
- || request.departureDate?.isEmpty == true
- || request.departureTime?.isEmpty == true {
- isValid = false
- }
+ guard request.departureType != nil,
+ request.arrivalType != nil,
+ let date = request.departureDate, !date.isEmpty,
+ let time = request.departureTime, !time.isEmpty,
+ let requestDate = formatter.date(from: "\(date) \(time)") else {
+ outputSubject.send(.enablePostButton(false))
+ return
+ }
/// 출발지와 도착지가 다른지 확인
switch (request.departureType, request.arrivalType) {
case (.custom, .custom):
if request.departureCustomName == request.arrivalCustomName {
isValid = false
}
default:
if request.departureType == request.arrivalType {
isValid = false
}
}
/// 현재 시각으로부터 최소 10분 이후 일정인지 확인
- let date = request.departureDate ?? ""
- let time = request.departureTime ?? ""
- if let requestDate = formatter.date(from: "\(date) \(time)"),
- requestDate.timeIntervalSince(Date()) < tenMinutes {
+ if requestDate.timeIntervalSince(Date()) < tenMinutes {
isValid = false
}
outputSubject.send(.enablePostButton(isValid))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Koin/Presentation/CallVan/CallVanPost/CallVanPostViewModel.swift` around
lines 101 - 136, In validate(), nils for departureDate/departureTime are not
treated as invalid; update the empty-value checks to explicitly reject nil or
empty strings for request.departureDate and request.departureTime (e.g., use
optional binding/guards to require non-nil, non-empty values before parsing), so
the parse branch always runs only when both date and time are present, and keep
the existing time-interval check using the bound values; reference validate(),
request.departureDate, request.departureTime, and the formatter/date parsing
logic.
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.
#️⃣연관된 이슈
📝작업 내용
콜밴팟 스프린트 QA 수정 작업니다.
스크린샷 (선택)
💬리뷰 요구사항(선택)
Summary by CodeRabbit
Bug Fixes
New Features
Improvements