Skip to content

TT-7621 fix: act on a segment click instead of swallowing it - #528

Open
nabalone wants to merge 5 commits into
developfrom
TT-7621_pbt-segment-click-not-swallowed
Open

TT-7621 fix: act on a segment click instead of swallowing it#528
nabalone wants to merge 5 commits into
developfrom
TT-7621_pbt-segment-click-not-swallowed

Conversation

@nabalone

@nabalone nabalone commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

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, 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 fix

The waveform now reports a click distinctly: useWavesurferRegions fires onRegionClicked from handleRegionClick, threaded up through useWaveSurferWSAudioPlayerPassageDetailPlayer as onSegmentClick. A click is deliberate and therefore never overshoot, so the step disarms the swallow when one arrives. Playhead-driven selection still flows through onCurrentRegion unchanged — 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.

  • selection spec: 6 passing / 1 failing (the remaining failure is Record operable during playback, a separate fix)
  • behaviour specs: 13 + 16 green
  • jest WSAudioPlayer / MediaRecord / PassageDetailCarefulSpeech: 41 green

🤖 Generated with Claude Code

@nabalone
nabalone marked this pull request as ready for review August 24, 2026 13:17
@nabalone
nabalone force-pushed the TT-7621_pbt-segment-selection-and-recorder-state branch from 5bef41e to 3f3ee57 Compare August 24, 2026 21:56
@nabalone
nabalone force-pushed the TT-7621_pbt-segment-click-not-swallowed branch from 8f02f20 to 83fb394 Compare August 24, 2026 21:57
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>
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>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 useWaveSurferWSAudioPlayerPassageDetailPlayer.
  • Uses the new click signal in Guided Phrase Record to disarm pendingOvershootSwallowRef so 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.

Comment on lines +290 to +294
onRegionClicked?.({
start: r.start,
end: r.end,
label: r.content?.textContent || '',
});

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in new commit

nabalone and others added 3 commits August 25, 2026 16:05
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>
@nabalone

Copy link
Copy Markdown
Collaborator Author

Note: de-flaking keeps Record off while a clicked segment plays

Pushed 2071f0b — this is a test change only, no product code touched.

That test (added by #527, unchanged by this PR) sampled the DOM every 25 ms for 6 s and failed if any single sample saw the reference audio playing and Record operable. The two flags are owned by different components on unrelated update chains — playing is WSAudioPlayer's own state, Record's operability is the step's allowRecord — so at the start of playback and at its end they flip on separate renders, and a sample can catch both live for a frame with nothing actually wrong. ~240 samples per run made hitting one of those windows a matter of scheduling luck, which is what the intermittent red here was.

It now waits for playback to start, settles 800 ms, and takes a single reading of both flags. Segment 3 runs 0:06–0:09, so that lands clear of both edges with ~2 s of margin even at half speed. Asserting playing in the same reading keeps it meaningful — if playback ended early the test fails there rather than passing on a Record button that is disabled for the wrong reason.

Trade-off: it no longer catches a Record button that is enabled only briefly mid-playback. That was never the reported defect, and a Record button that stays operable through the clicked segment still fails the test.

The tagged DEFECT: Record is operable while a newly clicked segment plays sibling still uses sampleDom and will inherit the same flakiness once its fix lands — worth giving it the same treatment then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants