fix lifecycle cleanup issues#222
Conversation
|
Visit the preview URL for this PR (updated for commit b355310): https://open-earable-web--pr222-ui-redesign-fixes-osyvhx8y.web.app (expires Mon, 06 Apr 2026 10:55:28 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c7397c11177c71d8d81172cea9365829823fb41c |
There was a problem hiding this comment.
Pull request overview
Improves lifecycle cleanup for wearable-related providers to reduce resource leaks during reconnect/disconnect cycles and long-running app sessions.
Changes:
- Add explicit disposal logic to
WearablesProvider, including canceling subscriptions, disposing child providers, and closing event stream controllers. - Add disposal logic to
SensorRecorderProviderto stop recorders and clear internal state on teardown. - Update
SensorStreams.sharedto evict cached broadcast streams when the stream is canceled.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| open_wearable/lib/view_models/wearables_provider.dart | Adds disposed-guarding and a full dispose() implementation; routes event emission through helper methods. |
| open_wearable/lib/view_models/sensor_recorder_provider.dart | Adds dispose() to stop per-wearable recorders and clear recorder state. |
| open_wearable/lib/models/sensor_streams.dart | Removes cached shared stream entries when the broadcast stream is canceled. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ), | ||
| ); | ||
| } | ||
| if (_disposed) { |
There was a problem hiding this comment.
The _disposed guard in _handleCapabilitiesChanged is currently after side effects (e.g., initializing sensor configs / scheduling work). If a capability event arrives during teardown or after removeWearable, this handler can recreate per-wearable state even though the provider is disposed/removed. Add the guard at the start of the method (and consider also checking the wearable is still tracked, e.g., present in _capabilitySubscriptions/_wearables) before doing any work.
| return sensor.sensorStream.asBroadcastStream( | ||
| onCancel: (_) { | ||
| _sharedStreams.remove(sensor); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
SensorStreams.shared only evicts the cached entry via onCancel. If the underlying sensor.sensorStream completes (e.g., because a device disconnects) without listeners explicitly canceling first, onCancel won’t run and _sharedStreams can retain a closed stream, causing future callers to receive an already-terminated stream. Consider wrapping the source stream so you also remove the cache entry on onDone/onError (or otherwise ensure clearForSensor is invoked on disconnect).
| static Stream<SensorValue> shared(Sensor sensor) { | ||
| return _sharedStreams.putIfAbsent( | ||
| sensor, | ||
| () => sensor.sensorStream.asBroadcastStream(), | ||
| ); | ||
| return _sharedStreams.putIfAbsent(sensor, () { | ||
| return sensor.sensorStream.asBroadcastStream( | ||
| onCancel: (_) { | ||
| _sharedStreams.remove(sensor); | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This change modifies the lifecycle semantics of the shared-stream cache (eviction on cancel). There doesn’t appear to be a unit test covering SensorStreams.shared cache insertion/eviction behavior, which makes regressions in long-session/reconnect scenarios hard to catch. Consider adding a small test that verifies the same stream instance is reused while active and that the cache entry is removed after the last listener cancels.
Prevents resource leaks during reconnect/disconnect cycles and long app sessions.