fix: keep Record off while the reference audio plays, and report the pauses wavesurfer performs itself by nabalone · Pull Request #558 · sillsdev/apm

nabalone · 2026-08-31T18:23:25Z

What was reported

In Phrase Back Translate: record the first segment, press the right arrow, and the next segment starts playing with the Record button still enabled — so a take can be recorded over the reference audio, which the listen-then-record flow prevents everywhere else.

Hand testing this branch then turned up a second, worse problem: on the last segment the audio stopped part way along, the pause icon stayed up, and Record never came back — no way to record that segment at all.

Not a regression. The first fails at every commit back to 6f6f209, the one that introduced the Prev/Next segment arrows. The arrow path had no test coverage, which is why it stayed open while the equivalent click path was tracked as a known defect.

Root cause

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. handleRegionPlayEnd read it as the clause finishing, so it marked the clause heard about 60ms in and Record stayed operable for the rest of it.

Separately, onPlayStatus was only ever raised from the imperative setPlayingx, so anything wavesurfer did on its own was invisible to the app: play(start, end) reaching stopAtPosition, wsGoto pausing on a seek to exactly the duration. On the last segment there is nothing to overshoot into, so no consumer stopped playback explicitly and nothing reported it.

The fix, in two parts

Withhold Record for the clause's own span (recordBlocked), computed from the segment length and playback rate when playback starts — not inferred from events. Deliberately a separate prop from allowRecord, because useWavRecorder stops the capture tracks when that goes false; using it here would drop the microphone at the start of every clause and leave the first click afterwards with nothing to record into.
Report the pauses wavesurfer performs itself — one ws.on('pause') listener. Report-only; it pauses nothing, so when playback stops is unchanged, and a playingRef guard keeps it to a falling edge so an app-requested pause is not reported twice.

Two event-based approaches were tried and reverted first; the branch keeps them in history because the reasons matter more than the code. Withholding the premature park strands the step (the navigation flows are built on it firing early). Gating on the player's raw playing state flickers for ~140ms, and removing that blip is worse still — it turns out to be load-bearing, since it is what makes the end-of-region event arrive at all.

Part 2 is the player-level fix #529 named and deferred to its own ticket. #529's stated reason for not guarding the 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.

Also closed

selection: Record is operable while a newly clicked segment plays, open since #528. Partly a false positive — the test targets the last segment, where the latched pause icon made readSourcePlaying report playback that had already finished.

Scope note

useWaveSurfer is shared by every waveform (Mark Verses, Transcribe, Discuss, the record steps). The listener is additive and report-only, but it does change behaviour outside PBT: the play/pause button now flips back to Play at the end of a segment play where it could previously stay showing Pause. That was a deliberate call.

This does not address the PBT hung state described in TT-7621 itself — it is a separate item in that family, like #527–#536 and #529.

Test plan

New coverage:

PassageDetailPhraseBackTranslate.cy.tsx — Record stays off while the arrowed-to segment plays; a clause shorter than the playback-start window must not strand.
PassageDetailPhraseBackTranslate.playback.cy.tsx (new) — pins the shared region-playback contract that Careful Speech, Mark Verses, Transcribe and Discuss all rely on, since PBT is the only step with a real-wavesurfer harness.

Which tests actually guard which fix was measured, by reverting both fixes and keeping every test:

Test	with both fixes reverted
main: Record stays off while the arrowed-to segment plays	fails
main: offers Record once a very short last clause has played	fails
selection: keeps Record off while a newly clicked segment plays	fails
playback: all three	pass

That last row deserves saying plainly. offers Record when the last segment ends at the end of the audio has since been strengthened to assert both halves of the contract - Record withheld while the last segment plays, then offered once it finishes - because asserting only that it eventually appears is equally satisfied by it appearing immediately, which is the other defect.

Even so, it is a statement of the contract rather than a repro. 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 kept, but the dependable guards are the three tests above, which fail every time the fixes are removed. The other two playback tests pass either way by design: they are contract guards for the shared engine, protecting the other steps against future changes.

Results: main 15/15 · playback 3/3 · edit 16/16 · selection 7 passing, only the pre-existing label-disagreement defect left · defects unchanged from its develop baseline.

Flakiness was addressed at the root rather than in assertions: every flake traced to readSourcePlaying reading the play/pause icon, a proxy that could latch. It now reads the step's own playing state via the harness API.

Still needs hand testing (no real-player harness exists outside PBT): where the playhead sits after a segment play in Mark Verses, which edits verse references against it, and Prev/Next segment in Transcribe.

🤖 Generated with Claude Code

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>

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>

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>

… 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>

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>

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>

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>

Copilot

Pull request overview

This PR fixes guided Phrase Back Translate (and shared player behavior) so recording cannot start while reference audio is playing, and so the app reliably detects when Wavesurfer ends playback on its own (e.g., bounded play(start, end) stopping at the end of a segment / file).

Changes:

Add a recordBlocked mechanism (distinct from allowRecord) to disable the Record button during reference playback without releasing the microphone stream.
Surface player-driven pauses by listening to Wavesurfer’s 'pause' event and reporting it through existing play-status plumbing.
Add/adjust Cypress CT coverage and harness helpers to pin the shared region-playback contract and reduce flakiness from UI-proxy playing detection.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/renderer/src/crud/useWaveSurfer.tsx	Reports engine-driven pauses via a Wavesurfer `'pause'` listener to keep app play state in sync.
src/renderer/src/components/WSAudioPlayer.tsx	Introduces `recordBlocked` prop gating record start; exposes `getPlaybackRate()` via controls for timing logic.
src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx	Times `recordBlocked` across clause playback (rate-aware) and clears it on real user pauses.
src/renderer/src/components/PassageDetail/carefulSpeech/CarefulSpeechControls.tsx	Threads `recordBlocked` down to the recorder UI.
src/renderer/src/components/MediaRecord.tsx	Threads `recordBlocked` down into `WSAudioPlayer` without dropping mic capability.
src/renderer/src/components/PassageDetail/mobile/MarkVerses/PassageDetailMarkVerses.test.tsx	Updates mocked player controls to include `getPlaybackRate`.
src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.selection.cy.tsx	Converts a known-defect repro into a passing assertion for “Record stays off while clicked segment plays”.
src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.playback.cy.tsx	Adds a new Cypress CT spec pinning the shared region-playback behavior (start position, stop position, last-segment behavior).
src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.cy.tsx	Adds coverage for arrow navigation keeping Record disabled during playback and very-short-last-clause behavior.
src/renderer/cypress/support/pbtHarness.tsx	Makes `readSourcePlaying` prefer harness API playing state (fallback to icon) to avoid latched-icon flakiness.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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>

…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>

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

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>

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>

nabalone · 2026-09-01T13:14:13Z

+      // Reaching it means pausing within 250ms of a clause starting, which
+      // Noel's call is unlikely in practice and near enough impossible without a
+      // touchscreen: the step starts the clause itself, so it needs the pointer
+      // already over Play and a press inside that window.


Flagging this comment block for the reviewer. Is this tolerable for the release? We should be able to fix it after

nabalone and others added 7 commits August 31, 2026 14:20

nabalone requested a lite review from Copilot August 31, 2026 18:23

Copilot started reviewing on behalf of nabalone August 31, 2026 18:24 View session

Copilot AI reviewed Aug 31, 2026

View reviewed changes

Comment thread src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.playback.cy.tsx

nabalone and others added 2 commits August 31, 2026 14:30

nabalone requested a lite review from Copilot August 31, 2026 19:10

Copilot started reviewing on behalf of nabalone August 31, 2026 19:11 View session

Copilot AI reviewed Aug 31, 2026

View reviewed changes

nabalone and others added 2 commits August 31, 2026 19:23

nabalone marked this pull request as ready for review September 1, 2026 01:42

nabalone commented Sep 1, 2026

View reviewed changes

gtryus approved these changes Sep 1, 2026

View reviewed changes

gtryus merged commit cb869c5 into develop Sep 1, 2026
2 checks passed

gtryus deleted the pbt-record-disabled-while-reference-plays branch September 1, 2026 15:04

Navigation Menu

Uh oh!

Uh oh!

fix: keep Record off while the reference audio plays, and report the pauses wavesurfer performs itself - #558

fix: keep Record off while the reference audio plays, and report the pauses wavesurfer performs itself#558
gtryus merged 11 commits into
developfrom
pbt-record-disabled-while-reference-plays

nabalone commented Aug 31, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

nabalone Sep 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

nabalone commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What was reported

Root cause

The fix, in two parts

Also closed

Scope note

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

nabalone Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nabalone commented Aug 31, 2026 •

edited

Loading