[ECO-5040] Channel presence now is exposed publicly as protocol #1995
Conversation
…cess `internalPresence` (in internal class) exists, or `presence`, but manually casted to internal when needed. In other words, public channel exposes public `presence`, internal one exposes internal `presence` (but only public properties/methods are visible by default which of `ARTRealtimeChannelProtocol`). `internalPresence` is for explicit and more convenient way of using internal type.
WalkthroughThe pull request introduces significant changes to the Changes
Assessment against linked issues
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: 4
🧹 Outside diff range and nitpick comments (22)
Source/ARTRealtimeChannel.m (1)
712-712: Check error handling consistency infailPendingPresenceWithState:info:In the method
failPendingPresenceWithState:info:, ensure that passing thestatusobject toself.internalPresencehandles errors consistently throughout the codebase.Consider centralizing error handling logic to reduce code duplication and improve maintainability.
Test/Tests/RealtimeClientPresenceTests.swift (19)
168-169: Verify the presence sync state before injecting the leave message.To ensure the test is accurate, check that the presence sync is in progress before injecting the fabricated leave message. This will confirm that the member is correctly marked as
.absentdue to the sync, and not due to other factors.Add an assertion to verify
syncInProgressistruebefore callingtransport.receive(leaveMessage):XCTAssertTrue(channel.internal.internalPresence.syncInProgress) transport.receive(leaveMessage)
245-246: Verify the presence sync state before checking the members.To ensure the test is accurate, check that the presence sync has completed before asserting the expected member state. This will confirm the member state after the sync process.
Add an expectation for
syncInProgressto befalsebefore checking themembers:expect(channel.internal.internalPresence.syncInProgress).toEventually(beFalse(), timeout: testTimeout) expect(channel.internal.internalPresence.members).to(beEmpty())
309-315: Verify the injected member is correctly handled.The test injects an internal member directly into the presence map. To ensure this simulated scenario works as expected, verify the injected member is present in the
membersandinternalMemberscollections.Add assertions to check the injected member's presence:
XCTAssertTrue(channel.internal.internalPresence.members.contains { $0.value.clientId == "internal-member" }) XCTAssertTrue(channel.internal.internalPresence.internalMembers.contains { $0.clientId == "internal-member" })
355-356: Verify the injected members are correctly handled.The test injects local members directly into the presence map. To ensure this simulated scenario works as expected, verify the injected members are present in the
memberscollection before the test continues.Add assertions to check the injected members' presence:
XCTAssertTrue(channel.internal.internalPresence.members.contains { $0.value.clientId == "tester1" }) XCTAssertTrue(channel.internal.internalPresence.members.contains { $0.value.clientId == "tester2" })
548-549: Verify the injected member is correctly handled.The test injects a member directly into the presence map. To ensure this simulated scenario works as expected, verify the injected member is present in the
memberscollection before the test continues.Add an assertion to check the injected member's presence:
XCTAssertTrue(channel.internal.internalPresence.members.contains { $0.value.clientId == "user" })
609-610: Verify the injected member is correctly handled.The test injects a member directly into the presence map. To ensure this simulated scenario works as expected, verify the injected member is present in the
membersandinternalMemberscollections before the test continues.Add assertions to check the injected member's presence:
XCTAssertTrue(channel.internal.internalPresence.members.contains { $0.value.clientId == "user" }) XCTAssertTrue(channel.internal.internalPresence.internalMembers.contains { $0.clientId == "user" })
793-794: Verify the presence map state while the channel is suspended.It's a good practice to confirm the expected state of the presence map while the channel is in the suspended state. This helps validate that the map is preserved correctly during the suspension.
Add assertions to verify the presence map content:
XCTAssertEqual(_internal.internalPresence.members.count, 4) XCTAssertEqual(_internal.internalPresence.internalMembers.count, 1) XCTAssertTrue(_internal.internalPresence.members.contains { $0.value.clientId == "main" }) XCTAssertTrue(_internal.internalPresence.members.contains { $0.value.clientId == "leaves" })
836-837: Verify the final state of the presence map.After the simulated reconnection and SYNC process, it's important to confirm the final state of the presence map. This helps validate that the map has been correctly updated to reflect the current members.
Add assertions to verify the final presence map content:
XCTAssertEqual(_internal.internalPresence.members.count, 3) XCTAssertEqual(_internal.internalPresence.internalMembers.count, 1) XCTAssertTrue(_internal.internalPresence.members.contains { $0.value.clientId == "main" }) XCTAssertFalse(_internal.internalPresence.members.contains { $0.value.clientId == "leaves" })
1322-1323: Verify initial presence map state.To ensure the test starts with a clean slate, assert that the presence map is empty before performing any actions. This helps validate that any subsequent changes are due to the test scenario.
Add an assertion to check the initial presence map state:
XCTAssertTrue(channel.internal.internalPresence.members.isEmpty)
Line range hint
1425-1436: Verify presence map state after entering and leaving.To validate the behavior of entering and leaving presence, assert the expected state of the presence map after each action. This helps confirm that the map is correctly updated in response to presence changes.
Add assertions to check the presence map state:
// After entering expect(channel.internal.internalPresence.members.count).toEventually(equal(1), timeout: testTimeout) expect(channel.internal.internalPresence.members.contains { $0.value.clientId == "john" }).toEventually(beTrue(), timeout: testTimeout) // After leaving expect(channel.internal.internalPresence.members.count).toEventually(equal(0), timeout: testTimeout) expect(channel.internal.internalPresence.members.contains { $0.value.clientId == "john" }).toEventually(beFalse(), timeout: testTimeout)
Line range hint
1523-1548: Verify the compareForNewness method is called with the correct arguments.To ensure the
compareForNewnessmethod is being used as expected, capture the arguments passed to it and assert they match the initial and updated presence messages. This helps validate the correct behavior of the presence map update process.Modify the
compareForNewnesshook to capture the arguments:var compareForNewnessArgs: (ARTPresenceMessage, ARTPresenceMessage)? let hook = channel.internal.internalPresence.testSuite_injectIntoMethod(after: #selector(ARTRealtimePresenceInternal.member(_:isNewerThan:))) { args in compareForNewnessArgs = (args[0] as! ARTPresenceMessage, args[1] as! ARTPresenceMessage) }Then, add assertions to check the captured arguments:
XCTAssertEqual(compareForNewnessArgs?.0, intialPresenceMessage) XCTAssertEqual(compareForNewnessArgs?.1, updatedPresenceMessage)
1751-1755: Verify the presence map state during the SYNC process.To validate the behavior of the presence map during the SYNC process, assert the expected state of the map while the SYNC is in progress. This helps confirm that the map correctly reflects the incoming presence messages.
Add assertions to check the presence map state during SYNC:
XCTAssertTrue(channel.internal.internalPresence.syncInProgress) XCTAssertEqual(channel.internal.internalPresence.members.count, 20) XCTAssertTrue(channel.internal.internalPresence.members.contains { $0.value.clientId == "user10" }) XCTAssertFalse(channel.internal.internalPresence.members.contains { $0.value.clientId == "user12" })
1790-1792: Verify presence map state after receiving an UPDATE action.To validate the handling of UPDATE actions, assert the expected state of the presence map after processing the UPDATE message. This helps confirm that the map correctly reflects the updated member state.
Add assertions to check the presence map state after the UPDATE:
XCTAssertEqual(channel.internal.internalPresence.members.count, 1) XCTAssertTrue(channel.internal.internalPresence.members.contains { $0.value.clientId == "tester" }) XCTAssertTrue(channel.internal.internalPresence.members.contains { $0.value.action == .present }) XCTAssertFalse(channel.internal.internalPresence.members.contains { $0.value.action == .update })
1810-1811: Verify the presence map state during the SYNC process.To validate the behavior of the presence map during the SYNC process, assert the expected state of the map while the SYNC is in progress. This helps confirm that the map correctly reflects the incoming presence messages.
Add assertions to check the presence map state during SYNC:
XCTAssertTrue(channel.internal.internalPresence.syncInProgress) XCTAssertEqual(channel.internal.internalPresence.members.count, 1) XCTAssertTrue(channel.internal.internalPresence.members.contains { $0.value.action == .present })
Line range hint
1849-1865: Verify presence map state after processing the LEAVE action.To validate the handling of LEAVE actions, assert the expected state of the presence map after processing the LEAVE message. This helps confirm that the map correctly reflects the removal of the member.
Add assertions to check the presence map state after the LEAVE:
expect(channel.internal.internalPresence.syncInProgress).toEventually(beFalse(), timeout: testTimeout) let user11MemberKey = "\(clientMembers?.connection.id ?? ""):user11" expect(channel.internal.internalPresence.members.contains { $0.key == user11MemberKey }).toEventually(beFalse(), timeout: testTimeout)
Line range hint
1889-1896: Verify the presence map state during the SYNC process.To validate the behavior of the presence map during the SYNC process, assert the expected state of the map while the SYNC is in progress. This helps confirm that the map correctly reflects the incoming presence messages.
Add assertions to check the presence map state during SYNC:
XCTAssertTrue(channel.internal.internalPresence.syncInProgress) XCTAssertEqual(channel.internal.internalPresence.members.count, 20) XCTAssertFalse(channel.internal.internalPresence.members.contains { $0.value.clientId == "user11" && $0.value.action == .leave })
1923-1928: Verify the final presence map state.To validate the end result of the test scenario, assert the expected state of the presence map after the SYNC process has completed. This helps confirm that the map correctly reflects the current members.
Add assertions to check the final presence map state:
expect(channel.internal.internalPresence.syncInProgress).toEventually(beFalse(), timeout: testTimeout) expect(channel.internal.internalPresence.members.count).toEventually(equal(20), timeout: testTimeout) expect(channel.internal.internalPresence.members.contains { $0.value.clientId == "user11" && $0.value.action == .leave }).toEventually(beFalse(), timeout: testTimeout) expect(channel.internal.internalPresence.members.contains { $0.value.clientId == "user11" && $0.value.action == .present }).toEventually(beTrue(), timeout: testTimeout)
1952-1953: Verify the presence map state after processing the ENTER action.To validate the handling of ENTER actions, assert the expected state of the presence map after processing the ENTER message. This helps confirm that the map correctly reflects the addition of the new member.
Add assertions to check the presence map state after the ENTER:
XCTAssertEqual(_internal.internalPresence.members.count, 1) XCTAssertTrue(_internal.internalPresence.members.contains { $0.value.clientId == "tester" && $0.value.action == .present }) XCTAssertFalse(_internal.internalPresence.members.contains { $0.value.clientId == "tester" && $0.value.action == .enter })
Line range hint
2495-2507: Verify the presence map state after processing presence messages.To validate the handling of presence messages, assert the expected state of the presence map after processing each message. This helps confirm that the map correctly reflects the addition and removal of members.
Add assertions to check the presence map state:
// After client A enters XCTAssertEqual(channelA.internal.internalPresence.members.count, 1) XCTAssertEqual(channelA.internal.internalPresence.internalMembers.count, 1) XCTAssertTrue(channelA.internal.internalPresence.members.contains { $0.value.clientId == "a" && $0.value.action == .present }) // After client B enters XCTAssertEqual(channelB.internal.internalPresence.members.count, 1) XCTAssertEqual(channelB.internal.internalPresence.internalMembers.count, 0) XCTAssertTrue(channelB.internal.internalPresence.members.contains { $0.value.clientId == "a" && $0.value.action == .present })Test/Tests/RealtimeClientChannelTests.swift (2)
215-217: Refactor tests to use public presence APIDirectly accessing
internalPresencein assertions can make tests brittle. Using the publicpresenceAPI ensures tests are robust and align with the intended usage of the API.
219-222: Avoid coupling tests to internal implementation detailsConsider modifying the tests to interact with presence via the public
presenceAPI instead ofinternalPresence, enhancing test reliability against internal changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
Source/ARTRealtimeChannel.m(10 hunks)Source/ARTRealtimeChannels.m(1 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(1 hunks)Source/include/Ably/ARTRealtimeChannel.h(1 hunks)Source/include/Ably/ARTRealtimePresence.h(0 hunks)Test/Tests/RealtimeClientChannelTests.swift(2 hunks)Test/Tests/RealtimeClientPresenceTests.swift(51 hunks)Test/Tests/RestClientPresenceTests.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Source/include/Ably/ARTRealtimePresence.h
🔇 Additional comments (11)
Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (1)
22-22: LGTM! Property rename improves API clarity.
The rename from presence to internalPresence better reflects its internal nature and aligns with the objective of separating public and internal presence interfaces.
Let's verify that all references to this property have been updated:
✅ Verification successful
The previous search showed some matches but we need to verify:
- If these are related to our specific property or other presence-related properties
- If there are any references specifically in ARTRealtimeChannel implementation
- If there are any other property declarations we should be aware of
Property rename verified, implementation is consistent
The verification shows that the rename from presence to internalPresence has been properly implemented:
- The private property is correctly renamed to
internalPresencein the header - The public API still exposes
presenceproperty that returns the internal presence wrapped appropriately - All references found are either to other presence-related properties or to the public API, not to the renamed private property
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old property name that might have been missed
# Test: Search for potential missed renames. Expect: No instances of old property access patterns
# Look for potential missed renames in implementation files
rg -g '*.m' -g '*.mm' '\b(presence\s*=|\.presence\b|->presence\b)'
# Look for potential missed renames in header files
rg -g '*.h' '\b(presence\s*=|\.presence\b|->presence\b)'
Length of output: 1259
Script:
#!/bin/bash
# Let's search for the old property name in a more targeted way
# 1. Look for the old property name in class implementations
ast-grep --pattern 'self.presence'
# 2. Look for property declarations with 'presence' name
ast-grep --pattern '@property $$ presence'
# 3. Look specifically in ARTRealtimeChannel implementation
rg -g '*ARTRealtimeChannel*' 'presence' -A 2 -B 2
Length of output: 2275
Source/ARTRealtimeChannels.m (2)
119-119: LGTM: Presence interface change aligns with PR objectives.
The change from presence to internalPresence correctly implements the separation of public and internal presence interfaces, which is the main goal of this PR.
119-119: Verify consistent usage of internalPresence across the codebase.
Let's ensure this renaming is consistently applied across all files.
✅ Verification successful
Renaming to internalPresence is correctly implemented
The codebase analysis confirms that the renaming to internalPresence is consistently implemented:
- The property is correctly declared as
internalPresenceinARTRealtimeChannelInternal - The
_unsubscribemethod is called oninternalPresenceinARTRealtimeChannels.m - All internal references use
internalPresenceconsistently - The public API still exposes
presenceproperty which internally usesinternalPresence, maintaining backward compatibility
The change is verified to be consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining uses of the old presence property in cleanup/unsubscribe scenarios
# Test 1: Look for any remaining direct presence unsubscribe calls
echo "Checking for old presence unsubscribe patterns..."
rg -l "presence\s+_unsubscribe" --type objc --type h
# Test 2: Look for the new pattern to confirm consistent usage
echo "Checking for new internalPresence pattern..."
rg -l "internalPresence\s+_unsubscribe" --type objc --type h
# Test 3: Look for potential missed renames in similar contexts
echo "Checking for other presence cleanup patterns..."
rg "presence\.(cleanup|detach|_detach|_unsubscribe)" --type objc --type h
Length of output: 535
Script:
#!/bin/bash
# Let's do a more comprehensive check for presence-related patterns
# Check for any presence property declarations and usages
echo "Checking for presence property declarations and usages..."
rg "presence" --type objc --type h -B 2 -A 2
# Look specifically for internalPresence patterns
echo "Checking for internalPresence patterns..."
rg "internalPresence" --type objc --type h -B 2 -A 2
# Check for property declarations using AST
echo "Checking for presence-related property declarations..."
ast-grep --pattern '@property $_ presence;'
ast-grep --pattern '@property $_ internalPresence;'
Length of output: 54117
Source/include/Ably/ARTRealtimeChannel.h (1)
41-44: LGTM! The protocol property addition looks good.
The new read-only presence property with id<ARTRealtimePresenceProtocol> return type is well-documented and correctly implements the protocol-based abstraction for channel presence.
Let's verify that all implementers of ARTRealtimeChannelProtocol properly implement this new property:
✅ Verification successful
Protocol implementation verified successfully
Based on the search results, the implementation is correct and consistent:
- The protocol
ARTRealtimeChannelProtocolis properly defined with the presence property - Two concrete implementations were found:
ARTRealtimeChannelclass implements the protocol and exposes the presence propertyARTRealtimeChannelInternalclass also implements the protocol with an internal presence implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all classes that implement ARTRealtimeChannelProtocol
# and verify they implement the presence property
# Search for protocol implementation
ast-grep --pattern 'interface $CLASS : $_ <ARTRealtimeChannelProtocol'
# Search for presence property implementation
ast-grep --pattern '@property (readonly) id<ARTRealtimePresenceProtocol> presence;'
Length of output: 158
Script:
#!/bin/bash
# Let's try different approaches to find implementations
# Search for ARTRealtimeChannelProtocol using ripgrep
echo "=== Protocol implementations ==="
rg "ARTRealtimeChannelProtocol" -A 2 -B 2
# Search for presence property declarations
echo -e "\n=== Presence property declarations ==="
rg "@property.*presence" -A 1 -B 1
# Search for interface declarations in header files
echo -e "\n=== Interface declarations ==="
fd -e h -x rg "^@interface.*ARTRealtimeChannel.*$" {} -A 2
Length of output: 9842
Test/Tests/RestClientPresenceTests.swift (1)
100-100: LGTM! Change aligns with new presence protocol design.
The update from presence to internalPresence correctly reflects the architectural changes introduced in this PR, where internal presence is now accessed through a dedicated method.
Let's verify if similar changes are needed elsewhere in the test file:
✅ Verification successful
Change from presence to internalPresence is consistently applied across test files
Based on the comprehensive search results, all presence-related test assertions are already using internal.internalPresence consistently throughout the codebase, including both RestClientPresenceTests.swift and RealtimeClientPresenceTests.swift. The change in line 100 aligns with this pattern, and no other occurrences of direct presence member access were found that would need similar updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct access to internal presence
# that might need to be updated to use internalPresence
# Search for patterns that might need updating
rg -l "internal\.presence\." --type swift
# Search for test files that might contain similar patterns
fd -e swift -x grep -l "presence\.members"
Length of output: 88
Script:
#!/bin/bash
# Let's try a more comprehensive search for presence-related patterns
# Search for presence members access patterns
rg "\.presence\.members" --type swift -A 2
# Search for internal presence patterns
rg "\.internalPresence" --type swift -A 2
# Search for any presence-related test files
fd ".*Presence.*Test.*\.swift$"
Length of output: 27735
Source/ARTRealtimeChannel.m (5)
80-81: Ensure returned object conforms to ARTRealtimePresenceProtocol
The presence method returns a new instance of ARTRealtimePresence. Confirm that ARTRealtimePresence conforms to ARTRealtimePresenceProtocol. This ensures the return type matches the protocol interface.
If ARTRealtimePresence does not conform to the protocol, update its interface:
@interface ARTRealtimePresence : NSObject <ARTRealtimePresenceProtocol>570-577: Avoid duplicate calls to failsSync: method
At lines 570 and 577, failsSync: is called on self.internalPresence. Ensure that calling this method in both setDetached: and setFailed: does not result in unintended side effects, such as duplicate error handling or state transitions.
Review the call hierarchy to confirm that failsSync: is called the appropriate number of times.
1029-1030: Ensure thread safety when accessing syncInProgress_nosync
At lines 1029-1030, self.internalPresence.syncInProgress_nosync is accessed. Verify that this property access is thread-safe and does not lead to race conditions.
If necessary, synchronize access to syncInProgress_nosync using appropriate locking mechanisms.
266-266: 🛠️ Refactor suggestion
Prevent retain cycle by using a weak reference to self
Initializing _internalPresence with self can create a strong reference cycle between ARTRealtimeChannelInternal and ARTRealtimePresenceInternal if ARTRealtimePresenceInternal maintains a strong reference to the channel. This can lead to memory leaks.
Verify whether ARTRealtimePresenceInternal holds a strong reference to the channel. If it does, modify ARTRealtimePresenceInternal to use a weak reference:
// In ARTRealtimePresenceInternal.h
@property (nonatomic, weak) ARTRealtimeChannelInternal *channel;Ensure that this change does not affect the functionality of ARTRealtimePresenceInternal.
327-328:
Confirm that _internalPresence conforms to ARTRealtimePresenceProtocol
The presence method returns _internalPresence as id<ARTRealtimePresenceProtocol>. Ensure that ARTRealtimePresenceInternal indeed conforms to the ARTRealtimePresenceProtocol. If it doesn't, this will cause runtime issues.
If ARTRealtimePresenceInternal does not conform to ARTRealtimePresenceProtocol, update its declaration:
@interface ARTRealtimePresenceInternal : NSObject <ARTRealtimePresenceProtocol>Alternatively, return an object that conforms to the protocol.
Test/Tests/RealtimeClientPresenceTests.swift (1)
Line range hint 2522-2534: Verify the presence map state after processing presence messages.
To validate the handling of presence messages, assert the expected state of the presence map after processing each message. This helps confirm that the map correctly reflects the addition and removal of members.
[
Channel presence now is exposed publicly as protocol. For internal access
internalPresence(in internal class) exists, orpresence, but manually casted to internal when needed.In other words, public channel exposes public
presence, internal one exposes internalpresence(but only public properties/methods are visible by default which ofARTRealtimeChannelProtocol).internalPresenceis for explicit and more convenient way of using internal type.Closes #1994
Summary by CodeRabbit
Release Notes
New Features
presenceproperty to theARTRealtimeChannelProtocol, improving access to presence-related capabilities.Bug Fixes
Tests