fix: use native region capture on modern macOS#15
Conversation
|
test |
|
deleted |
There was a problem hiding this comment.
Pull request overview
Fixes repeated Screen Recording permission prompting on modern macOS by routing region capture through a native ScreenCaptureKit-backed path (macOS 15.2+) while keeping a legacy /usr/sbin/screencapture fallback for older versions.
Changes:
- Added a new
ScreenCaptureServicewith a macOS 15.2+ native region-selection overlay and ScreenCaptureKit capture implementation. - Updated app capture actions to use the new service instead of directly invoking
screencapture. - Added a lightweight strategy test validating backend selection across macOS versions.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
Tests/ScreenCaptureStrategyTests.swift |
Adds a small strategy-selection test runner for backend selection by OS version. |
ScreenScribe/Sources/Services/ScreenCaptureBackend.swift |
Introduces backend enum, strategy logic, native overlay selection UI, ScreenCaptureKit capture, and legacy CLI fallback. |
ScreenScribe/Sources/App.swift |
Routes capture actions through ScreenCaptureService.captureSelectionImage() instead of directly invoking screencapture. |
ScreenScribe.xcodeproj/project.pbxproj |
Adds the new service file to the Xcode project build sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return await withCheckedContinuation { (continuation: CheckedContinuation<NSImage?, Never>) in | ||
| let task = Process() | ||
| task.executableURL = URL(fileURLWithPath: "/usr/sbin/screencapture") | ||
| task.arguments = ["-i", "-c", "-x"] | ||
| task.terminationHandler = { _ in | ||
| DispatchQueue.main.async { | ||
| let pasteboard = NSPasteboard.general | ||
| guard pasteboard.changeCount != initialChangeCount else { | ||
| continuation.resume(returning: nil) | ||
| return | ||
| } | ||
|
|
||
| continuation.resume(returning: Self.pasteboardImage(from: pasteboard)) | ||
| } | ||
| } | ||
|
|
||
| do { | ||
| try task.run() | ||
| } catch { | ||
| Logger.log(.error, "Legacy screencapture launch failed: \(error.localizedDescription)") | ||
| continuation.resume(returning: nil) | ||
| } | ||
| } |
There was a problem hiding this comment.
Process must be strongly retained for the duration of its execution; otherwise it may be deallocated immediately after this closure returns, preventing terminationHandler from firing and leaving the continuation un-resumed (hanging the capture). Keep a strong reference to the task until completion (e.g., store it on the service or capture it in a way that guarantees lifetime), and ensure any retain cycle with terminationHandler is broken after termination.
| private var continuation: CheckedContinuation<CGRect?, Never>? | ||
|
|
||
| func selectRegion() async -> CGRect? { | ||
| await withCheckedContinuation { continuation in |
There was a problem hiding this comment.
selectRegion() overwrites self.continuation unconditionally. If captureSelectionImage() is triggered again while a selection overlay is already active, the first awaiting task will never be resumed (hang), and the overlay state becomes ambiguous. Add a reentrancy guard (e.g., return/cancel if continuation is already set) or explicitly finish the in-flight selection before starting a new one.
| await withCheckedContinuation { continuation in | |
| if continuation != nil { | |
| // Cancel any in-flight selection before starting a new one | |
| finish(with: nil) | |
| } | |
| return await withCheckedContinuation { continuation in |
| if let data = pasteboard.data(forType: .fileURL), | ||
| let string = String(data: data, encoding: .utf8), | ||
| let url = URL(string: string) { | ||
| return NSImage(contentsOf: url) | ||
| } | ||
|
|
There was a problem hiding this comment.
The .fileURL pasteboard type isn’t guaranteed to be UTF-8 string data; interpreting data(forType:) as a UTF-8 String can fail and incorrectly return nil even when a file URL is present. Prefer pasteboard.string(forType: .fileURL), propertyList(forType:), or readObjects(forClasses: [NSURL.self]) (and trim whitespace/newlines) before constructing the URL.
| if let data = pasteboard.data(forType: .fileURL), | |
| let string = String(data: data, encoding: .utf8), | |
| let url = URL(string: string) { | |
| return NSImage(contentsOf: url) | |
| } | |
| if let urlString = pasteboard.string(forType: .fileURL)? | |
| .trimmingCharacters(in: .whitespacesAndNewlines), | |
| let url = URL(string: urlString) { | |
| return NSImage(contentsOf: url) | |
| } | |
| if let urls = pasteboard.readObjects(forClasses: [NSURL.self]) as? [URL], | |
| let url = urls.first { | |
| return NSImage(contentsOf: url) | |
| } |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Free Tier Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| return | ||
| } | ||
|
|
||
| onComplete(window.convertToScreen(selectionRect)) |
There was a problem hiding this comment.
AppKit-to-Quartz coordinate mismatch in region capture
High Severity
window.convertToScreen() returns a rect in AppKit screen coordinates (origin at bottom-left, Y increases upward), but SCScreenshotManager.captureImage(in:) expects Quartz Display Services coordinates (origin at top-left, Y increases downward). This means the Y coordinate of the user's selection is vertically inverted — selecting a region near the top of the screen captures from near the bottom, and vice versa. The rect needs a coordinate-space conversion before being passed to ScreenCaptureKit.
Additional Locations (1)
Code ReviewOverall, this is a solid architectural improvement. Moving away from the screencapture CLI to a native ScreenCaptureKit backend is the right direction, and the implementation is well-structured. A few issues worth addressing. --- Bugs --- 1. Retina display: pixel dimensions passed as point dimensions In captureWithNativeRegionSelection, the NSImage is constructed with CGImage.width/height, which are pixel dimensions. On Retina displays these are 2x or 3x the point size, so the image will be reported as oversized to downstream consumers (performVisionExtraction, performAIExtraction). Fix: pass .zero as the size to let AppKit infer it from the image resolution metadata (backing scale). Replace: NSImage(cgImage: image, size: NSSize(width: image.width, height: image.height)) with: NSImage(cgImage: image, size: .zero) 2. pasteboardImage(from:) - fragile .fileURL data decoding .fileURL pasteboard data is encoded as a property list (NSURL serialisation), not plain UTF-8. URL(string:) also silently fails on paths containing spaces or special characters. Safer approach: use pasteboard.readObjects(forClasses: [NSURL.self]) as? [URL], then NSImage(contentsOf: url) 3. Strategy/availability mismatch - silent fallback can mask bugs ScreenCaptureStrategy.preferred() returns .nativeRegionSelection for macOS 15.2+, but captureSelectionImage() then duplicates the version gate. The branch where nativeRegionSelection is selected but the availability check fails is theoretically impossible. Use assertionFailure there rather than a plain error log to make such inconsistency visible during development. --- Code Quality --- 4. Magic number for Escape key - keyCode 53 is undocumented. Use kVK_Escape from Carbon.HIToolbox or a local named constant so the intent is clear. 5. Hardcoded 75 ms dismiss delay - This heuristic could be too short on a loaded system. Extract it to a named constant: private let overlayDismissDelay: UInt64 = 75_000_000 (ns, wait for overlay compositing) --- Tests --- 6. Standalone test runner not integrated with Xcode - ScreenCaptureStrategyTests.swift uses @main to compile as a standalone executable. Tests won't appear in Xcode Test navigator, can't be run with xcodebuild test, and won't participate in CI. Migrating to XCTest would give proper IDE and CI integration. --- Minor notes ---
|
Summary
Users on modern macOS could grant Screen Recording permission, relaunch ScreenScribe, and still get sent back through the permission flow. The effect was that the app looked like it never remembered the grant, even though Screen Recording was enabled in System Settings.
The root cause was that the app still relied on
/usr/sbin/screencapturefor the actual region capture path. The permission manager had become more defensive, but the user-facing capture action was still routed through a legacy CLI that can re-trigger screen-capture consent behavior on newer macOS releases.This PR adds a native capture backend for macOS 15.2+ and Tahoe-era releases. It introduces a lightweight overlay for region selection and uses
SCScreenshotManager.captureImage(in:)to capture the selected rectangle directly with ScreenCaptureKit. Older systems keep the legacy fallback, and the app capture actions now flow through the new service instead of directly invokingscreencapture.Fixes #12
Testing
swiftc ScreenScribe/Sources/Logger.swift ScreenScribe/Sources/Services/ScreenCaptureBackend.swift Tests/ScreenCaptureStrategyTests.swift -o /tmp/screen-capture-strategy-tests && /tmp/screen-capture-strategy-testsxcodebuild -project ScreenScribe.xcodeproj -scheme ScreenScribe -configuration Debug build CODE_SIGNING_ALLOWED=NONote
Medium Risk
Changes the core screenshot capture path and introduces a new overlay-based region selector, which could affect capture reliability across macOS versions and permission behavior. Includes a legacy CLI fallback and basic strategy tests, reducing but not eliminating rollout risk.
Overview
Switches region capture on modern macOS away from the
/usr/sbin/screencaptureCLI to a new native backend to avoid re-triggering screen recording consent flows after permission is granted.Adds
ScreenCaptureServicewith an OS-version-based strategy (ScreenCaptureStrategy) that uses a lightweight overlay for region selection and captures the selected rect viaSCScreenshotManager.captureImage(in:)on macOS 15.2+ (with a pasteboard-based legacy CLI fallback for older systems or failures). App capture actions (initiateCaptureForText/ prompt capture) now route through this service, and a small standalone test validates the backend selection logic.Written by Cursor Bugbot for commit 721a341. This will update automatically on new commits. Configure here.