fix: video: apply the configured jitter buffer target to RTSP streams

[rafaellehmkuhl]

Summary

This is not a latency fix — it changes nothing at the default setting. It closes a configuration gap and makes RTSP streams observable. Please read the section below before assuming it helps with RTSP lag.

The "RTP Jitter Buffer (Target) duration" setting was only ever applied to mavlink-camera-manager streams: Session pushes it onto its receivers on every track, while Go2RTCManager builds its own RTCPeerConnection and never did. So a user who raises the target gets it on WebRTC streams and is silently ignored on RTSP ones. Separately, RTSP streams report nothing about what the browser does with their frames, which makes any latency investigation on that path guesswork.

• The receiver tuning became a free function (src/libs/webrtc/jitter-buffer.ts), moved out of Session so both managers can reach the same code. No behavior change for WebRTC streams.
• RTSP streams now apply the configured target, so a nonzero value behaves the same on both protocols.
• RTSP streams report their client-side receive stats. The stats overlay and the data-lake stream variables only ever saw the mavlink-camera-manager peer connection, so for RTSP they showed go2rtc's ingest bitrate and packet rate and nothing about the browser's receive side. Their peer connection is now exposed through the same store accessor, keyed by a fresh id per connection so a reconnect re-registers instead of leaving the stats pinned to a closed one, and both protocols show the average jitter buffer delay so they can be compared directly.

The target is read when a stream is activated, so changing the setting requires restarting the stream. That matches the existing WebRTC behavior.

Why this cannot affect the default case

Worth writing down, because it is counter-intuitive and it took reading the Chromium and libwebrtc sources to establish:

• Cockpit's default is 0 (cockpit-jitter-buffer-target), and clearing the field snaps it back to 0 rather than leaving it empty, so in practice every user is on 0.
• At 0 the code sends playoutDelayHint = null, which means "no hint, use the browser default". So the WebRTC path, at the default, explicitly asks for the same adaptive behavior that the RTSP path was getting by not asking for anything.
• RTCRtpReceiver.jitterBufferTarget only shipped in Chrome 124. Electron 29.4.6 pins Chromium 122, so in this build that assignment is a dead property on a JS object and reaches no WebRTC code at all. Only the legacy playoutDelayHint does anything here.
• Even where the attribute is live, both funnel into SetJitterBufferMinimumDelay, which sets min_playout_delay_ in libwebrtc's VCMTiming, and the target is max(min_playout_delay_, jitter_delay_ + RequiredDecodeTime() + render_delay_). It is a floor, not a ceiling: it can only add delay on top of the adaptive estimate, and it cannot make Chromium drop frames. Zero means no floor, which is already the default. The genuine "render frames as they arrive" mode needs max_playout_delay <= 500ms as well, and that has no JavaScript API — only the sender-side RTP playout-delay extension, which go2rtc does not send.

So the parity is real and worth having, but the RTSP latency that prompted this (on a 1.1GHz Celeron topside machine, where RTSP takes a permanent latency step on every resource spike and never recovers) is not explained by it. That investigation continues, and the stats added here are what it needs: if the new Buffer line climbs during a spike and stays up, the delay is on the browser's receive side; if it stays at a few tens of milliseconds while the picture is seconds behind, the delay is accumulating upstream in go2rtc or its socket, which is where I would now look first.

Test plan

• Set the RTP jitter buffer target to something large (e.g. 2000ms) in Configuration → Video, restart an RTSP stream, and confirm its latency grows accordingly. Before this PR the setting had no effect on RTSP streams.
• Set it back to 0, restart, and confirm RTSP latency returns to what it was.
• Confirm a WebRTC stream responds to the same setting exactly as it did before.
• With stats for nerds enabled on an RTSP stream, confirm the overlay still shows the go2rtc ingest numbers (codec, size, bitrate, packets, stalls) and now also shows Buffer and Frame drops.
• Let an RTSP stream reconnect (take the source down and bring it back) and confirm the stats keep updating instead of freezing on the old connection.
• Confirm WebRTC streams connect, record and report stats as before.

Checks

• src/tests/libs/webrtc/jitter-buffer.test.ts covers the video-only receiver filter, the 0–4000ms clamp, and that a zero target leaves the legacy hint unset.
• yarn lint clean with --max-warnings=0, yarn test:unit passing.
• Note for whoever reviews the types: yarn typecheck exits early in this repo with languageId not found for src/App.vue and checks nothing. I ran plain tsc instead; the only errors in the touched files are two pre-existing URI | undefined complaints on untouched new WebRTCManager(...) lines.

[github-actions]

Automated PR Review — round 1

Warning

⚠️ IMPORTANT FIXES REQUIRED — 4 open findings: 1 major, 1 minor, 2 nits.

RTSP video in Cockpit is played by a second, separate connection manager that never applied the user's video-latency setting, so those streams ran on whatever buffering the browser decided on its own. This PR moves the one-line-per-receiver tuning into a shared function, hands it to the RTSP manager as well, and additionally makes the RTSP connection report the browser-side receive statistics (buffer delay, dropped frames) that the debug overlay and the data lake previously only had for the other stream type.

What still needs attention

#	Problem	What it means	Severity	Status
5.1	Statistics monitors are added per RTSP reconnect and never removed	Each time an RTSP camera drops and reconnects, Cockpit starts another ten-times-per-second measurement loop it never stops, so a long session with a flaky camera gets slower and slower — on exactly the low-powered computers this PR is meant to help.	major	❌
6.1	Twelfth line of the stats overlay now sits on top of the graphs	The bottom row of the debug overlay for regular streams is drawn over the little charts underneath it, making both harder to read.	minor	❌
11.1	Needless lint suppression in the new test	Cosmetic: a disabled lint rule that would not have fired anyway.	nit	❌
11.2	New test file does not mirror the source folder	Cosmetic: the test sits one folder higher than the code it tests, unlike the existing tests.	nit	❌

Change map — what was established before judging

Claims (from the PR body — the author's hypothesis, checked against the code):

• Symptom: RTSP takes a permanent latency step on a slow topside machine after a resource spike, WebRTC does not. Field report about browser/OS behaviour; not verifiable from this checkout. Not adopted as fact, but nothing in the code contradicts it.
• Cause: Go2RTCManager builds its own RTCPeerConnection and never applied the configured target, while the mavlink-camera-manager path did. Verified. Base src/composables/go2rtc.ts:74-92 creates the peer connection and, in ontrack, only sets contentHint — no receiver tuning anywhere in the class. The other path does it at src/composables/webRTC.ts:239. The setting itself is real and defaults to zero: src/stores/video.ts:59 (useBlueOsStorage<number>('cockpit-jitter-buffer-target', 0)), UI at src/views/ConfigurationVideoView.vue:239-267.
• Mechanism: Chromium's adaptive jitter buffer grows under starvation and does not shrink back. Browser-internal behaviour; not verifiable here. The code-level half of the claim (that leaving the attributes unset is what selects that behaviour) matches the moved implementation, which writes null to playoutDelayHint only when the target is falsy.
• "The receiver tuning became a free function … No behavior change for WebRTC streams." Verified. The body of src/libs/webrtc/jitter-buffer.ts is identical to the deleted Session.setJitterBufferTarget (src/libs/webrtc/session.ts:132-171 at base), and Session.peerConnection is a non-optional field (src/libs/webrtc/session.ts:21), so the new if (this.session?.peerConnection) guard in webRTC.ts:240-242 is equivalent to the previous this.session?.setJitterBufferTarget(...).
• "The target is read when a stream is activated … That matches the existing WebRTC behavior." Verified. webRTC.ts:100 captures jitterBufferTarget.value inside startStream, and video.ts:443 now passes jitterBufferTarget.value to the Go2RTCManager constructor. Both need a stream restart to pick up a new value.
• "keyed by a fresh id per connection so a reconnect re-registers instead of leaving the stats pinned to a closed one." Partially contradicted. Re-registration does happen (the deep watcher on activeStreams re-fires when the manager's status refs change during a reconnect, and getStreamPeerConnection then returns the new uuid). But nothing removes the previous entry from the monitor, so the closed connection stays registered alongside the new one — see 5.1.
• "yarn lint clean … yarn test:unit passing … yarn typecheck exits early in this repo." Not verifiable here (no install, no PR code execution). Treated as an unverified author claim; the review does not lean on it either way.

Failure site: src/composables/go2rtc.ts, connect() and its pc.ontrack handler (base lines 67-92). It is in the diff, and the fix is at the shared level: both new RTCPeerConnection sites in the tree (src/libs/webrtc/session.ts:114, src/composables/go2rtc.ts:74) now route through the one function.

Entry points

Function	Reached from	Frequency
setJitterBufferTarget (src/libs/webrtc/jitter-buffer.ts:7)	WebRTCManager.onTrackAdded (src/composables/webRTC.ts:235, Session track listener) and pc.ontrack in Go2RTCManager.connect (src/composables/go2rtc.ts:79)	one-shot per (re)connection
Go2RTCManager constructor (src/composables/go2rtc.ts:35)	activateStream (src/stores/video.ts:410), itself driven by VideoPlayer's 1s activation polling	per user action
Go2RTCManager.peerConnection getter (new)	getStreamPeerConnection (src/stores/video.ts:621)	per activeStreams mutation
getStreamPeerConnection (src/stores/video.ts:621, changed)	deep watch(videoStore.activeStreams) at src/stores/omniscientLogger.ts:190 and src/components/VideoPlayerStatsForNerds.vue:198	per stream-state change — several times per connect, and again on every automatic reconnect
draw (src/components/VideoPlayerStatsForNerds.vue:97, changed)	requestAnimationFrame loop	per frame
webrtcStats.on('stats') handler (src/components/VideoPlayerStatsForNerds.vue:248, changed)	WebRTCStats poll, getStatsInterval: 100	10 Hz per monitored connection

Invariants

• Every receive-side peer connection Cockpit creates gets the configured target. Producers: src/libs/webrtc/session.ts:114 and src/composables/go2rtc.ts:74. Both are covered after this PR; there is no third new RTCPeerConnection in src/.
• Stats consumers reach a stream's peer connection through one accessor. getStreamPeerConnection (src/stores/video.ts:621) is the chokepoint, and the PR removes omniscientLogger's direct webRtcManager.session bypass — the single-consumer form the guidelines prefer. The remaining direct webRtcManager.session read (src/stores/video.ts:515) is teardown, not stats.
• A peerId identifies a monitored connection for the life of a WebRTCStats instance. At base this held: session.consumerId is stable per manager, so addConnection ran once per stream. The PR replaces it with a fresh uuid per go2rtc connection while adding no removal call — the violated invariant behind 5.1. Sites that register: src/stores/omniscientLogger.ts:201 (store, never torn down) and src/components/VideoPlayerStatsForNerds.vue:204 (bounded only by widget unmount at line 287). Sites that remove: only that destroy().

5. Performance — 1 finding

5.1 — go2rtc stats connections are registered per reconnect and never removed — major

src/composables/go2rtc.ts:92 assigns a fresh connectionId = uuid() inside connect(), and src/stores/video.ts:277-281 returns it as both peerId and sessionId. The two registration sites guard on that id:

• src/stores/omniscientLogger.ts:247 — if (webrtcStreamStats[streamName].peersToMonitor[pcInfo.peerId]) return
• src/components/VideoPlayerStatsForNerds.vue:203 — the same guard on the component's own instance

Because the id is new on every connection, the guard never matches after a reconnect and addConnection runs again — which is the intended half. The missing half is removal: there is no removeConnection/removePeer-style call anywhere in src/ (only webrtcStats.destroy() on unmount at VideoPlayerStatsForNerds.vue:287), so the entry for the previous, now-closed RTCPeerConnection stays in peersToMonitor and keeps being polled at getStatsInterval: 100. Go2RTCManager reconnects on its own — RECONNECT_DELAY_MS = 3000 on disconnected/failed/socket close, plus the CONNECT_WATCHDOG_MS = 8000 forced reconnect when the camera is unreachable (src/composables/go2rtc.ts:188-215) — so with an offline or flapping RTSP camera this adds a permanent 10 Hz getStats() loop roughly every 11 seconds, in a Pinia store that lives for the whole session. That is exactly the shape the guidelines escalate: cost that grows from a timer the user never triggered and cannot stop, on the 1.1 GHz machine this PR exists to help. At base the WebRTC path could not do this, since session.consumerId is stable, so the accumulation is introduced here.

(I could not check whether @peermetrics/webrtc-stats v5.7 drops a connection by itself once its connectionState reaches closed — node_modules is not part of this checkout and there is no network access. The explicit removal is the right call regardless, and note Go2RTCManager.cleanup() nulls pc.onconnectionstatechange before closing, so any assumption about who observes the close should be stated rather than inherited.)

Second, smaller half of the same problem: both 'stats' handlers (src/stores/omniscientLogger.ts:223-234, src/components/VideoPlayerStatsForNerds.vue:248-277) read ev.data.video.inbound[0] and ignore ev.peerId, which was safe while one peer per stream was monitored. With two or more registered under one instance, whichever connection emits last wins the data-lake variables and the overlay numbers.

Fix: remove the stream's previous peer before adding the new one (keep the last registered id per stream next to webrtcStreamStats, and call the library's per-connection removal — the counterpart to addConnection — before re-adding), and filter the handler on ev.peerId so a straggler event from a dead connection cannot overwrite live numbers. Both are a few lines at the two registration sites; no restructuring needed.

Consequence: on a machine with an RTSP camera that reconnects, Cockpit slowly piles up measurement loops it never stops, so video and the whole interface get progressively slower the longer the session runs.

6. UI / UX — 1 finding

6.1 — the added Buffer row pushes the WebRTC stats overlay's last line into the graph area — minor

src/components/VideoPlayerStatsForNerds.vue:173 renders the WebRTC list at y = 12 + index * 12, and the list grows from 11 to 12 entries with the new { label: 'Buffer', … } at line 170 of the diff. VideoPlayer.vue:8 mounts the component without width/height, so the defaults apply (height: 200, plotHeight = 60): the plots occupy y 140…200 (drawPlot, line 116: height - normalizeValue(...)), while the last row's baseline moves from 132 to 144 — inside the plot band. Before this PR there were 8 px of clearance.

The RTSP branch is fine: 11 rows after the two additions, last baseline 132.

Fix options, cheapest first: bump the default height prop past 212, or drop a line the overlay no longer needs now that Buffer is there. Both keep the change inside this component.

Consequence: with a video player's stats overlay enabled, the bottom line of text is drawn over the little bitrate/framerate charts, so neither reads cleanly.

11. Nitpicks / Optional — 2 findings

11.1 — needless lint suppression in the new test — nit

src/tests/libs/webrtc-jitter-buffer.test.ts wraps fakePeerConnection in /* eslint-disable jsdoc/require-jsdoc */ … /* eslint-enable */, but .eslintrc.cjs:33 sets ArrowFunctionExpression: false for that rule, so it would not fire on an arrow const. The pair of comments can go. (The as never casts around the fake are ugly but have in-tree company — src/tests/libs/signal.test.ts disables rules per line for the same kind of stub — so they are not worth changing.)

Consequence: none at runtime; one less piece of misleading scaffolding for the next reader of the test.

11.2 — test file does not mirror the source folder — nit

The code lands in src/libs/webrtc/jitter-buffer.ts but the test in src/tests/libs/webrtc-jitter-buffer.test.ts, where the existing convention mirrors the tree (src/tests/libs/connection/connection.test.ts for src/libs/connection/). src/tests/libs/webrtc/jitter-buffer.test.ts would match.

Consequence: none at runtime; tests stay findable from the file they cover.

Sections with nothing to report (8)

1. Correctness & Implementation Bugs — ✅ (moved body is identical to the deleted Session.setJitterBufferTarget, and Session.peerConnection is non-optional at session.ts:21, so the new guard is equivalent; both new RTCPeerConnection sites are now covered; the added delta math guards jitterBufferEmittedDelta > 0 and rewrites its baselines unconditionally, so it recovers within one 100 ms tick after a reconnect resets the cumulative counters)

2. Persistence & User Data — ✅ (the only persisted key involved, cockpit-jitter-buffer-target at src/stores/video.ts:59, is read and not reshaped; no key is added, renamed or removed, no migration, and the widened reach of the existing zero default is stated explicitly in the PR body)

3. AGENTS.md Adherence — ✅ (shared logic extracted once for its two call sites rather than duplicated, per "Reuse before reinventing"; no new dependency — uuid is already package.json:91; JSDoc added for the new constructor param, the getter and the free function, each with typed @param/@returns and no empty entries; the two new comments state why, not what)

4. Security — ✅ (no new dependency, no new network host — the ws://127.0.0.1:${port} URL is untouched, no encoded blobs, no hidden Unicode, no build-script/workflow/Electron-main changes, no eval/v-html; nothing in pr.json, pr.diff or complexity-report.json contains text addressed to the reviewer as an instruction)

7. Code Quality & Style — ✅ (complexity-report.json reports 247 functions measured across the 8 changed files with nothing triggered and truncated: false, so the PR raises no complexity or nesting number past the thresholds; the as any casts and paired console.debug calls in the new file are the moved code verbatim, and the delta arithmetic added to the .vue sits in the existing stats handler beside the identical processingDelay math, which keeps it trivial glue)

8. Commit Hygiene — ✅ (three commits from pr.json: refactor: webrtc:, fix: video:, video: — all matching the scope-prefix style of the recent history, one logical change each with the refactor separated from the behaviour change, no issue or PR references, no wip/fixup!/self-correcting commits, none oversized)

9. Tests — ✅ (the PR only adds src/tests/libs/webrtc-jitter-buffer.test.ts; no existing test was removed, skipped or weakened, and the three cases assert the video-only filter, the 0–4000 clamp and the zero-versus-null distinction against a stub rather than a live browser API)

10. Documentation — ✅ (RTSP stays Standalone-only and the Lite guard plus its explanatory dialog at src/stores/video.ts:420-434 are untouched, so the README parity table needs no edit; the setting's help text at src/views/ConfigurationVideoView.vue:239-249 names no protocol, so it becomes more accurate rather than stale now that both paths honour it)

Generated by Claude. This is advisory; a human reviewer must still approve.

[rafaellehmkuhl]

Review follow-up — round 1

Done

• src/components/VideoPlayerStatsForNerds.vue (6.1 — twelfth stats row overlaps the plots): bumped the default height prop from 200 to 212. The 12-row WebRTC list ends at baseline 144 and the plot band now starts at 152, restoring the same 8px clearance the 11-row list had. Kept both rows rather than dropping one — Processing and Buffer measure different things, and losing either weakens the comparison the stats were added for.
• src/tests/libs/webrtc/jitter-buffer.test.ts (11.2 — test file did not mirror the source folder): moved from src/tests/libs/webrtc-jitter-buffer.test.ts.

Done differently

• src/tests/libs/webrtc/jitter-buffer.test.ts (11.1 — needless lint suppression): the suppression was load-bearing, not needless. ArrowFunctionExpression: false does exempt the arrow itself, but .eslintrc.cjs:39 lists TSPropertySignature in contexts, and the old signature returned an inline type literal { receivers: Record<string, unknown>[] } — that property is what the rule fired on. Verified: dropping just the two comments gives 1:49 error Missing JSDoc comment jsdoc/require-jsdoc. So instead of deleting the comments I removed the reason they existed — split the fake into fakeReceivers (returns the array) and fakePeerConnection (wraps it), so no inline type literal remains. The suppression is gone, and the as never casts moved out of the three call sites as a side effect.

Won't change (with reasoning)

• 5.1 — go2rtc stats connections registered per reconnect and never removed: false positive, on both halves. @peermetrics/webrtc-stats removes closed connections itself, and you flagged that you could not check because node_modules was not in your checkout. It is in mine (v5.7.1), so:

  Its stats loop drops the closed connection before polling it — src/index.ts:447 is if (!pc || this.checkIfConnectionIsClosed(id, connectionId, pc)) continue, and checkIfConnectionIsClosed (:518-536) calls removeConnection when isConnectionClosed (:538-540) sees connectionState === 'closed' || iceConnectionState === 'closed'. Go2RTCManager.cleanup() calls pc.close(), which sets connectionState to closed synchronously, so the stale entry is gone on the next 100ms tick. There is a second sweep at 1Hz doing the same (:502-516). Nothing accumulates.

  The second half falls out of the same line: the closed connection is continued before any event is built for it, so a dead peer cannot emit a straggler stats event that overwrites live numbers. ev.peerId filtering would guard against something that cannot happen. Two peers coexist in peersToMonitor for at most one tick, and only the live one emits.

  Two smaller corrections to the finding's cost model: getStatsInterval drives one shared interval per WebRTCStats instance (:397-413, guarded by if (this.monitoringSetInterval) return), not one per connection, so registering an extra peer adds an iteration to an existing tick rather than a new 10Hz loop. And the intervals only start when the peer count goes 0→1 (:388-391), which on a reconnect it never does, since the new pc is registered while the old entry is still present.

  Happy to be shown wrong if you can point at a path where the old pc is not closed before the new one registers — cleanup() is the only route into a reconnect and it always closes.

[rafaellehmkuhl]

/review

[github-actions]

Automated PR Review — round 2

Warning

⚠️ IMPORTANT FIXES REQUIRED — 1 open finding (1 major, disputed); 3 closed since round 1.

Cockpit plays RTSP video through a second, separate connection manager that never applied the user's video-buffer setting, so that setting silently did nothing on those streams. This PR moves the per-receiver tuning into one shared function and hands it to the RTSP manager too, and additionally exposes the RTSP connection's browser-side receive statistics (buffer delay, dropped frames) to the debug overlay and the data lake, which previously only had them for the other stream type.

What still needs attention

#	Problem	What it means	Severity	Status
5.1	Statistics monitors added per RTSP reconnect, with no removal in our code	Each time an RTSP camera drops and reconnects, Cockpit registers another measurement target and never unregisters the dead one, so a long session with a flaky camera could get slower and slower — unless the third-party statistics library cleans up on its own, which the author says it does and this checkout cannot confirm.	major	💬

🙋 Decisions for a human

5.1 — go2rtc stats connections registered per reconnect and never removed
Author's argument: the statistics library drops a closed peer connection by itself before ever polling or emitting for it, so nothing accumulates and no dead connection can overwrite live numbers; the author read this in their local copy of the package, which is not present in this checkout.

• Accept the argument and leave the code as it is
• Ask for the explicit removal and the ev.peerId filter anyway

The checkbox records the decision; the finding itself closes only on /resolve 5.1 <reason>.

Since round 1 — 3 closed, 1 disputed, comparing f744099 → 66d33f9

Range. f744099fee63d0b42324fa51074c670eede6631d → 66d33f9c3afe51d03974f2e5368e730c5865746d.

The increment is not usable this round. All three commits in pr.json carry the same committedDate (2026-08-19T14:40:22Z) and f744099 is not among their oids, so the branch was force-pushed with the round-1 fixes folded into their original commits. incremental.diff is consequently byte-for-byte the whole pr.diff and carries no isolated increment. Every status below was therefore judged against pr.diff and the base checkout, not against the increment.

No /resolve commands have been issued on this PR (resolutions.json is empty), so nothing was closed by a maintainer decision this round.

6.1 — Added Buffer row pushes the WebRTC stats overlay's last line into the graph area — ✅ Addressed

The finding asked for the 12-row list to regain clearance over the plot band. src/components/VideoPlayerStatsForNerds.vue now defaults height to 212 instead of 200. Checked against the geometry rather than the claim: plotHeight is still 60 (base :83) and drawPlot anchors to the bottom (y = height - normalizeValue(...), base :116), so the plot band starts at y=152; the WebRTC list's 12th row has its baseline at 12 + 11 * 12 = 144, which is the same 8 px of clearance the 11-row list had at height 200. The RTSP branch (11 rows, last baseline 132) gains clearance. src/components/widgets/VideoPlayer.vue:8 still mounts the component with only :stream-name, so the default is what actually applies. Both rows were kept rather than one being dropped, which the finding offered as the alternative.

11.1 — Needless jsdoc/require-jsdoc suppression in the new test — ✅ Addressed

The suppression pair is gone from src/tests/libs/webrtc/jitter-buffer.test.ts, and the fake is split into fakeReceivers / fakePeerConnection so no inline type literal remains.

The finding's stated reason was wrong, and the author is right about why: .eslintrc.cjs:39 lists TSPropertySignature in the rule's contexts, so the rule fires on the members of an inline type literal regardless of ArrowFunctionExpression: false. That is confirmed in-tree rather than only in the author's reply — src/composables/go2rtc.ts:145-154 and :36-53 both carry a JSDoc block on every member of an inline type literal for exactly this reason. Round 1 read only the ArrowFunctionExpression line and missed the contexts entry.

11.2 — Test file does not mirror the source folder — ✅ Addressed

The test now lands at src/tests/libs/webrtc/jitter-buffer.test.ts, mirroring src/libs/webrtc/jitter-buffer.ts.

5.1 — go2rtc stats connections registered per reconnect and never removed — 💬 Disputed (stays open)

No code changed at the two registration sites or in Go2RTCManager, so this is settled by argument, not by the diff — which leaves it open by the rules of this review. The full finding is reprinted in section 5 below with what I could and could not verify.

Discussion since round 1. One substantive comment, from the PR author (#issuecomment-5343722105), plus a bare /review treated as noise. The comment reports the two nits and the overlay height as done (verified above), and disputes 5.1 as a "false positive, on both halves", quoting line numbers from its local @peermetrics/webrtc-stats v5.7.1 install and closing with "Happy to be shown wrong if you can point at a path where the old pc is not closed before the new one registers". Those package internals are a claim about code this checkout does not contain (node_modules is absent and there is no network access); the part of it that lives in our own tree is checked in section 5. The PR body was also rewritten since round 1 and now leads with "This is not a latency fix" — the Change map below is rebuilt against the new body.

Nothing in pr.json, pr.diff, complexity-report.json or new-comments.json contains text addressed to the reviewer as an instruction.

Change map — what was established before judging

Claims (from the rewritten PR body — the author's hypothesis, checked against the code):

• The jitter-buffer target setting was only ever applied to mavlink-camera-manager streams. Verified. Base src/composables/go2rtc.ts:67-92 creates its own peer connection and only sets contentHint in ontrack; no receiver tuning anywhere in the class. The other path does it at src/composables/webRTC.ts:239. The setting is real and defaults to zero (src/stores/video.ts:59, useBlueOsStorage<number>('cockpit-jitter-buffer-target', 0)), UI at src/views/ConfigurationVideoView.vue:253-262.
• The receiver tuning became a free function, with no behavior change for WebRTC streams. Verified. The body of the new src/libs/webrtc/jitter-buffer.ts is identical to the deleted Session.setJitterBufferTarget (src/libs/webrtc/session.ts:132-171 at base), and Session.peerConnection is a non-optional field (src/libs/webrtc/session.ts:21), so the new if (this.session?.peerConnection) guard is equivalent to the old this.session?.setJitterBufferTarget(...).
• RTSP streams now apply the configured target. Verified. setJitterBufferTarget(pc, this.jitterBufferTarget) in the go2rtc ontrack handler, with the value passed at construction from src/stores/video.ts:443.
• Keyed by a fresh id per connection so a reconnect re-registers instead of leaving the stats pinned to a closed one. Half verified. Re-registration does happen: connect() sets connectionId = uuid() after building the new pc, and both watchers re-run because connect() writes signallerStatus/streamStatus, which are reactive refs inside activeStreams. Removal of the previous entry is not in this code — see 5.1.
• RTCRtpReceiver.jitterBufferTarget shipped in Chrome 124, and Electron 29.4.6 pins Chromium 122, so only playoutDelayHint does anything in this build. Partly verifiable. The pin is real: package.json:126 is "electron": "^29.2.0" and yarn.lock:5096-5097 resolves it to 29.4.6. Which Chromium that Electron carries, and which Chrome version shipped the attribute, are upstream facts this checkout cannot confirm; nothing in the code contradicts them, and the code writes both attributes anyway.
• The target is a floor rather than a ceiling in libwebrtc, so zero cannot help the reported RTSP latency. Not verifiable here (libwebrtc sources are not in this repo). Recorded as the author's reasoning, not adopted. It is consistent with the code, which writes playoutDelayHint = null whenever the target is falsy.
• yarn lint clean, yarn test:unit passing, yarn typecheck exits early in this repo. Not verifiable here (no install, and PR code is not executed). Treated as an unverified author claim, which no finding below depends on.

Failure site: src/composables/go2rtc.ts, connect() and its pc.ontrack handler (base :67-92) — the code that omits the tuning. It is in the diff, and the fix is at the shared level: both new RTCPeerConnection sites in the tree (src/libs/webrtc/session.ts:114, src/composables/go2rtc.ts:74) now route through one function.

Entry points

Function	Reached from	Frequency
setJitterBufferTarget (src/libs/webrtc/jitter-buffer.ts, new)	WebRTCManager.onTrackAdded (src/composables/webRTC.ts:237) and pc.ontrack in Go2RTCManager.connect	one-shot per (re)connection
Go2RTCManager.connect (changed)	start(), scheduleReconnect() (RECONNECT_DELAY_MS = 3000) and the CONNECT_WATCHDOG_MS = 8000 forced reconnect	per user action, then automatically every ~3-11 s while the source is unreachable
Go2RTCManager constructor (changed)	activateStream (src/stores/video.ts:443), driven by VideoPlayer's 1 s activation polling	per user action
Go2RTCManager.peerConnection getter (new)	getStreamPeerConnection (src/stores/video.ts:621)	per activeStreams mutation
getStreamPeerConnection (src/stores/video.ts:621, changed)	deep watch(videoStore.activeStreams) at src/stores/omniscientLogger.ts:190 and src/components/VideoPlayerStatsForNerds.vue:198	per stream-state change — several times per connect, and again on every automatic reconnect
watch(videoStore.activeStreams) callback (src/stores/omniscientLogger.ts:190, changed)	Pinia store, alive for the whole session	per stream-state change
draw (src/components/VideoPlayerStatsForNerds.vue:97, changed)	requestAnimationFrame loop	per frame
webrtcStats.on('stats') handler (src/components/VideoPlayerStatsForNerds.vue:248, changed)	WebRTCStats poll, getStatsInterval: 100	10 Hz per monitored connection
Session.setJitterBufferTarget (deleted)	no remaining caller at head (searching the tree for setJitterBufferTarget finds only the new free function and its two call sites)	n/a — correctly removed, not orphaned

Invariants

• Every receive-side peer connection Cockpit creates gets the configured target. Producers: src/libs/webrtc/session.ts:114 and src/composables/go2rtc.ts:74. Both covered after this PR; there is no third new RTCPeerConnection in src/.
• Stats consumers reach a stream's peer connection through one accessor. getStreamPeerConnection (src/stores/video.ts:621) is the chokepoint, and the PR removes omniscientLogger's direct webRtcManager.session bypass — the single-consumer form the guidelines prefer. The remaining direct webRtcManager.session read (src/stores/video.ts:515) is teardown, not stats.
• A peerId identifies a monitored connection for the life of a WebRTCStats instance. At base this held: session.consumerId is stable per manager, so addConnection ran once per stream. The PR replaces it with a fresh uuid per go2rtc connection while adding no removal call — the invariant behind 5.1. Sites that register: src/stores/omniscientLogger.ts:201 (store, never torn down) and src/components/VideoPlayerStatsForNerds.vue:204 (bounded by widget unmount at :287). Sites that remove, in our code: only that destroy().
• A new go2rtc peer connection never coexists open with the previous one. Verified: connect() calls cleanup() as its first statement (src/composables/go2rtc.ts:70), and cleanup() calls pc.close() and nulls the field (:250-256) before any new RTCPeerConnection is built. The watchdog path also goes through cleanup() (:213). This is the half of the author's 5.1 argument that lives in our tree, and it holds.

5. Performance — 1 finding (disputed, carried from round 1)

5.1 — go2rtc stats connections are registered per reconnect and never removed — major — (carried from round 1, disputed)

src/composables/go2rtc.ts assigns a fresh connectionId = uuid() inside connect(), and src/stores/video.ts:627-631 returns it as both peerId and sessionId. The two registration sites guard on that id:

• src/stores/omniscientLogger.ts:199 — if (webrtcStreamStats[streamName].peersToMonitor[pcInfo.peerId]) return
• src/components/VideoPlayerStatsForNerds.vue:203 — the same guard on the component's own instance

Because the id is new on every connection, the guard never matches after a reconnect and addConnection runs again — which is the intended half. The missing half is removal: there is no removeConnection/removePeer-style call anywhere in src/ (only webrtcStats.destroy() on unmount at VideoPlayerStatsForNerds.vue:287), so in our code nothing takes the previous, now-closed connection out of peersToMonitor. Go2RTCManager reconnects on its own — RECONNECT_DELAY_MS = 3000 on disconnected/failed/socket close, plus the CONNECT_WATCHDOG_MS = 8000 forced reconnect when the camera is unreachable (src/composables/go2rtc.ts:188-215) — so with an offline or flapping RTSP camera the registration runs roughly every 11 seconds, into a WebRTCStats instance that lives in a Pinia store for the whole session. Second, smaller half: both 'stats' handlers (src/stores/omniscientLogger.ts:223-234, src/components/VideoPlayerStatsForNerds.vue:248-277) read ev.data.video.inbound[0] and ignore ev.peerId, which was safe while one peer per stream was monitored.

The author disputes both halves (comment), on the grounds that @peermetrics/webrtc-stats v5.7.1 removes a closed connection itself before polling or emitting for it, that its polling is one shared interval per instance rather than one per connection, and that the interval only starts when the peer count goes 0→1. What I can check in this repository supports the premise his argument rests on: connect() calls cleanup() as its first statement (src/composables/go2rtc.ts:70), cleanup() calls pc.close() and nulls the field (:250-256), and there is no other route to a new pc, so the old connection is always closed before the new one is registered. What I cannot check is the step that does the actual work in his argument — that the library then sweeps that closed connection out of peersToMonitor and skips it before building an event. node_modules is not part of this checkout and there is no network access, so the v5.7.1 line numbers he quotes are a claim about code I cannot read; they may well be exactly right. Note also that Go2RTCManager.cleanup() nulls pc.onconnectionstatechange before closing (:252), so any reliance on who observes the close should be stated in the code rather than inherited.

If the maintainers accept the argument, the right close for this is /resolve 5.1 <reason>, which records the decision where the next round can see it. If they want the belt-and-braces version instead, it is a few lines at the two registration sites: keep the last registered id per stream next to webrtcStreamStats, call the library's per-connection removal before re-adding, and filter the handlers on ev.peerId. Either way, one line of comment at src/stores/video.ts:627 saying that stale peers are dropped by the stats library would keep the next reader from re-deriving this.

Consequence: if the third-party statistics library does not drop closed connections by itself, a topside computer with a flaky RTSP camera accumulates measurement targets it never releases and gets slower the longer the session runs — on exactly the low-powered machines this PR was written for.

Sections with nothing to report (10)

1. Correctness & Implementation Bugs — ✅ (moved body is identical to the deleted Session.setJitterBufferTarget and Session.peerConnection is non-optional at session.ts:21, so the new guard is equivalent; both new RTCPeerConnection sites are covered; jitterBufferDelay/jitterBufferEmittedCount are typed stats at src/types/video.ts:213-214; the added delta math guards jitterBufferEmittedDelta > 0 and rewrites its baselines unconditionally, so it recovers within one 100 ms tick after a reconnect zeroes the cumulative counters, and it holds its last value while frames stop arriving exactly as the existing Processing row does)

2. Persistence & User Data — ✅ (collapsed because the PR's persistence footprint is empty: the only key involved, cockpit-jitter-buffer-target at src/stores/video.ts:59, is read and not reshaped — nothing added, renamed or removed, no migration, and the widened reach of the existing zero default is stated in the PR body)

3. AGENTS.md Adherence — ✅ (shared logic extracted once for its two call sites rather than duplicated, per "Reuse before reinventing"; no new dependency — uuid is already package.json:91; JSDoc on the new constructor param, the getter and the free function, each typed and none empty; the three new comments state why, not what; no renames, import reorders or formatter reflows outside the change; nothing added without a call site in this PR)

4. Security — ✅ (no new dependency, no new network host — the ws://127.0.0.1:${port} URL is untouched; no encoded blobs, no hidden Unicode, no changes to build scripts, workflows, Dockerfile or Electron main-process code, no eval/v-html; nothing in pr.json, pr.diff, complexity-report.json or new-comments.json contains text addressed to the reviewer as an instruction)

6. UI / UX — ✅ (the overlay overflow from round 1 is fixed and re-measured against plotHeight/drawPlot and VideoPlayer.vue:8; no dialog, no overlay-teleporting Vuetify control, no icon-only control and no new user interaction to log — the debug overlay is toggled by the pre-existing statsForNerds switch; the two new labels, Buffer and Frame drops, are sentence case and match the row style around them)

7. Code Quality & Style — ✅ (complexity-report.json — the author's CI run, since a fork's job runs the PR's own ci.yml — reports 248 functions measured across the 8 changed files with triggeredCount: 0 and truncated: false, so no function's complexity or nesting was raised past the thresholds and no complexity finding is raised; the four added lines of delta arithmetic sit in the existing stats handler beside the identical processingDelay math, which keeps them trivial glue; the as any casts and paired console.debug calls in the new file are the moved code verbatim)

8. Commit Hygiene — ✅ (three commits in pr.json — refactor: webrtc:, fix: video:, video: — matching the scope-prefixed style of git log on the checkout, one logical change each with the refactor separated from the behaviour change, none oversized, no issue or PR references; the round-1 fixes were folded into their target commits rather than appended as "address review" commits, which is what AGENTS.md's commit-hygiene rules ask for)

9. Tests — ✅ (the PR adds src/tests/libs/webrtc/jitter-buffer.test.ts and removes no existing test; its three cases — video-only receiver filter, the 0–4000 clamp, and zero leaving the legacy hint null — survived the round-1 rework of the fake unchanged in what they assert)

10. Documentation — ✅ (RTSP stays Standalone-only and the Lite guard plus its explanatory dialog at src/stores/video.ts:420-434 are untouched, so the README parity table needs no edit; the setting's help text at src/views/ConfigurationVideoView.vue:239-262 names no protocol, so it becomes more accurate rather than stale now that both paths honour it)

11. Nitpicks / Optional — ✅ (both round-1 nits are closed this round — the lint suppression and the test's location, see the since-last-round block — and the reread of pr.diff turned up nothing new at this level)

Generated by Claude. This is advisory; a human reviewer must still approve.

[rafaellehmkuhl]

Review follow-up — round 2

Done

• src/stores/video.ts (5.1 — the assumption about who drops the closed peer was inherited, not stated): added the one line you asked for at the go2rtc branch of getStreamPeerConnection, saying the fresh id re-registers on reconnect and that the stats library drops the closed peer on its next poll. That is the part of the finding that survives regardless of who is right about the library, so it is in.

Won't change (with reasoning)

• 5.1 — explicit removeConnection + ev.peerId filter: still declined. Re-verified against node_modules/@peermetrics/webrtc-stats/dist/index.js in this checkout (v5.7.1), the built file rather than the source I quoted last round, so the line numbers differ:

  • :645 — if (!pc || this.checkIfConnectionIsClosed(id, connectionId, pc)) continue, inside the stats poll.
  • :704-719 — checkIfConnectionIsClosed calls removeConnection({ pc }) and returns true, so the closed peer is dropped and skipped before any event is built for it.
  • :720-722 — isConnectionClosed is connectionState === 'closed' || iceConnectionState === 'closed'.
  • :696-702 — a second 1 Hz sweep running the same check.
  • :602-605 — one shared setInterval per instance, guarded by if (this.monitoringSetInterval) return, so an extra peer is an extra iteration on an existing tick, not a new 10 Hz loop.

  You confirmed the half that lives in our tree: connect() calls cleanup() first and cleanup() closes the pc, so the old connection is always closed before the new one registers. pc.close() sets connectionState synchronously, so the stale entry is gone on the next 100 ms tick and cannot emit in between — which is why the ev.peerId filter would guard against something that cannot happen.

  On the onconnectionstatechange point: cleanup() nulls our handler, and the library never installs one of its own for this — it polls pc.connectionState from the two loops above. So nulling it does not affect the sweep. The new comment states the dependency in code rather than leaving it inherited.

  One correction to my round-1 reply that does not change the conclusion: the closed-peer sweep inside addConnection (:382-391) is keyed by peerId and never fires for us, since our peerId is new every time. The removal comes from the poll and the 1 Hz sweep, not from registration.

[rafaellehmkuhl]

/review

[rafaellehmkuhl]

/resolve 5.1 - Should be fixed.

[github-actions]

Recorded: rafaellehmkuhl resolved 5.1. Comment /review to apply it.

[github-actions]

Automated PR Review — round 3

Warning

⚠️ IMPORTANT FIXES REQUIRED — 1 open finding (1 major, disputed); 3 closed in earlier rounds, none closed this round.

Cockpit plays RTSP video through a second, separate connection manager that never applied the user's video-buffer setting, so that setting silently did nothing on those streams. This PR moves the per-receiver tuning into one shared function and hands it to the RTSP manager too, and additionally exposes the RTSP connection's browser-side receive statistics (buffer delay, dropped frames) to the debug overlay and to the data lake, which previously had them only for the other stream type.

What still needs attention

#	Problem	What it means	Severity	Status
5.1	Statistics monitors added per RTSP reconnect, with no removal in our code	Each time an RTSP camera drops and reconnects, Cockpit registers another measurement target and never unregisters the dead one, so a long session with a flaky camera could get slower and slower — unless the third-party statistics library cleans up on its own, which the author says it does and this checkout still cannot confirm.	major	💬

🙋 Decisions for a human

5.1 — go2rtc stats connections registered per reconnect and never removed
Author's argument: the statistics library itself drops a closed peer connection before it ever polls or emits for it — this round he cites the built file shipped in the package rather than the source he quoted last round, plus a second once-per-second sweep doing the same check and a single shared poll interval per instance — so nothing accumulates and no dead connection can overwrite live numbers; closing a peer connection marks it closed synchronously, so the stale entry is gone by the next 100 ms tick. He also corrects one detail of his round-1 reply that does not change his conclusion. All of it is read from a package copy that is not present in this review checkout.

• Accept the argument and leave the code as it is
• Ask for the explicit removal and the ev.peerId filter anyway

The checkbox records the decision; the finding itself closes only on /resolve 5.1 <reason>.

Since round 2 — 0 closed, 1 still disputed, comparing 66d33f9 → e17a7e4

Range. 66d33f9c3afe51d03974f2e5368e730c5865746d → e17a7e4f3633912ebe26a2236b86c30c931cd5b6.

The increment is again not usable as a delta. 66d33f9 is not among the three commit oids in pr.json (4a59ac6, e70a829, e17a7e4), and the third commit's committedDate (2026-08-19T14:55:29Z) is later than the other two (14:40:22Z), so the branch was force-pushed with the last commit amended in place. incremental.diff consequently contains that whole commit rather than what changed since round 2 — it lists the Buffer / Frame drops rows and the height default of 212, all of which round 2 already reviewed as present at 66d33f9. Every status below was therefore judged against pr.diff and the base checkout. The one change that is genuinely new since round 2 is the added comment line at src/stores/video.ts (pr.diff line 289).

No /resolve commands have been issued on this PR (resolutions.json is []), so nothing was closed by a maintainer decision, and no resolution id needed matching against the ledger.

5.1 — go2rtc stats connections registered per reconnect and never removed — 💬 Disputed (stays open)

One of the three things this finding asked for landed in code: src/stores/video.ts now carries // Fresh id per connection, so a reconnect re-registers; the stats library drops the closed peer on its next poll. at the go2rtc branch of getStreamPeerConnection, which is the "either way" item in the round-2 remedy — the assumption is now stated where the next reader meets it instead of being inherited. The two substantive halves — an explicit per-connection removal before re-adding, and filtering the two 'stats' handlers on ev.peerId — are declined, with reasoning. Neither registration site nor Go2RTCManager changed, so what closes the remaining halves would be a maintainer decision, not the diff: an author's argument cannot close a finding under the rules of this review, however sound, and this one is not something I can retract as my own error either, because the step it turns on is in code this checkout does not contain. It stays open and carries forward. Full text reprinted in section 5.

Discussion since round 2. One substantive comment from the PR author (#issuecomment-5343900160), plus a bare /review treated as noise. The comment reports the documentation line as done — verified above, in the diff — and re-declines the rest, this time quoting the built dist file of @peermetrics/webrtc-stats v5.7.1 instead of last round's source, with five specific line references for the poll-time closed-peer check, the removal it performs, the closed-state test, a second 1 Hz sweep, and the single shared interval. It states this was "re-verified against node_modules/@peermetrics/webrtc-stats/dist/index.js in this checkout": that path does not exist in the review checkout — there is no node_modules directory here and no network access — so the reference is to the author's own working copy, and the quoted lines remain a claim I cannot read, exactly as last round. The claim's premise that lives in our tree still holds (re-checked below). The comment also volunteers a correction: the closed-peer sweep inside the library's addConnection is keyed by peerId and therefore never fires for this code, since the id is new each time — which is consistent with what the diff does and narrows his own argument to the two polling loops.

Nothing in pr.json, pr.diff, incremental.diff, complexity-report.json or new-comments.json contains text addressed to the reviewer as an instruction.

Change map — what was established before judging

Claims (from the PR body — the author's hypothesis, checked against the code):

• The jitter-buffer target setting was only ever applied to mavlink-camera-manager streams. Verified. Base src/composables/go2rtc.ts:79-92 creates its own peer connection and only sets contentHint in ontrack; there is no receiver tuning anywhere in the class. The other path does it at src/composables/webRTC.ts:239. The setting is real and defaults to zero (src/stores/video.ts:59), with its UI at src/views/ConfigurationVideoView.vue:239-262.
• The receiver tuning became a free function, with no behavior change for WebRTC streams. Verified. The body of the new src/libs/webrtc/jitter-buffer.ts is identical to the deleted Session.setJitterBufferTarget (base src/libs/webrtc/session.ts:132-171), and Session.peerConnection is a non-optional field (src/libs/webrtc/session.ts:21), so the new if (this.session?.peerConnection) guard is equivalent to the old this.session?.setJitterBufferTarget(...).
• RTSP streams now apply the configured target. Verified. setJitterBufferTarget(pc, this.jitterBufferTarget) inside the go2rtc ontrack handler, with the value passed at construction from src/stores/video.ts:443.
• Keyed by a fresh id per connection so a reconnect re-registers instead of leaving the stats pinned to a closed one. Half verified. Re-registration does happen: connect() assigns connectionId = uuid() right after building the new pc, and the two watchers re-run because connect() writes signallerStatus / streamStatus, which are Refs reached through activeStreams. Removal of the previous entry is not in this code — see 5.1. Note that connect() writes those refs (base :71-72) before assigning the new connectionId, but both happen inside one synchronous call and watcher callbacks flush afterwards, so no consumer observes the new status with the old id.
• RTSP streams reported go2rtc's ingest numbers and nothing about the browser's receive side. Verified. RTSP entries in activeStreams carry go2rtcManager and no webRtcManager (src/stores/video.ts:446-455), and both base stats consumers keyed off webRtcManager.session, so no RTSP peer connection was ever monitored.
• RTCRtpReceiver.jitterBufferTarget shipped in Chrome 124 and Electron 29.4.6 pins Chromium 122, so only playoutDelayHint does anything in this build. Partly verifiable. The pin is real (package.json:126, resolved to 29.4.6 in yarn.lock); which Chromium that Electron carries and which Chrome shipped the attribute are upstream facts this checkout cannot confirm. The code writes both attributes regardless.
• The target is a floor rather than a ceiling in libwebrtc, so zero cannot help the reported RTSP latency. Not verifiable here. Recorded as the author's reasoning, not adopted; it is consistent with the code, which writes playoutDelayHint = null whenever the target is falsy.
• yarn lint clean, yarn test:unit passing, yarn typecheck exits early in this repo. Not verifiable here (no install, and PR code is not executed). No finding below depends on it.

Failure site: src/composables/go2rtc.ts, connect() and its pc.ontrack handler (base :67-92) — the code that omits the tuning. It is in the diff, and the fix is made at the shared level: both new RTCPeerConnection sites in the tree (src/libs/webrtc/session.ts:114, src/composables/go2rtc.ts:74) now route through one function.

Entry points

Function	Reached from	Frequency
setJitterBufferTarget (src/libs/webrtc/jitter-buffer.ts, new)	WebRTCManager.onTrackAdded (src/composables/webRTC.ts:237) and pc.ontrack in Go2RTCManager.connect	one-shot per (re)connection
Go2RTCManager.connect (changed)	start(), scheduleReconnect() (RECONNECT_DELAY_MS = 3000) and the CONNECT_WATCHDOG_MS = 8000 forced reconnect	per user action, then automatically every ~3-11 s while the source is unreachable
Go2RTCManager constructor (changed)	activateStream (src/stores/video.ts:443), driven by VideoPlayer's 1 s activation polling	per user action
Go2RTCManager.peerConnection getter (new)	getStreamPeerConnection (src/stores/video.ts:621)	per activeStreams mutation
getStreamPeerConnection (src/stores/video.ts:621, changed)	deep watch(videoStore.activeStreams) at src/stores/omniscientLogger.ts:190 and src/components/VideoPlayerStatsForNerds.vue:198	per stream-state change — several times per connect, and again on every automatic reconnect
watch(videoStore.activeStreams) callback (src/stores/omniscientLogger.ts:190, changed)	Pinia store, alive for the whole session	per stream-state change
draw (src/components/VideoPlayerStatsForNerds.vue:97, changed)	requestAnimationFrame loop	per frame
webrtcStats.on('stats') handler (src/components/VideoPlayerStatsForNerds.vue:248, changed)	WebRTCStats poll, getStatsInterval: 100	10 Hz per monitored connection
Session.setJitterBufferTarget (deleted)	no remaining caller at head — searching the tree for setJitterBufferTarget finds only the new free function and its two call sites plus the test	n/a — correctly removed, not orphaned

Invariants

• Every receive-side peer connection Cockpit creates gets the configured target. Producers: src/libs/webrtc/session.ts:114 and src/composables/go2rtc.ts:74. Both covered after this PR; there is no third new RTCPeerConnection in src/.
• Stats consumers reach a stream's peer connection through one accessor. getStreamPeerConnection (src/stores/video.ts:621) is the chokepoint, and the PR removes omniscientLogger's direct webRtcManager.session bypass — the single-consumer form the guidelines prefer. The remaining direct webRtcManager.session read (src/stores/video.ts:515) is teardown, not stats.
• A peerId identifies a monitored connection for the life of a WebRTCStats instance. At base this held: session.consumerId is stable per manager, so addConnection ran once per stream. The PR replaces it with a fresh uuid per go2rtc connection while adding no removal call — the invariant behind 5.1. Sites that register: src/stores/omniscientLogger.ts:201 (store, never torn down) and src/components/VideoPlayerStatsForNerds.vue:204 (bounded by widget unmount at :287). Sites that remove, in our code: only that destroy(). As of this round the assumption is at least written down at the accessor (src/stores/video.ts, pr.diff line 289).
• A new go2rtc peer connection never coexists open with the previous one. Verified: connect() calls cleanup() as its first statement (src/composables/go2rtc.ts:70), and cleanup() calls pc.close() and nulls the field (:250-256) before any new RTCPeerConnection is built; the watchdog path also goes through cleanup() (:213). This is the half of the author's 5.1 argument that lives in our tree, and it holds.

5. Performance — 1 finding (disputed, carried from round 1)

5.1 — go2rtc stats connections are registered per reconnect and never removed — major — (carried from round 1, disputed; documentation half addressed this round)

src/composables/go2rtc.ts assigns a fresh connectionId = uuid() inside connect(), and src/stores/video.ts (the new go2rtc branch of getStreamPeerConnection, base :621-628) returns it as both peerId and sessionId. The two registration sites guard on that id:

• src/stores/omniscientLogger.ts:199 — if (webrtcStreamStats[streamName].peersToMonitor[pcInfo.peerId]) return
• src/components/VideoPlayerStatsForNerds.vue:203 — the same guard on the component's own instance

Because the id is new on every connection, the guard never matches after a reconnect and addConnection runs again — which is the intended half. The missing half is removal: there is no removeConnection/removePeer-style call anywhere in src/ (only webrtcStats.destroy() on unmount at VideoPlayerStatsForNerds.vue:287), so in our code nothing takes the previous, now-closed connection out of peersToMonitor. Go2RTCManager reconnects on its own — RECONNECT_DELAY_MS = 3000 on disconnected/failed/socket close, plus the CONNECT_WATCHDOG_MS = 8000 forced reconnect when the camera is unreachable (src/composables/go2rtc.ts:188-215) — so with an offline or flapping RTSP camera the registration runs roughly every 11 seconds, into a WebRTCStats instance that lives in a Pinia store for the whole session (src/stores/omniscientLogger.ts:182, never destroyed). Second, smaller half: both 'stats' handlers (src/stores/omniscientLogger.ts:223-234, src/components/VideoPlayerStatsForNerds.vue:248-277) read ev.data.video.inbound[0] and ignore ev.peerId, which was safe while one peer per stream was monitored.

What changed this round. The accessor now documents the assumption: src/stores/video.ts carries // Fresh id per connection, so a reconnect re-registers; the stats library drops the closed peer on its next poll. That was the third item in the round-2 remedy and it is done — the dependency on third-party behavior is stated in code instead of being inherited by the next reader. Nothing changed at either registration site, in either 'stats' handler, or in Go2RTCManager.

The author disputes both remaining halves (comment), now quoting the built dist/index.js of @peermetrics/webrtc-stats v5.7.1: a closed-connection check inside the stats poll that both removes the peer and continues past it, the closed-state test it uses, a second once-per-second sweep running the same check, and one shared setInterval per instance guarded so an extra peer costs an extra iteration rather than a new 10 Hz loop. What I can check in this repository still supports the premise his argument rests on: connect() calls cleanup() first (src/composables/go2rtc.ts:70), cleanup() calls pc.close() and nulls the field (:250-256), and there is no other route to a new pc, so the old connection is always closed before the new one registers. What I still cannot check is the step that does the work in his argument — that the library then sweeps that closed connection out and skips it before building an event. There is no node_modules directory in this checkout and no network access, so the dist lines he cites are code I cannot read; they may well be exactly right, and he is the only party here who can see them.

One in-tree detail corroborates the "the library does not watch our handlers" part without verifying it: Go2RTCManager installs its reconnect logic on the single-slot property pc.onconnectionstatechange (src/composables/go2rtc.ts:94), whereas Session — the connection this library has always monitored — uses peerConnection.addEventListener('connectionstatechange', ...) (src/libs/webrtc/session.ts:125). If the library assigned that property for its own bookkeeping it would silently replace Cockpit's RTSP reconnect handler, since addConnection runs after connect() has installed it, and RTSP streams would stop reconnecting altogether. Matching session.ts:125 and using addEventListener here would make the go2rtc manager immune to that regardless of what the library does, which is worth the one-line change whichever way 5.1 is settled.

If the maintainers accept the argument, the right close for this is /resolve 5.1 <reason>, which records the decision where the next round can see it. If they want the belt-and-braces version instead, it is a few lines at the two registration sites: keep the last registered id per stream next to webrtcStreamStats, call the library's per-connection removal before re-adding, and filter the handlers on ev.peerId.

Consequence: if the third-party statistics library does not drop closed connections by itself, a topside computer with a flaky RTSP camera accumulates measurement targets it never releases and gets slower the longer the session runs — on exactly the low-powered machines this PR was written for.

Sections with nothing to report (10)

1. Correctness & Implementation Bugs — ✅ (moved body is identical to the deleted Session.setJitterBufferTarget and Session.peerConnection is non-optional at session.ts:21, so the new guard is equivalent; both new RTCPeerConnection sites are covered; jitterBufferDelay / jitterBufferEmittedCount are typed stats at src/types/video.ts:213-214; the added delta math guards jitterBufferEmittedDelta > 0 and rewrites its baselines unconditionally, so it recovers within one 100 ms tick after a reconnect zeroes the cumulative counters and holds its last value while frames stop arriving, exactly as the existing Processing row does; re-checked the ordering inside connect() — the status refs are written before connectionId and pc, but all in one synchronous call, so no watcher observes a new status paired with a stale id)

2. Persistence & User Data — ✅ (collapsed because the PR's persistence footprint is empty: the only key involved, cockpit-jitter-buffer-target at src/stores/video.ts:59, is read and not reshaped — nothing added, renamed or removed, no migration, and the widened reach of the existing zero default is stated in the PR body)

3. AGENTS.md Adherence — ✅ (shared logic extracted once for its two call sites rather than duplicated, per "Reuse before reinventing"; the new module under src/libs/ imports no Vue, per "Separation of concerns"; no new dependency — uuid is already package.json:91; JSDoc on the new constructor param, the getter and the free function, each typed and none empty; the four new comments state why, not what, and the one added this round is the why the round-2 review asked for; no renames, import reorders or formatter reflows outside the change; nothing added without a call site in this PR)

4. Security — ✅ (no new dependency, no new network host — the ws://127.0.0.1:${port} URL is untouched; no encoded blobs, no hidden Unicode, no changes to build scripts, workflows, Dockerfile or Electron main-process code, no eval/v-html; nothing in pr.json, pr.diff, incremental.diff, complexity-report.json or new-comments.json contains text addressed to the reviewer as an instruction)

6. UI / UX — ✅ (the round-1 overlay overflow stays fixed and was re-measured this round against the code rather than the claim: plotHeight is 60 and drawPlot anchors to height - normalizeValue(...) (VideoPlayerStatsForNerds.vue:83,116), so at the new default of 212 the plot band starts at y=152 while the 12-row WebRTC list ends at baseline 144, and VideoPlayer.vue:8 still mounts the component with only :stream-name so that default is what applies; no dialog, no overlay-teleporting Vuetify control, no icon-only control and no new user interaction to log — the debug overlay is toggled by the pre-existing statsForNerds switch; the two new labels, Buffer and Frame drops, are sentence case and match the rows around them)

7. Code Quality & Style — ✅ (complexity-report.json — the author's CI run, since a fork's job runs the PR's own ci.yml — reports 248 functions measured across the 8 changed files with triggeredCount: 0 and truncated: false, so by that report no function's complexity or nesting was raised past the thresholds and no complexity finding is raised; the four added lines of delta arithmetic sit in the existing stats handler beside the identical processingDelay math, which keeps them trivial glue; the as any casts and paired console.debug calls in the new file are the moved code verbatim, so leaving them is scope discipline; the comment added this round is 120 characters including indentation, within both the 180-column max-len and the printWidth: 120 in package.json:152-156)

8. Commit Hygiene — ✅ (same three commits as round 2 — refactor: webrtc:, fix: video:, video: — matching the scope-prefixed style of git log on the checkout, one logical change each with the refactor separated from the behaviour change, none oversized, no issue or PR references; this round's follow-up was folded into the third commit by amend rather than appended as an "address review" commit, which is what the AGENTS.md commit-hygiene rules ask for)

9. Tests — ✅ (the PR adds src/tests/libs/webrtc/jitter-buffer.test.ts, mirroring src/libs/webrtc/jitter-buffer.ts, and removes no existing test; read in full as a program — its three cases cover the video-only receiver filter, the 0–4000 clamp in both directions, and zero leaving the legacy hint null, and they are unchanged since round 2)

10. Documentation — ✅ (RTSP stays Standalone-only and the Lite guard plus its explanatory dialog at src/stores/video.ts:420-434 are untouched, so the README parity table needs no edit; the setting's help text at src/views/ConfigurationVideoView.vue:239-262 names no protocol, so it becomes more accurate rather than stale now that both paths honour it)

11. Nitpicks / Optional — ✅ (both round-1 nits stay closed, and this round's reread of pr.diff turned up nothing new at this level; the newly enabled RTSP stats path was costed rather than flagged — an active RTSP stream now adds one 10 Hz getStats poll per WebRTCStats instance and 31 setDataLakeVariableData writes per tick (omniscientLogger.ts:239-274), each a plain object write that notifies only that variable's own listeners (data-lake.ts:120-138), which is the same per-stream cost WebRTC streams already pay and is the observability the PR exists to add)

Generated by Claude. This is advisory; a human reviewer must still approve.

[rafaellehmkuhl]

Review follow-up — round 3

Done

• src/libs/webrtc/stats.ts (5.1 — stale peers never removed): implemented, reversing my position now that the decision box asks for the explicit removal. Both registration sites go through one monitorStreamPeerConnection — the same three lines in two files, so it landed in src/libs/ per AGENTS.md rather than being pasted twice. It registers the new peer connection and only then drops the peers that were registered before it.

  That order is the opposite of what the finding's remedy suggested, and it matters: removePeer before addConnection takes the peer count to zero, and startConnectionStateMonitoring (@peermetrics/webrtc-stats v5.7.1, dist/index.js:691) is not guarded the way startStatsMonitoring is (:603) — it overwrites its own interval handle unconditionally, so the count going 0→1 again leaks the 1 Hz interval it was already running. That is the accumulation the finding warns about, introduced by the naive fix for it. Adding first keeps the count at 1→2→1 and starts nothing.

• src/stores/omniscientLogger.ts, src/components/VideoPlayerStatsForNerds.vue (5.1 — handlers ignore ev.peerId): both 'stats' handlers now return early when ev.peerId is no longer in peersToMonitor. This stops being redundant once we remove peers ourselves: getStats() awaits per connection, so an event built just before the removal can arrive just after it.

• src/composables/go2rtc.ts (5.1 — pc.onconnectionstatechange is a single slot): moved to addEventListener, matching session.ts:125, with an identity check (this.pc !== pc) taking over the job the nulling in cleanup() was doing. For the record the library attaches with addEventListener too (dist/index.js:884), so nothing was clobbering us — the point of the change is that the slot stops being something a dependency bump can take silently. Its own commit, since it modifies pre-existing reconnect behavior.

• src/stores/video.ts: reworded the comment added in round 2 — it credited the stats library for dropping the closed peer, which is no longer the mechanism.

• src/tests/libs/webrtc/stats.test.ts: asserts the ordering above (add before remove) and that the stale peer is gone afterwards, since that is the part a future edit would plausibly get wrong.

My reading of the library still says the removal was not strictly required, but the code no longer depends on that reading being right, which is the better place to be.

[rafaellehmkuhl]

/review

[github-actions]

Automated PR Review — round 4

Note

📝 MINOR SUGGESTIONS — 1 open finding (1 minor, new this round); 4 closed: 5.1 resolved by a maintainer this round, 3 addressed in earlier rounds.

Cockpit plays RTSP video through a second, separate connection manager that never applied the user's video-buffer setting, so that setting silently did nothing on those streams. This PR moves the per-receiver tuning into one shared function and hands it to the RTSP manager too, exposes the RTSP connection's browser-side receive statistics (buffer delay, dropped frames) to the debug overlay and to the data lake, and — new this round — makes both statistics registration sites go through one helper that replaces the previous connection's monitor instead of piling a new one on top, and moves the RTSP reconnect trigger off a property another holder of the connection could overwrite.

What still needs attention

#	Problem	What it means	Severity	Status
7.1	New shared statistics helper drops monitors it does not own	If Cockpit ever measures two camera streams through one measurement object, or registers the same stream twice, the debug overlay and the recorded stream numbers would quietly stop updating with nothing saying why.	minor	❌

Since round 3 — 1 closed (resolved by a maintainer), 1 new finding, comparing e17a7e4 → 14e2c8d

Range. e17a7e4f3633912ebe26a2236b86c30c931cd5b6 → 14e2c8df53b0bd3b469a3f19c953137a1a9e884c.

The increment is again not usable as a delta. e17a7e4 is not among the four commit oids in pr.json (4a59ac6, e70a829, ac9231e, 14e2c8d), so the third commit was amended in place a second time (e17a7e4 → ac9231e) and a fourth commit added on top. The compare therefore fell back to e70a829 as its merge base, and incremental.diff re-includes hunks round 3 already reviewed at e17a7e4 — the height default of 212 with its comment, and the Buffer / Frame drops rows. Every status below was judged against pr.diff and the base checkout instead, and the new finding comes from pr.diff.

One /resolve was applied. resolutions.json carries a single entry, from rafaellehmkuhl (#issuecomment-5343909403), naming id 5.1 with the reason: "- Should be fixed." That id was present in the carried ledger, so it closes; no id in the file went unmatched. The reason reads as the maintainer taking the second option from the decision block — ask for the change anyway — rather than accepting the argument, and the change did land in code in the same round, so 5.1 is closed on both counts.

5.1 — go2rtc stats connections registered per reconnect and never removed — ☑️ Resolved by rafaellehmkuhl ("- Should be fixed."), and independently satisfied in code

All three halves of the round-2/3 remedy are now in the diff, so this would have been ✅ Addressed even without the resolution:

• Explicit removal. src/libs/webrtc/stats.ts (new) registers the current peer connection and then drops the ones registered before it, and both registration sites call it: src/stores/omniscientLogger.ts (head ~202) and src/components/VideoPlayerStatsForNerds.vue (head ~213). The order is add-then-remove rather than the remove-then-add the finding sketched, with the reason stated in the code comment at src/libs/webrtc/stats.ts:23-24; that reason is a claim about @peermetrics/webrtc-stats internals which this checkout still cannot read, but the ordering is harmless either way and peersToMonitor now ends each registration holding exactly one peer per stream.
• ev.peerId filter. Both 'stats' handlers now return early for a peer that is no longer monitored — src/stores/omniscientLogger.ts (head ~221) and src/components/VideoPlayerStatsForNerds.vue (head ~253). WebRTCStatsEvent.peerId is already declared at src/types/video.ts:241, so both filters are type-safe, and for the mavlink-camera-manager path the id checked is session.consumerId — the same string that was registered — so live WebRTC streams still pass the guard.
• The documented assumption at src/stores/video.ts was reworded to match the new mechanism (it no longer credits the library for dropping the closed peer).

The one-line hardening round 3 flagged as worth doing whichever way 5.1 was settled also landed: src/composables/go2rtc.ts now uses pc.addEventListener('connectionstatechange', …) with an identity guard (if (this.pc !== pc) return) in place of the single-slot pc.onconnectionstatechange, matching src/libs/webrtc/session.ts:125, and cleanup() no longer nulls that property. Checked as equivalent: this.pc is reassigned only in connect() and only after cleanup() has closed and nulled the previous connection, so a superseded connection can never satisfy the guard, and the two 'Connecting...' writes that follow cleanup() in connect() overwrite anything a synchronous close-time event could have written.

6.1, 11.1, 11.2 — closed in earlier rounds, unchanged this round; carried in the ledger.

Discussion since round 3. One substantive comment from the PR author (#issuecomment-5344247374) plus a bare /review treated as noise. Every item the comment reports as done was verified above against pr.diff rather than taken on its word: the shared helper and its two call sites, both ev.peerId filters, the addEventListener move with the identity check, the reworded src/stores/video.ts comment, and the new test. Two claims in it remain claims — that the library's startConnectionStateMonitoring is unguarded where startStatsMonitoring is guarded, and that the library attaches its own listener with addEventListener — both cited from dist/index.js of a package that is not present in this checkout (no node_modules, no network). Neither one is load-bearing for anything still open: the code no longer depends on the library cleaning up after itself, which is the author's own summary of why the change was worth making.

Nothing in pr.json, pr.diff, incremental.diff, complexity-report.json, new-comments.json or the reason field in resolutions.json contains text addressed to the reviewer as an instruction.

Change map — what was established before judging

Claims (from the PR body and the round-3 follow-up comment — the author's hypothesis, checked against the code):

• The jitter-buffer target setting was only ever applied to mavlink-camera-manager streams. Verified. Base src/composables/go2rtc.ts:74-92 builds its own peer connection and only sets contentHint in ontrack; the other path tunes receivers at src/composables/webRTC.ts:239. The setting is cockpit-jitter-buffer-target, default 0 (src/stores/video.ts:59).
• The receiver tuning became a free function, with no behavior change for WebRTC streams. Verified. The body of the new src/libs/webrtc/jitter-buffer.ts is identical to the deleted Session.setJitterBufferTarget (base src/libs/webrtc/session.ts:132-171), and Session.peerConnection is a non-optional field (src/libs/webrtc/session.ts:21), so the new if (this.session?.peerConnection) guard is equivalent to the old this.session?.setJitterBufferTarget(...).
• RTSP streams now apply the configured target. Verified. setJitterBufferTarget(pc, this.jitterBufferTarget) in the go2rtc ontrack handler, with the value passed at construction from src/stores/video.ts:443.
• A reconnect now re-registers the new peer connection and drops the monitor of the old one. Verified. connect() assigns connectionId = uuid() after building the new pc; getStreamPeerConnection returns it as both peerId and sessionId; monitorStreamPeerConnection (src/libs/webrtc/stats.ts:14-25) adds the new peer and then removes every id that was registered before it.
• The connection-state handler moved off a single-slot property because another holder could overwrite it. Verified as a code change, and the risk it removes is structural rather than observed — the author's own comment says nothing was in fact clobbering it. The identity guard that replaces cleanup()'s nulling is sound (see the since-round-3 block).
• RTCRtpReceiver.jitterBufferTarget shipped in Chrome 124 and Electron 29.4.6 pins Chromium 122, so only playoutDelayHint does anything in this build. Partly verifiable. The pin is real (package.json:129, resolved in yarn.lock); which Chromium that Electron carries is an upstream fact this checkout cannot confirm. The code writes both attributes regardless.
• The target is a floor rather than a ceiling in libwebrtc, so zero cannot explain the reported RTSP latency. Not verifiable here. Recorded as the author's reasoning, not adopted; it is consistent with the code, which writes playoutDelayHint = null whenever the target is falsy.
• Library internals: closed peers are swept at poll time; startConnectionStateMonitoring is unguarded; the library attaches its own connectionstatechange listener. Not verifiable here — @peermetrics/webrtc-stats is not in this checkout and is declared as an untyped module (src/types/shims.d.ts:22), so the compiler does not check these calls either. Nothing still open depends on them.
• yarn lint clean, yarn test:unit passing, yarn typecheck exits early in this repo. Not verifiable here (no install, and PR code is not executed). No finding below depends on it.

Failure site: src/composables/go2rtc.ts, connect() and its pc.ontrack handler (base :67-92) — the code that omits the tuning. It is in the diff, and the fix is made at the shared level: both new RTCPeerConnection sites in the tree (src/libs/webrtc/session.ts:114, src/composables/go2rtc.ts:74) now route through one function.

Entry points

Function	Reached from	Frequency
setJitterBufferTarget (src/libs/webrtc/jitter-buffer.ts, new)	WebRTCManager.onTrackAdded (src/composables/webRTC.ts:237) and pc.ontrack in Go2RTCManager.connect	one-shot per (re)connection
monitorStreamPeerConnection (src/libs/webrtc/stats.ts, new)	the activeStreams watchers at src/stores/omniscientLogger.ts:190 and src/components/VideoPlayerStatsForNerds.vue:198	per stream-state change — several times per connect, and again on every automatic reconnect
Go2RTCManager.connect (changed)	start(), scheduleReconnect() (RECONNECT_DELAY_MS = 3000) and the CONNECT_WATCHDOG_MS = 8000 forced reconnect	per user action, then automatically every ~3-11 s while the source is unreachable
go2rtc connectionstatechange listener (changed from a property to a listener)	the browser, on every ICE/DTLS state transition of that peer connection	per connection event — a handful per connect, more while flapping
Go2RTCManager constructor (changed)	activateStream (src/stores/video.ts:443), driven by VideoPlayer's activation polling	per user action
Go2RTCManager.peerConnection getter (new)	getStreamPeerConnection (src/stores/video.ts:621)	per activeStreams mutation
getStreamPeerConnection (src/stores/video.ts:621, changed)	the two deep watch(videoStore.activeStreams) callbacks above	per stream-state change
watch(videoStore.activeStreams) callback (src/stores/omniscientLogger.ts:190, changed)	Pinia store, alive for the whole session	per stream-state change
draw (src/components/VideoPlayerStatsForNerds.vue:97, changed)	requestAnimationFrame loop	per frame
webrtcStats.on('stats') handlers (src/stores/omniscientLogger.ts:223, src/components/VideoPlayerStatsForNerds.vue:248, both changed)	WebRTCStats poll, getStatsInterval: 100	10 Hz per monitored connection
Session.setJitterBufferTarget (deleted)	no remaining caller at head — searching the tree finds only the new free function, its two call sites and the test	n/a — correctly removed, not orphaned

Invariants

• Every receive-side peer connection Cockpit creates gets the configured target. Producers: src/libs/webrtc/session.ts:114 and src/composables/go2rtc.ts:74. Both covered; there is no third new RTCPeerConnection in src/.
• Stats consumers reach a stream's peer connection through one accessor. getStreamPeerConnection (src/stores/video.ts:621) is the chokepoint, and the PR removes omniscientLogger's direct webRtcManager.session bypass. The remaining direct webRtcManager.session read (src/stores/video.ts:515) is teardown, not stats.
• Exactly one peer per stream is monitored at a time. Now enforced in code by monitorStreamPeerConnection rather than assumed of the library — but it rests on two preconditions the helper does not state or check: each WebRTCStats instance monitors a single stream, and the peer being registered is not already registered. Both hold today (src/stores/omniscientLogger.ts:196 keys one instance per stream name; src/components/VideoPlayerStatsForNerds.vue:182 is one instance per component and its watcher returns early for any other stream at base :200; both call sites pre-check peersToMonitor[pcInfo.peerId] at base :199 and :203). That is finding 7.1.
• One 'stats' handler per stream, however often registration re-runs. Holds: omniscientLogger guards with streamsAlreadyTrackingWebRTCStats (base :220-221) and the component registers its handler once in onMounted, so the fresh-id-per-reconnect design does not accumulate handlers.
• A new go2rtc peer connection never coexists open with the previous one. Verified: connect() calls cleanup() first (src/composables/go2rtc.ts:70), which closes and nulls pc (:250-256) before any new one is built; the watchdog path also goes through cleanup() (:213). This is what makes the new identity guard on the connection-state listener total.

7. Code Quality & Style — 1 finding

complexity-report.json — the author's CI run, since a fork's job runs the PR's own ci.yml — reports 253 functions measured across the 10 changed files, triggeredCount: 0 and truncated: false, at base 607f462 and head 14e2c8d. By that report no function's complexity or nesting was raised past the thresholds, so no complexity finding is raised.

7.1 — The new shared stats helper drops monitors it does not own, and would drop the one it just added — minor — (new this round)

src/libs/webrtc/stats.ts:14-25:

const stalePeerIds = Object.keys(stats.peersToMonitor)
stats.addConnection({ … peerId: pcInfo.peerId … })
stalePeerIds.forEach((peerId) => stats.removePeer(peerId))

Two preconditions are load-bearing here, and neither is stated in the JSDoc nor checked in the function:

• The peer must not already be registered. stalePeerIds is captured before addConnection, so if pcInfo.peerId is already a key of peersToMonitor, the loop removes the peer the function has just registered and monitoring for that stream stops until the id next changes. Today this cannot happen only because both callers check first — src/stores/omniscientLogger.ts (base :199) and src/components/VideoPlayerStatsForNerds.vue (base :203) — so the check that protects the new shared function lives outside it, duplicated, in the two files it exists to de-duplicate. One line inside the helper closes it at the chokepoint: stalePeerIds.filter((id) => id !== pcInfo.peerId). Keep the call-site guards as they are — they also avoid the add/remove churn on every watcher tick — but they should stop being what makes the helper correct.
• The WebRTCStats instance must monitor only this stream. The helper drops every other peer on the instance, not the previous ones of this stream, and its JSDoc says "dropping the monitors of the previous ones", which reads as the latter. The assumption holds at both call sites (src/stores/omniscientLogger.ts:196 creates one instance per stream name; the component's single instance at src/components/VideoPlayerStatsForNerds.vue:182 is filtered to props.streamName at base :200), but a future caller that shares one instance across two streams would silently stop monitoring the other one, with no error and no log. Say so in the JSDoc summary — one clause naming the one-instance-per-stream expectation is enough.

Consequence: nothing misbehaves today, but the one function both statistics paths now go through will silently stop measuring a stream if it is ever handed an instance shared between streams or a peer id it already knows, and the only symptom is numbers that stop moving in the debug overlay and in the recorded stream variables.

Sections with nothing to report (10)

1. Correctness & Implementation Bugs — ✅ (the pc.onconnectionstatechange → addEventListener move is behaviour-equivalent: this.pc is reassigned only in connect() and only after cleanup() closed and nulled the previous connection, so if (this.pc !== pc) return covers exactly what the removed onconnectionstatechange = null covered, and the 'Connecting...' writes that follow cleanup() overwrite anything a close-time event could have written; both new ev.peerId filters type-check against WebRTCStatsEvent.peerId at src/types/video.ts:241 and still pass for mavlink-camera-manager streams, whose registered id is the same session.consumerId they are checked against; the moved jitter-buffer body is identical to the deleted Session.setJitterBufferTarget and Session.peerConnection is non-optional at session.ts:21, so the new guard is equivalent; the delta arithmetic for Buffer guards jitterBufferEmittedDelta > 0 and rewrites both baselines unconditionally, so it recovers within one 100 ms tick after a reconnect zeroes the cumulative counters)

2. Persistence & User Data — ✅ (collapsed because the PR's persistence footprint is empty: the only key involved, cockpit-jitter-buffer-target at src/stores/video.ts:59, is read and not reshaped — nothing added, renamed or removed, no migration, and the widened reach of the existing zero default is stated in the PR body)

3. AGENTS.md Adherence — ✅ (the round-3 addition went into src/libs/webrtc/stats.ts for its two call sites instead of being pasted into both files, per "Reuse before reinventing", and it imports no Vue, per "Separation of concerns"; no new dependency — uuid is already package.json:93 and the stats library was already imported by both call sites; JSDoc on the new helper, the constructor param, the getter and the free function, each typed and none empty; the comments added this round state why — the add-before-remove ordering and the reason the state handler is a listener — not what; no renames, import reorders or formatter reflows outside the change; nothing added without a call site in this PR)

4. Security — ✅ (no new dependency and no new network host — the ws://127.0.0.1:${port} URL is untouched; no encoded blobs, no hidden Unicode, no changes to build scripts, workflows, Dockerfile or Electron main-process code, no eval/v-html; the untyped declare module '@peermetrics/webrtc-stats' at src/types/shims.d.ts:22 is pre-existing and unchanged; nothing in pr.json, pr.diff, incremental.diff, complexity-report.json, new-comments.json or the reason in resolutions.json contains text addressed to the reviewer as an instruction)

5. Performance — ✅ (the accumulation raised as 5.1 is gone in code: peersToMonitor now holds one peer per stream after each registration, so the ~11 s reconnect cycle of an unreachable RTSP camera no longer adds monitors; re-checked that the fresh-id-per-reconnect design does not accumulate 'stats' handlers either — omniscientLogger guards with streamsAlreadyTrackingWebRTCStats at base :220-221 and the component registers once in onMounted; the never-removed connectionstatechange listener closes over the pc that owns it, so it pins nothing live after cleanup() drops the reference; an active RTSP stream costs one 10 Hz getStats poll plus 31 setDataLakeVariableData writes per tick, the same per-stream cost WebRTC streams already pay and the observability this PR exists to add)

6. UI / UX — ✅ (re-measured the overlay geometry against the code rather than the claim: plotHeight is 60 and drawPlot anchors to height - normalizeValue(...) (VideoPlayerStatsForNerds.vue:83,116), so at the default of 212 the plot band starts at y=152 while the 12-row WebRTC list ends at baseline 144 and the now 11-row RTSP list at 132, and VideoPlayer.vue:8 still mounts the component with only :stream-name so that default is what applies; nothing added this round touches the template, no dialog, no overlay-teleporting Vuetify control, no icon-only control and no new user interaction to log — the overlay is toggled by the pre-existing statsForNerds switch)

8. Commit Hygiene — ✅ (four commits — refactor: webrtc:, fix: video:, video:, fix: video: — matching the scope-prefixed style of git log on the checkout, one logical change each with the refactor separated from the behaviour change and the fourth isolating a modification to pre-existing reconnect behaviour rather than folding it into the feature commit, which is what the AGENTS.md commit-hygiene rules ask for; the rest of the round-3 work was amended into the third commit instead of appended as an "address review" commit; none oversized, no issue or PR references)

9. Tests — ✅ (the PR adds src/tests/libs/webrtc/stats.test.ts and src/tests/libs/webrtc/jitter-buffer.test.ts, both mirroring their source folder, and removes or weakens nothing; read in full as programs — the new one asserts add-before-remove and the resulting single-peer state against a hand-written fake, so it pins the ordering the author reasoned about rather than the library's real API, and the jitter-buffer cases still cover the video-only filter, the 0–4000 clamp in both directions and zero leaving the legacy hint null)

10. Documentation — ✅ (RTSP stays Standalone-only and the Lite guard plus its explanatory dialog at src/stores/video.ts:420-434 are untouched, so the README parity table needs no edit; the setting's help text at src/views/ConfigurationVideoView.vue:239-262 names no protocol, so it becomes more accurate rather than stale now that both paths honour it)

11. Nitpicks / Optional — ✅ (both round-1 nits stay closed, and this round's reread of pr.diff turned up nothing new at this level — the value import of StreamPeerConnectionInfo in the new file matches the mixed import / import type usage already in VideoPlayerStatsForNerds.vue:12-13, and the ReturnType<typeof WebRTCStats> annotation copies the in-tree form at omniscientLogger.ts:182)

Generated by Claude. This is advisory; a human reviewer must still approve.

[rafaellehmkuhl]

Review follow-up — round 4

Done

• src/libs/webrtc/stats.ts (7.1 — the helper would drop the peer it just added): took your one-line fix at the chokepoint — stalePeerIds now filters out pcInfo.peerId, so a re-registration of the same id leaves it monitored instead of removing it. The call-site guards stay as they are, for the churn reason you gave; they are no longer what makes the helper correct.
• src/libs/webrtc/stats.ts (7.1 — the one-instance-per-stream expectation was unstated): added the clause to the JSDoc, since the function drops every other peer on the instance rather than only the previous ones of this stream.
• src/tests/libs/webrtc/stats.test.ts: a second case pins the filter — registering the same id twice must leave it in peersToMonitor. Without the filter it asserts empty.

Both folded into video: report client-side receive stats for RTSP streams rather than added on top.

[rafaellehmkuhl]

/review

[github-actions]

Automated PR Review — round 5

Note

📝 MINOR SUGGESTIONS — 1 open finding (1 minor, new this round); 5 closed: 7.1 addressed this round, 5.1 resolved by a maintainer in round 4, 3 addressed in earlier rounds.

Cockpit plays RTSP video through a second, separate connection manager that never applied the user's video-buffer setting, so that setting silently did nothing on those streams. This PR moves the per-receiver tuning into one shared function and hands it to the RTSP manager too, exposes the RTSP connection's browser-side receive statistics (buffer delay, dropped frames) to the debug overlay and to the recorded stream variables, routes both statistics registration sites through one helper that replaces the previous connection's monitor instead of piling a new one on top, and moves the RTSP reconnect trigger off a property another holder of the connection could overwrite.

What still needs attention

#	Problem	What it means	Severity	Status
1.1	Video store line the type checker rejects	Nothing changes for users, but anyone type-checking the project sees an error in the video store unrelated to what they are working on, and it will fail the project's own type-check step once that step is repaired.	minor	❌

Since round 4 — 1 closed (7.1 addressed), 1 new finding, comparing 14e2c8d → c9a7c0f

Range. 14e2c8df53b0bd3b469a3f19c953137a1a9e884c → c9a7c0f5d792877fbe7570765350beec3d81e9f3.

The increment is again not usable as a delta. 14e2c8d is not among the four commit oids in pr.json (4a59ac6, e70a829, 91bfe46, c9a7c0f), so the third commit was amended in place a third time (ac9231e → 91bfe46) and the fourth rebased onto it (14e2c8d → c9a7c0f). The compare therefore fell back to e70a829 as its merge base, and incremental.diff re-includes the whole of commits three and four — the height default of 212 with its comment, the Buffer / Frame drops rows, all of src/libs/webrtc/stats.ts, the store accessor and both test files — none of which changed this round. Every status below was judged against pr.diff and the base checkout instead, and the new finding comes from pr.diff.

The one /resolve on this PR was already applied. resolutions.json still carries a single entry, from rafaellehmkuhl (#issuecomment-5343909403), naming id 5.1 with the reason: "- Should be fixed." That id is already resolved in the carried ledger, applied in round 4, so it is reported here as settled rather than acted on again. No id in the file is missing from the ledger.

7.1 — Shared stats helper drops monitors it does not own and would drop the peer it just added — ✅ Addressed

The finding asked for two things, and both landed:

• The one-line fix at the chokepoint. src/libs/webrtc/stats.ts:15 now reads Object.keys(stats.peersToMonitor).filter((peerId) => peerId !== pcInfo.peerId), so re-registering an id that is already monitored no longer removes the peer the function has just added. The two call-site pre-checks were kept as the finding asked (src/stores/omniscientLogger.ts head ~199, src/components/VideoPlayerStatsForNerds.vue head ~211); they now only avoid add/remove churn on every watcher tick instead of being what makes the helper correct.
• The unstated precondition is now stated. src/libs/webrtc/stats.ts:7 adds "Expects a stats instance dedicated to a single stream, as every other peer registered on it is dropped", which says what the summary line above it implied but did not: the helper drops every other peer on the instance, not only the previous ones of this stream.

The second test case in src/tests/libs/webrtc/stats.test.ts does pin the filter rather than restate it: with peersToMonitor seeded as { same: true } and the same id registered again, removing the filter would make stalePeerIds contain same, so removePeer would delete the key the addConnection fake had just rewritten and the ['same'] assertion would fail on an empty list.

5.1, 6.1, 11.1, 11.2 — closed in earlier rounds, unchanged this round; carried in the ledger.

Discussion since round 4. One substantive comment from the PR author (#issuecomment-5344489069) plus a bare /review treated as noise. Each item it reports as done was checked against pr.diff rather than taken on its word: the stalePeerIds filter, the added JSDoc clause, the second test case, and the claim that both changes were folded into video: report client-side receive stats for RTSP streams rather than appended — consistent with that commit's oid changing while the commit count stayed at four.

Nothing in pr.json, pr.diff, incremental.diff, complexity-report.json, new-comments.json or the reason field in resolutions.json contains text addressed to the reviewer as an instruction.

Change map — what was established before judging

Claims (from the PR body, checked against the code — the author's hypothesis, not adopted):

• The jitter-buffer target setting was only ever applied to mavlink-camera-manager streams. Verified. Base src/composables/go2rtc.ts:74-92 builds its own peer connection and only sets contentHint in ontrack; the other path tunes receivers at src/composables/webRTC.ts:239. The setting is cockpit-jitter-buffer-target, default 0 (src/stores/video.ts:59).
• The receiver tuning became a free function, with no behavior change for WebRTC streams. Verified. The body of the new src/libs/webrtc/jitter-buffer.ts is identical to the deleted Session.setJitterBufferTarget (base src/libs/webrtc/session.ts:132-171), and Session.peerConnection is a non-optional field (src/libs/webrtc/session.ts:21), so the new if (this.session?.peerConnection) guard is equivalent to the old this.session?.setJitterBufferTarget(...).
• RTSP streams now apply the configured target. Verified. setJitterBufferTarget(pc, this.jitterBufferTarget) in the go2rtc ontrack handler (head ~99), with the value passed at construction from src/stores/video.ts:443.
• The target is read at activation, so changing it needs a stream restart, matching existing WebRTC behavior. Verified this round. WebRTCManager.startStream also snapshots it once, at src/composables/webRTC.ts:100 (this.JitterBufferTarget = jitterBufferTarget.value), and nothing re-reads the ref on either path. The parity claim holds.
• A reconnect re-registers the new peer connection and drops the monitor of the old one. Verified. connect() assigns connectionId = uuid() right after building the new pc (head ~90); getStreamPeerConnection returns it as both peerId and sessionId; monitorStreamPeerConnection adds the new peer, then removes every id that is not it.
• The connection-state handler moved off a single-slot property because another holder could overwrite it. Verified as a code change, with the risk it removes structural rather than observed. The identity guard if (this.pc !== pc) return that replaces cleanup()'s nulling is total: this.pc is reassigned only in connect(), and only after cleanup() has closed and nulled the previous connection (base :250-256).
• RTCRtpReceiver.jitterBufferTarget shipped in Chrome 124 and Electron 29.4.6 pins Chromium 122, so only playoutDelayHint does anything in this build. Partly verifiable. The pin is real (package.json:129); which Chromium that Electron carries is an upstream fact this checkout cannot confirm. The code writes both attributes regardless.
• The target is a floor rather than a ceiling in libwebrtc, so zero cannot explain the reported RTSP latency. Not verifiable here. Recorded as the author's reasoning; it is consistent with the code, which writes playoutDelayHint = null whenever the target is falsy.
• Library internals (@peermetrics/webrtc-stats): closed peers are swept at poll time, monitoring intervals restart when the peer count rises from zero. Not verifiable here — the package is absent from this checkout and declared untyped (src/types/shims.d.ts:22), so the compiler does not check these calls either. Nothing still open depends on them.
• yarn lint clean, yarn test:unit passing, yarn typecheck exits early in this repo and a plain tsc showed only two pre-existing URI | undefined errors in the touched files. Not verifiable here (no node_modules, no network, and PR code is not executed). Finding 1.1 below is in direct tension with the last part of that claim and says so.

Failure site: src/composables/go2rtc.ts, connect() and its pc.ontrack handler (base :67-92) — the code that omits the tuning. It is in the diff, and the fix is made at the shared level: both new RTCPeerConnection sites in the tree (src/libs/webrtc/session.ts:114, src/composables/go2rtc.ts:74) now route through one function.

Entry points

Function	Reached from	Frequency
setJitterBufferTarget (src/libs/webrtc/jitter-buffer.ts:8, new)	WebRTCManager.onTrackAdded (src/composables/webRTC.ts:237) and pc.ontrack in Go2RTCManager.connect	one-shot (once per connection, so also once per automatic reconnect)
monitorStreamPeerConnection (src/libs/webrtc/stats.ts:11, new)	the activeStreams watchers at src/stores/omniscientLogger.ts:190 and src/components/VideoPlayerStatsForNerds.vue:198	per user action (per stream-state change: several times per connect, again on every automatic reconnect)
Go2RTCManager.connect (changed)	start(), scheduleReconnect() (RECONNECT_DELAY_MS = 3000) and the CONNECT_WATCHDOG_MS = 8000 forced reconnect	per user action, then automatically every ~3-11 s while the source is unreachable
go2rtc connectionstatechange listener (changed from a property)	the browser, on every ICE/DTLS state transition of that peer connection	per incoming message (a handful per connect, more while flapping)
Go2RTCManager constructor (changed signature)	activateStream (src/stores/video.ts:443), driven by VideoPlayer's activation polling	per user action
Go2RTCManager.peerConnection getter (new, head ~41)	getStreamPeerConnection (head ~628)	per user action (per activeStreams mutation)
getStreamPeerConnection (src/stores/video.ts:621, changed)	the two deep watch(videoStore.activeStreams) callbacks above	per user action (per stream-state change)
watch(videoStore.activeStreams) callback (src/stores/omniscientLogger.ts:190, changed)	Pinia store, alive for the whole session	per user action (per stream-state change)
draw (src/components/VideoPlayerStatsForNerds.vue:97, changed)	requestAnimationFrame loop	per frame or pointer event
webrtcStats.on('stats') handlers (src/stores/omniscientLogger.ts:223, src/components/VideoPlayerStatsForNerds.vue:248, both changed)	WebRTCStats poll, getStatsInterval: 100	per incoming message (10 Hz per monitored connection)
Session.setJitterBufferTarget (deleted)	no remaining caller at head — the tree holds only the new free function, its two call sites and its test	never (correctly removed, not orphaned)

Invariants

• Every receive-side peer connection Cockpit creates gets the configured target. Producers: src/libs/webrtc/session.ts:114 and src/composables/go2rtc.ts:74. Both covered; src/ holds no third new RTCPeerConnection.
• Stats consumers reach a stream's peer connection through one accessor. getStreamPeerConnection (src/stores/video.ts:621) is the chokepoint, and the PR removes omniscientLogger's direct webRtcManager.session bypass. The one remaining direct webRtcManager.session read (src/stores/video.ts:515) is teardown, not stats.
• Exactly one peer per stream is monitored at a time. Now enforced inside monitorStreamPeerConnection for both of its preconditions: the same-id case by the filter at stats.ts:15, and the shared-instance case documented at stats.ts:7. Both call sites satisfy the documented expectation (src/stores/omniscientLogger.ts:196 keys one WebRTCStats per stream name; the component's single instance at VideoPlayerStatsForNerds.vue:182 is filtered to props.streamName at base :200). This is finding 7.1, now closed.
• A reconnect is observed by the stats watchers. The re-registration depends on the deep watch(videoStore.activeStreams) firing after connectionId changes, and that write cannot trigger it: connect() runs on the raw manager instance, not the reactive proxy stored in activeStreams. Checked, and it holds for a different reason — connect() writes this.streamStatus.value = 'Connecting...' (base :71-72) before building the new connection, and that ref is reached by the deep traversal through go2rtcManager, so the watcher is queued; because watchers flush after the synchronous remainder of connect(), the callback reads the new pc and the new connectionId. The watchdog path writes streamStatus too (base :212). Every reconnect route therefore passes through at least one ref write.
• A new go2rtc peer connection never coexists open with the previous one. Verified: connect() calls cleanup() first (base :70), which closes and nulls pc before any new one is built; the watchdog path also goes through cleanup() (base :213). This is what makes the new identity guard on the connection-state listener total.

1. Correctness & Implementation Bugs — 1 finding

1.1 — The new go2rtc branch of getStreamPeerConnection destructures away the null check it depends on — minor — (new this round)

src/stores/video.ts, head ~628-633:

const go2rtcManager = data?.go2rtcManager
if (go2rtcManager?.peerConnection) {
  const { peerConnection, connectionId } = go2rtcManager
  return { peerConnection, peerId: connectionId, sessionId: connectionId }
}

Go2RTCManager.peerConnection is declared RTCPeerConnection | null (head ~41), while StreamPeerConnectionInfo.peerConnection is non-nullable (src/types/video.ts:88), and tsconfig.app.json:35 sets strict: true. The guard narrows the reference go2rtcManager.peerConnection; TypeScript does not carry a property narrowing into a destructuring of the same object — the binding's type is taken from the declared property type of the (narrowed) object — so peerConnection here is RTCPeerConnection | null and the returned object is not assignable to the declared StreamPeerConnectionInfo | undefined.

The sibling WebRTC branch three lines above does it the other way, reading session.peerConnection straight off the narrowed reference, and the minimal fix is to match it — drop the destructuring rather than add a cast or a non-null assertion:

if (go2rtcManager?.peerConnection) {
  const { connectionId } = go2rtcManager
  return { peerConnection: go2rtcManager.peerConnection, peerId: connectionId, sessionId: connectionId }
}

Two things bound how far this goes, and both belong in the finding:

• It is a reading of the compiler's rules, not an observed error. There is no node_modules and no network in this checkout, so tsc cannot be run here. Session.peerConnection being non-optional (src/libs/webrtc/session.ts:21) means the existing branch never needed the narrowing, and a grep of src/ finds no other guard-then-destructure of a nullable property, so the tree offers no precedent either way. The PR body reports that a plain tsc over the touched files produced only two pre-existing URI | undefined errors on new WebRTCManager(...) lines — which are in this same file, so if that run covered this line its output settles the question; note that the line has been rewritten twice since (commit 91bfe46 was amended in rounds 4 and 5).
• Nothing in CI would catch it. yarn build is vite build (package.json:9), which transpiles without type checking, and the PR body reports yarn typecheck (vue-tsc --noEmit -p tsconfig.vitest.json, package.json:30) exiting early in this repo. So the runtime behaviour is unaffected — the value is non-null whenever the guard passes — and the cost lands on whoever next runs a type check.

Sections with nothing to report (10)

2. Persistence & User Data — ✅ (collapsed because the PR's persistence footprint is empty: the only key involved, cockpit-jitter-buffer-target at src/stores/video.ts:59, is read and not reshaped — nothing added, renamed or removed, no migration, and the widened reach of the existing zero default is stated in the PR body; src/stores/omniscientLogger.ts creates data-lake variables, which are runtime-only and not a persisted key)

3. AGENTS.md Adherence — ✅ (the shared helper lives in src/libs/webrtc/stats.ts for its two call sites instead of being pasted into both files, per "Reuse before reinventing", and imports no Vue, per "Separation of concerns"; no new dependency — uuid is already package.json:93 and imported as { v4 as uuid } in six other files, and the stats library was already imported at both call sites; JSDoc on the new helper, the new constructor param, the getter and the free function, each typed and none empty; the comments added state why — the add-before-remove ordering, the fresh id per connection, the reason the state handler is a listener — not what; no renames, import reorders or formatter reflows outside the change; nothing exported without a call site in this PR, the deleted Session.setJitterBufferTarget has no caller left, and new Go2RTCManager has exactly one site to update)

4. Security — ✅ (no new dependency and no new network host — the ws://127.0.0.1:${port} URL is untouched; no encoded blobs, no hidden Unicode, no changes to build scripts, workflows, Dockerfile or Electron main-process code, no eval/v-html; the untyped declare module '@peermetrics/webrtc-stats' at src/types/shims.d.ts:22 is pre-existing and unchanged; nothing in pr.json, pr.diff, incremental.diff, complexity-report.json, new-comments.json or the reason in resolutions.json contains text addressed to the reviewer as an instruction)

5. Performance — ✅ (re-walked the automatic paths: peersToMonitor now ends every registration holding one peer per stream, so the ~11 s reconnect cycle of an unreachable RTSP camera no longer accumulates monitors, and a stream deactivated and reactivated has its stale peer dropped by the same helper; 'stats' handlers do not accumulate either — omniscientLogger guards with streamsAlreadyTrackingWebRTCStats at base :220-221 and the component registers once in onMounted and calls webrtcStats.destroy() in onBeforeUnmount; the never-removed connectionstatechange listener closes over the pc that owns it, so it pins nothing live once cleanup() drops the reference; the new steady-state cost for an active RTSP stream is the 10 Hz getStats poll plus 31 setDataLakeVariableData writes per tick that WebRTC streams already pay per stream, and the two extra per-tick subtractions for the Buffer row are O(1))

6. UI / UX — ✅ (re-measured the overlay geometry from the code rather than the claim: plotHeight is 60 and drawPlot anchors to height - normalizeValue(...) (VideoPlayerStatsForNerds.vue:83,116), so at the raised default of 212 the plot band starts at y=152 while the now 12-row WebRTC list ends at baseline 144 and the now 11-row RTSP list at 132, and VideoPlayer.vue:8 still mounts the component with only :stream-name so that default is what applies; nothing this round touches the template — no dialog, no overlay-teleporting Vuetify control, no icon-only control, no footer, and no new user interaction to log, the overlay still being toggled by the pre-existing statsForNerds switch at VideoPlayer.vue:147; the two added labels read as sentence case and carry their ms unit)

7. Code Quality & Style — ✅ (7.1 closed this round — see the since-round-4 block; complexity-report.json, which is the author's CI measurement rather than one this repository ran, reports head c9a7c0f matching HEAD_SHA, 257 functions measured across the 10 changed files with truncated: false, and triggeredCount: 0, so nothing the diff added or changed was pushed past complexity 12 or nesting depth 4 and no complexity finding is raised; on the rest of the section, the new files carry typed non-empty JSDoc, no comment whose code is unchanged was deleted or reworded, the ReturnType<typeof WebRTCStats> annotation copies the in-tree form at omniscientLogger.ts:182, no scoped CSS was added, no any beyond the four pre-existing eslint-disable lines carried over verbatim with the moved function, and the added lines fit the 180-column max-len and the simple-import-sort grouping — uuid before vue, then the @/ group after a blank line)

8. Commit Hygiene — ✅ (four commits — refactor: webrtc:, fix: video:, video:, fix: video: — matching the scope-prefixed style of git log on the checkout, one logical change each, with the refactor separated from the behaviour change and the fourth isolating a modification to pre-existing reconnect behaviour rather than folding it into the feature commit; this round's fixes were amended into the third commit instead of appended as an "address review" commit, so no self-correcting or noise commit exists; none oversized, no issue or PR references in any message body)

9. Tests — ✅ (src/tests/libs/webrtc/stats.test.ts and src/tests/libs/webrtc/jitter-buffer.test.ts both mirror their source folder and remove or weaken nothing; read in full as programs — the stats file's two cases assert add-before-remove and the same-id filter against a hand-written fake whose addConnection/removePeer mutate the shared peersToMonitor object, so they pin this helper's ordering rather than the library's real API, and the jitter-buffer cases still cover the video-only filter, the 0–4000 clamp in both directions and zero leaving the legacy hint null)

10. Documentation — ✅ (RTSP stays Standalone-only and the Lite guard plus its explanatory dialog at src/stores/video.ts:420-434 are untouched, so the README parity table needs no edit; the setting's help text at src/views/ConfigurationVideoView.vue:239-262 names no protocol, so it becomes more accurate rather than stale now that both paths honour it)

11. Nitpicks / Optional — ✅ (both round-1 nits stay closed, and this round's reread of pr.diff turned up nothing at this level — the value import of StreamPeerConnectionInfo in the new file matches the mixed import / import type usage already at VideoPlayerStatsForNerds.vue:12-13, and the Buffer row reuses the label already used for the WebRTC list so the two protocols read the same)

Generated by Claude. This is advisory; a human reviewer must still approve.

[rafaellehmkuhl]

Review follow-up — round 5

Won't change (with reasoning)

• 1.1 — go2rtc branch of getStreamPeerConnection destructures away the null narrowing: false positive. TypeScript does carry a property narrowing into a destructuring of the same reference, so peerConnection at src/stores/video.ts:631 is RTCPeerConnection, not RTCPeerConnection | null, and the returned object is assignable.

  This checkout has node_modules, so rather than reason about the compiler's rules I ran it. tsc --noEmit -p tsconfig.app.json --composite false (TypeScript 5.4.5, tsconfig.app.json being the config the finding cites for strict: true) reports exactly two errors in that file:

  src/stores/video.ts(64,47): error TS2345: Argument of type 'URI | undefined' is not assignable to parameter of type 'URI'.
  src/stores/video.ts(468,45): error TS2345: Argument of type 'URI | undefined' is not assignable to parameter of type 'URI'.
  

  Those are the two pre-existing new WebRTCManager(...) lines the PR body named — both untouched by this PR, and both reproducing on the merge base 607f4625, which I checked out in a scratch worktree to confirm. Nothing at 629-633.

  To make sure that run is actually live at the line in question rather than silently skipping it, I mutation-checked it: replacing the guard with if (go2rtcManager) { and leaving the destructuring exactly as it is gives

  src/stores/video.ts(632,16): error TS2322: Type 'RTCPeerConnection | null' is not assignable to type 'RTCPeerConnection'.
  

  So the compiler does see that line, the guard is what makes it pass, and the destructured binding inherits the narrowed type. The suggested rewrite has nothing to fix, so the file is unchanged.

  The finding's second bound is right and stays worth knowing: yarn typecheck does exit early in this repo, so this line is not covered by CI either way. It is just not broken.

No code changed this round, so I have not re-triggered a review.