video: stop dropping the first press of the record-all toggle

[rafaellehmkuhl]

Summary

The record-all toggle (the joystick record button) lost presses. Seven commits in src/stores/video.ts, one logical change each:

• video: let the record-all toggle follow the streams. isRecordingAllStreams was flipped by the record-all actions only, so a recording started or stopped from the recorder mini-widget, or a start the stream refused, left it pointing the wrong way, and the next press did the opposite of what the operator asked (e.g. "No streams available to be recorded." while a recording was running). The flag is gone; the toggle asks whether any stream is recording or waiting to resume (Video: Fix WebRTC stream never recovering from a lost connection #3060).
• video: report record-all success only once the recorders run. The batch announced "Started recording all N streams" for every stream it handed to startRecording, refused ones included. It now awaits the starts and names only the streams that are recording.
• video: wait for a connecting stream instead of refusing to record it. Right after switching cameras, or for a stream no widget shows, the press hit "Media stream not yet active. Wait a second and try again." — more likely with Video: Fix WebRTC stream never recovering from a lost connection #3060's stricter isStreamReadyToRecord (connected + first frame). A start now waits up to 10 s for the stream (waitForStreamReadyToRecord), showing "Recording of '' starts once its video arrives..." while it does, for the widget and the joystick alike. A stop during the wait cancels it (and counts as "about to record" for the toggle), a second start for the same stream is ignored, and the stream is kept open while the wait holds it and released once the wait ends without recording. Streams that time out together get one dialog and a named alert each.
• video: leave RTSP streams out of record-all on Lite. RTSP streams reach Lite through the synced correspondency list but can never start there, so with the wait they would hold the batch for 10 s only to fail.
• video: write a recording's last chunk before finalizing its file. Stopping a recorder hands over one last chunk and fires stop right after. The chunk handler awaits storing the raw chunk first, so stop had already finalized the FFmpeg process by the time the chunk reached the live processor, and it was rejected ("Live stream process … not found or already finalized"): up to the last second of every Standalone recording was missing from its video, and recordings stopped within a couple of seconds could be too short for a thumbnail. onstop now waits up to 5 s for the chunks still in flight before finalizing, so a stalled write costs those chunks rather than the finalization.
• video: say so when record-all finds every stream already recording. Start-all with everything already recording reported "No streams available to be recorded." as an error; it now says the streams are already being recorded.
• video: announce when a stream that was waited for starts recording. The wait announced itself but never its end; a start that had to wait now says "Video of '' arrived. Recording started."

Test plan

• Start a recording from the video recorder mini-widget, then press the joystick record (toggle all) button: the recording stops.

• Switch a video widget to another camera and press the joystick record button right away: a "Recording of '' starts once its video arrives..." snackbar shows, the recording starts as soon as the video arrives, on the first press, and "Video of '' arrived. Recording started." follows.

• Press record while a stream is connecting, then press again before it connects: nothing records afterwards, and a stream no widget shows is closed.

• With a stream that never connects: after ~10 s a dialog says a video stream did not start sending video (the alert names it), and the next press starts again rather than stopping.

• On Standalone, record for a few seconds and stop: no "Live stream process … not found or already finalized" errors in the log, and the clip has its thumbnail and its last second.

• Press start-all while every stream records: an info alert says they are already being recorded.

Checks

Driven in headless Chrome with a fake WebRTC stream (stubbed signalling, canvas captureStream) through the real toggle_recording_all_streams action callback:

Scenario	Before	After
Recording started from the widget, then toggle	still recording, "No streams available to be recorded."	stopped
Toggle while the stream connects, connected 1 s later	not recording, "not yet active" dialog, success alert anyway	recording, success alert after it started
Toggle while connecting, toggle again before it connects	—	nothing records, no success alert, stream released
Two streams never connect	—	one dialog, one named alert per stream
Toggle to stop while another listed stream is idle	—	the idle stream is not opened
Stream never connects	—	dialog after 10 s, stream released, next press records
Widget releases the stream mid-wait	—	stream kept open, records

• yarn lint and yarn typecheck clean.

Closes #2688
To be merged in favor of #2715

[github-actions]

⚠️ IMPORTANT FIXES REQUIRED (Automated PR Review — round 1)

7 open findings: 1 major and 6 minor.

The record-all toggle (the joystick record button) no longer keeps its own on/off flag. Each press now checks whether any stream is recording, is waiting to resume after an outage, or is waiting to start, and stops everything if so. A start on a stream that is still connecting now polls for up to ten seconds until video arrives, instead of refusing it. A stop pressed during that wait cancels the start. While it waits, the start keeps the stream from being closed, and it releases the stream again on timeout. The batch start shows a "Starting to record" snackbar when it has to wait. It counts a stream as started only once its recorder is actually attached. On Lite it skips RTSP streams. The diff also carries the 14 commits of the stacked #3060, which is reviewed on its own PR. The findings below are about the last commit.

What still needs attention

#	Problem	What it means	Severity	Status
7.1	Wait-for-video loop added inside an already large recording function	The recording start routine got noticeably harder to follow and change safely, and it handles the same "wait for the video" problem differently from the resume code next to it.	major	❌
1.1	Cancelling a waiting start never closes the stream	If the operator presses record and then cancels before the video arrives, a camera no widget is showing keeps streaming in the background until Cockpit is restarted.	minor	❌
1.2	Checking the toggle state opens streams	Every joystick press opens every available camera stream just to check whether it is recording. On the web version that includes the RTSP cameras the PR meant to skip, which pops the "RTSP not supported" dialog.	minor	❌
6.1	Several timed-out starts replace each other's dialogs	When several cameras fail to start in one press, the operator only reads the error for the last one.	minor	❌
6.2	Recorder widget shows nothing while a start waits	Clicking record on the widget can do nothing visible for up to ten seconds, and clicking again is silently ignored, so it looks broken.	minor	❌
7.2	Two start checks can no longer fail	Two error messages in the start routine can never be shown any more, which leaves dead code that misleads readers.	minor	❌
8.1	Commits from the stacked PR are still in the branch	Merging as-is would bring in 14 commits that belong to another PR.	minor	❌

Change map — what was established before judging

• Claims
  • Symptom 1: the toggle does the opposite of what was asked after a recording was started or stopped from the widget — verified. Base src/stores/video.ts:1494-1499 reads only isRecordingAllStreams, which is written only at :1458 and :1477. The widget path (src/components/mini-widgets/MiniVideoRecorder.vue:340,366) never touches it.
  • Symptom 2: a stream that is still connecting refuses the start, yet the batch reports success and sets the flag — verified. Base video.ts:967-970 refuses the start. Base :1460-1464 pushes the stream into streamsThatStarted without waiting for the result, and :1458 sets the flag regardless.
  • Mechanism: the start waits up to 10 s, a stop cancels it, a second start is ignored, and the stream is held open while waiting and released on timeout — partly verified. The wait is at head video.ts:1173-1192, the ignored second start at :1174, the stop cancel at :1183, and the release on timeout at :1189. A stop during the wait does not release the stream (1.1).
  • Success is reported only once recorders run — verified (:1760-1764, read straight from activeStreams).
  • RTSP streams are left out on Lite — partly verified. The start batch filters them at :1725-1727, but the toggle's probe at :1793 and the stop loop at :1776 still call isRecording on them, and that activates them (1.2).
• Failure site: base src/stores/video.ts:1494-1499 (the toggle reads a flag) and :954-970 plus :1456-1472 (refused start counted as started). Both are in the diff.
• Entry points

Function	Reached from	Frequency
toggleRecordingAllStreams	registerActionCallback(toggle_recording_all_streams, useThrottleFn(…, 3000)), base video.ts:1626 (joystick button)	per user action
startRecordingAllStreams	the toggle above, and start_recording_all_streams action (base :1618)	per user action
stopRecordingAllStreams	the toggle, and stop_recording_all_streams action (base :1622)	per user action
isRecordingOrAboutTo	the toggle and stopRecordingAllStreams	per user action
startRecording	MiniVideoRecorder.vue:366 click; startRecordingAllStreams; recordAgain in the resume watcher/unmute listener (#3060)	per user action
stopRecording	MiniVideoRecorder.vue:340; stopRecordingAllStreams; the stream-config watcher (video.ts:~601)	per user action
deactivateStreamIfUnused	unregisterStreamConsumer (widget unmount/switch); recorder onstop; the start timeout at :1189	per user action

• Invariants
  • A stream that a start's wait activated is released when the wait ends. There are three ways a wait can end. The timeout releases the stream (:1189). The stop cancel returns at :1183 without releasing it. The stopRecording no-recorder branch (:1126-1135) releases only when a resume was waiting. So 1 of 2 cancel paths is covered (1.1).
  • Probing whether a stream records must not activate it. The PR states this itself at :1757-1759 and reads activeStreams directly for the success check. Other probes still go through isRecording → getStreamData → activateStream (base :735-740, :837-840): isRecordingOrAboutTo (:1714, reached from the toggle :1793 and stop-all :1776) and the batch loop at :1732. So 1 of 4 sites is covered. The chokepoint is isRecordingOrAboutTo (1.2).

1. Correctness & Implementation Bugs — 2 findings

1.1 minor — a stop during the wait leaves the stream it activated running.
Consequence: if the operator presses record on a camera no widget shows and then cancels before its video arrives, that camera keeps streaming over the tether until Cockpit is restarted.
The wait activates the stream through isStreamReadyToRecord → getStreamData (src/stores/video.ts:1173, base :735-740). deactivateStreamIfUnused then refuses to tear it down while the start is in recordingStartsWaitingForVideo (head :~727). When stopRecording deletes the entry (:~834), the loop exits and returns at :1183 with no release. The no-recorder branch of stopRecording (:1126-1135) calls deactivateStreamIfUnused only when wasWaitingToResume. The PR body says the stream is "released if the wait gives up", but that is only true for the timeout.
Fix: in stopRecording, record const wasWaitingToStart = recordingStartsWaitingForVideo.delete(streamName). Then call deactivateStreamIfUnused in the no-recorder branch for wasWaitingToResume || wasWaitingToStart. That is one place, and it also covers the PR's "widget releases the stream mid-wait, then the user cancels" case.

1.2 minor — the toggle's state probe activates every available stream, including RTSP on Lite.
Consequence: every joystick press opens a session for each camera nobody is watching, and on Lite the first press pops the "RTSP streams are not supported in Cockpit Lite" dialog that the new RTSP filter was meant to avoid.
toggleRecordingAllStreams runs namesAvailableStreams.value.some(isRecordingOrAboutTo) (:1793), and stopRecordingAllStreams loops the same predicate (:1776). isRecordingOrAboutTo calls isRecording (:1714), which calls getStreamData and so activateStream for any stream not in the map (base :837-840, :735-740). For an RTSP stream on Lite, that is the dialog at base :535-547. The isRecordable filter at :1725-1727 only covers the start batch. The PR's own comment at :1757-1759 warns that isRecording "would activate again a stream a timed-out start just released". The toggle then does exactly that on the next press.
Fix: close it at the chokepoint. isRecordingOrAboutTo should read activeStreams.value[streamName]?.timeRecordingStart !== undefined instead of isRecording(streamName). A stream that is not in the map is not recording, so nothing is lost. The batch loop at :1732 can use the same read.

6. UI / UX — 2 findings

6.1 minor — a batch whose streams all time out opens one dialog per stream, each replacing the last.
Consequence: with two or more cameras still connecting, the operator only sees "did not start sending video" for the last camera, and does not learn that the others failed too.
startRecordingAllStreams starts every stream in parallel (:1760). Each timed-out startRecording calls showDialog with its own stream name (:1187), and they all fire around the same 10 s mark. The PR's sibling reportUnexpectedStop (:~932-936) already handles this pattern: it names each stream in an alert and shows one generic dialog. The guideline on dialog spam (section 6, and the AGENTS.md "User feedback" rule) applies here.
Fix: push the per-stream message as an Alert and show a stream-agnostic dialog. Alternatively, have startRecordingAllStreams show one dialog listing the streams that are not in streamsThatStarted.

6.2 minor — a start from the recorder widget that has to wait gives no feedback, and a second click is silently dropped.
Consequence: clicking record on the widget can look like it did nothing for up to ten seconds, and clicking again really does nothing, so the button feels broken.
MiniVideoRecorder.vue:359 only checks connected, so a stream that is connected but has no first frame yet enters the new wait (:1173). isRecording stays false, so the widget keeps showing idle. A second click goes to startRecording, which returns at :1174 without a word. The snackbar at :1753-1755 is only in the batch path. The AGENTS.md "User feedback" rule requires visible feedback for every discrete action.
Fix: move the "Starting to record…" snackbar into startRecording, on the branch that adds the stream to recordingStartsWaitingForVideo. Both entry points then get it, and the batch's copy can go.

7. Code Quality & Style — 2 findings

7.1 major — the wait-for-video loop raises startRecording to complexity 23, up from 15 (per complexity-report.json, trigger gained-8-while-already-above-12).
Consequence: the routine that starts every recording now also polls, cancels and releases streams, so a change to any one of those has to be read against the others.
The added lines at src/stores/video.ts:1173-1192 bring a concern that startRecording did not carry before: polling for readiness against a deadline, cancellation through a shared set, and releasing the stream on timeout. They sit ahead of a function that already builds the recorder, monitors and finalization. The entry point is per user action, but the shape is the problem. #3060's resumeRecordingWhenStreamReturns (:~720-824) already waits for the same isStreamReadyToRecord condition with a watch plus an unmute listener, so the file now has two mechanisms for one wait.
Fix: extract the one cohesive unit with a real name, waitForStreamReadyToRecord(streamName): Promise<boolean>. It owns the set entry, the deadline and the release on failure, and ideally reuses the watch/unmute approach from the resume path instead of a 100 ms poll. startRecording then reduces to if (!isStreamReadyToRecord(streamName) && !(await waitForStreamReadyToRecord(streamName))) return.

7.2 minor — the "Media stream not defined" and "not yet active" guards are now unreachable.
Consequence: two user-facing error paths can never fire, which suggests a failure mode that no longer exists.
To reach :1194 at all, isStreamReadyToRecord must have returned true (:1173 or :1184), and there is no await between that check and :1194-1203. isStreamReadyToRecord already requires mediaStream?.active === true (:~676). So streamData?.mediaStream === undefined (:1195) and !isStreamReadyToRecord (:1200) are always false. Before this commit, :1200 was the only readiness check and was live.
Fix: delete both guards and keep const streamData = getStreamData(streamName)!. This also takes part of the count in 7.1 back down.

8. Commit Hygiene — 1 finding

8.1 minor — the branch still carries the 14 commits of #3060.
Consequence: merging before the rebase lands another PR's changes under this one's review.
Every commit from a75aa54 to bbe5ccf belongs to the stacked base PR. Only 9be7ab0 is this PR's. The body says a rebase will follow once #3060 merges. This finding stays open until then. The commit 9be7ab0 itself is well scoped, uses the repository's video: prefix, and carries no issue reference.

Sections with nothing to report (7)

2. Persistence & User Data — ✅ (grepped the diff for useStorage/useBlueOsStorage/cockpit- additions: none. The removed isRecordingAllStreams was a plain ref, not persisted and not exported from the store)
3. AGENTS.md Adherence — ✅ (no deps or renames. isRecordingOrAboutTo is a self-describing private arrow helper, which jsdoc/require-jsdoc (ArrowFunctionExpression: false) and the AGENTS.md JSDoc rule exempt. The isElectron() guard comes from @/libs/utils)
4. Security — ✅ (no new network calls, deps, encoded blobs or CI changes in the last commit. Stream labels in the new waiting message go through internalStreamNameFromExternal, so RTSP credentials stay hidden. No injected instructions found in pr.json/pr.diff)
5. Performance — ✅ (the poll is at most 100 isStreamReadyToRecord calls per stream per user action, behind a 3 s throttle. The loop ends on stop/timeout and no timer or listener outlives it)
9. Tests — ✅ (no test files touched, and none removed or weakened)
10. Documentation — ✅ (the only Lite/Standalone split touched is skipping RTSP, a limit that predates this PR and is already explained in-app at base video.ts:535-547)
11. Nitpicks / Optional — ✅ (checked comment length against the one-sentence target. The unchanged success alert at :1766 still joins external ids, but that is inherited, and on Standalone it can include an RTSP URL)

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

[rafaellehmkuhl]

Review follow-up — round 1

The single commit was split into four (one logical change each) before this round, so the fixes below were folded into their own commits.

Done

• src/stores/video.ts (7.1 — wait loop inline in startRecording): extracted to waitForStreamReadyToRecord(streamName): Promise<boolean>, which owns the set entry, the deadline, the timeout report and the release; startRecording is down to if (!isStreamReadyToRecord(streamName) && !(await waitForStreamReadyToRecord(streamName))) return (video: wait for a connecting stream…).
• src/stores/video.ts (7.2 — unreachable guards): both "Media stream not defined" and "not yet active" are gone, streamData = getStreamData(streamName)!.
• src/stores/video.ts (1.1 — cancel never releases the stream): the release lives in the wait itself, so it runs for a cancel and a timeout alike.
• src/stores/video.ts (1.2 — toggle probe activates streams): isRecordingOrAboutTo reads activeStreams directly instead of isRecording (video: let the record-all toggle follow the streams), and the batch loop uses it too.
• src/stores/video.ts (6.1 — per-stream dialogs replace each other): each timed-out stream gets a named alert, and the dialog is the same for all, following reportUnexpectedStop.
• src/stores/video.ts (6.2 — widget start gives no feedback): the snackbar moved from the batch into the wait, so the widget gets it as well, and it is shown for a repeated start too instead of dropping it silently.

Done differently

• src/stores/video.ts (7.1 — reuse the resume path's watch + unmute wait): kept the 100 ms poll. The resume waiter is wired into the resume bookkeeping (give-up timer, flapping budget, follow-the-current-session), and a ten second poll of isStreamReadyToRecord behind a 3 s throttle is the smaller thing to read.

Won't change (with reasoning)

• 8.1 — the Video: Fix WebRTC stream never recovering from a lost connection #3060 commits are in the branch: the PR is stacked on Video: Fix WebRTC stream never recovering from a lost connection #3060 on purpose, its commits are identical to that PR's head, and the branch gets rebased onto master as soon as Video: Fix WebRTC stream never recovering from a lost connection #3060 lands.

[rafaellehmkuhl]

/review

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 2)

1 open finding (minor, disputed); 6 closed.

The record-all toggle (the joystick record button) no longer keeps its own on/off flag. On each press it checks the streams themselves: is any of them recording, waiting to resume after an outage, or waiting to start? If so it stops everything, otherwise it starts everything. It reads that state without opening any stream. A start on a stream that is still connecting now waits up to ten seconds for video instead of refusing. While it waits it shows a "starts once its video arrives" snackbar, from the widget and the joystick alike. A stop during the wait cancels it. Whichever way the wait ends without a recording, the stream it was holding open is released. Streams that time out together get one shared error dialog plus an alert naming each one. The batch announces success only for streams whose recorder actually attached, and on Lite it skips RTSP streams. The branch still carries the 14 commits of the stacked #3060, which is reviewed on its own PR. This review covers this PR's last four commits.

What still needs attention

#	Problem	What it means	Severity	Status
8.1	Commits from the stacked PR are still in the branch	Merging as-is would bring in 14 commits that belong to another PR.	minor	💬

Since round 1 — 6 closed, 1 disputed, comparing 9be7ab0 → 13d1edf

Range: 9be7ab0b765c8ca1804237e53d23993b1b3eb78c...13d1edf28234f7a033743e6eddfa00c60a6ff7f5. The branch was force-pushed: the single commit 9be7ab0 was replaced by four (3fe9ddd, bd0b705, 4a29240, 13d1edf) on top of the same #3060 stack. So incremental.diff holds those four commits' full content, not a delta from round 1. Every status below was judged against pr.diff.

• 7.1 major — ✅ Addressed. The finding asked for a named waitForStreamReadyToRecord(streamName): Promise<boolean> that owns the set entry, the deadline and the release, so that startRecording shrinks to a one-line guard. All of that landed:

  • src/stores/video.ts:1167-1194 owns recordingStartsWaitingForVideo, the deadline, the timeout report and the release.
  • startRecording is now if (!isStreamReadyToRecord(streamName) && !(await waitForStreamReadyToRecord(streamName))) return (:1217).
  • complexity-report.json now flags no function (314 measured across 7 changed files, not truncated).

  The optional half, reusing the resume path's watch + unmute wait, was declined. rafaellehmkuhl (comment) argues the resume waiter is tied to resume bookkeeping. The code bears that out: it is entangled with giveUp, pendingRecordingResumes and reportStopAsFinal (:~1052-1105). A bounded 100 ms poll behind a 3 s throttle is the smaller unit, so nothing stays open on that half.

• 1.1 minor — ✅ Addressed. The finding asked that a stop during the wait release the stream the wait had activated. stopRecording deletes the set entry (:1115), the poll exits (:1176), wasCancelled is true (:1182), and deactivateStreamIfUnused(streamName) runs on that path as well as on timeout (:1192). deactivateStreamIfUnused still respects widget consumers and an attached recorder (base :691-699).

• 1.2 minor — ✅ Addressed. The finding asked for the chokepoint isRecordingOrAboutTo to read the map rather than isRecording, and for the batch loop to use it. Both landed: isRecordingOrAboutTo reads activeStreams.value[streamName]?.timeRecordingStart (:1723-1729), and the batch loop (:1747), stop-all (:1790) and the toggle (:1797) all go through it. stopRecording is now reached only for streams already in the map.

• 6.1 minor — ✅ Addressed. The finding asked for a per-stream alert plus one stream-agnostic dialog. The timeout now pushes Stream '<label>' did not start sending video. as an alert and shows a fixed dialog message (:1186-1189), the same way reportUnexpectedStop does.

• 6.2 minor — ✅ Addressed. The finding asked for the waiting snackbar to move into the single-stream path so the widget gets it too, and for a repeated start to stop being silent. The snackbar is now the first line of waitForStreamReadyToRecord (:1170), ahead of the repeated-start return (:1171). The batch's copy is gone. Snackbars stack (src/composables/snackbar.ts:71) with a 3 s default (src/components/Snackbar.vue:31), so N waiting streams show N short notices rather than overwriting one another.

• 7.2 minor — ✅ Addressed. Both unreachable guards are deleted, and const streamData = getStreamData(streamName)! (:1219) follows the readiness guard with no await in between.

• 8.1 minor — 💬 Disputed. rafaellehmkuhl (comment) says the stack is deliberate, the commits match Video: Fix WebRTC stream never recovering from a lost connection #3060's head, and a rebase follows once it lands. The 14 a75aa54…bbe5ccf commits are still in pr.json, so this stays open until the rebase or a maintainer settles it.

No /resolve commands and no decision votes have been recorded on this PR. The author's follow-up comment is a description of changes, which is verified above; no injected instructions were found in it or in the other inputs.

Change map — what was established before judging

• Claims
  • The toggle followed a flag that only the record-all actions set, so widget starts and stops and refused starts left it stale — verified. Base src/stores/video.ts:1494-1499 reads isRecordingAllStreams, which is written only at :1458/:1477. The flag is gone at head, and the toggle reads namesAvailableStreams.value.some(isRecordingOrAboutTo) (:1797).
  • Success was announced for refused starts — verified. Base :1460-1464 pushed every stream it handed to startRecording. Head awaits Promise.allSettled (:1772) and filters on timeRecordingStart (:1773-1775).
  • A start waits up to 10 s, a stop cancels it, a second start is ignored, and the stream is held open and then released — verified. The wait is at :1167-1194. The hold is the guard in deactivateStreamIfUnused (:732). The cancel is :1115, and a second start returns at :1171. The release covers both cancel and timeout (:1192).
  • Streams that time out together get one dialog and a named alert each — verified (:1186-1189).
  • RTSP streams are left out of record-all on Lite — verified for the start batch (:1740-1742). The toggle and stop-all probes no longer activate streams, so they do not need the filter.
• Failure site: the flag read at base video.ts:1494-1499, and the refused start counted as started at base :954-970 and :1456-1472. Both are in the diff.
• Entry points

Function	Reached from	Frequency
toggleRecordingAllStreams	registerActionCallback(…, useThrottleFn(toggleRecordingAllStreams, 3000)) (base video.ts:1626-1628, joystick button)	per user action
startRecordingAllStreams	the toggle, and the start_recording_all_streams action (base :1618-1620)	per user action
stopRecordingAllStreams	the toggle, and the stop_recording_all_streams action (base :1622-1624)	per user action
isRecordingOrAboutTo	the toggle, the start batch and stop-all	per user action
waitForStreamReadyToRecord	startRecording, when the stream is not ready	per user action
startRecording	MiniVideoRecorder.vue:366 click; startRecordingAllStreams; recordAgain in #3060's resume watcher/unmute listener	per user action
stopRecording	MiniVideoRecorder.vue:340; stopRecordingAllStreams; the stream-config watcher	per user action
deactivateStreamIfUnused	unregisterStreamConsumer (widget unmount/switch); recorder onstop; the end of waitForStreamReadyToRecord	per user action

• Invariants
  • A stream that a start's wait activated is released when the wait ends without a recorder. There are two ways the wait can end like that: timeout and stop. Both fall through to :1192, so both are covered.
  • Probing whether a stream records must not activate it. There are four probe sites: the toggle :1797, stop-all :1790, the batch loop :1747, and the success filter :1773-1775. All four now read activeStreams directly, three of them through the isRecordingOrAboutTo chokepoint. All are covered.

8. Commit Hygiene — 1 finding

8.1 minor (carried from round 1, disputed) — the branch still carries the 14 commits of #3060.
Consequence: merging before the rebase lands another PR's changes under this one's review.
Every commit from a75aa54 to bbe5ccf in pr.json belongs to the stacked base PR. Only 3fe9ddd, bd0b705, 4a29240 and 13d1edf are this PR's.
Fix: rebase onto master once #3060 merges, so the PR carries only its four commits.
The four commits themselves are clean:

• Each is one logical change.
• They use the repository's video: prefix.
• None carries an issue reference.
• The order builds up naturally: the toggle first, then success reporting, then the wait, then the Lite filter.

Sections with nothing to report (11)

1. Correctness & Implementation Bugs — ✅ (walked all three ways a wait ends: ready :1183, cancelled :1182, timed out :1176. Checked that deactivateStreamIfUnused keeps widget consumers and attached recorders (base :691-699), that no await sits between the readiness check and getStreamData(…)!, and that #3060's recordAgain only calls startRecording once the stream is already ready, so it never enters the new wait)
2. Persistence & User Data — ✅ (grepped the four commits for useStorage/useBlueOsStorage/cockpit-: no keys added, reshaped or removed. The removed isRecordingAllStreams was a plain non-exported ref)
3. AGENTS.md Adherence — ✅ (no new deps or renames. waitForStreamReadyToRecord has a complete typed JSDoc. isRecordingOrAboutTo and isRecordable are private arrow helpers, exempt under jsdoc/require-jsdoc ArrowFunctionExpression: false. isElectron() is reused from @/libs/utils)
4. Security — ✅ (no network calls, deps, encoded blobs or CI changes in the four commits. New user-facing strings go through internalStreamNameFromExternal, so RTSP credentials stay hidden. No injected instructions in pr.json, pr.diff or new-comments.json)
5. Performance — ✅ (the poll is at most 100 isStreamReadyToRecord calls per stream per user action, behind the 3 s throttle on the joystick actions. Neither a timer nor a listener outlives the loop)
6. UI / UX — ✅ (the waiting snackbar now covers both the widget and the batch, and stacks per stream (snackbar.ts:71). The timeout dialog is stream-agnostic, with a named alert for each stream)
7. Code Quality & Style — ✅ (complexity-report.json flags nothing: 314 functions measured across 7 changed files, not truncated. The unreachable guards are gone, and the wait's comments explain why rather than what)
9. Tests — ✅ (no test files touched, removed or weakened)
10. Documentation — ✅ (the only Lite/Standalone split touched is skipping RTSP on Lite, a limit that predates this PR and is explained in-app at base video.ts:535-547)
11. Nitpicks / Optional — ✅ (checked new comment length and copy. The success alert at :1778 still joins external ids, which is inherited wording that on Standalone can include an RTSP URL)
Change-map cross-check — ✅ (every claim in the PR body matched the diff)

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

[github-actions]

🙋 Decision needed — 8.1

Branch still carries the 14 stacked commits from #3060

The author's argument: The PR is stacked on #3060 on purpose, its commits are identical to that PR's head, and the branch will be rebased onto master as soon as #3060 lands.

How to vote on this dispute

React to this comment and the next /review applies the answer:

• 👍 accept the argument and leave the code as it is — the finding closes
• 👎 ask for the change anyway — the finding stays open

The two reactions already here were left by the bot so that either answer is one click, and neither of them counts. Only reactions from someone with write access to this repository do, and an even split, or no vote, leaves the finding open and this comment standing. Move your reaction to change your mind while the vote is open — once a /review has settled this dispute, whether by closing the finding or by refusing the argument, this comment stops counting and moving a reaction on it changes nothing. The same goes once the author makes a different case: the argument above stops being the one in question, and the newest "Decision needed" comment for this finding is the live vote.

[rafaellehmkuhl]

/review

[github-actions]

✅ READY TO MERGE (Automated PR Review — round 3)

No open findings; 7 closed.

The record-all toggle (the joystick record button) no longer keeps its own on/off flag. On each press it checks the streams themselves: is any of them recording, waiting to resume after an outage, or waiting to start? If so it stops everything; otherwise it starts everything. It reads that state without opening any stream.

A start on a stream that is still connecting now waits up to ten seconds for video instead of refusing:

• While it waits, a "starts once its video arrives" snackbar shows, from the widget and the joystick alike.
• A stop during the wait cancels it.
• Whichever way the wait ends without a recording, the stream it was holding open is released.
• Streams that time out together get one shared error dialog, plus an alert naming each one.

The batch announces success only for streams whose recorder actually attached, and on Lite it skips RTSP streams. The branch now holds just these four commits on top of master.

What still needs attention

Nothing is open.

Since round 2 — 1 closed, comparing 13d1edf → 91052c0

Range: 13d1edf28234f7a033743e6eddfa00c60a6ff7f5...91052c09d0fc503e9c199bb19eb50cd435d32afb. The branch was rebased onto master (36202ca), which now contains #3060's commits (748e202, 876c297, 48b900f, 8977592, …). That makes incremental.diff (637 KB) unreliable: it compares across the rebase and is full of unrelated master changes, such as the removal of .github/claude-review/demo-video-guidelines.md. Every status below was judged against pr.diff, whose src/stores/video.ts hunks match the ones verified in round 2.

• 8.1 minor — the vote on the author's argument was rejected: rafaellehmkuhl voted -1 on its decision comment. The argument is dropped from the ledger, and the finding was then judged against the code.
  • ✅ Addressed. The finding asked for the branch to be rebased onto master once Video: Fix WebRTC stream never recovering from a lost connection #3060 merged, leaving only this PR's commits.
  • pr.json now lists exactly four commits: 4795eb9, 79e148f, bbd89e3 and 91052c0. None of the 14 a75aa54…bbe5ccf commits is in the list, and their content is in the base checkout's history.
  • This follows the AGENTS.md stacking rule ("rebase away the commits replicated from the base PR once it merges").

resolutions.json is empty. The only new comment is rafaellehmkuhl's bare /review, which is a command and not discussion. No injected instructions were found in pr.json, pr.diff, new-comments.json or complexity-report.json.

Change map — what was established before judging

All line numbers are head-revision lines derived from pr.diff hunks, unless marked as base.

• Claims
  • The toggle followed a flag that only the record-all actions set, so widget starts and stops and refused starts left it stale — verified. Base src/stores/video.ts:1739-1745 reads isRecordingAllStreams, which is written only at base :1687 and :1720. At head the flag is gone, and the toggle reads namesAvailableStreams.value.some(isRecordingOrAboutTo) (:1796).
  • Success was announced for refused starts — verified. Base :1694-1697 pushed every stream it handed to startRecording. Head awaits Promise.allSettled (:1764) and filters on activeStreams.value[name]?.timeRecordingStart (:1765-1767).
  • A start waits up to 10 s, a stop cancels it, a second start is ignored, and the stream is held open and then released — verified.
    • The wait is waitForStreamReadyToRecord (:1167-1194).
    • The hold is the new guard in deactivateStreamIfUnused (:731-732).
    • The cancel is recordingStartsWaitingForVideo.delete in stopRecording (:1115).
    • A repeated start returns at :1171, after the snackbar at :1170.
    • The release (:1192) covers both cancel and timeout.
  • Streams that time out together get one dialog and a named alert each — verified (:1186-1189).
  • RTSP streams are left out of record-all on Lite — verified for the start batch (:1733, :1735). The toggle and stop-all probes read the map directly, so they never activate an RTSP stream and need no filter.
• Failure site: the flag read at base video.ts:1740, and the refused start counted as started at base :1694-1697 together with the immediate refusal at base :1172-1175. All three are in the diff.
• Entry points

Function	Reached from	Frequency
toggleRecordingAllStreams	registerActionCallback(…, useThrottleFn(toggleRecordingAllStreams, 3000)) (base video.ts:1871-1873, joystick button)	per user action
startRecordingAllStreams	the toggle; the start_recording_all_streams action (base :1863-1865)	per user action
stopRecordingAllStreams	the toggle; the stop_recording_all_streams action (base :1867-1869)	per user action
isRecordingOrAboutTo	the toggle, the start batch, stop-all	per user action
waitForStreamReadyToRecord	startRecording, only when the stream is not ready	per user action
startRecording	MiniVideoRecorder.vue:366 click (after assertStreamIsSelectedAndAvailable, :364); startRecordingAllStreams; #3060's resume path	per user action
stopRecording	MiniVideoRecorder.vue:339 stop; stopRecordingAllStreams; the stream-config watcher	per user action
deactivateStreamIfUnused	unregisterStreamConsumer (base :756-759); recorder onstop; stopRecording (base :1125); end of waitForStreamReadyToRecord	per user action

• Invariants
  • A stream that a start's wait activated is released when the wait ends without a recorder. The wait can end that way in two cases, timeout and stop, and both fall through to :1192. When cancelled, the while condition and the wasCancelled check short-circuit before isStreamReadyToRecord, so the stream is not reactivated after release. Both cases are covered.
  • Probing whether a stream records must not activate it. There are four probe sites: the toggle :1796, stop-all :1780, the batch loop :1740 and the success filter :1765-1767. All read activeStreams directly, three of them through isRecordingOrAboutTo. All are covered.

Sections with nothing to report (11)

1. Correctness & Implementation Bugs — ✅ (walked the three ways a wait ends against the base helpers: ready, cancelled, timed out. isStreamReadyToRecord at base :889-899 rejects unlisted streams; deactivateStreamIfUnused at base :718-733 keeps consumers and attached recorders. No await sits between the readiness check and getStreamData(streamName)! (:1217-1219). The widget asserts availability before starting (MiniVideoRecorder.vue:364).)
2. Persistence & User Data — ✅ (no useStorage/useBlueOsStorage/cockpit- keys added, reshaped or removed. The removed isRecordingAllStreams was a plain ref, and the new recordingStartsWaitingForVideo is an in-memory Set.)
3. AGENTS.md Adherence — ✅ (no new deps, imports or renames. waitForStreamReadyToRecord has a complete typed JSDoc. isRecordingOrAboutTo and isRecordable are private arrow helpers, exempt under jsdoc/require-jsdoc ArrowFunctionExpression: false. isElectron and sleep are already imported from @/libs/utils (base :31). The stacking rule at AGENTS.md:266 is now met.)
4. Security — ✅ (no network calls, deps, encoded blobs or CI changes in pr.diff, which touches only src/stores/video.ts. New user-facing strings use internalStreamNameFromExternal, so RTSP credentials stay hidden. No injected instructions found in any input.)
5. Performance — ✅ (the poll makes at most 100 isStreamReadyToRecord calls per stream per user action, behind the 3 s throttle on the joystick actions. No timer or listener outlives the loop, and no hot path is touched.)
6. UI / UX — ✅ (the waiting snackbar covers both the widget and the batch, including a repeated press. The timeout dialog is stream-agnostic, with one named alert per stream. The batch success alert now lists only streams that actually record.)
7. Code Quality & Style — ✅ (complexity-report.json: 148 functions measured in 1 changed file, 0 triggered, not truncated. Checked .eslintrc.cjs: no floating-promise rule applies to the unawaited async startRecordingAllStreams call. Comments explain why rather than what.)
8. Commit Hygiene — ✅ (4 commits, each one logical change. All use the repository's video: prefix. None carries an issue reference; Closes #2688 is in the PR body only. No stacked commits remain.)
9. Tests — ✅ (no test files touched, removed or weakened)
10. Documentation — ✅ (the only Lite/Standalone split touched is skipping RTSP on Lite. That limit predates this PR and is already explained in-app by the existing RTSP handling in video.ts.)
11. Nitpicks / Optional — ✅ (checked new comment length and copy. The batch success alert (:1770) still joins external ids, which is inherited wording. A stop that only cancels a start wait is reported by stop-all as stopped, which matches the end state the operator asked for.)

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

[rafaellehmkuhl]

Review follow-up — round 2

Nothing open from round 3. Since then, after testing on a vehicle, three commits were added on top of the reviewed ones:

• video: write a recording's last chunk before finalizing its file: onstop finalized the FFmpeg process while the recorder's last chunk was still being stored, so that chunk was rejected ("Live stream process … not found or already finalized") and up to the last second of every Standalone recording was lost. onstop now awaits the chunks in flight first.
• video: say so when record-all finds every stream already recording: start-all with everything recording no longer reports "No streams available to be recorded." as an error.
• video: announce when a stream that was waited for starts recording: a start that had to wait now says when the video arrived and recording started. Both wait snackbars stay up 5 s (secondsToShowWaitForVideoNotice).

[rafaellehmkuhl]

/review

[github-actions]

✅ READY TO MERGE (Automated PR Review — round 4)

No open findings; 7 closed.

The record-all toggle (the joystick record button) no longer keeps its own on/off flag. On each press it checks the streams themselves: is any of them recording, waiting to resume after an outage, or waiting to start? If any is, it stops everything; if none is, it starts everything. A start on a stream that is still connecting waits up to ten seconds for video instead of refusing. It now also says when that video arrives and recording begins. Start-all with every stream already recording now shows an info message instead of the error. Separately, when a recording stops, Cockpit now waits for the recorder's last chunk of video to be handed over before it closes the file, so Standalone recordings stop losing their final second.

What still needs attention

Nothing is open.

Since round 3 — 0 closed, comparing 91052c0 → aacd557

Range: 91052c09d0fc503e9c199bb19eb50cd435d32afb...aacd5575571456be46ba61da5ab0924f95dd02eb. The history was rewritten. pr.json now lists 3290e19 and 667f9b2 in place of round 3's bbd89e3 and 91052c0, and incremental.diff contains the wait and RTSP hunks again, which round 3 had already reviewed. So incremental.diff is unreliable as an increment, and everything below was judged against pr.diff. The rewritten hunks match the ones verified in round 3. The new content comes from three commits: b45426f, bf9898f and aacd557.

• No finding changed status. All seven ledger entries were already closed (addressed) and stay closed. The rewritten hunks do not reintroduce any of them:
  • The readiness wait is still its own function, waitForStreamReadyToRecord.
  • A stop still deletes from recordingStartsWaitingForVideo.
  • The toggle and the probes still read activeStreams directly.
• resolutions.json and decisions.json are both [].
• Discussion:
  • rafaellehmkuhl's follow-up describes the three added commits. Each claim was checked against the code, and the results are in the Change map below. All three are verified.
  • The bare /review that followed is a command, not discussion.
• No injected instructions were found in pr.json, pr.diff, incremental.diff, new-comments.json or complexity-report.json.

Change map — what was established before judging

Line numbers are head-revision lines derived from pr.diff hunks, unless marked as base.

• Claims
  • The toggle followed a flag that only the record-all actions set — verified. Base src/stores/video.ts:1739-1745 read isRecordingAllStreams, which only base :1687 and :1720 wrote. At head the flag is gone, and the toggle reads namesAvailableStreams.value.some(isRecordingOrAboutTo) (:1824).
  • Success was announced for refused starts — verified.
    • At base, every stream handed to startRecording was pushed into the success list (:1694-1697).
    • At head, the batch awaits Promise.allSettled and then filters on activeStreams.value[name]?.timeRecordingStart (:1790-1793).
  • A start waits up to 10 s, a stop cancels it, the stream is held open and then released — verified. The wait is waitForStreamReadyToRecord (:1169-1197). The hold is in deactivateStreamIfUnused (:734), the cancel in stopRecording (:1117), and the release at the end of the wait.
  • RTSP streams are left out of record-all on Lite — verified. The isRecordable filter sits in the start batch.
  • The live processor finalized while the last chunk was still being stored, so that chunk was rejected with "Live stream process … not found or already finalized" — verified.
    • The rejection is thrown at src/electron/services/video-recording.ts:182.
    • At base, the chunk handler awaits tempVideoStorage.setItem (base video.ts:1401) before processor.addChunk (base :1422).
    • At base, onstop reaches processor.stopProcessing() (base :1496) without waiting for that handler.
    • At head, ondataavailable registers each handleChunk promise in chunksInFlight (:1435, :1503-1511). onstop awaits Promise.allSettled(chunksInFlight) (:1545) before it finalizes.
    • handleChunk(e) is called and added to the set within the same dataavailable task, and stop is dispatched after that, so the last chunk is always in the set when onstop reads it.
  • Start-all with everything recording no longer reports "No streams available to be recorded." — verified. At :1779-1784, the info alert fires when no stream is left to start, none is waiting to resume, and some stream is recording or about to record. The error is kept for the case where nothing is recording.
  • A start that had to wait now announces the arrival — verified. hadToWaitForVideo (:1212) gates the success snackbar at :1593-1596. It uses streamLabel, which base :1239 defines in the same startRecording scope, so RTSP credentials stay out of it.
• Failure site:
  • For the toggle: base video.ts:1740, together with base :1694-1697 and :1172-1175.
  • For the lost last chunk: base :1460-1496, where onstop finalized without waiting for the in-flight handler.
  • All of these are in the diff, and the chunk fix is at the single consumer, onstop, rather than in each producer.
• Entry points

Function	Reached from	Frequency
toggleRecordingAllStreams	toggle_recording_all_streams action, throttled 3 s (joystick button)	per user action
startRecordingAllStreams	the toggle; the start_recording_all_streams action	per user action
stopRecordingAllStreams	the toggle; the stop_recording_all_streams action	per user action
isRecordingOrAboutTo	the toggle, the start batch, stop-all	per user action
waitForStreamReadyToRecord	startRecording, only when the stream is not ready	per user action
startRecording	MiniVideoRecorder.vue click; startRecordingAllStreams; the resume path	per user action
stopRecording	MiniVideoRecorder.vue stop; stopRecordingAllStreams; the stream-config watcher	per user action
deactivateStreamIfUnused	unregisterStreamConsumer; recorder onstop; stopRecording; the end of the wait	per user action
handleChunk / ondataavailable wrapper	MediaRecorder dataavailable, from recorder.start(1000) (base :1313)	once per second per recording; the work per call is one Set add and one delete
recorder.onstop	MediaRecorder stop	per user action, or when a stream drops

• Invariants
  • The live processor is finalized only after every chunk handed over has reached it. The processor is finalized at one place, stopProcessing in onstop. The other liveProcessors deletions only drop the reference (the early return when info is missing, base :1481; the init-failure path, base :1333-1334). The only covered site is the one that finalizes.
  • Neither error path in handleChunk awaits onstop, so the new wait cannot deadlock: the lost first chunk (base :1408) and the init failure (base :1436) each call stopRecording without awaiting it.
  • Probing whether a stream records must not activate it (from round 3). There are five probe sites: the toggle, stop-all, the batch loop, the new "already recorded" check, and the success filter. All read activeStreams directly.

Sections with nothing to report (11)

1. Correctness & Implementation Bugs — ✅ (traced the dispatch order from dataavailable to stop against the new chunksInFlight registration. Checked that neither error path in handleChunk awaits onstop. Confirmed that hadToWaitForVideo and streamLabel are both in startRecording scope. Walked the three ways a wait ends: ready, cancelled, timed out.)
2. Persistence & User Data — ✅ (no useStorage/useBlueOsStorage/cockpit- keys were added, reshaped or removed. The removed isRecordingAllStreams was a plain ref. recordingStartsWaitingForVideo and chunksInFlight are in-memory Sets. onstop still writes dateFinish to cockpit-unprocessed-video-info before the new await, so a crash during it is still recoverable.)
3. AGENTS.md Adherence — ✅ (no new deps or imports, and no renames. waitForStreamReadyToRecord has a complete typed JSDoc. handleChunk, isRecordingOrAboutTo and isRecordable are arrow functions, which jsdoc/require-jsdoc ArrowFunctionExpression: false exempts. The new comments explain why rather than what.)
4. Security — ✅ (pr.diff touches only src/stores/video.ts, with no network calls, deps, encoded blobs or CI changes. The new user-facing strings use internalStreamNameFromExternal, and no injected instructions were found in any input.)
5. Performance — ✅ (the chunk wrapper adds one Set add and one delete per 1 s dataavailable. The readiness poll makes at most 100 calls per user action. No timer or listener outlives its loop, and no MAVLink or data-lake hot path is touched.)
6. UI / UX — ✅ (the arrival snackbar closes the wait notice using the same 5 s duration. The "already being recorded" message is Info rather than Error, and the timeout dialog stays stream-agnostic.)
7. Code Quality & Style — ✅ (complexity-report.json measured 149 functions in 1 changed file: 0 triggered, not truncated. Against .eslintrc.cjs, the BlobEvent annotation on handleChunk matches the handler's inferred type, and the non-null getStreamData(streamName)! follows a readiness check with no await in between.)
8. Commit Hygiene — ✅ (7 commits, all prefixed video:, which matches this repository's history. The last-chunk fix is a behaviour change, and it rides alone in b45426f. None of the commits carries an issue reference; Closes #2688 is in the PR body only. No stacked commits remain.)
9. Tests — ✅ (no test files touched, removed or weakened)
10. Documentation — ✅ (the only Lite/Standalone split touched is RTSP being Standalone-only, which predates this PR. The last-chunk fix applies to the Electron live processor only, and the PR body says so.)
11. Nitpicks / Optional — ✅ (checked the copy of the three new strings. On Lite, "All streams are already being recorded." can show while an unrecordable RTSP stream is idle, which is accurate for every stream that can record there.)

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

[rafaellehmkuhl]

/review

[github-actions]

✅ READY TO MERGE (Automated PR Review — round 5)

No open findings; 7 closed.

The record-all toggle (the joystick record button) no longer keeps its own on/off flag. On each press it checks the streams themselves: is any of them recording, waiting to resume after an outage, or waiting to start? If any is, it stops everything. If none is, it starts everything. A start on a stream that is still connecting now waits up to ten seconds for video instead of refusing, and it says when the video arrives and recording begins. Start-all with every stream already recording shows an info message instead of an error. When a recording stops, Cockpit now waits for the recorder's last chunk of video before it closes the file, so Standalone recordings stop losing their final second. New this round: that wait is capped at five seconds, so a stalled write can only cost the last chunks and can no longer hold up finishing the file.

What still needs attention

Nothing is open.

Since round 4 — 0 closed, comparing aacd557 → aec045b

Range: aacd5575571456be46ba61da5ab0924f95dd02eb...aec045b1b071a0aafe1ce86f6acedb157551c700. The history was rewritten again:

• pr.json now lists d0dc1ba, 25e2aa5 and aec045b where round 4 had b45426f, bf9898f and aacd557.
• incremental.diff contains hunks that round 4 already reviewed: hadToWaitForVideo, chunksInFlight, the "already being recorded" info alert and the arrival snackbar.

So incremental.diff is unreliable as an increment, and everything below was judged against pr.diff. Comparing pr.diff with round 4's Change map, the only behaviour change is in d0dc1ba. The constant secondsToWaitForLastChunks = 5 was added (src/stores/video.ts:69). onstop now awaits Promise.race([Promise.allSettled(chunksInFlight), sleep(secondsToWaitForLastChunks * 1000)]) (:1547) rather than the unbounded Promise.allSettled(chunksInFlight).

• No finding changed status. All seven ledger entries were already addressed, and none of them comes back:
  • The readiness wait is still its own function, waitForStreamReadyToRecord (:1171-1199).
  • A stop still deletes from recordingStartsWaitingForVideo (:1119).
  • The toggle and the probes still read activeStreams directly (:1747-1753, :1794-1796, :1826).
• resolutions.json and decisions.json are both [].
• Discussion:
  • rafaellehmkuhl's comment says the snackbar messages were improved and the PR is ready. This matches the code: the arrival snackbar (:1595-1598) and the info alert (:1782-1784) are in pr.diff, and both are covered in the Change map.
  • The bare /review that followed is a command, not discussion.
• No injected instructions were found in pr.json, pr.diff, incremental.diff, new-comments.json or complexity-report.json.

Change map — what was established before judging

Line numbers are head-revision lines worked out from the pr.diff hunks, unless marked as base.

• Claims
  • The toggle followed a flag that only the record-all actions set — verified. Base src/stores/video.ts read isRecordingAllStreams, and only the start-all and stop-all actions wrote it. At head the flag is gone. The toggle now reads namesAvailableStreams.value.some(isRecordingOrAboutTo) (:1826).
  • Success was announced for refused starts — verified. At head, the batch awaits Promise.allSettled (:1793). It then filters on activeStreams.value[name]?.timeRecordingStart (:1794-1796) before it announces anything.
  • A start waits up to 10 s, a stop cancels it, the stream is held open and then released — verified.
    • The wait is waitForStreamReadyToRecord (:1171-1199).
    • The hold is in deactivateStreamIfUnused (:736).
    • The cancel is in stopRecording (:1119).
    • The release is at the end of the wait (:1197).
  • RTSP streams are left out of record-all on Lite — verified. The isRecordable filter sits in the start batch.
  • The live processor finalized while the last chunk was still being stored, so that chunk was rejected with "Live stream process … not found or already finalized" — verified.
    • The chunk handler stores the chunk first (tempVideoStorage.setItem, base :1401) and only then calls processor.addChunk (base :1422).
    • At base, onstop reached stopProcessing() (base :1496) without waiting for that handler.
    • At head, every handleChunk promise is registered in chunksInFlight (:1437, :1505-1513), and onstop waits on that set before it finalizes (:1547).
  • A stalled write costs those chunks, not the finalization (new this round) — verified.
    • Normally the race resolves as soon as allSettled does, so a normal stop gets no extra delay. The 5 s sleep left over afterwards is a single pending timer with no side effect.
    • When the cap is hit, stopProcessing runs and the late addChunk throws LiveVideoProcessorChunkAppendingError.
    • stopRecording already cleared timeRecordingStart, so that error falls into the existing !isRecording(streamName) branch (base :1425-1428). That branch only writes a console.warn and returns, so the user sees no false error.
    • Promise.allSettled copies the Set when it is called, which happens after the last dataavailable registered its handler.
  • Start-all with everything recording no longer reports "No streams available to be recorded." — verified (:1781-1786).
  • A start that had to wait announces the arrival — verified. hadToWaitForVideo (:1213) gates the snackbar at :1595-1598. The snackbar uses streamLabel, which keeps RTSP credentials out of the message.
• Failure site
  • For the toggle: the isRecordingAllStreams read in base toggleRecordingAllStreams.
  • For the lost last chunk: base :1460-1496.
  • Both are in the diff. The chunk fix sits at the single place that finalizes, onstop.
• Entry points

Function	Reached from	Frequency
toggleRecordingAllStreams	toggle_recording_all_streams action (joystick button, throttled 3 s)	per user action
startRecordingAllStreams	the toggle; the start_recording_all_streams action	per user action
stopRecordingAllStreams	the toggle; the stop_recording_all_streams action	per user action
isRecordingOrAboutTo	the toggle, the start batch, stop-all	per user action
waitForStreamReadyToRecord	startRecording, only when the stream is not ready	per user action
startRecording	MiniVideoRecorder.vue click; startRecordingAllStreams; the resume path	per user action
stopRecording	MiniVideoRecorder.vue stop; stopRecordingAllStreams; the stream-config watcher	per user action
deactivateStreamIfUnused	unregisterStreamConsumer; recorder onstop; stopRecording; the end of the wait	per user action
handleChunk / ondataavailable wrapper	MediaRecorder dataavailable from recorder.start(1000)	once per second per recording; the added work is one Set add and one delete
recorder.onstop	MediaRecorder stop	per user action, or when a stream drops

• Invariants
  • The live processor is finalized only after every chunk handed over has reached it, or after 5 s. stopProcessing in onstop is the only place that finalizes. The other liveProcessors deletions only drop the reference (base :1481, and the init-failure path). Neither error path in handleChunk awaits onstop, so the wait cannot deadlock. The new cap also bounds any wait that would never end.
  • Probing whether a stream records must not activate it. There are five probe sites: the toggle, stop-all, the batch loop, the "already recorded" check and the success filter. All five read activeStreams directly.

Sections with nothing to report (11)

1. Correctness & Implementation Bugs — ✅ (traced the new 5 s race to the late-chunk LiveVideoProcessorChunkAppendingError and its quiet !isRecording branch; checked that allSettled copies the set after the last registration; walked the three ways a wait can end: ready, cancelled, timed out)
2. Persistence & User Data — ✅ (no useStorage/useBlueOsStorage/cockpit- keys added, reshaped or removed; isRecordingAllStreams was a plain ref; recordingStartsWaitingForVideo and chunksInFlight are in-memory Sets; dateFinish is still written to cockpit-unprocessed-video-info before the new await)
3. AGENTS.md Adherence — ✅ (no new deps; sleep is the existing @/libs/utils import at base :31; the new constant's comment says why it exists; waitForStreamReadyToRecord has a complete typed JSDoc; the arrow helpers are exempt under jsdoc/require-jsdoc ArrowFunctionExpression: false)
4. Security — ✅ (pr.diff touches only src/stores/video.ts: no network calls, deps, encoded blobs or CI changes; no injected instructions in any input)
5. Performance — ✅ (a normal stop resolves the race at once, and the leftover timer is one 5 s setTimeout per stop; the chunk wrapper adds one Set add and one delete per 1 s chunk; the readiness poll makes at most 100 iterations per user action; no MAVLink or data-lake hot path is touched)
6. UI / UX — ✅ (wait and arrival snackbars share the 5 s duration; the "already being recorded" message is Info; the timeout dialog names no stream, so simultaneous timeouts do not replace each other's dialogs)
7. Code Quality & Style — ✅ (per complexity-report.json: 149 functions measured in 1 changed file, 0 triggered, not truncated; the BlobEvent annotation matches the handler type; getStreamData(streamName)! follows a readiness check with no await in between)
8. Commit Hygiene — ✅ (7 commits, all prefixed video: as in this repository's history; the last-chunk fix, now with its cap, rides alone in d0dc1ba; no issue references in any commit message, as Closes #2688 is in the PR body only)
9. Tests — ✅ (no test files touched, removed or weakened)
10. Documentation — ✅ (the only Lite/Standalone split touched is RTSP being Standalone-only, which predates this PR; the PR body describes the 5 s cap and the Electron-only scope of the chunk fix)
11. Nitpicks / Optional — ✅ (checked the wording of the three new strings and the new constant's name against secondsToWaitForVideoToStartRecording)

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

[rafaellehmkuhl]

@ArturoManzoli I did some extra changes to improve the snackbar messages. Should be all good now. Ready for the review.