TT-7621 test: PBT CT harness and defect repro coverage - #527
Conversation
| * can therefore arrive carrying the number the previous writer used — e.g. | ||
| * selecting segment 2 (waveform: 1+1) right after the step selected segment 3 | ||
| * (step: 2) — and an effect watching the index alone never re-runs. | ||
| * |
There was a problem hiding this comment.
0-based vs 1-based currentSegmentIndex — background, and why this change token exists.
The field has two conventions in active use:
| Convention | Where | |
|---|---|---|
| Writes | 1-based (sortedIndex + 1, 0 = whole) |
usePlayerLogic.ts:113 (the waveform), Mark Verses (table row — row 0 is the header) |
| Writes | 0-based clause index | PassageDetailGuidedPhraseRecord (17 call sites) |
| Reads | 1-based | PassageDetailItem.tsx:246 (filename postfix sN), DiscussionList.tsx:107, Mark Verses row lookup |
| Reads | 0-based | useWavesurferRegions.tsx:955 — regarray[defaultRegionIndex]?.start |
Nothing forces agreement: setCurrentSegment(segment, index: number) accepts any number, so each author picked what was natural locally. That is how a genuine move can arrive carrying the number the previous writer used, which is the bug this commit fixes.
Noel's call: unify the numbering after the next release — it touches every caller across Mark Verses, the players and the guided-record step, which is more than we want to move right now.
When that happens, this token becomes unnecessary: with one convention the index is a reliable change signal for this case, and the two dependency lists in PassageDetailGuidedPhraseRecord can go back to watching currentSegmentIndex alone. One caveat to check then — same sorted position with different bounds (after Combine, position 1 spans 0–6 where it spanned 0–3) still repeats the number. No reproduction for it today; the token covers it for free.
That last table row is also a latent off-by-one worth its own ticket: loadRegions ends with onRegionGoTo(regarray[defaultRegionIndex]?.start ?? 0) and applyRegionAtPosition then selects the region at that position — so a region load reading a 1-based value selects the segment after the current one. loadRegionsJson is called by Split, Combine, segment undo and Reset. PBT happens to mask it (an effect there keeps re-asserting its own 0-based index), and I could not produce a user-visible failure from that screen — but Mark Verses and the generic segment player have no such effect.
| // is still read below. | ||
| }, [ | ||
| currentSegmentIndex, | ||
| currentSegmentSeq, |
There was a problem hiding this comment.
This dependency list is the concrete thing that becomes removable once the 0-based / 1-based numbering is unified — see the note on PassageDetailContext.tsx.
After unification (Noel: after the next release), both navigation effects can drop currentSegmentSeq and watch currentSegmentIndex alone, and the 17 setCurrentSegment(region, idx) calls in this file send whatever the agreed convention is.
Worth knowing while reading this effect: the step never uses the number — it derives its own idx from the region via findClauseIndex. The index was only ever here as a change signal, which is why it was so easy for the two numbering schemes to collide silently.
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>
5bef41e to
3f3ee57
Compare
This branch was cut before #527 gained the `@known-defect` tagging, so its copy of the defects spec won the merge and took the tags with it. Every still-broken repro then ran in CI and failed the check. Restores develop's tags on the three defects this branch does not fix, and keeps the pause repro untagged — that one is fixed here, so it should run. Also tags `clicking back to segment 2 leaves the step on segment 3` in the selection spec. It is a known defect (fixed in #536) that #527 missed, so it was failing CI untagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This branch was cut before #527 gained the `@known-defect` tagging, so its copy of the defects spec won the merge and took the tags with it. Every still-broken repro then ran in CI and failed the check. Restores develop's tags on the three defects this branch does not fix, and keeps the loading-hang repro untagged — that one is fixed here, so it should run. Also tags `clicking back to segment 2 leaves the step on segment 3` in the selection spec. It is a known defect (fixed in #536) that #527 missed, so it was failing CI untagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* TT-7621 fix: clear the loading flag when a load is abandoned Leaving a segment while its take was still loading killed the recorder: the status line stuck, and Record stayed disabled on that segment and every later one. Only leaving the step and coming back recovered it, which is what the hung-PBT report describes. MediaRecord holds `loading` until an effect sees `blobReady && originalBlob`. Both abandon paths - mediaId becoming undefined, and an explicit doReset, which this step triggers on every segment change - call reset(), which drops originalBlob. Once that happened mid-load the condition could never be met again, so `loading` stayed true, and the record button is disabled by Boolean(loading). Abandoning a load now clears the flag it set. Kept separate from reset() on purpose: handleLoadAudio calls reset() itself right after setting loading, so clearing it in there would cancel every load immediately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * TT-7621 fix: gate the abandon on a load-only flag (Devin) Devin flagged that abandonLoadInFlight gated on a mirror of `loading`, which the save path sets too (MediaRecord.tsx:522). A doReset arriving mid-save - and this step sets resetMedia from handleClearRecording, which can run during a save - would have cleared the flag and the status text, re-enabling Record and dropping "Saving..." while the save was still running. Correct, and reachable. Replaced the mirror with loadInFlightRef, true only while handleLoadAudio is fetching a take, cleared wherever that load ends. It is also set synchronously rather than in an effect, which was Devin's second point: the [mediaId] and [doReset] effects now see it immediately instead of one commit late. jest MediaRecord + MediaRecord.load: 13 green. The loading-hang repro stays fixed; edit spec 16 green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * TT-7621 test: keep the still-broken defect repros out of CI This branch was cut before #527 gained the `@known-defect` tagging, so its copy of the defects spec won the merge and took the tags with it. Every still-broken repro then ran in CI and failed the check. Restores develop's tags on the three defects this branch does not fix, and keeps the loading-hang repro untagged — that one is fixed here, so it should run. Also tags `clicking back to segment 2 leaves the step on segment 3` in the selection spec. It is a known defect (fixed in #536) that #527 missed, so it was failing CI untagged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This PR is now tests-only for the TT-7621 PBT work.
What is in this PR
--mute-audio, node tasks for CT, pre-bundled harness deps, CT-specific Vite cache)What is not in this PR
The segment-selection state fix that was previously bundled here has been split to:
TT-7621_pbt-follow-segment-change)Notes
develop