[ECO-5015] Implement public API changes of CHADR-062 #92
Conversation
WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on the renaming and restructuring of room status handling within the chat application. Key modifications include updates to methods and properties related to room status, the introduction of new structs for status changes, and the removal of outdated components. The changes aim to enhance clarity and maintainability in the codebase while aligning with new naming conventions for state management. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift (1)
Line range hint
4-18: Consider adding documentation for error states.While the implementation is correct, adding documentation would improve maintainability by explaining when and why
.failedand.suspendedstates contain errors.import Ably import AblyChat +/// Provides error information for room status states. +/// - Returns: An `ARTErrorInfo` for error states (`.failed` and `.suspended`), +/// and `nil` for all other states. extension RoomStatus { var error: ARTErrorInfo? {Sources/AblyChat/RoomStatus.swift (3)
3-3: Remove outdated TODO comment.The TODO comment appears to be outdated since the renaming from
RoomLifecycletoRoomStatushas already been implemented as part of CHADR-062.-// TODO: rename
15-17: Improve code style in documentation examples.The documentation is clear and helpful, but the code examples could be improved for better readability.
- // 1. testing (e.g. `#expect(status.isFailed)`) - // 2. testing that a status does _not_ have a particular case (e.g. if !status.isFailed), which a `case` statement cannot succinctly express + // 1. Testing: `XCTAssertTrue(status.isFailed)` + // 2. Negative testing: `if !status.isFailed { ... }` (more concise than using case statements)
Line range hint
19-42: Simplify boolean property implementations using Swift pattern matching.The current implementation is more verbose than necessary. Swift offers a more concise way to implement these properties.
public var isAttaching: Bool { - if case .attaching = self { - true - } else { - false - } + guard case .attaching = self else { return false } + return true } public var isSuspended: Bool { - if case .suspended = self { - true - } else { - false - } + guard case .suspended = self else { return false } + return true } public var isFailed: Bool { - if case .failed = self { - true - } else { - false - } + guard case .failed = self else { return false } + return true }Or even more concisely:
public var isAttaching: Bool { guard case .attaching = self else { return false }; return true } public var isSuspended: Bool { guard case .suspended = self else { return false }; return true } public var isFailed: Bool { guard case .failed = self else { return false }; return true }Example/AblyChatExample/ContentView.swift (1)
Line range hint
237-245: LGTM! Well-implemented animation system.The rotation animation implementation is clean and efficient, using SwiftUI's animation system appropriately. The cleanup is handled properly to prevent memory leaks.
Consider adding documentation comments to explain the rotation parameters:
struct Reaction: Identifiable { let id: UUID let emoji: String var xPosition: CGFloat var yPosition: CGFloat var scale: CGFloat var opacity: Double + /// Current rotation angle in degrees var rotationAngle: Double + /// Speed of rotation in degrees per second var rotationSpeed: Double var duration: Double }Also applies to: 272-276
Tests/AblyChatTests/RoomLifecycleManagerTests.swift (1)
Line range hint
1165-1224: Consider grouping related test cases.The test cases
contributorAttachedEvent_withNoOperationInProgress_roomNotAttached_allContributorsAttachedandcontributorAttachedEvent_withNoOperationInProgress_roomNotAttached_notAllContributorsAttachedare closely related. Consider grouping them using a test helper function to reduce code duplication and improve maintainability.Example refactor:
private func testContributorAttachedEvent( allContributorsAttached: Bool, file: StaticString = #file, line: UInt = #line ) async throws { // Given: A RoomLifecycleManager with contributors in specified states let contributors = [ createContributor(initialState: .attached), createContributor(initialState: allContributorsAttached ? .attached : .detached) ] let initialManagerStatus = RoomLifecycleManager<MockRoomLifecycleContributor>.Status.detached let manager = await createManager( forTestingWhatHappensWhenCurrentlyIn: initialManagerStatus, contributors: contributors ) // When: A contributor emits state change to ATTACHED let contributorStateChange = ARTChannelStateChange( current: .attached, previous: .attaching, event: .attached, reason: ARTErrorInfo(domain: "SomeDomain", code: 123), resumed: false ) if allContributorsAttached { let roomStatusSubscription = await manager.onChange(bufferingPolicy: .unbounded) async let maybeAttachedRoomStatusChange = roomStatusSubscription.first { $0.current == .attached } await contributors[0].channel.emitStateChange(contributorStateChange) // Then: Room status transitions to ATTACHED _ = try #require(await maybeAttachedRoomStatusChange) #expect(await manager.roomStatus == .attached) } else { await waitForManager(manager, toHandleContributorStateChange: contributorStateChange) { await contributors[0].channel.emitStateChange(contributorStateChange) } // Then: Room status does not change #expect(await manager.roomStatus == initialManagerStatus.toRoomStatus) } } @Test func contributorAttachedEvent_withNoOperationInProgress_roomNotAttached_allContributorsAttached() async throws { try await testContributorAttachedEvent(allContributorsAttached: true) } @Test func contributorAttachedEvent_withNoOperationInProgress_roomNotAttached_notAllContributorsAttached() async throws { try await testContributorAttachedEvent(allContributorsAttached: false) }Sources/AblyChat/Room.swift (2)
46-46: Prioritize cleaning up old subscriptionsThere's a TODO comment at line 46 referencing issue #36 about cleaning up old subscriptions. Since
statusSubscriptionscould potentially grow indefinitely, leading to increased memory usage and retention of resources, consider prioritizing the implementation of a cleanup mechanism.
121-127: Ensure thread safety and actor isolation intransitionmethodSince
DefaultRoomis an actor, thetransitionmethod is protected from concurrent access issues. However, ensure that any code within this method that interacts with non-actor-isolated properties or external systems maintains thread safety and respects actor isolation.Example/AblyChatExample/Mocks/MockClients.swift (2)
67-68: Consider markingstatusasnonisolatedif accessed externallyIf the
statusproperty is intended to be accessed from outside theMockRoomactor without requiringawait, consider marking it asnonisolatedto allow synchronous access.
79-85: Avoid force unwrapping when usingrandomElement()Force unwrapping the result of
randomElement()can lead to runtime crashes if the array is empty. While the array currently has elements, it's safer to use optional binding to prevent potential issues in the future.You can modify the code to safely unwrap the optional:
private func createSubscription() -> MockSubscription<RoomStatusChange> { let subscription = MockSubscription<RoomStatusChange>(randomElement: { - RoomStatusChange(current: [.attached, .attached, .attached, .attached, .attaching(error: nil), .attaching(error: nil), .suspended(error: .createUnknownError())].randomElement()!, previous: .attaching(error: nil)) + if let currentStatus = [ + .attached, + .attached, + .attached, + .attached, + .attaching(error: nil), + .attaching(error: nil), + .suspended(error: .createUnknownError()) + ].randomElement() { + return RoomStatusChange(current: currentStatus, previous: .attaching(error: nil)) + } else { + return RoomStatusChange(current: .initialized, previous: .attaching(error: nil)) + } }, interval: 8) mockSubscriptions.append(subscription) return subscription }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- Example/AblyChatExample/ContentView.swift (1 hunks)
- Example/AblyChatExample/Mocks/MockClients.swift (2 hunks)
- Sources/AblyChat/Connection.swift (2 hunks)
- Sources/AblyChat/Room.swift (4 hunks)
- Sources/AblyChat/RoomLifecycleManager.swift (4 hunks)
- Sources/AblyChat/RoomStatus.swift (2 hunks)
- Tests/AblyChatTests/DefaultRoomStatusTests.swift (0 hunks)
- Tests/AblyChatTests/DefaultRoomTests.swift (7 hunks)
- Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift (1 hunks)
- Tests/AblyChatTests/RoomLifecycleManagerTests.swift (32 hunks)
💤 Files with no reviewable changes (1)
- Tests/AblyChatTests/DefaultRoomStatusTests.swift
🧰 Additional context used
📓 Learnings (5)
Sources/AblyChat/Room.swift (2)
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#77 File: Sources/AblyChat/Room.swift:14-14 Timestamp: 2024-10-01T21:58:50.246Z Learning: In the codebase, the term `status` now has a new meaning, and its usage is intentional.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#77 File: Sources/AblyChat/Room.swift:14-14 Timestamp: 2024-10-08T15:58:47.376Z Learning: In the codebase, the term `status` now has a new meaning, and its usage is intentional.Sources/AblyChat/RoomLifecycleManager.swift (5)
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#77 File: Sources/AblyChat/Room.swift:14-14 Timestamp: 2024-10-01T21:58:50.246Z Learning: In the codebase, the term `status` now has a new meaning, and its usage is intentional.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#77 File: Sources/AblyChat/Room.swift:14-14 Timestamp: 2024-10-08T15:58:47.376Z Learning: In the codebase, the term `status` now has a new meaning, and its usage is intentional.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Sources/AblyChat/RoomLifecycleManager.swift:217-220 Timestamp: 2024-10-08T15:58:47.376Z Learning: In `RoomLifecycleManager`, handling of missing `stateChange.reason` when `stateChange.resumed` is `false` is being tracked in issue #74. Future reviews should be aware of this planned work.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Sources/AblyChat/RoomLifecycleManager.swift:217-220 Timestamp: 2024-10-01T19:44:34.032Z Learning: In `RoomLifecycleManager`, handling of missing `stateChange.reason` when `stateChange.resumed` is `false` is being tracked in issue #74. Future reviews should be aware of this planned work.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#85 File: Sources/AblyChat/RoomLifecycleManager.swift:140-191 Timestamp: 2024-10-15T18:36:12.786Z Learning: In `Sources/AblyChat/RoomLifecycleManager.swift`, all cases of the `Status` enum are exhaustively handled in switch statements, so suggesting exhaustive handling is unnecessary.Sources/AblyChat/RoomStatus.swift (2)
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#77 File: Sources/AblyChat/Room.swift:14-14 Timestamp: 2024-10-01T21:58:50.246Z Learning: In the codebase, the term `status` now has a new meaning, and its usage is intentional.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#77 File: Sources/AblyChat/Room.swift:14-14 Timestamp: 2024-10-08T15:58:47.376Z Learning: In the codebase, the term `status` now has a new meaning, and its usage is intentional.Tests/AblyChatTests/DefaultRoomTests.swift (2)
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631 Timestamp: 2024-10-01T12:55:21.968Z 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:627-631 Timestamp: 2024-10-08T15:58:47.376Z Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.Tests/AblyChatTests/RoomLifecycleManagerTests.swift (8)
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:941-941 Timestamp: 2024-10-01T19:46:16.467Z 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#67 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:941-941 Timestamp: 2024-10-08T15:58:47.376Z 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#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:335-335 Timestamp: 2024-09-23T19:56:39.435Z 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:836-836 Timestamp: 2024-10-01T19:46:04.461Z Learning: Before suggesting the removal of TODO comments, check if they reference existing GitHub issues that track the pending work.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:836-836 Timestamp: 2024-10-08T15:58:47.376Z Learning: Before suggesting the removal of TODO comments, check if they reference existing GitHub issues that track the pending work.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:335-335 Timestamp: 2024-10-08T15:58:47.376Z Learning: In Swift Testing, test method names do not need to start with `test`.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#54 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631 Timestamp: 2024-10-01T12:55:21.968Z 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:627-631 Timestamp: 2024-10-08T15:58:47.376Z Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
🔇 Additional comments (24)
Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift (1)
4-4: LGTM! Extension rename aligns with API changes.The rename from
RoomLifecycletoRoomStatuscorrectly implements the API changes outlined in CHADR-062, maintaining consistency across the codebase.Sources/AblyChat/Connection.swift (6)
4-4: LGTM! Improved type safety by using concrete ConnectionStatus type.The change from
any ConnectionStatustoConnectionStatusremoves the use of an existential type, which can improve performance and type safety.
7-7: LGTM! Method name better reflects its purpose.Renaming from
onChangetoonStatusChangeimproves API clarity by being more specific about what changes are being observed.
Line range hint
10-17: LGTM! Well-structured connection states.The ConnectionStatus enum provides a comprehensive set of states for connection lifecycle management and is appropriately marked as Sendable for thread-safety.
20-21: LGTM! Consistent type usage for status properties.The properties correctly use the new ConnectionStatus type, maintaining consistency with the enum definition.
Line range hint
26-32: LGTM! Well-structured initializer with proper optional handling.The initializer provides good flexibility with an optional error parameter while maintaining the required status parameters.
23-23: Consider addressing the TODO comment about unwrap requirement.The TODO comment references issue #12 regarding unwrap requirements. This might indicate a potential improvement in error handling design.
Let's check the referenced issue and related discussions:
✅ Verification successful
Let me check the actual implementation and context around this error property to better understand the current state.
Let me check the ConnectionStatus enum values to understand the states where error might be relevant.
TODO comment is tracking a valid enhancement for error handling
Based on the evidence gathered, the TODO comment references issue #12 which proposes making
erroran associated value of the state enum instead of an optional property. This would indeed improve type safety as:
- The
failedand potentiallysuspendedstates should have associated errors- Other states like
initialized,connecting,connectedshould never have errors- The current optional design requires unnecessary unwrapping even in states where errors are guaranteed
The issue is open and tracked, so this TODO serves as a valid reminder for future enhancement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the status and discussion of the referenced issue # Test: Fetch the issue details gh issue view 12 || echo "Issue not accessible or doesn't exist" # Test: Search for related discussions about unwrap requirements rg -l "unwrap" --type swiftLength of output: 1075
Script:
#!/bin/bash # Check the implementation and usage of error property rg -A 5 -B 5 "error: ARTErrorInfo\?" Sources/AblyChat/Connection.swift # Check for any related state enums that might be relevant ast-grep --pattern 'enum $_ { $$$ }'Length of output: 1111
Script:
#!/bin/bash # Get the full ConnectionStatus enum implementation rg -A 20 "enum ConnectionStatus" Sources/AblyChat/Connection.swift # Check for any usage patterns of error property with different states rg -A 3 -B 3 "\.error" Sources/AblyChat/Length of output: 3940
Sources/AblyChat/RoomStatus.swift (1)
Line range hint
4-13: Implementation aligns well with CHADR-062 requirements.The
RoomStatusenum effectively replacesRoomLifecycle, maintaining proper error handling withARTErrorInfoand providing a comprehensive set of states for room lifecycle management. The implementation is clean, well-documented, and follows Swift best practices.Tests/AblyChatTests/DefaultRoomTests.swift (3)
Line range hint
6-69: Well-structured attach tests with good coverage!The tests effectively verify both success and failure scenarios for the attach operation. The changes align well with the new API design, removing intermediate Status objects. Good use of the Arrange-Act-Assert pattern and comprehensive error handling.
Line range hint
70-132: Clean and symmetric detach tests!The detach tests maintain a consistent structure with the attach tests, providing good coverage for both success and failure scenarios. The changes properly reflect the new API design.
136-140: Good baseline test for initial room status!The test effectively verifies that a new room starts in the initialized state, establishing a clear baseline for the room lifecycle.
Example/AblyChatExample/ContentView.swift (1)
Line range hint
173-189: LGTM! Status handling changes align with API updates.The implementation correctly adopts the new
onStatusChangemethod, replacing the previousstatus.onChangeapproach. The status display logic remains robust with appropriate UI feedback.Let's verify that all status.onChange usage has been migrated:
✅ Verification successful
Migration to
onStatusChangeis complete and consistentThe verification confirms:
- No instances of the old
status.onChangepattern remain in the codebase- The new
onStatusChangeAPI is consistently used across:
- Main implementation in
Room.swift- Tests in
DefaultRoomTests.swift- Example app in
ContentView.swift- Mock implementations in
MockClients.swift🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining old status.onChange usage that might have been missed rg "status\.onChange" # Search for new onStatusChange usage to ensure consistent adoption rg "onStatusChange"Length of output: 1466
Sources/AblyChat/RoomLifecycleManager.swift (4)
Line range hint
160-182: LGTM: Status mapping is complete and accurate.The mapping from internal
Statusto publicRoomStatusis comprehensive and correctly implements the API changes outlined in CHADR-062.
267-268: LGTM: Room status getter is correctly implemented.The getter properly exposes the internal status through the public API using the status mapping.
282-282: LGTM: Status change event creation is consistent.The status change event correctly uses the status mapping for both current and previous states.
748-748: LGTM: Failed status check is properly implemented.The check for failed status correctly uses the public API through toRoomStatus, maintaining consistency with the API changes.
Tests/AblyChatTests/RoomLifecycleManagerTests.swift (5)
101-101: LGTM! Property rename aligns with API changes.The change from
currenttoroomStatuscorrectly reflects the API changes outlined in CHADR-062.
Line range hint
109-242: LGTM! Consistent terminology updates in attach operation tests.The changes correctly update all state-related terminology to status, maintaining consistency with the new API design. The test coverage remains comprehensive while adapting to the new naming conventions.
Line range hint
469-579: LGTM! Detach operation tests properly updated.The changes maintain test coverage while correctly implementing the new status-based terminology throughout the detach operation tests.
Line range hint
671-870: LGTM! Release operation tests successfully updated.The changes correctly implement the new status-based terminology throughout the release operation tests while maintaining comprehensive test coverage.
Line range hint
1034-1270: LGTM! Contributor update tests properly adapted.The changes successfully implement the new status-based terminology throughout the contributor update tests while maintaining test coverage.
Sources/AblyChat/Room.swift (2)
22-30: Introduction ofRoomStatusChangestruct enhances status trackingThe addition of the
RoomStatusChangestruct provides a clear and structured way to represent status transitions, improving the observability and maintainability of the room status changes.
14-14:⚠️ Potential issueRemove outdated TODO comment
The TODO comment at line 14 suggests changing to
status, but thestatusproperty has already been updated accordingly. Consider removing the TODO comment as it is no longer necessary.Apply this diff to remove the outdated TODO comment:
- // TODO: change to `status`⛔ Skipped due to learnings
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#9 File: Sources/AblyChat/RoomStatus.swift:3-33 Timestamp: 2024-08-28T13:01:11.701Z Learning: When reviewing code, avoid commenting on TODOs that already have a linked GitHub issue, as per the user's preference.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#9 File: Sources/AblyChat/RoomStatus.swift:3-33 Timestamp: 2024-10-08T15:58:47.376Z Learning: When reviewing code, avoid commenting on TODOs that already have a linked GitHub issue, as per the user's preference.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#77 File: Sources/AblyChat/Room.swift:14-14 Timestamp: 2024-10-01T21:58:50.246Z Learning: In the codebase, the term `status` now has a new meaning, and its usage is intentional.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#77 File: Sources/AblyChat/Room.swift:14-14 Timestamp: 2024-10-08T15:58:47.376Z Learning: In the codebase, the term `status` now has a new meaning, and its usage is intentional.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Sources/AblyChat/RoomLifecycleManager.swift:235-237 Timestamp: 2024-10-08T15:58:47.376Z Learning: When code includes a TODO comment referencing an issue, it indicates that the issue is being tracked, and no further action is needed.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Sources/AblyChat/RoomLifecycleManager.swift:235-237 Timestamp: 2024-10-01T19:47:18.144Z Learning: When code includes a TODO comment referencing an issue, it indicates that the issue is being tracked, and no further action is needed.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:941-941 Timestamp: 2024-10-01T19:46:16.467Z 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#67 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:836-836 Timestamp: 2024-10-08T15:58:47.376Z Learning: Before suggesting the removal of TODO comments, check if they reference existing GitHub issues that track the pending work.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#67 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:941-941 Timestamp: 2024-10-08T15:58:47.376Z 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#67 File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:836-836 Timestamp: 2024-10-01T19:46:04.461Z Learning: Before suggesting the removal of TODO comments, check if they reference existing GitHub issues that track the pending work.Example/AblyChatExample/Mocks/MockClients.swift (1)
87-89: LGTM!The implementation of
onStatusChangecorrectly provides a subscription toRoomStatusChangeand aligns with the updated API.
lawrence-forooghian
force-pushed
the
48-transient-disconnect-timeouts
branch
3 times, most recently
from
1118faa
to
19c1a27
Compare
lawrence-forooghian
force-pushed
the
address-duplicate-spec-points
branch
from
b539224
to
20f21c7
Compare
|
Will rebase and fix conflicts. |
Missed these in d51887c.
We apply the changes that we decided upon in [1]; that is, we: - remove the RoomStatus protocol, moving its functionality directly into the Room protocol, and rename the `current` property to `status`; - rename the RoomLifecycle enum to RoomStatus As part of this, I’ve also corrected references to room “state” to the correct terminology of “status” (in [2] Andy explains why they used the word “status”, choosing to reserve “state” for some larger composite state). [1] https://ably.atlassian.net/wiki/spaces/CHA/pages/3410526209/CHADR-062+Renaming+Lifecycle+Types+for+Clarity [2] #73 (comment)
It was called `current` in 25e5052 to match the naming used in the public API at the time. Post-7e4460d, calling it `roomStatus` makes more sense (or even `status` but that’s already taken by a private property).
As we did for Room in 7e4460d (a lot fewer changes here, though, because we haven’t implemented Connection yet).
lawrence-forooghian
force-pushed
the
73-change-some-api-names
branch
from
965a740
to
b771238
Compare
|
OK, fixed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
Sources/AblyChat/RoomStatus.swift (3)
3-4: Remove outdated TODO comment.The TODO comment appears to be outdated since the renaming from
RoomLifecycletoRoomStatushas already been implemented as per the PR objectives.-// TODO: rename public enum RoomStatus: Sendable, Equatable {
Line range hint
15-19: Consider improving documentation formatting.While the documentation content is excellent, its readability could be enhanced with proper Swift documentation formatting.
- // Helpers to allow us to test whether a `RoomStatus` value has a certain case, without caring about the associated value. These are useful for in contexts where we want to use a `Bool` to communicate a case. For example: - // - // 1. testing (e.g. `#expect(status.isFailed)`) - // 2. testing that a status does _not_ have a particular case (e.g. if !status.isFailed), which a `case` statement cannot succinctly express + /// Helpers to allow us to test whether a `RoomStatus` value has a certain case, + /// without caring about the associated value. These are useful in contexts where + /// we want to use a `Bool` to communicate a case. For example: + /// + /// 1. Testing: `#expect(status.isFailed)` + /// 2. Negative testing: `if !status.isFailed` + /// + /// The latter is particularly useful when a `case` statement cannot succinctly + /// express the negative condition.
Line range hint
21-44: Simplify boolean properties using Swift pattern matching.The current implementation can be made more concise using Swift's pattern matching capabilities.
public var isAttaching: Bool { - if case .attaching = self { - true - } else { - false - } + case .attaching = self } public var isSuspended: Bool { - if case .suspended = self { - true - } else { - false - } + case .suspended = self } public var isFailed: Bool { - if case .failed = self { - true - } else { - false - } + case .failed = self }Example/AblyChatExample/Mocks/MockClients.swift (2)
67-68: Consider usingletforstatusif it remains immutableIf the
statusproperty does not change after initialization, declaring it withletcan prevent accidental modifications.
87-89: Manage subscriptions withinonStatusChangeto prevent resource buildupEach call to
onStatusChangecreates a new subscription added tomockSubscriptions, potentially leading to multiple unmanaged subscriptions. This could strain resources over time.Consider reusing existing subscriptions or providing a mechanism to clean up unused subscriptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
Example/AblyChatExample/ContentView.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Sources/AblyChat/Connection.swift(2 hunks)Sources/AblyChat/Room.swift(4 hunks)Sources/AblyChat/RoomLifecycleManager.swift(4 hunks)Sources/AblyChat/RoomStatus.swift(2 hunks)Tests/AblyChatTests/DefaultRoomStatusTests.swift(0 hunks)Tests/AblyChatTests/DefaultRoomTests.swift(7 hunks)Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift(1 hunks)Tests/AblyChatTests/RoomLifecycleManagerTests.swift(32 hunks)
💤 Files with no reviewable changes (1)
- Tests/AblyChatTests/DefaultRoomStatusTests.swift
🚧 Files skipped from review as they are similar to previous changes (6)
- Example/AblyChatExample/ContentView.swift
- Sources/AblyChat/Connection.swift
- Sources/AblyChat/Room.swift
- Sources/AblyChat/RoomLifecycleManager.swift
- Tests/AblyChatTests/Helpers/RoomLifecycle+Error.swift
- Tests/AblyChatTests/RoomLifecycleManagerTests.swift
🧰 Additional context used
📓 Learnings (2)
Sources/AblyChat/RoomStatus.swift (2)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#77
File: Sources/AblyChat/Room.swift:14-14
Timestamp: 2024-10-08T15:58:47.376Z
Learning: In the codebase, the term `status` now has a new meaning, and its usage is intentional.
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#77
File: Sources/AblyChat/Room.swift:14-14
Timestamp: 2024-10-01T21:58:50.246Z
Learning: In the codebase, the term `status` now has a new meaning, and its usage is intentional.
Tests/AblyChatTests/DefaultRoomTests.swift (2)
Learnt from: lawrence-forooghian
PR: ably-labs/ably-chat-swift#54
File: Tests/AblyChatTests/RoomLifecycleManagerTests.swift:627-631
Timestamp: 2024-10-01T12:55:21.968Z
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:627-631
Timestamp: 2024-10-08T15:58:47.376Z
Learning: The closure `detachImpl` in `RoomLifecycleManagerTests.swift` functions as intended without external state variables to track invocation count.
🔇 Additional comments (6)
Sources/AblyChat/RoomStatus.swift (1)
Line range hint 5-13: LGTM! Well-structured room status states.
The enum cases effectively capture all possible room states with appropriate error handling where needed. The naming is clear and follows a logical progression.
Tests/AblyChatTests/DefaultRoomTests.swift (5)
6-7: LGTM! Good test organization.
The addition of MARK comments improves the test file's organization and readability by clearly separating different test categories.
Also applies to: 70-71, 134-135
Line range hint 24-35: LGTM! Comprehensive status transition testing.
The changes properly verify both the final status and the transition events during the attach operation. The use of onStatusChange aligns with the new API design.
Line range hint 88-99: LGTM! Consistent test pattern.
The detach tests mirror the attach tests' structure, maintaining consistency in how status transitions are verified.
136-146: LGTM! Clear initial state verification.
The test effectively verifies that a new room starts in the initialized state.
148-177:
Fix hardcoded status assertion in transition test.
The test comprehensively verifies status transitions and subscriber notifications. However, there's a hardcoded status assertion at the end that should use the dynamic status value.
Apply this diff to use the dynamic status value:
- #expect(await room.status == .attached)
+ #expect(await room.status == newStatus)
Implements the changes to the public API which we decided in CHADR-062. Highlights:
Statusobjects forRoomandConnection, putting access to the status directly on the room / connection itself*Lifecycleenums to*StatusSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation