refactor handles.ts into smaller files#254
Merged
webadderall merged 6 commits intomainfrom Apr 17, 2026
Merged
Conversation
handlers.ts was ~5967 lines. Extracted into 22 focused modules: - ipc/types.ts — shared TypeScript interfaces and types - ipc/constants.ts — module-level constants - ipc/state.ts — all mutable state with typed setters - ipc/utils.ts — shared low-level utilities (getScreen, normalizePath, etc.) - ipc/ffmpeg/binary.ts — ffmpeg binary resolution - ipc/ffmpeg/filters.ts — audio sync/filter builders - ipc/captions/parser.ts — SRT/Whisper JSON parsers - ipc/captions/whisper.ts — Whisper model download/status - ipc/captions/generate.ts — auto-caption generation - ipc/paths/binaries.ts — native binary path resolution - ipc/cursor/monitor.ts — cursor monitor process management - ipc/cursor/telemetry.ts — cursor sampling and telemetry - ipc/cursor/bounds.ts — window bounds capture and resolution - ipc/cursor/interaction.ts — mouse hook and interaction capture - ipc/recording/events.ts — recording lifecycle events - ipc/recording/diagnostics.ts — media validation and diagnostics - ipc/recording/prune.ts — auto-recording cleanup - ipc/recording/ffmpeg.ts — FFmpeg screen capture - ipc/recording/windows.ts — Windows native capture (WGC) - ipc/recording/mac.ts — Mac ScreenCaptureKit integration - ipc/export/native-video.ts — native video export sessions - ipc/project/session.ts — recording session manifests - ipc/project/manager.ts — project library and file management handlers.ts reduced from 5967 → 2930 lines (registerIpcHandlers + helpers only)
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Captions electron/ipc/captions/generate.ts, electron/ipc/captions/parser.ts, electron/ipc/captions/whisper.ts, electron/ipc/register/captions.ts |
New auto-caption pipeline: audio candidate resolution/extraction (ffmpeg), Whisper executable/model discovery, model download/delete with progress IPC, JSON↔SRT parsing and retry logic, and IPC handlers to drive caption workflows. |
Cursor capture & telemetry electron/ipc/cursor/... electron/ipc/cursor/bounds.ts, electron/ipc/cursor/interaction.ts, electron/ipc/cursor/monitor.ts, electron/ipc/cursor/telemetry.ts |
Window-bounds resolution per-platform, uiohook-based interaction capture, native cursor-monitor process handling, sampling/normalization/buffering of cursor telemetry and persistence. |
Native recording (platform) electron/ipc/recording/... electron/ipc/recording/mac.ts, electron/ipc/recording/windows.ts, electron/ipc/recording/ffmpeg.ts, electron/ipc/recording/diagnostics.ts, electron/ipc/recording/prune.ts, electron/ipc/recording/events.ts, electron/ipc/register/recording.ts |
Platform-specific capture lifecycles (mac/Windows) plus FFmpeg fallback: start/stop synchronization, diagnostics, audio probing/sync/muxing, recovery/finalization, pruning, and comprehensive IPC handlers. |
Native video export electron/ipc/export/native-video.ts, electron/ipc/register/export.ts |
FFmpeg-backed export sessions supporting stdin frame writes, backpressure/queueing, encoder probing/selection, audio mux post-processing, and IPC session lifecycle handlers. |
FFmpeg utilities electron/ipc/ffmpeg/binary.ts, electron/ipc/ffmpeg/filters.ts |
Bundled/system ffmpeg resolution (asar-unpacked handling), encoder probing helpers, filter graph builders (tempo/delay, pause trimming), and duration parsing utilities. |
Native helper paths & provisioning electron/ipc/paths/binaries.ts |
Centralized bundled/native helper path resolution, legacy migration, asar-unpacked mapping, platform candidates for helpers/Whisper, and optional Swift helper compilation logic. |
Project & session management electron/ipc/project/manager.ts, electron/ipc/project/session.ts, electron/ipc/register/project.ts |
Project persistence, recent list and thumbnails, recording-session manifest creation/resolution, approved local-read tracking, and IPC handlers for project flows. |
IPC registration & utilities electron/ipc/register/*.ts, electron/ipc/state.ts, electron/ipc/types.ts, electron/ipc/utils.ts, electron/ipc/constants.ts |
New centralized constants, shared state setters and types, path/util helpers, and many IPC handlers for assets, permissions, settings, sources, export, and miscellaneous app workflows. |
Sequence Diagram(s)
sequenceDiagram
participant UI as Renderer (UI)
participant Main as Main Process
participant FS as File System
participant FFmpeg as FFmpeg
participant Whisper as Whisper CLI
UI->>Main: generate-auto-captions(videoPath, whisperModelPath?)
Main->>Main: normalize paths, resolve ffmpeg & whisper executables
Main->>FS: stat/validate audio candidate files
Main->>FFmpeg: spawn to extract 16kHz mono WAV
FFmpeg-->>Main: success (wavPath) / failure
alt extracted audio
Main->>Whisper: spawn Whisper CLI (JSON preferred)
Whisper-->>Main: JSON or SRT output
Main->>Main: parse cues (JSON or SRT)
Main->>FS: cleanup temp files
Main-->>UI: return { cues, audioSourceLabel }
else no audio extracted
Main-->>UI: throw "no audio track" error
end
sequenceDiagram
participant UI as Renderer
participant Main as Main Process
participant Native as Native Helper (mac/win)
participant FFmpeg as FFmpeg
participant FS as File System
UI->>Main: start native recording(options)
Main->>Native: spawn helper process with config
Native-->>Main: stdout "Recording started"
Main-->>UI: ack started
par capture running
Native->>FS: write raw video (+ optional audio files)
end
UI->>Main: stop recording
Main->>Native: send "stop"
Native-->>Main: stdout "Recording stopped" + outputPath
Main->>FFmpeg: probe durations
Main->>Main: compute audio sync adjustments
Main->>FFmpeg: mux video + adjusted audio -> muxed output
FFmpeg->>FS: write muxed file
Main->>FS: replace original, delete temp audio
Main-->>UI: { success, videoPath }
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~65 minutes
Possibly related PRs
- webadderall/Recordly#142 — overlaps macOS native recording recovery and muxing companion audio into recovered videos.
- webadderall/Recordly#124 — touches Whisper/auto-captions IPC and model download/generation flows.
- webadderall/Recordly#228 — related FFmpeg audio-muxing and encoder/export resolution logic.
Suggested labels
Checked
Poem
🐰 I thumped my paw and built a stream,
Whisper hummed and captions gleam,
Cursors hopped on sampled trails,
Native helpers told their tales,
Exports stitched the final dream.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 1.32% 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 'refactor handles.ts into smaller files' accurately describes the main change: splitting a monolithic 5,967-line file into 22 focused sub-modules. |
| Description check | ✅ Passed | The pull request description comprehensively covers the refactor scope, lists all 22 new modules with their responsibilities, provides metrics (lines reduced), and explicitly states no functional changes. However, it does not follow the provided template structure with Description/Motivation/Type/Related Issues/Testing sections. |
✏️ 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
refactor/handlers-modules
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.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/video-editor/timeline/TimelineEditor.tsx (2)
1780-1804:⚠️ Potential issue | 🟡 MinorDead code: trim/speed rows removed but their add/delete/shortcut paths remain.
With trim and speed regions no longer rendered into
timelineItems(line 1780) orallRegionSpans(line 1803), the following code paths become effectively dead / silently broken:
handleAddTrim(lines 1411-1449) andhandleAddSpeed(lines 1458-1496) still fireonTrimAdded/onSpeedAdded, but the resulting regions have no UI to display. PerVideoEditor.tsx(context snippet 1),trimRegionsis derived fromclipRegionsvia auseEffect, so user-added trim regions would also be overwritten on the next render.- Keyboard shortcuts
keyShortcuts.addTrimandkeyShortcuts.addSpeed(lines 1595-1606) remain wired — pressing them does "nothing visible" from the user's perspective.deleteSelectedTrim/deleteSelectedSpeed(lines 1007, 1025) and the Delete/Backspace branches (1647, 1653) remain, but nothing can select them now.hasOverlapstill branches onisTrimItem/isSpeedItem(lines 1237, 1240), which can never match.Either re-introduce trim/speed rendering, or remove these handlers/shortcuts/props (
onTrimAdded,onSpeedAdded,onTrimDelete,onSpeedDelete,selectedTrimId,selectedSpeedId, the two shortcut bindings, and the correspondinghasOverlapbranches) to avoid user-visible dead paths and reduce prop surface.Want me to draft the cleanup patch that strips the trim/speed surface from this component?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 1780 - 1804, Trim/speed handlers and shortcuts are now dead because trim/speed regions are no longer rendered into timelineItems/allRegionSpans; remove the orphaned API surface instead of leaving silent no-ops: delete or unhook handleAddTrim, handleAddSpeed, deleteSelectedTrim, deleteSelectedSpeed, the keyboard bindings for keyShortcuts.addTrim and keyShortcuts.addSpeed, and any props and callbacks related to trims/speeds (onTrimAdded, onSpeedAdded, onTrimDelete, onSpeedDelete, selectedTrimId, selectedSpeedId), and remove the isTrimItem/isSpeedItem branches in hasOverlap so it no longer checks unreachable item types; ensure any useEffect that maps clipRegions→trimRegions or code that overwrites user-added trims is removed or adjusted so there are no leftover effects that recreate invisible regions.
501-531:⚠️ Potential issue | 🟡 MinorRTL:
transform: translateX(-50%)miscenters markers.The marker container anchors via
[sideProperty](set torightin RTL) but appliestransform: translateX(-50%), which always translates toward negative physical X. In RTL, this shifts the marker left of the anchor point instead of centering it on the tick. Flip the sign based on text direction:[sideProperty]: `${offset}px`, - transform: "translateX(-50%)", + transform: direction === "rtl" ? "translateX(50%)" : "translateX(-50%)", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 501 - 531, The marker centering uses a fixed transform: "translateX(-50%)" which miscenters when sideProperty is "right" (RTL); update the markerStyle in the markers.markers.map block to compute the translateX sign based on direction (e.g., derive isRtl from sideProperty === "right" or from a provided dir prop) and set transform to `translateX(-50%)` for LTR and `translateX(50%)` for RTL (or use `${isRtl ? "translateX(50%)" : "translateX(-50%)"}`), keeping the rest of markerStyle and the use of marker.time as key unchanged so markers are centered correctly in both LTR and RTL.
🧹 Nitpick comments (14)
src/components/video-editor/timeline/Item.tsx (1)
136-145: Inlinestyleon resize handles is redundant with the CSS module.
glassStyles.zoomEndCapalready setspointer-events: auto, and.zoomEndCap.left/.zoomEndCap.rightalready setcursor: col-resize(seeItemGlass.module.csslines 162, 192, 197). The inlinestyleprops can be dropped to keep styling centralized in the CSS module.♻️ Proposed simplification
<div className={cn(glassStyles.zoomEndCap, glassStyles.left)} - style={{ cursor: "col-resize", pointerEvents: "auto" }} title="Resize left" /> <div className={cn(glassStyles.zoomEndCap, glassStyles.right)} - style={{ cursor: "col-resize", pointerEvents: "auto" }} title="Resize right" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/Item.tsx` around lines 136 - 145, The inline style attributes on the two resize handle divs inside the Item component are redundant; remove the style={{ cursor: "col-resize", pointerEvents: "auto" }} props from both divs so styling is controlled by the CSS module. Locate the two elements that use className={cn(glassStyles.zoomEndCap, glassStyles.left)} and className={cn(glassStyles.zoomEndCap, glassStyles.right)} in Item.tsx and delete their inline style props, leaving the className and title props intact to rely on ItemGlass.module.css (zoomEndCap/.left/.right) for cursor and pointer-events.src/components/video-editor/timeline/TimelineEditor.tsx (1)
536-580: Minor: align memoization withTimelineAxisfor consistency.
TimelineAxis(lines 428-479 in this file) memoizes marker times based onintervalMs,range.start/end,videoDurationMsonly, and appliesvalueToPixelsduring render. Here,valueToPixelsis invoked inside theuseMemoand included in deps, which means the memo will re-run whenever the timeline context returns a new function identity (common withuseTimelineContext). Consider moving thevalueToPixelscall out of the memo to match the existing pattern and make the memoization actually effective:♻️ Proposed adjustment
- const markers = useMemo(() => { - if (intervalMs <= 0) return []; - const maxTime = videoDurationMs > 0 ? videoDurationMs : range.end; - const visibleStart = Math.max(0, range.start); - const visibleEnd = Math.min(range.end, maxTime); - const firstMarker = Math.ceil(visibleStart / intervalMs) * intervalMs; - const result: { time: number; offset: number }[] = []; - for (let time = firstMarker; time <= maxTime; time += intervalMs) { - if (time > visibleStart && time < visibleEnd) { - result.push({ - time: Math.round(time), - offset: valueToPixels(Math.round(time) - range.start), - }); - } - } - return result; - }, [intervalMs, range.start, range.end, videoDurationMs, valueToPixels]); + const markerTimes = useMemo(() => { + if (intervalMs <= 0) return [] as number[]; + const maxTime = videoDurationMs > 0 ? videoDurationMs : range.end; + const visibleStart = Math.max(0, range.start); + const visibleEnd = Math.min(range.end, maxTime); + const firstMarker = Math.ceil(visibleStart / intervalMs) * intervalMs; + const result: number[] = []; + for (let time = firstMarker; time <= maxTime; time += intervalMs) { + if (time > visibleStart && time < visibleEnd) { + result.push(Math.round(time)); + } + } + return result; + }, [intervalMs, range.start, range.end, videoDurationMs]);Then compute
offsetat render time inside the.map(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineEditor.tsx` around lines 536 - 580, The markers useMemo in ClipMarkerOverlay currently calls valueToPixels and includes it in the dependency array, causing unnecessary re-runs; change the memo to only compute and return times (e.g., result: { time: number }[]) based on intervalMs, range.start, range.end, and videoDurationMs (matching TimelineAxis), remove valueToPixels from the deps, and then compute offset = valueToPixels(time - range.start) inside the markers.map render so valueToPixels identity changes don't invalidate the memo.electron/ipc/captions/whisper.ts (2)
42-104: No request/socket timeout — a stalled connection hangs the download forever.
httpsGetwithout atimeoutoption (and no correspondingreq.setTimeout/req.destroywiring) means a dead peer or half-open TCP connection will leave the promise unresolved indefinitely, leaving the UI stuck in"downloading"with no way to recover. Consider adding a socket timeout and surfacing it as an error so the caller can show a retry path.♻️ Suggested change
- const req = httpsGet(currentUrl, (response) => { + const req = httpsGet(currentUrl, { timeout: 30_000 }, (response) => { @@ req.on("error", reject); + req.on("timeout", () => { + req.destroy(new Error("Whisper model download timed out.")); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/captions/whisper.ts` around lines 42 - 104, The download logic using httpsGet in the request function lacks a socket timeout, so add a timeout on the outgoing request (use req.setTimeout or pass a timeout option to httpsGet) and wire a timeout handler that destroys the request/response and rejects the Promise with a clear error so the caller can recover; update the handlers around httpsGet/req (inside request) to call req.destroy()/response.destroy() on timeout, call fileStream.destroy() if needed, and ensure onProgress and resolve/reject behave correctly for timeout involving destinationPath and the request function.
106-142: Consider verifying the downloaded model.The final
.binis renamed into place with no size/checksum check. A truncated or corrupted download (e.g. proxy cut short a 2xx response without anerrorevent) will be persisted as a seemingly-valid model and then fail later insidewhisper-cliwith opaque errors. Comparingcontent-length(when present) againstdownloadedBytesbefore rename, or validating against a known SHA‑256 forggml-small.bin, would catch this early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/captions/whisper.ts` around lines 106 - 142, The download currently renames the temp file into WHISPER_SMALL_MODEL_PATH without verifying integrity; update downloadWhisperSmallModel to validate the downloaded file (tempPath) before renaming by either comparing the downloaded byte count against the HTTP Content-Length (when available from downloadFileWithProgress) or computing and comparing a known SHA-256 (e.g., add a WHISPER_SMALL_MODEL_SHA256 constant) of tempPath; if you use Content-Length, have downloadFileWithProgress expose the final downloadedBytes or response.contentLength, then after downloadFileWithProgress completes call fs.stat(tempPath).size and compare to contentLength, or compute a SHA-256 hash of tempPath and compare to WHISPER_SMALL_MODEL_SHA256; on mismatch, rm(tempPath), sendWhisperModelDownloadProgress with status "error" and an explanatory message, and throw to avoid renaming a truncated/corrupt file.electron/ipc/paths/binaries.ts (1)
208-211: Consider asyncexecFileforswiftccompilation.
spawnSync("swiftc", ...)blocks the Electron main process for up to 120 seconds during Swift helper compilation. On first launch (when the binary is absent or stale) this can freeze UI, block IPC, and stall other startup work. Since all callers (ensureNativeCaptureHelperBinary,ensureNativeWindowListBinary,ensureNativeCursorMonitorBinary) are alreadyasync, switching toexecFile/spawnwith a promise wrapper would keep the event loop responsive without changing semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/paths/binaries.ts` around lines 208 - 211, The sync call to spawnSync("swiftc", ...) blocks the Electron main process; change it to an asynchronous child process call (use child_process.execFile or spawn wrapped in a Promise) so compilation runs off the event loop. Replace the spawnSync invocation in the compilation helper with an async wrapper that executes "swiftc" with the same args, preserves encoding, enforces the same timeout, resolves on exit code 0 returning stdout/stderr, and rejects with a detailed error (including stderr) on non-zero exit or timeout; ensure callers ensureNativeCaptureHelperBinary, ensureNativeWindowListBinary, and ensureNativeCursorMonitorBinary keep their async signatures and await the Promise so behavior remains the same.electron/ipc/recording/prune.ts (2)
40-40:fs.readdirrejects withENOENTif the recordings dir hasn't been created yet.If
pruneAutoRecordingsruns before any recording has ever been saved (e.g., first‑launch startup hook), this will throw and propagate an unhandled rejection to the caller. Since the rest of the function is already best‑effort, guarding this with atry/catch or pre‑creating the directory viafs.mkdir(recordingsDir, { recursive: true })would make startup more resilient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/recording/prune.ts` at line 40, pruneAutoRecordings currently calls fs.readdir(recordingsDir) which throws ENOENT if the recordingsDir doesn't exist; make startup resilient by ensuring the directory exists first: before the call to fs.readdir in pruneAutoRecordings, call await fs.mkdir(recordingsDir, { recursive: true }) (or alternatively wrap fs.readdir in a try/catch that treats ENOENT as an empty directory) so that reading entries cannot reject due to a missing recordingsDir; keep using the existing recordingsDir and fs.readdir symbols and preserve the function's best‑effort behavior for other errors.
75-86: Suffix list drifts fromCOMPANION_AUDIO_LAYOUTS.The hard-coded
[".system.m4a", ".mic.m4a", ".system.wav", ".mic.wav", ".mic.webm", ".system.webm"]duplicates the source of truth now inelectron/ipc/constants.ts(COMPANION_AUDIO_LAYOUTS). Future additions there (e.g., a Linux layout) would silently fail to be pruned here. Deriving the list fromCOMPANION_AUDIO_LAYOUTSkeeps both in sync.♻️ Suggested change
import { AUTO_RECORDING_RETENTION_COUNT, AUTO_RECORDING_MAX_AGE_MS, PROJECT_FILE_EXTENSION, LEGACY_PROJECT_FILE_EXTENSIONS, + COMPANION_AUDIO_LAYOUTS, } from "../constants"; @@ - for (const suffix of [ - ".system.m4a", - ".mic.m4a", - ".system.wav", - ".mic.wav", - ".mic.webm", - ".system.webm", - ]) { + const companionSuffixes = Array.from( + new Set( + COMPANION_AUDIO_LAYOUTS.flatMap((layout) => [ + layout.systemSuffix, + layout.micSuffix, + ]), + ), + ); + for (const suffix of companionSuffixes) { await fs.rm(base + suffix, { force: true }).catch(() => undefined); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/recording/prune.ts` around lines 75 - 86, Replace the hard-coded companion suffix array with a derived list built from the COMPANION_AUDIO_LAYOUTS constant: import COMPANION_AUDIO_LAYOUTS and iterate its layout keys/names to produce suffixes like `.${layout}.m4a`, `.${layout}.wav`, `.${layout}.webm` (or whatever extensions the project supports), then use that derived array in the existing loop that computes `base = entry.filePath.replace(/\.(mp4|mov|webm)$/i, "")` and calls `await fs.rm(base + suffix, { force: true }).catch(() => undefined);` so prune.ts always stays in sync with COMPANION_AUDIO_LAYOUTS.electron/ipc/recording/windows.ts (1)
32-50: Remove deadhelperExistslocal (and thevoidworkaround).
helperExistsis only assigned after a successfulfs.access; the failure branch already returnsfalseat Line 45, so by the time execution reaches Line 48 the value is alwaystrueand never read. Thevoid helperExists;statement exists only to silencenoUnusedLocals. Drop the local entirely.Suggested refactor
const helperPath = getWindowsCaptureExePath(); const os = await import("node:os"); const [major, , build] = os.release().split(".").map(Number); const supported = major >= 10 && build >= 19041; - let helperExists = false; try { await fs.access(helperPath, fsConstants.X_OK); - helperExists = true; } catch { return false; } - void helperExists; return supported;Also consider short-circuiting when
!supportedto skip thefs.accessprobe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/recording/windows.ts` around lines 32 - 50, In isNativeWindowsCaptureAvailable(), remove the dead local helperExists and the void helperExists; workaround: compute supported from os.release() first and if !supported return false immediately, then perform await fs.access(getWindowsCaptureExePath(), fsConstants.X_OK) inside a try/catch that returns false on error; finally return true (or supported) after successful access — update references to getWindowsCaptureExePath, supported, and the fs.access try/catch accordingly.electron/ipc/recording/ffmpeg.ts (1)
162-189: Hoistfs/promisesimport out of the close handler.
await import("node:fs/promises")on every process close adds an unnecessary microtask and defeats normal module bundling/tree-shaking. Since other files in this module already import fromnode:fs/promisesat the top, move it here too.Suggested refactor
import type { ChildProcessWithoutNullStreams } from "node:child_process"; +import { access } from "node:fs/promises"; import type { SelectedSource } from "../types"; ... const onClose = async (code: number | null) => { cleanup(); try { - const { access } = await import("node:fs/promises"); await access(outputPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/recording/ffmpeg.ts` around lines 162 - 189, The dynamic import of "node:fs/promises" inside waitForFfmpegCaptureStop's onClose adds unnecessary microtasks; hoist the import to the module top and use the named export (access) directly in onClose. Modify the file to add `import { access } from "node:fs/promises"` at the top and replace `const { access } = await import("node:fs/promises"); await access(outputPath);` in the onClose handler with a direct call to `await access(outputPath);` while preserving the existing resolve/reject logic and references to waitForFfmpegCaptureStop and ffmpegCaptureOutputBuffer.electron/ipc/cursor/interaction.ts (1)
171-186:mousemovehandler is registered on non-Linux platforms despite being a no-op.
onMouseMoveearly-exits whenprocess.platform !== "linux", yethook.on("mousemove", onMouseMove)is always called. Given that mousemove fires very frequently on a native hook, skipping registration outside Linux avoids unnecessary cross-boundary callbacks.Suggested refactor
hook.on("mousedown", onMouseDown); hook.on("mouseup", onMouseUp); - hook.on("mousemove", onMouseMove); + if (process.platform === "linux") { + hook.on("mousemove", onMouseMove); + }And guard
off/removeListenerin the cleanup similarly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/cursor/interaction.ts` around lines 171 - 186, The onMouseMove handler is being registered unconditionally even though it early-exits on non-Linux, so change the registration to only call hook.on("mousemove", onMouseMove) when process.platform === "linux" (and optionally when isCursorCaptureActive if you prefer), and mirror that guard in the teardown by only calling hook.off("mousemove", onMouseMove) (or removeListener) under the same platform/activation condition; keep onMouseDown/onMouseUp registered as before and reference the existing onMouseMove, hook.on, hook.off/removeListener, isCursorCaptureActive and process.platform symbols when applying the change.electron/ipc/recording/diagnostics.ts (1)
35-68: Two near-duplicate FFmpegDuration:parsers with divergent accept ranges.
parseFfmpegDurationSeconds(Line 35) accepts any fractional digit count via\d+(?:\.\d+)?, whileprobeMediaDurationSeconds(Line 58) hand-rolls a stricter pattern that only accepts.ddor.ddd. They also diverge on error handling: the former returnsnull, the latter swallows and returns0. Callers can't distinguish "no output" from "zero-length media" viaprobeMediaDurationSeconds. Consider consolidating on a single helper (returningnumber | null) and letting callers decide the default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/recording/diagnostics.ts` around lines 35 - 68, probeMediaDurationSeconds duplicates and narrows the Duration parser and hides parse failures; update probeMediaDurationSeconds to reuse parseFfmpegDurationSeconds (which accepts any fractional digit count) instead of its bespoke regex, change probeMediaDurationSeconds to return number | null (propagate null when parseFfmpegDurationSeconds returns null) and stop coercing failures into 0, and ensure you still extract stderr from the execFileAsync error (using the same stderr handling already present) before calling parseFfmpegDurationSeconds.electron/ipc/project/manager.ts (1)
238-248: Hardcoded extension list diverges from constants.Line 241 hardcodes
/\.(recordly|openscreen)$/ifor basename stripping, whilehasProjectFileExtension(Line 157) correctly derives fromPROJECT_FILE_EXTENSION+LEGACY_PROJECT_FILE_EXTENSIONS. If a new legacy extension is added to the constants module this name will silently retain the extension suffix. Derive the regex from the shared constants.♻️ Proposed refactor
+const PROJECT_EXTENSIONS_PATTERN = new RegExp( + `\\.(${[PROJECT_FILE_EXTENSION, ...LEGACY_PROJECT_FILE_EXTENSIONS].join("|")})$`, + "i", +); ... - name: path.basename(normalizedPath).replace(/\.(recordly|openscreen)$/i, ""), + name: path.basename(normalizedPath).replace(PROJECT_EXTENSIONS_PATTERN, ""),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/project/manager.ts` around lines 238 - 248, The filename basename stripping currently uses a hardcoded regex /\.(recordly|openscreen)$/i; replace this with a dynamic regex built from the shared constants (PROJECT_FILE_EXTENSION and LEGACY_PROJECT_FILE_EXTENSIONS) so the logic matches whatever extensions hasProjectFileExtension uses. Locate the object creation that returns name (the code using path.basename(...).replace(...)), construct the extension pattern from PROJECT_FILE_EXTENSION plus LEGACY_PROJECT_FILE_EXTENSIONS, build a case-insensitive regex from that pattern, and use it in place of the hardcoded regex to strip the extension consistently with hasProjectFileExtension/normalizePath.electron/ipc/state.ts (1)
78-79: Array-typedexport let+ setter is a brittle pattern.
activeCursorSamples/pendingCursorSamplesare exported asletand also have setters that reassign them. Consumers that callactiveCursorSamples.push(...)(e.g. incursor/telemetry.ts) rely on the ES-module live binding, which works — but if any importer ever captures a local reference (const local = activeCursorSamples) they'll silently desync after asetActiveCursorSamples(...)call. Consider exporting a wrapper with explicitpush/clear/snapshotmethods, or keeping these asconstarrays and exposingclearActiveCursorSamples()/setPendingCursorSamples(arr => arr.splice(...))instead. Not blocking for this refactor PR, but worth tightening as a follow-up since several modules now depend on this binding behavior.Also applies to: 154-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/state.ts` around lines 78 - 79, activeCursorSamples and pendingCursorSamples are exported as mutable lets and reassigned by setters which breaks live-binding for any import that captured a local reference; replace the brittle pattern with a stable wrapper API: keep internal arrays private (e.g., _activeCursorSamples, _pendingCursorSamples) and export objects ActiveCursorSamples and PendingCursorSamples that expose push(item), clear(), snapshot(): CursorTelemetryPoint[], and replace any setActiveCursorSamples/setPendingCursorSamples implementations to mutate the internal arrays (splice/length=0/push) instead of reassigning; ensure cursor/telemetry.ts and other consumers call the new push/clear/snapshot methods (or adapt them to use snapshot() for reads) and apply the same change for the similar exports around the lines noted (the other pair at 154-155).electron/ipc/captions/generate.ts (1)
15-24: Description-driven branching inensureReadableFileis a code smell.
ensureReadableFilechanges behavior based on a human-readable string ("whisper executable"vs. any other value). A future caller that passes"Whisper Executable"or"whisper-executable"silently loses the executability check. Consider either splitting intoensureReadable/ensureExecutable, or taking an explicit{ executable?: boolean }option.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/captions/generate.ts` around lines 15 - 24, The function ensureReadableFile currently branches behavior based on the human string description which is brittle; change it to accept an explicit option or split responsibilities: either create two functions ensureReadable(filePath: string) and ensureExecutable(filePath: string) or modify ensureReadableFile to take a parameter object like ensureReadableFile(filePath: string, options?: {executable?: boolean}) and use options.executable to decide the X_OK check, updating all callers that passed "whisper executable" to call the executable-aware API (or call ensureExecutable) and removing the string-based branching in ensureReadableFile.
🤖 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/ipc/captions/generate.ts`:
- Around line 114-146: The catch block records attemptedCandidates with
readable: true even when ensureReadableFile(candidate.path, "video file") fails;
fix by tracking the readable check separately: before calling execFileAsync, set
a local flag (e.g., readableChecked = false), then after a successful
ensureReadableFile set readableChecked = true; in the catch, set readable to
readableChecked (or false if ensureReadableFile threw) and keep extractedAudio
false and include the error message; update the attemptedCandidates pushes and
the return logic around the candidates loop (refer to ensureReadableFile,
execFileAsync, attemptedCandidates, extractedAudio) so diagnostics accurately
reflect whether the file was readable vs audio extraction failure.
- Around line 206-235: When jsonEnabled is true, reading/parsing the JSON
sidecar must be defensive: wrap the fs.readFile(jsonPath, "utf-8") and
parseWhisperJsonCues call in its own try/catch so that any failure (ENOENT,
parse error, etc.) sets timedCues = [] (and optionally logs a warning) instead
of letting the error propagate; then fall back to
parseSrtCues(fs.readFile(srtPath, "utf-8")) as currently done. Modify the block
that computes timedCues/cues to catch errors around
parseWhisperJsonCues/jsonPath and ensure parseSrtCues/srtPath is consulted when
JSON is missing or unreadable.
In `@electron/ipc/cursor/bounds.ts`:
- Around line 21-55: getNativeMacWindowSources currently calls execFileAsync and
JSON.parse(stdout) without catching exceptions, which can surface unhandled
rejections to callers; wrap the execFileAsync and parse steps in a try/catch
inside getNativeMacWindowSources, log the error (using the same logger used in
this module) and return an empty array on any failure to preserve the existing
"return [] on error" contract; ensure you still set/refresh cache only on
success (i.e., call setCachedNativeMacWindowSources and
setCachedNativeMacWindowSourcesAtMs only when parsing succeeds) and keep
behavior consistent with resolveMacWindowBounds callers.
- Around line 199-210: The PowerShell call currently passes windowTitle unquoted
and trusts JSON.parse(stdout) as WindowBounds; fix by properly quoting/escaping
the windowTitle argument passed to execFileAsync (e.g., escape single quotes in
windowTitle and wrap it in single quotes so the PowerShell -like/-eq comparisons
won't break) and add a runtime type guard (e.g., an isWindowBounds(obj):
boolean) that verifies required properties (width, height, x, y) exist and are
finite numbers (and width/height > 0) before returning bounds; update the
execFileAsync invocation to use the escaped/quoted windowTitle and replace the
direct cast/conditional on bounds with the type guard check that returns the
validated WindowBounds or null.
In `@electron/ipc/cursor/interaction.ts`:
- Around line 184-218: Listeners are registered with hook.on(...) before calling
hook.start(), but the cleanup closure via setInteractionCaptureCleanup(...) is
only set after start() succeeds, so if hook.start() throws listeners leak; fix
by ensuring the cleanup is registered immediately after attaching listeners
(i.e., call setInteractionCaptureCleanup(...) right after hook.on("mousedown",
onMouseDown)/"mouseup"/"mousemove") so that any exception from hook.start()
still leaves a valid teardown, or alternatively wrap hook.start() in try/catch
and explicitly remove listeners in the catch using the same off/removeListener
logic found in the current cleanup closure (referencing hook.on, hook.start,
setInteractionCaptureCleanup, onMouseDown/onMouseUp/onMouseMove).
In `@electron/ipc/cursor/monitor.ts`:
- Around line 15-22: The function emitCursorStateChanged uses a runtime require
which fails in ESM; replace the dynamic require with a static top-level import
of BrowserWindow from "electron" (matching the pattern in
electron/ipc/recording/events.ts) and then use that imported BrowserWindow
inside emitCursorStateChanged to iterate windows and send the
"cursor-state-changed" message.
In `@electron/ipc/cursor/telemetry.ts`:
- Around line 118-125: The guard for !point in sampleCursorPoint is dead because
getNormalizedCursorPoint always returns a {cx, cy} object; remove the
unnecessary null-check to simplify flow: update sampleCursorPoint (which calls
getNormalizedCursorPoint and then pushCursorSample) to assume a valid point and
call pushCursorSample(point.cx, point.cy, Date.now() - cursorCaptureStartTimeMs,
"move") unconditionally, or alternatively change getNormalizedCursorPoint's
signature to return null on invalid inputs and make both window-bounds/display
branches return null so the existing guard becomes meaningful—choose one
approach and apply it consistently to sampleCursorPoint and
getNormalizedCursorPoint.
- Around line 34-62: The getNormalizedCursorPoint function currently divides
cached cursor and window coordinates by primarySf before locating the display,
which breaks mixed-DPI handling; change the flow so you first use unscaled
coordinates (do not divide by primarySf) to call getDisplayNearestPoint (or
otherwise determine the display containing the cursor/window), then read that
display.scaleFactor and use it to normalize the cursor and window bounds; update
logic around linuxCursorCache, primarySf usage, and the block that computes
sf/width/height (references: getNormalizedCursorPoint, linuxCursorCache,
primarySf, selectedWindowBounds, getDisplayNearestPoint) so the correct display
scaleFactor is applied to the coordinates used for clamping.
In `@electron/ipc/export/native-video.ts`:
- Around line 451-462: The cleanup list contains a stale entry for
`${tempVideoPath}.muxed.mp4` that is never produced by this module
(muxNativeVideoExportAudio writes `-final.mp4` into path.dirname(videoPath));
remove the `${tempVideoPath}.muxed.mp4` element from the Promise.allSettled call
in the finally block (where removeTemporaryExportFile is invoked) so only actual
produced paths are cleaned, or if you intentionally expect a producer to create
`${tempVideoPath}.muxed.mp4`, add/restore that producer and a clear comment
linking it to removeTemporaryExportFile to make the contract explicit.
- Around line 325-350: The cache for native encoders currently keys only by
ffmpegPath causing resolveNativeVideoEncoder to return an encoder that may not
be valid for a different encodingMode; update the cachedNativeVideoEncoder shape
(in state.ts) and all uses so the cache key includes encodingMode, change
resolveNativeVideoEncoder to check cachedNativeVideoEncoder.ffmpegPath and
cachedNativeVideoEncoder.encodingMode before returning, and adjust
setCachedNativeVideoEncoder to store the mode; optionally extend the cache key
to incorporate ffmpeg version/mtime (probe or stat ffmpeg) so changes to the
binary invalidate the cache.
In `@electron/ipc/ffmpeg/binary.ts`:
- Around line 8-19: The unguarded nodeRequire call inside loadFfmpegStatic can
throw and prevent getFfmpegBinaryPath from falling back to
resolveSystemFfmpegBinaryPath; wrap the require in a try/catch inside
loadFfmpegStatic (or check module existence safely) so any synchronous error is
caught and the function returns null on failure, ensuring getFfmpegBinaryPath
can proceed to the resolveSystemFfmpegBinaryPath fallback; reference
loadFfmpegStatic, nodeRequire, getFfmpegBinaryPath, and
resolveSystemFfmpegBinaryPath when making the change.
In `@electron/ipc/project/manager.ts`:
- Around line 294-336: loadProjectFromPath mutates shared module state across
awaits which can leave partial state if a later await fails; fix by computing
all values locally first (keep local normalizedPath, mediaSources,
approvedProjectPaths, and recordingSession object) and only call the mutating
functions setCurrentProjectPath, setCurrentVideoPath,
replaceApprovedSessionLocalReadPaths, setCurrentRecordingSession, and
rememberRecentProject in one synchronous block at the end (or alternatively
protect the whole load with a module-level mutex and perform mutations inside a
try/catch that rolls back to previous values on error); ensure you gather
audioTracks and build approvedProjectPaths before calling
replaceApprovedSessionLocalReadPaths so replaceApprovedSessionLocalReadPaths
runs after all async resolution and all state changes happen atomically.
- Around line 43-49: The function isPathInsideDirectory currently only
normalizes directoryPath but compares candidatePath verbatim; update
isPathInsideDirectory to first normalize candidatePath (e.g., const
normalizedCandidatePath = normalizePath(candidatePath)) and then use
normalizedCandidatePath in the equality and startsWith checks (keep the existing
normalizedDirectoryPath and `${normalizedDirectoryPath}${path.sep}` logic); this
ensures mixed separators and relative segments in candidatePath cannot bypass
the check.
- Around line 51-59: The current isAllowedLocalReadPath wrongly allows any
existing file (existsSync(candidatePath)) which bypasses the allowlist; change
the logic so the path must exist AND be either inside an allowed prefix or
explicitly approved: return existsSync(candidatePath) &&
(allowedPrefixes.some(prefix => isPathInsideDirectory(candidatePath, prefix)) ||
approvedLocalReadPaths.has(candidatePath)). Also harden isPathInsideDirectory to
normalize both inputs (or normalize candidatePath before calling) so comparisons
are safe; update uses of RECORDINGS_DIR, USER_DATA_PATH, getAssetRootPath(),
app.getPath("temp"), approvedLocalReadPaths, and existsSync accordingly to
reflect these checks.
In `@electron/ipc/project/session.ts`:
- Around line 64-70: The early return for the no-webcam case hardcodes
timeOffsetMs: 0 which drops a valid offset from the parsed manifest; update the
return to compute timeOffsetMs using
normalizeRecordingTimeOffsetMs(parsed.timeOffsetMs) (same as the webcam branch)
so the returned object with videoPath: normalizedVideoPath, webcamPath: null,
and timeOffsetMs preserves the manifest offset; locate the no-webcam branch that
checks webcamFileName and replace the hardcoded 0 with the normalized value.
In `@electron/ipc/recording/ffmpeg.ts`:
- Around line 33-54: The ffmpeg gdigrab window capture builds the input string
with `title=${windowTitle}` (see the block guarded by
`source?.id?.startsWith("window:")` that computes `windowTitle` and returns the
args array), but special characters like colon break FFmpeg parsing; update the
returned input argument to wrap the title value in single quotes and escape
colons (e.g., replace ":" with "\:") before interpolation so the element passed
to `-i` becomes `title='escapedTitle'`; modify the code that constructs the
`"-i", \`title=${windowTitle}\`` entry to use the escaped and quoted
`windowTitle` instead.
- Around line 111-124: The macOS branch in the ffmpeg argument builder hardcodes
the avfoundation device "1:none" (in the function that returns the args for
macOS), which is environment-specific and ignores the provided source parameter;
update that branch to use the source.display_id when present (or translate it to
the correct avfoundation index) instead of the hardcoded "1", and as a fallback
query available avfoundation devices (e.g., via ffmpeg -f avfoundation
-list_devices true -i "") and match by device name to determine the correct
index; change the code that currently returns
["-y","-f","avfoundation","-capture_cursor","0","-framerate","60","-i","1:none",...commonOutputArgs]
to compute the correct "{index}:none" using source.display_id or the device
lookup and return that value.
In `@electron/ipc/recording/mac.ts`:
- Around line 55-61: The onStdout handler in mac.ts can miss the "Recording
started" marker when it is split across chunks; update the onStdout
implementation used by waitForMacCaptureStart to append each chunk to a rolling
buffer (or reuse the existing nativeCaptureOutputBuffer), then test
buffer.includes("Recording started") and only cleanup/resolve when the
aggregated buffer contains the marker; ensure the buffer is trimmed to a
reasonable max length to avoid unbounded growth. Do the analogous change for
waitForWindowsCaptureStart in windows.ts (use windowsCaptureOutputBuffer or a
rolling buffer) so both handlers reliably detect the marker across chunk
boundaries.
In `@electron/ipc/utils.ts`:
- Around line 82-105: The cached flag recordingsDirLoaded can become stale when
the settings file is written by handlers other than
persistRecordingsDirectorySetting; update writers to either clear the cache or
use the central updater: ensure any code that writes RECORDINGS_SETTINGS_FILE
(e.g. the IPC handler set-recording-preferences or any future writers) calls
setRecordingsDirLoaded(false) before writing or, better, route all writes
through persistRecordingsDirectorySetting which atomically calls
setCustomRecordingsDir(...) and setRecordingsDirLoaded(true) so
loadRecordingsDirectorySetting() and getRecordingsDir() always see current
state.
---
Outside diff comments:
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 1780-1804: Trim/speed handlers and shortcuts are now dead because
trim/speed regions are no longer rendered into timelineItems/allRegionSpans;
remove the orphaned API surface instead of leaving silent no-ops: delete or
unhook handleAddTrim, handleAddSpeed, deleteSelectedTrim, deleteSelectedSpeed,
the keyboard bindings for keyShortcuts.addTrim and keyShortcuts.addSpeed, and
any props and callbacks related to trims/speeds (onTrimAdded, onSpeedAdded,
onTrimDelete, onSpeedDelete, selectedTrimId, selectedSpeedId), and remove the
isTrimItem/isSpeedItem branches in hasOverlap so it no longer checks unreachable
item types; ensure any useEffect that maps clipRegions→trimRegions or code that
overwrites user-added trims is removed or adjusted so there are no leftover
effects that recreate invisible regions.
- Around line 501-531: The marker centering uses a fixed transform:
"translateX(-50%)" which miscenters when sideProperty is "right" (RTL); update
the markerStyle in the markers.markers.map block to compute the translateX sign
based on direction (e.g., derive isRtl from sideProperty === "right" or from a
provided dir prop) and set transform to `translateX(-50%)` for LTR and
`translateX(50%)` for RTL (or use `${isRtl ? "translateX(50%)" :
"translateX(-50%)"}`), keeping the rest of markerStyle and the use of
marker.time as key unchanged so markers are centered correctly in both LTR and
RTL.
---
Nitpick comments:
In `@electron/ipc/captions/generate.ts`:
- Around line 15-24: The function ensureReadableFile currently branches behavior
based on the human string description which is brittle; change it to accept an
explicit option or split responsibilities: either create two functions
ensureReadable(filePath: string) and ensureExecutable(filePath: string) or
modify ensureReadableFile to take a parameter object like
ensureReadableFile(filePath: string, options?: {executable?: boolean}) and use
options.executable to decide the X_OK check, updating all callers that passed
"whisper executable" to call the executable-aware API (or call ensureExecutable)
and removing the string-based branching in ensureReadableFile.
In `@electron/ipc/captions/whisper.ts`:
- Around line 42-104: The download logic using httpsGet in the request function
lacks a socket timeout, so add a timeout on the outgoing request (use
req.setTimeout or pass a timeout option to httpsGet) and wire a timeout handler
that destroys the request/response and rejects the Promise with a clear error so
the caller can recover; update the handlers around httpsGet/req (inside request)
to call req.destroy()/response.destroy() on timeout, call fileStream.destroy()
if needed, and ensure onProgress and resolve/reject behave correctly for timeout
involving destinationPath and the request function.
- Around line 106-142: The download currently renames the temp file into
WHISPER_SMALL_MODEL_PATH without verifying integrity; update
downloadWhisperSmallModel to validate the downloaded file (tempPath) before
renaming by either comparing the downloaded byte count against the HTTP
Content-Length (when available from downloadFileWithProgress) or computing and
comparing a known SHA-256 (e.g., add a WHISPER_SMALL_MODEL_SHA256 constant) of
tempPath; if you use Content-Length, have downloadFileWithProgress expose the
final downloadedBytes or response.contentLength, then after
downloadFileWithProgress completes call fs.stat(tempPath).size and compare to
contentLength, or compute a SHA-256 hash of tempPath and compare to
WHISPER_SMALL_MODEL_SHA256; on mismatch, rm(tempPath),
sendWhisperModelDownloadProgress with status "error" and an explanatory message,
and throw to avoid renaming a truncated/corrupt file.
In `@electron/ipc/cursor/interaction.ts`:
- Around line 171-186: The onMouseMove handler is being registered
unconditionally even though it early-exits on non-Linux, so change the
registration to only call hook.on("mousemove", onMouseMove) when
process.platform === "linux" (and optionally when isCursorCaptureActive if you
prefer), and mirror that guard in the teardown by only calling
hook.off("mousemove", onMouseMove) (or removeListener) under the same
platform/activation condition; keep onMouseDown/onMouseUp registered as before
and reference the existing onMouseMove, hook.on, hook.off/removeListener,
isCursorCaptureActive and process.platform symbols when applying the change.
In `@electron/ipc/paths/binaries.ts`:
- Around line 208-211: The sync call to spawnSync("swiftc", ...) blocks the
Electron main process; change it to an asynchronous child process call (use
child_process.execFile or spawn wrapped in a Promise) so compilation runs off
the event loop. Replace the spawnSync invocation in the compilation helper with
an async wrapper that executes "swiftc" with the same args, preserves encoding,
enforces the same timeout, resolves on exit code 0 returning stdout/stderr, and
rejects with a detailed error (including stderr) on non-zero exit or timeout;
ensure callers ensureNativeCaptureHelperBinary, ensureNativeWindowListBinary,
and ensureNativeCursorMonitorBinary keep their async signatures and await the
Promise so behavior remains the same.
In `@electron/ipc/project/manager.ts`:
- Around line 238-248: The filename basename stripping currently uses a
hardcoded regex /\.(recordly|openscreen)$/i; replace this with a dynamic regex
built from the shared constants (PROJECT_FILE_EXTENSION and
LEGACY_PROJECT_FILE_EXTENSIONS) so the logic matches whatever extensions
hasProjectFileExtension uses. Locate the object creation that returns name (the
code using path.basename(...).replace(...)), construct the extension pattern
from PROJECT_FILE_EXTENSION plus LEGACY_PROJECT_FILE_EXTENSIONS, build a
case-insensitive regex from that pattern, and use it in place of the hardcoded
regex to strip the extension consistently with
hasProjectFileExtension/normalizePath.
In `@electron/ipc/recording/diagnostics.ts`:
- Around line 35-68: probeMediaDurationSeconds duplicates and narrows the
Duration parser and hides parse failures; update probeMediaDurationSeconds to
reuse parseFfmpegDurationSeconds (which accepts any fractional digit count)
instead of its bespoke regex, change probeMediaDurationSeconds to return number
| null (propagate null when parseFfmpegDurationSeconds returns null) and stop
coercing failures into 0, and ensure you still extract stderr from the
execFileAsync error (using the same stderr handling already present) before
calling parseFfmpegDurationSeconds.
In `@electron/ipc/recording/ffmpeg.ts`:
- Around line 162-189: The dynamic import of "node:fs/promises" inside
waitForFfmpegCaptureStop's onClose adds unnecessary microtasks; hoist the import
to the module top and use the named export (access) directly in onClose. Modify
the file to add `import { access } from "node:fs/promises"` at the top and
replace `const { access } = await import("node:fs/promises"); await
access(outputPath);` in the onClose handler with a direct call to `await
access(outputPath);` while preserving the existing resolve/reject logic and
references to waitForFfmpegCaptureStop and ffmpegCaptureOutputBuffer.
In `@electron/ipc/recording/prune.ts`:
- Line 40: pruneAutoRecordings currently calls fs.readdir(recordingsDir) which
throws ENOENT if the recordingsDir doesn't exist; make startup resilient by
ensuring the directory exists first: before the call to fs.readdir in
pruneAutoRecordings, call await fs.mkdir(recordingsDir, { recursive: true }) (or
alternatively wrap fs.readdir in a try/catch that treats ENOENT as an empty
directory) so that reading entries cannot reject due to a missing recordingsDir;
keep using the existing recordingsDir and fs.readdir symbols and preserve the
function's best‑effort behavior for other errors.
- Around line 75-86: Replace the hard-coded companion suffix array with a
derived list built from the COMPANION_AUDIO_LAYOUTS constant: import
COMPANION_AUDIO_LAYOUTS and iterate its layout keys/names to produce suffixes
like `.${layout}.m4a`, `.${layout}.wav`, `.${layout}.webm` (or whatever
extensions the project supports), then use that derived array in the existing
loop that computes `base = entry.filePath.replace(/\.(mp4|mov|webm)$/i, "")` and
calls `await fs.rm(base + suffix, { force: true }).catch(() => undefined);` so
prune.ts always stays in sync with COMPANION_AUDIO_LAYOUTS.
In `@electron/ipc/recording/windows.ts`:
- Around line 32-50: In isNativeWindowsCaptureAvailable(), remove the dead local
helperExists and the void helperExists; workaround: compute supported from
os.release() first and if !supported return false immediately, then perform
await fs.access(getWindowsCaptureExePath(), fsConstants.X_OK) inside a try/catch
that returns false on error; finally return true (or supported) after successful
access — update references to getWindowsCaptureExePath, supported, and the
fs.access try/catch accordingly.
In `@electron/ipc/state.ts`:
- Around line 78-79: activeCursorSamples and pendingCursorSamples are exported
as mutable lets and reassigned by setters which breaks live-binding for any
import that captured a local reference; replace the brittle pattern with a
stable wrapper API: keep internal arrays private (e.g., _activeCursorSamples,
_pendingCursorSamples) and export objects ActiveCursorSamples and
PendingCursorSamples that expose push(item), clear(), snapshot():
CursorTelemetryPoint[], and replace any
setActiveCursorSamples/setPendingCursorSamples implementations to mutate the
internal arrays (splice/length=0/push) instead of reassigning; ensure
cursor/telemetry.ts and other consumers call the new push/clear/snapshot methods
(or adapt them to use snapshot() for reads) and apply the same change for the
similar exports around the lines noted (the other pair at 154-155).
In `@src/components/video-editor/timeline/Item.tsx`:
- Around line 136-145: The inline style attributes on the two resize handle divs
inside the Item component are redundant; remove the style={{ cursor:
"col-resize", pointerEvents: "auto" }} props from both divs so styling is
controlled by the CSS module. Locate the two elements that use
className={cn(glassStyles.zoomEndCap, glassStyles.left)} and
className={cn(glassStyles.zoomEndCap, glassStyles.right)} in Item.tsx and delete
their inline style props, leaving the className and title props intact to rely
on ItemGlass.module.css (zoomEndCap/.left/.right) for cursor and pointer-events.
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Around line 536-580: The markers useMemo in ClipMarkerOverlay currently calls
valueToPixels and includes it in the dependency array, causing unnecessary
re-runs; change the memo to only compute and return times (e.g., result: { time:
number }[]) based on intervalMs, range.start, range.end, and videoDurationMs
(matching TimelineAxis), remove valueToPixels from the deps, and then compute
offset = valueToPixels(time - range.start) inside the markers.map render so
valueToPixels identity changes don't invalidate the memo.
🪄 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: be2c70a8-dcce-403e-ae07-2d4427ccb944
📒 Files selected for processing (27)
electron/ipc/captions/generate.tselectron/ipc/captions/parser.tselectron/ipc/captions/whisper.tselectron/ipc/constants.tselectron/ipc/cursor/bounds.tselectron/ipc/cursor/interaction.tselectron/ipc/cursor/monitor.tselectron/ipc/cursor/telemetry.tselectron/ipc/export/native-video.tselectron/ipc/ffmpeg/binary.tselectron/ipc/ffmpeg/filters.tselectron/ipc/handlers.tselectron/ipc/paths/binaries.tselectron/ipc/project/manager.tselectron/ipc/project/session.tselectron/ipc/recording/diagnostics.tselectron/ipc/recording/events.tselectron/ipc/recording/ffmpeg.tselectron/ipc/recording/mac.tselectron/ipc/recording/prune.tselectron/ipc/recording/windows.tselectron/ipc/state.tselectron/ipc/types.tselectron/ipc/utils.tssrc/components/video-editor/timeline/Item.tsxsrc/components/video-editor/timeline/ItemGlass.module.csssrc/components/video-editor/timeline/TimelineEditor.tsx
handlers.ts reduced from 2930 → 65 lines (pure delegation). New files under electron/ipc/register/: - sources.ts — get-sources, select-source, show-source-highlight, open-source-selector - recording.ts — start/stop/pause native + ffmpeg, mux, store, set-recording-state, get-cursor-telemetry - permissions.ts — accessibility/screen permissions, open-external-url - assets.ts — wallpaper thumbnails, asset-base-path, list-asset-dir, read-local-file - export.ts — native-video-export-*, save-exported-video - captions.ts — whisper model, file pickers, generate-auto-captions - project.ts — project files, recordings dir, video/session state, delete-recording - settings.ts — shortcuts, recording prefs, countdown, platform info Also moved shared helpers: - getMacPrivacySettingsUrl, approveUserPath → utils.ts - isTrustedProjectPath → project/manager.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
electron/ipc/register/permissions.ts (1)
1-87: LGTM — clean extraction of permission handlers.Module boundaries look appropriate: URL protocol allowlist is preserved, platform guards for
darwinare consistent across handlers, and all failure paths return structured{ success, error }results with logging. Matches the stated pure-refactor intent.One minor, optional nit (pre-existing behavior, safe to defer): on Line 12 you pass the original
urlstring toshell.openExternalrather thanparsed.href. Usingparsed.hrefwould ensure the exact validated/normalized form is what actually gets opened, eliminating any theoretical divergence between what was validated and what is launched.Optional tightening
- await shell.openExternal(url) + await shell.openExternal(parsed.href)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/register/permissions.ts` around lines 1 - 87, The open-external-url handler in registerPermissionHandlers validates the input as a URL (const parsed = new URL(url)) but then calls shell.openExternal(url); change this to call shell.openExternal(parsed.href) so the normalized/validated URL is what gets opened; update the handler where shell.openExternal is invoked (inside the ipcMain.handle('open-external-url') block) to use parsed.href and keep the existing try/catch and return shapes.electron/ipc/register/export.ts (1)
254-256: Brittle finalized-path derivation in the failure branch.
session.outputPath.replace(/\.mp4$/, '-final.mp4')hard-codes knowledge of the naming convention used bymuxNativeVideoExportAudio. If that helper ever changes its suffix (e.g.,-muxed.mp4) or returns an out-of-temp path, the failure-path cleanup silently leaks files. Consider havingmuxNativeVideoExportAudioexpose the derived path (or a cleanup helper) and reuse it here, so the two sides cannot drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/register/export.ts` around lines 254 - 256, The cleanup currently derives the finalized path with session.outputPath.replace(/\.mp4$/, '-final.mp4'), which duplicates muxNativeVideoExportAudio's naming logic and is brittle; change muxNativeVideoExportAudio to return (or expose via a helper) the finalized output path (or a dedicated cleanup function) and use that returned path here instead of recomputing it so removeTemporaryExportFile is called on the exact path produced by muxNativeVideoExportAudio (or invoke the exported cleanup helper) to avoid leaking files if the suffix/logic changes.electron/ipc/register/captions.ts (1)
36-43: Inconsistent error response shape across handlers.The video/audio pickers return
{ success: false, message, error }, while the whisper executable/model pickers and the whisper lifecycle handlers return{ success: false, error }(nomessage), andget-whisper-small-model-statusreturns yet another shape ({ success: false, exists: false, path: null, error }). If this mirrors the pre-refactor behavior exactly, it's fine to leave for now; otherwise consider normalizing so the renderer can handle errors uniformly. No action required if intentional.Also applies to: 66-73, 93-96, 116-119, 144-147, 159-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/register/captions.ts` around lines 36 - 43, Handlers in captions.ts return inconsistent error shapes; standardize all catch returns (including openVideoFilePicker, openAudioFilePicker, getWhisperExecutable/getWhisperModel handlers, installWhisper/uninstallWhisper lifecycle handlers, and get-whisper-small-model-status) to the same shape (e.g., { success: false, message: string, error: string }); implement or call a small helper to produce this object (accepting an Error and optional message) and replace the differing return objects in the catch blocks so the renderer always receives a uniform error response.electron/ipc/project/manager.ts (1)
241-241: Hardcoded project extensions drift fromPROJECT_FILE_EXTENSION/LEGACY_PROJECT_FILE_EXTENSIONS.The inline regex
/\.(recordly|openscreen)$/iduplicates the constants imported on Lines 8–9. A future extension rename/add inconstants.tswill silently desync the display name. Prefer deriving from the constants, e.g.:♻️ Proposed refactor
- name: path.basename(normalizedPath).replace(/\.(recordly|openscreen)$/i, ""), + name: path.basename( + normalizedPath, + path.extname(normalizedPath), + ),Or build a regex from
[PROJECT_FILE_EXTENSION, ...LEGACY_PROJECT_FILE_EXTENSIONS].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/project/manager.ts` at line 241, The display name is being derived with a hardcoded regex (/\\.(recordly|openscreen)$/i) which duplicates the constants PROJECT_FILE_EXTENSION and LEGACY_PROJECT_FILE_EXTENSIONS; update the logic that sets the `name` (the path.basename(normalizedPath) .replace(...) call) to build a regex from PROJECT_FILE_EXTENSION and the entries in LEGACY_PROJECT_FILE_EXTENSIONS (e.g., join them into an alternation, escape as needed, and use a case-insensitive regex) and then use that regex to strip the extension so the display name always reflects the canonical constants.electron/ipc/register/project.ts (1)
290-312: Handler parameterpathshadows thenode:pathimport.The
path: stringparameter hides thepathmodule imported on Line 3 for the entire handler body. It's not breaking today (nopath.*call in this block), but any future edit that reaches forpath.basename(...)orpath.resolve(...)will silently receive astringand fail at runtime/compile in a confusing way. Rename for safety:♻️ Proposed refactor
- ipcMain.handle('set-current-video-path', async (_, path: string) => { - setCurrentVideoPath(normalizeVideoSourcePath(path) ?? path) + ipcMain.handle('set-current-video-path', async (_, videoPath: string) => { + setCurrentVideoPath(normalizeVideoSourcePath(videoPath) ?? videoPath) approveUserPath(currentVideoPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/register/project.ts` around lines 290 - 312, The handler for ipcMain.handle('set-current-video-path') uses a parameter named path which shadows the node:path import; rename the parameter (e.g., videoPath or inputPath) and update all uses inside the handler (normalizeVideoSourcePath(...), setCurrentVideoPath(...), resolveRecordingSession(...), and any other references) to the new name so the node:path module remains available and no shadowing occurs.
🤖 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/ipc/register/assets.ts`:
- Around line 106-121: The handler registered with
ipcMain.handle('read-local-file') currently uses fs.readFile and buffers the
entire file into main-process memory; update the implementation to first call
fs.stat (or fs.promises.stat) on the resolved path (use normalizePath and
realResolved checks as already present) and enforce a MAX_READ_BYTES ceiling
(define a constant like MAX_READ_BYTES) returning { success: false, error: 'File
too large' } if size exceeds it; for allowed files under the limit, either read
with fs.createReadStream and stream chunks to the renderer or continue using
fs.readFile for small files—do not remove the existing path-allow checks
(isAllowedLocalReadPath) and ensure errors still return via the same error
shape.
In `@electron/ipc/register/project.ts`:
- Around line 352-369: The delete-recording-file IPC handler must resolve and
validate the actual file target before unlinking: use fs.realpath on filePath
(or via approveUserPath if available) and ensure the resolved path is contained
within getRecordingsDir() (reject if not), then verify the basename with
isAutoRecordingPath() on the resolved name before calling fs.unlink; also
compute the telemetry path from the resolved video path via
getTelemetryPathForVideo and handle currentVideoPath/setCurrentVideoPath and
setCurrentRecordingSession as before, returning errors for any validation
failures.
In `@electron/ipc/register/recording.ts`:
- Around line 1040-1058: The IPC handler 'get-recorded-video-path' currently
filters only by extension and sorts filenames lexicographically, so replace that
logic in the handler to: list files from getRecordingsDir(), filter using the
auto-recording filename pattern (e.g. match the project's auto-recording naming
convention like /^recording-\\d+/ plus the existing extensions) to exclude
user-dropped/exports, then stat each candidate (fs.stat or fs.promises.stat) to
read mtimeMs and pick the file with the largest mtimeMs as the latest recording;
return its path as before and keep the existing error handling. Ensure you
reference getRecordingsDir, the IPC handler name 'get-recorded-video-path', and
use fs.stat/fs.promises.stat to determine mtimeMs.
In `@electron/ipc/register/sources.ts`:
- Around line 317-352: The computed bounds (variable bounds) can still be null
or zero-area after trying
resolveMacWindowBounds/resolveWindowsWindowBounds/resolveLinuxWindowBounds and
getDisplayBoundsForSource, so before constructing the BrowserWindow
(highlightWin) ensure bounds is a valid rectangle: if bounds is null or
width/height <= 0, attempt a firm fallback by querying the primary display
bounds (e.g., screen.getPrimaryDisplay().bounds) and assign that; if that also
fails, return/abort the operation (or return a { success: false } result)
instead of proceeding to new BrowserWindow, so references to
bounds.x/bounds.width cannot throw. Ensure this check is placed after the
existing fallback call to getDisplayBoundsForSource(source) and before creating
highlightWin.
- Around line 285-298: The code calls execFileAsync("osascript", ["-e", `tell
application "${appName}" to activate`], ...) using renderer-supplied
source.appName which allows AppleScript injection; fix by
validating/whitelisting source.appName before interpolation (e.g., allow only
characters matching a strict safe regex like /^[\w .&()+'-]{1,64}$/) or, safer,
avoid direct interpolation and call execFileAsync with a script that receives
the app name as an argv parameter (use osascript with an on run argv / item 1 of
argv pattern) so execFileAsync and the osascript binding treat the app name as
data rather than source; apply this check/escape where isWindow &&
process.platform === "darwin" before calling execFileAsync in the same block
that references source.appName / source.name.
---
Nitpick comments:
In `@electron/ipc/project/manager.ts`:
- Line 241: The display name is being derived with a hardcoded regex
(/\\.(recordly|openscreen)$/i) which duplicates the constants
PROJECT_FILE_EXTENSION and LEGACY_PROJECT_FILE_EXTENSIONS; update the logic that
sets the `name` (the path.basename(normalizedPath) .replace(...) call) to build
a regex from PROJECT_FILE_EXTENSION and the entries in
LEGACY_PROJECT_FILE_EXTENSIONS (e.g., join them into an alternation, escape as
needed, and use a case-insensitive regex) and then use that regex to strip the
extension so the display name always reflects the canonical constants.
In `@electron/ipc/register/captions.ts`:
- Around line 36-43: Handlers in captions.ts return inconsistent error shapes;
standardize all catch returns (including openVideoFilePicker,
openAudioFilePicker, getWhisperExecutable/getWhisperModel handlers,
installWhisper/uninstallWhisper lifecycle handlers, and
get-whisper-small-model-status) to the same shape (e.g., { success: false,
message: string, error: string }); implement or call a small helper to produce
this object (accepting an Error and optional message) and replace the differing
return objects in the catch blocks so the renderer always receives a uniform
error response.
In `@electron/ipc/register/export.ts`:
- Around line 254-256: The cleanup currently derives the finalized path with
session.outputPath.replace(/\.mp4$/, '-final.mp4'), which duplicates
muxNativeVideoExportAudio's naming logic and is brittle; change
muxNativeVideoExportAudio to return (or expose via a helper) the finalized
output path (or a dedicated cleanup function) and use that returned path here
instead of recomputing it so removeTemporaryExportFile is called on the exact
path produced by muxNativeVideoExportAudio (or invoke the exported cleanup
helper) to avoid leaking files if the suffix/logic changes.
In `@electron/ipc/register/permissions.ts`:
- Around line 1-87: The open-external-url handler in registerPermissionHandlers
validates the input as a URL (const parsed = new URL(url)) but then calls
shell.openExternal(url); change this to call shell.openExternal(parsed.href) so
the normalized/validated URL is what gets opened; update the handler where
shell.openExternal is invoked (inside the ipcMain.handle('open-external-url')
block) to use parsed.href and keep the existing try/catch and return shapes.
In `@electron/ipc/register/project.ts`:
- Around line 290-312: The handler for ipcMain.handle('set-current-video-path')
uses a parameter named path which shadows the node:path import; rename the
parameter (e.g., videoPath or inputPath) and update all uses inside the handler
(normalizeVideoSourcePath(...), setCurrentVideoPath(...),
resolveRecordingSession(...), and any other references) to the new name so the
node:path module remains available and no shadowing occurs.
🪄 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: e7ef365c-b275-4879-b8b2-74733deed769
📒 Files selected for processing (11)
electron/ipc/handlers.tselectron/ipc/project/manager.tselectron/ipc/register/assets.tselectron/ipc/register/captions.tselectron/ipc/register/export.tselectron/ipc/register/permissions.tselectron/ipc/register/project.tselectron/ipc/register/recording.tselectron/ipc/register/settings.tselectron/ipc/register/sources.tselectron/ipc/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/ipc/utils.ts
- Remove dead helperExists local in recording/windows.ts
- Hoist fs/promises import out of close handler in recording/ffmpeg.ts
- Guard fs.readdir with mkdir in recording/prune.ts (ENOENT resilience)
- Derive companion audio suffixes from COMPANION_AUDIO_LAYOUTS in prune.ts
- Guard mousemove hook registration to Linux only in cursor/interaction.ts
- Replace dynamic require('electron') with static import in cursor/monitor.ts
- Wrap nodeRequire in try/catch in ffmpeg/binary.ts for fallback safety
- Fix hardcoded timeOffsetMs: 0 in project/session.ts (use normalizer)
- Fix isPathInsideDirectory to normalize candidatePath in project/manager.ts
- Fix isAllowedLocalReadPath security: require path to be in allowlist (AND not OR)
- Derive extension regex from constants in project/manager.ts
- Consolidate duplicate Duration parsers in recording/diagnostics.ts
- Refactor ensureReadableFile to use options object instead of description string
- Make swiftc compilation async (execFile) in paths/binaries.ts
- Add socket timeout to httpsGet in captions/whisper.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
electron/ipc/recording/prune.ts (1)
42-51:Promise.alloffs.stataborts pruning on a single transient failure.If any recording file disappears between
readdirandfs.stat(concurrent deletion, renaming, unmount, permission hiccup, etc.), the wholePromise.allrejects andpruneAutoRecordingsthrows, skipping all other eligible prunes. Since the per-deletiontry/catchbelow already embraces best-effort semantics, it would be more consistent to tolerate per-entry stat failures too — e.g. viaPromise.allSettledor a try/catch inside themapcallback that returnsnulland is filtered out.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/recording/prune.ts` around lines 42 - 51, pruneAutoRecordings currently uses Promise.all over fs.stat for each entry so a single transient fs.stat failure aborts the whole prune; change the entries.map logic to tolerate per-entry stat errors by either using Promise.allSettled on the array of stat promises or wrapping the await fs.stat call in a try/catch inside the map (returning null on failure) and then filter out null results before proceeding, ensuring you still reference recordingsDir, entries, and the autoRecordingStats variable when updating the subsequent processing logic.electron/ipc/captions/generate.ts (1)
53-71: SynchronousspawnSyncblocks the main-process event loop.
resolveWhisperExecutablePathis awaited on the IPC handler path forgenerate-auto-captions. Shelling out synchronously towhich/where(potentially up to 4 times on non-Windows) blocks the Electron main process — freezing all IPC, renderer communications, and window events until each probe returns. Even though these lookups are normally fast, a slow PATH entry (stale network mount, etc.) can stall the whole app. Switch to the already-available asyncexecFileAsyncso this stays non-blocking.♻️ Proposed refactor
- const pathCommand = process.platform === "win32" ? "where" : "which"; + const pathCommand = process.platform === "win32" ? "where" : "which"; const binaryNames = process.platform === "win32" ? ["whisper-cli.exe", "whisper.exe", "main.exe"] : ["whisper-cli", "whisper-cpp", "whisper", "main"]; for (const binaryName of binaryNames) { - const result = spawnSync(pathCommand, [binaryName], { encoding: "utf-8" }); - if (result.status === 0) { - const resolvedPath = result.stdout + try { + const { stdout } = await execFileAsync(pathCommand, [binaryName], { + encoding: "utf-8", + }); + const resolvedPath = stdout .split(/\r?\n/) .map((line) => line.trim()) .find(Boolean); - if (resolvedPath && (await isExecutableFile(resolvedPath))) { return resolvedPath; } + } catch { + // which/where exits non-zero when not found; try next name } }This also lets you drop the
spawnSyncimport.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/captions/generate.ts` around lines 53 - 71, The function resolveWhisperExecutablePath currently uses synchronous spawnSync which blocks the Electron main process; replace each spawnSync(pathCommand, [binaryName]) call with the async execFileAsync (or promisified execFile) and await its result, handling errors (non-zero exit or thrown errors) and extracting stdout the same way to produce resolvedPath, then continue to call isExecutableFile(resolvedPath) before returning; also remove the spawnSync import and ensure the function remains async and non-blocking when probing the binaryNames with "which"/"where".electron/ipc/recording/windows.ts (1)
209-220: Deduplicate the defaultAudioSyncAdjustmentliteral.The
{ mode: "none", delayMs: 0, tempoRatio: 1, durationDeltaMs: 0 }object is repeated three times. Hoist a singleNO_SYNC_ADJUSTMENTconstant (or a small helper) to keep these call sites in sync if the type ever gains a field.♻️ Suggested refactor
+const NO_SYNC_ADJUSTMENT: AudioSyncAdjustment = { + mode: "none", + delayMs: 0, + tempoRatio: 1, + durationDeltaMs: 0, +}; @@ - const systemAdjustment = audioAdjustments.get("system") ?? { - mode: "none", - delayMs: 0, - tempoRatio: 1, - durationDeltaMs: 0, - }; - const micAdjustment = audioAdjustments.get("mic") ?? { - mode: "none", - delayMs: 0, - tempoRatio: 1, - durationDeltaMs: 0, - }; + const systemAdjustment = audioAdjustments.get("system") ?? NO_SYNC_ADJUSTMENT; + const micAdjustment = audioAdjustments.get("mic") ?? NO_SYNC_ADJUSTMENT; @@ - const singleAdjustment = audioAdjustments.get(audioInputs[0]) ?? { - mode: "none", - delayMs: 0, - tempoRatio: 1, - durationDeltaMs: 0, - }; + const singleAdjustment = audioAdjustments.get(audioInputs[0]) ?? NO_SYNC_ADJUSTMENT;Also applies to: 273-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/recording/windows.ts` around lines 209 - 220, Duplicate default AudioSyncAdjustment objects are used when reading from audioAdjustments for "system" and "mic" (and again at the other call site); define a single constant like NO_SYNC_ADJUSTMENT = { mode: "none", delayMs: 0, tempoRatio: 1, durationDeltaMs: 0 } and replace the repeated object literals in the audioAdjustments.get("system") ?? ..., audioAdjustments.get("mic") ?? ..., and the other locations (lines around 273-278) to use NO_SYNC_ADJUSTMENT so all defaults stay in sync if the type changes.electron/ipc/project/manager.ts (1)
240-252: ReusehasProjectFileExtensionand fix indentation.The inline
new RegExp(\.(${[PROJECT_FILE_EXTENSION, ...LEGACY_PROJECT_FILE_EXTENSIONS].join("|")})$ , "i")duplicates the extension logic already encapsulated inhasProjectFileExtension(Line 158). Extracting the basename without any known project extension keeps the source of truth single, is cheaper (no regex compilation per entry), and avoids the ast-grep ReDoS flag. The indentation of Lines 243–245 is also off relative to the surrounding object literal.Proposed refactor
+function stripProjectFileExtension(fileName: string) { + const extension = path.extname(fileName).replace(/^\./, "").toLowerCase(); + return [PROJECT_FILE_EXTENSION, ...LEGACY_PROJECT_FILE_EXTENSIONS].includes(extension) + ? fileName.slice(0, -(extension.length + 1)) + : fileName; +} + return { path: normalizedPath, - name: path.basename(normalizedPath).replace( - new RegExp(`\\.(${[PROJECT_FILE_EXTENSION, ...LEGACY_PROJECT_FILE_EXTENSIONS].join("|")})$`, "i"), - "", - ), + name: stripProjectFileExtension(path.basename(normalizedPath)), updatedAt: stats.mtimeMs,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/project/manager.ts` around lines 240 - 252, The object construction duplicates extension logic and mis-indents the basename replacement: replace the inline RegExp-based strip in the name field with a call to the existing hasProjectFileExtension helper (and/or use it to decide how to strip known extensions from path.basename) so extension detection is centralized (referencing hasProjectFileExtension, PROJECT_FILE_EXTENSION, LEGACY_PROJECT_FILE_EXTENSIONS and the name field in the returned object), and fix the indentation of the name property's multiline expression to match the surrounding object literal formatting.
🤖 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/ipc/cursor/monitor.ts`:
- Around line 102-129: The helper process is spawned with stderr piped but never
consumed, which can block the helper when its stderr buffer fills; update the
spawn/options or attach a handler: when creating the process via
spawn(helperPath, [], { stdio: ["pipe","pipe","pipe"] }) either change the
options to set stderr to "ignore" or add a proc.stderr.on("data", ...) handler
that logs/forwards or discards stderr, and ensure this occurs alongside the
existing proc.stdout listener and the proc.once("error"/"close") cleanup so
setNativeCursorMonitorProcess, setNativeCursorMonitorOutputBuffer, and
setCurrentCursorVisualType behave correctly.
- Around line 75-129: The handlers attached to a newly spawned proc can be
tripped by a prior killed process and clobber state; fix
startNativeCursorMonitor by capturing the spawned process in a local variable
(e.g., localProc = proc) and in the 'error' and 'close' callbacks only clear
shared state via setNativeCursorMonitorProcess(...),
setNativeCursorMonitorOutputBuffer(...), and setCurrentCursorVisualType(...) if
the current shared native process still equals that localProc (use the existing
getter like getNativeCursorMonitorProcess() or compare to the value passed into
setNativeCursorMonitorProcess). Ensure you reference the proc/localProc inside
the once('error') and once('close') handlers so they conditionally no-op when a
newer process has been set.
In `@electron/ipc/project/manager.ts`:
- Around line 43-60: The fast-path approval check in isAllowedLocalReadPath is
using the raw candidatePath while rememberApprovedLocalReadPath stores
normalizePath(...) values, so non-normalized inputs can miss approval; normalize
candidatePath once at the top (e.g., const normalizedCandidatePath =
normalizePath(candidatePath)) and use that normalized value for existsSync,
approvedLocalReadPaths.has(...), and when calling isPathInsideDirectory (or
compare against allowedPrefixes) so all checks use the same canonical form; keep
isPathInsideDirectory and allowedPrefixes usage unchanged but pass the
normalizedCandidatePath.
In `@electron/ipc/project/session.ts`:
- Around line 52-83: The manifest-reading block currently returns null for any
error (including missing webcam file), which discards a valid timeOffsetMs;
update the logic in the async function that reads the manifest (the try block
that parses RecordingSessionManifest and calls fs.access on webcamPath) to
distinguish errors: after parsing and validating parsed.version and computing
webcamFileName and timeOffsetMs via
normalizeRecordingTimeOffsetMs(parsed.timeOffsetMs), if webcamFileName exists
call fs.access inside its own try/catch and on failure set webcamPath = null
(but keep timeOffsetMs and videoPath) instead of bubbling the error to the outer
catch; only return null from the outer catch when the manifest is
unreadable/invalid (JSON parse or version check) so callers still receive a
session object with webcamPath: null and preserved timeOffsetMs when the webcam
file is missing.
In `@electron/ipc/recording/prune.ts`:
- Around line 58-72: The loop uses the loop index from sorted.entries()
(variable index) to decide retention, but exempted or sibling-protected entries
(normalizedFilePath, exempt, hasSiblingProjectFile) still increment that index
and thus consume retention slots; change the logic to maintain an explicit
prunableCount (e.g., prunableIndex) that is only incremented for entries that
are eligible for pruning (i.e., after skipping when
exempt.has(normalizedFilePath) and after hasSiblingProjectFile(entry.filePath)
checks), and replace the overLimit check (index >=
AUTO_RECORDING_RETENTION_COUNT) with prunableCount >=
AUTO_RECORDING_RETENTION_COUNT so retention applies only to truly prunable
auto-recordings.
In `@electron/ipc/recording/windows.ts`:
- Around line 249-340: The ffmpeg muxing calls (execFileAsync) can throw and
currently skip cleanup; wrap the entire mux+moveFileWithOverwrite flow in a
try/finally so systemAudioPath and micAudioPath are always fs.rm'd in the
finally block (referencing execFileAsync, mixedOutputPath, systemAudioPath,
micAudioPath, moveFileWithOverwrite), and in the catch/try you should also
attempt to unlink any partial mixedOutputPath artifact on error to avoid stale
*.muxed.mp4 files; apply this to both the multi-input branch (where filterParts,
buildPausedAudioFilter, appendSyncedAudioFilter are used) and the single-input
branch so temp audio files are removed regardless of execFileAsync failure.
- Line 269: The hardcoded 120000 ms ffmpeg timeout in the child process options
must be replaced with a duration scaled to the probed recording length so long
recordings aren't killed; locate the options object(s) that set { timeout:
120000, maxBuffer: ... } in electron/ipc/recording/windows.ts (appears multiple
times) and change the timeout to something like Math.max(120_000, videoDuration
* 2_000) where videoDuration is the probed video length for that recording, or
remove the timeout entirely and rely on the existing cancellation signal/path;
apply the same change to all occurrences (the other two option objects
mentioned) so ffmpeg muxing isn't prematurely terminated.
- Around line 56-62: The stdout handler onStdout currently checks each Buffer
chunk in isolation so the "Recording started" sentinel can be split across
chunks; modify onStdout to append each chunk.toString() into the shared
windowsCaptureOutputBuffer (or a local accumulator) and perform the
includes("Recording started") check against that cumulative string, calling
cleanup() and resolve() when found; also ensure the accumulator is trimmed or
capped to a reasonable max length to avoid unbounded growth and that you still
remove the listener and timers in cleanup.
---
Nitpick comments:
In `@electron/ipc/captions/generate.ts`:
- Around line 53-71: The function resolveWhisperExecutablePath currently uses
synchronous spawnSync which blocks the Electron main process; replace each
spawnSync(pathCommand, [binaryName]) call with the async execFileAsync (or
promisified execFile) and await its result, handling errors (non-zero exit or
thrown errors) and extracting stdout the same way to produce resolvedPath, then
continue to call isExecutableFile(resolvedPath) before returning; also remove
the spawnSync import and ensure the function remains async and non-blocking when
probing the binaryNames with "which"/"where".
In `@electron/ipc/project/manager.ts`:
- Around line 240-252: The object construction duplicates extension logic and
mis-indents the basename replacement: replace the inline RegExp-based strip in
the name field with a call to the existing hasProjectFileExtension helper
(and/or use it to decide how to strip known extensions from path.basename) so
extension detection is centralized (referencing hasProjectFileExtension,
PROJECT_FILE_EXTENSION, LEGACY_PROJECT_FILE_EXTENSIONS and the name field in the
returned object), and fix the indentation of the name property's multiline
expression to match the surrounding object literal formatting.
In `@electron/ipc/recording/prune.ts`:
- Around line 42-51: pruneAutoRecordings currently uses Promise.all over fs.stat
for each entry so a single transient fs.stat failure aborts the whole prune;
change the entries.map logic to tolerate per-entry stat errors by either using
Promise.allSettled on the array of stat promises or wrapping the await fs.stat
call in a try/catch inside the map (returning null on failure) and then filter
out null results before proceeding, ensuring you still reference recordingsDir,
entries, and the autoRecordingStats variable when updating the subsequent
processing logic.
In `@electron/ipc/recording/windows.ts`:
- Around line 209-220: Duplicate default AudioSyncAdjustment objects are used
when reading from audioAdjustments for "system" and "mic" (and again at the
other call site); define a single constant like NO_SYNC_ADJUSTMENT = { mode:
"none", delayMs: 0, tempoRatio: 1, durationDeltaMs: 0 } and replace the repeated
object literals in the audioAdjustments.get("system") ?? ...,
audioAdjustments.get("mic") ?? ..., and the other locations (lines around
273-278) to use NO_SYNC_ADJUSTMENT so all defaults stay in sync if the type
changes.
🪄 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: af328111-058b-4934-8031-49469e3b936e
📒 Files selected for processing (20)
electron/ipc/captions/generate.tselectron/ipc/captions/whisper.tselectron/ipc/cursor/interaction.tselectron/ipc/cursor/monitor.tselectron/ipc/ffmpeg/binary.tselectron/ipc/paths/binaries.tselectron/ipc/project/manager.tselectron/ipc/project/session.tselectron/ipc/recording/diagnostics.tselectron/ipc/recording/ffmpeg.tselectron/ipc/recording/prune.tselectron/ipc/recording/windows.tselectron/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-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-list
🚧 Files skipped from review as they are similar to previous changes (5)
- electron/ipc/ffmpeg/binary.ts
- electron/ipc/recording/diagnostics.ts
- electron/ipc/captions/whisper.ts
- electron/ipc/recording/ffmpeg.ts
- electron/ipc/paths/binaries.ts
The AND gate broke access to user-selected files outside the allowlist (wallpapers, user videos, etc). Keep isPathInsideDirectory normalization fix, revert the existsSync AND guard back to the original OR behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
electron/ipc/project/manager.ts (2)
312-334:⚠️ Potential issue | 🟡 MinorState mutation across awaits still has no rollback (unchanged from prior review).
Between Line 312 and Line 334,
setCurrentProjectPath,setCurrentVideoPath,replaceApprovedSessionLocalReadPaths(whichclear()s first),setCurrentRecordingSession, andrememberRecentProjectexecute across multipleawaitpoints with no recovery. A failure inreplaceApprovedSessionLocalReadPaths(e.g., transientfs.realpatherror) orrememberRecentProject(disk write failure) leaves the app with a newcurrentProjectPath/currentVideoPathpointing at the target project, but withapprovedLocalReadPathscleared or partially populated — meaning subsequent renderer reads of video/webcam/audio sources will fail the allowlist check even though the UI believes the project is loaded. ConcurrentloadProjectFromPathcalls would also interleave theirclear()+add()cycles.Consider gathering all derived values first (validate shape, resolve media, build
approvedProjectPaths,realpatheach entry into a localSet) and only then applying state mutations in one synchronous block at the end, or serializing loads via a module-level mutex with try/rollback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/project/manager.ts` around lines 312 - 334, Collect and validate all derived values (mediaSources.videoPath, mediaSources.webcamPath, editorObj?.audioTracks -> sourcePath) and resolve/realpath each candidate into a local Set first (fail fast on any unexpected shapes or realpath errors), then in one synchronous block apply state changes: call setCurrentProjectPath and setCurrentVideoPath, call replaceApprovedSessionLocalReadPaths once with the fully-resolved list, then call setCurrentRecordingSession and await rememberRecentProject; alternatively protect loadProjectFromPath with a module-level mutex and perform the same gather-then-commit pattern with a try/rollback path that restores previous state if replaceApprovedSessionLocalReadPaths or rememberRecentProject throws.
52-60:⚠️ Potential issue | 🔴 CriticalSecurity regression:
isAllowedLocalReadPathis back to the permissive OR logic.The commit "fix: revert overly strict isAllowedLocalReadPath to OR logic" reintroduces the previously-fixed privilege-escalation vector. With
existsSync(candidatePath)as a top-level OR branch, any file that exists on disk is considered allowed — including/etc/shadow, SSH private keys,C:\Windows\System32\config\SAM, user documents, etc. The allowlist (allowedPrefixes) and the session-approval set (approvedLocalReadPaths) are effectively dead code for anything that happens to exist, which defeats the entire purpose of the gate against a compromised renderer.The PR author's stated reason for the revert is that the stricter AND gate "broke access to user-selected files." The correct fix for that is to add user-selected paths to
approvedLocalReadPathsat selection time (viarememberApprovedLocalReadPath, which already exists), not to remove the allowlist check. If certain flows legitimately need to read arbitrary files, they should go through an explicit approval step rather than an implicit "exists means allowed" bypass.Additionally,
candidatePathis passed raw toexistsSyncandapprovedLocalReadPaths.has(line 58), whilerememberApprovedLocalReadPathstores the normalized form — so the fast-path approval check can also miss on Windows separator/casing differences.🔒 Recommended fix — require existence AND allowlist/approval, normalize once
export function isAllowedLocalReadPath(candidatePath: string) { const allowedPrefixes = [RECORDINGS_DIR, USER_DATA_PATH, getAssetRootPath(), app.getPath("temp")]; + const normalizedCandidatePath = normalizePath(candidatePath); return ( - existsSync(candidatePath) || - allowedPrefixes.some((prefix) => isPathInsideDirectory(candidatePath, prefix)) || - approvedLocalReadPaths.has(candidatePath) + existsSync(normalizedCandidatePath) && + (allowedPrefixes.some((prefix) => isPathInsideDirectory(normalizedCandidatePath, prefix)) || + approvedLocalReadPaths.has(normalizedCandidatePath)) ); }Then ensure every call site that legitimately needs to open a user-picked file calls
rememberApprovedLocalReadPath(selectedPath)right after the picker/drop event resolves, so it's inapprovedLocalReadPathsbefore the read is attempted.Please identify which flow broke under the AND gate and wire it through
rememberApprovedLocalReadPathrather than reverting the whole check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/project/manager.ts` around lines 52 - 60, isAllowedLocalReadPath currently treats any existing file as allowed because of the top-level existsSync OR branch; change it to require the file exists AND is either inside an allowed prefix or explicitly approved, and normalize candidatePath once using the same normalization logic used by rememberApprovedLocalReadPath before checking approvedLocalReadPaths. Concretely, update isAllowedLocalReadPath to (1) normalize candidatePath (using the same path.resolve/normalize and any Windows casing logic used by rememberApprovedLocalReadPath), (2) check existsSync(normalizedPath) AND (allowedPrefixes.some(prefix => isPathInsideDirectory(normalizedPath, prefix)) || approvedLocalReadPaths.has(normalizedPath)), and (3) keep allowedPrefixes built from RECORDINGS_DIR, USER_DATA_PATH, getAssetRootPath(), app.getPath("temp"); also audit flows that relied on the old AND gate and ensure they call rememberApprovedLocalReadPath(selectedPath) after user file selection so the path gets added to approvedLocalReadPaths before reads occur.
🤖 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/ipc/project/manager.ts`:
- Around line 298-310: The loadProjectFromPath function currently calls
fs.readFile and JSON.parse without protection so I/O or malformed JSON will
throw instead of returning the expected { success: false, message } envelope;
wrap the read+parse+resolveProjectMediaSources sequence in a try/catch inside
loadProjectFromPath and on any thrown error (fs.readFile permission/ENOENT and
JSON.parse syntax errors) return the same structured failure object (e.g. {
success: false, canceled: false, message: error.message }) so callers always
receive the discriminated-union result; keep calls to normalizePath and
resolveProjectMediaSources as-is but ensure thrown errors from those calls are
also caught and mapped to the failure response.
---
Duplicate comments:
In `@electron/ipc/project/manager.ts`:
- Around line 312-334: Collect and validate all derived values
(mediaSources.videoPath, mediaSources.webcamPath, editorObj?.audioTracks ->
sourcePath) and resolve/realpath each candidate into a local Set first (fail
fast on any unexpected shapes or realpath errors), then in one synchronous block
apply state changes: call setCurrentProjectPath and setCurrentVideoPath, call
replaceApprovedSessionLocalReadPaths once with the fully-resolved list, then
call setCurrentRecordingSession and await rememberRecentProject; alternatively
protect loadProjectFromPath with a module-level mutex and perform the same
gather-then-commit pattern with a try/rollback path that restores previous state
if replaceApprovedSessionLocalReadPaths or rememberRecentProject throws.
- Around line 52-60: isAllowedLocalReadPath currently treats any existing file
as allowed because of the top-level existsSync OR branch; change it to require
the file exists AND is either inside an allowed prefix or explicitly approved,
and normalize candidatePath once using the same normalization logic used by
rememberApprovedLocalReadPath before checking approvedLocalReadPaths.
Concretely, update isAllowedLocalReadPath to (1) normalize candidatePath (using
the same path.resolve/normalize and any Windows casing logic used by
rememberApprovedLocalReadPath), (2) check existsSync(normalizedPath) AND
(allowedPrefixes.some(prefix => isPathInsideDirectory(normalizedPath, prefix))
|| approvedLocalReadPaths.has(normalizedPath)), and (3) keep allowedPrefixes
built from RECORDINGS_DIR, USER_DATA_PATH, getAssetRootPath(),
app.getPath("temp"); also audit flows that relied on the old AND gate and ensure
they call rememberApprovedLocalReadPath(selectedPath) after user file selection
so the path gets added to approvedLocalReadPaths before reads occur.
🪄 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: 3afa7d00-dd81-4bb3-83a4-da440cc8ae42
📒 Files selected for processing (1)
electron/ipc/project/manager.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (5)
electron/ipc/project/manager.ts (2)
52-61:⚠️ Potential issue | 🔴 Critical🔴 Critical regression —
isAllowedLocalReadPathstill allows ANY existing file.Despite the earlier "✅ Addressed in commit 2ae0aa9" note on this hunk, the current code is still the vulnerable version: the three clauses are joined with
||, soexistsSync(normalizedCandidatePath)alone is sufficient to pass the check. That means a renderer-driven IPCread-local-filecall can target any path that exists on disk —/etc/shadow,~/.ssh/id_rsa,C:\Windows\System32\config\SAM, browser profile stores, etc. — regardless of the allowlist orapprovedLocalReadPaths. The allowlist/approval set only gets consulted when the file doesn't exist (which is when it's useless).The intended predicate is "(exists) AND (inside an allowed prefix OR explicitly approved)". Please re-apply the fix from the prior review.
🔒 Fix
export function isAllowedLocalReadPath(candidatePath: string) { const allowedPrefixes = [RECORDINGS_DIR, USER_DATA_PATH, getAssetRootPath(), app.getPath("temp")]; const normalizedCandidatePath = normalizePath(candidatePath); return ( - existsSync(normalizedCandidatePath) || - allowedPrefixes.some((prefix) => isPathInsideDirectory(normalizedCandidatePath, prefix)) || - approvedLocalReadPaths.has(normalizedCandidatePath) + existsSync(normalizedCandidatePath) && + (allowedPrefixes.some((prefix) => isPathInsideDirectory(normalizedCandidatePath, prefix)) || + approvedLocalReadPaths.has(normalizedCandidatePath)) ); }Please confirm whether the previous fix was reverted or never landed — search the PR's commit history for
isAllowedLocalReadPath:#!/bin/bash # Surface every historical state of isAllowedLocalReadPath on the branch so # we can tell whether the && form ever landed and was later reverted. git log --all --oneline -S 'isAllowedLocalReadPath' -- electron/ipc/project/manager.ts electron/ipc/handlers.ts git log --all -p -S 'isAllowedLocalReadPath' -- electron/ipc/project/manager.ts electron/ipc/handlers.ts | sed -n '1,200p'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/project/manager.ts` around lines 52 - 61, The predicate in isAllowedLocalReadPath is wrong: currently any existing file passes because existsSync(normalizedCandidatePath) is OR'd with allowlist checks; change the logic so the function only returns true if the path exists AND (it is inside one of the allowedPrefixes or is in approvedLocalReadPaths). Update the return expression in isAllowedLocalReadPath to require existsSync(normalizedCandidatePath) && (allowedPrefixes.some(prefix => isPathInsideDirectory(normalizedCandidatePath, prefix)) || approvedLocalReadPaths.has(normalizedCandidatePath)), keeping normalizedCandidatePath, allowedPrefixes and isPathInsideDirectory as the referenced symbols.
299-342:⚠️ Potential issue | 🟡 Minor
loadProjectFromPathstill throws on read/parse errors and mutates shared state without rollback.Two previously-raised concerns are still live:
- Lines 301-302:
fs.readFileandJSON.parsecan throw on truncated/malformed.recordlyfiles or permission errors, so callers get an uncaught rejection instead of the{ success: false, message }envelope that the rest of the function returns.- Lines 326-335:
replaceApprovedSessionLocalReadPathsclearsapprovedLocalReadPathsfirst, then awaitsrememberRecentProjectbefore finally updating project/video/session state. IfrememberRecentProject(or any later step) fails, the session is left half-loaded — approved paths wiped, but project/video pointers still on the prior project — with no recovery.Wrap the read/parse in try/catch returning the structured failure, and defer the mutating calls (
replaceApprovedSessionLocalReadPaths,setCurrentProjectPath,setCurrentVideoPath,setCurrentRecordingSession,rememberRecentProject) into a single synchronous tail after all awaited computations succeed — or guard with a module-level load mutex.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/project/manager.ts` around lines 299 - 342, loadProjectFromPath currently can throw from fs.readFile/JSON.parse and mutates global session state in multiple awaited steps causing partial state if an awaited call fails; wrap the read + JSON.parse in a try/catch that returns { success: false, message: error.message, canceled: false } on failure, and move all mutating calls (replaceApprovedSessionLocalReadPaths, rememberRecentProject, setCurrentProjectPath, setCurrentVideoPath, setCurrentRecordingSession) into a single synchronous tail executed only after all awaits succeed (or protect the sequence with a module-level load mutex); ensure replaceApprovedSessionLocalReadPaths is not called before rememberRecentProject completes or provide rollback logic and include the caught error text in the returned message.electron/ipc/recording/ffmpeg.ts (2)
113-126:⚠️ Potential issue | 🟡 MinorHardcoded avfoundation
"1:none"is still environment-specific.Unlike the Windows/Linux branches, the darwin path ignores
sourceentirely and pins the avfoundation screen device to index1. On systems where the screen index is0/2(e.g., machines with cameras), capture targets the wrong device or fails. Prior feedback still open — consider mappingsource.display_idto the avfoundation index, or enumerating-list_devicesand matching by name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/recording/ffmpeg.ts` around lines 113 - 126, The darwin ffmpeg branch hardcodes the avfoundation device string "1:none" which ignores the provided source and will select the wrong device on some Macs; update the macOS branch that builds the avfoundation args to derive the device index from the incoming source (e.g., map source.display_id to the avfoundation index) or, if mapping is unclear, run ffmpeg -f avfoundation -list_devices true to enumerate device names and match the source.name to pick the correct index, then replace the literal "1:none" with the computed "<index>:none" value so the chosen screen/camera is correct on all environments.
53-53:⚠️ Potential issue | 🟡 MinorStill needs colon-escaping for
gdigrab title=....Window titles containing
:(very common, e.g."App: Document — Editor") break gdigrab's option parser. Prior comment hasn't been addressed — please wrap the value in single quotes and escape colons:`title='${windowTitle.replace(/'/g, "\\'").replace(/:/g, "\\:")}'`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/recording/ffmpeg.ts` at line 53, The ternary that builds the gdigrab source string (currently using windowId ? `hwnd=${windowId}` : `title=${windowTitle}` in ffmpeg.ts) must wrap the title value in single quotes and escape single quotes and colons so gdigrab parses correctly; replace the title branch with a value constructed from windowTitle.replace(/'/g, "\\'").replace(/:/g, "\\:") and wrap it in single quotes (i.e., produce `title='...escaped...'`) while keeping the hwnd branch unchanged.electron/ipc/cursor/telemetry.ts (1)
34-62:⚠️ Potential issue | 🟡 MinorMixed-DPI scale-factor handling still uses primary display unconditionally.
primarySffromgetPrimaryDisplay().scaleFactoris applied to both the Linux cached cursor (Line 43) and the selected-window bounds (Lines 51-52) before locating the target display. On systems with secondary displays at a different scale factor, this pre-scales with the wrong factor and skews thegetDisplayNearestPointlookup and the final normalized coordinates. Prior feedback still open — identify the target display from the unscaled point first, then normalize using that display'sscaleFactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/cursor/telemetry.ts` around lines 34 - 62, getNormalizedCursorPoint incorrectly uses primarySf to pre-scale the Linux cached cursor and window bounds before locating the target display, which breaks multi-DPI setups; change the logic to first compute the raw screen point (use linuxCursorScreenPoint.x/y or fallbackCursor without dividing by primarySf), use getScreen().getDisplayNearestPoint(rawPoint) to find the target display and read that display.scaleFactor, then normalize cursor and selectedWindowBounds by that display's scaleFactor (use that sf to compute width/height and cx/cy). Update references in this function: linuxCursorScreenPoint, primarySf, selectedWindowBounds, getDisplayNearestPoint, and scaleFactor so the display is chosen from the unscaled point and scaling is applied with the correct per-display sf.
🧹 Nitpick comments (2)
electron/ipc/register/recording.ts (1)
655-661: Nit: localconst process = …shadows Node's globalprocess.Inside these try blocks,
const process = nativeCaptureProcess(Line 655) andconst process = ffmpegCaptureProcess(Line 985) shadow the globalprocess. Nothing downstream currently references the global, so there's no bug today — but a future edit that does (e.g.process.platform) would silently break. Renaming to e.g.proc(matching the convention used inwindows.ts'sattachWindowsCaptureLifecycle) avoids the foot-gun.Also applies to: 985-988
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/register/recording.ts` around lines 655 - 661, The local variable named process shadows Node's global process; rename the locals (e.g. change const process = nativeCaptureProcess and const process = ffmpegCaptureProcess) to a non-conflicting name such as proc, procNative, or procFfmpeg in the functions where setNativeCaptureStopRequested and where the ffmpeg capture stop logic is used (and any downstream usages like process.stdin.write) to avoid masking the global process; update all references in those blocks accordingly (both the nativeCaptureProcess assignment and the ffmpegCaptureProcess assignment sites).electron/ipc/recording/mac.ts (1)
409-416: Minor: nullability ordering in the message ternary is confusing.
validation?.durationSeconds !== null && validation !== nullworks today only because short-circuit evaluation saves you whenvalidationitself is null (the first clause becomesundefined !== null === true, but the second fails). Reordering reads more clearly and is defensive against future refactors.♻️ Proposed tweak
- message: - validation?.durationSeconds !== null && validation !== null - ? `Video stored successfully (${validation.fileSizeBytes} bytes, ${validation.durationSeconds.toFixed(2)}s)` - : `Video stored successfully`, + message: + validation !== null && validation.durationSeconds !== null + ? `Video stored successfully (${validation.fileSizeBytes} bytes, ${validation.durationSeconds.toFixed(2)}s)` + : `Video stored successfully`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/recording/mac.ts` around lines 409 - 416, The ternary constructing the message uses a confusing null-check order: change the condition to first ensure validation is non-null (e.g., validation !== null or validation != null) and then check durationSeconds (e.g., validation.durationSeconds != null) so you only call validation.durationSeconds.toFixed(2) when validation is present; update the message expression that builds the success string (the code that returns { success, path: videoPath, message: ... }) to use this reordered, defensive nullability check.
🤖 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/ipc/cursor/interaction.ts`:
- Around line 91-224: Add an early-return guard at the top of
startInteractionCapture to avoid attaching listeners twice by checking for an
existing interaction-capture cleanup handler (the value set by
setInteractionCaptureCleanup) and returning immediately if it exists;
specifically, before calling loadUiohookModule(), detect the current cleanup
(e.g. if (interactionCaptureCleanup) return; or via a getter like
getInteractionCaptureCleanup()) so repeated calls to startInteractionCapture()
won’t attach duplicate listeners to the singleton hook returned by
loadUiohookModule().
In `@electron/ipc/recording/mac.ts`:
- Line 295: The fixed exec call uses a static 120s timeout which is too short
for long recordings; update the execFileAsync(ffmpegPath, args, { timeout: ...
}) invocation to scale the timeout by the probed video duration (e.g. compute
timeoutMs = Math.max(120_000, Math.floor(videoDuration * 2_000)) and pass {
timeout: timeoutMs, maxBuffer: 10 * 1024 * 1024 }) or remove the timeout
entirely; apply the same change to the equivalent execFileAsync calls found in
the Windows recording code paths (the other execFileAsync usages that take
ffmpegPath and args) so long recordings aren’t killed mid-mux.
- Around line 294-310: The ffmpeg mux call around execFileAsync can throw and
currently skips cleanup (systemAudioPath, microphonePath and the partial
mixedOutputPath); wrap the execFileAsync + related logic in a try/finally so the
cleanup loop that removes systemAudioPath and microphonePath always runs, and in
the catch ensure you explicitly unlink the stale mixedOutputPath before
rethrowing the error; update the block surrounding execFileAsync, ffmpegPath,
moveFileWithOverwrite and mixedOutputPath accordingly (and apply the same
pattern to the analogous function in electron/ipc/recording/windows.ts).
In `@electron/ipc/register/project.ts`:
- Around line 352-374: The containment/equality checks are using realpath for
resolvedPath but not for recordingsDir or currentVideoPath, causing mismatches
on symlinked paths; update the handler for 'delete-recording-file' so you call
fs.realpath (with fallback to path.resolve) on the recordingsDir returned by
getRecordingsDir() and on currentVideoPath (if set) before calling
isPathInsideDirectory(resolvedPath, recordingsDir) and comparing
currentVideoPath === resolvedPath, ensuring both sides are normalized the same
way; keep the existing fallback behavior and preserve the telemetry unlink logic
and error handling.
In `@electron/ipc/register/recording.ts`:
- Around line 975-1006: The stop-ffmpeg-recording handler can leak the ffmpeg
child if waitForFfmpegCaptureStop rejects; modify the catch block in the
ipcMain.handle('stop-ffmpeg-recording') handler to, before nulling refs, capture
the current ffmpegCaptureProcess reference and attempt to kill it (wrap
process.kill() in a try/catch and ignore errors), then proceed to
setFfmpegCaptureProcess(null), setFfmpegCaptureTargetPath(null) and
setFfmpegScreenRecordingActive(false); keep the existing error return flow and
include the killed-process cleanup to mirror the native-capture start handlers.
In `@electron/ipc/register/sources.ts`:
- Around line 358-445: The created BrowserWindow highlightWin can leak if await
highlightWin.loadURL(...) rejects because the auto-close setTimeout is scheduled
only after loadURL resolves; ensure highlightWin is always cleaned up by
scheduling the timeout immediately after creating highlightWin (before awaiting
loadURL) or by adding a finally/explicit catch that calls
highlightWin.close()/destroy() if loadURL throws; reference highlightWin,
loadURL, setTimeout, isDestroyed, and close when making the change so the window
is reliably closed on errors.
---
Duplicate comments:
In `@electron/ipc/cursor/telemetry.ts`:
- Around line 34-62: getNormalizedCursorPoint incorrectly uses primarySf to
pre-scale the Linux cached cursor and window bounds before locating the target
display, which breaks multi-DPI setups; change the logic to first compute the
raw screen point (use linuxCursorScreenPoint.x/y or fallbackCursor without
dividing by primarySf), use getScreen().getDisplayNearestPoint(rawPoint) to find
the target display and read that display.scaleFactor, then normalize cursor and
selectedWindowBounds by that display's scaleFactor (use that sf to compute
width/height and cx/cy). Update references in this function:
linuxCursorScreenPoint, primarySf, selectedWindowBounds, getDisplayNearestPoint,
and scaleFactor so the display is chosen from the unscaled point and scaling is
applied with the correct per-display sf.
In `@electron/ipc/project/manager.ts`:
- Around line 52-61: The predicate in isAllowedLocalReadPath is wrong: currently
any existing file passes because existsSync(normalizedCandidatePath) is OR'd
with allowlist checks; change the logic so the function only returns true if the
path exists AND (it is inside one of the allowedPrefixes or is in
approvedLocalReadPaths). Update the return expression in isAllowedLocalReadPath
to require existsSync(normalizedCandidatePath) && (allowedPrefixes.some(prefix
=> isPathInsideDirectory(normalizedCandidatePath, prefix)) ||
approvedLocalReadPaths.has(normalizedCandidatePath)), keeping
normalizedCandidatePath, allowedPrefixes and isPathInsideDirectory as the
referenced symbols.
- Around line 299-342: loadProjectFromPath currently can throw from
fs.readFile/JSON.parse and mutates global session state in multiple awaited
steps causing partial state if an awaited call fails; wrap the read + JSON.parse
in a try/catch that returns { success: false, message: error.message, canceled:
false } on failure, and move all mutating calls
(replaceApprovedSessionLocalReadPaths, rememberRecentProject,
setCurrentProjectPath, setCurrentVideoPath, setCurrentRecordingSession) into a
single synchronous tail executed only after all awaits succeed (or protect the
sequence with a module-level load mutex); ensure
replaceApprovedSessionLocalReadPaths is not called before rememberRecentProject
completes or provide rollback logic and include the caught error text in the
returned message.
In `@electron/ipc/recording/ffmpeg.ts`:
- Around line 113-126: The darwin ffmpeg branch hardcodes the avfoundation
device string "1:none" which ignores the provided source and will select the
wrong device on some Macs; update the macOS branch that builds the avfoundation
args to derive the device index from the incoming source (e.g., map
source.display_id to the avfoundation index) or, if mapping is unclear, run
ffmpeg -f avfoundation -list_devices true to enumerate device names and match
the source.name to pick the correct index, then replace the literal "1:none"
with the computed "<index>:none" value so the chosen screen/camera is correct on
all environments.
- Line 53: The ternary that builds the gdigrab source string (currently using
windowId ? `hwnd=${windowId}` : `title=${windowTitle}` in ffmpeg.ts) must wrap
the title value in single quotes and escape single quotes and colons so gdigrab
parses correctly; replace the title branch with a value constructed from
windowTitle.replace(/'/g, "\\'").replace(/:/g, "\\:") and wrap it in single
quotes (i.e., produce `title='...escaped...'`) while keeping the hwnd branch
unchanged.
---
Nitpick comments:
In `@electron/ipc/recording/mac.ts`:
- Around line 409-416: The ternary constructing the message uses a confusing
null-check order: change the condition to first ensure validation is non-null
(e.g., validation !== null or validation != null) and then check durationSeconds
(e.g., validation.durationSeconds != null) so you only call
validation.durationSeconds.toFixed(2) when validation is present; update the
message expression that builds the success string (the code that returns {
success, path: videoPath, message: ... }) to use this reordered, defensive
nullability check.
In `@electron/ipc/register/recording.ts`:
- Around line 655-661: The local variable named process shadows Node's global
process; rename the locals (e.g. change const process = nativeCaptureProcess and
const process = ffmpegCaptureProcess) to a non-conflicting name such as proc,
procNative, or procFfmpeg in the functions where setNativeCaptureStopRequested
and where the ffmpeg capture stop logic is used (and any downstream usages like
process.stdin.write) to avoid masking the global process; update all references
in those blocks accordingly (both the nativeCaptureProcess assignment and the
ffmpegCaptureProcess assignment sites).
🪄 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: 0ce59db8-c252-4bcd-a701-8d3a10937ba9
📒 Files selected for processing (13)
electron/ipc/cursor/bounds.tselectron/ipc/cursor/interaction.tselectron/ipc/cursor/monitor.tselectron/ipc/cursor/telemetry.tselectron/ipc/export/native-video.tselectron/ipc/project/manager.tselectron/ipc/recording/ffmpeg.tselectron/ipc/recording/mac.tselectron/ipc/recording/windows.tselectron/ipc/register/project.tselectron/ipc/register/recording.tselectron/ipc/register/sources.tselectron/ipc/state.ts
✅ Files skipped from review due to trivial changes (1)
- electron/ipc/export/native-video.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/ipc/cursor/bounds.ts
Collaborator
Author
|
Follow-up fixes are pushed in 0b3169f and 528a31b. I addressed the concrete correctness and safety issues from review around path handling, process cleanup, source highlighting, chunk-safe startup detection, project/session loading, and duplicate listener registration. I am not taking the remaining speculative suggestions that would materially change product behavior in this refactor PR, including stricter local-file size caps, avfoundation device discovery, and mux timeout policy changes, because those need separate product-level validation and could over-tighten existing user workflows. |
This was referenced Apr 18, 2026
This was referenced Apr 28, 2026
Closed
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
Splits the monolithic
electron/ipc/handlers.ts(5967 lines) into 22 focused sub-modules, reducing handlers.ts to ~2930 lines containing onlyregisterIpcHandlersand its direct helpers.New modules
ipc/types.tsipc/constants.tsipc/state.tsipc/utils.tsipc/ffmpeg/binary.tsipc/ffmpeg/filters.tsipc/captions/parser.tsipc/captions/whisper.tsipc/captions/generate.tsipc/paths/binaries.tsipc/cursor/monitor.tsipc/cursor/telemetry.tsipc/cursor/bounds.tsipc/cursor/interaction.tsipc/recording/events.tsipc/recording/diagnostics.tsipc/recording/prune.tsipc/recording/ffmpeg.tsipc/recording/windows.tsipc/recording/mac.tsipc/export/native-video.tsipc/project/session.tsipc/project/manager.tsChanges
handlers.ts: 5967 → 2930 linesnoUnusedLocals: trueSummary by CodeRabbit
New Features
Bug Fixes