fix(cursor): make the Windows cursor sampler DPI-aware - #278
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds a headless Electron CLI, timestamp-based compositor playback, chunked transcription with progress and cancellation, native audio peaks, teleprompter controls, timeline geometry updates, export handling, localized strings, and Nix/FFmpeg tooling changes. ChangesPlayback and media timing
Headless CLI
Editor and supporting tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (12)
src/hooks/useScreenRecorder.ts-58-59 (1)
58-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd package coverage for
startRecordingImmediately.This method is consumed by
CliRecordRunnerand bypasses the countdown-overlay flow, but no test covers it. Add a test that calls this method and verifies capture starts without using countdown-overlay IPC.🤖 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 58 - 59, Add package-level test coverage for useScreenRecorder’s startRecordingImmediately method, invoking it through the hook or its public API and verifying capture starts without sending or relying on countdown-overlay IPC. Use the existing recording and IPC test helpers and preserve the normal countdown flow tests.Source: Coding guidelines
crates/compositor/src/pipeline_windows.rs-607-608 (1)
607-608: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnref the lookahead frame when a reposition discards it.
All five sites invalidate a pending peek by setting
has_peek = falseonly. TheAVFramekeeps its picture buffers. They are released later, when the nextavcodec_receive_frametargetspeek_frameand unrefs the destination first. If no further peek happens, the frame stays resident for the life of the decoder.The impact differs by backend. On the CPU and Linux paths this holds system memory, roughly 3 MB for 1080p YUV420P. On the Windows D3D11VA path it holds a texture from the fixed-size decoder surface pool, and
seek_toruns on every scrub step. That is the site most likely to matter.Make the invalidation complete at each site:
crates/compositor/src/pipeline_windows.rs#L607-L608: inseek_to, callav_frame_unref(self.peek_frame)before clearinghas_peek, so the pooled D3D11 surface returns to the decoder.crates/compositor/src/pipeline_windows.rs#L585-L589: apply the same unref inrewind.crates/compositor/src/pipeline_macos.rs#L254-L256: apply the same unref inseek_to, usingcrate::ffi::av_frame_unref.crates/compositor/src/pipeline_macos.rs#L219-L224: apply the same unref inrewind.crates/compositor/src/linux_decode.rs#L326-L327: apply the same unref indecode_at.A small private helper on each decoder keeps the three implementations aligned:
/// Invalide le peek en attente ET rend ses buffers — `has_peek = false` seul /// laisse la frame référencée jusqu'au prochain `avcodec_receive_frame`. unsafe fn drop_peek(&mut self) { if self.has_peek { av_frame_unref(self.peek_frame); self.has_peek = 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 `@crates/compositor/src/pipeline_windows.rs` around lines 607 - 608, Pending peek invalidation must unreference the AVFrame buffers, not only clear has_peek. In crates/compositor/src/pipeline_windows.rs lines 607-608 (seek_to) and 585-589 (rewind), crates/compositor/src/pipeline_macos.rs lines 254-256 (seek_to) and 219-224 (rewind), and crates/compositor/src/linux_decode.rs lines 326-327 (decode_at), add or reuse a private decoder helper that calls the appropriate av_frame_unref on peek_frame before clearing has_peek, and invoke it at each site.electron/stt/index.test.ts-161-170 (1)
161-170: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the comment: the failure lands on chunk 1, not chunk 2.
Line 162 uses
mockRejectedValue, so every attempt of every chunk rejects.transcribeChunkexhaustsCHUNK_ATTEMPTSon the first chunk andtranscribethrows before it reaches chunk 2. The assertion on line 168 confirms this by matchingchunk 1/3.The comment on lines 165-166 states the failure is on chunk 2 of 3. It contradicts the assertion directly below it.
📝 Proposed comment correction
- // 200s in, the failure is on chunk 2 of 3 — "Transcription failed" alone - // tells the user nothing about a recording this long. + // Every attempt rejects, so the run dies on chunk 1 of 3 — "Transcription + // failed" alone tells the user nothing about a recording this long.🤖 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/stt/index.test.ts` around lines 161 - 170, Correct the explanatory comment in the test around SttManager.transcribe to state that the request fails on chunk 1 of 3 after all attempts reject, matching the existing assertion and mockRejectedValue behavior.electron/media/audioPeaks.test.ts-43-50 (1)
43-50: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore
OPENSCREEN_FFMPEG_PATHwithdeleteinstead of assigningundefined.
process.env.values are stringified. Line 48 leavesffmpegCandidates()preferring"undefined"for later tests in the same worker. Capture the previous value and restore it, or usedelete process.env.OPENSCREEN_FFMPEG_PATHif it was unset.🤖 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/media/audioPeaks.test.ts` around lines 43 - 50, Update the environment cleanup in the “honours the env override first” test to restore OPENSCREEN_FFMPEG_PATH correctly instead of assigning undefined. Capture its prior value and restore it after the assertion, or delete the property when it was originally unset, ensuring later ffmpegCandidates calls do not see the string "undefined".src/components/ai-edition/v4/V4Timeline.geometry.test.tsx-88-105 (1)
88-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRegister lifecycle cleanup in this test.
renderTimeline()callsrender()without invoking React Testing Librarycleanup()inafterEach, and the Vitest config does not register cleanup globally. Add importcleanupfrom@testing-library/reactandafterEach(cleanup), or callcleanup()from the helper return context.🤖 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/ai-edition/v4/V4Timeline.geometry.test.tsx` around lines 88 - 105, Update the V4Timeline geometry test setup around renderTimeline to register React Testing Library cleanup by importing cleanup and adding afterEach(cleanup), ensuring each render is unmounted between tests.src/lib/captioning/transcribe.ts-83-89 (1)
83-89: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject a resolved transcription result if the caller already aborted.
api.cancel()sendsstt:cancel, butstt:transcribecan still deliver the pending successful result afteronabortruns. Reject before mappingapi.transcribe()’sresult, and add a same-package test where the controller aborts before a mocked transcription resolves.🤖 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/captioning/transcribe.ts` around lines 83 - 89, Update the transcription flow around api.transcribe() so an already-aborted options.signal rejects before the successful result is mapped, even if api.cancel() was called. Preserve normal result mapping when the signal remains active, and add a same-package test that aborts the controller before the mocked transcription resolves.Source: Coding guidelines
src/i18n/locales/tr/common.json-46-49 (1)
46-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the standard Turkish unit name
piksel.
piksis not the standard Turkish spelling forpixel. Replace the current labels withpiksel/snandpiksel.Proposed translation
- "pixelsPerSecond": "{{value}} piks/sn", - "pixels": "{{value}} piks" + "pixelsPerSecond": "{{value}} piksel/sn", + "pixels": "{{value}} piksel"🤖 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/common.json` around lines 46 - 49, Update the units translations in the units object, replacing “piks/sn” with “piksel/sn” and “piks” with “piksel” while preserving the {{value}} placeholders.electron/cli/cliMain.ts-406-418 (1)
406-418: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe final
elseprints a recording message for any non-record command.The chain covers
sourcesonly whenresult.sourcesis set. A successfulsourcesresult without asourcespayload falls into the finalelseand printsRecording saved → undefined. Make the last branch explicit forrecord.🔧 Proposed change
} else if (command.kind === "export") { output.info(`Exported ${result.format ?? ""} → ${result.outputPath}`); - } else { + } else if (command.kind === "record") { output.info(`Recording saved → ${result.screenVideoPath}`); if (result.cursorDataPath) output.info(`Cursor data → ${result.cursorDataPath}`); if (result.projectPath) output.info(`Project → ${result.projectPath}`); }🤖 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/cli/cliMain.ts` around lines 406 - 418, Update the result-reporting conditional around the command-kind checks so the final recording and cursor/project output executes only when command.kind is "record". Preserve the existing sources, captions, and export branches, and ensure a sources command without result.sources does not print recording output or undefined paths.electron/ipc/handlers.ts-385-401 (1)
385-401: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe sibling fallback does not work across platforms.
path.basenameuses only the host separator. A project packed on Windows stores paths such asC:\packed\rec.mp4. On Linux,path.basenamereturns that whole string, the join produces a bogus path,statfails, and the original path is returned. The packed-project promise printed byrunPackCommand("the loader falls back to files next to the project") then does not hold for a folder moved between platforms, which is the main reasonopenscreen packexists. Split on both separators.🔧 Proposed fix
+ // A packed project may carry paths written on another platform, so the + // basename must not depend on the host separator. + const portableBasename = (p: string): string => p.split(/[\\/]/).pop() ?? p; + const resolveWithSiblingFallback = async (mediaPath: string): Promise<string> => { if (!projectFilePath) return mediaPath; const exists = await fs .stat(mediaPath) .then((stats) => stats.isFile()) .catch(() => false); if (exists) return mediaPath; const sibling = path.join( path.dirname(path.resolve(projectFilePath)), - path.basename(mediaPath), + portableBasename(mediaPath), );🤖 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 385 - 401, Update resolveWithSiblingFallback to derive the media filename using both Windows and POSIX path separators before joining it with the project directory. Preserve the existing direct-file check and sibling fallback behavior, ensuring packed paths such as C:\packed\rec.mp4 resolve to rec.mp4 when loaded on another platform.electron/cli/cliMain.ts-79-99 (1)
79-99: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winProgress text says "Exporting" for every command.
cli-progressis a generic channel. Therecordandcaptionsrunners can also send progress events. The human-readable line then prints "Exporting 40% [transcribing]", which is wrong for those commands. Derive the verb fromcommand.kind, or pass a label intocreateOutput.🔧 Proposed change
-function createOutput(json: boolean): CliOutput { +function createOutput(json: boolean, verb = "Working"): CliOutput { const isTty = process.stdout.isTTY === true; @@ - const text = `Exporting ${Math.round(p.percentage)}%${frames}${eta}${phase}`; + const text = `${verb} ${Math.round(p.percentage)}%${frames}${eta}${phase}`;Then pass the verb at the call site:
- const output = createOutput(command.json === true); + const verb = { export: "Exporting", record: "Recording", captions: "Captioning" }[ + command.kind as string + ]; + const output = createOutput(command.json === true, verb ?? "Working");🤖 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/cli/cliMain.ts` around lines 79 - 99, Update the progress formatting in createOutput to derive the human-readable verb from command.kind instead of hardcoding “Exporting,” covering record and captions progress events while preserving the existing percentage, frame, ETA, phase, and TTY/non-TTY behavior.src/lib/cliContracts.ts-21-31 (1)
21-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the orphan doc comment.
Lines 21-25 document a "reference preview box" field that does not exist in
CliExportRequest. The next doc comment at lines 26-30 documentsautoZoom. A reader sees two consecutive doc blocks for one field.♻️ Proposed cleanup
- /** - * Reference preview box used to scale annotation text and border radii the - * same way the editor's on-screen preview does. The composition is fitted - * into this box, mirroring the editor layout. Defaults to 1280x720. - */ /** * Add automatic zoom regions derived from cursor-dwell telemetry (same * suggestion engine as the editor's magic wand) before rendering. Existing * zoom regions are preserved; suggestions never overlap them. */ autoZoom: boolean;🤖 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/cliContracts.ts` around lines 21 - 31, Remove the orphan “reference preview box” documentation block immediately before the autoZoom field, leaving the existing comment that documents autoZoom unchanged.src/cli/CliCaptionsRunner.tsx-64-89 (1)
64-89: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd cancellation for CLI captions transcription.
transcribeMono16kToSegmentsonly callsapi.cancel?.()when asignalaborts, and the CLI captions flow does not set up a stop target. Duringopenscreen captions, an interrupt leaves the main-process whisper helper running until the current chunk finishes. Add a captions stop channel/API path (or pass an existing abort signal) into both transcription calls.🤖 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/cli/CliCaptionsRunner.tsx` around lines 64 - 89, Update the CLI captions flow around transcribeMono16kToSegments to create or reuse an AbortSignal/stop channel connected to the command’s interrupt handling, and pass it in both the trimmed and untrimmed transcription calls. Ensure cancellation reaches the renderer’s whisper helper through the signal so an interrupt stops the active transcription instead of waiting for the chunk to finish.
🧹 Nitpick comments (10)
src/components/launch/NotesToolbar.test.tsx (1)
218-223: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the control query to the teleprompter row.
rowis asserted non-null and then never used.controlsis queried fromcontainer, so the test passes even if adata-teleprompter-controlbutton renders outside the teleprompter row. Query the controls fromrowto make the containment assertion real and to remove the unused variable.♻️ Proposed change
const row = container.querySelector<HTMLElement>('[data-testid="notes-teleprompter-controls"]'); + expect(row).not.toBeNull(); const controls = Array.from( - container.querySelectorAll<HTMLButtonElement>("[data-teleprompter-control]"), + (row as HTMLElement).querySelectorAll<HTMLButtonElement>("[data-teleprompter-control]"), ); - expect(row).not.toBeNull(); expect(controls).toHaveLength(6);🤖 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/NotesToolbar.test.tsx` around lines 218 - 223, Update the test around the teleprompter row query to remove the unused row variable while retaining the non-null assertion, then query data-teleprompter-control buttons from the confirmed teleprompter row element rather than the container. Keep the expected control count of six.crates/compositor/src/timeline_walk.rs (1)
113-129: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a commit cap to the advance loop.
The
Commitbranch has no bound. It stops only when a peeked pts exceeds the target, or at EOF, or on a null frame.
NextFrameTime::Unknowncovers the case wherebest_effort_timestampisi64::MIN. It does not cover a stream whose timestamps parse successfully but never advance. For a source where every frame reports pts 0,frame_stepreturnsCommitfor every peek, and a single call toadvance_decoder_todrains the whole file to produce one output frame.The previous
cur_time_sec() < targetloop had the same unbounded shape, so this is not a regression. A cap makes the invariant explicit and keeps one bad source from stalling an export.♻️ Proposed change
if decoder.cur_frame().is_null() { return Ok(false); } - loop { + // Un flux dont les pts n'avancent pas (tous à 0, par ex.) rendrait `Commit` à + // chaque peek : sans plafond, UNE frame de sortie viderait tout le fichier. + const MAX_COMMITS_PER_TARGET: u32 = 1_000; + let mut commits = 0u32; + loop { let next = decoder.peek_next_time_sec()?; match frame_step(next, timeline_offset_sec, target_source_time) { FrameStep::Hold => return Ok(true), FrameStep::Commit => { if decoder.commit_peek()?.is_null() { return Ok(false); } + commits += 1; + if commits >= MAX_COMMITS_PER_TARGET { + return Ok(true); + } } FrameStep::CommitAndStop => { return Ok(!decoder.commit_peek()?.is_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 `@crates/compositor/src/timeline_walk.rs` around lines 113 - 129, Bound the Commit path in the advance loop by tracking the number of committed frames and stopping once a defined cap is reached. Update the loop around frame_step and decoder.commit_peek so sources with non-advancing timestamps cannot drain the entire file in one advance_decoder_to call, while preserving existing Hold, CommitAndStop, EOF, and null-frame behavior.crates/compositor/src/linux_decode.rs (1)
326-327: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRelease the stale lookahead buffer when a seek discards it.
decode_atclearshas_peekbut leaves the decoded picture referenced bypeek_frame. The buffers stay held until the nextavcodec_receive_frame(self.dctx, self.peek_frame)unrefs them. On a scrub that seeks repeatedly and never peeks again, one full frame stays resident. Add an explicit unref to make the invalidation complete.♻️ Proposed change
- // Tout seek invalide un éventuel peek en attente — cf. pipeline_macos::Decoder::seek_to. - self.has_peek = false; + // Tout seek invalide un éventuel peek en attente — cf. pipeline_macos::Decoder::seek_to. + if self.has_peek { + av_frame_unref(self.peek_frame); + self.has_peek = 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 `@crates/compositor/src/linux_decode.rs` around lines 326 - 327, Update the seek invalidation logic in decode_at to explicitly unref the stale decoded picture held by peek_frame when clearing has_peek. Ensure both the lookahead state and its referenced frame buffer are released immediately, while preserving normal peek behavior after subsequent seeks.crates/poc-d3d/src/app.rs (1)
140-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport
consume_accinstead of copying its arithmetic here.Lines 140-144 reimplement
crates/compositor/src/live.rs::consume_accexactly, including theafter < beforeloop-reset case. That function was extracted in this PR specifically to make the arithmetic testable, and three tests now lock its behavior.
consume_accispub(crate), sopoc-d3dcannot call it today. Widening it topubputs both callers on one tested implementation and stops the two copies from drifting.♻️ Proposed change
In
crates/compositor/src/live.rs:-pub(crate) fn consume_acc(acc: f64, before: f64, after: f64) -> f64 { +pub fn consume_acc(acc: f64, before: f64, after: f64) -> f64 {In
crates/poc-d3d/src/app.rs:stepped = true; let after = self.player.screen_time_sec(); - self.acc = if after >= before { - (self.acc - (after - before)).max(0.0) - } else { - 0.0 // reboucle sur l'EOF (temps qui recule) : accumulateur remis à zéro. - }; + self.acc = consume_acc(self.acc, before, after);Confirm that
consume_accis re-exported from the crate root before using this import path.🤖 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 `@crates/poc-d3d/src/app.rs` around lines 140 - 144, Replace the inline accumulator arithmetic in the relevant app update flow with a call to compositor::consume_acc, preserving the existing accumulator, before, and after values. Widen consume_acc in live.rs from pub(crate) to pub, re-export it from the compositor crate root, and import it in app.rs through that root path so both callers use the tested implementation.electron/cli/args.ts (1)
112-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLeading switches that take a separate value silently disable CLI mode.
The loop stops at the first token that does not start with
-. ForOpenscreen --user-data-dir /tmp/x export demo.openscreen, the loop stops on/tmp/x. That token is not inSUBCOMMANDS, soparseCliArgsreturnsnulland the GUI launches instead. Only valueless switches such as--no-sandboxwork. Scanning for the first token present inSUBCOMMANDSremoves the arity assumption.♻️ Proposed refactor
- let subIndex = 0; - while ( - subIndex < rawArgs.length && - rawArgs[subIndex].startsWith("-") && - rawArgs[subIndex] !== "--help" && - rawArgs[subIndex] !== "-h" - ) { - subIndex++; - } - - const args = rawArgs.slice(subIndex); - const sub = args[0]; - if (!sub || !SUBCOMMANDS.has(sub)) return null; + // Chromium/Electron switches may carry a separate value token, so skipping + // only dash-prefixed tokens is not enough. Find the subcommand itself. + const subIndex = rawArgs.findIndex((token) => SUBCOMMANDS.has(token)); + if (subIndex === -1) return null; + const args = rawArgs.slice(subIndex); + const sub = args[0];🤖 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/cli/args.ts` around lines 112 - 128, Update the argument preprocessing in parseCliArgs to scan rawArgs for the first token present in SUBCOMMANDS rather than stopping when a non-dash token appears. Start subcommand parsing at that recognized token so leading switches with separate values, such as --user-data-dir /tmp/x, do not disable CLI mode; preserve the existing --help and -h handling.src/cli/vendor/leadingSilence.ts (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd colocated tests for the vendored module.
This module was vendored because the original was deleted, so its original tests are gone.
trimLeadingSilenceMono16kandshiftTrimRegionsMsForCaptionBuffernow carry the caption timing contract for the CLI, and a timing regression here shifts every caption. Addsrc/cli/vendor/leadingSilence.test.tscovering the all-silent buffer, the pre-roll clamp at index 0, and a trim region that straddles the trim point. The siblingsrc/cli/captionAnnotations.test.tsshows the expected placement.As per coding guidelines: "Add a test for every new behavior in the same package as the code under test" and "Place unit tests next to the source they test and use Vitest with the jsdom environment."
🤖 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/cli/vendor/leadingSilence.ts` around lines 1 - 4, Add colocated Vitest tests in leadingSilence.test.ts for trimLeadingSilenceMono16k and shiftTrimRegionsMsForCaptionBuffer, covering an all-silent buffer, pre-roll clamping at index 0, and a trim region that straddles the trim point. Follow the placement and jsdom test setup used by captionAnnotations.test.ts.Source: Coding guidelines
src/lib/exporter/voiceoverMix.ts (1)
63-77: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe silent catch hides a real decode failure.
The comment covers the expected case of a video with no audio track. The same catch also swallows a genuine decode error for a video that does have audio. The user then gets an MP4 with the voiceover only and no indication that the original bed was dropped, even though they asked for
--audio-mode mix. The CLI already has a warnings channel (result.warnings). Return the condition to the caller so it can emit a warning.♻️ Proposed change
async function renderMixedAudio( videoData: ArrayBuffer | null, durationSec: number, options: VoiceoverMixOptions, -): Promise<AudioBuffer> { +): Promise<{ audio: AudioBuffer; originalDropped: boolean }> { @@ + let originalDropped = false; if (options.mode === "mix" && videoData) { try { @@ } catch { // The exported video has no decodable audio track; the voiceover // becomes the only audio, same as "replace". + originalDropped = true; } } - return context.startRendering(); + return { audio: await context.startRendering(), originalDropped }; }🤖 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/exporter/voiceoverMix.ts` around lines 63 - 77, Update the mix handling around decodeToBuffer and the enclosing exporter flow so decode failures are returned to the caller rather than silently swallowed; preserve the expected no-audio-track fallback while exposing genuine decode failures through result.warnings, including that the original audio bed was omitted in mix mode.src/cli/CliRecordRunner.tsx (1)
153-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ascasts oncliGetRequest()defeat thekindguards in both runners. Each runner casts the union result to one concrete member and then checksrequest.kind. The cast already asserts the member the check is meant to prove, so the check no longer narrows and the error branch interpolates a literal that cannot occur.CliSourcesRunnerandCliCaptionsRunnernarrow directly fromCliRequestand need no cast.
src/cli/CliRecordRunner.tsx#L153-L156: removeas CliRecordRequestand let therequest.kind !== "record"check narrow the value; theCliRecordRequestimport then serves onlyrequestRefandpickSource.src/cli/CliExportRunner.tsx#L340-L343: removeas CliExportRequestand let therequest.kind !== "export"check narrow the value beforerunExport(request).🤖 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/cli/CliRecordRunner.tsx` around lines 153 - 156, Remove the concrete-member cast from the cliGetRequest() result in CliRecordRunner.tsx around lines 153-156, allowing the request.kind check to narrow the union; retain the CliRecordRequest import for requestRef and pickSource. Apply the same change in CliExportRunner.tsx around lines 340-343 by removing the CliExportRequest cast before the kind guard and runExport(request).src/cli/captionAnnotations.test.ts (1)
22-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the word-count options.
CliCaptionsRunnerpassesminWordsPerCaptionandmaxWordsPerCaptionstraight from user arguments intocaptionSegmentsToAnnotationRegions, and the runner relies on a relaxed retry when the first grouping yields nothing (src/cli/CliCaptionsRunner.tsx lines 108-119). Neither test exercises those options. Add a case that sets an explicitmaxWordsPerCaptionand asserts the resulting line lengths.🤖 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/cli/captionAnnotations.test.ts` around lines 22 - 35, Add a test alongside the existing phrase-granularity case that passes an explicit maxWordsPerCaption value to captionSegmentsToAnnotationRegions and asserts the resulting regions’ content or word counts are split at that limit. Cover the word-count option directly while preserving the existing one-line phrase behavior.Source: Coding guidelines
src/cli/CliExportRunner.tsx (1)
292-316: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffVoiceover mixing holds the whole export in renderer memory.
The path loads the exported MP4 as a Blob (line 295), converts it to an ArrayBuffer inside
mixVoiceoverIntoVideo, produces a mixed Blob, converts that to another ArrayBuffer (line 310), and sends it over IPC. Several copies of the full file are resident at the same time.The CLI is the path most likely to process long unattended recordings, so the renderer heap limit is reachable here. Consider mixing in the main process with a streaming muxer, or document a supported input length.
🤖 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/cli/CliExportRunner.tsx` around lines 292 - 316, Move voiceover mixing out of the renderer-side `CliExportRunner` flow, replacing the Blob/ArrayBuffer-based `mixVoiceoverIntoVideo` and IPC write path with a main-process streaming mux operation that reads from `outPath` and `request.audioPath` and writes the mixed result directly to `outPath`. Preserve the existing progress reporting, audio options, and error propagation while avoiding full-file copies in renderer memory.
🤖 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 @.github/workflows/nix-check.yml:
- Line 45: Update the actions/checkout@v4 step in the pull-request workflow to
set persist-credentials to false, ensuring the read-only job does not leave
GITHUB_TOKEN in the repository’s Git configuration.
In `@electron/cli/cliMain.ts`:
- Around line 184-187: Update writeProjectFile to write JSON to a uniquely named
temporary file in the same directory as projectOut, then rename that temporary
file to projectOut after the write completes. Ensure cleanup of the temporary
file on failure, preserving atomic replacement and avoiding direct truncation of
the source file.
In `@electron/cli/projectCommands.ts`:
- Around line 72-85: Update the cursor sidecar handling near newScreenPath so
its destination is derived from the deduplicated screen video path, using the
`<newScreenPath>.cursor.json` contract rather than allowing
copyIn(cursorSidecar) to use the source basename. Preserve the existing
source-sidecar existence check and ensure the packed project links telemetry to
newScreenPath.
In `@electron/stt/index.ts`:
- Around line 183-201: Update transcribeChunk to accept the captured cancel
epoch and check it before each retry attempt and during retry backoff, aborting
when it no longer matches. Pass epoch from the transcribe call site, and update
its catch wrapper to rethrow AbortError unchanged instead of converting
cancellation into a generic chunk-failure error.
In `@scripts/fetch-ffmpeg.mjs`:
- Around line 460-468: Update the reuse check in the fetch flow before the copy
block to also require the path.join(binDir, "ffmpeg-shared.exe") file alongside
the shared DLLs and SDK. If that executable is missing, continue the setup so
the existing copyFileSync call copies it during incremental builds.
In `@src/cli/CliExportRunner.tsx`:
- Around line 156-169: Update runExport’s session-selection logic to retrieve
and use the approved recording session for request.projectPath, rather than
blindly applying the current global session after currentProjectPath is cleared.
Only replace the project’s stored media paths when the requested project’s
session is invalid, and preserve the existing fallback to project-file paths
when no valid matching session is available.
- Around line 88-98: Update the export builders in CliExportRunner and
ExportDialog so webcamPath uses a camera source only when cam exists, is
visible, and has sourcePath; otherwise use the same empty-string fallback as
buildSceneDescription. Apply this rule consistently in both builders without
changing other clip fields.
In `@src/hooks/useAudioPeaks.ts`:
- Around line 150-165: Update loadPeaks and the associated effect to distinguish
in-flight requests by both videoUrl and durationSec, so duration-less and
duration-aware work cannot share a promise; ensure durationSec changes trigger
the effect and allow the correct native or streaming route to start. Keep
completed peaksCache entries keyed only by videoUrl.
In `@src/lib/exporter/voiceoverMix.ts`:
- Around line 40-46: Update decodeToBuffer and its videoData call site so the
one-time-read exported video buffer is passed directly to
context.decodeAudioData without data.slice(0), allowing it to be detached.
Preserve the existing copy for options.voiceoverData because that buffer belongs
to the caller.
---
Minor comments:
In `@crates/compositor/src/pipeline_windows.rs`:
- Around line 607-608: Pending peek invalidation must unreference the AVFrame
buffers, not only clear has_peek. In crates/compositor/src/pipeline_windows.rs
lines 607-608 (seek_to) and 585-589 (rewind),
crates/compositor/src/pipeline_macos.rs lines 254-256 (seek_to) and 219-224
(rewind), and crates/compositor/src/linux_decode.rs lines 326-327 (decode_at),
add or reuse a private decoder helper that calls the appropriate av_frame_unref
on peek_frame before clearing has_peek, and invoke it at each site.
In `@electron/cli/cliMain.ts`:
- Around line 406-418: Update the result-reporting conditional around the
command-kind checks so the final recording and cursor/project output executes
only when command.kind is "record". Preserve the existing sources, captions, and
export branches, and ensure a sources command without result.sources does not
print recording output or undefined paths.
- Around line 79-99: Update the progress formatting in createOutput to derive
the human-readable verb from command.kind instead of hardcoding “Exporting,”
covering record and captions progress events while preserving the existing
percentage, frame, ETA, phase, and TTY/non-TTY behavior.
In `@electron/ipc/handlers.ts`:
- Around line 385-401: Update resolveWithSiblingFallback to derive the media
filename using both Windows and POSIX path separators before joining it with the
project directory. Preserve the existing direct-file check and sibling fallback
behavior, ensuring packed paths such as C:\packed\rec.mp4 resolve to rec.mp4
when loaded on another platform.
In `@electron/media/audioPeaks.test.ts`:
- Around line 43-50: Update the environment cleanup in the “honours the env
override first” test to restore OPENSCREEN_FFMPEG_PATH correctly instead of
assigning undefined. Capture its prior value and restore it after the assertion,
or delete the property when it was originally unset, ensuring later
ffmpegCandidates calls do not see the string "undefined".
In `@electron/stt/index.test.ts`:
- Around line 161-170: Correct the explanatory comment in the test around
SttManager.transcribe to state that the request fails on chunk 1 of 3 after all
attempts reject, matching the existing assertion and mockRejectedValue behavior.
In `@src/cli/CliCaptionsRunner.tsx`:
- Around line 64-89: Update the CLI captions flow around
transcribeMono16kToSegments to create or reuse an AbortSignal/stop channel
connected to the command’s interrupt handling, and pass it in both the trimmed
and untrimmed transcription calls. Ensure cancellation reaches the renderer’s
whisper helper through the signal so an interrupt stops the active transcription
instead of waiting for the chunk to finish.
In `@src/components/ai-edition/v4/V4Timeline.geometry.test.tsx`:
- Around line 88-105: Update the V4Timeline geometry test setup around
renderTimeline to register React Testing Library cleanup by importing cleanup
and adding afterEach(cleanup), ensuring each render is unmounted between tests.
In `@src/hooks/useScreenRecorder.ts`:
- Around line 58-59: Add package-level test coverage for useScreenRecorder’s
startRecordingImmediately method, invoking it through the hook or its public API
and verifying capture starts without sending or relying on countdown-overlay
IPC. Use the existing recording and IPC test helpers and preserve the normal
countdown flow tests.
In `@src/i18n/locales/tr/common.json`:
- Around line 46-49: Update the units translations in the units object,
replacing “piks/sn” with “piksel/sn” and “piks” with “piksel” while preserving
the {{value}} placeholders.
In `@src/lib/captioning/transcribe.ts`:
- Around line 83-89: Update the transcription flow around api.transcribe() so an
already-aborted options.signal rejects before the successful result is mapped,
even if api.cancel() was called. Preserve normal result mapping when the signal
remains active, and add a same-package test that aborts the controller before
the mocked transcription resolves.
In `@src/lib/cliContracts.ts`:
- Around line 21-31: Remove the orphan “reference preview box” documentation
block immediately before the autoZoom field, leaving the existing comment that
documents autoZoom unchanged.
---
Nitpick comments:
In `@crates/compositor/src/linux_decode.rs`:
- Around line 326-327: Update the seek invalidation logic in decode_at to
explicitly unref the stale decoded picture held by peek_frame when clearing
has_peek. Ensure both the lookahead state and its referenced frame buffer are
released immediately, while preserving normal peek behavior after subsequent
seeks.
In `@crates/compositor/src/timeline_walk.rs`:
- Around line 113-129: Bound the Commit path in the advance loop by tracking the
number of committed frames and stopping once a defined cap is reached. Update
the loop around frame_step and decoder.commit_peek so sources with non-advancing
timestamps cannot drain the entire file in one advance_decoder_to call, while
preserving existing Hold, CommitAndStop, EOF, and null-frame behavior.
In `@crates/poc-d3d/src/app.rs`:
- Around line 140-144: Replace the inline accumulator arithmetic in the relevant
app update flow with a call to compositor::consume_acc, preserving the existing
accumulator, before, and after values. Widen consume_acc in live.rs from
pub(crate) to pub, re-export it from the compositor crate root, and import it in
app.rs through that root path so both callers use the tested implementation.
In `@electron/cli/args.ts`:
- Around line 112-128: Update the argument preprocessing in parseCliArgs to scan
rawArgs for the first token present in SUBCOMMANDS rather than stopping when a
non-dash token appears. Start subcommand parsing at that recognized token so
leading switches with separate values, such as --user-data-dir /tmp/x, do not
disable CLI mode; preserve the existing --help and -h handling.
In `@src/cli/captionAnnotations.test.ts`:
- Around line 22-35: Add a test alongside the existing phrase-granularity case
that passes an explicit maxWordsPerCaption value to
captionSegmentsToAnnotationRegions and asserts the resulting regions’ content or
word counts are split at that limit. Cover the word-count option directly while
preserving the existing one-line phrase behavior.
In `@src/cli/CliExportRunner.tsx`:
- Around line 292-316: Move voiceover mixing out of the renderer-side
`CliExportRunner` flow, replacing the Blob/ArrayBuffer-based
`mixVoiceoverIntoVideo` and IPC write path with a main-process streaming mux
operation that reads from `outPath` and `request.audioPath` and writes the mixed
result directly to `outPath`. Preserve the existing progress reporting, audio
options, and error propagation while avoiding full-file copies in renderer
memory.
In `@src/cli/CliRecordRunner.tsx`:
- Around line 153-156: Remove the concrete-member cast from the cliGetRequest()
result in CliRecordRunner.tsx around lines 153-156, allowing the request.kind
check to narrow the union; retain the CliRecordRequest import for requestRef and
pickSource. Apply the same change in CliExportRunner.tsx around lines 340-343 by
removing the CliExportRequest cast before the kind guard and runExport(request).
In `@src/cli/vendor/leadingSilence.ts`:
- Around line 1-4: Add colocated Vitest tests in leadingSilence.test.ts for
trimLeadingSilenceMono16k and shiftTrimRegionsMsForCaptionBuffer, covering an
all-silent buffer, pre-roll clamping at index 0, and a trim region that
straddles the trim point. Follow the placement and jsdom test setup used by
captionAnnotations.test.ts.
In `@src/components/launch/NotesToolbar.test.tsx`:
- Around line 218-223: Update the test around the teleprompter row query to
remove the unused row variable while retaining the non-null assertion, then
query data-teleprompter-control buttons from the confirmed teleprompter row
element rather than the container. Keep the expected control count of six.
In `@src/lib/exporter/voiceoverMix.ts`:
- Around line 63-77: Update the mix handling around decodeToBuffer and the
enclosing exporter flow so decode failures are returned to the caller rather
than silently swallowed; preserve the expected no-audio-track fallback while
exposing genuine decode failures through result.warnings, including that the
original audio bed was omitted in mix mode.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2168d88a-e2ab-4844-83ec-eedf8a229d3f
⛔ Files ignored due to path filters (1)
electron/media/__fixtures__/peaks-sample.m4ais excluded by!**/*.m4a
📒 Files selected for processing (113)
.github/workflows/bump-nix-package.yml.github/workflows/nix-check.yml.gitignoreREADME.mdcrates/compositor/src/linux_decode.rscrates/compositor/src/live.rscrates/compositor/src/pipeline_linux.rscrates/compositor/src/pipeline_macos.rscrates/compositor/src/pipeline_windows.rscrates/compositor/src/timeline_walk.rscrates/poc-d3d/src/app.rsdocs/cli.mdelectron/cli/args.test.tselectron/cli/args.tselectron/cli/cliMain.tselectron/cli/projectCommands.test.tselectron/cli/projectCommands.tselectron/electron-env.d.tselectron/ipc/handlers.tselectron/main.tselectron/media/audioPeaks.test.tselectron/media/audioPeaks.tselectron/native-bridge/cursor/recording/windowsNativeRecordingSession.tselectron/native/wgc-capture/src/cursor-sampler.cppelectron/preload.tselectron/stt/chunking.test.tselectron/stt/chunking.tselectron/stt/index.test.tselectron/stt/index.tselectron/stt/transcriptionContract.tselectron/stt/whisperServer.tselectron/windows.tsnix/package.nixpackage.jsonscripts/fetch-ffmpeg-macos.mjsscripts/fetch-ffmpeg.mjssrc/App.tsxsrc/cli/CliCaptionsRunner.tsxsrc/cli/CliExportRunner.tsxsrc/cli/CliRecordRunner.tsxsrc/cli/CliSourcesRunner.tsxsrc/cli/captionAnnotations.test.tssrc/cli/captionAnnotations.tssrc/cli/vendor/leadingSilence.tssrc/cli/vendor/zoomHelpers.tssrc/components/ai-edition/ExportDialog.tsxsrc/components/ai-edition/RightPanes.tsxsrc/components/ai-edition/TranscriptionStatus.tsxsrc/components/ai-edition/v4/EditorShellV4.module.csssrc/components/ai-edition/v4/MediaStage.tsxsrc/components/ai-edition/v4/V4Timeline.geometry.test.tsxsrc/components/ai-edition/v4/V4Timeline.tsxsrc/components/launch/NotesToolbar.test.tsxsrc/components/launch/NotesToolbar.tsxsrc/components/launch/NotesWindow.editable.test.tsxsrc/components/launch/NotesWindow.module.csssrc/components/launch/NotesWindow.test.tsxsrc/components/launch/NotesWindow.tsxsrc/components/launch/notesTeleprompter.test.tssrc/components/launch/notesTeleprompter.tssrc/hooks/useAudioPeaks.test.tssrc/hooks/useAudioPeaks.tssrc/hooks/useScreenRecorder.tssrc/i18n/locales/ar/common.jsonsrc/i18n/locales/ar/editor.jsonsrc/i18n/locales/ar/launch.jsonsrc/i18n/locales/en/common.jsonsrc/i18n/locales/en/editor.jsonsrc/i18n/locales/en/launch.jsonsrc/i18n/locales/es/common.jsonsrc/i18n/locales/es/editor.jsonsrc/i18n/locales/es/launch.jsonsrc/i18n/locales/fr/common.jsonsrc/i18n/locales/fr/editor.jsonsrc/i18n/locales/fr/launch.jsonsrc/i18n/locales/it/common.jsonsrc/i18n/locales/it/editor.jsonsrc/i18n/locales/it/launch.jsonsrc/i18n/locales/ja-JP/common.jsonsrc/i18n/locales/ja-JP/editor.jsonsrc/i18n/locales/ja-JP/launch.jsonsrc/i18n/locales/ko-KR/common.jsonsrc/i18n/locales/ko-KR/editor.jsonsrc/i18n/locales/ko-KR/launch.jsonsrc/i18n/locales/pt-BR/common.jsonsrc/i18n/locales/pt-BR/editor.jsonsrc/i18n/locales/pt-BR/launch.jsonsrc/i18n/locales/ru/common.jsonsrc/i18n/locales/ru/editor.jsonsrc/i18n/locales/ru/launch.jsonsrc/i18n/locales/tr/common.jsonsrc/i18n/locales/tr/editor.jsonsrc/i18n/locales/tr/launch.jsonsrc/i18n/locales/vi/common.jsonsrc/i18n/locales/vi/editor.jsonsrc/i18n/locales/vi/launch.jsonsrc/i18n/locales/zh-CN/common.jsonsrc/i18n/locales/zh-CN/editor.jsonsrc/i18n/locales/zh-CN/launch.jsonsrc/i18n/locales/zh-TW/common.jsonsrc/i18n/locales/zh-TW/editor.jsonsrc/i18n/locales/zh-TW/launch.jsonsrc/lib/ai-edition/document/transcribe.tssrc/lib/ai-edition/store/transcriptionStore.tssrc/lib/ai-edition/transcription/status.test.tssrc/lib/ai-edition/transcription/status.tssrc/lib/captioning/transcribe.test.tssrc/lib/captioning/transcribe.tssrc/lib/cliContracts.tssrc/lib/exporter/mp4ExportSettings.test.tssrc/lib/exporter/mp4ExportSettings.tssrc/lib/exporter/voiceoverMix.tstechnical-documentation/architecture/editor-shell.md
| name: npmDepsHash matches package-lock.json | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository workflow files:"
git ls-files '.github/workflows/**'
echo
echo "Target workflow relevant contents:"
if [ -f .github/workflows/nix-check.yml ]; then
nl -ba .github/workflows/nix-check.yml | sed -n '1,140p'
else
echo "missing .github/workflows/nix-check.yml"
fi
echo
echo "Search for checkout persistence or token usage in target workflow:"
rg -n "actions/checkout|persist-credentials|GITHUB_TOKEN|permissions:|pull_request|push:" .github/workflows/nix-check.yml || trueRepository: getopenscreen/openscreen
Length of output: 852
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Workflow structure and nix-check.yml contents:"
printf '\n--- git diff --stat HEAD~1..HEAD .github/workflows/nix-check.yml (if available) ---\n'
git diff --stat HEAD~1..HEAD .github/workflows/nix-check.yml || true
printf '\n--- nix-check.yml lines 1-140 ---\n'
awk '{ printf "%6d %s\n", NR, $0 }' .github/workflows/nix-check.yml | sed -n '1,140p'
printf '\n--- focused search in target workflow ---\n'
grep -InE "actions/checkout|persist-credentials|GITHUB_TOKEN|permissions:|pull_request|push:" .github/workflows/nix-check.yml || true
printf '\n--- nearby checkout uses in repository ---\n'
grep -InE "actions/checkout|persist-credentials: false|persist-credentials: true" .github/workflows || trueRepository: getopenscreen/openscreen
Length of output: 3898
🌐 Web query:
actions/checkout persist-credentials default true README
💡 Result:
In the actions/checkout GitHub Action, the persist-credentials input has a default value of true [1][2]. When set to true (the default), the action configures the authentication token or SSH key within the local git configuration [1][2]. This allows your workflow scripts to execute authenticated git commands (such as git fetch, git push, or git pull) automatically without requiring additional setup [1][3][4]. The token is removed during the post-job cleanup phase [1][5][6]. If you wish to opt out of this behavior, you can explicitly set persist-credentials to false in your workflow file [1][3]: - persist-credentials: false Recent versions of actions/checkout (beginning with v6.0.0) have improved security by storing these credentials in a separate file under the $RUNNER_TEMP directory, rather than directly in the repository's.git/config file, which helps prevent potential credential leakage [3][4][7].
Citations:
- 1: https://github.com/actions/checkout
- 2: https://raw.githubusercontent.com/actions/checkout/v6/action.yml
- 3: https://github.com/actions/checkout/tree/v6.0.2?tab=readme-ov-file
- 4: https://github.com/actions/checkout/?tab=readme-ov-file
- 5: https://github.com/marketplace/actions/checkout?version=v3.6.0
- 6: https://github.com/actions/checkout/blob/24cb9080177205b6e8c946b17badbe402adc938f/README.md
- 7: https://github.com/actions/checkout/tree/v6.0.0
Security And Privacy (CWE-522): Insufficiently Protected Credentials
Reachability: External · Exploitability: Moderate
Disable checkout credential persistence in this pull-request job.
This job runs pull_request code after checkout, including third-party action and Nix tooling. actions/checkout@v4 keeps GITHUB_TOKEN in .git/config by default; contents: read does not prevent the token from being read.
Set persist-credentials: false since the job only reads files and does not push.
Proposed fix
- uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 45-45: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/nix-check.yml at line 45, Update the actions/checkout@v4
step in the pull-request workflow to set persist-credentials to false, ensuring
the read-only job does not leave GITHUB_TOKEN in the repository’s Git
configuration.
Source: Linters/SAST tools
There was a problem hiding this comment.
Outdated on this PR: this file is no longer part of the diff. The branch was cut from a pre-merge snapshot of work that has since landed on main under different SHAs, so the review ran against ~100 files of already-merged code. Rebased onto current main -- the PR is now two files, neither of them this one.
The finding is not dismissed on merit; it targets code already on main and needs its own issue. See #278 (comment) for the full disposition.
There was a problem hiding this comment.
@EtienneLescot, acknowledged. This finding is outside the current PR after the rebase.
The finding remains valid for the workflow on main. I will not treat it as addressed or withdrawn in this PR.
Would you like me to create a separate GitHub issue for setting persist-credentials: false in .github/workflows/nix-check.yml?
🐇
You are interacting with an AI system.
| async function writeProjectFile(projectOut: string, projectData: unknown): Promise<void> { | ||
| await fs.mkdir(path.dirname(projectOut), { recursive: true }); | ||
| await fs.writeFile(projectOut, JSON.stringify(projectData, null, 2), "utf8"); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Write the project file atomically.
openscreen captions <project.openscreen> passes the user's own input project as projectOut (line 394), so this function overwrites the source file in place. fs.writeFile truncates first. If the process dies during the write, the user's project file is destroyed. The repository already uses temp+rename for project saves for this reason (see the DocumentService note in electron/ipc/handlers.ts). Write to a temporary file in the same directory, then rename.
🛡️ Proposed fix
async function writeProjectFile(projectOut: string, projectData: unknown): Promise<void> {
- await fs.mkdir(path.dirname(projectOut), { recursive: true });
- await fs.writeFile(projectOut, JSON.stringify(projectData, null, 2), "utf8");
+ const dir = path.dirname(projectOut);
+ await fs.mkdir(dir, { recursive: true });
+ // Temp+rename: `captions` overwrites the user's own project file in place,
+ // so a partial write must never be observable.
+ const temp = path.join(dir, `.${path.basename(projectOut)}.${process.pid}.tmp`);
+ await fs.writeFile(temp, JSON.stringify(projectData, null, 2), "utf8");
+ await fs.rename(temp, projectOut);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function writeProjectFile(projectOut: string, projectData: unknown): Promise<void> { | |
| await fs.mkdir(path.dirname(projectOut), { recursive: true }); | |
| await fs.writeFile(projectOut, JSON.stringify(projectData, null, 2), "utf8"); | |
| } | |
| async function writeProjectFile(projectOut: string, projectData: unknown): Promise<void> { | |
| const dir = path.dirname(projectOut); | |
| await fs.mkdir(dir, { recursive: true }); | |
| // Temp+rename: `captions` overwrites the user's own project file in place, | |
| // so a partial write must never be observable. | |
| const temp = path.join(dir, `.${path.basename(projectOut)}.${process.pid}.tmp`); | |
| await fs.writeFile(temp, JSON.stringify(projectData, null, 2), "utf8"); | |
| await fs.rename(temp, projectOut); | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 185-185: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFile(projectOut, JSON.stringify(projectData, null, 2), "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/cli/cliMain.ts` around lines 184 - 187, Update writeProjectFile to
write JSON to a uniquely named temporary file in the same directory as
projectOut, then rename that temporary file to projectOut after the write
completes. Ensure cleanup of the temporary file on failure, preserving atomic
replacement and avoiding direct truncation of the source file.
There was a problem hiding this comment.
Outdated on this PR: this file is no longer part of the diff. The branch was cut from a pre-merge snapshot of work that has since landed on main under different SHAs, so the review ran against ~100 files of already-merged code. Rebased onto current main -- the PR is now two files, neither of them this one.
The finding is not dismissed on merit; it targets code already on main and needs its own issue. See #278 (comment) for the full disposition.
There was a problem hiding this comment.
@EtienneLescot, acknowledged. This finding is out of scope for the rebased PR because electron/cli/cliMain.ts is no longer changed here.
The atomic-write concern still applies to the version on main. I will leave this review finding unresolved for this PR. Would you like me to open a follow-up GitHub issue for the writeProjectFile atomic replacement change?
You are interacting with an AI system.
| const screenSource = await resolveSource(screenVideoPath); | ||
| const newScreenPath = await copyIn(screenSource); | ||
|
|
||
| let newWebcamPath: string | undefined; | ||
| if (media.webcamVideoPath) { | ||
| newWebcamPath = await copyIn(await resolveSource(media.webcamVideoPath)); | ||
| } | ||
|
|
||
| // Cursor telemetry sidecar sits at "<video path>.cursor.json". | ||
| const cursorSidecar = `${screenSource}.cursor.json`; | ||
| const hasCursorData = await isFile(cursorSidecar); | ||
| if (hasCursorData) { | ||
| await copyIn(cursorSidecar); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The cursor sidecar can lose its link to the renamed screen video.
The sidecar contract is <screen video path>.cursor.json. electron/ipc/handlers.ts writes and reads it with exactly that rule. Here, copyIn names the sidecar from its own source basename. If the dedupe loop at line 62 renamed the screen video to <stem>-1.mp4, the sidecar is written as <stem>.mp4.cursor.json while the packed project points at <stem>-1.mp4. The loader then finds no telemetry and the packed project silently loses its editable cursor. Derive the sidecar destination from newScreenPath.
🐛 Proposed fix
// Cursor telemetry sidecar sits at "<video path>.cursor.json".
const cursorSidecar = `${screenSource}.cursor.json`;
const hasCursorData = await isFile(cursorSidecar);
if (hasCursorData) {
- await copyIn(cursorSidecar);
+ // Must follow the copied video's final name, which copyIn may have
+ // de-duplicated; the loader derives the sidecar from the video path.
+ const sidecarDestination = `${newScreenPath}.cursor.json`;
+ if (path.resolve(cursorSidecar) !== path.resolve(sidecarDestination)) {
+ await fs.copyFile(cursorSidecar, sidecarDestination);
+ }
+ copied.push(sidecarDestination);
}🤖 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/cli/projectCommands.ts` around lines 72 - 85, Update the cursor
sidecar handling near newScreenPath so its destination is derived from the
deduplicated screen video path, using the `<newScreenPath>.cursor.json` contract
rather than allowing copyIn(cursorSidecar) to use the source basename. Preserve
the existing source-sidecar existence check and ensure the packed project links
telemetry to newScreenPath.
There was a problem hiding this comment.
Outdated on this PR: this file is no longer part of the diff. The branch was cut from a pre-merge snapshot of work that has since landed on main under different SHAs, so the review ran against ~100 files of already-merged code. Rebased onto current main -- the PR is now two files, neither of them this one.
The finding is not dismissed on merit; it targets code already on main and needs its own issue. See #278 (comment) for the full disposition.
There was a problem hiding this comment.
@EtienneLescot, acknowledged. This finding is outside the rebased PR scope because electron/cli/projectCommands.ts is no longer in the diff.
The cursor-sidecar naming issue remains a valid follow-up for code already on main. Would you like me to create a GitHub issue with the required change and acceptance criteria?
🐇🔎
You are interacting with an AI system.
| // The shared CLI too, beside the DLLs it links against. 1 MB, against | ||
| // the 109 MB of the static exe the installer excludes — and unlike that | ||
| // one, this is spawned at runtime: electron/media/audioPeaks.ts decodes | ||
| // waveform peaks with it, ~6x faster than either browser pipeline and | ||
| // off the UI process. Named apart from `ffmpeg.exe` on purpose, so the | ||
| // packager's `!win32-*/ffmpeg.exe` rule keeps dropping the static build | ||
| // while this one ships under the plain `win32-*/*` include. | ||
| fs.copyFileSync(exe, path.join(binDir, "ffmpeg-shared.exe")); | ||
| console.log(`Vendored ${libs.length} shared librar(ies) + ffmpeg-shared.exe -> ${binDir}`); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include ffmpeg-shared.exe in the reuse check.
The early-return condition at Line 394-404 checks only for shared DLLs and the SDK. After this change, an existing DLL/SDK directory can skip Lines 460-468, so ffmpeg-shared.exe is never copied. Waveform decoding then fails on an incremental build.
Require path.join(binDir, "ffmpeg-shared.exe") in the reuse condition, or copy the executable whenever it is missing.
Proposed fix
const sdkPresent = sdkDest == null || fs.existsSync(sdkDest);
- if (alreadyVendored && sdkPresent && !process.argv.includes("--force")) {
+ const sharedExePresent = fs.existsSync(path.join(binDir, "ffmpeg-shared.exe"));
+ if (alreadyVendored && sdkPresent && sharedExePresent && !process.argv.includes("--force")) {🤖 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/fetch-ffmpeg.mjs` around lines 460 - 468, Update the reuse check in
the fetch flow before the copy block to also require the path.join(binDir,
"ffmpeg-shared.exe") file alongside the shared DLLs and SDK. If that executable
is missing, continue the setup so the existing copyFileSync call copies it
during incremental builds.
There was a problem hiding this comment.
Outdated on this PR: this file is no longer part of the diff. The branch was cut from a pre-merge snapshot of work that has since landed on main under different SHAs, so the review ran against ~100 files of already-merged code. Rebased onto current main -- the PR is now two files, neither of them this one.
The finding is not dismissed on merit; it targets code already on main and needs its own issue. See #278 (comment) for the full disposition.
There was a problem hiding this comment.
@EtienneLescot, acknowledged. This finding is out of scope for the rebased PR and remains valid against code on main.
I will leave the finding unresolved here. Would you like me to create a follow-up GitHub issue for the incremental-build case where ffmpeg-shared.exe is missing but the DLLs and SDK already exist?
You are interacting with an AI system.
| // Prefer the main process's approved session paths: they carry the | ||
| // packed-project sibling fallback when the stored absolute paths are stale. | ||
| try { | ||
| const sessionResult = await window.electronAPI.getCurrentRecordingSession(); | ||
| const session = sessionResult?.session; | ||
| if (session?.screenVideoPath) { | ||
| media.screenVideoPath = session.screenVideoPath; | ||
| if (media.webcamVideoPath && session.webcamVideoPath) { | ||
| media.webcamVideoPath = session.webcamVideoPath; | ||
| } | ||
| } | ||
| } catch { | ||
| // Fall back to the paths stored in the project file. | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the session manifest fallback and how the GUI resolves stale project media paths.
set -euo pipefail
rg -n -C8 'getCurrentRecordingSession' --type=ts --type=tsx
fd -g 'projectPersistence*' --exec rg -n -C6 'resolveProjectMedia|sibling|stale' {} \;Repository: getopenscreen/openscreen
Length of output: 194
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate CliExportRunner =="
fd -a 'CliExportRunner\.tsx$' .
echo "== getCurrentRecordingSession usages =="
rg -n -C8 'getCurrentRecordingSession' --glob '*.ts' --glob '*.tsx'
echo "== projectPersistence related grep =="
fd -a 'projectPersistence.*|Session.*|SessionProvider|Session' . | rg -E 'projectPersistence|Session|sessions' | head -200
echo "== stale/sibling/resolveProjectMedia occurrences =="
rg -n -C6 'resolveProjectMedia|stale|sibling|packed-project|getCurrentRecordingSession|window\.electronAPI\.getCurrentRecordingSession' --glob '*.ts' --glob '*.tsx' .Repository: getopenscreen/openscreen
Length of output: 686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CliExportRunner relevant section =="
sed -n '120,190p' src/cli/CliExportRunner.tsx | cat -n
echo "== session API definitions/decorators =="
rg -n -C8 'getCurrentRecordingSession|export.*getCurrentRecordingSession|RecordingSession|recordingSession' --glob '*.ts' --glob '*.tsx' .
echo "== project media path resolution =="
rg -n -C6 'screenVideoPath|webcamVideoPath|media.*path|resolveProjectMedia|ProjectMedia|ProjectPath|paths.*media' --glob '*.ts' --glob '*.tsx' .Repository: getopenscreen/openscreen
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== projectPersistence outline =="
ast-grep outline src/components/video-editor/projectPersistence.ts --view expanded | sed -n '1,220p'
echo "== resolveProjectMedia implementation =="
rg -n -C40 'function resolveProjectMedia|const resolveProjectMedia|export .*resolveProjectMedia|async function|accessMedia|packed' src/components/video-editor/projectPersistence.ts
echo "== resolveProjectMedia tests =="
fd -a 'projectPersistence.*test.*|projectPersistence.test.*|projectPersistenceTests.*' . --exec sh -c 'echo "--- $1"; rg -n -C25 "resolveProjectMedia|packedProject|sibling|stale|screenVideoPath" "$1" || true' sh {}
echo "== session state initialization =="
rg -n -C12 'currentRecordingSession|setCurrentRecordingSessionState|get-current-recording-session' electron/ipc/handlers.tsRepository: getopenscreen/openscreen
Length of output: 24929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== getApprovedProjectSession implementation =="
rg -n -C60 'async function getApprovedProjectSession|function getApprovedProjectSession|getApprovedProjectSession' electron/ipc/handlers.ts src --glob '*.ts' --glob '*.tsx'
echo "== CliExportRunner request/project loading section =="
sed -n '1,156p' src/cli/CliExportRunner.tsx | cat -n
echo "== CLI runExport entrypoint =="
rg -n -C25 'runExport|projectPath|readFile|loadProjectFileFromPath|loadProjectFile' src/cli src electron/cli --glob '*.ts' --glob '*.tsx' | sed -n '1,220p'Repository: getopenscreen/openscreen
Length of output: 43931
Use the project’s approved session only if the requested project was loaded.
runExport clears currentProjectPath after loading request.projectPath. If another project had already set currentRecordingSession, this path still uses that unrelated session instead of the requested project’s approved media. Load this project’s approved session first and replace the project paths only when the project’s session is invalid.
🤖 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/cli/CliExportRunner.tsx` around lines 156 - 169, Update runExport’s
session-selection logic to retrieve and use the approved recording session for
request.projectPath, rather than blindly applying the current global session
after currentProjectPath is cleared. Only replace the project’s stored media
paths when the requested project’s session is invalid, and preserve the existing
fallback to project-file paths when no valid matching session is available.
There was a problem hiding this comment.
Outdated on this PR: this file is no longer part of the diff. The branch was cut from a pre-merge snapshot of work that has since landed on main under different SHAs, so the review ran against ~100 files of already-merged code. Rebased onto current main -- the PR is now two files, neither of them this one.
The finding is not dismissed on merit; it targets code already on main and needs its own issue. See #278 (comment) for the full disposition.
There was a problem hiding this comment.
@EtienneLescot, acknowledged. This finding is out of scope for the rebased PR because src/cli/CliExportRunner.tsx is no longer in its diff.
The finding remains valid for the code on main. I will leave this review comment unresolved. Would you like me to create a follow-up GitHub issue for the session-selection fix, with links to #278 and this discussion?
🐇⬛
You are interacting with an AI system.
| function loadPeaks(videoUrl: string, durationSec?: number): Promise<Float32Array> { | ||
| const existing = peaksInFlight.get(videoUrl); | ||
| if (existing) return existing; | ||
| // Deliberately NOT wired to any component's AbortSignal: the work is shared, | ||
| // so one subscriber unmounting must not cancel it for the others. An unmount | ||
| // drops the result instead — and the cache means the next mount is free. | ||
| const promise = computePeaksForUrl(videoUrl, undefined, durationSec) | ||
| .then((p) => { | ||
| peaksCache.set(videoUrl, p); | ||
| return p; | ||
| }) | ||
| .finally(() => { | ||
| peaksInFlight.delete(videoUrl); | ||
| }); | ||
| peaksInFlight.set(videoUrl, promise); | ||
| return promise; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not share a duration-less decode with a duration-aware request.
durationSec selects the native or streaming route, but peaksInFlight is keyed only by videoUrl. The effect also does not rerun when durationSec changes. If the first render has no duration, it can start the full browser decode. A later known duration cannot switch the request to native or streaming extraction.
For a long compressed recording, this defeats the decoded-memory limit and can cause a renderer memory spike or hang. Track the routing inputs for in-flight work, or defer local decoding until duration is available. Keep completed peaksCache entries keyed by URL because the final peaks remain reusable.
Also applies to: 194-204
🤖 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/useAudioPeaks.ts` around lines 150 - 165, Update loadPeaks and the
associated effect to distinguish in-flight requests by both videoUrl and
durationSec, so duration-less and duration-aware work cannot share a promise;
ensure durationSec changes trigger the effect and allow the correct native or
streaming route to start. Keep completed peaksCache entries keyed only by
videoUrl.
There was a problem hiding this comment.
Outdated on this PR: this file is no longer part of the diff. The branch was cut from a pre-merge snapshot of work that has since landed on main under different SHAs, so the review ran against ~100 files of already-merged code. Rebased onto current main -- the PR is now two files, neither of them this one.
The finding is not dismissed on merit; it targets code already on main and needs its own issue. See #278 (comment) for the full disposition.
There was a problem hiding this comment.
@EtienneLescot, acknowledged. This finding is out of scope for the rebased PR and remains unresolved on main.
Would you like me to open a follow-up GitHub issue for the useAudioPeaks duration-aware in-flight routing problem?
You are interacting with an AI system.
| async function decodeToBuffer( | ||
| context: OfflineAudioContext, | ||
| data: ArrayBuffer, | ||
| ): Promise<AudioBuffer> { | ||
| // decodeAudioData detaches the buffer, so hand it a copy. | ||
| return context.decodeAudioData(data.slice(0)); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
"mix" mode holds the exported file in memory three times.
Line 108 materializes the whole MP4 with videoBlob.arrayBuffer(). decodeToBuffer then copies it again with data.slice(0), and decodeAudioData produces a third buffer of decoded PCM. For a long recording this is several GB at once, on top of the export that just finished. The videoData copy is avoidable: nothing else reads it after decodeToBuffer, so pass the buffer directly and let decodeAudioData detach it.
♻️ Proposed change
async function decodeToBuffer(
context: OfflineAudioContext,
data: ArrayBuffer,
+ transfer = false,
): Promise<AudioBuffer> {
- // decodeAudioData detaches the buffer, so hand it a copy.
- return context.decodeAudioData(data.slice(0));
+ // decodeAudioData detaches the buffer. Copy only when the caller still
+ // needs the bytes afterwards.
+ return context.decodeAudioData(transfer ? data : data.slice(0));
}options.voiceoverData belongs to the caller, so it keeps the copy. videoData is read once, so it can be transferred:
- const original = await decodeToBuffer(context, videoData);
+ const original = await decodeToBuffer(context, videoData, 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/lib/exporter/voiceoverMix.ts` around lines 40 - 46, Update decodeToBuffer
and its videoData call site so the one-time-read exported video buffer is passed
directly to context.decodeAudioData without data.slice(0), allowing it to be
detached. Preserve the existing copy for options.voiceoverData because that
buffer belongs to the caller.
There was a problem hiding this comment.
Outdated on this PR: this file is no longer part of the diff. The branch was cut from a pre-merge snapshot of work that has since landed on main under different SHAs, so the review ran against ~100 files of already-merged code. Rebased onto current main -- the PR is now two files, neither of them this one.
The finding is not dismissed on merit; it targets code already on main and needs its own issue. See #278 (comment) for the full disposition.
There was a problem hiding this comment.
@EtienneLescot, acknowledged. This finding is out of scope for #278 after the rebase. I will not treat it as resolved or withdrawn on this pull request.
The memory-use finding still applies to src/lib/exporter/voiceoverMix.ts on main. Would you like me to create a separate GitHub issue for it?
You are interacting with an AI system.
cursor-sampler.exe shipped with no dpiAware manifest and never called SetProcessDpiAwareness*, so the process was DPI-unaware and Win32 handed it *virtualized* coordinates: GetCursorInfo().ptScreenPos and GetWindowRect both come back divided by the primary display's scale factor. b31bb71 assumed the opposite ("reports raw x/y in physical screen pixels") and converted the Electron display bounds to physical before normalizing. Both sides then lived in different spaces, so on a scaled display normalizeSample produced cx = (x/s)/W instead of x/W: the preview cursor sits at 1/s of its real offset from the top-left, an error that grows with the distance to the display origin (at 150% on a 2560px-wide screen, ~850px short at the right edge). Opt the helper into per-monitor-v2 awareness rather than walking the normalization back to DIPs: the numbers really do become physical, which is what every consumer already assumes, and unlike DIP normalization it also holds on mixed-DPI multi-monitor setups (virtualization always uses the primary display's scale, whatever monitor the cursor is actually on). Window captures were never affected: there the sampler supplies its own GetWindowRect bounds, so numerator and denominator were virtualized together and the ratio came out right either way. payload.x/y being physical now, the asset's display lookup needs screenToDipPoint -- screen.getDisplayNearestPoint works in DIPs and would otherwise pick the wrong monitor. macOS and Linux are unaffected: the SCK helper reports its capture frame in points, the same space as screen.getCursorScreenPoint(), and the PipeWire helper normalizes against the stream's own pixel dimensions. Verified: GetProcessDpiAwareness on the rebuilt binary reports PER_MONITOR_AWARE (was UNAWARE), and the reported position is unchanged at 100% scaling. Fixes #272
99a1ca1 to
065f3d2
Compare
Force-pushed: the diff was 44 commits too wideThis branch was cut from a pre-merge snapshot of the CLI / notes / compositor work. That work has since landed on Rebased the single fix commit onto current Re-verified after the rebase: On the CodeRabbit reviewThe review ran against that oversized diff. None of its 9 actionable comments are on this PR's own two files — both appear in the "files reviewed" list with no findings against them. All 9 target code that is already merged on
They are now outdated on this PR. Not dismissed on merit — several look genuine — but they belong in their own issue against |
1 participant
Fixes #272.
The bug
The cursor drawn in the editor preview sits short of where it really was, by more the further it is from the top-left of the display. Only on Windows, only on a display (screen) capture, only at a display scaling other than 100%.
Root cause
cursor-sampler.exeships with nodpiAwaremanifest and never callsSetProcessDpiAwareness*:So the process is DPI-unaware, and Win32 hands it virtualized coordinates —
GetCursorInfo().ptScreenPosandGetWindowRectboth come back divided by the primary display's scale factor.b31bb71f(shipped in v1.7.0, so present in the reported 1.8.0) assumed the opposite — "the cursor-sampler reports raw x/y in physical screen pixels" — and converted the Electron display bounds to physical withdipToScreenRectbefore normalizing. Numerator and denominator then lived in different spaces:The preview cursor lands at
1/sof its true offset from the display origin. At 150% on a 2560px-wide screen that is ~850px short at the right edge. At 100% the two spaces coincide, which is why it was invisible on the dev machines and in the Windows smoke tests.Window captures were never affected: there the sampler supplies its own
GetWindowRectbounds, so both sides were virtualized together and the ratio came out right either way.The fix
Opt the helper into per-monitor-v2 awareness, rather than walking the normalization back to DIPs. The reported numbers then really are physical — which is what every consumer already assumes — and unlike DIP normalization this also holds on mixed-DPI multi-monitor setups, where virtualization always uses the primary display's scale whatever monitor the cursor is on.
Second hunk:
payload.x/ybeing physical now, the asset's display lookup goes throughscreenToDipPoint—screen.getDisplayNearestPointworks in DIPs and would otherwise pick the wrong monitor.Verification
GetProcessDpiAwarenesscursor-sampler.exeas shipped in 1.8.0UNAWAREPER_MONITOR_AWAREtsc --noEmitandbiome checkpass.253,611==253,611) — no regression on unscaled displays.Cross-OS
ScreenCaptureRecorder.swift:127), the same space asscreen.getCursorScreenPoint()No test is added: the whole failure lives in a Win32 process attribute, and CI is Linux-only, so a guard here would never run.