Video: Broadcast recording and snapshot actions over MAVLink

[ArturoManzoli]

• sendCommand/sendCommandLong gain an awaitAck flag (plus a trailing targetComponent/awaitAck options object), so camera commands can be sent fire-and-forget instead of blocking on a COMMAND_ACK that Cockpit's autopilot-only ACK filter never surfaces.
• MAVLinkVehicle gains camera helpers for VIDEO_START/STOP_CAPTURE, IMAGE_START_CAPTURE, SET_CAMERA_MODE, REQUEST_CAMERA_CAPTURE_STATUS, and a REQUEST_MESSAGE-based CAMERA_INFORMATION fetch.
• resolveCameraTarget maps a configured camera id to the MAVLink target component and camera param (0 broadcasts, 1-6 autopilot-connected, 7-255 dedicated camera component).
• onIncomingMessage intercepts CAMERA_CAPTURE_STATUS/CAMERA_INFORMATION from camera components ahead of the autopilot-only component filter and re-emits them as signals.
• Main-vehicle store exposes thin wrappers around the new command methods and caches the latest per-component capture status and camera information.
• Video store derives remote camera capability, recording, staleness and unanswered state from those caches; video and snapshot stores emit the commands on recording start/stop and snapshot, guarded by new cockpit--prefixed BlueOS-synced settings (master toggle default off, target camera/stream ids, optional set-mode-before-capture and request-capture-status).
• Video configuration panel adds the opt-in toggle, id inputs, sub-option checkboxes and a live camera-feedback readout; the recorder mini-widget shows a badge mirroring the in-vehicle recording/capability/staleness state. Every new control logs through logUserAction.

Closes #2695

[ArturoManzoli]

/review

[github-actions]

Automated PR Re-review 1 (Claude)

This is a full initial review — no previous review exists for this PR.

This PR adds MAVLink camera control commands (video start/stop capture, image start/stop capture, set camera mode, request capture status) to the vehicle layer, exposes them through the main-vehicle store, and wires them into the video and snapshot stores as an opt-in broadcast behind new cockpit--prefixed settings. A configuration panel surfaces the toggle and sub-options with logUserAction logging.

0. Summary

Verdict: MINOR SUGGESTIONS

Well-structured PR with clean commit separation and good adherence to project conventions. The MAVLink camera commands are correctly sent fire-and-forget to avoid the ACK-filter timeout, settings use proper cockpit- prefixes, and user interactions are logged. A few items worth addressing: redundant try/catch blocks around synchronous fire-and-forget calls, missing input clamping on the camera/stream ID fields, and duplicate MAVLink broadcasts when recording multiple streams.

Minor items to consider: 1.1, 1.2, 1.3, 4.1, 5.1, 6.1.

1. Correctness & Implementation Bugs

1.1 (minor) — Dead try/catch around fire-and-forget calls.
src/stores/snapshot.ts:283-289 (diff) and src/stores/video.ts broadcastRecordingStart/broadcastRecordingStop: the camera command wrappers (startImageCapture, setCameraMode, etc.) are synchronous void-returning functions that internally call void this.sendCommandLong(...). The void discards the promise, so rejections are swallowed at the vehicle layer and will never propagate to these try/catch blocks. The catch clauses are dead code. Consider either removing the try/catch (since errors can't reach it) or, if error handling is genuinely desired, awaiting the promise chain instead of voiding it.

1.2 (minor) — No input clamping on mavlinkCameraTargetId.
src/views/ConfigurationVideoView.vue — the v-text-field for "Target camera ID" sets max="255" and min="0" as HTML attributes, but these don't prevent a user from typing or pasting values outside [0, 255]. A value like 999 would be stored and passed to resolveCameraTarget, which would return [999, 999] — an invalid MAVLink component ID (uint8 0-255). Either clamp the value in the blur handler or add Vuetify validation rules (like the existing jitterBufferTargetRules pattern).

1.3 (minor) — Duplicate MAVLink broadcast when recording multiple streams.
broadcastRecordingStart() and broadcastRecordingStop() are called once per startRecording/stopRecording invocation. If the user records multiple streams simultaneously (e.g. via the "record all" action), the same MAVLink command is sent once per stream. This is harmless since the commands are fire-and-forget and idempotent, but sending multiple identical MAV_CMD_VIDEO_START_CAPTURE commands per user action is noisy. Consider guarding with a flag or moving the broadcast to the action entry point.

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance

4.1 (nit) — Voided promise rejections are silently swallowed.
All camera command methods in vehicle.ts (startVideoCapture, stopVideoCapture, startImageCapture, stopImageCapture, setCameraMode, requestCameraCaptureStatus) use void this.sendCommandLong(...), which discards the promise. If sendMavlinkMessage throws synchronously or the send fails, the error is silently lost. Since these are fire-and-forget by design, this is acceptable, but adding a .catch(console.debug) to at least one representative call would make the design intent explicit and aid debugging.

5. UI / UX

5.1 (nit) — Sub-options remain visible even when the parent toggle is off.
When broadcastCameraActionsOverMavlink is toggled off, the <template v-if="videoStore.broadcastCameraActionsOverMavlink"> correctly hides the camera ID, stream ID, and sub-checkboxes. This is good. However, the text fields don't have Vuetify :rules for range validation, which means the user gets no visual feedback for out-of-range values (see 1.2).

6. Code Quality & Style

6.1 (nit) — JSDoc @returns {void} on synchronous void functions.
The camera methods in vehicle.ts (startVideoCapture, stopVideoCapture, etc.) all have @returns {void} in their JSDoc. Per the eslint jsdoc/require-returns config (forceReturnsWithAsync: false), @returns is not required for void functions and adds no information. This is a stylistic choice, though — the codebase does include @returns {void} in other places, so it's consistent with local convention.

7. Commit Hygiene

Commits are well-structured:

1. lib: vehicle-mavlink: add camera capture and control commands — protocol layer
2. stores: main-vehicle: expose camera capture commands — store wrappers
3. stores: broadcast recording and snapshot actions over mavlink — feature wiring
4. views: configuration-video: add mavlink broadcast settings — UI

Each commit is a single logical change, subjects use appropriate types, and there are no noise commits. Clean.

7. Commit Hygiene — ✅

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional

10.1 (nit) — Consider extracting the resolveCameraTarget logic or at least its constants (the 1-6 / 7-255 ranges) into a named constant or comment block in types.ts, since the same split is documented in three separate JSDoc blocks and the configuration UI hint text.

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

[ArturoManzoli]

Done:

• (1.1) dropped the dead try/catch in broadcastRecordingStart/broadcastRecordingStop and the snapshot broadcast block — the camera sends are fire-and-forget so rejections never reached those catches; errors are now handled at the vehicle layer.
• (4.1) routed all six camera sends through a single sendCameraCommand helper that attaches an explicit .catch(console.debug), so fire-and-forget failures are logged on purpose (also removes the repeated trailing-zero param lists).
• (1.2) handleCameraTargetIdBlur/handleVideoStreamIdBlur now clamp the value to a valid uint8 (0-255, rounded, NaN→0) on blur, so out-of-range ids can't reach resolveCameraTarget.

Not addressed:

• (1.3) left as-is — the commands are idempotent fire-and-forget (harmless per the review), and deduping per user-action needs reworking the multi-stream recording entry point, which isn't worth the risk here.
• (5.1) the blur clamp already prevents out-of-range values, so adding :rules on top would be redundant.
• (6.1) @returns {void} matches the existing codebase convention and isn't required to be removed by eslint.
• (10.1) the range logic lives only in resolveCameraTarget; the repetition cited is human-facing JSDoc/hint prose, not duplicated code, so a shared constant wouldn't remove real duplication.

[ArturoManzoli]

/review

[github-actions]

Automated PR Re-review 2 (Claude)

Comparing 185773e72f427969189e7cdaf2daa2d2d8f57828 → 82e0298341bfaceb2fa16e0150f7a6a6b9aecc01. The commits were rebased/rewritten since the last review, so the incremental diff covers the entire PR; re-checked against the full pr.diff.

Previous findings status

#	Finding	Severity	Status
1.1	Dead try/catch around fire-and-forget calls	minor	✅ Addressed — try/catch removed; errors now handled via .catch(console.debug) in sendCameraCommand
1.2	No input clamping on mavlinkCameraTargetId	minor	✅ Addressed — clampToUint8 added and applied on blur for both camera target ID and video stream ID
1.3	Duplicate MAVLink broadcast when recording multiple streams	minor	❌ Not addressed — author explicitly chose to keep as-is; the commands are idempotent/fire-and-forget so the noise is harmless
4.1	Voided promise rejections silently swallowed	nit	✅ Addressed — sendCameraCommand now catches with console.debug, making the fire-and-forget intent explicit
5.1	No Vuetify :rules for range validation visual feedback	nit	:large_yellow_circle: Partially addressed — blur clamp prevents invalid values reaching resolveCameraTarget, but no inline visual feedback while typing; author considers :rules redundant given the clamp, which is reasonable
6.1	@returns {void} on synchronous void functions	nit	⚪ No longer applicable — matches existing codebase convention
10.1	Consider extracting resolveCameraTarget constants	nit	⚪ No longer applicable — the range logic lives only in resolveCameraTarget; the repetition is human-facing prose, not duplicated code

Discussion since last review

@ArturoManzoli provided a detailed comment explaining which findings were addressed and which were intentionally left as-is. The reasoning is clear and sound — 1.1, 1.2, and 4.1 were fixed; 1.3 was acknowledged as harmless noise not worth the refactoring risk; 5.1's blur clamp is functionally sufficient; 6.1 and 10.1 are stylistic choices consistent with the codebase.

New findings

0. Summary

Verdict: READY TO MERGE

The three actionable findings from the initial review (1.1, 1.2, 4.1) have been addressed cleanly. The sendCameraCommand helper centralises the fire-and-forget pattern with an explicit .catch, the UI clamps camera/stream IDs to valid uint8 on blur, and the dead try/catch blocks are gone. The remaining open item (1.3 — duplicate broadcast per stream) is acknowledged as harmless and intentionally deferred. The PR is well-structured and ready.

1. Correctness & Implementation Bugs — ✅

2. AGENTS.md Adherence — ✅

3. Security — ✅

4. Performance — ✅

5. UI / UX — ✅

6. Code Quality & Style — ✅

7. Commit Hygiene — ✅

8. Tests — ✅

9. Documentation — ✅

10. Nitpicks / Optional — ✅

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

[ArturoManzoli]

Ready to go.

[joaoantoniocardoso]

I want to test it with my setup once it's ready.

[ArturoManzoli]

I want to test it with my setup once it's ready.

It's ready for testing, @joaoantoniocardoso

[rafaellehmkuhl]

The PR looks good to me.

@joaoantoniocardoso can will leave the approval to you as you are more familiar with the protocol and has a proper way to test it?

[rafaellehmkuhl]

I believe it would be good to rename that one and the others to something like "sendVideoCaptureCommand", so it's not confused with the regular video/snapshot commands.

[ArturoManzoli]

I believe it would be good to rename that one and the others to something like "sendVideoCaptureCommand", so it's not confused with the regular video/snapshot commands.

Changed:
startVideoCapture -> sendStartVideoCaptureCommand
stopVideoCapture -> sendStopVideoCaptureCommand
startImageCapture -> sendStartImageCaptureCommand
stopImageCapture -> sendStopImageCaptureCommand
setCameraMode -> sendSetCameraModeCommand
requestCameraCaptureStatus -> sendRequestCameraCaptureStatusCommand

[rafaellehmkuhl]

All good from my side. Will leave the behaviour review with Joao.

[joaoantoniocardoso]

Tested.

A few observations:

1. The main workflow does work, and it can be tested using this BlueOS Experimental recorder branch, observing the logs from blueos-recorder tmux panel.
2. MCM doesn't honor broadcast ID 0 yet (being added here), so I had to set the ID to 100 manually.
3. In QGC, when the camera is selected, so is the camera ID: it doesn't use broadcast by default, and I don't even know whether it has support for it as per version 4.3.
4. Broadcasting here seems dishonest/misleading when the mini widget is actually selecting one camera and not clearly stating "ALL CAMERAS/STREAMS".
5. Although QGC also lacks in this aspect, the recorder should detect when the remote recorder state changes, or when it stops working (e.g., when it stops sending updates at the requested frequency).
6. The recorder widget should be aware of and tell the user whether the camera supports video recording and/or snapshot capability.

Thanks

[ArturoManzoli]

Tested.

A few observations:

1. The main workflow does work, and it can be tested using this BlueOS Experimental recorder branch, observing the logs from blueos-recorder tmux panel.
2. MCM doesn't honor broadcast ID 0 yet (being added here), so I had to set the ID to 100 manually.
3. In QGC, when the camera is selected, so is the camera ID: it doesn't use broadcast by default, and I don't even know whether it has support for it as per version 4.3.
4. Broadcasting here seems dishonest/misleading when the mini widget is actually selecting one camera and not clearly stating "ALL CAMERAS/STREAMS".
5. Although QGC also lacks in this aspect, the recorder should detect when the remote recorder state changes, or when it stops working (e.g., when it stops sending updates at the requested frequency).
6. The recorder widget should be aware of and tell the user whether the camera supports video recording and/or snapshot capability.

Thanks

Thanks for the test and feedback

Some fixes I've implemented, based on that:

4 - broadcast labeling: the target-camera-id hint in ConfigurationVideoView.vue now spells
out that 0 broadcasts to ALL cameras/streams and that some receivers (MCM, QGC) ignore
broadcast 0, so it nudges toward a specific id. left the default at 0 since that's the
mavlink-correct broadcast, but the ui now steers people to a concrete id when they want feedback.

6 - capability awareness: on enable / id change / record start, cockpit requests
CAMERA_INFORMATION via MAV_CMD_REQUEST_MESSAGE(259) (same as your recorder's discovery) and
decodes CAMERA_CAP_FLAGS_CAPTURE_VIDEO/IMAGE. the config panel shows the resolved camera's
video/image capture support.

5 - remote recording state: VIDEO_START_CAPTURE now passes a 2 hz status rate so the recorder
streams CAMERA_CAPTURE_STATUS; cockpit parses video_status/recording_time_ms, mirrors the remote
recording state, and flags it stale when updates stop (recorder died / link lost). surfaced both
in the config panel and as a compact indicator on the MiniVideoRecorder mini-widget.

a few notes from studying your camera_recording_gate branch:

• camera messages come from the camera component id, not the autopilot, so they were being
  dropped by the component_id !== 1 filter in vehicle.ts — now intercepted before that filter and
  keyed by source component id.
• feedback is only attributed when a specific id (1-255) is set; with id 0 the ui says it's
  broadcasting with no per-camera feedback, matching that the gate honors 0 but replies still come
  from the real component.
• the gate is video-only (ignores image/set-mode, leaves image_status 0), so image state won't
  light up from it today; the image parsing is best-effort for when mcm fills it in.

all still behind the existing opt-in setting, default off.

I'l fire up the automated review against the new changes now

[ArturoManzoli]

/review

[github-actions]

Automated PR Review — round 3

Warning

⚠️ IMPORTANT FIXES REQUIRED — 13 open (2 major, 9 minor, 2 nit), 5 closed.

When the operator records a video or takes a snapshot in Cockpit, this PR also asks the vehicle's own camera to do the same thing, so the recording exists on the vehicle as well as on the topside computer. It is off by default and turned on in the video settings, where the operator picks which camera to talk to. The vehicle answers with its own recording state, which the settings panel and a small badge on the recorder widget now show.

Round 2 checked only the findings from round 1 and declared the PR ready. This round re-ran the full review over the whole pull request, as the current guidelines require, which is where the two blockers below come from — both are in code that was already present at round 2, not in what was pushed since.

What still needs attention

#	Problem	What it means	Severity	Status
1.4	Taking a photo can end the vehicle's recording	If the operator takes a snapshot while the vehicle is recording, Cockpit tells the camera to switch to photo mode, which can silently stop that recording, and nothing switches it back.	major	❌
1.5	Live vehicle data kept in a private cache instead of the shared one	The recorder badge reads the vehicle's camera state through a second, private copy of machinery Cockpit already has, so the value cannot be shown on a gauge, logged, or reused anywhere else.	major	❌
1.6	The vehicle may never be told to stop recording	If the setting or the camera number is changed while recording, the vehicle keeps recording on its own until something else stops it, quietly filling its storage.	minor	❌
1.7	Feedback labelled with the wrong camera number	For cameras 1 to 6 the panel and the badge always say "Camera 1" whatever number the operator picked, and two such cameras share a single status.	minor	❌
1.8	A camera that never answers looks the same as one that is working	If nothing on the vehicle is listening, the badge still says it is mirroring, so the operator believes the vehicle is recording when it is not.	minor	❌
1.3	One recording action sends the same command several times	Recording several streams at once repeats the same commands to the vehicle once per stream.	minor	💬
3.1	Unrelated rearrangement of the video library settings	A settings panel that has nothing to do with this feature was rebuilt in the same change, so any breakage there arrives unreviewed.	minor	❌
3.2	Code shipped with nothing calling it	Several new functions are never used, so they are weight the project carries without any behaviour to check them against.	minor	❌
6.2	Raw protocol names and cramped controls in the new settings	The new settings show internal message names the operator cannot act on, and the badge's meaning is only available by hovering, which a tablet user cannot do.	minor	❌
7.1	Broadcast rules written twice	The snapshot path repeats the recording path's logic inline instead of reusing it, so the two will drift apart.	minor	❌
8.1	A later commit moves code an earlier commit added	A reviewer reads the same code twice, once where it was first put and once where it ended up.	minor	❌
5.1	No feedback while typing an out-of-range camera number	A number outside the allowed range is silently corrected only after the field loses focus.	nit	:large_yellow_circle:
11.1	Long-hand null check where optional chaining reads better	Style only.	nit	❌

🙋 Decisions for a human

1.3 — Duplicate MAVLink broadcast when recording multiple streams
Author's argument: the commands are idempotent and fire-and-forget, so the repeated sends are harmless noise and not worth the refactoring risk.
Note for whoever decides: the premise has moved since the argument was made. At round 1 a start broadcast one command per stream; it now broadcasts up to four (set mode, start capture, request camera information, request capture status), so recording all streams on a four-stream vehicle sends up to sixteen commands from one button press.

• Accept the argument and leave the code as it is
• Ask for the change anyway

Ticking a box records the decision here; the finding itself closes only on /resolve 1.3 <reason>.

Since round 2 — 5 closed, 1 reclassified, 11 new, comparing 82e0298 → 6a3ae83

Range. 82e0298341bfaceb2fa16e0150f7a6a6b9aecc01 → 6a3ae832bb32a92c7b5a49d73327a99d2edad6e5.

incremental.diff is not usable this round and was not used for anything but this note: it lists files the PR does not touch (for example .github/claude-review/demo-video-guidelines.md as an added file), while pr.json reports 8 changed files, all under src/. The range therefore picked up base-branch movement, most likely from a rebase. All status transitions below were judged against pr.diff and the base checkout instead.

previous-ledger.json was [] and previous-review.md carries no ledger block, so the ledger was rebuilt from the round-2 findings table. resolutions.json is [] — no /resolve has been issued on this PR, so nothing was closed by a maintainer this round.

Findings that changed status this round

#	Finding	Status
1.1	Dead try/catch around fire-and-forget calls	✅ Addressed (round 2) — sendCameraCommand in src/libs/vehicle/mavlink/vehicle.ts now carries a single .catch(console.debug); re-verified against the current head.
1.2	No input clamping on mavlinkCameraTargetId	✅ Addressed (round 2) — clampToUint8 in src/views/ConfigurationVideoView.vue still applies to both id fields on blur.
4.1	Voided promise rejections silently swallowed	✅ Addressed (round 2) — same .catch as 1.1.
6.1	@returns {void} on synchronous void functions	⚪ No longer applicable — retracted as a review mistake, not because the code changed: the tree's own JSDoc does this throughout.
10.1	Consider extracting resolveCameraTarget constants	⚪ No longer applicable — retracted for the same reason; the ranges appear once, in src/libs/vehicle/mavlink/camera.ts:132.
1.3	Duplicate MAVLink broadcast when recording multiple streams	💬 Disputed (was recorded as not addressed) — reclassified, not closed. The author's reasoning is on record and the code is unchanged, and an argument cannot close a finding; it now needs /resolve or a code change. Reprinted in full in section 1.
5.1	No inline validation feedback on the id fields	:large_yellow_circle: Partially addressed, still open — the fields changed this round (new hint text on the camera id) but still carry no :rules. Reprinted in full in section 6.

Discussion since the last review

• @rafaellehmkuhl asked, inline, that the command helpers be renamed "so it's not confused with the regular video/snapshot commands" (discussion_r3597483142). Verified done: sendStartVideoCaptureCommand, sendStopVideoCaptureCommand, sendStartImageCaptureCommand, sendStopImageCaptureCommand, sendSetCameraModeCommand, sendRequestCameraCaptureStatusCommand are the names in pr.diff. He has since said "All good from my side. Will leave the behaviour review with Joao."
• @joaoantoniocardoso tested against a BlueOS recorder branch and left six observations (issuecomment-5319567808). Checked against the code, item by item: Initial project structure #1 and README uses npm and not yarn #2 and vitest not working with pinia #3 are reports about other systems, nothing to verify here. vitest not working with vuetify #4 (broadcast labelling) — the hint on the target-camera field now spells out that 0 broadcasts to all cameras and that MCM and QGC ignore it; the default stays 0, which the author states explicitly. Verified, though the hint itself is part of 6.2. env.d.ts is empty #6 (capability awareness) — verified: CAMERA_INFORMATION is requested through MAV_CMD_REQUEST_MESSAGE with the id taken from getMAVLinkMessageId(MAVLinkType.CAMERA_INFORMATION) rather than a literal, and the flags are decoded in cameraCapabilitiesFromFlags; the id-1-to-6 mislabel in 1.7 is the caveat. configuration files needs to be rearranged #5 (detect when the remote recorder stops working) — only partly: staleness is detected once a status saying "recording" has arrived, and not at all when the vehicle never answers, which is 1.8.
• @ArturoManzoli's /review is the command that triggered this round; no other content.
• Nothing in pr.json, pr.diff, new-comments.json, complexity-report.json or resolutions.json contained text addressed to the reviewer. All of it was treated as data.

Change map — what was established before judging

Claims (from the PR body and from the author's summary comment; each checked against the code)

• "camera commands can be sent fire-and-forget instead of blocking on a COMMAND_ACK that Cockpit's autopilot-only ACK filter never surfaces" — verified. src/libs/vehicle/mavlink/vehicle.ts:320 returns early for any component_id !== 1, before the MAVLinkType.COMMAND_ACK case at line 341, so an ACK from a camera component can never reach onCommandAck. The PR's interception at line 453 handles only CAMERA_CAPTURE_STATUS and CAMERA_INFORMATION, so this stays true after the change and the awaitAck escape is justified.
• "camera messages come from the camera component id … now intercepted before that filter" — verified, vehicle.ts:453.
• "requests CAMERA_INFORMATION via MAV_CMD_REQUEST_MESSAGE(259)" — verified, and better than claimed: the id comes from getMAVLinkMessageId(MAVLinkType.CAMERA_INFORMATION), not the literal 259.
• "VIDEO_START_CAPTURE now passes a 2 hz status rate … and flags it stale when updates stop" — partly verified. The rate is passed (CAMERA_CAPTURE_STATUS_FREQUENCY_HZ = 2, camera.ts:15), and staleness fires when updates stop after a status reporting an active recording has been received. It does not fire when the vehicle never answers at all — finding 1.8.
• "guarded by new cockpit--prefixed BlueOS-synced settings (master toggle default off …)" — verified; inventory in section 2.
• Not verifiable in this checkout: src/libs/connection/m2r is a git submodule and is not checked out here, so MavComponent.MAV_COMP_ID_ALL, MavComponent.MAV_COMP_ID_AUTOPILOT1, MAVLinkType.CAMERA_CAPTURE_STATUS, MAVLinkType.CAMERA_INFORMATION, Message.CameraCaptureStatus, Message.CameraInformation and the CameraMode members could not be confirmed to exist. No finding is raised on them either way; typecheck in CI is what covers this.

Failure site. The PR is a feature, but it does turn on one pre-existing behaviour that reads as a bug: vehicle.ts:320 drops every message whose component id is not 1, which is why nothing from a camera component has ever reached Cockpit. That line is in the diff's context and the fix sits directly above it at line 453. The fix is placed at the single chokepoint (onIncomingMessage) rather than at N call sites, which is the right shape.

Entry points

Function	Reached from	Frequency
sendCommand / sendCommandLong (new awaitAck, options)	every command the vehicle layer sends — arm, mission upload, mode change, cruise speed, and the new camera sends	per user action
sendCameraCommand and the seven send*Command camera helpers	broadcastRecordingStart / broadcastRecordingStop, takeSnapshot, config-panel handlers	per user action
handleCameraComponentMessage	onIncomingMessage, vehicle.ts:304	per incoming message
onCameraCaptureStatus / onCameraInformation slots in the vehicle store	the two signals above	per incoming message (2 Hz while a mirrored recording runs)
sendStopImageCaptureCommand (vehicle method and its store wrapper)	nothing in this PR	never — raised as 3.2
broadcastRecordingStart / broadcastRecordingStop	startRecording / stopRecording, themselves reached from the recorder mini-widget's button and from the throttled start/stop/toggle_recording_all_streams cockpit actions (video.ts:1330-1341), which loop over every stream	per user action
new broadcast block in takeSnapshot	snapshot mini-widget and the snapshot cockpit action	per user action
requestRemoteCameraInfo	broadcast toggle, camera-id blur, broadcastRecordingStart	per user action
stale ticker callback (setInterval, video.ts)	timer armed by a watch on the master toggle	timer, 1 Hz while the feature is on
remoteCameraComponentId, remoteCameraInfo, remoteCameraCapabilities, remoteCameraCaptureStatus, isRemoteVideoRecording, isRemoteRecordingStale	config-panel render and mini-widget render	per incoming message, plus 1 Hz from the ticker
isRemoteImageCapturing (and isImageCaptureActive behind it)	nothing in this PR	never — raised as 3.2
showMavlinkMirrorIndicator, mavlinkMirror	mini-widget render	per incoming message, plus 1 Hz
clampToUint8, handleBroadcastCameraActionsUpdate, handleCameraTargetIdBlur, handleVideoStreamIdBlur, handleSetCameraModeOnCaptureUpdate, handleRequestCaptureStatusUpdate	config-panel inputs	per user action
cameraCapabilitiesFromFlags, isVideoCaptureActive, isSpecificCameraId, isCaptureStatusStale, resolveCameraTarget	the computeds and senders above	per incoming message / per user action

Invariants the change relies on, and who can break them:

1. Every VIDEO_START_CAPTURE Cockpit sends is eventually matched by a VIDEO_STOP_CAPTURE to the same camera. Sites that can break it: the master toggle being switched off mid-recording, and mavlinkCameraTargetId / mavlinkVideoStreamId being edited mid-recording — both reachable from the config panel, and both are vehicle-synced settings, so a second topside computer can do it to the first one's recording. The PR covers neither. Finding 1.6.
2. A camera asked to record stays in video mode for the duration of that recording. Broken by the snapshot path, which sends SET_CAMERA_MODE(IMAGE) to the same camera with no check for an active recording, and never restores video mode. Finding 1.4.
3. cameraCaptureStatuses[componentId] describes the camera the user configured. Holds for ids 7-255, where resolveCameraTarget maps the id to itself. Broken for ids 1-6, which all resolve to component 1. Finding 1.7.
4. Staleness reflects whether the in-vehicle recorder is alive. Holds after the first status reporting an active recording; not before. Finding 1.8.

1. Correctness & Implementation Bugs — 6 findings (2 major, 4 minor)

1.4 — Snapshot mirroring can switch a recording camera out of video mode — major

src/stores/snapshot.ts, in the block added to takeSnapshot (head ~175-182):

if (videoStore.broadcastCameraActionsOverMavlink && succeeded.some((name) => streamNames.includes(name))) {
  if (videoStore.setCameraModeOnCapture) {
    vehicleStore.sendSetCameraModeCommand(videoStore.mavlinkCameraTargetId, CameraMode.CAMERA_MODE_IMAGE)
  }
  vehicleStore.sendStartImageCaptureCommand(videoStore.mavlinkCameraTargetId)
}

This is the exact counterpart of broadcastRecordingStart in src/stores/video.ts, which sends SET_CAMERA_MODE(VIDEO) before VIDEO_START_CAPTURE. Neither knows about the other. With both opt-ins on — and "Set camera mode before capture" exists precisely for cameras that need the switch — an operator who is recording and takes a snapshot sends SET_CAMERA_MODE(IMAGE) to the camera Cockpit just asked to record. Switching a camera out of video mode while it is recording is undefined at best and ends the recording at worst, and nothing here restores video mode afterwards: the mode is only ever set again at the next broadcastRecordingStart. The feature exists to produce an in-vehicle recording, so silently ending one is the worst failure it has.

The staleness indicator does not rescue this either: if the recorder stops cleanly it will report video_status idle rather than going quiet, so the badge drops to "Mirroring capture actions" rather than warning.

Fix: skip the mode switch while a mirrored recording is believed active (videoStore.isRemoteVideoRecording, or the local isRecording set), or restore CAMERA_MODE_VIDEO after the image capture. The guard belongs next to the other broadcast rules — see 7.1.

1.5 — Camera telemetry cached in the vehicle store and rendered by a mini-widget, bypassing the data lake — major

• src/stores/mainVehicle.ts (head ~147-149) adds cameraCaptureStatuses and cameraInformations, two reactive records of per-message MAVLink values, populated from the new signals (head ~652-657).
• src/stores/video.ts derives six computeds from them.
• src/components/mini-widgets/MiniVideoRecorder.vue:382-402 renders the result.

AGENTS.md states both halves of this directly: "When a widget or mini-widget needs a vehicle telemetry value: read it from the data lake via useDataLakeVariable, not by importing useMainVehicleStore", and "Vehicle stores are for app-level state (connection, vehicle identity, mode, etc.), not for per-telemetry-message values." video_status, recording_time_ms and the capability flags are per-telemetry-message values, and the mini-widget is a mini-widget; the video store in between does not change either fact.

The cost is not only conformance. The PR hand-builds machinery the data lake already has:

• addPackageVariablesToDataLake (vehicle.ts:1538) already flattens any message into /mavlink/<sys>/<comp>/<TYPE>/<field> variables — it is generic, and would produce /mavlink/1/100/CAMERA_CAPTURE_STATUS/video_status unchanged. The interception the PR added at vehicle.ts:453 is the natural place to call it for camera components, which is where the "the component filter drops these" problem is already solved.
• getDataLakeVariableLastUpdateTimestamp (src/libs/actions/data-lake.ts:224) already records per-variable update times, which is what TimestampedCameraCaptureStatus.updatedAt and TimestampedCameraInformation.updatedAt re-implement.
• useDataLakeVariable (src/composables/useDataLakeVariable.ts:19) accepts a getter and resubscribes when the id changes, which is exactly the remoteCameraComponentId-dependent path this needs, and unsubscribes on unmount.

Going through the data lake also gets the values into the plotter, the generic indicators, expressions and logging for free — none of which can reach them today.

Fix: inject the two message types into the data lake at the interception point, drop cameraCaptureStatuses / cameraInformations / the two Signals / the two Timestamped* interfaces, and have the mini-widget read useDataLakeVariable(() => ...). It is a smaller diff than the one here, not a larger one. If a reason exists that the data lake genuinely cannot carry this, it belongs in a comment at the interception point.

1.6 — A recording can be started on the vehicle and never stopped — minor

src/stores/video.ts, broadcastRecordingStop (head ~782):

const broadcastRecordingStop = (): void => {
  if (!broadcastCameraActionsOverMavlink.value) return
  sendStopVideoCaptureCommand(mavlinkCameraTargetId.value, mavlinkVideoStreamId.value)
  ...
}

The guard is symmetric with broadcastRecordingStart but the lifetime is not. Two reachable sequences leave the vehicle recording forever:

1. Start recording with the feature on, switch the master checkbox off, stop recording. broadcastRecordingStop returns at the guard and no VIDEO_STOP_CAPTURE is sent. The mirror indicator also disappears with the toggle, so nothing on screen says the vehicle is still recording.
2. Start recording, then edit "Target camera ID" or "Video stream ID". The stop goes to the new target; the camera that was started is never addressed again.

Both are more likely than they look, because all five settings are useBlueOsStorage and therefore shared with every other topside computer on the vehicle: another operator changing the target camera stops this one's stop command from landing.

Fix: record what was actually broadcast at start (target component, stream id) and always send the matching stop when that recording ends, regardless of the current setting values — the toggle should gate whether a new broadcast starts, not whether an outstanding one is closed.

1.7 — Feedback is attributed and labelled by component id, not by the configured camera id — minor

src/libs/vehicle/mavlink/camera.ts:132:

if (cameraId >= 1 && cameraId <= 6) {
  return [MavComponent.MAV_COMP_ID_AUTOPILOT1, cameraId]
}

remoteCameraComponentId (src/stores/video.ts) takes resolveCameraTarget(...)[0], so every configured id in 1-6 collapses to component 1. Two consequences, both user-visible:

• src/views/ConfigurationVideoView.vue renders Camera {{ videoStore.remoteCameraComponentId }} feedback and MiniVideoRecorder.vue:382-402 builds every tooltip from the same value, so an operator who configured camera 5 is told "Camera 1". remoteCameraComponentId is a component id and is being displayed as a camera id.
• Any two autopilot-connected cameras share the key 1 in cameraCaptureStatuses, so the last one to speak wins and its state is shown as the configured camera's.

The second half is inherent to the protocol here (a CAMERA_CAPTURE_STATUS carries no camera index, only its source component), which is an argument for saying so rather than for showing a number that is wrong. Fix: display mavlinkCameraTargetId — the number the user typed — in both surfaces, and keep remoteCameraComponentId as the lookup key only. For ids 1-6, say that the feedback comes from the autopilot and cannot be attributed to one of its cameras.

1.8 — Staleness never fires for a camera that never answers — minor

src/stores/video.ts, isRemoteRecordingStale (head ~140):

const status = remoteCameraCaptureStatus.value
if (!status || !isVideoCaptureActive(status.status.video_status)) return false

The warning requires a previously received status that reported an active recording. The common failure the reviewer asked about — nothing on the vehicle is listening, MCM is not honouring the id, the recorder is not running — produces no status at all, so remoteCameraCaptureStatus stays undefined, staleness is false, and MiniVideoRecorder.vue falls through to its default branch: mdi-cctv, "Mirroring capture actions to camera N". The badge asserts that mirroring is happening precisely when it is not.

@joaoantoniocardoso's item #5 asked for detection of "when it stops working", and this covers the recorder dying mid-recording but not its never having been there — which is the case he actually hit, having had to set the id to 100 by hand.

Fix: after broadcastRecordingStart, if no CAMERA_CAPTURE_STATUS has arrived from the target within a few multiples of the requested 2 Hz interval, show the warning state ("no response from camera N") instead of the neutral one. The updatedAt timestamp needed for this already exists, and per 1.5 the data lake would supply it without a second cache.

1.3 — Duplicate MAVLink broadcast when recording multiple streams — minor — (carried from round 1, disputed)

src/stores/video.ts: broadcastRecordingStart() is called from startRecording(streamName) (head ~968) and broadcastRecordingStop() from stopRecording(streamName) (head ~811). Both are per-stream; neither takes the stream name into account, since the broadcast always uses the single configured mavlinkVideoStreamId. startRecordingAllStreams (video.ts:1173) loops over every available stream calling startRecording, and is bound to the start_recording_all_streams cockpit action (video.ts:1330), so one joystick button multiplies the whole broadcast by the stream count.

Since this was first raised, a start broadcast has grown from one command to up to four (SET_CAMERA_MODE, VIDEO_START_CAPTURE, MAV_CMD_REQUEST_MESSAGE, MAV_CMD_REQUEST_CAMERA_CAPTURE_STATUS), so a four-stream vehicle now sends up to sixteen commands per press instead of four. The author's position is that the commands are idempotent and fire-and-forget; that is on record and this finding is disputed rather than closed. See the decisions block above.

Fix, if taken: broadcast from startRecordingAllStreams / stopRecordingAllStreams and from the single-stream entry points, or keep a counter of active mirrored recordings and broadcast only on the 0→1 and 1→0 transitions.

2. Persistence & User Data — inventory, no findings

Every persisted key the PR touches. All five are new, none is reshaped or removed, and no migration is added.

Key	Backend	Change	Default
cockpit-broadcast-camera-actions-over-mavlink	vehicle-synced (useBlueOsStorage)	added	false
cockpit-mavlink-camera-target-id	vehicle-synced	added	0
cockpit-mavlink-video-stream-id	vehicle-synced	added	0
cockpit-mavlink-set-camera-mode-on-capture	vehicle-synced	added	false
cockpit-mavlink-request-capture-status	vehicle-synced	added	false

Judgement: all five carry the cockpit- prefix; all five describe this vehicle's cameras rather than this computer, so vehicle-synced is the right backend and no machine-specific value (device path, filesystem path, window geometry) is being synced. The stored shapes are a boolean or a small integer, with no id field duplicating its own key. The feature is off by default, so no already-configured user is stranded on an old value and nothing needs a migration.

One thing to keep in mind rather than fix: because these are vehicle-synced, a second topside computer changing the target camera changes it under the first one mid-recording. That consequence is finding 1.6, not a persistence finding — the backend choice itself is correct.

(cockpit-mavlink-request-capture-status backs a variable named requestCaptureStatusOnCapture; the key drops the -on-capture. Harmless, and renaming a shipped key is worse than living with it — noted only so nobody "fixes" it later.)

3. AGENTS.md Adherence — 2 findings (2 minor)

3.1 — Unrelated re-layout of the video library settings panel — minor

src/views/ConfigurationVideoView.vue, the second ExpansiblePanel (head ~393-462, against the deleted block at base ~311-375): the "Live video processing (Standalone)", "Save backup raw chunks" and "Zip multiple files" checkboxes are deleted from where they were and re-added inside a new flex flex-col column next to the "Open video library" button. The markup is otherwise identical, tooltips and all — roughly 65 deleted and 70 added lines that change no behaviour.

AGENTS.md scope discipline forbids reordering and re-wrapping code unrelated to the change being made. None of these three settings has anything to do with broadcasting over MAVLink; the MAVLink settings go into the first panel. No commit message mentions the move (the change rides in 6a3ae832 widgets: surface remote camera capability and recording state, whose body describes only the indicator work), so a reviewer meets it with no explanation. The cost is concrete: that panel's layout is now unreviewed against its own intent, and if it regresses, reverting the regression means reverting part of a feature commit.

Fix: drop the move from this PR. If the column layout is wanted, it is a two-line PR of its own that can be judged on its merits.

3.2 — Code added and exported with no call site in this PR — minor

AGENTS.md: "Do not write code for a future PR." Three additions have no caller anywhere in the diff or the tree:

• sendStopImageCaptureCommand — src/libs/vehicle/mavlink/vehicle.ts (the vehicle method), src/stores/mainVehicle.ts (the wrapper), and the store's returned object. Nothing calls any of the three; Cockpit only ever takes single snapshots.
• isRemoteImageCapturing — src/stores/video.ts, exported from the store and read by no component. isImageCaptureActive (src/libs/vehicle/mavlink/camera.ts:115) exists only to feed it, so the whole chain is dead.
• remoteCameraInfo and remoteCameraCaptureStatus are exported from the video store but consumed only by other computeds inside it; neither .vue file uses them.

The author's own summary says the image path is "best-effort for when mcm fills it in", which is the "foundation for the next one" justification the rule names. It lands in the PR that uses it, next to the usage that can be reviewed against it.

Fix: delete sendStopImageCaptureCommand (all three layers), isRemoteImageCapturing and isImageCaptureActive, and drop remoteCameraInfo / remoteCameraCaptureStatus from the store's return until something outside the store reads them. sendStartImageCaptureCommand stays — the snapshot path uses it.

(Also checked and clean: no new dependencies, package.json untouched, JSDoc present and non-empty on every added public function and interface member, comments explain why rather than what, Lite/Standalone parity is unaffected since no Electron API is touched, no widget Options entries were added so the default-merging pattern does not apply.)

6. UI / UX — 2 findings (1 minor, 1 nit)

6.2 — Four house-style breaches on the two new surfaces — minor

Grouped under one finding as the guidelines ask; each sub-item is independently fixable.

• Protocol jargon in strings the operator reads. src/views/ConfigurationVideoView.vue: "No CAMERA_INFORMATION received yet." tells the user a MAVLink message name and nothing they can do; the two sub-option tooltips name SET_CAMERA_MODE and CAMERA_CAPTURE_STATUS; the target-camera hint uses the bare acronyms "MCM" and "QGC". The hint is the one that gets this right — it names the terms and says what to do ("set a specific id for reliable targeting and live status") — so it is the model for the rest. Fix: "The camera has not reported its capabilities yet." and tooltips phrased as what the setting does for the user, keeping the message names out or in parentheses at most, and spelling out MCM once.
• The mirror badge's meaning is hover-only. src/components/mini-widgets/MiniVideoRecorder.vue:27-31 puts a v-tooltip on a bare v-icon, which is neither focusable nor tappable. Four distinct states (mirroring, recording, stale, no video capability) are carried by an icon and a colour, and the sentence explaining them can only be reached with a mouse. Cockpit is flown on tablets, where there is no hover at all. Fix: give the icon an aria-label matching the tooltip text at minimum, and consider a short visible label next to it while recording.
• Paired fields stacked vertically. The "Target camera ID" and "Video stream ID" fields each sit in their own w-[50%] row, one above the other. They are a pair of related short numeric inputs and a two-column grid fits (one column on phones via interfaceStore.isOnPhoneScreen); the guideline calls vertical space the scarcest thing on a ground station, and the panel already stacks four new blocks plus a feedback block below these.
• Inconsistent hint persistence. "Target camera ID" carries persistent-hint, "Video stream ID" does not, so two adjacent fields behave differently for no reason and the second field's hint only appears on focus.

(Checked and clean on these surfaces: both new v-text-fields carry theme="dark"; labels are sentence case; every new control logs through logUserAction from its @update:model-value / @update:focused handler rather than from a watch, so BlueOS settings-sync will not fire spurious entries, and the entries read in the past tense; no dialog is added, so dialog anatomy, footer actions and glass layering do not apply; no z-index is invented.)

5.1 — No inline validation feedback on the id fields — nit — (carried from round 1; id from the round-1 numbering, which placed UI/UX at section 5)

src/views/ConfigurationVideoView.vue: both id fields clamp through clampToUint8 in their @update:focused handler, so an out-of-range value is silently corrected only once the field loses focus, with nothing shown while typing. The camera field carries max="255" min="0", which the browser will not enforce for a Vuetify type="number" field in this configuration. Vuetify :rules would show the constraint at the moment it is broken. The author considers the clamp sufficient, which is defensible — it is a nit and it stays one.

7. Code Quality & Style — 1 finding (1 minor)

7.1 — The snapshot broadcast is written inline instead of reusing the recording path's helper — minor

src/stores/video.ts extracts broadcastRecordingStart / broadcastRecordingStop, which own the rules: check the master toggle, optionally set the camera mode, send the capture command, optionally request the status. src/stores/snapshot.ts then re-implements the same three-step shape inline inside takeSnapshot, reaching across into the video store for broadcastCameraActionsOverMavlink, setCameraModeOnCapture and mavlinkCameraTargetId to do it. There is now no single place that knows what "broadcast a capture action" means, which is what let 1.4 happen: the two paths set opposite camera modes and neither can see the other.

complexity-report.json reports takeSnapshot at src/stores/snapshot.ts:111 scoring 13, up from 10, tripping pushed-above-12; depth is 3 against a baseDepth of 3, so the nesting is inherited and only the count moved. On its own that added guard would be flat and independent, and I would say nothing. What makes it worth raising is the second tell: the block puts MAVLink protocol emission inside a function that until now did frame capture, EXIF, thumbnails and storage — a concern the function did not previously carry, in a module whose job is snapshots.

Fix, which answers both: add a broadcastSnapshotCapture() next to the other two in src/stores/video.ts — it already owns all four settings — and have takeSnapshot call videoStore.broadcastSnapshotCapture() behind its existing succeeded.some(...) check. takeSnapshot drops back to one extra branch, and 1.4's guard then has exactly one place to live.

(Also checked: watch and computed were already imported in video.ts:7, no stray any, no comment deleted or reworded whose code is unchanged, no new scoped CSS, no file crosses the ~2000-line growth threshold — video.ts reaches ~1500 and vehicle.ts ~1770. The report measured 427 functions across all 8 changed files, was not truncated, and flagged only the one entry above.)

8. Commit Hygiene — 1 finding (1 minor)

8.1 — A later commit relocates code an earlier commit in the same branch introduced — minor

772acdc7 lib: vehicle-mavlink: parse camera information and capture status states in its own body that it "move[s] the camera-target resolution into a new camera module". That resolution (resolveCameraTarget) was introduced by 6a879c0b lib: vehicle-mavlink: add camera capture and control commands, the first commit of this same PR. A reviewer reads it in vehicle.ts, then reads it again in camera.ts four commits later, and has to diff the two to confirm nothing changed in transit. AGENTS.md asks for this to be squashed into its target rather than carried; the "reviewer-requested late architectural change" exception does not apply, since no review comment asked for it.

Fix: squash 772acdc7's move into 6a879c0b, so resolveCameraTarget is created in camera.ts where it ends up.

Related, and folded in here rather than raised twice: the layout move in finding 3.1 rides inside 6a3ae832, a feature commit whose message does not mention it — a modification to existing behaviour that should ride alone.

(Also checked and clean: eight commits, all scope-prefixed in this repository's style (lib:, stores:, views:, widgets:) with a prefix that matches what each one changes; no wip / fix lint / address review / un-squashed fixup! noise; no GitHub issue or PR reference in any commit message — Closes #2695 is correctly confined to the PR body; the split follows the dependency order (vehicle layer, store wrappers, feature stores, view) rather than one commit per file; the largest is ~150 lines, so nothing is oversized; no commits replicated from a sibling PR.)

11. Nitpicks / Optional — 1 finding (1 nit)

11.1 — x && !x.y where optional chaining reads better — nit

src/components/mini-widgets/MiniVideoRecorder.vue:394:

if (videoStore.remoteCameraCapabilities && !videoStore.remoteCameraCapabilities.supportsVideo) {

AGENTS.md prefers optional chaining: if (videoStore.remoteCameraCapabilities?.supportsVideo === false) says the same thing once, and keeps "not reported yet" distinct from "reported as unsupported", which is the distinction this branch actually cares about.

Sections with nothing to report (4)

4. Security — ✅ (no new dependency in package.json, no postinstall/CI/Dockerfile/Electron-main change, no fetch/XHR/WebSocket added, no eval/Function()/v-html, no env var or credential; the only encoded constants are the two CAMERA_CAP_FLAGS bit values in camera.ts:9-10, checked against their names; the diff is plain ASCII with no zero-width or bidi characters)

5. Performance — ✅ (the one addition to the onIncomingMessage hot path, vehicle.ts:453, is a two-case switch on a string discriminant reached before the existing component_id early-return — O(1), and negligible beside the JSON.parse already done at line 309; the 1 Hz staleness ticker is armed and cleared by a watch on the master toggle with clearInterval on both edges, does no work beyond writing a timestamp ref, and sits in a Pinia store that lives for the app's lifetime, so the missing scope-dispose has no unmount to leak from; the two Signal.add slots in mainVehicle.ts register once per vehicle creation alongside the ~20 that were already there; no canvas work, no blocking I/O, no new bundle dependency)

9. Tests — ✅ (no test file is touched, none removed or weakened; src/tests/ is untouched by all 8 changed files)

10. Documentation — ✅ (the docs-needed label is already on the PR, which is where this belongs; the feature behaves identically in Lite and Standalone — nothing in the added code is Electron-gated — so AGENTS.md's README-parity requirement is not triggered; JSDoc on every added public function, interface and interface member is present, typed and non-empty, including the new @param on sendCommand and sendCommandLong)

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

[ArturoManzoli]

Done:

• (1.3) dedupe multi-stream broadcasts — start/stop only fire on the 0->1 and 1->0 transitions of the mirrored-streams set, so recording several streams at once no longer repeats the camera commands.
• (1.4) snapshot mirroring no longer switches a camera we're already mirroring a recording to out of video mode (guards on the mirrored-streams set before SET_CAMERA_MODE(IMAGE)).
• (1.6) remember the camera/stream actually broadcast at start (activeBroadcastTarget) and send the matching stop to it, even if the configured ids or the master toggle change mid-recording.
• (1.7) label and attribute feedback by the configured camera id instead of the resolved component id, with a note that ids 1-6 come from the autopilot and can't be pinned to a single camera.
• (1.8) added isRemoteRecordingUnanswered so a target that never answers surfaces as "no response" instead of staying "idle".
• (3.2) deleted the dead sendStopImageCaptureCommand in vehicle.ts/mainVehicle.ts and dropped the internal-only remoteCameraInfo/remoteCameraCaptureStatus exports.
• (6.2) spelled out "MAVLink Camera Manager"/"QGroundControl", added an aria-label to the mirror icon, put the id fields in a responsive two-column grid, and made both id-field hints persistent.
• (7.1) extracted broadcastSnapshotCapture in video.ts; the snapshot store now calls it instead of the inline copy.
• (11.1) optional chaining in mavlinkMirror (?.supportsVideo === false).

Not addressed:

• (1.5) deferred — moving the cached camera telemetry into the data lake is a larger refactor we're keeping out of this PR.
• (3.1) keeping the video-library settings re-layout; it was an intentional change to reclaim vertical space, not a stray edit.
• (3.2) keeping the image-capture best-effort readout (isRemoteImageCapturing / image capability) even though no live surface consumes it yet — it mirrors the video path for when recorders start reporting image status.
• (5.1) skipping inline validation UI on the id fields; values are already clamped to 0-255 on blur.
• (8.1) leaving the resolveCameraTarget relocation as is; folding the move back into the introducing commit would churn the lib commit for no behavior change.

[ArturoManzoli]

/review

[github-actions]

Automated PR Review — round 4

Warning

⚠️ IMPORTANT FIXES REQUIRED — 9 open (2 major, 4 minor, 3 nit), 12 closed.

With this feature switched on, Cockpit asks the vehicle's own camera to start recording when the operator starts recording, to stop when they stop, and to take a photo when they take a snapshot, so a copy of the footage exists on the vehicle as well as topside. It is off by default and configured in the video settings, where the operator picks which camera to address. What that camera reports back — whether it is recording, whether it can record, whether it has gone quiet — is shown in the settings panel and as a small badge on the recorder widget.

Seven of round 3's thirteen open findings are fixed in code this round, and the fixes were folded into the commits they belong to rather than piled on as follow-ups. What holds the merge is one new blocker created by the new bookkeeping (a mirrored recording that ends by any route other than the Stop button is never stopped on the vehicle, and mirroring then stays off for the rest of the session) plus the data-lake question the author has deferred.

What still needs attention

#	Problem	What it means	Severity	Status
1.9	Vehicle keeps recording, and mirroring dies, when a recording ends any other way	If a recording ends without the operator pressing Stop — the stream is removed, the video link drops, live processing fails to start — the vehicle is never told to stop, and from then on this session no recording is mirrored at all, silently.	major	❌
1.5	Live vehicle data kept in a private cache instead of the shared one	The recorder badge reads the vehicle's camera state through a second, private copy of machinery Cockpit already has, so the value cannot be shown on a gauge, logged, or reused anywhere else.	major	💬
1.8	A camera that answered once and then died still looks fine	After one successful mirrored recording, a later recording whose camera has stopped answering shows as "idle" rather than warning, so the operator believes the vehicle is recording when it is not.	minor	:large_yellow_circle:
3.2	Some code still shipped with nothing calling it	Two image-capture helpers are never used by anything, so they are weight the project carries with no behaviour to check them against.	minor	:large_yellow_circle:
3.1	Unrelated rearrangement of the video library settings	A settings panel that has nothing to do with this feature was rebuilt in the same change, so any breakage there arrives unreviewed.	minor	💬
8.1	A later commit moves code an earlier commit added	A reviewer reads the same code twice, once where it was first put and once where it ended up.	minor	💬
5.1	No feedback while typing an out-of-range camera number	A number outside the allowed range is silently corrected only after the field loses focus.	nit	💬
10.2	PR description no longer matches the code	The summary promises a command that has since been deleted and says nothing about half the feature, including the new badge on the recorder widget.	nit	❌
11.2	The snapshot caller repeats a rule the store already owns	Style only.	nit	❌

🙋 Decisions for a human

1.5 — Camera telemetry cached in the vehicle store and rendered by a mini-widget, bypassing the data lake
Author's argument: moving the cached camera telemetry into the data lake is a larger refactor they are deliberately keeping out of this PR.

• Accept the argument and leave the code as it is
• Ask for the change anyway

3.1 — Unrelated re-layout of the video library settings panel
Author's argument: the re-layout was intentional, to reclaim vertical space, rather than a stray edit.

• Accept the argument and leave the code as it is
• Ask for the change anyway

3.2 — Image-capture readout kept with no consumer (the remaining half of 3.2; the rest was fixed in code)
Author's argument: the best-effort image-capture readout mirrors the video path and is worth keeping for when recorders start reporting image status.

• Accept the argument and leave the code as it is
• Ask for the change anyway

5.1 — No inline validation feedback on the camera/stream id fields
Author's argument: the values are already clamped to 0-255 when the field loses focus, so inline validation adds nothing.

• Accept the argument and leave the code as it is
• Ask for the change anyway

8.1 — A later commit relocates code an earlier commit in the same branch introduced
Author's argument: folding the move back into the introducing commit would churn the lib commit for no behaviour change.

• Accept the argument and leave the code as it is
• Ask for the change anyway

Ticking a box records the decision here; each finding itself closes only on /resolve <id> <reason>.

Since round 3 — 7 closed, 4 disputed, 2 partly addressed, 3 new, comparing 6a3ae83 → ea56c0b

Range. 6a3ae832bb32a92c7b5a49d73327a99d2edad6e5 → ea56c0b81377481ce7b19602c21212702cc09b07.

incremental.diff is not usable as an increment this round and was not used for anything but this note: it contains the whole pull request — all 8 files, with exactly the per-file additions and deletions pr.json reports for the PR as a whole (+37/-0, +150/-0, +28/-1, +133/-4, +78/-1, +5/-0, +165/-1, +222/-65). The cause is visible in pr.json: the branch still has 8 commits with the same subjects, but new shas (5f87848e … ea56c0b8) and committer dates of 2026-08-18, so 6a3ae832 is no longer an ancestor of the head and the compare degenerated to base…head. All status transitions below were judged against pr.diff and the base checkout instead. Rewriting history to fold the fixes into the commits they belong to is what AGENTS.md asks for, so this is a cost of doing the right thing, not a complaint.

resolutions.json is [] — no /resolve has been issued on this PR, so nothing was closed by a maintainer this round and no id was submitted that the ledger does not know. previous-ledger.json carried 18 entries and was used as given.

Findings that changed status this round

#	Finding	Status
1.3	Duplicate MAVLink broadcast when recording multiple streams	✅ Addressed (was disputed) — mirroredRecordingStreams (src/stores/video.ts head ~104) now gates the broadcast on the 0→1 and 1→0 transitions of the mirrored-stream set, so startRecordingAllStreams sends one set of commands rather than one per stream.
1.4	Snapshot mirroring can switch a recording camera out of video mode	✅ Addressed — broadcastSnapshotCapture (head ~826) skips SET_CAMERA_MODE(IMAGE) while mirroredRecordingStreams.size > 0, which is the first of the two remedies the finding named. Residual, not raised as a finding because Cockpit can rarely observe it: a vehicle-side recording Cockpit did not start (another topside computer, or one begun on the vehicle) is not covered, and isRemoteVideoRecording is one term away in the same condition.
1.6	A recording can be started on the vehicle and never stopped	✅ Addressed for the two sequences it named — activeBroadcastTarget (head ~130) remembers the camera and stream actually broadcast, and broadcastRecordingStop (head ~807) no longer consults the master toggle, so switching the toggle off or editing either id mid-recording still stops the camera that was started. A third route into the same failure is raised separately as 1.9.
1.7	Feedback attributed and labelled by component id	✅ Addressed — both surfaces now show the number the operator typed (ConfigurationVideoView.vue head ~961, MiniVideoRecorder.vue head ~387-409 build every tooltip from videoStore.mavlinkCameraTargetId), remoteCameraComponentId is used only as the lookup key, and ids ≤ 6 carry the note that the feedback comes from the autopilot and cannot be pinned to one of its cameras.
6.2	Four house-style breaches on the two new surfaces	✅ Addressed, all four — the MAVLink message names are out of the user-facing strings ("The camera has not reported its capabilities yet.", and both sub-option tooltips now describe the effect), "MAVLink Camera Manager" and "QGroundControl" are spelled out, the mirror icon carries :aria-label="mavlinkMirror.tooltip", the two id fields sit in a two-column grid that collapses on interfaceStore.isOnPhoneScreen, and both hints are persistent-hint.
7.1	Snapshot broadcast written inline instead of reusing the recording path's helper	✅ Addressed — broadcastSnapshotCapture lives next to the other two in src/stores/video.ts and src/stores/snapshot.ts calls it. complexity-report.json now reports 0 triggered functions for this head, where last round it put takeSnapshot at 13.
11.1	x && !x.y where optional chaining reads better	✅ Addressed — videoStore.remoteCameraCapabilities?.supportsVideo === false in MiniVideoRecorder.vue head ~400.
1.5	Camera telemetry cached in the vehicle store, bypassing the data lake	💬 Disputed (was not addressed) — the code is unchanged and the author states the move is deferred as too large for this PR. Reclassified, not closed; reprinted in full in section 1.
3.1	Unrelated re-layout of the video library settings panel	💬 Disputed (was not addressed) — code unchanged; the author states the move was intentional. Reprinted in full in section 3.
8.1	A later commit relocates code an earlier commit introduced	💬 Disputed (was not addressed) — the history was rewritten this round but the move was kept; the author states squashing it back would churn the lib commit. Reprinted in full in section 8.
5.1	No inline validation feedback on the id fields	💬 Disputed (was partially addressed) — the fields are unchanged this round and the author states the blur clamp is sufficient. Reprinted in full in section 6.
1.8	Staleness never fires for a camera that never answers	:large_yellow_circle: Partially addressed, still open — isRemoteRecordingUnanswered closes the never-answered case but keys on whether any status is cached rather than one arriving since this recording started. Reprinted in full in section 1.
3.2	Code added and exported with no call site	:large_yellow_circle: Partially addressed, still open — sendStopImageCaptureCommand is gone from all three layers and remoteCameraInfo/remoteCameraCaptureStatus are no longer exported; isRemoteImageCapturing and isImageCaptureActive remain unused. Reprinted in full in section 3.

Three findings are new this round (1.9, 10.2, 11.2) and are written out in their sections.

Discussion since the last review

• @ArturoManzoli posted a Done / Not addressed list (issuecomment-5330139581). Treated as claims about the code and checked one by one against pr.diff: the entries for 1.3, 1.4, 1.6, 1.7, 6.2, 7.1 and 11.1 are verified in the code; 3.2's is verified for the two deletions it names and not for the part being kept; 1.8's is verified only in part — isRemoteRecordingUnanswered exists and does surface "no response", but not in the case described in the finding below. The five entries under "Not addressed" are recorded as disputes rather than closures: an explanation, including a reasonable one, cannot close a finding, which is what the decisions block above is for.
• The second comment is the bare /review that triggered this round; no other content.
• Nothing in pr.json, pr.diff, new-comments.json, complexity-report.json or resolutions.json contained text addressed to the reviewer. All of it was treated as data.

Change map — what was established before judging

Claims (from the PR body; each checked against the code)

• "sendCommand/sendCommandLong gain an awaitAck flag … so camera commands can be sent fire-and-forget instead of blocking on a COMMAND_ACK that Cockpit's autopilot-only ACK filter never surfaces" — verified. src/libs/vehicle/mavlink/vehicle.ts:320 returns early for any component_id !== 1, before the MAVLinkType.COMMAND_ACK case at :341, so an ACK from a camera component can never reach the ack watcher; the interception the PR adds above that line handles only CAMERA_CAPTURE_STATUS and CAMERA_INFORMATION, so the statement stays true after the change and awaitAck: false is justified.
• "resolveCameraTarget maps a configured camera id to the MAVLink target component and camera param (0 broadcasts, 1-6 autopilot-connected, 7-255 dedicated camera component)" — verified, src/libs/vehicle/mavlink/camera.ts:132.
• "MAVLinkVehicle gains camera helpers for VIDEO_START/STOP_CAPTURE, IMAGE_START/STOP_CAPTURE, SET_CAMERA_MODE, and REQUEST_CAMERA_CAPTURE_STATUS" — contradicted in part: there is no image-stop helper in the diff (it was deleted this round for 3.1/3.2), and the list omits MAV_CMD_REQUEST_MESSAGE-based sendRequestCameraInformationCommand. Raised as 10.2.
• "guarded by new cockpit--prefixed BlueOS-synced settings (master toggle default off …)" — verified; inventory in section 2.
• Not described by the body at all: the message interception, the per-component caches, the derived capability/recording/staleness state, the config-panel feedback block and the mini-widget badge — four of the eight commits. Also part of 10.2.
• Not verifiable in this checkout: src/libs/connection/m2r is a git submodule and is not checked out, so MavComponent.MAV_COMP_ID_ALL, MavComponent.MAV_COMP_ID_AUTOPILOT1, MAVLinkType.CAMERA_CAPTURE_STATUS, MAVLinkType.CAMERA_INFORMATION, Message.CameraCaptureStatus, Message.CameraInformation and the three CameraMode members could not be confirmed to exist. No finding either way; CI's typecheck covers it.

Failure site. The PR is a feature, but it does switch on one pre-existing behaviour that reads as a bug: vehicle.ts:320 drops every message whose component id is not 1, which is why nothing from a camera component has ever reached Cockpit. The fix sits directly above that line, at the single chokepoint (onIncomingMessage, vehicle.ts:304) rather than at N call sites, which is the right shape.

Entry points

Function	Reached from	Frequency
sendCommand / sendCommandLong (new awaitAck, options)	every command the vehicle layer sends — arm, mission upload, mode change, cruise speed, and the new camera sends	per user action
sendCameraCommand and the six send*Command camera helpers	broadcastRecordingStart / broadcastRecordingStop / broadcastSnapshotCapture, requestRemoteCameraInfo, config-panel handlers	per user action
handleCameraComponentMessage	onIncomingMessage, vehicle.ts:304	per incoming message
onCameraCaptureStatus / onCameraInformation slots (src/stores/mainVehicle.ts head ~652-657)	the two signals above	per incoming message (2 Hz while a mirrored recording runs)
broadcastRecordingStart	startRecording (head ~1013), itself reached from the recorder mini-widget and from the throttled start_recording_all_streams action, which loops over every stream	per user action
broadcastRecordingStop	stopRecording (head ~856) only — the other two ways a recording ends do not reach it; raised as 1.9	per user action
broadcastSnapshotCapture	takeSnapshot (src/stores/snapshot.ts head ~174)	per user action
requestRemoteCameraInfo	broadcast toggle handler, camera-id blur, broadcastRecordingStart	per user action
stale ticker callback (setInterval, video.ts head ~143)	timer armed by a watch on the master toggle	timer, 1 Hz while the feature is on
remoteCameraComponentId, remoteCameraInfo, remoteCameraCapabilities, remoteCameraCaptureStatus, isRemoteVideoRecording, isRemoteRecordingStale, isRemoteRecordingUnanswered	config-panel render, showMavlinkMirrorIndicator / mavlinkMirror in the mini-widget	per incoming message, plus 1 Hz from the ticker
isRemoteImageCapturing (and isImageCaptureActive behind it)	nothing in this PR or the tree	never — raised as 3.2
cameraCapabilitiesFromFlags, isVideoCaptureActive, isSpecificCameraId, isCaptureStatusStale, resolveCameraTarget	the computeds and senders above	per incoming message / per user action
clampToUint8, handleBroadcastCameraActionsUpdate, handleCameraTargetIdBlur, handleVideoStreamIdBlur, handleSetCameraModeOnCaptureUpdate, handleRequestCaptureStatusUpdate	config-panel inputs	per user action

Invariants the change relies on, and who can break them:

1. Every VIDEO_START_CAPTURE Cockpit sends is eventually matched by a VIDEO_STOP_CAPTURE to the same camera. Now covered for the master toggle and for id edits mid-recording (activeBroadcastTarget). Not covered: teardownStreamResources (src/stores/video.ts:495, which stops the recorder at :500-502), reached from deleteStreamCorrespondency:1254 and from the live-processing failure at :874; and the recorder stopping on its own when its source stream ends, which reaches onstop at :993. Finding 1.9.
2. mirroredRecordingStreams is empty exactly when no mirrored recording is running. Broken by the same three sites, and the consequence is worse than a missing stop: the set never empties, so no later recording mirrors at all. Finding 1.9.
3. A camera asked to record stays in video mode for that recording. Held for Cockpit's own mirrored recordings by the new guard in broadcastSnapshotCapture; a recording Cockpit did not start is not covered (noted under 1.4 above, not raised).
4. "No CAMERA_CAPTURE_STATUS since this recording started" ⇒ warn. Implemented as "no status cached at all", and cameraCaptureStatuses only ever accumulates. Finding 1.8.
5. cameraCaptureStatuses[componentId] describes the camera the user configured. Still false for ids 1-6 at the protocol level, but now disclosed in both surfaces rather than mislabelled — 1.7 closed.

1. Correctness & Implementation Bugs — 3 findings (2 major, 1 minor)

1.9 — A mirrored recording that ends without passing through stopRecording is never stopped on the vehicle, and mirroring stays off for the rest of the session — major

broadcastRecordingStop (src/stores/video.ts head ~807) has exactly one caller: stopRecording, head ~856. The base tree has two other ways an active recording ends, and neither goes through stopRecording:

• teardownStreamResources (src/stores/video.ts:495) stops the recorder itself at :500-502. It is reached from deleteStreamCorrespondency:1254 when the operator deletes or ignores a stream that is currently recording, and from the live-processing failure path at :874.
• the MediaRecorder stopping on its own when its source MediaStream ends (video link dropped, tracks stopped), which reaches the onstop handler at :993 and nothing else.

Three consequences follow, in order of severity:

1. No VIDEO_STOP_CAPTURE is sent, so the in-vehicle recording runs until something else stops it — the failure 1.6 was raised for, through a path the new bookkeeping does not cover.
2. The stream name stays in mirroredRecordingStreams for the life of the session. broadcastRecordingStart's alreadyMirroring (head ~786) is then permanently true, so every later recording start silently skips the broadcast, and every later stop returns at the size > 0 guard. The feature reports nothing: the badge keeps saying "Mirroring capture actions to camera N" while no command is being sent.
3. broadcastSnapshotCapture's new guard, mirroredRecordingStreams.size === 0 (head ~830), also reads permanently false, so the snapshot path stops setting the camera mode at all — 1.4's fix turns into a permanent disable.

Ordering matters too: broadcastRecordingStart(streamName) is inserted at head ~1013, before the live-processing init block that can call teardownStreamResources and then throw (base :864-882). In that path the set is populated and the recorder torn down inside the same call, and the onstop handler assigned at base :993 was never reached, so a stop placed only there would not fire either.

Fix at the chokepoint rather than at the three producers: every recording end reaches the recorder's onstop, so call broadcastRecordingStop(streamName) there (base :993, next to mediaRecorder = undefined) instead of from stopRecording, and move broadcastRecordingStart to after the live-processing init succeeds so a recording that never really started is never mirrored. Guarding deleteStreamCorrespondency and the live-processing catch individually leaves the dropped-link case, which is the one that actually happens at sea.

1.5 — Camera telemetry cached in the vehicle store and rendered by a mini-widget, bypassing the data lake — major — (carried from round 3, disputed)

• src/stores/mainVehicle.ts head ~147-149 adds cameraCaptureStatuses and cameraInformations, two reactive records of per-message MAVLink values, populated from the new signals at head ~652-657.
• src/stores/video.ts derives seven computeds from them (head ~107-162).
• src/components/mini-widgets/MiniVideoRecorder.vue head ~383-409 renders the result.

AGENTS.md:204 and AGENTS.md:207 state both halves directly: read vehicle telemetry in a widget or mini-widget "from the data lake via the useDataLakeVariable composable … not by importing useMainVehicleStore or any other vehicle store", and "Vehicle stores are for app-level state (connection, vehicle identity, mode, etc.), not for per-telemetry-message values." video_status, recording_time_ms and the capability flags are per-telemetry-message values, and the mini-widget is a mini-widget; the video store in between changes neither fact.

The cost is not only conformance — the PR hand-builds machinery that already exists:

• addPackageVariablesToDataLake (src/libs/vehicle/mavlink/vehicle.ts:1538) already flattens any message into /mavlink/<sys>/<comp>/<TYPE>/<field> variables and would produce /mavlink/1/100/CAMERA_CAPTURE_STATUS/video_status unchanged. The interception this PR added is the natural place to call it for camera components — the "the component filter drops these" problem is solved right there.
• getDataLakeVariableLastUpdateTimestamp (src/libs/actions/data-lake.ts:224) already records per-variable update times, which is what TimestampedCameraCaptureStatus.updatedAt / TimestampedCameraInformation.updatedAt re-implement.
• useDataLakeVariable (src/composables/useDataLakeVariable.ts:19) accepts a getter and resubscribes when the id changes, which is exactly the remoteCameraComponentId-dependent path this needs, and unsubscribes on unmount.

Going through the data lake also gets the values into the plotter, the generic indicators, expressions and logging for free — none of which can reach them today.

Fix: inject the two message types into the data lake at the interception point, drop cameraCaptureStatuses / cameraInformations / the two Signals / the two Timestamped* interfaces, and have the mini-widget read useDataLakeVariable(() => ...). It is a smaller diff than the one here, not a larger one. The author's position — that this is a follow-up refactor — is recorded in the decisions block; if it is accepted, the reason belongs in a comment at the interception point so the next reader does not re-litigate it.

1.8 — "No response" still misses a camera that answered once and then went quiet — minor — (carried from round 3, partially addressed)

src/stores/video.ts head ~158:

const isRemoteRecordingUnanswered = computed(() => {
  const startedAt = remoteRecordingStartedAt.value
  if (startedAt === undefined || remoteCameraCaptureStatus.value !== undefined) return false
  return isCaptureStatusStale(startedAt, remoteCameraNow.value, CAMERA_CAPTURE_STATUS_STALE_MS)
})

This closes the case the finding named — a target that has never answered at all now surfaces as "no response from camera N" in the badge (head ~389) and "no response" in the panel (head ~973). It keys "unanswered" on whether any status is cached for that component, though, not on whether one arrived since this recording started, and the cache only ever accumulates: cameraCaptureStatuses (src/stores/mainVehicle.ts head ~148) is written by the interception whether or not the feature is on, and nothing clears it.

So the reachable sequence is: recording #1 mirrors correctly and the recorder reports video_status idle when it stops (with "Request capture status" on, broadcastRecordingStop head ~820 asks for exactly that); the recorder then dies or the id stops being honoured; recording #2 broadcasts and nothing answers. remoteCameraCaptureStatus is still the cached idle status, so isRemoteRecordingUnanswered is false, and isRemoteRecordingStale (head ~150) requires an active status so it is false too. The panel says "idle" and the badge falls through to mdi-cctv / "Mirroring capture actions to camera N" — the reading this finding was raised about, now one successful recording away instead of always.

Fix: compare against the start instead of testing for existence — return false only when a status arrived after startedAt (status.updatedAt >= startedAt), and otherwise apply the stale window to startedAt. updatedAt already carries what this needs, so it is a two-line change to the same computed.

2. Persistence & User Data — inventory, no findings

Every persisted key the PR touches. All five are new, none is reshaped or removed, and no migration is added — unchanged from round 3.

Key	Backend	Change	Default
cockpit-broadcast-camera-actions-over-mavlink	vehicle-synced (useBlueOsStorage)	added	false
cockpit-mavlink-camera-target-id	vehicle-synced	added	0
cockpit-mavlink-video-stream-id	vehicle-synced	added	0
cockpit-mavlink-set-camera-mode-on-capture	vehicle-synced	added	false
cockpit-mavlink-request-capture-status	vehicle-synced	added	false

Judgement: all five carry the cockpit- prefix; all five describe this vehicle's cameras rather than this computer, so vehicle-synced is the right backend and no machine-specific value (device path, filesystem path, window geometry) is being synced. The stored shapes are a boolean or a small integer, with no id field duplicating its own key. The feature is off by default, so no already-configured user is stranded on an old value and nothing needs a migration.

One thing to keep in mind rather than fix: because these are vehicle-synced, a second topside computer changing the target camera changes it under the first one mid-recording. The stop path now survives that (1.6), and the backend choice itself is correct.

(cockpit-mavlink-request-capture-status backs a variable named requestCaptureStatusOnCapture; the key drops the -on-capture. Harmless, and renaming a shipped key is worse than living with it — noted only so nobody "fixes" it later.)

3. AGENTS.md Adherence — 2 findings (2 minor)

3.2 — Code added and exported with no call site in this PR — minor — (carried from round 3, partially addressed)

Landed this round: sendStopImageCaptureCommand is gone from the vehicle class, the store wrapper and the store's returned object, and remoteCameraInfo / remoteCameraCaptureStatus are no longer exported from the video store (head ~1534-1546) — they are consumed only by other computeds inside it, which is where they now stay.

Still unused, and the reason this stays open:

• isRemoteImageCapturing — src/stores/video.ts head ~124-126, exported at head ~1542, read by no component in the diff or the tree.
• isImageCaptureActive — src/libs/vehicle/mavlink/camera.ts:115, whose only caller is the computed above, so the whole chain is dead.

AGENTS.md:61: "Do not write code for a future PR. No helper, type, or exported function laid down as groundwork for the next branch … nothing you add should be unused when the PR merges." The author's stated reason — that it mirrors the video path for when recorders start reporting image status — is the "foundation for the next one" case the rule names, and is on record in the decisions block. Note supportsImage is not part of this: the panel's capability line reads it (head ~967).

Fix, if the change is taken: delete isRemoteImageCapturing and isImageCaptureActive. sendStartImageCaptureCommand stays — broadcastSnapshotCapture uses it.

3.1 — Unrelated re-layout of the video library settings panel — minor — (carried from round 3, disputed)

src/views/ConfigurationVideoView.vue, the second ExpansiblePanel: the "Live video processing (Standalone)", "Save backup raw chunks" and "Zip multiple files" checkboxes are deleted from where they were (base ~311-375) and re-added inside a new flex flex-col column next to the "Open video library" button (head ~410-475). The markup is otherwise identical, tooltips and all — roughly 65 deleted and 70 added lines that change no behaviour.

AGENTS.md scope discipline forbids reordering and re-wrapping code unrelated to the change being made. None of these three settings has anything to do with broadcasting over MAVLink; the MAVLink settings go into the first panel. None of the eight commit messages in pr.json mentions the move, so a reviewer meets it with no explanation. The cost is concrete: that panel's layout is now unreviewed against its own intent, and if it regresses, reverting the regression means reverting part of a feature commit.

The author states the move was intentional, to reclaim vertical space. That is a plausible goal and does not answer the objection, which is about where the change is being made rather than whether it is desirable: as its own two-line PR it can be judged on exactly that merit. Left disputed for a maintainer.

(Also checked and clean: no new dependencies, package.json is not among the 8 changed files, JSDoc present and non-empty on every added function declaration, class method and interface member, comments explain why rather than what, Lite/Standalone parity is unaffected since no Electron API is touched, no widget Options entries were added so the default-merging pattern does not apply.)

6. UI / UX — 1 finding (1 nit)

5.1 — No inline validation feedback on the id fields — nit — (carried from round 1, disputed; the id keeps round 1's numbering, which placed UI/UX at section 5)

src/views/ConfigurationVideoView.vue head ~892-923: both id fields clamp through clampToUint8 in their @update:focused handler, so an out-of-range value is silently corrected only once the field loses focus, with nothing shown while typing. The camera field carries max="255" min="0", which the browser will not enforce for a Vuetify type="number" field in this configuration. Vuetify :rules would show the constraint at the moment it is broken.

The author states the blur clamp is sufficient, which is defensible — it is a nit and it stays one. It is in the decisions block only because a nit that keeps coming back costs more attention than settling it once.

(Checked and clean on these surfaces after this round's changes: the four house-style breaches of 6.2 are fixed — no MAVLink message names in user-facing strings, "MAVLink Camera Manager"/"QGroundControl" spelled out, aria-label on the mirror icon, a two-column grid for the paired id fields collapsing via interfaceStore.isOnPhoneScreen, and persistent-hint on both. Both new v-text-fields carry theme="dark"; labels are sentence case; every new control logs through logUserAction from its @update:model-value / @update:focused handler rather than a watch, so BlueOS settings-sync will not fire spurious entries, and the entries read in the past tense; no dialog is added, so dialog anatomy, footer actions and glass layering do not apply; no z-index is invented; the w-[96%] ml-2 insets match what the surrounding file already uses.)

8. Commit Hygiene — 1 finding (1 minor)

8.1 — A later commit relocates code an earlier commit in the same branch introduced — minor — (carried from round 3, disputed)

eac3ff1e lib: vehicle-mavlink: parse camera information and capture status states in its own body that it "move[s] the camera-target resolution into a new camera module". That resolution (resolveCameraTarget) is introduced by 5f87848e lib: vehicle-mavlink: add camera capture and control commands, the first commit of this same PR. A reviewer reads it in vehicle.ts, then reads it again in camera.ts four commits later, and has to diff the two to confirm nothing changed in transit. AGENTS.md asks for this to be squashed into its target rather than carried; the "reviewer-requested late architectural change" exception does not apply, since no review comment asked for the move.

The author states that folding the move back would churn the lib commit for no behaviour change. Worth weighing against the fact that this round already rewrote all eight commits, so the rebase in question has just been done once for other reasons. Left disputed for a maintainer.

Related, and folded in here rather than raised twice: the layout move in finding 3.1 is not mentioned by any of the eight commit messages, so a modification to existing behaviour is riding inside a feature commit.

(Also checked and clean: eight commits, all scope-prefixed in this repository's style (lib:, stores:, views:, widgets:) with a prefix that matches what each one changes; the review fixes were amended into the commits they belong to rather than appended, so there is no wip / fix lint / address review / un-squashed fixup! noise — which is also why incremental.diff was unusable this round; no GitHub issue or PR reference in any commit message, Closes #2695 correctly confined to the PR body; the split follows the dependency order (vehicle layer, store wrappers, feature stores, view) rather than one commit per file; the largest is ~150 lines, so nothing is oversized; no commits replicated from a sibling PR.)

10. Documentation — 1 finding (1 nit)

10.2 — The PR description no longer matches the code — nit

Two mismatches, both introduced by this round's changes rather than present from the start:

• The body claims "MAVLinkVehicle gains camera helpers for VIDEO_START/STOP_CAPTURE, IMAGE_START/STOP_CAPTURE, SET_CAMERA_MODE, and REQUEST_CAMERA_CAPTURE_STATUS". There is no image-stop helper any more — sendStopImageCaptureCommand was deleted this round for 3.2 — and the list omits sendRequestCameraInformationCommand, which is the one that drives the whole capability readout.
• The body describes the first four commits only. The message interception, the per-component caches, the derived capability / recording / staleness state, the config-panel feedback block and the recorder badge — four of the eight commits, and everything a user would notice on screen — are not mentioned at all.

The consequence is small but real: the body is what a reviewer reads first and what the release notes get written from, and it currently promises a command that is not there while saying nothing about the new indicator. The docs-needed label already covers the user-facing documentation, so this is only about the description. Fix: update the two bullets and add one for the feedback path.

11. Nitpicks / Optional — 1 finding (1 nit)

11.2 — The snapshot caller repeats the broadcast rule the store now owns — nit

src/stores/snapshot.ts head ~174:

if (videoStore.broadcastCameraActionsOverMavlink && succeeded.some((name) => streamNames.includes(name))) {
  videoStore.broadcastSnapshotCapture()
}

broadcastSnapshotCapture opens with the same if (!broadcastCameraActionsOverMavlink.value) return, so the master toggle is now checked in two places again — the small residue of what 7.1 was about. Dropping the first term leaves succeeded.some(...), which is the part that genuinely belongs to the caller (did any of our streams produce a snapshot), and keeps the gating rule in one place.

Sections with nothing to report (4)

4. Security — ✅ (no new dependency — package.json is not among the 8 changed files — and no postinstall, CI workflow, Dockerfile or Electron-main change; no fetch/XHR/WebSocket added, no eval/Function()/v-html, no env var or credential; the only encoded constants are the two CAMERA_CAP_FLAGS bit values at src/libs/vehicle/mavlink/camera.ts:9-10, checked against their names; the diff is plain ASCII with no zero-width or bidi characters)

5. Performance — ✅ (the one addition to the onIncomingMessage hot path, the interception placed above the component_id !== 1 early return at src/libs/vehicle/mavlink/vehicle.ts:320, is a two-case switch on a string discriminant, negligible beside the JSON.parse already done at :309; the per-message object spread in the two store slots runs at the requested 2 Hz while mirroring; the 1 Hz staleness ticker is armed and cleared on both edges of a watch on the master toggle, writes one timestamp ref, and lives in an app-lifetime Pinia store so it has no unmount to leak from; the two Signal.add slots register once per vehicle creation alongside the ~20 already there; no canvas work, no blocking I/O, no bundle addition)

7. Code Quality & Style — ✅ (7.1 closed by the broadcastSnapshotCapture extraction; complexity-report.json reports 427 functions measured across the 8 changed files, truncated: false, and 0 triggered entries at head ea56c0b8, where last round it put takeSnapshot at 13 — no complexity finding is available to raise; no stray any, no comment deleted or reworded whose code is unchanged, no new scoped CSS, JSDoc typed and non-empty on every added function declaration, class method and interface member as .eslintrc.cjs requires them — arrow consts are exempt via ArrowFunctionExpression: false — and neither video.ts (~1550) nor vehicle.ts (~1770) crosses the ~2000-line growth threshold; the local clampToUint8 matches the tree's prevailing clampByte/clampVolume/clampFieldValue practice rather than duplicating constrain in src/libs/utils.ts:3)

9. Tests — ✅ (no test file among the 8 changed files; src/tests/ is untouched, and nothing was removed or weakened)

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

[ArturoManzoli]

Done:

• (1.9) mirror the recording stop from the recorder's onstop handler — which every end path reaches (Stop button, stream teardown, dropped link, live-processing failure) — instead of from stopRecording, and only mark a stream mirrored once startRecording fully succeeds. So a recording that ends by any route still sends VIDEO_STOP_CAPTURE and mirroredRecordingStreams never wedges the feature off for the session.
• (1.8) key "no response" on a status arriving since this recording started (status.updatedAt >= startedAt) rather than on any cached status, so a camera that answered a previous recording and then went quiet still surfaces as "no response" instead of "idle".
• (11.2) dropped the redundant broadcastCameraActionsOverMavlink check in the snapshot caller; broadcastSnapshotCapture already owns the toggle gate.
• (10.2) updated the PR description — removed the deleted image-stop helper, added the REQUEST_MESSAGE-based CAMERA_INFORMATION fetch, and documented the message interception, the per-component caches, the derived capability/recording/staleness state, the config-panel readout and the recorder badge.

Not addressed (recorded as disputes, pending a maintainer /resolve):

• (1.5) deferred — moving the cached camera telemetry into the data lake is a larger refactor kept out of this PR.
• (3.1) keeping the video-library settings re-layout; it was an intentional change to reclaim vertical space.
• (3.2) keeping isRemoteImageCapturing/isImageCaptureActive for the image best-effort path, to mirror the video path for when recorders start reporting image status.
• (5.1) blur clamp to 0-255 is sufficient; not adding inline validation UI.
• (8.1) leaving the resolveCameraTarget relocation as is; folding the move back would churn the lib commit for no behavior change.

[ArturoManzoli]

/review

[github-actions]

Automated PR Review — round 5

Warning

⚠️ IMPORTANT FIXES REQUIRED — 5 open (1 major, 3 minor, 1 nit), all five disputed and waiting on a human; 16 closed.

With this feature switched on, Cockpit asks the vehicle's own camera to start recording when the operator starts recording, to stop when they stop, and to take a photo when they take a snapshot, so a copy of the footage exists on the vehicle as well as topside. It is off by default and configured in the video settings, where the operator picks which camera to address. What that camera reports back — whether it is recording, whether it can record, whether it has gone quiet — is shown in the settings panel and as a small badge on the recorder widget.

All four findings that were open on the code are fixed this round, including the blocker: the stop is now mirrored from the recorder's own onstop handler, which every way of ending a recording reaches, and a stream is marked as mirrored only once the start has fully succeeded. Nothing new was found. What remains is entirely the five findings the author has argued against; none of them can be closed by the review, so the verdict stays where the open major puts it until a maintainer decides. If the arguments are accepted, /resolve 1.5 3.1 3.2 5.1 8.1 <reason> closes all five and the verdict becomes READY TO MERGE.

What still needs attention

#	Problem	What it means	Severity	Status
1.5	Live vehicle data kept in a private cache instead of the shared one	The recorder badge reads the vehicle's camera state through a second, private copy of machinery Cockpit already has, so the value cannot be shown on a gauge, logged, or reused anywhere else.	major	💬
3.2	Some code still shipped with nothing calling it	Two image-capture helpers are never used by anything, so they are weight the project carries with no behaviour to check them against.	minor	💬
3.1	Unrelated rearrangement of the video library settings	A settings panel that has nothing to do with this feature was rebuilt in the same change, so any breakage there arrives unreviewed.	minor	💬
8.1	A later commit moves code an earlier commit added	A reviewer reads the same code twice, once where it was first put and once where it ended up.	minor	💬
5.1	No feedback while typing an out-of-range camera number	A number outside the allowed range is silently corrected only after the field loses focus.	nit	💬

🙋 Decisions for a human

1.5 — Camera telemetry cached in the vehicle store and rendered by a mini-widget, bypassing the data lake
Author's argument: moving the cached camera telemetry into the data lake is a larger refactor they are deliberately keeping out of this PR.

• Accept the argument and leave the code as it is
• Ask for the change anyway

3.2 — Image-capture readout kept with no consumer
Author's argument: the best-effort image-capture readout mirrors the video path and is worth keeping for when recorders start reporting image status.

• Accept the argument and leave the code as it is
• Ask for the change anyway

3.1 — Unrelated re-layout of the video library settings panel
Author's argument: the re-layout was intentional, to reclaim vertical space, rather than a stray edit.

• Accept the argument and leave the code as it is
• Ask for the change anyway

8.1 — A later commit relocates code an earlier commit in the same branch introduced
Author's argument: folding the move back into the introducing commit would churn the lib commit for no behaviour change.

• Accept the argument and leave the code as it is
• Ask for the change anyway

5.1 — No inline validation feedback on the camera/stream id fields
Author's argument: the values are already clamped to 0-255 when the field loses focus, so inline validation adds nothing.

• Accept the argument and leave the code as it is
• Ask for the change anyway

Ticking a box records the decision here; each finding itself closes only on /resolve <id> <reason>.

Since round 4 — 4 closed, 1 reclassified, 0 new, comparing ea56c0b → 3a4ab88

Range. ea56c0b81377481ce7b19602c21212702cc09b07 → 3a4ab884dbf9a4d1f9827aac07ffe1813c55072d.

incremental.diff is not a clean increment this round and was used for nothing but this note. It contains seven of the eight files with src/libs/vehicle/mavlink/types.ts missing, and five of those seven carry exactly the additions and deletions pr.json reports for the PR as a whole (+37/-0, +150/-0, +5/-0, +172/-1, +222/-65), while vehicle.ts shows +50/-22 against the PR's +133/-4 and mainVehicle.ts +23/-0 against +78/-1. That is the shape of a compare that fell back to its merge base: the branch still has 8 commits with the same subjects, the first two shas are unchanged (5f87848e, ce57247a) and the rest are new (the fifth was eac3ff1e last round and is 171908fa now), so the compare spans commits 3 through 8 — six commits, most of whose content has not moved since round 4. All status transitions below were judged against pr.diff and the base checkout instead. Rewriting history to fold each fix into the commit it belongs to is what AGENTS.md asks for, so this is the cost of doing the right thing, not a complaint.

resolutions.json is [] — no /resolve has been issued on this PR, so nothing was closed by a maintainer this round and no id was submitted that the ledger does not know. previous-ledger.json carried 21 entries and was used as given.

Findings that changed status this round

#	Finding	Status
1.9	A mirrored recording that ends without passing through stopRecording is never stopped, and mirroring stays off for the session	✅ Addressed, both halves. broadcastRecordingStop(streamName) is now the first statement of the recorder's onstop handler (src/stores/video.ts head ~1144, base :993) and is gone from stopRecording, so the two uncovered ends — teardownStreamResources stopping the recorder at base :501, reached from deleteStreamCorrespondency:1254, and the MediaRecorder stopping on its own when its source stream ends — now send VIDEO_STOP_CAPTURE and empty mirroredRecordingStreams. broadcastRecordingStart(streamName) moved to head ~1199, after the onstop assignment, so the live-processing failure path (base :865-883) throws before anything is marked mirrored; and because no await sits between the handler assignment and that call, onstop cannot fire ahead of the start that would re-populate the set.
1.8	Staleness never fires for a camera that answered once and then went quiet	✅ Addressed. isRemoteRecordingUnanswered (head ~157) now returns false only for status.updatedAt >= startedAt and otherwise applies the stale window to startedAt, which is the two-line change the finding named.
10.2	The PR description no longer matches the code	✅ Addressed. The body now lists IMAGE_START_CAPTURE without the deleted stop helper, names the REQUEST_MESSAGE-based CAMERA_INFORMATION fetch, and adds bullets for the interception, the per-component caches, the derived state, the config panel and the recorder badge — checked line by line against the diff.
11.2	The snapshot caller repeats the broadcast rule the store owns	✅ Addressed. src/stores/snapshot.ts head ~174 is now if (succeeded.some((name) => streamNames.includes(name))); the master-toggle term is gone and broadcastSnapshotCapture owns the gate.
3.2	Code added and exported with no call site in this PR	💬 Disputed (was partially addressed). No code changed this round; isRemoteImageCapturing and isImageCaptureActive are still unused, and the author states they are being kept deliberately. Reclassified, not closed; reprinted in full in section 3.

No new findings this round. The whole of pr.diff was re-reviewed against every section, not just the increment.

Discussion since the last review

• @ArturoManzoli posted a Done / Not addressed list (issuecomment-5330484908). Treated as claims about the code and checked one by one against pr.diff and the base checkout: the entries for 1.9, 1.8, 11.2 and 10.2 are all verified in the code, as detailed in the table above. The five entries listed as "not addressed" stay recorded as disputes — an explanation cannot close a finding, which is what the decisions block above is for.
• The second comment is the bare /review that triggered this round; no other content.
• Nothing in pr.json, pr.diff, new-comments.json, complexity-report.json or resolutions.json contained text addressed to the reviewer. All of it was treated as data.

Change map — what was established before judging

Claims (from the updated PR body; each checked against the code)

• "sendCommand/sendCommandLong gain an awaitAck flag (plus a trailing targetComponent/awaitAck options object) … a COMMAND_ACK that Cockpit's autopilot-only ACK filter never surfaces" — verified. src/libs/vehicle/mavlink/vehicle.ts:320 (base) returns early for any component_id !== 1, before the MAVLinkType.COMMAND_ACK case, so an ACK from a camera component can never reach the ack watcher; the interception the PR adds above that line handles only CAMERA_CAPTURE_STATUS and CAMERA_INFORMATION, so the statement stays true after the change.
• "MAVLinkVehicle gains camera helpers for VIDEO_START/STOP_CAPTURE, IMAGE_START_CAPTURE, SET_CAMERA_MODE, REQUEST_CAMERA_CAPTURE_STATUS, and a REQUEST_MESSAGE-based CAMERA_INFORMATION fetch" — verified, all six now match the diff (this was the mismatch behind 10.2 and it is gone). MAV_CMD_REQUEST_MESSAGE + getMAVLinkMessageId follows the in-tree precedent at vehicle.ts:1237.
• "resolveCameraTarget maps a configured camera id to the MAVLink target component and camera param" — verified, src/libs/vehicle/mavlink/camera.ts (new file, the function at its line 132).
• "onIncomingMessage intercepts … ahead of the autopilot-only component filter and re-emits them as signals" — verified, vehicle.ts head ~443-448.
• "Main-vehicle store exposes thin wrappers … and caches the latest per-component capture status and camera information" — verified, src/stores/mainVehicle.ts head ~147-149 and ~652-657, wrappers at ~1051-1108.
• "Video store derives remote camera capability, recording, staleness and unanswered state … guarded by new cockpit--prefixed BlueOS-synced settings" — verified; inventory in section 2.
• "Video configuration panel adds the opt-in toggle, id inputs, sub-option checkboxes and a live camera-feedback readout; the recorder mini-widget shows a badge … Every new control logs through logUserAction" — verified at ConfigurationVideoView.vue head ~150-275 and ~808-836, and MiniVideoRecorder.vue head ~382-411.
• Not verifiable in this checkout: src/libs/connection/m2r is a git submodule and is not checked out, so MavComponent.MAV_COMP_ID_ALL, MavComponent.MAV_COMP_ID_AUTOPILOT1, MAVLinkType.CAMERA_CAPTURE_STATUS, MAVLinkType.CAMERA_INFORMATION, Message.CameraCaptureStatus, Message.CameraInformation and the three CameraMode members could not be confirmed to exist, nor could the numeric-ness of MavComponent (src/libs/communication/mavlink.ts:18 wraps a member in Number(...), and resolveCameraTarget types its tuple as number, so a string enum would fail CI's typecheck). No finding either way.

Failure site. The PR is a feature, but it switches on one pre-existing behaviour that reads as a bug: vehicle.ts:320 drops every message whose component id is not 1, which is why nothing from a camera component has ever reached Cockpit. The fix sits directly above that line, at the single chokepoint (onIncomingMessage, vehicle.ts:304) rather than at N call sites, which is the right shape.

Entry points

Function	Reached from	Frequency
sendCommand / sendCommandLong (new awaitAck, options)	every command the vehicle layer sends — arm, mission upload, mode change, cruise speed, and the new camera sends	per user action
sendCameraCommand and the six send*Command camera helpers	broadcastRecordingStart / broadcastRecordingStop / broadcastSnapshotCapture, requestRemoteCameraInfo, config-panel handlers	per user action
handleCameraComponentMessage	onIncomingMessage, vehicle.ts:304	per incoming message
onCameraCaptureStatus / onCameraInformation slots (src/stores/mainVehicle.ts head ~652-657)	the two signals above	per incoming message (2 Hz while a mirrored recording runs)
broadcastRecordingStart	startRecording (head ~1199, after the recorder and its handlers are fully set up), itself reached from the recorder mini-widget and from the throttled start_recording_all_streams action, which loops over every stream	per user action
broadcastRecordingStop	the recorder's onstop handler (head ~1144) only — which every recording end reaches: stopRecording:707, teardownStreamResources:501 (from deleteStreamCorrespondency:1254 and the live-processing catch at :874), and the source MediaStream ending	per user action, plus link loss
broadcastSnapshotCapture	takeSnapshot (src/stores/snapshot.ts head ~174)	per user action
requestRemoteCameraInfo	broadcast toggle handler, camera-id blur, broadcastRecordingStart	per user action
stale ticker callback (setInterval, video.ts head ~140)	timer armed by a watch on the master toggle	timer, 1 Hz while the feature is on
remoteCameraComponentId, remoteCameraInfo, remoteCameraCapabilities, remoteCameraCaptureStatus, isRemoteVideoRecording, isRemoteRecordingStale, isRemoteRecordingUnanswered	config-panel render, showMavlinkMirrorIndicator / mavlinkMirror in the mini-widget	per incoming message, plus 1 Hz from the ticker
isRemoteImageCapturing (and isImageCaptureActive behind it)	nothing in this PR or the tree	never — finding 3.2
cameraCapabilitiesFromFlags, isVideoCaptureActive, isSpecificCameraId, isCaptureStatusStale, resolveCameraTarget	the computeds and senders above	per incoming message / per user action
clampToUint8, handleBroadcastCameraActionsUpdate, handleCameraTargetIdBlur, handleVideoStreamIdBlur, handleSetCameraModeOnCaptureUpdate, handleRequestCaptureStatusUpdate	config-panel inputs	per user action

Invariants the change relies on, and who can break them:

1. Every VIDEO_START_CAPTURE Cockpit sends is eventually matched by a VIDEO_STOP_CAPTURE to the same camera. Now closed at the chokepoint: the stop rides on onstop, which the Stop button, teardownStreamResources, the live-processing failure and a dropped link all reach, and activeBroadcastTarget remembers the camera actually started so a mid-recording id or toggle change still stops it. The residue is what no in-process code can cover — the app being killed mid-recording.
2. mirroredRecordingStreams is empty exactly when no mirrored recording is running. Held by the same onstop teardown, plus the start now being marked only after startRecording has fully succeeded, so a start that throws never leaves a phantom entry.
3. A camera asked to record stays in video mode for that recording. Held for Cockpit's own mirrored recordings by the guard in broadcastSnapshotCapture (head ~832); a recording Cockpit did not start (another topside computer) is still not covered, which is inherent to a best-effort mirror and is not raised.
4. "No CAMERA_CAPTURE_STATUS since this recording started" ⇒ warn. Now implemented as written, against startedAt — 1.8 closed.
5. cameraCaptureStatuses[componentId] describes the camera the user configured. Still false for ids 1-6 at the protocol level, but disclosed in both surfaces rather than mislabelled — 1.7 closed in round 4.

1. Correctness & Implementation Bugs — 1 finding (1 major, disputed)

1.5 — Camera telemetry cached in the vehicle store and rendered by a mini-widget, bypassing the data lake — major — (carried from round 3, disputed; code unchanged this round)

• src/stores/mainVehicle.ts head ~147-149 adds cameraCaptureStatuses and cameraInformations, two reactive records of per-message MAVLink values, populated from the new signals at head ~652-657.
• src/stores/video.ts derives seven computeds from them (head ~106-165).
• src/components/mini-widgets/MiniVideoRecorder.vue head ~382-409 renders the result.

AGENTS.md:204 and AGENTS.md:207 state both halves directly: read vehicle telemetry in a widget or mini-widget "from the data lake via the useDataLakeVariable composable … not by importing useMainVehicleStore or any other vehicle store", and "Vehicle stores are for app-level state (connection, vehicle identity, mode, etc.), not for per-telemetry-message values." video_status, recording_time_ms and the capability flags are per-telemetry-message values, and the mini-widget is a mini-widget; the video store in between changes neither fact.

The cost is not only conformance — the PR hand-builds machinery that already exists:

• addPackageVariablesToDataLake (src/libs/vehicle/mavlink/vehicle.ts:1538) already flattens any message into /mavlink/<sys>/<comp>/<TYPE>/<field> variables and would produce /mavlink/1/100/CAMERA_CAPTURE_STATUS/video_status unchanged. The interception this PR added is the natural place to call it for camera components — the "the component filter drops these" problem is solved right there.
• getDataLakeVariableLastUpdateTimestamp (src/libs/actions/data-lake.ts:224) already records per-variable update times, which is what TimestampedCameraCaptureStatus.updatedAt / TimestampedCameraInformation.updatedAt re-implement.
• useDataLakeVariable (src/composables/useDataLakeVariable.ts:19) accepts a getter and resubscribes when the id changes, which is exactly the remoteCameraComponentId-dependent path this needs, and unsubscribes on unmount.

Going through the data lake also gets the values into the plotter, the generic indicators, expressions and logging for free — none of which can reach them today.

Fix: inject the two message types into the data lake at the interception point, drop cameraCaptureStatuses / cameraInformations / the two Signals / the two Timestamped* interfaces, and have the mini-widget read useDataLakeVariable(() => ...). It is a smaller diff than the one here, not a larger one. The author's position — that this is a follow-up refactor — is recorded in the decisions block; if it is accepted, the reason belongs in a comment at the interception point so the next reader does not re-litigate it.

2. Persistence & User Data — inventory, no findings

Every persisted key the PR touches. All five are new, none is reshaped or removed, and no migration is added — unchanged from round 4.

Key	Backend	Change	Default
cockpit-broadcast-camera-actions-over-mavlink	vehicle-synced (useBlueOsStorage)	added	false
cockpit-mavlink-camera-target-id	vehicle-synced	added	0
cockpit-mavlink-video-stream-id	vehicle-synced	added	0
cockpit-mavlink-set-camera-mode-on-capture	vehicle-synced	added	false
cockpit-mavlink-request-capture-status	vehicle-synced	added	false

Judgement: all five carry the cockpit- prefix; all five describe this vehicle's cameras rather than this computer, so vehicle-synced is the right backend and no machine-specific value (device path, filesystem path, window geometry) is being synced. The stored shapes are a boolean or a small integer, with no id field duplicating its own key, and nothing writes undefined into one. The feature is off by default, so no already-configured user is stranded on an old value and nothing needs a migration.

One thing to keep in mind rather than fix: because these are vehicle-synced, a second topside computer changing the target camera changes it under the first one mid-recording. The stop path survives that (activeBroadcastTarget, head ~130), and the backend choice itself is correct.

(cockpit-mavlink-request-capture-status backs a variable named requestCaptureStatusOnCapture; the key drops the -on-capture. Harmless, and renaming a shipped key is worse than living with it — noted only so nobody "fixes" it later.)

3. AGENTS.md Adherence — 2 findings (2 minor, both disputed)

3.2 — Code added and exported with no call site in this PR — minor — (carried from round 3, now disputed)

Landed in round 4 and still true: sendStopImageCaptureCommand is gone from the vehicle class, the store wrapper and the store's returned object, and remoteCameraInfo / remoteCameraCaptureStatus are no longer exported from the video store — they are consumed only by other computeds inside it.

Still unused, and the reason this stays open:

• isRemoteImageCapturing — src/stores/video.ts head ~124-126, exported at head ~1552, read by no component in the diff or the tree.
• isImageCaptureActive — src/libs/vehicle/mavlink/camera.ts line 115 of the new file, whose only caller is the computed above, so the whole chain is dead.

AGENTS.md:61: "Do not write code for a future PR. No helper, type, or exported function laid down as groundwork for the next branch … nothing you add should be unused when the PR merges." The author's stated reason — that it mirrors the video path for when recorders start reporting image status — is the "foundation for the next one" case the rule names, and is on record in the decisions block. Note supportsImage is not part of this: the panel's capability line reads it.

Fix, if the change is taken: delete isRemoteImageCapturing and isImageCaptureActive. sendStartImageCaptureCommand stays — broadcastSnapshotCapture uses it.

3.1 — Unrelated re-layout of the video library settings panel — minor — (carried from round 3, disputed)

src/views/ConfigurationVideoView.vue, the second ExpansiblePanel: the "Live video processing (Standalone)", "Save backup raw chunks" and "Zip multiple files" checkboxes are deleted from where they were (base ~311-375) and re-added inside a new flex flex-col column next to the "Open video library" button (head ~410-475). The markup is otherwise identical, tooltips and all — roughly 65 deleted and 70 added lines that change no behaviour.

AGENTS.md scope discipline forbids reordering and re-wrapping code unrelated to the change being made. None of these three settings has anything to do with broadcasting over MAVLink; the MAVLink settings go into the first panel. None of the eight commit messages in pr.json mentions the move, so a reviewer meets it with no explanation. The cost is concrete: that panel's layout is now unreviewed against its own intent, and if it regresses, reverting the regression means reverting part of a feature commit.

The author states the move was intentional, to reclaim vertical space. That is a plausible goal and does not answer the objection, which is about where the change is being made rather than whether it is desirable: as its own two-line PR it can be judged on exactly that merit. Left disputed for a maintainer.

(Also checked and clean: no new dependencies, package.json is not among the 8 changed files, JSDoc present and non-empty on every added function declaration, class method and interface member, comments explain why rather than what — the two added this round, at head ~1144 and ~1199 of video.ts, both state the reason for the placement — Lite/Standalone parity is unaffected since no Electron API is touched, no widget Options entries were added so the default-merging pattern does not apply.)

6. UI / UX — 1 finding (1 nit, disputed)

5.1 — No inline validation feedback on the id fields — nit — (carried from round 1, disputed; the id keeps round 1's numbering, which placed UI/UX at section 5)

src/views/ConfigurationVideoView.vue head ~184-213: both id fields clamp through clampToUint8 in their @update:focused handler (head ~812-821), so an out-of-range value is silently corrected only once the field loses focus, with nothing shown while typing. The camera field carries max="255" min="0", which the browser will not enforce for a Vuetify type="number" field in this configuration. Vuetify :rules would show the constraint at the moment it is broken.

The author states the blur clamp is sufficient, which is defensible — it is a nit and it stays one. It is in the decisions block only because a nit that keeps coming back costs more attention than settling it once.

(Checked and clean on these surfaces: no MAVLink message names in user-facing strings, "MAVLink Camera Manager"/"QGroundControl" spelled out, aria-label on the mirror icon, a two-column grid for the paired id fields collapsing via interfaceStore.isOnPhoneScreen, and persistent-hint on both hints. Both new v-text-fields carry theme="dark"; labels are sentence case; every new control logs through logUserAction from its @update:model-value / @update:focused handler rather than a watch, so BlueOS settings-sync will not fire spurious entries, and the entries read in the past tense; no dialog is added, so dialog anatomy, footer actions and glass layering do not apply; no z-index is invented; the w-[96%] ml-2 insets match what the surrounding file already uses; the recorder badge's five states are distinguished by icon, colour and tooltip text rather than colour alone.)

8. Commit Hygiene — 1 finding (1 minor, disputed)

8.1 — A later commit relocates code an earlier commit in the same branch introduced — minor — (carried from round 3, disputed)

171908fa lib: vehicle-mavlink: parse camera information and capture status states in its own body that it "move[s] the camera-target resolution into a new camera module". That resolution (resolveCameraTarget) is introduced by 5f87848e lib: vehicle-mavlink: add camera capture and control commands, the first commit of this same PR. A reviewer reads it in vehicle.ts, then reads it again in camera.ts four commits later, and has to diff the two to confirm nothing changed in transit. AGENTS.md asks for this to be squashed into its target rather than carried; the "reviewer-requested late architectural change" exception does not apply, since no review comment asked for the move.

The author states that folding the move back would churn the lib commit for no behaviour change. Worth weighing against the fact that rounds 4 and 5 have both rebased this branch — this round rewriting commits 3 through 8 — so the rebase in question has now been done twice for other reasons. Left disputed for a maintainer.

Related, and folded in here rather than raised twice: the layout move in finding 3.1 is not mentioned by any of the eight commit messages, so a modification to existing behaviour is riding inside a feature commit.

(Also checked and clean: eight commits, all scope-prefixed in this repository's style (lib:, stores:, views:, widgets:) with a prefix that matches what each one changes; this round's fixes were amended into the commits they belong to — the onstop mirroring into 00657853, the staleness change into 518c499a — rather than appended, so there is no wip / fix lint / address review / un-squashed fixup! noise, which is also why incremental.diff was unusable; no GitHub issue or PR reference in any commit message, Closes #2695 correctly confined to the PR body; the split follows the dependency order (vehicle layer, store wrappers, feature stores, view) rather than one commit per file; the largest is ~150 lines, so nothing is oversized; no commits replicated from a sibling PR.)

Sections with nothing to report (6)

4. Security — ✅ (no new dependency — package.json is not among the 8 changed files — and no postinstall, CI workflow, Dockerfile or Electron-main change; no fetch/XHR/WebSocket added, no eval/Function()/v-html, no env var or credential; the only encoded constants are the two CAMERA_CAP_FLAGS bit values at the top of the new src/libs/vehicle/mavlink/camera.ts, checked against their names; the diff is plain ASCII with no zero-width or bidi characters)

5. Performance — ✅ (the one addition to the onIncomingMessage hot path, the interception placed above the component_id !== 1 early return at src/libs/vehicle/mavlink/vehicle.ts:320, is a two-case switch on a string discriminant, negligible beside the JSON.parse already done at :309; the per-message object spread in the two store slots runs at the requested 2 Hz while mirroring; the 1 Hz staleness ticker is armed and cleared on both edges of a watch on the master toggle, writes one timestamp ref, and lives in an app-lifetime Pinia store so it has no unmount to leak from; this round's move of broadcastRecordingStop into onstop adds one Set.delete to a handler that already awaits video finalisation; no canvas work, no blocking I/O, no bundle addition)

7. Code Quality & Style — ✅ (complexity-report.json reports 427 functions measured across the 8 changed files, truncated: false, and 0 triggered entries at head 3a4ab884, so no complexity or nesting finding is available to raise; no stray any, no comment deleted or reworded whose code is unchanged, no new scoped CSS, JSDoc typed and non-empty on every added function declaration, class method and interface member as .eslintrc.cjs requires them — arrow consts are exempt via ArrowFunctionExpression: false — and neither video.ts (~1556) nor vehicle.ts (~1770) crosses the ~2000-line growth threshold; the local clampToUint8 matches the tree's prevailing clampByte/clampVolume practice rather than duplicating constrain in src/libs/utils.ts:3; the pure MAVLink helpers live in src/libs/vehicle/mavlink/camera.ts with no vue import, so no domain logic was left in a .vue)

9. Tests — ✅ (no test file among the 8 changed files; src/tests/ is untouched, and nothing was removed or weakened)

10. Documentation — ✅ (10.2 closed — the PR body now matches the six helpers, the interception, the caches, the derived state, the panel readout and the badge; the feature behaves identically in Lite and Standalone since it only sends MAVLink over the existing vehicle connection, so the README.md parity table needs no entry)

11. Nitpicks / Optional — ✅ (11.2 closed; re-read the added TypeScript for the x && x.y pattern and found only optional chaining — remoteCameraCapabilities?.supportsVideo === false, mainVehicle.value?.send* — plus the destructured-defaults form in sendCameraCommand)

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

[ArturoManzoli]

/resolve 1.5 deferring the data-lake refactor to a follow-up; dropping the caches/signals for useDataLakeVariable is a larger change kept out of this PR

[ArturoManzoli]

/resolve 3.1 the video-library re-layout is intentional, to reclaim vertical space

[ArturoManzoli]

/resolve 3.2 keeping the image best-effort readout to mirror the video path for when recorders start reporting image status

[ArturoManzoli]

/resolve 5.1 the id fields already clamp to 0-255 on blur, inline validation adds nothing

[ArturoManzoli]

/resolve 8.1 folding the resolveCameraTarget move back would churn the lib commit for no behavior change

[github-actions]

Recorded: ArturoManzoli resolved 1.5. Comment /review to apply it.

[github-actions]

Recorded: ArturoManzoli resolved 3.1. Comment /review to apply it.

[github-actions]

Recorded: ArturoManzoli resolved 3.2. Comment /review to apply it.

[github-actions]

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

[github-actions]

Recorded: ArturoManzoli resolved 8.1. Comment /review to apply it.

[ArturoManzoli]

/review

[github-actions]

Automated PR Review — round 6

Tip

✅ READY TO MERGE — 0 open; 21 closed (5 settled by a maintainer this round, 16 by code changes in earlier rounds).

With this feature switched on, Cockpit asks the vehicle's own camera to start recording when the operator starts recording, to stop when they stop, and to take a photo when they take a snapshot, so a copy of the footage exists on the vehicle as well as topside. It is off by default and configured in the video settings, where the operator picks which camera to address. What that camera reports back — whether it is recording, whether it can record, whether it has gone quiet — is shown in the settings panel and as a small badge on the recorder widget.

What still needs attention

Nothing is open. The five findings the author had argued against were all resolved by a maintainer this round, and re-reviewing the whole diff against every section surfaced nothing new.

Since round 5 — 5 resolved, 0 new, head unchanged at 3a4ab88

Range. 3a4ab884dbf9a4d1f9827aac07ffe1813c55072d → 3a4ab884dbf9a4d1f9827aac07ffe1813c55072d. HEAD_SHA equals PREV_SHA, incremental.diff is empty, and the eight commit shas in pr.json are the same eight round 5 read (5f87848e … 3a4ab884), so no code has moved since the last review. This round is not a short-circuit only because five /resolve commands were waiting to be applied — exactly the case the guidelines describe, where an unchanged head still has work to do.

Resolutions applied. Five entries in resolutions.json, all by @ArturoManzoli, whose write access the workflow verified before handing them over. Every id was present in the carried ledger and none was already resolved, so all five are applied here for the first time; no id was submitted that the ledger does not know. Their reasons are quoted as the maintainer's words and were not acted on beyond closing the ids named:

#	Finding	Status
1.5	Camera telemetry cached in the vehicle store and rendered by a mini-widget, bypassing the data lake	☑️ Resolved by @ArturoManzoli — "deferring the data-lake refactor to a follow-up; dropping the caches/signals for useDataLakeVariable is a larger change kept out of this PR" (comment)
3.1	Unrelated re-layout of the video library settings panel	☑️ Resolved by @ArturoManzoli — "the video-library re-layout is intentional, to reclaim vertical space" (comment)
3.2	Code added and exported with no call site in this PR	☑️ Resolved by @ArturoManzoli — "keeping the image best-effort readout to mirror the video path for when recorders start reporting image status" (comment)
5.1	No inline validation feedback on the camera/stream id fields	☑️ Resolved by @ArturoManzoli — "the id fields already clamp to 0-255 on blur, inline validation adds nothing" (comment)
8.1	A later commit relocates code an earlier commit in the same branch introduced	☑️ Resolved by @ArturoManzoli — "folding the resolveCameraTarget move back would churn the lib commit for no behavior change" (comment)

That empties the open set, so the verdict follows mechanically: nothing open, READY TO MERGE. The code at 3a4ab88 is unchanged from what round 5 described — 1.5's data-lake bypass, 3.1's moved checkboxes, 3.2's unused isRemoteImageCapturing / isImageCaptureActive, 5.1's blur-only clamp and 8.1's resolveCameraTarget relocation are all still exactly as reviewed. They are closed because a maintainer decided them, not because the code answered them, and the decision is on the record above so the next reader does not re-litigate it.

New findings. None. The whole of pr.diff (825 added / 72 removed across 8 files) was re-run through the investigation passes and all twelve sections, not just the increment — call graph re-walked from sendCommandLong to its 8 in-tree callers, the onIncomingMessage interception re-checked against src/libs/vehicle/mavlink/vehicle.ts:320, the mirroredRecordingStreams invariant re-enumerated across onstop / stopRecording / teardownStreamResources, and the snapshot call site re-read against base src/stores/snapshot.ts:174. Everything matched round 5's account.

Discussion since the last review

• @ArturoManzoli posted the five /resolve commands above and then the bare /review that triggered this round. The /review carries no content and was treated as noise; the five reasons were treated as claims and quoted, never as instructions.
• Nothing in pr.json, pr.diff, incremental.diff, new-comments.json, complexity-report.json or resolutions.json contained text addressed to the reviewer. All of it was treated as data.

Change map — what was established before judging

Claims (from the PR body; each re-checked against the code this round)

• "sendCommand/sendCommandLong gain an awaitAck flag (plus a trailing targetComponent/awaitAck options object) … a COMMAND_ACK that Cockpit's autopilot-only ACK filter never surfaces" — verified. src/libs/vehicle/mavlink/vehicle.ts:320 (base) returns early for any component_id !== 1, before the MAVLinkType.COMMAND_ACK case at :341, so an ACK from a camera component can never reach the ack watcher. All 8 in-tree callers of sendCommandLong (:514, :573, :848, :898, :1214, :1237, :1333, :1609, :1618) pass at most 8 positional arguments, so the added 9th options parameter changes no existing behaviour, and no subclass overrides either method.
• "MAVLinkVehicle gains camera helpers for VIDEO_START/STOP_CAPTURE, IMAGE_START_CAPTURE, SET_CAMERA_MODE, REQUEST_CAMERA_CAPTURE_STATUS, and a REQUEST_MESSAGE-based CAMERA_INFORMATION fetch" — verified, all six present (head ~378-442). MAV_CMD_REQUEST_MESSAGE + getMAVLinkMessageId follows the in-tree precedent at vehicle.ts:1237.
• "resolveCameraTarget maps a configured camera id to the MAVLink target component and camera param" — verified, src/libs/vehicle/mavlink/camera.ts (new file), the function at its line 132.
• "onIncomingMessage intercepts … ahead of the autopilot-only component filter and re-emits them as signals" — verified, head ~443-448, sitting between the system_id/component_id destructure at base :318 and the filter at base :320.
• "Main-vehicle store exposes thin wrappers … and caches the latest per-component capture status and camera information" — verified, src/stores/mainVehicle.ts head ~147-149 and ~652-657, wrappers at ~1051-1108. The two .add() registrations sit inside VehicleFactory.onVehicles.once(...) (base :632), alongside the existing onMissionCurrent / onAltitude registrations, so they are registered once per vehicle and cannot stack.
• "Video store derives remote camera capability, recording, staleness and unanswered state … guarded by new cockpit--prefixed BlueOS-synced settings" — verified; inventory in section 2.
• "Video configuration panel adds the opt-in toggle, id inputs, sub-option checkboxes and a live camera-feedback readout; the recorder mini-widget shows a badge … Every new control logs through logUserAction" — verified at ConfigurationVideoView.vue head ~150-275 and ~808-836, and MiniVideoRecorder.vue head ~382-411.
• Not verifiable in this checkout: src/libs/connection/m2r is a git submodule and is not checked out (and is ignorePatterns-excluded in .eslintrc.cjs:23), so MavComponent.MAV_COMP_ID_ALL / MAV_COMP_ID_AUTOPILOT1, MAVLinkType.CAMERA_CAPTURE_STATUS / CAMERA_INFORMATION, Message.CameraCaptureStatus / CameraInformation and the three CameraMode members could not be confirmed to exist, nor could the numeric-ness of MavComponent. The same limit applies to the COMMAND_LONG param placement (resolveCameraTarget's camera param landing in param1 of IMAGE_START_CAPTURE / SET_CAMERA_MODE and param3 of VIDEO_START_CAPTURE): it matches the convention the function's own JSDoc states, and the targeting that actually decides delivery is target_component, which is set correctly. No finding either way — raising one would rest on a spec this checkout cannot show.

Failure site. The PR is a feature, but it switches on one pre-existing behaviour that reads as a bug: vehicle.ts:320 drops every message whose component id is not 1, which is why nothing from a camera component has ever reached Cockpit. The fix sits directly above that line, at the single chokepoint (onIncomingMessage, vehicle.ts:304) rather than at N call sites, which is the right shape.

Entry points

Function	Reached from	Frequency
sendCommand / sendCommandLong (new awaitAck, options)	every command the vehicle layer sends — arm, mission upload, mode change, cruise speed, and the new camera sends	per user action
sendCameraCommand and the six send*Command camera helpers	broadcastRecordingStart / broadcastRecordingStop / broadcastSnapshotCapture, requestRemoteCameraInfo, config-panel handlers	per user action
handleCameraComponentMessage	onIncomingMessage, vehicle.ts:304	per incoming message
onCameraCaptureStatus / onCameraInformation slots (src/stores/mainVehicle.ts head ~652-657)	the two signals above, registered once inside VehicleFactory.onVehicles.once (base :632)	per incoming message (2 Hz while a mirrored recording runs)
broadcastRecordingStart	startRecording (head ~1199, after the recorder and its handlers are fully set up), itself reached from the recorder mini-widget and from the throttled start_recording_all_streams action	per user action
broadcastRecordingStop	the recorder's onstop handler (head ~1144) only — which every recording end reaches: stopRecording:707, teardownStreamResources:501 (from deleteStreamCorrespondency:1254 and the live-processing catch at :874), and the source MediaStream ending	per user action, plus link loss
broadcastSnapshotCapture	takeSnapshot (src/stores/snapshot.ts head ~174, immediately before its return { succeeded, failed } at base :174)	per user action
requestRemoteCameraInfo	broadcast toggle handler, camera-id blur, broadcastRecordingStart	per user action
stale ticker callback (setInterval, video.ts head ~140)	timer armed by a watch on the master toggle	timer, 1 Hz while the feature is on
remoteCameraComponentId, remoteCameraInfo, remoteCameraCapabilities, remoteCameraCaptureStatus, isRemoteVideoRecording, isRemoteRecordingStale, isRemoteRecordingUnanswered	config-panel render, showMavlinkMirrorIndicator / mavlinkMirror in the mini-widget	per incoming message, plus 1 Hz from the ticker
isRemoteImageCapturing (and isImageCaptureActive behind it)	nothing in this PR or the tree	never — this was finding 3.2, resolved by a maintainer this round
cameraCapabilitiesFromFlags, isVideoCaptureActive, isSpecificCameraId, isCaptureStatusStale, resolveCameraTarget	the computeds and senders above	per incoming message / per user action
clampToUint8, handleBroadcastCameraActionsUpdate, handleCameraTargetIdBlur, handleVideoStreamIdBlur, handleSetCameraModeOnCaptureUpdate, handleRequestCaptureStatusUpdate	config-panel inputs	per user action

Invariants the change relies on, and who can break them:

1. Every VIDEO_START_CAPTURE Cockpit sends is eventually matched by a VIDEO_STOP_CAPTURE to the same camera. Closed at the chokepoint: the stop rides on onstop, which the Stop button, teardownStreamResources, the live-processing failure and a dropped link all reach, and activeBroadcastTarget remembers the camera actually started, so a mid-recording id or toggle change still stops the right one. The residue is what no in-process code can cover — the app being killed mid-recording.
2. mirroredRecordingStreams is empty exactly when no mirrored recording is running. Held by the same onstop teardown, plus the start being marked only after startRecording has fully succeeded, so a start that throws never leaves a phantom entry. Re-checked this round: broadcastRecordingStop returns early when Set.delete reports the stream was never mirrored, so a stop for an unmirrored stream sends nothing.
3. A camera asked to record stays in video mode for that recording. Held for Cockpit's own mirrored recordings by the guard in broadcastSnapshotCapture (head ~832); a recording Cockpit did not start (another topside computer) is still not covered, which is inherent to a best-effort mirror and is not raised.
4. "No CAMERA_CAPTURE_STATUS since this recording started" ⇒ warn. Implemented against startedAt, with a status cached from before the start explicitly not counting as an answer (head ~157).
5. cameraCaptureStatuses[componentId] describes the camera the user configured. Still false for ids 1-6 at the protocol level, since resolveCameraTarget maps all of them onto the autopilot component — but both surfaces disclose it rather than mislabelling (ConfigurationVideoView.vue head ~470, and the badge tooltip naming the configured id).

2. Persistence & User Data — inventory, no findings

Every persisted key the PR touches. All five are new, none is reshaped or removed, and no migration is added — unchanged from rounds 4 and 5.

Key	Backend	Change	Default
cockpit-broadcast-camera-actions-over-mavlink	vehicle-synced (useBlueOsStorage)	added	false
cockpit-mavlink-camera-target-id	vehicle-synced	added	0
cockpit-mavlink-video-stream-id	vehicle-synced	added	0
cockpit-mavlink-set-camera-mode-on-capture	vehicle-synced	added	false
cockpit-mavlink-request-capture-status	vehicle-synced	added	false

Judgement: all five carry the cockpit- prefix; all five describe this vehicle's cameras rather than this computer, so vehicle-synced is the right backend and no machine-specific value (device path, filesystem path, window geometry) is being synced. The stored shapes are a boolean or a small integer, with no id field duplicating its own key, and nothing writes undefined into one — the blur handlers write a clamped number and clampToUint8 maps a non-finite input to 0. The feature is off by default, so no already-configured user is stranded on an old value and nothing needs a migration.

One thing to keep in mind rather than fix: because these are vehicle-synced, a second topside computer changing the target camera changes it under the first one mid-recording. The stop path survives that (activeBroadcastTarget, head ~130), and the backend choice itself is correct.

(cockpit-mavlink-request-capture-status backs a variable named requestCaptureStatusOnCapture; the key drops the -on-capture. Harmless, and renaming a shipped key is worse than living with it — noted only so nobody "fixes" it later.)

Sections with nothing to report (10)

1. Correctness & Implementation Bugs — ✅ (re-walked the mirror lifecycle end to end: broadcastRecordingStart marks the stream before its early return so multi-stream recordings broadcast once, broadcastRecordingStop fires from onstop — the one point every ending reaches — and gates on Set.delete so an unmirrored stream sends nothing; the snapshot call site's succeeded.some((name) => streamNames.includes(name)) correctly excludes a workspace-only capture, checked against base src/stores/snapshot.ts:110-174; the two signal registrations live inside VehicleFactory.onVehicles.once so no listener stacks; the 9th sendCommandLong parameter is optional and none of its 8 in-tree callers passes one; 1.5's data-lake bypass is the only vehicle-store telemetry read and it is now maintainer-resolved)

3. AGENTS.md Adherence — ✅ (3.1 and 3.2 maintainer-resolved; no new dependency — package.json is not among the 8 changed files — JSDoc present and non-empty on every added function declaration, class method and interface member as jsdoc/require-jsdoc demands, yarn untouched, optional chaining used throughout the added TypeScript, no widget Options entries added so the default-merging pattern does not apply, and no Electron-only API touched so Lite parity is unaffected)

4. Security — ✅ (no new dependency and no postinstall, CI workflow, Dockerfile or Electron-main change; no fetch/XHR/WebSocket added, no eval/Function()/v-html, no env var or credential; the only encoded constants are the two CAMERA_CAP_FLAGS bit values at the top of src/libs/vehicle/mavlink/camera.ts, checked against their names; the diff is plain ASCII with no zero-width or bidi characters)

5. Performance — ✅ (the one addition to the onIncomingMessage hot path sits above the component_id !== 1 return at vehicle.ts:320 and is a two-case switch on a string discriminant, negligible beside the JSON.parse already done at :309; the per-message object spread in the two store slots runs at the requested 2 Hz while mirroring; the 1 Hz staleness ticker is armed and cleared on both edges of a watch on the master toggle, writes one timestamp ref, and lives in an app-lifetime Pinia store so it has no unmount to leak from; no canvas work, no blocking I/O, no bundle addition)

6. UI / UX — ✅ (5.1 maintainer-resolved; both new v-text-fields carry theme="dark", the paired id fields use a two-column grid collapsing via interfaceStore.isOnPhoneScreen, labels are sentence case, the mirror icon carries an aria-label and a tooltip, the five badge states are distinguished by icon, colour and text rather than colour alone, every new control logs through logUserAction from an @update:model-value / @update:focused handler rather than a watch so BlueOS settings-sync will not fire spurious entries, and no dialog, z-index or glass layer is added; also re-checked the "camera answered but reports idle" path — the panel and badge report idle truthfully rather than misleading the operator)

7. Code Quality & Style — ✅ (complexity-report.json reports 8 changed files, 427 functions measured, truncated: false and triggeredCount: 0 at head 3a4ab884, so no complexity or nesting finding is available to raise; no stray any, no comment deleted or reworded whose code is unchanged, no new scoped CSS, and neither video.ts (~1556) nor vehicle.ts (~1770) crosses the ~2000-line growth threshold; the pure MAVLink helpers live in src/libs/vehicle/mavlink/camera.ts with no vue import, so no domain logic was left in a .vue)

8. Commit Hygiene — ✅ (8.1 maintainer-resolved; the eight commits in pr.json are unchanged from round 5 and all scope-prefixed in this repository's style — lib:, stores:, views:, widgets:, matching the map: / base-station: / libs: prefixes on the last twelve commits of master — with no wip / fix lint / address review / un-squashed fixup! noise, no GitHub issue or PR reference in any message, a split that follows the dependency order rather than one commit per file, and a largest commit of ~150 lines)

9. Tests — ✅ (no test file among the 8 changed files; src/tests/ is untouched, and nothing was removed or weakened)

10. Documentation — ✅ (the PR body still matches the code — same head as round 5, where it was checked line by line against the six helpers, the interception, the caches, the derived state, the panel readout and the badge; the feature behaves identically in Lite and Standalone since it only sends MAVLink over the existing vehicle connection, so the README.md parity table needs no entry)

11. Nitpicks / Optional — ✅ (re-read the added TypeScript for the x && x.y pattern and found only optional chaining — remoteCameraCapabilities?.supportsVideo === false, mainVehicle.value?.send* — plus the destructured-defaults form in sendCameraCommand)

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

[rafaellehmkuhl]

@ArturoManzoli @joaoantoniocardoso this PR looks good already, so I just want us to settle on the broadcast thing.

The PR does not introduce any mechanism for mapping streams to MAVLink IDs, which means the only thing we cannot send proper "start THIS stream" messages, so the default is to broadcast instead (ID = 0).

The PR also allows the user to change the ID from 0 to something else, but for an user with a single camera this setting adds nothing, and for one with multiple cameras it's a bad UX as well (it has to discover the stream ID and set there every time they start a recording, which kills the "automatic" gain.

Is there any other scenario where allowing the user to change that ID makes sense?
If not, my proposal would be to just remove this ID setting from the PR and always send broadcast (ID=0), till we add a proper stream->ID mapping that can correctly send the message for specific streams.

[ArturoManzoli]

@ArturoManzoli @joaoantoniocardoso this PR looks good already, so I just want us to settle on the broadcast thing.

The PR does not introduce any mechanism for mapping streams to MAVLink IDs, which means the only thing we cannot send proper "start THIS stream" messages, so the default is to broadcast instead (ID = 0).

The PR also allows the user to change the ID from 0 to something else, but for an user with a single camera this setting adds nothing, and for one with multiple cameras it's a bad UX as well (it has to discover the stream ID and set there every time they start a recording, which kills the "automatic" gain.

Is there any other scenario where allowing the user to change that ID makes sense? If not, my proposal would be to just remove this ID setting from the PR and always send broadcast (ID=0), till we add a proper stream->ID mapping that can correctly send the message for specific streams.

As suggested by @joaoantoniocardoso, only the broadcast is now implemented, until we add the rest of the MAVLink camera protocol