feat: some public API improvements #74
Conversation
WalkthroughThe pull request introduces significant modifications to the Ably Chat SDK for Android, primarily focusing on simplifying method calls related to sending messages and reactions. The changes involve removing parameter objects for these methods, allowing direct passing of parameters. Additionally, updates are made to the handling of metadata, including changes to data types and nullable properties. The Changes
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 (
|
Code Coverage
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (14)
chat-android/src/main/java/com/ably/chat/Metadata.kt (2)
Line range hint
6-17: Consider enhancing documentation with JsonElement examples.While the current documentation explains the purpose well, it would be helpful to add examples of valid JsonElement structures and common patterns.
Add examples like:
* messages. It is not used by Ably and is sent as part of the realtime * message payload. Example use cases are setting custom styling like * background or text colors or fonts, adding links to external images, * emojis, etc. +* +* Example valid metadata: +* { +* "style": { +* "color": "#ff0000", +* "font": "arial" +* }, +* "links": ["https://example.com/image.png"] +* } * * Do not use metadata for authoritative information. There is no server-side
3-4: Consider adding security measures for JsonElement parsing.Since metadata can now contain complex JSON structures, it's important to implement proper validation and sanitization.
Consider:
- Adding size limits for nested structures
- Implementing JSON schema validation
- Adding sanitization for potentially dangerous values
- Setting up proper error handling for malformed JSON
Would you like me to provide example implementations for these security measures?
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2)
31-33: Update parameter documentation referencesThe parameter documentation still references
SendReactionParams(e.g.,See [SendReactionParams.type]) even though the method signature no longer uses this class. These references should be updated or removed to reflect the new direct parameter approach.
129-130: Consider adding null check for type parameterWhile metadata is properly null-checked, the
typeparameter should also be validated to ensure it's not blank or empty, as it's a mandatory parameter according to the documentation.addProperty("type", type) +require(type.isNotBlank()) { "Reaction type cannot be blank" }chat-android/src/main/java/com/ably/chat/ChatApi.kt (2)
Line range hint
89-96: Add null check and improve type safety in metadata validationThe current implementation might throw a ClassCastException if metadata is not null but also not a JsonObject.
Consider this safer implementation:
-if ((params.metadata as? JsonObject)?.has(RESERVED_ABLY_CHAT_KEY) == true) { +if (params.metadata != null && params.metadata !is JsonObject) { + throw AblyException.fromErrorInfo( + ErrorInfo( + "Metadata must be a JsonObject", + HttpStatusCode.BadRequest, + ErrorCode.InvalidRequestBody.code, + ), + ) +} +if ((params.metadata as? JsonObject)?.has(RESERVED_ABLY_CHAT_KEY) == true) {
Line range hint
43-200: Consider documenting metadata format requirementsWith the switch to JsonElement for metadata, it would be valuable to:
- Document the expected metadata format and constraints in KDoc
- Consider providing helper methods for common metadata operations
- Add examples in the README for proper metadata usage
Would you like me to help draft the documentation and helper methods?
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1)
138-138: Document the breaking change in metadata nullability.The test now expects
nullmetadata instead of an empty map, which is a breaking change. This should be clearly documented in:
- Migration guide
- API documentation
- Release notes
Would you like me to help draft the documentation for this breaking change?
example/src/main/java/com/ably/chat/example/MainActivity.kt (3)
216-216: Consider utilizing metadata and headers parameters.The simplified message sending looks good, but consider demonstrating the usage of
metadataandheadersparameters in this example, as they are part of the public API improvements mentioned in the PR objectives.room.messages.send( text = messageText.text, + metadata = null, // Add example metadata if needed + headers = mapOf(), // Add example headers if needed )
224-224: Consider making the reaction type configurable.While the simplified reaction sending looks good, the reaction type is hardcoded. Consider making it configurable to better demonstrate the API's flexibility.
-room.reactions.send(type = "\uD83D\uDC4D") +room.reactions.send(type = reactionType) // Pass as parameter to onReactionClick
322-323: Consider adding a preview variant with metadata.The current preview demonstrates a message without metadata. Consider adding another preview variant that demonstrates a message with metadata to showcase this aspect of the API improvements.
@Preview @Composable fun MessageBubblePreview() { AblyChatExampleTheme { MessageBubble( message = Message( text = "Hello World!", timeserial = "fake", roomId = "roomId", clientId = "clientId", createdAt = System.currentTimeMillis(), metadata = null, headers = mapOf(), ), ) } } +@Preview(name = "With Metadata") +@Composable +fun MessageBubbleWithMetadataPreview() { + AblyChatExampleTheme { + MessageBubble( + message = Message( + text = "Hello World!", + timeserial = "fake", + roomId = "roomId", + clientId = "clientId", + createdAt = System.currentTimeMillis(), + metadata = JsonElement(...), // Add example metadata + headers = mapOf("custom" to "value"), + ), + ) + } +}README.md (1)
293-293: Add examples for sending messages with metadata and headersWhile the basic example is good, consider adding examples demonstrating how to send messages with metadata and headers, similar to the reaction examples. This would provide a complete picture of the API's capabilities.
Example to add:
val message = room.messages.send( text = "text", metadata = mapOf("priority" to "high"), headers = mapOf("source" to "user-input") )chat-android/src/main/java/com/ably/chat/Messages.kt (3)
41-44: Update documentation references to reflect parameter changesThe documentation for the
getmethod parameters still referencesQueryOptions.start,QueryOptions.end, etc., even though the method now accepts individual parameters instead of aQueryOptionsobject. Consider updating these references to point directly to the parameters or provide updated links.
64-66: Update documentation references insendmethod parametersSimilar to the
getmethod, the documentation for thesendmethod parameters still referencesSendMessageParams.text,SendMessageParams.metadata, etc., despite the method now accepting individual parameters. Update these references to match the new parameter structure to maintain clarity.
Line range hint
394-404: Handle potential null values when parsingmetadataIn the
parsePubSubMessageDatafunction,metadatais retrieved usingdata.get("metadata")without checking for nullability or ensuring correct type casting. This could lead to unexpected behavior ifmetadatais not present or not of the expected type. Consider adding null checks or using safe-call operators to handle potential null values safely.Here's a suggested change:
private data class PubSubMessageData(val text: String, val metadata: MessageMetadata?) private fun parsePubSubMessageData(data: Any): PubSubMessageData { if (data !is JsonObject) { throw AblyException.fromErrorInfo( ErrorInfo("Unrecognized Pub/Sub channel's message for `Message.created` event", HttpStatusCode.InternalServerError), ) } return PubSubMessageData( text = data.requireString("text"), - metadata = data.get("metadata"), + metadata = data.get("metadata")?.asJsonObject, ) }This modification ensures that
metadatais safely parsed as aJsonObjectif present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
README.md(3 hunks)chat-android/src/main/java/com/ably/chat/ChatApi.kt(4 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(7 hunks)chat-android/src/main/java/com/ably/chat/Metadata.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Reaction.kt(1 hunks)chat-android/src/main/java/com/ably/chat/RoomReactions.kt(3 hunks)chat-android/src/test/java/com/ably/chat/ChatApiTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(5 hunks)chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(2 hunks)
🧰 Additional context used
📓 Learnings (1)
chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
🔇 Additional comments (15)
chat-android/src/main/java/com/ably/chat/Metadata.kt (1)
18-18: LGTM! Verify the migration impact.
The change to JsonElement provides more flexibility for metadata structures. However, this is a breaking change that requires careful migration.
Let's verify the impact on existing metadata usage:
✅ Verification successful
Let me gather more information about metadata usage with different search patterns.
Based on the search results, I can now provide a final response:
Migration to JsonElement is properly integrated and safe to use
The change from Map<String, String> to JsonElement is well-integrated across the codebase:
- All metadata usages (MessageMetadata, ReactionMetadata) consistently use the new JsonElement type
- Proper validation is in place to check for reserved 'ably-chat' keys
- Test coverage exists for both valid and invalid metadata scenarios
- Utility methods in JsonUtils.kt safely handle JsonElement parsing and validation
- The change maintains backward compatibility as JsonElement can represent simple key-value structures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing metadata access patterns that might need updates
rg -l "Map<String,\s*String>" | xargs -I {} rg "Metadata" {}
# Find places where metadata is being processed
ast-grep --pattern 'metadata.$$$'
Length of output: 177
Script:
#!/bin/bash
# Search for Metadata type usage
rg "Metadata" -A 2 -B 2
# Search for JsonElement usage to understand the impact
rg "JsonElement" -A 2 -B 2
# Look for metadata-related test files
fd "test|spec" --exec rg "Metadata|metadata" {}
Length of output: 22223
chat-android/src/main/java/com/ably/chat/Reaction.kt (1)
25-25: Verify the impact of nullable metadata
The change from non-nullable ReactionMetadata with default value to nullable ReactionMetadata? is a breaking change. Let's verify its usage across the codebase.
chat-android/src/main/java/com/ably/chat/Message.kt (1)
54-54: Verify the impact of making metadata nullable
This is a breaking change in the public API that could affect message consumers. Let's verify all message creation and consumption points to ensure they handle null metadata correctly.
✅ Verification successful
Based on the shell script results, I can now provide a final response:
Making metadata nullable is safe and already handled correctly
The codebase already handles nullable metadata appropriately:
- All Message construction sites either pass null explicitly or handle nullable metadata
- No unsafe metadata access (
.metadata.) was found in the codebase - The metadata parameter is properly documented as "Optional metadata" in the API docs
- The change aligns with existing usage patterns where null metadata is already being used in tests and example code
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all places where Message is constructed or its metadata is accessed
# Find Message construction sites
echo "=== Message Construction Sites ==="
ast-grep --pattern 'Message($$$)'
# Find direct metadata access
echo -e "\n=== Direct Metadata Access ==="
rg -A 2 '\.metadata\b'
# Find potential null-unsafe access
echo -e "\n=== Potential Null-Unsafe Access ==="
rg -A 2 '\.metadata\.'
Length of output: 9621
chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt (1)
97-97: LGTM! The metadata assertion change aligns with the API improvements.
The change from mapOf() to null correctly reflects the metadata type modification described in the PR objectives. The test maintains comprehensive coverage of all reaction properties.
Let's verify the test coverage for other metadata scenarios:
✅ Verification successful
Metadata handling is properly tested across the codebase
The test coverage for metadata is comprehensive and aligns with the API changes:
- Tests cover both null metadata cases (ChatApiTest, RoomReactionsTest)
- Tests verify non-null JsonObject metadata (MessagesTest)
- Tests include validation for restricted metadata keys
- Tests verify metadata preservation across message transformations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of different metadata scenarios
# Expected: Tests covering both null and non-null metadata cases
# Search for other metadata-related tests
rg -l "metadata\s*=" --type kotlin --glob "**/*Test.kt"
# Look for reaction-related test cases
ast-grep --pattern 'fun `$_`() = runTest {
$$$
metadata = $_
$$$
}'
Length of output: 308
Script:
#!/bin/bash
# Let's examine the test coverage for metadata in the identified test files
# Check metadata usage in ChatApiTest
rg "metadata" -B 2 -A 2 chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
# Check metadata usage in MessagesTest
rg "metadata" -B 2 -A 2 chat-android/src/test/java/com/ably/chat/MessagesTest.kt
# Check all metadata test cases in RoomReactionsTest
rg "metadata" -B 2 -A 2 chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt
Length of output: 1690
chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (3)
101-101: LGTM! Consistent metadata handling
The change maintains consistency with the new metadata handling approach across the test suite.
Line range hint 13-142: Comprehensive test coverage maintained
The test suite maintains good coverage of both happy path and error scenarios, including:
- Handling of unknown fields
- Validation of required fields
- Error cases for missing data
- Consistent metadata handling across different operations
48-48: Verify the impact of nullable metadata
The change from mapOf() to null for metadata reflects the API changes, but this is a breaking change that might affect clients expecting non-null metadata.
Let's verify the impact:
✅ Verification successful
Nullable metadata is an intentional API design
The change from mapOf() to null for metadata is consistent with the codebase's design and implementation:
- All metadata parameters are explicitly nullable (
MessageMetadata?,ReactionMetadata?) in the API signatures - Documentation clearly states metadata is optional and can be null
- The codebase handles null metadata safely throughout:
- Optional parameters default to null in function signatures
- Null-safe calls when accessing metadata (e.g.,
metadata?.let) - Tests cover both null and non-null metadata scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct metadata access patterns that might break with null values
rg -t kotlin '\.metadata\.' --no-heading
# Search for potential null-safety issues in metadata handling
ast-grep --pattern 'metadata.$$$'
Length of output: 78
Script:
#!/bin/bash
# Search for metadata usage patterns in Kotlin files
rg -t kotlin 'metadata' -B 2 -A 2
# Search for metadata property declarations and usages
ast-grep --pattern 'val metadata'
# Search for metadata parameter declarations
ast-grep --pattern 'metadata:'
Length of output: 19769
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (2)
39-39: LGTM! Method signature simplification improves API usability
The flattened method signature with optional parameters makes the API more intuitive while maintaining backward compatibility.
156-156: Verify metadata type compatibility across the codebase
The change from Map<String, String> to JsonElement for metadata handling is significant. Ensure that all consumers of this metadata are updated to handle the new type correctly.
✅ Verification successful
Type compatibility for metadata is properly maintained across the codebase
The codebase consistently uses typed metadata across all relevant components:
ReactionMetadatafor reactions (inRoomReactions.ktandReaction.kt)MessageMetadatafor messages (inMessages.ktandMessage.kt)- All assignments and usages follow these type definitions
- Test files demonstrate correct JsonObject handling for metadata
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining Map<String, String> metadata references
rg "Map<String,\s*String>.*metadata"
# Search for metadata usage patterns
ast-grep --pattern 'metadata.$$$'
Length of output: 74
Script:
#!/bin/bash
# Search for metadata type declarations and usages in Kotlin files
rg "metadata.*:.*" --type kotlin -A 2
# Search for metadata parameter declarations in function signatures
ast-grep --pattern 'fun $_($$$metadata: $_, $$$)'
# Search for metadata property declarations
ast-grep --pattern 'val metadata: $_'
# Search for metadata assignments and usages
rg "metadata\s*=" --type kotlin -A 2
Length of output: 6361
chat-android/src/main/java/com/ably/chat/ChatApi.kt (3)
65-65: LGTM: Consistent metadata handling in send operation
The metadata handling in sendMessage now matches the getMessages implementation, maintaining consistency across the API.
Also applies to: 81-81
199-200: LGTM: Appropriate enum renaming
The rename from MessageOrder to ResultOrder is more generic and better reflects its usage across different result types.
Let's verify that all MessageOrder references have been updated:
✅ Verification successful
Enum rename from MessageOrder to ResultOrder has been properly implemented
The verification confirms that:
- The enum is now correctly defined as
ResultOrderin Messages.kt - All references use the new
ResultOrdername consistently - No remaining references to the old
MessageOrdername exist - The enum is properly used in parameter types and when mapping to direction values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to MessageOrder
rg "MessageOrder"
Length of output: 17
Script:
#!/bin/bash
# Let's search for both the old and new enum names to verify the changes
# First, let's find all Kotlin files that might contain these enums
fd -e kt | xargs rg -l "ResultOrder|MessageOrder"
# Then, let's look at the actual enum definition and its usages
ast-grep --pattern 'enum class $_ {
$$$
}'
Length of output: 1140
Script:
#!/bin/bash
# Let's look at all occurrences of ResultOrder to verify its usage
rg "ResultOrder" -B 2 -A 2
# Also check for any remaining MessageOrder references that might be in comments or strings
rg "MessageOrder" -B 2 -A 2
Length of output: 2708
43-44: LGTM: Metadata handling improvement
The change from Map to JsonElement aligns with the PR objectives and provides more flexible metadata handling. The headers conversion to Map is maintained for backward compatibility.
Let's verify that all metadata consumers handle JsonElement correctly:
✅ Verification successful
The previous ast-grep pattern wasn't specific enough. Let's search for metadata-related patterns using ripgrep to understand how metadata is used across the codebase.
The search results show that metadata is used across multiple files with different types (MessageMetadata, ReactionMetadata). Let's examine the type definitions and test implementations to ensure the JsonElement change is properly supported throughout the codebase.
Metadata type change is properly implemented and verified
The change from Map to JsonElement is correctly implemented and supported throughout the codebase:
Metadata.ktdefines the type aliasMetadata = JsonElement- Tests in
MessagesTest.ktdemonstrate proper usage with both JsonObject metadata and null cases - The implementation properly handles metadata validation (e.g., checking for reserved "ably-chat" key)
- All consumers are using the JsonElement type consistently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for metadata usage patterns to ensure compatibility
ast-grep --pattern 'metadata.$$$'
Length of output: 35
Script:
#!/bin/bash
# Search for metadata usage in the codebase
rg -l "metadata" --type kotlin
# Then search for specific metadata access patterns
rg "metadata\." --type kotlin -A 2
rg "metadata\[" --type kotlin -A 2
rg "metadata =|metadata:" --type kotlin -A 2
Length of output: 7226
Script:
#!/bin/bash
# Let's check the type definitions and interfaces for metadata
cat chat-android/src/main/java/com/ably/chat/Metadata.kt
# Check the test implementations to see how JsonElement metadata is handled
cat chat-android/src/test/java/com/ably/chat/MessagesTest.kt
Length of output: 10369
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1)
230-231: LGTM! Test maintains proper validation with the new API.
The test correctly validates the restricted headers using the flattened parameters while maintaining the same error handling.
README.md (1)
Line range hint 315-324: LGTM!
The message history retrieval example clearly demonstrates the new simplified API with proper pagination handling.
chat-android/src/main/java/com/ably/chat/Messages.kt (1)
112-117: Ensure consistent renaming from MessageOrder to ResultOrder
The MessageOrder enum has been renamed to ResultOrder, and usages have been updated accordingly. To prevent any inconsistencies or references to the old enum name, verify that all occurrences of MessageOrder have been replaced throughout the codebase and documentation.
Run the following script to check for any remaining references:
✅ Verification successful
Renaming from MessageOrder to ResultOrder has been properly implemented
The verification shows that:
- No references to the old
MessageOrderenum remain in the codebase - The new
ResultOrderenum is consistently used across the relevant files - All usages are properly updated, including imports and parameter types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find any remaining references to `MessageOrder` in the codebase
rg 'MessageOrder'
Length of output: 17
Script:
#!/bin/bash
# Let's search for both the old and new enum names to understand the context better
# and verify the changes across the codebase
# Search for ResultOrder
echo "Searching for ResultOrder references:"
rg "ResultOrder" -A 2
# Search for any potential test files or documentation that might reference MessageOrder
echo -e "\nSearching specifically in test and documentation files:"
fd -e kt -e md | xargs rg "Message.*Order"
Length of output: 2048
ttypic
force-pushed
the
chore-api-improvements
branch
from
6218ba7 to
a59760a
Compare
- Metadata changed from `Map<String, String>` to the `JsonElement` - Flatten `Messages.send()`, `Messages.get()` and `Reactions.send()` arguments
ttypic
force-pushed
the
chore-api-improvements
branch
from
a59760a to
a1ffba2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (9)
chat-android/src/main/java/com/ably/chat/Discontinuities.kt (2)
12-12: Documentation improvement looks good, consider adding more contextThe updated documentation accurately describes the channel property. However, since this is part of a public API, consider enhancing it further by explaining the significance of this association and its implications for discontinuity handling.
Consider expanding the documentation like this:
- * The channel that this object is associated with. + * The channel that this object is associated with. This channel is monitored + * for discontinuities, which may occur during network interruptions or + * reconnection events.
Line range hint
1-63: Well-structured error handling implementationThe overall implementation demonstrates robust error handling practices:
- Proper null safety for error information
- Protected event emission with logging
- Clear separation between handling and emission concerns
Consider documenting the error handling strategy in the project's technical documentation, as this pattern could be valuable for other parts of the SDK.
chat-android/src/main/java/com/ably/chat/Typing.kt (3)
Line range hint
186-208: Consider enhancing error handling in processEventThe retry mechanism could benefit from more specific error handling and logging:
- Different retry strategies for different types of errors
- More detailed error context in logs
Consider this improvement:
private suspend fun processEvent() { var numRetries = 0 while (numRetries <= PRESENCE_GET_MAX_RETRIES) { try { val currentlyTyping = get() emit(currentlyTyping) return // Exit if successful } catch (e: Exception) { + when (e) { + is AblyException -> { + logger.error("Ably-specific error occurred", e, mapOf( + "errorCode" to e.errorInfo.code, + "statusCode" to e.errorInfo.statusCode + )) + } + else -> { + logger.error("Unexpected error type", e) + } + } numRetries++ val delayDuration = min( PRESENCE_GET_RETRY_MAX_INTERVAL_MS, PRESENCE_GET_RETRY_INTERVAL_MS * 2.0.pow(numRetries).toLong(), ) logger.debug("Retrying in $delayDuration ms... (Attempt $numRetries of $PRESENCE_GET_MAX_RETRIES)", e) delay(delayDuration) } } - logger.error("Failed to get members after $PRESENCE_GET_MAX_RETRIES retries") + logger.error("Failed to get members after $PRESENCE_GET_MAX_RETRIES retries", + staticContext = mapOf("maxRetries" to PRESENCE_GET_MAX_RETRIES)) }
Line range hint
134-146: Consider improving scope managementThe typingScope could benefit from more robust cancellation handling:
- Ensure all child jobs are properly cancelled
- Add timeout for long-running operations
Consider this improvement:
override fun release() { + typingJob?.cancel() presenceSubscription.unsubscribe() typingScope.cancel() + typingScope.coroutineContext[Job]?.let { job -> + runCatching { + job.children.forEach { it.cancel() } + }.onFailure { e -> + logger.error("Error while cancelling child jobs", e) + } + } room.realtimeClient.channels.release(channel.name) }
Line range hint
211-217: Consider adding state validation in emitThe emit function could benefit from additional validation to ensure state consistency:
Consider this improvement:
private fun emit(currentlyTyping: Set<String>) { if (lastTyping == currentlyTyping) return + if (currentlyTyping.any { it.isBlank() }) { + logger.warn("Received invalid clientId in typing set") + return + } lastTyping = currentlyTyping listeners.forEach { it.onEvent(TypingEvent(currentlyTyping)) } }chat-android/src/main/java/com/ably/chat/Presence.kt (1)
45-46: Update parameter documentation for technical accuracyThe documentation for the
dataparameter describes it as a "JSON serializable object", but it's actually of typePresenceData(alias forJsonElement). This should be corrected for technical accuracy.Consider updating the documentation to:
- * @param data The users data, a JSON serializable object that will be sent to all subscribers. + * @param data The users data as a JsonElement that will be sent to all subscribers.Also applies to: 52-53, 59-60
chat-android/src/main/java/com/ably/chat/RoomLifecycleManager.kt (1)
287-290: Incomplete sentence in method documentation.The documentation for the
attachmethod contains an incomplete sentence at line 288. The sentence ends abruptly with "as is the case", which seems incomplete. Please complete the sentence for clarity.chat-android/src/main/java/com/ably/chat/Messages.kt (2)
303-307: Refactor Suggestion: SimplifygetMethod ImplementationIn the
getmethod, you're reconstructing aQueryOptionsobject using the individual parameters before passing it tochatApi.getMessages. Since the individual parameters are already available, consider updatingchatApi.getMessagesto accept these parameters directly. This refactoring would eliminate the need to create aQueryOptionsobject, reduce unnecessary object creation, and streamline the code.
309-312: Refactor Suggestion: SimplifysendMethod ImplementationSimilarly, in the
sendmethod, you create aSendMessageParamsobject with the provided parameters before passing it tochatApi.sendMessage. Consider updatingchatApi.sendMessageto accept the parameters directly. This change would remove the dependency onSendMessageParams, reduce object creation overhead, and improve code readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
README.md(3 hunks)chat-android/src/main/java/com/ably/chat/ChatApi.kt(5 hunks)chat-android/src/main/java/com/ably/chat/Discontinuities.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(8 hunks)chat-android/src/main/java/com/ably/chat/Metadata.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Occupancy.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Presence.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Reaction.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Room.kt(2 hunks)chat-android/src/main/java/com/ably/chat/RoomLifecycleManager.kt(2 hunks)chat-android/src/main/java/com/ably/chat/RoomOptions.kt(2 hunks)chat-android/src/main/java/com/ably/chat/RoomReactions.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Rooms.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Typing.kt(2 hunks)chat-android/src/test/java/com/ably/chat/ChatApiTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(5 hunks)chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- chat-android/src/main/java/com/ably/chat/Room.kt
- chat-android/src/main/java/com/ably/chat/Occupancy.kt
- chat-android/src/main/java/com/ably/chat/RoomOptions.kt
🚧 Files skipped from review as they are similar to previous changes (8)
- chat-android/src/main/java/com/ably/chat/Metadata.kt
- chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt
- chat-android/src/main/java/com/ably/chat/Reaction.kt
- chat-android/src/main/java/com/ably/chat/Message.kt
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
- example/src/main/java/com/ably/chat/example/MainActivity.kt
- README.md
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
🔇 Additional comments (24)
chat-android/src/main/java/com/ably/chat/Typing.kt (1)
41-41: Documentation improvements look good!
The documentation changes follow Kotlin conventions and improve clarity:
- Updated to use Kotlin-style link syntax for obtaining instances
- Simplified the return documentation to be more concise
Also applies to: 59-59
chat-android/src/main/java/com/ably/chat/Presence.kt (3)
17-17: Documentation improvements look good!
The enhanced parameter descriptions provide better clarity and are more informative for API users.
Also applies to: 28-32
Line range hint 89-91: Implementation aligns well with PR objectives
The change from Map<String, String> to JsonElement for the data property while maintaining Map<String, String> for extras provides a good balance between flexibility and backward compatibility.
Line range hint 171-177: Verify type consistency in data handling
The implementation extracts data using (it.data as? JsonObject)?.get("userCustomData"). We should verify that all places in the codebase that set presence data maintain this structure.
Also applies to: 191-197
✅ Verification successful
Let me gather more information about the data handling implementation.
Type consistency in presence data handling is maintained across the codebase
The verification shows consistent type handling across the codebase:
- All presence data extraction uses safe casting with
as? JsonObject - The
userCustomDatafield is consistently accessed using the same pattern - The example UI and core components maintain the same data structure expectations
- Error handling is in place for malformed data in various components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all places where presence data is set or handled
rg -l "presence.*data" | while read -r file; do
echo "=== $file ==="
rg -A 2 "presence.*data" "$file"
done
# Search specifically for JsonObject usage with userCustomData
ast-grep --pattern 'JsonObject().apply { $$ }'
Length of output: 1488
Script:
#!/bin/bash
# Search for JsonObject and data handling implementations
ast-grep --pattern 'fun wrapInUserCustomData($_) {
$$$
}'
# Search for presence data extraction patterns
rg -A 3 "data as\? JsonObject"
# Look for presence message handling
rg -A 3 "PresenceMessage.*data"
Length of output: 3092
chat-android/src/main/java/com/ably/chat/RoomReactions.kt (4)
14-14: Documentation improvements look good!
The updated documentation follows Kotlin standards, provides clear parameter descriptions, and properly documents the new flattened parameter structure. The changes improve clarity and maintainability.
Also applies to: 21-21, 26-37
125-132: Implementation of send method looks good!
The implementation properly handles the new parameter structure and correctly constructs the JSON payload.
39-39: Method signature change looks good, verify consistency
The flattened method signature improves API usability while maintaining backward compatibility through default parameters. This aligns well with the PR objectives.
Let's verify that this signature change is consistently applied across the codebase:
✅ Verification successful
Method signature change is correctly implemented
The flattened method signature is consistently implemented across the codebase. The SendReactionParams class is only referenced in the documentation and is no longer used in the implementation. The new flattened signature with optional parameters is properly integrated in the DefaultRoomReactions class and is being used correctly in the example app.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of SendReactionParams to ensure complete migration
rg "SendReactionParams" --type kotlin
# Search for other send methods to verify consistent parameter patterns
ast-grep --pattern 'fun send($$$)'
Length of output: 668
Script:
#!/bin/bash
# Let's check the full context of SendReactionParams class and its usages
rg -A 5 -B 5 "SendReactionParams" --type kotlin
# Let's also check if there are any other reaction-related methods that might need similar treatment
rg -A 3 "reaction" --type kotlin -i
Length of output: 42514
156-156: Verify backward compatibility of metadata type change
The change from Map<String, String> to JsonElement for metadata might impact existing clients. Please ensure that:
- All consumers of this API are updated to handle the new metadata type
- Migration guide is provided if needed
- Consider adding a compatibility layer if this is a breaking change
Let's check for existing metadata usage:
chat-android/src/main/java/com/ably/chat/RoomLifecycleManager.kt (2)
27-27: Documentation update for 'channel' property looks good.
The updated documentation provides a clearer description of the channel property.
Line range hint 287-320: Ensure exceptions thrown by attach are properly handled.
Since attach now throws exceptions instead of rejecting promises, please verify that all calls to attach are updated to handle these exceptions appropriately to avoid unhandled exceptions at runtime.
chat-android/src/main/java/com/ably/chat/Messages.kt (7)
5-5: Approval: Correct Import Statement
The import statement correctly brings in NewestFirst from QueryOptions.ResultOrder, which is necessary for the default parameter in the get method.
22-22: Approval: Documentation Reference Updated
The documentation now accurately references [Room.messages], enhancing clarity for users accessing the Messages instance.
42-47: Approval: Enhanced Parameter Documentation for get Method
The added parameter descriptions for start, end, limit, and orderBy in the get method are comprehensive and improve the usability of the API. Each parameter now clearly references the corresponding property in QueryOptions.
49-54: Potential Breaking Change: Modified get Method Signature
The get method now accepts individual parameters instead of a QueryOptions object. This change may break existing code that relies on the previous method signature. Ensure that this breaking change is intentional, update the documentation accordingly, and consider providing guidance for users to update their code.
65-69: Approval: Updated Parameter Documentation for send Method
The parameter descriptions for text, metadata, and headers in the send method are detailed and enhance the clarity of the API documentation. They provide users with clear guidance on how to utilize each parameter.
71-71: Potential Breaking Change: Modified send Method Signature
The send method signature has changed to accept individual parameters (text, metadata, headers) instead of a SendMessageParams object. This alteration may break existing implementations that use the previous signature. Verify that this change is intended, update the documentation, and consider providing migration instructions for users.
Line range hint 388-398: Verify Null Handling for metadata in parsePubSubMessageData
In the parsePubSubMessageData function, metadata is retrieved using data.get("metadata"), which may return null. Ensure that any downstream code correctly handles cases where metadata is null to prevent potential NullPointerException.
✅ Verification successful
Metadata Null Handling is Properly Implemented
Based on the search results, the metadata field is handled safely throughout the codebase:
- In
ChatApi.kt, metadata access uses the safe call operator (?.) when checking for reserved keys:(params.metadata as? JsonObject)?.has(RESERVED_ABLY_CHAT_KEY) - When constructing messages, metadata is passed through without direct access to its properties
- The
metadatafield is properly documented as optional in the API documentation - All usages of the metadata field from
PubSubMessageDataare treating it as a nullable value without any unsafe direct access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usages of `metadata` without proper null checks.
# Search for all usages of `PubSubMessageData.metadata` where it might be accessed without null checks
rg -t kotlin 'PubSubMessageData\(\w+, metadata\s*=\s*\w+\)' -A 5
# Look for instances where `metadata` is accessed directly
rg -t kotlin 'metadata\.\w+' --files-with-matches
# Expectation: All accesses of `metadata` should include null checks or safe calls
Length of output: 118
Script:
#!/bin/bash
# Let's try a more comprehensive search strategy
# 1. Find all files containing PubSubMessageData
rg -l "PubSubMessageData"
# 2. Search for any usage of 'metadata' field from PubSubMessageData
rg -t kotlin "\.metadata" -B 3 -A 3
# 3. Look for the complete implementation of parsePubSubMessageData
ast-grep --pattern 'fun parsePubSubMessageData($$$) {
$$$
}'
Length of output: 4479
chat-android/src/main/java/com/ably/chat/Rooms.kt (3)
27-31: Approval: Clarified Documentation for get Method Behavior
The updated documentation for the get method provides a clearer explanation of its behavior, especially regarding concurrent calls to get and release. It helps users understand how the method resolves in different scenarios.
35-35: Approval: Updated Exception Annotation
The documentation now specifies that the method throws [io.ably.lib.types.ErrorInfo], improving the accuracy of the documentation and helping developers anticipate potential exceptions.
47-47: Approval: Documentation Reference for release Method
The updated documentation correctly references [Rooms.get], providing a clear instruction for users on how to reacquire a room after release.
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (4)
70-72: Update Test Case to Reflect New send Method Signature
The test case has been updated to use the new send method signature with individual parameters (text, headers, metadata). This change ensures that the test aligns with the updated API.
142-142: Ensure Consistent Handling of metadata in Incoming Messages
In the test for subscribing to incoming messages, metadata is set to null. Confirm that this reflects the intended behavior and that the system handles messages without metadata correctly.
251-252: Critical Issue: Reserved Key Used in metadata
The metadata contains the key "ably-chat", which is reserved and should not be used. This will result in an exception being thrown.
Apply the following diff to correct the issue:
messages.send(
text = "lala",
- metadata = mapOf("ably-chat" to "data").toJson(),
+ metadata = JsonObject().apply { addProperty("meta", "data") },
)82-82: Verify metadata Handling in Test Assertions
In the test assertion, metadata is compared using a JsonObject with meta set to "data". Ensure that the comparison accounts for null cases and that the metadata field is correctly handled when null or non-null.
✅ Verification successful
Metadata Handling is Properly Tested
The test cases demonstrate comprehensive coverage of metadata handling:
- Tests verify sending messages with non-null metadata (
JsonObjectwith "meta": "data") - Tests verify receiving messages with null metadata
- Tests validate that metadata cannot contain reserved "ably-chat" key (throws exception)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that test cases correctly handle `metadata` being null or non-null.
# Search for test cases involving `metadata` in MessagesTest.kt
rg -A 5 -B 5 'metadata' chat-android/src/test/java/com/ably/chat/MessagesTest.kt
# Expectation: Tests should cover scenarios where `metadata` is null and where it contains data
Length of output: 1642
Map<String, String>to theJsonElementMessages.send(),Messages.get()andReactions.send()argumentsSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests