fix(macos): correct cursor offset in single-window capture#22
Conversation
When recording a single window on macOS, the cursor in the exported video was offset by a fixed translation: clicks landed in the wrong place. Full-screen capture was correct. Root cause: the cursor sampler normalized the global cursor position against the selected display's bounds, never against the captured window's region. `getSelectedSourceBounds()` resolved a window source to its display (the window origin was never subtracted), so the normalized coordinates carried a constant offset equal to the window origin. Fix: the ScreenCaptureKit helper now reports the captured region's global frame (the window frame for window captures, the display frame for display captures) via a `captureBounds` field on its `ready` and `recording-started` events. The main process stores it and `getSelectedSourceBounds()` returns the window frame for window sources, so the cursor is normalized into the captured window's coordinate space. Display capture is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013iccbhodxNjMBraYPXpX6q
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe macOS helper now emits capture bounds for recording-started events, TypeScript accepts the new field, and Electron IPC stores the bounds for window source selection while clearing them around recording lifecycle transitions. ChangesmacOS capture bounds propagation
Sequence DiagramsequenceDiagram
participant Helper as Native macOS helper
participant Handlers as electron/ipc/handlers.ts
participant State as activeMacCaptureBounds
participant Bounds as getSelectedSourceBounds()
Helper->>Handlers: parsed helper event with captureBounds
Handlers->>State: store captureBounds
Handlers-->>Handlers: emit nativeMacCaptureEvents
Bounds->>State: read activeMacCaptureBounds for window source
Handlers->>State: clear on recording start/stop
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
- Add ROADMAP.md at repo root for top-level discoverability. - Stability queue pulled from open issues / PRs on getopenscreen/openscreen (#8, #21, #22, #19, #18, #24). - AI direction framed as opt-in / off by default, with per-feature tags. - Extend .github/workflows/discord.yaml with a roadmap-notify job that posts an embed to the #🗺️・roadmap Discord channel whenever a PR changes ROADMAP.md (or docs/roadmap.md). Mirrors the existing PR -> #pr-reviews sync pattern. Activated by setting the DISCORD_ROADMAP_WEBHOOK_URL repo secret; silently skipped otherwise so CI never breaks.
- Add ROADMAP.md at repo root for top-level discoverability. - Stability queue pulled from open issues / PRs on getopenscreen/openscreen (#8, #21, #22, #19, #18, #24). - AI direction framed as opt-in / off by default, with per-feature tags. - Link the new roadmap from the README Community section. - Extend .github/workflows/discord.yaml with a roadmap-notify job that posts an embed to the #🗺️・roadmap Discord channel whenever a PR changes ROADMAP.md (or docs/roadmap.md). Mirrors the existing PR -> #pr-reviews sync pattern. Activated by setting the DISCORD_ROADMAP_WEBHOOK_URL repo secret; silently skipped otherwise so CI never breaks.
- Add ROADMAP.md at repo root for top-level discoverability. - Stability queue pulled from open issues / PRs on getopenscreen/openscreen (#8, #21, #22, #19, #18, #24). - AI direction framed as opt-in / off by default, with per-feature tags. - Link the new roadmap from the README Community section. - Extend .github/workflows/discord.yaml with a roadmap-notify job that posts an embed to the #🗺️・roadmap Discord channel whenever a PR changes ROADMAP.md (or docs/roadmap.md). Mirrors the existing PR -> #pr-reviews sync pattern. Activated by setting the DISCORD_ROADMAP_WEBHOOK_URL repo secret; silently skipped otherwise so CI never breaks.
There was a problem hiding this comment.
Ey @giulio333 thanks for the PR!!
Ponytail review — over-engineering
electron/ipc/handlers.ts
- shrink
parseCaptureBounds(21 lines for a payload the Swift helper emits with hardcoded dict literals). We control both ends of this IPC. Replace with one inline check at the single use site, oractiveMacCaptureBounds = event.captureBounds as Rectangle | undefinedand delete the helper. - shrink the 3-line comment block above
activeMacCaptureBounds(it duplicates the use-site comment ingetSelectedSourceBounds). One short line, or move the explanation to the call site only. - shrink the 2-line comment inside
dispatchNativeMacHelperEvent. The variable name + one-line body are self-explanatory. - shrink the 2-line comment before
activeMacCaptureBounds = nullin the recording-start block.activeMacCaptureBounds = null;reads itself.
electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/main.swift
- delete
captureBoundsfrom thereadyevent payload. Cursor polling only starts afterwaitForNativeMacCaptureStartresolves onrecording-started; thereadypayload'scaptureBoundsis read by nothing.
src/lib/nativeMacRecording.ts
- delete the
captureBounds?field onNativeMacHelperReadyEvent(consequence of the Swift delete above).
Net lines removable: ~28.
Functional review
Correctness — looks right. Root cause matches the symptom. Window source predicate (selectedSource.id.startsWith("window:")) is consistent with the rest of the codebase (useScreenRecorder.ts:810, nativeMacRecording.ts:94, SourceSelector.tsx:37,63). Display path untouched.
Race / ordering — safe. nativeMacCaptureOutput is cleared (line 1810) before the new helper spawns, so inspectNativeMacCaptureOutput() replay sees an empty buffer. recording-started always fires after ready in Swift, so activeMacCaptureBounds is populated before cursor polling starts.
Real gap — window resize mid-recording. Swift captures window.frame once in makeCaptureTarget. If the user resizes the window while recording, the cursor offset returns. Cheap upgrade later: emit captureBounds again from SCStreamDelegate.stream(_:didChangeWith:). Not blocking; file as a follow-up.
No test added. The JS pipeline (captureBounds → global → getSelectedSourceBounds) is pure and worth a 20-line Vitest covering: window source + bounds set → returns bounds; window source + bounds null → falls through to display; display source → ignores bounds. CI can't catch macOS regressions but it can lock the JS contract.
Coordinate units. Swift window.frame is in points, Electron screen bounds are in pixels — macOS reports consistently in points through this stack, but worth a manual sanity check on a retina vs. non-retina machine.
Verdict
Ship-able. The four shrinks and the one delete above are concrete polish items that would tighten this PR. Functional concerns are follow-ups.
|
@giulio333 just to let you know, my review is an automated review made with https://github.com/DietrichGebert/ponytail This is why it can feel a bit nitpicky. In the future, those reviews will be better marked as automated, with better P0 → nitpicky ranking. |
…ady-event field - inline the captureBounds check in dispatchNativeMacHelperEvent and remove parseCaptureBounds; we control both ends of this IPC - drop captureBounds from the `ready` event (Swift + TS): cursor polling only starts after `recording-started` resolves, so it was read by nothing - trim the duplicated/self-explanatory comments around activeMacCaptureBounds Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S4UdyNkRoEn2B8JaG5YdLK
|
Thanks for the review, @EtienneLescot — good catches, all addressed in Over-engineering
Functional follow-ups (not in this PR, agree they're non-blocking):
|
EtienneLescot
left a comment
EtienneLescot
left a comment
There was a problem hiding this comment.
All five ponytail findings applied. ~45 lines removed vs. the original +78. Remaining follow-ups (window resize mid-recording, JS contract test) are non-blocking. Ship.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 2915-2919: The diagnostics payload currently serializes raw
helperOutput and mainProcessLogs, which can expose sensitive capture details.
Update the diagnostics assembly in the handler that builds this response to
redact these values before they are written, using the existing helper-output
and main-log collection points to apply scrubbing just before serialization.
Keep the structure intact, but ensure the windows/mac helper tails and the
mainLogBuffer snapshot are sanitized for local paths, source/window names, and
device metadata before saving.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b232496e-8cea-451f-9d5d-30975b54e95c
📒 Files selected for processing (1)
electron/ipc/handlers.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 2915-2919: The diagnostics payload currently serializes raw
helperOutput and mainProcessLogs, which can expose sensitive capture details.
Update the diagnostics assembly in the handler that builds this response to
redact these values before they are written, using the existing helper-output
and main-log collection points to apply scrubbing just before serialization.
Keep the structure intact, but ensure the windows/mac helper tails and the
mainLogBuffer snapshot are sanitized for local paths, source/window names, and
device metadata before saving.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b232496e-8cea-451f-9d5d-30975b54e95c
📒 Files selected for processing (1)
electron/ipc/handlers.ts
🛑 Comments failed to post (1)
electron/ipc/handlers.ts (1)
2915-2919: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact helper and main-process logs before saving diagnostics.
These outputs can include local paths, selected source/window names, and device metadata from capture logs. Since diagnostic files are likely shared for support, scrub sensitive values before serialization.
Proposed localized redaction
const HELPER_OUTPUT_MAX_BYTES = 64 * 1024; - const tail = (s: string, max: number) => (s.length <= max ? s : s.slice(s.length - max)); + const redactDiagnosticText = (value: string) => + value + .replaceAll(app.getPath("home"), "~") + .replaceAll(RECORDINGS_DIR, "<recordings-dir>"); + const tail = (s: string, max: number) => { + const redacted = redactDiagnosticText(s); + return redacted.length <= max ? redacted : redacted.slice(redacted.length - max); + }; const diagnostic = { timestamp: new Date().toISOString(), @@ helperOutput: { windows: tail(nativeWindowsCaptureOutput, HELPER_OUTPUT_MAX_BYTES), mac: tail(nativeMacCaptureOutput, HELPER_OUTPUT_MAX_BYTES), }, - mainProcessLogs: mainLogBuffer.snapshot(), + mainProcessLogs: mainLogBuffer.snapshot().map(redactDiagnosticText), };📝 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.const HELPER_OUTPUT_MAX_BYTES = 64 * 1024; const redactDiagnosticText = (value: string) => value .replaceAll(app.getPath("home"), "~") .replaceAll(RECORDINGS_DIR, "<recordings-dir>"); const tail = (s: string, max: number) => { const redacted = redactDiagnosticText(s); return redacted.length <= max ? redacted : redacted.slice(redacted.length - max); }; const diagnostic = { timestamp: new Date().toISOString(), helperOutput: { windows: tail(nativeWindowsCaptureOutput, HELPER_OUTPUT_MAX_BYTES), mac: tail(nativeMacCaptureOutput, HELPER_OUTPUT_MAX_BYTES), }, mainProcessLogs: mainLogBuffer.snapshot().map(redactDiagnosticText),🧰 Tools
🪛 ast-grep (0.44.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { type ChildProcessWithoutNullStreams, spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(detect-child-process-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/ipc/handlers.ts` around lines 2915 - 2919, The diagnostics payload currently serializes raw helperOutput and mainProcessLogs, which can expose sensitive capture details. Update the diagnostics assembly in the handler that builds this response to redact these values before they are written, using the existing helper-output and main-log collection points to apply scrubbing just before serialization. Keep the structure intact, but ensure the windows/mac helper tails and the mainLogBuffer snapshot are sanitized for local paths, source/window names, and device metadata before saving.
2 participants
Summary
Fixes a cursor-position offset in the exported video when recording a single application window on macOS. The cursor (and therefore clicks) appeared shifted by a fixed translation from where they actually were on screen. Full-screen capture was already correct.
Root cause
The cursor sampler normalizes the global cursor position into the captured region's coordinate space. For window captures it was normalizing against the selected display's bounds instead of the window's bounds:
getSelectedSourceBounds()resolved a window source to its display and never subtracted the window origin.So the normalized coordinates carried a constant offset equal to the window origin — exactly the "fixed translation" reported.
Fix
captureBoundsfield on itsreadyandrecording-startedevents.getSelectedSourceBounds()now returns the window frame for window sources, so the cursor is normalized into the captured window's coordinate space.Changes
electron/native/screencapturekit/.../main.swift— carry and emit the captured frame ascaptureBounds.electron/ipc/handlers.ts— track the helper-reported bounds and use them for window captures.src/lib/nativeMacRecording.ts— addcaptureBoundsto the helper event types.Testing
tsc --noEmitand Biome pass.🤖 Generated with Claude Code
Summary by CodeRabbit