Skip to content

[ECO-5496] Remove throwing handler#345

Merged
maratal merged 1 commit intomainfrom
344-remove-throwing-handler
Aug 18, 2025
Merged

[ECO-5496] Remove throwing handler#345
maratal merged 1 commit intomainfrom
344-remove-throwing-handler

Conversation

@maratal
Copy link
Copy Markdown
Collaborator

@maratal maratal commented Jul 27, 2025

Closes #344

Summary by CodeRabbit

  • Bug Fixes

    • Prevents potential retain cycles in presence event subscriptions, reducing risk of memory leaks and improving app stability.
  • Refactor

    • Streamlined presence subscription callbacks to use a safer capture pattern.
    • Unified handling for single and multiple event subscriptions.
    • Simplified internal error handling without changing behavior or public APIs.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jul 27, 2025

Walkthrough

Converted presence subscription closures to capture weak self, removed try/catch handling, and made processPresenceSubscribe non-throwing. Updated single- and multi-event subscription paths accordingly. No changes to public APIs or other presence behaviors.

Changes

Cohort / File(s) Summary of Changes
Presence subscriptions refactor
Sources/AblyChat/DefaultPresence.swift
- Closures capture weak self; early-exit if deallocated
- Removed try/catch around processing; direct invocation
- Changed helper to non-throwing: processPresenceSubscribe(_:for:) -> PresenceEvent
- Applied same pattern to single/multi-event subscriptions; logging retained

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant AblySDK as Ably SDK
  participant DefaultPresence
  participant Callback

  Client->>AblySDK: subscribe(presence events)
  AblySDK-->>DefaultPresence: PresenceMessage(event)
  DefaultPresence->>DefaultPresence: processPresenceSubscribe(message, event) // non-throwing
  DefaultPresence-->>Callback: callback(PresenceEvent)
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Assessment against linked issues

Objective Addressed Explanation
Remove throwing in presence subscription handler (#344, ECO-5496)

Assessment against linked issues: Out-of-scope changes

[No out-of-scope functional changes detected.]

Possibly related issues

Poem

In burrows of code where callbacks hop,
I snip the throws—a gentle plop.
Weak little self peeks out to see,
“Am I alive? Then let it be.”
Presence events now glide with ease—
No tangled nets, just breezy breeze. 🐇✨

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 Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 344-remove-throwing-handler

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.

❤️ Share
🪧 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.
    • 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.
  • 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 the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

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

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • 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.

@maratal maratal changed the base branch from main to 315-occupancy-current July 27, 2025 11:40
@github-actions github-actions Bot temporarily deployed to staging/pull/345/AblyChat July 27, 2025 11:41 Inactive
@maratal maratal force-pushed the 315-occupancy-current branch 2 times, most recently from 054b5d2 to 956f65c Compare August 18, 2025 12:08
Base automatically changed from 315-occupancy-current to main August 18, 2025 12:16
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between abed8e1 and d777a43.

📒 Files selected for processing (1)
  • Sources/AblyChat/DefaultPresence.swift (3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 Learning: 2025-05-12T21:04:36.263Z
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.

Applied to files:

  • Sources/AblyChat/DefaultPresence.swift
📚 Learning: 2025-05-22T20:57:48.147Z
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.

Applied to files:

  • Sources/AblyChat/DefaultPresence.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:

  • Sources/AblyChat/DefaultPresence.swift
📚 Learning: 2025-05-12T21:11:08.937Z
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.

Applied to files:

  • Sources/AblyChat/DefaultPresence.swift
🧬 Code Graph Analysis (1)
Sources/AblyChat/DefaultPresence.swift (2)
Sources/AblyChat/Presence.swift (5)
  • subscribe (137-151)
  • subscribe (164-178)
  • subscribe (181-183)
  • subscribe (186-188)
  • toARTPresenceAction (244-255)
Sources/AblyChat/DefaultOccupancy.swift (1)
  • subscribe (20-58)
🔇 Additional comments (1)
Sources/AblyChat/DefaultPresence.swift (1)

264-279: Non-throwing processing helper aligns with PR objective.

processPresenceSubscribe is now non-throwing and returns a PresenceEvent directly. This matches the goal of removing throwing behavior from the subscription path. Implementation is straightforward and logs appropriately.

Comment thread Sources/AblyChat/DefaultPresence.swift
Comment thread Sources/AblyChat/DefaultPresence.swift
@maratal maratal merged commit 4476dfa into main Aug 18, 2025
19 checks passed
@maratal maratal deleted the 344-remove-throwing-handler branch August 18, 2025 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Remove throwing in presence subscription handler

2 participants