[Refactor] Combine+UIKit, withLatestFrom 구현하기 #202
Conversation
- UIControlEvent - UIControlProperty - UIGesture
- 뷰잇 생성 페이지에 부분 적용
|
Caution Review failedThe pull request is closed. """ WalkthroughThe changes remove the CombineCocoa dependency and introduce custom Combine publishers for UIKit controls and gestures. New files implement publishers for UIControl events, properties, and gestures. The withLatestFrom operator is added for Combine publishers. Event handling in several view controllers is refactored to use these new publishers instead of CombineCocoa or manual relays. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UIControl
participant CombinePublisher
participant ViewController
participant ViewModel
User->>UIControl: Interact (e.g., tap, edit)
UIControl->>CombinePublisher: Emits event/property change
CombinePublisher->>ViewController: Publishes event
ViewController->>ViewModel: Binds published event as input
ViewModel-->>ViewController: Processes input, emits output
sequenceDiagram
participant PublisherA
participant PublisherB
participant withLatestFrom
participant Subscriber
PublisherA->>withLatestFrom: Emits value
withLatestFrom->>PublisherB: Get latest value
withLatestFrom->>Subscriber: Emit (PublisherA, PublisherB.latest)
Assessment against linked issues
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
Wable-iOS/Presentation/Viewit/Create/View/Subview/ViewitInputView.swift (1)
95-100: Good button tap publisher with debouncing.The publisher correctly handles button taps with debouncing to prevent rapid successive taps. The 300ms debounce timing might be slightly conservative (typically 200-250ms), but this could be intentional for your use case.
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIGesture.swift (1)
160-163: Enhancement: Consider gesture recognizer configuration options.The current implementation creates new gesture recognizers each time. Consider adding support for:
- Configuring gesture recognizer properties (number of taps, minimum duration, etc.)
- Reusing gesture recognizers when possible
- Providing a builder pattern for complex gesture configurations
Example enhancement:
func gesture<T: UIGestureRecognizer>( _ gestureType: Publishers.GestureType = .tap(), configure: ((T) -> Void)? = nil ) -> Publishers.GesturePublisher { self.isUserInteractionEnabled = true if let config = configure, case let gesture = gestureType.gesture as? T { config(gesture) } return Publishers.GesturePublisher(view: self, gestureType: gestureType) }Wable-iOS/Core/Combine+/Publisher+.swift (1)
63-197: Consider creating comprehensive unit tests for these operators.These operators are complex and handle edge cases that need thorough testing, especially:
- Behavior when referenced publishers haven't emitted yet
- Rapid emission scenarios
- Memory management with weak references
- Different failure types and error propagation
Would you like me to generate comprehensive unit tests for these
withLatestFromoperators?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
Wable-iOS.xcodeproj/project.pbxproj(8 hunks)Wable-iOS.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)Wable-iOS/Core/Combine+/Publisher+.swift(1 hunks)Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/CombineCompatible.swift(1 hunks)Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlEvent.swift(1 hunks)Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlProperty.swift(1 hunks)Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIGesture.swift(1 hunks)Wable-iOS/Presentation/Login/LoginViewController.swift(1 hunks)Wable-iOS/Presentation/Overview/Rank/View/RankListViewController.swift(1 hunks)Wable-iOS/Presentation/Viewit/Create/View/CreateViewitViewController.swift(1 hunks)Wable-iOS/Presentation/Viewit/Create/View/Subview/ViewitInputView.swift(2 hunks)Wable-iOS/Presentation/Viewit/Create/ViewModel/CreateViewitViewModel.swift(0 hunks)
💤 Files with no reviewable changes (1)
- Wable-iOS/Presentation/Viewit/Create/ViewModel/CreateViewitViewModel.swift
🧰 Additional context used
🧬 Code Graph Analysis (4)
Wable-iOS/Presentation/Login/LoginViewController.swift (1)
Wable-iOS/Core/Logger/WableLogger.swift (1)
log(14-25)
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlProperty.swift (2)
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlEvent.swift (5)
receive(48-51)eventHandler(89-91)request(81-81)cancel(84-86)publisher(111-113)Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIGesture.swift (3)
receive(81-88)request(125-125)cancel(128-133)
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlEvent.swift (2)
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlProperty.swift (5)
receive(54-62)eventHandler(99-102)request(91-91)cancel(94-96)publisher(126-131)Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIGesture.swift (3)
receive(81-88)request(125-125)cancel(128-133)
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIGesture.swift (2)
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlEvent.swift (3)
receive(48-51)request(81-81)cancel(84-86)Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlProperty.swift (3)
receive(54-62)request(91-91)cancel(94-96)
🔇 Additional comments (17)
Wable-iOS.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
2-2:Details
✅ Verification successful
LGTM: Package dependency correctly updated
The originHash update correctly reflects the removal of the CombineCocoa package dependency.
Please verify that all CombineCocoa usage has been successfully migrated to the new custom Combine+UIKit implementations:
🏁 Script executed:
#!/bin/bash # Description: Verify complete migration from CombineCocoa to custom publishers # Expected: No remaining CombineCocoa imports or usage echo "=== Checking for remaining CombineCocoa imports ===" rg -i "import.*combinecocoa" --type swift echo "=== Checking for remaining CombineCocoa usage patterns ===" rg "\.tapPublisher|\.textPublisher|\.isEnabledPublisher" --type swift -A 2 echo "=== Verifying custom publisher usage ===" rg "\.publisher\(for:" --type swift -A 2Length of output: 5050
LGTM: CombineCocoa removal validated and custom Combine+UIKit publishers in use
- No remaining
import CombineCocoaor CombineCocoa-specific APIs detected.- Confirmed
.publisher(for:)usage across UI controls in:
- RankListViewController
- ProfileSegmentedHeaderView
- ViewitInputView
- LoginViewController
- Custom Combine+UIKit extension files (Publishers+UIControlEvent, Publishers+UIControlProperty) are present and in use.
All CombineCocoa references have been replaced as intended.
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/CombineCompatible.swift (1)
10-12: LGTM: Clean marker protocol implementationThe empty
CombineCompatibleprotocol serves as an effective marker for enabling custom Combine extensions on UIKit controls. This follows the established pattern used by libraries like RxSwift's reactive extensions.Wable-iOS/Presentation/Overview/Rank/View/RankListViewController.swift (1)
116-116:Details
❓ Verification inconclusive
Migration to custom Combine publisher looks correct
The replacement of
.tapPublisherwith.publisher(for: .touchUpInside)maintains equivalent functionality for button tap events.Please verify that the custom
publisher(for:)implementation handles edge cases consistently with CombineCocoa'stapPublisher:
🏁 Script executed:
#!/bin/bash # Description: Check the custom UIControlEventPublisher implementation # Expected: Proper event handling and memory management echo "=== Checking custom publisher implementation ===" ast-grep --pattern 'extension UIControl { $$$ func publisher(for $EVENT) -> $RETURN_TYPE { $$$ } $$$ }' echo "=== Checking for proper event handling ===" rg "UIControlEventPublisher|UIControlEvent" --type swift -A 5Length of output: 4502
Migration to custom Combine publisher looks correct
The switch from
.tapPublisherto.publisher(for: .touchUpInside)is functionally equivalent, and the custom implementation lives in
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlEvent.swift.Please verify the following to ensure full behavioral parity with CombineCocoa’s
tapPublisher:
UIControlEventSubscriptioncallscontrol.addTarget(_:action:for:)when the subscription starts andcontrol.removeTarget(_:action:for:)incancel()(ordeinit) to avoid leaks.- The subscription holds only a weak reference to
controlto prevent retention cycles.- Events are emitted strictly while the subscription is active and stop immediately upon cancellation.
Wable-iOS/Presentation/Login/LoginViewController.swift (1)
138-138: Consistent migration pattern applied correctlyBoth login buttons have been successfully migrated from
.tapPublisherto.publisher(for: .touchUpInside), maintaining the existing event handling and logging functionality.Also applies to: 144-144
Wable-iOS/Presentation/Viewit/Create/View/Subview/ViewitInputView.swift (4)
8-8: LGTM! Combine import added for reactive publishers.The Combine import is correctly added to support the new reactive publishers in the extension.
102-110: Consistent and well-implemented text field publisher.The publisher follows the same pattern as
urlStringChangedwith appropriate background color changes for the description field context.
112-117:Details
✅ Verification successful
Consistent button tap publisher implementation.
The upload button publisher follows the same pattern as
nextTappedwith appropriate debouncing.Verify that the custom
.publisher(for:)extension is properly implemented for UIButton:
🏁 Script executed:
#!/bin/bash # Description: Verify the custom UIButton publisher extension implementation # Expected: Find the implementation of publisher(for:) for UIButton/UIControl rg -A 10 "func publisher.*UIControl\.Event" --type swiftLength of output: 578
✅ Button Tap Publisher Implementation Verified
The
uploadTappedpublisher correctly mirrors thenextTappedpattern with a 300 ms debounce. Thepublisher(for:)extension forUIControl.Eventis defined in:
- Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlEvent.swift
No further changes required.
85-93:Details
✅ Verification successful
Well-implemented reactive text field publisher.
The publisher correctly handles text changes with proper memory management using
weak self. The background color side effect provides good visual feedback.Verify that the custom
.publisher(for:keyPath:)extension is properly implemented:
🏁 Script executed:
#!/bin/bash # Description: Verify the custom UITextField publisher extension implementation # Expected: Find the implementation of publisher(for:keyPath:) for UITextField rg -A 10 "func publisher.*keyPath" --type swiftLength of output: 49
🏁 Script executed:
#!/bin/bash # Search for any custom publisher(for:keyPath:) implementations in Swift rg -A5 "extension UITextField" --type swift rg -A5 "extension UIControl" --type swift rg -n "publisher(" --type swiftLength of output: 814
🏁 Script executed:
#!/bin/bash # Search for the custom publisher(for:keyPath:) extension across Swift files rg -n "publisher(for" -A5 --type swiftLength of output: 114
🏁 Script executed:
#!/bin/bash # Properly search for any publisher( occurrences, escaping regex chars rg -n "publisher\\(" -A5 --type swift echo "---- Inspect CombineCompatible.swift ----" # Display the first 200 lines of the CombineCompatible.swift to locate publisher extensions sed -n '1,200p' Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/CombineCompatible.swiftLength of output: 9455
Custom publisher extension confirmed
The
publisher(for: keyPath:)API is implemented inPublishers+UIControlProperty.swift, so no further action is needed:
- Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlProperty.swift (lines ~126): defines
func publisher<Value>(for events: UIControl.Event, keyPath: KeyPath<Control, Value?>) -> UIControlPropertyPublisher<Control, Value?>Everything looks correct—go ahead and merge.
Wable-iOS.xcodeproj/project.pbxproj (4)
280-283: LGTM! Clean dependency replacement with proper build configuration.The addition of the four new Combine+UIKit files with proper PBXBuildFile entries ensures they'll be correctly compiled into the project. This architectural change from CombineCocoa to a custom implementation provides better control over the codebase and reduces external dependencies.
620-623: Proper file reference management.The file references are correctly added with appropriate naming conventions and source tree configuration. The consistent naming pattern (
Publishers+UIControlEvent.swift,Publishers+UIControlProperty.swift, etc.) makes the API surface clear and discoverable.
1951-1961: Excellent code organization with dedicated group structure.Creating a dedicated "Combine+UIKit" group under the Extension hierarchy provides clear separation of concerns and makes the custom UIKit integration code easily discoverable. This organization will help with long-term maintainability as the codebase grows.
Also applies to: 1978-1978
2591-2591: All new files properly integrated into the build system.The source files are correctly added to the Sources build phase, ensuring they'll be compiled and available throughout the application. The integration looks complete and follows Xcode project best practices.
Also applies to: 2625-2625, 2765-2765, 2789-2789
Wable-iOS/Presentation/Viewit/Create/View/CreateViewitViewController.swift (1)
130-134: LGTM! Clean refactor to reactive event handling.The refactor from manual event relays to reactive Combine publishers significantly improves the code by:
- Eliminating intermediate relay properties and manual event forwarding
- Creating a more declarative event handling approach
- Reducing boilerplate code
The implementation correctly leverages the new custom publishers for UIKit components.
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlEvent.swift (1)
31-52: LGTM! Well-designed publisher with excellent documentation.The
UIControlEventPublisherimplementation follows Combine best practices with:
- Clear generic constraints and type safety
- Comprehensive documentation with usage examples
- Proper publisher protocol implementation
The design correctly separates concerns between the publisher and subscription objects.
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlProperty.swift (1)
35-63: LGTM! Excellent property publisher design with type safety.The
UIControlPropertyPublisherimplementation provides:
- Type-safe property access via KeyPath
- Clean separation of concerns
- Comprehensive documentation with practical examples
- Proper generic constraints ensuring type safety
The KeyPath-based approach is particularly elegant for accessing control properties reactively.
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIGesture.swift (2)
27-46: LGTM! Well-designed GestureType enum with proper encapsulation.The
GestureTypeenum design is excellent:
- Encapsulates gesture recognizer instances as associated values
- Provides default initializers for convenience
- Clean computed property for accessing the underlying recognizer
- Supports all major UIKit gesture types
This approach allows for easy customization of gesture recognizers while maintaining type safety.
127-133: LGTM! Proper resource cleanup implementation.The
cancel()method correctly handles cleanup by:
- Removing the gesture recognizer from the view
- Clearing the subscriber reference
- Preventing memory leaks
This is the correct pattern that should be followed in the other publisher implementations.
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlEvent.swift
Show resolved
Hide resolved
Wable-iOS/Presentation/Helper/Extension/Combine+UIKit/Publishers+UIControlProperty.swift
Show resolved
Hide resolved
| func withLatestFrom<Other1: Publisher, Other2: Publisher, Result>( | ||
| _ other1: Other1, | ||
| _ other2: Other2, | ||
| _ resultSelector: @escaping (Output, Other1.Output, Other2.Output) -> Result | ||
| ) -> AnyPublisher<Result, Failure> | ||
| where Other1.Failure == Failure, Other2.Failure == Failure { | ||
| self | ||
| .map { ($0, Date.now.timeIntervalSince1970) } | ||
| .combineLatest(other1, other2) | ||
| .removeDuplicates { $0.0.1 == $1.0.1 } | ||
| .map { trigger, value1, value2 in | ||
| resultSelector(trigger.0, value1, value2) | ||
| } | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
Same critical issue: Replace timestamp with UUID for reliable operation.
This final variant also needs the same fix as the others.
Apply this fix:
-func withLatestFrom<Other1: Publisher, Other2: Publisher, Result>(
- _ other1: Other1,
- _ other2: Other2,
- _ resultSelector: @escaping (Output, Other1.Output, Other2.Output) -> Result
-) -> AnyPublisher<Result, Failure>
-where Other1.Failure == Failure, Other2.Failure == Failure {
- self
- .map { ($0, Date.now.timeIntervalSince1970) }
- .combineLatest(other1, other2)
- .removeDuplicates { $0.0.1 == $1.0.1 }
- .map { trigger, value1, value2 in
- resultSelector(trigger.0, value1, value2)
- }
- .eraseToAnyPublisher()
-}
+func withLatestFrom<Other1: Publisher, Other2: Publisher, Result>(
+ _ other1: Other1,
+ _ other2: Other2,
+ _ resultSelector: @escaping (Output, Other1.Output, Other2.Output) -> Result
+) -> AnyPublisher<Result, Failure>
+where Other1.Failure == Failure, Other2.Failure == Failure {
+ self
+ .map { ($0, UUID()) }
+ .combineLatest(other1, other2)
+ .removeDuplicates { $0.0.1 == $1.0.1 }
+ .map { trigger, value1, value2 in
+ resultSelector(trigger.0, value1, value2)
+ }
+ .eraseToAnyPublisher()
+}📝 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 withLatestFrom<Other1: Publisher, Other2: Publisher, Result>( | |
| _ other1: Other1, | |
| _ other2: Other2, | |
| _ resultSelector: @escaping (Output, Other1.Output, Other2.Output) -> Result | |
| ) -> AnyPublisher<Result, Failure> | |
| where Other1.Failure == Failure, Other2.Failure == Failure { | |
| self | |
| .map { ($0, Date.now.timeIntervalSince1970) } | |
| .combineLatest(other1, other2) | |
| .removeDuplicates { $0.0.1 == $1.0.1 } | |
| .map { trigger, value1, value2 in | |
| resultSelector(trigger.0, value1, value2) | |
| } | |
| .eraseToAnyPublisher() | |
| } | |
| func withLatestFrom<Other1: Publisher, Other2: Publisher, Result>( | |
| _ other1: Other1, | |
| _ other2: Other2, | |
| _ resultSelector: @escaping (Output, Other1.Output, Other2.Output) -> Result | |
| ) -> AnyPublisher<Result, Failure> | |
| where Other1.Failure == Failure, Other2.Failure == Failure { | |
| self | |
| .map { ($0, UUID()) } | |
| .combineLatest(other1, other2) | |
| .removeDuplicates { $0.0.1 == $1.0.1 } | |
| .map { trigger, value1, value2 in | |
| resultSelector(trigger.0, value1, value2) | |
| } | |
| .eraseToAnyPublisher() | |
| } |
🤖 Prompt for AI Agents
In Wable-iOS/Core/Combine+/Publisher+.swift between lines 183 and 197, the
function uses a timestamp to track changes, which can cause unreliable behavior.
Replace the timestamp (Date.now.timeIntervalSince1970) with a UUID to ensure
uniqueness and reliable operation. Update the map operator to pair the output
with a UUID instead of a timestamp, and adjust the removeDuplicates comparison
accordingly.
| func withLatestFrom<Other1: Publisher, Other2: Publisher>( | ||
| _ other1: Other1, | ||
| _ other2: Other2 | ||
| ) -> AnyPublisher<(Other1.Output, Other2.Output), Failure> where Other1.Failure == Failure, Other2.Failure == Failure { | ||
| self | ||
| .map { _ in Date.now.timeIntervalSince1970 } | ||
| .combineLatest(other1, other2) | ||
| .removeDuplicates { $0.0 == $1.0 } | ||
| .map { ($0.1, $0.2) } | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
Same critical issue: Replace timestamp with UUID for proper deduplication.
This variant also suffers from the timestamp precision problem.
Apply this fix:
-func withLatestFrom<Other1: Publisher, Other2: Publisher>(
- _ other1: Other1,
- _ other2: Other2
-) -> AnyPublisher<(Other1.Output, Other2.Output), Failure> where Other1.Failure == Failure, Other2.Failure == Failure {
- self
- .map { _ in Date.now.timeIntervalSince1970 }
- .combineLatest(other1, other2)
- .removeDuplicates { $0.0 == $1.0 }
- .map { ($0.1, $0.2) }
- .eraseToAnyPublisher()
-}
+func withLatestFrom<Other1: Publisher, Other2: Publisher>(
+ _ other1: Other1,
+ _ other2: Other2
+) -> AnyPublisher<(Other1.Output, Other2.Output), Failure> where Other1.Failure == Failure, Other2.Failure == Failure {
+ let trigger = self.map { _ in UUID() }
+ return trigger
+ .combineLatest(other1, other2)
+ .removeDuplicates { $0.0 == $1.0 }
+ .map { ($0.1, $0.2) }
+ .eraseToAnyPublisher()
+}📝 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 withLatestFrom<Other1: Publisher, Other2: Publisher>( | |
| _ other1: Other1, | |
| _ other2: Other2 | |
| ) -> AnyPublisher<(Other1.Output, Other2.Output), Failure> where Other1.Failure == Failure, Other2.Failure == Failure { | |
| self | |
| .map { _ in Date.now.timeIntervalSince1970 } | |
| .combineLatest(other1, other2) | |
| .removeDuplicates { $0.0 == $1.0 } | |
| .map { ($0.1, $0.2) } | |
| .eraseToAnyPublisher() | |
| } | |
| func withLatestFrom<Other1: Publisher, Other2: Publisher>( | |
| _ other1: Other1, | |
| _ other2: Other2 | |
| ) -> AnyPublisher<(Other1.Output, Other2.Output), Failure> where Other1.Failure == Failure, Other2.Failure == Failure { | |
| let trigger = self.map { _ in UUID() } | |
| return trigger | |
| .combineLatest(other1, other2) | |
| .removeDuplicates { $0.0 == $1.0 } | |
| .map { ($0.1, $0.2) } | |
| .eraseToAnyPublisher() | |
| } |
🤖 Prompt for AI Agents
In Wable-iOS/Core/Combine+/Publisher+.swift between lines 149 and 159, the
withLatestFrom function uses a timestamp for deduplication which can fail due to
precision issues. Replace the timestamp mapping with a UUID to ensure unique
values for proper deduplication. Update the map operator to emit a new UUID
instead of the current timestamp before combineLatest.
| func withLatestFrom<Other: Publisher>( | ||
| _ other: Other | ||
| ) -> AnyPublisher<Other.Output, Failure> where Other.Failure == Failure { | ||
| self | ||
| .map { _ in Date.now.timeIntervalSince1970 } | ||
| .combineLatest(other) | ||
| .removeDuplicates(by: { $0.0 == $1.0 }) | ||
| .map { $0.1 } | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
Critical issue: Timestamp-based deduplication may fail with rapid emissions.
The timestamp approach using Date.now.timeIntervalSince1970 has a fundamental flaw: if multiple emissions occur within the same timestamp resolution (which can happen with rapid UI events or high-frequency publishers), they will be incorrectly deduplicated, leading to lost events.
Consider using a more robust approach:
-func withLatestFrom<Other: Publisher>(
- _ other: Other
-) -> AnyPublisher<Other.Output, Failure> where Other.Failure == Failure {
- self
- .map { _ in Date.now.timeIntervalSince1970 }
- .combineLatest(other)
- .removeDuplicates(by: { $0.0 == $1.0 })
- .map { $0.1 }
- .eraseToAnyPublisher()
-}
+func withLatestFrom<Other: Publisher>(
+ _ other: Other
+) -> AnyPublisher<Other.Output, Failure> where Other.Failure == Failure {
+ let trigger = self.map { _ in UUID() }
+ return trigger
+ .combineLatest(other)
+ .removeDuplicates(by: { $0.0 == $1.0 })
+ .map { $0.1 }
+ .eraseToAnyPublisher()
+}Alternatively, consider implementing a custom publisher that properly tracks emission sources.
📝 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 withLatestFrom<Other: Publisher>( | |
| _ other: Other | |
| ) -> AnyPublisher<Other.Output, Failure> where Other.Failure == Failure { | |
| self | |
| .map { _ in Date.now.timeIntervalSince1970 } | |
| .combineLatest(other) | |
| .removeDuplicates(by: { $0.0 == $1.0 }) | |
| .map { $0.1 } | |
| .eraseToAnyPublisher() | |
| } | |
| func withLatestFrom<Other: Publisher>( | |
| _ other: Other | |
| ) -> AnyPublisher<Other.Output, Failure> where Other.Failure == Failure { | |
| let trigger = self.map { _ in UUID() } | |
| return trigger | |
| .combineLatest(other) | |
| .removeDuplicates(by: { $0.0 == $1.0 }) | |
| .map { $0.1 } | |
| .eraseToAnyPublisher() | |
| } |
🤖 Prompt for AI Agents
In Wable-iOS/Core/Combine+/Publisher+.swift around lines 86 to 95, the current
withLatestFrom implementation uses timestamps for deduplication, which can fail
when multiple events occur within the same timestamp resolution, causing lost
events. To fix this, replace the timestamp-based deduplication with a method
that tracks emissions more reliably, such as using a sequence number or a custom
publisher that explicitly tracks and combines the latest values from both
publishers without relying on timestamps.
| func withLatestFrom<Other: Publisher, Result>( | ||
| _ other: Other, | ||
| _ resultSelector: @escaping (Output, Other.Output) -> Result | ||
| ) -> AnyPublisher<Result, Failure> where Other.Failure == Failure { | ||
| self | ||
| .map { ($0, Date.now.timeIntervalSince1970) } | ||
| .combineLatest(other) | ||
| .removeDuplicates { $0.0.1 == $1.0.1 } | ||
| .map { lhs, rhs in resultSelector(lhs.0, rhs) } | ||
| .eraseToAnyPublisher() | ||
| } |
There was a problem hiding this comment.
Same critical issue: Use UUID instead of timestamp for reliable deduplication.
This variant has the same timestamp precision issue as the first variant.
Apply this fix:
-func withLatestFrom<Other: Publisher, Result>(
- _ other: Other,
- _ resultSelector: @escaping (Output, Other.Output) -> Result
-) -> AnyPublisher<Result, Failure> where Other.Failure == Failure {
- self
- .map { ($0, Date.now.timeIntervalSince1970) }
- .combineLatest(other)
- .removeDuplicates { $0.0.1 == $1.0.1 }
- .map { lhs, rhs in resultSelector(lhs.0, rhs) }
- .eraseToAnyPublisher()
-}
+func withLatestFrom<Other: Publisher, Result>(
+ _ other: Other,
+ _ resultSelector: @escaping (Output, Other.Output) -> Result
+) -> AnyPublisher<Result, Failure> where Other.Failure == Failure {
+ self
+ .map { ($0, UUID()) }
+ .combineLatest(other)
+ .removeDuplicates { $0.0.1 == $1.0.1 }
+ .map { lhs, rhs in resultSelector(lhs.0, rhs) }
+ .eraseToAnyPublisher()
+}📝 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 withLatestFrom<Other: Publisher, Result>( | |
| _ other: Other, | |
| _ resultSelector: @escaping (Output, Other.Output) -> Result | |
| ) -> AnyPublisher<Result, Failure> where Other.Failure == Failure { | |
| self | |
| .map { ($0, Date.now.timeIntervalSince1970) } | |
| .combineLatest(other) | |
| .removeDuplicates { $0.0.1 == $1.0.1 } | |
| .map { lhs, rhs in resultSelector(lhs.0, rhs) } | |
| .eraseToAnyPublisher() | |
| } | |
| func withLatestFrom<Other: Publisher, Result>( | |
| _ other: Other, | |
| _ resultSelector: @escaping (Output, Other.Output) -> Result | |
| ) -> AnyPublisher<Result, Failure> where Other.Failure == Failure { | |
| self | |
| .map { ($0, UUID()) } | |
| .combineLatest(other) | |
| .removeDuplicates { $0.0.1 == $1.0.1 } | |
| .map { lhs, rhs in resultSelector(lhs.0, rhs) } | |
| .eraseToAnyPublisher() | |
| } |
🤖 Prompt for AI Agents
In Wable-iOS/Core/Combine+/Publisher+.swift around lines 118 to 128, the current
implementation uses timestamps for deduplication, which can cause reliability
issues due to limited precision. Replace the timestamp with a UUID to ensure
each event is uniquely identified. Update the map operator to pair the output
with a UUID instead of the timestamp, and keep the rest of the logic unchanged
to maintain deduplication based on the UUID.
- 그 외 변수명 수정
youz2me
left a comment
youz2me
left a comment
There was a problem hiding this comment.
고생하셨습니다!! 직접 설명 들으면서 qna한 부분도 있어서 그 부분은 따로 남기지 않았습니다 ㅎ ㅎ
추가로 궁금한 부분만 리뷰 남겨두었어요! 확인해주시면 좋을 것 같습니닷.
구현해주신 코드 잘 가져다 쓰도록 하겠습니다 ,, 굿.
| extension ViewitInputView { | ||
| var urlStringChanged: AnyPublisher<String, Never> { | ||
| urlTextField | ||
| .publisher(for: .editingChanged, keyPath: \.text) | ||
| .compactMap { $0 } | ||
| .handleEvents(receiveOutput: { [weak self] text in | ||
| self?.urlTextField.backgroundColor = text.isEmpty ? .gray100 : .blue10 | ||
| }) | ||
| .eraseToAnyPublisher() | ||
| } | ||
|
|
||
| var nextTapped: AnyPublisher<Void, Never> { | ||
| nextButton | ||
| .publisher(for: .touchUpInside) | ||
| .debounce(for: .milliseconds(300), scheduler: DispatchQueue.main) | ||
| .eraseToAnyPublisher() | ||
| } | ||
|
|
||
| var descriptionChanged: AnyPublisher<String, Never> { | ||
| descriptionTextField | ||
| .publisher(for: .editingChanged, keyPath: \.text) | ||
| .compactMap { $0 } | ||
| .handleEvents(receiveOutput: { [weak self] text in | ||
| self?.descriptionTextField.backgroundColor = text.isEmpty ? .gray100 : .wableWhite | ||
| }) | ||
| .eraseToAnyPublisher() | ||
| } | ||
|
|
||
| var uploadTapped: AnyPublisher<Void, Never> { | ||
| uploadButton | ||
| .publisher(for: .touchUpInside) | ||
| .debounce(for: .milliseconds(300), scheduler: DispatchQueue.main) | ||
| .eraseToAnyPublisher() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
View에서 Combine을 임포트해서 이런 액션을 정의하는 게 맞을까라는 의문이 들어요. 개인적으로는 뷰컨에 있는 게 맞는 것 같아서 의견이 궁금합니닷.
There was a problem hiding this comment.
또 듣다 보니, 그게 더 맞을지도 모르겠네요.
일단 뷰컨의 setupBinding에서 코드를 작성하여 반영해 보겠습니다.
| urlStringChanged: viewitInputView.urlStringChanged, | ||
| next: viewitInputView.nextTapped, | ||
| descriptionChanged: viewitInputView.descriptionChanged, | ||
| upload: viewitInputView.uploadTapped, | ||
| backgroundTap: dimmedBackgroundView.gesture().asVoid() |
| func setupAction() { | ||
| let tapGesture = UITapGestureRecognizer(target: self, action: #selector(backgroundViewDidTap)) | ||
| dimmedBackgroundView.addGestureRecognizer(tapGesture) | ||
|
|
||
| viewitInputView.urlTextField.addTarget(self, action: #selector(urlTextFieldDidChange(_:)), for: .editingChanged) | ||
| viewitInputView.nextButton.addTarget(self, action: #selector(nextButtonDidTap), for: .touchUpInside) | ||
| viewitInputView.descriptionTextField.addTarget( | ||
| self, | ||
| action: #selector(descriptionTextFieldDidChange(_:)), | ||
| for: .editingChanged | ||
| ) | ||
| viewitInputView.uploadButton.addTarget(self, action: #selector(uploadButtonDidTap), for: .touchUpInside) | ||
| } | ||
|
|
…o refactor/#199-combine+-uikit-withlatestfrom
…tfrom [Refactor] Combine+UIKit, withLatestFrom 구현하기
👻 PULL REQUEST
📄 작업 내용
withLatestFrom을 비슷하게 Combine의 오퍼레이터로 구현하였습니다.💻 주요 코드 설명
withLatestFrom비슷하게 구현하기1. 기본 형태
2. 클로저 형태
3. 두 개의 참조 퍼블리셔와 함께 사용하는 경우
왜 Date.now.timeIntervalSince1970을 사용하는가?
Combine의 combineLatest는 두 퍼블리셔 중 어느 하나라도 새로운 값을 방출하면 호출됩니다.
하지만 self의 이벤트만을 트리거로 사용하기 위해, 다음 방식으로 해결합니다:
Date.now.timeIntervalSince1970로 타임스탬프를 부여합니다..removeDuplicates { $0.0 == $1.0 }를 통해 같은 타임스탬프로 인한 중복 호출을 방지합니다.📚 참고자료
스택오버플로우
🔗 연결된 이슈
Summary by CodeRabbit
New Features
withLatestFromoperators to Combine publishers for enhanced reactive stream composition.Refactor
Bug Fixes
Chores