fix(ui): prevent cooldown resume crash if channel state is null #2343
Conversation
This commit addresses a potential crash that could occur when accessing properties like `cooldown`, `getRemainingCooldown`, or `cooldownStream` on a `Channel` instance that has been disposed. Previously, `_checkInitialized` only verified if the `_initializedCompleter` was completed. This did not account for scenarios where the channel was initialized and then subsequently disposed. The fix involves: - Setting `state` to `null` in the `dispose` method. - Updating `_checkInitialized` to also check if `state` is not `null`. If the channel is not initialized or has been disposed, it now throws a more informative `StateError`. This ensures that any attempt to access state-dependent properties on a disposed channel will result in a clear error message rather than a null pointer exception. Additionally, new tests have been added to verify: - `StateError` is thrown when accessing cooldown properties on a non-initialized channel. - Cooldown properties work as expected on an initialized channel. - `StateError` is thrown when accessing cooldown properties on a disposed channel. - A specific race condition scenario involving rapid initialization and disposal is handled correctly by throwing `StateError`.
This commit addresses a potential null pointer exception in `StreamMessageInput`. Previously, when the `StreamMessageInput` was initialized and not in editing mode, it would attempt to resume a message cooldown by calling `channel.getRemainingCooldown()`. However, if `channel.state` was null (e.g., if the channel hadn't been fully initialized or loaded yet), this would lead to a crash. The fix adds a check to ensure `channel.state` is not null before attempting to resume the cooldown. This prevents the crash and ensures the cooldown logic is only applied when the channel state is available.
WalkthroughThis change improves error handling for the Changes
Sequence Diagram(s)sequenceDiagram
participant UI as StreamMessageInputState
participant Channel
participant ChannelState
UI->>Channel: getRemainingCooldown()
alt Channel.state is null or not initialized
Channel-->>UI: throw StateError
else Channel.state is available
Channel->>ChannelState: get remaining cooldown
ChannelState-->>Channel: cooldown value
Channel-->>UI: return cooldown value
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. 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. 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_flutter/lib/src/message_input/stream_message_input.dart (1)
1508-1513:channel.state!will still crash insendMessage()when the channel is not initialised
sendMessage()dereferenceschannel.state!(Line 1510) without the same null-safety that was just added in_initializeState(). A user can reach this code path because the input is rendered even whenchannel.state == null(see_buildAutocompleteMessageInput).- // If the channel is not up to date, we should reload it before sending - // the message. - if (!channel.state!.isUpToDate) { + // Bail out or wait until the channel is initialised. + final channelState = channel.state; + if (channelState == null) { + // Option 1: silently wait until state arrives + await streamChannel.reloadChannel(); + await WidgetsBinding.instance.endOfFrame; + } else if (!channelState.isUpToDate) { await streamChannel.reloadChannel(); await WidgetsBinding.instance.endOfFrame; }If silently waiting is undesirable, return early with an error instead.
Either way, remove the!operator.
🧹 Nitpick comments (2)
packages/stream_chat/CHANGELOG.md (1)
1-7: Consider adding an issue / PR reference for traceabilityThe new entry is clear, but including a link to the fixing PR (e.g.
[#2338]) or the commit hash helps consumers track the change back to source and improves auditability.packages/stream_chat_flutter/CHANGELOG.md (1)
7-8: Grammar-nit: subject/verb agreement“
Fixed StreamMessageInput cooldown resume error …” reads a bit awkwardly.
Consider “Fixed a cooldown-resume error in StreamMessageInput …” for smoother phrasing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/stream_chat/CHANGELOG.md(1 hunks)packages/stream_chat/lib/src/client/channel.dart(1 hunks)packages/stream_chat/test/src/client/channel_test.dart(2 hunks)packages/stream_chat_flutter/CHANGELOG.md(1 hunks)packages/stream_chat_flutter/lib/src/message_input/stream_message_input.dart(1 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: build (ios)
- GitHub Check: build (android)
- GitHub Check: test
- GitHub Check: analyze
- GitHub Check: stream_chat_flutter_core
- GitHub Check: stream_chat_localizations
- GitHub Check: stream_chat_persistence
- GitHub Check: stream_chat
- GitHub Check: stream_chat_flutter
- GitHub Check: analyze_legacy_versions
🔇 Additional comments (7)
packages/stream_chat_flutter/lib/src/message_input/stream_message_input.dart (1)
574-576: Null-check before cooldown resumption is spot-onThe additional
channel.state != nullguard correctly prevents the NPE reported in #2338.packages/stream_chat/lib/src/client/channel.dart (2)
2052-2052: LGTM! Excellent defensive programming practice.Explicitly setting
state = nullafter disposal ensures the channel's disposed state is clearly detectable and helps prevent potential memory leaks. This change works well with the updated_checkInitializedmethod to provide robust error handling.
2057-2064: Excellent error handling improvement that directly addresses the PR objectives!This change effectively prevents the crash described in issue #2338 by:
- Dual validation: Now checks both initialization completion AND non-null state, preventing race conditions where cooldown operations are attempted on channels with null state
- Better error handling: Using
StateErrorinstead of assertions provides catchable exceptions and clearer debugging information- Clear guidance: The error message provides actionable guidance for developers
This robust validation ensures that all channel operations, including
getRemainingCooldown(), will fail safely with a descriptive error rather than crashing with null pointer exceptions.packages/stream_chat/test/src/client/channel_test.dart (4)
5099-5124: Well-structured test group setup.The test group setup follows established patterns and properly initializes all required mocks for comprehensive testing of channel state validation and cooldown functionality.
5125-5149: Comprehensive validation for non-initialized channel state.The tests properly verify that accessing cooldown-related properties and methods on non-initialized channels throws
StateErrorinstead of causing null pointer exceptions. This directly addresses the crash prevention objective described in the PR.
5151-5187: Thorough testing of initialized channel cooldown functionality.The tests comprehensively verify that initialized channels correctly handle cooldown-related operations, including default values, custom cooldown settings, remaining cooldown calculations, and stream behavior. The use of separate test fixtures for custom cooldown testing is a good practice.
5189-5259: Excellent coverage of disposed channel state validation.These tests directly address the production crash scenario by verifying that disposed channels throw
StateErrorinstead of causing null pointer exceptions. The race condition test (lines 5239-5258) is particularly valuable as it simulates the exact crash scenario described in the PR objectives where rapid navigation could cause theStreamMessageInputto access null channel state.The pattern of verifying functionality before disposal, then verifying error handling after disposal, provides robust validation of the state transition behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2343 +/- ##
==========================================
+ Coverage 63.76% 63.80% +0.04%
==========================================
Files 411 411
Lines 25694 25696 +2
==========================================
+ Hits 16383 16396 +13
+ Misses 9311 9300 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Submit a pull request
Fixes: #2338
Description of the pull request
This PR addresses a potential null pointer exception in
StreamMessageInput.Previously, when the
StreamMessageInputwas initialized and not in editing mode, it would attempt to resume a message cooldown by callingchannel.getRemainingCooldown(). However, ifchannel.statewas null (e.g., if the channel hadn't been fully initialized or loaded yet), this would lead to a crash.The fix adds a check to ensure
channel.stateis not null before attempting to resume the cooldown. This prevents the crash and ensures the cooldown logic is only applied when the channel state is available.Summary by CodeRabbit
Bug Fixes
Tests
Documentation