[ECO-5483] Loosening validation for Occupancy#340
Conversation
WalkthroughThis change updates subscription closures in several classes to capture Changes
Sequence Diagram(s)sequenceDiagram
participant SDKClient
participant DefaultMessages
participant EventSource
SDKClient->>DefaultMessages: subscribe()
DefaultMessages->>EventSource: subscribe (closure captures self weakly)
EventSource-->>DefaultMessages: event callback
alt self is alive
DefaultMessages->>SDKClient: handle event
else self is nil
EventSource-->>EventSource: closure returns early
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningsSources/AblyChat/Errors.swift (5)Learnt from: maratal Learnt from: maratal Learnt from: maratal Learnt from: maratal Learnt from: maratal ⏰ 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)
🔇 Additional comments (4)
✨ 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. 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 (
|
maratal
force-pushed
the
337-message-validation-for-all-features
branch
from
2522ee1 to
65fee52
Compare
… there). Add self capture for subscription handlers instead of particular objects capture list.
maratal
force-pushed
the
337-message-validation-for-all-features
branch
from
65fee52 to
d161f7b
Compare
…oned in the task, since `clientID` and `updatedAt` is non null in the `PresenceMember` structure. I could make them nullable, but it doesn't have a lot of sense IMO (internal conversation - https://ably-real-time.slack.com/archives/C03JDBVM5MY/p1753443612066009?thread_ts=1753373214.880469&cid=C03JDBVM5MY)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Sources/AblyChat/DefaultPresence.swift(2 hunks)
🧰 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.
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.
Learnt from: maratal
PR: ably/ably-chat-swift#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.
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.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/ContentView.swift:203-209
Timestamp: 2025-05-12T21:11:08.937Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription callbacks continue to work even when the handle is not stored, as the underlying services maintain their own storage of callbacks that remain registered until explicitly unsubscribed via the handle's unsubscribe() method.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Example/AblyChatExample/ContentView.swift:203-209
Timestamp: 2025-05-12T21:11:08.937Z
Learning: In the AblyChat Swift codebase, SubscriptionHandle doesn't need to be retained/stored to keep subscriptions active. The subscription continues to work even if the handle is not stored, as the underlying services maintain references to the callbacks until explicitly unsubscribed via the handle's unsubscribe() method.
Learnt from: maratal
PR: ably/ably-chat-swift#249
File: Tests/AblyChatTests/DefaultMessagesTests.swift:45-48
Timestamp: 2025-05-22T19:17:21.392Z
Learning: The `let doIt = {}` closures in DefaultMessagesTests.swift are temporary workarounds for compiler crashes (related to issue #233) and will be removed once Xcode 16.3 is released, so they should not be flagged for missing async/throws annotations.
Learnt from: maratal
PR: ably/ably-chat-swift#286
File: Sources/AblyChat/DefaultPresence.swift:269-277
Timestamp: 2025-05-16T22:07:33.286Z
Learning: In Swift, capturing an instance method in a closure's capture list (like `[someMethod]`) implicitly captures `self` strongly, as instance methods are functions that take `self` as their first parameter. This can lead to retain cycles, and should be avoided by using `[weak self]` instead.
Sources/AblyChat/DefaultPresence.swift (12)
Learnt from: maratal
PR: #286
File: Example/AblyChatExample/Mocks/MockClients.swift:0-0
Timestamp: 2025-05-16T21:04:26.244Z
Learning: In the ably-chat-swift project, mock implementations (like those in MockClients.swift) are intentionally kept simple, sometimes omitting parameter-based filtering behavior for testing simplicity.
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/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: #249
File: Tests/AblyChatTests/DefaultMessagesTests.swift:45-48
Timestamp: 2025-05-22T19:17:21.392Z
Learning: The let doIt = {} closures in DefaultMessagesTests.swift are temporary workarounds for compiler crashes (related to issue #233) and will be removed once Xcode 16.3 is released, so they should not be flagged for missing async/throws annotations.
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.
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: #165
File: docs-coverage-report:5-8
Timestamp: 2024-12-10T01:59:12.404Z
Learning: In the AblyChat Swift project, documentation coverage should focus on public methods and properties. Private and internal entities do not require documentation.
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.
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: #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.
Learnt from: maratal
PR: #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.
🔇 Additional comments (2)
Sources/AblyChat/DefaultPresence.swift (2)
313-313: Timestamp fallback strategy looks reasonable.Using
Date()as the fallback for missing timestamps is a sensible approach. It provides a meaningful timestamp representing when the message was processed, which is better than using an arbitrary date or leaving it nil.Also applies to: 327-327
307-320: processPresenceGet is intentionally permissive — no downstream error handlers are affected
- Both
processPresenceGetandprocessPresenceSubscribenever actually throw today (theirthrowssignatures are unused), so:
• The outercatch { throw error.toARTErrorInfo() }aroundprocessPresenceGetwill never be entered
• The subscription-levelcatch {}afterprocessPresenceSubscribelikewise becomes dead code- All real error paths continue to surface only channel- or lifecycle-related errors
- Falling back to
""for missingclientIdandDate()for missingtimestamponly applies when the server omits those fields—confirm these defaults meet your data-quality requirementsNo changes required; behavior aligns with the PR’s goal.
maratal
force-pushed
the
337-message-validation-for-all-features
branch
from
7b6d013 to
0441c2b
Compare
why? |
@maratal waiting for an answer to this before approving |
I decided to make it the same everywhere to avoid any risk of retain cycle, since coderabbit complained about it elsewhere. Holding |
2 participants
Closes #337
Loosening validation for
Occupancy(for everything else was already there).Add self capture for subscription handlers instead of particular objects capture list.
Also:
Loose validation for presence as well despite the fact it's not mentioned in the task, since
clientIDandupdatedAtis non null in thePresenceMemberstructure. I could make them nullable, but it doesn't have a lot of sense IMO (internal conversation - https://ably-real-time.slack.com/archives/C03JDBVM5MY/p1753443612066009?thread_ts=1753373214.880469&cid=C03JDBVM5MY)Related PR - #330
Summary by CodeRabbit
Summary by CodeRabbit