[ECO-5147][CHA-RC3] Fix shared channel options for messages feature #75
Conversation
WalkthroughThe pull request introduces significant refactoring across several classes in the chat-android module, particularly focusing on the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (4)
👮 Files not reviewed due to content moderation or server errors (3)
🧰 Additional context used📓 Learnings (1)chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt (1)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 (
|
sacOO7
changed the title
Code Coverage
|
sacOO7
force-pushed
the
feature/shared-channel-modes-options
branch
from
09b8306 to
900e607
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: 1
🧹 Outside diff range and nitpick comments (1)
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1)
38-46: Consider adding tests for shared channel optionsWhile the setup is cleaner, consider adding test cases to verify that channel options are properly merged when shared features are enabled.
Example test case:
@Test fun `should merge channel options from shared features`() = runTest { // Given room with multiple features enabled val room = createMockRoom( "room1", options = RoomOptions( presence = PresenceOptions(), occupancy = OccupancyOptions() ), realtimeClient = realtimeClient, chatApi = chatApi ) val messages = DefaultMessages(room) // When getting channel options val channelOptions = messages.channel.options // Then options should include modes from all features assertEquals(true, channelOptions.modes.contains("presence")) assertEquals(true, channelOptions.modes.contains("occupancy")) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
chat-android/src/main/java/com/ably/chat/Messages.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Occupancy.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(1 hunks)chat-android/src/main/java/com/ably/chat/RoomOptions.kt(2 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/room/RoomTestHelpers.kt(1 hunks)
🧰 Additional context used
📓 Learnings (1)
chat-android/src/main/java/com/ably/chat/Occupancy.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Occupancy.kt:145-145
Timestamp: 2024-11-28T11:11:20.423Z
Learning: In `chat-android/src/main/java/com/ably/chat/Occupancy.kt`, within the `DefaultOccupancy` class, when methods use `room.chatApi`, which utilizes the REST API, there's no need to call `room.ensureAttached()` before performing the operation.
🔇 Additional comments (12)
chat-android/src/main/java/com/ably/chat/Messages.kt (5)
7-7: Import statement is appropriate
The import of io.ably.lib.realtime.Channel is necessary for the updated channel property.
214-215: Consolidating constructor parameters improves encapsulation
Passing the DefaultRoom instance to DefaultMessages simplifies the constructor and enhances encapsulation by reducing parameter clutter.
223-230: Initializing properties from room improves cohesion
Using properties directly from the room instance ensures consistency and reduces redundancy in accessing shared resources like roomId, chatApi, and realtimeChannels.
237-237: Channel name construction is correct
The messagesChannelName is correctly constructed using the roomId and the chat message identifiers.
239-239: Channel initialization with merged options is appropriate
Initializing channel with room.options.messagesChannelOptions() ensures that channel options are properly merged for shared features like presence and occupancy.
chat-android/src/main/java/com/ably/chat/RoomOptions.kt (2)
95-95: Changing validateRoomOptions visibility to internal improves encapsulation
Making validateRoomOptions internal restricts its accessibility to within the module, enhancing encapsulation and preventing unintended external usage.
103-127: Addition of messagesChannelOptions method enhances channel configuration
The new messagesChannelOptions method correctly merges channel options from presence and occupancy settings, facilitating the configuration of shared channels.
chat-android/src/test/java/com/ably/chat/room/RoomTestHelpers.kt (1)
101-101: Refactored instantiation of DefaultMessages aligns with updated constructor
Passing the room directly to DefaultMessages simplifies the test setup and ensures consistency with the updated constructor in the main codebase.
chat-android/src/main/java/com/ably/chat/Occupancy.kt (1)
86-86: Utilizing shared channel for occupancy aligns with shared channel strategy
Assigning room.messages.channel to the channel property ensures that DefaultOccupancy uses the shared channel, promoting consistency across features and adhering to the shared channel design.
chat-android/src/main/java/com/ably/chat/Room.kt (1)
130-130: LGTM: Simplified DefaultMessages initialization improves design
The simplified constructor reduces parameter coupling and allows DefaultMessages to access room configuration directly, which is essential for proper shared channel options handling.
Let's verify that DefaultMessages is using the room parameter correctly:
✅ Verification successful
Verified: DefaultMessages correctly uses room parameter for channel configuration
The implementation shows that DefaultMessages properly uses the room parameter to access channel options through room.options.messagesChannelOptions() when initializing the channel. This confirms that the simplified constructor change is working as intended, allowing DefaultMessages to access room configuration directly for proper shared channel options handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DefaultMessages implementation uses room parameter
# Expected: DefaultMessages should access room.options for channel configuration
ast-grep --pattern 'class DefaultMessages {
$$$
val channel = $room.$$$
$$$
}'
Length of output: 86
Script:
#!/bin/bash
# Let's try a broader search for DefaultMessages class implementation
ast-grep --pattern 'class DefaultMessages($$$) {
$$$
}'
# Also search for any usage of room parameter within DefaultMessages
rg "class DefaultMessages.*\{" -A 20
Length of output: 98
Script:
#!/bin/bash
# Let's try finding the DefaultMessages file first
fd "DefaultMessages.kt"
# Then search for room usage in all Kotlin files
rg "DefaultMessages.*room.*" --type kotlin
Length of output: 422
Script:
#!/bin/bash
# Search for DefaultMessages implementation in chat-android/src/main/java/
rg "class DefaultMessages" -A 10 --type kotlin chat-android/src/main/java/
# Also search for Messages interface to understand the contract
rg "interface Messages" -A 10 --type kotlin chat-android/src/main/java/
Length of output: 4107
Script:
#!/bin/bash
# Let's check how DefaultMessages uses the room parameter to access channel or options
rg "val channel.*=.*" --type kotlin chat-android/src/main/java/com/ably/chat/Messages.kt -A 5
# Also check for any room.options usage
rg "room\.options" --type kotlin chat-android/src/main/java/com/ably/chat/Messages.kt
Length of output: 590
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (2)
3-6: LGTM: Improved test setup with centralized mock utilities
The use of mock creation utilities improves test maintainability and consistency.
Also applies to: 30-31
Line range hint 49-249: LGTM: Existing test coverage remains comprehensive
The test suite maintains good coverage of core messaging functionality including message sending, subscription handling, and validation rules.
chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt
Show resolved
Hide resolved
1. Updated DefaultMessages constructor to accept 'room` as the only argument 2. Updated DefaultMessages.channel to accept merged options across shared features 3. Added extention method to 'RoomOptions', messagesChannelOptions, merged modes from all shared channels ( presence and occupancy ) 4. Fixed unit/integration tests related to Messages feature 5. Added test to check shared channel is accessed only once with combined modes+params
sacOO7
force-pushed
the
feature/shared-channel-modes-options
branch
from
900e607 to
73333df
Compare
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests