Skip to content

Conversation

@lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Aug 19, 2025

Note: This PR is based on top of #66; please review that one first.

This changes the plugin architecture so that the plugin-related protocols now come from a separate repository (ably/ably-cocoa-plugin-support). This is necessitated by ably/ably-cocoa#2088, which explains the motivation for this change.

Summary by CodeRabbit

  • New Features
    • Added ARTRealtimeChannel.objects for direct access to Live Objects.
  • Documentation
    • Updated import guidance and array-literal formatting examples.
  • Refactor
    • Migrated internals to a private plugin support layer and unified logging types with no behavior changes.
  • Chores
    • Added a new package dependency and updated package lockfiles/submodule references.

@lawrence-forooghian lawrence-forooghian changed the base branch from main to 65-LiveMapValue-improvements August 19, 2025 20:46
@coderabbitai
Copy link

coderabbitai bot commented Aug 19, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The PR migrates from AblyPlugin to _AblyPluginSupportPrivate across the package, updates dependencies to add ably-cocoa-plugin-support, introduces an internal logger abstraction, updates wire/encoding and marker-bridging helpers, adjusts channel-state handling via the plugin API, revises tests accordingly, and adds a public ARTRealtimeChannel.objects property.

Changes

Cohort / File(s) Summary
Docs: rules
..cursor/rules/swift.mdc, ..cursor/rules/testing.mdc
Update guidance to import _AblyPluginSupportPrivate; adjust array literal formatting example.
Package management
Package.swift, Package.resolved, AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved, ably-cocoa
Add ably-cocoa-plugin-support dependency (pinned); switch product usages to _AblyPluginSupportPrivate; update submodule pointer for ably-cocoa.
Core SDK bridge
Sources/.../Internal/CoreSDK.swift, Sources/.../Public/ARTRealtimeChannel+Objects.swift
Replace AblyPlugin types with _AblyPluginSupportPrivate; channel state read via pluginAPI; add public ARTRealtimeChannel.objects; wrap plugin logger with DefaultLogger.
Internal plugin
Sources/.../Internal/DefaultInternalPlugin.swift
Migrate protocols/types to _AblyPluginSupportPrivate; adjust object message encode/decode bridging and error/ data conversions.
Client options bridge
Sources/.../Internal/ARTClientOptions+Objects.swift
Use Plugin.defaultPluginAPI and PublicClientOptions bridging; update imports.
Internal objects and pool
Sources/.../Internal/InternalDefaultLiveCounter.swift, .../Internal/InternalDefaultLiveMap.swift, .../Internal/InternalDefaultRealtimeObjects.swift, .../Internal/ObjectsPool.swift, .../Internal/InternalLiveObject.swift, .../Internal/ObjectCreationHelpers.swift, .../Internal/LiveObjectMutableState.swift
Replace AblyPlugin.Logger with internal Logger; update imports and signatures; logic unchanged.
Protocol wire types
Sources/.../Protocol/ObjectMessage.swift, Sources/.../Protocol/WireObjectMessage.swift
Switch encoding/decoding types to _AblyPluginSupportPrivate.EncodingFormat and DecodingContextProtocol.
Utilities: logging/bridging
Sources/.../Utility/Logger.swift, .../Utility/MarkerProtocolHelpers.swift, .../Utility/WireValue.swift, .../Utility/InternalError.swift, .../Utility/Errors.swift, .../Utility/APLogger+Swift.swift
Add internal Logger and DefaultLogger; add marker-bridging helpers; rename WireValue bridging APIs to pluginSupport variants; add PublicErrorInfo to InternalError conversion; update error case to private RealtimeChannelState; remove old AblyPlugin logger extension.
Public proxies
Sources/.../Public/Public Proxy Objects/*
Replace AblyPlugin.Logger with internal Logger in counters, maps, realtime objects, object store.
Plugin API access
Sources/.../Public/Plugin.swift
Fetch plugin API via DependencyStore; switch imports and references to private plugin support.
Tests: type/import migration
Tests/AblyLiveObjectsTests/*
Import _AblyPluginSupportPrivate; replace ARTRealtimeChannelState with private RealtimeChannelState; update decoding context; update WireValue tests to pluginSupport naming; adjust TestLogger to new Logger API; minor JS helper dictionary rename.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor App as App Code
  participant ARC as ARTRealtimeChannel
  participant LO as AblyLiveObjects
  participant API as _AblyPluginSupportPrivate.PluginAPI
  participant L as DefaultLogger
  participant CORES as DefaultCoreSDK
  participant INT as InternalDefaultRealtimeObjects

  App->>ARC: access .objects
  ARC->>API: underlyingObjects(for: asPluginPublicRealtimeChannel)
  API-->>ARC: { channel, client, pluginLogger }
  ARC->>LO: DefaultLogger(pluginLogger, pluginAPI)
  ARC->>LO: DefaultCoreSDK(channel, client, pluginAPI, logger: DefaultLogger)
  ARC->>LO: create PublicDefaultRealtimeObjects(proxied: InternalDefaultRealtimeObjects, coreSDK, logger)
  LO-->>App: RealtimeObjects
  note over LO,API: Logging via DefaultLogger.log(...) -> pluginAPI.log(...)
Loading
sequenceDiagram
  autonumber
  participant INT as DefaultInternalPlugin
  participant API as _AblyPluginSupportPrivate.PluginAPI
  participant CH as _AblyPluginSupportPrivate.RealtimeChannel
  participant CL as _AblyPluginSupportPrivate.RealtimeClient

  INT->>API: prepare(CH, CL)
  API-->>INT: Decoding/Encoding formats, logger
  INT->>INT: encodeObjectMessage(..., format: EncodingFormat)
  INT->>API: sendObject(messages, channel: CH, client: CL)
  API-->>INT: ack / error (PublicErrorInfo)
  INT->>INT: map error -> ARTErrorInfo -> InternalError
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60–90 minutes

Assessment against linked issues

Objective Addressed Explanation
ECO-5531: Prevent AblyPlugin from being pulled in by default by Xcode by moving/renaming to a private support package and updating imports/usages

Possibly related PRs

Suggested reviewers

  • maratal
  • umair-ably
  • VeskeR

Poem

A rabbit taps keys with a whisker’s delight,
Swapping plugins in code under moonlit night.
New logger burrows, errors hop right,
Wires re-threaded, messages take flight—
Carrots of green checks, reviews in sight. 🥕✨

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ECO-5531-move-plugin-API-to-separate-repo

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot temporarily deployed to staging/pull/69/AblyLiveObjects August 19, 2025 20:48 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the ECO-5531-move-plugin-API-to-separate-repo branch from 6a534e5 to e40ed95 Compare August 20, 2025 15:21
@github-actions github-actions bot temporarily deployed to staging/pull/69/AblyLiveObjects August 20, 2025 15:22 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the ECO-5531-move-plugin-API-to-separate-repo branch from e40ed95 to 398c96c Compare August 20, 2025 16:45
@github-actions github-actions bot temporarily deployed to staging/pull/69/AblyLiveObjects August 20, 2025 16:46 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the ECO-5531-move-plugin-API-to-separate-repo branch 2 times, most recently from 75495fb to 1acd770 Compare August 20, 2025 18:12
@lawrence-forooghian lawrence-forooghian changed the title [WIP, ECO-5531] Move plugin API to a separate repo [WIP, ECO-5531] Get PluginAPI from an external repo Aug 20, 2025
@github-actions github-actions bot temporarily deployed to staging/pull/69/AblyLiveObjects August 20, 2025 18:18 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the ECO-5531-move-plugin-API-to-separate-repo branch from 1acd770 to bb421ba Compare August 20, 2025 18:20
@lawrence-forooghian
Copy link
Collaborator Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Aug 20, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@github-actions github-actions bot temporarily deployed to staging/pull/69/AblyLiveObjects August 20, 2025 18:22 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (8)
.cursor/rules/testing.mdc (1)

3-5: Rule not applied anywhere: empty globs disables this guideline.

With alwaysApply: false and no patterns under globs, this rule set won’t match any files, so none of the guidance is enforced.

Apply this diff to scope the rule to test sources:

-globs: 
+globs:
+  - "**/Tests/**/*.swift"
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (1)

106-118: Fix missing self in the garbage-collection task.

Inside the Task closure, performGarbageCollection() requires explicit self.. As written, this won’t compile.

Apply:

-                    performGarbageCollection()
+                    self.performGarbageCollection()
Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift (2)

265-266: Fix NSNumber casts: as NSNumber is invalid for Ints

Casting an Int to NSNumber using as won’t compile reliably. Use NSNumber(value:) instead.

Apply:

-        result[WireKey.action.rawValue] = .number(action.rawValue as NSNumber)
+        result[WireKey.action.rawValue] = .number(NSNumber(value: action.rawValue))
-            WireKey.semantics.rawValue: .number(semantics.rawValue as NSNumber),
+            WireKey.semantics.rawValue: .number(NSNumber(value: semantics.rawValue)),

Also applies to: 398-399


117-118: Encode timestamps as integer milliseconds (use Int) — fix NSNumber to avoid type-sensitive mismatches

Tests and other code expect integer millisecond timestamps; currently code emits Double ms which can break type-sensitive equality/serialization. Change timestamp serialisation to Int milliseconds.

Files to update:

  • Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift
    • Replace timestamp encoding:
      -            result[WireObjectMessageWireKey.timestamp.rawValue] = .number(NSNumber(value: (timestamp.timeIntervalSince1970) * 1000))
      +            result[WireObjectMessageWireKey.timestamp.rawValue] = .number(NSNumber(value: Int(timestamp.timeIntervalSince1970 * 1000)))
    • Replace serialTimestamp encoding:
      -            result[WireObjectMessageWireKey.serialTimestamp.rawValue] = .number(NSNumber(value: serialTimestamp.timeIntervalSince1970 * 1000))
      +            result[WireObjectMessageWireKey.serialTimestamp.rawValue] = .number(NSNumber(value: Int(serialTimestamp.timeIntervalSince1970 * 1000)))
  • Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift (other toWire implementation)
    • Replace serialTimestamp encoding:
      -            result[WireKey.serialTimestamp.rawValue] = .number(NSNumber(value: serialTimestamp.timeIntervalSince1970 * 1000))
      +            result[WireKey.serialTimestamp.rawValue] = .number(NSNumber(value: Int(serialTimestamp.timeIntervalSince1970 * 1000)))

Note: ObjectCreationHelpers already uses Int for object-ID timestamps (consistent with these changes) and tests (Tests/AblyLiveObjectsTests/WireObjectMessageTests.swift) assert integer ms — adjust the above lines to match the test expectations.

Sources/AblyLiveObjects/Internal/ObjectsPool.swift (4)

13-20: Computed properties are missing return statements (will not compile)

The switch statements in mapValue and counterValue evaluate expressions but never return them.

Apply this diff:

 internal var mapValue: InternalDefaultLiveMap? {
   switch self {
   case let .map(map):
-    map
+    return map
   case .counter:
-    nil
+    return nil
   }
 }

 internal var counterValue: InternalDefaultLiveCounter? {
   switch self {
   case .map:
-    nil
+    return nil
   case let .counter(counter):
-    counter
+    return counter
   }
 }

Also applies to: 23-30


82-106: replaceData(using:) does not return a value (missing return in cases)

The function promises DeferredUpdate but neither branch returns a value.

Apply this diff:

 fileprivate func replaceData(
   using state: ObjectState,
   objectMessageSerialTimestamp: Date?,
   objectsPool: inout ObjectsPool,
 ) -> DeferredUpdate {
   switch self {
   case let .map(map):
-    .map(
+    return .map(
       map,
       map.replaceData(
         using: state,
         objectMessageSerialTimestamp: objectMessageSerialTimestamp,
         objectsPool: &objectsPool,
       ),
     )
   case let .counter(counter):
-    .counter(
+    return .counter(
       counter,
       counter.replaceData(
         using: state,
         objectMessageSerialTimestamp: objectMessageSerialTimestamp,
       ),
     )
   }
 }

109-116: Computed properties isTombstone and tombstonedAt also miss return statements

Same issue as above; both properties will fail to compile.

Apply this diff:

 internal var isTombstone: Bool {
   switch self {
   case let .counter(counter):
-    counter.isTombstone
+    return counter.isTombstone
   case let .map(map):
-    map.isTombstone
+    return map.isTombstone
   }
 }

 internal var tombstonedAt: Date? {
   switch self {
   case let .counter(counter):
-    counter.tombstonedAt
+    return counter.tombstonedAt
   case let .map(map):
-    map.tombstonedAt
+    return map.tombstonedAt
   }
 }

Also applies to: 118-125


291-296: Bug: Set subtraction uses unsupported + operator between Set and Array

receivedObjectIds + [Self.rootKey] is invalid; use Set.union instead.

Apply this diff:

-        let objectIdsToRemove = Set(entries.keys).subtracting(receivedObjectIds + [Self.rootKey])
+        let objectIdsToRemove = Set(entries.keys).subtracting(receivedObjectIds.union([Self.rootKey]))
🧹 Nitpick comments (29)
Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift (1)

217-222: Nonce generation: avoid force-unwrapping and prefer binary randomness with base64url encoding

Current approach is fine, but we can make it more robust and unambiguous by generating 128 bits of randomness and base64url-encoding it, while also avoiding !.

Apply:

-    private static func generateNonce() -> String {
-        // TODO: confirm if there's any specific rules here: https://github.com/ably/specification/pull/353/files#r2228252389
-        let letters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789"
-        return String((0 ..< 16).map { _ in letters.randomElement()! })
-    }
+    private static func generateNonce() -> String {
+        // 128-bit random nonce, Base64URL (no padding). Uses SystemRandomNumberGenerator (cryptographically secure).
+        var rng = SystemRandomNumberGenerator()
+        let bytes = (0..<16).map { _ in UInt8.random(in: .min ... .max, using: &rng) }
+        return Data(bytes).base64EncodedString()
+            .replacingOccurrences(of: "+", with: "-")
+            .replacingOccurrences(of: "/", with: "_")
+            .replacingOccurrences(of: "=", with: "")
+    }
Sources/AblyLiveObjects/Internal/LiveObjectMutableState.swift (2)

47-55: Improve log context for invalid serial values

Including objectID helps correlate logs to a specific object instance when diagnosing message ordering issues.

-            logger.log("Object operation message has invalid serial values: serial=\(objectMessageSerial ?? "nil"), siteCode=\(objectMessageSiteCode ?? "nil")", level: .warn)
+            logger.log("Object operation message has invalid serial values for objectID=\(objectID): serial=\(objectMessageSerial ?? "nil"), siteCode=\(objectMessageSiteCode ?? "nil")", level: .warn)

118-131: Potential thread-safety hazard around subscriptions storage

subscriptionsByID is mutated from subscribe/unsubscribe and read during emit. If these can be invoked from different queues/threads, you risk races or “mutation while enumerating.” Define single-thread/actor ownership for this state or guard access with a lock.

Can you confirm all accesses to LiveObjectMutableState (subscribe/unsubscribe/emit) are serialized on the same queue/actor via coreSDK or updateLiveObject? If not, I can sketch an actor-backed refactor for the subscriptions map.

Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultLiveCounter.swift (1)

13-19: Logger is stored but unused; prefix to make intent clear (and appease linters)

If the logger is intentionally kept for future use, prefixing avoids “unused” warnings and clarifies intent.

-    private let logger: Logger
+    private let _logger: Logger
@@
-    internal init(proxied: InternalDefaultLiveCounter, coreSDK: CoreSDK, logger: Logger) {
+    internal init(proxied: InternalDefaultLiveCounter, coreSDK: CoreSDK, logger: Logger) {
         self.proxied = proxied
         self.coreSDK = coreSDK
-        self.logger = logger
+        self._logger = logger
     }
.cursor/rules/swift.mdc (1)

17-17: Import guidance switched to _AblyPluginSupportPrivate — LGTM

This aligns with the PR’s objective to source the Plugin API from a separate repo and keep plugin internals private to library code.

  • Verified tests do not use internal import _AblyPluginSupportPrivate (via rg -g 'Tests/**' '^\s*internal\s+import\s+_AblyPluginSupportPrivate\b').
  • Please update .cursor/rules/testing.mdc to specify that tests should use a normal import for the private module, e.g.:
In .cursor/rules/testing.mdc:
- (no entry for _AblyPluginSupportPrivate)
+ `_AblyPluginSupportPrivate`: use `import _AblyPluginSupportPrivate`
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)

4-12: Pin ably-cocoa-plugin-support to a version or revision, not a branch

The manifest currently uses a branch spec (.branch("5282886")), which:

  • Is volatile—branches can be force-updated or deleted.
  • Doesn’t actually exist upstream (git ls-remote returned zero hits).

Please update the .package entry for ably-cocoa-plugin-support in your Package.swift to use either:

  • An exact semver tag, e.g.
    .package(
      url: "https://github.com/ably/ably-cocoa-plugin-support",
      .exact("1.2.3") // replace with the latest published tag
    )
  • Or a commit SHA, e.g.
    .package(
      url: "https://github.com/ably/ably-cocoa-plugin-support",
      .revision("5282886070764bf9d3d5fe72f9b5fd891629e68f")
    )

Afterwards, regenerate your Package.resolved so it records "version" or a bare "revision" instead of a "branch".

Package.resolved (1)

4-12: Pin plugin-support to a stable ref (commit or tag)

Your Package.swift is currently using a branch name as the revision:

• In Package.swift (around lines 24–30):

- .package(
-     // TODO: Unpin this before release
-     url: "https://github.com/ably/ably-cocoa-plugin-support",
-     revision: "5282886",
- ),
+ .package(
+     url: "https://github.com/ably/ably-cocoa-plugin-support",
+     revision: "5282886070764bf9d3d5fe72f9b5fd891629e68f",
+ ),

Recommendation:

  • Use the full commit SHA (revision: "<full-sha>") or, if available, a released tag/version (from: "x.y.z" or exact: "x.y.z").
  • This ensures deterministic and reproducible builds across machines and CI.
Sources/AblyLiveObjects/Utility/Errors.swift (1)

9-9: Case payload updated to private RealtimeChannelState — check error message readability

Interpolating the new RealtimeChannelState directly into messages assumes it yields human-friendly strings. If it doesn’t conform with a good description, consider providing a local extension to map to stable strings used in error messages/logs.

Example extension (place within this module if needed):

extension _AblyPluginSupportPrivate.RealtimeChannelState: CustomStringConvertible {
    public var description: String {
        switch self {
        case .initialized: "initialized"
        case .attaching: "attaching"
        case .attached: "attached"
        case .detaching: "detaching"
        case .detached: "detached"
        case .suspended: "suspended"
        case .failed: "failed"
        @unknown default: "unknown"
        }
    }
}
Sources/AblyLiveObjects/Internal/ARTClientOptions+Objects.swift (2)

18-21: Bridging via defaultPluginAPI + asPluginPublicClientOptions — check key collisions

The get-path looks fine. Minor nit: consider namespacing Self.garbageCollectionOptionsKey more explicitly (e.g., "AblyLiveObjects.Objects.garbageCollectionOptions") to avoid accidental collisions with other plugins that might share the options store.


39-43: Consider supporting “unset” for tests/dev tooling

Right now, attempting to set nil traps with preconditionFailure. If the plugin API supports clearing a key (e.g., setting nil or a dedicated remove call), consider handling nil to make toggling this option in tests less brittle.

Sources/AblyLiveObjects/Public/Plugin.swift (1)

25-27: Add an explicit type annotation to defaultPluginAPI

Being explicit helps guard against accidental API shape changes and improves readability at call sites.

Apply this diff:

-    internal static let defaultPluginAPI = _AblyPluginSupportPrivate.DependencyStore.sharedInstance().fetchPluginAPI()
+    internal static let defaultPluginAPI: _AblyPluginSupportPrivate.PluginAPIProtocol =
+        _AblyPluginSupportPrivate.DependencyStore.sharedInstance().fetchPluginAPI()
Sources/AblyLiveObjects/Utility/Logger.swift (1)

32-41: Qualify LogLevel type to avoid ambiguity

For consistency with the rest of the file and to avoid name collisions, qualify LogLevel in the method signature.

Apply this diff:

-    internal func log(_ message: String, level: LogLevel, codeLocation: CodeLocation) {
+    internal func log(_ message: String, level: _AblyPluginSupportPrivate.LogLevel, codeLocation: CodeLocation) {
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (3)

372-377: Nit: Logging string typo (missing closing parenthesis).

For consistency/readability in logs, close the parenthesis.

-            logger.log("onChannelAttached(hasObjects: \(hasObjects)", level: .debug)
+            logger.log("onChannelAttached(hasObjects: \(hasObjects))", level: .debug)

350-356: Docstring references a non-existent stream name.

The comment lists testsOnly_receivedObjectStateProtocolMessages, but the actual property is testsOnly_receivedObjectSyncProtocolMessages.

-    /// - testsOnly_receivedObjectStateProtocolMessages
+    /// - testsOnly_receivedObjectSyncProtocolMessages

281-289: Yielding AsyncStream events while holding the mutex can increase lock hold time and risk re-entrancy.

yield is typically lightweight, but resumption of awaiting tasks can introduce unexpected re-entrancy while the lock is held. Consider capturing messages under lock, releasing the lock, and then yielding.

Also applies to: 296-307, 502-503

Sources/AblyLiveObjects/Utility/MarkerProtocolHelpers.swift (1)

4-11: Doc wording: this is a downcast, not an upcast.

The function casts from a marker protocol value (Any/protocol existential) to the concrete public type T. Adjust docstring to avoid confusion.

-/// Upcasts an instance of an `_AblyPluginSupportPrivate` marker protocol to the concrete type that this marker protocol represents.
+/// Casts an instance of an `_AblyPluginSupportPrivate` marker protocol value to the concrete type it represents.
Tests/AblyLiveObjectsTests/ObjectMessageTests.swift (1)

55-55: Remove redundant CFNumberGetType call.

This standalone call has no effect on assertions and can be removed to reduce noise.

-                    CFNumberGetType(testNumber)
Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsHelper.swift (1)

334-336: Avoid force-unwrapping the encoded ProtocolMessage

If encoding fails, tests will crash. Prefer a guarded failure that surfaces a clearer error.

Apply:

-                let protocolMessage = withExtendedLifetime(jsonLikeEncoderDelegate) {
-                    encoder.protocolMessage(from: foundationObject)!
-                }
+                guard let protocolMessage = withExtendedLifetime(jsonLikeEncoderDelegate) {
+                    encoder.protocolMessage(from: foundationObject)
+                } else {
+                    preconditionFailure("Failed to encode ProtocolMessage from plugin-support foundation object")
+                }
Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift (1)

66-81: Minor: prefer self. when assigning to shadowed properties for clarity

Assignments like id = ... and timestamp = ... rely on property resolution. Using self.id = ... and self.timestamp = ... avoids ambiguity near if let bindings with the same names.

Example:

-        } else if let parentID = decodingContext.parentID {
-            id = "\(parentID):\(decodingContext.indexInParent)"
+        } else if let parentID = decodingContext.parentID {
+            self.id = "\(parentID):\(decodingContext.indexInParent)"
         }
...
-        } else if let parentTimestamp = decodingContext.parentTimestamp {
-            timestamp = parentTimestamp
+        } else if let parentTimestamp = decodingContext.parentTimestamp {
+            self.timestamp = parentTimestamp
         }

Also applies to: 91-97

Sources/AblyLiveObjects/Public/ARTRealtimeChannel+Objects.swift (1)

18-23: Nit: reuse the local pluginAPI for consistency

Use the pluginAPI variable instead of calling Plugin.defaultPluginAPI again. This avoids potential mismatches if default resolution ever changes and improves readability.

Apply this diff:

         let coreSDK = DefaultCoreSDK(
             channel: underlyingObjects.channel,
             client: underlyingObjects.client,
-            pluginAPI: Plugin.defaultPluginAPI,
+            pluginAPI: pluginAPI,
             logger: logger,
         )
Tests/AblyLiveObjectsTests/Helpers/TestLogger.swift (1)

6-6: Nit: Drop NSObject inheritance unless needed

If there’s no ObjC interop requirement in tests, you can remove NSObject to simplify the type.

-final class TestLogger: NSObject, AblyLiveObjects.Logger {
+final class TestLogger: AblyLiveObjects.Logger {
Sources/AblyLiveObjects/Utility/WireValue.swift (2)

4-4: Fix grammar in doc comment.

Minor nit: “can be represents” → “can represent” for clarity.

Apply this diff:

-/// A wire value that can be represents the kinds of data that we expect to find inside a deserialized wire object received from `_AblyPluginSupportPrivate`, or which we may put inside a serialized wire object that we send to `_AblyPluginSupportPrivate`.
+/// A wire value that can represent the kinds of data that we expect to find inside a deserialized wire object received from `_AblyPluginSupportPrivate`, or which we may put inside a serialized wire object that we send to `_AblyPluginSupportPrivate`.

139-147: Add a descriptive precondition failure message.

Guard’s preconditionFailure() has no message; adding context eases debugging when assumptions are violated.

Apply this diff:

-        guard case let .object(wireObject) = wireValue else {
-            preconditionFailure()
-        }
+        guard case let .object(wireObject) = wireValue else {
+            preconditionFailure("objectFromPluginSupportData(_:): expected top-level .object, got \(wireValue)")
+        }
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicObjectsStore.swift (1)

1-1: Consider removing unused import.

It looks like _AblyPluginSupportPrivate isn’t referenced in this file; if truly unused, drop the import to reduce coupling.

Apply this diff if nothing in this file (directly or via typealiases) depends on it:

-internal import _AblyPluginSupportPrivate
Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)

198-206: Normalize typeString to String to avoid Substring matching pitfalls

Substring vs String matching in switch is usually fine, but normalizing avoids surprises and clarifies intent.

Apply this diff:

-        let components = objectID.split(separator: ":")
-        guard let typeString = components.first else {
+        let components = objectID.split(separator: ":")
+        guard let typeString = components.first.map(String.init) else {
             return nil
         }

206-214: Optional: log unsupported object type on zero-value creation

Returning nil is fine, but a warning helps diagnose malformed IDs.

If helpful, add:

         switch typeString {
         case "map":
             entry = .map(.createZeroValued(objectID: objectID, logger: logger, userCallbackQueue: userCallbackQueue, clock: clock))
         case "counter":
             entry = .counter(.createZeroValued(objectID: objectID, logger: logger, userCallbackQueue: userCallbackQueue, clock: clock))
         default:
-            return nil
+            logger.log("Unsupported object type prefix '\(typeString)' in objectId '\(objectID)'", level: .warn)
+            return nil
         }
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (1)

136-161: Async send bridge is sound; minor stylistic alternative

The continuation + Result pattern is correct and preserves typed throws. As an alternative, withCheckedThrowingContinuation could be used with explicit mapping to InternalError to reduce the extra Result hop, but current code is fine.

Example alternative (no change required):

try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Void, Error>) in
  let internalQueue = pluginAPI.internalQueue(for: client)
  internalQueue.async {
    pluginAPI.sendObject(withObjectMessages: objectMessageBoxes, channel: channel) { error in
      if let error { continuation.resume(throwing: error.toInternalError()) }
      else { continuation.resume(returning: ()) }
    }
  }
}
Sources/AblyLiveObjects/Protocol/ObjectMessage.swift (1)

292-299: Open TODOs around JSON payload spec

There are outstanding TODOs around ObjectData.json handling. Consider tracking with an issue link to avoid losing context.

I can open a follow-up issue summarizing the current behavior and the spec gaps if helpful.

Also applies to: 343-346

Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)

401-404: Unify parameter ordering: prefer (logger, userCallbackQueue, clock) across MutableState APIs

Most methods that thread dependencies use the order (logger, userCallbackQueue, clock). replaceData currently uses (logger, clock, userCallbackQueue), which is inconsistent and increases cognitive load.

Apply these diffs to standardize ordering:

Change the signature order:

-        internal mutating func replaceData(
+        internal mutating func replaceData(
             using state: ObjectState,
             objectMessageSerialTimestamp: Date?,
             objectsPool: inout ObjectsPool,
-            logger: Logger,
-            clock: SimpleClock,
-            userCallbackQueue: DispatchQueue,
+            logger: Logger,
+            userCallbackQueue: DispatchQueue,
+            clock: SimpleClock,
         ) -> LiveObjectUpdate<DefaultLiveMapUpdate> {

And (optionally) reorder the call to match:

-            mutableState.replaceData(
+            mutableState.replaceData(
                 using: state,
                 objectMessageSerialTimestamp: objectMessageSerialTimestamp,
                 objectsPool: &objectsPool,
-                logger: logger,
-                clock: clock,
-                userCallbackQueue: userCallbackQueue,
+                logger: logger,
+                userCallbackQueue: userCallbackQueue,
+                clock: clock,
             )

Also applies to: 253-261

As required by the accompanying ably-cocoa submodule bump.
@lawrence-forooghian lawrence-forooghian force-pushed the ECO-5531-move-plugin-API-to-separate-repo branch from bb421ba to d5f011e Compare August 20, 2025 19:34
@github-actions github-actions bot temporarily deployed to staging/pull/69/AblyLiveObjects August 20, 2025 19:35 Inactive
@lawrence-forooghian lawrence-forooghian force-pushed the ECO-5531-move-plugin-API-to-separate-repo branch from d5f011e to c614aed Compare August 20, 2025 19:47
@github-actions github-actions bot temporarily deployed to staging/pull/69/AblyLiveObjects August 20, 2025 19:48 Inactive
@lawrence-forooghian lawrence-forooghian changed the title [WIP, ECO-5531] Get PluginAPI from an external repo [ECO-5531] Get PluginAPI from an external repo Aug 20, 2025
As required by the accompanying ably-cocoa submodule bump (see the
ably-cocoa commit message for explanation of why we're making this
change).
@lawrence-forooghian lawrence-forooghian force-pushed the ECO-5531-move-plugin-API-to-separate-repo branch from c614aed to b4e1899 Compare August 20, 2025 19:59
@lawrence-forooghian lawrence-forooghian changed the title [ECO-5531] Get PluginAPI from an external repo [ECO-5531] Get ably-cocoa plugin APIs from an external repo Aug 20, 2025
Base automatically changed from 65-LiveMapValue-improvements to main August 21, 2025 12:30
This was referenced Aug 21, 2025
@lawrence-forooghian lawrence-forooghian merged commit 604a89e into main Aug 22, 2025
18 checks passed
@lawrence-forooghian lawrence-forooghian deleted the ECO-5531-move-plugin-API-to-separate-repo branch August 22, 2025 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants