Unpin ably-cocoa #215
Unpin ably-cocoa #215
Conversation
We won't be able to do this; see message of ably-cocoa commit e216ae5. The approach currently taken by the Chat SDK (adding these properties and methods onto its own Swift protocol that uses associated types) is the one that we should continue.
WalkthroughThis pull request introduces several significant changes to the AblyChat project, primarily focusing on dependency updates and type system modifications. The main updates include upgrading the Changes
Suggested reviewers
Possibly related PRs
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/215/AblyChat
lawrence-forooghian
force-pushed
the
unpin-ably-cocoa
branch
from
f686e7a to
effd502
Compare
github-actions
bot
temporarily deployed
to
staging/pull/215/AblyChat
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (1)
4-98: Add documentation for the mock class.Consider adding documentation that explains:
- The purpose of this mock class
- When and how it should be used in tests
- Why methods are intentionally unimplemented
Add documentation like this:
+/// A mock implementation of `RealtimePresenceProtocol` for testing purposes. +/// All methods intentionally throw `fatalError` to ensure tests properly mock +/// only the methods they need. final class MockRealtimePresence: RealtimePresenceProtocol {
📜 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/Mocks/MockClients.swift(4 hunks)Example/AblyChatExample/Mocks/MockRealtime.swift(2 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift(1 hunks)Sources/AblyChat/Dependencies.swift(1 hunks)Sources/AblyChat/Messages.swift(1 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Sources/AblyChat/Room.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(2 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/Typing.swift(1 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Package.resolved
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: SPM (Xcode 16)
- GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16)
🔇 Additional comments (15)
Package.swift (1)
23-23: Verify the impact of unpinning ably-cocoa dependency.While unpinning the dependency allows for easier updates, we should verify:
- Breaking changes between 1.2.36 and 1.2.37
- Known issues with newer versions
✅ Verification successful
✅ Safe to unpin ably-cocoa dependency to 1.2.37+
Version 1.2.37 actually reverts a breaking change by moving
presenceback toARTRealtime, making it more stable. Recent version history shows careful maintenance with no concerning changes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for breaking changes and issues in ably-cocoa # Get the changelog between versions gh api repos/ably/ably-cocoa/releases | jq -r '.[] | select(.tag_name | contains("1.2.3")) | .body' # Check for open issues affecting version range gh api repos/ably/ably-cocoa/issues --jq '.[] | select(.labels[].name | contains("bug")) | {title: .title, url: .html_url, labels: [.labels[].name]}'Length of output: 5340
Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift (1)
9-10: LGTM! Protocol conformance follows established patterns.The extension follows the same pattern as other protocol conformances in this file, maintaining consistency.
Tests/AblyChatTests/Mocks/MockFeatureChannel.swift (1)
5-5: Type system changes align with Swift's direction.The shift to existential types (
any RealtimeChannelProtocol) is consistent with Swift's evolution. However, be aware that this might affect:
- Type inference in dependent code
- Compile-time protocol conformance checks
Also applies to: 10-10
✅ Verification successful
Change to use existential types is consistent with codebase patterns.
The modification to use
any RealtimeChannelProtocolin MockFeatureChannel.swift aligns with the existing usage pattern throughout the codebase, where protocol conformance is consistently handled using existential types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other usages that might need similar updates # Find other RealtimeChannelProtocol usages that might need updating rg "RealtimeChannelProtocol" --type swift -l | xargs rg ":" -A 1 --type swiftLength of output: 106046
Sources/AblyChat/Occupancy.swift (1)
37-37: LGTM! Consistent with project-wide type system changes.The update to use
any RealtimeChannelProtocolmaintains consistency with similar changes across the codebase.AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
9-10: Verify ably-cocoa version 1.2.37 for compatibility and security.The dependency upgrade from 1.2.36 to 1.2.37 appears to be a minor patch version increment.
Let's verify the version and check for any security advisories:
✅ Verification successful
Version 1.2.37 verified successfully
The version exists in the official repository and no security vulnerabilities were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the version exists in the repository gh api \ -H "Accept: application/vnd.github+json" \ repos/ably/ably-cocoa/git/refs/tags/v1.2.37 # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: COCOAPODS, package: "Ably") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1030
Script:
#!/bin/bash # List recent tags gh api \ -H "Accept: application/vnd.github+json" \ repos/ably/ably-cocoa/tags?per_page=5 # Check security advisories with correct ecosystem value gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: SWIFT, package: "Ably") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 2540
Sources/AblyChat/Typing.swift (1)
57-57: LGTM! Good use of existential type.The change to
any RealtimeChannelProtocolaligns with modern Swift best practices for protocol conformance.Sources/AblyChat/Dependencies.swift (2)
22-26: LGTM! Well-structured protocol hierarchy.The addition of the presence-related associated type and property to
RealtimeChannelProtocolfollows good protocol-oriented design principles.
28-29: LGTM! Good protocol design.The new
RealtimePresenceProtocolproperly extendsARTRealtimePresenceProtocolwithSendableconformance for thread safety.Sources/AblyChat/RoomReactions.swift (1)
25-25: LGTM! Good use of explicit existential types.The change to
any RealtimeChannelProtocolaligns with modern Swift best practices for protocol usage. This makes the existential type usage explicit and clearer.Sources/AblyChat/RoomFeature.swift (1)
49-49: LGTM! Consistent type system updates.The changes to use
any RealtimeChannelProtocolin both the protocol and implementation are consistent with the codebase-wide update to explicit existential types.Also applies to: 65-65
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
5-5: LGTM! Enhanced test coverage with mock presence implementation.Replacing the unimplemented presence feature with a concrete
MockRealtimePresenceimplementation improves the testing capabilities of the codebase.Sources/AblyChat/Messages.swift (1)
54-54: LGTM! Consistent type system update.The change to use
any RealtimeChannelProtocolmaintains consistency with the codebase-wide update to explicit existential types.Example/AblyChatExample/Mocks/MockRealtime.swift (1)
108-108: LGTM! Improved mock implementation.The change replaces a
fatalErrorimplementation with a proper mock instance, which is a better approach for testing.Example/AblyChatExample/Mocks/MockClients.swift (1)
91-91: LGTM! Improved type safety with explicit existential types.The changes from
RealtimeChannelProtocoltoany RealtimeChannelProtocolalign with Swift's best practices for explicit protocol type usage.Also applies to: 149-149, 196-196, 359-359
Sources/AblyChat/Room.swift (1)
303-303: LGTM! Consistent use of explicit existential types.The change from
RealtimeChannelProtocoltoany RealtimeChannelProtocolmaintains consistency with similar changes across the codebase.
This reverts b261155. Handle ably-cocoa's new commit e216ae5, which reverted the backwards compatibility-breaking commit 26d9bf7. To handle e216ae5, we need to add the realtime client's `presence` member to our requirements.
lawrence-forooghian
force-pushed
the
unpin-ably-cocoa
branch
from
effd502 to
a339784
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Example/AblyChatExample/Mocks/MockRealtime.swift (1)
251-345: 🛠️ Refactor suggestionEnhance mock implementations in
RealtimePresenceThe
RealtimePresenceclass methods currently raisefatalError("Not implemented"), except for thehistory(_:)method, which has an empty implementation. This inconsistency could lead to unexpected behavior during testing. Consider providing basic mock implementations for these methods to support more effective testing scenarios.
🧹 Nitpick comments (1)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (1)
Line range hint
1-4: Consider adding documentation for mock usage patterns.While the implementation is solid, consider adding:
- Class-level documentation explaining which methods are implemented vs unimplemented
- Usage examples in comments
- Explanation of thread-safety decisions
📜 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/Mocks/MockClients.swift(4 hunks)Example/AblyChatExample/Mocks/MockRealtime.swift(2 hunks)Package.resolved(1 hunks)Package.swift(1 hunks)Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift(1 hunks)Sources/AblyChat/Dependencies.swift(1 hunks)Sources/AblyChat/Messages.swift(1 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Sources/AblyChat/Room.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(2 hunks)Sources/AblyChat/RoomReactions.swift(1 hunks)Sources/AblyChat/Typing.swift(1 hunks)Tests/AblyChatTests/Mocks/MockFeatureChannel.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- Sources/AblyChat/AblyCocoaExtensions/Ably+Dependencies.swift
- Sources/AblyChat/Occupancy.swift
- Sources/AblyChat/Messages.swift
- Package.swift
- Sources/AblyChat/RoomReactions.swift
- Sources/AblyChat/Typing.swift
- Sources/AblyChat/Room.swift
- Sources/AblyChat/RoomFeature.swift
- Sources/AblyChat/Dependencies.swift
- Tests/AblyChatTests/Mocks/MockFeatureChannel.swift
- AblyChat.xcworkspace/xcshareddata/swiftpm/Package.resolved
- Example/AblyChatExample/Mocks/MockClients.swift
- Package.resolved
- Tests/AblyChatTests/Mocks/MockRealtimePresence.swift
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: SPM (Xcode 16)
- GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: check-documentation
🔇 Additional comments (6)
Example/AblyChatExample/Mocks/MockRealtime.swift (1)
108-108: Presence property correctly initializedThe
presenceproperty in theChannelclass is now initialized with an instance ofRealtimePresence(), which aligns with the protocol requirements and enhances functionality.Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (5)
Line range hint
14-16: Review thread-safety implications ofnonisolated(unsafe).While the comment suggests that
nonisolated(unsafe)is acceptable for tests, consider using a thread-safe approach (like theCounterclass implementation) to prevent potential race conditions in concurrent test scenarios.
Line range hint
91-104: LGTM! Improved error handling in attach method.The guard statement provides better error messaging and maintains the mock's contract by ensuring
attachResultis set before use.
Line range hint
111-121: LGTM! Consistent error handling pattern in detach method.The implementation follows the same pattern as
attach, maintaining consistency in the mock's behavior.
Line range hint
209-213: LGTM! Clear implementation for tracking published messages.The method effectively captures message details for verification in tests.
Line range hint
19-26: Address TODO comment about@unchecked Sendable.The TODO comment indicates that the
@unchecked Sendableusage needs review. Consider prioritizing the type tightening work referenced in issue #195 to ensure proper concurrency safety.Run this script to check the status of the referenced issue:
✅ Verification successful
The
@unchecked SendableTODO is correctly tracked in issue #195 and represents a known technical debt itemThis is part of a broader pattern of manual concurrency handling in the codebase, with similar
@unchecked Sendableusage found in other mock objects and even some production code. The issue remains open and acknowledges the need for type tightening work.
- The TODO comment in
MockRealtimeChannel.swiftexplicitly links to issue Improve testing for messages functionality #195- Similar patterns are found in other mock objects using manual synchronization
- There's a related discussion about concurrency concerns in
DefaultConnection.swift🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if issue #195 is still open gh issue view 195 --json state,title,bodyLength of output: 903
Script:
#!/bin/bash # Search for @unchecked Sendable usage and TODO comments echo "=== Searching for @unchecked Sendable usage ===" rg "@unchecked\s+Sendable" -A 2 echo -e "\n=== Searching for TODO comments about Sendable or concurrency ===" rg -i "todo.*sendable|todo.*concurrency" -A 2Length of output: 5962
This reverts b261155. See commit messages for more details.
Summary by CodeRabbit
Release Notes
Dependencies
ably-cocoapackage from version1.2.36to1.2.37Type System
RealtimePresenceProtocolto enhance presence management capabilitiesMocking
MockRealtimePresenceclass to simulate presence management in testsThese changes improve the library's type flexibility and testing infrastructure while updating a core dependency.