[ECO-5128] Fetch each channel once #134
Conversation
This was not caught, because we have no tests for the presence or occupancy features.
This code is not actually doing anything at the moment; the way in which we’re currently fetching channels means that these modes get overwritten, which we’ll fix in #132. But, when we fix the aforementioned, we’ll see that setting these flags leads to some weird Realtime behaviours, so we’ll turn them off for now and restore them separately in #133.
Based on spec at 28d09cc. Resolves #132.
WalkthroughThe pull request introduces significant modifications to the 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: 0
🧹 Outside diff range and nitpick comments (4)
Tests/AblyChatTests/DefaultRoomTests.swift (1)
28-47: Enhance test coverage for merged optionsWhile the test verifies basic channel fetching and occupancy parameter, it could be more comprehensive in testing the merging of options from presence and occupancy features.
Consider adding these assertions:
func fetchesEachChannelOnce() async throws { // ... existing setup ... let chatMessagesChannelGetOptions = try #require(channelsGetArguments.first { $0.name == "basketball::$chat::$chatMessages" }?.options) #expect(chatMessagesChannelGetOptions.params?["occupancy"] == "metrics") + + // Verify presence options are merged + #expect(chatMessagesChannelGetOptions.params?["presence"] != nil) + + // Verify channel is fetched exactly once + #expect(channelsGetArguments.filter { $0.name == "basketball::$chat::$chatMessages" }.count == 1) }Sources/AblyChat/Room.swift (3)
148-159: Offer assistance with presence channel setup codeThe presence-related channel options are commented out pending issue #133. If you need help in understanding the Realtime behavior or implementing the necessary code once the issue is resolved, I can assist in restoring and adapting this code.
Line range hint
198-221: Consider safer error handling instead offatalErrorin property accessorsUsing
fatalErrorin thepresence,reactions, andoccupancyproperty accessors will crash the application if these properties are accessed when the respective features are not enabled. Consider using a more graceful error handling approach, such as throwing an exception or returning an optional, to allow the caller to handle the absence of the feature without causing a crash.
Line range hint
212-214: Offer assistance in implementing thetypingfeatureThe
typingproperty currently raises afatalErrorindicating it's not yet implemented. If you'd like, I can help with implementing thetypingfeature or outline a plan to integrate this functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Sources/AblyChat/Room.swift(1 hunks)Tests/AblyChatTests/DefaultRoomTests.swift(1 hunks)
🔇 Additional comments (2)
Tests/AblyChatTests/DefaultRoomTests.swift (1)
45-46: Track the resolution of channel modes behavior
The TODO references issue #133 regarding Realtime behavior. This should be tracked and resolved to ensure complete test coverage.
Let's check the status of the referenced issue:
Sources/AblyChat/Room.swift (1)
135-172: Efficient refactoring of channel creation
The new method createChannelsForFeatures effectively consolidates the channel creation logic, ensuring that each channel is fetched only once with merged options. This aligns with the PR's objective of optimizing channel fetching and improves code maintainability.
Implements CHA-RC3a. See commit messages for more details.
Resolves #132.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Roomprotocol.