[ECO-5592] Remove transient disconnect handling from Connection. #464
Conversation
|
Warning Rate limit exceeded@maratal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis PR removes transient-disconnect timer logic from DefaultConnection and simplifies onStatusChange to emit events directly. It adds comprehensive connection state tests and enhances mock connection helpers to allow state mutation and event emission for testing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant DefaultConnection
participant Listener
Note over DefaultConnection: Old Flow (with transient timer)
rect rgb(230,240,250)
DefaultConnection->>DefaultConnection: onStatusChange detected
DefaultConnection->>DefaultConnection: evaluate/start/cancel transient timer
DefaultConnection->>Listener: emit statusChange (after timer logic)
end
Note over DefaultConnection: New Flow (direct emit)
rect rgb(230,250,230)
DefaultConnection->>DefaultConnection: onStatusChange detected
DefaultConnection->>Listener: emit statusChange immediately
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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/464/AblyChat
maratal
force-pushed
the
393-remove-transient-disconnect
branch
from
3dc9e07 to
cf9e95a
Compare
github-actions
bot
temporarily deployed
to
staging/pull/464/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 (1)
Tests/AblyChatTests/Mocks/MockConnection.swift (1)
5-24: Consider marking mock with @mainactor for consistency.The mock now has mutable state (
state,errorReason,listeners) and methods that use@MainActorcallbacks. Per coding guidelines, stateful objects should be marked with@MainActor. While this may be more flexible for test code, marking the mock with@MainActorwould ensure thread-safety and align with the guidelines.As per coding guidelines.
Apply this diff to add
@MainActorisolation:import Ably @testable import AblyChat +@MainActor final class MockConnection: InternalConnectionProtocol { var state: ARTRealtimeConnectionState
📜 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 (3)
Sources/AblyChat/DefaultConnection.swift(2 hunks)Tests/AblyChatTests/DefaultConnectionTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockConnection.swift(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.swift: Use protocol-based design; expose SDK functionality via protocols and prefer associated types with opaque return types (some Protocol) instead of existentials (any Protocol)
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API must use typed throws with ErrorInfo; use InternalError internally and convert at the public API boundary
For public structs emitted by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
Task, CheckedContinuation, and AsyncThrowingStream do not support typed errors; use Result and call .get() to surface typed errors
Do not use Dictionary.mapValues for typed throws; use ablyChat_mapValuesWithTypedThrow instead
When the compiler struggles with typed throws, explicitly declare the error type on do blocks (e.g., do throws(InternalError))
Specify error types in closures using the throws(ErrorType) syntax (e.g., try items.map { jsonValue throws(InternalError) in ... })
Mark any test-only APIs with testsOnly_ prefix and wrap them in #if DEBUG
Files:
Tests/AblyChatTests/DefaultConnectionTests.swiftSources/AblyChat/DefaultConnection.swiftTests/AblyChatTests/Mocks/MockConnection.swift
Tests/AblyChatTests/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Tests/AblyChatTests/**/*.swift: Use test attribution tags to reference spec coverage: @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 until Xcode 16.3+
Files:
Tests/AblyChatTests/DefaultConnectionTests.swiftTests/AblyChatTests/Mocks/MockConnection.swift
Tests/AblyChatTests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place tests under Tests/AblyChatTests/ as the root for test targets
Files:
Tests/AblyChatTests/DefaultConnectionTests.swiftTests/AblyChatTests/Mocks/MockConnection.swift
Tests/AblyChatTests/Mocks/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Put mock implementations under Tests/AblyChatTests/Mocks/
Files:
Tests/AblyChatTests/Mocks/MockConnection.swift
🧬 Code graph analysis (2)
Tests/AblyChatTests/DefaultConnectionTests.swift (2)
Sources/AblyChat/DefaultConnection.swift (1)
onStatusChange(22-53)Tests/AblyChatTests/Mocks/MockConnection.swift (2)
emit(27-50)off(22-24)
Tests/AblyChatTests/Mocks/MockConnection.swift (2)
Sources/AblyChat/AblyCocoaExtensions/InternalAblyCocoaTypes.swift (5)
on(261-265)on(473-477)on(479-483)off(267-269)off(509-511)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (3)
on(166-169)on(171-177)off(197-199)
⏰ 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). (12)
- GitHub Check: Example app, tvOS (Xcode 26.0)
- GitHub Check: Example app, macOS (Xcode 26.0)
- GitHub Check: Xcode, tvOS (Xcode 26.0)
- GitHub Check: Xcode, iOS (Xcode 26.0)
- GitHub Check: Example app, iOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 26.0) - GitHub Check: Xcode, macOS (Xcode 26.0)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 26.0) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 26.0) - GitHub Check: SPM,
releaseconfiguration (Xcode 26.0) - GitHub Check: SPM (Xcode 26.0)
- GitHub Check: lint
🔇 Additional comments (6)
Sources/AblyChat/DefaultConnection.swift (2)
26-28: Guard pattern is valid but serves only as a lifecycle check.The change from
guard let selftoguard self != nilis correct sinceselfis not used after the guard. This pattern ensures the callback stops executing if theDefaultConnectionis deallocated, which is a reasonable lifecycle management approach.
47-52: Verify listener cleanup lifecycle is intentional.The subscription cleanup also uses
weak self, which means ifDefaultConnectionis deallocated before the user callssubscription.off(), the listener will remain registered on the underlying realtime connection (though the closure will return early due to the guard check at line 26). This could lead to listener accumulation on the realtime connection object.An alternative pattern would be to track registered listeners and clean them up in a
deinitmethod. However, the current pattern is simpler and places lifecycle management responsibility on the caller.Please confirm this lifecycle pattern is intentional and that callers are expected to manage subscription cleanup explicitly.
Tests/AblyChatTests/DefaultConnectionTests.swift (3)
9-98: LGTM! Comprehensive status mapping coverage.The CHA-CS1 tests thoroughly cover all connection status mappings (initialized, connecting, connected, disconnected, suspended, failed) with proper @SPEC attribution. The test structure is clear with Given/When/Then comments, and the use of mocks is appropriate.
105-142: LGTM! Status and error exposure properly tested.The CHA-CS2/CHA-CS3 tests verify that the connection properly exposes its current status and error. Both scenarios (with and without error) are covered, and the tests correctly validate error properties (code, message, statusCode).
153-195: LGTM! Comprehensive listener lifecycle test.This test thoroughly validates the complete listener behavior including registration, event notification with correct current/previous status and error, and unsubscription. The use of
MockConnection.emit()to simulate state changes is appropriate, and the test properly verifies that unsubscription stops further event delivery.Tests/AblyChatTests/Mocks/MockConnection.swift (1)
27-50: LGTM! Well-designed test helper.The
emit()helper properly simulates connection state changes by:
- Capturing the previous state before transitioning
- Updating current state and error as needed
- Constructing a proper
ConnectionStateChangeobject- Notifying all registered listeners
The
retryIn ?? 0behavior is consistent with the known limitation documented in the TODO comment at line 39-40 ofDefaultConnection.swift.
|
@AndyTWF is going to make some changes to connection status soon (reintroducing |
| struct DefaultConnectionTests { | ||
| // MARK: - CHA-CS1: Connection Status Values | ||
|
|
||
| // @spec CHA-CS1a |
There was a problem hiding this comment.
These spec references are generating warnings about non-testable spec points in the spec coverage report. Isn't CHA-CS3 the relevant spec point here?
There was a problem hiding this comment.
Oh, I've missed that the whole CHA-CS1 is non-testable. Removed.
github-actions
bot
temporarily deployed
to
staging/pull/464/AblyChat
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Tests/AblyChatTests/DefaultConnectionTests.swift (1)
90-91: Consider checking statusCode for consistency.The error validation here only checks
codeandmessage, but the test at lines 46-48 also validatesstatusCode. Since theconnectionErrorincludesstatusCode: 500(line 69), consider adding that assertion for consistency:#expect(statusChange.error?.code == connectionError.code) #expect(statusChange.error?.message == connectionError.message) +#expect(statusChange.error?.statusCode == connectionError.statusCode)
📜 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 (1)
Tests/AblyChatTests/DefaultConnectionTests.swift(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.swift: Use protocol-based design; expose SDK functionality via protocols and prefer associated types with opaque return types (some Protocol) instead of existentials (any Protocol)
Isolate all mutable state to the main actor; mark stateful objects with @mainactor
Public API must use typed throws with ErrorInfo; use InternalError internally and convert at the public API boundary
For public structs emitted by the API, provide an explicit public memberwise initializer
When using AsyncSequence operators in @mainactor contexts, mark operator closures as @sendable
Task, CheckedContinuation, and AsyncThrowingStream do not support typed errors; use Result and call .get() to surface typed errors
Do not use Dictionary.mapValues for typed throws; use ablyChat_mapValuesWithTypedThrow instead
When the compiler struggles with typed throws, explicitly declare the error type on do blocks (e.g., do throws(InternalError))
Specify error types in closures using the throws(ErrorType) syntax (e.g., try items.map { jsonValue throws(InternalError) in ... })
Mark any test-only APIs with testsOnly_ prefix and wrap them in #if DEBUG
Files:
Tests/AblyChatTests/DefaultConnectionTests.swift
Tests/AblyChatTests/**/*.swift
📄 CodeRabbit inference engine (CLAUDE.md)
Tests/AblyChatTests/**/*.swift: Use test attribution tags to reference spec coverage: @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 until Xcode 16.3+
Files:
Tests/AblyChatTests/DefaultConnectionTests.swift
Tests/AblyChatTests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place tests under Tests/AblyChatTests/ as the root for test targets
Files:
Tests/AblyChatTests/DefaultConnectionTests.swift
🧬 Code graph analysis (1)
Tests/AblyChatTests/DefaultConnectionTests.swift (2)
Sources/AblyChat/DefaultConnection.swift (1)
onStatusChange(22-53)Tests/AblyChatTests/Mocks/MockConnection.swift (2)
emit(27-50)off(22-24)
🔇 Additional comments (3)
Tests/AblyChatTests/DefaultConnectionTests.swift (3)
5-27: LGTM! Well-structured initialization test.The test correctly verifies the initial connection status and error state. Using a real ARTRealtime instance (instead of mocks) is appropriate here since you're testing the integration of the initial state without any state transitions.
29-49: LGTM! Thorough error exposure test.The test correctly verifies that connection errors are properly exposed through the API. The individual field assertions (code, message, statusCode) provide clear validation.
51-102: Excellent comprehensive listener test!This test thoroughly covers the connection status observation lifecycle:
- Registration and event delivery (CHA-CS4a/b/c/d)
- Proper state transitions with error propagation
- Unsubscription behavior (CHA-CS4e)
- Alignment with underlying realtime library (CHA-CS5c)
The use of Given/When/Then comments enhances readability.
maratal
force-pushed
the
393-remove-transient-disconnect
branch
from
76b0632 to
5311848
Compare
Remove transient disconnect handling from Connection.
Add tests for Connection.
Closes #393
Summary by CodeRabbit
Bug Fixes
Tests
Documentation