TT-7621 fix: act on a segment click instead of swallowing it - #528
Merged
Conversation
This was referenced Aug 22, 2026
nabalone
marked this pull request as ready for review
August 24, 2026 13:17
nabalone
force-pushed
the
TT-7621_pbt-segment-selection-and-recorder-state
branch
from
August 24, 2026 21:56
5bef41e to
3f3ee57
Compare
nabalone
force-pushed
the
TT-7621_pbt-segment-click-not-swallowed
branch
from
August 24, 2026 21:57
8f02f20 to
83fb394
Compare
Base automatically changed from
TT-7621_pbt-segment-selection-and-recorder-state
to
develop
August 24, 2026 22:33
Clicking the segment immediately after the current one did nothing: the playhead snapped back, the label never changed, and the user had to click again. Record also stayed enabled for the segment they were leaving, so a take recorded then went to the old segment. After a segment finishes playing, handleRegionPlayEnd parks and arms pendingOvershootSwallowRef, because playback overshoot into the next region produces a spurious +1 segment change that would otherwise read as navigation (TT-7360). The swallow cannot tell that change apart from the user clicking the next segment, so it ate both. The waveform now reports a click distinctly: useWavesurferRegions fires onRegionClicked from handleRegionClick, threaded up through useWaveSurfer, WSAudioPlayer and PassageDetailPlayer as onSegmentClick. A click is deliberate and so can never be overshoot, so the step disarms the swallow when one arrives. Playhead-driven selection still flows through onCurrentRegion exactly as before, which is what the swallow is there for. Fixes two of the reported behaviours: the ignored first click, and the waveform selection disagreeing with the segment label (the same swallowed click, seen from the other side). Both tests are renamed to what they now assert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nabalone
force-pushed
the
TT-7621_pbt-segment-click-not-swallowed
branch
from
August 25, 2026 12:40
83fb394 to
eadea7b
Compare
This change only stops a segment click being swallowed. Two other defects the same spec covers - Record staying operable while a clicked segment plays, and the waveform selection disagreeing with the segment label - are fixed separately, but their @known-defect tags had been dropped along with the one that is fixed here. CI runs cy:run-ct with grepTags=-@known-defect, so both went red. Restore the tags and the original names on those two, leaving only the click-swallow test in the default run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gtryus
approved these changes
Aug 25, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a Phrase Back Translate (PBT) segment-selection defect where a user click on the next waveform segment could be swallowed by the “overshoot swallow” logic (intended to ignore playhead overshoot), causing the UI/recording target to remain on the previous segment.
Changes:
- Introduces a distinct “region clicked” callback from the wavesurfer region click handler and threads it up through
useWaveSurfer→WSAudioPlayer→PassageDetailPlayer. - Uses the new click signal in Guided Phrase Record to disarm
pendingOvershootSwallowRefso deliberate clicks are not treated like overshoot. - Updates Cypress CT selection specs to reflect the fixed behavior and strengthens assertions around correct take filing.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/renderer/src/crud/useWavesurferRegions.tsx | Emits a dedicated callback on waveform region click (distinct from playhead-driven selection). |
| src/renderer/src/crud/useWaveSurfer.tsx | Threads the new click callback into the regions hook API. |
| src/renderer/src/components/WSAudioPlayer.tsx | Adds an optional onSegmentClick prop and passes it through to the wavesurfer hook. |
| src/renderer/src/components/PassageDetail/PassageDetailPlayer.tsx | Plumbs onSegmentClick down into WSAudioPlayer. |
| src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.selection.cy.tsx | Renames/updates selection specs to assert first-click behavior and correct take attribution. |
| src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx | Disarms the overshoot swallow on deliberate waveform clicks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
onRegionClicked fired for every waveform click, including clicking the already-current region (wasCurrentRegion === true). Its sole consumer disarms pendingOvershootSwallowRef, so a no-op click on the current segment would cancel a swallow that still needs to absorb the imminent +1 overshoot/recorder-mount region-in, letting that spurious advance through as real navigation. Fire onRegionClicked only inside the !wasCurrentRegion branch, where the click actually changes the selection — the only case that must be told apart from playhead-driven selection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t-click-not-swallowed # Conflicts: # src/renderer/src/components/PassageDetail/PassageDetailPhraseBackTranslate.selection.cy.tsx
'keeps Record off while a clicked segment plays' sampled the DOM every 25ms for 6s and failed on any single sample where the reference audio was playing and Record was operable. Those two flags live in different components on unrelated update chains - `playing` is WSAudioPlayer's own state, Record's operability is the step's `allowRecord` - so at the start and the end of playback they flip on separate renders and a sample can catch both live for a frame without anything being wrong. That made the test flaky. It now waits for playback to start, settles for 800ms, and takes a single reading of both flags. Segment 3 runs 0:06-0:09, so that lands well clear of both edges. Asserting `playing` in the same reading keeps it honest: if playback ended early the test fails there instead of passing on a Record button that is disabled for the wrong reason. This trades away detection of a Record button that is enabled only briefly mid-playback, which was never the reported defect; a Record button that stays operable through the clicked segment still fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
Note: de-flaking
|
gtryus
pushed a commit
that referenced
this pull request
Sep 1, 2026
…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>
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.
Fix 2 of the PBT series. Into #527.
The fault
Clicking the segment immediately after the current one sometimes failed to fully change the selected segment, so a take recorded then went to the old segment.
After a segment finishes playing,
handleRegionPlayEndparks and armspendingOvershootSwallowRef, because playback overshoot into the next region produces a spurious +1 segment change that would otherwise read as navigation (TT-7360). The swallow cannot tell that change apart from the user clicking the next segment, so it ate both.The fix
The waveform now reports a click distinctly:
useWavesurferRegionsfiresonRegionClickedfromhandleRegionClick, threaded up throughuseWaveSurfer→WSAudioPlayer→PassageDetailPlayerasonSegmentClick. A click is deliberate and therefore never overshoot, so the step disarms the swallow when one arrives. Playhead-driven selection still flows throughonCurrentRegionunchanged — which is what the swallow exists for.Additive throughout: every layer takes an optional callback, so other players are unaffected.
Result
Two reported behaviours fixed — the ignored first click, and the waveform selection disagreeing with the segment label (the same swallowed click from the other side). Both tests renamed to what they now assert.
WSAudioPlayer/MediaRecord/PassageDetailCarefulSpeech: 41 green🤖 Generated with Claude Code