-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ECO-5018] Use wrapper SDK proxy to track Chat SDK usage #211
Conversation
WalkthroughThis PR updates dependency references for the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DefaultChatClient
participant RealtimeClient
participant SDKProxy
Caller->>DefaultChatClient: init(realtime: any SuppliedRealtimeClientProtocol, clientOptions?)
DefaultChatClient->>RealtimeClient: createWrapperSDKProxy(with: options)
RealtimeClient-->>DefaultChatClient: Return proxy client
DefaultChatClient->>DefaultChatClient: Initialize rooms & connection using proxy
DefaultChatClient-->>Caller: Instance created
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
57a8abb
to
79890bf
Compare
79890bf
to
4b3fb22
Compare
4b3fb22
to
3cfb0bd
Compare
3cfb0bd
to
2c70f80
Compare
2c70f80
to
08cfe03
Compare
08cfe03
to
cb21da6
Compare
cb21da6
to
b894916
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Sources/AblyChat/Version.swift (1)
7-7
: Consider using semantic versioning constants.While the version string follows semantic versioning format, consider breaking it down into constants for better maintainability:
-internal let version = "0.1.2" +internal let majorVersion = 0 +internal let minorVersion = 1 +internal let patchVersion = 2 +internal let version = "\(majorVersion).\(minorVersion).\(patchVersion)"Sources/AblyChat/Dependencies.swift (2)
6-10
: Consider adding more documentation and concurrency notes.You've introduced a new
SuppliedRealtimeClientProtocol
that extendsRealtimeClientProtocol
and adds acreateWrapperSDKProxy(with:)
method. This is a great way to cleanly separate the creation of a proxy client. However, since the protocol is markedSendable
, it might be helpful to add documentation clarifying any concurrency guarantees or usage constraints.
42-54
: Refine force casting in channel options copy.Force casting with
// swiftlint:disable:next force_cast
can make debugging difficult ifcopy()
ever returns an unexpected type. Consider replacing it with a safe cast or an explicit error pathway if the cast fails.Here is a suggested fix:
-// swiftlint:disable:next force_cast -opts.copy() as! ARTRealtimeChannelOptions +guard let newOpts = opts.copy() as? ARTRealtimeChannelOptions else { + fatalError("Failed to copy ARTRealtimeChannelOptions to the expected type.") +} +let resolvedOptions: ARTRealtimeChannelOptions = newOpts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved
(1 hunks)Example/AblyChatExample/ContentView.swift
(1 hunks)Example/AblyChatExample/Mocks/MockRealtime.swift
(0 hunks)Package.resolved
(1 hunks)Package.swift
(1 hunks)Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift
(1 hunks)Sources/AblyChat/ChatClient.swift
(2 hunks)Sources/AblyChat/Dependencies.swift
(2 hunks)Sources/AblyChat/Room.swift
(1 hunks)Sources/AblyChat/Version.swift
(1 hunks)Tests/AblyChatTests/ChatAPITests.swift
(7 hunks)Tests/AblyChatTests/DefaultChatClientTests.swift
(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift
(6 hunks)Tests/AblyChatTests/DefaultRoomTests.swift
(11 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift
(10 hunks)Tests/AblyChatTests/Mocks/MockRealtime.swift
(3 hunks)
💤 Files with no reviewable changes (1)
- Example/AblyChatExample/Mocks/MockRealtime.swift
✅ Files skipped from review due to trivial changes (4)
- Tests/AblyChatTests/DefaultRoomTests.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
- AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved
- Package.resolved
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode, macOS (Xcode 16)
🔇 Additional comments (22)
Sources/AblyChat/Version.swift (2)
3-3
: Address or update the TODO comment.The comment indicates this is a temporary implementation copied from chat-js. Since this PR implements ADR-117 for tracking wrapper SDK usage, we should either:
- Remove the TODO if the current implementation aligns with ADR-117
- Update it to reflect the current implementation status
Would you like me to help track this in a new issue?
9-9
: LGTM! The new agent structure aligns with wrapper SDK tracking.The change from a concatenated string to a dictionary structure is a good improvement that:
- Makes the agent information more structured and maintainable
- Aligns with the wrapper SDK proxy tracking requirements from ADR-117
Let's verify this matches the implementation in ably-cocoa:
Sources/AblyChat/Dependencies.swift (1)
12-13
: Confirm concurrency assumptions.
RealtimeClientProtocol
now inheritsSendable
, which is beneficial for Swift concurrency. Ensure that all conforming types can be safely passed across concurrency domains without introducing data races.Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift (4)
3-4
: Smooth transition toSuppliedRealtimeClientProtocol
.Designating
ARTRealtime
asSuppliedRealtimeClientProtocol
is a logical approach that keeps the codebase consistent with the newly introduced proxy client concept.
5-5
: Straightforward conformance toRealtimeClientProtocol
.Extending
ARTWrapperSDKProxyRealtime
allows the code to leverage existing real-time client features. No issues spotted here.
8-8
: Proxy channels conformance.Aligning
ARTWrapperSDKProxyRealtimeChannels
withRealtimeChannelsProtocol
maintains consistent usage alongside the primary channel structures.
11-11
: Proxy channel conformance looks good.Registering
ARTWrapperSDKProxyRealtimeChannel
underRealtimeChannelProtocol
is consistent with the approach above and ensures uniform usage of channel APIs.Tests/AblyChatTests/DefaultChatClientTests.swift (5)
8-11
: Clean initialization for a default client.Switching to a direct
MockRealtime(createWrapperSDKProxyReturnValue: .init())
instantiation clarifies test setup. Good improvement.
18-27
: Ensures real-time reference integrity.The new
test_realtime()
method confirms that theDefaultChatClient
retains the originalrealtime
instance rather than replacing it with a proxy. This is valuable coverage.
29-36
: Checks wrapper SDK proxy agent creation.Verifying
createWrapperSDKProxyOptionsArgument?.agents
ensures correct agent info is passed. Good test for usage tracking requirements.
41-42
: Verifying proxy instantiation consistency.Creating both
proxyClient
andrealtime
in tandem effectively tests different code paths for the real-time client setup.
46-46
: Rooms ownership verification.By asserting
defaultRooms.testsOnly_realtime
is the proxy, you confirm therooms
feature uses the intended wrapped client, aligning with the rest of the abstraction.Sources/AblyChat/ChatClient.swift (2)
47-47
: LGTM! Good use ofnonisolated
.The
nonisolated
keyword is correctly applied to allow access to therealtime
property without requiring actor isolation.
63-68
: LGTM! Good implementation of the wrapper SDK proxy pattern.The constructor correctly accepts a
SuppliedRealtimeClientProtocol
and creates a wrapper SDK proxy for tracking Chat SDK usage, aligning with ADR-117.Tests/AblyChatTests/Mocks/MockRealtime.swift (2)
6-6
: LGTM! Good use of@unchecked Sendable
.The class correctly conforms to
SuppliedRealtimeClientProtocol
and is marked as@unchecked Sendable
since thread safety is manually implemented.
81-91
: LGTM! Well-implemented thread-safe proxy creation.The
createWrapperSDKProxy
method correctly:
- Validates the return value
- Uses mutex for thread-safe options storage
- Returns the appropriate type
Tests/AblyChatTests/ChatAPITests.swift (1)
11-13
: LGTM! Consistent test initialization pattern.The test initialization has been updated to use the new MockRealtime API consistently across all test cases while maintaining the same test coverage.
Also applies to: 32-34, 58-60, 81-83, 108-110, 130-132, 174-176
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
11-11
: LGTM! Consistent test initialization pattern.The test initialization has been updated to use the new MockRealtime API consistently across all test cases while maintaining the same test coverage.
Also applies to: 29-29, 49-49, 75-75, 109-109, 143-143
Example/AblyChatExample/ContentView.swift (1)
19-19
: LGTM!The change simplifies mock instantiation by using direct initialization instead of a factory method, which aligns with the wrapper SDK proxy implementation.
Sources/AblyChat/Room.swift (1)
318-318
: LGTM!The change appropriately:
- Makes the channel options immutable since they're only modified within the loop's scope
- Uses
ARTRealtimeChannelOptions
which aligns with the wrapper SDK proxy implementationPackage.swift (2)
22-23
: Consider using a released version instead of a commit reference.The PR objectives indicate that this change depends on functionality from ably-cocoa#2014 which hasn't been released yet. Using an unreleased commit could be risky:
- The referenced commit might change if the PR is rebased
- The functionality might change before release
- You won't automatically get bug fixes
Consider waiting for the release and then updating to use a version constraint instead:
- revision: "58bbd149cbd92da6c29fad40b51ccdc3ca71404d" + from: "x.y.z" // Replace with the version that includes ably-cocoa#2014✅ Verification successful
Error: No shell scripts or web queries found
Switch to a Released Version for Dependency Stability
The current dependency in
Package.swift
uses an unreleased commit reference for functionality introduced in ably-cocoa#2014. This approach carries the following risks:
- The commit might change if the PR is rebased.
- The intended functionality may evolve before a stable release.
- You won’t automatically benefit from future bug fixes.
Recommendation:
• Update the dependency to use a version constraint (e.g.,
from: "x.y.z"
) once the release incorporating ably-cocoa#2014 is available.
• If you must use the commit reference temporarily, document the reasoning and ensure there’s a plan to switch to a released version promptly.
22-23
: Verify the referenced commit exists and contains the required functionality.The dependency is pinned to a specific commit (
58bbd149cbd92da6c29fad40b51ccdc3ca71404d
) that should contain the new-[ARTRealtime createWrapperSDKProxyWithOptions:]
method.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the commit exists and contains the required functionality # Check if the commit exists in the repository gh api repos/ably/ably-cocoa/commits/58bbd149cbd92da6c29fad40b51ccdc3ca71404d --jq '.sha' # Look for the new method in the codebase at this commit gh api repos/ably/ably-cocoa/git/trees/58bbd149cbd92da6c29fad40b51ccdc3ca71404d?recursive=1 | \ jq -r '.tree[].path' | \ grep -i "ARTRealtime" | \ xargs -I {} gh api repos/ably/ably-cocoa/contents/{}?ref=58bbd149cbd92da6c29fad40b51ccdc3ca71404d | \ jq -r '.content' | base64 -d | \ grep -A 5 "createWrapperSDKProxyWithOptions"Length of output: 582
I'll run a modified shell script that decodes file contents using Python rather than the missing
base64
command. This script will:
- Confirm the commit exists.
- Iterate over files in the commit whose paths contain "ARTRealtime".
- Decode their base64-encoded content with Python.
- Search for "createWrapperSDKProxyWithOptions" and report if the method is found.
Let's run the following script:
Please re-run this script and share the output so we can verify if the required method is present in the commit.
🏁 Script executed:
#!/bin/bash # Description: Verify the commit exists and check for the presence of the method "createWrapperSDKProxyWithOptions" # using Python for the base64 decoding. # Confirm commit exists COMMIT=$(gh api repos/ably/ably-cocoa/commits/58bbd149cbd92da6c29fad40b51ccdc3ca71404d --jq '.sha') echo "Commit found: ${COMMIT}" # Retrieve tree and filter for files containing "ARTRealtime" in their path, # then iterate over them. gh api repos/ably/ably-cocoa/git/trees/58bbd149cbd92da6c29fad40b51ccdc3ca71404d?recursive=1 | \ jq -r '.tree[].path' | grep -i "ARTRealtime" | while read filepath; do echo "Checking file: $filepath" # Get the file content and decode with Python gh api repos/ably/ably-cocoa/contents/"$filepath"?ref=58bbd149cbd92da6c29fad40b51ccdc3ca71404d --jq '.content' | \ python3 -c "import sys, base64; print(base64.b64decode(sys.stdin.read().strip()).decode('utf-8'))" > tmp.txt if grep -q "createWrapperSDKProxyWithOptions" tmp.txt; then echo "Method found in: $filepath" else echo "Method not found in: $filepath" fi done rm -f tmp.txtLength of output: 11311
Commit verification confirmed
The specified commit exists, and the new
createWrapperSDKProxyWithOptions
method is present in the expected files (e.g.,Source/ARTRealtime.m
andSource/include/Ably/ARTRealtime+WrapperSDKProxy.h
). No issues were found with the dependency pin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
So that we can use the wrapper SDK proxy functionality.
Allows us to get rid of the required initializers and to allow the use of `init()`.
There's no need for it to be actor-isolated. Not sure why the compiler allowed it to satisfy the non-async requirement in the ChatClient protocol, because when I tried to fetch it from an explicit DefaultChatClient instance, it said I needed to write `await`.
This implements ADR-117 [1], and will allow us to track usage of the Chat SDK. [1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3413016589/ADR-117+Tracking+wrapper+SDK+usage+continued
Introduced in 1.2.38.
b894916
to
47752d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Tests/AblyChatTests/DefaultChatClientTests.swift (1)
29-36
: Add Given/Then section comments for clarity.While the test logic is correct, adding section comments would improve readability and maintain consistency with other tests.
@Test func createsWrapperSDKProxyRealtimeClientWithAgents() throws { + // Given: An instance of DefaultChatClient let realtime = MockRealtime(createWrapperSDKProxyReturnValue: .init()) let options = ClientOptions() _ = DefaultChatClient(realtime: realtime, clientOptions: options) + // Then: The wrapper SDK proxy is created with correct agent information #expect(realtime.createWrapperSDKProxyOptionsArgument?.agents == ["chat-swift": AblyChat.version]) }Sources/AblyChat/Dependencies.swift (1)
41-56
: Consider safer options handling.While the implementation works, the force cast could be replaced with a safer alternative.
- // swiftlint:disable:next force_cast - opts.copy() as! ARTRealtimeChannelOptions + guard let copiedOpts = opts.copy() as? ARTRealtimeChannelOptions else { + // This should never happen as opts is already ARTRealtimeChannelOptions + return .init() + } + copiedOpts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved
(1 hunks)Example/AblyChatExample/ContentView.swift
(1 hunks)Example/AblyChatExample/Mocks/MockRealtime.swift
(0 hunks)Package.resolved
(1 hunks)Package.swift
(1 hunks)Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift
(1 hunks)Sources/AblyChat/ChatClient.swift
(2 hunks)Sources/AblyChat/Dependencies.swift
(2 hunks)Sources/AblyChat/Room.swift
(1 hunks)Sources/AblyChat/Version.swift
(1 hunks)Tests/AblyChatTests/ChatAPITests.swift
(7 hunks)Tests/AblyChatTests/DefaultChatClientTests.swift
(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift
(6 hunks)Tests/AblyChatTests/DefaultRoomTests.swift
(11 hunks)Tests/AblyChatTests/DefaultRoomsTests.swift
(10 hunks)Tests/AblyChatTests/Mocks/MockRealtime.swift
(3 hunks)
💤 Files with no reviewable changes (1)
- Example/AblyChatExample/Mocks/MockRealtime.swift
🚧 Files skipped from review as they are similar to previous changes (11)
- Example/AblyChatExample/ContentView.swift
- Tests/AblyChatTests/DefaultRoomTests.swift
- Tests/AblyChatTests/DefaultMessagesTests.swift
- Sources/AblyChat/Version.swift
- Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift
- Sources/AblyChat/Room.swift
- AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved
- Tests/AblyChatTests/ChatAPITests.swift
- Sources/AblyChat/ChatClient.swift
- Package.swift
- Tests/AblyChatTests/DefaultRoomsTests.swift
🔇 Additional comments (11)
Package.resolved (2)
2-2
: LGTM! Dependency update aligns with PR objectives.The update to ably-cocoa v1.2.38 and the specific commit containing the wrapper SDK proxy functionality looks correct.
Let's verify the version compatibility and commit contents:
Also applies to: 9-9, 10-10
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the ably-cocoa dependency version and commit contents # Check if the commit exists and contains the wrapper SDK proxy functionality curl -s -H "Accept: application/vnd.github.v3+json" \ "https://api.github.com/repos/ably/ably-cocoa/commits/11f67876d3f2070ac976f639c4cc959d2995e1ca" | \ jq -r '.commit.message' # Check the version tag curl -s -H "Accept: application/vnd.github.v3+json" \ "https://api.github.com/repos/ably/ably-cocoa/git/refs/tags/1.2.38"Length of output: 743
Dependency Update Verified for ably-cocoa v1.2.38
The commit message confirms the release (Merge pull request #2027 … Release version 1.2.38) and the tag lookup corroborates that the commit hash matches the updated version. The changes in
Package.resolved
(originHash, revision, and version bump) align with the intended dependency update and the new wrapper SDK proxy functionality.
2-2
: Verify wrapper SDK proxy implementation.Since this update is specifically for implementing the wrapper SDK proxy tracking feature, let's verify the implementation exists in the referenced commit.
Also applies to: 9-9, 10-10
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify the wrapper SDK proxy implementation in the specified commit # Clone only the specific commit to check for the implementation git clone --quiet --depth 1 --branch 1.2.38 https://github.com/ably/ably-cocoa.git cd ably-cocoa # Search for the wrapper SDK proxy implementation rg -l "createWrapperSDKProxyWithOptions"Length of output: 854
Wrapper SDK Proxy Implementation Verified
The implementation is present in the commit. The search for
createWrapperSDKProxyWithOptions
found its occurrence in:
Source/ARTRealtime.m
Source/include/Ably/ARTRealtime+WrapperSDKProxy.h
This confirms that the wrapper SDK proxy feature has been implemented as expected.
Tests/AblyChatTests/DefaultChatClientTests.swift (3)
8-11
: LGTM! Initialization updated to use wrapper SDK proxy.The initialization has been correctly updated to use the new wrapper SDK proxy functionality.
18-27
: LGTM! Good test coverage for realtime property.The test effectively verifies that the realtime property returns the original client instance rather than the proxy client.
41-47
: LGTM! Test updated to verify proxy client usage.The test has been correctly updated to verify that DefaultRooms uses the wrapper SDK proxy client.
Sources/AblyChat/Dependencies.swift (2)
6-10
: LGTM! Well-designed protocol for wrapper SDK proxy support.The protocol design effectively supports wrapper SDK proxy creation while maintaining type safety through associated types.
12-14
: LGTM! Protocol inheritance updated appropriately.The protocol now correctly extends ARTRealtimeInstanceMethodsProtocol to support wrapper SDK proxy functionality.
Tests/AblyChatTests/Mocks/MockRealtime.swift (4)
6-16
: LGTM! Thread-safe implementation of SuppliedRealtimeClientProtocol.The implementation correctly handles thread safety for proxy options and properly implements the new protocol.
26-36
: LGTM! Initializer updated with backward compatibility.The initializer has been correctly updated to support wrapper SDK proxy while maintaining backward compatibility.
81-92
: LGTM! Thread-safe implementation of createWrapperSDKProxy.The implementation correctly handles proxy creation with proper thread safety and error handling.
93-99
: LGTM! Thread-safe access to proxy options.The property implementation correctly provides synchronized access to the proxy options.
This implements the API agreed in ADR-117 [1], to allow us to track usage of a "modern wrapper SDK" (the term used in that ADR to refer to a wrapper SDK which accepts an already-instantiated core SDK client). We will use it to track usage of the Chat SDK. The functionality described here has already been partially implemented in ably-cocoa [2] and is being used by ably-chat-swift [3]. I found it quite hard to write a spec for this, and the language might still be both too vague and too convoluted. I'm putting this up for review as a first attempt in order not to end up getting stuck trying to write something perfect. [1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3413016589/ADR-117+Tracking+wrapper+SDK+usage+continued [2] ably/ably-cocoa#2014 [3] ably/ably-chat-swift#211
This specifies the API agreed in ADR-117 [1], to allow us to track usage of a "modern wrapper SDK" (the term used in that ADR to refer to a wrapper SDK which accepts an already-instantiated core SDK client). We will use it to track usage of the Chat SDK. The functionality described here has already been partially implemented in ably-cocoa [2] and is being used by ably-chat-swift [3]. I found it quite hard to write a spec for this, and the language might still be both too vague and too convoluted. I'm putting this up for review as a first attempt in order not to end up getting stuck trying to write something perfect. [1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3413016589/ADR-117+Tracking+wrapper+SDK+usage+continued [2] ably/ably-cocoa#2014 [3] ably/ably-chat-swift#211
This specifies the API agreed in ADR-117 [1], to allow us to track usage of a "modern wrapper SDK" (the term used in that ADR to refer to a wrapper SDK which accepts an already-instantiated core SDK client). We will use it to track usage of the Chat SDK. The functionality described here has already been partially implemented in ably-cocoa [2] and is being used by ably-chat-swift [3]. I found it quite hard to write a spec for this, and the language might still be both too vague and too convoluted. I'm putting this up for review as a first attempt in order not to end up getting stuck trying to write something perfect. [1] https://ably.atlassian.net/wiki/spaces/ENG/pages/3413016589/ADR-117+Tracking+wrapper+SDK+usage+continued [2] ably/ably-cocoa#2014 [3] ably/ably-chat-swift#211
This demonstrates the use of the new
-[ARTRealtime createWrapperSDKProxyWithOptions:]
method introduced in ably/ably-cocoa#2014, which implements ADR-117: Tracking wrapper SDK usage, continued, allowing us to track usage of the Chat SDK.Once I've written a specification for the API introduced in ably-cocoa, I will also add something to the chat spec to describe the behaviour implemented in the current PR.
Summary by CodeRabbit
Summary by CodeRabbit
Chores
Refactor
Tests
DefaultChatClient
to ensure correct instance behavior.