From 303a3692340754e9224f44e535f71f12de0f8f93 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Tue, 1 Sep 2026 17:41:02 -0400 Subject: [PATCH 1/3] TT-7432 fix: give each take of a segment its own upload name Clearing a guided-phrase take deletes its mediafile but not the audio cached under that take's name, and the desktop app resolves a mediafile's audio by name (dataPath -> /media/), never by id. A re-record of the same segment reused the same name, so playback served the take the user had just discarded. buildFilenamePostfix now takes a parts object and adds a per-attempt token, renewed when capture starts and held steady through the upload, so an attempt cannot collide with the one it replaces. Take order still comes from dateCreated. Co-Authored-By: Claude Opus 5 (1M context) --- docs/adr/0009-phrase-bt-language-scoping.md | 1 + src/renderer/cypress/support/pbtHarness.tsx | 1 + ...etailGuidedPhraseRecord.stepScope.test.tsx | 38 +++++++++++++ .../PassageDetailGuidedPhraseRecord.tsx | 26 +++++++-- .../PassageDetailPhraseBackTranslate.cy.tsx | 27 ++++++++++ .../carefulSpeech/CarefulSpeechControls.tsx | 1 + .../guidedPhraseRecord/types.test.ts | 54 +++++++++++++++---- .../PassageDetail/guidedPhraseRecord/types.ts | 44 ++++++++++----- 8 files changed, 165 insertions(+), 27 deletions(-) diff --git a/docs/adr/0009-phrase-bt-language-scoping.md b/docs/adr/0009-phrase-bt-language-scoping.md index 65b02f7d2..a4cfd300c 100644 --- a/docs/adr/0009-phrase-bt-language-scoping.md +++ b/docs/adr/0009-phrase-bt-language-scoping.md @@ -14,3 +14,4 @@ Non-BOLD **Phrase Back Translation** (and **Retell Back Translation**) must supp - BOLD **LWC Translation** keeps shared **`clause`** boundaries; may still stamp **LWC language** on recordings without per-language clause maps - Language scoping is an **allowlist** keyed on `artifactStampsStepLanguage` — **Phrase BT only**, the one artifact whose takes carry `languagebcp47`. **Whole Back Translation** is recorded by `PassageDetailItem`, which never stamps a language, and vernacular / Q&A / Retell use the org vernacular — a Transcribe step's `language` is their ASR / font / spell-check language only, so scoping their task lists by it would match nothing - The allowlist is deliberately **narrower than `isPhraseSegmentArtifact`**, which also covers **Careful Speech**. Those two predicates answer different questions: `isPhraseSegmentArtifact` means "uses segmented regions on the waveform" (still true of Careful Speech, and still what `phraseRegions` / `phraseArtifactSlug` key on), while `artifactStampsStepLanguage` means "takes carry a language". Careful Speech is BOLD-only — `CAREFUL_SPEECH_CONFIG` sets `requireBoldWorkflow`, which makes `stepLanguageField` resolve to `undefined`, so its takes are recorded untagged and BOLD keeps shared **`clause`** boundaries rather than per-language ones +- **The uploaded file name is part of the discriminator, not decoration.** On the desktop app a take's audio is resolved by name: `useFetchMediaUrl` calls `dataPath(mediafile.audioUrl, PathType.MEDIA)`, which maps to `/media/` and returns that file if it exists — the mediafile id is never consulted, and `store/upload/actions.tsx` stages every online upload into that same folder. Two takes uploaded under one name therefore share one cached file, and the first one cached is what plays for both. `buildFilenamePostfix` must keep every axis that separates one take from another: segment index, source version, **step language**, and a **per-attempt token** (clearing a take deletes its mediafile but not the audio cached under its name, so a re-record that reused the name played the discarded take back — TT-7432). TT-7643 diff --git a/src/renderer/cypress/support/pbtHarness.tsx b/src/renderer/cypress/support/pbtHarness.tsx index 6e643e9af..52226a2d5 100644 --- a/src/renderer/cypress/support/pbtHarness.tsx +++ b/src/renderer/cypress/support/pbtHarness.tsx @@ -80,6 +80,7 @@ export const PBT = { prevUnit: '#phrase-back-translate-prev-unit', nextUnit: '#phrase-back-translate-next-unit', speaker: '#phrase-back-translate-speaker', + clear: '#phrase-back-translate-clear', retrySave: '#phrase-back-translate-retry-save', dockedRecord: '[data-cy="phrase-back-translate-docked-record"]', /** The record control itself (RecordButton renders role=button + aria-disabled). */ diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx index 952e79675..ab34f1e69 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx @@ -279,6 +279,44 @@ describe('PassageDetailGuidedPhraseRecord - step scope (TT-7643)', () => { expect(controlsProps?.defaultFilename).toContain('he'); }); + it('names each attempt at a segment apart from the one it replaces', async () => { + // Clearing a take deletes its mediafile but not the audio cached under its + // name, and the next attempt at the same segment in the same language + // built the very same name - so `dataPath` handed the new take the old + // take's file and the recording the user had just discarded played back + // (TT-7643). + await mountAndSettle(); + const firstAttempt = controlsProps?.defaultFilename as string; + expect(firstAttempt).toBeTruthy(); + + await act(async () => { + (controlsProps?.onRecording as (active: boolean) => void)(true); + }); + + await waitFor(() => + expect(controlsProps?.defaultFilename).not.toEqual(firstAttempt) + ); + // Still this segment, in this language - only the attempt is new. + expect(controlsProps?.defaultFilename).toContain('backtranslation1'); + expect(controlsProps?.defaultFilename).toContain('seh'); + }); + + it('holds a name steady for the length of one recording', async () => { + await mountAndSettle(); + await act(async () => { + (controlsProps?.onRecording as (active: boolean) => void)(true); + }); + const whileRecording = controlsProps?.defaultFilename as string; + // The name is chosen when recording starts and has to survive every + // re-render between there and the upload, or MediaRecord would save under + // a different name than the one the step showed. + await act(async () => { + (controlsProps?.onRecording as (active: boolean) => void)(false); + }); + await waitFor(() => expect(controlsProps).toBeDefined()); + expect(controlsProps?.defaultFilename).toEqual(whileRecording); + }); + 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..fbd668799 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx @@ -104,6 +104,9 @@ interface IProps { */ const SPURIOUS_STOP_WINDOW_MS = 250; +/** When a take was made, short and filename-safe. */ +const takeStamp = (): string => Date.now().toString(36); + function findClauseIndex(clauseRegions: IRegion[], region: IRegion): number { return clauseRegions.findIndex( (r) => @@ -220,6 +223,16 @@ export function PassageDetailGuidedPhraseRecord({ localStorage.getItem(config.speakerLocalKey) ?? '' ); const [showRecorder, setShowRecorder] = useState(false); + /** + * When this attempt at a segment was made, in the name the take uploads + * under. Clearing a take deletes its mediafile but not the audio cached + * under its name, and `dataPath` resolves a mediafile's audioUrl by that + * name - so a re-record that reused the name played back the take the user + * had just discarded (TT-7432). Renewed when capture starts, and held steady + * from there through the upload. Take *order* comes from `dateCreated`, not + * from this. + */ + const [takeToken, setTakeToken] = useState(takeStamp); const [resetMedia, setResetMedia] = useState(false); const [statusText, setStatusText] = useState(''); const [canSave, setCanSave] = useState(false); @@ -520,11 +533,12 @@ export function PassageDetailGuidedPhraseRecord({ ); const defaultFilename = useMemo(() => { - const postfix = config.buildFilenamePostfix( - currentIndex, - currentVersion, - stepLanguageBcp47 - ); + const postfix = config.buildFilenamePostfix({ + unitIndex: currentIndex, + sourceVersion: currentVersion, + languageBcp47: stepLanguageBcp47, + takeToken, + }); return passageDefaultFilename( passage, plan, @@ -542,6 +556,7 @@ export function PassageDetailGuidedPhraseRecord({ currentIndex, currentVersion, stepLanguageBcp47, + takeToken, config, ]); @@ -1863,6 +1878,7 @@ export function PassageDetailGuidedPhraseRecord({ onRecording={(active) => { if (active) { recordingActiveRef.current = true; + setTakeToken(takeStamp()); // A new take supersedes any earlier rejected save (TT-7583). saveRejectedRef.current = false; setSaveRejected(false); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx b/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx index 88099205c..65f308f6a 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx @@ -32,6 +32,7 @@ import { sourcePlay, startRecordingPass, recordAndSettle, + waitForRecorderIdle, } from '../../../cypress/support/pbtHarness'; const SEGMENTS = SEGMENTS_3; @@ -331,4 +332,30 @@ describe('PBT language scoping', () => { expect(posted?.originalFile).to.not.equal(senaName); }); }); + + it('uploads a re-record under a name of its own', () => { + // Clearing a take deletes its mediafile but not the audio cached under + // its name, so a second attempt at the same segment that reuses the name + // reads back the take the user just discarded (TT-7643). + mountPbt({ segments: SEGMENTS, stepLanguage: 'Hebrew|he' }); + waitForPbtReady(); + startRecordingPass(); + recordAndSettle(1); + + cy.get(PBT.clear).click(); + waitForRecorderIdle(); + expectRecordEnabled(); + recordAndSettle(2); + + cy.then(() => { + const [first, second] = postedTakes(); + expect(first?.parsedSegments, 'same segment both times').to.deep.equal( + second?.parsedSegments + ); + expect( + second?.originalFile, + 'the retake does not reuse the cleared take name' + ).to.not.equal(first?.originalFile); + }); + }); }); diff --git a/src/renderer/src/components/PassageDetail/carefulSpeech/CarefulSpeechControls.tsx b/src/renderer/src/components/PassageDetail/carefulSpeech/CarefulSpeechControls.tsx index be1aa9fb8..172fd0613 100644 --- a/src/renderer/src/components/PassageDetail/carefulSpeech/CarefulSpeechControls.tsx +++ b/src/renderer/src/components/PassageDetail/carefulSpeech/CarefulSpeechControls.tsx @@ -364,6 +364,7 @@ export default function CarefulSpeechControls({ /> {phase === 'recorded' && !readOnly && ( diff --git a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts index 1fb961201..5678e539a 100644 --- a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts +++ b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts @@ -51,8 +51,12 @@ describe('guidedPhraseRecord config', () => { ArtifactTypeSlug.PhraseBackTranslation, NamedRegions.BackTranslation ); - expect(config.buildFilenamePostfix(0, 2)).toBe('backtranslation1_v2'); - expect(config.buildFilenamePostfix(1, 2)).toBe('backtranslation2_v2s1'); + expect( + config.buildFilenamePostfix({ unitIndex: 0, sourceVersion: 2 }) + ).toBe('backtranslation1_v2'); + expect( + config.buildFilenamePostfix({ unitIndex: 1, sourceVersion: 2 }) + ).toBe('backtranslation2_v2s1'); }); it('buildFilenamePostfix separates the languages of the same segment', () => { @@ -66,16 +70,46 @@ describe('guidedPhraseRecord config', () => { ArtifactTypeSlug.PhraseBackTranslation, NamedRegions.BackTranslation ); - expect(config.buildFilenamePostfix(0, 1, 'seh')).toBe( - 'backtranslation1_v1_seh' - ); - expect(config.buildFilenamePostfix(0, 1, 'he')).toBe( + const parts = { unitIndex: 0, sourceVersion: 1 }; + expect( + config.buildFilenamePostfix({ ...parts, languageBcp47: 'seh' }) + ).toBe('backtranslation1_v1_seh'); + expect(config.buildFilenamePostfix({ ...parts, languageBcp47: 'he' })).toBe( 'backtranslation1_v1_he' ); - expect(config.buildFilenamePostfix(1, 1, 'he')).toBe( - 'backtranslation2_v1s1_he' - ); + expect( + config.buildFilenamePostfix({ + unitIndex: 1, + sourceVersion: 1, + languageBcp47: 'he', + }) + ).toBe('backtranslation2_v1s1_he'); // Steps with no configured language keep the names they always had. - expect(config.buildFilenamePostfix(0, 1)).toBe('backtranslation1_v1'); + expect(config.buildFilenamePostfix(parts)).toBe('backtranslation1_v1'); + }); + + it('buildFilenamePostfix separates one attempt at a segment from the next', () => { + // Clearing a take deletes its mediafile but not the audio cached under + // its name, so a re-record that reused the name played the discarded take + // back (TT-7432). + const config = phraseBackTranslateConfig( + ArtifactTypeSlug.PhraseBackTranslation, + NamedRegions.BackTranslation + ); + const parts = { unitIndex: 0, sourceVersion: 1, languageBcp47: 'seh' }; + expect(config.buildFilenamePostfix({ ...parts, takeToken: 'aaa' })).toBe( + 'backtranslation1_v1_seh_taaa' + ); + expect( + config.buildFilenamePostfix({ ...parts, takeToken: 'aaa' }) + ).not.toEqual(config.buildFilenamePostfix({ ...parts, takeToken: 'bbb' })); + // Careful Speech records per clause and needs the same separation. + expect( + CAREFUL_SPEECH_CONFIG.buildFilenamePostfix({ + unitIndex: 1, + sourceVersion: 3, + takeToken: 'zzz', + }) + ).toBe('carefulspeech2_v3_tzzz'); }); }); diff --git a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts index b582628bf..a309e0c0a 100644 --- a/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts +++ b/src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts @@ -23,6 +23,23 @@ export interface IGuidedPhraseRecordControlStrings { resetConfirmBoundaries?: string; } +/** What distinguishes one guided-phrase take from another. */ +export interface GuidedPhraseFilenameParts { + /** 0-based segment/clause index. */ + unitIndex: number; + /** Version of the vernacular being recorded against. */ + sourceVersion: number; + /** Step language, for steps that can have a sibling step in another one. */ + languageBcp47?: string; + /** + * When this attempt was made, distinguishing it from the ones it replaces. + * Clearing a take deletes its mediafile but not the audio cached under its + * name, so without this the next attempt at the same segment reads back the + * discarded one. + */ + takeToken?: string; +} + export interface GuidedPhraseRecordConfig { namedRegion: NamedRegions; defaultArtifactSlug: ArtifactTypeSlug; @@ -46,20 +63,15 @@ export interface GuidedPhraseRecordConfig { /** Persist segment map on vernacular named regions (false for Retell). */ persistSegments: boolean; /** - * Filename postfix for a unit at `unitIndex` (0-based) on `sourceVersion`. + * Filename postfix for one take. * * The result has to be unique per take, not just pretty: the uploaded name * becomes the media object's name, and `dataPath` resolves a mediafile's * audioUrl to `/media/`. Two takes uploaded under one * name therefore share a single cached file, and whichever was cached first - * is what plays for both. `languageBcp47` is passed for the steps that can - * have a sibling step over the same audio in another language (TT-7643). + * is what plays for both (TT-7643). */ - buildFilenamePostfix: ( - unitIndex: number, - sourceVersion: number, - languageBcp47?: string - ) => string; + buildFilenamePostfix: (opts: GuidedPhraseFilenameParts) => string; } const carefulSpeechBoundaryDefaults = { @@ -81,8 +93,10 @@ export const CAREFUL_SPEECH_CONFIG: GuidedPhraseRecordConfig = { containerId: 'careful-speech', requireBoldWorkflow: true, ...carefulSpeechBoundaryDefaults, - buildFilenamePostfix: (unitIndex, sourceVersion) => - `carefulspeech${unitIndex + 1}_v${sourceVersion}`, + buildFilenamePostfix: ({ unitIndex, sourceVersion, takeToken }) => + `carefulspeech${unitIndex + 1}_v${sourceVersion}${ + takeToken ? `_t${takeToken}` : '' + }`, }; export function phraseBackTranslateConfig( @@ -106,13 +120,19 @@ 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; + const lang = languageBcp47 ? `${unit}_${languageBcp47}` : unit; + return `${lang}${takeToken ? `_t${takeToken}` : ''}`; }, }; } From 9197ba14630dc4ba9c90fc9ae0f05f91aac04eb8 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Tue, 1 Sep 2026 17:41:14 -0400 Subject: [PATCH 2/3] TT-7666 fix: show one task per segment in the PBT Transcribe list A segment can carry several takes - one saved while rowData had not caught up, an upload retried, an offline row merged back, or a boundary adjusted and the segment re-recorded. The phrase step itself only ever shows the newest (pickLatestGuidedOutputRow), but the Transcribe task list was built from artifact type and step language alone, so every take a segment had ever carried arrived as its own task and superseded takes read as extra work. latestTakePerSourceSegment collapses phrase-segment takes to the newest per sourceMedia + sourceSegments, sharing its isNewerTake rule with pickLatestGuidedOutputRow so the list and the step cannot disagree. A take whose boundaries no longer match the current segment map is left alone: it is the only take for its own region, and hiding it would hide transcription work already done. Co-Authored-By: Claude Opus 5 (1M context) --- docs/adr/0009-phrase-bt-language-scoping.md | 1 + .../PassageDetailTranscribe.test.tsx | 72 +++++++++++-- .../PassageDetail/PassageDetailTranscribe.tsx | 1 + .../carefulSpeech/matchesGuidedOutputRow.ts | 15 ++- .../src/context/TranscriberContext.tsx | 21 +++- .../crud/latestTakePerSourceSegment.test.ts | 101 ++++++++++++++++++ .../src/crud/latestTakePerSourceSegment.ts | 60 +++++++++++ 7 files changed, 254 insertions(+), 17 deletions(-) create mode 100644 src/renderer/src/crud/latestTakePerSourceSegment.test.ts create mode 100644 src/renderer/src/crud/latestTakePerSourceSegment.ts diff --git a/docs/adr/0009-phrase-bt-language-scoping.md b/docs/adr/0009-phrase-bt-language-scoping.md index a4cfd300c..1840c20ab 100644 --- a/docs/adr/0009-phrase-bt-language-scoping.md +++ b/docs/adr/0009-phrase-bt-language-scoping.md @@ -15,3 +15,4 @@ Non-BOLD **Phrase Back Translation** (and **Retell Back Translation**) must supp - Language scoping is an **allowlist** keyed on `artifactStampsStepLanguage` — **Phrase BT only**, the one artifact whose takes carry `languagebcp47`. **Whole Back Translation** is recorded by `PassageDetailItem`, which never stamps a language, and vernacular / Q&A / Retell use the org vernacular — a Transcribe step's `language` is their ASR / font / spell-check language only, so scoping their task lists by it would match nothing - The allowlist is deliberately **narrower than `isPhraseSegmentArtifact`**, which also covers **Careful Speech**. Those two predicates answer different questions: `isPhraseSegmentArtifact` means "uses segmented regions on the waveform" (still true of Careful Speech, and still what `phraseRegions` / `phraseArtifactSlug` key on), while `artifactStampsStepLanguage` means "takes carry a language". Careful Speech is BOLD-only — `CAREFUL_SPEECH_CONFIG` sets `requireBoldWorkflow`, which makes `stepLanguageField` resolve to `undefined`, so its takes are recorded untagged and BOLD keeps shared **`clause`** boundaries rather than per-language ones - **The uploaded file name is part of the discriminator, not decoration.** On the desktop app a take's audio is resolved by name: `useFetchMediaUrl` calls `dataPath(mediafile.audioUrl, PathType.MEDIA)`, which maps to `/media/` and returns that file if it exists — the mediafile id is never consulted, and `store/upload/actions.tsx` stages every online upload into that same folder. Two takes uploaded under one name therefore share one cached file, and the first one cached is what plays for both. `buildFilenamePostfix` must keep every axis that separates one take from another: segment index, source version, **step language**, and a **per-attempt token** (clearing a take deletes its mediafile but not the audio cached under its name, so a re-record that reused the name played the discarded take back — TT-7432). TT-7643 +- **The Transcribe task list shows one take per segment.** A segment can carry several takes — one saved while `rowData` had not caught up, an upload retried, an offline row merged back — and the phrase step itself only ever shows the newest (`pickLatestGuidedOutputRow`). The task list, built from artifact type + language alone, showed all of them, so superseded takes read as extra work. It now collapses to the newest take per `sourceMedia` + `sourceSegments` for phrase-segment artifacts (`latestTakePerSourceSegment`). This does **not** hide a take whose boundaries no longer match the current segment map: it is still the only take for its own region, and hiding it would hide transcription work already done on it. TT-7666 diff --git a/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx index 2e7af2b02..090f8efb7 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx @@ -1,7 +1,14 @@ import React from 'react'; import { render, screen } from '@testing-library/react'; -let captured: { hasPermission?: boolean; curRole?: string } = {}; +let captured: { + hasPermission?: boolean; + curRole?: string; + collapseSegmentTakes?: boolean; +} = {}; + +/** Artifact the step under test is configured for; drives the slug mocks. */ +let artifactSlug = 'vernacular'; const linkedSharedResource = { id: 'sr1', @@ -18,7 +25,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 +40,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 +52,11 @@ jest.mock('../../context/PassageDetailContext', () => ({ jest.mock('../../context/TranscriberContext', () => ({ TranscriberProvider: (props: { curRole?: string; + collapseSegmentTakes?: boolean; children?: React.ReactNode; }) => { captured.curRole = props.curRole; + captured.collapseSegmentTakes = props.collapseSegmentTakes; return <>{props.children}; }, })); @@ -83,20 +98,21 @@ jest.mock('../../crud', () => ({ jest.mock('../../crud/useArtifactType', () => ({ useArtifactType: () => ({ localizedArtifactTypeFromId: () => 'bt', - slugFromId: () => 'vernacular', + slugFromId: () => artifactSlug, }), })); jest.mock('../../crud/artifactTypeSlug', () => ({ ArtifactTypeSlug: { CarefulSpeech: 'carefulspeech' }, artifactStampsStepLanguage: () => false, - isPhraseSegmentArtifact: () => false, + isPhraseSegmentArtifact: (slug: string) => + slug === 'backtranslation' || slug === 'carefulspeech', })); jest.mock('../../crud/related', () => ({ - related: jest.fn(), + related: jest.fn(() => 'mf1'), __esModule: true, - default: jest.fn(), + default: jest.fn(() => 'mf1'), })); jest.mock('../../utils/useStepPermission', () => ({ @@ -155,6 +171,8 @@ import { PassageDetailTranscribe } from './PassageDetailTranscribe'; describe('PassageDetailTranscribe linked note (TT-5873)', () => { beforeEach(() => { captured = {}; + artifactSlug = 'vernacular'; + passageDetailCtx.rowData = []; passageDetailCtx.sharedResource = undefined; passageDetailCtx.mediafileId = 'mf1'; }); @@ -175,3 +193,43 @@ describe('PassageDetailTranscribe linked note (TT-5873)', () => { expect(captured.curRole).toBe('view'); }); }); + +/** + * TT-7643 - a phrase step records one take per segment and shows only the + * newest, but nothing prunes the ones it replaced. The task list was built + * from artifact type and step language alone, so every superseded take + * arrived as its own transcribe task. + */ +describe('PassageDetailTranscribe segment takes (TT-7643)', () => { + beforeEach(() => { + captured = {}; + artifactSlug = 'vernacular'; + passageDetailCtx.rowData = []; + passageDetailCtx.sharedResource = undefined; + passageDetailCtx.mediafileId = 'mf1'; + }); + + const renderForArtifact = (slug: string) => { + artifactSlug = slug; + passageDetailCtx.rowData = [ + { artifactType: 'bt', mediafile: { id: 'take-1' } }, + ] as unknown as typeof passageDetailCtx.rowData; + render(); + }; + + it('collapses a Phrase BT segment to its newest take', () => { + renderForArtifact('backtranslation'); + expect(captured.collapseSegmentTakes).toBe(true); + }); + + it('collapses Careful Speech the same way', () => { + renderForArtifact('carefulspeech'); + expect(captured.collapseSegmentTakes).toBe(true); + }); + + it('leaves a step that is not per-segment listing everything', () => { + // Whole BT, Q&A, Retell: one take for the passage, nothing to collapse. + renderForArtifact('wholebacktranslation'); + expect(captured.collapseSegmentTakes).toBe(false); + }); +}); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.tsx b/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.tsx index 72de9a697..d6dbf7923 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} + collapseSegmentTakes={Boolean(phraseArtifactSlug)} > { - 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.reduce( + (best, row) => + !best || isNewerTake(row.mediafile, best.mediafile) ? row : best, + undefined + ); } /** 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..88edddc96 100644 --- a/src/renderer/src/context/TranscriberContext.tsx +++ b/src/renderer/src/context/TranscriberContext.tsx @@ -33,6 +33,7 @@ import { } from '../crud'; import { mediaFileName } from '../crud/media'; import { mediaMatchesStepLanguage } from '../utils/mediaLanguage'; +import { latestTakePerSourceSegment } from '../crud/latestTakePerSourceSegment'; import StickyRedirect from '../components/StickyRedirect'; import { useSelector } from 'react-redux'; import { useDispatch } from 'react-redux'; @@ -133,9 +134,16 @@ interface IProps { curRole?: string; /** Step language. When set (and not `und`), only media tagged with it become tasks. */ stepLanguageBcp47?: string; + /** + * Set for the phrase steps (Phrase BT, Careful Speech), whose takes are one + * per segment of the vernacular. Only the newest take of a segment becomes a + * task; see {@link latestTakePerSourceSegment}. + */ + collapseSegmentTakes?: boolean; } const TranscriberProvider = (props: IProps) => { - const { artifactTypeId, curRole, stepLanguageBcp47 } = props; + const { artifactTypeId, curRole, stepLanguageBcp47, collapseSegmentTakes } = + 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)); + if (collapseSegmentTakes) m = latestTakePerSourceSegment(m); setPlanMedia(m); planMediaRef.current = m; // eslint-disable-next-line react-hooks/exhaustive-deps - }, [mediafiles, devPlan, artifactId, stepLanguageBcp47, pasId, memory]); + }, [ + mediafiles, + devPlan, + artifactId, + stepLanguageBcp47, + collapseSegmentTakes, + pasId, + memory, + ]); const setRows = (rowData: IRowData[]) => { setState((state: ICtxState) => { diff --git a/src/renderer/src/crud/latestTakePerSourceSegment.test.ts b/src/renderer/src/crud/latestTakePerSourceSegment.test.ts new file mode 100644 index 000000000..e663f6383 --- /dev/null +++ b/src/renderer/src/crud/latestTakePerSourceSegment.test.ts @@ -0,0 +1,101 @@ +import { MediaFileD } from '../model'; +import { latestTakePerSourceSegment } from './latestTakePerSourceSegment'; + +/** + * TT-7666 - the Transcribe task list was built from artifact type (and, since + * TT-7557, step language) alone, so every take a segment had ever carried + * arrived as its own task. Superseded takes read as extra work to transcribe, + * and the transcriber could not tell which one the phrase step is showing. + */ + +const take = ( + id: string, + seg: { start: number; end: number } | string | null, + dateCreated: string, + sourceMedia = 'mf-vern' +): MediaFileD => + ({ + id, + type: 'mediafile', + attributes: { + dateCreated, + sourceSegments: + seg === null + ? null + : typeof seg === 'string' + ? seg + : JSON.stringify({ ...seg, label: '' }), + }, + relationships: { + sourceMedia: { data: { type: 'mediafile', id: sourceMedia } }, + }, + }) as unknown as MediaFileD; + +const ids = (media: MediaFileD[]) => media.map((m) => m.id); + +describe('latestTakePerSourceSegment', () => { + it('keeps only the newest take of a segment', () => { + const media = [ + take('old', { start: 0, end: 3 }, '2026-08-01T10:00:00Z'), + take('new', { start: 0, end: 3 }, '2026-08-02T10:00:00Z'), + take('other-segment', { start: 3, end: 6 }, '2026-08-01T10:00:00Z'), + ]; + expect(ids(latestTakePerSourceSegment(media))).toEqual([ + 'new', + 'other-segment', + ]); + }); + + it('keeps the newest whichever order they arrive in', () => { + const media = [ + take('new', { start: 0, end: 3 }, '2026-08-02T10:00:00Z'), + take('old', { start: 0, end: 3 }, '2026-08-01T10:00:00Z'), + ]; + expect(ids(latestTakePerSourceSegment(media))).toEqual(['new']); + }); + + it('breaks a dateCreated tie the way the phrase step does', () => { + const media = [ + take('aaa', { start: 0, end: 3 }, '2026-08-01T10:00:00Z'), + take('zzz', { start: 0, end: 3 }, '2026-08-01T10:00:00Z'), + ]; + // isNewerTake, which the phrase step's own pick also calls. + expect(ids(latestTakePerSourceSegment(media))).toEqual(['zzz']); + }); + + it('treats the same segment on a different vernacular as its own', () => { + const media = [ + take('v1-take', { start: 0, end: 3 }, '2026-08-01T10:00:00Z', 'mf-v1'), + take('v2-take', { start: 0, end: 3 }, '2026-08-02T10:00:00Z', 'mf-v2'), + ]; + expect(ids(latestTakePerSourceSegment(media))).toEqual([ + 'v1-take', + 'v2-take', + ]); + }); + + it('ignores boundary noise below the display precision', () => { + const media = [ + take('old', { start: 0, end: 3.001 }, '2026-08-01T10:00:00Z'), + take('new', { start: 0, end: 3.002 }, '2026-08-02T10:00:00Z'), + ]; + expect(ids(latestTakePerSourceSegment(media))).toEqual(['new']); + }); + + it('leaves media that are not per-segment takes alone', () => { + const media = [ + take('vernacular', null, '2026-08-01T10:00:00Z'), + take('whole-bt', '{}', '2026-08-02T10:00:00Z'), + take('unparseable', 'not json', '2026-08-03T10:00:00Z'), + ]; + expect(ids(latestTakePerSourceSegment(media))).toEqual([ + 'vernacular', + 'whole-bt', + 'unparseable', + ]); + }); + + it('returns an empty list unchanged', () => { + expect(latestTakePerSourceSegment([])).toEqual([]); + }); +}); diff --git a/src/renderer/src/crud/latestTakePerSourceSegment.ts b/src/renderer/src/crud/latestTakePerSourceSegment.ts new file mode 100644 index 000000000..83aec2770 --- /dev/null +++ b/src/renderer/src/crud/latestTakePerSourceSegment.ts @@ -0,0 +1,60 @@ +import { MediaFile } from '../model'; +import { related } from './related'; + +/** + * Collapse guided-phrase takes to one per segment: the newest. + * + * A segment can end up with several takes if the user re-records - nothing prunes the superseded + * ones - and only the newest should be shown. Use this filter to avoid showing the dead extra takes, + * as happened in the Transcribe task list before TT-7666. + * + * Takes whose `sourceSegments` cannot be read are left alone: they are not + * per-segment takes, so there is nothing to collapse them against. + */ + +/** `start|end` at 2dp, or undefined when this is not a per-segment take. */ +function segmentKey(seg: string | undefined | null): string | undefined { + if (!seg) return undefined; + try { + const parsed = JSON.parse(seg) as { start?: number; end?: number }; + if (typeof parsed?.start !== 'number' || typeof parsed?.end !== 'number') { + return undefined; + } + return `${parsed.start.toFixed(2)}|${parsed.end.toFixed(2)}`; + } catch { + return undefined; + } +} + +/** + * Which of two takes of a segment supersedes the other: the later + * `dateCreated`, and on a tie the higher id, so the answer is stable whatever + * order the rows arrive in. `pickLatestGuidedOutputRow` shows what this keeps, + * so it has to agree - it calls this. + */ +export function isNewerTake( + candidate: MediaFile | undefined, + incumbent: MediaFile | undefined +): boolean { + const dc = candidate?.attributes?.dateCreated ?? ''; + const di = incumbent?.attributes?.dateCreated ?? ''; + if (dc !== di) return dc > di; + return (candidate?.id ?? '') > (incumbent?.id ?? ''); +} + +export function latestTakePerSourceSegment( + media: T[] +): T[] { + const winners = new Map(); + const perSegment = new Set(); + for (const m of media) { + const seg = segmentKey(m.attributes?.sourceSegments); + if (seg === undefined) continue; + perSegment.add(m); + const key = `${related(m, 'sourceMedia') ?? ''}|${seg}`; + const held = winners.get(key); + if (!held || isNewerTake(m, held)) winners.set(key, m); + } + const kept = new Set(winners.values()); + return media.filter((m) => !perSegment.has(m) || kept.has(m)); +} From 29a27278d509106b8433ca2a7203ab5bcc1ec065 Mon Sep 17 00:00:00 2001 From: Noel Chou Date: Tue, 1 Sep 2026 20:43:10 -0400 Subject: [PATCH 3/3] TT-7666 fix: do not collapse takes whose vernacular cannot be read latestTakePerSourceSegment keyed on `related(m, 'sourceMedia') ?? ''`, so every take with an unresolved sourceMedia shared one key per boundary pair and all but the newest dropped out of the task list. Boundaries alone do not make two takes the same segment - without a sourceMedia they could be cut from different vernaculars - so those takes are now left alone, the same as takes with unreadable sourceSegments. Also retags three test comments that still cited TT-7643 for behavior that belongs to TT-7432 and TT-7666. Both from the Copilot review. Co-Authored-By: Claude Opus 5 (1M context) --- ...etailGuidedPhraseRecord.stepScope.test.tsx | 2 +- .../PassageDetailPhraseBackTranslate.cy.tsx | 2 +- .../PassageDetailTranscribe.test.tsx | 4 ++-- .../crud/latestTakePerSourceSegment.test.ts | 20 ++++++++++++++++--- .../src/crud/latestTakePerSourceSegment.ts | 9 +++++++-- 5 files changed, 28 insertions(+), 9 deletions(-) diff --git a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx index ab34f1e69..199fb9be3 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx @@ -284,7 +284,7 @@ describe('PassageDetailGuidedPhraseRecord - step scope (TT-7643)', () => { // name, and the next attempt at the same segment in the same language // built the very same name - so `dataPath` handed the new take the old // take's file and the recording the user had just discarded played back - // (TT-7643). + // (TT-7432). await mountAndSettle(); const firstAttempt = controlsProps?.defaultFilename as string; expect(firstAttempt).toBeTruthy(); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx b/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx index 65f308f6a..310304ad2 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx @@ -336,7 +336,7 @@ describe('PBT language scoping', () => { it('uploads a re-record under a name of its own', () => { // Clearing a take deletes its mediafile but not the audio cached under // its name, so a second attempt at the same segment that reuses the name - // reads back the take the user just discarded (TT-7643). + // reads back the take the user just discarded (TT-7432). mountPbt({ segments: SEGMENTS, stepLanguage: 'Hebrew|he' }); waitForPbtReady(); startRecordingPass(); diff --git a/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx b/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx index 090f8efb7..929f34323 100644 --- a/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx +++ b/src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx @@ -195,12 +195,12 @@ describe('PassageDetailTranscribe linked note (TT-5873)', () => { }); /** - * TT-7643 - a phrase step records one take per segment and shows only the + * TT-7666 - a phrase step records one take per segment and shows only the * newest, but nothing prunes the ones it replaced. The task list was built * from artifact type and step language alone, so every superseded take * arrived as its own transcribe task. */ -describe('PassageDetailTranscribe segment takes (TT-7643)', () => { +describe('PassageDetailTranscribe segment takes (TT-7666)', () => { beforeEach(() => { captured = {}; artifactSlug = 'vernacular'; diff --git a/src/renderer/src/crud/latestTakePerSourceSegment.test.ts b/src/renderer/src/crud/latestTakePerSourceSegment.test.ts index e663f6383..26d55d842 100644 --- a/src/renderer/src/crud/latestTakePerSourceSegment.test.ts +++ b/src/renderer/src/crud/latestTakePerSourceSegment.test.ts @@ -26,9 +26,9 @@ const take = ( ? seg : JSON.stringify({ ...seg, label: '' }), }, - relationships: { - sourceMedia: { data: { type: 'mediafile', id: sourceMedia } }, - }, + relationships: sourceMedia + ? { sourceMedia: { data: { type: 'mediafile', id: sourceMedia } } } + : {}, }) as unknown as MediaFileD; const ids = (media: MediaFileD[]) => media.map((m) => m.id); @@ -95,6 +95,20 @@ describe('latestTakePerSourceSegment', () => { ]); }); + it('leaves a take whose vernacular cannot be read alone', () => { + // Boundaries alone do not make two takes the same segment - without a + // sourceMedia they could be cut from different vernaculars, and collapsing + // them would drop a task that is really someone else's work. + const media = [ + take('no-source-old', { start: 0, end: 3 }, '2026-08-01T10:00:00Z', ''), + take('no-source-new', { start: 0, end: 3 }, '2026-08-02T10:00:00Z', ''), + ]; + expect(ids(latestTakePerSourceSegment(media))).toEqual([ + 'no-source-old', + 'no-source-new', + ]); + }); + it('returns an empty list unchanged', () => { expect(latestTakePerSourceSegment([])).toEqual([]); }); diff --git a/src/renderer/src/crud/latestTakePerSourceSegment.ts b/src/renderer/src/crud/latestTakePerSourceSegment.ts index 83aec2770..07046afa2 100644 --- a/src/renderer/src/crud/latestTakePerSourceSegment.ts +++ b/src/renderer/src/crud/latestTakePerSourceSegment.ts @@ -9,7 +9,10 @@ import { related } from './related'; * as happened in the Transcribe task list before TT-7666. * * Takes whose `sourceSegments` cannot be read are left alone: they are not - * per-segment takes, so there is nothing to collapse them against. + * per-segment takes, so there is nothing to collapse them against. So are takes + * with no readable `sourceMedia`: without knowing which vernacular a take was + * cut from, boundaries alone do not say two takes are of the same segment, and + * collapsing on them would drop a task that is really someone else's work. */ /** `start|end` at 2dp, or undefined when this is not a per-segment take. */ @@ -50,8 +53,10 @@ export function latestTakePerSourceSegment( for (const m of media) { const seg = segmentKey(m.attributes?.sourceSegments); if (seg === undefined) continue; + const source = related(m, 'sourceMedia'); + if (!source) continue; perSegment.add(m); - const key = `${related(m, 'sourceMedia') ?? ''}|${seg}`; + const key = `${source}|${seg}`; const held = winners.get(key); if (!held || isNewerTake(m, held)) winners.set(key, m); }