[ECO-4948] Spec complete for Presence + Occupancy (excluding room status behaviour) #113
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe pull request includes updates to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
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 (
|
umair-ably
changed the title
umair-ably
force-pushed
the
ECO-4948
branch
5 times, most recently
from
86de60e
to
e557487
Compare
umair-ably
changed the title
|
@coderabbitai pause |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (15)
Sources/AblyChat/Dependencies.swift (1)
35-40: Consider using property copying loop for maintainabilityThe current implementation manually copies specific properties from
opts. This approach might miss properties if new ones are added toARTRealtimeChannelOptionsin the future.Consider using Swift's Mirror API for a more maintainable approach:
- if let customOpts = opts { - resolvedOptions.modes = customOpts.modes - resolvedOptions.cipher = customOpts.cipher - resolvedOptions.attachOnSubscribe = customOpts.attachOnSubscribe - } + if let customOpts = opts { + // Use Mirror to copy all properties except 'params' which was handled separately + let mirror = Mirror(reflecting: customOpts) + for child in mirror.children { + if child.label != "params", + let value = child.value as? Any { + resolvedOptions.setValue(value, forKey: child.label!) + } + } + }Sources/AblyChat/RoomFeature.swift (1)
25-28: Consider documenting the planned typing channel format.While the
.typingcase is not yet implemented, it would be helpful to document the planned channel format in the comment, similar to how other features are documented. This would maintain consistency in documentation and help future implementers.Example/AblyChatExample/Mocks/MockRealtime.swift (1)
Line range hint
1-3: Track TODO for mock removal.The file contains a TODO comment indicating this mock should be removed once API key integration is implemented in the example app.
Would you like me to create a GitHub issue to track the removal of this mock implementation once API key integration is complete?
Tests/AblyChatTests/IntegrationTests.swift (2)
132-172: LGTM: Comprehensive presence testing with thorough validationThe presence tests cover all essential scenarios (enter, update, leave) with proper custom data validation for both tx and rx clients.
Consider reducing code duplication by extracting the common test logic:
func verifyPresenceEvent( room: Room, action: PresenceAction, subscription: AsyncStream<PresenceEvent>, customData: [String: Any] ) async throws { try await room.presence.performAction(action, data: .init(userCustomData: customData)) let event = try await subscription.first { _ in true } #expect(event.action == action) #expect(event.data?.userCustomData?["randomData"]?.value as? String == customData["randomData"]) }
Line range hint
1-191: Consider adding error scenarios and edge casesWhile the happy path testing is thorough, consider adding these scenarios:
- Presence conflicts (same client entering multiple times)
- Network disconnection handling during presence updates
- Race conditions between presence and occupancy updates
- Invalid custom data formats
- Maximum presence member limits
Would you like me to provide example test cases for these scenarios?
Example/AblyChatExample/Mocks/MockClients.swift (1)
301-302: Consider implementing missing methods for complete test coverageSeveral methods in the mock implementation are still unimplemented (e.g.,
isUserPresent). Consider implementing these methods to ensure comprehensive integration testing.Would you like help implementing the missing methods to improve test coverage?
Example/AblyChatExample/ContentView.swift (1)
197-203: Refactor presence message handling for better maintainabilityThe presence message construction logic could be extracted to improve readability and maintainability.
Consider refactoring the message construction:
+ private func createPresenceMessage(clientID: String, action: PresenceEventType, status: String?) -> String { + let clientPresenceChangeMessage = "\(clientID) \(action.displayedText)" + return status != nil ? "\(clientPresenceChangeMessage) with status: \(status!)" : clientPresenceChangeMessage + } for await event in try await room().presence.subscribe(events: [.enter, .leave, .update]) { withAnimation { let status = event.data?.userCustomData?["status"]?.value as? String - let clientPresenceChangeMessage = "\(event.clientID) \(event.action.displayedText)" - let presenceMessage = status != nil ? "\(clientPresenceChangeMessage) with status: \(status!)" : clientPresenceChangeMessage + let presenceMessage = createPresenceMessage(clientID: event.clientID, action: event.action, status: status) messages.insert(BasicListItem(id: UUID().uuidString, title: "System", text: presenceMessage), at: 0) } }Also consider separating system messages from chat messages to improve user experience:
Consider creating a separate view model property for system messages and displaying them in a different UI component to avoid mixing them with chat messages.
Sources/AblyChat/DefaultOccupancy.swift (2)
30-31: Use descriptive variable names for better clarityThe variables
data1anddata2are not descriptive, which can make the code less readable. It's recommended to use meaningful names that reflect the data they hold.Consider renaming them to
connectionsandpresenceMembers:-let data1 = metrics["connections"] as? Int ?? 0 -let data2 = metrics["presenceMembers"] as? Int ?? 0 +let connections = metrics["connections"] as? Int ?? 0 +let presenceMembers = metrics["presenceMembers"] as? Int ?? 0
20-21: Enhance comments for maintainabilityThe comments reference specification codes like
(CHA-04a)and(CHA-04c). To improve code readability and maintainability, consider adding brief descriptions or linking to the relevant documentation.Sources/AblyChat/Presence.swift (1)
111-111: ReplaceUsing
Replace the
@unknown default: self = .unknown - print("Unknown presence action encountered: \(action)") + // Replace with appropriate logging method + Logger.log("Unknown presence action encountered: \(action)", level: .warning)Ensure that you have a
Loggerutility or use an existing logging framework to handle logging.Sources/AblyChat/Room.swift (4)
127-151: RefactorcreateFeatureChannelsfor Improved ClarityThe method
createFeatureChannelscontains nested conditionals that can be simplified for better readability and maintainability.Refactor the method to reduce complexity:
private static func createFeatureChannels(roomID: String, roomOptions: RoomOptions, realtime: RealtimeClient) -> [RoomFeature: DefaultFeatureChannel] { var channels: [RoomFeature: DefaultFeatureChannel] = [:] for feature in [RoomFeature.messages, .reactions, .presence, .occupancy] { let channelOptions = ARTRealtimeChannelOptions() if feature == .presence, let presenceOptions = roomOptions.presence { if presenceOptions.enter { channelOptions.modes.insert(.presence) } if presenceOptions.subscribe { channelOptions.modes.insert(.presenceSubscribe) } } if feature == .occupancy { channelOptions.params = ["occupancy": "metrics"] } let channel = realtime.getChannel(feature.channelNameForRoomID(roomID), opts: channelOptions) let contributor = DefaultRoomLifecycleContributor(channel: .init(underlyingChannel: channel), feature: feature) channels[feature] = .init(channel: channel, contributor: contributor) } return channels }
160-163: Enhance Error Message for Disabled PresenceThe
fatalErrormessage for thepresenceproperty might not provide sufficient context in release builds.Consider using a custom error or assertion that includes more diagnostic information to aid debugging.
178-181: Enhance Error Message for Disabled OccupancySimilarly, improve the
fatalErrormessage for theoccupancyproperty to provide clearer context.Providing detailed error messages can help developers quickly identify configuration issues.
112-124: Ensure Consistent Initialization of FeaturesThe initialization of
_presenceand_occupancydiffers in structure. Align their initialization patterns for consistency and readability.Consider refactoring
_occupancyto match the async initialization pattern of_presence:-_occupancy = options.occupancy != nil ? DefaultOccupancy( - featureChannel: featureChannels[.occupancy]!, - chatAPI: chatAPI, - roomID: roomID, - logger: logger -) : nil +_occupancy = options.occupancy != nil ? await DefaultOccupancy( + featureChannel: featureChannels[.occupancy]!, + chatAPI: chatAPI, + roomID: roomID, + logger: logger +) : nilSources/AblyChat/DefaultPresence.swift (1)
145-146: Remove redundantdecodePresenceData(from:)The
logger.log. Consider removing theApply this diff:
- print("Failed to decode PresenceData: \(error)") logger.log(message: "Failed to decode PresenceData: \(error)", level: .error)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)Example/AblyChatExample/ContentView.swift(3 hunks)Example/AblyChatExample/Mocks/MockClients.swift(3 hunks)Example/AblyChatExample/Mocks/MockRealtime.swift(1 hunks)Package.resolved(1 hunks)Sources/AblyChat/DefaultOccupancy.swift(1 hunks)Sources/AblyChat/DefaultPresence.swift(1 hunks)Sources/AblyChat/Dependencies.swift(1 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Sources/AblyChat/Presence.swift(4 hunks)Sources/AblyChat/Room.swift(4 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomOptions.swift(1 hunks)Tests/AblyChatTests/IntegrationTests.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved
- Package.resolved
- Sources/AblyChat/RoomOptions.swift
🧰 Additional context used
📓 Learnings (2)
Example/AblyChatExample/ContentView.swift (2)
Learnt from: maratal
PR: ably-labs/ably-chat-swift#34
File: Example/AblyChatExample/ContentView.swift:27-33
Timestamp: 2024-11-12T15:07:31.866Z
Learning: In `ContentView.swift`, the `room()` method does not use force try; it uses `try await` and handles errors appropriately.
Learnt from: maratal
PR: ably-labs/ably-chat-swift#34
File: Example/AblyChatExample/ContentView.swift:27-33
Timestamp: 2024-11-12T15:07:39.465Z
Learning: In `ContentView.swift`, the `room()` method does not use force try; it uses `try await` and handles errors appropriately.
Sources/AblyChat/Dependencies.swift (2)
Learnt from: umair-ably
PR: ably-labs/ably-chat-swift#45
File: Sources/AblyChat/Dependencies.swift:25-42
Timestamp: 2024-11-12T15:07:31.865Z
Learning: In the `AblyChat` project, the variable `defaultChannelOptions` is defined elsewhere and is marked with a TODO for future rework.
Learnt from: umair-ably
PR: ably-labs/ably-chat-swift#45
File: Sources/AblyChat/Dependencies.swift:25-42
Timestamp: 2024-11-12T15:07:31.866Z
Learning: In the `AblyChat` project, the variable `defaultChannelOptions` is defined elsewhere and is marked with a TODO for future rework.
🔇 Additional comments (22)
Sources/AblyChat/Occupancy.swift (2)
9-9: LGTM! Good documentation practice.
The link to the specification provides valuable context for the implementation and helps maintain consistency with the Ably SDK requirements.
Line range hint 10-17: LGTM! Well-structured data model.
The OccupancyEvent struct is well-designed with:
- Appropriate protocol conformance (
Sendable,Encodable,Decodable) - Clear public interface
- Immutable properties initialized through constructor
Let's verify the struct matches the specification exactly:
✅ Verification successful
Let me gather more information about the specification to verify the struct matches exactly.
The OccupancyEvent struct implementation is correct and matches the specification
The code shows that:
- The struct is properly documented with spec reference
(CHA-O2)inSources/AblyChat/Occupancy.swift - It's correctly used in both REST API calls (
getOccupancy) and realtime subscriptions as per(CHA-O3)and(CHA-04a) - The properties
connectionsandpresenceMembersmatch the expected format and are used consistently throughout the codebase - Integration tests verify the expected behavior with real data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the OccupancyEvent struct matches the specification
# Expected: The struct should match the occupancy event format in the specification
# Fetch the specification and check the occupancy event format
curl -s "https://sdk.ably.com/builds/ably/specification/main/chat-features/#chat-structs-occupancy-event" | grep -A 5 "occupancy event format"
# Check if this struct is used correctly in the codebase
ast-grep --pattern 'OccupancyEvent' -A 3
Length of output: 2146
Script:
#!/bin/bash
# Search for any documentation or comments about the OccupancyEvent format
rg -i "occupancy.*event" --type md
rg -i "occupancy.*event" --type swift -A 3
# Look for any test files that might contain expected format
fd "test.*swift" --exec grep -l "OccupancyEvent" {} \;
Length of output: 6971
Sources/AblyChat/Dependencies.swift (1)
27-31:
Verify complete transfer of defaultChannelOptions properties
The current implementation only merges the params dictionary from defaultChannelOptions. Other properties from defaultChannelOptions might be lost in this new implementation.
Let's verify what properties are available in defaultChannelOptions:
Consider applying this fix to preserve all properties:
- let resolvedOptions = ARTRealtimeChannelOptions()
+ // Start with defaultChannelOptions as the base
+ let resolvedOptions = defaultChannelOptions.copy() as! ARTRealtimeChannelOptions
// Merge params if available, using opts first, then defaultChannelOptions as fallback
resolvedOptions.params = (opts?.params ?? [:]).merging(
defaultChannelOptions.params ?? [:]
) { _, new in new }Sources/AblyChat/RoomFeature.swift (2)
17-21: LGTM! Well-documented channel naming conventions.
The grouping of .messages, .presence, and .occupancy cases with shared channel suffix is well-documented with clear references to specification points (CHA-M1, CHA-PR1, CHA-O1). The comments provide excellent clarity on the channel naming format.
17-21: Consider the implications of channel sharing.
While grouping messages, presence, and occupancy on the same channel (chatMessages) can reduce complexity and network overhead, it's worth considering:
- Whether this might impact scalability if message volume becomes high
- If there are any potential race conditions or ordering requirements between these different types of events
- How error handling and recovery strategies might differ for these different concerns
Let's check if there are any relevant integration tests covering this shared channel behavior:
✅ Verification successful
Let's examine the integration tests to understand how the shared channel behavior is implemented and tested.
Let's search for more details about how the room and channel interactions are tested, particularly focusing on presence and message handling.
Let's check for any concurrent message, presence, and occupancy handling in the tests to understand how the shared channel behavior is tested.
Let's try a broader search to understand how messages and events are handled in the tests.
Based on the comprehensive integration tests, I can now provide a final response about the channel sharing concerns:
Channel sharing implementation is well-tested and properly handled
The integration tests demonstrate that the shared channel design is robust and well-implemented:
- Messages, presence, and occupancy events are handled independently with separate subscriptions and clear event segregation
- Tests verify concurrent operations without interference:
- Message publishing and receiving (steps 6-9)
- Presence state changes with enter/leave/update (steps 17-23)
- Occupancy updates tracking both connections and presence members (steps 12-16)
- The implementation includes proper timing controls to handle race conditions
- Error scenarios are covered through detach/release operations (steps 24-28)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for integration tests related to channel sharing
rg -l "basicIntegrationTest|channelNameForRoomID" Tests/
Length of output: 101
Script:
#!/bin/bash
# Look at the integration tests content
rg -A 10 -B 10 "basicIntegrationTest|channelNameForRoomID" Tests/AblyChatTests/IntegrationTests.swift
Length of output: 825
Script:
#!/bin/bash
# Look for room and channel interaction tests
rg -A 20 "roomID = \"basketball\"" Tests/AblyChatTests/IntegrationTests.swift
Length of output: 723
Script:
#!/bin/bash
# Search for message, presence, and occupancy handling in the integration tests
rg -A 30 "onMessage|onPresenceChange|onOccupancyChange" Tests/AblyChatTests/IntegrationTests.swift
Length of output: 100
Script:
#!/bin/bash
# Search for any message or event handling patterns in the tests
rg -A 30 "subscribe|publish|enter|leave|update" Tests/AblyChatTests/IntegrationTests.swift
Length of output: 8177
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
5-7:
Implement presence mock for testing presence functionality
Since this PR focuses on Presence + Occupancy implementation, the presence property should be properly implemented instead of triggering fatalError. This is crucial for testing the new presence-related features.
Let's verify the usage of this mock in tests:
Would you like me to provide a sample implementation of the presence mock that aligns with the PR objectives?
Example/AblyChatExample/Mocks/MockRealtime.swift (2)
39-41: LGTM! Implementation matches mock pattern.
The presence property implementation follows the consistent pattern used throughout this mock class, throwing fatalError for unimplemented functionality. This is appropriate for a mock implementation that will be replaced later.
39-41: Verify tracking of presence implementation.
While the fatalError implementation is appropriate for now, ensure there's a tracking issue for implementing proper presence functionality in this mock, as it will be needed for testing the presence and occupancy features being developed.
Tests/AblyChatTests/IntegrationTests.swift (2)
35-50: LGTM: Well-structured room initialization with feature options
The initialization of rooms with presence, reactions, and occupancy options is consistent between both tx and rx clients, which is a good practice for integration testing.
175-191: LGTM: Proper cleanup and verification
The detach and release process is well-implemented with appropriate status change verification and object identity checks.
Example/AblyChatExample/Mocks/MockClients.swift (3)
280-280: LGTM: Improved type safety with PresenceData
The change from using a dictionary to a structured PresenceData type improves type safety and aligns with the protocol streamlining objective.
Also applies to: 292-292
Line range hint 304-314: LGTM: Consistent presence data handling
The changes properly implement the optional PresenceData parameter across all presence methods (enter, update, leave) with appropriate default values. The update method now correctly uses .update action instead of .leave.
Also applies to: 317-327, 330-340
288-288: Verify consistency with real implementation
The parameter change from optional to required is a breaking change. Let's verify this matches the real implementation.
✅ Verification successful
Parameter is correctly required in the protocol definition
The get(params:) method in the mock implementation correctly matches the protocol definition in Sources/AblyChat/Presence.swift where the parameter is required, not optional. The protocol defines two distinct methods:
func get() async throws -> [PresenceMember]func get(params: PresenceQuery) async throws -> [PresenceMember]
The implementation in the mock client is consistent with both the protocol and the real implementation in DefaultPresence.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the real Presence protocol definition and its implementations
ast-grep --pattern 'protocol Presence {
$$$
func get(params: PresenceQuery) $$$
$$$
}'
Length of output: 94
Script:
#!/bin/bash
# Let's try a broader search for Presence protocol and its method definitions
rg "protocol Presence" -A 10
# Also search for any get method with PresenceQuery parameter
ast-grep --pattern 'func get(params: PresenceQuery)'
# And search for actual implementations
rg "func get.*PresenceQuery" -A 2
Length of output: 1976
Example/AblyChatExample/ContentView.swift (2)
137-138: LGTM: Proper integration of presence and occupancy tasks
The new tasks are appropriately placed and follow the established error handling pattern using tryTask.
53-60: LGTM: Room initialization properly configured for presence and occupancy
The initialization of room options with empty instances for presence, reactions, and occupancy aligns well with the PR objectives.
Let's verify the room options are properly defined:
✅ Verification successful
Based on the search results, I can now provide a definitive response about the room options verification:
Room initialization with presence, reactions, and occupancy options is correctly implemented
The codebase shows proper support for these options:
RoomOptionsstruct inSources/AblyChat/RoomOptions.swiftcorrectly defines all required fields:presence: PresenceOptions?reactions: RoomReactionsOptions?occupancy: OccupancyOptions?
- Empty initialization is valid as these are optional properties with proper default values
- The implementation follows the specification requirements (CHA-PR9)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for room options structure definition
ast-grep --pattern 'struct $ROOM_OPTIONS {
$$$
}'
Length of output: 53
Script:
#!/bin/bash
# Let's try a broader search for room-related options or configurations
rg -i "room.*options" -A 5 -B 5
Length of output: 44497
Sources/AblyChat/DefaultOccupancy.swift (1)
47-49: Verify access control levels for consistency
The method subscribeToDiscontinuities() is marked as internal. Ensure that this access level aligns with how the method will be used elsewhere in the codebase. If it needs to be accessible outside the module, consider marking it as public.
Please review all method access levels to confirm they match the intended visibility.
Sources/AblyChat/Presence.swift (1)
81-83: Ensure nil data is properly handled in presence methods
The methods enter(data:), update(data:), and leave(data:) now accept optional PresenceData?. Ensure that the implementations of these methods correctly handle nil data and that passing nil aligns with the intended behavior of the presence system.
Run the following script to check for method implementations handling nil data:
✅ Verification successful
nil data is properly handled in presence methods
After reviewing the implementation in DefaultPresence.swift, I can confirm that nil data is properly handled:
- All presence methods (
enter,update,leave) have default parameter values ofnil - The data is safely passed through using optional chaining:
data?.asQueryItems() - When decoding presence data,
nilis properly handled indecodePresenceData - The
PresenceDatastructure itself is designed to handle optionaluserCustomData - When
nilor empty data is provided, it's converted to an empty string inasQueryItems()method
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for implementations of presence methods and check for handling of nil data.
# Find all implementations of Presence protocol methods
rg -A 5 'func enter\(data: PresenceData\?\)' --no-heading
# Check for handling of nil data within these methods
rg -A 10 'func enter\(data: PresenceData\?\)' --no-heading | rg 'if.*data == nil' -B 2 -A 2
Length of output: 662
Script:
#!/bin/bash
# Let's search for the actual implementations of these presence methods
# First, find files that might contain the implementations
fd -e swift | xargs rg -l "class.*:.*Presence"
# Then look for the actual implementations
rg -A 10 "func (enter|update|leave)\(data: PresenceData\?\)" --no-heading
# Also search for any test files that might show usage patterns
fd -e swift | xargs rg -l "TestCase.*Presence"
Length of output: 1101
Script:
#!/bin/bash
# Let's check the DefaultPresence implementation
cat Sources/AblyChat/DefaultPresence.swift
# Also search for any usage of PresenceData to understand the data structure
rg "PresenceData" -A 5 -B 5
Length of output: 26208
Sources/AblyChat/Room.swift (1)
87-90: Verify All Required Features are Properly Initialized
Ensure that all feature channels are initialized when their corresponding options are enabled, and handle cases where they might be missing to prevent crashes.
Run the following script to check for any uninitialized feature channels:
✅ Verification successful
Let me gather more information about how these feature channels are created and initialized.
Let me gather one final piece of information about the RoomFeature enum to understand all possible features.
Feature Channel Initialization is Properly Handled
The code is correctly handling feature channel initialization. Here's why:
- All required features (messages, reactions, presence, occupancy) are explicitly initialized in
createFeatureChannels. - The force unwrapping of feature channels is safe because:
- The channels dictionary is populated with all features during initialization
- Each feature access is guarded by corresponding options in RoomOptions
- There's an existing GitHub issue (Only create channels / features for the features that the user has requested #105) tracking the force unwrapping improvement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all enabled features have corresponding channels initialized.
# Search for feature channel initializations
rg 'featureChannels\[\.\w+\]!\s*' -A 5
Length of output: 1366
Script:
#!/bin/bash
# Search for feature channel creation and initialization
rg "createFeatureChannels" -A 10
# Search for feature channel options
rg "RoomOptions" -A 5
Length of output: 16561
Script:
#!/bin/bash
# Search for RoomFeature enum definition
rg "enum RoomFeature" -A 10
Length of output: 659
Sources/AblyChat/DefaultPresence.swift (4)
35-42: Ensure continuation is properly resumed in get(params:) method
Similar to the get() method, the get(params:) method needs to ensure the continuation is resumed in all code paths. Remove the unnecessary Task and handle errors appropriately.
Apply this diff:
return try await withCheckedThrowingContinuation { continuation in
channel.presence.get(params.asARTRealtimePresenceQuery()) { [processPresenceGet] members, error in
- Task {
do {
try processPresenceGet(continuation, members, error)
+ // Continuation is resumed inside processPresenceGet
} catch {
+ continuation.resume(throwing: error)
}
- }
}
}75-85: Await asynchronous call and handle errors in update(data:) method
Similarly to enter, the update method should await the completion and handle errors using withCheckedThrowingContinuation.
Apply this diff:
- channel.presence.update(data?.asQueryItems()) { [logger] error in
- Task {
- if let error {
- logger.log(message: "Error updating presence: \(error)", level: .error)
- throw error
- }
- }
- }
+ try await withCheckedThrowingContinuation { continuation in
+ channel.presence.update(data?.asQueryItems()) { [logger] error in
+ if let error {
+ logger.log(message: "Error updating presence: \(error)", level: .error)
+ continuation.resume(throwing: error)
+ } else {
+ continuation.resume()
+ }
+ }
+ }88-98: Await asynchronous call and handle errors in leave(data:) method
The leave method should also use withCheckedThrowingContinuation to properly await the asynchronous operation and handle any errors.
Apply this diff:
- channel.presence.leave(data?.asQueryItems()) { [logger] error in
- Task {
- if let error {
- logger.log(message: "Error leaving presence: \(error)", level: .error)
- throw error
- }
- }
- }
+ try await withCheckedThrowingContinuation { continuation in
+ channel.presence.leave(data?.asQueryItems()) { [logger] error in
+ if let error {
+ logger.log(message: "Error leaving presence: \(error)", level: .error)
+ continuation.resume(throwing: error)
+ } else {
+ continuation.resume()
+ }
+ }
+ }115-128: Handle errors in subscribe(events:) method's closure
The same issue exists in the subscribe(events:) method. Ensure that errors thrown by processPresenceSubscribe are caught and handled to prevent unhandled exceptions.
Apply this diff:
channel.presence.subscribe(event.toARTPresenceAction()) { [processPresenceSubscribe, logger] message in
logger.log(message: "Received presence message: \(message)", level: .debug)
Task {
- let presenceEvent = try processPresenceSubscribe(message, event)
- subscription.emit(presenceEvent)
+ do {
+ let presenceEvent = try processPresenceSubscribe(message, event)
+ subscription.emit(presenceEvent)
+ } catch {
+ logger.log(message: "Error processing presence message: \(error)", level: .error)
+ // Handle error appropriately
+ }
}
}
✅ Actions performedReviews paused. |
Dataparam where possibleWill add unit tests as part of a separate PR/ticket
Summary by CodeRabbit
New Features
DefaultOccupancyandDefaultPresenceclasses for improved event handling.Documentation
RoomOptionsandPresenceOptionsto clarify configuration details.Tests