fix: 분실물 도메인 분리 QA 이슈 해결#343
Conversation
습득물일 경우 '주인을 찾았나요?', 분실물일 경우 '물건을 찾았나요?'가 보이도록 분기처리합니다.
키보드가 올라왔을 때, 작성 완료 버튼이 항상 가려지지 않도록 변경합니다.
키보드가 올라왔을 때 수정완료 버튼이 항상 가려지지 않도록 수정합니다.
응답본문에서 추가된 사진의 id가 null입니다. Image의 id의 타입을 Int?으로 변경했습니다.
/lost-item 을 추가했습니다.
키보드가 올라왔을 때 버튼 상단 패딩을 24에서 16으로 바꾸도록 합니다.
키보드가 올라왔을 때 버튼 상단 패팅을 24에서 16으로 수정합니다.
viewWillAppear에서 lottie 애니메이션을 시작합니다.
There was a problem hiding this comment.
Pull request overview
This PR addresses 5 QA issues identified during lost item domain separation testing. The changes fix UI/animation bugs, improve form validation, handle API response changes, and update the category filtering mechanism.
Changes:
- Fixed Lottie animation issues in force update and force modify user modals by properly implementing animation lifecycle management
- Added essential field markers (*) to required form fields and updated state change labels to vary by item type (lost/found)
- Implemented proper keyboard handling to keep the complete button visible when keyboard appears
- Changed category filtering from single selection to multi-selection using Set
- Updated data models to handle nullable image IDs and replaced isCouncil boolean with organization object
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| koin.xcodeproj/project.pbxproj | Updated version to 4.6.0 (build 433) and renamed UpdateModelViewController to UpdateModalViewController |
| ShopSummaryViewController.swift | Optimized gradient layer creation to avoid recreating on every layout pass |
| AddLostItemHeaderView.swift | Added essential field indicator to header |
| AddLostItemCollectionViewCell.swift | Added essential field markers and improved location validation for found items |
| PostLostItemViewController.swift | Added keyboard-aware button positioning with background view |
| LostItemListFilterViewController.swift | Refactored category filtering to support multiple selections using Set |
| LostItemListFilterButton.swift | Made text property public for category matching |
| LostItemDataRecentTableView.swift | Removed unnecessary content inset |
| LostItemDataContentView.swift | Changed from isCouncil to organization with dynamic location text |
| LostItemDataButtonsView.swift | Updated state change label to vary by item type |
| LostItemDataViewModel.swift | Adjusted pagination limits and added error logging |
| LostItemDataViewController.swift | Updated scroll view constraints and organization handling |
| EditLostItemImagesView.swift | Enforced 10-image limit on initialization |
| EditLostItemHeaderView.swift | Added essential field indicator |
| EditLostItemFoundPlaceView.swift | Added essential field marker |
| EditLostItemFoundDateView.swift | Added essential field marker |
| EditLostItemCategoryView.swift | Added essential field marker |
| EditLostItemButton.swift | Fixed inverted color logic for selected state |
| EditLostItemViewController.swift | Added keyboard-aware button positioning |
| ForceModifyUserViewController.swift | Replaced static image with Lottie animation |
| UpdateModalViewController.swift | Renamed from UpdateModelViewController (spelling fix) |
| ForceUpdateViewController.swift | Added startLottieAnimation call in viewWillAppear |
| EditLostItemUseCase.swift | Changed Image.id to optional and handle null with default value |
| LostItemData.swift | Replaced isCouncil with organization object and removed updatedAt |
| LostItemAPI.swift | Updated API endpoint to v2 and changed URL encoding for arrays |
| FetchLostItemListRequest.swift | Changed category from single value to Set and added description methods |
| LostArticleDetailDto.swift | Changed Image.id to optional Int |
| LostItemDataDto.swift | Made foundPlace optional, replaced isCouncil with organization, renamed prev/next ID fields |
| ZoomedImageViewControllerB.swift | Added light status bar style and gradient overlay |
| ZoomedImageRootViewController.swift | Added gradient view for status bar area |
| ZoomedImageCollectionViewCell.swift | Removed navigation bar hiding on zoom |
| ZoomedImageCollectionView.swift | Removed navigation bar hiding subscriptions |
| SceneDelegate.swift | Added support for "/lost-item" deep link path |
Comments suppressed due to low confidence (2)
Koin/Presentation/Home/ForceUpdate/UpdateModalViewController.swift:2
- The file and class were renamed from UpdateModelViewController to UpdateModalViewController. This is a good spelling correction (Model -> Modal), as this is indeed a modal view controller, not a model. Ensure that all references to this class have been updated throughout the codebase. The references in the project file and ForceUpdateViewController.swift have been updated correctly.
Koin/Presentation/LostItem/LostItemData/LostItemDataViewModel.swift:114 - The loadList method sets limit to 15 but doesn't reset the page counter to 1. If loadList is called after some loadMoreList calls have incremented the page counter, it will fetch page 2+ with limit 15 instead of page 1. This could happen if the view is refreshed or reloaded. Consider resetting filterState.page = 1 in loadList to ensure it always fetches the first page when called, or create a new filterState instance if a full reset is intended.
private func loadList() {
filterState.limit = 15
fetchLostItemListUseCase.execute(requestModel: filterState).sink(
receiveCompletion: { _ in },
receiveValue: { [weak self] lostItemList in
self?.outputSubject.send(.updateList(lostItemList.articles.filter { !$0.isReported }))
}
).store(in: &subscriptions)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // URL의 경로가 "/articles/lost-item"인지 확인 | ||
| if incomingURL.path == "/articles/lost-item" { | ||
| if incomingURL.path == "/articles/lost-item" || incomingURL.path == "/lost-item" { |
There was a problem hiding this comment.
The deep link handling now supports both "/articles/lost-item" and "/lost-item" paths. Ensure this change is intentional and documented, as it introduces a new URL scheme. Also verify that both paths are properly configured in the app's associated domains and universal links configuration.
| $0.top.equalTo(recentHeaderView.snp.bottom) | ||
| $0.leading.trailing.bottom.equalTo(scrollContentView) | ||
| $0.height.equalTo(lostItemDataTableView.rowHeight * 5) | ||
| $0.height.greaterThanOrEqualTo(lostItemDataTableView.rowHeight * 4.6) |
There was a problem hiding this comment.
The table view height constraint changed from exactly rowHeight * 5 to greaterThanOrEqualTo(rowHeight * 4.6). The magic number 4.6 appears arbitrary. If this is meant to show 4-5 rows with some partial row visible, consider using a more explicit calculation or adding a comment explaining why 4.6 was chosen. Also, with a greaterThanOrEqualTo constraint and the scrollContentView also having a greaterThanOrEqualTo height constraint (line 327), ensure that Auto Layout can properly resolve the table view's actual height, especially when there are fewer than 5 items.
| $0.height.greaterThanOrEqualTo(lostItemDataTableView.rowHeight * 4.6) | |
| $0.height.equalTo(lostItemDataTableView.rowHeight * 5) // Show exactly 5 rows |
| let deleteImageIds: [Int] = originalData.images.filter { | ||
| !updatedUrls.contains($0.imageUrl) | ||
| }.map { | ||
| $0.id | ||
| $0.id ?? 0 | ||
| } |
There was a problem hiding this comment.
When an image's id is nil, it defaults to 0 for the delete operation. However, 0 is likely not a valid image ID (IDs typically start from 1), and sending 0 to the backend might cause unexpected behavior or silently fail to delete the intended images. According to the PR description, newly added images have null IDs, which makes sense because they haven't been persisted to the backend yet. Newly added images shouldn't be in the delete list at all since they were never saved. Consider filtering out images with nil IDs from the delete list instead of using a default value, as they don't need to be deleted from the server.
| var page: Int = 1 | ||
| var limit: Int = 10 | ||
| var category: LostItemCategory = .all | ||
| var category: Set<LostItemCategory> = [.all] |
There was a problem hiding this comment.
The category field is changed from a single LostItemCategory to a Set<LostItemCategory>, but there's no custom encoding implementation. Swift's default encoding for Set will encode it as an array, which may not match the API's expected format. Additionally, the URLEncoding was changed to use .noBrackets encoding in LostItemAPI.swift (line 74), but it's unclear if the API expects multiple category parameters (e.g., category=CARD&category=WALLET) or a different format. Please verify that the API correctly handles the Set encoding and that the URL parameter format matches the backend's expectations.
| } else { | ||
| filterState.category.insert(category) | ||
| } | ||
| } |
There was a problem hiding this comment.
The category button tap handler now allows selecting multiple categories by toggling them on/off (except for "all" which deselects others). However, when no categories are selected (all are toggled off), the Set will be empty. The UI doesn't handle this empty state - there's no logic to prevent having zero categories selected or to automatically re-select "all" when the last category is deselected. This could lead to API requests with no category filters, which may not be the intended behavior.
| } | |
| } | |
| // Ensure at least one category is always selected. | |
| if filterState.category.isEmpty { | |
| filterState.category.insert(.all) | |
| } |
| private let essentialLabel = UILabel().then { | ||
| $0.attributedText = NSAttributedString( | ||
| string: " *", | ||
| attributes: [ | ||
| .font: UIFont.appFont(.pretendardRegular, size: 11), | ||
| .foregroundColor : UIColor(hexCode: "C82A2A") | ||
| ] | ||
| ) | ||
| } |
There was a problem hiding this comment.
The essential label (*) is always visible for the location field in EditLostItemFoundPlaceView, regardless of the item type (lost/found). According to the PR description and the pattern in AddLostItemCollectionViewCell (line 308 in AddLostItemCollectionViewCell.swift), the location field should only be required (and show the essential marker) for found items, not lost items. Consider adding logic to hide the essential label when the type is .lost, either in the init method or by adding essentialLabel.isHidden = (type == .lost) in the configureView method.
| private func handleIncomingDeepLink(url: URL, navigationController: UINavigationController) { | ||
| // URL 경로가 "/articles/lost-item"인 경우에 처리 | ||
| if url.path == "/articles/lost-item" { | ||
| if url.path == "/articles/lost-item" || url.path == "/lost-item" { |
There was a problem hiding this comment.
The deep link handling now supports both "/articles/lost-item" and "/lost-item" paths. Ensure this change is intentional and documented, as it introduces a new URL scheme. Also verify that both paths are properly configured in the app's associated domains and universal links configuration.
| if locationTextField.textColor == UIColor.appColor(.neutral500) { | ||
| locationWarningLabel.isHidden = false | ||
| isValid = false | ||
| } | ||
| else { | ||
| if locationTextField.text?.trimmingCharacters(in: .whitespaces).isEmpty == true { | ||
| locationWarningLabel.isHidden = false | ||
| isValid = false | ||
| } | ||
| } |
There was a problem hiding this comment.
The validation logic for the location field has redundant nesting. When type == .found, if the text color is NOT neutral500, it enters the else block (line 464) and checks if the text is empty. However, this check should also apply when the text color IS neutral500 (placeholder state), since a placeholder means no actual input. The current logic will not show a warning when the placeholder is visible but somehow has non-whitespace text (an unlikely but possible edge case). Consider simplifying this to check both conditions together: if the location is required (found type) and either the text is a placeholder OR the trimmed text is empty, show the warning.
| if locationTextField.textColor == UIColor.appColor(.neutral500) { | |
| locationWarningLabel.isHidden = false | |
| isValid = false | |
| } | |
| else { | |
| if locationTextField.text?.trimmingCharacters(in: .whitespaces).isEmpty == true { | |
| locationWarningLabel.isHidden = false | |
| isValid = false | |
| } | |
| } | |
| let trimmedLocation = locationTextField.text?.trimmingCharacters(in: .whitespaces) ?? "" | |
| if locationTextField.textColor == UIColor.appColor(.neutral500) || trimmedLocation.isEmpty { | |
| locationWarningLabel.isHidden = false | |
| isValid = false | |
| } |
|
|
||
| guard let targetView = [foundPlaceView.locationTextField, | ||
| contentView.contentTextView].first(where: { $0.isFirstResponder }) else { | ||
| print("no targetview that is firstResponder") |
There was a problem hiding this comment.
Debug print statement left in production code. This print statement should be removed before merging, or replaced with proper logging using the app's logging framework if diagnostic logging is needed.
| guard let targetView = [foundPlaceView.locationTextField, | |
| contentView.contentTextView].first(where: { $0.isFirstResponder }) else { | |
| print("no targetview that is firstResponder") | |
| guard let targetView = [foundPlaceView.locationTextField, | |
| contentView.contentTextView].first(where: { $0.isFirstResponder }) else { |
| receiveCompletion: { _ in }, | ||
| receiveCompletion: { completion in | ||
| if case .failure(let failure) = completion { | ||
| print(failure) |
There was a problem hiding this comment.
Debug print statement left in production code. This print statement should be removed before merging, or replaced with proper error handling/logging using the app's logging framework. Simply printing the error without proper handling may hide important failures from users and developers.
| print(failure) | |
| NSLog("Failed to fetch lost item data: \(failure)") |
#️⃣연관된 이슈
📝작업 내용
분실물 작성/수정 화면에서 키보드가 올라왔을 때, 완료 버튼이 항상 보이도록 변경
분실물 수정 화면에서 사진을 10개 이상 등록할 수 있는 문제 해결
분실물 수정 화면에서 사진을 추가했을 때 '알 수 없는 에러' 오류 해결
스크린샷 (선택)
💬리뷰 요구사항(선택)