TT-7621 fix: offer Record once the reference audio stops - #529
Open
nabalone wants to merge 5 commits into
Open
Conversation
This was referenced Aug 22, 2026
Collaborator
Author
|
Claude opened this but I don't think we actually want this change in behavior |
Collaborator
Author
|
Actually, asking the team |
Collaborator
Author
|
We decided that for now we do want to make this change and allow recording after a pause. And then when we raise it to the product owners, if they object we can back it back out |
nabalone
marked this pull request as ready for review
August 24, 2026 21:05
Mounts the real Phrase Back Translate step - real wavesurfer, MediaRecord,
Orbit memory source, UnsavedProvider and upload action chain - so UI-level
faults show up: record button enable/disable, playhead position, waveform
region colours, and which segment a take is actually filed under.
Three things stand in for the real world:
- the microphone, via the existing installRecordingMocks
- the source audio, a generated WAV with silence around each segment boundary
so region-out fires as it does on speech and auto-segment has gaps to find
- the server, via cy.intercept on the mediafile POST, the audio PUT and
fileurl. Every upload's source-segments is captured, so "which segment did
this take land on" is an assertion rather than a guess.
Because the server is ours, putDelayMs / fileurlDelayMs / rowDataLagMs make
the save- and load-in-flight races deterministic instead of flaky.
33 tests pass and pin down current good behaviour. 8 fail on purpose, each a
repro for a defect found while writing them, kept in their own specs so a
green run of the others still means something:
- navigating while a take loads hangs the recorder ("Loading..." sticks and
Record is disabled on every later segment)
- clearing a take mid-upload brings the take back
- pausing the reference playback leaves Record disabled with no way out
- Fewer Segments can produce more segments
- navigation is offered while a take is still unsaved (intermittent)
- a segment click is only half applied: the waveform selection and playhead
move but the step does not follow, so the first click on the next segment is
ignored, the selection and label disagree, Record is operable during
playback, and a take can be filed under the previously selected segment
Also: Cypress now launches Chrome with --mute-audio (these specs play real
audio), component specs get the node tasks so cy.task('log') works, the
harness's deps are pre-bundled to stop a mid-run re-optimize leaving two
copies of React in the AUT, and CT gets its own vite cacheDir so it and the
app dev server stop invalidating each other.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The cypress-tests job runs the whole component suite, so the deliberately-red repro tests turned the check red on this PR. A red check that is expected to be red teaches everyone to ignore the check. Each known-defect test is now tagged @known-defect (per test, not per describe, so a test does not stay excluded once its defect is fixed). cy:run-ct - what CI runs - excludes that tag. Added cy:run-ct-known-defects to run only the repros and cy:run-ct-all to run everything. CI-equivalent run of the four PBT specs: 33 passing, 8 skipped, none failing. When a fix lands it must drop the tag from the test it fixes, or that test stops being run. The fix branches off this one already rename those tests, so their merge conflicts resolve as "take the fix branch's version, without the tag". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nabalone
force-pushed
the
TT-7621_pbt-segment-selection-and-recorder-state
branch
from
August 24, 2026 21:56
5bef41e to
3f3ee57
Compare
Pausing the reference playback part way through left Record disabled with no way forward but replaying the whole clause. Record needs currentClausePlayed and phase 'recordReady', and handleRegionPlayEnd was the only thing that set them - so the step depended on a region-out that does not always come. A user pause has none by definition. The step now treats the reference audio stopping as the signal that the clause has been heard, which is the thing the user actually did. A stop reported within 250ms of playback starting is the seek that started it rather than the audio, so those are ignored; auto-segmenting never produces a clause anywhere near that short. Also fixes the waveform selection and the segment label disagreeing after a segment is selected, which came from the same stranded state. Both tests are renamed to what they now assert. Not fixed here, and still red: Record stays operable while a clicked segment plays. Suppressing the spurious park that causes it turns out to strand the click flow instead, because playback started by clicking a segment is not region-bounded and reports neither a region-out nor a stop - so the honest fix is at the player level, and it is not this commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Devin flagged that clausePlaybackStartedAtRef was only set by playCurrentClause, so playback started any other way - the user pressing Play, which goes through handleBeforeSourcePlay - left the timestamp stale. The 250ms seek-suppression window then compared against an old time, and a stop caused by that start would fall outside the window and mark the clause played before it had been. Correct, and exactly the failure the window exists to prevent. The timestamp is now re-based wherever playback begins: in handleBeforeSourcePlay for a user-initiated play, and on the play-status change for the real start however it was triggered, as well as in playCurrentClause for the seek. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comments and one behaviour-preserving split, no logic change. The seek-suppression window needed explaining rather than naming: starting a clause seeks the playhead to its start, and that seek reports a stop of its own before anything has been heard, so elapsed playback time is what separates it from a real stop such as a user pause. Split `if (playingNow || !recordingPassStartedRef.current) return;` into its two unrelated halves - one is "this is a start, not a stop", the other is "the listen pass has nothing to record" - and said why the recording/saving guard reads refs rather than state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nabalone
force-pushed
the
TT-7621_pbt-record-after-playback-stops
branch
from
August 24, 2026 21:57
4751e03 to
3c433f6
Compare
Base automatically changed from
TT-7621_pbt-segment-selection-and-recorder-state
to
develop
August 24, 2026 22:33
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
1 participant
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.
Fix 3 of the PBT series. Into #527.
The fault
Pausing the reference playback part way through left Record disabled with no way forward but replaying the whole clause.
allowRecordneedscurrentClausePlayedand phase'recordReady', andhandleRegionPlayEndwas the only thing that set them — so the step depended on a region-out that does not always come. A user pause has none by definition.The fix
The step now treats the reference audio stopping as the signal that the clause has been heard — which is the thing the user actually did. A stop reported within 250ms of playback starting is the seek that started it rather than the audio, so those are ignored; auto-segmenting never produces a clause anywhere near that short.
Also fixes the waveform selection and the segment label disagreeing after a segment is selected, which came from the same stranded state.
Deliberately not fixed here
Record stays operable while a clicked segment plays is still red. I tried it on this branch and backed it out: the cause is a spurious park (wavesurfer emits the clicked clause's region-out during the seek, ~10ms in, so the step parks on a segment that is about to play). Suppressing that park does fix Record, but it then strands the click flow entirely — playback started by clicking a segment is not region-bounded and reports neither a region-out nor a stop, so nothing ever re-enables Record. Shipping that would trade a wrong-enabled button for a dead end.
The honest fix is at the player level: a segment click should start region-bounded playback that reports its end like
playCurrentClausedoes. Worth its own ticket — it also means clicking a segment currently plays past it into the following audio.Verification
PassageDetailCarefulSpeech: 24 green (this touches shared guided-record code)🤖 Generated with Claude Code