[Feat] FCM을 이용한 푸시 알림 기능 구현 #186
Conversation
WalkthroughThis change implements and integrates push notification (알림) functionality into the iOS app. It adds FCM (Firebase Cloud Messaging) token management, notification handling in the AppDelegate, badge updates, and necessary repository and use case logic. UI components are updated to reflect notification status, and supporting code is refactored to accommodate these features. Changes
Sequence Diagram(s)sequenceDiagram
participant AppDelegate
participant Firebase
participant UserSessionRepository
participant ProfileRepository
participant UpdateFCMTokenUseCase
participant UpdateUserBadgeUseCase
participant UNUserNotificationCenter
participant ViewController
AppDelegate->>Firebase: Configure FCM, register for notifications
Firebase->>AppDelegate: Provides device token/FCM token
AppDelegate->>ProfileRepository: saveFCMToken(token)
AppDelegate->>UpdateFCMTokenUseCase: execute(nickname)
UpdateFCMTokenUseCase->>ProfileRepository: fetchFCMToken()
UpdateFCMTokenUseCase->>ProfileRepository: updateUserProfile(nickname, fcmToken)
UNUserNotificationCenter->>AppDelegate: didReceive notification
AppDelegate->>UpdateUserBadgeUseCase: execute(number)
UpdateUserBadgeUseCase->>AccountRepository: updateUserBadge(badge)
AppDelegate->>UserSessionRepository: updateUserSession(notificationBadgeCount)
AppDelegate->>ViewController: (optional) Navigate to detail view
Assessment against linked issues
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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: 10
🔭 Outside diff range comments (2)
Wable-iOS/Presentation/Home/View/HomeViewController.swift (1)
170-184:⚠️ Potential issueStrong capture of
selfandcellrisks retain cycle
contentImageViewTapHandlerstores a closure inside the cell; the closure capturesselfstrongly, while the view controller retains the collection view → cell. This cycle (VC ➜ collectionView ➜ cell ➜ closure ➜ VC) prevents both from deallocating.- contentImageViewTapHandler: { - guard let image = cell.contentImageView.image else { - return - } - - self.present(PhotoDetailViewController(image: image), animated: true) - }, + contentImageViewTapHandler: { [weak self, weak cell] in + guard + let self, + let image = cell?.contentImageView.image + else { return } + self.present(PhotoDetailViewController(image: image), animated: true) + },The same pattern appears in other handlers; consider applying
[weak self]consistently.🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 170-170: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
Wable-iOS/Presentation/WableComponent/Cell/ContentCollectionViewCell.swift (1)
35-40: 🛠️ Refactor suggestionHandlers retained across reuse may leak or trigger stale actions
contentImageViewHandler,likeButtonTapHandler, etc. are not reset inprepareForReuse().
If a handler closes over a view controller, the cell will keep that controller alive until the handler is replaced, and taps on a reused cell could call logic from the previous screen.override func prepareForReuse() { super.prepareForReuse() + // Release handlers to break retain cycles & avoid stale callbacks + contentImageViewHandler = nil + likeButtonTapHandler = nil + profileImageViewTapHandler = nil + settingButtonTapHandler = nil + ghostButtonTapHandler = nil + contentImageView.kf.cancelDownloadTask() contentImageView.image = nil contentTextView.text = nil titleTextView.text = nil }
🧹 Nitpick comments (13)
Wable-iOS/Presentation/Home/View/HomeViewController.swift (2)
29-30: Static mutable flag is not thread-safe and survives app life-cycle
hasShownLoadingScreenis mutated on the main queue but is technically accessible from any thread.
Usingstaticalso means it will never reset until the app is killed, so a subsequent log-out/login inside the same run won’t show the loading screen again, even if desired.If the intent is “once per app launch, on main thread” document it explicitly or move the flag into
AppDelegate/SceneDelegatewhere life-cycle ownership is clearer.
98-103: Guard clause reads better & prevents accidental double-presentation-if shouldShowLoadingScreen && !HomeViewController.hasShownLoadingScreen { - showLoadingScreen() - HomeViewController.hasShownLoadingScreen = true -} +guard shouldShowLoadingScreen, + !HomeViewController.hasShownLoadingScreen else { return } +HomeViewController.hasShownLoadingScreen = true +showLoadingScreen()Minor, but keeps early-exit semantics consistent with the rest of the class.
Wable-iOS/Presentation/WableComponent/Navigation/NavigationView.swift (2)
274-275:setNavigationTitleonly updates hub titles
setNavigationTitle(text:)writes tohubTitleLabel, so calling it on a.pagenavigation view has no effect.
Either:
- Rename to
setHubTitleto clarify intent, or- Add a
switchto update the correct label perNavigationType.
277-281: DRY violation: duplicated notification-badge logic
updateNotificationStatus(hasNewNotification:)repeats code already present inconfigureView()(lines 221-228). Consider extracting the image-selection into a private helper to avoid divergent behaviour when the asset names change.private func notificationIcon(for state: Bool) -> UIImage? { return state ? .icNotiBadge : .icNotiDefault }Then both places can use
notificationButton.setImage(notificationIcon(for: …), for: .normal).Wable-iOS/Presentation/Onboarding/ViewController/AgreementViewController.swift (2)
169-177: Consider adding retry logic for FCM token updates.The FCM token update implementation looks good, but it only logs errors without retry logic or user notification. Consider implementing a retry mechanism for critical operations like this.
self.updateFCMTokenUseCase.execute(nickname: owner.nickname) .sink { completion in if case .failure(let error) = completion { WableLogger.log("FCM 토큰 저장 중 에러 발생: \(error)", for: .error) + // Consider implementing retry logic here + // Or notify the user if this is a critical error that affects functionality } } receiveValue: { () in } .store(in: cancelBag)
169-177: Inconsistent references to nickname property.There's inconsistency in how the nickname is referenced - using
owner.nicknamein the FCM token update butself.nicknamein the welcome message. Consider standardizing to useselfthroughout for consistency.-self.updateFCMTokenUseCase.execute(nickname: owner.nickname) +self.updateFCMTokenUseCase.execute(nickname: self.nickname)Also applies to: 203-203
Wable-iOS/Domain/UseCase/Onboarding/UpdateFCMTokenUseCase.swift (1)
22-28: Consider implementing a retry mechanism for FCM token retrieval.The implementation correctly handles the case when the FCM token is not available by returning an error. However, since FCM token retrieval might be asynchronous or delayed in some cases, consider implementing a retry mechanism or a callback approach for situations where the token might become available shortly after this method is called.
func execute(nickname: String) -> AnyPublisher<Void, WableError> { guard let token = repository.fetchFCMToken() else { + // Consider implementing a retry mechanism here + // Example: return retryWhen condition occurs return .fail(WableError.noToken) } return repository.updateUserProfile(nickname: nickname, fcmToken: token) }Wable-iOS/Domain/RepositoryInterface/ProfileRepository.swift (1)
18-26: Method overloads are drifting – prefer a single variadic method with defaulted parametersMaintaining two
updateUserProfileoverloads (lines 18-26) invites ambiguity and duplicated implementation in concrete repositories.
Because every parameter is already optional, you can collapse them into one method with sensible defaults:func updateUserProfile( nickname: String? = nil, profile: UserProfile? = nil, isPushAlarmAllowed: Bool? = nil, isAlarmAllowed: Bool? = nil, image: UIImage? = nil, fcmToken: String? = nil, defaultProfileType: String? = nil ) -> AnyPublisher<Void, WableError>This keeps the public surface small and far less error-prone for future-callers.
Wable-iOS/Presentation/Login/LoginViewModel.swift (1)
72-90: Chain FCM-token persistence and session update to guarantee ordering and reduce duplicate code
updateFCMTokenUseCaseandupdateUserSessionUseCase.updateUserSessionare launched in parallel.
If the profile update fails but the session is already stored, the app may display stale data.
Chaining the publishers ensures “token saved → session updated” semantics and removes the inner sink:self.updateFCMTokenUseCase.execute(nickname: account.user.nickname) - .sink { completion in - if case .failure(let error) = completion { - WableLogger.log("FCM 토큰 저장 중 에러 발생: \(error)", for: .error) - } - } receiveValue: { () in - } - .store(in: cancelBag) - -self.updateUserSessionUseCase.updateUserSession( - userID: account.user.id, - nickname: account.user.nickname, - profileURL: account.user.profileURL, - isPushAlarmAllowed: account.isPushAlarmAllowed, - isAdmin: account.isAdmin, - isAutoLoginEnabled: true -) -... + .flatMap { [unowned self] _ in + self.updateUserSessionUseCase.updateUserSession( + userID: account.user.id, + nickname: account.user.nickname, + profileURL: account.user.profileURL, + isPushAlarmAllowed: account.isPushAlarmAllowed, + isAdmin: account.isAdmin, + isAutoLoginEnabled: true + ) + } + .sink { completion in + if case .failure(let error) = completion { + WableLogger.log("로그인 후 세션 저장 중 에러: \(error)", for: .error) + } + } receiveValue: { _ in + WableLogger.log("세션 저장 완료", for: .debug) + } + .store(in: cancelBag)This guarantees data consistency and simplifies the reactive flow.
Wable-iOS/App/AppDelegate.swift (1)
70-75: Resolve SwiftLint warning – discard unusederrorparameterThe closure never touches
error, so SwiftLint raisesunused_closure_parameter.- UNUserNotificationCenter.current().requestAuthorization(options: [.alert, .badge, .sound]) { [weak self] granted, error in + UNUserNotificationCenter.current().requestAuthorization(options: [.alert, .badge, .sound]) { [weak self] granted, _ inThis keeps the signature clean and silences the warning.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 70-70: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
Wable-iOS/Presentation/WableComponent/Cell/ContentCollectionViewCell.swift (1)
243-247: Allow a default value for the new tap handler to ease migrationEvery call-site now has to pass
contentImageViewTapHandler, even when not needed.
Adding a defaultnilkeeps the API source-compatible and reduces churn:- cellType: CellType = .list, - contentImageViewTapHandler: (() -> Void)?, + cellType: CellType = .list, + contentImageViewTapHandler: (() -> Void)? = nil,Wable-iOS/App/AppDelegate+Firebase.swift (2)
14-22: Firebase token registration implemented correctly.The implementation properly registers for remote notifications, sets the APNs token for Firebase Messaging, and retrieves the FCM token. However, there's an unused parameter in the closure.
- Messaging.messaging().token() { [weak self] token, error in + Messaging.messaging().token() { [weak self] token, _ in🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 17-17: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
89-107: HomeDetailViewController initialization may be too coupled.The initialization of HomeDetailViewController with its dependencies is very verbose and may indicate tight coupling.
Consider using dependency injection or a factory pattern to create view controllers with their dependencies. This would make the code more maintainable and testable.
// Example factory approach func createHomeDetailViewController(contentID: Int) -> HomeDetailViewController { return HomeDetailViewController( viewModel: HomeDetailViewModel( contentID: contentID, fetchContentInfoUseCase: FetchContentInfoUseCase(repository: contentRepository), // other dependencies... ), cancelBag: CancelBag() ) }Then you could simplify this code to:
let detailViewController = createHomeDetailViewController(contentID: contentID)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
Wable-iOS.xcodeproj/project.pbxproj(8 hunks)Wable-iOS/App/AppDelegate+Firebase.swift(1 hunks)Wable-iOS/App/AppDelegate.swift(2 hunks)Wable-iOS/App/SceneDelegate.swift(3 hunks)Wable-iOS/Data/Mapper/ContentMapper.swift(2 hunks)Wable-iOS/Data/RepositoryImpl/AccountRepositoryImpl.swift(1 hunks)Wable-iOS/Data/RepositoryImpl/ContentRepositoryImpl.swift(1 hunks)Wable-iOS/Data/RepositoryImpl/ProfileRepositoryImpl.swift(2 hunks)Wable-iOS/Data/RepositoryImpl/UserSessionRepositoryImpl.swift(2 hunks)Wable-iOS/Domain/RepositoryInterface/ContentRepository.swift(1 hunks)Wable-iOS/Domain/RepositoryInterface/ProfileRepository.swift(1 hunks)Wable-iOS/Domain/RepositoryInterface/UserSessionRepository.swift(1 hunks)Wable-iOS/Domain/UseCase/Home/FetchUserInformationUseCase.swift(1 hunks)Wable-iOS/Domain/UseCase/Login/FetchUserAuthUseCase.swift(1 hunks)Wable-iOS/Domain/UseCase/Onboarding/CreateUserProfileUseCase.swift(1 hunks)Wable-iOS/Domain/UseCase/Onboarding/UpdateFCMTokenUseCase.swift(1 hunks)Wable-iOS/Domain/UseCase/Profile/FetchContentInfoUseCase.swift(1 hunks)Wable-iOS/Domain/UseCase/Profile/UpdateUserBadgeUseCase.swift(1 hunks)Wable-iOS/Infra/Network/DTO/Response/Content/FetchContent.swift(1 hunks)Wable-iOS/Infra/Network/TargetType/AccountTargetType.swift(2 hunks)Wable-iOS/Infra/Token/TokenStorage.swift(1 hunks)Wable-iOS/Presentation/Home/View/HomeDetailViewController.swift(2 hunks)Wable-iOS/Presentation/Home/View/HomeViewController.swift(4 hunks)Wable-iOS/Presentation/Home/ViewModel/HomeDetailViewModel.swift(3 hunks)Wable-iOS/Presentation/Home/ViewModel/HomeViewModel.swift(4 hunks)Wable-iOS/Presentation/Login/LoginViewController.swift(1 hunks)Wable-iOS/Presentation/Login/LoginViewModel.swift(2 hunks)Wable-iOS/Presentation/Notification/Activity/View/ActivityNotificationViewController.swift(0 hunks)Wable-iOS/Presentation/Notification/Activity/ViewModel/ActivityNotificationViewModel.swift(3 hunks)Wable-iOS/Presentation/Notification/Information/View/InformationNotificationViewController.swift(0 hunks)Wable-iOS/Presentation/Notification/Page/View/NotificationPageViewController.swift(2 hunks)Wable-iOS/Presentation/Onboarding/ViewController/AgreementViewController.swift(3 hunks)Wable-iOS/Presentation/Viewit/Create/View/Subview/ViewitInputView.swift(1 hunks)Wable-iOS/Presentation/WableComponent/Cell/ContentCollectionViewCell.swift(4 hunks)Wable-iOS/Presentation/WableComponent/Cell/NotificationCell.swift(1 hunks)Wable-iOS/Presentation/WableComponent/Navigation/NavigationView.swift(1 hunks)Wable-iOS/Resource/Base.lproj/LaunchScreen 2.storyboard(0 hunks)
💤 Files with no reviewable changes (3)
- Wable-iOS/Presentation/Notification/Activity/View/ActivityNotificationViewController.swift
- Wable-iOS/Presentation/Notification/Information/View/InformationNotificationViewController.swift
- Wable-iOS/Resource/Base.lproj/LaunchScreen 2.storyboard
🧰 Additional context used
🧬 Code Graph Analysis (11)
Wable-iOS/Domain/UseCase/Onboarding/CreateUserProfileUseCase.swift (1)
Wable-iOS/Data/RepositoryImpl/ProfileRepositoryImpl.swift (1)
fetchFCMToken(44-50)
Wable-iOS/Data/RepositoryImpl/ContentRepositoryImpl.swift (1)
Wable-iOS/Data/Mapper/ContentMapper.swift (3)
toDomain(13-51)toDomain(53-99)toDomain(101-144)
Wable-iOS/Domain/RepositoryInterface/ContentRepository.swift (1)
Wable-iOS/Data/RepositoryImpl/ContentRepositoryImpl.swift (1)
fetchContentInfo(46-53)
Wable-iOS/Infra/Network/TargetType/AccountTargetType.swift (1)
Wable-iOS/Data/RepositoryImpl/AccountRepositoryImpl.swift (1)
updateUserBadge(42-49)
Wable-iOS/Domain/UseCase/Profile/UpdateUserBadgeUseCase.swift (1)
Wable-iOS/Data/RepositoryImpl/AccountRepositoryImpl.swift (1)
updateUserBadge(42-49)
Wable-iOS/Domain/UseCase/Home/FetchUserInformationUseCase.swift (1)
Wable-iOS/Data/RepositoryImpl/UserSessionRepositoryImpl.swift (1)
updateUserSession(50-78)
Wable-iOS/Domain/RepositoryInterface/UserSessionRepository.swift (2)
Wable-iOS/Data/RepositoryImpl/UserSessionRepositoryImpl.swift (1)
updateUserSession(50-78)Wable-iOS/Domain/UseCase/Home/FetchUserInformationUseCase.swift (1)
updateUserSession(33-54)
Wable-iOS/Presentation/Home/ViewModel/HomeViewModel.swift (1)
Wable-iOS/Domain/UseCase/Home/FetchUserInformationUseCase.swift (1)
fetchActiveUserInfo(28-31)
Wable-iOS/Presentation/Notification/Activity/ViewModel/ActivityNotificationViewModel.swift (2)
Wable-iOS/Presentation/Notification/Activity/View/ActivityNotificationViewController.swift (1)
viewDidLoad(77-88)Wable-iOS/Core/Logger/WableLogger.swift (1)
log(14-25)
Wable-iOS/Presentation/Home/ViewModel/HomeDetailViewModel.swift (1)
Wable-iOS/Domain/UseCase/Profile/FetchContentInfoUseCase.swift (1)
execute(22-24)
Wable-iOS/Presentation/Home/View/HomeViewController.swift (1)
Wable-iOS/Presentation/WableComponent/Navigation/NavigationView.swift (1)
updateNotificationStatus(277-281)
🪛 SwiftLint (0.57.0)
Wable-iOS/App/AppDelegate.swift
[Warning] 16-16: TODOs should be resolved (Repository를 UseCase로 변경)
(todo)
[Warning] 70-70: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
Wable-iOS/App/AppDelegate+Firebase.swift
[Warning] 17-17: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
🔇 Additional comments (53)
Wable-iOS/Presentation/Viewit/Create/View/Subview/ViewitInputView.swift (1)
24-24: Verify theurlIconImageViewasset reference.The icon was changed to
.add. Please confirm that this asset exists in the catalog (or is provided via an extension) and that it matches the intended design for the URL input action.Wable-iOS/Presentation/WableComponent/Cell/NotificationCell.swift (1)
22-22: Good UI enhancement for profile imageAdding a corner radius of 18 points (half of the 36-point width/height) creates a perfectly circular profile image view, which is consistent with modern UI design patterns and improves the visual presentation of user avatars in notifications.
Wable-iOS/Infra/Token/TokenStorage.swift (1)
16-16: Good addition of FCM token storageAdding the
fcmTokencase to theTokenTypeenum is essential for securely storing the Firebase Cloud Messaging token in the keychain. This properly enables push notification functionality while maintaining security best practices for token storage.Wable-iOS/Presentation/Home/View/HomeDetailViewController.swift (2)
105-105: Good UI presentation style improvementSetting
modalPresentationStyle = .fullScreenensures a better user experience by having the view controller fill the entire screen when presented modally.
185-189: Good implementation of image viewing functionalityAdding the content image view tap handler improves user experience by allowing users to view images in a dedicated photo detail screen. The implementation correctly checks if the image exists before presenting the detail view.
Wable-iOS/Data/RepositoryImpl/AccountRepositoryImpl.swift (1)
44-44: Good API integration for FCM badge updatesThe modification to wrap the badge count in a
DTO.Request.UpdateUserBadgeobject properly aligns with the API endpoint expectations. This change supports the push notification feature by ensuring badge counts are properly synchronized with the server.Wable-iOS/Domain/UseCase/Onboarding/CreateUserProfileUseCase.swift (1)
27-27: Integrates FCM token with user profile creation, good additionThis change successfully incorporates FCM token management into the user profile creation flow, which is essential for the push notification functionality mentioned in the PR objectives. The token is fetched from the repository and passed during profile update, allowing the server to associate push notifications with specific users.
Wable-iOS/Domain/RepositoryInterface/ContentRepository.swift (1)
15-15: Simplified interface by removing unnecessary parameterRemoving the
titleparameter from thefetchContentInfomethod is a good design improvement as it simplifies the interface. According to the context, the title will now be retrieved directly from the content response rather than passed as a parameter.Could you clarify how this change relates to the FCM notification implementation mentioned in the PR objectives? Is it possible that notifications might include content titles that need to be fetched?
Wable-iOS/Data/RepositoryImpl/ContentRepositoryImpl.swift (1)
46-53: Implementation correctly aligns with protocol changesThe implementation has been updated to match the protocol changes, removing the
titleparameter and simplifying the mapping. The code now relies on the title embedded in the content response, which is a more robust approach.Wable-iOS/Domain/UseCase/Profile/FetchContentInfoUseCase.swift (1)
22-23: Updated use case correctly matches repository interfaceThe use case has been properly updated to reflect the changes in the repository interface. The simplified method signature now only requires the essential
contentIDparameter to fetch content information.Wable-iOS/Data/Mapper/ContentMapper.swift (2)
13-13: Method signature updated to use DTO's contentTitle directlyThe method signature has been simplified by removing the external
titleparameter, now directly relying onresponse.contentTitlefrom the DTO. This is a good refactoring that reduces redundancy and aligns with the DTO structure updates.
40-40: Updated to use response.contentTitle directlyThe
titlefield now usesresponse.contentTitledirectly from the response DTO, matching the method signature change. This simplifies the data flow and removes dependency on external parameters.Wable-iOS/Infra/Network/DTO/Response/Content/FetchContent.swift (3)
23-23: Added contentTitle property to FetchContent DTOGood addition of the
contentTitlefield to the DTO. This enables direct access to the content title from the response, supporting the push notification feature and simplifying the mapper implementation.
25-25: Added isDeleted flag to track content statusThe addition of the
isDeletedflag is important for the notification feature, allowing the app to check if referenced content has been deleted before showing notifications.
33-33: Updated CodingKeys to include new propertiesCodingKeys have been properly updated to include the new properties, ensuring they'll be correctly decoded from the JSON response.
Wable-iOS/Presentation/Login/LoginViewController.swift (1)
87-88: Removed temporary login buttonThe removal of the temporary login button is appropriate as the PR implements a proper login flow with FCM token management. This cleanup helps maintain code quality by removing development shortcuts.
Wable-iOS/Domain/UseCase/Login/FetchUserAuthUseCase.swift (3)
27-38: Enhanced token handling with improved loggingThe token saving logic has been improved with:
- Better formatting with a multi-line closure for readability
- Explicit debug logging after saving both tokens
- Proper error logging for failures
These changes improve the debugging experience and align with the FCM push notification implementation.
40-48: Updated user session management for notification supportThe user session update now passes
nilfornotificationBadgeCountinstead of a hardcoded0, which is more appropriate as it allows the repository to determine the default value. This change supports the push notification feature by not overriding potentially existing notification counts.
50-51: Improved code formattingThe call to update the active user ID is now formatted more clearly, improving readability.
Wable-iOS/Infra/Network/TargetType/AccountTargetType.swift (2)
16-16: Well-structured API design change for badge management.The evolution from a simple
Intparameter to a structuredDTO.Request.UpdateUserBadgeobject is a good design decision. This provides better type safety, documentation of the API contract, and allows for future extensibility if additional badge-related parameters are needed.
48-49: Consistent request body handling.This implementation now aligns with the pattern used for other cases like
deleteAccount, creating consistency across the codebase. The change properly maps the structured request object to the request body, completing the refactoring started with the parameter type change.Wable-iOS.xcodeproj/project.pbxproj (4)
154-155: New UseCase added for FCM token management.This file
UpdateFCMTokenUseCase.swiftis correctly added to handle token updates for Firebase Cloud Messaging, which is essential for the push notification implementation. It's appropriately placed in the Onboarding module, likely to register/update tokens during user onboarding and login flows.
159-159: Firebase integration added to AppDelegate.The file
AppDelegate+Firebase.swiftextends the AppDelegate to handle Firebase-specific functionality. This is a good architectural decision to separate concerns by placing Firebase configuration and notification handling in a dedicated extension rather than cluttering the main AppDelegate.
160-161: Badge management UseCase added for notifications.The
UpdateUserBadgeUseCase.swiftfile is added to the Profile module to handle notification badge updates. This will likely be used to update the UI badge counts when notifications are received, read, or cleared.
1-2884: Project structure is properly maintained.The newly added files for FCM push notifications are correctly integrated into their appropriate modules within the project structure. The project already includes the necessary Firebase dependencies (FirebaseCore and FirebaseMessaging), which are required for push notification functionality.
Wable-iOS/Domain/UseCase/Home/FetchUserInformationUseCase.swift (1)
33-54: LGTM! Well-structured method signature for updating user session attributes.The implementation correctly accepts individual optional parameters for user session attributes and properly delegates to the repository layer using the
Justpublisher. This approach provides more flexibility by allowing selective updates to specific session attributes.Wable-iOS/Domain/RepositoryInterface/UserSessionRepository.swift (1)
19-25: Effective protocol refactoring for user session updates.The protocol now uses a single consolidated method with optional parameters rather than multiple specific methods. This simplifies the interface while maintaining flexibility for partial updates.
Wable-iOS/Presentation/Notification/Page/View/NotificationPageViewController.swift (2)
112-129: Good implementation of real use cases replacing mock dependencies.The code properly initializes the activity notification view model with three distinct use cases:
- Notification use case for fetching notifications
- User badge use case for updating badge counts
- User information use case for managing user session data
This change supports the new push notification functionality by providing the necessary dependencies for badge management.
141-142: UI refinement with proper top offset.The 10-point top offset for the paging view improves the vertical alignment and provides better visual spacing.
Wable-iOS/Presentation/Home/ViewModel/HomeDetailViewModel.swift (2)
141-141: Content title parameter removal aligns with data layer changes.Removed the unnecessary
contentTitleparameter from thefetchContentInfoUseCase.executecall, which now retrieves the title from the response data directly.
303-313: Simplified content data fetching with proper null checks.The code now uses a guard statement for early return and no longer passes the content title parameter when fetching content information, aligning with the updated use case interface.
Wable-iOS/Presentation/Onboarding/ViewController/AgreementViewController.swift (3)
23-23: Good addition of FCM token update capability.The
updateFCMTokenUseCaseproperly encapsulates the token update functionality, adhering to clean architecture principles by using dependency injection with the repository.
159-159: Improved push notification handling.Nice update to use the actual marketing checkbox state for the
isPushAlarmAllowedparameter instead of the hardcodedfalsevalue. This ensures user preferences are correctly captured.
178-186: Good preservation of notification badge count.The user session update now properly preserves the existing
notificationBadgeCountwhen updating other session parameters, which is important for maintaining notification state.Wable-iOS/Domain/UseCase/Profile/UpdateUserBadgeUseCase.swift (2)
11-17: Well-structured use case with proper dependency injection.The
UpdateUserBadgeUseCaseclass follows clean architecture principles by depending on theAccountRepositoryabstraction rather than a concrete implementation, which enhances testability and modularity.
21-25: Clean and focused implementation.The
executemethod has a clear purpose and follows good functional reactive programming practices with Combine. It's appropriately returning an error when needed and delegating to the repository layer.Wable-iOS/Presentation/Home/ViewModel/HomeViewModel.swift (4)
58-58: Good addition of badge count publisher to Output.This addition properly exposes the notification badge count to subscribers, following the established pattern for the view model's outputs.
75-75: Well-structured subject for badge count tracking.The
badgeCountSubjectis correctly implemented as aCurrentValueSubjectwith an appropriate initial value ofnil, consistent with other subjects in this view model.
94-102: Good implementation of user session and badge count retrieval.The code now properly retrieves the full
UserSession?object and extracts both the admin status and notification badge count. This single fetch operation for multiple values is efficient and maintains data consistency.
315-315: Output structure correctly updated to include badge count.The badge count publisher is properly included in the Output struct initialization, making it available to subscribers.
Wable-iOS/Domain/UseCase/Onboarding/UpdateFCMTokenUseCase.swift (1)
11-17: Well-structured use case with proper dependency injection.The
UpdateFCMTokenUseCasefollows clean architecture principles with proper separation of concerns and dependency injection. This makes the code testable and maintainable.Wable-iOS/Data/RepositoryImpl/UserSessionRepositoryImpl.swift (1)
50-70: Confirm default values, especially auto-login, before shipping
isAutoLoginEnabledfalls back totruewhen a brand-new session is created (existingSession == nil& parameter isnil).
That means the very first time a user logs in they will automatically be opted-in to auto-login, which is easy to overlook and may be contrary to privacy requirements.isAutoLoginEnabled: isAutoLoginEnabled ?? existingSession?.isAutoLoginEnabled ?? true,Consider whether
falseis a safer default:- isAutoLoginEnabled: isAutoLoginEnabled ?? existingSession?.isAutoLoginEnabled ?? true, + isAutoLoginEnabled: isAutoLoginEnabled ?? existingSession?.isAutoLoginEnabled ?? false,(or force the caller to pass an explicit value).
Please double-check the product spec; if auto-login should remain off by default, apply the change above or pass the flag explicitly from the login flow.
Wable-iOS/Data/RepositoryImpl/ProfileRepositoryImpl.swift (6)
17-17: Good addition of token storage for FCM token management.The
tokenStorageproperty usingKeychainStorageis a secure way to store sensitive tokens like FCM tokens, which is important for maintaining user privacy and security.
21-42: New method to update user profile with nickname and FCM token.This implementation correctly adds the ability to update a user's profile with just a nickname and optional FCM token, keeping all other profile fields as nil. This supports the FCM token registration flow without requiring all profile fields.
44-50: FCM token retrieval with proper error handling.The implementation properly handles errors when attempting to load the FCM token from storage, returning nil instead of propagating the error, which is appropriate for this use case.
52-59: FCM token update with appropriate logging.The method correctly handles token storage with error handling and logging. This is important for debugging any issues with FCM token updates.
79-85: Enhanced profile update method with FCM token support.The updated signature properly makes all parameters optional with default values, allowing for partial profile updates. The addition of the FCM token parameter is consistent with the PR's objectives.
91-97: Updated profile fields mapping with FCM token integration.The modified implementation correctly handles mapping profile fields including the FCM token to the DTO request structure. The use of optional chaining with profile properties is appropriate.
Wable-iOS/App/AppDelegate+Firebase.swift (5)
28-40: Firebase configuration is well-implemented.The method properly configures Firebase with appropriate notification permissions (alert, sound, badge). Setting logger level to
.minis a good practice for production code. The implementation correctly sets up the messaging delegate and notification center delegate.
42-46: FCM token saving method is properly implemented.The method correctly guards against nil tokens and uses the profileRepository to update the token.
51-55: MessagingDelegate implementation handles token refresh.The implementation correctly updates the FCM token when a new registration token is received from Firebase.
81-87: Content navigation from notification properly guarded.The implementation correctly guards against missing content ID, scene, or root view controller before attempting navigation.
120-128: Foreground notification presentation configured correctly.The method properly configures how notifications should be presented when the app is in the foreground, enabling banner, sound, and badge options.
| private let tokenProvider = OAuthTokenProvider() | ||
|
|
||
| private let loginRepository = LoginRepositoryImpl() | ||
| private let profileRepository = ProfileRepositoryImpl() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicate repository instance may lead to inconsistent state
profileRepository is declared as a stored property (line 22), yet new instances of ProfileRepositoryImpl are created later (e.g. in configureLoginScreen). Re-using the shared instance avoids redundant network caches / Keychain access and guarantees a single source of truth.
- private let profileRepository = ProfileRepositoryImpl()
+ private let profileRepository = ProfileRepositoryImpl() // keep one instanceConsider injecting this same instance wherever it is needed.
📝 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.
| private let profileRepository = ProfileRepositoryImpl() | |
| private let profileRepository = ProfileRepositoryImpl() // keep one instance |
| updateFCMTokenUseCase: UpdateFCMTokenUseCase( | ||
| repository: ProfileRepositoryImpl() | ||
| ), | ||
| fetchUserAuthUseCase: FetchUserAuthUseCase( |
There was a problem hiding this comment.
Reuse the existing profileRepository instead of instantiating a new one
Here a fresh ProfileRepositoryImpl() is passed into UpdateFCMTokenUseCase, defeating any benefit of the singleton created on line 22.
- updateFCMTokenUseCase: UpdateFCMTokenUseCase(
- repository: ProfileRepositoryImpl()
- ),
+ updateFCMTokenUseCase: UpdateFCMTokenUseCase(
+ repository: profileRepository // reuse
+ ),Besides memory overhead, two repositories can race while reading / writing FCM tokens to storage.
📝 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.
| updateFCMTokenUseCase: UpdateFCMTokenUseCase( | |
| repository: ProfileRepositoryImpl() | |
| ), | |
| fetchUserAuthUseCase: FetchUserAuthUseCase( | |
| updateFCMTokenUseCase: UpdateFCMTokenUseCase( | |
| repository: profileRepository // reuse | |
| ), | |
| fetchUserAuthUseCase: FetchUserAuthUseCase( |
| if let id = userSessionRepository.fetchActiveUserID() { | ||
| profileRepository.fetchUserProfile(memberID: id) | ||
| .receive(on: DispatchQueue.main) | ||
| .sink { _ in | ||
| } receiveValue: { [weak self] info in | ||
| guard let self = self else { return } | ||
| let token = self.profileRepository.fetchFCMToken() | ||
|
|
||
| self.userSessionRepository.updateUserSession( | ||
| userID: id, | ||
| nickname: info.user.nickname, | ||
| profileURL: info.user.profileURL, | ||
| isAutoLoginEnabled: true | ||
| ) | ||
|
|
||
| self.profileRepository.updateUserProfile(nickname: info.user.nickname, fcmToken: token) | ||
| .sink { completion in | ||
| switch completion { | ||
| case .failure(let error): | ||
| WableLogger.log("토큰 업데이트 실패: \(error)", for: .error) | ||
| default: | ||
| break | ||
| } | ||
| } receiveValue: { _ in | ||
| WableLogger.log("토큰 업데이트 성공", for: .debug) | ||
| } | ||
| .store(in: cancelBag) | ||
|
|
||
| } | ||
| .store(in: cancelBag) | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing error handling and optional-token guard in the login flow
- The upstream publisher
fetchUserProfileignores its failure case – a network error will silently skip the rest of the chain. fetchFCMToken()may returnnil; passing it straight intoupdateUserProfilerisks sending “null” to your backend.
profileRepository.fetchUserProfile(memberID: id)
+ .catch { [weak self] error -> Empty<ProfileInfo, Never> in
+ WableLogger.log("프로필 조회 실패: \(error)", for: .error)
+ return .init() // bail-out; still completes
+ }
...
- let token = self.profileRepository.fetchFCMToken()
+ guard let token = self.profileRepository.fetchFCMToken() else {
+ WableLogger.log("FCM 토큰이 없어 업데이트를 생략합니다.", for: .debug)
+ return
+ }Fail-fast logging helps diagnose login issues early.
📝 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.
| if let id = userSessionRepository.fetchActiveUserID() { | |
| profileRepository.fetchUserProfile(memberID: id) | |
| .receive(on: DispatchQueue.main) | |
| .sink { _ in | |
| } receiveValue: { [weak self] info in | |
| guard let self = self else { return } | |
| let token = self.profileRepository.fetchFCMToken() | |
| self.userSessionRepository.updateUserSession( | |
| userID: id, | |
| nickname: info.user.nickname, | |
| profileURL: info.user.profileURL, | |
| isAutoLoginEnabled: true | |
| ) | |
| self.profileRepository.updateUserProfile(nickname: info.user.nickname, fcmToken: token) | |
| .sink { completion in | |
| switch completion { | |
| case .failure(let error): | |
| WableLogger.log("토큰 업데이트 실패: \(error)", for: .error) | |
| default: | |
| break | |
| } | |
| } receiveValue: { _ in | |
| WableLogger.log("토큰 업데이트 성공", for: .debug) | |
| } | |
| .store(in: cancelBag) | |
| } | |
| .store(in: cancelBag) | |
| } | |
| if let id = userSessionRepository.fetchActiveUserID() { | |
| profileRepository.fetchUserProfile(memberID: id) | |
| .catch { [weak self] error -> Empty<ProfileInfo, Never> in | |
| WableLogger.log("프로필 조회 실패: \(error)", for: .error) | |
| return .init() // bail-out; still completes | |
| } | |
| .receive(on: DispatchQueue.main) | |
| .sink { _ in | |
| } receiveValue: { [weak self] info in | |
| guard let self = self else { return } | |
| guard let token = self.profileRepository.fetchFCMToken() else { | |
| WableLogger.log("FCM 토큰이 없어 업데이트를 생략합니다.", for: .debug) | |
| return | |
| } | |
| self.userSessionRepository.updateUserSession( | |
| userID: id, | |
| nickname: info.user.nickname, | |
| profileURL: info.user.profileURL, | |
| isAutoLoginEnabled: true | |
| ) | |
| self.profileRepository.updateUserProfile(nickname: info.user.nickname, fcmToken: token) | |
| .sink { completion in | |
| switch completion { | |
| case .failure(let error): | |
| WableLogger.log("토큰 업데이트 실패: \(error)", for: .error) | |
| default: | |
| break | |
| } | |
| } receiveValue: { _ in | |
| WableLogger.log("토큰 업데이트 성공", for: .debug) | |
| } | |
| .store(in: cancelBag) | |
| } | |
| .store(in: cancelBag) | |
| } |
| output.badgeCount | ||
| .receive(on: DispatchQueue.main) | ||
| .sink { [weak self] count in | ||
| guard let self = self, | ||
| let count = count | ||
| else { | ||
| return | ||
| } | ||
|
|
||
| self.navigationView.updateNotificationStatus(hasNewNotification: count > 0) | ||
| } | ||
| .store(in: cancelBag) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
badgeCount nil case leaves stale UI
When output.badgeCount publishes nil, the navigation bar retains whatever badge state it had previously.
If the server clears notifications and your view model publishes nil (or finishes), the icon will still show the red badge.
A safer approach:
-guard let self = self,
- let count = count
-else {
- return
-}
-
-self.navigationView.updateNotificationStatus(hasNewNotification: count > 0)
+guard let self else { return }
+self.navigationView.updateNotificationStatus(hasNewNotification: (count ?? 0) > 0)This treats “no value” as “no new notifications”.
📝 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.
| output.badgeCount | |
| .receive(on: DispatchQueue.main) | |
| .sink { [weak self] count in | |
| guard let self = self, | |
| let count = count | |
| else { | |
| return | |
| } | |
| self.navigationView.updateNotificationStatus(hasNewNotification: count > 0) | |
| } | |
| .store(in: cancelBag) | |
| output.badgeCount | |
| .receive(on: DispatchQueue.main) | |
| .sink { [weak self] count in | |
| guard let self else { return } | |
| self.navigationView.updateNotificationStatus(hasNewNotification: (count ?? 0) > 0) | |
| } | |
| .store(in: cancelBag) |
| func fetchFCMToken() -> String? | ||
| func updateFCMToken(token: String) | ||
| func updateUserProfile(nickname: String, fcmToken: String?) -> AnyPublisher<Void, WableError> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider returning publishers for token I/O to keep the interface consistent
All existing read / write operations in this protocol return AnyPublisher<…, WableError>.
fetchFCMToken() and updateFCMToken(token:) break that pattern by returning a plain optional String and Void.
If key-chain (or any secure storage) access fails, there is currently no way to surface the error to the caller.
-func fetchFCMToken() -> String?
-func updateFCMToken(token: String)
+func fetchFCMToken() -> AnyPublisher<String?, WableError>
+func updateFCMToken(token: String) -> AnyPublisher<Void, WableError>Aligning the signatures keeps error propagation uniform across the repository layer and avoids hidden failures.
📝 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 fetchFCMToken() -> String? | |
| func updateFCMToken(token: String) | |
| func updateUserProfile(nickname: String, fcmToken: String?) -> AnyPublisher<Void, WableError> | |
| func fetchFCMToken() -> AnyPublisher<String?, WableError> | |
| func updateFCMToken(token: String) -> AnyPublisher<Void, WableError> | |
| func updateUserProfile(nickname: String, fcmToken: String?) -> AnyPublisher<Void, WableError> |
| loadTrigger | ||
| .withUnretained(self) | ||
| .sink { owner, _ in | ||
| owner.userBadgeUseCase.execute(number: 0) | ||
| .sink { completion in | ||
| if case .failure(let error) = completion { | ||
| WableLogger.log("뱃지 수정 중 오류 발생: \(error)", for: .error) | ||
| } | ||
| } receiveValue: { [weak self] _ in | ||
| guard let self = self else { return } | ||
|
|
||
| self.userInformationUseCase.fetchActiveUserID() | ||
| .sink { [weak self] id in | ||
| guard let self = self, | ||
| let id = id | ||
| else { | ||
| return | ||
| } | ||
|
|
||
| self.userInformationUseCase.updateUserSession(userID: id, notificationBadgeCount: 0) | ||
| .sink { _ in } | ||
| .store(in: cancelBag) | ||
| } | ||
| .store(in: cancelBag) | ||
| } | ||
| .store(in: cancelBag) | ||
| } | ||
| .store(in: cancelBag) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Nested sinks create multiple subscriptions and possible retain cycles – flatten the Combine chain
loadTrigger opens a sink that in turn opens two more sinks (userBadgeUseCase.execute ➜ fetchActiveUserID).
Every trigger therefore allocates three independent subscriptions and stores them all in the same cancelBag, steadily growing the bag and retaining self.
A flatter pipeline avoids both issues and is easier to reason about:
-loadTrigger
- .withUnretained(self)
- .sink { owner, _ in
- owner.userBadgeUseCase.execute(number: 0)
- .sink { completion in
- if case .failure(let error) = completion {
- WableLogger.log("뱃지 수정 중 오류 발생: \(error)", for: .error)
- }
- } receiveValue: { [weak self] _ in
- guard let self = self else { return }
- self.userInformationUseCase.fetchActiveUserID()
- .sink { [weak self] id in
- guard let self = self, let id = id else { return }
- self.userInformationUseCase.updateUserSession(userID: id, notificationBadgeCount: 0)
- .sink { _ in }
- .store(in: cancelBag)
- }
- .store(in: cancelBag)
- }
- .store(in: cancelBag)
- }
- .store(in: cancelBag)
+loadTrigger
+ .flatMap { [weak self] _ -> AnyPublisher<Int?, Never> in
+ guard let self = self else { return Empty().eraseToAnyPublisher() }
+ return self.userBadgeUseCase.execute(number: 0)
+ .catch { error -> AnyPublisher<Void, Never> in
+ WableLogger.log("뱃지 수정 중 오류 발생: \(error)", for: .error)
+ return Empty().eraseToAnyPublisher()
+ }
+ .flatMap { [weak self] _ -> AnyPublisher<Int?, Never> in
+ self?.userInformationUseCase.fetchActiveUserID() ?? Empty().eraseToAnyPublisher()
+ }
+ .eraseToAnyPublisher()
+ }
+ .compactMap { $0 }
+ .flatMap { [weak self] id -> AnyPublisher<Void, Never> in
+ self?.userInformationUseCase.updateUserSession(userID: id, notificationBadgeCount: 0)
+ ?? Empty().eraseToAnyPublisher()
+ }
+ .sink { _ in }
+ .store(in: cancelBag)Benefits
• Only one subscription per trigger
• No manual nested store(in:) calls
• self is captured weakly throughout, eliminating retain-cycle risk
📝 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.
| loadTrigger | |
| .withUnretained(self) | |
| .sink { owner, _ in | |
| owner.userBadgeUseCase.execute(number: 0) | |
| .sink { completion in | |
| if case .failure(let error) = completion { | |
| WableLogger.log("뱃지 수정 중 오류 발생: \(error)", for: .error) | |
| } | |
| } receiveValue: { [weak self] _ in | |
| guard let self = self else { return } | |
| self.userInformationUseCase.fetchActiveUserID() | |
| .sink { [weak self] id in | |
| guard let self = self, | |
| let id = id | |
| else { | |
| return | |
| } | |
| self.userInformationUseCase.updateUserSession(userID: id, notificationBadgeCount: 0) | |
| .sink { _ in } | |
| .store(in: cancelBag) | |
| } | |
| .store(in: cancelBag) | |
| } | |
| .store(in: cancelBag) | |
| } | |
| .store(in: cancelBag) | |
| loadTrigger | |
| .flatMap { [weak self] _ -> AnyPublisher<Int?, Never> in | |
| guard let self = self else { return Empty().eraseToAnyPublisher() } | |
| return self.userBadgeUseCase.execute(number: 0) | |
| .catch { error -> AnyPublisher<Void, Never> in | |
| WableLogger.log("뱃지 수정 중 오류 발생: \(error)", for: .error) | |
| return Empty().eraseToAnyPublisher() | |
| } | |
| .flatMap { [weak self] _ -> AnyPublisher<Int?, Never> in | |
| self?.userInformationUseCase.fetchActiveUserID() ?? Empty().eraseToAnyPublisher() | |
| } | |
| .eraseToAnyPublisher() | |
| } | |
| .compactMap { $0 } | |
| .flatMap { [weak self] id -> AnyPublisher<Void, Never> in | |
| self?.userInformationUseCase.updateUserSession(userID: id, notificationBadgeCount: 0) | |
| ?? Empty().eraseToAnyPublisher() | |
| } | |
| .sink { _ in } | |
| .store(in: cancelBag) |
| sessions[userID] = updatedSession | ||
| try? userDefaults.setValue(sessions, for: Keys.userSessions) | ||
|
|
||
| if fetchActiveUserID() == nil { | ||
| updateActiveUserID(session.id) | ||
| } | ||
| } | ||
|
|
||
| func updateNotificationBadge(count: Int, forUserID userID: Int) { | ||
| var sessions = fetchAllUserSessions() | ||
|
|
||
| if let session = sessions[userID] { | ||
| let updatedSession = UserSession( | ||
| id: session.id, | ||
| nickname: session.nickname, | ||
| profileURL: session.profileURL, | ||
| isPushAlarmAllowed: session.isPushAlarmAllowed, | ||
| isAdmin: session.isAdmin, | ||
| isAutoLoginEnabled: session.isAutoLoginEnabled, | ||
| notificationBadgeCount: count | ||
| ) | ||
| sessions[userID] = updatedSession | ||
| try? userDefaults.setValue(sessions, for: Keys.userSessions) | ||
| updateActiveUserID(userID) | ||
| } | ||
| } |
There was a problem hiding this comment.
Log or surface persistence errors instead of silencing them
try? userDefaults.setValue(…) swallows any exception, making it impossible to know when persisting the session failed (e.g. JSON encoding issue, disk full, etc.).
- try? userDefaults.setValue(sessions, for: Keys.userSessions)
+ do {
+ try userDefaults.setValue(sessions, for: Keys.userSessions)
+ } catch {
+ WableLogger.log("⚠️ Failed to persist user session: \(error)", for: .error)
+ }Propagating or at least logging the error will save debugging time later.
📝 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.
| sessions[userID] = updatedSession | |
| try? userDefaults.setValue(sessions, for: Keys.userSessions) | |
| if fetchActiveUserID() == nil { | |
| updateActiveUserID(session.id) | |
| } | |
| } | |
| func updateNotificationBadge(count: Int, forUserID userID: Int) { | |
| var sessions = fetchAllUserSessions() | |
| if let session = sessions[userID] { | |
| let updatedSession = UserSession( | |
| id: session.id, | |
| nickname: session.nickname, | |
| profileURL: session.profileURL, | |
| isPushAlarmAllowed: session.isPushAlarmAllowed, | |
| isAdmin: session.isAdmin, | |
| isAutoLoginEnabled: session.isAutoLoginEnabled, | |
| notificationBadgeCount: count | |
| ) | |
| sessions[userID] = updatedSession | |
| try? userDefaults.setValue(sessions, for: Keys.userSessions) | |
| updateActiveUserID(userID) | |
| } | |
| } | |
| sessions[userID] = updatedSession | |
| do { | |
| try userDefaults.setValue(sessions, for: Keys.userSessions) | |
| } catch { | |
| WableLogger.log("⚠️ Failed to persist user session: \(error)", for: .error) | |
| } | |
| if fetchActiveUserID() == nil { | |
| updateActiveUserID(userID) | |
| } | |
| } |
| // MARK: Property | ||
| // TODO: Repository를 UseCase로 변경 | ||
|
|
||
| let userBadgeUseCase = UpdateUserBadgeUseCase(repository: AccountRepositoryImpl()) | ||
| let userSessionRepository = UserSessionRepositoryImpl(userDefaults: UserDefaultsStorage(jsonEncoder: JSONEncoder(), jsonDecoder: JSONDecoder())) | ||
| let profileRepository = ProfileRepositoryImpl() | ||
| let contentRepository = ContentRepositoryImpl() | ||
| let commentRepository = CommentRepositoryImpl() | ||
| let contentLikedRepository = ContentLikedRepositoryImpl() | ||
| let commentLikedRepository = CommentLikedRepositoryImpl() | ||
| let reportRepository = ReportRepositoryImpl() | ||
| let cancelBag = CancelBag() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace TODO with a tracked task or implement DI now
Line 16 still contains a TODO about replacing repositories with use-cases.
These comments easily get lost; either
- Create an issue in the tracker and reference it here, or
- Finish the refactor before merging.
Leaving it as-is increases technical debt.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 16-16: TODOs should be resolved (Repository를 UseCase로 변경)
(todo)
| if let tabBarController = rootViewController as? TabBarController { | ||
| guard let viewController = tabBarController.selectedViewController as? UINavigationController else { return } | ||
|
|
||
| tabBarController.selectedIndex = 0 | ||
| viewController.pushViewController(detailViewController, animated: true) | ||
| } else | ||
| if let viewController = rootViewController as? UINavigationController { | ||
| viewController.pushViewController(detailViewController, animated: true) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
View controller navigation logic handles different root view structures.
The implementation correctly handles both TabBarController and UINavigationController root view controllers for navigation, but has some potential issues.
The nested if-else structure could be simplified, and there's a potential issue if the root view controller is neither a TabBarController nor a UINavigationController.
- if let tabBarController = rootViewController as? TabBarController {
- guard let viewController = tabBarController.selectedViewController as? UINavigationController else { return }
-
- tabBarController.selectedIndex = 0
- viewController.pushViewController(detailViewController, animated: true)
- } else
- if let viewController = rootViewController as? UINavigationController {
- viewController.pushViewController(detailViewController, animated: true)
- }
+ if let tabBarController = rootViewController as? TabBarController {
+ tabBarController.selectedIndex = 0
+ if let navigationController = tabBarController.selectedViewController as? UINavigationController {
+ navigationController.pushViewController(detailViewController, animated: true)
+ } else {
+ WableLogger.log("Unable to navigate: selected view controller is not a navigation controller", for: .error)
+ }
+ } else if let navigationController = rootViewController as? UINavigationController {
+ navigationController.pushViewController(detailViewController, animated: true)
+ } else {
+ WableLogger.log("Unable to navigate: root view controller is neither a tab bar nor a navigation controller", for: .error)
+ }| func userNotificationCenter(_ center: UNUserNotificationCenter, didReceive response: UNNotificationResponse) async { | ||
| guard let aps = response.notification.request.content.userInfo["aps"] as? [String: Any], | ||
| let badge = aps["badge"] as? Int | ||
| else { | ||
| return | ||
| } | ||
|
|
||
| userBadgeUseCase.execute(number: badge - 1) | ||
| .receive(on: DispatchQueue.main) | ||
| .sink { completion in | ||
| if case .failure(let error) = completion { | ||
| WableLogger.log("뱃지 수정 중 오류 발생: \(error)", for: .error) | ||
| } | ||
| } receiveValue: { [weak self] _ in | ||
| guard let self = self else { return } | ||
|
|
||
| self.userSessionRepository.updateUserSession(userID: activeID, notificationBadgeCount: badge - 1) | ||
| WableLogger.log("뱃지 수정 완료: \(badge - 1)개", for: .debug) | ||
| } | ||
| .store(in: cancelBag) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Notification handling implemented with badge count update.
The implementation correctly extracts the badge count from the notification and updates it. It uses Combine for asynchronous operations and properly logs success and error cases.
Consider extracting the badge update logic to a separate method for better readability and reusability. Also, you should consider handling potential race conditions if multiple notifications are received in quick succession.
+ private func updateBadgeCount(_ badge: Int) {
+ userBadgeUseCase.execute(number: badge - 1)
+ .receive(on: DispatchQueue.main)
+ .sink { completion in
+ if case .failure(let error) = completion {
+ WableLogger.log("뱃지 수정 중 오류 발생: \(error)", for: .error)
+ }
+ } receiveValue: { [weak self] _ in
+ guard let self = self else { return }
+
+ self.userSessionRepository.updateUserSession(userID: activeID, notificationBadgeCount: badge - 1)
+ WableLogger.log("뱃지 수정 완료: \(badge - 1)개", for: .debug)
+ }
+ .store(in: cancelBag)
+ }Then call this method instead:
- userBadgeUseCase.execute(number: badge - 1)
- .receive(on: DispatchQueue.main)
- .sink { completion in
- if case .failure(let error) = completion {
- WableLogger.log("뱃지 수정 중 오류 발생: \(error)", for: .error)
- }
- } receiveValue: { [weak self] _ in
- guard let self = self else { return }
-
- self.userSessionRepository.updateUserSession(userID: activeID, notificationBadgeCount: badge - 1)
- WableLogger.log("뱃지 수정 완료: \(badge - 1)개", for: .debug)
- }
- .store(in: cancelBag)
+ updateBadgeCount(badge)📝 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 userNotificationCenter(_ center: UNUserNotificationCenter, didReceive response: UNNotificationResponse) async { | |
| guard let aps = response.notification.request.content.userInfo["aps"] as? [String: Any], | |
| let badge = aps["badge"] as? Int | |
| else { | |
| return | |
| } | |
| userBadgeUseCase.execute(number: badge - 1) | |
| .receive(on: DispatchQueue.main) | |
| .sink { completion in | |
| if case .failure(let error) = completion { | |
| WableLogger.log("뱃지 수정 중 오류 발생: \(error)", for: .error) | |
| } | |
| } receiveValue: { [weak self] _ in | |
| guard let self = self else { return } | |
| self.userSessionRepository.updateUserSession(userID: activeID, notificationBadgeCount: badge - 1) | |
| WableLogger.log("뱃지 수정 완료: \(badge - 1)개", for: .debug) | |
| } | |
| .store(in: cancelBag) | |
| func userNotificationCenter(_ center: UNUserNotificationCenter, didReceive response: UNNotificationResponse) async { | |
| guard let aps = response.notification.request.content.userInfo["aps"] as? [String: Any], | |
| let badge = aps["badge"] as? Int | |
| else { | |
| return | |
| } | |
| updateBadgeCount(badge) | |
| } | |
| private func updateBadgeCount(_ badge: Int) { | |
| userBadgeUseCase.execute(number: badge - 1) | |
| .receive(on: DispatchQueue.main) | |
| .sink { completion in | |
| if case .failure(let error) = completion { | |
| WableLogger.log("뱃지 수정 중 오류 발생: \(error)", for: .error) | |
| } | |
| } receiveValue: { [weak self] _ in | |
| guard let self = self else { return } | |
| self.userSessionRepository.updateUserSession(userID: activeID, notificationBadgeCount: badge - 1) | |
| WableLogger.log("뱃지 수정 완료: \(badge - 1)개", for: .debug) | |
| } | |
| .store(in: cancelBag) | |
| } |
👻 PULL REQUEST
📄 작업 내용
UserDefaultsRepository에서 세션이 제대로 저장되지 않던 오류를 개선했어요.👀 기타 더 이야기해볼 점
김진웅 화이팅!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
🔗 연결된 이슈
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Style
Chores