[ECO-5001] Some spec alignment #156
Conversation
This spec point was removed because it was redundant.
This has been clarified in the spec.
Mistake in b6c3ddb.
It was a redundant spec point that was implied by CHA-RL4b7.
Mistake in 775ec51.
This is what you’ll now receive when you subscribe to a room feature’s discontinuities. I should have done this in 30cfd30, when we started allowing a discontinuity’s error to be nil; it’s not nice to work with an AsyncSequence that emits optional values, because it makes the return value of operators like `first` ambiguous; did you reach the end of the sequence or did you get a discontinuity without an associated error?
This is implied by the latest version of CHA-RL4a3, which states that you must not overwrite an existing pending discontinuity (will implement and test this behaviour in the next commit).
Will add further behaviour here.
This requirement was added to CHA-RL4a3 subsequent to my implementing the spec point.
Have used the modified “has been attached” criterion from 8b5aa47, for consistency.
lawrence-forooghian
changed the title
WalkthroughThe pull request introduces significant changes across multiple classes in the Ably Chat SDK, primarily focusing on the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (12)
Sources/AblyChat/EmitsDiscontinuities.swift (1)
3-10: Add comprehensive documentation for the struct.The implementation looks good with appropriate protocol conformance and access levels. Consider enhancing the documentation to include:
- Purpose of the struct
- Usage examples
- When errors might be present/nil
Add documentation like this:
+/// Represents a discontinuity event in the Ably Chat system. +/// +/// A discontinuity event occurs when there's an interruption or change in the normal flow of operations, +/// such as connection issues or state transitions. +/// +/// Example usage: +/// ```swift +/// let event = DiscontinuityEvent(error: someError) +/// // Handle the discontinuity... +/// ``` public struct DiscontinuityEvent: Sendable, Equatable {Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
Line range hint
24-28: LGTM: Method properly handles the new DiscontinuityEvent typeThe implementation correctly emits the discontinuity event to all subscriptions. Consider adding a brief documentation comment explaining the purpose of this method in the test context.
Add documentation:
+/// Emits a discontinuity event to all registered subscriptions. +/// Used in tests to simulate discontinuity scenarios. func emitDiscontinuity(_ discontinuity: DiscontinuityEvent) {Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)
74-80: LGTM! Test correctly verifies discontinuity event propagation.The test properly validates that discontinuity events are propagated through the
DefaultRoomReactionsinstance. The change fromARTErrorInfotoDiscontinuityEventaligns with the broader changes in the codebase, and the switch to value equality makes sense for comparing event objects.Consider adding these improvements to make the test more robust:
let featureChannelDiscontinuity = DiscontinuityEvent(error: ARTErrorInfo.createUnknownError() /* arbitrary */ ) let messagesDiscontinuitySubscription = await roomReactions.subscribeToDiscontinuities() await featureChannel.emitDiscontinuity(featureChannelDiscontinuity) // Then: The DefaultRoomReactions instance emits this discontinuity through `subscribeToDiscontinuities` -let messagesDiscontinuity = try #require(await messagesDiscontinuitySubscription.first { _ in true }) +// Add timeout to prevent test from hanging +let messagesDiscontinuity = try #require(await messagesDiscontinuitySubscription.first { _ in true }.timeout(.seconds(5))) #expect(messagesDiscontinuity == featureChannelDiscontinuity) + +// Verify no additional events are emitted +let noMoreEvents = await messagesDiscontinuitySubscription.first { _ in true }.timeout(.milliseconds(100)) +#expect(noMoreEvents == nil)Tests/AblyChatTests/DefaultMessagesTests.swift (1)
89-89: Consider enhancing the test coverage.While the basic functionality is well-tested, consider adding more test cases to verify:
- Multiple discontinuity events in sequence
- Specific error scenarios rather than using an arbitrary unknown error
- Subscription behavior when no discontinuities occur
Example enhancement:
@Test func subscribeToDiscontinuities() async throws { // ... existing setup ... // Test multiple specific discontinuity events + let specificError = ARTErrorInfo.create(withCode: 90001, status: 500, message: "Channel disconnected") + let secondDiscontinuity = DiscontinuityEvent(error: specificError) + await featureChannel.emitDiscontinuity(secondDiscontinuity) + let secondEvent = try #require(await messagesDiscontinuitySubscription.first { $0.error?.code == 90001 }) + #expect(secondEvent == secondDiscontinuity) // Test no discontinuities scenario + let emptySubscription = await messages.subscribeToDiscontinuities() + let hasEvents = await emptySubscription.contains { _ in true } + #expect(!hasEvents) }Example/AblyChatExample/Mocks/MockClients.swift (2)
386-388: Consider implementing a basic mock subscriptionWhile the current unimplemented
subscribeToDiscontinuitiesinMockOccupancyfollows the established pattern, this actor already has a working implementation ofcreateSubscription()that could be leveraged. Consider implementing a basic mock subscription to improve testing capabilities.Here's a suggested implementation:
- func subscribeToDiscontinuities() -> Subscription<DiscontinuityEvent> { - fatalError("Not yet implemented") - } + func subscribeToDiscontinuities() -> Subscription<DiscontinuityEvent> { + let subscription = MockSubscription<DiscontinuityEvent>(randomElement: { + DiscontinuityEvent(reason: .connectionDisconnected) + }, interval: 10) + return .init(mockAsyncSequence: subscription) + }
Protocol conformance issue detected in mock implementations
The mock implementations (
MockRoomReactions,MockTyping,MockPresence,MockOccupancy) are not correctly conforming to theEmitsDiscontinuitiesprotocol. The protocol requires an async function:func subscribeToDiscontinuities() async -> Subscription<DiscontinuityEvent>But the mock implementations are using a non-async signature:
func subscribeToDiscontinuities() -> Subscription<DiscontinuityEvent>The
asynckeyword is missing in all mock implementations, which makes them incompatible with the protocol requirements.🔗 Analysis chain
Line range hint
147-388: Verify protocol conformance across implementationsThe return type change from
Subscription<ARTErrorInfo?>toSubscription<DiscontinuityEvent>appears consistent across all implementations. Let's verify this change aligns with the protocol requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for protocol definitions that might contain subscribeToDiscontinuities rg -l "protocol.*\{" | xargs rg "subscribeToDiscontinuities.*->.*Subscription"Length of output: 330
Script:
#!/bin/bash # Let's check the EmitsDiscontinuities protocol definition and its usage rg -A 5 "protocol EmitsDiscontinuities" # Also check for any other implementations of subscribeToDiscontinuities ast-grep --pattern 'func subscribeToDiscontinuities() $$$'Length of output: 355
Sources/AblyChat/DefaultMessages.swift (1)
Line range hint
1-270: Excellent documentation and error handling.The code demonstrates high quality in several areas:
- Comprehensive spec references in comments
- Well-tracked TODOs with GitHub issues
- Thorough error handling with detailed logging
- Proper use of actor-based concurrency
Consider adding a class-level documentation comment explaining the role of
DefaultMessagesin the SDK architecture and its relationship withEmitsDiscontinuities.Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (1)
Line range hint
1357-1625: Consider extracting setup logic to improve test readability.The contributor event handling tests provide excellent coverage and are well-structured with clear Given/When/Then sections. However, some test methods are quite long and contain repeated setup logic. Consider extracting common setup code into helper methods, particularly for:
- Contributor state change creation
- Manager initialization with specific states
- Assertion verification
Example refactoring:
private func createContributorStateChange( current: ARTRealtimeChannelState, previous: ARTRealtimeChannelState, event: ARTChannelEvent, reason: ARTErrorInfo? = nil, resumed: Bool = false ) -> ARTChannelStateChange { ARTChannelStateChange( current: current, previous: previous, event: event, reason: reason, resumed: resumed ) }Sources/AblyChat/RoomLifecycleManager.swift (4)
114-114: Update initializers to includependingDiscontinuityEventsparameterThe
internal initmethod now includes apendingDiscontinuityEventsparameter. Ensure that all initializations provide appropriate values for this parameter, especially in test cases where this initializer is used.
399-400: Test utility method for accessingpendingDiscontinuityEventThe addition of
testsOnly_pendingDiscontinuityEvent(for:)provides a way to accesspendingDiscontinuityEventduring testing. Ensure this method remains confined to testing environments and does not affect production code.Consider adding documentation to clarify that this method is intended for testing purposes only.
867-869: Emitting pending discontinuity events to contributorsThe
emitPendingDiscontinuityEventsmethod iterates over contributors and emits any pending discontinuity events. Verify that this asynchronous operation properly handles any potential concurrency issues.Consider adding error handling for the
await contributor.emitDiscontinuity(pendingDiscontinuityEvent)call to catch and log any exceptions that might occur during the emission.
1059-1059: Maintain consistency in status transition casesThe inclusion of multiple statuses in a single line improves readability but may deviate from standard formatting practices.
Consider formatting each case on its own line for better clarity:
case .attached, .attachingDueToAttachOperation, .attachingDueToRetryOperation, .attachingDueToContributorStateChange, .detaching, .suspendedAwaitingStartOfRetryOperation, .suspended, .failed: break
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
Example/AblyChatExample/Mocks/MockClients.swift(5 hunks)Sources/AblyChat/DefaultMessages.swift(1 hunks)Sources/AblyChat/DefaultOccupancy.swift(1 hunks)Sources/AblyChat/DefaultPresence.swift(1 hunks)Sources/AblyChat/DefaultRoomLifecycleContributor.swift(2 hunks)Sources/AblyChat/DefaultRoomReactions.swift(1 hunks)Sources/AblyChat/DefaultTyping.swift(1 hunks)Sources/AblyChat/EmitsDiscontinuities.swift(1 hunks)Sources/AblyChat/Errors.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Sources/AblyChat/RoomLifecycleManager.swift(14 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(18 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift(1 hunks)
🧰 Additional context used
📓 Learnings (6)
Example/AblyChatExample/Mocks/MockClients.swift (1)
Learnt from: maratal
PR: ably-labs/ably-chat-swift#34
File: Example/AblyChatExample/Mocks/MockClients.swift:367-369
Timestamp: 2024-11-12T15:07:31.865Z
Learning: In mock implementations, it's acceptable to leave `subscribeToDiscontinuities` unimplemented during early development.
Sources/AblyChat/DefaultRoomReactions.swift (1)
Learnt from: umair-ably
PR: ably-labs/ably-chat-swift#89
File: Sources/AblyChat/DefaultRoomReactions.swift:73-75
Timestamp: 2024-11-12T15:07:31.866Z
Learning: In the 'ably-chat-swift' repository (Swift project), always ignore giving feedback on usage of `fatalError("Not implemented")` in the code, including for methods like `subscribeToDiscontinuities()`.
Sources/AblyChat/RoomLifecycleManager.swift (1)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Sources/AblyChat/RoomLifecycleManager.swift:0-0
Timestamp: 2024-11-12T15:07:31.866Z
Learning: In `RoomLifecycleManager`, handling of unexpected contributor states after attach failure will be addressed in issue #49.
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (6)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-11-12T15:07:39.465Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:335-335
Timestamp: 2024-11-12T15:07:31.865Z
Learning: In Swift Testing, test method names do not need to start with `test`.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:941-941
Timestamp: 2024-11-12T15:07:31.866Z
Learning: When a TODO comment references an existing GitHub issue, it's acceptable to leave it in the code without requesting its removal.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#118
File: Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift:988-988
Timestamp: 2024-11-18T19:45:48.787Z
Learning: In Swift, not awaiting an `async let` variable does not lead to resource leaks.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:499-504
Timestamp: 2024-11-12T15:07:31.865Z
Learning: In test closures, parameters like `callCount` may be used to track invocation counts correctly across invocations.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:836-836
Timestamp: 2024-11-12T15:07:31.866Z
Learning: Before suggesting the removal of TODO comments, check if they reference existing GitHub issues that track the pending work.
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#115
File: Tests/AblyChatTests/Mocks/MockFeatureChannel.swift:30-30
Timestamp: 2024-11-18T16:02:11.878Z
Learning: Swift supports typed `throws` syntax, allowing function declarations to specify the type of errors they can throw using the syntax `throws(ErrorType)`.
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift (1)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#67
File: Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift:98-102
Timestamp: 2024-11-12T15:07:31.865Z
Learning: The cleanup of old subscriptions in `MockRoomLifecycleContributorChannel` is acknowledged with a TODO comment and is tracked in issue #36.
🔇 Additional comments (36)
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift (2)
8-8: LGTM: Property type updated to align with new spec
The change from [ARTErrorInfo?] to [DiscontinuityEvent] correctly aligns with the broader refactoring of discontinuity handling across the codebase.
15-16: LGTM: Method updated to handle DiscontinuityEvent
The implementation correctly records discontinuity events for test verification purposes.
Let's verify that this change aligns with the protocol definition:
✅ Verification successful
LGTM: Method signature matches protocol definition
The implementation in MockRoomLifecycleContributor correctly matches the RoomLifecycleContributor protocol definition, which requires an async method emitDiscontinuity(_ discontinuity: DiscontinuityEvent). The mock implementation appropriately records the events for test verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the RoomLifecycleContributor protocol definition matches this implementation
# Search for the protocol definition
ast-grep --pattern 'protocol RoomLifecycleContributor {
$$$
func emitDiscontinuity($_) async
$$$
}'
Length of output: 107
Script:
#!/bin/bash
# Let's try a broader search for the protocol definition
rg -l "protocol RoomLifecycleContributor"
Length of output: 87
Script:
#!/bin/bash
# Search for any file containing emitDiscontinuity method
rg "func emitDiscontinuity"
Length of output: 523
Script:
#!/bin/bash
# Search for files containing DiscontinuityEvent to understand the type
rg "DiscontinuityEvent"
Length of output: 12135
Script:
#!/bin/bash
# Let's check the protocol definition in RoomLifecycleManager.swift
rg -A 5 "protocol RoomLifecycleContributor" Sources/AblyChat/RoomLifecycleManager.swift
Length of output: 690
Script:
#!/bin/bash
# Let's get more context from the RoomLifecycleContributor protocol
rg -A 15 "protocol RoomLifecycleContributor" Sources/AblyChat/RoomLifecycleManager.swift
Length of output: 1785
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (2)
7-7: LGTM: Property type update aligns with system-wide changes
The updated type for discontinuitySubscriptions properly reflects the transition from ARTErrorInfo? to DiscontinuityEvent as mentioned in the PR summary.
18-22: LGTM: Method signature and implementation properly updated
The implementation correctly creates and manages subscriptions with the new DiscontinuityEvent type.
Let's verify that all consumers of this mock have been updated to handle the new type:
✅ Verification successful
Let me search for actual usage of subscribeToDiscontinuities to verify the consumers are properly handling the new type.
Method signature and implementation are correctly used across the codebase
The verification shows that all consumers (DefaultRoomReactionsTests and DefaultMessagesTests) are properly handling the DiscontinuityEvent type and correctly using the subscribeToDiscontinuities method. The tests demonstrate proper type handling, subscription management, and event verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files using MockFeatureChannel to ensure they handle DiscontinuityEvent
rg -t swift "MockFeatureChannel" Tests/ -C 3
Length of output: 6942
Script:
#!/bin/bash
# Search for usage of subscribeToDiscontinuities to verify type handling
rg -t swift "subscribeToDiscontinuities" Tests/ -C 5
Length of output: 5684
Sources/AblyChat/DefaultRoomLifecycleContributor.swift (2)
6-6: LGTM: Type update aligns with spec changes
The update from ARTErrorInfo? to DiscontinuityEvent is consistent with the spec alignment objectives.
15-18: LGTM: Method signature and implementation correctly updated
The changes to emitDiscontinuity properly handle the new DiscontinuityEvent type.
Sources/AblyChat/DefaultOccupancy.swift (1)
53-55: LGTM! Implementation aligns with spec point CHA-O5.
The change correctly updates the return type to Subscription<DiscontinuityEvent> and properly delegates to the feature channel.
Let's verify the implementation in featureChannel:
✅ Verification successful
Implementation verified and correctly follows protocol requirements
The verification shows that:
FeatureChannelis a protocol that inherits fromEmitsDiscontinuitiesDefaultFeatureChannelis the concrete implementation used in production- Multiple features (Messages, Presence, Typing, etc.) consistently delegate to their
featureChannel.subscribeToDiscontinuities() - The return type
Subscription<DiscontinuityEvent>is consistent across all implementations
The implementation in DefaultOccupancy correctly follows the established pattern and protocol requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of subscribeToDiscontinuities in FeatureChannel
# Expected: Find the implementation to confirm proper delegation
# Search for FeatureChannel class and its subscribeToDiscontinuities implementation
ast-grep --pattern 'class FeatureChannel {
$$$
func subscribeToDiscontinuities($$$) {
$$$
}
$$$
}'
Length of output: 112
Script:
#!/bin/bash
# Let's try a broader search to find the FeatureChannel implementation
# First, find files that might contain FeatureChannel
fd "FeatureChannel" --type f
# Then use ripgrep to find the implementation with context
rg "class FeatureChannel" -A 10
# Also search for the method specifically
rg "func subscribeToDiscontinuities" -A 5
Length of output: 8248
Script:
#!/bin/bash
# Let's find the actual FeatureChannel protocol or class definition
rg "protocol FeatureChannel|class FeatureChannel" -A 15
# Also search for any files containing FeatureChannel in their content
rg "FeatureChannel" --type swift
Length of output: 5863
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
83-85: LGTM! The discontinuity event setup and emission is well-structured.
The code correctly tests the new DiscontinuityEvent type and follows good testing practices with clear setup and execution steps.
Sources/AblyChat/RoomFeature.swift (2)
61-63: LGTM: Return type change improves type safety
The change from Subscription<ARTErrorInfo?> to Subscription<DiscontinuityEvent> makes the return type more specific and meaningful, improving type safety and clarity.
Line range hint 65-67: LGTM: Explicit error type improves error handling clarity
The explicit error type throws(ARTErrorInfo) clearly communicates the error handling contract to API consumers, aligning with the documented error cases in the function's documentation.
Sources/AblyChat/DefaultRoomReactions.swift (2)
83-85: LGTM! Clean implementation following specification point CHA-ER5.
The method correctly delegates to the feature channel's implementation, maintaining a clean separation of concerns.
Line range hint 1-4: Verify the status of task isolation issue #83.
The TODO comment indicates that @MainActor is a temporary fix for task isolation. Let's check the status of this issue.
✅ Verification successful
Task isolation issue #83 is still open and requires further investigation.
The issue is actively tracking the need for @MainActor annotation. According to the issue description:
- This was introduced to fix an "Incorrect actor executor assumption" error that appeared with Swift 6
- It's considered a temporary solution pending further investigation
- The team plans to revisit this once there's more functionality implemented and better understanding of Swift Concurrency
Since this is a known limitation being tracked, and the current solution is working as intended, no immediate action is required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the status of issue #83 and related discussions
gh issue view 83 --json title,state,body,comments
Length of output: 1176
Sources/AblyChat/Errors.swift (2)
Line range hint 1-248: Track TODOs in GitHub issues.
The implementation follows Swift best practices for error handling. I notice there are TODOs referencing GitHub issues #32. This is good practice, but let's verify these are being tracked.
Let's check the referenced issue:
✅ Verification successful
TODOs are properly tracked in issue #32
The TODOs in the error handling implementation are being tracked in issue #32, which is currently open and actively maintained. The issue specifically addresses:
- Custom error domain and error codes implementation
- The copied code in
init(errorCode:) - The broader discussion about error handling approach
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if issue #32 exists and is open
# Expected: Find an open issue tracking these TODOs
gh issue view 32 --json title,state,body
Length of output: 1316
41-59: LGTM! Verify spec alignment for status codes.
The status code mappings have been updated to align with the "Chat-specific Error Codes" section of the spec. Moving detachment failures to 500 status codes correctly indicates these as server-side errors.
Let's verify the spec alignment:
Example/AblyChatExample/Mocks/MockClients.swift (4)
147-149: LGTM: Implementation follows mock pattern
The unimplemented subscribeToDiscontinuities method in MockMessages follows the established pattern for mock implementations. This aligns with the learning from PR #34 where it's acceptable to leave this method unimplemented during early development.
198-200: LGTM: Implementation follows mock pattern
The unimplemented subscribeToDiscontinuities method in MockRoomReactions maintains consistency with other mock implementations.
247-249: LGTM: Implementation follows mock pattern
The unimplemented subscribeToDiscontinuities method in MockTyping follows the same pattern as other mock implementations.
351-353: LGTM: Implementation follows mock pattern
The unimplemented subscribeToDiscontinuities method in MockPresence maintains consistency with other mock implementations.
Sources/AblyChat/DefaultPresence.swift (2)
197-199: LGTM: Return type change aligns with spec
The change from Subscription<ARTErrorInfo?> to Subscription<DiscontinuityEvent> aligns with the spec update and provides more specific event handling. The implementation correctly delegates to the feature channel.
Let's verify consistent implementation across related files:
✅ Verification successful
Return type change is consistently implemented across the codebase
The search results confirm that the change from ARTErrorInfo? to DiscontinuityEvent is consistently implemented across all related files:
- The
EmitsDiscontinuitiesprotocol defines the interface withSubscription<DiscontinuityEvent> - All implementations (
DefaultPresence,DefaultTyping,DefaultOccupancy,DefaultMessages,DefaultRoomReactions) correctly returnSubscription<DiscontinuityEvent> - The mock implementations and tests are aligned with this return type
- The change is part of a broader spec-compliant implementation where discontinuity events are properly managed through the
RoomLifecycleManager
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent implementation of subscribeToDiscontinuities
# across all related files
# Search for all implementations of subscribeToDiscontinuities
ast-grep --pattern 'func subscribeToDiscontinuities() async -> Subscription<$_>'
# Search for any remaining references to ARTErrorInfo in discontinuity contexts
rg "Subscription<ARTErrorInfo"
Length of output: 115
Script:
#!/bin/bash
# Let's try a broader search to find all references to subscribeToDiscontinuities
rg "subscribeToDiscontinuities" -A 2 -B 2
# Also search for any protocol definitions or implementations related to discontinuities
ast-grep --pattern 'protocol $_Presence {
$$$
}'
# And search for DiscontinuityEvent usage
rg "DiscontinuityEvent" -A 2 -B 2
Length of output: 49374
196-199: Verify documentation reflects the new return type
The implementation looks good, but since this is a spec change (CHA-PR8), please ensure that any relevant documentation (README, API docs, etc.) has been updated to reflect the new DiscontinuityEvent return type.
Let's check for documentation that might need updates:
✅ Verification successful
Let me search for documentation related to discontinuity events and subscriptions.
Based on the search results, I can now generate the final response:
Documentation appears to be consistent with the new return type
The implementation of subscribeToDiscontinuities() returning Subscription<DiscontinuityEvent> is well-documented in the codebase. The DiscontinuityEvent type and its usage are properly documented in the source files, particularly in EmitsDiscontinuities.swift. No documentation updates are needed as this appears to be an internal API change that's consistently implemented across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for documentation files that might need updates
# Search for documentation files
fd -e md -e txt
# Search for references to the old return type in documentation
rg -i "arterrorinfo"
Length of output: 24453
Script:
#!/bin/bash
# Search for documentation about discontinuity events and subscriptions
rg -i "discontinuit|subscrib" README.md CONTRIBUTING.md
# Search for API documentation
fd "README|API" -e md
# Look for any Swift documentation comments related to discontinuity events
rg -A 3 "/// .*discontinuit"
Length of output: 2428
Sources/AblyChat/DefaultMessages.swift (1)
112-114: LGTM! Verify protocol conformance across codebase.
The return type change from Subscription<ARTErrorInfo?> to Subscription<DiscontinuityEvent> aligns with the spec updates. The implementation correctly delegates to featureChannel.
Let's verify the protocol conformance and usage:
✅ Verification successful
Return type change is consistent across the codebase
The verification shows that:
- The
EmitsDiscontinuitiesprotocol correctly defines the method withSubscription<DiscontinuityEvent>return type - All implementations (
DefaultMessages,DefaultTyping,DefaultPresence,DefaultOccupancy,DefaultRoomReactions,RoomFeature) consistently use the new return type - Test files and mocks have been updated accordingly
- The change is properly integrated across the entire codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify EmitsDiscontinuities protocol implementation and usage
# Check EmitsDiscontinuities protocol definition and implementations
ast-grep --pattern 'protocol EmitsDiscontinuities {
$$$
}'
# Check other implementations of subscribeToDiscontinuities
ast-grep --pattern 'func subscribeToDiscontinuities() async -> Subscription<DiscontinuityEvent>'
# Check usages of subscribeToDiscontinuities
rg -A 2 'subscribeToDiscontinuities\(\)'
Length of output: 5685
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (2)
57-57: LGTM! Parameter type change improves efficiency.
The change from array to dictionary for pendingDiscontinuityEvents is a good improvement. Using a dictionary with contributor IDs as keys provides more efficient lookups and better represents the relationship between contributors and their discontinuity events.
Line range hint 752-873: Great test refactoring using parameterization!
Excellent refactoring of the release operation tests. Using @Test(arguments:) to combine related test cases reduces code duplication while maintaining clear test coverage for both detached and initialized states. The implementation is clean and well-documented.
Sources/AblyChat/DefaultTyping.swift (1)
163-163: LGTM!
The subscribeToDiscontinuities() method correctly returns Subscription<DiscontinuityEvent> and delegates to featureChannel.subscribeToDiscontinuities(). The implementation aligns with the updated return type and the overall design.
Sources/AblyChat/RoomLifecycleManager.swift (12)
133-133: Consistent initialization of pendingDiscontinuityEvents
The private initializer now includes pendingDiscontinuityEvents to initialize ContributorAnnotations. This change ensures that pending discontinuity events are properly managed during initialization.
265-265: Addition of pendingDiscontinuityEvent property to ContributorAnnotation
The new pendingDiscontinuityEvent property in ContributorAnnotation allows for individual tracking of pending discontinuity events per contributor. This enhances state management and contributes to more precise event handling.
281-281: Initialize ContributorAnnotations with pendingDiscontinuityEvents
When creating ContributorAnnotations, you now initialize it with pendingDiscontinuityEvents. Verify that this initialization correctly maps pending events to the corresponding contributors.
286-286: Accurate mapping of pending discontinuity events
The assignment of pendingDiscontinuityEvent using pendingDiscontinuityEvents[contributor.id] ensures correct association. This mapping is crucial for handling pending events specific to each contributor.
310-310: Clearing pendingDiscontinuityEvent in clearPendingDiscontinuityEvents
Resetting pendingDiscontinuityEvent to nil when clearing pending events ensures that old events do not persist unintentionally. This maintains the integrity of the event handling process.
443-446: Prevent processing for contributors not yet attached
The guard statement ensures that state changes are only processed for contributors that have already been attached. This prevents unnecessary handling of events for unattached contributors.
452-452: Record pending discontinuity event during operations
When operations are in progress, discontinuity events are now recorded for later processing. This ensures that events are not lost and are handled appropriately once the operation concludes.
455-456: Immediate emission of discontinuity events when no operations are in progress
If there are no ongoing operations, discontinuity events are emitted immediately to the contributor. This aligns with the expected behavior for real-time event handling.
584-594: Implementation of recordPendingDiscontinuityEvent method
The recordPendingDiscontinuityEvent method checks for existing pending events before recording a new one, preventing overwriting unhandled events. This logic ensures that each event is processed in order.
969-969: Proper error creation for detachment failures
Creating an ARTErrorInfo with detachmentFailed ensures that detachment errors are accurately represented and can be handled appropriately by the calling functions.
1047-1051: Consolidate status handling in RELEASE operation
The switch cases for the RELEASE operation now include .detached, .detachedDueToRetryOperation, and .initialized statuses, transitioning directly to .released. This streamlines the release process.
40-40: Ensure all conforming implementations of emitDiscontinuity are updated
The emitDiscontinuity method in the RoomLifecycleContributor protocol now accepts a DiscontinuityEvent instead of an optional ARTErrorInfo. Please ensure that all conforming types and their implementations have been updated to reflect this signature change.
Run the following script to verify all implementations:
✅ Verification successful
All implementations of emitDiscontinuity are correctly updated to use DiscontinuityEvent
Based on the search results, all implementations of emitDiscontinuity have been properly updated to use the DiscontinuityEvent parameter type:
MockRoomLifecycleContributor.swift:func emitDiscontinuity(_ discontinuity: DiscontinuityEvent)DefaultRoomLifecycleContributor.swift:func emitDiscontinuity(_ discontinuity: DiscontinuityEvent)MockFeatureChannel.swift:func emitDiscontinuity(_ discontinuity: DiscontinuityEvent)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all conforming types implement the updated `emitDiscontinuity` method.
# Find all implementations of `RoomLifecycleContributor` and check their `emitDiscontinuity` methods.
ast-grep --pattern $'extension $_: RoomLifecycleContributor {
$$$
func emitDiscontinuity(_ discontinuity: $_) {
$$$
}
$$$
}'
Length of output: 137
Script:
#!/bin/bash
# Let's try a different approach to find implementations
# First, let's find all Swift files that mention RoomLifecycleContributor
rg -l "RoomLifecycleContributor" --type swift
# Then let's look for any emitDiscontinuity implementations
rg "func emitDiscontinuity" --type swift -A 2
Length of output: 1702
Part of #66. This brings us up to date with the latest version of some spec points, and implements some newly-added spec points. See commit messages for details.
Summary by CodeRabbit
DiscontinuityEventstruct for better handling of discontinuity events.Subscription<DiscontinuityEvent>instead ofSubscription<ARTErrorInfo?>, enhancing event specificity.