From 5951e4c8fcad6100e264b4ee5bbc415e2085560a Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Wed, 2 Sep 2026 15:53:23 -0400 Subject: [PATCH 1/3] TT-7666 test: takes left behind by a moved segment boundary Failing tests first. A Careful Speech / Phrase BT take records the slice of vernacular it covers in `sourceSegments`, and that is its only link back to a segment - segments are boundaries on the vernacular's named regions, not records, so moving one rewrites the slices in place. Takes made before the move 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 the takes matching the boundaries it is reading, but the Transcribe task list is built from every take there is: two segments, four tasks. Co-Authored-By: Claude Opus 5 (1M context) --- .../PassageDetailTranscribe.test.tsx | 99 ++++++++++++--- src/renderer/src/crud/phraseTakes.test.ts | 115 ++++++++++++++++++ 2 files changed, 199 insertions(+), 15 deletions(-) create mode 100644 src/renderer/src/crud/phraseTakes.test.ts 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/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 0df83bad6990809d8a6966e067a28bc335683ecb Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Wed, 2 Sep 2026 15:53:33 -0400 Subject: [PATCH 2/3] TT-7666 fix: scope the Transcribe task list to the current boundaries `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. Co-Authored-By: Claude Opus 5 (1M context) --- .../PassageDetail/PassageDetailTranscribe.tsx | 1 + .../carefulSpeech/carefulSpeechCompletion.ts | 27 ++---- .../carefulSpeech/matchesGuidedOutputRow.ts | 10 +- .../src/context/TranscriberContext.tsx | 21 ++++- src/renderer/src/crud/phraseTakes.ts | 91 +++++++++++++++++++ 5 files changed, 121 insertions(+), 29 deletions(-) create mode 100644 src/renderer/src/crud/phraseTakes.ts 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.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 8a2543aa2efce5a63d435e0bba6165507d0fbafb Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Wed, 2 Sep 2026 17:31:06 -0400 Subject: [PATCH 3/3] TT-7666 fix: judge a take stale on its own boundary, and parse it once Two changes to the same comparison. The tolerance is inclusive. It is half the 0.1s grid `prettySegment` rounds to, so a boundary rounded to tenths lands exactly on it, and a strict `<` called such a take stale and hid a recording the UI showed as matching. Anything wider is a boundary someone actually moved, which is what stale is meant to mean. `regionsMatch` compares two regions already parsed, so `regionMatchesClause` no longer parses `sourceSegments` to ask whether it names a region and then parses it again to compare, and `selectCurrentPhraseTakes` parses each take once rather than once per region. Co-Authored-By: Claude Opus 5 (1M context) --- .../carefulSpeech/carefulSpeechCompletion.ts | 10 ++-- src/renderer/src/crud/phraseTakes.test.ts | 14 ++++++ src/renderer/src/crud/phraseTakes.ts | 50 ++++++++++++++----- 3 files changed, 54 insertions(+), 20 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechCompletion.ts b/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechCompletion.ts index 813b45d51..10214b1e5 100644 --- a/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechCompletion.ts +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechCompletion.ts @@ -5,10 +5,7 @@ import { matchesGuidedOutputRow, pickLatestGuidedOutputRow, } from './matchesGuidedOutputRow'; -import { - parseTakeSourceRegion, - takeMatchesRegion, -} from '../../../crud/phraseTakes'; +import { parseTakeSourceRegion, regionsMatch } from '../../../crud/phraseTakes'; function isEmptySourceSegments(seg: string | undefined): boolean { if (!seg) return true; @@ -34,9 +31,8 @@ function regionMatchesClause( ) { return true; } - if (parseTakeSourceRegion(storedSeg)) { - return takeMatchesRegion(storedSeg, clauseRegion); - } + const stored = parseTakeSourceRegion(storedSeg); + if (stored) return regionsMatch(stored, clauseRegion); return prettySegment(storedSeg).trim() === prettySegment(clauseRegion).trim(); } diff --git a/src/renderer/src/crud/phraseTakes.test.ts b/src/renderer/src/crud/phraseTakes.test.ts index e9d3bba53..df626fc9e 100644 --- a/src/renderer/src/crud/phraseTakes.test.ts +++ b/src/renderer/src/crud/phraseTakes.test.ts @@ -78,6 +78,20 @@ describe('selectCurrentPhraseTakes', () => { ]); }); + 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 diff --git a/src/renderer/src/crud/phraseTakes.ts b/src/renderer/src/crud/phraseTakes.ts index 063ceab59..51e031d73 100644 --- a/src/renderer/src/crud/phraseTakes.ts +++ b/src/renderer/src/crud/phraseTakes.ts @@ -15,7 +15,17 @@ import { IRegion } from './useWavesurferRegions'; * component subtree for it. */ -/** Seconds of slack allowed between a take's stored region and a segment. */ +/** + * 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. */ @@ -32,17 +42,24 @@ export function parseTakeSourceRegion( return undefined; } -/** True when a take's stored region is the given segment. */ +/** 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); - if (!stored) return false; - return ( - Math.abs(stored.start - region.start) < PHRASE_REGION_TOLERANCE && - Math.abs(stored.end - region.end) < PHRASE_REGION_TOLERANCE - ); + return stored ? regionsMatch(stored, region) : false; } /** @@ -77,15 +94,22 @@ export function selectCurrentPhraseTakes( 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 = takes - .filter((t) => takeMatchesRegion(t.attributes?.sourceSegments, 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 takes.filter( - (t) => - current.has(t) || !parseTakeSourceRegion(t.attributes?.sourceSegments) - ); + return named + .filter((n) => current.has(n.take) || !n.region) + .map((n) => n.take); }