fix: address CodeRabbit review feedback

- 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
This commit is contained in:
webadderall
2026-04-17 20:35:08 +10:00
parent 673dfdadd3
commit 2ae0aa9a92
20 changed files with 67 additions and 53 deletions
+5 -5
View File
@@ -12,9 +12,9 @@ import { resolveRecordingSession } from "../project/session";
const execFileAsync = promisify(execFile);
export async function ensureReadableFile(filePath: string, description: string) {
export async function ensureReadableFile(filePath: string, options?: { executable?: boolean }) {
await fs.access(filePath, fsConstants.R_OK);
if (description === "whisper executable") {
if (options?.executable) {
try {
await fs.access(filePath, fsConstants.X_OK);
} catch {
@@ -113,7 +113,7 @@ export async function extractCaptionAudioSource(options: {
for (const candidate of candidates) {
try {
await ensureReadableFile(candidate.path, "video file");
await ensureReadableFile(candidate.path);
await execFileAsync(
options.ffmpegPath,
[
@@ -169,8 +169,8 @@ export async function generateAutoCaptionsFromVideo(options: {
const whisperExecutablePath = await resolveWhisperExecutablePath(options.whisperExecutablePath);
const whisperModelPath = path.resolve(options.whisperModelPath);
await ensureReadableFile(whisperExecutablePath, "whisper executable");
await ensureReadableFile(whisperModelPath, "whisper model");
await ensureReadableFile(whisperExecutablePath, { executable: true });
await ensureReadableFile(whisperModelPath);
const tempBase = path.join(
app.getPath("temp"),
+4 -1
View File
@@ -41,7 +41,7 @@ export function downloadFileWithProgress(
): Promise<void> {
const request = (currentUrl: string, redirectCount = 0): Promise<void> => {
return new Promise((resolve, reject) => {
const req = httpsGet(currentUrl, (response) => {
const req = httpsGet(currentUrl, { timeout: 30_000 }, (response) => {
const statusCode = response.statusCode ?? 0;
const location = response.headers.location;
@@ -97,6 +97,9 @@ export function downloadFileWithProgress(
});
req.on("error", reject);
req.on("timeout", () => {
req.destroy(new Error("Whisper model download timed out."));
});
});
};
+9 -3
View File
@@ -183,7 +183,9 @@ export async function startInteractionCapture() {
hook.on("mousedown", onMouseDown);
hook.on("mouseup", onMouseUp);
hook.on("mousemove", onMouseMove);
if (process.platform === "linux") {
hook.on("mousemove", onMouseMove);
}
hook.start();
@@ -192,11 +194,15 @@ export async function startInteractionCapture() {
if (typeof hook.off === "function") {
hook.off("mousedown", onMouseDown);
hook.off("mouseup", onMouseUp);
hook.off("mousemove", onMouseMove);
if (process.platform === "linux") {
hook.off("mousemove", onMouseMove);
}
} else if (typeof hook.removeListener === "function") {
hook.removeListener("mousedown", onMouseDown);
hook.removeListener("mouseup", onMouseUp);
hook.removeListener("mousemove", onMouseMove);
if (process.platform === "linux") {
hook.removeListener("mousemove", onMouseMove);
}
}
} catch {
// ignore listener cleanup errors
+1 -1
View File
@@ -1,6 +1,7 @@
import { spawn } from "node:child_process";
import { constants as fsConstants } from "node:fs";
import fs from "node:fs/promises";
import { BrowserWindow } from "electron";
import type { CursorVisualType } from "../types";
import {
currentCursorVisualType,
@@ -13,7 +14,6 @@ import {
import { getCursorMonitorExePath, ensureNativeCursorMonitorBinary } from "../paths/binaries";
export function emitCursorStateChanged(cursorType: CursorVisualType) {
const { BrowserWindow } = require("electron") as typeof import("electron");
BrowserWindow.getAllWindows().forEach((window) => {
if (!window.isDestroyed()) {
window.webContents.send("cursor-state-changed", { cursorType });
+10 -6
View File
@@ -6,13 +6,17 @@ import { app } from "electron";
const nodeRequire = createRequire(import.meta.url);
export function loadFfmpegStatic(): string | null {
const moduleExports = nodeRequire("ffmpeg-static");
if (typeof moduleExports === "string") {
return moduleExports;
}
try {
const moduleExports = nodeRequire("ffmpeg-static");
if (typeof moduleExports === "string") {
return moduleExports;
}
if (typeof moduleExports?.default === "string") {
return moduleExports.default as string;
if (typeof moduleExports?.default === "string") {
return moduleExports.default as string;
}
} catch {
// ffmpeg-static not available; fall through to system FFmpeg
}
return null;
+12 -8
View File
@@ -1,13 +1,16 @@
import { spawnSync } from "node:child_process";
import { execFile } from "node:child_process";
import { existsSync, constants as fsConstants } from "node:fs";
import fs from "node:fs/promises";
import path from "node:path";
import { promisify } from "node:util";
import { app } from "electron";
import {
nativeHelperMigrationPromise,
setNativeHelperMigrationPromise,
} from "../state";
const execFileAsync = promisify(execFile);
/**
* Resolve a path within the app bundle, handling asar unpacking in production.
* Files listed in asarUnpack are extracted to app.asar.unpacked/ and must be
@@ -205,13 +208,14 @@ export async function ensureSwiftHelperBinary(
return binaryPath;
}
const result = spawnSync("swiftc", ["-O", sourcePath, "-o", binaryPath], {
encoding: "utf8",
timeout: 120000,
});
if (result.status !== 0) {
const details = [result.stderr, result.stdout].filter(Boolean).join("\n").trim();
try {
await execFileAsync("swiftc", ["-O", sourcePath, "-o", binaryPath], {
encoding: "utf8",
timeout: 120000,
});
} catch (error) {
const err = error as NodeJS.ErrnoException & { stdout?: string; stderr?: string };
const details = [err.stderr, err.stdout].filter(Boolean).join("\n").trim();
throw new Error(details || `Failed to compile ${label}`);
}
+10 -6
View File
@@ -41,10 +41,11 @@ export function getAssetRootPath() {
}
export function isPathInsideDirectory(candidatePath: string, directoryPath: string) {
const normalizedCandidatePath = normalizePath(candidatePath);
const normalizedDirectoryPath = normalizePath(directoryPath);
return (
candidatePath === normalizedDirectoryPath ||
candidatePath.startsWith(`${normalizedDirectoryPath}${path.sep}`)
normalizedCandidatePath === normalizedDirectoryPath ||
normalizedCandidatePath.startsWith(`${normalizedDirectoryPath}${path.sep}`)
);
}
@@ -52,9 +53,9 @@ export function isAllowedLocalReadPath(candidatePath: string) {
const allowedPrefixes = [RECORDINGS_DIR, USER_DATA_PATH, getAssetRootPath(), app.getPath("temp")];
return (
existsSync(candidatePath) ||
allowedPrefixes.some((prefix) => isPathInsideDirectory(candidatePath, prefix)) ||
approvedLocalReadPaths.has(candidatePath)
existsSync(candidatePath) &&
(allowedPrefixes.some((prefix) => isPathInsideDirectory(candidatePath, prefix)) ||
approvedLocalReadPaths.has(candidatePath))
);
}
@@ -238,7 +239,10 @@ export async function buildProjectLibraryEntry(
return {
path: normalizedPath,
name: path.basename(normalizedPath).replace(/\.(recordly|openscreen)$/i, ""),
name: path.basename(normalizedPath).replace(
new RegExp(`\\.(${[PROJECT_FILE_EXTENSION, ...LEGACY_PROJECT_FILE_EXTENSIONS].join("|")})$`, "i"),
"",
),
updatedAt: stats.mtimeMs,
thumbnailPath: thumbnailExists ? thumbnailPath : null,
isCurrent: Boolean(
+1 -1
View File
@@ -65,7 +65,7 @@ export async function resolveRecordingSessionManifest(
return {
videoPath: normalizedVideoPath,
webcamPath: null,
timeOffsetMs: 0,
timeOffsetMs: normalizeRecordingTimeOffsetMs(parsed.timeOffsetMs),
};
}
+3 -7
View File
@@ -55,13 +55,9 @@ export async function probeMediaDurationSeconds(filePath: string): Promise<numbe
await execFileAsync(ffmpegPath, ["-i", filePath, "-hide_banner"], { timeout: 5000 });
} catch (error) {
const stderr = (error as NodeJS.ErrnoException & { stderr?: string })?.stderr ?? "";
const match = stderr.match(/Duration:\s*(\d{2}):(\d{2}):(\d{2})\.(\d{2,3})/);
if (match) {
const h = Number(match[1]);
const m = Number(match[2]);
const s = Number(match[3]);
const frac = Number(match[4]) / (match[4].length === 3 ? 1000 : 100);
return h * 3600 + m * 60 + s + frac;
const duration = parseFfmpegDurationSeconds(stderr);
if (duration !== null) {
return duration;
}
}
return 0;
+1 -1
View File
@@ -1,4 +1,5 @@
import type { ChildProcessWithoutNullStreams } from "node:child_process";
import { access } from "node:fs/promises";
import type { SelectedSource } from "../types";
import {
ffmpegCaptureOutputBuffer,
@@ -165,7 +166,6 @@ export function waitForFfmpegCaptureStop(process: ChildProcessWithoutNullStreams
cleanup();
try {
const { access } = await import("node:fs/promises");
await access(outputPath);
if (code === 0 || code === null) {
resolve(outputPath);
+8 -8
View File
@@ -5,6 +5,7 @@ import {
AUTO_RECORDING_MAX_AGE_MS,
PROJECT_FILE_EXTENSION,
LEGACY_PROJECT_FILE_EXTENSIONS,
COMPANION_AUDIO_LAYOUTS,
} from "../constants";
import { currentVideoPath } from "../state";
import { normalizePath, getTelemetryPathForVideo, isAutoRecordingPath, getRecordingsDir } from "../utils";
@@ -31,6 +32,7 @@ export { isAutoRecordingPath };
export async function pruneAutoRecordings(exemptPaths: string[] = []) {
const recordingsDir = await getRecordingsDir();
await fs.mkdir(recordingsDir, { recursive: true });
const exempt = new Set(
[currentVideoPath, ...exemptPaths]
.filter((value): value is string => Boolean(value))
@@ -74,14 +76,12 @@ export async function pruneAutoRecordings(exemptPaths: string[] = []) {
await fs.rm(getTelemetryPathForVideo(entry.filePath), { force: true });
// Clean up companion audio files left from recording (macOS .m4a, Windows .wav)
const base = entry.filePath.replace(/\.(mp4|mov|webm)$/i, "");
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);
}
} catch (error) {
+3 -6
View File
@@ -32,21 +32,18 @@ const execFileAsync = promisify(execFile);
export async function isNativeWindowsCaptureAvailable(): Promise<boolean> {
if (process.platform !== "win32") return false;
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;
if (!supported) return false;
try {
await fs.access(helperPath, fsConstants.X_OK);
helperExists = true;
await fs.access(getWindowsCaptureExePath(), fsConstants.X_OK);
} catch {
return false;
}
void helperExists;
return supported;
return true;
}
export function waitForWindowsCaptureStart(proc: ChildProcessWithoutNullStreams) {
Binary file not shown.