[ECO-5474] Publish and get annotations #2080
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThis change introduces comprehensive support for message annotations in both REST and Realtime APIs. It adds new public and private headers, implementation files, and test suites for annotation publishing, deletion, retrieval, and idempotency. The Xcode project and module maps are updated to include these files. Extensive tests validate annotation behaviors, including size limits and idempotent operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RealtimeAnnotations
participant RealtimeAnnotationsInternal
participant Channel
participant Server
Client->>RealtimeAnnotations: publishForMessage(message, annotation, callback)
RealtimeAnnotations->>RealtimeAnnotationsInternal: publishForMessage(message, annotation, callback)
RealtimeAnnotationsInternal->>Channel: send annotation via protocol message
Channel->>Server: Send annotation
Server-->>Channel: Acknowledge annotation
Channel-->>RealtimeAnnotationsInternal: Confirmation
RealtimeAnnotationsInternal-->>Client: callback(success/error)
sequenceDiagram
participant Client
participant RestAnnotations
participant RestAnnotationsInternal
participant RestChannel
participant Server
Client->>RestAnnotations: publishForMessageSerial(serial, annotation, callback)
RestAnnotations->>RestAnnotationsInternal: publishForMessageSerial(serial, annotation, callback)
RestAnnotationsInternal->>RestChannel: HTTP POST /annotations
RestChannel->>Server: POST annotation
Server-->>RestChannel: Response
RestChannel-->>RestAnnotationsInternal: Result
RestAnnotationsInternal-->>Client: callback(success/error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope functional code changes detected. Possibly related PRs
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
maratal
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
Source/include/Ably/ARTRestChannel.h (1)
58-62: Consider adding concise API docs for the newannotationspropertyEvery other public surface on
ARTRestChannelcarries at least a one-liner.
A short description such as “Provides annotation publish / fetch / delete operations for this channel” will make generated docs clearer and align with the existing style.Source/include/Ably/ARTRestAnnotations.h (2)
12-22: Consider adding a public convenience initializer.The class only exposes a private initializer marked with
:nodoc:, but users will need a way to create instances. Consider adding a public convenience method or making the existing initializer public.+/** + * Creates a new annotations query with the specified limit. + * + * @param limit An upper limit on the number of annotations returned. + */ ++ (instancetype)queryWithLimit:(NSUInteger)limit; + +/** + * Creates a new annotations query with default limit. + */ ++ (instancetype)query; + -/// :nodoc: +/** + * Designated initializer. + * + * @param limit An upper limit on the number of annotations returned. + */ - (instancetype)initWithLimit:(NSUInteger)limit;
74-75: Fix documentation formatting inconsistency.The comment block starting at line 74 has inconsistent spacing compared to other method documentation blocks.
- /** +/** * Get all annotations for a given message (as a paginated result).Ably.xcodeproj/project.pbxproj (1)
1449-1451: Sanity-check path & group for new REST annotation sourcesThe new file refs for:
include/Ably/ARTRestAnnotations.hPrivateHeaders/Ably/ARTRestAnnotations+Private.hARTRestAnnotations.msit at the root of the project hierarchy. For consistency with existing files (e.g.
ARTRestPresence.*) they usually live underSource/andPrivateHeaders/.No functional impact, but keeping the physical layout parallel improves discoverability.
Source/include/Ably/ARTRealtimeAnnotations.h (1)
56-56: Fix typo in documentation.- * @param callback A callback for retriving an `ARTPaginatedResult` containing annotations. + * @param callback A callback for retrieving an `ARTPaginatedResult` containing annotations.Test/Tests/RealtimeAnnotationsTests.swift (1)
101-101: Replace unused closure parameter with underscore.- realtimeChannel.once(.attached) { stateChange in + realtimeChannel.once(.attached) { _ inTest/Tests/RestAnnotationsTests.swift (1)
106-106: Replace unused closure parameters with underscore.- realtimeChannel.once(.attached) { stateChange in + realtimeChannel.once(.attached) { _ inAlso applies to: 256-256
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
Ably.xcodeproj/project.pbxproj(18 hunks)Source/ARTAnnotation.m(3 hunks)Source/ARTConstants.m(1 hunks)Source/ARTJsonLikeEncoder.m(4 hunks)Source/ARTRealtimeAnnotations.m(5 hunks)Source/ARTRestAnnotations.m(1 hunks)Source/ARTRestChannel.m(7 hunks)Source/ARTWrapperSDKProxyRealtimeAnnotations.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTAnnotation+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTConstants.h(1 hunks)Source/PrivateHeaders/Ably/ARTEncoder.h(2 hunks)Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h(1 hunks)Source/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRestAnnotations+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRestChannel+Private.h(1 hunks)Source/include/Ably/ARTAnnotation.h(1 hunks)Source/include/Ably/ARTRealtimeAnnotations.h(2 hunks)Source/include/Ably/ARTRestAnnotations.h(1 hunks)Source/include/Ably/ARTRestChannel.h(2 hunks)Source/include/Ably/ARTTypes.h(0 hunks)Source/include/Ably/AblyPublic.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/Codables/TestAppSetup.swift(1 hunks)Test/Tests/RealtimeAnnotationsTests.swift(1 hunks)Test/Tests/RestAnnotationsTests.swift(1 hunks)ably-common(1 hunks)
💤 Files with no reviewable changes (1)
- Source/include/Ably/ARTTypes.h
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTBaseQuery.h:4-8
Timestamp: 2024-09-12T20:12:29.428Z
Learning: As a general rule, `NS_SWIFT_SENDABLE` annotations are not needed for private Objective-C interfaces in the ably-cocoa project.
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTBaseQuery.h:4-8
Timestamp: 2024-10-08T15:37:26.766Z
Learning: As a general rule, `NS_SWIFT_SENDABLE` annotations are not needed for private Objective-C interfaces in the ably-cocoa project.
Learnt from: maratal
PR: ably/ably-cocoa#2066
File: Source/ARTJsonLikeEncoder.m:341-351
Timestamp: 2025-06-16T15:39:16.005Z
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.
📚 Learning: in `artrealtimechannelinternal`, the `internalpresence` property is declared in the private header `...
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`.
Applied to files:
Source/PrivateHeaders/Ably/ARTConstants.hSource/include/Ably/AblyPublic.hSource/include/Ably/ARTAnnotation.hSource/ARTConstants.mSource/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.hSource/PrivateHeaders/Ably/ARTRestChannel+Private.hSource/include/Ably/ARTRestChannel.hSource/Ably.modulemapSource/include/module.modulemapSource/PrivateHeaders/Ably/ARTAnnotation+Private.hSource/PrivateHeaders/Ably/ARTEncoder.hSource/ARTRestChannel.mSource/PrivateHeaders/Ably/ARTRestAnnotations+Private.hSource/include/Ably/ARTRestAnnotations.hSource/include/Ably/ARTRealtimeAnnotations.hSource/ARTWrapperSDKProxyRealtimeAnnotations.mSource/ARTRestAnnotations.mSource/ARTRealtimeAnnotations.mAbly.xcodeproj/project.pbxproj
📚 Learning: `artchanneloptions` is already marked with `ns_swift_sendable`....
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`.
Applied to files:
Source/PrivateHeaders/Ably/ARTConstants.hSource/include/Ably/AblyPublic.hSource/include/Ably/ARTAnnotation.hSource/ARTConstants.mSource/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.hSource/PrivateHeaders/Ably/ARTRestChannel+Private.hSource/include/Ably/ARTRestChannel.hSource/Ably.modulemapSource/include/module.modulemapSource/PrivateHeaders/Ably/ARTJsonLikeEncoder.hSource/PrivateHeaders/Ably/ARTAnnotation+Private.hSource/PrivateHeaders/Ably/ARTEncoder.hSource/ARTRestChannel.mSource/PrivateHeaders/Ably/ARTRestAnnotations+Private.hSource/include/Ably/ARTRestAnnotations.hSource/ARTJsonLikeEncoder.mTest/Tests/RealtimeAnnotationsTests.swiftSource/include/Ably/ARTRealtimeAnnotations.hTest/Tests/RestAnnotationsTests.swiftSource/ARTRestAnnotations.mSource/ARTRealtimeAnnotations.mAbly.xcodeproj/project.pbxproj
📚 Learning: in 'source/artjsonlikeencoder.m', when handling messages from 'input', using `mutablecopy` on the ar...
Learnt from: maratal
PR: ably/ably-cocoa#1998
File: Source/ARTJsonLikeEncoder.m:766-774
Timestamp: 2024-11-26T17:50:25.730Z
Learning: In 'Source/ARTJsonLikeEncoder.m', when handling messages from 'input', using `mutableCopy` on the array ensures that modifications to `messages` do not affect the original `input`.
Applied to files:
Test/Codables/TestAppSetup.swiftSource/ARTAnnotation.mSource/PrivateHeaders/Ably/ARTJsonLikeEncoder.hSource/PrivateHeaders/Ably/ARTAnnotation+Private.hSource/PrivateHeaders/Ably/ARTEncoder.hSource/ARTRestChannel.mSource/ARTJsonLikeEncoder.mSource/ARTRestAnnotations.mSource/ARTRealtimeAnnotations.m
📚 Learning: in the swift test files within the `test/tests/` directory of the ably cocoa project, it's acceptabl...
Learnt from: maratal
PR: ably/ably-cocoa#1995
File: Test/Tests/RealtimeClientChannelTests.swift:192-193
Timestamp: 2024-11-04T23:11:06.909Z
Learning: In the Swift test files within the `Test/Tests/` directory of the Ably Cocoa project, it's acceptable to access internal types and properties directly in tests.
Applied to files:
Test/Codables/TestAppSetup.swiftSource/include/Ably/AblyPublic.hSource/PrivateHeaders/Ably/ARTRestChannel+Private.hSource/Ably.modulemapSource/include/module.modulemapSource/PrivateHeaders/Ably/ARTRestAnnotations+Private.hTest/Tests/RealtimeAnnotationsTests.swiftSource/include/Ably/ARTRealtimeAnnotations.hTest/Tests/RestAnnotationsTests.swiftAbly.xcodeproj/project.pbxproj
📚 Learning: parsing the message's `id` is necessary for checking `msgserial` and `index` in the `member:isnewert...
Learnt from: maratal
PR: ably/ably-cocoa#1982
File: Source/ARTPresenceMessage.m:59-59
Timestamp: 2024-10-06T12:27:40.858Z
Learning: Parsing the message's `id` is necessary for checking `msgSerial` and `index` in the `member:isNewerThan:` method.
Applied to files:
Test/Codables/TestAppSetup.swiftSource/ARTRestChannel.m
📚 Learning: as a general rule, `ns_swift_sendable` annotations are not needed for private objective-c interfaces...
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTBaseQuery.h:4-8
Timestamp: 2024-09-12T20:12:29.428Z
Learning: As a general rule, `NS_SWIFT_SENDABLE` annotations are not needed for private Objective-C interfaces in the ably-cocoa project.
Applied to files:
Source/include/Ably/AblyPublic.hSource/include/Ably/ARTAnnotation.hSource/PrivateHeaders/Ably/ARTRestChannel+Private.hSource/include/Ably/ARTRestChannel.hSource/Ably.modulemapSource/include/module.modulemapSource/PrivateHeaders/Ably/ARTJsonLikeEncoder.hSource/PrivateHeaders/Ably/ARTAnnotation+Private.hSource/PrivateHeaders/Ably/ARTEncoder.hSource/ARTRestChannel.mSource/PrivateHeaders/Ably/ARTRestAnnotations+Private.hSource/include/Ably/ARTRestAnnotations.hSource/include/Ably/ARTRealtimeAnnotations.hTest/Tests/RestAnnotationsTests.swiftSource/ARTRealtimeAnnotations.mAbly.xcodeproj/project.pbxproj
📚 Learning: in the ably cocoa sdk, when decoding annotation actions from integers, unknown/invalid action values...
Learnt from: maratal
PR: ably/ably-cocoa#2066
File: Source/ARTJsonLikeEncoder.m:341-351
Timestamp: 2025-06-16T15:39:16.005Z
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.
Applied to files:
Source/include/Ably/AblyPublic.hSource/include/Ably/ARTAnnotation.hSource/PrivateHeaders/Ably/ARTRestChannel+Private.hSource/include/Ably/ARTRestChannel.hSource/ARTAnnotation.mSource/PrivateHeaders/Ably/ARTJsonLikeEncoder.hSource/PrivateHeaders/Ably/ARTAnnotation+Private.hSource/PrivateHeaders/Ably/ARTEncoder.hSource/ARTRestChannel.mSource/PrivateHeaders/Ably/ARTRestAnnotations+Private.hSource/include/Ably/ARTRestAnnotations.hSource/ARTJsonLikeEncoder.mTest/Tests/RealtimeAnnotationsTests.swiftSource/include/Ably/ARTRealtimeAnnotations.hTest/Tests/RestAnnotationsTests.swiftSource/ARTWrapperSDKProxyRealtimeAnnotations.mSource/ARTRestAnnotations.mSource/ARTRealtimeAnnotations.m
📚 Learning: avoid suggesting refactoring the conditional compilation in `restclientchannelstests.swift` for `art...
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Test/Tests/RestClientChannelsTests.swift:5-18
Timestamp: 2024-09-17T13:25:42.060Z
Learning: Avoid suggesting refactoring the conditional compilation in `RestClientChannelsTests.swift` for `ARTRestChannels` extensions, as the user prefers to keep the current structure.
Applied to files:
Source/include/Ably/AblyPublic.hSource/PrivateHeaders/Ably/ARTRestChannel+Private.hSource/include/Ably/ARTRestChannel.hSource/Ably.modulemapSource/include/module.modulemapSource/ARTRestChannel.mSource/PrivateHeaders/Ably/ARTRestAnnotations+Private.hSource/include/Ably/ARTRestAnnotations.hTest/Tests/RealtimeAnnotationsTests.swiftTest/Tests/RestAnnotationsTests.swiftSource/ARTRestAnnotations.mAbly.xcodeproj/project.pbxproj
📚 Learning: in `artmessage`, the `action` and `serial` fields are set by the realtime service and should not be ...
Learnt from: maratal
PR: ably/ably-cocoa#1998
File: Source/ARTJsonLikeEncoder.m:278-279
Timestamp: 2024-11-22T10:11:05.305Z
Learning: In `ARTMessage`, the `action` and `serial` fields are set by the realtime service and should not be serialized in the `messageToDictionary:` method in `Source/ARTJsonLikeEncoder.m`.
Applied to files:
Source/include/Ably/ARTAnnotation.hSource/ARTConstants.mSource/ARTAnnotation.mSource/PrivateHeaders/Ably/ARTJsonLikeEncoder.hSource/PrivateHeaders/Ably/ARTAnnotation+Private.hSource/PrivateHeaders/Ably/ARTEncoder.hSource/ARTRestChannel.mSource/ARTJsonLikeEncoder.mSource/include/Ably/ARTRealtimeAnnotations.hSource/ARTWrapperSDKProxyRealtimeAnnotations.mSource/ARTRestAnnotations.mSource/ARTRealtimeAnnotations.m
📚 Learning: the unpublish method in artrealtimeannotations works by publishing an annotation with a delete actio...
Learnt from: maratal
PR: ably/ably-cocoa#2066
File: Source/include/Ably/ARTRealtimeAnnotations.h:25-28
Timestamp: 2025-06-16T14:18:14.569Z
Learning: The unpublish method in ARTRealtimeAnnotations works by publishing an annotation with a DELETE action, rather than directly deleting. This is why the documentation correctly refers to "The annotation to publish" even though it's an unpublish method.
Applied to files:
Test/Tests/RealtimeAnnotationsTests.swiftSource/include/Ably/ARTRealtimeAnnotations.hTest/Tests/RestAnnotationsTests.swiftSource/ARTWrapperSDKProxyRealtimeAnnotations.mSource/ARTRestAnnotations.mSource/ARTRealtimeAnnotations.m
📚 Learning: in xcode project files, multiple pbxbuildfile entries for the same source or header file are valid i...
Learnt from: maratal
PR: ably/ably-cocoa#2066
File: Ably.xcodeproj/project.pbxproj:416-424
Timestamp: 2025-06-16T14:12:36.705Z
Learning: In Xcode project files, multiple PBXBuildFile entries for the same source or header file are valid if they are associated with different build targets.
Applied to files:
Ably.xcodeproj/project.pbxproj
🪛 SwiftLint (0.57.0)
Test/Tests/RealtimeAnnotationsTests.swift
[Warning] 101-101: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
Test/Tests/RestAnnotationsTests.swift
[Warning] 106-106: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 256-256: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (iOS, test_iOS18_4)
🔇 Additional comments (41)
ably-common (1)
1-1: Submodule advance verified for annotation APIsCommit 3fe9352 ("Add rest annotations. Add publish method for realtime annotations. Add test for annotations") contains the expected spec updates and accompanying tests, with no unintended breaking changes detected.
Test/Codables/TestAppSetup.swift (1)
46-46: LGTM! Property addition follows established patterns.The new
mutableMessagesoptional Boolean property is consistent with the existing optional Boolean properties in theNamespacestruct and supports the test configuration needs for the annotations feature.Source/ARTConstants.m (1)
5-5: LGTM! Good centralization of the constant.The new
ARTIdempotentLibraryGeneratedIdLengthconstant centralizes what was previously a local definition, improving maintainability. The descriptive name and comment clarifying the unit (bytes) make the purpose clear.Source/PrivateHeaders/Ably/ARTConstants.h (1)
5-5: LGTM! Proper extern declaration for the new constant.The extern declaration correctly exposes the
ARTIdempotentLibraryGeneratedIdLengthconstant for internal SDK use, following the established pattern of the existing constant declarations.Source/include/Ably/ARTAnnotation.h (1)
77-78: LGTM! Important utility method for size validation.The new
annotationSizemethod provides necessary size calculation functionality for annotation publishing workflows. The:nodoc:annotation correctly marks it as internal-only, and the method name clearly conveys its purpose.Source/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.h (1)
9-9: LGTM! Proper weak reference with clear documentation.The new
realtimeproperty provides necessary back-reference to the owning realtime instance. The weak reference correctly prevents retain cycles, and the comment clearly explains the ownership relationship. This enables internal coordination between the annotations and realtime components.Source/include/module.modulemap (1)
49-50: Addition looks correct – keep both module maps in sync
ARTRestAnnotations+Private.his now exposed to SPM consumers viaAbly.Private.
The header is placed next to the other annotation headers and the alphabetical order is preserved, so no issues here.Source/include/Ably/AblyPublic.h (1)
24-25: Public umbrella updated correctlyAdding
#import <Ably/ARTRestAnnotations.h>surfaces the new REST-side annotations API to SDK users and keeps the umbrella header in step with the newly-exposed class. All good.Source/Ably.modulemap (1)
49-50: Xcode module map kept in lock-step with the SPM map
ARTRestAnnotations+Private.his now available when clients useimport Ably.Privateinside Xcode builds.
The update mirrors the change in the SPM module map, avoiding the mismatches we have hit in the past.Source/include/Ably/ARTRestChannel.h (1)
7-7: Forward declaration added – OKThe new forward declaration for
ARTRestAnnotationsprevents an extra public import in this header. Looks good.Source/PrivateHeaders/Ably/ARTRestChannel+Private.h (1)
15-22: Internal plumbing in place – matches the public API
ARTRestAnnotationsInternalis wired in exactly the same pattern aspresenceandpush, so the internal/external surfaces stay consistent. No concerns.Source/PrivateHeaders/Ably/ARTAnnotation+Private.h (1)
11-12: LGTM! Method renaming improves clarity.The addition of "Data" to the method names (
decodeDataWithEncoder:error:andencodeDataWithEncoder:error:) provides better clarity about what these methods encode/decode, distinguishing them from other encoding operations in the annotation system.Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h (1)
36-37: LGTM! Consistent API design for annotation support.The new annotation encoding/decoding methods follow the established patterns for messages and presence messages, maintaining consistency in the encoder interface. The method names are clear and the placement in the private interface is appropriate.
Also applies to: 45-46
Source/ARTRestChannel.m (4)
21-23: LGTM! Proper imports for annotation support.The addition of imports for
ARTRestAnnotations,ARTRestAnnotations+Private.h, andARTConstants.hcorrectly supports the new annotation functionality being integrated.
42-44: LGTM! Consistent pattern for exposing internal functionality.The
annotationsgetter method follows the same pattern as the existingpresenceandpushgetters, maintaining consistency in the API design.
120-120: LGTM! Clean integration of annotations into the channel.The addition of the
_annotationsproperty and its eager initialization in the constructor, along with the simple getter method, cleanly integrates annotation support into the REST channel. The eager initialization differs from the lazy initialization of presence, which is appropriate based on expected annotation usage patterns.Also applies to: 133-133, 151-153
309-309: LGTM! Replaced magic number with named constant.The replacement of the hardcoded value with
ARTIdempotentLibraryGeneratedIdLengthimproves code maintainability and consistency across the codebase.Also applies to: 333-333
Source/PrivateHeaders/Ably/ARTRestAnnotations+Private.h (1)
1-24: LGTM! Well-structured private header following project conventions.The private header follows established patterns in the codebase:
- Proper forward declarations for dependencies
- Clean separation between internal implementation and wrapper classes
- Consistent naming conventions
- Appropriate use of NS_ASSUME_NONNULL_BEGIN/END blocks
The structure aligns with other private headers like
ARTRestPresence+Private.hand maintains the project's architectural consistency.Source/PrivateHeaders/Ably/ARTEncoder.h (1)
7-7: LGTM! Consistent protocol extension for annotation support.The additions to the
ARTEncoderprotocol properly extend the encoding infrastructure:
- Forward declaration for
ARTAnnotationis correctly placed- New encoding/decoding methods follow the established pattern for messages and presence messages
- Method signatures are consistent with existing conventions
- Logical grouping maintains protocol organization
The protocol extension enables proper annotation serialization across the SDK.
Also applies to: 52-55, 60-63
Source/include/Ably/ARTRestAnnotations.h (1)
85-92: LGTM!The class declaration is well-structured with appropriate Swift sendable annotation and clear documentation referencing the protocol for implementation details.
Source/ARTAnnotation.m (4)
76-86: LGTM!The method rename from
decodeWithEncodertodecodeDataWithEncoderimproves clarity about the operation's purpose. The implementation logic and error handling remain correct.
88-98: LGTM!The method rename to
encodeDataWithEncoderis consistent with the decode method and improves API clarity. The implementation and error handling are correct.
133-135: LGTM!The
isIdEmptymethod correctly handles both nil and empty string cases with a clear and simple implementation.
100-131: Improve null safety and error handling.The size calculation logic is comprehensive, but there are several potential null pointer issues and silent failures:
self.nameandself.clientIdcould be nil, causing crashes when callinglengthOfBytesUsingEncoding:[self.extras toJSONString]could return nil- JSON serialization errors are silently ignored, potentially underestimating size
- (NSInteger)annotationSize { // TO3l8* NSInteger finalResult = 0; - finalResult += [self.name lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; - finalResult += [[self.extras toJSONString] lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; - finalResult += [self.clientId lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; + if (self.name) { + finalResult += [self.name lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; + } + NSString *extrasJSON = [self.extras toJSONString]; + if (extrasJSON) { + finalResult += [extrasJSON lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; + } + if (self.clientId) { + finalResult += [self.clientId lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; + } if (self.data) { if ([self.data isKindOfClass:[NSString class]]) { finalResult += [self.data lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; } else if ([self.data isKindOfClass:[NSData class]]) { finalResult += [self.data length]; } else { NSError *error = nil; NSJSONWritingOptions options; if (@available(macOS 10.15, iOS 13.0, tvOS 13.0, *)) { options = NSJSONWritingWithoutEscapingSlashes; } else { options = 0; //no specific format } NSData *jsonData = [NSJSONSerialization dataWithJSONObject:self.data options:options error:&error]; - if (!error) { + if (!error && jsonData) { finalResult += [jsonData length]; + } else { + // Log warning about failed JSON serialization + ARTLogWarn(@"Failed to serialize annotation data for size calculation: %@", error.localizedDescription); } } } return finalResult; }⛔ Skipped due to learnings
Learnt from: maratal PR: ably/ably-cocoa#2066 File: Source/ARTJsonLikeEncoder.m:341-351 Timestamp: 2025-06-16T15:39:16.005Z 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.Ably.xcodeproj/project.pbxproj (3)
401-406: Verify test-file membership – 3 build-file entries per test source could be overkill
RealtimeAnnotationsTests.swiftandRestAnnotationsTests.swifteach appear three times in the PBXBuildFile section.
Duplicating a file across different test targets (iOS / tvOS / macOS) is fine, but duplicating within the same target will trigger duplicate symbol / “file listed twice” linker warnings.Please double-check the target membership in the Build Phases ▸ Compile Sources pages for every test bundle to ensure each file is present once per target.
If any duplicates are in the same target, drop the extra reference(s).
437-446: Public/Private header flags look correct – confirm they land in the right header phase
ARTRestAnnotations.his markedPublic, whileARTRestAnnotations+Private.hisPrivate, which matches the existing header-visibility conventions.
Just make sure the Public header is added to the “Public Headers” phase of all framework targets (Ably iOS / macOS / tvOS) so that CocoaPods / SwiftPM consumers can see it.
3545-3545: Triple inclusion ofARTRestAnnotations.m– verify per-target duplication only
ARTRestAnnotations.mshows up in three separate Compile Sources lists. If those map to the iOS, macOS and tvOS framework targets respectively, all good. If they map to the same target, a duplicate-object build failure will occur.Run an Xcode build for each platform to be safe.
Also applies to: 3808-3808, 3947-3947
Source/include/Ably/ARTRealtimeAnnotations.h (1)
15-68: Well-designed API for annotation operations.The new methods provide a comprehensive interface for annotation operations with good separation between message-based and serial-based variants. The consistent pattern across publish, delete, and get operations makes the API intuitive to use.
Test/Tests/RealtimeAnnotationsTests.swift (2)
128-128: Temporary sleep could cause test flakiness.The hardcoded
sleep(1)could make tests unreliable. I see this is documented as a temporary measure pending CHA-887. Consider using a more robust synchronization mechanism or at least wrapping this in a retry logic to reduce flakiness.
10-152: Comprehensive test coverage for annotation lifecycle.The test thoroughly validates the annotation publish, delete, and retrieval flow with proper verification of annotation properties and message summaries. Good use of split done pattern for managing multiple async operations.
Source/ARTWrapperSDKProxyRealtimeAnnotations.m (1)
35-58: Correct proxy implementation for annotation methods.All six new methods correctly forward calls to the underlying ARTRealtimeAnnotations instance, maintaining consistency with the existing proxy pattern.
Source/ARTJsonLikeEncoder.m (1)
87-109: Comprehensive annotation encoding/decoding implementation.The implementation correctly handles:
- Single annotation and array encoding/decoding
- Proper action value conversion with intentional default to ARTAnnotationCreate
- All annotation properties in serialization
- Integration with protocol message encoding/decoding
The code follows established patterns and properly handles edge cases.
Also applies to: 357-367, 422-457, 493-535, 564-573, 634-636, 893-893
Test/Tests/RestAnnotationsTests.swift (2)
173-303: Excellent test for idempotent annotation publishing.This test thoroughly validates that annotations with implicit IDs are only published once when idempotent publishing is enabled. The verification that duplicate publish attempts succeed without creating duplicate annotations is particularly valuable.
305-343: Good edge case testing for message size limits.The test properly validates that annotations exceeding maxMessageSize are rejected with the correct error code (40009). Using
ARTDefault.maxMessageSize()ensures the test adapts to configuration changes.Source/ARTRestAnnotations.m (3)
19-45: LGTM!The
ARTAnnotationsQueryimplementation correctly handles limit validation and defaults.
47-84: LGTM!The
ARTRestAnnotationswrapper correctly delegates all operations to the internal implementation.
149-213: Potential crash due to weak reference in async blockThe weak
_channelreference could become nil during async execution, potentially causing crashes when accessing channel properties. Consider capturing a strong reference or checking for nil.-dispatch_async(_queue, ^{ + ARTRestChannelInternal *channel = _channel; + if (!channel) { + if (callback) { + callback([ARTErrorInfo createWithCode:ARTErrorConnectionFailed message:@"Channel has been deallocated"]); + } + return; + } + +dispatch_async(_queue, ^{ + // Re-check channel availability inside the block + if (!channel) { + if (callback) { + callback([ARTErrorInfo createWithCode:ARTErrorConnectionFailed message:@"Channel has been deallocated"]); + } + return; + } + // RSAN1c3: encode annotation data NSError *encodeError = nil; - ARTAnnotation *annotationToPublish = self->_dataEncoder ? [annotation encodeDataWithEncoder:self->_dataEncoder error:&encodeError] : annotation; + ARTAnnotation *annotationToPublish = self->_dataEncoder ? [annotation encodeDataWithEncoder:self->_dataEncoder error:&encodeError] : annotation;Then use
channelinstead ofself->_channelthroughout the block.⛔ Skipped due to learnings
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`.Learnt from: maratal PR: ably/ably-cocoa#1973 File: Test/Tests/RestClientChannelsTests.swift:5-18 Timestamp: 2024-09-17T13:25:42.060Z Learning: Avoid suggesting refactoring the conditional compilation in `RestClientChannelsTests.swift` for `ARTRestChannels` extensions, as the user prefers to keep the current structure.Learnt from: maratal PR: ably/ably-cocoa#1973 File: Test/Tests/RestClientChannelsTests.swift:5-18 Timestamp: 2024-10-08T15:37:26.765Z Learning: Avoid suggesting refactoring the conditional compilation in `RestClientChannelsTests.swift` for `ARTRestChannels` extensions, as the user prefers to keep the current structure.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`.Learnt from: maratal PR: ably/ably-cocoa#1998 File: Source/ARTJsonLikeEncoder.m:278-279 Timestamp: 2024-11-22T10:11:05.305Z Learning: In `ARTMessage`, the `action` and `serial` fields are set by the realtime service and should not be serialized in the `messageToDictionary:` method in `Source/ARTJsonLikeEncoder.m`.Learnt from: maratal PR: ably/ably-cocoa#2066 File: Source/ARTJsonLikeEncoder.m:341-351 Timestamp: 2025-06-16T15:39:16.005Z 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.Learnt from: maratal PR: ably/ably-cocoa#1973 File: Source/include/Ably/ARTBaseQuery.h:4-8 Timestamp: 2024-09-12T20:12:29.428Z Learning: As a general rule, `NS_SWIFT_SENDABLE` annotations are not needed for private Objective-C interfaces in the ably-cocoa project.Learnt from: maratal PR: ably/ably-cocoa#1973 File: Source/include/Ably/ARTBaseQuery.h:4-8 Timestamp: 2024-10-08T15:37:26.766Z Learning: As a general rule, `NS_SWIFT_SENDABLE` annotations are not needed for private Objective-C interfaces in the ably-cocoa project.Learnt from: maratal PR: ably/ably-cocoa#1998 File: Source/ARTJsonLikeEncoder.m:766-774 Timestamp: 2024-11-26T17:50:25.730Z Learning: In 'Source/ARTJsonLikeEncoder.m', when handling messages from 'input', using `mutableCopy` on the array ensures that modifications to `messages` do not affect the original `input`.Learnt from: maratal PR: ably/ably-cocoa#2066 File: Source/include/Ably/ARTRealtimeAnnotations.h:25-28 Timestamp: 2025-06-16T14:18:14.569Z Learning: The unpublish method in ARTRealtimeAnnotations works by publishing an annotation with a DELETE action, rather than directly deleting. This is why the documentation correctly refers to "The annotation to publish" even though it's an unpublish method.Source/ARTRealtimeAnnotations.m (4)
32-54: LGTM!The new forwarding methods in
ARTRealtimeAnnotationscorrectly delegate to the internal implementation, maintaining consistency with the existing pattern.
107-191: Well-structured implementation with proper state handlingThe
publishAnnotationmethod correctly handles connection state validation, annotation encoding, size limits, and channel state transitions. The use ofdispatch_syncis appropriate for state checking.
192-221: LGTM! Clean refactoringThe
_subscribemethod has been refactored for better readability while maintaining the same functionality. The state checking and attachOnSubscribe handling remain correct.
287-287: LGTM!The method call update to
decodeDataWithEncoder:error:is consistent with the renamed API.
maratal
force-pushed
the
feature/2077-annotations
branch
from
3fe9352 to
966de7f
Compare
|
@coderabbitai pause |
There was a problem hiding this comment.
Pull Request Overview
This PR implements support for message annotations in both REST and Realtime APIs, enabling users to publish, delete, and retrieve annotations (such as reactions) on individual messages. The implementation adds new query options for fetching annotations with pagination and ensures proper error handling for annotations exceeding maximum message size.
- Adds
ARTRestAnnotationsandARTRealtimeAnnotationsclasses with comprehensive methods for annotation management - Implements annotation size validation against
maxMessageSizewith appropriate error codes - Adds comprehensive test coverage for both REST and realtime annotation operations including idempotency and error conditions
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
Source/include/Ably/ARTRestAnnotations.h |
Public header defining REST annotations API with query parameters and protocol methods |
Source/ARTRestAnnotations.m |
Implementation of REST annotations functionality with HTTP requests and validation |
Source/ARTRealtimeAnnotations.m |
Implementation of realtime annotations with protocol message handling |
Test/Tests/RestAnnotationsTests.swift |
Comprehensive tests for REST annotation operations |
Test/Tests/RealtimeAnnotationsTests.swift |
Comprehensive tests for realtime annotation operations |
| Various header/project files | Module map updates and public API exposure |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
Source/ARTRestAnnotations.m (3)
224-226: Add nil check for message parameterThe method should validate that the message parameter is not nil before accessing its serial property.
- (void)getForMessage:(ARTMessage *)message query:(ARTAnnotationsQuery *)query callback:(ARTPaginatedAnnotationsCallback)callback { + if (!message || !message.serial) { + if (callback) { + dispatch_async(_userQueue, ^{ + callback(nil, [ARTErrorInfo createWithCode:ARTErrorInvalidParams message:@"Message or message serial cannot be nil"]); + }); + } + return; + } [self getForMessageSerial:message.serial query:query callback:callback]; }
286-288: Add nil check for message parameter in publishForMessageThe method should validate that the message parameter is not nil before accessing its serial property.
- (void)publishForMessage:(ARTMessage *)message annotation:(ARTAnnotation *)annotation callback:(ARTCallback)callback { + if (!message || !message.serial) { + if (callback) { + dispatch_async(_userQueue, ^{ + callback([ARTErrorInfo createWithCode:ARTErrorInvalidParams message:@"Message or message serial cannot be nil"]); + }); + } + return; + } [self publishAnnotation:annotation messageSerial:message.serial action:ARTAnnotationCreate callback:callback]; }
294-296: Add nil check for message parameter in deleteForMessageThe method should validate that the message parameter is not nil before accessing its serial property.
- (void)deleteForMessage:(ARTMessage *)message annotation:(ARTAnnotation *)annotation callback:(ARTCallback)callback { + if (!message || !message.serial) { + if (callback) { + dispatch_async(_userQueue, ^{ + callback([ARTErrorInfo createWithCode:ARTErrorInvalidParams message:@"Message or message serial cannot be nil"]); + }); + } + return; + } [self publishAnnotation:annotation messageSerial:message.serial action:ARTAnnotationDelete callback:callback]; }
🧹 Nitpick comments (3)
Test/Tests/RealtimeAnnotationsTests.swift (1)
101-101: Replace unused closure parameter with underscoreThe
stateChangeparameter is not used in the closure.-realtimeChannel.once(.attached) { stateChange in +realtimeChannel.once(.attached) { _ inTest/Tests/RestAnnotationsTests.swift (2)
106-106: Replace unused closure parameter with underscoreThe
stateChangeparameter is not used in the closure.-realtimeChannel.once(.attached) { stateChange in +realtimeChannel.once(.attached) { _ in
256-256: Replace unused closure parameter with underscoreThe
stateChangeparameter is not used in the closure.-realtimeChannel.once(.attached) { stateChange in +realtimeChannel.once(.attached) { _ in
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
Ably.xcodeproj/project.pbxproj(18 hunks)Source/ARTAnnotation.m(3 hunks)Source/ARTConstants.m(1 hunks)Source/ARTJsonLikeEncoder.m(4 hunks)Source/ARTRealtimeAnnotations.m(5 hunks)Source/ARTRestAnnotations.m(1 hunks)Source/ARTRestChannel.m(7 hunks)Source/ARTWrapperSDKProxyRealtimeAnnotations.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTAnnotation+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTConstants.h(1 hunks)Source/PrivateHeaders/Ably/ARTEncoder.h(2 hunks)Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h(1 hunks)Source/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRestAnnotations+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRestChannel+Private.h(1 hunks)Source/include/Ably/ARTAnnotation.h(1 hunks)Source/include/Ably/ARTRealtimeAnnotations.h(2 hunks)Source/include/Ably/ARTRestAnnotations.h(1 hunks)Source/include/Ably/ARTRestChannel.h(2 hunks)Source/include/Ably/ARTTypes.h(0 hunks)Source/include/Ably/AblyPublic.h(1 hunks)Source/include/module.modulemap(1 hunks)Test/Codables/TestAppSetup.swift(1 hunks)Test/Tests/RealtimeAnnotationsTests.swift(1 hunks)Test/Tests/RestAnnotationsTests.swift(1 hunks)
💤 Files with no reviewable changes (1)
- Source/include/Ably/ARTTypes.h
✅ Files skipped from review due to trivial changes (5)
- Source/include/module.modulemap
- Source/Ably.modulemap
- Source/include/Ably/AblyPublic.h
- Source/ARTWrapperSDKProxyRealtimeAnnotations.m
- Ably.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (17)
- Test/Codables/TestAppSetup.swift
- Source/PrivateHeaders/Ably/ARTConstants.h
- Source/ARTConstants.m
- Source/PrivateHeaders/Ably/ARTRealtimeAnnotations+Private.h
- Source/include/Ably/ARTAnnotation.h
- Source/PrivateHeaders/Ably/ARTRestChannel+Private.h
- Source/include/Ably/ARTRestChannel.h
- Source/ARTRestChannel.m
- Source/PrivateHeaders/Ably/ARTRestAnnotations+Private.h
- Source/PrivateHeaders/Ably/ARTAnnotation+Private.h
- Source/PrivateHeaders/Ably/ARTJsonLikeEncoder.h
- Source/PrivateHeaders/Ably/ARTEncoder.h
- Source/include/Ably/ARTRestAnnotations.h
- Source/ARTAnnotation.m
- Source/include/Ably/ARTRealtimeAnnotations.h
- Source/ARTJsonLikeEncoder.m
- Source/ARTRealtimeAnnotations.m
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTBaseQuery.h:4-8
Timestamp: 2024-09-12T20:12:29.428Z
Learning: As a general rule, `NS_SWIFT_SENDABLE` annotations are not needed for private Objective-C interfaces in the ably-cocoa project.
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTBaseQuery.h:4-8
Timestamp: 2024-10-08T15:37:26.766Z
Learning: As a general rule, `NS_SWIFT_SENDABLE` annotations are not needed for private Objective-C interfaces in the ably-cocoa project.
Learnt from: maratal
PR: ably/ably-cocoa#2066
File: Source/ARTJsonLikeEncoder.m:341-351
Timestamp: 2025-06-16T15:39:16.005Z
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.
📚 Learning: in the swift test files within the `test/tests/` directory of the ably cocoa project, it's acceptabl...
Learnt from: maratal
PR: ably/ably-cocoa#1995
File: Test/Tests/RealtimeClientChannelTests.swift:192-193
Timestamp: 2024-11-04T23:11:06.909Z
Learning: In the Swift test files within the `Test/Tests/` directory of the Ably Cocoa project, it's acceptable to access internal types and properties directly in tests.
Applied to files:
Test/Tests/RealtimeAnnotationsTests.swiftTest/Tests/RestAnnotationsTests.swift
📚 Learning: avoid suggesting refactoring the conditional compilation in `restclientchannelstests.swift` for `art...
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Test/Tests/RestClientChannelsTests.swift:5-18
Timestamp: 2024-09-17T13:25:42.060Z
Learning: Avoid suggesting refactoring the conditional compilation in `RestClientChannelsTests.swift` for `ARTRestChannels` extensions, as the user prefers to keep the current structure.
Applied to files:
Test/Tests/RealtimeAnnotationsTests.swiftSource/ARTRestAnnotations.mTest/Tests/RestAnnotationsTests.swift
📚 Learning: the unpublish method in artrealtimeannotations works by publishing an annotation with a delete actio...
Learnt from: maratal
PR: ably/ably-cocoa#2066
File: Source/include/Ably/ARTRealtimeAnnotations.h:25-28
Timestamp: 2025-06-16T14:18:14.569Z
Learning: The unpublish method in ARTRealtimeAnnotations works by publishing an annotation with a DELETE action, rather than directly deleting. This is why the documentation correctly refers to "The annotation to publish" even though it's an unpublish method.
Applied to files:
Test/Tests/RealtimeAnnotationsTests.swiftSource/ARTRestAnnotations.mTest/Tests/RestAnnotationsTests.swift
📚 Learning: `artchanneloptions` is already marked with `ns_swift_sendable`....
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`.
Applied to files:
Test/Tests/RealtimeAnnotationsTests.swiftSource/ARTRestAnnotations.mTest/Tests/RestAnnotationsTests.swift
📚 Learning: in the ably cocoa sdk, when decoding annotation actions from integers, unknown/invalid action values...
Learnt from: maratal
PR: ably/ably-cocoa#2066
File: Source/ARTJsonLikeEncoder.m:341-351
Timestamp: 2025-06-16T15:39:16.005Z
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.
Applied to files:
Test/Tests/RealtimeAnnotationsTests.swiftSource/ARTRestAnnotations.mTest/Tests/RestAnnotationsTests.swift
📚 Learning: in `artmessage`, the `action` and `serial` fields are set by the realtime service and should not be ...
Learnt from: maratal
PR: ably/ably-cocoa#1998
File: Source/ARTJsonLikeEncoder.m:278-279
Timestamp: 2024-11-22T10:11:05.305Z
Learning: In `ARTMessage`, the `action` and `serial` fields are set by the realtime service and should not be serialized in the `messageToDictionary:` method in `Source/ARTJsonLikeEncoder.m`.
Applied to files:
Source/ARTRestAnnotations.m
📚 Learning: in `artrealtimechannelinternal`, the `internalpresence` property is declared in the private header `...
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`.
Applied to files:
Source/ARTRestAnnotations.m
📚 Learning: in 'source/artjsonlikeencoder.m', when handling messages from 'input', using `mutablecopy` on the ar...
Learnt from: maratal
PR: ably/ably-cocoa#1998
File: Source/ARTJsonLikeEncoder.m:766-774
Timestamp: 2024-11-26T17:50:25.730Z
Learning: In 'Source/ARTJsonLikeEncoder.m', when handling messages from 'input', using `mutableCopy` on the array ensures that modifications to `messages` do not affect the original `input`.
Applied to files:
Source/ARTRestAnnotations.m
📚 Learning: parsing the message's `id` is necessary for checking `msgserial` and `index` in the `member:isnewert...
Learnt from: maratal
PR: ably/ably-cocoa#1982
File: Source/ARTPresenceMessage.m:59-59
Timestamp: 2024-10-06T12:27:40.858Z
Learning: Parsing the message's `id` is necessary for checking `msgSerial` and `index` in the `member:isNewerThan:` method.
Applied to files:
Source/ARTRestAnnotations.m
📚 Learning: as a general rule, `ns_swift_sendable` annotations are not needed for private objective-c interfaces...
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTBaseQuery.h:4-8
Timestamp: 2024-09-12T20:12:29.428Z
Learning: As a general rule, `NS_SWIFT_SENDABLE` annotations are not needed for private Objective-C interfaces in the ably-cocoa project.
Applied to files:
Test/Tests/RestAnnotationsTests.swift
🪛 SwiftLint (0.57.0)
Test/Tests/RealtimeAnnotationsTests.swift
[Warning] 101-101: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
Test/Tests/RestAnnotationsTests.swift
[Warning] 106-106: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 256-256: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
⏰ 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). (4)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check
- GitHub Check: build
✅ Actions performedReviews paused. |
maratal
force-pushed
the
feature/2077-annotations
branch
from
966de7f to
62f61a8
Compare
maratal
force-pushed
the
feature/2077-annotations
branch
from
a6b3107 to
3f0ce6a
Compare
maratal
force-pushed
the
feature/2077-annotations
branch
from
3f0ce6a to
953996e
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2080/features
github-actions
bot
temporarily deployed
to
staging/pull/2080/jazzydoc
maratal
force-pushed
the
feature/2077-annotations
branch
from
953996e to
1512bda
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2080/features
github-actions
bot
temporarily deployed
to
staging/pull/2080/jazzydoc
maratal
force-pushed
the
feature/2077-annotations
branch
from
1512bda to
feb0394
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2080/features
github-actions
bot
temporarily deployed
to
staging/pull/2080/jazzydoc
maratal
force-pushed
the
feature/2077-annotations
branch
from
feb0394 to
a54d445
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2080/features
github-actions
bot
temporarily deployed
to
staging/pull/2080/jazzydoc
- Rest annotations - Publishing realtime annotations - Tests
maratal
force-pushed
the
feature/2077-annotations
branch
from
a54d445 to
f308a8c
Compare
| let protocolMessage = ARTProtocolMessage() | ||
| protocolMessage.action = ARTProtocolMessageAction.attached | ||
| protocolMessage.channel = "test" | ||
| protocolMessage.flags = Int64(ARTProtocolMessageFlag.hasBacklog.rawValue | ARTProtocolMessageFlag.resumed.rawValue | ARTChannelMode.publish.rawValue | ARTChannelMode.subscribe.rawValue) // adding flags which are not modes |
There was a problem hiding this comment.
Can you make it clearer here which are and aren't modes?
| // Then: The modes property should be set from the protocol message and contain only channel modes flags | ||
| let expectedModes = ARTChannelMode.publish.rawValue | ARTChannelMode.subscribe.rawValue | ||
| XCTAssertEqual(channel.modes, ARTChannelMode(rawValue: expectedModes)) | ||
| XCTAssertNotEqual(channel.modes, ARTChannelMode(rawValue: UInt(protocolMessage.flags))) |
There was a problem hiding this comment.
Why assert channel.modes is not equal to something when on the previous line you already asserted its exact value?
|
|
||
| // Then: The modes property should be set from the protocol message and contain only channel modes flags | ||
| let expectedModes = ARTChannelMode.publish.rawValue | ARTChannelMode.subscribe.rawValue | ||
| XCTAssertEqual(channel.modes, ARTChannelMode(rawValue: expectedModes)) |
There was a problem hiding this comment.
Can't this just be XCTAssertEqual(channel.modes, [.publish, .subscribe])?
| @@ -184,6 +184,14 @@ - (ARTConnectionDetails *)getConnectionDetails { | |||
| return _connectionDetails; | |||
| } | |||
|
|
|||
| + (ARTChannelMode)allChannelModes { | |||
| return ARTChannelModePresence | ARTChannelModePublish | ARTChannelModeSubscribe | ARTChannelModePresenceSubscribe | ARTChannelModeAnnotationPublish | ARTChannelModeAnnotationSubscribe; | |||
There was a problem hiding this comment.
I worry about the maintainability of this. It seems very likely we'll forget to update it when we add new modes. We either need some good internal documentation or could we just chop off the bottom 4 bits per https://sdk.ably.com/builds/ably/specification/main/features/#TB2d?
| * @param annotation The annotation to publish. (Must include at least the `type`. Assumed to be an annotation.create if no action is specified) | ||
| * @param callback A success or failure callback function. | ||
| */ | ||
| - (void)publishForMessage:(ARTMessage *)message annotation:(ARTAnnotation *)annotation callback:(nullable ARTCallback)callback; |
There was a problem hiding this comment.
Why did you decide that the publish-annotation methods should accept an ARTAnnotation? This forces users to use that huge initializer and to specify non-nullable ARTAnnotation properties that it doesn't make sense for them to specify, such as serial or action. See RSAN1a2
| NSError *error = nil; | ||
| NSJSONWritingOptions options; | ||
| if (@available(macOS 10.15, iOS 13.0, tvOS 13.0, *)) { | ||
| options = NSJSONWritingWithoutEscapingSlashes; |
| type:annotation.type | ||
| extras:annotation.extras]; | ||
| dispatch_sync(_queue, ^{ | ||
| if (!self.realtime.connection.isActive_nosync) { |
There was a problem hiding this comment.
Please include the spec points that this check and the channel state check are implementing.
| ARTLogVerbose(self.logger, @"R:%p C:%p (%@) annotation subscribe to '%@' action(s)", self->_realtime, self->_channel, self->_channel.name, type == nil ? @"all" : type); | ||
| }); | ||
| [self->_channel.internalEventEmitter once:^(ARTChannelStateChange *stateChange) { | ||
| if (stateChange.current == ARTRealtimeChannelAttached && !self->_channel.isAnnotationSubscribeGranted) { // RTAN4e |
There was a problem hiding this comment.
What if the channel was already attached and so we didn't need to do an implicit attach, but the modes don't contain ANNOTATION_SUBSCRIBE?
| - (instancetype)init { | ||
| self = [super init]; | ||
| if (self) { | ||
| _limit = 100; |
There was a problem hiding this comment.
Where did this come from?
| - (instancetype)initWithLimit:(NSUInteger)limit { | ||
| self = [super init]; | ||
| if (self) { | ||
| _limit = limit > 0 ? limit : 100; |
There was a problem hiding this comment.
Why? Seems like a negative limit is a programmer error
|
Also, what about the https://sdk.ably.com/builds/ably/specification/main/features/#TM7 convenience types? (can do it as a separate issue if you want) |
Closes #2077, #1466
Summary by CodeRabbit