video: generate a recording's telemetry subtitles when it has none

[rafaellehmkuhl]

Summary

A recording whose .ass subtitle file was never written could not get it back from the application, even with the telemetry it is built from still sitting in the browser. Downloading a chunk group on Cockpit Lite only looked the file up and reported Failed to find .ass telemetry file for the recording., while the Standalone chunk processing already knew how to regenerate the overlay — from the recording metadata, and failing that from the chunk timestamps — but reads its own telemetry storage, not the browser's, so exporting to Standalone does not recover a Lite recording either.

This moves that fallback into the candidate windows an overlay can be generated for (src/libs/video-telemetry.ts), and resolves the overlay the same way on both paths, generating it when the recording has none and storing it next to the recording so it also survives the download. The Standalone path keeps its behavior and loses its duplicate copy of the fallback, which is where the net deletion comes from.

Before	After

Test plan

• On Cockpit Lite, record a stream, stop it, and delete the recording's .ass from the cockpit-video-recovery-db IndexedDB store, leaving the chunks in place.
• Open the Video Library, go to Videos -> Raw and download that chunk group: the ZIP should carry the chunks plus a regenerated .ass, and the subtitle should play over the video once processed.
• Download a chunk group whose .ass does exist and confirm it is the stored file that ships, not a fresh one.
• Download a chunk group whose recording window has no logged telemetry (e.g. delete the entries in Settings -> Logs first): the download should still succeed, with a warning that it carries no subtitle file.
• On Standalone, process a leftover chunk group with and without an existing .ass, and confirm the overlay lands next to the MP4 in both cases.

Checks

• Recording with 3 chunks, 1280x720, 10 s of logged telemetry, subtitle removed. Before: cockpit-video-recovery-db keys [] and the red "Failed to find .ass telemetry file" snackbar. After: key Cockpit (...) #a1b2c3d4.ass, 7854 bytes, PlayResX: 1280 PlayResY: 720, 90 dialogue lines carrying the logged values, and no error snackbar.
• yarn lint:fix, yarn typecheck and the new src/tests/libs/video-telemetry.test.ts (5 cases) are clean. src/tests/libs/cosmos.test.ts and src/tests/libs/connection/connection.test.ts fail on master too, unrelated to this change.

Closes #3000

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 1)

4 open findings: 3 minor and 1 nit.

When you download the raw pieces of a recording from the video library in the browser version, Cockpit used to look for the telemetry subtitle file and, if the recording never got one, tell you it failed and hand you a download with no telemetry. This PR makes both the browser and the desktop version build that subtitle file on the spot from the telemetry Cockpit already logged — using the recording's own start and end time when it still knows them, and otherwise reconstructing the time span from the timestamps of the recorded pieces — and saves the result next to the recording so it is there next time too. The desktop version already had that fallback ladder; the change lifts it into a small shared module with unit tests and deletes the desktop-only copy.

What still needs attention

#	Problem	What it means	Severity	Status
1.1	Storage read failure now skips generation entirely	If Cockpit cannot list its stored files for a moment, the desktop version no longer falls back to building the subtitles, so the processed video comes out with none.	minor	❌
1.2	A failed save throws away subtitles that were already built	If the browser's storage is full, the subtitles Cockpit just produced are discarded instead of being put in the download the user asked for.	minor	❌
7.1	The place recordings normally get their subtitles was left out	Recordings that never stored their video size still get a subtitle file with an invalid size written into it, even though this PR adds the fallback that would fix it.	minor	❌
11.1	Whole subtitle file read through IPC just for its name	The desktop version loads a file it does not use before copying it, which is wasted work on every processed recording.	nit	❌

Change map — what was established before judging

Claims (from the commit body; the PR body is the same text)

• Symptom: "Downloading a chunk group on Lite only looked an .ass file up and gave up when the recording never got one." — verified, with one nuance: src/composables/videoChunkManager.ts:364-372 (base) looks the file up with findAssTelemetryFile and, when there is none, opens an error snackbar. The download itself continued — it produced a ZIP without subtitles rather than aborting.
• Cause: the recording never got an .ass because processing died before writing it. — verified as plausible and reachable: the only producer at recording time is generateTelemetryOverlay (src/stores/video.ts:788), called from src/stores/video.ts:1210 after processing; its failure is caught at src/stores/video.ts:1211 and reported, leaving the recording with chunks and no .ass.
• Cause: "Standalone regenerates the overlay while processing chunks, but reads its own telemetry, not the browser's." — verified: copyOrGenerateTelemetryOverlayFile (base src/composables/videoChunkManager.ts:524-575) is only reachable from processChunkGroup, which returns early unless isElectron().
• Mechanism: move the metadata-then-chunk-timestamps fallback into candidate windows and resolve the overlay the same way on both paths, storing what it generates. — verified: telemetryOverlayWindowCandidates reproduces tier 2 (recording.dateStart/dateFinish, vWidth ?? 1920, vHeight ?? 1080) and tier 3 (min/max of chunk epochs, +1000 on the end, 1920×1080), and resolveTelemetryOverlayFile is now called from both downloadChunkGroup and processChunkGroup.

Failure site — src/composables/videoChunkManager.ts:364-372 on the base ref, and it is in the diff. The diff replaces the else branch that only reported failure with a call to the new resolver, so a Lite download of a recording that has telemetry logged but no .ass now gets one. Checked the interrupted-recording case end to end on Lite: getChunkTimestamp (src/composables/videoChunkManager.ts:82-90) needs only dateStart, so when dateFinish is missing — the exact state a died-during-processing recording is left in — candidate 1 is skipped and candidate 2 still yields a real window. The fallback ladder is therefore reachable on Lite, not decorative.

Entry points

Function	Reached from	Frequency
resolveTelemetryOverlayFile (new)	downloadChunkGroup → onDownloadChunkGroup (src/components/VideoLibraryModal.vue:1254), and processChunkGroup → onProcessChunkGroup (src/components/VideoLibraryModal.vue:1249) — both button clicks in the video library's chunk list	per user action
downloadChunkGroup (changed)	src/components/VideoLibraryModal.vue:1254, button click	per user action
processChunkGroup (changed)	src/components/VideoLibraryModal.vue:1249, button click	per user action
telemetryOverlayWindowCandidates (new, src/libs/video-telemetry.ts)	resolveTelemetryOverlayFile only	per user action

Both call sites already log the interaction (logUserAction at src/components/VideoLibraryModal.vue:1250 and :1255), so no new logging is owed.

Invariants

• An .ass for a recording is findable by "key contains the hash and ends in .ass". Consumers: findAssTelemetryFile (src/composables/videoChunkManager.ts:286-295), src/electron/services/video-recording.ts:672, and the library listing at src/components/VideoLibraryModal.vue:1124. Producers: src/stores/video.ts:809, the ZIP extraction path (src/electron/services/video-recording.ts:445), and now resolveTelemetryOverlayFile. The name it writes, videoSubtitlesFilename(group.fileName || videoFilename(group.hash, group.firstChunkDate)), always embeds #<hash> via videoFilename (src/utils/video.ts:17), so the invariant holds and a second download reuses the generated file instead of regenerating it. ChunkGroup.fileName is never assigned anywhere in the tree — fetchChunkGroups builds the group without it — so that first operand is dead, but it is inherited from the base and not something this PR introduced.
• The candidate loop can only advance when a failed window throws. datalogger.generateLog does throw on an empty window (src/libs/sensors-logging.ts:523-525, 'No log points found in the given range.'), so falling from the metadata window to the chunk-timestamp window works as written.

1. Correctness & Implementation Bugs — 2 findings

1.1 — minor — the existing-file lookup sits outside the fallback's error handling (src/composables/videoChunkManager.ts, added resolveTelemetryOverlayFile, first two lines)

On the base ref, tier 1 wrapped the storage listing in its own try/catch (src/composables/videoChunkManager.ts:527-539): a videoStorage.keys() failure was logged and execution fell through to tier 2, which generated the overlay. In the new resolver, await findAssTelemetryFile(group.hash) runs before the loop with no guard, and findAssTelemetryFile performs both a keys() and a getItem() — on Standalone both are IPC round-trips through ElectronStorage (src/libs/videoStorage.ts:43-61). Any rejection there now aborts the whole resolution: processChunkGroup reports Could not generate telemetry overlay for the processed video: … and the processed video keeps no subtitles, where before it would have been generated from the recording metadata.

Fix: put the lookup inside a try/catch that falls through to the candidate loop, the way tier 1 did — the loop is already the "everything else failed" path.

Consequence: if Cockpit momentarily cannot list its stored video files, the desktop version stops producing telemetry subtitles for a processed recording instead of rebuilding them.

1.2 — minor — a storage write failure discards an overlay that was already generated successfully (src/composables/videoChunkManager.ts, added resolveTelemetryOverlayFile, candidate loop)

const assContent = datalogger.toAssOverlay(telemetryLog, candidate.width, candidate.height, startEpoch)
const blob = new Blob([assContent], { type: 'text/plain' })
await videoStore.videoStorage.setItem(subtitlesFileName, blob)
return { blob, filename: subtitlesFileName }

setItem is inside the same try that decides whether the candidate window worked, so a storage failure — an IndexedDB quota error on Lite is the realistic one, since the same database holds recovered recordings — is treated as "this window produced nothing". The loop then retries with the next window, whose setItem fails identically, and the caller ends up in the catch at the download site: the ZIP is written without the subtitle file that was sitting in blob a moment earlier. The persistence is a bonus the commit message describes as "so the file also survives the download"; it should not be able to cancel the download's own content.

Fix: catch around the setItem alone (log and continue to return), so the descriptor is returned whether or not the copy could be persisted. On the Standalone path the subsequent copyTelemetryFile will then fail on its own and be reported, which is the accurate error rather than "no telemetry was logged".

Consequence: when the browser's storage is full, the user's chunk download loses its telemetry subtitles even though Cockpit had already built them.

2. Persistence & User Data — inventory, no findings

Key / store	Backend	What happened
<recording name>.ass in videoStorage	Machine-local. Electron: the videos/ folder via ElectronStorage (src/libs/videoStorage.ts:137,142). Lite: the Cockpit - Video Recovery IndexedDB (src/libs/videoStorage.ts:116-121).	New write site. The key shape is unchanged (videoSubtitlesFilename(...), the same helper src/stores/video.ts:808 uses) and the Standalone path already wrote it here (base saveAndCopyTelemetry, src/composables/videoChunkManager.ts:510); what is new is that the Lite download path writes it too.
Cockpit (…).clog in the Cockpit - Sensor Logs IndexedDB	Machine-local (src/libs/sensors-logging.ts:293).	Indirect, unchanged shape. datalogger.generateLog persists the generated log as a side effect (src/libs/sensors-logging.ts:551), so a Lite chunk download now also leaves one telemetry log file behind — the same thing the Standalone processing path has always done.

Judged: both are media/log artifacts rather than cockpit--prefixed settings, so the cockpit- naming rule and useBlueOsStorage do not apply; neither is machine-specific data being vehicle-synced; no key is reshaped, no value duplicates its own key, and the PR adds no migration and changes no default. The generated .ass surfaces in the video library's log-file list (src/components/VideoLibraryModal.vue:1124), where the user can already delete it, so it is not an invisible accumulation.

7. Code Quality & Style — 1 finding

Complexity: the report attributes one trigger to this PR — downloadChunkGroup at src/composables/videoChunkManager.ts:335, nested-4-or-deeper, depth 5 against a base depth of 5 and complexity 13 against a base complexity of 13. Depth equals base depth, so the nesting is inherited and the trigger is answered; the PR's own contribution there is one try/catch replacing an if/else. No complexity finding.

7.1 — minor — the recording-time overlay generator still carries its own copy of the window-and-resolution decision (src/stores/video.ts:788-815)

The PR extracts "which window and which resolution do we render this recording's overlay for" into telemetryOverlayWindowCandidates, including a documented 1920×1080 fallback for "recordings made before the resolution was stored". generateTelemetryOverlay — the site where overlays are normally produced, called after every recording finishes (src/stores/video.ts:1210) — makes the same decision inline and asserts the values away instead:

const telemetryLog = await datalogger.generateLog(recordingData.dateStart!, recordingData.dateFinish!)
const assLog = datalogger.toAssOverlay(telemetryLog, recordingData.vWidth!, recordingData.vHeight!, …)

vWidth/vHeight are genuinely optional — the new module and its test (falls back to a standard resolution when the recording did not store one) exist because of it — and toAssOverlay interpolates them straight into the header (src/libs/sensors-logging.ts:607-608), so an undefined one writes PlayResX: undefined into the file. Per AGENTS.md "Reuse before reinventing" ("If the same logic would live in two or more places, extract it once and reuse it"), this third copy is the one that should have been switched to the new helper — it is a two-line change at src/stores/video.ts:797-805 reading the first candidate, and it is what makes the extraction pay for itself rather than adding a second implementation.

Consequence: a recording that never stored its video size still gets a subtitle file declaring an invalid size, so players scale the telemetry overlay wrongly or refuse it — and the fallback this PR introduces does not reach that case.

11. Nitpicks / Optional — 1 finding

11.1 — nit — the Standalone path downloads a blob it never uses (src/composables/videoChunkManager.ts, resolveTelemetryOverlayFile / processChunkGroup)

findAssTelemetryFile reads the file's contents (videoStore.videoStorage.getItem(assFile), src/composables/videoChunkManager.ts:292), but processChunkGroup destructures only { filename } and hands it to copyTelemetryFile, which copies from disk. Base tier 1 matched on the key alone and never read the blob (src/composables/videoChunkManager.ts:529-534). For a long recording the .ass is a few hundred kilobytes of text pulled over IPC and thrown away. Not worth a new lookup function; returning the key and reading the blob only on the download path would cover it if you touch this again.

Sections with nothing to report (7)

3. AGENTS.md Adherence — ✅ (no new dependency and package.json untouched; the added JSDoc blocks on resolveTelemetryOverlayFile, telemetryOverlayWindowCandidates and every TelemetryOverlayWindow member carry typed @param/@returns with real text; domain logic moved out of the composable into src/libs/video-telemetry.ts as the separation-of-concerns rule asks; both added comments explain why; no renames, import reorders or unrelated reflows in the diff; the one exported symbol has a call site in this PR)

4. Security — ✅ (read src/libs/video-telemetry.ts and the test file in full: plain arithmetic on Date, no network call, no eval/Function, no encoded blobs, no hidden or bidirectional Unicode, no environment variables or credentials; no dependency, workflow, postinstall or Electron main-process file is touched, and the only Electron API used, copyTelemetryFile, sits behind the existing isElectron() && window.electronAPI guard at the top of processChunkGroup)

5. Performance — ✅ (both changed functions trace only to video-library button clicks, never to onIncomingMessage, setVariable or a watcher; the generation runs at most twice per click and generateLog reads a bounded IDBKeyRange window rather than the whole store; no listener, interval or watcher is registered, so nothing needs teardown, and no canvas work is added)

6. UI / UX — ✅ (no dialog, control, overlay or icon is added or restyled, so the dialog-anatomy, theme="dark", button-token, footer, padding, glass and stacking clauses have nothing to bind to; the failure path gained visible feedback as a warning snackbar in place of the old error one, the success path keeps its snackbar, no snackbar is paired with a duplicate console.*, nothing can open a dialog in a loop, and both entry points already call logUserAction at src/components/VideoLibraryModal.vue:1250,1255)

8. Commit Hygiene — ✅ (one commit, c8b3d72; the video: scope prefix matches the dominant style in git log and fits the change; no wip/fixup!/AI-authorship noise, no #N or closing keyword in the message, no self-correcting or replicated commit, and the 266 changed lines are one logical change of which 93 are the deletion it replaces)

9. Tests — ✅ (src/tests/libs/video-telemetry.test.ts is new and sits where the tree puts library tests; no existing test was removed, skipped or weakened, and the five added cases assert observable outputs rather than internals)

10. Documentation — ✅ (the change narrows a Lite/Standalone gap instead of adding a Lite-only limitation, so the README parity table needs no row; JSDoc is added on both new public functions and on the new interface)

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

[rafaellehmkuhl]

Review follow-up — round 1

Done

• src/composables/videoChunkManager.ts (1.1 — existing-file lookup outside the fallback's error handling): findAssTelemetryFile now runs inside its own try/catch that logs and falls through to the candidate loop, so a keys()/getItem() rejection regenerates the overlay the way tier 1 used to instead of aborting the whole resolution. Folded into 7a3bee6.
• src/composables/videoChunkManager.ts (1.2 — a failed save discards an overlay that was already built): the videoStorage.setItem has its own try/catch and the descriptor is returned either way, so a quota error costs only the persistence, not the subtitle file the download asked for. On Standalone the copyTelemetryFile that follows then fails on its own and reports the accurate error. Folded into 7a3bee6.
• src/stores/video.ts (7.1 — the place recordings normally get their subtitles was left out): generateTelemetryOverlay reads the window and the resolution from telemetryOverlayWindowCandidates, which drops its four non-null assertions and the PlayResX: undefined header a recording without a stored size used to get. It is a fix to existing behavior rather than part of the feature, so it is its own commit, a915b43.

Won't change (with reasoning)

• 11.1 — the Standalone path reads a blob it only needs the name of: both download batches need the bytes, so FileDescriptor is the contract that fits the callers. Narrowing the resolver to a key would make the Lite path re-read from storage an overlay it had just built in memory, trading an IPC read on Standalone for an IndexedDB read on Lite plus a branchier signature. Worth revisiting if that read ever shows up in a profile.

[rafaellehmkuhl]

/review

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 2)

1 open finding, a nit the author disputes; 3 closed since round 1.

When you download the raw pieces of a recording from the video library, Cockpit used to look for the telemetry subtitle file and, if the recording never got one, tell you it failed and hand you a download with no telemetry. This PR makes both the browser and the desktop version build that subtitle file on the spot from the telemetry Cockpit already logged — using the recording's own start and end time when it still knows them, and otherwise reconstructing the time span from the timestamps of the recorded pieces — and saves the result next to the recording so it is there next time too. A second commit added this round points the place recordings normally get their subtitles at the same shared helper, so a recording that never stored its video size no longer gets a subtitle file declaring an invalid one.

What still needs attention

#	Problem	What it means	Severity	Status
11.1	Whole subtitle file read through IPC just for its name	The desktop version loads a file it does not use before copying it, which is wasted work on every processed recording.	nit	💬

Since round 1 — 3 closed, 1 disputed, comparing c8b3d72 → a915b43

The range could not be used. incremental.diff reproduces the entire PR diff rather than the increment: it carries all four files at their full +198/-97, byte-for-byte the content of pr.diff minus the file headers. The round-1 head c8b3d72 no longer exists on the branch — the author folded the round-1 fixes into 7a3bee6 (an amend/rewrite of that commit) and added a915b43 on top — so the compare had no common ancestor to subtract. Every status below is therefore judged against pr.diff at a915b43 and the code round 1 quoted, not against the increment.

No /resolve command and no decision vote has been recorded on this PR, so nothing was settled by a maintainer this round.

1.1 — ✅ Addressed. The finding asked for the existing-file lookup to sit inside a try/catch that falls through to the candidate loop, the way base tier 1 did. resolveTelemetryOverlayFile now opens with exactly that: findAssTelemetryFile(group.hash) inside a try, an early return only on a hit, and a console.warn in the catch that lets execution continue into the candidate loop. A keys()/getItem() rejection on Standalone regenerates the overlay instead of aborting the resolution.

1.2 — ✅ Addressed. The finding asked for a catch around the setItem alone so the descriptor is returned whether or not the copy persists. The loop now builds overlay in one try (whose catch continues to the next window, which is the intended meaning of "this window produced nothing"), then persists it in a second try whose catch only warns, then returns { blob: overlay, filename: subtitlesFileName } unconditionally. A quota error now costs the persistence and not the download's subtitle file.

7.1 — ✅ Addressed. The finding asked for generateTelemetryOverlay to read its window and resolution from the new helper rather than keeping a third inline copy with vWidth!/vHeight!. src/stores/video.ts:795-805 now calls telemetryOverlayWindowCandidates(recordingData), takes the first candidate, and passes overlayWindow.width/height/start to toAssOverlay; all four non-null assertions are gone, and a recording with no stored resolution gets the documented 1920×1080 instead of PlayResX: undefined. It rides in its own commit, a915b43, which is what AGENTS.md's "behavior changes ride alone" asks for. Checked for a regression on the way in: the new if (!overlayWindow) throw can only fire where dateStart/dateFinish are missing, and on that path generateLog previously raised a TypeError on an undefined date that reached the user through the same error snackbar at src/stores/video.ts:1212.

11.1 — 💬 Disputed, still open. rafaellehmkuhl declined it in the round-1 follow-up comment (link), arguing in plain terms that the download path needs the file's bytes so a file descriptor is the contract that fits the callers, and that returning only a key would make the Lite path re-read from storage an overlay it had just built in memory. Verified against the diff: downloadChunkGroup does use assFile.blob, processChunkGroup does use only filename, so both halves of the trade are real; the argument is a reason not to change the code, which is not the same as the code having changed, so the finding stays open per the re-review rules. One detail of the argument does not hold — the .ass is added only to the first batch (if (i === 0)), not to "both download batches" — but it does not affect the trade.

Discussion since round 1. The only other new comment is the bare /review that triggered this run. The follow-up comment's three "Done" claims were each checked against the code above rather than taken at face value; all three hold.

Change map — what was established before judging

Claims (two commits now; the PR body is the first commit's text)

• Symptom: "Downloading a chunk group on Lite only looked an .ass file up and gave up when the recording never got one." — verified, with one nuance: src/composables/videoChunkManager.ts:364-372 (base) looks the file up with findAssTelemetryFile and, when there is none, opens an error snackbar. The download itself continued — it produced a ZIP without subtitles rather than aborting.
• Cause: the recording never got an .ass because processing died before writing it. — verified as reachable: the only producer at recording time is generateTelemetryOverlay (src/stores/video.ts:788), called from the recorder's onstop at src/stores/video.ts:1210; its failure is caught at :1211 and reported, leaving the recording with chunks and no .ass.
• Cause: "Standalone regenerates the overlay while processing chunks, but reads its own telemetry, not the browser's." — verified: copyOrGenerateTelemetryOverlayFile (base src/composables/videoChunkManager.ts:524-575) is only reachable from processChunkGroup, which returns early unless isElectron() (:583).
• Mechanism: move the metadata-then-chunk-timestamps fallback into candidate windows and resolve the overlay the same way on both paths, storing what it generates. — verified: telemetryOverlayWindowCandidates reproduces base tier 2 (recording.dateStart/dateFinish, vWidth ?? 1920, vHeight ?? 1080) and tier 3 (min/max of chunk epochs, +1000 on the end, 1920×1080), and resolveTelemetryOverlayFile is called from both downloadChunkGroup and processChunkGroup.
• Second commit — symptom: "the overlay written when a recording finishes asserted the stored video size away, so a recording that never got one wrote PlayResX: undefined". — verified: base src/stores/video.ts:802-803 passes recordingData.vWidth!/vHeight!, vWidth/vHeight are optional on CommonVideoInfo (src/types/video.ts:125-129), and toAssOverlay interpolates them straight into the header (src/libs/sensors-logging.ts:607-608).
• Second commit — mechanism: "read the window and the resolution from the same candidate helper the chunk recovery uses". — verified: src/stores/video.ts:798-805 at head.

Failure sites — two, both in the diff. The Lite download gap lives at src/composables/videoChunkManager.ts:364-372 on the base ref, and the diff replaces the reporting-only else with the resolver. The invalid-resolution header lives at src/stores/video.ts:797-805 on the base ref, and the second commit replaces it. Checked the interrupted-recording case end to end on Lite: getChunkTimestamp (src/composables/videoChunkManager.ts:69-92) needs only dateStart, so when dateFinish is missing — the exact state a died-during-processing recording is left in — candidate 1 is skipped and candidate 2 still yields a real window, making the ladder reachable rather than decorative.

Entry points

Function	Reached from	Frequency
resolveTelemetryOverlayFile (new)	downloadChunkGroup → onDownloadChunkGroup (src/components/VideoLibraryModal.vue:1254) and processChunkGroup → onProcessChunkGroup (:1249), both button clicks in the video library's chunk list	per user action
downloadChunkGroup (changed)	src/components/VideoLibraryModal.vue:1254, button click	per user action
processChunkGroup (changed)	src/components/VideoLibraryModal.vue:1249, button click	per user action
telemetryOverlayWindowCandidates (new, src/libs/video-telemetry.ts)	resolveTelemetryOverlayFile and generateTelemetryOverlay	per user action
generateTelemetryOverlay (changed this round)	mediaRecorder.onstop (src/stores/video.ts:1169) via src/stores/video.ts:1210 — fires once when a recording ends, from the Stop button, stream teardown or a dropped link	per user action

All three call sites already log the interaction (logUserAction at src/components/VideoLibraryModal.vue:1250,1255 for the two chunk-list buttons, and src/components/mini-widgets/MiniVideoRecorder.vue:339,365 for the recording whose end reaches onstop), so no new logging is owed.

Invariants

• An .ass for a recording is findable by "key contains the hash and ends in .ass". Consumers: findAssTelemetryFile (src/composables/videoChunkManager.ts:286-295), src/electron/services/video-recording.ts:672, and the library listing at src/components/VideoLibraryModal.vue:1124. Producers: src/stores/video.ts:809, the ZIP-extraction path (src/electron/services/video-recording.ts:445-446), and now resolveTelemetryOverlayFile. The name it writes, videoSubtitlesFilename(group.fileName || videoFilename(group.hash, group.firstChunkDate)), always embeds #<hash> via videoFilename (src/utils/video.ts:13-18), so the invariant holds and a second download reuses the generated file instead of regenerating it.
• Two writers can disagree on the key for one recording. The store writes videoSubtitlesFilename(recordingData.fileName), where fileName came from videoFilename(hash, timeRecordingStart, safeMissionName) (src/stores/video.ts:932), while the resolver defaults the mission name to Cockpit and uses firstChunkDate. Downloading the chunks of a recording that has not stopped yet can therefore leave a second .ass for the same hash, after which findAssTelemetryFile returns whichever key keys() yields first. Checked and not raised: the base Standalone path (saveAndCopyTelemetry, base src/composables/videoChunkManager.ts:510) already wrote under the resolver's naming, so the possibility predates this PR, and on the flow the PR targets — a finished recording whose overlay was never written — only one writer ever runs.
• The candidate loop can only advance when a failed window throws. datalogger.generateLog does throw on an empty window (src/libs/sensors-logging.ts:523-525), so falling from the metadata window to the chunk-timestamp window works as written.
• The recording-finish path always has a first candidate. onstop sets info.dateFinish at src/stores/video.ts:1186, two statements before generateTelemetryOverlay runs, so the new if (!overlayWindow) throw at :798 is unreachable on the normal path and only replaces the TypeError an undefined date used to raise inside generateLog.

2. Persistence & User Data — inventory, no findings

Key / store	Backend	What happened
<recording name>.ass in videoStorage	Machine-local. Electron: the videos/ folder via ElectronStorage (src/libs/videoStorage.ts:137,142). Lite: the Cockpit - Video Recovery IndexedDB (src/libs/videoStorage.ts:116-121).	New write site, unchanged key shape (videoSubtitlesFilename(...), the same helper src/stores/video.ts:808 uses). The Standalone path already wrote here (base saveAndCopyTelemetry); what is new is that the Lite download path writes it too. This round's second commit changes only the contents of the store's own write — the resolution in the header — not the key.
Cockpit (…).clog in the Cockpit - Sensor Logs IndexedDB	Machine-local (src/libs/sensors-logging.ts:293).	Indirect, unchanged shape. datalogger.generateLog persists the generated log as a side effect (src/libs/sensors-logging.ts:551), so a Lite chunk download now also leaves one telemetry log file behind — the same thing the Standalone processing path has always done.

Judged: both are media/log artifacts rather than cockpit--prefixed settings, so the cockpit- naming rule and useBlueOsStorage do not apply; neither is machine-specific data being vehicle-synced; no key is reshaped, no value duplicates its own key, and the PR adds no migration and changes no default. The generated .ass surfaces in the video library's log-file list (src/components/VideoLibraryModal.vue:1124), where the user can already delete it, so it is not an invisible accumulation.

11. Nitpicks / Optional — 1 finding

11.1 — nit — the Standalone path downloads a blob it never uses (carried from round 1, disputed) (src/composables/videoChunkManager.ts, resolveTelemetryOverlayFile / processChunkGroup)

findAssTelemetryFile reads the file's contents (videoStore.videoStorage.getItem(assFile), src/composables/videoChunkManager.ts:292), but processChunkGroup destructures only { filename } and hands it to copyTelemetryFile, which copies from disk. Base tier 1 matched on the key alone and never read the blob (base src/composables/videoChunkManager.ts:529-534). For a long recording the .ass is a few hundred kilobytes of text pulled over IPC and thrown away. Not worth a new lookup function; returning the key and reading the blob only on the download path would cover it if you touch this again.

The author declined it, reasoning that the download path needs the bytes so a file descriptor is the contract that fits both callers, and that returning a key would only move the read to IndexedDB on Lite. That reasoning is sound as a trade; per the re-review rules an argument leaves the finding open rather than closing it, and at nit it is not something to hold the merge for.

Consequence: on the desktop version, every processed recording that already has subtitles reads the whole subtitle file over IPC before copying it, which is work nobody needs.

Sections with nothing to report (9)

1. Correctness & Implementation Bugs — ✅ (re-read resolveTelemetryOverlayFile at head: both storage calls now sit in their own try/catch, the loop continues only when the window itself failed, and the built overlay is returned whether or not the write lands; traced the second commit's new throw at src/stores/video.ts:798 back to onstop, where dateFinish is set at :1186 two statements earlier, so it replaces a TypeError rather than adding a failure; no widget reads telemetry from a Pinia store, no widget Options entry is touched, copyTelemetryFile stays behind the isElectron() && window.electronAPI guard at src/composables/videoChunkManager.ts:583, and the added TypeScript uses ?./?? throughout)

3. AGENTS.md Adherence — ✅ (no new dependency and package.json untouched; the added JSDoc blocks on resolveTelemetryOverlayFile, telemetryOverlayWindowCandidates and every TelemetryOverlayWindow member carry typed @param/@returns with real text, satisfying the jsdoc/require-jsdoc TSPropertySignature context in .eslintrc.cjs; domain logic lives in src/libs/video-telemetry.ts per separation of concerns; this round's second commit removes the third copy of the window/resolution decision rather than adding one, which is what "Reuse before reinventing" asks; no renames, import reorders or unrelated reflows, and both exported symbols have call sites in this PR)

4. Security — ✅ (read src/libs/video-telemetry.ts and src/tests/libs/video-telemetry.test.ts in full and the changed hunks of the other two files: plain arithmetic on Date, no network call, no eval/Function, no encoded blobs, no hidden or bidirectional Unicode, no environment variables or credentials; no dependency, workflow, postinstall script or Electron main-process file is touched; nothing in pr.json, the diff or the new comments contains text addressed to this reviewer)

5. Performance — ✅ (all three changed functions trace to a button click or to mediaRecorder.onstop, never to onIncomingMessage, dataLake:setVariable or a watcher; generation runs at most twice per click and generateLog reads a bounded IDBKeyRange window (src/libs/sensors-logging.ts:517-521) rather than the whole store; no listener, interval or watcher is registered, so nothing needs teardown, and no canvas work is added)

6. UI / UX — ✅ (no dialog, control, overlay or icon is added or restyled, so the dialog-anatomy, theme="dark", button-token, footer, padding, glass and stacking clauses have nothing to bind to; the Lite failure path gained visible feedback as a warning snackbar in place of the old error one, the success path keeps its snackbar, no snackbar is paired with a duplicate console.*, nothing can open a dialog in a loop, and both entry points already call logUserAction. The one new user-visible string, Downloading the chunks without a telemetry subtitle file. ${error}, interpolates a message carrying the recording hash — the same ${error} pattern every other snackbar in that composable already uses, so not raised as a departure)

7. Code Quality & Style — ✅ (the complexity report attributes one trigger to this PR — downloadChunkGroup at src/composables/videoChunkManager.ts:349, nested-4-or-deeper, with the report's figures being complexity 13 against a base 13 and depth 5 against a base depth 5; depth equals base depth, so the nesting is inherited and the trigger is answered, and the PR's own contribution there is one try/catch replacing an if/else. The report measured 159 functions across the 4 changed files and is not truncated. Beyond that: 7.1's fix removes a duplicated decision instead of adding one, no any or stray type is introduced, no existing comment is reworded over unchanged code, the two added comments explain why, and the added lines fit the 120-column prettier width and the 180-column max-len)

8. Commit Hygiene — ✅ (two commits, 7a3bee6 and a915b43; the round-1 fixes were folded into 7a3bee6 rather than left as an "address review" commit, which is exactly what AGENTS.md's fixup rule asks, and the behavior fix rides alone in a915b43 as its own rule asks; both video: prefixes match the dominant style in git log and describe their change; no wip/fixup!/AI-authorship trailer, no #N or closing keyword in either message, no self-correcting or replicated commit, and neither is oversized)

9. Tests — ✅ (src/tests/libs/video-telemetry.test.ts still carries the same five cases unchanged this round and sits where the tree puts library tests; no existing test was removed, skipped or weakened, and the cases assert observable outputs rather than internals)

10. Documentation — ✅ (the change narrows a Lite/Standalone gap instead of adding a Lite-only limitation, so the README parity table needs no row; JSDoc is present on both new public functions and on the new interface, and the store change needs none since generateTelemetryOverlay's existing block stays accurate)

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

[github-actions]

🙋 Decision needed — 11.1

Standalone path reads the .ass blob over IPC only to use its filename

The author's argument: Both callers are served by returning the whole file descriptor because the download path needs the bytes, and narrowing the resolver to a key would only trade an IPC read on Standalone for an IndexedDB re-read on Lite of an overlay it had just built in memory, plus a branchier signature.

How to vote on this dispute

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

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

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

[rafaellehmkuhl]

Review follow-up — round 2

Nothing actionable this round: 1.1, 1.2 and 7.1 were confirmed addressed at a915b43, and 11.1 was settled by vote in favour of leaving the resolver returning the whole file descriptor, so no code changed and nothing was pushed. Re-review triggered only to apply that vote.

One correction to my round-1 reasoning, for the record: the .ass goes into the first ZIP batch only (if (i === 0)), not into every batch — which does not affect the trade it described.

[rafaellehmkuhl]

/review

[rafaellehmkuhl]

/review

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 3)

1 open finding, raised this round; 4 closed, one of them settled by maintainer vote.

When you download the raw pieces of a recording from the video library, Cockpit used to look for the telemetry subtitle file and, if the recording never got one, tell you it failed and hand you a download with no telemetry. This PR makes both the browser and the desktop version build that subtitle file on the spot from the telemetry Cockpit already logged — using the recording's own start and end time when it still knows them, and otherwise reconstructing the time span from the timestamps of the recorded pieces — and saves the result next to the recording so it is there next time too. A second commit points the place recordings normally get their subtitles at the same shared helper, so a recording that never stored its video size no longer gets a subtitle file declaring an invalid one.

What still needs attention

#	Problem	What it means	Severity	Status
1.3	Desktop reports success when the rebuilt subtitle file never reaches the disk	On the desktop version, if saving the rebuilt subtitle file fails, Cockpit still says the video processed successfully while the video quietly ends up with no telemetry subtitles.	minor	❌

Since round 2 — 1 settled by vote, 1 new finding, no new commits (head unchanged at a915b43)

The range is empty, and legitimately so. PREV_SHA and HEAD_SHA are both a915b43, incremental.diff is a zero-byte file, and pr.json still lists the same two commits (7a3bee6, a915b43) it listed in round 2. Nothing was pushed since the last review, so every status below is judged against pr.diff at a915b43 — the same code round 2 read. This run exists because a decision arrived on a disputed finding, which is the one case an unchanged head still has work to do.

11.1 — ☑️ Resolved by vote. rafaellehmkuhl accepted the author's argument on the decision comment (link); the tally was gated against the carried ledger, so it answers the argument the ledger was actually showing. The resolver keeps returning the whole file descriptor, and the finding leaves the open set. No code changed for it, which is exactly what the vote is for.

No /resolve command has been issued on this PR — resolutions.json is empty — so nothing else was settled by hand this round, and there is no unknown finding id to report back.

1.1, 1.2 and 7.1 stay closed. They were confirmed addressed at a915b43 in round 2 and the code under them is byte-for-byte the same this round; they are carried in the ledger as addressed and are not re-judged.

1.3 — new, minor. Re-running the sections over the whole diff rather than over the (empty) increment surfaced one thing round 2 missed, and it follows directly from the fix round 1 asked for in 1.2: swallowing the setItem failure is right for the download path, which already holds the blob, but the same resolver is the Standalone processing path's only way of getting the file onto disk. Written out in section 1.

Discussion since round 2. rafaellehmkuhl left a follow-up (link) stating that nothing was pushed and that the re-review was triggered only to apply the vote — both checked and true (incremental.diff empty, commit list unchanged, decisions.json carrying the accept). Their correction that the .ass goes into the first ZIP batch only also checks out: pr.diff guards it with if (i === 0). The other two comments are bare /review triggers and carry nothing to act on.

Change map — what was established before judging

Claims (the PR body is the first commit's text; the second commit's message carries its own)

• Symptom: "Downloading a chunk group on Lite only looked an .ass file up and gave up when the recording never got one." — verified, with one nuance: src/composables/videoChunkManager.ts:362-372 (base) looks the file up with findAssTelemetryFile and, when there is none, opens an error snackbar. The download itself continued — it produced a ZIP without subtitles rather than aborting.
• Cause: the recording never got an .ass because processing died before writing it. — verified as reachable: the only producer at recording time is generateTelemetryOverlay (src/stores/video.ts:788), called from the recorder's onstop at src/stores/video.ts:1210; its failure is caught at :1211 and reported, leaving the recording with chunks and no .ass.
• Cause: "Standalone regenerates the overlay while processing chunks, but reads its own telemetry, not the browser's." — verified: copyOrGenerateTelemetryOverlayFile (base src/composables/videoChunkManager.ts:524-575) is only reachable from processChunkGroup, which returns early unless isElectron() && window.electronAPI (:583). Note the Standalone download never reaches the new code either — downloadChunkGroup takes the createVideoChunksZip branch at :306-316 — so the resolver's download half is a Lite-only path.
• Mechanism: move the metadata-then-chunk-timestamps fallback into candidate windows and resolve the overlay the same way on both paths, storing what it generates. — verified: telemetryOverlayWindowCandidates reproduces base tier 2 (recording.dateStart/dateFinish, vWidth ?? 1920, vHeight ?? 1080) and tier 3 (min/max of chunk epochs, +1000 on the end, 1920×1080), and resolveTelemetryOverlayFile is called from both downloadChunkGroup and processChunkGroup.
• Second commit — symptom: the recording-finish overlay asserted the stored video size away and wrote PlayResX: undefined. — verified: base src/stores/video.ts:802-803 passes recordingData.vWidth!/vHeight!, both optional on CommonVideoInfo (src/types/video.ts:125-129), and toAssOverlay interpolates them straight into the header (src/libs/sensors-logging.ts:607-608).

Failure sites — two, both in the diff. The Lite download gap lives at src/composables/videoChunkManager.ts:362-372 on the base ref, and the diff replaces the reporting-only else with the resolver. The invalid-resolution header lives at src/stores/video.ts:797-805 on the base ref, and the second commit replaces it. Re-checked the interrupted-recording case end to end on Lite: getChunkTimestamp (src/composables/videoChunkManager.ts:69-92) needs only dateStart, so when dateFinish is missing — the exact state a died-during-processing recording is left in — candidate 1 is skipped and candidate 2 still yields a real window, making the ladder reachable rather than decorative.

Entry points

Function	Reached from	Frequency
resolveTelemetryOverlayFile (new)	downloadChunkGroup → onDownloadChunkGroup (src/components/VideoLibraryModal.vue:1254) and processChunkGroup → onProcessChunkGroup (:1249), both button clicks in the video library's chunk list	per user action
downloadChunkGroup (changed)	src/components/VideoLibraryModal.vue:1254, button click	per user action
processChunkGroup (changed)	src/components/VideoLibraryModal.vue:1249, button click	per user action
telemetryOverlayWindowCandidates (new, src/libs/video-telemetry.ts)	resolveTelemetryOverlayFile and generateTelemetryOverlay	per user action
generateTelemetryOverlay (changed)	mediaRecorder.onstop (src/stores/video.ts:1169) via :1210 — fires once when a recording ends, from the Stop button, stream teardown or a dropped link	per user action

All three call sites already log the interaction (logUserAction at src/components/VideoLibraryModal.vue:1250,1255, and src/components/mini-widgets/MiniVideoRecorder.vue:339,365 for the recording whose end reaches onstop), so no new logging is owed.

Invariants

• On Standalone, the file copyTelemetryFile is asked for exists in the videos folder. The resolver is now the only producer for that path, and its setItem failure is caught and only warned (pr.diff, the second try in resolveTelemetryOverlayFile), while the main-process copy swallows its own error too (src/electron/services/video-recording.ts:552-562, catch → console.warn). No site left covers the violation — this is finding 1.3.
• An .ass for a recording is findable by "key contains the hash and ends in .ass". Consumers: findAssTelemetryFile (src/composables/videoChunkManager.ts:286-295), src/electron/services/video-recording.ts:672, and the library listing at src/components/VideoLibraryModal.vue:1124. Producers: src/stores/video.ts:809, the ZIP-extraction path (src/electron/services/video-recording.ts:445-446), and now resolveTelemetryOverlayFile. The name it writes always embeds #<hash> via videoFilename (src/utils/video.ts:13-18), so the invariant holds and a second download reuses the generated file.
• Two writers can disagree on the key for one recording. The store writes videoSubtitlesFilename(recordingData.fileName) while the resolver defaults the mission name to Cockpit and uses firstChunkDate. Checked and not raised: the base Standalone path already wrote under the resolver's naming, so the possibility predates this PR, and on the flow the PR targets only one writer ever runs.
• The candidate loop can only advance when a failed window throws. datalogger.generateLog does throw on an empty window (src/libs/sensors-logging.ts:523-525) and its declared return type is non-optional, so both the fall-through and the unchecked toAssOverlay(telemetryLog, …) are sound.
• The recording-finish path always has a first candidate. onstop sets info.dateFinish at src/stores/video.ts:1186, two statements before generateTelemetryOverlay runs, so the new if (!overlayWindow) throw is unreachable on the normal path and only replaces the error an undefined date used to raise inside generateLog.

1. Correctness & Implementation Bugs — 1 finding

1.3 — minor — a swallowed store failure leaves the Standalone copy with nothing to copy, and the user with a success snackbar (new this round) (src/composables/videoChunkManager.ts, resolveTelemetryOverlayFile and processChunkGroup)

resolveTelemetryOverlayFile persists the generated overlay in its own try whose catch only warns, then returns { blob: overlay, filename: subtitlesFileName } regardless. That is right for the download path, which puts assFile.blob straight into the ZIP — it is what finding 1.2 asked for. On the processing path it is not: processChunkGroup takes only filename and calls window.electronAPI.copyTelemetryFile(filename, videoSubtitlesFilename(outputPath)), and the main-process helper resolves that name inside the videos folder and copies it from disk (src/electron/services/video-recording.ts:552-557). When the write did not land there is no such file, and that helper catches its own failure and only logs (:559-561), so the try/catch around the call in processChunkGroup never fires and the run continues to the Video processed successfully! snackbar. The user gets an MP4 with no subtitles and no indication anything went wrong.

On the base ref this surfaced: tier 3's saveAndCopyTelemetry sat outside any catch (base src/composables/videoChunkManager.ts:568-574), so a videoStorage.setItem rejection propagated to processChunkGroup and produced the Could not generate telemetry overlay for the processed video: … snackbar. The head loses that last signal.

The asymmetry is the point: for the download the store write is a cache, for the processing path it is the delivery mechanism. Smallest fix that keeps 1.2 intact is to let the caller know which happened — have the resolver report whether the blob was persisted (or have processChunkGroup re-attempt/report on the warning) and open the existing error snackbar when it was not. Fixing it one level down instead — letting copyTelemetryFile in src/electron/services/video-recording.ts propagate rather than swallow — would also close the pre-existing case where the copy itself fails, but it changes behavior for src/libs/live-video-processor.ts:292 as well, so it is the larger call to make.

Consequence: on the desktop version, a recording whose rebuilt subtitle file cannot be written to disk is reported as processed successfully and silently ends up with no telemetry overlay.

2. Persistence & User Data — inventory, no findings

Key / store	Backend	What happened
<recording name>.ass in videoStorage	Machine-local. Electron: the videos/ folder via ElectronStorage (src/libs/videoStorage.ts:23-61,137,142). Lite: the Cockpit - Video Recovery IndexedDB (src/libs/videoStorage.ts:116-121).	New write site, unchanged key shape (videoSubtitlesFilename(...), the same helper src/stores/video.ts:808 uses). The Standalone path already wrote here (base saveAndCopyTelemetry); what is new is that the Lite download path writes it too. The second commit changes only the contents of the store's own write — the resolution in the header — not the key.
Cockpit (…).clog in the Cockpit - Sensor Logs IndexedDB	Machine-local (src/libs/sensors-logging.ts:293).	Indirect, unchanged shape. datalogger.generateLog persists the generated log as a side effect (src/libs/sensors-logging.ts:551), so a Lite chunk download now also leaves one telemetry log file behind — the same thing the Standalone processing path has always done.

Judged: both are media/log artifacts rather than cockpit--prefixed settings, so the cockpit- naming rule and useBlueOsStorage do not apply; neither is machine-specific data being vehicle-synced; no key is reshaped, no value duplicates its own key, and the PR adds no migration and changes no default. The generated .ass surfaces in the video library's log-file list (src/components/VideoLibraryModal.vue:1124), where the user can already delete it, so it is not an invisible accumulation. The failure to write one is a reporting problem rather than a data one, and is filed as 1.3 rather than here.

Sections with nothing to report (9)

3. AGENTS.md Adherence — ✅ (no new dependency and package.json untouched; the added JSDoc blocks on resolveTelemetryOverlayFile, telemetryOverlayWindowCandidates and every TelemetryOverlayWindow member carry typed @param/@returns with real text, satisfying jsdoc/require-jsdoc and jsdoc/require-returns as configured at .eslintrc.cjs:29-46; domain logic lives in the framework-agnostic src/libs/video-telemetry.ts; the second commit removes the third copy of the window/resolution decision rather than adding one; no renames, import reorders or unrelated reflows, and both exported symbols have call sites in this PR)

4. Security — ✅ (re-read src/libs/video-telemetry.ts and src/tests/libs/video-telemetry.test.ts in full and the changed hunks of the other two files: plain arithmetic on Date, no network call, no eval/Function, no encoded blobs, no hidden or bidirectional Unicode, no environment variables or credentials; no dependency, workflow, postinstall script or Electron main-process file is touched; nothing in pr.json, the diff, new-comments.json or decisions.json contains text addressed to this reviewer)

5. Performance — ✅ (all four changed functions trace to a button click or to mediaRecorder.onstop, never to onIncomingMessage, dataLake:setVariable or a watcher; generation runs at most twice per click and generateLog reads a bounded IDBKeyRange window (src/libs/sensors-logging.ts:517-521) rather than the whole store; no listener, interval or watcher is registered, so nothing needs teardown, and no canvas work is added)

6. UI / UX — ✅ (no dialog, control, overlay or icon is added or restyled, so the dialog-anatomy, theme="dark", button-token, footer, padding, glass and stacking clauses have nothing to bind to; the Lite failure path gained visible feedback as a warning snackbar in place of the old error one, no snackbar is paired with a duplicate console.*, nothing can open a dialog in a loop, and both entry points already call logUserAction. The one missing-feedback case is the Standalone one, filed as 1.3 rather than duplicated here. The new user-visible string interpolates ${error} the way every other snackbar in that composable already does)

7. Code Quality & Style — ✅ (the complexity report attributes one trigger to this PR — downloadChunkGroup at src/composables/videoChunkManager.ts:349, nested-4-or-deeper, with the report's figures being complexity 13 against a base 13 and depth 5 against a base depth 5; depth equals base depth, so the nesting is inherited and the trigger is answered, and the PR's own contribution there is one try/catch replacing an if/else. The report covers all 4 changed files, measured 159 functions and is not truncated. Beyond that: no any or stray type is introduced, no existing comment is reworded over unchanged code, the two added comments explain why, and the added lines fit the 120-column prettier width and the 180-column max-len at .eslintrc.cjs:47)

8. Commit Hygiene — ✅ (the same two commits as round 2, 7a3bee6 and a915b43, with nothing pushed since; the round-1 fixes were folded into 7a3bee6 rather than left as an "address review" commit, and the behavior fix rides alone in a915b43; both video: prefixes match the dominant style in git log and describe their change; no wip/fixup!/AI-authorship trailer, no #N or closing keyword in either message, no self-correcting or replicated commit, and neither is oversized)

9. Tests — ✅ (src/tests/libs/video-telemetry.test.ts carries the same five cases unchanged and sits where the tree puts library tests; no existing test was removed, skipped or weakened, and the cases assert observable outputs rather than internals)

10. Documentation — ✅ (the change narrows a Lite/Standalone gap instead of adding a Lite-only limitation, so the README parity table needs no row; JSDoc is present on both new public functions and on the new interface, and generateTelemetryOverlay's existing block stays accurate after the store change)

11. Nitpicks / Optional — ✅ (the one nit ever raised here — the Standalone path reading the .ass blob through IPC only to use its filename — was settled by maintainer vote this round in favour of the current signature; re-read both callers of resolveTelemetryOverlayFile at head and found no other taste-level call worth a line)

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

[rafaellehmkuhl]

Review follow-up — round 3

Done

• src/composables/videoChunkManager.ts (1.3 — a swallowed store failure leaves the Standalone copy with nothing to copy): the resolver now reports whether the overlay reached the video storage, and processChunkGroup throws when it did not, so the existing error snackbar fires instead of the copy silently no-opping into a success message. Verified the premise first: copyTelemetryFile really does catch and only warn (src/electron/services/video-recording.ts:552-561), so the signal base tier 3 gave by propagating was indeed lost. The download path is untouched and still puts the built blob in the ZIP, which keeps 1.2 intact. Folded into 767df13.

[rafaellehmkuhl]

/review

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 4)

1 open finding, 1 minor, raised this round; 5 closed — 1.3 addressed by this push, 4 settled earlier, one of them by maintainer vote.

When you download the raw pieces of a recording from the video library, Cockpit used to look for the telemetry subtitle file and, when the recording never got one, tell you it failed and hand you an archive with no telemetry. This PR makes the browser download and the desktop chunk-processing flow build that subtitle file on the spot out of the telemetry Cockpit already logged — using the recording's own start and end time when it still knows them, otherwise reconstructing the time span from the timestamps of the recorded pieces — and saves the result next to the recording so it is there next time too. A second commit points the place recordings normally get their subtitles at that same shared helper, so a recording that never stored its video size no longer produces a subtitle file declaring an invalid one. This push adds the missing failure signal on the desktop path: when the rebuilt file cannot be saved, processing now reports it instead of claiming success.

What still needs attention

#	Problem	What it means	Severity	Status
1.4	Desktop download still ships without the rebuilt subtitles	On the desktop version, downloading a recording's raw pieces still hands you an archive with no telemetry subtitles and says nothing about it, even though Cockpit can now rebuild them the way the browser version does.	minor	❌

Since round 3 — 1 addressed, 1 new finding, comparing a915b43 → 9d90583 (range rebuilt from pr.diff after a rebase)

The range is not usable as delivered, and I did not use it. incremental.diff is supposed to hold a915b43...9d90583, but it carries the whole PR: it still shows the removal of saveAndCopyTelemetry and copyOrGenerateTelemetryOverlayFile, which were already gone from the tree at a915b43 in round 3. The commit list in pr.json confirms why — the two subjects are the ones round 3 read, but the SHAs changed (7a3bee6, a915b43 → 767df13, 9d90583) and both carry the same committedDate of 2026-09-02T17:16:02Z, which is a rebase, not two new commits. Every status below is therefore judged against pr.diff at 9d90583, compared with the code round 3 quoted in its own findings.

1.3 — ✅ Addressed. The finding asked for two things: that the resolver report whether the generated overlay actually reached the video storage, and that the existing error snackbar fire when it did not. Both landed. resolveTelemetryOverlayFile now returns ResolvedTelemetryOverlay (FileDescriptor plus stored: boolean), sets stored = false in the catch around videoStore.videoStorage.setItem, and processChunkGroup throws Could not store the telemetry overlay as '…' before it reaches window.electronAPI.copyTelemetryFile, so the surrounding catch (telemetryError) opens the error snackbar. That restores exactly the signal base tier 3 gave by letting the rejection propagate. Checked the two things a fix like this could have broken: the download path still pushes assFile.blob into the ZIP and ignores stored, so finding 1.2 stays closed, and the tier-1 hit returns stored: true with the key it found in videoStorage, which on Electron is the same videos folder copyTelemetryFile resolves against (src/libs/videoStorage.ts:137,142 against src/electron/services/video-recording.ts:554-557).

1.4 — new, minor. Re-running the sections over the whole diff turned the one asymmetry the last two rounds recorded in the change map without raising it into something I am now raising: the recovery reaches the Lite download and the Standalone processing path, but not the Standalone download, which builds its ZIP in the main process from its own hash lookup and ships silently without a subtitle. Written out in section 1, with the three-line fix.

No /resolve command has been issued on this PR — resolutions.json is [] — so nothing was settled by hand this round, and there is no unknown finding id to report back. decisions.json is [] as well: the round-3 accept on 11.1 was applied then and stays resolved in the ledger, and no new dispute exists to vote on.

1.1, 1.2, 7.1 and 11.1 stay closed and are not re-judged; they are carried in the ledger with the statuses they were closed under.

Discussion since round 3. rafaellehmkuhl posted a follow-up (link) describing the 1.3 fix. Every claim in it checks out against the code rather than being taken on trust: the resolver does report storage success and processChunkGroup does throw on it; the download path is untouched; and the premise they say they verified first — that copyTelemetryFile catches and only warns — is true at src/electron/services/video-recording.ts:552-561, which is why the copy would otherwise no-op into a success message. Their claim that the fix was folded into 767df13 rather than added as an "address review" commit is consistent with what is on disk (two commits, the original two subjects, no fixup), though which of the two commits carries the hunk is not checkable from the data available here. The second comment is a bare /review trigger and carries nothing to act on. Nothing in the PR body, the diff, the comments or the decision files contains text addressed to this reviewer.

Change map — what was established before judging

Claims (the PR body is the first commit's text; the second commit carries its own)

• Symptom: "Downloading a chunk group on Lite only looked an .ass file up and gave up when the recording never got one." — verified, with one nuance: base src/composables/videoChunkManager.ts:362-372 looks the file up with findAssTelemetryFile and opens an error snackbar when there is none. The download itself continued — it produced a ZIP without subtitles rather than aborting.
• Cause: the recording never got an .ass because processing died before writing it. — verified as reachable: the only recording-time producer is generateTelemetryOverlay (src/stores/video.ts:788), called from the recorder's onstop at :1210, whose failure is caught at :1211-1213 and reported, leaving chunks with no .ass.
• Cause: "Standalone regenerates the overlay while processing chunks, but reads its own telemetry, not the browser's." — verified: base copyOrGenerateTelemetryOverlayFile is reachable only from processChunkGroup, which returns early unless isElectron() && window.electronAPI (src/composables/videoChunkManager.ts:583). The Standalone download never reached it either, taking the createVideoChunksZip branch at :306-316 — that is the ground for finding 1.4, and it makes the resolver's download half a Lite-only path.
• Mechanism: move the metadata-then-chunk-timestamps fallback into candidate windows and resolve the overlay the same way on both paths, storing what it generates. — verified: telemetryOverlayWindowCandidates reproduces base tier 2 (dateStart/dateFinish, vWidth ?? 1920, vHeight ?? 1080) and tier 3 (min/max of chunk epochs, +1000 on the end, 1920×1080), and resolveTelemetryOverlayFile is called from downloadChunkGroup and processChunkGroup.
• Second commit — symptom: the recording-finish overlay asserted the stored video size away and wrote PlayResX: undefined. — verified as a real hazard, with narrower reachability than the message suggests: base src/stores/video.ts:797-805 passes recordingData.vWidth!/vHeight!, both optional on CommonVideoInfo (src/types/video.ts:125-129), straight into the header (src/libs/sensors-logging.ts:607-608). New this round: startRecording has been writing both since src/stores/video.ts:958-959, with its own || 1920 fallback, and nothing else creates an entry in cockpit-unprocessed-video-info (:83, :973), so a resolution-less recording can now only come from an entry persisted by an earlier version. The commit message does not claim otherwise; recorded as reachability, not as a contradiction.
• The author's round-3 follow-up — verified, item by item, in the since-last-round block above.

Failure sites — three, two of them in the diff. The Lite download gap lives at base src/composables/videoChunkManager.ts:362-372, which the diff replaces with the resolver. The invalid-resolution header lives at base src/stores/video.ts:797-805, which the second commit replaces. The third, the Standalone chunk-group ZIP builder at src/electron/services/video-recording.ts:668-675 and :737-760, has its own hash lookup and is not in the diff — finding 1.4. Re-checked the interrupted-recording case end to end on Lite: getChunkTimestamp (src/composables/videoChunkManager.ts:69-92) needs only dateStart, so a recording that died during processing and never got a dateFinish skips candidate 1 and still yields a real window from candidate 2, making the ladder reachable rather than decorative.

Entry points

Function	Reached from	Frequency
resolveTelemetryOverlayFile (new)	downloadChunkGroup → onDownloadChunkGroup (src/components/VideoLibraryModal.vue:1254) and processChunkGroup → onProcessChunkGroup (:1249), both button clicks in the video library's chunk list	per user action
downloadChunkGroup (changed)	src/components/VideoLibraryModal.vue:1254, button click	per user action
processChunkGroup (changed)	src/components/VideoLibraryModal.vue:1249, button click	per user action
telemetryOverlayWindowCandidates (new, src/libs/video-telemetry.ts)	resolveTelemetryOverlayFile and generateTelemetryOverlay	per user action
generateTelemetryOverlay (changed)	mediaRecorder.onstop (src/stores/video.ts:1169) via :1210 — fires once when a recording ends, from the Stop button, stream teardown or a dropped link	per user action

All three call sites already log the interaction (logUserAction at src/components/VideoLibraryModal.vue:1250,1255, and src/components/mini-widgets/MiniVideoRecorder.vue:339,365 for the recording whose end reaches onstop), so no new logging is owed.

Invariants

• On Standalone, the file copyTelemetryFile is asked for exists in the videos folder. Closed this round at the single consumer rather than at the producer: processChunkGroup refuses to hand the name over unless the resolver reports stored. The remaining hole is pre-existing and out of scope — copyTelemetryFile still swallows a failure of its own copy (src/electron/services/video-recording.ts:559-561), which also covers src/libs/live-video-processor.ts:292; the PR does not touch it and it is not raised.
• An .ass for a recording is findable by "key contains the hash and ends in .ass". Consumers: findAssTelemetryFile (src/composables/videoChunkManager.ts:286-295), src/electron/services/video-recording.ts:672, and the library's log listing (src/components/VideoLibraryModal.vue:1124). Producers: src/stores/video.ts:809, the ZIP-extraction path (src/electron/services/video-recording.ts:445-446), and now resolveTelemetryOverlayFile, whose name always embeds #<hash> via videoFilename (src/utils/video.ts:13-17). The invariant holds, a second download reuses the generated file, and it is what makes 1.4's fix three lines instead of a main-process change.
• The candidate loop can only advance when a failed window throws. datalogger.generateLog throws on an empty window (src/libs/sensors-logging.ts:523-525) and its declared return is non-optional (:510), so both the fall-through and the unchecked toAssOverlay(telemetryLog, …) are sound.
• The recording-finish path always has a first candidate. onstop sets info.dateFinish at src/stores/video.ts:1186, two statements before generateTelemetryOverlay runs, so the new if (!overlayWindow) throw is unreachable on the normal path and only replaces the error an undefined date used to raise inside generateLog.
• The dates the helper reads are Date objects. They are not, after a reload: unprocessedVideos is a vueuse useStorage on cockpit-unprocessed-video-info (src/stores/video.ts:83), so JSON serialization hands back ISO strings. The helper's new Date(recording.dateStart) tolerates both, where the base store path's recordingData.dateStart!.getTime() would have thrown — covered, and slightly better than the base.

1. Correctness & Implementation Bugs — 1 finding

1.4 — minor — the Standalone download of a chunk group still ships without a subtitle, and says nothing (new this round) (src/composables/videoChunkManager.ts:306-316, against src/electron/services/video-recording.ts:668-675,737-760)

downloadChunkGroup has two halves. The Lite half now calls resolveTelemetryOverlayFile and, when no telemetry was logged at all, warns the user that the archive carries no subtitle. The Electron half never enters that code: it returns at :306-316 through window.electronAPI?.createVideoChunksZip(group.hash), and the main process builds the ZIP from its own lookup — filesystemStorage.keys(['videos']), then key.includes(hash) && key.endsWith('.ass') (src/electron/services/video-recording.ts:671-672). When that finds nothing it takes the else at :756-760, marks the .ass "done" and finishes the archive, and the renderer reports ZIP file created successfully with N chunks at: …. So on the exact recording this PR is about — chunks present, .ass never written — the Standalone user downloading the raw pieces gets no subtitle and, unlike the Lite user, not even a warning that one is missing.

Two things make this worth closing rather than leaving. The capability is already in the renderer: the same button on the same row in the same list behaves differently per build, and the PR's own framing is that the overlay should be recoverable from the telemetry the app still holds. And the fix is small, because the main-process lookup reads the very folder the resolver writes to — videoStorage on Electron is ElectronStorage(['videos']) (src/libs/videoStorage.ts:137,142), the same directory filesystemStorage.keys(['videos']) lists. Priming it before the IPC call is enough:

if (isElectron()) {
  try {
    await resolveTelemetryOverlayFile(group)
  } catch (error) {
    openSnackbar({ message: `Creating the ZIP without a telemetry subtitle file. ${error}`, variant: 'warning' })
  }
  const zipFilePath = await window.electronAPI?.createVideoChunksZip(group.hash)
  …

No main-process change, no new IPC, and createVideoChunksZip's existing lookup finds the freshly stored file. A maintainer may reasonably read the Standalone download as out of this PR's scope, since a Standalone user can still recover the overlay by pressing Process, which is why this is minor rather than an incomplete fix at major — but as it stands the PR closes the gap on one build and widens the difference between them on the other.

Consequence: on the desktop version, downloading a recording's raw pieces still produces an archive with no telemetry subtitles and reports success, even though Cockpit could rebuild them the way the browser version now does.

2. Persistence & User Data — inventory, no findings

Key / store	Backend	What happened
<recording name>.ass in videoStorage	Machine-local. Electron: the videos/ folder via ElectronStorage (src/libs/videoStorage.ts:23-61,137,142). Lite: the Cockpit - Video Recovery IndexedDB (:116-121).	New write site, unchanged key shape (videoSubtitlesFilename(...), the same helper src/stores/video.ts:808 uses). The Standalone path already wrote here (base saveAndCopyTelemetry); what is new is that the Lite download path writes it too. The second commit changes only the contents of the store's own write — the resolution in the header — not the key. Unchanged by this round's push, which only adds the stored report on that same write.
Cockpit (…).clog in the Cockpit - Sensor Logs IndexedDB	Machine-local (src/libs/sensors-logging.ts:293).	Indirect, unchanged shape. datalogger.generateLog persists the generated log as a side effect (:551), so a Lite chunk download now also leaves one telemetry log file behind — the same thing the Standalone processing path has always done.
cockpit-unprocessed-video-info	Machine-local, vueuse useStorage (src/stores/video.ts:83).	Read-only for this PR. No key, shape or default is changed, and no migration is added; the helper simply tolerates the string dates JSON round-tripping produces.

Judged: the first two are media/log artifacts rather than cockpit--prefixed settings, so the naming rule and useBlueOsStorage do not apply, and the third already carries the prefix. Nothing machine-specific is vehicle-synced, no key is reshaped, no value duplicates its own key, and the PR adds no migration and changes no default. On accumulation: deleteChunkGroup removes only tempVideoStorage keys (src/composables/videoChunkManager.ts:221-223), so an overlay generated for a group that is later deleted outlives it — pre-existing on Standalone, newly possible on Lite, roughly 8 KB, and listed as a log file the user can delete in the video library (src/components/VideoLibraryModal.vue:1124-1126), so not an invisible accumulation and not raised. A failure to write one is a reporting problem rather than a data one, and was filed as 1.3 rather than here.

Sections with nothing to report (9)

3. AGENTS.md Adherence — ✅ (no new dependency and package.json untouched; the interface added this round, ResolvedTelemetryOverlay, sits outside the file's jsdoc/require-jsdoc disable block and carries a block on itself and on stored, and the blocks on resolveTelemetryOverlayFile, telemetryOverlayWindowCandidates and every TelemetryOverlayWindow member have typed @param/@returns with real text, satisfying the rules at .eslintrc.cjs:29-46; the domain logic lives in the framework-agnostic src/libs/video-telemetry.ts; the second commit removes a duplicate of the window/resolution decision rather than adding one; no renames, import reorders or unrelated reflows, and every added symbol has a call site in this PR)

4. Security — ✅ (re-read src/libs/video-telemetry.ts and src/tests/libs/video-telemetry.test.ts in full and every changed hunk of the other two files: plain arithmetic on Date, no network call, no eval/Function, no encoded blobs, no hidden or bidirectional Unicode, no environment variable or credential; no dependency, workflow, postinstall script or Electron main-process file is touched — the main-process code cited in 1.4 is read, not modified; nothing in pr.json, pr.diff, new-comments.json, resolutions.json or decisions.json contains text addressed to this reviewer)

5. Performance — ✅ (all four changed functions trace to a button click or to mediaRecorder.onstop, never to onIncomingMessage, dataLake:setVariable or a watcher; generation runs at most twice per click and generateLog reads a bounded IDBKeyRange window (src/libs/sensors-logging.ts:517-521) rather than the whole store; this round's addition is one boolean and one throw; no listener, interval or watcher is registered, so nothing needs teardown, and no canvas work is added)

6. UI / UX — ✅ (no dialog, control, overlay or icon is added or restyled, so the dialog-anatomy, theme="dark", button-token, footer, padding, glass and stacking clauses have nothing to bind to; the Lite failure path gained visible feedback as a warning snackbar in place of the old error one, and the Standalone processing failure gained the error snackbar it had lost, both through openSnackbar with no duplicate console.* of the same message; nothing can open a dialog in a loop, and both entry points already call logUserAction. The chunk-group hash in the new strings is the same identifier that list already shows the user (src/composables/videoChunkManager.ts:210,225), so it is not internal jargon leaking into copy. The missing-feedback case is the Standalone download, filed as 1.4 rather than duplicated here)

7. Code Quality & Style — ✅ (the complexity report, measured at head 9d90583 against base cf83753, attributes one trigger to this PR — downloadChunkGroup at src/composables/videoChunkManager.ts:362, nested-4-or-deeper, with the report's figures being complexity 13 against a base 13 and depth 5 against a base depth 5. Depth equals base depth, so the nesting is inherited and the trigger is answered, and the PR's own contribution there is one try/catch replacing an if/else. The report covers all 4 changed files, measured 159 functions and is not truncated. Beyond that: no any or stray type, the stored flag replaces no non-null assertion with a looser one, window.electronAPI.copyTelemetryFile relies on the same narrowing from the guard at :583 that :612,627,631 already rely on, no existing comment is reworded over unchanged code, the three added comments explain why, and every added line fits the 180-column max-len at .eslintrc.cjs:47)

8. Commit Hygiene — ✅ (the branch was rebased since round 3 — same two subjects, new SHAs 767df13 and 9d90583, both recommitted at the same instant — and the 1.3 fix was folded into an existing commit rather than left as an "address review" or fixup! commit, which is what AGENTS.md asks; which of the two carries the hunk is not determinable from the data on disk, but the resolver it changes belongs to the first, and the behavior fix still rides alone in the second. Both video: prefixes match the dominant scope-prefixed style in git log and describe their change; no wip commit, no AI-authorship trailer, no #N or closing keyword in either message, no self-correcting or replicated commit, and neither is oversized)

9. Tests — ✅ (src/tests/libs/video-telemetry.test.ts carries the same five cases as round 3, unchanged, and sits where the tree puts library tests; the fixture satisfies every required member of UnprocessedVideoInfo (src/types/video.ts:133-151) so it typechecks; no existing test was removed, skipped or weakened, and the cases assert observable outputs rather than internals)

10. Documentation — ✅ (the change narrows a Lite/Standalone gap rather than adding a Lite-only limitation, so the README parity table needs no row — the gap 1.4 describes runs the other way and is a wiring gap, not a platform limitation to document; JSDoc is present on both new public functions, on the new interface and on the interface added this round, and generateTelemetryOverlay's existing block stays accurate after the store change)

11. Nitpicks / Optional — ✅ (the one nit ever raised here — the Standalone path reading the .ass blob through IPC only to use its filename — was settled by maintainer vote in round 3 in favour of the current signature, and this round's stored flag rides on that same return value without widening it; re-read both callers of resolveTelemetryOverlayFile at head and found no other taste-level call worth a line)

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

[rafaellehmkuhl]

Review follow-up — round 4

Done

• src/composables/videoChunkManager.ts (1.4 — the Standalone download still ships without the rebuilt subtitles): the Electron branch of downloadChunkGroup now resolves the overlay before asking the main process for the ZIP, and warns when there is no telemetry to build one from, so the same button behaves the same on both builds. Verified the premise the fix rests on: the ZIP builder finds the file through filesystemStorage.keys(['videos']) (src/electron/services/video-recording.ts:671-672), which is the folder videoStorage writes to on Electron, so priming it needs no main-process change. Folded into 1808486.

[rafaellehmkuhl]

/review

[github-actions]

📝 MINOR SUGGESTIONS (Automated PR Review — round 5)

1 open finding, 1 minor, raised this round; 6 closed — 1.4 addressed by this push, 5 settled earlier, one of them by maintainer vote.

When you download the raw pieces of a recording from the video library, Cockpit used to look for the telemetry subtitle file and, when the recording never got one, tell you it failed and hand you an archive with no telemetry. This PR makes both the browser download and the desktop flows rebuild that subtitle on the spot out of the telemetry Cockpit already logged — using the recording's own start and end time when it still knows them, otherwise reconstructing the time span from the timestamps of the recorded pieces — and saves the result next to the recording so it is there next time too. A second commit points the place recordings normally get their subtitles at the same shared helper, so a recording that never stored its video size no longer produces a subtitle file declaring an invalid one. This push closes the last gap between the two builds: the desktop download now rebuilds the subtitle before asking for the archive, and warns when there is no telemetry to rebuild it from.

What still needs attention

#	Problem	What it means	Severity	Status
1.5	Desktop download stays silent when the rebuilt subtitle cannot be saved	If Cockpit cannot write the rebuilt subtitle file to disk, the desktop download hands you an archive without it and still reports success, so you only find out when the video plays with no telemetry.	minor	❌

Since round 4 — 1 addressed, 1 new finding, comparing 9d90583 → ad37f59 (range rebuilt from pr.diff after a rebase)

The range is not usable as delivered, and I did not use it. incremental.diff is supposed to hold 9d90583...ad37f59, but it carries the whole PR again: it still shows the removal of saveAndCopyTelemetry and copyOrGenerateTelemetryOverlayFile, which were already gone from the tree at 9d90583 in round 4, and it re-adds src/libs/video-telemetry.ts in full. pr.json shows why — the same two commit subjects round 4 read, with new SHAs (767df13, 9d90583 → 1808486, ad37f59) and both carrying the same committedDate of 2026-09-02T17:41:39Z, which is a rebase, not two new commits. Every status below is therefore judged against pr.diff at ad37f59, compared with the code round 4 quoted in its own findings. The actual delta against that quoted code is one eight-line block, the Electron priming call in downloadChunkGroup.

1.4 — ✅ Addressed. The finding asked for two things: that the Electron half of downloadChunkGroup reach resolveTelemetryOverlayFile before it asks the main process for the ZIP, and that the user be told when there is no telemetry to build a subtitle from — the warning the Lite half already gives. Both landed, in the shape the finding proposed: the isElectron() branch now awaits resolveTelemetryOverlayFile(group) inside a try, and its catch opens Creating the ZIP without a telemetry subtitle file. … as a warning snackbar, before window.electronAPI?.createVideoChunksZip(group.hash). I re-verified the premise the fix rests on rather than taking it from the finding: videoStorage on Electron is ElectronStorage(['videos']) (src/libs/videoStorage.ts:137,142), whose setItem writes <cockpit folder>/videos/<key> (src/electron/services/storage.ts:51-56), and the main-process ZIP builder lists exactly that folder with filesystemStorage.keys(['videos']) and matches key.includes(hash) && key.endsWith('.ass') (src/electron/services/video-recording.ts:671-672). The generated key always embeds #<hash> through videoFilename (src/utils/video.ts:13-17), so the lookup finds it, and no main-process change was needed. Checked the two things this could have broken: the Lite half is untouched and still pushes assFile.blob into the ZIP, and processChunkGroup still checks stored before copying, so 1.2 and 1.3 stay closed.

1.5 — new, minor. The priming call ignores the stored flag that the same resolver return carries and that processChunkGroup was taught to check in round 3. When the write to the videos folder fails, the resolver returns stored: false without throwing, the catch never fires, and the main-process lookup finds nothing — so the archive ships with no subtitle under the ZIP file created successfully… message, which is the exact failure mode 1.3 closed on the sibling path. Written out in section 1 with the two-line fix.

No /resolve command has been issued on this PR — resolutions.json is [] — so nothing was settled by hand this round, and there is no unknown finding id to report back. decisions.json is [] as well: the round-3 accept on 11.1 was applied then and stays resolved in the ledger, and no dispute is currently open to vote on.

1.1, 1.2, 7.1, 11.1 and 1.3 stay closed and are not re-judged; they are carried in the ledger with the statuses they were closed under.

Discussion since round 4. rafaellehmkuhl posted a follow-up (link) describing the 1.4 fix. Its claims check out against the code rather than being taken on trust: the Electron branch does resolve the overlay before the IPC call and does warn when it cannot, and the premise they say they verified — that the ZIP builder reads the folder videoStorage writes to — is true at the four sites cited above. Their claim that the fix was folded into 1808486 rather than left as an "address review" commit is consistent with what is on disk (two commits, the original two subjects, no fixup), though which of the two carries the hunk is not checkable from the data available here. The other two comments are bare /review triggers and carry nothing to act on. Nothing in the PR body, the diff, the comments, the complexity report or the decision files contains text addressed to this reviewer.

Change map — what was established before judging

Claims (the PR body is the first commit's text; the second commit carries its own)

• Symptom: "Downloading a chunk group on Lite only looked an .ass file up and gave up when the recording never got one." — verified, with one nuance: base src/composables/videoChunkManager.ts:362-372 looks the file up with findAssTelemetryFile and opens an error snackbar when there is none. The download itself continued — it produced a ZIP without subtitles rather than aborting.
• Cause: the recording never got an .ass because processing died before writing it. — verified as reachable: the only recording-time producer is generateTelemetryOverlay (src/stores/video.ts:788), called from the recorder's onstop at :1210, whose failure is caught at :1211-1213 and reported, leaving chunks with no .ass.
• Cause: "Standalone regenerates the overlay while processing chunks, but reads its own telemetry, not the browser's." — verified: base copyOrGenerateTelemetryOverlayFile is reachable only from processChunkGroup, which returns early unless isElectron() && window.electronAPI (base src/composables/videoChunkManager.ts:583). The Standalone download did not reach it either, taking the createVideoChunksZip branch at base :306-316 — that was finding 1.4, and it is closed by this push, so the resolver is no longer a Lite-only recovery.
• Mechanism: move the metadata-then-chunk-timestamps fallback into candidate windows and resolve the overlay the same way on both paths, storing what it generates. — verified: telemetryOverlayWindowCandidates reproduces base tier 2 (dateStart/dateFinish, vWidth ?? 1920, vHeight ?? 1080) and tier 3 (min/max of chunk epochs, +1000 on the end, 1920×1080), and resolveTelemetryOverlayFile is now called from three sites: the Lite download, the Electron download, and processChunkGroup.
• Second commit — symptom: the recording-finish overlay asserted the stored video size away and wrote PlayResX: undefined. — verified as a real hazard, with narrower reachability than the message suggests: base src/stores/video.ts:797-805 passes recordingData.vWidth!/vHeight!, both optional on CommonVideoInfo (src/types/video.ts:125-129), straight into the header (src/libs/sensors-logging.ts:607-608). startRecording has been writing both since src/stores/video.ts:958-959, with its own || 1920 fallback, and nothing else creates an entry in cockpit-unprocessed-video-info (:83, :973), so a resolution-less recording can now only come from an entry persisted by an earlier version. The commit message does not claim otherwise; recorded as reachability, not as a contradiction.
• The author's round-4 follow-up — verified, item by item, in the since-last-round block above.

Failure sites — three, all now answered in the renderer. The Lite download gap lives at base src/composables/videoChunkManager.ts:362-372, which the diff replaces with the resolver. The invalid-resolution header lives at base src/stores/video.ts:797-805, which the second commit replaces. The third, the Standalone chunk-group ZIP builder at src/electron/services/video-recording.ts:668-675,737-760, is still not in the diff and does not need to be: it takes the else at :756-760 and finishes the archive when it finds no .ass, so the renderer priming the videos folder before the IPC call is what makes it ship one. What survives is the case where that priming silently fails to write — finding 1.5, which lives in the diff itself. Re-checked the interrupted-recording case end to end: getChunkTimestamp (src/composables/videoChunkManager.ts:69-92) needs only dateStart, so a recording that died during processing and never got a dateFinish skips candidate 1 and still yields a real window from candidate 2, making the ladder reachable rather than decorative.

Entry points

Function	Reached from	Frequency
resolveTelemetryOverlayFile (new)	downloadChunkGroup on both builds → onDownloadChunkGroup (src/components/VideoLibraryModal.vue:1254) and processChunkGroup → onProcessChunkGroup (:1249), both button clicks in the video library's chunk list	per user action
downloadChunkGroup (changed this round)	src/components/VideoLibraryModal.vue:1254, button click	per user action
processChunkGroup (changed)	src/components/VideoLibraryModal.vue:1249, button click	per user action
telemetryOverlayWindowCandidates (new, src/libs/video-telemetry.ts)	resolveTelemetryOverlayFile and generateTelemetryOverlay	per user action
generateTelemetryOverlay (changed)	mediaRecorder.onstop (src/stores/video.ts:1169) via :1210 — fires once when a recording ends, from the Stop button, stream teardown or a dropped link	per user action

All three call sites already log the interaction (logUserAction at src/components/VideoLibraryModal.vue:1250,1255, and src/components/mini-widgets/MiniVideoRecorder.vue:339,365 for the recording whose end reaches onstop), so no new logging is owed.

Invariants

• On Standalone, the file the consumer reads back from the videos folder exists. Two consumers now, closed at one of them. processChunkGroup refuses to hand the name to copyTelemetryFile unless the resolver reports stored; the download's consumer is the main-process ZIP builder, which is handed no name at all and simply finds — or does not find — the file, and the priming call does not look at stored before letting it try. That asymmetry is finding 1.5. The pre-existing hole below it is out of scope and unchanged: copyTelemetryFile still swallows a failure of its own copy (src/electron/services/video-recording.ts:559-561).
• An .ass for a recording is findable by "key contains the hash and ends in .ass". Consumers: findAssTelemetryFile (src/composables/videoChunkManager.ts:286-295), src/electron/services/video-recording.ts:672, and the library's log listing (src/components/VideoLibraryModal.vue:1124). Producers: src/stores/video.ts:809, the ZIP-extraction path (src/electron/services/video-recording.ts:445-446), and resolveTelemetryOverlayFile, whose name always embeds #<hash> via videoFilename (src/utils/video.ts:13-17). Verified this round at the byte level for the new Electron path — ElectronStorage(['videos']).setItem writes <cockpit folder>/videos/<key> (src/libs/videoStorage.ts:137,142, src/electron/services/storage.ts:51-56) and filesystemStorage.keys(['videos']) lists that same folder — which is what makes the 1.4 fix eight lines in the renderer instead of a main-process change.
• The candidate loop can only advance when a failed window throws. datalogger.generateLog throws on an empty window (src/libs/sensors-logging.ts:523-525) and its declared return is non-optional (:510), so both the fall-through and the unchecked toAssOverlay(telemetryLog, …) are sound.
• The recording-finish path always has a first candidate. onstop sets info.dateFinish at src/stores/video.ts:1186, two statements before generateTelemetryOverlay runs, so the new if (!overlayWindow) throw is unreachable on the normal path and only replaces the error an undefined date used to raise inside generateLog.
• The dates the helper reads are Date objects. They are not, after a reload: unprocessedVideos is a vueuse useStorage on cockpit-unprocessed-video-info (src/stores/video.ts:83), so JSON serialization hands back ISO strings. The helper's new Date(recording.dateStart) tolerates both, where the base store path's recordingData.dateStart!.getTime() would have thrown — covered, and slightly better than the base.

1. Correctness & Implementation Bugs — 1 finding

1.5 — minor — the Electron download primes the overlay but ignores whether it was stored, so a failed write ships a subtitle-less ZIP under a success message (new this round) (src/composables/videoChunkManager.ts, the isElectron() branch of downloadChunkGroup)

The new priming block is:

if (isElectron()) {
  // The main process picks the overlay up from the videos folder, so it has to be there already.
  try {
    await resolveTelemetryOverlayFile(group)
  } catch (error) {
    const message = `Creating the ZIP without a telemetry subtitle file. ${error}`
    openSnackbar({ message, variant: 'warning' })
  }

  const zipFilePath = await window.electronAPI?.createVideoChunksZip(group.hash)

The comment above it states the invariant precisely — the main process picks the overlay up from the videos folder — and then the code does not check that it got there. resolveTelemetryOverlayFile only throws when no candidate window produced an overlay at all; a write failure is caught inside it, logged with console.warn, and reported to the caller as stored: false on an otherwise successful return (that is finding 1.2's fix, and it is correct for the Lite caller, which uses blob and never needs the file on disk). So when videoStore.videoStorage.setItem rejects — ElectronStorage.setItem forwards the rejection from filesystemStorage.setItem's fs.writeFile (src/libs/videoStorage.ts:38-41, src/electron/preload.ts:31-33, src/electron/services/storage.ts:51-56), so a full disk or a permission error on the Cockpit folder gets here — the catch never fires, createVideoChunksZip finds no .ass at src/electron/services/video-recording.ts:671-672, takes the else at :756-760, and the renderer reports ZIP file created successfully with N chunks at: …. The user gets the same silent subtitle-less archive that 1.4 was about, from the same button, with no warning.

This is the same defect processChunkGroup was fixed for in round 3, and the fix is the same two lines — the flag is already on the value being discarded:

try {
  const { stored } = await resolveTelemetryOverlayFile(group)
  if (!stored) throw new Error('Could not store the telemetry overlay next to the recording.')
} catch (error) {
  …
}

minor rather than major: it needs a storage failure to reach a user, the failure is on a path the user explicitly triggered, and console.warn leaves a trace in the log. But the whole point of the stored flag is that a caller working from the name rather than the blob has to know, which is what the flag's own JSDoc says at src/composables/videoChunkManager.ts — and this is now the second such caller.

Consequence: on the desktop version, if Cockpit cannot save the rebuilt subtitle file, the downloaded archive silently has no telemetry subtitles while the message says it was created successfully.

2. Persistence & User Data — inventory, no findings

Key / store	Backend	What happened
<recording name>.ass in videoStorage	Machine-local. Electron: the videos/ folder via ElectronStorage (src/libs/videoStorage.ts:23-61,137,142). Lite: the Cockpit - Video Recovery IndexedDB (:116-121).	New write sites, unchanged key shape (videoSubtitlesFilename(...), the same helper src/stores/video.ts:808 uses). The Standalone processing path already wrote here (base saveAndCopyTelemetry); new is that both download paths write it too — the Lite one since round 1, the Standalone one as of this push. The second commit changes only the contents of the store's own write — the resolution in the header — not the key.
Cockpit (…).clog in the Cockpit - Sensor Logs IndexedDB	Machine-local (src/libs/sensors-logging.ts:293).	Indirect, unchanged shape. datalogger.generateLog persists the generated log as a side effect (:551), so a chunk download on either build now also leaves one telemetry log file behind — the same thing the Standalone processing path has always done.
cockpit-unprocessed-video-info	Machine-local, vueuse useStorage (src/stores/video.ts:83).	Read-only for this PR. No key, shape or default is changed, and no migration is added; the helper simply tolerates the string dates JSON round-tripping produces.

Judged: the first two are media/log artifacts rather than cockpit--prefixed settings, so the naming rule and useBlueOsStorage do not apply, and the third already carries the prefix. Nothing machine-specific is vehicle-synced, no key is reshaped, no value duplicates its own key, and the PR adds no migration and changes no default. On accumulation, re-checked for the new Standalone write: deleteChunkGroup removes only tempVideoStorage keys (src/composables/videoChunkManager.ts:221-223), so an overlay generated for a group later deleted outlives it — roughly 8 KB, on both builds now, and listed as a log file the user can delete in the video library (src/components/VideoLibraryModal.vue:1124-1126), so not an invisible accumulation and not raised. A failure to write one is a reporting problem rather than a data one, and is filed as 1.5 rather than here.

Sections with nothing to report (9)

3. AGENTS.md Adherence — ✅ (no new dependency and package.json untouched; the block added this round is eight lines in an existing function with no new symbol, and every symbol the PR adds still has a call site in it; JSDoc on resolveTelemetryOverlayFile, telemetryOverlayWindowCandidates, ResolvedTelemetryOverlay and every TelemetryOverlayWindow member has typed @param/@returns with real text, satisfying .eslintrc.cjs:29-46; the domain logic lives in the framework-agnostic src/libs/video-telemetry.ts; the fix reuses the existing resolver rather than duplicating the lookup into the Electron branch, which is the "reuse before reinventing" rung; no rename, import reorder or unrelated reflow in this push)

4. Security — ✅ (re-read src/libs/video-telemetry.ts and src/tests/libs/video-telemetry.test.ts in full and every changed hunk of the other two files: plain arithmetic on Date, no network call, no eval/Function, no encoded blob, no hidden or bidirectional Unicode, no environment variable or credential; no dependency, workflow, postinstall script or Electron main-process file is touched — the main-process code cited in 1.5 and in the change map is read, not modified; nothing in pr.json, pr.diff, new-comments.json, complexity-report.json, resolutions.json or decisions.json contains text addressed to this reviewer)

5. Performance — ✅ (all four changed functions trace to a button click or to mediaRecorder.onstop, never to onIncomingMessage, dataLake:setVariable or a watcher; this round's addition puts at most one generateLog on the Standalone download, and that reads a bounded IDBKeyRange window (src/libs/sensors-logging.ts:517-521) rather than the whole store, with the tier-1 hit short-circuiting it entirely on a recording that already has its .ass; no listener, interval or watcher is registered, so nothing needs teardown, and no canvas work is added)

6. UI / UX — ✅ (no dialog, control, overlay or icon is added or restyled, so the dialog-anatomy, theme="dark", button-token, footer, padding, glass and stacking clauses have nothing to bind to; the Standalone download gained the same warning snackbar the Lite one has, through openSnackbar with no duplicate console.* of the same message, and its copy says what the user gets — an archive without the subtitle file — rather than naming an internal failure; nothing can open a dialog in a loop, and both entry points already call logUserAction. The chunk-group hash carried in the message is the same identifier that list already shows the user (src/composables/videoChunkManager.ts:210,225), so it is not internal jargon leaking into copy. The one remaining missing-feedback case is filed as 1.5 rather than duplicated here)

7. Code Quality & Style — ✅ (the complexity report, measured at head ad37f59 against base cf83753, attributes one trigger to this PR — downloadChunkGroup at src/composables/videoChunkManager.ts:362, nested-4-or-deeper, with the report's figures being complexity 14 against a base 13 and depth 5 against a base depth 5. Depth equals base depth, so the nesting is inherited and the trigger is answered; the count rose by one, below the bump threshold and in a function already above it, and this round's contribution is one flat try/catch at the top of an existing branch. The report covers all 4 changed files, measured 159 functions and is not truncated. Beyond that: no any or stray type, window.electronAPI?.createVideoChunksZip keeps its existing optional call, no existing comment is reworded over unchanged code, the one comment added this round explains why the priming has to happen before the IPC call, and every added line fits the 180-column max-len at .eslintrc.cjs:47)

8. Commit Hygiene — ✅ (the branch was rebased again since round 4 — same two subjects, new SHAs 1808486 and ad37f59, both recommitted at 2026-09-02T17:41:39Z — and the 1.4 fix was folded into an existing commit rather than left as an "address review" or fixup! commit, which is what AGENTS.md asks; which of the two carries the hunk is not determinable from the data on disk, but the function it changes belongs to the first, and the behavior fix still rides alone in the second. Both video: prefixes match the dominant scope-prefixed style in git log and describe their change; no wip commit, no AI-authorship trailer, no #N or closing keyword in either message — the Closes #3000 stays in the PR body where it belongs — no self-correcting or replicated commit, and neither is oversized)

9. Tests — ✅ (src/tests/libs/video-telemetry.test.ts carries the same five cases as rounds 3 and 4, byte-identical in pr.diff, and sits where the tree puts library tests; the fixture satisfies every required member of UnprocessedVideoInfo (src/types/video.ts:133-151) so it typechecks; no existing test was removed, skipped or weakened, and the cases assert observable outputs rather than internals)

10. Documentation — ✅ (the change closes a Lite/Standalone gap rather than adding a Lite-only limitation, and this push closes the last of it, so the README parity table needs no row; JSDoc is present on both new public functions and on both new interfaces, generateTelemetryOverlay's existing block stays accurate after the store change, and the priming block added this round introduces no public symbol needing one)

11. Nitpicks / Optional — ✅ (the one nit ever raised here — the Standalone path reading the .ass blob through IPC only to use its filename — was settled by maintainer vote in round 3 in favour of the current signature; the priming call added this round inherits that same read on the tier-1 hit, roughly 8 KB over IPC per download, which is the settled trade rather than a new one, so it is not re-raised. Re-read all three callers of resolveTelemetryOverlayFile at head and found no other taste-level call worth a line)

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

[rafaellehmkuhl]

Review follow-up — round 5

Done

• src/composables/videoChunkManager.ts (1.5 — the Electron download ignores whether the overlay was stored): the priming block now reads stored and throws when the write did not land, so the warning fires instead of the archive going out subtitle-less under a success message. This was my own oversight — stored exists precisely for a caller that works from the file's name, which the Electron download is, since the main process re-finds it by name. It now mirrors the check processChunkGroup already does. Folded into 90bf807.

[rafaellehmkuhl]

/review

[github-actions]

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

0 open findings; 7 closed — 1.5 addressed by this push, 6 settled in earlier rounds, one of those by maintainer vote.

When you download the raw pieces of a recording from the video library, Cockpit used to look for the telemetry subtitle file and, when the recording never got one, tell you it failed and hand you an archive with no telemetry. This PR makes both the browser download and the desktop flows rebuild that subtitle on the spot out of the telemetry Cockpit already logged — using the recording's own start and end time when it still knows them, otherwise reconstructing the time span from the timestamps of the recorded pieces — and saves the result next to the recording so it is there next time too. A second commit points the place recordings normally get their subtitles at the same shared helper, so a recording that never stored its video size no longer produces a subtitle file declaring an invalid one. This push closes the last gap: the desktop download now checks that the rebuilt subtitle actually reached disk and warns when it did not, instead of reporting success over an archive without it.

What still needs attention

Nothing is open. Every finding raised over the six rounds is closed — six by code changes in the PR, one by a maintainer vote.

Since round 5 — 1 addressed, 0 new findings, comparing ad37f59 → 52bb993 (range rebuilt from pr.diff after another rebase)

The range is not usable as delivered, and I did not use it. incremental.diff is supposed to hold ad37f59...52bb993, but it carries the whole PR again: it re-adds src/libs/video-telemetry.ts and src/tests/libs/video-telemetry.test.ts in full and re-removes saveAndCopyTelemetry and copyOrGenerateTelemetryOverlayFile, all of which were already in that state at ad37f59 in round 5. pr.json shows why — the same two commit subjects round 5 read, with new SHAs (1808486, ad37f59 → 90bf807, 52bb993) and both carrying the same committedDate of 2026-09-02T17:54:14Z, which is a rebase, not two new commits. Every status below is therefore judged against pr.diff at 52bb993, compared with the code round 5 quoted in its own finding. The actual delta against that quoted code is three lines inside the Electron priming block of downloadChunkGroup.

1.5 — ✅ Addressed. The finding asked for one thing: that the Electron priming call stop discarding the stored flag its own resolver returns, so a failed write to the videos folder cannot ship a subtitle-less ZIP under the ZIP file created successfully… message. It landed in exactly the shape the finding proposed, and mirroring what processChunkGroup already does:

const { filename, stored } = await resolveTelemetryOverlayFile(group)
if (!stored) {
  throw new Error(`Could not store the telemetry overlay as '${filename}'.`)
}

The throw is inside the existing try, so it reaches the same catch that opens Creating the ZIP without a telemetry subtitle file. … as a warning snackbar before window.electronAPI?.createVideoChunksZip(group.hash) runs — the archive still gets built, which is the right call for a download the user asked for, and the user is now told it will have no subtitles. I re-checked the premise rather than carrying it from the finding: stored is true on a tier-1 hit and on a successful videoStore.videoStorage.setItem, and false only on the caught write failure inside resolveTelemetryOverlayFile, so the new branch cannot fire spuriously on the ordinary path. And the consumer it protects is unchanged — createVideoChunksZip still finds the overlay only by listing the videos folder (src/electron/services/video-recording.ts:671-672) and silently takes the else at :756-760 when there is none, which is precisely the outcome the check now announces. Also confirmed the two sibling paths were not disturbed: the Lite half still pushes assFile.blob into the ZIP and warns on a throw, and processChunkGroup still guards stored before copyTelemetryFile, so 1.2, 1.3 and 1.4 stay closed.

No /resolve command has been issued on this PR — resolutions.json is [] — so nothing was settled by hand this round, and there is no unknown finding id to report back. decisions.json is [] as well: the round-3 accept on 11.1 was applied then and stays resolved in the ledger, and no dispute is open to vote on.

1.1, 1.2, 7.1, 11.1, 1.3 and 1.4 stay closed and are not re-judged; they are carried in the ledger with the statuses they were closed under.

Discussion since round 5. rafaellehmkuhl posted a follow-up (link) describing the 1.5 fix. Its claims check out against the code rather than being taken on trust: the priming block does read stored and does throw when the write did not land, and their reading of why stored exists — that a caller working from the file's name rather than its blob has to know, which the Electron download is, since the main process re-finds it by name — matches what the flag's own JSDoc says and what the two consumers actually do. Their claim that the fix was folded into 90bf807 rather than left as an "address review" commit is consistent with what is on disk (two commits, the original two subjects, no fixup), though which of the two carries the hunk is not checkable from the data available here. The other comment is a bare /review trigger and carries nothing to act on. Nothing in the PR body, the diff, the comments, the complexity report or the decision files contains text addressed to this reviewer.

Change map — what was established before judging

Claims (the PR body is the first commit's text; the second commit carries its own)

• Symptom: "Downloading a chunk group on Lite only looked an .ass file up and gave up when the recording never got one." — verified, with one nuance: base src/composables/videoChunkManager.ts:362-372 looks the file up with findAssTelemetryFile and opens an error snackbar when there is none. The download itself continued — it produced a ZIP without subtitles rather than aborting.
• Cause: the recording never got an .ass because processing died before writing it. — verified as reachable: the only recording-time producer is generateTelemetryOverlay (src/stores/video.ts:788), called from the recorder's onstop at :1210, whose failure is caught at :1211-1213 and reported, leaving chunks with no .ass.
• Cause: "Standalone regenerates the overlay while processing chunks, but reads its own telemetry, not the browser's." — verified: base copyOrGenerateTelemetryOverlayFile is reachable only from processChunkGroup, which returns early unless isElectron() && window.electronAPI (base src/composables/videoChunkManager.ts:583). The Standalone download did not reach it either, taking the createVideoChunksZip branch at base :306-316 — that was finding 1.4, closed in round 5, so the resolver is no longer a Lite-only recovery.
• Mechanism: move the metadata-then-chunk-timestamps fallback into candidate windows and resolve the overlay the same way on both paths, storing what it generates. — verified: telemetryOverlayWindowCandidates reproduces base tier 2 (dateStart/dateFinish, vWidth ?? 1920, vHeight ?? 1080) and tier 3 (min/max of chunk epochs, +1000 on the end, 1920×1080), and resolveTelemetryOverlayFile is called from three sites: the Lite download, the Electron download, and processChunkGroup.
• Second commit — symptom: the recording-finish overlay asserted the stored video size away and wrote PlayResX: undefined. — verified as a real hazard, with narrower reachability than the message suggests: base src/stores/video.ts:797-805 passes recordingData.vWidth!/vHeight!, both optional on CommonVideoInfo (src/types/video.ts:125-129), straight into the header (src/libs/sensors-logging.ts:607-608). startRecording has been writing both since src/stores/video.ts:958-959, with its own || 1920 fallback, and nothing else creates an entry in cockpit-unprocessed-video-info (:83, :973), so a resolution-less recording can now only come from an entry persisted by an earlier version. The commit message does not claim otherwise; recorded as reachability, not as a contradiction.
• The author's round-5 follow-up — verified, item by item, in the since-last-round block above.

Failure sites — three, all now answered in the renderer. The Lite download gap lives at base src/composables/videoChunkManager.ts:362-372, which the diff replaces with the resolver. The invalid-resolution header lives at base src/stores/video.ts:797-805, which the second commit replaces. The third, the Standalone chunk-group ZIP builder at src/electron/services/video-recording.ts:668-675,737-760, is still not in the diff and does not need to be: it takes the else at :756-760 and finishes the archive when it finds no .ass, so the renderer priming the videos folder before the IPC call is what makes it ship one — and as of this push the renderer also checks that the priming landed before letting the IPC call proceed unwarned. Re-checked the interrupted-recording case end to end: getChunkTimestamp (src/composables/videoChunkManager.ts:69-92) needs only dateStart, so a recording that died during processing and never got a dateFinish skips candidate 1 and still yields a real window from candidate 2, making the ladder reachable rather than decorative.

Entry points

Function	Reached from	Frequency
resolveTelemetryOverlayFile (new)	downloadChunkGroup on both builds → onDownloadChunkGroup (src/components/VideoLibraryModal.vue:1254) and processChunkGroup → onProcessChunkGroup (:1249), both button clicks in the video library's chunk list	per user action
downloadChunkGroup (changed this round)	src/components/VideoLibraryModal.vue:1254, button click	per user action
processChunkGroup (changed)	src/components/VideoLibraryModal.vue:1249, button click	per user action
telemetryOverlayWindowCandidates (new, src/libs/video-telemetry.ts)	resolveTelemetryOverlayFile and generateTelemetryOverlay	per user action
generateTelemetryOverlay (changed)	mediaRecorder.onstop (src/stores/video.ts:1169) via :1210 — fires once when a recording ends, from the Stop button, stream teardown or a dropped link	per user action

All three call sites already log the interaction (logUserAction at src/components/VideoLibraryModal.vue:1250,1255, and src/components/mini-widgets/MiniVideoRecorder.vue:339,365 for the recording whose end reaches onstop), so no new logging is owed.

Invariants

• On Standalone, the file the consumer reads back from the videos folder exists. Two consumers, and as of this push both are closed. processChunkGroup refuses to hand the name to copyTelemetryFile unless the resolver reports stored; the download's consumer is the main-process ZIP builder, which is handed no name at all and simply finds — or does not find — the file, and the priming call now refuses to leave that silent, throwing into its own catch so the user is warned before the archive is built. The asymmetry that was finding 1.5 is gone. The pre-existing hole below it is out of scope and unchanged: copyTelemetryFile still swallows a failure of its own copy (src/electron/services/video-recording.ts:559-561).
• An .ass for a recording is findable by "key contains the hash and ends in .ass". Consumers: findAssTelemetryFile (src/composables/videoChunkManager.ts:286-295), src/electron/services/video-recording.ts:672, and the library's log listing (src/components/VideoLibraryModal.vue:1124). Producers: src/stores/video.ts:809, the ZIP-extraction path (src/electron/services/video-recording.ts:445-446), and resolveTelemetryOverlayFile, whose name always embeds #<hash> via videoFilename (src/utils/video.ts:13-17) and always ends in .ass via videoSubtitlesFilename (:45-47). Verified at the byte level for the Electron path — ElectronStorage(['videos']).setItem writes <cockpit folder>/videos/<key> (src/libs/videoStorage.ts:137,142, src/electron/services/storage.ts:51-56) and filesystemStorage.keys(['videos']) lists that same folder.
• The candidate loop can only advance when a failed window throws. datalogger.generateLog throws on an empty window (src/libs/sensors-logging.ts:523-525) and its declared return is non-optional (:510), so both the fall-through and the unchecked toAssOverlay(telemetryLog, …) are sound.
• The recording-finish path always has a first candidate. onstop sets info.dateFinish at src/stores/video.ts:1186, two statements before generateTelemetryOverlay runs, so the new if (!overlayWindow) throw is unreachable on the normal path and only replaces the error an undefined date used to raise inside generateLog.
• The dates the helper reads are Date objects. They are not, after a reload: unprocessedVideos is a vueuse useStorage on cockpit-unprocessed-video-info (src/stores/video.ts:83), so JSON serialization hands back ISO strings. The helper's new Date(recording.dateStart) tolerates both, where the base store path's recordingData.dateStart!.getTime() would have thrown — covered, and slightly better than the base.

2. Persistence & User Data — inventory, no findings

Key / store	Backend	What happened
<recording name>.ass in videoStorage	Machine-local. Electron: the videos/ folder via ElectronStorage (src/libs/videoStorage.ts:23-61,137,142). Lite: the Cockpit - Video Recovery IndexedDB (:116-121).	New write sites, unchanged key shape (videoSubtitlesFilename(...), the same helper src/stores/video.ts:808 uses). The Standalone processing path already wrote here (base saveAndCopyTelemetry); new is that both download paths write it too. The second commit changes only the contents of the store's own write — the resolution in the header — not the key.
Cockpit (…).clog in the Cockpit - Sensor Logs IndexedDB	Machine-local (src/libs/sensors-logging.ts:293).	Indirect, unchanged shape. datalogger.generateLog persists the generated log as a side effect (:551), so a chunk download on either build now also leaves one telemetry log file behind — the same thing the Standalone processing path has always done.
cockpit-unprocessed-video-info	Machine-local, vueuse useStorage (src/stores/video.ts:83).	Read-only for this PR. No key, shape or default is changed, and no migration is added; the helper simply tolerates the string dates JSON round-tripping produces.

Judged: the first two are media/log artifacts rather than cockpit--prefixed settings, so the naming rule and useBlueOsStorage do not apply, and the third already carries the prefix. Nothing machine-specific is vehicle-synced, no key is reshaped, no value duplicates its own key, and the PR adds no migration and changes no default. On accumulation: deleteChunkGroup removes only tempVideoStorage keys (src/composables/videoChunkManager.ts:221-223), so an overlay generated for a group later deleted outlives it — roughly 8 KB, on both builds, and listed as a log file the user can delete in the video library (src/components/VideoLibraryModal.vue:1124-1126), so not an invisible accumulation and not raised. A failure to write one is a reporting problem rather than a data one, and it is now reported on both Standalone consumers.

Sections with nothing to report (10)

1. Correctness & Implementation Bugs — ✅ (the last open defect, 1.5, is closed by this push and re-verified above against the three consumers of the resolver's return; re-walked the whole diff independently rather than only the delta: the candidate loop's fall-through is sound because generateLog throws on an empty window, stored is now checked at both callers that work from the filename and correctly ignored at the one that works from the blob, the Lite branch's error snackbar became a warning on a path that still completes, no telemetry is read from a Pinia store where the data lake applies, no Electron API is called outside isElectron()/optional chaining, no widget Options object is touched, and the composable's new state is per-call rather than module-scope)

3. AGENTS.md Adherence — ✅ (no new dependency and package.json untouched; the delta this round is three lines inside an existing branch with no new symbol, and every symbol the PR adds has a call site in it — telemetryOverlayWindowCandidates from two files, resolveTelemetryOverlayFile from three sites, findAssTelemetryFile still called by the resolver; JSDoc on resolveTelemetryOverlayFile, telemetryOverlayWindowCandidates, ResolvedTelemetryOverlay and every TelemetryOverlayWindow member has typed @param/@returns with real text, satisfying .eslintrc.cjs:29-46; the domain logic lives in the framework-agnostic src/libs/video-telemetry.ts, which imports no vue; the fix reuses the flag the resolver already returns rather than re-listing storage in the Electron branch; no rename, import reorder or unrelated reflow in this push)

4. Security — ✅ (re-read src/libs/video-telemetry.ts and src/tests/libs/video-telemetry.test.ts in full and every changed hunk of the other two files: plain arithmetic on Date, no network call, no eval/Function, no encoded blob, no hidden or bidirectional Unicode, no environment variable or credential; no dependency, workflow, postinstall script or Electron main-process file is touched — the main-process code cited in the change map is read, not modified; nothing in pr.json, pr.diff, new-comments.json, complexity-report.json, resolutions.json or decisions.json contains text addressed to this reviewer)

5. Performance — ✅ (all four changed functions trace to a button click or to mediaRecorder.onstop, never to onIncomingMessage, dataLake:setVariable or a watcher; the Standalone download carries at most one generateLog, which reads a bounded IDBKeyRange window (src/libs/sensors-logging.ts:517-521) rather than the whole store, with the tier-1 hit short-circuiting it entirely on a recording that already has its .ass; the check added this round is a boolean test on a value already in hand, so it adds no work; no listener, interval or watcher is registered, so nothing needs teardown, and no canvas work is added)

6. UI / UX — ✅ (no dialog, control, overlay or icon is added or restyled, so the dialog-anatomy, theme="dark", button-token, footer, padding, glass and stacking clauses have nothing to bind to; the Standalone download's failure now reaches the same warning snackbar the Lite one uses, through openSnackbar with no duplicate console.* of the same message, and its copy says what the user gets — an archive without the subtitle file — rather than naming an internal failure; the interpolated error names the overlay's filename, which is the name the user sees in the library's log listing, not internal jargon; nothing can open a dialog in a loop, and both entry points already call logUserAction)

7. Code Quality & Style — ✅ (the complexity report, measured at head 52bb993 against base cf83753, attributes one trigger to this PR — downloadChunkGroup at src/composables/videoChunkManager.ts:362, nested-4-or-deeper, with the report's figures being complexity 15 against a base 13 and depth 5 against a base depth 5. Depth equals base depth, so the nesting is inherited and the trigger is answered; the count rose by two, below the report's bump threshold of 5, in a function already above 12, and this round's contribution is one flat if inside an existing try. The report covers all 4 changed files, measured 159 functions and is not truncated. Beyond that: no any or stray type added, window.electronAPI?.createVideoChunksZip keeps its existing optional call, no existing comment is reworded over unchanged code, the comments added by the PR explain why — why a storage failure must not cost an already-built overlay, and why the priming has to precede the IPC call — and every added line fits the 180-column max-len at .eslintrc.cjs:47)

8. Commit Hygiene — ✅ (the branch was rebased again since round 5 — same two subjects, new SHAs 90bf807 and 52bb993, both recommitted at 2026-09-02T17:54:14Z — and the 1.5 fix was folded into an existing commit rather than left as an "address review" or fixup! commit, which is what AGENTS.md asks; which of the two carries the hunk is not determinable from the data on disk, but the function it changes belongs to the first, and the behavior fix still rides alone in the second. Both video: prefixes match the dominant scope-prefixed style in git log and describe their change; no wip commit, no AI-authorship trailer, no #N or closing keyword in either message — the Closes #3000 stays in the PR body where it belongs — no self-correcting or replicated commit, and neither is oversized)

9. Tests — ✅ (src/tests/libs/video-telemetry.test.ts carries the same five cases as rounds 3 through 5, byte-identical in pr.diff, and sits where the tree puts library tests; the fixture satisfies every required member of UnprocessedVideoInfo (src/types/video.ts:133-151) so it typechecks; no existing test was removed, skipped or weakened, and the cases assert observable outputs rather than internals)

10. Documentation — ✅ (the change closes a Lite/Standalone gap rather than adding a Lite-only limitation, and this push closes the last of it, so the README parity table needs no row; JSDoc is present on both new public functions and on both new interfaces, generateTelemetryOverlay's existing block stays accurate after the store change, and the check added this round introduces no public symbol needing one)

11. Nitpicks / Optional — ✅ (the one nit ever raised here — the Standalone path reading the .ass blob through IPC only to use its filename — was settled by maintainer vote in round 3 in favour of the current signature; the priming call inherits that same read on the tier-1 hit, roughly 8 KB over IPC per download, which is the settled trade rather than a new one, so it is not re-raised. Re-read all three callers of resolveTelemetryOverlayFile at head and found no other taste-level call worth a line)

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