[ECO-5531] Move AblyPlugin to an external repo#2088
Conversation
They were originally just added as a demonstration in e0e7f17; will write a proper API when needed.
WalkthroughRemoves the AblyPlugin product and its public headers/impl, replaces it with a dependency on an external private plugin-support package (_AblyPluginSupportPrivate), introduces ABLY_SUPPORTS_PLUGINS and gates plugin code, adds ARTPluginAPI bridging/registration and private headers, updates module maps, SPM/Pod configs, project files, and docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Channel as ARTRealtimeChannel
participant API as ARTPluginAPI
participant Plugin as LiveObjectsPlugin
note over Channel,API: Flow active only when ABLY_SUPPORTS_PLUGINS is defined
App->>Channel: init / attach
Channel->>API: +registerSelf()
API->>Plugin: register/fetch via APDependencyStore
Channel->>Plugin: prepareChannel(...)/onChannelAttached(...)
App->>Channel: sendObjectWithObjectMessages(messages, completion)
Channel->>API: sendObjectWithObjectMessages(...) (forward)
API->>Plugin: send -> plugin handles publish
Plugin-->>Channel: completion(error?)
Channel-->>App: completion(error?)
sequenceDiagram
autonumber
participant Encoder as ARTJsonLikeEncoder
participant API as ARTPluginAPI
participant Plugin as LiveObjectsPlugin
participant PM as ARTProtocolMessage
note over Encoder,API: Object-message decode/encode path (gated)
Encoder->>API: decodeObjectMessage(serialized, context, format)
API->>Plugin: decodeObjectMessage(...)
Plugin-->>API: objectMessages or error
API-->>Encoder: objectMessages or nil
Encoder->>PM: set state = objectMessages (if decoded)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/jazzydoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Docs/plugins.md (1)
23-29: Polish wording/formatting; capture the product rename mitigationA few minor grammar/formatting nits and an opportunity to document the Xcode product-selection mitigation explicitly.
Apply this diff:
-## Notes TODO incorporate +## Notes (to incorporate) -- all of the stuff that depends on `AblyPlugin` is gated behind `#ifdef ABLY_IS_SPM_BUILD` so as not to affect the non-SPM builds, which do not have access to `AblyPlugin` -- note that AblyPlugin can not depend on ably-cocoa at all, which means having to duplicate some types (e.g. enums), or having to weaken some classes e.g. the public types such as `ARTRealtimeChannel`, `ARTClientOptions`, `ARTErrorInfo` etc - - TODO we can probably improve this a bit by at least introducing some empty marker protocols and declaring conformance -- mention that we are now going to not have any instance methods on the channel or realtime because then you have to deal with clashes because our `state` doesn't return the same type as their `state`. same for logger and its level +- All content that depends on `AblyPlugin` is gated behind `#ifdef ABLY_IS_SPM_BUILD` to avoid impacting non-SPM builds, which do not have access to `AblyPlugin`. +- Note that `AblyPlugin` cannot depend on ably-cocoa, which means duplicating some types (for example, enums) or weakening some classes (for example, public types such as `ARTRealtimeChannel`, `ARTClientOptions`, `ARTErrorInfo`, etc.). + - TODO: We can probably improve this by introducing empty marker protocols and declaring conformance. +- We will avoid adding instance methods on the channel or realtime (and similarly on the logger) to prevent clashes where our `state` types differ. +- Short‑term mitigation for Xcode’s auto-selection: rename the library product to `AblyPluginPrivate` so users are less likely to link it by accident, while keeping the target/module name `AblyPlugin` for source imports.Additionally, fix the unordered list indentation (two spaces for sub-items) to satisfy markdownlint (MD007).
Source/ARTJsonLikeEncoder.m (1)
1112-1122: Non-exhaustive switch; missing return path can cause undefined behavior
apEncodingFormatFromARTEncoderFormat:has no default branch. If a newARTEncoderFormatvalue is introduced, this will return an undefined value.Apply this diff to make the mapping safe and future-proof:
- (APEncodingFormat)apEncodingFormatFromARTEncoderFormat:(ARTEncoderFormat)format { switch (format) { case ARTEncoderFormatJson: return APEncodingFormatJSON; case ARTEncoderFormatMsgPack: return APEncodingFormatMessagePack; } + NSCAssert(NO, @"Unhandled ARTEncoderFormat: %ld", (long)format); + return APEncodingFormatJSON; }
🧹 Nitpick comments (24)
Source/PrivateHeaders/Ably/ARTRealtime+Private.h (2)
34-39: Align conditional conformance with the same availability gate and reduce duplicationConformance should use the same gate as the import to avoid mismatches if the header isn’t available. You can also avoid duplicating the @interface line by conditionally appending the protocol list.
Apply this diff:
-#ifdef ABLY_IS_SPM_BUILD -@interface ARTRealtimeInternal : NSObject <APRealtimeClient> -#else -@interface ARTRealtimeInternal : NSObject -#endif +@interface ARTRealtimeInternal : NSObject +#if ABLY_CAN_IMPORT_PLUGIN +<APRealtimeClient> +#endifPlease confirm whether CocoaPods/Carthage builds should also adopt APRealtimeClient if the plugin support library is present. If so, consider broadening the gate to include header availability even without ABLY_IS_SPM_BUILD (e.g., rely solely on ABLY_CAN_IMPORT_PLUGIN as above).
13-16: Harden plugin import and introduce a reusable capability macroGuard the SPM-only
@import AblyPluginby checking for the plugin’s header, and defineABLY_CAN_IMPORT_PLUGINfor use throughout the codebase when referring to plugin APIs. This prevents build failures if the plugin module is missing or renamed.• File: Source/PrivateHeaders/Ably/ARTRealtime+Private.h
• Replace the plainABLY_IS_SPM_BUILDguard with an__has_includecheck
• After this change, wrap any references toAPRealtimeClient(in headers/implementations) in#if ABLY_CAN_IMPORT_PLUGIN…#endifApply this diff:
--- a/Source/PrivateHeaders/Ably/ARTRealtime+Private.h +++ b/Source/PrivateHeaders/Ably/ARTRealtime+Private.h @@ -#ifdef ABLY_IS_SPM_BUILD -@import AblyPlugin; -#endif +#if defined(ABLY_IS_SPM_BUILD) && __has_include(<AblyPlugin/APRealtimeClient.h>) + @import AblyPlugin; +# define ABLY_CAN_IMPORT_PLUGIN 1 +#else +# define ABLY_CAN_IMPORT_PLUGIN 0 +#endifPackage.resolved (1)
4-12: Ensure SwiftPM pins are machine-generated, not manualFile: Package.resolved (lines 4–12)
- The
branchfield is set to"97c3d52a", a short SHA indicating this was hand-edited.- SwiftPM normally sets
"branch": nullwhen pinning by revision and keeps these entries in sync.Suggested change:
--- a/Package.resolved +++ b/Package.resolved @@ state { - "branch": "97c3d52a", + "branch": null, "revision": "97c3d52acede2831ee8cfea6a0260d6ddd065573", "version": null }Action:
• Runswift package resolveto regenerate Package.resolved and commit the result.
• Prefer tagging and using a semantic version; if you must pin to a commit, let SwiftPM manage the entry to avoid future churn.Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (2)
14-16: Conditional AblyPlugin import is correct; optionally hide forward decls in non-SPM buildsThe guarded AblyPlugin import is correct. To avoid leaking plugin symbol names in non-SPM builds, consider guarding the AP* forward declarations too (purely cosmetic, forward-declaring protocols is harmless).
Outside the changed lines, consider wrapping the forward declaration:
#ifdef ABLY_IS_SPM_BUILD @protocol APObjectMessageProtocol; #endif
27-31: Conditional conformance to APRealtimeChannel is appropriate; consider gating plugin-only methods in this headerConformance only under ABLY_IS_SPM_BUILD is the right call. For consistency, consider also guarding plugin-only methods in the Private category (e.g., sendStateWithObjectMessages:completion:) behind ABLY_IS_SPM_BUILD, so they don’t appear in non-SPM header surfaces.
If you choose to change it, wrap the plugin methods in the Private category like:
#ifdef ABLY_IS_SPM_BUILD /// Provides the implementation for `-[APPluginAPI setPluginDataValue:forKey:channel]`. See documentation for that method. - (void)setPluginDataValue:(id)value forKey:(NSString *)key; /// Provides the implementation for `-[APPluginAPI pluginDataValueForKey:channel]`. See documentation for that method. - (nullable id)pluginDataValueForKey:(NSString *)key; /// Provides the implementation for `-[APPluginAPI sendStateWithObjectMessages:completion:]`. See documentation for that method. - (void)sendStateWithObjectMessages:(NSArray<id<APObjectMessageProtocol>> *)objectMessages completion:(ARTCallback)completion; #endifSource/PrivateHeaders/Ably/ARTInternalLog.h (1)
75-81: Minor doc clarification.Consider clarifying “APPluginAPI” as “APPluginAPI (from the AblyPlugin module)” to make the dependency explicit to readers navigating the code.
Package.swift (3)
20-23: Avoid shipping with a pinned commit; prefer a branch or a tagged version.Pinning to a revision is fine during WIP, but please switch before merge to keep dependency resolution healthy.
Proposed change (use branch for now, and swap to a tag when available):
- // TODO: Unpin this before releasing LiveObjects branch - .package(url: "https://github.com/ably/ably-cocoa-plugin-support", .revision("97c3d52a")), + // swiftlint:disable:next todo + // TODO(ECO-5531): Unpin this before releasing LiveObjects branch + .package(url: "https://github.com/ably/ably-cocoa-plugin-support", .branch("main")),
21-21: Silence SwiftLint’s TODO warning or link the ticket.If keeping the TODO during WIP, either disable the rule for this line or link the Jira ticket explicitly (as above) so future cleanup isn’t missed.
29-31: Depending on AblyPlugin as a target dep (not a product) aligns with the objective.This keeps plugin symbols internal to Ably’s build and avoids exposing extra products to end users adding ably-cocoa. Nice.
If/when ably-cocoa-plugin-support is consumed directly, consider renaming its product to something “private”-sounding to further reduce accidental selection in Xcode (e.g., AblyPluginPrivate). Not a blocker here.
Source/ARTPublicRealtimeChannelUnderlyingObjects.m (1)
3-3: Import AblyPlugin explicitly to ensure AP protocol symbols resolve.*If ARTPublicRealtimeChannelUnderlyingObjects.h doesn’t import AblyPlugin itself, this file will need it. Being explicit here avoids hidden transitive dependencies.
-#import "ARTPublicRealtimeChannelUnderlyingObjects.h" +#import "ARTPublicRealtimeChannelUnderlyingObjects.h" +@import AblyPlugin;Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h (1)
4-4: Reduce compile-time coupling by forward-declaring the protocol.You can avoid importing the entire AblyPlugin module in a header by forward-declaring the protocol. This reduces rebuilds and avoids accidental transitive dependencies.
-@import AblyPlugin; +@protocol APDecodingContextProtocol;Note: Keep the @import in the .m if needed.
Source/PrivateHeaders/Ably/ARTPluginAPI.h (1)
8-16: Clarify idempotency and thread-safety of +registerSelfThe API contract says “If not already registered…”. Ensure the implementation is idempotent and thread-safe (e.g., dispatch_once or equivalent). Optionally, consider returning a BOOL to indicate whether registration happened vs. was already in place for easier call-site telemetry.
Happy to propose a concrete implementation in ARTPluginAPI.m if you share it.
Source/ARTRealtimeChannel.m (1)
68-77: Registering the plugin API early is the right moveRegistering in
-initWithInternal:realtimeInternal:queuedDealloc:ensures extensions on ARTRealtimeChannel from the plugin can safely resolve the API. The nil-check aroundliveObjectsPluginand targeted preparation are sound.If
+[ARTPluginAPI registerSelf]may be called elsewhere (e.g., in ARTClientOptions), ensure it’s idempotent to avoid work on hot paths.Source/ARTClientOptions.m (1)
35-39: Appropriate place to register ARTPluginAPIRegistering during
-initDefaultsis early enough that plugin extensions on ARTClientOptions resolve correctly. Ensure the registration is idempotent for repeated initializations.If
+[ARTPluginAPI registerSelf]triggers any heavy work, consider gating it behind a static predicate to avoid repeated execution across multiple options instances.Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
27-31: Good: liveObjectsPlugin property is SPM-only; optional micro-optimizationThe property exposure behind
ABLY_IS_SPM_BUILDprevents accidental usage by non-SPM builds. If you want to decouple further, you could replace the module import with a forward declaration of@protocol APLiveObjectsInternalPluginProtocol;inside the guard to reduce header compile time fan-out.Example:
#ifdef ABLY_IS_SPM_BUILD @protocol APLiveObjectsInternalPluginProtocol; @property (nullable, readonly) id<APLiveObjectsInternalPluginProtocol> liveObjectsPlugin; #endifAbly.xcodeproj/project.pbxproj (1)
1414-1417: Public headers are stored under PrivateHeaders; consider moving to include/Ably for consistency
ARTPluginAPI.handARTPublicRealtimeChannelUnderlyingObjects.hare placed underPrivateHeaders/Ably/.... If they are to be consumed by an external package, place them underinclude/Ably/...(like the rest of Ably’s public headers) to keep structure consistent and avoid confusion.Would you like me to generate the minimal pbxproj changes to relocate these file references to
include/Ably?Source/ARTJsonLikeEncoder.m (3)
581-585: Encoding state only when present; confirm plugin-only invariant holdsThis will crash (by design) if
objectMessagesToArray:raises due to a missing plugin. Please confirm there’s no code path that setsmessage.stateoutside the plugin so we never attempt to encode state without the plugin at runtime.If you want to make the encoder resilient, consider downgrading to a warn-and-omit (instead of raising) when the plugin is missing.
831-833: Decoding state is gated and safe; consider a debug log when plugin is absentDecoding returns
nilif the plugin isn’t supplied or any item fails to decode. That’s safe, but adding a one-time debug log when the plugin is missing could aid diagnostics.
1124-1200: LiveObjects encode/decode: minor robustness and API consistency nits
- When the plugin is missing,
objectMessagesFromArray:silently returnsnil. A debug log will help explain dropped state.objectMessagesToArray:can returnnilwhen passednil, but the signature is nonnull. Consider marking it nullable for consistency with the behavior.Here’s a small improvement:
- (nullable NSArray<id<APObjectMessageProtocol>> *)objectMessagesFromArray:(nullable id)input protocolMessage:(ARTProtocolMessage *)protocolMessage { @@ - if (!liveObjectsPlugin) { - return nil; - } + if (!liveObjectsPlugin) { + ARTLogDebug(_logger, @"RS:%p ARTJsonLikeEncoder<%@>: LiveObjects plugin not supplied; dropping state", _rest, [_delegate formatAsString]); + return nil; + } @@ - (NSArray<NSDictionary *> *)objectMessagesToArray:(nullable NSArray<id<APObjectMessageProtocol>> *)objectMessages { + (nullable NSArray<NSDictionary *> *)objectMessagesToArray:(nullable NSArray<id<APObjectMessageProtocol>> *)objectMessages { if (!objectMessages) { return nil; }Source/ARTPluginAPI.m (5)
1-3: Ensure AblyPlugin module symbols are available in SPM buildsThis file references plugin types (APRegister, AP* protocols). To make the dependency explicit and avoid relying on transitive imports, import the module here under SPM.
If not already included via headers, add the module import:
#ifdef ABLY_IS_SPM_BUILD -#import "ARTPluginAPI.h" +#import "ARTPluginAPI.h" +@import AblyPlugin;If
ARTPluginAPI.halready imports the module, feel free to ignore.
24-29: Logger downcast is brittle if a non-ART logger is ever passedThis enforces
ARTInternalLogspecifically. If a plugin mistakenly passes any otherid<APLogger>, this raises. If that’s an intentional contract (logger must be fromloggerForChannel:), consider documenting it on the public API; otherwise, consider a softer check and a graceful fallback.
50-67: Add a default to future-proof log level mappingIf
APLogLevelgains a new case, this will return an undefined value. Defaulting toInfois harmless.switch (pluginLogLevel) { @@ case APLogLevelNone: return ARTLogLevelNone; + default: + return ARTLogLevelInfo; } }
78-79: TODO: remove mentions of public types — track this before mergeThere’s a lingering TODO. If this is a blocker or follow-up, let’s track it explicitly so it doesn’t get lost.
I can open a follow-up issue and add a checklist item for replacing/removing public-type mentions in this file. Want me to do that?
131-140: Logging bridge assumes an ARTInternalLog; tighten contract or make it tolerantThis will raise if
loggerisn’t the instance obtained fromloggerForChannel:. Either:
- Document that requirement in the public API; or
- Allow any
id<APLogger>by checking capabilities and degrading gracefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (33)
.github/workflows/check-plugin.yaml(0 hunks)Ably.podspec(0 hunks)Ably.xcodeproj/project.pbxproj(22 hunks)AblyPlugin/PrivateHeaders/ARTInternalLog+APLogger.h(0 hunks)AblyPlugin/PrivateHeaders/ARTRealtimeChannelInternal+APRealtimeChannel.h(0 hunks)AblyPlugin/PrivateHeaders/ARTRealtimeInternal+APRealtimeClient.h(0 hunks)AblyPlugin/include/APDecodingContext.h(0 hunks)AblyPlugin/include/APEncodingFormat.h(0 hunks)AblyPlugin/include/APLiveObjectsPlugin.h(0 hunks)AblyPlugin/include/APLogger.h(0 hunks)AblyPlugin/include/APPluginAPI.h(0 hunks)AblyPlugin/include/APPublicRealtimeChannelUnderlyingObjects.h(0 hunks)AblyPlugin/include/APRealtimeChannel.h(0 hunks)AblyPlugin/include/APRealtimeClient.h(0 hunks)CONTRIBUTING.md(0 hunks)Docs/plugins.md(1 hunks)Package.resolved(1 hunks)Package.swift(2 hunks)Source/ARTClientOptions.m(4 hunks)Source/ARTJsonLikeEncoder.m(6 hunks)Source/ARTPluginAPI.m(3 hunks)Source/ARTPluginDecodingContext.m(2 hunks)Source/ARTPublicRealtimeChannelUnderlyingObjects.m(2 hunks)Source/ARTRealtimeChannel.m(5 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTInternalLog.h(3 hunks)Source/PrivateHeaders/Ably/ARTPluginAPI.h(1 hunks)Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h(2 hunks)Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtime+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/include/module.modulemap(1 hunks)
💤 Files with no reviewable changes (14)
- AblyPlugin/include/APEncodingFormat.h
- .github/workflows/check-plugin.yaml
- CONTRIBUTING.md
- AblyPlugin/PrivateHeaders/ARTRealtimeInternal+APRealtimeClient.h
- AblyPlugin/PrivateHeaders/ARTRealtimeChannelInternal+APRealtimeChannel.h
- Ably.podspec
- AblyPlugin/include/APPluginAPI.h
- AblyPlugin/include/APPublicRealtimeChannelUnderlyingObjects.h
- AblyPlugin/include/APRealtimeClient.h
- AblyPlugin/include/APRealtimeChannel.h
- AblyPlugin/PrivateHeaders/ARTInternalLog+APLogger.h
- AblyPlugin/include/APDecodingContext.h
- AblyPlugin/include/APLogger.h
- AblyPlugin/include/APLiveObjectsPlugin.h
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2024-11-04T23:10:23.027Z
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/Ably.modulemapSource/ARTPublicRealtimeChannelUnderlyingObjects.mSource/PrivateHeaders/Ably/ARTPluginDecodingContext.hSource/include/module.modulemapSource/ARTJsonLikeEncoder.mSource/PrivateHeaders/Ably/ARTPluginAPI.hSource/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.hSource/PrivateHeaders/Ably/ARTRealtimeChannel+Private.hSource/ARTRealtimeChannel.mSource/PrivateHeaders/Ably/ARTRealtime+Private.hAbly.xcodeproj/project.pbxprojSource/ARTPluginAPI.m
📚 Learning: 2024-10-08T15:37:26.766Z
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/ARTPublicRealtimeChannelUnderlyingObjects.mSource/PrivateHeaders/Ably/ARTClientOptions+Private.hSource/ARTJsonLikeEncoder.mSource/PrivateHeaders/Ably/ARTRealtimeChannel+Private.hSource/ARTRealtimeChannel.mSource/PrivateHeaders/Ably/ARTInternalLog.hSource/PrivateHeaders/Ably/ARTRealtime+Private.hAbly.xcodeproj/project.pbxproj
📚 Learning: 2025-08-13T16:50:28.236Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2084
File: Source/ARTClientOptions.m:249-251
Timestamp: 2025-08-13T16:50:28.236Z
Learning: In ARTClientOptions.m, the setPluginOptionsValue:forKey: method intentionally provides a simple dictionary-like interface without defensive copying, expecting callers to handle proper value management themselves rather than adding defensive programming overhead.
Applied to files:
Source/PrivateHeaders/Ably/ARTClientOptions+Private.hSource/ARTClientOptions.m
📚 Learning: 2024-11-26T17:50:25.730Z
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/ARTJsonLikeEncoder.m
📚 Learning: 2024-11-04T23:11:06.909Z
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:
Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h
📚 Learning: 2024-09-12T20:12:29.428Z
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:
Docs/plugins.md
🪛 SwiftLint (0.57.0)
Package.swift
[Warning] 21-21: TODOs should be resolved (Unpin this before releasing Li...)
(todo)
🪛 LanguageTool
Docs/plugins.md
[style] ~25-~25: Consider removing “of” to be more concise
Context: ...s names). ## Notes TODO incorporate - all of the stuff that depends on AblyPlugin is g...
(ALL_OF_THE)
[style] ~26-~26: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ... to AblyPlugin - note that AblyPlugin can not depend on ably-cocoa at all, which mean...
(CAN_NOT_PREMIUM)
[style] ~26-~26: In American English, abbreviations like “etc.” require a period.
Context: ...el, ARTClientOptions, ARTErrorInfo` etc - TODO we can probably improve this...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
Docs/plugins.md
27-27: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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). (3)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (macOS, test_macOS)
🔇 Additional comments (27)
Source/Ably.modulemap (1)
138-140: Private plugin headers confirmed in Private module and SPM module map
All three headers exist under Source/PrivateHeaders/Ably and are correctly referenced in Source/include/module.modulemap. No further action needed—changes can be merged as is.Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h (1)
1-5: SPM-gated import and product name alignment confirmed
- The
@import AblyPlugin;in ARTPublicRealtimeChannelUnderlyingObjects.h is correctly wrapped in#ifdef ABLY_IS_SPM_BUILD…#endif.- Package.swift defines a product with name
"AblyPlugin"(from theably-cocoa-plugin-supportpackage), matching the module import.No further changes needed.
Source/PrivateHeaders/Ably/ARTInternalLog.h (2)
4-6: SPM-only import and protocol exposure is spot on.Conditionally importing AblyPlugin only for SPM builds keeps non-SPM consumers insulated from plugin APIs. Good separation.
40-45: Confirm APLogger requirements are fully met by ARTInternalLog.Given the conditional conformance, please ensure ARTInternalLog implements exactly the APLogger protocol’s required methods/signatures (no name or parameter mismatches).
Would you like me to scan the plugin-support repo’s APLogger protocol and cross-check against this header/implementation?
Source/ARTPublicRealtimeChannelUnderlyingObjects.m (3)
1-3: LGTM: SPM-only gating for this implementation.Compiles this bridge only in SPM builds, which is exactly where the AblyPlugin dependency exists.
10-17: Consider weak references for client/channel to avoid retain cycles.If the properties synthesized here are strong, plugins holding on to these underlying objects may retain the client/channel longer than intended. Recommend weak unless ownership is required.
If not already weak in the header, I can draft a change once we confirm the property attributes. Want me to proceed?
5-5: Implementation matches header declaration
Verified thatAPDefaultPublicRealtimeChannelUnderlyingObjectsis declared inARTPublicRealtimeChannelUnderlyingObjects.h, so the implementation aligns with the imported header. No changes required.Source/include/module.modulemap (2)
138-141: Private exposure of plugin-related headers looks consistent.Adding ARTPluginDecodingContext.h, ARTPluginAPI.h, and ARTPublicRealtimeChannelUnderlyingObjects.h under Ably.Private matches the SPM-gated design.
138-141: All ModuleMap Headers Present
All three headers referenced inSource/include/module.modulemapwere found at their declared paths. Module imports should resolve correctly—no further changes needed.Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h (1)
1-2: Good: header is SPM-gated.This keeps AP* symbols out of non-SPM builds while allowing SPM to compile the plugin bridge.
Also applies to: 21-22
Source/PrivateHeaders/Ably/ARTPluginAPI.h (1)
1-5: SPM import validated: module “AblyPlugin” matches Package.swift
The Package.swift declares
- .package(url: "https://github.com/ably/ably-cocoa-plugin-support", …)
- .product(name: "AblyPlugin", package: "ably-cocoa-plugin-support")
which aligns with the
@import AblyPluginguarded byABLY_IS_SPM_BUILD. No changes required.Source/ARTRealtimeChannel.m (4)
709-713: Safe LiveObjects plugin callback on channel attachmentThe invocation is properly wrapped in
ABLY_IS_SPM_BUILD. Sending a message to a nil plugin instance is safe in Objective-C, so this won’t crash if the plugin isn’t configured.
901-905: Object protocol message handoff is correctly guardedOnly SPM builds will forward object messages to the plugin. The early return on
!pm.stateavoids unnecessary calls when decoding/setup failed.
913-918: Object sync handoff is correctly guardedLikewise for sync messages, the plugin hook is only active for SPM builds. Looks good.
40-43: No missing forward declaration for APObjectMessageProtocolThe
APObjectMessageProtocolis already forward-declared inARTRealtimeChannel+Private.h(whichARTRealtimeChannel.mimports) outside of anyABLY_IS_SPM_BUILDguard. The existing setup compiles correctly in both SPM and non-SPM builds, so the proposed forward-declaration inARTRealtimeChannel.misn’t needed. Disregard the suggested diff.Likely an incorrect or invalid review comment.
Source/ARTClientOptions.m (2)
13-16: Good: plugin-specific imports are SPM-onlyMoving
@import AblyPlugin;behindABLY_IS_SPM_BUILDreduces coupling for non-SPM consumers and addresses accidental linkage concerns.Confirm your build defines
ABLY_IS_SPM_BUILDonly for the SPM product and that non-SPM (CocoaPods/Carthage/Xcodeproj) configurations do not define it. If helpful, I can suggest an xcconfig snippet.
247-257: LiveObjects accessor is correctly isolated to SPM builds
- The plugin class lookup via
self.plugins[ARTPluginNameLiveObjects]is clean.- Returning
nilif not present is the right contract.- The bridging to the internal plugin via
[publicPlugin internalPlugin]keeps public/private boundaries intact.Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (1)
3-5: SPM-only import avoids leaking AblyPlugin into non-SPM headersGuarding
@import AblyPlugin;prevents non-SPM integrators from seeing plugin types, aligning with the packaging goals.Ably.xcodeproj/project.pbxproj (2)
1934-1946: Nice separation: new “Plugin Support” groupGood call extracting plugin-related files into a dedicated group. This improves discoverability and future maintenance.
3479-3527: Sources wired into all platformsThe new sources (
ARTPluginAPI.m,ARTPublicRealtimeChannelUnderlyingObjects.m, andARTPluginDecodingContext.m) are correctly added to iOS, macOS, and tvOS targets.Also applies to: 3742-3786, 3884-3926
Source/ARTJsonLikeEncoder.m (1)
33-36: SPM-only import of AblyPlugin looks correctThe conditional module import is scoped properly to SPM builds and avoids leaking plugin symbols to non-SPM integrations.
Source/ARTPluginAPI.m (6)
6-7: Headers look fine; verify no missing plugin declarationsGiven the use of
APRegisterand other AP* types, double-check that these headers (or@import AblyPlugin) bring their declarations into scope for this TU.
68-68: Implementation scope under SPM looks goodThe class is correctly compiled only for SPM builds; aligns with the packaging split.
70-76: Singleton-style registration is fine; ensure it’s invoked
+registerSelfusesdispatch_once, which is good. Confirm the call site is wired early enough in SPM builds so plugins can discover the API before use.
122-126: sendObjectWithObjectMessages bridging looks correctThe bridge delegates to
sendStateWithObjectMessages:completion:on the internal channel; matches the new state flow.
127-130: stateForChannel depends on the enum converter fixOnce
_convertOurRealtimeChannelStatehandlesFailedcorrectly, this will report the right value for all states.
144-144: SPM guard terminatorEnd guard present; file won’t leak into non-SPM builds.
lawrence-forooghian
force-pushed
the
ECO-5531-move-plugin-API-to-separate-repo
branch
from
90ac3d2 to
1a25f27
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/jazzydoc
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/ARTJsonLikeEncoder.m (1)
1124-1173: Initialize error before using; avoid logging an uninitialized pointerARTErrorInfo *error; is used in the log on failure, but isn’t initialized. If the plugin returns nil without populating error, logging an uninitialized value is undefined behavior. Initialize to nil before passing its address.
Apply this diff:
- ARTErrorInfo *error; + ARTErrorInfo *error = nil; APEncodingFormat format = [self apEncodingFormatFromARTEncoderFormat:[self format]];
♻️ Duplicate comments (5)
Package.swift (1)
41-41: Build flag placement is correct; double-check all AP references are gated*Good to see ABLY_IS_SPM_BUILD defined at the target. Please ensure all AP* references are guarded consistently in headers/impls.
Run:
#!/bin/bash set -euo pipefail # Finds AP* usages and shows nearby preprocessor guards. rg -n -C3 --type=objc '\bAP[A-Za-z0-9_]*\b' Source | sed -E 's/^/-> /' echo echo "Known offender from earlier review to fix if still present:" rg -n -C2 --type=objc 'APObjectMessageProtocol' Source/PrivateHeaders/Ably/ARTProtocolMessage.h || trueAbly.xcodeproj/project.pbxproj (3)
359-361: Make plugin support headers Public (required for external consumption).Both ARTPluginAPI.h and ARTPublicRealtimeChannelUnderlyingObjects.h are added as Private headers. If the new external plugin-support package needs to import these types from Ably, they must be exposed as Public across all targets.
Apply this diff:
-21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; +21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; -21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; +21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; };If these are intentionally internal, please document how the external repo will obtain/see these headers (import path and search paths), because leaving them Private will prevent downstream import from the Ably framework artifacts.
Also applies to: 365-367
1415-1415: Place plugin headers under the Public include tree to match visibility.If you flip the PBXBuildFile attributes to Public, also move the header file references into the public header tree so they’re installed/exported predictably:
-21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPluginAPI.h; path = PrivateHeaders/Ably/ARTPluginAPI.h; sourceTree = "<group>"; }; +21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPluginAPI.h; path = include/Ably/ARTPluginAPI.h; sourceTree = "<group>"; }; -21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPublicRealtimeChannelUnderlyingObjects.h; path = PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h; sourceTree = "<group>"; }; +21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPublicRealtimeChannelUnderlyingObjects.h; path = include/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h; sourceTree = "<group>"; };This keeps public headers under Source/include/Ably and avoids consumers having to reach into PrivateHeaders.
Also applies to: 1417-1417
4267-4267: Fix HEADER_SEARCH_PATHS: current values don’t match the repository layout.
- "$(PROJECT_DIR)/Source/Private" doesn’t exist (the tree uses Source/PrivateHeaders).
- "$(SRCROOT)/include" skips Source/include.
Use both roots consistently across targets:
-HEADER_SEARCH_PATHS = "$(PROJECT_DIR)/Source/Private"; +HEADER_SEARCH_PATHS = ( + "$(SRCROOT)/Source/include", + "$(SRCROOT)/Source/PrivateHeaders", +);-HEADER_SEARCH_PATHS = "$(SRCROOT)/include"; +HEADER_SEARCH_PATHS = ( + "$(SRCROOT)/Source/include", + "$(SRCROOT)/Source/PrivateHeaders", +);Apply to all shown targets (iOS, macOS, tvOS) to prevent CI build failures and brittle include resolution.
Also applies to: 4294-4294, 4467-4467, 4508-4508, 4543-4543, 4582-4582
Source/ARTPluginAPI.m (1)
61-78: Add a default case and defined fallback; avoid “control reaches end of non-void function”The switch covers known states, but a future enum value will yield undefined return and likely a compiler warning. Add a default that asserts and returns a safe value. This also addresses the earlier review note.
Apply this diff:
static APRealtimeChannelState _convertOurRealtimeChannelState(ARTRealtimeChannelState ourRealtimeChannelState) { switch (ourRealtimeChannelState) { case ARTRealtimeChannelInitialized: return APRealtimeChannelStateInitialized; case ARTRealtimeChannelAttaching: return APRealtimeChannelStateAttaching; case ARTRealtimeChannelAttached: return APRealtimeChannelStateAttached; case ARTRealtimeChannelDetaching: return APRealtimeChannelStateDetaching; case ARTRealtimeChannelDetached: return APRealtimeChannelStateDetached; case ARTRealtimeChannelSuspended: return APRealtimeChannelStateSuspended; case ARTRealtimeChannelFailed: return APRealtimeChannelStateFailed; + default: + NSCAssert(NO, @"Unhandled ARTRealtimeChannelState: %ld", (long)ourRealtimeChannelState); + return APRealtimeChannelStateInitialized; } }
🧹 Nitpick comments (11)
Package.resolved (1)
4-12: Package.resolved revision matches Package.swiftThe commit hash
bb7b6b33169bf99a9749177dc57116e21545a24cis in sync between Package.resolved and Package.swift.Optional: Before release, consider pinning to an official semver tag (e.g.
v1.2.3) or version range instead of a raw commit hash for clearer dependency management.To guard against future drift, you can add this CI check:
#!/bin/bash set -euo pipefail # Extract revision from Package.resolved RESOLVED_REV=$(jq -r '.object.pins[] | select(.package=="ably-cocoa-plugin-support") | .state.revision' Package.resolved) # Extract revision from Package.swift SWIFT_REV=$(rg -nPo 'revision\("([0-9a-f]{7,40})"\)' Package.swift | head -n1 | sed -E 's/.*"([0-9a-f]+)".*/\1/') echo "Package.resolved revision: $RESOLVED_REV" echo "Package.swift revision: $SWIFT_REV" if [[ "$RESOLVED_REV" != "$SWIFT_REV" ]]; then echo "Mismatch detected"; exit 1 else echo "Revisions match" fiSource/PrivateHeaders/Ably/ARTInternalLog.h (1)
78-79: Doc tweak for clarityMinor wording to read better in the sentence about plugin conformance.
Apply:
- - `ARTPluginAPI`, to implement its conformance to the `APPluginAPI` protocol, which is used by plugins written in Swift + - `ARTPluginAPI`, to implement conformance to the `APPluginAPI` protocol used by plugins written in SwiftDocs/plugins.md (3)
22-23: Tone down guidance on removing prefixes (Swift naming)Consider cautioning against removing identity-defining prefixes wholesale; keep names discoverable and avoid collisions.
Apply:
-- Make good use of `NS_SWIFT_NAME` to make `_AblyPluginSupportPrivate` nicer to use from Swift (e.g. to remove the `AP` prefix from class names). +- Use `NS_SWIFT_NAME` to improve Swift ergonomics. Prefer targeted renames; avoid removing the `AP` prefix wholesale to preserve identity and reduce collision risk.
26-29: Grammar/style fixes and clearer wording for the TODO notesTighten phrasing, use “cannot”, and punctuate “etc.”. Also clarify “stuff”.
Apply:
-## Notes TODO incorporate - -- all of the stuff that depends on `_AblyPluginSupportPrivate` is gated behind `#ifdef ABLY_IS_SPM_BUILD` so as not to affect the non-SPM builds, which do not have access to `_AblyPluginSupportPrivate` -- note that _AblyPluginSupportPrivate can not depend on ably-cocoa at all, which means having to duplicate some types (e.g. enums), or having to weaken some classes e.g. the public types such as `ARTRealtimeChannel`, `ARTClientOptions`, `ARTErrorInfo` etc - - TODO we can probably improve this a bit by at least introducing some empty marker protocols and declaring conformance -- mention that we are now going to not have any instance methods on the channel or realtime because then you have to deal with clashes because our `state` doesn't return the same type as their `state`. same for logger and its level +## Notes (to incorporate) + +- All code that depends on `_AblyPluginSupportPrivate` is gated behind `#ifdef ABLY_IS_SPM_BUILD` so as not to affect non-SPM builds, which do not have access to `_AblyPluginSupportPrivate`. +- Note that `_AblyPluginSupportPrivate` cannot depend on ably-cocoa at all, which means duplicating some types (e.g. enums) or weakening some classes (e.g. public types such as `ARTRealtimeChannel`, `ARTClientOptions`, `ARTErrorInfo`, etc.). + - TODO: We can probably improve this by introducing empty marker protocols and declaring conformance. +- We will avoid adding instance methods on the channel or realtime types to prevent clashes (e.g. differing `state` types). Same applies to the logger and its `level`.
18-18: Testing strategy — consider keeping a minimal contract test in this repoEven if most tests live in the plugin repo, a minimal contract/integration test here can catch accidental API drift in the gated surface.
I can propose a lightweight test harness that compiles a stub plugin against the AP* headers under ABLY_IS_SPM_BUILD to verify linkage and symbol visibility. Want me to sketch that?
Package.swift (1)
20-23: Revision-pinned dependency — update to a tagged version before releaseWe’ve confirmed that
ably-cocoa-plugin-supportis currently pinned to commitbb7b6b33169bf99a9749177dc57116e21545a24cin both Package.swift and Package.resolved. For a predictable, auditable release, please switch this to a semantic version tag (e.g..package(url: "...", from: "1.2.3")) before shipping.• Location: Package.swift line 22
• Current pin:.package(url: "https://github.com/ably/ably-cocoa-plugin-support", .revision("bb7b6b33169bf99a9749177dc57116e21545a24c"))• Confirmed in Package.resolved under
"state".revisionLink or create an issue to track the readiness of the tagged release, then update the manifest and run a resolution sync:
rg -n 'ably-cocoa-plugin-support.*revision' Package.swift jq -r '.object.pins[] | select(.package=="ably-cocoa-plugin-support")' Package.resolvedSource/ARTJsonLikeEncoder.m (4)
33-36: SPM-only import is correct; consider aligning other plugin-specific imports too@import _AblyPluginSupportPrivate is correctly gated under ABLY_IS_SPM_BUILD. If ARTPluginDecodingContext.h is plugin-only, consider gating that import similarly to avoid non-SPM build breakage from a missing header.
581-586: Encoding state into protocol message: OK; ensure objectMessagesToArray never returns nil for non-nil inputThis looks correct. Minor robustness suggestion: objectMessagesToArray currently returns nil only if input is nil (per doc), but it also raises if the plugin is missing. If you ever relax that behavior, ensure the value assigned via subscripting is never nil for non-nil state, to avoid implicit key removal.
1112-1123: Make switch exhaustive or add a defensive default to avoid UB/warningsapEncodingFormatFromARTEncoderFormat lacks a default; if ARTEncoderFormat gains a new case, this function may return an indeterminate value. Recommend adding a default with a log/assert and a safe fallback.
Apply this diff:
- (APEncodingFormat)apEncodingFormatFromARTEncoderFormat:(ARTEncoderFormat)format { switch (format) { case ARTEncoderFormatJson: return APEncodingFormatJSON; case ARTEncoderFormatMsgPack: return APEncodingFormatMessagePack; + default: + // Defensive: future-proof against new encoder formats + ARTLogWarn(_logger, @"Unknown ARTEncoderFormat %ld; defaulting to JSON", (long)format); + return APEncodingFormatJSON; } }
1178-1201: Invariant handling: replace exception with assert in debug and warning in releaseRaising NSInternalInconsistencyException here will crash release builds. Since this method already returns nil on invalid inputs, prefer:
- Use
NSAssertto catch the missing plugin in debug builds- Log a warning and return nil in release
Also, update the method’s return annotation to reflect that it may return nil:
- (NSArray<NSDictionary *> *)objectMessagesToArray:(nullable NSArray<id<APObjectMessageProtocol>> *)objectMessages { + (nullable NSArray<NSDictionary *> *)objectMessagesToArray:(nullable NSArray<id<APObjectMessageProtocol>> *)objectMessages { if (!objectMessages) { return nil; } id<APLiveObjectsInternalPluginProtocol> liveObjectsPlugin = _rest.options.liveObjectsPlugin; - if (!liveObjectsPlugin) { - // The only thing that sends ObjectMessage is the LiveObjects plugin, so if we have some to encode then the plugin must be present - [NSException raise:NSInternalInconsistencyException format:@"Attempted to encode ObjectMessages without a LiveObjects plugin; this should not be possible."]; - return nil; - } + if (!liveObjectsPlugin) { + NSAssert(liveObjectsPlugin != nil, @"Attempted to encode ObjectMessages without a LiveObjects plugin; this should not be possible."); + ARTLogWarn(_logger, @"ARTJsonLikeEncoder<%@>: missing LiveObjects plugin during ObjectMessage encoding; skipping state", [_delegate formatAsString]); + return nil; + } NSMutableArray<NSDictionary *> *result = [NSMutableArray array]; APEncodingFormat format = [self apEncodingFormatFromARTEncoderFormat:[self format]]; for (id objectMessage in objectMessages) { [result addObject:[liveObjectsPlugin encodeObjectMessage:objectMessage format:format]]; } return result; }
- Location: Source/ARTJsonLikeEncoder.m:1178–1201
This change ensures you catch the invariant in debug, avoid crashing in production, and keep your annotations in sync with runtime behavior.
Source/ARTPluginAPI.m (1)
99-105: Confirm retention semantics of the registered instanceregisterSelf creates a short-lived instance and hands it to APRegister. If APRegister doesn’t retain it, the API could be deallocated.
If APRegister retains: no change needed. If it doesn’t, keep a strong static:
- dispatch_once(&onceToken, ^{ - ARTPluginAPI *instanceToRegister = [[ARTPluginAPI alloc] init]; - [[APRegister sharedInstance] registerPluginAPI:instanceToRegister]; - }); + dispatch_once(&onceToken, ^{ + sRegisteredPluginAPI = [[ARTPluginAPI alloc] init]; + [[APRegister sharedInstance] registerPluginAPI:sRegisteredPluginAPI]; + });Add near the top of the implementation:
static ARTPluginAPI *sRegisteredPluginAPI;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (34)
.github/workflows/check-plugin.yaml(0 hunks)Ably.podspec(0 hunks)Ably.xcodeproj/project.pbxproj(22 hunks)AblyPlugin/APPluginAPI.m(0 hunks)AblyPlugin/PrivateHeaders/ARTInternalLog+APLogger.h(0 hunks)AblyPlugin/PrivateHeaders/ARTRealtimeChannelInternal+APRealtimeChannel.h(0 hunks)AblyPlugin/PrivateHeaders/ARTRealtimeInternal+APRealtimeClient.h(0 hunks)AblyPlugin/include/APDecodingContext.h(0 hunks)AblyPlugin/include/APEncodingFormat.h(0 hunks)AblyPlugin/include/APLiveObjectsPlugin.h(0 hunks)AblyPlugin/include/APLogger.h(0 hunks)AblyPlugin/include/APPluginAPI.h(0 hunks)AblyPlugin/include/APPublicRealtimeChannelUnderlyingObjects.h(0 hunks)AblyPlugin/include/APRealtimeChannel.h(0 hunks)AblyPlugin/include/APRealtimeClient.h(0 hunks)CONTRIBUTING.md(0 hunks)Docs/plugins.md(1 hunks)Package.resolved(1 hunks)Package.swift(2 hunks)Source/ARTClientOptions.m(4 hunks)Source/ARTJsonLikeEncoder.m(6 hunks)Source/ARTPluginAPI.m(1 hunks)Source/ARTPluginDecodingContext.m(2 hunks)Source/ARTPublicRealtimeChannelUnderlyingObjects.m(2 hunks)Source/ARTRealtimeChannel.m(5 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTInternalLog.h(3 hunks)Source/PrivateHeaders/Ably/ARTPluginAPI.h(1 hunks)Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h(2 hunks)Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtime+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/include/module.modulemap(1 hunks)
💤 Files with no reviewable changes (15)
- AblyPlugin/include/APLogger.h
- AblyPlugin/PrivateHeaders/ARTInternalLog+APLogger.h
- AblyPlugin/include/APEncodingFormat.h
- AblyPlugin/PrivateHeaders/ARTRealtimeInternal+APRealtimeClient.h
- CONTRIBUTING.md
- AblyPlugin/APPluginAPI.m
- AblyPlugin/include/APRealtimeClient.h
- Ably.podspec
- .github/workflows/check-plugin.yaml
- AblyPlugin/PrivateHeaders/ARTRealtimeChannelInternal+APRealtimeChannel.h
- AblyPlugin/include/APDecodingContext.h
- AblyPlugin/include/APPluginAPI.h
- AblyPlugin/include/APLiveObjectsPlugin.h
- AblyPlugin/include/APPublicRealtimeChannelUnderlyingObjects.h
- AblyPlugin/include/APRealtimeChannel.h
🚧 Files skipped from review as they are similar to previous changes (9)
- Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h
- Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h
- Source/Ably.modulemap
- Source/ARTPluginDecodingContext.m
- Source/PrivateHeaders/Ably/ARTRealtime+Private.h
- Source/ARTRealtimeChannel.m
- Source/PrivateHeaders/Ably/ARTClientOptions+Private.h
- Source/include/module.modulemap
- Source/ARTClientOptions.m
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-04T23:10:23.027Z
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/ARTPublicRealtimeChannelUnderlyingObjects.hSource/PrivateHeaders/Ably/ARTPluginAPI.hSource/ARTPublicRealtimeChannelUnderlyingObjects.mSource/ARTJsonLikeEncoder.mSource/ARTPluginAPI.mAbly.xcodeproj/project.pbxproj
📚 Learning: 2024-11-04T23:11:06.909Z
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:
Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h
📚 Learning: 2024-10-08T15:37:26.766Z
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.
Applied to files:
Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h
📚 Learning: 2024-10-08T15:37:26.766Z
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/ARTPublicRealtimeChannelUnderlyingObjects.mSource/PrivateHeaders/Ably/ARTInternalLog.hSource/ARTJsonLikeEncoder.mAbly.xcodeproj/project.pbxproj
📚 Learning: 2024-11-26T17:50:25.730Z
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/ARTJsonLikeEncoder.m
🪛 SwiftLint (0.57.0)
Package.swift
[Warning] 21-21: TODOs should be resolved (Unpin this before releasing Li...)
(todo)
🪛 LanguageTool
Docs/plugins.md
[grammar] ~14-~14: There might be a mistake here.
Context: ...y-cocoa and _AblyPluginSupportPrivate: - ably-cocoa imports `_AblyPluginSupportPr...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...'s internals to plugins via PluginAPI. - Currently, the plan is to test all the L...
(QB_NEW_EN)
[style] ~26-~26: Consider removing “of” to be more concise
Context: ...s names). ## Notes TODO incorporate - all of the stuff that depends on `_AblyPluginSuppo...
(ALL_OF_THE)
[style] ~27-~27: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...` - note that _AblyPluginSupportPrivate can not depend on ably-cocoa at all, which mean...
(CAN_NOT_PREMIUM)
[style] ~27-~27: In American English, abbreviations like “etc.” require a period.
Context: ...el, ARTClientOptions, ARTErrorInfo` etc - TODO we can probably improve this...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
Docs/plugins.md
9-9: Bare URL used
(MD034, no-bare-urls)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ 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 (macOS, test_macOS)
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
🔇 Additional comments (18)
Package.resolved (1)
4-12: Revision pin added for ably-cocoa-plugin-support — OK for WIPPinning to a commit is fine for a WIP branch and ensures reproducible CI.
Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h (2)
1-1: Good SPM-only gating around the entire headerWrapping the whole header with ABLY_IS_SPM_BUILD keeps AP* symbols out of non-SPM builds, which aligns with the packaging goals.
Also applies to: 17-17
4-4: Confirm SPM module availability for_AblyPluginSupportPrivate
@import _AblyPluginSupportPrivate;assumes that the plugin‐support Swift-PM package exports a Clang module with exactly that name (including case and underscores). Please verify and adjust as needed:• Check the plugin-support repo’s Package.swift and any module.modulemap to ensure there is a target or product exporting
_AblyPluginSupportPrivate.
• If the actual module name differs, update this import to match, for example:@import ActualModuleName; // or #import <ActualModuleName/Header.h>• If no Clang module is currently defined, add a
module.modulemapunder the plugin-support target that exposes the private headers under_AblyPluginSupportPrivate.Source/PrivateHeaders/Ably/ARTInternalLog.h (2)
4-7: SPM-only import of _AblyPluginSupportPrivate looks rightThe conditional import prevents leaking AP* types into non-SPM builds.
40-45: Verify APLogger protocol conformanceUnder
ABLY_IS_SPM_BUILD,ARTInternalLogadoptsAPLogger. We couldn’t locate theAPLoggerprotocol definition in this repo, so please:
- Inspect the
APLoggerprotocol in the SPM-provided plugin API.- Confirm it declares exactly:
(and no additional required selectors).- (void)log:(NSString *)message withLevel:(APLogLevel)level file:(const char *)fileName line:(NSInteger)line;- If the protocol has extra methods, add corresponding shims in
ARTInternalLog.Location:
- Source/PrivateHeaders/Ably/ARTInternalLog.h lines 40–45
Package.swift (1)
41-53: Header search paths — OKRetaining PrivateHeaders and include/Ably paths without AblyPlugin search paths aligns with the separation work.
Source/ARTPublicRealtimeChannelUnderlyingObjects.m (2)
21-21: LGTM: closed SPM guardThe #endif is correctly closing the compilation guard.
1-3: No issues found with SPM gating or class/header alignmentI’ve confirmed that:
- The header
ARTPublicRealtimeChannelUnderlyingObjects.h(inSource/PrivateHeaders/Ably) declares
@interface APDefaultPublicRealtimeChannelUnderlyingObjects ….- The implementation in
Source/ARTPublicRealtimeChannelUnderlyingObjects.mmatches that class.- All references to
APDefaultPublicRealtimeChannelUnderlyingObjects(e.g. inARTPluginAPI.m) are within the#ifdef ABLY_IS_SPM_BUILDguard.No changes required here.
Source/PrivateHeaders/Ably/ARTPluginAPI.h (1)
1-20: Ensure +registerSelf is thread-safe and verify module-map exposure
- We confirmed the
#ifdef ABLY_IS_SPM_BUILDgating and imports inARTPluginAPI.hare correct.ARTPluginAPI.his referenced in bothSource/Ably.modulemapandSource/include/module.modulemap. If you intended this private header to be exposed only in SPM builds, you’ll need to adjust your module-map setup. Otherwise, theABLY_IS_SPM_BUILDguard will ensure the interface is empty on non-SPM builds.- For
+registerSelf, please implement a thread-safe, idempotent pattern (e.g.dispatch_onceor an atomic flag) so it can’t register more than once under concurrent early-entry calls.Let me know if you’d like a sample
dispatch_onceimplementation inARTPluginAPI.m.Source/ARTJsonLikeEncoder.m (1)
831-834: Decoding state: OK and symmetricThe state decode path is symmetric with encoding and correctly gated to SPM. No issues.
Ably.xcodeproj/project.pbxproj (2)
356-358: LGTM: plugin sources added across all platforms.ARTPluginAPI.m and ARTPublicRealtimeChannelUnderlyingObjects.m are wired into iOS, macOS, and tvOS targets. Once header visibility is corrected, the compilation scope looks correct.
If the intention is to keep these symbols internal for CocoaPods/SPM consumers, consider wrapping any plugin-only codepaths behind an internal flag to avoid accidental exposure or linkage side effects.
Also applies to: 362-364, 3484-3484, 3743-3743, 3884-3884
1934-1946: Project structure cleanup is clear.The new “Plugin Support” group under Source cleanly scopes the plugin files; no issues spotted with group membership or file refs.
Also applies to: 2088-2089
Source/ARTPluginAPI.m (6)
10-59: Bridging helpers are defensive and clearThe cast-and-assert helpers for public/internal types are concise and fail-fast with actionable messages. This pattern is consistent and easy to audit.
107-112: Underlying objects handoff looks correctPassing realtimeInternal and internal to APDefaultPublicRealtimeChannelUnderlyingObjects aligns with the bridging strategy.
154-156: State exposure via mapping is straightforwardThis is a thin, sensible bridge over internal state.
158-167: Logger forwarding is clean; level mapping covers all known casesOnce the default is added to the level converter, this is solid.
149-152: sendStateWithObjectMessages:completion: invocation is correctVerified that ARTRealtimeChannelInternal declares
-sendStateWithObjectMessages:completion:in PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (lines 142–145) and implements it in ARTRealtimeChannel.m (line 422). No renaming needed.
1-9: SPM macro naming is consistent across the codebaseConfirmed that
ABLY_IS_SPM_BUILDis the sole variant used in all relevant files and no misspelledABLY_ISPM_BUILDoccurrences exist.
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/jazzydoc
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Source/ARTJsonLikeEncoder.m (2)
1112-1122: Add a default/fallback branch to avoid missing return path if new encoder formats are addedThe switch lacks a default/fallback, which can lead to a missing return path if ARTEncoderFormat gains new cases. Assert and return a sensible default to keep the function total.
- (APEncodingFormat)apEncodingFormatFromARTEncoderFormat:(ARTEncoderFormat)format { switch (format) { case ARTEncoderFormatJson: return APEncodingFormatJSON; case ARTEncoderFormatMsgPack: return APEncodingFormatMessagePack; } + NSCAssert(NO, @"Unhandled ARTEncoderFormat: %ld", (long)format); + return APEncodingFormatJSON; }
1124-1200: Nullability mismatch and unreachable code in objectMessagesToArray; tighten types and logging
- The method returns nil when objectMessages is nil, but (per the PR summary) its signature is nonnull. Either return an empty array or make the return type nullable to match behavior.
- After raising NSInternalInconsistencyException, the subsequent return nil is unreachable.
- Minor: tighten the for-in variable type to id.
- Optional: log at debug level when the plugin is absent on decode to make the drop explicit.
Option A (keep nonnull return): return an empty array and remove the unreachable return.
- (NSArray<NSDictionary *> *)objectMessagesToArray:(nullable NSArray<id<APObjectMessageProtocol>> *)objectMessages { - if (!objectMessages) { - return nil; - } + if (!objectMessages) { + return @[]; + } id<APLiveObjectsInternalPluginProtocol> liveObjectsPlugin = _rest.options.liveObjectsPlugin; if (!liveObjectsPlugin) { // The only thing that sends ObjectMessage is the LiveObjects plugin, so if we have some to encode then the plugin must be present [NSException raise:NSInternalInconsistencyException format:@"Attempted to encode ObjectMessages without a LiveObjects plugin; this should not be possible."]; - return nil; } NSMutableArray<NSDictionary *> *result = [NSMutableArray array]; APEncodingFormat format = [self apEncodingFormatFromARTEncoderFormat:[self format]]; - for (id objectMessage in objectMessages) { + for (id<APObjectMessageProtocol> objectMessage in objectMessages) { [result addObject:[liveObjectsPlugin encodeObjectMessage:objectMessage format:format]]; }Option B (change API contract): make the return type nullable in the header to match current behavior. Apply in the header (outside this diff):
// In Source/ARTJsonLikeEncoder.h - (nullable NSArray<NSDictionary *> *)objectMessagesToArray:(nullable NSArray<id<APObjectMessageProtocol>> *)objectMessages;Optional: add a debug log when the plugin is absent on decode:
- if (!liveObjectsPlugin) { - return nil; - } + if (!liveObjectsPlugin) { + ARTLogDebug(_logger, @"RS:%p ARTJsonLikeEncoder<%@>: LiveObjects plugin not supplied; dropping 'state' on decode", _rest, [_delegate formatAsString]); + return nil; + }
♻️ Duplicate comments (6)
Docs/plugins.md (1)
11-16: Clarify SPM linking vs non-SPM and fix list indentationSPM does link _AblyPluginSupportPrivate; non-SPM does not. Also fixes MD007 indentation.
-## Notes on `_AblyPluginSupportPrivate` +## Notes on `_AblyPluginSupportPrivate` - Written in Objective-C (so that it can access ably-cocoa's private headers). -- There is a two-way dependency between ably-cocoa and `_AblyPluginSupportPrivate`: - - ably-cocoa imports `_AblyPluginSupportPrivate`'s plugin headers (e.g. `APLiveObjectsPlugin.h`) so that it can access the methods exposed by plugins. However, ably-cocoa does not _link_ to `_AblyPluginSupportPrivate`, and so it cannot depend on any classes defined in `_AblyPluginSupportPrivate`. - - `_AblyPluginSupportPrivate` imports ably-cocoa's private headers so that it can expose ably-cocoa's internals to plugins via `PluginAPI`. +- There is a compile-time two-way surface between ably-cocoa and `_AblyPluginSupportPrivate`: + - In SPM builds, ably-cocoa both imports and links `_AblyPluginSupportPrivate` (Package.swift adds it as a target dependency), with AP* usage gated behind `ABLY_SUPPORTS_PLUGINS`. + - In non-SPM builds (e.g. Xcode project/CocoaPods), ably-cocoa does not link `_AblyPluginSupportPrivate`; it only exposes headers conditionally, and `_AblyPluginSupportPrivate` imports ably-cocoa's private headers to expose internals via `PluginAPI`.Source/ARTPluginDecodingContext.m (1)
1-1: Follow-up: ensure all imports/usages are also gated (ARTJsonLikeEncoder.m in particular)Past feedback suggested guarding import/use sites of ARTPluginDecodingContext. With the macro now ABLY_SUPPORTS_PLUGINS, verify that imports/usages (e.g., in ARTJsonLikeEncoder.m) are wrapped accordingly.
Run:
#!/bin/bash set -euo pipefail echo "Find import sites for ARTPluginDecodingContext.h and ensure nearby ABLY_SUPPORTS_PLUGINS guards:" rg -nC3 '#import\s+"ARTPluginDecodingContext\.h"' -g '*.m' -g '*.mm' -g '*.h' || true echo echo "Scan for legacy ABLY_IS_SPM_BUILD guards that should have been migrated:" rg -n 'ABLY_IS_SPM_BUILD' -g '*.m' -g '*.mm' -g '*.h' || trueAbly.xcodeproj/project.pbxproj (2)
4275-4275: Unify and correct HEADER_SEARCH_PATHS to match the repo layout (Source/include, Source/PrivateHeaders)Current values point to non-existent or wrong roots (e.g., "$(PROJECT_DIR)/Source/Private", "$(SRCROOT)/include"). Align them across all targets to avoid brittle includes and CI breakage.
Apply this diff at each occurrence shown in the selected lines:
-HEADER_SEARCH_PATHS = "$(PROJECT_DIR)/Source/Private"; +HEADER_SEARCH_PATHS = ( + "$(SRCROOT)/Source/include", + "$(SRCROOT)/Source/PrivateHeaders", +); -HEADER_SEARCH_PATHS = "$(SRCROOT)/include"; +HEADER_SEARCH_PATHS = ( + "$(SRCROOT)/Source/include", + "$(SRCROOT)/Source/PrivateHeaders", +);Also applies to: 4302-4302, 4476-4476, 4516-4516, 4551-4551, 4590-4590
359-361: Make plugin API headers Public for external plugin-supportTo allow the separate
ably-cocoa-plugin-supportrepo (and downstream SPM/framework consumers) to import these plugin API headers, they must be marked Public rather than Private in the Xcode project.Please update the following in
Ably.xcodeproj/project.pbxproj:• Lines 359–361 (ARTPluginAPI.h in Headers)
• Lines 365–367 (ARTPublicRealtimeChannelUnderlyingObjects.h in Headers)Example diff:
- 21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; - 21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; - 21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; + 21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; + 21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; + 21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; - 21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; - 21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; - 21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; + 21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; + 21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; + 21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; };If these headers are intended to remain internal-only, please explain how
ably-cocoa-plugin-supportimports them without Public visibility (note that SPM and downstream frameworks cannot access Private headers).Source/ARTPluginAPI.m (2)
53-70: Add a default branch to future‑proof enum conversion and avoid undefined returnGood catch fixing the wrong enum label to ARTRealtimeChannelFailed. However, the switch still lacks a default, so a future enum value would lead to undefined return.
Apply this diff:
static APRealtimeChannelState _convertOurRealtimeChannelState(ARTRealtimeChannelState ourRealtimeChannelState) { switch (ourRealtimeChannelState) { case ARTRealtimeChannelInitialized: return APRealtimeChannelStateInitialized; case ARTRealtimeChannelAttaching: return APRealtimeChannelStateAttaching; case ARTRealtimeChannelAttached: return APRealtimeChannelStateAttached; case ARTRealtimeChannelDetaching: return APRealtimeChannelStateDetaching; case ARTRealtimeChannelDetached: return APRealtimeChannelStateDetached; case ARTRealtimeChannelSuspended: return APRealtimeChannelStateSuspended; case ARTRealtimeChannelFailed: return APRealtimeChannelStateFailed; + default: + NSCAssert(NO, @"Unhandled ARTRealtimeChannelState: %ld", (long)ourRealtimeChannelState); + return APRealtimeChannelStateInitialized; } }
72-87: Also add a default for log level conversionSame rationale: ensure a defined return if the plugin adds a new APLogLevel in the future.
static ARTLogLevel _convertPluginLogLevel(APLogLevel pluginLogLevel) { switch (pluginLogLevel) { case APLogLevelVerbose: return ARTLogLevelVerbose; case APLogLevelDebug: return ARTLogLevelDebug; case APLogLevelInfo: return ARTLogLevelInfo; case APLogLevelWarn: return ARTLogLevelWarn; case APLogLevelError: return ARTLogLevelError; case APLogLevelNone: return ARTLogLevelNone; + default: + NSCAssert(NO, @"Unhandled APLogLevel: %ld", (long)pluginLogLevel); + return ARTLogLevelNone; } }
🧹 Nitpick comments (13)
Docs/plugins.md (3)
5-5: Name the external repository to aid discoverabilityExplicitly reference the new repo that provides the private support library.
-ably-cocoa exposes a separate library called `_AblyPluginSupportPrivate`. This provides an API that plugins can use to access ably-cocoa's internals. +ably-cocoa exposes a separate library called `_AblyPluginSupportPrivate` (from https://github.com/ably/ably-cocoa-plugin-support). This provides an API that plugins can use to access ably-cocoa's internals.
9-9: Avoid bare URL; use a proper Markdown linkFixes MD034 (no-bare-urls).
-Currently, our only plugin is for adding LiveObjects functionality. This plugin can be found at https://github.com/ably/ably-cocoa-liveobjects-plugin. +Currently, our only plugin is for adding LiveObjects functionality. See the LiveObjects plugin repo: <https://github.com/ably/ably-cocoa-liveobjects-plugin>.
24-29: Polish wording and tighten the TODO notesConcise language, American English “cannot”, and “etc.” period. Also adjust phrasing to avoid negatives.
-## Notes TODO incorporate +## Notes TODO incorporate -- all of the stuff that depends on `_AblyPluginSupportPrivate` is gated behind `#ifdef ABLY_SUPPORTS_PLUGINS` so as not to affect the non-SPM builds, which do not have access to `_AblyPluginSupportPrivate` -- note that _AblyPluginSupportPrivate can not depend on ably-cocoa at all, which means having to duplicate some types (e.g. enums), or having to weaken some classes e.g. the public types such as `ARTRealtimeChannel`, `ARTClientOptions`, `ARTErrorInfo` etc - - TODO we can probably improve this a bit by at least introducing some empty marker protocols and declaring conformance -- mention that we are now going to not have any instance methods on the channel or realtime because then you have to deal with clashes because our `state` doesn't return the same type as their `state`. same for logger and its level +- all plugin-dependent code is gated behind `#ifdef ABLY_SUPPORTS_PLUGINS` to avoid affecting non-SPM builds, which do not have access to `_AblyPluginSupportPrivate` +- note that `_AblyPluginSupportPrivate` cannot depend on ably-cocoa at all, which means duplicating some types (e.g. enums), or weakening some public types such as `ARTRealtimeChannel`, `ARTClientOptions`, `ARTErrorInfo`, etc. + - TODO: we can likely improve this by introducing empty marker protocols and declaring conformance +- prefer not to add instance methods on channel or realtime that would clash with AP types (e.g. `state` returning different types). Same for logger and its levelSource/PrivateHeaders/Ably/ARTClientOptions+Private.h (2)
26-28: Naming nit: avoid “get” prefix in ObjC boolean accessorsApple’s conventions reserve “get” for out-parameters. Consider a name that reads as a value query.
-+ (BOOL)getDefaultIdempotentRestPublishingForVersion:(NSString *)version; ++ (BOOL)defaultIdempotentRestPublishingForVersion:(NSString *)version;
32-35: Make property nonatomic to avoid implicit atomicityProperties are atomic by default; use
nonatomichere since thread-safe access is not implied and atomic adds overhead.-@property (nullable, readonly) id<APLiveObjectsInternalPluginProtocol> liveObjectsPlugin; +@property (nullable, nonatomic, readonly) id<APLiveObjectsInternalPluginProtocol> liveObjectsPlugin;Source/ARTStatus.m (1)
5-5: Consider guarding the private header import or removing if unusedIf this file doesn’t require symbols from
ARTErrorInfo+Private.h, either remove the import or guard it to mirror non-SPM builds.-#import "ARTErrorInfo+Private.h" +#ifdef ABLY_SUPPORTS_PLUGINS +#import "ARTErrorInfo+Private.h" +#endifIf it’s needed for a category adoption with only guarded content, the guard keeps intent explicit without changing behavior.
Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h (1)
1-5: Good gating; optionally reduce coupling by forward-declaring protocolsThe
@import _AblyPluginSupportPrivate;creates a hard module dependency for this header. Forward-declaring the AP protocols keeps the header lighter while retaining type safety.#ifdef ABLY_SUPPORTS_PLUGINS -@import Foundation; -@import _AblyPluginSupportPrivate; +@import Foundation; + +@protocol APPublicRealtimeChannelUnderlyingObjects; +@protocol APRealtimeClient; +@protocol APRealtimeChannel;Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h (1)
2-2: Decouple from ARTStatus.h to avoid unnecessary dependency cyclesThis header only needs the ARTErrorInfo type for the category declaration; importing ARTStatus.h here increases coupling and risks circular imports. Forward-declare ARTErrorInfo instead.
Apply this diff:
-#import "ARTStatus.h" +@class ARTErrorInfo;Source/ARTRealtimeChannel.m (1)
68-77: Make plugin registration idempotent (avoid per-channel overhead)Registering the plugin API on every channel init is safe but redundant. Wrap in dispatch_once to ensure one-time registration per process.
Apply this diff:
- // The LiveObjects repository provides an extension to `ARTRealtimeChannel` so we need to ensure that we register the pluginAPI before that extension is used. - [ARTPluginAPI registerSelf]; + // The LiveObjects repository provides an extension to `ARTRealtimeChannel` so we need to ensure that we register the pluginAPI before that extension is used. + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + [ARTPluginAPI registerSelf]; + });Source/ARTJsonLikeEncoder.m (2)
581-585: Avoid assigning nil into the dictionary if encoding returns nilWhile objectMessagesToArray currently raises before it can return nil when the plugin is missing, guarding the assignment prevents future regressions and avoids potential exceptions from setting a nil value via keyed subscripting.
#ifdef ABLY_SUPPORTS_PLUGINS - if (message.state) { - output[@"state"] = [self objectMessagesToArray:message.state]; - } + if (message.state) { + NSArray<NSDictionary *> *state = [self objectMessagesToArray:message.state]; + if (state) { + output[@"state"] = state; + } + } #endif
831-834: Decoding 'state' is silent when plugin is absent — consider a debug log to aid diagnosticsWhen the plugin is not supplied, state is quietly dropped (objectMessagesFromArray returns nil). A one-line ARTLogDebug when the plugin is absent would make this behavior explicit during troubleshooting.
Ably.xcodeproj/project.pbxproj (1)
1417-1420: Optional: consider relocating Public plugin headers under include/Ably for consistencyIf
ARTPluginAPI.handARTPublicRealtimeChannelUnderlyingObjects.hbecome Public, consider moving them under include/Ably to match the rest of the public surface, simplifying consumer include paths and umbrella management.Example change (requires moving files accordingly):
-21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPluginAPI.h; path = PrivateHeaders/Ably/ARTPluginAPI.h; sourceTree = "<group>"; }; +21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPluginAPI.h; path = include/Ably/ARTPluginAPI.h; sourceTree = "<group>"; }; -21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPublicRealtimeChannelUnderlyingObjects.h; path = PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h; sourceTree = "<group>"; }; +21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPublicRealtimeChannelUnderlyingObjects.h; path = include/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h; sourceTree = "<group>"; };Source/ARTPluginAPI.m (1)
11-16: Remove or use the unused _ourPublicErrorInfo helperThis helper isn’t used in this implementation. Either remove it now or wire it where error objects cross the boundary to avoid dead code.
-static ARTErrorInfo *_ourPublicErrorInfo(id<APPublicErrorInfo> pluginPublicErrorInfo) { - if (![pluginPublicErrorInfo isKindOfClass:[ARTErrorInfo class]]) { - [NSException raise:NSInternalInconsistencyException format:@"Expected ARTErrorInfo, got %@", pluginPublicErrorInfo]; - } - return (ARTErrorInfo *)pluginPublicErrorInfo; -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
Ably.xcodeproj/project.pbxproj(27 hunks)Docs/plugins.md(1 hunks)Package.resolved(1 hunks)Package.swift(2 hunks)Source/ARTClientOptions.m(4 hunks)Source/ARTJsonLikeEncoder.m(6 hunks)Source/ARTPluginAPI.m(1 hunks)Source/ARTPluginDecodingContext.m(2 hunks)Source/ARTPublicRealtimeChannelUnderlyingObjects.m(2 hunks)Source/ARTRealtimeChannel.m(5 hunks)Source/ARTStatus.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTInternalLog.h(3 hunks)Source/PrivateHeaders/Ably/ARTPluginAPI.h(1 hunks)Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h(2 hunks)Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtime+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/include/module.modulemap(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- Source/Ably.modulemap
- Package.resolved
- Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h
- Source/PrivateHeaders/Ably/ARTRealtime+Private.h
- Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h
- Source/ARTClientOptions.m
- Source/PrivateHeaders/Ably/ARTPluginAPI.h
- Source/PrivateHeaders/Ably/ARTInternalLog.h
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-04T23:10:23.027Z
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/ARTStatus.mSource/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.hSource/include/module.modulemapSource/ARTJsonLikeEncoder.mSource/ARTRealtimeChannel.mSource/ARTPublicRealtimeChannelUnderlyingObjects.mSource/ARTPluginAPI.mAbly.xcodeproj/project.pbxproj
📚 Learning: 2024-11-26T17:50:25.730Z
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/ARTJsonLikeEncoder.m
📚 Learning: 2024-10-08T15:37:26.766Z
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/ARTJsonLikeEncoder.mSource/PrivateHeaders/Ably/ARTClientOptions+Private.hSource/ARTRealtimeChannel.mSource/ARTPublicRealtimeChannelUnderlyingObjects.mAbly.xcodeproj/project.pbxproj
📚 Learning: 2025-08-13T16:50:28.236Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2084
File: Source/ARTClientOptions.m:249-251
Timestamp: 2025-08-13T16:50:28.236Z
Learning: In ARTClientOptions.m, the setPluginOptionsValue:forKey: method intentionally provides a simple dictionary-like interface without defensive copying, expecting callers to handle proper value management themselves rather than adding defensive programming overhead.
Applied to files:
Source/PrivateHeaders/Ably/ARTClientOptions+Private.h
📚 Learning: 2025-06-16T14:12:36.705Z
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
🪛 LanguageTool
Docs/plugins.md
[grammar] ~14-~14: There might be a mistake here.
Context: ...y-cocoa and _AblyPluginSupportPrivate: - ably-cocoa imports `_AblyPluginSupportPr...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...'s internals to plugins via PluginAPI. - Currently, the plan is to test all the L...
(QB_NEW_EN)
[style] ~26-~26: Consider removing “of” to be more concise
Context: ...s names). ## Notes TODO incorporate - all of the stuff that depends on `_AblyPluginSuppo...
(ALL_OF_THE)
[style] ~27-~27: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...` - note that _AblyPluginSupportPrivate can not depend on ably-cocoa at all, which mean...
(CAN_NOT_PREMIUM)
[style] ~27-~27: In American English, abbreviations like “etc.” require a period.
Context: ...el, ARTClientOptions, ARTErrorInfo` etc - TODO we can probably improve this...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
Docs/plugins.md
9-9: Bare URL used
(MD034, no-bare-urls)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 SwiftLint (0.57.0)
Package.swift
[Warning] 21-21: TODOs should be resolved (Unpin this before releasing Li...)
(todo)
⏰ 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 (tvOS, test_tvOS18_4)
- GitHub Check: build
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (24)
Docs/plugins.md (2)
18-18: LGTM: doc rationale for underscore + PrivateClear rationale and aligned with the goal to reduce accidental linkage.
22-23: LGTM: encourage NS_SWIFT_NAME usageGood guidance to improve Swift ergonomics despite AP prefixes.
Package.swift (3)
29-31: SPM correctly links the private support productLinking
_AblyPluginSupportPrivatehere is correct for SPM builds. Ensure the docs reflect this (see Docs/plugins.md fix).
20-23: Unpin commit-pinned SPM dependency before releaseThe
.revision("f7576f5311bea896ecd883cf4b53451978af7759")entry in Package.swift is acceptable for WIP, but must be replaced with a version range prior to shipping.• Location: Package.swift line 22
.package(url: "https://github.com/ably/ably-cocoa-plugin-support", .revision("f7576f5311bea896ecd883cf4b53451978af7759"))Action items:
- Add a release-checklist task to swap this revision pin for a proper version specifier.
- Verify no other hard-pinned dependencies remain by running:
rg -nC2 -P 'package\([^)]*(?:\.revision|branch|exact)\(' Package.swift
41-41: Verify plugin API guards and stale guard removalABLY_IS_SPM_BUILD occurrences have been removed (0 found), but numerous
AP*symbols still appear unguarded. Please ensure every plugin‐related type or protocol is wrapped in#ifdef ABLY_SUPPORTS_PLUGINS. Key locations to audit:
- Source/ARTRest.m around
@implementation NSData (APNS)- Source/ARTPush.m (activation state machine using
UIApplication.sharedApplication.delegate)- Source/ARTPublicRealtimeChannelUnderlyingObjects.m (
APDefaultPublicRealtimeChannelUnderlyingObjects)- Source/ARTRealtimeChannel.m (uses of
APLiveObjectsInternalPluginProtocol)- Source/ARTClientOptions.m (the
liveObjectsPluginproperty)- Source/ARTPluginAPI.m (all
AP*protocol casts and registrations)- Source/ARTJsonLikeEncoder.m (methods using
APEncodingFormatandAPObjectMessageProtocol)- PrivateHeaders (e.g., ARTRealtime+Private.h, ARTInternalLog.h, ARTErrorInfo+Private.h, ARTClientOptions+Private.h)
Confirm each of these (and any other
AP*matches) lives inside an#ifdef ABLY_SUPPORTS_PLUGINSblock, then remove any remainingABLY_IS_SPM_BUILDguards.Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (3)
3-5: Good: plugin surface imported only when plugins are supportedGating the import on
ABLY_SUPPORTS_PLUGINSkeeps non-SPM builds clean.
9-12: Adopting APPublicClientOptions only when supported is correctThis avoids exposing AP protocols in non-plugin builds.
39-42: Matches prior decision: simple dictionary-like plugin options APIThe signatures align with the earlier choice to avoid defensive copying and keep a lightweight interface. Keep as-is.
Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h (1)
4-6: Confirm _AblyPluginSupportPrivate module is exposed in plugin-enabled buildsThe
@import _AblyPluginSupportPrivate;guard is correct, but this codebase doesn’t declare that module—it must come from your external plugin-support package. Please verify in both your Xcode and SPM plugin-support configurations that:
- A modulemap (e.g.
module _AblyPluginSupportPrivate { … }) is present and included in the Xcode target whenABLY_SUPPORTS_PLUGINSis defined.- The SPM package manifest for the plugin-support repo includes an
include/module.modulemapthat defines_AblyPluginSupportPrivate, and that modulemap is exposed viacSettings,headerSearchPaths, or equivalent.- In both build systems, the plugin-support headers and modulemap are added to the compiler’s search paths so that
@import _AblyPluginSupportPrivate;resolves without errors.Without that modulemap in place, plugin‐enabled builds will fail to find
_AblyPluginSupportPrivate. Please double-check the plugin-support repo’s modulemaps and your project/SPM integration.Source/include/module.modulemap (1)
138-141: Private plugin headers are correctly configured
- All four headers exist under
Source/PrivateHeaders/Ably/- Each header is wrapped in
#ifdef ABLY_SUPPORTS_PLUGINS- Both SPM and Xcode modulemaps reference them at the same locations
No further action required.
Source/ARTPublicRealtimeChannelUnderlyingObjects.m (2)
1-4: Gating on ABLY_SUPPORTS_PLUGINS looks correctWrapping this implementation behind ABLY_SUPPORTS_PLUGINS isolates plugin code paths from default builds. Import path switch to ARTPublicRealtimeChannelUnderlyingObjects.h aligns with the new private/plugin surface.
Also applies to: 21-21
3-3: Confirmed: ARTPublicRealtimeChannelUnderlyingObjects.h Declares APDefaultPublicRealtimeChannelUnderlyingObjectsThe header at
Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.hcorrectly declares:@interface APDefaultPublicRealtimeChannelUnderlyingObjects : NSObject <APPublicRealtimeChannelUnderlyingObjects>No further changes required.
Source/ARTPluginDecodingContext.m (2)
1-1: LGTM: implementation correctly gated by ABLY_SUPPORTS_PLUGINSThe entire class compiles only when plugins are enabled, keeping default builds clean of plugin internals.
Also applies to: 28-28
12-24: Inconsistent summary: initializer is presentThe AI summary mentions the initializer was removed, but this implementation includes it. Treat the source of truth as this file.
Source/ARTRealtimeChannel.m (4)
40-44: LGTM: plugin imports are correctly gatedImporting _AblyPluginSupportPrivate and ARTPluginAPI only when ABLY_SUPPORTS_PLUGINS is defined prevents accidental coupling in default builds.
709-713: Nil-safe notification to plugin on attach looks goodThis call is gated and is nil-safe if liveObjectsPlugin is not provided. No action needed.
901-905: Forwarding object protocol messages is correctly gatedThe forwarding respects ABLY_SUPPORTS_PLUGINS and will no-op when the plugin is not enabled or pm.state is nil.
913-918: Forwarding object sync protocol messages is correctly gatedMatches the onObject pattern; no concerns.
Source/ARTJsonLikeEncoder.m (1)
33-36: Gate plugin-only imports behind ABLY_SUPPORTS_PLUGINS — verify build flags across package managersThis looks correct. Please verify ABLY_SUPPORTS_PLUGINS is consistently defined for SPM, CocoaPods, and any Xcode targets to avoid accidental hard dependency on the plugin-support module.
Ably.xcodeproj/project.pbxproj (3)
2138-2150: Nice separation: dedicated “Plugin Support” groupThe new group keeps plugin-related types cohesive and discoverable. This helps maintainability as we iterate on the plugin surface.
386-388: Confirm whether ARTErrorInfo+Private.h must be consumable by plugin-supportIf the external plugin-support repo includes this header (directly or indirectly), it will also need Public visibility; otherwise keep it Private. Please confirm intended usage so we don’t inadvertently block downstream builds.
3492-3492: LGTM: wiring plugin sources into all platform targets
ARTPublicRealtimeChannelUnderlyingObjects.mandARTPluginAPI.mare correctly added to the Sources phases for iOS, macOS, and tvOS.Also applies to: 3750-3750, 3892-3892, 3534-3534, 3794-3794, 3935-3935
Source/ARTPluginAPI.m (2)
129-139: Returning copies/options and exposing queues looks correctoptionsForClient returns a copy (good for isolation) and the queue accessors expose the intended dispatch queues from options. No issues from a threading/API standpoint.
91-97: +registerSelf invocation confirmeddispatch_once guarantees the plugin is registered only once, even if invoked multiple times. We’ve verified the call sites under
ABLY_SUPPORTS_PLUGINS:
- Source/ARTClientOptions.m:37
- Source/ARTRealtimeChannel.m:70
Ensure that the
ABLY_SUPPORTS_PLUGINSflag is enabled in any build where plugin APIs must be active.
lawrence-forooghian
force-pushed
the
ECO-5531-move-plugin-API-to-separate-repo
branch
from
28ebad5 to
521ce11
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/jazzydoc
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Source/ARTRealtimeChannel.m (1)
423-438: Queue-safety: don’t require callers to be on _queue; dispatch onto it internally
dispatch_assert_queue(_queue)will trip if the caller isn’t on the channel’s internal queue. Follow the established pattern (seeinternalPostMessages:) and dispatch to_queueinside this method.Apply:
- (void)sendStateWithObjectMessages:(NSArray<id<APObjectMessageProtocol>> *)objectMessages completion:(ARTCallback)completion { - dispatch_assert_queue(_queue); - - ARTProtocolMessage *pm = [[ARTProtocolMessage alloc] init]; - pm.action = ARTProtocolMessageObject; - pm.channel = self.name; - pm.state = objectMessages; - - [self publishProtocolMessage:pm callback:^(ARTStatus *status) { - if (completion) { - completion(status.errorInfo); - } - }]; - } + (void)sendStateWithObjectMessages:(NSArray<id<APObjectMessageProtocol>> *)objectMessages + completion:(ARTCallback)completion { + dispatch_sync(_queue, ^{ + ARTProtocolMessage *pm = [[ARTProtocolMessage alloc] init]; + pm.action = ARTProtocolMessageObject; + pm.channel = self.name; + pm.state = objectMessages; + + [self publishProtocolMessage:pm callback:^(ARTStatus *status) { + if (completion) { + completion(status.errorInfo); + } + }]; + }); +}Source/ARTJsonLikeEncoder.m (1)
1156-1168: Uninitialized out-parameter used in log; initialize with __autoreleasing
erroris used in the warning log if decoding fails, but it’s not initialized; if the plugin does not set it, logging an uninitialized variable is undefined behavior. Initialize toniland mark as__autoreleasingto match ARC out-parameter conventions.Apply this diff:
- id<APPublicErrorInfo> error; + id<APPublicErrorInfo> __autoreleasing error = nil; APEncodingFormat format = [self apEncodingFormatFromARTEncoderFormat:[self format]]; id<APObjectMessageProtocol> objectMessage = [liveObjectsPlugin decodeObjectMessage:item context:decodingContext format:format error:&error];
♻️ Duplicate comments (5)
Source/PrivateHeaders/Ably/ARTProtocolMessage.h (1)
73-75: Good fix: AP exposure is now gated*The
stateproperty is correctly wrapped in#if ABLY_SUPPORTS_PLUGINS. This resolves the earlier concern about unguarded AP* references in this header.Docs/plugins.md (1)
14-16: Correct the “does not link” statement and fix list indentationSPM builds do link
_AblyPluginSupportPrivate(see Package.swift). The current text is misleading and the list indentation violates MD007.Apply:
-- There is a two-way dependency between ably-cocoa and `_AblyPluginSupportPrivate`: - - ably-cocoa imports `_AblyPluginSupportPrivate`'s plugin headers (e.g. `APLiveObjectsPlugin.h`) so that it can access the methods exposed by plugins. However, ably-cocoa does not _link_ to `_AblyPluginSupportPrivate`, and so it cannot depend on any classes defined in `_AblyPluginSupportPrivate`. - - `_AblyPluginSupportPrivate` imports ably-cocoa's private headers so that it can expose ably-cocoa's internals to plugins via `PluginAPI`. +- There is a compile-time two-way surface between ably-cocoa and `_AblyPluginSupportPrivate`: + - In SPM builds, ably-cocoa both imports and links `_AblyPluginSupportPrivate` (see Package.swift target dependency), while AP* usage remains gated behind `ABLY_SUPPORTS_PLUGINS`. + - In non-SPM builds (e.g., Xcode/CocoaPods), ably-cocoa does not link `_AblyPluginSupportPrivate`; it conditionally exposes headers, and `_AblyPluginSupportPrivate` imports ably-cocoa’s private headers to expose internals via `PluginAPI`.Source/ARTPluginAPI.m (1)
145-148: Fix block signature/nullability mismatch to avoid crashes and warningsThe plugin completion parameter is declared nonnull, but the internal callback passes
nilon success (ARTCallbackisnullable). This can lead to type-safety warnings and potential crashes. Accept a nullable error parameter and wrap the internal callback.Apply:
-- (void)sendObjectWithObjectMessages:(NSArray<id<APObjectMessageProtocol>> *)objectMessages channel:(id<APRealtimeChannel>)channel completion:(void (^)(_Nonnull id<APPublicErrorInfo>))completion { - [_internalRealtimeChannel(channel) sendStateWithObjectMessages:objectMessages - completion:completion]; +- (void)sendObjectWithObjectMessages:(NSArray<id<APObjectMessageProtocol>> *)objectMessages + channel:(id<APRealtimeChannel>)channel + completion:(void (^)(id<APPublicErrorInfo> _Nullable))completion { + [_internalRealtimeChannel(channel) sendStateWithObjectMessages:objectMessages + completion:^(ARTErrorInfo * _Nullable error) { + if (completion) { + completion((id<APPublicErrorInfo>)error); + } + }]; }Note: Update the corresponding declaration in
ARTPluginAPI.hto match the new nullable error parameter.Ably.xcodeproj/project.pbxproj (2)
4275-4275: Align HEADER_SEARCH_PATHS with actual folder layoutThe current HEADER_SEARCH_PATHS entries reference non-existent roots (“$(PROJECT_DIR)/Source/Private” and “$(SRCROOT)/include”), leading to fragile imports and CI failures. Replace all six occurrences in Ably.xcodeproj/project.pbxproj with the correct include paths:
Files & line numbers to update:
- Lines 4275, 4302: HEADER_SEARCH_PATHS = "$(PROJECT_DIR)/Source/Private";
- Lines 4475, 4515, 4550, 4589: HEADER_SEARCH_PATHS = "$(SRCROOT)/include";
Apply this diff for each occurrence:
- HEADER_SEARCH_PATHS = "$(PROJECT_DIR)/Source/Private"; + HEADER_SEARCH_PATHS = ( + "$(SRCROOT)/Source/include", + "$(SRCROOT)/Source/PrivateHeaders", + );- HEADER_SEARCH_PATHS = "$(SRCROOT)/include"; + HEADER_SEARCH_PATHS = ( + "$(SRCROOT)/Source/include", + "$(SRCROOT)/Source/PrivateHeaders", + );
359-361: Make plugin support headers Public in Xcode projectARTPluginAPI.h and ARTPublicRealtimeChannelUnderlyingObjects.h are currently marked Private in all targets. They must be Public so that external plugins or the new plugin-support package can import them without breaking downstream builds.
• Ably.xcodeproj/project.pbxproj: lines 359–361 (ARTPluginAPI.h in Headers)
• Ably.xcodeproj/project.pbxproj: lines 365–367 (ARTPublicRealtimeChannelUnderlyingObjects.h in Headers)Apply this diff to switch both headers from Private to Public:
--- a/Ably.xcodeproj/project.pbxproj +++ b/Ably.xcodeproj/project.pbxproj @@ -359,9 +359,9 @@ - 21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; - 21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; - 21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; + 21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; + 21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; + 21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; @@ -365,9 +365,9 @@ - 21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; - 21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; - 21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; + 21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; + 21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; + 21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; };
🧹 Nitpick comments (18)
Source/PrivateHeaders/Ably/ARTChannel+Private.h (1)
17-19: Comment rename to ARTPluginAPI looks correct; confirm exposure intent ofloggerin non-plugin buildsThe doc fix is good. Given the PR’s gating theme, double-check that exposing
ARTInternalLog *loggerin this private header is intentional even when ABLY_SUPPORTS_PLUGINS is not defined. If not, consider gating or documenting usage constraints more explicitly to avoid accidental coupling outside plugin-support.Would you like me to propose a small follow-up that either:
- Gates the property behind ABLY_SUPPORTS_PLUGINS, or
- Keeps it always available but updates the comment to clarify non-plugin usage expectations?
Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h (2)
4-4: Make the private module import resilient (modules-off), or decouple via protocol forward-declare.Depending on the consumer’s build settings,
@importrequires Clang modules. To keep this header consumable in non-modular builds (and reduce coupling), consider a conditional import with a forward-declaration fallback.Apply this diff:
-@import _AblyPluginSupportPrivate; +#if __has_feature(modules) +@import _AblyPluginSupportPrivate; +#else +@protocol APDecodingContextProtocol; +#endifNotes:
- Forward-declaring the protocol is sufficient at the header level for protocol adoption; ensure the corresponding .m imports the plugin-support header/module so conformance is type-checked at implementation time.
- This keeps the header robust across SPM/Pods/Carthage/Xcode projects where modules may be toggled.
1-2: Update plugin‐flag checks to avoid “-Wundef” warningsBased on a search of the repository,
ABLY_SUPPORTS_PLUGINSis only defined in Package.swift for SPM; it isn’t set in any Xcode project/xcconfig. To ensure we don’t get “undefined macro” warnings—and to keep the feature‐flag intent clear—please update the check in Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h:• File: Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h (lines 1–2)
-#if ABLY_SUPPORTS_PLUGINS +#if defined(ABLY_SUPPORTS_PLUGINS) && ABLY_SUPPORTS_PLUGINS• (Optional but recommended) In a common config header that’s included by all targets, add a safe default to silence any remaining undefined‐macro warnings:
#ifndef ABLY_SUPPORTS_PLUGINS #define ABLY_SUPPORTS_PLUGINS 0 #endifThis change preserves the existing behaviour in SPM (where the flag is defined to 1) while avoiding warnings in any build that doesn’t explicitly define the macro.
Source/PrivateHeaders/Ably/ARTRealtime+Private.h (1)
13-16: Harden the preprocessor guard to avoid -Wundef; keep import gatedUsing
#if ABLY_SUPPORTS_PLUGINScan trigger -Wundef when the macro isn’t set. Prefer guarding withdefined(...) && ...so undefined evaluates cleanly without warnings.Apply this minimal diff:
-#if ABLY_SUPPORTS_PLUGINS +// Prefer defined(...) to avoid -Wundef when the macro is not passed by the build. +#if defined(ABLY_SUPPORTS_PLUGINS) && ABLY_SUPPORTS_PLUGINS @import _AblyPluginSupportPrivate; #endifSource/PrivateHeaders/Ably/ARTProtocolMessage.h (1)
3-5: Optional: forward-declare the AP protocol to avoid importing the private module in a headerTo reduce transitive coupling and speed up compilation, you can replace the module import with a protocol forward declaration (still under the same guard). This header only needs the protocol name for the property type.
Apply:
-#if ABLY_SUPPORTS_PLUGINS -@import _AblyPluginSupportPrivate; -#endif +#if ABLY_SUPPORTS_PLUGINS +@protocol APObjectMessageProtocol; +#endifDocs/plugins.md (2)
9-9: Avoid bare URLUse a proper markdown link to satisfy MD034 and improve readability.
Apply:
-Currently, our only plugin is for adding LiveObjects functionality. This plugin can be found at https://github.com/ably/ably-cocoa-liveobjects-plugin. +Currently, our only plugin is for adding LiveObjects functionality. See the LiveObjects plugin repository: [ably/ably-cocoa-liveobjects-plugin](https://github.com/ably/ably-cocoa-liveobjects-plugin).
26-29: Polish the “Notes TODO incorporate” section (grammar, tone, and indentation)Minor fixes: remove colloquialisms, use “cannot”, end “etc.” with a period, and normalize list indentation.
Apply:
-## Notes TODO incorporate +## Notes to incorporate -- all of the stuff that depends on `_AblyPluginSupportPrivate` is gated behind `#if ABLY_SUPPORTS_PLUGINS` so as not to affect the non-SPM builds, which do not have access to `_AblyPluginSupportPrivate` -- note that _AblyPluginSupportPrivate can not depend on ably-cocoa at all, which means having to duplicate some types (e.g. enums), or having to weaken some classes e.g. the public types such as `ARTRealtimeChannel`, `ARTClientOptions`, `ARTErrorInfo` etc - - TODO we can probably improve this a bit by at least introducing some empty marker protocols and declaring conformance -- mention that we are now going to not have any instance methods on the channel or realtime because then you have to deal with clashes because our `state` doesn't return the same type as their `state`. same for logger and its level +- All components that depend on `_AblyPluginSupportPrivate` are gated behind `#if ABLY_SUPPORTS_PLUGINS` so as not to affect non-SPM builds, which do not have access to `_AblyPluginSupportPrivate`. +- Note that `_AblyPluginSupportPrivate` cannot depend on ably-cocoa at all, which means duplicating some types (e.g., enums) or weakening some classes (e.g., public types such as `ARTRealtimeChannel`, `ARTClientOptions`, `ARTErrorInfo`, etc.). + - TODO: We can likely improve this by introducing empty marker protocols and declaring conformance. +- We will avoid instance methods on channel or realtime in the plugin surface to prevent type clashes (e.g., differing `state` types). Same consideration applies to the logger and its level.Package.swift (1)
21-23: Pinned revision: add an owner or issue, and plan to unpin before releaseThe explicit revision pin is fine for WIP; consider annotating with the owning issue to prevent drift.
Apply:
- // TODO: Unpin this before releasing LiveObjects branch + // TODO(ECO-5531): Unpin before releasing LiveObjects branchSource/ARTPluginAPI.m (3)
53-72: Make enum mapping future-proof; avoid throwing exceptions in productionRaising an exception for an unmapped
ARTRealtimeChannelStatewill crash in production if new values are added. Prefer a default with an assert and a safe fallback.Apply:
static APRealtimeChannelState _convertOurRealtimeChannelState(ARTRealtimeChannelState ourRealtimeChannelState) { switch (ourRealtimeChannelState) { case ARTRealtimeChannelInitialized: return APRealtimeChannelStateInitialized; case ARTRealtimeChannelAttaching: return APRealtimeChannelStateAttaching; case ARTRealtimeChannelAttached: return APRealtimeChannelStateAttached; case ARTRealtimeChannelDetaching: return APRealtimeChannelStateDetaching; case ARTRealtimeChannelDetached: return APRealtimeChannelStateDetached; case ARTRealtimeChannelSuspended: return APRealtimeChannelStateSuspended; case ARTRealtimeChannelFailed: return APRealtimeChannelStateFailed; + default: + NSCAssert(NO, @"Unhandled ARTRealtimeChannelState: %lu", (unsigned long)ourRealtimeChannelState); + return APRealtimeChannelStateInitialized; } - - [NSException raise:NSInternalInconsistencyException format:@"_convertOurRealtimeChannelState failed to map %lu", ourRealtimeChannelState]; }
74-91: Same here: add default for log level conversionMirror the approach above for
_convertPluginLogLevelto avoid exceptions for future enum values.Apply:
static ARTLogLevel _convertPluginLogLevel(APLogLevel pluginLogLevel) { switch (pluginLogLevel) { case APLogLevelVerbose: return ARTLogLevelVerbose; case APLogLevelDebug: return ARTLogLevelDebug; case APLogLevelInfo: return ARTLogLevelInfo; case APLogLevelWarn: return ARTLogLevelWarn; case APLogLevelError: return ARTLogLevelError; case APLogLevelNone: return ARTLogLevelNone; + default: + NSCAssert(NO, @"Unhandled APLogLevel: %lu", (unsigned long)pluginLogLevel); + return ARTLogLevelNone; } - - [NSException raise:NSInternalInconsistencyException format:@"_convertPluginLogLevel failed to map %lu", pluginLogLevel]; }
145-148: Naming consistency: “sendObjectWithObjectMessages” vs “sendStateWithObjectMessages”The API method name differs from the internal selector it forwards to. Consider aligning the naming or adding a doc comment clarifying the intent to reduce confusion for plugin authors.
Source/ARTRealtimeChannel.m (1)
711-714: Optional: consider a nil-check for the plugin before messagingObjective-C safely no-ops on nil receivers, so this is fine. If you want clearer intent (and logs when no plugin is present), guard with a local
if (self.realtime.options.liveObjectsPlugin).Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (2)
14-16: Guard style: prefer defined(...) to avoid -WundefUsing
#if ABLY_SUPPORTS_PLUGINSis fine, but if-Wundefis enabled it can warn on undefined macros. Consider guarding with#if defined(ABLY_SUPPORTS_PLUGINS) && ABLY_SUPPORTS_PLUGINSfor robustness.Apply this diff:
-#if ABLY_SUPPORTS_PLUGINS +#if defined(ABLY_SUPPORTS_PLUGINS) && ABLY_SUPPORTS_PLUGINS @import _AblyPluginSupportPrivate; #endif
141-145: Doc references: verify symbol names are correct and consistentComments reference
-[ARTPluginAPI ...]and “APPluginAPIProtocol”. Please confirm the canonical names exported by the new plugin-support repo and the private bridge here (e.g., whether the protocol isAPPluginAPIProtocolor something likeAPPublicPluginAPIProtocol). Aligning these comments avoids confusion for maintainers.If adjustment is needed, it’s comment-only; no code change required.
Source/ARTJsonLikeEncoder.m (2)
1112-1122: Add default branch to future-proof enum conversion
apEncodingFormatFromARTEncoderFormat:has no default branch; if newARTEncoderFormatvalues are added, control can reach end of a non-void function. Add a defensive default with an assertion and a sensible fallback.Apply this diff:
#if ABLY_SUPPORTS_PLUGINS /// Converts an `ARTEncoderFormat` to an `APEncodingFormat`. - (APEncodingFormat)apEncodingFormatFromARTEncoderFormat:(ARTEncoderFormat)format { switch (format) { case ARTEncoderFormatJson: return APEncodingFormatJSON; case ARTEncoderFormatMsgPack: return APEncodingFormatMessagePack; + default: + NSCAssert(NO, @"Unknown ARTEncoderFormat: %ld", (long)format); + // Fallback to JSON to avoid returning an indeterminate value in release builds. + return APEncodingFormatJSON; } } #endif
1183-1189: Runtime invariant: consider assertion over exception for missing pluginThrowing
NSInternalInconsistencyExceptionwill crash release builds. If this is a strict invariant (state present implies plugin present), considerNSCAssert(liveObjectsPlugin, ...)which strips in release by default, plus a log and gracefulnilreturn. Alternatively, keep the exception but expand the message to include guidance for integrators.Apply this diff to prefer assertion and graceful handling:
- if (!liveObjectsPlugin) { - // The only thing that sends ObjectMessage is the LiveObjects plugin, so if we have some to encode then the plugin must be present - [NSException raise:NSInternalInconsistencyException format:@"Attempted to encode ObjectMessages without a LiveObjects plugin; this should not be possible."]; - return nil; - } + if (!liveObjectsPlugin) { + NSCAssert(NO, @"Attempted to encode ObjectMessages without a LiveObjects plugin; this should not be possible."); + ARTLogError(_logger, @"ARTJsonLikeEncoder<%@>: missing LiveObjects plugin when encoding ObjectMessages", [_delegate formatAsString]); + return nil; + }Ably.xcodeproj/project.pbxproj (2)
1417-1420: Optional: relocate public headers under include/Ably for consistencyIf ARTPublicRealtimeChannelUnderlyingObjects.h and ARTPluginAPI.h are to be Public, consider placing their file refs under include/Ably (like other public headers) for consistency; it reduces surprises when navigating the tree. Not required if you mark them Public in the build phase.
-21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPluginAPI.h; path = PrivateHeaders/Ably/ARTPluginAPI.h; sourceTree = "<group>"; }; +21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPluginAPI.h; path = include/Ably/ARTPluginAPI.h; sourceTree = "<group>"; }; -21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPublicRealtimeChannelUnderlyingObjects.h; path = PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h; sourceTree = "<group>"; }; +21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPublicRealtimeChannelUnderlyingObjects.h; path = include/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h; sourceTree = "<group>"; };
356-358: Ensure plugin API files are either properly gated or removed
- We verified that
ABLY_SUPPORTS_PLUGINSis not defined in any target’sGCC_PREPROCESSOR_DEFINITIONS(checked inAbly.xcodeproj/project.pbxproj), soARTPluginAPI.mcompiles as an empty translation unit on all platforms.- All plugin-specific code in
.mfiles (includingARTPluginAPI.m) is already wrapped in#if ABLY_SUPPORTS_PLUGINS … #endif.- If plugin implementations now live exclusively in the external plugin repo, remove all three
ARTPluginAPI.mentries from the iOS/macOS/tvOS Sources build phases inAbly.xcodeproj/project.pbxproj.- If you intend to ship plugin support from this repo, add
ABLY_SUPPORTS_PLUGINS=1to the relevant targets’GCC_PREPROCESSOR_DEFINITIONSand retain the existing#if ABLY_SUPPORTS_PLUGINSguards.Optional diff to remove the file entries:
--- a/Ably.xcodeproj/project.pbxproj @@ /* Sources Build Phase */ - 21A28E592E54CFA7007D0BFE /* ARTPluginAPI.m in Sources */ = {isa = PBXBuildFile; fileRef = 21A28E582E54CFA7007D0BFE /* ARTPluginAPI.m */; }; - 21A28E5A2E54CFA7007D0BFE /* ARTPluginAPI.m in Sources */ = {isa = PBXBuildFile; fileRef = 21A28E582E54CFA7007D0BFE /* ARTPluginAPI.m */; }; - 21A28E5B2E54CFA7007D0BFE /* ARTPluginAPI.m in Sources */ = {isa = PBXBuildFile; fileRef = 21A28E582E54CFA7007D0BFE /* ARTPluginAPI.m */; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
.github/workflows/check-plugin.yaml(0 hunks)Ably.podspec(0 hunks)Ably.xcodeproj/project.pbxproj(27 hunks)CONTRIBUTING.md(0 hunks)Docs/plugins.md(1 hunks)Package.resolved(1 hunks)Package.swift(2 hunks)Source/ARTClientOptions.m(4 hunks)Source/ARTJsonLikeEncoder.m(7 hunks)Source/ARTPluginAPI.m(1 hunks)Source/ARTPluginDecodingContext.m(2 hunks)Source/ARTPublicRealtimeChannelUnderlyingObjects.m(2 hunks)Source/ARTRealtimeChannel.m(7 hunks)Source/ARTStatus.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTChannel+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTInternalLog.h(3 hunks)Source/PrivateHeaders/Ably/ARTPluginAPI.h(1 hunks)Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h(2 hunks)Source/PrivateHeaders/Ably/ARTProtocolMessage.h(2 hunks)Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtime+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/include/module.modulemap(1 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/check-plugin.yaml
- Ably.podspec
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (12)
- Source/Ably.modulemap
- Package.resolved
- Source/include/module.modulemap
- Source/PrivateHeaders/Ably/ARTInternalLog.h
- Source/ARTClientOptions.m
- Source/ARTStatus.m
- Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h
- Source/ARTPublicRealtimeChannelUnderlyingObjects.m
- Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h
- Source/ARTPluginDecodingContext.m
- Source/PrivateHeaders/Ably/ARTClientOptions+Private.h
- Source/PrivateHeaders/Ably/ARTPluginAPI.h
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2024-11-04T23:10:23.027Z
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/ARTRealtime+Private.hSource/ARTRealtimeChannel.mAbly.xcodeproj/project.pbxprojSource/PrivateHeaders/Ably/ARTRealtimeChannel+Private.hSource/PrivateHeaders/Ably/ARTChannel+Private.hSource/ARTJsonLikeEncoder.m
📚 Learning: 2025-08-13T16:50:28.236Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2084
File: Source/ARTClientOptions.m:249-251
Timestamp: 2025-08-13T16:50:28.236Z
Learning: In ARTClientOptions.m, the setPluginOptionsValue:forKey: method intentionally provides a simple dictionary-like interface without defensive copying, expecting callers to handle proper value management themselves rather than adding defensive programming overhead.
Applied to files:
Source/PrivateHeaders/Ably/ARTRealtime+Private.hSource/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h
📚 Learning: 2024-10-08T15:37:26.766Z
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/ARTRealtime+Private.hSource/ARTRealtimeChannel.mAbly.xcodeproj/project.pbxprojSource/PrivateHeaders/Ably/ARTRealtimeChannel+Private.hSource/PrivateHeaders/Ably/ARTChannel+Private.hSource/ARTJsonLikeEncoder.m
📚 Learning: 2025-06-16T14:12:36.705Z
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
📚 Learning: 2024-11-26T17:50:25.730Z
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/ARTJsonLikeEncoder.m
🪛 LanguageTool
Docs/plugins.md
[grammar] ~14-~14: There might be a mistake here.
Context: ...y-cocoa and _AblyPluginSupportPrivate: - ably-cocoa imports `_AblyPluginSupportPr...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...'s internals to plugins via PluginAPI. - Currently, the plan is to test all the L...
(QB_NEW_EN)
[style] ~26-~26: Consider removing “of” to be more concise
Context: ...s names). ## Notes TODO incorporate - all of the stuff that depends on `_AblyPluginSuppo...
(ALL_OF_THE)
[style] ~27-~27: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...` - note that _AblyPluginSupportPrivate can not depend on ably-cocoa at all, which mean...
(CAN_NOT_PREMIUM)
[style] ~27-~27: In American English, abbreviations like “etc.” require a period.
Context: ...el, ARTClientOptions, ARTErrorInfo` etc - TODO we can probably improve this...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
Docs/plugins.md
9-9: Bare URL used
(MD034, no-bare-urls)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 SwiftLint (0.57.0)
Package.swift
[Warning] 21-21: TODOs should be resolved (Unpin this before releasing Li...)
(todo)
⏰ 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 (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check
🔇 Additional comments (11)
Source/PrivateHeaders/Ably/ARTChannel+Private.h (1)
11-11: Initializer still takeslogger:; verify alignment with the new plugin-driven logging flowThis private initializer receiving
logger:appears to contradict the PR summary that suggests removing logger provisioning at init time. If the intent is to have logging provided via the plugin pathway, consider deprecating or removing this initializer, or adding a logger-less initializer and migrating call sites.If you want, I can scan the codebase to identify all call sites using this initializer and draft a safe migration plan to a logger-less initializer.
Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h (1)
8-17: Interface contract remains stable. LGTM.No signature changes to ARTPluginDecodingContext or its initializer. The designated initializer and nullability annotations look consistent.
Source/ARTRealtimeChannel.m (1)
68-71: Plugin API registration timing looks goodRegistering
ARTPluginAPIearly ininitWithInternal:ensures any category methods from the plugin repo have the API available.Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (3)
26-29: LGTM: private conformance of ARTRealtimeChannel to plugin-facing protocolConditionally conforming the public wrapper
ARTRealtimeChanneltoAPPublicRealtimeChannelin a private header is a clean way to expose plugin behavior without leaking it publicly.
31-35: LGTM: conditional protocol conformance on Internal classMaking
ARTRealtimeChannelInternalconform toAPRealtimeChannelonly when plugins are supported keeps compilation paths clean and avoids symbol exposure in non-plugin builds.
146-151: Method availability correctly gated; keep doc names consistentThe
sendStateWithObjectMessages:completion:signature andAPObjectMessageProtocolusage are correctly gated behindABLY_SUPPORTS_PLUGINS. As above, verify the doc references to the correct protocol/class names from plugin-support to avoid drift.Source/ARTJsonLikeEncoder.m (3)
32-35: LGTM: plugin imports gated properlyConditionally importing
_AblyPluginSupportPrivateand the local decoding context ensures non-plugin builds stay slim and free of private symbols.
581-586: LGTM: encodingstateonly when present (and plugins enabled)The serialization of
message.stateis well-isolated behind the plugin flag and uses a dedicated conversion helper.
831-833: LGTM: decodingstatevia plugin pathDecoding
stateviaobjectMessagesFromArray:protocolMessage:is correct and will naturally yieldnilwhen the plugin isn’t present or decoding fails, matching the intended semantics.Ably.xcodeproj/project.pbxproj (2)
1938-1950: Good structure: new “Plugin Support” group is clearThe grouping of ARTPluginAPI, ARTPublicRealtimeChannelUnderlyingObjects, and ARTPluginDecodingContext under “Plugin Support” improves discoverability.
386-388: ARTErrorInfo+Private.h wired as Private across targets — OKThe file reference and its inclusion as Private in Copy Headers across iOS/macOS/tvOS look correct.
Also applies to: 1427-1427, 2617-2617, 2750-2750, 2996-2996
"Support" to make its purpose clear, and the underscore and "Private" to discourage users from importing it. (Don't worry about whether the directory should have an underscore in it or not; I'm about to move this stuff out of this repo soon anyway).
lawrence-forooghian
force-pushed
the
ECO-5531-move-plugin-API-to-separate-repo
branch
from
521ce11 to
027d5e6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
lawrence-forooghian
force-pushed
the
ECO-5531-move-plugin-API-to-separate-repo
branch
from
027d5e6 to
2eadbce
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/jazzydoc
lawrence-forooghian
force-pushed
the
ECO-5531-move-plugin-API-to-separate-repo
branch
from
2eadbce to
16e35a3
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/jazzydoc
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
Source/PrivateHeaders/Ably/ARTRealtime+Private.h (1)
34-38: Conditional protocol conformance LGTM; guard style is correctUsing
#ifdef ABLY_SUPPORTS_PLUGINSto gate the<APRealtimeClient>conformance is consistent and avoids -Wundef. This matches the plugin gating policy and keeps the core build clean when plugins are off.Ably.xcodeproj/project.pbxproj (2)
359-367: Headers for plugin support are marked Private; confirm packaging plan or make them PublicIf the separate plugin-support package needs to import these headers from Ably, they must be Public. Keeping them Private will prevent downstream import from the built framework. If they are intentionally internal, document the expected integration path for the new repo (e.g., vendoring headers) to avoid future regressions.
Apply this to expose them as Public (for all platforms) if external consumption is intended:
-21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; +21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; -21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; +21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; };Would you like me to also update umbrella/public headers if you choose the “Public” route?
2696-2696: Duplicate: header visibility concern already notedThe same headers are flagged as Private in other header phases too. Please resolve consistently across all targets as per the earlier comment.
Also applies to: 2792-2792, 2997-2997
Source/ARTPluginAPI.m (2)
53-71: Enum mapping for channel state is now correct (including ‘Failed’); strict fallback is goodThe case for ARTRealtimeChannelFailed is present and maps to APRealtimeChannelStateFailed; raising an exception for unmapped values keeps failures loud.
74-91: Log level conversion looks good; strict fallback via exception is acceptableSwitch covers all known values and fails fast for unknown ones, resolving earlier concerns about undefined returns.
🧹 Nitpick comments (19)
Source/PrivateHeaders/Ably/ARTRealtime+Private.h (1)
13-16: Gated import looks good; optionally add a non-modular fallbackFor consumers that build without Clang modules enabled, consider a guarded fallback to a header import.
-#ifdef ABLY_SUPPORTS_PLUGINS -@import _AblyPluginSupportPrivate; -#endif +#ifdef ABLY_SUPPORTS_PLUGINS +# if defined(__has_feature) && __has_feature(modules) +@import _AblyPluginSupportPrivate; +# else +#import <_AblyPluginSupportPrivate/_AblyPluginSupportPrivate.h> +# endif +#endifSource/PrivateHeaders/Ably/ARTClientOptions+Private.h (2)
3-5: Prefer forward-declaring plugin protocols over importing the plugin module in a headerReduce coupling and avoid requiring modules/cross-package availability by forward-declaring the protocols instead of importing the plugin support module from a header. This keeps the dependency out of compilation units that don’t need it and helps avoid potential cyclical package dependencies if the plugin package ever references ably-cocoa.
Apply this diff to replace the import with forward declarations:
-#ifdef ABLY_SUPPORTS_PLUGINS -@import _AblyPluginSupportPrivate; -#endif +#ifdef ABLY_SUPPORTS_PLUGINS +@protocol APPublicClientOptions; +@protocol APLiveObjectsInternalPluginProtocol; +#endif
39-42: Tighten comment wording and align naming in docsThe methods are intentionally simple (per prior learnings) and look fine. The docstrings can be clearer and more consistent about who calls them.
-/// Provides the implementation for `-[ARTPluginAPI setPluginOptionsValue:forKey:options:]`. See documentation for that method in `APPluginAPIProtocol`. +/// Backing implementation invoked by ARTPluginAPI for `setPluginOptionsValue:forKey:options:`. +/// See `APPluginAPIProtocol` for semantics. ... -/// Provides the implementation for `-[ARTPluginAPI pluginOptionsValueForKey:options:]`. See documentation for that method in `APPluginAPIProtocol`. +/// Backing implementation invoked by ARTPluginAPI for `pluginOptionsValueForKey:options:`. +/// See `APPluginAPIProtocol` for semantics.Note: Keeping these methods ungated is reasonable given the intent to provide a lightweight, dictionary-like interface without extra defensive overhead.
Docs/plugins.md (8)
5-5: Clarify private/gated nature and tie to build flag.Tighten the wording to make it explicit that this is a private, gated support lib (aligns with the PR objective and ADR).
Apply this diff:
-ably-cocoa exposes a separate library called `_AblyPluginSupportPrivate`. This provides an API that plugins can use to access ably-cocoa's internals. +ably-cocoa provides a separate, private support library named `_AblyPluginSupportPrivate` (not part of the public, stable API). It exposes a Plugin API that Ably-authored plugins can use to access ably-cocoa internals. Access to this support is gated at compile time via the `ABLY_SUPPORTS_PLUGINS` build flag.
11-11: Section header LGTM. Consider adding a one-line “not a public API” disclaimer.The heading is clear. If you want to be extra explicit, add a short note that this surface can change without notice.
14-16: Avoid “two-way dependency” wording; fix nested list indentation; clarify runtime vs header coupling.“Two-way dependency” reads as a circular binary dependency. The intent seems to be mutual header-level coupling, with no link-time dependency from ably-cocoa to the private lib. Also addresses MD007 (indentation = 2 spaces) and minor grammar.
Apply this diff:
-- There is a two-way dependency between ably-cocoa and `_AblyPluginSupportPrivate`: - - ably-cocoa imports `_AblyPluginSupportPrivate`'s plugin headers (e.g. `APLiveObjectsPlugin.h`) so that it can access the methods exposed by plugins. However, ably-cocoa does not _link_ to `_AblyPluginSupportPrivate`, and so it cannot depend on any classes defined in `_AblyPluginSupportPrivate`. - - `_AblyPluginSupportPrivate` imports ably-cocoa's private headers so that it can expose ably-cocoa's internals to plugins via `PluginAPI`. +- There is mutual header-level coupling between ably-cocoa and `_AblyPluginSupportPrivate`, but no runtime linking from ably-cocoa to `_AblyPluginSupportPrivate`: + - ably-cocoa imports `_AblyPluginSupportPrivate`'s plugin headers (e.g., `APLiveObjectsPlugin.h`) to call methods exposed by plugins. However, ably-cocoa does not link to `_AblyPluginSupportPrivate`, so it cannot depend on any classes defined there at runtime. + - `_AblyPluginSupportPrivate` imports ably-cocoa's private headers to expose ably-cocoa internals to plugins via `PluginAPI`.
18-18: Tie the naming rationale to the Xcode preselection behavior.Make the mitigation explicit so readers understand why the underscore/suffix exist.
-- The underscore and `Private` are to reduce the chances of the user accidentally importing it into their project. +- The leading underscore and `Private` suffix are deliberate to reduce the chance of users accidentally linking it (mitigates Xcode’s default product preselection).
22-22: Minor wording polish for readability.Add the missing comma after “e.g.” and keep the guidance crisp.
-- Make good use of `NS_SWIFT_NAME` to make `_AblyPluginSupportPrivate` nicer to use from Swift (e.g. to remove the `AP` prefix from class names). +- Make good use of `NS_SWIFT_NAME` to make `_AblyPluginSupportPrivate` nicer to use from Swift (e.g., to remove the `AP` prefix from class names).
24-24: Rename section to something publishable.“Notes TODO incorporate” reads as a placeholder. Prefer a durable section title.
-## Notes TODO incorporate +## Open questions / TODOs to incorporate
11-11: Also fix markdownlint MD034 (bare URL) elsewhere (non-changed line).Line 9 uses a bare URL; convert to a link to satisfy MD034.
Outside the selected range, update line 9 to:
Currently, our only plugin is for adding LiveObjects functionality. This plugin can be found at [ably-cocoa-liveobjects-plugin](https://github.com/ably/ably-cocoa-liveobjects-plugin).
26-29: Refine plugin gating notes, fix grammar/style, and add example import guardApply the following updates in
Docs/plugins.md(around lines 26–29) to improve clarity, correct grammar, and introduce a protocol-based mitigation:- all of the stuff that depends on `_AblyPluginSupportPrivate` is gated behind `#ifdef ABLY_SUPPORTS_PLUGINS` so as not to affect the non-SPM builds, which do not have access to `_AblyPluginSupportPrivate` - note that _AblyPluginSupportPrivate can not depend on ably-cocoa at all, which means having to duplicate some types (e.g. enums), or having to weaken some classes e.g. the public types such as `ARTRealtimeChannel`, `ARTClientOptions`, `ARTErrorInfo` etc - - TODO we can probably improve this a bit by at least introducing some empty marker protocols and declaring conformance - mention that we are now going to not have any instance methods on the channel or realtime because then you have to deal with clashes because our `state` doesn't return the same type as their `state`. same for logger and its level + All code that depends on `_AblyPluginSupportPrivate` is gated behind `#ifdef ABLY_SUPPORTS_PLUGINS` to avoid affecting non-SPM builds, which do not link `_AblyPluginSupportPrivate`. Example: + + ```objc + #ifdef ABLY_SUPPORTS_PLUGINS + #import <_AblyPluginSupportPrivate/APLiveObjectsPlugin.h> + #endif + ``` + +- `_AblyPluginSupportPrivate` cannot depend on ably-cocoa, which means we may need to duplicate a few lightweight types (e.g., enums) or expose narrower abstractions for public types such as `ARTRealtimeChannel`, `ARTClientOptions`, and `ARTErrorInfo`, etc. + - Potential mitigation: introduce marker protocols in ably-cocoa and declare conformance to bridge types without creating a binary dependency. + +- To avoid type-clash pitfalls (e.g., differing `state` types), avoid adding instance methods on channel or realtime in this surface; prefer free functions or protocol-oriented helpers. Apply the same principle to logger level.Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h (2)
4-4: Reduce header coupling: forward-declare the protocol instead of importing the module here.Pulling in the module from a header increases compile-time fanout and forces module availability in all includers. Forward-declare the protocol in the header and import the module in the .m instead.
Apply this diff in this header:
-@import _AblyPluginSupportPrivate; +@protocol APDecodingContextProtocol;Then, in ARTPluginDecodingContext.m (implementation file), import the module:
@import _AblyPluginSupportPrivate;
15-15: Also mark +new as unavailable.You’ve made -init unavailable; mirror that for +new to prevent accidental usage.
- (instancetype)init NS_UNAVAILABLE; + (instancetype)new NS_UNAVAILABLE;Package.swift (3)
20-23: Avoid long-lived pinning to a specific revision for plugin-support.The hard pin to a git revision is fine for a WIP branch, but it will hinder dependency resolution downstream and makes upgrades harder.
Consider switching to a semver-based requirement or (temporarily) a branch while the API stabilizes. For example:
- // TODO: Unpin this before releasing LiveObjects branch - .package(url: "https://github.com/ably/ably-cocoa-plugin-support", .revision("3f286932ee6e0b2dc2fa4ae72a08854022f3bc97")), + // TODO: Switch to a tagged release before merging LiveObjects + .package(url: "https://github.com/ably/ably-cocoa-plugin-support", branch: "main"),If you already have a tag available, prefer:
.package(url: "https://github.com/ably/ably-cocoa-plugin-support", from: "X.Y.Z"),Would you like me to open a tracking issue to ensure we unpin before merging this branch?
29-30: Depending on the private plugin-support product from Ably target is the right approach.This keeps the Ably package’s public surface area limited to the “Ably” product, avoiding Xcode offering any plugin-related products to app integrators. This directly mitigates the original issue where users could accidentally link the plugin product.
41-41: Define ABLY_SUPPORTS_PLUGINS for ObjC++ as well to avoid edge-case mismatches.You’ve set the macro in cSettings, which covers C/ObjC. If any .mm files include guarded headers or use the macro, add it to cxxSettings too.
publicHeadersPath: "include", cSettings: [ .define("ABLY_SUPPORTS_PLUGINS"), .headerSearchPath("PrivateHeaders"), .headerSearchPath("PrivateHeaders/Ably"), .headerSearchPath("include/Ably"), .headerSearchPath("SocketRocket"), .headerSearchPath("SocketRocket/Internal"), .headerSearchPath("SocketRocket/Internal/Security"), .headerSearchPath("SocketRocket/Internal/Proxy"), .headerSearchPath("SocketRocket/Internal/Utilities"), .headerSearchPath("SocketRocket/Internal/RunLoop"), .headerSearchPath("SocketRocket/Internal/Delegate"), .headerSearchPath("SocketRocket/Internal/IOConsumer"), ] + , cxxSettings: [ + .define("ABLY_SUPPORTS_PLUGINS") + ]Source/ARTPluginAPI.m (3)
71-71: Use signed format specifier for enum in exception messagesMinor format mismatch: AP*/ART* enums are typically NSInteger-backed. Prefer %ld with an explicit cast to long to avoid signedness/format warnings.
Apply this diff:
- [NSException raise:NSInternalInconsistencyException format:@"_convertOurRealtimeChannelState failed to map %lu", ourRealtimeChannelState]; + [NSException raise:NSInternalInconsistencyException format:@"_convertOurRealtimeChannelState failed to map %ld", (long)ourRealtimeChannelState];- [NSException raise:NSInternalInconsistencyException format:@"_convertPluginLogLevel failed to map %lu", pluginLogLevel]; + [NSException raise:NSInternalInconsistencyException format:@"_convertPluginLogLevel failed to map %ld", (long)pluginLogLevel];Also applies to: 90-90
11-16: Remove unused helper _ourPublicErrorInfo (dead code)This helper isn’t referenced. Either remove it now or wire it where appropriate (e.g., if/when adapting completion errors).
Apply this diff:
-static ARTErrorInfo *_ourPublicErrorInfo(id<APPublicErrorInfo> pluginPublicErrorInfo) { - if (![pluginPublicErrorInfo isKindOfClass:[ARTErrorInfo class]]) { - [NSException raise:NSInternalInconsistencyException format:@"Expected ARTErrorInfo, got %@", pluginPublicErrorInfo]; - } - return (ARTErrorInfo *)pluginPublicErrorInfo; -}
129-131: Mark pluginOptionsValueForKey: as nullableThis likely returns nil when absent. Consider annotating the return type as nullable for clarity and API correctness.
Apply this diff in the implementation:
-- (id)pluginOptionsValueForKey:(NSString *)key clientOptions:(id<APPublicClientOptions>)options { +- (nullable id)pluginOptionsValueForKey:(NSString *)key clientOptions:(id<APPublicClientOptions>)options { return [_ourPublicClientOptions(options) pluginOptionsValueForKey:key]; }Also update the corresponding declaration in the header (Source/PrivateHeaders/Ably/ARTPluginAPI.h):
- (nullable id)pluginOptionsValueForKey:(NSString *)key clientOptions:(id<APPublicClientOptions>)options;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
.github/workflows/check-spm.yaml(1 hunks)Ably.podspec(0 hunks)Ably.xcodeproj/project.pbxproj(27 hunks)CONTRIBUTING.md(0 hunks)Docs/plugins.md(1 hunks)Package.resolved(1 hunks)Package.swift(2 hunks)Source/ARTClientOptions.m(4 hunks)Source/ARTJsonLikeEncoder.m(7 hunks)Source/ARTPluginAPI.m(1 hunks)Source/ARTPluginDecodingContext.m(2 hunks)Source/ARTPublicRealtimeChannelUnderlyingObjects.m(2 hunks)Source/ARTRealtimeChannel.m(7 hunks)Source/ARTStatus.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTChannel+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTInternalLog.h(3 hunks)Source/PrivateHeaders/Ably/ARTPluginAPI.h(1 hunks)Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h(2 hunks)Source/PrivateHeaders/Ably/ARTProtocolMessage.h(2 hunks)Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtime+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/include/module.modulemap(1 hunks)
💤 Files with no reviewable changes (2)
- CONTRIBUTING.md
- Ably.podspec
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/check-spm.yaml
🚧 Files skipped from review as they are similar to previous changes (16)
- Source/Ably.modulemap
- Source/PrivateHeaders/Ably/ARTPluginAPI.h
- Source/include/module.modulemap
- Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h
- Source/ARTClientOptions.m
- Package.resolved
- Source/ARTPluginDecodingContext.m
- Source/ARTPublicRealtimeChannelUnderlyingObjects.m
- Source/PrivateHeaders/Ably/ARTChannel+Private.h
- Source/ARTStatus.m
- Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h
- Source/ARTRealtimeChannel.m
- Source/PrivateHeaders/Ably/ARTInternalLog.h
- Source/PrivateHeaders/Ably/ARTProtocolMessage.h
- Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h
- Source/ARTJsonLikeEncoder.m
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2088
File: Package.swift:0-0
Timestamp: 2025-08-20T14:58:23.698Z
Learning: In ably-cocoa, ABLY_IS_SPM_BUILD should not be used anywhere; plugin gating uses ABLY_SUPPORTS_PLUGINS instead.
📚 Learning: 2024-11-04T23:10:23.027Z
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/ARTRealtime+Private.hAbly.xcodeproj/project.pbxproj
📚 Learning: 2025-08-20T14:58:23.698Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2088
File: Package.swift:0-0
Timestamp: 2025-08-20T14:58:23.698Z
Learning: In ably-cocoa, ABLY_IS_SPM_BUILD should not be used anywhere; plugin gating uses ABLY_SUPPORTS_PLUGINS instead.
Applied to files:
Source/PrivateHeaders/Ably/ARTRealtime+Private.hSource/PrivateHeaders/Ably/ARTPluginDecodingContext.hDocs/plugins.mdAbly.xcodeproj/project.pbxprojPackage.swift
📚 Learning: 2024-09-17T13:25:42.060Z
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/PrivateHeaders/Ably/ARTRealtime+Private.h
📚 Learning: 2025-08-13T16:50:28.236Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2084
File: Source/ARTClientOptions.m:249-251
Timestamp: 2025-08-13T16:50:28.236Z
Learning: In ARTClientOptions.m, the setPluginOptionsValue:forKey: method intentionally provides a simple dictionary-like interface without defensive copying, expecting callers to handle proper value management themselves rather than adding defensive programming overhead.
Applied to files:
Source/PrivateHeaders/Ably/ARTRealtime+Private.hSource/PrivateHeaders/Ably/ARTClientOptions+Private.h
📚 Learning: 2024-10-08T15:37:26.766Z
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/ARTRealtime+Private.hSource/PrivateHeaders/Ably/ARTClientOptions+Private.hAbly.xcodeproj/project.pbxproj
📚 Learning: 2025-06-16T14:12:36.705Z
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
🪛 LanguageTool
Docs/plugins.md
[grammar] ~14-~14: There might be a mistake here.
Context: ...y-cocoa and _AblyPluginSupportPrivate: - ably-cocoa imports `_AblyPluginSupportPr...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...'s internals to plugins via PluginAPI. - Currently, the plan is to test all the L...
(QB_NEW_EN)
[style] ~26-~26: Consider removing “of” to be more concise
Context: ...s names). ## Notes TODO incorporate - all of the stuff that depends on `_AblyPluginSuppo...
(ALL_OF_THE)
[style] ~27-~27: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...` - note that _AblyPluginSupportPrivate can not depend on ably-cocoa at all, which mean...
(CAN_NOT_PREMIUM)
[style] ~27-~27: In American English, abbreviations like “etc.” require a period.
Context: ...el, ARTClientOptions, ARTErrorInfo` etc - TODO we can probably improve this...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
Docs/plugins.md
9-9: Bare URL used
(MD034, no-bare-urls)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 SwiftLint (0.57.0)
Package.swift
[Warning] 21-21: TODOs should be resolved (Unpin this before releasing Li...)
(todo)
⏰ 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 (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (11)
Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (3)
9-12: Confirm APPublicClientOptions protocol requirements are implemented under ABLY_SUPPORTS_PLUGINSDeclaring conformance in the header will trigger “incomplete implementation” errors unless the .m actually implements all required methods when ABLY_SUPPORTS_PLUGINS is defined. Please verify that ARTClientOptions.m (or another compilation unit) conditionally includes the plugin headers and implements the protocol.
28-28: Inconsistent with PR summary: restUrlComponents is still declaredThe AI summary says this instance method was removed, but it remains in this header. If removal was intended, please confirm and remove this declaration; otherwise, update the summary to reflect the current state.
32-35: LGTM: liveObjectsPlugin is correctly gated behind ABLY_SUPPORTS_PLUGINSThe property is properly hidden from builds that don’t enable plugin support, aligning with the gating strategy (ABLY_SUPPORTS_PLUGINS) preferred in this codebase.
Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h (1)
1-2: Good gating behind ABLY_SUPPORTS_PLUGINS.Wrapping the entire header ensures AP* symbols don’t leak when plugin support is off, aligning with the repo-wide policy.
Also applies to: 21-21
Ably.xcodeproj/project.pbxproj (5)
356-358: Added ARTPluginAPI.m to all framework targets — looks goodCompiled into iOS/macOS/tvOS frameworks; consistent with the Plugin Support group.
386-388: Private category header correctly added as PrivateThe “+Private” header is correctly scoped as Private across targets.
1417-1420: File references for Plugin Support look consistentPaths under PrivateHeaders/Ably for headers and .m at source root are fine. This relies on PrivateHeaders being on the header search path — see my separate comment on HEADER_SEARCH_PATHS.
1938-1950: Good separation via new “Plugin Support” groupNice organization of ARTPluginAPI / decoding context / underlying objects under a dedicated group.
1938-1950: Plugin Gating Flag Verified
- Package.swift defines only
ABLY_SUPPORTS_PLUGINS(noABLY_IS_SPM_BUILD).- All plugin-related code (
ARTPluginAPI,ARTPluginDecodingContext, underlying objects, etc.) is correctly wrapped in#ifdef ABLY_SUPPORTS_PLUGINS.- No instances of
ABLY_IS_SPM_BUILDfound in the codebase.No further changes needed.
Source/ARTPluginAPI.m (2)
1-2: Gating on ABLY_SUPPORTS_PLUGINS is correct and consistent with project guidanceThis aligns with the team preference to gate plugin code behind ABLY_SUPPORTS_PLUGINS (not ABLY_IS_SPM_BUILD).
145-148: Completion nullability fix and forwarding look goodCompletion now accepts a nullable error and is forwarded directly to the internal API. Assumes ARTErrorInfo conforms to APPublicErrorInfo, which matches the design here.
lawrence-forooghian
force-pushed
the
ECO-5531-move-plugin-API-to-separate-repo
branch
from
16e35a3 to
c843790
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/jazzydoc
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h (1)
1-2: No need to adjust module map exposure beyond the preprocessor guardReiterating the earlier conclusion: the #ifdef ABLY_SUPPORTS_PLUGINS at the top is sufficient even if the header appears in a non-SPM module map; the contents won’t be compiled without the flag.
Also applies to: 17-17
Ably.xcodeproj/project.pbxproj (3)
359-361: If external plugins must import ARTPluginAPI, these headers should be Public (not Private)Keeping ARTPluginAPI.h as Private prevents downstream consumers from importing it from the built framework. If the separate plugin-support repo needs these symbols, expose them as Public. If they’re intentionally internal, please confirm how the external repo accesses them.
Proposed change:
-21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; +21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; };Follow-up (if exposing publicly): move the header under the public include root (see comment at Lines 1417–1420).
365-367: Make ARTPublicRealtimeChannelUnderlyingObjects.h Public if it’s part of the external plugin surfaceSame rationale as ARTPluginAPI.h. If external plugin-support depends on this header, expose it as Public; otherwise, document its intended private consumption path.
-21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; +21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; };
4275-4275: Header search paths point to incorrect/nonexistent locations; align with Source/include and Source/PrivateHeaders
- iOS uses "$(PROJECT_DIR)/Source/Private" (this path doesn’t exist; should be Source/PrivateHeaders).
- macOS/tvOS use "$(SRCROOT)/include" (should be Source/include).
Standardize across all platforms to avoid brittle imports and CI failures.Apply minimal fixes:
-HEADER_SEARCH_PATHS = "$(PROJECT_DIR)/Source/Private"; +HEADER_SEARCH_PATHS = ( + "$(PROJECT_DIR)/Source/include", + "$(PROJECT_DIR)/Source/PrivateHeaders", +);-HEADER_SEARCH_PATHS = "$(SRCROOT)/include"; +HEADER_SEARCH_PATHS = ( + "$(SRCROOT)/Source/include", + "$(SRCROOT)/Source/PrivateHeaders", +);Note: This matches the established layout (public: Source/include, private: Source/PrivateHeaders).
Also applies to: 4303-4303, 4476-4476, 4516-4516, 4551-4551, 4590-4590
Source/ARTPluginAPI.m (2)
53-71: Channel state mapping is now correct; previous enum mix-up resolvedThe switch maps
ARTRealtimeChannelFailedcorrectly and asserts on unmapped cases after the switch. Good defensive stance.
74-91: Log level conversion: strict post-switch assert prevents undefined returnsRaising
NSInternalInconsistencyExceptionafter the switch is acceptable and stricter than a default return. Looks good.
🧹 Nitpick comments (10)
Docs/plugins.md (4)
5-5: Explain “separate library” and avoid accidental import riskThe sentence is clear. Consider making it explicit that ably-cocoa imports only headers (not linking) from the private library to prevent confusion about dependency direction.
-ably-cocoa exposes a separate library called `_AblyPluginSupportPrivate`. This provides an API that plugins can use to access ably-cocoa's internals. +ably-cocoa exposes a separate library called `_AblyPluginSupportPrivate`. It provides headers that plugins use to access ably-cocoa's internals; ably-cocoa itself imports those headers but does not link to the library.
11-16: Clarify “two-way dependency” and fix list indentation and grammarTighten wording to distinguish header dependencies from link-time dependencies and fix MD007 indentation.
-## Notes on `_AblyPluginSupportPrivate` +## Notes on `_AblyPluginSupportPrivate` -- There is a two-way dependency between ably-cocoa and `_AblyPluginSupportPrivate`: - - ably-cocoa imports `_AblyPluginSupportPrivate`'s plugin headers (e.g. `APLiveObjectsPlugin.h`) so that it can access the methods exposed by plugins. However, ably-cocoa does not _link_ to `_AblyPluginSupportPrivate`, and so it cannot depend on any classes defined in `_AblyPluginSupportPrivate`. - - `_AblyPluginSupportPrivate` imports ably-cocoa's private headers so that it can expose ably-cocoa's internals to plugins via `PluginAPI`. +- There is a compile-time header dependency in both directions, but no link-time dependency from ably-cocoa to `_AblyPluginSupportPrivate`: + - ably-cocoa imports `_AblyPluginSupportPrivate` plugin headers (e.g. `APLiveObjectsPlugin.h`) to call plugin-exposed methods. However, ably-cocoa does not link to `_AblyPluginSupportPrivate`, so it cannot depend on any classes defined there. + - `_AblyPluginSupportPrivate` imports ably-cocoa's private headers to expose internals to plugins via `PluginAPI`.
22-23: Minor style cleanup in guideline sentenceRemove a filler phrase for concision.
-- Make good use of `NS_SWIFT_NAME` to make `_AblyPluginSupportPrivate` nicer to use from Swift (e.g. to remove the `AP` prefix from class names). +- Use `NS_SWIFT_NAME` to make `_AblyPluginSupportPrivate` nicer to use from Swift (e.g., to remove the `AP` prefix from class names).
26-29: Polish “Notes TODO incorporate” section (style, wording, indentation)Tighten language and fix MD007 list indentation, “cannot” usage, and “etc.” punctuation.
-## Notes TODO incorporate +## Notes to incorporate -- all of the stuff that depends on `_AblyPluginSupportPrivate` is gated behind `#ifdef ABLY_SUPPORTS_PLUGINS` so as not to affect the non-SPM builds, which do not have access to `_AblyPluginSupportPrivate` -- note that _AblyPluginSupportPrivate can not depend on ably-cocoa at all, which means having to duplicate some types (e.g. enums), or having to weaken some classes e.g. the public types such as `ARTRealtimeChannel`, `ARTClientOptions`, `ARTErrorInfo` etc - - TODO we can probably improve this a bit by at least introducing some empty marker protocols and declaring conformance -- mention that we are now going to not have any instance methods on the channel or realtime because then you have to deal with clashes because our `state` doesn't return the same type as their `state`. same for logger and its level +- all the code that depends on `_AblyPluginSupportPrivate` is gated behind `#ifdef ABLY_SUPPORTS_PLUGINS` so as not to affect non-SPM builds, which do not have access to `_AblyPluginSupportPrivate`. +- note that `_AblyPluginSupportPrivate` cannot depend on ably-cocoa at all, which means duplicating some types (e.g., enums) or weakening some classes (e.g., the public types such as `ARTRealtimeChannel`, `ARTClientOptions`, `ARTErrorInfo`, etc.). + - TODO: we can probably improve this by introducing empty marker protocols and declaring conformance. +- we will avoid instance methods on channel or realtime to prevent type clashes (e.g., `state` return type differs), and similarly for logger level.Package.swift (1)
20-23: Use a full revision (or tag) for the plugin-support dependency; tighten the TODO with a tracker reference.Short SHAs are ambiguous and may break resolution. Prefer a full 40-char commit or, ideally, a tag. Also, consider annotating the TODO with ECO-5531 for traceability.
Apply this diff:
- // TODO: Unpin this before releasing LiveObjects branch - .package(url: "https://github.com/ably/ably-cocoa-plugin-support", .revision("5282886")), + // TODO(ECO-5531): Unpin before releasing LiveObjects branch; prefer a tagged version once available. + .package( + url: "https://github.com/ably/ably-cocoa-plugin-support", + .revision("5282886070764bf9d3d5fe72f9b5fd891629e68f") + ),Source/ARTJsonLikeEncoder.m (3)
1112-1122: Add a default branch to future-proof the enum mapping.If ARTEncoderFormat gains new cases, this function currently has no return path. Add a default with an assertion and a safe fallback.
Apply this diff:
- switch (format) { + switch (format) { case ARTEncoderFormatJson: return APEncodingFormatJSON; case ARTEncoderFormatMsgPack: return APEncodingFormatMessagePack; + default: + NSCAssert(NO, @"Unhandled ARTEncoderFormat: %ld", (long)format); + return APEncodingFormatJSON; }
1124-1173: Minor: initialize the out-parameter to satisfy static analyzers.Initialize the plugin error out-parameter variable to nil to avoid potential analyzer warnings about passing an uninitialized pointer.
Apply this diff:
- id<APPublicErrorInfo> error; + __autoreleasing id<APPublicErrorInfo> error = nil;
1178-1200: Align nullability: method returns nil but the signature isn’t marked nullable.The implementation returns nil when input is nil. Mark the return type nullable here and ensure the header declaration matches.
Apply this diff:
-- (NSArray<NSDictionary *> *)objectMessagesToArray:(nullable NSArray<id<APObjectMessageProtocol>> *)objectMessages { +- (nullable NSArray<NSDictionary *> *)objectMessagesToArray:(nullable NSArray<id<APObjectMessageProtocol>> *)objectMessages {Please confirm the corresponding declaration in ARTJsonLikeEncoder.h also uses a nullable return to match this implementation.
Ably.xcodeproj/project.pbxproj (1)
1417-1420: Place headers under the public include root if they are meant to be imported externallyBoth headers live under PrivateHeaders/Ably. If you change visibility to Public, move them under Source/include/Ably for consistency with the project’s header layout and to ensure installation with the framework.
-21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPluginAPI.h; path = PrivateHeaders/Ably/ARTPluginAPI.h; sourceTree = "<group>"; }; +21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPluginAPI.h; path = include/Ably/ARTPluginAPI.h; sourceTree = "<group>"; }; -21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPublicRealtimeChannelUnderlyingObjects.h; path = PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h; sourceTree = "<group>"; }; +21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = ARTPublicRealtimeChannelUnderlyingObjects.h; path = include/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h; sourceTree = "<group>"; };If these should stay private, please document the import path for the plugin-support repo to avoid confusion later.
Source/ARTPluginAPI.m (1)
71-72: Use correct printf specifier for NS_ENUM(NSInteger, …) in exception messagesUse %ld with an explicit cast to long to avoid format warnings on 64-bit.
- [NSException raise:NSInternalInconsistencyException format:@"_convertOurRealtimeChannelState failed to map %lu", ourRealtimeChannelState]; + [NSException raise:NSInternalInconsistencyException format:@"_convertOurRealtimeChannelState failed to map %ld", (long)ourRealtimeChannelState]; @@ - [NSException raise:NSInternalInconsistencyException format:@"_convertPluginLogLevel failed to map %lu", pluginLogLevel]; + [NSException raise:NSInternalInconsistencyException format:@"_convertPluginLogLevel failed to map %ld", (long)pluginLogLevel];Also applies to: 90-91
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
.github/workflows/check-spm.yaml(1 hunks)Ably.podspec(0 hunks)Ably.xcodeproj/project.pbxproj(27 hunks)CONTRIBUTING.md(0 hunks)Docs/plugins.md(1 hunks)Package.resolved(1 hunks)Package.swift(2 hunks)Source/ARTClientOptions.m(4 hunks)Source/ARTJsonLikeEncoder.m(7 hunks)Source/ARTPluginAPI.m(1 hunks)Source/ARTPluginDecodingContext.m(2 hunks)Source/ARTPublicRealtimeChannelUnderlyingObjects.m(2 hunks)Source/ARTRealtimeChannel.m(7 hunks)Source/ARTStatus.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTChannel+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTInternalLog.h(3 hunks)Source/PrivateHeaders/Ably/ARTPluginAPI.h(1 hunks)Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h(2 hunks)Source/PrivateHeaders/Ably/ARTProtocolMessage.h(2 hunks)Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtime+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/include/module.modulemap(1 hunks)
💤 Files with no reviewable changes (2)
- CONTRIBUTING.md
- Ably.podspec
🚧 Files skipped from review as they are similar to previous changes (13)
- Source/include/module.modulemap
- Source/Ably.modulemap
- Source/ARTStatus.m
- Source/PrivateHeaders/Ably/ARTRealtime+Private.h
- Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h
- Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h
- Source/ARTPublicRealtimeChannelUnderlyingObjects.m
- Source/PrivateHeaders/Ably/ARTProtocolMessage.h
- Source/ARTClientOptions.m
- Source/ARTPluginDecodingContext.m
- Source/PrivateHeaders/Ably/ARTPluginAPI.h
- Source/ARTRealtimeChannel.m
- .github/workflows/check-spm.yaml
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2088
File: Package.swift:0-0
Timestamp: 2025-08-20T14:58:23.721Z
Learning: In ably-cocoa, ABLY_IS_SPM_BUILD should not be used anywhere; plugin gating uses ABLY_SUPPORTS_PLUGINS instead.
📚 Learning: 2025-08-20T14:58:23.721Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2088
File: Package.swift:0-0
Timestamp: 2025-08-20T14:58:23.721Z
Learning: In ably-cocoa, ABLY_IS_SPM_BUILD should not be used anywhere; plugin gating uses ABLY_SUPPORTS_PLUGINS instead.
Applied to files:
Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.hDocs/plugins.mdAbly.xcodeproj/project.pbxprojPackage.swift
📚 Learning: 2024-11-04T23:10:23.027Z
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/ARTPublicRealtimeChannelUnderlyingObjects.hSource/PrivateHeaders/Ably/ARTRealtimeChannel+Private.hAbly.xcodeproj/project.pbxprojSource/ARTJsonLikeEncoder.mSource/PrivateHeaders/Ably/ARTChannel+Private.h
📚 Learning: 2024-10-08T15:37:26.766Z
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/ARTRealtimeChannel+Private.hSource/PrivateHeaders/Ably/ARTClientOptions+Private.hAbly.xcodeproj/project.pbxprojSource/ARTJsonLikeEncoder.mSource/PrivateHeaders/Ably/ARTChannel+Private.h
📚 Learning: 2025-08-13T16:50:28.236Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2084
File: Source/ARTClientOptions.m:249-251
Timestamp: 2025-08-13T16:50:28.236Z
Learning: In ARTClientOptions.m, the setPluginOptionsValue:forKey: method intentionally provides a simple dictionary-like interface without defensive copying, expecting callers to handle proper value management themselves rather than adding defensive programming overhead.
Applied to files:
Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.hSource/PrivateHeaders/Ably/ARTClientOptions+Private.h
📚 Learning: 2025-08-20T16:47:07.996Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2088
File: Ably.xcodeproj/project.pbxproj:4275-4275
Timestamp: 2025-08-20T16:47:07.996Z
Learning: In ably-cocoa’s Xcode project, the correct header roots are Source/include (public) and Source/PrivateHeaders (private). Ably-iOS previously pointed to Source/Private (nonexistent), and macOS/tvOS to repo-root include; future header search paths should consistently use Source/include and Source/PrivateHeaders across targets.
Applied to files:
Ably.xcodeproj/project.pbxprojPackage.swift
📚 Learning: 2025-06-16T14:12:36.705Z
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
📚 Learning: 2024-11-26T17:50:25.730Z
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/ARTJsonLikeEncoder.m
🪛 LanguageTool
Docs/plugins.md
[grammar] ~14-~14: There might be a mistake here.
Context: ...y-cocoa and _AblyPluginSupportPrivate: - ably-cocoa imports `_AblyPluginSupportPr...
(QB_NEW_EN)
[grammar] ~16-~16: There might be a mistake here.
Context: ...'s internals to plugins via PluginAPI. - Currently, the plan is to test all the L...
(QB_NEW_EN)
[style] ~26-~26: Consider removing “of” to be more concise
Context: ...s names). ## Notes TODO incorporate - all of the stuff that depends on `_AblyPluginSuppo...
(ALL_OF_THE)
[style] ~27-~27: Unless you want to emphasize “not”, use “cannot” which is more common.
Context: ...` - note that _AblyPluginSupportPrivate can not depend on ably-cocoa at all, which mean...
(CAN_NOT_PREMIUM)
[style] ~27-~27: In American English, abbreviations like “etc.” require a period.
Context: ...el, ARTClientOptions, ARTErrorInfo` etc - TODO we can probably improve this...
(ETC_PERIOD)
🪛 markdownlint-cli2 (0.17.2)
Docs/plugins.md
9-9: Bare URL used
(MD034, no-bare-urls)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🪛 SwiftLint (0.57.0)
Package.swift
[Warning] 21-21: TODOs should be resolved (Unpin this before releasing Li...)
(todo)
⏰ 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: build
- 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 (31)
Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h (2)
1-2: Good gating with ABLY_SUPPORTS_PLUGINS; import scoping is correctThe header’s entire surface is gated, and the module import for _AblyPluginSupportPrivate is only pulled when the flag is defined. This matches the repo’s policy to use ABLY_SUPPORTS_PLUGINS (and not ABLY_IS_SPM_BUILD).
Also applies to: 17-17
4-4: Module import for _AblyPluginSupportPrivate is correctly guardedUsing @import (instead of direct header imports) under the compile flag keeps dependencies tidy and avoids leaking plugin types into non-plugin builds.
Source/PrivateHeaders/Ably/ARTInternalLog.h (4)
4-7: Guarded import of plugin support is appropriateLimiting _AblyPluginSupportPrivate to plugin builds prevents accidental type exposure and keeps non-plugin builds clean.
78-79: Doc reference update to ARTPluginAPI/APPluginAPIProtocol is consistentThe note aligns with the new PluginAPI surface and the gating strategy.
4-7: No lingering ABLY_IS_SPM_BUILD references foundA repository-wide search (
rg -nP '\bABLY_IS_SPM_BUILD\b') returned zero matches. All plugin gating now correctly usesABLY_SUPPORTS_PLUGINS.
40-45: Conditional protocol conformance is sound; verify APLogger implementation
Please confirm that when ABLY_SUPPORTS_PLUGINS is defined,ARTInternalLog.mimplements every method declared in theAPLoggerprotocol (for example,log:withLevel:,setLogLevel:, etc.).
- Locate the
APLoggerprotocol definition.- Cross-check each required selector against the implementations in Source/ARTInternalLog.m.
Source/PrivateHeaders/Ably/ARTChannel+Private.h (2)
17-18: Doc tweak looks fineClarifying that the logger exposure is for ARTPluginAPI usage is helpful and consistent with the new plugin surface.
11-12: AI summary is inconsistent with the current header contentThe initializer and internalPostMessages are still present here, whereas the summary claimed they were removed. If the intention was to remove them, this header needs updating; otherwise, the summary should be adjusted.
Would you like me to scan implementations/usages to confirm whether these members are still used, and if so where they’re referenced?
Also applies to: 20-21
Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (5)
14-17: Gated import of _AblyPluginSupportPrivate is correctKeeps plugin-only symbols out of non-plugin builds.
26-30: Adopting APPublicRealtimeChannel via class extension is fine under gatingThis makes the public wrapper conform for plugin builds without exposing APPublicRealtimeChannel to non-plugin consumers.
32-36: Conditional APRealtimeChannel conformance for the internal class looks goodFits with the strategy to keep plugin protocol dependencies behind ABLY_SUPPORTS_PLUGINS.
141-145: Ungated plugin-data helpers are OKThese methods don’t reference plugin types, so keeping them always available avoids proliferating #ifdefs and maintains a stable internal surface.
146-151: All gated API hooks are correctly implemented; no stale calls remain
- In
Source/ARTRealtimeChannel.munder#ifdef ABLY_SUPPORTS_PLUGINS, there is a matching
- (void)sendObjectWithObjectMessages:completion:implementation at line 423.- In
Source/ARTPluginAPI.m, the new API forwards to that plugin‐gated channel call.- No references to the old
sendStateWithObjectMessages:remain.No further action needed here unless you want to double-check your imports for the protocol definition under the same guard.
Package.resolved (1)
4-12: Pin added for private plugin support; plan to unpin to a tagged release before shipping.The resolved pin to ably-cocoa-plugin-support looks fine for a WIP branch, but make sure we switch to a tagged version (or at least a full revision in Package.swift) before release to avoid brittle dependency resolution.
Would you like me to open a follow-up task to track unpinning to a tag on ably-cocoa-plugin-support?
Package.swift (1)
29-31: Dependency wiring for _AblyPluginSupportPrivate looks correct.The product name aligns with imports used under ABLY_SUPPORTS_PLUGINS and keeps the plugin surface private.
Source/ARTJsonLikeEncoder.m (3)
31-36: Imports are correctly gated behind ABLY_SUPPORTS_PLUGINS.This avoids leaking plugin symbols when plugin support is disabled. Good.
581-586: State field encoding is properly gated.Conditionally adding “state” only when plugins are enabled and present is appropriate and non-invasive to existing payloads.
831-834: State field decoding is properly gated.This prevents AP* symbol usage on builds without plugin support.
Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (4)
3-5: Private plugin module import is correctly gated.This keeps plugin-only types out of non-plugin builds.
9-12: Conformance to APPublicClientOptions is correctly isolated to plugin-enabled builds.This maintains a clean separation of concerns without exposing plugin contracts broadly.
32-35: liveObjectsPlugin exposure is appropriate under a private header and flag.Readonly, nullable accessor under ABLY_SUPPORTS_PLUGINS is the right balance.
39-42: AI summary inconsistency: these remain instance methods (not class methods).The added methods here are instance methods; the summary claiming a migration to class methods is inaccurate. Keeping them as instance methods matches the intended design where ARTPluginAPI forwards to the options instance.
Ably.xcodeproj/project.pbxproj (6)
356-361: Plugin API wired into all Apple targets — OKAdding ARTPluginAPI.m to Sources across iOS/macOS/tvOS looks correct.
362-364: Underlying objects source added to all targets — OKARTPublicRealtimeChannelUnderlyingObjects.m is correctly included for iOS/macOS/tvOS.
386-388: New private category header added — OKARTErrorInfo+Private.h correctly marked Private for all platforms.
1928-1950: New “Plugin Support” group — structure looks goodCo-locating ARTPluginAPI, ARTPublicRealtimeChannelUnderlyingObjects and ARTPluginDecodingContext is a sensible organization.
2092-2093: Source group includes “Plugin Support” — OKThe group is correctly referenced under Source.
3492-3492: Sources added to all three framework targets — OKARTPublicRealtimeChannelUnderlyingObjects.m and ARTPluginAPI.m are included in iOS, macOS, and tvOS build phases.
Also applies to: 3751-3751, 3892-3892, 3534-3534, 3793-3793
Source/ARTPluginAPI.m (3)
133-136: Good: return a defensive copy of client optionsReturning
copyavoids leaking internal mutable state to plugins. Nice.
1-9: Build gating and imports look appropriate for plugin-bridgingGuarding the implementation with
ABLY_SUPPORTS_PLUGINSaligns with the project guidance; the selected private headers match the usage below.
95-101: No explicit import required for APDependencyStoreAPDependencyStore is already exposed via the
@import _AblyPluginSupportPrivate;in ARTPluginAPI.h, so the symbol will be visible at compile time. No “unknown receiver” warning should occur under normal module builds.If you ever need to support non-module compilations (or encounter unexpected errors), you can add:
#import "APDependencyStore.h"to Source/ARTPluginAPI.m.
lawrence-forooghian
force-pushed
the
ECO-5531-move-plugin-API-to-separate-repo
branch
from
c843790 to
f74f241
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2088/features
github-actions
bot
temporarily deployed
to
staging/pull/2088/jazzydoc
This modifies the implementation of the plugins mechanism introduced in 7f1cc48, moving _AblyPluginSupportPrivate out of ably-cocoa and into a new repository ably-cocoa-plugin-support [1], which becomes a shared dependency of ably-cocoa and its plugins. The motivation for moving _AblyPluginSupportPrivate out of ably-cocoa is so that this library product does not appear in Xcode's "Add Package Dependencies" UI when a user tries to add ably-cocoa to their app. This UI's initial state offers to add all of ably-cocoa's library products, which would likely lead to many users accidentally adding this private library as a dependency of their app. This change does introduce some implementation burden onto both ably-cocoa and its plugins. In order to eliminate circular dependencies between ably-cocoa and _AblyPluginSupportPrivate (which I needed to do because SPM doesn't allow circular dependencies), I've had to introduce a bunch of types in _AblyPluginSupportPrivate that duplicate ably-cocoa types, which necessitates a load of casting and converting (basically a bunch of boilerplate code). But, our previous usage of circular dependencies was a bit of a hack and felt somewhat held together by string and tape, so I'm happy for it to go away. A pleasant side-effect of this change is that it gives us an easy way to continue to iterate on the ably-cocoa <-> plugin internal API by decoupling the internal API's version number from that of ably-cocoa or the plugin; both ably-cocoa and the plugin can now declare a dependency on the same pre-release version of ably-cocoa-plugin-support, and we can bump this pre-release version number whenever we need to make a breaking change. (We should still aim to move to a 1.x version of ably-cocoa-plugin-support eventually so that users are not faced with cryptic SPM error messages about inconsistent dependency versions when they, say, try to upgrade ably-cocoa without upgrading the LiveObjects plugin.) [1] https://github.com/ably/ably-cocoa-plugin-support
lawrence-forooghian
force-pushed
the
ECO-5531-move-plugin-API-to-separate-repo
branch
from
f74f241 to
9e172dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/ARTJsonLikeEncoder.m (1)
1156-1163: Bug: uninitialized error out-parameter used in logerror is declared but not initialized before being passed by reference. If the plugin doesn’t set it, the subsequent log may print garbage or crash.
Apply this diff:
- id<APPublicErrorInfo> error; + id<APPublicErrorInfo> error = nil; APEncodingFormat format = [self apEncodingFormatFromARTEncoderFormat:[self format]]; id<APObjectMessageProtocol> objectMessage = [liveObjectsPlugin decodeObjectMessage:item context:decodingContext format:format error:&error];
♻️ Duplicate comments (2)
Ably.xcodeproj/project.pbxproj (2)
2527-2527: Same visibility issue applies to these header-phase entries.These are the platform-specific “in Headers” entries for the same files; once you flip the PBXBuildFile ATTRIBUTES to Public (see earlier comment), these will be exported correctly.
Also applies to: 2696-2697, 2974-2974, 2997-2997
356-367: Expose plugin support headers as Public (currently marked Private).If the plugin API is to be consumed from a separate repo, these headers must be Public; keeping them Private prevents downstream import from the built framework. Change the three PBXBuildFile entries for ARTPluginAPI.h and the three for ARTPublicRealtimeChannelUnderlyingObjects.h to Public.
Apply this diff:
-21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Private, ); }; }; +21A28E5D2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E5E2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E5F2E54CFC0007D0BFE /* ARTPluginAPI.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E5C2E54CFC0007D0BFE /* ARTPluginAPI.h */; settings = {ATTRIBUTES = (Public, ); }; }; -21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; -21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Private, ); }; }; +21A28E712E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E722E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; }; +21A28E732E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h in Headers */ = {isa = PBXBuildFile; fileRef = 21A28E702E54EF3D007D0BFE /* ARTPublicRealtimeChannelUnderlyingObjects.h */; settings = {ATTRIBUTES = (Public, ); }; };Would you like me to also update AblyPublic.h/Ably.modulemap to export these headers if they’re not already? I can generate a patch.
#!/bin/bash # Verify umbrella/module export of plugin headers set -euo pipefail echo "Searching for AblyPublic.h..." fd -t f -a "AblyPublic.h" | sed -n '1p' | xargs -I{} sh -c 'echo "--- {}"; rg -n "ARTPluginAPI|ARTPublicRealtimeChannelUnderlyingObjects" "{}" || true' echo echo "Searching modulemap..." fd -t f -a "Ably.modulemap" | sed -n '1p' | xargs -I{} sh -c 'echo "--- {}"; cat "{}"'
🧹 Nitpick comments (5)
Source/PrivateHeaders/Ably/ARTPluginAPI.h (1)
11-16: Ensure +registerSelf is idempotent and thread-safeThe docs imply idempotence. Please confirm the implementation guards against concurrent calls (e.g., dispatch_once or equivalent) to avoid duplicate registration under races.
Source/ARTJsonLikeEncoder.m (2)
1112-1122: Future-proof the enum conversion with a default branchapEncodingFormatFromARTEncoderFormat lacks a default path; adding one prevents silent UB if new encoder formats are introduced later.
Apply this diff:
#ifdef ABLY_SUPPORTS_PLUGINS /// Converts an `ARTEncoderFormat` to an `APEncodingFormat`. - (APEncodingFormat)apEncodingFormatFromARTEncoderFormat:(ARTEncoderFormat)format { switch (format) { case ARTEncoderFormatJson: return APEncodingFormatJSON; case ARTEncoderFormatMsgPack: return APEncodingFormatMessagePack; + default: + NSCAssert(NO, @"Unknown ARTEncoderFormat: %ld", (long)format); + return APEncodingFormatJSON; } } #endif
1136-1141: Optional: log when plugin is absent but state is presentWhen LiveObjects plugin isn’t configured, objectMessagesFromArray returns nil silently. A low-level log helps diagnose dropped state without being noisy.
Apply this diff:
- if (!liveObjectsPlugin) { - return nil; - } + if (!liveObjectsPlugin) { + ARTLogDebug(_logger, @"RS:%p ARTJsonLikeEncoder<%@>: LiveObjects plugin not configured; dropping ObjectMessage state for ProtocolMessage id=%@", _rest, [_delegate formatAsString], protocolMessage.id); + return nil; + }Docs/plugins.md (1)
24-24: Minor wording nit: “class names” → “type names”This is slightly more accurate given protocols and other types.
Apply this diff:
-- Make good use of `NS_SWIFT_NAME` to make `_AblyPluginSupportPrivate` nicer to use from Swift (e.g. to remove the `AP` prefix from class names). +- Make good use of `NS_SWIFT_NAME` to make `_AblyPluginSupportPrivate` nicer to use from Swift (e.g., to remove the `AP` prefix from type names).Ably.xcodeproj/project.pbxproj (1)
4275-4275: Fix incorrect HEADER_SEARCH_PATHS; point to Source/include and Source/PrivateHeaders.Current paths reference non-existent or legacy locations (“Source/Private”, repo-root “include”). Align all targets to the canonical roots to avoid brittle imports.
Apply these diffs:
-HEADER_SEARCH_PATHS = "$(PROJECT_DIR)/Source/Private"; +HEADER_SEARCH_PATHS = ( + "$(PROJECT_DIR)/Source/include", + "$(PROJECT_DIR)/Source/PrivateHeaders", +);-HEADER_SEARCH_PATHS = "$(SRCROOT)/include"; +HEADER_SEARCH_PATHS = ( + "$(SRCROOT)/Source/include", + "$(SRCROOT)/Source/PrivateHeaders", +);I can push a follow-up patch or keep this in the separate issue if you prefer minimal scope here. What’s your preference?
Also applies to: 4303-4303, 4476-4476, 4516-4516, 4551-4551, 4590-4590
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
.github/workflows/check-spm.yaml(1 hunks)Ably.podspec(0 hunks)Ably.xcodeproj/project.pbxproj(27 hunks)CONTRIBUTING.md(0 hunks)Docs/plugins.md(1 hunks)Package.resolved(1 hunks)Package.swift(2 hunks)Source/ARTClientOptions.m(4 hunks)Source/ARTJsonLikeEncoder.m(7 hunks)Source/ARTPluginAPI.m(1 hunks)Source/ARTPluginDecodingContext.m(2 hunks)Source/ARTPublicRealtimeChannelUnderlyingObjects.m(2 hunks)Source/ARTRealtimeChannel.m(7 hunks)Source/ARTStatus.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTChannel+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTInternalLog.h(3 hunks)Source/PrivateHeaders/Ably/ARTPluginAPI.h(1 hunks)Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h(2 hunks)Source/PrivateHeaders/Ably/ARTProtocolMessage.h(2 hunks)Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtime+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/include/module.modulemap(1 hunks)
💤 Files with no reviewable changes (2)
- CONTRIBUTING.md
- Ably.podspec
🚧 Files skipped from review as they are similar to previous changes (18)
- Source/ARTPublicRealtimeChannelUnderlyingObjects.m
- Source/include/module.modulemap
- Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h
- Source/ARTStatus.m
- Source/PrivateHeaders/Ably/ARTRealtime+Private.h
- Source/Ably.modulemap
- Source/PrivateHeaders/Ably/ARTProtocolMessage.h
- Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h
- Source/ARTPluginDecodingContext.m
- .github/workflows/check-spm.yaml
- Source/ARTClientOptions.m
- Package.resolved
- Source/PrivateHeaders/Ably/ARTChannel+Private.h
- Source/PrivateHeaders/Ably/ARTInternalLog.h
- Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h
- Source/ARTRealtimeChannel.m
- Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h
- Source/ARTPluginAPI.m
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2088
File: Package.swift:0-0
Timestamp: 2025-08-20T14:58:23.721Z
Learning: In ably-cocoa, ABLY_IS_SPM_BUILD should not be used anywhere; plugin gating uses ABLY_SUPPORTS_PLUGINS instead.
📚 Learning: 2025-08-20T14:58:23.721Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2088
File: Package.swift:0-0
Timestamp: 2025-08-20T14:58:23.721Z
Learning: In ably-cocoa, ABLY_IS_SPM_BUILD should not be used anywhere; plugin gating uses ABLY_SUPPORTS_PLUGINS instead.
Applied to files:
Source/PrivateHeaders/Ably/ARTPluginAPI.hPackage.swiftDocs/plugins.mdAbly.xcodeproj/project.pbxproj
📚 Learning: 2025-08-20T16:47:07.996Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2088
File: Ably.xcodeproj/project.pbxproj:4275-4275
Timestamp: 2025-08-20T16:47:07.996Z
Learning: In ably-cocoa’s Xcode project, the correct header roots are Source/include (public) and Source/PrivateHeaders (private). Ably-iOS previously pointed to Source/Private (nonexistent), and macOS/tvOS to repo-root include; future header search paths should consistently use Source/include and Source/PrivateHeaders across targets.
Applied to files:
Package.swiftAbly.xcodeproj/project.pbxproj
📚 Learning: 2024-10-08T15:37:26.766Z
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.
Applied to files:
Docs/plugins.md
📚 Learning: 2024-11-26T17:50:25.730Z
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/ARTJsonLikeEncoder.m
📚 Learning: 2024-10-08T15:37:26.766Z
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/ARTJsonLikeEncoder.mSource/PrivateHeaders/Ably/ARTClientOptions+Private.hAbly.xcodeproj/project.pbxproj
📚 Learning: 2024-11-04T23:10:23.027Z
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/ARTJsonLikeEncoder.mAbly.xcodeproj/project.pbxproj
📚 Learning: 2025-08-13T16:50:28.236Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2084
File: Source/ARTClientOptions.m:249-251
Timestamp: 2025-08-13T16:50:28.236Z
Learning: In ARTClientOptions.m, the setPluginOptionsValue:forKey: method intentionally provides a simple dictionary-like interface without defensive copying, expecting callers to handle proper value management themselves rather than adding defensive programming overhead.
Applied to files:
Source/PrivateHeaders/Ably/ARTClientOptions+Private.h
📚 Learning: 2025-06-16T14:12:36.705Z
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)
Package.swift
[Warning] 21-21: TODOs should be resolved (Unpin this before releasing Li...)
(todo)
🪛 LanguageTool
Docs/plugins.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...e access to _AblyPluginSupportPrivate. - ably-cocoa provides an implementation of...
(QB_NEW_EN)
[grammar] ~14-~14: Ensure spelling is correct
Context: ...ependencyStore`, from where plugins can subsequetly fetch it. - Currently, the plan is to t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~14-~14: There might be a mistake here.
Context: ... where plugins can subsequetly fetch it. - Currently, the plan is to test all the L...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...emove the AP prefix from class names).
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
Docs/plugins.md
9-9: Bare URL used
(MD034, no-bare-urls)
⏰ 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
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (iOS, test_iOS18_4)
🔇 Additional comments (18)
Source/PrivateHeaders/Ably/ARTPluginAPI.h (1)
1-5: Good: Plugin support correctly gated and imported for SPM-only buildsHeader content is wrapped in ABLY_SUPPORTS_PLUGINS and uses @import for _AblyPluginSupportPrivate. This matches the agreed gating strategy and prevents accidental exposure in non-SPM builds.
Package.swift (3)
20-23: Pinned plugin-support dependency: track and unpin before releaseYou’ve pinned ably-cocoa-plugin-support to a specific revision with a TODO. Recommend converting to a tagged version (or upToNextMajor) before shipping the LiveObjects branch to avoid drift and ease auditing.
29-31: Target-level dependency on _AblyPluginSupportPrivate looks correctUsing the private product as a target dependency avoids surfacing additional products to app integrators while keeping plugin internals available to Ably. LGTM.
41-41: Correct flag: ABLY_SUPPORTS_PLUGINS (not ABLY_IS_SPM_BUILD)Defining ABLY_SUPPORTS_PLUGINS here aligns with the guards across the codebase and the stated approach. Good consistency.
Source/PrivateHeaders/Ably/ARTClientOptions+Private.h (4)
3-5: Gated import of plugin-support is correctKeeping _AblyPluginSupportPrivate imports behind ABLY_SUPPORTS_PLUGINS prevents non-SPM builds from seeing plugin types. LGTM.
9-12: Conditional conformance to APPublicClientOptions is appropriateConforming only when plugin support is enabled keeps the private header lean for non-plugin builds. LGTM.
32-35: liveObjectsPlugin exposure is properly gatedProperty is correctly behind ABLY_SUPPORTS_PLUGINS and nullable. This avoids leaking AP* types in non-SPM builds. LGTM.
39-42: Plugin options helpers placement makes senseThese remain available without AP* types and keep the storage logic centralized. Documentation references to APPluginAPIProtocol are clear. LGTM.
Source/ARTJsonLikeEncoder.m (4)
31-31: Import of ARTClientOptions+Private is necessary for plugin accessThis import unlocks access to liveObjectsPlugin on _rest.options. LGTM.
32-35: Plugin imports are properly guardedARTPluginDecodingContext and _AblyPluginSupportPrivate are wrapped in ABLY_SUPPORTS_PLUGINS. LGTM.
581-585: State serialization is correctly gatedEncoding message.state only when plugin support is enabled avoids schema divergence across builds. LGTM.
831-833: State deserialization path is consistent with encoding pathDecoding into message.state is gated and aligns with the encoding logic. LGTM.
Docs/plugins.md (1)
5-5: Doc structure and repo link are clearThe move to a separate support repo and the rationale are explained succinctly. LGTM.
Ably.xcodeproj/project.pbxproj (5)
356-358: LGTM: wiring new plugin sources into all Apple targets.Adding ARTPluginAPI.m and ARTPublicRealtimeChannelUnderlyingObjects.m across iOS/macOS/tvOS Sources build phases looks correct.
Also applies to: 362-364, 3492-3492, 3534-3534, 3793-3793, 3892-3892
386-388: LGTM: ARTErrorInfo+Private.h added as Private.Keeping this as Private matches the intent of internal-only error helpers.
Also applies to: 2617-2617, 2750-2750
1417-1420: LGTM: Added file references for plugin support files.File refs for ARTPluginAPI.{h,m} and ARTPublicRealtimeChannelUnderlyingObjects.{h,m} are consistent with the group structure.
1938-1950: LGTM: New “Plugin Support” group organization is clear.Good separation; keeping decoding context and public-underlying objects together improves discoverability.
2092-2093: LGTM: Exposed “Plugin Support” under Source.Group is visible in the main Source tree; no concerns.
lawrence-forooghian
changed the title
PluginAPI to an external repo
lawrence-forooghian
changed the title
PluginAPI to an external repoAblyPlugin to an external repo
lawrence-forooghian
changed the title
AblyPlugin to an external repoAblyPlugin to an external repo
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
Docs/plugins.md (2)
7-7: Tighten language in ADR note (duplicate of prior suggestion)Rephrase to neutral, concise wording and reflect the repo split.
-I will expand on this documentation once this mechanism is more mature; currently [ADR-128: Plugins for ably-cocoa SDK](https://ably.atlassian.net/wiki/spaces/ENG/pages/3838574593/ADR-128+Plugins+for+ably-cocoa+SDK) provides a decent description of our approach. The key divergence between ADR-128 and our current approach is that `_AblyPluginSupportPrivate` (called `AblyPlugin` in the ADR) is no longer part of ably-cocoa and instead lives in a separate repository. I will update that ADR or create a new one later on as part of clearing technical debt caused by this last-minute change of approach. +We will expand this documentation once the mechanism is more mature; currently [ADR-128: Plugins for ably-cocoa SDK](https://ably.atlassian.net/wiki/spaces/ENG/pages/3838574593/ADR-128+Plugins+for+ably-cocoa+SDK) describes the approach. The key divergence is that `_AblyPluginSupportPrivate` (called `AblyPlugin` in the ADR) is no longer part of ably-cocoa and instead lives in a separate repository. The ADR will be updated or replaced to reflect this change.
13-21: Fix typos and polish phrasing (duplicate of prior suggestion)Address “subsequetly”→“subsequently”, “can not”→“cannot”, “casted”→“cast”, and minor clarity tweaks.
- - Everything in ably-cocoa that depends on `_AblyPluginSupportPrivate` is gated behind `#ifdef ABLY_SUPPORTS_PLUGINS`, which is only defined in SPM builds. This is so as not to affect the non-SPM builds (i.e. Xcode, CocoaPods), which do not have access to `_AblyPluginSupportPrivate`. - - ably-cocoa provides an implementation of `_AblyPluginSupportPrivate`'s `APPluginAPI` protocol, which is the interface that plugins use to access ably-cocoa's internals. On library initialization, it registers this implementation with `APDependencyStore`, from where plugins can subsequetly fetch it. + - Everything in ably-cocoa that depends on `_AblyPluginSupportPrivate` is gated behind `#ifdef ABLY_SUPPORTS_PLUGINS`, which is only defined in SPM builds. This avoids affecting non-SPM builds (i.e., Xcode, CocoaPods), which do not have access to `_AblyPluginSupportPrivate`. + - ably-cocoa provides an implementation of `_AblyPluginSupportPrivate`’s `APPluginAPI` protocol, which is the interface plugins use to access ably-cocoa internals. On library initialization, it registers this implementation with `APDependencyStore`, from which plugins can subsequently fetch it. @@ - - The underscore and `Private` are to reduce the chances of the user accidentally importing it into their project. + - The leading underscore and `Private` suffix reduce the chance of users accidentally importing it into their projects. - - `_AblyPluginSupportPrivate` can not refer to any of ably-cocoa's types (since SPM forbids circular dependencies) and so it contains various types that duplicate ably-cocoa's public types. These fall into two categories: + - `_AblyPluginSupportPrivate` cannot refer to any ably-cocoa types (SPM forbids circular dependencies), so it defines types that duplicate ably-cocoa’s public types. These fall into two categories: @@ - - marker protocols e.g. `APPublicRealtimeChannel` which are empty protocols whose instances can always be safely casted to and from their corresponding ably-cocoa type (in this example, `ARTRealtimeChannel`) + - marker protocols e.g. `APPublicRealtimeChannel`, which are empty protocols whose instances can always be safely cast to and from their corresponding ably-cocoa type (in this example, `ARTRealtimeChannel`)Source/ARTPluginAPI.m (3)
53-72: Channel state mapping fixed; now complete.The wrong enum label reported earlier is corrected (
ARTRealtimeChannelFailed→APRealtimeChannelStateFailed), and the post-switch exception keeps mappings strict. LGTM.
74-91: Log level conversion is robust.Explicit cases for all known APLogLevel values plus a hard failure for unknowns is acceptable. LGTM.
145-148: Wrap completion to adapt block type (ARTErrorInfo → id).*Passing
completiondirectly risks an incompatible block pointer type warning. Wrap it so the internal API receives anARTCallbackand the public completion remains typed toid<APPublicErrorInfo>.Apply this diff:
- [_internalRealtimeChannel(channel) sendObjectWithObjectMessages:objectMessages - completion:completion]; + [_internalRealtimeChannel(channel) sendObjectWithObjectMessages:objectMessages + completion:^(ARTErrorInfo * _Nullable error) { + if (completion) { + completion(error); + } + }];
🧹 Nitpick comments (14)
.github/workflows/check-spm.yaml (2)
24-26: Warnings-as-errors step is still enabled (summary says removed)The AI summary states the step was removed, but the job continues to run swift build with -warnings-as-errors. If the intent is to keep builds warning-free, this is fine; otherwise, drop the flag.
Apply if you do want to drop it:
- # https://forums.swift.org/t/warnings-as-errors-for-libraries-frameworks/58393/2 - - run: swift build -Xswiftc -warnings-as-errors + # Build SPM target(s) + - run: swift build
14-23: Verify runner and Xcode availability on GitHub-hosted macOS imagesmacos-15 with Xcode 16.3 may not be available across all org runners. Consider macos-latest or pin to a known available image, and cache toolchains if needed.
If needed, a more resilient setup:
- runs-on: macos-15 + runs-on: macos-latest @@ - - uses: maxim-lobanov/setup-xcode@v1 - with: - xcode-version: 16.3 + - uses: maxim-lobanov/setup-xcode@v1 + with: + xcode-version: 16.3 # or remove to use the image defaultOptionally ensure resolution and tests compile too:
- - run: swift build -Xswiftc -warnings-as-errors + - run: swift package resolve + - run: swift build -Xswiftc -warnings-as-errors + - run: swift test -c debug --parallelSource/Ably.modulemap (2)
138-142: Ensure plugin headers are self-contained when ABLY_SUPPORTS_PLUGINS is undefinedSince Xcode always consumes this modulemap (SPM uses a different one), these headers must compile cleanly even when ABLY_SUPPORTS_PLUGINS is not set and _AblyPluginSupportPrivate is unavailable. Please confirm each header either:
- contains only forward declarations or
- wraps any dependency on _AblyPluginSupportPrivate in #ifdef ABLY_SUPPORTS_PLUGINS blocks.
Otherwise, non-SPM/Xcode builds can break at import time.
If any header cannot be made inert, consider moving only the parts that are independent of the plugin support module into these headers, and placing the dependent pieces into .mm/.m files or separate headers that are not listed in the Xcode modulemap.
138-142: Optional: group or comment these as “Plugin Support” for future maintainersA tiny navigability win: add a comment marker to signal why these four headers are here.
header "ARTDeviceStorage.h" - header "ARTPluginDecodingContext.h" - header "ARTPluginAPI.h" - header "ARTPublicRealtimeChannelUnderlyingObjects.h" - header "ARTErrorInfo+Private.h" + // Plugin Support (private, guarded in headers by ABLY_SUPPORTS_PLUGINS) + header "ARTPluginDecodingContext.h" + header "ARTPluginAPI.h" + header "ARTPublicRealtimeChannelUnderlyingObjects.h" + header "ARTErrorInfo+Private.h"Source/ARTRealtimeChannel.m (5)
68-77: Make ARTPluginAPI registration idempotentinitWithInternal: will run per-channel. To avoid repeated registration (and any potential side effects), wrap [ARTPluginAPI registerSelf] with dispatch_once here, or make registerSelf itself idempotent.
- // The LiveObjects repository provides an extension to `ARTRealtimeChannel` so we need to ensure that we register the pluginAPI before that extension is used. - [ARTPluginAPI registerSelf]; + // Ensure plugin API registered exactly once before any LiveObjects extensions are used. + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + [ARTPluginAPI registerSelf]; + });Follow-up: if registerSelf is already idempotent, add a brief comment stating so and leave as-is.
711-715: Guard cross-repo protocol evolution to avoid unrecognized selector at runtimeIf a plugin built against an older _AblyPluginSupportPrivate doesn’t implement onChannelAttached:hasObjects:, this direct call would crash at runtime. Guarding by respondsToSelector (with a fallback log) improves resilience during transition periods.
- [self.realtime.options.liveObjectsPlugin onChannelAttached:self - hasObjects:message.hasObjects]; + id plugin = self.realtime.options.liveObjectsPlugin; + SEL sel = NSSelectorFromString(@"onChannelAttached:hasObjects:"); + if ([plugin respondsToSelector:sel]) { + // Suppress ARC warning for performSelector leaks is unnecessary since no ARC-managed objects are returned. + ((void (*)(id, SEL, id, BOOL))objc_msgSend)(plugin, sel, self, message.hasObjects); + } else { + ARTLogDebug(self.logger, @"R:%p C:%p (%@) LiveObjects plugin does not respond to onChannelAttached:hasObjects:, skipping.", _realtime, self, self.name); + }Note: This keeps binary compatibility if the protocol evolves across repos. If you mandate a minimum plugin support version, documenting that may be sufficient instead.
422-439: Queue assertion and publish path look sounddispatch_assert_queue(_queue) enforces internal serialization; constructing ARTProtocolMessage with action=ARTProtocolMessageObject and routing through publishProtocolMessage centralizes ack/error handling.
Minor naming nit: sendObjectWithObjectMessages: could be shortened to sendObjectMessages: for readability.
898-907: Log early return to aid diagnostics when pm.state is nilRight now the code silently returns if the plugin isn’t set up or decoding failed. A debug-level log helps correlate dropped messages during integration.
- if (!pm.state) { - // Because the plugin isn't set up or because decoding failed - return; - } + if (!pm.state) { + // Plugin not set up or decoding failed + ARTLogDebug(self.logger, @"R:%p C:%p (%@) OBJECT message ignored: plugin state unavailable.", _realtime, self, self.name); + return; + }
910-920: Same: add a debug log for OBJECT_SYNC early returnParity with onObject: improves traceability during plugin bootstrap.
- if (!pm.state) { - // Because the plugin isn't set up or because decoding failed - return; - } + if (!pm.state) { + ARTLogDebug(self.logger, @"R:%p C:%p (%@) OBJECT_SYNC ignored: plugin state unavailable.", _realtime, self, self.name); + return; + }Docs/plugins.md (2)
9-9: Avoid bare URL (MD034)Use Markdown link syntax for the LiveObjects plugin URL.
-Currently, our only plugin is for adding LiveObjects functionality. This plugin can be found at https://github.com/ably/ably-cocoa-liveobjects-plugin. +Currently, our only plugin is for adding LiveObjects functionality. This plugin can be found at [ably-cocoa-liveobjects-plugin](https://github.com/ably/ably-cocoa-liveobjects-plugin).
24-25: Minor grammar polishSmall readability tweak.
-- Make good use of `NS_SWIFT_NAME` to make `_AblyPluginSupportPrivate` nicer to use from Swift (e.g. to remove the `AP` prefix from class names). +- Use `NS_SWIFT_NAME` to make `_AblyPluginSupportPrivate` nicer to use from Swift (e.g., to drop the `AP` prefix from type names).Source/ARTPluginAPI.m (3)
5-5: Remove unused import to tighten compile surface.
ARTChannel+Private.his not referenced in this file. Drop it to reduce build time and accidental coupling.Apply this diff:
-#import "ARTChannel+Private.h"
11-16: Unused helper_ourPublicErrorInfo— prune to avoid dead code warnings.This function isn’t used in this file. Unless it’s referenced elsewhere soon, remove it.
Apply this diff:
-static ARTErrorInfo *_ourPublicErrorInfo(id<APPublicErrorInfo> pluginPublicErrorInfo) { - if (![pluginPublicErrorInfo isKindOfClass:[ARTErrorInfo class]]) { - [NSException raise:NSInternalInconsistencyException format:@"Expected ARTErrorInfo, got %@", pluginPublicErrorInfo]; - } - return (ARTErrorInfo *)pluginPublicErrorInfo; -}
53-91: Add small unit tests for enum conversions.Recommend tests to lock down mappings and the failure behavior on unknown values for both state and log level conversions.
I can draft XCTests that:
- Enumerate all ARTRealtimeChannelState values and assert the expected APRealtimeChannelState.
- Enumerate all APLogLevel values and assert the expected ARTLogLevel, plus a targeted test verifying the exception path via an injected unknown value (if feasible).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
.github/workflows/check-spm.yaml(1 hunks)Ably.podspec(0 hunks)Ably.xcodeproj/project.pbxproj(27 hunks)CONTRIBUTING.md(0 hunks)Docs/plugins.md(1 hunks)Package.resolved(1 hunks)Package.swift(2 hunks)Source/ARTClientOptions.m(4 hunks)Source/ARTJsonLikeEncoder.m(7 hunks)Source/ARTPluginAPI.m(1 hunks)Source/ARTPluginDecodingContext.m(2 hunks)Source/ARTPublicRealtimeChannelUnderlyingObjects.m(2 hunks)Source/ARTRealtimeChannel.m(7 hunks)Source/ARTStatus.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTChannel+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTClientOptions+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTInternalLog.h(3 hunks)Source/PrivateHeaders/Ably/ARTPluginAPI.h(1 hunks)Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h(2 hunks)Source/PrivateHeaders/Ably/ARTProtocolMessage.h(2 hunks)Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtime+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/include/module.modulemap(1 hunks)
💤 Files with no reviewable changes (2)
- CONTRIBUTING.md
- Ably.podspec
✅ Files skipped from review due to trivial changes (1)
- Ably.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (17)
- Source/include/module.modulemap
- Package.resolved
- Source/PrivateHeaders/Ably/ARTRealtime+Private.h
- Source/ARTJsonLikeEncoder.m
- Source/PrivateHeaders/Ably/ARTPublicRealtimeChannelUnderlyingObjects.h
- Source/PrivateHeaders/Ably/ARTInternalLog.h
- Source/ARTPublicRealtimeChannelUnderlyingObjects.m
- Source/PrivateHeaders/Ably/ARTPluginDecodingContext.h
- Source/PrivateHeaders/Ably/ARTProtocolMessage.h
- Source/ARTClientOptions.m
- Package.swift
- Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h
- Source/ARTStatus.m
- Source/PrivateHeaders/Ably/ARTClientOptions+Private.h
- Source/ARTPluginDecodingContext.m
- Source/PrivateHeaders/Ably/ARTPluginAPI.h
- Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2088
File: Package.swift:0-0
Timestamp: 2025-08-20T14:58:23.721Z
Learning: In ably-cocoa, ABLY_IS_SPM_BUILD should not be used anywhere; plugin gating uses ABLY_SUPPORTS_PLUGINS instead.
📚 Learning: 2024-11-04T23:10:23.027Z
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/ARTPluginAPI.mSource/Ably.modulemapSource/ARTRealtimeChannel.mSource/PrivateHeaders/Ably/ARTChannel+Private.h
📚 Learning: 2025-08-13T16:50:28.236Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2084
File: Source/ARTClientOptions.m:249-251
Timestamp: 2025-08-13T16:50:28.236Z
Learning: In ARTClientOptions.m, the setPluginOptionsValue:forKey: method intentionally provides a simple dictionary-like interface without defensive copying, expecting callers to handle proper value management themselves rather than adding defensive programming overhead.
Applied to files:
Source/ARTPluginAPI.m
📚 Learning: 2025-08-20T14:58:23.721Z
Learnt from: lawrence-forooghian
PR: ably/ably-cocoa#2088
File: Package.swift:0-0
Timestamp: 2025-08-20T14:58:23.721Z
Learning: In ably-cocoa, ABLY_IS_SPM_BUILD should not be used anywhere; plugin gating uses ABLY_SUPPORTS_PLUGINS instead.
Applied to files:
.github/workflows/check-spm.yamlDocs/plugins.md
📚 Learning: 2024-10-08T15:37:26.766Z
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/ARTRealtimeChannel.mSource/PrivateHeaders/Ably/ARTChannel+Private.h
📚 Learning: 2024-10-08T15:37:26.766Z
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.
Applied to files:
Docs/plugins.md
🪛 LanguageTool
Docs/plugins.md
[grammar] ~13-~13: There might be a mistake here.
Context: ...e access to _AblyPluginSupportPrivate. - ably-cocoa provides an implementation of...
(QB_NEW_EN)
[grammar] ~14-~14: Ensure spelling is correct
Context: ...ependencyStore`, from where plugins can subsequetly fetch it. - Currently, the plan is to t...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~14-~14: There might be a mistake here.
Context: ... where plugins can subsequetly fetch it. - Currently, the plan is to test all the L...
(QB_NEW_EN)
[grammar] ~24-~24: There might be a mistake here.
Context: ...emove the AP prefix from class names).
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
Docs/plugins.md
9-9: Bare URL used
(MD034, no-bare-urls)
⏰ 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
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (13)
Source/PrivateHeaders/Ably/ARTChannel+Private.h (2)
17-18: Doc comment tweak looks correctReferencing ARTPluginAPI (instead of APPluginAPI) matches the new private plugin bridge and avoids confusion.
11-12: AI summary inconsistency: initializer and internalPostMessages still presentThe AI summary claims these were removed, but both the initializer and -internalPostMessages:callback: are still declared here. Proceed assuming they remain part of the private surface, or update the summary/PR description to avoid confusion.
Also applies to: 20-21
.github/workflows/check-spm.yaml (1)
1-3: Workflow rename/description LGTM“Check SPM build” and the clarified description align with gating plugin support via SPM.
Source/ARTRealtimeChannel.m (2)
39-43: Good: plugin imports gated by ABLY_SUPPORTS_PLUGINSUsing @import _AblyPluginSupportPrivate under the guard avoids pulling the private module into non-SPM builds.
422-439: VerifysendObjectWithObjectMessages:is always invoked on the channel’s internal queueThe internal implementation in
ARTRealtimeChannel.masserts_queue, but the plugin‐bridge inARTPluginAPI.mcalls it directly:
Source/ARTRealtimeChannel.m (423–430):
dispatch_assert_queue(_queue); // … build and publish pm … }Source/ARTPluginAPI.m (145–147):
[_internalRealtimeChannel(channel) sendObjectWithObjectMessages:objectMessages completion:completion];You need to ensure that this bridge runs on the channel’s dispatch queue. Two approaches:
• Wrap the call here in a
dispatch_asyncon the channel’s queue:- [_internalRealtimeChannel(channel) sendObjectWithObjectMessages:objectMessages - completion:completion]; + ARTRealtimeChannelInternal *internal = _internalRealtimeChannel(channel); + dispatch_async(internal.queue, ^{ + [internal sendObjectWithObjectMessages:objectMessages + completion:completion]; + });• Or, if it’s always driven by the plugin runner, confirm that every invocation is scheduled on the queue obtained via
-internalQueueForClient:(i.e. the plugin should dispatch there before calling this API).Either update the bridge or document/verify that callers are dispatching correctly to avoid triggering the assertion.
Docs/plugins.md (1)
11-14: No strayABLY_IS_SPM_BUILDreferences foundI ran a project-wide search for
ABLY_IS_SPM_BUILDand confirmed there are no occurrences. All plugin-related gating relies solely onABLY_SUPPORTS_PLUGINS, as intended. This comment can be resolved.Source/ARTPluginAPI.m (7)
1-1: Plugin gating is correct (ABLY_SUPPORTS_PLUGINS).Good adherence to the packaging plan; this aligns with the learning to gate via ABLY_SUPPORTS_PLUGINS rather than ABLY_IS_SPM_BUILD.
Also applies to: 167-167
103-108: Bridging underlying objects looks correct.Passing
realtimeInternalandinternalintoAPDefaultPublicRealtimeChannelUnderlyingObjectsmatches the intended public–internal split. LGTM.
125-131: Plugin options bridging matches the intended lightweight contract.Forwarding directly to
setPluginOptionsValue:forKey:without defensive copying is consistent with prior decisions for this API.
133-135: Returning a copy of client options is a good defensive choice.Prevents plugins from mutating internal state. LGTM.
121-123: Ensure compile-time protocol conformance — VerifiedThe class returned by
loggerForChannel:isARTInternalLog, which does indeed adopt theAPLoggerprotocol when plugin support is enabled:
- In
Source/PrivateHeaders/Ably/ARTInternalLog.hat line 41, under#ifdef ABLY_SUPPORTS_PLUGINS, you’ll find:Outside that block (when plugins aren’t supported),@interface ARTInternalLog : NSObject <APLogger>ARTInternalLogisn’t compiled, so the method never returns a non-conforming type.No changes are necessary here.
137-143: Queues are guaranteed nonnull — no fallback neededThe
dispatchQueueandinternalDispatchQueueproperties onARTClientOptionsare declared underNS_ASSUME_NONNULL_BEGINin the public header and are initialized to non-nil values in-initDefaults:
dispatchQueueis set todispatch_get_main_queue().internalDispatchQueueis set to a newly created serial queue viadispatch_queue_create(...).Because these properties are nonnullable by contract and always have valid defaults, the methods in
ARTPluginAPI.mthat return those queues can safely assume non-nil values. No additional documentation or fallback logic is required.
95-101: Verify APDependencyStore is visible to ARTPluginAPI.mWe’re calling
[[APDependencyStore sharedInstance] registerPluginAPI:instanceToRegister];in
+registerSelf, butARTPluginAPI.monly does:#import "ARTPluginAPI.h"and
ARTPluginAPI.hdoes@import _AblyPluginSupportPrivate;. You need to ensure that the_AblyPluginSupportPrivatemodule (or umbrella header) actually exports the header declaringAPDependencyStore. If it doesn’t, this call will break in non-modular or stricter build variants.• Confirm in your plugin-support module map (or umbrella header) that the
APDependencyStoreinterface is included under_AblyPluginSupportPrivate.
• If it isn’t, add an explicit import inARTPluginAPI.m, for example:#import "ARTPluginAPI.h" +#import <AblyPluginSupportPrivate/APDependencyStore.h> // adjust to actual pathThis ensures
APDependencyStoreis guaranteed available regardless of transitive module setups.
umair-ably
left a comment
umair-ably
left a comment
There was a problem hiding this comment.
LGTM - good documentation 👌
lawrence-forooghian
merged commit 1544881
into
integration/liveobjects
lawrence-forooghian
deleted the
ECO-5531-move-plugin-API-to-separate-repo
branch
2 participants
(Note: The first thing that this PR does is to rename the
AblyPluginlibrary to_AblyPluginSupportPrivatein order to reduce the chance of a user importing it by accident; all subsequent references in this PR description are to_AblyPluginSupportPrivate.)This modifies the implementation of the plugins mechanism introduced in 7f1cc48, moving
_AblyPluginSupportPrivateout of ably-cocoa and into a new repository,ably-cocoa-plugin-support, which becomes a shared dependency of ably-cocoa and its plugins.The motivation for moving
_AblyPluginSupportPrivateout of ably-cocoa is so that this library product does not appear in Xcode's "Add Package Dependencies" UI when a user tries to add ably-cocoa to their app. This UI's initial state offers to add all of ably-cocoa's library products, which would likely lead to many users accidentally adding this private library as a dependency of their app. Here's an example of what that UI looks like (screenshot taken before renamingAblyPluginto_AblyPluginSupportPrivate):This change does introduce some implementation burden onto both ably-cocoa and its plugins. In order to eliminate circular dependencies between ably-cocoa and
_AblyPluginSupportPrivate(which I needed to do because SPM doesn't allow circular dependencies), I've had to introduce a bunch of types in_AblyPluginSupportPrivatethat duplicate ably-cocoa types, which necessitates a load of casting and converting (basically a bunch of boilerplate code). But, our previous usage of circular dependencies was a bit of a hack and felt somewhat held together by string and tape, so I'm happy for it to go away.A pleasant side-effect of this change is that it gives us an easy way to continue to iterate on the ably-cocoa <-> plugin internal API by decoupling the internal API's version number from that of ably-cocoa or the plugin; both ably-cocoa and the plugin can now declare a dependency on the same pre-release version of
ably-cocoa-plugin-support, and we can bump this pre-release version number whenever we need to make a breaking change. (We should still aim to move to a 1.x version ofably-cocoa-plugin-supporteventually so that users are not faced with cryptic SPM error messages about inconsistent dependency versions when they, say, try to upgrade ably-cocoa without upgrading the LiveObjects plugin.)The corresponding LiveObjects plugin changes can be found in ably/ably-liveobjects-swift-plugin#69.
Summary by CodeRabbit
New Features
Documentation
Chores
Refactor