refactor(core, ui): improve app lifecycle and ws connectivity handling #2339
Conversation
This commit introduces several improvements to how the StreamChatCore widget handles app lifecycle and network connectivity changes: - **Refactored Lifecycle Management:** The logic for handling app lifecycle states (foreground, background) and network connectivity changes has been extracted into a new `_ChatLifecycleManager` class. This improves code organization and readability. - **Improved Reconnection Logic:** The client now attempts to reconnect immediately (bypassing retry delays) when the app returns to the foreground or when network connectivity is restored. This is achieved through the new `client.maybeReconnect()` method. - **Simplified Disconnection Logic:** The `client.maybeDisconnect()` method is now used to ensure the client disconnects only when necessary (e.g., when connectivity is lost or the app goes to the background without a background event handler). - **Enhanced Test Coverage:** Unit tests have been updated and expanded to cover the new lifecycle and connectivity management logic. These changes result in a more robust and responsive chat experience, ensuring the client connects and disconnects appropriately based on app state and network conditions.
This commit refactors how connection status is managed between `StreamChatClient` and `WebSocketConnection`. - `StreamChatClient` now directly consumes the `connectionStatusStream` from `WebSocketConnection` instead of managing its own `BehaviorSubject`. - `WebSocketConnection` now exposes a `dispose` method to properly close its `BehaviorSubject` and other resources. - `StreamChatClient.dispose` now correctly calls `disconnectUser` which in turn calls `WebSocketConnection.dispose`. - `StreamChatClient.closeConnection` no longer redundantly sets its own connection status as it's now derived from the web socket. - Sample app is updated to reflect the client disposal change: `client.dispose()` is no longer called directly after `disconnectUser` in the channel list page as `disconnectUser` handles it. `client.dispose()` is called in the `App` widget's `dispose` method.
The FakeWebSocket in tests was not being disposed, which could lead to test flakiness. This commit fixes the issue by disposing the FakeWebSocket in the tearDown method of the tests.
WalkthroughThis set of changes refactors connection and lifecycle management for StreamChat clients and widgets. Connection status handling is centralized in the WebSocket layer, removing duplicated state in the client. Lifecycle and connectivity logic is modularized into a dedicated manager. Tests and sample app code are updated to align with the new architecture and resource management patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant StreamChatCore
participant _ChatLifecycleManager
participant StreamChatClient
participant WebSocket
App->>StreamChatCore: AppLifecycleState.paused
StreamChatCore->>_ChatLifecycleManager: onAppLifecycleChanged(paused)
_ChatLifecycleManager->>StreamChatClient: maybeDisconnect()
StreamChatClient->>WebSocket: closeConnection()
App->>StreamChatCore: AppLifecycleState.resumed
StreamChatCore->>_ChatLifecycleManager: onAppLifecycleChanged(resumed)
_ChatLifecycleManager->>StreamChatClient: maybeReconnect()
StreamChatClient->>WebSocket: openConnection()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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 (
|
![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
🔭 Outside diff range comments (1)
packages/stream_chat/test/src/client/client_test.dart (1)
28-30: Missingawaitkeyword for async dispose call.The
tearDownmethod should useawait client.dispose()to match the async pattern used in other test groups (lines 515, 828) and ensure proper resource cleanup.tearDown(() { - client.dispose(); + await client.dispose(); });
🧹 Nitpick comments (1)
packages/stream_chat/lib/src/ws/websocket.dart (1)
501-509: Redundant unsubscribe call in dispose method.The
dispose()method calls_unsubscribeFromWebSocketChannel()on line 506, but_closeWebSocketChannel()on line 507 also calls it internally (see line 136). Consider removing the redundant call:Future<void> dispose() async { _logger?.info('Disposing web-socket connection'); _stopMonitoringEvents(); - _unsubscribeFromWebSocketChannel(); _closeWebSocketChannel(); _connectionStatusController.close(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/stream_chat/lib/src/client/client.dart(5 hunks)packages/stream_chat/lib/src/ws/websocket.dart(2 hunks)packages/stream_chat/test/src/client/client_test.dart(4 hunks)packages/stream_chat/test/src/fakes.dart(3 hunks)packages/stream_chat_flutter/lib/src/channel/channel_info.dart(1 hunks)packages/stream_chat_flutter/lib/src/channel/channel_list_header.dart(1 hunks)packages/stream_chat_flutter_core/lib/src/stream_chat_core.dart(4 hunks)packages/stream_chat_flutter_core/test/stream_chat_core_test.dart(1 hunks)sample_app/lib/app.dart(1 hunks)sample_app/lib/pages/channel_list_page.dart(0 hunks)
💤 Files with no reviewable changes (1)
- sample_app/lib/pages/channel_list_page.dart
⏰ 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). (10)
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat
- GitHub Check: stream_chat_persistence
- GitHub Check: build (android)
- GitHub Check: build (ios)
- GitHub Check: test
- GitHub Check: analyze_legacy_versions
- GitHub Check: analyze
🔇 Additional comments (20)
packages/stream_chat_flutter/lib/src/channel/channel_list_header.dart (1)
273-273: LGTM! Improved abstraction for reconnection logic.The replacement of explicit connection management calls with
client.maybeReconnectis a good refactoring that encapsulates the reconnection logic and aligns with the broader connection management improvements across the codebase.sample_app/lib/app.dart (1)
438-438: LGTM! Proper resource cleanup added.The addition of client disposal ensures proper cleanup of resources when the widget is disposed, preventing memory leaks. The use of optional chaining is appropriate since
initDatamight be null.packages/stream_chat_flutter/lib/src/channel/channel_info.dart (1)
186-186: LGTM! Consistent application of reconnection abstraction.The replacement with
client.maybeReconnectmaintains consistency with the same pattern applied inchannel_list_header.dart, demonstrating good architectural coherence across UI components.packages/stream_chat/test/src/client/client_test.dart (8)
23-26: LGTM! Improved test isolation with local WebSocket instances.Creating fresh
FakeWebSocketinstances insetUpinstead of sharing group-level instances improves test isolation and prevents potential test pollution.
505-505: LGTM! Consistent test isolation improvement.Same good practice of using local WebSocket instances for better test isolation.
514-516: LGTM! Proper async resource cleanup.Correctly implemented async
tearDownwithawait client.dispose()ensures proper resource cleanup and aligns with the updated client disposal flow.
819-819: LGTM! Consistent test isolation pattern.Another properly implemented local WebSocket instance for test isolation.
827-829: LGTM! Consistent async cleanup pattern.Properly implemented async
tearDownwithawait client.dispose()consistent with the updated architecture.
485-485: LGTM! Consistent WebSocket instance management.Local WebSocket instance creation maintains the improved test isolation pattern throughout the file.
490-494: LGTM! Proper async tearDown implementation.Correctly implemented async
tearDownwith properawait client.dispose()call ensures resources are properly cleaned up.
152-168: Inconsistency with AI summary regarding removed test case.The AI summary mentions that "a test case verifying an error when opening a connection while one is already in progress was removed," but this test case still exists in the code. This suggests the summary may be inaccurate about which specific test was removed.
Likely an incorrect or invalid review comment.
packages/stream_chat/lib/src/ws/websocket.dart (1)
108-122: Good refactoring of connection status management.The changes improve code clarity and safety:
- Making
_connectionStatusControllerfinal and eagerly initialized prevents null issues- Using
safeAddprevents errors when adding to a closed controller- Adding
distinct()to the stream prevents duplicate connection status eventspackages/stream_chat/test/src/fakes.dart (2)
120-163: Well-structured test fake implementation.The changes properly mirror the real WebSocket implementation:
- Connection status controller lifecycle matches production code
- Separation of
disconnect()anddispose()responsibilities is clean- Test fakes maintain high fidelity with the actual implementation
167-205: Consistent implementation across fake classes.The
FakeWebSocketWithConnectionErrorfollows the same pattern asFakeWebSocket, maintaining consistency in the test infrastructure.packages/stream_chat_flutter_core/test/stream_chat_core_test.dart (1)
131-330: Comprehensive lifecycle behavior test coverage.The test suite provides excellent coverage of the refactored lifecycle management:
- Well-structured with shared setup/teardown
- Good use of helper method
pumpStreamChatCoreto reduce duplication- Tests cover all key scenarios: backgrounding, foregrounding, connectivity changes, and background event handling
- Proper use of
clearInteractions()to ensure test isolationpackages/stream_chat_flutter_core/lib/src/stream_chat_core.dart (2)
277-356: Well-designed lifecycle manager with clear separation of concerns.The
_ChatLifecycleManagerclass effectively encapsulates lifecycle and connectivity management. The implementation is clean and follows good practices:
- Clear state tracking with
_isInBackground- Proper timer and subscription management
- Good resource cleanup in
dispose()One consideration: The early return in
onConnectivityChanged(line 334) when the app is in background might miss important connectivity state changes. Consider tracking the last connectivity state to handle it when the app returns to foreground.
358-387: Excellent API design with the MaybeReconnect extension.The extension methods provide a clean API for conditional connection management:
- Proper guards prevent unnecessary operations
- The pattern of closing before reopening ensures immediate reconnection without retry delays
- Clear documentation explains the purpose
packages/stream_chat/lib/src/client/client.dart (3)
242-248: Clean delegation of connection status to WebSocket.Removing the internal connection status controller and delegating directly to the WebSocket instance eliminates state duplication and potential synchronization issues. This follows the single source of truth principle.
444-446: Correct handling of all connection status events.Removing
skip(1)ensures the initial connection status is properly handled, which is important for state consistency.
2029-2035: Proper resource cleanup in dispose method.The updated
dispose()method follows the correct cleanup order:
- High-level cleanup with
disconnectUser()- Low-level cleanup with
_ws.dispose()- Close event streams
This ensures all resources are properly released.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2339 +/- ##
==========================================
+ Coverage 63.74% 63.76% +0.02%
==========================================
Files 411 411
Lines 25682 25694 +12
==========================================
+ Hits 16370 16383 +13
+ Misses 9312 9311 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This commit introduces a new parameter `maxReconnectAttempts` to `WebSocketOptions`. This allows users to configure the maximum number of times the WebSocket will attempt to reconnect after a disconnection. The default value is 6 attempts, which corresponds to approximately 30 seconds of reconnection attempts. If the maximum number of attempts is reached, the WebSocket will stop trying to reconnect and will disconnect.
This commit refactors how `StreamChatClient` handles connection status changes from its underlying `WebSocketConnection`. Previously, `StreamChatClient` would create a new subscription to the `wsConnectionStatusStream` every time `connectUser` was called and cancel it in `closeConnection`. It also had its own logic to track the previous connection state to determine if a `connectionChanged` event should be emitted. With this change: - A single, long-lived subscription to `wsConnectionStatusStream` is created in the `StreamChatClient` constructor. This subscription is now disposed of in the `dispose` method. - The `wsConnectionStatusStream` now uses `.distinct()` to only emit when the status actually changes, simplifying the client-side logic. - The client's `_onConnectionStatusChanged` (formerly `_connectionStatusHandler`) now receives both the previous and current status directly from the stream (using `.pairwise()`). This removes the need for the client to manually track the previous state. - The logic for emitting `EventType.connectionChanged` is now more straightforward, directly using the `isConnected` and `wasConnected` states. These changes lead to cleaner and more robust connection status management within the client.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/stream_chat/lib/src/client/client.dart (1)
126-131: Consider error handling for async connection status changes.The
_onConnectionStatusChangedmethod is async but not awaited in the subscription. Consider adding error handling to prevent unhandled exceptions during connection recovery:_connectionStatusSubscription = wsConnectionStatusStream.pairwise().listen( - (statusPair) { + (statusPair) async { final [prevStatus, currStatus] = statusPair; - return _onConnectionStatusChanged(prevStatus, currStatus); + try { + await _onConnectionStatusChanged(prevStatus, currStatus); + } catch (e, stk) { + logger.severe('Error handling connection status change', e, stk); + } }, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/stream_chat/lib/src/client/client.dart(6 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). (10)
- GitHub Check: stream_chat
- GitHub Check: stream_chat_flutter
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_persistence
- GitHub Check: analyze_legacy_versions
- GitHub Check: build (android)
- GitHub Check: build (ios)
- GitHub Check: test
- GitHub Check: analyze
🔇 Additional comments (6)
packages/stream_chat/lib/src/client/client.dart (6)
126-132: Excellent refactor to centralize connection status management!The use of
pairwise()to track connection status transitions is a clean architectural improvement. This approach provides better control over status changes and should help prevent the infinite reconnection loops mentioned in the PR objectives.
230-230: Good type change reflecting the new architecture.The field type correctly reflects the subscription to the pairwise connection status stream, moving away from manual state management to reactive subscription-based handling.
249-255: Clean delegation to WebSocket layer connection status.The direct delegation to
_ws.connectionStatusand_ws.connectionStatusStream.distinct()removes unnecessary state duplication and ensures consistency. Thedistinct()call is particularly important for preventing duplicate status change events.
501-545: Excellent centralized connection status change handling.This method provides comprehensive status transition management:
- Proper early return to avoid unnecessary processing
- Clear connection recovery logic with channel queries and sync
- Appropriate event emissions for connection state changes
This implementation should significantly help prevent infinite reconnection loops by providing controlled, centralized status transition handling.
2019-2033: Improved disconnection flow with explicit connection closure.The explicit
closeConnection()call ensures proper WebSocket cleanup at the start of the disconnection process. The simplified flow removes manual connection status management while maintaining proper cleanup of state, tokens, and persistence.
2036-2043: Comprehensive resource cleanup in dispose method.The dispose method now properly handles all resource cleanup:
- User disconnection and state cleanup
- WebSocket disposal with
_ws.dispose()- Connection status subscription cancellation
- Event controller closure
This should prevent resource leaks and align with the improved lifecycle management objectives.
| StreamSubscription<ConnectionStatus>? _connectionStatusSubscription; | ||
| StreamSubscription<List<ConnectionStatus>>? _connectionStatusSubscription; |
There was a problem hiding this comment.
Isn't it better to make this a (named) record instead of a list? Should also be 2 states right?
There was a problem hiding this comment.
Yeah, but then we have to map the value to a record before listening to it. Also the type is not really useful for us here.
There was a problem hiding this comment.
Ah right, I thought the pairwise() was something from us, but it's a feature from rxdart.
Submit a pull request
Fixes: FLU-215
Screen_recording_20250731_180320.mp4
Summary by CodeRabbit
Summary by CodeRabbit
Refactor
Bug Fixes
Tests