diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.test.ts b/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.test.ts new file mode 100644 index 000000000..55ff60506 --- /dev/null +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.test.ts @@ -0,0 +1,75 @@ +/** + * TT-7621 regression test for the Phrase Back Translate bootstrap. + * + * `PassageDetailGuidedPhraseRecord` runs a 250ms poll that calls + * `ensureSegments()` until it returns true. When auto-segment legitimately finds + * no boundaries (e.g. audio the silence math cannot split) `ensureSegments` + * returned false forever, so the poll — and its effect churn — never stopped. + * + * With audio actually loaded (duration > 0) it must instead fall back to a + * single full-length segment and return true, so the step can settle. Returning + * false stays correct only while the player has no audio yet. + */ +import { renderHook, act } from '@testing-library/react'; + +jest.mock('../Internalization/useProjectSegmentSave', () => ({ + useProjectSegmentSave: () => jest.fn().mockResolvedValue(undefined), +})); + +import { useGuidedPhraseSegments } from './useGuidedPhraseSegments'; +import { hasPhraseRegions } from './carefulSpeechBoundary'; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +function makeControls(overrides: Record = {}): any { + return { + current: { + isReady: () => true, + getDuration: () => 10, + runAutoSegment: jest.fn().mockResolvedValue(0), + getRegionsJson: () => '{}', + loadRegionsJson: jest.fn(), + applyRegionColors: jest.fn(), + ...overrides, + }, + }; +} + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +const mediafile: any = { id: 'v1', attributes: { segments: '[]' } }; + +describe('useGuidedPhraseSegments.ensureSegments (TT-7621)', () => { + it('falls back to one full-length segment when auto-segment finds none', async () => { + const controls = makeControls(); + const { result } = renderHook(() => + useGuidedPhraseSegments(mediafile, controls, { + namedRegion: 'BT:en', + persistSegments: true, + }) + ); + + let ok: boolean | undefined; + await act(async () => { + ok = await result.current.ensureSegments(); + }); + + expect(ok).toBe(true); + expect(hasPhraseRegions(result.current.phraseSegString)).toBe(true); + }); + + it('still returns false while the player has no audio loaded', async () => { + const controls = makeControls({ getDuration: () => 0 }); + const { result } = renderHook(() => + useGuidedPhraseSegments(mediafile, controls, { + namedRegion: 'BT:en', + persistSegments: true, + }) + ); + + let ok: boolean | undefined; + await act(async () => { + ok = await result.current.ensureSegments(); + }); + + expect(ok).toBe(false); + }); +}); diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.ts b/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.ts index dd3b851ce..3b22643c6 100644 --- a/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.ts +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.ts @@ -170,19 +170,22 @@ export function useGuidedPhraseSegments( ); regionJson = ctrl.getRegionsJson?.() ?? '{}'; if (!hasPhraseRegions(regionJson) && (count ?? 0) <= 0) { - // Claude's suggestion for possible future implementation: auto-segment can legitimately yield nothing (e.g. audio - // the silence math can't split), and returning false leaves the - // 250ms bootstrap poll in PassageDetailGuidedPhraseRecord spinning - // forever. Consider falling back to createSingleSegmentJson() here - // when getDuration() > 0, and returning false only while the player - // has no audio loaded yet. - return false; + // Auto-segment can legitimately yield nothing (e.g. audio the silence + // math can't split). Fall back to one full-length segment so the + // 250ms bootstrap poll in PassageDetailGuidedPhraseRecord can stop; + // returning false there left it - and its effect churn - spinning + // forever (TT-7621). createSingleSegmentJson returns false only while + // the player has no audio yet, which is the one case we still defer. + const single = createSingleSegmentJson(); + if (!single) return false; + regionJson = single; + } else { + const toSave = regionsJsonFromList( + parseRegions(regionJson).regions, + boldDefaultSegParams + ); + regionJson = toSave; } - const toSave = regionsJsonFromList( - parseRegions(regionJson).regions, - boldDefaultSegParams - ); - regionJson = toSave; } allSegs = (await persistSegmentBucket(namedRegion, regionJson, allSegs)) ?? diff --git a/src/renderer/src/crud/useFetchMediaBlob.test.ts b/src/renderer/src/crud/useFetchMediaBlob.test.ts new file mode 100644 index 000000000..3ca7e8d99 --- /dev/null +++ b/src/renderer/src/crud/useFetchMediaBlob.test.ts @@ -0,0 +1,109 @@ +/** + * TT-7621 regression tests for the reference-audio blob loader. + * + * Two ways `useFetchMediaBlob` could leave the Phrase Back Translate step's top + * player stuck on "Loading..." forever (context `loading` never clears because + * `fetching.current` is never reset): + * + * 1. The signed URL resolves but the download is an error page (S3/CloudFront + * returns HTML/XML with a 200). The old code dispatched neither FETCHED nor + * ERROR for a text/html|application/xml blob, so `blobStat` stayed PENDING. + * 2. A persistently-403 object drove an unbounded RESET->PENDING->403 loop, + * re-issuing a signed-URL request and a blob GET on every turn (the network + * storm in the hung-PBT report), and never reaching a terminal state. + * + * Both must end in ERROR so the caller can stop waiting. + */ +import { renderHook, act, waitFor } from '@testing-library/react'; + +const mockMediaClean = { + status: 0, + error: null, + url: '', + id: '', + remoteId: '', + cancelled: false, +}; + +// eslint-disable-next-line prefer-const +let mockMediaState: typeof mockMediaClean = { ...mockMediaClean }; +const mockFetchMediaUrl = jest.fn(); +// eslint-disable-next-line @typescript-eslint/no-explicit-any +let mockLoadBlob: (url: string, cb: (u: string, b?: Blob) => void) => void; + +jest.mock('./useFetchMediaUrl', () => ({ + __esModule: true, + mediaClean: mockMediaClean, + default: () => ({ + fetchMediaUrl: mockFetchMediaUrl, + mediaState: mockMediaState, + }), +})); + +jest.mock('../context/useGlobal', () => ({ + useGlobal: () => [undefined, () => {}], +})); + +jest.mock('../utils/loadBlob', () => ({ + loadBlob: (url: string, cb: (u: string, b?: Blob) => void) => + mockLoadBlob(url, cb), +})); + +import { useFetchMediaBlob, BlobStatus } from './useFetchMediaBlob'; + +beforeEach(() => { + mockMediaState = { ...mockMediaClean }; + mockFetchMediaUrl.mockReset(); +}); + +describe('useFetchMediaBlob (TT-7621)', () => { + it('dispatches ERROR (not a permanent PENDING) when the download is an error page', async () => { + mockLoadBlob = (url, cb) => + cb(url, new Blob(['nope'], { type: 'application/xml' })); + + const { result, rerender } = renderHook(() => useFetchMediaBlob()); + act(() => { + result.current[1]('m1'); + }); + + // The signed URL arrives; the effect now runs loadBlob against it. + mockMediaState = { + ...mockMediaClean, + id: 'm1', + url: 'https://s3.invalid/x.wav', + }; + act(() => { + rerender(); + }); + + await waitFor(() => + expect(result.current[0].blobStat).toBe(BlobStatus.ERROR) + ); + }); + + it('reaches a terminal ERROR after bounded 403 retries instead of looping forever', async () => { + mockLoadBlob = (_url, cb) => cb('403 Forbidden', undefined); + + const { result, rerender } = renderHook(() => useFetchMediaBlob()); + act(() => { + result.current[1]('m1'); + }); + + // Feed a fresh signed URL each turn, exactly as a real re-request would, and + // let the RESET/PENDING cycle run. It must converge, not spin. + for (let i = 0; i < 16; i++) { + mockMediaState = { + ...mockMediaClean, + id: 'm1', + url: `https://s3.invalid/x.wav?sig=${i}`, + }; + // eslint-disable-next-line no-await-in-loop + await act(async () => { + rerender(); + }); + if (result.current[0].blobStat === BlobStatus.ERROR) break; + } + + expect(result.current[0].blobStat).toBe(BlobStatus.ERROR); + }); +}); diff --git a/src/renderer/src/crud/useFetchMediaBlob.ts b/src/renderer/src/crud/useFetchMediaBlob.ts index 41c747e6e..760ececb1 100644 --- a/src/renderer/src/crud/useFetchMediaBlob.ts +++ b/src/renderer/src/crud/useFetchMediaBlob.ts @@ -1,4 +1,4 @@ -import { useEffect, useReducer, useState } from 'react'; +import { useEffect, useReducer, useRef, useState } from 'react'; import useFetchMediaUrl, { IMediaState, mediaClean } from './useFetchMediaUrl'; import { useGlobal } from '../context/useGlobal'; import { loadBlob } from '../utils/loadBlob'; @@ -69,11 +69,21 @@ const stateReducer = (state: IBlobState, action: Action): IBlobState => { } }; +/** + * A 403 on the signed URL means it expired: we drop the URL and re-request a + * fresh one (RESET -> PENDING). But a URL that keeps coming back 403 - a genuine + * permission problem, not expiry - would loop that forever, re-issuing a + * signed-URL request and a blob GET every turn (part of the TT-7621 network + * storm). Cap the re-requests, then surface the error. + */ +const MAX_URL_RESETS = 3; + export const useFetchMediaBlob = () => { const [reporter] = useGlobal('errorReporter'); const [mediaId, setMediaId] = useState(''); const { fetchMediaUrl, mediaState } = useFetchMediaUrl(reporter); const [state, dispatch] = useReducer(stateReducer, blobClean); + const resetTriesRef = useRef(0); const fetchBlob = (url: string) => { setMediaId(url); @@ -82,6 +92,14 @@ export const useFetchMediaBlob = () => { type retValue = [IBlobState, typeof fetchBlob]; useEffect(() => { + resetTriesRef.current = 0; + if (!mediaId) { + // Nothing requested yet - stay IDLE rather than PENDING, so a consumer + // that reads its loading state from blobStat === PENDING does not show a + // spurious spinner before the first fetchBlob (Copilot). + dispatch({ type: BlobStatus.IDLE, payload: undefined }); + return; + } fetchMediaUrl({ id: mediaId }); dispatch({ type: BlobStatus.PENDING, @@ -96,6 +114,12 @@ export const useFetchMediaBlob = () => { loadBlob(mediaState.url, (urlOrError, blob) => { if (!blob) { if (urlOrError.includes('403')) { + if (resetTriesRef.current >= MAX_URL_RESETS) { + // Not expiry - the object keeps 403ing. Stop re-requesting. + dispatch({ type: BlobStatus.ERROR, payload: urlOrError }); + return; + } + resetTriesRef.current += 1; fetchMediaUrl({ id: '' }); dispatch({ type: BlobStatus.RESET, payload: mediaState }); } else { @@ -105,8 +129,18 @@ export const useFetchMediaBlob = () => { } // we have a blob blob if (blob.type !== 'text/html' && blob.type !== 'application/xml') { + resetTriesRef.current = 0; const url = urlOrError; dispatch({ type: BlobStatus.FETCHED, payload: { url, blob } }); + } else { + // An HTML/XML body is an error page (S3/CDN), not audio. Terminate + // instead of leaving blobStat PENDING forever, which stranded the + // reference player on "Loading..." (TT-7621). Name the unexpected + // content type so the logged error is actionable (Copilot). + dispatch({ + type: BlobStatus.ERROR, + payload: `unexpected content type ${blob.type}: ${urlOrError}`, + }); } }); } catch (errorResult: unknown) {