Messages and room reactions tests #219
Conversation
WalkthroughThis pull request implements improved error handling and debugging support throughout the chat API. The production code now uses conditional compilation blocks in debug mode to manage malformed message events, including new properties and methods in both the messages and reactions modules. Test files have been reorganized—some tests have been removed, renamed, or enhanced to verify new behaviors. Updates to helper and mock classes introduce optional parameters and asynchronous state callbacks, altering the way state transitions and pagination responses are handled. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant DM as DefaultMessages
participant Subscriber as MalformedMessageSubscription
Client->>DM: Receive incoming message
DM->>DM: Process message in do-catch block
alt Error Occurred (DEBUG)
DM->>Subscriber: Emit malformed message event
else Normal
DM->>Client: Process message normally
end
sequenceDiagram
participant Test as Test
participant MRC as MockRealtimeChannel
participant Callback as State Callback
Test->>MRC: Call attach()
MRC->>MRC: Update channel state (optional _state set)
MRC-->>Callback: Invoke attach callbacks asynchronously
Possibly related PRs
Poem
✨ Finishing Touches
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
maratal
force-pushed
the
86-messages-tests
branch
from
efa31ef to
33ff630
Compare
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
maratal
force-pushed
the
86-messages-tests
branch
from
33ff630 to
a874516
Compare
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
maratal
force-pushed
the
86-messages-tests
branch
from
a874516 to
87dc17a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
maratal
force-pushed
the
86-messages-tests
branch
from
87dc17a to
82263d8
Compare
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
maratal
force-pushed
the
86-messages-tests
branch
from
82263d8 to
b4a61c1
Compare
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
maratal
force-pushed
the
86-messages-tests
branch
from
b4a61c1 to
8df25ec
Compare
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
maratal
force-pushed
the
86-messages-tests
branch
from
8df25ec to
a15630d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
maratal
force-pushed
the
86-messages-tests
branch
from
a15630d to
ea960b3
Compare
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
|
I'm seeing failures for message tests Locally failure rate is 14-18% and on CI it feels like more than 50%. The chain of events leading to failures looks the same as for when those tests passed: 
So before diving into it I would like to assure that the code under these tests is valid, and I'm marking this PR as ready for review now to start a discussion. FYI @umair-ably @lawrence-forooghian UPD: Those fail 90% of the time if executed together with the other tests. UPD: Tests don't fail if parallel execution is turned off (it's still very fast - the duration of the longest (integration) test takes almost all time). I've tried to edit BuildTool, but it doesn't take my parameter. Couldn't you please look into it? @lawrence-forooghian FYI @umair-ably |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
Tests/AblyChatTests/DefaultMessagesTests.swift (2)
6-24: Validate returned message fields in “clientMaySendMessageViaRESTChatAPI”.Testing only the text field might not catch potential serialization errors in other fields (e.g.,
serialormetadata). Consider asserting on additional returned attributes (if relevant) to strengthen this test.
322-356: Additional field checks recommended in “subscriptionCanBeRegisteredToReceiveIncomingMessages”.Currently, only
headersandmetadataare checked. For completeness, consider verifyingtextor other fields.Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (3)
122-133: Asynchronous attach callbacks.
Using@MainActoris acceptable in tests. If the code might be used outside tests, consider more robust error handling instead offatalError.
135-146: Asynchronous detach callbacks.
Similar to attach callbacks, consider capturing errors more gracefully rather than callingfatalError.
161-163: Spawned Task inside attach.
Consider using structured concurrency (e.g., makingattachan async function) rather than spawning a top-level Task if attach is repeatedly called. This prevents concurrency mishaps.Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (1)
131-133: Asynchronous state change emission.
Creating a new Task from within the actor is safe, but you might prefer usingawaitin the actor scope. Ensure you truly need concurrency here.Tests/AblyChatTests/DefaultRoomPresenceTests.swift (1)
84-84: Consider a more robust solution for timing-sensitive tests.The current changes double the delay to prevent race conditions where attach failure happens before the lifecycle manager starts waiting. While this fix works, it makes the tests more brittle as they depend on specific timing values.
Consider implementing a more robust solution:
- Extract the delay multiplier to a constant to make it easier to adjust if needed.
- Investigate if
SignallableChannelOperationcan be made to work with the test framework's async/await syntax.- Consider implementing a custom expectation mechanism that doesn't rely on timing.
+ // At the top of the file + private enum Constants { + static let attachFailureDelayMultiplier = 2 + } - let contributor = RoomLifecycleHelper.createContributor(feature: .presence, attachBehavior: .completeAndChangeState(.failure(attachError), newState: .failed, delayInMilliseconds: RoomLifecycleHelper.fakeNetworkDelay * 2)) + let contributor = RoomLifecycleHelper.createContributor(feature: .presence, attachBehavior: .completeAndChangeState(.failure(attachError), newState: .failed, delayInMilliseconds: RoomLifecycleHelper.fakeNetworkDelay * Constants.attachFailureDelayMultiplier))Also applies to: 204-204, 382-382
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Sources/AblyChat/DefaultMessages.swift(4 hunks)Tests/AblyChatTests/ChatAPITests.swift(0 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(4 hunks)Tests/AblyChatTests/DefaultRoomPresenceTests.swift(3 hunks)Tests/AblyChatTests/DefaultRoomTypingTests.swift(2 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(2 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(4 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(9 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift(5 hunks)
💤 Files with no reviewable changes (1)
- Tests/AblyChatTests/ChatAPITests.swift
✅ Files skipped from review due to trivial changes (1)
- Tests/AblyChatTests/DefaultRoomTypingTests.swift
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Xcode, tvOS (Xcode 16)
🔇 Additional comments (44)
Tests/AblyChatTests/DefaultMessagesTests.swift (11)
28-47: Looks good.The test thoroughly verifies the absence of headers and metadata when not specified. This coverage is sufficient.
51-64: All set.The test properly checks for thrown errors, aligning well with the code’s intended behavior.
68-86: Proper subscription point for attached channel.This test accurately ensures
fromSerialis set to the channel serial once attached. No issues found.
90-118: Correct subscription point for non-attached channel.The test properly waits for attach and verifies
attachSerialusage. Implementation seems correct.
122-162: Reattached channel behavior validated.This covers the resumed=false scenario thoroughly, resetting subscription points to
attachSerial.
166-213: UPDATE event handling appears correct.Resets subscription point to
attachSerialon resumed=false. This behavior aligns with the specification.
220-251: Adequate coverage for history query options.The forced “backwards” direction is confirmed, and
fromSerialis verified. Test coverage looks complete.
255-272: Error handling on previous messages is validated.Throws the expected server error, matching the specification.
277-296: History query options for getMessages are verified.Pagination checks are correct and consistent with your mock responses.
300-315: Server error path verified for getMessages.The test ensures correct error throwing, aligning with the expected behavior.
361-383: Handling of malformed realtime events.Test confirms such events are not emitted to the normal subscription and instead go to the malformed subscription. No issues found.
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (6)
13-14: Constructor parameter update is consistent.Removing an explicit
isLastparameter in favor ofhasNextsimplifies the logic. This remains clear and maintainable.
43-43: InvertedisLastlogic is correct.
override var isLast: Bool { !_hasNext }properly mirrors thehasNextflag.
117-118: Pagination setup for partial results.
hasNext: truehelps test multi-page scenarios. This aligns with typical paginated flows.
128-131: Next page items updated.No issues in modifying these fields for test data.
139-142: Corrected message content.Updating serial and text for next-page items is straightforward; logic is unchanged.
148-149:hasNext: falsefinalizes pagination.Ensures the next page is accurately flagged as the last page.
Sources/AblyChat/DefaultMessages.swift (4)
9-11: MarkingARTMessageas @unchecked Sendable.This approach is valid for debugging and concurrency, but be wary that forced sendability can hide potential thread-safety concerns.
61-117: Potentially unhandled error throw in subscription callback.Rethrowing inside
Taskmight not surface in normal channel usage. Confirm whether these thrown errors need additional handling or logging beyond the debug log.
135-145: Debug-only malformed messages subscription.Implementation correctly isolates malformed message events under
#if DEBUG. This seems safe.
192-193: Update event listener added.Aligns with spec for resetting the subscription point on
updatewithresumed=false.Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (14)
9-9: Storing the optional channel state.
No issues found. This is a valid approach to store an optional channel state for test usage.
20-20: Introducing a typealias for clarity.
This typealias improves readability by giving a clear name to the callback type.
22-23: Nonisolated(unsafe) usage might allow unsynchronized access.
Consider clarifying concurrency usage or adding synchronization if multiple tasks modify these callbacks concurrently.
37-37: Additional initializer parameter.
Including an optional state parameter is useful. Ensure references tostateare consistent throughout the codebase.
41-41: Adding messageJSONToEmitOnSubscribe.
This new parameter seamlessly supports JSON-based message emission. Looks good.
45-45: Assignment of _state.
Straightforward assignment. No issues found.
49-49: Assignment of messageJSONToEmitOnSubscribe.
No immediate concerns. A clear way to store JSON messages for subscription.
83-87: Possible oversimplification in fallback state.
When_stateis nil, the code sets the channel to.attachedonly ifattachResultis.success; otherwise.failed. Consider whether intermediate states like.detachingor.suspendedmight be missed.
116-120: Performing state change callbacks.
Invoking bothstateCallbackandstateCallbacks[...]for the same event is fine, but ensure it doesn't cause duplicated effects if both are set.
152-152: Overloaded attach function.
Attaching without a callback is convenient. Ensure any side effects or concurrency issues are addressed when callingattach().
190-190: New JSON parameter.
Duplicated from the initializer; referencing it here as well ensures clarity.
206-221: Parsing JSON-based message data.
This approach is valid. Confirm that defaultingactionto.createon unknown raw values won't mask potential issues.
250-253: on(event:) callback registration.
Storing callbacks by event is a standard approach. Looks good.
255-257: on() callback registration.
Similarly, registering a single callback is concise.Tests/AblyChatTests/Helpers/Helpers.swift (3)
67-67: Added underlyingChannel parameter.
Useful for injecting a mock realtime channel for specialized testing. Nicely done.
75-75: Initializer assignment.
Assigning the underlyingChannel to the newly introduced property. Straightforward.
85-92: New waitForManager method.
This async helper method is a neat solution for ensuring manager side effects are processed before continuing. Be mindful of potential race conditions if multiple state changes happen concurrently.Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (5)
8-8: New optional underlyingChannel property.
Allows bridging the contributor channel with an actual mock realtime channel. Good for advanced testing scenarios.
18-18: New constructor parameter underlyingChannel.
Ensures that we can inject a mock channel when needed.
25-25: Assigning self.underlyingChannel.
Straightforward assignment. No issues found.
75-75: Calling attach on underlyingChannel.
Verify that the underlying channel’s attach logic is thread-safe, especially since this method is an async function in an actor.
86-86: Calling detach on underlyingChannel.
Same concurrency considerations as attach. Confirm no data races or re-entrancy issues.Tests/AblyChatTests/DefaultRoomPresenceTests.swift (1)
1-487: Comprehensive test coverage with well-structured test cases.The test suite thoroughly covers presence-related functionality including basic operations, error handling, state transitions, event subscriptions, and discontinuity handling. Each test is well-documented with spec references.
maratal
force-pushed
the
86-messages-tests
branch
from
ea960b3 to
e2188cc
Compare
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
maratal
force-pushed
the
86-messages-tests
branch
from
e2188cc to
3fafdf5
Compare
github-actions
bot
temporarily deployed
to
staging/pull/219/AblyChat
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently, please check "Code review limits" under "Moderation" settings.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
199-227:⚠️ Potential issueFix potential crash in timestamp parsing.
The timestamp parsing uses force unwrap which could crash if the timestamp string is invalid.
Apply this diff to safely handle timestamp parsing:
- if let ts = json["timestamp"] as? String { - message.timestamp = Date(timeIntervalSince1970: TimeInterval(ts)!) + if let ts = json["timestamp"] as? String, + let interval = TimeInterval(ts) { + message.timestamp = Date(timeIntervalSince1970: interval) }
🧹 Nitpick comments (4)
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)
51-66: Add trailing commas in multi-line collections.Add trailing commas after the last item in multi-line collections to maintain consistency with Swift style guidelines.
Apply this diff to fix the trailing commas:
"metadata": [ - "foo": "bar" + "foo": "bar", ], "extras": [ "headers": [ - "baz": "qux" + "baz": "qux", ] ]🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 58-58: Multi-line collection literals should have trailing commas
(trailing_comma)
[Error] 64-64: Multi-line collection literals should have trailing commas
(trailing_comma)
[Error] 65-65: Multi-line collection literals should have trailing commas
(trailing_comma)
Sources/AblyChat/DefaultRoomReactions.swift (1)
105-105: Remove trailing whitespace.Remove the trailing whitespace to maintain code cleanliness.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 105-105: Lines should not have trailing whitespace
(trailing_whitespace)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
135-146: Fix error message in detach callback handling.The error message incorrectly references "attachResult" instead of "detachResult".
Apply this diff to fix the error message:
guard let detachResult else { - fatalError("attachResult must be set before attach is called") + fatalError("detachResult must be set before detach is called") }Tests/AblyChatTests/DefaultMessagesTests.swift (1)
5-404: Consider general improvements to test suite reliability.To prevent similar issues in other test cases, consider implementing these improvements across the test suite:
- Create a base test class or helper methods to standardize state verification.
- Replace fixed delays with proper synchronization mechanisms.
- Add timeout handling for all async operations.
- Add retry mechanisms for flaky assertions.
Example helper methods to consider adding:
extension DefaultMessagesTests { func waitForState(_ state: ARTRealtimeChannelState, timeout: TimeInterval = 5.0) async throws { let expectation = AsyncExpectation() // Add state change listener and fulfill expectation try await expectation.wait(timeout: .seconds(timeout)) } func retryAssertion( timeout: TimeInterval = 5.0, interval: TimeInterval = 0.1, assertion: () async throws -> Void ) async throws { let deadline = Date().addingTimeInterval(timeout) var lastError: Error? while Date() < deadline { do { try await assertion() return } catch { lastError = error try await Task.sleep(nanoseconds: UInt64(interval * 1_000_000_000)) } } throw lastError ?? NSError(domain: "Assertion failed", code: -1) } }
🛑 Comments failed to post (3)
Tests/AblyChatTests/DefaultMessagesTests.swift (2)
166-213:
⚠️ Potential issueImprove UPDATE event handling and state verification.
The test is failing intermittently due to timing issues and lack of proper state verification. Consider these improvements:
- Add explicit state verification after the UPDATE event.
- Remove fixed delays and use proper synchronization.
- Add verification that the UPDATE event was actually processed.
@Test func whenChannelUPDATEReceivedWithResumedFalseThenSubscriptionPointResetsToAttachSerial() async throws { // Given let attachSerial = "attach123" let channelSerial = "channel456" let channel = MockRealtimeChannel(properties: ARTChannelProperties(attachSerial: attachSerial, channelSerial: channelSerial), state: .attached, attachResult: .success, detachResult: .success) - let contributor = RoomLifecycleHelper.createContributor(feature: .messages, underlyingChannel: channel, attachBehavior: .completeAndChangeState(.success, newState: .attached, delayInMilliseconds: RoomLifecycleHelper.fakeNetworkDelay * 2), detachBehavior: .completeAndChangeState(.success, newState: .detached, delayInMilliseconds: RoomLifecycleHelper.fakeNetworkDelay * 2)) + let contributor = RoomLifecycleHelper.createContributor(feature: .messages, underlyingChannel: channel, attachBehavior: .completeAndChangeState(.success, newState: .attached), detachBehavior: .completeAndChangeState(.success, newState: .detached)) let lifecycleManager = await RoomLifecycleManager.createManager(contributors: [contributor]) // ... setup code ... // Wait for room to become ATTACHED try await lifecycleManager.performAttachOperation(testsOnly_forcingOperationID: UUID()) + // Verify initial state + let roomStatusSubscription = await lifecycleManager.onRoomStatusChange(bufferingPolicy: .unbounded) + _ = try #require(await roomStatusSubscription.attachedElements().first { _ in true }) // ... test code ... // When: This contributor emits an UPDATE event with `resumed` flag set to false + let updateEventProcessed = AsyncExpectation() let contributorStateChange = ARTChannelStateChange( current: .attached, previous: .attached, event: .update, reason: ARTErrorInfo(domain: "SomeDomain", code: 123), resumed: false ) await RoomLifecycleHelper.waitForManager(lifecycleManager, toHandleContributorStateChange: contributorStateChange) { await contributor.channel.emitStateChange(contributorStateChange) + await updateEventProcessed.fulfill() } + // Verify UPDATE event was processed + try await updateEventProcessed.wait(timeout: .seconds(5)) // ... assertions ... }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func whenChannelUPDATEReceivedWithResumedFalseThenSubscriptionPointResetsToAttachSerial() async throws { // Given let attachSerial = "attach123" let channelSerial = "channel456" let channel = MockRealtimeChannel( properties: ARTChannelProperties(attachSerial: attachSerial, channelSerial: channelSerial), state: .attached, attachResult: .success, detachResult: .success ) let contributor = RoomLifecycleHelper.createContributor( feature: .messages, underlyingChannel: channel, attachBehavior: .completeAndChangeState(.success, newState: .attached), detachBehavior: .completeAndChangeState(.success, newState: .detached) ) let lifecycleManager = await RoomLifecycleManager.createManager(contributors: [contributor]) let realtime = MockRealtime.create { (MockHTTPPaginatedResponse.successSendMessageWithNoItems, nil) } let chatAPI = ChatAPI(realtime: realtime) let featureChannel = DefaultFeatureChannel(channel: channel, contributor: contributor, roomLifecycleManager: lifecycleManager) let defaultMessages = await DefaultMessages(featureChannel: featureChannel, chatAPI: chatAPI, roomID: "basketball", clientID: "clientId", logger: TestLogger()) // Wait for room to become ATTACHED try await lifecycleManager.performAttachOperation(testsOnly_forcingOperationID: UUID()) // Verify initial state let roomStatusSubscription = await lifecycleManager.onRoomStatusChange(bufferingPolicy: .unbounded) _ = try #require(await roomStatusSubscription.attachedElements().first { _ in true }) // When: subscription is added let subscription = try await defaultMessages.subscribe() // When: history get is called _ = try await subscription.getPreviousMessages(params: .init()) // Then: subscription point is the channelSerial let requestParams1 = try #require(realtime.requestArguments.first?.params) #expect(requestParams1["fromSerial"] == channelSerial) // When: This contributor emits an UPDATE event with `resumed` flag set to false let updateEventProcessed = AsyncExpectation() let contributorStateChange = ARTChannelStateChange( current: .attached, // arbitrary previous: .attached, // arbitrary event: .update, reason: ARTErrorInfo(domain: "SomeDomain", code: 123), // arbitrary resumed: false ) await RoomLifecycleHelper.waitForManager(lifecycleManager, toHandleContributorStateChange: contributorStateChange) { await contributor.channel.emitStateChange(contributorStateChange) await updateEventProcessed.fulfill() } // Verify UPDATE event was processed try await updateEventProcessed.wait(timeout: .seconds(5)) // When: history get is called _ = try await subscription.getPreviousMessages(params: .init()) // Then: The subscription point of any subscribers must be reset to the attachSerial let requestParams2 = try #require(realtime.requestArguments.last?.params) #expect(requestParams2["fromSerial"] == attachSerial) }
122-162:
⚠️ Potential issueAddress potential race conditions in the test.
The test is failing intermittently due to timing-sensitive operations. Consider these improvements to make the test more reliable:
- Add explicit state verification after each lifecycle operation.
- Use dynamic delays or proper synchronization mechanisms instead of fixed delays.
- Add retry mechanism for assertions that might be affected by timing.
@Test func whenChannelReentersATTACHEDWithResumedFalseThenSubscriptionPointResetsToAttachSerial() async throws { // Given let attachSerial = "attach123" let channelSerial = "channel456" let channel = MockRealtimeChannel(properties: ARTChannelProperties(attachSerial: attachSerial, channelSerial: channelSerial), state: .attached, attachResult: .success, detachResult: .success) - let contributor = RoomLifecycleHelper.createContributor(feature: .messages, underlyingChannel: channel, attachBehavior: .completeAndChangeState(.success, newState: .attached, delayInMilliseconds: RoomLifecycleHelper.fakeNetworkDelay * 2), detachBehavior: .completeAndChangeState(.success, newState: .detached, delayInMilliseconds: RoomLifecycleHelper.fakeNetworkDelay * 2)) + let contributor = RoomLifecycleHelper.createContributor(feature: .messages, underlyingChannel: channel, attachBehavior: .completeAndChangeState(.success, newState: .attached), detachBehavior: .completeAndChangeState(.success, newState: .detached)) let lifecycleManager = await RoomLifecycleHelper.createManager(contributors: [contributor]) // ... setup code ... // Wait for room to become ATTACHED try await lifecycleManager.performAttachOperation(testsOnly_forcingOperationID: UUID()) + // Verify channel state + let roomStatusSubscription = await lifecycleManager.onRoomStatusChange(bufferingPolicy: .unbounded) + _ = try #require(await roomStatusSubscription.attachedElements().first { _ in true }) // ... test code ... // Wait for room to become DETACHED try await lifecycleManager.performDetachOperation(testsOnly_forcingOperationID: UUID()) + // Verify channel state + _ = try #require(await roomStatusSubscription.detachedElements().first { _ in true }) // And then to become ATTACHED again try await lifecycleManager.performAttachOperation(testsOnly_forcingOperationID: UUID()) + // Verify channel state + _ = try #require(await roomStatusSubscription.attachedElements().first { _ in true }) // ... assertions ... }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func whenChannelReentersATTACHEDWithResumedFalseThenSubscriptionPointResetsToAttachSerial() async throws { // Given let attachSerial = "attach123" let channelSerial = "channel456" let channel = MockRealtimeChannel( properties: ARTChannelProperties(attachSerial: attachSerial, channelSerial: channelSerial), state: .attached, attachResult: .success, detachResult: .success ) - let contributor = RoomLifecycleHelper.createContributor(feature: .messages, underlyingChannel: channel, attachBehavior: .completeAndChangeState(.success, newState: .attached, delayInMilliseconds: RoomLifecycleHelper.fakeNetworkDelay * 2), detachBehavior: .completeAndChangeState(.success, newState: .detached, delayInMilliseconds: RoomLifecycleHelper.fakeNetworkDelay * 2)) + let contributor = RoomLifecycleHelper.createContributor(feature: .messages, underlyingChannel: channel, attachBehavior: .completeAndChangeState(.success, newState: .attached), detachBehavior: .completeAndChangeState(.success, newState: .detached)) let lifecycleManager = await RoomLifecycleHelper.createManager(contributors: [contributor]) let realtime = MockRealtime.create { (MockHTTPPaginatedResponse.successSendMessageWithNoItems, nil) } let chatAPI = ChatAPI(realtime: realtime) let featureChannel = DefaultFeatureChannel(channel: channel, contributor: contributor, roomLifecycleManager: lifecycleManager) let defaultMessages = await DefaultMessages(featureChannel: featureChannel, chatAPI: chatAPI, roomID: "basketball", clientID: "clientId", logger: TestLogger()) // Wait for room to become ATTACHED try await lifecycleManager.performAttachOperation(testsOnly_forcingOperationID: UUID()) + // Verify channel state after attach + let roomStatusSubscription = await lifecycleManager.onRoomStatusChange(bufferingPolicy: .unbounded) + _ = try #require(await roomStatusSubscription.attachedElements().first { _ in true }) // When: subscription is added let subscription = try await defaultMessages.subscribe() // When: history get is called _ = try await subscription.getPreviousMessages(params: .init()) // Then: subscription point is the channelSerial let requestParams1 = try #require(realtime.requestArguments.first?.params) #expect(requestParams1["fromSerial"] == channelSerial) // Wait for room to become DETACHED try await lifecycleManager.performDetachOperation(testsOnly_forcingOperationID: UUID()) + // Verify channel state after detach + _ = try #require(await roomStatusSubscription.detachedElements().first { _ in true }) // And then to become ATTACHED again try await lifecycleManager.performAttachOperation(testsOnly_forcingOperationID: UUID()) + // Verify channel state after re-attach + _ = try #require(await roomStatusSubscription.attachedElements().first { _ in true }) // When: history get is called _ = try await subscription.getPreviousMessages(params: .init()) // Then: The subscription point of any subscribers must be reset to the attachSerial let requestParams2 = try #require(realtime.requestArguments.last?.params) #expect(requestParams2["fromSerial"] == attachSerial) }Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
200-217:
⚠️ Potential issueHandle timestamp conversion safely.
The force unwrapping of the timestamp conversion could lead to runtime crashes if the timestamp string is invalid.
Apply this diff to handle the timestamp conversion safely:
- message.timestamp = Date(timeIntervalSince1970: TimeInterval(ts)!) + if let timeInterval = TimeInterval(ts) { + message.timestamp = Date(timeIntervalSince1970: timeInterval) + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if let json = messageJSONToEmitOnSubscribe { let message = ARTMessage(name: nil, data: json["data"] ?? "") if let action = json["action"] as? UInt { message.action = ARTMessageAction(rawValue: action) ?? .create } if let serial = json["serial"] as? String { message.serial = serial } if let clientId = json["clientId"] as? String { message.clientId = clientId } if let extras = json["extras"] as? ARTJsonCompatible { message.extras = extras } if let ts = json["timestamp"] as? String { - message.timestamp = Date(timeIntervalSince1970: TimeInterval(ts)!) + if let timeInterval = TimeInterval(ts) { + message.timestamp = Date(timeIntervalSince1970: timeInterval) + } } callback(message)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)
79-80: Track untestable specification.The comment indicates that specification CHA-ER4c is currently untestable. This should be tracked to ensure it gets addressed in the future.
Would you like me to create an issue to track the untestable specification CHA-ER4c?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Sources/AblyChat/DefaultRoomReactions.swift(2 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/AblyChat/DefaultRoomReactions.swift
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode, macOS (Xcode 16)
🔇 Additional comments (2)
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (2)
46-77: LGTM! Test improvements enhance clarity and validation.The renamed test method and enhanced validation logic provide better clarity and more thorough testing. The detailed JSON structure for simulating room reaction messages is well-structured and comprehensive.
81-96: LGTM! Well-structured test for malformed message handling.The test effectively validates that malformed messages are properly handled and don't propagate to regular subscribers. The test structure follows the AAA pattern and provides clear validation.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (25)
CHANGELOG.md (1)
3-19: LGTM, but consider addressing formatting inconsistenciesThe changelog updates correctly document the new features (updating and deleting messages) and breaking changes (renaming
ClientOptionstoChatClientOptions).Consider addressing the following formatting issues:
- Line 14: Heading level should increment by one (h3 instead of h4)
- Lines 16, 18, 24, 26: Bare URLs should be formatted as proper markdown links
- Line 22: Duplicate heading content (same as line 5)
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
14-14: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4(MD001, heading-increment)
16-16: Bare URL used
null(MD034, no-bare-urls)
18-18: Bare URL used
null(MD034, no-bare-urls)
Example/AblyChatExample/MessageViews/PresenceMessageView.swift (1)
25-28: Consider optional unwrapping with nil coalescing for safer code.The optional unwrapping with force unwrapping (
status!) could be replaced with nil coalescing for a more robust implementation.- let presenceMessage = status != nil ? "\(clientPresenceChangeMessage) with status: \(status!)" : clientPresenceChangeMessage + let presenceMessage = status.map { "\(clientPresenceChangeMessage) with status: \($0)" } ?? clientPresenceChangeMessageExample/AblyChatExample/MessageViews/MessageView.swift (1)
27-45: Consider extracting the confirmation dialog to improve readabilityThe confirmation dialog logic could be extracted into a separate view or view modifier to make the main view more concise and focused solely on presentation.
Sources/AblyChat/Version.swift (1)
3-9: Update the TODO commentThe TODO comment about copying chat-js implementation is no longer relevant as you've now implemented a custom solution with the dictionary approach.
-// TODO: Just copied chat-js implementation for now to send up agent info. https://github.com/ably-labs/ably-chat-swift/issues/76 +// Agent information to identify this library when communicating with AblyTests/AblyChatTests/IntegrationTests.swift (1)
50-50: Consider implications of enabling trace logging.Introducing
ChatClientOptions(logHandler: ChatLogger(label: loggingLabel), logLevel: .trace)might add significant overhead in production environments due to verbose logging. If this is only for debugging, ensure it’s disabled in release builds.Sources/AblyChat/Messages.swift (2)
85-85: Using an existential forchannelis flexible but can have a performance trade-off.Changing
var channel: RealtimeChannelProtocoltovar channel: any RealtimeChannelProtocolwidens the type, which is useful for dynamic dispatch. Keep in mind this introduces a slight runtime overhead compared to a concrete type. This is likely fine if you need that flexibility.
141-189: New parameter structs enhance clarity of message operations.The
UpdateMessageParamsandDeleteMessageParamsstructures encapsulate operation-specific data, reducing confusion over method signatures. Consider adding test coverage for edge cases (e.g., empty metadata) if not already covered.Sources/AblyChat/DefaultMessages.swift (1)
319-330: Safely convertingARTMessageOperationtoMessageOperation.This extension enforces a non-nil
clientIdto avoid hidden crashes. Throwing an error on nilclientIdis reasonable for data integrity. Confirm through tests that any edge cases (e.g., partial fields) are handled gracefully.Example/AblyChatExample/ContentView.swift (7)
19-20: Consider customizingChatClientOptionsfor the mock environment.
Providing mock-specific configurations (e.g., logging level, or dummy credentials) can enhance test fidelity.- return MockChatClient( - realtime: MockRealtime(), - clientOptions: ChatClientOptions() - ) + let mockOptions = ChatClientOptions() + // e.g., mockOptions.logLevel = .verbose + return MockChatClient( + realtime: MockRealtime(), + clientOptions: mockOptions + )
80-87: Gracefully handle whitespace-only inputs.
Currently, the logic switches to a reaction ifnewMessageis empty. However, whitespace-only inputs also appear empty to users but are not strictly"". You might trim whitespace to provide a consistent experience.- if newMessage.isEmpty { + if newMessage.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { ... }
103-129: Reverse-chronological insertion may confuse users.
New.createmessages are inserted at index 0, while older messages are appended. This ensures new content shows up at the top, but you might confirm that this is the desired chat ordering, as many chat apps place new messages at the bottom.
135-149: Throttle typing event triggers.
CallingstartTyping()orstopTyping()on every character change may overload the server. A short debounce or throttling interval can reduce excess network traffic.
165-172: Add accessibility label to the cancel-edit button.
Providing a label or text ensures that screen readers can identify this control properly.Button("", systemImage: "xmark.circle.fill") { + Text("Cancel edit") editingItemID = nil newMessage = "" }
303-311: Confirm that user presence updates are shown in the intended order.
Presence events are prepended to the list. If real-time presence updates should appear chronologically, inserting at position 0 might reverse that.
417-421: Consider error handling for failed typing updates.
IfstartTyping()orstopTyping()fails, the current code quietly returns. Logging or indicating an error might help diagnose issues with typing signals.Sources/AblyChat/Dependencies.swift (1)
12-13: Elaborate on concurrency safety.
The protocol inheritsSendable, so document any concurrency guarantees or restrictions forRealtimeClientProtocol.Sources/AblyChat/ChatAPI.swift (3)
64-78: Use consistent naming forversionfields.
You are repurposingresponse.serialas theversionin the newMessage. Consider naming consistency across the server and client to reduce confusion.
132-170: Consider using HTTP DELETE method for deletions.
You're using aPOSTrequest to an endpoint named.../delete. This is valid if your API mandates it, but using DELETE might be more semantically consistent.
187-189: Add the same error handling comment toupdateMessageanddeleteMessage.
The comment explicitly mentions “send call”. Making it generic for update/delete calls clarifies that same error path is used for all operations.- // If an error is returned from the REST API, its ErrorInfo representation shall be thrown as the result of the send call. + // If an error is returned from the REST API, its ErrorInfo representation shall be thrown as the result of the send, update, or delete calls.Sources/AblyChat/Message.swift (1)
85-101: New message version fields
Addingversion,timestamp, andoperationhelps track modifications to a message. Consider clarifying in doc comments how this newtimestampdiffers fromcreatedAt.Example/AblyChatExample/Mocks/MockClients.swift (2)
138-145: Sending messages
Settingversionandtimestampto current values is fine for mock usage. Consider generating more realistic unique versions if needed.
165-181: Ignoringparamsindelete
paramsmight carry important details for deletion logic, but it’s not reflected in the resultingMessage. Consider whether you need to add or log it inoperationfor completeness.Example/AblyChatExample/Mocks/MockRealtime.swift (1)
251-345: Good addition of the RealtimePresence implementation.The new
RealtimePresenceclass implements the necessary protocol methods, most of which throw "Not implemented" errors as placeholder stubs. This is appropriate for a mock class that's under development.Note that the
historymethod on line 340 is implemented as a no-op (empty method) rather than throwing an error. Consider documenting why this method is implemented differently than the others or make it consistent with the other methods.Tests/AblyChatTests/DefaultMessagesTests.swift (2)
53-54: Consider using a more specific error!While the test is correct, using a more specific error domain and code that matches actual API errors would make the test more realistic and valuable.
- let sendError = ARTErrorInfo(domain: "SomeDomain", code: 123) + let sendError = ARTErrorInfo(domain: "io.ably.error", code: 40000) // Using Ably's error domain and a realistic error code
359-361: Fix the incorrect spec reference in the comment!The comment indicates that the spec reference should be CHA-M4k, not CHA-M5k.
- // Wrong name, should be CHA-M4k - // @spec CHA-M5k + // @spec CHA-M4k
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
AblyChat-Package.xctestplan(1 hunks)AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)CHANGELOG.md(1 hunks)Example/AblyChatExample/ContentView.swift(10 hunks)Example/AblyChatExample/MessageViews/DeletedMessageView.swift(1 hunks)Example/AblyChatExample/MessageViews/MenuButtonView.swift(1 hunks)Example/AblyChatExample/MessageViews/MessageView.swift(1 hunks)Example/AblyChatExample/MessageViews/PresenceMessageView.swift(1 hunks)Example/AblyChatExample/Mocks/Misc.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(9 hunks)Example/AblyChatExample/Mocks/MockRealtime.swift(3 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)README.md(1 hunks)Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift(1 hunks)Sources/AblyChat/ChatAPI.swift(3 hunks)Sources/AblyChat/ChatClient.swift(4 hunks)Sources/AblyChat/DefaultMessages.swift(6 hunks)Sources/AblyChat/Dependencies.swift(1 hunks)Sources/AblyChat/Events.swift(1 hunks)Sources/AblyChat/Message.swift(4 hunks)Sources/AblyChat/Messages.swift(2 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Sources/AblyChat/Room.swift(2 hunks)Sources/AblyChat/RoomFeature.swift(2 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/Rooms.swift(3 hunks)Sources/AblyChat/Typing.swift(1 hunks)Sources/AblyChat/Version.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(8 hunks)Tests/AblyChatTests/DefaultChatClientTests.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(4 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(3 hunks)Tests/AblyChatTests/DefaultRoomTests.swift(11 hunks)Tests/AblyChatTests/DefaultRoomTypingTests.swift(3 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift(10 hunks)Tests/AblyChatTests/IntegrationTests.swift(2 hunks)Tests/AblyChatTests/MessageSubscriptionTests.swift(1 hunks)Tests/AblyChatTests/MessageTests.swift(4 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(1 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(5 hunks)Tests/AblyChatTests/Mocks/MockRealtime.swift(3 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(7 hunks)
✅ Files skipped from review due to trivial changes (4)
- README.md
- AblyChat-Package.xctestplan
- Tests/AblyChatTests/DefaultRoomTests.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/AblyChatTests/DefaultRoomTypingTests.swift
- Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~24-~24: Although a hyphen is possible, it is not necessary in a compound modifier in which the first word is an adverb that ends in ‘ly’.
Context: ...ed This release reverts the pinning of ably-cocoa that was introduced in version 0.1.1 (h...
(HYPHENATED_LY_ADVERB_ADJECTIVE)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
14-14: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
16-16: Bare URL used
null
(MD034, no-bare-urls)
18-18: Bare URL used
null
(MD034, no-bare-urls)
22-22: Multiple headings with the same content
null
(MD024, no-duplicate-heading)
24-24: Bare URL used
null
(MD034, no-bare-urls)
26-26: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: SPM (Xcode 16)
🔇 Additional comments (104)
Package.swift (1)
23-23: Updated dependency specification for ably-cocoa.The dependency has been updated from using an exact version (1.2.36) to a range that allows any version from 1.2.40 and above. This change provides more flexibility for future updates while maintaining a minimum compatible version.
Package.resolved (1)
2-2: Package.resolved updated to match new ably-cocoa dependency.The Package.resolved file has been correctly updated to reflect the dependency change to ably-cocoa version 1.2.40. The originHash has been updated accordingly.
Also applies to: 9-10
AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
2-2: Workspace Package.resolved updated to match new ably-cocoa dependency.The workspace Package.resolved file has been correctly updated to reflect the dependency change to ably-cocoa version 1.2.40. The originHash has been updated accordingly.
Also applies to: 9-10
Sources/AblyChat/Occupancy.swift (1)
37-37: Protocol update to match Swift's explicit handling of existential typesThe change from
RealtimeChannelProtocoltoany RealtimeChannelProtocolfollows Swift's evolution toward more explicit handling of existential types, making the code more future-proof.Sources/AblyChat/Typing.swift (1)
57-57: Protocol update to match Swift's explicit handling of existential typesThe change from
RealtimeChannelProtocoltoany RealtimeChannelProtocolfollows Swift's evolution toward more explicit handling of existential types, making the code more future-proof.Sources/AblyChat/RoomReactions.swift (1)
25-25: Protocol update to match Swift's explicit handling of existential typesThe change from
RealtimeChannelProtocoltoany RealtimeChannelProtocolfollows Swift's evolution toward more explicit handling of existential types, making the code more future-proof.Example/AblyChatExample/MessageViews/DeletedMessageView.swift (2)
3-23: Clean implementation of DeletedMessageView.The view structure is well-designed with appropriate visual hierarchy for displaying deleted messages. Good use of SwiftUI styling for differentiating the client ID and the deletion message.
19-21: Good platform-specific customization.Appropriate use of conditional compilation to hide the list row separator on non-tvOS platforms, ensuring consistent appearance across different Apple platforms.
Example/AblyChatExample/MessageViews/MenuButtonView.swift (2)
3-20: Well-structured MenuButtonView implementation.Clean implementation of a reusable menu component with appropriate actions for editing and deleting messages. Good use of SF Symbols for visual consistency.
13-15: Appropriate role assignment for destructive action.The delete button is correctly configured with the
.destructiverole, providing users with a visual cue about the nature of this action.Sources/AblyChat/Events.swift (3)
11-12: Appropriate extension of MessageAction enum.The addition of
updateanddeletecases aligns with the PR objective of enhancing message handling capabilities.
18-21: Comprehensive handling of Realtime actions.The
fromRealtimeActionmethod has been properly updated to handle the new message action types, ensuring consistency between the application's enum and the underlying Ably library's actions.
23-24: Well-maintained ignored actions list.The list of actions to ignore has been correctly updated to remove the now-handled actions, leaving only truly ignored cases.
Example/AblyChatExample/Mocks/Misc.swift (1)
21-24: Updated Message initialization with new required properties.The mock implementation has been properly updated to include the new properties required by the Message model: version, timestamp, and operation.
Tests/AblyChatTests/ChatAPITests.swift (3)
11-13: UpdatedMockRealtimeinstantiation.The code has been updated to use the new
MockRealtimeinitialization pattern, which appears to be a consistent change across the test suite.
50-52: Addedversionandtimestampproperties toMessageinstantiation.These new properties (
versionandtimestamp) have been added to align with an update to theMessagemodel structure. Theversionis set to match the message serial, maintaining data consistency.
148-150: Addedversionandtimestampproperties to multipleMessageobjects.The expected test message objects have been properly updated to include the new required properties. The implementation correctly maps the timestamp to match the creation date when available, and sets it to nil when the creation date is nil.
Also applies to: 160-162
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (2)
10-19: UpdatedMockRealtimeinstantiation with closure.The code has been updated to use the new
MockRealtimeinitialization pattern with a closure that returns the test response data. This change is consistent with updates in other test files.
38-38: SimplifiedMockRealtimeinstantiation.The
MockRealtimeinitialization has been simplified to use the default constructor since these tests don't require custom HTTP responses. This makes the code cleaner while maintaining test functionality.Also applies to: 60-60
Tests/AblyChatTests/MessageSubscriptionTests.swift (1)
29-29: UpdatedMessageinstantiation with new required properties.The
Messageinitialization has been updated to include the newversionandtimestampproperties, maintaining compatibility with the updatedMessagestruct. This change ensures consistent use of theMessagemodel across the test suite.Example/AblyChatExample/MessageViews/PresenceMessageView.swift (1)
4-34: New view for displaying presence messages.The implementation looks clean and follows SwiftUI best practices. The
generatePresenceMessage()function effectively constructs a human-readable message based on presence data.Example/AblyChatExample/MessageViews/MessageView.swift (2)
4-51: Well-structured SwiftUI view with good separation of concernsThis is a clean implementation of a message view that properly handles both sender and receiver message presentations. The view appropriately manages state for editing and deletion confirmation.
53-56: Good use of a dedicated model struct for view dataCreating a separate
MessageListItemstruct to wrap the message with presentation metadata is a good practice that separates view concerns from data model concerns.Tests/AblyChatTests/MessageTests.swift (3)
13-15: Consistent addition of new Message properties in testsThe new
headers,version, andtimestampproperties have been added to all test fixtures, maintaining test consistency with updated model properties.
39-41: Good test coverage for invalid scenarioSetting the version to "invalid" for the invalidMessage test fixture helps ensure the system handles malformed data appropriately.
85-87: Maintain consistency in test fixturesAll message instances now properly include the new properties required by the Message struct, ensuring tests remain valid with the updated model.
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (3)
5-5: Updated to use explicit existential type syntaxThe change to
any RealtimeChannelProtocolreflects modern Swift practices for working with protocol types.
10-10: Consistent use of explicit existential type in initializerThe initializer parameter has been properly updated to match the property declaration, maintaining consistency throughout the class.
4-32:✅ Verification successful
Verify consistent usage of existential types across codebase
Ensure that all other implementations using
RealtimeChannelProtocolhave been similarly updated to use theanykeyword for consistency.
🏁 Script executed:
#!/bin/bash # Check for any remaining instances of RealtimeChannelProtocol without 'any' rg "RealtimeChannelProtocol" --type swift -g "!*.generated.swift" | grep -v "any RealtimeChannelProtocol" | grep -v "protocol RealtimeChannelProtocol"Length of output: 1833
Existential types usage verified.
After reviewing the code, theMockFeatureChannelfile correctly declares its property asany RealtimeChannelProtocol. The grep results confirm that other references toRealtimeChannelProtocol—such as in class conformances, type constraints, and comments—are used correctly without theanykeyword. This usage distinction is intentional and aligns with Swift’s language rules regarding existentials versus protocol conformances.Sources/AblyChat/Version.swift (3)
7-7: Version bump from 0.1.1 to 0.2.0The version increment follows semantic versioning principles, indicating significant new functionality without breaking changes.
9-9: Improved structure for agent informationChanging from a string-based approach to a dictionary makes the agent information more structured and extensible for future additions.
3-10:❓ Verification inconclusive
Verify usage of the new agents dictionary throughout the codebase
Since the previous
defaultChannelParamsvariable has been removed, ensure all code that used it has been updated to use the newagentsdictionary.
🏁 Script executed:
#!/bin/bash # Check for any remaining references to defaultChannelParams rg "defaultChannelParams" --type swift # Check how the new agents dictionary is used rg "agents\[" --type swiftLength of output: 67
Agents Dictionary Migration Verification
- Automated searches did not find any occurrences of the old
defaultChannelParamsin the Swift codebase.- The new
agentsdictionary declared inSources/AblyChat/Version.swiftappears to be in place.- However, since the search commands returned no output, please manually verify that no indirect usages of
defaultChannelParamsexist and that all references now correctly use the newagentsdictionary.Sources/AblyChat/RoomFeature.swift (2)
49-49: Updated property to use 'any' keyword for protocol type.The code has been updated to use the modern Swift syntax for existential types, adding the
anykeyword beforeRealtimeChannelProtocol. This change makes the protocol usage more explicit without changing the functionality.
65-65: Consistent protocol syntax update.Updating the implementation to match the protocol declaration with the
anykeyword ensures consistency across the codebase.Sources/AblyChat/Room.swift (2)
303-303: Added 'any' keyword for existential type.This change follows the same pattern as seen in RoomFeature.swift, using the
anykeyword to explicitly mark the protocol type as existential.
318-318: Changed options initialization to use direct Ably types.Two changes here:
- Changed from
vartolet- this is good as it promotes immutability- Changed from
RealtimeChannelOptionstoARTRealtimeChannelOptions- using the direct Ably type instead of a wrapperBoth changes improve the code quality.
Sources/AblyChat/Rooms.swift (3)
50-50: Updated return type from 'ClientOptions' to 'ChatClientOptions'.This type name change makes the purpose more explicit and specific to chat functionality, improving code readability.
63-63: Updated property type for consistency.Property type has been updated from
ClientOptionstoChatClientOptionsto match the protocol definition.
118-118: Updated constructor parameter type.The parameter type change ensures consistency with the property declaration and protocol changes.
Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift (5)
5-6: Added protocol conformance for wrapper class.New extension added to make
ARTWrapperSDKProxyRealtimeconform toRealtimeClientProtocol, ensuring consistent interfaces across both direct and proxy classes.
8-8: Added protocol conformance for channels wrapper class.Similar to the previous change, this extends protocol conformance to the wrapper class.
11-12: Added protocol conformance for channel wrapper.This ensures that the wrapper implementation for realtime channels can be used interchangeably with the direct implementation.
13-14: Added protocol conformance for presence classes.Both the direct
ARTRealtimePresenceand its wrapper now conform toRealtimePresenceProtocol, completing the protocol conformance pattern used throughout the file.
3-4:✅ Verification successful
Changed ARTRealtime protocol conformance.
The protocol conformance has been updated from
RealtimeClientProtocoltoSuppliedRealtimeClientProtocol. This appears to be part of a broader architectural change in the protocol hierarchy.
🏁 Script executed:
#!/bin/bash # Check definition of the new protocol to ensure ARTRealtime still satisfies all required functionality rg -A 10 "protocol SuppliedRealtimeClientProtocol" --type swiftLength of output: 1004
Protocol conformance update verified
The updated conforming declaration in
Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swiftnow usesSuppliedRealtimeClientProtocol, which has been confirmed by the search. The protocol definition inSources/AblyChat/Dependencies.swiftclearly shows thatSuppliedRealtimeClientProtocolextendsRealtimeClientProtocoland requires thecreateWrapperSDKProxy(with:)method. Assuming thatARTRealtimeeither implements or inherits the necessary functionality (as the build currently passes), this change appears valid and in-line with the broader architectural update.Tests/AblyChatTests/IntegrationTests.swift (1)
162-218: Be mindful of potential race conditions in async message flow.These added test steps for editing and deleting messages ensure correct client-server round-trip validation. However, relying on immediate subscription events in concurrent environments can be a source of flakiness. Consider adding small timeouts or retry logic if you notice intermittent CI failures, and ensure that the subscription receives events when system load is high.
Sources/AblyChat/Messages.swift (2)
43-43: Doc comment for send method looks good.The note clarifies that the returned message uses
.createaction. No further issues identified.
49-79: New 'update' and 'delete' doc comments are clear.Documentation for
updateanddeletethoroughly explains parameter usage. The highlight on returning a message with.updateor.deleteaction is very helpful.Sources/AblyChat/DefaultMessages.swift (3)
9-11: Debug-only concurrency marking.Marking
ARTMessageas@unchecked Sendablein debug builds can be useful for testing. However, be aware that it bypasses some thread-safety checks, so guard carefully against concurrency bugs.
62-125: Structured error handling for malformed messages.The do-catch flow for parsing incoming realtime messages is an improvement over silent guards. Emitting malformed events in debug mode is helpful for diagnosing issues. Ensure that production builds do not inadvertently expose these malformed events or logs to end users.
208-208: RESET logic on channel UPDATE with resumed=false is appropriate.Resetting the subscription point for all listeners aligns with the requirement to handle missed messages when the channel transitions without resume. This prevents message gaps. Continue monitoring potential timing or concurrency issues in high-latency environments.
Example/AblyChatExample/ContentView.swift (2)
55-70: Ensure ID uniqueness for presence events.
Usingtimestamp.descriptionas an ID may result in collisions if multiple events share the same timestamp. Consider a combination of timestamp plus a random UUID or an event-specific ID to avoid potential collisions.
246-280: Validate message ordering for previously fetched messages vs. newly created ones.
Older messages are appended, while newly created messages are prepended, potentially mixing chronological order. Ensure it matches the intended user experience.Sources/AblyChat/Dependencies.swift (2)
6-10: Specify whethercreateWrapperSDKProxyrequires error handling.
IfcreateWrapperSDKProxycan fail for certain inputs, consider handling or throwing an error. Otherwise, confirming it always succeeds clarifies usage.
29-33: Double-check presence concurrency logic.
You’ve added an associated typePresencetoRealtimeChannelProtocol. Ensure presence operations remain thread-safe, especially if multiple tasks modify presence concurrently.Sources/AblyChat/ChatAPI.swift (2)
28-40: Confirm required JSON keys from the backend.
MessageOperationResponseassumesversionandtimestampkeys exist. This is acceptable, but ensure your server always returns them, or handle missing fields gracefully.
80-130: Handle partial updates carefully.
When updating a message, unspecified fields are removed. Confirm that client logic or UI accounts for the possible removal of optional fields (e.g.,headers).Tests/AblyChatTests/DefaultChatClientTests.swift (5)
8-11: Looks good
The updated initialization withMockRealtime(createWrapperSDKProxyReturnValue: .init())is correct and aligns well with the intent to test default client options behavior when none are provided.
14-14: No concerns
Defininglet defaultOptions = ChatClientOptions()to validate the fallback logic is consistent with the rest of the codebase.
18-27: New test method is correct
Verifies thatclient.realtimecorrectly references the passed-in instance rather than a proxy.
32-38: No issues found
The assertion confirming that the proxy is created with the expected agents ensures proper agent injection.
44-54: Rooms property test
These assertions confirm thatroomspoints to the wrapper SDK proxy client. Implementation looks consistent.Sources/AblyChat/Message.swift (5)
13-17: Definition forOperationMetadata
Introduces a clear alias for operation-related metadata, aligning well with the rest of the message model.
102-114: Expanded initializer
Properly sets the new fields. This aligns with how versioning and operations are being handled in other parts of the code.
116-139: Introduction ofcopymethod
Allows for selectively updating a subset of fields while retaining the rest. Straightforward and readable.
142-151:MessageOperationstruct
Defines a simple structure for describing who performed an operation, optional descriptions, and related metadata.
156-175: JSON decoding for new properties
Parsingversion,timestamp, andoperationfrom the JSON object appears correct. Verify how missing or invalid values for these fields are handled in edge cases.Example/AblyChatExample/Mocks/MockClients.swift (11)
6-6: Switched to ChatClientOptions
Enhances consistency across the mock and main code.
10-10: Updated init method
Falls back to a defaultChatClientOptions()if none is provided. Logical and clean.
18-18: clientID fallback
Using"AblyTest"as a fallback in mocks is reasonable for testing scenarios.
23-23: Added chat client options property
Matches the other code changes for moving fromClientOptionstoChatClientOptions.
39-39: Initialization
Storing the providedclientOptionsensures eachMockRoomsinstance aligns with the real environment’s setup.
91-91: Switchedchannelto an existential
Leveragingany RealtimeChannelProtocolaffords greater flexibility in mocking.
111-115: Mock message creation
Populatingheaders,version,timestamp, andoperationaligns with the updatedMessagestructure for consistency.
191-191: Channel existential
Switching toany RealtimeChannelProtocolis consistent with your approach elsewhere.
211-211: Random interval range changed
Expanding to(0.3 ... 0.6)looks fine, just ensures slightly longer intervals in subscription mock generation.
238-238: Channel existential
Reinforces consistent usage of existential channels across mocks.
401-401: Channel existential
Allows diverse channel conformances for occupancy handling without losing type safety.Example/AblyChatExample/Mocks/MockRealtime.swift (2)
12-14: Good improvement on the clientId implementation.Returning a static string instead of throwing a fatal error is a better approach for a mock implementation as it provides a predictable value for testing without crashing.
108-108: Appropriate implementation of the presence property.Replacing the computed property that previously raised a fatal error with a concrete instance of
RealtimePresenceis a good change for testing as it provides an actual implementation to work with.Sources/AblyChat/ChatClient.swift (5)
38-38: Improved type naming with ChatClientOptions.Changing from
ClientOptionstoChatClientOptionsis a good improvement for clarity, making it explicit that these options are specifically for the chat functionality.
47-48: Appropriate use of nonisolated for frequently accessed properties.Marking
realtimeandclientOptionsasnonisolatedallows them to be accessed from outside the actor without awaiting, which is suitable for properties that are frequently accessed and don't change after initialization.
63-68: Enhanced initialization with wrapper SDK proxy support.The updated initializer now properly uses the
SuppliedRealtimeClientProtocolto create a wrapper SDK proxy with the agents configuration. This change allows for more flexibility in how the realtime client is created and configured.
75-80: Better error handling for clientID.The improved implementation provides a clear error message when the clientId is missing, which will help developers identify and fix the issue more quickly.
86-86: Consistent renaming to ChatClientOptions.The struct has been renamed from
ClientOptionstoChatClientOptionsalong with its usage in the equality comparison method, maintaining consistency throughout the codebase.Also applies to: 107-107
Tests/AblyChatTests/Mocks/MockRealtime.swift (5)
6-6: Updated protocol conformance for wrapper SDK support.The class now conforms to
SuppliedRealtimeClientProtocolinstead ofRealtimeClientProtocol, which is necessary to support the wrapper SDK proxy functionality.
10-10: Added properties for wrapper SDK proxy testing.The addition of
createWrapperSDKProxyReturnValueand_createWrapperSDKProxyOptionsArgumentproperties enables testing of the wrapper SDK proxy functionality by allowing tests to control the returned value and inspect the passed options.Also applies to: 16-16
29-36: Updated initializer to support wrapper SDK proxy testing.The initializer now accepts a
createWrapperSDKProxyReturnValueparameter, allowing tests to specify what value should be returned by thecreateWrapperSDKProxymethod.
81-91: Well-implemented createWrapperSDKProxy method.This method properly implements the requirement from
SuppliedRealtimeClientProtocol, with good error handling when the return value is not set. It also stores the options argument for later inspection.
93-99: Thread-safe access to options argument.The computed property provides thread-safe access to the
_createWrapperSDKProxyOptionsArgumentproperty by using a mutex lock, which is a good practice for properties that might be accessed from multiple threads.Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (8)
9-9: Enhanced state management capabilities.The addition of the
_stateproperty and updatedstatecomputed property allows for more flexible control over the channel state during tests, which is valuable for testing different state scenarios.Also applies to: 47-48, 86-90
13-13: Improved type specificity for presence property.Using
some RealtimePresenceProtocolinstead of the less specific type provides better type information and is a good Swift practice.
20-24: Good addition of callback storage for state changes.The new typealias and callback properties provide a structured way to store and manage callbacks for channel state changes, which is essential for testing event handling.
32-33: Enhanced MessageToEmit struct with operation and version.Adding
operationandversionproperties to theMessageToEmitstruct allows for more detailed message configuration, which is helpful for comprehensive testing.
119-148: Well-structured state change callback methods.The new methods for handling state change callbacks are well-organized and handle the different scenarios (attach/detach success/failure) appropriately. The use of
@MainActorensures that UI updates are performed on the main thread.
163-166: Proper asynchronous execution of state callbacks.Using
Taskto asynchronously execute the state callbacks after the attach/detach operations is a good approach that better simulates the real-world behavior of these operations.Also applies to: 185-188
191-192: Added support for JSON-based message data.The new
messageJSONToEmitOnSubscribeproperty and corresponding implementation in thesubscribemethod provide a more flexible way to define test messages using JSON data.Also applies to: 202-229
257-265: Improved event listener implementation.The updated
onmethods now properly store callbacks in the appropriate collection, allowing them to be invoked later when state changes occur, which is crucial for testing event handling.Tests/AblyChatTests/DefaultMessagesTests.swift (7)
10-24: Test implementation looks good!This new test correctly verifies that clients can send messages via the REST Chat API. The test structure follows the Arrange-Act-Assert pattern, properly mocks dependencies, and has an appropriate assertion.
26-47: Clear test for payload structure verification!This test effectively verifies that metadata and headers are properly omitted from REST payloads when not specified by the user.
88-118: Test implementation looks good!This new test properly verifies the behavior of subscription points when the underlying realtime channel is not in the ATTACHED state. The test setup correctly simulates the ATTACHING state and the assertions validate the expected behavior.
215-251: Test implementation is thorough and clear!This test effectively verifies that subscription.getPreviousMessages:
- Uses the correct subscription point
- Forces direction to "backwards" regardless of input
- Properly handles pagination
Good use of multiple assertions to verify different aspects of the behavior.
362-384: Well-structured test for malformed event handling!The test correctly verifies that malformed events are not passed to regular subscribers but are emitted to the malformed message subscribers. The test setup with a malformed JSON message is particularly effective.
317-319: Consider adding tests for the untestable specs if possible!The comments indicate that CHA-M4b and CHA-M4d are currently untestable. Consider if there are alternative approaches to verify this functionality, such as:
- Testing memory management with a weak reference
- Using reflection or debug-only features for verification
- Restructuring the code to make these aspects testable
120-162:❓ Verification inconclusive
Investigate potential race condition in failing test!
This test is reported as failing 14-18% locally and >50% in CI according to PR comments. The test logic is sound, but the intermittent failures suggest a potential race condition in the test or implementation.
The failure could be due to timing issues between the channel state transitions and checking the subscription point. Consider:
- Adding explicit awaits or delays between state transitions
- Adding logging to track the exact sequence of events
- Adding more deterministic synchronization points
🏁 Script executed:
#!/bin/bash # Look for any potential race conditions or timing issues in DefaultMessages implementation rg -A 10 -B 10 "resetSubscriptionPoint|updateSubscriptionPoint" Sources/Length of output: 74
Action Required: Investigate and Mitigate Potential Race Conditions in Subscription State Updates
While intermittent failures—with a 14–18% local and >50% CI failure rate—indicate a timing sensitivity between channel state transitions and the subscription point check, our search for functions explicitly named like
resetSubscriptionPointorupdateSubscriptionPointin the Sources directory returned no clues. This lack of explicit evidence makes it hard to pinpoint the root cause automatically, so please verify that the asynchronous operations around attach/detach transitions are reliably synchronized.Consider the following improvements:
- Introduce explicit awaits or brief delays between state transitions to ensure the channel state is fully updated.
- Add detailed logging to trace the sequence of events and verify that the subscription point is updated as expected.
- Enhance synchronization mechanisms (or introduce additional deterministic checkpoints) to avoid potential race conditions.
maratal
force-pushed
the
140-typing-tests
branch
from
3e11ea0 to
cce8978
Compare
maratal
force-pushed
the
86-messages-tests
branch
from
2aeac57 to
5bf2afd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
359-384: Test for malformed event handling.This test ensures that malformed realtime events are not propagated to subscribers but are captured by the specialized subscription method. There appears to be a discrepancy in the spec reference (@SPEC CHA-M5k vs comment that mentions it should be CHA-M4k).
There's a minor inconsistency in the test annotation. The comment indicates the spec should be CHA-M4k, but the actual annotation is @SPEC CHA-M5k. Consider updating the annotation to match the correct specification reference.
-// Wrong name, should be CHA-M4k -// @spec CHA-M5k +// @spec CHA-M4k
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Sources/AblyChat/DefaultMessages.swift(4 hunks)Sources/AblyChat/DefaultRoomReactions.swift(2 hunks)Tests/AblyChatTests/ChatAPITests.swift(0 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(4 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(2 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(4 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(8 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift(5 hunks)
💤 Files with no reviewable changes (1)
- Tests/AblyChatTests/ChatAPITests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift
- Tests/AblyChatTests/Helpers/Helpers.swift
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: SPM (Xcode 16)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16) - GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: check-documentation
🔇 Additional comments (31)
Sources/AblyChat/DefaultRoomReactions.swift (2)
81-85: Good implementation of debug-only error handling.This code correctly emits malformed messages to test subscriptions in DEBUG mode only, which is a good approach for diagnostic purposes without affecting production behavior.
106-116: Clear testing API for malformed messages.The implementation provides a well-designed testing API that follows Swift conventions:
- Debug-only code is properly isolated with #if DEBUG
- Clear documentation comments explaining the purpose of each component
- The method name clearly indicates it's for testing only
This matches the similar implementation in DefaultMessages.swift, maintaining consistency across the codebase.
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (4)
48-48: Improved test naming for clarity.The new method name
subscriptionCanBeRegisteredToReceiveReactionEventsmore precisely describes the test purpose compared to the previoussubscribe_returnsSubscription, making the test intention clearer.
50-67: More comprehensive test fixture setup.The detailed JSON structure for the mock message provides better test coverage by including all relevant fields (name, clientId, data, timestamp, extras) that would be present in a real message.
74-77: More thorough assertion of reaction properties.The test now verifies the actual reaction content (type is ":like:") rather than just checking that a subscription exists, providing stronger validation that the message parsing works correctly.
79-96: Good implementation of malformed message test.This test effectively validates the new malformed message handling functionality:
- Tests spec point CHA-ER4d
- Creates a malformed message scenario
- Verifies that malformed messages are properly captured for debugging while not disrupting regular subscriptions
This complements the changes in DefaultRoomReactions.swift by ensuring the debug-only functionality works as expected.
Sources/AblyChat/DefaultMessages.swift (6)
9-11: Appropriate use of Sendable conformance for testing.The
@retroactive @unchecked Sendableextension for ARTMessage is correctly isolated to DEBUG builds only. This approach allows testing code to work with ARTMessage in concurrent contexts without compiler warnings, while maintaining stricter type safety in production builds.
61-124: Improved error handling with structured catch blocks.The refactoring from direct guard statement throws to a do-catch block significantly improves the code structure:
- The change maintains the same validation logic while providing better error handling
- The debug-only malformed message emission is cleanly integrated
- Error logging is properly implemented at the appropriate level
This approach is consistent with the changes in DefaultRoomReactions.
143-153: Consistent implementation of debug testing helpers.The malformed message subscription implementation matches the one in DefaultRoomReactions, maintaining consistency across similar components in the codebase. This is a good practice for ensuring a unified approach to testing and debugging.
164-166: New API method for updating messages.The addition of the
updatemethod enhances the Messages API functionality, allowing for message updates. The implementation follows the same pattern as other API methods in the class.
168-170: New API method for deleting messages.The addition of the
deletemethod completes the CRUD operations for messages. The implementation is consistent with other API methods in the class.
209-209: Updated specification reference.The comment reference has been updated from CHA-M4d to CHA-M5d, ensuring accurate cross-referencing to the specification document.
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (4)
13-13: Simplified constructor parameter list.Removing the redundant
isLastparameter simplifies the API by eliminating a potentially confusing parameter that could lead to contradictory states (ifhasNextandisLastwere set inconsistently).
43-44: More maintainable property derivation.The
isLastproperty now directly derives its value from!_hasNext, which:
- Eliminates the need for a separate
_isLastproperty- Ensures consistency between
hasNextandisLastproperties- Reduces the chance of bugs due to inconsistent property values
This is a more maintainable approach following the DRY principle.
120-122: Enhanced pagination test configuration.Setting
hasNext: trueon thesuccessGetMessagesWithItemsinstance properly configures this mock to represent a non-final page in a paginated sequence, making it more suitable for testing pagination functionality.
127-155: More complete mock message structure.The updated mock messages now include all required fields:
- Addition of
clientId,action,createdAt,version- Updated serial values to ensure proper sequence
- More descriptive text fields ("next hello" vs "hello")
- Correct configuration of
hasNext: falsefor the last pageThese changes make the mocks more accurately reflect real API responses and support testing the pagination behavior.
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (8)
10-11: Added private state property provides better mock flexibility.The addition of the
_stateproperty enables explicit control over the channel state in test scenarios, which is a good enhancement for testing various channel states.
21-25: Improved callback mechanism for state change events.Adding the type alias and callback properties creates a more flexible system for handling state transitions in tests, allowing verification of state change behaviors.
40-45: Enhanced initializer with better state control and message handling options.The updated initializer provides more flexibility by:
- Making state optional with a default of
nil(instead of the previous.suspended)- Adding the
messageJSONToEmitOnSubscribeparameter for more granular control over emitted messagesThese changes will make testing different scenarios easier and more precise.
87-91: Improved state computation logic.The state property now first checks for an explicitly set state before falling back to the attachment-based logic. This provides more control and clarity in test scenarios.
119-149: Well-structured state change callback methods.The implementation of these methods provides a clean, organized approach to handling state change notifications:
performStateChangeCallbacks: Centralized method for all callbacksperformStateAttachCallbacks: Specialized for attach operationsperformStateDetachCallbacks: Specialized for detach operationsThis structure improves testability of state transitions.
164-167: Asynchronous state change notifications.Using
Taskto perform state change callbacks asynchronously better simulates real-world behavior where state changes occur after the operation completes. This is particularly important for testing race conditions that might exist in the actual implementation.Also applies to: 186-189
192-192: Enhanced message subscription with JSON support.The addition of
messageJSONToEmitOnSubscribeand the corresponding handling in thesubscribemethod allows for more flexible and realistic message creation in tests. This is particularly valuable for testing edge cases with different message formats.Also applies to: 203-220
258-266: Updated callback registration methods.The
onmethods now store callbacks in the appropriate properties, completing the implementation of the callback mechanism. This will help ensure that state change events are properly propagated in tests.Tests/AblyChatTests/DefaultMessagesTests.swift (7)
10-24: Improved test case for REST API message sending.The renamed test
clientMaySendMessageViaRESTChatAPInow properly focuses on verifying successful message sending via the REST API rather than error conditions. This better aligns with the CHA-M3f and CHA-M3a specifications noted in the comments.
26-47: Clear test for payload field omission.The renamed test
whenMetadataAndHeadersAreNotSpecifiedByUserTheyAreOmittedFromRESTPayloadnow explicitly verifies that metadata and headers are omitted from the REST payload when not specified by the user, which is an important validation of the API behavior.
88-118: Good test for subscription point behavior with not-attached channels.This new test verifies the important behavior that the subscription point is set to the attachSerial when the underlying channel is not in the ATTACHED state, covering a distinct edge case in the protocol.
120-162: Tests for subscription point reset on channel reattachment.This test verifies the critical behavior of resetting the subscription point when a channel reenters the ATTACHED state with resumed=false. This is exactly the kind of test needed to catch regression issues in state management.
Based on past review comments, this test was reported as failing intermittently (14-18% locally, >50% in CI). Since this PR seems to modify the test infrastructure with better async state handling, we should verify that it now passes consistently.
#!/bin/bash # Check if this test has been fixed by looking for any recent changes that might address the issue rg -A 5 "whenChannelReentersATTACHEDWithResumedFalseThenSubscriptionPointResetsToAttachSerial" --type swift Tests/ Sources/ rg -A 10 "performStateChangeCallbacks" --type swift Tests/ Sources/
164-213: Tests for subscription point reset on UPDATE event.Similar to the previous test, this verifies the reset of subscription points when an UPDATE event with resumed=false is received. This test is essential for validating the channel's recovery mechanism in response to server-initiated updates.
This test was also reported as failing intermittently in previous review comments. The updated MockRealtimeChannel should address this with its improved state handling, but we should verify that the fixes are effective.
#!/bin/bash # Check if this test's behavior has been fixed or improved in recent changes rg -A 5 "whenChannelUPDATEReceivedWithResumedFalseThenSubscriptionPointResetsToAttachSerial" --type swift Tests/ Sources/ rg -A 10 "resumed.*false" --type swift Sources/
215-251: Comprehensive test for history query options.This test thoroughly validates that the subscription's
getPreviousMessagesmethod properly handles standard history query options, with the exception of direction which has special behavior. The assertions for pagination also verify that the returned results work as expected.
317-321: Clear documentation of untestable specifications.The comments clearly explain which specifications cannot be tested (CHA-M4b and CHA-M4d) and why, which is valuable information for future developers. This kind of documentation helps maintain awareness of potential coverage gaps.
Closes #86 #88
Summary by CodeRabbit
New Features
Bug Fixes
Refactor