[AIT-125, AIT-126] Realtime edits and deletes, new edits REST API, appends API#2178
[AIT-125, AIT-126] Realtime edits and deletes, new edits REST API, appends API#2178lawrence-forooghian merged 11 commits intomainfrom
Conversation
WalkthroughAdds publish/update/delete/append result types and a message-send status wrapper; changes per-message ack callbacks to ARTMessageSendCallback and mutation callbacks to ARTEditResultCallback/ARTUpdateDeleteResult; introduces appendMessage and a unified protocol/REST mutation flow; removes message-bundling/merge logic; updates encoders, headers, tests, and project wiring. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Channel
participant Realtime as Realtime/REST
participant Server
Client->>Channel: update/delete/append(message, operation, params, callback)
Channel->>Channel: editMessage (set action, attach version if any)
Channel->>Realtime: internalSendEditRequestForMessage(message, params, wrapperSDKAgents)
Realtime->>Server: send PATCH request or publish protocol message
Server-->>Realtime: ARTPublishResult (res/serials)
Realtime->>Channel: decode ARTPublishResult -> ARTUpdateDeleteResult
Channel-->>Client: callback(ARTUpdateDeleteResult, error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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/2178/features
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
github-actions
Bot
temporarily deployed
to
staging/pull/2178/jazzydoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @Source/ARTChannel.m:
- Around line 159-209: The
editMessage:action:operation:params:wrapperSDKAgents:callback: method declares
ARTErrorInfo *error but passes &error to encodeMessageIfNeeded:error: which
expects NSError **; change the local variable to NSError *nsError = nil, pass
&nsError to encodeMessageIfNeeded:, and if nsError is non-nil convert/wrap it
into an ARTErrorInfo (e.g. via the same helper used elsewhere) before invoking
the callback; update uses of error in this method to the wrapped ARTErrorInfo so
the callback receives the correct type and no pointer-type mismatch remains.
In @Source/ARTMessage.m:
- Around line 74-75: The switch in ARTRestChannel.m that sets logOperation based
on message.action is missing break statements causing fall-through; update the
switch handling of ARTMessageActionUpdate, ARTMessageActionAppend, and
ARTMessageActionDelete (the switch on message.action that assigns logOperation)
to include a break; after each case block (after setting logOperation) so each
case returns the intended value and does not fall through to the default.
In @Source/ARTRealtime.m:
- Around line 1390-1398: In failQueuedMessages (and mirror the same change in
failPendingMessages) guard optional callbacks before invoking them: check if
message.sentCallback is non-nil before calling it with status.errorInfo and
check if message.ackCallback is non-nil before calling it with the created
ARTMessageSendStatus instance; leave creation of ARTMessageSendStatus unchanged
but only pass it to ackCallback if that block exists. This prevents crashes when
callbacks are nil while preserving current behavior for messages that do provide
callbacks.
In @Source/ARTRestChannel.m:
- Around line 414-519: The switch in internalSendMutationRequestForMessage that
sets logOperation falls through because it lacks breaks; update the switch on
message.action (in the internalSendMutationRequestForMessage method) to assign
the correct string for each case (ARTMessageActionUpdate,
ARTMessageActionAppend, ARTMessageActionDelete) and add break statements (or use
return/explicit case values) so logOperation isn't overwritten by subsequent
cases; optionally also use the same per-action name when constructing the "lacks
a serial" error message to distinguish append vs update vs delete.
🧹 Nitpick comments (9)
Source/ARTPublishResult.m (1)
6-11: Consider copying the serials array for defensive immutability.The
serialsarray is assigned directly without copying. If the caller mutates the passed-in mutable array after initialization, this object's state would change unexpectedly. SinceARTPublishResultappears to be a value-like result object with areadonlyproperty, copying ensures immutability.♻️ Proposed fix
- (instancetype)initWithSerials:(NSArray<ARTPublishResultSerial *> *)serials { if (self = [super init]) { - _serials = serials; + _serials = [serials copy]; } return self; }Source/ARTMessageVersion.m (1)
21-29: Consider calling[self init]for consistency with base initializer pattern.The new initializer calls
[super init]directly, leaving_serialand_timestampimplicitly nil (ARC behavior). The baseinitmethod explicitly initializes all ivars to nil. For consistency and clarity, consider chaining to[self init]:♻️ Proposed fix
- (instancetype)initWithOperation:(ARTMessageOperation *)operation { - self = [super init]; + self = [self init]; if (self) { _clientId = operation.clientId; _descriptionText = operation.descriptionText; _metadata = operation.metadata; } return self; }Source/PrivateHeaders/Ably/ARTMessageSendStatus.h (1)
8-22: Consider broadening the documentation scope.The documentation on lines 9 and 19 specifically mentions "ProtocolMessage send operation," but according to the PR context, this wrapper is used across REST and Realtime mutation flows. Consider whether the documentation should be more general to reflect all usage contexts, or if it should remain protocol-message-specific.
Source/include/Ably/ARTChannelProtocol.h (2)
86-93: Consider simplifying the documentation.The documentation on line 87 is quite long and includes a nested parenthetical explanation. Consider breaking it into multiple sentences for improved readability, such as:
Marks a message as deleted by publishing an update with an action of
ARTMessageActionDelete. This does not remove the message from the server, and the full message history remains accessible.Uses shallow mixin semantics: non-
nilproperties in the provided message replace the corresponding properties in the existing message, whilenilfields are left unchanged. To explicitly set a field to empty in the delete action, set the property to an empty object rather thannil.
99-110: Document append semantics for different data types in the header comment.The documentation does not clarify how the append operation handles different data types. Consider explaining:
- How
datais appended for strings (concatenation?)- How
datais appended for arrays (concatenation/merge?)- How
datais appended for objects (merge/replace?)- What happens when the appended data type is incompatible with the existing data type
If this behavior is specified in the Ably specification (indicated by the RTL15b1 and RTL32b1 references in the implementation), consider adding a brief reference or link to the specification for developers seeking detailed semantics.
Ably.xcodeproj/project.pbxproj (1)
319-342: Confirm intended public vs private visibility for ARTPublishResult and ARTMessageSendStatus*
ARTUpdateDeleteResult.his configured as a public header (include/Ably/ARTUpdateDeleteResult.h, ATTRIBUTES = (Public)), while:
ARTPublishResult.handARTPublishResultSerial.hare underPrivateHeaders/Ablyand marked Private.ARTMessageSendStatus.his also underPrivateHeaders/Ablyand marked Private.Given the PR introduces
ARTPublishResultandARTUpdateDeleteResultas message mutation result types and addsARTMessageSendStatusas a new send-status wrapper, this asymmetric visibility may be unintentional if these types are meant to be part of the public API (e.g., used in public callbacks).Please double-check:
- Whether
ARTPublishResultis intentionally internal-only whereasARTUpdateDeleteResultis public, or ifARTPublishResult.hshould also live underinclude/Ablyand be marked Public.- Whether
ARTMessageSendStatusis only used internally, or should be exposed as a public header alongside any public callback types that reference it.Also applies to: 1244-1251, 1973-1980, 2163-2377, 2379-2593, 2595-2809
Source/ARTChannel.m (1)
206-209: Consider aligningappendMessagenullability with update/delete and the header
appendMessage:operation:params:wrapperSDKAgents:callback:in this implementation omits thenullableannotations thatupdateMessage/deleteMessageuse. The body already safely handlesoperation == nilandparams == nil, and Swift tests passnilhere.To avoid confusion between header and implementation and to keep API surface consistent, consider updating the signature to:
Suggested signature alignment
-- (void)appendMessage:(ARTMessage *)message operation:(ARTMessageOperation *)operation params:(NSDictionary<NSString *,ARTStringifiable *> *)params wrapperSDKAgents:(NSStringDictionary *)wrapperSDKAgents callback:(ARTUpdateDeleteResultCallback)callback { +- (void)appendMessage:(ARTMessage *)message + operation:(nullable ARTMessageOperation *)operation + params:(nullable NSDictionary<NSString *, ARTStringifiable *> *)params + wrapperSDKAgents:(nullable NSStringDictionary *)wrapperSDKAgents + callback:(nullable ARTUpdateDeleteResultCallback)callback {Also applies to: 254-259
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (1)
376-494: Append tests look correct; fix minor action-name comment typoThe new
_test__rest_and_realtime__appendMessagecoverage looks good:
- REST path correctly asserts
PATCHto/channels/{channelName}/messages/{serial}with query params and a MsgPack body containingserial,name, appendeddata,action == 5, and aversionpayload derived fromARTMessageOperation.- It also validates that
appendResult.versionSerialmatches the updated message’sversion.serial, and thatgetMessageWithSerialreturns the fully appended data ("Hello world!",.action == .update).Tiny nit: the inline comment on the action constant says “MESSAGE_DELETE” for value 5:
// RSL15b1: action field (5 = MESSAGE_DELETE) XCTAssertEqual(bodyDict["action"] as? Int, 5)This should say
MESSAGE_APPENDto match the enum:Suggested comment fix
- // RSL15b1: action field (5 = MESSAGE_DELETE) + // RSL15b1: action field (5 = MESSAGE_APPEND)Source/ARTRealtimeChannel.m (1)
477-515: Align realtime mutation callbacks with REST semantics on error
internalSendMutationRequestForMessage:…currently always constructs anARTUpdateDeleteResultfromstatus.publishResult.serials.firstObject.valueand passes it to the callback, even whenstatus.status.errorInfois non-nil (i.e. the mutation failed). By contrast, the REST implementation calls the callback withresult == nilwhenever there is an HTTP or decode error.To keep the API behavior consistent between REST and Realtime, consider:
- Returning
(nil, error)whenstatus.status.errorInfois non-nil.- Only constructing
ARTUpdateDeleteResulton success.For example:
Suggested behavioral tweak
- [self publishProtocolMessage:msg callback:^void(ARTMessageSendStatus *status) { - if (callback) { - // RTL32d: Extract the versionSerial from the ACK and create an UpdateDeleteResult - NSString *versionSerial = status.publishResult.serials.firstObject.value; - ARTUpdateDeleteResult *updateDeleteResult = [[ARTUpdateDeleteResult alloc] initWithVersionSerial:versionSerial]; - callback(updateDeleteResult, status.status.errorInfo); - } - }]; + [self publishProtocolMessage:msg callback:^void(ARTMessageSendStatus *status) { + if (!callback) { + return; + } + + ARTErrorInfo *errorInfo = status.status.errorInfo; + if (errorInfo) { + callback(nil, errorInfo); + return; + } + + // RTL32d: Extract the versionSerial from the ACK and create an UpdateDeleteResult + NSString *versionSerial = status.publishResult.serials.firstObject.value; + ARTUpdateDeleteResult *updateDeleteResult = [[ARTUpdateDeleteResult alloc] initWithVersionSerial:versionSerial]; + callback(updateDeleteResult, nil); + }];This also makes it explicit that
versionSerialmay benilif the server does not returnres/serials for some reason, while keeping the success/error contract clear.
📜 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 (42)
Ably.xcodeproj/project.pbxprojSource/ARTChannel.mSource/ARTDefault.mSource/ARTJsonLikeEncoder.mSource/ARTMessage.mSource/ARTMessageSendStatus.mSource/ARTMessageVersion.mSource/ARTPendingMessage.mSource/ARTProtocolMessage.mSource/ARTPublishResult.mSource/ARTPublishResultSerial.mSource/ARTQueuedMessage.mSource/ARTRealtime.mSource/ARTRealtimeAnnotations.mSource/ARTRealtimeChannel.mSource/ARTRealtimePresence.mSource/ARTRestChannel.mSource/ARTUpdateDeleteResult.mSource/ARTWrapperSDKProxyRealtimeChannel.mSource/Ably.modulemapSource/PrivateHeaders/Ably/ARTChannel+Private.hSource/PrivateHeaders/Ably/ARTChannel.hSource/PrivateHeaders/Ably/ARTEncoder.hSource/PrivateHeaders/Ably/ARTJsonLikeEncoder.hSource/PrivateHeaders/Ably/ARTMessageSendStatus.hSource/PrivateHeaders/Ably/ARTMessageVersion+Private.hSource/PrivateHeaders/Ably/ARTPendingMessage.hSource/PrivateHeaders/Ably/ARTProtocolMessage+Private.hSource/PrivateHeaders/Ably/ARTProtocolMessage.hSource/PrivateHeaders/Ably/ARTPublishResult.hSource/PrivateHeaders/Ably/ARTPublishResultSerial.hSource/PrivateHeaders/Ably/ARTQueuedMessage.hSource/PrivateHeaders/Ably/ARTRealtime+Private.hSource/PrivateHeaders/Ably/ARTRealtimeChannel+Private.hSource/include/Ably/ARTChannelProtocol.hSource/include/Ably/ARTMessage.hSource/include/Ably/ARTTypes.hSource/include/Ably/ARTUpdateDeleteResult.hSource/include/Ably/AblyPublic.hSource/include/module.modulemapTest/AblyTests/Tests/MessageUpdatesDeletesTests.swiftTest/AblyTests/Tests/RealtimeClientChannelTests.swift
💤 Files with no reviewable changes (2)
- Test/AblyTests/Tests/RealtimeClientChannelTests.swift
- Source/PrivateHeaders/Ably/ARTProtocolMessage+Private.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTUpdateDeleteResult.mSource/ARTMessage.mSource/ARTPublishResult.mSource/ARTPublishResultSerial.mSource/ARTDefault.mSource/ARTProtocolMessage.mSource/ARTRealtimeAnnotations.mSource/ARTMessageSendStatus.mSource/ARTRealtime.mSource/ARTWrapperSDKProxyRealtimeChannel.mSource/ARTMessageVersion.mSource/ARTQueuedMessage.mSource/ARTRealtimePresence.mSource/ARTPendingMessage.mSource/ARTRestChannel.mSource/ARTJsonLikeEncoder.mSource/ARTChannel.mSource/ARTRealtimeChannel.m
🧬 Code graph analysis (1)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (2)
extractBodyAsMsgPack(698-716)splitDone(248-258)
⏰ 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). (6)
- GitHub Check: build
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (53)
Source/PrivateHeaders/Ably/ARTProtocolMessage.h (2)
17-17: LGTM! Forward declaration follows the established pattern.The forward declaration of
ARTPublishResultis correctly placed and follows the same pattern as other forward declarations in this file.
75-75: LGTM! Property declaration is correct.The
resproperty is correctly declared as nullable and nonatomic, consistent with other properties in this interface. The short name matches the protocol specification.Source/include/Ably/ARTMessage.h (1)
38-44: LGTM! Well-documented enum addition.The
ARTMessageActionAppendenum constant is clearly documented with comprehensive semantics explaining how the serial, data, and other fields are handled during append operations.Source/ARTUpdateDeleteResult.m (2)
5-10: LGTM! Standard initializer implementation.The designated initializer correctly follows the Objective-C pattern: calls
[super init], stores the parameter in the instance variable, and returns self.
12-14: LGTM! Useful description implementation.The description method provides helpful debugging information, correctly handles nil values, and follows a clear format showing the class name, pointer, and versionSerial value.
Source/ARTRealtimePresence.m (4)
564-567: LGTM! Method signature updated correctly.The
addPendingPresence:callback:method correctly usesARTMessageSendCallbacktype, consistent with the protocol v5 callback signature changes.
607-609: LGTM! Callback handling updated correctly.The callback correctly accesses
status.status.errorInfoto extract the error information from the wrappedARTMessageSendStatus→ARTStatusstructure.
619-623: LGTM! Consistent callback handling.The pending presence callback correctly follows the same pattern as the attached state callback, properly accessing
status.status.errorInfo.
665-672: LGTM! Correct status wrapping for failure case.The
failPendingPresencemethod correctly wraps theARTStatusin anARTMessageSendStatuswithnilpublishResult, which is appropriate for failure scenarios. This properly adapts the old-style status parameter to the new callback signature.Source/ARTMessageSendStatus.m (3)
6-13: LGTM! Standard designated initializer.The initializer correctly stores both the status and publishResult parameters, following standard Objective-C initialization patterns.
15-18: LGTM! Convenient success factory method.The
okWithPublishResult:factory method correctly creates a success status (ARTStateOk) and associates it with the optional publish result, providing a clean API for creating successful send statuses.
20-23: LGTM! Appropriate error factory method.The
errorWithInfo:factory method correctly creates an error status with the provided error information andnilpublishResult, which is appropriate since errors don't have associated publish results.Source/include/Ably/AblyPublic.h (1)
52-52: LGTM!The addition of
ARTUpdateDeleteResult.hto the public header correctly exposes the new result type for the updatedupdateMessageanddeleteMessageAPIs as part of Protocol v5.Source/ARTDefault.m (1)
6-6: Verify backend compatibility for API version 5.The API version bump from
"4"to"5"is a significant change that affects all API requests. Ensure that the Ably backend supports Protocol v5 before merging.Also, consider whether the
// CSV2comment is still accurate for version 5, or if it should be updated to reflect the new protocol version semantics.Source/PrivateHeaders/Ably/ARTRealtime+Private.h (2)
4-4: LGTM!The import of
ARTMessageSendStatus.his correctly added to support the new callback type used in the send methods.
166-168: LGTM!The signature changes from
ARTStatusCallbacktoARTMessageSendCallbackcorrectly align with Protocol v5's enhanced ACK handling, which now propagates publish result details (per TR4s) through the newARTMessageSendStatustype.Source/PrivateHeaders/Ably/ARTQueuedMessage.h (1)
3-3: LGTM!The changes correctly update the callback types to use
ARTMessageSendCallbackfor ACK handling, consistent with the Protocol v5 changes. The removal of themergeFrom:maxSize:sentCallback:ackCallback:method (noted in the AI summary) aligns with the PR objective to remove RTL6d message bundling functionality.Also applies to: 16-16, 19-19
Source/Ably.modulemap (1)
145-147: LGTM!The new private headers for
ARTPublishResult,ARTPublishResultSerial, andARTMessageSendStatusare correctly added to the Private module, enabling access for internal testing while keeping them out of the public API surface.Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h (2)
8-10: LGTM!Forward declarations for
ARTPublishResultandARTPublishResultSerialare correctly added to avoid circular imports.
62-70: LGTM!The new serialization and deserialization methods follow the established patterns in this encoder. The distinction between
ARTPublishResultSerial(value-based serialization) andARTPublishResult(dictionary-based serialization) is clear and the nullable annotations are correctly applied to deserialization methods.Source/PrivateHeaders/Ably/ARTEncoder.h (2)
15-15: LGTM!Forward declaration for
ARTUpdateDeleteResultcorrectly added to support the new protocol method.
87-88: LGTM!The new
decodeUpdateDeleteResult:error:method follows the established pattern for decoder methods in this protocol. Per the AI summary, the implementation inARTJsonLikeEncoder.mprovides the correspondingdecodeUpdateDeleteResult:andupdateDeleteResultFromDictionary:methods.Source/ARTRealtimeAnnotations.m (1)
172-174: LGTM! Callback type migration is correct.The migration from
ARTStatusCallbacktoARTMessageSendCallbackand the corresponding error access pattern change (status.errorInfo→status.status.errorInfo) correctly reflects the newARTMessageSendStatuswrapper type introduced in this PR.Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (2)
10-10: LGTM! Import added for new type dependency.The import for
ARTMessageSendStatus.his necessary to support the newARTMessageSendCallbacktype used in the method signature at line 133.
133-133: LGTM! Method signature updated consistently.The callback type change from
ARTStatusCallbacktoARTMessageSendCallbackaligns with the project-wide migration to the new messaging result types.Source/ARTPendingMessage.m (1)
5-8: LGTM! Initializer parameter type updated correctly.The parameter type change from
ARTStatusCallbacktoARTMessageSendCallbackis consistent with the project-wide callback type migration, and the parameter is correctly forwarded to the superclass initializer.Source/include/module.modulemap (1)
145-147: LGTM! Private API headers added correctly.The three new headers (
ARTPublishResult.h,ARTPublishResultSerial.h,ARTMessageSendStatus.h) are appropriately added to the explicit Private module for test access, consistent with the comment at line 9 indicating this module is "Intended only for testing."Source/include/Ably/ARTTypes.h (2)
20-20: LGTM! Forward declaration added for new public type.The forward declaration for
ARTUpdateDeleteResultis necessary to support the new callback typedef introduced at lines 568-569.
568-569: LGTM! New callback typedef follows established patterns.The
ARTUpdateDeleteResultCallbacktypedef is consistent with other callback patterns in this file (e.g.,ARTPaginatedMessagesCallbackat line 560) and appropriately supports the new update/delete mutation result API introduced in this PR.Source/ARTPublishResultSerial.m (1)
1-16: LGTM!The implementation is clean and follows Objective-C best practices. The initializer correctly uses
[value copy]to ensure immutability of the string property, and thedescriptionmethod provides useful debugging output consistent with other types in this PR.Source/ARTProtocolMessage.m (1)
28-28: LGTM!The new
resfield is properly integrated: initialized tonilininit, included indescriptionfor debugging, and correctly propagated incopyWithZone:. The changes follow the existing patterns for other protocol message fields.Also applies to: 60-60, 83-83
Source/include/Ably/ARTUpdateDeleteResult.h (1)
1-27: LGTM!The header is well-designed:
NS_SWIFT_SENDABLEis appropriate for a value-like result type used in async contexts.- Disabling default
initwith a designated initializer enforces proper construction.- Good documentation explains when
versionSerialwould benil(message superseded).Source/PrivateHeaders/Ably/ARTPublishResultSerial.h (1)
1-26: LGTM!The header follows the same well-designed pattern as
ARTUpdateDeleteResult.h:
NS_SWIFT_SENDABLEfor Swift concurrency compatibility.- Default
initdisabled with a proper designated initializer.- Clear documentation explaining when
valuewould benil(message discarded due to conflation).Source/PrivateHeaders/Ably/ARTPendingMessage.h (1)
3-3: Callback type change properly implemented and all call sites updated.The callback type has been correctly changed from
ARTStatusCallbacktoARTMessageSendCallbackto support propagating publish result information through the newARTMessageSendStatustype. All initializer call sites inARTRealtime.m:1315use the correct callback type, and the implementation is consistent with the base classARTQueuedMessage.Source/PrivateHeaders/Ably/ARTPublishResult.h (1)
1-31: LGTM! Well-designed result type.The interface is clean, properly documented, and correctly uses NS_SWIFT_SENDABLE for thread-safe access from Swift. The documentation clearly explains that serials can be nil when messages are conflated, which is important context for consumers of this API.
Source/PrivateHeaders/Ably/ARTMessageVersion+Private.h (1)
4-4: LGTM! Clean extension of serialization capabilities.The forward declaration avoids header coupling, and the new
initWithOperation:initializer provides a clear path for constructing MessageVersion from MessageOperation. The comment effectively explains the wire serialization use case.Also applies to: 16-17
Source/PrivateHeaders/Ably/ARTChannel+Private.h (2)
20-45: Excellent documentation improvement.The expanded contract documentation clearly delineates responsibilities between the caller and subclass implementations. This will help prevent bugs and makes the API easier to maintain.
47-67: LGTM! Well-designed mutation API.The new
internalSendMutationRequestForMessagemethod follows the same excellent documentation pattern asinternalPostMessages, with clear contracts and subclass responsibilities. The use ofARTUpdateDeleteResultCallbackis semantically appropriate for mutation operations.Source/ARTQueuedMessage.m (2)
8-22: LGTM! Consistent callback type migration.The callback type changes from
ARTStatusCallbacktoARTMessageSendCallbackare consistent with the broader PR shift toARTMessageSendStatus. All related types are updated correctly throughout the implementation.Also applies to: 36-42
1-44: Removal of mergeFrom method is complete. No remaining call sites to the removedmergeFrom:maxSize:sentCallback:ackCallback:method were found in the codebase. The removal aligns with the PR objective to remove RTL6d message bundling functionality.Source/ARTWrapperSDKProxyRealtimeChannel.m (2)
113-130: LGTM! Consistent callback type updates.The callback type changes from
ARTCallbacktoARTUpdateDeleteResultCallbackfor bothupdateMessageanddeleteMessagealign with the PR's introduction ofARTUpdateDeleteResult. The implementations correctly delegate to the underlying channel with wrapper SDK agents.
132-138: LGTM! Well-integrated new appendMessage functionality.The new
appendMessagemethod follows the same pattern asupdateMessageanddeleteMessage, correctly delegating to the underlying channel's internal implementation with wrapper SDK agents. The implementation is consistent and correct.Source/PrivateHeaders/Ably/ARTMessageSendStatus.h (2)
31-39: LGTM: Well-designed convenience constructors.The convenience constructors provide clear, ergonomic API surface for the common success and error cases.
44-44: LGTM: Standard callback typedef.The callback signature appropriately takes a non-nullable status parameter and follows Objective-C conventions.
Source/PrivateHeaders/Ably/ARTChannel.h (2)
60-64: LGTM: Consistent API design for appendMessage.The new
appendMessagemethod follows the same signature pattern asupdateMessageanddeleteMessage, providing good API consistency for message mutation operations.
48-58: ARTUpdateDeleteResultCallback is properly defined and exposed.The callback type change to
ARTUpdateDeleteResultCallbackis confirmed as a valid breaking change for the experimental mutable messages API. The type is correctly defined inSource/include/Ably/ARTTypes.h(line 569) and the corresponding result typeARTUpdateDeleteResultis included in the public API viaAblyPublic.h. Both types are accessible to SDK consumers through the module maps, which useexport *to expose all public headers.Source/include/Ably/ARTChannelProtocol.h (1)
73-84: LGTM: Clear documentation of shallow mixin semantics.The documentation clearly explains the shallow mixin behavior where
nilproperties are left unchanged while non-nilproperties replace existing values. The note about not mutating the original message is helpful.Ably.xcodeproj/project.pbxproj (1)
319-342: New result/status files are consistently wired into all targetsThe ARTPublishResult*, ARTMessageSendStatus*, and ARTUpdateDeleteResult* files are correctly added as PBXFileReference and PBXBuildFile entries, grouped under
Types, and included in the Sources phases for all three frameworks (iOS/macOS/tvOS). The structure and duplication pattern match existing types and look sound.Also applies to: 1244-1251, 1973-1980, 3008-3156, 3158-3305, 3307-3454
Source/ARTRealtime.m (1)
1450-1463: ACK handling + TR4s publish results mapping looks soundThe updated
ack:implementation correctly:
- Maps nack’d pending messages to
ARTMessageSendStatus errorWithInfo:message.error.- Iterates over ack’d messages and, per TR4s, pulls the corresponding
ARTPublishResultfrommessage.res[index]when available.- Wraps each in
+[ARTMessageSendStatus okWithPublishResult:], gracefully handling missing/shortresarrays.This keeps per-message status and optional publish result aligned with protocol semantics.
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (1)
590-638:test_subscribeToAppendssemantics align with MESSAGE_APPEND behavior
test_subscribeToAppendsnicely exercises the realtime append flow:
- Attaches to a mutable realtime channel.
- Publishes an initial
"Hello"message and waits for it to hit history.- Subscribes on the channel, then calls
appendwith data" world!"for the existing message.- Asserts that the received message has data
" world!"and.action == .append, matching the expected MESSAGE_APPEND semantics, distinct from the full updated message fetched viagetMessageWithSerial.This is a solid end-to-end validation of the new append subscription behavior.
Source/ARTJsonLikeEncoder.m (1)
412-429: Publish/update/delete result encoding/decoding and action handling look consistentKey points in this file look solid:
intFromMessageAction:now includesARTMessageActionAppend -> 5, matching how tests assert action codes.messageToDictionary:still only emits theactionfield whenmessage.actionIsInternallySetisYES, whilemessageFromDictionary:never sets that flag. That keeps the “SDK-set vs wire-provided action” distinction intact, as per the earlier guidance. Based on learnings, this is the desired behavior.- The new ARTPublishResult/ARTPublishResultSerial helpers and
protocolMessageToDictionary/protocolMessageFromDictionarymapping ofresare nil-safe and correctly round-trip arrays of per-message publish results.decodeUpdateDeleteResult:andupdateDeleteResultFromDictionary:cleanly map the REST JSON{ "versionSerial": ... }intoARTUpdateDeleteResult, returningnilon malformed input without disturbing other decode paths.No changes needed here from a correctness perspective.
Also applies to: 625-714, 773-815, 1067-1068, 1129-1140
Source/ARTRealtimeChannel.m (2)
150-160: Realtime channel update/delete/append wrappers match new result callback typeSwitching the public
updateMessage/deleteMessage/appendMessagewrappers to useARTUpdateDeleteResultCallbackand delegating to the internal implementation withwrapperSDKAgents:nilis consistent with the new REST/realtime mutation APIs and the tests’ expectations aroundUpdateDeleteResult.No issues here.
525-546:publishProtocolMessagecorrectly routes mutations through the new send/ack flow
publishProtocolMessage:callback::
- Immediately fails with an
ARTMessageSendStatuserror when the channel isSuspendedorFailed, using a clearARTErrorChannelOperationFailedInvalidStateerror.- For all other states, forwards the protocol message to
realtime send:sentCallback:ackCallback:, wrapping the underlyingARTMessageSendStatusinto the provided callback.This is a clean abstraction for both publish and mutation flows over the new
ARTMessageSendStatustype.
lawrence-forooghian
changed the title
lawrence-forooghian
force-pushed
the
protocol-v5
branch
from
77970c5 to
ad49f8d
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2178/features
lawrence-forooghian
force-pushed
the
protocol-v5
branch
from
ad49f8d to
9e08c7f
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2178/features
github-actions
Bot
temporarily deployed
to
staging/pull/2178/jazzydoc
lawrence-forooghian
changed the title
lawrence-forooghian
force-pushed
the
protocol-v5
branch
from
9e08c7f to
449b018
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2178/features
github-actions
Bot
temporarily deployed
to
staging/pull/2178/jazzydoc
Those added in ae7e2e6 must have been copied from a pre-00af41e version of the spec.
Generated by Claude.
lawrence-forooghian
force-pushed
the
protocol-v5
branch
from
449b018 to
4be92a5
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2178/features
github-actions
Bot
temporarily deployed
to
staging/pull/2178/jazzydoc
This has been non-required functionality for a long time and has been removed in the latest version of the spec (see [1]). [1] ably/specification#408
Protocol v5 includes a new `res` field in the ACK, as described by spec point TR4s; this allows us to find out the serial of the message that was published. Here, we extract it and propagate its contents to callers of -send:sentCallback:ackCallback: and -publishProtocolMessage:callback:. Note that Objective-C offers no good way of allowing nullable string pointers in an NSArray, hence the intermediate ARTPublishResultSerial type. ARTPublishResult is currently private, but this may change if we later choose to return it from `publish()` as the latest version of the spec describes (we'd first have to figure out how to handle this breaking public API change). Largely written by Claude. I've manually tested it; will add automated tests for it once we've got some public API that exposes it.
lawrence-forooghian
force-pushed
the
protocol-v5
branch
from
4be92a5 to
df3377e
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2178/features
github-actions
Bot
temporarily deployed
to
staging/pull/2178/jazzydoc
github-actions
Bot
temporarily deployed
to
staging/pull/2178/features
github-actions
Bot
temporarily deployed
to
staging/pull/2178/jazzydoc
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)
Source/PrivateHeaders/Ably/ARTQueuedMessage.h (1)
1-20: Fix nullability mismatch forackCallbackaccessor.
init...ackCallback:is nullable (Line 16) but-ackCallbackreturns nonnull (Line 19). Either:
- make the accessor nullable, or
- guarantee it always returns a nonnull no-op block (and document it).
Proposed fix (make accessor nullable)
@@ -- (ARTMessageSendCallback)ackCallback; +- (nullable ARTMessageSendCallback)ackCallback;
🤖 Fix all issues with AI agents
In @Ably.xcodeproj/project.pbxproj:
- Around line 319-342: The modulemap is missing ARTUpdateDeleteResult.h from the
umbrella (public) module, causing that public header and its typedefs
(ARTUpdateDeleteResultCallback) to not be exported; update the modulemap by
adding ARTUpdateDeleteResult.h to the umbrella module's public header list (not
the Private section) so the header is exported alongside other public headers
referenced by AblyPublic.h.
In @Source/ARTChannel.m:
- Around line 206-208: The comment above -[ARTChannel
appendMessage:operation:params:wrapperSDKAgents:callback:] incorrectly
references "RTL15b1"; update the spec reference to match the other methods (use
"RSL15b1" or include both "RSL15b1, RTL32b1" as appropriate) so the inline
comment is consistent with the REST spec references used elsewhere; modify the
comment text only, leaving the call to [self editMessage:action:...] unchanged.
In @Source/ARTRealtimeChannel.m:
- Around line 477-515: In internalSendMutationRequestForMessage: (inside the
publishProtocolMessage callback) add nil-safety before accessing
status.publishResult.serials.firstObject.value: check that status,
status.publishResult, status.publishResult.serials.firstObject, and its value
are non-nil; only create ARTUpdateDeleteResult when there is no error
(status.status.errorInfo is nil) and a non-nil versionSerial, otherwise call the
callback with a nil result and the error; ensure you still invoke the wrapped
callback on the user queue as currently done.
In @Source/PrivateHeaders/Ably/ARTMessageSendStatus.h:
- Around line 1-46: The header is missing a declaration for ARTErrorInfo used by
+errorWithInfo:, so add a forward declaration for ARTErrorInfo (e.g., @class
ARTErrorInfo;) near the existing @class ARTPublishResult; declaration at the top
of ARTMessageSendStatus.h to make the header self-sufficient and avoid relying
on transitive includes.
🧹 Nitpick comments (11)
Source/ARTUpdateDeleteResult.m (1)
5-10: Consider consistent initialization pattern.The initializer uses
if (self = [super init])while other files in this PR (e.g.,ARTMessageVersion.mline 22) use the pattern:self = [super init]; if (self) {Both are valid, but consistency across the codebase improves readability.
♻️ Suggested consistency fix
- (instancetype)initWithVersionSerial:(nullable NSString *)versionSerial { - if (self = [super init]) { + self = [super init]; + if (self) { _versionSerial = versionSerial; } return self; }Source/ARTProtocolMessage.m (2)
40-63: Keep-descriptionsafe/usable with potentially largeres.Including
resin-description(Line 60) is helpful, but double-check it won’t dump excessively large payloads or sensitive contents into logs in typical debug/error flows.
11-31: Consider applyingcopysemantics toresproperty for consistency with defensive copying practices.The
resproperty is declared as@property (nullable, nonatomic) NSArray<ARTPublishResult *> *res;without thecopyattribute. While NSArray is immutable and the current direct assignment in-copyWithZone:(pm.res = self.res;) is safe from mutation, applyingcopysemantics to immutable collections is a defensive best practice that ensures consistent copying behavior across the codebase and provides protection if the type is refactored in the future.Source/PrivateHeaders/Ably/ARTMessageVersion+Private.h (1)
4-18: Clarify failure semantics for new construction paths (esp. dictionary-based).The new operation-based initializer is fine, but for the dictionary deserialization API in this header, consider whether it should be
nullable(or explicitly documented as “throws/asserts/never fails”) to avoid hard-to-debug crashes on malformed input.Source/include/Ably/ARTUpdateDeleteResult.h (1)
1-27: Clean public API; consider designated-initializer annotation + copy in impl.Shape looks good. In the
.m, ensureversionSerialis stored as a copied value (and consider marking-initWithVersionSerial:asNS_DESIGNATED_INITIALIZERfor clarity).Source/PrivateHeaders/Ably/ARTPublishResult.h (1)
1-31: Looks good; ensureserialsis defensively copied.Given
serialsis foundational to ACK result propagation, make sure the implementation stores an immutable copy ([serials copy]) to prevent accidental mutation by callers.Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (3)
400-402: Avoid force cast; use safe unwrapping instead.Static analysis flagged this force cast. While
ARTMessagelikely conforms toNSCopyingand returns the correct type, usingas?with a guard would be safer and more explicit.Suggested fix
- let messageAppend = publishedMessage.copy() as! ARTMessage + guard let messageAppend = publishedMessage.copy() as? ARTMessage else { + XCTFail("Failed to copy published message") + return + }The same pattern appears at lines 157 and 279 - consider applying consistent safe unwrapping across all similar casts.
222-231: Consider adding an iteration limit to polling loops.This polling pattern could run indefinitely if the server never returns an updated message version. While
waitUntilhas a timeout for each iteration, the outer loop has no bound. Consider adding a maximum iteration count to prevent tests from hanging indefinitely in edge cases.Example with iteration limit
+ var pollIterations = 0 + let maxPollIterations = 30 // Fetch the updated message (poll getMessageWithSerial until it returns the updated message) - while updatedMessage == nil || updatedMessage!.version?.serial == publishedMessage.version?.serial { + while (updatedMessage == nil || updatedMessage!.version?.serial == publishedMessage.version?.serial) && pollIterations < maxPollIterations { + pollIterations += 1 // Get the updated message by serial string waitUntil(timeout: testTimeout) { done in ... } } + XCTAssertLessThan(pollIterations, maxPollIterations, "Polling exceeded max iterations")This pattern appears at lines 344-354 and 467-477 as well.
638-638: Remove debug print statement.This debug print statement appears to be left over from development and should be removed from the test code.
Suggested fix
channel.append(publishedMessage, operation: nil, params: nil) { _, error in - print("Append completed") XCTAssertNil(error) partialDone() }Source/ARTRestChannel.m (1)
529-531: Consider using an Ably-specific error code for non-200 responses.Using
response.statusCodedirectly as the error code conflates HTTP status codes with Ably error codes. Consider using a dedicated Ably error code (e.g.,ARTErrorBadRequestor similar) and including the HTTP status in the message.Example
} else { - callback(nil, [ARTErrorInfo createWithCode:response.statusCode message:[NSString stringWithFormat:@"Failed to %@ message: HTTP %ld", logOperation, (long)response.statusCode]]); + callback(nil, [ARTErrorInfo createWithCode:ARTErrorBadRequest status:response.statusCode message:[NSString stringWithFormat:@"Failed to %@ message: HTTP %ld", logOperation, (long)response.statusCode]]); }Source/ARTRealtimeChannel.m (1)
498-504: Consider defensive nil check for stringValue result.The code converts
ARTStringifiablevalues to strings usingstringValuebut doesn't check if the result could be nil. While this is likely safe if the protocol contract guarantees non-nil returns, adding a defensive check would improve robustness.♻️ Optional defensive coding suggestion
if (params && params.count > 0) { NSMutableDictionary<NSString *, NSString *> *stringParams = [NSMutableDictionary dictionaryWithCapacity:params.count]; for (NSString *key in params) { - stringParams[key] = params[key].stringValue; + NSString *stringValue = params[key].stringValue; + if (stringValue) { + stringParams[key] = stringValue; + } } msg.params = stringParams; }
📜 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 (46)
Ably.xcodeproj/project.pbxprojSource/ARTChannel.mSource/ARTDefault.mSource/ARTJsonLikeEncoder.mSource/ARTMessage.mSource/ARTMessageSendStatus.mSource/ARTMessageVersion.mSource/ARTPendingMessage.mSource/ARTProtocolMessage.mSource/ARTPublishResult.mSource/ARTPublishResultSerial.mSource/ARTQueuedMessage.mSource/ARTRealtime.mSource/ARTRealtimeAnnotations.mSource/ARTRealtimeChannel.mSource/ARTRealtimePresence.mSource/ARTRestChannel.mSource/ARTUpdateDeleteResult.mSource/ARTWrapperSDKProxyRealtimeChannel.mSource/Ably.modulemapSource/PrivateHeaders/Ably/ARTChannel+Private.hSource/PrivateHeaders/Ably/ARTChannel.hSource/PrivateHeaders/Ably/ARTEncoder.hSource/PrivateHeaders/Ably/ARTJsonLikeEncoder.hSource/PrivateHeaders/Ably/ARTMessageSendStatus.hSource/PrivateHeaders/Ably/ARTMessageVersion+Private.hSource/PrivateHeaders/Ably/ARTPendingMessage.hSource/PrivateHeaders/Ably/ARTProtocolMessage+Private.hSource/PrivateHeaders/Ably/ARTProtocolMessage.hSource/PrivateHeaders/Ably/ARTPublishResult.hSource/PrivateHeaders/Ably/ARTPublishResultSerial.hSource/PrivateHeaders/Ably/ARTQueuedMessage.hSource/PrivateHeaders/Ably/ARTRealtime+Private.hSource/PrivateHeaders/Ably/ARTRealtimeChannel+Private.hSource/include/Ably/ARTChannelProtocol.hSource/include/Ably/ARTMessage.hSource/include/Ably/ARTTypes.hSource/include/Ably/ARTUpdateDeleteResult.hSource/include/Ably/AblyPublic.hSource/include/module.modulemapTest/AblyTests/Tests/ARTDefaultTests.swiftTest/AblyTests/Tests/MessageUpdatesDeletesTests.swiftTest/AblyTests/Tests/RealtimeClientChannelTests.swiftTest/AblyTests/Tests/RealtimeClientConnectionTests.swiftTest/AblyTests/Tests/RealtimeClientTests.swiftTest/AblyTests/Tests/RestClientTests.swift
💤 Files with no reviewable changes (2)
- Test/AblyTests/Tests/RealtimeClientChannelTests.swift
- Source/PrivateHeaders/Ably/ARTProtocolMessage+Private.h
✅ Files skipped from review due to trivial changes (2)
- Test/AblyTests/Tests/RealtimeClientTests.swift
- Test/AblyTests/Tests/RestClientTests.swift
🚧 Files skipped from review as they are similar to previous changes (10)
- Source/PrivateHeaders/Ably/ARTPublishResultSerial.h
- Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h
- Source/PrivateHeaders/Ably/ARTPendingMessage.h
- Source/PrivateHeaders/Ably/ARTProtocolMessage.h
- Source/ARTRealtimeAnnotations.m
- Source/ARTPublishResult.m
- Source/ARTWrapperSDKProxyRealtimeChannel.m
- Source/ARTPublishResultSerial.m
- Source/ARTMessageSendStatus.m
- Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTProtocolMessage.mSource/ARTUpdateDeleteResult.mSource/ARTRealtime.mSource/ARTQueuedMessage.mSource/ARTPendingMessage.mSource/ARTDefault.mSource/ARTMessage.mSource/ARTJsonLikeEncoder.mSource/ARTMessageVersion.mSource/ARTRestChannel.mSource/ARTChannel.mSource/ARTRealtimePresence.mSource/ARTRealtimeChannel.m
🧬 Code graph analysis (3)
Source/PrivateHeaders/Ably/ARTRealtime+Private.h (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (2)
send(1283-1286)send(1288-1310)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (2)
haveParam(1814-1829)haveParam(1832-1847)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (3)
extractBodyAsMsgPack(698-716)value(1920-1922)data(932-934)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift
[Error] 400-400: Force casts should be avoided
(force_cast)
🔇 Additional comments (43)
Test/AblyTests/Tests/RealtimeClientConnectionTests.swift (2)
345-345: LGTM! Protocol version correctly updated to v5.The change from
"4"to"5"aligns with this PR's protocol v5 migration objectives. The test correctly validates that the version parameter is included in the WebSocket connection URL.
1115-1125: LGTM! Message serial assertions correctly updated for protocol v5.The changes reflect protocol v5's updated message aggregation behavior after connection resumption:
- ACK aggregation: The count changed from 1 to 3, indicating that three queued messages (msgSerials 5, 6, 7) are now acknowledged in a single ACK response
- Serial offsets: The NACK msgSerial shifts from 6 to 8, and final msgSerial from 7 to 9, consistent with the three messages sent instead of one
- Test logic: The values trace correctly through the test scenario:
- Pre-disconnect: msgSerial 0-4 (5 operations)
- Post-reconnect: msgSerial 5-8 (4 more operations)
- Final: msgSerial 9
The inline comment on Line 1115 helpfully documents the message sequence breakdown.
Source/include/Ably/ARTMessage.h (1)
38-44: LGTM! Clear documentation for the new append action.The new
ARTMessageActionAppendenum value is well-positioned at the end of the enum and includes clear documentation explaining the append semantics.Source/ARTMessage.m (1)
74-75: LGTM! Correctly handles the new append action.The implementation properly returns "Append" for
ARTMessageActionAppend, following the existing pattern for other message actions.Source/ARTQueuedMessage.m (2)
36-42: LGTM! Callback implementation is consistent.The method correctly returns
ARTMessageSendCallbackand handlesARTMessageSendStatus*parameters, maintaining consistency with the initializer changes.
8-22: Callback type is properly defined and exposed.The typedef
ARTMessageSendCallbackis correctly defined inARTMessageSendStatus.hastypedef void (^ARTMessageSendCallback)(ARTMessageSendStatus *status);and properly imported inARTQueuedMessage.h. The parameter type change aligns with the PR's refactoring objectives and is correctly implemented throughout the codebase.Source/ARTRealtimePresence.m (5)
564-567: LGTM! Method signature updated consistently.The method signature correctly updates to use
ARTMessageSendCallback, aligning with the broader callback type migration throughout the codebase.
607-609: LGTM! Callback correctly uses the wrapper pattern.The code properly accesses
status.status.errorInfoto extract the error from the wrappedARTMessageSendStatus. This pattern is consistent with the new status wrapper design.
619-623: LGTM! Consistent wrapper pattern usage.The callback correctly uses
ARTMessageSendStatusand accesses the underlying error viastatus.status.errorInfo, maintaining consistency with other callback sites.
665-672: LGTM! Proper status wrapper initialization.The method correctly wraps
ARTStatusintoARTMessageSendStatuswith anilpublish result for the failure case. This demonstrates the wrapper pattern whereARTMessageSendStatusencapsulates both the status and optional publish result information.
171-171: No issues found with the factory method implementation.The class method
+newWithPresenceSyncState:is properly declared, correctly implemented via the standard factory pattern ([[self alloc] initWithPresenceSyncState:value]), and used consistently throughout the codebase in appropriate contexts (emitting presence sync state events at lines 932, 942, 950, 955, and 959).Test/AblyTests/Tests/ARTDefaultTests.swift (1)
8-8: LGTM! API version correctly updated to 5.The test expectation correctly reflects the protocol version bump from v4 to v5, aligning with the PR's implementation of protocol v5 changes.
Source/include/Ably/AblyPublic.h (1)
52-52: LGTM!The import correctly exposes the new
ARTUpdateDeleteResultpublic API type introduced in this PR.Source/Ably.modulemap (1)
145-147: LGTM!The three new private headers (
ARTPublishResult.h,ARTPublishResultSerial.h,ARTMessageSendStatus.h) are correctly exported in the Private module for testing access.Source/PrivateHeaders/Ably/ARTEncoder.h (2)
15-15: LGTM!The forward declaration for
ARTUpdateDeleteResultis appropriate for the protocol header.
87-88: LGTM!The new
decodeUpdateDeleteResult:error:protocol method correctly follows the existing encoder pattern with nullable return type and error propagation.Source/include/module.modulemap (1)
145-147: LGTM!The three new private headers are correctly exported in the SPM modulemap, consistent with the Xcode modulemap.
Source/ARTDefault.m (1)
6-6: Verify the REST API version and clarify the CSV2 annotation.The
apiVersionis bumped from"4"to"5"and is used to set theX-Ably-Versionheader on REST API calls and thevquery parameter on realtime connections. The"CSV2"comment on this line requires clarification—it's unclear what it references. Confirm that version"5"aligns with the Ably REST API specification and internal tracking (what does CSV2 mean?).Source/include/Ably/ARTTypes.h (1)
20-20: LGTM: New public API additions are well-structured.The forward declaration and callback typedef follow existing patterns in this file. The nullability annotations are correct and consistent with other callback types.
Also applies to: 568-569
Source/PrivateHeaders/Ably/ARTChannel+Private.h (1)
20-66: Excellent documentation of internal method contracts.The detailed
@discussionsections clearly define the responsibilities and boundaries between the base class and subclasses. This makes the mutation flow much easier to understand and maintain.Particularly helpful:
- Clear delineation of "work already performed" vs "subclass responsibility"
- Explicit mention of cipher encryption and encoding steps
- Reference to RSL4 specification requirement
Source/ARTUpdateDeleteResult.m (1)
12-14: LGTM: Clean description implementation.The description method properly handles nil versionSerial with the
?:operator and provides useful debugging output.Source/ARTPendingMessage.m (1)
5-8: No action required - callback types are already compatible.The superclass
ARTQueuedMessagealready acceptsARTMessageSendCallbackfor theackCallbackparameter (as confirmed in both header and implementation). TheARTPendingMessage.initWithProtocolMessage:ackCallback:method correctly passes this callback type to the superclass, and no type change occurred. The types are already aligned.Source/ARTMessageVersion.m (1)
21-29: The initialization is intentional and complete.ARTMessageOperationprovides only three properties (clientId,descriptionText, andmetadata), all of which are extracted by the initializer. Theserialandtimestampfields do not exist inARTMessageOperationand are separate properties ofARTMessageVersionthat must be set through other means.Source/ARTProtocolMessage.m (1)
65-85: Copy behavior looks consistent with new field.Propagating
resin-copyWithZone:(Line 83) matches how the other fields are handled.Source/PrivateHeaders/Ably/ARTRealtime+Private.h (2)
1-9: Private header include change is reasonable.Importing
ARTMessageSendStatus.hhere aligns with the ack callback signature changes.
154-169: Ack callback contract appears correctly updated—no lingering type mismatches detected.All call sites consistently use
ARTMessageSendCallback(ARTMessageSendStatus*)rather than the olderARTStatusCallback(ARTStatus*)pattern. BothsentCallbackandackCallbackare invoked identically on the internal queue insendImpl, and callback dispatch semantics are uniform across queued/pending message paths. User-level queue dispatch wrapping (to_userQueue) is handled at the channel level, not the connection level, which is architecturally appropriate.Source/PrivateHeaders/Ably/ARTChannel.h (1)
48-64: LGTM!The updated callback types and new
appendMessagemethod follow a consistent pattern across all mutation methods. The API surface is well-designed with uniform signatures forupdateMessage,deleteMessage, andappendMessage.Source/include/Ably/ARTChannelProtocol.h (1)
73-110: LGTM!The public protocol updates are well-documented with clear descriptions of the shallow mixin semantics for each operation. The new
appendMessagemethod follows the established pattern, and the callback documentation correctly describes theARTUpdateDeleteResultobject returned on success.Source/ARTRealtime.m (2)
1453-1462: LGTM!The ACK handling correctly extracts per-message
PublishResultfrom the protocol message'sresarray using index tracking. The bounds check at line 1458 (index < message.res.count) properly guards against out-of-bounds access when the server returns fewer results than expected.
1284-1356: LGTM!The refactoring from
ARTStatusCallbacktoARTMessageSendCallbackwithARTMessageSendStatusis well-implemented. The factory methods (errorWithInfo:andokWithPublishResult:) provide a clean abstraction, and error handling paths consistently construct the appropriate status objects.Source/ARTChannel.m (1)
159-186: LGTM!The
editMessageabstraction is well-designed:
- Properly encodes the message data before modification (RSL4)
- Creates a copy of the message to avoid mutating the user-supplied object (RSL15c)
- Correctly sets
actionIsInternallySet = YESsince the SDK is explicitly setting the action field (per retrieved learnings)- Assigns version from operation when provided (RSL15b7)
Source/ARTRestChannel.m (1)
414-535: LGTM!The
internalSendMutationRequestForMessageimplementation is comprehensive and well-structured:
- Properly validates message serial with a helpful error message (RSL15)
- Correctly constructs PATCH request to
/channels/{channelName}/messages/{serial}(RSL15b)- Encodes params in querystring (RSL15f)
- Sets appropriate Content-Type header (RSL15d)
- Successfully decodes
ARTUpdateDeleteResultfrom responseSource/ARTJsonLikeEncoder.m (3)
625-679: LGTM!The publish result serialization/deserialization methods are well-implemented:
publishResultSerialFromValuecorrectly handles bothNSStringandNSNullfor nullable strings- Proper type validation throughout (
isKindOfClasschecks)- Clean array/dictionary conversion following existing encoder patterns
1129-1140: LGTM!The
ARTUpdateDeleteResultdecoding is correctly implemented, extracting theversionSerialfield per the spec. The method follows the existing decoder patterns with proper type validation.
426-428: LGTM!The new
ARTMessageActionAppendcase correctly maps to action value 5, maintaining consistency with the TM5 specification for message actions.Ably.xcodeproj/project.pbxproj (4)
1244-1251: PBXFileReference paths look consistent with existing layout.
Private headers are underPrivateHeaders/Ably/...and the publicARTUpdateDeleteResult.his underinclude/Ably/..., which matches the project’s existing structure.
1973-1980: New types are grouped under “Types” as expected.
Keeps project organization consistent and makes it easier to locate the new API/result types.
2208-2208: Target header build phases updated consistently (iOS/macOS/tvOS).
Looks like all three framework targets now include the new headers;ARTUpdateDeleteResult.his marked Public everywhere it appears. Main follow-up is the visibility check in the earlier comment.Also applies to: 2264-2265, 2356-2357, 2420-2421, 2507-2522, 2636-2636, 2723-2738
3078-3129: New.msources are added to all platform targets.
This is the most common pbxproj miss; glad to seeARTPublishResult*.m,ARTMessageSendStatus.m, andARTUpdateDeleteResult.mare present across iOS/macOS/tvOS sources phases.Also applies to: 3227-3277, 3376-3426
Source/ARTRealtimeChannel.m (4)
36-38: LGTM: New result type imports.These imports support the new mutation result types introduced in this PR.
150-160: LGTM: Consistent public API surface for mutation operations.The callback type changes to
ARTUpdateDeleteResultCallbackare breaking changes as noted in the PR objectives. The implementation correctly delegates to internal methods and follows a consistent pattern across all three mutation operations.
525-546: TheARTMessageSendStatus.errorWithInfo:class method is properly defined and declared in the codebase. No issues found.
454-458: No issue found with the nested accessor pattern.The accessor chain
status.status.errorInfois correct. The callback parameter isARTMessageSendStatus *status, which has astatusproperty of typeARTStatus, which in turn has anerrorInfoproperty of typeARTErrorInfo. The code properly extracts the error information from the nested structure and passes it to the user's callback.
lawrence-forooghian
force-pushed
the
protocol-v5
branch
from
6e6667c to
885f0b4
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2178/features
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @Ably.xcodeproj/project.pbxproj:
- Around line 319-342: ARTPublishResult, ARTPublishResultSerial, and
ARTMessageSendStatus are marked Private in build phases but are exported in the
module (in Ably.modulemap), causing an exposure mismatch; either remove their
entries from Source/Ably.modulemap to keep them internal, or make them truly
public by moving their headers into the public headers area (e.g.,
include/Ably/) and change their PBXBuildFile settings (the build-file entries
referencing ARTPublishResult.h, ARTPublishResultSerial.h,
ARTMessageSendStatus.h) to use ATTRIBUTES = (Public, ); mirror the pattern used
by ARTUpdateDeleteResult so modulemap, header location, and build phase
visibility agree.
In @Source/ARTRealtimeChannel.m:
- Around line 477-519: In
internalSendMutationRequestForMessage:params:wrapperSDKAgents:callback: add
defensive nil checks before using
status.publishResult.serials.firstObject.value: verify status.publishResult and
status.publishResult.serials are non-nil and contain at least one element and
that the first serial's value is non-nil; if any of these are missing, invoke
the provided callback with a suitable ARTErrorInfo (or pass an error indicating
missing ACK serial) instead of creating ARTUpdateDeleteResult with a nil
versionSerial; otherwise extract the value and construct ARTUpdateDeleteResult
as before.
🧹 Nitpick comments (4)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (4)
400-402: Consider safer unwrapping for force cast.Static analysis flagged force cast at line 400. While
ARTMessage.copy()reliably returns anARTMessageand this is test code, you could useguard letfor consistency with Swift best practices. The same pattern appears at lines 157 and 279.Proposed fix
- let messageAppend = publishedMessage.copy() as! ARTMessage + guard let messageAppend = publishedMessage.copy() as? ARTMessage else { + XCTFail("Failed to copy publishedMessage") + return + }
222-231: Polling loop lacks a maximum iteration guard.The
whileloop pollsgetMessageWithSerialuntil the version serial changes, but has no upper bound on iterations. If the server never returns an updated message (flaky test, server bug), this could hang indefinitely. The same pattern appears in_test__rest_and_realtime__deleteMessage(lines 345-354) and_test__rest_and_realtime__appendMessage(lines 468-477).Consider adding a max iteration count or timeout check in the outer loop for test reliability.
Example approach
+ var attempts = 0 + let maxAttempts = 30 // Fetch the updated message (poll getMessageWithSerial until it returns the updated message) - while updatedMessage == nil || updatedMessage!.version?.serial == publishedMessage.version?.serial { + while (updatedMessage == nil || updatedMessage!.version?.serial == publishedMessage.version?.serial) && attempts < maxAttempts { + attempts += 1 // Get the updated message by serial string waitUntil(timeout: testTimeout) { done in channel.getMessageWithSerial(publishedMessageSerial) { message, error in XCTAssertNil(error) updatedMessage = message done() } } } + XCTAssertLessThan(attempts, maxAttempts, "Timed out waiting for updated message")
486-489: Clarify expected action difference between REST and realtime.The test asserts
.updateaction for the REST-fetched message (line 489) whiletest_subscribeToAppendsasserts.appendaction for the realtime-received message (line 646). This difference appears intentional but may benefit from a brief comment explaining why the fetched message has action.updaterather than.append.
638-638: Remove debug print statement.This
print("Append completed")appears to be a debug artifact that should be removed before merge.Proposed fix
channel.append(publishedMessage, operation: nil, params: nil) { _, error in - print("Append completed") XCTAssertNil(error) partialDone() }
📜 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 (19)
Ably.xcodeproj/project.pbxprojSource/ARTChannel.mSource/ARTJsonLikeEncoder.mSource/ARTMessage.mSource/ARTMessageVersion.mSource/ARTRealtimeChannel.mSource/ARTRestChannel.mSource/ARTUpdateDeleteResult.mSource/ARTWrapperSDKProxyRealtimeChannel.mSource/PrivateHeaders/Ably/ARTChannel+Private.hSource/PrivateHeaders/Ably/ARTChannel.hSource/PrivateHeaders/Ably/ARTEncoder.hSource/PrivateHeaders/Ably/ARTMessageVersion+Private.hSource/include/Ably/ARTChannelProtocol.hSource/include/Ably/ARTMessage.hSource/include/Ably/ARTTypes.hSource/include/Ably/ARTUpdateDeleteResult.hSource/include/Ably/AblyPublic.hTest/AblyTests/Tests/MessageUpdatesDeletesTests.swift
🚧 Files skipped from review as they are similar to previous changes (5)
- Source/ARTMessageVersion.m
- Source/include/Ably/ARTTypes.h
- Source/PrivateHeaders/Ably/ARTEncoder.h
- Source/ARTMessage.m
- Source/include/Ably/AblyPublic.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTChannel.mSource/ARTWrapperSDKProxyRealtimeChannel.mSource/ARTUpdateDeleteResult.mSource/ARTRealtimeChannel.mSource/ARTRestChannel.mSource/ARTJsonLikeEncoder.m
🧬 Code graph analysis (1)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (2)
extractBodyAsMsgPack(698-716)value(1920-1922)
🪛 Clang (14.0.6)
Source/include/Ably/ARTUpdateDeleteResult.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift
[Error] 400-400: Force casts should be avoided
(force_cast)
⏰ 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). (6)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (30)
Source/include/Ably/ARTMessage.h (1)
38-44: LGTM! Clear documentation for the new append action.The documentation clearly explains the append semantics—data is appended while other fields replace previous values. This aligns well with the PR's goal of adding append message functionality.
Source/PrivateHeaders/Ably/ARTChannel+Private.h (2)
20-45: LGTM! Excellent documentation of method contracts.The contract sections clearly delineate responsibilities between the base class (encoding, validation) and subclasses (transport-specific logic). This will help maintainers understand the method's expectations and prevent implementation errors.
47-66: LGTM! Well-documented internal mutation method.The method signature and contract documentation follow the same clear pattern as
internalPostMessages. The separation of encoding concerns (handled before this method) from transport concerns (handled by subclasses) is well-articulated.Source/PrivateHeaders/Ably/ARTMessageVersion+Private.h (1)
4-4: LGTM! Operation-based initializer enables edit workflow.The forward declaration and new initializer support the operation-based message version construction needed for the mutation workflow. The comment clearly explains the use case.
Also applies to: 16-17
Source/ARTUpdateDeleteResult.m (2)
5-10: LGTM! Standard initializer implementation.The initializer follows the standard Objective-C pattern correctly, with proper super initialization check and ivar assignment.
12-14: LGTM! Helpful debug description.The description method provides clear output for debugging, including the class name, pointer, and version serial (with nil handling).
Source/include/Ably/ARTUpdateDeleteResult.h (3)
5-14: LGTM! Well-designed result type.The interface is properly marked
NS_SWIFT_SENDABLEfor Swift concurrency support, and the readonly nullable property is appropriate for a result type. The documentation clearly explains whenversionSerialwill benil.
16-16: LGTM! Appropriate API design.Disabling the default initializer ensures that
versionSerialis always explicitly provided through the designated initializer, preventing unintended usage.
18-23: LGTM! Clear designated initializer.The initializer signature and documentation are clear and appropriate. The nullable parameter matches the property's nullability.
Note: The static analysis error about the Foundation import is a false positive, as the header will compile correctly within the project context.
Source/ARTWrapperSDKProxyRealtimeChannel.m (1)
113-138: LGTM!The proxy correctly delegates
updateMessage,deleteMessage, and the newappendMessageto the underlying channel's internal implementation, with proper threading of the newARTUpdateDeleteResultCallbacktype. The pattern is consistent with existing methods in this wrapper.Source/include/Ably/ARTChannelProtocol.h (1)
73-110: LGTM! Well-documented public API additions.The updated documentation clearly describes:
- Shallow mixin semantics for
updateMessageanddeleteMessage- The new
appendMessagemethod with proper append semantics (data appended, other properties replaced)- Consistent callback type (
ARTUpdateDeleteResultCallback) across all mutation methods- Clear explanation that
ARTUpdateDeleteResultis returned on successSource/PrivateHeaders/Ably/ARTChannel.h (1)
48-64: LGTM!The private header correctly extends the public
ARTChannelProtocolwith the internalwrapperSDKAgentsparameter across all mutation methods. The newappendMessagedeclaration follows the established pattern.Source/ARTChannel.m (3)
159-186: LGTM! Well-structured mutation flow.The
editMessage:action:operation:params:wrapperSDKAgents:callback:method correctly:
- Encodes the message data first (RSL4), handling encoding errors
- Creates a copy of the encoded message to avoid mutating user-supplied message (RSL15c)
- Sets
actionandactionIsInternallySet = YES(RSL15b1) - correctly indicating SDK-set action per retrieved learnings- Conditionally sets
versionfrom theARTMessageOperationwhen provided (RSL15b7)- Delegates to
internalSendMutationRequestForMessage:for subclass implementationBased on learnings, the
actionIsInternallySetflag is correctly used here since the SDK is explicitly setting the action field.
188-209: LGTM!Clean delegation pattern for all three mutation methods. Each correctly specifies its respective action type (
Update,Delete,Append) and passes through all parameters to the sharededitMessage:implementation.
254-259: LGTM!The stub follows the established pattern used for
internalPostMessagesand other methods that must be overridden by subclasses. BothARTRestChannelandARTRealtimeChannelcorrectly override this method.Ably.xcodeproj/project.pbxproj (1)
3078-3130: Project wiring for new .m implementations looks consistent across targets.The new implementations (
ARTPublishResult*.m,ARTMessageSendStatus.m,ARTUpdateDeleteResult.m) are added into the relevantPBXSourcesBuildPhaselists (e.g., Lines 3078-3129, 3258-3278, 3407-3427), matching the pattern of triplicated build file entries for iOS/macOS/tvOS.Also applies to: 3258-3278, 3407-3427
Source/ARTRestChannel.m (3)
8-8: LGTM: Private imports support mutation flow.The addition of
ARTMessage+Private.handARTMessageVersion+Private.himports is appropriate for the new mutation encoding/decoding pathway.Also applies to: 11-11
106-116: LGTM: Public API updated to return mutation results.The updated signatures for
updateMessage,deleteMessage, and newappendMessagecorrectly useARTUpdateDeleteResultCallbackto returnARTUpdateDeleteResult. This is a documented breaking change for mutable messages (marked experimental).
414-535: Unified mutation flow correctly implements REST API spec with proper message action initialization.The
internalSendMutationRequestForMessagemethod correctly:
- Validates message serial presence (RSL15)
- Encodes message body per RSC8 (RSL15d)
- Issues PATCH to
/channels/{channelName}/messages/{serial}(RSL15b)- Includes params in query string (RSL15f)
- Decodes
ARTUpdateDeleteResultfrom successful responseThe action-specific error messages (lines 434-447) and logging (lines 485-498) properly handle Update, Delete, and Append operations with appropriate defaults.
Callers properly initialize the message before invoking this method: the
editMessagemethod sets bothmessage.actionandmessage.actionIsInternallySet = YES(per RSL15b1) before passing the message to this internal method.Source/ARTJsonLikeEncoder.m (6)
16-18: LGTM: Result type imports.The new imports for
ARTPublishResult,ARTPublishResultSerial, andARTUpdateDeleteResultsupport the updated publish/mutation result handling.
426-427: LGTM: Append action mapping added.The
ARTMessageActionAppendcase correctly maps to integer value 5, extending the message action encoding per TM5.
625-714: Publish result serialization handles nullable serials correctly.The bidirectional serialization logic for
ARTPublishResultandARTPublishResultSerialproperly:
- Uses
NSNullfor nullable string values (lines 629-630, 686)- Validates input types before processing (lines 627, 636, 652, 666)
- Returns
nilon validation failure to signal decode errors- Omits empty
serialsarray when serializing (lines 701-702)
813-815: LGTM: Protocol message res field serialization.The
resfield is correctly serialized usingpublishResultsToArraywhen present in the protocol message.
1067-1067: LGTM: Protocol message res field deserialization.The
resfield is correctly deserialized usingpublishResultsFromArray, maintaining symmetry with the serialization path.
1129-1140: LGTM: UpdateDeleteResult decoding extracts versionSerial.The
decodeUpdateDeleteResultandupdateDeleteResultFromDictionarymethods correctly parse theversionSerialfield from the response dictionary.Source/ARTRealtimeChannel.m (5)
36-38: LGTM: Result type imports.The imports for
ARTPublishResult,ARTPublishResultSerial, andARTUpdateDeleteResultare consistent with the mutation result handling updates.
150-160: LGTM: Public API updated for realtime mutations.The realtime channel public API correctly mirrors the REST channel API changes, using
ARTUpdateDeleteResultCallbackforupdateMessage,deleteMessage, and the newappendMessage.
454-457: LGTM: Message publishing uses status wrapper.The
internalPostMessagesmethod correctly usesARTMessageSendStatuswrapper and extractsstatus.status.errorInfofor the error callback.
469-473: LGTM: Object publishing uses status wrapper.The
sendObjectWithObjectMessagesmethod consistently usesARTMessageSendStatusfor status handling.
529-550: LGTM: Protocol message publishing updated to use send status.The
publishProtocolMessagemethod correctly:
- Changed callback type to
ARTMessageSendCallback- Creates
ARTMessageSendStatuserror wrapper for failed channel states (line 535)- Forwards ACK with status to callback (lines 544-546)
github-actions
Bot
temporarily deployed
to
staging/pull/2178/jazzydoc
This new REST API accepts a Message object. Since the upcoming Realtime implementation of publishing updates and deletes will work similarly, I've pulled some of the message editing functionality up into ARTChannel, with an abstract internalSendMutationRequestForMessage to be overridden by the REST and Realtime channel classes. This is analoguous to how we do normal message publishing (i.e. the internalPostMessages method). The UpdateDeleteResult is not yet exposed via the public API and its extraction from the response isn't yet tested. The callback type was originally called ARTUpdateDeleteResultCallback; changed it to ARTEditResultCallback at Marat's request since he said it sounds like you're "updating the delete"; neither is great (perhaps the UpdateDeleteResult name is the actual culprit here but I'm not gonna get into that). Largely done by Claude.
That is, implement ARTRealtimeChannel's edits and deletes by sending a ProtocolMessage instead of making a REST call (this functionality has only recently been added to Realtime). As in 20d8067, the UpdateDeleteResult is not yet exposed via the public API and its extraction isn't yet tested.
Per the new RTL32e spec point. Docs adapted from [1] at e46fd34. [1] ably/specification#412
Occasionally getMessageWithSerial doesn't immediately return the new version of the message. (Ideally we'd not use any polling in these tests and wait to receive the new version on a Realtime connection, but I don't really want to restructure these tests now.)
lawrence-forooghian
force-pushed
the
protocol-v5
branch
from
885f0b4 to
79248c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @Ably.xcodeproj/project.pbxproj:
- Around line 319-342: ARTUpdateDeleteResult.h is marked Public and used by
public headers (imported by AblyPublic.h and referenced in ARTChannelProtocol.h
and ARTTypes.h) but is missing from Ably.modulemap; fix by adding
ARTUpdateDeleteResult.h to the modulemap’s public headers list (the Public
submodule) so the header is exported with the module and its symbol is visible
to clients, mirroring how ARTUpdateDeleteResult.m/.h are declared Public in the
PBX build files.
🧹 Nitpick comments (2)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (2)
65-92: Polling loops lack explicit iteration guards.The
whileloops inwaitUntilHistoryBecomesAvailableOnChannelandwaitUntilEditingHistoryBecomesAvailableForMessageSerialcan run indefinitely if the expected condition is never met. AlthoughwaitUntil(timeout: testTimeout)guards individual requests, the outer loop has no max-iteration safeguard. Consider adding an iteration cap or a cumulative timeout to prevent test hangs in edge cases.🔧 Suggested improvement
private func waitUntilHistoryBecomesAvailableOnChannel(_ channel: ARTChannelProtocol) -> ARTMessage { var firstMessage: ARTMessage! + var attempts = 0 + let maxAttempts = 30 // ~30 seconds max - while firstMessage == nil { + while firstMessage == nil && attempts < maxAttempts { + attempts += 1 sleep(1) // wait before to increase the chance of the first request waitUntil(timeout: testTimeout) { done in channel.history { result, _ in firstMessage = result?.items.first done() } } } + XCTAssertNotNil(firstMessage, "Failed to retrieve message from history after \(maxAttempts) attempts") return firstMessage }
399-402: Avoid force cast; use safe unwrapping instead.Static analysis flagged the force cast at line 400. While
copy()onARTMessageshould return anARTMessage, using a safe unwrap withXCTUnwrapaligns better with Swift best practices and provides a clearer failure message if the cast unexpectedly fails.🔧 Suggested fix
- let messageAppend = publishedMessage.copy() as! ARTMessage + let messageAppend = try XCTUnwrap(publishedMessage.copy() as? ARTMessage)Apply the same pattern to similar force casts at lines 157 and 279.
📜 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 (19)
Ably.xcodeproj/project.pbxprojSource/ARTChannel.mSource/ARTJsonLikeEncoder.mSource/ARTMessage.mSource/ARTMessageVersion.mSource/ARTRealtimeChannel.mSource/ARTRestChannel.mSource/ARTUpdateDeleteResult.mSource/ARTWrapperSDKProxyRealtimeChannel.mSource/PrivateHeaders/Ably/ARTChannel+Private.hSource/PrivateHeaders/Ably/ARTChannel.hSource/PrivateHeaders/Ably/ARTEncoder.hSource/PrivateHeaders/Ably/ARTMessageVersion+Private.hSource/include/Ably/ARTChannelProtocol.hSource/include/Ably/ARTMessage.hSource/include/Ably/ARTTypes.hSource/include/Ably/ARTUpdateDeleteResult.hSource/include/Ably/AblyPublic.hTest/AblyTests/Tests/MessageUpdatesDeletesTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Source/ARTMessage.m
- Source/PrivateHeaders/Ably/ARTChannel+Private.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTMessageVersion.mSource/ARTChannel.mSource/ARTRestChannel.mSource/ARTUpdateDeleteResult.mSource/ARTRealtimeChannel.mSource/ARTJsonLikeEncoder.mSource/ARTWrapperSDKProxyRealtimeChannel.m
🪛 Clang (14.0.6)
Source/include/Ably/ARTUpdateDeleteResult.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift
[Error] 400-400: Force casts should be avoided
(force_cast)
⏰ 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). (6)
- GitHub Check: check
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (iOS, test_iOS18_4)
🔇 Additional comments (36)
Source/include/Ably/AblyPublic.h (1)
52-52: Consider maintaining alphabetical import order.The new import for
ARTUpdateDeleteResult.his placed afterARTSummaryTypes.h, but alphabetically "Update" comes before "Summary" (wait, 'S' < 'U', so this is actually correct placement). Looking more closely at the file, the imports don't appear to follow strict alphabetical ordering throughout (e.g.,ARTAnnotation.happears afterARTSummaryTypes.h).The addition looks correct and aligns with the existing import style in this file.
Source/PrivateHeaders/Ably/ARTMessageVersion+Private.h (1)
4-5: LGTM!The forward declaration for
ARTMessageOperationand the new-initWithOperation:initializer are well-structured. The documentation clearly explains the purpose of this initializer in the context of message edit operations.Also applies to: 16-18
Source/include/Ably/ARTChannelProtocol.h (2)
74-84: Breaking change to callback type acknowledged.The callback type change from
ARTCallbacktoARTEditResultCallbackis a breaking change for consumers of this API. The PR objectives note that mutable messages remain marked as experimental, which justifies this breaking change. The updated documentation correctly describes that success now yields anARTUpdateDeleteResultobject.
99-110: LGTM! NewappendMessageAPI follows consistent pattern.The new
appendMessage:operation:params:callback:method follows the same signature pattern asupdateMessageanddeleteMessage, maintaining API consistency. The documentation clearly explains the append semantics: data is appended while other fields replace previous values.Source/PrivateHeaders/Ably/ARTEncoder.h (1)
15-15: LGTM!The forward declaration and new
decodeUpdateDeleteResult:error:protocol method follow the established patterns in this encoder protocol. The method signature is consistent with other decode methods.Also applies to: 87-88
Source/include/Ably/ARTTypes.h (1)
20-20: LGTM!The forward declaration for
ARTUpdateDeleteResultand the newARTEditResultCallbacktypedef follow the established patterns in this file. The callback signature correctly takes both the result and error as nullable parameters, consistent with other callback types defined here.Also applies to: 568-569
Source/ARTMessageVersion.m (1)
21-29: No changes required – implementation is correct.The
initWithOperation:method correctly initializes only the properties available fromARTMessageOperation(clientId, descriptionText, metadata). Theserialandtimestampproperties remainnilby design, asARTMessageOperationdoes not provide these values. These fields are server-assigned versioning data used for tracking message history, not client-provided operation metadata. The method is intentionally designed for populating outbound edit requests with operation metadata only, per the RSL15b7 specification.Source/include/Ably/ARTMessage.h (1)
38-44: The implementation is correct. TheARTMessageActionAppendenum value properly aligns with the wire protocol specification (value 5). The serialization mapping is implemented inintFromMessageAction:(TM5), the deserialization handles it inmessageFromDictionary:, and the string representation is complete inARTMessageActionToStr. No changes needed.Source/ARTWrapperSDKProxyRealtimeChannel.m (1)
113-138: LGTM - Consistent proxy pattern for mutation methods.The callback type updates to
ARTEditResultCallbackand the newappendMessagemethod correctly follow the established proxy pattern, forwarding all parameters includingwrapperSDKAgentsto the underlying channel's internal implementation.Source/include/Ably/ARTUpdateDeleteResult.h (1)
1-27: Well-structured public interface.The header follows Objective-C best practices:
- Proper nullability annotations with
NS_ASSUME_NONNULL_BEGIN/ENDNS_SWIFT_SENDABLEfor Swift concurrency compatibility- Clear documentation for the class and property
- Default
-initdisabled to enforce use of the designated initializerThe static analysis warning about
Foundation/Foundation.hnot found is a false positive from running clang outside the Xcode build environment.Source/ARTUpdateDeleteResult.m (1)
1-16: LGTM - Clean value object implementation.The implementation correctly:
- Calls
[super init]and assigns the property- Provides a helpful
descriptionmethod with nil handling for debuggingSource/PrivateHeaders/Ably/ARTChannel.h (1)
48-65: LGTM - Consistent API surface for mutation methods.The callback type updates to
ARTEditResultCallbackand the newappendMessagemethod maintain consistency across all three mutation operations (update, delete, append) with identical parameter patterns.Source/ARTChannel.m (3)
159-186: LGTM - Well-structured mutation orchestration.The
editMessage:action:operation:params:wrapperSDKAgents:callback:method correctly:
- Encodes message data with error handling (RSL4)
- Creates a copy to avoid mutating the original message
- Sets
actionandactionIsInternallySet = YESper RSL15b1 (this is correct per the learning thatactionIsInternallySetshould only be YES when the SDK explicitly sets the action)- Sets version from operation when provided (RSL15b7)
- Delegates to the
internalSendEditRequestForMessage:...hook for REST/Realtime-specific implementation
188-209: LGTM - Clean delegation to unified edit flow.The
updateMessage,deleteMessage, andappendMessagemethods correctly delegate toeditMessage:action:...with their respective action types, consolidating the mutation logic in one place.
254-259: LGTM - Standard stub pattern for subclass override.The
NSAssertpattern enforces that subclasses (ARTRestChannel, ARTRealtimeChannel) must override this method, consistent with other stubs likeinternalPostMessages:callback:in this class.Source/ARTRestChannel.m (2)
106-116: LGTM - Public API correctly updated.The public
updateMessage,deleteMessage, andappendMessagemethods now useARTEditResultCallbackand delegate to the internal implementation withwrapperSDKAgents:nil.
414-535: LGTM - Well-implemented REST mutation endpoint.The
internalSendEditRequestForMessage:...implementation correctly:
- Validates
message.serialpresence before making request (RSL15)- Builds PATCH request to
/channels/{channelName}/messages/{serial}(RSL15b)- Encodes message body and sets Content-Type (RSL15d, RSC8)
- Adds params to query string (RSL15f)
- Handles response decoding with proper error handling for missing decoders
- Returns
ARTUpdateDeleteResulton successThe error messages for missing serial are user-friendly and actionable, guiding developers to enable the feature in their dashboard.
Source/ARTJsonLikeEncoder.m (6)
16-18: LGTM - Required imports for new types.The imports for
ARTPublishResult,ARTPublishResultSerial, andARTUpdateDeleteResultenable serialization/deserialization support for the new mutation result types.
426-428: LGTM - Append action encoding added.The
ARTMessageActionAppendcase returning5aligns with the TM5 specification for message actions.
625-714: LGTM - Well-structured publish result serialization.The new methods follow the established encoder patterns in this file:
- Input validation with
isKindOfClass:checks- Graceful
nilreturns for invalid input- Fail-fast behavior in array parsing (returns nil if any item fails)
- Proper handling of nullable strings via
NSNull
813-815: LGTM - Protocol message res field serialization.The
resfield is correctly serialized to the protocol message dictionary when present.
1067-1067: LGTM - Protocol message res field deserialization.The
resfield is correctly parsed from the wire protocol message using the newpublishResultsFromArray:method.
1129-1140: LGTM - UpdateDeleteResult decoding implementation.The decoding follows the established pattern with proper type validation and verbose logging. The
versionSerialis correctly extracted as a nullable string.Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (2)
382-503: Good test coverage for the newappendMessagefunctionality.The test comprehensively covers:
- REST PATCH request verification with correct endpoint and params
- Request body encoding validation (msgpack, action field = 5 for MESSAGE_APPEND)
versionfield population fromARTMessageOperation- Polling for updated message and verifying concatenated data (
"Hello world!")ARTUpdateDeleteResult.versionSerialpropagation
599-647: LGTM!The subscription test correctly validates that the realtime channel emits a message with
.appendaction whenappendMessageis called. The use ofsplitDonefor coordinating the subscription callback and append completion is appropriate.Source/ARTRealtimeChannel.m (5)
36-38: LGTM!The new imports for
ARTPublishResult.h,ARTPublishResultSerial.h, andARTUpdateDeleteResult.halign with the PR's introduction of new result types for mutation operations.
150-160: LGTM!The updated method signatures consistently use
ARTEditResultCallbackforupdateMessage,deleteMessage, and the newappendMessage. The delegation pattern to internal implementations withwrapperSDKAgents:nilis consistent with the existing API design.
529-550: LGTM!The
publishProtocolMessagemethod correctly handles the transition toARTMessageSendStatus. Error cases create anARTMessageSendStatusviaerrorWithInfo:, and success cases propagate the status through to the callback. The pattern is consistent with the overall refactoring in this PR.
454-457: LGTM!The callback correctly extracts the error via
status.status.errorInfo, which aligns with the newARTMessageSendStatusstructure introduced in this PR. The nested access pattern is consistent with howsendObjectWithObjectMessagesat line 471 handles it.
506-517: No action required —versionSerialcan safely benilper documented specification.The code at line 512 correctly handles the case where
serialsis empty ornil. TheARTUpdateDeleteResultheader explicitly documents thatversionSerial"will benilif the message was superseded by a subsequent update before it could be published." This is a valid and expected state, not an error condition. The implementation acceptsnullable NSStringand poses no issues.Ably.xcodeproj/project.pbxproj (6)
1244-1251: Double-check file placement matches intended API surface.
ARTPublishResult.h,ARTPublishResultSerial.h, andARTMessageSendStatus.hare referenced fromPrivateHeaders/Ably/...(Line 1244-1249), whileARTUpdateDeleteResult.his ininclude/Ably/...(Line 1250). IfARTPublishResult*/ARTMessageSendStatusare meant to be public, they likely need to live underinclude/Ably(and be marked Public in Headers phases), notPrivateHeaders.
1973-1980: Project grouping looks consistent.New files are added under the
Typesgroup, keeping related domain types together (Line 1973-1980).
2163-2375: Header phase wiring looks correct; same visibility concern applies.For the iOS target’s Headers phase,
ARTUpdateDeleteResult.his included and Public (Line 2208), andARTPublishResult.h/ARTPublishResultSerial.hplusARTMessageSendStatus.hare included as Private (Line 2264-2265, Line 2356). This is consistent internally, but please confirm it matches the intended public API surface.
3008-3156: iOS target Sources phase includes new implementations (good).
ARTUpdateDeleteResult.m,ARTMessageSendStatus.m,ARTPublishResultSerial.m, andARTPublishResult.mare all present in the iOS Sources build phase (Line 3078-3079, Line 3109, Line 3127-3128).
3157-3305: macOS target Sources phase includes new implementations (good).New
.mfiles are also present in the macOS Sources build phase (Line 3227, Line 3258, Line 3276-3277), avoiding platform-specific build breaks.
3306-3454: tvOS target Sources phase includes new implementations (good).New
.mfiles are present in the tvOS Sources build phase (Line 3376, Line 3407, Line 3425-3426), keeping target wiring consistent.
2 participants
Note: The tests on this PR are currently failing, awaiting Realtime changes. See https://ably-real-time.slack.com/archives/CURL4U2FP/p1767780565601149
This PR implements the changes from the following spec PRs:
publish()though)Specifically, it:
RealtimeChanneledits and deletes to be done by sending aProtocolMessageinstead of REST requestappendMessagechannel method and support for subscribing toMESSAGE_APPENDmessage updatesupdateMessageanddeleteMessagemethods to returnUpdateDeleteResult(a breaking change we've decided we'll make since mutable messages are still marked as experimental on the docs siteSee commit messages for more details.
Summary by CodeRabbit
New Features
Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.