[ECO-5139] feat: switched API to v2 #67
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (11)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
🔇 Additional comments (6)chat-android/src/main/java/com/ably/chat/ChatApi.kt (6)
The addition of MessageAction import aligns with the v2 API changes for handling message actions.
This transformation logic can exclude messages with null or unrecognized latestAction values.
The changes correctly implement:
The occupancy endpoint has been consistently updated to use v2.
The nullable transform function allows silent filtering of messages which might mask issues.
The messages endpoint has been updated to v2, which aligns with the PR objective. ✅ Verification successfulAPI version consistency verified All chat API endpoints in the codebase are consistently using v2. This includes:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify all endpoints are consistently using v2
# Search for any remaining v1 endpoints in the codebase
rg -A 2 '"/chat/v1/'
Length of output: 22 Script: #!/bin/bash
# Let's search for all chat API endpoints to verify version consistency
rg -A 2 '"/chat/v[0-9]/'
# Also search for any URL construction patterns
rg -A 2 'url\s*=.*chat'
Length of output: 2252 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: 7
🧹 Outside diff range and nitpick comments (10)
chat-android/src/main/java/com/ably/chat/PaginatedResult.kt (1)
34-34: Consider documenting null handling behaviorSince the transformation function can now return null values that will be filtered out, it would be helpful to document this behavior in KDoc comments. This will help other developers understand when and why they might want to return null from their transform functions.
Add documentation like this:
+/** + * Transforms an AsyncHttpPaginatedResponse into a PaginatedResult. + * @param transform A function that transforms JsonElement to T. If the function returns null, + * the item will be filtered out from the final result. + */ fun <T> AsyncHttpPaginatedResponse?.toPaginatedResult(transform: (JsonElement) -> T?): PaginatedResult<T>chat-android/src/main/java/com/ably/chat/EventTypes.kt (2)
19-24: Consider enhancing PubSubMessageNames documentationWhile the documentation is clear, it would be helpful to add more context about:
- The purpose of using a separate naming pattern ("chat.message" vs "message.*")
- Whether this object is intended to be extended with more message types in the future
3-48: Consider documenting the relationship between MessageEventType and messageActionNameToActionThe file now contains two different ways to represent message states:
- MessageEventType enum for event handling ("message.created", "message.updated", etc.)
- messageActionNameToAction mapping for action handling ("message.create", "message.update", etc.)
Consider adding documentation to clarify:
- The distinction between events and actions
- When to use each type
- How they relate to the v2 API architecture
chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (3)
30-30: Consider adding test coverage for different message actionsWhile the migration to API v2's
serialfield is correctly implemented, the test only coversMessageAction.MESSAGE_CREATE. Consider adding test cases for other possible message actions to ensure comprehensive coverage of the new API's functionality.Example test case structure:
@Test fun `getMessages should handle different message actions`() = runTest { // Test with different MessageAction values // Test action updates // Test action transitions }Also applies to: 44-51
Line range hint
1-144: Consider organizing tests by API v2 feature setsAs the API transitions to v2, consider organizing the test cases into feature-based test classes (e.g.,
MessageActionTest,MessageSerializationTest) to improve maintainability and make it easier to ensure complete coverage of new API features.
Add test coverage for additional message actions
The codebase supports multiple message actions including
MESSAGE_UNSET,MESSAGE_UPDATE,MESSAGE_DELETE,ANNOTATION_CREATE,ANNOTATION_DELETE, andMETA_OCCUPANCY, but the test cases only coverMESSAGE_CREATE. Consider adding test cases for:
getMessagewith different message actions to verify proper handlingsendMessagewith various action types to ensure correct action propagationExample locations needing coverage:
chat-android/src/test/java/com/ably/chat/ChatApiTest.ktchat-android/src/test/java/com/ably/chat/MessagesTest.kt🔗 Analysis chain
Line range hint
88-104: Maintain test symmetry with getMessage implementationThe sendMessage test cases should maintain symmetry with the getMessage implementation. Consider adding the same comprehensive message action coverage suggested for the getMessage tests here as well.
Let's verify if there are any message action types we should be testing:
Let me gather more information about the test coverage of message actions:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MessageAction usage in the codebase to identify all possible actions ast-grep --pattern 'MessageAction.$_'Length of output: 1836
Script:
#!/bin/bash # Search for test cases involving MessageAction rg -B 2 -A 2 "MessageAction\." "chat-android/src/test/"Length of output: 1937
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1)
Based on the gathered information, I can now provide the final response:
Test coverage for API v2 changes is incomplete
The current test suite only covers
MessageAction.MESSAGE_CREATE, while the codebase defines multiple message actions:
- MESSAGE_UNSET
- MESSAGE_CREATE
- MESSAGE_UPDATE
- MESSAGE_DELETE
- ANNOTATION_CREATE
- ANNOTATION_DELETE
- META_OCCUPANCY
Missing test coverage for:
- Message update scenarios (
MESSAGE_UPDATE)- Message deletion scenarios (
MESSAGE_DELETE)- Annotation handling (
ANNOTATION_CREATE,ANNOTATION_DELETE)- Meta occupancy messages (
META_OCCUPANCY)🔗 Analysis chain
Line range hint
1-296: Verify complete test coverage of API v2 changesWhile the existing tests cover the basic API v2 changes, let's verify we haven't missed any edge cases.
Let me gather more information about the MessageAction usage and test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to timeserial and untested MessageAction values echo "Checking for any remaining timeserial references:" rg "timeserial" --type kotlin echo "Checking for all possible MessageAction values:" ast-grep --pattern 'enum class MessageAction { $$$ }'Length of output: 715
Script:
#!/bin/bash # Check for MessageAction usage in tests and implementation echo "Checking MessageAction usage in tests:" rg "MessageAction\." --type kotlin -A 2 echo "Checking Message class implementation:" ast-grep --pattern 'data class Message( $$$ )' echo "Checking DefaultMessages implementation for action handling:" ast-grep --pattern 'class DefaultMessages( $$$ )'Length of output: 10006
example/src/main/java/com/ably/chat/example/MainActivity.kt (1)
Line range hint
89-95: Replace runBlocking with LaunchedEffect for room initializationUsing
runBlockingin a Composable function can block the UI thread and affect app responsiveness. While previous feedback suggested it's acceptable for instant operations, room initialization might take longer.Consider refactoring to use
LaunchedEffect:-val room = runBlocking { - chatClient.rooms.get( - Settings.ROOM_ID, - RoomOptions(typing = TypingOptions(), presence = PresenceOptions(), reactions = RoomReactionsOptions), - ) -} +var room by remember { mutableStateOf<Room?>(null) } +LaunchedEffect(Unit) { + room = chatClient.rooms.get( + Settings.ROOM_ID, + RoomOptions(typing = TypingOptions(), presence = PresenceOptions(), reactions = RoomReactionsOptions), + ) +}Then update the rest of the composable to handle the nullable room state:
room?.let { nonNullRoom -> // Existing room usage code } ?: run { // Show loading state CircularProgressIndicator() }chat-android/src/main/java/com/ably/chat/Messages.kt (1)
Line range hint
1-377: Consider providing migration guidance for API v2The changes introduce several breaking changes:
- Message filtering based on
MessageAction- Property rename from
timeserialtoserial- New
latestActionproperty- Changed event subscription mechanism
Consider:
- Adding migration documentation
- Providing deprecation notices if supporting both v1 and v2 temporarily
- Updating integration tests to cover v2 specific scenarios
detekt.yml (1)
Line range hint
1-1000: Critical: Multiple important code quality checks are being disabledI notice a pattern of disabling several critical code quality checks that could impact code reliability and maintainability. Some notable concerns:
Security and Bugs:
UnsafeCallOnNullableType: Prevents null pointer exceptionsCastNullableToNonNullableType: Prevents runtime casting errorsAvoidReferentialEquality: Prevents incorrect equality comparisonsCode Quality:
ComplexInterface: Maintains interface simplicityRedundantSuspendModifier: Ensures proper coroutine usageSuspendFunSwallowedCancellation: Prevents coroutine cancellation issuesPerformance:
ArrayPrimitive: Optimizes array usageCouldBeSequence: Improves collection processing efficiencyConsider:
- Instead of disabling these checks globally, use suppression annotations (
@Suppress) for specific cases where the check doesn't apply- Document the rationale for disabling each check in the configuration
- Create a tracking issue to re-enable these checks in the future
Would you like me to help create a GitHub issue to track the re-enabling of these checks?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
chat-android/src/main/java/com/ably/chat/ChatApi.kt(4 hunks)chat-android/src/main/java/com/ably/chat/EventTypes.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(3 hunks)chat-android/src/main/java/com/ably/chat/PaginatedResult.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Timeserial.kt(0 hunks)chat-android/src/test/java/com/ably/chat/ChatApiTest.kt(6 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(7 hunks)detekt.yml(1 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(2 hunks)gradle/libs.versions.toml(1 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/Timeserial.kt
🧰 Additional context used
📓 Learnings (1)
example/src/main/java/com/ably/chat/example/MainActivity.kt (1)
Learnt from: sacOO7
PR: ably-labs/ably-chat-kotlin#61
File: example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt:36-36
Timestamp: 2024-11-25T10:28:29.320Z
Learning: In `example/src/main/java/com/ably/chat/example/ui/PresencePopup.kt`, using `runBlocking` within a Composable function is acceptable when the called coroutine returns instantly and does not block the UI thread.
🔇 Additional comments (23)
chat-android/src/main/java/com/ably/chat/PaginatedResult.kt (2)
48-50: LGTM! Proper null handling with mapNotNull
The change to use mapNotNull is the correct approach to handle nullable transformations while ensuring the items list never contains null values. This implementation maintains type safety and the PaginatedResult contract.
34-34: Verify the impact of nullable transformation function
The change to make transform return nullable T is a breaking change that could affect existing callers. This aligns with the API v2 migration but requires careful verification.
Let's verify the usage of this function across the codebase:
chat-android/src/main/java/com/ably/chat/Message.kt (3)
3-4: LGTM!
The import for MessageAction from the Ably library is correctly added.
22-22: Verify message ordering capabilities after removing comparison functions
The removal of isBefore, isAfter, and isAtTheSameTime functions might impact message ordering. Please ensure that message ordering is still properly handled with the new serial property.
Let's check for any custom ordering implementations:
22-22: Update documentation and verify usage after property rename
The property rename from timeserial to serial is a breaking change. The removal of "time" prefix suggests a change in semantic meaning.
Let's verify the impact:
Consider updating the documentation to clarify if the semantic meaning of this identifier has changed with the v2 API upgrade.
✅ Verification successful
Let me gather more context about the documentation and usage patterns.
Property rename from timeserial to serial is consistent with internal implementation
The property rename is correctly implemented throughout the codebase. The evidence shows:
- The
serialproperty is consistently used in the Messages implementation for message identification - Test files have been updated to use "serial" in property names
- The property maintains its role as a unique identifier for messages, as documented in the Message class
- No remaining references to "timeserial" in production code, only in test data values
The semantic meaning appears unchanged as it's still used as a unique identifier for messages, and the implementation in Messages.kt shows it's properly integrated with Ably's channel serial system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to 'timeserial'
rg -i 'timeserial' --type kotlin
# Search for new 'serial' usage to ensure consistent adoption
rg -i '\.serial\b' --type kotlin
Length of output: 581
Script:
#!/bin/bash
# Check documentation and implementation of Message class
rg -A 5 -B 5 'serial:' chat-android/src/main/java/com/ably/chat/Message.kt
# Check the Messages.kt implementation to understand usage context
rg -A 5 -B 5 'serial =' chat-android/src/main/java/com/ably/chat/Messages.kt
# Look for any API documentation or migration guides
fd README.md CHANGELOG.md MIGRATION.md | xargs rg -i 'serial|timeserial|breaking change'
Length of output: 9181
chat-android/src/main/java/com/ably/chat/EventTypes.kt (1)
11-17: LGTM: MessageEventType enum update
The addition of the Updated event type follows a consistent pattern and is well-documented.
chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (2)
5-5: LGTM: Required import for API v2 changes
The addition of MessageAction import is necessary for the new message action handling in API v2.
114-114: LGTM: Consistent test naming
The test name has been appropriately updated to reflect the API v2 terminology.
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (5)
13-13: LGTM: Import addition aligns with API v2 changes
The addition of MessageAction import supports the new message action functionality.
63-63: LGTM: Message construction test updated for API v2
The test correctly validates the new message structure with:
- Renamed serial property (previously timeserial)
- New latestAction property with MESSAGE_CREATE value
Also applies to: 79-86
120-120: LGTM: Subscription test properly updated for API v2
The test assertions correctly validate the updated message structure in the subscription flow.
Also applies to: 140-144
Line range hint 267-275: LGTM: Serial validation test maintains proper error handling
The test correctly validates the subscription point serial constraints with the updated serial format.
294-294: LGTM: Helper function updated consistently
The buildDummyPubSubMessage helper correctly uses the new serial property format.
example/src/main/java/com/ably/chat/example/MainActivity.kt (1)
58-58: LGTM: Import added for MessageAction
The addition of MessageAction import aligns with the API v2 migration.
chat-android/src/main/java/com/ably/chat/Messages.kt (4)
12-12: LGTM: Import required for message action handling
The addition of MessageAction import is necessary for the new message filtering functionality.
281-281: LGTM: Consistent unsubscribe implementation
The unsubscribe call has been correctly updated to use PubSubMessageNames.ChatMessage, maintaining consistency with the subscription change.
263-267: Verify consistent usage of serial and latestAction properties
The migration from timeserial to serial and addition of latestAction needs to be consistently applied across the codebase to prevent runtime errors.
✅ Verification successful
Property usage is consistent across the codebase
All Message constructions in the codebase consistently use serial and latestAction properties. The only occurrences of "timeserial" are in test files (ChatApiTest.kt) where it's used as a test value, which is acceptable and doesn't affect runtime behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of timeserial
rg "timeserial" --type kotlin
# Search for Message construction to verify latestAction usage
ast-grep --pattern 'Message($$$)' --lang kotlin
Length of output: 8539
254-257: Verify handling of other message actions
The code now filters out all message actions except MESSAGE_CREATE. While this aligns with the v2 API changes, we should verify that other message actions (e.g., updates, deletes) don't need to be handled.
✅ Verification successful
Let me search for any message event handling to understand how different message actions are processed.
Based on the evidence gathered, I can now generate the final response:
Filtering of message actions is intentional and correct
The codebase shows that while there are multiple message action types defined (MESSAGE_CREATE, MESSAGE_UPDATE, MESSAGE_DELETE, etc.), the SDK is specifically designed to only handle MESSAGE_CREATE actions and emit MessageEventType.Created events. This aligns with the v2 API design where the Android Chat SDK focuses on message creation functionality. Other message actions like updates and deletes are not part of the current SDK's scope.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other message action handling in the codebase
rg -A 5 "MessageAction\." --type kotlin
Length of output: 6247
Script:
#!/bin/bash
# Search for MessageEventType and its handling
rg -A 5 "MessageEventType\." --type kotlin
Length of output: 1241
detekt.yml (1)
Line range hint 1-1000: Verify impact on API v2 migration
Given that this PR is about switching to API v2, it's concerning to see multiple code quality checks being disabled. This could mask potential issues during the API migration.
chat-android/src/main/java/com/ably/chat/ChatApi.kt (4)
34-34: API Endpoint Updated to v2
The endpoint URL for fetching messages has been correctly updated to /chat/v2/rooms/$roomId/messages to use the new API version.
77-77: API Endpoint Updated to v2 in sendMessage
The endpoint URL for sending messages has been correctly updated to /chat/v2/rooms/$roomId/messages to align with the updated API version.
83-90: Including serial and latestAction in Sent Message
The sendMessage method now constructs the Message object with the serial obtained from the API response and sets latestAction to MessageAction.MESSAGE_CREATE, which is appropriate for newly created messages.
169-169: Adjusting Transform Function to Return Nullable Type
The transform function in makeAuthorizedPaginatedRequest now returns a nullable type (JsonElement) -> T?, allowing for the possibility of filtering out elements during pagination.
Ensure this change aligns with the intended behavior and that any elements filtered out are meant to be excluded from the paginated results.
| @@ -3,7 +3,7 @@ | |||
|
|
|||
| [versions] | |||
| ably-chat = "0.0.1" | |||
| ably = "1.2.45" | |||
| ably = "1.2.46" | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide a final response:
Version 1.2.46 is not compatible with API v2 migration
The version bump from 1.2.45 to 1.2.46 of the Ably SDK is insufficient for API v2 migration. The current version range (1.2.x) is part of the v1 API series. For API v2 support, you need to upgrade to version 2.x of the SDK.
- The latest version from Maven Central is still 1.2.45
- The codebase shows extensive usage of v1 API patterns (AblyRealtime, ClientOptions, etc.)
- No v2 migration guides or documentation were found in the repository
🔗 Analysis chain
Verify Ably SDK version compatibility with API v2
The version bump from 1.2.45 to 1.2.46 seems minor for a significant API version upgrade. Let's verify:
- If this version fully supports API v2
- If there's a more recent version available
- If there are any security concerns
Consider adding a comment in the version catalog to document why this specific version was chosen, especially since it's tied to an API version upgrade:
[versions]
-ably = "1.2.46"
+# Required for API v2 support
+ably = "1.2.46"Let me try another verification approach to check the repository for API v2 related changes and version compatibility.
Let me try one final verification to check the build files and any migration guides or documentation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Ably SDK version compatibility and check for newer versions
# Check latest version from Maven Central
echo "Latest version from Maven Central:"
curl -s "https://search.maven.org/solrsearch/select?q=g:io.ably+AND+a:ably-android&rows=1&wt=json" | jq -r '.response.docs[0].latestVersion'
# Check GitHub releases for changelog and breaking changes
echo -e "\nRecent releases and changes:"
gh api repos/ably/ably-android/releases --jq '.[0:3] | .[] | {tag_name, published_at, body}'
# Check for security advisories
echo -e "\nSecurity advisories:"
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "io.ably:ably-android") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 955
Script:
#!/bin/bash
# Search for API v2 related changes and version requirements
echo "Searching for API v2 related information in documentation and code:"
rg -i "api.*v2|version.*2" --type md
echo -e "\nChecking build files for Ably SDK usage:"
rg -l "io\.ably" --type gradle
echo -e "\nSearching for Ably client initialization or version specific code:"
rg -B2 -A2 "ClientOptions|AblyRealtime|AblyRest" --type kotlin --type java
Length of output: 25119
Script:
#!/bin/bash
# Search for build files and documentation
echo "Searching for build files:"
fd -e gradle -e toml
echo -e "\nChecking README and documentation files:"
fd -e md
echo -e "\nChecking build file contents:"
cat gradle/libs.versions.toml
echo -e "\nSearching for migration or upgrade related information:"
rg -i "migration|upgrade|v2|version.2" --type md --type gradle
Length of output: 4095
| serial = "fake", | ||
| roomId = "roomId", | ||
| clientId = "clientId", | ||
| createdAt = System.currentTimeMillis(), | ||
| metadata = mapOf(), | ||
| headers = mapOf(), | ||
| latestAction = MessageAction.MESSAGE_CREATE, |
There was a problem hiding this comment.
💡 Codebase verification
Update remaining timeserial occurrences in test files
The verification revealed that while most Message constructor calls have been properly updated, there are still instances of timeserial being used in test files that need to be updated to match the new API v2 format:
chat-android/src/test/java/com/ably/chat/ChatApiTest.kt:- Line 44:
serial = "timeserial" - Line 97:
serial = "timeserial"
- Line 44:
All Message constructor calls consistently include the required latestAction parameter with MessageAction.MESSAGE_CREATE, but the test data should use a properly formatted serial value like the one used in MessagesTest.kt ("abcdefghij@1672531200000-123").
🔗 Analysis chain
Verify Message constructor updates across the codebase
The Message constructor parameters have been correctly updated for API v2:
timeserial→serial- Added
latestActionwithMessageAction.MESSAGE_CREATE
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Message constructor usage consistency across the codebase
# Look for any remaining instances of timeserial that might have been missed
# Search for timeserial usage
echo "Checking for any remaining timeserial usage:"
rg "timeserial"
# Search for Message constructor calls to verify latestAction parameter
echo "Checking Message constructor calls:"
ast-grep --pattern 'Message($$$)'
Length of output: 8691
| ) | ||
| listener.onEvent(MessageEvent(type = MessageEventType.Created, message = chatMessage)) | ||
| } | ||
| // (CHA-M4d) | ||
| channel.subscribe(MessageEventType.Created.eventName, messageListener) | ||
| channel.subscribe(PubSubMessageNames.ChatMessage, messageListener) |
There was a problem hiding this comment.
💡 Codebase verification
Breaking change detected in event subscription
The verification reveals a significant breaking change in the event subscription mechanism:
- Old event name:
message.created(fromMessageEventType.Created.eventName) - New event name:
chat.message(fromPubSubMessageNames.ChatMessage)
This change affects:
- Event subscription in
Messages.kt - Existing tests in
MessagesTest.ktthat rely onmessage.created - The event handling system that uses
MessageEventType.Createdfor event type classification
The codebase still contains references to the old event name pattern, indicating incomplete migration or potential compatibility issues.
🔗 Analysis chain
Verify the event name constant and its usage
The subscription now uses PubSubMessageNames.ChatMessage instead of MessageEventType.Created.eventName. This is a breaking change that requires verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the constant definition and its usage
rg -A 3 "PubSubMessageNames" --type kotlin
# Search for any remaining usage of MessageEventType.Created.eventName
rg "MessageEventType\.Created\.eventName" --type kotlin
Length of output: 1266
Script:
#!/bin/bash
# Search for any historical usage of MessageEventType or similar event types
rg -B 2 -A 2 "MessageEventType|message\.created" --type kotlin
# Search for any tests or documentation that might reference the event names
fd -e kt -e md | xargs rg "chat\.message|message\.created"
Length of output: 4055
detekt.yml
Outdated
| @@ -98,8 +98,7 @@ complexity: | |||
| - 'use' | |||
| - 'with' | |||
| LabeledExpression: | |||
| active: true | |||
| ignoredLabels: [ 'async', 'coroutineScope' ] | |||
| active: false | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider disabling LabeledExpression check
The LabeledExpression check helps prevent confusion and maintains code clarity by ensuring proper usage of labels. Consider keeping this enabled, especially when working with nested lambdas and coroutines where labels are crucial for readability.
- active: false
+ active: trueAlso applies to: 101-101
|
Not sure why CI is not green : ( |
ttypic
force-pushed
the
ECO-5139/switch-to-v2
branch
from
3b8aea7 to
9fa9cbe
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
chat-android/src/main/java/com/ably/chat/Message.kt (1)
Line range hint
1-77: Consider adding API version documentationSince this is part of an API v2 upgrade, consider adding class-level documentation about:
- The API version compatibility
- Breaking changes from v1
- Migration guidelines for consumers
Example addition to the class documentation:
/** * Represents a single message in a chat room. + * + * @since API v2 + * @breaking-changes + * - 'timeserial' renamed to 'serial' + * - Added 'latestAction' for message lifecycle tracking */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
chat-android/src/main/java/com/ably/chat/ChatApi.kt(4 hunks)chat-android/src/main/java/com/ably/chat/EventTypes.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(3 hunks)chat-android/src/main/java/com/ably/chat/PaginatedResult.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Timeserial.kt(0 hunks)chat-android/src/test/java/com/ably/chat/ChatApiTest.kt(6 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(7 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(2 hunks)gradle/libs.versions.toml(1 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/Timeserial.kt
🚧 Files skipped from review as they are similar to previous changes (7)
- chat-android/src/main/java/com/ably/chat/EventTypes.kt
- chat-android/src/main/java/com/ably/chat/Messages.kt
- chat-android/src/main/java/com/ably/chat/PaginatedResult.kt
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt
- example/src/main/java/com/ably/chat/example/MainActivity.kt
- gradle/libs.versions.toml
🔇 Additional comments (6)
chat-android/src/main/java/com/ably/chat/Message.kt (3)
3-4: LGTM: Import added for MessageAction
The import is correctly added from the official Ably library to support the new message action functionality.
73-77:
Ensure comprehensive message lifecycle handling
While adding latestAction is a good step for tracking message states, the previous review identified missing handlers for different message actions.
Please ensure:
- All
MessageActionstates (CREATE/UPDATE/DELETE) have corresponding handlers - Message state transitions are properly managed
- UI correctly reflects message states
Consider adding documentation for:
- Valid state transitions
- UI handling for each state
- Error handling for invalid state transitions
22-22: Verify the impact of timeserial to serial migration
The property rename from timeserial to serial is a breaking change that affects message identification. The removal of comparison methods suggests changes in message ordering logic.
Let's verify the migration handling:
✅ Verification successful
Let me gather more information about message ordering and comparison logic with a different approach.
Let me try one final verification approach to check for any message ordering implementations.
Property rename from timeserial to serial appears to be a safe change
Based on the verification results:
- The only remaining references to
timeserialare in test files where it's used as a test value for the newserialproperty - No message ordering or comparison logic was found that depends on this property
- The Message class doesn't implement Comparable or any sorting interfaces
- No collection operations or sorting methods were found that rely on message ordering
The removal of comparison methods and property rename appears to be an intentional API change that doesn't break any existing functionality in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining timeserial references that need updating
rg 'timeserial' --type kotlin
# Search for message ordering/comparison logic
ast-grep --pattern 'fun $_(message1: Message, message2: Message) {
$$$
}'
Length of output: 521
Script:
#!/bin/bash
# Search for any comparison or ordering logic related to Message class
rg -A 5 "Message.*compare|compare.*Message|sort.*Message|Message.*sort" --type kotlin
# Check for any implementations or usages of Comparable interface with Message
ast-grep --pattern 'class Message : $$$Comparable<Message>$$$'
# Look for serial property usage in comparison contexts
rg -A 3 "\.serial.*compare|compare.*\.serial" --type kotlin
Length of output: 214
Script:
#!/bin/bash
# Check Message class implementation and its interfaces
ast-grep --pattern 'class Message$$$'
# Look for any collection operations with Message type
rg -A 3 "List<Message>|Collection<Message>" --type kotlin
# Check for any sorting or ordering methods in test files
rg -A 3 "sort|order|compare" chat-android/src/test --type kotlin
Length of output: 572
chat-android/src/main/java/com/ably/chat/ChatApi.kt (3)
8-8: LGTM!
The addition of MessageAction import supports the new message action functionality.
38-52:
Potential message loss due to null latestAction filtering.
The current implementation may exclude messages without a recognized latestAction, as they will be filtered out by mapNotNull. This could lead to unexpected data loss.
Consider handling messages without a recognized action by providing a default or unknown action type:
val latestActionName = it.requireJsonObject().get("latestAction")?.asString
-val latestAction = latestActionName?.let { name -> messageActionNameToAction[name] }
+val latestAction = latestActionName?.let { name ->
+ messageActionNameToAction[name]
+} ?: MessageAction.UNKNOWN
-latestAction?.let { action ->
- Message(
+Message(
serial = it.requireString("serial"),
clientId = it.requireString("clientId"),
roomId = it.requireString("roomId"),
text = it.requireString("text"),
createdAt = it.requireLong("createdAt"),
metadata = it.asJsonObject.get("metadata")?.toMap() ?: mapOf(),
headers = it.asJsonObject.get("headers")?.toMap() ?: mapOf(),
- latestAction = action,
+ latestAction = latestAction,
)
-}83-90: Verify MESSAGE_CREATE action aligns with API contract.
The hardcoded MessageAction.MESSAGE_CREATE for new messages seems logical, but we should ensure this aligns with the v2 API contract.
✅ Verification successful
MESSAGE_CREATE action is correctly aligned with API contract
Based on the evidence found in EventTypes.kt, the MESSAGE_CREATE action is part of a well-defined message action protocol where:
- It's mapped to the canonical event name "message.create"
- It's documented as "Action applied to a new message"
- It's part of a complete set of message lifecycle actions (create, update, delete)
- The action is imported from Ably's core library (
io.ably.lib.types.MessageAction)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for API documentation or tests that verify the MESSAGE_CREATE action
# Search for MESSAGE_CREATE usage in tests
echo "Checking MESSAGE_CREATE usage in tests:"
rg -A 5 'MESSAGE_CREATE'
# Search for API documentation
echo "Checking for API documentation:"
fd -t f "README.md" | xargs rg -A 5 "API.*v2|message.*action"
Length of output: 4934
Script:
#!/bin/bash
# Let's search for the MessageAction enum definition and its documentation
echo "Checking MessageAction enum definition:"
ast-grep --pattern 'enum class MessageAction'
# Let's also check for any API contract or interface definitions
echo "Checking for API interfaces and contracts:"
rg -l "interface.*Chat.*Api|interface.*Message.*Api"
# Let's look for any protocol documentation files
echo "Looking for protocol or API documentation files:"
fd -e md -e txt | xargs rg -l "protocol|api.*specification|message.*action"
Length of output: 471
Script:
#!/bin/bash
# Let's try a different approach to find MessageAction enum and its documentation
echo "Searching for MessageAction definition:"
rg -A 5 "enum.*MessageAction|sealed.*MessageAction"
# Let's also look for any message action related constants or string values
echo "Searching for message action string values:"
rg -A 3 '"message\.'
# Let's check the imports in ChatApi.kt to understand where MessageAction is defined
echo "Checking imports in ChatApi.kt:"
rg -A 10 "^import.*Message" "chat-android/src/main/java/com/ably/chat/ChatApi.kt"
Length of output: 3979
ttypic
force-pushed
the
ECO-5139/switch-to-v2
branch
from
9fa9cbe to
3d1c747
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
chat-android/src/test/java/com/ably/chat/TestUtils.kt (1)
Line range hint
24-48: Consider adding integration tests for v2 APIWhile the mock updates are correct, consider adding integration tests to verify the behavior of the actual v2 API endpoints. This could help identify any issues causing CI failures mentioned in the PR comments.
Some considerations:
- Verify that all v2 endpoints are available in the test environment
- Ensure the mock responses match the actual v2 API response structure
- Consider adding test cases for backward compatibility if needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
chat-android/src/main/java/com/ably/chat/ChatApi.kt(5 hunks)chat-android/src/main/java/com/ably/chat/EventTypes.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(3 hunks)chat-android/src/main/java/com/ably/chat/PaginatedResult.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Timeserial.kt(0 hunks)chat-android/src/test/java/com/ably/chat/ChatApiTest.kt(6 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(7 hunks)chat-android/src/test/java/com/ably/chat/TestUtils.kt(3 hunks)example/src/main/java/com/ably/chat/example/MainActivity.kt(2 hunks)gradle/libs.versions.toml(1 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/Timeserial.kt
🚧 Files skipped from review as they are similar to previous changes (8)
- chat-android/src/main/java/com/ably/chat/EventTypes.kt
- chat-android/src/main/java/com/ably/chat/Message.kt
- chat-android/src/main/java/com/ably/chat/Messages.kt
- chat-android/src/main/java/com/ably/chat/PaginatedResult.kt
- chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt
- example/src/main/java/com/ably/chat/example/MainActivity.kt
- gradle/libs.versions.toml
🔇 Additional comments (9)
chat-android/src/test/java/com/ably/chat/TestUtils.kt (3)
48-48: Verify completeness of API v2 migration
The occupancy endpoint update to v2 is correct. This completes the API version updates in the test utilities.
✅ Verification successful
All API endpoints have been successfully migrated to v2
The verification results show that all API endpoints in both the main source code (ChatApi.kt) and test utilities (TestUtils.kt) are consistently using /chat/v2/. No occurrences of /chat/v1/ were found in any Kotlin or test files, confirming complete migration to v2.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if all API endpoints in the codebase have been migrated to v2
rg "/chat/v[12]/" --type kotlin
# Check for any test files that might still be using old endpoints
rg "/chat/v1/" --type-add 'test:*.{kt,java}' -t test
Length of output: 928
24-24: Verify test coverage for v2 API endpoints
The API endpoint update from v1 to v2 looks good. However, we should ensure all test cases using this mock are updated to handle any potential changes in the v2 API response structure.
✅ Verification successful
API v2 migration is properly implemented in tests
The verification shows that:
- All test files are consistently using v2 endpoints
- No v1 endpoints remain in the test files
- The main implementation in
ChatApi.ktand test utilities inTestUtils.ktare aligned with v2 endpoints - Test cases in
ChatApiTest.ktare properly set up to work with the v2 API mocks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files that might need updates for v2 API
rg -l "mockMessagesApiResponse" --type-add 'test:*.{kt,java}' -t test
# Look for any remaining v1 references in test files
rg "/chat/v1/" --type-add 'test:*.{kt,java}' -t test
Length of output: 232
Script:
#!/bin/bash
# Check the content of ChatApiTest.kt to verify if it's using v2 endpoints
rg -A 5 -B 5 "mockMessagesApiResponse" chat-android/src/test/java/com/ably/chat/ChatApiTest.kt
# Check for any v2 endpoint usage in test files
rg "/chat/v2/" --type-add 'test:*.{kt,java}' -t test
Length of output: 1647
35-35: Verify v2 API response structure compatibility
The endpoint update to v2 is correct. However, please verify that the response structure handling in the mock matches any changes in the v2 API response format.
✅ Verification successful
Let me gather more information about the response structure by checking the implementation of the message sending functionality.
Let me check how the response is being handled in the tests and implementation.
Based on the gathered information, I can now generate the final response:
Response structure is compatible with v2 API
Looking at the test implementations and mocks:
- The response handling remains consistent, using
serialandcreatedAtfields which are present in both v1 and v2 responses - Test cases in
MessagesTest.ktandChatApiTest.ktdemonstrate the same response structure is maintained - The mock response builder wraps responses in the same way for both versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files using this mock to verify response handling
rg -A 5 "mockSendMessageApiResponse" --type-add 'test:*.{kt,java}' -t test
Length of output: 2449
Script:
#!/bin/bash
# Search for message sending implementation to check response handling
ast-grep --pattern 'fun $_ sendMessage($$$) {
$$$
}'
# Also check for any message-related data classes or interfaces
rg -A 10 "data class.*Message" --type kotlin
Length of output: 3652
Script:
#!/bin/bash
# Check the test response handling and actual implementation
rg -A 10 "onResponse.*callback" --type kotlin
rg -A 10 "requestAsync.*POST.*messages" --type kotlin
Length of output: 1036
chat-android/src/main/java/com/ably/chat/ChatApi.kt (6)
8-8: LGTM: Required import for MessageAction
The addition of MessageAction import supports the new message action handling in API v2.
34-34: LGTM: API endpoint updated to v2
The endpoint has been correctly updated to use the v2 API path.
77-77: LGTM: Consistent v2 endpoint update and message creation
The changes correctly implement:
- Updated endpoint to v2
- New message structure with serial and latestAction
Also applies to: 83-90
123-123: LGTM: Occupancy endpoint updated to v2
The occupancy endpoint has been updated to v2, resolving the API version inconsistency.
38-52:
Handle messages with unrecognized latestAction
The current implementation drops messages with null or unrecognized latestAction values.
Apply this fix to handle unknown actions:
val latestActionName = it.requireJsonObject().get("latestAction")?.asString
-val latestAction = latestActionName?.let { name -> messageActionNameToAction[name] }
+val latestAction = latestActionName?.let { name ->
+ messageActionNameToAction[name] ?: MessageAction.UNKNOWN
+} ?: MessageAction.UNKNOWN
-latestAction?.let { action ->
- Message(
+Message(
serial = it.requireString("serial"),
clientId = it.requireString("clientId"),
roomId = it.requireString("roomId"),
text = it.requireString("text"),
createdAt = it.requireLong("createdAt"),
metadata = it.asJsonObject.get("metadata")?.toMap() ?: mapOf(),
headers = it.asJsonObject.get("headers")?.toMap() ?: mapOf(),
latestAction = latestAction,
)
-}169-169: 🛠️ Refactor suggestion
Add error handling for transform failures
The nullable transform function could silently drop messages without proper error tracking.
Consider adding logging for transform failures:
private suspend fun <T> makeAuthorizedPaginatedRequest(
url: String,
method: String,
params: List<Param> = listOf(),
- transform: (JsonElement) -> T?,
+ transform: (JsonElement) -> T?,
) : PaginatedResult<T> = suspendCancellableCoroutine { continuation ->
+ fun logTransformFailure(element: JsonElement) {
+ logger.error(
+ "Failed to transform message",
+ staticContext = mapOf("message" to element.toString())
+ )
+ }
// ... rest of the implementation
// Update the transform usage to log failures
+ val transformWithLogging: (JsonElement) -> T? = { element ->
+ transform(element).also { result ->
+ if (result == null) logTransformFailure(element)
+ }
+ }
ttypic
force-pushed
the
ECO-5139/switch-to-v2
branch
from
3d1c747 to
19811b6
Compare
Code Coverage
|
Switched API to v2
Summary by CodeRabbit
New Features
latestActionproperty for messages to track their state (created, updated, deleted).Updated.Bug Fixes
Documentation
Chores
ablylibrary version to 1.2.46.