From 801f111f616f146b70627158863d62cedd712b37 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Mon, 31 Aug 2026 18:58:07 -0400 Subject: [PATCH 1/4] TT-7643 fix: keep a Phrase BT step's audio out of the next language's step A team can configure one Phrase Back Translation step per language, and every one of them renders the same PassageDetailGuidedPhraseRecord against the same vernacular audio. The mobile route rendered that component unkeyed, so moving between two PBT steps reused the instance - and the component resets its per-step state only when the vernacular mediafile changes, which it does not here. The recording pass, the segment it was parked on, the baseline boundaries, the optimistic greens and the take mounted in its recorder all carried into the next language's step, which then played the previous language's audio. The route now keys the guided-record steps on currentstep, the way the desktop grid already does with and the way the adjacent transcription steps were keyed for TT-7503. The component also scopes its own reset to the step as well as the mediafile, so a reused instance still opens the next step from scratch. The legacy claim had a second cross-language path: it stamped every untagged take of its artifact type with the opening step's language. That premise only holds for data recorded before takes carried a language, in a project working in one language. Once the passage has Phrase BT boundaries for another language an untagged take could belong to either step, so the claim is now skipped there rather than handing the take to whichever step happened to open. Co-Authored-By: Claude Opus 5 (1M context) --- src/renderer/cypress/support/pbtHarness.tsx | 89 ++++-- ...etailGuidedPhraseRecord.stepScope.test.tsx | 265 ++++++++++++++++++ .../PassageDetailGuidedPhraseRecord.tsx | 30 +- .../PassageDetailPhraseBackTranslate.cy.tsx | 90 ++++++ .../carefulSpeech/claimLegacyPhraseBt.test.ts | 67 +++++ .../carefulSpeech/claimLegacyPhraseBt.ts | 42 +++ src/renderer/src/routes/PassageDetail.tsx | 22 +- 7 files changed, 575 insertions(+), 30 deletions(-) create mode 100644 src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx diff --git a/src/renderer/cypress/support/pbtHarness.tsx b/src/renderer/cypress/support/pbtHarness.tsx index 725d14045..cf3efb42c 100644 --- a/src/renderer/cypress/support/pbtHarness.tsx +++ b/src/renderer/cypress/support/pbtHarness.tsx @@ -100,6 +100,14 @@ export const STEP_BCP47 = 'en'; /** Named-region bucket PBT stores its boundaries in (phraseBtBoundaryRegionName). */ export const BT_REGION_NAME = `BT:${STEP_BCP47}`; +function parseBcp47FromLanguageField(value: string): string { + const trimmed = String(value ?? '').trim(); + if (!trimmed) return 'und'; + const pipe = trimmed.indexOf('|'); + if (pipe === -1) return trimmed; + return trimmed.slice(pipe + 1) || 'und'; +} + /** Fake S3 host the intercepted POST hands back for the audio PUT. */ const FAKE_AUDIO_HOST = 'https://pbt-test.invalid'; @@ -144,6 +152,8 @@ interface ServerState { generation: number; /** Outstanding lagged writes, so a reset can cancel them. */ pendingTimers: ReturnType[]; + /** ids requested through GET /mediafiles/:id/fileurl (in request order). */ + fileurlRequestedIds: string[]; } const serverState: ServerState = { @@ -154,6 +164,7 @@ const serverState: ServerState = { nextRemoteId: 1000, generation: 0, pendingTimers: [], + fileurlRequestedIds: [], }; /** Reset the fake server. Call from beforeEach before installing intercepts. */ @@ -168,6 +179,7 @@ export function resetPbtServer(options?: { serverState.pendingTimers = []; serverState.generation += 1; serverState.takes = []; + serverState.fileurlRequestedIds = []; serverState.nextRemoteId = 1000; serverState.putDelayMs = options?.putDelayMs ?? 0; serverState.fileurlDelayMs = options?.fileurlDelayMs ?? 0; @@ -182,6 +194,11 @@ export function postedTakes(): PostedTake[] { return serverState.takes; } +/** mediafile ids requested by MediaRecord load calls (GET /mediafiles/:id/fileurl). */ +export function fileurlRequestedIds(): string[] { + return serverState.fileurlRequestedIds; +} + /** Make later uploads fail (used mid-spec for save-failure paths). */ export function failNextUploads(putStatus = 500) { serverState.failPutWithStatus = putStatus; @@ -269,6 +286,10 @@ export function installPbtServer() { // Existing-take load (useFetchUrlNow → GET mediafiles//fileurl). cy.intercept('GET', '**/mediafiles/*/fileurl', (req) => { + const match = /\/mediafiles\/([^/]+)\/fileurl(?:\?|$)/.exec(req.url); + if (match?.[1]) { + serverState.fileurlRequestedIds.push(decodeURIComponent(match[1])); + } req.reply({ statusCode: 200, delay: serverState.fileurlDelayMs, @@ -378,6 +399,15 @@ export interface MountPbtOptions { segments?: SegmentSpec[]; /** Indices that already have a saved take when the step opens. */ existingTakes?: number[]; + /** Exact seeded takes for multi-language and duplicate-segment cases. */ + existingTakeRows?: Array<{ + segmentIndex: number; + languagebcp47: string; + remoteId: string; + performedBy?: string; + }>; + /** Step language stamped in Step Settings (`Name|bcp47`). */ + stepLanguage?: string; /** Source audio length (s). Must cover the last segment end. */ durationSec?: number; /** ms before an uploaded take shows up in rowData (0 = immediate). */ @@ -419,7 +449,7 @@ function interiorBoundaries( .filter((t) => t > 0.01 && t < durationSec - 0.01); } -function segmentsAttribute(segments: SegmentSpec[]): string { +function segmentsAttribute(segments: SegmentSpec[], bcp47: string): string { const regions: IRegion[] = segments.map((s) => ({ start: s.start, end: s.end, @@ -427,7 +457,7 @@ function segmentsAttribute(segments: SegmentSpec[]): string { })); return JSON.stringify([ { - name: BT_REGION_NAME, + name: `BT:${bcp47}`, regionInfo: regionsJsonFromList(regions, boldDefaultSegParams), }, ]); @@ -437,7 +467,8 @@ function takeRecord( id: string, remoteId: string, sourceSegments: string, - performedBy: string | null + performedBy: string | null, + languagebcp47 = STEP_LANGUAGE ): MediaFileD { return { type: 'mediafile', @@ -450,7 +481,7 @@ function takeRecord( originalFile: `${id}.ogg`, audioUrl: `${FAKE_AUDIO_HOST}/audio/${id}.wav`, sourceSegments, - languagebcp47: STEP_LANGUAGE, + languagebcp47, performedBy, dateCreated: new Date(2026, 0, 1).toISOString(), segments: '[]', @@ -467,10 +498,12 @@ function takeRecord( function seedRecords(memory: Memory, options: MountPbtOptions) { const segments = options.segments ?? []; + const stepLanguage = options.stepLanguage ?? STEP_LANGUAGE; + const stepBcp47 = parseBcp47FromLanguageField(stepLanguage); const stepTool = JSON.stringify({ tool: 'phraseBackTranslate', settings: JSON.stringify({ - language: STEP_LANGUAGE, + language: stepLanguage, artifactTypeId: ARTIFACT_TYPE_ID, }), }); @@ -536,7 +569,7 @@ function seedRecords(memory: Memory, options: MountPbtOptions) { contentType: 'audio/wav', originalFile: 'vern.wav', audioUrl: `${FAKE_AUDIO_HOST}/audio/vern.wav`, - segments: segmentsAttribute(segments), + segments: segmentsAttribute(segments, stepBcp47), transcription: '', }, relationships: { @@ -546,19 +579,37 @@ function seedRecords(memory: Memory, options: MountPbtOptions) { }, ]; - (options.existingTakes ?? []).forEach((idx) => { - const seg = segments[idx]; - if (!seg) return; - records.push( - takeRecord( - `mf-take-${idx}`, - String(500 + idx), - JSON.stringify({ start: seg.start, end: seg.end, label: '' }), - 'Existing Speaker' - // eslint-disable-next-line @typescript-eslint/no-explicit-any - ) as any - ); - }); + if ((options.existingTakeRows?.length ?? 0) > 0) { + options.existingTakeRows?.forEach((row) => { + const seg = segments[row.segmentIndex]; + if (!seg) return; + records.push( + takeRecord( + `mf-take-${row.remoteId}`, + row.remoteId, + JSON.stringify({ start: seg.start, end: seg.end, label: '' }), + row.performedBy ?? 'Existing Speaker', + row.languagebcp47 + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ) as any + ); + }); + } else { + (options.existingTakes ?? []).forEach((idx) => { + const seg = segments[idx]; + if (!seg) return; + records.push( + takeRecord( + `mf-take-${idx}`, + String(500 + idx), + JSON.stringify({ start: seg.start, end: seg.end, label: '' }), + 'Existing Speaker', + stepLanguage + // eslint-disable-next-line @typescript-eslint/no-explicit-any + ) as any + ); + }); + } memory.cache.update((t) => // eslint-disable-next-line @typescript-eslint/no-explicit-any diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx new file mode 100644 index 000000000..32e9387c7 --- /dev/null +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx @@ -0,0 +1,265 @@ +import { act, cleanup, render, waitFor } from '@testing-library/react'; +import { IRegion } from '../../crud/useWavesurferRegions'; + +/** + * TT-7643 - a team can configure one Phrase Back Translation step per language, + * and every one of them renders this same component against the same vernacular + * audio. Where the route does not remount it between steps (the mobile route + * did not), the instance is reused, and everything it holds for the step just + * left - the recording pass, the clause it is parked on, and the take mounted + * in its recorder - carried into the next language's step, so that step played + * the other language's audio. + * + * The component now scopes its own reset to the step as well as the mediafile, + * so a reused instance still opens the next step from scratch. + */ + +const regions: IRegion[] = [ + { start: 0, end: 10, label: '' }, + { start: 10, end: 20, label: '' }, + { start: 20, end: 30, label: '' }, +]; + +let mockCompleted = new Set(); +let controlsProps: Record | undefined; +let mockRecordingRow: + | { mediafile: { id: string; attributes?: Record } } + | undefined; + +/** Step settings by step id: two PBT steps, different languages. */ +const stepSettings: Record> = { + 'step-sena': { artifactTypeId: 'art1', language: 'Sena|seh' }, + 'step-hebrew': { artifactTypeId: 'art1', language: 'Hebrew|he' }, +}; + +const stubControls = { + isReady: jest.fn(() => true), + isPlaying: jest.fn(() => false), + gotoTime: jest.fn().mockResolvedValue(undefined), + setPlay: jest.fn(), + applyRegionColors: jest.fn(), + loadRegionsJson: jest.fn(), +}; + +const ctx: { + _seg: IRegion | undefined; + currentSegmentIndex: number; + currentstep: string; + [k: string]: unknown; +} = { + _seg: regions[0], + currentSegmentIndex: 0, + currentstep: 'step-sena', + passage: { id: 'p1', type: 'passage' }, + playerMediafile: { id: 'm1', type: 'mediafile' }, + mediafileId: 'm1', + rowData: [], + section: { id: 's1', type: 'section' }, + setPlaying: jest.fn(), + setRecording: jest.fn(), + forceRefresh: jest.fn(), + getCurrentSegment: jest.fn(() => ctx._seg), + isBoldWorkflow: false, + carefulSpeechSegParams: {}, + setCarefulSpeechSegParams: jest.fn(), + setStepComplete: jest.fn().mockResolvedValue(undefined), + stepComplete: jest.fn(() => false), + setCurrentSegment: jest.fn((region: IRegion) => { + ctx._seg = region; + ctx.currentSegmentIndex += 1; + }), +}; + +jest.mock('../../context/usePassageDetailContext', () => () => ctx); + +const mockResetForMediafile = jest.fn(); +jest.mock('./carefulSpeech/useGuidedPhraseSegments', () => ({ + useGuidedPhraseSegments: () => ({ + phraseSegString: '[]', + setPhraseSegString: jest.fn(), + bootstrapped: true, + ensureSegments: jest.fn().mockResolvedValue(true), + resetForMediafile: mockResetForMediafile, + resegmentWithParams: jest.fn().mockResolvedValue(false), + resetToDefaultSegments: jest.fn().mockResolvedValue(false), + persistPhraseSegments: jest.fn().mockResolvedValue(undefined), + }), +})); + +jest.mock('../../utils/namedSegments', () => { + const actual = jest.requireActual('../../utils/namedSegments'); + return { ...actual, getSortedRegions: jest.fn(() => regions) }; +}); + +jest.mock('./carefulSpeech/carefulSpeechCompletion', () => { + const actual = jest.requireActual('./carefulSpeech/carefulSpeechCompletion'); + return { + ...actual, + getCompletedClauseIndices: jest.fn(() => mockCompleted), + getRecordingForClause: jest.fn(() => mockRecordingRow), + }; +}); + +jest.mock('../../crud', () => ({ + ArtifactTypeSlug: { PhraseBackTranslation: 'phrase-back-translation' }, + remoteIdGuid: jest.fn((_t: string, id: string) => id), + useArtifactType: () => ({ getTypeId: () => 'art1' }), + useStepTool: (step: string) => ({ settings: stepSettings[step] ?? {} }), +})); +jest.mock('../../crud/related', () => ({ related: () => 'p1' })); +jest.mock('../../utils/useStepPermission', () => ({ + useStepPermissions: () => ({ canDoSectionStep: () => true }), +})); +jest.mock('../../utils/passageDefaultFilename', () => ({ + passageDefaultFilename: () => 'file.ogg', +})); +jest.mock('../../selector', () => ({ + sharedSelector: jest.fn(), + mediaTabSelector: jest.fn(), + mediaTitleSelector: jest.fn(), +})); +jest.mock('react-redux', () => ({ + useSelector: () => ({ + uploadFailed: 'Upload Failed!', + pendingUploadRetryOne: 'Retry', + }), + shallowEqual: jest.fn(), +})); +jest.mock('../../context/useGlobal', () => ({ + useGlobal: (key: string) => + key === 'memory' + ? [ + { keyMap: {}, update: jest.fn().mockResolvedValue(undefined) }, + jest.fn(), + ] + : [undefined, jest.fn()], +})); +jest.mock('../../context/UnsavedContext', () => { + const ReactActual = jest.requireActual('react'); + return { + UnsavedContext: ReactActual.createContext({ + state: { + startSave: jest.fn(), + waitForSave: jest.fn().mockResolvedValue(undefined), + }, + }), + }; +}); +jest.mock('../../hoc/useOrbitData', () => ({ + useOrbitData: () => [ + { id: 'm1', type: 'mediafile', attributes: { versionNumber: 1 } }, + ], +})); + +jest.mock('./PassageDetailPlayer', () => ({ + __esModule: true, + default: (props: Record) => { + const ref = props.controlsRef as { current: unknown } | undefined; + if (ref) ref.current = stubControls; + return
; + }, +})); + +jest.mock('./carefulSpeech/CarefulSpeechControls', () => ({ + __esModule: true, + default: (props: Record) => { + controlsProps = props; + return
; + }, +})); + +// imported after the mocks so the component picks them up +import { PassageDetailGuidedPhraseRecord } from './PassageDetailGuidedPhraseRecord'; +import { phraseBackTranslateConfig } from './guidedPhraseRecord/types'; +import { ArtifactTypeSlug } from '../../crud/artifactTypeSlug'; +import { NamedRegions } from '../../utils/namedSegments'; + +const config = phraseBackTranslateConfig( + ArtifactTypeSlug.PhraseBackTranslation, + NamedRegions.BackTranslation +); + +const strings = { + allComplete: 'All segments recorded', + unitLabel: 'Segment: {0}', + clearRecording: 'Clear', + combineWithNext: 'Combine', + fewerUnits: 'Fewer', + moreUnits: 'More', + nextUnit: 'Next', + splitUnit: 'Split', + speaker: 'Speaker', + startRecording: 'Start', + undo: 'Undo', + noStepLanguage: 'Configure a language', +}; + +const ui = () => ( + +); + +const mountAndSettle = async () => { + const utils = render(ui()); + await waitFor(() => expect(controlsProps).toBeDefined()); + await waitFor(() => expect(stubControls.gotoTime).toHaveBeenCalled()); + return utils; +}; + +beforeEach(() => { + mockCompleted = new Set(); + controlsProps = undefined; + mockRecordingRow = undefined; + ctx._seg = regions[0]; + ctx.currentSegmentIndex = 0; + ctx.currentstep = 'step-sena'; + jest.clearAllMocks(); + stubControls.isReady.mockReturnValue(true); + stubControls.isPlaying.mockReturnValue(false); + stubControls.gotoTime.mockResolvedValue(undefined); +}); + +afterEach(() => cleanup()); + +describe('PassageDetailGuidedPhraseRecord - step scope (TT-7643)', () => { + it('opens the next language step from scratch when the instance is reused', async () => { + // Sena: every segment recorded, so the step opens in review mode with the + // recorder mounted on Sena's take. + mockCompleted = new Set([0, 1, 2]); + mockRecordingRow = { mediafile: { id: 'sena-take-1' } }; + const { rerender } = await mountAndSettle(); + expect(controlsProps?.recordingPassStarted).toBe(true); + expect(controlsProps?.showRecorder).toBe(true); + expect(controlsProps?.recordingMediaId).toBe('sena-take-1'); + + // Move to the Hebrew step without unmounting - nothing is recorded there. + mockCompleted = new Set(); + mockRecordingRow = undefined; + ctx.currentstep = 'step-hebrew'; + await act(async () => { + rerender(ui()); + }); + + await waitFor(() => + expect(controlsProps?.recordingPassStarted).toBe(false) + ); + expect(controlsProps?.showRecorder).toBe(false); + expect(controlsProps?.recordingMediaId).toBeUndefined(); + // Segment boundaries are per language: the new step re-reads its own. + expect(mockResetForMediafile).toHaveBeenCalledTimes(2); + }); + + it('records against the language of the step now showing', async () => { + const { rerender } = await mountAndSettle(); + expect(controlsProps?.languagebcp47).toBe('Sena|seh'); + + ctx.currentstep = 'step-hebrew'; + await act(async () => { + rerender(ui()); + }); + await waitFor(() => expect(controlsProps?.languagebcp47).toBe('Hebrew|he')); + }); +}); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index bbcf9ad38..ce7bbe42f 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -196,7 +196,7 @@ export function PassageDetailGuidedPhraseRecord({ ); const bootstrapPollRef = useRef | null>(null); const bootstrapCompletedRef = useRef(false); - const lastResetMediafileRef = useRef(undefined); + const lastResetScopeRef = useRef(undefined); const initialPositionDoneRef = useRef(false); const suppressClauseAutoPlayRef = useRef(0); const playClauseInFlightRef = useRef(false); @@ -838,9 +838,22 @@ export function PassageDetailGuidedPhraseRecord({ // mid-entry, clobbering an already-started recording pass and dropping the // user back into the listen pass (TT-7360). Only reset once per actual // mediafile change. - if (lastResetMediafileRef.current === mediafileId) return; - lastResetMediafileRef.current = mediafileId; + // + // The step is part of that identity, not just the mediafile: a team can + // configure a Phrase BT step per language, and every one of them renders + // this same component against the same vernacular. Where the route does not + // remount it between steps, keying the reset on the mediafile alone carried + // the previous language's recording pass - its clause index, its baseline + // boundaries, its optimistic greens, and the take showing in the recorder - + // into the next language's step (TT-7643). + if (!currentstep) return; + const stepScope = `${mediafileId ?? ''}|${currentstep}`; + if (lastResetScopeRef.current === stepScope) return; + lastResetScopeRef.current = stepScope; resetForMediafile(mediafileId); + // The legacy claim is per step language, so a reused instance has to be + // allowed to run it again for the step just moved to. + claimRanRef.current = false; bootstrapCompletedRef.current = false; setPhase('bootstrapping'); setCurrentIndex(0); @@ -865,12 +878,13 @@ export function PassageDetailGuidedPhraseRecord({ setEntryPositioned(false); suppressClauseAutoPlayRef.current = 0; setHighlightPlayButton(false); - // Gate only on the stable mediafileId string. resetForMediafile's identity - // changes whenever the mediafile record is updated (e.g. persisting combined - // clause segments), which would otherwise re-fire this reset and drop the - // user from the recording pass back into the listen pass (TT-7360). + // Gate only on the stable mediafileId / currentstep strings. + // resetForMediafile's identity changes whenever the mediafile record is + // updated (e.g. persisting combined clause segments), which would otherwise + // re-fire this reset and drop the user from the recording pass back into + // the listen pass (TT-7360). // eslint-disable-next-line react-hooks/exhaustive-deps - }, [mediafileId]); + }, [mediafileId, currentstep]); useEffect(() => { if (!mediafileId || !stepEnabled) return; diff --git a/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx b/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx index 40de12702..a4c36dc80 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx @@ -13,6 +13,7 @@ import { mountPbt, waitForPbtReady, + fileurlRequestedIds, postedTakes, waitForUploads, expectSegmentColors, @@ -217,3 +218,92 @@ describe('PBT out-of-order recording', () => { }); }); }); + +/** + * A team can configure one PBT step per language, so the same passage carries + * takes from several of them, recorded against the very same segment. A step + * must only ever see - and play - its own language's takes (TT-7643). + */ +describe('PBT language scoping', () => { + it('loads this step language take when another language has one too', () => { + mountPbt({ + segments: SEGMENTS, + stepLanguage: 'Hebrew|he', + existingTakeRows: [ + // Higher remote id than the Hebrew take, so a chooser that ignores + // language would land on the Sena one. + { + segmentIndex: 0, + languagebcp47: 'Sena|seh', + remoteId: '999', + performedBy: 'Sena Speaker', + }, + { + segmentIndex: 0, + languagebcp47: 'Hebrew|he', + remoteId: '101', + performedBy: 'Hebrew Speaker', + }, + // Hebrew is finished, so the step opens in review mode on segment 1 + // with its take mounted in the recorder - the moment the wrong take + // would become audible. + { + segmentIndex: 1, + languagebcp47: 'Hebrew|he', + remoteId: '102', + performedBy: 'Hebrew Speaker', + }, + { + segmentIndex: 2, + languagebcp47: 'Hebrew|he', + remoteId: '103', + performedBy: 'Hebrew Speaker', + }, + ], + }); + waitForPbtReady(); + + cy.wrap(null, { timeout: 20000 }).should(() => { + expect( + fileurlRequestedIds().length, + 'a take was loaded' + ).to.be.greaterThan(0); + }); + cy.then(() => { + expect( + fileurlRequestedIds(), + 'only this step language was ever fetched' + ).to.not.include('999'); + expect(fileurlRequestedIds()[0]).to.equal('101'); + }); + }); + + it('treats another language take as no take at all', () => { + mountPbt({ + segments: SEGMENTS, + stepLanguage: 'Hebrew|he', + existingTakeRows: [ + { + segmentIndex: 0, + languagebcp47: 'Sena|seh', + remoteId: '999', + performedBy: 'Sena Speaker', + }, + ], + }); + waitForPbtReady(); + + // Nothing recorded in Hebrew yet: the listen pass, not review mode. + cy.get(PBT.start).should('be.visible'); + expectSegmentColors([ + SEGMENT_COLOR.current, + SEGMENT_COLOR.pending, + SEGMENT_COLOR.pending, + ]); + cy.then(() => + expect(fileurlRequestedIds(), 'no foreign take fetched').to.not.include( + '999' + ) + ); + }); +}); diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/claimLegacyPhraseBt.test.ts b/src/renderer/src/components/PassageDetail/carefulSpeech/claimLegacyPhraseBt.test.ts index 0b913433c..a18ac6801 100644 --- a/src/renderer/src/components/PassageDetail/carefulSpeech/claimLegacyPhraseBt.test.ts +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/claimLegacyPhraseBt.test.ts @@ -54,6 +54,73 @@ describe('planLegacyPhraseBtClaim', () => { expect(segs).toContain(phraseBtBoundaryRegionName('fr')); }); + it('leaves untagged takes alone once a second language has boundaries', () => { + // Sena has already been back-translated on this passage, so an untagged + // take may be Sena's. The Hebrew step must not adopt it (TT-7643). + const multiLang = { + ...vern, + attributes: { + segments: updateSegments( + phraseBtBoundaryRegionName('seh'), + vern.attributes.segments, + JSON.stringify({ params: {}, regions: [{ start: 0, end: 4 }] }) + ), + }, + }; + const result = planLegacyPhraseBtClaim({ + languageName: 'Hebrew', + languageBcp47: 'he', + artifactTypeId: 'art1', + vernacularMedia: [multiLang], + outputMedia: [untagged, tagged], + }); + expect(result.languageUpdates.size).toBe(0); + // The step still gets its own boundaries seeded from the legacy bucket. + expect(result.segmentUpdates.has('v1')).toBe(true); + }); + + it('still claims when only this language has boundaries', () => { + const mine = { + ...vern, + attributes: { + segments: updateSegments( + phraseBtBoundaryRegionName('he'), + vern.attributes.segments, + JSON.stringify({ params: {}, regions: [{ start: 0, end: 4 }] }) + ), + }, + }; + const result = planLegacyPhraseBtClaim({ + languageName: 'Hebrew', + languageBcp47: 'he', + artifactTypeId: 'art1', + vernacularMedia: [mine], + outputMedia: [untagged], + }); + expect(result.languageUpdates.get('p1')).toBe('Hebrew|he'); + }); + + it('ignores an empty bucket for another language', () => { + const emptyOther = { + ...vern, + attributes: { + segments: updateSegments( + phraseBtBoundaryRegionName('seh'), + vern.attributes.segments, + JSON.stringify({ params: {}, regions: [] }) + ), + }, + }; + const result = planLegacyPhraseBtClaim({ + languageName: 'Hebrew', + languageBcp47: 'he', + artifactTypeId: 'art1', + vernacularMedia: [emptyOther], + outputMedia: [untagged], + }); + expect(result.languageUpdates.get('p1')).toBe('Hebrew|he'); + }); + it('does not overwrite an existing language bucket', () => { const already = { ...vern, diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/claimLegacyPhraseBt.ts b/src/renderer/src/components/PassageDetail/carefulSpeech/claimLegacyPhraseBt.ts index 9f5a982ae..87fc0d950 100644 --- a/src/renderer/src/components/PassageDetail/carefulSpeech/claimLegacyPhraseBt.ts +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/claimLegacyPhraseBt.ts @@ -1,5 +1,6 @@ import { related } from '../../../crud/related'; import { MediaFileD } from '../../../model'; +import { tryParseJSON } from '../../../utils/tryParseJson'; import { getSegments, NamedRegions, @@ -29,9 +30,45 @@ export interface IClaimLegacyPhraseBtResult { segmentUpdates: Map; } +const BUCKET_PREFIX = phraseBtBoundaryRegionName(''); + +/** + * True when a vernacular already carries Phrase BT boundaries for some language + * other than `bcp47` - i.e. this passage is already being back-translated into + * more than one language. + */ +function hasOtherLanguageBoundaries( + vernacularMedia: MediaFileD[], + bcp47: string +): boolean { + const mine = phraseBtBoundaryRegionName(bcp47).toLowerCase(); + return vernacularMedia.some((v) => { + const all = v.attributes?.segments ?? '[]'; + const parsed = tryParseJSON(all); + if (!Array.isArray(parsed)) return false; + return parsed.some((entry) => { + const name = String((entry as { name?: unknown })?.name ?? ''); + const lower = name.toLowerCase(); + if (!lower.startsWith(BUCKET_PREFIX.toLowerCase())) return false; + if (lower === mine) return false; + return hasPhraseRegions(getSegments(name, all)); + }); + }); +} + /** * Claim untagged outputs and copy legacy `BT` into `BT:${bcp47}` when empty. * Does not touch Retell or other artifact types. + * + * Claiming is for data recorded before takes were stamped with a language, + * which by definition belongs to whichever single language the team was working + * in. Once a passage has boundaries for a second language the premise is gone: + * an untagged take could belong to either step, and handing it to whichever one + * happened to open played one language's audio in the other's step (TT-7643). + * So the language claim is skipped there - an unclaimed take stays out of every + * step rather than joining the wrong one. Copying legacy boundaries into this + * language's empty bucket is unaffected: boundaries are per language and the + * copy never overwrites an existing bucket. */ export function planLegacyPhraseBtClaim( args: IClaimLegacyPhraseBtArgs @@ -43,8 +80,13 @@ export function planLegacyPhraseBtClaim( const languageUpdates = new Map(); const segmentUpdates = new Map(); const bucket = phraseBtBoundaryRegionName(args.languageBcp47); + const multiLanguage = hasOtherLanguageBoundaries( + args.vernacularMedia, + args.languageBcp47 + ); for (const m of args.outputMedia) { + if (multiLanguage) break; if (related(m, 'artifactType') !== args.artifactTypeId) continue; const existing = parseMediaLanguageBcp47(m.attributes?.languagebcp47); if (existing === 'und' || !m.attributes?.languagebcp47) { diff --git a/src/renderer/src/routes/PassageDetail.tsx b/src/renderer/src/routes/PassageDetail.tsx index b53adb8c2..8906673fc 100644 --- a/src/renderer/src/routes/PassageDetail.tsx +++ b/src/renderer/src/routes/PassageDetail.tsx @@ -81,11 +81,27 @@ const MobileStep = () => { ) : tool === ToolSlug.Verses ? ( ) : tool === ToolSlug.CarefulSpeech ? ( - + // Keyed for the same reason as the transcription steps below: a team can + // configure several guided-record steps in a row, and they all render the + // one PassageDetailGuidedPhraseRecord component. (TT-7643) + ) : tool === ToolSlug.PhraseBackTranslate && isBoldWorkflow ? ( - + ) : tool === ToolSlug.PhraseBackTranslate && !isBoldWorkflow ? ( - + // A team can have one Phrase Back Translation step per language. Without a + // key the reused instance carries the previous step's recording pass - + // including the take loaded in its recorder - into the next language's + // step, so the other language's audio plays there. (TT-7643) + ) : boldClauseTranscription ? ( // Key on currentstep so the shared transcription component remounts when // moving between the adjacent Careful- and LWC-Transcription steps. Desktop From f54d7cbd229cf7abd7a09e78d7083581d7edd0c6 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Mon, 31 Aug 2026 19:56:38 -0400 Subject: [PATCH 2/4] TT-7643 refactor: reuse the shared language-field and BT-bucket helpers in the PBT harness parseMediaLanguageBcp47 and phraseBtBoundaryRegionName already own the `Name|bcp47` parse and the `BT:` bucket name; the harness had grown its own copies of both. Co-Authored-By: Claude Opus 5 (1M context) --- src/renderer/cypress/support/pbtHarness.tsx | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/renderer/cypress/support/pbtHarness.tsx b/src/renderer/cypress/support/pbtHarness.tsx index cf3efb42c..0400a1e98 100644 --- a/src/renderer/cypress/support/pbtHarness.tsx +++ b/src/renderer/cypress/support/pbtHarness.tsx @@ -60,6 +60,8 @@ import type { MediaFileD } from '../../src/model'; import { boldDefaultSegParams } from '../../src/components/PassageDetail/carefulSpeech/boldCarefulSpeechSegParams'; import { regionsJsonFromList } from '../../src/components/PassageDetail/carefulSpeech/carefulSpeechBoundary'; import { prettySegment } from '../../src/utils/prettySegment'; +import { parseMediaLanguageBcp47 } from '../../src/utils/mediaLanguage'; +import { phraseBtBoundaryRegionName } from '../../src/components/PassageDetail/carefulSpeech/matchesGuidedOutputRow'; import PassageDetailPhraseBackTranslate from '../../src/components/PassageDetail/PassageDetailPhraseBackTranslate'; // --------------------------------------------------------------------------- @@ -97,16 +99,8 @@ export const USER_ID = 'user-1'; export const ORG_ID = 'org-1'; export const STEP_LANGUAGE = 'English|en'; export const STEP_BCP47 = 'en'; -/** Named-region bucket PBT stores its boundaries in (phraseBtBoundaryRegionName). */ -export const BT_REGION_NAME = `BT:${STEP_BCP47}`; - -function parseBcp47FromLanguageField(value: string): string { - const trimmed = String(value ?? '').trim(); - if (!trimmed) return 'und'; - const pipe = trimmed.indexOf('|'); - if (pipe === -1) return trimmed; - return trimmed.slice(pipe + 1) || 'und'; -} +/** Named-region bucket PBT stores its boundaries in. */ +export const BT_REGION_NAME = phraseBtBoundaryRegionName(STEP_BCP47); /** Fake S3 host the intercepted POST hands back for the audio PUT. */ const FAKE_AUDIO_HOST = 'https://pbt-test.invalid'; @@ -457,7 +451,7 @@ function segmentsAttribute(segments: SegmentSpec[], bcp47: string): string { })); return JSON.stringify([ { - name: `BT:${bcp47}`, + name: phraseBtBoundaryRegionName(bcp47), regionInfo: regionsJsonFromList(regions, boldDefaultSegParams), }, ]); @@ -499,7 +493,7 @@ function takeRecord( function seedRecords(memory: Memory, options: MountPbtOptions) { const segments = options.segments ?? []; const stepLanguage = options.stepLanguage ?? STEP_LANGUAGE; - const stepBcp47 = parseBcp47FromLanguageField(stepLanguage); + const stepBcp47 = parseMediaLanguageBcp47(stepLanguage); const stepTool = JSON.stringify({ tool: 'phraseBackTranslate', settings: JSON.stringify({ From 088b87a1c9afbc0fa28e6c8810b19b685d910b07 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Mon, 31 Aug 2026 20:10:16 -0400 Subject: [PATCH 3/4] TT-7643 fix: re-read phrase boundaries when the step's language bucket changes useGuidedPhraseSegments guarded its own reset on the mediafile id, so a step change on the same vernacular was rejected as a no-op: bootstrapped stayed true and ensureSegments short-circuited on the phrase string already loaded. The next language's step opened on the previous language's boundaries even though the parent had asked for a reset. The bucket is part of that scope, not just the audio - each Phrase BT step reads its own BT: regions off the same vernacular - so the guard now keys on both, and the function is named resetForScope to say so. Covered by a test against the real hook rather than the mocked one (Devin review). Co-Authored-By: Claude Opus 5 (1M context) --- .../PassageDetailCarefulSpeech.test.tsx | 2 +- ...etailGuidedPhraseRecord.stepScope.test.tsx | 11 ++- .../PassageDetailGuidedPhraseRecord.tsx | 6 +- .../useGuidedPhraseSegments.test.tsx | 81 +++++++++++++++++++ .../carefulSpeech/useGuidedPhraseSegments.ts | 23 ++++-- 5 files changed, 109 insertions(+), 14 deletions(-) create mode 100644 src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.test.tsx diff --git a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx index a1323a799..4e7127fc2 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx @@ -102,7 +102,7 @@ jest.mock('./carefulSpeech/useGuidedPhraseSegments', () => ({ setPhraseSegString: jest.fn(), bootstrapped: true, ensureSegments: jest.fn().mockResolvedValue(true), - resetForMediafile: jest.fn(), + resetForScope: jest.fn(), resegmentWithParams: jest.fn().mockResolvedValue(false), resetToDefaultSegments: jest.fn().mockResolvedValue(false), persistPhraseSegments: jest.fn().mockResolvedValue(undefined), diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx index 32e9387c7..13730d6f8 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx @@ -72,14 +72,14 @@ const ctx: { jest.mock('../../context/usePassageDetailContext', () => () => ctx); -const mockResetForMediafile = jest.fn(); +const mockResetForScope = jest.fn(); jest.mock('./carefulSpeech/useGuidedPhraseSegments', () => ({ useGuidedPhraseSegments: () => ({ phraseSegString: '[]', setPhraseSegString: jest.fn(), bootstrapped: true, ensureSegments: jest.fn().mockResolvedValue(true), - resetForMediafile: mockResetForMediafile, + resetForScope: mockResetForScope, resegmentWithParams: jest.fn().mockResolvedValue(false), resetToDefaultSegments: jest.fn().mockResolvedValue(false), persistPhraseSegments: jest.fn().mockResolvedValue(undefined), @@ -248,8 +248,11 @@ describe('PassageDetailGuidedPhraseRecord - step scope (TT-7643)', () => { ); expect(controlsProps?.showRecorder).toBe(false); expect(controlsProps?.recordingMediaId).toBeUndefined(); - // Segment boundaries are per language: the new step re-reads its own. - expect(mockResetForMediafile).toHaveBeenCalledTimes(2); + // The boundaries the new step opens on are the segment hook's business, + // and this suite mocks that hook - so whether the reset it is handed + // actually re-reads the next language's bucket is asserted against the + // real hook in useGuidedPhraseSegments.test.tsx, not here. + expect(mockResetForScope).toHaveBeenCalledWith('m1'); }); it('records against the language of the step now showing', async () => { diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index ce7bbe42f..2ec36c835 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -388,7 +388,7 @@ export function PassageDetailGuidedPhraseRecord({ setPhraseSegString: setClauseSegString, bootstrapped, ensureSegments, - resetForMediafile, + resetForScope, resegmentWithParams, resetToDefaultSegments, persistPhraseSegments: persistClauseSegments, @@ -850,7 +850,7 @@ export function PassageDetailGuidedPhraseRecord({ const stepScope = `${mediafileId ?? ''}|${currentstep}`; if (lastResetScopeRef.current === stepScope) return; lastResetScopeRef.current = stepScope; - resetForMediafile(mediafileId); + resetForScope(mediafileId); // The legacy claim is per step language, so a reused instance has to be // allowed to run it again for the step just moved to. claimRanRef.current = false; @@ -879,7 +879,7 @@ export function PassageDetailGuidedPhraseRecord({ suppressClauseAutoPlayRef.current = 0; setHighlightPlayButton(false); // Gate only on the stable mediafileId / currentstep strings. - // resetForMediafile's identity changes whenever the mediafile record is + // resetForScope's identity changes whenever the mediafile record is // updated (e.g. persisting combined clause segments), which would otherwise // re-fire this reset and drop the user from the recording pass back into // the listen pass (TT-7360). diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.test.tsx b/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.test.tsx new file mode 100644 index 000000000..df92bafbc --- /dev/null +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.test.tsx @@ -0,0 +1,81 @@ +import { act, renderHook } from '@testing-library/react'; +import { createRef } from 'react'; +import { MediaFileD } from '../../../model'; +import { WSAudioPlayerControls } from '../../WSAudioPlayer'; +import { phraseBtBoundaryRegionName } from './matchesGuidedOutputRow'; + +/** + * TT-7643 - a Phrase BT step per language reads its own `BT:` boundary + * bucket off the same vernacular audio. The reset has to key on that bucket as + * well as the mediafile, or a step change is rejected as "same audio, nothing + * to do" and the next language opens on the previous language's boundaries. + */ + +jest.mock('../Internalization/useProjectSegmentSave', () => ({ + useProjectSegmentSave: () => jest.fn().mockResolvedValue(undefined), +})); + +import { useGuidedPhraseSegments } from './useGuidedPhraseSegments'; + +const bucket = (name: string, regions: { start: number; end: number }[]) => ({ + name, + regionInfo: JSON.stringify({ params: {}, regions }), +}); + +const SENA = [{ start: 0, end: 11 }]; +const HEBREW = [ + { start: 0, end: 4 }, + { start: 4, end: 9 }, +]; + +const mediafile = { + id: 'mf-vern', + type: 'mediafile', + attributes: { + segments: JSON.stringify([ + bucket(phraseBtBoundaryRegionName('seh'), SENA), + bucket(phraseBtBoundaryRegionName('he'), HEBREW), + ]), + }, + relationships: {}, +} as unknown as MediaFileD; + +const controlsRef = createRef() as React.RefObject< + WSAudioPlayerControls | undefined +> as React.RefObject; + +const regionsOf = (json: string) => + (JSON.parse(json) as { regions?: { start: number; end: number }[] }).regions; + +describe('useGuidedPhraseSegments - reset scope', () => { + it('re-reads boundaries when the language bucket changes on the same audio', () => { + const { result, rerender } = renderHook( + ({ namedRegion }: { namedRegion: string }) => + useGuidedPhraseSegments(mediafile, controlsRef, { namedRegion }), + { initialProps: { namedRegion: phraseBtBoundaryRegionName('seh') } } + ); + + act(() => result.current.resetForScope(mediafile.id)); + expect(regionsOf(result.current.phraseSegString)).toEqual(SENA); + + // Same vernacular, next language's step. + rerender({ namedRegion: phraseBtBoundaryRegionName('he') }); + act(() => result.current.resetForScope(mediafile.id)); + expect(regionsOf(result.current.phraseSegString)).toEqual(HEBREW); + }); + + it('stays put when neither the audio nor the bucket moved', () => { + const { result } = renderHook(() => + useGuidedPhraseSegments(mediafile, controlsRef, { + namedRegion: phraseBtBoundaryRegionName('he'), + }) + ); + + act(() => result.current.resetForScope(mediafile.id)); + act(() => result.current.setPhraseSegString('{"params":{},"regions":[]}')); + // A repeat call must not undo work done since - the guard that keeps + // StrictMode's double-invoke from clobbering a started pass (TT-7360). + act(() => result.current.resetForScope(mediafile.id)); + expect(regionsOf(result.current.phraseSegString)).toEqual([]); + }); +}); diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.ts b/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.ts index dd3b851ce..4e72f2c89 100644 --- a/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.ts +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/useGuidedPhraseSegments.ts @@ -50,7 +50,7 @@ export function useGuidedPhraseSegments( const [phraseSegString, setPhraseSegString] = useState('{}'); const [bootstrapped, setBootstrapped] = useState(false); const bootstrapInProgress = useRef(false); - const mediafileIdRef = useRef(undefined); + const scopeRef = useRef(undefined); const persistSegmentBucket = useCallback( async ( @@ -88,16 +88,27 @@ export function useGuidedPhraseSegments( if (hasPhraseRegions(regionJson)) setPhraseSegString(regionJson); }, [mediafile, persistSegments, singleSegmentMode, readRegionJson]); - const resetForMediafile = useCallback( + /** + * Re-read boundaries for the scope now showing, unless it is the one already + * loaded. + * + * The bucket is part of that scope, not just the audio: a team can configure + * a Phrase BT step per language, and each reads its own `BT:` + * boundaries off the same vernacular. Keyed on the mediafile alone, a step + * change was rejected as a no-op and the next language opened on the previous + * language's phrase boundaries (TT-7643). + */ + const resetForScope = useCallback( (mediafileId: string | undefined) => { - if (mediafileIdRef.current === mediafileId) return; - mediafileIdRef.current = mediafileId; + const scope = `${mediafileId ?? ''}|${namedRegion}`; + if (scopeRef.current === scope) return; + scopeRef.current = scope; bootstrapInProgress.current = false; setBootstrapped(false); setPhraseSegString('{}'); if (mediafileId) hydrateFromMediafile(); }, - [hydrateFromMediafile] + [hydrateFromMediafile, namedRegion] ); const loadRegionsOnPlayer = useCallback( @@ -283,7 +294,7 @@ export function useGuidedPhraseSegments( setPhraseSegString, bootstrapped, ensureSegments, - resetForMediafile, + resetForScope, resegmentWithParams, resetToDefaultSegments, persistPhraseSegments: (regionJson: string) => From 8a81b097392e73f893b1a55aeee7651a01580fb6 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Tue, 1 Sep 2026 13:22:54 -0400 Subject: [PATCH 4/4] TT-7643 fix: name a Phrase BT take for its step language so it cannot collide The real cause of PBT audio from one language playing in another: on the desktop app a take's audio is resolved by FILE NAME, not by id. useFetchMediaUrl calls dataPath(mediafile.audioUrl, PathType.MEDIA), which maps to /media/ and hands back whatever file is already sitting there. The upload name is passageDefaultFilename + buildFilenamePostfix, which carried the passage, the segment index and the source version but not the step language. A team can configure one Phrase BT step per language, and every one of them records the same segment of the same vernacular - so Hebrew segment 1 uploaded under exactly the name Sena segment 1 had already cached, and the first file cached is what played for both. buildFilenamePostfix now takes the step bcp47 and appends it. Steps with no configured language (Careful Speech) keep the names they always had, so existing files are untouched. Also fixes a harness defect that hid this: the fake server stamped every uploaded take English|en rather than the language that was posted. Note the earlier commits on this branch do not fix the reported defect - the desktop route already remounted the step on currentstep. They stay because they are correct on their own terms: the mobile route lacked that parity, and the segment hook's reset had to key on the language bucket as well as the audio. Co-Authored-By: Claude Opus 5 (1M context) --- src/renderer/cypress/support/pbtHarness.tsx | 6 ++++- ...etailGuidedPhraseRecord.stepScope.test.tsx | 26 ++++++++++++++++++- .../PassageDetailGuidedPhraseRecord.tsx | 7 ++++- .../PassageDetailPhraseBackTranslate.cy.tsx | 25 ++++++++++++++++++ .../guidedPhraseRecord/types.test.ts | 24 +++++++++++++++++ .../PassageDetail/guidedPhraseRecord/types.ts | 26 +++++++++++++++---- 6 files changed, 106 insertions(+), 8 deletions(-) diff --git a/src/renderer/cypress/support/pbtHarness.tsx b/src/renderer/cypress/support/pbtHarness.tsx index 0400a1e98..6e643e9af 100644 --- a/src/renderer/cypress/support/pbtHarness.tsx +++ b/src/renderer/cypress/support/pbtHarness.tsx @@ -738,7 +738,11 @@ function PbtHarnessInner({ options, memory, blob }: HarnessProps) { id, take.remoteId, take.sourceSegments, - take.performedBy + take.performedBy, + // The real pull-after-upload returns what was posted; stamping + // every uploaded take English hid whether the step language ever + // reached the upload at all. + take.languagebcp47 // eslint-disable-next-line @typescript-eslint/no-explicit-any ) as any ) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx index 13730d6f8..952e79675 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx @@ -110,8 +110,11 @@ jest.mock('../../crud/related', () => ({ related: () => 'p1' })); jest.mock('../../utils/useStepPermission', () => ({ useStepPermissions: () => ({ canDoSectionStep: () => true }), })); +// Echo the postfix the config built, so a spec can see what the take would be +// named. Real signature: (passage, plan, memory, artifactType, offline, postfix). jest.mock('../../utils/passageDefaultFilename', () => ({ - passageDefaultFilename: () => 'file.ogg', + passageDefaultFilename: (...args: unknown[]) => + `GEN001_014-019${args[5]}_plan`, })); jest.mock('../../selector', () => ({ sharedSelector: jest.fn(), @@ -255,6 +258,27 @@ describe('PassageDetailGuidedPhraseRecord - step scope (TT-7643)', () => { expect(mockResetForScope).toHaveBeenCalledWith('m1'); }); + it('names a take so it cannot collide with another language of the same segment', async () => { + // The uploaded file name is what the media cache is keyed on: `dataPath` + // resolves a mediafile's audioUrl to `/media/`, so + // two takes that upload under one name share one cached file and the first + // one downloaded is what plays. Segment index and source version were in + // the name but the step language was not, so Hebrew segment 1 and Sena + // segment 1 were the same file (TT-7643). + const { rerender } = await mountAndSettle(); + const senaName = controlsProps?.defaultFilename as string; + expect(senaName).toContain('seh'); + + ctx.currentstep = 'step-hebrew'; + await act(async () => { + rerender(ui()); + }); + await waitFor(() => + expect(controlsProps?.defaultFilename).not.toEqual(senaName) + ); + expect(controlsProps?.defaultFilename).toContain('he'); + }); + it('records against the language of the step now showing', async () => { const { rerender } = await mountAndSettle(); expect(controlsProps?.languagebcp47).toBe('Sena|seh'); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index 2ec36c835..79ade9f69 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -520,7 +520,11 @@ export function PassageDetailGuidedPhraseRecord({ ); const defaultFilename = useMemo(() => { - const postfix = config.buildFilenamePostfix(currentIndex, currentVersion); + const postfix = config.buildFilenamePostfix( + currentIndex, + currentVersion, + stepLanguageBcp47 + ); return passageDefaultFilename( passage, plan, @@ -537,6 +541,7 @@ export function PassageDetailGuidedPhraseRecord({ offline, currentIndex, currentVersion, + stepLanguageBcp47, config, ]); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx b/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx index a4c36dc80..88099205c 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx @@ -306,4 +306,29 @@ describe('PBT language scoping', () => { ) ); }); + + it('uploads a take under a name no other language can take', () => { + // The name a take uploads under is the name its audio is cached under on + // disk: dataPath resolves a mediafile's audioUrl to + // `/media/` and hands back whatever file is already + // sitting there. The name carried the segment index and the source version + // but not the language, so the Hebrew step's segment 1 uploaded as exactly + // the name the Sena step's segment 1 had already cached - and Sena is what + // played (TT-7643). + mountPbt({ segments: SEGMENTS, stepLanguage: 'Hebrew|he' }); + waitForPbtReady(); + startRecordingPass(); + recordAndSettle(1); + + cy.then(() => { + const posted = postedTakes()[0]; + expect(posted?.languagebcp47, 'take is stamped').to.equal('Hebrew|he'); + expect(posted?.originalFile, 'name carries the language').to.contain( + '_he' + ); + // What the Sena step would have uploaded for this same segment. + const senaName = (posted?.originalFile ?? '').replace('_he', '_seh'); + expect(posted?.originalFile).to.not.equal(senaName); + }); + }); }); diff --git a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts index dd92412a3..1fb961201 100644 --- a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts +++ b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts @@ -54,4 +54,28 @@ describe('guidedPhraseRecord config', () => { expect(config.buildFilenamePostfix(0, 2)).toBe('backtranslation1_v2'); expect(config.buildFilenamePostfix(1, 2)).toBe('backtranslation2_v2s1'); }); + + it('buildFilenamePostfix separates the languages of the same segment', () => { + // Media is cached on disk under the uploaded file's name (dataPath maps a + // mediafile's audioUrl to `/media/`), so a name + // shared by two takes means one cached file for both, and the first one + // cached is what plays. A Phrase BT step per language records the same + // segment of the same vernacular, so the language has to be in the name + // (TT-7643). + const config = phraseBackTranslateConfig( + ArtifactTypeSlug.PhraseBackTranslation, + NamedRegions.BackTranslation + ); + expect(config.buildFilenamePostfix(0, 1, 'seh')).toBe( + 'backtranslation1_v1_seh' + ); + expect(config.buildFilenamePostfix(0, 1, 'he')).toBe( + 'backtranslation1_v1_he' + ); + expect(config.buildFilenamePostfix(1, 1, 'he')).toBe( + 'backtranslation2_v1s1_he' + ); + // Steps with no configured language keep the names they always had. + expect(config.buildFilenamePostfix(0, 1)).toBe('backtranslation1_v1'); + }); }); diff --git a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts index bc05feb9d..b582628bf 100644 --- a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts +++ b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts @@ -45,8 +45,21 @@ export interface GuidedPhraseRecordConfig { sequentialUnitNavAroundRecord: boolean; /** Persist segment map on vernacular named regions (false for Retell). */ persistSegments: boolean; - /** Filename postfix for a unit at `unitIndex` (0-based) on `sourceVersion`. */ - buildFilenamePostfix: (unitIndex: number, sourceVersion: number) => string; + /** + * Filename postfix for a unit at `unitIndex` (0-based) on `sourceVersion`. + * + * The result has to be unique per take, not just pretty: the uploaded name + * becomes the media object's name, and `dataPath` resolves a mediafile's + * audioUrl to `/media/`. Two takes uploaded under one + * name therefore share a single cached file, and whichever was cached first + * is what plays for both. `languageBcp47` is passed for the steps that can + * have a sibling step over the same audio in another language (TT-7643). + */ + buildFilenamePostfix: ( + unitIndex: number, + sourceVersion: number, + languageBcp47?: string + ) => string; } const carefulSpeechBoundaryDefaults = { @@ -93,10 +106,13 @@ export function phraseBackTranslateConfig( multiLevelSegmentUndo: phraseBoundaryTools, sequentialUnitNavAroundRecord: phraseBoundaryTools, persistSegments: phraseBoundaryTools, - buildFilenamePostfix: (unitIndex, sourceVersion) => { + buildFilenamePostfix: (unitIndex, sourceVersion, languageBcp47) => { const base = `${artifactSlug}${unitIndex + 1}_v${sourceVersion}`; - if (unitIndex > 0) return `${base}s${unitIndex}`; - return base; + const unit = unitIndex > 0 ? `${base}s${unitIndex}` : base; + // A Phrase BT step per language records the same segment of the same + // vernacular, so without the language every one of them uploads under + // the same name. Takes made before this stay on their old names. + return languageBcp47 ? `${unit}_${languageBcp47}` : unit; }, }; }