MLSDictionaryFeature 모듈을 구성합니다.#334
Conversation
This reverts commit 4f85137.
DI 구조 정리 / 데모앱 구성 / 의존성 정리 / 단순래핑 useCase 제거 / 테스트 코드 필요
pinocchio22
added
feat
![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 MLSDictionaryFeature module, which implements the main dictionary views, detail views for items, monsters, maps, NPCs, and quests, search functionality, and notification management. The code reviewer identified several critical and high-severity issues, particularly regarding Rx execution timing and potential runtime crashes. Specifically, dynamically appending to contentViews based on asynchronous network responses can cause index out-of-bounds crashes, and several Rx operations (such as addRecentSearch and CheckLoginUseCase.execute) are executing synchronously outside the stream instead of being deferred to subscription time. Additionally, pagination logic in scroll views lacks proper loading state guards or timestamp updates, which can trigger duplicate network requests.
dongglehada
left a comment
dongglehada
left a comment
There was a problem hiding this comment.
수고 많으셨습니다!!
모듈 이전은 diff가 너무커서 pr을 확인하기가 어렵네요 ㅎㅎ..
AI로 받은 리뷰내용만 공유 드립니다! 참고만 부탁드려요!
🔴 Critical
-
navigatTo 오타 — DictionaryListReactor, ItemDictionaryDetailReactor
Mutation case 이름이 navigateTo가 아니라 navigatTo로 되어있음. 오타인데 전체적으로 통일돼있어서 빌드는 되지만, 다른 Reactor들과 이름이 달라서 검색/추적이 어려움. -
addRecentSearch — Rx 지연 실행 원칙 위반 (RecentSearchRepositoryImpl)
Completable을 반환하지만 UserDefaults 쓰기가 구독 시점이 아니라 함수 호출 시점에 즉시 실행됨. 구독 전에 side effect가 발생. Completable.create 안으로 감싸야 함. (removeRecentSearch는 올바르게 감싸져 있는데
addRecentSearch만 빠져있음) -
DictionaryDetailFactoryImpl — 자기 자신 순환 참조
var detailFactory: DictionaryDetailFactoryImpl!
detailFactory = DictionaryDetailFactoryImpl(
dictionaryDetailFactory: { detailFactory } // strong 캡처
)
detailFactory가 자신을 strong capture하는 클로저를 들고 있어 retain cycle. SceneDelegate에서도 동일하게 구성됨. [weak detailFactory]로 약한 참조하거나 구조 재검토 필요.
🟠 High
-
Index out of bounds — MonsterDictionaryDetailReactor
return .just(.navigateTo(.detail(type: .item, id: currentState.dropItems[index].itemId)))
배열 범위 체크 없이 index로 직접 접근. 빠른 탭이나 비동기 타이밍 이슈로 crash 가능. -
Info.plist에 UILaunchStoryboardName 누락
LaunchScreen.storyboard를 추가했지만 Info.plist에 키가 없어서 iOS가 인식 못 함. 결과적으로 앱이 letterbox(호환성) 모드로 실행되어 풀스크린이 아닌 검은 여백 발생. -
DictionaryDetailBaseViewController — firstStickyIndexButton 잘못된 할당
if index == 0 {
firstIndexButton = button
firstStickyIndexButton = button // ← stickyButton을 넣어야 하는데 button이 들어감
}
stickyButton용 변수에 일반 button이 들어가는 버그. -
MLSDictionaryFeature가 MLSMyPageFeature에 의존
Package.swift를 보면 MLSDictionaryFeature 타깃이 MLSMyPageFeatureInterface에 의존하고 있음. 도감 기능이 마이페이지 모듈에 의존하는 건 모듈 경계가 흐릿해지는 것. fetchProfileUseCase 때문인데, 공통 Interface
계층으로 빼거나 DictionaryFeatureInterface에 자체 프로토콜 정의가 필요함.
🟡 Medium
-
MLSMyPageFeatureTesting import — Example 앱
MLSDictionaryFeatureExample이 MLSMyPageFeatureTesting을 import하고 있음. Dictionary Example이 MyPage Testing에 의존할 이유가 없음. 해당 Mock들을 MLSDictionaryFeatureTesting에 직접 두는 게 맞음. -
ViewController.swift 파일 삭제 필요
// SceneDelegate에서 직접 DictionaryMainViewController를 띄우기 때문에 사용하지 않습니다.
class ViewController: UIViewController {}
주석으로 설명하고 파일 살려두는 것보다 그냥 삭제하는 게 맞음. -
매직 넘버 상수화 — DictionaryListViewController
guard now.timeIntervalSince(lastPagingTime) > 0.5 else { return }
if offsetY > contentHeight - height - 100 {
0.5, 100 모두 상수로 빼야 의도가 명확해짐. -
테스트 누락 케이스
- toggleBookmark error case (bookmarkError route)
- undoLastDeletedBookmark 성공/실패
- pagination 연속 호출
- itemFilterOptionSelected
🟢 Low
- currentState vs self.currentState 혼용
DictionaryListReactor에서 같은 메서드 내에 self.currentState와 currentState가 혼용됨. 동작은 같지만 스타일 일관성이 떨어짐.
2 participants
📌 이슈
✅ 작업 사항
1. MLSDictionaryFeature SPM 패키지 분리 및 구조 재정비
Dictionary 기능을 독립 Feature 단위로 관리할 수 있도록 SPM 패키지 구조를 분리 및 정리했습니다. 기존 앱 내부에 혼재되어 있던 도감 관련 기능들을 하나의 Feature 모듈로 통합하고, 내부적으로 Presentation / Domain / Data / Testing / Tests 역할이 명확하게 분리되도록 구성했습니다.
이를 통해 도감 기능 수정 시 다른 영역에 미치는 영향을 최소화하고, 독립적인 개발 및 유지보수가 가능하도록 개선했습니다.
구성된 모듈:
2. Dictionary 기능 세분화 및 화면 책임 분리
도감 기능 내부를 사용자 흐름 기준으로 세분화하여 각 화면의 역할을 명확하게 분리했습니다.
구성 화면:
기존 ViewController 내부에 분산되어 있던 상태 관리 및 화면 이동 로직을 Reactor 중심으로 이동하여 화면 책임을 최소화하고 상태 흐름을 명확하게 관리할 수 있도록 개선했습니다.
주요 정리 내용:
3. 공통 컴포넌트 및 DesignSystem 정리
도감 기능 개발 과정에서 반복 사용되는 공통 UI 컴포넌트를 DesignSystem 및 Core 계층으로 정리했습니다.
추가 / 이동된 컴포넌트:
또한 Dictionary 기능 내부에서 사용되던 셀 컴포넌트의 재사용 가능성을 고려하여 구조를 재정비했습니다.
검토 대상:
현재는 DictionaryFeature 내부 Presentation 계층에 위치하지만, Bookmark 기능 등 다른 Feature에서도 사용될 가능성이 있어 이후 공통 모듈 이동 여부를 검토 중입니다.
4. Navigation 구조 개선 및 Registry 도입
Dictionary 탭 이동을 위해 MLSCore 내부 Navigation 계층에 다음 구조를 추가했습니다.
추가 구성:
Feature 모듈 분리 이후에도 Tab 이동 의존성을 최소화하기 위해 Registry 기반 접근 구조를 적용했습니다.
다만 현재 위치가 MLSCore.Navigation이 적절한지에 대해서는 추가 검토 중이며, 이후 FeatureRouter 또는 AppCoordinator 중심 구조로 개선 가능성을 고려하고 있습니다.
5. ImageLoader MainActor 보장 수정
비동기 이미지 로딩 시 completion closure의 Main Thread 보장이 명확하지 않았던 문제를 수정했습니다.
기존:
수정:
이를 통해 이미지 로딩 이후 발생할 수 있는 UI 업데이트 관련 race condition 가능성을 줄였습니다.
6. Mock 객체 분리 및 Testing 모듈 재사용성 강화
여러 모듈에 분산되어 있는 Mock 객체들의 위치 재정의가 필요합니다.
추가 Mock:
7. 단위 테스트 작성 및 회귀 안정성 확보
구조 개선과 함께 주요 상태 흐름 및 비즈니스 로직에 대한 단위 테스트를 추가했습니다.
UseCase
Reactor
Detail Reactor
각 Detail 화면별:
등을 검증하여 회귀 안정성을 확보했습니다.