TT-7690 fix: re-enable Record after navigating to the next segment during playback - #612
Merged
Merged
Conversation
…ayback Next during playback calls playCurrentClause, whose setPlay(true) is gated on !ctrl.isPlaying(); while audio is still playing the guard is true so playback is never re-armed for the new clause. playRegionRef stays cleared, region-out never fires onRegionPlayEnd, phase never reaches recordReady, and Record stays disabled. Test passes for the paused baseline and fails for the playing case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
playCurrentClause only (re)started playback when !ctrl.isPlaying(), which skipped it whenever *any* clause was playing. Pressing Next mid-playback therefore seeked to the new clause (clearing playRegionRef via gotoTime) but never re-armed it, so region-out never fired onRegionPlayEnd, phase never reached recordReady, and the Record button stayed disabled after the new segment finished. Restart region playback for the seeked-to clause unconditionally, except the true re-entrant case (this same clause already playing, measured by the playhead position before the seek) so an in-progress clause is not yanked back to its start. In region-only mode setPlay(true) replays the current segment via handlePlayStatus's wouldReplayRegion path, which re-arms playRegionRef. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new “already playing this clause” guard uses strict start-boundary comparison and can fail at exact clause starts (notably the first clause at 0s), triggering an unnecessary replay in the re-entrant case.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR fixes TT-7690 where, in Phrase Back Translation / Careful Speech, clicking Next while a clause is still playing could leave the Record button disabled after the next clause finishes, by ensuring region playback is (re)armed for the newly selected clause.
Changes:
- Update
playCurrentClauseto restart region playback after seeking to a new clause even when audio is already playing, skipping only the true re-entrant “same clause already playing” case. - Add a regression test suite that asserts
setPlay(true)is invoked when pressing Next both while paused and while playing. - Minor formatting adjustments for readability.
File summaries
| File | Description |
|---|---|
src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx |
Adjusts clause playback logic to re-arm region playback after Next-during-playback so Record can re-enable correctly. |
src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.nextDuringPlayback.test.tsx |
Adds a focused regression test ensuring Next-during-playback triggers a region replay/start for the newly selected clause. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…tion guard) Review (Devin + Copilot) showed the position-based 'already playing this clause' guard is fragile: after a boundary edit (split/combine/undo) reloads the regions mid-playback, the live playhead still sits inside the new region, so the guard reads it as the armed clause, skips setPlay(true), and strands Record — the exact TT-7690 symptom. Copilot separately flagged the strict start-boundary comparison failing at a clause start (e.g. 0s). Drop the heuristic and re-arm unconditionally. In region-only mode setPlay(true) replays the current segment via handlePlayStatus's wouldReplayRegion path, so playRegionRef is armed and region-out fires onRegionPlayEnd. The trade-off (an audible replay-from-start if the currently-playing clause is re-played) is the safe failure mode and is what the boundary-edit callers want anyway; a skipped re-arm is not. Comment expanded to record the trade-off and why the heuristic was rejected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The PR description’s stated behavior does not match the final implementation and should be aligned (either update the description or adjust code to match it).
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.nextDuringPlayback.test.tsx:18
- This comment explains the bug incorrectly: when audio is playing, the condition
!ctrl.isPlaying()evaluates to false (not true), which is whysetPlay(true)was skipped. The current wording is confusing when debugging regressions later.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+862
to
+876
| // Always (re)start region playback for the clause we just seeked to. | ||
| // gotoTime made this clause the current segment and cleared the play | ||
| // region lock (resetPlayingRegion); in region-only mode setPlay(true) | ||
| // replays the current segment even while audio is already playing | ||
| // (WSAudioPlayer.handlePlayStatus `wouldReplayRegion`), re-arming | ||
| // playRegionRef so region-out fires onRegionPlayEnd and Record | ||
| // re-enables. The old `!ctrl.isPlaying()` guard skipped this whenever | ||
| // any clause was playing, so pressing Next mid-playback never armed the | ||
| // new clause and Record stayed disabled after it finished (TT-7690). | ||
| // | ||
| // We restart unconditionally rather than trying to detect the | ||
| // already-playing-this-clause case. The trade-off: if playCurrentClause | ||
| // is ever invoked for the clause that is *already* playing, this yanks | ||
| // it back to its start instead of letting it continue — an audible | ||
| // replay from the top. That is acceptable here because the callers that |
Per Copilot review: when audio is playing, ctrl.isPlaying() is true, so the !ctrl.isPlaying() guard evaluates false and the setPlay(true) block is skipped. The prior comment mis-stated the condition as 'true'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The fix directly addresses the documented root cause with a targeted behavior change and is backed by a focused regression test.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
Problem (TT-7690)
In the Phrase Back Translation / Careful Speech steps, playing a recorded segment and clicking Next while it is still playing selects and plays the next segment — but once that segment finishes, the Record button stays disabled.
Root cause
playCurrentClauseonly (re)started playback when!ctrl.isPlaying(). Pressing Next mid-playback seeks to the new clause (which clearsplayRegionRefviagotoTime→resetPlayingRegion) but, because audio was still playing, the guard skippedsetPlay(true), so the new clause's region was never armed. With no armed region,region-outnever firesonRegionPlayEnd, sophasenever reachesrecordReadyandcurrentClausePlayedstays false →allowRecordstays false → Record stays disabled.Fix
Re-arm region playback for the seeked-to clause unconditionally (drop the
!ctrl.isPlaying()guard). In region-only modesetPlay(true)replays the current segment viahandlePlayStatus'swouldReplayRegionpath even while audio is already playing, soplayRegionRefis re-armed andregion-outfiresonRegionPlayEnd.An earlier revision tried to preserve the old "don't restart an in-progress clause" intent with a position-based guard (skip the restart only when the playhead is already inside the target clause). Review (Devin + Copilot) showed that heuristic is fragile: after a boundary edit (split/combine/undo) reloads the regions mid-playback, the live playhead still sits inside the new region, so the guard would read it as the armed clause, skip the re-arm, and strand Record again — the exact TT-7690 symptom. Unconditional restart is the safe failure mode; the only cost is an audible replay-from-start if the currently-playing clause is re-played, which is what the boundary-edit callers want anyway.
Test plan
PassageDetailGuidedPhraseRecord.nextDuringPlayback.test.tsx: baseline (Next while paused) passes; Next during playback (re)starts playback of the next clause — red on the original guard, green on the fix.npm run typecheckclean.🤖 Generated with Claude Code