chore(llc): call stats improvements #1053
Conversation
WalkthroughReplaces legacy per-connection stats with a StatsReporter/CallMetrics pipeline: Call exposes call.statsReporter, session creates and wires StatsReporter, CallState drops publisher/subscriber/local/latency fields, dogfooding UI consumes CallMetrics and adds battery/thermal charts and UI bindings. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as CallStatsScreen
participant Call as Call
participant Session as CallSession
participant SR as StatsReporter
participant RTC as RTCManager
participant Device as Device
UI->>Call: subscribe to statsReporter?.stream (initialData: currentMetrics)
Call-->>UI: emits CallMetrics updates
Note over Session,SR: Session builds StatsReporter after RTC manager creation
Session->>SR: instantiate StatsReporter(clientEnvironment, rtcManager)
Session-->>Call: expose statsReporter
loop periodic ticks
SR->>RTC: collect publisher/subscriber PC bundles
SR->>SR: process bundles -> update CallMetrics (latency, bitrate, jitter, codecs, resolution)
alt every 10th tick
SR->>Device: sample battery & thermal
Device-->>SR: battery%, thermal severity
SR->>SR: append to battery/thermal histories
end
SR-->>Call: publish updated metrics
Call-->>UI: UI renders metrics & charts
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 2 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
⏰ 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). (6)
🔇 Additional comments (7)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1053 +/- ##
========================================
- Coverage 4.83% 4.80% -0.04%
========================================
Files 591 591
Lines 40324 40395 +71
========================================
- Hits 1951 1941 -10
- Misses 38373 38454 +81 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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_flutter/lib/src/call_screen/call_diagnostics_content/call_diagnostics_content.dart (1)
54-57: Fix: dispose() must be synchronous and cancel subscriptions before super.dispose()Overriding dispose() as async returns Future, which does not match State.dispose() signature (must be void). Also cancel subscriptions before calling super.dispose() to avoid setState after dispose race.
- @override - Future<void> dispose() async { - super.dispose(); - await _subscription?.cancel(); - } + @override + void dispose() { + // Cancel before super.dispose() to prevent events from hitting a dead State. + _subscription?.cancel(); + super.dispose(); + }
🧹 Nitpick comments (11)
packages/stream_video/CHANGELOG.md (1)
6-15: Unify unordered list style to pass markdownlint (MD004).Sub-list items under “Breaking changes” use dashes, while the configured style is asterisk. Align to asterisks to satisfy MD004.
- - The record field names have changed and the element types are different. + * The record field names have changed and the element types are different. - - Use `call.stats` for periodic WebRTC stats updates, or - - Access `call.statsReporter?.currentMetrics` for the latest aggregated metrics instead. + * Use `call.stats` for periodic WebRTC stats updates, or + * Access `call.statsReporter?.currentMetrics` for the latest aggregated metrics instead.dogfooding/lib/screens/stats_battery_chart.dart (2)
58-69: Make X-axis dynamic and guard against empty historyHard-coding maxX: 20 will clip data for longer histories and mis-scale shorter ones. Also consider clamping Y to [0, 100] and handling empty lists gracefully.
- minX: 0, - maxX: 20, + minX: 0, + maxX: batteryLevelHistory.isEmpty + ? 0 + : (batteryLevelHistory.length - 1).toDouble(), @@ - spots: batteryLevelHistory.indexed - .map<FlSpot>( - (m) => FlSpot(m.$1.toDouble(), m.$2.toDouble()), - ) - .toList(), + spots: batteryLevelHistory.indexed + .map<FlSpot>((m) => FlSpot( + m.$1.toDouble(), + m.$2.clamp(0, 100).toDouble(), + )) + .toList(),Optional: if you want a sliding window of N points (e.g., 21), slice the tail and rescale X from 0..windowLen-1 for consistent spacing.
17-35: Minor: remove unused vertical grid interval when vertical lines are disabledverticalInterval has no effect with drawVerticalLine: false.
- drawVerticalLine: false, - drawHorizontalLine: true, - verticalInterval: 1, + drawVerticalLine: false, + drawHorizontalLine: true,dogfooding/lib/screens/stats_thermal_chart.dart (2)
74-79: Nit: background bar height aligns better with chart maxYou add +1 to the data bars to ensure visibility; align the background rod with maxY for consistency.
- backDrawRodData: BackgroundBarChartRodData( + backDrawRodData: BackgroundBarChartRodData( show: true, - toY: _maxSeverity.toDouble(), + toY: (_maxSeverity + 1).toDouble(), color: Colors.white.withOpacity(0.04), ),
32-50: Optional: map severity ticks to labels for readabilityRight axis is numeric; consider custom getTitlesWidget to map 0..6 to severity names (e.g., none/light/moderate/severe/critical/emergency/shutdown) if available from your thermal model.
packages/stream_video/lib/src/call/call.dart (2)
866-873: Trace payload: prefer serializable names for enum-like fieldsIf pc.type is an enum (e.g., StreamPeerType), pass a string (name) instead of the enum object to avoid inconsistent serialization in traces.
- _session?.trace('pc_reconnection_needed', { - 'peerConnectionId': pc.type, + _session?.trace('pc_reconnection_needed', { + 'peerConnectionId': pc.type.name, 'reconnectionStrategy': strategy.name, });
1342-1345: Consider moving 'call_reconnect' trace inside the reconnect lockTracing before acquiring the lock can produce duplicate or misleading events if a concurrent reconnection slips in between the lock check and acquisition.
- _session?.trace('call_reconnect', { - 'strategy': strategy.name, - }); + // Move this trace inside the _callReconnectLock.synchronized block after setting _reconnectStrategyAdd this inside the synchronized block, right after setting _reconnectStrategy:
_session?.trace('call_reconnect', { 'strategy': _reconnectStrategy.name, });dogfooding/lib/screens/call_stats_screen.dart (2)
43-45: Clamp battery consumption to avoid negative values when chargingIf the device charged during the call, batteryDrained can be negative. Clamp to zero for a clearer UX.
- final batteryDrained = (state.initialBatteryLevel ?? 0) - - (state.batteryLevelHistory.lastOrNull ?? 0); + final batteryDrained = max( + 0, + (state.initialBatteryLevel ?? 0) - + (state.batteryLevelHistory.lastOrNull ?? 0), + );
212-213: Prefer null over 0 for unknown values to render “--” consistentlyPassing 0 implies a measured value. Let the widget show “--” when latency is not available.
- value: state.publisher?.latency ?? 0, + value: state.publisher?.latency,packages/stream_video/lib/src/models/call_stats.dart (2)
2-2: Remove unused thermal importthermal.dart is not referenced in this file.
-import 'package:thermal/thermal.dart';
10-41: Fix toString class name and consider deep equality for lists
- toString still says CallStats; update to PeerConnectionStatsBundle.
- Equality on List fields (stats/raw) uses reference equality. If structural equality is desired, use DeepCollectionEquality.
@override String toString() { - return 'CallStats{peerType: $peerType, printable: $printable, raw: $raw}'; + return 'PeerConnectionStatsBundle{peerType: $peerType, printable: $printable, raw: $raw}'; }Optional (if structural equality matters here):
- Add
import 'package:collection/collection.dart';- Replace
stats == other.statsandraw == other.rawwithconst DeepCollectionEquality().equals(stats, other.stats)and same forraw.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
dogfooding/lib/screens/call_stats_screen.dart(7 hunks)dogfooding/lib/screens/stats_battery_chart.dart(1 hunks)dogfooding/lib/screens/stats_thermal_chart.dart(1 hunks)packages/stream_video/CHANGELOG.md(1 hunks)packages/stream_video/lib/src/call/call.dart(8 hunks)packages/stream_video/lib/src/call/session/call_session.dart(5 hunks)packages/stream_video/lib/src/call/state/mixins/state_lifecycle_mixin.dart(0 hunks)packages/stream_video/lib/src/call/stats/stats_reporter.dart(8 hunks)packages/stream_video/lib/src/call_state.dart(0 hunks)packages/stream_video/lib/src/models/call_stats.dart(6 hunks)packages/stream_video_flutter/lib/src/call_screen/call_diagnostics_content/call_diagnostics_content.dart(2 hunks)
💤 Files with no reviewable changes (2)
- packages/stream_video/lib/src/call_state.dart
- packages/stream_video/lib/src/call/state/mixins/state_lifecycle_mixin.dart
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/stream_video/CHANGELOG.md
11-11: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
13-13: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
14-14: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
🔇 Additional comments (28)
packages/stream_video/CHANGELOG.md (2)
18-20: LGTM: Clear API addition notes (StatsReporter and new metrics)
21-22: LGTM: “Changed” section correctly cross-refers to breaking changespackages/stream_video_flutter/lib/src/call_screen/call_diagnostics_content/call_diagnostics_content.dart (5)
30-31: Type migration to PeerConnectionStatsBundle looks correctPublisher stats field updated to the new bundle type without logic change.
33-34: Type migration to PeerConnectionStatsBundle looks correctSubscriber stats field updated to the new bundle type without logic change.
36-41: Updated stats stream record type is correct and matches new Call.stats payloadThe StreamSubscription type reflects the new record shape with publisherStatsBundle/subscriberStatsBundle.
47-49: Correct mapping to new record fieldsAssignments pull from stats.publisherStatsBundle/subscriberStatsBundle as expected.
137-141: Propagated type changes to child widget props are consistentPublisher/subscriber fields now use PeerConnectionStatsBundle?.
packages/stream_video/lib/src/call/session/call_session.dart (4)
38-38: Import of stats_reporter.dart is appropriate for new StatsReporter wiring
143-146: Trace passthrough is a nice convenienceSimple passthrough keeps existing tracing semantics.
414-423: Environment capture LGTM; webRTC version empty on non-Android/iOS is acceptableGood capture of SDK and SFU info. If/when web/mac/windows versions are derivable, we can enrich later.
739-744: Confirm nullability of usedCodec is handled downstreamusedCodec can be null until initial metrics are available. Verify rtcManager.onPublishQualityChanged handles null gracefully; otherwise, consider falling back to previous codec or skipping the update until populated.
packages/stream_video/lib/src/call/call.dart (9)
304-304: Nice: Call exposes StatsReporter cleanlyAdding a getter for the session’s StatsReporter is a good, minimal surface to let clients consume metrics directly.
1192-1206: LGTM: Wiring session.statsReporter.run into the Call.stats emitterGood reuse of the session-owned StatsReporter and proper subscription lifecycle via _subscriptions. The SharedEmitter now emits the new tuple type, matching the model changes.
1271-1275: Good addition: trace SFU socket disconnection detailsAdding closeCode and closeReason to traces will help correlate reconnection flows.
1284-1287: Good addition: trace SFU socket failure with errorLightweight diagnostics that will pay off when debugging.
1291-1293: Good addition: trace SFU go-away eventsCapturing the go-away reason clarifies migrations at the telemetry layer.
1305-1307: Good addition: trace SFU errors with messageCapturing SFU error text is useful and appropriately scoped.
1411-1414: LGTM: trace reconnect success including strategyUseful signal to confirm which strategy resolved the reconnection.
1421-1425: LGTM: trace reconnect failure detailsCapturing the error string alongside strategy will aid backend telemetry correlation.
310-320: Stats API consumers OKI searched for legacy
CallStatsand old field names; the only hits are in protobuf-generated code and the SFU stats‐reporter mapping (which correctly maps the new record fields to protobuf strings). There are no remaining references toCallStatsor tostats.publisherStats/stats.subscriberStatsin application or UI code—every consumer now usespublisherStatsBundleandsubscriberStatsBundleas intended.dogfooding/lib/screens/call_stats_screen.dart (4)
111-114: LGTM: latency chart switches to new metrics sourceUsing state.latencyHistory aligns with the new CallMetrics surface.
241-249: LGTM: bitrate rendering using new fieldsBindings to publisher/subscriber bitrateKbps match CallMetrics.
259-267: LGTM: resolution and codecs from new publisher/subscriber statsStringifying resolution and codecs via the new stats shape is correct.
273-282: LGTM: environment fields switched to ClientEnvironmentRegion/Sdk/WebRTC versions now read from clientEnvironment; aligns with PR goals.
packages/stream_video/lib/src/models/call_stats.dart (2)
43-86: LGTM: CallMetrics shape cleanly encapsulates histories and environmentThe new CallMetrics model fits the UI/telemetry requirements and keeps publisher/subscriber details optional.
238-285: LGTM: ClientEnvironment replaces LocalStatsNaming and fields are clearer and directly actionable in the UI.
packages/stream_video/lib/src/call/stats/stats_reporter.dart (2)
35-42: Non-blocking processing is fine hereReturning the raw bundle stream while processing metrics asynchronously is reasonable; the Run stream stays timely, UI can read metrics from the separate state stream.
73-86: LGTM: stats processing aligns with new bundle shapeDeriving outbound media metrics from publisher and inbound from subscriber is correct and consistent with the new PeerConnectionStatsBundle.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/stream_video/lib/src/call/session/call_session.dart (1)
121-122: Duplicate comment: StatsReporter lifecycle management neededpackages/stream_video/lib/src/call/stats/stats_reporter.dart (1)
18-28: StatsReporter must expose a broadcast stream and clean up resourcesThe dogfooding call stats screen relies on
call.statsReporter?.stream(see dogfooding/lib/screens/call_stats_screen.dart), soStatsReporterneeds to:
- Expose a
Stream<CallMetrics?>that updates wheneverstatechanges- Properly close resources by overriding
dispose()Locations to update:
- packages/stream_video/lib/src/call/stats/stats_reporter.dart (around lines 18–28)
Suggested diff:
class StatsReporter extends StateNotifier<CallMetrics?> { StatsReporter({ required this.rtcManager, required this.clientEnvironment, }) : super(null); final RtcManager rtcManager; final ClientEnvironment clientEnvironment; + // Broadcast controller to emit metrics updates + late final _streamController = StreamController<CallMetrics?>.broadcast(); + CallMetrics? get currentMetrics => state; + Stream<CallMetrics?> get stream => _streamController.stream; + @override + set state(CallMetrics? value) { + super.state = value; + _streamController.add(value); + } + + @override + void dispose() { + _streamController.close(); + super.dispose(); + } }
🧹 Nitpick comments (2)
packages/stream_video/CHANGELOG.md (1)
4-8: Inconsistent list style detected - consider fixing.Static analysis flagged inconsistent unordered list styles (mixing dashes and asterisks). While not critical, consider using consistent bullet points for better readability.
- - The record field names have changed and the element types are different. + * The record field names have changed and the element types are different.packages/stream_video/lib/src/call/stats/stats_reporter.dart (1)
191-229: Platform guard implemented but could be more robustThe platform checks for battery and thermal monitoring are good, but the error handling could be more specific about which platforms and errors are expected.
try { if (batteryCheckAvailable) { batteryLevel = await Battery().batteryLevel; batteryLevelHistory = <int>[ ...state?.batteryLevelHistory.reversed.take(49).toList().reversed ?? [], batteryLevel, ]; } - } catch (_) { - // Ignore battery read failures + } on MissingPluginException catch (_) { + // Platform doesn't support battery monitoring + } catch (e) { + // Log unexpected battery read failures for debugging + // Ignore to prevent crashes }Similar improvement for thermal status error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/stream_video/CHANGELOG.md(1 hunks)packages/stream_video/lib/src/call/call.dart(8 hunks)packages/stream_video/lib/src/call/session/call_session.dart(6 hunks)packages/stream_video/lib/src/call/stats/stats_reporter.dart(7 hunks)packages/stream_video_flutter/lib/src/call_screen/call_diagnostics_content/call_diagnostics_content.dart(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
packages/stream_video/CHANGELOG.md
8-8: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
10-10: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
11-11: Unordered list style
Expected: asterisk; Actual: dash
(MD004, ul-style)
⏰ 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). (7)
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: stream_video_flutter
- GitHub Check: stream_video_screen_sharing
- GitHub Check: stream_video
- GitHub Check: analyze
- GitHub Check: build
🔇 Additional comments (28)
packages/stream_video_flutter/lib/src/call_screen/call_diagnostics_content/call_diagnostics_content.dart (5)
30-33: LGTM! Type update aligns with model refactoring.The field type changes from
CallStats?toPeerConnectionStatsBundle?correctly align with the broader refactoring of stats models in this PR.
36-40: LGTM! Stream subscription type correctly updated.The subscription type update to use the new record structure with
PeerConnectionStatsBundlefields properly aligns with the changes in the stats emitter.
47-48: LGTM! Assignment correctly uses new field names.The assignments now correctly reference
publisherStatsBundleandsubscriberStatsBundlefrom the stats record, matching the updated emission structure.
56-56: Verify dispose order doesn't cause issues.The dispose order was changed to call
super.dispose()after the subscription cancellation. This is generally safer as it ensures cleanup happens before parent disposal, but verify this doesn't cause any timing issues.
137-140: LGTM! Field types updated consistently.The
_CallStatsContentclass field types are correctly updated to usePeerConnectionStatsBundle?, maintaining consistency with the data model changes.packages/stream_video/CHANGELOG.md (2)
10-11: Well-documented breaking changes with clear migration path.The breaking changes are clearly documented with specific field names and types, making it easy for users to understand what needs to be updated. The migration guidance to use
call.statsorcall.statsReporter?.currentMetricsis helpful.
14-15: Excellent addition of new functionality.The addition of
StatsReporterwith consolidated metrics and battery/thermal tracking significantly enhances the monitoring capabilities of the SDK.packages/stream_video/lib/src/call/session/call_session.dart (5)
38-38: LGTM! Import added for StatsReporter integration.The import correctly brings the StatsReporter into scope for the session integration.
143-146: LGTM! Trace method provides useful debugging capability.The trace method provides a clean passthrough to the internal tracer, allowing external components to add trace information.
414-428: LGTM! Environment construction and StatsReporter initialization.The ClientEnvironment construction correctly captures SFU URL, SDK version, and platform-specific WebRTC versions. The StatsReporter initialization with the rtcManager and environment is properly positioned after rtcManager creation.
566-568: LGTM! StatsReporter disposal properly implemented.The StatsReporter disposal is correctly placed in the close method with proper null safety. This addresses the lifecycle concern mentioned in previous reviews.
743-743: LGTM! Updated to use new metrics structure.The codec lookup now correctly uses the new
statsReporter?.currentMetrics?.publisher?.videoCodecpath instead of the previous structure.packages/stream_video/lib/src/call/call.dart (11)
304-304: LGTM! StatsReporter exposure provides clean public API.The public getter correctly exposes the session's StatsReporter, providing access to consolidated metrics.
310-319: LGTM! Stats stream type correctly updated.The stats stream now correctly emits the new record structure with
PeerConnectionStatsBundletypes, aligning with the model refactoring.
866-872: Excellent tracing addition for debugging reconnection issues.The trace calls for peer connection reconnection events provide valuable debugging information, including the peer connection ID and reconnection strategy.
1192-1206: LGTM! Stats reporter integration correctly wired.The integration properly checks for statsReporter availability, subscribes to its run stream with the configured interval, and emits the stats to the public _stats stream.
1271-1274: LGTM! SFU socket event tracing enhances debugging.The trace calls for SFU socket disconnection events provide valuable debugging information with close codes and reasons.
1284-1286: LGTM! SFU socket failure tracing.The trace call for socket failure events correctly captures error information for debugging.
1290-1292: LGTM! SFU go-away event tracing.The trace call correctly captures the go-away reason for debugging SFU migration events.
1305-1307: LGTM! SFU error event tracing.The trace call properly captures SFU error information for debugging reconnection strategies.
1342-1344: LGTM! Reconnection attempt tracing.The trace call at the start of reconnection attempts provides valuable debugging information with the strategy being used.
1411-1413: LGTM! Reconnection success tracing.The trace call for successful reconnection provides closure on debugging reconnection flows.
1421-1425: LGTM! Reconnection failure tracing with error details.The trace call for reconnection failures correctly captures both the strategy and error information for comprehensive debugging.
packages/stream_video/lib/src/call/stats/stats_reporter.dart (5)
3-6: LGTM! Dependencies added for enhanced metrics collection.The addition of
battery_plus,state_notifier, andthermaldependencies correctly supports the new battery and thermal monitoring capabilities.
29-42: LGTM! Run method correctly returns new bundle structure.The run method properly returns the new record structure with
PeerConnectionStatsBundletypes and correctly processes stats with tick information.
45-72: LGTM! Stats collection updated to new bundle structure.The
collectStatsmethod correctly constructs and returnsPeerConnectionStatsBundleobjects instead of the previousCallStatsstructure.
74-83: LGTM! Stats processing signature correctly updated.The
_processStatsmethod signature correctly accepts the new bundle structure and tick information.
231-248: LGTM! State update logic correctly constructs CallMetrics.The state update logic properly handles both existing state updates via
copyWithand new state creation with all required fields including the new battery and thermal histories.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/stream_video/lib/src/call/call.dart (3)
1347-1350: Move 'call_reconnect' trace inside the reconnect lock to avoid duplicate startsBetween the locked check and synchronized block, multiple callers can still race and emit duplicate “call_reconnect” entries. Emit inside the lock to represent an actual attempt.
Apply:
- _session?.trace('call_reconnect', { - 'strategy': strategy.name, - }); - - await _callReconnectLock.synchronized(() async { + await _callReconnectLock.synchronized(() async { + _session?.trace('call_reconnect', { + 'strategy': strategy.name, + }); _reconnectStrategy = strategy;
1526-1551: Fix typos in log messages: “Awailable” → “Available”Purely cosmetic but noisy in logs.
Apply:
- _logger.v(() => '[awaitNetworkAwailable] starting timer'); + _logger.v(() => '[awaitNetworkAvailable] starting timer'); - _logger.w(() => '[awaitNetworkAwailable] too late for fast reconnect'); + _logger.w(() => '[awaitNetworkAvailable] too late for fast reconnect'); - _logger.w(() => '[awaitNetworkAwailable] timeout'); + _logger.w(() => '[awaitNetworkAvailable] timeout'); - _logger.w(() => '[awaitNetworkAwailable] call was left'); + _logger.w(() => '[awaitNetworkAvailable] call was left');
2660-2663: User-facing message mentions “video” while checking audio permissionMinor copy fix to avoid confusing developers.
Apply:
- return Result.error('Missing permission to send video'); + return Result.error('Missing permission to send audio');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/stream_video/lib/src/call/call.dart(9 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). (8)
- GitHub Check: stream_video_screen_sharing
- GitHub Check: stream_video
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video_flutter
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: build
- GitHub Check: analyze
- GitHub Check: analyze_legacy_version
🔇 Additional comments (9)
packages/stream_video/lib/src/call/call.dart (9)
304-304: Public getter for StatsReporter looks goodExposing the session-backed reporter via a nullable getter is reasonable and keeps the surface minimal.
752-755: Trace added for join migration source — LGTMCapturing the SFU name on forced migration will help triage join instability.
872-877: PC reconnection-needed trace — LGTMEmitting peerConnectionId and strategy is useful; callback delegates to existing reconnect flow.
1276-1280: Trace on sfu_socket_disconnected — LGTMClose code and reason captured; aligns with reconnection behavior.
1290-1292: Trace on sfu_socket_failed — LGTM
1296-1298: Trace on sfu_go_away — LGTM
1311-1313: Trace on sfu_error — LGTM
1426-1430: Reconnect failed trace — LGTMUnrecoverable case captures strategy and error before exiting.
310-320: Stats API migration verified All downstream usages referencepublisherStatsBundle/subscriberStatsBundleonPeerConnectionStatsBundle; no remainingCallStats,publisherStats, orsubscriberStatsreferences found.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/stream_video/lib/src/call/call.dart (2)
1418-1421: Only trace reconnect success when actually connectedCurrently emitted unconditionally; can produce false positives. Guard on connected state (or the actual method result).
Apply:
- _session?.trace('call_reconnect_success', { - 'strategy': _reconnectStrategy.name, - }); + if (state.value.status is CallStatusConnected) { + _session?.trace('call_reconnect_success', { + 'strategy': _reconnectStrategy.name, + }); + }
1198-1212: Gate StatsReporter.run() on active listeners; start/stop on first/last subscriberThis currently always subscribes when the session starts, which conflicts with the PR objective “Process stats only when there is at least one active listener.” Please bridge lazily and cancel when the last listener unsubscribes.
Proposed approach (adapt emitter API names as needed):
- if (session.statsReporter != null) { - _subscriptions.add( - _idSessionStats, - session.statsReporter! - .run( - interval: - _stateManager.callState.preferences.callStatsReportingInterval, - ) - .listen( - (stats) { - _stats.emit(stats); - }, - ), - ); - } + if (session.statsReporter != null) { + // Create/cancel subscription based on listener count. + StreamSubscription? _statsSub; + void _startIfNeeded() { + if (_statsSub != null) return; + _statsSub = session.statsReporter! + .run( + interval: _stateManager + .callState.preferences.callStatsReportingInterval, + ) + .listen(_stats.emit); + _subscriptions.add(_idSessionStats, _statsSub!); + } + void _stopIfIdle() { + if (!_stats.hasListeners) { + _subscriptions.cancel(_idSessionStats); + _statsSub = null; + } + } + // Hook into emitter listener lifecycle. + _stats.onListen(_startIfNeeded); + _stats.onCancel(_stopIfIdle); + // If someone already listens, start immediately. + if (_stats.hasListeners) _startIfNeeded(); + }If SharedEmitter/MutableSharedEmitterImpl lacks onListen/onCancel/hasListeners, please add them by delegating the underlying subject/stream controller.
Run to confirm available hooks:
#!/bin/bash rg -nP --type=dart -C3 'class\s+MutableSharedEmitterImpl\b|class\s+SharedEmitter\b' rg -nP --type=dart -C2 '\b(hasListeners|onListen|onCancel|listenerCount|onFirstListener|onZeroListeners)\b'
🧹 Nitpick comments (8)
packages/stream_video/lib/src/call/call.dart (5)
310-320: Public API emits a named record; consider a small value type for forward-compatUsing a record in the public surface is fine, but adding/removing fields later becomes a source/binary compatibility risk for downstream destructuring. Consider introducing a tiny value class (e.g., CallPeerConnectionStats) and return that via SharedEmitter, or mark this API @experimental.
751-755: Enrich join-migration trace with attempt and callCidAdds helpful signal for diagnosing repeated failures and correlating events.
Apply:
- _session?.trace('call_join_migrate', { - 'migrateFrom': sfuName, - }); + _session?.trace('call_join_migrate', { + 'migrateFrom': sfuName, + 'attempt': attempt, + 'callCid': callCid.value, + });
1277-1314: SFU socket/error traces: include stable identifiers for better correlationConsider adding sessionId and sfuName to each trace, and for failures include a machine-parseable errorCode when available. This improves cross-system joins.
Apply (illustrative):
- _session?.trace('sfu_socket_disconnected', { - 'closeCode': sfuEvent.reason.closeCode, - 'closeReason': sfuEvent.reason.closeReason, - }); + _session?.trace('sfu_socket_disconnected', { + 'sessionId': _session?.sessionId, + 'sfuName': _session?.config.sfuName, + 'closeCode': sfuEvent.reason.closeCode, + 'closeReason': sfuEvent.reason.closeReason, + });Repeat similarly for sfu_socket_failed, sfu_go_away, and sfu_error.
1369-1376: Add attempt counters to call_reconnect traceYou already track attempt locally; include it in the trace for observability.
Apply:
- _session?.trace('call_reconnect', { - 'strategy': _reconnectStrategy.name, - }); + _session?.trace('call_reconnect', { + 'strategy': _reconnectStrategy.name, + 'attempt': attempt, + });
1429-1432: Include structured error data in reconnect_failed traceAdd error type/category/status code to help aggregation (e.g., OpenApiError.apiError.code).
Example:
- _session?.trace('call_reconnect_failed', { - 'strategy': _reconnectStrategy.name, - 'error': error.toString(), - }); + _session?.trace('call_reconnect_failed', { + 'strategy': _reconnectStrategy.name, + 'error': error.toString(), + 'errorType': error.runtimeType.toString(), + 'statusCode': (error is OpenApiError) ? error.apiError.code : null, + });packages/stream_video/lib/src/call/stats/trace_record.dart (1)
40-52: Enable clearing fields to null in copyWith (current impl prevents setting null).Using
??meanscopyWith(id: null)won’t clear the field—it keeps the old value. If you ever need to null-outid/data, switch to a sentinel-based approach.Apply within this method:
- TraceRecord copyWith({ - String? tag, - String? id, - dynamic data, - int? timestamp, - }) { - return TraceRecord( - tag: tag ?? this.tag, - id: id ?? this.id, - data: data ?? this.data, - timestamp: timestamp ?? this.timestamp, - ); - } + TraceRecord copyWith({ + Object? tag = _u, + Object? id = _u, + Object? data = _u, + Object? timestamp = _u, + }) { + return TraceRecord( + tag: tag != _u ? tag as String : this.tag, + id: id != _u ? id as String? : this.id, + data: data != _u ? data : this.data, + timestamp: timestamp != _u ? timestamp as int : this.timestamp, + ); + }Add this once inside the class (outside the method):
static const Object _u = Object();packages/stream_video/lib/src/call/stats/tracer.dart (1)
29-33: Accept Iterable to avoid unnecessary list allocations.Let
traceMultipletakeIterable<TraceRecord>so callers can pass mapped iterables withouttoList().- void traceMultiple(List<TraceRecord> entries) { + void traceMultiple(Iterable<TraceRecord> entries) { if (!_enabled) return; _buffer.addAll(entries); }packages/stream_video/lib/src/call/session/call_session_factory.dart (1)
36-37: Prefer Iterable for flexibility and fewer copies.Align parameter type with a batched API and callers that may pass mapped views; keeps the default the same.
- List<TraceRecord> leftoverTraceRecords = const [], + Iterable<TraceRecord> leftoverTraceRecords = const [],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/stream_video/lib/src/call/call.dart(9 hunks)packages/stream_video/lib/src/call/session/call_session_factory.dart(3 hunks)packages/stream_video/lib/src/call/stats/trace_record.dart(1 hunks)packages/stream_video/lib/src/call/stats/tracer.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). (6)
- GitHub Check: build
- GitHub Check: analyze
- GitHub Check: stream_video_flutter
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: stream_video
- GitHub Check: stream_video_push_notification
🔇 Additional comments (4)
packages/stream_video/lib/src/call/call.dart (2)
304-305: Expose statsReporter: LGTMGetter is correctly nullable and scoped to the active session.
871-878: Reconnection-needed instrumentation and leftover trace carryover: LGTMCarrying leftoverTraceRecords and tracing pc_reconnection_needed with strategy and peerConnectionId will help field triage.
packages/stream_video/lib/src/call/session/call_session_factory.dart (2)
9-9: Import looks correct.
59-69: Fix ‘create’ trace payload and guard previous‐session ID
- Change
trace('create', {'url': sfuName})to usesfuUrl(trace('create', {'url': sfuUrl})).- In
traceMultiple, only overrider.idwhensessionSeq > 0(e.g.r.copyWith(id: '${sessionSeq-1}')) and leave it untouched otherwise.
# Conflicts: # dogfooding/lib/screens/call_stats_screen.dart # packages/stream_video/CHANGELOG.md # packages/stream_video/lib/src/call/call.dart # packages/stream_video/lib/src/call/stats/stats_reporter.dart # packages/stream_video/lib/src/call_state.dart # packages/stream_video/lib/src/models/call_stats.dart # packages/stream_video_flutter/lib/src/call_screen/call_diagnostics_content/call_diagnostics_content.dart
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
packages/stream_video/lib/src/call/session/call_session_factory.dart (1)
36-36: Document the leftoverTraceRecords parameter.As noted in a previous review, the purpose and expected content of
leftoverTraceRecordsis not immediately clear. Add a doc comment explaining that this parameter carries trace records from a previous session into the new session context.required StatsOptions statsOptions, required StreamVideo streamVideo, ClientPublishOptions? clientPublishOptions, + /// Trace records from a previous session to be carried forward into the new session. + /// Each record will be re-emitted with an id of `sessionSeq - 1`. List<TraceRecord> leftoverTraceRecords = const [], }) async {packages/stream_video/lib/src/call/call.dart (2)
1250-1266: Verify: Stats collection still runs unconditionally (doesn't match PR objective).Per FLU-205 objective: "Process stats only when someone listens to the stats stream." Currently,
session.statsReporter.run(...)is always subscribed when the session starts, regardless of whether_statshas active listeners.To align with the PR objective, consider:
- Starting the reporter only when
_statsgains its first subscriber- Canceling when the last listener unsubscribes
- Reattaching on subsequent listeners
Check if
SharedEmitter/MutableSharedEmitterImplexposes listener hooks:#!/bin/bash # Search for listener management APIs in SharedEmitter rg -nP --type=dart -C3 'class\s+(MutableSharedEmitterImpl|SharedEmitter)\b' | head -50 # Check for onListen/onCancel/hasListeners methods rg -nP --type=dart -C2 '\b(onListen|onCancel|hasListeners|listenerCount)\b' packages/stream_video/lib/src/shared_emitter.dart
1474-1477: Verify: Reconnect success traced unconditionally (may yield false positives).The
call_reconnect_successtrace is emitted after the reconnect switch, even if the underlying methods returned without throwing but didn't actually connect. This can produce false positives.Consider guarding on actual connection state before tracing success, or use the result from the reconnect methods.
Apply this diff to guard the success trace:
- _session?.trace('call_reconnect_success', { - 'strategy': _reconnectStrategy.name, - }); + if (state.value.status is CallStatusConnected) { + _session?.trace('call_reconnect_success', { + 'strategy': _reconnectStrategy.name, + }); + }packages/stream_video/lib/src/call/stats/stats_reporter.dart (1)
18-28: Suggest: Expose a publicstreamgetter for external consumers.The dogfooding UI and other consumers expect to read
call.statsReporter?.stream, butStatsReportercurrently only exposescurrentMetrics. Consider adding a broadcastStream<CallMetrics?>and forwarding state changes.Apply this pattern:
class StatsReporter extends StateNotifier<CallMetrics?> { StatsReporter({ required this.rtcManager, required this.clientEnvironment, }) : super(null); final RtcManager rtcManager; final ClientEnvironment clientEnvironment; + final _metricsController = StreamController<CallMetrics?>.broadcast(); CallMetrics? get currentMetrics => state; + Stream<CallMetrics?> get stream => _metricsController.stream; + @override + set state(CallMetrics? value) { + super.state = value; + _metricsController.add(value); + } + + @override + void dispose() { + _metricsController.close(); + super.dispose(); + }
🧹 Nitpick comments (5)
packages/stream_video/CHANGELOG.md (3)
14-23: Make migration explicit (old/new names, types, listener gating).Great high‑level notes, but devs need copy‑pasteable guidance. Please add:
- Explicit mapping: CallStats → PeerConnectionStatsBundle; record fields publisherStats → publisherStatsBundle, subscriberStats → subscriberStatsBundle.
- Public types: CallMetrics for currentMetrics; LocalStats → ClientEnvironment.
- Note: stats are processed only when there’s at least one listener to statsReporter.stream.
Suggested changelog patch:
* `Call.stats` payload changed. It now emits `({ PeerConnectionStatsBundle publisherStatsBundle, PeerConnectionStatsBundle subscriberStatsBundle })` instead of the previous `({ CallStats publisherStats, CallStats subscriberStats })`. - The record field names have changed and the element types are different. + - Migration example (Dart): + ```dart + // Before + final stats = await call.stats.first; + final pub = stats.publisherStats; + final sub = stats.subscriberStats; + // After + final stats = await call.stats.first; + final pub = stats.publisherStatsBundle; + final sub = stats.subscriberStatsBundle; + ``` * Stats-related fields were removed from `CallState` (e.g., `publisherStats`, `subscriberStats`, `latencyHistory`). - - Use `call.stats` for periodic WebRTC stats updates, or - - Access `call.statsReporter?.currentMetrics` for the latest aggregated metrics instead. + - Use `call.stats` for periodic WebRTC stats updates, or + - Access `call.statsReporter?.currentMetrics` (type: `CallMetrics`) for the latest aggregated metrics. + - Internal refactor: `LocalStats` renamed to `ClientEnvironment`. + - Performance: stats are computed/emitted only when there is at least one active listener to `call.statsReporter?.stream`.
24-27: ClarifycurrentMetricsshape and safe access.Name the type and list key fields so consumers know what to expect. Add a short usage snippet.
* `StatsReporter` is now exposed on `Call` as `call.statsReporter`, providing `currentMetrics` — a consolidated view of publisher/subscriber WebRTC quality, client environment, and rolling histories (latency, battery level, thermal status). -* Battery level and device thermal status are now tracked and available via `call.statsReporter?.currentMetrics`. +* `currentMetrics` type: `CallMetrics` with (non‑exhaustive): + `publisherStatsBundle`, `subscriberStatsBundle`, `latencyHistory`, + `batteryLevelHistory`, `thermalStatusHistory`, `initialBatteryLevel`, + `clientEnvironment`. +* Battery level and device thermal status are now tracked and available via `call.statsReporter?.currentMetrics`. +* Example: + ```dart + final metrics = call.statsReporter?.currentMetrics; + final battery = metrics?.batteryLevelHistory.lastOrNull; + final thermal = metrics?.thermalStatusHistory.lastOrNull; + ```
28-30: Call out singlegetStats()consolidation and cadence.Under “Changed”, note the internal consolidation and confirm emission cadence hasn’t changed to avoid surprises.
* `Call.stats` continues to emit periodically, but the record field names/types changed as noted under breaking changes. +* Stats collection is consolidated into a single `RTCPeerConnection.getStats()` call for local + SFU data. +* Emission cadence is unchanged (verify if different, document the new interval).dogfooding/lib/screens/stats_battery_chart.dart (1)
50-51: Consider dynamic maxX based on data length.The chart's
maxXis hardcoded to 20, butbatteryLevelHistorycan be any length. If the history contains more than 20 points, data will be compressed; if fewer, empty space will appear. Consider settingmaxXdynamically:minX: 0, - maxX: 20, + maxX: (batteryLevelHistory.length - 1).toDouble().clamp(1, 20), minY: 0,This caps the visible window at 20 points for readability while adapting to shorter histories.
packages/stream_video/lib/src/call/stats/stats_reporter.dart (1)
200-239: Battery and thermal reads correctly guarded, but consider dependency injection for testing.Platform checks and try/catch blocks now prevent crashes on unsupported platforms. However, for testability, consider injecting
BatteryandThermalinstances in the constructor (as noted in a past comment). This would allow mocking in tests.Based on learnings
Example:
class StatsReporter extends StateNotifier<CallMetrics?> { StatsReporter({ required this.rtcManager, required this.clientEnvironment, + Battery? battery, + Thermal? thermal, - }) : super(null); + }) : _battery = battery ?? Battery(), + _thermal = thermal ?? Thermal(), + super(null); final RtcManager rtcManager; final ClientEnvironment clientEnvironment; + final Battery _battery; + final Thermal _thermal; ... // In _processStats: - batteryLevel = await Battery().batteryLevel; + batteryLevel = await _battery.batteryLevel; - final thermalStatus = await Thermal().thermalStatus; + final thermalStatus = await _thermal.thermalStatus;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
dogfooding/lib/screens/call_stats_screen.dart(3 hunks)dogfooding/lib/screens/stats_battery_chart.dart(1 hunks)dogfooding/lib/screens/stats_thermal_chart.dart(1 hunks)packages/stream_video/CHANGELOG.md(1 hunks)packages/stream_video/lib/src/call/call.dart(9 hunks)packages/stream_video/lib/src/call/session/call_session.dart(6 hunks)packages/stream_video/lib/src/call/session/call_session_factory.dart(3 hunks)packages/stream_video/lib/src/call/state/mixins/state_lifecycle_mixin.dart(0 hunks)packages/stream_video/lib/src/call/stats/stats_reporter.dart(8 hunks)packages/stream_video/lib/src/call_state.dart(0 hunks)packages/stream_video/lib/src/models/call_stats.dart(6 hunks)packages/stream_video_flutter/lib/src/call_screen/call_diagnostics_content/call_diagnostics_content.dart(2 hunks)
💤 Files with no reviewable changes (2)
- packages/stream_video/lib/src/call_state.dart
- packages/stream_video/lib/src/call/state/mixins/state_lifecycle_mixin.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/stream_video/lib/src/call/session/call_session.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). (7)
- GitHub Check: build
- GitHub Check: analyze
- GitHub Check: stream_video_screen_sharing
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: stream_video
- GitHub Check: stream_video_flutter
🔇 Additional comments (13)
packages/stream_video_flutter/lib/src/call_screen/call_diagnostics_content/call_diagnostics_content.dart (1)
56-59: Improved dispose order.Canceling the subscription before calling
super.dispose()is the correct pattern and prevents potential issues with ongoing async operations.dogfooding/lib/screens/call_stats_screen.dart (1)
167-202: New thermal and battery charts integration looks good.The integration of
StatsThermalChartandStatsBatteryChartwith the newCallMetricsdata model is well-structured. The battery drain calculation correctly handles null values, and both charts are properly bound to their respective history fields.dogfooding/lib/screens/stats_thermal_chart.dart (1)
70-70: Confirmed Flutter SDK constraint covers withValues API
The project’s pubspec.yaml requires Flutter “>=3.32.0”, which is above the 3.27 version that introducedwithValues. No changes are needed.packages/stream_video/lib/src/call/call.dart (3)
311-311: LGTM! StatsReporter public getter added.Correctly exposes the session's stats reporter for external consumers (UI/dogfooding).
790-793: LGTM! Migration trace added.Correctly logs migration attempts for debugging.
910-917: LGTM! Trace continuity and reconnection instrumentation.Carrying forward trace records and logging reconnection events ensures complete diagnostic context across session boundaries.
packages/stream_video/lib/src/models/call_stats.dart (3)
9-40: LGTM! PeerConnectionStatsBundle correctly renamed and extended.The rename from
CallStatstoPeerConnectionStatsBundleclarifies the scope, and the newstatsfield provides raw stats access. Equality and hashCode are correctly updated.
42-85: LGTM! CallMetrics aggregates all stats and history.Clean design that consolidates publisher/subscriber stats, environment, and temporal histories (latency, battery, thermal) in one immutable class. The
copyWithmethod supports efficient state updates.
237-283: LGTM! ClientEnvironment renamed and enhanced.The rename from
LocalStatstoClientEnvironmentbetter describes the data, and the addedempty()factory,copyWith, equality, and hashCode follow Dart conventions.packages/stream_video/lib/src/call/stats/stats_reporter.dart (4)
29-45: LGTM! Stream API correctly returns bundles with tick counter.The
runmethod now returns a stream ofPeerConnectionStatsBundlerecords and passes the tick to_processStatsfor periodic battery/thermal sampling.
47-77: LGTM! collectStats correctly constructs PeerConnectionStatsBundle.Properly handles nullable publisher and creates bundles with all required fields (peerType, stats, printable, raw).
79-195: LGTM! Stats processing correctly updated for bundle types.Safe null handling (
state?.latencyHistory), correct extraction of codec/resolution/jitter from the newPeerConnectionStatsBundlestructure.
241-259: LGTM! State update correctly uses copyWith or creates new CallMetrics.Immutable state management pattern correctly applied. All fields (publisher, subscriber, clientEnvironment, histories, initialBatteryLevel) are properly included.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/stream_video/lib/src/call/stats/stats_reporter.dart (1)
18-35: Missingstreamgetter causes compilation failure in UI.The dogfooding UI (dogfooding/lib/screens/call_stats_screen.dart:25) references
call.statsReporter?.stream, butStatsReporterdoes not expose aStream<CallMetrics?>property. This will cause a compilation error.Add a broadcast stream that mirrors state changes:
class StatsReporter extends StateNotifier<CallMetrics?> { StatsReporter({ required this.rtcManager, required this.clientEnvironment, Battery? battery, Thermal? thermal, - }) : super(null) { + }) : _metricsController = StreamController<CallMetrics?>.broadcast(), + super(null) { _battery = battery ?? Battery(); _thermal = thermal ?? Thermal(); } final RtcManager rtcManager; final ClientEnvironment clientEnvironment; + final StreamController<CallMetrics?> _metricsController; late Battery _battery; late Thermal _thermal; CallMetrics? get currentMetrics => state; + Stream<CallMetrics?> get stream => _metricsController.stream; + + @override + set state(CallMetrics? value) { + super.state = value; + _metricsController.add(value); + } + + @override + void dispose() { + _metricsController.close(); + super.dispose(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
dogfooding/lib/screens/call_stats_screen.dart(3 hunks)packages/stream_video/lib/src/call/session/call_session_factory.dart(5 hunks)packages/stream_video/lib/src/call/stats/stats_reporter.dart(8 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). (7)
- GitHub Check: build
- GitHub Check: analyze
- GitHub Check: stream_video_push_notification
- GitHub Check: stream_video
- GitHub Check: stream_video_noise_cancellation
- GitHub Check: stream_video_flutter
- GitHub Check: analyze_legacy_version
🔇 Additional comments (8)
packages/stream_video/lib/src/call/session/call_session_factory.dart (3)
1-1: Import added to support max function.The
dart:mathimport is correctly added to support themax(0, sessionSeq - 1)clamping operation used in the trace ID calculation at line 81.
27-41: Documentation clearly explains leftoverTraceRecords parameter.The updated documentation explains the purpose of
leftoverTraceRecordsand its relationship to previous sessions. This addresses the previous review comment requesting better documentation.
77-85: Negative trace ID issue resolved with max clamping.The
max(0, sessionSeq - 1)expression at line 81 correctly prevents negative trace IDs whensessionSeqis 0, addressing the concern raised in the previous review.dogfooding/lib/screens/call_stats_screen.dart (2)
30-34: LGTM! Appropriate null handling.The loading state is correctly guarded with a null check and displays a progress indicator while metrics are unavailable.
104-277: LGTM! New monitoring sections are well structured.The thermal state, battery level, and call performance sections are properly implemented with appropriate charts and metrics. Note: A past review incorrectly flagged lines 104-138 as a duplicate of the latency section, but these are distinct new sections for thermal and battery monitoring.
packages/stream_video/lib/src/call/stats/stats_reporter.dart (3)
19-27: LGTM! Dependency injection enables testing.Battery and Thermal are now injectable via optional constructor parameters, making the class testable. This addresses the previous review feedback.
Based on past review comments
208-247: LGTM! Proper platform checks and error handling for battery/thermal.The battery and thermal readings are now guarded with platform availability checks and wrapped in try-catch blocks to prevent crashes on unsupported platforms or transient failures. This addresses the previous review feedback.
Based on past review comments
249-266: LGTM! State update logic correctly preserves initialBatteryLevel.The state update properly uses
copyWithfor existing state and creates a newCallMetricswhen state is null, ensuringinitialBatteryLevelis captured only once at the start of the call.
2 participants
part of FLU-232
resolves FLU-205
This PR moves call stats from CallState to StatsReporter and adds additional battery and thermal state tracking.
Summary by CodeRabbit
New Features
Improvements
User Experience