Added attachOnSubscribe to ARTRealtimeChannelOptions.

[maratal]

Closes #1972

Summary by CodeRabbit

• New Features

  • Introduced a new option attachOnSubscribe in channel options to control implicit attachment behavior during subscription.

• Bug Fixes

  • Improved error handling for channel subscriptions in a Failed state based on the attachOnSubscribe setting.

• Documentation

  • Updated documentation to clarify the conditions under which the attach method is implicitly invoked and the behavior of the attachOnSubscribe option.

• Tests

  • Enhanced test coverage for subscription behavior related to the attachOnSubscribe option, including new tests for both attachment and error scenarios.

[coderabbitai]

Walkthrough

The changes implemented in this pull request introduce a new attachOnSubscribe option within the ARTRealtimeChannelOptions class, affecting the subscription behavior of the ARTRealtimeChannel and ARTRealtimePresence classes. This option determines whether a channel should automatically attach when subscribed, allowing for more flexible control over channel state management. The modifications include updates to method logic, the introduction of new properties, and enhancements to test cases to validate the new functionality.

Changes

Files	Change Summary
Source/ARTRealtimeChannel.m	Modified _subscribe:onAttach: method to incorporate attachOnSubscribe logic for subscription.
Source/ARTRealtimeChannelOptions.m	Added _attachOnSubscribe property with getter and setter methods to manage attachment behavior.
Source/ARTRealtimePresence.m	Updated _subscribe:onAttach: method to use attachOnSubscribe for controlling subscription flow.
Source/include/Ably/ARTRealtimeChannelOptions.h	Introduced attachOnSubscribe property to manage implicit attachment during subscription.
Test/Tests/RealtimeClientChannelTests.swift	Renamed and added tests to validate behavior based on attachOnSubscribe option.
Test/Tests/RealtimeClientPresenceTests.swift	Renamed and added tests to verify presence subscription behavior with respect to attachOnSubscribe.

Assessment against linked issues

Objective	Addressed	Explanation
Implement the attachOnSubscribe channel option (TB4) (#1972)	✅

Possibly related PRs

• [ECO-4940] NS_SWIFT_SENDABLE to every type that has a docstring #1963: This PR adds the attachOnSubscribe property to ARTRealtimeChannelOptions, directly related to the subscription logic changes in this PR.

Poem

In a channel where options play,
A rabbit hops and finds the way.
With attachOnSubscribe, oh what a sight,
Subscriptions dance in the soft moonlight.
Flexibility blooms, like flowers in spring,
Hooray for the changes that this PR brings! 🐇✨

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

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

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

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

CodeRabbit Configuration File (.coderabbit.yaml)

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

Documentation and Community

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

[coderabbitai]

Actionable comments posted: 4

Outside diff range and nitpick comments (5)

Source/include/Ably/ARTRealtimeChannelOptions.h (1)

45-49: LGTM! Consider a minor documentation enhancement.

The new attachOnSubscribe property is well-implemented and properly documented. It aligns with the PR objectives and the Ably specification requirement.

Consider adding a brief explanation of the implications when this property is set to false. This would provide developers with a clearer understanding of when they might want to change the default behavior. Here's a suggested addition:

 /**
  * Determines whether calling `subscribe` on a channel or presence object should trigger an implicit attach. Defaults to `true`.
+ * When set to `false`, developers must manually attach the channel before subscribing if they want to ensure the channel is attached.
  */
 @property (nonatomic) BOOL attachOnSubscribe;

Source/ARTRealtimeChannelOptions.m (2)

50-61: LGTM: Getter and setter methods implemented correctly.

The attachOnSubscribe getter and setAttachOnSubscribe: setter methods have been implemented correctly. They follow the established patterns in the class, including the frozen state check in the setter.

For consistency with other properties in this class, consider declaring attachOnSubscribe as a property in the header file (.h) with the readwrite attribute. This would make the API more uniform:

@property (nonatomic, readwrite) BOOL attachOnSubscribe;

This change would maintain the current implementation while providing a more consistent public interface.

---

Implementation verified, but tests are missing

The implementation of the attachOnSubscribe option in ARTRealtimeChannelOptions.m has been verified and is consistent with the code provided in the original review comment. Additionally, the option is being properly used in ARTRealtimeChannel.m and ARTRealtimePresence.m, indicating correct integration into the existing codebase.

However, there is a significant concern:

• No test files were found that reference the attachOnSubscribe option, suggesting that this new functionality may not be adequately tested.

To ensure the reliability and correctness of this new feature, it is strongly recommended to:

1. Add unit tests specifically for the attachOnSubscribe option in ARTRealtimeChannelOptions.
2. Create integration tests that verify the behavior of attachOnSubscribe in ARTRealtimeChannel and ARTRealtimePresence.

Analysis chain

Line range hint 1-62: Overall implementation looks good and meets the PR objectives.

The changes to ARTRealtimeChannelOptions.m successfully implement the attachOnSubscribe option as required. The new instance variable, initializer updates, and accessor methods are all implemented correctly and consistently with the existing codebase. These changes provide the necessary functionality to control channel attachment behavior during subscription, which is crucial for improving chat application performance as outlined in the PR objectives.

To ensure that these changes are properly integrated and don't introduce any regressions, please run the following verification script:

This script will help verify the proper integration of the new option and highlight any areas that might need additional attention or testing.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation and usage of attachOnSubscribe option

# Test 1: Check if the new property is declared in the header file
echo "Checking for attachOnSubscribe property declaration:"
rg --type objc 'property.*attachOnSubscribe' Source/ARTRealtimeChannelOptions.h

# Test 2: Verify usage of attachOnSubscribe in other parts of the codebase
echo "Checking for attachOnSubscribe usage:"
rg --type objc 'attachOnSubscribe'

# Test 3: Look for any TODO comments related to attachOnSubscribe
echo "Checking for related TODO comments:"
rg --type objc 'TODO.*attachOnSubscribe'

# Test 4: Ensure test cases have been added or updated
echo "Checking for test cases:"
rg --type objc 'test.*attachOnSubscribe' Spec/

Length of output: 1730

---

Script:

#!/bin/bash
# Verify directory structure and file locations

echo "Checking directory structure:"
find . -type d -maxdepth 2

echo "\nChecking for ARTRealtimeChannelOptions.h:"
find . -name "ARTRealtimeChannelOptions.h"

echo "\nChecking for test directories:"
find . -type d -name "*[tT]est*" -o -name "*[sS]pec*"

echo "\nChecking for test files related to attachOnSubscribe:"
find . -type f -name "*[tT]est*.m" -o -name "*[sS]pec*.m" | xargs grep -i "attachOnSubscribe"

Length of output: 1602

---

Script:

#!/bin/bash
# Search for test files related to attachOnSubscribe

echo "Checking for test files in ./Test directory:"
find ./Test -type f \( -name "*[tT]est*.m" -o -name "*[sS]pec*.m" \) -print0 | xargs -0 grep -i "attachOnSubscribe"

echo "\nChecking for test files in ./Examples directory:"
find ./Examples -type f \( -name "*[tT]est*.m" -o -name "*[sS]pec*.m" \) -print0 | xargs -0 grep -i "attachOnSubscribe"

echo "\nChecking for usage of attachOnSubscribe in all .m files:"
find . -name "*.m" -type f -print0 | xargs -0 grep -i "attachOnSubscribe"

Length of output: 1721

Test/Tests/RealtimeClientChannelTests.swift (2)

3361-3361: Replace unused closure parameter errorInfo with _ to silence warning

The closure parameter errorInfo is unused in the attachCallback closure on line 3361. Replacing it with _ will address the compiler warning about unused parameters.

Apply this diff to fix the issue:

-            channel.subscribe(attachCallback: { errorInfo in
+            channel.subscribe(attachCallback: { _ in

Tools

SwiftLint

[Warning] 3361-3361: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

---

3405-3405: Replace unused closure parameter errorInfo with _ to silence warning

The closure parameter errorInfo is unused in the attachCallback closure on line 3405. Replacing it with _ will address the compiler warning about unused parameters.

Apply this diff to fix the issue:

-            channel.subscribe(attachCallback: { errorInfo in
+            channel.subscribe(attachCallback: { _ in

Tools

SwiftLint

[Warning] 3405-3405: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 80f05fd and d15fcc8.

Files selected for processing (6)

• Source/ARTRealtimeChannel.m (1 hunks)
• Source/ARTRealtimeChannelOptions.m (2 hunks)
• Source/ARTRealtimePresence.m (2 hunks)
• Source/include/Ably/ARTRealtimeChannelOptions.h (1 hunks)
• Test/Tests/RealtimeClientChannelTests.swift (3 hunks)
• Test/Tests/RealtimeClientPresenceTests.swift (3 hunks)

Additional context used

SwiftLint

Test/Tests/RealtimeClientChannelTests.swift

[Warning] 3361-3361: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

---

[Warning] 3405-3405: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

Test/Tests/RealtimeClientPresenceTests.swift

[Warning] 927-927: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

---

[Warning] 969-969: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

Additional comments not posted (15)

Source/ARTRealtimeChannelOptions.m (3)

7-7: LGTM: New instance variable added correctly.

The addition of the _attachOnSubscribe instance variable is correct and aligns with the PR objectives. The naming convention and type are appropriate for this kind of option.

---

10-15: LGTM: Default value set correctly in initializer.

The init method has been properly updated to set the default value of _attachOnSubscribe to true. This maintains backwards compatibility and follows good initialization practices.

---

17-21: LGTM: Consistent initialization in initWithCipher: method.

The initWithCipher: method has been correctly updated to set the default value of _attachOnSubscribe to true, maintaining consistency with the init method. This ensures that the new option is properly initialized regardless of which initializer is used.

Source/ARTRealtimeChannel.m (3)

Line range hint 1-1000: Overall assessment of changes

The changes in this file are focused and minimal, which is positive for maintainability. The implementation of the attachOnSubscribe option in the _subscribe:onAttach:callback: method appears to be correct and aligns with the PR objectives.

Key points:

1. The new option is implemented without disrupting the existing code structure.
2. Backward compatibility is maintained with the default value of true.
3. The change is localized, reducing the risk of unintended side effects.

Recommendations:

1. Add test cases to cover the new behavior, especially when attachOnSubscribe is set to false.
2. Update the public API documentation to reflect this new option.
3. Consider adding a brief comment in the code explaining the purpose of the attachOnSubscribe option for future maintainers.

---

438-443: Consider backward compatibility and edge cases

While the implementation looks correct, there are a few points to consider:

1. Backward Compatibility: The default value of true for attachOnSubscribe maintains backward compatibility with the previous behavior. However, we should ensure that existing code using this method won't be affected negatively.

2. Edge Cases: Consider what happens if the channel is already in a FAILED state and attachOnSubscribe is true. The current implementation seems to handle this case correctly by calling the onAttach callback with an error.

3. Documentation: Ensure that the public API documentation is updated to reflect this new option and its behavior.

Consider adding a test case that specifically checks the behavior when attachOnSubscribe is set to false to ensure it works as expected.

Let's check if the documentation has been updated:

#!/bin/bash
# Check for updates in documentation files
rg "attachOnSubscribe" --type markdown

Also applies to: 447-447

---

438-443: Implement attachOnSubscribe option

The implementation of the attachOnSubscribe option looks good. Here's a breakdown of the changes:

1. The attachOnSubscribe value is correctly retrieved from the channel options.
2. The default value is set to true when options are not available, which aligns with the expected behavior.
3. The attachOnSubscribe flag is used to conditionally execute the attachment and callback logic.

These changes successfully implement the attachOnSubscribe channel option as per the PR objectives.

To ensure this change doesn't affect other parts of the codebase, let's verify the usage of attachOnSubscribe:

Also applies to: 447-447

Verification successful

---

Implementation of attachOnSubscribe is consistent and correct

After reviewing the usage of attachOnSubscribe across the codebase, I can confirm that the implementation is consistent and correct. Key observations:

• The attachOnSubscribe property is properly defined in ARTRealtimeChannelOptions with a default value of true.
• Its usage in ARTRealtimeChannel.m and ARTRealtimePresence.m is consistent, following the same pattern we initially reviewed.
• There are no conflicting or unexpected uses of this option in other parts of the codebase.

The changes implement the attachOnSubscribe channel option as intended, maintaining consistency throughout the SDK.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any other occurrences of attachOnSubscribe
rg "attachOnSubscribe" --type objc

Length of output: 118

---

Script:

#!/bin/bash
# Search for "attachOnSubscribe" in Objective-C files
rg "attachOnSubscribe" --type objc -n

Length of output: 1142

Source/ARTRealtimePresence.m (4)

19-19: Correctly imported ARTRealtimeChannelOptions.h

The addition of #import "ARTRealtimeChannelOptions.h" is necessary to access the ARTRealtimeChannelOptions class used in the new implementation.

---

501-502: Retrieving attachOnSubscribe option with default value

The implementation correctly retrieves the attachOnSubscribe option from the channel's options, defaulting to true when options are not set.

---

504-506: Potential issue: onAttach callback may not be invoked when attachOnSubscribe is false

When attachOnSubscribe is set to false and the channel is in a Failed state, the onAttach callback is not called. This might lead to unexpected behavior if the caller relies on onAttach for error handling or state updates. Please verify whether onAttach should be invoked regardless of the attachOnSubscribe value to ensure consistent callback handling.

---

510-510: Conditional attachment respects attachOnSubscribe option

The condition correctly checks attachOnSubscribe along with shouldAttach to decide whether to attach the channel, aligning with the new option's intended behavior.

Test/Tests/RealtimeClientPresenceTests.swift (1)

Line range hint 889-910: Test case correctly verifies implicit channel attachment

The test test__026__Presence__subscribe__should_implicitly_attach_the_channel_if_options_attachOnSubscribe_is_true() accurately checks that the channel implicitly attaches when attachOnSubscribe is set to true during presence subscription across various channel states.

Test/Tests/RealtimeClientChannelTests.swift (4)

Line range hint 3317-3342: Test correctly verifies implicit attachment when attachOnSubscribe is true

The test method test__109__Channel__subscribe__should_implicitly_attach_the_channel_if_options_attachOnSubscribe_is_true accurately checks that subscribing to a channel with attachOnSubscribe set to true results in the channel being implicitly attached. The assertions confirm the expected state transitions from initialized to attaching to attached.

---

3344-3368: Test correctly verifies that attachOnSubscribe set to false prevents implicit attachment

The test method test__109b__Channel__subscribe__should_not_implicitly_attach_the_channel_if_options_attachOnSubscribe_is_false confirms that when attachOnSubscribe is false, subscribing to a channel does not cause it to attach implicitly. The assertions ensure the channel remains in the initialized state, verifying that no unintended state transitions occur.

Tools

SwiftLint

[Warning] 3361-3361: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

---

3393-3408: Test correctly verifies error handling when subscribing with attachOnSubscribe true in FAILED state

The test method test__110__Channel__subscribe__should_result_in_an_error_if_channel_is_in_the_FAILED_state_and_options_attachOnSubscribe_is_true appropriately checks that an error is received when attempting to subscribe to a channel in the FAILED state with attachOnSubscribe set to true, ensuring proper error handling in this scenario.

Tools

SwiftLint

[Warning] 3405-3405: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

---

3395-3412: Test correctly verifies no error when subscribing with attachOnSubscribe false in FAILED state

The test method test__110b__Channel__subscribe__should_not_result_in_an_error_if_channel_is_in_the_FAILED_state_and_options_attachOnSubscribe_is_false confirms that when attachOnSubscribe is false, subscribing to a channel in the FAILED state does not result in an error and the channel remains in the FAILED state, as expected.

Tools

SwiftLint

[Warning] 3405-3405: Unused parameter in a closure should be replaced with _

(unused_closure_parameter)

[coderabbitai]

Actionable comments posted: 0

Outside diff range and nitpick comments (1)

Source/include/Ably/ARTRealtimeChannel.h (1)

48-50: Approve documentation update with a minor suggestion.

The updated documentation for the attach: method provides clearer information about when the method is implicitly called, including the new attachOnSubscribe option. This change aligns well with the PR objective of implementing the attachOnSubscribe channel option.

To further improve clarity, consider restructuring the sentence slightly:

* As a convenience, `attach:` is called implicitly in the following cases:
* 1. When `-[ARTRealtimePresenceProtocol enter:]` is called.
* 2. When `-[ARTRealtimeChannelProtocol subscribe:]` for the channel or `-[ARTRealtimePresenceProtocol subscribe:]` on the `ARTRealtimePresence` object for this channel are called, unless the `-[ARTRealtimeChannelOptions attachOnSubscribe]` option is set to `false`.

This structure makes it easier to distinguish between the different conditions for implicit attach: calls.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 224edb8 and 1625e1b.

Files selected for processing (7)

• Source/ARTRealtimeChannel.m (1 hunks)
• Source/ARTRealtimeChannelOptions.m (2 hunks)
• Source/ARTRealtimePresence.m (2 hunks)
• Source/include/Ably/ARTRealtimeChannel.h (1 hunks)
• Source/include/Ably/ARTRealtimeChannelOptions.h (1 hunks)
• Test/Tests/RealtimeClientChannelTests.swift (4 hunks)
• Test/Tests/RealtimeClientPresenceTests.swift (3 hunks)

Files skipped from review as they are similar to previous changes (6)

• Source/ARTRealtimeChannel.m
• Source/ARTRealtimeChannelOptions.m
• Source/ARTRealtimePresence.m
• Source/include/Ably/ARTRealtimeChannelOptions.h
• Test/Tests/RealtimeClientChannelTests.swift
• Test/Tests/RealtimeClientPresenceTests.swift

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)

Source/include/Ably/ARTRealtimePresence.h (2)

Line range hint 149-153: LGTM! Consider adding a link to ARTRealtimeChannelOptions.

The added documentation clearly explains the behavior of the onAttach callback in relation to the attachOnSubscribe option. This aligns well with the PR objectives and provides valuable information to developers.

Consider adding a link to the ARTRealtimeChannelOptions documentation for easier reference:

- * called if the `ARTRealtimeChannelOptions.attachOnSubscribe` channel option is set to `false`.
+ * called if the `ARTRealtimeChannelOptions.attachOnSubscribe` channel option is set to `false`.
+ * @see ARTRealtimeChannelOptions

---

Line range hint 169-173: LGTM! Consider adding a link to ARTRealtimeChannelOptions.

The added documentation for this method overload is consistent with the previous change and provides the same valuable information about the onAttach callback behavior.

As suggested for the previous comment, consider adding a link to the ARTRealtimeChannelOptions documentation:

- * called if the `ARTRealtimeChannelOptions.attachOnSubscribe` channel option is set to `false`.
+ * called if the `ARTRealtimeChannelOptions.attachOnSubscribe` channel option is set to `false`.
+ * @see ARTRealtimeChannelOptions

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 2ad0f16 and e7ab691.

📒 Files selected for processing (8)

• Source/ARTRealtimeChannel.m (1 hunks)
• Source/ARTRealtimeChannelOptions.m (2 hunks)
• Source/ARTRealtimePresence.m (2 hunks)
• Source/include/Ably/ARTRealtimeChannel.h (3 hunks)
• Source/include/Ably/ARTRealtimeChannelOptions.h (1 hunks)
• Source/include/Ably/ARTRealtimePresence.h (2 hunks)
• Test/Tests/RealtimeClientChannelTests.swift (4 hunks)
• Test/Tests/RealtimeClientPresenceTests.swift (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (7)

• Source/ARTRealtimeChannel.m
• Source/ARTRealtimeChannelOptions.m
• Source/ARTRealtimePresence.m
• Source/include/Ably/ARTRealtimeChannel.h
• Source/include/Ably/ARTRealtimeChannelOptions.h
• Test/Tests/RealtimeClientChannelTests.swift
• Test/Tests/RealtimeClientPresenceTests.swift

🔇 Additional comments (1)

Source/include/Ably/ARTRealtimePresence.h (1)

Line range hint 1-238: Overall, the documentation changes look good and align with the PR objectives.

The updates to the method documentation in this file provide clear and valuable information about the new attachOnSubscribe option. This aligns well with the PR objectives of implementing this feature. The changes are consistent across both affected methods and will help developers understand the behavior of the onAttach callback in relation to the new option.