[ECO-5323] Add annotations #2066
Conversation
WalkthroughThis change introduces comprehensive support for "annotations" in the core SDK. It adds new types, properties, methods, and protocol message handling for real-time annotations, including public and private headers, implementation files, and updates to the Xcode project structure. All changes are additive and integrate annotation functionality throughout the SDK. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ARTRealtimeChannel
participant ARTRealtimeAnnotations
participant ARTRealtimeAnnotationsInternal
participant ARTProtocolMessage
Client->>ARTRealtimeChannel: .annotations.subscribe(type, callback)
ARTRealtimeChannel->>ARTRealtimeAnnotations: subscribe(type, callback)
ARTRealtimeAnnotations->>ARTRealtimeAnnotationsInternal: subscribe(type, callback)
ARTRealtimeAnnotationsInternal-->>Client: EventListener (subscribed)
ARTProtocolMessage->>ARTRealtimeChannel: (action = Annotation) onChannelMessage
ARTRealtimeChannel->>ARTRealtimeChannelInternal: onAnnotation(message)
ARTRealtimeChannelInternal->>ARTRealtimeAnnotationsInternal: onMessage(message)
ARTRealtimeAnnotationsInternal-->>Client: callback(annotation)
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
maratal
force-pushed
the
ECO-5323-annotations
branch
from
059c3fd to
7520098
Compare
maratal
force-pushed
the
ECO-5323-annotations
branch
from
7520098 to
2f33baf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (3)
Source/ARTBaseMessage.m (1)
17-25: 🛠️ Refactor suggestion
extrasshould be copied, not referenced
copyWithZone:currently assigns the pointer directly:message->_extras = self.extras;If the caller later mutates
extras, the copy will silently change too, breaking the “copy” contract. Use an immutable copy instead:- message->_extras = self.extras; + message->_extras = [self.extras copy];This assumes
extrasconforms toNSCopying(NSDictionary/NSArray do).Test/Tests/RestClientChannelTests.swift (1)
1214-1225: 💡 Verification agent❓ Verification inconclusive
Disabling a passing test reduces coverage – prefer
XCTSkipRenaming the method to
skip__…silently drops it from execution.
Usethrow XCTSkip("…")within the test or@available(*, deprecated)if it is flaky, so that the reason shows in CI and the test remains discoverable.func test__035__presence__get__should_return_presence_fixture_data() throws { throw XCTSkip("Temporarily disabled pending presence-fixture rework (ECO-5323)") … }
Use XCTSkip Instead of Renaming Tests to Skip
Renaming the method to
skip__…removes it from XCTest discovery and lowers coverage. Instead, keep thetest…name and throwXCTSkip(...)with a clear reason so CI will report it as skipped and the test remains visible.• File: Test/Tests/RestClientChannelTests.swift
Lines: 1214–1225Proposed change:
-func skip__test__035__presence__get__should_return_presence_fixture_data() throws { +func test__035__presence__get__should_return_presence_fixture_data() throws { + throw XCTSkip("Temporarily disabled pending presence-fixture rework (ECO-5323)") let test = Test() let options = try AblyTests.commonAppSetup(for: test) options.testOptions.channelNamePrefix = nil let client = ARTRest(options: options) let key = appSetupModel.cipher.key let cipherParams = ARTCipherParams( algorithm: appSetupModel.cipher.algorithm, key: key as ARTCipherKeyCompatible, iv: Data(base64Encoded: appSetupModel.cipher.iv, options: [])! ) let channel = client.channels.get("persisted:presence_fixtures", options: ARTChannelOptions(cipher: cipherParams)) … }Source/ARTProtocolMessage.m (1)
167-169:⚠️ Potential issue
ackRequiredignores the newAnnotationaction
ackRequirednow returnsYESonly forMessageandPresence, so a client-side publish of an annotation will never wait for an ACK/NACK.
If the spec requires reliability guarantees that match ordinary messages (very likely — annotations modify channel state and should participate in the same confirm/abort flow), this is a bug.- return self.action == ARTProtocolMessageMessage || self.action == ARTProtocolMessagePresence; + return self.action == ARTProtocolMessageMessage + || self.action == ARTProtocolMessagePresence + || self.action == ARTProtocolMessageAnnotation;
🧹 Nitpick comments (25)
Test/Tests/AuthTests.swift (2)
3552-3556: PrefercanImport(UIKit)for broader platform safetyHard-coding every UIKit-based platform (
os(iOS) || os(tvOS)) can become brittle (e.g. macCatalyst, future Apple platforms).
canImport(UIKit)is a neater capability test and avoids the need to maintain the OS list.-#if os(iOS) || os(tvOS) - NotificationCenter.default.post(name: UIApplication.significantTimeChangeNotification, object: nil) -#else - NotificationCenter.default.post(name: .NSSystemClockDidChange, object: nil) -#endif +#if canImport(UIKit) + import UIKit // add once at the top of the file inside the same `#if` + NotificationCenter.default.post(name: UIApplication.significantTimeChangeNotification, object: nil) +#else + NotificationCenter.default.post(name: .NSSystemClockDidChange, object: nil) +#endif(You’ll also need an
#if canImport(UIKit) import UIKit #endifnear the top of the file to satisfy the compiler.)
No functional change, just leaner conditional logic.
3644-3660: Apply the samecanImport(UIKit)pattern & DRY up duplicationThe duplicate notification-posting block directly below repeats the new
os(iOS) || os(tvOS)check.
After adopting thecanImport(UIKit)approach above, mirror it here – or factor both occurrences into a tiny helper to keep the test body terse.Source/ARTBaseMessage.m (1)
52-61: Minor formatting inconsistency indescriptionAll previous fields (except
data) end with a comma. For consistency either:
- Add a comma after
data, or- Drop commas from all lines.
Purely cosmetic but keeps debug logs tidy.
Source/PrivateHeaders/Ably/ARTRestPresence+Private.h (1)
9-13: Return immutable collection fromasQueryItemsReturning
NSMutableArrayexposes internal mutability to callers who should treat query items as value objects. Prefer:-- (NSMutableArray<NSURLQueryItem *> *)asQueryItems; +- (NSArray<NSURLQueryItem *> *)asQueryItems;or document the mutation contract.
Source/include/Ably/AblyInternal.h (1)
14-14: Import order / groupingAll previous wrapper-proxy imports are grouped; the new annotations proxy fits but breaks alphabetical order. Consider re-ordering to keep maintenance friction low.
Source/ARTWrapperSDKProxyRealtimeChannel.m (1)
6-6: Import guard unnecessary if not iOS-specific
ARTWrapperSDKProxyRealtimeAnnotations+Private.his imported unconditionally while the feature is unsupported on macOS Catalyst/tvOS. Confirm availability or wrap with platform guards as done for Push.Source/include/Ably/ARTRealtimeChannel.h (2)
13-14: Missing public docstring for newARTRealtimeAnnotationsAPIThe new forward declaration is fine, but the public header should briefly explain what annotations are and reference the spec section, matching other properties (
presence,push).
188-193: Property retains strongly – confirm thread-safety guarantees
annotationsis exposed as readonly but mutable operations will occur internally. Document threading expectations (main vs. internal queue) to avoid misuse similar topresence.Source/include/Ably/ARTRealtimeChannelOptions.h (1)
27-33: Missing prose for the two new flagsThe docstring block above the enum lists and explains the existing flags, but the new annotation-related flags lack equivalent comments.
Add concise documentation so SDK consumers understand what “annotation publish / subscribe” actually enables.Source/Ably.modulemap (1)
47-49: Preserve alphabetical ordering to minimise merge painPrivate headers are kept alphabetically elsewhere in the list.
Insert the three new annotation headers in their correct positions (ARTRestAnnotations+Private.hafterARTRestPresence+Private.h, etc.) to keep diffs tidy.Also applies to: 129-129
Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h (2)
7-8: Forward declaration location nitFor consistency with the surrounding declarations, move the new forward declaration so the list remains alphabetically ordered.
23-24: Add API docs for the newannotationsproxy
presenceandpushhave docstrings in the implementation files; the new property is missing one.
Expose at least a single-line comment explaining that this proxiesARTRealtimeAnnotationsso wrapper authors understand its purpose.Source/include/Ably/ARTMessage.h (1)
38-39: Move the helper declaration belowNS_ASSUME_NONNULL_BEGINPlacing it inside the nullable-annotated region prevents warning noise in mixed-nullability projects.
Source/PrivateHeaders/Ably/ARTProtocolMessage.h (1)
61-62: Usecopyfor theannotationsarray to protect immutabilityOther mutable containers passed in by API consumers could be mutated after assignment, breaking thread-safety assumptions. Align with the common Cocoa pattern (
messages,presenceare already vulnerable, but we don’t need to repeat the mistake):-@property (nullable, readwrite, nonatomic) NSArray<ARTAnnotation *> *annotations; +@property (nullable, readwrite, nonatomic, copy) NSArray<ARTAnnotation *> *annotations;Source/PrivateHeaders/Ably/ARTRestAnnotations+Private.h (1)
11-12: Mark the internal initializer as designated for clarityMinor tidy-up: explicitly declaring this initializer as the designated one clarifies construction rules and prevents accidental creation paths.
-- (instancetype)initWithChannel:(ARTRestChannelInternal *)channel logger:(ARTInternalLog *)logger; +- (instancetype)initWithChannel:(ARTRestChannelInternal *)channel + logger:(ARTInternalLog *)logger NS_DESIGNATED_INITIALIZER;Source/ARTProtocolMessage.m (2)
83-96:mergeFrom:still runs the client-ID consistency check againstmessagesFor
Annotationpacketsself.messagesandsrc.messagesarenil, so theclientIdsAreDifferent:call is meaningless and only wastes cycles.
Consider short-circuiting the check to keep the code self-describing and avoid accidental coupling.Also applies to: 122-134
228-235: Enum list is getting sparse – consider documenting or reserving 18-20Unrelated to functionality, but the switch is starting to look odd: values 18-20 are skipped. Add a
/* reserved */comment or explicit cases to avoid future confusion.Source/include/Ably/ARTRestAnnotations.h (1)
37-47: Doc block says “publish” inunpublish:descriptionMinor wording slip: the second parameter block still says “publish”. Replace with “delete” to avoid confusion.
Source/include/Ably/ARTAnnotation.h (1)
35-43: Clarify “count” semanticsComment says “optional count, only relevant to certain aggregation methods”.
Consider tightening to something like “Server-supplied aggregated count for thistype/namepair”. Helps SDK users understand they shouldn’t set it on publish.Source/ARTWrapperSDKProxyRealtimeAnnotations.m (1)
9-11:proxyOptionsis never read – dead code / warning candidate
@property (nonatomic, readonly) ARTWrapperSDKProxyOptions *proxyOptions;
is assigned ininitWithRealtimeAnnotations:proxyOptions:but never referenced afterwards.
If genuinely not needed, drop it; otherwise add usage or expose via public API to avoid an “unused ivar” compiler warning.Source/include/Ably/ARTRealtimeAnnotations.h (2)
38-38: Fix typo in documentation."retriving" should be "retrieving".
Apply this diff:
-* @param callback A callback for retriving an `ARTPaginatedResult` containing annotations. +* @param callback A callback for retrieving an `ARTPaginatedResult` containing annotations.
47-47: Fix typo in documentation."retriving" should be "retrieving".
Apply this diff:
-* @param callback A callback for retriving an `ARTPaginatedResult` containing annotations. +* @param callback A callback for retrieving an `ARTPaginatedResult` containing annotations.Source/ARTRestAnnotations.m (1)
97-106: Core REST annotation methods are not implemented.All the main functionality methods (
getForMessageSerial,publish,unpublish) are TODO stubs. This aligns with the PR objectives stating that testing cannot be performed due to missing publish implementation.Would you like me to help implement these REST methods or create issues to track their implementation?
Source/ARTRealtimeAnnotations.m (2)
136-136: Fix typo in log message."anotation" should be "annotation".
Apply this diff:
-ARTLogWarn(self.logger, @"R:%p C:%p (%@) anotation subscribe to '%@' action(s) has been ignored (attempted to subscribe while channel is in FAILED state)", self->_realtime, self->_channel, self->_channel.name, type); +ARTLogWarn(self.logger, @"R:%p C:%p (%@) annotation subscribe to '%@' action(s) has been ignored (attempted to subscribe while channel is in FAILED state)", self->_realtime, self->_channel, self->_channel.name, type);
102-112: Core realtime annotation methods are not implemented.The
getForMessageSerial,publish, andunpublishmethods are TODO stubs. This aligns with the PR objectives focusing on subscription support only.Would you like me to help implement these realtime methods or create issues to track their implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
Ably.xcodeproj/project.pbxproj(32 hunks)Source/ARTAnnotation.m(1 hunks)Source/ARTBaseMessage.m(1 hunks)Source/ARTJsonLikeEncoder.m(5 hunks)Source/ARTMessage.m(3 hunks)Source/ARTProtocolMessage.m(6 hunks)Source/ARTRealtimeAnnotations.m(1 hunks)Source/ARTRealtimeChannel.m(7 hunks)Source/ARTRest.m(1 hunks)Source/ARTRestAnnotations.m(1 hunks)Source/ARTWrapperSDKProxyRealtimeAnnotations.m(1 hunks)Source/ARTWrapperSDKProxyRealtimeChannel.m(2 hunks)Source/Ably.modulemap(2 hunks)Source/PrivateHeaders/Ably/ARTPresence+Private.h(0 hunks)Source/PrivateHeaders/Ably/ARTProtocolMessage.h(3 hunks)Source/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRestAnnotations+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRestPresence+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimeAnnotations+Private.h(1 hunks)Source/include/Ably/ARTAnnotation.h(1 hunks)Source/include/Ably/ARTMessage.h(2 hunks)Source/include/Ably/ARTRealtimeAnnotations.h(1 hunks)Source/include/Ably/ARTRealtimeChannel.h(2 hunks)Source/include/Ably/ARTRealtimeChannelOptions.h(1 hunks)Source/include/Ably/ARTRestAnnotations.h(1 hunks)Source/include/Ably/ARTTypes.h(2 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtimeAnnotations.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h(2 hunks)Source/include/Ably/AblyInternal.h(1 hunks)Source/include/Ably/AblyPublic.h(1 hunks)Test/Tests/AuthTests.swift(2 hunks)Test/Tests/RestClientChannelTests.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Source/PrivateHeaders/Ably/ARTPresence+Private.h
🧰 Additional context used
🧠 Learnings (7)
Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1995
File: Source/ARTRealtimeChannel.m:227-227
Timestamp: 2024-11-04T23:10:23.027Z
Learning: In `ARTRealtimeChannelInternal`, the `internalPresence` property is declared in the private header `ARTRealtimeChannel+Private.h`, so it does not need to be redeclared in `ARTRealtimeChannel.m`.
Source/include/Ably/ARTRealtimeChannel.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1995
File: Source/ARTRealtimeChannel.m:227-227
Timestamp: 2024-11-04T23:10:23.027Z
Learning: In `ARTRealtimeChannelInternal`, the `internalPresence` property is declared in the private header `ARTRealtimeChannel+Private.h`, so it does not need to be redeclared in `ARTRealtimeChannel.m`.
Source/Ably.modulemap (1)
Learnt from: maratal
PR: ably/ably-cocoa#1995
File: Source/ARTRealtimeChannel.m:227-227
Timestamp: 2024-11-04T23:10:23.027Z
Learning: In `ARTRealtimeChannelInternal`, the `internalPresence` property is declared in the private header `ARTRealtimeChannel+Private.h`, so it does not need to be redeclared in `ARTRealtimeChannel.m`.
Source/include/Ably/ARTRealtimeChannelOptions.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTRealtimeChannelOptions.h:33-33
Timestamp: 2024-10-08T15:37:26.766Z
Learning: `ARTChannelOptions` is already marked with `NS_SWIFT_SENDABLE`.
Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1995
File: Source/ARTRealtimeChannel.m:227-227
Timestamp: 2024-11-04T23:10:23.027Z
Learning: In `ARTRealtimeChannelInternal`, the `internalPresence` property is declared in the private header `ARTRealtimeChannel+Private.h`, so it does not need to be redeclared in `ARTRealtimeChannel.m`.
Source/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1995
File: Source/ARTRealtimeChannel.m:227-227
Timestamp: 2024-11-04T23:10:23.027Z
Learning: In `ARTRealtimeChannelInternal`, the `internalPresence` property is declared in the private header `ARTRealtimeChannel+Private.h`, so it does not need to be redeclared in `ARTRealtimeChannel.m`.
Source/ARTRealtimeChannel.m (1)
Learnt from: maratal
PR: ably/ably-cocoa#1995
File: Source/ARTRealtimeChannel.m:227-227
Timestamp: 2024-11-04T23:10:23.027Z
Learning: In `ARTRealtimeChannelInternal`, the `internalPresence` property is declared in the private header `ARTRealtimeChannel+Private.h`, so it does not need to be redeclared in `ARTRealtimeChannel.m`.
🧬 Code Graph Analysis (2)
Source/include/Ably/ARTRestAnnotations.h (1)
Source/include/Ably/ARTTypes.h (2)
annotation(508-508)annotation(511-511)
Source/include/Ably/ARTRealtimeAnnotations.h (2)
Source/include/Ably/ARTTypes.h (6)
annotation(508-508)annotation(511-511)message(490-490)message(499-499)message(502-502)_Nullable(481-481)Source/PrivateHeaders/Ably/ARTEventEmitter+Private.h (1)
ARTEventListener(8-8)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: check (macOS, test_macOS)
🔇 Additional comments (52)
Source/include/Ably/AblyPublic.h (1)
24-29: Verify umbrella & modulemap exposure for new headersThe new public imports look correct, but please confirm they are also:
- Exported from the umbrella header in the framework target.
- Listed in
Ably.modulemap(it appears you touched it, but double-check).- Added to the
publicHeadersbuild phase for CocoaPods / SwiftPM.A missing export here won’t be caught by unit-tests and only surfaces for integrators.
Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (1)
16-26: Clarify ownership semantics ofannotations
presenceis a strong reference (default), andannotationsfollows the same pattern. Confirm that:
ARTRealtimeChannelInternalcreates and owns the annotations object.- There is no retain cycle back to the channel.
If lifecycle differs, annotate with
weak/assignaccordingly to make intent explicit.Source/ARTWrapperSDKProxyRealtimeChannel.m (1)
31-32: Initialiser order guarantees underlying object lifespan
_annotationscorrectly wrapschannel.annotations; however, consider marking the ivar__strongexplicitly or using the property setter (self->_annotations = …) for consistency with other members (_presence,_push).Source/include/Ably/ARTRealtimeChannelOptions.h (1)
27-33: Mind the gap in bit-flag allocation
ARTChannelModePresenceSubscribeends at1 << 19, but the next allocated value jumps to1 << 21.
If1 << 20is intentionally reserved, consider adding a comment to avoid accidental reuse later.
Otherwise, renumber to keep the enum dense.Source/ARTMessage.m (2)
33-42: 👍 Enhanced debug output looks goodIncluding action, serials, timestamps and summary will make troubleshooting far easier. No issues spotted.
70-84: Good use of a dedicated helper for enum ⇒ stringThe helper prevents magic numbers in logs and keeps
descriptionclean. Implementation looks correct.Source/PrivateHeaders/Ably/ARTProtocolMessage.h (1)
33-34:Details
❓ Verification inconclusive
Ensure downstream code handles the new
ARTProtocolMessageAnnotationactionAdding a new enum case is only half of the work – every
switch/ifthat converts or interpretsARTProtocolMessageActionnow needs an extra branch. In particular, verify:
ARTProtocolMessageActionToStr- Any encoder / decoder (
ARTJsonLikeEncoder, MsgPack, etc.)- Realtime-channel dispatchers that route protocol messages to handlers
If any of these miss the new value, annotation packets will be silently dropped or raise
defaultcase assertions.
🏁 Script executed:
#!/bin/bash # Quick scan for unhandled enum values in switch-statements rg --no-heading --line-number 'switch\s*\(.*ARTProtocolMessageAction' -A4 | \ grep -vE 'Annotation'Length of output: 97
To verify the new enum case is properly handled, let’s check the conversion routines:
🏁 Script executed:
#!/bin/bash # Check if ARTProtocolMessageAnnotation is included in the string‐conversion functions rg --no-heading --line-number 'ARTProtocolMessageActionToStr' -C5 echo "----" rg --no-heading --line-number 'ARTProtocolMessageActionFromStr' -C5Length of output: 10781
Let’s inspect the enum-to-string and string-to-enum implementations directly:
🏁 Script executed:
#!/bin/bash # Show the ToStr function and its cases rg --no-heading --line-number 'ARTProtocolMessageActionToStr' -C10 Source/ARTProtocolMessage.m echo "----" # Verify if the Annotation case is present in that switch rg --no-heading --line-number 'case ARTProtocolMessageAnnotation' -C5 Source/ARTProtocolMessage.m echo "----" # Show the FromStr function (if implemented) rg --no-heading --line-number 'ARTProtocolMessageActionFromStr' -C10 Source/ARTProtocolMessage.mLength of output: 3832
Double-check
ARTProtocolMessageAnnotationhandling across encoders & dispatchersWe’ve confirmed:
ARTProtocolMessageActionToStrincludescase ARTProtocolMessageAnnotation: return @"Annotation";- Protocol-message merge routines handle the new
annotationsarray.- No other
switch (message.action)overARTProtocolMessageActionappears to lack anAnnotationbranch.Before merging, please verify:
- JSON-like and MsgPack encoder/decoder implementations handle the new
Annotationaction (e.g. inARTJsonLikeEncoder.m, MsgPack transport).- Real-time channel dispatchers in
ARTRealtimeChannel.*/ARTRealtime.*properly routeARTProtocolMessageAnnotationinstead of falling throughdefault.Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimeAnnotations+Private.h (1)
7-10: Private initializer looks soundThe designated initializer cleanly scopes construction to internal code while keeping the public API closed. No issues spotted.
Source/include/Ably/ARTWrapperSDKProxyRealtimeAnnotations.h (1)
14-16: Confirm that external callers never need to instantiate this class
initis markedNS_UNAVAILABLE, yet no public alternative is exposed. That’s fine if instances are only produced by the SDK (e.g. viachannel.annotations). Please double-check wrapper SDK samples/tests to ensure they don’t try[[ARTWrapperSDKProxyRealtimeAnnotations alloc] init].Source/include/Ably/ARTTypes.h (1)
10-11: Typedef additions LGTMForward declaration and the three new callback typedefs follow existing style and nullability conventions.
Also applies to: 508-515
Source/ARTRealtimeChannel.m (2)
85-87: Publicannotationsaccessor looks goodThe synchronous wrapper mirrors
presenceperfectly and maintains queue ownership viaqueuedDealloc. Nice, consistent API surface.
632-634: Annotation handling path mirrors Presence – solid, but watch out for back-pressure
onAnnotation:copies the same pattern asonPresence:(updates serial, forwards to handler). Looks correct.
Down the line make sure that_realtimeAnnotationsimplements overflow protection akin to presence to avoid emitter overload.Also applies to: 824-831
Source/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.h (1)
14-15:queueproperty is public but never initialised hereIf
ARTRealtimeAnnotationsInternalrelies onqueuebeing non-null, ensure it is assigned ininitWithChannel:logger:; otherwise expose it asreadonlyor remove.Source/include/Ably/ARTAnnotation.h (1)
8-17: Enum raw values start at 0 – check server expectationsIf the wire protocol already defines numeric codes for
create/delete, confirm they match0and1. If they’re1/2, explicitly set the raw values.typedef NS_ENUM(NSUInteger, ARTAnnotationAction) { ARTAnnotationCreate = 1, ARTAnnotationDelete = 2, };Ably.xcodeproj/project.pbxproj (37)
1433-1435: Verify new PBXFileReference entries for annotation sources and headers.
Paths, file types, and sourceTree match the existing project conventions.Also applies to: 1437-1444
1816-1818: Include annotation files in the Wrapper SDK Proxy group.
Public header, private header, and implementation added alongside the Presence proxy entries.
2155-2157: Add REST annotations files to the REST group.
Public header, private header, and implementation are correctly inserted with surrounding REST entries.
2192-2194: Add Realtime annotations files to the Realtime group.
Public header, private header, and implementation entries follow the pattern of Presence files.
2483-2483: Include Realtime annotations private header in main Headers build phase.
Private header entry aligns with other private headers in this phase.
2517-2517: Include Wrapper SDK Proxy annotations public header in Headers build phase.
Public header correctly added to the target’s header phase.
2523-2523: Include annotation model public header in Headers build phase.
ARTAnnotation.his now exposed with other headers.
2570-2570: Include Realtime annotations public header in Headers build phase.
Ensures realtime annotations API is exposed.
2595-2595: Include Wrapper SDK Proxy annotations private header in Headers build phase.
Matches the public header inclusion for proxy annotations.
2597-2597: Include REST annotations public header in Headers build phase.
Makes REST annotations API available.
2637-2637: Include REST annotations private header in Headers build phase.
Keeps private implementation encapsulated.
2696-2696: Include Wrapper SDK Proxy annotations private header in another Headers phase.
Added to a second target’s headers build phase.
2740-2740: Include Wrapper SDK Proxy annotations public header in Headers phase.
Added to a third target’s headers.
2777-2777: Include Realtime annotations public header in Headers build phase.
Inserted into a fourth target’s header phase.
2822-2822: Include Realtime annotations private header in Headers build phase.
Inserted into a fifth target’s header phase.
2849-2849: Include REST annotations private header in Headers build phase.
Added to another target’s headers.
2851-2851: Include REST annotations public header in Headers build phase.
Ensures symmetry with private header.
2877-2877: Include annotation model public header in Headers build phase.
Added to an additional target’s headers.
2898-2898: Include Wrapper SDK Proxy annotations private header in Headers build phase.
Added to yet another target.
2942-2943: Include Wrapper SDK Proxy annotations public header in Headers build phase.
Ensures proxy annotations API is exported.
2979-2979: Include Realtime annotations public header in Headers build phase.
Consistent with its private counterpart.
3024-3024: Include Realtime annotations private header in Headers build phase.
3051-3051: Include REST annotations private header in Headers build phase.
3053-3053: Include REST annotations public header in Headers build phase.
3079-3079: Include annotation model public header in Headers build phase.
3489-3489: Add Realtime annotations implementation to Sources group.
Implementation file is now compiled.
3495-3495: Add annotation model implementation to Sources group.
3525-3525: Add REST annotations implementation to Sources group.
3545-3545: Add Wrapper SDK Proxy annotations implementation to Sources group.
3748-3748: Add Realtime annotations implementation to Sources group.
3755-3755: Add annotation model implementation to Sources group.
3784-3784: Add REST annotations implementation to Sources group.
3804-3804: Add Wrapper SDK Proxy annotations implementation to Sources group.
3887-3887: Add Realtime annotations implementation to Sources group.
3893-3893: Add annotation model implementation to Sources group.
3923-3923: Add REST annotations implementation to Sources group.
3943-3943: Add Wrapper SDK Proxy annotations implementation to Sources group.Source/ARTRealtimeAnnotations.m (1)
114-146: Well-implemented subscription logic.The subscription implementation correctly:
- Checks for the required
ARTChannelModeAnnotationSubscribemode- Handles channel state validation
- Implements proper attachment behavior
- Logs appropriate warnings
This aligns perfectly with the PR objectives to add annotation support specifically for subscriptions.
maratal
force-pushed
the
ECO-5323-annotations
branch
from
2f33baf to
590a5d8
Compare
…it was omiitted, RSC19f mentions it).
maratal
force-pushed
the
ECO-5323-annotations
branch
from
590a5d8 to
813b6b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Source/ARTJsonLikeEncoder.m (1)
298-304: Guard againstNSNull/ non-dictionarysummaryvalues
objectForKey:may returnNSNullwhen the wire value is JSONnull, and nothing prevents a malicious/buggy producer from sending a non-dictionary. Both cases would later surface as type errors when client code treatssummaryas anNSDictionary.- message.summary = [input objectForKey:@"summary"] ?: (message.action == ARTMessageActionMessageSummary ? @{} : nil); // enforce TM2q (https://ably-real-time.slack.com/archives/C03JDBVM5MY/p1749124425224069?thread_ts=1749051904.445159&cid=C03JDBVM5MY) + id rawSummary = input[@"summary"]; + if (rawSummary == (id)[NSNull null]) { + rawSummary = nil; + } + if (rawSummary && ![rawSummary isKindOfClass:[NSDictionary class]]) { + ARTLogError(_logger, @"RS:%p ARTJsonEncoder invalid summary type %@", _rest, [rawSummary class]); + rawSummary = nil; + } + message.summary = rawSummary ?: (message.action == ARTMessageActionMessageSummary ? @{} : nil);
🧹 Nitpick comments (1)
Source/ARTJsonLikeEncoder.m (1)
431-445: Early-exit drops all annotations on single bad elementIf one malformed annotation is encountered the whole array returns
nil, which silently discards the annotation set. An alternative is to skip invalid entries and log, matching the more resilient behaviour ofmessagesFromArray:.Optional refactor:
- if (!annotation) { - return nil; - } - [output addObject:annotation]; + if (!annotation) { + ARTLogError(_logger, @"RS:%p ARTJsonEncoder dropped malformed annotation", _rest); + continue; // keep the rest + } + [output addObject:annotation];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
Ably.xcodeproj/project.pbxproj(32 hunks)Source/ARTAnnotation.m(1 hunks)Source/ARTBaseMessage.m(1 hunks)Source/ARTJsonLikeEncoder.m(5 hunks)Source/ARTMessage.m(3 hunks)Source/ARTProtocolMessage.m(6 hunks)Source/ARTRealtimeAnnotations.m(1 hunks)Source/ARTRealtimeChannel.m(7 hunks)Source/ARTRest.m(1 hunks)Source/ARTRestAnnotations.m(1 hunks)Source/ARTWrapperSDKProxyRealtimeAnnotations.m(1 hunks)Source/ARTWrapperSDKProxyRealtimeChannel.m(2 hunks)Source/Ably.modulemap(2 hunks)Source/PrivateHeaders/Ably/ARTPresence+Private.h(0 hunks)Source/PrivateHeaders/Ably/ARTProtocolMessage.h(3 hunks)Source/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRestAnnotations+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRestPresence+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimeAnnotations+Private.h(1 hunks)Source/include/Ably/ARTAnnotation.h(1 hunks)Source/include/Ably/ARTMessage.h(2 hunks)Source/include/Ably/ARTRealtimeAnnotations.h(1 hunks)Source/include/Ably/ARTRealtimeChannel.h(2 hunks)Source/include/Ably/ARTRealtimeChannelOptions.h(1 hunks)Source/include/Ably/ARTRestAnnotations.h(1 hunks)Source/include/Ably/ARTTypes.h(2 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtimeAnnotations.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h(2 hunks)Source/include/Ably/AblyInternal.h(1 hunks)Source/include/Ably/AblyPublic.h(1 hunks)
💤 Files with no reviewable changes (1)
- Source/PrivateHeaders/Ably/ARTPresence+Private.h
✅ Files skipped from review due to trivial changes (3)
- Source/ARTBaseMessage.m
- Source/include/Ably/ARTRestAnnotations.h
- Ably.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (26)
- Source/include/Ably/AblyPublic.h
- Source/PrivateHeaders/Ably/ARTRestPresence+Private.h
- Source/Ably.modulemap
- Source/ARTRest.m
- Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h
- Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h
- Source/include/Ably/AblyInternal.h
- Source/include/Ably/ARTRealtimeChannelOptions.h
- Source/ARTWrapperSDKProxyRealtimeChannel.m
- Source/include/Ably/ARTWrapperSDKProxyRealtimeAnnotations.h
- Source/PrivateHeaders/Ably/ARTProtocolMessage.h
- Source/ARTMessage.m
- Source/include/Ably/ARTMessage.h
- Source/include/Ably/ARTRealtimeChannel.h
- Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimeAnnotations+Private.h
- Source/PrivateHeaders/Ably/ARTRestAnnotations+Private.h
- Source/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.h
- Source/include/Ably/ARTTypes.h
- Source/include/Ably/ARTAnnotation.h
- Source/ARTRealtimeChannel.m
- Source/ARTAnnotation.m
- Source/include/Ably/ARTRealtimeAnnotations.h
- Source/ARTProtocolMessage.m
- Source/ARTWrapperSDKProxyRealtimeAnnotations.m
- Source/ARTRestAnnotations.m
- Source/ARTRealtimeAnnotations.m
🧰 Additional context used
🧠 Learnings (1)
Source/ARTJsonLikeEncoder.m (1)
Learnt from: maratal
PR: ably/ably-cocoa#2066
File: Source/ARTJsonLikeEncoder.m:341-351
Timestamp: 2025-06-16T15:39:15.987Z
Learning: In the Ably Cocoa SDK, when decoding annotation actions from integers, unknown/invalid action values should default to ARTAnnotationCreate rather than failing or using an unknown state. This is an intentional design decision for the annotation system.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (macOS, test_macOS)
🔇 Additional comments (4)
Source/ARTJsonLikeEncoder.m (4)
8-8: Header import looks correct
ARTAnnotation.his now required for the new decoding helpers – no issues spotted.
341-351: Enum decoding follows agreed fallback
annotationActionFromInt:returnsARTAnnotationCreatefor unknown values, matching the design decision captured in team learnings.
406-429: Double-check numeric fields & null handling inannotationFromDictionary:
messageSerialis parsed as a string, but the protocol generally treats serials as integers. Confirm the type matches the spec; otherwise useartNumber:to avoid a laterNSNumber/NSStringmismatch.datais fetched withobjectForKey:so a JSONnullwill propagate asNSNull. If downstream consumers expectnil, consider mirroring the approach used formessage.data(or add the sameNSNullguard as suggested forsummary).No functional blocker, but worth verifying.
822-823: Ensureannotationsis copied if mutability matters
[input objectForKey:@"annotations"]is immutable, butannotationsFromArray:returns a mutable array. IfARTProtocolMessage.annotationsis exposed as immutable (NSArray) everything is fine; otherwise consider returning an immutable copy to avoid external mutation.
|
@maratal My understanding is that you haven't written tests for annotations here due to time pressure to get the Chat message reactions feature out, is that right? If so, what's your plan for adding tests in the future? |
maratal
force-pushed
the
ECO-5323-annotations
branch
from
e5f7fc1 to
5012150
Compare
maratal
force-pushed
the
ECO-5323-annotations
branch
from
642ac01 to
205ecc2
Compare
maratal
force-pushed
the
ECO-5323-annotations
branch
from
205ecc2 to
e89dc55
Compare
maratal
force-pushed
the
ECO-5323-annotations
branch
from
e89dc55 to
a26f7dd
Compare
maratal
force-pushed
the
ECO-5323-annotations
branch
from
0b48d97 to
208cff8
Compare
This need |
… SDK reactions - ably/ably-chat-swift#293). Reference implementation - ably/ably-js#1953 Docstrings - ably/specification#292 Annotation subscription (both summary and raw) is tested in ably/ably-chat-swift#293 (see integration test).
maratal
force-pushed
the
ECO-5323-annotations
branch
from
8590f70 to
d7c8b45
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Source/ARTAnnotation.m (2)
39-57: Fix inconsistent comma formatting in description method.The description method has inconsistent comma placement - some lines end with commas while others don't, making the output format unpredictable.
- [description appendFormat:@" action: %@\n", ARTAnnotationActionToStr(self.action)]; + [description appendFormat:@" action: %@,\n", ARTAnnotationActionToStr(self.action)]; - [description appendFormat:@" name: %@\n", self.name]; + [description appendFormat:@" name: %@,\n", self.name]; - [description appendFormat:@" count: %@\n", self.count]; + [description appendFormat:@" count: %@,\n", self.count]; - [description appendFormat:@" data: %@\n", self.data]; + [description appendFormat:@" data: %@,\n", self.data]; - [description appendFormat:@" serial: %@\n", self.serial]; + [description appendFormat:@" serial: %@,\n", self.serial]; - [description appendFormat:@" messageSerial: %@\n", self.messageSerial]; + [description appendFormat:@" messageSerial: %@,\n", self.messageSerial]; - [description appendFormat:@" type: %@\n", self.type]; + [description appendFormat:@" type: %@,\n", self.type];
88-98: Consider using a more specific error code for encoding failures.The encode method uses a hardcoded error code of
0which may not be as informative as using a specific error code from the ARTErrorCode enum.- *error = [NSError errorWithDomain:ARTAblyErrorDomain code:0 userInfo:@{NSLocalizedDescriptionKey: @"encoding failed", + *error = [NSError errorWithDomain:ARTAblyErrorDomain code:ARTErrorUnableToEncodeMessage userInfo:@{NSLocalizedDescriptionKey: @"encoding failed",Source/ARTRealtimeAnnotations.m (1)
133-138: Inconsistent logging message in unsubscribe method.The unsubscribe method with a specific listener logs "unsubscribe to all types" which is misleading since it's only removing a specific listener, not all subscriptions.
- ARTLogVerbose(self.logger, @"R:%p C:%p (%@) annotations unsubscribe to all types", self->_realtime, self->_channel, self->_channel.name); + ARTLogVerbose(self.logger, @"R:%p C:%p (%@) annotations unsubscribe listener", self->_realtime, self->_channel, self->_channel.name);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
Ably.xcodeproj/project.pbxproj(28 hunks)Source/ARTAnnotation.m(1 hunks)Source/ARTBaseMessage.m(1 hunks)Source/ARTJsonLikeEncoder.m(5 hunks)Source/ARTMessage.m(3 hunks)Source/ARTProtocolMessage.m(7 hunks)Source/ARTRealtimeAnnotations.m(1 hunks)Source/ARTRealtimeChannel.m(7 hunks)Source/ARTWrapperSDKProxyRealtimeAnnotations.m(1 hunks)Source/ARTWrapperSDKProxyRealtimeChannel.m(2 hunks)Source/Ably.modulemap(2 hunks)Source/PrivateHeaders/Ably/ARTAnnotation+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTProtocolMessage.h(3 hunks)Source/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimeAnnotations+Private.h(1 hunks)Source/include/Ably/ARTAnnotation.h(1 hunks)Source/include/Ably/ARTMessage.h(2 hunks)Source/include/Ably/ARTRealtimeAnnotations.h(1 hunks)Source/include/Ably/ARTRealtimeChannel.h(2 hunks)Source/include/Ably/ARTRealtimeChannelOptions.h(1 hunks)Source/include/Ably/ARTTypes.h(2 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtimeAnnotations.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h(2 hunks)Source/include/Ably/AblyInternal.h(1 hunks)Source/include/Ably/AblyPublic.h(1 hunks)Source/include/module.modulemap(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- Source/PrivateHeaders/Ably/ARTAnnotation+Private.h
- Ably.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (22)
- Source/include/Ably/AblyInternal.h
- Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h
- Source/ARTBaseMessage.m
- Source/include/Ably/AblyPublic.h
- Source/Ably.modulemap
- Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h
- Source/include/module.modulemap
- Source/include/Ably/ARTRealtimeChannelOptions.h
- Source/include/Ably/ARTRealtimeChannel.h
- Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimeAnnotations+Private.h
- Source/ARTMessage.m
- Source/include/Ably/ARTMessage.h
- Source/PrivateHeaders/Ably/ARTProtocolMessage.h
- Source/include/Ably/ARTWrapperSDKProxyRealtimeAnnotations.h
- Source/ARTProtocolMessage.m
- Source/ARTWrapperSDKProxyRealtimeChannel.m
- Source/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.h
- Source/include/Ably/ARTTypes.h
- Source/include/Ably/ARTRealtimeAnnotations.h
- Source/ARTRealtimeChannel.m
- Source/ARTWrapperSDKProxyRealtimeAnnotations.m
- Source/ARTJsonLikeEncoder.m
🧰 Additional context used
🧠 Learnings (1)
Source/include/Ably/ARTAnnotation.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTRealtimeChannelOptions.h:33-33
Timestamp: 2024-10-08T15:37:26.766Z
Learning: `ARTChannelOptions` is already marked with `NS_SWIFT_SENDABLE`.
🔇 Additional comments (9)
Source/ARTAnnotation.m (1)
76-86: Good error handling in decode method.The decode method properly handles encoding errors by creating an NSError with appropriate domain and user info, while preserving the original annotation structure.
Source/include/Ably/ARTAnnotation.h (3)
5-18: Well-designed enum with clear documentation.The
ARTAnnotationActionenum is properly documented and marked asNS_SWIFT_SENDABLE, providing clear semantic meaning for create/delete operations.
25-76: Excellent API design following past feedback.The interface successfully addresses previous feedback by:
- Adding
NS_SWIFT_SENDABLEannotation- Making all properties
readonlyfor immutability- Providing comprehensive memberwise initializer
- Including detailed documentation for each property
80-84: Useful ARTEvent category for type-safe event handling.The category extension provides a clean way to create annotation-specific events, enhancing type safety and readability in the event system.
Source/ARTRealtimeAnnotations.m (5)
18-51: Clean facade pattern implementation.The public
ARTRealtimeAnnotationsclass properly delegates all operations to the internal implementation while maintaining proper memory management withARTQueuedDealloc.
61-80: Proper initialization with weak references.The internal class correctly uses weak references to prevent retain cycles with the owning channel, and properly initializes all required components including event emitter and data encoder.
82-110: Good subscription logic with proper error handling.The subscription method properly handles:
- Channel state validation with appropriate error responses
- Conditional attachment based on channel options
- Thread-safe callback wrapping for user queue dispatch
- Comprehensive logging for debugging
122-127: Missing space in dispatch_sync formatting.The formatting is inconsistent with other dispatch_sync calls in the file.
- dispatch_sync(_queue, ^{ + dispatch_sync(_queue, ^{Actually, looking more closely, the formatting appears consistent. The line spacing is fine.
147-160: Robust message processing with proper error handling.The
onMessagemethod correctly:
- Iterates through annotations in the protocol message
- Conditionally decodes data when encoder is available
- Handles decoding errors with proper logging
- Emits events with appropriate type-specific event objects
Add annotations (in this PR limited to subscribtions only for supporting Chat SDK reactions - ably/ably-chat-swift#293).
Reference implementation - ably/ably-js#1953
Docstrings - ably/specification#292
Annotation subscription (both summary and raw) is tested in ably/ably-chat-swift#293 (see integration test) and can't be tested here due to lack of
publish:...method implementation.Tests failures are addressed in another PR - #2065
Summary by CodeRabbit
New Features
Improvements
Documentation