TT-7621 fix: act on a segment click instead of swallowing it - #528
Conversation
5bef41e to
3f3ee57
Compare
8f02f20 to
83fb394
Compare
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>
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>
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>
Note: de-flaking
|
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