presence: remove event filter #403
Conversation
WalkthroughPresence subscription APIs were simplified by removing per-event and multi-event subscribe overloads, consolidating to a single subscribe() path that derives event types internally. Example app and mocks were updated to call the unified subscription; tests were updated to use the new API and adjusted channel names. Internal handling now derives PresenceEvent from PresenceMessage.action. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as App UI
participant Presence as Presence API
participant Sub as Subscription
participant Channel as Realtime Channel
UI->>Presence: subscribe(callback)
activate Presence
Presence->>Sub: create unified subscription
deactivate Presence
Channel-->>Sub: PresenceMessage(action, data)
activate Sub
Sub->>Presence: deliver PresenceMessage
Presence->>Presence: derive PresenceEvent from action
Presence->>UI: callback(PresenceEvent)
deactivate Sub
rect rgba(230,245,255,0.6)
note right of Presence: Removed per-event filtering<br/>Event type resolved internally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (7)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (2)**/*.swift📄 CodeRabbit inference engine (CLAUDE.md)
Files:
Sources/**/*.swift📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)Example/AblyChatExample/ContentView.swift (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/403/AblyChat
AndyTWF
force-pushed
the
presence-filter-removal
branch
from
d730484 to
9794add
Compare
github-actions
bot
temporarily deployed
to
staging/pull/403/AblyChat
|
Looks like dup of 6964536 |
|
Not quite as this one goes further - will catch up with Lawrence |
AndyTWF
force-pushed
the
presence-filter-removal
branch
from
9794add to
efcd7ff
Compare
github-actions
bot
temporarily deployed
to
staging/pull/403/AblyChat
AndyTWF
force-pushed
the
presence-filter-removal
branch
from
efcd7ff to
f781955
Compare
github-actions
bot
temporarily deployed
to
staging/pull/403/AblyChat
AndyTWF
force-pushed
the
presence-filter-removal
branch
from
f781955 to
0f50931
Compare
github-actions
bot
temporarily deployed
to
staging/pull/403/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: 0
🧹 Nitpick comments (3)
Tests/AblyChatTests/DefaultPresenceTests.swift (1)
457-457: Consider markingfirstpredicates as @sendable for consistencyNot required here, but mirrors usage elsewhere and avoids potential warnings.
As per coding guidelines
Also applies to: 465-465, 473-473, 481-481
Sources/AblyChat/DefaultPresence.swift (1)
236-253: Event-type mapping: avoid duplication and clarify unknownsYou derive the type via
PresenceEventType(ablyCocoaPresenceAction:), which defaults unknown/absent to.leave. Consider:
- Deduplicate mapping by forwarding to the existing failable initializer.
- Optionally log/ignore unknown or
.absentactions rather than mapping to.leaveto prevent misclassification.Suggested minimal dedupe:
- internal init(ablyCocoaPresenceAction action: ARTPresenceAction) { - switch action { - case .present: self = .present - case .enter: self = .enter - case .leave: self = .leave - case .update: self = .update - case .absent: self = .leave - @unknown default: self = .leave - } - } + internal init(ablyCocoaPresenceAction action: ARTPresenceAction) { + self = Self(ablyCocoaValue: action) ?? .leave + }Sources/AblyChat/Presence.swift (1)
236-253: Deduplicate PresenceEventType mappingThis initializer duplicates the logic of
init?(ablyCocoaValue:). Prefer delegating to it to keep one mapping source:- internal init(ablyCocoaPresenceAction action: ARTPresenceAction) { - switch action { - case .present: self = .present - case .enter: self = .enter - case .leave: self = .leave - case .update: self = .update - case .absent: self = .leave - @unknown default: self = .leave - } - } + internal init(ablyCocoaPresenceAction action: ARTPresenceAction) { + self = Self(ablyCocoaValue: action) ?? .leave + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
Example/AblyChatExample/ContentView.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(0 hunks)Sources/AblyChat/DefaultPresence.swift(2 hunks)Sources/AblyChat/Presence.swift(3 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(16 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(0 hunks)Tests/AblyChatTests/IntegrationTests.swift(1 hunks)
💤 Files with no reviewable changes (2)
- Tests/AblyChatTests/Helpers/Helpers.swift
- Example/AblyChatExample/Mocks/MockClients.swift
🧰 Additional context used
📓 Path-based instructions (3)
**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.swift: Expose SDK functionality via protocols; prefer associated types with opaque return types (some Protocol) over existentials (any Protocol)
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
Task, CheckedContinuation, and AsyncThrowingStream don’t support typed errors—use Result and call .get()
Dictionary.mapValues doesn’t support typed throws—use ablyChat_mapValuesWithTypedThrow extension
When the compiler struggles with typed throws, explicitly specify the error type with do throws(InternalError)
Specify the error type in closures, e.g., try items.map { jsonValue throws(InternalError) in … }
Files:
Sources/AblyChat/DefaultPresence.swiftSources/AblyChat/Presence.swiftTests/AblyChatTests/IntegrationTests.swiftTests/AblyChatTests/DefaultPresenceTests.swiftExample/AblyChatExample/ContentView.swift
Sources/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Sources/**/*.swift: Public API must use typed throws with ARTErrorInfo; use InternalError internally and convert at public API boundaries
Reference Chat SDK features spec items in code comments when implementing behavior (e.g., // @SPEC CHA-RL3g)
For public structs exposed by the API, provide an explicit public memberwise initializer
Test-only APIs in the library must be prefixed with testsOnly_ and wrapped in #if DEBUG
Files:
Sources/AblyChat/DefaultPresence.swiftSources/AblyChat/Presence.swift
Tests/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Tests/**/*.swift: Use test attribution tags in tests: @SPEC, @specOneOf(m/n), @specPartial, @specUntested - , @specNotApplicable -
For Swift Testing #expect(throws:) with typed errors, move typed-throw code into a separate non-typed-throw function
Files:
Tests/AblyChatTests/IntegrationTests.swiftTests/AblyChatTests/DefaultPresenceTests.swift
🧬 Code graph analysis (4)
Sources/AblyChat/Presence.swift (3)
Example/AblyChatExample/Mocks/MockClients.swift (10)
enter(469-471)enter(473-475)enter(477-490)leave(515-517)leave(519-521)leave(523-536)update(181-195)update(492-494)update(496-498)update(500-513)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (3)
enter(42-47)leave(35-40)update(49-54)Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (3)
enter(275-289)leave(259-273)update(291-305)
Tests/AblyChatTests/IntegrationTests.swift (2)
Sources/AblyChat/DefaultPresence.swift (1)
subscribe(199-219)Sources/AblyChat/Presence.swift (2)
subscribe(126-140)subscribe(143-145)
Tests/AblyChatTests/DefaultPresenceTests.swift (6)
Sources/AblyChat/DefaultPresence.swift (10)
subscribe(199-219)enter(95-97)enter(99-101)enter(104-125)update(127-129)update(131-133)update(136-157)leave(159-161)leave(163-165)leave(168-189)Sources/AblyChat/Presence.swift (2)
subscribe(126-140)subscribe(143-145)Example/AblyChatExample/Mocks/MockClients.swift (16)
subscribe(127-156)subscribe(284-308)subscribe(339-356)subscribe(370-386)subscribe(538-540)subscribe(554-565)enter(469-471)enter(473-475)enter(477-490)update(181-195)update(492-494)update(496-498)update(500-513)leave(515-517)leave(519-521)leave(523-536)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (5)
subscribe(7-9)subscribe(11-13)enter(42-47)update(49-54)leave(35-40)Sources/AblyChat/Messages.swift (1)
emit(407-409)Example/AblyChatExample/Mocks/MockSubscriptionStorage.swift (3)
emit(55-59)emit(113-117)emit(176-180)
Example/AblyChatExample/ContentView.swift (3)
Sources/AblyChat/DefaultPresence.swift (1)
subscribe(199-219)Sources/AblyChat/Presence.swift (2)
subscribe(126-140)subscribe(143-145)Example/AblyChatExample/Mocks/MockClients.swift (6)
subscribe(127-156)subscribe(284-308)subscribe(339-356)subscribe(370-386)subscribe(538-540)subscribe(554-565)
🔇 Additional comments (7)
Example/AblyChatExample/ContentView.swift (1)
317-329: Presence subscribe simplification LGTMSwitching to unfiltered subscribe aligns with the new API and keeps UI updates on MainActor.
Tests/AblyChatTests/IntegrationTests.swift (1)
378-380: Adoption of subscribe() without filters looks goodMatches the unified presence subscription API.
Tests/AblyChatTests/DefaultPresenceTests.swift (2)
13-13: Channel name updates to "basketball::$chat" are consistentAll adjusted tests target the correct channel namespace.
Also applies to: 37-37, 61-61, 89-89, 123-123, 153-153, 177-177, 205-205, 239-239, 269-269, 295-295, 323-323, 349-349, 377-377, 404-404
435-484: New test covers all presence events end-to-endGood coverage for present/enter/update/leave via unified subscription.
Sources/AblyChat/DefaultPresence.swift (1)
204-212: Unified subscribe path looks correctDeriving PresenceEvent from the message and invoking the callback on MainActor is sound.
Sources/AblyChat/Presence.swift (2)
76-88: Public subscribe(callback) docs/readability improvedClear that this subscribes to all presence events; matches API surface reduction.
114-146: AsyncSequence wrapper for subscribe is solidTermination handler unsubscribes on MainActor; good pattern.
AndyTWF
force-pushed
the
presence-filter-removal
branch
3 times, most recently
from
344dec8 to
8d9a763
Compare
In the rest of the SDK, we've not provided event filters on subscription events - this is a fairly trivial thing for users to write (if/switch). Also, when dealing with multiple events in the list, the if/switch is required anyway and in many languages must be exhaustive, so the real benefit is when you're listening to a single event (but still marginal). Furthermore, the overloading is somewhat cumbersome to maintain. This change removes the event filtering for presence.
AndyTWF
force-pushed
the
presence-filter-removal
branch
from
8d9a763 to
57b06d6
Compare
In the rest of the SDK, we've not provided event filters on subscription events - this is a fairly trivial thing for users to write (if/switch). Also, when dealing with multiple events in the list, the if/switch is required anyway and in many languages must be exhaustive, so the real benefit is when you're listening to a single event (but still marginal).
Furthermore, the overloading is somewhat cumbersome to maintain.
This change removes the event filtering for presence.
JS reference PR: ably/ably-chat-js#675
Summary by CodeRabbit