From 044818329fff5194ad16ea11e8bd0319895fa72a Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sat, 24 Oct 2026 13:40:44 -0400 Subject: [PATCH 1/6] feat(notifications): add actionable native alerts Move OS notifications into a main-process service behind typed IPC, with native actions, grouping, and list anchoring. Keep focus scrolling animated and make the browser tests wait for the scroll to settle. Address the Sonar findings on identifier validation, optional chaining, stable string sorting, and the intentionally sequential native publish loop. --- ARCHITECTURE.md | 16 +- CONTRIBUTING.md | 10 + electron-builder.js | 2 + package.json | 1 + scripts/sign-dev-electron.sh | 84 +++ src/main/handlers/index.ts | 1 + .../handlers/native-notifications.test.ts | 99 +++ src/main/handlers/native-notifications.ts | 43 ++ src/main/index.test.ts | 7 +- src/main/index.ts | 2 + src/main/notifications.test.ts | 303 ++++++++++ src/main/notifications.ts | 389 ++++++++++++ src/preload/index.test.ts | 129 ++-- src/preload/index.ts | 72 ++- src/renderer/App.tsx | 2 +- src/renderer/__helpers__/visual.setup.ts | 10 +- src/renderer/__helpers__/vitest.setup.ts | 10 +- src/renderer/__mocks__/state-mocks.ts | 6 + .../components/GlobalEffects.test.tsx | 1 + src/renderer/components/GlobalEffects.tsx | 9 +- .../notifications/NotificationList.test.tsx | 390 +++++++++++- .../notifications/NotificationList.tsx | 150 ++++- .../NotificationList.visual.test.tsx | 259 +++++++- .../notifications/NotificationRow.tsx | 5 + .../settings/AlertSettings.test.tsx | 137 +++-- .../components/settings/AlertSettings.tsx | 119 +++- .../notifications/useNotificationEffects.ts | 283 ++++++++- .../useNotificationListDataKey.ts | 59 ++ .../notifications/useNotificationsHost.ts | 15 +- src/renderer/hooks/useNotifications.test.tsx | 183 ++++++ .../__snapshots__/Settings.test.tsx.snap | 568 +++++++++++++++++- src/renderer/stores/defaults.ts | 6 + src/renderer/stores/index.ts | 1 + src/renderer/stores/types.ts | 22 + .../stores/useNotificationFocusStore.test.ts | 61 ++ .../stores/useNotificationFocusStore.ts | 37 ++ src/renderer/types.ts | 17 + src/renderer/utils/notifications/listItems.ts | 7 +- .../notifications/listPositioning.test.ts | 397 ++++++++++++ .../utils/notifications/listPositioning.ts | 180 ++++++ src/renderer/utils/system/native.test.ts | 167 ++++- src/renderer/utils/system/native.ts | 132 +++- src/shared/events.ts | 69 +++ src/shared/nativeNotifications.ts | 46 ++ 44 files changed, 4279 insertions(+), 227 deletions(-) create mode 100644 scripts/sign-dev-electron.sh create mode 100644 src/main/handlers/native-notifications.test.ts create mode 100644 src/main/handlers/native-notifications.ts create mode 100644 src/main/notifications.test.ts create mode 100644 src/main/notifications.ts create mode 100644 src/renderer/hooks/notifications/useNotificationListDataKey.ts create mode 100644 src/renderer/stores/useNotificationFocusStore.test.ts create mode 100644 src/renderer/stores/useNotificationFocusStore.ts create mode 100644 src/renderer/utils/notifications/listPositioning.test.ts create mode 100644 src/renderer/utils/notifications/listPositioning.ts create mode 100644 src/shared/nativeNotifications.ts diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 80d1d9379..d71bee508 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -51,13 +51,13 @@ The renderer is layered. **Runtime imports flow in one direction only** โ€” a mo Every kind of renderer state has exactly one owner. Put new state with the mechanism that owns its kind: -| Kind of state | Owner | Examples | -| ---------------------- | ---------------------------------------------------- | -------------------------------------------------------- | -| Server / async data | TanStack Query | the notification list, periodic account refreshes | -| Persisted client state | Zustand with `persist` | `accounts`, `settings` | -| Session client state | Zustand without `persist` | `filters`, notification action failures | -| Derived values | A store selector/action, or a pure `utils/` function | `isLoggedIn`, `hasMultipleAccounts`, notification status | -| Ephemeral view state | React `useState` / `useMemo` | row expand/collapse, exit animations | +| Kind of state | Owner | Examples | +| ---------------------- | ---------------------------------------------------- | ------------------------------------------------------------------ | +| Server / async data | TanStack Query | the notification list, periodic account refreshes | +| Persisted client state | Zustand with `persist` | `accounts`, `settings` | +| Session client state | Zustand without `persist` | `filters`, notification action failures, notification focus intent | +| Derived values | A store selector/action, or a pure `utils/` function | `isLoggedIn`, `hasMultipleAccounts`, notification status | +| Ephemeral view state | React `useState` / `useMemo` | row expand/collapse, exit animations | Rules: @@ -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, routes OS actions and clicks back into the existing mutation/recovery pipeline, 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 aea7fb27a..97e449a89 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -85,6 +85,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 and have platform-specific prerequisites: + +- **macOS**: native notifications require a properly signed app; the ad-hoc-signed Electron development binary is rejected by `UNUserNotificationCenter`. Run `pnpm sign:dev` once after installing dependencies (and again after changing the Electron version), then restart `pnpm dev`. Action buttons additionally require `NSUserNotificationAlertStyle: alert` in the packaged app. +- **Windows**: action buttons use the toast action support; `app.setAppUserModelId` is set when the menubar is ready. Verify Action Center persistence and cold-start activation manually. +- **Linux**: action buttons appear only when the desktop notification server advertises the `actions` capability. Without it, the notification still shows without buttons. + +Verify the notification content, click behavior, actions, and removal-on-read on a signed macOS build, Windows, and an action-capable Linux desktop before releasing a change in this area. + ### ๐ŸŽจ Code Style & Conventions - Linting and formatting are configured in `vite.config.mts` (the `lint` and `fmt` blocks). Please run `pnpm check` before submitting a PR. diff --git a/electron-builder.js b/electron-builder.js index dc20ce5d4..8a33858fd 100644 --- a/electron-builder.js +++ b/electron-builder.js @@ -26,6 +26,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 ef2454977..aceabfc91 100644 --- a/package.json +++ b/package.json @@ -58,6 +58,7 @@ "codegen:watch": "pnpm codegen --watch", "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\"", + "sign:dev": "bash scripts/sign-dev-electron.sh", "start": "pnpm build && pnpm dev", "package:linux": "electron-builder --linux", "package:macos": "electron-builder --mac", diff --git a/scripts/sign-dev-electron.sh b/scripts/sign-dev-electron.sh new file mode 100644 index 000000000..b7b9e5101 --- /dev/null +++ b/scripts/sign-dev-electron.sh @@ -0,0 +1,84 @@ +#!/usr/bin/env bash +# +# Sign the development Electron binary so macOS accepts native notifications. +# +# Electron's main-process Notification API uses UNUserNotificationCenter, which +# rejects ad-hoc/linker-signed binaries with UNErrorDomain error 1. A proper +# Developer ID or Apple Development signature is required even for local +# development. +# +# This signs the Electron.app inside the installed `electron` package. Run it +# after `pnpm install` and after changing the Electron version. +# +# Usage: +# pnpm sign:dev +# GITIFY_DEV_SIGN_IDENTITY="Apple Development: ..." pnpm sign:dev + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "$REPO_ROOT" + +if [[ "$(uname -s)" != "Darwin" ]]; then + echo "Skipping dev Electron signing: not macOS." + exit 0 +fi + +IDENTITY="${GITIFY_DEV_SIGN_IDENTITY:-}" + +if [[ -z "$IDENTITY" ]]; then + IDENTITY="$( + security find-identity -v -p codesigning 2>/dev/null | + sed -n 's/.*"\(Developer ID Application:.*\)"/\1/p' | + head -1 + )" +fi + +if [[ -z "$IDENTITY" ]]; then + IDENTITY="$( + security find-identity -v -p codesigning 2>/dev/null | + sed -n 's/.*"\(Apple Development:.*\)"/\1/p' | + head -1 + )" +fi + +if [[ -z "$IDENTITY" ]]; then + echo "No macOS code-signing identity found." >&2 + echo "Install a Developer ID or Apple Development certificate, or set" >&2 + echo "GITIFY_DEV_SIGN_IDENTITY to one from 'security find-identity -v -p codesigning'." >&2 + exit 1 +fi + +ELECTRON_BIN="$(node -p "require('electron')")" +ELECTRON_APP="${ELECTRON_BIN%/Contents/MacOS/Electron}" + +if [[ ! -d "$ELECTRON_APP" ]]; then + echo "Could not locate the installed Electron.app at: $ELECTRON_APP" >&2 + exit 1 +fi + +PLIST="$ELECTRON_APP/Contents/Info.plist" + +# Electron's notification action support requires the alert notification style. +# The stock development Electron bundle does not declare it. +if /usr/libexec/PlistBuddy -c 'Print :NSUserNotificationAlertStyle' "$PLIST" >/dev/null 2>&1; then + /usr/libexec/PlistBuddy -c 'Set :NSUserNotificationAlertStyle alert' "$PLIST" +else + /usr/libexec/PlistBuddy -c 'Add :NSUserNotificationAlertStyle string alert' "$PLIST" +fi + +echo "Signing $ELECTRON_APP" +echo "Identity: $IDENTITY" + +codesign \ + --force \ + --deep \ + --options runtime \ + --entitlements "$REPO_ROOT/assets/entitlements.mac.plist" \ + --sign "$IDENTITY" \ + "$ELECTRON_APP" + +codesign --verify --deep --strict "$ELECTRON_APP" + +echo "Signed development Electron binary. Restart pnpm dev to pick up the new signature." +codesign -dv --verbose=2 "$ELECTRON_APP" 2>&1 | grep -E 'Identifier=|Authority=|TeamIdentifier=' 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..a0405e9fc --- /dev/null +++ b/src/main/handlers/native-notifications.test.ts @@ -0,0 +1,99 @@ +import type { Menubar } from 'electron-menubar'; + +import { EVENTS } from '../../shared/events'; + +const mocks = vi.hoisted(() => ({ + service: { + start: vi.fn(), + show: vi.fn(), + isSupported: vi.fn(), + remove: vi.fn(), + removeAll: vi.fn(), + markRendererReady: vi.fn(), + destroy: vi.fn(), + }, + handleMainEvent: vi.fn(), + onMainEvent: vi.fn(), + appOnce: vi.fn(), +})); + +vi.mock('../notifications', () => ({ + NativeNotificationService: vi.fn(function () { + return mocks.service; + }), +})); + +vi.mock('../events', () => ({ + handleMainEvent: (...args: unknown[]) => mocks.handleMainEvent(...args), + 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.handleMainEvent.mockClear(); + mocks.onMainEvent.mockClear(); + mocks.appOnce.mockClear(); + mocks.service.start.mockClear(); + mocks.service.destroy.mockClear(); + }); + + it('registers the show and support handlers plus the interaction listeners', () => { + registerNativeNotificationHandlers(mb); + + expect(mocks.service.start).toHaveBeenCalledTimes(1); + expect(mocks.handleMainEvent).toHaveBeenCalledWith( + EVENTS.NATIVE_NOTIFICATION_SHOW, + expect.any(Function), + ); + expect(mocks.handleMainEvent).toHaveBeenCalledWith( + EVENTS.NATIVE_NOTIFICATION_SUPPORTED, + 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('is idempotent', () => { + const first = registerNativeNotificationHandlers(mb); + const second = registerNativeNotificationHandlers(mb); + + expect(first).toBe(second); + expect(mocks.service.start).toHaveBeenCalledTimes(1); + expect(mocks.handleMainEvent).toHaveBeenCalledTimes(2); + expect(mocks.onMainEvent).toHaveBeenCalledTimes(3); + }); + + 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..6533a88e0 --- /dev/null +++ b/src/main/handlers/native-notifications.ts @@ -0,0 +1,43 @@ +import { app } from 'electron'; +import type { Menubar } from 'electron-menubar'; + +import { EVENTS } from '../../shared/events'; + +import { handleMainEvent, 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; + instance.start(); + + handleMainEvent(EVENTS.NATIVE_NOTIFICATION_SHOW, (_, payload) => instance.show(payload)); + handleMainEvent(EVENTS.NATIVE_NOTIFICATION_SUPPORTED, () => instance.isSupported()); + 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 56f7e478e..9de0316dc 100644 --- a/src/main/index.test.ts +++ b/src/main/index.test.ts @@ -38,6 +38,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('applies the Ozone backend before Electron becomes ready', () => { - expect(calls).toEqual(['ozone', 'whenReady']); + expect(calls.slice(0, 2)).toEqual(['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 8b9f58646..938ffd8c1 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..124f75a10 --- /dev/null +++ b/src/main/notifications.test.ts @@ -0,0 +1,303 @@ +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(), + toError: (error: unknown) => (error instanceof Error ? error : new Error(String(error))), +})); + +vi.mock('electron', () => { + class MockNotification { + static instances: MockNotification[] = []; + static isSupported = vi.fn(() => true); + static remove = vi.fn(); + static removeAll = vi.fn(); + static removeGroup = vi.fn(); + static getHistory = vi.fn(async () => []); + static handleActivation = vi.fn(); + + id: string; + groupId?: string; + options: unknown; + show = vi.fn(); + close = vi.fn(); + private listeners = new Map void>(); + + constructor(options: { id: string; groupId?: string }) { + this.id = options.id; + this.groupId = options.groupId; + 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; + groupId?: 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; + removeGroup: ReturnType; + getHistory: ReturnType; + handleActivation: ReturnType; +}; + +function makeMenubar() { + const send = vi.fn(); + const mb = { + window: { + webContents: { send }, + isDestroyed: () => false, + }, + } as unknown as Menubar; + + return { mb, send }; +} + +const basePayload: INativeNotificationPayload = { + id: 'account:notification', + groupId: 'account:repo', + groupTitle: 'owner/repo', + title: 'owner/repo', + subtitle: 'Updated ยท @octocat', + body: 'A title', + url: 'https://example.com/1', + actions: ['show', 'read', 'unsubscribe'], + keepVisible: false, +}; + +describe('main/notifications.ts', () => { + beforeEach(() => { + NotificationMock.instances = []; + NotificationMock.isSupported.mockReturnValue(true); + NotificationMock.remove.mockClear(); + NotificationMock.removeAll.mockClear(); + NotificationMock.removeGroup.mockClear(); + NotificationMock.getHistory.mockResolvedValue([]); + NotificationMock.handleActivation.mockClear(); + sendRendererEventMock.mockClear(); + }); + + it('returns unsupported without constructing a notification', () => { + NotificationMock.isSupported.mockReturnValue(false); + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'darwin'); + + expect(service.show(basePayload)).toEqual({ supported: false }); + expect(NotificationMock.instances).toHaveLength(0); + }); + + it('constructs and shows a notification with the requested options', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'linux'); + + expect(service.show(basePayload)).toEqual({ supported: true }); + const instance = NotificationMock.instances[0]; + + expect(instance.options).toMatchObject({ + id: basePayload.id, + groupId: basePayload.groupId, + title: basePayload.title, + body: `${basePayload.subtitle}\n${basePayload.body}`, + silent: true, + timeoutType: 'default', + actions: [ + { type: 'button', text: 'Show notifications' }, + { type: 'button', text: 'Mark as read' }, + { type: 'button', text: 'Unsubscribe' }, + ], + }); + expect(instance.show).toHaveBeenCalledTimes(1); + }); + + it('uses subtitle on macOS and keep-visible timeout when requested', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'darwin'); + + service.show({ ...basePayload, keepVisible: true }); + const instance = NotificationMock.instances[0]; + + expect(instance.options.subtitle).toBe(basePayload.subtitle); + expect(instance.options.body).toBe(basePayload.body); + expect(instance.options.timeoutType).toBe('never'); + }); + + it('replaces an existing notification with the same identity', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'linux'); + + service.show(basePayload); + const first = NotificationMock.instances[0]; + service.show(basePayload); + + expect(first.close).toHaveBeenCalledTimes(1); + expect(NotificationMock.instances).toHaveLength(2); + }); + + it('rejects malformed payloads without constructing a notification', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'linux'); + + expect(service.show({ ...basePayload, id: '' })).toEqual({ supported: true }); + expect( + service.show({ + ...basePayload, + actions: ['unknown'] as unknown as INativeNotificationPayload['actions'], + }), + ).toEqual({ supported: true }); + expect(service.show({ ...basePayload, url: 'javascript:alert(1)' })).toEqual({ + supported: true, + }); + expect(NotificationMock.instances).toHaveLength(0); + }); + + it('validates identifiers, actions, and URLs', () => { + expect(validateNativeNotificationPayload(basePayload)).toBe(true); + expect(validateNativeNotificationPayload({ ...basePayload, groupId: '' })).toBe(false); + expect( + validateNativeNotificationPayload({ + ...basePayload, + actions: ['show', 'bad'] as unknown as INativeNotificationPayload['actions'], + }), + ).toBe(false); + expect(validateNativeNotificationPayload({ ...basePayload, url: 'file:///tmp/x' })).toBe(false); + }); + + it('delivers click and action events 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', {}, 1); + + expect(sendRendererEventMock).toHaveBeenCalledWith(mb, EVENTS.NATIVE_NOTIFICATION_CLICK, { + id: basePayload.id, + }); + expect(sendRendererEventMock).toHaveBeenCalledWith(mb, EVENTS.NATIVE_NOTIFICATION_ACTION, { + id: basePayload.id, + action: 'read', + }); + }); + + 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('removes by identity, group, and all', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'darwin'); + service.show(basePayload); + service.show({ ...basePayload, id: 'second' }); + + service.remove({ ids: [basePayload.id] }); + expect(NotificationMock.instances[0].close).toHaveBeenCalledTimes(1); + expect(NotificationMock.remove).toHaveBeenCalledWith(basePayload.id); + + service.remove({ groupId: basePayload.groupId }); + expect(NotificationMock.instances[1].close).toHaveBeenCalledTimes(1); + expect(NotificationMock.removeGroup).toHaveBeenCalledWith(basePayload.groupId); + + service.removeAll(); + expect(NotificationMock.removeAll).toHaveBeenCalledTimes(1); + }); + + it('treats unknown identities as a no-op', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'linux'); + + expect(() => service.remove({ ids: ['missing'] })).not.toThrow(); + }); + + it('restores delivered macOS notifications', async () => { + const history = [ + { + id: 'history-id', + on: vi.fn(), + close: vi.fn(), + }, + ]; + NotificationMock.getHistory.mockResolvedValue(history); + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'darwin'); + service.start(); + service.markRendererReady(); + + await Promise.resolve(); + + const clickHandler = history[0].on.mock.calls[0][1] as () => void; + clickHandler(); + + expect(sendRendererEventMock).toHaveBeenCalledWith(mb, EVENTS.NATIVE_NOTIFICATION_CLICK, { + id: 'history-id', + }); + }); + + it('handles Windows activation through the registered callback', () => { + const { mb } = makeMenubar(); + const service = new NativeNotificationService(mb, 'win32'); + service.start(); + service.markRendererReady(); + service.show(basePayload); + + const activation = NotificationMock.handleActivation.mock.calls[0][0] as (details: { + type: string; + arguments: string; + actionIndex?: number; + }) => void; + + activation({ type: 'action', arguments: basePayload.id, actionIndex: 1 }); + expect(sendRendererEventMock).toHaveBeenCalledWith(mb, EVENTS.NATIVE_NOTIFICATION_ACTION, { + id: basePayload.id, + action: 'read', + }); + + activation({ type: 'click', arguments: 'unknown' }); + expect(sendRendererEventMock).toHaveBeenCalledWith(mb, EVENTS.NATIVE_NOTIFICATION_CLICK, { + id: '', + }); + }); +}); diff --git a/src/main/notifications.ts b/src/main/notifications.ts new file mode 100644 index 000000000..b32958bf3 --- /dev/null +++ b/src/main/notifications.ts @@ -0,0 +1,389 @@ +import { Notification } from 'electron'; +import type { Menubar } from 'electron-menubar'; + +import { + EVENTS, + type INativeNotificationActionEvent, + type INativeNotificationClickEvent, + type INativeNotificationPayload, + type INativeNotificationRemovePayload, + type INativeNotificationShowResult, + type NativeNotificationActionId, +} from '../shared/events'; +import { logError, toError } from '../shared/logger'; + +import { sendRendererEvent } from './events'; + +const ALLOWED_ACTIONS: ReadonlySet = new Set([ + 'show', + 'read', + 'done', + 'unsubscribe', +]); + +const ACTION_LABELS: Record = { + show: 'Show notifications', + read: 'Mark as read', + done: 'Mark as done', + unsubscribe: 'Unsubscribe', +}; + +const MAX_IDENTIFIER_LENGTH = 512; +const MAX_QUEUED_INTERACTIONS = 20; + +type QueuedInteraction = + | { event: 'action'; payload: INativeNotificationActionEvent } + | { event: 'click'; payload: INativeNotificationClickEvent }; + +interface LiveNotification { + notification: Notification; + groupId?: string; + actions: NativeNotificationActionId[]; +} + +/** + * 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 history: Notification[] = []; + private rendererReady = false; + private started = false; + + constructor( + private readonly mb: Menubar, + private readonly platform: NodeJS.Platform = process.platform, + ) {} + + /** Install platform-specific interaction handling. Idempotent. */ + start(): void { + if (this.started) { + return; + } + + this.started = true; + this.restoreDeliveredHistory(); + this.registerWindowsActivation(); + } + + isSupported(): boolean { + return Notification.isSupported(); + } + + show(payload: INativeNotificationPayload): INativeNotificationShowResult { + if (!this.isSupported()) { + return { supported: false }; + } + + if (!validateNativeNotificationPayload(payload)) { + return { supported: true }; + } + + 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, + groupId: payload.groupId, + groupTitle: payload.groupTitle, + title: payload.title, + subtitle, + body, + silent: true, + timeoutType: payload.keepVisible ? 'never' : 'default', + actions: payload.actions.map((action) => ({ + type: 'button', + text: ACTION_LABELS[action], + })), + }); + + notification.on('click', () => { + this.deliver('click', { id: payload.id }); + }); + + notification.on('action', (_event, actionIndex) => { + const action = payload.actions[actionIndex]; + + if (action) { + this.deliver('action', { id: payload.id, action }); + } + }); + + 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, + groupId: payload.groupId, + actions: payload.actions, + }); + notification.show(); + + return { supported: true }; + } + + remove(payload: INativeNotificationRemovePayload): void { + for (const id of payload.ids ?? []) { + this.closeOne(id); + } + + if (payload.groupId) { + this.closeGroup(payload.groupId); + } + } + + removeAll(): void { + for (const entry of this.live.values()) { + entry.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.flushQueue(); + } + + destroy(): void { + this.removeAll(); + + // Release the retained history references so their listeners can be + // collected with the service. + if (this.history.length > 0) { + this.history = []; + } + } + + private closeOne(id: string): void { + const entry = this.live.get(id); + + if (entry) { + entry.notification.close(); + this.live.delete(id); + } + + if (this.platform === 'darwin' && typeof Notification.remove === 'function') { + Notification.remove(id); + } + } + + private closeGroup(groupId: string): void { + for (const [id, entry] of this.live.entries()) { + if (entry.groupId === groupId) { + entry.notification.close(); + this.live.delete(id); + } + } + + if (this.platform === 'darwin' && typeof Notification.removeGroup === 'function') { + Notification.removeGroup(groupId); + } + } + + 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 (!this.rendererReady || !window) { + return false; + } + + return typeof window.isDestroyed !== 'function' || !window.isDestroyed(); + } + + private send( + event: QueuedInteraction['event'], + payload: INativeNotificationActionEvent | INativeNotificationClickEvent, + ): void { + if (event === 'action') { + sendRendererEvent( + this.mb, + EVENTS.NATIVE_NOTIFICATION_ACTION, + payload as INativeNotificationActionEvent, + ); + } else { + sendRendererEvent( + this.mb, + EVENTS.NATIVE_NOTIFICATION_CLICK, + payload as INativeNotificationClickEvent, + ); + } + } + + private flushQueue(): void { + while (this.queue.length > 0 && this.canSendToRenderer()) { + const interaction = this.queue.shift()!; + this.send(interaction.event, interaction.payload); + } + } + + private restoreDeliveredHistory(): void { + if (this.platform !== 'darwin' || typeof Notification.getHistory !== 'function') { + return; + } + + void Notification.getHistory() + .then((history) => { + // Keep the references alive so interactions on restored notifications + // continue to fire. + this.history = history; + + for (const notification of history) { + notification.on('click', () => { + this.deliver('click', { id: notification.id }); + }); + } + }) + .catch((error) => { + logError( + 'native-notifications', + 'Failed to restore delivered notifications', + toError(error), + ); + }); + } + + private registerWindowsActivation(): void { + if (this.platform !== 'win32' || typeof Notification.handleActivation !== 'function') { + return; + } + + Notification.handleActivation((details) => { + const id = parseActivationId(details.arguments); + const entry = id ? this.live.get(id) : undefined; + + if (details.type === 'action' && entry && typeof details.actionIndex === 'number') { + const action = entry.actions[details.actionIndex]; + + if (action) { + this.deliver('action', { id, action }); + return; + } + } + + // A cold start has no live entry. Treat the activation as a click so the + // renderer can resolve the identity after its first fetch, or fall back + // to a generic app activation. + this.deliver('click', { id }); + }); + } +} + +/** 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 (payload.groupId !== undefined && !isValidIdentifier(payload.groupId)) { + 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; + } + + if (payload.groupTitle !== undefined && typeof payload.groupTitle !== 'string') { + return false; + } + + if (payload.url !== undefined && !isHttpUrl(payload.url)) { + return false; + } + + if (!Array.isArray(payload.actions)) { + return false; + } + + return payload.actions.every((action) => ALLOWED_ACTIONS.has(action)); +} + +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; + }) + ); +} + +function isHttpUrl(value: string): boolean { + try { + const url = new URL(value); + return url.protocol === 'http:' || url.protocol === 'https:'; + } catch { + return false; + } +} + +function parseActivationId(argumentsValue: string): string { + const value = argumentsValue?.trim(); + + if (!value) { + return ''; + } + + try { + const parsed: unknown = JSON.parse(value); + + if (typeof parsed === 'string') { + return parsed; + } + + if (parsed && typeof parsed === 'object' && 'id' in parsed) { + const id = (parsed as { id?: unknown }).id; + return typeof id === 'string' ? id : ''; + } + } catch { + // Windows may pass the identity as a raw string rather than JSON. + } + + return value.includes(':') ? value : ''; +} diff --git a/src/preload/index.test.ts b/src/preload/index.test.ts index 954844cc2..b96dc56d3 100644 --- a/src/preload/index.test.ts +++ b/src/preload/index.test.ts @@ -42,36 +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) => Promise; + remove: (payload: unknown) => void; + removeAll: () => void; + notifyReady: () => void; + isSupported: () => Promise; + onAction: (callback: (payload: unknown) => void) => () => void; + onClick: (callback: (payload: unknown) => void) => () => void; + }; } describe('preload/index', () => { beforeEach(() => { - MockNotification.instances = []; exposeInMainWorldMock('gitify', api); }); @@ -143,31 +132,99 @@ describe('preload/index', () => { await expect(api.app.version()).resolves.toBe('v1.2.3'); }); - it('raiseNativeNotification without url calls app.show', async () => { + it('nativeNotifications.show invokes the main process with the payload', async () => { const api = getExposedApi(); + const payload = { + id: 'account:notification', + title: 'Title', + body: 'Body', + actions: ['open'], + }; + + invokeMainEventMock.mockResolvedValueOnce({ supported: true }); + + await expect(api.nativeNotifications.show(payload)).resolves.toEqual({ supported: true }); + expect(invokeMainEventMock).toHaveBeenCalledWith(EVENTS.NATIVE_NOTIFICATION_SHOW, payload); + }); - const notification = api.raiseNativeNotification('Title', 'Body') as MockNotification; + it('nativeNotifications.remove sends the removal payload', () => { + const api = getExposedApi(); - notification.triggerClick(); + api.nativeNotifications.remove({ ids: ['one', 'two'] }); - expect(sendMainEventMock).toHaveBeenCalledWith(EVENTS.WINDOW_SHOW); + 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.isSupported invokes the support query', async () => { + const api = getExposedApi(); + + invokeMainEventMock.mockResolvedValueOnce(false); + + await expect(api.nativeNotifications.isSupported()).resolves.toBe(false); + expect(invokeMainEventMock).toHaveBeenCalledWith(EVENTS.NATIVE_NOTIFICATION_SUPPORTED); + }); + + 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 f760cd767..ff1ceaa3e 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1,6 +1,14 @@ import { contextBridge, webFrame } from 'electron'; -import type { IKeyboardShortcut, NativeThemeSource, TrayIconAppearance } from '../shared/events'; +import type { + IKeyboardShortcut, + INativeNotificationActionEvent, + INativeNotificationClickEvent, + INativeNotificationPayload, + INativeNotificationRemovePayload, + NativeThemeSource, + TrayIconAppearance, +} from '../shared/events'; import { EVENTS } from '../shared/events'; import { isLinux, isMacOS, isWindows } from '../shared/platform'; @@ -255,29 +263,47 @@ export const api = { return onRendererEvent(EVENTS.AUTH_CALLBACK, (_, url) => callback(url)); }, - /** - * 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. + * + * The payload carries display data and whitelisted action IDs only; the + * main process owns validation and construction. + */ + show: (payload: INativeNotificationPayload) => + invokeMainEvent(EVENTS.NATIVE_NOTIFICATION_SHOW, payload), + + /** Remove published notifications by identity or group. */ + remove: (payload: INativeNotificationRemovePayload) => + sendMainEvent(EVENTS.NATIVE_NOTIFICATION_REMOVE, payload), + + /** 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), - return notification; + /** Whether the operating system supports desktop notifications. */ + isSupported: () => invokeMainEvent(EVENTS.NATIVE_NOTIFICATION_SUPPORTED), + + /** + * Register a callback for an activated notification 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. + * + * @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/App.tsx b/src/renderer/App.tsx index 19f4e11ef..067522607 100644 --- a/src/renderer/App.tsx +++ b/src/renderer/App.tsx @@ -53,8 +53,8 @@ export function App() { - + diff --git a/src/renderer/__helpers__/visual.setup.ts b/src/renderer/__helpers__/visual.setup.ts index fddd2c2bd..8efad76a1 100644 --- a/src/renderer/__helpers__/visual.setup.ts +++ b/src/renderer/__helpers__/visual.setup.ts @@ -124,7 +124,15 @@ function createGitifyBridgeApi(): Window['gitify'] { setShowUpdateNotifications: vi.fn(), setUseX11Backend: vi.fn(), applyKeyboardShortcut: vi.fn().mockResolvedValue({ success: true }), - raiseNativeNotification: vi.fn(), + nativeNotifications: { + show: vi.fn().mockResolvedValue({ supported: true }), + remove: vi.fn(), + removeAll: vi.fn(), + notifyReady: vi.fn(), + isSupported: vi.fn().mockResolvedValue(true), + onAction: vi.fn(() => vi.fn()), + onClick: vi.fn(() => vi.fn()), + }, }; } diff --git a/src/renderer/__helpers__/vitest.setup.ts b/src/renderer/__helpers__/vitest.setup.ts index 62f733325..c8e2a50cc 100644 --- a/src/renderer/__helpers__/vitest.setup.ts +++ b/src/renderer/__helpers__/vitest.setup.ts @@ -133,7 +133,15 @@ function createGitifyBridgeApi(): Window['gitify'] { setShowUpdateNotifications: vi.fn(), setUseX11Backend: vi.fn(), applyKeyboardShortcut: vi.fn().mockResolvedValue({ success: true }), - raiseNativeNotification: vi.fn(), + nativeNotifications: { + show: vi.fn().mockResolvedValue({ supported: true }), + remove: vi.fn(), + removeAll: vi.fn(), + notifyReady: vi.fn(), + isSupported: vi.fn().mockResolvedValue(true), + onAction: vi.fn(() => vi.fn()), + onClick: vi.fn(() => vi.fn()), + }, }; } diff --git a/src/renderer/__mocks__/state-mocks.ts b/src/renderer/__mocks__/state-mocks.ts index 5506b49e3..f56e52078 100644 --- a/src/renderer/__mocks__/state-mocks.ts +++ b/src/renderer/__mocks__/state-mocks.ts @@ -6,6 +6,8 @@ import { DesignLanguage, GroupBy, type KeyboardAcceleratorShortcut, + NativeNotificationClickAction, + NativeNotificationStyle, type NotificationSettingsState, OpenPreference, type Percentage, @@ -59,6 +61,10 @@ const mockSystemSettings: SystemSettingsState = { keyboardShortcut: true, openGitifyShortcut: 'CommandOrControl+Shift+G' as KeyboardAcceleratorShortcut, showNotifications: true, + nativeNotificationStyle: NativeNotificationStyle.INDIVIDUAL, + nativeNotificationActions: true, + nativeNotificationKeepVisible: false, + nativeNotificationClickAction: NativeNotificationClickAction.BROWSER, playSound: true, notificationVolume: 20 as Percentage, openAtStartup: false, diff --git a/src/renderer/components/GlobalEffects.test.tsx b/src/renderer/components/GlobalEffects.test.tsx index 9f83546f7..12fec1ec3 100644 --- a/src/renderer/components/GlobalEffects.test.tsx +++ b/src/renderer/components/GlobalEffects.test.tsx @@ -51,6 +51,7 @@ describe('renderer/components/GlobalEffects.tsx', () => { expect(useAccountsStore.getState().accounts).toEqual([]); expect(useSettingsStore.getState().theme).toBe(Theme.SYSTEM); expect(useShortcutRegistrationStore.getState().shortcutRegistrationError).toBeNull(); + expect(window.gitify.nativeNotifications.removeAll).toHaveBeenCalled(); }); describe('persistRotatedAccountTokens (startup)', () => { diff --git a/src/renderer/components/GlobalEffects.tsx b/src/renderer/components/GlobalEffects.tsx index 235096bb4..4e6b44e44 100644 --- a/src/renderer/components/GlobalEffects.tsx +++ b/src/renderer/components/GlobalEffects.tsx @@ -1,4 +1,5 @@ import { useEffect } from 'react'; +import { useNavigate } from 'react-router-dom'; import { useQueryClient } from '@tanstack/react-query'; @@ -10,6 +11,7 @@ import { useShortcutRegistration } from '../hooks/useShortcutRegistration'; import { useAccountsStore, useFiltersStore, + useNotificationFocusStore, useSettingsStore, useShortcutRegistrationStore, } from '../stores'; @@ -25,6 +27,7 @@ import { setTrayIconColorAndTitle } from '../utils/system/tray'; */ export function GlobalEffects() { const queryClient = useQueryClient(); + const navigate = useNavigate(); const resetAccounts = useAccountsStore((s) => s.reset); const resetFilters = useFiltersStore((s) => s.reset); @@ -42,7 +45,9 @@ export function GlobalEffects() { // Notification fetching side effects (sound / native alerts, interval/focus/reconnect // polling, and the system-wake refetch) - const { status, notificationCount } = useNotificationsHost(); + const { status, notificationCount } = useNotificationsHost({ + onReveal: () => navigate('/'), + }); // Periodic account refreshes (startup + hourly interval) useAccounts(); @@ -70,6 +75,8 @@ export function GlobalEffects() { resetFilters(); queryClient.clear(); useShortcutRegistrationStore.getState().reset(); + useNotificationFocusStore.getState().reset(); + window.gitify.nativeNotifications.removeAll(); }); }, [resetAccounts, resetSettings, resetFilters, queryClient]); diff --git a/src/renderer/components/notifications/NotificationList.test.tsx b/src/renderer/components/notifications/NotificationList.test.tsx index 981258c85..30eb4978e 100644 --- a/src/renderer/components/notifications/NotificationList.test.tsx +++ b/src/renderer/components/notifications/NotificationList.test.tsx @@ -1,6 +1,6 @@ -import { screen } from '@testing-library/react'; +import { act, fireEvent, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import type { ReactElement } from 'react'; +import type { ForwardedRef, ReactElement } from 'react'; import { renderWithProviders } from '../../__helpers__/test-utils'; import { @@ -8,28 +8,83 @@ import { mockGitHubEnterpriseServerAccount, } from '../../__mocks__/account-mocks'; import { + mockGitHubCloudGitifyNotifications, mockGithubEnterpriseGitifyNotifications, + mockGitifyNotification, mockMultipleAccountNotifications, + mockSingleAccountNotifications, } from '../../__mocks__/notifications-mocks'; +import { useFiltersStore, useNotificationFocusStore, useSettingsStore } from '../../stores'; + import { GroupBy } from '../../types'; +import { getAccountUUID } from '../../utils/auth/utils'; +import { FOCUS_HIGHLIGHT_MS, FOCUS_VIEW_OFFSET } from '../../utils/notifications/listPositioning'; import { NotificationList } from './NotificationList'; +const legendListMock = vi.hoisted(() => ({ + scrollToIndex: vi.fn(() => Promise.resolve()), + scrollToOffset: vi.fn(() => Promise.resolve()), + autoReady: true, + readyCallback: null as (() => void) | null, + props: [] as Array<{ dataKey?: unknown }>, +})); + // LegendList virtualizes based on real scroll metrics, which happy-dom cannot // provide. Renders every flattened item through the real renderItem callback so -// the component's useMemo flattening still runs and can be asserted on. -vi.mock('@legendapp/list/react', () => ({ - LegendList: ({ - data, - renderItem, - }: { +// the component's useMemo flattening still runs and can be asserted on. The +// ref spies let tests assert programmatic positioning without a real scroller. +vi.mock('@legendapp/list/react', async () => { + const React = await import('react'); + + interface MockLegendListProps { data: unknown[]; + dataKey?: unknown; renderItem: (info: { item: unknown }) => ReactElement; - }) =>
{data.map((item) => renderItem({ item }))}
, -})); + onReady?: () => void; + } + + const LegendList = React.forwardRef( + ({ data, dataKey, renderItem, onReady }, ref: ForwardedRef) => { + legendListMock.props.push({ dataKey }); + + React.useImperativeHandle(ref, () => ({ + scrollToIndex: legendListMock.scrollToIndex, + scrollToOffset: legendListMock.scrollToOffset, + })); + + React.useEffect(() => { + legendListMock.readyCallback = onReady ?? null; + + if (legendListMock.autoReady) { + onReady?.(); + } + }, [onReady]); + + return ( +
+ {data.map((item, index) => ( +
{renderItem({ item })}
+ ))} +
+ ); + }, + ); + + return { LegendList }; +}); describe('renderer/components/notifications/NotificationList.tsx', () => { + beforeEach(() => { + legendListMock.autoReady = true; + legendListMock.readyCallback = null; + legendListMock.props = []; + useNotificationFocusStore.getState().reset(); + useFiltersStore.getState().reset(); + useSettingsStore.getState().reset(); + }); + describe('NotificationList', () => { it('renders account headers, repository headers and notification rows flattened in order', () => { renderWithProviders( @@ -120,4 +175,319 @@ describe('renderer/components/notifications/NotificationList.tsx', () => { expect(screen.getAllByTestId('notification-row')).toHaveLength(2); }); }); + + describe('positioning and focus', () => { + const accountUUID = getAccountUUID(mockGitHubCloudAccount); + + it('focuses a visible notification after the list is ready', async () => { + renderWithProviders( + , + { + accounts: [mockGitHubCloudAccount], + settings: { groupBy: GroupBy.REPOSITORY }, + }, + ); + + act(() => + useNotificationFocusStore.getState().requestFocus(accountUUID, mockGitifyNotification.id), + ); + + await waitFor(() => + expect(legendListMock.scrollToIndex).toHaveBeenCalledWith( + expect.objectContaining({ + index: 1, + viewOffset: FOCUS_VIEW_OFFSET, + viewPosition: 0, + viewPositionFallback: 'start', + }), + ), + ); + expect(document.querySelectorAll('[aria-current="true"]')).toHaveLength(1); + expect(useNotificationFocusStore.getState().intent).toBeNull(); + }); + + it('waits for readiness before focusing', async () => { + legendListMock.autoReady = false; + + renderWithProviders( + , + { + accounts: [mockGitHubCloudAccount], + settings: { groupBy: GroupBy.REPOSITORY }, + }, + ); + + act(() => + useNotificationFocusStore.getState().requestFocus(accountUUID, mockGitifyNotification.id), + ); + + expect(legendListMock.scrollToIndex).not.toHaveBeenCalled(); + + act(() => legendListMock.readyCallback?.()); + + await waitFor(() => expect(legendListMock.scrollToIndex).toHaveBeenCalledTimes(1)); + }); + + it('expands a collapsed repository before focusing', async () => { + const user = userEvent.setup(); + + renderWithProviders( + , + { + accounts: [mockGitHubCloudAccount], + settings: { groupBy: GroupBy.REPOSITORY }, + }, + ); + + await user.click(screen.getByTestId('repository-toggle')); + expect(screen.queryAllByTestId('notification-row')).toHaveLength(0); + + act(() => + useNotificationFocusStore.getState().requestFocus(accountUUID, mockGitifyNotification.id), + ); + + await waitFor(() => expect(screen.getAllByTestId('notification-row')).toHaveLength(1)); + await waitFor(() => expect(legendListMock.scrollToIndex).toHaveBeenCalledTimes(1)); + }); + + it('expands a collapsed account before focusing', async () => { + const user = userEvent.setup(); + + renderWithProviders( + , + { + accounts: [mockGitHubCloudAccount], + settings: { groupBy: GroupBy.REPOSITORY }, + }, + ); + + await user.click(screen.getByTestId('account-toggle')); + expect(screen.queryAllByTestId('notification-row')).toHaveLength(0); + + act(() => + useNotificationFocusStore.getState().requestFocus(accountUUID, mockGitifyNotification.id), + ); + + await waitFor(() => expect(screen.getAllByTestId('notification-row')).toHaveLength(1)); + await waitFor(() => expect(legendListMock.scrollToIndex).toHaveBeenCalledTimes(1)); + }); + + it('clears a filtered-out target without scrolling', async () => { + renderWithProviders( + , + { + accounts: [mockGitHubCloudAccount], + settings: { groupBy: GroupBy.REPOSITORY }, + }, + ); + + act(() => useNotificationFocusStore.getState().requestFocus(accountUUID, 'missing')); + + await waitFor(() => expect(useNotificationFocusStore.getState().intent).toBeNull()); + expect(legendListMock.scrollToIndex).not.toHaveBeenCalled(); + }); + + it('focuses the notification belonging to the requested account when IDs repeat', async () => { + const first = { + ...mockGitHubCloudGitifyNotifications[0], + id: '42', + subject: { ...mockGitHubCloudGitifyNotifications[0].subject, title: 'First account' }, + }; + const second = { + ...mockGitHubCloudGitifyNotifications[0], + id: '42', + account: mockGitHubEnterpriseServerAccount, + subject: { ...mockGitHubCloudGitifyNotifications[0].subject, title: 'Second account' }, + }; + + renderWithProviders( + , + { + accounts: [mockGitHubCloudAccount, mockGitHubEnterpriseServerAccount], + settings: { groupBy: GroupBy.DATE }, + }, + ); + + act(() => + useNotificationFocusStore + .getState() + .requestFocus(getAccountUUID(mockGitHubEnterpriseServerAccount), '42'), + ); + + await waitFor(() => expect(legendListMock.scrollToIndex).toHaveBeenCalledTimes(1)); + + const focused = document.querySelector('[aria-current="true"]'); + expect(focused?.textContent).toContain('Second account'); + }); + + it('clears the highlight when the user interacts with the list', async () => { + renderWithProviders( + , + { + accounts: [mockGitHubCloudAccount], + settings: { groupBy: GroupBy.REPOSITORY }, + }, + ); + + act(() => + useNotificationFocusStore.getState().requestFocus(accountUUID, mockGitifyNotification.id), + ); + await waitFor(() => + expect(document.querySelectorAll('[aria-current="true"]')).toHaveLength(1), + ); + + fireEvent.pointerDown(screen.getByTestId('notification-row')); + + expect(document.querySelectorAll('[aria-current="true"]')).toHaveLength(0); + + act(() => + useNotificationFocusStore.getState().requestFocus(accountUUID, mockGitifyNotification.id), + ); + await waitFor(() => + expect(document.querySelectorAll('[aria-current="true"]')).toHaveLength(1), + ); + + fireEvent.keyDown(screen.getByTestId('notification-row'), { key: 'ArrowDown' }); + + expect(document.querySelectorAll('[aria-current="true"]')).toHaveLength(0); + }); + + it('focuses the newest request and clears the previous highlight', async () => { + const first = { + ...mockGitHubCloudGitifyNotifications[0], + id: 'first', + order: 0, + subject: { ...mockGitHubCloudGitifyNotifications[0].subject, title: 'First title' }, + }; + const second = { + ...first, + id: 'second', + order: 1, + subject: { ...first.subject, title: 'Second title' }, + }; + + renderWithProviders( + , + { + accounts: [mockGitHubCloudAccount], + settings: { groupBy: GroupBy.DATE }, + }, + ); + + act(() => useNotificationFocusStore.getState().requestFocus(accountUUID, 'first')); + await waitFor(() => + expect(document.querySelectorAll('[aria-current="true"]')).toHaveLength(1), + ); + expect(document.querySelector('[aria-current="true"]')?.textContent).toContain('First title'); + + act(() => useNotificationFocusStore.getState().requestFocus(accountUUID, 'second')); + await waitFor(() => + expect(document.querySelector('[aria-current="true"]')?.textContent).toContain( + 'Second title', + ), + ); + expect(document.querySelectorAll('[aria-current="true"]')).toHaveLength(1); + }); + + it('clears the highlight after its display duration', () => { + vi.useFakeTimers(); + + try { + renderWithProviders( + , + { + accounts: [mockGitHubCloudAccount], + settings: { groupBy: GroupBy.REPOSITORY }, + }, + ); + + act(() => + useNotificationFocusStore.getState().requestFocus(accountUUID, mockGitifyNotification.id), + ); + expect(document.querySelectorAll('[aria-current="true"]')).toHaveLength(1); + + act(() => { + vi.advanceTimersByTime(FOCUS_HIGHLIGHT_MS); + }); + + expect(document.querySelectorAll('[aria-current="true"]')).toHaveLength(0); + } finally { + vi.useRealTimers(); + } + }); + + it('keeps the data key stable across content updates and changes it for the dataset', async () => { + const tree = renderWithProviders( + , + { + accounts: [mockGitHubCloudAccount], + settings: { groupBy: GroupBy.REPOSITORY }, + }, + ); + + const initialKey = legendListMock.props.at(-1)?.dataKey; + const updatedNotifications = [ + { + ...mockSingleAccountNotifications[0], + notifications: [ + { + ...mockGitifyNotification, + subject: { ...mockGitifyNotification.subject, title: 'Updated title' }, + }, + ], + }, + ]; + + tree.rerender( + , + ); + + expect(legendListMock.props.at(-1)?.dataKey).toBe(initialKey); + + act(() => useSettingsStore.getState().updateSetting('groupBy', GroupBy.DATE)); + + await waitFor(() => expect(legendListMock.props.at(-1)?.dataKey).not.toBe(initialKey)); + + const groupedKey = legendListMock.props.at(-1)?.dataKey; + + act(() => useFiltersStore.getState().updateFilter('reasons', 'mention', true)); + + await waitFor(() => expect(legendListMock.props.at(-1)?.dataKey).not.toBe(groupedKey)); + }); + }); }); diff --git a/src/renderer/components/notifications/NotificationList.tsx b/src/renderer/components/notifications/NotificationList.tsx index 005f05dd0..16c17e999 100644 --- a/src/renderer/components/notifications/NotificationList.tsx +++ b/src/renderer/components/notifications/NotificationList.tsx @@ -1,8 +1,9 @@ -import { useCallback, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; -import { LegendList } from '@legendapp/list/react'; +import { LegendList, type LegendListRef } from '@legendapp/list/react'; -import { useAccountsStore, useSettingsStore } from '../../stores'; +import { useNotificationListDataKey } from '../../hooks/notifications/useNotificationListDataKey'; +import { useAccountsStore, useNotificationFocusStore, useSettingsStore } from '../../stores'; import { type AccountNotifications, type GitifyNotification } from '../../types'; @@ -12,7 +13,14 @@ import { type BuildItemsContext, getItemKey, getItemType, + getNotificationItemKey, } from '../../utils/notifications/listItems'; +import { + FOCUS_HIGHLIGHT_MS, + FOCUS_VIEW_OFFSET, + getStickyHeaderIndices, + resolveNotificationFocus, +} from '../../utils/notifications/listPositioning'; import { AllRead } from '../AllRead'; import { Oops } from '../Oops'; import { AccountHeader } from './AccountHeader'; @@ -33,7 +41,29 @@ export function NotificationList({ showAccountHeader, }: Readonly) { const groupBy = useSettingsStore((s) => s.groupBy); + const accounts = useAccountsStore((s) => s.accounts); const hasMultipleAccounts = useAccountsStore((s) => s.hasMultipleAccounts()); + const dataKey = useNotificationListDataKey({ groupBy, accounts }); + + const focusIntent = useNotificationFocusStore((s) => s.intent); + const clearFocus = useNotificationFocusStore((s) => s.clearFocus); + + const listRef = useRef(null); + const highlightTimeoutRef = useRef | null>(null); + + const [isReady, setIsReady] = useState(false); + const [focusedKey, setFocusedKey] = useState(null); + + // Adjust state during render so the list receives the new dataset identity + // and the readiness gate in the same commit, before its first placement. + const [previousDataKey, setPreviousDataKey] = useState(dataKey); + const [pendingDatasetReset, setPendingDatasetReset] = useState(false); + + if (previousDataKey !== dataKey) { + setPreviousDataKey(dataKey); + setPendingDatasetReset(true); + setIsReady(false); + } const [animatingNotifications, setAnimatingNotifications] = useState>( new Set(), @@ -129,14 +159,123 @@ export function NotificationList({ showAccountHeader, ]); + const stickyHeaderIndices = useMemo(() => getStickyHeaderIndices(items), [items]); + const stickyHeaderConfig = useMemo(() => ({ offset: 0 }), []); + const maintainVisibleContentPosition = useMemo( + () => ({ + data: true, + shouldRestorePosition: () => !pendingDatasetReset, + }), + [pendingDatasetReset], + ); + + const clearHighlight = useCallback(() => { + if (highlightTimeoutRef.current) { + clearTimeout(highlightTimeoutRef.current); + highlightTimeoutRef.current = null; + } + + setFocusedKey(null); + }, []); + + const handleReady = useCallback(() => { + if (pendingDatasetReset) { + setPendingDatasetReset(false); + void listRef.current?.scrollToOffset({ offset: 0, animated: false }); + } + + setIsReady(true); + }, [pendingDatasetReset]); + + useEffect(() => { + return () => { + if (highlightTimeoutRef.current) { + clearTimeout(highlightTimeoutRef.current); + } + }; + }, []); + + useEffect(() => { + if (!focusIntent || !isReady) { + return; + } + + const resolution = resolveNotificationFocus({ + accountNotifications, + items, + accountUUID: focusIntent.accountUUID, + notificationId: focusIntent.notificationId, + collapsedAccounts, + collapsedRepositories, + }); + + if (resolution.status === 'filtered-out' || resolution.status === 'missing') { + clearFocus(focusIntent.requestId); + return; + } + + if (resolution.status === 'collapsed-account') { + // oxlint-disable-next-line react/set-state-in-effect -- A focus request is an external event; expanding its group is the response. + toggleCollapsedAccount(focusIntent.accountUUID); + return; + } + + if (resolution.status === 'collapsed-repository') { + // oxlint-disable-next-line react/set-state-in-effect -- A focus request is an external event; expanding its group is the response. + toggleCollapsedRepository(resolution.repoKey); + return; + } + + const key = getNotificationItemKey(focusIntent.accountUUID, focusIntent.notificationId); + + void listRef.current + ?.scrollToIndex({ + index: resolution.index, + viewPosition: 0, + viewOffset: FOCUS_VIEW_OFFSET, + viewPositionFallback: 'start', + animated: true, + }) + .catch(() => { + // The list may unmount while the scroll is in flight; the intent is + // still consumed so a later data update does not replay it. + }); + + if (highlightTimeoutRef.current) { + clearTimeout(highlightTimeoutRef.current); + } + + setFocusedKey(key); + highlightTimeoutRef.current = setTimeout(() => setFocusedKey(null), FOCUS_HIGHLIGHT_MS); + clearFocus(focusIntent.requestId); + }, [ + accountNotifications, + clearFocus, + collapsedAccounts, + collapsedRepositories, + focusIntent, + isReady, + items, + toggleCollapsedAccount, + toggleCollapsedRepository, + ]); + return ( -
+
( <> {item.kind === 'account' && ( @@ -169,6 +308,7 @@ export function NotificationList({ {item.kind === 'notification' && ( setNotificationAnimatingExit( `${getAccountUUID(item.notification.account)}:${item.notification.id}`, @@ -180,6 +320,8 @@ export function NotificationList({ )} )} + stickyHeaderConfig={stickyHeaderConfig} + stickyHeaderIndices={stickyHeaderIndices} style={{ height: '100%', overflowX: 'hidden' }} />
diff --git a/src/renderer/components/notifications/NotificationList.visual.test.tsx b/src/renderer/components/notifications/NotificationList.visual.test.tsx index 20d63cfc8..65a0bcf98 100644 --- a/src/renderer/components/notifications/NotificationList.visual.test.tsx +++ b/src/renderer/components/notifications/NotificationList.visual.test.tsx @@ -14,6 +14,7 @@ import { import { mockGitifyNotification } from '../../__mocks__/notifications-mocks'; import { NotificationsRoute } from '../../routes/Notifications'; +import { useNotificationFocusStore, useSettingsStore } from '../../stores'; import { type AccountNotifications, @@ -23,6 +24,8 @@ import { Theme, } from '../../types'; +import { getAccountUUID } from '../../utils/auth/utils'; + const account = mockGitHubCloudAccount; const fixture = (count: number) => Array.from({ length: count }, (_, index) => ({ @@ -78,7 +81,45 @@ async function scrollToStart() { }); } +async function waitForRowBelowHeader(scroller: HTMLElement, excludeId?: string) { + return waitFor(() => { + const target = Array.from( + document.querySelectorAll('[data-testid="notification-row"]'), + ) + .map((row) => row.closest('[id]')) + .find( + (row): row is HTMLElement => + row !== null && + row.id !== excludeId && + row.getBoundingClientRect().top > scroller.getBoundingClientRect().top + 60, + ); + + expect(target).not.toBeUndefined(); + + return target!; + }); +} + +function headersPinnedToTop(testId: string) { + const containerTop = scrollContainer().getBoundingClientRect().top; + + return Array.from(document.querySelectorAll(`[data-testid="${testId}"]`)).filter( + (button) => { + const header = button.parentElement; + if (!header) { + return false; + } + + return Math.abs(header.getBoundingClientRect().top - containerTop) <= 2; + }, + ); +} + describe('notification list in a browser', () => { + beforeEach(() => { + useNotificationFocusStore.getState().reset(); + }); + it.each([GroupBy.DATE, GroupBy.REPOSITORY])( 'renders grouping %s and reaches every row in a 2000-notification inbox', async (groupBy) => { @@ -105,14 +146,30 @@ describe('notification list in a browser', () => { await renderList(fixture(300)); await page.getByTestId('repository-toggle').first().click(); await waitFor(() => expect(document.getElementById('notification-0')).toBeNull()); - await scrollToEnd(); - await waitFor(() => expect(document.getElementById('notification-299')).not.toBeNull()); - await scrollToStart(); - await waitFor(() => - expect(document.querySelector('[data-testid="repository-toggle"]')).not.toBeNull(), + await waitFor( + async () => { + await scrollToEnd(); + expect(document.getElementById('notification-299')).not.toBeNull(); + }, + { timeout: 5000 }, ); + await scrollToStart(); + + const repositoryToggle = await waitFor(() => { + const toggle = document + .querySelector('img[alt="owner/repo-0"]') + ?.closest('.group') + ?.querySelector('[data-testid="repository-toggle"]'); + + expect(toggle).not.toBeUndefined(); + + return toggle!; + }); + expect(document.getElementById('notification-0')).toBeNull(); - await page.getByTestId('repository-toggle').first().click(); + await act(async () => { + repositoryToggle.click(); + }); await waitFor(() => expect(document.getElementById('notification-0')).not.toBeNull()); }); @@ -320,4 +377,194 @@ describe('notification list in a browser', () => { false, ); }); + + it('keeps the visible row anchored when a notification is prepended', async () => { + const tree = await renderList(fixture(300)); + const scroller = scrollContainer(); + + await act(async () => { + scroller.scrollTop = 1500; + }); + + const target = await waitForRowBelowHeader(scroller); + + const id = target.id; + const top = target.getBoundingClientRect().top; + const original = getMockedNotificationsState().notifications; + const added = { ...original[0].notifications[0], id: 'new-anchor', order: -1 }; + + await act(async () => { + setNotificationsOverrides({ + notifications: [{ ...original[0], notifications: [added, ...original[0].notifications] }], + }); + tree.rerender(); + }); + + await waitFor(() => expect(document.getElementById(id)).not.toBeNull()); + expect(Math.abs(document.getElementById(id)!.getBoundingClientRect().top - top)).toBeLessThan( + 2, + ); + }); + + it('keeps the visible row anchored when a notification above it is removed', async () => { + const tree = await renderList(fixture(300)); + const scroller = scrollContainer(); + + await act(async () => { + scroller.scrollTop = 1500; + }); + + const target = await waitForRowBelowHeader(scroller, 'notification-0'); + + const id = target.id; + const top = target.getBoundingClientRect().top; + const original = getMockedNotificationsState().notifications; + + await act(async () => { + setNotificationsOverrides({ + notifications: original.map((entry) => ({ + ...entry, + notifications: entry.notifications.filter( + (notification) => notification.id !== 'notification-0', + ), + })), + }); + tree.rerender(); + }); + + await waitFor(() => expect(document.getElementById(id)).not.toBeNull()); + expect(Math.abs(document.getElementById(id)!.getBoundingClientRect().top - top)).toBeLessThan( + 2, + ); + }); + + it('resets to the top when the grouping dataset changes', async () => { + await renderList(fixture(300)); + + await act(async () => { + scrollContainer().scrollTop = 1500; + }); + + await waitFor(() => expect(scrollContainer().scrollTop).toBeGreaterThan(0)); + + act(() => useSettingsStore.getState().updateSetting('groupBy', GroupBy.DATE)); + + await waitFor(() => expect(scrollContainer().scrollTop).toBe(0)); + }); + + it('keeps exactly one repository header pinned while scrolling', async () => { + await renderList(fixture(300)); + + await scrollToEnd(); + await waitFor(() => expect(headersPinnedToTop('open-repository')).toHaveLength(1)); + + await scrollToStart(); + await waitFor(() => expect(headersPinnedToTop('open-repository')).toHaveLength(1)); + }); + + it('keeps an account header pinned when repository grouping is off', async () => { + const second = fixture(300).map((notification) => ({ + ...notification, + id: `second-${notification.id}`, + account: mockGitHubEnterpriseServerAccount, + })); + + await renderList([], { groupBy: GroupBy.DATE, showAccountHeader: true }, [ + { account, notifications: fixture(2), error: null }, + { account: mockGitHubEnterpriseServerAccount, notifications: second, error: null }, + ]); + + await scrollToEnd(); + await waitFor(() => expect(headersPinnedToTop('account-profile')).toHaveLength(1)); + }); + + it('keeps an error or all-read account header pinned', async () => { + const accounts = Array.from({ length: 20 }, (_, index) => ({ + ...mockGitHubCloudAccount, + user: { + ...mockGitHubCloudAccount.user!, + id: `${1000 + index}`, + login: `user-${index}`, + }, + })); + const entries: AccountNotifications[] = accounts.map((entry, index) => ({ + account: entry, + notifications: [], + error: + index % 2 === 0 + ? { + title: `Account failed ${index}`, + descriptions: ['Try again'], + emojis: ['๐Ÿ”ฅ'], + } + : null, + })); + + await renderList([], { groupBy: GroupBy.DATE, showAccountHeader: true }, entries); + + await scrollToEnd(); + await waitFor(() => expect(headersPinnedToTop('account-profile')).toHaveLength(1)); + }); + + it('focuses a notification below the sticky header', async () => { + await renderList(fixture(300)); + + act(() => + useNotificationFocusStore + .getState() + .requestFocus(getAccountUUID(account), 'notification-150'), + ); + + await waitFor(() => expect(document.querySelector('[aria-current="true"]')).not.toBeNull()); + + const focused = document.querySelector('[aria-current="true"]')!; + + await waitFor( + () => { + const containerTop = scrollContainer().getBoundingClientRect().top; + const top = focused.getBoundingClientRect().top; + + expect(top).toBeGreaterThan(containerTop + 30); + expect(top).toBeLessThan(containerTop + 100); + }, + { timeout: 5000 }, + ); + }); + + it('aligns the start of a focused row taller than the viewport', async () => { + const tall = fixture(300).map((notification, index) => + index === 150 + ? { + ...notification, + subject: { + ...notification.subject, + title: 'A very long notification title '.repeat(120), + }, + } + : notification, + ); + + await renderList(tall, { wrapNotificationTitle: true }); + + act(() => + useNotificationFocusStore + .getState() + .requestFocus(getAccountUUID(account), 'notification-150'), + ); + + await waitFor(() => expect(document.querySelector('[aria-current="true"]')).not.toBeNull()); + + const focused = document.querySelector('[aria-current="true"]')!; + + await waitFor( + () => { + const containerTop = scrollContainer().getBoundingClientRect().top; + const top = focused.getBoundingClientRect().top; + + expect(top).toBeGreaterThan(containerTop + 30); + expect(top).toBeLessThan(containerTop + 120); + }, + { timeout: 5000 }, + ); + }); }); diff --git a/src/renderer/components/notifications/NotificationRow.tsx b/src/renderer/components/notifications/NotificationRow.tsx index a235f2d3d..b1daec6d4 100644 --- a/src/renderer/components/notifications/NotificationRow.tsx +++ b/src/renderer/components/notifications/NotificationRow.tsx @@ -29,12 +29,14 @@ import { NotificationTitle } from './NotificationTitle'; export interface NotificationRowProps { notification: GitifyNotification; isAnimatingExit: boolean; + isFocused?: boolean; onAnimateExit: (animate: boolean) => void; } export function NotificationRow({ notification, isAnimatingExit, + isFocused = false, onAnimateExit, }: Readonly) { const { markNotificationsAsRead, markNotificationsAsDone, unsubscribeNotification } = @@ -90,13 +92,16 @@ export function NotificationRow({ return (
diff --git a/src/renderer/components/settings/AlertSettings.test.tsx b/src/renderer/components/settings/AlertSettings.test.tsx index 760d0ce53..e462d1efc 100644 --- a/src/renderer/components/settings/AlertSettings.test.tsx +++ b/src/renderer/components/settings/AlertSettings.test.tsx @@ -1,4 +1,4 @@ -import { screen } from '@testing-library/react'; +import { act, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { renderWithProviders } from '../../__helpers__/test-utils'; @@ -19,12 +19,58 @@ describe('renderer/components/settings/AlertSettings.tsx', () => { updateSettingSpy = vi.spyOn(useSettingsStore.getState(), 'updateSetting'); }); + it('should describe the style options as individual or summary', async () => { + await act(async () => { + renderWithProviders(); + }); + + expect(screen.getByText('Individual')).toBeInTheDocument(); + expect(screen.getByText('Summary')).toBeInTheDocument(); + expect(screen.queryByText('Grouped')).not.toBeInTheDocument(); + }); + + it('should change the native notification style', async () => { + await act(async () => { + renderWithProviders(); + }); + + await userEvent.click(screen.getByTestId('choice-nativeNotificationStyle-summary')); + + expect(updateSettingSpy).toHaveBeenCalledWith('nativeNotificationStyle', 'summary'); + }); + + it('should change the native notification click behavior', async () => { + await act(async () => { + renderWithProviders(); + }); + + await userEvent.click(screen.getByTestId('choice-nativeNotificationClickAction-app')); + + expect(updateSettingSpy).toHaveBeenCalledWith('nativeNotificationClickAction', 'app'); + }); + + it('should explain when native notifications are unsupported', async () => { + vi.mocked(window.gitify.nativeNotifications.isSupported).mockResolvedValueOnce(false); + + await act(async () => { + renderWithProviders(); + }); + + await waitFor(() => + expect(screen.getByTestId('native-notifications-unsupported')).toBeInTheDocument(), + ); + }); + it.each([ ['Show desktop notifications', 'showNotifications'], + ['Show notification actions', 'nativeNotificationActions'], + ['Keep notifications on screen', 'nativeNotificationKeepVisible'], ['Play notification sound', 'playSound'], ['Show app update notifications', 'showUpdateNotifications'], ] as const)('should toggle %s switch', async (label, setting) => { - renderWithProviders(); + await act(async () => { + renderWithProviders(); + }); await userEvent.click(screen.getByRole('button', { name: label })); @@ -32,63 +78,72 @@ describe('renderer/components/settings/AlertSettings.tsx', () => { expect(toggleSettingSpy).toHaveBeenCalledWith(setting); }); - it('shows detailed help in tooltips', async () => { - renderWithProviders(); + describe('playSound', () => { + it('shows the sound details in a tooltip', async () => { + renderWithProviders(); - await userEvent.click(screen.getByTestId('tooltip-icon-tooltip-playSound')); + await userEvent.click(screen.getByRole('button', { name: 'tooltip-playSound' })); - expect( - screen.getByText( - 'Play a sound when new unread notifications arrive after filtering. Use the notification volume controls to adjust the sound level.', - ), - ).toBeInTheDocument(); - }); + expect( + screen.getByText( + 'Play a sound when new unread notifications arrive after filtering. Use the adjacent volume controls to adjust the sound level.', + ), + ).toBeInTheDocument(); + }); - it('shows notification volume and its tooltip when sound is enabled', async () => { - renderWithProviders(); + it('shows the notification volume label and tooltip', async () => { + renderWithProviders(); - expect(screen.getByText('Notification volume')).toBeInTheDocument(); - expect(screen.getByTestId('reveal-volume')).toHaveAttribute('data-state', 'open'); + expect(screen.getByText('Notification volume')).toBeInTheDocument(); - await userEvent.click(screen.getByTestId('tooltip-icon-tooltip-notificationVolume')); + await userEvent.click(screen.getByTestId('tooltip-icon-tooltip-notificationVolume')); - expect(screen.getByText('Adjust the volume of the notification sound.')).toBeInTheDocument(); - }); + expect(screen.getByText('Adjust the volume of the notification sound.')).toBeInTheDocument(); + }); - it('hides volume controls when playSound is false', () => { - renderWithProviders(, { - settings: { ...mockSettings, playSound: false }, + it('volume controls should not be shown if playSound checkbox is false', async () => { + renderWithProviders(, { + settings: { ...mockSettings, playSound: false }, + }); + + expect(screen.getByTestId('reveal-volume')).toHaveAttribute('data-state', 'closed'); }); - expect(screen.getByTestId('reveal-volume')).toHaveAttribute('data-state', 'closed'); - }); + it('volume controls should be shown if playSound checkbox is true', async () => { + renderWithProviders(, { + settings: { ...mockSettings, playSound: true }, + }); - it('should increase notification volume', async () => { - renderWithProviders(); + expect(screen.getByTestId('reveal-volume')).toHaveAttribute('data-state', 'open'); + }); - await userEvent.click(screen.getByTestId('settings-volume-up')); + it('should increase notification volume', async () => { + renderWithProviders(); - expect(updateSettingSpy).toHaveBeenCalledTimes(1); - expect(updateSettingSpy).toHaveBeenCalledWith('notificationVolume', 30); - }); + await userEvent.click(screen.getByTestId('settings-volume-up')); - it('should decrease notification volume', async () => { - renderWithProviders(); + expect(updateSettingSpy).toHaveBeenCalledTimes(1); + expect(updateSettingSpy).toHaveBeenCalledWith('notificationVolume', 30); + }); - await userEvent.click(screen.getByTestId('settings-volume-down')); + it('should decrease notification volume', async () => { + renderWithProviders(); - expect(updateSettingSpy).toHaveBeenCalledTimes(1); - expect(updateSettingSpy).toHaveBeenCalledWith('notificationVolume', 10); - }); + await userEvent.click(screen.getByTestId('settings-volume-down')); - it('should reset notification volume', async () => { - renderWithProviders(, { - settings: { ...mockSettings, notificationVolume: 30 as Percentage }, + expect(updateSettingSpy).toHaveBeenCalledTimes(1); + expect(updateSettingSpy).toHaveBeenCalledWith('notificationVolume', 10); }); - await userEvent.click(screen.getByTestId('settings-volume-reset')); + it('should reset notification volume', async () => { + renderWithProviders(, { + settings: { ...mockSettings, notificationVolume: 30 as Percentage }, + }); - expect(updateSettingSpy).toHaveBeenCalledTimes(1); - expect(updateSettingSpy).toHaveBeenCalledWith('notificationVolume', 20); + await userEvent.click(screen.getByTestId('settings-volume-reset')); + + expect(updateSettingSpy).toHaveBeenCalledTimes(1); + expect(updateSettingSpy).toHaveBeenCalledWith('notificationVolume', 20); + }); }); }); diff --git a/src/renderer/components/settings/AlertSettings.tsx b/src/renderer/components/settings/AlertSettings.tsx index 4b725b10d..e41342bc4 100644 --- a/src/renderer/components/settings/AlertSettings.tsx +++ b/src/renderer/components/settings/AlertSettings.tsx @@ -1,12 +1,14 @@ -import { BellIcon, SyncIcon } from '@primer/octicons-react'; -import { Button, ButtonGroup, IconButton } from '@primer/react'; +import { useEffect, useState } from 'react'; -import { APPLICATION } from '../../../shared/constants'; +import { BellIcon, SyncIcon } from '@primer/octicons-react'; +import { Banner, Button, ButtonGroup, IconButton, Stack, Text } from '@primer/react'; import { DEFAULT_SETTINGS_STATE, useSettingsStore } from '../../stores'; import { SectionHeading } from '../primitives/SectionHeading'; +import { NativeNotificationClickAction, NativeNotificationStyle } from '../../types'; + import { VolumeDownIcon } from '../../icons/VolumeDownIcon'; import { VolumeUpIcon } from '../../icons/VolumeUpIcon'; import { @@ -15,6 +17,7 @@ import { decreaseVolume, increaseVolume, } from '../../utils/ui/volume'; +import { SettingsChoice } from './controls/SettingsChoice'; import { SettingsList } from './controls/SettingsList'; import { SettingsReveal } from './controls/SettingsReveal'; import { SettingsRow } from './controls/SettingsRow'; @@ -28,9 +31,36 @@ export function AlertSettings() { const showNotifications = useSettingsStore((s) => s.showNotifications); const showUpdateNotifications = useSettingsStore((s) => s.showUpdateNotifications); + const nativeNotificationStyle = useSettingsStore((s) => s.nativeNotificationStyle); + const nativeNotificationActions = useSettingsStore((s) => s.nativeNotificationActions); + const nativeNotificationKeepVisible = useSettingsStore((s) => s.nativeNotificationKeepVisible); + const nativeNotificationClickAction = useSettingsStore((s) => s.nativeNotificationClickAction); const playSound = useSettingsStore((s) => s.playSound); const notificationVolume = useSettingsStore((s) => s.notificationVolume); + const [nativeNotificationsSupported, setNativeNotificationsSupported] = useState(true); + + useEffect(() => { + let active = true; + + void window.gitify.nativeNotifications + .isSupported() + .then((supported) => { + if (active) { + setNativeNotificationsSupported(supported); + } + }) + .catch(() => { + if (active) { + setNativeNotificationsSupported(false); + } + }); + + return () => { + active = false; + }; + }, []); + return (
@@ -46,12 +76,91 @@ export function AlertSettings() { tooltip="Display native operating system notifications for new unread notifications." /> + {!nativeNotificationsSupported && ( + + )} + + + updateSetting('nativeNotificationStyle', value as NativeNotificationStyle) + } + options={[ + { label: 'Individual', value: NativeNotificationStyle.INDIVIDUAL }, + { label: 'Summary', value: NativeNotificationStyle.SUMMARY }, + ]} + tooltip={ + + Choose how notifications from a new batch are delivered. + + Individual shows up to five separate notifications, grouped + by repository where the platform supports it. A larger batch becomes one summary. + + + Summary always shows one notification with the total count. + + + } + value={nativeNotificationStyle} + /> + + toggleSetting('nativeNotificationActions')} + tooltip="Offer Mark as read, Mark as done, and Unsubscribe actions where the platform and account support them. Clicking a notification opens it." + /> + + toggleSetting('nativeNotificationKeepVisible')} + tooltip="Keep notifications visible until dismissed, where the operating system supports it." + /> + + + updateSetting('nativeNotificationClickAction', value as NativeNotificationClickAction) + } + options={[ + { label: 'Gitify', value: NativeNotificationClickAction.APP }, + { label: 'Browser', value: NativeNotificationClickAction.BROWSER }, + ]} + tooltip={ + + Choose what clicking a notification body opens. + + Gitify reveals the matching notification in the app. + + + Browser opens the notification target in your default + browser. + + + Browser focus is controlled by Link focus in the System + section. + + + } + value={nativeNotificationClickAction} + /> + toggleSetting('playSound')} - tooltip="Play a sound when new unread notifications arrive after filtering. Use the notification volume controls to adjust the sound level." + tooltip="Play a sound when new unread notifications arrive after filtering. Use the adjacent volume controls to adjust the sound level." /> @@ -114,7 +223,7 @@ export function AlertSettings() { label="Show app update notifications" name="showUpdateNotifications" onChange={() => toggleSetting('showUpdateNotifications')} - tooltip={`Show a notification when a ${APPLICATION.NAME} update is ready. Updates are still checked and shown in the menu bar.`} + tooltip="Show a notification when a Gitify update is ready. Updates are still checked and shown in the menu bar." />
diff --git a/src/renderer/hooks/notifications/useNotificationEffects.ts b/src/renderer/hooks/notifications/useNotificationEffects.ts index 4a629f61c..ef4885d1f 100644 --- a/src/renderer/hooks/notifications/useNotificationEffects.ts +++ b/src/renderer/hooks/notifications/useNotificationEffects.ts @@ -1,24 +1,50 @@ -import { useEffect, useRef } from 'react'; +import { useCallback, useEffect, useRef } from 'react'; import { focusManager, type QueryKey, useQueryClient } from '@tanstack/react-query'; +import type { + INativeNotificationActionEvent, + INativeNotificationClickEvent, + NativeNotificationActionId, +} from '../../../shared/events'; +import { + decodeNativeNotificationIdentity, + encodeNativeNotificationIdentity, +} from '../../../shared/nativeNotifications'; + import { getNotificationFailureKey, useNotificationActionFailuresStore, + useNotificationFocusStore, useSettingsStore, } from '../../stores'; -import type { AccountNotifications, GitifyNotification } from '../../types'; +import type { AccountNotifications, GitifyNotification, Link } from '../../types'; +import { NativeNotificationClickAction } from '../../types'; +import { getAccountUUID } from '../../utils/auth/utils'; import { doesAllAccountsHaveErrors } from '../../utils/core/errors'; +import { rendererLogError, toError } from '../../utils/core/logger'; import { filterBaseNotifications, filterDetailedNotifications, } from '../../utils/notifications/filters/filter'; +import { findNotificationFocusTarget } from '../../utils/notifications/listPositioning'; import { refreshEnrichmentCache } from '../../utils/notifications/notifications'; import { getNewNotifications } from '../../utils/notifications/utils'; import { raiseSoundNotification } from '../../utils/system/audio'; +import { openExternalLink } from '../../utils/system/comms'; +import { openNotification } from '../../utils/system/links'; import { raiseNativeNotification } from '../../utils/system/native'; +import type { NotificationActions } from './useNotificationActions'; + +const PENDING_INTERACTION_TTL_MS = 30_000; +const MAX_PENDING_INTERACTIONS = 20; + +interface PendingInteraction { + payload: INativeNotificationActionEvent; + expiresAt: number; +} export interface NotificationEffectsInput { /** @@ -32,12 +58,15 @@ export interface NotificationEffectsInput { notificationsQueryKey: QueryKey; refetch: () => void; fetchIntervalMs: number; + actions: NotificationActions; + /** Navigate to the notifications route when a notification is revealed. */ + onReveal: () => void; } /** * 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,13 +79,166 @@ export const useNotificationEffects = ({ notificationsQueryKey, refetch, fetchIntervalMs, + actions, + onReveal, }: NotificationEffectsInput): void => { const queryClient = useQueryClient(); const previousNotificationsRef = useRef([]); + const actionsRef = useRef(actions); + const onRevealRef = useRef(onReveal); + const publishedIdsRef = useRef>(new Set()); + const publishedUrlsRef = useRef>(new Map()); + const pendingInteractionsRef = useRef([]); + + useEffect(() => { + actionsRef.current = actions; + onRevealRef.current = onReveal; + }, [actions, onReveal]); const playSoundNewNotifications = useSettingsStore((s) => s.playSound); const showSystemNotifications = useSettingsStore((s) => s.showNotifications); const notificationVolume = useSettingsStore((s) => s.notificationVolume); + const nativeNotificationStyle = useSettingsStore((s) => s.nativeNotificationStyle); + const nativeNotificationActions = useSettingsStore((s) => s.nativeNotificationActions); + const nativeNotificationKeepVisible = useSettingsStore((s) => s.nativeNotificationKeepVisible); + + const readUnfilteredNotifications = useCallback( + () => queryClient.getQueryData(notificationsQueryKey) ?? [], + [queryClient, notificationsQueryKey], + ); + + const resolveNotification = useCallback( + (id: string) => { + const decoded = decodeNativeNotificationIdentity(id); + + if (!decoded) { + return null; + } + + const notification = findNotificationFocusTarget( + readUnfilteredNotifications(), + decoded.accountUUID, + decoded.notificationId, + ); + + return { decoded, notification }; + }, + [readUnfilteredNotifications], + ); + + const executeAction = useCallback( + (action: NativeNotificationActionId, notification: GitifyNotification) => { + switch (action) { + case 'show': + return; + case 'read': + void actionsRef.current.markNotificationsAsRead([notification]); + return; + case 'done': + void actionsRef.current.markNotificationsAsDone([notification]); + return; + case 'unsubscribe': + void actionsRef.current.unsubscribeNotification(notification); + return; + default: + return; + } + }, + [], + ); + + const handleNativeClick = useCallback( + (payload: INativeNotificationClickEvent) => { + const resolved = payload.id ? resolveNotification(payload.id) : null; + + if (!resolved) { + window.gitify.app.show(); + onRevealRef.current(); + return; + } + + if ( + useSettingsStore.getState().nativeNotificationClickAction === + NativeNotificationClickAction.BROWSER + ) { + if (resolved.notification) { + void openNotification(resolved.notification); + return; + } + + const fallbackUrl = publishedUrlsRef.current.get(payload.id); + + if (fallbackUrl) { + openExternalLink(fallbackUrl as Link); + return; + } + + window.gitify.app.show(); + onRevealRef.current(); + return; + } + + window.gitify.app.show(); + onRevealRef.current(); + useNotificationFocusStore + .getState() + .requestFocus(resolved.decoded.accountUUID, resolved.decoded.notificationId); + }, + [resolveNotification], + ); + + const handleNativeAction = useCallback( + (payload: INativeNotificationActionEvent) => { + if (payload.action === 'show') { + handleNativeClick({ id: payload.id }); + return; + } + + const resolved = resolveNotification(payload.id); + + if (!resolved?.notification) { + pendingInteractionsRef.current.push({ + payload, + expiresAt: Date.now() + PENDING_INTERACTION_TTL_MS, + }); + + if (pendingInteractionsRef.current.length > MAX_PENDING_INTERACTIONS) { + pendingInteractionsRef.current.shift(); + } + + return; + } + + executeAction(payload.action, resolved.notification); + }, + [executeAction, handleNativeClick, resolveNotification], + ); + + const processPendingInteractions = useCallback(() => { + if (pendingInteractionsRef.current.length === 0) { + return; + } + + const now = Date.now(); + const remaining: PendingInteraction[] = []; + + for (const pending of pendingInteractionsRef.current) { + if (pending.expiresAt <= now) { + continue; + } + + const resolved = resolveNotification(pending.payload.id); + + if (!resolved?.notification) { + remaining.push(pending); + continue; + } + + executeAction(pending.payload.action, resolved.notification); + } + + pendingInteractionsRef.current = remaining; + }, [executeAction, resolveNotification]); // Refetch notifications immediately when the system wakes from sleep or the // user unlocks their screen, without waiting for the next poll interval. @@ -96,7 +278,31 @@ 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]); + + // Retry interactions that arrived before the notification cache settled. + useEffect(() => { + if (enabled) { + processPendingInteractions(); + } + }, [enabled, notifications, processPendingInteractions]); + + // Handle sound and native notifications when new notifications arrive. useEffect(() => { if (!enabled || isLoading || isError || notifications.length === 0) { return; @@ -107,8 +313,7 @@ export const useNotificationEffects = ({ return; } - const unfilteredNotifications = - queryClient.getQueryData(notificationsQueryKey) || []; + const unfilteredNotifications = readUnfilteredNotifications(); const diffNotifications = getNewNotifications( previousNotificationsRef.current, @@ -127,7 +332,29 @@ export const useNotificationEffects = ({ } if (showSystemNotifications) { - void raiseNativeNotification(filteredDiffNotifications); + void Promise.resolve( + raiseNativeNotification(filteredDiffNotifications, { + style: nativeNotificationStyle, + actions: nativeNotificationActions, + keepVisible: nativeNotificationKeepVisible, + }), + ) + .then((published) => { + for (const payload of published ?? []) { + publishedIdsRef.current.add(payload.id); + + if (payload.url) { + publishedUrlsRef.current.set(payload.id, payload.url); + } + } + }) + .catch((error) => { + rendererLogError( + 'useNotificationEffects', + 'Failed to publish native notifications', + toError(error), + ); + }); } } } @@ -141,10 +368,41 @@ export const useNotificationEffects = ({ playSoundNewNotifications, showSystemNotifications, notificationVolume, - queryClient, - notificationsQueryKey, + nativeNotificationStyle, + nativeNotificationActions, + nativeNotificationKeepVisible, + readUnfilteredNotifications, ]); + // Remove published notifications when they leave the unfiltered cache, + // including read/done/unsubscribe, account removal, and reset. + useEffect(() => { + if (!enabled || publishedIdsRef.current.size === 0) { + return; + } + + const currentIds = new Set( + readUnfilteredNotifications().flatMap((accountNotifications) => + accountNotifications.notifications.map((notification) => + encodeNativeNotificationIdentity(getAccountUUID(notification.account), notification.id), + ), + ), + ); + + const removed = [...publishedIdsRef.current].filter((id) => !currentIds.has(id)); + + if (removed.length === 0) { + return; + } + + window.gitify.nativeNotifications.remove({ ids: removed }); + + for (const id of removed) { + publishedIdsRef.current.delete(id); + publishedUrlsRef.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 +414,7 @@ export const useNotificationEffects = ({ return; } - const unfilteredNotifications = - queryClient.getQueryData(notificationsQueryKey) || []; + const unfilteredNotifications = readUnfilteredNotifications(); const currentNotificationKeys = unfilteredNotifications.flatMap((accountNotifications) => accountNotifications.notifications.map((notification) => @@ -166,5 +423,5 @@ export const useNotificationEffects = ({ ); useNotificationActionFailuresStore.getState().pruneFailures(currentNotificationKeys); - }, [enabled, notifications, queryClient, notificationsQueryKey]); + }, [enabled, notifications, readUnfilteredNotifications]); }; diff --git a/src/renderer/hooks/notifications/useNotificationListDataKey.ts b/src/renderer/hooks/notifications/useNotificationListDataKey.ts new file mode 100644 index 000000000..bdf0916a4 --- /dev/null +++ b/src/renderer/hooks/notifications/useNotificationListDataKey.ts @@ -0,0 +1,59 @@ +import { useMemo } from 'react'; + +import { useFiltersStore } from '../../stores'; + +import type { Account, GroupBy } from '../../types'; + +import { getNotificationListDataKey } from '../../utils/notifications/listPositioning'; + +/** + * Build the logical dataset identity used by the notification list's + * positioning behavior. Filter values are read reactively so a filter change + * resets the list, while notification content and counts do not affect it. + */ +export function useNotificationListDataKey({ + groupBy, + accounts, +}: { + groupBy: GroupBy; + accounts: readonly Account[]; +}): string { + const includeSearchTokens = useFiltersStore((s) => s.includeSearchTokens); + const excludeSearchTokens = useFiltersStore((s) => s.excludeSearchTokens); + const filterAccounts = useFiltersStore((s) => s.accounts); + const userTypes = useFiltersStore((s) => s.userTypes); + const subjectTypes = useFiltersStore((s) => s.subjectTypes); + const states = useFiltersStore((s) => s.states); + const reasons = useFiltersStore((s) => s.reasons); + const reviewRequestTypes = useFiltersStore((s) => s.reviewRequestTypes); + + return useMemo( + () => + getNotificationListDataKey({ + groupBy, + accounts, + filters: { + includeSearchTokens, + excludeSearchTokens, + accounts: filterAccounts, + userTypes, + subjectTypes, + states, + reasons, + reviewRequestTypes, + }, + }), + [ + groupBy, + accounts, + includeSearchTokens, + excludeSearchTokens, + filterAccounts, + userTypes, + subjectTypes, + states, + reasons, + reviewRequestTypes, + ], + ); +} diff --git a/src/renderer/hooks/notifications/useNotificationsHost.ts b/src/renderer/hooks/notifications/useNotificationsHost.ts index 1aeebb1d6..dfa3f6592 100644 --- a/src/renderer/hooks/notifications/useNotificationsHost.ts +++ b/src/renderer/hooks/notifications/useNotificationsHost.ts @@ -13,7 +13,18 @@ import { useNotificationsQuery } from './useNotificationsQuery'; * * @returns The same reader/actions surface as `useNotifications`. */ -export const useNotificationsHost = (): NotificationsState => { +export interface NotificationsHostOptions { + /** + * Navigate to the notifications route when an OS notification click reveals + * a notification in-app. Defaults to a no-op for consumers that only need the + * host's data/actions (for example tests). + */ + onReveal?: () => void; +} + +export const useNotificationsHost = ({ + onReveal = () => undefined, +}: NotificationsHostOptions = {}): NotificationsState => { const query = useNotificationsQuery({ poll: true }); const actions = useNotificationActions(); @@ -25,6 +36,8 @@ export const useNotificationsHost = (): NotificationsState => { notificationsQueryKey: query.notificationsQueryKey, refetch: query.refetch, fetchIntervalMs: query.fetchIntervalMs, + actions, + onReveal, }); return buildNotificationsState(query, actions); diff --git a/src/renderer/hooks/useNotifications.test.tsx b/src/renderer/hooks/useNotifications.test.tsx index d722818e2..6923bb976 100644 --- a/src/renderer/hooks/useNotifications.test.tsx +++ b/src/renderer/hooks/useNotifications.test.tsx @@ -14,16 +14,22 @@ import { mockSingleAccountNotifications, } from '../__mocks__/notifications-mocks'; +import type { INativeNotificationPayload } from '../../shared/events'; +import { encodeNativeNotificationIdentity } from '../../shared/nativeNotifications'; + import { getNotificationFailureKey, useAccountsStore, useFiltersStore, useNotificationActionFailuresStore, + useNotificationFocusStore, useSettingsStore, } from '../stores'; import type { AccountNotifications, Percentage } from '../types'; +import { NativeNotificationClickAction } from '../types'; +import { getAccountUUID } from '../utils/auth/utils'; import { Errors } from '../utils/core/errors'; import * as logger from '../utils/core/logger'; import { getAdapter } from '../utils/forges/registry'; @@ -33,6 +39,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'; @@ -646,6 +653,18 @@ describe('renderer/hooks/useNotifications.ts', () => { expect(raiseNativeNotificationSpy).toHaveBeenCalledTimes(1); }); + it('does not publish an OS notification for a filtered-out new notification', async () => { + useFiltersStore.setState({ reasons: ['mention'] }); + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + const { result } = renderNotificationsHook(); + + await waitFor(() => expect(result.current.status).toBe('success')); + + expect(raiseSoundNotificationSpy).not.toHaveBeenCalled(); + expect(raiseNativeNotificationSpy).not.toHaveBeenCalled(); + }); + it('does not raise sound or native notifications when disabled', async () => { useSettingsStore.setState({ playSound: false, @@ -662,6 +681,170 @@ describe('renderer/hooks/useNotifications.ts', () => { }); }); + describe('OS notification interactions', () => { + const notification = mockSingleAccountNotifications[0].notifications[0]; + const notificationId = encodeNativeNotificationIdentity( + getAccountUUID(notification.account), + notification.id, + ); + + const actionHandler = () => + vi.mocked(window.gitify.nativeNotifications.onAction).mock.calls.at(-1)?.[0] as (payload: { + id: string; + action: 'show' | 'read' | 'done' | 'unsubscribe'; + }) => void; + + const clickHandler = () => + vi.mocked(window.gitify.nativeNotifications.onClick).mock.calls.at(-1)?.[0] as (payload: { + id: string; + }) => void; + + it.each([ + ['read', 'markThreadAsRead'], + ['done', 'markThreadAsDone'], + ['unsubscribe', 'unsubscribeThread'], + ] as const)( + 'routes a %s action through the existing mutation pipeline', + async (action, method) => { + const actionSpy = vi.spyOn(githubAdapter.accountOps, method).mockResolvedValue(undefined); + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.hasNotifications).toBe(true)); + + await act(async () => { + actionHandler()({ id: notificationId, action }); + }); + + await waitFor(() => expect(actionSpy).toHaveBeenCalledTimes(1)); + }, + ); + + it('opens the target in the browser when the click setting is browser', async () => { + useSettingsStore.setState({ + nativeNotificationClickAction: NativeNotificationClickAction.BROWSER, + }); + const openNotificationSpy = vi.spyOn(links, 'openNotification').mockResolvedValue(); + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.hasNotifications).toBe(true)); + + act(() => clickHandler()({ id: notificationId })); + + await waitFor(() => expect(openNotificationSpy).toHaveBeenCalledTimes(1)); + }); + + it('reveals the notification in the app when the click setting is app', async () => { + useSettingsStore.setState({ + nativeNotificationClickAction: NativeNotificationClickAction.APP, + }); + const onReveal = vi.fn(); + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + const { result } = renderHook(() => useNotificationsHost({ onReveal }), { + wrapper: createWrapper(), + }); + await waitFor(() => expect(result.current.hasNotifications).toBe(true)); + + act(() => clickHandler()({ id: notificationId })); + + expect(window.gitify.app.show).toHaveBeenCalled(); + expect(onReveal).toHaveBeenCalledTimes(1); + expect(useNotificationFocusStore.getState().intent).toMatchObject({ + accountUUID: getAccountUUID(notification.account), + notificationId: notification.id, + }); + }); + + it('reveals the notifications list when the summary action is activated', async () => { + const onReveal = vi.fn(); + + renderHook(() => useNotificationsHost({ onReveal }), { + wrapper: createWrapper(), + }); + + act(() => actionHandler()({ id: 'summary', action: 'show' })); + + expect(window.gitify.app.show).toHaveBeenCalled(); + expect(onReveal).toHaveBeenCalledTimes(1); + }); + + it('removes a published notification when it leaves the unfiltered cache', async () => { + raiseNativeNotificationSpy.mockResolvedValueOnce([ + { id: notificationId } as INativeNotificationPayload, + ]); + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.hasNotifications).toBe(true)); + await waitFor(() => expect(raiseNativeNotificationSpy).toHaveBeenCalled()); + + getAllNotificationsMock.mockResolvedValue([]); + + await act(async () => { + await result.current.refetchNotifications(); + }); + + await waitFor(() => + expect(window.gitify.nativeNotifications.remove).toHaveBeenCalledWith({ + ids: [notificationId], + }), + ); + }); + + it('retries an action that arrives before the cache settles', async () => { + let resolveFetch: (value: AccountNotifications[]) => void = () => {}; + getAllNotificationsMock.mockReturnValue( + new Promise((resolve) => { + resolveFetch = resolve; + }), + ); + const actionSpy = vi + .spyOn(githubAdapter.accountOps, 'markThreadAsRead') + .mockResolvedValue(undefined); + + renderNotificationsHook(); + await waitFor(() => expect(actionHandler()).toBeTypeOf('function')); + + act(() => actionHandler()({ id: notificationId, action: 'read' })); + expect(actionSpy).not.toHaveBeenCalled(); + + await act(async () => { + resolveFetch(mockSingleAccountNotifications); + }); + + await waitFor(() => expect(actionSpy).toHaveBeenCalledTimes(1)); + }); + + it('does not remove a published notification merely because a filter hides it', async () => { + raiseNativeNotificationSpy.mockResolvedValueOnce([ + { id: notificationId } as INativeNotificationPayload, + ]); + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + const { result } = renderNotificationsHook(); + await waitFor(() => expect(result.current.hasNotifications).toBe(true)); + await waitFor(() => expect(raiseNativeNotificationSpy).toHaveBeenCalled()); + + act(() => { + useFiltersStore.setState({ reasons: ['mention'] }); + }); + + await waitFor(() => expect(result.current.hasNotifications).toBe(false)); + expect(window.gitify.nativeNotifications.remove).not.toHaveBeenCalled(); + }); + + it('tells the main process the interaction listeners are ready', async () => { + getAllNotificationsMock.mockResolvedValue(mockSingleAccountNotifications); + + renderNotificationsHook(); + await waitFor(() => + expect(window.gitify.nativeNotifications.notifyReady).toHaveBeenCalledTimes(1), + ); + }); + }); + describe('markNotificationsAsRead', () => { it('marks notifications as read via the forge adapter and removes them', async () => { const markThreadAsReadSpy = vi diff --git a/src/renderer/routes/__snapshots__/Settings.test.tsx.snap b/src/renderer/routes/__snapshots__/Settings.test.tsx.snap index baa628b99..90cae8e0c 100644 --- a/src/renderer/routes/__snapshots__/Settings.test.tsx.snap +++ b/src/renderer/routes/__snapshots__/Settings.test.tsx.snap @@ -3120,6 +3120,518 @@ exports[`renderer/routes/Settings.tsx > should render itself & its children 1`]
+
+
+
+ + +
+
+
    +
  • + +
    • +
  • + +
    • +
+
+
+
+
+
+
+ + +
+
+
+ +
+ + + +
+
+
+
+
+ + +
+
+
+ +
+ + + +
+
+
+
+
+ + +
+
+
    +
  • + +
    • +
  • + +
    • +
+
+
+
@@ -3132,7 +3644,7 @@ exports[`renderer/routes/Settings.tsx > should render itself & its children 1`]