diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index d2eca5a78..255527009 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -69,6 +69,8 @@ Rules: App-level side effects are owned by a single host. `GlobalEffects` mounts exactly once (inside the query and theme providers) and owns theme application, tray updates, global shortcut registration, periodic account refreshes, and notification polling and alerts. +Operating-system notifications are created by the main process behind the typed `window.gitify.nativeNotifications` bridge. The single notification host publishes the filtered new-notification set, marks a notification read when its OS action is used, opens the target when its body is clicked, and removes published notifications when they leave the unfiltered cache. The sandboxed preload only forwards requests and listeners; it never constructs an OS notification. + Consumers read the shared query cache and stores, and **do not register their own** polling, alerts, wake, or focus listeners. This keeps background work (for example, firing one poll per interval) from multiplying with the number of mounted components. When you add a new background effect, add it to the host, not to a leaf component. Subscription is separate from side effects, but it multiplies the same way: every TanStack Query observer re-runs the query's `select` and structural-sharing walk whenever the cached data changes. A component that renders once per notification โ€” the notification row, the repository header โ€” must therefore not subscribe. It obtains notification actions (mark as read/done, unsubscribe, remove an account, refetch) with `useNotificationActions`, which never calls `useQuery`. Components that read notification data use `useNotifications` and stay subscribed by design. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 038ef9a71..1a540f41d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -93,6 +93,16 @@ guard fails the run outside Linux rather than letting that happen. Note that these baselines capture the `backdrop-filter` fallback for Glass, not the macOS native vibrancy material, which Chromium cannot render. +#### ๐Ÿ”” Native notification checks + +Operating-system notifications are created in the main process. Each unread notification offers a single **Mark as read** action, and clicking the body opens the notification target. + +- **macOS**: native notifications require a code-signed app. Electron ships ad-hoc _linker_-signed, which macOS rejects for `UNUserNotificationCenter`, so `pnpm dev` re-signs the development Electron bundle automatically before starting. Ad-hoc signing is the default and needs no certificate; set `GITIFY_DEV_SIGN_IDENTITY` to use a Developer ID or Apple Development identity. Run `pnpm dev` again after `pnpm install` or an Electron upgrade. The same step sets `NSUserNotificationAlertStyle: alert` for development, and the packaged app declares it in its `Info.plist`. +- **Windows**: toast interactions arrive through the notification instance events; verify Action Center persistence and cold-start activation manually. +- **Linux**: the action button appears only when the desktop notification server advertises the `actions` capability. Without it, the notification still shows without a button. + +Verify the notification content, body click, Mark as read, and removal-on-read on macOS, Windows, and an action-capable Linux desktop before releasing a change in this area. + ### Translations Gitify's interface is translated through [i18next][i18next-website]. The app ships English as the only locale today, but the infrastructure supports additional languages โ€” adding one is a contributor-friendly change. diff --git a/electron-builder.js b/electron-builder.js index 9fef15d00..23d654f7f 100644 --- a/electron-builder.js +++ b/electron-builder.js @@ -20,6 +20,8 @@ const config = { icon: 'assets/images/app-icon.icns', extendInfo: { LSUIElement: true, + // Required for extra action buttons on macOS notifications. + NSUserNotificationAlertStyle: 'alert', }, identity: 'Adam Setch (5KD23H9729)', type: 'distribution', diff --git a/package.json b/package.json index 5292bb695..853d4b4c2 100644 --- a/package.json +++ b/package.json @@ -56,6 +56,7 @@ "build": "vp build", "codegen": "graphql-codegen --config codegen.ts", "codegen:watch": "pnpm codegen --watch", + "predev": "node scripts/sign-dev-electron.mjs", "dev": "concurrently --names \"vite,codegen\" --prefix-colors \"blue,magenta\" \"vp dev\" \"pnpm codegen:watch\"", "devtools": "concurrently --names \"dev,devtools\" --prefix-colors \"cyan,green\" \"pnpm dev\" \"react-devtools\"", "start": "pnpm build && pnpm dev", diff --git a/scripts/sign-dev-electron.mjs b/scripts/sign-dev-electron.mjs new file mode 100644 index 000000000..460080d58 --- /dev/null +++ b/scripts/sign-dev-electron.mjs @@ -0,0 +1,131 @@ +#!/usr/bin/env node +/** + * Prepare the development Electron bundle for native notification testing. + * + * Electron ships ad-hoc *linker*-signed, which macOS rejects for + * `UNUserNotificationCenter` with `UNErrorDomain error 1`. A real `codesign` + * pass fixes that, and an ad-hoc signature (`-`) is enough for local testing โ€” + * contributors do not need an Apple Developer certificate. + * + * Runs automatically from `predev` on macOS. Set `GITIFY_DEV_SIGN_IDENTITY` to + * use a Developer ID / Apple Development identity instead of ad-hoc. + */ +import { execFileSync, spawnSync } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import { createRequire } from 'node:module'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const scriptDir = path.dirname(fileURLToPath(import.meta.url)); +const repoRoot = path.resolve(scriptDir, '..'); + +if (process.platform !== 'darwin') { + process.exit(0); +} + +const require = createRequire(import.meta.url); +const electronBinary = require('electron'); +const electronApp = electronBinary?.replace(/\/Contents\/MacOS\/Electron$/, ''); + +if (!electronApp || !existsSync(electronApp)) { + console.warn('[sign-dev] could not locate the development Electron bundle; skipping'); + process.exit(0); +} + +const plist = path.join(electronApp, 'Contents', 'Info.plist'); +const entitlements = path.join(repoRoot, 'assets', 'entitlements.mac.plist'); + +const requestedIdentity = process.env.GITIFY_DEV_SIGN_IDENTITY?.trim(); +const identity = requestedIdentity || findIdentity() || '-'; +const adHoc = identity === '-'; + +if (isPrepared(electronApp, plist, identity)) { + console.log('[sign-dev] development Electron is already signed for notifications'); + process.exit(0); +} + +setAlertStyle(plist); + +console.log( + adHoc + ? '[sign-dev] signing development Electron ad-hoc for native notifications' + : `[sign-dev] signing development Electron with ${identity}`, +); + +execFileSync( + 'codesign', + [ + '--force', + '--deep', + '--options', + 'runtime', + '--entitlements', + entitlements, + '--sign', + identity, + electronApp, + ], + { stdio: 'inherit' }, +); + +/** Prefer a Developer ID, then an Apple Development identity, when present. */ +function findIdentity() { + try { + const identities = execFileSync('security', ['find-identity', '-v', '-p', 'codesigning'], { + encoding: 'utf8', + }); + + return ( + identities.match(/"([^"]*Developer ID Application:[^"]*)"/)?.[1] ?? + identities.match(/"([^"]*Apple Development:[^"]*)"/)?.[1] ?? + null + ); + } catch { + return null; + } +} + +/** Skip re-signing when the bundle already has what notifications need. */ +function isPrepared(app, plistPath, identity) { + try { + const verify = spawnSync('codesign', ['--verify', '--deep', app], { encoding: 'utf8' }); + + if (verify.status !== 0) { + return false; + } + + const signature = spawnSync('codesign', ['-dv', '--verbose=2', app], { encoding: 'utf8' }); + const flags = `${signature.stdout}${signature.stderr}`; + const alertStyle = execFileSync( + '/usr/libexec/PlistBuddy', + ['-c', 'Print :NSUserNotificationAlertStyle', plistPath], + { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'] }, + ); + + if (alertStyle.trim() !== 'alert') { + return false; + } + + return identity === '-' + ? !flags.includes('linker-signed') + : flags.includes(`Authority=${identity}`); + } catch { + return false; + } +} + +function setAlertStyle(plistPath) { + try { + execFileSync('/usr/libexec/PlistBuddy', [ + '-c', + 'Set :NSUserNotificationAlertStyle alert', + plistPath, + ]); + } catch { + execFileSync('/usr/libexec/PlistBuddy', [ + '-c', + 'Add :NSUserNotificationAlertStyle string alert', + plistPath, + ]); + } +} diff --git a/src/__helpers__/gitify-bridge.ts b/src/__helpers__/gitify-bridge.ts index 3ece4b950..9a1940abb 100644 --- a/src/__helpers__/gitify-bridge.ts +++ b/src/__helpers__/gitify-bridge.ts @@ -47,6 +47,13 @@ export function createGitifyBridgeApi(platform: TestPlatform = 'macos'): Window[ setShowUpdateNotifications: vi.fn(), setUseX11Backend: vi.fn(), applyKeyboardShortcut: vi.fn().mockResolvedValue({ success: true }), - raiseNativeNotification: vi.fn(), + nativeNotifications: { + show: vi.fn(), + remove: vi.fn(), + removeAll: vi.fn(), + notifyReady: vi.fn(), + onAction: vi.fn(() => vi.fn()), + onClick: vi.fn(() => vi.fn()), + }, }; } diff --git a/src/main/handlers/index.ts b/src/main/handlers/index.ts index 7a2353259..ea0632c50 100644 --- a/src/main/handlers/index.ts +++ b/src/main/handlers/index.ts @@ -1,5 +1,6 @@ export * from './app'; export * from './github-cli'; +export * from './native-notifications'; export * from './storage'; export * from './system'; export * from './tray'; diff --git a/src/main/handlers/native-notifications.test.ts b/src/main/handlers/native-notifications.test.ts new file mode 100644 index 000000000..2f7437c4c --- /dev/null +++ b/src/main/handlers/native-notifications.test.ts @@ -0,0 +1,105 @@ +import type { Menubar } from 'electron-menubar'; + +import { EVENTS } from '../../shared/events'; + +const mocks = vi.hoisted(() => ({ + service: { + show: vi.fn(), + remove: vi.fn(), + removeAll: vi.fn(), + markRendererReady: vi.fn(), + destroy: vi.fn(), + }, + onMainEvent: vi.fn(), + appOnce: vi.fn(), +})); + +vi.mock('../notifications', () => ({ + NativeNotificationService: vi.fn(function () { + return mocks.service; + }), +})); + +vi.mock('../events', () => ({ + onMainEvent: (...args: unknown[]) => mocks.onMainEvent(...args), +})); + +vi.mock('electron', () => ({ + app: { + once: (...args: unknown[]) => mocks.appOnce(...args), + }, +})); + +import { + __resetNativeNotificationServiceForTests, + registerNativeNotificationHandlers, +} from './native-notifications'; + +describe('main/handlers/native-notifications.ts', () => { + const mb = {} as Menubar; + + beforeEach(() => { + __resetNativeNotificationServiceForTests(); + mocks.onMainEvent.mockClear(); + mocks.appOnce.mockClear(); + mocks.service.destroy.mockClear(); + }); + + it('registers the notification commands and interaction listener', () => { + registerNativeNotificationHandlers(mb); + + expect(mocks.onMainEvent).toHaveBeenCalledWith( + EVENTS.NATIVE_NOTIFICATION_SHOW, + expect.any(Function), + ); + expect(mocks.onMainEvent).toHaveBeenCalledWith( + EVENTS.NATIVE_NOTIFICATION_REMOVE, + expect.any(Function), + ); + expect(mocks.onMainEvent).toHaveBeenCalledWith( + EVENTS.NATIVE_NOTIFICATION_REMOVE_ALL, + expect.any(Function), + ); + expect(mocks.onMainEvent).toHaveBeenCalledWith( + EVENTS.NATIVE_NOTIFICATION_READY, + expect.any(Function), + ); + expect(mocks.appOnce).toHaveBeenCalledWith('will-quit', expect.any(Function)); + }); + + it('forwards commands to the service', () => { + registerNativeNotificationHandlers(mb); + + const handlers = Object.fromEntries( + mocks.onMainEvent.mock.calls.map(([event, handler]) => [event, handler]), + ) as Record void>; + + const payload = { id: 'n1a2b3c4d5e6', title: 'Title', body: 'Body' }; + handlers[EVENTS.NATIVE_NOTIFICATION_SHOW]({}, payload); + handlers[EVENTS.NATIVE_NOTIFICATION_REMOVE]({}, { ids: [payload.id] }); + handlers[EVENTS.NATIVE_NOTIFICATION_REMOVE_ALL]({}, undefined); + handlers[EVENTS.NATIVE_NOTIFICATION_READY]({}, undefined); + + expect(mocks.service.show).toHaveBeenCalledWith(payload); + expect(mocks.service.remove).toHaveBeenCalledWith({ ids: [payload.id] }); + expect(mocks.service.removeAll).toHaveBeenCalledTimes(1); + expect(mocks.service.markRendererReady).toHaveBeenCalledTimes(1); + }); + + it('is idempotent', () => { + const first = registerNativeNotificationHandlers(mb); + const second = registerNativeNotificationHandlers(mb); + + expect(first).toBe(second); + expect(mocks.onMainEvent).toHaveBeenCalledTimes(4); + }); + + it('destroys the service on will-quit', () => { + registerNativeNotificationHandlers(mb); + const willQuit = mocks.appOnce.mock.calls[0][1] as () => void; + + willQuit(); + + expect(mocks.service.destroy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/main/handlers/native-notifications.ts b/src/main/handlers/native-notifications.ts new file mode 100644 index 000000000..830a1f295 --- /dev/null +++ b/src/main/handlers/native-notifications.ts @@ -0,0 +1,41 @@ +import { app } from 'electron'; +import type { Menubar } from 'electron-menubar'; + +import { EVENTS } from '../../shared/events'; + +import { onMainEvent } from '../events'; +import { NativeNotificationService } from '../notifications'; + +let service: NativeNotificationService | null = null; + +/** + * Register the operating-system notification service and its typed IPC + * handlers. Idempotent: a second call reuses the existing service rather than + * registering duplicate handlers. + */ +export function registerNativeNotificationHandlers(mb: Menubar): NativeNotificationService { + if (service) { + return service; + } + + const instance = new NativeNotificationService(mb); + service = instance; + + onMainEvent(EVENTS.NATIVE_NOTIFICATION_SHOW, (_, payload) => instance.show(payload)); + onMainEvent(EVENTS.NATIVE_NOTIFICATION_REMOVE, (_, payload) => instance.remove(payload)); + onMainEvent(EVENTS.NATIVE_NOTIFICATION_REMOVE_ALL, () => instance.removeAll()); + onMainEvent(EVENTS.NATIVE_NOTIFICATION_READY, () => instance.markRendererReady()); + + app.once('will-quit', () => { + instance.destroy(); + service = null; + }); + + return instance; +} + +/** @internal Reset the module-level service between tests. */ +export function __resetNativeNotificationServiceForTests(): void { + service?.destroy(); + service = null; +} diff --git a/src/main/index.test.ts b/src/main/index.test.ts index 0b35b539b..895fe0aa5 100644 --- a/src/main/index.test.ts +++ b/src/main/index.test.ts @@ -37,6 +37,7 @@ vi.mock('./config', () => ({ vi.mock('./handlers', () => ({ registerAppHandlers: vi.fn(), registerGitHubCliHandlers: vi.fn(), + registerNativeNotificationHandlers: vi.fn(() => calls.push('native-notifications')), registerStorageHandlers: vi.fn(), registerSystemHandlers: vi.fn(), registerTrayHandlers: vi.fn(), @@ -87,7 +88,11 @@ describe('main/index.ts', () => { }); it('configures user data and the Ozone backend before Electron becomes ready', () => { - expect(calls).toEqual(['userData', 'ozone', 'whenReady']); + expect(calls.slice(0, 3)).toEqual(['userData', 'ozone', 'whenReady']); + }); + + it('registers native notification handlers during startup', async () => { + await vi.waitFor(() => expect(calls).toContain('native-notifications')); }); it('logs an error when Electron fails to become ready', async () => { diff --git a/src/main/index.ts b/src/main/index.ts index 7a7c0f76d..19ee24d31 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -8,6 +8,7 @@ import { Paths, WindowConfig } from './config'; import { registerAppHandlers, registerGitHubCliHandlers, + registerNativeNotificationHandlers, registerStorageHandlers, registerSystemHandlers, registerTrayHandlers, @@ -67,6 +68,7 @@ app // Register IPC handlers for various channels registerTrayHandlers(mb); registerSystemHandlers(mb); + registerNativeNotificationHandlers(mb); registerStorageHandlers(); registerGitHubCliHandlers(); registerAppHandlers(mb); diff --git a/src/main/notifications.test.ts b/src/main/notifications.test.ts new file mode 100644 index 000000000..7afb8d8e2 --- /dev/null +++ b/src/main/notifications.test.ts @@ -0,0 +1,248 @@ +import type { Menubar } from 'electron-menubar'; + +import { EVENTS, type INativeNotificationPayload } from '../shared/events'; + +const sendRendererEventMock = vi.fn(); + +vi.mock('./events', () => ({ + sendRendererEvent: (...args: unknown[]) => sendRendererEventMock(...args), +})); + +vi.mock('../shared/logger', () => ({ + logError: vi.fn(), +})); + +vi.mock('electron', () => { + class MockNotification { + static instances: MockNotification[] = []; + static isSupported = vi.fn(() => true); + static remove = vi.fn(); + static removeAll = vi.fn(); + + id: string; + options: unknown; + show = vi.fn(); + close = vi.fn(); + private listeners = new Map void>(); + + constructor(options: { id: string }) { + this.id = options.id; + this.options = options; + MockNotification.instances.push(this); + } + + on(event: string, listener: (...args: unknown[]) => void) { + this.listeners.set(event, listener); + return this; + } + + emit(event: string, ...args: unknown[]) { + this.listeners.get(event)?.(...args); + } + } + + return { Notification: MockNotification }; +}); + +import { Notification } from 'electron'; + +import { NativeNotificationService, validateNativeNotificationPayload } from './notifications'; + +interface NotificationMock { + id: string; + options: Record; + show: ReturnType; + close: ReturnType; + emit: (event: string, ...args: unknown[]) => void; +} + +const NotificationMock = Notification as unknown as { + instances: NotificationMock[]; + isSupported: ReturnType; + remove: ReturnType; + removeAll: ReturnType; +}; + +function makeMenubar() { + const send = vi.fn(); + const webContentsListeners = new Map void>(); + const mb = { + window: { + webContents: { + send, + on: (event: string, listener: () => void) => webContentsListeners.set(event, listener), + }, + isDestroyed: () => false, + }, + } as unknown as Menubar; + + return { mb, emitWebContents: (event: string) => webContentsListeners.get(event)?.() }; +} + +const basePayload: INativeNotificationPayload = { + id: 'n1a2b3c4d5e6', + title: 'owner/repo', + subtitle: 'Updated ยท @octocat', + body: 'A title', + actionLabel: 'Mark as read', +}; + +describe('main/notifications.ts', () => { + beforeEach(() => { + NotificationMock.instances = []; + NotificationMock.isSupported.mockReturnValue(true); + NotificationMock.remove.mockClear(); + NotificationMock.removeAll.mockClear(); + sendRendererEventMock.mockClear(); + }); + + it('does not construct a notification when the platform does not support them', () => { + NotificationMock.isSupported.mockReturnValue(false); + const { mb } = makeMenubar(); + + new NativeNotificationService(mb, 'darwin').show(basePayload); + + expect(NotificationMock.instances).toHaveLength(0); + }); + + it('constructs a macOS notification with a subtitle and one action', () => { + const { mb } = makeMenubar(); + + new NativeNotificationService(mb, 'darwin').show(basePayload); + + expect(NotificationMock.instances[0].options).toMatchObject({ + id: basePayload.id, + title: basePayload.title, + subtitle: basePayload.subtitle, + body: basePayload.body, + silent: true, + actions: [{ type: 'button', text: 'Mark as read' }], + }); + }); + + it('folds the subtitle into the body on Windows and Linux', () => { + const { mb } = makeMenubar(); + + new NativeNotificationService(mb, 'linux').show(basePayload); + + expect(NotificationMock.instances[0].options).toMatchObject({ + subtitle: undefined, + body: `${basePayload.subtitle}\n${basePayload.body}`, + }); + }); + + it('publishes a notification without actions when no label is provided', () => { + const { mb } = makeMenubar(); + + new NativeNotificationService(mb, 'darwin').show({ ...basePayload, actionLabel: undefined }); + + expect(NotificationMock.instances[0].options).toMatchObject({ actions: [] }); + }); + + it('delivers clicks and the Mark as read action after the renderer is ready', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'linux'); + service.show(basePayload); + service.markRendererReady(); + + const instance = NotificationMock.instances[0]; + instance.emit('click'); + instance.emit('action', { actionIndex: 0 }); + + expect(sendRendererEventMock).toHaveBeenCalledWith(mb, EVENTS.NATIVE_NOTIFICATION_CLICK, { + id: basePayload.id, + }); + expect(sendRendererEventMock).toHaveBeenCalledWith(mb, EVENTS.NATIVE_NOTIFICATION_ACTION, { + id: basePayload.id, + }); + }); + + it('ignores unknown action indexes', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'linux'); + service.show(basePayload); + service.markRendererReady(); + + NotificationMock.instances[0].emit('action', { actionIndex: 1 }); + + expect(sendRendererEventMock).not.toHaveBeenCalled(); + }); + + it('queues interactions until the renderer is ready', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'linux'); + service.show(basePayload); + + NotificationMock.instances[0].emit('click'); + expect(sendRendererEventMock).not.toHaveBeenCalled(); + + service.markRendererReady(); + expect(sendRendererEventMock).toHaveBeenCalledWith(mb, EVENTS.NATIVE_NOTIFICATION_CLICK, { + id: basePayload.id, + }); + }); + + it('queues interactions again while the renderer reloads', () => { + const { mb, emitWebContents } = makeMenubar(); + const service = new NativeNotificationService(mb, 'linux'); + service.show(basePayload); + service.markRendererReady(); + + emitWebContents('did-start-loading'); + NotificationMock.instances[0].emit('click'); + expect(sendRendererEventMock).not.toHaveBeenCalled(); + + service.markRendererReady(); + expect(sendRendererEventMock).toHaveBeenCalledTimes(1); + }); + + it('replaces an existing notification with the same identity', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'linux'); + + service.show(basePayload); + service.show({ ...basePayload, body: 'Updated body' }); + + expect(NotificationMock.instances[0].close).toHaveBeenCalledTimes(1); + expect(NotificationMock.instances).toHaveLength(2); + }); + + it('removes notifications by identity', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'darwin'); + service.show(basePayload); + + service.remove({ ids: [basePayload.id] }); + + expect(NotificationMock.instances[0].close).toHaveBeenCalledTimes(1); + expect(NotificationMock.remove).toHaveBeenCalledWith(basePayload.id); + }); + + it('removes every published notification', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'darwin'); + service.show(basePayload); + service.show({ ...basePayload, id: 'n2b3c4d5e6f7' }); + + service.removeAll(); + + expect( + NotificationMock.instances.every((instance) => instance.close.mock.calls.length === 1), + ).toBe(true); + expect(NotificationMock.removeAll).toHaveBeenCalled(); + }); + + it('validates identifiers and action labels', () => { + expect(validateNativeNotificationPayload(basePayload)).toBe(true); + expect(validateNativeNotificationPayload({ ...basePayload, id: '' })).toBe(false); + expect(validateNativeNotificationPayload({ ...basePayload, id: 'n'.repeat(65) })).toBe(false); + expect(validateNativeNotificationPayload({ ...basePayload, title: ' ' })).toBe(false); + expect(validateNativeNotificationPayload({ ...basePayload, actionLabel: ' ' })).toBe(false); + expect(validateNativeNotificationPayload({ ...basePayload, actionLabel: 'x'.repeat(65) })).toBe( + false, + ); + expect( + validateNativeNotificationPayload({ ...basePayload, actionLabel: 1 as unknown as string }), + ).toBe(false); + }); +}); diff --git a/src/main/notifications.ts b/src/main/notifications.ts new file mode 100644 index 000000000..4b9d0f6bd --- /dev/null +++ b/src/main/notifications.ts @@ -0,0 +1,229 @@ +import { Notification } from 'electron'; +import type { Menubar } from 'electron-menubar'; + +import { + EVENTS, + type INativeNotificationActionEvent, + type INativeNotificationClickEvent, + type INativeNotificationPayload, + type INativeNotificationRemovePayload, +} from '../shared/events'; +import { logError } from '../shared/logger'; + +import { sendRendererEvent } from './events'; + +// Windows caps a toast tag at 64 characters. +const MAX_IDENTIFIER_LENGTH = 64; +const MAX_ACTION_LABEL_LENGTH = 64; +const MAX_QUEUED_INTERACTIONS = 20; + +type QueuedInteraction = + | { event: 'action'; payload: INativeNotificationActionEvent } + | { event: 'click'; payload: INativeNotificationClickEvent }; + +/** + * Owns operating-system notifications in the main process. + * + * The sandboxed preload cannot reach Electron's `Notification` class, so every + * publish, removal, click, and action is routed through this service. + */ +export class NativeNotificationService { + private readonly live = new Map(); + private readonly queue: QueuedInteraction[] = []; + private readonly watchedWindows = new WeakSet(); + private rendererReady = false; + + constructor( + private readonly mb: Menubar, + private readonly platform: NodeJS.Platform = process.platform, + ) {} + + show(payload: INativeNotificationPayload): void { + if (!Notification.isSupported() || !validateNativeNotificationPayload(payload)) { + return; + } + + this.closeOne(payload.id); + + const subtitle = this.platform === 'darwin' ? payload.subtitle : undefined; + const body = + this.platform !== 'darwin' && payload.subtitle + ? `${payload.subtitle}\n${payload.body}` + : payload.body; + + const notification = new Notification({ + id: payload.id, + title: payload.title, + subtitle, + body, + silent: true, + actions: payload.actionLabel ? [{ type: 'button', text: payload.actionLabel }] : [], + }); + + notification.on('click', () => { + this.deliver('click', { id: payload.id }); + }); + + // Notifications publish at most one action: Mark as read. + notification.on('action', (details) => { + if (payload.actionLabel && details.actionIndex === 0) { + this.deliver('action', { id: payload.id }); + } + }); + + notification.on('failed', (_event, error) => { + logError('native-notifications', 'Failed to show a native notification', new Error(error), [ + payload.id, + ]); + }); + + this.live.set(payload.id, notification); + notification.show(); + } + + remove(payload: INativeNotificationRemovePayload): void { + for (const id of payload.ids) { + this.closeOne(id); + } + } + + removeAll(): void { + for (const notification of this.live.values()) { + notification.close(); + } + + this.live.clear(); + this.queue.length = 0; + + if (this.platform === 'darwin' && typeof Notification.removeAll === 'function') { + Notification.removeAll(); + } + } + + markRendererReady(): void { + this.rendererReady = true; + this.canSendToRenderer(); + this.flushQueue(); + } + + /** Release references without dismissing delivered notifications on quit. */ + destroy(): void { + this.live.clear(); + this.queue.length = 0; + } + + private closeOne(id: string): void { + const notification = this.live.get(id); + + if (notification) { + notification.close(); + this.live.delete(id); + } + + if (this.platform === 'darwin' && typeof Notification.remove === 'function') { + Notification.remove(id); + } + } + + private deliver( + event: QueuedInteraction['event'], + payload: INativeNotificationActionEvent | INativeNotificationClickEvent, + ): void { + if (this.canSendToRenderer()) { + this.send(event, payload); + return; + } + + this.queue.push({ event, payload } as QueuedInteraction); + + if (this.queue.length > MAX_QUEUED_INTERACTIONS) { + this.queue.shift(); + } + } + + private canSendToRenderer(): boolean { + const window = this.mb.window; + + if (window) { + this.watchRendererReloads(window); + } + + if (!this.rendererReady || !window) { + return false; + } + + return typeof window.isDestroyed !== 'function' || !window.isDestroyed(); + } + + private send( + event: QueuedInteraction['event'], + payload: INativeNotificationActionEvent | INativeNotificationClickEvent, + ): void { + sendRendererEvent( + this.mb, + event === 'action' ? EVENTS.NATIVE_NOTIFICATION_ACTION : EVENTS.NATIVE_NOTIFICATION_CLICK, + payload, + ); + } + + /** A reloading renderer drops its listeners, so queue until it reports ready. */ + private watchRendererReloads(window: Electron.BrowserWindow): void { + if (this.watchedWindows.has(window)) { + return; + } + + this.watchedWindows.add(window); + window.webContents?.on('did-start-loading', () => { + this.rendererReady = false; + }); + } + + private flushQueue(): void { + while (this.queue.length > 0 && this.canSendToRenderer()) { + const interaction = this.queue.shift()!; + this.send(interaction.event, interaction.payload); + } + } +} + +/** Validate every field that crosses the renderer/main boundary. */ +export function validateNativeNotificationPayload(payload: INativeNotificationPayload): boolean { + if (!payload || typeof payload !== 'object') { + return false; + } + + if (!isValidIdentifier(payload.id)) { + return false; + } + + if (typeof payload.title !== 'string' || payload.title.trim().length === 0) { + return false; + } + + if (typeof payload.body !== 'string') { + return false; + } + + if (payload.subtitle !== undefined && typeof payload.subtitle !== 'string') { + return false; + } + + return ( + payload.actionLabel === undefined || + (typeof payload.actionLabel === 'string' && + payload.actionLabel.trim().length > 0 && + payload.actionLabel.length <= MAX_ACTION_LABEL_LENGTH) + ); +} + +function isValidIdentifier(value: unknown): value is string { + return ( + typeof value === 'string' && + value.length > 0 && + value.length <= MAX_IDENTIFIER_LENGTH && + !Array.from(value).some((character) => { + const code = character.codePointAt(0) ?? 0; + return code < 32 || code === 127; + }) + ); +} diff --git a/src/preload/index.test.ts b/src/preload/index.test.ts index de11e2aff..a50249fe2 100644 --- a/src/preload/index.test.ts +++ b/src/preload/index.test.ts @@ -42,37 +42,25 @@ vi.mock('electron', () => ({ } satisfies Pick, })); -// Simple Notification stub -class MockNotification { - static instances: MockNotification[] = []; - public onclick: (() => void) | null = null; - constructor( - public title: string, - public options: { body: string; silent: boolean }, - ) { - MockNotification.instances.push(this); - } - triggerClick() { - this.onclick?.(); - } -} - -// Attach to global before importing preload -(global as unknown as { Notification: unknown }).Notification = MockNotification; - interface TestApi { tray: { updateColor: (n?: number, isOnline?: boolean) => void }; openExternalLink: (u: string, f: boolean) => void; setShowUpdateNotifications: (value: boolean) => void; setUseX11Backend: (value: boolean) => void; app: { version: () => Promise; show?: () => void; hide?: () => void }; - raiseNativeNotification: (t: string, b: string, u?: string) => unknown; + nativeNotifications: { + show: (payload: unknown) => void; + remove: (payload: unknown) => void; + removeAll: () => void; + notifyReady: () => void; + onAction: (callback: (payload: unknown) => void) => () => void; + onClick: (callback: (payload: unknown) => void) => () => void; + }; onZoom: (callback: (action: 'in' | 'out' | 'reset') => void) => () => void; } describe('preload/index', () => { beforeEach(() => { - MockNotification.instances = []; exposeInMainWorldMock('gitify', api); }); @@ -162,31 +150,89 @@ describe('preload/index', () => { expect(callback).toHaveBeenCalledWith('out'); }); - it('raiseNativeNotification without url calls app.show', async () => { + it('nativeNotifications.show sends the payload to the main process', () => { const api = getExposedApi(); + const payload = { + id: 'account:notification', + title: 'Title', + body: 'Body', + actionLabel: 'Mark as read', + }; - const notification = api.raiseNativeNotification('Title', 'Body') as MockNotification; + api.nativeNotifications.show(payload); - notification.triggerClick(); + expect(sendMainEventMock).toHaveBeenCalledWith(EVENTS.NATIVE_NOTIFICATION_SHOW, payload); + }); - expect(sendMainEventMock).toHaveBeenCalledWith(EVENTS.WINDOW_SHOW); + it('nativeNotifications.remove sends the removal payload', () => { + const api = getExposedApi(); + + api.nativeNotifications.remove({ ids: ['one', 'two'] }); + + expect(sendMainEventMock).toHaveBeenCalledWith(EVENTS.NATIVE_NOTIFICATION_REMOVE, { + ids: ['one', 'two'], + }); }); - it('raiseNativeNotification with url hides app then opens link', async () => { + it('nativeNotifications.removeAll sends the clear event', () => { const api = getExposedApi(); - const notification = api.raiseNativeNotification( - 'Title', - 'Body', - 'https://x', - ) as MockNotification; + api.nativeNotifications.removeAll(); - notification.triggerClick(); + expect(sendMainEventMock).toHaveBeenCalledWith(EVENTS.NATIVE_NOTIFICATION_REMOVE_ALL); + }); - expect(sendMainEventMock).toHaveBeenCalledWith(EVENTS.WINDOW_HIDE); - expect(sendMainEventMock).toHaveBeenCalledWith(EVENTS.OPEN_EXTERNAL, { - url: 'https://x', - activate: true, - }); + it('nativeNotifications.notifyReady sends the ready event', () => { + const api = getExposedApi(); + + api.nativeNotifications.notifyReady(); + + expect(sendMainEventMock).toHaveBeenCalledWith(EVENTS.NATIVE_NOTIFICATION_READY); + }); + + it('nativeNotifications.onAction forwards actions and returns the unsubscribe function', () => { + const api = getExposedApi(); + const unsubscribe = vi.fn(); + const callback = vi.fn(); + onRendererEventMock.mockReturnValueOnce(unsubscribe); + + const cleanup = api.nativeNotifications.onAction(callback); + const listener = onRendererEventMock.mock.calls[0][1] as ( + event: unknown, + payload: unknown, + ) => void; + const payload = { id: 'account:notification', action: 'read' }; + + listener({}, payload); + + expect(onRendererEventMock).toHaveBeenCalledWith( + EVENTS.NATIVE_NOTIFICATION_ACTION, + expect.any(Function), + ); + expect(callback).toHaveBeenCalledWith(payload); + expect(cleanup).toBe(unsubscribe); + }); + + it('nativeNotifications.onClick forwards clicks and returns the unsubscribe function', () => { + const api = getExposedApi(); + const unsubscribe = vi.fn(); + const callback = vi.fn(); + onRendererEventMock.mockReturnValueOnce(unsubscribe); + + const cleanup = api.nativeNotifications.onClick(callback); + const listener = onRendererEventMock.mock.calls[0][1] as ( + event: unknown, + payload: unknown, + ) => void; + const payload = { id: 'account:notification' }; + + listener({}, payload); + + expect(onRendererEventMock).toHaveBeenCalledWith( + EVENTS.NATIVE_NOTIFICATION_CLICK, + expect.any(Function), + ); + expect(callback).toHaveBeenCalledWith(payload); + expect(cleanup).toBe(unsubscribe); }); }); diff --git a/src/preload/index.ts b/src/preload/index.ts index af6447934..a2100ad65 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -2,6 +2,10 @@ import { contextBridge, webFrame } from 'electron'; import type { IKeyboardShortcut, + INativeNotificationActionEvent, + INativeNotificationClickEvent, + INativeNotificationPayload, + INativeNotificationRemovePayload, NativeThemeSource, TrayIconAppearance, ZoomAction, @@ -269,29 +273,49 @@ export const api = { return onRendererEvent(EVENTS.ZOOM, (_event, action) => callback(action)); }, - /** - * Display a native OS notification. - * - * Clicking the notification opens `url` in the browser (hiding the app window), - * or shows the app window if no URL is provided. - * - * @param title - The notification title. - * @param body - The notification body text. - * @param url - Optional URL to open when the notification is clicked. - * @returns The created `Notification` instance. - */ - raiseNativeNotification: (title: string, body: string, url?: string) => { - const notification = new Notification(title, { body: body, silent: true }); - notification.onclick = () => { - if (url) { - api.app.hide(); - api.openExternalLink(url, true); - } else { - api.app.show(); - } - }; + /** Operating-system notification controls. */ + nativeNotifications: { + /** + * Publish one structured notification through the main process. + * + * @param payload - The notification to publish. + */ + show: (payload: INativeNotificationPayload) => + sendMainEvent(EVENTS.NATIVE_NOTIFICATION_SHOW, payload), + + /** + * Remove published notifications by identity. + * + * @param payload - The identities to remove. + */ + remove: (payload: INativeNotificationRemovePayload) => + sendMainEvent(EVENTS.NATIVE_NOTIFICATION_REMOVE, payload), - return notification; + /** Remove every notification published by this app. */ + removeAll: () => sendMainEvent(EVENTS.NATIVE_NOTIFICATION_REMOVE_ALL), + + /** Tell the main process the renderer action/click listeners are mounted. */ + notifyReady: () => sendMainEvent(EVENTS.NATIVE_NOTIFICATION_READY), + + /** + * Register a callback for an activated notification action. + * + * @param callback - Called with the activated action. + * @returns A function that removes the listener. + */ + onAction: (callback: (payload: INativeNotificationActionEvent) => void) => { + return onRendererEvent(EVENTS.NATIVE_NOTIFICATION_ACTION, (_, payload) => callback(payload)); + }, + + /** + * Register a callback for a notification body click. + * + * @param callback - Called with the clicked notification. + * @returns A function that removes the listener. + */ + onClick: (callback: (payload: INativeNotificationClickEvent) => void) => { + return onRendererEvent(EVENTS.NATIVE_NOTIFICATION_CLICK, (_, payload) => callback(payload)); + }, }, }; diff --git a/src/renderer/components/GlobalEffects.tsx b/src/renderer/components/GlobalEffects.tsx index 235096bb4..684f69318 100644 --- a/src/renderer/components/GlobalEffects.tsx +++ b/src/renderer/components/GlobalEffects.tsx @@ -70,6 +70,7 @@ export function GlobalEffects() { resetFilters(); queryClient.clear(); useShortcutRegistrationStore.getState().reset(); + window.gitify.nativeNotifications.removeAll(); }); }, [resetAccounts, resetSettings, resetFilters, queryClient]); diff --git a/src/renderer/hooks/notifications/useNotificationEffects.ts b/src/renderer/hooks/notifications/useNotificationEffects.ts index 4a629f61c..b15f34943 100644 --- a/src/renderer/hooks/notifications/useNotificationEffects.ts +++ b/src/renderer/hooks/notifications/useNotificationEffects.ts @@ -1,7 +1,16 @@ -import { useEffect, useRef } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; import { focusManager, type QueryKey, useQueryClient } from '@tanstack/react-query'; +import type { + INativeNotificationActionEvent, + INativeNotificationClickEvent, +} from '../../../shared/events'; +import { + getNativeNotificationId, + NATIVE_NOTIFICATION_SUMMARY_ID, +} from '../../../shared/nativeNotifications'; + import { getNotificationFailureKey, useNotificationActionFailuresStore, @@ -10,7 +19,9 @@ import { import type { AccountNotifications, GitifyNotification } from '../../types'; +import { getAccountUUID } from '../../utils/auth/utils'; import { doesAllAccountsHaveErrors } from '../../utils/core/errors'; +import { rendererLogError, toError } from '../../utils/core/logger'; import { filterBaseNotifications, filterDetailedNotifications, @@ -18,7 +29,9 @@ import { import { refreshEnrichmentCache } from '../../utils/notifications/notifications'; import { getNewNotifications } from '../../utils/notifications/utils'; import { raiseSoundNotification } from '../../utils/system/audio'; +import { openNotification } from '../../utils/system/links'; import { raiseNativeNotification } from '../../utils/system/native'; +import type { NotificationActions } from './useNotificationActions'; export interface NotificationEffectsInput { /** @@ -32,12 +45,13 @@ export interface NotificationEffectsInput { notificationsQueryKey: QueryKey; refetch: () => void; fetchIntervalMs: number; + actions: NotificationActions; } /** * The side-effect half of notification state: sound and native alerts for new - * notifications, refetch on system wake and window focus, and pruning of - * session-local failure entries. + * notifications, OS notification interactions and lifecycle cleanup, refetch on + * system wake and window focus, and pruning of session-local failure entries. * * Kept separate from the query and mutations so the side effects can be owned * by a single host rather than repeated per consumer. @@ -50,14 +64,74 @@ export const useNotificationEffects = ({ notificationsQueryKey, refetch, fetchIntervalMs, + actions, }: NotificationEffectsInput): void => { const queryClient = useQueryClient(); const previousNotificationsRef = useRef([]); + const actionsRef = useRef(actions); + const publishedIdsRef = useRef>(new Set()); + + useEffect(() => { + actionsRef.current = actions; + }, [actions]); const playSoundNewNotifications = useSettingsStore((s) => s.playSound); const showSystemNotifications = useSettingsStore((s) => s.showNotifications); const notificationVolume = useSettingsStore((s) => s.notificationVolume); + const readUnfilteredNotifications = useCallback( + () => queryClient.getQueryData(notificationsQueryKey) ?? [], + [queryClient, notificationsQueryKey], + ); + + // Identities are one-way hashes, so resolve one by hashing the cached + // notifications and matching. + const resolveNotification = useCallback( + (id: string): GitifyNotification | null => { + if (!id || id === NATIVE_NOTIFICATION_SUMMARY_ID) { + return null; + } + + for (const accountNotifications of readUnfilteredNotifications()) { + const accountUUID = getAccountUUID(accountNotifications.account); + + for (const notification of accountNotifications.notifications) { + if (getNativeNotificationId(accountUUID, notification.id) === id) { + return notification; + } + } + } + + return null; + }, + [readUnfilteredNotifications], + ); + + const handleNativeAction = useCallback( + (payload: INativeNotificationActionEvent) => { + const notification = resolveNotification(payload.id); + + if (notification) { + void actionsRef.current.markNotificationsAsRead([notification]); + } + }, + [resolveNotification], + ); + + const handleNativeClick = useCallback( + (payload: INativeNotificationClickEvent) => { + const notification = resolveNotification(payload.id); + + if (notification) { + void openNotification(notification); + return; + } + + window.gitify.app.show(); + }, + [resolveNotification], + ); + // Refetch notifications immediately when the system wakes from sleep or the // user unlocks their screen, without waiting for the next poll interval. // Owned by the singleton side-effects host: every consumer registering a @@ -96,7 +170,24 @@ export const useNotificationEffects = ({ }); }, [enabled, fetchIntervalMs, refetch]); - // Handle sound and native notifications when new notifications arrive + // Register OS notification interaction listeners exactly once for the host. + useEffect(() => { + if (!enabled) { + return; + } + + const unsubscribeAction = window.gitify.nativeNotifications.onAction(handleNativeAction); + const unsubscribeClick = window.gitify.nativeNotifications.onClick(handleNativeClick); + + window.gitify.nativeNotifications.notifyReady(); + + return () => { + unsubscribeAction(); + unsubscribeClick(); + }; + }, [enabled, handleNativeAction, handleNativeClick]); + + // Handle sound and native notifications when new notifications arrive. useEffect(() => { if (!enabled || isLoading || isError || notifications.length === 0) { return; @@ -107,8 +198,7 @@ export const useNotificationEffects = ({ return; } - const unfilteredNotifications = - queryClient.getQueryData(notificationsQueryKey) || []; + const unfilteredNotifications = readUnfilteredNotifications(); const diffNotifications = getNewNotifications( previousNotificationsRef.current, @@ -127,7 +217,19 @@ export const useNotificationEffects = ({ } if (showSystemNotifications) { - void raiseNativeNotification(filteredDiffNotifications); + void Promise.resolve(raiseNativeNotification(filteredDiffNotifications)) + .then((published) => { + for (const payload of published ?? []) { + publishedIdsRef.current.add(payload.id); + } + }) + .catch((error) => { + rendererLogError( + 'useNotificationEffects', + 'Failed to publish native notifications', + toError(error), + ); + }); } } } @@ -141,10 +243,44 @@ export const useNotificationEffects = ({ playSoundNewNotifications, showSystemNotifications, notificationVolume, - queryClient, - notificationsQueryKey, + readUnfilteredNotifications, ]); + // Remove published notifications when they leave the unfiltered cache, or + // keep the batch summary until nothing unread remains. + useEffect(() => { + if (!enabled || publishedIdsRef.current.size === 0) { + return; + } + + const unfilteredNotifications = readUnfilteredNotifications(); + const currentIds = new Set( + unfilteredNotifications.flatMap((accountNotifications) => { + const accountUUID = getAccountUUID(accountNotifications.account); + return accountNotifications.notifications.map((notification) => + getNativeNotificationId(accountUUID, notification.id), + ); + }), + ); + const hasUnread = unfilteredNotifications.some((accountNotifications) => + accountNotifications.notifications.some((notification) => notification.unread), + ); + + const removed = [...publishedIdsRef.current].filter((id) => + id === NATIVE_NOTIFICATION_SUMMARY_ID ? !hasUnread : !currentIds.has(id), + ); + + if (removed.length === 0) { + return; + } + + window.gitify.nativeNotifications.remove({ ids: removed }); + + for (const id of removed) { + publishedIdsRef.current.delete(id); + } + }, [enabled, notifications, readUnfilteredNotifications]); + // Session-local failure entries are independent of the notifications // cache, so they must be pruned separately once a notification no longer // appears in the (unfiltered) list - e.g. actioned successfully elsewhere, @@ -156,8 +292,7 @@ export const useNotificationEffects = ({ return; } - const unfilteredNotifications = - queryClient.getQueryData(notificationsQueryKey) || []; + const unfilteredNotifications = readUnfilteredNotifications(); const currentNotificationKeys = unfilteredNotifications.flatMap((accountNotifications) => accountNotifications.notifications.map((notification) => @@ -166,5 +301,5 @@ export const useNotificationEffects = ({ ); useNotificationActionFailuresStore.getState().pruneFailures(currentNotificationKeys); - }, [enabled, notifications, queryClient, notificationsQueryKey]); + }, [enabled, notifications, readUnfilteredNotifications]); }; diff --git a/src/renderer/hooks/notifications/useNotificationsHost.ts b/src/renderer/hooks/notifications/useNotificationsHost.ts index 1aeebb1d6..1d5f99e54 100644 --- a/src/renderer/hooks/notifications/useNotificationsHost.ts +++ b/src/renderer/hooks/notifications/useNotificationsHost.ts @@ -25,6 +25,7 @@ export const useNotificationsHost = (): NotificationsState => { notificationsQueryKey: query.notificationsQueryKey, refetch: query.refetch, fetchIntervalMs: query.fetchIntervalMs, + actions, }); return buildNotificationsState(query, actions); diff --git a/src/renderer/hooks/useNotifications.test.tsx b/src/renderer/hooks/useNotifications.test.tsx index 40105b920..403cdfea4 100644 --- a/src/renderer/hooks/useNotifications.test.tsx +++ b/src/renderer/hooks/useNotifications.test.tsx @@ -14,6 +14,8 @@ import { mockSingleAccountNotifications, } from '../__mocks__/notifications-mocks'; +import { getNativeNotificationId } from '../../shared/nativeNotifications'; + import { getNotificationFailureKey, useAccountsStore, @@ -24,6 +26,7 @@ import { import type { AccountNotifications, Percentage } from '../types'; +import { getAccountUUID } from '../utils/auth/utils'; import { getErrorDetails } from '../utils/core/errors'; import * as logger from '../utils/core/logger'; import { getAdapter } from '../utils/forges/registry'; @@ -33,6 +36,7 @@ import { reportServerPollInterval, } from '../utils/notifications/pollInterval'; import * as audio from '../utils/system/audio'; +import * as links from '../utils/system/links'; import * as native from '../utils/system/native'; import { useNotificationsHost } from './notifications/useNotificationsHost'; import { useNotifications } from './useNotifications'; @@ -935,6 +939,68 @@ describe('renderer/hooks/useNotifications.ts', () => { }); }); + describe('OS notification interactions', () => { + const notification = mockSingleAccountNotifications[0].notifications[0]; + const notificationId = getNativeNotificationId( + getAccountUUID(notification.account), + notification.id, + ); + + const actionHandler = () => + vi.mocked(window.gitify.nativeNotifications.onAction).mock.calls.at(-1)?.[0] as (payload: { + id: string; + }) => void; + + const clickHandler = () => + vi.mocked(window.gitify.nativeNotifications.onClick).mock.calls.at(-1)?.[0] as (payload: { + id: string; + }) => void; + + it('marks the notification as read from its OS action', async () => { + const actionSpy = vi + .spyOn(githubAdapter.accountOps, 'markThreadAsRead') + .mockResolvedValue(undefined); + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.hasNotifications).toBe(true)); + + await act(async () => { + actionHandler()({ id: notificationId }); + }); + + await waitFor(() => expect(actionSpy).toHaveBeenCalledTimes(1)); + }); + + it('opens the notification target when the body is clicked', async () => { + const openSpy = vi.spyOn(links, 'openNotification').mockResolvedValue(); + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.hasNotifications).toBe(true)); + + act(() => { + clickHandler()({ id: notificationId }); + }); + + expect(openSpy).toHaveBeenCalledWith(expect.objectContaining({ id: notification.id })); + }); + + it('shows the app when a click cannot be resolved', async () => { + const showSpy = vi.spyOn(window.gitify.app, 'show'); + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.hasNotifications).toBe(true)); + + act(() => { + clickHandler()({ id: 'unknown' }); + }); + + expect(showSpy).toHaveBeenCalled(); + }); + }); + describe('system wake', () => { it('refetches notifications when the system wakes', async () => { useAccountsStore.setState({ accounts: [mockGitHubCloudAccount] }); diff --git a/src/renderer/i18n/locales/en.json b/src/renderer/i18n/locales/en.json index 427d058f0..9f2c5076a 100644 --- a/src/renderer/i18n/locales/en.json +++ b/src/renderer/i18n/locales/en.json @@ -297,6 +297,9 @@ "stacked_pr": "Stacked pull request", "stacked_pr_position": "Stacked pull request: {{position}} of {{depth}}" }, + "native": { + "summary_body_other": "You have {{count}} notifications" + }, "notifications": { "failure_tooltip": "{{title}}: {{description}} You can also try opening this notification in your browser.", "mark_as_done": "Mark as done", diff --git a/src/renderer/utils/system/native.test.ts b/src/renderer/utils/system/native.test.ts index 416d331c9..0f4722378 100644 --- a/src/renderer/utils/system/native.test.ts +++ b/src/renderer/utils/system/native.test.ts @@ -1,43 +1,115 @@ -import { waitFor } from '@testing-library/react'; - import { - mockMultipleAccountNotifications, - mockSingleAccountNotifications, + mockGitHubCloudGitifyNotifications, + mockGitifyNotification, } from '../../__mocks__/notifications-mocks'; -import * as url from '../notifications/url'; -import * as native from './native'; +import { + getNativeNotificationId, + NATIVE_NOTIFICATION_SUMMARY_ID, +} from '../../../shared/nativeNotifications'; + +import { getAccountUUID } from '../auth/utils'; +import { + buildNativeNotificationPayload, + NATIVE_NOTIFICATION_BATCH_CAP, + raiseNativeNotification, +} from './native'; describe('renderer/utils/system/native.ts', () => { - const mockHtmlUrl = mockSingleAccountNotifications[0].notifications[0].repository.htmlUrl; + const notification = mockGitifyNotification; + const accountUUID = getAccountUUID(notification.account); + + beforeEach(() => { + vi.mocked(window.gitify.nativeNotifications.show).mockClear(); + }); + + describe('buildNativeNotificationPayload', () => { + it('builds the identity, content, and Mark as read action', () => { + const payload = buildNativeNotificationPayload(notification); + + expect(payload.id).toBe(getNativeNotificationId(accountUUID, notification.id)); + expect(payload.title).toBe(notification.repository.fullName); + expect(payload.subtitle).toContain(notification.reason.title); + expect(payload.body).toContain(notification.subject.title); + expect(payload.actionLabel).toBe('Mark as read'); + }); - vi.spyOn(url, 'generateNotificationWebUrl').mockImplementation(async () => mockHtmlUrl); + it('omits the action for a notification that is already read', () => { + const payload = buildNativeNotificationPayload({ ...notification, unread: false }); - it('raises a native notification for a single new notification', async () => { - void native.raiseNativeNotification(mockSingleAccountNotifications[0].notifications); + expect(payload.actionLabel).toBeUndefined(); + }); - await waitFor(() => expect(window.gitify.raiseNativeNotification).toHaveBeenCalledTimes(1)); + it('handles a notification without an actor', () => { + const payload = buildNativeNotificationPayload({ + ...notification, + subject: { + ...notification.subject, + user: undefined, + author: undefined, + commenter: undefined, + }, + }); - expect(window.gitify.raiseNativeNotification).toHaveBeenCalledWith( - expect.stringContaining( - mockSingleAccountNotifications[0].notifications[0].repository.fullName, - ), - mockSingleAccountNotifications[0].notifications[0].subject.title, - expect.stringContaining(mockHtmlUrl), - ); - expect(url.generateNotificationWebUrl).toHaveBeenCalledTimes(1); + expect(payload.subtitle).toBe(notification.reason.title); + }); + + it('includes the identifier when the title does not contain it', () => { + const payload = buildNativeNotificationPayload({ + ...notification, + subject: { ...notification.subject, title: 'A title without a number' }, + display: { ...notification.display, identifier: '#123' }, + }); + + expect(payload.body).toContain('#123'); + }); }); - it('raises a native notification for multiple new notifications', async () => { - void native.raiseNativeNotification(mockMultipleAccountNotifications[0].notifications); + describe('raiseNativeNotification', () => { + it('publishes individual notifications at or below the cap', async () => { + const showMock = vi.mocked(window.gitify.nativeNotifications.show); + const notifications = mockGitHubCloudGitifyNotifications.slice( + 0, + NATIVE_NOTIFICATION_BATCH_CAP, + ); + + const published = await raiseNativeNotification(notifications); + + expect(published).toHaveLength(notifications.length); + expect(showMock).toHaveBeenCalledTimes(notifications.length); + expect( + showMock.mock.calls.every(([payload]) => payload.id !== NATIVE_NOTIFICATION_SUMMARY_ID), + ).toBe(true); + }); + + it('publishes one summary above the cap', async () => { + const showMock = vi.mocked(window.gitify.nativeNotifications.show); + const notifications = Array.from( + { length: NATIVE_NOTIFICATION_BATCH_CAP + 1 }, + (_, index) => ({ + ...notification, + id: `notification-${index}`, + order: index, + }), + ); + + const published = await raiseNativeNotification(notifications); + + expect(published).toHaveLength(1); + expect(showMock).toHaveBeenCalledTimes(1); + expect(showMock).toHaveBeenCalledWith( + expect.objectContaining({ + id: NATIVE_NOTIFICATION_SUMMARY_ID, + body: `You have ${notifications.length} notifications`, + }), + ); + }); - await waitFor(() => expect(window.gitify.raiseNativeNotification).toHaveBeenCalledTimes(1)); + it('does nothing for an empty batch', async () => { + const showMock = vi.mocked(window.gitify.nativeNotifications.show); - expect(window.gitify.raiseNativeNotification).toHaveBeenCalledWith( - 'Gitify', - 'You have 2 notifications', - undefined, - ); - expect(url.generateNotificationWebUrl).toHaveBeenCalledTimes(0); + await expect(raiseNativeNotification([])).resolves.toEqual([]); + expect(showMock).not.toHaveBeenCalled(); + }); }); }); diff --git a/src/renderer/utils/system/native.ts b/src/renderer/utils/system/native.ts index 872f138e3..62d03788b 100644 --- a/src/renderer/utils/system/native.ts +++ b/src/renderer/utils/system/native.ts @@ -1,33 +1,90 @@ import { APPLICATION } from '../../../shared/constants'; +import type { INativeNotificationPayload } from '../../../shared/events'; +import { + getNativeNotificationId, + NATIVE_NOTIFICATION_SUMMARY_ID, +} from '../../../shared/nativeNotifications'; import type { GitifyNotification } from '../../types'; -import { generateNotificationWebUrl } from '../notifications/url'; +import i18n from '../../i18n'; +import { getAccountUUID } from '../auth/utils'; + +/** Maximum individual notifications published for one batch. */ +export const NATIVE_NOTIFICATION_BATCH_CAP = 5; /** - * Raises a native OS notification. + * Build the structured payload for one notification. * - * For a single notification, the repository full name is the title, the subject - * title is the body, and the generated web URL is the click target. For - * multiple notifications, the app name is the title and the body summarizes the - * notification count. + * @param notification - The notification to publish. + * @returns The main-process payload. + */ +export function buildNativeNotificationPayload( + notification: GitifyNotification, +): INativeNotificationPayload { + const accountUUID = getAccountUUID(notification.account); + const actor = notification.subject.user?.login ?? notification.subject.author?.login; + const subtitle = [notification.reason.title, actor ? `@${actor}` : ''] + .filter(Boolean) + .join(' ยท '); + const identifier = notification.display.identifier; + const body = + identifier && !notification.subject.title.includes(identifier) + ? `${notification.subject.title} (${identifier})` + : notification.subject.title; + + return { + id: getNativeNotificationId(accountUUID, notification.id), + title: notification.repository.fullName, + subtitle: subtitle || undefined, + body, + actionLabel: notification.unread + ? i18n.t('notifications.mark_as_read', 'Mark as read') + : undefined, + }; +} + +/** + * Summary text for a batch of notifications. + * + * @param count - The number of new notifications. + * @returns The translated summary body. + */ +function getSummaryBody(count: number): string { + return i18n.t('native.summary_body_other', 'You have {{count}} notifications', { count }); +} + +/** + * Publish new notifications, falling back to one summary above the cap. * - * @param notifications - The notifications to surface as a native OS notification. + * @param notifications - The new notifications to publish. + * @returns The published payloads, so the host can track them for removal. */ -export async function raiseNativeNotification(notifications: GitifyNotification[]) { - let title: string; - let body: string; - let url: string | null = null; - - if (notifications.length === 1) { - const notification = notifications[0]; - title = notification.repository.fullName; - body = notification.subject.title; - url = await generateNotificationWebUrl(notification); - } else { - title = APPLICATION.NAME; - body = `You have ${notifications.length} notifications`; +export async function raiseNativeNotification( + notifications: GitifyNotification[], +): Promise { + if (notifications.length === 0) { + return []; + } + + if (notifications.length > NATIVE_NOTIFICATION_BATCH_CAP) { + const summary: INativeNotificationPayload = { + id: NATIVE_NOTIFICATION_SUMMARY_ID, + title: APPLICATION.NAME, + body: getSummaryBody(notifications.length), + }; + + window.gitify.nativeNotifications.show(summary); + + return [summary]; + } + + const published = notifications.map(buildNativeNotificationPayload); + + for (const payload of published) { + // Notifications must be published in order. + window.gitify.nativeNotifications.show(payload); } - return window.gitify.raiseNativeNotification(title, body, url ?? undefined); + return published; } diff --git a/src/shared/events.ts b/src/shared/events.ts index 09614578a..2ca952268 100644 --- a/src/shared/events.ts +++ b/src/shared/events.ts @@ -28,6 +28,12 @@ export const EVENTS = { RESET_APP: `${P}reset-app`, TWEMOJI_DIRECTORY: `${P}twemoji-directory`, SYSTEM_WAKE: `${P}system-wake`, + NATIVE_NOTIFICATION_SHOW: `${P}native-notification-show`, + NATIVE_NOTIFICATION_REMOVE: `${P}native-notification-remove`, + NATIVE_NOTIFICATION_REMOVE_ALL: `${P}native-notification-remove-all`, + NATIVE_NOTIFICATION_READY: `${P}native-notification-ready`, + NATIVE_NOTIFICATION_ACTION: `${P}native-notification-action`, + NATIVE_NOTIFICATION_CLICK: `${P}native-notification-click`, ZOOM: `${P}zoom`, } as const; @@ -103,6 +109,33 @@ export type IGitHubCliTokenResult = | { token: string; error?: never; detail?: never } | { token?: never; error: GitHubCliTokenError; detail?: string }; +/** A structured request to publish an operating-system notification. */ +export interface INativeNotificationPayload { + /** Short, opaque identity for this notification (see `getNativeNotificationId`). */ + id: string; + title: string; + /** macOS subtitle. Other platforms append it to the body. */ + subtitle?: string; + body: string; + /** Translated label for the single Mark as read action, when one is offered. */ + actionLabel?: string; +} + +/** Payload for removing published notifications. */ +export interface INativeNotificationRemovePayload { + ids: string[]; +} + +/** An action was activated on an operating-system notification. */ +export interface INativeNotificationActionEvent { + id: string; +} + +/** A notification body click. An empty identity is a generic app activation. */ +export interface INativeNotificationClickEvent { + id: string; +} + /** Shape of a single event contract: a request payload and a response payload. */ type Contract = { request: unknown; response: unknown }; @@ -161,6 +194,24 @@ export type EventContracts = AssertEventCoverage<{ [EVENTS.RESET_APP]: { request: undefined; response: undefined }; [EVENTS.TWEMOJI_DIRECTORY]: { request: undefined; response: string }; [EVENTS.SYSTEM_WAKE]: { request: undefined; response: undefined }; + [EVENTS.NATIVE_NOTIFICATION_SHOW]: { + request: INativeNotificationPayload; + response: undefined; + }; + [EVENTS.NATIVE_NOTIFICATION_REMOVE]: { + request: INativeNotificationRemovePayload; + response: undefined; + }; + [EVENTS.NATIVE_NOTIFICATION_REMOVE_ALL]: { request: undefined; response: undefined }; + [EVENTS.NATIVE_NOTIFICATION_READY]: { request: undefined; response: undefined }; + [EVENTS.NATIVE_NOTIFICATION_ACTION]: { + request: INativeNotificationActionEvent; + response: undefined; + }; + [EVENTS.NATIVE_NOTIFICATION_CLICK]: { + request: INativeNotificationClickEvent; + response: undefined; + }; [EVENTS.ZOOM]: { request: ZoomAction; response: undefined }; }>; diff --git a/src/shared/nativeNotifications.test.ts b/src/shared/nativeNotifications.test.ts new file mode 100644 index 000000000..354cbcfb7 --- /dev/null +++ b/src/shared/nativeNotifications.test.ts @@ -0,0 +1,40 @@ +import { getNativeNotificationId, NATIVE_NOTIFICATION_SUMMARY_ID } from './nativeNotifications'; + +// Windows caps a toast's tag at 64 characters. +const WINDOWS_TOAST_ID_LIMIT = 64; + +const LONG_ACCOUNT_UUID = btoa( + 'github.enterprise.some-very-long-company-hostname.example.com-123456789-Personal Access Token', +); + +describe('shared/nativeNotifications.ts', () => { + describe('getNativeNotificationId', () => { + it('is deterministic', () => { + expect(getNativeNotificationId('account', '123')).toBe( + getNativeNotificationId('account', '123'), + ); + }); + + it('differs per account and per notification', () => { + const id = getNativeNotificationId('account', '123'); + + expect(getNativeNotificationId('other-account', '123')).not.toBe(id); + expect(getNativeNotificationId('account', '124')).not.toBe(id); + }); + + it('does not collide when the separator moves', () => { + expect(getNativeNotificationId('ab', 'c')).not.toBe(getNativeNotificationId('a', 'bc')); + }); + + it('stays within the Windows toast tag limit for long accounts', () => { + const id = getNativeNotificationId(LONG_ACCOUNT_UUID, '99999999999999999999'); + + expect(id.length).toBeLessThanOrEqual(WINDOWS_TOAST_ID_LIMIT); + expect(id).toMatch(/^n[0-9a-z]+$/); + }); + + it('never equals the summary identity', () => { + expect(getNativeNotificationId('account', '1')).not.toBe(NATIVE_NOTIFICATION_SUMMARY_ID); + }); + }); +}); diff --git a/src/shared/nativeNotifications.ts b/src/shared/nativeNotifications.ts new file mode 100644 index 000000000..f380de713 --- /dev/null +++ b/src/shared/nativeNotifications.ts @@ -0,0 +1,43 @@ +/** + * Identity of the summary notification published for a large batch. + */ +export const NATIVE_NOTIFICATION_SUMMARY_ID = 'summary'; + +/** + * Build the operating-system notification identity for a notification. + * + * Windows caps a toast's tag at 64 characters, and account UUIDs plus thread + * IDs easily exceed that. The identity is therefore a short, deterministic + * hash: the main process treats it as opaque, and the renderer resolves an + * interaction by hashing its cached notifications and matching. + * + * @param accountUUID - The account that owns the notification. + * @param notificationId - The notification's thread ID. + * @returns A stable identity of at most 12 characters. + */ +export function getNativeNotificationId(accountUUID: string, notificationId: string): string { + return `n${hash(`${accountUUID}\u0000${notificationId}`)}`; +} + +/** + * 53-bit string hash (cyrb53), rendered in base 36. Not cryptographic: it only + * needs to be stable and collision-resistant across one user's notifications. + * + * @param value - The string to hash. + * @returns The hash as a base-36 string of at most 11 characters. + */ +function hash(value: string): string { + let h1 = 0xdeadbeef; + let h2 = 0x41c6ce57; + + for (let i = 0; i < value.length; i++) { + const code = value.charCodeAt(i); + h1 = Math.imul(h1 ^ code, 2654435761); + h2 = Math.imul(h2 ^ code, 1597334677); + } + + h1 = Math.imul(h1 ^ (h1 >>> 16), 2246822507) ^ Math.imul(h2 ^ (h2 >>> 13), 3266489909); + h2 = Math.imul(h2 ^ (h2 >>> 16), 2246822507) ^ Math.imul(h1 ^ (h1 >>> 13), 3266489909); + + return (4294967296 * (2097151 & h2) + (h1 >>> 0)).toString(36); +}