[ECO-5120] Adds and improves logging for Messages and Reactions #117
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
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: 12
🧹 Outside diff range and nitpick comments (20)
Sources/AblyChat/Occupancy.swift (1)
Line range hint
11-13: Consider adding property documentationWhile the struct has a reference to the specification, it would be helpful to add documentation comments for the properties explaining their purpose and any constraints.
public struct OccupancyEvent: Sendable, Encodable, Decodable { + /// The number of active connections to the channel public var connections: Int + /// The number of presence members in the channel public var presenceMembers: IntAblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
8-10: Consider pinning ably-cocoa to a specific version instead of main branchUsing the
mainbranch for dependencies can lead to non-reproducible builds and unexpected breaking changes. Consider pinning to a specific version or commit hash for better stability and reproducibility.Sources/AblyChat/Dependencies.swift (1)
27-31: Consider handling nil defaultChannelOptions.paramsThe merging logic assumes
defaultChannelOptions.paramsis not nil. While the current implementation won't crash due to the nil-coalescing operator, it might be clearer to handle this explicitly.Consider this more defensive approach:
- resolvedOptions.params = (opts?.params ?? [:]).merging( - defaultChannelOptions.params ?? [:] - ) { _, new in new } + resolvedOptions.params = (opts?.params ?? [:]).merging( + defaultChannelOptions.params ?? [:] + ) { current, new in + // Explicitly choose to keep the new value + new + }Sources/AblyChat/RoomFeature.swift (2)
17-20: LGTM! Good architectural decision to group related features.The consolidation of messages, presence, and occupancy features to use the same channel is a clean approach that follows the DRY principle and simplifies the channel management.
19-20: Consider consolidating redundant documentation.The comments for presence (CHA-PR1) and occupancy (CHA-O1) contain identical channel format information. Consider consolidating these to reduce duplication while maintaining the spec references.
- // (CHA-PR1) Presence for a Room is exposed on the realtime channel used for chat messages, in the format <roomId>::$chat::$chatMessages. For example, if your room id is my-room then the presence channel will be my-room::$chat::$chatMessages. - // (CHA-O1) Occupancy for a room is exposed on the realtime channel used for chat messages, in the format <roomId>::$chat::$chatMessages. For example, if your room id is my-room then the presence channel will be my-room::$chat::$chatMessages. + // (CHA-PR1, CHA-O1) Both presence and occupancy for a Room are exposed on the same realtime channel used for chat messages, + // in the format <roomId>::$chat::$chatMessages. For example: my-room::$chat::$chatMessagesTests/AblyChatTests/DefaultMessagesTests.swift (1)
Line range hint
15-21: Consider parameterizing common test setup.Each test method has similar initialization code for
DefaultMessages. Consider extracting this into a helper method or using a test fixture to reduce code duplication and improve maintainability.Example refactor:
private func createDefaultMessages( channel: MockRealtimeChannel = MockRealtimeChannel(), realtime: MockRealtime = MockRealtime.create() ) async -> DefaultMessages { let chatAPI = ChatAPI(realtime: realtime) let featureChannel = MockFeatureChannel(channel: channel) return await DefaultMessages( featureChannel: featureChannel, chatAPI: chatAPI, roomID: "basketball", clientID: "clientId", logger: TestLogger() ) }Also applies to: 33-39, 58-65, 80-86
Sources/AblyChat/DefaultRoomReactions.swift (1)
38-38: Consider limiting logged message contentThe full message object might contain sensitive information. Consider logging only necessary fields or implementing message sanitization before logging.
Example implementation:
-logger.log(message: "Received roomReaction message: \(message)", level: .debug) +logger.log(message: "Received roomReaction message with id: \(message.id)", level: .debug)Tests/AblyChatTests/IntegrationTests.swift (2)
106-106: Make connection count assertion more deterministicThe current check for non-zero connections is non-deterministic. Consider asserting the exact expected count based on the test setup (2 clients in this case).
#expect(currentOccupancy.connections == 2) // exactly 2 clients (txRoom and rxRoom)
134-172: Reduce duplication in presence test dataThe presence tests are thorough but contain repeated test data. Consider extracting common test data and expectations into helper functions.
Example refactor:
private func verifyPresenceEvent( event: PresenceEvent, expectedAction: PresenceAction, expectedData: String = "randomValue" ) { #expect(event.action == expectedAction) #expect(event.data?.userCustomData?["randomData"]?.value as? String == expectedData) } // Usage: let rxPresenceEnterTxEvent = try #require(await rxPresenceSubscription.first { _ in true }) verifyPresenceEvent(event: rxPresenceEnterTxEvent, expectedAction: .enter)Also consider adding a test for concurrent presence states:
// Test concurrent presence try await txRoom.presence.enter(data: nil) try await rxRoom.presence.enter(data: nil) let presenceMembers = try await rxRoom.presence.get() #expect(presenceMembers.count == 2) // Both clients should be presentSources/AblyChat/DefaultMessages.swift (4)
168-171: Consider enhancing error logging with more contextWhile the logging implementation is good, consider enriching the error logging with additional context:
-logger.log(message: "Error handling attach: \(error)", level: .error) +logger.log(message: "Error handling attach for room \(roomID): \(error)", level: .error)Also applies to: 179-184
208-217: Consider adding channel state context to logsWhile the logging is good, adding channel state information could help with debugging:
-logger.log(message: "Resolving timeserial on channel attach", level: .debug) +logger.log(message: "Resolving timeserial on channel attach (current state: \(channel.state))", level: .debug)
Line range hint
244-252: Consider adding more context to failure loggingThe error logging could be enhanced with additional state information:
-logger.log(message: "Channel failed to attach", level: .error) +logger.log(message: "Channel failed to attach - Room: \(roomID), State: \(stateChange.current), Previous: \(stateChange.previous)", level: .error)
Line range hint
19-252: Consider implementing structured logging for better observabilityWhile the current logging implementation is solid, consider adopting a structured logging approach for better observability:
- Use consistent key-value pairs for common fields (roomID, channelState, etc.)
- Consider adding correlation IDs for tracking message flow
- Add timestamp information in a standardized format
This would make log aggregation and analysis more efficient in production environments.
Sources/AblyChat/DefaultOccupancy.swift (3)
7-7: Consider removing 'nonisolated' from 'featureChannel' property in non-actor classThe
nonisolatedkeyword is intended for use with actor classes to indicate that a property or method is not isolated to the actor. SinceDefaultOccupancyis not declared as an actor, the use ofnonisolatedhere is unnecessary and can be removed.Apply this diff to remove
nonisolated:-public nonisolated let featureChannel: FeatureChannel +public let featureChannel: FeatureChannel
9-9: Consider removing 'nonisolated' from 'channel' computed propertySimilarly, since
DefaultOccupancyis not an actor, thenonisolatedkeyword on thechannelproperty is redundant and can be omitted.Apply this diff:
-internal nonisolated var channel: any RealtimeChannelProtocol { +internal var channel: any RealtimeChannelProtocol {
23-23: Re-evaluate the necessity of 'async' in 'subscribe' methodThe
subscribe(bufferingPolicy:)method is marked asasync, but the implementation does not contain anyawaitcalls. Unless required by protocol conformance or for anticipated asynchronous operations, you may consider removing theasyncmodifier to simplify the method signature.Apply this diff if appropriate:
-internal func subscribe(bufferingPolicy: BufferingPolicy) async -> Subscription<OccupancyEvent> { +internal func subscribe(bufferingPolicy: BufferingPolicy) -> Subscription<OccupancyEvent> {Sources/AblyChat/Presence.swift (2)
5-5: Consider usingDoublefor numeric presence data to support floating-point numbersChanging the
numbercase fromNSNumbertoIntlimits the presence data to integer values only. If there's a possibility of handling floating-point numbers in presence data, it's advisable to useDoubleinstead ofInt.Apply this diff to update the
PresenceCustomDataenum:public enum PresenceCustomData: Sendable, Codable, Equatable { case string(String) - case number(Int) // Changed from NSNumber to Int to conform to Codable. Address in linked issue above. + case number(Double) // Use Double to support floating-point numbers case bool(Bool) case nullAlso, update the
init(from decoder:)method to decodeDoublevalues:public init(from decoder: Decoder) throws { let container = try decoder.singleValueContainer() + if let value = try? container.decode(Double.self) { + self = .number(value) + } else if let value = try? container.decode(Int.self) { + self = .number(Double(value)) } else if let value = try? container.decode(String.self) { self = .string(value) } else if let value = try? container.decode(Bool.self) { self = .bool(value) } else { self = .null } }Update the
asQueryItems()method to handleDoublevalues:switch value { case let .string(stringValue): userCustomDataDict[key] = stringValue case let .number(numberValue): userCustomDataDict[key] = numberValue case let .bool(boolValue): userCustomDataDict[key] = boolValue case .null: userCustomDataDict[key] = NSNull() }
48-74: Avoid usingNSNull()in Swift dictionariesUsing
NSNull()in Swift dictionaries is not idiomatic and can lead to issues when handling data. Instead, consider omitting the key or usingOptionalvalues to representnull.Modify the
asQueryItems()method to exclude keys withnullvalues:for (key, value) in userCustomData { switch value { case let .string(stringValue): userCustomDataDict[key] = stringValue case let .number(numberValue): userCustomDataDict[key] = numberValue case let .bool(boolValue): userCustomDataDict[key] = boolValue - case .null: - userCustomDataDict[key] = NSNull() // Use NSNull to represent null in the dictionary + case .null: + // Omit keys with null values + continue } }Alternatively, if it's necessary to represent
null, you can useuserCustomDataDict[key] = nil, but ensure that the dictionary is of type[String: Any?]to acceptnilvalues.- func asQueryItems() -> [String: Any] { + func asQueryItems() -> [String: Any?] {Sources/AblyChat/DefaultPresence.swift (2)
159-159: RemoveUsing
logger.log, you can remove theApply this diff to remove the unnecessary
- print("Failed to decode PresenceData: \(error)")
116-116: Correct log message to reflect singular event subscriptionThe log message should indicate that you're subscribing to a single presence event. Additionally, including the event type in the log message can enhance clarity.
Apply this diff to update the log message:
- logger.log(message: "Subscribing to presence events", level: .debug) + logger.log(message: "Subscribing to presence event: \(event)", level: .debug)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
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/DefaultMessages.swift(5 hunks)Sources/AblyChat/DefaultOccupancy.swift(1 hunks)Sources/AblyChat/DefaultPresence.swift(1 hunks)Sources/AblyChat/DefaultRoomReactions.swift(2 hunks)Sources/AblyChat/Dependencies.swift(1 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Sources/AblyChat/Presence.swift(4 hunks)Sources/AblyChat/Room.swift(5 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomOptions.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(4 hunks)Tests/AblyChatTests/IntegrationTests.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- 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 (27)
Sources/AblyChat/Occupancy.swift (1)
Line range hint 9-17: Well-structured implementation with good protocol conformance!
The OccupancyEvent struct is well-designed with appropriate protocol conformance for thread-safety (Sendable) and serialization (Codable). The public access level is consistent with the protocol's requirements.
AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
10-10: Verify the ably-cocoa revision update
The ably-cocoa dependency has been updated to revision f7bff4b1. Let's verify this specific version supports the logging features being added.
✅ Verification successful
Let me gather more information about the logging features in this version.
Let me check the CHANGELOG.md to verify if there are any breaking changes or logging-related updates around this version.
Let me try a different approach to check the changelog and recent changes.
Based on the gathered information, I can now provide the final response:
The ably-cocoa revision is safe to use
The version f7bff4b1 is a stable release that focuses on presence-related changes and contains no breaking changes or modifications to the logging system. The CHANGELOG.md and commit history show this is a minor update that exposes channel presence publicly as a protocol, which doesn't affect logging functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the ably-cocoa commit for logging-related changes and breaking changes
# Fetch commit details and changes
gh api \
-H "Accept: application/vnd.github+json" \
repos/ably/ably-cocoa/commits/f7bff4b1c941b4c7b952b9224a33674e2302e19f \
--jq '.commit.message'
# Check for any breaking changes in the diff
gh api \
-H "Accept: application/vnd.github+json" \
repos/ably/ably-cocoa/commits/f7bff4b1c941b4c7b952b9224a33674e2302e19f \
--jq '.files[].patch' | grep -i "break"
Length of output: 505
Script:
#!/bin/bash
# Check for logging-related changes and features in the repository
# Look for logging-related files and changes around this commit
# Search for logging-related files and changes in recent commits
gh api \
-H "Accept: application/vnd.github+json" \
repos/ably/ably-cocoa/commits/f7bff4b1c941b4c7b952b9224a33674e2302e19f~5..f7bff4b1c941b4c7b952b9224a33674e2302e19f \
--jq '.[] | {sha: .sha, message: .commit.message}'
# Search for logging-related files in the repository at this commit
gh api \
-H "Accept: application/vnd.github+json" \
repos/ably/ably-cocoa/git/trees/f7bff4b1c941b4c7b952b9224a33674e2302e19f?recursive=1 \
--jq '.tree[] | select(.path | test("(?i)log|debug|trace"))'
Length of output: 7333
Script:
#!/bin/bash
# Get the content of CHANGELOG.md at this specific commit
gh api \
-H "Accept: application/vnd.github+json" \
repos/ably/ably-cocoa/contents/CHANGELOG.md?ref=f7bff4b1c941b4c7b952b9224a33674e2302e19f \
--jq '.content' | base64 -d | head -n 100
# Also check the logging-related files for any recent changes
gh api \
-H "Accept: application/vnd.github+json" \
repos/ably/ably-cocoa/commits/f7bff4b1c941b4c7b952b9224a33674e2302e19f \
--jq '.files[] | select(.filename | test("(?i)log|debug|trace")) | {filename: .filename, status: .status}'
Length of output: 482
Script:
#!/bin/bash
# Get raw content of CHANGELOG.md
curl -s -L https://raw.githubusercontent.com/ably/ably-cocoa/f7bff4b1c941b4c7b952b9224a33674e2302e19f/CHANGELOG.md | head -n 50
# Get the commit message and changed files
gh api \
-H "Accept: application/vnd.github+json" \
repos/ably/ably-cocoa/commits/f7bff4b1c941b4c7b952b9224a33674e2302e19f \
--jq '{message: .commit.message, files: [.files[].filename]}'
Length of output: 2611
Sources/AblyChat/Dependencies.swift (2)
35-40: LGTM! Clean and explicit options handling
The code clearly handles each custom option individually, making it easy to understand and maintain. The explicit handling of modes, cipher, and attachOnSubscribe ensures that these important channel configurations are properly set when custom options are provided.
27-41: Verify alignment with PR objectives
The PR objectives mention adding logging for Messages and Reactions, but this file contains channel configuration changes. While these changes might be necessary infrastructure updates, please ensure they are related to the logging implementation.
Let's verify the relationship between these changes and the logging implementation:
✅ Verification successful
Channel configuration changes support logging implementation
The changes in Dependencies.swift are justified as they support the logging implementation found in both DefaultMessages and DefaultRoomReactions classes. The evidence shows:
- Both classes have
logger: InternalLoggeras a dependency and actively use it for logging - Both classes interact with channels through
featureChannelproperty - The channel options being configured in
Dependencies.swiftare part of the infrastructure that enables these logging-enabled components to function properly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for logging-related changes in Messages and Reactions components
echo "Searching for logging implementation in Messages..."
rg -l "Logger|logging" --type swift | grep -i "message"
echo "Searching for logging implementation in Reactions..."
rg -l "Logger|logging" --type swift | grep -i "reaction"
echo "Searching for references to getChannel in logging context..."
rg -B 3 -A 3 "getChannel.*logging"
Length of output: 690
Script:
#!/bin/bash
# Let's examine the logging implementation in Messages and Reactions files
echo "=== DefaultMessages.swift logging implementation ==="
rg -A 10 -B 10 "Logger|logging" Sources/AblyChat/DefaultMessages.swift
echo -e "\n=== DefaultRoomReactions.swift logging implementation ==="
rg -A 10 -B 10 "Logger|logging" Sources/AblyChat/DefaultRoomReactions.swift
echo -e "\n=== Checking if channel options are used in logging context ==="
rg -A 5 -B 5 "ARTRealtimeChannelOptions.*Logger|Logger.*ARTRealtimeChannelOptions" --type swift
echo -e "\n=== Checking DefaultMessages.swift for channel options usage ==="
rg "ARTRealtimeChannelOptions" Sources/AblyChat/DefaultMessages.swift
echo -e "\n=== Checking DefaultRoomReactions.swift for channel options usage ==="
rg "ARTRealtimeChannelOptions" Sources/AblyChat/DefaultRoomReactions.swift
Length of output: 3998
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
15-15: Add test coverage for logging behavior.
While the logger parameter has been added to DefaultMessages initialization, there are no tests verifying that logging is working as expected. Consider adding test cases that:
- Verify log messages are emitted during error scenarios
- Validate logging during successful operations
- Ensure correct log levels are used
Let's verify the TestLogger implementation:
Also applies to: 33-33, 58-58, 80-80
Sources/AblyChat/DefaultRoomReactions.swift (3)
74-74: Good error logging implementation
Excellent addition of error logging for failed message processing. This will help with debugging issues in production.
Line range hint 33-71: Well-implemented logging enhancement
The addition of logging throughout the message flow provides good traceability for debugging. The implementation covers all key points in the reaction processing pipeline with appropriate log levels.
25-25: Consider PII exposure in logging
While adding debug logging is helpful, the params object might contain sensitive user information. Consider sanitizing or limiting the logged information to non-sensitive fields.
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)
Line range hint 1-250: Consider adding logging-related mock methods
Given that this PR's primary objective is to add and improve logging for Messages and Reactions, consider adding methods to verify logging behavior in tests.
Let's verify if there are any logging-related methods that need to be mocked:
Consider adding properties to track logging calls:
// Add these properties
var loggedMessages: [(level: String, message: String)] = []
// Add a method to verify logging
func verifyLogged(level: String, message: String) -> Bool {
loggedMessages.contains { $0.level == level && $0.message == message }
}5-7: 🛠️ Refactor suggestion
Consider implementing the presence property for testing purposes
Since this PR builds upon previous presence/occupancy work (PR #113), the mock presence implementation might be needed for testing the new logging features. Consider implementing a basic mock presence object instead of throwing fatalError.
Here's a suggested implementation:
- var presence: ARTRealtimePresenceProtocol {
- fatalError("Not implemented")
- }
+ private lazy var mockPresence = MockRealtimePresence()
+ var presence: ARTRealtimePresenceProtocol {
+ return mockPresence
+ }You'll need to create a MockRealtimePresence class implementing ARTRealtimePresenceProtocol with the necessary mock methods for testing.
⛔ Skipped due to learnings
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#35
File: Example/AblyChatExample/Mocks/MockRealtime.swift:16-32
Timestamp: 2024-11-12T15:07:31.865Z
Learning: When reviewing code, do not ask lawrence-forooghian about "not yet implemented" `fatalError` calls.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#35
File: Example/AblyChatExample/Mocks/MockRealtime.swift:34-174
Timestamp: 2024-11-12T15:07:31.865Z
Learning: When reviewing code, do not ask lawrence-forooghian about "not yet implemented" `fatalError` calls.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#35
File: Example/AblyChatExample/Mocks/MockRealtime.swift:16-32
Timestamp: 2024-11-12T15:07:31.865Z
Learning: When reviewing code, do not ask lawrence-forooghian about "not yet implemented" `fatalError` calls.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#35
File: Example/AblyChatExample/Mocks/MockRealtime.swift:34-174
Timestamp: 2024-11-12T15:07:31.865Z
Learning: When reviewing code, do not ask lawrence-forooghian about "not yet implemented" `fatalError` calls.
Example/AblyChatExample/Mocks/MockRealtime.swift (2)
39-41: LGTM! The presence property implementation follows the established pattern.
The implementation is consistent with other unimplemented properties in this mock class and correctly conforms to ARTRealtimePresenceProtocol.
Line range hint 1-42: Verify the lifecycle of this mock implementation.
There's a TODO comment at the top of the file indicating this mock is temporary and should be removed once the example app is built. Given that we're now adding presence support, we should verify if this assumption still holds true.
Tests/AblyChatTests/IntegrationTests.swift (2)
35-50: LGTM: Room initialization with presence and occupancy options
The initialization is well-structured and symmetric for both transmitting and receiving rooms, properly setting up the test environment for presence and occupancy features.
175-191: LGTM: Cleanup section renumbering
The cleanup operations are properly sequenced and maintain the correct logical flow of the test.
Example/AblyChatExample/Mocks/MockClients.swift (6)
280-280: LGTM: Type-safe presence data initialization
The change to use PresenceData(userCustomData: nil) improves type safety compared to the previous dictionary approach.
Also applies to: 292-292
Line range hint 304-314: LGTM: Proper handling of optional presence data in enter
The implementation correctly handles the optional presence data and properly emits it in the PresenceEvent.
317-327: LGTM: Correct action type for presence updates
The implementation now correctly uses .update action type and properly handles the optional presence data.
Line range hint 330-341: LGTM: Proper handling of optional presence data in leave
The implementation correctly handles the optional presence data while maintaining the proper .leave action type.
Line range hint 288-297: Consider utilizing the PresenceQuery parameter
The method accepts a PresenceQuery parameter but doesn't use it in the implementation. This could be misleading for test scenarios that rely on query parameters.
Let's check how PresenceQuery is used in the actual implementation:
Consider either:
- Using the query params to filter the mock results
- Adding a comment explaining why the parameter is ignored in the mock implementation
Line range hint 1-391: Verify logging implementation for presence events
The PR objectives mention adding and improving logging, but I don't see any logging implementation in this mock client. Consider adding logging to track presence events in the mock implementation to maintain consistency with the real implementation.
Let's check for logging implementation in the actual presence implementation:
Sources/AblyChat/DefaultMessages.swift (4)
19-19: LGTM: Logger initialization is properly implemented
The logger property and its initialization in the constructor are correctly implemented, maintaining the nonisolated context.
Also applies to: 25-30
43-43: LGTM: Appropriate debug logging for subscription start
The debug log is well-placed at the beginning of the subscription process with a clear message.
189-198: LGTM: Comprehensive logging for subscription resolution
The logging implementation provides good coverage of both success and error paths with appropriate log levels.
235-240: LGTM: Well-structured logging for channel attachment
The logging implementation properly handles both successful and failed attachment scenarios with appropriate detail.
Sources/AblyChat/DefaultOccupancy.swift (1)
47-50: Implementation of 'get()' method is correct
The get() method appropriately logs the action and uses the chatAPI to retrieve the occupancy data, handling any thrown errors as expected.
Example/AblyChatExample/ContentView.swift (2)
53-60: Initialization of Room with Presence, Reactions, and Occupancy
The room is initialized with presence, reactions, and occupancy options as empty configurations. Ensure that these initializations meet the requirements and that any necessary configurations are provided within these options.
137-138: Verify Concurrent Asynchronous Tasks Execution
Adding showPresence() and showOccupancy() to the asynchronous tasks list. Verify that executing these tasks concurrently does not lead to race conditions or unintended side effects.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Summary by CodeRabbit
New Features
DefaultOccupancyandDefaultPresenceclasses for managing occupancy and presence events.Bug Fixes
Documentation
RoomOptions.Tests