Skip to content

fix(mac): ignore stale picks from Apple's system picker - #1370

Open
EtienneLescot wants to merge 1 commit into
mainfrom
pilot/fix-1013
Open

EtienneLescot wants to merge 1 commit into
mainfrom
pilot/fix-1013

Conversation

@EtienneLescot

@EtienneLescot EtienneLescot commented Oct 23, 2026 •

Copy link
Copy Markdown
Member

What was wrong. With Apple's system picker (macOS 15.2+), after Hide desktop icons dropped the chosen source, the next Start recorded a window picked in an earlier picker session. The TS side is correct: the stale window arrives from the Swift helper as a new picker-selected. The helper set SCContentSharingPicker.shared.isActive = true once and never reset it, and reported every didUpdateWith as an answer, including ones no presentation asked for.

Fix (PickerSession.swift only):

  • Each present() deactivates the shared picker before reconfiguring and reactivating it (skipped while a take is running, so its stream is untouched).
  • A selection or cancel that arrives while no presentation is waiting is ignored: it no longer replaces the retained pick and is logged as a picker-unrequested-pick warning.

Refs #1013

Needs a manual check on a Mac with Apple's picker before merge (CI only builds and tests the helper):

  1. Pick a window with Apple's picker, record a take.
  2. Pick a screen (Record mode), then toggle Hide desktop icons so the source is dropped.
  3. Press Start: the picker must open and wait; the earlier window must not be recorded unless picked again.
  4. Cancel the picker once: nothing starts.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved screen capture picker reliability when starting a new selection without an active recording.
    • Prevented outdated picker selections from being accepted when no selection is in progress.
    • Ensured cancellation notifications are sent only for an active, non-stream picker interaction.

@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 37eaa97f-7339-409b-a377-f1e2f3423f4b
📥 Commits

Reviewing files that changed from the base of the PR and between 3a51ffd and 89a2abf.

📒 Files selected for processing: 1
  • electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/PickerSession.swift

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

PickerSession now tracks pending picker presentations. It resets the shared picker when no recorder is running and handles cancellations and selections only when a pick is pending.

Changes: Picker session handling

Layer / File(s) Summary
Track and handle pending picker selections
electron/native/screencapturekit/Sources/OpenScreenScreenCaptureKitHelper/PickerSession.swift
PickerSession tracks pending presentations, clears that state on cancellation or picker-start failure, and ignores selections received without a pending presentation.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 89a2a

The change makes the helper ignore stale picks from Apple's system picker and ignore cancellations that arrive with no pick pending. No concrete defect was established. The manual check on a Mac with Apple's picker, which the PR author describes, should still be done before merging.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Description check Warning The description explains the bug, fix, issue reference, and manual test steps, but it does not follow the required template. It omits the required section headings, change-type selection, release-impa… Rewrite the description using the repository template. Add the Summary, Related issue, Type of change, Release impact, Desktop impact, Screenshots / video, and Testing sections. Select the applicable checkboxes and place the manual validati…
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: preventing stale selections from Apple's macOS system picker.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the bug, fix, issue reference, and manual test steps, but it does not follow the required template. It omits the required section headings, change-type selection, release-impact selection, desktop-impact selection, and explicit testing section.

Resolution

Rewrite the description using the repository template. Add the Summary, Related issue, Type of change, Release impact, Desktop impact, Screenshots / video, and Testing sections. Select the applicable checkboxes and place the manual validation steps under Testing.

✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant