diff --git a/hub/src/push/notificationCopy.test.ts b/hub/src/push/notificationCopy.test.ts index ba7df570..1ab57aa1 100644 --- a/hub/src/push/notificationCopy.test.ts +++ b/hub/src/push/notificationCopy.test.ts @@ -64,6 +64,18 @@ describe('loadNotificationCopy', () => { await rm(dir, { recursive: true, force: true }) } }) + + it('returns empty config when persisted copy is malformed', async () => { + const dir = await mkdtemp(join(tmpdir(), 'hapi-copy-test-')) + try { + await writeFile(join(dir, 'settings.json'), JSON.stringify({ + notificationCopy: { ready: { title: 42, body: 'hello' } } + })) + expect(await loadNotificationCopy(dir)).toEqual({}) + } finally { + await rm(dir, { recursive: true, force: true }) + } + }) }) describe('resolveCopy', () => { @@ -71,12 +83,18 @@ describe('resolveCopy', () => { expect(resolveCopy('ready', {})).toEqual(DEFAULT_COPY.ready) }) - it('returns the default when title is empty', () => { - expect(resolveCopy('ready', { ready: { title: ' ', body: 'custom' } })).toEqual(DEFAULT_COPY.ready) + it('falls back only the title when title is empty', () => { + expect(resolveCopy('ready', { ready: { title: ' ', body: 'custom' } })).toEqual({ + title: DEFAULT_COPY.ready.title, + body: 'custom' + }) }) - it('returns the default when body is empty', () => { - expect(resolveCopy('ready', { ready: { title: 'custom', body: '' } })).toEqual(DEFAULT_COPY.ready) + it('falls back only the body when body is empty', () => { + expect(resolveCopy('ready', { ready: { title: 'custom', body: '' } })).toEqual({ + title: 'custom', + body: DEFAULT_COPY.ready.body + }) }) it('returns the stored template when both fields are non-empty', () => { diff --git a/hub/src/push/notificationCopy.ts b/hub/src/push/notificationCopy.ts index 02c30dae..390aa837 100644 --- a/hub/src/push/notificationCopy.ts +++ b/hub/src/push/notificationCopy.ts @@ -1,4 +1,5 @@ import type { SessionEndReason } from '@hapi/protocol' +import { z } from 'zod' import { getSettingsFile, readSettings } from '../config/settings' import type { TaskNotification } from '../notifications/notificationTypes' import { getAgentName, getSessionName } from '../notifications/sessionInfo' @@ -6,12 +7,21 @@ import type { Session } from '../sync/syncEngine' export type CopyKey = 'permissionRequest' | 'ready' | 'taskCompleted' | 'taskFailed' | 'sessionCompletion' -export type CopyTemplate = { - title: string - body: string -} +const copyTemplateSchema = z.object({ + title: z.string().max(500), + body: z.string().max(500) +}) -export type NotificationCopyConfig = Partial> +export const notificationCopySchema = z.object({ + permissionRequest: copyTemplateSchema.optional(), + ready: copyTemplateSchema.optional(), + taskCompleted: copyTemplateSchema.optional(), + taskFailed: copyTemplateSchema.optional(), + sessionCompletion: copyTemplateSchema.optional() +}) + +export type CopyTemplate = z.infer +export type NotificationCopyConfig = z.infer export const COPY_KEYS: readonly CopyKey[] = [ 'permissionRequest', @@ -36,22 +46,24 @@ export const DEFAULT_COPY: Record = { export async function loadNotificationCopy(dataDir: string): Promise { try { const settings = await readSettings(getSettingsFile(dataDir)) - return settings?.notificationCopy ?? {} + const parsed = notificationCopySchema.safeParse(settings?.notificationCopy ?? {}) + return parsed.success ? parsed.data : {} } catch { return {} } } /** - * A stored template only takes effect when BOTH title and body are non-empty; - * an empty field falls back to the default template. + * Empty fields fall back independently so a title-only or body-only override + * still takes effect. */ export function resolveCopy(key: CopyKey, stored: NotificationCopyConfig): CopyTemplate { const template = stored[key] - if (template && template.title.trim() && template.body.trim()) { - return { title: template.title, body: template.body } + const defaults = DEFAULT_COPY[key] + return { + title: template?.title.trim() ? template.title : defaults.title, + body: template?.body.trim() ? template.body : defaults.body } - return DEFAULT_COPY[key] } /** diff --git a/hub/src/push/pushService.test.ts b/hub/src/push/pushService.test.ts new file mode 100644 index 00000000..7d381c50 --- /dev/null +++ b/hub/src/push/pushService.test.ts @@ -0,0 +1,69 @@ +import { beforeEach, describe, expect, it, mock } from 'bun:test' +import { PushService, type PushPayload } from './pushService' + +const sendNotification = mock(async (_subscription: unknown, _body: string) => ({})) + +mock.module('web-push', () => ({ + setVapidDetails: mock(() => {}), + sendNotification +})) + +type Subscription = { + endpoint: string + p256dh: string + auth: string +} + +function createService(subscriptions: Subscription[]) { + const removePushSubscription = mock(() => {}) + const store = { + push: { + getPushSubscriptionsByNamespace: mock(() => subscriptions), + removePushSubscription + } + } + return { + service: new PushService( + { publicKey: 'test-public', privateKey: 'test-private' }, + 'mailto:test@example.com', + store as never + ), + removePushSubscription + } +} + +const payload: PushPayload = { + title: 'Test', + body: 'Test body' +} + +beforeEach(() => { + sendNotification.mockClear() + sendNotification.mockImplementation(async () => ({})) +}) + +describe('PushService.sendToNamespace', () => { + it('returns zero when the namespace has no subscriptions', async () => { + const { service } = createService([]) + + expect(await service.sendToNamespace('default', payload)).toBe(0) + expect(sendNotification).not.toHaveBeenCalled() + }) + + it('counts only successful deliveries', async () => { + const subscriptions = [ + { endpoint: 'https://push.example/ok', p256dh: 'key-1', auth: 'auth-1' }, + { endpoint: 'https://push.example/gone', p256dh: 'key-2', auth: 'auth-2' } + ] + sendNotification.mockImplementation(async (subscription: unknown) => { + if ((subscription as { endpoint: string }).endpoint.endsWith('/gone')) { + throw { statusCode: 410 } + } + return {} + }) + const { service, removePushSubscription } = createService(subscriptions) + + expect(await service.sendToNamespace('default', payload)).toBe(1) + expect(removePushSubscription).toHaveBeenCalledWith('default', 'https://push.example/gone') + }) +}) diff --git a/hub/src/push/pushService.ts b/hub/src/push/pushService.ts index e44a8fd9..fb60f2ba 100644 --- a/hub/src/push/pushService.ts +++ b/hub/src/push/pushService.ts @@ -38,23 +38,24 @@ export class PushService { webPush.setVapidDetails(this.subject, this.vapidKeys.publicKey, this.vapidKeys.privateKey) } - async sendToNamespace(namespace: string, payload: PushPayload): Promise { + async sendToNamespace(namespace: string, payload: PushPayload): Promise { const subscriptions = this.store.push.getPushSubscriptionsByNamespace(namespace) if (subscriptions.length === 0) { - return + return 0 } const body = JSON.stringify(payload) - await Promise.all(subscriptions.map((subscription) => { + const delivered = await Promise.all(subscriptions.map((subscription) => { return this.sendToSubscription(namespace, subscription, body) })) + return delivered.filter(Boolean).length } private async sendToSubscription( namespace: string, subscription: StoredSubscription, body: string - ): Promise { + ): Promise { const pushSubscription: PushSubscription = { endpoint: subscription.endpoint, keys: { @@ -65,6 +66,7 @@ export class PushService { try { await webPush.sendNotification(pushSubscription, body) + return true } catch (error) { const statusCode = typeof (error as { statusCode?: unknown }).statusCode === 'number' ? (error as { statusCode: number }).statusCode @@ -72,10 +74,11 @@ export class PushService { if (statusCode === 410) { this.store.push.removePushSubscription(namespace, subscription.endpoint) - return + return false } console.error('[PushService] Failed to send notification:', error) + return false } } } diff --git a/hub/src/web/routes/notificationCopy.test.ts b/hub/src/web/routes/notificationCopy.test.ts index edefde73..8b48f13e 100644 --- a/hub/src/web/routes/notificationCopy.test.ts +++ b/hub/src/web/routes/notificationCopy.test.ts @@ -57,6 +57,16 @@ describe('GET /api/notification-copy', () => { const body = await res.json() as { copy: Record } expect(body.copy).toEqual({}) }) + + it('returns empty copy when persisted notification copy is malformed', async () => { + await writeFile(join(dir, 'settings.json'), JSON.stringify({ + notificationCopy: { ready: { title: 42, body: 'hello' } } + })) + const app = await createApp('default') + const res = await app.request('/api/notification-copy') + const body = await res.json() as { copy: Record } + expect(body.copy).toEqual({}) + }) }) describe('PUT /api/notification-copy', () => { diff --git a/hub/src/web/routes/notificationCopy.ts b/hub/src/web/routes/notificationCopy.ts index 3d8d3e90..f1311356 100644 --- a/hub/src/web/routes/notificationCopy.ts +++ b/hub/src/web/routes/notificationCopy.ts @@ -1,22 +1,14 @@ import { Hono } from 'hono' -import { z } from 'zod' import { getSettingsFile, readSettingsOrThrow, writeSettings } from '../../config/settings' -import { COPY_KEYS, DEFAULT_COPY, type CopyKey, type NotificationCopyConfig } from '../../push/notificationCopy' +import { + COPY_KEYS, + DEFAULT_COPY, + notificationCopySchema, + type CopyKey, + type NotificationCopyConfig +} from '../../push/notificationCopy' import type { WebAppEnv } from '../middleware/auth' -const copyTemplateSchema = z.object({ - title: z.string().max(500), - body: z.string().max(500) -}) - -const updateSchema = z.object({ - permissionRequest: copyTemplateSchema.optional(), - ready: copyTemplateSchema.optional(), - taskCompleted: copyTemplateSchema.optional(), - taskFailed: copyTemplateSchema.optional(), - sessionCompletion: copyTemplateSchema.optional() -}) - function isAdmin(namespace: string): boolean { return namespace === 'default' } @@ -31,8 +23,9 @@ export function createNotificationCopyRoutes(dataDir: string): Hono { return c.json({ error: 'Forbidden: admin only' }, 403) } const settings = await readSettingsOrThrow(settingsFile) + const parsed = notificationCopySchema.safeParse(settings.notificationCopy ?? {}) return c.json({ - copy: settings.notificationCopy ?? {}, + copy: parsed.success ? parsed.data : {}, defaults: DEFAULT_COPY }) }) @@ -43,7 +36,7 @@ export function createNotificationCopyRoutes(dataDir: string): Hono { return c.json({ error: 'Forbidden: admin only' }, 403) } const json = await c.req.json().catch(() => null) - const parsed = updateSchema.safeParse(json) + const parsed = notificationCopySchema.safeParse(json) if (!parsed.success) { return c.json({ error: 'Invalid body', issues: parsed.error.flatten() }, 400) } diff --git a/hub/src/web/routes/push.test.ts b/hub/src/web/routes/push.test.ts new file mode 100644 index 00000000..23e3202d --- /dev/null +++ b/hub/src/web/routes/push.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from 'bun:test' +import { Hono } from 'hono' +import type { PushPayload } from '../../push/pushService' +import type { WebAppEnv } from '../middleware/auth' +import { createPushRoutes } from './push' + +function createApp(sendToNamespace: (namespace: string, payload: PushPayload) => Promise): Hono { + const app = new Hono() + app.use('*', async (c, next) => { + c.set('namespace', 'default') + await next() + }) + app.route('/api', createPushRoutes({} as never, 'test-key', { sendToNamespace } as never)) + return app +} + +describe('POST /api/push/test', () => { + it('returns success when at least one subscription receives the push', async () => { + const sent: Array<{ namespace: string; payload: PushPayload }> = [] + const app = createApp(async (namespace, payload) => { + sent.push({ namespace, payload }) + return 1 + }) + + const response = await app.request('/api/push/test', { method: 'POST' }) + + expect(response.status).toBe(200) + expect(sent[0]?.namespace).toBe('default') + expect(sent[0]?.payload.tag).toBe('test-push') + }) + + it('returns 503 when no subscription receives the push', async () => { + const app = createApp(async () => 0) + + const response = await app.request('/api/push/test', { method: 'POST' }) + + expect(response.status).toBe(503) + expect(await response.json()).toEqual({ error: 'No push notification was delivered' }) + }) +}) diff --git a/hub/src/web/routes/push.ts b/hub/src/web/routes/push.ts index 0173ea95..7c3cd5c3 100644 --- a/hub/src/web/routes/push.ts +++ b/hub/src/web/routes/push.ts @@ -61,7 +61,7 @@ export function createPushRoutes( app.post('/push/test', async (c) => { const namespace = c.get('namespace') try { - await pushService.sendToNamespace(namespace, { + const delivered = await pushService.sendToNamespace(namespace, { title: 'HAPI Test Notification', body: 'Your push notifications are working.', tag: 'test-push', @@ -71,6 +71,9 @@ export function createPushRoutes( url: '/settings/notifications' } }) + if (delivered === 0) { + return c.json({ error: 'No push notification was delivered' }, 503) + } return c.json({ ok: true }) } catch (error) { console.error('[PushRoutes] Test push failed:', error) diff --git a/web/src/routes/settings/notifications.test.tsx b/web/src/routes/settings/notifications.test.tsx index c3767dbf..00c9524d 100644 --- a/web/src/routes/settings/notifications.test.tsx +++ b/web/src/routes/settings/notifications.test.tsx @@ -152,4 +152,12 @@ describe('SettingsNotificationsPage', () => { renderPage() expect(await screen.findByText(/Claude is waiting in My Project/)).toBeTruthy() }) + + it('previews a title-only override with the default body', async () => { + renderPage() + const titleInputs = await screen.findAllByLabelText('Title') + fireEvent.change(titleInputs[1], { target: { value: 'Custom {agentName}' } }) + + expect(await screen.findByText(/Custom Claude.*Claude is waiting in My Project/)).toBeTruthy() + }) }) diff --git a/web/src/routes/settings/notifications.tsx b/web/src/routes/settings/notifications.tsx index 276785d1..2e4b4eab 100644 --- a/web/src/routes/settings/notifications.tsx +++ b/web/src/routes/settings/notifications.tsx @@ -177,12 +177,9 @@ export default function SettingsNotificationsPage() { const d = draft[block] const defaults = copyQuery.data?.defaults const def = defaults ? defaults[block] as CopyTemplate | undefined : undefined - if (d && d.title.trim() && d.body.trim()) { - return { title: renderTemplate(d.title, PREVIEW_VARS), body: renderTemplate(d.body, PREVIEW_VARS) } - } return { - title: renderTemplate(def?.title ?? '', PREVIEW_VARS), - body: renderTemplate(def?.body ?? '', PREVIEW_VARS) + title: renderTemplate(d?.title.trim() ? d.title : (def?.title ?? ''), PREVIEW_VARS), + body: renderTemplate(d?.body.trim() ? d.body : (def?.body ?? ''), PREVIEW_VARS) } }