fix: Two related bugs in v1.1.8 on macOS (reproducible on M1):#139
fix: Two related bugs in v1.1.8 on macOS (reproducible on M1):#139sethsticle wants to merge 1 commit intowebadderall:mainfrom
Conversation
…elemetry on failure
|
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Configuration .gitignore |
Updated line endings to CRLF; re-added native build artifact patterns and debug script exclusions; added .claude/ directory ignore for local editor settings. |
Video Finalization & Cursor Handling electron/ipc/handlers.ts |
Modified finalizeStoredVideo() to snapshot and persist cursor telemetry before validation with logged errors; made validation results mandatory in file size calculation; removed nullable validation fallback. Updated macOS window highlight flow to fetch and use window bounds from AppleScript. Simplified FFmpeg recording path timestamping by inlining Date.now(). Added thumbnail filtering to source enumeration on macOS to drop sources with missing thumbnails. |
Native Session Timing electron/native/ScreenCaptureKitRecorder.swift |
Changed finishCapture() to compute AVAssetWriter session end time from last recorded frame timing and duration rather than directly from sample buffer timestamp; returns zero when no frames recorded. |
Sequence Diagram(s)
sequenceDiagram
participant Handler as IPC Handler
participant Telemetry as Cursor Telemetry
participant Validation as Video Validation
participant Native as Native Recorder
Handler->>Telemetry: snapshot() & persist()
Note over Telemetry: Log errors if persist fails
Handler->>Validation: validateRecordedVideo(path)
Validation-->>Handler: {fileSizeBytes, durationSeconds}
Handler->>Handler: Assert fileSizeBytes not null
Handler->>Handler: Build success message<br/>(always include size,<br/>conditionally include duration)
Handler->>Native: finishCapture()
Note over Native: Calculate sessionEndTime<br/>from frame timing
Native-->>Handler: confirm
Handler-->>Handler: Return finalized result
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~23 minutes
Suggested labels
Checked
Poem
🐰 Hops through macOS frames with glee,
Cursor captured, now we see!
Timing mended, bounds are bright,
Window sources filtered right—
Video finalized just so, what a sight!
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly identifies the specific bugs being fixed (two related macOS bugs in v1.1.8 on M1) and mentions the main areas affected, directly relating to the core changes. |
| Description check | ✅ Passed | The PR description comprehensively covers root causes, fixes, and testing performed. While it doesn't strictly follow the template structure (no checkboxes, no explicit sections), it provides all critical information needed: motivation, changes, and validation. |
| Linked Issues check | ✅ Passed | The PR fixes both primary requirements from issue #135: it corrects the malformed MP4 edit list bug causing FFmpeg validation failures [ScreenCaptureKitRecorder.swift], and preserves cursor telemetry persistence [handlers.ts] to ensure cursor data is saved. |
| Out of Scope Changes check | ✅ Passed | All changes are directly scoped to fixing the two identified bugs. The .gitignore updates (Claude local settings) and additional handler optimizations are minimal and incidental to the core fixes. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
electron/ipc/handlers.ts (1)
3381-3388: Consider reusing one timestamp for all artifacts in a recording.Calling
Date.now()three times can give the video and sidecars different basenames, which makes diagnostics and manual recovery noisier. Capturing the timestamp once keeps the files grouped.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@electron/ipc/handlers.ts` around lines 3381 - 3388, The three separate Date.now() calls create mismatched basenames; replace them by capturing a single timestamp (e.g., const ts = Date.now()) before constructing paths and use that ts when building outputPath, systemAudioOutputPath and microphoneOutputPath (still honoring recordingsDir and options?.capturesSystemAudio / options?.capturesMicrophone) so all recording artifacts share the same basename.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 1-5: The .gitignore file contains Windows-style CRLF line endings;
revert them to LF (Unix-style) so Git ignores work consistently across
macOS/Linux; open the .gitignore, convert all CRLF to LF (or run git
normalization with core.autocrlf=input), save and re-stage the .gitignore so the
commit only changes line endings back to LF and remove any unrelated whitespace
edits.
In `@electron/ipc/handlers.ts`:
- Line 2949: Replace the current filter predicate that only checks
Boolean(source.thumbnail) with the usable-thumbnail gate so unusable Electron
thumbnails are excluded before data-URL conversion; specifically, change the
filter on the array (currently .filter((source) => Boolean(source.thumbnail)))
to use hasUsableSourceThumbnail(source) or run hasUsableSourceThumbnail(source)
prior to calling source.toDataURL() (the place where toDataURL() is invoked),
ensuring only sources passing hasUsableSourceThumbnail() are mapped to data
URLs.
- Around line 3055-3066: The current logic prefers asBounds then falls back to
platform-specific resolution which can pick the frontmost app window rather than
the user-selected window; change the branch so for window sources you call
resolveMacWindowBounds(source) (on darwin) or resolveLinuxWindowBounds(source)
(on linux) first and set bounds from that result only if it returns a valid
value, and only then fall back to asBounds; keep the screen source path using
getDisplayBoundsForSource(source) unchanged and ensure you still check isWindow
before the platform-specific resolution.
- Around line 3015-3038: The AppleScript invocations in the execFileAsync calls
interpolate appName directly and are vulnerable to code injection; change both
usages to pass appName via osascript's argv (use an 'on run argv' wrapper that
assigns the first argv to a local variable) and remove any direct string
interpolation of appName. Specifically, update the main execFileAsync (the block
that returns x,y,w,h and assigns asBounds) and the fallback execFileAsync (the
'activate' fallback) to use an AppleScript script that starts with "on run argv"
/ "set appName to item 1 of argv" / ... / "end run" and then pass [appName] as
the argv argument to execFileAsync, keeping the same timeouts and the
surrounding logic (execFileAsync, stdout parsing, asBounds assignment)
unchanged.
---
Nitpick comments:
In `@electron/ipc/handlers.ts`:
- Around line 3381-3388: The three separate Date.now() calls create mismatched
basenames; replace them by capturing a single timestamp (e.g., const ts =
Date.now()) before constructing paths and use that ts when building outputPath,
systemAudioOutputPath and microphoneOutputPath (still honoring recordingsDir and
options?.capturesSystemAudio / options?.capturesMicrophone) so all recording
artifacts share the same basename.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a4a493ff-c24d-44d3-a9aa-8809e82d3894
📒 Files selected for processing (3)
.gitignoreelectron/ipc/handlers.tselectron/native/ScreenCaptureKitRecorder.swift
| # Logs | ||
| logs | ||
| *.log | ||
| npm-debug.log* | ||
| yarn-debug.log* |
There was a problem hiding this comment.
Revert CRLF line endings to LF (Unix-style).
The AI summary indicates these lines were changed to use Windows-style CRLF line endings. This is problematic for a cross-platform project, especially one addressing macOS bugs. .gitignore files should use LF (Unix) line endings to ensure consistent behavior across macOS, Linux, and Windows (Git automatically handles line ending conversions for repository files).
CRLF line endings in .gitignore can cause Git to malfunction on Unix-like systems, potentially failing to ignore specified files and directories.
Additionally, these formatting changes appear unrelated to the PR's stated objectives (fixing macOS video recording and cursor telemetry bugs).
🔧 Recommended action
Ensure the entire .gitignore file uses LF line endings. Most editors and Git tools can automatically convert line endings. If you're using Git, you can configure:
git config core.autocrlf inputThen re-stage the file to normalize line endings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 1 - 5, The .gitignore file contains Windows-style
CRLF line endings; revert them to LF (Unix-style) so Git ignores work
consistently across macOS/Linux; open the .gitignore, convert all CRLF to LF (or
run git normalization with core.autocrlf=input), save and re-stage the
.gitignore so the commit only changes line endings back to LF and remove any
unrelated whitespace edits.
| sourceType: 'window' as const, | ||
| } | ||
| }) | ||
| .filter((source) => Boolean(source.thumbnail)) |
There was a problem hiding this comment.
Apply the usable-thumbnail check before converting to a data URL.
Boolean(source.thumbnail) only checks that a string exists after toDataURL(). It does not enforce the hasUsableSourceThumbnail() gate you already use in the fallback path, so unusable Electron thumbnails can still survive the merged macOS path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` at line 2949, Replace the current filter predicate
that only checks Boolean(source.thumbnail) with the usable-thumbnail gate so
unusable Electron thumbnails are excluded before data-URL conversion;
specifically, change the filter on the array (currently .filter((source) =>
Boolean(source.thumbnail))) to use hasUsableSourceThumbnail(source) or run
hasUsableSourceThumbnail(source) prior to calling source.toDataURL() (the place
where toDataURL() is invoked), ensuring only sources passing
hasUsableSourceThumbnail() are mapped to data URLs.
| const { stdout } = await execFileAsync('osascript', ['-e', | ||
| `tell application "${appName}"\n` + | ||
| ` activate\n` + | ||
| `end tell\n` + | ||
| `delay 0.3\n` + | ||
| `tell application "System Events"\n` + | ||
| ` tell process "${appName}"\n` + | ||
| ` set frontWindow to front window\n` + | ||
| ` set {x1, y1} to position of frontWindow\n` + | ||
| ` set {w1, h1} to size of frontWindow\n` + | ||
| ` return (x1 as text) & "," & (y1 as text) & "," & (w1 as text) & "," & (h1 as text)\n` + | ||
| ` end tell\n` + | ||
| `end tell` | ||
| ], { timeout: 4000 }) | ||
| const parts = stdout.trim().split(',').map(Number) | ||
| if (parts.length === 4 && parts.every(n => Number.isFinite(n))) { | ||
| asBounds = { x: parts[0], y: parts[1], width: parts[2], height: parts[3] } | ||
| } | ||
| } catch { | ||
| // Fallback: just activate without bounds | ||
| try { | ||
| await execFileAsync('osascript', ['-e', | ||
| `tell application "${appName}" to activate` | ||
| ], { timeout: 2000 }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /root && find . -name "handlers.ts" -type f | head -20Repository: webadderall/Recordly
Length of output: 119
🏁 Script executed:
# Check if the file exists and its size
wc -l electron/ipc/handlers.ts 2>/dev/null || find . -name "handlers.ts" -path "*/ipc/*" -type fRepository: webadderall/Recordly
Length of output: 93
🏁 Script executed:
# Read the specific section mentioned
sed -n '3000,3050p' electron/ipc/handlers.tsRepository: webadderall/Recordly
Length of output: 2356
🏁 Script executed:
# Also search for the appName variable definition and context around those lines
sed -n '2950,3100p' electron/ipc/handlers.tsRepository: webadderall/Recordly
Length of output: 5555
🏁 Script executed:
# Search for other osascript calls in the file to see if there are similar patterns
rg -n "execFileAsync.*osascript" electron/ipc/handlers.ts -A 3Repository: webadderall/Recordly
Length of output: 508
🏁 Script executed:
# Also check if there are other vulnerable string interpolation patterns with execFileAsync and shell commands
rg -n "execFileAsync.*\$\{.*\}" electron/ipc/handlers.ts | head -20Repository: webadderall/Recordly
Length of output: 46
🏁 Script executed:
# Verify the AppleScript on run argv pattern is valid by checking if there are any existing uses in the codebase
rg -n "on run argv" . --type typescript --type javascript --type applescriptRepository: webadderall/Recordly
Length of output: 98
Fix AppleScript code injection vulnerability via appName parameter.
Both osascript calls (lines 3015 and 3036) interpolate appName directly into AppleScript without escaping. A quote or newline character in the app name can break out of the string literal and execute arbitrary AppleScript code. Pass the value via on run argv instead.
🔒 Safer pattern
- const { stdout } = await execFileAsync('osascript', ['-e',
- `tell application "${appName}"\n` +
- ` activate\n` +
- `end tell\n` +
- `delay 0.3\n` +
- `tell application "System Events"\n` +
- ` tell process "${appName}"\n` +
- ` set frontWindow to front window\n` +
- ` set {x1, y1} to position of frontWindow\n` +
- ` set {w1, h1} to size of frontWindow\n` +
- ` return (x1 as text) & "," & (y1 as text) & "," & (w1 as text) & "," & (h1 as text)\n` +
- ` end tell\n` +
- `end tell`
- ], { timeout: 4000 })
+ const script = `
+on run argv
+ set appName to item 1 of argv
+ tell application appName to activate
+ delay 0.3
+ tell application "System Events"
+ tell process appName
+ set frontWindow to front window
+ set {x1, y1} to position of frontWindow
+ set {w1, h1} to size of frontWindow
+ return (x1 as text) & "," & (y1 as text) & "," & (w1 as text) & "," & (h1 as text)
+ end tell
+ end tell
+end run`
+ const { stdout } = await execFileAsync('osascript', ['-e', script, appName], { timeout: 4000 })Also apply the same fix to the fallback call at line 3036.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 3015 - 3038, The AppleScript
invocations in the execFileAsync calls interpolate appName directly and are
vulnerable to code injection; change both usages to pass appName via osascript's
argv (use an 'on run argv' wrapper that assigns the first argv to a local
variable) and remove any direct string interpolation of appName. Specifically,
update the main execFileAsync (the block that returns x,y,w,h and assigns
asBounds) and the fallback execFileAsync (the 'activate' fallback) to use an
AppleScript script that starts with "on run argv" / "set appName to item 1 of
argv" / ... / "end run" and then pass [appName] as the argv argument to
execFileAsync, keeping the same timeouts and the surrounding logic
(execFileAsync, stdout parsing, asBounds assignment) unchanged.
| let bounds = asBounds | ||
|
|
||
| if (source.id?.startsWith('screen:')) { | ||
| bounds = getDisplayBoundsForSource(source) | ||
| } else if (isWindow) { | ||
| if (process.platform === 'darwin') { | ||
| bounds = await resolveMacWindowBounds(source) | ||
| } else if (process.platform === 'linux') { | ||
| bounds = await resolveLinuxWindowBounds(source) | ||
| if (!bounds) { | ||
| if (source.id?.startsWith('screen:')) { | ||
| bounds = getDisplayBoundsForSource(source) | ||
| } else if (isWindow) { | ||
| if (process.platform === 'darwin') { | ||
| bounds = await resolveMacWindowBounds(source) | ||
| } else if (process.platform === 'linux') { | ||
| bounds = await resolveLinuxWindowBounds(source) | ||
| } | ||
| } |
There was a problem hiding this comment.
Prefer the selected window's bounds over the app's front window.
asBounds is for whichever window becomes frontmost after activate, not necessarily the windowId the user selected. In multi-window apps this can highlight the wrong window. Use resolveMacWindowBounds(source) first and fall back to asBounds only if that lookup fails.
🎯 Suggested fix
- let bounds = asBounds
-
- if (!bounds) {
- if (source.id?.startsWith('screen:')) {
- bounds = getDisplayBoundsForSource(source)
- } else if (isWindow) {
- if (process.platform === 'darwin') {
- bounds = await resolveMacWindowBounds(source)
- } else if (process.platform === 'linux') {
- bounds = await resolveLinuxWindowBounds(source)
- }
- }
- }
+ let bounds: WindowBounds | null = null
+
+ if (source.id?.startsWith('screen:')) {
+ bounds = getDisplayBoundsForSource(source)
+ } else if (isWindow) {
+ if (process.platform === 'darwin') {
+ bounds = await resolveMacWindowBounds(source) ?? asBounds
+ } else if (process.platform === 'linux') {
+ bounds = await resolveLinuxWindowBounds(source)
+ }
+ }📝 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.
| let bounds = asBounds | |
| if (source.id?.startsWith('screen:')) { | |
| bounds = getDisplayBoundsForSource(source) | |
| } else if (isWindow) { | |
| if (process.platform === 'darwin') { | |
| bounds = await resolveMacWindowBounds(source) | |
| } else if (process.platform === 'linux') { | |
| bounds = await resolveLinuxWindowBounds(source) | |
| if (!bounds) { | |
| if (source.id?.startsWith('screen:')) { | |
| bounds = getDisplayBoundsForSource(source) | |
| } else if (isWindow) { | |
| if (process.platform === 'darwin') { | |
| bounds = await resolveMacWindowBounds(source) | |
| } else if (process.platform === 'linux') { | |
| bounds = await resolveLinuxWindowBounds(source) | |
| } | |
| } | |
| let bounds: WindowBounds | null = null | |
| if (source.id?.startsWith('screen:')) { | |
| bounds = getDisplayBoundsForSource(source) | |
| } else if (isWindow) { | |
| if (process.platform === 'darwin') { | |
| bounds = await resolveMacWindowBounds(source) ?? asBounds | |
| } else if (process.platform === 'linux') { | |
| bounds = await resolveLinuxWindowBounds(source) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@electron/ipc/handlers.ts` around lines 3055 - 3066, The current logic prefers
asBounds then falls back to platform-specific resolution which can pick the
frontmost app window rather than the user-selected window; change the branch so
for window sources you call resolveMacWindowBounds(source) (on darwin) or
resolveLinuxWindowBounds(source) (on linux) first and set bounds from that
result only if it returns a valid value, and only then fall back to asBounds;
keep the screen source path using getDisplayBoundsForSource(source) unchanged
and ensure you still check isWindow before the platform-specific resolution.
|
issue seems to be resolved but thank you for the PR! |
2 participants
Root Cause
Bug 1 —
ScreenCaptureKitRecorder.swiftendSession(atSourceTime:)was passedlastSampleBuffer?.presentationTimeStamp— the raw host clocktime (~3600s if Mac has been running an hour). But the session starts at
.zeroand allframes are re-timed to 0-based adjusted timestamps. The resulting MP4 has a malformed edit
list, which the new
validateRecordedVideoFFmpeg check (added in 1.1.8) correctly rejects.Bug 2 —
handlers.tsfinalizeStoredVideopersistPendingCursorTelemetrywas called aftervalidateRecordedVideo. If validationthrew (due to Bug 1), the cursor JSON was never written — silently losing all cursor tracking data.
Fix
lastVideoPresentationTime + frameDuration(for: lastSampleBuffer)forendSession—consistent with the existing adjacent code that computes
additionalTimethe same wayTesting
Fixes #135
Summary by CodeRabbit