[AIT-98] feat: realtime edits and deletes #1183
Conversation
WalkthroughCentralizes update/delete/append message handling into ChannelBase.updateDeleteImpl, replaces CompletionListener with typed Callback across send/ack paths, embeds per-message PublishResult[] in ProtocolMessage.res, adds PublishResult type and listener adapters, and updates tests for realtime message edits and versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Channel as ChannelBase
participant Validator as updateDeleteImpl
participant ListenersUtil as Listeners
participant ConnMgr as ConnectionManager
participant Server
Client->>Channel: updateMessage(message, op, Callback<UpdateDeleteResult>)
Channel->>Validator: updateDeleteImpl(message, op, action, listener)
Validator->>Validator: validate serial, clientId, version, connection/queue rules
Validator->>ListenersUtil: toPublishResultListener(listener)
Validator->>ConnMgr: send(ProtocolMessage{action, version, ...}, Callback<PublishResult>)
ConnMgr->>Server: transmit ProtocolMessage
Server-->>ConnMgr: ACK with PublishResult[] (ProtocolMessage.res)
ConnMgr->>ConnMgr: onAck -> map per-message PublishResult[] to queued callbacks
ConnMgr->>ListenersUtil: invoke Callback<PublishResult>.onSuccess(result)
ListenersUtil->>Channel: adapter converts PublishResult -> UpdateDeleteResult
Channel->>Client: original Callback<UpdateDeleteResult>.onSuccess(result)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/1183/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI Agents
In @lib/src/main/java/io/ably/lib/realtime/ChannelBase.java:
- Around line 1357-1396: The ProtocolMessage is sending the original Message
object instead of the constructed updatedMessage, so the action, version and
encoded data are lost; change the send call to use updatedMessage (i.e., set
msg.messages = new Message[] { updatedMessage }) so the ProtocolMessage includes
the modified action/version/encoded payload before connectionManager.send(...).
- Around line 1374-1381: The code in ChannelBase constructs an updatedMessage
(setting action and version fields) but then sends the original message, and
updatedMessage also never copies the original message.serial; fix by sending the
updatedMessage and ensuring its serial is copied from the original message:
populate updatedMessage.serial = message.serial after building updatedMessage,
and replace msg.messages = new Message[] { message } with msg.messages = new
Message[] { updatedMessage } (or remove updatedMessage construction if you
intend to send the original message instead).
In @lib/src/main/java/io/ably/lib/types/PublishResult.java:
- Around line 52-63: In PublishResult msgpack deserialization the inner loop
declares j but mistakenly tests and increments i, corrupting the outer loop and
leaving serials mostly unset; change the inner loop to iterate on j (e.g., for
(int j = 0; j < count; j++) ) and use j++ so unpacker results are stored into
serials[j] while leaving the outer variable i untouched, keeping the existing
nil check/unpack logic and final return new PublishResult(serials).
In
@lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java:
- Around line 309-324: The test getMessageVersions_async currently only
publishes and reads history synchronously; update it to call the asynchronous
API getMessageVersionsAsync for the published message (using the
publishedMessage.serial from waitForMessageAppearInHistory) and wait for the
async result (future/callback) to complete with a timeout; then assert that the
returned versions array is non-empty and contains an entry whose serial equals
publishedMessage.serial (and any other expected fields). Ensure you handle
errors by failing the test on exception and cleanly await the async completion
before finishing the test.
- Around line 39-45: Add an @After cleanup method (e.g., tearDownAfter) to close
the AblyRealtime instance created in setUpBefore: implement a method annotated
with @After that checks if the field ably is non-null, calls ably.close() (or
the appropriate shutdown method on AblyRealtime), catches/logs any exceptions,
and sets ably to null to avoid resource leaks between tests.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/realtime/Presence.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/types/ProtocolMessage.javalib/src/main/java/io/ably/lib/types/PublishResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java
🧰 Additional context used
🧬 Code graph analysis (8)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (2)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)liveobjects/src/main/kotlin/io/ably/lib/objects/serialization/DefaultSerialization.kt (1)
writeMsgpackArray(23-27)
lib/src/main/java/io/ably/lib/types/PublishResult.java (2)
lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (9)
lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1)
AblyRealtime(31-319)lib/src/main/java/io/ably/lib/types/ChannelOptions.java (1)
ChannelOptions(13-131)lib/src/main/java/io/ably/lib/types/ClientOptions.java (1)
ClientOptions(19-392)lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (2)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
ProtocolMessage(34-375)lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
lib/src/main/java/io/ably/lib/util/Listeners.java (2)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java (1)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)
lib/src/main/java/io/ably/lib/realtime/Presence.java (2)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check (19)
- GitHub Check: check (29)
🔇 Additional comments (24)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java (1)
397-407: LGTM!The test correctly adapts to the new
Callback<PublishResult>interface for theconnectionManager.send()method. The callback signature change from parameterlessonSuccess()toonSuccess(PublishResult result)aligns with the broader refactoring to support per-message publish results.lib/src/main/java/io/ably/lib/realtime/Presence.java (3)
125-130: LGTM!The method signature change to accept
Callback<PublishResult>and the use ofListeners.unwrap(listener)to extract the underlyingCompletionListenerfor storage maintains backward compatibility with the existingQueuedPresencestructure while adapting to the new callback model.
768-768: LGTM!Correctly wraps the
CompletionListenerusingListeners.fromCompletionListener(listener)to match the updatedconnectionManager.send()signature.
897-897: LGTM!Consistent with the pattern used in
updatePresence- correctly wraps the listener for the send operation.lib/src/main/java/io/ably/lib/util/Listeners.java (2)
15-25: LGTM!The new adapter methods are well-designed:
toPublishResultListenerbridges theUpdateDeleteResultcallback toPublishResultcallbackunwrapsafely extracts the underlyingCompletionListenerwith proper null handling
49-71: LGTM!The
UpdateResultToPublishAdaptercorrectly:
- Handles null
listener,result,result.serials, and empty arrays- Extracts only the first serial for
UpdateDeleteResult(appropriate for single-message operations)- Properly delegates error handling
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (3)
140-141: LGTM!The new
resfield follows the established pattern for optional array fields inProtocolMessageand is properly annotated with@Nullable.
215-218: LGTM!The msgpack serialization for the
resfield correctly follows the pattern used for other array fields (e.g.,messages,annotations).
290-292: LGTM!The deserialization case for the
resfield is correctly integrated into the existing switch statement.lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (4)
51-74: LGTM!The
getMessage_retrieveBySerialtest correctly validates the publish-and-retrieve-by-serial flow.
79-111: LGTM!The
updateMessage_updateDatatest covers the core update flow with proper assertions for data, name, and action.
154-185: LGTM!The async update test correctly uses
Listeners.fromCompletionListenerand validates the callback-based flow.
386-426: LGTM!Comprehensive workflow test covering publish → update → verify → delete → verify deletion → verify version history. Well-structured end-to-end test.
lib/src/main/java/io/ably/lib/types/PublishResult.java (3)
15-27: LGTM!The class structure and
serialsfield documentation clearly explain the 1:1 correspondence with published messages and the null case for conflation.
71-90: LGTM!The array serialization/deserialization methods follow standard patterns used elsewhere in the codebase.
113-128: LGTM!The body handler correctly handles both JSON and msgpack content types and wraps decode exceptions appropriately.
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
1128-1128: LGTM!The
publishmethod correctly usesListeners.fromCompletionListener(listener)to adapt to the new callback model.
1217-1257: LGTM!The
updateMessagemethod overloads properly delegate to the centralized implementation, maintaining a clean API surface.
1714-1714: LGTM!Consistent with the pattern used in
publish- correctly wraps the listener for the send operation.lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (5)
31-42: LGTM!The added imports are appropriate for the new
Callback<PublishResult>type and null safety annotations.
1408-1410: LGTM!The
message.res(per-message publish results) is correctly propagated to the ack handler.
1728-1735: LGTM!The
QueuedMessageclass correctly updated to useCallback<PublishResult>. Note that this is a breaking change for the public class, but the AI summary indicates that callers have been updated to use adapter wrappers.
1737-1751: LGTM!The
sendmethod signature correctly updated to acceptCallback<PublishResult>. This is a breaking public API change that enables per-message publish result handling.
1831-1885: LGTM!The
ackmethod implementation correctly handles per-message results:
- Defensive bounds checking on the
resultsarray- Index-based iteration properly correlates results with acknowledged messages
- Callbacks are invoked outside the synchronized block, avoiding potential deadlocks
The design gracefully handles cases where
resultsis null or shorter than expected by passingnullto the callback. TheCallback<PublishResult>interface supports null values (no @nonnull annotation), and the codebase already establishes this pattern in implementations likeListeners.javaandCompletionListener.javathat handle null defensively.
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java
Show resolved
Hide resolved
ttypic
force-pushed
the
AIT-98/message-edits-deletes
branch
from
2846cde to
9f0407f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1183/features
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @lib/src/main/java/io/ably/lib/realtime/ChannelBase.java:
- Around line 1374-1381: The updatedMessage is missing the original message
serial so server cannot identify which message is targeted; set
updatedMessage.serial = message.serial after creating updatedMessage (in the
block where Message updatedMessage = new Message(message.name, message.data,
message.extras); is constructed) so the serial is preserved for
update/delete/append operations, keeping the rest of the version/client metadata
logic unchanged.
- Around line 1374-1395: The code prepares and encodes updatedMessage but then
sends the original message and checks clientId on the wrong object; change the
ProtocolMessage creation to use updatedMessage (msg.messages = new Message[] {
updatedMessage }) and call ably.auth.checkClientId(updatedMessage, true,
connected) before encoding; also add the same channel state guard used in
publish() (reject when channel is failed or suspended) so this method behaves
consistently with publish() and doesn't send messages when the channel is in an
invalid state.
In @lib/src/main/java/io/ably/lib/types/PublishResult.java:
- Around line 52-62: The inner array-unpacking loop for SERIALS in PublishResult
uses the wrong loop variable: it checks and increments outer variable i while
indexing with j (which is never incremented), causing an infinite loop and wrong
writes to serials[0]; change the loop to use j as the loop counter (for (int j =
0; j < count; j++)) and ensure the increment and condition reference j, leaving
unpacker logic and serials[j] writes intact so each element is populated
correctly via
unpacker.getNextFormat()/unpacker.unpackNil()/unpacker.unpackString().
In
@lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java:
- Around line 318-333: The test getMessageVersions_async only publishes and
reads history but never exercises the async message-versions API; update the
test to call the channel's async versions retrieval (e.g.,
channel.getMessageVersions or getMessageVersionsAsync) using the
publishedMessage.serial, register a callback/future to wait for the async
result, and assert the returned versions array/collection is non-empty and
contains expected data (timestamps/contents) and that there are no errors;
ensure you use the existing publishedMessage variable and verify callback
invocation before finishing the test.
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
71-81: Consider wrapping IOException in AblyException for consistency.Other serialization methods in the codebase throw
AblyExceptionfor IO errors. ThrowingRuntimeExceptionhere may cause inconsistent error handling behavior upstream.♻️ Proposed improvement
public static void writeMsgpackArray(PublishResult[] results, MessagePacker packer) { try { int count = results.length; packer.packArrayHeader(count); for (PublishResult result : results) { result.writeMsgpack(packer); } } catch (IOException e) { - throw new RuntimeException(e.getMessage(), e); + throw new AblyException.fromThrowable(e); } }Note: This would require changing the method signature to declare
throws AblyException, which may require updates to callers.lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (1)
41-42: Consider reducing the test timeout.A 5-minute (300 seconds) timeout seems excessive for these tests. The helper methods already use 5-second polling timeouts. A timeout of 60-120 seconds would likely be sufficient and would fail faster on CI if something goes wrong.
♻️ Suggested change
@Rule - public Timeout testTimeout = Timeout.seconds(300); + public Timeout testTimeout = Timeout.seconds(120);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/realtime/Presence.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/types/ProtocolMessage.javalib/src/main/java/io/ably/lib/types/PublishResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/main/java/io/ably/lib/util/Listeners.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java
🧰 Additional context used
🧬 Code graph analysis (6)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)
lib/src/main/java/io/ably/lib/types/PublishResult.java (2)
lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
HttpCore(39-583)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (10)
lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1)
AblyRealtime(31-319)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)lib/src/main/java/io/ably/lib/types/ChannelOptions.java (1)
ChannelOptions(13-131)lib/src/main/java/io/ably/lib/types/ClientOptions.java (1)
ClientOptions(19-392)lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)
lib/src/main/java/io/ably/lib/realtime/Presence.java (2)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (2)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
ProtocolMessage(34-375)lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check
- GitHub Check: check (21)
- GitHub Check: check (19)
- GitHub Check: check (29)
- GitHub Check: build
- GitHub Check: check (24)
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-okhttp
- GitHub Check: check (29)
🔇 Additional comments (16)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
140-141: LGTM!The
resfield addition and its MessagePack serialization/deserialization follow the established patterns in this class. The null-safe handling is consistent with other array fields likemessages,presence, andannotations.Also applies to: 166-166, 215-218, 290-292
lib/src/main/java/io/ably/lib/realtime/Presence.java (2)
125-130: LGTM!The callback type change from
CompletionListenertoCallback<PublishResult>integrates correctly with the new PublishResult flow. UsingListeners.unwrap()to extract the underlyingCompletionListenerfor storage inQueuedPresencemaintains backward compatibility with the internal queue processing.
768-768: LGTM!The
Listeners.fromCompletionListener(listener)wrapping is applied consistently in bothupdatePresenceandsendQueuedMessages, ensuring that presence operations correctly propagate through the newCallback<PublishResult>pipeline.Also applies to: 897-897
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
113-128: LGTM!The
PublishResultBodyHandlercorrectly handles both JSON and MessagePack content types and provides a sensible fallback (empty array) when parsing fails or returns null.lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (2)
481-518: LGTM!The polling helper methods are well-structured with reasonable 5-second timeouts and 200ms sleep intervals. The predicate-based approach for
waitForMessageAppearInVersionHistoryprovides good flexibility for different test scenarios.
184-185: No action required—callback types are compatible.
Listeners.fromCompletionListener()is a generic method (public static <T> Callback<T> fromCompletionListener(...)) that infers its type parameter from context. When passed tochannel.updateMessage()andchannel.deleteMessage(), which both expectCallback<UpdateDeleteResult>, the compiler correctly infersTasUpdateDeleteResult. The test code is type-safe and correct.lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (4)
1728-1735: LGTM!The
QueuedMessageclass update to useCallback<PublishResult>is clean and straightforward. The generic callback type enables proper per-message result propagation.
1737-1737: LGTM!The
send()andsendImpl()method signature changes to acceptCallback<PublishResult>are consistent with the broader refactoring to support per-message publish results.Also applies to: 1753-1753
1831-1831: LGTM!The
ack()method correctly propagates per-messagePublishResultto listeners. The bounds checkresults != null && results.length > isafely handles cases where the results array might be shorter than expected or null.Also applies to: 1873-1883
1408-1409: LGTM!The
onAckhandler correctly extractsmessage.resfrom the protocol message and passes it to the pending message queue'sack()method, enabling per-message result propagation.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (6)
34-34: LGTM!The new imports for
MessageActionandListenersare required for the refactored update/delete/append operations.Also applies to: 50-50
1128-1128: LGTM!The change to use
Listeners.fromCompletionListener(listener)aligns with the broader refactor to unify callback handling through theCallback<PublishResult>pattern inConnectionManager.
1217-1257: LGTM!The
updateMessageoverloads properly delegate to the centralizedupdateDeleteImplmethod with correct parameter forwarding.
1266-1306: LGTM!The
deleteMessageoverloads follow the same consistent delegation pattern toupdateDeleteImpl.
1315-1355: LGTM!The
appendMessageoverloads follow the same consistent delegation pattern toupdateDeleteImpl.
1712-1715: LGTM!The change to use
Listeners.fromCompletionListener(listener)is consistent with the refactoring pattern applied to thepublishmethod.
ttypic
force-pushed
the
AIT-99/message-appends
branch
from
9c5ac4a to
d4cf2d1
Compare
ttypic
force-pushed
the
AIT-98/message-edits-deletes
branch
from
9f0407f to
36cc288
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1183/features
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @lib/src/main/java/io/ably/lib/realtime/ChannelBase.java:
- Around line 1374-1395: The code prepares updatedMessage with action and
version but then sends the original message and omits the serial; update
ChannelBase where ProtocolMessage msg is built so msg.messages contains the
prepared updatedMessage (not message) and copy the original message.serial into
updatedMessage.serial before encoding/sending (ensure updatedMessage.serial is
set from message.serial), then send msg via connectionManager.send as before.
In @lib/src/main/java/io/ably/lib/types/PublishResult.java:
- Around line 52-63: The SERIALS deserialization loop incorrectly uses the
outer-loop variable i instead of the inner-loop variable j, causing misreads; in
the block that unpacks the serials array (where count, serials, and the for loop
are defined), change the loop condition and increment to use j (for (int j = 0;
j < count; j++)) and ensure all array writes use serials[j], leaving
unpacker.getNextFormat(), unpacker.unpackNil(), unpacker.unpackString(), and the
new PublishResult(serials) unchanged.
In
@lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java:
- Around line 321-339: The test getMessageVersions_async is incomplete: after
publishing and retrieving publishedMessage, call the async API to fetch message
versions (use Channel.getMessageVersions or the async variant provided by the
SDK) passing publishedMessage.serial and a callback/future, wait for the async
result, then assert the response is non-null, contains at least one version, and
that the earliest/latest version data matches "Original data" (or the expected
payload); add appropriate timeout handling (similar to
waitForMessageAppearInHistory) and meaningful assert messages so the test
actually verifies async version retrieval.
🧹 Nitpick comments (3)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
71-81: Consider propagating IOException instead of wrapping in RuntimeException.
writeMsgpackArraycatchesIOExceptionand wraps it in aRuntimeException. This is inconsistent withreadMsgpackArraywhich propagatesIOException. For consistency and proper error handling, consider declaringthrows IOException.♻️ Suggested improvement
- public static void writeMsgpackArray(PublishResult[] results, MessagePacker packer) { - try { - int count = results.length; - packer.packArrayHeader(count); - for (PublishResult result : results) { - result.writeMsgpack(packer); - } - } catch (IOException e) { - throw new RuntimeException(e.getMessage(), e); - } + public static void writeMsgpackArray(PublishResult[] results, MessagePacker packer) throws IOException { + int count = results.length; + packer.packArrayHeader(count); + for (PublishResult result : results) { + result.writeMsgpack(packer); + } }lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (2)
42-43: Consider reducing the test timeout.A 300-second (5-minute) timeout is quite long for unit/integration tests. While message operations may need time, consider whether a shorter timeout (e.g., 60 seconds) would be sufficient while still allowing adequate time for network operations.
111-111: Use diamond operator for type inference.♻️ Minor cleanup
- Helpers.AsyncWaiter<UpdateDeleteResult> waiter = new Helpers.AsyncWaiter(); + Helpers.AsyncWaiter<UpdateDeleteResult> waiter = new Helpers.AsyncWaiter<>();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/realtime/Presence.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/types/ProtocolMessage.javalib/src/main/java/io/ably/lib/types/PublishResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java
- lib/src/main/java/io/ably/lib/types/ProtocolMessage.java
🧰 Additional context used
🧬 Code graph analysis (3)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (6)
lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)
lib/src/main/java/io/ably/lib/realtime/Presence.java (2)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check (19)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check-liveobjects
- GitHub Check: check (29)
- GitHub Check: check-rest-okhttp
🔇 Additional comments (18)
lib/src/main/java/io/ably/lib/util/Listeners.java (3)
15-17: LGTM!The adapter factory method correctly creates a wrapper to translate
PublishResultcallbacks toUpdateDeleteResultcallbacks, enabling integration between the new callback-based flow and existing update/delete result handling.
19-25: LGTM!The
unwrapmethod provides a clean way to extract the underlyingCompletionListenerfrom a wrapped callback, with appropriate null handling when the callback isn't a wrapper.
49-71: LGTM!The adapter correctly translates
PublishResulttoUpdateDeleteResultby extracting the first serial from the array. The null-safety checks forresult,result.serials, and array length are properly implemented.lib/src/main/java/io/ably/lib/realtime/Presence.java (3)
125-130: LGTM!The method signature change to
Callback<PublishResult>aligns with the broader API shift. UsingListeners.unwrap(listener)correctly extracts the underlyingCompletionListenerfor storage inQueuedPresence, maintaining compatibility with the existing queued presence handling.
768-768: LGTM!Properly wraps the
CompletionListenerusingListeners.fromCompletionListenerbefore passing toConnectionManager.send, aligning with the newCallback<PublishResult>signature.
897-897: LGTM!Correctly wraps the listener for the new callback-based ConnectionManager API when sending queued presence messages.
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
92-128: LGTM!The body handler correctly handles both JSON and msgpack content types, with appropriate error handling and fallback to empty array.
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (1)
487-524: LGTM on helper methods!The polling helpers are well-structured with appropriate timeouts and sleep intervals. The predicate-based approach in
waitForMessageAppearInVersionHistoryprovides flexibility for different wait conditions.lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (4)
1728-1734: LGTM!The
QueuedMessageclass is correctly updated to useCallback<PublishResult>instead ofCompletionListener, aligning with the new callback-based architecture for publish operations.
1737-1751: LGTM!The
sendmethod signature change toCallback<PublishResult>is consistent with the broader refactoring to support per-message publish results.
1831-1885: LGTM!The
ackmethod correctly handles the newPublishResult[]parameter:
- Null-safe access to results array with bounds checking
- Properly maps each result to its corresponding message callback
- Maintains backward compatibility when results array is null or shorter than expected
1408-1410: LGTM!The
onAckmethod correctly passesmessage.res(thePublishResult[]from the protocol message) to the pending message acknowledgment handler.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (6)
34-34: LGTM!The new imports for
MessageActionandListenersare required for the refactored update/delete/append operations and the listener wrapping changes.Also applies to: 50-50
1128-1128: LGTM!The wrapping of
CompletionListenerwithListeners.fromCompletionListener()aligns with the updatedConnectionManager.send()signature that now expectsCallback<PublishResult>.
1211-1257: LGTM!The
updateMessageoverloads follow a consistent delegation pattern and properly funnel toupdateDeleteImplwithMessageAction.MESSAGE_UPDATE. The API is symmetric withdeleteMessageandappendMessage.
1260-1306: LGTM!The
deleteMessageoverloads mirror theupdateMessagepattern and correctly useMessageAction.MESSAGE_DELETE.
1309-1355: LGTM!The
appendMessageoverloads complete the symmetric API withMessageAction.MESSAGE_APPEND.
1712-1715: LGTM!The
sendProtocolMessagemethod correctly wraps theCompletionListenerto match the updatedConnectionManager.send()signature.
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
ttypic
force-pushed
the
AIT-98/message-edits-deletes
branch
from
36cc288 to
a80788d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1183/features
github-actions
bot
temporarily deployed
to
staging/pull/1183/javadoc
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @lib/src/main/java/io/ably/lib/realtime/ChannelBase.java:
- Around line 1357-1396: In updateDeleteImpl you build and encode updatedMessage
but then send the original message, losing action/version and potentially
serial; copy the original serial (e.g. updatedMessage.serial = message.serial)
and any other identifying fields you need (timestamp/id if applicable), and
change the ProtocolMessage construction to send updatedMessage (msg.messages =
new Message[] { updatedMessage }) instead of the original message; also consider
running ably.auth.checkClientId against the same updatedMessage you encode/send.
In @lib/src/main/java/io/ably/lib/transport/ConnectionManager.java:
- Around line 1408-1410: PendingMessageQueue.ack currently adjusts msgSerial and
count when msgSerial < startSerial but doesn't offset the PublishResult[]
(results), causing misalignment between acked messages and their callbacks;
update PendingMessageQueue.ack (and the same logic in the other affected block)
to compute a resultsOffset = startSerial - originalMsgSerial (or equivalent)
whenever you shift msgSerial, then apply that offset when selecting/iterating
results and when passing PublishResult items to pending message callbacks so
each callback receives the matching PublishResult for its message; ensure any
array slicing, indexing, or loop start uses resultsOffset consistently and
preserve original behaviour when no offset is needed.
In @lib/src/main/java/io/ably/lib/types/PublishResult.java:
- Around line 42-69: In readMsgpack, the inner array loop uses the wrong loop
variable and mutates the outer loop index (for (int j = 0; i < count; i++)),
breaking parsing; change that inner loop to iterate j (for (int j = 0; j <
count; j++)) and increment j, assigning serials[j] inside, so the outer variable
i is not modified and the serials array is correctly populated when handling the
SERIALS field using the unpacker.
In
@lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java:
- Around line 52-55: The tearDown method in RealtimeChannelMessageEditTest
unconditionally calls ably.close(), which can throw a NullPointerException if
setUp failed and ably is null; update tearDown to guard the call (e.g., if (ably
!= null) ably.close() or use Objects.requireNonNullElse/try-finally pattern) so
close() is only invoked when ably was successfully initialized and ensure the
test class variable name ably and method tearDown are used to locate the change.
🧹 Nitpick comments (1)
liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
4-32: Guard coroutine resumption against cancellation/double-callback.Today
continuation.resume(...)can throw if the coroutine was cancelled before the callback fires (or if an error+success bug happens upstream).Proposed hardening
internal suspend fun ObjectsAdapter.sendAsync(message: ProtocolMessage) = suspendCancellableCoroutine { continuation -> try { connectionManager.send(message, clientOptions.queueMessages, object : Callback<PublishResult> { override fun onSuccess(result: PublishResult) { - continuation.resume(Unit) + if (continuation.isActive) continuation.resume(Unit) } override fun onError(reason: ErrorInfo) { - continuation.resumeWithException(ablyException(reason)) + if (continuation.isActive) continuation.resumeWithException(ablyException(reason)) } }) } catch (e: Exception) { - continuation.resumeWithException(e) + if (continuation.isActive) continuation.resumeWithException(e) } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/realtime/Presence.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/types/ProtocolMessage.javalib/src/main/java/io/ably/lib/types/PublishResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.javaliveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/main/java/io/ably/lib/types/ProtocolMessage.java
- lib/src/main/java/io/ably/lib/realtime/Presence.java
🧰 Additional context used
🧬 Code graph analysis (4)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java (2)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
ProtocolMessage(34-375)lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (5)
lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (2)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
ProtocolMessage(34-375)lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)
lib/src/main/java/io/ably/lib/types/PublishResult.java (3)
lib/src/main/java/io/ably/lib/http/HttpCore.java (1)
HttpCore(39-583)lib/src/main/java/io/ably/lib/util/Serialisation.java (1)
Serialisation(40-299)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: build
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check
- GitHub Check: check (29)
🔇 Additional comments (3)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java (1)
22-27: Callback migration in test is correct.Using
Callback<PublishResult>here preserves the intent (assert onError path) and safely ignores the success payload.Also applies to: 397-407
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
15-25: Listener adapters look sound (null-safe + correct 1st-serial mapping).Also applies to: 49-71
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1101-1129: publish() now correctly bridges CompletionListener -> Callback via Listeners.
ttypic
force-pushed
the
AIT-99/message-appends
branch
3 times, most recently
from
b852631 to
58541a7
Compare
ttypic
force-pushed
the
AIT-98/message-edits-deletes
branch
2 times, most recently
from
a80788d to
d4684b0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1183/features
github-actions
bot
temporarily deployed
to
staging/pull/1183/javadoc
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @lib/src/main/java/io/ably/lib/realtime/ChannelBase.java:
- Around line 1384-1392: In ChannelBase, the catch block around
ably.auth.checkClientId(...) and updatedMessage.encode(...) should notify errors
using callCompletionListenerError(listener, e.errorInfo) (not only
listener.onError) so null-safe listener handling matches publish; additionally
if listener is null (i.e., synchronous overloads like updateMessage(Message)),
rethrow the AblyException so callers receive the failure instead of it being
silently swallowed. Update the catch to call callCompletionListenerError(...)
and then if listener == null throw e; otherwise return after notification.
🧹 Nitpick comments (6)
lib/src/main/java/io/ably/lib/types/PublishResult.java (3)
29-31: Specify charset for JSON parsing.Using
new String(packed)relies on the platform's default charset, which can vary and cause encoding issues. Explicitly use UTF-8.♻️ Suggested fix
public static PublishResult readFromJson(byte[] packed) throws MessageDecodeException { - return Serialisation.gson.fromJson(new String(packed), PublishResult.class); + return Serialisation.gson.fromJson(new String(packed, java.nio.charset.StandardCharsets.UTF_8), PublishResult.class); }
71-81: Consider consistent exception handling.
writeMsgpackArraywrapsIOExceptioninRuntimeException, whilereadMsgpack(byte[])wraps it inAblyException. This inconsistency could make error handling unpredictable for callers.♻️ Suggested fix for consistency
- public static void writeMsgpackArray(PublishResult[] results, MessagePacker packer) { + public static void writeMsgpackArray(PublishResult[] results, MessagePacker packer) throws AblyException { try { int count = results.length; packer.packArrayHeader(count); for (PublishResult result : results) { result.writeMsgpack(packer); } } catch (IOException e) { - throw new RuntimeException(e.getMessage(), e); + throw AblyException.fromThrowable(e); } }Note: This would require updating the call site in
ProtocolMessage.writeMsgpackto handle the exception, which already throwsIOException, so the change may not be necessary if the calling context handles exceptions appropriately.
113-128: Unrecognized content types silently return empty array.When
contentTypeis neitherapplication/jsonnorapplication/x-msgpack,publishResultremains null and an empty array is returned. Consider logging or throwing for unexpected content types.lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (3)
112-112: Use parameterized type forAsyncWaiter.Raw type
Helpers.AsyncWaitershould use the generic type parameter for type safety.Suggested fix
- Helpers.AsyncWaiter<UpdateDeleteResult> waiter = new Helpers.AsyncWaiter(); + Helpers.AsyncWaiter<UpdateDeleteResult> waiter = new Helpers.AsyncWaiter<>();
514-536: Empty catch blocks may hide real errors.The empty
catch (Exception e) {}blocks inwaitForUpdatedMessageAppearandwaitForDeletedMessageAppearswallow all exceptions silently. This can mask unexpected errors and cause tests to timeout rather than fail fast with meaningful error messages.Consider logging the exception or at least checking for expected exception types (e.g., 404 for message not found yet).
Suggested improvement for waitForUpdatedMessageAppear
private Message waitForUpdatedMessageAppear(Channel channel, String serial) throws Exception { long timeout = System.currentTimeMillis() + 5_000; while (true) { try { Message message = channel.getMessage(serial); if ((message != null && message.action == MessageAction.MESSAGE_UPDATE) || System.currentTimeMillis() > timeout) return message; Thread.sleep(200); - } catch (Exception e) {} + } catch (AblyException e) { + // Expected when message not yet available, continue polling + if (System.currentTimeMillis() > timeout) throw e; + Thread.sleep(200); + } } }
499-500: Consider adding parentheses for clarity.The boolean expression relies on operator precedence (
&&before||). While the current behavior is correct, explicit parentheses would improve readability.Suggested fix
- if (history.items().length > 0 && predicate.test(history.items()) || System.currentTimeMillis() > timeout) + if ((history.items().length > 0 && predicate.test(history.items())) || System.currentTimeMillis() > timeout)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/realtime/Presence.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/types/ProtocolMessage.javalib/src/main/java/io/ably/lib/types/PublishResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.javaliveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/src/main/java/io/ably/lib/util/Listeners.java
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java
- lib/src/main/java/io/ably/lib/realtime/Presence.java
🧰 Additional context used
🧬 Code graph analysis (4)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (8)
lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)lib/src/main/java/io/ably/lib/types/ChannelOptions.java (1)
ChannelOptions(13-131)lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (2)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
ProtocolMessage(34-375)lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check-liveobjects
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check (19)
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check-realtime-okhttp
- GitHub Check: check (29)
- GitHub Check: check (21)
🔇 Additional comments (15)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java (1)
33-34: LGTM!The new
RealtimeChannelMessageEditTest.classis correctly added to the test suite, following the existing pattern.liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
18-32: LGTM!The callback type migration from
CompletionListenertoCallback<PublishResult>aligns with the broader PR changes. ThePublishResultparameter inonSuccessis intentionally unused here sincesendAsynconly needs success/failure signaling for coroutine resumption.lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (3)
140-141: LGTM!The new
resfield follows the existing pattern for nullable array fields inProtocolMessage.
215-218: LGTM!MsgPack serialization of
resfollows the established pattern used for other array fields likemessages,presence, andannotations.
290-292: LGTM!MsgPack deserialization of
rescorrectly usesPublishResult.readMsgpackArrayand follows the existing switch-case pattern.lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (3)
1728-1734: LGTM!The
QueuedMessageclass correctly updated to useCallback<PublishResult>instead ofCompletionListener, aligning with the new callback model.
1408-1409: LGTM!The
onAckmethod correctly passesmessage.resto theackmethod, enabling per-message publish results to flow through to listeners.
1831-1885: LGTM with a note on null handling.The ack handling correctly maps
PublishResult[]entries to corresponding acknowledged messages using index-based iteration. The bounds checkresults != null && results.length > iproperly handles cases where the results array might be shorter than expected or null.The known
Callback<PublishResult>implementations in the codebase properly handle null results:UpdateResultToPublishAdapterexplicitly checksresult != nullbefore accessing its properties, andCompletionListenerWrapperignores the result parameter entirely.lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (3)
38-56: LGTM!Test class setup and teardown are well-structured. The 5-minute timeout is appropriate for integration tests that involve network operations and polling.
316-319: Verify version history ordering assumption.The test assumes
versions.items()[versions.items().length - 1]contains the latest version. This depends on the API's default ordering (likelyforwards). Consider explicitly setting thedirectionparameter to ensure consistent behavior, or add a comment clarifying the expected ordering.
454-493: LGTM!Good coverage for testing the append message functionality, verifying both the history state and realtime subscription behavior including the
MESSAGE_APPENDaction and delta data.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (4)
1121-1130: LGTM!The change to use
Listeners.fromCompletionListener(listener)properly wraps theCompletionListenerfor use with the updatedConnectionManager.sendAPI that now expects aCallback<PublishResult>.
1217-1257: LGTM!The
updateMessageoverloads follow a clean delegation pattern, routing all variants through the centralupdateDeleteImplmethod. The API surface provides flexibility for callers with or withoutMessageOperationmetadata and callbacks.
1308-1355: LGTM!The
appendMessageoverloads follow the same consistent delegation pattern asupdateMessageanddeleteMessage, routing all variants throughupdateDeleteImplwithMessageAction.MESSAGE_APPEND.
1713-1716: LGTM!The
sendProtocolMessagemethod is correctly updated to useListeners.fromCompletionListener(listener), maintaining consistency with the publish method and the updatedConnectionManager.sendAPI.
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java
Outdated
Show resolved
Hide resolved
ttypic
force-pushed
the
AIT-98/message-edits-deletes
branch
from
d4684b0 to
8924312
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1183/features
github-actions
bot
temporarily deployed
to
staging/pull/1183/javadoc
ttypic
force-pushed
the
AIT-99/message-appends
branch
from
58541a7 to
31969a8
Compare
ttypic
force-pushed
the
AIT-98/message-edits-deletes
branch
from
8924312 to
6dac2cf
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1183/features
github-actions
bot
temporarily deployed
to
staging/pull/1183/javadoc
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (1)
1831-1885: Critical:PendingMessageQueue.ack()misalignsresults[]when adjustingmsgSerial < startSerial.When you reduce
countand advancemsgSerialtostartSerial, you also need to skip the corresponding leading entries inresults, otherwise callbacks receive the wrongPublishResult.Proposed fix
- public void ack(long msgSerial, int count, @Nullable PublishResult[] results, ErrorInfo reason) { + public void ack(long msgSerial, int count, @Nullable PublishResult[] results, ErrorInfo reason) { QueuedMessage[] ackMessages = null, nackMessages = null; + int resultsOffset = 0; synchronized(this) { if (queue.isEmpty()) return; long startSerial = queue.get(0).msg.msgSerial; if(msgSerial < startSerial) { /* this is an error condition and shouldn't happen but * we can handle it gracefully by only processing the * relevant portion of the response */ - count -= (int)(startSerial - msgSerial); + resultsOffset = (int)(startSerial - msgSerial); + count -= resultsOffset; if(count < 0) count = 0; msgSerial = startSerial; } if(msgSerial > startSerial) { /* this counts as a nack of the messages earlier than serial, * as well as an ack */ int nCount = (int)(msgSerial - startSerial); + if (nCount > queue.size()) nCount = queue.size(); List<QueuedMessage> nackList = queue.subList(0, nCount); nackMessages = nackList.toArray(new QueuedMessage[nCount]); nackList.clear(); startSerial = msgSerial; } if(msgSerial == startSerial) { + if (count > queue.size()) count = queue.size(); List<QueuedMessage> ackList = queue.subList(0, count); ackMessages = ackList.toArray(new QueuedMessage[count]); ackList.clear(); } } @@ if(ackMessages != null) { for (int i = 0; i < ackMessages.length; i++) { QueuedMessage msg = ackMessages[i]; try { if (msg.listener != null) { - PublishResult messageResult = results != null && results.length > i ? results[i] : null; + int idx = i + resultsOffset; + PublishResult messageResult = results != null && results.length > idx ? results[idx] : null; msg.listener.onSuccess(messageResult); } } catch (Throwable t) { Log.e(TAG, "ack(): listener exception", t); } } } }
🤖 Fix all issues with AI agents
In
@lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java:
- Around line 357-393: The subscription's mismatch branch currently calls
appendWaiter.onError(null) which wrongly fails the test when unrelated messages
arrive; update the channel.subscribe callback (the lambda that checks
msg.serial) to only call appendWaiter.onSuccess(msg) when msg.serial equals
serial (and optionally check msg.action == MessageAction.MESSAGE_APPEND), and
otherwise do nothing (ignore unrelated messages); also ensure you unsubscribe or
remove the listener after success to avoid spurious future triggers.
- Around line 48-51: The tearDown method currently calls ably.close() unguarded
which can throw an NPE if ably is null; update the @After method tearDown() to
check that the ably field is non-null before calling close (e.g., if (ably !=
null) ably.close()) so the cleanup is safe when setup failed or tests didn't
initialize ably.
🧹 Nitpick comments (2)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (1)
395-436: Avoid swallowing exceptions + make timeouts fail loudly in polling helpers.Catching
Exceptionand ignoring it can mask regressions; also returning “whatever we last got” on timeout makes failures harder to diagnose than an explicit assert/fail (e.g., “timed out waiting for MESSAGE_UPDATE for serial …”).lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1210-1397: Verify message-edit wire semantics: avoid sending an “empty”version, and align auth-check with the sent message.Today
updatedMessage.versionis always non-null (Line 1377), which may change the payload compared to omitting it when there’s no operation metadata. AlsocheckClientId(...)runs onmessagebut you encode/sendupdatedMessage(Line 1385-1396).Proposed tightening
- Message updatedMessage = new Message(message.name, message.data, message.extras); + Message updatedMessage = new Message(message.name, message.data, message.extras); updatedMessage.serial = message.serial; updatedMessage.action = action; - updatedMessage.version = new MessageVersion(); if (operation != null) { + updatedMessage.version = new MessageVersion(); updatedMessage.version.clientId = operation.clientId; updatedMessage.version.description = operation.description; updatedMessage.version.metadata = operation.metadata; } @@ - ably.auth.checkClientId(message, true, connected); + ably.auth.checkClientId(updatedMessage, true, connected); updatedMessage.encode(options);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/realtime/Presence.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/types/ProtocolMessage.javalib/src/main/java/io/ably/lib/types/PublishResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.javaliveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/src/main/java/io/ably/lib/util/Listeners.java
- lib/src/main/java/io/ably/lib/types/PublishResult.java
- liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
🧰 Additional context used
🧬 Code graph analysis (5)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (6)
lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)lib/src/main/java/io/ably/lib/types/ChannelOptions.java (1)
ChannelOptions(13-131)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)
lib/src/main/java/io/ably/lib/realtime/Presence.java (2)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java (2)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-129)liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (2)
onSuccess(21-23)onSuccess(37-39)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check
- GitHub Check: check (29)
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check (29)
- GitHub Check: check (24)
- GitHub Check: build
- GitHub Check: check (19)
- GitHub Check: check (21)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-okhttp
🔇 Additional comments (8)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
154-219: MsgPack encode/decode forreslooks consistent; consider defensive handling for malformed/partial arrays.Encode/decode symmetry is good, but if you ever receive an ack with
msgSerial < startSerial(or other partial processing scenarios), downstream ack handling may need to tolerateresarrays that don’t align perfectly withcount. (See ConnectionManager review comment on results alignment.)Also applies to: 221-293
lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java (1)
33-35: Good: new test class is included in the realtime suite.lib/src/main/java/io/ably/lib/realtime/Presence.java (1)
754-773: Good: presence sends now consistently adapt CompletionListener -> Callback.Also applies to: 864-902
lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java (1)
21-26: Test update matches the newCallback<PublishResult>send API.Also applies to: 396-406
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (2)
1408-1410: Good:acknow plumbs per-message results (ProtocolMessage.res) into pending queue processing.
1728-1766: Good: send API migration toCallback<PublishResult>is consistent with queuing + pending tracking.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (2)
1101-1129: Good: publish path is updated cleanly viaListeners.fromCompletionListener(...).
1713-1716: Good:sendProtocolMessagenow uses the new internal callback type via Listeners.
| void addPendingPresence(PresenceMessage presenceMessage, Callback<PublishResult> listener) { | ||
| synchronized(channel) { | ||
| final QueuedPresence queuedPresence = new QueuedPresence(presenceMessage,listener); | ||
| final QueuedPresence queuedPresence = new QueuedPresence(presenceMessage, Listeners.unwrap(listener)); | ||
| pendingPresence.add(queuedPresence); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the Presence.java file
find . -type f -name "Presence.java" | grep -E "(lib|src)"Repository: ably/ably-java
Length of output: 112
🏁 Script executed:
# Let's look at the addPendingPresence method around lines 125-130
head -150 lib/src/main/java/io/ably/lib/realtime/Presence.java | tail -50Repository: ably/ably-java
Length of output: 2578
🏁 Script executed:
# Find the Listeners class to understand unwrap() behavior
find . -type f -name "Listeners.java" | head -5Repository: ably/ably-java
Length of output: 109
🏁 Script executed:
# Examine the Listeners.java file to understand unwrap() and fromCompletionListener()
cat -n ./lib/src/main/java/io/ably/lib/util/Listeners.javaRepository: ably/ably-java
Length of output: 2810
🏁 Script executed:
# Find all callers of addPendingPresence to see how it's being called
rg -B 3 -A 3 "addPendingPresence" lib/src/main/java/io/ably/lib/realtime/Presence.javaRepository: ably/ably-java
Length of output: 462
🏁 Script executed:
# Search for other callers of addPendingPresence in the entire codebase
rg "addPendingPresence" --type javaRepository: ably/ably-java
Length of output: 362
🏁 Script executed:
# Find the call to addPendingPresence in ChannelBase.java
rg -B 10 -A 5 "this.presence.addPendingPresence" lib/src/main/java/io/ably/lib/realtime/ChannelBase.javaRepository: ably/ably-java
Length of output: 872
🏁 Script executed:
# Find where queuedMessage.listener is created/set
rg -B 5 "queuedMessage.listener" lib/src/main/java/io/ably/lib/realtime/ChannelBase.java | head -100Repository: ably/ably-java
Length of output: 506
🏁 Script executed:
# Look at the QueuedMessage class to understand its listener field
rg -B 5 -A 15 "class QueuedMessage" lib/src/main/java/io/ably/lib/realtime/ChannelBase.javaRepository: ably/ably-java
Length of output: 40
🏁 Script executed:
# Search for QueuedMessage class definition in the codebase
rg -B 5 -A 20 "class QueuedMessage" --type javaRepository: ably/ably-java
Length of output: 2556
🏁 Script executed:
# Look at the ConnectionManager to see where QueuedMessage is created
find . -type f -name "ConnectionManager.java" | head -1 | xargs cat -n | head -200Repository: ably/ably-java
Length of output: 9747
🏁 Script executed:
# Search for where QueuedMessage listener is set in ConnectionManager
rg "new QueuedMessage" --type java -B 2 -A 5Repository: ably/ably-java
Length of output: 4022
🏁 Script executed:
# Find where presence messages are sent to ConnectionManager.send
rg "presence" lib/src/main/java/io/ably/lib/realtime/Presence.java -i -A 3 | grep -E "(send|Listeners\.from)" | head -20Repository: ably/ably-java
Length of output: 489
🏁 Script executed:
# Look for public methods in Presence that accept listeners and handle presence operations
rg -B 5 -A 10 "public.*Callback<PublishResult>" lib/src/main/java/io/ably/lib/realtime/Presence.javaRepository: ably/ably-java
Length of output: 40
🏁 Script executed:
# Search for calls to send() in Presence with presence messages
rg "connectionManager\.send|\.send\(" lib/src/main/java/io/ably/lib/realtime/Presence.java -B 3 -A 3Repository: ably/ably-java
Length of output: 941
🏁 Script executed:
# Let's check the full sendQueuedMessages method to understand the complete flow
rg -B 2 -A 30 "void sendQueuedMessages" lib/src/main/java/io/ably/lib/realtime/Presence.javaRepository: ably/ably-java
Length of output: 1478
🏁 Script executed:
# Check QueuedPresence class to see what it does with the listener
rg -B 5 -A 20 "class QueuedPresence" lib/src/main/java/io/ably/lib/realtime/Presence.javaRepository: ably/ably-java
Length of output: 1095
🏁 Script executed:
# Let's verify if there are any other code paths where addPendingPresence is called
rg "addPendingPresence\|QueuedPresence" --type javaRepository: ably/ably-java
Length of output: 40
🏁 Script executed:
# Double-check if addPendingPresence is called from anywhere else besides transferQueuedPresenceMessages
rg -r "addPendingPresence" --type java --entire-file lib/src/main/java/Repository: ably/ably-java
Length of output: 502
🏁 Script executed:
# Verify all the public presence methods to see the full public API
rg -B 3 "public.*CompletionListener\|public.*Callback<PublishResult>" lib/src/main/java/io/ably/lib/realtime/Presence.java | head -60Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
# Check if there are any tests that demonstrate how addPendingPresence should be used
find . -type f -name "*PresenceTest*.java" -o -name "*Presence*Test.java" | xargs rg "addPendingPresence" 2>/dev/null | head -20Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
# Check the visibility of addPendingPresence method
rg "addPendingPresence" lib/src/main/java/io/ably/lib/realtime/Presence.java -B 1 -A 1Repository: ably/ably-java
Length of output: 187
🏁 Script executed:
# Examine the complete flow - look at what listeners are stored in ConnectionManager.queuedMessages
rg -B 5 -A 10 "queuedMessages.add" lib/src/main/java/io/ably/lib/transport/ConnectionManager.javaRepository: ably/ably-java
Length of output: 1342
🏁 Script executed:
# Check if CompletionListenerWrapper is the only type of Callback that gets stored in QueuedMessage
rg "ConnectionManager.send\|new QueuedMessage" lib/src/main/java/io/ably/lib/realtime/Presence.java -B 3 -A 3Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
# Check the exact line where addPendingPresence is defined to confirm its visibility
sed -n '125,130p' lib/src/main/java/io/ably/lib/realtime/Presence.javaRepository: ably/ably-java
Length of output: 366
🏁 Script executed:
# Look at the complete Presence class to see all methods that take CompletionListener or callbacks
rg "public|protected|private|void " lib/src/main/java/io/ably/lib/realtime/Presence.java | grep -E "(enter|update|leave)" | head -20Repository: ably/ably-java
Length of output: 1516
🏁 Script executed:
# Check if there's any way a non-wrapped callback could be passed through the public API
rg -B 10 "enter\(|update\(|leave\(" lib/src/main/java/io/ably/lib/realtime/Presence.java | grep -E "(public|Callback|CompletionListener)" | head -30Repository: ably/ably-java
Length of output: 393
🏁 Script executed:
# Let's trace the complete flow from public methods to see if there's ANY way a non-wrapped callback could reach addPendingPresence
rg -B 15 "void updatePresence" lib/src/main/java/io/ably/lib/realtime/Presence.java | head -40Repository: ably/ably-java
Length of output: 800
🏁 Script executed:
# Check if ConnectionManager.send is ever called from outside Presence with presence messages
rg "connectionManager.send" lib/src/main/java/io/ably/lib/realtime/ --type java -B 2 | grep -v "Listeners.from"Repository: ably/ably-java
Length of output: 2039
🏁 Script executed:
# Verify all the paths that lead to transferQueuedPresenceMessages
rg "transferQueuedPresenceMessages\|transferToChannelQueue" lib/src/main/java/io/ably/lib/transport/ConnectionManager.java -B 5 -A 5Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
# Let's verify the specific lines 768 and 897 mentioned in the review
sed -n '760,780p' lib/src/main/java/io/ably/lib/realtime/Presence.javaRepository: ably/ably-java
Length of output: 1062
🏁 Script executed:
# Check line 897
sed -n '890,910p' lib/src/main/java/io/ably/lib/realtime/Presence.javaRepository: ably/ably-java
Length of output: 777
🏁 Script executed:
# Verify the complete data flow: public methods -> updatePresence -> send with Listeners.from
rg -A 20 "public void updatePresence" lib/src/main/java/io/ably/lib/realtime/Presence.java | head -30Repository: ably/ably-java
Length of output: 862
🏁 Script executed:
# Let's check if there are any other code paths that might call addPendingPresence
rg "pendingPresence" lib/src/main/java/io/ably/lib/realtime/Presence.java -B 2 -A 2Repository: ably/ably-java
Length of output: 1619
🏁 Script executed:
# Verify the complete updatePresence method to see all branches
sed -n '740,780p' lib/src/main/java/io/ably/lib/realtime/Presence.javaRepository: ably/ably-java
Length of output: 1780
🏁 Script executed:
# Confirm the visibility and full signature of addPendingPresence
sed -n '124,131p' lib/src/main/java/io/ably/lib/realtime/Presence.javaRepository: ably/ably-java
Length of output: 368
🏁 Script executed:
# Check if there are integration tests that might show the expected behavior
find . -type f -name "*.java" -path "*/test/*" | xargs rg "addPendingPresence\|QueuedMessage" 2>/dev/null | head -10Repository: ably/ably-java
Length of output: 40
🏁 Script executed:
# Verify that ConnectionManager only ever creates QueuedMessage with wrapped callbacks
rg "new QueuedMessage" lib/src/main/java/io/ably/lib/transport/ConnectionManager.java -B 8 -A 2Repository: ably/ably-java
Length of output: 2333
🏁 Script executed:
# Final verification: check if transferQueuedPresenceMessages is only called from one place
rg "transferQueuedPresenceMessages" lib/src/main/java/io/ably/lib/ --type javaRepository: ably/ably-java
Length of output: 360
Design fragility: addPendingPresence relies on unwrapping without defensive checks.
The method is package-private and currently safe—all call paths from transferQueuedPresenceMessages receive listeners wrapped via Listeners.fromCompletionListener(). However, the design is brittle: Listeners.unwrap() silently returns null for any non-wrapper Callback<PublishResult>, making it easy for future code paths to lose callbacks without notice.
Consider adding an assertion or log when unwrapping fails to catch accidental misuse:
final CompletionListener completionListener = Listeners.unwrap(listener);
if (listener != null && completionListener == null) {
Log.w(TAG, "Warning: addPendingPresence received unwrapped callback");
// or assert: assert completionListener != null;
}
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java
Show resolved
Hide resolved
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java
Show resolved
Hide resolved
ttypic
force-pushed
the
AIT-98/message-edits-deletes
branch
from
6dac2cf to
c4ffcd4
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1183/features
github-actions
bot
temporarily deployed
to
staging/pull/1183/javadoc
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @lib/src/main/java/io/ably/lib/types/PublishResult.java:
- Around line 71-83: writeMsgpackArray currently packs an array header with
results.length but skips null entries, corrupting MsgPack structure; change
PublishResult.writeMsgpackArray to iterate all indices and call packer.packNil()
for any null element (instead of skipping), and update readMsgpackArray to
accept and preserve nils by mapping a packed nil to a null PublishResult in the
returned PublishResult[] so the array length and element positions remain
consistent.
🧹 Nitpick comments (3)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (2)
370-378: PassingnulltoonErrormay cause issues.When the message serial doesn't match,
appendWaiter.onError(null)is called. IfAsyncWaiter.onErrordoesn't handle nullErrorInfo, this could cause NPE or unexpected behavior.Consider creating a meaningful error:
channel.subscribe(msg -> { if (msg.serial.equals(serial)) { appendWaiter.onSuccess(msg); } else { - appendWaiter.onError(null); + appendWaiter.onError(new ErrorInfo("Unexpected message serial: " + msg.serial, 400, 40000)); } });
414-436: Silent exception swallowing in helper methods may hide test failures.The
catch (Exception e) {}blocks silently discard exceptions, which could mask real issues and lead to false positives if the loop times out.Consider logging or tracking exceptions:
private Message waitForUpdatedMessageAppear(Channel channel, String serial) throws Exception { long timeout = System.currentTimeMillis() + 5_000; + Exception lastException = null; while (true) { try { Message message = channel.getMessage(serial); if ((message != null && message.action == MessageAction.MESSAGE_UPDATE) || System.currentTimeMillis() > timeout) return message; Thread.sleep(200); - } catch (Exception e) {} + } catch (Exception e) { + lastException = e; + if (System.currentTimeMillis() > timeout) { + throw new AssertionError("Timeout waiting for updated message", lastException); + } + } } }lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (1)
1357-1397: Consider null-checkinglistenerbefore callingtoPublishResultListener.The
Listeners.toPublishResultListener(listener)is called even whenlisteneris null. While looking at theUpdateResultToPublishAdapterinListeners.java, it does null-check the listener before invoking callbacks. However, passing null through creates an unnecessary adapter wrapper.Also, the error handling is inconsistent: validation errors (lines 1363-1371) throw
AblyException, while encoding errors (line 1389) calllistener.onError(). This matches the pattern inpublish(), but callers need to handle both paths.♻️ Optional: Short-circuit when listener is null
- connectionManager.send(msg, queueMessages, Listeners.toPublishResultListener(listener)); + connectionManager.send(msg, queueMessages, listener != null ? Listeners.toPublishResultListener(listener) : null);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/realtime/Presence.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/types/ProtocolMessage.javalib/src/main/java/io/ably/lib/types/PublishResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.javaliveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java
- liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt
- lib/src/main/java/io/ably/lib/util/Listeners.java
🧰 Additional context used
🧬 Code graph analysis (6)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (7)
lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1)
AblyRealtime(31-319)lib/src/main/java/io/ably/lib/types/ChannelOptions.java (1)
ChannelOptions(13-131)lib/src/main/java/io/ably/lib/types/Message.java (1)
Message(25-443)lib/src/main/java/io/ably/lib/types/MessageOperation.java (1)
MessageOperation(14-100)lib/src/main/java/io/ably/lib/types/Param.java (1)
Param(6-88)lib/src/main/java/io/ably/lib/types/UpdateDeleteResult.java (1)
UpdateDeleteResult(14-81)lib/src/main/java/io/ably/lib/util/Crypto.java (1)
Crypto(25-410)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java (2)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-131)liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (2)
onSuccess(21-23)onSuccess(37-39)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (1)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-131)
lib/src/main/java/io/ably/lib/realtime/Presence.java (2)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-131)lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (3)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
ProtocolMessage(34-375)lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-131)liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (2)
onSuccess(21-23)onSuccess(37-39)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (3)
lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)lib/src/main/java/io/ably/lib/util/Log.java (1)
Log(21-157)lib/src/main/java/io/ably/lib/types/AblyException.java (1)
AblyException(12-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check (29)
- GitHub Check: check-rest-okhttp
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check-liveobjects
- GitHub Check: check (21)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check (29)
🔇 Additional comments (35)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (3)
140-141: LGTM - Newresfield for publish results.The field follows the existing nullable pattern and integrates cleanly with other ProtocolMessage fields.
166-166: LGTM - MsgPack serialization forresfield.Serialization correctly guards with null check and uses
PublishResult.writeMsgpackArrayconsistent with other array fields.Also applies to: 215-218
290-292: LGTM - MsgPack deserialization forresfield.Deserialization follows the established pattern and correctly uses
PublishResult.readMsgpackArray. JSON deserialization for theresfield is handled automatically by Gson via reflection—neitherPublishResultnorProtocolMessageregister custom JSON deserializers inSerialisation.java, allowing Gson to deserialize theresarray field using standard reflection-based mechanisms.liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (2)
31-34: LGTM - Test correctly updated forCallback<PublishResult>API.The mock now properly reflects the updated
ConnectionManager.send()signature that acceptsCallback<PublishResult>instead ofCompletionListener.
54-57: LGTM - Error propagation test updated consistently.The error path test correctly uses the new callback type while maintaining the same error propagation behavior.
lib/src/main/java/io/ably/lib/realtime/Presence.java (4)
18-19: LGTM - Required imports for the new callback pattern.
768-768: LGTM - Adapter wrapping for ConnectionManager.send().Correctly wraps
CompletionListenertoCallback<PublishResult>for the updatedConnectionManager.send()signature.
897-897: LGTM - Consistent adapter usage insendQueuedMessages.The queued message sending correctly uses the same adapter pattern for consistency with the direct send path.
125-130: The null handling inListeners.unwrapis not a concern for this code path.
addPendingPresenceis a private method with only one caller:ChannelBase.java:249, which passes a listener fromqueuedMessage.listener. All listeners stored inQueuedMessageoriginate fromConnectionManager.send()(line 1746), which receives pre-wrapped callbacks from public entry points viaListeners.fromCompletionListener(). Therefore,queuedMessage.listeneris always aCompletionListenerWrapper, andListeners.unwrap()will successfully extract the underlying listener rather than returning null.lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java (2)
21-21: LGTM - Import updates for new callback types.Also applies to: 25-25
396-406: LGTM - Test correctly updated forCallback<PublishResult>API.The test properly reflects the updated
ConnectionManager.send()signature. The unusedresultparameter inonSuccessis appropriate since this test expects failure, not success.lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (9)
36-51: LGTM - Test class structure and lifecycle management.The test properly extends
ParameterizedTest, sets upAblyRealtimein@Before, and cleans up in@After.
53-80: LGTM - Basic message retrieval test.Good coverage for the publish-then-retrieve-by-serial flow.
82-122: LGTM - Update message test with result verification.Good test that verifies both the updated data and the version serial from the callback result.
124-160: LGTM - Encrypted message update test.Good coverage for the cipher key path using
ChannelOptions.withCipherKey.
162-194: LGTM - Soft delete test.Correctly verifies the delete operation results in
MESSAGE_DELETEaction.
196-243: LGTM - Version history test with metadata.Good coverage for version history including
MessageOperationmetadata verification.
245-300: LGTM - Error handling tests.Good coverage for invalid serial, null serial, and empty serial error cases with appropriate assertions on error messages.
302-355: LGTM - Complete workflow test.Comprehensive end-to-end test covering publish → update → verify → delete → verify → version history. Good assertions on version serials matching callback results.
395-412: LGTM - Helper polling methods.The polling approach with 200ms sleep and 5s timeout is reasonable for integration tests.
lib/src/main/java/io/ably/lib/types/PublishResult.java (4)
1-27: LGTM!The class structure, imports, and field declaration are well-organized. The nullable
serialsarray with appropriate Javadoc is clear.
29-69: LGTM!The JSON and MsgPack deserialization methods handle null fields correctly and return an empty array as a safe default when the
serialsfield is absent.
85-96: LGTM!The
readMsgpackArrayandgetBodyHandlermethods are implemented correctly, assuming the null handling fix is applied to align withwriteMsgpackArray.
115-130: There is no type parameter mismatch. TheHttpCore.BodyHandler<T>interface declaresT[] handleResponseBody(...), so implementingHttpCore.BodyHandler<String>correctly requires returningString[]. The implementation is correct.lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (5)
31-42: LGTM!The new imports for
Callback,PublishResult, and@Nullableare correctly added to support the typed callback pattern.
1408-1410: LGTM!Correctly passes the
resfield from the ProtocolMessage to propagate per-message publish results.
1728-1735: LGTM!The
QueuedMessageclass correctly updated to useCallback<PublishResult>instead ofCompletionListener, aligning with the new typed callback pattern.
1737-1751: LGTM!The
sendmethod signature correctly updated to acceptCallback<PublishResult>, maintaining consistent behavior for queueing and immediate send paths.
1831-1885: LGTM!The
ackmethod correctly handles per-messagePublishResultpropagation. The bounds check at line 1877 (results.length > i) properly guards against array index out of bounds when the results array is shorter than the acknowledged messages.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (6)
34-50: LGTM!The new imports for
MessageActionandListenersare correctly added to support the message edit/delete functionality and listener adaptation.
1128-1128: LGTM!Using
Listeners.fromCompletionListener(listener)correctly adapts theCompletionListenerto the newCallback<PublishResult>expected byConnectionManager.send().
1217-1257: LGTM!The
updateMessageoverloads properly delegate to the centralizedupdateDeleteImplmethod with appropriate defaults for optional parameters.
1266-1306: LGTM!The
deleteMessageoverloads follow the same consistent delegation pattern asupdateMessage.
1315-1355: LGTM!The
appendMessageoverloads complete the trio of message mutation operations with consistent API design.
1713-1716: LGTM!The
sendProtocolMessagemethod correctly uses the listener adapter for consistency with the new callback pattern.
| public static void writeMsgpackArray(PublishResult[] results, MessagePacker packer) { | ||
| try { | ||
| int count = results.length; | ||
| packer.packArrayHeader(count); | ||
| for (PublishResult result : results) { | ||
| if (result != null) { | ||
| result.writeMsgpack(packer); | ||
| } | ||
| } | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e.getMessage(), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Null elements in array will corrupt MsgPack serialization.
When results contains null elements, the array header declares results.length elements, but the loop only writes non-null elements. This causes a mismatch during deserialization since readMsgpackArray expects exactly count elements.
🐛 Proposed fix
public static void writeMsgpackArray(PublishResult[] results, MessagePacker packer) {
try {
int count = results.length;
packer.packArrayHeader(count);
for (PublishResult result : results) {
if (result != null) {
result.writeMsgpack(packer);
+ } else {
+ packer.packNil();
}
}
} catch (IOException e) {
throw new RuntimeException(e.getMessage(), e);
}
}Also update readMsgpackArray to handle null values:
public static PublishResult[] readMsgpackArray(MessageUnpacker unpacker) throws IOException {
int count = unpacker.unpackArrayHeader();
PublishResult[] results = new PublishResult[count];
for (int i = 0; i < count; i++) {
+ if (unpacker.getNextFormat().equals(MessageFormat.NIL)) {
+ unpacker.unpackNil();
+ results[i] = null;
+ } else {
results[i] = readMsgpack(unpacker);
+ }
}
return results;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static void writeMsgpackArray(PublishResult[] results, MessagePacker packer) { | |
| try { | |
| int count = results.length; | |
| packer.packArrayHeader(count); | |
| for (PublishResult result : results) { | |
| if (result != null) { | |
| result.writeMsgpack(packer); | |
| } | |
| } | |
| } catch (IOException e) { | |
| throw new RuntimeException(e.getMessage(), e); | |
| } | |
| } | |
| public static void writeMsgpackArray(PublishResult[] results, MessagePacker packer) { | |
| try { | |
| int count = results.length; | |
| packer.packArrayHeader(count); | |
| for (PublishResult result : results) { | |
| if (result != null) { | |
| result.writeMsgpack(packer); | |
| } else { | |
| packer.packNil(); | |
| } | |
| } | |
| } catch (IOException e) { | |
| throw new RuntimeException(e.getMessage(), e); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @lib/src/main/java/io/ably/lib/types/PublishResult.java around lines 71 - 83,
writeMsgpackArray currently packs an array header with results.length but skips
null entries, corrupting MsgPack structure; change
PublishResult.writeMsgpackArray to iterate all indices and call packer.packNil()
for any null element (instead of skipping), and update readMsgpackArray to
accept and preserve nils by mapping a packed nil to a null PublishResult in the
returned PublishResult[] so the array length and element positions remain
consistent.
ttypic
force-pushed
the
AIT-98/message-edits-deletes
branch
from
c4ffcd4 to
99a2c94
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
@lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java:
- Around line 426-436: The helper waitForDeletedMessageAppear currently swallows
all exceptions in its loop via an empty catch, which can hide real errors from
channel.getMessage; change the catch in waitForDeletedMessageAppear to either
rethrow the exception (or throw a wrapped RuntimeException) after any necessary
cleanup or log the exception and then rethrow so failures surface to the test
runner; keep the existing timeout and sleep behavior and still return when
message.action == MessageAction.MESSAGE_DELETE, but do not leave the catch block
empty—use the exception variable (e) and propagate it.
- Around line 414-424: The waitForUpdatedMessageAppear method currently swallows
all exceptions in an empty catch block; change it to either catch only expected
exceptions (e.g., InterruptedException) or rethrow/unwrap unexpected ones and
log details so test failures aren't hidden: update the catch in
waitForUpdatedMessageAppear to log the exception (including stack trace) using
the test logger or throw a RuntimeException for unexpected errors, and only
suppress transient, documented exceptions around channel.getMessage/Thread.sleep
while still honoring the timeout and MessageAction.MESSAGE_UPDATE check.
- Around line 370-378: The subscriber's error path calls
appendWaiter.onError(null), which can cause NPEs; modify the lambda in
channel.subscribe to construct and pass a non-null ErrorInfo (or an appropriate
error object) to appendWaiter.onError when msg.serial does not match serial,
e.g., create an ErrorInfo with a clear message mentioning Message.serial
mismatch and include relevant context (serial and msg.serial) so
Helpers.AsyncWaiter#onError receives a valid error object.
🧹 Nitpick comments (2)
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java (2)
38-39: Consider reducing the test timeout.A 5-minute (300s) timeout is unusually long for integration tests. If tests are expected to complete within seconds, a timeout of 60-120 seconds would catch stuck tests faster while still providing adequate margin.
395-403: Operator precedence could cause unexpected behavior.The condition
history.items().length > 0 && predicate.test(history.items()) || System.currentTimeMillis() > timeoutmay not behave as intended due to operator precedence.&&binds tighter than||, so this reads as(length > 0 && predicate) || timeout. If you intend to return early on timeout regardless of predicate, the current logic is correct but parentheses would improve clarity.Proposed fix for clarity
- if (history.items().length > 0 && predicate.test(history.items()) || System.currentTimeMillis() > timeout) + if ((history.items().length > 0 && predicate.test(history.items())) || System.currentTimeMillis() > timeout)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
lib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/realtime/Presence.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/types/ProtocolMessage.javalib/src/main/java/io/ably/lib/types/PublishResult.javalib/src/main/java/io/ably/lib/util/Listeners.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.javalib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.javaliveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.ktliveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/main/java/io/ably/lib/util/Listeners.java
- lib/src/main/java/io/ably/lib/types/PublishResult.java
🧰 Additional context used
🧬 Code graph analysis (3)
lib/src/main/java/io/ably/lib/realtime/Presence.java (2)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-133)lib/src/main/java/io/ably/lib/util/Listeners.java (1)
Listeners(9-72)
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (1)
lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-133)
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (2)
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (1)
ProtocolMessage(34-375)lib/src/main/java/io/ably/lib/types/PublishResult.java (1)
PublishResult(15-133)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: build
- GitHub Check: check (21)
- GitHub Check: check (24)
- GitHub Check: check (29)
- GitHub Check: check (19)
- GitHub Check: check (29)
- GitHub Check: check
- GitHub Check: check-rest-httpurlconnection
- GitHub Check: check-realtime-httpurlconnection
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-rest-okhttp
- GitHub Check: check-liveobjects
🔇 Additional comments (21)
liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt (1)
18-32: LGTM!The migration from
CompletionListenertoCallback<PublishResult>is correctly implemented. The coroutine now properly resumes with thePublishResulton success, maintaining the same error handling pattern.liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt (2)
31-34: LGTM!The test mock correctly uses
Callback<PublishResult>and invokesonSuccesswithPublishResult(null), which is appropriate for this test scenario where the actual serials aren't being verified.
54-57: LGTM!The error path test is correctly updated to use the new
Callback<PublishResult>interface while maintaining the same error propagation verification.lib/src/main/java/io/ably/lib/types/ProtocolMessage.java (3)
140-141: LGTM!The new
resfield for per-message publish results follows the established pattern for nullable array fields in this class.
215-218: LGTM!The msgpack serialization correctly follows the pattern used for other array fields, with proper null-checking before serialization.
290-292: LGTM!The msgpack deserialization correctly reads the
resfield usingPublishResult.readMsgpackArray, consistent with other array field deserialization in this class.lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java (1)
33-34: LGTM!The new
RealtimeChannelMessageEditTestis appropriately added to the test suite.lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java (1)
396-406: LGTM!The test callback is correctly migrated to
Callback<PublishResult>. The unusedresultparameter inonSuccessis appropriate here since the test only verifies that success is NOT called (it would fail the test if invoked).lib/src/main/java/io/ably/lib/realtime/Presence.java (3)
125-130: LGTM - Parameter type migration with unwrap.The migration from
CompletionListenertoCallback<PublishResult>is handled correctly.Listeners.unwrap()extracts the underlyingCompletionListenerfrom the wrapper, andQueuedPresencecontinues to storeCompletionListenerwhich is appropriate since presence messages don't needPublishResultserials.
768-768: LGTM - Consistent callback wrapping.The
Listeners.fromCompletionListener(listener)correctly wraps theCompletionListenerfor the updatedconnectionManager.send()API.
897-897: LGTM - Consistent callback wrapping for queued messages.Same pattern applied correctly for
sendQueuedMessages().lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (4)
1728-1734: LGTM - QueuedMessage updated for new callback type.The
QueuedMessageclass correctly updated to storeCallback<PublishResult>instead ofCompletionListener, aligning with the new per-message publish result handling.
1737-1737: LGTM - Send signature updated.The public
send()method signature correctly updated to acceptCallback<PublishResult>.
1831-1831: LGTM - Ack method signature extended.The
ack()method now accepts an optionalPublishResult[]array to deliver per-message results, with proper@Nullableannotation.
1872-1884: LGTM - Per-message PublishResult delivery.The indexed loop correctly matches each acknowledged message with its corresponding
PublishResultfrom the results array. The bounds checkresults.length > isafely handles cases where the results array might be shorter than expected.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java (6)
1128-1128: LGTM - Publish callback wrapping.The
publish()method correctly usesListeners.fromCompletionListener(listener)to adapt the callback for the updatedconnectionManager.send()API.
1217-1257: LGTM - updateMessage overloads restructured.The method overloads correctly delegate to the most complete signature which calls
updateDeleteImpl. The naming and delegation chain are clear.
1266-1306: LGTM - deleteMessage overloads restructured.Same clean delegation pattern as updateMessage.
1315-1355: LGTM - appendMessage overloads restructured.Same clean delegation pattern as updateMessage and deleteMessage.
1357-1397: LGTM - Centralized updateDeleteImpl handles all update/delete/append operations.The implementation correctly:
- Validates the message serial (lines 1363-1365)
- Checks connection state and queue policy (lines 1366-1371)
- Constructs the updated message with the appropriate action and version metadata (lines 1374-1382)
- Encodes the message and handles errors appropriately (lines 1384-1392)
- Uses
Listeners.toPublishResultListener(listener)to adapt theCallback<UpdateDeleteResult>toCallback<PublishResult>(line 1396)This centralization reduces code duplication across the three operations.
1715-1715: LGTM - sendProtocolMessage callback wrapping.Consistent use of
Listeners.fromCompletionListener(listener)for the internal protocol message sending.
| Helpers.AsyncWaiter<Message> appendWaiter = new Helpers.AsyncWaiter<>(); | ||
|
|
||
| channel.subscribe(msg -> { | ||
| if (msg.serial.equals(serial)) { | ||
| appendWaiter.onSuccess(msg); | ||
| } else { | ||
| appendWaiter.onError(null); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Passing null to onError may cause issues.
When the serial doesn't match, appendWaiter.onError(null) is called with a null ErrorInfo. This could cause NPEs if the waiter or downstream code expects a non-null error info. Consider providing a descriptive ErrorInfo instead.
Proposed fix
channel.subscribe(msg -> {
if (msg.serial.equals(serial)) {
appendWaiter.onSuccess(msg);
} else {
- appendWaiter.onError(null);
+ appendWaiter.onError(new ErrorInfo("Unexpected message serial: " + msg.serial, 500));
}
});🤖 Prompt for AI Agents
In
@lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java
around lines 370 - 378, The subscriber's error path calls
appendWaiter.onError(null), which can cause NPEs; modify the lambda in
channel.subscribe to construct and pass a non-null ErrorInfo (or an appropriate
error object) to appendWaiter.onError when msg.serial does not match serial,
e.g., create an ErrorInfo with a clear message mentioning Message.serial
mismatch and include relevant context (serial and msg.serial) so
Helpers.AsyncWaiter#onError receives a valid error object.
| private Message waitForUpdatedMessageAppear(Channel channel, String serial) throws Exception { | ||
| long timeout = System.currentTimeMillis() + 5_000; | ||
| while (true) { | ||
| try { | ||
| Message message = channel.getMessage(serial); | ||
| if ((message != null && message.action == MessageAction.MESSAGE_UPDATE) || System.currentTimeMillis() > timeout) | ||
| return message; | ||
| Thread.sleep(200); | ||
| } catch (Exception e) {} | ||
| } | ||
| } |
There was a problem hiding this comment.
Swallowing exceptions may hide real test failures.
The empty catch (Exception e) {} block silently ignores all exceptions, which could mask real issues like network errors or unexpected API behavior. Consider logging the exception or only catching expected exceptions.
Proposed fix
try {
Message message = channel.getMessage(serial);
if ((message != null && message.action == MessageAction.MESSAGE_UPDATE) || System.currentTimeMillis() > timeout)
return message;
Thread.sleep(200);
- } catch (Exception e) {}
+ } catch (AblyException e) {
+ // Expected when message not yet available, continue polling
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private Message waitForUpdatedMessageAppear(Channel channel, String serial) throws Exception { | |
| long timeout = System.currentTimeMillis() + 5_000; | |
| while (true) { | |
| try { | |
| Message message = channel.getMessage(serial); | |
| if ((message != null && message.action == MessageAction.MESSAGE_UPDATE) || System.currentTimeMillis() > timeout) | |
| return message; | |
| Thread.sleep(200); | |
| } catch (Exception e) {} | |
| } | |
| } | |
| private Message waitForUpdatedMessageAppear(Channel channel, String serial) throws Exception { | |
| long timeout = System.currentTimeMillis() + 5_000; | |
| while (true) { | |
| try { | |
| Message message = channel.getMessage(serial); | |
| if ((message != null && message.action == MessageAction.MESSAGE_UPDATE) || System.currentTimeMillis() > timeout) | |
| return message; | |
| Thread.sleep(200); | |
| } catch (AblyException e) { | |
| // Expected when message not yet available, continue polling | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java
around lines 414 - 424, The waitForUpdatedMessageAppear method currently
swallows all exceptions in an empty catch block; change it to either catch only
expected exceptions (e.g., InterruptedException) or rethrow/unwrap unexpected
ones and log details so test failures aren't hidden: update the catch in
waitForUpdatedMessageAppear to log the exception (including stack trace) using
the test logger or throw a RuntimeException for unexpected errors, and only
suppress transient, documented exceptions around channel.getMessage/Thread.sleep
while still honoring the timeout and MessageAction.MESSAGE_UPDATE check.
| private Message waitForDeletedMessageAppear(Channel channel, String serial) throws Exception { | ||
| long timeout = System.currentTimeMillis() + 5_000; | ||
| while (true) { | ||
| try { | ||
| Message message = channel.getMessage(serial); | ||
| if ((message != null && message.action == MessageAction.MESSAGE_DELETE) || System.currentTimeMillis() > timeout) | ||
| return message; | ||
| Thread.sleep(200); | ||
| } catch (Exception e) {} | ||
| } | ||
| } |
There was a problem hiding this comment.
Same exception swallowing issue in waitForDeletedMessageAppear.
This helper has the same empty catch block pattern that could hide real errors.
🤖 Prompt for AI Agents
In
@lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java
around lines 426 - 436, The helper waitForDeletedMessageAppear currently
swallows all exceptions in its loop via an empty catch, which can hide real
errors from channel.getMessage; change the catch in waitForDeletedMessageAppear
to either rethrow the exception (or throw a wrapped RuntimeException) after any
necessary cleanup or log the exception and then rethrow so failures surface to
the test runner; keep the existing timeout and sleep behavior and still return
when message.action == MessageAction.MESSAGE_DELETE, but do not leave the catch
block empty—use the exception variable (e) and propagate it.
There was a problem hiding this comment.
Pull request overview
This pull request implements realtime message edit and delete functionality by replacing REST-based operations with realtime protocol messages for the realtime client. The changes enable publish operations to return per-message results including serials, and introduce a new flow for message update, delete, and append operations through the realtime connection.
Changes:
- Updated message publish and presence operations to return
PublishResultwith per-message serials via callbacks - Replaced synchronous REST-based message edit/delete operations with asynchronous realtime protocol message operations
- Added comprehensive test coverage for message edit, delete, append, and versioning workflows
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
lib/src/main/java/io/ably/lib/types/PublishResult.java |
New type to hold publish results with message serials array |
lib/src/main/java/io/ably/lib/types/ProtocolMessage.java |
Added res field for publish results in protocol messages |
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java |
Updated send methods and ack handling to work with PublishResult callbacks |
lib/src/main/java/io/ably/lib/util/Listeners.java |
Added adapter to convert between PublishResult and UpdateDeleteResult listeners |
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java |
Refactored update/delete/append methods to use realtime protocol instead of REST |
lib/src/main/java/io/ably/lib/realtime/Presence.java |
Updated to use new callback signatures |
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelMessageEditTest.java |
Added comprehensive tests for message edit functionality |
lib/src/test/java/io/ably/lib/test/realtime/RealtimeConnectFailTest.java |
Updated test mocks for new callback signature |
lib/src/test/java/io/ably/lib/test/realtime/RealtimeSuite.java |
Added new test class to suite |
liveobjects/src/main/kotlin/io/ably/lib/objects/Helpers.kt |
Updated to handle PublishResult returns |
liveobjects/src/test/kotlin/io/ably/lib/objects/unit/HelpersTest.kt |
Updated test mocks for new callback signature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assertEquals("Expected MESSAGE_APPEND action through realtime",MessageAction.MESSAGE_APPEND, appendWaiter.result.action); | ||
| assertEquals("Expected delta through realtime","Append data", appendWaiter.result.data); |
There was a problem hiding this comment.
There's a missing space after the comma in this assertion message. The string should be "Expected MESSAGE_APPEND action through realtime" with a space between the comma and "MessageAction".
| assertEquals("Expected MESSAGE_APPEND action through realtime",MessageAction.MESSAGE_APPEND, appendWaiter.result.action); | |
| assertEquals("Expected delta through realtime","Append data", appendWaiter.result.data); | |
| assertEquals("Expected MESSAGE_APPEND action through realtime", MessageAction.MESSAGE_APPEND, appendWaiter.result.action); | |
| assertEquals("Expected delta through realtime", "Append data", appendWaiter.result.data); |
| assertEquals("Expected updated data in history", "Initial dataAppend data", retrieved.data); | ||
| assertEquals("Expected MESSAGE_UPDATE action in history", MessageAction.MESSAGE_UPDATE, retrieved.action); | ||
| assertEquals("Expected MESSAGE_APPEND action through realtime",MessageAction.MESSAGE_APPEND, appendWaiter.result.action); | ||
| assertEquals("Expected delta through realtime","Append data", appendWaiter.result.data); |
There was a problem hiding this comment.
There's a missing space after the comma in this assertion message. The string should be "Expected delta through realtime" with a space between the comma and the string literal.
| assertEquals("Expected delta through realtime","Append data", appendWaiter.result.data); | |
| assertEquals("Expected delta through realtime", "Append data", appendWaiter.result.data); |
|
|
||
| ProtocolMessage msg = new ProtocolMessage(Action.message, this.name); | ||
| msg.messages = new Message[] { updatedMessage }; | ||
| connectionManager.send(msg, queueMessages, Listeners.toPublishResultListener(listener)); |
There was a problem hiding this comment.
The error handling in this method is inconsistent. When an exception occurs during checkClientId or encode (lines 1385-1392), the listener's onError is called and the method returns early. However, when the send operation at line 1396 fails, any AblyException thrown by connectionManager.send will propagate up to the caller, which means the listener won't be notified of the error. This creates inconsistent behavior where some errors notify the listener while others throw exceptions to the caller. The send call should be wrapped in a try-catch to maintain consistent error handling.
| connectionManager.send(msg, queueMessages, Listeners.toPublishResultListener(listener)); | |
| try { | |
| connectionManager.send(msg, queueMessages, Listeners.toPublishResultListener(listener)); | |
| } catch (AblyException e) { | |
| if (listener != null) { | |
| listener.onError(e.errorInfo); | |
| } | |
| } |
| import static org.junit.Assert.assertTrue; | ||
|
|
||
| /** | ||
| * Tests for REST channel message edit and delete operations |
There was a problem hiding this comment.
The comment states "Tests for REST channel message edit and delete operations" but this test file is actually testing realtime channel operations, not REST operations. The comment should be updated to reflect that these are tests for realtime channel message edit and delete operations.
| * Tests for REST channel message edit and delete operations | |
| * Tests for realtime channel message edit and delete operations |
| if (history.items().length > 0 && predicate.test(history.items()) || System.currentTimeMillis() > timeout) | ||
| return history; |
There was a problem hiding this comment.
The condition in this if statement has a logical operator precedence issue. The condition history.items().length > 0 && predicate.test(history.items()) || System.currentTimeMillis() > timeout evaluates as (history.items().length > 0 && predicate.test(history.items())) || System.currentTimeMillis() > timeout, which means the method could return early with empty items if the timeout has passed. This should likely be (history.items().length > 0 && predicate.test(history.items())) || System.currentTimeMillis() > timeout with the timeout check properly grouped, or the logic should be restructured for clarity. Consider adding parentheses: (history.items().length > 0 && predicate.test(history.items())) || System.currentTimeMillis() > timeout or restructuring to check timeout separately.
| if ((message != null && message.action == MessageAction.MESSAGE_UPDATE) || System.currentTimeMillis() > timeout) | ||
| return message; | ||
| Thread.sleep(200); | ||
| } catch (Exception e) {} |
There was a problem hiding this comment.
Empty catch blocks that silently swallow exceptions make debugging difficult. When getMessage fails with an exception, this catch block hides the error, which could mask real issues during test execution. Consider logging the exception or at least adding a comment explaining why the exception is intentionally ignored.
| if ((message != null && message.action == MessageAction.MESSAGE_DELETE) || System.currentTimeMillis() > timeout) | ||
| return message; | ||
| Thread.sleep(200); | ||
| } catch (Exception e) {} |
There was a problem hiding this comment.
Empty catch blocks that silently swallow exceptions make debugging difficult. When getMessage fails with an exception, this catch block hides the error, which could mask real issues during test execution. Consider logging the exception or at least adding a comment explaining why the exception is intentionally ignored.
Replaced rest message edits and deletes with realtime for realtime client. Spec PR https://github.com/ably/specification/pull/406/files
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.