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/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/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); + }); +}); 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); 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/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.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/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.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']); + }); +}); 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) + ); +}