Add an “all features enabled” room option #170
Conversation
|
Warning Rate limit exceeded@lawrence-forooghian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
CI failing due to #169. |
![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
🧹 Outside diff range and nitpick comments (1)
Example/AblyChatExample/ContentView.swift (1)
Documentation needs to be updated for the new mock case
The codebase inspection reveals that while there is inline documentation for
RoomOptionsand its components, the newly addedmockcase in the enum is not properly documented in:
- The README.md which should mention the mock functionality for testing/development
- The inline documentation in the source file where the enum is defined
Specific locations that need documentation updates:
- Add mock functionality description in README.md under a new "Development and Testing" section
- Add XML documentation for the
mockcase in the enum definition, explaining its purpose and usage🔗 Analysis chain
Line range hint
9-15: Verify documentation coverage for the new APIWhile the implementation looks good, let's ensure all relevant documentation is updated to reflect this new option.
Let me check the README.md content to verify the documentation coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential documentation files that might need updates echo "Searching for documentation files..." fd -e md -e txt . | grep -i -E 'doc|guide|readme|changelog' # Search for other potential usages of RoomOptions that might benefit from this change echo "Searching for other RoomOptions usage..." rg "RoomOptions" --type swiftLength of output: 4810
Script:
#!/bin/bash # Check README.md content for RoomOptions documentation echo "Checking README content..." cat README.md # Check if there are any inline documentation comments for RoomOptions echo -e "\nChecking inline documentation..." rg -A 5 "RoomOptions" --type swift Sources/AblyChat/RoomOptions.swiftLength of output: 3171
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Example/AblyChatExample/ContentView.swift(1 hunks)Sources/AblyChat/RoomOptions.swift(1 hunks)
🔇 Additional comments (2)
Sources/AblyChat/RoomOptions.swift (1)
9-15: Well-structured implementation with clear naming!
The new allFeaturesEnabled static property accurately reflects its purpose of enabling all room features with their default settings. The naming choice is better than "defaults" as it clearly communicates the intent to users.
Example/AblyChatExample/ContentView.swift (1)
59-59: Clean implementation demonstrating the new API!
The simplified room initialization provides a good example of how to use the new allFeaturesEnabled option, making the code more maintainable while serving as clear documentation for SDK users.
lawrence-forooghian
force-pushed
the
add-all-features-enabled-room-options
branch
from
2fe43db to
edae406
Compare
lawrence-forooghian
changed the base branch from
main
to
119-CHA-RL1h5-asynchronously
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (16)
Tests/AblyChatTests/Mocks/MockRoom.swift (2)
56-61: Consider using a precondition instead of fatalErrorThe implementation looks good, but consider using
preconditioninstead offatalErrorfor the guard clause. This would make the failure condition more explicit as a test requirement.- fatalError("releaseImplementation must be set before calling `release`") + precondition(releaseImplementation != nil, "releaseImplementation must be set before calling `release`")
63-68: Enhance documentation with usage exampleThe documentation is clear but could be more helpful with a usage example. Consider adding an example showing how to consume this sequence in tests.
/// Emits an element each time ``release()`` is called. +/// +/// Example usage: +/// ```swift +/// for await _ in mockRoom.releaseCallsAsyncSequence { +/// // Handle release call +/// } +/// ```Sources/AblyChat/Errors.swift (5)
57-90: Add explicitreturnstatements in computed properties for clarityIn the
toNumericErrorCodecomputed property, the switch cases lack explicitreturnstatements. While Swift allows implicit returns in single-expression closures and computed properties, addingreturnstatements can improve readability and prevent potential confusion.Apply the following diff to add
returnstatements:internal var toNumericErrorCode: ErrorCode { switch self { case .inconsistentRoomOptions: - .inconsistentRoomOptions + return .inconsistentRoomOptions case .messagesAttachmentFailed: - .messagesAttachmentFailed + return .messagesAttachmentFailed case .presenceAttachmentFailed: - .presenceAttachmentFailed + return .presenceAttachmentFailed // ... (continue for each case) } }Consider making similar changes in other computed properties and functions where implicit returns are used.
92-115: Ensure explicitreturnstatements instatusCodecomputed propertyIn the
statusCodecomputed property ofCaseThatImpliesFixedStatusCode, the switch cases lack explicitreturnstatements. Adding them enhances clarity.Apply the following diff:
internal var statusCode: Int { switch self { case .inconsistentRoomOptions, .roomInFailedState, .roomIsReleasing, .roomIsReleased, .roomReleasedBeforeOperationCompleted: - 400 + return 400 case .messagesAttachmentFailed, .presenceAttachmentFailed, .reactionsAttachmentFailed, .occupancyAttachmentFailed, .typingAttachmentFailed, .messagesDetachmentFailed, .presenceDetachmentFailed, .reactionsDetachmentFailed, .occupancyDetachmentFailed, .typingDetachmentFailed: - 500 + return 500 } }
130-146: Add explicitreturnstatements incodecomputed propertyIn the
codecomputed property ofErrorCodeAndStatusCode, explicitreturnstatements improve readability.Apply the following diff:
internal var code: ErrorCode { switch self { case let .fixedStatusCode(code): - code.toNumericErrorCode + return code.toNumericErrorCode case let .variableStatusCode(code, _): - code.toNumericErrorCode + return code.toNumericErrorCode } }
147-154: Add explicitreturnstatements instatusCodecomputed propertySimilarly, in the
statusCodecomputed property, addreturnstatements.Apply the following diff:
internal var statusCode: Int { switch self { case let .fixedStatusCode(code): - code.statusCode + return code.statusCode case let .variableStatusCode(_, statusCode): - statusCode + return statusCode } }
215-218: Consider handling optional room status change gracefullyIn
waitToBeAbleToPerformPresenceOperations, the code comments mention uncertainty about handling anilnextRoomStatusChange. Consider adding a guard statement or error handling to manage potentialnilvalues explicitly.For example:
guard let statusChange = nextRoomStatusChange else { throw ARTErrorInfo(chatError: .roomTransitionedToInvalidStateForPresenceOperation(cause: nil)) }Tests/AblyChatTests/DefaultRoomsTests.swift (3)
133-154: Ensure consistent error handling in testget_whenRoomExistsInRoomMap_throwsErrorWhenOptionsDoNotMatchThe test correctly expects an
inconsistentRoomOptionserror. However, consider adding assertions for the status code to ensure comprehensive testing of error objects.Update the assertion to check for both the error code and status code:
await #expect { try await rooms.get(roomID: roomID, options: differentOptions) } throws: { error in isChatError(error, withCodeAndStatusCode: .fixedStatusCode(.inconsistentRoomOptions)) }
195-197: Remove unnecessary code after test completionAfter the test
get_whenFutureExistsInRoomMap_throwsErrorWhenOptionsDoNotMatch, the code includes a post-test section to allow the release operation to complete. If this does not impact other tests, consider removing it for clarity.-// Post-test: Allow the CHA-RC1g release operation to complete -roomReleaseOperation.complete()
298-349: Improve test coverage forrelease_withReleaseInProgress_failsPendingGetOperationsThe test effectively checks that pending
getoperations fail when a release is in progress. To enhance the test, consider verifying that the second release reuses the existing release operation and does not trigger a new release.Add assertions to confirm that:
- The
releaseoperation is not called again on the room.- The room state is correctly managed.
Sources/AblyChat/RoomLifecycleManager.swift (2)
Line range hint
195-254: Clarify state transitions in theStatusenumThe addition of new cases in the
Statusenum enhances error handling, but the transitions between states might benefit from additional comments or clarifications. This will help future maintainers understand the flow better.Consider adding documentation comments explaining when each state is used and how transitions occur between states.
Line range hint
754-772: Ensure proper error handling inscheduleAnOperationmethodWhen scheduling operations like
retryandrundown, make sure that any errors thrown are appropriately handled or propagated to prevent the application from entering an inconsistent state.Review the error handling within the tasks and consider adding do-catch blocks if necessary.
Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift (2)
486-511: Enhance the test forattach_whenAttachPutsChannelIntoFailedState_schedulesRundownOperationThe test checks that a
RUNDOWNoperation is scheduled when a contributor enters a failed state during attachment. To improve the test:
- Verify that the
RUNDOWNoperation detaches all non-failed contributors.- Confirm that the manager transitions to the correct final state.
Add assertions to check the detach call counts and the final manager status.
1346-1412: Add comments for clarity in theRUNDOWNoperation testsThe tests for the
RUNDOWNoperation are comprehensive but can benefit from additional inline comments explaining each step. This aids readability and understanding for future contributors.Include comments that describe the purpose of each section within the tests, particularly around the expected outcomes and assertions.
Sources/AblyChat/RoomFeature.swift (1)
44-48: Consider enhancing error documentation with examplesWhile the documentation updates clearly describe the error conditions, it would be helpful to add code examples demonstrating each error scenario.
Consider adding examples like:
// Example for CHA-PR3d/CHA-RL9: try await channel.waitToBeAbleToPerformPresenceOperations(requestedByFeature: .presence) // Throws ChatError.roomTransitionedToInvalidStateForPresenceOperation when room transitions from ATTACHING to FAILED // Example for CHA-PR3e: // Returns immediately when room is ATTACHED // Example for CHA-PR3h: // Throws ChatError.presenceOperationRequiresRoomAttach when room is DETACHEDSources/AblyChat/DefaultMessages.swift (1)
237-244: Enhance error logging for better debuggingThe error handling implementation looks good with the new
ErrorCode.CaseThatImpliesFixedStatusCode.messagesAttachmentFailedcase. However, consider including more context in the error message for better debugging.Consider this enhancement:
let errorCodeCase = ErrorCode.CaseThatImpliesFixedStatusCode.messagesAttachmentFailed nillableContinuation?.resume( throwing: ARTErrorInfo.create( withCode: errorCodeCase.toNumericErrorCode.rawValue, status: errorCodeCase.statusCode, - message: "Channel failed to attach" + message: "Channel failed to attach. Current state: \(stateChange.current), Previous state: \(stateChange.previous)" ) )
🛑 Comments failed to post (1)
Sources/AblyChat/Rooms.swift (1)
241-244:
⚠️ Potential issueHandle potential errors from
createRoomappropriatelyIn the
createRoommethod, ifroomFactory.createRoomthrows an error, the room state won't be updated, and the error may not be propagated correctly. Ensure that errors are properly handled and the room state is consistent.Consider wrapping the room creation in a do-catch block and updating the state accordingly:
do { let room = try await roomFactory.createRoom(...) roomStates[roomID] = .roomMapEntry(.created(room: room)) return room } catch { // Handle error, possibly update roomStates or propagate the error throw error }
lawrence-forooghian
force-pushed
the
add-all-features-enabled-room-options
branch
from
edae406 to
3c46c60
Compare
lawrence-forooghian
changed the base branch from
119-CHA-RL1h5-asynchronously
to
main
In 20e7f5f I said > RoomOptionsDefaults in JS is instead implemented here by giving the > *Options types a no-args initializer that populates the default values. but I had not noticed that JS’s RoomOptionsDefaults is not actually a “default”, since the JS SDK does not even have a concept of a “default” RoomOptions. What it actually is is a RoomOptions in which all of the room features are switched on; we discussed this today in standup and Andy said that the intention of this value was to give users who are just playing around with the SDK an easy way to turn all features on. So, here, I add such an API for Swift, but with a name that more accurately describes its intention. Andy’s created [1] to revisit the JS naming. [1] https://ably.atlassian.net/browse/CHA-766
lawrence-forooghian
force-pushed
the
add-all-features-enabled-room-options
branch
from
3c46c60 to
c595df1
Compare
In 20e7f5f I said
but I had not noticed that JS’s RoomOptionsDefaults is not actually a “default”, since the JS SDK does not even have a concept of a “default” RoomOptions. What it actually is is a RoomOptions in which all of the room features are switched on; we discussed this today in standup and Andy said that the intention of this value was to give users who are just playing around with the SDK an easy way to turn all features on. So, here, I add such an API for Swift, but with a name that more accurately describes its intention. Andy’s created [1] to revisit the JS naming.
[1] https://ably.atlassian.net/browse/CHA-766
Summary by CodeRabbit
New Features
allFeaturesEnabledoption for room configurations.Bug Fixes