fix: latestAction -> action renaming + test
#85
Conversation
WalkthroughThe changes in this pull request primarily involve renaming the JSON field Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
🔇 Additional comments (1)example/src/main/java/com/ably/chat/example/MainActivity.kt (1)
The change from Let's verify that this example correctly reflects the core API usage: ✅ Verification successfulProperty rename from The verification confirms that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Verify the Message class property rename across the codebase
# Expected: All instances of Message creation should use 'action' instead of 'latestAction'
# Search for any remaining instances of 'latestAction' in Message class usage
rg 'latestAction.*MessageAction' --type kotlin
# Search for new 'action' property usage to confirm consistent adoption
rg 'action.*=.*MessageAction' --type kotlin
Length of output: 1236 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: 2
🧹 Outside diff range and nitpick comments (3)
chat-android/src/main/java/com/ably/chat/Message.kt (1)
76-76: Update documentation to reflect renamed propertyThe documentation comment above still refers to "latest action" while the property has been renamed to
action. Please update the comment to maintain consistency.- /** - * The latest action of the message. This can be used to determine if the message was created, updated, or deleted. - */ + /** + * The action of the message. This can be used to determine if the message was created, updated, or deleted. + */chat-android/src/test/java/com/ably/chat/ChatApiTest.kt (1)
35-35: LGTM! Consider adding explicit action value testsThe property rename from
latestActiontoactionis consistently applied across all test cases. However, consider adding explicit tests for different action values (create, update, delete) to ensure the property rename handles all cases correctly.Example test case:
@Test fun `getMessages should handle different message actions`() = runTest { val actions = listOf( "message.create" to MessageAction.MESSAGE_CREATE, "message.update" to MessageAction.MESSAGE_UPDATE, "message.delete" to MessageAction.MESSAGE_DELETE ) for ((actionString, expectedAction) in actions) { mockMessagesApiResponse( realtime, listOf( JsonObject().apply { addProperty("serial", "timeserial") addProperty("roomId", "roomId") addProperty("clientId", "clientId") addProperty("text", "hello") addProperty("createdAt", 1_000_000) addProperty("action", actionString) } ) ) val messages = chatApi.getMessages("roomId", QueryOptions()) assertEquals(expectedAction, messages.items.first().action) } }Also applies to: 52-52, 69-69, 106-106
chat-android/src/main/java/com/ably/chat/ChatApi.kt (1)
38-38: Consider renaming variable for consistency.The variable name
latestActionNameshould be renamed toactionNameto maintain consistency with the new naming convention.- val latestActionName = it.requireJsonObject().get("action")?.asString + val actionName = it.requireJsonObject().get("action")?.asString
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
chat-android/src/main/java/com/ably/chat/ChatApi.kt(3 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(1 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(1 hunks)chat-android/src/test/java/com/ably/chat/ChatApiTest.kt(4 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/SandboxTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/TestUtils.kt(1 hunks)
🔇 Additional comments (5)
chat-android/src/test/java/com/ably/chat/TestUtils.kt (1)
Line range hint 72-80: LGTM! Good enhancement to support suspend functions
The modification to accept a suspend block parameter improves the utility's flexibility for testing asynchronous operations while maintaining proper timeout and context handling.
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (1)
131-152: LGTM! Well-structured test for message history.
The test effectively verifies message sending and retrieval functionality, with proper assertions for all relevant message properties.
chat-android/src/main/java/com/ably/chat/ChatApi.kt (1)
50-50: LGTM! Consistent property renaming.
The action property is consistently used in Message construction across both methods.
Also applies to: 90-90
chat-android/src/test/java/com/ably/chat/MessagesTest.kt (2)
78-78: LGTM! Tests properly updated for property renaming.
The tests have been consistently updated to use the new action property name in all relevant assertions.
Also applies to: 138-138
Line range hint 38-38: Verify complete migration of property renaming.
Let's ensure no instances of latestAction remain in the codebase.
| @@ -290,7 +290,7 @@ internal class DefaultMessages( | |||
| text = data.text, | |||
| metadata = data.metadata, | |||
| headers = pubSubMessage.extras.asJsonObject().get("headers")?.toMap() ?: mapOf(), | |||
| latestAction = MessageAction.MESSAGE_CREATE, | |||
| action = MessageAction.MESSAGE_CREATE, | |||
There was a problem hiding this comment.
💡 Codebase verification
Remaining instances of latestAction found in the codebase
The verification revealed that latestAction is still being used in two files:
ChatApi.kt: Used as local variables (latestActionName,latestAction) for processing message actionsMainActivity.ktin the example app: Still using the old property name
The local variables in ChatApi.kt are fine as they serve a different purpose, but the example app needs to be updated to maintain consistency.
example/src/main/java/com/ably/chat/example/MainActivity.kt: UpdatelatestActiontoactionin Message construction
🔗 Analysis chain
LGTM: Property renamed from latestAction to action
The renaming aligns with the PR objectives to match changes in the chat-js repository. The hardcoded MessageAction.MESSAGE_CREATE value is consistent with the message listener's action check.
Let's verify the consistent renaming across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent renaming from latestAction to action across the codebase
# Check for any remaining instances of latestAction
echo "Checking for remaining instances of 'latestAction'..."
rg --type kotlin --type java "latestAction"
# Check for consistent usage of 'action' property in Message construction
echo "Checking Message construction patterns..."
ast-grep --pattern 'Message($$$, action = $_, $$$)'
Length of output: 7784
| headers = pubSubMessage.extras.asJsonObject().get("headers")?.toMap() ?: mapOf(), | ||
| latestAction = MessageAction.MESSAGE_CREATE, | ||
| action = MessageAction.MESSAGE_CREATE, | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Update needed in example app's MainActivity
The Message class constructor has been properly updated to use action parameter, but there's an inconsistency in the example app's MainActivity.kt which still uses the old latestAction parameter name. This needs to be updated to match the current Message class definition.
example/src/main/java/com/ably/chat/example/MainActivity.kt:323: ChangelatestAction = MessageAction.MESSAGE_CREATEtoaction = MessageAction.MESSAGE_CREATE
🔗 Analysis chain
Verify Message class constructor parameter update
While the property renaming here looks correct, we should ensure the Message class constructor has been updated to accept action instead of latestAction.
Let's verify the Message class changes:
Let me generate another verification script to check the Message class definition since the previous pattern didn't match.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Message class constructor parameter update
# Check Message class definition
echo "Checking Message class definition..."
ast-grep --pattern 'class Message($$$, action: MessageAction, $$$)'
# Check all Message instantiations
echo "Checking Message instantiations..."
ast-grep --pattern 'Message($$$)'
Length of output: 8257
Script:
#!/bin/bash
# Search for Message class definition with a more flexible pattern
echo "Checking Message class definition..."
rg -A 10 "^data class Message\b" --type kotlin
# Also check for any remaining references to latestAction
echo -e "\nChecking for latestAction references..."
rg "latestAction" --type kotlin
Length of output: 1534
Script:
#!/bin/bash
# Get more context around Message class definition
echo "Getting full Message class definition..."
rg -B 5 -A 20 "^data class Message\b" chat-android/src/main/java/com/ably/chat/Message.kt
# Get context around latestAction usage in MainActivity
echo -e "\nChecking MainActivity latestAction context..."
rg -B 5 -A 5 "latestAction = MessageAction" example/src/main/java/com/ably/chat/example/MainActivity.kt
Length of output: 1164
ttypic
force-pushed
the
fix-action-field
branch
from
8fc510a to
7627409
Compare
Follow chat-js changes ably/ably-chat-js#427
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores