fix(notifications): replay completion sounds reliably (#15940)

* fix(notifications): replay completion sounds reliably

The in-flight `isNotificationSoundPlaying` gate dropped every notification
sound that arrived while the previous one was still ringing, so bursts played
once. Drop the gate and restart the cached Audio instead, and reuse an entry a
concurrent call already cached for the same path so parallel first playbacks
share one Audio rather than revoking each other's blob URL.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016S8SZpkXGcnU5UWwA8UiF2

* test(notifications): verify real sound replay and remove unused playback listeners

---------

Co-authored-by: Laku <laku@LakudeMacBook-Pro.local>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Neil <neil@stably.ai>

src/preload/api/notification-sound-playback.test.ts

@@ -0,0 +1,115 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import type { notificationsApi } from './notifications-bridge'
+
+const SOUND_PATH = join(tmpdir(), 'notification.mp3')
+
+const { construct, invoke, play } = vi.hoisted(() => ({
+  construct: vi.fn((audio: { currentTime: number; volume: number; pause: () => void }) => audio),
+  invoke: vi.fn(),
+  play: vi.fn(() => Promise.resolve())
+}))
+
+vi.mock('electron', () => ({ ipcRenderer: { invoke } }))
+
+async function loadNotificationsApi(): Promise<typeof notificationsApi> {
+  vi.resetModules()
+  return (await import('./notifications-bridge')).notificationsApi
+}
+
+describe('notificationsApi.playSound', () => {
+  beforeEach(() => {
+    construct.mockClear()
+    play.mockClear()
+    vi.stubGlobal(
+      'Audio',
+      class extends EventTarget {
+        currentTime = 0
+        volume = 1
+        src = ''
+        pause = vi.fn()
+        play = play
+
+        constructor() {
+          super()
+          construct(this)
+        }
+      }
+    )
+    invoke.mockReset()
+    invoke.mockImplementation((channel: string) => {
+      if (channel === 'notifications:resolveSoundPath') {
+        return Promise.resolve({ ok: true, path: SOUND_PATH })
+      }
+      if (channel === 'notifications:loadSound') {
+        return Promise.resolve({
+          ok: true,
+          data: new Uint8Array([1]),
+          mimeType: 'audio/mpeg',
+          path: SOUND_PATH
+        })
+      }
+      return Promise.resolve(undefined)
+    })
+  })
+
+  afterEach(() => vi.unstubAllGlobals())
+
+  it('replays the cached sound for each notification instead of deduping mid-playback', async () => {
+    const notificationsApi = await loadNotificationsApi()
+
+    await expect(notificationsApi.playSound()).resolves.toEqual({ played: true })
+    const audio = construct.mock.calls[0]?.[0]
+    if (!audio) {
+      throw new Error('Audio was not constructed')
+    }
+    audio.currentTime = 0.75
+    await expect(notificationsApi.playSound()).resolves.toEqual({ played: true })
+
+    expect(audio.currentTime).toBe(0)
+    expect(construct).toHaveBeenCalledOnce()
+    expect(play).toHaveBeenCalledTimes(2)
+  })
+
+  it('retries after a rejected play without leaving future notifications silent', async () => {
+    const notificationsApi = await loadNotificationsApi()
+    play.mockRejectedValueOnce(new Error('audio device unavailable'))
+    await expect(notificationsApi.playSound()).resolves.toEqual({
+      played: false,
+      reason: 'playback-failed'
+    })
+    await expect(notificationsApi.playSound()).resolves.toEqual({ played: true })
+    expect(construct).toHaveBeenCalledOnce()
+  })
+
+  it('clamps volume and stays silent when no sound is configured', async () => {
+    const notificationsApi = await loadNotificationsApi()
+    await notificationsApi.playSound({ volume: 150 })
+    const audio = construct.mock.calls[0]?.[0]
+    if (!audio) {
+      throw new Error('Audio was not constructed')
+    }
+    expect(audio.volume).toBe(1)
+    await notificationsApi.playSound({ volume: -10 })
+    expect(audio.volume).toBe(0)
+    invoke.mockResolvedValueOnce({ ok: false, reason: 'missing-path' })
+    await expect(notificationsApi.playSound()).resolves.toEqual({
+      played: false,
+      reason: 'missing-path'
+    })
+    expect(audio.pause).toHaveBeenCalledOnce()
+    expect(play).toHaveBeenCalledTimes(2)
+  })
+
+  it('shares one cached Audio across concurrent first playback', async () => {
+    const notificationsApi = await loadNotificationsApi()
+
+    await expect(
+      Promise.all([notificationsApi.playSound(), notificationsApi.playSound()])
+    ).resolves.toEqual([{ played: true }, { played: true }])
+
+    expect(construct).toHaveBeenCalledOnce()
+    expect(play).toHaveBeenCalledTimes(2)
+  })
+})

src/preload/api/notifications-bridge.ts

@@ -16,19 +16,8 @@ let cachedNotificationSound: {
   blobUrl: string
   audio: HTMLAudioElement
 } | null = null
-let isNotificationSoundPlaying = false
-// Why: audio.play() can reject before ended/error fires; cleanup prevents leaked listeners.
-let cleanupNotificationSoundPlayback: (() => void) | null = null
-
-function clearNotificationSoundPlaybackState(): void {
-  cleanupNotificationSoundPlayback?.()
-  cleanupNotificationSoundPlayback = null
-  isNotificationSoundPlaying = false
-}
-
 function disposeCachedNotificationSound(): void {
   if (cachedNotificationSound) {
-    clearNotificationSoundPlaybackState()
     cachedNotificationSound.audio.pause()
     cachedNotificationSound.audio.src = ''
     URL.revokeObjectURL(cachedNotificationSound.blobUrl)
@@ -53,11 +42,6 @@ export const notificationsApi = {
     volume?: number
   }): Promise<NotificationSoundResult> => {
     try {
-      // Why: drop replays while still ringing; the test button passes force to always confirm.
-      if (!options?.force && isNotificationSoundPlaying) {
-        return { played: false, reason: 'deduped' }
-      }
-
       const resolved = (await ipcRenderer.invoke(
         'notifications:resolveSoundPath'
       )) as NotificationSoundPathResult
@@ -77,13 +61,19 @@ export const notificationsApi = {
           disposeCachedNotificationSound()
           return { played: false, reason: sound.reason }
         }
-        const arrayBuffer = new ArrayBuffer(sound.data.byteLength)
-        new Uint8Array(arrayBuffer).set(sound.data)
-        const blob = new Blob([arrayBuffer], { type: sound.mimeType })
-        disposeCachedNotificationSound()
-        const blobUrl = URL.createObjectURL(blob)
-        entry = { path: sound.path, blobUrl, audio: new Audio(blobUrl) }
-        cachedNotificationSound = entry
+        // Why: a concurrent playSound may have cached the same path while this load was in flight.
+        const latestEntry = cachedNotificationSound
+        if (latestEntry?.path === sound.path) {
+          entry = latestEntry
+        } else {
+          const arrayBuffer = new ArrayBuffer(sound.data.byteLength)
+          new Uint8Array(arrayBuffer).set(sound.data)
+          const blob = new Blob([arrayBuffer], { type: sound.mimeType })
+          disposeCachedNotificationSound()
+          const blobUrl = URL.createObjectURL(blob)
+          entry = { path: sound.path, blobUrl, audio: new Audio(blobUrl) }
+          cachedNotificationSound = entry
+        }
       }
 
       const audio = entry.audio
@@ -92,31 +82,13 @@ export const notificationsApi = {
       if (typeof options?.volume === 'number' && Number.isFinite(options.volume)) {
         audio.volume = Math.min(1, Math.max(0, options.volume / 100))
       }
-      isNotificationSoundPlaying = true
-      cleanupNotificationSoundPlayback?.()
-      const release = (): void => {
-        cleanup()
-        if (cleanupNotificationSoundPlayback === cleanup) {
-          cleanupNotificationSoundPlayback = null
-        }
-        isNotificationSoundPlaying = false
-      }
-      const cleanup = (): void => {
-        audio.removeEventListener('ended', release)
-        audio.removeEventListener('error', release)
-      }
-      cleanupNotificationSoundPlayback = cleanup
-      audio.addEventListener('ended', release)
-      audio.addEventListener('error', release)
       try {
         await audio.play()
       } catch {
-        release()
         return { played: false, reason: 'playback-failed' }
       }
       return { played: true }
     } catch {
-      clearNotificationSoundPlaybackState()
       return { played: false, reason: 'playback-failed' }
     }
   }

tests/e2e/notification-sound-replay.spec.ts

@@ -0,0 +1,153 @@
+import { writeFileSync } from 'node:fs'
+import { test, expect } from './helpers/orca-app'
+import { waitForSessionReady } from './helpers/store'
+
+// A long tone makes the second request unambiguously arrive during playback.
+function notificationTone(): Buffer {
+  const rate = 48_000
+  const samples = rate * 2
+  const wav = Buffer.alloc(44 + samples * 2)
+  wav.write('RIFF', 0)
+  wav.writeUInt32LE(wav.length - 8, 4)
+  wav.write('WAVEfmt ', 8)
+  wav.writeUInt32LE(16, 16)
+  wav.writeUInt16LE(1, 20)
+  wav.writeUInt16LE(1, 22)
+  wav.writeUInt32LE(rate, 24)
+  wav.writeUInt32LE(rate * 2, 28)
+  wav.writeUInt16LE(2, 32)
+  wav.writeUInt16LE(16, 34)
+  wav.write('data', 36)
+  wav.writeUInt32LE(samples * 2, 40)
+  for (let i = 0; i < samples; i++) {
+    const seconds = i / rate
+    const envelope = Math.min(1, seconds * 100, (2 - seconds) * 100)
+    const frequency = seconds < 0.12 ? 880 : 440
+    wav.writeInt16LE(
+      Math.round(Math.sin(2 * Math.PI * frequency * seconds) * envelope * 6000),
+      44 + i * 2
+    )
+  }
+  return wav
+}
+
+test('completion sound restarts during playback without overlapping players', async ({
+  orcaPage
+}, testInfo) => {
+  await waitForSessionReady(orcaPage)
+  const soundPath = testInfo.outputPath('notification-tone.wav')
+  writeFileSync(soundPath, notificationTone())
+  await orcaPage.evaluate(async (soundPath) => {
+    const settings = await window.api.settings.get()
+    const next = await window.api.settings.set({
+      notifications: {
+        ...settings.notifications,
+        customSoundId: 'custom',
+        customSoundPath: soundPath,
+        customSoundVolume: 100
+      }
+    })
+    window.__store?.setState({ settings: next })
+    window.__store?.getState().openSettingsTarget({ pane: 'notifications', repoId: null })
+    window.__store?.getState().openSettingsPage()
+  }, soundPath)
+  const cdp = await orcaPage.context().newCDPSession(orcaPage)
+  let isolatedContextId: number | undefined
+  cdp.on('Runtime.executionContextCreated', ({ context }) => {
+    if (context.name === 'Electron Isolated Context') {
+      isolatedContextId = context.id
+    }
+  })
+  await cdp.send('Runtime.enable')
+  expect(isolatedContextId).toBeDefined()
+  // Intercept construction only; decoding, seeking and play remain Chromium's real Audio.
+  await cdp.send('Runtime.evaluate', {
+    contextId: isolatedContextId,
+    expression: `(() => {
+      const OriginalAudio = Audio;
+      const audios = [];
+      const context = new AudioContext();
+      const destination = context.createMediaStreamDestination();
+      const chunks = [];
+      const recorder = new MediaRecorder(destination.stream);
+      recorder.ondataavailable = event => chunks.push(event.data);
+      recorder.start();
+      globalThis.Audio = function (...args) {
+        const audio = new OriginalAudio(...args);
+        context.createMediaElementSource(audio).connect(destination);
+        audios.push(audio);
+        return audio;
+      };
+      globalThis.notificationAudioProbe = { audios, recorder, chunks, context };
+      return context.resume();
+    })()`,
+    awaitPromise: true
+  })
+  const started = Date.now()
+  const first = await orcaPage.evaluate(() => window.api.notifications.playSound())
+  expect(first).toEqual({ played: true })
+  await orcaPage.waitForTimeout(600)
+  const beforeSeek = await cdp.send('Runtime.evaluate', {
+    contextId: isolatedContextId,
+    expression: 'notificationAudioProbe.audios[0].currentTime',
+    returnByValue: true
+  })
+  expect(beforeSeek.result.value).toBeGreaterThan(0.3)
+  const second = await orcaPage.evaluate(() => window.api.notifications.playSound())
+  const baseline = process.env.ORCA_NOTIFICATION_SOUND_BASELINE === '1'
+  expect(second).toEqual(baseline ? { played: false, reason: 'deduped' } : { played: true })
+  const afterSeek = await cdp.send('Runtime.evaluate', {
+    contextId: isolatedContextId,
+    expression: 'notificationAudioProbe.audios[0].currentTime',
+    returnByValue: true
+  })
+  if (!baseline) {
+    expect(afterSeek.result.value).toBeLessThan(0.2)
+  }
+  await orcaPage.waitForTimeout(2400)
+  expect(await orcaPage.evaluate(() => window.api.notifications.playSound())).toEqual({
+    played: true
+  })
+  await orcaPage.waitForTimeout(2200)
+  const count = await cdp.send('Runtime.evaluate', {
+    contextId: isolatedContextId,
+    expression: 'notificationAudioProbe.audios.length',
+    returnByValue: true
+  })
+  expect(count.result.value).toBe(1)
+  const recording = await cdp.send('Runtime.evaluate', {
+    contextId: isolatedContextId,
+    awaitPromise: true,
+    returnByValue: true,
+    expression: `new Promise(resolve => {
+      const probe = notificationAudioProbe;
+      probe.recorder.onstop = async () => {
+        const bytes = new Uint8Array(await new Blob(probe.chunks).arrayBuffer());
+        let binary = ''; for (const byte of bytes) binary += String.fromCharCode(byte);
+        resolve(btoa(binary));
+      };
+      probe.recorder.stop();
+    })`
+  })
+  if (typeof recording.result.value !== 'string') {
+    throw new Error('No audio recording')
+  }
+  writeFileSync(
+    testInfo.outputPath('decoded-audio.webm'),
+    Buffer.from(recording.result.value, 'base64')
+  )
+  writeFileSync(
+    testInfo.outputPath('playback-results.json'),
+    JSON.stringify(
+      { baseline, first, second, elapsedMs: Date.now() - started, players: count.result.value },
+      null,
+      2
+    )
+  )
+  await orcaPage.screenshot({ path: testInfo.outputPath('notification-settings.png') })
+  await testInfo.attach('decoded notification audio', {
+    path: testInfo.outputPath('decoded-audio.webm'),
+    contentType: 'audio/webm'
+  })
+  await cdp.detach()
+})