Improve recording and playback performance#516
Conversation
|
Caution Review failedFailed to post review comments 📝 WalkthroughWalkthroughImplements disk-backed streaming for recorded video and microphone sidecars via new Electron IPC, refactors renderer to write ordered chunk streams with abort/fallback paths, and improves playback responsiveness via throttled time updates and cached/binary-search lookups for captions, cursor, and zoom regions. ChangesStream-based Recording and Playback Optimization
Sequence DiagramsequenceDiagram
participant Renderer as Renderer Process
participant Preload as Preload Bridge
participant Main as Main Process
participant Disk as Disk
rect rgba(100, 200, 255, 0.5)
Note over Renderer,Main: Stream-based Recording Flow
Renderer->>Preload: recording-stream-open
Preload->>Main: IPC recording-stream-open
Main-->>Preload: streamId
Preload-->>Renderer: streamId
loop For each recorded chunk
Renderer->>Preload: recording-stream-write (streamId, position, chunk)
Preload->>Main: IPC recording-stream-write
Main->>Disk: append chunk to temp file
end
Renderer->>Preload: recording-stream-close (streamId, options)
Preload->>Main: IPC recording-stream-close
Main->>Main: finalize (remux/convert/metadata)
Main->>Disk: move temp to final path
Main-->>Preload: close result
Preload-->>Renderer: close result (path/bytesWritten)
end
rect rgba(200, 100, 255, 0.5)
Note over Renderer: Playback Responsiveness Flow
Renderer->>Renderer: Emit time updates (throttled)
Renderer->>Renderer: Binary search cursor & active caption word
Renderer->>Renderer: Use cached zoom-region relationships
Renderer->>Renderer: Render UI update
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/components/video-editor/videoPlayback/zoomRegionUtils.ts (1)
272-274: ⚡ Quick winHoist the empty connected-region cache to avoid per-call Map allocations.
findDominantRegionis called on the playback hot path; allocating two newMaps and an array per invocation whenconnectZoomsis disabled is avoidable. A module-level frozen empty cache works becausegetActiveRegiononly reads from the maps.♻️ Proposed refactor
const connectedRegionCache = new WeakMap<ZoomRegion[], ConnectedRegionCache>(); + +const EMPTY_CONNECTED_REGION_CACHE: ConnectedRegionCache = { + pairs: [], + outgoingByRegionId: new Map(), + incomingByRegionId: new Map(), +};const connectedCache = options.connectZooms ? getConnectedRegionCache(regions) - : { pairs: [], outgoingByRegionId: new Map(), incomingByRegionId: new Map() }; + : EMPTY_CONNECTED_REGION_CACHE;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/videoPlayback/zoomRegionUtils.ts` around lines 272 - 274, The code allocates new empty Maps and an array on every call when options.connectZooms is false (in the connectedCache assignment used by findDominantRegion/getActiveRegion); hoist a module-level frozen EMPTY_CONNECTED_CACHE constant (with pairs: [], outgoingByRegionId: new Map(), incomingByRegionId: new Map()) and return that instead of constructing new objects per call, ensuring the maps are only created once and kept read-only for callers like getActiveRegion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/register/recording.ts`:
- Around line 166-187: The in-memory sessions in recordingFileStreams and
microphoneSidecarStreams leak when a renderer dies because sessions aren't tied
to an owner; modify session creation (the FileStreamSession /
RecordingFileStreamSession entries) to record the owning renderer/webContents
id, maintain a secondary map from webContentsId -> Set<streamId>, and update
that set when streams are opened/closed. Add handlers for render-process-gone /
window destroyed / app quit that iterate the streamIds for the affected
webContentsId and call the existing stream teardown logic (the same cleanup used
by your close*Stream paths: close fileHandle, delete tempPath, clear
promises/flags, and remove entries from recordingFileStreams /
microphoneSidecarStreams). Ensure all code paths that remove streams also update
the webContentsId mapping so no stale references remain.
In `@src/components/video-editor/captionLayout.ts`:
- Around line 416-421: getCaptionLayoutCacheKey currently builds a cache key
from maxWidthPx and maxRows only, which can return stale layouts when
font-affecting settings change; update the key logic in getCaptionLayoutCacheKey
to include a font signature (e.g., settings.fontFamily and settings.fontSize or
a combined string derived from AutoCaptionSettings) so the inner Map entries are
keyed by font metrics used by buildCaptionLines/measureText, or alternatively
detect changes to those settings and clear the inner Map for the corresponding
cues; also note callers must avoid in-place mutation of the cues array since the
outer WeakMap uses reference identity.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 1343-1360: The effect in VideoPlayback that controls
bgVideo/webcam playback misses handling when the source changes and also a
resume branch calls webcamVideo.play() without guarding
webcamEnabled/webcamVideoPath; update the effect that references bgVideoRef and
webcamVideoRef to include the background source (bgVideoPath) in its dependency
list so a newly mounted bgVideo gets play() when isPlaying is true, and modify
the resume code path later that calls webcamVideo.play() to first check
webcamEnabled && webcamVideoPath before invoking play(), using the same
play().catch(() => undefined) pattern; touch the identifiers bgVideoRef,
webcamVideoRef, isPlaying, webcamEnabled, webcamVideoPath, and bgVideoPath to
locate and fix the logic.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 129-169: createQueuedBlobWriter currently swallows the first
streaming write error by marking writeError but leaving write() as a no-op,
causing subsequent chunks to be dropped; update the implementation so the first
writeChunk rejection fails fast: in the catch handler set writeError (as now)
and also set writeQueue = Promise.reject(writeError) to break the queue chain,
and change the write function to immediately throw or return a rejected promise
when writeError is set (i.e., reference createQueuedBlobWriter, the write
function, writeQueue, writeError, and params.writeChunk) so any subsequent write
calls fail fast instead of silently dropping chunks; keep waitForWrites to
rethrow writeError when awaited.
---
Nitpick comments:
In `@src/components/video-editor/videoPlayback/zoomRegionUtils.ts`:
- Around line 272-274: The code allocates new empty Maps and an array on every
call when options.connectZooms is false (in the connectedCache assignment used
by findDominantRegion/getActiveRegion); hoist a module-level frozen
EMPTY_CONNECTED_CACHE constant (with pairs: [], outgoingByRegionId: new Map(),
incomingByRegionId: new Map()) and return that instead of constructing new
objects per call, ensuring the maps are only created once and kept read-only for
callers like getActiveRegion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b396e62b-06e7-4b56-aa6b-f5fc9b92ec29
📒 Files selected for processing (10)
README.mdelectron/electron-env.d.tselectron/ipc/register/recording.tselectron/preload.tssrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/captionLayout.tssrc/components/video-editor/videoPlayback/cursorRenderer.tssrc/components/video-editor/videoPlayback/videoEventHandlers.tssrc/components/video-editor/videoPlayback/zoomRegionUtils.tssrc/hooks/useScreenRecorder.ts
| type FileStreamSession = { | ||
| streamId: string; | ||
| fileHandle: Awaited<ReturnType<typeof fs.open>>; | ||
| tempPath: string; | ||
| bytesWritten: number; | ||
| highestWatermark: number; | ||
| writeQueue: Promise<void>; | ||
| aborted: boolean; | ||
| }; | ||
|
|
||
| type RecordingFileStreamSession = FileStreamSession & { | ||
| finalPath: string; | ||
| }; | ||
|
|
||
| type RecordingStreamCloseOptions = { | ||
| abort?: boolean; | ||
| mimeType?: string | null; | ||
| }; | ||
|
|
||
| const recordingFileStreams = new Map<string, RecordingFileStreamSession>(); | ||
| const microphoneSidecarStreams = new Map<string, FileStreamSession>(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether streamed recording sessions have any renderer-teardown cleanup.
# Expected: session maps are defined in recording.ts, but there is no cleanup hook
# tied to renderer/webContents lifecycle events for them.
rg -n -C3 'recordingFileStreams|microphoneSidecarStreams|render-process-gone|destroyed|did-start-navigation|webContents' electronRepository: webadderallorg/Recordly
Length of output: 23677
🏁 Script executed:
# Check the handler registration and closeStreamSession implementation
sed -n '1750,1850p' electron/ipc/register/recording.tsRepository: webadderallorg/Recordly
Length of output: 3336
🏁 Script executed:
# Search for any cleanup tied to BrowserWindow events or render-process-gone
rg -n 'render-process-gone|destroyed|did-start-navigation' electron/ipc/register/recording.tsRepository: webadderallorg/Recordly
Length of output: 49
🏁 Script executed:
# Check if closeStreamSession is defined and what it does
rg -n -A15 'function closeStreamSession' electron/ipc/register/recording.tsRepository: webadderallorg/Recordly
Length of output: 489
🏁 Script executed:
# Get full closeStreamSession implementation
sed -n '236,280p' electron/ipc/register/recording.tsRepository: webadderallorg/Recordly
Length of output: 1112
🏁 Script executed:
# Check if there are any BrowserWindow event listeners or cleanup tied to stream sessions
rg -n 'ipcMain.once|webContents.once|BrowserWindow.on|\.on\(' electron/ipc/register/recording.ts | head -20Repository: webadderallorg/Recordly
Length of output: 397
🏁 Script executed:
# Check if there's any cleanup in main.ts or windows.ts for these specific stream maps
rg -n 'recordingFileStreams|microphoneSidecarStreams' electron/Repository: webadderallorg/Recordly
Length of output: 998
🏁 Script executed:
# Check for any app-level cleanup or window destruction handlers that might handle streams
rg -n 'app.on.*quit|app.on.*before|window.*closed|window.*destroyed' electron/main.ts | head -15Repository: webadderallorg/Recordly
Length of output: 141
🏁 Script executed:
# Check the before-quit and window-all-closed handlers in main.ts
sed -n '877,920p' electron/main.tsRepository: webadderallorg/Recordly
Length of output: 1414
Tie streamed file sessions to the owning renderer and clean them up on teardown.
These sessions only disappear on an explicit close*Stream call. If the renderer reloads, crashes, or is destroyed mid-recording, the open file handle and temp file stay pinned in the process-global maps recordingFileStreams and microphoneSidecarStreams, leaking file descriptors and leaving junk temp artifacts behind. No cleanup occurs on render-process-gone, window destruction, or app quit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@electron/ipc/register/recording.ts` around lines 166 - 187, The in-memory
sessions in recordingFileStreams and microphoneSidecarStreams leak when a
renderer dies because sessions aren't tied to an owner; modify session creation
(the FileStreamSession / RecordingFileStreamSession entries) to record the
owning renderer/webContents id, maintain a secondary map from webContentsId ->
Set<streamId>, and update that set when streams are opened/closed. Add handlers
for render-process-gone / window destroyed / app quit that iterate the streamIds
for the affected webContentsId and call the existing stream teardown logic (the
same cleanup used by your close*Stream paths: close fileHandle, delete tempPath,
clear promises/flags, and remove entries from recordingFileStreams /
microphoneSidecarStreams). Ensure all code paths that remove streams also update
the webContentsId mapping so no stale references remain.
| function getCaptionLayoutCacheKey(settings: AutoCaptionSettings, maxWidthPx: number) { | ||
| return [ | ||
| Math.round(maxWidthPx * 100) / 100, | ||
| clamp(Math.round(settings.maxRows || 1), 1, 4), | ||
| ].join(":"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Identify call sites of buildActiveCaptionLayout and how measureText is constructed.
rg -nP --type=ts -C5 '\bbuildActiveCaptionLayout\s*\('
# 2. Look at AutoCaptionSettings shape to see which fields could affect text measurement.
ast-grep --pattern $'export interface AutoCaptionSettings {
$$$
}'
# 3. Check whether cues arrays are ever mutated in place vs. replaced immutably.
rg -nP --type=ts -C2 '\bcues\s*\.\s*(push|splice|pop|shift|unshift|sort|reverse)\b|\bcues\s*\[\s*\d+\s*\]\s*='Repository: webadderallorg/Recordly
Length of output: 5374
🏁 Script executed:
sed -n '469,530p' src/components/video-editor/captionLayout.tsRepository: webadderallorg/Recordly
Length of output: 1602
🏁 Script executed:
# Find where captionStaticLayoutCache is declared and used
rg -n 'captionStaticLayoutCache' src/components/video-editor/captionLayout.ts
# Check context around declaration
sed -n '400,425p' src/components/video-editor/captionLayout.ts
# Search for any cache clearing/invalidation
rg -n 'captionStaticLayoutCache\s*\=' src/Repository: webadderallorg/Recordly
Length of output: 1042
🏁 Script executed:
# Check buildActiveCaptionLayout signature and cache handling
sed -n '532,545p' src/components/video-editor/captionLayout.ts
# Look for any settings validation or cache clearing in buildActiveCaptionLayout
sed -n '532,570p' src/components/video-editor/captionLayout.ts
# Check if there's any comment or documentation about cache invalidation
rg -n 'cache|Cache' src/components/video-editor/captionLayout.tsRepository: webadderallorg/Recordly
Length of output: 2577
Cache key must include font-related settings to avoid stale layouts.
getCaptionLayoutCacheKey includes only maxWidthPx and maxRows, but the cached layout is computed from buildCaptionLines, which depends on measureText—a closure over the caller-provided font context. If a caller changes settings.fontSize or settings.fontFamily while passing the same cues reference and maxWidthPx, the inner Map will return a stale layout computed with the old font metrics (wrong line breaks and word boundaries).
Include a font signature in the cache key (e.g., derived from fontFamily and fontSize in AutoCaptionSettings), or clear the inner Map when font-affecting settings change.
Note: Callers must also avoid mutating the cues array in place (e.g., cues[i] = newCue), since the outer WeakMap depends on reference identity. React typically replaces arrays on state updates, but any in-place mutations will silently return a stale entry.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/video-editor/captionLayout.ts` around lines 416 - 421,
getCaptionLayoutCacheKey currently builds a cache key from maxWidthPx and
maxRows only, which can return stale layouts when font-affecting settings
change; update the key logic in getCaptionLayoutCacheKey to include a font
signature (e.g., settings.fontFamily and settings.fontSize or a combined string
derived from AutoCaptionSettings) so the inner Map entries are keyed by font
metrics used by buildCaptionLines/measureText, or alternatively detect changes
to those settings and clear the inner Map for the corresponding cues; also note
callers must avoid in-place mutation of the cues array since the outer WeakMap
uses reference identity.
| if (bgVideo.paused) { | ||
| bgVideo.play().catch(() => undefined); | ||
| } | ||
| } else { | ||
| bgVideo.pause(); | ||
| } | ||
| } | ||
| }, [isPlaying]); | ||
| const webcamVideo = webcamVideoRef.current; | ||
| if (webcamVideo) { | ||
| if (isPlaying && webcamEnabled && webcamVideoPath) { | ||
| if (webcamVideo.paused) { | ||
| webcamVideo.play().catch(() => undefined); | ||
| } | ||
| } else { | ||
| webcamVideo.pause(); | ||
| } | ||
| } | ||
| }, [isPlaying, webcamEnabled, webcamVideoPath]); |
There was a problem hiding this comment.
Cover source changes and resume paths in the new auxiliary-media play/pause flow.
This effect is now the main owner of background/webcam playback, but it still misses two lifecycle paths: swapping to a new background video while isPlaying stays true can leave the newly mounted element paused, and the resume branch later in the file still calls webcamVideo.play() without the webcamEnabled / webcamVideoPath guard. That means the refactor can still regress playback correctness and hidden decode work after suspendRendering flips back off.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/video-editor/VideoPlayback.tsx` around lines 1343 - 1360, The
effect in VideoPlayback that controls bgVideo/webcam playback misses handling
when the source changes and also a resume branch calls webcamVideo.play()
without guarding webcamEnabled/webcamVideoPath; update the effect that
references bgVideoRef and webcamVideoRef to include the background source
(bgVideoPath) in its dependency list so a newly mounted bgVideo gets play() when
isPlaying is true, and modify the resume code path later that calls
webcamVideo.play() to first check webcamEnabled && webcamVideoPath before
invoking play(), using the same play().catch(() => undefined) pattern; touch the
identifiers bgVideoRef, webcamVideoRef, isPlaying, webcamEnabled,
webcamVideoPath, and bgVideoPath to locate and fix the logic.
| function createQueuedBlobWriter(params: { | ||
| streamId: string; | ||
| writeChunk: ( | ||
| streamId: string, | ||
| position: number, | ||
| chunk: Uint8Array, | ||
| ) => Promise<{ success: boolean; error?: string }>; | ||
| }) { | ||
| let position = 0; | ||
| let writeQueue = Promise.resolve(); | ||
| let writeError: Error | null = null; | ||
|
|
||
| const write = (blob: Blob) => { | ||
| if (blob.size <= 0 || writeError) { | ||
| return; | ||
| } | ||
|
|
||
| writeQueue = writeQueue | ||
| .then(async () => { | ||
| const buffer = await blob.arrayBuffer(); | ||
| const bytes = new Uint8Array(buffer); | ||
| const writePosition = position; | ||
| position += bytes.byteLength; | ||
| const result = await params.writeChunk(params.streamId, writePosition, bytes); | ||
| if (!result.success) { | ||
| throw new Error(result.error || "Failed to write recording chunk"); | ||
| } | ||
| }) | ||
| .catch((error) => { | ||
| writeError = error instanceof Error ? error : new Error(String(error)); | ||
| }); | ||
| }; | ||
|
|
||
| const waitForWrites = async () => { | ||
| await writeQueue; | ||
| if (writeError) { | ||
| throw writeError; | ||
| } | ||
| }; | ||
|
|
||
| return { write, waitForWrites }; |
There was a problem hiding this comment.
Fail fast on the first streamed write error.
After the first writeChunk rejection, later write() calls just no-op until close() finally rethrows. That means the UI can keep “recording” for minutes while every subsequent chunk is being dropped, and the user only finds out at stop time.
Also applies to: 172-253
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/hooks/useScreenRecorder.ts` around lines 129 - 169,
createQueuedBlobWriter currently swallows the first streaming write error by
marking writeError but leaving write() as a no-op, causing subsequent chunks to
be dropped; update the implementation so the first writeChunk rejection fails
fast: in the catch handler set writeError (as now) and also set writeQueue =
Promise.reject(writeError) to break the queue chain, and change the write
function to immediately throw or return a rejected promise when writeError is
set (i.e., reference createQueuedBlobWriter, the write function, writeQueue,
writeError, and params.writeChunk) so any subsequent write calls fail fast
instead of silently dropping chunks; keep waitForWrites to rethrow writeError
when awaited.
|
|
|
|
|
Thanks for the heads-up. This PR is legitimate work, not an automated drive-by submission. Context:
I resolved the conflict by preserving the microphone sidecar finalization logic and retaining the Windows path recovery helpers. I also checked the later usages in the file to ensure the kept functions are still referenced and necessary. Happy to add more testing notes or link to an issue if useful. |
1 participant
Summary
Tests
Summary by CodeRabbit