[Fix] QA 최종 반영 #215
[Fix] QA 최종 반영 #215
Conversation
WalkthroughThis update introduces a modular app version management and update requirement feature. It adds new entities, repositories, and use cases for version checking and update alert policies. The dependency injection and environment configuration logic are refactored, and the update check flow in the app's scene delegate is replaced with a new use case-driven approach. Profile image tap-to-detail functionality is also added. Changes
Sequence Diagram(s)sequenceDiagram
participant SceneDelegate
participant CheckAppUpdateRequirementUseCase
participant AppVersionRepository
participant UpdateAlertPolicyRepository
SceneDelegate->>CheckAppUpdateRequirementUseCase: execute()
CheckAppUpdateRequirementUseCase->>AppVersionRepository: fetchAppStoreVersion()
AppVersionRepository-->>CheckAppUpdateRequirementUseCase: AppStoreVersion
CheckAppUpdateRequirementUseCase->>AppVersionRepository: fetchCurrentVersion()
AppVersionRepository-->>CheckAppUpdateRequirementUseCase: CurrentVersion
CheckAppUpdateRequirementUseCase->>UpdateAlertPolicyRepository: hasSeenOptionalAlert()
UpdateAlertPolicyRepository-->>CheckAppUpdateRequirementUseCase: Bool
CheckAppUpdateRequirementUseCase-->>SceneDelegate: UpdateRequirement
SceneDelegate->>SceneDelegate: showUpdateAlert(for: requirement)
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🧹 Nitpick comments (4)
Wable-iOS/Domain/Entity/AppVersion.swift (1)
15-23: Consider input validation and error handling.The current implementation silently ignores invalid version components and defaults to 0, which may mask malformed version strings. Consider adding validation or throwing errors for invalid inputs.
init(from versionString: String) { + guard !versionString.isEmpty else { + self.major = 0 + self.minor = 0 + self.patch = 0 + return + } + let components = versionString .split(separator: ".") - .compactMap { Int($0) } + .compactMap { + let intValue = Int($0) + return intValue != nil && intValue! >= 0 ? intValue : nil + } self.major = components.count > 0 ? components[0] : 0 self.minor = components.count > 1 ? components[1] : 0 self.patch = components.count > 2 ? components[2] : 0 }Wable-iOS/App/AppDelegate+InjectDependency.swift (1)
67-69: Fix SwiftLint warning: Replace unused parameter with underscore.The
envparameter is not used in the closure and should be replaced with an underscore to follow Swift conventions.- diContainer.register(for: GhostRepository.self) { env in + diContainer.register(for: GhostRepository.self) { _ in return GhostRepositoryImpl() }🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 67-67: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
Wable-iOS/Domain/UseCase/AppVersion/CheckAppUpdateRequirementUseCase.swift (2)
18-32: Consider using Comparable for cleaner version comparison.The version comparison logic could be simplified if
AppVersionimplementedComparable. This would make the code more readable and less error-prone.If
AppVersionimplementsComparable, this logic could be simplified to:func execute() async throws -> UpdateRequirement { let appStoreVersion = try await appVersionRepository.fetchAppStoreVersion() let currentVersion = appVersionRepository.fetchCurrentVersion() let isAlreadyShown = updateAlertPolicyRepository.hasSeenOptionalAlert() - let requirement: UpdateRequirement - if appStoreVersion.major > currentVersion.major { - requirement = .force - } else if appStoreVersion.minor > currentVersion.minor { - requirement = .frequent - } else if appStoreVersion.patch > currentVersion.patch { - requirement = .optional - } else { - requirement = .none - } + guard appStoreVersion > currentVersion else { + return .none + } + + let requirement: UpdateRequirement + if appStoreVersion.major > currentVersion.major { + requirement = .force + } else if appStoreVersion.minor > currentVersion.minor { + requirement = .frequent + } else { + requirement = .optional + }
34-43: Simplify the optional update logic.The conditional logic for handling optional updates can be simplified for better readability.
- if requirement == .optional, isAlreadyShown { - return .none - } - - if requirement == .optional, !isAlreadyShown { - updateAlertPolicyRepository.markOptionalAlertShown() - return .optional - } - - return requirement + if requirement == .optional { + if isAlreadyShown { + return .none + } else { + updateAlertPolicyRepository.markOptionalAlertShown() + return .optional + } + } + + return requirement
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
Wable-iOS.xcodeproj/project.pbxproj(19 hunks)Wable-iOS/App/AppDelegate+InjectDependency.swift(3 hunks)Wable-iOS/App/SceneDelegate.swift(3 hunks)Wable-iOS/Core/DI/AppEnvironment.swift(1 hunks)Wable-iOS/Core/DI/BuildConfiguration.swift(0 hunks)Wable-iOS/Core/DI/DIContainer.swift(2 hunks)Wable-iOS/Core/DI/Injected.swift(1 hunks)Wable-iOS/Core/Literals/String/StringLiterals+Update.swift(1 hunks)Wable-iOS/Data/RepositoryImpl/AppVersionRepositoryImpl.swift(1 hunks)Wable-iOS/Data/RepositoryImpl/UpdateAlertPolicyRepositoryImpl.swift(1 hunks)Wable-iOS/Domain/Entity/AppVersion.swift(1 hunks)Wable-iOS/Domain/Enum/UpdateRequirement.swift(1 hunks)Wable-iOS/Domain/RepositoryInterface/AppVersionRepository.swift(1 hunks)Wable-iOS/Domain/RepositoryInterface/UpdateAlertPolicyRepository.swift(1 hunks)Wable-iOS/Domain/UseCase/AppVersion/CheckAppUpdateRequirementUseCase.swift(1 hunks)Wable-iOS/Presentation/Profile/My/View/MyProfileViewController.swift(1 hunks)Wable-iOS/Presentation/Profile/Other/View/OtherProfileViewController.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Wable-iOS/Core/DI/BuildConfiguration.swift
🧰 Additional context used
🧬 Code Graph Analysis (5)
Wable-iOS/Domain/RepositoryInterface/UpdateAlertPolicyRepository.swift (1)
Wable-iOS/Data/RepositoryImpl/UpdateAlertPolicyRepositoryImpl.swift (4)
hasSeenOptionalAlert(20-22)hasSeenOptionalAlert(32-34)markOptionalAlertShown(24-26)markOptionalAlertShown(36-36)
Wable-iOS/Core/DI/Injected.swift (1)
Wable-iOS/Core/DI/DIContainer.swift (1)
resolve(47-60)
Wable-iOS/Domain/RepositoryInterface/AppVersionRepository.swift (1)
Wable-iOS/Data/RepositoryImpl/AppVersionRepositoryImpl.swift (4)
fetchAppStoreVersion(17-37)fetchAppStoreVersion(49-51)fetchCurrentVersion(39-43)fetchCurrentVersion(53-55)
Wable-iOS/Domain/UseCase/AppVersion/CheckAppUpdateRequirementUseCase.swift (2)
Wable-iOS/Data/RepositoryImpl/AppVersionRepositoryImpl.swift (4)
fetchAppStoreVersion(17-37)fetchAppStoreVersion(49-51)fetchCurrentVersion(39-43)fetchCurrentVersion(53-55)Wable-iOS/Data/RepositoryImpl/UpdateAlertPolicyRepositoryImpl.swift (4)
hasSeenOptionalAlert(20-22)hasSeenOptionalAlert(32-34)markOptionalAlertShown(24-26)markOptionalAlertShown(36-36)
Wable-iOS/App/AppDelegate+InjectDependency.swift (1)
Wable-iOS/Core/DI/DIContainer.swift (2)
register(35-37)register(39-41)
🪛 SwiftLint (0.57.0)
Wable-iOS/App/AppDelegate+InjectDependency.swift
[Warning] 67-67: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
🔇 Additional comments (24)
Wable-iOS/Core/DI/AppEnvironment.swift (1)
10-13: LGTM! Clean and semantically improved enum definition.The replacement of
BuildConfigurationwithAppEnvironmentprovides better semantic clarity. Usingmockandproductioncases is more descriptive thandebugandreleasefor dependency injection contexts.Wable-iOS/Domain/RepositoryInterface/UpdateAlertPolicyRepository.swift (1)
10-13: LGTM! Well-designed repository interface.The protocol follows the single responsibility principle with a clean, focused interface for managing update alert policies. Method signatures are appropriately designed with meaningful names and correct return types.
Wable-iOS/Domain/Enum/UpdateRequirement.swift (1)
10-15: LGTM! Logical and well-structured enum design.The enum cases form a clear hierarchy representing different levels of update necessity. The progression from
none→optional→frequent→forceprovides intuitive semantics for the update checking system.Wable-iOS/Presentation/Profile/Other/View/OtherProfileViewController.swift (1)
213-217: LGTM! Properly implemented image tap handler.The implementation correctly:
- Uses
[weak self]to prevent retain cycles- Guards against nil image before presentation
- Follows iOS conventions for modal presentation with animation
- Provides smooth user experience for photo detail viewing
Wable-iOS/Presentation/Profile/My/View/MyProfileViewController.swift (1)
214-218: LGTM! Good implementation of image tap functionality.The implementation correctly follows iOS best practices:
- Uses weak
selfcapture to prevent retain cycles- Safely unwraps the image with a guard statement
- Presents the detail view with animation
This enhances the user experience by allowing users to view profile content images in detail.
Wable-iOS/Domain/RepositoryInterface/AppVersionRepository.swift (1)
10-13: Well-designed protocol for app version management.The protocol design is excellent:
- Clear separation of concerns between App Store and current version fetching
- Appropriate use of
async throwsfor network operations- Clean abstraction that supports dependency injection and testing
- Follows Swift naming conventions
This provides a solid foundation for the app update requirement feature.
Wable-iOS/Core/DI/Injected.swift (1)
14-15: LGTM! Consistent refactoring to AppEnvironment.The parameter change from
BuildConfigurationtoAppEnvironmentis well-executed:
- The default value change from
.releaseto.productionis semantically appropriate- Consistent with the broader dependency injection refactoring
- Maintains the same functionality while supporting the new environment-based approach
This change properly supports the new app versioning feature's dependency injection requirements.
Wable-iOS/Core/Literals/String/StringLiterals+Update.swift (1)
15-16: LGTM! Improved string formatting for better UX.The string literal updates enhance the user experience:
- The title "업데이트 안내" (Update Notice) is more descriptive and informative
- The reformatted message with additional line breaks improves readability
- These changes align well with the new app update notification system
The updated strings provide clearer communication to users about app updates.
Wable-iOS/App/AppDelegate+InjectDependency.swift (2)
22-29: LGTM! Consistent environment parameter usage.The migration from
configtoenvparameter and the switch fromBuildConfigurationtoAppEnvironmentcases is correctly implemented.
71-89: Well-structured new repository registrations.The new
AppVersionRepositoryandUpdateAlertPolicyRepositoryregistrations follow the established pattern and properly implement environment-based instantiation.Wable-iOS/Data/RepositoryImpl/UpdateAlertPolicyRepositoryImpl.swift (2)
10-27: Excellent implementation following best practices.The repository implementation demonstrates good design:
- Dependency injection of
UserDefaultsenables testability- Private static key prevents external access
- Clean, focused interface for managing update alert policy
- Proper separation of concerns
31-37: Good mock implementation for testing.The mock implementation correctly returns
falseforhasSeenOptionalAlert()and provides a no-op implementation formarkOptionalAlertShown(), which is appropriate for testing scenarios.Wable-iOS/Domain/UseCase/AppVersion/CheckAppUpdateRequirementUseCase.swift (1)
14-16: Well-designed use case with proper dependency injection.The use case follows clean architecture principles with clear separation of concerns and proper dependency injection using the
@Injectedproperty wrapper.Wable-iOS/Data/RepositoryImpl/AppVersionRepositoryImpl.swift (2)
39-43: LGTM!Good implementation with appropriate fallback handling for missing version info.
48-56: LGTM!Good practice to provide a mock implementation for testing.
Wable-iOS/Core/DI/DIContainer.swift (1)
14-14: LGTM! Consistent refactoring from BuildConfiguration to AppEnvironment.The changes properly update all references from
BuildConfigurationtoAppEnvironment, including:
- Protocol method signatures
- Implementation methods
- Environment check from
.releaseto.production- Error messages
The refactoring maintains the same functionality while aligning with the new naming convention.
Also applies to: 19-19, 39-39, 47-60
Wable-iOS/App/SceneDelegate.swift (2)
189-204: LGTM! Well-structured alert handling.The method properly handles different update requirements:
- Shows cancel option only for non-mandatory updates (frequent/optional)
- Always provides update option
- Clean separation of UI logic
206-215: LGTM! Good extraction with proper validation.The method properly validates the URL and checks if it can be opened before attempting to open the App Store. Good error logging for debugging.
Wable-iOS.xcodeproj/project.pbxproj (6)
255-261: PBXBuildFile entries for app update feature
The newly addedPBXBuildFileentries for AppVersion, UpdateRequirement, the repositories, use case implementations, andAppEnvironment.swiftare correctly declared. These ensure that Xcode knows to compile and bundle the new Swift files.Also applies to: 358-358
621-627: PBXFileReference entries for new source files
ThePBXFileReferenceentries forAppVersion.swift,UpdateRequirement.swift, the repository interfaces and implementations, andAppEnvironment.swiftare accurately added under the appropriate groups.Also applies to: 722-722
828-828: DI group updated with AppEnvironment
AppEnvironment.swifthas been added to theDIgroup, replacing the former build-configuration approach. The group path and sourceTree settings align with other DI artifacts.
1474-1474: Domain/UseCase group extended
AnAppVersionsubgroup is now referenced in theDomain/UseCasegroup, preparing the directory structure for version-checking use cases.
1675-1682: AppVersion PBXGroup configuration
The newAppVersionPBXGroup is correctly defined, containingCheckAppUpdateRequirementUseCase.swift. Itspath = AppVersionandsourceTree = "<group>"settings are consistent with the rest of the domain hierarchy.
2660-2660: Sources build phase entries for update logic
All new Swift files related to forced-update functionality (UpdateAlertPolicyRepository.swift,AppVersionRepository.swift,CheckAppUpdateRequirementUseCase.swift,AppVersionRepositoryImpl.swift,UpdateAlertPolicyRepositoryImpl.swift,AppVersion.swift, andUpdateRequirement.swift) are properly listed in theSourcesbuild phase. This guarantees their inclusion in the compilation process.Also applies to: 2673-2673, 2747-2747, 2855-2855, 2882-2882, 2924-2924, 2972-2972
| struct AppVersion { | ||
| let major: Int | ||
| let minor: Int | ||
| let patch: Int | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Comparable conformance for version comparisons.
The AppVersion struct should implement Comparable to enable direct version comparisons, which would be useful for the update requirement logic.
-struct AppVersion {
+struct AppVersion: Comparable {
let major: Int
let minor: Int
let patch: Int
+
+ static func < (lhs: AppVersion, rhs: AppVersion) -> Bool {
+ if lhs.major != rhs.major {
+ return lhs.major < rhs.major
+ }
+ if lhs.minor != rhs.minor {
+ return lhs.minor < rhs.minor
+ }
+ return lhs.patch < rhs.patch
+ }📝 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.
| struct AppVersion { | |
| let major: Int | |
| let minor: Int | |
| let patch: Int | |
| struct AppVersion: Comparable { | |
| let major: Int | |
| let minor: Int | |
| let patch: Int | |
| static func < (lhs: AppVersion, rhs: AppVersion) -> Bool { | |
| if lhs.major != rhs.major { | |
| return lhs.major < rhs.major | |
| } | |
| if lhs.minor != rhs.minor { | |
| return lhs.minor < rhs.minor | |
| } | |
| return lhs.patch < rhs.patch | |
| } |
🤖 Prompt for AI Agents
In Wable-iOS/Domain/Entity/AppVersion.swift around lines 10 to 14, the
AppVersion struct lacks Comparable conformance, which is needed for direct
version comparisons. Implement the Comparable protocol by adding a static
function that compares two AppVersion instances, comparing major, then minor,
then patch values in order. This will enable natural sorting and comparison of
versions.
- 에러 매핑 또한 추가
| enum AppEnvironment { | ||
| case mock | ||
| case production | ||
| } |
There was a problem hiding this comment.
debug와 release가 아닌 mock과 production으로 설정하신 이유가 있을까요?
There was a problem hiding this comment.
DI 자체에 대해서, debug이어도 Mock으로 앱을 실행할 수도 있고, 실제 객체로 앱을 실행할 수도 있기 때문에 혼동이 올 수 있을거라 판단했습니다.
그래서 아예 네이밍을 직관적으로 mock 혹은 production으로 명칭하였습니다.
| enum UpdateRequirement { | ||
| case none | ||
| case optional | ||
| case frequent | ||
| case force | ||
| } |
👻 PULL REQUEST
📄 작업 내용
💻 주요 코드 설명
강제 업데이트를 위한 객체 선언과 구조 설계
관련 코드는 커밋 기록을 확인해 주세요!
👀 기타 더 이야기해볼 점
🔗 연결된 이슈
Summary by CodeRabbit
New Features
Improvements
Refactor