Add error and close callbacks to Swift Realtime service #1224
Conversation
This change adds support for custom error and close event handlers in the Swift Realtime implementation. Developers can now register callbacks to be notified when connection errors occur or when the connection closes, enabling better error handling and connection state management in applications.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds two public callback registration methods to Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Changes are limited to one file and are repetitive additions following existing callback patterns. Review should confirm API signatures, callback storage/invocation, and any thread-safety or consistency concerns with existing callbacks. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds error and close callback handlers to the Swift Realtime service, allowing developers to register custom callbacks for monitoring connection state changes and handling errors. This enables better error handling and connection lifecycle management in Swift applications.
- Adds
onError(_:)andonClose(_:)public methods for registering custom callbacks - Integrates callback invocations into
WebSocketClientDelegatemethods when connection events occur
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![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
🧹 Nitpick comments (1)
templates/swift/Sources/Services/Realtime.swift.twig (1)
29-35: API design is clean and follows Swift conventions.The callback registration methods are well-designed with appropriate signatures. The single-callback design (where each call replaces the previous callback) is reasonable for this use case.
Consider adding documentation to clarify:
- When these callbacks are invoked (connection errors, connection closure)
- That registering a new callback replaces the previous one
- Thread safety considerations (especially once synchronization is added per the previous comment)
Example:
+ /// Registers a callback to be invoked when a WebSocket error occurs. + /// Calling this method replaces any previously registered error callback. + /// - Parameter callback: A closure called with the error and HTTP status (if available). public func onError(_ callback: @escaping (Swift.Error?, HTTPResponseStatus?) -> Void) { self.onErrorCallback = callback } + /// Registers a callback to be invoked when the WebSocket connection closes. + /// Calling this method replaces any previously registered close callback. + /// - Parameter callback: A closure called when the connection closes. public func onClose(_ callback: @escaping () -> Void) { self.onCloseCallback = callback }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/swift/Sources/Services/Realtime.swift.twig(3 hunks)
⏰ 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). (15)
- GitHub Check: build (8.3, PHP80)
- GitHub Check: build (8.3, Python312)
- GitHub Check: build (8.3, KotlinJava17)
- GitHub Check: build (8.3, Node20)
- GitHub Check: build (8.3, Node16)
- GitHub Check: build (8.3, Go112)
- GitHub Check: build (8.3, FlutterStable)
- GitHub Check: build (8.3, CLINode16)
- GitHub Check: build (8.3, DotNet60)
- GitHub Check: build (8.3, CLINode18)
- GitHub Check: build (8.3, Android5Java17)
- GitHub Check: build (8.3, Android14Java17)
- GitHub Check: swift (server)
- GitHub Check: android (client)
- GitHub Check: apple (client)
🔇 Additional comments (2)
templates/swift/Sources/Services/Realtime.swift.twig (2)
225-225: Callback invocation placement is appropriate.Invoking the close callback early in the method ensures users are notified immediately when the connection closes, before any reconnection logic executes.
Consider wrapping the callback invocation in error handling to prevent unexpected crashes:
- onCloseCallback?() + do { + onCloseCallback?() + } catch { + print("Close callback failed: \(error.localizedDescription)") + }While the callback signature is non-throwing, defensive error handling can prevent crashes if the callback performs unexpected operations or force-throws.
247-247: Callback invocation correctly passes error parameters.The error callback is invoked with both the error and HTTP status, providing users with complete error information.
Consider the same defensive error handling as suggested for the close callback:
- onErrorCallback?(error, status) + do { + onErrorCallback?(error, status) + } catch { + print("Error callback failed: \(error.localizedDescription)") + }This prevents cascading failures if the user's error handling callback itself encounters issues.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
templates/swift/Sources/Services/Realtime.swift.twig (1)
26-27: Add synchronization for thread-safe callback access.These callback arrays are accessed from multiple threads without synchronization:
- Written by user code when calling
onError(_:)oronClose(_:)(potentially any thread) at lines 30 and 34- Read by WebSocket delegate methods (background thread) at lines 226 and 246
This creates a race condition that can lead to crashes or undefined behavior.
Apply this diff to protect the arrays using the existing
connectSyncqueue:- private var onErrorCallbacks: [((Swift.Error?, HTTPResponseStatus?) -> Void)] = [] - private var onCloseCallbacks: [(() -> Void)] = [] + private var _onErrorCallbacks: [((Swift.Error?, HTTPResponseStatus?) -> Void)] = [] + private var _onCloseCallbacks: [(() -> Void)] = []Then update the public methods to use synchronized access:
public func onError(_ callback: @escaping (Swift.Error?, HTTPResponseStatus?) -> Void) { - self.onErrorCallbacks.append(callback) + connectSync.sync { + self._onErrorCallbacks.append(callback) + } } public func onClose(_ callback: @escaping () -> Void) { - self.onCloseCallbacks.append(callback) + connectSync.sync { + self._onCloseCallbacks.append(callback) + } }And at invocation sites (lines 226, 246):
- onCloseCallbacks.forEach { $0() } + let callbacks = connectSync.sync { self._onCloseCallbacks } + callbacks.forEach { $0() }- onErrorCallbacks.forEach { $0(error, status) } + let callbacks = connectSync.sync { self._onErrorCallbacks } + callbacks.forEach { $0(error, status) }
🧹 Nitpick comments (1)
templates/swift/Sources/Services/Realtime.swift.twig (1)
29-35: Consider adding callback unregistration mechanism.The current API allows registering callbacks but provides no way to unregister them. Callbacks will accumulate for the lifetime of the
Realtimeinstance, which could lead to memory issues if callbacks capture strong references or if many callbacks are registered and unregistered frequently.Consider returning an opaque token or subscription object that allows unregistering:
public struct CallbackToken { fileprivate let id: UUID } public func onError(_ callback: @escaping (Swift.Error?, HTTPResponseStatus?) -> Void) -> CallbackToken { let token = CallbackToken(id: UUID()) connectSync.sync { self._onErrorCallbacks[token.id] = callback } return token } public func removeErrorCallback(_ token: CallbackToken) { connectSync.sync { self._onErrorCallbacks.removeValue(forKey: token.id) } }Note: This would require changing the storage from arrays to dictionaries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
templates/swift/Sources/Services/Realtime.swift.twig(3 hunks)
🔇 Additional comments (2)
templates/swift/Sources/Services/Realtime.swift.twig (2)
225-229: Close callback timing is correct.The callbacks are invoked only when
reconnectisfalse, which occurs during intentional disconnections (e.g., whenactiveChannelsis empty or manual disconnect viacloseSocket). During automatic reconnection attempts,reconnectremainstrueand the callbacks are not invoked. This design appears correct as it prevents spurious close notifications during transient network issues.
242-247: Error callback invocation looks correct.The error callbacks are invoked after logging the error, which is appropriate. Each registered callback receives both the error and HTTP status for comprehensive error handling.
There was a problem hiding this comment.
Should we add a e2e test for it? Is it easy?
Summary
onErrorandonClosecallback handlers to the Swift Realtime serviceChanges
onErrorCallbackandonCloseCallbackprivate propertiesonError(_:)andonClose(_:)methods to register callbacksWebSocketClientDelegatemethodsBenefits
This enhancement enables better error handling and connection state management for Swift applications using the Realtime service.
Summary by CodeRabbit