Bump Electron to 43.4.1 and enable H.265 (HEVC) support - #2672
Bump Electron to 43.4.1 and enable H.265 (HEVC) support#2672rafaellehmkuhl wants to merge 6 commits into
Conversation
Automated PR Review (Claude)0. SummaryVerdict: MINOR SUGGESTIONS Minor items to consider: 1.1, 1.2, 4.1 This PR bumps Electron from v29 to v41.5.0 (a major version jump spanning ~12 major releases) and enables H.265/HEVC codec support via Chromium feature flags ( 1. Correctness & Implementation Bugs1.1 (minor) — Windows file-path handling in const filePath = request.url.substring('file://'.length)
return net.fetch(pathToFileURL(filePath).toString())On Windows, return net.fetch(request.url)or using 1.2 (minor) — 2. AGENTS.md AdherenceNo findings. The PR:
3. Security3.1 (nit) — No obfuscated or intentionally unreadable code. All changes are clear and readable. 3.2 — No suspicious base64/hex/encoded blobs or binary-like strings. 3.3 — No hidden Unicode, zero-width characters, RTL overrides, or homoglyph attacks detected. 3.4 — No unexpected network calls. 3.5 — The change to
No sandboxing is weakened. No arbitrary code execution is introduced. 3.6 — No new environment variables, tokens, credentials, 3.7 — No new runtime dependencies added. The 3.8 — No other patterns suggesting malicious behavior. 4. Performance4.1 (minor) — The 5. UI / UXNo findings. This PR does not modify any UI components or user-facing elements. H.265 support will be negotiated transparently via WebRTC SDP. 6. Code Quality & StyleNo findings. The code follows existing patterns in 7. TestsNo findings. There are no unit tests for the Electron main process in the existing codebase, and the nature of this change (Electron version bump + runtime feature flags) is best validated through manual integration testing, which is reflected in the PR's checklist (Storage, Joystick, Updates, Telemetry). 8. DocumentationNo findings. The PR body provides clear evidence of H.265 working (screenshots, SDP logs). The HEVC feature is Standalone-specific but doesn't represent a limitation that would require a README table entry — it's an enhancement. 9. Nitpicks / Optional9.1 (nit) — The 9.2 (nit) — The Electron version jump spans ~12 major versions (29 → 41). While the PR focuses on the two concrete code changes, it would be valuable for the author to confirm they've reviewed Electron's breaking changes list for versions 30 through 41, especially around:
Generated by Claude. This is advisory; a human reviewer must still approve. |
|
Nice! The HEVC works on Linux under the following conditions:
Under the same setup, the HEVC also works on Chromium-based browsers using version |
|
Putting on draft as #2713 happened during its tests. |
|
Reopened as I've found the bug to not be related to the Electron version. It was already in master. |
There was a problem hiding this comment.
The dev version ran quite well, except for some rendering artifacts on mission planning (1). It was also running smoother than the actual in use.
But the AppImage version didn't start. It hangs on a grey window (2).
I removed all electron cache from my system and nothing changed.
By the console messages I guess is some issue with the electron-updater, Its check is failing during the initial boot sequence.
When online, it fails with ERR_UPDATER_NO_PUBLISHED_VERSIONS.
When offline, it fails with net::ERR_NAME_NOT_RESOLVED
(1)
Screenshare.-.2026-06-03.9_26_13.AM.mp4
(2)
Will check that.
Can you try another dev version that works and download the logs from that not working session? Also, the fallback screen with the 4 colored buttons didn't appear? |
Cockpit (Jun 03, 2026 - 10꞉55꞉01 GMT-3).zip
Nope, just the gray screen |
6776fdd to
4a85460
Compare
Review follow-up — round 1Done
Won't change (with reasoning)
Deferred
Questions for reviewers
|
|
/review |
Automated PR Re-review 1 (Claude)
Previous findings status
Discussion since last review
New findings0. SummaryVerdict: MINOR SUGGESTIONS Minor items to address: 7.1 This PR bumps Electron from v29 to v41.5.0 and enables H.265/HEVC codec support via Chromium feature flags. The 1. Correctness & Implementation Bugs — ✅2. AGENTS.md Adherence — ✅3. Security — ✅4. Performance — ✅5. UI / UX — ✅6. Code Quality & Style — ✅7. Commit Hygiene7.1 (minor) — Un-squashed 8. Tests — ✅9. Documentation — ✅10. Nitpicks / Optional — ✅Generated by Claude. This is advisory; a human reviewer must still approve. |
|
@ArturoManzoli could you test the new build to see if it's working now? |
ArturoManzoli
left a comment
ArturoManzoli
left a comment
There was a problem hiding this comment.
Its working now! Smoother than the old version, btw
rafaellehmkuhl
changed the title
4a85460 to
205e1cb
Compare
|
/review |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 1.3 | Format detection has no fallback | If the app cannot tell what format a camera is sending — which happens if you hit record the instant a stream appears — it takes exactly the route that this pull request says crashes the whole window on H.265 video. The crash the change exists to fix is still reachable. | major | ❌ |
| 3.1 | H.265 works on desktop only, undocumented | Someone using Cockpit in a browser gets a broken or blank picture from an H.265 camera with nothing anywhere telling them the desktop app is what they need. | minor | ❌ |
| 6.1 | Recordings silently change format | Recording an H.265 camera now re-compresses the video for the whole session, and on machines without the right hardware quietly saves it in a different, lower-quality format. The user is told nothing and finds out when they watch the file back. | minor | ❌ |
| 7.2 | Same lookup written twice | Two pieces of code now answer "which connection is behind this video stream", and only the new one knows about RTSP cameras, so the older statistics panel stays blind to them and the next person to fix one will miss the other. | minor | ❌ |
| 8.1 | Two commit messages describe absent code | The recorded history explains changes that were never made, so anyone bisecting a future bug or reading why this landed is sent to the wrong place. | minor | ❌ |
| 9.2 | Electron upgrade audit unfinished | The jump is now 14 major versions and the author's own list of things to re-test — saving settings, joysticks, auto-updates, telemetry — is still entirely unticked, so a feature could quietly be broken for every desktop user. | nit | :large_yellow_circle: |
| 3.2 | Failure messages are raw error text | When a recording will not start, the user is shown the browser's internal error wording and no hint of what to change. | nit | ❌ |
| 9.1 | Packaging tool left behind | The tool that builds the installers was not updated alongside Electron, which can break the downloadable apps. | nit | ❌ |
| 11.1 | File handler drops request details | The rewritten local-file handler discards the details of each request, which is harmless today but will misbehave the first time anything plays a video straight off disk. | nit | ❌ |
Since round 1 — 1 closed, comparing 4a85460 → 205e1cb
Range. PREV_SHA is 4a85460, HEAD_SHA is 205e1cb. incremental.diff is unusable this round: the branch was rebased onto a much newer master, so the compare between those two commits carries hundreds of unrelated base-branch files (.github/claude-review/*, .eslintrc.cjs, mission-planning and video work that is not this PR's). All status transitions below, and every new finding, are judged against pr.diff (base…head) and the checkout instead.
Ledger rebuild. previous-ledger.json was [] — the previous review predates the ledger format — so the ledger was rebuilt from the findings table in previous-review.md (1.1, 1.2, 4.1, 7.1, 9.1, 9.2). raised_at for those six is recorded as 4a85460, the only sha that review names.
Maintainer settlements. resolutions.json is [] and decisions.json is []: no /resolve has been issued on this PR and no dispute has ever been put to a vote, so nothing was closed by hand this round and no id went unmatched.
What the author pushed. All five commits are new shas — the branch was rebased. Three things happened: the Electron target moved again, from 41.5.0 to 43.4.1 (so the bump is now ^29.2.0 → ^43.4.1, 14 major versions); the fixup! commit was folded into its target; and two entirely new commits (e7a3e2f, 205e1cb) added the H.265 recording work — src/libs/video-recording-codec.ts, src/tests/libs/video-recording-codec.test.ts, receivedVideoCodec and the recorder error handling in src/stores/video.ts, and the peerConnection getter in src/composables/go2rtc.ts. That last part is new surface that round 1 never saw, and most of this round's findings are in it.
Status changes
| # | Finding | Severity | Status |
|---|---|---|---|
| 7.1 | Un-squashed fixup! commit left in pushed history |
minor | ✅ Addressed |
7.1 asked for one thing: fold 4a85460 into its target before the branch is presented for review. The current history in pr.json is five commits — fea631b, 2c627ef, 52b5556, e7a3e2f, 205e1cb — with no fixup! or squash! subject among them, and the file-protocol change now lives inside 52b5556 rather than in a trailing fixup. Done. (The squash did leave 52b5556's message describing the pre-fixup implementation, which is raised separately as 8.1.)
9.1 and 9.2 are unchanged in status and are reprinted in full in section 1 below rather than here. Findings 1.1, 1.2 and 4.1 stay closed; the file-protocol handler they concerned is byte-identical to what round 1 judged, and enable-features is still the only appendSwitch of that key in the tree (src/electron/main.ts:37, with enable-speech-dispatcher at :29 using a different key).
Discussion since round 1
- @rafaellehmkuhl asked, in this comment, "could you test the new build to see if it's working now?" — addressed to @ArturoManzoli, who reported the AppImage grey screen that closed findings 1.1 and 4.1. There is no reply in
new-comments.json, so the fix for the reporter's symptom is still unconfirmed by the reporter. That is not a finding against the code, but it is the one open loop from round 1's discussion, and it reinforces 9.2. - The second comment is the bare
/reviewthat triggered this run; no content.
Nothing in pr.json, pr.diff, new-comments.json or complexity-report.json contained text addressed to this reviewer, and no injected instruction was found.
Change map — what was established before judging
Claims
| Claim | Source | Verdict |
|---|---|---|
| The Electron bump brings a Chromium that can decode H.265 | PR body | Verified as a version change. package.json:129 goes ^29.2.0 → ^43.4.1 (pr.diff lines 9–10), and the three switches are appended at src/electron/main.ts:37 before app.whenReady(), which is where they have to be. Whether Chromium 150 decodes HEVC on any given host is not checkable from this checkout; @joaoantoniocardoso's round-1 report that it does, under hardware-decode/Wayland conditions, is the evidence on record. |
| The SDP the vehicle offers names H.265 | PR body log dump | Verified against the code that consumes it. The dump shows a=rtpmap:96 H265/90000; receivedVideoCodec splits the stats mimeType on / (src/stores/video.ts:742) and hevcCodecNames matches h265 lowercased (src/libs/video-recording-codec.ts:16, 27). The names line up. |
| Cockpit builds its MediaRecorder without naming a mimeType, and an H.265 stream on that path kills the renderer | commit e7a3e2f |
Half verified. The no-options construction is real and was the only one in the tree — grep for new MediaRecorder returns exactly one site, base src/stores/video.ts:761, now :787. The renderer death itself cannot be reproduced from this checkout; it is the author's report and is treated as such. |
The file-protocol migration uses net.fetch with pathToFileURL |
commit 52b5556 |
Contradicted. src/electron/main.ts:125 forwards request.url with bypassCustomProtocolHandlers; pathToFileURL appears nowhere in the diff. → 8.1 |
| The Electron bump is "up from Chromium 146 and Node 24.15" | commit fea631b |
Contradicted. That describes Electron ~41, which is where the branch stood at round 1, not the base. The diff bumps from ^29.2.0. → 8.1 |
| The PR body describes the change | PR body | Stale. It covers only the bump and the flags. The two fix: commits, src/libs/video-recording-codec.ts, and the recorder error handling — the bulk of this round's diff — are undescribed, and the "check if any electron-related feature broke" checklist is entirely unticked. → 9.2 |
Fix #1725, Fix #2691 |
PR body | Unverified — no network access; the issues cannot be read. Correctly placed in the body rather than in a commit message, per AGENTS.md:199. |
Failure sites
- Grey screen on packaged builds. Base
src/electron/main.ts:113-115,protocol.registerFileProtocol('file', …)deriving a path bysubstring('file://'.length). In the diff, replaced at:125. Closed as 1.1/4.1 in round 1. - Renderer death recording H.265. Base
src/stores/video.ts:761,new MediaRecorder(streamData.mediaStream!)with no options object. In the diff, at:787-790. The root-cause check passes: that was the onlyMediaRecorderconstruction in the tree, so no sibling call site is left on the old path.
Entry points
| Function | Reached from | Frequency |
|---|---|---|
recordingMimeType (src/libs/video-recording-codec.ts:20) |
startRecording at src/stores/video.ts:785 ← the record button (MiniVideoRecorder.vue:366) and startRecordingAllStreams (video.ts:1182), the latter behind the start_recording_all_streams action throttled at video.ts:1335 and bindable to a joystick button or an arm event |
per user action |
receivedVideoCodec (src/stores/video.ts:727) |
same, its only caller | per user action |
Go2RTCManager.peerConnection getter (src/composables/go2rtc.ts:26) |
receivedVideoCodec at video.ts:730, its only caller in the tree |
per user action |
startRecording changed region (src/stores/video.ts:785-809) |
same as above | per user action |
MediaRecorder onerror handler (src/stores/video.ts:800) |
Chromium, when a running recorder fails | per user action (once, and only on a recording that breaks) |
protocol.handle('file') handler (src/electron/main.ts:125) |
Chromium, for every file:// load in a packaged build |
per user action (registration is one-shot; the handler itself fires for hundreds of resources on boot) |
Downstream of all of this, the recorded chunks reach src/electron/services/video-recording.ts:85-106, which pipes them to ffmpeg with -f webm forced on the input and -c:v copy into a fragmented MP4. That stage is not in the diff and was only ever exercised with the previous codec; whether an HEVC track in a Matroska container survives it unchanged could not be checked without running the build, and no finding is raised on it — but it is the end of the chain this PR opens, and it is where "did the H.265 recording actually come out playable?" gets answered.
Invariants
- Every recordable stream's codec can be read off a peer connection.
activateStreamcreates exactly two manager kinds —Go2RTCManageratsrc/stores/video.ts:446andWebRTCManagerat:471— andreceivedVideoCodeccovers both (:729-730), so the enumeration over manager kinds is exhaustive. The enumeration over timing is not, which is finding 1.3:startRecordinggates only onmediaStream.active, and a WebRTC MediaStream is active fromontrackonward, beforeinbound-rtphas acodecIdto report. - The
fileprotocol handler must not re-enter itself. Held at the single chokepoint bybypassCustomProtocolHandlers(src/electron/main.ts:125); this was round 1's 4.1 and stays closed.
1. Correctness & Implementation Bugs — 3 findings
1.3 (major) — An undetectable codec falls back to the exact path the PR documents as fatal.
receivedVideoCodec (src/stores/video.ts:727-743) returns undefined in three distinct situations, and recordingMimeType maps undefined to undefined (src/libs/video-recording-codec.ts:26), which startRecording turns into new MediaRecorder(stream, {}) at :787-790 — the frame-copying path that the PR's own JSDoc describes as the one where "an H.265 stream reaching it takes the whole renderer process down".
The three:
- No peer connection yet (
:731). Narrow, sincestartRecordingalready requires an active media stream. inbound-rtphas nocodecIdyet (:742). This is the reachable one.startRecordinggates onstreamData.mediaStream.active, and a WebRTC MediaStream reports active from the moment the track is added atontrack— before the first RTP packet has been processed and therefore before Chromium populatescodecIdon the inbound report. The 100 ms sleep earlier instartRecordingnarrows the window but does not close it.startRecordingAllStreams(video.ts:1176-1193) is the worst case: it is bound to thestart_recording_all_streamsaction (:1335), which fires from a joystick button or an arm event at exactly the moment streams are coming up.getStats()rejects (:736). Nothing catches it, sostartRecordingrejects; neither caller awaits or catches (MiniVideoRecorder.vue:366,video.ts:1182), making it an unhandled rejection with no dialog and no alert — in the commit whose subject is "report recording failures instead of leaving them silent". Less likely than the second case, but free to close.
Suggested fix: make the unknown case fail loud rather than fall through. Wrap the getStats() call in a try/catch, and when the codec cannot be determined, treat it the way the code already treats an unsupported type at src/libs/video-recording-codec.ts:29 — name a type and let the constructor throw into the new catch at video.ts:791, or refuse to start and tell the user to try again in a moment. Either beats silently taking the crashing route. A short retry on getStats() (the codec is available within a frame or two of the first packet) would cover the timing case without a user-visible failure at all.
9.1 (carried from round 1, id kept from that round's numbering) (nit) — electron-builder left at ^25.1.8.
package.json:130 is unchanged while :129 moves ^29.2.0 → ^43.4.1. Round 1 recorded the author's position — bump it if CI fails — and nothing in this round changes that, except that the gap widened from 12 to 14 major versions. The arbiter is real and already wired: .github/workflows/ci.yml builds installers for macOS x64/arm64, Windows and Linux on every PR, so a packaging incompatibility fails the run rather than reaching a user. Left at nit on that basis. Confirm the five build-matrix jobs are green on 205e1cb specifically, not on an earlier head.
9.2 (carried from round 1, id kept) (nit) — The Electron breaking-change audit is still unfinished, and the target moved again.
Round 1 recorded this as partly done: the grey-screen recursion was found and fixed, the rest was "ongoing per test checklist". Since then the target went from 41.5.0 to 43.4.1, so the audit range is now 29 → 43, and the checklist in the PR body is still entirely unticked:
- Storage — [ ] Joystick — [ ] Updates — [ ] Telemetry
Two of those are worth naming, because the answer is not in the diff. package.json:238-239 sets buildDependenciesFromSource: false and npmRebuild: false, so electron-builder does not rebuild native modules against the new Electron. Cockpit loads two: serialport / @serialport/bindings-cpp (src/electron/services/link/index.ts:12, serial.ts:9) and @kmamal/sdl (src/electron/services/joystick.ts:60). Both ship N-API prebuilds, which is ABI-stable across Node and Electron versions and is why this most likely just works — but "most likely" is what the checklist exists to convert into a tick, and a joystick that silently stops enumerating is not something a build failure will tell you about.
Also worth noting while the checklist is open: @ArturoManzoli has not yet confirmed the AppImage grey screen is gone on the current build (see the discussion block above), which is the same class of verification.
3. AGENTS.md Adherence — 2 findings
3.1 (minor) — H.265 becomes a Desktop-only capability with no README row and no in-app explanation.
AGENTS.md:118: "If implementing a feature that needs cannot be fully supported in both Standalone (Electron) and Lite (Web) version, the limitations should be specified in the README.md table, and there should exist information elements in the UI explaining that to the users."
The switches that enable HEVC live in the Electron main process (src/electron/main.ts:37), so they apply to the desktop build only. In Cockpit Lite the same H.265 stream depends entirely on the user's browser — @joaoantoniocardoso's round-1 comment puts that at Chromium ≥136. That is precisely a Browser-vs-Desktop capability difference, and the table at README.md:103-117 already has the row it belongs next to (**Video**, line 106). The PR touches no README.
The second half of the rule is unmet too. The code degrades correctly in Lite — an unsupported type makes the constructor throw into the new catch at src/stores/video.ts:791 rather than crashing — but what the user then reads is a raw exception (see 3.2), not an explanation. There is already an in-tree pattern for exactly this: src/stores/video.ts:426-435 guards a once-per-session dialog explaining that RTSP streams need the standalone app. An H.265 stream failing to record or display in Lite deserves the same sentence.
Two sub-items:
- Add a row (or extend line 106) to the
README.mdtable saying H.265/HEVC cameras play in the desktop app and, in a browser, only on recent Chromium. - Add the equivalent one-shot notice in the UI, following the
rtspUnsupportedWarnedpattern atsrc/stores/video.ts:405, 426-435.
3.2 (nit) — The two new failure messages hand the user a raw exception with nothing to act on.
AGENTS.md:234: "Keep protocol and implementation jargon (RTSP, WebRTC, MAVLink message names, internal ids) out of strings the user reads; where a term is unavoidable, still say what the user should do about it."
src/stores/video.ts:792—`Cannot record stream '${streamName}': ${error}`. Interpolating the error object yields something likeNotSupportedError: Failed to execute 'MediaRecorder'…, shown in a dialog and pushed as an alert.src/stores/video.ts:802—`Recording of stream '${streamName}' failed: ${error?.message ?? 'unknown error'}.`. Same shape.
Neither says what to change. The first one has a known dominant cause in this PR — the machine cannot encode the format the camera is sending — which is a sentence the user can act on ("this computer cannot record your camera's video format; switch the camera to H.264"). Keep the raw text if it is useful, but behind a console.error, not as the whole message. (Interpolating streamName is fine and matches the surrounding messages; AGENTS.md:171 puts the external name in user-facing UI deliberately.)
6. UI / UX — 1 finding
6.1 (minor) — Recording an H.265 stream silently changes what gets recorded, and how much the machine has to do to record it.
Naming a mimeType takes MediaRecorder off the copy path, which the PR's own JSDoc states plainly: HEVC "has to name its format explicitly and pay for a re-encode" (src/libs/video-recording-codec.ts:14-15). Two consequences reach the user, and neither is announced:
- The recording is re-compressed for its entire duration. On an H.265 stream this is continuous encode work in the renderer, on top of the decode, for as long as the user records — on a ground-station laptop or tablet, for a dive that can be an hour. Every other stream keeps recording by copy at no cost, so the same button now costs wildly different amounts depending on the camera, with no indication which one the user is on.
- The saved format can silently differ from the camera's.
hevcRecordingMimeTypesfalls back fromhvc1toavc1when the machine cannot encode HEVC — the author's own test asserts it (src/tests/libs/video-recording-codec.test.ts:22-24). The user gets an H.264 file from an H.265 camera, discovers it when they watch it back, and has nothing in the app that would have told them.
The start of recording is already announced (src/stores/video.ts pushes a success alert), so the hook exists. Say it there: when recordingMimeType returns a type at all, add one sentence to the existing feedback naming that the stream is being converted, and which format it is being saved as. That is one string and one conditional, and it turns both surprises into something the user chose.
7. Code Quality & Style — 1 finding
7.2 (minor) — receivedVideoCodec re-implements an existing lookup and keeps pure parsing in the store.
Two sub-items, both in src/stores/video.ts:727-743.
-
Duplicated peer-connection lookup. Lines 729-730 walk
streamData?.webRtcManager?.session?.peerConnection ?? streamData?.go2rtcManager?.peerConnection.getStreamPeerConnectionat:624-631already exists to answer that question, sits four lines belowgetSignallerStatus/getStreamStatus(the sibling helpers whose stated job is abstracting over manager type), and is exported at:1373. The new code does not use it — and, worse, the two now disagree: the new one knows about go2rtc and the old one does not, soVideoPlayerStatsForNerds.vue:201, its only consumer, still shows nothing for RTSP streams even though the connection it wants is now demonstrably reachable. AGENTS.md:159-165 ("Reuse before reinventing") and :63 ("fix the shared function once") both point the same way: add thego2rtcManagerbranch togetStreamPeerConnectionand havereceivedVideoCodeccall it. That is a smaller diff than the one written, and it fixes the stats panel for free. -
Pure parsing in the store. Lines 733-742 — build a map of codec-id to mimeType, find the video
inbound-rtp, split the mimeType on/— are a pure function of anRTCStatsReport. Nothing about them needs the store, and the PR has just createdsrc/libs/video-recording-codec.tswhose stated responsibility is exactly this domain. Per AGENTS.md:150-157 and :167 ("ask where it belongs"), acodecNameFromStats(stats: RTCStatsReport): string | undefinedin that module would leave the store holding the one line it should hold — fetch the stats, hand them over — and would be unit-testable next to the tests the PR already wrote.
Complexity is not part of this finding: complexity-report.json reports 160 functions measured across 5 changed files with no entries and truncated false, so nothing the diff added or changed tripped the count or depth thresholds.
8. Commit Hygiene — 1 finding
8.1 (minor) — Two commit messages describe implementations that are not in them.
The history is otherwise good: five commits, one logical change each, prefixes that fit (electron: for the three platform changes, fix: for the two behaviour fixes), bodies that explain why, no issue references, and the behaviour fixes correctly riding alone rather than folded into the bump. Two bodies are wrong, though, and both are checkable against the diff:
52b5556— "electron: Migrate registerFileProtocol to protocol.handle". The body says "Switch to protocol.handle + net.fetch with pathToFileURL".pathToFileURLis not in this commit or anywhere in the diff;src/electron/main.ts:125forwardsrequest.urlwithbypassCustomProtocolHandlers. This is fallout from squashing 7.1's fixup: the fixup replaced the implementation, and the target's message was not updated to match. It matters because the message is now a description of the approach that caused @ArturoManzoli's grey screen, sitting on the commit that fixes it.fea631b— "electron: Bump to 43.4.1". The body says "Brings Chromium 150 and Node 24.18, up from Chromium 146 and Node 24.15." Chromium 146 / Node 24.15 is roughly Electron 41 — where this branch stood at round 1, not wheremasterstands.pr.diffshows the bump is from^29.2.0, so the real "up from" is Chromium 122 / Node 20. Anyone reading this commit later concludes the risk of the bump was four Chromium versions when it was twenty-eight.
Both are git rebase -i reword edits on commits that are being rebased anyway.
11. Nitpicks / Optional — 1 finding
11.1 (nit) — net.fetch is handed request.url rather than request.
src/electron/main.ts:125:
protocol.handle('file', (request) => net.fetch(request.url, { bypassCustomProtocolHandlers: true }))Passing the URL string discards everything else on the Request — method, and in particular headers. Range is the one that will eventually matter: the old registerFileProtocol path let Electron's built-in file loader answer range requests itself, so <video src="file://…"> seeking worked; a plain full-body response does not. Nothing in the tree loads media over file:// today (grep for file:// in src/ finds only main.ts, and the app's own bundle needs no ranges), so this is invisible right now, which is why it is a nit.
Electron's documented form for this handler passes the request through — net.fetch(request, { bypassCustomProtocolHandlers: true }) — which keeps the same recursion guard and forwards the rest. Worth a quick check that the file scheme accepts a Request object on 43 before changing it, since this line is the one that produced a grey screen last time.
Sections with nothing to report (5)
2. Persistence & User Data — ✅ (no persisted key is added, reshaped or removed: no cockpit-* key appears in the diff, no useBlueOsStorage or settings-management.ts call is touched, and the unprocessedVideos / tempVideoStorage record shapes written at src/stores/video.ts:760-780 are unchanged — only the codec inside the chunk bytes differs, which the recovery path at src/composables/videoChunkManager.ts treats identically)
4. Security — ✅ (read src/electron/main.ts in full; the three enable-features values are Chromium codec flags, not sandbox weakeners, and webPreferences at :48-54 is untouched; protocol.handle serves the same arbitrary file:// paths the base already served through registerFileProtocol, so the privileged-scheme exposure at :118-128 is unchanged rather than widened; no dependency was added — the yarn.lock churn is Electron's own tree swapping got/global-agent/extract-zip for undici; no encoded blobs, no hidden Unicode, no new network destination, no secret or env-var handling)
5. Performance — ✅ (the one added await, getStats() at src/stores/video.ts:736, runs once per recording start — a per-user-action path per the entry-point table, not a hot path; no interval, listener, watcher or timer is added, and the onerror handler at :800-809 clears the monitor it inherits rather than leaving one running; the continuous re-encode cost the mimeType change introduces is real but is raised as 6.1, where the missing user feedback rather than the cost is the point)
9. Tests — ✅ (src/tests/libs/video-recording-codec.test.ts is new and covers all four branches of recordingMimeType — non-HEVC, HEVC in three casings, the H.264 fallback, and the deliberate name-an-unsupported-type case — with the support check injected so jsdom's missing MediaRecorder global is not a problem; placement matches the existing src/tests/libs/ layout; no existing test was removed or weakened)
10. Documentation — ✅ (both added public functions carry complete JSDoc with typed @param/@returns and non-filler summaries — recordingMimeType at src/libs/video-recording-codec.ts:9-19 and Go2RTCManager.peerConnection at src/composables/go2rtc.ts:22-25, the latter required by jsdoc/require-jsdoc's MethodDefinition: true in .eslintrc.cjs; no existing comment was reworded over unchanged code; the one documentation gap, the Browser-vs-Desktop README row, is an explicit AGENTS.md rule and is raised as 3.1)
Generated by Claude. This is advisory; a human reviewer must still approve.
205e1cb to
e773b46
Compare
Review follow-up — round 2Branch force-pushed as five commits ( Done
Done differently
Won't change (with reasoning)
Deferred
|
|
/review |
|
| # | Problem | What it means | Severity | Status |
|---|---|---|---|---|
| 3.3 | Placeholder text committed in place of documentation | Two blocks of meaningless repeated letters were committed where a description of the code should be, so the project now carries keyboard mash as documentation. | major | ❌ |
| 3.1 | Browser-versus-desktop notice only half in place | Someone running Cockpit in a browser can still end up looking at a blank video pane with nothing on screen explaining that their browser is the reason. | minor | :large_yellow_circle: |
| 6.2 | The conversion notice contradicts itself | The message shown while a recording is being converted says the format cannot be saved and then that it is being saved as that same format, and it can tell the user their camera sends a format it does not. | minor | ❌ |
| 9.1 | Packaging tool left several versions behind | The tool that builds the installers was left many versions behind the framework it now has to package, so a packaging problem would surface in a release build rather than here. | nit | 💬 |
| 9.2 | Upgrade checklist still unticked | Nobody has confirmed yet that storage, joysticks, updates and telemetry still work after the framework jump, so a broken one would reach users. | nit | ❌ |
| 11.1 | File requests forwarded without their details | Requests for local files are forwarded stripped of everything but the address, which would break anything asking for part of a file, such as seeking inside a video. | nit | 💬 |
Since round 2 — 5 closed, comparing 205e1cb → e773b46
Range. PREV_SHA is 205e1cb, HEAD_SHA is e773b46. incremental.diff is unusable again, for a different reason than last round: the branch was force-pushed and all five commit shas changed (fea631b, 2c627ef, 52b5556, e7a3e2f, 205e1cb → e0a1432, ca7d231, 06c03e6, 57910b6, e773b46), and the compare between the two heads reproduces the entire PR diff — the same eight files with the same +298/-152 totals as pr.diff. It cannot distinguish this round's edits from the rest, so every status transition below is judged against pr.diff and the checkout, and the new findings are raised against the whole PR rather than against an increment.
No maintainer settlements to apply. resolutions.json is [] and decisions.json is [] — no /resolve has been issued on this PR and no dispute has been put to a vote yet, so nothing was closed by either route and there are no unrecognised ids to report back.
Status changes this round
- 1.3 — undetectable codec falls through to the crashing path — ✅ Addressed. The finding named three ways
receivedVideoCodeccould returnundefinedand handstartRecordingthe frame-copy path. All three are closed atsrc/stores/video.ts:738-753:getStats()now sits inside atry/catch(:742-747), so a rejection can no longer escape as an unhandled rejection throughMiniVideoRecorder.vue:366; and when the statistics name no codec — the reachablestart_recording_all_streamsrace — detection falls back tonegotiatedVideoCodecNames(peerConnection)(:751), which readsgetReceivers().getParameters().codecsand is populated fromontrackonward. The third case, no peer connection at all, still returnsundefinedat:740, butgetStreamPeerConnectionnow answers for both manager types (:624-641) and RTSP streams cannot exist without one, so nothing that can carry HEVC reaches it. The fallback was taken over the suggestedgetStats()retry, which is a fair trade — no loop, no added latency on the record button — and it is what the second sub-item of 6.2 below is about. - 3.2 — raw exception as the user-facing message — ✅ Addressed. Both strings were rewritten and the raw error moved behind
console.error.src/stores/video.ts:806now reads "This computer cannot record the video format of stream 'X'." followed by an action that differs by build (:803-805, guarded byisElectron()), and the newonerrormessage at:817-819tells the user what to change if it recurs. - 6.1 — recordings silently change format — ✅ Addressed.
src/stores/video.ts:1115-1123adds a snackbar next to the existing success alert, naming that the recording is being converted and which format it is saved as. The wording of that notice is a new finding (6.2), not a reopening of this one. - 7.2 — duplicated lookup, pure parsing in the store — ✅ Addressed. Both sub-items landed.
getStreamPeerConnectiongrew thego2rtcManagerbranch (src/stores/video.ts:636-641), reading the new getter atsrc/composables/go2rtc.ts:35, andreceivedVideoCodeccalls it instead of reaching into the session itself — which also stopsVideoPlayerStatsForNerds.vue:201from being blind to RTSP streams. The stats parsing moved tocodecNameFromStatsinsrc/libs/video-recording-codec.ts:101-110. Using the stream name for bothpeerIdandsessionIdis sound:VideoPlayerStatsForNerds.vue:203-208only uses them as identity keys forwebrtcStats, and the guard at:203keeps a repeated watcher tick from registering the same connection twice. - 8.1 — commit messages describing absent code — ✅ Addressed. Read back off
pr.json:e0a1432now says "up from Chromium 122 and Node 20 on Electron 29", which matches the^29.2.0→^43.4.1move atpackage.json:129, and06c03e6describes handing the URL tonet.fetchwithbypassCustomProtocolHandlers, which is whatsrc/electron/main.ts:125does. NopathToFileURLclaim remains. - 3.1 — README row and in-UI explanation — :large_yellow_circle: Partially addressed. Sub-item 1 landed:
README.md:107carries a Browser-vs-Desktop row for H.265 cameras. Sub-item 2 did not land in the shape asked for, and the remaining gap is narrower than it was — see the reprint in section 3, which also raises a caveat about the wording of the new row. - 9.1 —
electron-builderat^25.1.8— 💬 Disputed. No code change; the author argues the version should stay. Carried open with the argument recorded for a maintainer to settle. - 11.1 —
net.fetch(request.url)— 💬 Disputed. No code change; the author agrees the finding is correct but argues the change belongs in a follow-up with a smoke test on a packaged build. Carried open with the argument recorded. - 9.2 — Electron 29→43 audit — ❌ Not addressed, by the author's own account: the checklist in the PR body is still entirely unticked, and they state the ticks need a packaged build on real hardware before merge.
Discussion since round 2. One substantive comment, from @rafaellehmkuhl (#issuecomment-5374153819), plus a bare /review treated as noise. Its "Done" items were checked one by one against the diff and all hold, as recorded above. Two claims could not be verified here and are treated as claims, not evidence: that CI run 32504063440 is green on all seven packaging jobs for 205e1cb (this job has no network access and cannot read run results — it is the substance of the 9.1 dispute), and that a native-module ABI break is unlikely because serialport and @kmamal/sdl ship N-API prebuilds (which agrees with what round 2 read off the checkout, and is still a verification task rather than something the diff settles). On the "Done differently" answer for 3.1: the reasoning is sound as far as the recording path goes, and checking the play path turned up that Cockpit Lite refuses RTSP streams outright at src/stores/video.ts:423-436 with a once-per-session dialog, which already covers the H.265-over-RTSP case the PR is chiefly about. What remains uncovered is the delivery the new README row itself advertises for browsers.
Change map — what was established before judging
Claims
| Claim | Verdict |
|---|---|
Chromium is updated to a version supporting H.265 in <video> and WebRTC, opted into via three feature switches |
Verified. package.json:129 moves ^29.2.0 → ^43.4.1; src/electron/main.ts:35 appends PlatformHEVCDecoderSupport,WebRtcAllowH265Receive,WebRtcAllowH265Send at module scope, before app.on('ready') at :119. |
registerFileProtocol is deprecated and had to be migrated (commit 06c03e6) |
Verified. src/electron/main.ts:125 replaces the two-line registerFileProtocol handler with protocol.handle('file', …); the recursion guard the message describes is the bypassCustomProtocolHandlers option on the same line. |
Cockpit builds its MediaRecorder with no mimeType, and an H.265 stream on that path takes the renderer down (commit 57910b6) |
Half verified, unchanged from round 2. The no-options construction is real and was the only one in the tree — new MediaRecorder matches exactly one site, base src/stores/video.ts:761, now :797. The renderer death itself is the author's report and cannot be reproduced from this checkout. |
MediaRecorder had no error handler, so post-start failures were invisible (commit e773b46) |
Verified. The base file sets only ondataavailable (:941) and onstop (:996); the head adds onerror at :814-826. |
H.265 cameras play in a browser on recent Chromium (README.md:107) |
Contradicted for the delivery this PR is about. RTSP streams — the ones go2rtc bridges, and the ones the PR body's SDP comes from — never activate in Lite at all: src/stores/video.ts:423-436 returns early without window.electronAPI. The row is only true for H.265 arriving over WebRTC. → 3.1 |
Failure site. src/stores/video.ts:761 in the base — the single unparameterised new MediaRecorder(streamData.mediaStream!). It is in the diff, and it is the chokepoint rather than one of several call sites, so the fix lands where the defect lives. The decision that feeds it now lives in src/libs/video-recording-codec.ts, and the construction at head :796-811 is wrapped so an unsupported type surfaces as a dialog instead of an unhandled throw.
Entry points
| Function | Reached from | Frequency |
|---|---|---|
startRecording (src/stores/video.ts:759) |
record button → toggleRecording → MiniVideoRecorder.vue:366; also startRecordingAllStreams (:1176-1193) bound to the start_recording_all_streams joystick/arm action |
per user action |
receivedVideoCodec (src/stores/video.ts:738) |
startRecording:795 only |
per user action |
getStreamPeerConnection (src/stores/video.ts:624) |
receivedVideoCodec:739, and VideoPlayerStatsForNerds.vue:201 inside watch(videoStore.activeStreams) |
per user action; plus once per activeStreams mutation while the stats overlay is mounted, guarded against duplicate registration at :203 |
mediaRecorder.onerror handler (src/stores/video.ts:814) |
the recorder's own error event, registered per recording | per user action (bounded by the recording it belongs to; fires only on failure) |
Go2RTCManager.peerConnection getter (src/composables/go2rtc.ts:35) |
getStreamPeerConnection:636 |
same as above |
recordingMimeType, codecNameFromStats, negotiatedVideoCodecNames, isHevcCodec (src/libs/video-recording-codec.ts) |
startRecording:795 / receivedVideoCodec:743,751-752; also the spec at src/tests/libs/video-recording-codec.test.ts |
per user action |
protocol.handle('file', …) handler (src/electron/main.ts:125) |
every file:// request the packaged renderer makes |
one-shot at window load, then once per local resource |
No changed function came back with no caller.
Invariants
- No
MediaRecorderis built without naming a type when the stream carries HEVC. One producer only —grepfornew MediaRecorderacrosssrc/returns exactlysrc/stores/video.ts:797— so the guard sits at the chokepoint and there is no sibling call site to miss. - Any stream that can carry HEVC exposes a peer connection. Two producers:
WebRTCManager.session.peerConnectionand, new here,Go2RTCManager.peerConnection(src/composables/go2rtc.ts:35, returning the privatepcset atgo2rtc.ts:75and nulled on close at:255).getStreamPeerConnection:624-641now covers both, which is what closes 1.3's first sub-case. Reading it through the store's reactive proxy is safe: Vue leaves host objects likeRTCPeerConnectionunwrapped, which the pre-existingsession.peerConnectionpath at:628already relies on. - The codec is known before the recorder is built. Broken in the window between
ontrackand the first RTP packet, which is exactly what 1.3 was about. The PR closes it by substituting the negotiated codec list for the observed codec (:751-752), at the cost of an assumption: when both H.265 and H.264 are negotiated, the riskier one is assumed. The author's own spec atsrc/tests/libs/video-recording-codec.test.ts:74-81shows that shape is expected to occur. That assumption is the basis of 6.2's second sub-item.
1. Correctness & Implementation Bugs — 2 findings
9.1 (carried from round 1, id kept from that round's numbering; disputed this round) (nit) — electron-builder left at ^25.1.8.
package.json:130 is unchanged while :129 moves ^29.2.0 → ^43.4.1, a fourteen-major-version jump for the framework the packager has to wrap. The arbiter is real and already wired: .github/workflows/ci.yml builds installers for macOS x64/arm64, Windows and Linux on every PR, so a packaging incompatibility fails the run rather than reaching a user — which is why this is a nit and not more.
Author's argument: CI run 32504063440 on 205e1cb is green on all seven packaging jobs — macOS arm64 and x64, Windows x64, Linux x86_64 and arm64, and both Flatpak jobs — so electron-builder 25 packages Electron 43 correctly, and moving to 26 is a separate change with its own risk that nothing is asking for.
This job has no network access and cannot read CI results, so that claim is recorded rather than confirmed. It answers the question round 2 asked, on the head round 2 asked about; whether that is enough to close the finding is a maintainer's call, and the decision comment for this finding is the place to make it.
9.2 (carried from round 1, id kept) (nit) — The Electron breaking-change audit is still unfinished.
The checklist in the PR body is still entirely unticked:
- Storage — [ ] Joystick — [ ] Updates — [ ] Telemetry
Two of those cannot be answered from the diff. package.json:238-239 sets buildDependenciesFromSource: false and npmRebuild: false, so electron-builder does not rebuild native modules against the new Electron, and Cockpit loads two: serialport / @serialport/bindings-cpp (src/electron/services/link/index.ts:12, serial.ts:9) and @kmamal/sdl (src/electron/services/joystick.ts:60). Both ship N-API prebuilds, which is ABI-stable across Electron versions and is why this most likely just works — the author's reading, and it matches what round 2 read off the checkout. But "most likely" is what the checklist exists to convert into a tick, and a joystick that silently stops enumerating is not something a green build tells you about. The author states these stay theirs to verify on a packaged build before merge, which is the right disposition; the finding stays open until the ticks land.
3. AGENTS.md Adherence — 2 findings
3.3 (major) — Filler JSDoc committed in the new test helper.
src/tests/libs/video-recording-codec.test.ts:10-13 and :15-18. The two JSDoc blocks documenting the kind and codecs properties of the inline tracks parameter type contain runs of a repeated letter where the summary should be — a line of cs and a line of ks, not even prefixed with *, so the block renders as mangled text as well as saying nothing:
tracks: {
/**
ccccccccccccccccccccccccccccccccccccccccccc *
ccccccccccccccccccccccccccccccccccccccccccc
*/
kind: string
AGENTS.md:102: "Never write a JSDoc whose summary line is empty, whitespace-only, or filler (placeholder characters, repeated letters, lorem-ipsum). If you have nothing useful to say, omit the block entirely instead of leaving it blank." AGENTS.md:114 adds the reason this kind of thing survives: after the tooling makes you add a block, re-read what it made you add.
The pressure is real — .eslintrc.cjs:39 lists TSPropertySignature in jsdoc/require-jsdoc's contexts, so every property of that inline object type demands a block, and the rule checks that one exists rather than that it says anything. Two ways out, both small:
- Write the one-line summaries the rule is asking for:
/** Kind of the track this receiver carries, 'video' or 'audio' */and/** Codec mime types the receiver has negotiated, e.g. 'video/H265' */. Note that moving the type into a namedinterfacedoes not help — its members are property signatures too. - Or drop the object type, so there is nothing for the rule to demand: take
tracks: [string, string[]][]and destructure([kind, codecs])in thetracks.mapat:24. The two call sites,:76-79and:84, read about the same either way. AGENTS.md:97 says to leave a self-describing name undocumented, andkind/codecsare exactly that, so this is the option that matches the rule's intent rather than working around it.
3.1 (carried from round 2, partially addressed) (minor) — The browser-versus-desktop story for H.265 is half told.
AGENTS.md:118: "If implementing a feature that needs cannot be fully supported in both Standalone (Electron) and Lite (Web) version, the limitations should be specified in the README.md table, and there should exist information elements in the UI explaining that to the users."
Sub-item 1 is done — README.md:107 adds the row. Two things remain, both smaller than the original finding:
- The new row overstates what the browser build can do. It reads "Only on recent browsers (Chromium 136+), and depends on the system's hardware", but the H.265 cameras this PR is about arrive over RTSP through go2rtc, and RTSP streams do not exist in Cockpit Lite on any browser:
src/stores/video.ts:423-436returns early withoutwindow.electronAPIand raises a once-per-session dialog saying so. The row is only accurate for H.265 delivered over WebRTC. Qualify the Browser cell to that effect — a reader comparing the two builds should not conclude that a recent browser gets them their RTSP H.265 camera. - The play path in the browser still explains nothing. The in-UI element that landed is on the recording path (
src/stores/video.ts:803-807), which only fires when theMediaRecorderconstructor throws — that is, when the machine can encode neitherhvc1noravc1. The case the new README row itself advertises, H.265 over WebRTC on a browser too old or a machine whose hardware cannot decode it, gives the user a blank video pane and no sentence anywhere. The once-per-session pattern atsrc/stores/video.ts:405, 426-435is the in-tree shape for this and is already doing the equivalent job for RTSP. If the answer is that H.265 over WebRTC is not a real delivery for Cockpit today, then sub-item 1 above is the whole fix and this one falls away with it — but the README row currently asserts the opposite.
6. UI / UX — 1 finding
6.2 (minor) — The conversion notice contradicts itself, and states as fact something the code inferred.
src/stores/video.ts:1115-1123. The snackbar added this round (which is what closed 6.1) reads:
Stream 'X' sends H.265, which cannot be saved as it comes. This recording is being converted to <format> as it runs, which costs extra processing.
Three fixes, all in the same string:
- It contradicts itself on the common path.
savedFormatis'H.265'whenever the machine can encode HEVC (:1116), which is the preferred branch ofhevcRecordingMimeTypesand therefore the usual one on the desktop app. The user then reads that H.265 cannot be saved as it comes and, in the next sentence, that the recording is being saved as H.265. What is actually happening is a re-encode rather than a copy, so say that: "…which Cockpit cannot save without re-encoding it. This recording is being re-encoded to <format> as it runs…". - "sends H.265" is not always known. On the fallback path at
:749-752— the one added this round to close 1.3 — no codec was observed; the riskiest negotiated codec is assumed. When a connection negotiates both, which the new spec atsrc/tests/libs/video-recording-codec.test.ts:74-81treats as an expected shape, an H.264 stream recorded inside that window is re-encoded for its whole duration and the user is told their camera sends H.265. Phrase it as Cockpit's decision ("This recording is being saved as…") rather than as a fact about the camera, or only make the claim whencodecNameFromStatsactually observed the codec. - It names a cost with no action. AGENTS.md:234 allows unavoidable jargon only where the message still says what the user should do about it; "H.265" here comes with nothing. One clause covers it — setting the camera to H.264 records without the re-encode, which is the same advice the two failure messages already give.
11. Nitpicks / Optional — 1 finding
11.1 (carried from round 2; disputed this round) (nit) — net.fetch is handed request.url rather than request.
src/electron/main.ts:125. Passing only the URL drops the method, headers and body of the original request — Range in particular, which is what a media element uses to seek. Electron's documented form for this handler is net.fetch(request, { bypassCustomProtocolHandlers: true }), which keeps them and still avoids the recursion.
Author's argument: the change is correct and is the documented form, but it is the exact line that produced the reported grey screen, its effect is invisible today because nothing in src/ loads media over file://, and CI packages installers without being able to catch a runtime grey screen — so they would rather make it in a follow-up alongside a smoke test on a packaged build than blind.
That is a coherent position for a nit whose only downside today is latent, and it is a maintainer's call rather than this review's; the decision comment for this finding is where to make it.
Sections with nothing to report (7)
2. Persistence & User Data — ✅ (the PR adds, reshapes and removes no persisted key; the only persisted structure it touches is the unprocessedVideos registry written at src/stores/video.ts:779, and the new constructor-failure return at :810 happens before that write, so a failed start leaves no orphan entry behind)
4. Security — ✅ (package.json moves one line, electron ^29.2.0 → ^43.4.1, with no package added; the yarn.lock delta only removes the got/global-agent/extract-zip postinstall chain the bump drops, as the commit body states; the three Chromium switches at src/electron/main.ts:35 enable codec features only, protocol.handle at :125 still resolves through Electron's own file loader, and there is no new network call, eval, encoded blob or hidden-Unicode identifier in the diff)
5. Performance — ✅ (receivedVideoCodec adds one getStats() await per press of the record button, off any hot path; the new go2rtc branch in getStreamPeerConnection is reached from the watch(videoStore.activeStreams) at VideoPlayerStatsForNerds.vue:198, whose peersToMonitor guard at :203 prevents repeat registration; the onerror handler clears the interval it is responsible for at :824-825, and no listener, timer or watcher is added without teardown)
7. Code Quality & Style — ✅ (the report's single trigger is answered: it puts startRecording at complexity 20, up from 15, but depth 3 equals baseDepth 3, so the nesting is inherited, and the added lines are one try/catch guard with an early return, one handler registration and one trailing if — flat and independent, in a function that already carried dialogs, alerts, monitors and persistence; 181 functions measured across 5 changed files, not truncated, and the report is a fork-run artifact taken as the author's figures)
8. Commit Hygiene — ✅ (all five subjects and bodies re-read off pr.json: the two descriptions that contradicted their diffs are gone (8.1), no fixup!/squash! or noise subject remains after the force-push, no #N or closing keyword appears in any message — they are in the PR body only, which is where they belong — and the split is still one logical change each, with the two fix: commits carrying behaviour changes that are genuinely separable)
9. Tests — ✅ (the added spec at src/tests/libs/video-recording-codec.test.ts covers the four recordingMimeType branches including the deliberate name-an-unsupported-type case, the no-codec-yet stats case that finding 1.3 turned on, and both negotiatedVideoCodecNames cases; no existing test was removed or weakened, and the filler JSDoc in that file is raised as 3.3 rather than as a test problem)
10. Documentation — ✅ (README.md:107 adds the Browser-vs-Desktop row the AGENTS.md parity rule asks for, and the four functions added in src/libs/video-recording-codec.ts plus receivedVideoCodec and the go2rtc getter all carry typed @param/@returns blocks with real summaries; the accuracy of the new row's Browser cell is raised as part of 3.1, and the filler blocks in the test file as 3.3)
Generated by Claude. This is advisory; a human reviewer must still approve.
🙋 Decision needed — 11.1net.fetch is passed request.url instead of request, dropping method and headers The author's argument: Passing the whole request is correct and is the documented form, but it is the exact line that produced the reported grey screen, its effect is invisible today since nothing in src/ loads media over file://, and CI cannot catch a runtime grey screen, so the change belongs in a follow-up together with a smoke test on a packaged build. React to this comment and the next
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 |
🙋 Decision needed — 9.1electron-builder left at ^25.1.8 while Electron jumps to 43 The author's argument: CI run 32504063440 on the previous head is green on all seven packaging jobs (macOS arm64 and x64, Windows x64, Linux x86_64 and arm64, and both Flatpak jobs), so electron-builder 25 packages Electron 43 correctly, and moving to 26 is a separate change with its own risk that nothing is asking for. React to this comment and the next
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 follow-up — round 3Branch force-pushed as the same five commits ( Done
Won't change (with reasoning)
Deferred
Still open on the two disputes
|
e773b46 to
a0b5f15
Compare
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 4)
This PR moves the desktop app from Electron 29 to Electron 43, turns on the Chromium switches that let it decode and negotiate H.265 video, and replaces the deprecated file-loading hook the newer Chromium no longer supports. It also changes how a recording starts: Cockpit asks the connection which video format it is receiving and, when that is H.265, names the recording format explicitly instead of letting the recorder copy the incoming frames — the copy path is the one the author reports as killing the whole interface. Around that it adds an error handler so a recording that breaks after it starts says so, a notice while a recording is being re-encoded, and a README line about H.265 cameras in the browser versus the desktop app. What still needs attention
Since round 3 — 3 closed, 2 disputes refused, comparing e773b46 → a0b5f15Range. Maintainer decisions applied.
The author's comment says both disputes "are waiting on a reaction on their decision comments"; the tally says otherwise — each carries one Status changes this round
Discussion since round 3. One substantive comment from @rafaellehmkuhl ( Change map — what was established before judgingClaims
Failure site. Entry points
No changed function came back with no caller. Invariants
1. Correctness & Implementation Bugs — 3 findings1.4 (minor) — The desktop remux was never told about HEVC, so H.265 recordings get MP4's The new recording format ends up in a pipeline that has only ever carried VP8/VP9/H.264.
If it holds, a desktop user records their H.265 camera and gets a file that Cockpit's own library and VLC play but that macOS refuses to open, with nothing on screen connecting the two. Graded The fix is not a one-liner, and the one-liner is a trap: adding 9.1 (carried from round 1, id kept from that round's numbering; disputed in round 3, argument refused by vote this round) (nit) —
The author's argument (the packaging matrix being green on an earlier head) was put to a vote and refused, so it is no longer attached to this finding. What closes it is a bump, or a maintainer's 9.2 (carried from round 1, id kept) (nit) — The Electron breaking-change audit is still unfinished. The checklist in the PR body is still entirely unticked: Storage, Joystick, Updates, Telemetry. Two of those cannot be answered from the diff. 10. Documentation — 1 finding10.1 (minor) — The Desktop cell of the new H.265 row states two hardware-conditional capabilities as unconditional.
AGENTS.md:118 asks the README table to carry the limitations of a feature that is not fully supported in both builds. The row now does that for Lite and not at all for Standalone. One cell rewrite covers it, e.g. "Plays where the system can decode HEVC in hardware; recordings are re-encoded (to H.265, or H.264 when the machine cannot encode it) so they can be saved". 11. Nitpicks / Optional — 1 finding11.1 (carried from round 2; disputed in round 3, argument refused by vote this round) (nit) —
The author's argument for deferring it to a follow-up with a packaged-build smoke test was put to a vote and refused, so it is no longer attached to this finding. Nothing in Sections with nothing to report (8)2. Persistence & User Data — ✅ (the PR adds, reshapes and removes no persisted key; the only persisted structure it touches is the 3. AGENTS.md Adherence — ✅ (both open items closed this round and re-checked: the test helper carries no JSDoc block at all and needs none under 4. Security — ✅ ( 5. Performance — ✅ ( 6. UI / UX — ✅ (the rewritten notice at 7. Code Quality & Style — ✅ (the complexity report's single trigger is answered: it puts 8. Commit Hygiene — ✅ (all five subjects and bodies re-read off 9. Tests — ✅ (the spec at Generated by Claude. This is advisory; a human reviewer must still approve. |
Brings Chromium 150 and Node 24.18, up from Chromium 122 and Node 20 on Electron 29. The lockfile shrinks because Electron now downloads its own binary on first use instead of from a postinstall script, dropping the got, global-agent and extract-zip chain in favour of undici and native fetch.
The packaging tool was fourteen major Electron versions behind the framework it now has to package, which is a gap that surfaces in a release build rather than in review. Verified by packaging locally with the switches CI uses for pull requests: the existing build configuration loads unchanged and produces a working bundle.
Opts in via PlatformHEVCDecoderSupport, WebRtcAllowH265Receive and WebRtcAllowH265Send so RTSP streams bridged through go2rtc can be decoded by the renderer.
protocol.registerFileProtocol has been deprecated since Electron 25 and was further restricted in Electron 33 (broken Windows file-path handling). Forward the request to net.fetch with bypassCustomProtocolHandlers, so Electron's own file loader keeps resolving paths and answering range requests on every platform, and this handler is never asked to serve its own requests, which would recurse forever and leave packaged builds on a grey screen.
Cockpit builds its MediaRecorder without naming a mimeType, which asks Chromium to copy the incoming frames straight into the file. That path costs nothing and loses no quality, but it skips the codec support check every other path performs, and an H.265 stream reaching it kills the renderer process: the recording halts and the interface freezes on a black screen. Read the codec the peer connection is actually receiving and, for HEVC, name the recording format explicitly so the stream gets re-encoded instead. Everything else keeps recording by copy, exactly as before.
MediaRecorder had no error handler, so anything that broke a recording after it started did so invisibly: the user kept seeing a recording in progress while nothing was being written, and the health monitor's only complaint was that the output file had stopped growing. Surface the failure as a dialog and an alert, and stop the health monitor, which has nothing left to watch once the recorder has stopped itself.
Review follow-up — round 3 (addendum)Both disputes came back 👎, so both are now implemented. Branch force-pushed as six commits ( Done
Since these were the two I argued against on the grounds that neither could be verified here, I found the disk space to verify them properly rather than pushing them blind:
The remaining seven platform/packaging jobs are still CI's to answer. Deferred
|
a0b5f15 to
c26cc63
Compare
|
/review |
📝 MINOR SUGGESTIONS (Automated PR Review — round 5)
This PR moves the desktop app from Electron 29 to Electron 43, brings the installer-building tool up with it, turns on the Chromium switches that let the app decode and negotiate H.265 video, and replaces the deprecated file-loading hook the newer Chromium no longer supports. It also changes how a recording starts: Cockpit asks the connection which video format it is receiving and, when that is H.265, names the recording format explicitly instead of letting the recorder copy the incoming frames — the copy path is the one the author reports as killing the whole interface. Around that it adds an error handler so a recording that breaks after it starts says so, a notice while a recording is being re-encoded, and a README line about H.265 cameras in the browser versus the desktop app. What still needs attention
Since round 4 — 2 closed, 1 new finding, comparing a0b5f15 → c26cc63Range. Maintainer decisions applied. Status changes this round
New this round
Discussion since round 4. One substantive comment from @rafaellehmkuhl ( Change map — what was established before judgingClaims
Failure site. Entry points
No changed function came back with no caller. The Invariants
1. Correctness & Implementation Bugs — 2 findings1.4 (carried from round 4) (minor) — The desktop remux was never told about HEVC, so H.265 recordings get MP4's Nothing changed here this round; The new recording format ends up in a pipeline that has only ever carried VP8/VP9/H.264.
If it holds, a desktop user records their H.265 camera and gets a file that Cockpit's own library and VLC play but that macOS refuses to open, with nothing on screen connecting the two. Graded The fix is not a one-liner, and the one-liner is a trap: adding 9.2 (carried from round 1, id kept from that round's numbering) (nit) — The Electron breaking-change audit is still unfinished. The checklist in the PR body is still entirely unticked: Storage, Joystick, Updates, Telemetry. The author's round-4 comment confirms it is deliberately deferred, which is the right disposition; the finding stays open until the ticks land. Two of those cannot be answered from the diff. The packager bump this round adds one item worth a look while the checklist is being worked: Updates. 10. Documentation — 2 findings10.1 (carried from round 4) (minor) — The Desktop cell of the new H.265 row states two hardware-conditional capabilities as unconditional.
AGENTS.md:118 asks the README table to carry the limitations of a feature that is not fully supported in both builds. One cell rewrite covers it, e.g. "Plays where the system can decode HEVC in hardware; recordings are re-encoded (to H.265, or H.264 when the machine cannot encode it) so they can be saved". 10.2 (minor) — The Browser cell explains the failure with a cause the PR's own evidence contradicts.
The verdict is right and the reason is not, and the reason is the part a user acts on.
For a Lite user in that configuration, the video pane simply stays blank: Chrome without The smaller of the two fixes is to drop the causal clause and keep the outcome — "❌ Not available: browsers cannot decode H.265" is true on both delivery paths. If the RTSP detail is worth keeping, it belongs as one of two cases rather than as the cause, e.g. "❌ Not available: browsers cannot decode H.265, and H.265 cameras added as RTSP/UDP sources cannot be opened at all". This does not reopen 3.1, which asked for the Lite limitation to be stated and is correctly closed; what is wrong is the explanatory clause that arrived with that fix. Sections with nothing to report (9)2. Persistence & User Data — ✅ (the PR adds, reshapes and removes no persisted key, and the two dependency lines it changes are 3. AGENTS.md Adherence — ✅ (this round's only source change is one argument in 4. Security — ✅ (the 5. Performance — ✅ ( 6. UI / UX — ✅ (re-checked unchanged this round: the notice at 7. Code Quality & Style — ✅ (the complexity report's single trigger is answered, unchanged from round 4: it puts 8. Commit Hygiene — ✅ (all six subjects and bodies re-read off 9. Tests — ✅ (the spec at 11. Nitpicks / Optional — ✅ (the section's only entry, 11.1, closed this round against Generated by Claude. This is advisory; a human reviewer must still approve. |
4 participants
Updates Chromium to a version that supports H.265 in both video tags and WebRTC, then opts in via PlatformHEVCDecoderSupport, WebRtcAllowH265Receive and WebRtcAllowH265Send so RTSP streams bridged through go2rtc can be decoded by the renderer.
The main part of the review is checking if any electron-related feature broke:
Evidence:
Fix #1725
Fix #2691