mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
Remove macOS notification permission checking (#404)
* fix: address review findings * fix: mock process.platform as darwin in system-denied test The test exercises the macOS notification permission gate, which only checks systemPreferences.getNotificationSettings on darwin. Without mocking the platform, Linux CI takes the non-darwin path and the assertion fails. * chore: remove stale test files * refactor: remove macOS notification permission checking The systemPreferences.getNotificationSettings() check added friction without meaningful benefit — macOS handles permission prompting natively when a notification is posted, making the manual check redundant. Removes getPermissionStatus(), the system-denied dispatch reason, the IPC handler, the renderer permission-blocked banner, and all related tests.
This commit is contained in:
@@ -9,8 +9,7 @@ const {
|
||||
notificationOnMock,
|
||||
notificationCtorMock,
|
||||
notificationIsSupportedMock,
|
||||
getAllWindowsMock,
|
||||
getNotificationSettingsMock
|
||||
getAllWindowsMock
|
||||
} = vi.hoisted(() => {
|
||||
const removeHandlerMock = vi.fn()
|
||||
const handleMock = vi.fn()
|
||||
@@ -26,7 +25,6 @@ const {
|
||||
})
|
||||
const notificationIsSupportedMock = vi.fn(() => true)
|
||||
const getAllWindowsMock = vi.fn(() => [])
|
||||
const getNotificationSettingsMock = vi.fn(() => ({ authorizationStatus: 'authorized' }))
|
||||
return {
|
||||
removeHandlerMock,
|
||||
handleMock,
|
||||
@@ -35,8 +33,7 @@ const {
|
||||
notificationOnMock,
|
||||
notificationCtorMock,
|
||||
notificationIsSupportedMock,
|
||||
getAllWindowsMock,
|
||||
getNotificationSettingsMock
|
||||
getAllWindowsMock
|
||||
}
|
||||
})
|
||||
|
||||
@@ -54,9 +51,6 @@ vi.mock('electron', () => ({
|
||||
app: {
|
||||
focus: vi.fn()
|
||||
},
|
||||
systemPreferences: {
|
||||
getNotificationSettings: getNotificationSettingsMock
|
||||
},
|
||||
shell: {
|
||||
openExternal: vi.fn()
|
||||
}
|
||||
@@ -64,8 +58,7 @@ vi.mock('electron', () => ({
|
||||
|
||||
import {
|
||||
registerNotificationHandlers,
|
||||
triggerStartupNotificationRegistration,
|
||||
getPermissionStatus
|
||||
triggerStartupNotificationRegistration
|
||||
} from './notifications'
|
||||
|
||||
describe('registerNotificationHandlers', () => {
|
||||
@@ -82,8 +75,6 @@ describe('registerNotificationHandlers', () => {
|
||||
notificationIsSupportedMock.mockReturnValue(true)
|
||||
getAllWindowsMock.mockReset()
|
||||
getAllWindowsMock.mockReturnValue([])
|
||||
getNotificationSettingsMock.mockReset()
|
||||
getNotificationSettingsMock.mockReturnValue({ authorizationStatus: 'authorized' })
|
||||
})
|
||||
|
||||
function getDispatchHandler(): (event: unknown, args: unknown) => unknown {
|
||||
@@ -130,36 +121,6 @@ describe('registerNotificationHandlers', () => {
|
||||
expect(notificationCtorMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('returns system-denied when macOS permission is denied', () => {
|
||||
// Why: getPermissionStatus only consults systemPreferences.getNotificationSettings
|
||||
// on macOS. On Linux CI the non-darwin branch would check Notification.isSupported()
|
||||
// instead, so we must force the darwin path to exercise the 'denied' gate.
|
||||
const originalPlatform = process.platform
|
||||
Object.defineProperty(process, 'platform', { value: 'darwin', configurable: true })
|
||||
|
||||
getNotificationSettingsMock.mockReturnValue({ authorizationStatus: 'denied' })
|
||||
|
||||
registerNotificationHandlers({
|
||||
getSettings: () => ({
|
||||
notifications: {
|
||||
enabled: true,
|
||||
agentTaskComplete: true,
|
||||
terminalBell: true,
|
||||
suppressWhenFocused: true
|
||||
}
|
||||
})
|
||||
} as never)
|
||||
|
||||
const handler = getDispatchHandler()
|
||||
expect(handler({}, { source: 'test' })).toEqual({
|
||||
delivered: false,
|
||||
reason: 'system-denied'
|
||||
})
|
||||
expect(notificationCtorMock).not.toHaveBeenCalled()
|
||||
|
||||
Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true })
|
||||
})
|
||||
|
||||
it('suppresses active-worktree notifications while Orca is focused', () => {
|
||||
getAllWindowsMock.mockReturnValue([
|
||||
{
|
||||
@@ -272,11 +233,9 @@ describe('registerNotificationHandlers', () => {
|
||||
|
||||
const handler = getDispatchHandler()
|
||||
|
||||
// Agent fires first — should deliver
|
||||
expect(handler({}, { source: 'agent-task-complete', worktreeId: 'repo::wt1' })).toEqual({
|
||||
delivered: true
|
||||
})
|
||||
// Bell fires immediately after for the same worktree — should be suppressed
|
||||
expect(handler({}, { source: 'terminal-bell', worktreeId: 'repo::wt1' })).toEqual({
|
||||
delivered: false,
|
||||
reason: 'cooldown'
|
||||
@@ -295,7 +254,6 @@ describe('triggerStartupNotificationRegistration', () => {
|
||||
notificationOnMock.mockClear()
|
||||
notificationIsSupportedMock.mockReset()
|
||||
notificationIsSupportedMock.mockReturnValue(true)
|
||||
getNotificationSettingsMock.mockReset()
|
||||
Object.defineProperty(process, 'platform', { value: 'darwin', configurable: true })
|
||||
})
|
||||
|
||||
@@ -303,9 +261,7 @@ describe('triggerStartupNotificationRegistration', () => {
|
||||
Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true })
|
||||
})
|
||||
|
||||
it('shows welcome notification when permission is not-determined and not yet requested', () => {
|
||||
getNotificationSettingsMock.mockReturnValue({ authorizationStatus: 'not determined' })
|
||||
|
||||
it('shows welcome notification when not yet requested', () => {
|
||||
const store = {
|
||||
getUI: () => ({ notificationPermissionRequested: undefined }),
|
||||
updateUI: vi.fn()
|
||||
@@ -321,23 +277,7 @@ describe('triggerStartupNotificationRegistration', () => {
|
||||
expect(notificationShowMock).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('does not fire when permission is already authorized', () => {
|
||||
getNotificationSettingsMock.mockReturnValue({ authorizationStatus: 'authorized' })
|
||||
|
||||
const store = {
|
||||
getUI: () => ({ notificationPermissionRequested: undefined }),
|
||||
updateUI: vi.fn()
|
||||
}
|
||||
|
||||
triggerStartupNotificationRegistration(store as never)
|
||||
|
||||
expect(notificationCtorMock).not.toHaveBeenCalled()
|
||||
expect(store.updateUI).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not fire when notificationPermissionRequested flag is set', () => {
|
||||
getNotificationSettingsMock.mockReturnValue({ authorizationStatus: 'not determined' })
|
||||
|
||||
const store = {
|
||||
getUI: () => ({ notificationPermissionRequested: true }),
|
||||
updateUI: vi.fn()
|
||||
@@ -350,8 +290,6 @@ describe('triggerStartupNotificationRegistration', () => {
|
||||
|
||||
it('does nothing on non-darwin platforms', () => {
|
||||
Object.defineProperty(process, 'platform', { value: 'linux', configurable: true })
|
||||
getNotificationSettingsMock.mockReturnValue({ authorizationStatus: 'not determined' })
|
||||
|
||||
const store = {
|
||||
getUI: () => ({ notificationPermissionRequested: undefined }),
|
||||
updateUI: vi.fn()
|
||||
@@ -362,44 +300,3 @@ describe('triggerStartupNotificationRegistration', () => {
|
||||
expect(notificationCtorMock).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
describe('getPermissionStatus', () => {
|
||||
const originalPlatform = process.platform
|
||||
|
||||
beforeEach(() => {
|
||||
getNotificationSettingsMock.mockReset()
|
||||
notificationIsSupportedMock.mockReset()
|
||||
Object.defineProperty(process, 'platform', { value: 'darwin', configurable: true })
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true })
|
||||
})
|
||||
|
||||
it('returns authorized when macOS reports authorized', () => {
|
||||
getNotificationSettingsMock.mockReturnValue({ authorizationStatus: 'authorized' })
|
||||
expect(getPermissionStatus()).toBe('authorized')
|
||||
})
|
||||
|
||||
it('returns denied when macOS reports denied', () => {
|
||||
getNotificationSettingsMock.mockReturnValue({ authorizationStatus: 'denied' })
|
||||
expect(getPermissionStatus()).toBe('denied')
|
||||
})
|
||||
|
||||
it('returns not-determined when macOS reports not determined', () => {
|
||||
getNotificationSettingsMock.mockReturnValue({ authorizationStatus: 'not determined' })
|
||||
expect(getPermissionStatus()).toBe('not-determined')
|
||||
})
|
||||
|
||||
it('returns authorized on non-darwin when Notification is supported', () => {
|
||||
Object.defineProperty(process, 'platform', { value: 'win32', configurable: true })
|
||||
notificationIsSupportedMock.mockReturnValue(true)
|
||||
expect(getPermissionStatus()).toBe('authorized')
|
||||
})
|
||||
|
||||
it('returns denied on non-darwin when Notification is not supported', () => {
|
||||
Object.defineProperty(process, 'platform', { value: 'linux', configurable: true })
|
||||
notificationIsSupportedMock.mockReturnValue(false)
|
||||
expect(getPermissionStatus()).toBe('denied')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -1,45 +1,12 @@
|
||||
import { app, BrowserWindow, Notification, ipcMain, systemPreferences, shell } from 'electron'
|
||||
import { app, BrowserWindow, Notification, ipcMain, shell } from 'electron'
|
||||
import type { Store } from '../persistence'
|
||||
import type { NotificationDispatchRequest, NotificationDispatchResult } from '../../shared/types'
|
||||
|
||||
const NOTIFICATION_COOLDOWN_MS = 5000
|
||||
|
||||
export type NotificationPermissionStatus = 'authorized' | 'denied' | 'not-determined' | 'unknown'
|
||||
|
||||
export function getPermissionStatus(): NotificationPermissionStatus {
|
||||
if (process.platform !== 'darwin') {
|
||||
// Windows/Linux don't have a per-app notification permission gate.
|
||||
return Notification.isSupported() ? 'authorized' : 'denied'
|
||||
}
|
||||
// Why: getNotificationSettings() is macOS-only and absent from Electron's
|
||||
// cross-platform type definitions, so we need the cast.
|
||||
const getSettings = (systemPreferences as unknown as Record<string, unknown>)
|
||||
.getNotificationSettings as (() => { authorizationStatus: string }) | undefined
|
||||
if (!getSettings) {
|
||||
return 'unknown'
|
||||
}
|
||||
const settings = getSettings()
|
||||
switch (settings.authorizationStatus) {
|
||||
case 'authorized':
|
||||
case 'provisional':
|
||||
return 'authorized'
|
||||
case 'denied':
|
||||
return 'denied'
|
||||
case 'not determined':
|
||||
return 'not-determined'
|
||||
default:
|
||||
return 'unknown'
|
||||
}
|
||||
}
|
||||
|
||||
export function registerNotificationHandlers(store: Store): void {
|
||||
const recentNotifications = new Map<string, number>()
|
||||
|
||||
ipcMain.removeHandler('notifications:getPermissionStatus')
|
||||
ipcMain.handle('notifications:getPermissionStatus', (): NotificationPermissionStatus => {
|
||||
return getPermissionStatus()
|
||||
})
|
||||
|
||||
ipcMain.removeHandler('notifications:openSystemSettings')
|
||||
ipcMain.handle('notifications:openSystemSettings', (): void => {
|
||||
if (process.platform === 'darwin') {
|
||||
@@ -63,16 +30,6 @@ export function registerNotificationHandlers(store: Store): void {
|
||||
return { delivered: false, reason: 'disabled' }
|
||||
}
|
||||
|
||||
// Why: even when in-app notifications are enabled, macOS can independently
|
||||
// block them at the system level. Checking here prevents a silent no-op
|
||||
// and lets the renderer show actionable feedback. We only block on 'denied'
|
||||
// — 'not-determined' is allowed through because posting the notification is
|
||||
// what triggers the macOS permission dialog for the first time.
|
||||
const permissionStatus = getPermissionStatus()
|
||||
if (permissionStatus === 'denied') {
|
||||
return { delivered: false, reason: 'system-denied' }
|
||||
}
|
||||
|
||||
if (
|
||||
(args.source === 'agent-task-complete' && !settings.agentTaskComplete) ||
|
||||
(args.source === 'terminal-bell' && !settings.terminalBell)
|
||||
@@ -158,10 +115,6 @@ export function triggerStartupNotificationRegistration(store: Store): void {
|
||||
if (process.platform !== 'darwin' || !Notification.isSupported()) {
|
||||
return
|
||||
}
|
||||
const status = getPermissionStatus()
|
||||
if (status !== 'not-determined') {
|
||||
return
|
||||
}
|
||||
// Why: only fire once per install — not on every launch where status stays
|
||||
// not-determined (e.g. if the user dismisses the macOS dialog without choosing).
|
||||
const ui = store.getUI()
|
||||
|
||||
Vendored
-3
@@ -116,11 +116,8 @@ type CliApi = {
|
||||
remove: () => Promise<CliInstallStatus>
|
||||
}
|
||||
|
||||
type NotificationPermissionStatus = 'authorized' | 'denied' | 'not-determined' | 'unknown'
|
||||
|
||||
type NotificationsApi = {
|
||||
dispatch: (args: NotificationDispatchRequest) => Promise<NotificationDispatchResult>
|
||||
getPermissionStatus: () => Promise<NotificationPermissionStatus>
|
||||
openSystemSettings: () => Promise<void>
|
||||
}
|
||||
|
||||
|
||||
@@ -283,8 +283,6 @@ const api = {
|
||||
notifications: {
|
||||
dispatch: (args: Record<string, unknown>): Promise<NotificationDispatchResult> =>
|
||||
ipcRenderer.invoke('notifications:dispatch', args),
|
||||
getPermissionStatus: (): Promise<string> =>
|
||||
ipcRenderer.invoke('notifications:getPermissionStatus'),
|
||||
openSystemSettings: (): Promise<void> => ipcRenderer.invoke('notifications:openSystemSettings')
|
||||
},
|
||||
|
||||
|
||||
@@ -1,10 +1,10 @@
|
||||
import { type ReactNode, useCallback, useEffect, useState } from 'react'
|
||||
import { type ReactNode } from 'react'
|
||||
import { toast } from 'sonner'
|
||||
import type { GlobalSettings } from '../../../../shared/types'
|
||||
import { Button } from '../ui/button'
|
||||
import { Label } from '../ui/label'
|
||||
import { Separator } from '../ui/separator'
|
||||
import { BellRing, Bot, Siren, TriangleAlert } from 'lucide-react'
|
||||
import { BellRing, Bot, Siren } from 'lucide-react'
|
||||
import type { SettingsSearchEntry } from './settings-search'
|
||||
|
||||
export const NOTIFICATIONS_PANE_SEARCH_ENTRIES: SettingsSearchEntry[] = [
|
||||
@@ -45,25 +45,6 @@ export function NotificationsPane({
|
||||
updateSettings
|
||||
}: NotificationsPaneProps): React.JSX.Element {
|
||||
const notificationSettings = settings.notifications
|
||||
const [permissionBlocked, setPermissionBlocked] = useState(false)
|
||||
|
||||
const recheckPermission = useCallback(() => {
|
||||
void window.api.notifications.getPermissionStatus().then((status) => {
|
||||
// Why: only flag 'denied', not 'not-determined'. 'not-determined' means
|
||||
// macOS hasn't prompted the user yet — the startup registration or the
|
||||
// next notification dispatch will trigger that dialog. Showing "blocked"
|
||||
// for a permission that was never asked would be misleading.
|
||||
setPermissionBlocked(status === 'denied')
|
||||
})
|
||||
}, [])
|
||||
|
||||
useEffect(() => {
|
||||
if (!notificationSettings.enabled) {
|
||||
setPermissionBlocked(false)
|
||||
return
|
||||
}
|
||||
recheckPermission()
|
||||
}, [notificationSettings.enabled, recheckPermission])
|
||||
|
||||
const updateNotificationSettings = (updates: Partial<GlobalSettings['notifications']>): void => {
|
||||
updateSettings({
|
||||
@@ -76,40 +57,13 @@ export function NotificationsPane({
|
||||
|
||||
const handleSendTestNotification = async (): Promise<void> => {
|
||||
const result = await window.api.notifications.dispatch({ source: 'test' })
|
||||
if (!result.delivered && result.reason === 'system-denied') {
|
||||
// Why: don't auto-open System Settings — the yellow banner above already
|
||||
// provides the "Open Notification Settings" button. Just tell the user
|
||||
// what happened so the silent failure isn't confusing.
|
||||
toast.error('Notification blocked by macOS', {
|
||||
description: 'Allow notifications for Orca in System Settings → Notifications.'
|
||||
})
|
||||
if (result.delivered) {
|
||||
toast.success('Test notification sent')
|
||||
}
|
||||
// Why: the macOS permission dialog may have appeared (for 'not-determined'
|
||||
// status), so re-check to update the banner accordingly.
|
||||
recheckPermission()
|
||||
}
|
||||
|
||||
return (
|
||||
<div className="space-y-1">
|
||||
{notificationSettings.enabled && permissionBlocked && (
|
||||
<div className="mx-1 mb-2 flex items-start gap-2.5 rounded-md border border-yellow-500/30 bg-yellow-500/10 px-3 py-2.5">
|
||||
<TriangleAlert className="mt-0.5 size-3.5 shrink-0 text-yellow-500" />
|
||||
<div className="space-y-1.5">
|
||||
<p className="text-xs text-foreground">
|
||||
Notifications are blocked by your system settings.
|
||||
</p>
|
||||
<Button
|
||||
variant="outline"
|
||||
size="sm"
|
||||
className="h-6 text-xs"
|
||||
onClick={() => void window.api.notifications.openSystemSettings()}
|
||||
>
|
||||
Open Notification Settings
|
||||
</Button>
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
|
||||
<SettingToggle
|
||||
label="Enable Notifications"
|
||||
description="Native system notifications for background events."
|
||||
|
||||
+1
-7
@@ -339,13 +339,7 @@ export type NotificationDispatchRequest = {
|
||||
export type NotificationDispatchResult = {
|
||||
delivered: boolean
|
||||
/** Present when delivered is false. Tells the caller why delivery was skipped. */
|
||||
reason?:
|
||||
| 'disabled'
|
||||
| 'source-disabled'
|
||||
| 'suppressed-focus'
|
||||
| 'cooldown'
|
||||
| 'not-supported'
|
||||
| 'system-denied'
|
||||
reason?: 'disabled' | 'source-disabled' | 'suppressed-focus' | 'cooldown' | 'not-supported'
|
||||
}
|
||||
|
||||
export type WorktreeCardProperty = 'status' | 'unread' | 'ci' | 'issue' | 'pr' | 'comment'
|
||||
|
||||
Reference in New Issue
Block a user