chore(release): sync the v1.9.0 RC fixes into main - #275
Merged
Conversation
electron-builder signed the bundle itself until 26.15.3. Its macPackager
carried a `noIdentity && fallBackToAdhoc` branch handing back
`Identity("-")` when no certificate was found — mandatory on arm64, where
an unsigned binary will not launch. 26.15.3 replaced that path with
`findSigningIdentity`, which returns null instead, so `sign()` leaves on
`return false` and nothing signs the bundle. What ships is the bare
linker signature on the Electron binary: `Identifier=Electron`,
`Sealed Resources=none`.
macOS keys TCC grants to an app's code signature, so such a bundle can
never hold one. v1.9.0-rc.1 asked for Accessibility, the user granted it,
`AXIsProcessTrusted()` still returned false, and the editable-cursor
preflight re-opened the same dialog on every press of record. Recording
was impossible on macOS.
Sign ad-hoc ourselves with the runtime and entitlements electron-builder
would have applied, on both arches — 26.8.1 only fell back on arm64, so
Intel DMGs were never signed at all.
The verification step that should have caught this was gated on signing
being enabled, i.e. it never ran for the only builds that could be
unsigned. Make it unconditional, and assert the signing identifier
against the bundle id: `codesign --verify` passes on the bare linker
signature too, so the identifier is the only thing that separates a
bundle macOS can attach permissions to from one it cannot.
On the macOS and Linux native capture paths the webcam handle was built with no file name, which selects in-memory buffering. Nothing reached disk during capture, and finalize had to flatten the whole clip into one ArrayBuffer to hand it across IPC. Past ~2GB — a take of roughly 20 minutes at BITRATE_BASE — that allocation throws. The throw was swallowed twice over: fixWebmDuration catches its own FileReader failure and returns the unpatched blob, then the finalize catch logged to console and returned undefined, which made the attach guard skip attachNative*WebcamRecording entirely. The session was written screen-only and the editor opened as if nothing had happened, with the camera simply absent. A 23-minute take lost its webcam this way; shorter takes in the same app session saved fine. Pass the webcam file name on both native paths so chunks stream to disk as they arrive, the way the legacy path and the Windows helper already do. Finalize now branches on isStreaming(): a streamed clip hands over its name alone and the main process closes the stream and patches the WebM duration on disk, so nothing multi-gigabyte is ever flattened or sent across IPC. Buffered short takes keep the existing behaviour. Every failure now comes back with a reason and reaches the user as a toast. Silently discarding a completed take is the worst available outcome, and it was the one that shipped. Because the bytes now land on disk during capture, a webcam stream that isn't folded into a saved session is closed and its partial file removed — otherwise a discarded or failed take orphans a half-written .webm. The macOS and Linux finalizers were line-for-line copies, so the shared logic moves into finalizeWebcamAsset() rather than being duplicated again. Its tests pin both halves of the fix: that a streamed clip is never read into memory, and that a failure is never silent. Fixes #253
reindexRecordingOnDisk is Linux-gated by intent — Windows and macOS record through native helpers that write indexed files at the source, so the wrapper returns `unsupported-platform` before touching anything else. The suite never accounted for that: six of its cases inject a fake remux service and assert the remuxed result, so on a macOS or Windows checkout they stop at the guard and fail. They were red on a clean tree, for environmental reasons, with nothing to distinguish them from a real regression. Pin process.platform to linux in beforeEach and restore the real value in afterEach, so the cases exercise the wrapper's own logic on every platform. The technique is the one the suite's last case already used inline; hoisting it removes the per-test save/restore boilerplate and makes the restore hold even when a case throws part-way. The guard itself stays covered, now over both platforms it exists for rather than win32 alone, so pinning to Linux can't quietly become the only thing the suite exercises. No production code changes. Verified the cases still have teeth: removing the empty-output size check from reindexRecordingOnDisk fails the truncated-file case, which had been passing vacuously on macOS. `npx vitest --run` is now green on macOS: 137 files, 1626 passing.
The four notarization steps carried `&& !contains(github.ref_name, '-')`, which skipped them for every pre-release. Two costs, and the second is the one that mattered. Testers paid the first. A DMG signed with Developer ID but not notarized is still refused by Gatekeeper — `spctl` answers `rejected, source= Unnotarized Developer ID` — so anyone testing an RC had to know about `xattr -rd com.apple.quarantine` before they could open the build they were being asked to try. The release paid the second. Notarization never ran until the stable tag, so the first exercise of the credentials, the certificate chain and Apple's acceptance of every nested Mach-O landed on the highest-stakes build there is. The run that first enabled signing died in `Package .app bundle` on a malformed `MAC_CSC_NAME`; it was caught only because a full build was dispatched deliberately. Notarizing each RC makes every candidate a rehearsal. The trade is a few minutes per macOS job and a dependency on Apple's notary service being reachable, with `--wait` capped at 15 minutes. If that turns flaky enough to block RCs, the answer is `continue-on-error` on pre-releases rather than skipping them again. Five documentation sites asserted the old behaviour and are corrected here, so nothing claims RCs are unnotarized after this lands.
The appx manifest advertised en-US and fr-FR only, so the Microsoft Store product page listed two supported languages for an app that ships thirteen (SUPPORTED_LOCALES in src/i18n/config.ts), and the listing never surfaced in Store searches run in the other eleven. Bare tags for the region-less locales so every region of that language matches, which is what the renderer's own locale resolution does.
`CSC_NAME` must name the identity without its certificate type;
electron-builder chooses the type itself and refuses a qualified name:
⨯ Please remove prefix "Developer ID Application:" from the specified
name — appropriate certificate will be chosen automatically
It refuses at `Package .app bundle`, which runs after the ffmpeg build
and the compositor addon — about twelve minutes into the macOS job, and
nowhere else. That is what happened the first time signing was enabled
here: twelve minutes to learn that a secret had four extra words.
The mistake is easy to make because the same secret also feeds
`codesign --sign` at `Sign DMG`, and codesign accepts the full common
name, so the qualified form looks correct right up until
electron-builder sees it. The short form satisfies both, since codesign
matches on a substring of the common name.
Check it in `Resolve macOS signing`, where every other signing input is
already validated, and fail in seconds with the value to use instead.
Only prefixes ending in a colon match, so a company whose name starts
with one of these words is not caught.
The helper's stop wait was gated on the frame mutex:
std::unique_lock lock(mutex);
control.cv.wait(lock, [&]{ return control.stopRequested.load(); });
`stopRequested` is an atomic with no relationship to what that mutex
protects, but `condition_variable::wait` has to re-acquire it before it
can return -- and that mutex is held across uninterruptible D3D11 work:
the WGC frame callback's CopyResource, and the video writer's
Map(D3D11_MAP_READ) readback. One stalled driver call and the main thread
never came back, before the first [stop-timing] line was ever printed.
That is why issue #252 arrived with an empty diagnostic log.
Give stop its own mutex/CV pair that no frame thread ever touches, route
all nine stop sites through requestStop(), and bound the wait. Then bound
the shutdown itself: every step after the wait calls into a driver or
joins a thread that does, so decoupling the wait alone would only have
moved the hang. Each step gets a deadline under a global ceiling, and a
watchdog force-exits the process naming the step it died in.
Report success as soon as the MP4 index is written rather than at the end
of the process's life, so a wedged GPU teardown no longer costs a
recording that is already complete on disk. Check what finalize()
returns while doing it.
On the app side: keep a listener on the helper for the whole recording so
its diagnostics reach the bug report instead of being dropped between
start and stop, short-circuit a helper that already exited rather than
burning the timeout on a 'close' that can never arrive, and let discard
escape a wedged helper immediately.
The follow-on "Native Windows capture is not running." was separate: the
main process releases its helper handle unconditionally, the renderer did
not, so the next Record click sent a second stop. The same gap existed on
macOS and Linux.
The underlying stall is untouched -- the readback still runs inside the
frame lock, and the D3D adapter is still whichever one Windows hands us
on a four-adapter machine. What changes is that neither can hang the app.
Refs #252
…ture Review follow-up. The single `Recording stopped. Output path:` line was gated on `screenFinalized && webcamFinalized`, and the app treats that line as the only proof a recording is worth keeping. So an optional second file could veto a complete one: `webcamEncoder.finalize()` returning false discarded a perfectly indexed screen MP4, which was a regression against the previous behaviour where its result was ignored entirely. The same gate had a second way to fire. Both finalize steps clamp their deadline to the shared global ceiling, so a screen finalize that spent most of it left the webcam step already past its deadline, and the shutdown watchdog killed the process before the announcement ran. Announce after the screen finalize and before the webcam's, gated on the screen file alone. A failed webcam finalize is now an ERROR on stderr and a non-zero exit, which is what it always should have been -- not a lost recording. Also from the review: - Log the `quiesceCapture()` drain outcome at `wgc-quiesce`. It decides whether `wgc-session-close` releases the device or skips it, so a report that omits it cannot be read. - Keep `phase=` when parsing `[stop-timing]`. The diagnostic tool discarded it, which is the one field naming the step that hung, and the summary listed every step twice because begin and end lines both matched. Same double-count in the helper harness. - Pin `OPENSCREEN_WGC_STOP_BUDGET_MS` into the harness's child env and derive its hang limit from it. The limit was 30s against a 50s ceiling, so a long software-encoder finalize -- the case issue #34 exists for -- would have been killed and reported as the #252 hang. - Correct the shutdown budgets in the architecture doc: 8s per step, 50s overall, not 10s. - Clear `pendingCursorRecordingData` on the failed-stop path, matching the discard path.
The job filtered on `main`, so retargeting this PR at `release/v1.9.0` removed the artifact its own testing steps tell a reviewer to download. A recording fix aimed at a release is precisely when someone needs the compiled helper without a local MSVC toolchain.
…s clips The layout preset is global — one panel for the whole timeline — but the camera is per clip: a project mixes a screen+webcam recording with a plain import without one. `LiveParams::has_webcam` already carried that distinction, but only `live.rs` derived it, so the preview was right and every export was wrong. An export sets its `LiveParams` once for the whole timeline (`compositor-view-napi`), keeping the `true` default. And `ExportDialog` sends the SCREEN path as `webcamPath` when a clip has no camera, purely so the decoder has something valid to open — so the PiP box was drawn with the screen recording behind it, duplicated into its own corner. That is the mirror reported in #248, which the greyed-out Layout panel (correctly gated on `hasAnyClipWithCamera`) then left no way to turn off. `webcam_is_real` moves next to the field it decides, and `walk_composited_timeline` — shared by MP4 and GIF on all three backends — rebinds it per clip. A targeted `set_has_webcam` rather than a per-clip `set_live_params`, which would clobber the settings the caller posted. Verified on a real export (`run_composited_multi`, h264_amf) with the camera path equal to the screen path: the thumbnail is gone. The bench gains a `--webcam` override because the no-camera case is not different *content* but an identical *path*, and cannot be replayed otherwise. Refs #248
The preset is global — one panel for the whole timeline — but the camera is per clip. `layoutByClip` already carried a resolved layout per visible clip; it just never asked whether that clip had a camera, so every clip got the preset whether or not it had anything to put in it. Gating the camera's draw (`has_webcam`, previous commit) is not enough, and this is the half that actually shows. The block presets — `dual-frame`, `vertical-stack` — size the SCREEN off the block: they reserve the camera's half of the frame. A camera-less clip therefore kept its screen squeezed into that half with nothing beside it, which no draw-time gate can undo. Under picture-in-picture the defect is invisible, since the screen stays full-frame there and only a thumbnail is added. So a clip with no camera now lays out as if the preset were "no-webcam" — which `computeCompositeLayout` already implements (full-frame screen, no webcam rect). The predicate matches the `webcamPath` sent with the clip exactly, so the layout and the decoder cannot disagree. It is deliberately NOT `hasAnyClipWithCamera`, which gates the Layout panel and ignores `visible` on purpose so the panel stays reachable to un-hide a camera. `PreviewCanvas` had the same hole, and its own comment named it: it hid the webcam SLOT for a camera-less clip but left the screen geometry alone. Five regression tests, all of which fail on the parent commit: one per preset for "only the clip that has a camera gets a webcam rect", and one per block preset for "the camera-less clip gets its full frame back". The existing webcamRect test asserted a PiP rect for an asset with no camera — that premise was the bug, so its fixture gains the camera its preset presupposes and it goes on testing the px→fraction conversion it describes. Refs #248
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (33)
📝 WalkthroughWalkthroughThe PR updates Windows capture shutdown and diagnostics, adds streamed native webcam finalization, resolves webcam layouts per clip, expands macOS prerelease signing, broadens diagnostic workflow branches, and adds AppX locales. ChangesWindows capture shutdown
Webcam layout and recording
Release and packaging workflows
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Recorder
participant ElectronHandlers
participant WindowsHelper
participant WgcSession
participant OutputFiles
Recorder->>ElectronHandlers: request native capture stop
ElectronHandlers->>WindowsHelper: send stop and drain output
WindowsHelper->>WgcSession: quiesce callbacks and tear down capture
WgcSession->>OutputFiles: finalize screen and webcam files
WindowsHelper-->>ElectronHandlers: return timing, paths, or failure diagnostics
ElectronHandlers-->>Recorder: complete recording or reset state
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
16 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Brings
mainlevel withrelease/v1.9.0. Eleven fixes landed on the release branch during the RC window and exist nowhere else —mainhas received nothing since the RC was cut except the Notes stylesheet fix (#270). Without this, every one of them regresses the day 1.10.0 is cut frommain.Cherry-picked in order, conflict-free. The two
chore(release): bump to 1.9.0-rc.Ncommits are deliberately excluded — the version bump belongs to the release branch and comes back through the promote sync.fix(recording)fix(export)/fix(layout)fix(build)/fix(ci)/ci(...)MAC_CSC_NAMEvalidation, diagnostic bundle on release branchestest(recording)After merge,
mainandrelease/v1.9.0differ only bypackage.json(the RC version) and the Notes fix, which goes the other way in a companion PR.Related issue
Refs #270
Type of change
Release impact
Desktop impact
Testing
Biome clean (13 pre-existing warnings, unchanged),
tsc --noEmitclean. Unit and browser suites left to CI — every commit here was already green onrelease/v1.9.0, which CI covers via therelease/**trigger.git diff release/v1.9.0..HEADis limited topackage.jsonand the three Notes files, i.e. the two intended divergences and nothing else.Summary by CodeRabbit
New Features
Bug Fixes
Release Improvements