[ECO-5657] Make both ARTMessageAnnotations and ARTMessageVersion sendable #2176
Conversation
WalkthroughAdded readonly-backed designated initializers and factory methods to ARTMessageAnnotations and ARTMessageVersion, changed several properties to readonly, and annotated both classes with NS_SWIFT_SENDABLE; initialization and copy semantics were refactored to use the new initializers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/2176/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/include/Ably/ARTMessageVersion.h (1)
9-25: Critical:NS_SWIFT_SENDABLEis unsafe with mutable properties.The
NS_SWIFT_SENDABLEannotation declares that this class is safe to share across concurrency domains, but all five properties (serial,timestamp,clientId,descriptionText,metadata) are explicitly markedreadwrite(mutable). This violates Sendable safety guarantees and can lead to data races when instances are shared across threads.Since the implementation only sets these properties during initialization and never modifies them afterward, all properties should be declared
readonlyto ensure thread safety.🔎 Proposed fix
/// The serial of the message version. -@property (nullable, readwrite, nonatomic) NSString *serial; +@property (nullable, readonly, nonatomic) NSString *serial; /// The timestamp of the message version. -@property (nullable, readwrite, nonatomic) NSDate *timestamp; +@property (nullable, readonly, nonatomic) NSDate *timestamp; /// The client ID associated with this version. -@property (nullable, readwrite, nonatomic) NSString *clientId; +@property (nullable, readonly, nonatomic) NSString *clientId; /// A description of the operation performed. -@property (nullable, readwrite, nonatomic) NSString *descriptionText; +@property (nullable, readonly, nonatomic) NSString *descriptionText; /// Metadata associated with the operation. -@property (nullable, readwrite, nonatomic) NSDictionary<NSString *, NSString *> *metadata; +@property (nullable, readonly, nonatomic) NSDictionary<NSString *, NSString *> *metadata;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
Source/ARTMessageAnnotations.mSource/ARTMessageVersion.mSource/include/Ably/ARTMessageAnnotations.hSource/include/Ably/ARTMessageVersion.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTMessageVersion.mSource/ARTMessageAnnotations.m
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: check
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check
🔇 Additional comments (10)
Source/include/Ably/ARTMessageAnnotations.h (1)
15-20: LGTM: Well-documented designated initializer.The new designated initializer follows Objective-C best practices and provides clear documentation.
Source/include/Ably/ARTMessageVersion.h (1)
27-40: LGTM: Comprehensive designated initializer.The new designated initializer covers all properties and follows Objective-C best practices with clear documentation.
Source/ARTMessageVersion.m (4)
8-10: LGTM: Proper delegation to designated initializer.The default
initcorrectly delegates to the designated initializer with all-nil values, following best practices.
12-26: LGTM: Clean designated initializer implementation.The designated initializer correctly:
- Assigns to ivars (bypassing property setters)
- Handles all nullable parameters
- Follows standard Objective-C initialization patterns
Once the properties are changed to
readonlyin the header (as flagged in the header review), this implementation will be fully thread-safe for Sendable conformance.
28-34: LGTM: Copy implementation uses designated initializer.The
copyWithZone:method correctly creates a copy using the designated initializer, ensuring consistency across the codebase.
54-66: LGTM: Factory method with proper type checking.The factory method correctly:
- Validates metadata type before assignment (lines 56-59)
- Uses the designated initializer for object creation
- Handles all nullable parameters safely
Source/ARTMessageAnnotations.m (4)
6-8: LGTM: Proper delegation to designated initializer.The default
initcorrectly delegates to the designated initializer with nil, following best practices.
10-16: LGTM: Clean designated initializer implementation.The designated initializer correctly assigns to the ivar (bypassing the property setter) and follows standard Objective-C initialization patterns.
Once the property is changed to
readonlyin the header (as flagged in the header review), this implementation will be fully thread-safe for Sendable conformance.
18-21: LGTM: Copy implementation uses designated initializer.The
copyWithZone:method correctly creates a copy using the designated initializer, ensuring consistency.
29-31: LGTM: Factory method uses designated initializer.The factory method correctly uses the designated initializer for object creation, maintaining consistency across the codebase.
github-actions
bot
temporarily deployed
to
staging/pull/2176/jazzydoc
maratal
force-pushed
the
fix/2155-make-version-and-annotations-sendable
branch
from
235fb2b to
66a90bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Source/ARTMessageAnnotations.m (1)
10-16: Consider copyingsummaryfor true Sendable safety.The direct assignment
_summary = summary;means if a caller passes anNSMutableDictionary, they retain a reference that can mutate the object's internal state from another thread, breakingSendableguarantees.🔎 Proposed fix
- (instancetype)initWithSummary:(nullable ARTJsonObject *)summary { self = [super init]; if (self) { - _summary = summary; + _summary = [summary copy]; } return self; }Source/ARTMessageVersion.m (1)
12-26: Consider copying mutable types for true Sendable safety.Similar to
ARTMessageAnnotations, the direct ivar assignments don't protect against external mutation if callers pass mutable variants (NSMutableString,NSMutableDictionary). For robustSendableconformance, copy the string and dictionary properties.
NSDateis immutable, sotimestampis fine as-is.🔎 Proposed fix
- (instancetype)initWithSerial:(nullable NSString *)serial timestamp:(nullable NSDate *)timestamp clientId:(nullable NSString *)clientId descriptionText:(nullable NSString *)descriptionText metadata:(nullable NSDictionary<NSString *, NSString *> *)metadata { self = [super init]; if (self) { - _serial = serial; + _serial = [serial copy]; _timestamp = timestamp; - _clientId = clientId; - _descriptionText = descriptionText; - _metadata = metadata; + _clientId = [clientId copy]; + _descriptionText = [descriptionText copy]; + _metadata = [metadata copy]; } return self; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
Source/ARTMessageAnnotations.mSource/ARTMessageVersion.mSource/include/Ably/ARTMessageAnnotations.hSource/include/Ably/ARTMessageVersion.h
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTMessageVersion.mSource/ARTMessageAnnotations.m
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: build
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check
🔇 Additional comments (5)
Source/include/Ably/ARTMessageAnnotations.h (1)
9-20: LGTM! Properly addresses the Sendable concern.The changes correctly make
ARTMessageAnnotationssafe forNS_SWIFT_SENDABLE:
- The
summaryproperty is nowreadonly, preventing mutation after initialization- A designated initializer is provided to set the value during construction
This resolves the issue flagged in the previous review about mutable properties with
NS_SWIFT_SENDABLE.Source/ARTMessageAnnotations.m (1)
18-31: LGTM!The
copyWithZone:correctly uses the designated initializer pattern, andcreateFromDictionary:properly extracts the summary from the input dictionary.Source/ARTMessageVersion.m (2)
28-34: LGTM!The
copyWithZone:implementation correctly uses the designated initializer pattern with[self.class allocWithZone:zone].
54-66: LGTM!The
createFromDictionary:factory method is cleanly refactored to use the designated initializer. The metadata extraction with type checking is appropriate.Source/include/Ably/ARTMessageVersion.h (1)
9-40: LGTM! Proper Sendable implementation.The changes correctly address the Sendable conformance issue:
NS_SWIFT_SENDABLEis now safe with all properties beingreadonly- The designated initializer provides a clean API for construction
- Documentation is thorough
This resolves ECO-5657 by making
ARTMessageVersiontruly immutable from the public interface.
Closes #2155
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.