Add presence to RealtimeChannelProtocol
#212
Conversation
I'm going to revert ably-cocoa's commit 26d9bf7, which added the `presence` member to ARTRealtimeChannelProtocol, since it broke backwards compatibility.
WalkthroughThis pull request introduces a series of modifications across multiple files to enhance type flexibility by changing the Changes
Sequence DiagramsequenceDiagram
participant Channel
participant Presence
Channel->>Presence: Initialize presence
Presence-->>Channel: Presence instance created
Channel->>Presence: Perform presence operations
Presence-->>Channel: Handle presence actions
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 2
🧹 Nitpick comments (3)
Example/AblyChatExample/Mocks/MockRealtime.swift (2)
108-108: Provide mock implementations forpresenceproperty to prevent runtime crashes during testing.Currently, the
presenceproperty inChannelis instantiated withRealtimePresence(), whose methods raisefatalError("Not implemented"). This could lead to unexpected crashes during testing if any presence methods are called. Consider providing mock implementations or stubs for these methods to enhance test stability.
251-345: Implement default or mocked methods inRealtimePresenceto improve test reliability.All methods in
RealtimePresencecurrently usefatalError("Not implemented"), which will cause a crash if these methods are invoked during testing. Providing minimal implementations or mock responses can prevent runtime exceptions and allow tests to proceed smoothly.Sources/AblyChat/Dependencies.swift (1)
32-33: Consider adding documentation for protocol requirements.The protocol lacks documentation explaining the expected behavior and requirements for conforming types. Consider adding documentation similar to other protocols in the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
Example/AblyChatExample/Mocks/MockClients.swift(4 hunks)Example/AblyChatExample/Mocks/MockRealtime.swift(2 hunks)Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift(1 hunks)Sources/AblyChat/Dependencies.swift(1 hunks)Sources/AblyChat/Messages.swift(1 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Sources/AblyChat/Room.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(2 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/Typing.swift(1 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check
Sources/AblyChat/Dependencies.swift
[error] 27-27: unable to infer associated type 'Presence' for protocol 'RealtimeChannelProtocol'
Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift
[error] 7-7: type 'ARTRealtimeChannel' does not conform to protocol 'RealtimeChannelProtocol'
🔇 Additional comments (11)
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (2)
5-5: Use ofany RealtimeChannelProtocolenhances flexibility.Changing the type of
channelto the existential typeany RealtimeChannelProtocolincreases the flexibility of the mock and aligns with similar changes across the codebase.
10-13: Initializer update is consistent with the type change.Updating the initializer parameter to accept
channel: any RealtimeChannelProtocolensures consistency with the property's type change and allows for a wider range of protocol conforming types to be used.Sources/AblyChat/Dependencies.swift (1)
26-30:⚠️ Potential issueVerify alignment with PR objectives.
The PR objectives mention reverting the addition of the
presencemember, but these changes are adding it. Please clarify if this is intentional or if the PR objectives need to be updated.Additionally, there's a type inference error. Consider providing a default associated type to resolve the inference issue:
public protocol RealtimeChannelProtocol: ARTRealtimeChannelProtocol, Sendable { - associatedtype Presence: RealtimePresenceProtocol + associatedtype Presence: RealtimePresenceProtocol = Self.DefaultPresence + typealias DefaultPresence = ARTRealtimePresence // or your default type var presence: Presence { get } }🧰 Tools
🪛 GitHub Actions: Check
[error] 27-27: unable to infer associated type 'Presence' for protocol 'RealtimeChannelProtocol'
Sources/AblyChat/Typing.swift (1)
57-57: LGTM! Good use of existential type.The change to use
any RealtimeChannelProtocolaligns with modern Swift practices and provides more implementation flexibility.Sources/AblyChat/RoomReactions.swift (1)
25-25: LGTM! Consistent use of existential type.The change to use
any RealtimeChannelProtocolis consistent with other files and maintains good documentation.Sources/AblyChat/RoomFeature.swift (1)
49-49: LGTM! Explicit existential type usage improves type system clarity.The change from
RealtimeChannelProtocoltoany RealtimeChannelProtocolaligns with Swift's move towards explicit existential types, making it clearer when we're working with type-erased wrappers.Also applies to: 65-65
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
5-5: LGTM! Improved testability with concrete mock implementation.Replacing the computed property that raised
fatalErrorwith a concreteMockRealtimePresenceinstance improves testability and aligns with the PR's objective of handling presence inRealtimeChannelProtocol.Sources/AblyChat/Messages.swift (1)
54-54: LGTM! Consistent use of explicit existential type.The change to
any RealtimeChannelProtocolmaintains consistency with similar changes across the codebase and improves type system clarity.Example/AblyChatExample/Mocks/MockClients.swift (1)
91-91: LGTM! Systematic update to explicit existential types.The consistent change from
RealtimeChannelProtocoltoany RealtimeChannelProtocolacross all mock implementations (MockMessages,MockRoomReactions,MockTyping,MockOccupancy) maintains codebase consistency and type system clarity.Also applies to: 149-149, 196-196, 359-359
Sources/AblyChat/Room.swift (1)
303-303: LGTM! Good use of explicit existential type.The change to
any RealtimeChannelProtocolfollows Swift's best practices for protocol conformance and improves type safety by making the existential nature of the type explicit.Sources/AblyChat/Occupancy.swift (1)
37-37: LGTM! Consistent use of existential type across the codebase.The change to
any RealtimeChannelProtocolmaintains API compatibility while improving type safety, and aligns with similar changes across the codebase.
| @@ -6,4 +6,6 @@ extension ARTRealtimeChannels: RealtimeChannelsProtocol {} | |||
|
|
|||
| extension ARTRealtimeChannel: RealtimeChannelProtocol {} | |||
There was a problem hiding this comment.
Fix protocol conformance for ARTRealtimeChannel.
The pipeline is failing with the error: "type 'ARTRealtimeChannel' does not conform to protocol 'RealtimeChannelProtocol'". This is likely because ARTRealtimeChannel is missing required members of RealtimeChannelProtocol, specifically the presence property of type any RealtimePresenceProtocol. To resolve this, you need to ensure that ARTRealtimeChannel provides an implementation for the presence property that conforms to RealtimePresenceProtocol.
Apply this diff to fix the issue:
extension ARTRealtimeChannel: RealtimeChannelProtocol {
+ public var presence: any RealtimePresenceProtocol {
+ return self.presence as any RealtimePresenceProtocol
+ }
}Ensure that self.presence correctly returns an instance conforming to RealtimePresenceProtocol. This should satisfy the protocol requirements and fix the pipeline failure.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: Check
[error] 7-7: type 'ARTRealtimeChannel' does not conform to protocol 'RealtimeChannelProtocol'
| final class MockRealtimePresence: RealtimePresenceProtocol { | ||
| var syncComplete: Bool { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func get(_: @escaping ARTPresenceMessagesCallback) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func get(_: ARTRealtimePresenceQuery, callback _: @escaping ARTPresenceMessagesCallback) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func enter(_: Any?) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func enter(_: Any?, callback _: ARTCallback? = nil) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func update(_: Any?) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func update(_: Any?, callback _: ARTCallback? = nil) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func leave(_: Any?) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func leave(_: Any?, callback _: ARTCallback? = nil) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func enterClient(_: String, data _: Any?) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func enterClient(_: String, data _: Any?, callback _: ARTCallback? = nil) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func updateClient(_: String, data _: Any?) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func updateClient(_: String, data _: Any?, callback _: ARTCallback? = nil) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func leaveClient(_: String, data _: Any?) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func leaveClient(_: String, data _: Any?, callback _: ARTCallback? = nil) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func subscribe(_: @escaping ARTPresenceMessageCallback) -> ARTEventListener? { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func subscribe(attachCallback _: ARTCallback?, callback _: @escaping ARTPresenceMessageCallback) -> ARTEventListener? { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func subscribe(_: ARTPresenceAction, callback _: @escaping ARTPresenceMessageCallback) -> ARTEventListener? { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func subscribe(_: ARTPresenceAction, onAttach _: ARTCallback?, callback _: @escaping ARTPresenceMessageCallback) -> ARTEventListener? { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func unsubscribe() { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func unsubscribe(_: ARTEventListener) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func unsubscribe(_: ARTPresenceAction, listener _: ARTEventListener) { | ||
| fatalError("Not implemented") | ||
| } | ||
|
|
||
| func history(_: @escaping ARTPaginatedPresenceCallback) {} | ||
|
|
||
| func history(_: ARTRealtimeHistoryQuery?, callback _: @escaping ARTPaginatedPresenceCallback) throws { | ||
| fatalError("Not implemented") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve mock implementation consistency and documentation.
Issues found:
- The
history(_:)method has an empty implementation while all other methods throwfatalError. This inconsistency could lead to confusion. - Missing documentation explaining the mock's intended use and which methods are expected to be implemented.
- Consider using a more descriptive error message in
fatalErrorcalls.
Consider these improvements:
- Add documentation:
/// A mock implementation of RealtimePresenceProtocol for testing purposes.
/// This mock currently throws fatalError for all methods as they are not yet implemented.
final class MockRealtimePresence: RealtimePresenceProtocol {- Make implementation consistent:
- func history(_: @escaping ARTPaginatedPresenceCallback) {}
+ func history(_: @escaping ARTPaginatedPresenceCallback) {
+ fatalError("Not implemented")
+ }- Add more descriptive error messages:
- fatalError("Not implemented")
+ fatalError("MockRealtimePresence.syncComplete is not implemented")
I'm going to revert ably-cocoa's commit 26d9bf7, which added the
presencemember toARTRealtimeChannelProtocol, since it broke backwards compatibility.Summary by CodeRabbit
New Features
RealtimePresenceProtocolto enhance presence-related operationsRefactor
Chores