TT-7437 / TT-7666: don't let a recorded segment be altered by nabalone · Pull Request #572 · sillsdev/apm

nabalone · 2026-09-02T21:34:12Z

Two halves of one rule: the user must not be able to alter a segment that already has (or is actively getting) a Careful Speech / Phrase BT recording. Careful Speech and non-BOLD Phrase Back Translation render the same guided component, so both are covered throughout.

TT-7437 — a take belongs to the segment it started on

A click-lock already existed on develop and the bug was reopened anyway. Two holes:

The lock only covered region-clicked. A waveform click is also a seek, so the playhead walked into the clicked segment and region-in moved the selection behind the lock's back. Dragging a boundary consulted the lock not at all.
Everything that files a take (sourceSegments, filename postfix, green mark) read the live selection at save time, so any path that moved the selection mid-record misfiled the audio.

Fix: region-in now honours the lock; the lock takes wavesurfer's own drag/resize flags away for its duration (restoring each region's own flags, so deliberately-fixed regions stay fixed); and the target clause is latched at record-start and read back for all three take-filing reads. Also closed three more routes reviewers found: double-click split, freezing regions created while the lock is up (region-created), and releasing the latch on a mediafile change.

TT-7666 — a recorded segment's boundaries are frozen

Once a segment has a take, its boundaries can't move for as long as the take exists (not only while recording). The old persistence-layer revert wasn't actually catching drags, so this blocks the gesture at the source instead:

useWavesurferRegions gains isSegmentRecorded(sortedIndex) (keyed on sorted index like applyRegionColor).
The resize handles are removed from every boundary a recording depends on — both sides of a shared boundary — recomputed on each color pass, so deleting a take gives them straight back.
region-update / region-updated refuse a boundary touching a recorded segment (backstop for a gesture already under way).
wsAddRegion / wsRemoveSplitRegion refuse to split inside, or merge across, a recorded segment, returning inertly without moving the playhead.
The +/- buttons are disabled over a recorded boundary, and the Add button drops its primary (solid) styling when disabled so it no longer draws attention to an action it will refuse.

The predicate threads guided-record → PassageDetailPlayer → WSAudioPlayer → useWaveSurfer → useWaveSurferRegions; WSAudioPlayer also uses it for the button-disable logic.

Tests

useWavesurferRegions.test.tsx — the recording-in-progress lock (click / region-in / drag / double-click), and the recorded-segment freeze: drag refused on either side of a shared boundary, an unrecorded boundary stays draggable, handles removed, and clicking +/- over a recorded segment adds/removes nothing and doesn't move the playhead.
PassageDetailCarefulSpeech.test.tsx — the take stays filed on its own clause across the whole lifecycle, and the green mark lands on the recorded clause.
segmentBoundaryLocks.ts + test — the pure +/- disable predicates.

Full renderer jest suite green: 183 suites, 1499 passed, 3 skipped. npm run typecheck clean; eslint/prettier clean on changed files (one pre-existing warning unrelated to this change).

Not yet verified by hand in the running app.

🤖 Generated with Claude Code

Failing specs for the reopened bug: a Careful Speech take is filed against whatever segment is selected when the save runs, not the one recording began on. The existing lock assertions pass while the bug reproduces because they check the flag's value, not that anything enforces it. useWavesurferRegions.test.tsx (new) drives the three engine events that move the selection and asserts the lock at onCurrentRegion, the single point they all reach: - region-clicked (tapping another segment) — already blocked, kept as a regression test; - region-in (the playhead entering the tapped segment after the click's seek) — deliberately bypasses the lock today; - region-updated (dragging a segment boundary) — never consulted the lock, and also reshapes the segment being recorded into. PassageDetailCarefulSpeech.test.tsx adds the save-side invariant across the whole take lifecycle. Moving the selection mid-record is already tolerated, but a change landing in the gap between the recorder stopping and the save starting retargets sourceSegments, and the green completion mark follows it onto the wrong clause. 5 of the new assertions fail; the fix follows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Two independent failures let a Careful Speech take land on the wrong clause. The engine-side lock only ever covered region-clicked. A waveform click is also a seek, so the playhead walked into the clicked segment and region-in selected it anyway — the lock was doing nothing the user could see. region-in now honours it: recording forces playback off, so no legitimate playback or overshoot tracking is lost while it is up. Dragging a boundary never consulted the lock at all, and moved both the selection and the neighbour's shared boundary; the lock now takes wavesurfer's own drag/resize flags away for the duration (restoring each region's own flags, so deliberately fixed regions stay fixed), which stops the gesture rather than undoing it and removes the resize handles as the visible cue that segments are held. region-update/-updated keep a guard as the backstop for a drag already in flight. Blocking events alone is not enough, though, because everything that files a take read the *live* selection at save time. The clause is now latched when capture begins and held until the take is stored or discarded, and sourceSegments, the filename postfix and the green completion mark all read it from there. A failed upload keeps the latch so Retry re-files on the same clause; navigating away from a failed take abandons it and releases it. Careful Speech and non-BOLD Phrase Back Translate share this component, so both are covered. Full renderer suite green (1479 passed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The new drag/resize-freeze logic only applies to regions present when the lock effect runs, so regions created/recreated while locked may remain editable and undermine the “no reshaping during lock” guarantee.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes TT-7437 by preventing Careful Speech (and non‑BOLD Phrase Back Translate) takes from being filed under a different segment if the selection changes during recording/saving. It does this by hardening the wavesurfer segment-selection lock and by latching the recording target (clause/region) so save-time metadata always reflects where recording started.

Changes:

Make region-in, region-update, and region-updated honor lockSegmentSelection, and temporarily disable region drag/resize while locked.
Latch the recording target clause/region at capture start and use it for sourceSegments, filename postfix, and optimistic completion coloring until the take is stored/discarded.
Add/extend renderer tests to cover selection-lock behavior and “take belongs to start clause” invariants across lifecycle timing gaps.

File summaries

File	Description
`src/renderer/src/crud/useWavesurferRegions.tsx`	Extends the segment-selection lock to cover `region-in` and boundary drag/resize, and freezes region editing while recording.
`src/renderer/src/crud/useWavesurferRegions.test.tsx`	New unit tests validating the lock blocks click/seek (`region-in`) and boundary drag selection changes.
`src/renderer/src/components/PassageDetail/PassageDetailGuidedPhraseRecord.tsx`	Latches the clause/region at record start and uses it consistently for take filing and UI completion feedback.
`src/renderer/src/components/PassageDetail/PassageDetailCarefulSpeech.test.tsx`	Adds end-to-end lifecycle assertions that takes remain associated with the clause they started on, including timing-gap scenarios.

Review details

Files reviewed: 4/4 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.

  useEffect(() => {
-    lockSegmentSelectionRef.current = lockSegmentSelection ?? false;
+    const locked = lockSegmentSelection ?? false;
+    lockSegmentSelectionRef.current = locked;
+    // Freeze the segment map itself while the lock is up. Blocking the events
+    // keeps the *selection* still, but a take also belongs to a fixed pair of
+    // boundaries, so the segment it is being recorded into must not be
+    // reshaped under it either (TT-7437). Taking wavesurfer's own drag/resize
+    // flags away is what stops the drag rather than undoing it afterwards, and
+    // it removes the resize handles, which is the user's cue that the segments
+    // are held. Each region's own flags are restored on unlock: some are
+    // deliberately fixed (the split preview) and must not come back resizable.
+    if (locked) {
+      dragLockedRegionsRef.current = regions().map((r) => ({
+        region: r,
+        resize: r.resize,
+        drag: r.drag,
+      }));
+      dragLockedRegionsRef.current.forEach(({ region: r }) =>
+        r.setOptions({ resize: false, drag: false })
+      );
+    } else {


Follow-ups from the Copilot and Devin reviews on #572. Double-click splits a segment, and handleRegionDoubleClick never consulted the lock — so a double-click mid-take reshaped the very segment being recorded into. It is now locked with the other boundary edits, covered by a locked/unlocked test pair. The drag/resize freeze only reached regions that existed when the lock went up. At mount the regions plugin is not attached yet, so a waveform that loads with the lock already on — or any reload during it — produced draggable regions. Extracted freezeRegionDrag and called it from region-created too, where every region passes exactly once; it still captures each region's own flags so unlock restores them individually. The latched clause survived a mediafile change. A new source has a different clause list, so the next take would have been filed against a region from the old waveform; the mediafile reset now releases it. Also rewrote the region-updated comment in plain terms — it described the mechanism rather than what the user would see happen to their segments. Devin's remaining flag (the specs drive mocked player callbacks rather than MediaRecord's real event ordering) is accurate and left as is: that gap wants the Cypress CT harness or a manual pass, not a heavier mock of the same non-path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Failing specs for the second half of the "no altering a segment that already has a recording" rule. Recordings are keyed to a segment's exact time range, so once a segment has a recording (Phrase BT or Careful Speech) its boundaries must be frozen — not only while recording is in progress (TT-7437) but for as long as the recording exists. A boundary is shared by two segments, so a drag is refused when EITHER side of it is recorded. The specs cover: - dragging a recorded segment's own boundary; - dragging an unrecorded segment's boundary that is shared with a recorded neighbour (both the end-side and start-side cases); - the negative: a boundary between two unrecorded segments stays draggable even when some other segment nearby is recorded — the freeze is per boundary, not "any recording freezes the whole map"; - the recorded segment's resize handles are removed as the visible cue. The hook gains an `isSegmentRecorded(sortedIndex)` predicate (keyed on sorted index like applyRegionColor, which is how consumers track completion); it is threaded but not yet enforced, so 4 of the 5 new assertions fail. The fix follows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Second half of the "don't let the user alter a segment that already has a recording" rule, alongside TT-7437. A recording is tied to a segment's exact time range, so once a segment has a take (Phrase BT or Careful Speech) its boundaries must be frozen — for as long as the take exists, not only while recording is in progress. The persistence-layer revert (preservesRecordedBoundaries) was not actually catching drags in the app, so a boundary drag on a recorded segment stuck. Rather than lean on an after-the-fact revert, this blocks the gesture at the source and reflects it in the UI. useWavesurferRegions gains an isSegmentRecorded(sortedIndex) predicate (keyed on sorted index like applyRegionColor, which is how consumers already track completion) and enforces it three ways: - the resize handles are taken off every boundary a recording depends on (both sides of a shared boundary), recomputed on each color pass so a deleted take gives them straight back; - region-update / region-updated refuse a boundary touching a recorded segment, as a backstop for a gesture already under way; - wsAddRegion / wsRemoveSplitRegion refuse to split inside, or merge across, a recorded segment — returning inertly without moving the playhead. The predicate is threaded guided-record -> PassageDetailPlayer -> WSAudioPlayer -> useWaveSurfer -> useWaveSurferRegions. WSAudioPlayer also uses it to disable the +/- buttons over a recorded boundary, and the Add button drops its primary (solid) styling when disabled so it no longer draws attention to an action it will refuse. Careful Speech and Phrase BT share the component, so both are covered. Tests: useWavesurferRegions.test.tsx covers the drag refusal (either side of a shared boundary), that an unrecorded boundary stays draggable, handle removal, and that clicking +/- over a recorded segment adds/removes nothing and does not move the playhead. segmentBoundaryLocks holds the pure +/- disable predicates with their own unit test. Full renderer suite green (1499 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The new recorded-boundary resize locking can unintentionally re-enable region resize handles during/after the recording lock, which risks allowing segment reshaping despite the intended protections.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/renderer/src/crud/useWavesurferRegions.tsx:241

When the segment-selection lock lifts, the code restores each region's prior resize/drag flags, but it doesn't re-apply the recorded-segment boundary locks. If applyRecordedResizeLocks() now no-ops during the lock, recorded segments may become resizable after unlocking until the next color pass happens.

      dragLockedRegionsRef.current.forEach(({ region: r, resize, drag }) =>
        r.setOptions({ resize, drag })
      );
      dragLockedRegionsRef.current = [];
    }

Files reviewed: 10/10 changed files
Comments generated: 1
Review effort level: Lite

+  const applyRecordedResizeLocks = () => {
+    const recorded = isSegmentRecordedRef.current;
+    if (!recorded) return;
+    const sorted = sortedRegions();
+    sorted.forEach((r, i) => {
+      if (recorded(i)) {
+        // Both edges frozen — the whole segment is locked.
+        r.setOptions({ resize: false });
+      } else {
+        // Draggable, except a side shared with a recorded neighbour.
+        r.setOptions({
+          resize: true,
+          resizeStart: !recorded(i - 1),
+          resizeEnd: !recorded(i + 1),
+        });
+      }
+    });
+  };


…ease Review follow-ups (Copilot + Devin) on the recorded-resize freeze. The recorded-freeze pass runs on every color update, including while the TT-7437 recording-in-progress lock holds. That lock takes every region's resize handle away; the freeze pass would set resize:true back on the unrecorded regions, handing back a handle mid-record. And on the flip side, after an upload the freeze could run before the lock's release restored the pre-record flags, so the just-recorded segment came back draggable ("recorded boundaries unlock after upload"). Both are the same ordering hazard between two writers of the resize flag. Fixed by making the lock the sole owner while it holds: applyRecordedResizeLocks early-returns when the lock is active, and the lock's release path re-runs it after restoring the snapshot, so the resting recorded/unrecorded state is re-derived once — picking up any take made during the lock. Also guarded the neighbour lookups so the isSegmentRecorded predicate is only ever asked about a real sorted index (no i-1 at the first region or i+1 at the last), matching its documented contract. New test covers the lock-active color pass not re-enabling handles. Devin's other findings need no change: "media reset leaves target latched" is already handled (the reset clears the latch), and "double-clicks split locked segments" is resolved; the test-approach flag stands as previously noted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

🔵 Needs a closer look

It makes deep behavioral changes in the waveform/recording interaction layer that are difficult to fully validate without manual in-app verification despite strong automated test coverage.

Review details

Files reviewed: 10/10 changed files
Comments generated: 0 new
Review effort level: Lite

The recorded-segment freeze used resize:false, which removes wavesurfer's handle elements — and those 2px handle lines are the clearest marker of where a segment ends, so boundaries became hard to see. Keep the handles rendered instead (resize stays true) and freeze the drag per-side with resizeStart/resizeEnd, so a recorded segment's own edges and the shared edge of an unrecorded neighbour are both inert while still visible. Their cursor is reverted from ew-resize to the default so a frozen boundary — the recorded segment's edges and the boundary it shares with an unrecorded neighbour — no longer looks draggable. The drag itself is still refused by the region-update/updated backstop and the resizeStart/resizeEnd gating, so this is purely a visibility/affordance change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Document at the handleSegment revert why it stays: it was TT-7666's original guard but did not reliably catch a boundary drag on a recorded segment in the app, and the root cause was never determined. The real protection is now at the source in useWavesurferRegions (frozen handles, region-update/updated refusal, wsAddRegion/wsRemoveSplitRegion guards, +/- disable), so this revert no longer fires for drag or +/-; it is kept as defense-in-depth for other segment-map writers (resegmentation, future paths) and should be removed only once those are proven blocked at the source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…karound Record that TT-7437 was fixed with a recordingTarget latch (capture the clause at record-start; take-filing reads it, not the live selection) plus extending the selection lock to region-in — aligned with the ADR's "intent is what's missing" thesis, but a fifth entry in the guessing table rather than the source-tag consolidation the ADR proposes. Flags that the latch should be revisited (kept as a save-time invariant, or removed as redundant) when the source-tagged writes land. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Devin review: during an active take the segment is not yet in the recorded set, so the isSegmentRecorded guard on wsAddRegion/wsRemoveSplitRegion did not cover it — the +/- tools (reachable by hotkey even when the buttons are hidden) could still split or merge the segment being recorded. Refuse both while the selection lock is up (recording or saving), which is exactly the case the isSegmentRecorded check misses. Test: Add is inert while locked before the take is saved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

… always visible Three connected fixes from review + testing feedback. 1. Boundary handles never disappear. The recording-in-progress lock used resize:false, which removes wavesurfer's handle elements — and those are the only clear marker of where a segment ends. Ripped that out: a single applyBoundaryEditability pass keeps resize:true always and gates the drag per side with resizeStart/resizeEnd (plus the default cursor on a frozen side), for both the recording lock and recorded segments. freezeRegionDrag and its snapshot/restore machinery are gone. 2. The guards re-enable when a recording is deleted. applyBoundaryEditability now runs from a reactive effect keyed on the lock and the recorded predicate, so removing a take unfreezes its boundary the same way — and on the same signal — as Combine/Split re-enabling. Previously the drag freeze was only re-applied imperatively on a color pass, so a deleted take left the boundary stuck non-draggable. 3. Consistency: one recorded view feeds every guard. The optimistic just-saved set was a ref (non-reactive), so guards that read it reactively (the +/- disable, drag) and those that read completedIndices (Split/Combine) disagreed in the window after a save before rowData caught up. Added an optimisticVersion that bumps on every optimistic change (via addOptimistic/removeOptimistic/ clearOptimistic helpers), making isSegmentRecorded reactive, and Split/Combine now read the same completedIndices-plus-optimistic set (recordedForTools) as the drag/+- guards. The ref stays for synchronous coloring reads. Also applies the lock to +/- created in the previous commit. Tests: hook — handles stay visible while locked, both sides frozen while locked, a boundary re-enables when its recording is removed; component — a just-saved clause is recorded for drag and Combine alike, and both re-enable together on clear. Full crud+components suites green (Mark Verses, a shared-hook consumer that passes neither prop, unaffected). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Devin flag: handleSplit/handleRemoveNextSplit returned true even when the underlying wsAddRegion/wsRemoveSplitRegion refused the edit (recorded segment or recording in progress), so a blocked hotkey reported success. Return the actual result instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

🟡 Changes recommended

The new +/- blocking helpers re-sort/clone region arrays on the hot progress-update path even though callers already provide sorted regions, adding avoidable per-tick overhead.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/renderer/src/components/segmentBoundaryLocks.ts:50

isRemoveBlockedByRecording also clones/sorts regions on every call even though regionBounds is already sorted when passed from WSAudioPlayer (via getSortedRegions). Since this check runs whenever progress changes, removing the redundant sort avoids repeated allocations and O(n log n) work on the hot path.

  if (!isSegmentRecorded || regions.length < 2) return false;
  const sorted = [...regions].sort((a, b) => a.start - b.start);
  for (let i = 0; i < sorted.length - 1; i++) {
    if (Math.abs(progressSec - sorted[i].end) <= tol) {
      return isSegmentRecorded(i) || isSegmentRecorded(i + 1);
    }
  }

Files reviewed: 11/11 changed files
Comments generated: 1
Review effort level: Lite

+/** Sorted index of the segment at the playhead, or -1. */
+export function segmentIndexAtProgress(
+  progressSec: number,
+  regions: IRegion[]
+): number {
+  const sorted = [...regions].sort((a, b) => a.start - b.start);
+  for (let i = 0; i < sorted.length; i++) {
+    const isLast = i === sorted.length - 1;
+    if (
+      progressSec >= sorted[i].start &&
+      (isLast ? progressSec <= sorted[i].end : progressSec < sorted[i].end)
+    ) {
+      return i;
+    }
+  }
+  return -1;
+}


Copilot review: segmentIndexAtProgress and isRemoveBlockedByRecording cloned and sorted the regions array on every call, and they run on every player progress update. The caller already passes regionBounds pre-sorted (getSortedRegions), so require sorted input and iterate directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

🟢 Approval recommended

The locking semantics are implemented at the gesture/source level with cohesive propagation and are backed by targeted unit tests covering the previously missed mutation paths.

Review details

Files reviewed: 11/11 changed files
Comments generated: 0 new
Review effort level: Lite

nabalone and others added 2 commits September 2, 2026 17:21

nabalone requested a lite review from Copilot September 2, 2026 21:34

Copilot started reviewing on behalf of nabalone September 2, 2026 21:34 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

nabalone and others added 3 commits September 3, 2026 12:03

nabalone changed the title ~~TT-7437 fix: keep a Careful Speech take on the segment it was recorded on~~ TT-7437 / TT-7666: don't let a recorded segment be altered Sep 3, 2026

nabalone requested a lite review from Copilot September 3, 2026 18:21

Copilot started reviewing on behalf of nabalone September 3, 2026 18:21 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

nabalone requested a lite review from Copilot September 3, 2026 18:35

Copilot started reviewing on behalf of nabalone September 3, 2026 18:36 View session

Copilot AI reviewed Sep 3, 2026

View reviewed changes

nabalone and others added 3 commits September 3, 2026 14:56

reword comment

7689629

nabalone force-pushed the TT-7437_no-segment-switch-while-recording branch from 1f842dc to 7689629 Compare September 3, 2026 21:04

nabalone and others added 5 commits September 3, 2026 17:59

chatgpt comment rewrites

c984a16

nabalone requested a lite review from Copilot September 4, 2026 14:02

Copilot started reviewing on behalf of nabalone September 4, 2026 14:02 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

nabalone requested a lite review from Copilot September 4, 2026 14:41

Copilot started reviewing on behalf of nabalone September 4, 2026 14:42 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Navigation Menu

Uh oh!

Uh oh!

TT-7437 / TT-7666: don't let a recorded segment be altered - #572

TT-7437 / TT-7666: don't let a recorded segment be altered#572
nabalone wants to merge 15 commits into
developfrom
TT-7437_no-segment-switch-while-recording

nabalone commented Sep 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

nabalone commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TT-7437 — a take belongs to the segment it started on

TT-7666 — a recorded segment's boundaries are frozen

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nabalone commented Sep 2, 2026 •

edited

Loading