From 6ae2d04b5a589d7b7af8ee7b4bf3365fbac8070d Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 8 Jul 2026 18:46:23 -0700 Subject: [PATCH] fix: drop confusing copy line; session-only permission evidence MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the 'Updates automatically.' line from both permission cards. Also drops the persisted notificationDeliveryConfirmed flag: OS-level permission changes between sessions, and a stale positive rendered a false green card. Delivery evidence is now session-scoped only. Documented detection ceiling (verified empirically on macOS 26): while the permission dialog is unanswered — and when notifications are toggled off in System Settings after being authorized — macOS accepts requests and silently swallows them, with no public API (Notification Center delivered-history and legacy ncprefs both included) able to distinguish that from real delivery. 'failed' remains definitive for unsigned builds and dialog-level denials. Co-authored-by: Orca --- src/main/ipc/notifications.test.ts | 41 +++++++++-------- src/main/ipc/notifications.ts | 74 +++++++++++------------------- src/shared/types.ts | 4 -- 3 files changed, 50 insertions(+), 69 deletions(-) diff --git a/src/main/ipc/notifications.test.ts b/src/main/ipc/notifications.test.ts index 321368e5e00..b5a690db46d 100644 --- a/src/main/ipc/notifications.test.ts +++ b/src/main/ipc/notifications.test.ts @@ -1345,7 +1345,7 @@ describe('notifications:probeDelivery', () => { await expect(result).resolves.toEqual({ state: 'blocked' }) }) - it('resolves delivered when the probe show event fires and persists confirmation', async () => { + it('resolves delivered when the probe show event fires', async () => { const store = createStore() registerNotificationHandlers(store as never) @@ -1354,19 +1354,33 @@ describe('notifications:probeDelivery', () => { getProbeOnceEventHandler('show')() await expect(result).resolves.toEqual({ state: 'delivered' }) - expect(store.updateUI).toHaveBeenCalledWith({ notificationDeliveryConfirmed: true }) + // No persisted confirmation on purpose: OS permission changes between runs. + expect(store.updateUI).not.toHaveBeenCalledWith({ notificationDeliveryConfirmed: true }) }) - it('short-circuits to delivered from the persisted confirmation without probing', () => { - const store = createStore({ notificationDeliveryConfirmed: true }) + it('serves session evidence without probing again until forced', async () => { + const store = createStore() registerNotificationHandlers(store as never) + const handler = getProbeDeliveryHandler() - expect(getProbeDeliveryHandler()({})).toEqual({ state: 'delivered' }) - expect(notificationCtorMock).not.toHaveBeenCalled() + const probeResult = handler({}) as Promise + getProbeOnceEventHandler('show')() + await expect(probeResult).resolves.toEqual({ state: 'delivered' }) + expect(notificationCtorMock).toHaveBeenCalledTimes(1) + + // Cached session evidence answers non-force calls with no new probe. + expect(handler({})).toEqual({ state: 'delivered' }) + expect(notificationCtorMock).toHaveBeenCalledTimes(1) + + // Force bypasses the cache and schedules a fresh probe. + const forced = handler({}, { force: true }) as Promise + expect(notificationCtorMock).toHaveBeenCalledTimes(2) + getProbeOnceEventHandler('show')() + await expect(forced).resolves.toEqual({ state: 'delivered' }) }) - it('prefers fresh session failure evidence over the persisted confirmation', async () => { - const store = createStore({ notificationDeliveryConfirmed: true }) + it('serves cached failure evidence after a rejected probe', async () => { + const store = createStore() registerNotificationHandlers(store as never) const handler = getProbeDeliveryHandler() @@ -1378,17 +1392,6 @@ describe('notifications:probeDelivery', () => { expect(notificationCtorMock).toHaveBeenCalledTimes(1) }) - it('force bypasses cached evidence and probes again', async () => { - const store = createStore({ notificationDeliveryConfirmed: true }) - registerNotificationHandlers(store as never) - const handler = getProbeDeliveryHandler() - - const probeResult = handler({}, { force: true }) as Promise - expect(notificationCtorMock).toHaveBeenCalledTimes(1) - getProbeOnceEventHandler('show')() - await expect(probeResult).resolves.toEqual({ state: 'delivered' }) - }) - it('resolves blocked on timeout without recording a definitive failure', async () => { const store = createStore() registerNotificationHandlers(store as never) diff --git a/src/main/ipc/notifications.ts b/src/main/ipc/notifications.ts index 71beb260486..63bbde50de7 100644 --- a/src/main/ipc/notifications.ts +++ b/src/main/ipc/notifications.ts @@ -101,29 +101,26 @@ const NOTIFICATION_PROBE_RESULT_TIMEOUT_MS = 3000 const NOTIFICATION_PROBE_BANNER_CLOSE_DELAY_MS = 4000 // Why: Electron has no API to read macOS UNUserNotificationCenter -// authorization, so the freshest signal we have is whether the last -// notification we scheduled was accepted ('show') or rejected ('failed'). -// Session-scoped on purpose: OS-level permission can change between runs. +// authorization, so the freshest signal we have is what happened to the last +// notification we scheduled. Session-scoped on purpose: OS-level permission +// can change between runs, and a stale positive renders a false green card. let lastObservedDeliveryOutcome: 'delivered' | 'failed' | null = null let deliveryProbeInFlight: Promise | null = null -function recordDeliveryOutcome(store: Store, outcome: 'delivered' | 'failed'): void { - lastObservedDeliveryOutcome = outcome - if (outcome === 'delivered' && store.getUI().notificationDeliveryConfirmed !== true) { - store.updateUI({ notificationDeliveryConfirmed: true }) - } -} - /** - * Schedules a silent probe notification and reports whether macOS accepted it. + * Schedules a silent probe notification and reports whether macOS accepted + * it. 'failed' means the request was rejected (permission denied, or an + * unsigned build). On a fresh install the probe also instantiates Electron's + * notification presenter, which is what makes macOS pop the "Allow + * notifications?" dialog. * - * Why: scheduling is the only permission signal Electron exposes — 'show' - * means the notification was accepted (permission granted), 'failed' means it - * was rejected (denied, or undeliverable e.g. unsigned build). On a fresh - * install the probe also instantiates Electron's notification presenter, - * which is what makes macOS pop the "Allow notifications?" dialog. + * Known ambiguity with no public API to resolve it (verified on macOS 26): + * while the dialog is unanswered — and when notifications are toggled off in + * System Settings after being authorized — macOS still accepts requests and + * silently swallows them, so 'delivered' can over-report. 'failed' fires for + * hard rejections (unsigned builds, dialog-level denial). */ -function probeNotificationDelivery(store: Store): Promise { +function probeNotificationDelivery(): Promise { if (deliveryProbeInFlight) { return deliveryProbeInFlight } @@ -155,15 +152,15 @@ function probeNotificationDelivery(store: Store): Promise void) | null = null let failedHandler: ((_event: unknown, error?: string) => void) | null = null - let shownHandler: (() => void) | null = null const entryForId: { notification: Notification; release: () => void } | null = args.notificationId ? { notification, release: () => {} } : null const release = retainNotificationUntilRelease(notification, () => { @@ -486,10 +475,6 @@ export function registerNotificationHandlers(store: Store, runtime?: OrcaRuntime notification.removeListener('failed', failedHandler) failedHandler = null } - if (shownHandler) { - notification.removeListener('show', shownHandler) - shownHandler = null - } if ( args.notificationId && activeNotificationsById.get(args.notificationId) === entryForId @@ -507,14 +492,11 @@ export function registerNotificationHandlers(store: Store, runtime?: OrcaRuntime // apps and native delivery errors here; release immediately instead // of retaining a dead notification until the fallback timer. logNativeNotificationFailure(args.source, error) - recordDeliveryOutcome(store, 'failed') + // A definitive rejection — feeds the permission card's evidence. + lastObservedDeliveryOutcome = 'failed' release() } notification.on('failed', failedHandler) - // Why: keep the passive permission signal fresh — every accepted - // notification proves delivery still works this session. - shownHandler = () => recordDeliveryOutcome(store, 'delivered') - notification.on('show', shownHandler) // Why: clicking a notification should bring Orca to the foreground and // switch to the worktree/pane that triggered it. Worktree activation owns @@ -569,6 +551,7 @@ export function registerNotificationHandlers(store: Store, runtime?: OrcaRuntime release() return { delivered: false, reason: 'not-displayed' } } + lastObservedDeliveryOutcome = 'delivered' return { delivered: true } }) } @@ -698,7 +681,6 @@ export function triggerStartupNotificationRegistration(store: Store): void { } function onShow(): void { - recordDeliveryOutcome(store, 'delivered') // Why: close after a short delay so the notification doesn't linger in // Notification Center. The macOS permission dialog is a system-level sheet // that appears independently and is not dismissed by closing this notification. @@ -712,7 +694,7 @@ export function triggerStartupNotificationRegistration(store: Store): void { // Why: Electron 42 requires code-signed macOS apps for UNNotification // delivery. Unsigned builds fail here instead of producing the permission UI. logNativeNotificationFailure('startup registration', error) - recordDeliveryOutcome(store, 'failed') + lastObservedDeliveryOutcome = 'failed' cleanup() } diff --git a/src/shared/types.ts b/src/shared/types.ts index 630cd363c81..47c62959259 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -3271,10 +3271,6 @@ export type PersistedUIState = { * permission dialog via a startup notification. Prevents re-firing on * every launch. */ notificationPermissionRequested?: boolean - /** Set once a notification is confirmed delivered by the OS, so later - * onboarding/setup visits can show "enabled" without re-probing (a probe - * briefly flashes a banner when permission is granted). */ - notificationDeliveryConfirmed?: boolean /** Once the user has seen the "your sessions won't be interrupted" * reassurance card, we never show it again. */ updateReassuranceSeen?: boolean