[ECO-4936] Implement the ability to fetch a room #33
Conversation
WalkthroughThe changes introduce a structured error handling mechanism in the Ably Chat SDK, alongside a new protocol for room management. The implementation of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DefaultRooms
participant Storage
participant DefaultRoom
User->>DefaultRooms: get(roomID, options)
DefaultRooms->>Storage: get(roomID, options)
Storage->>DefaultRooms: return room or create new DefaultRoom
DefaultRooms->>User: return DefaultRoom instance
Assessment against linked issues
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 (
|
lawrence-forooghian
force-pushed
the
19-fetch-room
branch
from
a1e16c6
to
5a15ddc
Compare
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- Sources/AblyChat/Errors.swift (1 hunks)
- Sources/AblyChat/Room.swift (2 hunks)
- Sources/AblyChat/RoomOptions.swift (3 hunks)
- Sources/AblyChat/Rooms.swift (1 hunks)
- Tests/AblyChatTests/AblyChatTests.swift (1 hunks)
- Tests/AblyChatTests/DefaultRoomsTests.swift (1 hunks)
- Tests/AblyChatTests/Helpers/Helpers.swift (1 hunks)
- Tests/AblyChatTests/Mocks/MockRealtime.swift (1 hunks)
Additional comments not posted (19)
Tests/AblyChatTests/AblyChatTests.swift (1)
6-6: LGTM!The change to use
MockRealtime.create()enhances the construction ofMockRealtime, possibly indicating a more complex setup or configuration.Tests/AblyChatTests/Helpers/Helpers.swift (1)
1-15: LGTM!The helper function
assertIsChatErroris well-implemented and follows best practices for error assertions in unit tests.Sources/AblyChat/RoomOptions.swift (5)
Line range hint
3-12: LGTM!The addition of
Equatableenhances the usability of theRoomOptionsstruct in contexts where equality checks are necessary.
Line range hint
17-25: LGTM!The addition of
Equatableenhances the usability of thePresenceOptionsstruct in contexts where equality checks are necessary.
27-33: LGTM!The addition of
Equatableenhances the usability of theTypingOptionsstruct in contexts where equality checks are necessary.
35-37: LGTM!The addition of
Equatableenhances the usability of theRoomReactionsOptionsstruct in contexts where equality checks are necessary.
39-41: LGTM!The addition of
Equatableenhances the usability of theOccupancyOptionsstruct in contexts where equality checks are necessary.Tests/AblyChatTests/Mocks/MockRealtime.swift (1)
18-25: LGTM!The
createmethod is correctly implemented and enhances the usability of theMockRealtimeclass.Sources/AblyChat/Room.swift (2)
1-1: LGTM!The
@preconcurrencyimport statement is correctly used to indicate that the imported module does not support concurrency.
Line range hint
2-18: LGTM!The
Roomprotocol is correctly defined and provides a clear contract for room implementations.Sources/AblyChat/Rooms.swift (2)
1-1: LGTM!The
@preconcurrencyimport statement is correctly used to indicate that the imported module does not support concurrency.
2-8: LGTM!The
Roomsprotocol is correctly defined and provides a clear contract for room implementations.Tests/AblyChatTests/DefaultRoomsTests.swift (3)
6-21: LGTM!The test function
test_get_returnsRoomWithGivenIDis well-written and covers the expected behavior.
24-38: LGTM!The test function
test_get_returnsExistingRoomWithGivenIDis well-written and covers the expected behavior.
41-63: LGTM!The test function
test_get_throwsErrorWhenOptionsDoNotMatchis well-written and covers the expected behavior.Sources/AblyChat/Errors.swift (4)
1-8: LGTM!The error domain definition is correct and follows best practices.
10-26: LGTM! But revisit the TODO comment.The
ErrorCodeenum is well-defined. However, the TODO comment indicates that the error code and status code are guesses and need to be revisited. Ensure that this is tracked and addressed in the future.
29-52: LGTM!The
ChatErrorenum is well-defined and provides useful metadata for the error.
54-67: LGTM! But revisit the TODO comment.The extension for
ARTErrorInfois well-defined. However, the TODO comment indicates that the implementation needs to be revisited once the related issue inably-cocoais resolved. Ensure that this is tracked and addressed in the future.
lawrence-forooghian
force-pushed
the
19-fetch-room
branch
from
5a15ddc
to
6ab6ad3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- Sources/AblyChat/Errors.swift (1 hunks)
- Sources/AblyChat/Room.swift (2 hunks)
- Sources/AblyChat/RoomOptions.swift (3 hunks)
- Sources/AblyChat/Rooms.swift (1 hunks)
- Tests/AblyChatTests/AblyChatTests.swift (1 hunks)
- Tests/AblyChatTests/DefaultRoomsTests.swift (1 hunks)
- Tests/AblyChatTests/Helpers/Helpers.swift (1 hunks)
- Tests/AblyChatTests/Mocks/MockRealtime.swift (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- Sources/AblyChat/Errors.swift
- Sources/AblyChat/Room.swift
- Sources/AblyChat/RoomOptions.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
- Tests/AblyChatTests/Helpers/Helpers.swift
- Tests/AblyChatTests/Mocks/MockRealtime.swift
Additional context used
Learnings (1)
Sources/AblyChat/Rooms.swift (1)
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#33 File: Sources/AblyChat/Rooms.swift:9-64 Timestamp: 2024-08-29T10:55:26.337Z Learning: When reviewing code, do not ask lawrence-forooghian about "not yet implemented" `fatalError` calls.
Additional comments not posted (4)
Tests/AblyChatTests/AblyChatTests.swift (1)
6-6: LGTM! But verify theMockRealtime.create()method.The change in the instantiation method for
MockRealtimesuggests an enhancement in the setup or configuration encapsulated within thecreate()method. Ensure that thecreate()method aligns with the intended behavior of the test.The code changes are approved.
Run the following script to verify the
MockRealtime.create()method:Verification successful
Verification Successful:
MockRealtime.create()Method ExistsThe
MockRealtime.create()method is correctly implemented in theTests/AblyChatTests/Mocks/MockRealtime.swiftfile. It returns an instance ofMockRealtimeinitialized with an empty key, which aligns with the intended behavior for testing. No further action is needed.
- File:
Tests/AblyChatTests/Mocks/MockRealtime.swift- Method:
static func create() -> MockRealtimeScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the `MockRealtime.create()` method. # Test: Search for the `create` method in the `MockRealtime` class. Expect: Correct implementation of the method. rg --type swift -A 5 $'class MockRealtime {\n static func create() -> MockRealtime'Length of output: 395
Script:
#!/bin/bash # Description: Verify the implementation of the `MockRealtime.create()` method. # Test: Search for the `create` method in the `MockRealtime` class. Expect: Correct implementation of the method. rg --type swift -A 5 $'class MockRealtime \\{\n static func create() -> MockRealtime'Length of output: 279
Script:
#!/bin/bash # Description: Verify the implementation of the `MockRealtime.create()` method. # Test: Search for the `create` method in the `MockRealtime` class. Expect: Correct implementation of the method. rg --type swift --multiline -A 5 $'class MockRealtime \\{\n static func create() -> MockRealtime'Length of output: 99
Script:
#!/bin/bash # Description: Search for the `MockRealtime` class and its static methods. # Search for the `MockRealtime` class definition. rg --type swift 'class MockRealtime' -A 10 # Search for static methods within the `MockRealtime` class. rg --type swift 'static func' -A 3Length of output: 3938
Sources/AblyChat/Rooms.swift (3)
2-7: LGTM!The
Roomsprotocol defines the necessary methods and properties for room management.The code changes are approved.
9-15: LGTM!The
DefaultRoomsclass is correctly implemented and conforms to theRoomsprotocol.The code changes are approved.
41-55: LGTM!The
get(roomID:options:)method correctly uses theStorageclass to obtain a room and includes a validation step for room options.The code changes are approved.
Part of #19. References to spec are based on [1] at commit aa7455d. I’ve decided to, for now, throw ably-cocoa’s ARTErrorInfo for consistency with JS; created #32 to revisit this later. We have decided (see [3]) that we’re going to try using actors as our mechanism for concurrency-safe management of mutable state. We accept that this will lead to more of the public API needing to be annotated as `async` (as has happened to Rooms.get here), which in some cases might lead to weird-looking API, and have chosen to accept this compromise in order to get the safety checking offered to us by the compiler, and because of developers’ aversion to writing "@unchecked Sendable". We might not have needed to make this compromise if we had access to Swift 6 / iOS 18’s Mutex type, which allows for synchronous management of mutable state in a way that the compiler is happy with. But, none of the decisions here need to be final; we can see how we feel about the API as it evolves and as our knowledge of the language grows. [1] ably/specification#200 [2] ably/ably-cocoa#1962 [3] #33 (comment)
lawrence-forooghian
force-pushed
the
19-fetch-room
branch
from
6ab6ad3
to
c8c85ce
Compare
|
I’ve pushed a new commit (before the original one) that implements the |
Part of #19. References to spec are based on [1] at commit aa7455d. I’ve decided to, for now, throw ably-cocoa’s ARTErrorInfo for consistency with JS; created #32 to revisit this later. We have decided (see [3]) that we’re going to try using actors as our mechanism for concurrency-safe management of mutable state. We accept that this will lead to more of the public API needing to be annotated as `async` (as has happened to Rooms.get here), which in some cases might lead to weird-looking API, and have chosen to accept this compromise in order to get the safety checking offered to us by the compiler, and because of developers’ aversion to writing "@unchecked Sendable". We might not have needed to make this compromise if we had access to Swift 6 / iOS 18’s Mutex type, which allows for synchronous management of mutable state in a way that the compiler is happy with. But, none of the decisions here need to be final; we can see how we feel about the API as it evolves and as our knowledge of the language grows. [1] ably/specification#200 [2] ably/ably-cocoa#1962 [3] #33 (comment)
lawrence-forooghian
force-pushed
the
19-fetch-room
branch
from
c8c85ce
to
a3fbf06
Compare
Part of #19. References to spec are based on [1] at commit aa7455d. I’ve decided to, for now, throw ably-cocoa’s ARTErrorInfo for consistency with JS; created #32 to revisit this later. We have decided (see [3]) that we’re going to try using actors as our mechanism for concurrency-safe management of mutable state. We accept that this will lead to more of the public API needing to be annotated as `async` (as has happened to Rooms.get here), which in some cases might lead to weird-looking API, and have chosen to accept this compromise in order to get the safety checking offered to us by the compiler, and because of developers’ aversion to writing "@unchecked Sendable". We might not have needed to make this compromise if we had access to Swift 6 / iOS 18’s Mutex type, which allows for synchronous management of mutable state in a way that the compiler is happy with. But, none of the decisions here need to be final; we can see how we feel about the API as it evolves and as our knowledge of the language grows. [1] ably/specification#200 [2] ably/ably-cocoa#1962 [3] #33 (comment)
lawrence-forooghian
force-pushed
the
19-fetch-room
branch
from
a3fbf06
to
983281a
Compare
Part of #19. References to spec are based on [1] at commit aa7455d. I’ve decided to, for now, throw ably-cocoa’s ARTErrorInfo for consistency with JS; created #32 to revisit this later. We have decided (see [2]) that we’re going to try using actors as our mechanism for concurrency-safe management of mutable state. We accept that this will lead to more of the public API needing to be annotated as `async` (as has happened to Rooms.get here), which in some cases might lead to weird-looking API, and have chosen to accept this compromise in order to get the safety checking offered to us by the compiler, and because of developers’ aversion to writing "@unchecked Sendable". We might not have needed to make this compromise if we had access to Swift 6 / iOS 18’s Mutex type, which allows for synchronous management of mutable state in a way that the compiler is happy with. But, none of the decisions here need to be final; we can see how we feel about the API as it evolves and as our knowledge of the language grows. [1] ably/specification#200 [2] #33 (comment)
lawrence-forooghian
force-pushed
the
19-fetch-room
branch
from
983281a
to
f8fc621
Compare
This is preparation for storing a realtime client inside the Chat SDK’s Sendable client types. The Sendable requirement added here will be satisfied by ably-cocoa’s ARTRealtime once [1] is resolved. [1] ably/ably-cocoa#1962
Part of #19.
Part of #19. References to spec are based on [1] at commit aa7455d. I’ve decided to, for now, throw ably-cocoa’s ARTErrorInfo for consistency with JS; created #32 to revisit this later. We have decided (see [2]) that we’re going to try using actors as our mechanism for concurrency-safe management of mutable state. We accept that this will lead to more of the public API needing to be annotated as `async` (as has happened to Rooms.get here), which in some cases might lead to weird-looking API, and have chosen to accept this compromise in order to get the safety checking offered to us by the compiler, and because of developers’ aversion to writing "@unchecked Sendable". We might not have needed to make this compromise if we had access to Swift 6 / iOS 18’s Mutex type, which allows for synchronous management of mutable state in a way that the compiler is happy with. But, none of the decisions here need to be final; we can see how we feel about the API as it evolves and as our knowledge of the language grows. [1] ably/specification#200 [2] #33 (comment)
lawrence-forooghian
force-pushed
the
19-fetch-room
branch
from
f8fc621
to
7d6acde
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- Example/AblyChatExample/ContentView.swift (1 hunks)
- Example/AblyChatExample/Mocks/MockRealtime.swift (1 hunks)
- Sources/AblyChat/ChatClient.swift (2 hunks)
- Sources/AblyChat/Errors.swift (1 hunks)
- Sources/AblyChat/Room.swift (2 hunks)
- Sources/AblyChat/RoomOptions.swift (3 hunks)
- Sources/AblyChat/Rooms.swift (1 hunks)
- Tests/AblyChatTests/DefaultChatClientTests.swift (1 hunks)
- Tests/AblyChatTests/DefaultRoomsTests.swift (1 hunks)
- Tests/AblyChatTests/Helpers/Helpers.swift (1 hunks)
- Tests/AblyChatTests/Mocks/MockRealtime.swift (1 hunks)
Files skipped from review as they are similar to previous changes (6)
- Sources/AblyChat/Errors.swift
- Sources/AblyChat/Room.swift
- Sources/AblyChat/RoomOptions.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
- Tests/AblyChatTests/Helpers/Helpers.swift
- Tests/AblyChatTests/Mocks/MockRealtime.swift
Additional context used
Learnings (1)
Sources/AblyChat/Rooms.swift (3)
Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#33 File: Sources/AblyChat/Rooms.swift:57-59 Timestamp: 2024-08-29T13:21:27.617Z Learning: The user, lawrence-forooghian, prefers not to be warned about `fatalError` calls for missing implementations. This preference should be respected in future reviews.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#33 File: Sources/AblyChat/Rooms.swift:9-64 Timestamp: 2024-08-29T10:55:26.337Z Learning: When reviewing code, do not ask lawrence-forooghian about "not yet implemented" `fatalError` calls.Learnt from: lawrence-forooghian PR: ably-labs/ably-chat-swift#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.
Additional comments not posted (15)
Example/AblyChatExample/ContentView.swift (1)
7-7: LGTM!Using a factory method
MockRealtime.create()improves encapsulation and maintainability.The code changes are approved.
Tests/AblyChatTests/DefaultChatClientTests.swift (2)
5-12: LGTM!The test correctly verifies that
DefaultChatClientuses default client options whenclientOptionsis nil.The code changes are approved.
14-26: LGTM!The test correctly verifies that the
roomsproperty ofDefaultChatClientreturns an instance ofDefaultRoomswith the samerealtimeclient andclientOptions.The code changes are approved.
Sources/AblyChat/Rooms.swift (5)
4-4: LGTM!Updating the
get(roomID:options:)method to be asynchronous aligns with modern concurrency practices in Swift.The code changes are approved.
9-20: LGTM!The
DefaultRoomsclass is correctly implemented and adheres to theRoomsprotocol.The code changes are approved.
22-37: LGTM!The
get(roomID:options:)method is correctly implemented and ensures consistency in room management.The code changes are approved.
39-41: Skipping comment on unimplemented method.The user prefers not to be warned about
fatalErrorcalls for missing implementations.
12-12: LGTM!The
clientOptionsproperty is correctly implemented and provides necessary access to client configuration options.The code changes are approved.
Example/AblyChatExample/Mocks/MockRealtime.swift (3)
4-4: LGTM!Marking the class as
finaland conforming toSendableis appropriate for a mock class.
9-11: LGTM!Triggering a
fatalErrorfor unimplemented properties in a mock class is appropriate.
19-26: LGTM!The
create()method provides a convenient way to create an instance without needing to provide a valid API key, which aligns with the current limitations of the example app.Sources/AblyChat/ChatClient.swift (4)
7-7: LGTM!Changing the
realtimeproperty type toRealtimeClientenhances type safety and clarity regarding the expected capabilities of the property.
11-11: LGTM!The typealias
RealtimeClientcombinesARTRealtimeProtocolandSendable, which is appropriate for therealtimeproperty.
13-28: LGTM!Transforming the class into an
actorsupports concurrency with isolated state management. The updates to the properties ensure proper initialization and nonisolated access where necessary.
42-45: LGTM!The
isEqualForTestPurposesmethod provides a way to compare instances for testing without requiring the struct to conform toEquatable.
|
I’ve pushed another commit to require that the realtime client conform to |
lawrence-forooghian
changed the title
Part of #19. References to spec are based on ably/specification#200 at commit
aa7455d.The
@preconcurrencyimports of ably-cocoa are temporary and will be removed once ably/ably-cocoa#1962 is done; created #31 for tracking.I’ve decided to, for now, throw ably-cocoa’s
ARTErrorInfofor consistency with JS; created #32 to revisit this later.https://ably.atlassian.net/browse/ECO-4936
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Roomprotocol andDefaultRoomclass to manage chat room functionalities.Roomsprotocol with a new implementation for better room instance management.Equatableconformance to various room option structs for easier equality checks.Bug Fixes
Tests
DefaultRoomsclass to validate room retrieval and error handling.