TT-7666 fix: PBT Transcribe shows duplicate audio after a segment boundary adjustment - #570
Closed
nabalone wants to merge 14 commits into
Closed
TT-7666 fix: PBT Transcribe shows duplicate audio after a segment boundary adjustment#570nabalone wants to merge 14 commits into
nabalone wants to merge 14 commits into
Conversation
…pauses wavesurfer performs itself (#558) * test: Record must stay off while the arrowed-to segment plays (red) Reported from hand testing: record segment 1, press the right arrow, and segment 2 starts playing with Record still operable, so a take can be recorded over the reference audio. Everywhere else the step prevents exactly that. Not a regression. The same probe fails at every commit back to 6f6f209, the one that introduced the Prev/Next segment arrows - the arrow path has never had coverage, which is why it stayed open while the equivalent click path was tracked as a known defect in the selection spec. The reading follows that click test: wait for playback to start, settle 800ms, then take a single reading of both flags. `playing` is the player's own state and Record's operability is the step's, so at either end of playback the two flip on unrelated renders and a sample can legitimately catch both live for a frame. Segment 2 runs 0:03-0:06, so the settle lands clear of both edges, and asserting `playing` in the same reading keeps a Record button that is disabled for the wrong reason from passing. Red until the fix in the next commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: a clause shorter than the playback-start window must not strand Found by hand testing the fix on this branch, not on develop: a very short last clause auto-played, the audio stopped part way along, and the step was stuck - the pause icon stayed up and Record never came back, so the clause could not be recorded at all. A dead end with no way forward. The cause is the shape of the fix, so it belongs in the branch that introduces it. Telling the seek that starts a clause from the clause finishing by how long playback has been running assumes every clause outlasts that window. #529 made that assumption for the stop signal - "auto-segmenting never produces a clause anywhere near that short" - and the same assumption is wrong here. A clause whose whole span is shorter than the window reports its genuine end inside the window, and the fix discards it as the seek. The clause is 0.2s against a 250ms window, so the test is deterministic rather than timing-dependent. It passes on develop, where the premature park hides the gap by enabling Record the instant playback starts - which is the defect this branch exists to fix. It is committed before the fix so the next commit has to answer it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: do not treat the seek that starts a clause as the clause ending Record segment 1, press the right arrow, and segment 2 auto-played with Record still operable, so a take could be recorded over the reference audio. The listen-then-record flow prevents that everywhere else. Starting a clause seeks twice - playCurrentClause seeks into the region, then wsPlayRegion seeks again to its start - and that leave-and-re-enter emits region-out for the very region being played, around 60ms in. handleRegionPlayEnd read it as the clause finishing, so it marked the clause heard and set 'recordReady' the instant playback began, and Record stayed operable for the whole clause. Still park on that region-out: the spurious +1 advance that follows needs swallowing either way, and the navigation flows are built on it - suppressing the park outright was tried and strands the click and overshoot paths, exactly as #529 predicted. Only the "clause has been heard" half is withheld, judged by how long playback has been running, reusing the window and the reasoning #529 already applies to the stop signal. That window is only meaningful on a clause long enough to outrun it. Playback covers the clause less the seek that starts it, so a short clause reports its genuine end inside the window; discarding that stranded the step outright - playback stopped part way along, Record never returned, and the clause could not be recorded at all. Hand testing hit exactly that on a sliver clause left by auto-segmenting. Below the cut-off the old behavior stands: Record offered as playback starts. That is the defect this fixes, but the clause is over in well under a second, and a brief wrong enable beats a dead end. Noel's call. The cut-off is derived rather than tuned - one window is the arithmetic floor, the second absorbs the lag between audio starting and the play status that timestamps it, which is where a slow machine shows up. Also fixes the click-path defect the selection spec tracked as @known-defect ("Record is operable while a newly clicked segment plays"), which #529 and #528 both left open; its tag is not dropped here because that spec is not part of this change. ADR 0011 gains a section recording that both call sites of the window rest on an assumption that is false for short clauses, that the stop-side hole is currently masked by the premature park this commit removes, and that Piece 2 is what removes the need for a window at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Revert "fix: do not treat the seek that starts a clause as the clause ending" This reverts commit aa9f830b. Withholding "the clause has been heard" until the genuine end made the step depend on that end actually being reported, and it is not reliably. The spurious region-out consumes playRegionRef about 60ms in, so the real region-out at the clause end no longer reaches onRegionPlayEnd; there is no ws.on('pause') listener anywhere, so a stop wavesurfer performs itself has no guaranteed path out either. #529 named this exact trap - "the step depended on a region-out that does not always come" - and this took the same window one level down into it. Two intermittent failures across runs, on normal clauses rather than short ones: Record never re-enabled at all after arrow navigation, and a take was filed under the wrong segment. Both are the premature park being load-bearing for more than Record enablement, which is not something to unpick before a release. Replaced by a gate on the record button alone, which changes no step state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: pin the shared region-playback contract, and the reported stall Guards for the change about to be made in useWaveSurferRegions, which is shared by every waveform in the app. Region-bounded playback - wsPlayRegion - is what Careful Speech plays every clause through, what Mark Verses and Transcribe reach via Prev/Next segment, what PassageDetailItem uses under forceRegionOnly, and what Discuss plays a topic region with. None of those has a harness that mounts a real wavesurfer, so Phrase Back Translate stands in for all of them: these are the closest thing the other steps have to a regression test. Three pass on develop and are there to keep passing: - a segment is heard from its start, not part way in. Starting a segment seeks twice and a spurious region-out re-seeks in between, so today the opening is effectively replayed; removing that must not clip the first syllable. - playback stops at the segment end rather than running into the next, which would play the following segment's audio under this segment's label. - a short last segment reports its stop. One fails on develop, and is the stall found by hand testing: when the last segment ends exactly where the audio does, the pause icon stays up and Record never returns. Seeking to precisely the duration pauses the media element directly (useWaveSurfer wsGoto) and onPlayStatus is only ever raised from the imperative setPlaying, so that pause never reaches the app. The fixture pins durationSec to the last segment's end to get there; a segment merely short but clear of the file end does not reproduce it, which is why the separate describe exists. Not covered, and needing hand testing in the other steps: where the playhead is left after a segment play (Mark Verses edits verse references against it), and anything timed to the pause-and-resume blip that starting a segment produces. Flutter at the start of a clause is deliberately not asserted - Noel's call is that it is tolerable, and pinning it would forbid a fix that is allowed to keep it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: withhold Record for as long as the clause will play Record segment 1, press the right arrow, and segment 2 auto-played with Record still operable, so a take could be recorded over the reference audio. The listen-then-record flow prevents that everywhere else. Starting a clause emits a region-out indistinguishable from the one that ends it - the play seeks to the region start, and with contiguous regions that boundary belongs to the previous region too - so handleRegionPlayEnd parks and marks the clause heard about 60ms in, and Record stays operable for the rest of it. Two event-based fixes were tried and reverted before this one. Both are recorded because the reasons matter more than the code: - Withholding the park. Reverted in 66a9efcf: the navigation flows are built on it firing early, and the genuine end is not reliably reported, so the step stranded with Record disabled and filed a take under the wrong segment. - Gating on the player's own playing state. Starting a clause pauses and resumes playback, so that state flickers for ~140ms and a click landing there was silently refused. Removing the blip made things worse, not better: it turns out to be load-bearing, because it is what makes the end-of-region event arrive at all. Suppressing it stranded the step from a third direction. So don't infer it from events at all. The clause span and the playback rate are both known when playback starts, so how long the audio will run is known too. Record is withheld for exactly that long, released by a timer, or early by a genuine user pause so #529's behaviour there is kept. It degrades safely in every direction: playback cut short returns Record a little late, a sliver clause returns it almost at once, and nothing can withhold it indefinitely. recordBlocked is deliberately a separate prop from allowRecord rather than folded into it. allowRecord is capability: useWavRecorder stops the capture tracks when it goes false, so using it here would drop the microphone at the start of every clause and leave the first click afterwards with nothing to record into. This disables the button, and changes no step state. WSAudioPlayerControls gains getPlaybackRate, which the span calculation needs - at 0.25x a clause takes four times as long, and Mark Verses users work at that rate. Test: the whole main PBT spec passes, 15 of 15, including the three long multi-record tests that failed under both earlier approaches. selection, edit and defects match their develop baselines exactly; every remaining failure there is a pre-existing @known-defect. Two things this does NOT fix, both tagged rather than left silent: - Record is still operable while a *clicked* segment plays, unchanged from develop. Same defect from a different entry point; not yet understood why the span is not withheld on that path. - The stall where the last segment ends exactly where the audio does. Both signals can be missing at once there: the playhead never leaves the region so nothing parks, and seeking to precisely the duration pauses the element directly, which onPlayStatus never hears. With no park and no stop, Record cannot be offered however the button is gated. Needs ADR 0011 Piece 2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: report the pauses wavesurfer performs itself Fixes the stall found by hand testing: the last segment played, the audio stopped part way along, and the step was stuck - pause icon up, Record never offered again, no way to record that segment at all. onPlayStatus was only ever raised from setPlayingx, the imperative setter. So anything wavesurfer did on its own was invisible to the app: `play(start, end)` reaching its stopAtPosition at the end of a segment, wsGoto pausing on a seek to exactly the duration, a media element dropping out of playback. Everything the app asked for was reported; nothing the engine decided was. That is survivable on most segments, because playback overshoots into the next region and a consumer stops it explicitly, which does report. The last segment has nothing to overshoot into. And when it also ends where the audio does, both signals the step could learn from are missing at once - the playhead never leaves the region so the plugin emits no region-out and nothing parks, and the pause is never reported - so currentClausePlayed was never set and Record could not be offered however the button was gated. One listener on wavesurfer's 'pause', which it emits from the media element's own pause event. It only reports: it pauses nothing, so when playback stops is unchanged, and the playingRef guard keeps it to a falling edge so a pause the app asked for is not reported twice. This is the player-level fix #529 named and deferred to its own ticket ("the honest fix is at the player level"). #529's stated reason for not guarding the premature park - that click-started playback "is not region-bounded and reports neither a region-out nor a stop" - is not true of current code: handleRegionClick only seeks, and the play that follows comes from the step's own effect via wsPlayRegion. Credit to a parallel review for catching that; 66a9efcf repeats the claim and is wrong on that point, though its revert stands on its own evidence. Also fixed by it, and untagged here: - selection: "Record is operable while a newly clicked segment plays", open since #528. It was partly a false positive - the test targets the last segment, where the latched pause icon made readSourcePlaying report playback that had finished, so any enabled Record counted as a violation. With the stop reported it is a real assertion, and it passes. Re-read once from the middle of playback rather than sampled across the edges, the same change 2071f0b made to its sibling for the same reason. Test hardening, since every flake on this ticket came from one signal: - readSourcePlaying now reads the step's own playing state via the harness API rather than the play/pause icon. The icon is a proxy that could latch, which is what made four tests flip between runs instead of catching anything. - the ends-at-the-audio-end fixture uses full-size segments. A 0.2s segment at the end of a 6.2s file made the harness itself unreliable - the step never reached it - and the segment's length was never the point. Verified: the new assertion fails at expectRecordEnabled without this listener and passes with it. main 15/15, playback 3/3, edit 16/16, selection 7 passing with only the pre-existing label-disagreement defect left, defects unchanged from its develop baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore: drop the ticket references, and an unused import The ticket this work was branched from describes the PBT hung state, which none of this addresses, so labelling the code with it pointed readers somewhere misleading. The two references this branch introduced are gone; the ones already on develop are untouched. Also removes the unused sampleDom import from the playback spec, left behind when the flutter assertion was dropped (Copilot). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test: assert both halves of the last-segment contract, and label its limits The ends-at-the-audio-end test asserted only that Record eventually appears. That covers the dead end, but expectRecordEnabled retries for 20s, so it is equally satisfied by Record appearing immediately - which is the other defect. It now requires Record to be withheld while the segment plays and then offered, which is the contract a user depends on: listen to the segment, then record it. It is also labelled as a contract statement rather than a repro, because measuring it properly showed it is not a dependable guard. Reverting only the pause listener turns it red; reverting both fixes does not. Whether the boundary emits a region-out at all is nondeterministic here, so on some runs the premature park supplies the parked state and the test goes green without either fix. It will never fail when the behaviour is right, which is why it is worth keeping, but the guards that fail every time are the two arrow tests in the main spec and the clicked-segment test in the selection spec. Corrects an earlier claim of mine in the PR description, which generalised a single measurement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: note that the clause-span rate is always 1 in this step The speed control only renders when PassageDetailPlayer is given allowSpeed or allowZoomAndSpeed, and the guided step passes neither, so nothing can change the playback rate here and getPlaybackRate always returns 1. Dividing by it is for whenever speed is enabled - at 0.25x a clause takes four times as long, and withholding Record for the unscaled span would release it three quarters of the way through the audio - but it has never been exercised at any other rate, because there is no way to reach one from this step. Noel's observation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: withhold Record when the user replays a clause too Review found the gate covered only playback the step starts itself. A user pressing Play to hear a clause again never goes through playCurrentClause, and by then the clause counts as heard, so Record was operable for the whole replay - this branch's own defect from a third entry point. Confirmed by probe: playback ran for 2.9s with Record enabled throughout. The span is now set whenever playback starts, in handlePlayStatusNotify, measured from the playhead rather than the clause start since a replay can begin part way in. It has to go there rather than in beforePlay: the player awaits that hook, and setting state inside it re-renders mid-start and the play never happens at all - which is what my first attempt did, caught only because the new test could not get playback to start. Also documents the other finding rather than fixing it: a pause inside SPURIOUS_STOP_WINDOW_MS never reaches the line that stops withholding Record, so it stays withheld for the rest of the clause span. Clearing it there is not available - inside that window a stop cannot be told apart from the seek that starts the clause, and clearing on the seek reinstates the defect this all exists to fix. The wait is bounded by the clause length, it is the same limitation #529 already has for currentClausePlayed, and reaching it needs a pause within 250ms of a clause the step started itself - unlikely in practice and near enough impossible without a touchscreen (Noel's call). Both found by Devin. Test: 'keeps Record off while the user replays a segment they have heard'. main 15/15, playback 4/4, edit 16/16, selection 7 passing, defects unchanged - every remaining failure is a pre-existing @known-defect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
* TT-7659 TeamActions should be rigid rather than flexible to not affect button visiblity * Project card titles should wrap in two lines before truncating into ellipses * Set the maximum to 4 cards per row instead of 5
* Add layout mode support to WSAudioPlayer and PassageDetail components - Introduced `layoutMode` prop to WSAudioPlayer and PassageDetailPlayer for better mobile responsiveness. - Updated WSAudioPlayer to conditionally render toolbar and controls based on `layoutMode`. - Enhanced PassageDetailMobileDetail to conditionally hide the mobile header based on `hideMobileHeader` prop. - Added new PassageDetailTranscribeMobile component for mobile transcription functionality. - Implemented tests for new transcription actions and components to ensure functionality and reliability. * Refactor Transcriber component to improve box height calculation - Introduced constants for extra height and minimum text box height to enhance readability and maintainability. - Updated box height calculation in state initialization and useEffect to ensure it respects the new constants. - Ensured that the box height does not fall below the minimum threshold, improving the user interface experience. * Update WSAudioPlayer to conditionally render loop and region controls based on allowAutoSegment prop - Modified loop, previous region, and next region nodes to only render when allowAutoSegment is true. - Enhanced tooltip titles to handle potential null values for improved localization support. * Refactor PassageDetailTranscribeMobile to simplify segment handling - Updated segmentsRef to initialize as undefined instead of relying on mediafile attributes. - Adjusted useEffect to set segmentsRef.current to undefined, improving clarity and reducing unnecessary dependencies. * Enhance PassageDetailTranscribeMobile with confirmation dialog for transcription updates - Added a confirmation dialog to notify users when the transcription has been updated by another user. - Implemented state management for handling incoming transcription changes and user confirmations. - Improved user experience by allowing users to accept or refuse updates to their local transcription changes. * Refactor PassageDetailTranscribeMobile to streamline transcription actions - Moved transcription action callbacks into useTranscribeActions for better organization and clarity. - Updated useEffect dependencies to ensure proper handling of media file changes and transcription updates. - Removed redundant callback definitions to enhance code readability and maintainability. * Enhance PassageDetailTranscribeMobile with improved state management and segment handling - Integrated PlayInPlayer context for better media file management. - Updated createMockMemory to accept memoryUpdate for dynamic testing. - Refined segment handling logic to ensure accurate updates and error logging. - Enhanced useEffect hooks to manage media file changes and transcription updates more effectively. * Enhance PassageDetailTranscribeMobile with error handling and save notifications - Updated createMockMemory to accept an optional memoryUpdate parameter for dynamic testing. - Added onSaveCompleted callback to handle save success and failure notifications. - Improved segment change handling to log errors and ensure proper state management during updates. - Enhanced tests to verify error forwarding and save completion behavior in transcription actions. * Enhance PassageDetailTranscribeMobile with artifact type handling and media selection improvements - Integrated artifact type handling to resolve media files based on selected artifact types. - Updated media selection logic to prioritize selected media rows for transcription actions. - Improved state management for media files and workflow steps to enhance user experience. - Refactored useEffect hooks to ensure accurate media file updates and player interactions. * Enhance PassageDetailTranscribeMobile with improved error handling and state management - Added error handling for save failures in transcription actions to prevent unintended state changes. - Updated `handleComplete` to ensure navigation only occurs on successful saves. - Enhanced tests to verify behavior when save operations fail, ensuring proper handling of step completion and navigation. - Refactored `useTranscribeActions` to throw errors on save failures, allowing callers to manage error states effectively. * Enhance PassageDetailTranscribeMobile with permission-based editing controls - Introduced permission checks to disable editing features when the user lacks edit permissions. - Updated text handling functions to prevent changes if the user does not have permission. - Enhanced component rendering logic to conditionally display editing options based on user permissions. - Added tests to verify that editing features are correctly disabled for users without permissions. * Enhance PassageDetailTranscribeMobile with navigation confirmation for unsaved changes - Introduced a NavigationTrigger component to handle navigation confirmation when there are unsaved changes. - Updated the UnsavedContext to include new state properties for managing save requests and tool changes. - Enhanced tests to verify the behavior of saving or discarding changes based on user confirmation during navigation. - Refactored component rendering logic to conditionally display navigation prompts based on unsaved changes. * Enhance PassageDetailTranscribeMobile with project type integration and transcription state management - Added support for project type handling in transcription actions, allowing differentiation between Scripture and General projects. - Implemented logic to advance transcription states based on project type and workflow steps, improving user experience during transcription submissions. - Enhanced tests to verify correct state transitions for various project types and workflow scenarios. - Refactored component logic to utilize new project type context and improve state management for transcription actions. * Update PassageDetailTranscribeMobile to await uncompletedSteps in handleRejectCallback - Modified the handleRejectCallback function to await the uncompletedSteps call, ensuring proper asynchronous handling of transcription state updates. - This change improves the reliability of the rejection process by ensuring that all steps are completed before proceeding with further logic. * Enhance PassageDetailTranscribeMobile with mediafile validation and action button controls - Added checks to disable action buttons and make the textarea readonly when no mediafile exists, improving user experience during transcription. - Updated text handling functions to ensure actions are only permitted when a mediafile is available. - Enhanced tests to verify the behavior of action buttons and submission processes when mediafile is undefined, ensuring robust error handling and state management. * Add unit tests for useProjectSegmentSave and refactor segment update logic - Introduced a new test file for the `useProjectSegmentSave` hook, validating that segments are updated using attribute-scoped operations instead of full record updates. - Refactored the `useProjectSegmentSave` function to utilize `UpdateAttribute` for segment updates, improving performance and clarity. - Enhanced `PassageDetailTranscribeMobile` to manage pending segment saves more effectively, ensuring that concurrent updates do not overwrite stale data. - Updated `useTranscribeActions` to read the latest attributes from memory, preventing stale mediafile snapshots from affecting segment persistence. - Added tests to ensure that the latest attributes are correctly utilized during concurrent saves, enhancing reliability in the transcription process. * Enhance PassageDetailTranscribeMobile with additional unit tests and role-based rendering - Added unit tests to verify the rendering of the Reopen button, readonly textarea, and disabled ASR for Approved and Done media states with appropriate permissions. - Improved role-based rendering logic to ensure correct access and visibility of UI elements based on user permissions and media states. - Refactored media selection logic to handle artifact types and roles more effectively, enhancing user experience during transcription tasks. - Updated state management to ensure accurate handling of mediafile attributes and transcription states. * Implement unit tests for transcription assignment failure scenarios in useTranscribeActions - Added tests to handle cases where transcription save succeeds but assignment fails, ensuring that save notifications are correctly reported without assignment errors. - Refactored the useTranscribeActions function to improve error handling during assignment, logging errors without disrupting the save process. - Enhanced mock functions to simulate unassigned transcriber scenarios and transcription save failures, improving test coverage and reliability. * Enhance PassageDetailTranscribeMobile with AI transcription feature and offline handling - Added support for AI transcription feature with conditional rendering of the ASR button based on team settings and offline status. - Implemented unit tests to verify the visibility of the ASR button under different scenarios, ensuring correct behavior when AI transcription is enabled or disabled. - Refactored component logic to incorporate new props for AI transcription and offline state management, improving user experience during transcription tasks. * Refactor PassageDetailTranscribeMobile for improved language and font handling - Enhanced the component to resolve transcription language and typography settings from step attributes, falling back to project attributes when necessary. - Updated the useEffect hook to include stepSettings and artifactTypeSlug as dependencies, ensuring accurate font data retrieval. - Refactored font data loading logic to improve clarity and maintainability, enhancing the overall user experience during transcription tasks. * Enhance PassageDetailTranscribeMobile with segment persistence and unsaved state management - Introduced new tests to verify segment persistence and unsaved coordination, ensuring that segment edits are correctly registered and handled during transcription. - Implemented logic to manage segment save failures, including reporting errors and blocking navigation when unsaved changes exist. - Refactored the handling of segment updates to improve clarity and maintainability, enhancing the overall user experience during transcription tasks. * Refactor PassageDetailTranscribeMobile tests to use custom stubs for error handling - Replaced direct use of cy.stub().rejects() with a custom createRejectingStub function to avoid unhandled test-code rejections in CI. - Updated multiple test cases to utilize the new stub for simulating save failures, improving test reliability and clarity. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Revert "Potential fix for pull request finding" This reverts commit 8d9ce30. --------- Co-authored-by: Greg Trihus <gtryus@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…age in Mobile. (#560) * TT-7548 Flesh out new user friendly email unverified page * Update the localization of email unverified page * Add fr/pt/ru fallback translations for email unverified screen * Regenerate localization build output for email unverified strings * Make the email icon and header text smaller * Show the unverified email on desktop, not just on the web app
…#563) * Redraw apm-logo so that it is padded with white outline rather than protruding white lines * Remove unreferenced assets * Add genLogoAssets script to generate every app logo assets from the source svg * Regenerate the app logo assets using the script * Give ApmLogo a size prop and the full brand svg, edit AppHead and AboutDialog to use the new size prop * Add favicon.svg to includeAssets and to the manifest icon list for installers that rasterize at their own size * Link both favicons from the index template * Document how to regenerate the logo assets * Remove unnecessary alt check from cypress test * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…555) * TT-6917 force the chosen device to be used (was a preference before) * handle selected device is unplugged * show a message if device is lost * Warn user if preferred microphone is disconnected * don't lose the recording if microphone lost * more devin and more tests * handle mute * more devin edge cases * fix second take * merge strings * fix tests * missed these! * more test changes * audiocontext not defined * this is the PR that never ends * CI Chrome has no mic, so acquire fails the { exact: deviceId } check. I’ll point capture at Chrome’s fake device and make the stream look like a live, matching mic. * tests run locally * the PR from hell
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The fix is narrowly scoped, aligns with the stated defect/root cause, and includes targeted unit/component tests covering the new filtering and boundary plumbing.
Pull request overview
Fixes TT-7666 by preventing PBT Transcribe from listing stale/duplicate Phrase BT takes after a phrase-segment boundary adjustment, by scoping the task list to the current segment boundaries and selecting only the newest take per current segment.
Changes:
- Add
crud/phraseTakes.selectCurrentPhraseTakesto keep the newest take per current phrase segment (and drop takes tied to outdated segments), while preserving passthrough behavior when boundaries are unknown orsourceSegmentsis absent. - Plumb current phrase-segment boundaries from
PassageDetailTranscribeintoTranscriberProvider, applying the filtering at task-list construction time. - Centralize “newest take wins” tie-breaking and add/extend unit/component tests to cover stale-take filtering and boundary plumbing.
File summaries
| File | Description |
|---|---|
| src/renderer/src/crud/phraseTakes.ts | Introduces region parsing/matching and “current takes” selection for phrase segments. |
| src/renderer/src/crud/phraseTakes.test.ts | Adds unit coverage for stale/duplicate take filtering, passthroughs, and tie-breaking. |
| src/renderer/src/context/TranscriberContext.tsx | Applies phrase-take filtering when building the Transcribe task list (using passed phraseRegions). |
| src/renderer/src/components/PassageDetail/PassageDetailTranscribe.tsx | Computes current phrase boundaries for phrase artifacts and passes them into TranscriberProvider. |
| src/renderer/src/components/PassageDetail/PassageDetailTranscribe.test.tsx | Verifies boundary plumbing into TranscriberProvider for phrase artifacts and absence otherwise. |
| src/renderer/src/components/PassageDetail/carefulSpeech/matchesGuidedOutputRow.ts | Reuses shared newest-take comparator to keep selection stable and centralized. |
| src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechCompletion.ts | Reuses shared phrase-region parsing/matching in completion logic. |
Review details
Suppressed comments (1)
src/renderer/src/components/PassageDetail/carefulSpeech/carefulSpeechCompletion.ts:39
regionMatchesClausecurrently parsesstoredSegand then calls a helper that parses it again. Parse once and compare the parsed region to avoid redundant JSON parsing.
if (parseTakeSourceRegion(storedSeg)) {
return takeMatchesRegion(storedSeg, clauseRegion);
}
- Files reviewed: 7/7 changed files
- Comments generated: 2
- 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
+46
| /** 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 | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
New/updated files introduce type-only imports as value imports, which conflicts with the repo’s consistent-type-imports convention and should be corrected before merge.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
src/renderer/src/context/TranscriberContext.tsx:37
IRegionis only referenced in theIPropstype (phraseRegions?: IRegion[]). Import it withimport typeto satisfy@typescript-eslint/consistent-type-importsand avoid a runtime dependency onuseWavesurferRegions.
src/renderer/src/crud/phraseTakes.test.ts:3MediaFileandIRegionare only used as types in this test file; importing them as values can violate@typescript-eslint/consistent-type-importsand adds unnecessary runtime imports. Switch them toimport type.
src/renderer/src/crud/phraseTakes.ts:2MediaFileandIRegionare only used as TypeScript types in this module, but they’re imported as runtime values. This violates the repo’s@typescript-eslint/consistent-type-importsrule and can also pulluseWavesurferRegionsinto the runtime bundle unnecessarily. Useimport typefor both.
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
…/Document) and fails during Conversion (#569) * TT-7585 Apply prettier formatting to upload dialog prop types Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * TT-7585 Hoist upload accept extension and mime tables to module scope Move the extension and MIME lists out of the effect body, drop the no-op map, and add the missing Link, MarkDown and FaithbridgeLink entries. Indexing now falls back to an empty string instead of casting undefined to string, so UploadType.Burrito no longer sets accept to "undefined". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * TT-7585 Add audio-only intellectual property upload string Generated localization files still need to be rebuilt from this XLIFF by running localization/bin/Debug/updateLocalization.exe on Windows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * TT-7585 Restrict AI IP rights upload to audio formats Thread an audioOnly prop from ProvideRights through Uploader, PassageRecordDlg and MediaUpload to MediaUploadContent. When set, the dialog accepts only the Media audio formats and shows the audio-only rights release instructions instead of the audio or visual wording. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * TT-7585 Add developer fallback translations for audio-only upload string Adapt the existing fr, pt, ru, zh, es and id translations of the audio-or-visual upload string to the new audio-only variant, dropping the visual-format wording, so those languages have a fallback before CrowdIn translates the new string. * TT-7585 Regenerate localization files for audio-only upload string Run localization/bin/Debug/updateLocalization.exe to rebuild TranscriberAdmin-en.xlf, model.tsx, reducers.tsx and strings*.json from the updated XLIFF sources. * Fix formatting issues in MediaUploadContent.tsx Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* TT-7431 speaker name chosen mobile Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- select smoke cypress tests - new method to optimize vite - no need for localhost app - full test run on merge - add pwa documentation
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>
nabalone
force-pushed
the
TT-7666_pbt-stale-takes
branch
from
September 3, 2026 13:26
8a2543a to
9104b14
Compare
Collaborator
Author
Collaborator
Author
Collaborator
Author
Collaborator
Author
An error occurred while trying to automatically change base from
TT-7643_pbt-cross-language-audio
to
develop
September 3, 2026 17:06
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 two commits on top.
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 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, 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, 72 tests green;npm run typecheckclean.🤖 Generated with Claude Code