feat: add H.264 stream-copy export, overhaul exporter pipeline, rename native binaries#247
Merged
Conversation
…e native binaries - IPC: new inputMode 'h264-stream' — browser VideoEncoder output stream-copied into MP4 via ffmpeg rather than piping raw RGBA frames, cuts memory usage - Security: replace flat approvedLocalReadPaths with userApprovedPaths fed from dialog pickers and project load; isAllowedLocalReadPath uses it alongside dir prefix / existsSync checks - nativeVideoExport: add buildNativeH264StreamExportArgs for stream-copy path - frameRenderer / modernFrameRenderer: cursor-follow camera integration, annotation blur post-processing, shadow profile, squircle geometry - audioEncoder / streamingDecoder / gifExporter / muxer: full pipeline hardening — drift correction, backpressure, proper teardown - Rename native helpers openscreen-* → recordly-* on darwin-arm64 and darwin-x64; update helpers-manifest.json for win32-x64 - electron/main, windows, updater, rendererServer: startup and window lifecycle improvements - preload: expose inputMode and new IPC surface to renderer
Contributor
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Formatting & whitespace electron/appPaths.ts, electron/rendererServer.ts, electron/uiohook-napi.d.ts, electron/native/bin/.../*.json, src/lib/exporter/*.test.ts, many src/lib/exporter/* files |
Quote style, semicolons, indentation, and trailing-newline normalizations applied broadly; no semantic changes. |
Electron typings & preload electron/electron-env.d.ts, electron/preload.ts |
Tightened renderer IPC typings (replace any with ProcessedDesktopSource), added `getAssetBasePath(): Promise<string |
Extensions: types & IPC electron/extensions/extensionTypes.ts, electron/extensions/extensionIpc.ts, electron/extensions/extensionMarketplace.ts, electron/extensions/errorUtils.ts |
Re-export extension types from canonical src/lib/extensions/types; added getErrorMessage(error: unknown) and use it to normalize error payloads in IPC handlers; streaming-body typing improvements. |
Native H.264 export & helpers electron/ipc/nativeVideoExport.ts, src/lib/exporter/modernVideoExporter.ts, src/lib/exporter/videoExporter.ts |
Added buildNativeH264StreamExportArgs() and a new native export flow where browser-side VideoEncoder produces Annex B H.264 chunks sent via IPC to native muxer; introduced encoder lifecycle, write-in-flight accounting, encode-queue/backpressure gates, and related cleanup. |
Frame renderer & native readback removal src/lib/exporter/modernFrameRenderer.ts, src/lib/exporter/modernFrameRenderer.test.ts |
Removed native WebGL pixel-readback API (nativeReadbackMode, capturePixelsForNativeExport) and refactored renderer to use the VideoEncoder-based native export path. |
Forward frame source & safety src/lib/exporter/forwardFrameSource.ts |
Added Windows absolute-path detection/normalization for file URLs, made concurrent getFrameAtTime() throw, and ensured cancel() resolves pending frame promises and cancels demuxer reader. |
Streaming decoder, muxer, MP4 support src/lib/exporter/streamingDecoder.ts, src/lib/exporter/muxer.ts, src/lib/exporter/videoDecoder.ts, src/lib/exporter/mp4Support.ts |
Reordered/refactored imports and teardown logic (demuxer/video decoder cleanup before reload); minor probe cache-key update including bitrate. |
Audio, captions, annotations src/lib/exporter/audioEncoder.ts, src/lib/exporter/captionRenderer.ts, src/lib/exporter/annotationRenderer.ts |
Null-safe blur buffer handling and import reorganization; minor API-safe guards; exported function signatures unchanged. |
Electron runtime & small logic electron/main.ts, electron/windows.ts, electron/updater.ts, electron/cursorHider.ts, electron/ipc/windowsCaptureSelection.ts, electron/preload.ts |
Import reorderings, conditional formatting, cursor show/hide stylistic refactors, handler formatting, and preload IPC type tightening; no behavioral regressions expected. |
Sequence Diagram(s)
sequenceDiagram
participant Renderer as Browser Renderer
participant Encoder as Browser VideoEncoder (H.264 Annex B)
participant Main as Electron Main (IPC)
participant Native as Native Muxer Process
Renderer->>Encoder: create VideoFrame(canvas) & encode()
Encoder-->>Renderer: output Annex B chunk (output callback)
Renderer->>Main: nativeVideoExportWriteFrame(chunk)
activate Main
Main->>Native: write chunk to muxer (stdin/append)
deactivate Main
Renderer->>Encoder: flush() / encoder.flush()
Encoder-->>Renderer: final chunks
Renderer->>Main: nativeVideoExportFinish()
Main->>Native: finalize MP4 & close
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- webadderall/Recordly#210 — overlaps annotation blur rendering and blur-buffer usage in exporter/annotationRenderer.
- webadderall/Recordly#38 — related to cursor motion settings and FrameRenderer/exporter cursor plumbing.
Suggested labels
Checked
"🐰 I hopped through code with care,
Quotes aligned and types made fair.
H.264 now streams from canvas bright,
IPC carries chunks through day and night.
A tidy hop — the export's light!"
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 14.86% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the three main changes: H.264 stream-copy export, exporter pipeline overhaul, and native binary rename. |
| Description check | ✅ Passed | The PR description provides a clear summary, lists major changes with technical details, and mentions specific fixes, but the provided description is informal and lacks full adherence to the template structure. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/export-pipeline
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 @coderabbitai help to get the list of available commands and usage tips.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (8)
src/lib/exporter/frameRenderer.test.ts (1)
135-137: Consider adding clarifying comments to intentional no-op methods.The empty
load()andpause()methods are intentional stubs for the mock video element. Adding brief comments would clarify this intent and silence static analysis warnings.📝 Optional documentation improvement
- load() {} + load() { + // Intentional no-op for mock + } - pause() {} + pause() { + // Intentional no-op for mock + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/frameRenderer.test.ts` around lines 135 - 137, The empty load() and pause() methods on the mock video element are intentional no-ops; add a brief inline comment above or inside each method (e.g. "intentionally no-op for mock VideoElement used in tests") to clarify intent and silence static analysis; reference the mock's load() and pause() methods so reviewers and linters understand they are deliberate stubs.src/lib/exporter/forwardFrameSource.ts (1)
251-267: Guard against concurrentgetFrameAtTime()callers.
getNextFrame()stores only one resolver (this.frameResolve). Concurrent calls can overwrite it and leave one caller hanging. If single-consumer is intended, fail fast with a reentrancy guard; otherwise queue resolvers.Also applies to: 269-335
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/forwardFrameSource.ts` around lines 251 - 267, getNextFrame() currently stores a single resolver in this.frameResolve which allows concurrent callers (e.g., multiple getFrameAtTime() callers) to overwrite the resolver and leave promises unresolved; fix by either adding a reentrancy guard that immediately throws if this.frameResolve is already set (fail-fast single-consumer behavior) or by replacing the single resolver with a queue of resolvers (e.g., an array this.frameResolves) so each concurrent call pushes its resolve and is fulfilled in order when frames become available; update usages of this.frameResolve, and ensure interactions with this.pendingFrames, this.decodeDone, and this.decodeError remain correct and that queued resolvers are drained when frames are emitted or when decode error/done occur.src/lib/exporter/audioEncoder.ts (1)
777-806: Cleanup block has duplicate operations.
timelineAudioSourceNode?.disconnect(),timelineMedia.src="",timelineMedia.load(), andtimelineMediaSource.revoke()are executed twice. Consolidating this into a single cleanup pass will reduce maintenance risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/audioEncoder.ts` around lines 777 - 806, The cleanup block duplicates calls to timelineAudioSourceNode?.disconnect(), timelineMedia.src = "", timelineMedia.load(), and timelineMediaSource.revoke(); consolidate them into one final cleanup pass: remove the earlier duplicate calls and keep a single section (after handling sourceAudioElements and audioRegionElements and stopping the recorder) that safely disconnects timelineAudioSourceNode, clears and reloads timelineMedia, revokes timelineMediaSource, stops destinationNode tracks, disconnects destinationNode, and closes audioContext; ensure you still iterate sourceAudioElements and audioRegionElements to pause, disconnect, clear src/load, call cleanup(), and check recorder.state !== "inactive" before calling recorder.stop() so behavior remains unchanged.src/lib/exporter/gifExporter.test.ts (1)
23-49: Loop property tests are tautological and don’t exercise production logic.These assertions validate
loopEnabled ? 0 : 1against itself instead of a function/class output, so they won’t catch integration regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/gifExporter.test.ts` around lines 23 - 49, The tests are tautological because they compute repeat from loopEnabled instead of calling the real code; update them to instantiate or call the production code (GifExporter) with loop true/false and assert the resulting repeat value coming from the class (e.g., the constructor behavior or a public method like GifExporter.prototype.getOptions / export method) equals 0 for loop=true and 1 for loop=false; similarly replace the second test to call the exporter and assert the produced options/metadata or instance field contains only 0 or 1 for repeat. Locate references to GifExporter, its constructor, and any method that returns export options to make the assertion against real output rather than the inline ternary.electron/extensions/extensionIpc.ts (1)
28-30: ExtractgetErrorMessageinto a shared extensions utility.This helper now exists in multiple extension modules; centralizing it will prevent drift in error-shaping behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/extensions/extensionIpc.ts` around lines 28 - 30, The getErrorMessage helper in extensionIpc.ts is duplicated across extension modules—extract it into a single shared extensions utility by creating an exported function getErrorMessage in the new shared utility module and replace local implementations to import and use that exported function (keep the signature getErrorMessage(error: unknown): string and its behavior). Update all extension modules that currently define their own getErrorMessage to remove the duplicate and import from the shared utility (ensure build and tests still reference the same symbol name). Run a quick search for other occurrences of getErrorMessage to consolidate them and update imports accordingly.src/lib/exporter/modernVideoExporter.ts (3)
1461-1464: Duplicate empty catch block; consider extracting helper.Same issue as in
cancel(). Bothcancel()andcleanup()have identical encoder disposal logic. Consider extracting to a private helper method.♻️ Suggested refactor
+private disposeNativeH264Encoder(): void { + if (!this.nativeH264Encoder) return; + try { + this.nativeH264Encoder.close(); + } catch (e) { + console.debug("[VideoExporter] Ignoring error closing native H.264 encoder:", e); + } + this.nativeH264Encoder = null; +} cancel(): void { // ... - if (this.nativeH264Encoder) { - try { this.nativeH264Encoder.close(); } catch (_) {} - this.nativeH264Encoder = null; - } + this.disposeNativeH264Encoder(); // ... } private cleanup(): void { // ... - if (this.nativeH264Encoder) { - try { this.nativeH264Encoder.close(); } catch (_) {} - this.nativeH264Encoder = null; - } + this.disposeNativeH264Encoder(); // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/modernVideoExporter.ts` around lines 1461 - 1464, Both cancel() and cleanup() duplicate the same empty-catch disposal logic for this.nativeH264Encoder; extract that repeated block into a private helper (e.g., disposeNativeH264Encoder) that checks this.nativeH264Encoder, calls close() inside a try/catch, and sets this.nativeH264Encoder = null, then call that helper from both cancel() and cleanup() to remove duplication and centralize disposal behavior.
863-867: Hardcoded backpressure limit could be configurable.The
encodeQueueSize >= 32threshold is hardcoded. Consider extracting this to a constant or using the existing backpressure profile for consistency with the WebCodecs path.+private static readonly NATIVE_ENCODER_QUEUE_LIMIT = 32; + private async encodeRenderedFrameNative(...): Promise<void> { // ... - while (this.nativeH264Encoder.encodeQueueSize >= 32) { + while (this.nativeH264Encoder.encodeQueueSize >= ModernVideoExporter.NATIVE_ENCODER_QUEUE_LIMIT) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/modernVideoExporter.ts` around lines 863 - 867, The hardcoded backpressure threshold in the wait loop (this.nativeH264Encoder.encodeQueueSize >= 32) should be made configurable: replace the literal 32 with a named constant or, better, reuse the existing backpressure/profile value used by the WebCodecs path (e.g., a shared BACKPRESSURE_LIMIT or backpressureProfile.limit) so both paths stay consistent; update references in modernVideoExporter.ts (the loop around this.nativeH264Encoder.encodeQueueSize and any related checks) to read from that constant/profile and add a sensible default and documentation where the constant/profile is declared.
828-830: Consider using a Set for more efficient promise tracking.The current approach creates a new filtered array on every promise resolution, which at high frame rates (e.g., 60fps) could cause memory churn. A
Setwould provide O(1) add/delete operations.♻️ Optional refactor
-private nativeWritePromises: Promise<void>[] = []; +private nativeWritePromises: Set<Promise<void>> = new Set(); // In output callback: -this.nativeWritePromises.push(writePromise); -void writePromise.finally(() => { - this.nativeWritePromises = this.nativeWritePromises.filter((p) => p !== writePromise); -}); +this.nativeWritePromises.add(writePromise); +void writePromise.finally(() => { + this.nativeWritePromises.delete(writePromise); +}); // In awaitPendingNativeWrites: -while (this.nativeWritePromises.length > 0) { +while (this.nativeWritePromises.size > 0) { + const oldestWritePromise = this.nativeWritePromises.values().next().value;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/modernVideoExporter.ts` around lines 828 - 830, The current removal pattern for this.nativeWritePromises (filtering the array in the writePromise.finally callback) causes allocation churn; change nativeWritePromises from an Array to a Set to get O(1) add/delete: initialize nativeWritePromises as a Set, replace places that push with this.nativeWritePromises.add(writePromise), replace the finally callback to this.nativeWritePromises.delete(writePromise), and update any code that awaits or inspects nativeWritePromises (e.g., Promise.all or iteration) to use Array.from(this.nativeWritePromises) or this.nativeWritePromises.size as appropriate so behavior remains the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@electron/cursorHider.ts`:
- Around line 128-142: The finally block in showCursor unconditionally sets
cursorHidden = false even when showing the cursor fails, preventing retries;
change the logic so cursorHidden is only set to false when the show actually
succeeds: call runPythonSnippet/runPowerShellSnippet (using
getPowerShellCommand) inside the try, capture the boolean result (didShow), and
only set cursorHidden = false when didShow is truthy before returning; leave
cursorHidden unchanged on exceptions or false results so subsequent calls can
retry. Ensure showCursor still returns the boolean outcome and that exceptions
are caught and logged as before.
In `@electron/extensions/extensionMarketplace.ts`:
- Around line 289-292: The fetch call that posts to
`${MARKETPLACE_API_BASE}/extensions/${encodeURIComponent(extensionId)}/download`
bypasses the configurable marketplace URL; change it to use the
getMarketplaceUrl() helper (call it to obtain the base URL, e.g. const base =
getMarketplaceUrl()) and build the download URL from that base instead of
MARKETPLACE_API_BASE so the RECORDLY_MARKETPLACE_URL override is respected; keep
the existing method, headers (X-Recordly-Version: app.getVersion()) and the
catch(() => undefined) behavior and still encodeURIComponent(extensionId) when
constructing the path.
In `@electron/ipc/nativeVideoExport.ts`:
- Around line 7-13: NativeVideoExportStartOptions is missing the new inputMode
property introduced by the PR; update the interface in nativeVideoExport.ts to
include an inputMode (e.g. the literal union "rawvideo" | "h264-stream" or a
named type like NativeExportInputMode) so the main-process IPC contract matches
the renderer/preload. If behavior differs by mode, make the type discriminated
(add inputMode to NativeVideoExportStartOptions and adjust fields or split into
a discriminated union such as RawVideoStartOptions vs H264StreamStartOptions)
and reference the existing encodingMode, width, height, frameRate and bitrate
symbols so callers and handlers can branch on inputMode correctly.
In `@electron/ipc/windowsCaptureSelection.ts`:
- Around line 29-39: The code currently uses the original requestedDisplayId as
displayId even when it falls back to primaryDisplay for bounds, producing an
inconsistent pair; change the logic so the returned displayId is the resolved
one from the matched display (e.g., use matchedDisplay.id or assign a
resolvedDisplayId after computing matchedDisplay) so both displayId and bounds
come from the same display object (references: requestedDisplayId, displayId,
matchedDisplay, primaryDisplay).
In `@src/lib/exporter/annotationRenderer.ts`:
- Around line 15-19: The SSR branch in get/create of blurBufferCanvas currently
returns {} as HTMLCanvasElement which will crash when code (around the blur
rendering using canvas APIs at lines ~394-396) calls canvas methods; update the
SSR/non-DOM fallback to either create an OffscreenCanvas when available (use
global OffscreenCanvas) or return a minimal stub object that implements the
canvas surface shape used by the renderer (e.g., properties width/height and a
getContext method that returns null or a no-op context) instead of a raw {}
cast; modify the blurBufferCanvas assignment/return (the blurBufferCanvas symbol
in annotationRenderer.ts) to produce this safe fallback so subsequent calls to
getContext/drawImage/width checks do not throw.
In `@src/lib/exporter/audioEncoder.ts`:
- Around line 507-613: The setup creates timelineMedia, audioContext,
destinationNode, timelineAudioSourceNode, sourceAudioElements and
audioRegionElements and then calls waitForLoadedMetadata and startAudioRecording
outside a protected scope, so if waitForLoadedMetadata, cancellation checks, or
startAudioRecording throw, allocated media resources and streams are leaked;
move the entire resource allocation and async loading logic (calls to
resolveMediaElementSource, creation of timelineMedia/audio elements,
audioContext, destinationNode, creation and population of sourceAudioElements
and audioRegionElements, and the call to startAudioRecording) inside a try block
and implement a finally that iterates over timelineMedia, sourceAudioElements,
audioRegionElements to revoke sources, disconnect and stop nodes, close
audioContext and stop any recorder/streams (use cleanup functions like
sourceFileSource.revoke and regionFileSource.revoke, disconnect
MediaElementAudioSourceNode and GainNode, and call audioContext.close()) so any
exception before the original try is handled and resources are always released.
In `@src/lib/exporter/forwardFrameSource.ts`:
- Around line 109-119: The resolveVideoResourceUrl function doesn't treat
Windows absolute drive-letter paths (e.g., "C:\\path\\to\\file" or
"C:/path/to/file") as file URLs, so update resolveVideoResourceUrl to detect
Windows drive-letter patterns (case-insensitive regex like /^[a-z]:[\\/]/) and
normalize them to a proper file URL (use "file:///" + encoded path with forward
slashes and ensure leading slash before the drive letter); keep existing
handling for blob:, data:, http(s):, file:, and leading "/" cases and return the
original videoUrl otherwise.
In `@src/lib/exporter/gifExporter.test.ts`:
- Around line 55-57: The test text and assertion tolerance in gifExporter.test
are inconsistent: the requirement comment says 0.01 but the assertion uses 0.02;
update the assertion(s) that compare exported GIF aspect ratio to the source
(the expectation that checks the absolute difference/tolerance) to use 0.01
instead of 0.02 in both places referenced (the aspect-ratio assertions around
the two blocks noted) so the comment and test match.
In `@src/lib/exporter/modernVideoExporter.ts`:
- Around line 1417-1420: The empty catch in the native H264 encoder cleanup
should not silently swallow errors; update the block around
this.nativeH264Encoder.close() in modernVideoExporter.ts to catch and either log
the caught error at debug/trace level via the existing logger (or processLogger)
or add a clear explanatory comment if the error is expected and safe to ignore;
ensure you still null out this.nativeH264Encoder after attempting close and
include the encoder identity/context in the log message so failures during
cleanup can be diagnosed.
- Around line 836-844: The encoder is being configured without first verifying
platform support; update tryStartNativeVideoExport to call and await
VideoEncoder.isConfigSupported() with the same config (codec: "avc1.640034",
width/height/bitrate/framerate, hardwareAcceleration: "prefer-hardware", avc:
{format: "annexb"}) before calling encoder.configure(), and if isConfigSupported
resolves with supported: false or throws, invoke the existing error path (the
error callback used around the encoder setup) or return false so the caller does
not proceed; only call encoder.configure() after the supported check succeeds.
In `@src/lib/exporter/mp4Support.ts`:
- Around line 170-173: The probe cache key (dimensionCacheKey) currently built
in mp4Support.ts uses codec, normalizedWidth, normalizedHeight, and frameRate
but omits bitrate, causing bitrate-sensitive capability results to be reused
incorrectly; update the key construction used by dimensionCacheKey (and the
other cache key around the 179-186 block that similarly builds/reads from
supportedDimensionCache) to include the effective bitrate (e.g., options.bitrate
or the resolved bitrate string/number), normalizing undefined values to a stable
token, and use that expanded key for both supportedDimensionCache.get and
supportedDimensionCache.set to ensure bitrate-specific probe results are cached
separately.
In `@src/lib/exporter/streamingDecoder.ts`:
- Around line 573-580: The loop that builds non-trim segments in
streamingDecoder.ts can move cursor backward when trim regions overlap; in the
for-loop over sorted trims (the block that computes trimStart/trimEnd and pushes
into segments) ensure you never set cursor to a smaller value by using cursor =
Math.max(cursor, trimEnd) (or equivalent) and only push a segment when trimStart
> cursor so overlapping trims are effectively merged and cursor never regresses.
- Around line 624-630: The loop over overlapping speed regions can move cursor
backwards for nested/overlapping intervals; change the logic in the loop that
iterates over overlapping/sr so you compute srStart and srEnd as before but then
use an effectiveStart = Math.max(cursor, srStart) and only push the speed region
if srEnd > effectiveStart (push { startSec: effectiveStart, endSec: srEnd,
speed: sr.speed }); keep the gap push when cursor < srStart, and finally set
cursor = Math.max(cursor, srEnd) (or cursor = srEnd after the conditional) to
ensure the cursor never moves backward; update references to overlapping, sr,
cursor, result, startSec/endSec/speed accordingly.
- Around line 158-166: The loadMetadata method creates a new WebDemuxer and
assigns it to this.demuxer without cleaning up any existing instance, which can
leak resources on repeated calls; before instantiating a new WebDemuxer in
loadMetadata, detect an existing this.demuxer and call its teardown method
(e.g., close/destroy/unload/terminate—use the actual API provided by WebDemuxer)
and await it, catching/logging errors, then replace this.demuxer with the new
instance created by new WebDemuxer(...); ensure the cleanup is performed
synchronously before loadVideoFile()/demuxer.load is called.
In `@src/lib/exporter/videoExporter.ts`:
- Around line 337-339: Update the native-export guard and startup flow so we
fully verify capabilities and fail safe: in shouldUseExperimentalNativeExport()
check for window.VideoEncoder (and window.VideoEncoder.isConfigSupported) as
well as all IPC methods used (e.g., nativeVideoExportStart,
nativeVideoExportFinish, nativeVideoExportData) instead of only
nativeVideoExportStart; in tryStartNativeVideoExport() call
VideoEncoder.isConfigSupported() with the assembled avc config before
instantiating, and wrap encoder.configure() in a try/catch that, on any error,
calls the native cancel/finish IPC to clean up, returns false, and falls back to
the non-native path. Ensure all failure paths release the native session and
return false so the code does not throw NotSupportedError.
- Around line 258-265: The flush/close sequence can race with in-flight async
writes from the encoder's async output callback (which calls
window.electronAPI.nativeVideoExportWriteFrame); modify the logic around
nativeH264Encoder.flush()/close() and finishNativeVideoExport(nativeAudioPlan,
...) to await all pending write Promises first: have the output callback push
each nativeVideoExportWriteFrame Promise into a list (e.g.,
this.pendingNativeWrites or similar on the exporter instance) and then after
await this.nativeH264Encoder.flush() call await
Promise.all(this.pendingNativeWrites) (and clear the list) before calling
this.nativeH264Encoder.close() and return await
this.finishNativeVideoExport(...); apply the same pattern where similar native
encoder shutdown happens (lines ~511–539).
---
Nitpick comments:
In `@electron/extensions/extensionIpc.ts`:
- Around line 28-30: The getErrorMessage helper in extensionIpc.ts is duplicated
across extension modules—extract it into a single shared extensions utility by
creating an exported function getErrorMessage in the new shared utility module
and replace local implementations to import and use that exported function (keep
the signature getErrorMessage(error: unknown): string and its behavior). Update
all extension modules that currently define their own getErrorMessage to remove
the duplicate and import from the shared utility (ensure build and tests still
reference the same symbol name). Run a quick search for other occurrences of
getErrorMessage to consolidate them and update imports accordingly.
In `@src/lib/exporter/audioEncoder.ts`:
- Around line 777-806: The cleanup block duplicates calls to
timelineAudioSourceNode?.disconnect(), timelineMedia.src = "",
timelineMedia.load(), and timelineMediaSource.revoke(); consolidate them into
one final cleanup pass: remove the earlier duplicate calls and keep a single
section (after handling sourceAudioElements and audioRegionElements and stopping
the recorder) that safely disconnects timelineAudioSourceNode, clears and
reloads timelineMedia, revokes timelineMediaSource, stops destinationNode
tracks, disconnects destinationNode, and closes audioContext; ensure you still
iterate sourceAudioElements and audioRegionElements to pause, disconnect, clear
src/load, call cleanup(), and check recorder.state !== "inactive" before calling
recorder.stop() so behavior remains unchanged.
In `@src/lib/exporter/forwardFrameSource.ts`:
- Around line 251-267: getNextFrame() currently stores a single resolver in
this.frameResolve which allows concurrent callers (e.g., multiple
getFrameAtTime() callers) to overwrite the resolver and leave promises
unresolved; fix by either adding a reentrancy guard that immediately throws if
this.frameResolve is already set (fail-fast single-consumer behavior) or by
replacing the single resolver with a queue of resolvers (e.g., an array
this.frameResolves) so each concurrent call pushes its resolve and is fulfilled
in order when frames become available; update usages of this.frameResolve, and
ensure interactions with this.pendingFrames, this.decodeDone, and
this.decodeError remain correct and that queued resolvers are drained when
frames are emitted or when decode error/done occur.
In `@src/lib/exporter/frameRenderer.test.ts`:
- Around line 135-137: The empty load() and pause() methods on the mock video
element are intentional no-ops; add a brief inline comment above or inside each
method (e.g. "intentionally no-op for mock VideoElement used in tests") to
clarify intent and silence static analysis; reference the mock's load() and
pause() methods so reviewers and linters understand they are deliberate stubs.
In `@src/lib/exporter/gifExporter.test.ts`:
- Around line 23-49: The tests are tautological because they compute repeat from
loopEnabled instead of calling the real code; update them to instantiate or call
the production code (GifExporter) with loop true/false and assert the resulting
repeat value coming from the class (e.g., the constructor behavior or a public
method like GifExporter.prototype.getOptions / export method) equals 0 for
loop=true and 1 for loop=false; similarly replace the second test to call the
exporter and assert the produced options/metadata or instance field contains
only 0 or 1 for repeat. Locate references to GifExporter, its constructor, and
any method that returns export options to make the assertion against real output
rather than the inline ternary.
In `@src/lib/exporter/modernVideoExporter.ts`:
- Around line 1461-1464: Both cancel() and cleanup() duplicate the same
empty-catch disposal logic for this.nativeH264Encoder; extract that repeated
block into a private helper (e.g., disposeNativeH264Encoder) that checks
this.nativeH264Encoder, calls close() inside a try/catch, and sets
this.nativeH264Encoder = null, then call that helper from both cancel() and
cleanup() to remove duplication and centralize disposal behavior.
- Around line 863-867: The hardcoded backpressure threshold in the wait loop
(this.nativeH264Encoder.encodeQueueSize >= 32) should be made configurable:
replace the literal 32 with a named constant or, better, reuse the existing
backpressure/profile value used by the WebCodecs path (e.g., a shared
BACKPRESSURE_LIMIT or backpressureProfile.limit) so both paths stay consistent;
update references in modernVideoExporter.ts (the loop around
this.nativeH264Encoder.encodeQueueSize and any related checks) to read from that
constant/profile and add a sensible default and documentation where the
constant/profile is declared.
- Around line 828-830: The current removal pattern for this.nativeWritePromises
(filtering the array in the writePromise.finally callback) causes allocation
churn; change nativeWritePromises from an Array to a Set to get O(1) add/delete:
initialize nativeWritePromises as a Set, replace places that push with
this.nativeWritePromises.add(writePromise), replace the finally callback to
this.nativeWritePromises.delete(writePromise), and update any code that awaits
or inspects nativeWritePromises (e.g., Promise.all or iteration) to use
Array.from(this.nativeWritePromises) or this.nativeWritePromises.size as
appropriate so behavior remains the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d513af3a-d4b1-48ab-85ab-3f893ab477b8
📒 Files selected for processing (60)
electron/appPaths.tselectron/cursorHider.tselectron/electron-env.d.tselectron/extensions/extensionIpc.tselectron/extensions/extensionMarketplace.tselectron/extensions/extensionTypes.tselectron/ipc/handlers.tselectron/ipc/nativeVideoExport.tselectron/ipc/windowsCaptureSelection.test.tselectron/ipc/windowsCaptureSelection.tselectron/main.tselectron/native/ScreenCaptureKitRecorder.swiftelectron/native/bin/darwin-arm64/openscreen-native-cursor-monitorelectron/native/bin/darwin-arm64/openscreen-screencapturekit-helperelectron/native/bin/darwin-arm64/openscreen-system-cursorselectron/native/bin/darwin-arm64/recordly-native-cursor-monitorelectron/native/bin/darwin-arm64/recordly-screencapturekit-helperelectron/native/bin/darwin-arm64/recordly-system-cursorselectron/native/bin/darwin-arm64/recordly-window-listelectron/native/bin/darwin-arm64/whisper-runtime.jsonelectron/native/bin/darwin-x64/openscreen-native-cursor-monitorelectron/native/bin/darwin-x64/openscreen-screencapturekit-helperelectron/native/bin/darwin-x64/openscreen-system-cursorselectron/native/bin/darwin-x64/openscreen-window-listelectron/native/bin/darwin-x64/recordly-native-cursor-monitorelectron/native/bin/darwin-x64/recordly-screencapturekit-helperelectron/native/bin/darwin-x64/recordly-system-cursorselectron/native/bin/darwin-x64/recordly-window-listelectron/native/bin/win32-x64/helpers-manifest.jsonelectron/preload.tselectron/rendererServer.tselectron/uiohook-napi.d.tselectron/updater.tselectron/windows.tssrc/lib/exporter/annotationRenderer.tssrc/lib/exporter/audioEncoder.tssrc/lib/exporter/captionRenderer.tssrc/lib/exporter/exportTuning.test.tssrc/lib/exporter/exportTuning.tssrc/lib/exporter/forwardFrameSource.tssrc/lib/exporter/frameRenderer.test.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.test.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/index.tssrc/lib/exporter/localMediaSource.test.tssrc/lib/exporter/localMediaSource.tssrc/lib/exporter/modernFrameRenderer.test.tssrc/lib/exporter/modernFrameRenderer.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/mp4Support.tssrc/lib/exporter/muxer.tssrc/lib/exporter/nativeFrameCapture.tssrc/lib/exporter/shadowProfile.tssrc/lib/exporter/streamingDecoder.test.tssrc/lib/exporter/streamingDecoder.tssrc/lib/exporter/types.test.tssrc/lib/exporter/types.tssrc/lib/exporter/videoDecoder.tssrc/lib/exporter/videoExporter.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/exporter/modernVideoExporter.ts (1)
767-783:⚠️ Potential issue | 🟠 MajorGate Breeze on the full IPC surface, not just
nativeVideoExportStart.This method later calls
nativeVideoExportWriteFrame,nativeVideoExportFinish, andnativeVideoExportCancelunconditionally, so an older or partially updated preload bridge will now fail mid-export instead of falling back.src/lib/exporter/videoExporter.tsalready does the fuller guard at Lines 343-352.Possible fix
- if (typeof window === "undefined" || !window.electronAPI?.nativeVideoExportStart) { + if ( + typeof window === "undefined" || + typeof window.electronAPI?.nativeVideoExportStart !== "function" || + typeof window.electronAPI?.nativeVideoExportWriteFrame !== "function" || + typeof window.electronAPI?.nativeVideoExportFinish !== "function" || + typeof window.electronAPI?.nativeVideoExportCancel !== "function" + ) { this.lastNativeExportError = `${NATIVE_EXPORT_ENGINE_NAME} export is not available in this build.`; return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/modernVideoExporter.ts` around lines 767 - 783, The guard currently only checks for window.electronAPI.nativeVideoExportStart, which risks runtime failures because modernVideoExporter later calls nativeVideoExportWriteFrame, nativeVideoExportFinish, and nativeVideoExportCancel; update the feature-detection to verify the full IPC surface exists (check nativeVideoExportStart, nativeVideoExportWriteFrame, nativeVideoExportFinish, nativeVideoExportCancel) before returning true, and keep the existing checks for even dimensions and VideoEncoder.isConfigSupported; set this.lastNativeExportError and the fallback warning using NATIVE_EXPORT_ENGINE_NAME as before when any required IPC method is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/exporter/forwardFrameSource.ts`:
- Around line 41-55: The toLocalFilePath function currently ignores the URL
hostname so UNC paths like file://server/share/video.mp4 lose the host; update
toLocalFilePath to preserve url.hostname when present: after building filePath
from decodeURIComponent(url.pathname), if url.hostname is non-empty (and not
just localhost), join it with the path as a UNC-style Windows path (e.g. prepend
`\\${url.hostname}` and convert forward slashes to backslashes), while keeping
the existing drive-letter handling (slice leading slash for /^\/[A-Za-z]:/);
also ensure the fallback catch path handles hostnames if parsing fails.
- Around line 346-348: The cancel() method currently only sets this.cancelled
and doesn't wake any pending waits in getFrameAtTime()/getNextFrame(); update
cancel() to also unblock any in-flight getNextFrame() promise by invoking the
stored resolver/rejecter or signaling the awaiting mechanism (e.g., call
this._pendingResolve or this._pendingReject/abortController) so getFrameAtTime()
returns immediately; ensure getNextFrame() sets that pending resolver when it
awaits and clears it after resolution to avoid leaks and race conditions.
In `@src/lib/exporter/frameRenderer.test.ts`:
- Line 238: Tests currently use "as any" on the result of createRenderer(),
bypassing TypeScript checks; define a FrameRendererTestAccess interface that
extends FrameRenderer and declares the private properties/methods you access
(e.g., the specific internal fields and helper methods referenced in these
tests), then replace each "createRenderer() as any" with a safe cast via
unknown: (createRenderer() as unknown as FrameRendererTestAccess). Update all
test locations that used "as any" (places referencing private members) to use
this FrameRendererTestAccess type so tests remain type-safe while still
accessing internals.
In `@src/lib/exporter/modernVideoExporter.ts`:
- Around line 226-234: The computed maxNativeWriteInFlight is never enforced:
after removing waitForNativeWriteSlot(), encoded chunks are dispatched
immediately via nativeVideoExportWriteFrame and only tracked in
nativeWritePromises, allowing unbounded buffering; restore enforcement by gating
calls to nativeVideoExportWriteFrame so the number of outstanding
nativeWritePromises never exceeds this.maxNativeWriteInFlight (e.g., await or
queue when nativeWritePromises.length >= this.maxNativeWriteInFlight), update
places where chunks are produced to push into a controlled queue and only call
nativeVideoExportWriteFrame when a slot is available, and keep/rehydrate any
helper logic from waitForNativeWriteSlot() to release slots when a
nativeWritePromise resolves so backpressure is applied consistently across
nativeWritePromises, nativeVideoExportWriteFrame, and maxNativeWriteInFlight.
In `@src/lib/exporter/videoExporter.ts`:
- Around line 549-570: The output handler currently preserves write order via
this.nativePendingWrite but lets unlimited H.264 chunks be copied into memory
ahead of IPC/FFmpeg, so add explicit backpressure: introduce a
maxNativePendingWrites constant (e.g., 2–4) and a counter (e.g.,
this.nativePendingWriteCount) that you increment before copying the chunk buffer
and decrement inside the chained handler when its write completes or errors; if
the counter would exceed maxNativePendingWrites, await this.nativePendingWrite
(or a Promise that resolves when pending writes drop) before copying/enqueuing
the next chunk. Apply the same bounded-write logic where nativePendingWrite is
used elsewhere (references: nativePendingWrite, nativeVideoExportWriteFrame,
encodeQueueSize) so renderer memory cannot grow unbounded.
---
Outside diff comments:
In `@src/lib/exporter/modernVideoExporter.ts`:
- Around line 767-783: The guard currently only checks for
window.electronAPI.nativeVideoExportStart, which risks runtime failures because
modernVideoExporter later calls nativeVideoExportWriteFrame,
nativeVideoExportFinish, and nativeVideoExportCancel; update the
feature-detection to verify the full IPC surface exists (check
nativeVideoExportStart, nativeVideoExportWriteFrame, nativeVideoExportFinish,
nativeVideoExportCancel) before returning true, and keep the existing checks for
even dimensions and VideoEncoder.isConfigSupported; set
this.lastNativeExportError and the fallback warning using
NATIVE_EXPORT_ENGINE_NAME as before when any required IPC method is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ede4a9eb-ae97-4912-9f38-7fe30ed126ed
📒 Files selected for processing (15)
electron/extensions/errorUtils.tselectron/extensions/extensionIpc.tselectron/extensions/extensionMarketplace.tselectron/ipc/nativeVideoExport.tselectron/ipc/windowsCaptureSelection.tssrc/lib/exporter/annotationRenderer.tssrc/lib/exporter/audioEncoder.tssrc/lib/exporter/forwardFrameSource.tssrc/lib/exporter/frameRenderer.test.tssrc/lib/exporter/gifExporter.test.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/mp4Support.tssrc/lib/exporter/streamingDecoder.tssrc/lib/exporter/videoExporter.ts
✅ Files skipped from review due to trivial changes (1)
- electron/ipc/windowsCaptureSelection.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- electron/extensions/extensionIpc.ts
- src/lib/exporter/mp4Support.ts
- electron/extensions/extensionMarketplace.ts
- electron/ipc/nativeVideoExport.ts
- src/lib/exporter/gifExporter.test.ts
- src/lib/exporter/gifExporter.ts
- src/lib/exporter/annotationRenderer.ts
- src/lib/exporter/audioEncoder.ts
- src/lib/exporter/streamingDecoder.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/exporter/videoExporter.ts`:
- Around line 568-590: The code assigns this.nativePendingWrite inside an
expression which Biome flags; refactor by computing the chained promise first
and then assigning it to this.nativePendingWrite before calling
trackNativeWritePromise. Specifically, build the promise from
this.nativePendingWrite.then(async () => { await
window.electronAPI.nativeVideoExportWriteFrame(sessionId, new
Uint8Array(buffer)); ... }).catch(error => { if (!this.cancelled &&
!this.nativeEncoderError) this.nativeEncoderError = error instanceof Error ?
error : new Error(String(error)); if (!this.cancelled && !this.nativeWriteError)
this.nativeWriteError = error instanceof Error ? error : new
Error(String(error)); }); then set this.nativePendingWrite = thatPromise, set
writePromise to thatPromise, and call
this.trackNativeWritePromise(writePromise); keep references to
nativePendingWrite, nativeVideoExportWriteFrame, cancelled, nativeEncoderError,
nativeWriteError, and trackNativeWritePromise intact.
- Around line 839-849: awaitOldestNativeWrite() currently awaits the oldest
promise from nativeWritePromises with no timeout, which can hang forever; modify
awaitOldestNativeWrite to race the oldestWritePromise against a timeout Promise
(use a new constant like MAX_NATIVE_WRITE_WAIT_MS or an existing config) and if
the timeout wins, set/throw a descriptive error (e.g., "Timed out waiting for
native write") so export/finalization can fail fast; keep existing behavior of
throwing this.nativeWriteError if it is set after the awaited promise resolves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0feba18b-b1c0-4a9c-a0a1-b1e42d684202
📒 Files selected for processing (6)
electron/cursorHider.tssrc/lib/exporter/audioEncoder.tssrc/lib/exporter/forwardFrameSource.tssrc/lib/exporter/frameRenderer.test.tssrc/lib/exporter/modernVideoExporter.tssrc/lib/exporter/videoExporter.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/exporter/audioEncoder.ts
- src/lib/exporter/frameRenderer.test.ts
- electron/cursorHider.ts
- src/lib/exporter/forwardFrameSource.ts
- src/lib/exporter/modernVideoExporter.ts
# Conflicts: # electron/electron-env.d.ts # electron/ipc/handlers.ts
This was referenced Apr 17, 2026
This was referenced Apr 27, 2026
9 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
Major export pipeline overhaul with a new H.264 stream-copy mode, full exporter hardening, and native binary rename.
Changes
inputMode: 'h264-stream'in native export IPC — browserVideoEncoderoutput stream-copied into MP4 via ffmpeg rather than piping raw RGBA frames, significantly cutting memory usageapprovedLocalReadPaths(undefined in current main) withuserApprovedPathsset populated from dialog pickers and project load;isAllowedLocalReadPathchecks it alongside dir prefix /existsSyncframeRenderer,modernFrameRenderer,audioEncoder,streamingDecoder,gifExporter,muxer— drift correction, backpressure, proper teardown, cursor-follow camera, annotation blur, shadow/squircle supportopenscreen-*→recordly-*on darwin-arm64 and darwin-x64; update win32-x64 helpers-manifestinputModeand new IPC surface to rendererFixes
ReferenceError: approvedLocalReadPaths is not definedcrash when loading a videoSummary by CodeRabbit
New Features
Bug Fixes
Style
Refactor