From 5e32ae4262899780c4f9902beed9f7a847c820d3 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Tue, 1 Sep 2026 21:32:04 -0400 Subject: [PATCH 1/4] TT-7666 TT-7432 test: stale and duplicate phrase takes Failing tests first, for two defects that both come of a take not being told apart from another take of the same segment. TT-7666: a take records the slice of vernacular it covers in sourceSegments. Adjusting a segment boundary rewrites the slices, so takes made before the adjustment answer to a segment that no longer exists - and recording the moved segments again leaves both generations attached to the same vernacular. The record step only shows takes matching the boundaries it is reading, but the Transcribe task list is built from every take there is: two segments, four tasks. TT-7432: segment index, source version and step language are the whole of a take's name, and they are identical for two takes of one segment in one step. Deleting a recording and recording it again therefore uploaded the replacement under the name the deleted take is cached on, and since dataPath resolves a mediafile's audioUrl to `/media/ `, the deleted audio is what played back. Co-Authored-By: Claude Opus 5 (1M context) --- ...etailGuidedPhraseRecord.stepScope.test.tsx | 35 ++++++ .../PassageDetailTranscribe.test.tsx | 99 ++++++++++++--- .../guidedPhraseRecord/types.test.ts | 46 ++++++- src/renderer/src/crud/phraseTakes.test.ts | 115 ++++++++++++++++++ 4 files changed, 279 insertions(+), 16 deletions(-) create mode 100644 src/renderer/src/crud/phraseTakes.test.ts diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx index 952e79675..9093446c0 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx @@ -279,6 +279,41 @@ describe('PassageDetailGuidedPhraseRecord - step scope (TT-7643)', () => { expect(controlsProps?.defaultFilename).toContain('he'); }); + /** + * TT-7432 - deleting a recording and recording the segment again produced a + * second take whose name was identical to the first: segment index, source + * version and step language are all unchanged. `dataPath` resolves a + * mediafile's audioUrl to `/media/`, so the new take + * resolved to the file already cached for the deleted one and playback kept + * playing the deleted recording. + */ + it('names each take of a segment separately (TT-7432)', async () => { + mockCompleted = new Set(); + mockRecordingRow = { mediafile: { id: 'take-1' } }; + await mountAndSettle(); + const onRecording = controlsProps?.onRecording as (a: boolean) => void; + + // First take of segment 1. + await act(async () => { + onRecording(true); + }); + const firstTake = controlsProps?.defaultFilename as string; + await act(async () => { + onRecording(false); + }); + // It still says which segment and which source version it belongs to. + expect(firstTake).toContain('backtranslation1_v1'); + + // Delete it and record the same segment again. + await act(async () => { + await (controlsProps?.onClearRecording as () => Promise)(); + }); + await act(async () => { + (controlsProps?.onRecording as (a: boolean) => void)(true); + }); + expect(controlsProps?.defaultFilename).not.toEqual(firstTake); + }); + 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/PassageDetailTranscribe.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx index 2e7af2b02..454752dbc 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx @@ -1,7 +1,20 @@ import React from 'react'; import { render, screen } from '@testing-library/react'; -let captured: { hasPermission?: boolean; curRole?: string } = {}; +let captured: { + hasPermission?: boolean; + curRole?: string; + phraseRegions?: unknown; +} = {}; + +/** Per-test knobs for the phrase-segment path (TT-7666). */ +const phrase = { + isPhraseArtifact: false, + slug: 'vernacular', + regions: [] as unknown[], + /** What `related()` answers for a row's sourceMedia. */ + sourceMedia: undefined as string | undefined, +}; const linkedSharedResource = { id: 'sr1', @@ -18,7 +31,10 @@ const passageDetailCtx = { orgWorkflowSteps: [ { id: 'step-transcribe', - attributes: { sequencenum: 1, tool: '{"tool":"transcribe","settings":{}}' }, + attributes: { + sequencenum: 1, + tool: '{"tool":"transcribe","settings":{}}', + }, }, ], setStepComplete: jest.fn(), @@ -30,7 +46,10 @@ const passageDetailCtx = { sharedResource: undefined as unknown, }; -jest.mock('../../context/usePassageDetailContext', () => () => passageDetailCtx); +jest.mock( + '../../context/usePassageDetailContext', + () => () => passageDetailCtx +); jest.mock('../../context/PassageDetailContext', () => ({ PassageDetailContext: React.createContext({ setState: jest.fn() }), @@ -39,9 +58,11 @@ jest.mock('../../context/PassageDetailContext', () => ({ jest.mock('../../context/TranscriberContext', () => ({ TranscriberProvider: (props: { curRole?: string; + phraseRegions?: unknown; children?: React.ReactNode; }) => { captured.curRole = props.curRole; + captured.phraseRegions = props.phraseRegions; return <>{props.children}; }, })); @@ -83,20 +104,20 @@ jest.mock('../../crud', () => ({ jest.mock('../../crud/useArtifactType', () => ({ useArtifactType: () => ({ localizedArtifactTypeFromId: () => 'bt', - slugFromId: () => 'vernacular', + slugFromId: () => phrase.slug, }), })); jest.mock('../../crud/artifactTypeSlug', () => ({ ArtifactTypeSlug: { CarefulSpeech: 'carefulspeech' }, artifactStampsStepLanguage: () => false, - isPhraseSegmentArtifact: () => false, + isPhraseSegmentArtifact: () => phrase.isPhraseArtifact, })); jest.mock('../../crud/related', () => ({ - related: jest.fn(), + related: () => phrase.sourceMedia, __esModule: true, - default: jest.fn(), + default: () => phrase.sourceMedia, })); jest.mock('../../utils/useStepPermission', () => ({ @@ -106,7 +127,13 @@ jest.mock('../../utils/useStepPermission', () => ({ })); jest.mock('../../hoc/useOrbitData', () => ({ - useOrbitData: () => [], + useOrbitData: () => [ + { + id: 'mf1', + type: 'mediafile', + attributes: { versionNumber: 1, segments: '[]' }, + }, + ], })); jest.mock('../../context/UnsavedContext', () => { @@ -130,12 +157,12 @@ jest.mock('react-redux', () => ({ jest.mock('../../utils/namedSegments', () => ({ getSegments: () => '{}', - getSortedRegions: () => [], + getSortedRegions: () => phrase.regions, NamedRegions: { Clause: 'clause', BackTranslation: 'bt' }, })); jest.mock('./carefulSpeech/carefulSpeechBoundary', () => ({ - hasPhraseRegions: () => false, + hasPhraseRegions: () => phrase.regions.length > 0, })); jest.mock('./carefulSpeech/matchesGuidedOutputRow', () => ({ @@ -152,12 +179,19 @@ jest.mock('./boldClause/StepMessage', () => () => null); import { PassageDetailTranscribe } from './PassageDetailTranscribe'; +const resetKnobs = () => { + captured = {}; + passageDetailCtx.sharedResource = undefined; + passageDetailCtx.mediafileId = 'mf1'; + passageDetailCtx.rowData = []; + phrase.isPhraseArtifact = false; + phrase.slug = 'vernacular'; + phrase.regions = []; + phrase.sourceMedia = undefined; +}; + describe('PassageDetailTranscribe linked note (TT-5873)', () => { - beforeEach(() => { - captured = {}; - passageDetailCtx.sharedResource = undefined; - passageDetailCtx.mediafileId = 'mf1'; - }); + beforeEach(resetKnobs); it('keeps transcribe permission on the source note', () => { render(); @@ -175,3 +209,38 @@ describe('PassageDetailTranscribe linked note (TT-5873)', () => { expect(captured.curRole).toBe('view'); }); }); + +/** + * TT-7666 - the task list is built from every take attached to the vernacular, + * so the takes left behind by a segment-boundary adjustment showed up beside + * the ones recorded after it: two segments, four tasks to transcribe. Which + * takes are still current is decided against the segment boundaries the step is + * reading, so the provider has to be told what they are. + */ +describe('PassageDetailTranscribe phrase takes (TT-7666)', () => { + const clauseRegions = [ + { start: 0, end: 6, label: '' }, + { start: 6, end: 10, label: '' }, + ]; + + beforeEach(() => { + resetKnobs(); + phrase.sourceMedia = 'mf1'; + passageDetailCtx.rowData = [ + { artifactType: 'bt', mediafile: { id: 'take1', type: 'mediafile' } }, + ] as never; + }); + + it('hands the current segment boundaries to the transcriber provider', () => { + phrase.isPhraseArtifact = true; + phrase.slug = 'carefulspeech'; + phrase.regions = clauseRegions; + render(); + expect(captured.phraseRegions).toEqual(clauseRegions); + }); + + it('leaves a non-phrase artifact unscoped', () => { + render(); + expect(captured.phraseRegions ?? []).toEqual([]); + }); +}); diff --git a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts index 1fb961201..90d5daf28 100644 --- a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts +++ b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts @@ -2,7 +2,11 @@ import { describe, expect, it } from '@jest/globals'; import { NamedRegions } from '../../../utils/namedSegments'; import { LocalKey } from '../../../utils/localUserKey'; import { ArtifactTypeSlug } from '../../../crud/artifactTypeSlug'; -import { CAREFUL_SPEECH_CONFIG, phraseBackTranslateConfig } from './types'; +import { + CAREFUL_SPEECH_CONFIG, + newTakeToken, + phraseBackTranslateConfig, +} from './types'; describe('guidedPhraseRecord config', () => { it('Careful Speech uses clause regions and boundary tools', () => { @@ -78,4 +82,44 @@ describe('guidedPhraseRecord config', () => { // Steps with no configured language keep the names they always had. expect(config.buildFilenamePostfix(0, 1)).toBe('backtranslation1_v1'); }); + + it('buildFilenamePostfix separates the takes of one segment (TT-7432)', () => { + // Segment index, source version and step language are all the same for two + // takes of the same segment in the same step, so deleting a recording and + // recording it again uploaded the replacement under the name the deleted + // take is cached on, and the deleted audio is what played back. Each take + // has to bring its own token. + const config = phraseBackTranslateConfig( + ArtifactTypeSlug.PhraseBackTranslation, + NamedRegions.BackTranslation + ); + expect( + CAREFUL_SPEECH_CONFIG.buildFilenamePostfix(0, 1, undefined, 't1') + ).toBe('carefulspeech1_v1_t1'); + expect( + CAREFUL_SPEECH_CONFIG.buildFilenamePostfix(0, 1, undefined, 't2') + ).not.toBe( + CAREFUL_SPEECH_CONFIG.buildFilenamePostfix(0, 1, undefined, 't1') + ); + expect(config.buildFilenamePostfix(1, 1, 'he', 't1')).toBe( + 'backtranslation2_v1s1_he_t1' + ); + // Takes made before this stay on the names they were uploaded under. + expect(CAREFUL_SPEECH_CONFIG.buildFilenamePostfix(0, 1)).toBe( + 'carefulspeech1_v1' + ); + }); +}); + +describe('newTakeToken', () => { + it('never repeats a token, even inside one millisecond', () => { + const now = 1767225600000; + expect(newTakeToken(now)).not.toEqual(newTakeToken(now)); + }); + + it('grows with the clock so a later take sorts after an earlier one', () => { + expect(newTakeToken(1767225600000) < newTakeToken(1767225700000)).toBe( + true + ); + }); }); diff --git a/src/renderer/src/crud/phraseTakes.test.ts b/src/renderer/src/crud/phraseTakes.test.ts new file mode 100644 index 000000000..e9d3bba53 --- /dev/null +++ b/src/renderer/src/crud/phraseTakes.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it } from '@jest/globals'; +import { MediaFile } from '../model'; +import { IRegion } from './useWavesurferRegions'; +import { selectCurrentPhraseTakes } from './phraseTakes'; + +/** + * TT-7666 - a phrase segment's take records which slice of the vernacular it + * covers in `sourceSegments`. Adjusting a boundary rewrites the slices, so the + * takes made before the adjustment answer to a segment that no longer exists; + * recording the moved segments again leaves both generations attached to the + * same vernacular. The record step only ever shows takes matching the segments + * it is looking at, but the Transcribe task list showed every take there was - + * two segments, four tasks. + */ + +const take = ( + id: string, + region: { start: number; end: number } | null, + dateCreated = '2026-01-01T00:00:00Z' +): MediaFile => + ({ + id, + type: 'mediafile', + attributes: { + sourceSegments: region === null ? '' : JSON.stringify(region), + dateCreated, + }, + }) as unknown as MediaFile; + +const ids = (media: MediaFile[]) => media.map((m) => m.id); + +const region = (start: number, end: number): IRegion => + ({ start, end, label: '' }) as IRegion; + +describe('selectCurrentPhraseTakes', () => { + it('drops takes recorded against boundaries that no longer exist', () => { + // Segments were [0,5] and [5,10], then the boundary moved to 6. + const takes = [ + take('stale-1', { start: 0, end: 5 }, '2026-01-01T00:00:00Z'), + take('stale-2', { start: 5, end: 10 }, '2026-01-01T00:01:00Z'), + take('current-1', { start: 0, end: 6 }, '2026-01-01T00:02:00Z'), + take('current-2', { start: 6, end: 10 }, '2026-01-01T00:03:00Z'), + ]; + const result = selectCurrentPhraseTakes(takes, [ + region(0, 6), + region(6, 10), + ]); + expect(ids(result)).toEqual(['current-1', 'current-2']); + }); + + it('keeps only the newest take of a segment recorded more than once', () => { + const takes = [ + take('first', { start: 0, end: 6 }, '2026-01-01T00:00:00Z'), + take('second', { start: 0, end: 6 }, '2026-01-02T00:00:00Z'), + ]; + expect(ids(selectCurrentPhraseTakes(takes, [region(0, 6)]))).toEqual([ + 'second', + ]); + }); + + it('breaks a tie on the creation date by id so the choice is stable', () => { + const takes = [ + take('aaa', { start: 0, end: 6 }, '2026-01-01T00:00:00Z'), + take('bbb', { start: 0, end: 6 }, '2026-01-01T00:00:00Z'), + ]; + expect(ids(selectCurrentPhraseTakes(takes, [region(0, 6)]))).toEqual([ + 'bbb', + ]); + expect( + ids(selectCurrentPhraseTakes([...takes].reverse(), [region(0, 6)])) + ).toEqual(['bbb']); + }); + + it('matches a segment whose stored boundaries drifted within tolerance', () => { + const takes = [take('drifted', { start: 0.01, end: 5.98 })]; + expect(ids(selectCurrentPhraseTakes(takes, [region(0, 6)]))).toEqual([ + 'drifted', + ]); + }); + + it('returns the takes untouched when the current segments are unknown', () => { + // No boundaries to compare against (vernacular unreadable, or an artifact + // that records no segment map) - nothing can be called stale, so nothing + // may be hidden. + const takes = [ + take('a', { start: 0, end: 5 }), + take('b', { start: 5, end: 10 }), + ]; + expect(ids(selectCurrentPhraseTakes(takes, []))).toEqual(['a', 'b']); + }); + + it('keeps takes that name no segment at all', () => { + // Retell and pre-segment-map takes carry no `sourceSegments`. They cannot + // be attributed to a segment, so they cannot be judged stale either. + const takes = [ + take('whole-passage', null), + take('stale', { start: 0, end: 5 }), + take('current', { start: 0, end: 6 }), + ]; + expect(ids(selectCurrentPhraseTakes(takes, [region(0, 6)]))).toEqual([ + 'whole-passage', + 'current', + ]); + }); + + it('keeps the order it was given', () => { + const takes = [ + take('second', { start: 6, end: 10 }), + take('first', { start: 0, end: 6 }), + ]; + expect( + ids(selectCurrentPhraseTakes(takes, [region(0, 6), region(6, 10)])) + ).toEqual(['second', 'first']); + }); +}); From 1093974eeaebe8bd313008b40363e823a5a380b7 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Tue, 1 Sep 2026 21:39:55 -0400 Subject: [PATCH 2/4] TT-7666 TT-7432 fix: tell one take of a segment from another TT-7666 - scope the Transcribe task list to the segment boundaries the step is reading. `selectCurrentPhraseTakes` keeps the newest take of each current segment and drops the takes that name a segment the boundaries have moved away from, so adjusting a boundary and recording again leaves one task per segment instead of one per take ever recorded. It hides rather than deletes: a take is only dropped where boundaries exist to judge it against. With no boundaries to read - a vernacular whose segments cannot be parsed, an artifact that records none, the plan-level task list that has no step context - the takes come through untouched, as do takes with no `sourceSegments` at all (Retell, and anything older than segment maps). Hiding audio on a guess would be worse than a duplicate row. The segment-matching and newest-take rules now live in one place, crud/phraseTakes, reached by both the step (carefulSpeechCompletion, matchesGuidedOutputRow) and the context layer that builds the task list. TT-7432 - give every take its own upload name. A take now carries a token minted where recording begins, so a segment re-recorded after its first take was deleted no longer resolves to the cached file of the take that was deleted. Takes already uploaded keep their names. Co-Authored-By: Claude Opus 5 (1M context) --- .../PassageDetailGuidedPhraseRecord.tsx | 15 ++- .../PassageDetail/PassageDetailTranscribe.tsx | 1 + .../carefulSpeech/carefulSpeechCompletion.ts | 27 ++---- .../carefulSpeech/matchesGuidedOutputRow.ts | 10 +- .../PassageDetail/guidedPhraseRecord/types.ts | 45 +++++++-- .../src/context/TranscriberContext.tsx | 21 ++++- src/renderer/src/crud/phraseTakes.ts | 91 +++++++++++++++++++ 7 files changed, 174 insertions(+), 36 deletions(-) create mode 100644 src/renderer/src/crud/phraseTakes.ts diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx index 79ade9f69..5caa57e5a 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -82,6 +82,7 @@ import { splitClauseAt, } from './carefulSpeech/carefulSpeechClauseSplit'; import { + newTakeToken, type GuidedPhraseRecordConfig, type IGuidedPhraseRecordControlStrings, } from '../../components/PassageDetail/guidedPhraseRecord/types'; @@ -220,6 +221,11 @@ export function PassageDetailGuidedPhraseRecord({ localStorage.getItem(config.speakerLocalKey) ?? '' ); const [showRecorder, setShowRecorder] = useState(false); + // Names this take apart from any other take of the same segment in the same + // step - see buildFilenamePostfix for why a repeated name plays the wrong + // audio (TT-7432). A new one for every take, so it is minted where recording + // begins rather than derived from anything the takes have in common. + const [takeToken, setTakeToken] = useState(newTakeToken); const [resetMedia, setResetMedia] = useState(false); const [statusText, setStatusText] = useState(''); const [canSave, setCanSave] = useState(false); @@ -523,7 +529,8 @@ export function PassageDetailGuidedPhraseRecord({ const postfix = config.buildFilenamePostfix( currentIndex, currentVersion, - stepLanguageBcp47 + stepLanguageBcp47, + takeToken ); return passageDefaultFilename( passage, @@ -542,6 +549,7 @@ export function PassageDetailGuidedPhraseRecord({ currentIndex, currentVersion, stepLanguageBcp47, + takeToken, config, ]); @@ -1863,6 +1871,11 @@ export function PassageDetailGuidedPhraseRecord({ onRecording={(active) => { if (active) { recordingActiveRef.current = true; + // This take is its own file, even where an earlier take of this + // segment was deleted first (TT-7432). MediaRecord reads + // defaultFilename when the save runs, so a token minted here is + // the one the take uploads under. + setTakeToken(newTakeToken()); // A new take supersedes any earlier rejected save (TT-7583). saveRejectedRef.current = false; setSaveRejected(false); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.tsx b/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.tsx index 72de9a697..1df715e0b 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.tsx @@ -317,6 +317,7 @@ export function PassageDetailTranscribe({ width, artifactTypeId }: IProps) { artifactTypeId={artifactTypeId} curRole={curRole as string} stepLanguageBcp47={stepLanguageBcp47} + phraseRegions={phraseRegions} > { - const da = a.mediafile?.attributes?.dateCreated ?? ''; - const db = b.mediafile?.attributes?.dateCreated ?? ''; - if (da !== db) return db.localeCompare(da); - return (b.mediafile?.id ?? '').localeCompare(a.mediafile?.id ?? ''); - })[0]; + return [...matches].sort((a, b) => + compareTakesNewestFirst(a.mediafile, b.mediafile) + )[0]; } /** Named-region key for Phrase BT segment boundaries for a language. */ diff --git a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts index b582628bf..3955dc08e 100644 --- a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts +++ b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts @@ -53,15 +53,43 @@ export interface GuidedPhraseRecordConfig { * 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). + * have a sibling step over the same audio in another language (TT-7643), and + * `takeToken` tells apart the takes of one segment in one step - re-recording + * a segment, with or without deleting the old take first, matches on every + * other part of the name (TT-7432). Both are omitted for takes that predate + * them, which keep the names they were uploaded under. */ buildFilenamePostfix: ( unitIndex: number, sourceVersion: number, - languageBcp47?: string + languageBcp47?: string, + takeToken?: string ) => string; } +let lastTokenMs = 0; +let tokenSeq = 0; + +/** + * A token for one take, unique and ascending. The clock alone would do, but two + * calls can land in the same millisecond, so same-millisecond calls get a + * counter appended rather than the same token. + */ +export function newTakeToken(now: number = Date.now()): string { + if (now === lastTokenMs) { + tokenSeq += 1; + } else { + lastTokenMs = now; + tokenSeq = 0; + } + const stamp = now.toString(36); + return tokenSeq === 0 ? stamp : `${stamp}${tokenSeq.toString(36)}`; +} + +/** `_`-joined name parts, skipping the ones this take has nothing for. */ +const withParts = (base: string, ...parts: (string | undefined)[]): string => + [base, ...parts.filter((p) => p)].join('_'); + const carefulSpeechBoundaryDefaults = { constrainAutoSegmentWithVerses: false, showPlayerSegmentControls: false, @@ -81,8 +109,8 @@ export const CAREFUL_SPEECH_CONFIG: GuidedPhraseRecordConfig = { containerId: 'careful-speech', requireBoldWorkflow: true, ...carefulSpeechBoundaryDefaults, - buildFilenamePostfix: (unitIndex, sourceVersion) => - `carefulspeech${unitIndex + 1}_v${sourceVersion}`, + buildFilenamePostfix: (unitIndex, sourceVersion, _languageBcp47, takeToken) => + withParts(`carefulspeech${unitIndex + 1}_v${sourceVersion}`, takeToken), }; export function phraseBackTranslateConfig( @@ -106,13 +134,18 @@ export function phraseBackTranslateConfig( multiLevelSegmentUndo: phraseBoundaryTools, sequentialUnitNavAroundRecord: phraseBoundaryTools, persistSegments: phraseBoundaryTools, - buildFilenamePostfix: (unitIndex, sourceVersion, languageBcp47) => { + buildFilenamePostfix: ( + unitIndex, + sourceVersion, + languageBcp47, + takeToken + ) => { const base = `${artifactSlug}${unitIndex + 1}_v${sourceVersion}`; 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; + return withParts(unit, languageBcp47, takeToken); }, }; } diff --git a/src/renderer/src/context/TranscriberContext.tsx b/src/renderer/src/context/TranscriberContext.tsx index 70a38384b..cfe998b35 100644 --- a/src/renderer/src/context/TranscriberContext.tsx +++ b/src/renderer/src/context/TranscriberContext.tsx @@ -33,6 +33,8 @@ import { } from '../crud'; import { mediaFileName } from '../crud/media'; import { mediaMatchesStepLanguage } from '../utils/mediaLanguage'; +import { selectCurrentPhraseTakes } from '../crud/phraseTakes'; +import { IRegion } from '../crud/useWavesurferRegions'; import StickyRedirect from '../components/StickyRedirect'; import { useSelector } from 'react-redux'; import { useDispatch } from 'react-redux'; @@ -133,9 +135,15 @@ interface IProps { curRole?: string; /** Step language. When set (and not `und`), only media tagged with it become tasks. */ stepLanguageBcp47?: string; + /** + * Phrase-segment boundaries the step is reading. When given, a segment + * contributes its newest take and the takes left behind by a boundary + * adjustment are not tasks (TT-7666). + */ + phraseRegions?: IRegion[]; } const TranscriberProvider = (props: IProps) => { - const { artifactTypeId, curRole, stepLanguageBcp47 } = props; + const { artifactTypeId, curRole, stepLanguageBcp47, phraseRegions } = props; const [isDetail] = useState(artifactTypeId !== undefined); const passages = useOrbitData('passage'); const sections = useOrbitData('section'); @@ -200,10 +208,19 @@ const TranscriberProvider = (props: IProps) => { return; } m = m.filter((mf) => mediaMatchesStepLanguage(mf, stepLanguageBcp47)); + m = selectCurrentPhraseTakes(m, phraseRegions ?? []); setPlanMedia(m); planMediaRef.current = m; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [mediafiles, devPlan, artifactId, stepLanguageBcp47, pasId, memory]); + }, [ + mediafiles, + devPlan, + artifactId, + stepLanguageBcp47, + phraseRegions, + pasId, + memory, + ]); const setRows = (rowData: IRowData[]) => { setState((state: ICtxState) => { diff --git a/src/renderer/src/crud/phraseTakes.ts b/src/renderer/src/crud/phraseTakes.ts new file mode 100644 index 000000000..063ceab59 --- /dev/null +++ b/src/renderer/src/crud/phraseTakes.ts @@ -0,0 +1,91 @@ +import { MediaFile } from '../model'; +import { IRegion } from './useWavesurferRegions'; + +/** + * Which take belongs to which phrase segment, and which take of a segment wins. + * + * A Careful Speech / Phrase BT take names the slice of vernacular it covers in + * `sourceSegments`. That is the only link back to a segment: segments are not + * records, they are boundaries stored on the vernacular's named regions, and + * moving a boundary rewrites them in place. Takes recorded before the move are + * left answering to boundaries that no longer exist (TT-7666). + * + * Lives in crud/ rather than beside the step because the Transcribe task list + * is built in the context layer, which should not have to reach into a + * component subtree for it. + */ + +/** Seconds of slack allowed between a take's stored region and a segment. */ +export const PHRASE_REGION_TOLERANCE = 0.05; + +/** The region a take names, or undefined when it names none. */ +export function parseTakeSourceRegion( + sourceSegments: string | undefined +): IRegion | undefined { + if (!sourceSegments) return undefined; + try { + const parsed = JSON.parse(sourceSegments) as IRegion; + if (parsed?.start !== undefined && parsed?.end !== undefined) return parsed; + } catch { + return undefined; + } + return undefined; +} + +/** True when a take's stored region is the given segment. */ +export function takeMatchesRegion( + sourceSegments: string | undefined, + region: IRegion +): boolean { + const stored = parseTakeSourceRegion(sourceSegments); + if (!stored) return false; + return ( + Math.abs(stored.start - region.start) < PHRASE_REGION_TOLERANCE && + Math.abs(stored.end - region.end) < PHRASE_REGION_TOLERANCE + ); +} + +/** + * Newest take first. The id breaks a tie on the creation date so the same take + * is picked every time - two takes saved in the same second otherwise swap + * places between renders. + */ +export function compareTakesNewestFirst( + a: MediaFile | undefined, + b: MediaFile | undefined +): number { + const da = a?.attributes?.dateCreated ?? ''; + const db = b?.attributes?.dateCreated ?? ''; + if (da !== db) return db.localeCompare(da); + return (b?.id ?? '').localeCompare(a?.id ?? ''); +} + +/** + * The takes still worth showing for `regions`: the newest take of each segment, + * plus every take that names no segment at all. + * + * Takes naming a segment that is not in `regions` are dropped - they were + * recorded against boundaries the step has since moved away from, so no step + * will ever offer them again. An empty `regions` returns the takes untouched: + * with no boundaries to compare against nothing can be called stale, and + * hiding audio on a guess is worse than a duplicate row. Takes with no + * `sourceSegments` (Retell, and anything recorded before segment maps) are kept + * for the same reason. Input order is preserved; callers sort for display. + */ +export function selectCurrentPhraseTakes( + takes: T[], + regions: IRegion[] +): T[] { + if (regions.length === 0 || takes.length === 0) return takes; + const current = new Set(); + regions.forEach((region) => { + const newest = takes + .filter((t) => takeMatchesRegion(t.attributes?.sourceSegments, region)) + .sort(compareTakesNewestFirst)[0]; + if (newest) current.add(newest); + }); + return takes.filter( + (t) => + current.has(t) || !parseTakeSourceRegion(t.attributes?.sourceSegments) + ); +} From e98738d134182cf6a934df086e7755e40f081a2c Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Tue, 1 Sep 2026 22:03:05 -0400 Subject: [PATCH 3/4] TT-7432 test: LWC Translation reuses a deleted take's name The same defect one step over. Clause index and source version are the whole of an LWC Translation take's name and neither changes when the clause is recorded again, so clearing a recording and recording it again uploads the replacement under the name the deleted take is cached on - and `dataPath` resolves a mediafile's audioUrl to `/media/`, so the deleted audio is what plays. The step's own test harness stubbed the filename to a constant, so it could not have caught this; it now echoes the postfix the step builds. Co-Authored-By: Claude Opus 5 (1M context) --- .../PassageDetailLwcTranslation.test.tsx | 69 ++++++++++++++++--- 1 file changed, 58 insertions(+), 11 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailLwcTranslation.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailLwcTranslation.test.tsx index 9e14ba82d..750ec004b 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailLwcTranslation.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailLwcTranslation.test.tsx @@ -153,12 +153,25 @@ jest.mock('./lwcTranslation/LwcTranslationControls', () => ({ }, })); +// Echo the postfix the step 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`, })); import { PassageDetailLwcTranslation } from './PassageDetailLwcTranslation'; +/** Play the reference clause through, which is what reveals the recorder. */ +const openRecorder = async () => { + render(); + await waitFor(() => expect(referenceProps).toBeDefined()); + await act(async () => { + (referenceProps?.onPlaybackComplete as () => void)(); + }); + await waitFor(() => expect(controlsProps?.showRecorder).toBe(true)); +}; + describe('PassageDetailLwcTranslation', () => { beforeEach(() => { mockCarefulSpeechComplete = new Set(); @@ -207,16 +220,6 @@ describe('PassageDetailLwcTranslation — rejected save (TT-7583)', () => { mockStartSave.mockClear(); }); - // Play the reference clause through, which is what reveals the recorder. - const openRecorder = async () => { - render(); - await waitFor(() => expect(referenceProps).toBeDefined()); - await act(async () => { - (referenceProps?.onPlaybackComplete as () => void)(); - }); - await waitFor(() => expect(controlsProps?.showRecorder).toBe(true)); - }; - // Record a take and request its auto-save, then have MediaRecord reject it. const recordAndRejectSave = async () => { await openRecorder(); @@ -374,3 +377,47 @@ describe('PassageDetailLwcTranslation — rejected save (TT-7583)', () => { expect(controlsProps?.phase).toBe('recorded'); }); }); + +/** + * TT-7432 - clause index and source version were the whole of a take's name, + * and they do not change when the clause is recorded again. `dataPath` resolves + * a mediafile's audioUrl to `/media/`, so clearing a + * recording and recording it again uploaded the replacement under the name the + * deleted take is already cached on, and the deleted audio is what played. + * Same defect the Careful Speech / Phrase BT steps had. + */ +describe('PassageDetailLwcTranslation - take names (TT-7432)', () => { + beforeEach(() => { + mockCarefulSpeechComplete = new Set([0]); + mockLwcComplete = new Set(); + mockClauseRegions = [{ start: 0, end: 5, label: '' }]; + controlsProps = undefined; + referenceProps = undefined; + mockStartSave.mockClear(); + }); + + it('names each take of a clause separately', async () => { + await openRecorder(); + const record = (active: boolean) => + (controlsProps?.onRecording as (a: boolean) => void)(active); + + await act(async () => { + record(true); + }); + const firstTake = controlsProps?.defaultFilename as string; + await act(async () => { + record(false); + }); + // It still says which clause and which source version it belongs to. + expect(firstTake).toContain('lwctranslation1_v1'); + + // Clear it and record the same clause again. + await act(async () => { + await (controlsProps?.onClearRecording as () => Promise)(); + }); + await act(async () => { + record(true); + }); + expect(controlsProps?.defaultFilename).not.toEqual(firstTake); + }); +}); From 4cc9c56fe39bc49ab8049e7edb0b84154fca4dc4 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Tue, 1 Sep 2026 22:12:08 -0400 Subject: [PATCH 4/4] TT-7432 fix: give each LWC Translation take its own upload name Same fix as the Careful Speech / Phrase BT steps: the take carries a token minted where recording begins, so re-recording a clause - with or without clearing the old take first - no longer resolves to the cached file of the take it replaced. The token generator is shared with the guided phrase-record steps rather than duplicated, so the whole session mints from one monotonic sequence. Co-Authored-By: Claude Opus 5 (1M context) --- .../PassageDetailLwcTranslation.tsx | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailLwcTranslation.tsx b/src/renderer/src/components/PassageDetail/PassageDetailLwcTranslation.tsx index ab1954dbb..ddb53c138 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailLwcTranslation.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailLwcTranslation.tsx @@ -51,6 +51,7 @@ import LwcTranslationControls, { LwcTranslationPhase, } from './lwcTranslation/LwcTranslationControls'; import { LocalKey } from '../../utils/localUserKey'; +import { newTakeToken } from './guidedPhraseRecord/types'; import { Button } from '../../control/Button'; const toolId = 'LwcTranslationTool'; @@ -106,6 +107,9 @@ export function PassageDetailLwcTranslation({ width }: IProps) { return stored ?? ''; }); const [showRecorder, setShowRecorder] = useState(false); + // Names this take apart from any other take of the same clause - see + // defaultFilename for why a repeated name plays the wrong audio (TT-7432). + const [takeToken, setTakeToken] = useState(newTakeToken); const [resetMedia, setResetMedia] = useState(false); const [canSave, setCanSave] = useState(false); const [savingRecording, setSavingRecording] = useState(false); @@ -228,7 +232,12 @@ export function PassageDetailLwcTranslation({ width }: IProps) { ); const defaultFilename = useMemo(() => { - const postfix = `lwctranslation${currentIndex + 1}_v${currentVersion}`; + // The token is what tells two takes of this clause apart: the uploaded name + // becomes the media object's name, and `dataPath` resolves a mediafile's + // audioUrl to `/media/`, so without it a re-recorded + // clause resolves to the cached file of the take it replaced (TT-7432). + const clause = `lwctranslation${currentIndex + 1}_v${currentVersion}`; + const postfix = `${clause}_${takeToken}`; return passageDefaultFilename( passage, plan, @@ -245,6 +254,7 @@ export function PassageDetailLwcTranslation({ width }: IProps) { offline, currentIndex, currentVersion, + takeToken, ]); // TT-7583: this step auto-saves on every rising edge of canSave. A failed @@ -520,6 +530,10 @@ export function PassageDetailLwcTranslation({ width }: IProps) { (active: boolean) => { if (active) { recordingActiveRef.current = true; + // This take is its own file, even where the clause's earlier take was + // cleared first (TT-7432). MediaRecord reads defaultFilename when the + // save runs, so a token minted here is the one the take uploads under. + setTakeToken(newTakeToken()); // A new take supersedes any earlier rejected save (TT-7583). saveRejectedRef.current = false; setSaveRejected(false);