[Fix] 커뮤니티 탭 에러 수정 #234
[Fix] 커뮤니티 탭 에러 수정 #234
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change refactors the community feature's reactive bindings from custom relays to Combine's subjects and publishers, reorganizes view and view model state management, and streamlines UI event handling. It also ensures that the community registration state is accurately reflected on the initial app launch, addressing an initialization issue. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CommunityView
participant CommunityViewController
participant CommunityViewModel
participant CommunityRepository
User->>CommunityView: Tap Ask Button / Pull to Refresh
CommunityView->>CommunityViewController: askDidTap / didRefresh (Publisher)
CommunityViewController->>CommunityViewModel: registerSubject.send / refreshSubject.send
CommunityViewModel->>CommunityRepository: checkUserRegistration (initial load)
CommunityRepository-->>CommunityViewModel: UserRegistrationState
CommunityViewModel->>CommunityRepository: fetchCommunityList
CommunityRepository-->>CommunityViewModel: [Community]
CommunityViewModel-->>CommunityViewController: communityItems (Publisher)
CommunityViewController->>CommunityView: Update UI
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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: 1
🧹 Nitpick comments (1)
Wable-iOS/Presentation/Community/ViewModel/CommunityViewModel.swift (1)
95-101: Consider thread safety for subject updatesWhile the current implementation should work correctly on the main thread, consider using
receive(on:)operator before updating thecommunityListSubjectto ensure thread safety..sink { [weak self] team, updatedRate in guard let index = self?.communityListSubject.value.firstIndex(where: { $0.team == team }) else { return WableLogger.log("팀을 찾을 수 없습니다.", for: .debug) } - self?.communityListSubject.value[index].registrationRate = updatedRate + var updatedList = self?.communityListSubject.value ?? [] + updatedList[index].registrationRate = updatedRate + self?.communityListSubject.send(updatedList) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Wable-iOS/Domain/UseCase/Community/CommunityUseCase.swift(1 hunks)Wable-iOS/Presentation/Community/View/CommunityViewController.swift(8 hunks)Wable-iOS/Presentation/Community/View/Subview/CommunityView.swift(4 hunks)Wable-iOS/Presentation/Community/ViewModel/CommunityViewModel.swift(3 hunks)
🔇 Additional comments (6)
Wable-iOS/Presentation/Community/View/Subview/CommunityView.swift (2)
8-8: Well-structured reactive refactoringThe promotion of UI components to properties and addition of Combine publishers properly exposes UI events for reactive bindings. This aligns well with the overall refactoring to use Combine's native publishers.
Also applies to: 18-21, 29-30, 39-39, 56-57
33-33: Good use of string literalsUsing
StringLiterals.Community.askButtonTitleinstead of hardcoded strings improves maintainability and localization support.Wable-iOS/Presentation/Community/View/CommunityViewController.swift (2)
35-36: Clean migration to Combine subjectsThe refactoring from custom relays to Combine's native
PassthroughSubjectis well-implemented. The setupAction properly subscribes to the view's publishers and the input bindings are correctly updated.Also applies to: 74-79, 152-154
254-255: Simplified scroll behaviorThe direct use of
setContentOffsetis cleaner than the previous implementation.Wable-iOS/Presentation/Community/ViewModel/CommunityViewModel.swift (2)
54-65: Correct sequencing fixes the initialization issueThe
bindInitialLoadmethod correctly ensures that user registration status is fetched first before loading the community list. This sequential execution addresses the PR's main objective of fixing the issue where pre-registered users saw the initial application screen.
40-50: Well-structured reactive architectureThe refactoring with separated binding methods and publisher creation improves code organization and maintainability. The state management using
CurrentValueSubjectis more idiomatic for Combine.Also applies to: 105-128
| func register(for communityTeam: LCKTeam) -> AnyPublisher<Double, WableError> { | ||
| return .just(0.92) | ||
| return .just(88.0) |
There was a problem hiding this comment.
Inconsistent value range with other mock data
The registration rate returned here (88.0) appears to be in a 0-100 range, while the registration rates in fetchCommunityList() are in a 0-1 range (e.g., 0.91, 0.88). This inconsistency could cause issues if consumers expect consistent value ranges.
Consider aligning the value ranges:
- return .just(88.0)
+ return .just(0.88)📝 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 register(for communityTeam: LCKTeam) -> AnyPublisher<Double, WableError> { | |
| return .just(0.92) | |
| return .just(88.0) | |
| func register(for communityTeam: LCKTeam) -> AnyPublisher<Double, WableError> { | |
| return .just(0.88) |
🤖 Prompt for AI Agents
In Wable-iOS/Domain/UseCase/Community/CommunityUseCase.swift at lines 66-67, the
mock registration rate returned is 88.0, which is inconsistent with the 0-1
range used in fetchCommunityList(). Change the returned value to a decimal
between 0 and 1, such as 0.88, to align with the other mock data and ensure
consistent value ranges.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
Wable-iOS/Data/RepositoryImpl/CommunityRepositoryImpl.swift (1)
20-31: Name mismatch between API target and method
updateRegistration()still calls.updateRegisteronCommunityTargetType..updateRegister( request: DTO.Request.UpdateRegister(...) )If the server endpoint was renamed in parallel to the Swift API, update the target as well to avoid 404s.
-.updateRegister( +.updateRegistration(
🧹 Nitpick comments (4)
Wable-iOS/App/AppDelegate+InjectDependency.swift (2)
81-88: Nit: unusedenvin several one-liner registrationsThroughout the file (
GhostRepository,CommunityRepository, …) closures keep theenvparameter even when it is ignored and a single implementation is returned.Example:
diContainer.register(for: GhostRepository.self) { env in return GhostRepositoryImpl() }Dropping the parameter and using the sugar
object:overload would remove noise.-diContainer.register(for: GhostRepository.self) { env in - GhostRepositoryImpl() -} +diContainer.register(for: GhostRepository.self, object: GhostRepositoryImpl())
83-87: Consider wiring the mock registration under#if DEBUGonlyShipping
MockCommunityRepository()in production by accident would silently bypass real network calls.
Guard the switch with#if DEBUGor remove.mockwhen not used to reduce attack surface.Wable-iOS/Data/RepositoryImpl/CommunityRepositoryImpl.swift (1)
49-56: Combine helper.justis non-standard
Combineitself does not offer a top-level.just(_)helper; it lives inCombineExt.
If that dependency is ever removed, these calls will fail to compile. Safer:return Just(88.0) .setFailureType(to: WableError.self) .delay(for: .seconds(randomDelay), scheduler: DispatchQueue.main) .eraseToAnyPublisher()Wable-iOS/Presentation/Community/ViewModel/CommunityViewModel.swift (1)
128-136: Authorization check spawns a newUNUserNotificationCenterquery per tap
getNotificationSettingsis inexpensive but you can cache the result for the session:private lazy var notificationAuthSubject = CurrentValueSubject<Bool, Never>(false) UNUserNotificationCenter.current().getNotificationSettings { settings in notificationAuthSubject.send(settings.authorizationStatus == .authorized) }Then expose
notificationAuthSubject.asDriver()instead of making a fresh call each time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Wable-iOS.xcodeproj/project.pbxproj(0 hunks)Wable-iOS/App/AppDelegate+InjectDependency.swift(3 hunks)Wable-iOS/Data/RepositoryImpl/CommunityRepositoryImpl.swift(2 hunks)Wable-iOS/Domain/RepositoryInterface/CommunityRepository.swift(1 hunks)Wable-iOS/Domain/UseCase/Community/CommunityUseCase.swift(0 hunks)Wable-iOS/Presentation/Community/ViewModel/CommunityViewModel.swift(2 hunks)Wable-iOS/Presentation/TabBar/TabBarController.swift(1 hunks)
💤 Files with no reviewable changes (2)
- Wable-iOS.xcodeproj/project.pbxproj
- Wable-iOS/Domain/UseCase/Community/CommunityUseCase.swift
✅ Files skipped from review due to trivial changes (1)
- Wable-iOS/Domain/RepositoryInterface/CommunityRepository.swift
🔇 Additional comments (1)
Wable-iOS/Presentation/TabBar/TabBarController.swift (1)
45-47: Verify that DI registration resolvesCommunityRepositoryat runtime
CommunityViewModel()now relies entirely on the@Injectedproperty wrapper.
If theCommunityRepositoryregistration is omitted or is registered after this VC is created (e.g. from a scene-specific container), the app will crash with a fatalError from the resolver.Double-check that:
AppDelegate.injectDependency()is executed before theTabBarControlleris constructed.- The registration for
.productionenv is the one selected at run time.Otherwise keep the explicit
viewModel:constructor that takes the repository for extra safety.
| .compactMap { [weak self] in self?.communityListSubject.value[$0].team } | ||
| .handleEvents(receiveOutput: { [weak self] team in | ||
| self?.userRegistrationState = CommunityRegistration(team: team, hasRegisteredTeam: true) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
State mutated before network confirmation
userRegistrationState is set optimistically before updateRegistration succeeds.
If the request fails you will present an incorrect state until next app launch.
Move the assignment to the success branch:
.handleEvents(receiveOutput: { [weak self] _ in
self?.registrationCompletedSubject.send(team)
self?.userRegistrationState = CommunityRegistration(team: team, hasRegisteredTeam: true)
})🤖 Prompt for AI Agents
In Wable-iOS/Presentation/Community/ViewModel/CommunityViewModel.swift around
lines 75 to 78, the userRegistrationState is being set optimistically before the
network request updateRegistration confirms success, which can cause incorrect
UI state if the request fails. To fix this, move the assignment of
userRegistrationState into the success branch of the network call, specifically
inside the handleEvents receiveOutput closure after confirming the update
succeeded, and also send the registrationCompletedSubject event there to ensure
state is only updated on successful registration.
| .sink { [weak self] team, updatedRate in | ||
| guard let index = self?.communityListSubject.value.firstIndex(where: { $0.team == team }) else { | ||
| return WableLogger.log("팀을 찾을 수 없습니다.", for: .debug) | ||
| } | ||
|
|
||
| communityListRelay.value[index].registrationRate = updatedRate | ||
| self?.communityListSubject.value[index].registrationRate = updatedRate | ||
| } |
There was a problem hiding this comment.
Registration rate update is not published
CurrentValueSubject only notifies subscribers when you assign a new value.
Mutating an element inside the stored array does not emit, so the UI never sees the updated rate.
-self?.communityListSubject.value[index].registrationRate = updatedRate
+if var list = self?.communityListSubject.value {
+ list[index].registrationRate = updatedRate
+ self?.communityListSubject.send(list) // emit new value
+}📝 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 { [weak self] team, updatedRate in | |
| guard let index = self?.communityListSubject.value.firstIndex(where: { $0.team == team }) else { | |
| return WableLogger.log("팀을 찾을 수 없습니다.", for: .debug) | |
| } | |
| communityListRelay.value[index].registrationRate = updatedRate | |
| self?.communityListSubject.value[index].registrationRate = updatedRate | |
| } | |
| .sink { [weak self] team, updatedRate in | |
| guard let index = self?.communityListSubject.value.firstIndex(where: { $0.team == team }) else { | |
| return WableLogger.log("팀을 찾을 수 없습니다.", for: .debug) | |
| } | |
| if var list = self?.communityListSubject.value { | |
| list[index].registrationRate = updatedRate | |
| self?.communityListSubject.send(list) // emit new value | |
| } | |
| } |
🤖 Prompt for AI Agents
In Wable-iOS/Presentation/Community/ViewModel/CommunityViewModel.swift around
lines 92 to 98, the issue is that mutating an element inside the array held by
communityListSubject does not trigger a publish event. To fix this, after
updating the registrationRate on the element, reassign the entire array to
communityListSubject.value to emit the change and notify subscribers.
youz2me
left a comment
youz2me
left a comment
There was a problem hiding this comment.
굿!! 커뮤니티 탭은 이미 완벽하다고 생각했는데 이런 에러가 있었군요. 저도 제 뷰 한번 다시 점검해보도록 하겠습니다 ㅎㅎ...
이번에 스프린트를 진행하면서 세세한 부분에서 각자 코드를 다르게 작성하는 부분이 있다고 느꼈는데요! 이런 부분은 한번 조율해봐도 좋을 것 같습니다. (e.g. MARK 주석 위치, 개행 등)
리뷰 몇가지를 달아놓았으니 확인 부탁드립니닷. (대부분 질문이예요 ㅎㅎ...) 고생하셨습니다 ~!
| case .mock: MockUpdateAlertPolicyRepository() | ||
| case .production: UpdateAlertPolicyRepositoryImpl() | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Community | ||
|
|
||
| diContainer.register(for: CommunityRepository.self) { env in | ||
| switch env { | ||
| case .mock: MockCommunityRepository() | ||
| case .production: CommunityRepositoryImpl() |
There was a problem hiding this comment.
저는 Switch 문이 한줄만 있어도 줄바꿈을 하는 편이라... 이런 부분은 이야기해보고 맞추면 좋지 않을까 생각이 들어요.
There was a problem hiding this comment.
코드를 많이 작성하다 보니, 가장 간결한게 최고인 것 같아서 우선 개행하지 않는 편으로 작성하게 되었습니다.
다가오는 토의 시간에 이야기를 해보면 좋을 것 같아요.
| } | ||
|
|
||
| func isUserRegistered() -> AnyPublisher<CommunityRegistration, WableError> { | ||
| func checkUserRegistration() -> AnyPublisher<CommunityRegistration, WableError> { |
There was a problem hiding this comment.
함수 이름이 깔끔해졌네요! 확실히 동사+주어 조합이 깔끔하고 알아보기 쉬운 것 같습니다. 👍
| private let viewDidLoadRelay = PassthroughRelay<Void>() | ||
| private let viewDidRefreshRelay = PassthroughRelay<Void>() | ||
| private let registerRelay = PassthroughRelay<Int>() | ||
| private let copyLinkRelay = PassthroughRelay<Void>() | ||
| private let checkNotificationAuthorizationRelay = PassthroughRelay<Void>() | ||
| private let registerSubject = PassthroughSubject<Int, Never>() | ||
| private let checkNotificationAuthorizationSubject = PassthroughSubject<Void, Never>() |
There was a problem hiding this comment.
Relay에서 Subject로 구현 방식을 변경하신 이유가 있을까요 ???
There was a problem hiding this comment.
Community 로직을 수정하면서 UIControl의 이벤트를 바로 사용할 수 있다보니, Relay라는 타입 별칭을 사용할 일이 줄어들었습니다.
그렇다 보니 선언 자체에 대한 의문증이 들었습니다.
그래서 일단은 서브젝트로 작성하였습니다.
| let viewDidRefresh: Driver<Void> | ||
| let refresh: Driver<Void> |
There was a problem hiding this comment.
헉 저는 개인적으로 ViewDidRefresh가 더 직관적인 네이밍이라 좋은 것 같긴 합니닷. 혹시 변경하신 이유가 어떤걸까요...?? 👀
There was a problem hiding this comment.
특별한 이유보다는 코드와 표현을 최대한 간결하게 해보고 싶었어서 다음과 같이 작성했습니다.
| let communityListRelay = CurrentValueRelay<[Community]>([]) | ||
| let isLoadingRelay = CurrentValueRelay<Bool>(false) | ||
| let completeRegistrationRelay = CurrentValueRelay<LCKTeam?>(nil) | ||
| bindInitialLoad(cancelBag: cancelBag) |
There was a problem hiding this comment.
오... 이렇게 하면 뷰가 초기화될 때 한번만 실행할 수 있어서 viewDidLoad를 지워도 되는건가요??
There was a problem hiding this comment.
전에도 viewDidLoad를 무엇 때문에, 사용했는지 모르겠습니다.
transform은 결국 뷰컨트롤러의 setupBinding에서 호출되고, setupBinding은 viewDidLoad에서 호출되기 때문에, 사실상 같은 시점이라고 생각되어 이번에 수정하게 되었습니다.
github-project-automation
bot
moved this from Ready for Review
to In Review
in Wable-iOS
👻 PULL REQUEST
📄 작업 내용
💻 주요 코드 설명
커뮤니티 탭의 오류 해결
👀 기타 더 이야기해볼 점
🔗 연결된 이슈
Summary by CodeRabbit