[Feat] FCM을 이용한 푸시 알림 기능 재구현 #195
Conversation
youz2me
added
✨ feat
|
Caution Review failedThe pull request is closed. WalkthroughThe changes remove the previous Firebase Cloud Messaging (FCM) integration and reimplement it directly within Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UIApplication
participant AppDelegate
participant Firebase
participant UNUserNotificationCenter
participant Messaging
participant ProfileRepository
participant ContentDetailViewController
User->>UIApplication: Launch app
UIApplication->>AppDelegate: didFinishLaunchingWithOptions
AppDelegate->>Firebase: Configure
AppDelegate->>Messaging: Set delegate
AppDelegate->>UNUserNotificationCenter: Set delegate, request authorization
UNUserNotificationCenter-->>AppDelegate: Return authorization status
AppDelegate->>UIApplication: Register for remote notifications
UIApplication->>AppDelegate: didRegisterForRemoteNotificationsWithDeviceToken
AppDelegate->>Messaging: Set APNs token
Messaging-->>AppDelegate: didReceiveRegistrationToken
AppDelegate->>ProfileRepository: Save FCM token
UNUserNotificationCenter-->>AppDelegate: didReceive notification response
AppDelegate->>AppDelegate: Parse badge count, content ID
AppDelegate->>ProfileRepository: Update badge count
AppDelegate->>ContentDetailViewController: Navigate to content detail (if ID present)
Assessment against linked issues
Possibly related PRs
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ 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 (3)
Wable-iOS/Presentation/Profile/AlarmSetting/ViewModel/AlarmSettingViewModel.swift (1)
19-20: Placeholder added for server sync - implementation neededA TODO comment has been added to mark where the alarm permission status should be sent to the server. This aligns with the PR's objective to synchronize notification preferences with the server.
Would you like help implementing this server sync functionality to complete the FCM push notification feature?
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 20-20: TODOs should be resolved (서버로 알람 수신 여부 보내기)
(todo)
Wable-iOS/Presentation/Login/LoginViewModel.swift (1)
17-21: Ambiguous property/type pairing
private let updateUserSessionUseCase: FetchUserInformationUseCasecouples an update-named variable to a fetch use-case type.
This mirrors the confusion flagged inSceneDelegate. If the injected use-case truly updates a session, its type name should reflect that; otherwise, rename the stored property.Wable-iOS/App/AppDelegate.swift (1)
48-58: Thread-safety when touchinguserSessionRepositoryinside the permission callback
requestAuthorization’s completion handler may be invoked on an arbitrary background queue.
userSessionRepository.updateUserSession(...)appears to mutate persistent storage; if the repository isn’t internally thread-safe, race conditions can occur.Wrap the call on the main queue (or the repository’s dedicated queue):
UNUserNotificationCenter.current().requestAuthorization( options: [.alert, .badge, .sound] ) { [weak self] granted, _ in guard let self = self, let session = self.userSessionRepository.fetchActiveUserSession() else { return } DispatchQueue.main.async { self.userSessionRepository.updateUserSession( userID: session.id, isPushAlarmAllowed: granted ) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (12)
Wable-iOS.xcodeproj/project.pbxproj(0 hunks)Wable-iOS/App/AppDelegate+Firebase.swift(0 hunks)Wable-iOS/App/AppDelegate.swift(3 hunks)Wable-iOS/App/SceneDelegate.swift(1 hunks)Wable-iOS/Presentation/Login/LoginViewModel.swift(4 hunks)Wable-iOS/Presentation/Notification/Activity/ViewModel/ActivityNotificationViewModel.swift(2 hunks)Wable-iOS/Presentation/Profile/AlarmSetting/View/AlarmSettingViewController.swift(1 hunks)Wable-iOS/Presentation/Profile/AlarmSetting/ViewModel/AlarmSettingViewModel.swift(1 hunks)Wable-iOS/Presentation/Profile/My/View/MyProfileViewController.swift(1 hunks)Wable-iOS/Presentation/Profile/Withdrawal/Guide/View/WithdrawalGuideViewController.swift(1 hunks)Wable-iOS/Presentation/Viewit/Create/ViewModel/CreateViewitViewModel.swift(0 hunks)Wable-iOS/Resource/Info.plist(1 hunks)
💤 Files with no reviewable changes (3)
- Wable-iOS/Presentation/Viewit/Create/ViewModel/CreateViewitViewModel.swift
- Wable-iOS.xcodeproj/project.pbxproj
- Wable-iOS/App/AppDelegate+Firebase.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Wable-iOS/Presentation/Profile/AlarmSetting/ViewModel/AlarmSettingViewModel.swift
[Warning] 20-20: TODOs should be resolved (서버로 알람 수신 여부 보내기)
(todo)
🔇 Additional comments (6)
Wable-iOS/Resource/Info.plist (1)
20-21: Use a Boolean value to disable the Firebase App Delegate proxy
Switching from a string to<false/>ensures that theFirebaseAppDelegateProxyEnabledkey is correctly interpreted as a boolean in Info.plist, which is required for manual FCM delegate handling inAppDelegate.swift.Wable-iOS/Presentation/Profile/My/View/MyProfileViewController.swift (1)
459-460: Dependency injection added for user profile managementAdded
userProfileUseCaseto the LoginViewModel initialization, which enables synchronizing the user's push notification preferences with the server during login.Wable-iOS/Presentation/Profile/Withdrawal/Guide/View/WithdrawalGuideViewController.swift (1)
117-118: Dependency injection added for user profile managementAdded
userProfileUseCaseto the LoginViewModel initialization, which is consistent with similar changes in other view controllers. This enables the synchronization of user profile data, including push notification preferences, with the server during login.Wable-iOS/Presentation/Notification/Activity/ViewModel/ActivityNotificationViewModel.swift (2)
9-9: Import changed from Foundation to UIKitChanged the import to UIKit which is necessary to access
UIApplicationfor the badge number update.
91-92: App icon badge reset addedAdded code to reset the application icon badge number to zero when notification badges are cleared. This ensures that the visual indicator on the app icon is synchronized with the internal badge count.
Wable-iOS/Presentation/Profile/AlarmSetting/View/AlarmSettingViewController.swift (1)
44-46: 👍 Good UX-centric tweakSetting
hidesBottomBarWhenPushed = truedirectly in the initializer guarantees consistent behaviour whenever the screen is shown. Looks perfect.
| updateUserSessionUseCase: FetchUserInformationUseCase(repository: userSessionRepository), | ||
| userProfileUseCase: UserProfileUseCase(repository: ProfileRepositoryImpl()) | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
updateUserSessionUseCase is being initialised with FetchUserInformationUseCase(...).
This will compile if the constructor actually expects FetchUserInformationUseCase, but the label implies a different responsibility (update vs fetch). The mismatch is confusing for future readers and makes the dependency graph harder to reason about.
Consider either:
- updateUserSessionUseCase: FetchUserInformationUseCase(repository: userSessionRepository),
+ fetchUserInformationUseCase: FetchUserInformationUseCase(repository: userSessionRepository),or injecting the real UpdateUserSessionUseCase if that is what the view-model needs.
🏁 Script executed:
#!/bin/bash
set -e
# Search for any definition or usage of UpdateUserSessionUseCase
rg "UpdateUserSessionUseCase" -n .Length of output: 43
🏁 Script executed:
#!/bin/bash
set -e
# Locate SceneDelegate.swift
scene_file=$(fd SceneDelegate.swift -t f)
echo "SceneDelegate file: $scene_file"
# Show lines 1-200 to inspect initializer context
sed -n '1,200p' "$scene_file"Length of output: 5767
🏁 Script executed:
#!/bin/bash
set -e
# Locate LoginViewModel.swift
login_vm_file=$(fd LoginViewModel.swift -t f)
echo "LoginViewModel file: $login_vm_file"
# Show the initializer signature and surrounding context
sed -n '1,200p' "$login_vm_file"Length of output: 5948
🏁 Script executed:
#!/bin/bash
set -e
# Find definition of FetchUserInformationUseCase
rg "struct FetchUserInformationUseCase" -n .
rg "class FetchUserInformationUseCase" -n .
# Show its file contents for context
file=$(rg "FetchUserInformationUseCase" -l . | head -n 1)
echo "File: $file"
sed -n '1,200p' "$file"
# Search for updateUserSession method within this file
rg "updateUserSession" -n "$file"Length of output: 55
🏁 Script executed:
#!/bin/bash
set -e
# Case-insensitive search for FetchUserInformationUseCase
rg -i "FetchUserInformationUseCase" -n .Length of output: 5419
Rename updateUserSessionUseCase to match the injected FetchUserInformationUseCase
The parameter label (updateUserSessionUseCase) currently does not match the type being injected (FetchUserInformationUseCase), which is confusing. Since there is no UpdateUserSessionUseCase in the codebase, you should rename the label (and corresponding property/parameter in LoginViewModel) to fetchUserInformationUseCase.
• SceneDelegate.swift (around lines 94–96):
- updateUserSessionUseCase: FetchUserInformationUseCase(repository: userSessionRepository),
+ fetchUserInformationUseCase: FetchUserInformationUseCase(repository: userSessionRepository),• Presentation/Login/LoginViewModel.swift:
- private let updateUserSessionUseCase: FetchUserInformationUseCase
+ private let fetchUserInformationUseCase: FetchUserInformationUseCase
init(
updateFCMTokenUseCase: UpdateFCMTokenUseCase,
fetchUserAuthUseCase: FetchUserAuthUseCase,
- updateUserSessionUseCase: FetchUserInformationUseCase,
+ fetchUserInformationUseCase: FetchUserInformationUseCase,
userProfileUseCase: UserProfileUseCase
) { … }
// Inside transform(...)
- self.updateUserSessionUseCase.updateUserSession(…)
+ self.fetchUserInformationUseCase.updateUserSession(…)Make sure to update all related call sites (AppDelegate, other ViewControllers/ViewModels) for consistency.
📝 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.
| updateUserSessionUseCase: FetchUserInformationUseCase(repository: userSessionRepository), | |
| userProfileUseCase: UserProfileUseCase(repository: ProfileRepositoryImpl()) | |
| ) | |
| fetchUserInformationUseCase: FetchUserInformationUseCase(repository: userSessionRepository), | |
| userProfileUseCase: UserProfileUseCase(repository: ProfileRepositoryImpl()) |
🤖 Prompt for AI Agents
In Wable-iOS/App/SceneDelegate.swift around lines 94 to 96, rename the parameter
label and any related property named updateUserSessionUseCase to
fetchUserInformationUseCase to match the injected type
FetchUserInformationUseCase. Also update the corresponding parameter and
property names in Presentation/Login/LoginViewModel.swift and all related call
sites such as AppDelegate and other ViewControllers/ViewModels to maintain
consistency and avoid confusion.
| Task { | ||
| let isAuthorized = await UNUserNotificationCenter.current().notificationSettings().authorizationStatus == .authorized | ||
|
|
||
| WableLogger.log("\(isAuthorized)", for: .debug) | ||
|
|
||
| self.updateUserSessionUseCase.updateUserSession( | ||
| userID: account.user.id, | ||
| nickname: account.user.nickname, | ||
| profileURL: account.user.profileURL, | ||
| isPushAlarmAllowed: isAuthorized, | ||
| isAdmin: account.isAdmin, | ||
| isAutoLoginEnabled: true | ||
| ) | ||
| .sink(receiveCompletion: { _ in | ||
| }, receiveValue: { _ in | ||
| WableLogger.log("로컬에 세션 저장 완료", for: .debug) | ||
| }) | ||
| .store(in: cancelBag) | ||
|
|
||
| self.userProfileUseCase.execute(userID: account.user.id) | ||
| .sink { _ in | ||
| } receiveValue: { profile in | ||
| self.userProfileUseCase.execute(profile: profile, isPushAlarmAllowed: isAuthorized) | ||
| .sink { completion in | ||
| switch completion { | ||
| case .failure(let error): | ||
| WableLogger.log("서버로 프로필 업데이트 중 오류 발생: \(error)", for: .error) | ||
| default: | ||
| WableLogger.log("로그인 및 서버로 프로필 업데이트 완료", for: .debug) | ||
| } | ||
| } receiveValue: { _ in | ||
|
|
||
| } | ||
| .store(in: cancelBag) | ||
| } | ||
| .store(in: cancelBag) | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Async Task + Combine may leak work onto an arbitrary actor
Inside the Combine sink you spin up a detached Task { … } that:
- Calls the async
UNUserNotificationCenterAPI. - Performs several Combine pipelines (
updateUserSessionUseCase…,userProfileUseCase…) without awaiting their completion, then - Immediately lets the
sinkof the outer publisher finish.
Potential issues:
• The Task captures self strongly; if the view-model is deallocated the Task keeps it alive.
• Any publisher callbacks will run on the thread chosen by the publisher (often a background queue) while you are also inside an actor-hop created by Task {}. Side-effects to shared state (e.g. userSessionRepository) risk race conditions.
Suggestion:
- Task {
- let settings = await UNUserNotificationCenter.current().notificationSettings()
- let isAuthorized = settings.authorizationStatus == .authorized
-
- updateLocalSessionAndProfile(isAuthorized: isAuthorized, account: account)
- }
+ UNUserNotificationCenter.current()
+ .notificationSettings() // async
+ .publisher // convert to Combine
+ .map { $0.authorizationStatus == .authorized}
+ .flatMap { [unowned self] isAuth in
+ self.updateLocalSessionAndProfilePublisher(
+ isAuthorized: isAuth, account: account)
+ }
+ .sink(receiveCompletion: { … })
+ .store(in: cancelBag)This keeps everything in the Combine world, avoids the additional Task, and guarantees lifecycle-managed cancellation.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In Wable-iOS/Presentation/Login/LoginViewModel.swift between lines 89 and 126,
the current code uses a detached Task to call async APIs and start Combine
pipelines without awaiting them, which can cause strong self-captures and race
conditions due to actor hops. To fix this, remove the detached Task and instead
integrate the async call to UNUserNotificationCenter into the Combine chain
using a Future or Deferred publisher, then chain the subsequent Combine
publishers sequentially, ensuring all asynchronous work is managed within
Combine's lifecycle and cancellation system without spawning detached Tasks.
| import UIKit | ||
|
|
||
| import FirebaseCore | ||
| import FirebaseMessaging | ||
| import KakaoSDKCommon | ||
|
|
There was a problem hiding this comment.
Missing mandatory imports cause compilation failure
UNUserNotificationCenter (and its delegate) and Combine are both referenced later in the file, but the headers are not imported:
import UserNotifications // for UNUserNotificationCenter & delegate
import Combine // for CancelBag, AnyPublisher, sink, …Without these, the file will not compile.
🤖 Prompt for AI Agents
In Wable-iOS/App/AppDelegate.swift around lines 8 to 13, the imports for
UserNotifications and Combine are missing, causing compilation errors due to
undefined references to UNUserNotificationCenter and Combine types. Add the
lines 'import UserNotifications' and 'import Combine' alongside the existing
imports to resolve these missing dependencies and enable successful compilation.
|
|
#Conflicts: # Wable-iOS/Presentation/Home/View/HomeViewController.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Wable-iOS/Presentation/Onboarding/View/ProfileRegisterView.swift (1)
26-26: Consider a cleaner alternative to the single space placeholder.Using a single space as placeholder text for
titleLabelis a workaround that could cause unexpected layout calculations or behavior. Consider these cleaner alternatives:- $0.attributedText = " ".pretendardString(with: .head0) + $0.text = nilOr if you need to maintain the styling:
- $0.attributedText = " ".pretendardString(with: .head0) + $0.attributedText = "".pretendardString(with: .head0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Wable-iOS.xcodeproj/project.pbxproj(0 hunks)Wable-iOS/Presentation/Home/View/HomeDetailViewController.swift(2 hunks)Wable-iOS/Presentation/Home/View/HomeViewController.swift(1 hunks)Wable-iOS/Presentation/Home/ViewModel/HomeDetailViewModel.swift(7 hunks)Wable-iOS/Presentation/Home/ViewModel/HomeViewModel.swift(1 hunks)Wable-iOS/Presentation/Onboarding/View/ProfileRegisterView.swift(2 hunks)Wable-iOS/Presentation/Onboarding/ViewController/AgreementViewController.swift(1 hunks)Wable-iOS/Presentation/Onboarding/ViewController/LCKTeamViewController.swift(1 hunks)Wable-iOS/Presentation/Onboarding/ViewController/LCKYearViewController.swift(1 hunks)Wable-iOS/Presentation/Onboarding/ViewController/ProfileRegisterViewController.swift(1 hunks)Wable-iOS/Presentation/Profile/AlarmSetting/View/AlarmSettingViewController.swift(1 hunks)Wable-iOS/Presentation/Profile/My/View/MyProfileViewController.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Wable-iOS.xcodeproj/project.pbxproj
✅ Files skipped from review due to trivial changes (1)
- Wable-iOS/Presentation/Onboarding/ViewController/AgreementViewController.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Wable-iOS/Presentation/Profile/AlarmSetting/View/AlarmSettingViewController.swift
- Wable-iOS/Presentation/Profile/My/View/MyProfileViewController.swift
🧰 Additional context used
🧬 Code Graph Analysis (3)
Wable-iOS/Presentation/Home/View/HomeDetailViewController.swift (1)
Wable-iOS/Presentation/TabBar/TabBarController.swift (1)
tabBarController(189-205)
Wable-iOS/Presentation/Home/ViewModel/HomeDetailViewModel.swift (1)
Wable-iOS/Domain/Entity/Opacity.swift (1)
reduced(72-78)
Wable-iOS/Presentation/Home/View/HomeViewController.swift (1)
Wable-iOS/Presentation/TabBar/TabBarController.swift (1)
tabBarController(189-205)
🪛 SwiftLint (0.57.0)
Wable-iOS/Presentation/Home/ViewModel/HomeViewModel.swift
[Warning] 321-321: where clauses are preferred over a single if inside a for
(for_where)
🔇 Additional comments (9)
Wable-iOS/Presentation/Onboarding/ViewController/ProfileRegisterViewController.swift (1)
58-59: LGTM! Good UX improvement.Enabling the interactive pop gesture recognizer allows users to use the standard iOS swipe-to-go-back gesture, which improves the navigation experience. The placement in
setupView()is appropriate and consistent with similar changes in other onboarding view controllers.Wable-iOS/Presentation/Onboarding/ViewController/LCKYearViewController.swift (1)
46-47: LGTM! Good UX improvement for onboarding flow.Enabling the interactive pop gesture recognizer allows users to swipe back to previous screens, which improves the navigation experience in the onboarding flow. The placement in
setupView()is appropriate and the optional chaining ensures safety.Wable-iOS/Presentation/Onboarding/View/ProfileRegisterView.swift (1)
34-34: Good approach for controlling UI state during setup.The pattern of initially hiding the
descriptionLabeland then making it visible during configuration is a solid approach for controlling the UI presentation flow. This prevents flash of content and allows for better control over when elements appear.Also applies to: 183-184
Wable-iOS/Presentation/Onboarding/ViewController/LCKTeamViewController.swift (1)
50-51:Details
❓ Verification inconclusive
LGTM! Consider gesture interaction and onboarding UX.
The implementation correctly enables the interactive pop gesture recognizer with proper optional chaining for safety. This change aligns with the pattern applied across other onboarding view controllers.
However, consider these aspects:
- Gesture conflicts: Since this view contains a collection view with horizontal scrolling potential, ensure the swipe-to-go-back gesture doesn't interfere with collection view interactions
- Onboarding UX: In an onboarding flow, users might accidentally swipe back and lose their team selection progress. Consider if this aligns with the intended user experience
You may want to test the gesture interactions to ensure smooth UX:
🏁 Script executed:
#!/bin/bash # Search for other interactive pop gesture configurations to understand the pattern rg -A 3 -B 3 "interactivePopGestureRecognizer" --type swiftLength of output: 8619
Enable Interactive Pop Gesture—Consistent but Verify UX
This change aligns with your existing pattern—11 other view controllers (e.g., AgreementViewController, ProfileRegisterViewController, LCKYearViewController, HomeDetailViewController, etc.) already enable
interactivePopGestureRecognizerin their setup methods.Before merging, please verify:
- Gesture conflict
• Ensure the collection view’s horizontal scrolling isn’t disrupted by the edge-swipe back gesture.- Onboarding UX
• Confirm users won’t accidentally navigate back and lose progress (consider a confirmation step or temporarily disabling the gesture during critical selections).Test on-device to validate smooth navigation and onboarding flow.
Wable-iOS/Presentation/Home/View/HomeDetailViewController.swift (2)
142-142: LGTM! Good UX improvement.Enabling the interactive pop gesture recognizer enhances the user experience by allowing swipe-back navigation, which is a standard iOS pattern.
430-449: Well-implemented profile navigation logic.The conditional navigation logic is well-structured:
- Correctly switches to the profile tab (index 4) when the user taps their own profile
- Properly initializes
OtherProfileViewControllerwith dependency injection for other users- Matches the similar implementation in
HomeViewController.swiftfor consistencyThe dependency injection pattern using
FetchUserProfileUseCaseImpl()andCheckUserRoleUseCaseImplis properly implemented.Wable-iOS/Presentation/Home/View/HomeViewController.swift (1)
269-292: Excellent consistency with the detail view implementation.The profile navigation logic is well-implemented and maintains consistency with the similar implementation in
HomeDetailViewController.swift. The conditional navigation pattern properly:
- Switches to the profile tab (index 4) for the active user
- Pushes
OtherProfileViewControllerfor other users- Uses proper dependency injection with
UserDefaultsStorageinitializationThis consistency improves maintainability and user experience across the app.
Wable-iOS/Presentation/Home/ViewModel/HomeDetailViewModel.swift (2)
355-381: Well-structured banned user handling integration.The updated banned user handling properly integrates the new
updateBannedCommentshelper method and maintains consistency with the existing ghost user handling pattern. The logic correctly:
- Updates all comments by the banned user
- Updates the content if the banned user is the author
- Sets the appropriate
.blindstatus and reduces opacityThe implementation aligns well with the broader banning functionality across the app.
538-574: Excellent recursive comment update implementation.The
updateBannedCommentsmethod is well-implemented with proper recursive handling:
- Follows the same pattern as
updateGhostCommentsfor consistency- Correctly handles nested child comments through recursion
- Properly sets
.blindstatus and reduces opacity using thereduced()method- Maintains immutability by creating new instances
The recursive approach ensures all comments by the banned user are updated regardless of nesting level.
| input.didBannedTappedItem | ||
| .withUnretained(self) | ||
| .flatMap { owner, content -> AnyPublisher<Int, Never> in | ||
| return owner.createBannedUseCase.execute(memberID: content.0, triggerType: .content, triggerID: content.1) | ||
| .map { _ in content.0 } | ||
| .asDriver(onErrorJustReturn: -1) | ||
| } | ||
| .sink(receiveValue: { userID in | ||
| guard userID != -1 else { return } | ||
|
|
||
| var updatedContents = contentsSubject.value | ||
|
|
||
| for i in 0..<updatedContents.count { | ||
| if updatedContents[i].content.contentInfo.author.id == userID { | ||
| let content = updatedContents[i] | ||
| let contentInfo = content.content.contentInfo | ||
| let userContent = content.content | ||
| let opacity = contentInfo.opacity.reduced() | ||
|
|
||
| let updatedContent = Content( | ||
| content: UserContent( | ||
| id: userContent.id, | ||
| contentInfo: ContentInfo( | ||
| author: contentInfo.author, | ||
| createdDate: contentInfo.createdDate, | ||
| title: contentInfo.title, | ||
| imageURL: contentInfo.imageURL, | ||
| text: contentInfo.text, | ||
| status: .blind, | ||
| like: contentInfo.like, | ||
| opacity: opacity, | ||
| commentCount: contentInfo.commentCount | ||
| ) | ||
| ), | ||
| isDeleted: content.isDeleted | ||
| ) | ||
|
|
||
| updatedContents[i] = updatedContent | ||
| } | ||
| } | ||
|
|
||
| contentsSubject.send(updatedContents) | ||
| }) | ||
| .store(in: cancelBag) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address code duplication and apply static analysis suggestion.
This new reactive pipeline duplicates the logic from the existing didBannedTappedItem handler (lines 267-306). The duplication creates maintenance issues and inconsistency risks.
Additionally, the static analysis tool correctly identifies that the for loop with an if condition (line 321) should use a where clause for better Swift idiom.
Consider refactoring to eliminate duplication and apply the static analysis suggestion:
- input.didBannedTappedItem
- .withUnretained(self)
- .flatMap { owner, content -> AnyPublisher<Int, Never> in
- return owner.createBannedUseCase.execute(memberID: content.0, triggerType: .content, triggerID: content.1)
- .map { _ in content.0 }
- .asDriver(onErrorJustReturn: -1)
- }
- .sink(receiveValue: { userID in
- guard userID != -1 else { return }
-
- var updatedContents = contentsSubject.value
-
- for i in 0..<updatedContents.count {
- if updatedContents[i].content.contentInfo.author.id == userID {
- let content = updatedContents[i]
- let contentInfo = content.content.contentInfo
- let userContent = content.content
- let opacity = contentInfo.opacity.reduced()
-
- let updatedContent = Content(
- content: UserContent(
- id: userContent.id,
- contentInfo: ContentInfo(
- author: contentInfo.author,
- createdDate: contentInfo.createdDate,
- title: contentInfo.title,
- imageURL: contentInfo.imageURL,
- text: contentInfo.text,
- status: .blind,
- like: contentInfo.like,
- opacity: opacity,
- commentCount: contentInfo.commentCount
- )
- ),
- isDeleted: content.isDeleted
- )
-
- updatedContents[i] = updatedContent
- }
- }
-
- contentsSubject.send(updatedContents)
- })
- .store(in: cancelBag)And update the existing handler to use a where clause:
- for i in 0..<updatedContents.count {
- if updatedContents[i].content.contentInfo.author.id == userID {
+ for i in 0..<updatedContents.count where updatedContents[i].content.contentInfo.author.id == userID {Instead, consider creating a helper method to update all contents by a banned user and remove the duplicate pipeline.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 321-321: where clauses are preferred over a single if inside a for
(for_where)
🤖 Prompt for AI Agents
In Wable-iOS/Presentation/Home/ViewModel/HomeViewModel.swift between lines 308
and 351, the reactive pipeline duplicates the logic already implemented in the
didBannedTappedItem handler at lines 267-306, causing maintenance and
consistency issues. Refactor by extracting the content update logic into a
helper method that updates all contents by a banned user, then call this helper
from the existing handler to avoid duplication. Also, replace the for-loop with
an if condition at line 321 by using a for-in loop with a where clause to
conform to Swift idiomatic style. Remove the duplicate pipeline entirely after
refactoring.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Wable-iOS/Presentation/Home/ViewModel/HomeViewModel.swift(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Wable-iOS/Presentation/Home/ViewModel/HomeViewModel.swift (1)
Wable-iOS/Domain/Entity/Opacity.swift (1)
reduced(72-78)
🪛 SwiftLint (0.57.0)
Wable-iOS/Presentation/Home/ViewModel/HomeViewModel.swift
[Warning] 208-208: where clauses are preferred over a single if inside a for
(for_where)
[Warning] 282-282: where clauses are preferred over a single if inside a for
(for_where)
| .sink(receiveValue: { userID in | ||
| var updatedContents = contentsSubject.value | ||
|
|
||
| let content = updatedContents[index] | ||
| let contentInfo = content.content.contentInfo | ||
| let userContent = content.content | ||
| let opacity = contentInfo.opacity.reduced() | ||
|
|
||
| let updatedContent = Content( | ||
| content: UserContent( | ||
| id: userContent.id, | ||
| contentInfo: ContentInfo( | ||
| author: contentInfo.author, | ||
| createdDate: contentInfo.createdDate, | ||
| title: contentInfo.title, | ||
| imageURL: contentInfo.imageURL, | ||
| text: contentInfo.text, | ||
| status: .ghost, | ||
| like: contentInfo.like, | ||
| opacity: opacity, | ||
| commentCount: contentInfo.commentCount | ||
| for i in 0..<updatedContents.count { | ||
| if updatedContents[i].content.contentInfo.author.id == userID { | ||
| let content = updatedContents[i] | ||
| let contentInfo = content.content.contentInfo | ||
| let userContent = content.content | ||
| let opacity = contentInfo.opacity.reduced() | ||
|
|
||
| let updatedContent = Content( | ||
| content: UserContent( | ||
| id: userContent.id, | ||
| contentInfo: ContentInfo( | ||
| author: contentInfo.author, | ||
| createdDate: contentInfo.createdDate, | ||
| title: contentInfo.title, | ||
| imageURL: contentInfo.imageURL, | ||
| text: contentInfo.text, | ||
| status: .ghost, | ||
| like: contentInfo.like, | ||
| opacity: opacity, | ||
| commentCount: contentInfo.commentCount | ||
| ) | ||
| ), | ||
| isDeleted: content.isDeleted | ||
| ) | ||
| ), | ||
| isDeleted: content.isDeleted | ||
| ) | ||
|
|
||
| updatedContents[i] = updatedContent | ||
| } | ||
| } | ||
|
|
||
| updatedContents[index] = updatedContent | ||
| contentsSubject.send(updatedContents) | ||
| }) | ||
| .store(in: cancelBag) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common content update logic to eliminate duplication.
The didGhostTappedItem handler duplicates significant logic with the didBannedTappedItem handler. Both iterate through all contents, check author IDs, create updated content with reduced opacity, and update the contents array. The only differences are the status value (.ghost vs .blind) and the presence of a guard clause.
Additionally, the SwiftLint warning correctly identifies that the for loop with if condition should use a where clause for better Swift idiom.
Consider extracting the common update logic into a helper method:
+private func updateContentsByUser(
+ userID: Int,
+ newStatus: ContentStatus,
+ contents: inout [Content]
+) {
+ for i in 0..<contents.count where contents[i].content.contentInfo.author.id == userID {
+ let content = contents[i]
+ let contentInfo = content.content.contentInfo
+ let userContent = content.content
+ let opacity = contentInfo.opacity.reduced()
+
+ let updatedContent = Content(
+ content: UserContent(
+ id: userContent.id,
+ contentInfo: ContentInfo(
+ author: contentInfo.author,
+ createdDate: contentInfo.createdDate,
+ title: contentInfo.title,
+ imageURL: contentInfo.imageURL,
+ text: contentInfo.text,
+ status: newStatus,
+ like: contentInfo.like,
+ opacity: opacity,
+ commentCount: contentInfo.commentCount
+ )
+ ),
+ isDeleted: content.isDeleted
+ )
+
+ contents[i] = updatedContent
+ }
+}Then simplify the ghost handler:
- .sink(receiveValue: { userID in
- var updatedContents = contentsSubject.value
-
- for i in 0..<updatedContents.count {
- if updatedContents[i].content.contentInfo.author.id == userID {
- let content = updatedContents[i]
- let contentInfo = content.content.contentInfo
- let userContent = content.content
- let opacity = contentInfo.opacity.reduced()
-
- let updatedContent = Content(
- content: UserContent(
- id: userContent.id,
- contentInfo: ContentInfo(
- author: contentInfo.author,
- createdDate: contentInfo.createdDate,
- title: contentInfo.title,
- imageURL: contentInfo.imageURL,
- text: contentInfo.text,
- status: .ghost,
- like: contentInfo.like,
- opacity: opacity,
- commentCount: contentInfo.commentCount
- )
- ),
- isDeleted: content.isDeleted
- )
-
- updatedContents[i] = updatedContent
- }
- }
-
- contentsSubject.send(updatedContents)
- })
+ .sink(receiveValue: { [weak self] userID in
+ var updatedContents = contentsSubject.value
+ self?.updateContentsByUser(userID: userID, newStatus: .ghost, contents: &updatedContents)
+ contentsSubject.send(updatedContents)
+ })📝 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.
| .sink(receiveValue: { userID in | |
| var updatedContents = contentsSubject.value | |
| let content = updatedContents[index] | |
| let contentInfo = content.content.contentInfo | |
| let userContent = content.content | |
| let opacity = contentInfo.opacity.reduced() | |
| let updatedContent = Content( | |
| content: UserContent( | |
| id: userContent.id, | |
| contentInfo: ContentInfo( | |
| author: contentInfo.author, | |
| createdDate: contentInfo.createdDate, | |
| title: contentInfo.title, | |
| imageURL: contentInfo.imageURL, | |
| text: contentInfo.text, | |
| status: .ghost, | |
| like: contentInfo.like, | |
| opacity: opacity, | |
| commentCount: contentInfo.commentCount | |
| for i in 0..<updatedContents.count { | |
| if updatedContents[i].content.contentInfo.author.id == userID { | |
| let content = updatedContents[i] | |
| let contentInfo = content.content.contentInfo | |
| let userContent = content.content | |
| let opacity = contentInfo.opacity.reduced() | |
| let updatedContent = Content( | |
| content: UserContent( | |
| id: userContent.id, | |
| contentInfo: ContentInfo( | |
| author: contentInfo.author, | |
| createdDate: contentInfo.createdDate, | |
| title: contentInfo.title, | |
| imageURL: contentInfo.imageURL, | |
| text: contentInfo.text, | |
| status: .ghost, | |
| like: contentInfo.like, | |
| opacity: opacity, | |
| commentCount: contentInfo.commentCount | |
| ) | |
| ), | |
| isDeleted: content.isDeleted | |
| ) | |
| ), | |
| isDeleted: content.isDeleted | |
| ) | |
| updatedContents[i] = updatedContent | |
| } | |
| } | |
| updatedContents[index] = updatedContent | |
| contentsSubject.send(updatedContents) | |
| }) | |
| .store(in: cancelBag) | |
| .sink(receiveValue: { [weak self] userID in | |
| var updatedContents = contentsSubject.value | |
| self?.updateContentsByUser(userID: userID, newStatus: .ghost, contents: &updatedContents) | |
| contentsSubject.send(updatedContents) | |
| }) | |
| .store(in: cancelBag) |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 208-208: where clauses are preferred over a single if inside a for
(for_where)
🤖 Prompt for AI Agents
In Wable-iOS/Presentation/Home/ViewModel/HomeViewModel.swift between lines 204
and 238, the code in the didGhostTappedItem handler duplicates logic found in
didBannedTappedItem, both iterating contents and updating items with reduced
opacity and different status values. To fix this, extract the common update
logic into a helper method that takes the author ID and status as parameters,
applies the opacity reduction, and updates the contents array accordingly. Also,
refactor the for loop to use a where clause for filtering by author ID to follow
Swift idioms. Then, simplify the didGhostTappedItem handler by calling this
helper method with the appropriate status.
| .sink(receiveValue: { userID in | ||
| guard userID != -1 else { return } | ||
|
|
||
| let content = updatedContents[index] | ||
| let contentInfo = content.content.contentInfo | ||
| let userContent = content.content | ||
| let opacity = contentInfo.opacity.reduced() | ||
| var updatedContents = contentsSubject.value | ||
|
|
||
| let updatedContent = Content( | ||
| content: UserContent( | ||
| id: userContent.id, | ||
| contentInfo: ContentInfo( | ||
| author: contentInfo.author, | ||
| createdDate: contentInfo.createdDate, | ||
| title: contentInfo.title, | ||
| imageURL: contentInfo.imageURL, | ||
| text: contentInfo.text, | ||
| status: .blind, | ||
| like: contentInfo.like, | ||
| opacity: opacity, | ||
| commentCount: contentInfo.commentCount | ||
| for i in 0..<updatedContents.count { | ||
| if updatedContents[i].content.contentInfo.author.id == userID { | ||
| let content = updatedContents[i] | ||
| let contentInfo = content.content.contentInfo | ||
| let userContent = content.content | ||
| let opacity = contentInfo.opacity.reduced() | ||
|
|
||
| let updatedContent = Content( | ||
| content: UserContent( | ||
| id: userContent.id, | ||
| contentInfo: ContentInfo( | ||
| author: contentInfo.author, | ||
| createdDate: contentInfo.createdDate, | ||
| title: contentInfo.title, | ||
| imageURL: contentInfo.imageURL, | ||
| text: contentInfo.text, | ||
| status: .blind, | ||
| like: contentInfo.like, | ||
| opacity: opacity, | ||
| commentCount: contentInfo.commentCount | ||
| ) | ||
| ), | ||
| isDeleted: content.isDeleted | ||
| ) | ||
| ), | ||
| isDeleted: content.isDeleted | ||
| ) | ||
|
|
||
| updatedContents[i] = updatedContent | ||
| } | ||
| } | ||
|
|
||
| updatedContents[index] = updatedContent | ||
| contentsSubject.send(updatedContents) | ||
| }) | ||
| .store(in: cancelBag) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Apply the same refactoring to eliminate code duplication.
This handler has nearly identical logic to the didGhostTappedItem handler, with only minor differences (the guard clause and .blind status). After implementing the helper method suggested above, this handler should also be simplified.
Apply this diff to use the extracted helper method:
- .sink(receiveValue: { userID in
- guard userID != -1 else { return }
-
- var updatedContents = contentsSubject.value
-
- for i in 0..<updatedContents.count {
- if updatedContents[i].content.contentInfo.author.id == userID {
- let content = updatedContents[i]
- let contentInfo = content.content.contentInfo
- let userContent = content.content
- let opacity = contentInfo.opacity.reduced()
-
- let updatedContent = Content(
- content: UserContent(
- id: userContent.id,
- contentInfo: ContentInfo(
- author: contentInfo.author,
- createdDate: contentInfo.createdDate,
- title: contentInfo.title,
- imageURL: contentInfo.imageURL,
- text: contentInfo.text,
- status: .blind,
- like: contentInfo.like,
- opacity: opacity,
- commentCount: contentInfo.commentCount
- )
- ),
- isDeleted: content.isDeleted
- )
-
- updatedContents[i] = updatedContent
- }
- }
-
- contentsSubject.send(updatedContents)
- })
+ .sink(receiveValue: { [weak self] userID in
+ guard userID != -1 else { return }
+
+ var updatedContents = contentsSubject.value
+ self?.updateContentsByUser(userID: userID, newStatus: .blind, contents: &updatedContents)
+ contentsSubject.send(updatedContents)
+ })This refactoring eliminates ~60 lines of duplicated code and addresses the SwiftLint warnings about for-where clause usage.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 282-282: where clauses are preferred over a single if inside a for
(for_where)
🤖 Prompt for AI Agents
In Wable-iOS/Presentation/Home/ViewModel/HomeViewModel.swift between lines 276
and 312, the sink handler duplicates logic found in the didGhostTappedItem
handler, differing mainly in the guard condition and the status set to .blind.
Refactor by extracting the common update logic into a helper method that accepts
parameters for the guard condition and status, then call this helper from both
handlers. This will remove about 60 lines of duplicated code and resolve
SwiftLint warnings related to for-where usage.
JinUng41
left a comment
JinUng41
left a comment
There was a problem hiding this comment.
고생하셨습니다~
이제 편히 자러 갑시다~
| jsonDecoder: JSONDecoder() | ||
| guard let self = self else { return } | ||
|
|
||
| if self.activeUserID == item.content.contentInfo.author.id, |
There was a problem hiding this comment.
네!!!!!!!!!!!!!!!!!!!!!!!
| let content = updatedContents[i] | ||
| let contentInfo = content.content.contentInfo | ||
| let userContent = content.content | ||
| let opacity = contentInfo.opacity.reduced() | ||
|
|
||
| let updatedContent = Content( | ||
| content: UserContent( | ||
| id: userContent.id, | ||
| contentInfo: ContentInfo( | ||
| author: contentInfo.author, | ||
| createdDate: contentInfo.createdDate, | ||
| title: contentInfo.title, | ||
| imageURL: contentInfo.imageURL, | ||
| text: contentInfo.text, | ||
| status: .ghost, | ||
| like: contentInfo.like, | ||
| opacity: opacity, | ||
| commentCount: contentInfo.commentCount | ||
| ) | ||
| ), | ||
| isDeleted: content.isDeleted |
There was a problem hiding this comment.
빨리 Content와 Comment를 하나로 통합해야 겠어요.
| guard userID != -1 else { return } | ||
|
|
There was a problem hiding this comment.
sink 전에 해당 조건을 가지고 filter를 써도 좋을 것 같네요.
|
|
||
| // TODO: 서버로 알람 수신 여부 보내기 | ||
| } |
👻 PULL REQUEST
📄 작업 내용
"문제의 원인은 예상치 못한 곳에 있을 수 있다"
💻 주요 코드 설명
푸시 알림 설정 확인 코드 및 서버 전송 코드 추가
isPushAlarmAllowed상태를 저장하고 서버에 전송하지 않아서(;;) 일어난 이슈였습니다.LoginViewModel에 아래와 같이 코드를 추가했습니다. (개선 예정...)📚 참고자료
[iOS/macOS] FCM을 사용한 Remote Push Notification 구현 방법과 문제 해결 기록
Remote Push Notification(with FCM) 예제 프로젝트
👀 기타 더 이야기해볼 점
AppDelegate코드에 주석이 좀 있는데 나중에 주석 참고해서 분리 예정이라 그때까지만 양해 부탁드립니다AppDelegate는 공사중!...🔗 연결된 이슈
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores