feat: add Windows native capture and cursor pipeline#217
Conversation
EtienneLescot
force-pushed
the
feat/cursor-pipeline
branch
from
b94ffca to
a5d52d8
Compare
EtienneLescot
changed the title
EtienneLescot
force-pushed
the
feat/cursor-pipeline
branch
from
36081e4 to
e8e6a95
Compare
|
Just wanted to thank you for working on this and helping contribute to the project 🙏 I know this is a draft, but wanted to reiterate that I was never against Native Capture. We can have the cursor related settings hidden for Linux and Windows. If that helps. Let me know if there's any further clarification needed. Sorry if this feels like going back and forth Had a couple of folks on X reach out and say they really wanted this for macOS. No rush on this at all. Don't feel obliged. This is a pretty large change which would require a lot of testing to avoid bugs. This is what usually helps me (might have missed something): Capture + Launch
Audio
Editor Load + Playback
Timeline
Project Persistence
Other
|
|
@siddharthvaddem |
take your time 🙏 no rush. want to make sure it is implemented correctly even if it takes time. |
|
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:
📝 WalkthroughWalkthroughAdds a typed native bridge and Windows-native recording pipeline: bridge contracts, state store, services, IPC/preload wiring, renderer client and hooks, cursor recording sessions/adapters, a native wgc-capture helper (C++), audio/video/webcam capture + encoder, exporter and cursor-rendering integration, build/test scripts, and documentation. ChangesNative bridge + Windows-native recording (single DAG)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
|
EtienneLescot
force-pushed
the
feat/cursor-pipeline
branch
from
ff8645e to
bc200cd
Compare
|
@EtienneLescot please close the PR if there are no plans on continuing work here. Appreciate all the work and help put in 🙏 |
@siddharthvaddem my goodness, time goes by so quickly! |
EtienneLescot
force-pushed
the
feat/cursor-pipeline
branch
2 times, most recently
from
00b6e83 to
e48dd9c
Compare
|
@siddharthvaddem I am working on it today ;-) |
EtienneLescot
changed the title
|
@siddharthvaddem I am still making some e2e tests but you can start reviewing if you want. |
![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: dcb402f7e8
ℹ️ 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: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/video-editor/projectPersistence.ts (1)
121-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
encodeURIon the whole path breaks legit file URL round-trips.Filenames with
#or?stay unescaped byencodeURI, so whennew URL()parses them later, it treats those chars as fragment/query delimiters. means persisted paths come back truncated—a#1.mp4loads asa. lowkey risky for project save/load. the fix is pretty straightforward: segment-level encoding so those chars get properly escaped before URL parsing.🤖 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/projectPersistence.ts` around lines 121 - 149, The toFileUrl currently calls encodeURI on the whole path which leaves '#' and '?' unescaped and breaks round-trips; change to encode each path segment with encodeURIComponent (split on "/", encode each segment, then rejoin) so special chars like '#' and '?' are percent-encoded, preserving proper parsing by new URL() in fromFileUrl; keep fromFileUrl as-is (it can continue to use new URL() and decodeURIComponent on the pathname) and ensure you preserve the drive-letter handling logic (the regex that checks for /^[a-zA-Z]:/ and the slice(1) behavior) in the updated toFileUrl implementation.package.json (1)
17-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
npm run buildcan still emit a broken Windows artifact.
build:winnow does the right thing, but the defaultbuildscript still skipsbuild:native:win. so anyone packaging on Windows via the standard entrypoint will ship an app without the helper binary, and the native capture path is kinda dead on arrival. I’d makebuildplatform-aware or route Windows builds throughbuild:winso there’s only one correct path.Also applies to: 24-25
🤖 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 `@package.json` at line 17, The current "build" npm script can produce a Windows package missing the helper binary because it doesn't invoke the Windows-specific step; update the "build" script in package.json to be platform-aware or to delegate to the correct platform-specific script: detect process.platform (or use a cross-platform runner) and run "build:win" when on win32, otherwise run the existing sequence (ensure it also executes "build:native:win" when targeting Windows); modify the "build" entry so it always triggers the Windows native build path for Windows hosts (preserving current behavior for other platforms) and keep references to "build", "build:win", and "build:native:win" so reviewers can locate the change.electron/ipc/handlers.ts (1)
1195-1218:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
get-recorded-video-pathfallback only finds .webm — native helper outputs .mp4.This is fallback-only (when
currentRecordingSessionis missing), so usually harmless, but if state ever gets dropped after a Windows native recording, the fallback will skip everyrecording-*.mp4and report "No recorded video found". Consider including.mp4(and excluding-webcam.mp4) so the fallback also covers the new pipeline.- const videoFiles = files.filter( - (file) => file.endsWith(".webm") && !file.endsWith("-webcam.webm"), - ); + const videoFiles = files.filter( + (file) => + (file.endsWith(".webm") || file.endsWith(".mp4")) && + !file.endsWith("-webcam.webm") && + !file.endsWith("-webcam.mp4"), + );🤖 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 1195 - 1218, The fallback in ipcMain.handle("get-recorded-video-path") only looks for ".webm" files so it misses native Windows ".mp4" outputs; update the files filtering logic (the videoFiles computation that references RECORDINGS_DIR and currentRecordingSession) to include both ".webm" and ".mp4" while still excluding webcam variants (e.g., files ending with "-webcam.webm" or "-webcam.mp4"), keep the existing sorting/reverse selection to pick the latest file, and preserve the same return shapes and error handling.
🟡 Minor comments (13)
src/components/video-editor/timeline/TimelineEditor.tsx-1442-1449 (1)
1442-1449:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHard-coded empty-state copy regresses i18n.
These new literals bypass
useScopedT, so non-English locales will suddenly get English for the first timeline state. lowkey easy fix: add translation keys for both branches instead of inlining the strings here.🤖 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/timeline/TimelineEditor.tsx` around lines 1442 - 1449, The empty-state text in TimelineEditor.tsx currently uses hard-coded English strings controlled by hasVideoSource ("Loading Timeline"/"No Video Loaded" and the subtitle lines); replace these inline literals with translated strings obtained via the existing i18n hook (useScopedT) and new translation keys (e.g. timeline.loadingTitle, timeline.noVideoTitle, timeline.loadingSubtitle, timeline.noVideoSubtitle) so both branches call t(...) instead of embedding English, and add the corresponding keys to the locale files.scripts/test-windows-native-cursor.mjs-1060-1084 (1)
1060-1084:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
JSON.parseon each stdout line will nuke the run on any non-JSON byteif PowerShell ever emits a non-JSON line — partial chunk straddling a flush boundary, a CLIXML preamble that didn't go to stderr, a transient warning, anything —
JSON.parse(trimmed)throws inside thedatahandler and takes the whole script with it. lowkey risky for a diagnostic that's meant to be self-healing.wrap it and surface the bad line so you can keep collecting valid samples.
🛡️ guard
for (const line of lines) { const trimmed = line.trim(); if (!trimmed) { continue; } - const event = JSON.parse(trimmed); - events.push(event); - if (event.asset?.id && !assets.has(event.asset.id)) { - assets.set(event.asset.id, event.asset); - } + let event; + try { + event = JSON.parse(trimmed); + } catch (error) { + process.stderr.write( + `[cursor-native-test] dropped non-JSON line: ${trimmed.slice(0, 200)}\n`, + ); + continue; + } + events.push(event); + if (event.asset?.id && !assets.has(event.asset.id)) { + assets.set(event.asset.id, event.asset); + } }🤖 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 `@scripts/test-windows-native-cursor.mjs` around lines 1060 - 1084, The onStdout handler currently calls JSON.parse(trimmed) directly which will throw on any non-JSON line and crash the sampler; wrap the parse in a try/catch inside the onStdout callback (around JSON.parse(trimmed)) so parsing errors are caught, write a brief diagnostic to stderr including the offending trimmed line and any parse error (use the existing prefix like `[cursor-native-test]`), and continue processing remaining lines so valid events still get pushed to events and assets is updated; reference the onStdout handler, lineBuffer, trimmed, JSON.parse(trimmed), events, assets and stoppingSampler when making the change.docs/engineering/windows-native-recorder-roadmap.md-19-21 (1)
19-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winduplicate non-goal bullet
"Replacing the editor/export pipeline." shows up twice back-to-back; the second one looks like the intended version with the trailing clarification. probably a rebase smudge.
📝 fix
## Non-Goals -- Replacing the editor/export pipeline. - Replacing the editor/export pipeline. A later pass can reintroduce a separate editable native `webcamVideoPath`; the current Windows-native milestone prioritizes a helper-owned multi-flux MP4 with deterministic screen/audio/mic/webcam sync. - Adding a native fallback for macOS or Linux in this branch.🤖 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/windows-native-recorder-roadmap.md` around lines 19 - 21, Remove the duplicate bullet that reads "Replacing the editor/export pipeline." and keep the clarified version that extends it ("Replacing the editor/export pipeline. A later pass can reintroduce a separate editable native `webcamVideoPath`; the current Windows-native milestone prioritizes a helper-owned multi-flux MP4 with deterministic screen/audio/mic/webcam sync."), ensuring only the single, full explanatory bullet remains in the list.electron/native/wgc-capture/src/dshow_webcam_capture.cpp-167-229 (1)
167-229:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove dead registry-resolution code that's never called.
findRegisteredVideoInput,deviceMatchScore,readPropertyString,readRegistryString,splitWords,normalizeDeviceName, andcontainsInsensitiveare all sitting in the anonymous namespace but never get invoked anywhere —initialize()just uses the supplieddirectShowClsiddirectly without any registry lookups. This is a chunk of nontrivial code that's kinda cursed to leave laying around. Since everything's TU-local anyway, might as well yeet it.🤖 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/wgc-capture/src/dshow_webcam_capture.cpp` around lines 167 - 229, Delete the dead registry-resolution functions and any related declarations in the anonymous namespace: remove findRegisteredVideoInput, deviceMatchScore, readPropertyString, readRegistryString, splitWords, normalizeDeviceName, and containsInsensitive (both their declarations and definitions) since initialize() never calls them and they are TU-local; ensure you also remove any forward declarations and using/including headers that exist solely to support these functions to avoid unused-symbol or include clutter.electron/native/wgc-capture/src/audio_sample_utils.cpp-183-205 (1)
183-205:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnbounded enqueue if the encoder stalls — lowkey risky on long captures.
pushSystem/pushMicrophoneonly bail onstopRequested_. If the sink writer (or whateveroutput_does inmain.cpp) blocks for any reason, bothsystemQueue_andmicrophoneQueue_grow without bound, and you'll only notice via memory pressure. A simple high-water-mark cap (drop or coalesce when queue exceeds, say, 2 seconds of audio) plus a one-shot warning would make failure modes far more debuggable.🤖 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/wgc-capture/src/audio_sample_utils.cpp` around lines 183 - 205, pushSystem and pushMicrophone can enqueue unbounded data if the downstream stalls; add a high-water-mark check inside the mutex before calling append to prevent uncontrolled growth. Specifically, in pushSystem and pushMicrophone (guarded by includeSystem_/includeMicrophone_ and stopRequested_), compute whether systemQueue_ / microphoneQueue_ already hold >= ~2 seconds worth of audio (based on your queue's byte-per-second or sample-count units) and if so drop or coalesce the incoming chunk instead of calling append; emit a one-shot warning log the first time this cap is hit and still call cv_.notify_all() as before. Keep the check+drop logic synchronous and non-blocking (do not wait on space), and reuse existing symbols append, systemQueue_, microphoneQueue_, microphoneGain_, mutex_, stopRequested_, and cv_.notify_all().electron/native/wgc-capture/src/mf_encoder.cpp-99-101 (1)
99-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
MFShutdowninfinalize()runs even ifMFStartupfailed → unbalanced ref-count.If
MFStartupon line 99 fails,initialize()returnsfalsewithout ever taking a Startup ref. The destructor then callsfinalize()(line 81), which unconditionally callsMFShutdown()on line 359. It's mostly harmless (returnsMF_E_SHUTDOWN-ish), but you've already got the right pattern inwebcam_capture.cppwithmfStarted_— mirror it here:🛡️ Proposed fix
@@ class MFEncoder ... + bool mfStarted_ = false; @@ MFEncoder::initialize ... - if (!succeeded(MFStartup(MF_VERSION), "MFStartup")) { + if (!succeeded(MFStartup(MF_VERSION), "MFStartup")) { return false; } + mfStarted_ = true; @@ MFEncoder::finalize ... - MFShutdown(); + if (mfStarted_) { + MFShutdown(); + mfStarted_ = false; + }🤖 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/wgc-capture/src/mf_encoder.cpp` around lines 99 - 101, MFShutdown is being called unconditionally in finalize() even when MFStartup failed; add a boolean member (e.g., mfStarted_) defaulting to false, set mfStarted_ = true only after a successful MFStartup() in initialize(), and modify finalize()/the destructor to call MFShutdown() only if mfStarted_ is true; ensure the new mfStarted_ field is declared on the encoder class so the startup/shutdown ref-counts are balanced.electron-builder.json5-7-10 (1)
7-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove the Windows helper to Windows-only packaging config.
The top-level
asarUnpack(line 10) andfiles(line 22) globs apply to all platforms in electron-builder, so macOS and Linux bundles will pick upelectron/native/bin/**too. Since the helper is Windows-only, throw both globs under thewintarget instead (lines 70–75) to keep it outta the other platforms and stay clean on package size.Also applies to: lines 19-22
🤖 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-builder.json5` around lines 7 - 10, Move the Windows-only helper globs out of the top-level "asarUnpack" and top-level "files" arrays and add them under the "win" target configuration so they apply only to Windows; specifically remove "electron/native/bin/**" from the top-level "asarUnpack" and the corresponding Windows helper entries in the top-level "files" array, then insert those globs into the "win" target (the existing win block) so the "asarUnpack" and "files" entries for that helper are Windows-only.src/hooks/useScreenRecorder.ts-188-200 (1)
188-200:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winhanding the camera off from preview to native helper is lowkey racy.
stopWebcamPreviewStream()callstrack.stop()and immediately returns. on Windows, webcam release isn't always synchronous — some drivers/virtual cams (NVIDIA Broadcast 👀) hold the device for a short tail. firing up the native helper right after to grab the samedeviceIdcan intermittently fail with "device in use".if you've seen this work reliably across the targeted webcams that's fine, but worth either (a) awaiting a small grace period, or (b) handling the native error and showing a friendlier toast/retry. otherwise users will hit it once and assume the feature is broken.
also: setting
webcamReady.current = trueafter stopping the preview stream reads weirdly — the comment on the variable is "preview is acquired and ready". in this branch you're using it as "we're done waiting on the preview, native owns it now". a tiny rename or a comment would save the next reader a double-take.🤖 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 188 - 200, stopWebcamPreviewStream currently stops tracks synchronously which can race with immediately acquiring the same device in the native helper; update stopWebcamPreviewStream to wait briefly after calling track.stop() (e.g., await a small timeout/poll for tracks to fully end) or implement retry logic when the native helper fails to open the device, and surface a friendly toast on failure; also clarify the semantics of webcamReady.current (either rename to something like previewReleasedOrReady or add a comment) so its meaning (“preview released and native may acquire”) matches how it’s set in stopWebcamPreviewStream.src/components/launch/LaunchWindow.tsx-258-259 (1)
258-259:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winthese pointer-down counters look like orphaned scaffolding.
setHudPointerDownCountandsetRecordPointerDownCountare declared with the count value discarded ([, setX]). nothing reads them anywhere in this component, yet every pointerdown on the HUD root and on the mic button triggers a state update → a full HUD re-render for zero observable behavior. on a recording HUD that's a real number of wasted renders during normal mouse activity.if these are placeholders for an upcoming feature, consider parking them behind a TODO with a tracking issue. otherwise they should probably go.
cleanup if not needed
- const [, setHudPointerDownCount] = useState(0); - const [, setRecordPointerDownCount] = useState(0); ... - <div - className={`...`} - onPointerDown={() => { - setHudPointerDownCount((count) => count + 1); - }} - > + <div + className={`...`} + > ... - onPointerDown={() => { - setRecordPointerDownCount((count) => count + 1); - }}Also applies to: 330-332, 541-543
scripts/test-windows-wgc-helper.mjs-14-14 (1)
14-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Number("")is 0, which is kinda cursed for a duration.
??only catches undefined/null. If anyone exportsOPENSCREEN_WGC_TEST_DURATION_MS=(empty) or sets it to a non-numeric value in CI, you'll silently get a 0ms stop and the helper never gets a chance to record. Same trap on line 14 — guard with finite check.🛡️ Proposed guard
-const DURATION_MS = Number(process.env.OPENSCREEN_WGC_TEST_DURATION_MS ?? 5000); +const DURATION_RAW = Number(process.env.OPENSCREEN_WGC_TEST_DURATION_MS); +const DURATION_MS = Number.isFinite(DURATION_RAW) && DURATION_RAW > 0 ? DURATION_RAW : 5000;🤖 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 `@scripts/test-windows-wgc-helper.mjs` at line 14, The DURATION_MS initialization using Number(process.env.OPENSCREEN_WGC_TEST_DURATION_MS ?? 5000) treats an empty string or non-numeric value as 0; change the logic around DURATION_MS to parse the env var then validate with a finite/numeric check and fall back to 5000 if invalid — e.g., read process.env.OPENSCREEN_WGC_TEST_DURATION_MS, convert to Number, and if Number.isFinite(value) (or !Number.isNaN(value) && value > 0) use it, otherwise use the default 5000; update the DURATION_MS constant and any related usage to rely on this validated value.electron/ipc/nativeBridge.ts-159-179 (1)
159-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent payload guarding — project handlers will TypeError on malformed requests.
cursorusesrequest.payload?.videoPath(lines 197, 202) butprojectaccessesrequest.payload.projectData/.pathdirectly. A renderer that sends{ domain: "project", action: "setCurrentVideoPath" }without a payload will throwCannot read properties of undefined (reading 'path'), which the outer catch then turns into anINTERNAL_ERRORwithretryable: true— both wrong (it's a validation failure, not a retryable internal error).lowkey safer to align styles or validate payload shape per action.
🛡️ Minimal fix — at least don't crash
case "saveProjectFile": + if (!request.payload || typeof request.payload !== "object") { + return createErrorResponse(requestId, "INVALID_REQUEST", "Missing payload for saveProjectFile."); + } return createSuccessResponse( requestId, await projectService.saveProjectFile( request.payload.projectData, request.payload.suggestedName, request.payload.existingProjectPath, ), ); @@ case "setCurrentVideoPath": + if (!request.payload || typeof request.payload.path !== "string") { + return createErrorResponse(requestId, "INVALID_REQUEST", "Missing path payload for setCurrentVideoPath."); + } return createSuccessResponse( requestId, projectService.setCurrentVideoPath(request.payload.path), );src/components/video-editor/VideoEditor.tsx-159-159 (1)
159-159:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMac exports silently drop click emphasis when "only on clicks" is enabled.
cursorClickTimestampsstays empty and exporters only read that array viaclickEmphasisAlpha(). WhenonlyOnClicks: true(macOS only), this returns alpha=0, killing all click viz in exports. Preview works fine because it usesinteractionTypefromcursorRecordingData.samples[*]instead. Exporters need to branch on that same path for macOS, or derive timestamps fromcursorRecordingDataduring export setup.🤖 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/VideoEditor.tsx` at line 159, cursorClickTimestamps is always empty so exports get alpha=0 when onlyOnClicks is true on macOS; update the export setup to derive click timestamps from cursorRecordingData instead of relying solely on cursorClickTimestamps. Specifically, in the export path that calls clickEmphasisAlpha(), detect when cursorClickTimestamps is empty and onlyOnClicks is enabled and then scan cursorRecordingData.samples for samples whose interactionType indicates a click, populate the timestamps array (or have clickEmphasisAlpha branch to use interactionType from cursorRecordingData.samples) so click emphasis is preserved in exports. Ensure you reference the cursorClickTimestamps variable, the clickEmphasisAlpha() callsite, the onlyOnClicks flag, and cursorRecordingData.samples/interactionType when implementing the change.electron/ipc/handlers.ts-1064-1067 (1)
1064-1067:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
proc.stdin.writecan throw — wrap it.If the helper crashed between
startandstop(or its stdin pipe is already gone), this throws synchronously and we skip awaitingstoppedPathPromise. Outer try/catch will still firestopCursorRecordingand reset state, which is okay-ish, but we lose any in-flightRecording stopped...output and propagate a less helpful error. Cheap fix:- const stoppedPathPromise = waitForNativeWindowsCaptureStop(proc); - proc.stdin.write("stop\n"); + const stoppedPathPromise = waitForNativeWindowsCaptureStop(proc); + try { + proc.stdin.write("stop\n"); + } catch (writeErr) { + console.warn("Failed to send stop to native helper, awaiting exit anyway:", writeErr); + }🤖 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 1064 - 1067, The proc.stdin.write("stop\n") call can throw synchronously; wrap that call in its own try/catch so a write failure doesn't skip awaiting the stoppedPathPromise from waitForNativeWindowsCaptureStop(proc). Specifically, around the write call in the same block that creates stoppedPathPromise (reference stoppedPathPromise, waitForNativeWindowsCaptureStop and proc), catch any synchronous error from proc.stdin.write, record or log it, then still await stoppedPathPromise and assign stoppedPath; after awaiting, rethrow or surface the original write error if needed so outer error handling still runs but you don't lose the in-flight "Recording stopped..." output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93b26c76-1739-40b9-822f-4d613be6bfd4
⛔ Files ignored due to path filters (26)
src/assets/cursors/Cursor=App-Starting.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Beachball.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Cross.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Default.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Hand-(Grabbing).svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Hand-(Open).svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Hand-(Pointing).svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Help.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Menu.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Move.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Not-Allowed.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Resize-(Down).svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Resize-(Left).svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Resize-(Left-Right).svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Resize-(Right).svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Resize-(Up).svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Resize-(Up-Down).svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Resize-North-East-South-West.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Resize-North-South.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Resize-North-West-South-East.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Resize-West-East.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Text-Cursor.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Up-Arrow.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Wait.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Zoom-In.svgis excluded by!**/*.svgsrc/assets/cursors/Cursor=Zoom-Out.svgis excluded by!**/*.svg
📒 Files selected for processing (68)
.gitignoreREADME.mddocs/architecture/native-bridge.mddocs/engineering/windows-native-recorder-roadmap.mddocs/testing/windows-native-cursor.mdelectron-builder.json5electron/electron-env.d.tselectron/ipc/handlers.tselectron/ipc/nativeBridge.tselectron/main.tselectron/native-bridge/cursor/adapter.tselectron/native-bridge/cursor/recording/factory.tselectron/native-bridge/cursor/recording/session.tselectron/native-bridge/cursor/recording/telemetryRecordingSession.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.script.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.types.tselectron/native-bridge/cursor/telemetryCursorAdapter.tselectron/native-bridge/services/cursorService.tselectron/native-bridge/services/projectService.tselectron/native-bridge/services/systemService.tselectron/native-bridge/store.tselectron/native/README.mdelectron/native/wgc-capture/CMakeLists.txtelectron/native/wgc-capture/src/audio_sample_utils.cppelectron/native/wgc-capture/src/audio_sample_utils.helectron/native/wgc-capture/src/dshow_webcam_capture.cppelectron/native/wgc-capture/src/dshow_webcam_capture.helectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/mf_encoder.cppelectron/native/wgc-capture/src/mf_encoder.helectron/native/wgc-capture/src/monitor_utils.cppelectron/native/wgc-capture/src/monitor_utils.helectron/native/wgc-capture/src/wasapi_loopback_capture.cppelectron/native/wgc-capture/src/wasapi_loopback_capture.helectron/native/wgc-capture/src/webcam_capture.cppelectron/native/wgc-capture/src/webcam_capture.helectron/native/wgc-capture/src/wgc_session.cppelectron/native/wgc-capture/src/wgc_session.helectron/preload.tspackage.jsonscripts/build-windows-wgc-helper.mjsscripts/capture-openscreen-preview.mjsscripts/test-windows-native-cursor.mjsscripts/test-windows-wgc-helper.mjssrc/components/launch/LaunchWindow.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/components/video-editor/projectPersistence.tssrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/types.tssrc/components/video-editor/videoPlayback/cursorRenderer.tssrc/components/video-editor/videoPlayback/motionSmoothing.tssrc/components/video-editor/videoPlayback/uploadedCursorAssets.tssrc/hooks/useScreenRecorder.tssrc/lib/cursor/nativeCursor.tssrc/lib/exporter/audioEncoder.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/muxer.tssrc/lib/exporter/videoExporter.tssrc/lib/nativeWindowsRecording.tssrc/native/client.tssrc/native/contracts.tssrc/native/hooks/useCursorRecordingData.tssrc/native/hooks/useCursorTelemetry.tssrc/native/index.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/components/launch/LaunchWindow.tsx`:
- Around line 294-296: The code calls window.electronAPI.switchToEditor()
unconditionally after awaiting
nativeBridgeClient.project.setCurrentVideoPath(result.path); change this so the
editor transition is gated on the bridge call's success: call
nativeBridgeClient.project.setCurrentVideoPath(result.path), inspect its
returned result (e.g., a success flag or error payload), and only call
window.electronAPI.switchToEditor() when that returned result indicates success;
on failure, handle it (log via processLogger/console or surface a user error)
and avoid switching to the editor. Ensure you reference the existing result
variable and the functions setCurrentVideoPath and switchToEditor when
implementing the check.
In `@tests/e2e/windows-native-checklist.spec.ts`:
- Around line 15-26: The tests are locale-fragile because electron.launch starts
with a fresh user-data-dir so the app picks the host Windows locale; update the
launch call (the electron.launch invocation that sets MAIN_JS and
testUserDataDir) to force a stable locale (e.g. add a --lang=en-US CLI arg or
set env LANG/LC_ALL to "en_US.UTF-8" in the env object) or alternatively add
deterministic selectors to the app (add data-testid attributes for the language
prompt, loading/editor/play/background components and update assertions to use
those data-testid values); change the electron.launch env/args in the
testUserDataDir block and any launchApp helper to ensure the locale is forced or
switch assertions to the new data-testid selectors referenced in the tests (also
apply same change to the other occurrences noted around the file).
- Around line 55-63: The cleanup races deleting the Electron temp profile with
the child process teardown: after calling childProcess.kill() (or when
childProcess exists), wait for the child's actual exit event instead of
proceeding immediately—e.g., create a Promise that resolves on the childProcess
'exit'/'close' event (with a fallback timeout like the existing 5_000 ms) and
await that before removing the testUserDataDir (the __testUserDataDir value
accessed as testUserDataDir) and before calling fs.rmSync; also keep the
existing guard checks for childProcess and childProcess.killed so you only wait
when needed.
🪄 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: c8099f92-8ba0-4a1f-83d6-70a21faee958
📒 Files selected for processing (4)
package.jsonsrc/components/launch/LaunchWindow.tsxsrc/components/launch/SourceSelector.tsxtests/e2e/windows-native-checklist.spec.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/launch/SourceSelector.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (11)
src/components/video-editor/SettingsPanel.tsx (2)
1251-1329: ⚡ Quick winAll new cursor accordion labels are hardcoded English — missing i18n.
Every other settings section in this file (zoom, speed, effects, background, layout, crop) goes through
t("…"), but the entire new "Cursor" accordion uses raw string literals:"Cursor","Show Cursor","Size","Smoothing","Motion Blur","Click Bounce". This will break any non-English locale.♻️ Example — wire through translation keys
-<span className="text-xs font-medium">Cursor</span> +<span className="text-xs font-medium">{t("cursor.title")}</span> -<div className="text-[10px] font-medium text-slate-300">Show Cursor</div> +<div className="text-[10px] font-medium text-slate-300">{t("cursor.showCursor")}</div> -<div className="text-[10px] font-medium text-slate-300">Size</div> +<div className="text-[10px] font-medium text-slate-300">{t("cursor.size")}</div> -<div className="text-[10px] font-medium text-slate-300">Smoothing</div> +<div className="text-[10px] font-medium text-slate-300">{t("cursor.smoothing")}</div> -<div className="text-[10px] font-medium text-slate-300">Motion Blur</div> +<div className="text-[10px] font-medium text-slate-300">{t("cursor.motionBlur")}</div> -<div className="text-[10px] font-medium text-slate-300">Click Bounce</div> +<div className="text-[10px] font-medium text-slate-300">{t("cursor.clickBounce")}</div>Add the corresponding keys to all locale JSON files.
🤖 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/SettingsPanel.tsx` around lines 1251 - 1329, Replace the hardcoded English labels in the new Cursor accordion with translation keys using the existing i18n helper (call t("...")) where the labels appear (the "Cursor" title inside AccordionTrigger, "Show Cursor" near the Switch, and the field labels "Size", "Smoothing", "Motion Blur", "Click Bounce" shown around the Slider components and value spans); update the JSX in SettingsPanel (look for AccordionTrigger, AccordionContent, Switch, Slider and the variables showCursor, cursorSize, cursorSmoothing, cursorMotionBlur, cursorClickBounce) to call t(...) for each literal and then add corresponding keys to all locale JSON files so the strings resolve in all locales.
1272-1327: ⚡ Quick winCursor sliders are missing
onValueCommit— lowkey risky on a hot drag path.Every other performance-sensitive slider in this file (motionBlur, shadow, roundness, padding, webcam size, crop highlight size/offset) pairs
onValueChangewith anonValueCommitthat triggers the expensive downstream operation (re-render, export re-computation, etc.). The four cursor sliders fire onlyonValueChange, which means downstream cursor rendering will thrash on every pointer-move tick during a drag.If the cursor pipeline (smoothing, motion blur, etc.) does any non-trivial work, consider adding commit callbacks:
⚡ Proposed fix
// Interface additions (SettingsPanelProps) +onCursorSizeCommit?: () => void; +onCursorSmoothingCommit?: () => void; +onCursorMotionBlurCommit?: () => void; +onCursorClickBounceCommit?: () => void; // Destructor + defaults +onCursorSizeCommit, +onCursorSmoothingCommit, +onCursorMotionBlurCommit, +onCursorClickBounceCommit, // Size slider onValueChange={(values) => onCursorSizeChange?.(values[0])} +onValueCommit={() => onCursorSizeCommit?.()} // Smoothing slider onValueChange={(values) => onCursorSmoothingChange?.(values[0])} +onValueCommit={() => onCursorSmoothingCommit?.()} // Motion blur slider onValueChange={(values) => onCursorMotionBlurChange?.(values[0])} +onValueCommit={() => onCursorMotionBlurCommit?.()} // Click bounce slider onValueChange={(values) => onCursorClickBounceChange?.(values[0])} +onValueCommit={() => onCursorClickBounceCommit?.()}🤖 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/SettingsPanel.tsx` around lines 1272 - 1327, The four cursor Sliders (value props using cursorSize, cursorSmoothing, cursorMotionBlur, cursorClickBounce and change handlers onCursorSizeChange, onCursorSmoothingChange, onCursorMotionBlurChange, onCursorClickBounceChange) are missing onValueCommit callbacks and currently cause downstream work on every drag tick; add onValueCommit handlers for each Slider that call a corresponding commit prop (e.g., onCursorSizeCommit, onCursorSmoothingCommit, onCursorMotionBlurCommit, onCursorClickBounceCommit) with values[0] so expensive rendering/export work only runs on commit (keep existing onValueChange for live UI feedback).electron/native/wgc-capture/src/main.cpp (2)
601-632: 💤 Low valueCapture-side timestamps are dropped; mixer paces audio off its own clock.
You explicitly discard
timestampHns/durationHnsfrom both the mic and loopback callbacks ((void)timestampHns; (void)durationHns;) and just push raw bytes into the mixer. The mixer then synthesizes its ownemittedFrames_-based timestamps (audio_sample_utils.cpp:394). Functionally OK as long as WASAPI keeps the queue real-time aligned (the silence backfill inwasapi_loopback_capture.cppdoes help here), but it means any persistent capture stall — without a discontinuity flag — translates into the mixer happily stamping silence at the "wrong" wall-clock position, producing slow A/V drift on long recordings.Not a release blocker, but if you ever see drift over 10+ minute recordings this is the first place I'd look.
🤖 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/wgc-capture/src/main.cpp` around lines 601 - 632, The callbacks passed to microphoneCapture.start and loopbackCapture.start currently drop timestampHns/durationHns and call audioMixer->pushMicrophone/pushSystem with only raw bytes; update the call chain to preserve and forward capture-side timestamps so the mixer can pace output from the capture clock (instead of synthesizing via emittedFrames_). Concretely: extend or add overloads on audioMixer (e.g., pushMicrophoneWithTimestamp / pushSystemWithTimestamp or add timestamp parameters to pushMicrophone/pushSystem) to accept timestampHns and durationHns, change the lambdas in microphoneCapture.start and loopbackCapture.start to pass those values through, and update the mixer logic (audio_sample_utils.cpp behavior that uses emittedFrames_) to prefer capture timestamps and handle discontinuities when timestamps jump or are missing. Ensure tests/usage that expect the old API are updated and that silence-backfill logic remains compatible.
126-242: 💤 Low value
parseConfigis a positional substring scan — fragile if the schema ever nests.
findBool/findInt/findDouble/findStringall dojson.find("\"" + key + "\"")against the whole blob. Today the helper contract is flat so it works, but any nested object that reuses a key name (e.g. asourceIdinside awebcamobject plus a top-levelsourceId) will silently grab the wrong one. AlsofindStringdoesn't handle\uXXXXescapes, so non-ASCII device names that arrive in escaped form will be mangled.Pulling in a tiny header-only JSON lib (nlohmann/json or simdjson) would kill a whole class of future foot-guns. Not urgent for v1, but the moment the schema grows this'll bite.
🤖 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/wgc-capture/src/main.cpp` around lines 126 - 242, The current helpers (findBool, findInt64, findInt, findDouble, findString used by parseConfig) are fragile because they substring-scan the whole JSON blob (wrong for nested keys) and findString doesn't decode \uXXXX; replace these ad-hoc parsers with a proper JSON parser (e.g. include nlohmann::json single-header), parse the input once into a nlohmann::json object, and change findBool/findInt64/findInt/findDouble/findString to look up keys from that parsed top-level object (using .contains() and appropriate .get<T>() calls) so nested fields won't collide and unicode escapes (including \uXXXX) are handled by the library. Ensure error handling mirrors current fallbacks by returning the provided fallback on parse or type errors.electron/native/wgc-capture/src/audio_sample_utils.cpp (4)
347-409: 💤 Low valuePacing the mixer with
sleep_untilagainst wall-clock is a bit redundant and lowkey risky.The mixer is fed by WASAPI capture, which is already realtime. The
sleep_until(audioClockStart + emittedFrames/sampleRate)at line 407 effectively re-paces to wall-clock — fine on the happy path, but if for any reason input bursts in (e.g. WASAPI gap-fill fromwasapi_loopback_capture.cppemits a backlog of silence frames after a discontinuity), the mixer will also sleep through that catch-up, growing the queues unboundedly while the encoder waits.Two things worth considering:
- If the queues are meant to be drained at realtime, the upstream capture rate already enforces that — the sleep is mostly just adding latency.
- There's no upper bound on
systemQueue_/microphoneQueue_size. A stalled output callback or a slow encoder will let these grow forever. A simple cap (e.g. drop oldest beyond N seconds) would prevent unbounded memory growth in pathological cases.Not a release blocker for typical usage, just flagging.
🤖 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/wgc-capture/src/audio_sample_utils.cpp` around lines 347 - 409, AudioMixer::mixLoop currently re-paces output with sleep_until(audioClockStart + emittedFrames_/sampleRate) which can delay draining when upstream bursts backlog, and you also lack a cap on systemQueue_/microphoneQueue_ sizes; change mixLoop to stop using long wall-clock sleep for pacing (remove sleep_until and rely on the existing cv_.wait_for and the real-time WASAPI feed / output_ timings) or replace it with a short non-blocking yield (e.g., sleep_for a few ms) only when necessary, and implement a bounded-queue policy for systemQueue_ and microphoneQueue_ (e.g., maxBytes = N_seconds * format_.sampleRate * format_.blockAlign) so producers drop oldest bytes when pushing beyond the cap (implement drop-in-push in the producer or in pop/push helpers), referencing AudioMixer::mixLoop, audioClockStart, emittedFrames_, sleep_until, systemQueue_, microphoneQueue_, pop(), includeSystem_/includeMicrophone_, and output_. Ensure queue-dropping is thread-safe under mutex_ and that stopRequested_ and timelineStarted_ logic remain unchanged.
178-198: 💤 Low valueNearest-neighbor resampling will produce audible aliasing on real format mismatches.
convertAudioWithGaindoes a naive nearest-neighbor frame mapping (std::llround(sourcePosition)) when sample rates differ. That's functional but kinda cursed for audio — any time mic is at 16k/44.1k and the target mix is 48k (or vice versa), you'll get noticeable aliasing/zipper artifacts, especially on speech. In practice WASAPI shared-mode usually hands you the mix format already at 48k so this path may rarely fire, but the moment a mic exposes a different native rate it's gonna sound rough.For v1 it's probably fine, but consider at minimum linear interpolation, or ideally a real polyphase resampler (or just use MF's
IMFTransformresampler since you've already got MF in the build). Worth a TODO if not addressing now.
263-275: 💤 Low valueNit:
start()has no guard against being called twice.If
start()ever gets invoked whilethread_is already joinable, the previousstd::threadgets stomped andstd::terminatefires. Looking atmain.cppit's only called once per AudioMixer instance, so this is purely defensive, but a quickif (thread_.joinable()) return false;(or assert) would prevent future foot-guns.🤖 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/wgc-capture/src/audio_sample_utils.cpp` around lines 263 - 275, AudioMixer::start lacks a guard against being called when a previous thread is still running; check thread_.joinable() at the top of start() and bail (return false or assert) if true to avoid stomping an active std::thread. Specifically, in AudioMixer::start() before mutating stopRequested_, emittedFrames_, timelineStarted_ and launching the lambda that calls mixLoop(), add a guard using thread_.joinable() to prevent starting a second thread while the first is still active.
200-241: ⚡ Quick winMix-then-clip without headroom = hard clipping when both sources are loud.
mixAudioInPlacestraight-up sums two streams and clamps at full-scale. When system audio and mic are both hot (e.g. full-volume music + a loud talker), the sum saturates and you get audible distortion in the recording. Very common pitfall for screen+mic mixing.A simple improvement: attenuate by ~0.5 (or sqrt(0.5) ≈ 0.707 for power-preserving) before summing, or apply a soft-knee limiter. Even just a constant -3 to -6 dB on each branch buys a lot of headroom. Not a blocker, but users on hot input will notice.
🤖 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/wgc-capture/src/audio_sample_utils.cpp` around lines 200 - 241, mixAudioInPlace currently sums destination and source samples then hard-clips, causing distortion when both are loud; modify each mixing branch in mixAudioInPlace (float path detected by isFloatFormat, 16-bit and 32-bit PCM paths detected by isPcmFormat) to apply a fixed attenuation/gain to inputs before summing (e.g., multiply both samples by ~0.7071 or 0.5 for headroom) and then add and clamp (use clampTo for integer paths and std::clamp for float) so the sum preserves headroom and avoids hard clipping; ensure conversions use floating math when applying the gain and keep existing clamping behavior after summation.electron/native/wgc-capture/src/wasapi_loopback_capture.h (1)
24-24: 💤 Low value
AudioCallbackreturnsvoidwhileAudioMixer::OutputCallbackreturnsbool— kinda inconsistent.The capture-side callback is fire-and-forget (
std::function<void(...)>), so even when downstream has good reason to bail (encoder failure, mixer stopped), there's no clean way to propagate that back here. Thewasapi_loopback_capture.cppcapture loop already discards whatever the callback "returns" (lines ~339, 388, 390), and consumers inmain.cpplean on the sharedstopRequestedatomic to short-circuit pushes. Works, but it's lowkey awkward — the audio capture keeps grinding away pushing into queues even after the encoder is toast, and the API shape diverges fromAudioMixer::OutputCallbackwhich deliberately usesboolfor flow control.Aligning the signatures (returning
booland breaking out ofcaptureLoopwhen the consumer says "done") would tighten things up. Not a blocker sincestopRequestedcovers it in practice, but worth a thought.🤖 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/wgc-capture/src/wasapi_loopback_capture.h` at line 24, Change the capture callback typedef to return bool (match AudioMixer::OutputCallback) and propagate that decision from the capture loop: update using AudioCallback = std::function<bool(const BYTE* data, DWORD byteCount, int64_t timestampHns, int64_t durationHns)> in wasapi_loopback_capture.h, modify the captureLoop implementation in wasapi_loopback_capture.cpp to check the callback return value and break/stop the loop when it returns false, and adjust all callers (e.g., the lambda passed from main.cpp and any other consumers) to return false when they want the capture to stop (encoder failure, mixer stopped) and true to continue; ensure stopRequested atomic handling remains consistent with the new early-exit behavior.electron/native/wgc-capture/src/wasapi_loopback_capture.cpp (2)
345-411: 💤 Low valueOuter
captureLoopkeeps spinning even ifGetBuffererrors out.If
GetBufferfails (line 362), webreakthe inner while, but the outerwhile (!stopRequested_)just sleeps 5ms and triesGetNextPacketSizeagain. Most transient HRESULTs will probably recover, but a sticky failure (device removal, format change, etc.) means we burn CPU spinning and silently produce no audio until the parent process notices and sendsstop. That's lowkey risky for the encoder timeline since video keeps moving.Minimal fix: track consecutive failures and bail (or surface the error to stderr + set a sentinel so
main.cppcan react). Even just propagating the device-removal case would help.Also, fwiw,
lastDevicePositionEnd_never resets across recovered failures, so if the device clock ever rolls back (e.g. on default-device switch), the gap-detection at line 369 silently swallows it. Edge case — probably fine for v1.
90-103: 💤 Low valuei18n nit: hardcoded English stopwords in
scoreDeviceName.The stopword filter (
L"microphone",L"mic",L"audio",L"input") only helps English-locale Windows installs. On a French/German/Japanese system, friendly names come back localized (e.g. "Microphone (Realtek)" in French is still "Microphone" but "Mikrofon" in German), so the heuristic silently does nothing useful and noisy filler words can dominate the score.Not a blocker — the exact-match and substring paths above (lines 75–82) usually win, and worst case we fall back to the default endpoint. Just flagging that name-based mic selection is implicitly English-biased.
🤖 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 1416-1421: The picker handler should re-run the existing
validation before calling approveFilePath: instead of directly calling
approveFilePath(result.filePaths[0]) in the open-video-file-picker handler, pass
the selected path through approveReadableVideoPath (or the same validation logic
used there) and only call approveFilePath if that validation returns success; if
validation fails, return { success: false, error: <validation message> } and do
not set currentProjectPath to null. Ensure you reference the
open-video-file-picker handler, approveFilePath, and approveReadableVideoPath
when making the change so the dialog cannot bless non-video or non-existent
paths.
- Around line 1729-1732: The handlers resolveVideoPath, readCursorRecordingFile,
and readCursorTelemetryFile currently accept a normalized videoPath directly,
bypassing the module's path approval checks; update resolveVideoPath to run
normalizeVideoSourcePath(videoPath ?? currentVideoPath) through the same
path-approval function used elsewhere in this file (the approval/check utility
used for other IPC reads) and only return or forward the approved path, and
change the cursor loaders (invocations of readCursorRecordingFile and
readCursorTelemetryFile) to receive the approved path; if approval fails, return
or throw a consistent error instead of calling the reader.
In `@electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts`:
- Around line 104-105: The await this.waitUntilReady() call in start() can
reject and currently leaves the spawned helper running; wrap the readiness await
in a try/catch, call the session's helper-teardown routine (e.g.,
this.teardownHelper() or the existing stop/kill/cleanup method that closes the
PowerShell helper process) inside the catch to ensure the helper is terminated,
then rethrow the error; apply the same pattern to the other readiness call site
around lines 260-267 so any failing waitUntilReady() always triggers the helper
cleanup before propagating the error.
In `@electron/native/wgc-capture/src/dshow_webcam_capture.cpp`:
- Around line 366-378: start() is not exception-safe: if std::thread constructor
throws after impl_->mediaControl->Run() and after impl_->running is set, the
graph remains running with no capture thread. Fix by making thread creation
exception-safe in DirectShowWebcamCapture::start(): after calling
impl_->mediaControl->Run(), set stopRequested_ and impl_->running only once the
thread is successfully created, or wrap the thread_ = std::thread(...) call in a
try/catch that on failure calls impl_->mediaControl->Stop(), clears
impl_->running/stopRequested_ as needed, and returns false so state is
consistent; reference impl_->mediaControl->Run(), impl_->running,
stopRequested_, and thread_.
- Around line 46-229: The listed helper functions (readPropertyString,
containsInsensitive, normalizeDeviceName, splitWords, deviceMatchScore,
readRegistryString, and findRegisteredVideoInput) are dead code; either wire
findRegisteredVideoInput into the DirectShow init/fallback path or
remove/deduplicate them: if registry-based fallback is intended, modify the
capture initialization code that resolves directShowClsid to call
findRegisteredVideoInput(deviceId, deviceName, selectedClsid, selectedName,
bestScore) when directShowClsid resolution fails and use
selectedClsid/selectedName as the resolved device; otherwise delete these helper
functions from dshow_webcam_capture.cpp and move any shared helpers into a
single shared utility used by webcam_capture.cpp and wasapi_loopback_capture.cpp
to avoid duplication.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 583-590: The startNativeWindowsRecordingIfAvailable function
currently throws an Error when isNativeWindowsCaptureAvailable reports
unsupported-os or missing-helper, preventing the caller from falling back to
getDisplayMedia; change the behavior so the function does not throw on those
availability failures but instead returns a clear "not available" result (e.g.,
false or null) so the caller can run the existing browser getDisplayMedia path;
update the same pattern in the other availability check block (the one at the
later occurrence) to mirror this fallback behavior and preserve throwing only
for unexpected fatal errors.
- Around line 398-441: finalizeNativeWindowsRecording currently nulls
nativeWindowsRecording.current and resets UI state before awaiting
window.electronAPI.stopNativeWindowsRecording, which can lose the active handle
on failure; change the flow so you only clear nativeWindowsRecording.current and
call setRecording/setPaused/setElapsedSeconds/reset
accumulatedDurationMs/segmentStartedAt after stopNativeWindowsRecording has
succeeded or the recording is discarded, and if stopNativeWindowsRecording
throws or returns a non-success keep nativeWindowsRecording.current intact and
reset activeNativeRecording.finalizing to false so the renderer can
retry/cancel; keep the initial check/setting of activeNativeRecording.finalizing
(in finalizeNativeWindowsRecording) but move the UI-reset and clearing of
discardRecordingId.current into the success/discard path or into finally only
when the stop completed successfully.
In `@tests/e2e/windows-native-checklist.spec.ts`:
- Around line 276-277: Replace the bare array return for the cursor.getTelemetry
stub with the real CursorTelemetryLoadResult-shaped object so callers see the
expected fields; specifically, in the branch checking request.domain ===
"cursor" && request.action === "getTelemetry" return success({ success: true,
samples: [] }) (or the equivalent production shape with a samples array) instead
of success([]).
---
Nitpick comments:
In `@electron/native/wgc-capture/src/audio_sample_utils.cpp`:
- Around line 347-409: AudioMixer::mixLoop currently re-paces output with
sleep_until(audioClockStart + emittedFrames_/sampleRate) which can delay
draining when upstream bursts backlog, and you also lack a cap on
systemQueue_/microphoneQueue_ sizes; change mixLoop to stop using long
wall-clock sleep for pacing (remove sleep_until and rely on the existing
cv_.wait_for and the real-time WASAPI feed / output_ timings) or replace it with
a short non-blocking yield (e.g., sleep_for a few ms) only when necessary, and
implement a bounded-queue policy for systemQueue_ and microphoneQueue_ (e.g.,
maxBytes = N_seconds * format_.sampleRate * format_.blockAlign) so producers
drop oldest bytes when pushing beyond the cap (implement drop-in-push in the
producer or in pop/push helpers), referencing AudioMixer::mixLoop,
audioClockStart, emittedFrames_, sleep_until, systemQueue_, microphoneQueue_,
pop(), includeSystem_/includeMicrophone_, and output_. Ensure queue-dropping is
thread-safe under mutex_ and that stopRequested_ and timelineStarted_ logic
remain unchanged.
- Around line 263-275: AudioMixer::start lacks a guard against being called when
a previous thread is still running; check thread_.joinable() at the top of
start() and bail (return false or assert) if true to avoid stomping an active
std::thread. Specifically, in AudioMixer::start() before mutating
stopRequested_, emittedFrames_, timelineStarted_ and launching the lambda that
calls mixLoop(), add a guard using thread_.joinable() to prevent starting a
second thread while the first is still active.
- Around line 200-241: mixAudioInPlace currently sums destination and source
samples then hard-clips, causing distortion when both are loud; modify each
mixing branch in mixAudioInPlace (float path detected by isFloatFormat, 16-bit
and 32-bit PCM paths detected by isPcmFormat) to apply a fixed attenuation/gain
to inputs before summing (e.g., multiply both samples by ~0.7071 or 0.5 for
headroom) and then add and clamp (use clampTo for integer paths and std::clamp
for float) so the sum preserves headroom and avoids hard clipping; ensure
conversions use floating math when applying the gain and keep existing clamping
behavior after summation.
In `@electron/native/wgc-capture/src/main.cpp`:
- Around line 601-632: The callbacks passed to microphoneCapture.start and
loopbackCapture.start currently drop timestampHns/durationHns and call
audioMixer->pushMicrophone/pushSystem with only raw bytes; update the call chain
to preserve and forward capture-side timestamps so the mixer can pace output
from the capture clock (instead of synthesizing via emittedFrames_). Concretely:
extend or add overloads on audioMixer (e.g., pushMicrophoneWithTimestamp /
pushSystemWithTimestamp or add timestamp parameters to
pushMicrophone/pushSystem) to accept timestampHns and durationHns, change the
lambdas in microphoneCapture.start and loopbackCapture.start to pass those
values through, and update the mixer logic (audio_sample_utils.cpp behavior that
uses emittedFrames_) to prefer capture timestamps and handle discontinuities
when timestamps jump or are missing. Ensure tests/usage that expect the old API
are updated and that silence-backfill logic remains compatible.
- Around line 126-242: The current helpers (findBool, findInt64, findInt,
findDouble, findString used by parseConfig) are fragile because they
substring-scan the whole JSON blob (wrong for nested keys) and findString
doesn't decode \uXXXX; replace these ad-hoc parsers with a proper JSON parser
(e.g. include nlohmann::json single-header), parse the input once into a
nlohmann::json object, and change
findBool/findInt64/findInt/findDouble/findString to look up keys from that
parsed top-level object (using .contains() and appropriate .get<T>() calls) so
nested fields won't collide and unicode escapes (including \uXXXX) are handled
by the library. Ensure error handling mirrors current fallbacks by returning the
provided fallback on parse or type errors.
In `@electron/native/wgc-capture/src/wasapi_loopback_capture.h`:
- Line 24: Change the capture callback typedef to return bool (match
AudioMixer::OutputCallback) and propagate that decision from the capture loop:
update using AudioCallback = std::function<bool(const BYTE* data, DWORD
byteCount, int64_t timestampHns, int64_t durationHns)> in
wasapi_loopback_capture.h, modify the captureLoop implementation in
wasapi_loopback_capture.cpp to check the callback return value and break/stop
the loop when it returns false, and adjust all callers (e.g., the lambda passed
from main.cpp and any other consumers) to return false when they want the
capture to stop (encoder failure, mixer stopped) and true to continue; ensure
stopRequested atomic handling remains consistent with the new early-exit
behavior.
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 1251-1329: Replace the hardcoded English labels in the new Cursor
accordion with translation keys using the existing i18n helper (call t("..."))
where the labels appear (the "Cursor" title inside AccordionTrigger, "Show
Cursor" near the Switch, and the field labels "Size", "Smoothing", "Motion
Blur", "Click Bounce" shown around the Slider components and value spans);
update the JSX in SettingsPanel (look for AccordionTrigger, AccordionContent,
Switch, Slider and the variables showCursor, cursorSize, cursorSmoothing,
cursorMotionBlur, cursorClickBounce) to call t(...) for each literal and then
add corresponding keys to all locale JSON files so the strings resolve in all
locales.
- Around line 1272-1327: The four cursor Sliders (value props using cursorSize,
cursorSmoothing, cursorMotionBlur, cursorClickBounce and change handlers
onCursorSizeChange, onCursorSmoothingChange, onCursorMotionBlurChange,
onCursorClickBounceChange) are missing onValueCommit callbacks and currently
cause downstream work on every drag tick; add onValueCommit handlers for each
Slider that call a corresponding commit prop (e.g., onCursorSizeCommit,
onCursorSmoothingCommit, onCursorMotionBlurCommit, onCursorClickBounceCommit)
with values[0] so expensive rendering/export work only runs on commit (keep
existing onValueChange for live UI feedback).
🪄 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: a8aafb2e-f190-41ac-a42c-a2d04f4bba31
📒 Files selected for processing (24)
electron/ipc/handlers.tselectron/ipc/nativeBridge.tselectron/main.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.script.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.tselectron/native-bridge/cursor/telemetryCursorAdapter.tselectron/native-bridge/services/projectService.tselectron/native/README.mdelectron/native/wgc-capture/src/audio_sample_utils.cppelectron/native/wgc-capture/src/audio_sample_utils.helectron/native/wgc-capture/src/dshow_webcam_capture.cppelectron/native/wgc-capture/src/main.cppelectron/native/wgc-capture/src/wasapi_loopback_capture.cppelectron/native/wgc-capture/src/wasapi_loopback_capture.hscripts/test-windows-wgc-helper.mjssrc/components/launch/LaunchWindow.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/projectPersistence.tssrc/components/video-editor/videoPlayback/cursorRenderer.tssrc/hooks/useScreenRecorder.tssrc/lib/exporter/frameRenderer.tssrc/lib/nativeWindowsRecording.tstests/e2e/windows-native-checklist.spec.ts
✅ Files skipped from review due to trivial changes (4)
- src/lib/nativeWindowsRecording.ts
- electron/main.ts
- src/lib/exporter/frameRenderer.ts
- electron/native/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- electron/native-bridge/services/projectService.ts
- electron/native-bridge/cursor/recording/windowsNativeRecordingSession.script.ts
- scripts/test-windows-wgc-helper.mjs
- electron/ipc/nativeBridge.ts
- src/components/video-editor/videoPlayback/cursorRenderer.ts
- src/components/video-editor/VideoEditor.tsx
There was a problem hiding this comment.
Actionable comments posted: 4
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/VideoEditor.tsx (1)
1934-1954:⚠️ Potential issue | 🟠 Major | ⚡ Quick winpreview is ignoring the saved cursor highlight settings now.
VideoPlaybackstill falls back toDEFAULT_CURSOR_HIGHLIGHTand[]when the parent omitscursorHighlight/cursorClickTimestamps. Since this call site stopped passing both props, preview no longer reflects the editor state even though export still useseffectiveCursorHighlight.small wiring fix
cursorRecordingData={cursorRecordingData} trimRegions={trimRegions} speedRegions={speedRegions} annotationRegions={annotationOnlyRegions} @@ onBlurDataChange={handleBlurDataPreviewChange} onBlurDataCommit={commitState} cursorTelemetry={cursorTelemetry} + cursorHighlight={effectiveCursorHighlight} + cursorClickTimestamps={cursorClickTimestamps} showCursor={showCursor} cursorSize={cursorSize} cursorSmoothing={cursorSmoothing}🤖 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/VideoEditor.tsx` around lines 1934 - 1954, The preview stopped reflecting saved cursor highlight because VideoPlayback is no longer being passed the cursorHighlight and cursorClickTimestamps props; update the VideoPlayback call (the component receiving props like cursorRecordingData, trimRegions, etc.) to pass cursorHighlight={effectiveCursorHighlight} and cursorClickTimestamps={cursorClickTimestamps} (or the corresponding state/prop names used in this file) so the preview uses the editor's effective settings rather than falling back to DEFAULT_CURSOR_HIGHLIGHT/[].
♻️ Duplicate comments (1)
electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts (1)
106-107:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReadiness failure can still orphan the helper process.
lowkey risky:
start()awaits readiness, but timeout/rejection only rejects the promise and doesn’t guarantee teardown. If caller drops the session on error, the helper can keep running.Suggested minimal fix
async start(): Promise<void> { @@ - await this.waitUntilReady(); + try { + await this.waitUntilReady(); + } catch (error) { + const normalizedError = + error instanceof Error ? error : new Error(String(error)); + this.failHelper(normalizedError); + throw normalizedError; + } } @@ private waitUntilReady() { return new Promise<void>((resolve, reject) => { this.readyResolve = resolve; this.readyReject = reject; this.readyTimer = setTimeout(() => { - this.rejectReady(new Error("Timed out waiting for Windows cursor helper readiness")); + this.failHelper( + new Error("Timed out waiting for Windows cursor helper readiness"), + ); }, READY_TIMEOUT_MS); }); }Also applies to: 249-255
🤖 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/windowsNativeRecordingSession.ts` around lines 106 - 107, start() currently awaits waitUntilReady() but if that promise times out or rejects the helper process can be orphaned; change start() (and the other call site that awaits waitUntilReady around lines 249-255) to ensure cleanup on failure by wrapping the await in try/catch/finally (or using Promise.race/AbortController) and calling the session teardown method (e.g., this.stop(), this.dispose(), or the helper-kill routine) in the catch/finally path so the helper process is always terminated when readiness fails.
🤖 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 `@src/components/video-editor/VideoEditor.tsx`:
- Around line 154-159: cursorClickTimestamps has been hardcoded to an empty
array which disables click-driven emphasis on macOS; change it to derive
timestamps conditionally instead: keep the same inputs (videoSourcePath,
useCursorTelemetry, useCursorRecordingData) and compute cursorClickTimestamps
based on available data (prefer cursorRecordingData if present, otherwise
extract click events from cursorTelemetry), and only fall back to an empty array
when neither source provides click timestamps; update the derivation referenced
by effectiveCursorHighlight and preview/export paths so mac projects get the
platform-specific fallback rather than a hardcoded [] (use the symbols
cursorClickTimestamps, useCursorTelemetry, useCursorRecordingData,
cursorRecordingData, cursorTelemetry, videoSourcePath/fromFileUrl, and
effectiveCursorHighlight to locate and change the logic).
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 382-390: The fallback that treats any positive video.currentTime
as the full duration in resolveCurrentDuration is unsafe (forceResolveDuration
sets currentTime to 0.1); change the check so we only accept currentTime as the
duration when playback has actually reached the end: i.e., require video.ended
=== true OR that video.duration is finite and video.currentTime >=
video.duration - EPSILON (or matches seekable end), then call onDurationChange;
otherwise ignore small positive currentTime values. Update
resolveCurrentDuration (and related logic that relies on
lastResolvedDurationRef/onDurationChange) to use this stricter check.
In `@src/lib/cursor/nativeCursor.ts`:
- Around line 291-315: getNativeCursorClickBounceProgress (and the similar
helper at lines 429-448) currently reverse-scans recordingData.samples on every
frame which causes O(samples × frames) perf; replace the per-frame linear tail
scan with a binary-search helper that finds the last sample with sample.timeMs
<= timeMs (or maintain a monotonic cursor index when iterating sequential
frames) and use that index to compute ageMs and the click-bounce value; update
resolveInterpolatedNativeCursorFrame and getNativeCursorClickBounceProgress to
call the new binary-search (or use the monotonic index) so semantics (handling
null/undefined recordingData, NATIVE_CURSOR_CLICK_ANIMATION_MS threshold, and
interactionType === "click") remain the same while avoiding repeated full
reverse scans called by drawNativeCursor and the VideoPlayback ticker.
In `@src/lib/exporter/frameRenderer.ts`:
- Around line 599-606: When projectNativeCursorToLocal(...) returns null we only
reset motion blur; also reset the cursor smoothing state so exports don't ease
in from stale off-screen positions. In the same branch where
resetNativeCursorMotionBlurState(this.nativeCursorMotionBlurState) is called,
also clear the smoothing state by calling the smoothing reset used elsewhere
(e.g. resetNativeCursorSmoothingState(this.nativeCursorSmoothingState) or
otherwise clear the smoothing buffer/last-position stored on
this.nativeCursorSmoothingState) so both motion blur and smoothing are reset
when the cursor falls outside the crop.
---
Outside diff comments:
In `@src/components/video-editor/VideoEditor.tsx`:
- Around line 1934-1954: The preview stopped reflecting saved cursor highlight
because VideoPlayback is no longer being passed the cursorHighlight and
cursorClickTimestamps props; update the VideoPlayback call (the component
receiving props like cursorRecordingData, trimRegions, etc.) to pass
cursorHighlight={effectiveCursorHighlight} and
cursorClickTimestamps={cursorClickTimestamps} (or the corresponding state/prop
names used in this file) so the preview uses the editor's effective settings
rather than falling back to DEFAULT_CURSOR_HIGHLIGHT/[].
---
Duplicate comments:
In `@electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts`:
- Around line 106-107: start() currently awaits waitUntilReady() but if that
promise times out or rejects the helper process can be orphaned; change start()
(and the other call site that awaits waitUntilReady around lines 249-255) to
ensure cleanup on failure by wrapping the await in try/catch/finally (or using
Promise.race/AbortController) and calling the session teardown method (e.g.,
this.stop(), this.dispose(), or the helper-kill routine) in the catch/finally
path so the helper process is always terminated when readiness fails.
🪄 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: 99cef220-8320-4f1c-a307-ab49414c1eee
📒 Files selected for processing (11)
electron/native-bridge/cursor/recording/windowsNativeRecordingSession.script.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.types.tsscripts/test-windows-native-cursor.mjssrc/components/video-editor/VideoEditor.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/lib/cursor/nativeCursor.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/gifExporter.tssrc/lib/exporter/videoExporter.tssrc/native/contracts.ts
✅ Files skipped from review due to trivial changes (1)
- src/native/contracts.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/exporter/videoExporter.ts
- scripts/test-windows-native-cursor.mjs
- electron/native-bridge/cursor/recording/windowsNativeRecordingSession.script.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
scripts/test-windows-native-cursor.mjs (2)
6-12: 💤 Low value
Number(process.env.X ?? default)will happily produceNaNand bake it into PowerShell.If anyone sets e.g.
CURSOR_TEST_SAMPLE_INTERVAL_MS=fastin CI, you getNaN, and thenStart-Sleep -Milliseconds NaN(line 322/469) fails inside PS, which throws under$ErrorActionPreference = 'Stop'and the sampler dies immediately with a not-super-obvious error. A tiny clamp/validate at the top would save someone a 2am debugging session.diff
-const SAMPLE_INTERVAL_MS = Number(process.env.CURSOR_TEST_SAMPLE_INTERVAL_MS ?? 25); -const DURATION_MS = Number(process.env.CURSOR_TEST_DURATION_MS ?? 1800); -const SCREEN_FRAME_INTERVAL_MS = Number(process.env.CURSOR_TEST_SCREEN_FRAME_INTERVAL_MS ?? 100); +function readPositiveIntEnv(name, fallback) { + const raw = process.env[name]; + if (raw === undefined) return fallback; + const parsed = Number(raw); + if (!Number.isFinite(parsed) || parsed <= 0) { + throw new Error(`Invalid ${name}=${raw}; expected positive number`); + } + return Math.floor(parsed); +} + +const SAMPLE_INTERVAL_MS = readPositiveIntEnv("CURSOR_TEST_SAMPLE_INTERVAL_MS", 25); +const DURATION_MS = readPositiveIntEnv("CURSOR_TEST_DURATION_MS", 1800); +const SCREEN_FRAME_INTERVAL_MS = readPositiveIntEnv("CURSOR_TEST_SCREEN_FRAME_INTERVAL_MS", 100);🤖 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 `@scripts/test-windows-native-cursor.mjs` around lines 6 - 12, The numeric environment parsing for SAMPLE_INTERVAL_MS, DURATION_MS, SCREEN_FRAME_INTERVAL_MS (and READY_TIMEOUT_MS if desired) can yield NaN when env vars are non-numeric; validate and clamp each parsed value immediately after creation (e.g., check Number.isFinite(value) and fall back to the intended default or a safe minimum/maximum) and emit a warning if an env value was invalid so downstream PowerShell calls (Start-Sleep) never receive NaN; update the initialization around SAMPLE_INTERVAL_MS, DURATION_MS, SCREEN_FRAME_INTERVAL_MS to perform these checks and fallback logic and ensure OUTPUT_DIR handling remains unchanged.
476-492: 💤 Low value
waitForReadypolls instead of resolving from the stream; nit: cleaner.The current 25ms
setIntervalover a sharedeventsarray works, but it's a polling loop tied to a buffer that's mutated elsewhere. A small "ready" promise resolved directly fromonStdoutwould remove the timer/race-y interplay and make the timeout the only wall-clock concern. Totally optional — current code is correct, just kinda cursed 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 `@scripts/test-windows-native-cursor.mjs` around lines 476 - 492, Replace the polling loop in waitForReady with a promise that is resolved directly when the stdout handler pushes a "ready" event: change waitForReady to return a new Promise that sets a single timeout (setTimeout) to reject after READY_TIMEOUT_MS and exposes a local resolve function; remove setInterval. In the stdout handler (the function that currently mutates events, e.g., onStdout or wherever events.push(...) happens) call that resolve when you detect an event.type === "ready" so the promise resolves immediately; ensure the timeout is cleared on resolve to avoid leaks. Keep the existing rejection message and behavior but switch polling to this direct-resolution pattern.electron/native-bridge/cursor/recording/windowsNativeRecordingSession.types.ts (1)
4-21: 💤 Low valuenit:
assetshould beWindowsCursorAssetPayload | nullfor consistency.The PowerShell sampler emits
asset = $nullon every sample where no asset is captured (seescripts/test-windows-native-cursor.mjsline 332/360), soJSON.parsewill produceasset: nullrather than absent.boundson line 19 already correctly allows| null, butasseton line 20 is just optional. Consumers happen to use optional chaining (payload.asset?.id) so it works, but the type lowkey lies about what comes off the wire.diff
- asset?: WindowsCursorAssetPayload; + asset?: WindowsCursorAssetPayload | null;🤖 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/windowsNativeRecordingSession.types.ts` around lines 4 - 21, The WindowsCursorSampleEvent type's asset field is declared optional but should explicitly allow null; update the WindowsCursorSampleEvent interface to change the asset property from optional (asset?) to a non-optional field typed as WindowsCursorAssetPayload | null (i.e., asset: WindowsCursorAssetPayload | null) so it matches incoming JSON from the PowerShell sampler, and then grep for uses of WindowsCursorSampleEvent.payload.asset to ensure callers handle null (or adjust code to use optional chaining if needed).
🤖 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 `@scripts/test-windows-native-cursor.mjs`:
- Around line 1025-1044: The current findPlaywrightChromiumExecutable uses
lexicographic sort on chromium-* directory names which misorders numeric
revisions; change the candidate selection to extract the numeric revision from
each entry.name (the suffix after "chromium-"), convert to integer (handle
non-numeric safely), sort candidates by that numeric revision descending, then
pick the highest numeric candidate (falling back to defaultPath). Update the
mapping/filtration logic around candidates in findPlaywrightChromiumExecutable
so it pairs each path with its parsed revision number, sorts by that number (not
by string), and returns the highest-version path or defaultPath if none valid.
- Around line 1100-1118: The handler that parses stdout lines calls
JSON.parse(trimmed) directly which can throw and crash the sampler; wrap that
parse in a try/catch (or otherwise validate the trimmed string is valid JSON)
inside the stdout data handler so malformed or non-JSON lines are skipped (and
optionally logged with the `[cursor-native-test]` prefix), and only push to
events/update assets when parsing succeeds — reference the JSON.parse(trimmed)
call, the events array, and the assets Map in the stdout handler and mirror the
defensive filtering used in onStderr for non-JSON/CLIXML lines.
---
Nitpick comments:
In
`@electron/native-bridge/cursor/recording/windowsNativeRecordingSession.types.ts`:
- Around line 4-21: The WindowsCursorSampleEvent type's asset field is declared
optional but should explicitly allow null; update the WindowsCursorSampleEvent
interface to change the asset property from optional (asset?) to a non-optional
field typed as WindowsCursorAssetPayload | null (i.e., asset:
WindowsCursorAssetPayload | null) so it matches incoming JSON from the
PowerShell sampler, and then grep for uses of
WindowsCursorSampleEvent.payload.asset to ensure callers handle null (or adjust
code to use optional chaining if needed).
In `@scripts/test-windows-native-cursor.mjs`:
- Around line 6-12: The numeric environment parsing for SAMPLE_INTERVAL_MS,
DURATION_MS, SCREEN_FRAME_INTERVAL_MS (and READY_TIMEOUT_MS if desired) can
yield NaN when env vars are non-numeric; validate and clamp each parsed value
immediately after creation (e.g., check Number.isFinite(value) and fall back to
the intended default or a safe minimum/maximum) and emit a warning if an env
value was invalid so downstream PowerShell calls (Start-Sleep) never receive
NaN; update the initialization around SAMPLE_INTERVAL_MS, DURATION_MS,
SCREEN_FRAME_INTERVAL_MS to perform these checks and fallback logic and ensure
OUTPUT_DIR handling remains unchanged.
- Around line 476-492: Replace the polling loop in waitForReady with a promise
that is resolved directly when the stdout handler pushes a "ready" event: change
waitForReady to return a new Promise that sets a single timeout (setTimeout) to
reject after READY_TIMEOUT_MS and exposes a local resolve function; remove
setInterval. In the stdout handler (the function that currently mutates events,
e.g., onStdout or wherever events.push(...) happens) call that resolve when you
detect an event.type === "ready" so the promise resolves immediately; ensure the
timeout is cleared on resolve to avoid leaks. Keep the existing rejection
message and behavior but switch polling to this direct-resolution pattern.
🪄 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: 42c3641f-1ebe-441e-a7cf-1ec0a2f42a1e
📒 Files selected for processing (4)
electron/native-bridge/cursor/recording/windowsNativeRecordingSession.script.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.types.tsscripts/test-windows-native-cursor.mjs
✅ Files skipped from review due to trivial changes (1)
- electron/native-bridge/cursor/recording/windowsNativeRecordingSession.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/native-bridge/cursor/recording/windowsNativeRecordingSession.script.ts
|
Addressed the non-inline nitpicks from the latest CodeRabbit summary in
Validation run:
|
|
Addressed the two latest non-inline CodeRabbit notes in
Validation run:
|
|
Addressed the older unresolved CodeRabbit thread group in
One thread was verified as not applicable: the E2E native bridge mock for Validation run:
|
|
@siddharthvaddem all tests ok on my side on windows. |
|
madlad 🐐 taking a look |
EtienneLescot
force-pushed
the
feat/cursor-pipeline
branch
from
764f60d to
0720a6d
Compare
|
@siddharthvaddem @FabLrc There was one risky area during the rebase: the editor UI and IPC wiring had diverged from upstream, so I restored the native cursor wiring explicitly in Validation run after the rebase:
Residual risk is mainly around the Windows editor cursor panel / preview / export integration because those files had the most overlap with upstream changes. I would manually re-check a short Windows editable-cursor recording, the cursor controls, and exports with and without the editable cursor enabled. |
# Conflicts: # src/components/video-editor/VideoEditor.tsx
3 participants
Description
This PR adds a Windows-only native recording path for OpenScreen. The goal is to avoid the preview/export cursor drift caused by splitting capture, cursor telemetry, audio, webcam, and muxing across Electron/browser APIs.
On Windows, recordings are routed through a bundled native Windows Graphics Capture helper. macOS and Linux behavior is intentionally unchanged in this PR.
What changed
electron/native/bin/win32-x64/wgc-capture.exe.asarUnpack.Windows-only scope
This PR does not add native capture for macOS or Linux. Windows-only cursor/capture controls are hidden outside Windows where applicable.
Linux remains on the existing Electron capture path. macOS behavior is not changed by this PR.
Validation run
Passed locally on Windows:
npm run build-vitenpm run build:winnpm run test:e2e:windows-native-checklistnpm run test:cursor-native:winnpm run test:wgc-helper:winnpm run test:wgc-window:winnpm run test:wgc-audio:winnpm run test:wgc-mic:winnpm run test:wgc-mixed-audio:winnpm run test:wgc-webcam:winOPENSCREEN_WGC_TEST_WEBCAM_DEVICE_NAME="NVIDIA Broadcast" npm run test:wgc-webcam:winOPENSCREEN_WGC_TEST_WEBCAM_DEVICE_NAME="NVIDIA Broadcast" npm run test:wgc-full:winnpx vitest --run src/lib/cursorTelemetryBuffer.test.tsKnown repo-level checks still needing separate cleanup:
npm run lintcurrently reports broad existing Biome formatting/line-ending issues outside the native-capture work.npm testhas existing failures in blur effects, tutorial translations, and browser exporter fixture/canvas handling.npm run test:e2ecurrently times out intests/e2e/gif-export.spec.ts.Maintainer test checklist
Capture + Launch
Audio
Editor Load + Playback
Timeline
Project Persistence
Other
Notes for reviewers
The checked items above are the paths covered by local Windows automation and manual iteration on this branch. The unchecked items are still useful manual QA targets before considering the PR ready to merge.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores