[Fix] DIContainer 수정 및 소식 탭 앰플리튜드 태그 추가 #150
Conversation
WalkthroughThis pull request integrates a new dependency injection mechanism and analytics tracking into the iOS application. A new file is added to extend the AppDelegate with DI logic, and the project file is updated to include it. The DIContainer registration has been simplified to remove unnecessary nil-checks. Additionally, the app launch sequence now calls the injection method and the Overview page controller is updated to track page change events for analytics. Changes
Sequence Diagram(s)sequenceDiagram
participant AppDelegate
participant Injector as "AppDelegate+InjectDependency"
participant DIContainer
participant App as "Application Lifecycle"
App->>AppDelegate: Launch Application
AppDelegate->>Injector: Call injectDependency()
Injector->>DIContainer: Utilize dependency registration
DIContainer-->>Injector: Return DI container
Injector-->>AppDelegate: Dependencies injected
sequenceDiagram
participant VC as "OverviewPageViewController"
participant Tracker as "trackPageChangeEvent()"
participant Analytics as "Analytics Module"
VC->>Tracker: currentIndex updated (new value ≠ old value)
Tracker->>Analytics: Switch-case triggers event tracking (indices 0-3)
Analytics-->>Tracker: Event logged
Assessment against linked issues
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Wable-iOS.xcodeproj/project.pbxproj (1)
1071-1813: 💡 Verification agent🧩 Analysis chain
Verify the implementation of the dependency injection mechanism.
The PR mentions that all dependencies must be registered at app launch to prevent crashes when resolving unregistered types. Since we can't see the actual implementation of
AppDelegate+InjectDependency.swift, ensure it handles edge cases appropriately.
🏁 Script executed:
#!/bin/bash # Description: Check implementation of the new AppDelegate+InjectDependency.swift file # Check how DIContainer and injectDependency are implemented echo "Checking AppDelegate+InjectDependency.swift implementation..." cat "Wable-iOS/App/AppDelegate+InjectDependency.swift" # Check how the injection is called in AppDelegate echo -e "\n\nChecking AppDelegate.swift for injection method call..." rg -A 3 -B 3 "injectDependency" "Wable-iOS/App/AppDelegate.swift" # Check for Amplitude tags in news tab echo -e "\n\nChecking for Amplitude tags implementation in news tab..." rg -A 3 "Amplitude" "Wable-iOS/Presentation/Overview/News/View/NewsViewController.swift" || echo "No direct Amplitude references found in NewsViewController" rg -A 3 "track" "Wable-iOS/Presentation/Overview/News/View/NewsViewController.swift" || echo "No tracking method calls found in NewsViewController" rg "OverviewPageViewController" "Wable-iOS/Presentation/Overview/Page/View/OverviewPageViewController.swift" -A 10 | rg -e "Amplitude|track|segment" || echo "No tracking references found in OverviewPageViewController"Length of output: 2009
Dependency Injection Implementation Requires Completion
- The file
Wable-iOS/App/AppDelegate+InjectDependency.swiftstill contains only a placeholder (// TODO: 객체를 주입) and does not register any dependencies.- While
injectDependency()is correctly called inAppDelegate.swift, the lack of an actual implementation could lead to unresolved types at runtime.- Additionally, although no direct Amplitude references are found in
NewsViewController.swift, tracking calls do exist inOverviewPageViewController.swift. Please ensure that the DI mechanism and event tracking integrations are consistent across the app.Please implement the dependency registration logic to fully initialize the DI container and handle potential edge cases.
🧹 Nitpick comments (2)
Wable-iOS/App/AppDelegate.swift (1)
35-35: Empty implementation for didDiscardSceneSessionsThe method has been simplified to an empty implementation. While this is valid since the method is optional, consider adding a comment explaining why no implementation is needed, or restore the previous comments if they provided useful context.
- func application(_ application: UIApplication, didDiscardSceneSessions sceneSessions: Set<UISceneSession>) {} + func application(_ application: UIApplication, didDiscardSceneSessions sceneSessions: Set<UISceneSession>) { + // This method is called when the user discards a scene session. + // No additional handling needed at this time. + }Wable-iOS/Presentation/Overview/Page/View/OverviewPageViewController.swift (1)
204-217: Amplitude tracking implementation meets requirementsThe implementation properly tracks different events based on the selected tab index. This fulfills the PR objective of adding Amplitude tags for tracking segment changes in the news tab.
Two suggestions to improve robustness:
func trackPageChangeEvent(for index: Int) { + // Map index to tab name for more readable tracking + let tabNames = ["gameschedule", "ranking", "news", "announcement"] + + guard index >= 0 && index < tabNames.count else { return } + + AmplitudeManager.shared.trackEvent(tag: "click_\(tabNames[index])") - switch index { - case 0: - AmplitudeManager.shared.trackEvent(tag: "click_gameschedule") - case 1: - AmplitudeManager.shared.trackEvent(tag: "click_ranking") - case 2: - AmplitudeManager.shared.trackEvent(tag: "click_news") - case 3: - AmplitudeManager.shared.trackEvent(tag: "click_announcement") - default: - break - } }This approach is more maintainable as it centralizes the tab names in one place and adds a safety check for index bounds.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Wable-iOS.xcodeproj/project.pbxproj(4 hunks)Wable-iOS/App/AppDelegate+InjectDependency.swift(1 hunks)Wable-iOS/App/AppDelegate.swift(2 hunks)Wable-iOS/Core/DI/DIContainer.swift(1 hunks)Wable-iOS/Presentation/Overview/Page/View/OverviewPageViewController.swift(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Wable-iOS/App/AppDelegate.swift (1)
Wable-iOS/App/AppDelegate+InjectDependency.swift (1)
injectDependency(13-17)
🪛 SwiftLint (0.57.0)
Wable-iOS/App/AppDelegate+InjectDependency.swift
[Warning] 15-15: TODOs should be resolved (객체를 주입)
(todo)
🔇 Additional comments (7)
Wable-iOS/App/AppDelegate+InjectDependency.swift (1)
10-12: Computation propertydiContainerlooks goodThe property correctly returns the shared instance of the AppDIContainer, making it easily accessible throughout the AppDelegate.
Wable-iOS/Core/DI/DIContainer.swift (1)
39-41: Simplified dependency registration looks goodThe dependency registration has been simplified by directly assigning the resolver closure to the dependencies dictionary without unnecessary wrapping. This approach is cleaner and reduces complexity.
The change aligns with the PR's objective to revise how closures are stored in the DIContainer.
Wable-iOS/App/AppDelegate.swift (1)
18-19: Dependency injection at startup looks goodAdding the
injectDependency()call before initializing the Kakao SDK ensures that all dependencies are properly registered at app launch.Wable-iOS/Presentation/Overview/Page/View/OverviewPageViewController.swift (1)
16-21: Observer for tracking page changes looks goodThe property observer effectively triggers the tracking function whenever the current index changes, with a guard clause to prevent unnecessary tracking when the index hasn't changed.
Wable-iOS.xcodeproj/project.pbxproj (3)
217-217: New file added to handle dependency injection.This addition of
AppDelegate+InjectDependency.swiftmatches the PR objective of modifying the DIContainer. Using an extension on AppDelegate is a clean approach to separate dependency injection concerns from the main AppDelegate implementation.
459-459: File reference added for AppDelegate+InjectDependency.This file reference properly includes the new extension file in the project's file references section, ensuring it's properly tracked by Xcode.
1071-1071: Extension file appropriately added to App group.The
AppDelegate+InjectDependency.swiftfile is correctly placed in the App group alongside the main AppDelegate, maintaining good project organization. This follows best practices for organizing related files together.
| func injectDependency() { | ||
|
|
||
| // TODO: 객체를 주입 | ||
|
|
||
| } |
There was a problem hiding this comment.
Implement the TODO for dependency injection
The method is currently empty with only a TODO comment. According to the PR description, this method should register all necessary dependencies at startup to avoid application crashes from unregistered dependencies.
func injectDependency() {
- // TODO: 객체를 주입
+ // Register your dependencies here
+ // Example:
+ // diContainer.register(for: SomeService.self) { _ in
+ // SomeServiceImplementation()
+ // }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func injectDependency() { | |
| // TODO: 객체를 주입 | |
| } | |
| func injectDependency() { | |
| // Register your dependencies here | |
| // Example: | |
| // diContainer.register(for: SomeService.self) { _ in | |
| // SomeServiceImplementation() | |
| // } | |
| } |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 15-15: TODOs should be resolved (객체를 주입)
(todo)
youz2me
left a comment
youz2me
left a comment
There was a problem hiding this comment.
고생하셨습니닷. 저도 이제 앰플리튜드를 슬슬 붙여야 하는데 진웅님 코드가 많은 도움이 될 것 같습니닷 ㅎㅎ
"Repository -> UseCase -> ViewModel -> ViewController"의 의존 관계에서 의존성 등록을 어디까지 할 것인가에 대한 논의
-> 이 부분에 대해서는 DIContainer를 이용한 의존성 주입을 해본 경험이 없어서 부가적으로 설명을 듣고 싶은데요 ..!!! 내일 회의 시간에 같이 이야기해보면 좋을 것 같습니다.
고생하셨습니다 🙇♀️
| @@ -0,0 +1,18 @@ | |||
| // | |||
| // AppDelegate+InjectDependency.swift | |||
There was a problem hiding this comment.
오... 보통 네이밍을 이런식으로 많이 짓나요? 처음 보는 네이밍이어서 여쭤봅니다요
There was a problem hiding this comment.
어떻게 해야할 지 저도 몰라서 우선 저렇게 지어보었습니다. ㅎㅎ
| func trackPageChangeEvent(for index: Int) { | ||
| switch index { | ||
| case 0: | ||
| AmplitudeManager.shared.trackEvent(tag: "click_gameschedule") | ||
| case 1: | ||
| AmplitudeManager.shared.trackEvent(tag: "click_ranking") | ||
| case 2: | ||
| AmplitudeManager.shared.trackEvent(tag: "click_news") | ||
| case 3: | ||
| AmplitudeManager.shared.trackEvent(tag: "click_announcement") | ||
| default: | ||
| break | ||
| } | ||
| } |
github-project-automation
bot
moved this from Ready for Review
to In Review
in Wable-iOS
…o fix/#149-dicontainer-overview-tag
[Fix] DIContainer 수정 및 소식 탭 앰플리튜드 태그 추가
👻 PULL REQUEST
📄 작업 내용
💻 주요 코드 설명
DIContainer + Injected 사용 방법
resolve()를 호출하게 되면fatalError가 발생하여 앱이 터지기 때문에 주의해야 합니다.AppDelegate+InjectDependency.swift
DIContainer+Injected 사용 예시
🔗 연결된 이슈
Summary by CodeRabbit
New Features
Refactor