[ECO-5634] Messages update, delete and versions#2146
Conversation
WalkthroughAdds ARTMessageOperation and ARTDictionarySerializable types, new channel APIs for getting/updating/deleting message versions (with wrapperSDKAgents), REST/Realtime implementations including request/response handling, project/module header updates, and end‑to‑end tests validating the flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite / Client
participant Channel as ARTChannel (Realtime/REST layer)
participant RestInt as ARTRestChannelInternal
participant HTTP as Server / HTTP Executor
Test->>Channel: updateMessage(message, operation, params)
Channel->>RestInt: updateMessage:operation:params:wrapperSDKAgents:callback
RestInt->>HTTP: PATCH /messages/{serial} (MsgPack body + wrapperSDKAgents)
HTTP-->>RestInt: 200 OK / error (body)
RestInt-->>Channel: callback(error?)
Channel-->>Test: callback(error?)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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/2146/features
github-actions
bot
temporarily deployed
to
staging/pull/2146/jazzydoc
maratal
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/2146/features
github-actions
bot
temporarily deployed
to
staging/pull/2146/jazzydoc
|
I know this one's in draft but I'm going to start taking a look at it now, because next week I'll need to build the Realtime equivalent on top of this. |
You mean |
maratal
force-pushed
the
ECO-5634-message-updates
branch
from
5086d4a to
b63cc8f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2146/features
github-actions
bot
temporarily deployed
to
staging/pull/2146/jazzydoc
No, I mean that the |
maratal
force-pushed
the
ECO-5634-message-updates
branch
from
b63cc8f to
79f6161
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2146/features
github-actions
bot
temporarily deployed
to
staging/pull/2146/jazzydoc
maratal
force-pushed
the
ECO-5634-message-updates
branch
from
79f6161 to
1fff24c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2146/features
github-actions
bot
temporarily deployed
to
staging/pull/2146/features
github-actions
bot
temporarily deployed
to
staging/pull/2146/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: 1
🧹 Nitpick comments (6)
Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h (1)
1-3: Prefer#import <Foundation/Foundation.h>and verifyARTMessageOperation.hvisibilityTo keep this private header friendly to all toolchains (including the static analysis configuration that reported an error), it would be safer and more consistent with typical Ably Cocoa headers to use a standard import:
-@import Foundation; -#import <Ably/ARTMessageOperation.h> -#import "ARTDictionarySerializable.h" +#import <Foundation/Foundation.h> +#import <Ably/ARTMessageOperation.h> +#import "ARTDictionarySerializable.h"Also, given the analyzer warning
'Ably/ARTMessageOperation.h' file not found, please double‑check thatARTMessageOperation.his added to the appropriate header phase and exposed in all targets that include this private header so<Ably/…>resolves correctly.Source/include/Ably/ARTChannelProtocol.h (1)
4-5: New channel message operation APIs look coherent and well-typed
- Importing
ARTStringifiableand forward‑declaringARTMessageOperationare appropriate for the new signatures.updateMessage:…params:callback:anddeleteMessage:…params:callback:cleanly model operation metadata plus optional stringifiable params, while relying on the existingARTCallbackpattern.getMessageWithSerial:callback:andgetMessageVersionsWithSerial:callback:correctly use the newARTMessageErrorCallbackand existingARTPaginatedMessagesCallback, matching their intended behaviors.From a public API perspective, these additions are consistent with the surrounding channel methods and with the callback typedefs defined in
ARTTypes.h.If you plan further polish, consider expanding the docstrings slightly (for example, clarifying what kinds of values are expected in
paramsand howmessage.serialis obtained) to make the new operations self‑explanatory to SDK consumers.Also applies to: 9-10, 73-113
Source/include/Ably/ARTMessageOperation.h (1)
3-22: Considercopysemantics for properties and trimming unused importsFor an immutable,
NS_SWIFT_SENDABLEtype it would be safer to declare the string/dictionary properties ascopyand copy the inputs in the initializer, to avoid external mutation via NSMutableString/NSMutableDictionary instances passed in.Also,
ARTStringifiable.hisn’t used in this header; if nothing transitively relies on it being re‑exported, you can drop that import to keep the public surface narrower.Source/ARTMessageOperation.m (2)
16-21: Copy currently shares underlying objects; consider defensive copies
copyWithZone:assignsclientId,descriptionText, andmetadataby reference:operation->_clientId = self.clientId; operation->_descriptionText = self.descriptionText; operation->_metadata = self.metadata;If callers ever pass mutable instances into the initializer, both the original and the copy will see subsequent mutations. If you want
ARTMessageOperationto behave immutably, prefer copying here (and in the initializer) instead.
45-53: Unimplemented dictionary factories may fail silently in release
createFromDictionary:andinitWithDictionary:currentlyNSAssert(false)and returnnil. In release builds this just returnsnilwith no signal if someone accidentally wires these up viaARTDictionarySerializable.If these aren’t needed yet, consider either:
- Explicitly documenting them as unsupported (e.g. comment +
ARTLogWarn), or- Implementing them now using the same keys as
writeToDictionary:.This avoids surprising
nilin future refactors.Source/ARTRestChannel.m (1)
104-118: REST message update/delete/get/version wiring looks correct and consistentThe new ARTRestChannel surface methods correctly forward to the internal counterparts with
wrapperSDKAgents:nil, andARTRestChannelInternalnow centralises update/delete in_updateMessage:isDelete:operation:params:wrapperSDKAgents:callback:with:
- Proper serial presence validation for RSL12/RSL13.
- Request bodies that match the spec (serial, optional operation/name/data/encoding/extras) and reuse of
dataEncoderfor the message payload.- Correct endpoint/method selection for update (
PATCH /messages/{serial}) vs delete (POST /messages/{serial}/delete), with params encoded into the query string.- Callbacks consistently marshalled back onto
_userQueue, and wrapperSDKAgents threaded through toexecuteRequest/executePaginated.Given there’s already a follow-up issue to de-duplicate REST request construction, this structure looks good for now.
Also applies to: 408-662
📜 Review details
Configuration used: CodeRabbit 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 (16)
Ably.xcodeproj/project.pbxproj(16 hunks)Source/ARTChannel.m(1 hunks)Source/ARTMessageOperation.m(1 hunks)Source/ARTRealtimeChannel.m(2 hunks)Source/ARTRestChannel.m(3 hunks)Source/ARTSummaryTypes.m(1 hunks)Source/ARTWrapperSDKProxyRealtimeChannel.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTChannel.h(2 hunks)Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h(1 hunks)Source/include/Ably/ARTChannelProtocol.h(2 hunks)Source/include/Ably/ARTMessageOperation.h(1 hunks)Source/include/Ably/ARTTypes.h(1 hunks)Source/include/Ably/AblyPublic.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift(1 hunks)
🧰 Additional context used
🪛 Clang (14.0.6)
Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h
[error] 1-1: expected identifier or '('
(clang-diagnostic-error)
[error] 2-2: 'Ably/ARTMessageOperation.h' file not found
(clang-diagnostic-error)
Source/include/Ably/ARTMessageOperation.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift
[Warning] 5-5: Imports should be unique
(duplicate_imports)
[Error] 126-126: Force casts should be avoided
(force_cast)
[Error] 252-252: Force casts should be avoided
(force_cast)
🔇 Additional comments (11)
Ably.xcodeproj/project.pbxproj (2)
384-392: ARTMessageOperation is correctly wired across targets and build phasesThe new
ARTMessageOperationfiles (public header, private header, and.m) are added consistently to:
PBXFileReferenceand theTypesgroup,- all three frameworks’
Headersphases with appropriate Public/Private attributes,- all three frameworks’
Sourcesphases.This matches the existing pattern for other message-related types (e.g.,
ARTMessageVersion) and should make the new type available on iOS, macOS, and tvOS without build issues.Also applies to: 1241-1243, 1866-1868, 3081-3082
393-395: Confirm whether ARTDictionarySerializable is intentionally header‑only
ARTDictionarySerializable.his added as:
- a
PBXFileReferenceunderPrivateHeaders/Ably,- a member of the
Typesgroup,- a Private header in the iOS/macOS/tvOS
Headersbuild phases.There is no
ARTDictionarySerializable.mreferenced anywhere in this project file. If the type is meant to be header‑only (e.g. a protocol or category with no implementation), this is fine; if an implementation file exists, it still needs aPBXFileReference,PBXBuildFile, andSourcesentries for each target to actually compile.Please double‑check whether an
.mis expected here and wire it up if so.Also applies to: 1244-1244, 1935-1935, 2169-2171, 2501-2501, 2712-2712
Source/ARTSummaryTypes.m (1)
2-2: ImportingARTDictionarySerializableis appropriate hereThe new import cleanly reflects that the summary types conform to
ARTDictionarySerializable, without changing runtime behavior. Looks correct and consistent with the protocol usage throughout this file.Source/include/Ably/ARTTypes.h (1)
504-505: NewARTMessageErrorCallbacktypedef is consistent with existing patternsThe message+error callback shape and nullability mirror other result/error callbacks in this header and are suitable for single‑message lookup APIs.
Source/include/Ably/AblyPublic.h (1)
48-48: ExposingARTMessageOperationvia the public umbrella is correctIncluding
ARTMessageOperation.hhere makes the new message operation API available to SDK consumers in the expected way alongside other message types.Source/include/module.modulemap (1)
142-143: SPM Private module now correctly exposes new internal headersAdding
ARTDictionarySerializable.handARTMessageOperation+Private.hto theAbly.Privateexplicit module keeps the SPM module map aligned with the new internal types and matches the Xcode module map changes.Source/ARTChannel.m (1)
156-182: New abstract channel methods are wired consistently with existing designThe four new methods for update/delete/lookups follow the existing pattern for abstract operations on
ARTChannel(assert‑only bodies to enforce subclass overrides) and align with the public protocol variants that don’t takewrapperSDKAgents. As long asARTRestChannelandARTRealtimeChanneloverride these, this is a clean extension of the existing design.Please confirm both concrete channel implementations override all four methods so these code paths are never hit in production.
Source/Ably.modulemap (1)
142-143: Xcode module map correctly updated for new private headersIncluding
ARTDictionarySerializable.handARTMessageOperation+Private.hin theAbly.Privatemodule keeps the Xcode module map consistent with SPM and ensures internal code usingimport Ably.Privatecan see these types.Source/ARTWrapperSDKProxyRealtimeChannel.m (1)
107-136: Wrapper forwarding for message operations looks consistentThe new
getMessageWithSerial,updateMessage,deleteMessage, andgetMessageVersionsWithSerialmethods correctly delegate tounderlyingChannel.internaland pass throughproxyOptions.agents, matching the internal API shape and keeping wrapperSDK agent propagation consistent.Source/PrivateHeaders/Ably/ARTChannel.h (1)
48-66: Channel-level message APIs align with internal implementationsThe added
updateMessage,deleteMessage,getMessageWithSerial, andgetMessageVersionsWithSerialsignatures match the corresponding internal/rest implementations (includingARTStringifiableparams andNSStringDictionary *wrapperSDKAgents), which should keep the public/internal surfaces in sync.Source/ARTRealtimeChannel.m (1)
147-161: Realtime message operation APIs are wired cleanly through to RESTThe new
updateMessage,deleteMessage,getMessageWithSerial, andgetMessageVersionsWithSerialoverloads onARTRealtimeChannelandARTRealtimeChannelInternalconsistently delegate to the REST channel, threadingwrapperSDKAgentswhere provided and defaulting tonilotherwise. This keeps the public realtime API thin while reusing the REST implementation for edits/deletes/versions.Also applies to: 470-492
maratal
force-pushed
the
ECO-5634-message-updates
branch
from
c6474a3 to
5109ce4
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2146/features
github-actions
bot
temporarily deployed
to
staging/pull/2146/jazzydoc
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (3)
1-6: Duplicate import flagged by SwiftLint.This issue was already noted in previous review comments. The duplicate
import Ablyshould be removed, keeping only@testable import Ably.
53-80: Polling pattern may cause flaky tests.The helpers use
sleep(1)in a loop to poll for message availability. This was already flagged in a previous review comment as potentially flaky. Consider using a more reliable mechanism such as subscribing to messages on a Realtime connection.
145-145: Force cast flagged by SwiftLint.The force cast
as! ARTMessagewas already noted in previous review comments and should be replaced with a safe downcast using guard-let.
🧹 Nitpick comments (4)
Source/include/Ably/ARTMessageOperation.h (1)
3-3: Remove unused import.The import of
ARTStringifiable.hdoesn't appear to be needed in this public header—none of the declared types referenceARTStringifiable.Apply this diff:
#import <Foundation/Foundation.h> #import <Ably/ARTTypes.h> -#import "ARTStringifiable.h"Source/ARTMessageOperation.m (1)
16-22: Consider using designated initializer in copyWithZone:.The current implementation directly accesses ivars after calling
init. While this works, using the designated initializer would be more maintainable and ensure any initialization logic is consistently applied.Apply this diff:
- (id)copyWithZone:(NSZone *)zone { - ARTMessageOperation *operation = [[[self class] allocWithZone:zone] init]; - operation->_clientId = self.clientId; - operation->_descriptionText = self.descriptionText; - operation->_metadata = self.metadata; - return operation; + return [[[self class] allocWithZone:zone] initWithClientId:self.clientId + descriptionText:self.descriptionText + metadata:self.metadata]; }Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (1)
38-38: Document the "mutable:" prefix requirement.The comment states "updates and deletes don't work without this prefix" but doesn't explain why. This appears to be a server-side or test environment constraint that should be documented more clearly for maintainability.
Source/ARTRealtimeChannel.m (1)
470-493: Document that this REST delegation is temporary.These methods currently forward to the REST channel, but per the PR discussion, they will be replaced to send messages over the Realtime connection instead of making REST requests.
Consider adding inline comments indicating this is temporary implementation that will be replaced when true Realtime support is added.
📜 Review details
Configuration used: CodeRabbit 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 (16)
Ably.xcodeproj/project.pbxproj(16 hunks)Source/ARTChannel.m(1 hunks)Source/ARTMessageOperation.m(1 hunks)Source/ARTRealtimeChannel.m(2 hunks)Source/ARTRestChannel.m(3 hunks)Source/ARTSummaryTypes.m(1 hunks)Source/ARTWrapperSDKProxyRealtimeChannel.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTChannel.h(2 hunks)Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h(1 hunks)Source/include/Ably/ARTChannelProtocol.h(2 hunks)Source/include/Ably/ARTMessageOperation.h(1 hunks)Source/include/Ably/ARTTypes.h(1 hunks)Source/include/Ably/AblyPublic.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Source/include/Ably/ARTTypes.h
- Source/include/Ably/AblyPublic.h
- Source/include/module.modulemap
- Source/include/Ably/ARTChannelProtocol.h
🧰 Additional context used
🧬 Code graph analysis (1)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (2)
Test/AblyTests/Test Utilities/TestUtilities.swift (4)
commonAppSetup(107-142)clientOptions(144-159)extractBodyAsMsgPack(698-716)value(1920-1922)Test/AblyTests/Test Utilities/Test.swift (1)
uniqueChannelName(15-32)
🪛 Clang (14.0.6)
Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h
[error] 1-1: expected identifier or '('
(clang-diagnostic-error)
[error] 2-2: 'Ably/ARTMessageOperation.h' file not found
(clang-diagnostic-error)
Source/include/Ably/ARTMessageOperation.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
⏰ 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 (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
🔇 Additional comments (24)
Source/ARTSummaryTypes.m (1)
2-2: LGTM! Clean header dependency update.The replacement of
ARTSummaryTypes+Private.hwithARTDictionarySerializable.his consistent with the classes' existing protocol conformance and implementation. The refactoring consolidates serialization protocols without affecting the logic.Ably.xcodeproj/project.pbxproj (7)
384-395: Build files for ARTMessageOperation and ARTDictionarySerializable are consistent across targetsThe new PBXBuildFile entries correctly register public and private headers and the implementation for
ARTMessageOperation, plus the privateARTDictionarySerializableheader, following the existing three‑per‑target pattern. No issues spotted.
1241-1244: File references for new types are correctly defined
ARTMessageOperationpublic/private headers and implementation, and the privateARTDictionarySerializableheader, all have coherent names, paths, and file types (public headers underinclude/Ably, private underPrivateHeaders/Ably). This matches how other types are wired.
1866-1868: Placement of new types in the Types group is appropriateAdding
ARTMessageOperation(public and private) andARTDictionarySerializableinto theTypesgroup keeps the project structure consistent with other message‑related entities and shared type utilities.Also applies to: 1935-1936
2169-2171: iOS headers phase wiring looks correctFor the iOS target,
ARTMessageOperation.his exposed as Public,ARTMessageOperation+Private.handARTDictionarySerializable.hare Private, matching their paths and intended visibility. All three are present, so iOS consumers and internals will see the right surfaces.Also applies to: 2266-2267, 2296-2297
2332-2333: macOS headers phase updated consistentlyThe macOS target’s Headers phase includes the same trio (
ARTMessageOperation.hPublic;ARTMessageOperation+Private.handARTDictionarySerializable.hPrivate), mirroring the iOS configuration. No target‑skew detected.Also applies to: 2351-2352, 2501-2502
2543-2544: tvOS headers phase updated consistentlyThe tvOS target also wires
ARTMessageOperationandARTDictionarySerializableinto its Headers phase with the same public/private split. All three platforms are aligned.Also applies to: 2562-2563, 2712-2713
3081-3090: ARTMessageOperation.m is compiled for all framework targets
ARTMessageOperation.mhas been added to the Sources phases for iOS, macOS, and tvOS, so the new type is available everywhere the framework builds. This matches how other core types are handled.Also applies to: 3226-3235, 3371-3380
Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h (1)
1-10: LGTM!The private header correctly declares the
ARTDictionarySerializableconformance forARTMessageOperation. The structure follows standard patterns for private category extensions in this codebase.Source/Ably.modulemap (1)
142-143: LGTM!The module map correctly exposes the new private headers
ARTDictionarySerializable.handARTMessageOperation+Private.hin the Private module, consistent with the existing module structure.Source/include/Ably/ARTMessageOperation.h (1)
7-24: LGTM!The public API design is clean and well-documented. Properties are correctly marked
readonlyto ensure immutability, andNS_SWIFT_SENDABLEis appropriate for this value type.Source/ARTChannel.m (1)
156-182: LGTM!The abstract method pattern correctly enforces subclass override requirements, consistent with the existing
historyWithWrapperSDKAgents:andinternalPostMessages:methods. ThewrapperSDKAgentsparameter is consistently threaded through all new APIs.Source/PrivateHeaders/Ably/ARTChannel.h (2)
5-5: LGTM!The
ARTStringifiableimport is correctly added to support theparamsparameter type used in the new method declarations.
48-67: LGTM!The method declarations correctly match the implementations in
ARTChannel.mand follow consistent parameter ordering and naming conventions with the existing API surface.Source/ARTMessageOperation.m (2)
33-43: LGTM!The serialization logic correctly maps
descriptionTextproperty to"description"key (line 38), which aligns with the API specification.
45-53: ARTMessageOperation is intentionally write-only and deserialization is not needed.The class conforms to
ARTDictionarySerializableprotocol but implementscreateFromDictionary:andinitWithDictionary:as "Not implemented" assertions, while providingwriteToDictionary:for serialization. No calls to these deserialization methods exist in the codebase. The design reflects that message operations are created client-side, serialized for sending, and never deserialized from received messages.Source/ARTWrapperSDKProxyRealtimeChannel.m (1)
107-136: LGTM!The forwarding methods correctly delegate to
underlyingChannel.internalwithwrapperSDKAgents, following the established pattern used by the existinghistory:method (lines 62-65).Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (1)
82-411: Tests provide good coverage of the new message operations API.The test implementations thoroughly verify:
- HTTP methods and endpoints
- Request body serialization (including operation metadata)
- Query parameters
- Response message properties and versions
- Both REST and Realtime code paths
The test structure effectively reuses logic between REST and Realtime via the
TestEnvironmentabstraction.Source/ARTRealtimeChannel.m (1)
147-162: LGTM: Public API delegation is correct.The public methods properly delegate to internal implementations with
wrapperSDKAgents:nil, following the established pattern used elsewhere in this file.Source/ARTRestChannel.m (5)
8-9: LGTM: Required imports added.
104-119: LGTM: Public API delegation is correct.
532-548: LGTM: Proper delegation to shared implementation.The spec point comments are helpful references.
550-623: LGTM: Solid implementation with proper error handling.The method correctly constructs the request, decodes the response, and applies data decoding consistently with the history method pattern.
625-662: LGTM: Follows established pagination pattern.The implementation correctly mirrors the history method's approach for paginated results, ensuring consistency across the codebase.
maratal
force-pushed
the
ECO-5634-message-updates
branch
from
5109ce4 to
911a9e4
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2146/features
github-actions
bot
temporarily deployed
to
staging/pull/2146/jazzydoc
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (3)
1-6: Remove duplicate import.The file contains both
import Ably(line 1) and@testable import Ably(line 5). The@testable importprovides access to all public symbols, making the first import redundant.Apply this diff:
-import Ably import Nimble import XCTest @testable import Ably
154-156: Replace force cast with safe downcast.The force cast
as! ARTMessageon line 154 will crash ifcopy()doesn't return anARTMessage. Use a safe downcast with a guard statement instead.Apply this diff:
- let messageUpdate = publishedMessage.copy() as! ARTMessage + guard let messageUpdate = publishedMessage.copy() as? ARTMessage else { + XCTFail("Failed to copy published message as ARTMessage") + return + }
270-273: Replace force cast with safe downcast.The force cast
as! ARTMessageon line 270 will crash ifcopy()doesn't return anARTMessage. Use a safe downcast with a guard statement instead.Apply this diff:
- let messageDelete = publishedMessage.copy() as! ARTMessage + guard let messageDelete = publishedMessage.copy() as? ARTMessage else { + XCTFail("Failed to copy published message as ARTMessage") + return + }
🧹 Nitpick comments (3)
Source/ARTMessageOperation.m (1)
6-21: Prefer going through property setters / designated initializer in init and copyRight now both the initializer and
copyWithZone:write directly to ivars. If the properties inARTMessageOperation+Private.hare declared withcopy(or later gain different memory semantics), this will bypass those guarantees and could be surprising.Consider:
- Using property setters in the initializer, and
- Having
copyWithZone:delegate to the designated initializer so future fields aren’t accidentally missed.For example:
- (instancetype)initWithClientId:(NSString *)clientId descriptionText:(NSString *)descriptionText metadata:(NSDictionary<NSString *, NSString *> *)metadata { - self = [super init]; - if (self) { - _clientId = clientId; - _descriptionText = descriptionText; - _metadata = metadata; - } - return self; -} + (instancetype)operationWithClientId:(NSString *)clientId + descriptionText:(NSString *)descriptionText + metadata:(NSDictionary<NSString *, NSString *> *)metadata { + return [[self alloc] initWithClientId:clientId + descriptionText:descriptionText + metadata:metadata]; +} + +- (instancetype)initWithClientId:(NSString *)clientId + descriptionText:(NSString *)descriptionText + metadata:(NSDictionary<NSString *, NSString *> *)metadata { + self = [super init]; + if (self) { + self.clientId = clientId; + self.descriptionText = descriptionText; + self.metadata = metadata; + } + return self; +} @@ - (id)copyWithZone:(NSZone *)zone { - ARTMessageOperation *operation = [[[self class] allocWithZone:zone] init]; - operation->_clientId = self.clientId; - operation->_descriptionText = self.descriptionText; - operation->_metadata = self.metadata; - return operation; + return [[[self class] allocWithZone:zone] initWithClientId:self.clientId + descriptionText:self.descriptionText + metadata:self.metadata]; }(Adjust to match your existing initializer patterns if you don’t want the extra factory method.)
Source/include/Ably/ARTMessageOperation.h (1)
3-3: Inconsistent import style for public header.This import uses quotes while other public framework headers in this file use angle brackets. For consistency and proper framework resolution, consider using the angle bracket style.
-#import "ARTStringifiable.h" +#import <Ably/ARTStringifiable.h>Source/include/Ably/ARTChannelProtocol.h (1)
4-4: Inconsistent import style for public header.Same as in
ARTMessageOperation.h—this import uses quotes while other public framework headers use angle brackets.-#import "ARTStringifiable.h" +#import <Ably/ARTStringifiable.h>
📜 Review details
Configuration used: CodeRabbit 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 (16)
Ably.xcodeproj/project.pbxproj(16 hunks)Source/ARTChannel.m(1 hunks)Source/ARTMessageOperation.m(1 hunks)Source/ARTRealtimeChannel.m(2 hunks)Source/ARTRestChannel.m(3 hunks)Source/ARTSummaryTypes.m(1 hunks)Source/ARTWrapperSDKProxyRealtimeChannel.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTChannel.h(2 hunks)Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h(1 hunks)Source/include/Ably/ARTChannelProtocol.h(2 hunks)Source/include/Ably/ARTMessageOperation.h(1 hunks)Source/include/Ably/ARTTypes.h(1 hunks)Source/include/Ably/AblyPublic.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Source/include/module.modulemap
- Source/ARTWrapperSDKProxyRealtimeChannel.m
- Source/include/Ably/AblyPublic.h
- Source/ARTRealtimeChannel.m
- Source/ARTSummaryTypes.m
🧰 Additional context used
🧬 Code graph analysis (1)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (2)
Test/AblyTests/Test Utilities/TestUtilities.swift (4)
commonAppSetup(107-142)clientOptions(144-159)data(932-934)extractBodyAsMsgPack(698-716)Test/AblyTests/Test Utilities/Test.swift (1)
uniqueChannelName(15-32)
🪛 Clang (14.0.6)
Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h
[error] 1-1: expected identifier or '('
(clang-diagnostic-error)
[error] 2-2: 'Ably/ARTMessageOperation.h' file not found
(clang-diagnostic-error)
Source/include/Ably/ARTMessageOperation.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift
[Warning] 5-5: Imports should be unique
(duplicate_imports)
[Error] 154-154: Force casts should be avoided
(force_cast)
[Error] 270-270: 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 (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (21)
Source/include/Ably/ARTTypes.h (1)
504-506: ARTMessageErrorCallback typedef looks consistent and well‑placedThe new
ARTMessageErrorCallbackmatches the existing callback style (message first, error second), sits alongsideARTMessageCallback, and uses appropriate nullability for message/error. This is a sensible public shape for the new message APIs.Ably.xcodeproj/project.pbxproj (2)
384-395: New ARTMessageOperation/ARTDictionarySerializable wiring looks consistent across targets*The new PBXBuildFile, PBXFileReference, group, and Headers‑phase entries for:
ARTMessageOperation+Private.hARTMessageOperation.hARTMessageOperation.mARTDictionarySerializable.hare internally consistent and follow the same pattern as existing types (e.g.
ARTMessageVersion*,ARTStringifiable*) for all three framework targets (iOS, macOS, tvOS).ARTMessageOperation.his exposed asPublic, whileARTMessageOperation+Private.handARTDictionarySerializable.harePrivate, which matches their directory locations (include/AblyvsPrivateHeaders/Ably).One thing to double‑check: if
ARTDictionarySerializableis ever intended to be part of the public ABI (e.g. appears in public method signatures), it would need to be moved underinclude/Ablyand markedPublicin the headers phases; otherwise the current private wiring is correct.Also applies to: 1241-1244, 1866-1936, 2169-2297, 2332-2352, 2501-2501, 2543-2563, 2712-2712
3081-3081: ARTMessageOperation.m is correctly added to all platform Sources phases
ARTMessageOperation.mhas three PBXBuildFile entries and is hooked into the Sources build phases for Ably‑iOS, Ably‑macOS, and Ably‑tvOS, mirroring how other shared Objective‑C sources are wired. No duplicate or missing source wiring is apparent.Also applies to: 3226-3226, 3371-3372
Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h (1)
1-10: LGTM!Clean private category extension declaring
ARTDictionarySerializableprotocol conformance. The import style is consistent with project conventions—angle brackets for framework public headers and quotes for private headers.Source/PrivateHeaders/Ably/ARTChannel.h (1)
48-66: LGTM!The internal
wrapperSDKAgentsvariants provide the necessary plumbing for wrapper SDKs to inject their agent information. Method signatures are consistent with the public protocol and the abstract implementations inARTChannel.m.Source/Ably.modulemap (1)
142-143: LGTM!The new private headers are correctly added to the explicit
Privatemodule, enabling test access viaimport Ably.Private.Source/include/Ably/ARTMessageOperation.h (1)
10-24: LGTM!The
ARTMessageOperationinterface is well-designed withreadonlyproperties ensuring thread-safety forNS_SWIFT_SENDABLE. The designated initializer follows standard conventions.Source/include/Ably/ARTChannelProtocol.h (1)
73-113: LGTM!The new protocol methods are well-documented and follow the existing API patterns. The distinction between
updateMessage/deleteMessage(with operation metadata) andgetMessageWithSerial/getMessageVersionsWithSerial(retrieval operations) is clear.Source/ARTChannel.m (1)
156-182: LGTM!The abstract method stubs follow the established pattern in this file. All four new methods are properly implemented in both concrete subclasses:
ARTRestChannelimplements all four methods (lines 104-117)ARTRealtimeChannelimplements all four methods (lines 147-160)Each method delegates to the
_internalimplementation, passingwrapperSDKAgents:nil, which is the correct behavior. UsingNSAssertensures that missing overrides are caught during development, and the consistent implementation across subclasses confirms the abstract pattern is working as intended.Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (6)
11-60: Well-structured test environment abstraction.The
TestEnvironmentenum provides a clean dual-protocol testing approach with shared helpers for REST and Realtime channels. The factory methods properly configure clients with HTTP proxies for request inspection.
62-74: Polling pattern is appropriate given server behavior.The
sleep(1)+ polling approach correctly handles the server's eventual consistency for history availability. The pattern was discussed in past reviews and is necessary since the server initially returns empty arrays.
91-134: Comprehensive getMessage test with proper spec coverage.The test correctly validates RSL11a/b/c: takes serial as argument, sends GET to correct endpoint, and returns decoded message. Good use of guard statements for early exits.
183-250: Thorough validation of update request body and version properties.The test comprehensively verifies RSL12b1-b6 (serial, operation, name, data, encoding, extras) and correctly validates version properties including serial ordering, timestamp ordering, and operation metadata.
288-365: Comprehensive delete operation validation.The test correctly verifies RSL13b (POST to /delete endpoint), RSL13b1-b6 (request body fields), and validates the message is marked with
action == .deletewith proper version metadata.
367-432: Well-organized test entry points with proper resource cleanup.The public test methods clearly map to spec points (RSL11/RTL28, RSL12/RSL14/RTL29/RTL31, RSL13/RTL30) and use
deferblocks to ensure Realtime clients are properly closed, preventing resource leaks.Source/ARTRestChannel.m (6)
8-9: LGTM!The new imports for
ARTMessageOperationsupport the message update/delete functionality introduced in this PR.
104-118: Consistent delegation pattern for new methods.The public methods correctly delegate to internal implementations with
wrapperSDKAgents:nil, matching the established pattern used in other methods likehistory:callback:error:(line 57).
408-530: Well-structured update/delete implementation following spec requirements.The shared
_updateMessage:isDelete:...method correctly:
- Validates serial presence with a helpful error message (lines 425-430)
- Constructs request body per RSL12b/RSL13b requirements (lines 433-470)
- Encodes data with proper encoder (lines 453-465)
- Differentiates PATCH vs POST to /delete endpoint (lines 485-493)
- Handles query params and error reporting consistently (lines 497-527)
532-548: Clean internal delegation for wrapper SDK support.The internal
wrapperSDKAgents-aware methods properly delegate to the shared_updateMessage:isDelete:...helper with the appropriateisDeleteflag, enabling wrapper SDK proxy implementations to pass through context.
550-623: Correct getMessage implementation with proper two-stage decoding.The implementation correctly:
- Constructs GET request to
/messages/{serial}with URL-encoded serial (lines 564-568)- Selects decoder based on response MIME type (lines 588-594)
- Applies two-stage decoding: first protocol decoding, then data decoding with
dataEncoder(lines 596-611)- Handles errors with optional request-id augmentation (lines 573-584)
This pattern matches the established
history:method at lines 228-239.
625-662: Correct paginated getMessageVersions implementation.The implementation properly:
- Constructs GET request to
/messages/{serial}/versions(lines 639-643)- Uses
ARTPaginatedResult.executePaginatedfor pagination support (line 660)- Applies the established two-stage decoding pattern with error logging (lines 647-658)
The response processor matches the pattern used in
history:at lines 228-239.
maratal
force-pushed
the
ECO-5634-message-updates
branch
from
911a9e4 to
de8df3f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
Source/ARTMessageOperation.m (1)
45-53: Unimplemented dictionary initializers return nil in release builds.This issue was already flagged in a past review. The
createFromDictionary:andinitWithDictionary:stubs assert and returnnil, which silently fails in release builds where assertions are compiled out. Consider implementing proper deserialization or marking these methodsNS_UNAVAILABLEif deserialization is not intended.Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (3)
1-5: Duplicate import already flagged.The duplicate
import Ably(line 1) alongside@testable import Ably(line 5) was already identified in a past review. Only@testable import Ablyis needed.
157-157: Force cast already flagged.The force cast
as! ARTMessagewas already identified in a past review. Consider usingguard let ... as? ARTMessagefor safer handling.
273-273: Force cast already flagged.The force cast
as! ARTMessageat line 273 was already identified in a past review for line 157. The same fix applies here.
🧹 Nitpick comments (6)
Source/ARTMessageOperation.m (1)
16-22: Consider using the designated initializer incopyWithZone:.The current implementation bypasses the designated initializer by directly assigning instance variables. This works but can become fragile if initialization logic changes in the future.
- (id)copyWithZone:(NSZone *)zone { - ARTMessageOperation *operation = [[[self class] allocWithZone:zone] init]; - operation->_clientId = self.clientId; - operation->_descriptionText = self.descriptionText; - operation->_metadata = self.metadata; + ARTMessageOperation *operation = [[[self class] allocWithZone:zone] initWithClientId:self.clientId + descriptionText:self.descriptionText + metadata:self.metadata]; return operation; }Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift (2)
238-245: Force unwraps could cause test crashes on unexpected nil values.Multiple force unwraps (
version!.serial!,version!.timestamp!,publishedMessage.timestamp!) will crash the test if these optionals are nil, making debugging harder than using proper assertions.Consider using
XCTUnwrapor explicit assertions:let versionSerial = try XCTUnwrap(updatedMessage.version?.serial) let versionTimestamp = try XCTUnwrap(updatedMessage.version?.timestamp) let publishedTimestamp = try XCTUnwrap(publishedMessage.timestamp) XCTAssertTrue(versionSerial > publishedMessageSerial) XCTAssertTrue(versionTimestamp > publishedTimestamp)
360-367: Same force unwrap pattern in delete test.The force unwraps at lines 362 and 364 have the same issue as the update test. Consider using
XCTUnwrapfor safer test assertions.Source/include/Ably/ARTMessageOperation.h (1)
7-24: Tighten initializer contract and broaden doc wording for multi‑use operationsThe type shape looks good (readonly properties +
NS_SWIFT_SENDABLE), but two small API‑polish tweaks could help:
- The comment “Contains operation metadata for message updates” is a bit narrow now that this type is also used for deletes; consider wording like “mutable message operations (for example updates and deletes)” so the intent is clear from the header alone.
- Since all properties are readonly and there’s a single custom initializer, consider marking
initWithClientId:descriptionText:metadata:asNS_DESIGNATED_INITIALIZERand explicitly disabling plain-init(e.g.- (instancetype)init NS_UNAVAILABLE;). That will prevent partially initialised instances and makes the construction pattern clearer for both Obj‑C and Swift users.Source/include/Ably/ARTChannelProtocol.h (1)
73-113: New mutable‑message and version APIs look coherent; consider minor doc clarificationsThe added methods fit well with the existing channel API:
- Using
ARTMessageOperationhere andARTStringifiablevalues forparamsmatches how other request parameters are handled.- Nullable callbacks on the mutating methods and non‑nullable callbacks on the retrieval methods are consistent with the existing
publish:vshistory:patterns.Two minor doc‑level suggestions:
- For
updateMessage:/deleteMessage:, you already mention thatmessagemust have a populatedserial. It might be worth briefly hinting where that comes from (for example, “obtained from a previously received or published message”) so users don’t need to hunt through other docs.- For the
serialparameter in thegetMessageWithSerial:andgetMessageVersionsWithSerial:docs, you could cross‑reference the same concept (theserialfield onARTMessage) to tie the terminology together.These are purely documentation refinements; the API surface itself looks solid.
Source/ARTRestChannel.m (1)
550-662: Solid implementation of message retrieval methods.Both methods correctly implement:
- Thread-safe callback dispatching
- URL construction with proper encoding
- Comprehensive error handling
- Response decoding flow (MIME type → message → data)
- Request ID augmentation when configured
The paginated versions method appropriately reuses the existing
ARTPaginatedResultinfrastructure and follows the pattern established by thehistorymethod.Consider adding upfront validation for
serialparameter (non-nil and non-empty) in both get methods, similar to the serial validation in_updateMessageat lines 425-430. While the server will reject malformed requests, client-side validation would provide clearer error messages to users.
📜 Review details
Configuration used: CodeRabbit 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 (16)
Ably.xcodeproj/project.pbxproj(16 hunks)Source/ARTChannel.m(1 hunks)Source/ARTMessageOperation.m(1 hunks)Source/ARTRealtimeChannel.m(2 hunks)Source/ARTRestChannel.m(3 hunks)Source/ARTSummaryTypes.m(1 hunks)Source/ARTWrapperSDKProxyRealtimeChannel.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTChannel.h(2 hunks)Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h(1 hunks)Source/include/Ably/ARTChannelProtocol.h(2 hunks)Source/include/Ably/ARTMessageOperation.h(1 hunks)Source/include/Ably/ARTTypes.h(1 hunks)Source/include/Ably/AblyPublic.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/AblyTests/Tests/MessageUpdatesDeletesTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- Source/include/Ably/AblyPublic.h
- Source/PrivateHeaders/Ably/ARTChannel.h
- Source/Ably.modulemap
- Source/include/Ably/ARTTypes.h
- Source/ARTSummaryTypes.m
- Source/ARTWrapperSDKProxyRealtimeChannel.m
🧰 Additional context used
🪛 Clang (14.0.6)
Source/include/Ably/ARTMessageOperation.h
[error] 1-1: 'Foundation/Foundation.h' file not found
(clang-diagnostic-error)
Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h
[error] 1-1: expected identifier or '('
(clang-diagnostic-error)
[error] 2-2: 'Ably/ARTMessageOperation.h' file not found
(clang-diagnostic-error)
⏰ 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 (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (14)
Source/ARTChannel.m (1)
156-182: LGTM! Abstract method stubs follow established patterns.The four new methods correctly use
NSAssert(false, ...)to enforce subclass override requirements, consistent with the existing abstract methods likehistoryWithWrapperSDKAgents:completion:andinternalPostMessages:callback:at lines 207-213.Source/include/module.modulemap (1)
142-143: LGTM! Private headers correctly added to module map.The new
ARTDictionarySerializable.handARTMessageOperation+Private.hheaders are appropriately added to the Private module for internal/testing access.Source/PrivateHeaders/Ably/ARTMessageOperation+Private.h (1)
1-10: LGTM! Private category header correctly structured.The private header properly declares the
ARTMessageOperationcategory conforming toARTDictionarySerializable. The static analysis errors (lines 1-2) are false positives caused by the analyzer lacking the full build context—@import Foundationand the umbrella header path are valid in the project's module structure.Ably.xcodeproj/project.pbxproj (5)
384-395: PBXBuildFile entries for ARTMessageOperation/ARTDictionarySerializable look consistentThe new build-file entries correctly:
- Add
ARTMessageOperation+Private.has Private andARTMessageOperation.has Public.- Wire
ARTMessageOperation.minto Sources for all three platforms.- Expose
ARTDictionarySerializable.has Private only.This matches existing multi-target patterns and the intended public/private API split.
1241-1244: File references for new types are correctly defined
ARTMessageOperation+Private.h,ARTMessageOperation.h, andARTMessageOperation.mare all givenPBXFileReferences, with paths consistent with other Types files.ARTDictionarySerializable.his added underPrivateHeaders/Ably, matching its private usage.No wiring issues apparent at the file-reference level.
1866-1868: Types group membership is organised and matches neighbouring message typesPlacing
ARTMessageOperation(public and private headers plus .m) andARTDictionarySerializable.hin theTypesgroup alongsideARTMessageVersionand message annotations keeps project structure coherent and discoverable.Looks good and consistent with existing organisation.
Also applies to: 1935-1936
2169-2169: Header exposure across iOS/macOS/tvOS targets is wired correctlyThe new header entries:
- Export
ARTMessageOperation.has Public in all three headers build phases.- Keep
ARTMessageOperation+Private.handARTDictionarySerializable.hPrivate in all three headers build phases.This ensures the public API surface is consistent across platforms while keeping internals hidden.
Also applies to: 2332-2332, 2351-2351, 2501-2501, 2543-2543, 2562-2562, 2712-2712
3081-3081: Sources build phases include ARTMessageOperation.m for all platforms
ARTMessageOperation.mis added to the Sources build phases for iOS (96BF612C), macOS (D710D457), and tvOS (D710D471), matching how other shared Types are handled.No missing target or asymmetry spotted.
Also applies to: 3226-3226, 3371-3371
Source/ARTRestChannel.m (4)
8-9: LGTM!The imports are correctly added to support the new message operations functionality.
104-118: LGTM!The public method delegates follow the established pattern in the codebase, correctly passing
wrapperSDKAgents:nilto the internal implementations.
408-530: Well-implemented shared update/delete logic.The implementation correctly handles:
- Thread-safe callback dispatching
- Serial validation with user-friendly error messages
- Request body construction per spec (RSL12b/RSL13b)
- Data encoding with proper error handling
- Differentiated paths for update (PATCH) vs delete (POST to /delete endpoint)
- Query parameter serialization
- Error augmentation with request IDs when configured
532-548: LGTM!The update and delete methods correctly delegate to the shared implementation with the appropriate
isDeleteflag, maintaining clean separation of concerns.Source/ARTRealtimeChannel.m (2)
147-161: LGTM!The public method delegates follow the established pattern, correctly forwarding to internal implementations with
wrapperSDKAgents:nil, consistent with other public methods in the file.
470-492: LGTM!The internal methods correctly forward to the REST channel implementation, passing through
wrapperSDKAgentsas intended. Per the PR objectives, this REST-based approach is the current implementation strategy, with future work planned to send these operations over the Realtime connection as protocol messages.
2 participants
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.