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/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..df626fc9e --- /dev/null +++ b/src/renderer/src/crud/phraseTakes.test.ts @@ -0,0 +1,129 @@ +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('matches a boundary that drifted by exactly the tolerance', () => { + // A legacy value rounded to tenths lands exactly on the tolerance; a strict + // comparison would hide the recording. + const takes = [take('tenths', { start: 0.05, end: 5.95 })]; + expect(ids(selectCurrentPhraseTakes(takes, [region(0, 6)]))).toEqual([ + 'tenths', + ]); + }); + + it('drops a take whose boundary moved past the tolerance', () => { + const takes = [take('moved', { start: 0.06, end: 6 })]; + expect(ids(selectCurrentPhraseTakes(takes, [region(0, 6)]))).toEqual([]); + }); + + 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..51e031d73 --- /dev/null +++ b/src/renderer/src/crud/phraseTakes.ts @@ -0,0 +1,115 @@ +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. + * + * Half of the 0.1s grid `prettySegment` rounds to, so a take is never judged + * stale over a difference the UI cannot show - a take labelled `0.0-5.9` always + * matches the segment labelled `0.0-5.9`. Boundaries are stored to five + * decimals (`roundToFiveDecimals` in useWavesurferRegions), so anything wider + * than this is a boundary someone actually moved, which is what we mean to call + * stale. Compared with `<=` because a value rounded to tenths lands exactly on + * the tolerance, and hiding a recording is worse than keeping a duplicate row. + */ +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 two regions name the same slice, within the tolerance. */ +export function regionsMatch(a: IRegion, b: IRegion): boolean { + return ( + Math.abs(a.start - b.start) <= PHRASE_REGION_TOLERANCE && + Math.abs(a.end - b.end) <= PHRASE_REGION_TOLERANCE + ); +} + +/** + * True when a take's stored region is the given segment. Callers that already + * hold the parsed region compare with `regionsMatch` instead of parsing again. + */ +export function takeMatchesRegion( + sourceSegments: string | undefined, + region: IRegion +): boolean { + const stored = parseTakeSourceRegion(sourceSegments); + return stored ? regionsMatch(stored, region) : false; +} + +/** + * 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; + // Parse each take once (not once per take-region pair). A passage can hold + // one take per segment per pass, so parsing inside the region loop would + // repeatedly parse the same take for every region. + const named = takes.map((take) => ({ + take, + region: parseTakeSourceRegion(take.attributes?.sourceSegments), + })); + const current = new Set(); + regions.forEach((region) => { + const newest = named + .filter((n) => n.region && regionsMatch(n.region, region)) + .map((n) => n.take) + .sort(compareTakesNewestFirst)[0]; + if (newest) current.add(newest); + }); + return named + .filter((n) => current.has(n.take) || !n.region) + .map((n) => n.take); +}