chore(llc): send sfu stats for thermal and low power mode#1221
chore(llc): send sfu stats for thermal and low power mode#1221
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughAdds device-level tracing for thermal and battery mode transitions to the SFU stats reporter (gated by Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stream_video/lib/src/call/stats/sfu_stats_reporter.dart (1)
36-49:⚠️ Potential issue | 🟡 MinorInitial thermal state is never traced.
_deviceTracer.trace('device.thermalState', ...)only fires inside theonThermalStatusChangedlistener, so the thermal state at the moment the reporter starts (or any state the device is already in when subscribing) is not captured in traces — only subsequent transitions are. This mirrors the_lastLowPowerModepattern, but unlike low-power-mode (which is polled eachsendSfuStatscycle), thermal state will appear absent from traces for devices that never transition during the call.Consider emitting an initial trace after subscribing, e.g. by reading/seeding the current
ThermalStatusonce, or by tracing it on the firstsendSfuStatscycle alongside the power-mode trace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stream_video/lib/src/call/stats/sfu_stats_reporter.dart` around lines 36 - 49, The reporter subscribes to Thermal().onThermalStatusChanged but never emits the current thermal state at startup, so initial state is missing from traces; after creating _thermalStatusSubscription (or immediately after the try block succeeds) read the current ThermalStatus (e.g., call Thermal().thermalStatus or otherwise obtain the current status) and set _thermalStatus and call _deviceTracer.trace('device.thermalState', _thermalStatus.name) to seed the initial value; alternatively, ensure sendSfuStats emits the thermal state if _thermalStatus is null (similar to the _lastLowPowerMode pattern) so the first sendSfuStats call traces the initial thermal state.
🧹 Nitpick comments (1)
packages/stream_video/lib/src/call/stats/sfu_stats_reporter.dart (1)
115-121:try/catchin_invokeSendStatscannot catch async failures.
sendSfuStats()returns aFutureand is not awaited here, so any asynchronous error it raises will not be intercepted by this synchronoustry/catch— only errors thrown synchronously before the firstawaitwould be. In practicesendSfuStatsalready wraps its body in its owntry/catch, so this wrapper is effectively dead code. Consider either removing it, or switching to.catchError/unawaited(...)with a proper async handler to make the intent explicit.♻️ Suggested simplification
- void _invokeSendStats() { - try { - sendSfuStats(); - } catch (e) { - _logger.v(() => 'Failed to send SFU stats'); - } - } + void _invokeSendStats() { + unawaited(sendSfuStats()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stream_video/lib/src/call/stats/sfu_stats_reporter.dart` around lines 115 - 121, The try/catch in _invokeSendStats cannot catch async errors from sendSfuStats because sendSfuStats returns a Future; either remove the dead synchronous try/catch or make the intent explicit: 1) if you want fire-and-forget, call sendSfuStats().catchError((e, st) => _logger.v(() => 'Failed to send SFU stats: $e')) (or wrap with unawaited(...) and attach a catchError), or 2) if you want to await and handle errors inline, make _invokeSendStats async and use try { await sendSfuStats(); } catch (e, st) { _logger.v(() => 'Failed to send SFU stats: $e'); } — update the code around _invokeSendStats and sendSfuStats accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/stream_video/lib/src/call/stats/sfu_stats_reporter.dart`:
- Around line 36-49: The reporter subscribes to Thermal().onThermalStatusChanged
but never emits the current thermal state at startup, so initial state is
missing from traces; after creating _thermalStatusSubscription (or immediately
after the try block succeeds) read the current ThermalStatus (e.g., call
Thermal().thermalStatus or otherwise obtain the current status) and set
_thermalStatus and call _deviceTracer.trace('device.thermalState',
_thermalStatus.name) to seed the initial value; alternatively, ensure
sendSfuStats emits the thermal state if _thermalStatus is null (similar to the
_lastLowPowerMode pattern) so the first sendSfuStats call traces the initial
thermal state.
---
Nitpick comments:
In `@packages/stream_video/lib/src/call/stats/sfu_stats_reporter.dart`:
- Around line 115-121: The try/catch in _invokeSendStats cannot catch async
errors from sendSfuStats because sendSfuStats returns a Future; either remove
the dead synchronous try/catch or make the intent explicit: 1) if you want
fire-and-forget, call sendSfuStats().catchError((e, st) => _logger.v(() =>
'Failed to send SFU stats: $e')) (or wrap with unawaited(...) and attach a
catchError), or 2) if you want to await and handle errors inline, make
_invokeSendStats async and use try { await sendSfuStats(); } catch (e, st) {
_logger.v(() => 'Failed to send SFU stats: $e'); } — update the code around
_invokeSendStats and sendSfuStats accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6726338-8716-4b88-9a7a-1ad13f50cee3
📒 Files selected for processing (1)
packages/stream_video/lib/src/call/stats/sfu_stats_reporter.dart
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1221 +/- ##
========================================
+ Coverage 5.77% 5.79% +0.02%
========================================
Files 676 676
Lines 49335 49345 +10
========================================
+ Hits 2848 2860 +12
+ Misses 46487 46485 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit