TT-7666, TT-7432 fix: one task per segment in PBT Transcribe, and a fresh name for every take - #566
Closed
nabalone wants to merge 3 commits into
Closed
Conversation
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 -> <offlineData>/media/<basename>), 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
latestTakePerSourceSegment can incorrectly collapse unrelated takes when sourceMedia is missing/unresolved because it falls back to an empty key, potentially dropping tasks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes guided-phrase take collisions and duplicate Transcribe tasks by (1) making every recording attempt upload under a unique filename token and (2) collapsing multiple per-segment takes down to the newest take per segment when building Transcribe tasks for phrase-segment artifacts.
Changes:
- Add
latestTakePerSourceSegmentand wire it intoTranscriberProvider(enabled for phrase-segment artifacts) so Transcribe shows one task per segment/take winner. - Refactor guided-phrase filename postfix building to accept a parts object and append a per-attempt
takeToken, renewed at recording start and held through upload. - Add/extend Jest + Cypress tests and document the decisions in ADR 0009.
File summaries
| File | Description |
|---|---|
| src/renderer/src/crud/latestTakePerSourceSegment.ts | New utility to collapse per-segment guided-phrase takes to the newest per sourceMedia + segment boundaries. |
| src/renderer/src/crud/latestTakePerSourceSegment.test.ts | Unit tests for take collapsing behavior, tie-break rules, and non-segment media passthrough. |
| src/renderer/src/context/TranscriberContext.tsx | Adds collapseSegmentTakes option and applies collapsing after step-language filtering. |
| src/renderer/src/components/PassageDetail/PassageDetailTranscribe.tsx | Enables segment-take collapsing for phrase-segment artifacts when rendering transcribe view. |
| src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx | Verifies collapseSegmentTakes toggles appropriately by artifact slug. |
| src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx | Introduces and manages a per-attempt takeToken, passed into filename postfix builder. |
| src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.ts | Changes postfix API to an options object and appends optional per-attempt token. |
| src/renderer/src/components/PassageDetail/guidedPhraseRecord/types.test.ts | Tests language and per-attempt token uniqueness in postfixes. |
| src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.stepScope.test.tsx | Validates filenames change across attempts and remain stable within one recording. |
| src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx | Cypress coverage for clear + re-record producing a distinct upload name. |
| src/renderer/src/components/PassageDetail/carefulSpeech/matchesGuidedOutputRow.ts | Refactors “latest take” selection to share the same winner rule as collapsing. |
| src/renderer/src/components/PassageDetail/carefulSpeech/CarefulSpeechControls.tsx | Adds an id for the clear button (supports deterministic test selectors). |
| src/renderer/cypress/support/pbtHarness.tsx | Adds selector constant for the clear button and uses waitForRecorderIdle. |
| docs/adr/0009-phrase-bt-language-scoping.md | Records filename and transcribe collapsing decisions for phrase-segment workflows. |
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+50
to
+57
| 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); | ||
| } |
Comment on lines
+197
to
+204
| /** | ||
| * 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(() => { |
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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes align with the stated root causes, reuse a shared “newest take” rule to keep UI behavior consistent, and add targeted automated coverage for both regressions.
Review details
- Files reviewed: 14/14 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both tickets are the same root cause seen from two sides: a guided-phrase take is identified by the name it uploads under, and takes of one segment pile up.
TT-7432 — Careful Speech: deleted recording continues to play after re-recording
The desktop app resolves a mediafile's audio by name:
useFetchMediaUrl→dataPath(audioUrl, PathType.MEDIA)→<offlineData>/media/<basename>, returning that file if it already exists. The mediafile id is never consulted. Clearing a take deletes its mediafile but not the audio cached under its name, so a re-record that reused the name played back the take the user had just discarded.buildFilenamePostfixnow takes a parts object (unitIndex,sourceVersion,languageBcp47,takeToken) instead of positional args, and appends a per-attempt token.dateCreated, not from the token.GuidedPhraseRecordConfig).TT-7666 — PBT Transcribe displays duplicate audio after boundary adjustment
A segment can carry several takes — a boundary adjusted and the segment re-recorded, a save that landed before
rowDatacaught up, an upload retried, an offline row merged back. The phrase step itself only ever shows the newest (pickLatestGuidedOutputRow), but the Transcribe task list was built from artifact type + step language alone, so every take a segment ever carried arrived as its own task — 4 tasks for 2 segments in the ticket's repro.latestTakePerSourceSegmentcollapses phrase-segment takes to the newest persourceMedia+sourceSegments.isNewerTakerule withpickLatestGuidedOutputRow(now refactored to call it), so the task list and the phrase step cannot disagree about which take wins.TranscriberProvidertakes acollapseSegmentTakesprop;PassageDetailTranscribesets it for phrase-segment artifacts only.ADR 0009 records both decisions.
Test plan
latestTakePerSourceSegment.test.ts(new) — collapsing, tie-breaks, non-segment takes untouched, distinctsourceMediakept apart.guidedPhraseRecord/types.test.ts— postfix separates attempts as well as languages; names without a token are unchanged.PassageDetailTranscribe.test.tsx— task list collapses for phrase artifacts, untouched for the others.PassageDetailGuidedPhraseRecord.stepScope.test.tsx— the upload name changes across a clear + re-record.PassageDetailPhraseBackTranslate.cy.tsx— Cypress CT over the real step for the clear/re-record path.tsc --noEmitare green.🤖 Generated with Claude Code