feat: add macOS native capture and cursor pipeline#573
Conversation
|
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:
📝 Walkthroughsorry — i can't reliably reconstruct the full validated review stack artifact with every rangeId assigned correctly in this session. give me permission to retry and i'll produce a complete, validated hidden artifact block that follows the exact constraints. |
|
@siddharthvaddem I am starting to work on this ;-) |
EtienneLescot
force-pushed
the
feat/macos-native-capture-pipeline
branch
from
603e06c to
b011cc6
Compare
|
Hey @EtienneLescot! Really excited to see this PR — I've been researching exactly this problem for openscreen. I did a deep dive into macOS cursor control for screen recording, covering:
I'd love to contribute to the Swift helper process part. What's the best way to help right now — is there a specific piece you'd like someone to pick up? |
Hey @auberginewly, sure, I'll probably need help for edge cases and Retina/multi-display setups, I'll let you know when I am there. |
|
Sounds good, ping me whenever you're ready! |
|
macOS validation note: I fixed the i18n pre-commit failure by syncing locale key structures against the English baseline. Part of that is directly related to this PR (macOS app/menu labels in common.actions.* and the launch cursor toggle labels), and part is broader catalog cleanup required because npm run i18n:check validates every locale/namespace, not only keys touched by this PR. I kept it as a separate, committable i18n cleanup so the macOS pre-commit check can legitimately stay checked. |
|
@siddharthvaddem I tested it on mac, all seems ok. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 303169b38f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useScreenRecorder.ts (1)
1403-1455:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestart/cancel are dead on the native macOS path.
Once
nativeMacRecording.currentis set, both handlers fall through: there is no native macOS branch, and there is no activescreenRecorderto catch the browser path. That leaves the HUD restart/cancel buttons as no-ops for native mac recording.Suggested fix
const restartRecording = async () => { if (restarting.current) return; if (nativeWindowsRecording.current) { const activeRecordingId = recordingId.current; restarting.current = true; discardRecordingId.current = activeRecordingId; try { await finalizeNativeWindowsRecording(true); await startRecording(); } finally { restarting.current = false; } return; } + + if (nativeMacRecording.current) { + const activeRecordingId = recordingId.current; + restarting.current = true; + discardRecordingId.current = activeRecordingId; + try { + await finalizeNativeMacRecording(true); + await startRecording(); + } finally { + restarting.current = false; + } + return; + } const activeScreenRecorder = screenRecorder.current; if (!activeScreenRecorder || activeScreenRecorder.recorder.state === "inactive") return; ... }; const cancelRecording = () => { if (nativeWindowsRecording.current) { const activeRecordingId = recordingId.current; discardRecordingId.current = activeRecordingId; allowAutoFinalize.current = false; void finalizeNativeWindowsRecording(true); return; } + + if (nativeMacRecording.current) { + const activeRecordingId = recordingId.current; + discardRecordingId.current = activeRecordingId; + allowAutoFinalize.current = false; + void finalizeNativeMacRecording(true); + return; + } const activeScreenRecorder = screenRecorder.current; ... };Also applies to: 1475-1501
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useScreenRecorder.ts` around lines 1403 - 1455, The restart/cancel handlers don't handle the native macOS recording path, so when nativeMacRecording.current is set the HUD buttons become no-ops; update the restartRecording flow (and the similar block around lines 1475-1501) to mirror the native Windows branch: detect nativeMacRecording.current, set restarting.current = true and discardRecordingId.current = recordingId.current, call the native mac finalizer (e.g. finalizeNativeMacRecording(true)) awaited, then await startRecording(), and finally clear restarting.current in a finally block so native mac recordings are properly restarted/cancelled; apply this same pattern to any other restart/cancel handler that currently only checks nativeWindowsRecording.current.
♻️ Duplicate comments (1)
src/i18n/locales/tr/settings.json (1)
197-199:⚠️ Potential issue | 🟡 Minorsame empty nrLevel pattern here.
the entire audio section got stripped down to just
"nrLevel": {}– previous translations for noise reduction (title/levels/descriptions) are gone. this matches the issue in tr/launch.json line 18.see the verification script in tr/launch.json review for checking if this is intentional across the codebase.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/locales/tr/settings.json` around lines 197 - 199, The "audio.nrLevel" object was accidentally emptied, removing the noise reduction translations (title, level labels and descriptions); restore the original keys and values under the "audio.nrLevel" object so it includes the noise reduction title, each level label and their descriptions (match the structure used elsewhere, e.g., the original "nrLevel" pattern referenced in tr/launch.json), and run the same verification script referenced in tr/launch.json to ensure the restored keys conform to the expected schema.
🧹 Nitpick comments (15)
src/i18n/locales/fr/settings.json (1)
12-12: 💤 Low valuenit: empty object serving no purpose
this
"speed": {}under zoom is empty and doesn't exist in the other locale files you're syncing. lowkey should just remove it entirely unless there's a specific reason to keep an empty placeholder here.🧹 suggested cleanup
}, - "speed": {}, "threeD": {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/locales/fr/settings.json` at line 12, Remove the extraneous empty "speed": {} key from the JSON object in the fr settings locale (the entry currently under the zoom group) so the file matches other locale files; if you intended a placeholder, replace it with a commented note or an actual translation string, otherwise delete the "speed" property entirely to avoid dead/unused keys.src/i18n/locales/zh-TW/settings.json (1)
12-12: ⚡ Quick winnit: remove the orphaned empty object for consistency
this empty
"speed": {}doesn't appear in the vi or zh-CN locale files and kinda disrupts the structure. probably leftover from a refactor. removing it would align zh-TW with the other locales and keep things tidy.🧹 suggested cleanup
"focusMode": { "title": "對焦模式", "manual": "手動", "auto": "自動", "autoDescription": "攝影機跟隨錄製時的游標位置" }, - "speed": {}, "threeD": { "title": "3D 旋轉",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/locales/zh-TW/settings.json` at line 12, Remove the orphaned empty "speed" key from the zh-TW settings JSON to match other locales; locate the "speed": {} entry in the settings.json locale object and delete that key/value pair so the structure aligns with vi and zh-CN locales and no trailing empty object remains.docs/engineering/macos-native-recorder-roadmap.md (2)
110-110: 💤 Low valuedoc might be slightly behind implementation state
line 110 says "The helper now writes ScreenCaptureKit system audio into the primary MP4 and attempts runtime-gated native microphone capture" – this matches the Swift implementation which already does native mic capture with
captureMicrophoneandmicrophoneCaptureDeviceID.earlier at line 13, the doc mentions "explicit companion path until it can be moved into the helper" which sounds like mic isn't native yet. might want to sync these statements so it's clear native mic is already implemented with runtime gating for macOS version support.
not blocking, just a clarity thing for future readers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/engineering/macos-native-recorder-roadmap.md` at line 110, Update the roadmap text to avoid conflicting statements about microphone capture: clarify that the ScreenCaptureKit helper already implements native mic capture (referencing captureMicrophone and microphoneCaptureDeviceID behavior) with runtime gating for macOS versions that support it, and remove or revise the earlier "explicit companion path until it can be moved into the helper" wording so it no longer implies mic capture remains non-native; keep the note that AVFoundation webcam composition is still the target end state and that webcam is currently an Electron-recorded sidecar.
12-12: 💤 Low valuenit: "macOS system audio" flagged by static analysis
technically "OS" = "operating system" so this is redundant, but in practice "macOS system audio" is clear and commonly used. could just say "system audio through ScreenCaptureKit" if you want to appease the linter, but honestly this is fine as-is.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/engineering/macos-native-recorder-roadmap.md` at line 12, Replace the phrase "Capture macOS system audio through ScreenCaptureKit" with "Capture system audio through ScreenCaptureKit" to satisfy the linter; locate and update the exact sentence "Capture macOS system audio through ScreenCaptureKit" in the macOS native recorder roadmap and make the wording change.electron/native/screencapturekit/Sources/OpenScreenMacOSCursorHelper/main.swift (1)
242-254: infinite loop with no stdin command handlingunlike the ScreenCaptureKit helper which reads stdin for pause/resume/stop commands, this cursor helper just runs forever in a while(true) loop until killed.
this is probably fine for a sampling helper that's meant to run alongside the main capture and get terminated externally, but worth noting there's no graceful shutdown path. if you ever need to stop sampling cleanly (e.g., to emit final stats), you'd need to add stdin command handling like the other helper.
not blocking, just pointing it out at 2am. 🫠
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/screencapturekit/Sources/OpenScreenMacOSCursorHelper/main.swift` around lines 242 - 254, The current infinite loop in main (while true { ... Thread.sleep(...) }) in the cursor helper never handles stdin commands for pause/resume/stop, so add stdin command handling similar to the ScreenCaptureKit helper: spawn a background reader that listens to stdin for "pause", "resume", and "stop" commands and sets a shared state/enum (e.g., isPaused/isRunning or CursorCaptureState) accessed by the main loop; modify the loop to check that state (call mouseTracker.pump(), consume/emit only when running or emit reduced heartbeat when paused), break cleanly on "stop" to emit any final stats via the existing emit(...) path, and ensure proper synchronization between the reader and main loop around mouseTracker.pump(), currentCursorType(), and leftButtonDown() so shutdown is graceful.src/lib/nativeMacRecording.ts (1)
93-117: 💤 Low valuelowkey DRY opportunity but not critical
both parsers follow the exact same pattern – could extract a helper like
parseIdFromSourcePrefix(sourceId, prefix)to reduce duplication. but with only 2 functions and clear names, this is more of a nice-to-have.tests look solid in the next file, edge cases are covered. ✓
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/nativeMacRecording.ts` around lines 93 - 117, Extract the duplicated parsing logic into a shared helper like parseIdFromSourcePrefix(sourceId, prefix) and update parseMacWindowIdFromSourceId and parseMacDisplayIdFromSourceId to call it; the helper should check sourceId startsWith(prefix), split on ":", validate /^\d+$/ and return Number(value) or null, preserving current behavior and tests for edge cases.electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift (1)
392-410: native microphone uses kvo-style setvalue for private apilines 404-406:
configuration.setValue(true, forKey: "captureMicrophone")andsetValue(deviceId, forKey: "microphoneCaptureDeviceID")are accessing undocumented ScreenCaptureKit properties.the runtime feature detection with
responds(to:)on lines 603-607 is good defensive coding, but this could break in future macOS updates if Apple changes the key names or removes the feature.documented risk – the warning emission on unavailability (lines 395-400) handles the failure case gracefully. just noting this is fragile and might need updates when macOS 15/16 drop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift` around lines 392 - 410, The code uses KVC to set undocumented ScreenCaptureKit keys via configuration.setValue(..., forKey: ...) (see configuration.setValue, resolveMicrophoneCaptureDeviceID, nativeMicrophoneEnabled, emit), which is fragile; wrap all KVC access in a single safe helper (e.g., setPrivateCaptureKey(_:value:on:)) that first feature-detects using supportsNativeMicrophoneCapture and responds(to:) or canRespondToKey, then performs the set inside an ObjC-safe try/catch (or use NSObject.performSelector/objc_setAssociated/try? pattern) so it cannot throw at runtime, log/emit a warning if the key is missing or the set fails, and fall back to configuration.capturesAudio only; centralize the private key strings as constants and call this helper from the existing microphone branch instead of calling setValue directly.electron/native-bridge/cursor/recording/macNativeCursorRecordingSession.ts (2)
315-348: 💤 Low valueheads up: cursor position comes from electron, not the helper
just leaving a breadcrumb —
captureSamplereads position viascreen.getCursorScreenPoint()rather than from the helper payload (sinceMacCursorEvent.sampleonly carries button/cursor-type fields). that's a deliberate split, but anyone reading this in isolation will probably do a double take. a one-line comment explaining "helper supplies button + cursor-type telemetry; position is sampled here to stay in lockstep with electron's display coords" would save a future spelunker some time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native-bridge/cursor/recording/macNativeCursorRecordingSession.ts` around lines 315 - 348, The captureSample function reads cursor position using screen.getCursorScreenPoint() instead of using the helper payload (the helper's MacCursorEvent.sample only includes button and cursor-type fields), so add a one-line clarifying comment at the top of captureSample (near function signature or before acquiring cursor/bounds) stating that position is sampled here from Electron to keep coordinates in lockstep with the display while the helper only supplies button/cursor-type telemetry; reference captureSample and MacCursorEvent.sample in the comment for clarity.
85-161: 💤 Low valueprobe spawn drains stdout but not stderr
minor nit: the
requestMacCursorAccessibilityAccessprobe pipes stderr but never attaches adatalistener to drain it. if the helper happens to emit something chatty on stderr before thereadyline shows up on stdout, the pipe buffer could backpressure the helper. realistically this is fine since you SIGTERM right afterready, but adding a no-op drain (orstderr.resume()) is cheap insurance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native-bridge/cursor/recording/macNativeCursorRecordingSession.ts` around lines 85 - 161, The helper process's stderr is piped but never drained, which risks backpressure; after spawning the helper in requestMacCursorAccessibilityAccess (the Promise block where `child = spawn(helperPath, ...)` is created), attach a no-op drain to stderr (e.g., call `child.stderr.resume()` or `child.stderr.on('data', () => {})`) so the stderr pipe is drained and cannot block the helper before the ready event; add this immediately after the spawn (or next to the child.stdout handling) and ensure it does not change existing error/exit handling.src/lib/cursor/nativeCursor.ts (2)
223-248: 💤 Low value
platform: "darwin"baked into the telemetry assetnit:
TELEMETRY_CURSOR_ASSEThardcodesplatform: "darwin", but the telemetry-only path is also reachable on linux (and theoretically anywhereprovider === "none"with samples). nothing currently keys offasset.platformfor rendering, so it's cosmetic today — but if anything ever branches on it, the linux fallback will silently claim to be a mac cursor. consider deriving fromprocess.platformor just leaving itnull-ish.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cursor/nativeCursor.ts` around lines 223 - 248, TELEMETRY_CURSOR_ASSET currently hardcodes platform: "darwin", which can mislabel telemetry-only cursors on other OSes; update TELEMETRY_CURSOR_ASSET (and its use in getTelemetryCursorAsset) to derive platform from the runtime (e.g. process.platform) or set it to null/undefined so it doesn't falsely report "darwin"; ensure the NativeCursorAsset type allows an optional platform if needed and propagate the derived/empty platform value when spreading TELEMETRY_CURSOR_ASSET in getTelemetryCursorAsset.
510-526: 💤 Low valueinterpolation ignores cursorType changes between samples
lowkey nit: the "asset changed" guard at line 522 only compares
assetId. for telemetry samples both sides arenull, so we happily interpolate across a sample wherecursorTypeflips (e.g.arrow→text). the rendered asset stays asactiveSample's type for the duration of the interpolation, which can look like the cursor "warps" before swapping glyphs. probably fine in practice since interpolation windows are short (~33ms), but worth knowing if anyone reports a flicker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/cursor/nativeCursor.ts` around lines 510 - 526, The interpolation guard currently only compares assetId so telemetry samples (both assetId null) can interpolate across a change in cursorType; update the condition that decides to skip interpolation (the if block using activeSample, nextSample, assetId, timeMs, activeIndex) to also treat a cursorType change as a break in continuity: compare activeSample.cursorType with nextSample.cursorType (or compare the resolved asset cursorType returned by getNativeCursorAsset/getTelemetryCursorAsset) and return { asset, sample: activeSample } when they differ so we don't interpolate across cursor glyph changes.electron/ipc/handlers.ts (3)
138-203: 💤 Low valuepreview-audio cache directory has no eviction
each prepared track lands in
~/Library/Application Support/<app>/preview-audio/<name>.track-N.<mtime>.m4aand stays there forever. mtime in the filename guarantees no stale-cache hits (nice), but it also guarantees that every re-edit of a clip leaves the previous.m4abehind — same goes for any recording the user later deletes. for someone who records daily this directory just grows.not a blocker, but a periodic sweep at startup (delete files older than N days, or whose stem doesn't exist anymore) would keep it tidy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/handlers.ts` around lines 138 - 203, The preview-audio cache never gets cleaned; modify prepareSupplementalPreviewAudioTrack (or add a startup helper invoked once) to sweep PREVIEW_AUDIO_DIR and remove stale files: delete files older than a configurable threshold (e.g., N days) and delete files whose source media no longer exists (compare parsed stem to source files or check referenced source mtime parsed from filename), before creating new outputPath; ensure use of fs.readdir/fs.stat/fs.unlink and handle errors silently so the sweep is best-effort and does not block normal conversion flow.
1744-1758: 💤 Low valuestart error path uses a single
kill(), no SIGTERM→SIGKILL escalationminor consistency thing: the cursor session's
killHelperProcessdoes SIGTERM, then SIGKILL after 500ms if the child is still alive. the catch block here just callsnativeMacCaptureProcess?.kill()(SIGTERM, no follow-up). if the SCK helper is mid-permission-prompt or wedged on AVAssetWriter teardown, you'll leave a zombie. lifting the same escalation helper out and using it in both spots would harmonize behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/handlers.ts` around lines 1744 - 1758, The catch block uses nativeMacCaptureProcess?.kill() only (no SIGTERM→SIGKILL escalation); replace that call with the same escalation helper used elsewhere (call and await killHelperProcess(nativeMacCaptureProcess) or extract a shared function if needed), then proceed to null out nativeMacCaptureProcess/nativeMacCaptureTargetPath/nativeMacCaptureRecordingId and related state and call await stopCursorRecording() as before so both the start error path and the normal shutdown use identical SIGTERM→SIGKILL behavior.
1761-1803: 💤 Low valuepause range uses electron-side wallclock; helper-side pause happens slightly later
pause-native-mac-recordingstampsnativeMacPauseStartedAtMs = Date.now()right after pushing"pause\n"into stdin, andresumesymmetrically usesDate.now()as the end. but the helper only actually freezes its writer once it reads that line and acts on it — so the real pause window is shifted by some ms (line-buffered stdin, scheduler latency, whatever swift is doing). cursor telemetry then getscompactPendingCursorTelemetryPauseRangesapplied with the wallclock range, which can drop or shift a few samples that should have stayed in the timeline.it's not user-visible at 33ms sample intervals in most cases, but if you ever want frame-accurate cursor sync over paused MP4s, having the swift helper echo
{"event":"paused","timestampMs":...}/"resumed"and using those for the range bounds would be the clean fix. logging this so it's not a mystery later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/ipc/handlers.ts` around lines 1761 - 1803, The pause/resume handlers (ipcMain.handle "pause-native-mac-recording" and "resume-native-mac-recording") record pause bounds using Date.now() but the helper only applies pause when it reads the stdin line, causing a skew; fix by having the Swift helper emit JSON events like {"event":"paused","timestampMs":...} and {"event":"resumed","timestampMs":...} on stdout and update the Electron side to listen to nativeMacCaptureProcess.stdout, parse those events and set nativeMacPauseStartedAtMs and the resume timestamp from the helper-provided timestampMs (call completeNativeMacCursorPauseRange() when you receive the resumed event and set nativeMacIsPaused accordingly); keep a Date.now() fallback if no helper timestamp arrives within a short timeout and log a warning to aid debugging.src/components/launch/LaunchWindow.tsx (1)
285-287: ⚡ Quick winCache no-op passthrough transitions before sending IPC.
setHudMouseEventsEnablednow sits on pointer move / enter / wheel paths, so the same boolean gets sent to main over and over while nothing actually changed. A tiny last-value ref here would keep this bridge path much quieter.Also applies to: 402-406, 588-590, 809-813
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/launch/LaunchWindow.tsx` around lines 285 - 287, The setHudMouseEventsEnabled callback is repeatedly sending the same boolean over IPC; add a local last-value ref (e.g., const hudMouseEnabledRef = useRef<boolean | undefined>(undefined)) inside the component and update setHudMouseEventsEnabled to compare the incoming enabled to hudMouseEnabledRef.current, only calling window.electronAPI?.setHudOverlayIgnoreMouseEvents when the value changes and then setting hudMouseEnabledRef.current = enabled; apply the same pattern to the other identical passthrough callbacks found around the other ranges (the pointer move/enter/wheel handlers referenced at 402-406, 588-590, and 809-813) so redundant IPC calls are suppressed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@electron/ipc/handlers.ts`:
- Around line 1006-1051: waitForNativeMacCaptureStart currently attaches a sole
data listener (inspect/onOutput) and calls cleanup() which detaches
stdout/stderr listeners as soon as "recording-started" arrives, leaving no
consumer during the active recording and risking pipe backpressure; fix by
keeping a persistent lightweight drain listener for proc.stdout/proc.stderr that
always appends chunks to nativeMacCaptureOutput (or emit events via a shared
EventEmitter) for the helper's lifetime, and refactor
waitForNativeMacCaptureStart and waitForNativeMacCaptureStop to subscribe to
that shared buffer/emitter instead of owning/removing the only data listener;
ensure cleanup used for early errors only removes temporary watchers (if any)
but does not remove the global drain listener until the helper process actually
exits (proc.once("close")/proc.once("error")).
In
`@electron/native/screencapturekit/Sources/OpenScreenMacOSCursorHelper/main.swift`:
- Line 104: The forced cast at "return (value as! AXUIElement)" can crash if the
accessibility API returns an unexpected type; change it to a conditional cast
(as?) and return nil on mismatch so the helper returns an optional AXUIElement
(e.g., AXUIElement?) instead of crashing. Update the function signature/return
type if needed, replace the forced unwrap with a safe conditional cast and
handle the fallback path where the value is not an AXUIElement by returning nil.
In
`@electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift`:
- Around line 321-338: The current microphone permission flow blocks
indefinitely because semaphore.wait() is used; change it to use a timed wait
(e.g., let result = semaphore.wait(timeout: .now() + 30)) after calling
AVCaptureDevice.requestAccess(for: .audio) and treat a timeout as a denied
permission by throwing HelperError.permissionDenied("Microphone permission is
required for native microphone capture.") — update the logic around
AVCaptureDevice.requestAccess(for: .audio) and the semaphore handling to check
the wait timeout result and also verify AVCaptureDevice.authorizationStatus(for:
.audio) after the wait, throwing the same permission-denied error on timeout or
non-authorized status.
In `@package.json`:
- Around line 23-24: The package.json "build:mac" script currently omits running
the native helper, risking stale helper binaries; update the "build:mac" script
to invoke the existing "build:native:mac" script before the
TypeScript/Vite/electron-builder steps (i.e., run the npm script named
build:native:mac first, then continue with the tsc, vite build and
electron-builder --mac sequence) so the mac packaging always includes freshly
built native helper binaries.
In `@scripts/build-macos-screencapturekit-helper.mjs`:
- Around line 73-76: Add explicit existence checks for the source artifacts
before calling fs.copyFileSync to avoid raw ENOENTs: verify builtHelperPath and
builtCursorHelperPath exist (e.g., with fs.existsSync) and if not, throw or
processLogger.error with a clear message including the missing path; only then
proceed to copy to localHelperPath, distributablePath, localCursorHelperPath,
distributableCursorHelperPath. Reference the variables builtHelperPath,
builtCursorHelperPath, localHelperPath, distributablePath,
localCursorHelperPath, distributableCursorHelperPath and perform the checks
immediately before the current fs.copyFileSync calls so failures report the
specific missing artifact.
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 296-300: The HUD passthrough state isn't reliably restored: the
useEffect watching isLanguageMenuOpen (and setHudMouseEventsEnabled) enables HUD
mouse events when the menu opens but doesn't disable when it closes, and
handleHudDragPointerEnd never re-enables passthrough after a drag ends. Update
the useEffect in LaunchWindow.tsx to setHudMouseEventsEnabled(true) when
isLanguageMenuOpen becomes true and setHudMouseEventsEnabled(false) in the
cleanup/else branch when it becomes false; similarly, modify
handleHudDragPointerEnd to explicitly call setHudMouseEventsEnabled(false) (or
the equivalent restorePassthrough method) when the drag completes or the pointer
is released outside the toolbar, ensuring both the language menu and drag
handlers symmetrically restore passthrough so the overlay stops eating clicks.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 812-938: The async path in startNativeMacRecordingIfAvailable can
continue after the webcam readiness wait even if the countdown was cancelled;
re-check isCountdownRunActive(countdownRunToken) immediately after any awaited
webcam wait (and again before calling
window.electronAPI.startNativeMacRecording) and bail out (return false) if it
returns false, ensuring you also stop/cleanup any created nativeWebcamRecorder
(recorder.stop()) and reset any state changes (e.g., do not set
recordingId/current state) so the stale async flow cannot flip
setRecording(true) or start a native recording.
- Around line 972-974: The new hardcoded toast in useScreenRecorder.ts (the
toast.info call that reads "Allow Accessibility access for OpenScreen, then
press record again to start the countdown.") must be replaced with a localized
string; update the toast.info call to use your project's i18n helper (e.g.,
i18n.t or the useTranslation hook) instead of a literal English message, add a
new localization key such as "accessibility.allow_and_retry" to the locale
resource files for all supported languages, and import/use the same localization
function used elsewhere in this hook/module so the message is shown in the
user's language.
In `@src/i18n/locales/tr/launch.json`:
- Line 18: Remove the dead "nrLevel" JSON key from the Turkish locale files
where it was added as an empty stub (both occurrences), making the Turkish
locales consistent with other locales that don't have this key; search the repo
for "nrLevel" to confirm no code or other locales reference it, delete those
key/value entries, and run your localization/lint checks to ensure no
missing-key regressions are introduced.
---
Outside diff comments:
In `@src/hooks/useScreenRecorder.ts`:
- Around line 1403-1455: The restart/cancel handlers don't handle the native
macOS recording path, so when nativeMacRecording.current is set the HUD buttons
become no-ops; update the restartRecording flow (and the similar block around
lines 1475-1501) to mirror the native Windows branch: detect
nativeMacRecording.current, set restarting.current = true and
discardRecordingId.current = recordingId.current, call the native mac finalizer
(e.g. finalizeNativeMacRecording(true)) awaited, then await startRecording(),
and finally clear restarting.current in a finally block so native mac recordings
are properly restarted/cancelled; apply this same pattern to any other
restart/cancel handler that currently only checks
nativeWindowsRecording.current.
---
Duplicate comments:
In `@src/i18n/locales/tr/settings.json`:
- Around line 197-199: The "audio.nrLevel" object was accidentally emptied,
removing the noise reduction translations (title, level labels and
descriptions); restore the original keys and values under the "audio.nrLevel"
object so it includes the noise reduction title, each level label and their
descriptions (match the structure used elsewhere, e.g., the original "nrLevel"
pattern referenced in tr/launch.json), and run the same verification script
referenced in tr/launch.json to ensure the restored keys conform to the expected
schema.
---
Nitpick comments:
In `@docs/engineering/macos-native-recorder-roadmap.md`:
- Line 110: Update the roadmap text to avoid conflicting statements about
microphone capture: clarify that the ScreenCaptureKit helper already implements
native mic capture (referencing captureMicrophone and microphoneCaptureDeviceID
behavior) with runtime gating for macOS versions that support it, and remove or
revise the earlier "explicit companion path until it can be moved into the
helper" wording so it no longer implies mic capture remains non-native; keep the
note that AVFoundation webcam composition is still the target end state and that
webcam is currently an Electron-recorded sidecar.
- Line 12: Replace the phrase "Capture macOS system audio through
ScreenCaptureKit" with "Capture system audio through ScreenCaptureKit" to
satisfy the linter; locate and update the exact sentence "Capture macOS system
audio through ScreenCaptureKit" in the macOS native recorder roadmap and make
the wording change.
In `@electron/ipc/handlers.ts`:
- Around line 138-203: The preview-audio cache never gets cleaned; modify
prepareSupplementalPreviewAudioTrack (or add a startup helper invoked once) to
sweep PREVIEW_AUDIO_DIR and remove stale files: delete files older than a
configurable threshold (e.g., N days) and delete files whose source media no
longer exists (compare parsed stem to source files or check referenced source
mtime parsed from filename), before creating new outputPath; ensure use of
fs.readdir/fs.stat/fs.unlink and handle errors silently so the sweep is
best-effort and does not block normal conversion flow.
- Around line 1744-1758: The catch block uses nativeMacCaptureProcess?.kill()
only (no SIGTERM→SIGKILL escalation); replace that call with the same escalation
helper used elsewhere (call and await killHelperProcess(nativeMacCaptureProcess)
or extract a shared function if needed), then proceed to null out
nativeMacCaptureProcess/nativeMacCaptureTargetPath/nativeMacCaptureRecordingId
and related state and call await stopCursorRecording() as before so both the
start error path and the normal shutdown use identical SIGTERM→SIGKILL behavior.
- Around line 1761-1803: The pause/resume handlers (ipcMain.handle
"pause-native-mac-recording" and "resume-native-mac-recording") record pause
bounds using Date.now() but the helper only applies pause when it reads the
stdin line, causing a skew; fix by having the Swift helper emit JSON events like
{"event":"paused","timestampMs":...} and {"event":"resumed","timestampMs":...}
on stdout and update the Electron side to listen to
nativeMacCaptureProcess.stdout, parse those events and set
nativeMacPauseStartedAtMs and the resume timestamp from the helper-provided
timestampMs (call completeNativeMacCursorPauseRange() when you receive the
resumed event and set nativeMacIsPaused accordingly); keep a Date.now() fallback
if no helper timestamp arrives within a short timeout and log a warning to aid
debugging.
In `@electron/native-bridge/cursor/recording/macNativeCursorRecordingSession.ts`:
- Around line 315-348: The captureSample function reads cursor position using
screen.getCursorScreenPoint() instead of using the helper payload (the helper's
MacCursorEvent.sample only includes button and cursor-type fields), so add a
one-line clarifying comment at the top of captureSample (near function signature
or before acquiring cursor/bounds) stating that position is sampled here from
Electron to keep coordinates in lockstep with the display while the helper only
supplies button/cursor-type telemetry; reference captureSample and
MacCursorEvent.sample in the comment for clarity.
- Around line 85-161: The helper process's stderr is piped but never drained,
which risks backpressure; after spawning the helper in
requestMacCursorAccessibilityAccess (the Promise block where `child =
spawn(helperPath, ...)` is created), attach a no-op drain to stderr (e.g., call
`child.stderr.resume()` or `child.stderr.on('data', () => {})`) so the stderr
pipe is drained and cannot block the helper before the ready event; add this
immediately after the spawn (or next to the child.stdout handling) and ensure it
does not change existing error/exit handling.
In
`@electron/native/screencapturekit/Sources/OpenScreenMacOSCursorHelper/main.swift`:
- Around line 242-254: The current infinite loop in main (while true { ...
Thread.sleep(...) }) in the cursor helper never handles stdin commands for
pause/resume/stop, so add stdin command handling similar to the ScreenCaptureKit
helper: spawn a background reader that listens to stdin for "pause", "resume",
and "stop" commands and sets a shared state/enum (e.g., isPaused/isRunning or
CursorCaptureState) accessed by the main loop; modify the loop to check that
state (call mouseTracker.pump(), consume/emit only when running or emit reduced
heartbeat when paused), break cleanly on "stop" to emit any final stats via the
existing emit(...) path, and ensure proper synchronization between the reader
and main loop around mouseTracker.pump(), currentCursorType(), and
leftButtonDown() so shutdown is graceful.
In
`@electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift`:
- Around line 392-410: The code uses KVC to set undocumented ScreenCaptureKit
keys via configuration.setValue(..., forKey: ...) (see configuration.setValue,
resolveMicrophoneCaptureDeviceID, nativeMicrophoneEnabled, emit), which is
fragile; wrap all KVC access in a single safe helper (e.g.,
setPrivateCaptureKey(_:value:on:)) that first feature-detects using
supportsNativeMicrophoneCapture and responds(to:) or canRespondToKey, then
performs the set inside an ObjC-safe try/catch (or use
NSObject.performSelector/objc_setAssociated/try? pattern) so it cannot throw at
runtime, log/emit a warning if the key is missing or the set fails, and fall
back to configuration.capturesAudio only; centralize the private key strings as
constants and call this helper from the existing microphone branch instead of
calling setValue directly.
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 285-287: The setHudMouseEventsEnabled callback is repeatedly
sending the same boolean over IPC; add a local last-value ref (e.g., const
hudMouseEnabledRef = useRef<boolean | undefined>(undefined)) inside the
component and update setHudMouseEventsEnabled to compare the incoming enabled to
hudMouseEnabledRef.current, only calling
window.electronAPI?.setHudOverlayIgnoreMouseEvents when the value changes and
then setting hudMouseEnabledRef.current = enabled; apply the same pattern to the
other identical passthrough callbacks found around the other ranges (the pointer
move/enter/wheel handlers referenced at 402-406, 588-590, and 809-813) so
redundant IPC calls are suppressed.
In `@src/i18n/locales/fr/settings.json`:
- Line 12: Remove the extraneous empty "speed": {} key from the JSON object in
the fr settings locale (the entry currently under the zoom group) so the file
matches other locale files; if you intended a placeholder, replace it with a
commented note or an actual translation string, otherwise delete the "speed"
property entirely to avoid dead/unused keys.
In `@src/i18n/locales/zh-TW/settings.json`:
- Line 12: Remove the orphaned empty "speed" key from the zh-TW settings JSON to
match other locales; locate the "speed": {} entry in the settings.json locale
object and delete that key/value pair so the structure aligns with vi and zh-CN
locales and no trailing empty object remains.
In `@src/lib/cursor/nativeCursor.ts`:
- Around line 223-248: TELEMETRY_CURSOR_ASSET currently hardcodes platform:
"darwin", which can mislabel telemetry-only cursors on other OSes; update
TELEMETRY_CURSOR_ASSET (and its use in getTelemetryCursorAsset) to derive
platform from the runtime (e.g. process.platform) or set it to null/undefined so
it doesn't falsely report "darwin"; ensure the NativeCursorAsset type allows an
optional platform if needed and propagate the derived/empty platform value when
spreading TELEMETRY_CURSOR_ASSET in getTelemetryCursorAsset.
- Around line 510-526: The interpolation guard currently only compares assetId
so telemetry samples (both assetId null) can interpolate across a change in
cursorType; update the condition that decides to skip interpolation (the if
block using activeSample, nextSample, assetId, timeMs, activeIndex) to also
treat a cursorType change as a break in continuity: compare
activeSample.cursorType with nextSample.cursorType (or compare the resolved
asset cursorType returned by getNativeCursorAsset/getTelemetryCursorAsset) and
return { asset, sample: activeSample } when they differ so we don't interpolate
across cursor glyph changes.
In `@src/lib/nativeMacRecording.ts`:
- Around line 93-117: Extract the duplicated parsing logic into a shared helper
like parseIdFromSourcePrefix(sourceId, prefix) and update
parseMacWindowIdFromSourceId and parseMacDisplayIdFromSourceId to call it; the
helper should check sourceId startsWith(prefix), split on ":", validate /^\d+$/
and return Number(value) or null, preserving current behavior and tests for edge
cases.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 622cafdd-1f89-4ad7-9895-9fc22c45ba45
📒 Files selected for processing (46)
.gitignoredocs/engineering/macos-native-recorder-roadmap.mdelectron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/native-bridge/cursor/recording/factory.tselectron/native-bridge/cursor/recording/macNativeCursorRecordingSession.tselectron/native/README.mdelectron/native/screencapturekit/Package.swiftelectron/native/screencapturekit/Sources/OpenScreenMacOSCursorHelper/main.swiftelectron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swiftelectron/preload.tselectron/windows.tspackage.jsonscripts/build-macos-screencapturekit-helper.mjssrc/components/launch/LaunchWindow.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/hooks/useScreenRecorder.tssrc/i18n/locales/ar/launch.jsonsrc/i18n/locales/ar/settings.jsonsrc/i18n/locales/es/common.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/common.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/ja-JP/common.jsonsrc/i18n/locales/ja-JP/settings.jsonsrc/i18n/locales/ko-KR/common.jsonsrc/i18n/locales/ko-KR/settings.jsonsrc/i18n/locales/ru/launch.jsonsrc/i18n/locales/ru/settings.jsonsrc/i18n/locales/tr/common.jsonsrc/i18n/locales/tr/launch.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/vi/common.jsonsrc/i18n/locales/vi/launch.jsonsrc/i18n/locales/vi/settings.jsonsrc/i18n/locales/zh-CN/common.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/i18n/locales/zh-TW/common.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/lib/cursor/nativeCursor.test.tssrc/lib/cursor/nativeCursor.tssrc/lib/nativeMacRecording.test.tssrc/lib/nativeMacRecording.tsvite.config.ts
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/components/launch/LaunchWindow.tsx (1)
390-395:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't always restore click-through at drag end.
Lines 390-395 unconditionally disable HUD mouse events after a drag. If the pointer is released over the toolbar/handle, the next click can pass through to the app underneath until the mouse moves again. Kinda cursed UX for something the user is actively holding.
Possible fix
const handleHudDragPointerEnd = (event: React.PointerEvent<HTMLDivElement>) => { dragLastPositionRef.current = null; if (event.currentTarget.hasPointerCapture(event.pointerId)) { event.currentTarget.releasePointerCapture(event.pointerId); } - setHudMouseEventsEnabled(false); + const hovered = document.elementFromPoint(event.clientX, event.clientY) as HTMLElement | null; + setHudMouseEventsEnabled( + isLanguageMenuOpen || Boolean(hovered?.closest("[data-hud-interactive='true']")), + ); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/launch/LaunchWindow.tsx` around lines 390 - 395, The handler handleHudDragPointerEnd currently always calls setHudMouseEventsEnabled(false) which makes clicks pass through if the pointer is released over the HUD handle/toolbar; change it to only disable HUD mouse events when the pointerup happened outside the handle/toolbar: keep dragLastPositionRef.current = null and release pointer capture as-is, then use document.elementFromPoint(event.clientX, event.clientY) and check whether that element is inside your HUD handle/toolbar (use existing refs such as hudHandleRef or hudToolbarRef, or add them if missing) and only call setHudMouseEventsEnabled(false) when the element is not contained in those refs (otherwise leave mouse events enabled so the next click won’t pass through).
🧹 Nitpick comments (3)
src/i18n/locales/en/editor.json (1)
43-44: ⚡ Quick winlowkey could use more guidance on where to grant these permissions
both messages tell users what went wrong but not WHERE to fix it. macOS users might not immediately know where "Privacy & Security" settings live, especially across different OS versions.
consider something like:
- line 43: "Recording permission denied. Please allow screen recording in System Settings > Privacy & Security."
- line 44: "Allow Accessibility access for OpenScreen in System Settings > Privacy & Security, then press record again to start the countdown."
not critical but would make the UX smoother for folks who aren't super familiar with macOS permission dialogs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/locales/en/editor.json` around lines 43 - 44, Update the two localization strings to explicitly tell macOS users where to grant permissions: change the "permissionDenied" value to instruct users to allow screen recording in System Settings > Privacy & Security, and change "accessibilityAllowAndRetry" to instruct users to allow Accessibility access for OpenScreen in System Settings > Privacy & Security before pressing record again; update the values for the keys permissionDenied and accessibilityAllowAndRetry accordingly.electron/native/screencapturekit/Sources/OpenScreenMacOSCursorHelper/main.swift (2)
130-135: 💤 Low valuenit:
isTextInputRolemisses a couple of common AX roles —AXSearchFieldandAXSecureTextField(password fields) won't get the"text"cursor. low stakes since they're rarer, but cheap to add:func isTextInputRole(_ role: String?) -> Bool { role == "AXTextField" || role == "AXTextArea" || role == "AXTextView" || + role == "AXSearchField" || + role == "AXSecureTextField" || role == "AXComboBox" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/screencapturekit/Sources/OpenScreenMacOSCursorHelper/main.swift` around lines 130 - 135, The isTextInputRole function currently checks AX roles but omits AXSearchField and AXSecureTextField, so password and search fields won't be treated as text inputs; update the function (isTextInputRole) to also return true when role equals "AXSearchField" or "AXSecureTextField" by adding those string checks alongside the existing "AXTextField", "AXTextArea", "AXTextView", and "AXComboBox" comparisons.
246-258: ⚡ Quick winsleep + 1ms pump means click events sit in the runloop until next tick
current shape:
pumpspins the runloop for ~1ms, thenThread.sleepblocks for ~33ms. mouse-down/up events fired during that 32ms gap aren't lost (the mach port queues them), but they're not delivered to the callback until the nextpump— which meansleftButtonPressed/leftButtonReleasedcan be attributed to the next sample window instead of the one they actually happened in, slightly skewing click telemetry timing.nit: replace sleep with a runloop-mode run for the full interval so events are processed in real time:
♻️ proposed refactor
-while true { - mouseTracker.pump() - let mouseEvents = mouseTracker.consume() +let intervalSec = Double(intervalMs) / 1000.0 +while true { + // Pump the runloop for the whole interval so tap callbacks fire promptly. + CFRunLoopRunInMode(.defaultMode, intervalSec, false) + let mouseEvents = mouseTracker.consume() emit([ "type": "sample", "timestampMs": timestampMs(), "cursorType": currentCursorType(), "leftButtonDown": leftButtonDown(), "leftButtonPressed": mouseEvents.leftDownCount > 0, "leftButtonReleased": mouseEvents.leftUpCount > 0, ]) - Thread.sleep(forTimeInterval: Double(intervalMs) / 1000.0) }(you can then drop the
pump()method entirely.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@electron/native/screencapturekit/Sources/OpenScreenMacOSCursorHelper/main.swift` around lines 246 - 258, The loop currently calls mouseTracker.pump() (which spins the runloop ~1ms) then Thread.sleep(...) for the remainder, causing input events to be delivered only on the next pump and skewing leftButtonPressed/leftButtonReleased timing; replace the pump+sleep pattern by running the CFRunLoop/RunLoop for the entire interval (e.g. RunLoop.current.run(mode: .default, before: Date(timeIntervalSinceNow: intervalMs/1000.0))) so events are processed continuously between samples, keep using mouseTracker.consume() and emit(... timestampMs(), cursorType, leftButtonDown/Pressed/Released) as before, and you can remove the pump() helper after switching to the runloop-run approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@electron/native/screencapturekit/Sources/OpenScreenMacOSCursorHelper/main.swift`:
- Around line 182-186: The conversion in accessibilityPointForMouse uses the max
Y across all screens which breaks AX coordinates on multi-monitor setups; change
it to flip Y relative to the primary display height (use
NSScreen.screens.first?.frame.height fallback to NSScreen.main?.frame.height)
instead of the global max so the returned CGPoint aligns with the AX coordinate
system used by AXUIElementCopyElementAtPosition; update
accessibilityPointForMouse to compute primaryScreenHeight and return CGPoint(x:
mouse.x, y: primaryScreenHeight - mouse.y).
- Around line 21-51: The callback in start() currently ignores disable events so
the CGEvent tap can die silently; update the callback passed to
CGEvent.tapCreate to handle kCGEventTapDisabledByTimeout and
kCGEventTapDisabledByUserInput by retrieving the MouseButtonTracker from
userInfo (as you already do) and calling CGEvent.tapEnable(tap:
tracker.eventTap, enable: true) (guarding that tracker.eventTap is non-nil) to
re-enable the tap, then continue processing normal mouse events; ensure you use
the same
Unmanaged<MouseButtonTracker>.fromOpaque(userInfo).takeUnretainedValue() path
and reference eventTap (the stored property on MouseButtonTracker) so the
re-enable call can succeed.
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 405-409: The pointer-move handler can disable HUD capture when the
pointer crosses transparent space between the trigger and the portaled language
menu; update the onPointerMove logic in LaunchWindow.tsx (the anonymous handler
that calls setHudMouseEventsEnabled) to also keep capture enabled while the
language menu is open: compute shouldCapture as
Boolean(target?.closest("[data-hud-interactive='true']") || isLanguageMenuOpen)
so setHudMouseEventsEnabled(true) whenever isLanguageMenuOpen is true (or the
existing closest check passes), ensuring the portaled menu remains interactive
while open.
In `@src/i18n/locales/tr/settings.json`:
- Line 197: Remove the orphaned "audio": {} key from the Turkish settings.json
(the empty "audio" object shown in the diff); delete that line so the locale
matches the English baseline and other locales, and ensure the resulting JSON
remains valid (adjust commas if necessary).
---
Duplicate comments:
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 390-395: The handler handleHudDragPointerEnd currently always
calls setHudMouseEventsEnabled(false) which makes clicks pass through if the
pointer is released over the HUD handle/toolbar; change it to only disable HUD
mouse events when the pointerup happened outside the handle/toolbar: keep
dragLastPositionRef.current = null and release pointer capture as-is, then use
document.elementFromPoint(event.clientX, event.clientY) and check whether that
element is inside your HUD handle/toolbar (use existing refs such as
hudHandleRef or hudToolbarRef, or add them if missing) and only call
setHudMouseEventsEnabled(false) when the element is not contained in those refs
(otherwise leave mouse events enabled so the next click won’t pass through).
---
Nitpick comments:
In
`@electron/native/screencapturekit/Sources/OpenScreenMacOSCursorHelper/main.swift`:
- Around line 130-135: The isTextInputRole function currently checks AX roles
but omits AXSearchField and AXSecureTextField, so password and search fields
won't be treated as text inputs; update the function (isTextInputRole) to also
return true when role equals "AXSearchField" or "AXSecureTextField" by adding
those string checks alongside the existing "AXTextField", "AXTextArea",
"AXTextView", and "AXComboBox" comparisons.
- Around line 246-258: The loop currently calls mouseTracker.pump() (which spins
the runloop ~1ms) then Thread.sleep(...) for the remainder, causing input events
to be delivered only on the next pump and skewing
leftButtonPressed/leftButtonReleased timing; replace the pump+sleep pattern by
running the CFRunLoop/RunLoop for the entire interval (e.g.
RunLoop.current.run(mode: .default, before: Date(timeIntervalSinceNow:
intervalMs/1000.0))) so events are processed continuously between samples, keep
using mouseTracker.consume() and emit(... timestampMs(), cursorType,
leftButtonDown/Pressed/Released) as before, and you can remove the pump() helper
after switching to the runloop-run approach.
In `@src/i18n/locales/en/editor.json`:
- Around line 43-44: Update the two localization strings to explicitly tell
macOS users where to grant permissions: change the "permissionDenied" value to
instruct users to allow screen recording in System Settings > Privacy &
Security, and change "accessibilityAllowAndRetry" to instruct users to allow
Accessibility access for OpenScreen in System Settings > Privacy & Security
before pressing record again; update the values for the keys permissionDenied
and accessibilityAllowAndRetry accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24e035f8-7fbe-483a-83dd-20d705a7c061
📒 Files selected for processing (20)
electron/ipc/handlers.tselectron/native/screencapturekit/Sources/OpenScreenMacOSCursorHelper/main.swiftelectron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swiftpackage.jsonscripts/build-macos-screencapturekit-helper.mjssrc/components/launch/LaunchWindow.tsxsrc/hooks/useScreenRecorder.tssrc/i18n/locales/ar/editor.jsonsrc/i18n/locales/en/editor.jsonsrc/i18n/locales/es/editor.jsonsrc/i18n/locales/fr/editor.jsonsrc/i18n/locales/ja-JP/editor.jsonsrc/i18n/locales/ko-KR/editor.jsonsrc/i18n/locales/ru/editor.jsonsrc/i18n/locales/tr/editor.jsonsrc/i18n/locales/tr/launch.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/vi/editor.jsonsrc/i18n/locales/zh-CN/editor.jsonsrc/i18n/locales/zh-TW/editor.json
✅ Files skipped from review due to trivial changes (8)
- src/i18n/locales/zh-CN/editor.json
- src/i18n/locales/fr/editor.json
- src/i18n/locales/ru/editor.json
- src/i18n/locales/ar/editor.json
- src/i18n/locales/ko-KR/editor.json
- src/i18n/locales/vi/editor.json
- src/i18n/locales/es/editor.json
- src/i18n/locales/zh-TW/editor.json
🚧 Files skipped from review as they are similar to previous changes (5)
- package.json
- scripts/build-macos-screencapturekit-helper.mjs
- electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift
- electron/ipc/handlers.ts
- src/hooks/useScreenRecorder.ts
ok |
|
@auberginewly except for camera testing, all is good on my side, were you able to do a camera test? |
|
@EtienneLescot Do you mean an external webcam or the one built in with mac. I do not have an external webcam unfortunately. Tagging @FabLrc @marcgabe15 for visibility |
|
Tested on macOS — here's what I found: ✅ Camera: Works fine, no issues. 🐛 Cursor goes off-screen (external display): When recording on an external monitor, the cursor overlay moves outside the screen boundary. Not sure if this also happens on a single display — could be a coordinate mapping issue specific to multi-display setups. (screenshot below) 
|
|
@auberginewly Thank you for the cam test !! Much appreciated! |
In the post-recording editor, the magnified cursor overlay overflows beyond the video canvas boundary into the Openscreen background area. The cursor overlay should be clipped to the video canvas — when the cursor reaches the edge of the recorded footage, the overlay should stop at the boundary rather than spilling into the background region, to ensure a clean viewing experience. |
Found the root cause — in projectNativeCursorToStage, the stage coordinate isn't clamped to the video canvas bounds after the global projection. Would it be ok if I submit a fix for this? |
|
@auberginewly of course, you can make a PR on my branch ! |
What I mean is: when the cursor is enlarged, part of it overflows outside the video canvas. My suggestion is to apply a clip mask to the video canvas, so the portion of the cursor that goes outside is hidden — not removing the cursor entirely. This way the cursor still appears naturally at the edges, just clipped by the canvas boundary, like CSS overflow: hidden. I'll start working on a fix for this now. |
@siddharthvaddem I meant whatever camere built-in or external. Concerning the cursor overflowing in the padding area, I find that it is better like that (with overflow). Tagging @FabLrc @marcgabe15 for visibility |
cursor overflow in the padding area — I understand you prefer keeping the overflow behavior, and it aligns with Windows' default behavior. No issue there. Quick update on my side: I cleaned up unnecessary mouse movement trails across multi-monitor setups and fixed the cursor jump/disconnect issue when moving between displays. One enhancement suggestion: could we make "cursor constrained to canvas / allowed to overflow" a configurable option? This gives users flexibility: What do you think? |
As for me, I am ok with the option solution but we should also have that checkbox option in windows then. |
|
there's a little bug and i'm trying to fix it |
|
everything looks good on my end - one nit is that the cursor type does not change between a pointer to insert text/ resize/ drag and so on, it remains the default so I am wondering if that is the intended behavior? If so - should we think about making that dynamic? and also support for custom cursors https://sweezy-cursors.com/ let me check if this is a regression from my local setup (not sure what I am doing wrong, but it seems like the cursor remains the default pointer type) |
|
现在我是已经把光标 overflow 给删掉了 修了多余运动轨迹采集的 bug 我来加一个可选 overflow or not 的开关 After more testing I've reconsidered the cursor-overflow approach: Removed the overflow clipping. The hard clip-path/canvas clipping that confined the cursor to the video bounds is gone — clipping the native cursor at the edge looked worse than letting it sit slightly outside in some cases. |
|
@siddharthvaddem @EtienneLescot I've added a set of macOS cursor fixes + an optional toggle on top of this native capture/cursor pipeline. The PR is opened against Etienne's fork: EtienneLescot/openscreen#2 Scope: multi-display ghost cursor/trail, cursor-overflow clipping, preview/export size normalization, lost rounded corners & zoom over-crop fixes, an optional "Clip to Canvas" toggle, and i18n for the cursor settings section across 11 locales. Plan: merge into @EtienneLescot's feat/macos-native-capture-pipeline branch first, then it lands in main together with this #573. Done and ready for review — thanks! 🙏 |
|
feel free to tag me again when everything has been cleared out and is ready to merge
let me check if this is a regression from my local setup (not sure what I am doing wrong, but it seems like the cursor remains the default pointer type) and putting this here again just in case it got missed in the thread |
@siddharthvaddem the cursor is changing correctly on my end, strange, what is your macOS version? |
is alright on my end, my macOS version is 26.4 |
…iew and export - Add nativeCursorClipRef div (outside preserve-3d) with CSS inset() clip-path that tracks the camera-transformed video boundary, including border-radius - Add cameraAwareMaskRect() in FrameRenderer that computes the same boundary for Canvas 2D clip in the export path; remove stage-clamping so rounded corners match the preview's inset() behavior when zoom/pan pushes the mask off-stage - Cache maskBorderRadius in LayoutCache so both shadow and direct composite paths can apply camera-aware rounded clipping - Fix double mask.x offset introduced by nativeCursorMaskRef; replace mask div with clip-path on the outer wrapper - Normalize cursor size relative to maskRect.width so preview and export scale match - Clip cursor to canvas boundary and hide on non-recorded display - Wire cursorClipToBounds flag through FrameRenderConfig and VideoExporter
Add a cursor.clipToBounds toggle to the Settings panel (default on) that controls whether the native cursor is clipped to the video canvas boundary in both preview and export. Wire up 11 locale files (ar, en, es, fr, ja-JP, ko-KR, ru, tr, vi, zh-CN, zh-TW) with the new cursor settings section.
… i18n - Add aria-label to cursorClipToBounds Switch so screen readers announce the control - Mirror composite3D 3D transform onto nativeCursorClipRef so the cursor clip layer rotates with the video during 3D zoom regions (cursor stays outside preserve-3d so clip-path continues to work; only the transform string is mirrored) - Fix vi cursor.motionBlur: "Mờ chuyển động" → "Làm mờ chuyển động" to match effects.motionBlur phrasing - Fix zh-TW cursor.motionBlur: "運動模糊" → "動態模糊" to match effects.motionBlur
EtienneLescot
force-pushed
the
feat/macos-native-capture-pipeline
branch
from
90a0447 to
8516707
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/video-editor/VideoPlayback.tsx (1)
1045-1094:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the old supplemental audio immediately on clip swap.
lowkey risky: when
videoPathchanges from one non-empty value to another, the previous supplemental<audio>stays mounted untilpreparePreviewAudioTrack()resolves. This effect resets the main video, but not the old audio element, so the previous track can briefly keep playing or get resynced against the new clip.🛠️ quick fix
useEffect(() => { + supplementalAudioRef.current?.pause(); + if (supplementalAudioRef.current) { + supplementalAudioRef.current.currentTime = 0; + } + setSupplementalAudioPath(null); + if (!videoPath) { lastResolvedDurationRef.current = null; isResolvingDurationRef.current = false; setVideoReady(false); - setSupplementalAudioPath(null); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/VideoPlayback.tsx` around lines 1045 - 1094, When videoPath changes we must clear any previous supplemental audio immediately to avoid the old <audio> staying mounted; in the useEffect that watches videoPath (the one that calls window.electronAPI.preparePreviewAudioTrack), call setSupplementalAudioPath(null) synchronously at the start (before the async preparePreviewAudioTrack call and before returning early) so the old audio unmounts instantly, and keep the existing cancelled flag and .then/.catch logic to set the new path when the async call resolves.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 1045-1094: When videoPath changes we must clear any previous
supplemental audio immediately to avoid the old <audio> staying mounted; in the
useEffect that watches videoPath (the one that calls
window.electronAPI.preparePreviewAudioTrack), call
setSupplementalAudioPath(null) synchronously at the start (before the async
preparePreviewAudioTrack call and before returning early) so the old audio
unmounts instantly, and keep the existing cancelled flag and .then/.catch logic
to set the new path when the async call resolves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c54c4e9b-b7f9-400d-b956-bc70ac1f52ab
📒 Files selected for processing (20)
electron/native-bridge/cursor/recording/macNativeCursorRecordingSession.tssrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/layoutUtils.tssrc/i18n/locales/ar/settings.jsonsrc/i18n/locales/en/settings.jsonsrc/i18n/locales/es/settings.jsonsrc/i18n/locales/fr/settings.jsonsrc/i18n/locales/ja-JP/settings.jsonsrc/i18n/locales/ko-KR/settings.jsonsrc/i18n/locales/ru/settings.jsonsrc/i18n/locales/tr/settings.jsonsrc/i18n/locales/vi/settings.jsonsrc/i18n/locales/zh-CN/settings.jsonsrc/i18n/locales/zh-TW/settings.jsonsrc/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/videoExporter.ts
✅ Files skipped from review due to trivial changes (6)
- src/i18n/locales/en/settings.json
- src/i18n/locales/zh-CN/settings.json
- src/i18n/locales/fr/settings.json
- src/i18n/locales/ko-KR/settings.json
- src/i18n/locales/ar/settings.json
- src/i18n/locales/es/settings.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/i18n/locales/ru/settings.json
- src/i18n/locales/zh-TW/settings.json
- src/i18n/locales/vi/settings.json
- electron/native-bridge/cursor/recording/macNativeCursorRecordingSession.ts
- src/i18n/locales/ja-JP/settings.json
|
@siddharthvaddem I just merged @auberginewly contribution. |
3 participants
Description
This draft PR starts the macOS-only port of the native capture and cursor pipeline introduced for Windows in PR #217.
The Windows native capture PR has been merged, so this branch is now based directly on latest
upstream/main.The goal is to bring the same architectural separation to macOS: Electron owns recording/session orchestration, while a native ScreenCaptureKit helper owns screen/window capture, cursor exclusion, audio/webcam timing, encoding, and muxing.
What changed
docs/engineering/macos-native-recorder-roadmap.md.src/lib/nativeMacRecording.ts.npm run build:native:macas a placeholder build entrypoint.electron/native/README.md.Follow-up fixes from macOS testing
macOS-only scope
This PR does not change Windows native capture behavior and does not add Linux native capture.
The macOS helper implementation is not shipped yet. This first step is the architecture/contract scaffold so the native helper can be added behind a clean boundary, matching the Windows WGC helper separation.
Architecture notes
The intended macOS backend follows the same separation used by the Windows helper:
Validation run
Passed locally on Windows:
npm run build:native:mac(skips successfully on non-macOS hosts)npm run build-vitenpx vitest run src\lib\nativeMacRecording.test.ts --pool=threadsPreviously passed before rebase:
npm test(137 tests passing)Passed locally on macOS (May 13, 2026):
npm run build:native:macnpm run build-vitenpm run lintnpm run i18n:checknpm test(139 tests passing)Known local validation gap:
Maintainer test checklist
Capture + Launch
Audio
Editor Load + Playback
Timeline
Project Persistence
Other
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores
Localization
Tests