From 84614a4c7cc220af7fc6a1a2b57ca52afcbcb45a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 20 May 2026 23:10:06 -0700 Subject: [PATCH] Fix notification settings recovery copy --- src/main/ipc/notifications.test.ts | 24 ++++ .../settings/NotificationsPane.test.tsx | 115 +++++++++++++++++- .../components/settings/NotificationsPane.tsx | 113 ++++++++++++++--- src/shared/types.ts | 2 +- 4 files changed, 234 insertions(+), 20 deletions(-) diff --git a/src/main/ipc/notifications.test.ts b/src/main/ipc/notifications.test.ts index 3f76973b4..a2272a2ad 100644 --- a/src/main/ipc/notifications.test.ts +++ b/src/main/ipc/notifications.test.ts @@ -203,6 +203,30 @@ describe('registerNotificationHandlers', () => { } }) + it('opens Windows notification settings', () => { + const originalPlatform = process.platform + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }) + try { + registerNotificationHandlers({ + getSettings: () => ({ + notifications: { + enabled: true, + agentTaskComplete: true, + terminalBell: true, + suppressWhenFocused: true + } + }) + } as never) + + const handler = getOpenSystemSettingsHandler() + handler({}) + + expect(shellOpenExternalMock).toHaveBeenCalledWith('ms-settings:notifications') + } finally { + Object.defineProperty(process, 'platform', { value: originalPlatform, configurable: true }) + } + }) + it('suppresses notifications when disabled in settings', () => { registerNotificationHandlers({ getSettings: () => ({ diff --git a/src/renderer/src/components/settings/NotificationsPane.test.tsx b/src/renderer/src/components/settings/NotificationsPane.test.tsx index ce8431c3e..0cc38b18a 100644 --- a/src/renderer/src/components/settings/NotificationsPane.test.tsx +++ b/src/renderer/src/components/settings/NotificationsPane.test.tsx @@ -2,14 +2,16 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { GlobalSettings, NotificationDispatchRequest } from '../../../../shared/types' import { sendNotificationSettingsTestNotification } from './NotificationsPane' -const { toastError, toastSuccess } = vi.hoisted(() => ({ +const { toastError, toastMessage, toastSuccess } = vi.hoisted(() => ({ toastError: vi.fn(), + toastMessage: vi.fn(), toastSuccess: vi.fn() })) vi.mock('sonner', () => ({ toast: { error: toastError, + message: toastMessage, success: toastSuccess } })) @@ -30,6 +32,7 @@ function createSettings(): GlobalSettings { describe('NotificationsPane', () => { beforeEach(() => { toastError.mockClear() + toastMessage.mockClear() toastSuccess.mockClear() }) @@ -66,6 +69,46 @@ describe('NotificationsPane', () => { requireDisplayConfirmation: true }) expect(toastError).not.toHaveBeenCalled() + expect(toastSuccess).not.toHaveBeenCalled() + expect(toastMessage).toHaveBeenCalledWith( + 'Test notification requested', + expect.objectContaining({ + description: 'If no macOS banner appeared, enable Allow notifications for Orca.', + action: expect.objectContaining({ label: 'Open Settings' }) + }) + ) + + const toastOptions = toastMessage.mock.calls[0]?.[1] as + | { action?: { onClick?: () => void } } + | undefined + toastOptions?.action?.onClick?.() + expect(notifications.openSystemSettings).toHaveBeenCalledTimes(1) + }) + + it('confirms delivered test notifications on platforms where show means displayed', async () => { + const notifications = { + getPermissionStatus: vi.fn(async () => ({ + supported: true, + platform: 'win32' as NodeJS.Platform, + requested: true + })), + dispatch: vi.fn(async (_args: NotificationDispatchRequest) => ({ delivered: true })), + playSound: vi.fn(), + openSystemSettings: vi.fn(), + requestPermission: vi.fn() + } + vi.stubGlobal('window', { + Notification: { permission: 'granted' }, + api: { + notifications, + shell: { pickAudio: vi.fn() } + } + }) + + await sendNotificationSettingsTestNotification(createSettings().notifications, 50) + + expect(toastMessage).not.toHaveBeenCalled() + expect(toastError).not.toHaveBeenCalled() expect(toastSuccess).toHaveBeenCalledWith('Test notification sent') }) @@ -108,4 +151,74 @@ describe('NotificationsPane', () => { toastOptions?.action?.onClick?.() expect(notifications.openSystemSettings).toHaveBeenCalledTimes(1) }) + + it('uses Windows notification settings copy when the native test notification is not shown', async () => { + const notifications = { + getPermissionStatus: vi.fn(async () => ({ + supported: true, + platform: 'win32' as NodeJS.Platform, + requested: true + })), + dispatch: vi.fn(async (_args: NotificationDispatchRequest) => ({ + delivered: false, + reason: 'not-displayed' as const + })), + playSound: vi.fn(), + openSystemSettings: vi.fn(), + requestPermission: vi.fn() + } + vi.stubGlobal('window', { + Notification: { permission: 'granted' }, + api: { + notifications, + shell: { pickAudio: vi.fn() } + } + }) + + await sendNotificationSettingsTestNotification(createSettings().notifications, 50) + + expect(toastSuccess).not.toHaveBeenCalled() + expect(toastError).toHaveBeenCalledWith( + 'Windows did not show the notification', + expect.objectContaining({ + description: 'Enable notifications for Orca in Windows Settings.', + action: expect.objectContaining({ label: 'Open Settings' }) + }) + ) + }) + + it('does not show an inert settings action on platforms without a settings shortcut', async () => { + const notifications = { + getPermissionStatus: vi.fn(async () => ({ + supported: true, + platform: 'linux' as NodeJS.Platform, + requested: true + })), + dispatch: vi.fn(async (_args: NotificationDispatchRequest) => ({ + delivered: false, + reason: 'not-displayed' as const + })), + playSound: vi.fn(), + openSystemSettings: vi.fn(), + requestPermission: vi.fn() + } + vi.stubGlobal('window', { + Notification: { permission: 'granted' }, + api: { + notifications, + shell: { pickAudio: vi.fn() } + } + }) + + await sendNotificationSettingsTestNotification(createSettings().notifications, 50) + + expect(toastSuccess).not.toHaveBeenCalled() + expect(toastError).toHaveBeenCalledWith( + 'System did not show the notification', + expect.not.objectContaining({ + action: expect.anything() + }) + ) + expect(notifications.openSystemSettings).not.toHaveBeenCalled() + }) }) diff --git a/src/renderer/src/components/settings/NotificationsPane.tsx b/src/renderer/src/components/settings/NotificationsPane.tsx index 430793abe..f3163f52a 100644 --- a/src/renderer/src/components/settings/NotificationsPane.tsx +++ b/src/renderer/src/components/settings/NotificationsPane.tsx @@ -1,6 +1,6 @@ import { type ReactNode, useEffect, useRef, useState } from 'react' import { toast } from 'sonner' -import type { GlobalSettings } from '../../../../shared/types' +import type { GlobalSettings, NotificationPermissionStatusResult } from '../../../../shared/types' import { Button } from '../ui/button' import { Label } from '../ui/label' import { Separator } from '../ui/separator' @@ -53,6 +53,34 @@ type NotificationsPaneProps = { updateSettings: (updates: Partial) => void } +type SystemNotificationSettingsCopy = { + buttonLabel: string + failureTitle: string + failureDescription: string +} + +function getSystemNotificationSettingsCopy( + platform: NodeJS.Platform +): SystemNotificationSettingsCopy | null { + if (platform === 'darwin') { + return { + buttonLabel: 'macOS Settings', + failureTitle: 'macOS did not show the notification', + failureDescription: 'Enable Allow notifications for Orca in System Settings.' + } + } + + if (platform === 'win32') { + return { + buttonLabel: 'Windows Settings', + failureTitle: 'Windows did not show the notification', + failureDescription: 'Enable notifications for Orca in Windows Settings.' + } + } + + return null +} + export async function sendNotificationSettingsTestNotification( notificationSettings: GlobalSettings['notifications'], volumeDraft: number @@ -81,20 +109,42 @@ export async function sendNotificationSettingsTestNotification( toast.error('Custom notification sound could not be played') return } + const settingsCopy = getSystemNotificationSettingsCopy(permissionStatus.platform) + if (permissionStatus.platform === 'darwin' && settingsCopy) { + // Why: Electron's native 'show' event can fire even when macOS silently + // drops the banner because the per-app Allow notifications switch is off. + toast.message('Test notification requested', { + description: 'If no macOS banner appeared, enable Allow notifications for Orca.', + action: { + label: 'Open Settings', + onClick: () => { + void window.api.notifications.openSystemSettings() + } + } + }) + return + } toast.success('Test notification sent') return } if (result.reason === 'not-displayed') { - toast.error('macOS did not show the notification', { - description: 'Enable Allow notifications for Orca in System Settings.', - action: { - label: 'Open Settings', - onClick: () => { - void window.api.notifications.openSystemSettings() + const settingsCopy = getSystemNotificationSettingsCopy(permissionStatus.platform) + if (settingsCopy) { + toast.error(settingsCopy.failureTitle, { + description: settingsCopy.failureDescription, + action: { + label: 'Open Settings', + onClick: () => { + void window.api.notifications.openSystemSettings() + } } - } - }) + }) + } else { + toast.error('System did not show the notification', { + description: 'Check your desktop notification settings for Orca.' + }) + } return } @@ -112,6 +162,8 @@ export function NotificationsPane({ const notificationSettings = settings.notifications const notificationSettingsRef = useRef(notificationSettings) const [isPickingSound, setIsPickingSound] = useState(false) + const [permissionStatus, setPermissionStatus] = + useState(null) const updateNotificationSettings = (updates: Partial): void => { updateSettings({ @@ -131,6 +183,25 @@ export function NotificationsPane({ setVolumeDraft(notificationSettings.customSoundVolume) }, [notificationSettings]) + useEffect(() => { + let cancelled = false + void window.api.notifications + .getPermissionStatus() + .then((status) => { + if (!cancelled) { + setPermissionStatus(status) + } + }) + .catch(() => { + if (!cancelled) { + setPermissionStatus(null) + } + }) + return () => { + cancelled = true + } + }, []) + const handleVolumeCommit = (value: number): void => { if (notificationSettingsRef.current.customSoundVolume !== value) { updateNotificationSettings({ customSoundVolume: value }) @@ -158,6 +229,9 @@ export function NotificationsPane({ } const selectedSoundPath = notificationSettings.customSoundPath + const systemSettingsCopy = permissionStatus + ? getSystemNotificationSettingsCopy(permissionStatus.platform) + : null return (
@@ -298,15 +372,18 @@ export function NotificationsPane({ Send Test Notification - + {systemSettingsCopy ? ( + + ) : null}
) diff --git a/src/shared/types.ts b/src/shared/types.ts index e9e6179bb..dd553236d 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -1855,7 +1855,7 @@ export type NotificationEventSource = 'agent-task-complete' | 'terminal-bell' | export type NotificationDispatchRequest = { source: NotificationEventSource - /** Why: the Settings test button must not report success unless macOS actually shows it. */ + /** Why: useful for fast native failures, but macOS can still drop notifications after 'show'. */ requireDisplayConfirmation?: boolean worktreeId?: string /** Stable `${tabId}:${leafId}` terminal pane key for click-to-focus routing. */