[ECO-5611] Address nullability change in ARTAnnotation initializer and timestamp property

[maratal]

Closes #2125, #2124

Summary by CodeRabbit

• Refactor

  • Simplified annotation construction with a streamlined initializer for internal use.
  • Event annotation creation now uses a class factory method for API consistency.

• API Changes

  • Annotation timestamp property and constructor now require a non-null timestamp.

• Bug Fix / Behavior

  • Parsing now defaults a missing annotation timestamp to the current time.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds a new nullable-friendly ARTAnnotation initializer (omitting encoding/timestamp/serial), updates call sites to use it, changes ARTAnnotation.timestamp to non-nullable, introduces a private initializer declaration, changes ARTEvent (AnnotationType) from an instance initializer to a class factory, and defaults missing annotation timestamps to the current date in JSON decoding.

Changes

Cohort / File(s)	Summary
ARTAnnotation implementation & private API Source/ARTAnnotation.m, Source/PrivateHeaders/Ably/ARTAnnotation+Private.h	Adds a new designated initializer initWithId:action:clientId:name:count:data:messageSerial:type:extras: and declares it in the private header; implements assignment of corresponding ivars.
Public header changes Source/include/Ably/ARTAnnotation.h	Changes timestamp property from nullable NSDate * to non-nullable NSDate *; updates existing initializer signature to require non-null timestamp; replaces -initWithAnnotationType: with +newWithAnnotationType: in the ARTEvent (AnnotationType) category.
Realtime & REST publish sites Source/ARTRealtimeAnnotations.m, Source/ARTRestAnnotations.m	Call sites updated to construct ARTAnnotation using the new initializer (removed explicit encoding, timestamp, serial nil arguments). Error-path message formatting unchanged aside from indentation.
JSON decoding behavior Source/ARTJsonLikeEncoder.m	When decoding annotations, timestamp now falls back to NSDate.date if input timestamp is nil ([input artTimestamp:@"timestamp"] ?: NSDate.date).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 In code I hop, with tiny feet,
New init paths make bindings neat,
Missing times? I fill the date,
Factory calls now feel first-rate,
A rabbit's cheer for changes complete! 🥕

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The pull request successfully reverts the nullability change by making ARTAnnotation.timestamp non-nullable and updating related initializers and usages across multiple files, directly addressing issue #2125/ECO-5611.
Out of Scope Changes check	✅ Passed	All changes are directly related to reverting the nullability modification; updates to ARTRealtimeAnnotations.m, ARTRestAnnotations.m, and ARTJsonLikeEncoder.m are necessary adjustments to accommodate the reverted initializer signature.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main changes: addressing nullability modifications in ARTAnnotation's initializer and timestamp property.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/2125-revert-nullability-change

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 211c51d and 8dea479.

📒 Files selected for processing (5)

• Source/ARTAnnotation.m
• Source/ARTRealtimeAnnotations.m
• Source/ARTRestAnnotations.m
• Source/PrivateHeaders/Ably/ARTAnnotation+Private.h
• Source/include/Ably/ARTAnnotation.h

💤 Files with no reviewable changes (1)

• Source/ARTRestAnnotations.m

🧰 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/ARTAnnotation.m
• Source/ARTRealtimeAnnotations.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 (iOS, test_iOS18_4)
• GitHub Check: check (macOS, test_macOS)
• GitHub Check: check (tvOS, test_tvOS18_4)
• GitHub Check: build
• GitHub Check: check
• GitHub Check: check

🔇 Additional comments (5)

Source/ARTRealtimeAnnotations.m (1)

131-139: LGTM! Verify default values for omitted fields.

The conversion now uses the new internal initializer without encoding, timestamp, and serial. This correctly addresses the nullability issue for outbound annotations.

Confirm that leaving encoding, timestamp, and serial as nil is the expected behavior for annotations being published. The server should presumably assign these values.

Source/include/Ably/ARTAnnotation.h (2)

83-84: LGTM! Provides convenient factory method.

The addition of the class factory method +newWithAnnotationType: alongside the instance initializer provides a convenient way to create ARTEvent instances. The implementation correctly delegates to the instance initializer.

---

71-71: Timestamp parameter design is intentional, not contradictory.

The public initializer requires timestamp (non-nullable at line 71), but the private initializer in ARTAnnotation+Private.h (lines 19-26) omits the timestamp parameter entirely, specifically for "converting outbound annotations to annotations before publishing." The nullable timestamp property (line 50) allows for nil values once created. This design—strict public API with flexible private API—is intentional and supports outbound annotation conversion without contradicting the PR objective.

Source/ARTAnnotation.m (2)

176-182: LGTM! Standard factory method pattern.

The implementation correctly provides both an instance initializer and a class factory method. The factory method properly allocates and initializes using the instance method.

---

39-60: This initializer intentionally omits encoding, timestamp, and serial as it's designed for outbound annotations being prepared for publication. These nil values are properly handled: the artAddEncoding method explicitly manages nil encoding via a ternary operator, while timestamp and serial are not accessed during the publish/encode flow. No action required.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

Source/include/Ably/ARTAnnotation.h (1)

83-84: Redundant method declarations in ARTEvent category.

Both an instance method -initWithAnnotationType: (line 83) and a class factory method +newWithAnnotationType: (line 84) are declared. This creates ambiguity:

• If the class factory method is the intended API, remove the instance method
• If both are intentional, clarify the use case for each

Typically, when adding a class factory method, the corresponding instance initializer is kept private or removed from the public interface.

🔎 Check usage of both methods

#!/bin/bash
# Verify which method is actually used in the codebase

# Search for calls to instance method
rg -n "\[\[.*alloc\].*initWithAnnotationType:" --type=m

# Search for calls to class factory method  
rg -n "\[.*newWithAnnotationType:" --type=m

# Check implementation in ARTEvent category
fd -e m -e h ARTEvent | xargs rg -n "AnnotationType"

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 8dea479 and 40673db.

📒 Files selected for processing (2)

• Source/ARTJsonLikeEncoder.m
• Source/include/Ably/ARTAnnotation.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/ARTJsonLikeEncoder.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: check
• GitHub Check: check (iOS, test_iOS18_4)
• GitHub Check: check (tvOS, test_tvOS18_4)
• GitHub Check: build
• GitHub Check: check