Remove InternalError #87
Conversation
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/87/AblyLiveObjects
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (3)
Sources/AblyLiveObjects/Utility/Errors.swift (1)
52-59: Wrap LiveObjectsError in NSObject for reliable Obj-C bridging
ARTErrorInfo.userInfo is bridged to NSDictionary; Swift enums don’t conform to AnyObject and won’t cross the bridge. ReplaceadditionalUserInfo: [liveObjectsErrorUserInfoKey: self]with
additionalUserInfo: [liveObjectsErrorUserInfoKey: LiveObjectsErrorBox(self)]add
private final class LiveObjectsErrorBox: NSObject { let value: LiveObjectsError init(_ value: LiveObjectsError) { self.value = value } } private let liveObjectsErrorUserInfoKey = "com.ably.liveobjects.LiveObjectsError"and update your unwrapping in
testsOnly_underlyingLiveObjectsErrorto cast toLiveObjectsErrorBoxand returnboxed.value.Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift (2)
2842-2856: Missingasyncin override closure; preserve ARTErrorInfo on rethrowThe override expects an
async throws(ARTErrorInfo)closure, butasyncis missing. Also, rethrowARTErrorInfoas-is to avoid losing details.- internallyTypedObjects.testsOnly_overridePublish(with: { objectMessages throws(ARTErrorInfo) in + internallyTypedObjects.testsOnly_overridePublish(with: { objectMessages async throws(ARTErrorInfo) in @@ - } catch { - throw LiveObjectsError.other(error).toARTErrorInfo() - } + } catch let e as ARTErrorInfo { + throw e + } catch { + throw LiveObjectsError.other(error).toARTErrorInfo() + }
3088-3113: Same here: addasyncand rethrow ARTErrorInfo unchangedMirror the fix from the earlier scenario.
- internallyTypedObjects.testsOnly_overridePublish(with: { objectMessages throws(ARTErrorInfo) in + internallyTypedObjects.testsOnly_overridePublish(with: { objectMessages async throws(ARTErrorInfo) in @@ - } catch { - throw LiveObjectsError.other(error).toARTErrorInfo() - } + } catch let e as ARTErrorInfo { + throw e + } catch { + throw LiveObjectsError.other(error).toARTErrorInfo() + }
🧹 Nitpick comments (15)
Sources/AblyLiveObjects/Utility/WireValue.swift (2)
238-243: Typed throws switch to ARTErrorInfo looks good; update the doc comment.The implementation correctly throws ARTErrorInfo. The doc still says it throws ConversionError; adjust to reflect ARTErrorInfo (and optionally mention the underlying LiveObjectsError for tests).
Apply:
- /// - Throws: `ConversionError.dataCannotBeConvertedToJSONValue` if `WireValue` represents binary data. + /// - Throws: `ARTErrorInfo` if `WireValue` represents binary data (underlying: `WireValue.ConversionError.dataCannotBeConvertedToJSONValue`).
238-243: Consider a dedicated LiveObjectsError case for this known conversion failure.Using
.otherloses specificity. A dedicated case (e.g.,dataCannotBeConvertedToJSONValue) would allow stable codes/messages without relying on stringification of an underlying Swift error.Sources/AblyLiveObjects/Utility/Errors.swift (2)
12-12: Defaulting.otherto 400/Bad Request—double-check semantics.Some
.othersources may indicate internal failures rather than client mistakes. If feasible, map specific known error types (e.g., DecodingError => 400) and reserve 500 for unexpected runtime failures.Also applies to: 22-24, 28-35, 47-49
126-129: Key namespacing nit.Use a reverse-DNS namespaced key to reduce collision risk with upstream keys.
Sources/AblyLiveObjects/Utility/Data+Extensions.swift (1)
15-16: Consider sanitizing error payload to avoid logging large/opaque base64 strings.Truncate the string before embedding it in the error to reduce log noise and potential leakage.
Apply:
- throw DecodingError.invalidBase64String(base64String).toARTErrorInfo() + let truncated = base64String.count > 256 ? String(base64String.prefix(256)) + "…" : base64String + throw DecodingError.invalidBase64String(truncated).toARTErrorInfo()Tests/AblyLiveObjectsTests/ObjectMessageTests.swift (1)
282-285: Tests updated to expect ARTErrorInfo — aligns with migration.Optionally, mirror SyncCursorTests by asserting the underlying LiveObjectsError in one representative case to lock in the mapping.
Also applies to: 305-308, 379-382, 406-409, 429-432
Tests/AblyLiveObjectsTests/SyncCursorTests.swift (1)
46-47: Good use of testsOnly_underlyingLiveObjectsError to assert the root cause.Optional: capture the error via #require(throws:) to avoid manual do/catch.
Example:
let e = try #require(throws: ARTErrorInfo.self) { _ = try SyncCursor(channelSerial: channelSerial) } let underlying = try #require(e.testsOnly_underlyingLiveObjectsError) guard case .other(SyncCursor.Error.channelSerialDoesNotMatchExpectedFormat) = underlying else { Issue.record("Expected channelSerialDoesNotMatchExpectedFormat error"); return }Also applies to: 65-66
Sources/AblyLiveObjects/Utility/JSONValue.swift (1)
272-283: Good error mapping; consider richer ARTErrorInfo detailsMapping JSON parsing failures to
LiveObjectsError.other(...).toARTErrorInfo()is fine. Optionally include a specific error code/message for invalid JSON to aid debugging.Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (2)
143-144: Use fully qualified type for PluginAPIProtocol for consistency
Elsewhere in this file you use_AblyPluginSupportPrivate.PluginAPIProtocol. Align the signature to avoid ambiguous references.- pluginAPI: PluginAPIProtocol, + pluginAPI: _AblyPluginSupportPrivate.PluginAPIProtocol,
146-165: Optional: simplify to withCheckedThrowingContinuation
Removes the intermediateResultand uses typed throwing directly.- try await withCheckedContinuation { (continuation: CheckedContinuation<Result<Void, ARTErrorInfo>, Never>) in + try await withCheckedThrowingContinuation { continuation in let internalQueue = pluginAPI.internalQueue(for: client) internalQueue.async { pluginAPI.nosync_sendObject( withObjectMessages: objectMessageBoxes, channel: channel, ) { error in // We don't currently rely on this documented behaviour of `nosync_sendObject` but we may do later, so assert it to be sure it's happening. dispatchPrecondition(condition: .onQueue(internalQueue)) if let error { - continuation.resume(returning: .failure(ARTErrorInfo.castPluginPublicErrorInfo(error))) + continuation.resume(throwing: ARTErrorInfo.castPluginPublicErrorInfo(error)) } else { - continuation.resume(returning: .success(())) + continuation.resume(returning: ()) } } } - }.get() + }Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (2)
153-174: Good: shrink critical section; build message under lock, publish outside
This reduces contention and avoids holding the mutex across I/O.Optional: re-validate channel state just before publish
Between validation (inside the lock) andpublishthere’s TOCTOU exposure; consider a second lightweight validation right before publishing.let objectMessage = try mutableStateMutex.withLock { mutableState throws(ARTErrorInfo) in // RTLM20c try coreSDK.nosync_validateChannelState(notIn: [.detached, .failed, .suspended], operationDescription: "LiveMap.set") return OutboundObjectMessage( /* … */ ) } + // Re-check right before I/O to prevent TOCTOU issues. + try coreSDK.nosync_validateChannelState(notIn: [.detached, .failed, .suspended], operationDescription: "LiveMap.set") try await coreSDK.publish(objectMessages: [objectMessage])
177-197: Same note for remove: optional re-validate before publish
Keeps behavior predictable if state changes between lock release and publish.let objectMessage = try mutableStateMutex.withLock { mutableState throws(ARTErrorInfo) in // RTLM21c try coreSDK.nosync_validateChannelState(notIn: [.detached, .failed, .suspended], operationDescription: "LiveMap.remove") return OutboundObjectMessage( /* … */ ) } + try coreSDK.nosync_validateChannelState(notIn: [.detached, .failed, .suspended], operationDescription: "LiveMap.remove") // RTLM21f try await coreSDK.publish(objectMessages: [objectMessage])Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
24-26: Optional: make the mock’s override usable.Implementing this to set the handler improves test ergonomics.
- func testsOnly_overridePublish(with _: @escaping ([OutboundObjectMessage]) async throws(ARTErrorInfo) -> Void) { - protocolRequirementNotImplemented() - } + func testsOnly_overridePublish(with newImplementation: @escaping ([OutboundObjectMessage]) async throws(ARTErrorInfo) -> Void) { + setPublishHandler(newImplementation) + }Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
50-66: Prefer surfacing unknown errors as ARTErrorInfo instead of crashing.Relying on
preconditionFailurewill crash release builds if a non-ARTErrorInfo escapes. Consider mapping unknown errors to a genericARTErrorInfo.- do { - try await overriddenImplementation(objectMessages) - } catch { - guard let artErrorInfo = error as? ARTErrorInfo else { - preconditionFailure("Expected ARTErrorInfo, got \(error)") - } - throw artErrorInfo - } + do { + try await overriddenImplementation(objectMessages) + } catch let artErrorInfo as ARTErrorInfo { + throw artErrorInfo + } catch { + // Be forgiving in tests; preserve typed-throws contract without crashing. + throw LiveObjectsError.other(error).toARTErrorInfo() + }Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (1)
209-213: Optional: avoid taking the mutex just to validate channel state.
nosync_validateChannelStatereads fromCoreSDK; locking here doesn’t protect local mutable state. You can drop the lock to cut contention.- try mutableStateMutex.withLock { _ throws(ARTErrorInfo) in - try coreSDK.nosync_validateChannelState(notIn: [.detached, .failed, .suspended], operationDescription: "RealtimeObjects.createCounter") - } + try coreSDK.nosync_validateChannelState(notIn: [.detached, .failed, .suspended], operationDescription: "RealtimeObjects.createCounter")
📜 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)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/AblyLiveObjects/Internal/CoreSDK.swift(5 hunks)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift(3 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift(1 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift(1 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(3 hunks)Sources/AblyLiveObjects/Protocol/ObjectMessage.swift(9 hunks)Sources/AblyLiveObjects/Protocol/SyncCursor.swift(3 hunks)Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift(13 hunks)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift(2 hunks)Sources/AblyLiveObjects/Utility/Data+Extensions.swift(1 hunks)Sources/AblyLiveObjects/Utility/Errors.swift(5 hunks)Sources/AblyLiveObjects/Utility/InternalError.swift(0 hunks)Sources/AblyLiveObjects/Utility/JSONValue.swift(2 hunks)Sources/AblyLiveObjects/Utility/WireCodable.swift(28 hunks)Sources/AblyLiveObjects/Utility/WireValue.swift(1 hunks)Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift(1 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift(1 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift(2 hunks)Tests/AblyLiveObjectsTests/InternalErrorTests.swift(0 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift(4 hunks)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift(2 hunks)Tests/AblyLiveObjectsTests/ObjectMessageTests.swift(6 hunks)Tests/AblyLiveObjectsTests/SyncCursorTests.swift(3 hunks)
💤 Files with no reviewable changes (2)
- Tests/AblyLiveObjectsTests/InternalErrorTests.swift
- Sources/AblyLiveObjects/Utility/InternalError.swift
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-26T11:32:26.789Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-08-26T11:32:26.789Z
Learning: Applies to Sources/AblyLiveObjects/**/*.swift : In AblyLiveObjects library (non-test) code, import modules as: Ably with `import Ably`, and _AblyPluginSupportPrivate with `internal import _AblyPluginSupportPrivate`
Applied to files:
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolvedTests/AblyLiveObjectsTests/SyncCursorTests.swiftTests/AblyLiveObjectsTests/AblyLiveObjectsTests.swiftSources/AblyLiveObjects/Protocol/ObjectMessage.swiftTests/AblyLiveObjectsTests/ObjectMessageTests.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Protocol/WireObjectMessage.swift
📚 Learning: 2025-08-26T11:32:54.222Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-08-26T11:32:54.222Z
Learning: Applies to Tests/**/*.swift : In tests, import AblyLiveObjects using `testable import AblyLiveObjects`
Applied to files:
Tests/AblyLiveObjectsTests/SyncCursorTests.swiftTests/AblyLiveObjectsTests/AblyLiveObjectsTests.swiftTests/AblyLiveObjectsTests/ObjectMessageTests.swiftTests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swiftSources/AblyLiveObjects/Protocol/WireObjectMessage.swift
📚 Learning: 2025-08-26T11:32:54.222Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-08-26T11:32:54.222Z
Learning: Applies to Tests/**/*.swift : In tests, import Ably using `import Ably`
Applied to files:
Tests/AblyLiveObjectsTests/SyncCursorTests.swiftTests/AblyLiveObjectsTests/ObjectMessageTests.swift
📚 Learning: 2025-08-26T11:32:54.222Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-08-26T11:32:54.222Z
Learning: Applies to Tests/**/*.swift : In tests, import `_AblyPluginSupportPrivate` using `import _AblyPluginSupportPrivate` (do not use `internal import`)
Applied to files:
Tests/AblyLiveObjectsTests/SyncCursorTests.swiftTests/AblyLiveObjectsTests/ObjectMessageTests.swift
📚 Learning: 2025-08-26T11:32:54.222Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/testing.mdc:0-0
Timestamp: 2025-08-26T11:32:54.222Z
Learning: Applies to Tests/**/*.swift : Do not use `fatalError` for expectation failures; prefer Swift Testing’s `#require`
Applied to files:
Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift
📚 Learning: 2025-08-26T11:32:26.789Z
Learnt from: CR
PR: ably/ably-liveobjects-swift-plugin#0
File: .cursor/rules/swift.mdc:0-0
Timestamp: 2025-08-26T11:32:26.789Z
Learning: Applies to **/*.swift : For JSONValue or WireValue, prefer using literal syntax via ExpressibleBy*Literal where possible
Applied to files:
Sources/AblyLiveObjects/Utility/WireValue.swiftSources/AblyLiveObjects/Utility/WireCodable.swift
⏰ 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). (7)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: SPM (Xcode 16.3)
- GitHub Check: Generate code coverage
🔇 Additional comments (45)
Package.swift (1)
24-24: Pin bump to ably-cocoa: OK—please confirm API compatibility with ARTErrorInfo changes.Matches the updated Package.resolved entries. Before merging, run a full build/test against this revision to catch any subtle ARTErrorInfo API differences introduced upstream. Also keep the “Unpin before next release” TODO tracked.
Package.resolved (1)
2-2: Resolved state aligns with Package.swift.originHash and ably-cocoa revision match the manifest. No concerns.
Also applies to: 9-10
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
2-2: Workspace resolution updated consistently.Pin matches root resolution and manifest. Good.
Also applies to: 9-10
Sources/AblyLiveObjects/Utility/Data+Extensions.swift (1)
12-14: Typed throws migration to ARTErrorInfo looks correct.Signature and docs align with the PR goal and import Ably is present.
Sources/AblyLiveObjects/Protocol/SyncCursor.swift (3)
15-16: Init now throws ARTErrorInfo; parsing and error mapping are consistent.The Scanner-based split and conversion via toARTErrorInfo() are sound; multiple-colon inputs will be preserved in cursorValue, which seems acceptable given the spec.
Also applies to: 24-25
1-1: Import Ably is necessary for typed throws and is appropriately added.
14-33: Audit complete: noInternalErrorortoInternalErrorreferences remain.All
throws(InternalError),toInternalError, and related test expectations have been removed, andtoARTErrorInfo()is used consistently.Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift (1)
112-115: Nice simplification: direct #require(throws: ARTErrorInfo.self).Removes unnecessary wrapping and matches the new typed throws surface.
Tests/AblyLiveObjectsTests/ObjectMessageTests.swift (1)
2-2: Import Ably added to reference ARTErrorInfo — correct.Tests/AblyLiveObjectsTests/SyncCursorTests.swift (1)
1-1: Import Ably added for ARTErrorInfo — correct.Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (2)
1493-1495: Consistent error type in remove() publish-failure pathGood alignment with ARTErrorInfo propagation throughout tests.
1412-1414: Switch to ARTErrorInfo for publish-failure injection looks correctAll publish handlers across tests now throw ARTErrorInfo and no InternalError usage remains.
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (1)
569-571: Typed-throws update to ARTErrorInfo is consistent with the migrationThe failure path now cleanly surfaces ARTErrorInfo; the assertion checks are appropriate.
Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
107-135: Great: build message under lock, publish outside; propagate ARTErrorInfo directly
- Validates channel state and amount, constructs OutboundObjectMessage under the mutex, then publishes after releasing it. This avoids holding the lock across I/O and removes the InternalError layer.
Sources/AblyLiveObjects/Utility/JSONValue.swift (1)
170-179: Typed throws migration looks correctThrowing
ARTErrorInfovia.toARTErrorInfo()is consistent with the PR goal.Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (2)
109-111: testsOnly_publish typed-throws update is consistentSignature aligns with repo-wide switch to
ARTErrorInfo.
122-124: testsOnly_overridePublish typed-throws update is consistentMatches call sites in tests once closures are marked
async.Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift (13)
2-2: Import Ably is requiredNeeded for
ARTErrorInfo; matches import conventions.
66-67: InboundWireObjectMessage decoding now uses typed throws — looks goodTyped-throws adoption and typed mapping for
extrasare consistent.Also applies to: 85-87
183-194: PartialWireObjectOperation typed-throws init — OK
238-253: WireObjectOperation typed-throws init — OK
292-304: WireObjectState typed-throws + precise wrong-type mapping — OKUsing
WireValueDecodingError...toARTErrorInfo()preserves context.
338-341: WireObjectsMapOp typed-throws init — OK
365-367: WireObjectsCounterOp typed-throws init — OK
387-395: WireObjectsMap typed-throws init with per-entry typed mapping — OK
419-421: WireObjectsCounter typed-throws init — OK
447-452: WireObjectsMapEntry typed-throws init — OK
493-500: WireObjectData typed-throws init — OK
540-548: StringOrData typed-throws init — OKClear failure path via
.toARTErrorInfo().
66-67: Typed‐throws support verified
Package.swift specifies tools version 6.1 and the Xcode project’s SWIFT_VERSION is 6.0, both of which support typed throws.Sources/AblyLiveObjects/Protocol/ObjectMessage.swift (3)
2-2: Imports follow house style
import Ablyalongsideinternal import _AblyPluginSupportPrivatematches the agreed convention.
251-256: Minor: ensure thrown codes/status are stable and spec-aligned
Data.fromBase64Throwingnow bubblesARTErrorInfo. Verify the error code/statusCode used here matches what downstream/public API expects for malformed Base64 in JSON.
98-114: Typed throws migration approved; Swift 6 toolchain confirmed
Throws annotations consistent; Package.swift declares tools version 6.1 and Example Xcode project targets Swift 6.0.Sources/AblyLiveObjects/Internal/CoreSDK.swift (4)
1-1: Import style matches project convention.Using
internal import _AblyPluginSupportPrivateis correct per repo guidance.
33-35: Good compatibility call on typed-throws function types.Keeping the stored override untyped avoids the macOS 15/iOS 18 runtime requirement.
78-82: Typed-to-untyped override storage LGTM.Capturing the typed-throws closure into an untyped storage is a practical workaround.
99-112: Channel-state validation helper LGTM.Consistent typed-throws usage and error shaping via
toARTErrorInfo().Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (5)
175-200: Great: creation under lock, publish outside.This removes await-under-lock risk and reduces contention while preserving correctness.
215-218: Input validation LGTM.Clear finiteness check with precise
ARTErrorInfoshaping.
221-227: Timestamp and creation op separation LGTM.The TODO to switch to server time is tracked; current placement is fine.
228-241: Post-publish object creation under lock LGTM.Applies state only after a successful publish; ordering is sound.
343-345: testsOnly_publish signature update LGTM.Consistent with CoreSDK’s typed-throws publish.
Sources/AblyLiveObjects/Utility/WireCodable.swift (3)
8-10: Consistent migration to ARTErrorInfo LGTM.Protocols and default impls now uniformly use typed-throws; error mapping via
toARTErrorInfo()is coherent.Also applies to: 36-45
70-88: Optional getters’ null-handling LGTM.Returning
nilon absent/null and throwing on wrong-type aligns with expected JSON semantics.Also applies to: 106-124, 142-160, 178-196, 214-232, 250-268
278-293: Helpers read well and are consistent.Date, RawRepresentable, WireEnum, and decodable accessors correctly propagate
ARTErrorInfo.Also applies to: 301-334, 338-358, 362-390
lawrence-forooghian
force-pushed
the
48-get-rid-of-InternalError
branch
from
64f1583 to
6a61f66
Compare
github-actions
bot
temporarily deployed
to
staging/pull/87/AblyLiveObjects
lawrence-forooghian
force-pushed
the
3-decide-how-threading-will-work
branch
2 times, most recently
from
a99fb43 to
b86e09a
Compare
It wasn't serving a useful purpose and was just another layer of indirection. Throw ARTErrorInfo directly inside the codebase and let it bubble up to the public API (but still keep the richer underlying errors for checking in tests). Resolves #48.
lawrence-forooghian
force-pushed
the
48-get-rid-of-InternalError
branch
from
6a61f66 to
5ba838b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift (2)
62-104: Make ACL explicit on initializer and avoid shadowing stored properties.
- The initializer lacks an explicit access level; SwiftLint explicit_acl will flag this.
- Local
if let id = …/connectionId/timestampshadow the stored properties; then you assign to the property in theelsebranch. It’s correct but easy to misread.Proposed fix (adds
internal, and uses distinct local names to remove shadowing):- init( + internal init( wireObject: [String: WireValue], decodingContext: _AblyPluginSupportPrivate.DecodingContextProtocol ) throws(ARTErrorInfo) { - if let id = try wireObject.optionalStringValueForKey(WireObjectMessageWireKey.id.rawValue) { - self.id = id + if let decodedID = try wireObject.optionalStringValueForKey(WireObjectMessageWireKey.id.rawValue) { + self.id = decodedID } else if let parentID = decodingContext.parentID { - id = "\(parentID):\(decodingContext.indexInParent)" + self.id = "\(parentID):\(decodingContext.indexInParent)" } … - if let connectionId = try wireObject.optionalStringValueForKey(WireObjectMessageWireKey.connectionId.rawValue) { - self.connectionId = connectionId + if let decodedConnectionID = try wireObject.optionalStringValueForKey(WireObjectMessageWireKey.connectionId.rawValue) { + self.connectionId = decodedConnectionID } else if let parentConnectionID = decodingContext.parentConnectionID { - connectionId = parentConnectionID + self.connectionId = parentConnectionID } … - if let timestamp = try wireObject.optionalAblyProtocolDateValueForKey(WireObjectMessageWireKey.timestamp.rawValue) { - self.timestamp = timestamp + if let decodedTimestamp = try wireObject.optionalAblyProtocolDateValueForKey(WireObjectMessageWireKey.timestamp.rawValue) { + self.timestamp = decodedTimestamp } else if let parentTimestamp = decodingContext.parentTimestamp { - timestamp = parentTimestamp + self.timestamp = parentTimestamp }
50-60: Explicit ACL missing on nested DecodingError.Add
internalto satisfy explicit_acl:- enum DecodingError: Error { + internal enum DecodingError: Error {
🧹 Nitpick comments (4)
Tests/AblyLiveObjectsTests/SyncCursorTests.swift (1)
46-48: Underlying LiveObjects error extraction viatestsOnly_underlyingLiveObjectsErroris appropriate.Pattern-matching
.other(SyncCursor.Error.channelSerialDoesNotMatchExpectedFormat)aligns with the new error plumbing.You could simplify using
#require(throws:)to capture the thrownARTErrorInfoand then inspecttestsOnly_underlyingLiveObjectsError, e.g.:let err = #require(throws: ARTErrorInfo.self) { _ = try SyncCursor(channelSerial: channelSerial) } guard case .other(SyncCursor.Error.channelSerialDoesNotMatchExpectedFormat) = err.testsOnly_underlyingLiveObjectsError else { Issue.record("Expected channelSerialDoesNotMatchExpectedFormat error"); return }Also applies to: 65-67
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (1)
1493-1495: De-duplicate publish-failure setup in testsThe failure stub repeats; consider a small helper (e.g., makePublishFailureError("Publish failed")) to DRY this across Set/Remove/Counter tests.
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (1)
570-572: Typed throws(ARTErrorInfo) aligns with MockCoreSDK; goodError construction via LiveObjectsError.other(...).toARTErrorInfo() is consistent with the migration. As above, a shared helper for failure stubs would reduce duplication.
Sources/AblyLiveObjects/Internal/CoreSDK.swift (1)
50-66: Override path: untyped storage + typed rethrow is soundStoring the override as async throws (untyped) avoids OS runtime constraints, while the catch/cast enforces ARTErrorInfo at the boundary. Consider enriching the preconditionFailure message with formatted messages to aid debugging if a non‑ARTErrorInfo slips through.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (26)
AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved(1 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/AblyLiveObjects/Internal/CoreSDK.swift(5 hunks)Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift(3 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift(1 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift(1 hunks)Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift(3 hunks)Sources/AblyLiveObjects/Protocol/ObjectMessage.swift(9 hunks)Sources/AblyLiveObjects/Protocol/SyncCursor.swift(3 hunks)Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift(13 hunks)Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift(2 hunks)Sources/AblyLiveObjects/Utility/Data+Extensions.swift(1 hunks)Sources/AblyLiveObjects/Utility/Errors.swift(5 hunks)Sources/AblyLiveObjects/Utility/InternalError.swift(0 hunks)Sources/AblyLiveObjects/Utility/JSONValue.swift(2 hunks)Sources/AblyLiveObjects/Utility/WireCodable.swift(28 hunks)Sources/AblyLiveObjects/Utility/WireValue.swift(1 hunks)Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift(1 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift(1 hunks)Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift(2 hunks)Tests/AblyLiveObjectsTests/InternalErrorTests.swift(0 hunks)Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift(4 hunks)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift(2 hunks)Tests/AblyLiveObjectsTests/ObjectMessageTests.swift(6 hunks)Tests/AblyLiveObjectsTests/SyncCursorTests.swift(3 hunks)
💤 Files with no reviewable changes (2)
- Sources/AblyLiveObjects/Utility/InternalError.swift
- Tests/AblyLiveObjectsTests/InternalErrorTests.swift
🚧 Files skipped from review as they are similar to previous changes (10)
- Tests/AblyLiveObjectsTests/ObjectMessageTests.swift
- Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift
- AblyLiveObjects.xcworkspace/xcshareddata/swiftpm/Package.resolved
- Sources/AblyLiveObjects/Utility/JSONValue.swift
- Sources/AblyLiveObjects/Protocol/SyncCursor.swift
- Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift
- Sources/AblyLiveObjects/Protocol/ObjectMessage.swift
- Tests/AblyLiveObjectsTests/JS Integration Tests/ObjectsIntegrationTests.swift
- Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift
- Sources/AblyLiveObjects/Utility/WireCodable.swift
🧰 Additional context used
📓 Path-based instructions (4)
**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
**/*.swift: Specify an explicit access control level (SwiftLint explicit_acl) for all declarations in Swift code (tests are exempt)
When extending a type, put the access level on the extension declaration rather than on each member (tests are exempt)
Prefer implicit .init(...) when the type can be inferred in initializer expressions
Prefer enum case shorthand (.caseName) when the type can be inferred
For JSONValue or WireValue, prefer using literal syntax via ExpressibleBy*Literal where possible
Prefer Swift raw string literals for JSON strings instead of escaping double quotes
When an array literal begins with an initializer expression, place the initializer on the line after the opening bracket
Files:
Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swiftSources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Utility/WireValue.swiftSources/AblyLiveObjects/Utility/Data+Extensions.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftTests/AblyLiveObjectsTests/SyncCursorTests.swiftSources/AblyLiveObjects/Protocol/WireObjectMessage.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Utility/Errors.swiftPackage.swift
Tests/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
Tests/**/*.swift: Use the Swift Testing framework (import Testing), not XCTest, in test files
Do not usefatalErrorfor expectation failures; prefer Swift Testing’s#require
Only add labels to test cases or suites when the label differs from the suite struct or test method name
Tag tests per CONTRIBUTING.md’s "Attributing tests to a spec point" with exact comment format; distinguish@specvs@specPartial; do not repeat@specfor the same spec point
Add comments in tests to clarify when certain test data is irrelevant to the scenario
In tests, import Ably usingimport Ably
In tests, import AblyLiveObjects using@testable import AblyLiveObjects
In tests, import_AblyPluginSupportPrivateusingimport _AblyPluginSupportPrivate(do not useinternal import)
When passing a logger to internal components in tests, useTestLogger()
When unwrapping optionals in tests, prefer#requireoverguard let
Files:
Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swiftTests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swiftTests/AblyLiveObjectsTests/SyncCursorTests.swift
Sources/AblyLiveObjects/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/swift.mdc)
In AblyLiveObjects library (non-test) code, import modules as: Ably with
import Ably, and _AblyPluginSupportPrivate withinternal import _AblyPluginSupportPrivate
Files:
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Utility/WireValue.swiftSources/AblyLiveObjects/Utility/Data+Extensions.swiftSources/AblyLiveObjects/Protocol/WireObjectMessage.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Utility/Errors.swift
Sources/**/*.swift
📄 CodeRabbit inference engine (.cursor/rules/testing.mdc)
For
testsOnly_property declarations, do not add generic explanatory comments (their meaning is understood)
Files:
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swiftSources/AblyLiveObjects/Internal/DefaultInternalPlugin.swiftSources/AblyLiveObjects/Utility/WireValue.swiftSources/AblyLiveObjects/Utility/Data+Extensions.swiftSources/AblyLiveObjects/Protocol/WireObjectMessage.swiftSources/AblyLiveObjects/Internal/CoreSDK.swiftSources/AblyLiveObjects/Utility/Errors.swift
🧬 Code graph analysis (9)
Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift (1)
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (1)
testsOnly_publish(109-111)
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (7)
Sources/AblyLiveObjects/Utility/DispatchQueueMutex.swift (1)
withSync(25-35)Sources/AblyLiveObjects/Internal/ObjectCreationHelpers.swift (2)
nosync_creationOperationForLiveMap(103-160)creationOperationForLiveCounter(51-96)Sources/AblyLiveObjects/Internal/InternalDefaultLiveMap.swift (1)
entries(136-140)Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
publish(16-22)Sources/AblyLiveObjects/Internal/ObjectsPool.swift (2)
nosync_getOrCreateMap(434-471)nosync_getOrCreateCounter(387-423)Sources/AblyLiveObjects/Internal/InternalDefaultLiveCounter.swift (1)
value(100-104)Sources/AblyLiveObjects/Utility/Errors.swift (2)
toARTErrorInfo(52-59)toARTErrorInfo(75-77)
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (1)
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
testsOnly_overridePublish(24-26)
Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (2)
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
setPublishHandler(33-37)Sources/AblyLiveObjects/Utility/Errors.swift (2)
toARTErrorInfo(52-59)toARTErrorInfo(75-77)
Sources/AblyLiveObjects/Internal/DefaultInternalPlugin.swift (1)
Sources/AblyLiveObjects/Utility/MarkerProtocolHelpers.swift (1)
castPluginPublicErrorInfo(51-53)
Sources/AblyLiveObjects/Utility/WireValue.swift (2)
Sources/AblyLiveObjects/Utility/ExtendedJSONValue.swift (1)
map(78-99)Sources/AblyLiveObjects/Utility/Errors.swift (2)
toARTErrorInfo(52-59)toARTErrorInfo(75-77)
Sources/AblyLiveObjects/Utility/Data+Extensions.swift (1)
Sources/AblyLiveObjects/Utility/Errors.swift (2)
toARTErrorInfo(52-59)toARTErrorInfo(75-77)
Tests/AblyLiveObjectsTests/InternalDefaultLiveCounterTests.swift (2)
Tests/AblyLiveObjectsTests/Mocks/MockCoreSDK.swift (1)
setPublishHandler(33-37)Sources/AblyLiveObjects/Utility/Errors.swift (2)
toARTErrorInfo(52-59)toARTErrorInfo(75-77)
Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift (3)
Sources/AblyLiveObjects/Utility/Dictionary+Extensions.swift (1)
ablyLiveObjects_mapValuesWithTypedThrow(3-7)Sources/AblyLiveObjects/Utility/WireCodable.swift (4)
stringValueForKey(130-140)objectValueForKey(58-68)wireEnumValueForKey(344-347)optionalObjectValueForKey(73-87)Sources/AblyLiveObjects/Utility/Errors.swift (2)
toARTErrorInfo(52-59)toARTErrorInfo(75-77)
⏰ 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). (12)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: SPM (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: Generate code coverage
🔇 Additional comments (18)
Sources/AblyLiveObjects/Protocol/WireObjectMessage.swift (8)
2-2: Imports style matches guidelines.Using
import Ablyalongsideinternal import _AblyPluginSupportPrivateconforms to our AblyLiveObjects import rules.
85-87: Typed-throws mapping looks correct.Using
ablyLiveObjects_mapValuesWithTypedThrowwiththrows(ARTErrorInfo)andtoJSONValuepreserves typed errors cleanly.
183-193: PartialWireObjectOperation decoding migration to ARTErrorInfo is sound.Constructor now throws
ARTErrorInfoand delegates to existing helpers; no behavior regressions spotted.
238-253: WireObjectOperation decoding order is appropriate.Decoding
actionandobjectIdfirst, then delegating toPartialWireObjectOperation, keeps invariants clear. Good use of typed throws.
292-304: siteTimeserials element-type validation is correct.The closure throws
WireValueDecodingError…toARTErrorInfo()on non-string entries; this aligns with typed throws and avoids silent coercion.
338-341: Consistent migration tothrows(ARTErrorInfo)across decoders.All affected initializers now surface
ARTErrorInfovia the WireCodable helpers; encoding paths unchanged. Looks good.Also applies to: 365-367, 387-395, 419-421, 447-452, 493-500
540-548: Swift 5.9+ syntax is supported. The project’s Package.swift pinsswift-tools-versionto 6.1, which covers the required Swift 5.9 switch-expression initializer.
1-606: InternalError fully removed
Ripgrep shows no standaloneInternalErrorreferences in the codebase; the only matches are withinhostInternalErroridentifiers in JS integration tests, which are unrelated.Tests/AblyLiveObjectsTests/AblyLiveObjectsTests.swift (1)
112-114: Test now expectsARTErrorInfodirectly — correct.This matches the public/test API changes (publish path throws
ARTErrorInfo). Assertions on.codeand.messagelook scoped appropriately.Tests/AblyLiveObjectsTests/InternalDefaultLiveMapTests.swift (1)
1412-1414: Switch to ARTErrorInfo for publish failures looks correctMatches MockCoreSDK’s typed throws signature and preserves underlying LiveObjectsError via toARTErrorInfo(). Consider also asserting userInfo[liveObjectsErrorUserInfoKey] to ensure the rich error is retained.
Sources/AblyLiveObjects/Utility/WireValue.swift (1)
238-248: Typed throws on WireValue.toJSONValue is a good fitUsing get throws(ARTErrorInfo) and mapping .data to ConversionError…toARTErrorInfo() keeps error flow consistent. Please confirm the resulting ARTErrorInfo code/status for this conversion error are stable and intended for client handling; if there’s a dedicated LiveObjectsError case/code for “non‑JSON data”, prefer throwing that directly for clearer semantics.
Sources/AblyLiveObjects/Internal/CoreSDK.swift (2)
9-15: Protocol surface migrated to ARTErrorInfopublish and testsOnly_overridePublish now throw ARTErrorInfo, which aligns SDK and tests. No further changes requested.
78-82: testsOnly_overridePublish forwarding is consistentTyped throws closure accepted and stored; mutex guarding is correct. LGTM.
Sources/AblyLiveObjects/Public/Public Proxy Objects/PublicDefaultRealtimeObjects.swift (2)
109-111: testsOnly_publish now throws ARTErrorInfo – consistent with proxiedForwarding looks correct.
122-124: testsOnly_overridePublish typed throws matches CoreSDKSignature matches CoreSDK/tests; simple pass-through is fine.
Sources/AblyLiveObjects/Internal/InternalDefaultRealtimeObjects.swift (3)
175-199: Appreciate the flattened map creation pathMoving the channel validation and creation operation into a single
withSyncblock keeps thenosync_helper contract honest, and publishing outside the mutex drops the deadlock risk while letting theARTErrorInfobubble straight through.
209-239: Counter creation path now surfaces richer errorsThe early finite check and direct
LiveObjectsError→ARTErrorInfoconversion remove the extra indirection, and keeping the publish outside the mutex preserves the concurrency model.
343-345: testsOnly hook stays aligned with CoreSDKUpdating this helper to throw
ARTErrorInfokeeps the tests in lockstep with the CoreSDK publish signature so we no longer juggle two error types.
Note: This PR is based on top of #85; please review that one first.
It wasn't serving a useful purpose and was just another layer of indirection. Throw
ARTErrorInfodirectly inside the codebase and let it bubble up to the public API (but still keep the richer underlying errors for checking in tests).Resolves #48.
Summary by CodeRabbit