fix(macos): raise the Screen Recording prompt on first run - #302
fix(macos): raise the Screen Recording prompt on first run#302heyitsR1 wants to merge 2 commits into
Conversation
`requestScreenAccess` only raised the TCC prompt when
`getMediaAccessStatus("screen")` returned "not-determined", which macOS
never reports. Chromium resolves that permission through
`CGPreflightScreenCaptureAccess()`, a bool, so a machine that has never
been asked is indistinguishable from an explicit refusal and both arrive
as "denied".
A first run therefore fell straight through to the "open System Settings"
dialog without macOS ever being asked, leaving a manual toggle as the only
way to grant. The renderer's retry loop in openSourceSelectorFlow arms on
the same status and never ran either.
Decide from whether this launch has already asked instead. The first ask
raises the prompt and reports "not-determined" so the retry loop arms;
later asks report the real status so the Settings dialog still reaches a
user who genuinely refused, without re-prompting on every click.
Verified on macOS 26.2: with no TCC screen-capture row for the bundle,
getMediaAccessStatus("screen") returns "denied" while
getMediaAccessStatus("camera") returns "not-determined".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change adds screen-access prompt decision helpers, tracks prompt timestamps per launch, updates macOS IPC handling, and adds tests for permission states and the prompt grace period. ChangesScreen access prompt handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant IPCHandler
participant ScreenAccessPrompt
participant macOS
IPCHandler->>ScreenAccessPrompt: evaluate permission status and prompt timestamp
ScreenAccessPrompt-->>IPCHandler: return prompt or pending-response decision
IPCHandler->>macOS: request screen access when prompting is allowed
macOS-->>IPCHandler: return permission status
IPCHandler-->>IPCHandler: report not-determined during the grace period
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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[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 459-460: Track the native Screen Recording request’s in-flight
state separately from hasPromptedForScreenAccess in the relevant IPC handler.
Set the in-flight flag before awaiting desktopCapturer.getSources(), clear it
after settlement, and return "not-determined" for concurrent requests until the
first call resolves. Add a handler-level regression test using a deferred
getSources() promise that issues a second request before resolution and verifies
this behavior.
🪄 Autofix
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: 5b6d6121-63f3-47d4-9df4-58adbba99965
📒 Files selected for processing (3)
electron/ipc/handlers.tselectron/ipc/screenAccessPrompt.test.tselectron/ipc/screenAccessPrompt.ts
| /** macOS raises its Screen Recording prompt once per launch. */ | ||
| let hasPromptedForScreenAccess = false; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Track the active native prompt separately.
Line 1687 marks the launch as prompted before desktopCapturer.getSources() settles. A retry can then return "denied" while the first request is still pending. open-source-selector can open System Settings over the native Screen Recording prompt.
Add an in-flight flag. Return "not-determined" until getSources() settles. Add a handler-level regression test with a deferred getSources() Promise and a second request before it resolves.
Proposed fix
/** macOS raises its Screen Recording prompt once per launch. */
let hasPromptedForScreenAccess = false;
+let screenAccessPromptInFlight = false;
+ if (screenAccessPromptInFlight) {
+ return { success: true, granted: false, status: "not-determined" };
+ }
if (shouldPromptForScreenAccess(status, hasPromptedForScreenAccess)) {
hasPromptedForScreenAccess = true;
+ screenAccessPromptInFlight = true;
const mainWin = getMainWindow();
// ...
- desktopCapturer
+ void desktopCapturer
.getSources({ types: ["screen"], thumbnailSize: { width: 1, height: 1 } })
.catch(() => {
// Permission probing failure is reported by the explicit status check below.
- });
+ })
+ .finally(() => {
+ screenAccessPromptInFlight = false;
+ });
return { success: true, granted: false, status: "not-determined" };
}As per coding guidelines, “Add a test for every new behavior in the same package as the code under test.”
Also applies to: 1686-1687
🧰 Tools
🪛 ast-grep (0.45.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 459 - 460, Track the native Screen
Recording request’s in-flight state separately from hasPromptedForScreenAccess
in the relevant IPC handler. Set the in-flight flag before awaiting
desktopCapturer.getSources(), clear it after settlement, and return
"not-determined" for concurrent requests until the first call resolves. Add a
handler-level regression test using a deferred getSources() promise that issues
a second request before resolution and verifies this behavior.
Source: Coding guidelines
Raising the prompt and immediately reporting the real status let open-source-selector show the Settings dialog over the native prompt, and stopped the renderer's retry loop on its first poll. An in-flight flag around getSources() does not cover this. Measured on macOS 26.2, that call settles in 4ms whether or not the prompt is still on screen (it rejects outright when access is denied), and the status stays "denied" for as long as the prompt is up — it only flips once the user accepts. So there is nothing observable to wait on. Time-box it instead: keep reporting "not-determined" for the renderer's retry budget after asking, then let the real status through so the Settings dialog still reaches a user who refused. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Good catch on the symptom — System Settings can indeed open over the native prompt. I took a different fix than the proposed one, because I measured the assumption it rests on and it doesn't hold. An in-flight flag around It settles in 4ms — it rejects outright rather than waiting — so the flag would be true for about four milliseconds and the renderer's first retry, 750ms later, would still see The measurement also shows the problem is wider than the race: macOS keeps answering Since macOS exposes nothing to wait on, Tests are in |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
electron/ipc/handlers.ts (1)
1686-1710: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd an IPC-level regression test for the screen access grace path.
screenAccessPrompt.test.tscovers the helper only. Add arequest-screen-accesshandler test with mocked clock, media status, anddesktopCapturer.getSources. Verify the first prompt request,"not-determined"during the grace window, and"denied"after the window expires.🤖 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 1686 - 1710, The request-screen-access IPC handler lacks regression coverage for its screen-access grace period. Add an IPC-level test around the request-screen-access handler, mocking the clock, media status, and desktopCapturer.getSources, and verify the initial prompt request, a not-determined response during the grace window, and a denied response after the grace window expires.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@electron/ipc/handlers.ts`:
- Around line 1686-1710: The request-screen-access IPC handler lacks regression
coverage for its screen-access grace period. Add an IPC-level test around the
request-screen-access handler, mocking the clock, media status, and
desktopCapturer.getSources, and verify the initial prompt request, a
not-determined response during the grace window, and a denied response after the
grace window expires.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 87795d40-43ae-48fc-88ed-c3f90c4bbfbe
📒 Files selected for processing (3)
electron/ipc/handlers.tselectron/ipc/screenAccessPrompt.test.tselectron/ipc/screenAccessPrompt.ts
|
On the IPC-level test for the grace path — I'd rather not add it, and I tried it before deciding.
Getting to a first assertion means faking Electron runtime internals across a 4,088-line module with 64 The repo's existing answer to this is the pattern I followed: lift the logic into a small pure module next to the code and test that ( Happy to add it if you'd prefer, or to open a separate PR that makes |
1 participant
Summary
On macOS, OpenScreen never raises the system Screen Recording prompt. A fresh install goes straight to the "Screen Recording permission is required" dialog, so the only way to grant is toggling the app manually in System Settings.
requestScreenAccess()gates the prompt onstatus === "not-determined":That branch is unreachable on macOS. Electron resolves the status through Chromium, which reads the permission with
CGPreflightScreenCaptureAccess()— a bool (ui/base/cocoa/permissions_utils.mm):There is no third state, so "never asked" and "explicitly refused" both arrive as
denied.Repro on macOS 26.2 / Electron 41.2.1, from one process with no
kTCCServiceScreenCapturerow for its bundle id — never asked for either permission:Same call, same instant. That asymmetry is the whole bug, and it is why the camera path just below in
handlers.tsworks while the screen path never fires.Two consequences:
open-source-selectoralways shows the Settings dialog on first run.shouldRetryAfterPermissionPrompt()inopenSourceSelectorFlow.tsonly arms whenaccess?.status === "not-determined", so the 8×750ms poll that reopens the selector after a grant never runs.electron/main.tsalready describes the intended behaviour — "Screen Recording is requested lazily from the source-picker action so its prompt isn't hidden behind the selector window" — and the microphone request beside it correctly keys off!== "granted".What changes
Decide from whether this launch has already asked rather than from the status.
New pure helpers in
electron/ipc/screenAccessPrompt.ts(with the reasoning documented) plus colocated unit tests;requestScreenAccess()calls them and records when it asked.not-determinedso the existing retry loop armsnot-determinedheld for the renderer's retry budget, so the dialog can't open over the promptnot-determined(non-macOS / future Electron)The grace window exists because macOS gives nothing to wait on.
desktopCapturer.getSources()settles in 4ms whether or not the prompt is still on screen, and the status staysdeniedthe entire time the prompt is up — it only flips once the user accepts:So an in-flight flag around that call would cover about four milliseconds, and reporting the real status straight away would abort
openSourceSelectorFlowon its first poll.SCREEN_PROMPT_GRACE_MSis set to the renderer's own budget (8 × 750ms).The prompt mechanism itself (
desktopCapturer.getSources, window focus) is untouched — this only changes when it is reached.Why it matters beyond the dialog
Because the prompt never runs, users grant through the System Settings toggle instead. On my machine that path produced a TCC row pinned to a bare cdhash rather than the app's designated requirement:
against an installed 1.5.0 whose cdhash is
8a171c66…. System Settings drew the toggle as on (the row says allowed) while every runtime check failed the requirement — a loop that survives app and machine restarts and looks unfixable to the user. The same app's Microphone, Camera and Audio rows, granted through real OS prompts in the same session, all carry the normal requirement and work:I can't fully explain how that particular hash was written and I'm not claiming this PR fixes TCC. But routing first-run grants through the OS prompt is the path that reliably records the signature-based requirement, and it is what every other permission in this app already does.
Related issue
No open issue on this repo — found while debugging a fresh Homebrew install. Happy to file one if you'd like it tracked in the milestone flow.
Type of change
Release impact
Desktop impact
Screenshots / video
No UI change — the difference is which dialog macOS shows on first run (its own prompt instead of OpenScreen's fallback).
Testing
Node 22.22.1 / npm 10.9.4, macOS 26.2 (arm64):
npx vitest --run electron/ipc/screenAccessPrompt.test.ts— 7 passednpm run test— 141 files, 1684 passed, 1 skippednpm run lint— exit 0; warning count identical tomain(13 before and after), none in the touched filesnpx tsc --noEmit— cleannpx tsc -p tsconfig.test.json --noEmit— cleannpm run docs:check— OK (22 files)The helpers are pure and have no
process.platformbranch, so the tests need no platform pinning.nowis injected rather than read from the clock, so the grace-window cases are deterministic without fake timers.Verified on real macOS, per AGENTS.md's requirement for permission changes: the
SCREEN_STATUS=denied/CAMERA_STATUS=not-determinedrepro above was measured on this hardware from a virgin TCC state, which is the premise the fix rests on.Partially verified end to end: calling
desktopCapturer.getSources()from a virgin-TCC bundle did engage macOS — it wrote akTCCServiceScreenCapturerow for the probe bundle (auth_value=0), which I reset afterwards. So the prompt path reaches TCC. What I did not do is accept the prompt and observe the status flip, since I have no desktop automation here. The prompt-raising code itself is unchanged by this PR — what changes is reachability, which the unit tests and the measurements above cover. Still worth a maintainer smoke test on a Mac that has never granted OpenScreen before merging.One caveat I'd flag: the grant may still only take effect on the next launch, since macOS can keep the process's cached answer at
deniedeven after the user accepts. The grace window gives the retry loop its chance either way, and the OS prompt now runs so TCC records a proper signature-based entry — but a "restart OpenScreen to finish enabling" hint could be a sensible follow-up.