TT-7666 fix: PBT Transcribe shows duplicate audio after a segment boundary adjustment - #575
Closed
nabalone wants to merge 3 commits into
Closed
TT-7666 fix: PBT Transcribe shows duplicate audio after a segment boundary adjustment#575nabalone wants to merge 3 commits into
nabalone wants to merge 3 commits into
Conversation
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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new parseTakeSourceRegion accepts non-numeric start/end values at runtime, which can lead to incorrect stale/current filtering; adding finite-number guards would make the behavior safe against malformed persisted data.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Addresses TT-7666 by preventing stale Phrase BT / Careful Speech takes (recorded before a segment-boundary adjustment) from appearing as duplicate Transcribe tasks. The approach centralizes “segment match” + “newest take wins” rules and applies them when building the transcribe task list, while preserving pass-through behavior when no trustworthy boundaries are available.
Changes:
- Added
crud/phraseTakeshelpers to parse a take’ssourceSegments, match it to current phrase regions with tolerance, and select the newest take per current segment. - Plumbed current phrase-segment boundaries from
PassageDetailTranscribeintoTranscriberProvider, and filtered plan/passage task media viaselectCurrentPhraseTakes. - Refactored step-side “latest take” selection and boundary matching to reuse the shared helper; added focused Jest tests for both the new selector and the boundary plumbing.
File summaries
| File | Description |
|---|---|
| src/renderer/src/crud/phraseTakes.ts | New shared logic for parsing/matching phrase regions and selecting current/newest takes. |
| src/renderer/src/crud/phraseTakes.test.ts | Unit tests covering stale vs current takes, tolerance edges, tie-breaking, and pass-through rules. |
| src/renderer/src/context/TranscriberContext.tsx | Applies phrase-take filtering when building the transcribe task list. |
| src/renderer/src/components/PassageDetail/PassageDetailTranscribe.tsx | Computes phrase boundaries from the current artifact/media and passes them to TranscriberProvider. |
| src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx | Verifies phrase boundaries are passed only for phrase-segment artifacts. |
| src/renderer/src/components/PassageDetail/carefulSpeech/matchesGuidedOutputRow.ts | Reuses the shared newest-take comparator for stable selection. |
| src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechCompletion.ts | Reuses shared parseTakeSourceRegion + regionsMatch for consistent boundary matching. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 1
- 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
+35
to
+42
| 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; |
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.
Fixes TT-7666 — the PBT Transcribe step listed four recordings to transcribe where the step had two segments.
Stacked on #561 (TT-7643) — review that first; this PR's diff is the three commits on top. (Successor to #570, which could not be reopened after a force-push; same commits,
8a2543aa.)The defect
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 recorded before the move are left answering to a segment that no longer exists.The record step never shows them — it only offers the takes matching the boundaries it is reading — but the Transcribe task list is built from every take there is. Two segments, four tasks.
The fix
selectCurrentPhraseTakes(new,crud/phraseTakes) keeps the newest take of each current segment and drops the takes naming a segment the boundaries have moved away from.PassageDetailTranscribepasses the boundaries it is reading down toTranscriberProvider, which applies it where the task list is built.It hides rather than deletes, and only where there are boundaries to judge against. With none to read — a vernacular whose segments cannot be parsed, an artifact that records no phrase segments, the plan-level task list that has no step context — takes come through untouched, as do takes with no
sourceSegmentsat all (Retell, and anything older than segment maps). Hiding audio on a guess would be worse than a duplicate row.The tolerance on the comparison is inclusive: it is half the 0.1s grid
prettySegmentrounds to, so a boundary rounded to tenths lands exactly on it, and a strict<would have called such a take stale and hidden a recording the UI showed as matching.The segment-matching and newest-take rules now live in one place, reached by both the step (
carefulSpeechCompletion,matchesGuidedOutputRow) and the context layer.Test plan
src/renderer/src/crud/phraseTakes.test.ts— stale/duplicate takes, the no-boundaries and no-sourceSegmentspassthroughs, exactly-on-tolerance and past-tolerance boundaries, newest-take tie-breaking.PassageDetailTranscribe.test.tsx— the boundaries reachTranscriberProviderfor a phrase artifact and are absent otherwise.npx jest src/crud/phraseTakes.test.ts src/components/PassageDetail/PassageDetailTranscribe.test.tsx src/components/PassageDetail/carefulSpeech→ 12 suites, 74 tests green;npm run typecheckclean. Verified green rebased ontodevelopas well (11 suites, 69 tests — the difference is a test file that belongs to TT-7643 fix: name a Phrase BT take for its step language so it cannot collide #561).Reviews already processed on #570
Devin: 0 bugs, 0 flags. Copilot round 1 (redundant double
JSON.parse) fixed in8a2543aa. Copilot round 2 asked forimport typeon type-only imports, citing@typescript-eslint/consistent-type-imports; that rule is set only in the legacysrc/renderer/.eslintrc.cjs, which ESLint 9 ignores in favour of the flateslint.config.mjs, and with noverbatimModuleSyntaxTypeScript elides those imports anyway.🤖 Generated with Claude Code