fix(desktop): streamline macOS permission requests and onboarding#1703
fix(desktop): streamline macOS permission requests and onboarding#1703richiemcilroy merged 2 commits intomainfrom
Conversation
Paragon SummaryThis pull request review analyzed 5 files and found no issues. The review examined code changes, potential bugs, security vulnerabilities, performance issues, and code quality concerns using automated analysis tools. Paragon did not detect any problems in the current diff. Proceed with merge after your normal checks. This PR streamlines macOS permission handling in the desktop app by centralizing request, verification, and settings-opening flows across Rust and shared TypeScript logic. It makes onboarding and the main permission hook use the same path, ensuring consistent behavior when access is denied or after users change system settings. Key changes:
Confidence score: 5/5
5 files reviewed, 0 comments |
![paragon-review[bot]](https://avatars.githubusercontent.com/in/1544258?s=60&v=4){kind=small}
richiemcilroy
force-pushed
the
feat/desktop-permissions-flow
branch
2 times, most recently
from
62dc964 to
bae447c
Compare
richiemcilroy
force-pushed
the
feat/desktop-permissions-flow
branch
from
bae447c to
1ff0cb2
Compare
|
@greptileai please review the PR :) |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| fn macos_open_permission_settings(app: &tauri::AppHandle, permission: &OSPermission) { | ||
| use std::process::Command; | ||
|
|
||
| let process = Command::new("open") |
There was a problem hiding this comment.
Minor nit: open returns as soon as it launches System Settings unless you pass -W, so the spawn_blocking(process.wait()) branch effectively runs immediately (not after the user is done in Settings). If the goal is to react when the user returns from Settings, consider either using open -W ... or dropping the wait/async here and triggering any re-checks off app focus/foreground events instead.
| macos_request_permission(&app, &permission); | ||
| }) | ||
| .await | ||
| .ok(); |
There was a problem hiding this comment.
Swallowing the JoinError here makes failures hard to diagnose (e.g. if the blocking task panics). Logging the error would help without changing control flow.
| .ok(); | |
| if let Err(err) = tauri::async_runtime::spawn_blocking(move || { | |
| macos_request_permission(&app, &permission); | |
| }) | |
| .await | |
| { | |
| tracing::error!("Permission request task failed: {err}"); | |
| } |
| }; | ||
| } | ||
|
|
||
| await client.openPermissionSettings(permission); |
There was a problem hiding this comment.
After opening settings, this currently returns the pre-settings check/status (and differs from the currentStatus === "denied" branch which re-checks). Re-checking after openPermissionSettings keeps the return value consistent and would also work if openPermissionSettings later starts waiting for the user.
| await client.openPermissionSettings(permission); | |
| await client.openPermissionSettings(permission); | |
| const updatedCheck = await client.doPermissionsCheck(false); | |
| return { | |
| check: updatedCheck, | |
| status: permissionStatusFor(updatedCheck, permission), | |
| openedSettings: true, | |
| }; |
| setRequestingPermission(true); | ||
| try { | ||
| await commands.requestPermission(permission); | ||
| const status = check()?.[permission] as OSPermissionStatus | undefined; |
There was a problem hiding this comment.
Redundant cast here — check() is already Record<string, OSPermissionStatus> | undefined, so check()?.[permission] is already OSPermissionStatus | undefined.
| const status = check()?.[permission] as OSPermissionStatus | undefined; | |
| const status = check()?.[permission]; |
| permission: OSPermission, | ||
| currentStatus?: OSPermissionStatus, | ||
| ): Promise<PermissionRequestResult> { | ||
| if (currentStatus === "denied") { |
There was a problem hiding this comment.
Since openPermissionSettings just launches System Settings, doing doPermissionsCheck(false) immediately after usually won't reflect any user change yet. Checking first (then opening settings) avoids returning a potentially misleading check snapshot.
| if (currentStatus === "denied") { | |
| if (currentStatus === "denied") { | |
| const check = await client.doPermissionsCheck(false); | |
| await client.openPermissionSettings(permission); | |
| return { | |
| check, | |
| status: permissionStatusFor(check, permission), | |
| openedSettings: true, | |
| }; | |
| } |
| }; | ||
| } | ||
|
|
||
| await client.openPermissionSettings(permission); |
There was a problem hiding this comment.
Heads-up: request_permission in Rust now opens settings for screenRecording/accessibility when still ungranted. This helper will then call openPermissionSettings again for those permissions, which can lead to duplicate open invocations (and potentially multiple focus/activation jumps). Might be worth making a single layer responsible for the fallback-to-settings behavior.
| macos_request_permission(&app, &permission); | ||
| }) | ||
| .await | ||
| .ok(); |
There was a problem hiding this comment.
Swallowing the join result here makes permission failures hard to diagnose. Logging the join error keeps this debuggable without changing behavior.
| .ok(); | |
| if let Err(err) = tauri::async_runtime::spawn_blocking(move || { | |
| macos_request_permission(&app, &permission); | |
| }) | |
| .await | |
| { | |
| tracing::error!("Permission request task failed: {err}"); | |
| } |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| tauri::async_runtime::spawn_blocking(move || { | ||
| macos_request_permission(&app, &permission); | ||
| }) | ||
| .await | ||
| .ok(); | ||
|
|
||
| match _permission { | ||
| OSPermission::ScreenRecording => { | ||
| macos_prompt_screen_recording_access(); | ||
| } | ||
| OSPermission::Camera => { | ||
| tauri::async_runtime::spawn_blocking(|| { | ||
| futures::executor::block_on(av::CaptureDevice::request_access_for_media_type( | ||
| av::MediaType::video(), | ||
| )) | ||
| .ok(); | ||
| }) | ||
| .await | ||
| .ok(); | ||
| } | ||
| OSPermission::Microphone => { | ||
| tauri::async_runtime::spawn_blocking(|| { | ||
| futures::executor::block_on(av::CaptureDevice::request_access_for_media_type( | ||
| av::MediaType::audio(), | ||
| )) | ||
| .ok(); | ||
| }) | ||
| .await | ||
| .ok(); | ||
| } | ||
| OSPermission::Accessibility => { | ||
| macos_prompt_accessibility_access(); | ||
| } | ||
| } | ||
| let granted = macos_wait_for_permission_update(&_permission).await; | ||
|
|
||
| if needs_activation | ||
| && let Err(err) = _app.set_activation_policy(tauri::ActivationPolicy::Accessory) | ||
| { | ||
| tracing::warn!("Failed to restore activation policy to Accessory: {err}"); | ||
| if macos_permission_needs_settings_fallback(&_permission) && !granted { | ||
| macos_open_permission_settings(&_app, &_permission); | ||
| } else { | ||
| macos_restore_activation_policy(&_app); | ||
| } |
There was a problem hiding this comment.
open_permission_settings invocation for ScreenRecording / Accessibility
When request_permission is called for ScreenRecording or Accessibility and the permission is not granted within the 2-second poll window, macos_open_permission_settings is called here in Rust. Control then returns to the TypeScript caller, which calls doPermissionsCheck(false) and — because the status is still denied/empty — also calls client.openPermissionSettings(...) (see os-permissions.ts line 67). This triggers open_permission_settings on the Rust side a second time, spawning a second open sysprefs://… subprocess and issuing a second macos_activate_permission_request / macos_restore_activation_policy cycle.
Camera and Microphone are exempt because macos_permission_needs_settings_fallback returns false for them — the TypeScript layer is the sole owner of their settings-fallback path. The same responsibility split should apply to ScreenRecording and Accessibility.
let granted = macos_wait_for_permission_update(&_permission).await;
- if macos_permission_needs_settings_fallback(&_permission) && !granted {
- macos_open_permission_settings(&_app, &_permission);
- } else {
- macos_restore_activation_policy(&_app);
- }
+ macos_restore_activation_policy(&_app);With this change requestAndVerifyPermission becomes the single place that decides whether to open settings for all four permission types.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/permissions.rs
Line: 304-316
Comment:
**Double `open_permission_settings` invocation for ScreenRecording / Accessibility**
When `request_permission` is called for `ScreenRecording` or `Accessibility` and the permission is not granted within the 2-second poll window, `macos_open_permission_settings` is called here in Rust. Control then returns to the TypeScript caller, which calls `doPermissionsCheck(false)` and — because the status is still `denied`/`empty` — also calls `client.openPermissionSettings(...)` (see `os-permissions.ts` line 67). This triggers `open_permission_settings` on the Rust side a **second time**, spawning a second `open sysprefs://…` subprocess and issuing a second `macos_activate_permission_request` / `macos_restore_activation_policy` cycle.
Camera and Microphone are exempt because `macos_permission_needs_settings_fallback` returns `false` for them — the TypeScript layer is the sole owner of their settings-fallback path. The same responsibility split should apply to ScreenRecording and Accessibility.
```rust
let granted = macos_wait_for_permission_update(&_permission).await;
- if macos_permission_needs_settings_fallback(&_permission) && !granted {
- macos_open_permission_settings(&_app, &_permission);
- } else {
- macos_restore_activation_policy(&_app);
- }
+ macos_restore_activation_policy(&_app);
```
With this change `requestAndVerifyPermission` becomes the single place that decides whether to open settings for all four permission types.
How can I resolve this? If you propose a fix, please make it concise.| fn macos_run_on_main_thread<R: Send + 'static>( | ||
| app: &tauri::AppHandle, | ||
| callback: impl FnOnce() -> R + Send + 'static, | ||
| ) -> Option<R> { | ||
| use std::sync::mpsc; | ||
|
|
||
| let (tx, rx) = mpsc::sync_channel(1); | ||
|
|
||
| if let Err(err) = app.run_on_main_thread(move || { | ||
| let _ = tx.send(callback()); | ||
| }) { | ||
| tracing::warn!("Failed to run permission action on main thread: {err}"); | ||
| return None; | ||
| } | ||
|
|
||
| rx.recv_timeout(Duration::from_secs(2)).ok() | ||
| } |
There was a problem hiding this comment.
run_on_main_thread succeeds (returns Ok) and the closure is queued on the main thread, but recv_timeout can still expire after 2 seconds if the main thread is slow to process it. When that happens, None is returned and the fallback immediately calls the callback again on the current thread — meaning the callback could execute on both threads nearly simultaneously.
For macos_prompt_screen_recording_access and macos_prompt_accessibility_access a duplicate OS-prompt call is harmless, but this is a latent risk if the helper is reused for stateful side effects in the future.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/permissions.rs
Line: 46-62
Comment:
**Potential double-execution on main-thread dispatch timeout**
`run_on_main_thread` succeeds (returns `Ok`) and the closure is queued on the main thread, but `recv_timeout` can still expire after 2 seconds if the main thread is slow to process it. When that happens, `None` is returned and the fallback immediately calls the callback again on the current thread — meaning the callback could execute on both threads nearly simultaneously.
For `macos_prompt_screen_recording_access` and `macos_prompt_accessibility_access` a duplicate OS-prompt call is harmless, but this is a latent risk if the helper is reused for stateful side effects in the future.
How can I resolve this? If you propose a fix, please make it concise.| if (currentStatus === "denied") { | ||
| await client.openPermissionSettings(permission); | ||
| const check = await client.doPermissionsCheck(false); | ||
| return { | ||
| check, | ||
| status: permissionStatusFor(check, permission), | ||
| openedSettings: true, | ||
| }; |
There was a problem hiding this comment.
doPermissionsCheck result in the "denied" fast-path is immediately stale
When currentStatus === "denied", openPermissionSettings is called and returns as soon as the open subprocess is spawned (fire-and-forget). The subsequent doPermissionsCheck(false) therefore always returns the same denied state already known before the call, so the status field of the returned PermissionRequestResult carries no new information. Callers updating UI state from result.check will see an identical stale snapshot.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/utils/os-permissions.ts
Line: 44-51
Comment:
**`doPermissionsCheck` result in the "denied" fast-path is immediately stale**
When `currentStatus === "denied"`, `openPermissionSettings` is called and returns as soon as the `open` subprocess is spawned (fire-and-forget). The subsequent `doPermissionsCheck(false)` therefore always returns the same denied state already known before the call, so the `status` field of the returned `PermissionRequestResult` carries no new information. Callers updating UI state from `result.check` will see an identical stale snapshot.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Refactors macOS permission handling in the Tauri shell and centralizes request, verify, and open-settings flows in Rust plus a shared TypeScript helper. Onboarding and the main-window permission hook use the same path for consistent behavior when access is denied or after system settings.
Greptile Summary
This PR refactors macOS permission handling by centralising request, verify, and settings-open flows into reusable Rust helpers and a new shared TypeScript utility (
requestAndVerifyPermission). Both onboarding and the main-window permission hook are updated to use the shared path, eliminating duplicated logic. Unit tests are added for both layers.Key changes:
permissions.rsextracts per-permission logic into small testable helpers and adds amacos_activate_permission_request/macos_restore_activation_policypair managing dock-icon activation state consistently.os-permissions.tsintroduces aPermissionClientinterface andrequestAndVerifyPermissionhandling the denied fast-path and normal request-then-verify path.useRequestPermission.tsandonboarding.tsxare simplified to delegate to the shared helper.Issue found: For
ScreenRecordingandAccessibility, both the Rustrequest_permissioncommand and the TypeScriptrequestAndVerifyPermissionindependently open System Preferences when permission is not granted, resulting in a duplicate invocation.Confidence Score: 4/5
Safe to merge after resolving the double System Preferences invocation for ScreenRecording/Accessibility.
One P1 logic bug: for ScreenRecording and Accessibility on a first-time request, both Rust and TypeScript independently open System Preferences, resulting in two concurrent subprocess calls and racing restore callbacks. The refactoring is otherwise well-structured and the new tests are meaningful.
apps/desktop/src-tauri/src/permissions.rs — the macos_permission_needs_settings_fallback branch inside request_permission should be removed so TypeScript is the sole owner of the settings-fallback decision for all permission types.
Important Files Changed
request_permissionopens System Preferences for ScreenRecording/Accessibility while the TypeScript caller also opens it, causing a double invocation.PermissionClientinterface. P2:doPermissionsCheckin the denied fast-path returns a stale result.requestAndVerifyPermission; good use of mocks and clear assertions.requestAndVerifyPermission; usesresult.openedSettingsto gate the restart prompt for screen recording.requestAndVerifyPermission, removing duplicated denied-path handling.Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(desktop): streamline macos permissio..." | Re-trigger Greptile