[AIT-324] Add supporting API needed for apply-on-ACK#11
Open
lawrence-forooghian wants to merge 1 commit intomainfrom
Open
[AIT-324] Add supporting API needed for apply-on-ACK#11lawrence-forooghian wants to merge 1 commit intomainfrom
lawrence-forooghian wants to merge 1 commit intomainfrom
Conversation
WalkthroughAdds optional protocol methods and new publish-result protocols, exposes connection site code, and updates README with dependency/setup and a change playbook. No behavioral changes to existing APIs; additions are opt-in/optional. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
README.md (1)
24-25: Tighten wording for clarity and consistency.Small doc polish: replace “is able to” with “can” (Line 24, Line 65), and consider “outdated version” instead of “old version” (Line 53) for stronger wording.
Also applies to: 53-53, 65-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 24 - 25, Update the README wording for clarity: replace the phrase "is able to" with "can" in both occurrences (the sentence "a given version of the plugin is able to specify a _minimum_ version of ably-cocoa" and the similar sentence at the other occurrence), and replace "old version" with "outdated version" where it appears (the sentence discussing version compatibility) to strengthen phrasing and maintain consistency.Sources/_AblyPluginSupportPrivate/include/APPublishResult.h (1)
5-21: Consider addingNS_SWIFT_SENDABLEfor consistency.Other protocols in this PR (e.g.,
APConnectionDetailsProtocol,APPluginAPIProtocol,APLiveObjectsPluginProtocol) include theNS_SWIFT_SENDABLEannotation. For consistency and to ensure these protocols can be safely used across concurrency boundaries in Swift, consider adding the annotation here as well.Note: The static analysis errors are false positives—the file uses valid Objective-C module imports and Foundation macros that require framework context to parse correctly.
Proposed change
/// The serial for a single published message. NS_SWIFT_NAME(PublishResultSerialProtocol) +NS_SWIFT_SENDABLE `@protocol` APPublishResultSerialProtocol <NSObject> /// The message serial of the published message, or `nil` if the message was discarded due to a configured conflation rule. `@property` (nullable, nonatomic, readonly) NSString *value; `@end` /// Contains the result of a publish operation. NS_SWIFT_NAME(PublishResultProtocol) +NS_SWIFT_SENDABLE `@protocol` APPublishResultProtocol <NSObject>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/_AblyPluginSupportPrivate/include/APPublishResult.h` around lines 5 - 21, Add the NS_SWIFT_SENDABLE annotation to the two publish-result protocols to match other protocols in the PR: annotate APPublishResultSerialProtocol and APPublishResultProtocol with NS_SWIFT_SENDABLE (e.g., NS_SWIFT_NAME(PublishResultSerialProtocol) NS_SWIFT_SENDABLE on APPublishResultSerialProtocol and NS_SWIFT_NAME(PublishResultProtocol) NS_SWIFT_SENDABLE on APPublishResultProtocol) so they are marked sendable for Swift concurrency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Around line 24-25: Update the README wording for clarity: replace the phrase
"is able to" with "can" in both occurrences (the sentence "a given version of
the plugin is able to specify a _minimum_ version of ably-cocoa" and the similar
sentence at the other occurrence), and replace "old version" with "outdated
version" where it appears (the sentence discussing version compatibility) to
strengthen phrasing and maintain consistency.
In `@Sources/_AblyPluginSupportPrivate/include/APPublishResult.h`:
- Around line 5-21: Add the NS_SWIFT_SENDABLE annotation to the two
publish-result protocols to match other protocols in the PR: annotate
APPublishResultSerialProtocol and APPublishResultProtocol with NS_SWIFT_SENDABLE
(e.g., NS_SWIFT_NAME(PublishResultSerialProtocol) NS_SWIFT_SENDABLE on
APPublishResultSerialProtocol and NS_SWIFT_NAME(PublishResultProtocol)
NS_SWIFT_SENDABLE on APPublishResultProtocol) so they are marked sendable for
Swift concurrency.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
README.mdSources/_AblyPluginSupportPrivate/include/APConnectionDetails.hSources/_AblyPluginSupportPrivate/include/APLiveObjectsPlugin.hSources/_AblyPluginSupportPrivate/include/APPluginAPI.hSources/_AblyPluginSupportPrivate/include/APPublishResult.h
Add methods for plugin to: - get PublishResult when performing a publish - learn about channel state changes - learn about the siteCode of the server it's connected to Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lawrence-forooghian
force-pushed
the
AIT-324-apply-on-ACK
branch
from
693fb5e to
242fac1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/_AblyPluginSupportPrivate/include/APPluginAPI.h (1)
86-89: Clarify callback invariants forcompletionWithResult.Please document whether
publishResultis guaranteed non-null on success and expected to be null on error. That removes ambiguity for plugin-side branching.✍️ Suggested doc tweak
-/// Same as `-nosync_sendObjectWithObjectMessages:channel:completion:`, but the completion handler additionally receives an `APPublishResultProtocol` containing the serials assigned to the published messages by the server. +/// Same as `-nosync_sendObjectWithObjectMessages:channel:completion:`, but the completion handler additionally receives an `APPublishResultProtocol` containing the serials assigned to the published messages by the server. +/// +/// Contract: on success, `error == nil` and `publishResult != nil`; on failure, `error != nil` and `publishResult == nil`. - (void)nosync_sendObjectWithObjectMessages:(NSArray<id<APObjectMessageProtocol>> *)objectMessages channel:(id<APRealtimeChannel>)channel completionWithResult:(void (^ _Nullable)(_Nullable id<APPublicErrorInfo> error, _Nullable id<APPublishResultProtocol> publishResult))completion;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/_AblyPluginSupportPrivate/include/APPluginAPI.h` around lines 86 - 89, Update the documentation comment for the method nosync_sendObjectWithObjectMessages:channel:completionWithResult: to state the callback invariants explicitly: on success the completion's error parameter is nil and the publishResult (id<APPublishResultProtocol>) is non-nil and contains assigned serials; on failure the error (id<APPublicErrorInfo>) is non-nil and publishResult is nil; ensure the comment uses those exact symbols (APPublishResultProtocol, APPublicErrorInfo, completionWithResult) so plugin authors know how to branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/_AblyPluginSupportPrivate/include/APPluginAPI.h`:
- Around line 86-89: Update the documentation comment for the method
nosync_sendObjectWithObjectMessages:channel:completionWithResult: to state the
callback invariants explicitly: on success the completion's error parameter is
nil and the publishResult (id<APPublishResultProtocol>) is non-nil and contains
assigned serials; on failure the error (id<APPublicErrorInfo>) is non-nil and
publishResult is nil; ensure the comment uses those exact symbols
(APPublishResultProtocol, APPublicErrorInfo, completionWithResult) so plugin
authors know how to branch.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
README.mdSources/_AblyPluginSupportPrivate/include/APConnectionDetails.hSources/_AblyPluginSupportPrivate/include/APLiveObjectsPlugin.hSources/_AblyPluginSupportPrivate/include/APPluginAPI.hSources/_AblyPluginSupportPrivate/include/APPublishResult.h
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/_AblyPluginSupportPrivate/include/APLiveObjectsPlugin.h
lawrence-forooghian
changed the title
maratal
reviewed
Mar 3, 2026
| /// Same as `-nosync_sendObjectWithObjectMessages:channel:completion:`, but the completion handler additionally receives an `APPublishResultProtocol` containing the serials assigned to the published messages by the server. | ||
| - (void)nosync_sendObjectWithObjectMessages:(NSArray<id<APObjectMessageProtocol>> *)objectMessages | ||
| channel:(id<APRealtimeChannel>)channel | ||
| completionWithResult:(void (^ _Nullable)(_Nullable id<APPublicErrorInfo> error, _Nullable id<APPublishResultProtocol> publishResult))completion; |
Collaborator
There was a problem hiding this comment.
Why error is before the result? Usually it's the last argument.
| @protocol APPublishResultProtocol <NSObject> | ||
|
|
||
| /// An array of serials corresponding 1:1 to the messages that were published. | ||
| @property (nonatomic, readonly) NSArray<id<APPublishResultSerialProtocol>> *serials; |
Collaborator
There was a problem hiding this comment.
Can't it be just an array of strings? Why this wrapping protocol?
|
|
||
| #### Removing functionality from ably-cocoa | ||
|
|
||
| Since new versions of ably-cocoa can be used with old versions of the plugin, ably-cocoa must continue to provide functioning implementations of all methods. |
Collaborator
There was a problem hiding this comment.
is there any "depth" of old plugins versions support?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add methods for plugin to:
PublishResultwhen performing a publishsiteCodeof the server it's connected toRelated PRs:
Summary by CodeRabbit
Documentation
New Features