[ECO-5216] Attribute most REST requests to wrapper SDK #2031
Conversation
WalkthroughThe changes span multiple files in the Ably Cocoa library. New retry state classes have been introduced for managing connection and attachment retries. Numerous method signatures throughout the library—including in auth, channel, paginated responses, push, realtime, and REST components—have been updated to include an additional Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client App
participant ProxyPushAdmin as ARTWrapperSDKProxyPushAdmin
participant UnderlyingPush as ARTPushAdminInternal
Client->>ProxyPushAdmin: publish(recipient, data, callback)
ProxyPushAdmin->>UnderlyingPush: publish(recipient, data, wrapperSDKAgents, callback)
UnderlyingPush-->>ProxyPushAdmin: Publish Confirmation
ProxyPushAdmin-->>Client: Result
sequenceDiagram
participant Client
participant ARTRealtime
participant ARTRestInternal
Client->>ARTRealtime: timeWithWrapperSDKAgents(wrapperSDKAgents, completion)
ARTRealtime->>ARTRestInternal: _timeWithWrapperSDKAgents(wrapperSDKAgents, completion)
ARTRestInternal-->>ARTRealtime: Time Result
ARTRealtime-->>Client: Callback(time, error)
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 (
|
github-actions
bot
temporarily deployed
to
staging/pull/2031/features
github-actions
bot
temporarily deployed
to
staging/pull/2031/jazzydoc
lawrence-forooghian
force-pushed
the
2029-attribute-all-requests-to-proxy
branch
from
efc0819 to
5737523
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2031/features
github-actions
bot
temporarily deployed
to
staging/pull/2031/jazzydoc
lawrence-forooghian
force-pushed
the
2029-attribute-all-requests-to-proxy
branch
from
5737523 to
44042cf
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2031/features
github-actions
bot
temporarily deployed
to
staging/pull/2031/jazzydoc
lawrence-forooghian
force-pushed
the
2029-attribute-all-requests-to-proxy
branch
from
44042cf to
fa17140
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2031/features
github-actions
bot
temporarily deployed
to
staging/pull/2031/jazzydoc
github-actions
bot
temporarily deployed
to
staging/pull/2031/features
github-actions
bot
temporarily deployed
to
staging/pull/2031/jazzydoc
lawrence-forooghian
force-pushed
the
2029-attribute-all-requests-to-proxy
branch
from
7cf48a3 to
f07057e
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2031/features
github-actions
bot
temporarily deployed
to
staging/pull/2031/jazzydoc
lawrence-forooghian
force-pushed
the
2029-attribute-all-requests-to-proxy
branch
from
f07057e to
880a3c5
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2031/features
github-actions
bot
temporarily deployed
to
staging/pull/2031/jazzydoc
lawrence-forooghian
force-pushed
the
2029-attribute-all-requests-to-proxy
branch
from
880a3c5 to
d6898a9
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2031/features
github-actions
bot
temporarily deployed
to
staging/pull/2031/jazzydoc
lawrence-forooghian
force-pushed
the
2029-attribute-all-requests-to-proxy
branch
from
d6898a9 to
2e1f908
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2031/features
github-actions
bot
temporarily deployed
to
staging/pull/2031/jazzydoc
lawrence-forooghian
force-pushed
the
2029-attribute-all-requests-to-proxy
branch
from
2e1f908 to
d7c0bc1
Compare
lawrence-forooghian
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
Source/include/Ably/ARTRestChannel.h (1)
28-28: 💡 Verification agent🧩 Analysis chain
Verify the impact of removing these method declarations.
The removal of these method declarations from
ARTRestChannelProtocolsuggests they've been moved toARTChannelProtocol. Please ensure:
- These methods are properly declared in
ARTChannelProtocol.h- All implementations still conform to the protocol
Also applies to: 35-35, 42-42
🏁 Script executed:
#!/bin/bash # Check if methods exist in ARTChannelProtocol.h echo "Checking ARTChannelProtocol.h for method declarations..." rg -A 2 "history:|status:|setOptions:" "Source/include/Ably/ARTChannelProtocol.h" # Check if any implementations might be affected echo "Checking for potential implementations..." rg -A 2 "^- \(.*\)(history|status|setOptions).*\{" "Source/"Length of output: 10603
Review: Ensure All Channel Methods Are Properly Declared in the New Protocol
- The relocation of the
history:method has been verified. In ARTChannelProtocol.h the declaration
- (void)history:(ARTPaginatedMessagesCallback)callback;
confirms that thehistory:method has been moved from ARTRestChannelProtocol.- However, the grep search did not reveal declarations for the
status:andsetOptions:methods in ARTChannelProtocol.h. Please verify whether:
- These methods are intended to be declared in ARTChannelProtocol (or another appropriate header), or
- Their omission is deliberate and they are either declared elsewhere or handled by a different mechanism.
- Also, double-check that all implementations (e.g., in ARTRestChannel.m and ARTRealtimeChannel.m) still conform to the updated protocol(s).
🧹 Nitpick comments (22)
Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (1)
89-91: LGTM! Consider adding documentation.The new history method declarations with
wrapperSDKAgentsparameter align well with the PR objectives. Consider adding documentation comments to describe:
- The purpose of the
wrapperSDKAgentsparameter- The difference between the two methods
- Any specific error conditions for the BOOL-returning variant
Add documentation comments in the following format:
/** * Retrieves channel history with wrapper SDK attribution. * @param wrapperSDKAgents Dictionary containing wrapper SDK information for request attribution * @param completion Callback with paginated messages result */Source/PrivateHeaders/Ably/ARTWrapperSDKProxyPushChannelSubscriptions+Private.h (1)
7-11: Consider adding documentation to improve maintainability.Adding documentation would help future maintainers understand:
- The purpose of this proxy class
- The relationship with ARTPushChannelSubscriptions
- The role of proxyOptions in request attribution
Example documentation:
@interface ARTWrapperSDKProxyPushChannelSubscriptions () +/** + * Initializes a new push channel subscriptions proxy that wraps the underlying implementation + * and attributes REST requests to the wrapper SDK. + * + * @param pushChannelSubscriptions The underlying push channel subscriptions implementation to wrap + * @param proxyOptions Configuration for the wrapper SDK, including agent information + * @return A new proxy instance + */ - (instancetype)initWithPushChannelSubscriptions:(ARTPushChannelSubscriptions *)pushChannelSubscriptions proxyOptions:(ARTWrapperSDKProxyOptions *)proxyOptions NS_DESIGNATED_INITIALIZER;Source/PrivateHeaders/Ably/ARTPushChannel+Private.h (1)
14-34: Add documentation for the wrapperSDKAgents parameter.Consider adding documentation comments to explain:
- The purpose and expected content of the
wrapperSDKAgentsdictionary- When it should/shouldn't be provided
- Any specific format requirements for the dictionary keys/values
Example:
/** * Subscribe device with optional wrapper SDK attribution. * @param wrapperSDKAgents Dictionary containing wrapper SDK information for request attribution. * Keys and values should follow the format specified in <link_to_spec>. */Source/include/Ably/ARTWrapperSDKProxyPushChannelSubscriptions.h (2)
6-10: Consider enhancing documentation with analytics details.While the documentation clearly states the purpose and intended users, it would be helpful to include:
- What specific analytics information is being sent
- How this information is used
- Any privacy implications
1-19: Architectural approach aligns well with PR objectives.The proxy pattern is a good choice for:
- Intercepting and attributing REST requests
- Maintaining a clean separation between core and wrapper SDK functionality
- Enabling analytics tracking without modifying the core implementation
Source/ARTWrapperSDKProxyRealtimePresence.m (3)
18-18: Use a more descriptive parameter name.The parameter named
pushin the initializer may be misleading since it is assigned to_underlyingRealtimePresence. Consider renaming the parameter torealtimePresenceto align better with the property name and improve readability.- (instancetype)initWithRealtimePresence:(ARTRealtimePresence *)push proxyOptions:(ARTWrapperSDKProxyOptions *)proxyOptions { - if (self = [super init]) { - _underlyingRealtimePresence = push; + (instancetype)initWithRealtimePresence:(ARTRealtimePresence *)realtimePresence proxyOptions:(ARTWrapperSDKProxyOptions *)proxyOptions { + if (self = [super init]) { + _underlyingRealtimePresence = realtimePresence;
18-24: Consider validating nil arguments in the initializer.Currently, there is no check to ensure that
push/realtimePresenceorproxyOptionsare non-null. If either is unexpectedly nil,_underlyingRealtimePresenceor_proxyOptionswould be assigned nil without warning, potentially leading to runtime errors. You might want to add guard checks or assertions to help detect issues early.
27-125: Noticeable pass-through pattern.Most methods simply forward calls to
underlyingRealtimePresence. This is acceptable for a proxy, but if additional methods are introduced in the future, consider a mechanism to reduce boilerplate. For example, you could dynamically forward unrecognized selectors to the underlying instance for fewer explicit pass-through methods, though that approach comes with trade-offs.Source/include/Ably/ARTWrapperSDKProxyRealtimePresence.h (1)
12-14: LGTM! Good practice preventing direct instantiation.The class properly conforms to
ARTRealtimePresenceProtocoland correctly prevents direct instantiation by markinginitas unavailable. However, consider adding documentation explaining the proper way to instantiate this class.Add a documentation comment explaining the intended initialization method:
- (instancetype)init NS_UNAVAILABLE; +/** + * This class should not be instantiated directly. + * Use the appropriate factory method from ARTWrapperSDKProxyRealtime instead. + */Source/include/Ably/ARTWrapperSDKProxyPushChannel.h (1)
6-12: Consider documenting thread-safety implications.The class is marked as
NS_SWIFT_SENDABLEwhich indicates thread-safety guarantees. Consider adding documentation about the thread-safety properties and any requirements for concurrent usage.Add the following to the documentation:
/** * An object which wraps an instance of `ARTPushChannel` and provides a similar API. It allows Ably-authored wrapper SDKs to send analytics information so that Ably can track the usage of the wrapper SDK. * * - Important: This class should only be used by Ably-authored SDKs. + * - Note: This class is thread-safe and can be safely used from multiple threads. */Source/PrivateHeaders/Ably/ARTChannel.h (2)
16-23: Enhance documentation for thread safety.The class is marked as
NS_SWIFT_SENDABLEwhich implies thread safety guarantees. Consider adding documentation about thread safety implications and any requirements for concurrent usage.Add the following to the class documentation:
/** * The base class for `ARTRestChannel` and `ARTRealtimeChannel`. * Ably platform service organizes the message traffic within applications into named channels. Channels are the medium through which messages are distributed; clients attach to channels to subscribe to messages, and every message published to a unique channel is broadcast by Ably to all subscribers. * + * This class is thread-safe and can be safely accessed from multiple threads concurrently. * * @see See `ARTChannelProtocol` for details. */
47-48: Add nullability annotation for completion callback.The completion callback parameter lacks an explicit nullability annotation. Based on the parameter name and type, it appears to be non-nullable.
- (void)historyWithWrapperSDKAgents:(nullable NSStringDictionary *)wrapperSDKAgents - completion:(ARTPaginatedMessagesCallback)callback; + completion:(nonnull ARTPaginatedMessagesCallback)callback;Source/PrivateHeaders/Ably/ARTPaginatedResult+Private.h (1)
19-33: LGTM! Consider reordering parameters for better readability.The changes correctly add the
wrapperSDKAgentsandloggerparameters. However, consider reordering the parameters to group related ones together.Consider this parameter ordering for better readability:
- - (instancetype)initWithItems:(NSArray *)items - rest:(ARTRestInternal *)rest - relFirst:(NSMutableURLRequest *)relFirst - relCurrent:(NSMutableURLRequest *)relCurrent - relNext:(NSMutableURLRequest *)relNext - responseProcessor:(ARTPaginatedResultResponseProcessor)responseProcessor - wrapperSDKAgents:(nullable NSStringDictionary *)wrapperSDKAgents - logger:(ARTInternalLog *)logger NS_DESIGNATED_INITIALIZER; + - (instancetype)initWithItems:(NSArray *)items + rest:(ARTRestInternal *)rest + logger:(ARTInternalLog *)logger + wrapperSDKAgents:(nullable NSStringDictionary *)wrapperSDKAgents + relFirst:(NSMutableURLRequest *)relFirst + relCurrent:(NSMutableURLRequest *)relCurrent + relNext:(NSMutableURLRequest *)relNext + responseProcessor:(ARTPaginatedResultResponseProcessor)responseProcessor + NS_DESIGNATED_INITIALIZER;Source/ARTWrapperSDKProxyPush.m (3)
38-52: Consider adding detailed error logging.While the implementation is correct, adding detailed error logging would help with debugging and monitoring.
+ (void)didFailToRegisterForLocationNotificationsWithError:(nonnull NSError *)error realtime:(nonnull ARTRealtime *)realtime { + NSLog(@"[ARTWrapperSDKProxyPush] Location notification registration failed: %@", error.localizedDescription); [ARTPush didFailToRegisterForLocationNotificationsWithError:error realtime:realtime]; }
54-68: Consider adding success logging.While the implementation is correct, adding success logging would help with monitoring and debugging.
+ (void)didRegisterForLocationNotificationsWithDeviceToken:(nonnull NSData *)deviceToken realtime:(nonnull ARTRealtime *)realtime { + NSLog(@"[ARTWrapperSDKProxyPush] Location notification registration succeeded with token: %@", deviceToken); [ARTPush didRegisterForLocationNotificationsWithDeviceToken:deviceToken realtime:realtime]; }
38-68: Consider consolidating duplicate code patterns.The notification registration methods follow identical patterns of delegating to the underlying
ARTPushclass. Consider using a macro or helper method to reduce code duplication.Example consolidation:
+#define ART_PUSH_DELEGATE(method, ...) \ + [ARTPush method:__VA_ARGS__] -+ (void)didFailToRegisterForLocationNotificationsWithError:(nonnull NSError *)error realtime:(nonnull ARTRealtime *)realtime { - [ARTPush didFailToRegisterForLocationNotificationsWithError:error realtime:realtime]; -} ++ (void)didFailToRegisterForLocationNotificationsWithError:(nonnull NSError *)error realtime:(nonnull ARTRealtime *)realtime { + ART_PUSH_DELEGATE(didFailToRegisterForLocationNotificationsWithError, error, realtime); +}Source/ARTRestPresence.m (1)
149-152: Consider addressing the FIXME comment.The comment indicates a need to refactor the encoder to handle message encoding/decoding more consistently.
Would you like me to help create an issue to track this refactoring task?
Source/ARTPushChannelSubscriptions.m (1)
65-118: Consider enhancing error handling in the save method.The save method could benefit from more robust error handling:
- The error from
encodePushChannelSubscriptionis ignored- The response data is not checked for nil before decoding
Apply this diff to improve error handling:
- request.HTTPBody = [[self->_rest defaultEncoder] encodePushChannelSubscription:channelSubscription error:nil]; + NSError *encodeError = nil; + NSData *body = [[self->_rest defaultEncoder] encodePushChannelSubscription:channelSubscription error:&encodeError]; + if (encodeError) { + callback([ARTErrorInfo createFromNSError:encodeError]); + return; + } + request.HTTPBody = body; if (response.statusCode == 200 /*Ok*/ || response.statusCode == 201 /*Created*/) { - ARTLogDebug(self->_logger, @"channel subscription saved successfully"); - callback(nil); + if (!data) { + callback([ARTErrorInfo createWithCode:ARTStateResponseDataError message:@"No response data"]); + return; + } + ARTLogDebug(self->_logger, @"channel subscription saved successfully"); + callback(nil); }Source/ARTPushChannel.m (1)
105-139: Consider enhancing error handling in subscribeDeviceWithWrapperSDKAgents.The method could benefit from more robust error handling:
- The response data is not checked for nil
- The response status code is not verified
Apply this diff to improve error handling:
[self->_rest executeRequest:request withAuthOption:ARTAuthenticationOn wrapperSDKAgents:wrapperSDKAgents completion:^(NSHTTPURLResponse *response, NSData *data, NSError *error) { - if (error) { + if (response.statusCode != 200 && response.statusCode != 201) { + NSString *message = data ? [[NSString alloc] initWithData:data encoding:NSUTF8StringEncoding] : @"No response data"; + ARTLogError(self->_logger, @"%@: subscribe notifications for device %@ in channel %@ failed with status code %ld (%@)", NSStringFromClass(self.class), deviceId, self->_channel.name, (long)response.statusCode, message); + callback([ARTErrorInfo createWithCode:response.statusCode*100 status:response.statusCode message:message]); + } else if (error) { ARTLogError(self->_logger, @"%@: subscribe notifications for device %@ in channel %@ failed (%@)", NSStringFromClass(self.class), deviceId, self->_channel.name, error.localizedDescription); + callback([ARTErrorInfo createFromNSError:error]); + } else { + ARTLogDebug(self->_logger, @"%@: subscribe notifications for device %@ in channel %@ successful", NSStringFromClass(self.class), deviceId, self->_channel.name); + callback(nil); } - if (callback) callback(error ? [ARTErrorInfo createFromNSError:error] : nil); }];Source/ARTPushDeviceRegistrations.m (1)
120-170: Consider enhancing error handling in get method.The method could benefit from more robust error handling:
- The response data is not checked for nil before decoding
- The error message could be more descriptive
Apply this diff to improve error handling:
[self->_rest executeRequest:request withAuthOption:ARTAuthenticationOn wrapperSDKAgents:wrapperSDKAgents completion:^(NSHTTPURLResponse *response, NSData *data, NSError *error) { if (response.statusCode == 200 /*OK*/) { + if (!data) { + callback(nil, [ARTErrorInfo createWithCode:ARTStateResponseDataError message:@"No response data for device details"]); + return; + } NSError *decodeError = nil; ARTDeviceDetails *device = [self->_rest.encoders[response.MIMEType] decodeDeviceDetails:data error:&decodeError]; if (decodeError) { - ARTLogDebug(self->_logger, @"%@: decode device failed (%@)", NSStringFromClass(self.class), error.localizedDescription); + ARTLogDebug(self->_logger, @"%@: decode device failed (%@)", NSStringFromClass(self.class), decodeError.localizedDescription); callback(nil, [ARTErrorInfo createFromNSError:decodeError]); }Test/Tests/RealtimeClientTests.swift (1)
351-351: Enhance test coverage for wrapper SDK agent functionality.While the
wrapperSDKAgentsparameter has been added, the test case doesn't verify that the wrapper SDK agent information is correctly propagated in the REST request. Consider adding assertions to validate this behavior.Add assertions to verify that:
- The REST request includes the wrapper SDK agent information when provided
- The REST request doesn't include wrapper SDK agent information when nil is passed
try client.internal.rest.stats(query, wrapperSDKAgents:nil, callback: { paginated, error in + // Verify that the request doesn't include wrapper SDK agent information + guard let transport = client.internal.transport as? TestProxyTransport else { + XCTFail("TestProxyTransport is not set") + return + } + XCTAssertNil(transport.lastRequestWrapperSDKAgent)Ably.xcodeproj/project.pbxproj (1)
377-379: LGTM! Good architectural improvement with channel protocol abstraction.The introduction of
ARTChannelProtocol.has a public header suggests improved abstraction in the channel implementation.Also applies to: 1376-1376, 2731-2731, 2924-2924
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
Ably.xcodeproj/project.pbxproj(41 hunks)Source/ARTAuth.m(1 hunks)Source/ARTChannel.m(1 hunks)Source/ARTHTTPPaginatedResponse.m(1 hunks)Source/ARTPaginatedResult.m(5 hunks)Source/ARTPushAdmin.m(3 hunks)Source/ARTPushChannel.m(11 hunks)Source/ARTPushChannelSubscriptions.m(9 hunks)Source/ARTPushDeviceRegistrations.m(10 hunks)Source/ARTRealtime.m(4 hunks)Source/ARTRealtimeChannel.m(3 hunks)Source/ARTRealtimePresence.m(2 hunks)Source/ARTRest.m(6 hunks)Source/ARTRestChannel.m(4 hunks)Source/ARTRestPresence.m(3 hunks)Source/ARTWrapperSDKProxyPush.m(1 hunks)Source/ARTWrapperSDKProxyPushAdmin.m(1 hunks)Source/ARTWrapperSDKProxyPushChannel.m(1 hunks)Source/ARTWrapperSDKProxyPushChannelSubscriptions.m(1 hunks)Source/ARTWrapperSDKProxyPushDeviceRegistrations.m(1 hunks)Source/ARTWrapperSDKProxyRealtime.m(3 hunks)Source/ARTWrapperSDKProxyRealtimeChannel.m(4 hunks)Source/ARTWrapperSDKProxyRealtimePresence.m(1 hunks)Source/Ably.modulemap(1 hunks)Source/PrivateHeaders/Ably/ARTChannel.h(1 hunks)Source/PrivateHeaders/Ably/ARTHTTPPaginatedResponse+Private.h(0 hunks)Source/PrivateHeaders/Ably/ARTPaginatedResult+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTPaginatedResult+Subclass.h(1 hunks)Source/PrivateHeaders/Ably/ARTPushAdmin+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTPushChannel+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTPushChannelSubscriptions+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTPushDeviceRegistrations+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRealtime+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRealtimePresence+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTRest+Private.h(3 hunks)Source/PrivateHeaders/Ably/ARTRestChannel+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTRestPresence+Private.h(2 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyPush+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyPushAdmin+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyPushChannel+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyPushChannelSubscriptions+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyPushDeviceRegistrations+Private.h(1 hunks)Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimePresence+Private.h(1 hunks)Source/include/Ably.modulemap(1 hunks)Source/include/Ably/ARTChannelProtocol.h(1 hunks)Source/include/Ably/ARTPushChannel.h(0 hunks)Source/include/Ably/ARTRestChannel.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyPush.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyPushAdmin.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyPushChannel.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyPushChannelSubscriptions.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyPushDeviceRegistrations.h(1 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtime.h(2 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h(2 hunks)Source/include/Ably/ARTWrapperSDKProxyRealtimePresence.h(1 hunks)Source/include/Ably/AblyInternal.h(1 hunks)Source/include/Ably/AblyPublic.h(1 hunks)Test/Tests/AuthTests.swift(8 hunks)Test/Tests/RealtimeClientPresenceTests.swift(1 hunks)Test/Tests/RealtimeClientTests.swift(1 hunks)Test/Tests/WrapperSDKProxyTests.swift(1 hunks)
💤 Files with no reviewable changes (2)
- Source/include/Ably/ARTPushChannel.h
- Source/PrivateHeaders/Ably/ARTHTTPPaginatedResponse+Private.h
✅ Files skipped from review due to trivial changes (2)
- Source/include/Ably/AblyPublic.h
- Test/Tests/RealtimeClientPresenceTests.swift
🧰 Additional context used
🧠 Learnings (6)
Source/include/Ably/ARTChannelProtocol.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTRealtimeChannelOptions.h:33-33
Timestamp: 2024-11-12T07:31:53.691Z
Learning: `ARTChannelOptions` is already marked with `NS_SWIFT_SENDABLE`.
Source/PrivateHeaders/Ably/ARTRestPresence+Private.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1995
File: Source/ARTRealtimeChannel.m:227-227
Timestamp: 2024-11-12T07:31:47.967Z
Learning: In `ARTRealtimeChannelInternal`, the `internalPresence` property is declared in the private header `ARTRealtimeChannel+Private.h`, so it does not need to be redeclared in `ARTRealtimeChannel.m`.
Source/PrivateHeaders/Ably/ARTRestChannel+Private.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTRealtimeChannelOptions.h:33-33
Timestamp: 2024-11-12T07:31:53.691Z
Learning: `ARTChannelOptions` is already marked with `NS_SWIFT_SENDABLE`.
Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h (2)
Learnt from: maratal
PR: ably/ably-cocoa#1995
File: Source/ARTRealtimeChannel.m:227-227
Timestamp: 2024-11-12T07:31:47.967Z
Learning: In `ARTRealtimeChannelInternal`, the `internalPresence` property is declared in the private header `ARTRealtimeChannel+Private.h`, so it does not need to be redeclared in `ARTRealtimeChannel.m`.
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTRealtimeChannelOptions.h:33-33
Timestamp: 2024-11-12T07:31:53.691Z
Learning: `ARTChannelOptions` is already marked with `NS_SWIFT_SENDABLE`.
Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (1)
Learnt from: maratal
PR: ably/ably-cocoa#1973
File: Source/include/Ably/ARTRealtimeChannelOptions.h:33-33
Timestamp: 2024-11-12T07:31:53.691Z
Learning: `ARTChannelOptions` is already marked with `NS_SWIFT_SENDABLE`.
Source/ARTWrapperSDKProxyRealtimeChannel.m (1)
Learnt from: maratal
PR: ably/ably-cocoa#1995
File: Source/ARTRealtimeChannel.m:227-227
Timestamp: 2024-11-12T07:31:47.967Z
Learning: In `ARTRealtimeChannelInternal`, the `internalPresence` property is declared in the private header `ARTRealtimeChannel+Private.h`, so it does not need to be redeclared in `ARTRealtimeChannel.m`.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS17_2)
- GitHub Check: check (iOS, test_iOS17_2)
🔇 Additional comments (99)
Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (1)
6-6: LGTM! Explicit import of base class header.The addition of explicit import for
ARTChannel.himproves code clarity by making the inheritance dependency visible.Source/PrivateHeaders/Ably/ARTWrapperSDKProxyPushChannelSubscriptions+Private.h (1)
1-14: LGTM! Clean implementation following Objective-C best practices.The implementation is well-structured with proper:
- Nullability annotations
- Use of NS_DESIGNATED_INITIALIZER
- Forward declarations
- Naming conventions
Source/PrivateHeaders/Ably/ARTPushChannel+Private.h (2)
4-4: LGTM! Good practice for dependency management.The addition of the
ARTChannelforward declaration is appropriate and follows good header file practices by avoiding potential circular dependencies.
14-37: LGTM! Consistent implementation of wrapper SDK attribution.The method signatures have been consistently updated to include the nullable
wrapperSDKAgentsparameter, maintaining both synchronous and asynchronous versions for each operation.Source/include/Ably/ARTWrapperSDKProxyPushChannelSubscriptions.h (1)
1-4: LGTM! Good use of nullability annotations.The imports are appropriate, and the use of nullability annotations follows best practices for modern Objective-C code.
Source/PrivateHeaders/Ably/ARTPaginatedResult+Subclass.h (2)
8-8: LGTM! Property declaration aligns with PR objectives.The property declaration is well-structured with appropriate attributes (nullable, nonatomic, readonly) for tracking wrapper SDK agents in paginated results.
8-8:✅ Verification successful
Verify NSStringDictionary type definition.
Please ensure that NSStringDictionary is properly defined and imported.
🏁 Script executed:
#!/bin/bash # Search for NSStringDictionary definition rg "NSStringDictionary" --type objc --type objcppLength of output: 18327
NSStringDictionary is properly defined and accessible.
We confirmed that the type is declared in
Source/include/Ably/ARTTypes.has:typedef NSDictionary<NSString *, NSString *> NSStringDictionary;Since this definition is available and used across multiple files, no further changes are required here.
Source/PrivateHeaders/Ably/ARTWrapperSDKProxyRealtimePresence+Private.h (3)
1-4: LGTM! Clean import and forward declaration.The import statement and forward declaration follow good practices, minimizing header dependencies.
5-5: LGTM! Proper nullability annotations.The use of nullability annotations improves Swift interoperability and type safety.
Also applies to: 14-14
7-12:✅ Verification successful
LGTM! Well-structured class extension with proper initialization.
The interface follows good practices:
- Uses dependency injection for better testability
- Properly marked designated initializer
- Clear parameter names
Let's verify the implementation of this interface:
🏁 Script executed:
#!/bin/bash # Description: Verify the implementation of ARTWrapperSDKProxyRealtimePresence # Check if the implementation file exists and contains the initializer rg -A 5 "initWithRealtimePresence:.+proxyOptions:" --type objc # Check for any direct instantiations without using this designated initializer rg "\\[\\s*ARTWrapperSDKProxyRealtimePresence\\s+new\\]|\\[\\s*ARTWrapperSDKProxyRealtimePresence\\s+alloc\\]\\s*init\\]" --type objcLength of output: 698
LGTM! The designated initializer is properly implemented.
- Verified that the implementation in Source/ARTWrapperSDKProxyRealtimePresence.m correctly uses the designated initializer, as evidenced by the matching pattern for
initWithRealtimePresence:proxyOptions:.- Confirmed that there are no direct instantiations via
[ARTWrapperSDKProxyRealtimePresence new]or[ARTWrapperSDKProxyRealtimePresence alloc] init], ensuring that dependency injection via the designated initializer is enforced.Source/include/Ably/ARTWrapperSDKProxyRealtime.h (2)
6-6: LGTM! Forward declaration aligns with wrapper SDK pattern.The change from
ARTPushtoARTWrapperSDKProxyPushmaintains consistency with the wrapper SDK architecture.
23-23:✅ Verification successful
LGTM! Property type change aligns with PR objectives.
The change from
ARTPushtoARTWrapperSDKProxyPushcorrectly implements the wrapper SDK pattern for push functionality.Let's verify the implementation of
ARTWrapperSDKProxyPush:
🏁 Script executed:
#!/bin/bash # Description: Verify the implementation of ARTWrapperSDKProxyPush # Check if ARTWrapperSDKProxyPush is properly implemented rg -t objc -t h "ARTWrapperSDKProxyPush" -A 10Length of output: 29062
Property type change verified – implementation confirmed
The ARTWrapperSDKProxyPush class is fully implemented and correctly integrated throughout the codebase, as evidenced by its usage in ARTWrapperSDKProxyRealtime and associated modules. The update from ARTPush to ARTWrapperSDKProxyPush aligns with the wrapper SDK pattern and meets the PR objectives.LGTM!
Source/PrivateHeaders/Ably/ARTWrapperSDKProxyPushAdmin+Private.h (3)
1-4: LGTM! Clean import and forward declaration.The import of the public header and forward declaration of dependencies follow good practices.
5-8: LGTM! Proper use of nullability annotations.The nullability context is correctly established, and the interface extension is properly declared.
9-10:❓ Verification inconclusive
Verify alignment with PR objectives regarding push activation.
The PR objectives mention that push activation should be an exception to the wrapper SDK agent implementation. Please verify that this proxy implementation respects that exception.
Let's search for push activation handling in the codebase:
🏁 Script executed:
#!/bin/bash # Search for push activation related code rg -l "push.*activat" --type objcLength of output: 35
Verify push activation exception handling manually.
The initializer appears to be implemented correctly per naming conventions and designated initialization. However, the PR objectives specify that push activation should be an exception for the wrapper SDK agent implementation. Our automated search did not find any push activation-related code, which suggests that this proxy implementation does not inadvertently integrate push activation.
Please verify manually that this behavior is intentional and that no push activation logic is missing or mismatching the PR objectives.
Source/ARTWrapperSDKProxyRealtimePresence.m (1)
55-65: Good introduction of wrapper SDK agents.Injecting the
wrapperSDKAgentsfromself.proxyOptions.agentsinto the history methods aligns with the goal of adding wrapper SDK context to relevant calls. Ensure that these agents correctly capture all necessary SDK metadata for analytics or debugging purposes.Source/include/Ably/ARTWrapperSDKProxyRealtimePresence.h (2)
6-12: LGTM! Well-documented and Swift-compatible interface.The documentation clearly states the purpose and intended usage of the class. The use of
NS_SWIFT_SENDABLEdemonstrates good Swift interoperability practices.
12-12:❓ Verification inconclusive
Verify protocol conformance implementation.
The class conforms to
ARTRealtimePresenceProtocol. Please ensure that all required protocol methods are implemented in the corresponding implementation file, including:
- Presence history with wrapper SDK agents
- Enter, leave, and update presence
- Subscribe/unsubscribe to presence events
🏁 Script executed:
#!/bin/bash # Description: Check for the implementation file and verify protocol conformance # Find the implementation file fd "ARTWrapperSDKProxyRealtimePresence.m" -t f # Check for required method implementations rg "ARTWrapperSDKProxyRealtimePresence" -A 5Length of output: 17129
Action: Verify Complete Protocol Implementation
The class interface correctly declares conformance to
ARTRealtimePresenceProtocol. However, the corresponding implementation in
Source/ARTWrapperSDKProxyRealtimePresence.mcurrently only shows the initialization logic. Please ensure that all required protocol methods are either implemented or properly delegated. In particular, confirm that the implementations (or appropriate forwarding to the underlyingARTRealtimePresenceinstance) for the following are present:
- Presence history support for wrapper SDK agents
- Enter, leave, and update presence methods
- Subscribe/unsubscribe handling for presence events
If these methods are missing or indirectly implemented, update the implementation file accordingly.
Source/include/Ably/ARTWrapperSDKProxyPushChannel.h (2)
1-2: LGTM! Clean and appropriate imports.The imports are minimal and correctly include the necessary dependencies.
4-5: LGTM! Proper nullability annotations.The file correctly uses nullability annotations which improves Swift interoperability.
Also applies to: 18-19
Source/PrivateHeaders/Ably/ARTChannel.h (1)
1-13: LGTM! Well-organized imports and forward declarations.The file structure follows good practices with clear separation of system and framework imports, followed by forward declarations of required classes.
Source/PrivateHeaders/Ably/ARTRestPresence+Private.h (3)
9-9: Check for any lost functionality by dropping ARTPresence inheritance.
Switching the inheritance toNSObjectmay drop methods or behaviors previously inherited fromARTPresence. Confirm that the required functionality has been re-implemented or is no longer needed.
19-19: Parameter alignment looks consistent.
The updatedhistory:wrapperSDKAgents:callback:error:signature aligns well with the rest of the wrapper SDK agents approach. Ensure downstream callers handle the additional parameter appropriately.
21-22: New convenience method is properly introduced.
This additional entry point for fetching history withwrapperSDKAgentsappears cleanly integrated and helps maintain a consistent API.Source/ARTHTTPPaginatedResponse.m (1)
25-25: Verify preserved_wrapperSDKAgentsusage.
You’re forwardingwrapperSDKAgentsto the superclass initializer but not storing it locally. Confirm that it’s intentional that this subclass no longer retains a copy ofwrapperSDKAgents.Source/ARTRestChannel.m (5)
48-48: Bridging to internal history method withnilagents is acceptable.
Forwarding anilvalue forwrapperSDKAgentsis consistent with current usage and poses no clear issues.
96-96: No concerns with the single-parameter history call.
This simple delegate method tohistoryWithWrapperSDKAgents:nilis a reasonable fallback to preserve existing API behavior.
151-152: Good default initialization ofARTDataQuery.
Providing a convenience approach that uses an empty data query when no parameters are given simplifies usage.
155-155: Extended history signature cleanly integrates wrapper SDK agents.
Adding thewrapperSDKAgentsparameter is consistent with the rest of the updates. The error handling and callback usage remain sound.
213-213: PassingwrapperSDKAgentsto executePaginated is consistent.
This line properly propagates the new parameter toARTPaginatedResultlogic, ensuring uniform wrapper tracking across requests.Source/ARTRealtime.m (6)
153-154: Consider whether passingnilis intentional.
These lines passnilforwrapperSDKAgents, which might be correct if there's no additional SDK agent context here. However, if future changes require capturing agent details, consider providing a way to set those agents from the public API.
172-173: No issues found in stats retrieval.
This forwarding logic to_internalis minimal and consistent with the newwrapperSDKAgentsstructure.
177-177: Straightforward bridging of stats call.
Forwarding this call withnilforwrapperSDKAgentsmaintains consistency with other stats overloads.
506-509: New method to retrieve time with wrapper SDK context.
The pass-through approach correctly propagates the newwrapperSDKAgentsparameter further down.
560-562: Convenience method for stats retrieval with wrapper SDK agents.
This addition provides a defaultARTStatsQuery, ensuring thatwrapperSDKAgentscan be used when needed.
565-566: Extended stats query method with wrapper SDK agents.
This directly delegates toself.rest, which is consistent with the rest of the design.Source/PrivateHeaders/Ably/ARTWrapperSDKProxyPush+Private.h (1)
1-14: New private interface for wrappingARTPushfunctionality.
The designated initializer is clearly declared, and the private category naming follows a logical convention. Overall, this file appears consistent and maintainable as an internal API extension.Source/PrivateHeaders/Ably/ARTWrapperSDKProxyPushChannel+Private.h (1)
1-14: New private interface forARTWrapperSDKProxyPushChannel.
This designated initializer cleanly separates proxy options from the underlyingARTPushChannel. The naming matches library conventions, and the nullability annotations are clear.Source/PrivateHeaders/Ably/ARTWrapperSDKProxyPushDeviceRegistrations+Private.h (1)
1-14: LGTM! Well-structured private interface.The private interface is well-designed with proper use of nullability annotations and designated initializer pattern. The implementation aligns with the PR objectives by requiring both push device registrations and proxy options for initialization.
Source/include/Ably/ARTWrapperSDKProxyPushDeviceRegistrations.h (1)
6-15: LGTM! Well-documented and properly designed interface.The interface is well-designed with:
- Clear documentation explaining purpose and usage restrictions
- Swift concurrency support
- Protocol conformance
- Prevention of direct initialization
Source/include/Ably/ARTWrapperSDKProxyPush.h (1)
8-19: LGTM! Well-documented and properly designed interface.The interface is well-designed with:
- Clear documentation explaining purpose and usage restrictions
- Swift concurrency support
- Protocol conformance
- Prevention of direct initialization
- Proper encapsulation of admin functionality through readonly property
Source/include/Ably/AblyInternal.h (1)
8-13: LGTM! Logically organized imports.The new imports are well-organized and properly grouped with other wrapper SDK proxy imports, maintaining the file's purpose of providing access to internal APIs for Ably-authored SDKs.
Source/include/Ably/ARTWrapperSDKProxyRealtimeChannel.h (1)
4-6: LGTM! Well-structured wrapper class declarations and property updates.The changes properly introduce wrapper classes for push and presence functionality, ensuring consistent propagation of wrapper SDK agent information. The class hierarchy is well-organized and the documentation clearly indicates the intended usage.
Also applies to: 20-20, 23-23
Source/include/Ably/ARTWrapperSDKProxyPushAdmin.h (1)
1-24: LGTM! Well-structured new wrapper class for push admin functionality.The class is properly designed with:
- Clear documentation about its purpose and usage restrictions
- Appropriate Swift concurrency support (NS_SWIFT_SENDABLE)
- Correctly marked readonly properties
- Proper protocol conformance
Source/PrivateHeaders/Ably/ARTPushAdmin+Private.h (1)
17-17: LGTM! Consistent method signature update for wrapper SDK support.The publish method signature is properly updated to include the nullable wrapperSDKAgents parameter, maintaining consistency with other similar changes across the codebase.
Source/PrivateHeaders/Ably/ARTPushChannelSubscriptions+Private.h (1)
13-21: LGTM! Comprehensive and consistent method signature updates.All relevant methods have been properly updated to include the nullable wrapperSDKAgents parameter, ensuring consistent tracking of wrapper SDK usage across push channel subscription operations.
Source/PrivateHeaders/Ably/ARTPushDeviceRegistrations+Private.h (1)
13-21: LGTM! Consistent parameter additions across all methods.The changes consistently add the
wrapperSDKAgentsparameter to all methods, with proper nullability annotations. This aligns well with the PR objectives of attributing REST requests to wrapper SDKs.Source/PrivateHeaders/Ably/ARTRestChannel+Private.h (1)
6-6: LGTM! Consistent parameter addition and necessary import.The changes properly add the
wrapperSDKAgentsparameter to the history method and include the necessary import. The implementation maintains consistency with the rest of the codebase.Also applies to: 29-29
Source/ARTWrapperSDKProxyPushAdmin.m (2)
20-31: LGTM! Proper initialization and memory management.The initializer correctly sets up the proxy with proper memory management using readonly properties.
33-38: LGTM! Correct forwarding of wrapper SDK agents.The
publishmethod correctly forwards the wrapper SDK agents from the proxy options to the underlying implementation.Source/include/Ably/ARTRestChannel.h (1)
3-3: LGTM! Good architectural improvement.The change from importing
ARTChannel.htoARTChannelProtocol.hindicates a move towards better protocol-oriented design, improving modularity and reducing coupling.Source/ARTWrapperSDKProxyPushChannelSubscriptions.m (1)
1-56: LGTM! Well-structured proxy implementation.The implementation follows the proxy pattern effectively:
- Clean separation of concerns
- Consistent propagation of wrapper SDK agents
- Proper delegation to underlying implementation
Source/ARTWrapperSDKProxyPushDeviceRegistrations.m (1)
1-56: LGTM! Consistent proxy implementation.The implementation maintains consistency with other proxy classes:
- Follows the same proxy pattern
- Properly forwards wrapper SDK agents
- Clean error handling delegation
Source/ARTWrapperSDKProxyPushChannel.m (2)
27-29: Verify if listSubscriptions should include wrapper SDK agents.The
listSubscriptionsmethod doesn't pass wrapper SDK agents, unlike other methods in this class. Based on the PR objectives, some requests are exempted from wrapper SDK attribution. Please verify if this is intentional.
1-67: LGTM! Clean and consistent proxy implementation.The implementation:
- Follows proxy pattern best practices
- Properly handles both synchronous and asynchronous methods
- Maintains clean separation of concerns
Source/ARTWrapperSDKProxyPush.m (5)
1-26: LGTM! Well-structured initialization.The private interface and initialization are well-implemented with proper memory management and encapsulation.
30-36: LGTM! Correctly handles push activation exception.The implementation correctly forwards activation calls without adding wrapper SDK agent, aligning with PR objectives that list push activation as an exception.
1-14: LGTM! Well-structured private interface.The private interface is well-defined with readonly properties for the underlying push instance and proxy options.
17-26: LGTM! Proper initialization.The initializer correctly sets up the proxy by:
- Initializing the underlying push instance
- Setting up proxy options
- Creating the admin proxy with proper dependencies
30-36: LGTM! Clean activation methods.The activate/deactivate methods properly delegate to the underlying push instance.
Source/include/Ably/ARTChannelProtocol.h (2)
15-17: LGTM! Documentation improvement.The updated documentation clearly explains the protocol hierarchy.
15-17: LGTM! Improved documentation clarity.The updated documentation better explains the protocol hierarchy by explicitly stating that
ARTRestChannelProtocolandARTRealtimeChannelProtocolare based on this protocol.Source/ARTWrapperSDKProxyRealtime.m (3)
27-28: LGTM! Correctly implements wrapper SDK attribution.The changes properly integrate wrapper SDK agents into REST requests while maintaining the exceptions specified in PR objectives.
Also applies to: 81-96
27-28: LGTM! Proper push initialization.The push proxy is correctly initialized with the underlying push instance and proxy options.
82-84: LGTM! Consistent SDK agents propagation.The methods correctly propagate wrapper SDK agents to the underlying implementations:
statsmethodstimemethod
All following the PR's objective to attribute REST requests to wrapper SDK.Also applies to: 87-91, 94-96
Source/ARTPushAdmin.m (3)
23-25: LGTM! Well-implemented push notification publishing.The changes correctly integrate wrapper SDK agents while maintaining robust error handling for missing parameters.
Also applies to: 56-95
23-25: LGTM! Clean public interface.The public method correctly delegates to the internal implementation with nil wrapper SDK agents, maintaining backward compatibility.
56-95: LGTM! Proper wrapper SDK agents handling.The internal implementation:
- Correctly accepts wrapper SDK agents parameter
- Maintains existing error handling
- Properly propagates agents to the REST request
Source/PrivateHeaders/Ably/ARTRealtimePresence+Private.h (1)
70-73: LGTM! Method signatures updated consistently.The addition of the
wrapperSDKAgentsparameter to the history methods is consistent with the PR objectives and follows the established pattern across the codebase.Source/Ably.modulemap (1)
118-124: LGTM! Headers added for wrapper SDK proxy implementation.The new headers are properly organized within the Private module and follow consistent naming patterns, supporting the wrapper SDK proxy functionality.
Source/PrivateHeaders/Ably/ARTRealtime+Private.h (1)
48-65: LGTM! Method signatures updated consistently.The addition of the
wrapperSDKAgentsparameter to time and stats methods is consistent with the PR objectives and follows the established pattern across the codebase.Source/PrivateHeaders/Ably/ARTRest+Private.h (1)
69-115: LGTM! Method signatures updated consistently.The addition of the
wrapperSDKAgentsparameter to time and stats methods is consistent with the PR objectives and follows the established pattern across the codebase.Source/ARTChannel.m (1)
179-181: LGTM!The abstract method signature is correctly defined with the new
wrapperSDKAgentsparameter, and the assertion ensures proper implementation by subclasses.Source/ARTWrapperSDKProxyRealtimeChannel.m (3)
2-5: LGTM!The new imports are necessary for accessing the private interfaces of the proxy classes.
24-29: LGTM!The initialization of proxy objects is well-structured:
- Push channel initialization is correctly guarded for iOS targets
- Presence initialization properly wraps the underlying channel's presence
57-58: LGTM!The history method calls correctly propagate the wrapper SDK agents from proxyOptions, maintaining the attribution of REST requests.
Also applies to: 118-118
Source/ARTPaginatedResult.m (2)
43-86: LGTM!The initializer correctly stores the wrapperSDKAgents parameter, maintaining the SDK attribution context throughout the object's lifecycle.
146-182: LGTM!The executePaginated method properly:
- Accepts the wrapperSDKAgents parameter
- Passes it through to the executeRequest method
- Includes it in the creation of new paginated results
Source/ARTRestPresence.m (2)
79-85: LGTM!The public history methods correctly pass nil for wrapperSDKAgents, as they are not part of the wrapper SDK interface.
164-167: LGTM!The new historyWithWrapperSDKAgents method properly handles the SDK attribution parameter.
Source/ARTPushChannelSubscriptions.m (1)
26-44: LGTM! Consistent handling of wrapperSDKAgents parameter.The public methods in
ARTPushChannelSubscriptionsconsistently passnilfor thewrapperSDKAgentsparameter to their internal counterparts, maintaining a clean public API while supporting the new functionality internally.Source/ARTPushChannel.m (1)
26-62: LGTM! Consistent handling of wrapperSDKAgents parameter.The public methods in
ARTPushChannelconsistently passnilfor thewrapperSDKAgentsparameter to their internal counterparts, maintaining a clean public API while supporting the new functionality internally.Source/ARTPushDeviceRegistrations.m (1)
27-45: LGTM! Consistent handling of wrapperSDKAgents parameter.The public methods in
ARTPushDeviceRegistrationsconsistently passnilfor thewrapperSDKAgentsparameter to their internal counterparts, maintaining a clean public API while supporting the new functionality internally.Test/Tests/WrapperSDKProxyTests.swift (2)
322-353: LGTM! Well-structured parameterized test helper.The
parameterizedTest_addsWrapperSDKAgentToRequestshelper method effectively reduces code duplication while maintaining test clarity. It handles common setup and verification logic, making individual test cases concise and focused.
355-518: LGTM! Comprehensive test coverage for wrapper SDK agent functionality.The test suite thoroughly verifies that the wrapper SDK agent is correctly added to various types of requests:
- Time requests
- History requests with pagination
- Stats requests
- Push admin requests
- Channel subscription requests
- Device registration requests
- Presence history requests
The tests also handle edge cases and verify error scenarios appropriately.
Source/ARTRest.m (5)
530-544: LGTM! Wrapper SDK agent support added to time method.The implementation correctly delegates to the internal method while maintaining the user queue callback pattern.
546-568: LGTM! Internal time method implementation with wrapper SDK agent support.The implementation:
- Correctly constructs the request URL
- Properly sets the Accept header
- Executes the request with authentication off (as per PR objectives for time requests)
570-667: LGTM! Request method updated to support wrapper SDK agents.The implementation correctly:
- Validates method, body, and path
- Constructs URL with query parameters
- Sets headers and content type
- Executes paginated response with wrapper SDK agents
684-687: LGTM! Stats method updated to support wrapper SDK agents.The implementation correctly delegates to the internal method with wrapper SDK agents.
689-735: LGTM! Internal stats method implementation with wrapper SDK agent support.The implementation correctly:
- Validates query parameters
- Constructs request URL
- Executes paginated result with wrapper SDK agents
Source/ARTAuth.m (1)
711-712: LGTM! Time method call updated to use wrapper SDK agents.The implementation correctly passes nil for wrapperSDKAgents as per PR objectives for token requests.
Source/ARTRealtimePresence.m (2)
275-278: LGTM! History method updated to support wrapper SDK agents.The implementation correctly delegates to the internal method with wrapper SDK agents.
280-283: LGTM! Internal history method implementation with wrapper SDK agent support.The implementation correctly:
- Sets realtime channel on query
- Delegates to rest channel's presence history with wrapper SDK agents
Source/ARTRealtimeChannel.m (2)
1046-1049: LGTM! History method updated to support wrapper SDK agents.The implementation correctly delegates to the internal method with wrapper SDK agents.
1051-1054: LGTM! Internal history method implementation with wrapper SDK agent support.The implementation correctly:
- Sets realtime channel on query
- Delegates to rest channel's history with wrapper SDK agents
Test/Tests/AuthTests.swift (1)
2030-2032: LGTM! Method signature updates for wrapper SDK attribution.The changes consistently update the test hooks to use the new
_time(withWrapperSDKAgents:completion:)method signature, aligning with the PR's objective of attributing REST requests to wrapper SDK agents.Also applies to: 2174-2176, 2779-2781
Ably.xcodeproj/project.pbxproj (3)
211-264: LGTM! Well-organized wrapper SDK proxy implementation for push functionality.The new push-related wrapper SDK proxy files are properly structured with clear separation of public and private headers. The implementation follows the proxy pattern and maintains consistency across all targets.
Also applies to: 1298-1315
256-264: LGTM! Clean implementation of realtime presence wrapper.The realtime presence wrapper SDK proxy files are well-organized with proper public/private header separation.
Also applies to: 1313-1315
754-754: LGTM! Proper access control for ARTChannel header.The
ARTChannel.his correctly marked as a private header, maintaining proper encapsulation.Also applies to: 772-772, 962-962
Continuation of #2020. Adds wrapper SDK agent to all HTTP requests initiated by a wrapper SDK proxy client, except for:
I'm going to update ably/specification#273 to describe the behaviour implemented here.
Resolves #2029.
Summary by CodeRabbit
These improvements help ensure a smoother, more adaptable experience in managing real‑time messaging and notifications.