[codex] Fix Clips screencapture fallback missing-file errors#1651
[codex] Fix Clips screencapture fallback missing-file errors#1651TimpiaAI wants to merge 1 commit into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Here's a visual recap of what changed:
Open the full interactive recap |
0fbfaa3 to
70f838c
Compare
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Reviewed PR #1651 (standard-risk / medium) for the Clips native recorder fallback path. The patch is directionally solid: it blocks the ScreenCaptureKit→screencapture fallback for clear macOS Screen Recording permission denials, keeps the fallback output path on the active backend, and adds immediate output validation plus focused unit coverage for the new matcher and file checks.
The main architectural concern is error propagation rather than the fallback detection itself. The new verify_screencapture_output() helper now produces actionable permission-oriented failures, but one of those failures is still masked later in the stop/save pipeline, so the PR does not fully eliminate the generic missing-file error it set out to replace. Aside from that, the approach is sound and the tests are appropriately scoped to the touched logic.
Key finding:
- 🟡 MEDIUM: actionable screencapture validation errors can still be replaced by the old generic missing-file failure in save/upload flows.
🧪 Browser testing: Skipped — PR only modifies backend/config/docs, no UI impact.
| .is_some() | ||
| { | ||
| return Ok(()); | ||
| return verify_screencapture_output(output_path, Some(status)); |
There was a problem hiding this comment.
🟡 Validated fallback errors are still masked by later save-path failures
stop_screencapture() now returns the new actionable missing-output/permission error, but upload/save flows still call into saved_recording_from_session() before they surface stop_outcome. In the single-segment fallback case, users can still end up with the old generic native recording file missing error instead of the new Screen Recording guidance.
Additional Info
Reported by 1/2 review agents; manually verified against take_and_finalize_active_session(), native_fullscreen_recording_stop_and_upload(), native_fullscreen_recording_stop_and_save(), and saved_recording_from_session().
|
thanks for the PR @TimpiaAI! theres just one potential issue cought by our bot - can you give it a look? |
Summary
screencapturewhen ScreenCaptureKit failed because Screen Recording permission is deniedFixes #1652.
Root cause
When ScreenCaptureKit failed with the macOS TCC denial path (
Content unavailable: The user declined TCCs for application, window, display capture), Clips still attempted the/usr/sbin/screencapturefallback. That fallback can exit without writing the requested recording file, but the recorder treated the fallback stop as successful. The save path was only checked later, which produced the unhelpful user-facing error:native recording file missing: No such file or directory (os error 2)This patch blocks the fallback for permission-denied ScreenCaptureKit failures and validates fallback output immediately when
screencaptureexits.Validation
cargo fmt --manifest-path templates/clips/desktop/src-tauri/Cargo.tomlcargo check --manifest-path templates/clips/desktop/src-tauri/Cargo.tomlcargo test --manifest-path templates/clips/desktop/src-tauri/Cargo.toml native_screen::tests -- --nocapturecargo checkand the focused test suite complete successfully. They still report the existingnative_preset_labeldead-code warning.