diff --git a/mobile/src/hooks/mobile-dictation-desktop-start.test.ts b/mobile/src/hooks/mobile-dictation-desktop-start.test.ts index 476ba1bf9a3..6fdd66a2354 100644 --- a/mobile/src/hooks/mobile-dictation-desktop-start.test.ts +++ b/mobile/src/hooks/mobile-dictation-desktop-start.test.ts @@ -124,7 +124,7 @@ describe('startMobileDictationDesktopSession', () => { }) }) - it('closes it even when the failure is not the current start to report', async () => { + it('leaves the capture alone when the failure is no longer the current start', async () => { let setNewerStart = () => undefined const harness = createStartHarness({ sendRequest: async (method) => { @@ -139,9 +139,15 @@ describe('startMobileDictationDesktopSession', () => { await expect(startMobileDictationDesktopSession(harness.options)).resolves.toBe(false) - // A start nobody will hear about still opened a microphone, and the screen does not care - // which generation held it. - expect(harness.rollbackRecordingStart).toHaveBeenCalledOnce() + // There is one capture seam and it carries no start identity, so a stale rejection rolling it + // back would stop whatever dictation replaced this one and hand back the screen it holds. By + // the time the generation moved, the capture was either already ended — `cancel`, `stop`, the + // unmount, a failed dictation — or belongs to a newer start. + expect(harness.rollbackRecordingStart).not.toHaveBeenCalled() + // Its own desktop session is still cancelled, which is the part that is this start's to undo. + expect(harness.sendRequest).toHaveBeenCalledWith('speech.dictation.cancel', { + dictationId: 'dictation-a' + }) }) it('does not surface a desktop-start failure after the start became stale', async () => { diff --git a/mobile/src/hooks/mobile-dictation-desktop-start.ts b/mobile/src/hooks/mobile-dictation-desktop-start.ts index 049f6cc633c..a8826c74279 100644 --- a/mobile/src/hooks/mobile-dictation-desktop-start.ts +++ b/mobile/src/hooks/mobile-dictation-desktop-start.ts @@ -56,16 +56,21 @@ export async function startMobileDictationDesktopSession( dictationSessionStart.interpret(reply) } catch (err) { const wasCurrent = isCurrentStart(options) - // The hook opened the capture before this ran, and an open microphone holds the screen. No - // session started, so both go back — through the same rollback the commit failure below uses, - // because "undo the capture this start opened" is one thing and the hook owns it. Before the - // screen moved onto the mic this path leaked only an idle audio session; now it would pin the - // display until the user cancelled, retried, or the screen unmounted. - try { - options.rollbackRecordingStart() - } catch { - // Guarded for the reason the commit arm below is: a seam that throws on the way down must - // not take the desktop cancel with it, nor replace the failure the caller is about to see. + // The hook opened the capture before this ran, and an open microphone holds the screen, so a + // failure that is still this start's gives both back — through the same rollback the commit + // failure below uses, because "undo the capture this start opened" is one thing the hook owns. + // + // Only while it is still current, though: there is one capture seam and it carries no start + // identity. A stale rejection — A opened the capture, the user cancelled, B is recording — + // would end B's microphone and hand back B's screen. Past the generation, the capture was + // already ended by whatever superseded this start, or belongs to the one that did. + if (wasCurrent) { + try { + options.rollbackRecordingStart() + } catch { + // Guarded for the reason the commit arm below is: a seam that throws on the way down must + // not take the desktop cancel with it, nor replace the failure the caller is about to see. + } } options.clearActiveId(dictationId) await dictationSessionCancel.request(client, { dictationId }).catch(() => undefined) diff --git a/mobile/src/hooks/mobile-dictation-native-start-failure.test.tsx b/mobile/src/hooks/mobile-dictation-native-start-failure.test.tsx index 639845e0866..1ab69d5eb37 100644 --- a/mobile/src/hooks/mobile-dictation-native-start-failure.test.tsx +++ b/mobile/src/hooks/mobile-dictation-native-start-failure.test.tsx @@ -9,7 +9,11 @@ import { createElement } from 'react' import { act, create } from 'react-test-renderer' import { beforeEach, describe, expect, it, vi } from 'vitest' -import { createFakeRpcClient, type FakeRpcClient } from '../mobile-web-shell/bridge-host-test-fakes' +import { + createFakeRpcClient, + type FakeRpcClient, + type SentRequest +} from '../mobile-web-shell/bridge-host-test-fakes' const device = vi.hoisted(() => ({ /** Every engine call, in order. */ @@ -73,6 +77,25 @@ function dictation(): UseMobileDictationResult { return current } +/** Answers everything but the one method a case wants to keep in flight, which it collects. */ +async function pumpHolding( + rpc: FakeRpcClient, + hold: string, + held: { request: SentRequest | null } +): Promise { + for (let round = 0; round < 8; round += 1) { + for (const request of rpc.requests.splice(0)) { + if (request.method === hold && held.request === null) { + held.request = request + continue + } + request.resolve({ id: 'desktop', ok: true, result: {} }) + } + await Promise.resolve() + await Promise.resolve() + } +} + /** Answers every forwarded request, refusing the one the case names. */ async function pump(rpc: FakeRpcClient, refuse: string): Promise { for (let round = 0; round < 8; round += 1) { @@ -128,3 +151,86 @@ describe('a native start the desktop refuses', () => { expect(device.calls).not.toContain('initialize') }) }) + +describe('a stale native start whose refusal arrives after a newer one is recording', () => { + it('leaves the newer dictation recording, with the microphone and the screen still its own', async () => { + const rpc = createFakeRpcClient() + mount(rpc) + const first: { request: SentRequest | null } = { request: null } + // A opens the microphone and waits on the desktop. + await act(async () => { + void dictation() + .start() + .catch(() => undefined) + await pumpHolding(rpc, 'speech.dictation.start', first) + }) + expect(device.screen).toEqual(['+']) + // The user gives up on A, then starts B, which takes the microphone and the screen again. + await act(async () => { + const cancelled = dictation().cancel() + await pump(rpc, 'none') + await cancelled + }) + await act(async () => { + const started = dictation() + .start() + .catch(() => undefined) + await pump(rpc, 'none') + await started + }) + expect(dictation().status).toBe('recording') + const screenWhileRecording = [...device.screen] + const callsWhileRecording = [...device.calls] + // Now A's request finally fails. It owns nothing: the capture and the screen are B's. + await act(async () => { + first.request?.resolve({ + id: 'desktop', + ok: false, + error: { code: 'refused', message: 'no model installed' } + }) + await pump(rpc, 'none') + }) + expect(dictation().status).toBe('recording') + expect(device.screen).toEqual(screenWhileRecording) + expect(device.calls).toEqual(callsWhileRecording) + }) +}) + +describe('a stale native start that succeeds after a newer one is recording', () => { + it('does not commit itself over the dictation that replaced it', async () => { + // The mirror image of the case above: A's request resolves rather than rejects. The stale + // check after the desktop start is what stops A committing, and `cancelStaleStart` cancels A's + // own session without touching the capture — which is B's. + const rpc = createFakeRpcClient() + mount(rpc) + const first: { request: SentRequest | null } = { request: null } + await act(async () => { + void dictation() + .start() + .catch(() => undefined) + await pumpHolding(rpc, 'speech.dictation.start', first) + }) + await act(async () => { + const cancelled = dictation().cancel() + await pump(rpc, 'none') + await cancelled + }) + await act(async () => { + const started = dictation() + .start() + .catch(() => undefined) + await pump(rpc, 'none') + await started + }) + expect(dictation().status).toBe('recording') + const screenWhileRecording = [...device.screen] + const callsWhileRecording = [...device.calls] + await act(async () => { + first.request?.resolve({ id: 'desktop', ok: true, result: { started: true } }) + await pump(rpc, 'none') + }) + expect(dictation().status).toBe('recording') + expect(device.screen).toEqual(screenWhileRecording) + expect(device.calls).toEqual(callsWhileRecording) + }) +}) diff --git a/mobile/src/session/mobile-dictation-mic-control.web.test.tsx b/mobile/src/session/mobile-dictation-mic-control.web.test.tsx index c114f069ac8..eba17305ea9 100644 --- a/mobile/src/session/mobile-dictation-mic-control.web.test.tsx +++ b/mobile/src/session/mobile-dictation-mic-control.web.test.tsx @@ -80,6 +80,10 @@ function createAudioShell(): (verb: BridgeNativeVerb, params: unknown) => Promis return (verb, params) => capture.serve(verb, params) } +/** The hook the composer holds, for a case that has to act on a state the button does not offer: + * the cancel affordance only exists once a dictation is recording or processing. */ +const composer: { dictation: ReturnType | null } = { dictation: null } + function Composer({ pair }: { pair: BridgePortPair }): ReactElement { const dictation = useMobileDictation({ client: pair.client, @@ -87,6 +91,7 @@ function Composer({ pair }: { pair: BridgePortPair }): ReactElement { onTranscript: () => {}, onError: (error) => reported.push(error.message) }) + composer.dictation = dictation return ( { +/** What the desktop answers a forwarded request with. `null` leaves it in flight for the case to + * settle later; the default is a plain success. */ +type DesktopReply = { id: string ok: boolean result?: unknown error?: unknown } +type DesktopAnswer = (method: string) => DesktopReply | null const DESKTOP_OK: DesktopAnswer = () => ({ id: 'desktop', ok: true, result: {} }) +/** Requests a case left unanswered, in the order the page sent them. */ +const pending: { method: string; resolve: (reply: DesktopReply) => void }[] = [] + type MicControl = { readonly label: () => string readonly tap: (answer?: DesktopAnswer) => Promise + /** Drives the bridge and the desktop without pressing anything, for a case acting on the hook. */ + readonly settle: (answer?: DesktopAnswer) => Promise } /** The mic button, found by the label it carries in every state rather than by position. */ @@ -158,23 +170,34 @@ async function mount(pair: BridgePortPair): Promise { if (rendered === null) { throw new Error('nothing mounted') } + // The tap crosses the bridge, the shell answers, and the desktop answers what was forwarded. + async function settle(answer: DesktopAnswer = DESKTOP_OK): Promise { + for (let round = 0; round < 4; round += 1) { + await act(async () => { + await pair.flush() + for (const request of pair.rpc.requests.splice(0)) { + const reply = answer(request.method) + if (reply === null) { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the fake client's resolver takes the reply shape the host would have sent. + pending.push({ method: request.method, resolve: request.resolve as never }) + continue + } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the fake client takes the reply shape the host would have sent, which is what a case builds here. + request.resolve(reply as never) + } + await pair.flush() + }) + } + } + return { label: () => String(micOf(rendered.root).props.accessibilityLabel), + settle, tap: async (answer: DesktopAnswer = DESKTOP_OK) => { await act(async () => { micOf(rendered.root).props.onPress() }) - // The tap crosses the bridge, the shell answers, and the desktop answers what was forwarded. - for (let round = 0; round < 4; round += 1) { - await act(async () => { - await pair.flush() - for (const request of pair.rpc.requests.splice(0)) { - // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the fake client takes the reply shape the host would have sent, which is what a case builds here. - request.resolve(answer(request.method) as never) - } - await pair.flush() - }) - } + await settle(answer) } } } @@ -182,6 +205,8 @@ async function mount(pair: BridgePortPair): Promise { beforeEach(() => { reported.length = 0 screen.length = 0 + pending.length = 0 + composer.dictation = null }) afterEach(() => { @@ -281,3 +306,40 @@ describe('the mic control on a page the shell did grant audio', () => { ).toBe(true) }) }) + +describe('a stale page start whose refusal arrives after a newer one is recording', () => { + it('leaves the newer dictation recording, with the shell capture and the screen still its own', async () => { + const shell = createAudioShell() + const verbs: string[] = [] + const pair = createFakeBridgePortPair({ + serveNativeVerb: (verb, params) => { + verbs.push(verb) + return shell(verb, params) + } + }) + const mic = await mount(pair) + // A opens the shell's microphone and waits on the desktop. + await mic.tap((method) => (method === 'speech.dictation.start' ? null : DESKTOP_OK(method))) + expect(screen).toEqual(['+']) + const first = pending.find((request) => request.method === 'speech.dictation.start') + expect(first).toBeDefined() + // The user gives up on A while it is still starting, which is a state the button has no + // affordance for, and then starts B. + await act(async () => { + void composer.dictation?.cancel() + }) + await mic.settle() + await mic.tap() + expect(mic.label()).toBe('Stop voice dictation') + const verbsWhileRecording = [...verbs] + const screenWhileRecording = [...screen] + // A's request finally fails. It owns nothing: the capture and the screen are B's. + await act(async () => { + first?.resolve({ id: 'desktop', ok: false, error: { code: 'refused', message: 'no model' } }) + }) + await mic.settle() + expect(mic.label()).toBe('Stop voice dictation') + expect(verbs).toEqual(verbsWhileRecording) + expect(screen).toEqual(screenWhileRecording) + }) +})