[ECO-5432] Occupancy.current() method
#343
Conversation
WalkthroughA synchronous Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DefaultOccupancy
participant Realtime
Client->>DefaultOccupancy: call current()
alt occupancy events enabled
DefaultOccupancy->>DefaultOccupancy: return lastOccupancyData (cached)
DefaultOccupancy-->>Client: OccupancyData? (may be nil)
else occupancy events not enabled
DefaultOccupancy-->>Client: throw ARTErrorInfo (occupancyEventsNotEnabled)
end
Note over Realtime,DefaultOccupancy: Realtime events update lastOccupancyData when received
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
Sources/AblyChat/Occupancy.swift (1)
31-38: Fix documentation terminology for Swift.The documentation mentions "undefined" which is JavaScript terminology. In Swift, we should use "nil" instead.
- * - Returns: The latest occupancy data, or undefined if no realtime events have been received yet. + * - Returns: The latest occupancy data, or nil if no realtime events have been received yet.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/AblyChat/DefaultOccupancy.swift(3 hunks)Sources/AblyChat/Errors.swift(4 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultOccupancy.swift:10-13
Timestamp: 2025-05-12T21:01:14.109Z
Learning: The `Occupancy` protocol in the Ably Chat Swift SDK is annotated with `@MainActor`, making all conforming types like `DefaultOccupancy` automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: ably/ably-chat-swift#165
File: Sources/AblyChat/Reaction.swift:14-14
Timestamp: 2024-12-10T01:59:02.065Z
Learning: For the 'ably-chat-swift' repository, documentation-only PRs should focus exclusively on public methods and properties. Avoid suggesting changes to internal comments or private methods in such PRs.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultTyping.swift:131-138
Timestamp: 2025-05-12T21:04:36.263Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle closures should capture self weakly with `[weak self]` to avoid potential retain cycles, particularly when calling channel.unsubscribe() within the closure.
Sources/AblyChat/Errors.swift (4)
Learnt from: maratal
PR: #286
File: Sources/AblyChat/DefaultOccupancy.swift:10-13
Timestamp: 2025-05-12T21:01:14.109Z
Learning: The Occupancy protocol in the Ably Chat Swift SDK is annotated with @MainActor, making all conforming types like DefaultOccupancy automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: #286
File: Sources/AblyChat/SubscriptionAsyncSequence.swift:36-41
Timestamp: 2025-06-29T16:02:43.988Z
Learning: In the AblyChat Swift codebase, the fatalError in SubscriptionAsyncSequence.swift's mock async sequence handling (around line 39) is intentional and should not be flagged. This fatalError catches programmer errors when a throwing AsyncSequence is incorrectly passed to the mock initializer, which is meant for testing/development purposes.
Learnt from: maratal
PR: #293
File: Example/AblyChatExample/ContentView.swift:94-99
Timestamp: 2025-06-14T21:47:20.509Z
Learning: In the Ably Chat Swift example app, using fatalError for missing clientID is intentional and appropriate since it represents a programmer error rather than a recoverable runtime condition. The clientID is considered an essential configuration requirement.
Learnt from: maratal
PR: #165
File: Sources/AblyChat/Reaction.swift:14-14
Timestamp: 2024-12-10T01:59:02.065Z
Learning: For the 'ably-chat-swift' repository, documentation-only PRs should focus exclusively on public methods and properties. Avoid suggesting changes to internal comments or private methods in such PRs.
Sources/AblyChat/Occupancy.swift (4)
Learnt from: maratal
PR: #286
File: Sources/AblyChat/DefaultOccupancy.swift:10-13
Timestamp: 2025-05-12T21:01:14.109Z
Learning: The Occupancy protocol in the Ably Chat Swift SDK is annotated with @MainActor, making all conforming types like DefaultOccupancy automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: #286
File: Sources/AblyChat/DefaultTyping.swift:11-14
Timestamp: 2025-05-12T21:03:31.914Z
Learning: The Typing protocol in the Ably Chat Swift SDK is annotated with @MainActor, making all conforming types like DefaultTyping automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: #286
File: Sources/AblyChat/DefaultTyping.swift:11-14
Timestamp: 2025-05-12T21:03:31.914Z
Learning: The Typing protocol in the Ably Chat Swift SDK is annotated with @MainActor, making all conforming types like DefaultTyping automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: #286
File: Example/AblyChatExample/Mocks/MockClients.swift:83-95
Timestamp: 2025-05-12T21:46:44.489Z
Learning: The periodic function in Example/AblyChatExample/Misc/Utils.swift is designed to execute its closure on the main actor (@MainActor), ensuring thread safety when used with shared mutable state that's only accessed from the main actor.
Sources/AblyChat/DefaultOccupancy.swift (3)
Learnt from: maratal
PR: #286
File: Sources/AblyChat/DefaultOccupancy.swift:10-13
Timestamp: 2025-05-12T21:01:14.109Z
Learning: The Occupancy protocol in the Ably Chat Swift SDK is annotated with @MainActor, making all conforming types like DefaultOccupancy automatically main actor-isolated without requiring explicit annotations on their methods.
Learnt from: maratal
PR: #286
File: Sources/AblyChat/DefaultTyping.swift:131-138
Timestamp: 2025-05-12T21:04:36.263Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle closures should capture self weakly with [weak self] to avoid potential retain cycles, particularly when calling channel.unsubscribe() within the closure.
Learnt from: maratal
PR: #286
File: Sources/AblyChat/RoomLifecycleManager.swift:611-624
Timestamp: 2025-05-22T20:57:48.147Z
Learning: In AblyChat Swift, when using continuations with subscription callbacks, the continuation object is captured directly by the closure and doesn't depend on self. Even with [weak self] capture, the continuation will still be resumed properly.
🧬 Code Graph Analysis (1)
Sources/AblyChat/Occupancy.swift (1)
Sources/AblyChat/DefaultOccupancy.swift (1)
current(71-79)
🔇 Additional comments (7)
Sources/AblyChat/Errors.swift (4)
180-180: LGTM! New error case properly defined.The error case follows the established patterns in the codebase.
219-220: LGTM! Appropriate status code mapping.Using
.badRequest(400) for the occupancy events not enabled error is appropriate as it represents a client precondition failure.
281-282: LGTM! Clear and descriptive error message.The error message clearly explains the issue and is consistent with similar error messages in the codebase.
307-308: LGTM! Properly included in cause handling.The error case is correctly included in the list of cases that don't have an underlying cause.
Sources/AblyChat/DefaultOccupancy.swift (3)
10-10: LGTM! Appropriate property for caching latest occupancy data.The optional property correctly stores the most recent occupancy data received from realtime events.
46-46: LGTM! Cache update in the right location.Updating the cached data when receiving new occupancy events is the correct approach and placement.
71-79: LGTM! Correct implementation of the current() method.The implementation properly:
- Validates the precondition (events enabled) and throws the appropriate error
- Returns the cached occupancy data
- Includes proper specification references
- Follows the established error handling patterns
maratal
force-pushed
the
315-occupancy-current
branch
from
c3f9d07 to
6aab393
Compare
maratal
force-pushed
the
315-occupancy-current
branch
from
6aab393 to
1e5f227
Compare
maratal
force-pushed
the
315-occupancy-current
branch
from
1e5f227 to
e4b8d13
Compare
maratal
force-pushed
the
315-occupancy-current
branch
from
e4b8d13 to
054b5d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Sources/AblyChat/Occupancy.swift (1)
34-34: Doc fix: use “nil” instead of “undefined” in SwiftSwift uses
nil, not “undefined”. Please update the Returns doc accordingly.Apply this diff:
- * - Returns: The latest occupancy data, or undefined if no realtime events have been received yet. + * - Returns: The latest occupancy data, or nil if no realtime events have been received yet.Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (2)
53-69: Nit: slim down the mock message to essentialsOnly
data["metrics"]is read by the production code; the extra fields (action,serial,clientId,version) add noise without value. Trimming them simplifies the test fixture.Apply this diff:
let channel = MockRealtimeChannel( name: "basketball::$chat", messageToEmitOnSubscribe: { let message = ARTMessage() - message.action = .create // arbitrary - message.serial = "" // arbitrary - message.clientId = "" // arbitrary message.data = [ "metrics": [ "connections": 5, // arbitrary "presenceMembers": 2, // arbitrary ], ] - message.version = "0" // arbitrary return message }() )
125-151: Error-path test is correct; minor typo in commentThe test correctly verifies
current()throwsoccupancyEventsNotEnabledwhen events are disabled. Small typo at Line 137 (“Wnen”).Apply this diff:
- // Wnen + // When
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
Example/AblyChatExample/Mocks/MockClients.swift(1 hunks)Sources/AblyChat/DefaultOccupancy.swift(3 hunks)Sources/AblyChat/Errors.swift(4 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Sources/AblyChat/Errors.swift
- Example/AblyChatExample/Mocks/MockClients.swift
- Sources/AblyChat/DefaultOccupancy.swift
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultOccupancy.swift:10-13
Timestamp: 2025-05-12T21:01:14.109Z
Learning: The `Occupancy` protocol in the Ably Chat Swift SDK is annotated with `MainActor`, making all conforming types like `DefaultOccupancy` automatically main actor-isolated without requiring explicit annotations on their methods.
📚 Learning: 2025-05-12T21:01:14.109Z
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultOccupancy.swift:10-13
Timestamp: 2025-05-12T21:01:14.109Z
Learning: The `Occupancy` protocol in the Ably Chat Swift SDK is annotated with `MainActor`, making all conforming types like `DefaultOccupancy` automatically main actor-isolated without requiring explicit annotations on their methods.
Applied to files:
Sources/AblyChat/Occupancy.swiftTests/AblyChatTests/DefaultRoomOccupancyTests.swift
📚 Learning: 2025-05-23T16:20:38.904Z
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:20:38.904Z
Learning: In Swift tests using mock objects like MockRealtimeChannel, explicit subscription cleanup (via unsubscribe() or similar methods) is generally unnecessary since the mocks don't maintain real connections and will be garbage collected after the test completes.
Applied to files:
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
📚 Learning: 2025-05-23T16:29:39.712Z
Learnt from: maratal
PR: ably/ably-chat-swift#262
File: Tests/AblyChatTests/DefaultRoomReactionsTests.swift:0-0
Timestamp: 2025-05-23T16:29:39.712Z
Learning: In the ably-chat-swift project, MockRealtimeChannel processes messages synchronously when subscribe is called, immediately delivering the configured messages to the callback.
Applied to files:
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
📚 Learning: 2025-06-14T16:19:29.327Z
Learnt from: maratal
PR: ably/ably-chat-swift#293
File: Example/AblyChatExample/Mocks/MockRealtime.swift:348-384
Timestamp: 2025-06-14T16:19:29.327Z
Learning: The MockRealtime class in Example/AblyChatExample/Mocks/MockRealtime.swift is specifically for example app construction, not for testing. The actual test mocks are located in the Tests directory. If example app mocks are unused, fatalError calls are appropriate to catch accidental usage.
Applied to files:
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
🧬 Code Graph Analysis (2)
Sources/AblyChat/Occupancy.swift (2)
Sources/AblyChat/DefaultOccupancy.swift (1)
current(71-79)Example/AblyChatExample/Mocks/MockClients.swift (1)
current(567-569)
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (2)
Sources/AblyChat/DefaultOccupancy.swift (2)
current(71-79)subscribe(20-58)Sources/AblyChat/Occupancy.swift (2)
subscribe(53-67)subscribe(70-72)
⏰ 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). (34)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.1) - GitHub Check: SPM (Xcode 16.1)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: SPM (Xcode 16.2)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: SPM (Xcode 16.3)
- GitHub Check: Generate code coverage
🔇 Additional comments (3)
Sources/AblyChat/Occupancy.swift (1)
31-38: Solid addition: synchronous cache-backedcurrent()API fits the spec and actor modelThe API shape looks good:
current()complementsget()with immediate, cached access, enforces enableEvents via typed throws, and leverages@MainActorvia the protocol annotation (consistent with our prior approach).Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (2)
10-41: Good coverage: verifiescurrent()is nil before any realtime eventsAsserting that
current()returns nil after a successfulget()(but before receiving any occupancy event) is exactly the expected behavior for a cache-backed API.
88-91: LGTM: confirmscurrent()reflects last emitted occupancyValidates that after a realtime update, the cache is populated and returned by
current(). Matches the implementation that updateslastOccupancyDatabefore invoking callbacks.
maratal
force-pushed
the
315-occupancy-current
branch
from
054b5d2 to
956f65c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Sources/AblyChat/Occupancy.swift (1)
31-38: Doc fix: use “nil” (Swift) instead of “undefined” and reference the specific error case.The method returns an Optional; in Swift, “nil” is the correct term (not “undefined”). Also consider explicitly naming the error case thrown for clarity.
Apply this diff to update the doc comment:
/** * Get the latest occupancy data received from realtime events. * - * - Returns: The latest occupancy data, or undefined if no realtime events have been received yet. + * - Returns: The latest occupancy data, or nil if no realtime events have been received yet. * - * - Throws: ``ARTErrorInfo`` if occupancy events are not enabled for this room. + * - Throws: ``ARTErrorInfo`` (``ChatError.occupancyEventsNotEnabled``) if occupancy events are not enabled for this room. */ func current() throws(ARTErrorInfo) -> OccupancyData?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
Example/AblyChatExample/Mocks/MockClients.swift(1 hunks)Sources/AblyChat/DefaultOccupancy.swift(3 hunks)Sources/AblyChat/Errors.swift(4 hunks)Sources/AblyChat/Occupancy.swift(1 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Example/AblyChatExample/Mocks/MockClients.swift
- Sources/AblyChat/DefaultOccupancy.swift
- Sources/AblyChat/Errors.swift
- Tests/AblyChatTests/DefaultRoomOccupancyTests.swift
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultOccupancy.swift:10-13
Timestamp: 2025-05-12T21:01:14.109Z
Learning: The `Occupancy` protocol in the Ably Chat Swift SDK is annotated with `MainActor`, making all conforming types like `DefaultOccupancy` automatically main actor-isolated without requiring explicit annotations on their methods.
📚 Learning: 2025-05-12T21:01:14.109Z
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultOccupancy.swift:10-13
Timestamp: 2025-05-12T21:01:14.109Z
Learning: The `Occupancy` protocol in the Ably Chat Swift SDK is annotated with `MainActor`, making all conforming types like `DefaultOccupancy` automatically main actor-isolated without requiring explicit annotations on their methods.
Applied to files:
Sources/AblyChat/Occupancy.swift
🧬 Code Graph Analysis (1)
Sources/AblyChat/Occupancy.swift (2)
Sources/AblyChat/DefaultOccupancy.swift (1)
current(71-79)Example/AblyChatExample/Mocks/MockClients.swift (1)
current(567-569)
🔇 Additional comments (1)
Sources/AblyChat/Occupancy.swift (1)
31-38: New protocol requirement is a source-breaking changeWe confirmed that within this repo only two types conform to
Occupancyand both already implementcurrent():
- Sources/AblyChat/DefaultOccupancy.swift
- Example/AblyChatExample/Mocks/MockClients.swift
However, because
Occupancyis a public protocol, any external client code that previously conformed to it (and didn’t implementcurrent()) will now fail to compile. Please choose one of the following:• Option A (preserve backwards compatibility):
Add a default implementation in apublic extension Occupancy, for example:public extension Occupancy { func current() throws(ARTErrorInfo) -> OccupancyData? { return nil } }SDK types (like
DefaultOccupancy) continue to override this with their real logic.• Option B (intentional breaking change):
Acknowledge the break, plan a semver-major release, and call it out in your release notes.
Closes #315
Summary by CodeRabbit