bookmarkfeature 모듈을 패키지로 구성합니다.#335
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the MLSBookmarkFeature module, implementing comprehensive bookmarking and collection management capabilities, including DTOs, endpoints, repositories, reactors, and view controllers. The code review identifies several critical issues and areas for improvement: potential data races and background thread UI updates during image loading, a lack of error handling in RxSwift streams that could lead to unexpected stream termination, a side effect in the onboarding query repository, and architectural improvements regarding navigation logic and hardcoded indices.
| func isLoggedIn() -> Observable<Bool> { | ||
| switch tokenRepository.fetchToken(type: .refreshToken) { | ||
| case .success(let token): | ||
| guard !token.isEmpty else { return .just(false) } | ||
| return authAPIRepository.reissueToken(refreshToken: token) | ||
| .map { [weak self] response -> Bool in | ||
| guard let self else { return false } | ||
| let accessResult = self.tokenRepository.saveToken(type: .accessToken, value: response.accessToken) | ||
| let refreshResult = self.tokenRepository.saveToken(type: .refreshToken, value: response.refreshToken) | ||
| switch (accessResult, refreshResult) { | ||
| case (.success, .success): return true | ||
| default: return false | ||
| } | ||
| } | ||
| .catch { _ in .just(false) } | ||
| case .failure: | ||
| return .just(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
isLoggedIn() 메서드가 호출될 때마다 매번 reissueToken 네트워크 요청을 보내고 토큰을 갱신하고 있습니다. 이 메서드는 viewWillAppear 등 화면이 나타날 때마다 빈번하게 호출되는데, 매번 네트워크 요청을 보내는 것은 서버 부하를 가중시키고 불필요한 리소스를 소모하게 됩니다.
일반적으로 isLoggedIn()은 로컬에 유효한 토큰(또는 만료되지 않은 토큰)이 존재하는지만 가볍게 확인하고, 토큰 만료 시 재발급(Reissue)하는 로직은 API 요청 시 TokenInterceptor 등에서 공통으로 처리하도록 분리하는 것이 효율적입니다.
func isLoggedIn() -> Observable<Bool> {
switch tokenRepository.fetchToken(type: .accessToken) {
case .success(let token):
return .just(!token.isEmpty)
case .failure:
return .just(false)
}
}| switch currentState.type { | ||
| case .total: | ||
| return bookmarkRepository.fetchBookmark(sort: resolvedSort).map { .setItems($0) } | ||
| case .monster: | ||
| return bookmarkRepository.fetchMonsterBookmark( | ||
| minLevel: currentState.startLevel ?? 1, | ||
| maxLevel: currentState.endLevel ?? 200, | ||
| sort: resolvedSort | ||
| ).map { .setItems($0) } | ||
| case .item: | ||
| return bookmarkRepository.fetchItemBookmark( | ||
| jobId: nil, | ||
| minLevel: currentState.startLevel, | ||
| maxLevel: currentState.endLevel, | ||
| categoryIds: nil, | ||
| sort: resolvedSort | ||
| ).map { .setItems($0) } | ||
| case .npc: | ||
| return bookmarkRepository.fetchNPCBookmark(sort: resolvedSort).map { .setItems($0) } | ||
| case .quest: | ||
| return bookmarkRepository.fetchQuestBookmark(sort: resolvedSort).map { .setItems($0) } | ||
| case .map: | ||
| return bookmarkRepository.fetchMapBookmark(sort: resolvedSort).map { .setItems($0) } | ||
| default: | ||
| return .empty() | ||
| } | ||
| } | ||
|
|
||
| func handleToggle(id: Int) -> Observable<Mutation> { |
There was a problem hiding this comment.
RxSwift에서 API 요청 등의 에러가 발생했을 때 적절히 처리하지 않으면, 에러가 스트림을 타고 올라가 전체 Reactor의 구독(Subscription)이 해제(Dispose)되어 화면이 더 이상 동작하지 않게 됩니다. fetchList() 메서드 내의 각 API 요청 Observable에 .catch 또는 .catchAndReturn 등을 추가하여 에러 발생 시에도 스트림이 유지되도록 에러 핸들링을 추가해야 합니다.
func fetchList(sort: SortType? = nil) -> Observable<Mutation> {
let resolvedSort = (sort ?? currentState.sort)?.sortParameter
let fetchObservable: Observable<[BookmarkResponse]>
switch currentState.type {
case .total:
fetchObservable = bookmarkRepository.fetchBookmark(sort: resolvedSort)
case .monster:
fetchObservable = bookmarkRepository.fetchMonsterBookmark(
minLevel: currentState.startLevel ?? 1,
maxLevel: currentState.endLevel ?? 200,
sort: resolvedSort
)
case .item:
fetchObservable = bookmarkRepository.fetchItemBookmark(
jobId: nil,
minLevel: currentState.startLevel,
maxLevel: currentState.endLevel,
categoryIds: nil,
sort: resolvedSort
)
case .npc:
fetchObservable = bookmarkRepository.fetchNPCBookmark(sort: resolvedSort)
case .quest:
fetchObservable = bookmarkRepository.fetchQuestBookmark(sort: resolvedSort)
case .map:
fetchObservable = bookmarkRepository.fetchMapBookmark(sort: resolvedSort)
default:
return .empty()
}
return fetchObservable
.map { Mutation.setItems($0) }
.catch { _ in .just(.navigateTo(.bookmarkError)) }
}| private func loadImages(from urls: [String?], completion: @escaping ([UIImage?]) -> Void) { | ||
| var results = [UIImage?](repeating: nil, count: urls.count) | ||
| let group = DispatchGroup() | ||
| for (index, urlString) in urls.enumerated() { | ||
| group.enter() | ||
| ImageLoader.shared.loadImage(stringURL: urlString) { image in | ||
| results[index] = image | ||
| group.leave() | ||
| } | ||
| } | ||
| group.notify(queue: .main) { | ||
| completion(results) | ||
| } | ||
| } |
There was a problem hiding this comment.
loadImages 메서드에서 비동기 클로저 내부의 results[index] = image 코드는 데이터 레이스(Data Race)를 유발할 수 있습니다. ImageLoader가 이미지를 백그라운드 스레드에서 로드하고 컴플리션을 호출하는 경우, 여러 스레드에서 동시에 results 배열의 서로 다른 인덱스에 접근하여 쓰기 작업을 수행하게 되므로 크래시나 메모리 오염이 발생할 수 있습니다.
이를 방지하기 위해 배열에 값을 쓰는 작업을 메인 스레드(DispatchQueue.main.async)에서 수행하도록 동기화해야 합니다.
| private func loadImages(from urls: [String?], completion: @escaping ([UIImage?]) -> Void) { | |
| var results = [UIImage?](repeating: nil, count: urls.count) | |
| let group = DispatchGroup() | |
| for (index, urlString) in urls.enumerated() { | |
| group.enter() | |
| ImageLoader.shared.loadImage(stringURL: urlString) { image in | |
| results[index] = image | |
| group.leave() | |
| } | |
| } | |
| group.notify(queue: .main) { | |
| completion(results) | |
| } | |
| } | |
| private func loadImages(from urls: [String?], completion: @escaping ([UIImage?]) -> Void) { | |
| var results = [UIImage?](repeating: nil, count: urls.count) | |
| let group = DispatchGroup() | |
| for (index, urlString) in urls.enumerated() { | |
| group.enter() | |
| ImageLoader.shared.loadImage(stringURL: urlString) { image in | |
| DispatchQueue.main.async { | |
| results[index] = image | |
| group.leave() | |
| } | |
| } | |
| } | |
| group.notify(queue: .main) { | |
| completion(results) | |
| } | |
| } |
| if let url = URL(string: input.imageUrl) { | ||
| ImageLoader.shared.loadImage(url: url) { [weak self] image in | ||
| guard let self else { return } | ||
| if let currentIndex = collectionView.indexPath(for: self), currentIndex == indexPath { | ||
| if isMap { | ||
| self.cellView.setMapImage(image: image ?? UIImage(), backgroundColor: input.type.backgroundColor) | ||
| } else { | ||
| self.cellView.setImage(image: image ?? UIImage(), backgroundColor: input.type.backgroundColor) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
ImageLoader.shared.loadImage는 컴플리션 핸들러를 메인 스레드에서 호출하는 것을 보장하지 않을 수 있습니다. 실제로 다른 코드에서는 DispatchQueue.main.async로 감싸서 UI를 업데이트하고 있는 반면, 여기서는 백그라운드 스레드에서 직접 UIKit 컴포넌트(cellView)를 업데이트할 위험이 있습니다.
UI 업데이트 및 collectionView.indexPath(for:) 호출을 DispatchQueue.main.async 블록 내부에서 수행하도록 수정해야 합니다.
| if let url = URL(string: input.imageUrl) { | |
| ImageLoader.shared.loadImage(url: url) { [weak self] image in | |
| guard let self else { return } | |
| if let currentIndex = collectionView.indexPath(for: self), currentIndex == indexPath { | |
| if isMap { | |
| self.cellView.setMapImage(image: image ?? UIImage(), backgroundColor: input.type.backgroundColor) | |
| } else { | |
| self.cellView.setImage(image: image ?? UIImage(), backgroundColor: input.type.backgroundColor) | |
| } | |
| } | |
| } | |
| } | |
| if let url = URL(string: input.imageUrl) { | |
| ImageLoader.shared.loadImage(url: url) { [weak self] image in | |
| DispatchQueue.main.async { | |
| guard let self else { return } | |
| if let currentIndex = collectionView.indexPath(for: self), currentIndex == indexPath { | |
| if isMap { | |
| self.cellView.setMapImage(image: image ?? UIImage(), backgroundColor: input.type.backgroundColor) | |
| } else { | |
| self.cellView.setImage(image: image ?? UIImage(), backgroundColor: input.type.backgroundColor) | |
| } | |
| } | |
| } | |
| } | |
| } |
| func hasVisitedOnboarding() -> Observable<Bool> { | ||
| let hasVisited = UserDefaults.standard.bool(forKey: key) | ||
| if !hasVisited { | ||
| UserDefaults.standard.set(true, forKey: key) | ||
| return .just(false) | ||
| } | ||
| return .just(true) | ||
| } |
There was a problem hiding this comment.
hasVisitedOnboarding() 메서드 내부에서 값을 조회함과 동시에 UserDefaults에 true를 저장하는 부작용(Side Effect)이 발생하고 있습니다. 이로 인해 사용자가 온보딩 화면을 완전히 확인하지 않고 앱을 강제 종료하더라도 다음 진입 시 온보딩이 다시 표시되지 않는 문제가 발생할 수 있습니다.
조회(Query)와 명령(Command)을 분리하여, hasVisitedOnboarding()은 단순히 값만 반환하고, 실제 온보딩을 완료하거나 건너뛰었을 때 markOnboardingVisited()를 호출하여 값을 저장하도록 수정하는 것이 안전합니다.
func hasVisitedOnboarding() -> Observable<Bool> {
let hasVisited = UserDefaults.standard.bool(forKey: key)
return .just(hasVisited)
}| case .viewWillAppear: | ||
| return collectionRepository.fetchCollectionList(sort: currentState.selectedSort?.sortParameter) | ||
| .map { .setListData($0) } |
There was a problem hiding this comment.
collectionRepository.fetchCollectionList API 요청 시 에러가 발생하면 스트림이 종료되어 화면이 먹통이 될 수 있습니다. .catch 블록을 추가하여 에러 발생 시에도 스트림이 유지되도록 방어 코드를 작성하는 것이 좋습니다.
| case .viewWillAppear: | |
| return collectionRepository.fetchCollectionList(sort: currentState.selectedSort?.sortParameter) | |
| .map { .setListData($0) } | |
| case .viewWillAppear: | |
| return collectionRepository.fetchCollectionList(sort: currentState.selectedSort?.sortParameter) | |
| .map { .setListData($0) } | |
| .catch { _ in .empty() } |
| case .viewWillAppear: | ||
| return collectionRepository.fetchCollection(id: currentState.collection.collectionId) | ||
| .map { .setItems($0) } |
There was a problem hiding this comment.
collectionRepository.fetchCollection API 요청 시 에러가 발생하면 스트림이 종료되어 화면이 먹통이 될 수 있습니다. .catch 블록을 추가하여 에러 발생 시에도 스트림이 유지되도록 방어 코드를 작성하는 것이 좋습니다.
| case .viewWillAppear: | |
| return collectionRepository.fetchCollection(id: currentState.collection.collectionId) | |
| .map { .setItems($0) } | |
| case .viewWillAppear: | |
| return collectionRepository.fetchCollection(id: currentState.collection.collectionId) | |
| .map { .setItems($0) } | |
| .catch { _ in .empty() } |
| reactor.pulse(\.$route) | ||
| .observe(on: MainScheduler.instance) | ||
| .bind(onNext: { [weak viewController] route in | ||
| switch route { | ||
| case .toMain: | ||
| onMoveToMain?() | ||
| viewController?.navigationController?.popToRootViewController(animated: true) | ||
| default: | ||
| break | ||
| } | ||
| }) | ||
| .disposed(by: viewController.disposeBag) |
There was a problem hiding this comment.
현재 .toMain 경로로의 화면 전환 로직이 CollectionDetailFactoryImpl 내부에서 처리되고 있으며, 다른 경로(.dismiss, .edit 등)는 CollectionDetailViewController 내부에서 처리되고 있어 내비게이션 로직이 분산되어 있습니다.
일관성 있는 유지보수를 위해 모든 화면 전환 로직을 CollectionDetailViewController 내부로 통합하거나, Coordinator 패턴 등을 도입하여 한 곳에서 관리하는 것이 좋습니다. 만약 팩토리에서 클로저를 전달받아야 한다면, CollectionDetailViewController에 클로저를 주입하여 컨트롤러 내부에서 호출하도록 수정하는 것을 권장합니다.
| self.viewControllers = type.pageTabList.enumerated().map { index, tabType in | ||
| if index == 1 { | ||
| return collectionListFactory.make() | ||
| } else { | ||
| return bookmarkListFactory.make(type: tabType, listType: type) | ||
| } | ||
| } |
There was a problem hiding this comment.
index == 1과 같이 하드코딩된 인덱스를 기준으로 CollectionList 뷰 컨트롤러를 생성하고 있습니다. 만약 향후 탭의 순서가 변경되거나 새로운 탭이 추가되는 경우 버그가 발생하기 쉽습니다.
index 대신 tabType == .collection과 같이 실제 탭 타입을 기준으로 분기 처리하는 것이 훨씬 안전하고 직관적입니다.
| self.viewControllers = type.pageTabList.enumerated().map { index, tabType in | |
| if index == 1 { | |
| return collectionListFactory.make() | |
| } else { | |
| return bookmarkListFactory.make(type: tabType, listType: type) | |
| } | |
| } | |
| self.viewControllers = type.pageTabList.map { tabType in | |
| if tabType == .collection { | |
| return collectionListFactory.make() | |
| } else { | |
| return bookmarkListFactory.make(type: tabType, listType: type) | |
| } | |
| } |
|
고생하셨습니다!! 제미나이 리뷰 한번 쫙 확인해주시고 DictionaryTabControllable 관련하여 제 모듈 분리 작업과 중복 사항 있을 수 있어보입니다. 해당 부분만 병합 후에 하나로 통일 시키면 좋을 것 같아요 :) |
2 participants
📌 이슈
✅ 작업 사항
1. MLSBookmarkFeature SPM 패키지 생성
북마크 관련 코드를 독립 SPM 패키지로 분리하고 4개의 타깃으로 구성했습니다.
2. 모듈 구조 — Interface 타깃으로 계약 통합
외부 소비자(본앱 등)가 알아야 하는 모든 것을
MLSBookmarkFeatureInterface로 통합했습니다.3. Presentation — 전체 화면 구성
BookmarkMainBookmarkListCollectionListCollectionDetailCollectionEditCollectionSettingBookmarkModalAddCollectionBookmarkOnBoarding4. Data 레이어 — API 연동 구조 세팅
엔드포인트 및 Repository 구현체를 패키지 내부에 배치했습니다. 본앱 연동 시 실제
NetworkProvider를 주입받아 동작합니다.BookmarkEndPointCollectionEndPoint5. MLSBookmarkFeatureTesting — Mock / Stub 모듈화
테스트 및 Example 앱에서 재사용 가능하도록 모든 Mock과 Stub을
MLSBookmarkFeatureTesting타깃으로 통합했습니다.MockBookmarkRepositoryMockCollectionRepositoryMockBookmarkAuthRepositoryMockBookmarkUserDefaultsRepositoryMockParseItemFilterResultUseCaseStubDictionaryDetailFactoryStubSortedBottomSheetFactoryStubItemFilterBottomSheetFactoryStubMonsterFilterBottomSheetFactoryStubLoginFactory6. 단위 테스트 작성
비즈니스 로직이 있는 Reactor 레이어에 집중해 테스트를 작성했습니다.
BookmarkListReactorTests7. MLSCore — DictionaryTabControllable 추가
BookmarkMain에서 탭바 탭 전환을 제어하기 위해MLSCore에 프로토콜과 레지스트리를 추가했습니다.8. MLSBookmarkFeatureExample 앱 구성
개별 화면을 독립적으로 확인할 수 있도록 Example 앱을 구성했습니다.
SceneDelegate에서DIContainer를 통해 모든 Factory 등록 (Mock 주입)MenuViewController— 9개 화면으로 바로 진입 가능한 메뉴 리스트LaunchScreen.storyboard추가 — 풀스크린 적용bottomInset: 0으로 풀스크린스크린샷
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2026-05-29.at.14.59.12.mov