@moq/watch: source network stats from the connection, not navigator#1386
Merged
Conversation
The stats panel's network row was driven by navigator.connection, which
on most browsers reports a coarse effectiveType ("4G") and synthetic
downlink/rtt values that have nothing to do with the actual MoQ session.
Replace it with the bandwidth and RTT signals that the connection
already exposes (PROBE for recv bandwidth and RTT, QUIC stats for send
bandwidth), plumbed through MultiBackend.network.
Contributor
WalkthroughAdds a public Connection interface and a ProviderProps.connection Peekable, passes connection from the top-level element through Stats -> StatsPanel -> StatsItem, refactors NetworkProvider to poll the provided connection for recv/send bandwidth and RTT (with new formatting helpers), and replaces RTT-only wiring in MultiBackend/Sync with the established connection signal so jitter is computed from conn.rtt. 🚥 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 docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/ui-core/src/stats/providers/network.ts`:
- Around line 21-24: Replace the raw window.setInterval usage in NetworkProvider
with the inherited signals Effect: instead of assigning this.updateInterval =
window.setInterval(...), call
this.signals.interval(NetworkProvider.POLLING_INTERVAL_MS,
this.updateDisplayData.bind(this)) so the subscription is managed by
BaseProvider's signals; remove the manual clearInterval call and any
this.updateInterval bookkeeping in the cleanup() method since signals.interval()
handles teardown automatically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdb78bd9-15ac-4f5f-9c6d-022264004091
📒 Files selected for processing (8)
js/ui-core/src/stats/components/StatsItem.tsxjs/ui-core/src/stats/components/StatsPanel.tsxjs/ui-core/src/stats/index.tsxjs/ui-core/src/stats/providers/network.tsjs/ui-core/src/stats/types.tsjs/watch/src/backend.tsjs/watch/src/element.tsjs/watch/src/ui/element.tsx
Lets BaseProvider's Effect manage the polling timer instead of tracking the interval id by hand.
…ed-watch-stats-AfO6H
… network props Rather than flattening connection.rtt/recvBandwidth/sendBandwidth into separate Signals on MoqWatch and threading them through MultiBackendProps and a NetworkBackend interface, expose the established connection signal directly: - Stats UI reads from connection.established (the only consumer of recv/send bandwidth) and pulls the inner signals on demand. - Sync takes the established connection signal and reads RTT internally for "real-time" jitter computation. - MultiBackend no longer has rtt or NetworkBackend in its API. Network metrics aren't a backend concern; the backend just consumes the connection that already owns them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
js/watch/src/sync.ts (2)
90-92: ⚡ Quick winExtract the RTT multiplier into a named constant.
This avoids an inline magic number in core jitter math.
As per coding guidelines, "Avoid using magic numbers; use named constants instead".Proposed update
const MIN_JITTER = 20 as Time.Milli; const FALLBACK_JITTER = 100 as Time.Milli; +const RTT_RETRANSMIT_MULTIPLIER = 1.25; @@ - const jitter = Math.max(MIN_JITTER, this.#minRtt * 1.25) as Time.Milli; + const jitter = Math.max(MIN_JITTER, this.#minRtt * RTT_RETRANSMIT_MULTIPLIER) as Time.Milli;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/sync.ts` around lines 90 - 92, Replace the inline multiplier 1.25 in the jitter calculation with a named constant to avoid the magic number: introduce a constant (e.g., RTT_MULTIPLIER = 1.25) near the top of the module or inside the class, then use it in the expression that computes jitter (currently using MIN_JITTER and this.#minRtt) so the line becomes Math.max(MIN_JITTER, this.#minRtt * RTT_MULTIPLIER) and keep the rest (this.jitter.set(jitter)) unchanged.
13-13: ⚡ Quick winAdd a doc comment for the new public
SyncProps.connectionfield.This keeps the public API self-describing at the type level.
As per coding guidelines, "Document public APIs with clear docstrings or comments".Proposed update
export interface SyncProps { latency?: Latency | Signal<Latency>; + /** Established connection signal used to read RTT in "real-time" mode. */ connection?: Signal<Moq.Connection.Established | undefined>; audio?: Time.Milli | Signal<Time.Milli | undefined>; video?: Time.Milli | Signal<Time.Milli | undefined>; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/watch/src/sync.ts` at line 13, Add a concise doc comment above the public SyncProps.connection field describing its purpose and semantics: explain that connection is an optional Signal that holds a Moq.Connection.Established or undefined, when it will be set/cleared, and how consumers should use it (e.g., subscribe/react to changes or check for undefined before use); reference the exact field name SyncProps.connection and its type Signal<Moq.Connection.Established | undefined> so the public API is self-describing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@js/watch/src/sync.ts`:
- Around line 90-92: Replace the inline multiplier 1.25 in the jitter
calculation with a named constant to avoid the magic number: introduce a
constant (e.g., RTT_MULTIPLIER = 1.25) near the top of the module or inside the
class, then use it in the expression that computes jitter (currently using
MIN_JITTER and this.#minRtt) so the line becomes Math.max(MIN_JITTER,
this.#minRtt * RTT_MULTIPLIER) and keep the rest (this.jitter.set(jitter))
unchanged.
- Line 13: Add a concise doc comment above the public SyncProps.connection field
describing its purpose and semantics: explain that connection is an optional
Signal that holds a Moq.Connection.Established or undefined, when it will be
set/cleared, and how consumers should use it (e.g., subscribe/react to changes
or check for undefined before use); reference the exact field name
SyncProps.connection and its type Signal<Moq.Connection.Established | undefined>
so the public API is self-describing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6452bcfd-27fb-40a9-a461-dfbc8a88356b
📒 Files selected for processing (10)
js/moq-boy/src/index.tsjs/ui-core/src/stats/components/StatsItem.tsxjs/ui-core/src/stats/components/StatsPanel.tsxjs/ui-core/src/stats/index.tsxjs/ui-core/src/stats/providers/network.tsjs/ui-core/src/stats/types.tsjs/watch/src/backend.tsjs/watch/src/element.tsjs/watch/src/sync.tsjs/watch/src/ui/element.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- js/ui-core/src/stats/components/StatsPanel.tsx
- js/watch/src/ui/element.tsx
- js/ui-core/src/stats/index.tsx
- js/ui-core/src/stats/components/StatsItem.tsx
- js/ui-core/src/stats/providers/network.ts
- js/ui-core/src/stats/types.ts
- js/watch/src/element.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The stats panel's network row was driven by navigator.connection, which
on most browsers reports a coarse effectiveType ("4G") and synthetic
downlink/rtt values that have nothing to do with the actual MoQ session.
Replace it with the bandwidth and RTT signals that the connection
already exposes (PROBE for recv bandwidth and RTT, QUIC stats for send
bandwidth), plumbed through MultiBackend.network.