[ECO-5065] feat: add message reactions feature to chat API #139
Conversation
|
""" WalkthroughThe changes introduce a comprehensive message reactions feature to the chat system. This includes new APIs for sending and deleting reactions, structured models for reaction summaries, integration with message retrieval, room and message options for reactions, and both unit and integration tests validating reaction functionality and event subscriptions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MessagesReactions
participant ChatApi
participant Server
Client->>MessagesReactions: send(messageSerial, name, type, count)
MessagesReactions->>ChatApi: sendMessageReaction(roomId, messageSerial, type, name, count)
ChatApi->>Server: POST /rooms/{roomId}/messages/{messageSerial}/reactions
Server-->>ChatApi: Response
ChatApi-->>MessagesReactions: Result
MessagesReactions-->>Client: Completion
Client->>MessagesReactions: delete(messageSerial, name, type)
MessagesReactions->>ChatApi: deleteMessageReaction(roomId, messageSerial, type, name)
ChatApi->>Server: DELETE /rooms/{roomId}/messages/{messageSerial}/reactions
Server-->>ChatApi: Response
ChatApi-->>MessagesReactions: Result
MessagesReactions-->>Client: Completion
Server-->>MessagesReactions: Reaction summary/annotation event
MessagesReactions-->>Client: Notifies subscribers with event
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (9)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🔇 Additional comments (8)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 3
🧹 Nitpick comments (6)
chat-android/src/main/java/com/ably/chat/Messages.kt (2)
32-35: Document lifecycle ofreactionsproperty
reactionsremains accessible even afterMessages.dispose()is called, yet any subsequent call will operate on an already-disposedDefaultMessagesReactionsinstance. Please either
- set
_reactionsto a no-op/failed state after disposal, or- clarify in KDoc that the property must not be used once the parent
Messagesis disposed.
528-534: Dispose reactions before other teardown to avoid event leaks
_reactions.dispose()is invoked after the channel’s state listener is removed and maps are cleared.
If_reactionshappens to unregister listeners on the same channel this ordering is harmless, but calling it first (or inside atry/finally) is safer and guarantees all internal listeners are dropped even if an earlier teardown step throws.chat-android/src/main/java/com/ably/chat/ChatApi.kt (1)
50-66: Gracefully handle malformedreactionsJSON
getMessagesblindly casts withgetAsJsonObject(MessageProperty.Reactions).
If the backend ever returns a non-object or corrupt payload this will throwIllegalStateExceptionand break pagination.
Consider guarding withtakeIf { it.isJsonObject }or catchingIllegalStateExceptionand logging instead of propagating.chat-android/src/main/java/com/ably/chat/Message.kt (1)
139-149:with(MessageEvent)keeps outdated reactionsThe helper copies the incoming
event.messagebut then resetsreactions = this.reactions, discarding any newer summary that might have arrived between the two events.If the intention is to preserve the latest reactions, consider comparing versions or timestamps before overwriting.
chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt (1)
128-140: Consider adding timeout to avoid hanging tests
waitForRawReactionsrelies on the test finishing only when all events arrive.
Wrappingawait()inwithTimeout()(e.g. 5 s) will fail fast instead of hanging indefinitely when infrastructure is flaky.chat-android/src/main/java/com/ably/chat/RoomOptions.kt (1)
244-257: Avoid over-provisioning channel modes
annotation_publishis added unconditionally, even whenmessages.rawMessageReactions == false.
If the application never sends reactions this widens the channel capabilities it must request/authorise for no benefit (principle of least privilege).-val channelModes = mutableSetOf( - ChannelMode.publish, ChannelMode.subscribe, - ChannelMode.presence, ChannelMode.annotation_publish) +val channelModes = mutableSetOf( + ChannelMode.publish, ChannelMode.subscribe, ChannelMode.presence).apply { + if (messages.rawMessageReactions) add(ChannelMode.annotation_publish) +}Same condition you already use for
annotation_subscribe, keeps behaviour unchanged for reaction-enabled rooms while tightening others.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
chat-android/src/main/java/com/ably/chat/ChatApi.kt(4 hunks)chat-android/src/main/java/com/ably/chat/EventTypes.kt(0 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(7 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(4 hunks)chat-android/src/main/java/com/ably/chat/MessagesReactions.kt(1 hunks)chat-android/src/main/java/com/ably/chat/RoomOptions.kt(7 hunks)chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt(1 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/EventTypes.kt
🧰 Additional context used
🧬 Code Graph Analysis (5)
chat-android/src/main/java/com/ably/chat/ChatApi.kt (1)
chat-android/src/main/java/com/ably/chat/Message.kt (1)
buildMessageReactions(222-234)
chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt (2)
chat-android/src/test/java/com/ably/chat/room/RoomTestHelpers.kt (2)
createMockRealtimeClient(50-62)createMockChatApi(82-86)chat-android/src/test/java/io/ably/lib/realtime/RealtimeUtils.kt (1)
buildRealtimeChannel(24-29)
chat-android/src/main/java/com/ably/chat/Message.kt (1)
chat-android/src/main/java/com/ably/chat/Utils.kt (1)
clientError(200-200)
chat-android/src/main/java/com/ably/chat/RoomOptions.kt (1)
chat-android/src/main/java/com/ably/chat/Utils.kt (2)
ablyException(173-181)ablyException(183-186)
chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (1)
chat-android/src/main/java/com/ably/chat/Utils.kt (1)
clientError(200-200)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: coverage
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (4)
chat-android/src/main/java/com/ably/chat/Message.kt (2)
96-100: Nice additionExposing reaction summaries on the
Messageinterface makes the model self-contained. 👍
222-234: Defensive parsing looks good
buildMessageReactionssafely defaults to empty maps and isolates JSON parsing. Looks solid.chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt (1)
40-54: Excellent coverageThe tests verify default-option propagation and explicit-parameter precedence for reaction type. Good job.
chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (1)
268-272: Potential unbounded memory growth
rawEventBusis created withextraBufferCapacity = UNLIMITED.
If the downstream consumer stops collecting (e.g. UI screen in background), the flow retains every raw reaction event and can exhaust memory under high traffic.Consider:
- Switching to a bounded buffer similar to the summary bus (e.g. 1 or 64) with
DROP_OLDEST.- Or pause the annotation subscription when no
subscribeRawlisteners are registered.Either approach caps memory usage without losing critical data for typical UI use-cases.
ttypic
force-pushed
the
ECO-5065/room-reactions
branch
from
79925d9 to
8a071fd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (1)
309-334: Still missing input validation insend()Blank reaction names and invalid
countvalues slip through, hitting the server. The same feedback was raised previously.
🧹 Nitpick comments (5)
chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt (1)
128-137:receivedReactionsis mutated from a listener thread – not thread-safe
mutableListOfisn’t safe for concurrent writes. Use a thread-safe collection or guard mutations withsynchronizedto avoid rareConcurrentModificationExceptions in CI.chat-android/src/main/java/com/ably/chat/RoomOptions.kt (2)
244-257:annotation_publishenabled unconditionally
annotation_publishis added even when reactions are disabled. Consider adding it only when the room can actually publish reactions to reduce unnecessary channel capabilities exposure.- val channelModes = mutableSetOf(ChannelMode.publish, ChannelMode.subscribe, ChannelMode.presence, ChannelMode.annotation_publish) + val channelModes = mutableSetOf(ChannelMode.publish, ChannelMode.subscribe, ChannelMode.presence) + if (reactionsEnabled() || messages.rawMessageReactions) { + channelModes += ChannelMode.annotation_publish + }
229-234: Extend validation to new message-level options
validateRoomOptions()still checks only typing throttle. Add sanity checks, e.g. positivecountdefault, or forbidrawMessageReactions=truewhen corresponding channel mode isn’t requested.chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (2)
260-262: Minor naming nit –rawEventlisteners
rawEventlisteners→rawEventListenersfor consistency with Kotlin naming conventions.
368-375: GuardsubscribeRaw()when raw reactions are disabledIf
messages.rawMessageReactionsis false, callers will silently receive no events. Consider throwing early or logging a warning to aid debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
chat-android/src/main/java/com/ably/chat/ChatApi.kt(4 hunks)chat-android/src/main/java/com/ably/chat/EventTypes.kt(0 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(7 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(4 hunks)chat-android/src/main/java/com/ably/chat/MessagesReactions.kt(1 hunks)chat-android/src/main/java/com/ably/chat/RoomOptions.kt(7 hunks)chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt(1 hunks)gradle/libs.versions.toml(1 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/EventTypes.kt
✅ Files skipped from review due to trivial changes (1)
- gradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (4)
- chat-android/src/main/java/com/ably/chat/Messages.kt
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/Message.kt
- chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt
🧰 Additional context used
🧬 Code Graph Analysis (1)
chat-android/src/main/java/com/ably/chat/RoomOptions.kt (1)
chat-android/src/main/java/com/ably/chat/Utils.kt (2)
ablyException(173-181)ablyException(183-186)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: coverage
🔇 Additional comments (1)
chat-android/src/main/java/com/ably/chat/RoomOptions.kt (1)
18-42: Switch to non-nullable option objects is a breaking API change
presence,typing,reactions,occupancy, and newmessagesare now mandatory. Down-stream callers that previously passednullwill no longer compile. Confirm semantic-versioning expectations and update release notes.
chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt
Outdated
Show resolved
Hide resolved
ttypic
force-pushed
the
ECO-5065/room-reactions
branch
from
8a071fd to
ce11c4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (2)
352-354: Wrong variable in error message (still unfixed)The exception references nullable
typeinstead of the resolvedreactionType, yielding confusing messages.- throw clientError("cannot delete reaction of type $type without a name") + throw clientError("cannot delete reaction of type $reactionType without a name")
319-337: Input validation forsend()still missingBlank
names and invalidcountvalues are not rejected early. See previous review comment with concrete diff.
🧹 Nitpick comments (1)
chat-android/src/main/java/com/ably/chat/RoomOptions.kt (1)
173-179:EquatableRoomOptionsdiscards futurereactionsstate
reactionsis always mapped to a singletonEquatableRoomReactionsOptions, so any fields added later toMutableRoomReactionsOptionswill never participate in equality checks or caching logic.
Convert the actual instance instead of a constant placeholder (or keep the mapping but add a TODO to revisit when the interface stops being empty).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
chat-android/src/main/java/com/ably/chat/ChatApi.kt(4 hunks)chat-android/src/main/java/com/ably/chat/EventTypes.kt(0 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(7 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(4 hunks)chat-android/src/main/java/com/ably/chat/MessagesReactions.kt(1 hunks)chat-android/src/main/java/com/ably/chat/RoomOptions.kt(7 hunks)chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt(1 hunks)gradle/libs.versions.toml(1 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/EventTypes.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- gradle/libs.versions.toml
- chat-android/src/main/java/com/ably/chat/Messages.kt
- chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt
- chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/Message.kt
🧰 Additional context used
🧬 Code Graph Analysis (1)
chat-android/src/main/java/com/ably/chat/RoomOptions.kt (1)
chat-android/src/main/java/com/ably/chat/Utils.kt (2)
ablyException(173-181)ablyException(183-186)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: check
- GitHub Check: coverage
🔇 Additional comments (1)
chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (1)
267-275: Unbounded raw-event buffer can exhaust memory
MutableSharedFlowis created withextraBufferCapacity = UNLIMITED; if listeners are slow, every reaction event will pile up.
Use a bounded buffer (or back-pressure) to avoid an OOM risk.- private val rawEventBus = MutableSharedFlow<MessageReactionRawEvent>( - extraBufferCapacity = UNLIMITED, - onBufferOverflow = BufferOverflow.DROP_OLDEST, - ) + private val rawEventBus = MutableSharedFlow<MessageReactionRawEvent>( + extraBufferCapacity = 64, // tune as appropriate + onBufferOverflow = BufferOverflow.DROP_OLDEST, + )⛔ Skipped due to learnings
Learnt from: sacOO7 PR: ably/ably-chat-kotlin#124 File: chat-android/src/main/java/com/ably/chat/Utils.kt:0-0 Timestamp: 2025-04-28T11:41:59.070Z Learning: The `AwaitableSharedFlow` class in `com.ably.chat` intentionally uses `Channel.UNLIMITED` buffer capacity to ensure `tryEmit()` never fails due to buffer constraints, which is an important design requirement for the Ably Chat Kotlin SDK.
ttypic
force-pushed
the
ECO-5065/room-reactions
branch
from
ce11c4b to
04dddcb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
chat-android/src/main/java/com/ably/chat/RoomOptions.kt (1)
244-256: ChannelMode.annotation_publish must be added conditionally
annotation_publishis still inserted unconditionally even when a room does not request raw reactions.
This widens the capability set for every room, risking permission errors for customers with restricted keys and breaking the principle of least privilege.- val channelModes = mutableSetOf(ChannelMode.publish, ChannelMode.subscribe, ChannelMode.presence, ChannelMode.annotation_publish) + val channelModes = mutableSetOf(ChannelMode.publish, ChannelMode.subscribe, ChannelMode.presence) + + // Enable publish-side annotations only if the room opted-in for raw reactions + if (messages.rawMessageReactions) { + channelModes.add(ChannelMode.annotation_publish) + }Please gate
annotation_publishbehindmessages.rawMessageReactions, update the affected unit tests, and re-run the integration suite.chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (2)
352-355: Wrong variable referenced in error pathThe exception message still interpolates the nullable
typeparameter instead of the resolvedreactionType, leading to confusing errors (e.g. “…type null…”).- throw clientError("cannot delete reaction of type $type without a name") + throw clientError("cannot delete reaction of type $reactionType without a name")
313-338: Missing input validation forsend()
send()accepts blanknames and anycountregardless of reaction type.
Validate early to surface client errors before making a network call:require(name.isNotBlank()) { "reaction name must not be blank" } if (reactionType != MessageReactionType.Multiple && count != 1) { throw clientError("count must be 1 for reaction type $reactionType") }
🧹 Nitpick comments (3)
chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (2)
264-266: Minor: inconsistent variable casing
rawEventlisteners→rawEventListenersfor consistency withlisteners.
A tiny rename avoids cognitive friction.
272-275: Unlimited buffer may exhaust memory under heavy load
rawEventBusis configured withUNLIMITEDextra buffer capacity. A flood of reactions could OOM the client.
Consider back-pressure (e.g. fixed size with DROP_OLDEST) or making the capacity configurable.chat-android/src/test/java/com/ably/chat/MessagesTest.kt (1)
198-204: Defensive check around captured listeners
channelStateListenerSlots.first()will throw if no listener was captured (e.g. if mocking logic changes).
UsefirstOrNull()?.onChannelStateChanged(...)and fail the test explicitly if null for clearer diagnostics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
chat-android/src/main/java/com/ably/chat/ChatApi.kt(4 hunks)chat-android/src/main/java/com/ably/chat/EventTypes.kt(0 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(7 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(4 hunks)chat-android/src/main/java/com/ably/chat/MessagesReactions.kt(1 hunks)chat-android/src/main/java/com/ably/chat/RoomOptions.kt(7 hunks)chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/MonitoringTest.kt(1 hunks)gradle/libs.versions.toml(1 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/EventTypes.kt
✅ Files skipped from review due to trivial changes (1)
- chat-android/src/test/java/com/ably/chat/room/lifecycle/MonitoringTest.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- gradle/libs.versions.toml
- chat-android/src/main/java/com/ably/chat/Messages.kt
- chat-android/src/main/java/com/ably/chat/Message.kt
- chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt
- chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
🧰 Additional context used
🧠 Learnings (1)
chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt (2)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#89
File: chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt:44-48
Timestamp: 2024-12-10T14:47:31.766Z
Learning: In tests in `RoomFeatureSharedChannelTest.kt` for the Ably Chat Kotlin project, the order of `ChannelMode.publish` and `ChannelMode.subscribe` in the `modes` array does not affect functionality; assertions should not enforce a specific order for these modes.
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#75
File: chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt:43-45
Timestamp: 2024-12-02T12:10:18.954Z
Learning: In the `RoomFeatureSharedChannelTest.kt` tests, prefer using assertions like `Assert.assertEquals` that provide detailed error messages over general assertions like `Assert.assertTrue`, to aid in debugging when a test fails due to a missing mode.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: check
- GitHub Check: coverage
🔇 Additional comments (2)
chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt (2)
74-83: Test now hard-codesannotation_publish– will break once the mode is gatedThe assertion list mirrors the current (incorrect) unconditional behaviour.
After fixingRoomOptions.channelOptions()as noted earlier, update this expectation to includeannotation_publishonly whenmessages.rawMessageReactionsis true.
108-112: Presence ofannotation_publishin second assertion block may become invalidSame reasoning as above – the test will fail once the production code stops always adding
annotation_publish.
Please adapt the expected size / content accordingly.
ttypic
force-pushed
the
ECO-5065/room-reactions
branch
from
04dddcb to
1a10c14
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
chat-android/src/main/java/com/ably/chat/RoomOptions.kt (1)
244-257:annotation_publishstill added unconditionally – resurrected issueThe mode is added regardless of whether the client has opted-in to raw reactions. This widens required capabilities and can break apps running with restricted API keys (exactly the concern raised in the previous review).
- val channelModes = mutableSetOf(ChannelMode.publish, ChannelMode.subscribe, ChannelMode.presence, ChannelMode.annotation_publish) + val channelModes = mutableSetOf(ChannelMode.publish, ChannelMode.subscribe, ChannelMode.presence) ... - if (messages.rawMessageReactions) { - channelModes.add(ChannelMode.annotation_subscribe) + if (messages.rawMessageReactions) { + channelModes.add(ChannelMode.annotation_publish) + channelModes.add(ChannelMode.annotation_subscribe) }Please gate
annotation_publishthe same way you already gateannotation_subscribe.chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (2)
313-338: Missing upfront argument validation insend()Empty reaction names or invalid
countvalues are still allowed through, even though this was highlighted earlier. Surface these errors client-side to avoid needless network calls.+ require(name.isNotBlank()) { "reaction name must not be blank" } + if (reactionType != MessageReactionType.Multiple && count != 1) { + throw clientError("count must be 1 for reaction type $reactionType") + }
352-355: Error message uses wrong variable and omits validation
$typemay benull, resulting in a confusing exception message. UsereactionType, and enforce the same non-blank rule as insend().- throw clientError("cannot delete reaction of type $type without a name") + throw clientError("cannot delete reaction of type $reactionType without a name")Also add
require(!(name?.isBlank() ?: false))for non-Uniquedeletions.
🧹 Nitpick comments (1)
chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (1)
270-276: UnboundedrawEventBusmay lead to memory pressure
extraBufferCapacity = UNLIMITEDrisks OOM if the consumer lags. Consider a reasonable bound (e.g. 10 000) or useDROP_OLDESTwith a finite buffer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
chat-android/src/main/java/com/ably/chat/ChatApi.kt(4 hunks)chat-android/src/main/java/com/ably/chat/EventTypes.kt(0 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(7 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(4 hunks)chat-android/src/main/java/com/ably/chat/MessagesReactions.kt(1 hunks)chat-android/src/main/java/com/ably/chat/RoomOptions.kt(7 hunks)chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/MonitoringTest.kt(1 hunks)gradle/libs.versions.toml(1 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/EventTypes.kt
🚧 Files skipped from review as they are similar to previous changes (9)
- gradle/libs.versions.toml
- chat-android/src/test/java/com/ably/chat/room/lifecycle/MonitoringTest.kt
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt
- chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt
- chat-android/src/main/java/com/ably/chat/Messages.kt
- chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt
- chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/Message.kt
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: coverage
- GitHub Check: check
Code Coverage
|
|
Spec PR -> ably/specification#319 |
| Assert.assertEquals(ChannelMode.publish, capturedChannelOptions[0].modes[0]) | ||
| Assert.assertEquals(ChannelMode.subscribe, capturedChannelOptions[0].modes[1]) | ||
| Assert.assertEquals(ChannelMode.presence, capturedChannelOptions[0].modes[2]) | ||
| Assert.assertEquals(ChannelMode.annotation_publish, capturedChannelOptions[0].modes[3]) |
There was a problem hiding this comment.
CHA-MR9, for future regressions, can you add buildRoomOptions with rawMessageReactions=true
We can assert on
Assert.assertEquals(ChannelMode.annotation_subscribe, capturedChannelOptions[0].modes[4])
ttypic
force-pushed
the
ECO-5065/room-reactions
branch
2 times, most recently
from
4c1ba0f to
0825b75
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
chat-android/src/main/java/com/ably/chat/RoomOptions.kt (1)
244-259: Critical issue: annotation_publish is enabled unconditionally.The
ChannelMode.annotation_publishis added to all rooms regardless of whether reactions are enabled. This could cause permission errors with restricted API keys and unnecessarily widens the capability set.The previous review comment about this issue has not been addressed. The code should conditionally add
annotation_publishonly when reactions are explicitly enabled:- val channelModes = mutableSetOf(ChannelMode.publish, ChannelMode.subscribe, ChannelMode.presence, ChannelMode.annotation_publish) + val channelModes = mutableSetOf(ChannelMode.publish, ChannelMode.subscribe, ChannelMode.presence) + + if (messages.rawMessageReactions) { + channelModes.add(ChannelMode.annotation_publish) + }chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (3)
334-359: Add input validation for better error handling.The
send()method accepts anyname(including empty/blank) and anycountfor all reaction types without validation. This can lead to unnecessary network calls and confusing server-side errors.Add upfront validation as suggested in previous reviews:
override suspend fun send( messageSerial: String, name: String, type: MessageReactionType?, count: Int, ) { + require(messageSerial.isNotBlank()) { "messageSerial must not be blank" } + require(name.isNotBlank()) { "reaction name must not be blank" } + val reactionType = type ?: options.defaultMessageReactionType + + if (reactionType != MessageReactionType.Multiple && count != 1) { + throw clientError("count must be 1 for reaction type $reactionType") + } logger.trace(
361-383: Fix error message variable reference.The error message on line 374 incorrectly references the nullable
$typeparameter instead of the non-nullreactionTypevariable, which could display "null" in error messages.+ require(messageSerial.isNotBlank()) { "messageSerial must not be blank" } + if (reactionType != MessageReactionType.Unique && name == null) { - throw clientError("cannot delete reaction of type $type without a name") + throw clientError("cannot delete reaction of type $reactionType without a name") }
394-401: Add validation for raw message reactions subscription.The
subscribeRaw()method should verify that raw message reactions are enabled before allowing subscription, as per specification CHA-MR7a.override fun subscribeRaw(listener: MessagesReactions.MessageRawReactionListener): Subscription { + if (!options.rawMessageReactions) { + throw clientError("Raw message reactions must be enabled to subscribe to raw events") + } + logger.trace("MessagesReactions.subscribeRaw()") rawEventlisteners.add(listener)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
chat-android/src/main/java/com/ably/chat/ChatApi.kt(4 hunks)chat-android/src/main/java/com/ably/chat/EventTypes.kt(0 hunks)chat-android/src/main/java/com/ably/chat/Message.kt(7 hunks)chat-android/src/main/java/com/ably/chat/Messages.kt(4 hunks)chat-android/src/main/java/com/ably/chat/MessagesReactions.kt(1 hunks)chat-android/src/main/java/com/ably/chat/RoomOptions.kt(7 hunks)chat-android/src/test/java/com/ably/chat/MessageTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/MessagesTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/room/lifecycle/MonitoringTest.kt(1 hunks)gradle/libs.versions.toml(1 hunks)
💤 Files with no reviewable changes (1)
- chat-android/src/main/java/com/ably/chat/EventTypes.kt
🚧 Files skipped from review as they are similar to previous changes (9)
- chat-android/src/test/java/com/ably/chat/room/lifecycle/MonitoringTest.kt
- gradle/libs.versions.toml
- chat-android/src/test/java/com/ably/chat/room/RoomFeatureSharedChannelTest.kt
- chat-android/src/test/java/com/ably/chat/MessagesTest.kt
- chat-android/src/main/java/com/ably/chat/Messages.kt
- chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt
- chat-android/src/main/java/com/ably/chat/ChatApi.kt
- chat-android/src/main/java/com/ably/chat/Message.kt
- chat-android/src/test/java/com/ably/chat/integration/MessageReactionsIntegrationTest.kt
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: coverage
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (16)
chat-android/src/test/java/com/ably/chat/MessageTest.kt (6)
18-42: LGTM! Version comparison logic test is well-structured.The test correctly validates that when an event version is less than or equal to the current message version, the same message instance is returned (using
assertSame). This prevents unnecessary updates and maintains referential equality.
47-67: LGTM! Message update test validates proper version handling.The test correctly verifies that when an event has a higher version, the message content is updated with the event's data. The assertion checks the updated text content as expected.
72-95: LGTM! Serial mismatch validation is properly tested.The test correctly validates that attempting to apply an event with a different serial throws an
AblyExceptionwith the expected error code (40_000) and status code (400). This ensures data integrity.
100-127: LGTM! Reaction summary update test is comprehensive.The test validates that applying a
MessageReactionSummaryEventcorrectly updates the message's reactions by comparing the expected updated message with the actual result. The test covers all reaction types (unique, distinct, multiple).
132-151: LGTM! Serial mismatch validation for reaction events is correct.The test properly validates that applying a
MessageReactionSummaryEventwith a different message serial throws anAblyExceptionwith appropriate error codes, maintaining data consistency.
178-196: LGTM! Helper function provides good test setup.The
createMessagehelper function provides reasonable defaults and allows for easy customization of message properties in tests. The function covers all necessary fields for creating test messages.chat-android/src/main/java/com/ably/chat/RoomOptions.kt (4)
18-41: LGTM! Non-nullable property change improves API clarity.Converting the optional properties to non-nullable with the addition of the
messagesproperty makes the API more predictable and easier to use. The documentation is clear about the purpose of each property.
91-115: LGTM! MessageOptions interface is well-designed.The interface provides clear configuration for message reactions with:
rawMessageReactionsflag for controlling individual reaction eventsdefaultMessageReactionTypefor setting the default reaction behavior- Comprehensive documentation explaining the purpose and defaults
144-148: LGTM! Implementation classes provide sensible defaults.The
MutableMessageOptionsclass provides appropriate defaults:
rawMessageReactions = false(conservative default)defaultMessageReactionType = MessageReactionType.Distinct(common use case)
254-256: LGTM! Conditional annotation_subscribe is correctly implemented.The code properly adds
ChannelMode.annotation_subscribeonly whenrawMessageReactionsis enabled, which matches the expected behavior for subscribing to individual reaction events.chat-android/src/main/java/com/ably/chat/MessagesReactions.kt (6)
22-86: LGTM! Comprehensive interface design for message reactions.The
MessagesReactionsinterface provides a well-structured API with:
- Clear method signatures for send/delete operations
- Proper separation of summary vs raw event subscriptions
- Well-documented parameters and return types
- Functional interfaces for event listeners
88-144: LGTM! Well-defined enums with proper documentation.The enum definitions are comprehensive:
MessageReactionEventTypecovers create/delete actionsMessageReactionSummaryEventTypefor summary eventsMessageReactionTypewith detailed spec references and use case examples- Proper companion object with utility method for type lookup
228-244: LGTM! Convenient extension functions for Message objects.The extension functions provide a more intuitive API by accepting
Messageobjects directly rather than requiring users to extract the serial manually. This improves developer experience.
272-332: LGTM! Well-structured implementation with proper resource management.The
DefaultMessagesReactionsclass demonstrates good practices:
- Proper coroutine scope management with supervisor job
- Thread-safe collections for listeners
- Appropriate event bus configuration with buffer overflow handling
- Proper subscription management and cleanup in
dispose()
409-452: LGTM! Robust message summary event handling.The
internalMessageSummaryListenermethod properly:
- Filters for summary events with serials
- Handles empty summaries (when all reactions are deleted)
- Correctly parses different reaction types from the summary
- Emits well-formed events to the event bus
454-515: LGTM! Comprehensive annotation event handling.The
internalAnnotationListenermethod demonstrates excellent error handling:
- Validates required fields with appropriate logging
- Handles unknown types and actions gracefully
- Special handling for unique reaction deletes (which may have empty names)
- Proper default value handling for counts
- Well-structured event creation and emission
chat-android/src/test/java/com/ably/chat/MessagesReactionsTest.kt
Outdated
Show resolved
Hide resolved
Introduced support for message reactions in the chat API, enabling the addition, deletion, and subscription to reactions. Implemented related interfaces, listeners, data types, and integration tests to ensure proper functionality and integration with messages.
…d error tests Added validation to ensure `messageSerial` is not empty in `send`, `delete`, and `subscribeRaw` methods. Updated tests to cover validation scenarios.
Enhanced validation in the `send` method to ensure proper usage of `count`, `name`, and `type` parameters. Updated tests to cover new validation scenarios.
ttypic
force-pushed
the
ECO-5065/room-reactions
branch
from
78a6a43 to
1f329de
Compare
Introduced support for message reactions in the chat API, enabling the addition, deletion, and subscription to reactions. Implemented related interfaces, listeners, data types, and integration tests to ensure proper functionality and integration with messages.
Summary by CodeRabbit
New Features
Bug Fixes
Tests